Re: ARC dw-mshc binding compat string

[Alexey Brodkin <Alexey.Brodkin@synopsys.com>]

Hi Marek, Vladimir,

On Sat, 2016-03-26 at 21:24 +0100, Marek Vasut wrote:
> On 03/26/2016 09:12 PM, Vladimir Zapolskiy wrote:
> > 
> > On 26.03.2016 21:52, Marek Vasut wrote:
> > > 
> > > On 03/26/2016 07:16 PM, Vladimir Zapolskiy wrote:
> > > > 
> > > > On 26.03.2016 20:10, Marek Vasut wrote:
> > > > > 
> > > > > On 03/26/2016 06:52 PM, Vladimir Zapolskiy wrote:
> > > > > > 
> > > > > > Hi Marek,
> > > > > > 
> > > > > > On 26.03.2016 19:30, Marek Vasut wrote:
> > > > > > > 
> > > > > > > On 03/26/2016 06:26 PM, Vladimir Zapolskiy wrote:
> > > > > > > > 
> > > > > > > > On 26.03.2016 12:14, Marek Vasut wrote:
> > > > > > > > > 
> > > > > > > > > Hi!
> > > > > > > > > 
> > > > > > > > > I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in
> > > > > > > > > the DT compatible string:
> > > > > > > > > 
> > > > > > > > > mmc@0x15000 {
> > > > > > > > >         compatible = "altr,socfpga-dw-mshc";
> > > > > > > > >         reg = < 0x15000 0x400 >;
> > > > > > > > >         num-slots = < 1 >;
> > > > > > > > >         fifo-depth = < 16 >;
> > > > > > > > >         card-detect-delay = < 200 >;
> > > > > > > > >         clocks = <&apbclk>, <&mmcclk>;
> > > > > > > > >         clock-names = "biu", "ciu";
> > > > > > > > >         interrupts = < 7 >;
> > > > > > > > >         bus-width = < 4 >;
> > > > > > > > > };
> > > > > > > > > 
> > > > > > > > > I don't think this is OK, since ARC is unrelated to Altera, which is
> > > > > > > > > what the "altr," prefix stands for. I think the socfpga-dw-mshc shim
> > > > > > > > > should be extended with another compatibility string, something like
> > > > > > > > > "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted
> > > > > > > > > accordingly. What do you think ?
> > > > > > > > > 
> > > > > > > > There is "snps,dw-mshc" described in
> > > > > > > > Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by
> > > > > > > > dw_mmc host controller driver.
> > > > > > > Thanks, that's even better.
> > > > > > > 
> > > > > > > btw what do you think of using altr, prefix on non-altera system, that
> > > > > > > doesn't seem ok, right ?
> > > > > > according to ePAPR the prefix should represent a device (IP block here
> > > > > > I believe) manufacturer, so it should be okay to use "altr" prefix on
> > > > > > non-Altera system, if Altera provides  another hardware vendor with
> > > > > > some own IP block.
> > > > > In this case, it's Synopsys who provides the SD/MMC/MS core to other
> > > > > chip makers (Altera etc).
> > > > Correct.
> > > > 
> > > > > 
> > > > > > 
> > > > > > That said, I would rather prefer to see "snps,dw-mshc" prefix on description
> > > > > > of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems
> > > > > > to be redundant.
> > > > > According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one
> > > > > "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one
> > > > > "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG),
> > > > > while the stock one "snps,dw-mshc" does not. I am not sure if the ARC
> > > > > one needs it as well, but most likely yes.
> > > > > 
> > > > > I wonder if that bit is needed on some particular version of the DWMMC
> > > > > core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN"
> > > > > binding ? Or should we use DT property to discern the need for this bit ?
> > > > > 
> > > > That's the most common way to take into account peculiarities, add
> > > > a property and handle it from the driver.
> > > And by "that" you mean which of those two I listed , the
> > > "snps,dw-mshc-vN" or adding new DT prop ?
> > > 
> > I meant to add a new property, not a new compatible, but that's just
> > my experience.
> > 
> > Let me say it __might__ happen that a particular change you need is
> > specific to a particular version of the DWMMC IP (query Synopsys
> > by the way), but more probably it might be e.g. the same IP version with
> > a different reduced or extended configuration or a minor fix/improvement
> > to the IP block without resulting version number bump.
> > 
> > For example I don't remember that errata fixes in IP blocks result in
> > a new compatible, instead there are quite common optional "quirk"
> > properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)
> Right, this very much matches how I see it as well. Thanks for confirming.
> 
> Alexey, can you tell us if the requirement for setting
> SDMMC_CMD_USE_HOLD_REG came with some new revision of the core or
> disappeared with some revision OR if this is some configuration
> option of the core during synthesis ?

Sorry for not following that discussion during my weekend but I'll try
to address all questions now.

DW Mobile Storage databook says:
--------------------->8-----------------------
To meet the relatively high Input Hold Time requirement for SDR12, SDR25,
and other MMC speed modes, you should program bit[29]use_hold_Reg of the
CMD register to 1'b1.
--------------------->8-----------------------

So I'd say this specific setting has nothing to do with a particular IP block
but instead it is related to card's mode of operation. More precisely bus clock.
SDR12 stands for 12.5 MByte/s, SDR25 stands for 25 MByte/s. I.e. we probably need
so set that bit just for certain cases and regardless board that uses DW MMC.

I'm adding DW MMC maintainer as well as linux-mmc mailing list so people who
understands that stuff better may comment here as well.

-Alexey