Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
From:   Emil Renner Berthing <emil.renner.berthing@canonical.com>
In-Reply-To: <ZW8zHoVWf94FDTx7@xhacker>
References: <20231129-th1520_mmc_dts-v7-0-c77fc19caa6f@baylibre.com>
 <20231129-th1520_mmc_dts-v7-2-c77fc19caa6f@baylibre.com> <CAJM55Z-qc7gc0fO-K8byqvpOBjDxFD4dP57mFHuijvpOzBWObQ@mail.gmail.com>
 <ZW8zHoVWf94FDTx7@xhacker>
Mime-Version: 1.0
Date:   Tue, 5 Dec 2023 06:48:16 -0800
Message-ID: <CAJM55Z9tPTu4N4UVCBs1AJYWrWxMUDL+8FpVZtGx4yQgKenWSQ@mail.gmail.com>
Subject: Re: [PATCH v7 2/4] riscv: dts: thead: Add TH1520 mmc controllers and
 sdhci clock
To:     Jisheng Zhang <jszhang@kernel.org>,
        Emil Renner Berthing <emil.renner.berthing@canonical.com>
Cc:     Drew Fustini <dfustini@baylibre.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>, Guo Ren <guoren@kernel.org>,
        Fu Wei <wefu@redhat.com>, Conor Dooley <conor@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jason Kridner <jkridner@beagleboard.org>,
        Robert Nelson <robertcnelson@beagleboard.org>,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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 <dfustini@baylibre.com>
> > > ---
> > >  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/5f437109d2be2b8843f549a661054a2e3ec0d66e.camel@xry111.site/
> > 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