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:

quoted

On 26.03.2016 21:52, Marek Vasut wrote:

quoted

On 03/26/2016 07:16 PM, Vladimir Zapolskiy wrote:

quoted

On 26.03.2016 20:10, Marek Vasut wrote:

quoted

On 03/26/2016 06:52 PM, Vladimir Zapolskiy wrote:

quoted

Hi Marek,

On 26.03.2016 19:30, Marek Vasut wrote:

quoted

On 03/26/2016 06:26 PM, Vladimir Zapolskiy wrote:

quoted

On 26.03.2016 12:14, Marek Vasut wrote:

quoted

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.

quoted

quoted

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