* Re: ARC dw-mshc binding compat string
2016-03-28 10:34 ` Jaehoon Chung
@ 2016-03-28 10:55 ` Alexey Brodkin
2016-03-28 11:44 ` Jaehoon Chung
2016-03-28 12:43 ` Rob Herring
2016-03-28 12:50 ` Marek Vasut
2 siblings, 1 reply; 9+ messages in thread
From: Alexey Brodkin @ 2016-03-28 10:55 UTC (permalink / raw)
To: jh80.chung@samsung.com
Cc: linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org,
marex@denx.de, vladimir_zapolskiy@mentor.com, robh+dt@kernel.org,
devicetree@vger.kernel.org
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: ARC dw-mshc binding compat string
2016-03-28 10:55 ` Alexey Brodkin
@ 2016-03-28 11:44 ` Jaehoon Chung
0 siblings, 0 replies; 9+ messages in thread
From: Jaehoon Chung @ 2016-03-28 11:44 UTC (permalink / raw)
To: Alexey Brodkin
Cc: linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org,
marex@denx.de, vladimir_zapolskiy@mentor.com, robh+dt@kernel.org,
devicetree@vger.kernel.org
On 03/28/2016 07:55 PM, Alexey Brodkin wrote:
> 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?
i think yes..since there is no SoC specific feature, isn't?
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> 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?
If you means card-type is card's speed mode, right..it's important.
In IP Databook, not mentioned about HS200 or HS400, but i thinks it doesn't need to set USE_HOLD_REG.
Because higher speed mode than 50MHz should be required the lowest input hold time. (~0.8ns)
>
>> 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)
In Exynos's case, there is CLKSEL register in DWMMC IP register.(as Vendor specific register.)
On other hands, rockchip is handling the phase sfiht with "clk_set_phase()"(as CMU.)
I can't know everything how they're implementing for shifting phase..
>
>> 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
Almost all have IMPLEMENT_HOLD_REG...?
If i will implement... in dw-mmc.c
switch (ios->timing) {
case SDR50/DDR50/.../
if (check cclk_in_drv > 0 for each SoC) {
SDMMC_CMD_USE_HOLD_REG is set to 1
break;
}
case SDR12/SD25
SDMMC_CMD_USE_HOLD_REG is set to 1
default:
SDMMC_CMD_USE_HOLD_REG is set to 0.
}
>
>> 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).
Yes, it's used by default..
Except exynos, other SoCs need to set this bit until now..otherwise it will be occurred CRC error. (Rockchip, Socfpga..)
>
> 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?
If have spare time to do cleanup, i will do for mshc compatible.
Best Regards,
Jaehoon Chung
>
> -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
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: ARC dw-mshc binding compat string
2016-03-28 10:34 ` Jaehoon Chung
2016-03-28 10:55 ` Alexey Brodkin
@ 2016-03-28 12:43 ` Rob Herring
2016-03-28 12:52 ` Marek Vasut
2016-03-28 12:50 ` Marek Vasut
2 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2016-03-28 12:43 UTC (permalink / raw)
To: Jaehoon Chung
Cc: Alexey Brodkin, marex@denx.de, vladimir_zapolskiy@mentor.com,
linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org,
devicetree@vger.kernel.org
On Mon, Mar 28, 2016 at 5:34 AM, Jaehoon Chung <jh80.chung@samsung.com> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: ARC dw-mshc binding compat string
2016-03-28 12:43 ` Rob Herring
@ 2016-03-28 12:52 ` Marek Vasut
0 siblings, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2016-03-28 12:52 UTC (permalink / raw)
To: Rob Herring, Jaehoon Chung
Cc: Alexey Brodkin, vladimir_zapolskiy@mentor.com,
linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org,
devicetree@vger.kernel.org
On 03/28/2016 02:43 PM, Rob Herring wrote:
> On Mon, Mar 28, 2016 at 5:34 AM, Jaehoon Chung <jh80.chung@samsung.com> 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.
This is exactly what I wanted to learn in this thread, thank you!
I knew I heard this argument somewhere already, but now I have it
confirmed that adding at least one SoC-specific compat is the way
to do it.
> Rob
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: ARC dw-mshc binding compat string
2016-03-28 10:34 ` Jaehoon Chung
2016-03-28 10:55 ` Alexey Brodkin
2016-03-28 12:43 ` Rob Herring
@ 2016-03-28 12:50 ` Marek Vasut
2016-03-28 16:16 ` Vladimir Zapolskiy
2 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2016-03-28 12:50 UTC (permalink / raw)
To: Jaehoon Chung, Alexey Brodkin, vladimir_zapolskiy@mentor.com
Cc: linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org,
robh+dt@kernel.org, devicetree@vger.kernel.org
On 03/28/2016 12:34 PM, Jaehoon Chung wrote:
> Hi,
Hi,
[...]
>>>>>>>> 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)
>>>>>>> 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
>
>>
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: ARC dw-mshc binding compat string
2016-03-28 12:50 ` Marek Vasut
@ 2016-03-28 16:16 ` Vladimir Zapolskiy
2016-03-28 19:00 ` Rob Herring
0 siblings, 1 reply; 9+ messages in thread
From: Vladimir Zapolskiy @ 2016-03-28 16:16 UTC (permalink / raw)
To: Marek Vasut, Jaehoon Chung, Alexey Brodkin, robh+dt@kernel.org
Cc: linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org,
devicetree@vger.kernel.org
Hi,
On 28.03.2016 15:50, Marek Vasut wrote:
> On 03/28/2016 12:34 PM, Jaehoon Chung wrote:
>> Hi,
>
> Hi,
>
> [...]
>
>>>>>>>>> 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)
IMO just 'compatible = "snps,dw-mshc"' is good enough, if it completely
describes the IP block on SoCFGPA --- and from what I get it is the case.
You can add a SoC-specific compatible if it is needed later on, and to my
taste only if SoC specific features can not be covered by properties.
The same sole "snps,dw-mshc" compatible is specified for NXP LPC18xx/43xx,
ZTE ZX and HiSilicon ARM SoCs.
Another similar example is ARM PrimeCell PLxxx IP blocks, as far as
I know there is no SoC-specific compatibles/aliases for PrimeCell IP blocks.
Rob, please correct me.
>>>>>>>> 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
>>
>>>
>>> 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
>>>
>>>
>>
>
>
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: ARC dw-mshc binding compat string
2016-03-28 16:16 ` Vladimir Zapolskiy
@ 2016-03-28 19:00 ` Rob Herring
0 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2016-03-28 19:00 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Marek Vasut, Jaehoon Chung, Alexey Brodkin,
linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org,
devicetree@vger.kernel.org
On Mon, Mar 28, 2016 at 11:16 AM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:
> Hi,
>
> On 28.03.2016 15:50, Marek Vasut wrote:
>> On 03/28/2016 12:34 PM, Jaehoon Chung wrote:
>>> Hi,
>>
>> Hi,
>>
>> [...]
>>
>>>>>>>>>> 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)
>
> IMO just 'compatible = "snps,dw-mshc"' is good enough, if it completely
> describes the IP block on SoCFGPA --- and from what I get it is the case.
> You can add a SoC-specific compatible if it is needed later on, and to my
> taste only if SoC specific features can not be covered by properties.
You can add the SoC-specific compatible string to the kernel later on.
You may not be able to update your DTB later on. So the specific
compatible strings need to be in the DT from the start.
There's no set rule on properties vs. implied by a compatible string,
but generally if it is fixed in the SoC, get the information based on
the compatible string. If it is a board level decision or has to be
tuned, then use a property.
> The same sole "snps,dw-mshc" compatible is specified for NXP LPC18xx/43xx,
> ZTE ZX and HiSilicon ARM SoCs.
They should be fixed.
> Another similar example is ARM PrimeCell PLxxx IP blocks, as far as
> I know there is no SoC-specific compatibles/aliases for PrimeCell IP blocks.
There are some for ST variants I think. PrimeCell blocks are a bit
different in that they generally pretty simple blocks, have not
changed much, and they
have a standard ID register that has been sufficient for determining
differences. And we have a standard way to override the ID register
when it is wrong. Look at recent patches for Denali NAND controller if
you want an example of why IP version registers (or compatible
strings) can't be trusted.
Once you get into IP blocks with lots of configuration options,
complicated clocking, power domains, and with phys on the front end,
then you hit all the integration differences.
Rob
^ permalink raw reply [flat|nested] 9+ messages in thread