diff mbox series

dt-bindings: nand: meson: fix controller clock

Message ID 20220106033130.37623-1-liang.yang@amlogic.com (mailing list archive)
State New, archived
Headers show
Series dt-bindings: nand: meson: fix controller clock | expand

Commit Message

Liang Yang Jan. 6, 2022, 3:31 a.m. UTC 
Change-Id: I1425b491d8b95061e1ce358ef33143433fc94d24
---
 .../bindings/mtd/amlogic,meson-nand.txt        | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

Comments

Miquel Raynal Jan. 6, 2022, 8:12 a.m. UTC | #1 
Hello,

liang.yang@amlogic.com wrote on Thu, 6 Jan 2022 11:31:30 +0800:

This patch should be in a series with "mtd: rawnand: meson: fix the
clock after discarding sd_emmc_c_clk" and possibly come first.

You miss a commit message which is _very_ important given the type of
change you propose.
 
> Change-Id: I1425b491d8b95061e1ce358ef33143433fc94d24

Change IDs have nothing to do here.

However a Fixes and a Signed-off-by are welcome.

You'll also need to Cc: Rob H. in your v2.

> ---
>  .../bindings/mtd/amlogic,meson-nand.txt        | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> index 5794ab1147c1..37f16fe4fe66 100644
> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> @@ -14,11 +14,6 @@ Required properties:
>  - clock-names: Should contain the following:
>  	"core" - NFC module gate clock
>  	"device" - device clock from eMMC sub clock controller
> -	"rx" - rx clock phase
> -	"tx" - tx clock phase
> -
> -- amlogic,mmc-syscon	: Required for NAND clocks, it's shared with SD/eMMC
> -				controller port C
>  
>  Optional children nodes:
>  Children nodes represent the available nand chips.
> @@ -28,11 +23,6 @@ see Documentation/devicetree/bindings/mtd/nand-controller.yaml for generic bindi
>  
>  Example demonstrate on AXG SoC:
>  
> -	sd_emmc_c_clkc: mmc@7000 {
> -		compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
> -		reg = <0x0 0x7000 0x0 0x800>;
> -	};
> -
>  	nand-controller@7800 {
>  		compatible = "amlogic,meson-axg-nfc";
>  		reg = <0x0 0x7800 0x0 0x100>;
> @@ -41,11 +31,9 @@ Example demonstrate on AXG SoC:
>  		interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
>  
>  		clocks = <&clkc CLKID_SD_EMMC_C>,
> -			<&sd_emmc_c_clkc CLKID_MMC_DIV>,
> -			<&sd_emmc_c_clkc CLKID_MMC_PHASE_RX>,
> -			<&sd_emmc_c_clkc CLKID_MMC_PHASE_TX>;
> -		clock-names = "core", "device", "rx", "tx";
> -		amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
> +			<&clkc CLKID_FCLK_DIV2>;
> +		clock-names = "core", "device";
> +		sd_emmc_c_clkc = <0xffe07000>;
>  
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&nand_pins>;


Thanks,
Miquèl
Miquel Raynal Jan. 6, 2022, 8:12 a.m. UTC | #2 
liang.yang@amlogic.com wrote on Thu, 6 Jan 2022 11:31:30 +0800:

> Change-Id: I1425b491d8b95061e1ce358ef33143433fc94d24
> ---
>  .../bindings/mtd/amlogic,meson-nand.txt        | 18 +++---------------

I forgot to mention, while you're at it, after fixing the bindings,
could you please convert it to yaml?

>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> index 5794ab1147c1..37f16fe4fe66 100644
> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> @@ -14,11 +14,6 @@ Required properties:
>  - clock-names: Should contain the following:
>  	"core" - NFC module gate clock
>  	"device" - device clock from eMMC sub clock controller
> -	"rx" - rx clock phase
> -	"tx" - tx clock phase
> -
> -- amlogic,mmc-syscon	: Required for NAND clocks, it's shared with SD/eMMC
> -				controller port C
>  
>  Optional children nodes:
>  Children nodes represent the available nand chips.
> @@ -28,11 +23,6 @@ see Documentation/devicetree/bindings/mtd/nand-controller.yaml for generic bindi
>  
>  Example demonstrate on AXG SoC:
>  
> -	sd_emmc_c_clkc: mmc@7000 {
> -		compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
> -		reg = <0x0 0x7000 0x0 0x800>;
> -	};
> -
>  	nand-controller@7800 {
>  		compatible = "amlogic,meson-axg-nfc";
>  		reg = <0x0 0x7800 0x0 0x100>;
> @@ -41,11 +31,9 @@ Example demonstrate on AXG SoC:
>  		interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
>  
>  		clocks = <&clkc CLKID_SD_EMMC_C>,
> -			<&sd_emmc_c_clkc CLKID_MMC_DIV>,
> -			<&sd_emmc_c_clkc CLKID_MMC_PHASE_RX>,
> -			<&sd_emmc_c_clkc CLKID_MMC_PHASE_TX>;
> -		clock-names = "core", "device", "rx", "tx";
> -		amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
> +			<&clkc CLKID_FCLK_DIV2>;
> +		clock-names = "core", "device";
> +		sd_emmc_c_clkc = <0xffe07000>;
>  
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&nand_pins>;


Thanks,
Miquèl
Neil Armstrong Jan. 6, 2022, 8:19 a.m. UTC | #3 
Hi,

On 06/01/2022 04:31, Liang Yang wrote:
> Change-Id: I1425b491d8b95061e1ce358ef33143433fc94d24
> ---
>  .../bindings/mtd/amlogic,meson-nand.txt        | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> index 5794ab1147c1..37f16fe4fe66 100644
> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> @@ -14,11 +14,6 @@ Required properties:
>  - clock-names: Should contain the following:
>  	"core" - NFC module gate clock
>  	"device" - device clock from eMMC sub clock controller
> -	"rx" - rx clock phase
> -	"tx" - tx clock phase
> -
> -- amlogic,mmc-syscon	: Required for NAND clocks, it's shared with SD/eMMC
> -				controller port C
>  
>  Optional children nodes:
>  Children nodes represent the available nand chips.
> @@ -28,11 +23,6 @@ see Documentation/devicetree/bindings/mtd/nand-controller.yaml for generic bindi
>  
>  Example demonstrate on AXG SoC:
>  
> -	sd_emmc_c_clkc: mmc@7000 {
> -		compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
> -		reg = <0x0 0x7000 0x0 0x800>;
> -	};
> -
>  	nand-controller@7800 {
>  		compatible = "amlogic,meson-axg-nfc";
>  		reg = <0x0 0x7800 0x0 0x100>;
> @@ -41,11 +31,9 @@ Example demonstrate on AXG SoC:
>  		interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
>  
>  		clocks = <&clkc CLKID_SD_EMMC_C>,
> -			<&sd_emmc_c_clkc CLKID_MMC_DIV>,
> -			<&sd_emmc_c_clkc CLKID_MMC_PHASE_RX>,
> -			<&sd_emmc_c_clkc CLKID_MMC_PHASE_TX>;
> -		clock-names = "core", "device", "rx", "tx";
> -		amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
> +			<&clkc CLKID_FCLK_DIV2>;
> +		clock-names = "core", "device";
> +		sd_emmc_c_clkc = <0xffe07000>;


sd_emmc_c_clkc should be documented, but it should be either:
- a syscon
- in reg

but since 0xffe07000 is part of the emmc memory space, you can't simply
put this address like that and pass it to a random undocumented property.

Adding "syscon" to the sd_emmc_c compatibles, and passing the sd_emmc_c node as syscon
could actually work.

Neil

>  
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&nand_pins>;
>
Jerome Brunet Jan. 6, 2022, 8:43 a.m. UTC | #4 
On Thu 06 Jan 2022 at 09:12, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> liang.yang@amlogic.com wrote on Thu, 6 Jan 2022 11:31:30 +0800:
>
>> Change-Id: I1425b491d8b95061e1ce358ef33143433fc94d24
>> ---
>>  .../bindings/mtd/amlogic,meson-nand.txt        | 18 +++---------------
>
> I forgot to mention, while you're at it, after fixing the bindings,
> could you please convert it to yaml?
>
>>  1 file changed, 3 insertions(+), 15 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> index 5794ab1147c1..37f16fe4fe66 100644
>> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> @@ -14,11 +14,6 @@ Required properties:
>>  - clock-names: Should contain the following:
>>  	"core" - NFC module gate clock
>>  	"device" - device clock from eMMC sub clock controller

If I refer to the related driver change, this is not true anymore and
should be updated

>> -	"rx" - rx clock phase
>> -	"tx" - tx clock phase
>> -
>> -- amlogic,mmc-syscon	: Required for NAND clocks, it's shared with SD/eMMC
>> -				controller port C
>>  
>>  Optional children nodes:
>>  Children nodes represent the available nand chips.
>> @@ -28,11 +23,6 @@ see Documentation/devicetree/bindings/mtd/nand-controller.yaml for generic bindi
>>  
>>  Example demonstrate on AXG SoC:
>>  
>> -	sd_emmc_c_clkc: mmc@7000 {
>> -		compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
>> -		reg = <0x0 0x7000 0x0 0x800>;
>> -	};
>> -
>>  	nand-controller@7800 {
>>  		compatible = "amlogic,meson-axg-nfc";
>>  		reg = <0x0 0x7800 0x0 0x100>;
>> @@ -41,11 +31,9 @@ Example demonstrate on AXG SoC:
>>  		interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
>>  
>>  		clocks = <&clkc CLKID_SD_EMMC_C>,
>> -			<&sd_emmc_c_clkc CLKID_MMC_DIV>,
>> -			<&sd_emmc_c_clkc CLKID_MMC_PHASE_RX>,
>> -			<&sd_emmc_c_clkc CLKID_MMC_PHASE_TX>;
>> -		clock-names = "core", "device", "rx", "tx";
>> -		amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
>> +			<&clkc CLKID_FCLK_DIV2>;
>> +		clock-names = "core", "device";

If you want to re-implement the mmc clock part directly in the nand
driver, you should provide both clock inputs, like the mmc driver (in
addition to the pclk) ... Even if you plan on using only FDIV2 in the
driver, this what the HW is.

Something like:

clock-names = "core", "clkin0", "clkin1";
clocks = <&clkc CLKID_SD_EMMC_C>,
         <&clkc CLKID_SD_EMMC_C_CLK0>,
	 <&clkc CLKID_FCLK_DIV2>;

>> +		sd_emmc_c_clkc = <0xffe07000>;

This is not how you provide memory regions

>>  
>>  		pinctrl-names = "default";
>>  		pinctrl-0 = <&nand_pins>;
>
>
> Thanks,
> Miquèl
Liang Yang Jan. 7, 2022, 2:48 a.m. UTC | #5 
Hi Miquel,

On 2022/1/6 16:12, Miquel Raynal wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello,
> 
> liang.yang@amlogic.com wrote on Thu, 6 Jan 2022 11:31:30 +0800:
> 
> This patch should be in a series with "mtd: rawnand: meson: fix the
> clock after discarding sd_emmc_c_clk" and possibly come first.
> 
> You miss a commit message which is _very_ important given the type of
> change you propose.
>   
>> Change-Id: I1425b491d8b95061e1ce358ef33143433fc94d24
> 
> Change IDs have nothing to do here.
> 
> However a Fixes and a Signed-off-by are welcome.
> 
> You'll also need to Cc: Rob H. in your v2.
> 
ok, i will fix these once we clarify the patch about sd_emmc_c_clk and 
decide to use this patch.
>> ---
>>   .../bindings/mtd/amlogic,meson-nand.txt        | 18 +++---------------
>>   1 file changed, 3 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> index 5794ab1147c1..37f16fe4fe66 100644
>> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> @@ -14,11 +14,6 @@ Required properties:
>>   - clock-names: Should contain the following:
>>   	"core" - NFC module gate clock
>>   	"device" - device clock from eMMC sub clock controller
>> -	"rx" - rx clock phase
>> -	"tx" - tx clock phase
>> -
>> -- amlogic,mmc-syscon	: Required for NAND clocks, it's shared with SD/eMMC
>> -				controller port C
>>   
>>   Optional children nodes:
>>   Children nodes represent the available nand chips.
>> @@ -28,11 +23,6 @@ see Documentation/devicetree/bindings/mtd/nand-controller.yaml for generic bindi
>>   
>>   Example demonstrate on AXG SoC:
>>   
>> -	sd_emmc_c_clkc: mmc@7000 {
>> -		compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
>> -		reg = <0x0 0x7000 0x0 0x800>;
>> -	};
>> -
>>   	nand-controller@7800 {
>>   		compatible = "amlogic,meson-axg-nfc";
>>   		reg = <0x0 0x7800 0x0 0x100>;
>> @@ -41,11 +31,9 @@ Example demonstrate on AXG SoC:
>>   		interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
>>   
>>   		clocks = <&clkc CLKID_SD_EMMC_C>,
>> -			<&sd_emmc_c_clkc CLKID_MMC_DIV>,
>> -			<&sd_emmc_c_clkc CLKID_MMC_PHASE_RX>,
>> -			<&sd_emmc_c_clkc CLKID_MMC_PHASE_TX>;
>> -		clock-names = "core", "device", "rx", "tx";
>> -		amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
>> +			<&clkc CLKID_FCLK_DIV2>;
>> +		clock-names = "core", "device";
>> +		sd_emmc_c_clkc = <0xffe07000>;
>>   
>>   		pinctrl-names = "default";
>>   		pinctrl-0 = <&nand_pins>;
> 
> 
> Thanks,
> Miquèl
> 
> .
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
index 5794ab1147c1..37f16fe4fe66 100644
--- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
@@ -14,11 +14,6 @@  Required properties:
 - clock-names: Should contain the following:
 	"core" - NFC module gate clock
 	"device" - device clock from eMMC sub clock controller
-	"rx" - rx clock phase
-	"tx" - tx clock phase
-
-- amlogic,mmc-syscon	: Required for NAND clocks, it's shared with SD/eMMC
-				controller port C
 
 Optional children nodes:
 Children nodes represent the available nand chips.
@@ -28,11 +23,6 @@  see Documentation/devicetree/bindings/mtd/nand-controller.yaml for generic bindi
 
 Example demonstrate on AXG SoC:
 
-	sd_emmc_c_clkc: mmc@7000 {
-		compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
-		reg = <0x0 0x7000 0x0 0x800>;
-	};
-
 	nand-controller@7800 {
 		compatible = "amlogic,meson-axg-nfc";
 		reg = <0x0 0x7800 0x0 0x100>;
@@ -41,11 +31,9 @@  Example demonstrate on AXG SoC:
 		interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
 
 		clocks = <&clkc CLKID_SD_EMMC_C>,
-			<&sd_emmc_c_clkc CLKID_MMC_DIV>,
-			<&sd_emmc_c_clkc CLKID_MMC_PHASE_RX>,
-			<&sd_emmc_c_clkc CLKID_MMC_PHASE_TX>;
-		clock-names = "core", "device", "rx", "tx";
-		amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
+			<&clkc CLKID_FCLK_DIV2>;
+		clock-names = "core", "device";
+		sd_emmc_c_clkc = <0xffe07000>;
 
 		pinctrl-names = "default";
 		pinctrl-0 = <&nand_pins>;