* [RFC 0/2] mmc: sdhci: Fix sdhci caps register bits with corrections provided by dt
@ 2016-10-25 19:58 Zach Brown
2016-10-25 19:58 ` [RFC 1/2] mmc: sdhci: dt: Add device tree properties sdhci-caps and sdhci-caps-mask Zach Brown
2016-10-25 19:58 ` [RFC 2/2] mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read during __sdhci_read_caps Zach Brown
0 siblings, 2 replies; 8+ messages in thread
From: Zach Brown @ 2016-10-25 19:58 UTC (permalink / raw)
To: ulf.hansson-QSEj5FYQhm4dnm+yROfE0A
Cc: adrian.hunter-ral2JQCrhuEAvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, zach.brown-acOepvfBmUk
For various reasons the sdhci caps register can be incorrect. This patch set
introduces a general way to correct the bits when they are read to accurately
reflect the capabilties of the controller/board combo.
The first patch creates sdhci-caps and sdhci-caps-mask dt properties that
combined represent the correction to the sdhci caps register.
The second patch uses the new dt properties to correct the caps from the
register as they read during __sdhci_read_caps.
Zach Brown (2):
mmc: sdhci: dt: Add device tree properties sdhci-caps and sdhci-caps-mask
mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read
during __sdhci_read_caps
Documentation/devicetree/bindings/mmc/mmc.txt | 7 +++++++
drivers/mmc/host/sdhci.c | 24 ++++++++++++++++++++++--
2 files changed, 29 insertions(+), 2 deletions(-)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC 1/2] mmc: sdhci: dt: Add device tree properties sdhci-caps and sdhci-caps-mask
2016-10-25 19:58 [RFC 0/2] mmc: sdhci: Fix sdhci caps register bits with corrections provided by dt Zach Brown
@ 2016-10-25 19:58 ` Zach Brown
[not found] ` <1477425538-3315-2-git-send-email-zach.brown-acOepvfBmUk@public.gmane.org>
2016-10-25 19:58 ` [RFC 2/2] mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read during __sdhci_read_caps Zach Brown
1 sibling, 1 reply; 8+ messages in thread
From: Zach Brown @ 2016-10-25 19:58 UTC (permalink / raw)
To: ulf.hansson
Cc: adrian.hunter, robh+dt, mark.rutland, linux-mmc, devicetree,
linux-kernel, zach.brown
On some systems the sdhci capabilty registers are incorrect for one
reason or another.
The sdhci-caps-mask property specifies which bits in the registers
are incorrect and should be turned off before using sdhci-caps to turn
on bits.
The sdhci-caps property specifies which bits should be turned on.
Signed-off-by: Zach Brown <zach.brown@ni.com>
---
Documentation/devicetree/bindings/mmc/mmc.txt | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 8a37782..1415aa0 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -52,6 +52,13 @@ Optional properties:
- no-sdio: controller is limited to send sdio cmd during initialization
- no-sd: controller is limited to send sd cmd during initialization
- no-mmc: controller is limited to send mmc cmd during initialization
+- sdhci-caps-mask: The sdhci capabilities registers are incorrect. This 64bit
+ property corresponds to the bits in the sdhci capabilty registers. If the bit
+ is on in the mask then the bit is incorrect in the registers and should be
+ turned off.
+- sdhci-caps: The sdhci capabilities registers are incorrect. This 64bit
+ property corresponds to the bits in the sdhci capability registers. If the
+ bit is on in the property then the bit should be on in the reigsters.
*NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
polarity properties, we have to fix the meaning of the "normal" and "inverted"
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [RFC 2/2] mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read during __sdhci_read_caps
2016-10-25 19:58 [RFC 0/2] mmc: sdhci: Fix sdhci caps register bits with corrections provided by dt Zach Brown
2016-10-25 19:58 ` [RFC 1/2] mmc: sdhci: dt: Add device tree properties sdhci-caps and sdhci-caps-mask Zach Brown
@ 2016-10-25 19:58 ` Zach Brown
1 sibling, 0 replies; 8+ messages in thread
From: Zach Brown @ 2016-10-25 19:58 UTC (permalink / raw)
To: ulf.hansson
Cc: adrian.hunter, robh+dt, mark.rutland, linux-mmc, devicetree,
linux-kernel, zach.brown
The sdhci capabilities registers can be incorrect. The sdhci-caps-mask
and sdhci-caps dt properties specify which bits of the registers are
incorrect and what their values should be. This patch makes the sdhci
driver use those properties to correct the caps during
__sdhci_read_caps.
During __sdhci_read_caps
Use the sdhci-caps-mask property to turn off the incorrect bits of the
sdhci registers after reading them.
Use the sdhci-caps to turn on bits after using sdhci-caps-mask to turn
off the incorrect ones.
Signed-off-by: Zach Brown <zach.brown@ni.com>
---
drivers/mmc/host/sdhci.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1e25b01..d5feae4 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -22,6 +22,7 @@
#include <linux/scatterlist.h>
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
+#include <linux/of.h>
#include <linux/leds.h>
@@ -2991,6 +2992,8 @@ static int sdhci_set_dma_mask(struct sdhci_host *host)
void __sdhci_read_caps(struct sdhci_host *host, u16 *ver, u32 *caps, u32 *caps1)
{
u16 v;
+ u64 dt_caps_mask = 0;
+ u64 dt_caps = 0;
if (host->read_caps)
return;
@@ -3005,18 +3008,35 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 *ver, u32 *caps, u32 *caps1)
sdhci_do_reset(host, SDHCI_RESET_ALL);
+ of_property_read_u64(mmc_dev(host->mmc)->of_node,
+ "sdhci-caps-mask", &dt_caps_mask);
+ of_property_read_u64(mmc_dev(host->mmc)->of_node,
+ "sdhci-caps", &dt_caps);
+
v = ver ? *ver : sdhci_readw(host, SDHCI_HOST_VERSION);
host->version = (v & SDHCI_SPEC_VER_MASK) >> SDHCI_SPEC_VER_SHIFT;
if (host->quirks & SDHCI_QUIRK_MISSING_CAPS)
return;
- host->caps = caps ? *caps : sdhci_readl(host, SDHCI_CAPABILITIES);
+ if (caps)
+ host->caps = *caps;
+ else {
+ host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+ host->caps &= ~lower_32_bits(dt_caps_mask);
+ host->caps |= lower_32_bits(dt_caps);
+ }
if (host->version < SDHCI_SPEC_300)
return;
- host->caps1 = caps1 ? *caps1 : sdhci_readl(host, SDHCI_CAPABILITIES_1);
+ if (caps1)
+ host->caps1 = *caps1;
+ else {
+ host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
+ host->caps1 &= ~upper_32_bits(dt_caps_mask);
+ host->caps1 |= upper_32_bits(dt_caps);
+ }
}
EXPORT_SYMBOL_GPL(__sdhci_read_caps);
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC 1/2] mmc: sdhci: dt: Add device tree properties sdhci-caps and sdhci-caps-mask
[not found] ` <1477425538-3315-2-git-send-email-zach.brown-acOepvfBmUk@public.gmane.org>
@ 2016-10-28 8:12 ` Ulf Hansson
[not found] ` <CAPDyKFpKoXd9yuX5vR-CACtXKGysmV0AqKnjxQ+hTGU+kgwceQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Ulf Hansson @ 2016-10-28 8:12 UTC (permalink / raw)
To: Zach Brown
Cc: Adrian Hunter, Rob Herring, Mark Rutland, linux-mmc,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 25 October 2016 at 21:58, Zach Brown <zach.brown-acOepvfBmUk@public.gmane.org> wrote:
> On some systems the sdhci capabilty registers are incorrect for one
> reason or another.
>
> The sdhci-caps-mask property specifies which bits in the registers
> are incorrect and should be turned off before using sdhci-caps to turn
> on bits.
>
> The sdhci-caps property specifies which bits should be turned on.
>
> Signed-off-by: Zach Brown <zach.brown-acOepvfBmUk@public.gmane.org>
> ---
> Documentation/devicetree/bindings/mmc/mmc.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> index 8a37782..1415aa0 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
The bindings in this document are common mmc DT bindings, not bindings
specific to a mmc controller.
So unless these bindings are applicable for another controller than
sdhci, I suggest we create a new file to document these.
How about Documentation/devicetree/bindings/mmc/sdhci.txt?
> @@ -52,6 +52,13 @@ Optional properties:
> - no-sdio: controller is limited to send sdio cmd during initialization
> - no-sd: controller is limited to send sd cmd during initialization
> - no-mmc: controller is limited to send mmc cmd during initialization
> +- sdhci-caps-mask: The sdhci capabilities registers are incorrect. This 64bit
/s/registers/register
This applies to some more places below as well.
> + property corresponds to the bits in the sdhci capabilty registers. If the bit
> + is on in the mask then the bit is incorrect in the registers and should be
> + turned off.
> +- sdhci-caps: The sdhci capabilities registers are incorrect. This 64bit
> + property corresponds to the bits in the sdhci capability registers. If the
> + bit is on in the property then the bit should be on in the reigsters.
/s/reigsters/register
>
> *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
> polarity properties, we have to fix the meaning of the "normal" and "inverted"
> --
> 2.7.4
>
Overall, I like this idea as it gives us good flexibility. Thus it
should avoid us to having to add any further new similar "sdhci broken
cap" DT binding. We could also decide to start deprecate some of the
existing sdhci bindings, if we think that makes sense.
The downside is that we get a "magic" hex value in the dts. Although,
people could address this issue by providing some comments about what
the bits it means in the dts files themselves.
Let's see what Rob thinks about this.
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 1/2] mmc: sdhci: dt: Add device tree properties sdhci-caps and sdhci-caps-mask
[not found] ` <CAPDyKFpKoXd9yuX5vR-CACtXKGysmV0AqKnjxQ+hTGU+kgwceQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-10-31 11:59 ` Jaehoon Chung
[not found] ` <6031045c-0eb2-7bea-1efd-874dade0f009-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Jaehoon Chung @ 2016-10-31 11:59 UTC (permalink / raw)
To: Ulf Hansson, Zach Brown
Cc: Adrian Hunter, Rob Herring, Mark Rutland, linux-mmc,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 10/28/2016 05:12 PM, Ulf Hansson wrote:
> On 25 October 2016 at 21:58, Zach Brown <zach.brown-acOepvfBmUk@public.gmane.org> wrote:
>> On some systems the sdhci capabilty registers are incorrect for one
>> reason or another.
>>
>> The sdhci-caps-mask property specifies which bits in the registers
>> are incorrect and should be turned off before using sdhci-caps to turn
>> on bits.
>>
>> The sdhci-caps property specifies which bits should be turned on.
>>
>> Signed-off-by: Zach Brown <zach.brown-acOepvfBmUk@public.gmane.org>
>> ---
>> Documentation/devicetree/bindings/mmc/mmc.txt | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
>> index 8a37782..1415aa0 100644
>> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
>
> The bindings in this document are common mmc DT bindings, not bindings
> specific to a mmc controller.
>
> So unless these bindings are applicable for another controller than
> sdhci, I suggest we create a new file to document these.
> How about Documentation/devicetree/bindings/mmc/sdhci.txt?
>
>> @@ -52,6 +52,13 @@ Optional properties:
>> - no-sdio: controller is limited to send sdio cmd during initialization
>> - no-sd: controller is limited to send sd cmd during initialization
>> - no-mmc: controller is limited to send mmc cmd during initialization
>> +- sdhci-caps-mask: The sdhci capabilities registers are incorrect. This 64bit
>
> /s/registers/register
>
> This applies to some more places below as well.
>
>> + property corresponds to the bits in the sdhci capabilty registers. If the bit
>> + is on in the mask then the bit is incorrect in the registers and should be
>> + turned off.
>> +- sdhci-caps: The sdhci capabilities registers are incorrect. This 64bit
>> + property corresponds to the bits in the sdhci capability registers. If the
>> + bit is on in the property then the bit should be on in the reigsters.
>
> /s/reigsters/register
>
>>
>> *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
>> polarity properties, we have to fix the meaning of the "normal" and "inverted"
>> --
>> 2.7.4
>>
>
> Overall, I like this idea as it gives us good flexibility. Thus it
> should avoid us to having to add any further new similar "sdhci broken
> cap" DT binding. We could also decide to start deprecate some of the
> existing sdhci bindings, if we think that makes sense.
>
> The downside is that we get a "magic" hex value in the dts. Although,
> people could address this issue by providing some comments about what
> the bits it means in the dts files themselves.
I think it's not good about getting "magic" hex value.
In my experience, it's too difficult what bits means and calculate..
Because some people who i know had already used like this.(locally..)
It needs to consider this...otherwise..it should become really complex magic code.
Best Regards,
Jaehoon Chung
>
> Let's see what Rob thinks about this.
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 1/2] mmc: sdhci: dt: Add device tree properties sdhci-caps and sdhci-caps-mask
[not found] ` <6031045c-0eb2-7bea-1efd-874dade0f009-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2016-10-31 12:34 ` Adrian Hunter
2016-10-31 22:13 ` Jaehoon Chung
0 siblings, 1 reply; 8+ messages in thread
From: Adrian Hunter @ 2016-10-31 12:34 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson, Zach Brown
Cc: Rob Herring, Mark Rutland, linux-mmc,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 31/10/16 13:59, Jaehoon Chung wrote:
> On 10/28/2016 05:12 PM, Ulf Hansson wrote:
>> On 25 October 2016 at 21:58, Zach Brown <zach.brown-acOepvfBmUk@public.gmane.org> wrote:
>>> On some systems the sdhci capabilty registers are incorrect for one
>>> reason or another.
>>>
>>> The sdhci-caps-mask property specifies which bits in the registers
>>> are incorrect and should be turned off before using sdhci-caps to turn
>>> on bits.
>>>
>>> The sdhci-caps property specifies which bits should be turned on.
>>>
>>> Signed-off-by: Zach Brown <zach.brown-acOepvfBmUk@public.gmane.org>
>>> ---
>>> Documentation/devicetree/bindings/mmc/mmc.txt | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
>>> index 8a37782..1415aa0 100644
>>> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
>>> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
>>
>> The bindings in this document are common mmc DT bindings, not bindings
>> specific to a mmc controller.
>>
>> So unless these bindings are applicable for another controller than
>> sdhci, I suggest we create a new file to document these.
>> How about Documentation/devicetree/bindings/mmc/sdhci.txt?
>>
>>> @@ -52,6 +52,13 @@ Optional properties:
>>> - no-sdio: controller is limited to send sdio cmd during initialization
>>> - no-sd: controller is limited to send sd cmd during initialization
>>> - no-mmc: controller is limited to send mmc cmd during initialization
>>> +- sdhci-caps-mask: The sdhci capabilities registers are incorrect. This 64bit
>>
>> /s/registers/register
>>
>> This applies to some more places below as well.
>>
>>> + property corresponds to the bits in the sdhci capabilty registers. If the bit
>>> + is on in the mask then the bit is incorrect in the registers and should be
>>> + turned off.
>>> +- sdhci-caps: The sdhci capabilities registers are incorrect. This 64bit
>>> + property corresponds to the bits in the sdhci capability registers. If the
>>> + bit is on in the property then the bit should be on in the reigsters.
>>
>> /s/reigsters/register
>>
>>>
>>> *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
>>> polarity properties, we have to fix the meaning of the "normal" and "inverted"
>>> --
>>> 2.7.4
>>>
>>
>> Overall, I like this idea as it gives us good flexibility. Thus it
>> should avoid us to having to add any further new similar "sdhci broken
>> cap" DT binding. We could also decide to start deprecate some of the
>> existing sdhci bindings, if we think that makes sense.
>>
>> The downside is that we get a "magic" hex value in the dts. Although,
>> people could address this issue by providing some comments about what
>> the bits it means in the dts files themselves.
>
> I think it's not good about getting "magic" hex value.
> In my experience, it's too difficult what bits means and calculate..
> Because some people who i know had already used like this.(locally..)
>
> It needs to consider this...otherwise..it should become really complex magic code.
The bits we use are listed in sdhci.h and how we use them can be determined
from the sdhci source code. Also, from the hardware perspective, there is
the SDHCI specification. So what the bits mean is readily available.
With regard to calculating the values, won't it be obvious from testing if
they are wrong?
>
> Best Regards,
> Jaehoon Chung
>
>>
>> Let's see what Rob thinks about this.
>>
>> Kind regards
>> Uffe
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 1/2] mmc: sdhci: dt: Add device tree properties sdhci-caps and sdhci-caps-mask
2016-10-31 12:34 ` Adrian Hunter
@ 2016-10-31 22:13 ` Jaehoon Chung
2016-11-01 15:08 ` Zach Brown
0 siblings, 1 reply; 8+ messages in thread
From: Jaehoon Chung @ 2016-10-31 22:13 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson, Zach Brown
Cc: Rob Herring, Mark Rutland, linux-mmc, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
On 10/31/2016 09:34 PM, Adrian Hunter wrote:
> On 31/10/16 13:59, Jaehoon Chung wrote:
>> On 10/28/2016 05:12 PM, Ulf Hansson wrote:
>>> On 25 October 2016 at 21:58, Zach Brown <zach.brown@ni.com> wrote:
>>>> On some systems the sdhci capabilty registers are incorrect for one
>>>> reason or another.
>>>>
>>>> The sdhci-caps-mask property specifies which bits in the registers
>>>> are incorrect and should be turned off before using sdhci-caps to turn
>>>> on bits.
>>>>
>>>> The sdhci-caps property specifies which bits should be turned on.
>>>>
>>>> Signed-off-by: Zach Brown <zach.brown@ni.com>
>>>> ---
>>>> Documentation/devicetree/bindings/mmc/mmc.txt | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
>>>> index 8a37782..1415aa0 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
>>>> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
>>>
>>> The bindings in this document are common mmc DT bindings, not bindings
>>> specific to a mmc controller.
>>>
>>> So unless these bindings are applicable for another controller than
>>> sdhci, I suggest we create a new file to document these.
>>> How about Documentation/devicetree/bindings/mmc/sdhci.txt?
>>>
>>>> @@ -52,6 +52,13 @@ Optional properties:
>>>> - no-sdio: controller is limited to send sdio cmd during initialization
>>>> - no-sd: controller is limited to send sd cmd during initialization
>>>> - no-mmc: controller is limited to send mmc cmd during initialization
>>>> +- sdhci-caps-mask: The sdhci capabilities registers are incorrect. This 64bit
>>>
>>> /s/registers/register
>>>
>>> This applies to some more places below as well.
>>>
>>>> + property corresponds to the bits in the sdhci capabilty registers. If the bit
>>>> + is on in the mask then the bit is incorrect in the registers and should be
>>>> + turned off.
>>>> +- sdhci-caps: The sdhci capabilities registers are incorrect. This 64bit
>>>> + property corresponds to the bits in the sdhci capability registers. If the
>>>> + bit is on in the property then the bit should be on in the reigsters.
>>>
>>> /s/reigsters/register
>>>
>>>>
>>>> *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
>>>> polarity properties, we have to fix the meaning of the "normal" and "inverted"
>>>> --
>>>> 2.7.4
>>>>
>>>
>>> Overall, I like this idea as it gives us good flexibility. Thus it
>>> should avoid us to having to add any further new similar "sdhci broken
>>> cap" DT binding. We could also decide to start deprecate some of the
>>> existing sdhci bindings, if we think that makes sense.
>>>
>>> The downside is that we get a "magic" hex value in the dts. Although,
>>> people could address this issue by providing some comments about what
>>> the bits it means in the dts files themselves.
>>
>> I think it's not good about getting "magic" hex value.
>> In my experience, it's too difficult what bits means and calculate..
>> Because some people who i know had already used like this.(locally..)
>>
>> It needs to consider this...otherwise..it should become really complex magic code.
>
> The bits we use are listed in sdhci.h and how we use them can be determined
> from the sdhci source code. Also, from the hardware perspective, there is
> the SDHCI specification. So what the bits mean is readily available.
>
> With regard to calculating the values, won't it be obvious from testing if
> they are wrong?
You're right. But I didn't see the real use case for this properties.
If it needs to add these properties, why didn't add codes relevant to these in device-tree?
Otherwise, this code should be dead code.
Best Regards,
Jaehoon Chung
>
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>> Let's see what Rob thinks about this.
>>>
>>> Kind regards
>>> Uffe
>>> --
>>> 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
>>>
>>>
>>>
>>
>>
>
> --
> 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] 8+ messages in thread
* Re: [RFC 1/2] mmc: sdhci: dt: Add device tree properties sdhci-caps and sdhci-caps-mask
2016-10-31 22:13 ` Jaehoon Chung
@ 2016-11-01 15:08 ` Zach Brown
0 siblings, 0 replies; 8+ messages in thread
From: Zach Brown @ 2016-11-01 15:08 UTC (permalink / raw)
To: Jaehoon Chung
Cc: Adrian Hunter, Ulf Hansson, Rob Herring, Mark Rutland, linux-mmc,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Tue, Nov 01, 2016 at 07:13:29AM +0900, Jaehoon Chung wrote:
> On 10/31/2016 09:34 PM, Adrian Hunter wrote:
> > On 31/10/16 13:59, Jaehoon Chung wrote:
> >> On 10/28/2016 05:12 PM, Ulf Hansson wrote:
> >>> On 25 October 2016 at 21:58, Zach Brown <zach.brown@ni.com> wrote:
> >>>> On some systems the sdhci capabilty registers are incorrect for one
> >>>> reason or another.
> >>>>
> >>>> The sdhci-caps-mask property specifies which bits in the registers
> >>>> are incorrect and should be turned off before using sdhci-caps to turn
> >>>> on bits.
> >>>>
> >>>> The sdhci-caps property specifies which bits should be turned on.
> >>>>
> >>>> Signed-off-by: Zach Brown <zach.brown@ni.com>
> >>>> ---
> >>>> Documentation/devicetree/bindings/mmc/mmc.txt | 7 +++++++
> >>>> 1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> >>>> index 8a37782..1415aa0 100644
> >>>> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> >>>> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> >>>
> >>> The bindings in this document are common mmc DT bindings, not bindings
> >>> specific to a mmc controller.
> >>>
> >>> So unless these bindings are applicable for another controller than
> >>> sdhci, I suggest we create a new file to document these.
> >>> How about Documentation/devicetree/bindings/mmc/sdhci.txt?
> >>>
> >>>> @@ -52,6 +52,13 @@ Optional properties:
> >>>> - no-sdio: controller is limited to send sdio cmd during initialization
> >>>> - no-sd: controller is limited to send sd cmd during initialization
> >>>> - no-mmc: controller is limited to send mmc cmd during initialization
> >>>> +- sdhci-caps-mask: The sdhci capabilities registers are incorrect. This 64bit
> >>>
> >>> /s/registers/register
> >>>
> >>> This applies to some more places below as well.
> >>>
> >>>> + property corresponds to the bits in the sdhci capabilty registers. If the bit
> >>>> + is on in the mask then the bit is incorrect in the registers and should be
> >>>> + turned off.
> >>>> +- sdhci-caps: The sdhci capabilities registers are incorrect. This 64bit
> >>>> + property corresponds to the bits in the sdhci capability registers. If the
> >>>> + bit is on in the property then the bit should be on in the reigsters.
> >>>
> >>> /s/reigsters/register
> >>>
> >>>>
> >>>> *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
> >>>> polarity properties, we have to fix the meaning of the "normal" and "inverted"
> >>>> --
> >>>> 2.7.4
> >>>>
> >>>
> >>> Overall, I like this idea as it gives us good flexibility. Thus it
> >>> should avoid us to having to add any further new similar "sdhci broken
> >>> cap" DT binding. We could also decide to start deprecate some of the
> >>> existing sdhci bindings, if we think that makes sense.
> >>>
> >>> The downside is that we get a "magic" hex value in the dts. Although,
> >>> people could address this issue by providing some comments about what
> >>> the bits it means in the dts files themselves.
> >>
> >> I think it's not good about getting "magic" hex value.
> >> In my experience, it's too difficult what bits means and calculate..
> >> Because some people who i know had already used like this.(locally..)
> >>
> >> It needs to consider this...otherwise..it should become really complex magic code.
> >
> > The bits we use are listed in sdhci.h and how we use them can be determined
> > from the sdhci source code. Also, from the hardware perspective, there is
> > the SDHCI specification. So what the bits mean is readily available.
> >
> > With regard to calculating the values, won't it be obvious from testing if
> > they are wrong?
>
> You're right. But I didn't see the real use case for this properties.
> If it needs to add these properties, why didn't add codes relevant to these in device-tree?
> Otherwise, this code should be dead code.
>
The issue here is the sdhci has a register that is supposed report the
capabilities of the device, but it can be wrong. It might turn on a mode or
behavior that should not be on.
For example, in our use case the sdhci itself is capable of highspeed so it
naturally has the highspeed capable bit set in it's capabilities register.
However due to board setup, the entire system is not actually capable of
highspeed. So we need a way to say the sdhci capabilities register's highspeed
bit is incorrect.
A simple "no-sdhci-highspeed" device tree property might suffice, but there are
roughly ~64 sdhci capabilities represented in the capabilities register.
Instead of creating ~64 device-tree properties to handle each individually this
patch set creates just two that handle them all. In a way that is flexible
enough to correct all ~64 or just 1 or any subset depending on the use case.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-11-01 15:08 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-25 19:58 [RFC 0/2] mmc: sdhci: Fix sdhci caps register bits with corrections provided by dt Zach Brown
2016-10-25 19:58 ` [RFC 1/2] mmc: sdhci: dt: Add device tree properties sdhci-caps and sdhci-caps-mask Zach Brown
[not found] ` <1477425538-3315-2-git-send-email-zach.brown-acOepvfBmUk@public.gmane.org>
2016-10-28 8:12 ` Ulf Hansson
[not found] ` <CAPDyKFpKoXd9yuX5vR-CACtXKGysmV0AqKnjxQ+hTGU+kgwceQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-10-31 11:59 ` Jaehoon Chung
[not found] ` <6031045c-0eb2-7bea-1efd-874dade0f009-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2016-10-31 12:34 ` Adrian Hunter
2016-10-31 22:13 ` Jaehoon Chung
2016-11-01 15:08 ` Zach Brown
2016-10-25 19:58 ` [RFC 2/2] mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read during __sdhci_read_caps Zach Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).