On Mon, Mar 28, 2016 at 5:34 AM, Jaehoon Chung [off-list ref] wrote:

Hi,

On 03/28/2016 06:37 PM, Alexey Brodkin wrote:

quoted

Hi Marek, Vladimir,

On Sat, 2016-03-26 at 21:24 +0100, Marek Vasut wrote:

quoted

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.

Yes..it's redundant..i should be combined to "snps,dw-mshc".

socfpga is done correctly, IMO.

quoted

quoted

quoted

quoted

quoted

quoted

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.

SDMMC_CMD_USE_HOLD_REG didn't come with new revision..It's using continuously.
But it's difficult to use the generic feature..because it's considered the below things.

If Card is SDR50/SDR104/DDR50 mode..
        1) and phase shift of cclk_in_drv is 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 0,
        2) and phase shift of cclk_in_drv > 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 1,
If Card is SDR12/SDR25 mode, then this bit is set to 1.

We need to check phase shift scheme..but as i knew, each SoC have been implemented differently for phase shift.
(Phase shift have dependency to SoC.)

And it have to check HCON register..there is IMPLEMENT_HOLD_REG(bit[22]).
(It described whether IP have hold register or not)

I didn't read this thread entirely.
I'm not sure what you have discussed..but my understanding is right..i recommend to use "snps,dw-mshc" for ARC compat string.
Otherwise it need to add "dw_mmc-<SoC>.c". dw_mmc-pltfm.c should provide the basic dw-mmc controller functionality.

You should use "snps,dw-mshc", but there should also be an SoC
specific compatible string. There are always integration differences
and even using just versions of IP blocks is not specific enough. This
should not require another driver file.

Rob