Hi Jaehoon,

On Mon, 2016-03-28 at 19:34 +0900, Jaehoon Chung wrote:

Hi,

[snip]

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

So for socfpga platform compat string should be something like "snps,dw-mshc-socfpga" then?

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.

So card type is also important here and for certain card type we don't need to
set SDMMC_CMD_USE_HOLD_REG, right?

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

Given my assumption above we need to check 2 things:
 * Card type
 * SoC-specific implementation detail (phase shift scheme)

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

Ah actually 3 things
 + IMPLEMENT_HOLD_REG

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.

Hm, interesting looks like you already made some changes here:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=aaaaeb7a933471f6413ca44dd36efd57f2fa9429

So now driver checks if SoC has HOLD REG then SDMMC_CMD_USE_HOLD_REG will be set
(regardless card type).

And what's interesting and connected to this discussion since mentioned commit
there's no point in having both "altr,socfpga-dw-mshc" and "img,pistachio-dw-mshc"
compat strings because the do nothing now. I.e. it's time to replace both mentioned
compat strings with generic "snps,dw-mshc".

Anybody volunteers for that .dts* cleanup?

-Alexey