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

[Ang, Chee Hong]

> -----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 {
> >