Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Arnd Bergmann]

On Tuesday 21 August 2012, Thomas Abraham wrote:
> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> index 8a6811f..1aa527a 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> @@ -16,6 +16,8 @@ Optional properties:
>  - wp-inverted: when present, polarity on the wp gpio line is inverted
>  - non-removable: non-removable slot (like eMMC)
>  - max-frequency: maximum operating clock frequency
> +- broken-cd: when present, indicates that the cd-gpios line is not
> +       connected to the card-detect pad of the MMC host controller.

What is the difference between listing the cd line as broken and
listing no cd line at all?

	Arnd

[PATCH] mmc: dt: Add 'broken-cd' DT binding

[Thomas Abraham]

'broken-cd' binding lets mmc controller device node to indicate that
the card detect line is broken.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
The 'broken-cd' DT binding for MMC controllers is picked up from the OLPC
project git repo and was originally conceived by Chris Ball <cjb@laptop.org>.

 Documentation/devicetree/bindings/mmc/mmc.txt |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 8a6811f..1aa527a 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -16,6 +16,8 @@ Optional properties:
 - wp-inverted: when present, polarity on the wp gpio line is inverted
 - non-removable: non-removable slot (like eMMC)
 - max-frequency: maximum operating clock frequency
+- broken-cd: when present, indicates that the cd-gpios line is not
+	connected to the card-detect pad of the MMC host controller.
 
 Example:
 
-- 
1.6.6.rc2

[PATCH] mmc: dt: Add 'broken-cd' DT binding

[Thomas Abraham]

'broken-cd' binding lets mmc controller device node to indicate that
the card detect line is broken.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
The 'broken-cd' DT binding for MMC controllers is picked up from the OLPC
project git repo and was originally conceived by Chris Ball <cjb@laptop.org>.

 Documentation/devicetree/bindings/mmc/mmc.txt |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 8a6811f..1aa527a 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -16,6 +16,8 @@ Optional properties:
 - wp-inverted: when present, polarity on the wp gpio line is inverted
 - non-removable: non-removable slot (like eMMC)
 - max-frequency: maximum operating clock frequency
+- broken-cd: when present, indicates that the cd-gpios line is not
+	connected to the card-detect pad of the MMC host controller.
 
 Example:
 
-- 
1.6.6.rc2

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Chris Ball]

Hi,

On Tue, Aug 21 2012, Arnd Bergmann wrote:
> On Tuesday 21 August 2012, Thomas Abraham wrote:
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt
>> b/Documentation/devicetree/bindings/mmc/mmc.txt
>> index 8a6811f..1aa527a 100644
>> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
>> @@ -16,6 +16,8 @@ Optional properties:
>>  - wp-inverted: when present, polarity on the wp gpio line is inverted
>>  - non-removable: non-removable slot (like eMMC)
>>  - max-frequency: maximum operating clock frequency
>> +- broken-cd: when present, indicates that the cd-gpios line is not
>> +       connected to the card-detect pad of the MMC host controller.
>
> What is the difference between listing the cd line as broken and
> listing no cd line at all?

I think the documentation's incorrect here -- "broken-cd" should be used
without a cd-gpios node, and tells the driver that there is no working
WP and we'll need to poll the host (SDHCI_QUIRK_BROKEN_CARD_DETECTION):

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index 25733ef..23ebdbb 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -198,6 +198,9 @@ static struct sdhci_pxa_platdata *pxav3_get_mmc_pdata(struct device *dev)
 	if (clk_delay_cycles > 0)
 		pdata->clk_delay_cycles = clk_delay_cycles;
 
+	if (of_find_property(np, "broken-cd", NULL))
+		pdata->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
+
 	return pdata;
 }
 #else

So, I'd say:

"- broken-cd: when present, indicates that there is no card-detect
   line available on the host; polling should be used instead."

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Thomas Abraham]

On 21 August 2012 16:31, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tuesday 21 August 2012, Thomas Abraham wrote:
> > diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt
> > b/Documentation/devicetree/bindings/mmc/mmc.txt
> > index 8a6811f..1aa527a 100644
> > --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> > +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> > @@ -16,6 +16,8 @@ Optional properties:
> >  - wp-inverted: when present, polarity on the wp gpio line is inverted
> >  - non-removable: non-removable slot (like eMMC)
> >  - max-frequency: maximum operating clock frequency
> > +- broken-cd: when present, indicates that the cd-gpios line is not
> > +       connected to the card-detect pad of the MMC host controller.
>
> What is the difference between listing the cd line as broken and
> listing no cd line at all?

I am trying to have a way to represent a gpio line as card detect line
that is not connected to the card-detect pad of the mmc controller but
instead used as a gpio interrupt line or polled gpio line.

'broken-cd' would imply that the card-detect pad of the mmc controller
is not connected to the card-detect pin at the slot. The 'cd-gpios'
property in that case would let the driver code to use the 'cd-gpios'
line as the external gpio interrupt or poll it.

Thanks,
Thomas.

>
>         Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Chris Ball]

Hi,

On Tue, Aug 21 2012, Thomas Abraham wrote:
> I am trying to have a way to represent a gpio line as card detect line
> that is not connected to the card-detect pad of the mmc controller but
> instead used as a gpio interrupt line or polled gpio line.
>
> 'broken-cd' would imply that the card-detect pad of the mmc controller
> is not connected to the card-detect pin at the slot. The 'cd-gpios'
> property in that case would let the driver code to use the 'cd-gpios'
> line as the external gpio interrupt or poll it.

How about this?

broken-cd: No CD available, use polling.

cd-gpios: The CD pin on the host is working and brought out to a GPIO.

external-cd-gpios: The CD pin on the host is broken, but there's an
                   independent external GPIO available.


(Each host should only have one of these properties.)
                   
Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Arnd Bergmann]

On Tuesday 21 August 2012, Chris Ball wrote:
> How about this?
> 
> broken-cd: No CD available, use polling.
> 
> cd-gpios: The CD pin on the host is working and brought out to a GPIO.
> 
> external-cd-gpios: The CD pin on the host is broken, but there's an
>                    independent external GPIO available.
> 
> 
> (Each host should only have one of these properties.)

The fsl-imx-esdhc.txt binding already has all three cases, but
describes them differently:

fsl,cd-internal: when present, the CD pin on the host is working
cd-gpios: when present, contains the gpio line that CD is connected to
If both are absent, it has to use polling.

	Arnd

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Thomas Abraham]

On 21 August 2012 17:26, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Tue, Aug 21 2012, Thomas Abraham wrote:
>> I am trying to have a way to represent a gpio line as card detect line
>> that is not connected to the card-detect pad of the mmc controller but
>> instead used as a gpio interrupt line or polled gpio line.
>>
>> 'broken-cd' would imply that the card-detect pad of the mmc controller
>> is not connected to the card-detect pin at the slot. The 'cd-gpios'
>> property in that case would let the driver code to use the 'cd-gpios'
>> line as the external gpio interrupt or poll it.
>
> How about this?
>
> broken-cd: No CD available, use polling.
>
> cd-gpios: The CD pin on the host is working and brought out to a GPIO.
>
> external-cd-gpios: The CD pin on the host is broken, but there's an
>                    independent external GPIO available.
>
>
> (Each host should only have one of these properties.)

How about a property using 'cd-external' property in place of
'external-cd-gpios'.  'cd-external' would mean that the gpio listed by
'cd-gpios' is a gpio line which is not connected the card-detect pad
of the mmc controller.

This can simplfy the dt parsing code since it is required to look for
only one gpio property, which is 'cd-gpios' which implies lesser error
handling code. 'cd-external' can qualify it further as an external cd
pin.

Either way, I am fine with the binding you have proposed. If you post
a patch for these new bindings, I will base the sdhci-s3c dt patch on
top of that patch.

Thanks,
Thomas.

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Rob Herring]

On 08/21/2012 07:08 AM, Arnd Bergmann wrote:
> On Tuesday 21 August 2012, Chris Ball wrote:
>> How about this?
>>
>> broken-cd: No CD available, use polling.
>>
>> cd-gpios: The CD pin on the host is working and brought out to a GPIO.
>>
>> external-cd-gpios: The CD pin on the host is broken, but there's an
>>                    independent external GPIO available.
>>
>>
>> (Each host should only have one of these properties.)
> 
> The fsl-imx-esdhc.txt binding already has all three cases, but
> describes them differently:
> 
> fsl,cd-internal: when present, the CD pin on the host is working
> cd-gpios: when present, contains the gpio line that CD is connected to
> If both are absent, it has to use polling.

This makes the most sense to me. However, I prefer broken-cd over
cd-internal. The binding should add properties for exceptions, not SDHCI
spec compliant implementations.

Rob

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Chris Ball]

Hi, adding Shawn and Wolfram,

On Tue, Aug 21 2012, Arnd Bergmann wrote:
> On Tuesday 21 August 2012, Chris Ball wrote:
>> How about this?
>> 
>> broken-cd: No CD available, use polling.
>> 
>> cd-gpios: The CD pin on the host is working and brought out to a GPIO.
>> 
>> external-cd-gpios: The CD pin on the host is broken, but there's an
>>                    independent external GPIO available.
>> 
>> 
>> (Each host should only have one of these properties.)
>
> The fsl-imx-esdhc.txt binding already has all three cases, but
> describes them differently:
>
> fsl,cd-internal: when present, the CD pin on the host is working
> cd-gpios: when present, contains the gpio line that CD is connected to
> If both are absent, it has to use polling.

Aside: the bindings do not match the code.  The bindings document says
to use "fsl,cd-internal", and imx51-babbage.dts does so -- but the code
doesn't check for "fsl,cd-internal", it checks for "fsl,cd-controller":

        if (of_get_property(np, "fsl,cd-controller", NULL))
                boarddata->cd_type = ESDHC_CD_CONTROLLER;

The same confusion is present for fsl,wp-{controller,internal}.

Ignoring that, these bindings are similar, but not the same -- imx-esdhc
only allows one of the cd_type cases to be true, so you either have
cd-internal or you have cd-gpios:

        if (of_get_property(np, "fsl,cd-controller", NULL))
                boarddata->cd_type = ESDHC_CD_CONTROLLER;

        boarddata->cd_gpio = of_get_named_gpio(np, "cd-gpios", 0);
        if (gpio_is_valid(boarddata->cd_gpio))
                boarddata->cd_type = ESDHC_CD_GPIO;

This differs from Thomas's binding -- he wants a way to say "the cd-gpio
mentioned in cd-gpios is [internal/external] to the card's CD pin".

Rob Herring said:
> This makes the most sense to me. However, I prefer broken-cd over
> cd-internal. The binding should add properties for exceptions, not SDHCI
> spec compliant implementations.

Agreed, I was going to say the same thing.  Putting it all together, it
sounds like we want:

no extra properties:  the CD pin on the host just works.
broken-cd:            the CD pin on the host is broken; use polling.
cd-gpios:             the GPIO listed is the CD pin on the host being
                      brought out directly to a GPIO.
cd-external:          when used with cd-gpios, specifies that the GPIO
                      in cd-gpios is external to the CD pin on the host.

cd-gpios and cd-external can be present on the same node.  if broken-cd
is present, it must be the only one of these nodes used.

Shawn, I guess I'll leave it up to you on whether/when you'd like to
move away from the "fsl," properties to the new common ones?

If this looks okay to everyone, I'll send a patch soon.

Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Rob Herring]

On 08/21/2012 09:48 AM, Chris Ball wrote:
> Hi, adding Shawn and Wolfram,

snip...

> Rob Herring said:
>> This makes the most sense to me. However, I prefer broken-cd over
>> cd-internal. The binding should add properties for exceptions, not SDHCI
>> spec compliant implementations.
> 
> Agreed, I was going to say the same thing.  Putting it all together, it
> sounds like we want:
> 
> no extra properties:  the CD pin on the host just works.
> broken-cd:            the CD pin on the host is broken; use polling.
> cd-gpios:             the GPIO listed is the CD pin on the host being
>                       brought out directly to a GPIO.
> cd-external:          when used with cd-gpios, specifies that the GPIO
>                       in cd-gpios is external to the CD pin on the host.
> 
> cd-gpios and cd-external can be present on the same node.  if broken-cd
> is present, it must be the only one of these nodes used.

I don't see the point of cd-external. Either you just use the CD
interrupt defined within the SDHCI or you have a gpio line independent
of the SDHCI and use cd-gpios.

Rob

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Chris Ball]

Hi,

On Tue, Aug 21 2012, Rob Herring wrote:
>> cd-gpios and cd-external can be present on the same node.  if broken-cd
>> is present, it must be the only one of these nodes used.
>
> I don't see the point of cd-external. Either you just use the CD
> interrupt defined within the SDHCI or you have a gpio line independent
> of the SDHCI and use cd-gpios.

You've described two of the possible cases, but not the third.  In the
third case, you have a gpio line that is not independent of the SDHCI,
because it is the SDHCI's CD pin brought out to be directly accessible
via a GPIO.

The difference is in the handling of the interrupt -- if you don't have
"cd-external" then you're just using the SDHCI's interrupt, but if you
have an independent line then you're going to need to register your own
IRQ handler on it, and "cd-external" signifies that.

Thomas wrote this explanation earlier in the thread:
> "samsung,sdhci-cd-gpio" means that the cd-gpio line is not connected
> to the card-detect pad of the sdhci controller. Instead, it identifies
> cd-gpio as a gpio pin, connected to the card-detect pin of the "card
> slot" and it can used as a source of external interrupt. The driver
> can register card insert/remove handler for this interrupt and get
> notified about the changes in card state.
> 
> "sdhci-cd-internal" means that the "cd-gpio" line is used to connect
> the card-detect pin of the card slot and the card-detect pad of the
> sdhci controller. The controller is then aware of any changes in card
> state and the controller generates appropriate interrupts to notify
> changes in card-state.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Thomas Abraham]

On 21 August 2012 20:33, Rob Herring <robherring2@gmail.com> wrote:
> On 08/21/2012 09:48 AM, Chris Ball wrote:
>> Hi, adding Shawn and Wolfram,
>
> snip...
>
>> Rob Herring said:
>>> This makes the most sense to me. However, I prefer broken-cd over
>>> cd-internal. The binding should add properties for exceptions, not SDHCI
>>> spec compliant implementations.
>>
>> Agreed, I was going to say the same thing.  Putting it all together, it
>> sounds like we want:
>>
>> no extra properties:  the CD pin on the host just works.
>> broken-cd:            the CD pin on the host is broken; use polling.
>> cd-gpios:             the GPIO listed is the CD pin on the host being
>>                       brought out directly to a GPIO.
>> cd-external:          when used with cd-gpios, specifies that the GPIO
>>                       in cd-gpios is external to the CD pin on the host.
>>
>> cd-gpios and cd-external can be present on the same node.  if broken-cd
>> is present, it must be the only one of these nodes used.
>
> I don't see the point of cd-external. Either you just use the CD
> interrupt defined within the SDHCI or you have a gpio line independent
> of the SDHCI and use cd-gpios.

There should be way to distinguish between the two types of 'cd-gpios' value.

(A) a 'cd-gpios' line that connects the card-detect pin of the mmc
physical slot to the card-detect pad to the mmc host controller.

(B) a 'cd-gpios' line that is connected to the card-detect pin of the
slot but not to the card-detect pad of the mmc host controller (used
either as gpio interrupt or polled gpio line).

So the binding 'cd-external' can act as an additional qualifier to
value of 'cd-gpios' to represent the second type. Hence, the bindings
proposed by Chris does support all the possible combinations of
card-detection used by the Samsung sdhci controller driver.

Thanks,
Thomas.





>
> Rob
>

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Rob Herring]

On 08/21/2012 10:18 AM, Chris Ball wrote:
> Hi,
> 
> On Tue, Aug 21 2012, Rob Herring wrote:
>>> cd-gpios and cd-external can be present on the same node.  if broken-cd
>>> is present, it must be the only one of these nodes used.
>>
>> I don't see the point of cd-external. Either you just use the CD
>> interrupt defined within the SDHCI or you have a gpio line independent
>> of the SDHCI and use cd-gpios.
> 
> You've described two of the possible cases, but not the third.  In the
> third case, you have a gpio line that is not independent of the SDHCI,
> because it is the SDHCI's CD pin brought out to be directly accessible
> via a GPIO.

That is covered by absence of cd-gpios and broken-cd. Any *-gpios
property means the signal is a GPIO line controlled by a GPIO controller.

I suppose you could have the CD state readable via the SDHCI, but the
interrupt comes from a GPIO controller. Or vice-versa, but that's a
pretty broken use case if you can't pick which way you are going to use
things.

> 
> The difference is in the handling of the interrupt -- if you don't have
> "cd-external" then you're just using the SDHCI's interrupt, but if you
> have an independent line then you're going to need to register your own
> IRQ handler on it, and "cd-external" signifies that.
> 
> Thomas wrote this explanation earlier in the thread:
>> "samsung,sdhci-cd-gpio" means that the cd-gpio line is not connected
>> to the card-detect pad of the sdhci controller. Instead, it identifies
>> cd-gpio as a gpio pin, connected to the card-detect pin of the "card
>> slot" and it can used as a source of external interrupt. The driver
>> can register card insert/remove handler for this interrupt and get
>> notified about the changes in card state.
>>
>> "sdhci-cd-internal" means that the "cd-gpio" line is used to connect
>> the card-detect pin of the card slot and the card-detect pad of the
>> sdhci controller. The controller is then aware of any changes in card
>> state and the controller generates appropriate interrupts to notify
>> changes in card-state.
> 

It seems you are mixing pin muxing and who controls/handles the CD
detect signal. Pin muxing is a separate issue and should be addressed by
pin mux bindings. Either the signal functions as a GPIO or it functions
as a CD as part of the SDHCI. You may have muxing on the CD pin that
allows it to function either way, but the DT binding should describe how
you want it to be configured and used.

Rob

> Thanks,
> 
> - Chris.
> 

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Thomas Abraham]

On 21 August 2012 21:31, Rob Herring <robherring2@gmail.com> wrote:
> On 08/21/2012 10:18 AM, Chris Ball wrote:
>> Hi,
>>
>> On Tue, Aug 21 2012, Rob Herring wrote:
>>>> cd-gpios and cd-external can be present on the same node.  if broken-cd
>>>> is present, it must be the only one of these nodes used.
>>>
>>> I don't see the point of cd-external. Either you just use the CD
>>> interrupt defined within the SDHCI or you have a gpio line independent
>>> of the SDHCI and use cd-gpios.
>>
>> You've described two of the possible cases, but not the third.  In the
>> third case, you have a gpio line that is not independent of the SDHCI,
>> because it is the SDHCI's CD pin brought out to be directly accessible
>> via a GPIO.
>
> That is covered by absence of cd-gpios and broken-cd. Any *-gpios
> property means the signal is a GPIO line controlled by a GPIO controller.
>
> I suppose you could have the CD state readable via the SDHCI, but the
> interrupt comes from a GPIO controller. Or vice-versa, but that's a
> pretty broken use case if you can't pick which way you are going to use
> things.
>
>>
>> The difference is in the handling of the interrupt -- if you don't have
>> "cd-external" then you're just using the SDHCI's interrupt, but if you
>> have an independent line then you're going to need to register your own
>> IRQ handler on it, and "cd-external" signifies that.
>>
>> Thomas wrote this explanation earlier in the thread:
>>> "samsung,sdhci-cd-gpio" means that the cd-gpio line is not connected
>>> to the card-detect pad of the sdhci controller. Instead, it identifies
>>> cd-gpio as a gpio pin, connected to the card-detect pin of the "card
>>> slot" and it can used as a source of external interrupt. The driver
>>> can register card insert/remove handler for this interrupt and get
>>> notified about the changes in card state.
>>>
>>> "sdhci-cd-internal" means that the "cd-gpio" line is used to connect
>>> the card-detect pin of the card slot and the card-detect pad of the
>>> sdhci controller. The controller is then aware of any changes in card
>>> state and the controller generates appropriate interrupts to notify
>>> changes in card-state.
>>
>
> It seems you are mixing pin muxing and who controls/handles the CD
> detect signal. Pin muxing is a separate issue and should be addressed by
> pin mux bindings. Either the signal functions as a GPIO or it functions
> as a CD as part of the SDHCI. You may have muxing on the CD pin that
> allows it to function either way, but the DT binding should describe how
> you want it to be configured and used.

Ok, I agree with Rob. I was mixing pin muxing here. So if we have
'cd-gpios' and 'broken-cd' as generic bindings, would the following be
valid?

[A] cd-gpios not present ,  broken-cd not present : This means that
there is no card detect pin available. Controller drivers are free to
infer this as "card detection is broken" and use implementation
specific behavior.

[B] cd-gpio not present , broken-cd present :  This means that there
is nothing connected to the card-detect pad of the mmc host
controller. Controller drivers are free to use implementation specific
behavior to deal with card detection.

[C] cd-gpio present, broken-cd not present : This means that 'cd-gpio'
line connects card-detect pad of the controller to the card-detect pin
of the mmc slot.

[D] cd-gpio present, broken-cd present : This means that there is
nothing connected to the card-detect pad of the mmc host controller.
Controller drivers are free to use the 'cd-gpio' line in any
implementation specific way.

Thanks,
Thomas.

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Mitch Bradley]

On 8/21/2012 7:33 AM, Thomas Abraham wrote:
> On 21 August 2012 21:31, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 08/21/2012 10:18 AM, Chris Ball wrote:
>>> Hi,
>>>
>>> On Tue, Aug 21 2012, Rob Herring wrote:
>>>>> cd-gpios and cd-external can be present on the same node.  if broken-cd
>>>>> is present, it must be the only one of these nodes used.
>>>>
>>>> I don't see the point of cd-external. Either you just use the CD
>>>> interrupt defined within the SDHCI or you have a gpio line independent
>>>> of the SDHCI and use cd-gpios.
>>>
>>> You've described two of the possible cases, but not the third.  In the
>>> third case, you have a gpio line that is not independent of the SDHCI,
>>> because it is the SDHCI's CD pin brought out to be directly accessible
>>> via a GPIO.
>>
>> That is covered by absence of cd-gpios and broken-cd. Any *-gpios
>> property means the signal is a GPIO line controlled by a GPIO controller.
>>
>> I suppose you could have the CD state readable via the SDHCI, but the
>> interrupt comes from a GPIO controller. Or vice-versa, but that's a
>> pretty broken use case if you can't pick which way you are going to use
>> things.
>>
>>>
>>> The difference is in the handling of the interrupt -- if you don't have
>>> "cd-external" then you're just using the SDHCI's interrupt, but if you
>>> have an independent line then you're going to need to register your own
>>> IRQ handler on it, and "cd-external" signifies that.
>>>
>>> Thomas wrote this explanation earlier in the thread:
>>>> "samsung,sdhci-cd-gpio" means that the cd-gpio line is not connected
>>>> to the card-detect pad of the sdhci controller. Instead, it identifies
>>>> cd-gpio as a gpio pin, connected to the card-detect pin of the "card
>>>> slot" and it can used as a source of external interrupt. The driver
>>>> can register card insert/remove handler for this interrupt and get
>>>> notified about the changes in card state.
>>>>
>>>> "sdhci-cd-internal" means that the "cd-gpio" line is used to connect
>>>> the card-detect pin of the card slot and the card-detect pad of the
>>>> sdhci controller. The controller is then aware of any changes in card
>>>> state and the controller generates appropriate interrupts to notify
>>>> changes in card-state.
>>>
>>
>> It seems you are mixing pin muxing and who controls/handles the CD
>> detect signal. Pin muxing is a separate issue and should be addressed by
>> pin mux bindings. Either the signal functions as a GPIO or it functions
>> as a CD as part of the SDHCI. You may have muxing on the CD pin that
>> allows it to function either way, but the DT binding should describe how
>> you want it to be configured and used.
> 
> Ok, I agree with Rob. I was mixing pin muxing here. So if we have
> 'cd-gpios' and 'broken-cd' as generic bindings, would the following be
> valid?
> 
> [A] cd-gpios not present ,  broken-cd not present : This means that
> there is no card detect pin available. Controller drivers are free to
> infer this as "card detection is broken" and use implementation
> specific behavior.
> 
> [B] cd-gpio not present , broken-cd present :  This means that there
> is nothing connected to the card-detect pad of the mmc host
> controller. Controller drivers are free to use implementation specific
> behavior to deal with card detection.
> 
> [C] cd-gpio present, broken-cd not present : This means that 'cd-gpio'
> line connects card-detect pad of the controller to the card-detect pin
> of the mmc slot.
> 
> [D] cd-gpio present, broken-cd present : This means that there is
> nothing connected to the card-detect pad of the mmc host controller.
> Controller drivers are free to use the 'cd-gpio' line in any
> implementation specific way.


Matrix encoding is too complicated; my brain froze when trying to decode
the message above.  I favor properties that can be understood in
isolation.  The following proberties are mutually-exclusive.

<no properties present>: The standard SDHCI register bit just works.  No
additional setup is necessary (either the system is hardwired to work
correctly or external software has already done any necessary setup).

broken-cd <no value>: CD is just not available - neither via the
standard SDHCI register bit/interrupt, nor by an external GPIO.

cd-gpios <&gpio, active_low> : The standard SDHCI register bit does not
work, but CD is available via the specified GPIO.  If active_low is 0,
reading 1 from the GPIO pin means that the card is present, otherwise
reading 0 from the GPIO pin means that the card is present.  I must
emphasize that this is an external GPIO, accessed via GPIO mechanisms,
not a connection of a multipurpose pin to the SDHCI core.

cd-pin <&pin> : The standard SDHCI register bit works, but the SDHCI
driver must ask for the indicated multipurpose pin to be connected to
the SDHCI controller as the CD pin.  I don't know what &pin is, exactly,
as I haven't been paying attention to the pinmux discussions.


> 
> Thanks,
> Thomas.
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Shawn Guo]

On Tue, Aug 21, 2012 at 11:03:59PM +0530, Thomas Abraham wrote:
> Ok, I agree with Rob. I was mixing pin muxing here. So if we have
> 'cd-gpios' and 'broken-cd' as generic bindings, would the following be
> valid?
> 
> [A] cd-gpios not present ,  broken-cd not present : This means that
> there is no card detect pin available. Controller drivers are free to
> infer this as "card detection is broken" and use implementation
> specific behavior.
> 
> [B] cd-gpio not present , broken-cd present :  This means that there
> is nothing connected to the card-detect pad of the mmc host
> controller. Controller drivers are free to use implementation specific
> behavior to deal with card detection.
> 
> [C] cd-gpio present, broken-cd not present : This means that 'cd-gpio'
> line connects card-detect pad of the controller to the card-detect pin
> of the mmc slot.
> 
> [D] cd-gpio present, broken-cd present : This means that there is
> nothing connected to the card-detect pad of the mmc host controller.
> Controller drivers are free to use the 'cd-gpio' line in any
> implementation specific way.
> 
The following is what I have on my mind.

broken-cd	cd-gpios	implication
-------------------------------------------
no		no		SDHCI CD
no		yes		GPIO CD
yes		no		NO CD / Broken CD
yes		yes		Invalid

yes: property presents
no: property does not present

-- 
Regards,
Shawn

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Shawn Guo]

On Tue, Aug 21, 2012 at 10:48:43AM -0400, Chris Ball wrote:
> Aside: the bindings do not match the code.  The bindings document says
> to use "fsl,cd-internal", and imx51-babbage.dts does so -- but the code
> doesn't check for "fsl,cd-internal", it checks for "fsl,cd-controller":
> 
>         if (of_get_property(np, "fsl,cd-controller", NULL))
>                 boarddata->cd_type = ESDHC_CD_CONTROLLER;
> 
> The same confusion is present for fsl,wp-{controller,internal}.
> 
Thanks for spotting it.  I will submit a patch to fix binding doc and
dts.

> Ignoring that, these bindings are similar, but not the same -- imx-esdhc
> only allows one of the cd_type cases to be true, so you either have
> cd-internal or you have cd-gpios:
> 
>         if (of_get_property(np, "fsl,cd-controller", NULL))
>                 boarddata->cd_type = ESDHC_CD_CONTROLLER;
> 
>         boarddata->cd_gpio = of_get_named_gpio(np, "cd-gpios", 0);
>         if (gpio_is_valid(boarddata->cd_gpio))
>                 boarddata->cd_type = ESDHC_CD_GPIO;
> 
> This differs from Thomas's binding -- he wants a way to say "the cd-gpio
> mentioned in cd-gpios is [internal/external] to the card's CD pin".
> 
> Rob Herring said:
> > This makes the most sense to me. However, I prefer broken-cd over
> > cd-internal. The binding should add properties for exceptions, not SDHCI
> > spec compliant implementations.
> 
> Agreed, I was going to say the same thing.  Putting it all together, it
> sounds like we want:
> 
> no extra properties:  the CD pin on the host just works.
> broken-cd:            the CD pin on the host is broken; use polling.
> cd-gpios:             the GPIO listed is the CD pin on the host being
>                       brought out directly to a GPIO.
> cd-external:          when used with cd-gpios, specifies that the GPIO
>                       in cd-gpios is external to the CD pin on the host.
> 
> cd-gpios and cd-external can be present on the same node.  if broken-cd
> is present, it must be the only one of these nodes used.
> 
> Shawn, I guess I'll leave it up to you on whether/when you'd like to
> move away from the "fsl," properties to the new common ones?
> 
Ok, I will move to the common one once it appears on your tree.

-- 
Regards,
Shawn

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Chris Ball]

Hi,

On Wed, Aug 22 2012, Shawn Guo wrote:
> The following is what I have on my mind.
>
> broken-cd	cd-gpios	implication
> -------------------------------------------
> no		no		SDHCI CD
> no		yes		GPIO CD
> yes		no		NO CD / Broken CD
> yes		yes		Invalid
>
> yes: property presents
> no: property does not present

This matches Mitch's last suggestion exactly -- I think we're all agreed
on these properties now.  The only remaining question is how to handle
the pinctrl for CD in Thomas's case.

Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Thomas Abraham]

On 22 August 2012 15:47, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Wed, Aug 22 2012, Shawn Guo wrote:
>> The following is what I have on my mind.
>>
>> broken-cd     cd-gpios        implication
>> -------------------------------------------
>> no            no              SDHCI CD
>> no            yes             GPIO CD
>> yes           no              NO CD / Broken CD
>> yes           yes             Invalid
>>
>> yes: property presents
>> no: property does not present
>
> This matches Mitch's last suggestion exactly -- I think we're all agreed
> on these properties now.  The only remaining question is how to handle
> the pinctrl for CD in Thomas's case.

Hi Chris,

For sdhci-s3c driver, the 'broken-cd' and 'cd-gpios' bindings are
sufficient. But, are drivers free to use implementation specific
behavior when using these bindings. Or do we strictly adhere to the
table which Shawn has listed. For sdhci-s3c driver, I would like to
deviate from the "implication" column listed above.

On second thoughts, I now understand that gpio's terminate at the gpio
controller, not at the card-detect pad of the mmc host controller.
Those that terminate at the mmc controller pads are actually mux
functions. I do agree that the long term solution for sdhci-s3c driver
is to use the pinctrl driver.

If sdhci-s3c has to strictly adhere to the Shawn's table above, then I
prefer to postpone the device tree support in sdhci-s3c driver after
having proper support for Samsung pinctrl driver. But then, sdhci-s3c
driver is used on multiple Samsung SoC's, all of which might not get
pinctrl driver support anytime soon.

Hence, for sdhci-s3c driver, 'broken-cd' and 'cd-gpios' bindings are
sufficient, but sdhci-s3c driver should be free to use these bindings
in a implementation specific way.

Thanks,
Thomas.

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Chris Ball]

Hi Thomas,

On Wed, Aug 22 2012, Thomas Abraham wrote:
>> This matches Mitch's last suggestion exactly -- I think we're all agreed
>> on these properties now.  The only remaining question is how to handle
>> the pinctrl for CD in Thomas's case.
>
> Hi Chris,
>
> For sdhci-s3c driver, the 'broken-cd' and 'cd-gpios' bindings are
> sufficient. But, are drivers free to use implementation specific
> behavior when using these bindings. Or do we strictly adhere to the
> table which Shawn has listed. For sdhci-s3c driver, I would like to
> deviate from the "implication" column listed above.

I'd rather you use the implications described above for the names
described above; there's not much point in defining named behaviors
across the subsystem if they're actually different in each driver.

But I think you can still use driver-internal properties that change the
interpretation of those names in a documented way.  If the meaning of
cd-gpios is modified when coupled with your "samsung,sdhci-cd-external",
that's okay with me.  That should cover all the cases you need until you
migrate to using pinctrl, right?  So, you could immediately support:

none          -> currently "samsung,sdhci-cd-internal"
broken-cd     -> currently "samsung,sdhci-cd-none"
cd-gpios      -> currently "samsung,sdhci-cd-gpios"
non-removable -> currently "samsung,sdhci-cd-permanent"
cd-gpios + samsung,sdhci-cd-external -> currently "samsung,sdhci-cd-external"

How does that sound?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Thomas Abraham]

On 22 August 2012 16:38, Chris Ball <cjb@laptop.org> wrote:
> Hi Thomas,
>
> On Wed, Aug 22 2012, Thomas Abraham wrote:
>>> This matches Mitch's last suggestion exactly -- I think we're all agreed
>>> on these properties now.  The only remaining question is how to handle
>>> the pinctrl for CD in Thomas's case.
>>
>> Hi Chris,
>>
>> For sdhci-s3c driver, the 'broken-cd' and 'cd-gpios' bindings are
>> sufficient. But, are drivers free to use implementation specific
>> behavior when using these bindings. Or do we strictly adhere to the
>> table which Shawn has listed. For sdhci-s3c driver, I would like to
>> deviate from the "implication" column listed above.
>
> I'd rather you use the implications described above for the names
> described above; there's not much point in defining named behaviors
> across the subsystem if they're actually different in each driver.

Ok.

>
> But I think you can still use driver-internal properties that change the
> interpretation of those names in a documented way.  If the meaning of
> cd-gpios is modified when coupled with your "samsung,sdhci-cd-external",
> that's okay with me.  That should cover all the cases you need until you
> migrate to using pinctrl, right?  So, you could immediately support:
>
> none          -> currently "samsung,sdhci-cd-internal"
> broken-cd     -> currently "samsung,sdhci-cd-none"
> cd-gpios      -> currently "samsung,sdhci-cd-gpios"
> non-removable -> currently "samsung,sdhci-cd-permanent"
> cd-gpios + samsung,sdhci-cd-external -> currently "samsung,sdhci-cd-external"
>
> How does that sound?

Not quite there yet. It is not possible to leave out cd-gpios for
"samsung,sdhci-cd-internal" since the current Samsung pinmux
configuration piggybacks on gpio dt binding. Can we use the following
instead.

none               -> currently "samsung,sdhci-cd-none"
broken-cd        -> currently "samsung,sdhci-cd-none"
cd-gpios          -> currently "samsung,sdhci-cd-gpios"
non-removable -> currently "samsung,sdhci-cd-permanent"
cd-gpios + samsung,sdhci-cd-internal -> currently "samsung,sdhci-cd-internal"

Thanks Chris.

Regards,
Thomas.

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Shawn Guo]

Hi Chris,

On Tue, Aug 21, 2012 at 10:48:43AM -0400, Chris Ball wrote:
> Aside: the bindings do not match the code.  The bindings document says
> to use "fsl,cd-internal", and imx51-babbage.dts does so -- but the code
> doesn't check for "fsl,cd-internal", it checks for "fsl,cd-controller":
> 
>         if (of_get_property(np, "fsl,cd-controller", NULL))
>                 boarddata->cd_type = ESDHC_CD_CONTROLLER;
> 
> The same confusion is present for fsl,wp-{controller,internal}.
> 
I'm trying to fix the mismatch between dts and code, but seeing that
the "controller" mode has been broken for quite a while.  I just ran
a bisect and was told as below.

30832ab56c80d96cfaf5a786524f0d8c57fadfa1 is the first bad commit
commit 30832ab56c80d96cfaf5a786524f0d8c57fadfa1
Author: Todd Poynor <toddpoynor@google.com>
Date:   Tue Dec 27 15:48:46 2011 +0200

    mmc: sdhci: Always pass clock request value zero to set_clock host op

    To allow the set_clock host op to disable the SDCLK source when not
    needed, always call the host op when the requested clock speed is
    zero.  Do this even if host->clock already equals zero, because
    the SDHCI driver may set that value (without calling the host op)
    to force an update at the next (non-zero-speed) call.

    Signed-off-by: Todd Poynor <toddpoynor@google.com>
    Acked-by: Adrian Hunter <adrian.hunter@intel.com>
    Signed-off-by: Chris Ball <cjb@laptop.org>

Reverting the commit will get "controller" mode back to work.

-- 
Regards,
Shawn

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Chris Ball]

Hi Shawn,

On Wed, Aug 22 2012, Shawn Guo wrote:
>     mmc: sdhci: Always pass clock request value zero to set_clock host op
>
>     To allow the set_clock host op to disable the SDCLK source when not
>     needed, always call the host op when the requested clock speed is
>     zero.  Do this even if host->clock already equals zero, because
>     the SDHCI driver may set that value (without calling the host op)
>     to force an update at the next (non-zero-speed) call.
>
>     Signed-off-by: Todd Poynor <toddpoynor@google.com>
>     Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>     Signed-off-by: Chris Ball <cjb@laptop.org>
>
> Reverting the commit will get "controller" mode back to work.

Weird; I wonder how that's related to card detection.

I'm afraid it's ultimately a driver bug -- esdhc should be able to
handle having set_clock called with a value of 0.  It seems to have
a case in esdhc_set_clock() for 0, so I'm surprised it's not working.

What's the symptom of the failure?  Can you debug some more?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Chris Ball]

Hi Thomas,

On Wed, Aug 22 2012, Thomas Abraham wrote:
>> none          -> currently "samsung,sdhci-cd-internal"
>> broken-cd     -> currently "samsung,sdhci-cd-none"
>> cd-gpios      -> currently "samsung,sdhci-cd-gpios"
>> non-removable -> currently "samsung,sdhci-cd-permanent"
>> cd-gpios + samsung,sdhci-cd-external -> currently "samsung,sdhci-cd-external"
>>
>> How does that sound?
>
> Not quite there yet. It is not possible to leave out cd-gpios for
> "samsung,sdhci-cd-internal" since the current Samsung pinmux
> configuration piggybacks on gpio dt binding. Can we use the following
> instead.
>
> none               -> currently "samsung,sdhci-cd-none"
> broken-cd        -> currently "samsung,sdhci-cd-none"
> cd-gpios          -> currently "samsung,sdhci-cd-gpios"
> non-removable -> currently "samsung,sdhci-cd-permanent"
> cd-gpios + samsung,sdhci-cd-internal -> currently "samsung,sdhci-cd-internal"

I see.  Okay; unless anyone has better ideas, it sounds like you should
go ahead and do that -- with something like this in your binding docs:

"This binding differs from the core MMC binding by requiring a cd-gpios
property to be present to use internal card-detection.  If no cd-gpios
are present (and "non-removable" is missing) the driver will poll for
card-detection as if a "broken-cd" property was provided."

I'll send out the new core bindings doc shortly.  Finishing off another
thread -- the "samsung-sdhci.txt" name is okay.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Stephen Warren]

On 08/22/2012 04:17 AM, Chris Ball wrote:
> Hi,
> 
> On Wed, Aug 22 2012, Shawn Guo wrote:
>> The following is what I have on my mind.
>>
>> broken-cd	cd-gpios	implication
>> -------------------------------------------
>> no		no		SDHCI CD
>> no		yes		GPIO CD
>> yes		no		NO CD / Broken CD
>> yes		yes		Invalid
>>
>> yes: property presents
>> no: property does not present
> 
> This matches Mitch's last suggestion exactly -- I think we're all agreed
> on these properties now.  The only remaining question is how to handle
> the pinctrl for CD in Thomas's case.

Sorry if this has already been mentioned, but I assume the standard
non-removable property overrides any of those, or is invalid in
combination with any of those?

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Chris Ball]

Hi Stephen,

On Wed, Aug 22 2012, Stephen Warren wrote:
> On 08/22/2012 04:17 AM, Chris Ball wrote:
>> Hi,
>> 
>> On Wed, Aug 22 2012, Shawn Guo wrote:
>>> The following is what I have on my mind.
>>>
>>> broken-cd	cd-gpios	implication
>>> -------------------------------------------
>>> no		no		SDHCI CD
>>> no		yes		GPIO CD
>>> yes		no		NO CD / Broken CD
>>> yes		yes		Invalid
>>>
>>> yes: property presents
>>> no: property does not present
>> 
>> This matches Mitch's last suggestion exactly -- I think we're all agreed
>> on these properties now.  The only remaining question is how to handle
>> the pinctrl for CD in Thomas's case.
>
> Sorry if this has already been mentioned, but I assume the standard
> non-removable property overrides any of those, or is invalid in
> combination with any of those?

Yes, absolutely.  I'll make sure to explain that in the new version of
the document.  (I think the reason it hasn't been mentioned much in this
thread is that it's already part of the core bindings.)

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Thomas Abraham]

On 22 August 2012 20:24, Chris Ball <cjb@laptop.org> wrote:
> Hi Thomas,
>
> On Wed, Aug 22 2012, Thomas Abraham wrote:
>>> none          -> currently "samsung,sdhci-cd-internal"
>>> broken-cd     -> currently "samsung,sdhci-cd-none"
>>> cd-gpios      -> currently "samsung,sdhci-cd-gpios"
>>> non-removable -> currently "samsung,sdhci-cd-permanent"
>>> cd-gpios + samsung,sdhci-cd-external -> currently "samsung,sdhci-cd-external"
>>>
>>> How does that sound?
>>
>> Not quite there yet. It is not possible to leave out cd-gpios for
>> "samsung,sdhci-cd-internal" since the current Samsung pinmux
>> configuration piggybacks on gpio dt binding. Can we use the following
>> instead.
>>
>> none               -> currently "samsung,sdhci-cd-none"
>> broken-cd        -> currently "samsung,sdhci-cd-none"
>> cd-gpios          -> currently "samsung,sdhci-cd-gpios"
>> non-removable -> currently "samsung,sdhci-cd-permanent"
>> cd-gpios + samsung,sdhci-cd-internal -> currently "samsung,sdhci-cd-internal"
>
> I see.  Okay; unless anyone has better ideas, it sounds like you should
> go ahead and do that -- with something like this in your binding docs:

Ok.

>
> "This binding differs from the core MMC binding by requiring a cd-gpios
> property to be present to use internal card-detection.  If no cd-gpios
> are present (and "non-removable" is missing) the driver will poll for
> card-detection as if a "broken-cd" property was provided."
>
> I'll send out the new core bindings doc shortly.  Finishing off another
> thread -- the "samsung-sdhci.txt" name is okay.

Thanks Chris for your time on this subject. I will prepare the
sdhci-s3c dt support patch on top of your new core bindings patch.

Thanks,
Thomas.

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Mitch Bradley]

Hi Thomas,

On 8/22/2012 2:04 AM, Thomas Abraham wrote:
> On 22 August 2012 16:38, Chris Ball <cjb-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org> wrote:
>> Hi Thomas,
>>
>> On Wed, Aug 22 2012, Thomas Abraham wrote:
>>>> This matches Mitch's last suggestion exactly -- I think we're all agreed
>>>> on these properties now.  The only remaining question is how to handle
>>>> the pinctrl for CD in Thomas's case.
>>>
>>> Hi Chris,
>>>
>>> For sdhci-s3c driver, the 'broken-cd' and 'cd-gpios' bindings are
>>> sufficient. But, are drivers free to use implementation specific
>>> behavior when using these bindings. Or do we strictly adhere to the
>>> table which Shawn has listed. For sdhci-s3c driver, I would like to
>>> deviate from the "implication" column listed above.
>>
>> I'd rather you use the implications described above for the names
>> described above; there's not much point in defining named behaviors
>> across the subsystem if they're actually different in each driver.
> 
> Ok.
> 
>>
>> But I think you can still use driver-internal properties that change the
>> interpretation of those names in a documented way.  If the meaning of
>> cd-gpios is modified when coupled with your "samsung,sdhci-cd-external",
>> that's okay with me.  That should cover all the cases you need until you
>> migrate to using pinctrl, right?  So, you could immediately support:
>>
>> none          -> currently "samsung,sdhci-cd-internal"
>> broken-cd     -> currently "samsung,sdhci-cd-none"
>> cd-gpios      -> currently "samsung,sdhci-cd-gpios"
>> non-removable -> currently "samsung,sdhci-cd-permanent"
>> cd-gpios + samsung,sdhci-cd-external -> currently "samsung,sdhci-cd-external"
>>
>> How does that sound?
> 
> Not quite there yet. It is not possible to leave out cd-gpios for
> "samsung,sdhci-cd-internal" since the current Samsung pinmux
> configuration piggybacks on gpio dt binding.


Sorry to interject on a topic that seems to have already been decided,
but I'm confused by one thing and would like clarification.  I
understand that you need to use a GPIO-style specifier as a surrogate
for a pinmux specification - that much is clear.  What is not clear is
why it's necessary to (ab)use the name "cd-gpios" for it.

Why not use a different property name, e.g. "samsung,cd-pinmux-gpio =
<gpio-specifier>" for the "cd-gpios + samsung,sdhci-cd-internal" case?
Then both "samsung,sdhci-cdi-internal" and "samsung,sdhci-cd-external"
could go away.  There would only be one system-dependent property
"samsung,cd-pinmux-gpio" whose name would make it clear that it's
conflating pinmuxing and gpios.

I think the scheme I propose would be clearer, less likely to confuse
other people who try to use the driver as a model, require less
hand-waving in the documentation, and easier to change to a proper
pinmuxing scheme should that become available later.


 Can we use the following
> instead.
> 
> none               -> currently "samsung,sdhci-cd-none"
> broken-cd        -> currently "samsung,sdhci-cd-none"
> cd-gpios          -> currently "samsung,sdhci-cd-gpios"
> non-removable -> currently "samsung,sdhci-cd-permanent"
> cd-gpios + samsung,sdhci-cd-internal -> currently "samsung,sdhci-cd-internal"
> 
> Thanks Chris.
> 
> Regards,
> Thomas.
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

[PATCH] mmc: dt: Add card-detection properties to core binding.

[Chris Ball]

Signed-off-by: Chris Ball <cjb@laptop.org>
---
 Documentation/devicetree/bindings/mmc/mmc.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 8a6811f..8e2e0ba 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -9,12 +9,17 @@ Interpreted by the OF core:
 Required properties:
 - bus-width: Number of data lines, can be <1>, <4>, or <8>
 
+Card detection:
+If no property below is supplied, standard SDHCI card detect is used.
+Only one of the properties in this section should be supplied:
+  - broken-cd: There is no card detection available; polling must be used.
+  - cd-gpios: Specify GPIOs for card detection, see gpio binding
+  - non-removable: non-removable slot (like eMMC); assume always present.
+
 Optional properties:
-- cd-gpios: Specify GPIOs for card detection, see gpio binding
 - wp-gpios: Specify GPIOs for write protection, see gpio binding
 - cd-inverted: when present, polarity on the cd gpio line is inverted
 - wp-inverted: when present, polarity on the wp gpio line is inverted
-- non-removable: non-removable slot (like eMMC)
 - max-frequency: maximum operating clock frequency
 
 Example:
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dt: Add 'broken-cd' DT binding

[Thomas Abraham]

On 22 August 2012 22:39, Mitch Bradley <wmb@firmworks.com> wrote:
> Sorry to interject on a topic that seems to have already been decided,
> but I'm confused by one thing and would like clarification.  I
> understand that you need to use a GPIO-style specifier as a surrogate
> for a pinmux specification - that much is clear.  What is not clear is
> why it's necessary to (ab)use the name "cd-gpios" for it.
>
> Why not use a different property name, e.g. "samsung,cd-pinmux-gpio =
> <gpio-specifier>" for the "cd-gpios + samsung,sdhci-cd-internal" case?
> Then both "samsung,sdhci-cdi-internal" and "samsung,sdhci-cd-external"
> could go away.  There would only be one system-dependent property
> "samsung,cd-pinmux-gpio" whose name would make it clear that it's
> conflating pinmuxing and gpios.

Right, I agree. I will prepare the sdhci-s3c based on Chris's new
generic binding patch and your suggestion.

>
> I think the scheme I propose would be clearer, less likely to confuse
> other people who try to use the driver as a model, require less
> hand-waving in the documentation, and easier to change to a proper
> pinmuxing scheme should that become available later.

Thanks for your time.

Regards,
Thomas.

Re: [PATCH] mmc: dt: Add card-detection properties to core binding.

[Shawn Guo]

On Wed, Aug 22, 2012 at 02:05:35PM -0400, Chris Ball wrote:
> Signed-off-by: Chris Ball <cjb@laptop.org>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Will move sdhci-esdhc-imx driver to this soon.

Regards,
Shawn

> ---
>  Documentation/devicetree/bindings/mmc/mmc.txt | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> index 8a6811f..8e2e0ba 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> @@ -9,12 +9,17 @@ Interpreted by the OF core:
>  Required properties:
>  - bus-width: Number of data lines, can be <1>, <4>, or <8>
>  
> +Card detection:
> +If no property below is supplied, standard SDHCI card detect is used.
> +Only one of the properties in this section should be supplied:
> +  - broken-cd: There is no card detection available; polling must be used.
> +  - cd-gpios: Specify GPIOs for card detection, see gpio binding
> +  - non-removable: non-removable slot (like eMMC); assume always present.
> +
>  Optional properties:
> -- cd-gpios: Specify GPIOs for card detection, see gpio binding
>  - wp-gpios: Specify GPIOs for write protection, see gpio binding
>  - cd-inverted: when present, polarity on the cd gpio line is inverted
>  - wp-inverted: when present, polarity on the wp gpio line is inverted
> -- non-removable: non-removable slot (like eMMC)
>  - max-frequency: maximum operating clock frequency
>  
>  Example:
> -- 
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child