diff mbox series

[v4,14/21] arch: arm: socfpga: Add 'altr, sysmgr-syscon' for MMC node in device tree

Message ID 1583744842-24632-15-git-send-email-chee.hong.ang@intel.com
State New
Headers show
Series Enable ARM Trusted Firmware for U-Boot | expand

Commit Message

Ang, Chee Hong March 9, 2020, 9:07 a.m. UTC 
From: Chee Hong Ang <chee.hong.ang at intel.com>

In device tree for all socfpga platforms, a phandle to System Manager
('altr,sysmgr-syscon') is needed for MMC node to enable the MMC driver
to configure the SDMMC's clock phase shift via System Manager driver
(altera_sysmgr).
This phandle specifies the offset of the SDMCC control register in
System Manager, start of bit field for drvsel and start of bit field
for smplsel.

Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com>
---
 arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi    | 1 +
 arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts     | 1 +
 arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi    | 1 +
 arch/arm/dts/socfpga_cyclone5.dtsi               | 1 +
 arch/arm/dts/socfpga_stratix10.dtsi              | 1 -
 arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi | 7 +++++++
 arch/arm/dts/socfpga_stratix10_socdk.dts         | 2 --
 7 files changed, 11 insertions(+), 3 deletions(-)

Comments

Simon Goldschmidt March 10, 2020, 5:05 p.m. UTC | #1 
Am 09.03.2020 um 10:07 schrieb chee.hong.ang at intel.com:
> From: Chee Hong Ang <chee.hong.ang at intel.com>
> 
> In device tree for all socfpga platforms, a phandle to System Manager
> ('altr,sysmgr-syscon') is needed for MMC node to enable the MMC driver
> to configure the SDMMC's clock phase shift via System Manager driver
> (altera_sysmgr).
> This phandle specifies the offset of the SDMCC control register in
> System Manager, start of bit field for drvsel and start of bit field
> for smplsel.
> 
> Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com>
> ---
>  arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi    | 1 +
>  arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts     | 1 +
>  arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi    | 1 +
>  arch/arm/dts/socfpga_cyclone5.dtsi               | 1 +
>  arch/arm/dts/socfpga_stratix10.dtsi              | 1 -
>  arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi | 7 +++++++
>  arch/arm/dts/socfpga_stratix10_socdk.dts         | 2 --

This looks strange. I would have expected you add the 'syscon' entry to
the base dtsi files (and to the ones in Linux, too, btw). But you're
adding it to "-u-boot.dtsi" files, too. Why?

Regards,
Simon

>  7 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi b/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi
> index 1908be4..56fd7d9 100644
> --- a/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi
> +++ b/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi
> @@ -34,6 +34,7 @@
>  &mmc {
>  	drvsel = <3>;
>  	smplsel = <0>;
> +	altr,sysmgr-syscon = <&sysmgr 0x28 0 4>;
>  	u-boot,dm-pre-reloc;
>  };
>  
> diff --git a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> index d6b6c2d..887673b 100644
> --- a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> +++ b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> @@ -44,6 +44,7 @@
>  	cap-sd-highspeed;
>  	broken-cd;
>  	bus-width = <4>;
> +	altr,sysmgr-syscon = <&sysmgr 0x28 0 4>;
>  };
>  
>  &eccmgr {
> diff --git a/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi b/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi
> index dfaff4c..d2189f1 100644
> --- a/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi
> +++ b/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi
> @@ -20,6 +20,7 @@
>  };
>  
>  &mmc {
> +	altr,sysmgr-syscon = <&sysmgr 0x108 0 3>;
>  	u-boot,dm-pre-reloc;
>  };
>  
> diff --git a/arch/arm/dts/socfpga_cyclone5.dtsi b/arch/arm/dts/socfpga_cyclone5.dtsi
> index 319a71e..c309681 100644
> --- a/arch/arm/dts/socfpga_cyclone5.dtsi
> +++ b/arch/arm/dts/socfpga_cyclone5.dtsi
> @@ -23,6 +23,7 @@
>  			bus-width = <4>;
>  			cap-mmc-highspeed;
>  			cap-sd-highspeed;
> +			altr,sysmgr-syscon = <&sysmgr 0x108 0 3>;
>  		};
>  
>  		sysmgr at ffd08000 {
> diff --git a/arch/arm/dts/socfpga_stratix10.dtsi b/arch/arm/dts/socfpga_stratix10.dtsi
> index a8e61cf..9c89065 100755
> --- a/arch/arm/dts/socfpga_stratix10.dtsi
> +++ b/arch/arm/dts/socfpga_stratix10.dtsi
> @@ -228,7 +228,6 @@
>  			interrupts = <0 96 4>;
>  			fifo-depth = <0x400>;
>  			resets = <&rst SDMMC_RESET>, <&rst SDMMC_OCP_RESET>;
> -			u-boot,dm-pre-reloc;
>  			status = "disabled";
>  		};
>  
> diff --git a/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi b/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi
> index a903040..ca91b40 100755
> --- a/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi
> +++ b/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi
> @@ -28,6 +28,13 @@
>  	u-boot,dm-pre-reloc;
>  };
>  
> +&mmc {
> +	drvsel = <3>;
> +	smplsel = <0>;
> +	altr,sysmgr-syscon = <&sysmgr 0x28 0 4>;
> +	u-boot,dm-pre-reloc;
> +};
> +
>  &sysmgr {
>  	u-boot,dm-pre-reloc;
>  };
> diff --git a/arch/arm/dts/socfpga_stratix10_socdk.dts b/arch/arm/dts/socfpga_stratix10_socdk.dts
> index b7b48a5..ff6e1b2 100755
> --- a/arch/arm/dts/socfpga_stratix10_socdk.dts
> +++ b/arch/arm/dts/socfpga_stratix10_socdk.dts
> @@ -91,8 +91,6 @@
>  	cap-mmc-highspeed;
>  	broken-cd;
>  	bus-width = <4>;
> -	drvsel = <3>;
> -	smplsel = <0>;
>  };
>  
>  &qspi {
>
Ang, Chee Hong March 11, 2020, 7:06 a.m. UTC | #2 
> -----Original Message-----
> From: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> Sent: Wednesday, March 11, 2020 1:06 AM
> To: Ang, Chee Hong <chee.hong.ang at intel.com>; u-boot at lists.denx.de
> Cc: Marek Vasut <marex at denx.de>; See, Chin Liang <chin.liang.see at intel.com>;
> Tan, Ley Foon <ley.foon.tan at intel.com>; Westergreen, Dalon
> <dalon.westergreen at intel.com>; Gong, Richard <richard.gong at intel.com>
> Subject: Re: [PATCH v4 14/21] arch: arm: socfpga: Add 'altr,sysmgr-syscon' for
> MMC node in device tree
> 
> Am 09.03.2020 um 10:07 schrieb chee.hong.ang at intel.com:
> > From: Chee Hong Ang <chee.hong.ang at intel.com>
> >
> > In device tree for all socfpga platforms, a phandle to System Manager
> > ('altr,sysmgr-syscon') is needed for MMC node to enable the MMC driver
> > to configure the SDMMC's clock phase shift via System Manager driver
> > (altera_sysmgr).
> > This phandle specifies the offset of the SDMCC control register in
> > System Manager, start of bit field for drvsel and start of bit field
> > for smplsel.
> >
> > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com>
> > ---
> >  arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi    | 1 +
> >  arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts     | 1 +
> >  arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi    | 1 +
> >  arch/arm/dts/socfpga_cyclone5.dtsi               | 1 +
> >  arch/arm/dts/socfpga_stratix10.dtsi              | 1 -
> >  arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi | 7 +++++++
> >  arch/arm/dts/socfpga_stratix10_socdk.dts         | 2 --
> 
> This looks strange. I would have expected you add the 'syscon' entry to the base
> dtsi files (and to the ones in Linux, too, btw). But you're adding it to "-u-
> boot.dtsi" files, too. Why?
Where to add new device tree entry is rather confusing to me.
Linux SDMMC driver doesn't set the SDMMC clock. So this only
applicable to U-Boot only.
I thought "-u-boot-dtsi" is the place where we should put those
device tree entries that are only applicable to U-Boot only ?
> 
> Regards,
> Simon
> 
> >  7 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi
> > b/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi
> > index 1908be4..56fd7d9 100644
> > --- a/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi
> > +++ b/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi
> > @@ -34,6 +34,7 @@
> >  &mmc {
> >  	drvsel = <3>;
> >  	smplsel = <0>;
> > +	altr,sysmgr-syscon = <&sysmgr 0x28 0 4>;
> >  	u-boot,dm-pre-reloc;
> >  };
> >
> > diff --git a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> > b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> > index d6b6c2d..887673b 100644
> > --- a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> > +++ b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
> > @@ -44,6 +44,7 @@
> >  	cap-sd-highspeed;
> >  	broken-cd;
> >  	bus-width = <4>;
> > +	altr,sysmgr-syscon = <&sysmgr 0x28 0 4>;
> >  };
> >
> >  &eccmgr {
> > diff --git a/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi
> > b/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi
> > index dfaff4c..d2189f1 100644
> > --- a/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi
> > +++ b/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi
> > @@ -20,6 +20,7 @@
> >  };
> >
> >  &mmc {
> > +	altr,sysmgr-syscon = <&sysmgr 0x108 0 3>;
> >  	u-boot,dm-pre-reloc;
> >  };
> >
> > diff --git a/arch/arm/dts/socfpga_cyclone5.dtsi
> > b/arch/arm/dts/socfpga_cyclone5.dtsi
> > index 319a71e..c309681 100644
> > --- a/arch/arm/dts/socfpga_cyclone5.dtsi
> > +++ b/arch/arm/dts/socfpga_cyclone5.dtsi
> > @@ -23,6 +23,7 @@
> >  			bus-width = <4>;
> >  			cap-mmc-highspeed;
> >  			cap-sd-highspeed;
> > +			altr,sysmgr-syscon = <&sysmgr 0x108 0 3>;
> >  		};
> >
> >  		sysmgr at ffd08000 {
> > diff --git a/arch/arm/dts/socfpga_stratix10.dtsi
> > b/arch/arm/dts/socfpga_stratix10.dtsi
> > index a8e61cf..9c89065 100755
> > --- a/arch/arm/dts/socfpga_stratix10.dtsi
> > +++ b/arch/arm/dts/socfpga_stratix10.dtsi
> > @@ -228,7 +228,6 @@
> >  			interrupts = <0 96 4>;
> >  			fifo-depth = <0x400>;
> >  			resets = <&rst SDMMC_RESET>, <&rst
> SDMMC_OCP_RESET>;
> > -			u-boot,dm-pre-reloc;
> >  			status = "disabled";
> >  		};
> >
> > diff --git a/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi
> > b/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi
> > index a903040..ca91b40 100755
> > --- a/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi
> > +++ b/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi
> > @@ -28,6 +28,13 @@
> >  	u-boot,dm-pre-reloc;
> >  };
> >
> > +&mmc {
> > +	drvsel = <3>;
> > +	smplsel = <0>;
> > +	altr,sysmgr-syscon = <&sysmgr 0x28 0 4>;
> > +	u-boot,dm-pre-reloc;
> > +};
> > +
> >  &sysmgr {
> >  	u-boot,dm-pre-reloc;
> >  };
> > diff --git a/arch/arm/dts/socfpga_stratix10_socdk.dts
> > b/arch/arm/dts/socfpga_stratix10_socdk.dts
> > index b7b48a5..ff6e1b2 100755
> > --- a/arch/arm/dts/socfpga_stratix10_socdk.dts
> > +++ b/arch/arm/dts/socfpga_stratix10_socdk.dts
> > @@ -91,8 +91,6 @@
> >  	cap-mmc-highspeed;
> >  	broken-cd;
> >  	bus-width = <4>;
> > -	drvsel = <3>;
> > -	smplsel = <0>;
> >  };
> >
> >  &qspi {
> >
Marek Vasut March 11, 2020, 7:14 a.m. UTC | #3 
On 3/11/20 8:06 AM, Ang, Chee Hong wrote:
> 
> 
>> -----Original Message-----
>> From: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>> Sent: Wednesday, March 11, 2020 1:06 AM
>> To: Ang, Chee Hong <chee.hong.ang at intel.com>; u-boot at lists.denx.de
>> Cc: Marek Vasut <marex at denx.de>; See, Chin Liang <chin.liang.see at intel.com>;
>> Tan, Ley Foon <ley.foon.tan at intel.com>; Westergreen, Dalon
>> <dalon.westergreen at intel.com>; Gong, Richard <richard.gong at intel.com>
>> Subject: Re: [PATCH v4 14/21] arch: arm: socfpga: Add 'altr,sysmgr-syscon' for
>> MMC node in device tree

Can you please fix your mailer to avoid re-adding the entire header into
the message ?

>> Am 09.03.2020 um 10:07 schrieb chee.hong.ang at intel.com:
>>> From: Chee Hong Ang <chee.hong.ang at intel.com>
>>>
>>> In device tree for all socfpga platforms, a phandle to System Manager
>>> ('altr,sysmgr-syscon') is needed for MMC node to enable the MMC driver
>>> to configure the SDMMC's clock phase shift via System Manager driver
>>> (altera_sysmgr).
>>> This phandle specifies the offset of the SDMCC control register in
>>> System Manager, start of bit field for drvsel and start of bit field
>>> for smplsel.
>>>
>>> Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com>
>>> ---
>>>  arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi    | 1 +
>>>  arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts     | 1 +
>>>  arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi    | 1 +
>>>  arch/arm/dts/socfpga_cyclone5.dtsi               | 1 +
>>>  arch/arm/dts/socfpga_stratix10.dtsi              | 1 -
>>>  arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi | 7 +++++++
>>>  arch/arm/dts/socfpga_stratix10_socdk.dts         | 2 --
>>
>> This looks strange. I would have expected you add the 'syscon' entry to the base
>> dtsi files (and to the ones in Linux, too, btw). But you're adding it to "-u-
>> boot.dtsi" files, too. Why?
> Where to add new device tree entry is rather confusing to me.
> Linux SDMMC driver doesn't set the SDMMC clock. So this only
> applicable to U-Boot only.

DT describes hardware, so U-Boot and Linux DTs should be ideally
identical. I would expect syscon, which is actual hardware, to be
applicable to both U-Boot and Linux (and other OSes too) ?

> I thought "-u-boot-dtsi" is the place where we should put those
> device tree entries that are only applicable to U-Boot only ?
That is more often used for things which are indeed U-Boot specific,
that is nodes which have u-boot, prefix and/or hardware bits which are
not yet part of Linux DT, but which _will_ be part of Linux DT once they
trickle through the upstream machinery.

[...]
diff mbox series

Patch

diff --git a/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi b/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi
index 1908be4..56fd7d9 100644
--- a/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi
+++ b/arch/arm/dts/socfpga_agilex_socdk-u-boot.dtsi
@@ -34,6 +34,7 @@ 
 &mmc {
 	drvsel = <3>;
 	smplsel = <0>;
+	altr,sysmgr-syscon = <&sysmgr 0x28 0 4>;
 	u-boot,dm-pre-reloc;
 };
 
diff --git a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
index d6b6c2d..887673b 100644
--- a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
+++ b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts
@@ -44,6 +44,7 @@ 
 	cap-sd-highspeed;
 	broken-cd;
 	bus-width = <4>;
+	altr,sysmgr-syscon = <&sysmgr 0x28 0 4>;
 };
 
 &eccmgr {
diff --git a/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi b/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi
index dfaff4c..d2189f1 100644
--- a/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi
+++ b/arch/arm/dts/socfpga_arria5_socdk-u-boot.dtsi
@@ -20,6 +20,7 @@ 
 };
 
 &mmc {
+	altr,sysmgr-syscon = <&sysmgr 0x108 0 3>;
 	u-boot,dm-pre-reloc;
 };
 
diff --git a/arch/arm/dts/socfpga_cyclone5.dtsi b/arch/arm/dts/socfpga_cyclone5.dtsi
index 319a71e..c309681 100644
--- a/arch/arm/dts/socfpga_cyclone5.dtsi
+++ b/arch/arm/dts/socfpga_cyclone5.dtsi
@@ -23,6 +23,7 @@ 
 			bus-width = <4>;
 			cap-mmc-highspeed;
 			cap-sd-highspeed;
+			altr,sysmgr-syscon = <&sysmgr 0x108 0 3>;
 		};
 
 		sysmgr at ffd08000 {
diff --git a/arch/arm/dts/socfpga_stratix10.dtsi b/arch/arm/dts/socfpga_stratix10.dtsi
index a8e61cf..9c89065 100755
--- a/arch/arm/dts/socfpga_stratix10.dtsi
+++ b/arch/arm/dts/socfpga_stratix10.dtsi
@@ -228,7 +228,6 @@ 
 			interrupts = <0 96 4>;
 			fifo-depth = <0x400>;
 			resets = <&rst SDMMC_RESET>, <&rst SDMMC_OCP_RESET>;
-			u-boot,dm-pre-reloc;
 			status = "disabled";
 		};
 
diff --git a/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi b/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi
index a903040..ca91b40 100755
--- a/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi
+++ b/arch/arm/dts/socfpga_stratix10_socdk-u-boot.dtsi
@@ -28,6 +28,13 @@ 
 	u-boot,dm-pre-reloc;
 };
 
+&mmc {
+	drvsel = <3>;
+	smplsel = <0>;
+	altr,sysmgr-syscon = <&sysmgr 0x28 0 4>;
+	u-boot,dm-pre-reloc;
+};
+
 &sysmgr {
 	u-boot,dm-pre-reloc;
 };
diff --git a/arch/arm/dts/socfpga_stratix10_socdk.dts b/arch/arm/dts/socfpga_stratix10_socdk.dts
index b7b48a5..ff6e1b2 100755
--- a/arch/arm/dts/socfpga_stratix10_socdk.dts
+++ b/arch/arm/dts/socfpga_stratix10_socdk.dts
@@ -91,8 +91,6 @@ 
 	cap-mmc-highspeed;
 	broken-cd;
 	bus-width = <4>;
-	drvsel = <3>;
-	smplsel = <0>;
 };
 
 &qspi {