On 03/28/2016 12:34 PM, Jaehoon Chung wrote:

Hi,

Hi,

[...]

quoted

quoted

quoted

quoted

quoted

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".

Should the compat string be
  compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc";
or just
  compatible = "snps,dw-mshc";
?

I am under the impression that a soc-specific identifier in addition to
a generic one (used by the driver compat table) is a good idea, because
it can help discerning the IP block from a generic one if needed at some
future point in time. It will also not break the DT for systems
which may depend on the non-generic compat, like *BSDs and such.

What do you think ? (btw this is very much my question in this thread)

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.

After read this thread entirely, i will check more detailed what you discussed.
If i missed something, let me know, plz.

Thanks for the clarification, linux-next indeed contains changes which
make snps,dw-mshc and altr,socfpga-dw-mshc equal.

Best Regards,
Jaehoon Chung

quoted

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--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Best regards,
Marek Vasut