Messages in this thread | | | From | Alexey Brodkin <> | Subject | Re: ARC dw-mshc binding compat string | Date | Mon, 28 Mar 2016 10:55:37 +0000 |
| |
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
| |