Re: [PATCH v4 14/21] arch: arm: socfpga: Add 'altr, sysmgr-syscon' for MMC node in device tree
On 3/11/20 8:06 AM, Ang, Chee Hong wrote:
>
>
>> -Original Message-
>> From: Simon Goldschmidt
>> Sent: Wednesday, March 11, 2020 1:06 AM
>> To: Ang, Chee Hong ; u-boot@lists.denx.de
>> Cc: Marek Vasut ; See, Chin Liang ;
>> Tan, Ley Foon ; Westergreen, Dalon
>> ; Gong, Richard
>> 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@intel.com:
>>> From: Chee Hong Ang
>>>
>>> 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
>>> ---
>>> 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.
[...]
--
Best regards,
Marek Vasut
RE: [PATCH v4 14/21] arch: arm: socfpga: Add 'altr, sysmgr-syscon' for MMC node in device tree
> -Original Message-
> From: Simon Goldschmidt
> Sent: Wednesday, March 11, 2020 1:06 AM
> To: Ang, Chee Hong ; u-boot@lists.denx.de
> Cc: Marek Vasut ; See, Chin Liang ;
> Tan, Ley Foon ; Westergreen, Dalon
> ; Gong, Richard
> 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@intel.com:
> > From: Chee Hong Ang
> >
> > 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
> > ---
> > 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 @@
> > {
> > drvsel = <3>;
> > smplsel = <0>;
> > + altr,sysmgr-syscon = < 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 = < 0x28 0 4>;
> > };
> >
> > {
> > 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 @@
> > };
> >
> > {
> > + altr,sysmgr-syscon = < 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 = < 0x108 0 3>;
> > };
> >
> > sysmgr@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 = < SDMMC_RESET>, <
> 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;
> > };
> >
> > + {
> > + drvsel = <3>;
> > + smplsel = <0>;
> > + altr,sysmgr-syscon = < 0x28 0 4>;
> > + u-boot,dm-pre-reloc;
> > +};
> > +
> > {
> > 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>;
> > };
> >
> > {
> >
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@intel.com:
> From: Chee Hong Ang
>
> 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
> ---
> 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 @@
> {
> drvsel = <3>;
> smplsel = <0>;
> + altr,sysmgr-syscon = < 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 = < 0x28 0 4>;
> };
>
> {
> 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 @@
> };
>
> {
> + altr,sysmgr-syscon = < 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 = < 0x108 0 3>;
> };
>
> sysmgr@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 = < SDMMC_RESET>, < 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;
> };
>
> + {
> + drvsel = <3>;
> + smplsel = <0>;
> + altr,sysmgr-syscon = < 0x28 0 4>;
> + u-boot,dm-pre-reloc;
> +};
> +
> {
> 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>;
> };
>
> {
>
