Re: ARC dw-mshc binding compat string

[Rob Herring]

On Mon, Mar 28, 2016 at 5:34 AM, Jaehoon Chung <jh80.chung@xxxxxxxxxxx> wrote:
> Hi,
>
> On 03/28/2016 06:37 PM, Alexey Brodkin wrote:
>> Hi Marek, Vladimir,
>>
>> On Sat, 2016-03-26 at 21:24 +0100, Marek Vasut wrote:
>>> On 03/26/2016 09:12 PM, Vladimir Zapolskiy wrote:
>>>>
>>>> On 26.03.2016 21:52, Marek Vasut wrote:
>>>>>
>>>>> On 03/26/2016 07:16 PM, Vladimir Zapolskiy wrote:
>>>>>>
>>>>>> On 26.03.2016 20:10, Marek Vasut wrote:
>>>>>>>
>>>>>>> On 03/26/2016 06:52 PM, Vladimir Zapolskiy wrote:
>>>>>>>>
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On 26.03.2016 19:30, Marek Vasut wrote:
>>>>>>>>>
>>>>>>>>> On 03/26/2016 06:26 PM, Vladimir Zapolskiy wrote:
>>>>>>>>>>
>>>>>>>>>> On 26.03.2016 12:14, Marek Vasut wrote:
>>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> 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.

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