| Message ID | 1345547371-6784-1-git-send-email-thomas.abraham@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 -- 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
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 -- 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
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.
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 -- 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
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. -- 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
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 -- 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
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.
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 -- 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
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.
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 > -- 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
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. > -- 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
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. -- 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
On 8/21/2012 7:33 AM, Thomas Abraham wrote: > 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. 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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss > -- 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
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
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.
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.
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. -- 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
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.
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. -- 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
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.
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.
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.
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? -- 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
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.
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. -- 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
Hi Thomas, On 8/22/2012 2:04 AM, Thomas Abraham wrote: > 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. 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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss > -- 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
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. -- 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
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:
'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(-)