On Thu, 2013-12-05 at 22:08 +0100, Arnd Bergmann wrote:
> On Thursday 05 December 2013, dinguyen@xxxxxxxxxx wrote:
> > From: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> >
> > Re-use the "rockchip,rk2928-dw-mshc" binding that will support SD/MMC on
> > Altera's SOCFPGA platform.
> >
> > Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> > ---
> > v3: Re-use "rockchip,rk2928-dw-mshc" binding
> > v3: none
> > v2: none
> > ---
> > arch/arm/boot/dts/socfpga.dtsi | 11 +++++++++++
> > arch/arm/boot/dts/socfpga_arria5.dtsi | 12 ++++++++++++
> > arch/arm/boot/dts/socfpga_cyclone5.dtsi | 12 ++++++++++++
> > arch/arm/boot/dts/socfpga_vt.dts | 12 ++++++++++++
> > 4 files changed, 47 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> > index f936476..3d9f01b 100644
> > --- a/arch/arm/boot/dts/socfpga.dtsi
> > +++ b/arch/arm/boot/dts/socfpga.dtsi
> > @@ -469,6 +469,17 @@
> > cache-level = <2>;
> > };
> >
> > + mmc: dwmmc0@ff704000 {
> > + compatible = "rockchip,rk2928-dw-mshc";
> > + reg = <0xff704000 0x1000>;
> > + interrupts = <0 139 4>;
> > + fifo-depth = <0x400>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + clocks = <&l4_mp_clk>, <&sdmmc_clk>;
> > + clock-names = "biu", "ciu";
> > + };
>
>
> I think it's great that you can reuse the existing driver, but I'd recommend
> using a generic compatible string here in addition to one that identifies
> your specific implementation. You can list "rockchip,rk2928-dw-mshc" as
> well, but that is probably not necessary.
>
> What I'd expect to see here is either
>
> compatible = "altr,socfpga-dw-mshc", "rockchip,rk2928-dw-mshc", "snps,dw-mshc";
>
> or
>
> compatible = "altr,socfpga-dw-mshc", "rockchip,rk2928-dw-mshc", "snps,dw-mshc";
>
>
> It's probably not too late to generalize the SDMMC_CMD_USE_HOLD_REG
> handling, which is currently the only difference between the generic
> "snps,dw-mshc" and the newer "rockchip,rk2928-dw-mshc" variant.
Ok, I will try to generalize the driver.
>
> It's quite likely that all implementations should actually set
> SDMMC_CMD_USE_HOLD_REG (if both rockchips and altera set it) and you
> don't need to have any distinction in the dw_mmc-pltfm.c driver at all.
>
> If it's actually needed on some but not others, you could add a binary
> DT property to tell the driver about it rather than keying it off of
> the compatible string. If it's needed only on older (or only on newer)
> versions of the dw-mshc design, it could be encoded as a version in the
> string, such as "snps,dw-mshc-1.234".
Perhaps Seungwon and Chris might have an opinion on this:
>From the Synopsys databook for this IP, using the SDMMC_CMD_USE_HOLD_REG
is not recommended for SDR104, SDR50 and DDR50 speed modes. For other
speeds, SDR12, and SDR25, it would be necessary to use
SDMMC_CMD_USE_HOLD_REG.
I am referencing v2.50a of the Synopsys DesignWare Cores Mobile Storage
Host Databook.
So I think a binary DT property should suffice?
Thanks,
Dinh
>
> Arnd
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
