linux-kernel - Re: [PATCH v2 4/5] ARM: dts: imx7: add GPMI NAND

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 04 May 2017 14:50:56 -0700
From:   Stefan Agner <stefan@...er.ch>
To:     Han Xu <han.xu@....com>
Cc:     dwmw2@...radead.org, computersforpeace@...il.com,
        boris.brezillon@...e-electrons.com, marek.vasut@...il.com,
        richard@....at, cyrille.pitchen@...el.com, robh+dt@...nel.org,
        mark.rutland@....com, shawnguo@...nel.org, kernel@...gutronix.de,
        fabio.estevam@...escale.com,
        "LW@...O-electronics.de" <LW@...o-electronics.de>,
        linux-mtd@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/5] ARM: dts: imx7: add GPMI NAND

On 2017-05-04 12:13, Han Xu wrote:
> On 04/21/2017 08:23 PM, Stefan Agner wrote:
>> Add i.MX 7 GPMI NAND module.
>>
>> Signed-off-by: Stefan Agner <stefan@...er.ch>
>> ---
>>   arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
>> index 843eb379e1ea..9645257638d4 100644
>> --- a/arch/arm/boot/dts/imx7s.dtsi
>> +++ b/arch/arm/boot/dts/imx7s.dtsi
>> @@ -995,5 +995,36 @@
>>   				status = "disabled";
>>   			};
>>   		};
>> +
>> +		dma_apbh: dma-apbh@...00000 {
>> +			compatible = "fsl,imx7d-dma-apbh", "fsl,imx28-dma-apbh";
>> +			reg = <0x33000000 0x2000>;
>> +			interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>> +			interrupt-names = "gpmi0", "gpmi1", "gpmi2", "gpmi3";
>> +			#dma-cells = <1>;
>> +			dma-channels = <4>;
>> +			clocks = <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>> +				<&clks IMX7D_NAND_ROOT_CLK>;
>> +			clock-names = "dma_apbh_bch", "dma_apbh_io";
>> +		};
> Do you need some patches to enable all clks for APBH DMA? Refer to 
> https://patchwork.ozlabs.org/patch/551967/
> 

Oh I see, with the current code and this device tree we actually only
enable the first clock... But it seems to work in practice...

What is interesting, the chapter 5.2.5 System Clocks only describes
APBHDMA, and no GPMI module.

Chapter 9.4 about APBH-Bridge-DMA bridge only specifies "hclk" as clock
sources.

When looking at the table in chapter 5.2.5, hclk is
NAND_USDHC_BUS_CLK_ROOT

Module    Syste Module Clock   Clock Root
APBHDMA   apbhdma.hclk         NAND_USDHC_BUS_CLK_ROOT
 
So my guess is that the actual DMA controller should work fine with
IMX7D_NAND_USDHC_BUS_ROOT_CLK only. It is only the GPMI module which
needs the NAND_ROOT_CLK. My guess is since the APBHDMA controller is
only used for GPMI these days, it has been combined in one module in the
table in chapter 5.2.5.

So we could actually only specify:

clocks = <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>;

Tested with this and NAND seems to work fine.

Also tested accessing APBHDMA register in U-Boot with
IMX7D_NAND_ROOT_CLK disabled, it worked fine.

What do you think? Should we go with this?

This probably even saves power since GPMI's runtime clock management
disables IMX7D_NAND_ROOT_CLK when NAND is idle (according to the
nand_root_clk counter in clk_summary and the root clock register at
0x3038AA00).

--
Stefan

>> +
>> +		gpmi: gpmi-nand@...02000{
>> +			compatible = "fsl,imx7d-gpmi-nand";
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			reg = <0x33002000 0x2000>, <0x33004000 0x4000>;
>> +			reg-names = "gpmi-nand", "bch";
>> +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>> +			interrupt-names = "bch";
>> +			clocks = <&clks IMX7D_NAND_ROOT_CLK>,
>> +				<&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>;
>> +			clock-names = "gpmi_io", "gpmi_bch_apb";
>> +			dmas = <&dma_apbh 0>;
>> +			dma-names = "rx-tx";
>> +			status = "disabled";
>> +		};
>>   	};
>>   };