diff mbox

[v2,4/5] ARM: dts: imx7: add GPMI NAND

Message ID 20170422012338.4635-5-stefan@agner.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Stefan Agner April 22, 2017, 1:23 a.m. UTC
Add i.MX 7 GPMI NAND module.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Han Xu May 4, 2017, 7:13 p.m. UTC | #1
On 04/21/2017 08:23 PM, Stefan Agner wrote:
> Add i.MX 7 GPMI NAND module.
>
> Signed-off-by: Stefan Agner <stefan@agner.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@33000000 {
> +			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/

> +
> +		gpmi: gpmi-nand@33002000{
> +			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";
> +		};
>   	};
>   };
Stefan Agner May 4, 2017, 9:50 p.m. UTC | #2
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@agner.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@33000000 {
>> +			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@33002000{
>> +			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";
>> +		};
>>   	};
>>   };
Han Xu May 4, 2017, 10:59 p.m. UTC | #3
On 05/04/2017 04:50 PM, Stefan Agner wrote:
> 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@agner.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@33000000 {
>>> +			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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F551967%2F&data=01%7C01%7Chan.xu%40nxp.com%7C98493f7e475341dbf67008d49337b441%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=FH9k%2FC4VcVCeyidkRgUJ5Ahu4%2B%2FlDAjXVwO0DY7XMhA%3D&reserved=0
>>
> 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).

It also surprised me how it can work on your side, I remember we made 
the code change for some purpose, please give me some time I can check 
if the doc is wrong.

> --
> Stefan
>
>>> +
>>> +		gpmi: gpmi-nand@33002000{
>>> +			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";
>>> +		};
>>>    	};
>>>    };
Stefan Agner May 25, 2017, 8:22 a.m. UTC | #4
On 2017-05-04 15:59, Han Xu wrote:
> On 05/04/2017 04:50 PM, Stefan Agner wrote:
>> 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@agner.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@33000000 {
>>>> +			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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F551967%2F&data=01%7C01%7Chan.xu%40nxp.com%7C98493f7e475341dbf67008d49337b441%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=FH9k%2FC4VcVCeyidkRgUJ5Ahu4%2B%2FlDAjXVwO0DY7XMhA%3D&reserved=0
>>>
>> 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).
> 
> It also surprised me how it can work on your side, I remember we made 
> the code change for some purpose, please give me some time I can check 
> if the doc is wrong.
> 

Hi Han,

Do you have an update to this?

Otherwise, I will send out a version which only uses
IMX7D_NAND_USDHC_BUS_ROOT_CLK for now. This binding is already defined
and needs no driver change. We can still extend the binding (and
ddriver) when it turns out it is actually required.

--
Stefan

>> --
>> Stefan
>>
>>>> +
>>>> +		gpmi: gpmi-nand@33002000{
>>>> +			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";
>>>> +		};
>>>>    	};
>>>>    };
diff mbox

Patch

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@33000000 {
+			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";
+		};
+
+		gpmi: gpmi-nand@33002000{
+			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";
+		};
 	};
 };