2023-11-30 01:49:43

by Drew Fustini

[permalink] [raw]
Subject: [PATCH v7 2/4] riscv: dts: thead: Add TH1520 mmc controllers and sdhci clock

Add node for the SDHCI fixed clock. Add mmc0 node for the first mmc
controller instance which is typically connected to the eMMC device.
Add mmc1 node for the second mmc controller instance which is typically
connected to microSD slot.

Signed-off-by: Drew Fustini <[email protected]>
---
arch/riscv/boot/dts/thead/th1520.dtsi | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index ba4d2c673ac8..af4fdcd82e0b 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -146,6 +146,13 @@ uart_sclk: uart-sclk-clock {
#clock-cells = <0>;
};

+ sdhci_clk: sdhci-clock {
+ compatible = "fixed-clock";
+ clock-frequency = <198000000>;
+ clock-output-names = "sdhci_clk";
+ #clock-cells = <0>;
+ };
+
soc {
compatible = "simple-bus";
interrupt-parent = <&plic>;
@@ -304,6 +311,24 @@ dmac0: dma-controller@ffefc00000 {
status = "disabled";
};

+ mmc0: mmc@ffe7080000 {
+ compatible = "thead,th1520-dwcmshc";
+ reg = <0xff 0xe7080000 0x0 0x10000>;
+ interrupts = <62 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&sdhci_clk>;
+ clock-names = "core";
+ status = "disabled";
+ };
+
+ mmc1: mmc@ffe7090000 {
+ compatible = "thead,th1520-dwcmshc";
+ reg = <0xff 0xe7090000 0x0 0x10000>;
+ interrupts = <64 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&sdhci_clk>;
+ clock-names = "core";
+ status = "disabled";
+ };
+
timer0: timer@ffefc32000 {
compatible = "snps,dw-apb-timer";
reg = <0xff 0xefc32000 0x0 0x14>;

--
2.34.1


2023-12-04 09:48:26

by Emil Renner Berthing

[permalink] [raw]
Subject: Re: [PATCH v7 2/4] riscv: dts: thead: Add TH1520 mmc controllers and sdhci clock

Drew Fustini wrote:
> Add node for the SDHCI fixed clock. Add mmc0 node for the first mmc
> controller instance which is typically connected to the eMMC device.
> Add mmc1 node for the second mmc controller instance which is typically
> connected to microSD slot.
>
> Signed-off-by: Drew Fustini <[email protected]>
> ---
> arch/riscv/boot/dts/thead/th1520.dtsi | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index ba4d2c673ac8..af4fdcd82e0b 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -146,6 +146,13 @@ uart_sclk: uart-sclk-clock {
> #clock-cells = <0>;
> };
>
> + sdhci_clk: sdhci-clock {
> + compatible = "fixed-clock";
> + clock-frequency = <198000000>;
> + clock-output-names = "sdhci_clk";
> + #clock-cells = <0>;
> + };
> +
> soc {
> compatible = "simple-bus";
> interrupt-parent = <&plic>;
> @@ -304,6 +311,24 @@ dmac0: dma-controller@ffefc00000 {
> status = "disabled";
> };
>
> + mmc0: mmc@ffe7080000 {
> + compatible = "thead,th1520-dwcmshc";
> + reg = <0xff 0xe7080000 0x0 0x10000>;
> + interrupts = <62 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&sdhci_clk>;
> + clock-names = "core";
> + status = "disabled";
> + };
> +
> + mmc1: mmc@ffe7090000 {
> + compatible = "thead,th1520-dwcmshc";
> + reg = <0xff 0xe7090000 0x0 0x10000>;
> + interrupts = <64 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&sdhci_clk>;
> + clock-names = "core";
> + status = "disabled";
> + };
> +

Hi Drew,

This doesn't seem to match the documentation shared here:
https://lore.kernel.org/linux-riscv/[email protected]/
From the TH1520 System User Manual.pdf in there, I'd expect something like

emmc: mmc@ffe7080000 {
compatible = "thead,th1520-dwcmshc";
reg = <0xff 0xe7080000 0x0 0x10000>;
...
};

sdio0: mmc@ffe7090000 {
compatible = "thead,th1520-dwcmshc";
reg = <0xff 0xe7090000 0x0 0x10000>;
...
};

sdio1: mmc@ffe70a0000 {
compatible = "thead,th1520-dwcmshc";
reg = <0xff 0xe70a0000 0x0 0x10000>;
...
};

/Emil

2023-12-05 14:39:40

by Jisheng Zhang

[permalink] [raw]
Subject: Re: [PATCH v7 2/4] riscv: dts: thead: Add TH1520 mmc controllers and sdhci clock

On Mon, Dec 04, 2023 at 01:47:45AM -0800, Emil Renner Berthing wrote:
> Drew Fustini wrote:
> > Add node for the SDHCI fixed clock. Add mmc0 node for the first mmc
> > controller instance which is typically connected to the eMMC device.
> > Add mmc1 node for the second mmc controller instance which is typically
> > connected to microSD slot.
> >
> > Signed-off-by: Drew Fustini <[email protected]>
> > ---
> > arch/riscv/boot/dts/thead/th1520.dtsi | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> > index ba4d2c673ac8..af4fdcd82e0b 100644
> > --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> > +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> > @@ -146,6 +146,13 @@ uart_sclk: uart-sclk-clock {
> > #clock-cells = <0>;
> > };
> >
> > + sdhci_clk: sdhci-clock {
> > + compatible = "fixed-clock";
> > + clock-frequency = <198000000>;
> > + clock-output-names = "sdhci_clk";
> > + #clock-cells = <0>;
> > + };
> > +
> > soc {
> > compatible = "simple-bus";
> > interrupt-parent = <&plic>;
> > @@ -304,6 +311,24 @@ dmac0: dma-controller@ffefc00000 {
> > status = "disabled";
> > };
> >
> > + mmc0: mmc@ffe7080000 {
> > + compatible = "thead,th1520-dwcmshc";
> > + reg = <0xff 0xe7080000 0x0 0x10000>;
> > + interrupts = <62 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&sdhci_clk>;
> > + clock-names = "core";
> > + status = "disabled";
> > + };
> > +
> > + mmc1: mmc@ffe7090000 {
> > + compatible = "thead,th1520-dwcmshc";
> > + reg = <0xff 0xe7090000 0x0 0x10000>;
> > + interrupts = <64 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&sdhci_clk>;
> > + clock-names = "core";
> > + status = "disabled";
> > + };
> > +
>
> Hi Drew,
>
> This doesn't seem to match the documentation shared here:
> https://lore.kernel.org/linux-riscv/[email protected]/
> From the TH1520 System User Manual.pdf in there, I'd expect something like

>
> emmc: mmc@ffe7080000 {
> compatible = "thead,th1520-dwcmshc";
> reg = <0xff 0xe7080000 0x0 0x10000>;
> ...
> };

Hi Emil,

I think this isn't necessary. From other soc dts files, I see such
naming, but lots socs just use mmc0, mmc1, and so on.

And IIRC, the host for sd and sdio can support both, IOW, below
sdio0/sdio1 may be used for sdcard.

Thanks
>
> sdio0: mmc@ffe7090000 {
> compatible = "thead,th1520-dwcmshc";
> reg = <0xff 0xe7090000 0x0 0x10000>;
> ...
> };
>
> sdio1: mmc@ffe70a0000 {
> compatible = "thead,th1520-dwcmshc";
> reg = <0xff 0xe70a0000 0x0 0x10000>;
> ...
> };
>
> /Emil

2023-12-05 14:48:32

by Emil Renner Berthing

[permalink] [raw]
Subject: Re: [PATCH v7 2/4] riscv: dts: thead: Add TH1520 mmc controllers and sdhci clock

Jisheng Zhang wrote:
> On Mon, Dec 04, 2023 at 01:47:45AM -0800, Emil Renner Berthing wrote:
> > Drew Fustini wrote:
> > > Add node for the SDHCI fixed clock. Add mmc0 node for the first mmc
> > > controller instance which is typically connected to the eMMC device.
> > > Add mmc1 node for the second mmc controller instance which is typically
> > > connected to microSD slot.
> > >
> > > Signed-off-by: Drew Fustini <[email protected]>
> > > ---
> > > arch/riscv/boot/dts/thead/th1520.dtsi | 25 +++++++++++++++++++++++++
> > > 1 file changed, 25 insertions(+)
> > >
> > > diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> > > index ba4d2c673ac8..af4fdcd82e0b 100644
> > > --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> > > +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> > > @@ -146,6 +146,13 @@ uart_sclk: uart-sclk-clock {
> > > #clock-cells = <0>;
> > > };
> > >
> > > + sdhci_clk: sdhci-clock {
> > > + compatible = "fixed-clock";
> > > + clock-frequency = <198000000>;
> > > + clock-output-names = "sdhci_clk";
> > > + #clock-cells = <0>;
> > > + };
> > > +
> > > soc {
> > > compatible = "simple-bus";
> > > interrupt-parent = <&plic>;
> > > @@ -304,6 +311,24 @@ dmac0: dma-controller@ffefc00000 {
> > > status = "disabled";
> > > };
> > >
> > > + mmc0: mmc@ffe7080000 {
> > > + compatible = "thead,th1520-dwcmshc";
> > > + reg = <0xff 0xe7080000 0x0 0x10000>;
> > > + interrupts = <62 IRQ_TYPE_LEVEL_HIGH>;
> > > + clocks = <&sdhci_clk>;
> > > + clock-names = "core";
> > > + status = "disabled";
> > > + };
> > > +
> > > + mmc1: mmc@ffe7090000 {
> > > + compatible = "thead,th1520-dwcmshc";
> > > + reg = <0xff 0xe7090000 0x0 0x10000>;
> > > + interrupts = <64 IRQ_TYPE_LEVEL_HIGH>;
> > > + clocks = <&sdhci_clk>;
> > > + clock-names = "core";
> > > + status = "disabled";
> > > + };
> > > +
> >
> > Hi Drew,
> >
> > This doesn't seem to match the documentation shared here:
> > https://lore.kernel.org/linux-riscv/[email protected]/
> > From the TH1520 System User Manual.pdf in there, I'd expect something like
>
> >
> > emmc: mmc@ffe7080000 {
> > compatible = "thead,th1520-dwcmshc";
> > reg = <0xff 0xe7080000 0x0 0x10000>;
> > ...
> > };
>
> Hi Emil,
>
> I think this isn't necessary. From other soc dts files, I see such
> naming, but lots socs just use mmc0, mmc1, and so on.

No it certainly isn't necessary. Those labels are purely for us humans to read
and are not present in the dtb. But that's exactly why I think it'd be a good
idea match the labels with the documentation, so it will easier for us humans
to match up the device tree source to documentation.

> And IIRC, the host for sd and sdio can support both, IOW, below
> sdio0/sdio1 may be used for sdcard.

Yes, all of the EMMC, SDIO0 and SDIO1 seem to be instances of the same IP.

/Emil

2023-12-05 15:52:27

by Damian Tometzki

[permalink] [raw]
Subject: Re: [PATCH v7 2/4] riscv: dts: thead: Add TH1520 mmc controllers and sdhci clock

On Tue, 05. Dec 06:48, Emil Renner Berthing wrote:
> Jisheng Zhang wrote:
> > On Mon, Dec 04, 2023 at 01:47:45AM -0800, Emil Renner Berthing wrote:
> > > Drew Fustini wrote:
> > > > Add node for the SDHCI fixed clock. Add mmc0 node for the first mmc
> > > > controller instance which is typically connected to the eMMC device.
> > > > Add mmc1 node for the second mmc controller instance which is typically
> > > > connected to microSD slot.
> > > >
> > > > Signed-off-by: Drew Fustini <[email protected]>
> > > > ---
> > > > arch/riscv/boot/dts/thead/th1520.dtsi | 25 +++++++++++++++++++++++++
> > > > 1 file changed, 25 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> > > > index ba4d2c673ac8..af4fdcd82e0b 100644
> > > > --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> > > > +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> > > > @@ -146,6 +146,13 @@ uart_sclk: uart-sclk-clock {
> > > > #clock-cells = <0>;
> > > > };
> > > >
> > > > + sdhci_clk: sdhci-clock {
> > > > + compatible = "fixed-clock";
> > > > + clock-frequency = <198000000>;
> > > > + clock-output-names = "sdhci_clk";
> > > > + #clock-cells = <0>;
> > > > + };
> > > > +
> > > > soc {
> > > > compatible = "simple-bus";
> > > > interrupt-parent = <&plic>;
> > > > @@ -304,6 +311,24 @@ dmac0: dma-controller@ffefc00000 {
> > > > status = "disabled";
> > > > };
> > > >
> > > > + mmc0: mmc@ffe7080000 {
> > > > + compatible = "thead,th1520-dwcmshc";
> > > > + reg = <0xff 0xe7080000 0x0 0x10000>;
> > > > + interrupts = <62 IRQ_TYPE_LEVEL_HIGH>;
> > > > + clocks = <&sdhci_clk>;
> > > > + clock-names = "core";
> > > > + status = "disabled";
> > > > + };
> > > > +
> > > > + mmc1: mmc@ffe7090000 {
> > > > + compatible = "thead,th1520-dwcmshc";
> > > > + reg = <0xff 0xe7090000 0x0 0x10000>;
> > > > + interrupts = <64 IRQ_TYPE_LEVEL_HIGH>;
> > > > + clocks = <&sdhci_clk>;
> > > > + clock-names = "core";
> > > > + status = "disabled";
> > > > + };
> > > > +
> > >
> > > Hi Drew,
> > >
> > > This doesn't seem to match the documentation shared here:
> > > https://lore.kernel.org/linux-riscv/[email protected]/
> > > From the TH1520 System User Manual.pdf in there, I'd expect something like
> >
> > >
> > > emmc: mmc@ffe7080000 {
> > > compatible = "thead,th1520-dwcmshc";
> > > reg = <0xff 0xe7080000 0x0 0x10000>;
> > > ...
> > > };
> >
> > Hi Emil,
> >
> > I think this isn't necessary. From other soc dts files, I see such
> > naming, but lots socs just use mmc0, mmc1, and so on.
>
> No it certainly isn't necessary. Those labels are purely for us humans to read
> and are not present in the dtb. But that's exactly why I think it'd be a good
> idea match the labels with the documentation, so it will easier for us humans
> to match up the device tree source to documentation.
Hello together,

yes i agree to this too. It is easier for troubleshooting etc.

Damian

>
> > And IIRC, the host for sd and sdio can support both, IOW, below
> > sdio0/sdio1 may be used for sdcard.
>
> Yes, all of the EMMC, SDIO0 and SDIO1 seem to be instances of the same IP.
>
> /Emil