Hi Jaehoon, On Mon, 2016-03-28 at 19:34 +0900, Jaehoon Chung wrote: > Hi,
[snip] > > > > > > > > > > > > > > > > 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? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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