| Message ID | 1360180020-18555-7-git-send-email-g.liakhovetski@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 06 February 2013, Guennadi Liakhovetski wrote: > +* Toshiba Mobile IO SD/MMC controller > + > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform > +driver. Those drivers supply the tmio-mmc driver with platform data, that either > +describe hardware capabilities, known to them, or are obtained by them from > +their own platform data or from their DT information. In the latter case all > +compulsory and any optional properties, common to all SD/MMC drivers, as > +described in mmc.txt, should or can be used. Additionally the following optional > +bindings can be used. They set respective TMIO_MMC_* flags. > + > +Optional properties: > +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag > +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES > +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT Please write the binding in a way that does not refer to a specific implementation in Linux: The binding should describe the hardware independent of details in the driver. In particular, I think you should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe in text what the flags are about. Regarding the toshiba,mmc-wrprotect-disable property, would it be enough to just check the presence of the wp-gpios property? TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't actually need to provide this here, but can keep that knowledge implicit based on whether we're talking to sh_mobile_sdhi or another tmio_mmc variant. For the other last one, is that actually board specific, or just a feature of a given chip? If we can tell by the SoC, then I'd suggest using separate "compatible" properties instead, and put a bitmask of features into the .data field of the of match table. For all I can tell, SH7372 does not set it, while SH73A0, R8A7740 and R8A7779 always do. 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 Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote: > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote: > > +* Toshiba Mobile IO SD/MMC controller > > + > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either > > +describe hardware capabilities, known to them, or are obtained by them from > > +their own platform data or from their DT information. In the latter case all > > +compulsory and any optional properties, common to all SD/MMC drivers, as > > +described in mmc.txt, should or can be used. Additionally the following optional > > +bindings can be used. They set respective TMIO_MMC_* flags. > > + > > +Optional properties: > > +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag > > +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES > > +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT > > Please write the binding in a way that does not refer to a specific > implementation in Linux: The binding should describe the hardware > independent of details in the driver. In particular, I think you > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe > in text what the flags are about. > > Regarding the toshiba,mmc-wrprotect-disable property, would it be > enough to just check the presence of the wp-gpios property? > > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't > actually need to provide this here, but can keep that knowledge > implicit based on whether we're talking to sh_mobile_sdhi > or another tmio_mmc variant. > > For the other last one, is that actually board specific, or just > a feature of a given chip? If we can tell by the SoC, then I'd > suggest using separate "compatible" properties instead, and > put a bitmask of features into the .data field of the of match > table. For all I can tell, SH7372 does not set it, while SH73A0, > R8A7740 and R8A7779 always do. My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based on the SoC in use. -- 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 Thursday 07 February 2013, Simon Horman wrote: > > Please write the binding in a way that does not refer to a specific > > implementation in Linux: The binding should describe the hardware > > independent of details in the driver. In particular, I think you > > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe > > in text what the flags are about. > > > > Regarding the toshiba,mmc-wrprotect-disable property, would it be > > enough to just check the presence of the wp-gpios property? > > > > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in > > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't > > actually need to provide this here, but can keep that knowledge > > implicit based on whether we're talking to sh_mobile_sdhi > > or another tmio_mmc variant. > > > > For the other last one, is that actually board specific, or just > > a feature of a given chip? If we can tell by the SoC, then I'd > > suggest using separate "compatible" properties instead, and > > put a bitmask of features into the .data field of the of match > > table. For all I can tell, SH7372 does not set it, while SH73A0, > > R8A7740 and R8A7779 always do. > > My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based > on the SoC in use. Ok, thanks for the confirmation. Just to be clear: Either way (compatible or separate property) works fine and can be used here. I tend to prefer basing these things on the "compatible" string in order to keep specific knowledge of the device internals out of the device tree binding though. 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 Thu, 7 Feb 2013, Simon Horman wrote: > On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote: > > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote: > > > +* Toshiba Mobile IO SD/MMC controller > > > + > > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to > > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform > > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either > > > +describe hardware capabilities, known to them, or are obtained by them from > > > +their own platform data or from their DT information. In the latter case all > > > +compulsory and any optional properties, common to all SD/MMC drivers, as > > > +described in mmc.txt, should or can be used. Additionally the following optional > > > +bindings can be used. They set respective TMIO_MMC_* flags. > > > + > > > +Optional properties: > > > +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag > > > +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES > > > +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT > > > > Please write the binding in a way that does not refer to a specific > > implementation in Linux: The binding should describe the hardware > > independent of details in the driver. In particular, I think you > > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe > > in text what the flags are about. > > > > Regarding the toshiba,mmc-wrprotect-disable property, would it be > > enough to just check the presence of the wp-gpios property? > > > > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in > > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't > > actually need to provide this here, but can keep that knowledge > > implicit based on whether we're talking to sh_mobile_sdhi > > or another tmio_mmc variant. Can do that, yes. > > For the other last one, is that actually board specific, or just > > a feature of a given chip? If we can tell by the SoC, then I'd > > suggest using separate "compatible" properties instead, and > > put a bitmask of features into the .data field of the of match > > table. For all I can tell, SH7372 does not set it, while SH73A0, > > R8A7740 and R8A7779 always do. > > My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based > on the SoC in use. So far TMIO_MMC_HAS_IDLE_WAIT is set on board-kzm9g.c (sh73a0 / AG5) board-ag5evm.c (sh73a0 / AG5) board-armadillo800eva.c (r8a7740 / A1) board-kota2.c (sh73a0 / AG5) board-marzen.c (r8a7779 / H1) and isn't set on board-ap4evb.c (sh7372 / ap4) board-bonito.c (r8a7740 / a1, SDHI isn't used) board-mackerel.c (sh7372 / ap4) So, shall we use a compatible property for this and drop this property? We can add later at any time, if needed, which is better, than defining something redundant. OTOH I seem to remember, that using SoC-version from the "compatible" property was considered by someone inappropriate. Magnus, what do you think? So, if we drop TMIO_MMC_BLKSZ_2BYTES and TMIO_MMC_HAS_IDLE_WAIT, we only keep toshiba,mmc-wrprotect-disable to set TMIO_MMC_WRPROTECT_DISABLE? And that one is definitely needed, because that even differs between SDHI instances on one SoC. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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 Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Thu, 7 Feb 2013, Simon Horman wrote: > >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote: >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote: >> > > +* Toshiba Mobile IO SD/MMC controller >> > > + >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either >> > > +describe hardware capabilities, known to them, or are obtained by them from >> > > +their own platform data or from their DT information. In the latter case all >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as >> > > +described in mmc.txt, should or can be used. Additionally the following optional >> > > +bindings can be used. They set respective TMIO_MMC_* flags. >> > > + >> > > +Optional properties: >> > > +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag >> > > +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES >> > > +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT >> > >> > Please write the binding in a way that does not refer to a specific >> > implementation in Linux: The binding should describe the hardware >> > independent of details in the driver. In particular, I think you >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe >> > in text what the flags are about. >> > >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be >> > enough to just check the presence of the wp-gpios property? >> > >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't >> > actually need to provide this here, but can keep that knowledge >> > implicit based on whether we're talking to sh_mobile_sdhi >> > or another tmio_mmc variant. > > Can do that, yes. > >> > For the other last one, is that actually board specific, or just >> > a feature of a given chip? If we can tell by the SoC, then I'd >> > suggest using separate "compatible" properties instead, and >> > put a bitmask of features into the .data field of the of match >> > table. For all I can tell, SH7372 does not set it, while SH73A0, >> > R8A7740 and R8A7779 always do. >> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based >> on the SoC in use. > > So far TMIO_MMC_HAS_IDLE_WAIT is set on > > board-kzm9g.c (sh73a0 / AG5) > board-ag5evm.c (sh73a0 / AG5) > board-armadillo800eva.c (r8a7740 / A1) > board-kota2.c (sh73a0 / AG5) > board-marzen.c (r8a7779 / H1) > > and isn't set on > > board-ap4evb.c (sh7372 / ap4) > board-bonito.c (r8a7740 / a1, SDHI isn't used) > board-mackerel.c (sh7372 / ap4) > > So, shall we use a compatible property for this and drop this property? We > can add later at any time, if needed, which is better, than defining > something redundant. OTOH I seem to remember, that using SoC-version from > the "compatible" property was considered by someone inappropriate. Magnus, > what do you think? I prefer you to use a hardware-block version compatible suffix instead of SoC suffix. This since we have more SoCs than actual hardware block configurations. Using the list above, how many configurations would we have? Actually, forcing the drivers to be updated for each new SoC sounds like a pretty terrible idea. Wouldn't that be against one of the merits of using DT? Also, don't you have enough interesting work piled up already? =) Basically, I can't see any point in adding an extra unnecessary need for updating the drivers when there is no real functional change. Thanks, / magnus -- 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 Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote: > On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > On Thu, 7 Feb 2013, Simon Horman wrote: > > > >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote: > >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote: > >> > > +* Toshiba Mobile IO SD/MMC controller > >> > > + > >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to > >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform > >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either > >> > > +describe hardware capabilities, known to them, or are obtained by them from > >> > > +their own platform data or from their DT information. In the latter case all > >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as > >> > > +described in mmc.txt, should or can be used. Additionally the following optional > >> > > +bindings can be used. They set respective TMIO_MMC_* flags. > >> > > + > >> > > +Optional properties: > >> > > +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag > >> > > +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES > >> > > +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT > >> > > >> > Please write the binding in a way that does not refer to a specific > >> > implementation in Linux: The binding should describe the hardware > >> > independent of details in the driver. In particular, I think you > >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe > >> > in text what the flags are about. > >> > > >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be > >> > enough to just check the presence of the wp-gpios property? > >> > > >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in > >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't > >> > actually need to provide this here, but can keep that knowledge > >> > implicit based on whether we're talking to sh_mobile_sdhi > >> > or another tmio_mmc variant. > > > > Can do that, yes. > > > >> > For the other last one, is that actually board specific, or just > >> > a feature of a given chip? If we can tell by the SoC, then I'd > >> > suggest using separate "compatible" properties instead, and > >> > put a bitmask of features into the .data field of the of match > >> > table. For all I can tell, SH7372 does not set it, while SH73A0, > >> > R8A7740 and R8A7779 always do. > >> > >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based > >> on the SoC in use. > > > > So far TMIO_MMC_HAS_IDLE_WAIT is set on > > > > board-kzm9g.c (sh73a0 / AG5) > > board-ag5evm.c (sh73a0 / AG5) > > board-armadillo800eva.c (r8a7740 / A1) > > board-kota2.c (sh73a0 / AG5) > > board-marzen.c (r8a7779 / H1) > > > > and isn't set on > > > > board-ap4evb.c (sh7372 / ap4) > > board-bonito.c (r8a7740 / a1, SDHI isn't used) > > board-mackerel.c (sh7372 / ap4) > > > > So, shall we use a compatible property for this and drop this property? We > > can add later at any time, if needed, which is better, than defining > > something redundant. OTOH I seem to remember, that using SoC-version from > > the "compatible" property was considered by someone inappropriate. Magnus, > > what do you think? > > I prefer you to use a hardware-block version compatible suffix instead > of SoC suffix. > > This since we have more SoCs than actual hardware block > configurations. Using the list above, how many configurations would we > have? > > Actually, forcing the drivers to be updated for each new SoC sounds > like a pretty terrible idea. Wouldn't that be against one of the > merits of using DT? Also, don't you have enough interesting work piled > up already? =) > > Basically, I can't see any point in adding an extra unnecessary need > for updating the drivers when there is no real functional change. My understanding is that the discussion is about the details of bindings that are required for SDHI to function when brought up using DT on a variety of boards. Not exciting new work. In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT. -- 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 Thu, Feb 14, 2013 at 10:59 AM, Simon Horman <horms@verge.net.au> wrote: > On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote: >> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski >> <g.liakhovetski@gmx.de> wrote: >> > On Thu, 7 Feb 2013, Simon Horman wrote: >> > >> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote: >> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote: >> >> > > +* Toshiba Mobile IO SD/MMC controller >> >> > > + >> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to >> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform >> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either >> >> > > +describe hardware capabilities, known to them, or are obtained by them from >> >> > > +their own platform data or from their DT information. In the latter case all >> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as >> >> > > +described in mmc.txt, should or can be used. Additionally the following optional >> >> > > +bindings can be used. They set respective TMIO_MMC_* flags. >> >> > > + >> >> > > +Optional properties: >> >> > > +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag >> >> > > +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES >> >> > > +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT >> >> > >> >> > Please write the binding in a way that does not refer to a specific >> >> > implementation in Linux: The binding should describe the hardware >> >> > independent of details in the driver. In particular, I think you >> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe >> >> > in text what the flags are about. >> >> > >> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be >> >> > enough to just check the presence of the wp-gpios property? >> >> > >> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in >> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't >> >> > actually need to provide this here, but can keep that knowledge >> >> > implicit based on whether we're talking to sh_mobile_sdhi >> >> > or another tmio_mmc variant. >> > >> > Can do that, yes. >> > >> >> > For the other last one, is that actually board specific, or just >> >> > a feature of a given chip? If we can tell by the SoC, then I'd >> >> > suggest using separate "compatible" properties instead, and >> >> > put a bitmask of features into the .data field of the of match >> >> > table. For all I can tell, SH7372 does not set it, while SH73A0, >> >> > R8A7740 and R8A7779 always do. >> >> >> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based >> >> on the SoC in use. >> > >> > So far TMIO_MMC_HAS_IDLE_WAIT is set on >> > >> > board-kzm9g.c (sh73a0 / AG5) >> > board-ag5evm.c (sh73a0 / AG5) >> > board-armadillo800eva.c (r8a7740 / A1) >> > board-kota2.c (sh73a0 / AG5) >> > board-marzen.c (r8a7779 / H1) >> > >> > and isn't set on >> > >> > board-ap4evb.c (sh7372 / ap4) >> > board-bonito.c (r8a7740 / a1, SDHI isn't used) >> > board-mackerel.c (sh7372 / ap4) >> > >> > So, shall we use a compatible property for this and drop this property? We >> > can add later at any time, if needed, which is better, than defining >> > something redundant. OTOH I seem to remember, that using SoC-version from >> > the "compatible" property was considered by someone inappropriate. Magnus, >> > what do you think? >> >> I prefer you to use a hardware-block version compatible suffix instead >> of SoC suffix. >> >> This since we have more SoCs than actual hardware block >> configurations. Using the list above, how many configurations would we >> have? >> >> Actually, forcing the drivers to be updated for each new SoC sounds >> like a pretty terrible idea. Wouldn't that be against one of the >> merits of using DT? Also, don't you have enough interesting work piled >> up already? =) >> >> Basically, I can't see any point in adding an extra unnecessary need >> for updating the drivers when there is no real functional change. > > My understanding is that the discussion is about the details of > bindings that are required for SDHI to function when brought > up using DT on a variety of boards. Not exciting new work. > > In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT. No, not exciting new work. More describing certain versions of the SDHI hardware. This is a SDHI-specific configuration which may end up being used on a certain SoC. There are also some board specific details that need to be taken into consideration. I believe it is important to understand the difference between hardware-block configuration (SDHI in this particular case), SoC and board. So the way I see it we have 3 ways to deal with it: 1) Use the "toshiba,mmc-has-idle-wait" property proposed by Guennadi or 2) Use a SoC suffix in the compatible string and deal with TMIO_MMC_HAS_IDLE_WAIT in the driver or 3) Use a SDHI-specific version suffix in the compatible string and deal with TMIO_MMC_HAS_IDLE_WAIT in the driver I am fine with 1) or 3) but I don't want to go down the route of 2) because it will just lead to more pointless driver changes than are actually needed. And the TMIO_MMC_HAS_IDLE_WAIT flag is not SoC-specific so using a SoC suffix seems incorrect to me. Thanks, / magnus -- 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 Thu, Feb 14, 2013 at 11:09:06AM +0900, Magnus Damm wrote: > On Thu, Feb 14, 2013 at 10:59 AM, Simon Horman <horms@verge.net.au> wrote: > > On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote: > >> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski > >> <g.liakhovetski@gmx.de> wrote: > >> > On Thu, 7 Feb 2013, Simon Horman wrote: > >> > > >> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote: > >> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote: > >> >> > > +* Toshiba Mobile IO SD/MMC controller > >> >> > > + > >> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to > >> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform > >> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either > >> >> > > +describe hardware capabilities, known to them, or are obtained by them from > >> >> > > +their own platform data or from their DT information. In the latter case all > >> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as > >> >> > > +described in mmc.txt, should or can be used. Additionally the following optional > >> >> > > +bindings can be used. They set respective TMIO_MMC_* flags. > >> >> > > + > >> >> > > +Optional properties: > >> >> > > +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag > >> >> > > +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES > >> >> > > +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT > >> >> > > >> >> > Please write the binding in a way that does not refer to a specific > >> >> > implementation in Linux: The binding should describe the hardware > >> >> > independent of details in the driver. In particular, I think you > >> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe > >> >> > in text what the flags are about. > >> >> > > >> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be > >> >> > enough to just check the presence of the wp-gpios property? > >> >> > > >> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in > >> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't > >> >> > actually need to provide this here, but can keep that knowledge > >> >> > implicit based on whether we're talking to sh_mobile_sdhi > >> >> > or another tmio_mmc variant. > >> > > >> > Can do that, yes. > >> > > >> >> > For the other last one, is that actually board specific, or just > >> >> > a feature of a given chip? If we can tell by the SoC, then I'd > >> >> > suggest using separate "compatible" properties instead, and > >> >> > put a bitmask of features into the .data field of the of match > >> >> > table. For all I can tell, SH7372 does not set it, while SH73A0, > >> >> > R8A7740 and R8A7779 always do. > >> >> > >> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based > >> >> on the SoC in use. > >> > > >> > So far TMIO_MMC_HAS_IDLE_WAIT is set on > >> > > >> > board-kzm9g.c (sh73a0 / AG5) > >> > board-ag5evm.c (sh73a0 / AG5) > >> > board-armadillo800eva.c (r8a7740 / A1) > >> > board-kota2.c (sh73a0 / AG5) > >> > board-marzen.c (r8a7779 / H1) > >> > > >> > and isn't set on > >> > > >> > board-ap4evb.c (sh7372 / ap4) > >> > board-bonito.c (r8a7740 / a1, SDHI isn't used) > >> > board-mackerel.c (sh7372 / ap4) > >> > > >> > So, shall we use a compatible property for this and drop this property? We > >> > can add later at any time, if needed, which is better, than defining > >> > something redundant. OTOH I seem to remember, that using SoC-version from > >> > the "compatible" property was considered by someone inappropriate. Magnus, > >> > what do you think? > >> > >> I prefer you to use a hardware-block version compatible suffix instead > >> of SoC suffix. > >> > >> This since we have more SoCs than actual hardware block > >> configurations. Using the list above, how many configurations would we > >> have? > >> > >> Actually, forcing the drivers to be updated for each new SoC sounds > >> like a pretty terrible idea. Wouldn't that be against one of the > >> merits of using DT? Also, don't you have enough interesting work piled > >> up already? =) > >> > >> Basically, I can't see any point in adding an extra unnecessary need > >> for updating the drivers when there is no real functional change. > > > > My understanding is that the discussion is about the details of > > bindings that are required for SDHI to function when brought > > up using DT on a variety of boards. Not exciting new work. > > > > In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT. > > No, not exciting new work. More describing certain versions of the > SDHI hardware. This is a SDHI-specific configuration which may end up > being used on a certain SoC. There are also some board specific > details that need to be taken into consideration. I believe it is > important to understand the difference between hardware-block > configuration (SDHI in this particular case), SoC and board. > > So the way I see it we have 3 ways to deal with it: > > 1) Use the "toshiba,mmc-has-idle-wait" property proposed by Guennadi > or > 2) Use a SoC suffix in the compatible string and deal with > TMIO_MMC_HAS_IDLE_WAIT in the driver > or > 3) Use a SDHI-specific version suffix in the compatible string and > deal with TMIO_MMC_HAS_IDLE_WAIT in the driver > > I am fine with 1) or 3) but I don't want to go down the route of 2) > because it will just lead to more pointless driver changes than are > actually needed. And the TMIO_MMC_HAS_IDLE_WAIT flag is not > SoC-specific so using a SoC suffix seems incorrect to me. It seems that 3) coincides best with your desires and Arnd's feedback to date. -- 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 Thu, 14 Feb 2013, Magnus Damm wrote: > On Thu, Feb 14, 2013 at 10:59 AM, Simon Horman <horms@verge.net.au> wrote: > > On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote: > >> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski > >> <g.liakhovetski@gmx.de> wrote: > >> > On Thu, 7 Feb 2013, Simon Horman wrote: > >> > > >> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote: > >> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote: > >> >> > > +* Toshiba Mobile IO SD/MMC controller > >> >> > > + > >> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to > >> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform > >> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either > >> >> > > +describe hardware capabilities, known to them, or are obtained by them from > >> >> > > +their own platform data or from their DT information. In the latter case all > >> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as > >> >> > > +described in mmc.txt, should or can be used. Additionally the following optional > >> >> > > +bindings can be used. They set respective TMIO_MMC_* flags. > >> >> > > + > >> >> > > +Optional properties: > >> >> > > +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag > >> >> > > +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES > >> >> > > +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT > >> >> > > >> >> > Please write the binding in a way that does not refer to a specific > >> >> > implementation in Linux: The binding should describe the hardware > >> >> > independent of details in the driver. In particular, I think you > >> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe > >> >> > in text what the flags are about. > >> >> > > >> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be > >> >> > enough to just check the presence of the wp-gpios property? No, normally WP would be implemented, using the native dedicated SDHI WP pin. So, toshiba,mmc-wrprotect-disable is used when no such pin is available on this interface, or it's not routed to the socket and no GPIO is used for that either. So, this flag is the only way to know, whether a WP pin is used, if no wp-gpios is used. > >> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in > >> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't > >> >> > actually need to provide this here, but can keep that knowledge > >> >> > implicit based on whether we're talking to sh_mobile_sdhi > >> >> > or another tmio_mmc variant. > >> > > >> > Can do that, yes. > >> > > >> >> > For the other last one, is that actually board specific, or just > >> >> > a feature of a given chip? If we can tell by the SoC, then I'd > >> >> > suggest using separate "compatible" properties instead, and > >> >> > put a bitmask of features into the .data field of the of match > >> >> > table. For all I can tell, SH7372 does not set it, while SH73A0, > >> >> > R8A7740 and R8A7779 always do. > >> >> > >> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based > >> >> on the SoC in use. > >> > > >> > So far TMIO_MMC_HAS_IDLE_WAIT is set on > >> > > >> > board-kzm9g.c (sh73a0 / AG5) > >> > board-ag5evm.c (sh73a0 / AG5) > >> > board-armadillo800eva.c (r8a7740 / A1) > >> > board-kota2.c (sh73a0 / AG5) > >> > board-marzen.c (r8a7779 / H1) > >> > > >> > and isn't set on > >> > > >> > board-ap4evb.c (sh7372 / ap4) > >> > board-bonito.c (r8a7740 / a1, SDHI isn't used) > >> > board-mackerel.c (sh7372 / ap4) > >> > > >> > So, shall we use a compatible property for this and drop this property? We > >> > can add later at any time, if needed, which is better, than defining > >> > something redundant. OTOH I seem to remember, that using SoC-version from > >> > the "compatible" property was considered by someone inappropriate. Magnus, > >> > what do you think? > >> > >> I prefer you to use a hardware-block version compatible suffix instead > >> of SoC suffix. > >> > >> This since we have more SoCs than actual hardware block > >> configurations. Using the list above, how many configurations would we > >> have? > >> > >> Actually, forcing the drivers to be updated for each new SoC sounds > >> like a pretty terrible idea. Wouldn't that be against one of the > >> merits of using DT? Also, don't you have enough interesting work piled > >> up already? =) > >> > >> Basically, I can't see any point in adding an extra unnecessary need > >> for updating the drivers when there is no real functional change. > > > > My understanding is that the discussion is about the details of > > bindings that are required for SDHI to function when brought > > up using DT on a variety of boards. Not exciting new work. > > > > In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT. > > No, not exciting new work. More describing certain versions of the > SDHI hardware. This is a SDHI-specific configuration which may end up > being used on a certain SoC. There are also some board specific > details that need to be taken into consideration. I believe it is > important to understand the difference between hardware-block > configuration (SDHI in this particular case), SoC and board. > > So the way I see it we have 3 ways to deal with it: > > 1) Use the "toshiba,mmc-has-idle-wait" property proposed by Guennadi > or > 2) Use a SoC suffix in the compatible string and deal with > TMIO_MMC_HAS_IDLE_WAIT in the driver > or > 3) Use a SDHI-specific version suffix in the compatible string and > deal with TMIO_MMC_HAS_IDLE_WAIT in the driver > > I am fine with 1) or 3) but I don't want to go down the route of 2) > because it will just lead to more pointless driver changes than are > actually needed. And the TMIO_MMC_HAS_IDLE_WAIT flag is not > SoC-specific so using a SoC suffix seems incorrect to me. My take on this is the following: having N optionally available on different IP-versions features, I'd rather have N DT properties, than up to 2^N abstract vX versions. Yes, I realise, that in practice we'll never have 2^N, rather just a bit more than N, still, my main problem with those versions, is that they are purely abstracted. I'd be happy if that was a real hardware revision string, that you could look up in a datasheet. But if you have to look in some text file... That just seems much less user-friendly to me, than selecting single properties. Just imagine, what would you prefer, either specifying feature-A; feature-B; in your .dts or going through a list of v1: has no features v2: feature A only v3: feature B only v4: features A and B ... Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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 Guennadi, [Added Laurent to CC] On Thu, Feb 14, 2013 at 5:28 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Thu, 14 Feb 2013, Magnus Damm wrote: > >> On Thu, Feb 14, 2013 at 10:59 AM, Simon Horman <horms@verge.net.au> wrote: >> > On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote: >> >> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski >> >> <g.liakhovetski@gmx.de> wrote: >> >> > On Thu, 7 Feb 2013, Simon Horman wrote: >> >> > >> >> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote: >> >> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote: >> >> >> > > +* Toshiba Mobile IO SD/MMC controller >> >> >> > > + >> >> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to >> >> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform >> >> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either >> >> >> > > +describe hardware capabilities, known to them, or are obtained by them from >> >> >> > > +their own platform data or from their DT information. In the latter case all >> >> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as >> >> >> > > +described in mmc.txt, should or can be used. Additionally the following optional >> >> >> > > +bindings can be used. They set respective TMIO_MMC_* flags. >> >> >> > > + >> >> >> > > +Optional properties: >> >> >> > > +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag >> >> >> > > +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES >> >> >> > > +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT >> >> >> > >> >> >> > Please write the binding in a way that does not refer to a specific >> >> >> > implementation in Linux: The binding should describe the hardware >> >> >> > independent of details in the driver. In particular, I think you >> >> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe >> >> >> > in text what the flags are about. >> >> >> > >> >> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be >> >> >> > enough to just check the presence of the wp-gpios property? > > No, normally WP would be implemented, using the native dedicated SDHI WP > pin. So, toshiba,mmc-wrprotect-disable is used when no such pin is > available on this interface, or it's not routed to the socket and no GPIO > is used for that either. So, this flag is the only way to know, whether a > WP pin is used, if no wp-gpios is used. Can't we request this information via PINCTRL somehow? I feel that I may have asked this before, so correct me if I am going in circles. If we have a native SDHI WP pin and it is used on our board then this information is known with the board configuration already. And that pin function needs to be select. Same thing if we happen to have a GPIO pin for WP, then that need to be selected as well. And if we have neither SDHI WP nor GPIO WP then we don't have to bother with the WP signal at all. >> >> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in >> >> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't >> >> >> > actually need to provide this here, but can keep that knowledge >> >> >> > implicit based on whether we're talking to sh_mobile_sdhi >> >> >> > or another tmio_mmc variant. >> >> > >> >> > Can do that, yes. >> >> > >> >> >> > For the other last one, is that actually board specific, or just >> >> >> > a feature of a given chip? If we can tell by the SoC, then I'd >> >> >> > suggest using separate "compatible" properties instead, and >> >> >> > put a bitmask of features into the .data field of the of match >> >> >> > table. For all I can tell, SH7372 does not set it, while SH73A0, >> >> >> > R8A7740 and R8A7779 always do. >> >> >> >> >> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based >> >> >> on the SoC in use. >> >> > >> >> > So far TMIO_MMC_HAS_IDLE_WAIT is set on >> >> > >> >> > board-kzm9g.c (sh73a0 / AG5) >> >> > board-ag5evm.c (sh73a0 / AG5) >> >> > board-armadillo800eva.c (r8a7740 / A1) >> >> > board-kota2.c (sh73a0 / AG5) >> >> > board-marzen.c (r8a7779 / H1) >> >> > >> >> > and isn't set on >> >> > >> >> > board-ap4evb.c (sh7372 / ap4) >> >> > board-bonito.c (r8a7740 / a1, SDHI isn't used) >> >> > board-mackerel.c (sh7372 / ap4) >> >> > >> >> > So, shall we use a compatible property for this and drop this property? We >> >> > can add later at any time, if needed, which is better, than defining >> >> > something redundant. OTOH I seem to remember, that using SoC-version from >> >> > the "compatible" property was considered by someone inappropriate. Magnus, >> >> > what do you think? >> >> >> >> I prefer you to use a hardware-block version compatible suffix instead >> >> of SoC suffix. >> >> >> >> This since we have more SoCs than actual hardware block >> >> configurations. Using the list above, how many configurations would we >> >> have? >> >> >> >> Actually, forcing the drivers to be updated for each new SoC sounds >> >> like a pretty terrible idea. Wouldn't that be against one of the >> >> merits of using DT? Also, don't you have enough interesting work piled >> >> up already? =) >> >> >> >> Basically, I can't see any point in adding an extra unnecessary need >> >> for updating the drivers when there is no real functional change. >> > >> > My understanding is that the discussion is about the details of >> > bindings that are required for SDHI to function when brought >> > up using DT on a variety of boards. Not exciting new work. >> > >> > In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT. >> >> No, not exciting new work. More describing certain versions of the >> SDHI hardware. This is a SDHI-specific configuration which may end up >> being used on a certain SoC. There are also some board specific >> details that need to be taken into consideration. I believe it is >> important to understand the difference between hardware-block >> configuration (SDHI in this particular case), SoC and board. >> >> So the way I see it we have 3 ways to deal with it: >> >> 1) Use the "toshiba,mmc-has-idle-wait" property proposed by Guennadi >> or >> 2) Use a SoC suffix in the compatible string and deal with >> TMIO_MMC_HAS_IDLE_WAIT in the driver >> or >> 3) Use a SDHI-specific version suffix in the compatible string and >> deal with TMIO_MMC_HAS_IDLE_WAIT in the driver >> >> I am fine with 1) or 3) but I don't want to go down the route of 2) >> because it will just lead to more pointless driver changes than are >> actually needed. And the TMIO_MMC_HAS_IDLE_WAIT flag is not >> SoC-specific so using a SoC suffix seems incorrect to me. > > My take on this is the following: having N optionally available on > different IP-versions features, I'd rather have N DT properties, than up > to 2^N abstract vX versions. Yes, I realise, that in practice we'll never > have 2^N, rather just a bit more than N, still, my main problem with those > versions, is that they are purely abstracted. I'd be happy if that was a > real hardware revision string, that you could look up in a datasheet. But > if you have to look in some text file... That just seems much less > user-friendly to me, than selecting single properties. Just imagine, what > would you prefer, either specifying > > feature-A; > feature-B; > > in your .dts or going through a list of > > v1: has no features > v2: feature A only > v3: feature B only > v4: features A and B > ... In this particular case based on the information above: toshiba,mmc-blksz-2bytes toshiba,mmc-has-idle-wait or my guess: renesas,shmobile-sdhi-v1: TMIO_MMC_BLKSZ_2BYTES renesas,shmobile-sdhi-v2: TMIO_MMC_BLKSZ_2BYTES + TMIO_MMC_HAS_IDLE_WAIT I think the WP and CD pins should be handled differently if possible. Thanks, / magnus -- 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 Magnus On Thu, 14 Feb 2013, Magnus Damm wrote: > Hi Guennadi, > > [Added Laurent to CC] > > On Thu, Feb 14, 2013 at 5:28 PM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > On Thu, 14 Feb 2013, Magnus Damm wrote: > > > >> On Thu, Feb 14, 2013 at 10:59 AM, Simon Horman <horms@verge.net.au> wrote: > >> > On Thu, Feb 14, 2013 at 10:42:21AM +0900, Magnus Damm wrote: > >> >> On Thu, Feb 14, 2013 at 12:59 AM, Guennadi Liakhovetski > >> >> <g.liakhovetski@gmx.de> wrote: > >> >> > On Thu, 7 Feb 2013, Simon Horman wrote: > >> >> > > >> >> >> On Wed, Feb 06, 2013 at 10:24:20PM +0000, Arnd Bergmann wrote: > >> >> >> > On Wednesday 06 February 2013, Guennadi Liakhovetski wrote: > >> >> >> > > +* Toshiba Mobile IO SD/MMC controller > >> >> >> > > + > >> >> >> > > +The tmio-mmc driver doesn't probe its devices actively, instead its binding to > >> >> >> > > +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform > >> >> >> > > +driver. Those drivers supply the tmio-mmc driver with platform data, that either > >> >> >> > > +describe hardware capabilities, known to them, or are obtained by them from > >> >> >> > > +their own platform data or from their DT information. In the latter case all > >> >> >> > > +compulsory and any optional properties, common to all SD/MMC drivers, as > >> >> >> > > +described in mmc.txt, should or can be used. Additionally the following optional > >> >> >> > > +bindings can be used. They set respective TMIO_MMC_* flags. > >> >> >> > > + > >> >> >> > > +Optional properties: > >> >> >> > > +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag > >> >> >> > > +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES > >> >> >> > > +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT > >> >> >> > > >> >> >> > Please write the binding in a way that does not refer to a specific > >> >> >> > implementation in Linux: The binding should describe the hardware > >> >> >> > independent of details in the driver. In particular, I think you > >> >> >> > should not refer to the TMIO_MMC_BLKSZ_2BYTES etc macros but describe > >> >> >> > in text what the flags are about. > >> >> >> > > >> >> >> > Regarding the toshiba,mmc-wrprotect-disable property, would it be > >> >> >> > enough to just check the presence of the wp-gpios property? > > > > No, normally WP would be implemented, using the native dedicated SDHI WP > > pin. So, toshiba,mmc-wrprotect-disable is used when no such pin is > > available on this interface, or it's not routed to the socket and no GPIO > > is used for that either. So, this flag is the only way to know, whether a > > WP pin is used, if no wp-gpios is used. > > Can't we request this information via PINCTRL somehow? I feel that I > may have asked this before, so correct me if I am going in circles. Yes, you have. And we did want to do that, and I even proposed patches, but the status hasn't changed, I'm afraid: the pinctrl maintainer doesn't want that API, he considers it going against the pinctrl concept. He doesn't want drivers to inquire pinctrl about the status... What you could do though, you could probably create a special "wp" configuration and try to request that and see whether that succeeds... I think there was a gotcha with that too, which is why you normally only want to request the default settings. But maybe this can be reconsidered. I hope I passed the contents of that old discussion correctly, if not - sorry, feel free to correct me. > If we have a native SDHI WP pin and it is used on our board then this > information is known with the board configuration already. And that > pin function needs to be select. Same thing if we happen to have a > GPIO pin for WP, then that need to be selected as well. And if we have > neither SDHI WP nor GPIO WP then we don't have to bother with the WP > signal at all. > > >> >> >> > TMIO_MMC_BLKSZ_2BYTES seems to be set unconditionally in > >> >> >> > sh_mobile_sdhi_probe and nowhere else, so I'd assume we don't > >> >> >> > actually need to provide this here, but can keep that knowledge > >> >> >> > implicit based on whether we're talking to sh_mobile_sdhi > >> >> >> > or another tmio_mmc variant. > >> >> > > >> >> > Can do that, yes. > >> >> > > >> >> >> > For the other last one, is that actually board specific, or just > >> >> >> > a feature of a given chip? If we can tell by the SoC, then I'd > >> >> >> > suggest using separate "compatible" properties instead, and > >> >> >> > put a bitmask of features into the .data field of the of match > >> >> >> > table. For all I can tell, SH7372 does not set it, while SH73A0, > >> >> >> > R8A7740 and R8A7779 always do. > >> >> >> > >> >> >> My understanding is that TMIO_MMC_HAS_IDLE_WAIT can be set based > >> >> >> on the SoC in use. > >> >> > > >> >> > So far TMIO_MMC_HAS_IDLE_WAIT is set on > >> >> > > >> >> > board-kzm9g.c (sh73a0 / AG5) > >> >> > board-ag5evm.c (sh73a0 / AG5) > >> >> > board-armadillo800eva.c (r8a7740 / A1) > >> >> > board-kota2.c (sh73a0 / AG5) > >> >> > board-marzen.c (r8a7779 / H1) > >> >> > > >> >> > and isn't set on > >> >> > > >> >> > board-ap4evb.c (sh7372 / ap4) > >> >> > board-bonito.c (r8a7740 / a1, SDHI isn't used) > >> >> > board-mackerel.c (sh7372 / ap4) > >> >> > > >> >> > So, shall we use a compatible property for this and drop this property? We > >> >> > can add later at any time, if needed, which is better, than defining > >> >> > something redundant. OTOH I seem to remember, that using SoC-version from > >> >> > the "compatible" property was considered by someone inappropriate. Magnus, > >> >> > what do you think? > >> >> > >> >> I prefer you to use a hardware-block version compatible suffix instead > >> >> of SoC suffix. > >> >> > >> >> This since we have more SoCs than actual hardware block > >> >> configurations. Using the list above, how many configurations would we > >> >> have? > >> >> > >> >> Actually, forcing the drivers to be updated for each new SoC sounds > >> >> like a pretty terrible idea. Wouldn't that be against one of the > >> >> merits of using DT? Also, don't you have enough interesting work piled > >> >> up already? =) > >> >> > >> >> Basically, I can't see any point in adding an extra unnecessary need > >> >> for updating the drivers when there is no real functional change. > >> > > >> > My understanding is that the discussion is about the details of > >> > bindings that are required for SDHI to function when brought > >> > up using DT on a variety of boards. Not exciting new work. > >> > > >> > In particular how to set TMIO_MMC_HAS_IDLE_WAIT via DT. > >> > >> No, not exciting new work. More describing certain versions of the > >> SDHI hardware. This is a SDHI-specific configuration which may end up > >> being used on a certain SoC. There are also some board specific > >> details that need to be taken into consideration. I believe it is > >> important to understand the difference between hardware-block > >> configuration (SDHI in this particular case), SoC and board. > >> > >> So the way I see it we have 3 ways to deal with it: > >> > >> 1) Use the "toshiba,mmc-has-idle-wait" property proposed by Guennadi > >> or > >> 2) Use a SoC suffix in the compatible string and deal with > >> TMIO_MMC_HAS_IDLE_WAIT in the driver > >> or > >> 3) Use a SDHI-specific version suffix in the compatible string and > >> deal with TMIO_MMC_HAS_IDLE_WAIT in the driver > >> > >> I am fine with 1) or 3) but I don't want to go down the route of 2) > >> because it will just lead to more pointless driver changes than are > >> actually needed. And the TMIO_MMC_HAS_IDLE_WAIT flag is not > >> SoC-specific so using a SoC suffix seems incorrect to me. > > > > My take on this is the following: having N optionally available on > > different IP-versions features, I'd rather have N DT properties, than up > > to 2^N abstract vX versions. Yes, I realise, that in practice we'll never > > have 2^N, rather just a bit more than N, still, my main problem with those > > versions, is that they are purely abstracted. I'd be happy if that was a > > real hardware revision string, that you could look up in a datasheet. But > > if you have to look in some text file... That just seems much less > > user-friendly to me, than selecting single properties. Just imagine, what > > would you prefer, either specifying > > > > feature-A; > > feature-B; > > > > in your .dts or going through a list of > > > > v1: has no features > > v2: feature A only > > v3: feature B only > > v4: features A and B > > ... > > In this particular case based on the information above: > > toshiba,mmc-blksz-2bytes > toshiba,mmc-has-idle-wait > > or my guess: > > renesas,shmobile-sdhi-v1: TMIO_MMC_BLKSZ_2BYTES > renesas,shmobile-sdhi-v2: TMIO_MMC_BLKSZ_2BYTES + TMIO_MMC_HAS_IDLE_WAIT toshiba,mmc-blksz-2bytes we don't need - it's set for all SDHI devices. So, _so far_ we only have one feature, that we have to specify in DT either via a property or a compatible version string. > I think the WP and CD pins should be handled differently if possible. WP we're currently discussing, for CD there's already a common MMC binding: broken-cd. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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 Thursday 14 February 2013, Guennadi Liakhovetski wrote: > On Thu, 14 Feb 2013, Magnus Damm wrote: > > On Thu, Feb 14, 2013 at 5:28 PM, Guennadi Liakhovetski > > > My take on this is the following: having N optionally available on > > > different IP-versions features, I'd rather have N DT properties, than up > > > to 2^N abstract vX versions. Yes, I realise, that in practice we'll never > > > have 2^N, rather just a bit more than N, still, my main problem with those > > > versions, is that they are purely abstracted. I'd be happy if that was a > > > real hardware revision string, that you could look up in a datasheet. But > > > if you have to look in some text file... That just seems much less > > > user-friendly to me, than selecting single properties. Just imagine, what > > > would you prefer, either specifying > > > > > > feature-A; > > > feature-B; > > > > > > in your .dts or going through a list of > > > > > > v1: has no features > > > v2: feature A only > > > v3: feature B only > > > v4: features A and B > > > ... > > > > In this particular case based on the information above: > > > > toshiba,mmc-blksz-2bytes > > toshiba,mmc-has-idle-wait > > > > or my guess: > > > > renesas,shmobile-sdhi-v1: TMIO_MMC_BLKSZ_2BYTES > > renesas,shmobile-sdhi-v2: TMIO_MMC_BLKSZ_2BYTES + TMIO_MMC_HAS_IDLE_WAIT > > toshiba,mmc-blksz-2bytes we don't need - it's set for all SDHI devices. > So, _so far_ we only have one feature, that we have to specify in DT > either via a property or a compatible version string. Right. I would still prefer the different "compatible" property, but it's not an extremely important thing here. Regarding the naming of the "compatible" string, I would not use v1 and v2 postfixes though, unless that is what they are called in the data sheet. Ideally we would know the exact version of the IP block that went into the chip and drop the "shmobile-" part. That would make it something like "toshiba,sdhi-1.23.45". If you cannot easily find out those versions, the next best solution would be naming it after the chip that first used a particular version of that IP block, like "renesas,shmobile1234-sdhi". The device tree include file for shmobile5678 should then list the sdhi part as being compatible with the "reneasas,shmobile1234-sdhi" if they are the same, or as a separate "reneasas,shmobile5678-sdhi" if they behave in a different way. In either case, I would also list the "toshiba,sdhi" name as the more generic option in the "compatible" list, but that is optional. 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
diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt new file mode 100644 index 0000000..5762a55 --- /dev/null +++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt @@ -0,0 +1,15 @@ +* Toshiba Mobile IO SD/MMC controller + +The tmio-mmc driver doesn't probe its devices actively, instead its binding to +devices is managed by either MFD drivers or by the sh_mobile_sdhi platform +driver. Those drivers supply the tmio-mmc driver with platform data, that either +describe hardware capabilities, known to them, or are obtained by them from +their own platform data or from their DT information. In the latter case all +compulsory and any optional properties, common to all SD/MMC drivers, as +described in mmc.txt, should or can be used. Additionally the following optional +bindings can be used. They set respective TMIO_MMC_* flags. + +Optional properties: +- toshiba,mmc-wrprotect-disable : set TMIO_MMC_WRPROTECT_DISABLE flag +- toshiba,mmc-blksz-2bytes : set TMIO_MMC_BLKSZ_2BYTES +- toshiba,mmc-has-idle-wait : set TMIO_MMC_HAS_IDLE_WAIT
Define device-tree bindings for the tmio-mmc driver to be able to specify parameters, currently provided in platform data. Cc: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- v3: make the property to set TMIO_MMC_SDIO_IRQ global Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/mmc/tmio_mmc.txt