| Message ID | 1368802520-16378-8-git-send-email-g.liakhovetski@gmx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Guennadi, On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled" > state, those of them, available on APE6EVM, are then added to the board DT. > This version assignes fixed regulators to all the interfaces, in future > versions support for regulators should be added. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> Thanks for your work on this. In general I'm happy to see your progress but I wonder about the following: > --- a/arch/arm/boot/dts/r8a73a4.dtsi > +++ b/arch/arm/boot/dts/r8a73a4.dtsi > @@ -98,4 +98,49 @@ > gpio-controller; > #gpio-cells = <2>; > }; > + > + mmcif0: mmcif@ee200000 { > + compatible = "renesas,sh-mmcif"; > + reg = <0 0xee200000 0 0x100>; > + interrupt-parent = <&gic>; > + interrupts = <0 169 0x4>; > + reg-io-width = <4>; > + status = "disabled"; > + }; > + > + mmcif1: mmcif@ee220000 { > + compatible = "renesas,sh-mmcif"; > + reg = <0 0xee220000 0 0x100>; > + interrupt-parent = <&gic>; > + interrupts = <0 170 0x4>; > + reg-io-width = <4>; > + status = "disabled"; > + }; > + > + sdhi0: sdhi@ee100000 { > + compatible = "renesas,r8a7740-sdhi"; > + reg = <0 0xee100000 0 0x100>; > + interrupt-parent = <&gic>; > + interrupts = <0 165 4>; > + cap-sd-highspeed; > + status = "disabled"; > + }; Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC. Sorry to say this but to me this seems like the first steps toward a future binary compatibility mess. Others seem to handle this differently. For instance, in drivers/mmc/host/omap_hsmmc.c there are separate compatible strings for omap2 and omap3. As I stated before, I prefer to see using a compatible string based on the version of the IP block, or if we have to use the SoC name then use the name of the actual SoC used. What is the common way to deal with the compatible strings? Just use an existing one if it happens to match? Seems a lot like copy-paste integration to me with the added benefit of forever binary compatibility. Perhaps we have agreed on something already? Care to remind me? =) Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 19 Jun 2013, Magnus Damm wrote: > Hi Guennadi, > > On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled" > > state, those of them, available on APE6EVM, are then added to the board DT. > > This version assignes fixed regulators to all the interfaces, in future > > versions support for regulators should be added. > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> > > Thanks for your work on this. In general I'm happy to see your > progress but I wonder about the following: > > > --- a/arch/arm/boot/dts/r8a73a4.dtsi > > +++ b/arch/arm/boot/dts/r8a73a4.dtsi > > @@ -98,4 +98,49 @@ > > gpio-controller; > > #gpio-cells = <2>; > > }; > > + > > + mmcif0: mmcif@ee200000 { > > + compatible = "renesas,sh-mmcif"; > > + reg = <0 0xee200000 0 0x100>; > > + interrupt-parent = <&gic>; > > + interrupts = <0 169 0x4>; > > + reg-io-width = <4>; > > + status = "disabled"; > > + }; > > + > > + mmcif1: mmcif@ee220000 { > > + compatible = "renesas,sh-mmcif"; > > + reg = <0 0xee220000 0 0x100>; > > + interrupt-parent = <&gic>; > > + interrupts = <0 170 0x4>; > > + reg-io-width = <4>; > > + status = "disabled"; > > + }; > > + > > + sdhi0: sdhi@ee100000 { > > + compatible = "renesas,r8a7740-sdhi"; > > + reg = <0 0xee100000 0 0x100>; > > + interrupt-parent = <&gic>; > > + interrupts = <0 165 4>; > > + cap-sd-highspeed; > > + status = "disabled"; > > + }; > > Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC. > Sorry to say this but to me this seems like the first steps toward a > future binary compatibility mess. > > Others seem to handle this differently. For instance, in > drivers/mmc/host/omap_hsmmc.c there are separate compatible strings > for omap2 and omap3. > > As I stated before, I prefer to see using a compatible string based on > the version of the IP block, or if we have to use the SoC name then > use the name of the actual SoC used. > > What is the common way to deal with the compatible strings? Just use > an existing one if it happens to match? Seems a lot like copy-paste > integration to me with the added benefit of forever binary > compatibility. > > Perhaps we have agreed on something already? Care to remind me? =) Indeed this has been discussed extensively: http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus=19248 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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Fri, Jun 21, 2013 at 6:10 AM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Wed, 19 Jun 2013, Magnus Damm wrote: > >> Hi Guennadi, >> >> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski >> <g.liakhovetski@gmx.de> wrote: >> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled" >> > state, those of them, available on APE6EVM, are then added to the board DT. >> > This version assignes fixed regulators to all the interfaces, in future >> > versions support for regulators should be added. >> > >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> >> >> Thanks for your work on this. In general I'm happy to see your >> progress but I wonder about the following: >> >> > --- a/arch/arm/boot/dts/r8a73a4.dtsi >> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi >> > @@ -98,4 +98,49 @@ >> > gpio-controller; >> > #gpio-cells = <2>; >> > }; >> > + >> > + mmcif0: mmcif@ee200000 { >> > + compatible = "renesas,sh-mmcif"; >> > + reg = <0 0xee200000 0 0x100>; >> > + interrupt-parent = <&gic>; >> > + interrupts = <0 169 0x4>; >> > + reg-io-width = <4>; >> > + status = "disabled"; >> > + }; >> > + >> > + mmcif1: mmcif@ee220000 { >> > + compatible = "renesas,sh-mmcif"; >> > + reg = <0 0xee220000 0 0x100>; >> > + interrupt-parent = <&gic>; >> > + interrupts = <0 170 0x4>; >> > + reg-io-width = <4>; >> > + status = "disabled"; >> > + }; >> > + >> > + sdhi0: sdhi@ee100000 { >> > + compatible = "renesas,r8a7740-sdhi"; >> > + reg = <0 0xee100000 0 0x100>; >> > + interrupt-parent = <&gic>; >> > + interrupts = <0 165 4>; >> > + cap-sd-highspeed; >> > + status = "disabled"; >> > + }; >> >> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC. >> Sorry to say this but to me this seems like the first steps toward a >> future binary compatibility mess. >> >> Others seem to handle this differently. For instance, in >> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings >> for omap2 and omap3. >> >> As I stated before, I prefer to see using a compatible string based on >> the version of the IP block, or if we have to use the SoC name then >> use the name of the actual SoC used. >> >> What is the common way to deal with the compatible strings? Just use >> an existing one if it happens to match? Seems a lot like copy-paste >> integration to me with the added benefit of forever binary >> compatibility. >> >> Perhaps we have agreed on something already? Care to remind me? =) > > Indeed this has been discussed extensively: > > http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus=19248 Yes, I know that, but did we also come to a conclusion how to handle the compatible string in case we have several SoCs that happen to have similar compatible hardware block versions? It seems that the SDHI driver has one compatible string for sh7372, and when we remove sh7372 SoC support in the future it will be easy to remove that too. But how about r8a7740? If we have multiple different SoCs that use same SoC compatible string (r8a7740 in this case) then how can we age out these strings? My take on this that if you use the SoC name for the compatible string then you should also keep on updating the driver with this information so the DTS for the SoC can use the correct SoC name. Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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 Fri, 21 Jun 2013, Magnus Damm wrote: > Hi Guennadi, > > On Fri, Jun 21, 2013 at 6:10 AM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > On Wed, 19 Jun 2013, Magnus Damm wrote: > > > >> Hi Guennadi, > >> > >> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski > >> <g.liakhovetski@gmx.de> wrote: > >> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled" > >> > state, those of them, available on APE6EVM, are then added to the board DT. > >> > This version assignes fixed regulators to all the interfaces, in future > >> > versions support for regulators should be added. > >> > > >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> > >> > >> Thanks for your work on this. In general I'm happy to see your > >> progress but I wonder about the following: > >> > >> > --- a/arch/arm/boot/dts/r8a73a4.dtsi > >> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi > >> > @@ -98,4 +98,49 @@ > >> > gpio-controller; > >> > #gpio-cells = <2>; > >> > }; > >> > + > >> > + mmcif0: mmcif@ee200000 { > >> > + compatible = "renesas,sh-mmcif"; > >> > + reg = <0 0xee200000 0 0x100>; > >> > + interrupt-parent = <&gic>; > >> > + interrupts = <0 169 0x4>; > >> > + reg-io-width = <4>; > >> > + status = "disabled"; > >> > + }; > >> > + > >> > + mmcif1: mmcif@ee220000 { > >> > + compatible = "renesas,sh-mmcif"; > >> > + reg = <0 0xee220000 0 0x100>; > >> > + interrupt-parent = <&gic>; > >> > + interrupts = <0 170 0x4>; > >> > + reg-io-width = <4>; > >> > + status = "disabled"; > >> > + }; > >> > + > >> > + sdhi0: sdhi@ee100000 { > >> > + compatible = "renesas,r8a7740-sdhi"; > >> > + reg = <0 0xee100000 0 0x100>; > >> > + interrupt-parent = <&gic>; > >> > + interrupts = <0 165 4>; > >> > + cap-sd-highspeed; > >> > + status = "disabled"; > >> > + }; > >> > >> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC. > >> Sorry to say this but to me this seems like the first steps toward a > >> future binary compatibility mess. > >> > >> Others seem to handle this differently. For instance, in > >> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings > >> for omap2 and omap3. > >> > >> As I stated before, I prefer to see using a compatible string based on > >> the version of the IP block, or if we have to use the SoC name then > >> use the name of the actual SoC used. > >> > >> What is the common way to deal with the compatible strings? Just use > >> an existing one if it happens to match? Seems a lot like copy-paste > >> integration to me with the added benefit of forever binary > >> compatibility. > >> > >> Perhaps we have agreed on something already? Care to remind me? =) > > > > Indeed this has been discussed extensively: > > > > http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus=19248 > > Yes, I know that, but did we also come to a conclusion how to handle > the compatible string in case we have several SoCs that happen to have > similar compatible hardware block versions? > > It seems that the SDHI driver has one compatible string for sh7372, > and when we remove sh7372 SoC support in the future it will be easy to > remove that too. But how about r8a7740? If we have multiple different > SoCs that use same SoC compatible string (r8a7740 in this case) then > how can we age out these strings? > > My take on this that if you use the SoC name for the compatible string > then you should also keep on updating the driver with this information > so the DTS for the SoC can use the correct SoC name. A quote from the above link: "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." 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-sh" 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 Arnd to CC] On Fri, Jun 21, 2013 at 3:33 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > Hi Magnus > > On Fri, 21 Jun 2013, Magnus Damm wrote: > >> Hi Guennadi, >> >> On Fri, Jun 21, 2013 at 6:10 AM, Guennadi Liakhovetski >> <g.liakhovetski@gmx.de> wrote: >> > On Wed, 19 Jun 2013, Magnus Damm wrote: >> > >> >> Hi Guennadi, >> >> >> >> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski >> >> <g.liakhovetski@gmx.de> wrote: >> >> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled" >> >> > state, those of them, available on APE6EVM, are then added to the board DT. >> >> > This version assignes fixed regulators to all the interfaces, in future >> >> > versions support for regulators should be added. >> >> > >> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> >> >> >> >> Thanks for your work on this. In general I'm happy to see your >> >> progress but I wonder about the following: >> >> >> >> > --- a/arch/arm/boot/dts/r8a73a4.dtsi >> >> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi >> >> > @@ -98,4 +98,49 @@ >> >> > gpio-controller; >> >> > #gpio-cells = <2>; >> >> > }; >> >> > + >> >> > + mmcif0: mmcif@ee200000 { >> >> > + compatible = "renesas,sh-mmcif"; >> >> > + reg = <0 0xee200000 0 0x100>; >> >> > + interrupt-parent = <&gic>; >> >> > + interrupts = <0 169 0x4>; >> >> > + reg-io-width = <4>; >> >> > + status = "disabled"; >> >> > + }; >> >> > + >> >> > + mmcif1: mmcif@ee220000 { >> >> > + compatible = "renesas,sh-mmcif"; >> >> > + reg = <0 0xee220000 0 0x100>; >> >> > + interrupt-parent = <&gic>; >> >> > + interrupts = <0 170 0x4>; >> >> > + reg-io-width = <4>; >> >> > + status = "disabled"; >> >> > + }; >> >> > + >> >> > + sdhi0: sdhi@ee100000 { >> >> > + compatible = "renesas,r8a7740-sdhi"; >> >> > + reg = <0 0xee100000 0 0x100>; >> >> > + interrupt-parent = <&gic>; >> >> > + interrupts = <0 165 4>; >> >> > + cap-sd-highspeed; >> >> > + status = "disabled"; >> >> > + }; >> >> >> >> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC. >> >> Sorry to say this but to me this seems like the first steps toward a >> >> future binary compatibility mess. >> >> >> >> Others seem to handle this differently. For instance, in >> >> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings >> >> for omap2 and omap3. >> >> >> >> As I stated before, I prefer to see using a compatible string based on >> >> the version of the IP block, or if we have to use the SoC name then >> >> use the name of the actual SoC used. >> >> >> >> What is the common way to deal with the compatible strings? Just use >> >> an existing one if it happens to match? Seems a lot like copy-paste >> >> integration to me with the added benefit of forever binary >> >> compatibility. >> >> >> >> Perhaps we have agreed on something already? Care to remind me? =) >> > >> > Indeed this has been discussed extensively: >> > >> > http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus=19248 >> >> Yes, I know that, but did we also come to a conclusion how to handle >> the compatible string in case we have several SoCs that happen to have >> similar compatible hardware block versions? >> >> It seems that the SDHI driver has one compatible string for sh7372, >> and when we remove sh7372 SoC support in the future it will be easy to >> remove that too. But how about r8a7740? If we have multiple different >> SoCs that use same SoC compatible string (r8a7740 in this case) then >> how can we age out these strings? >> >> My take on this that if you use the SoC name for the compatible string >> then you should also keep on updating the driver with this information >> so the DTS for the SoC can use the correct SoC name. > > A quote from the above link: > > "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." Thanks for the quote. I can now see clearly about shmoble5678 and shmobile1234. I hate to be difficult, but I disagree. =) Perhaps my logic is flawed, but if I understand the above then we are supposed to put DTS device nodes with compatible strings based on current driver behaviour? I see one problem with that. Say that a certain new hardware feature is included in one SoC, but the driver does not yet implement it. And perhaps the driver developer does not understand the difference fully when the device node is added to the DTS. Then won't we end up in a situation where people may roll out DTBs in products and that would not allow us to enable the feature later in the driver with a software update? Now, if we always used shmobile5678 for shmobile5678 then we would never have any problems. Of course, I prefer to use a version number for the hardware IP block instead of the SoC. This since this is not a SoC property. But if we now must be using SoC compatible strings then I think we should do it in a sane way that would reduce or risk of shooting ourself it the foot. As I pointed out earlier, the MMC drivers from other vendors seem to use compatible strings with names per-SoC or per-SoC-family. This even though they don't seem to handle any difference from a software perspective. So it seems to me that exactly how to handle this varies from place to place. Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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 Fri, 21 Jun 2013, Magnus Damm wrote: > Hi Guennadi, > > [Added Arnd to CC] > > On Fri, Jun 21, 2013 at 3:33 PM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > Hi Magnus > > > > On Fri, 21 Jun 2013, Magnus Damm wrote: > > > >> Hi Guennadi, > >> > >> On Fri, Jun 21, 2013 at 6:10 AM, Guennadi Liakhovetski > >> <g.liakhovetski@gmx.de> wrote: > >> > On Wed, 19 Jun 2013, Magnus Damm wrote: > >> > > >> >> Hi Guennadi, > >> >> > >> >> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski > >> >> <g.liakhovetski@gmx.de> wrote: > >> >> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled" > >> >> > state, those of them, available on APE6EVM, are then added to the board DT. > >> >> > This version assignes fixed regulators to all the interfaces, in future > >> >> > versions support for regulators should be added. > >> >> > > >> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> > >> >> > >> >> Thanks for your work on this. In general I'm happy to see your > >> >> progress but I wonder about the following: > >> >> > >> >> > --- a/arch/arm/boot/dts/r8a73a4.dtsi > >> >> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi > >> >> > @@ -98,4 +98,49 @@ > >> >> > gpio-controller; > >> >> > #gpio-cells = <2>; > >> >> > }; > >> >> > + > >> >> > + mmcif0: mmcif@ee200000 { > >> >> > + compatible = "renesas,sh-mmcif"; > >> >> > + reg = <0 0xee200000 0 0x100>; > >> >> > + interrupt-parent = <&gic>; > >> >> > + interrupts = <0 169 0x4>; > >> >> > + reg-io-width = <4>; > >> >> > + status = "disabled"; > >> >> > + }; > >> >> > + > >> >> > + mmcif1: mmcif@ee220000 { > >> >> > + compatible = "renesas,sh-mmcif"; > >> >> > + reg = <0 0xee220000 0 0x100>; > >> >> > + interrupt-parent = <&gic>; > >> >> > + interrupts = <0 170 0x4>; > >> >> > + reg-io-width = <4>; > >> >> > + status = "disabled"; > >> >> > + }; > >> >> > + > >> >> > + sdhi0: sdhi@ee100000 { > >> >> > + compatible = "renesas,r8a7740-sdhi"; > >> >> > + reg = <0 0xee100000 0 0x100>; > >> >> > + interrupt-parent = <&gic>; > >> >> > + interrupts = <0 165 4>; > >> >> > + cap-sd-highspeed; > >> >> > + status = "disabled"; > >> >> > + }; > >> >> > >> >> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC. > >> >> Sorry to say this but to me this seems like the first steps toward a > >> >> future binary compatibility mess. > >> >> > >> >> Others seem to handle this differently. For instance, in > >> >> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings > >> >> for omap2 and omap3. > >> >> > >> >> As I stated before, I prefer to see using a compatible string based on > >> >> the version of the IP block, or if we have to use the SoC name then > >> >> use the name of the actual SoC used. > >> >> > >> >> What is the common way to deal with the compatible strings? Just use > >> >> an existing one if it happens to match? Seems a lot like copy-paste > >> >> integration to me with the added benefit of forever binary > >> >> compatibility. > >> >> > >> >> Perhaps we have agreed on something already? Care to remind me? =) > >> > > >> > Indeed this has been discussed extensively: > >> > > >> > http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus=19248 > >> > >> Yes, I know that, but did we also come to a conclusion how to handle > >> the compatible string in case we have several SoCs that happen to have > >> similar compatible hardware block versions? > >> > >> It seems that the SDHI driver has one compatible string for sh7372, > >> and when we remove sh7372 SoC support in the future it will be easy to > >> remove that too. But how about r8a7740? If we have multiple different > >> SoCs that use same SoC compatible string (r8a7740 in this case) then > >> how can we age out these strings? > >> > >> My take on this that if you use the SoC name for the compatible string > >> then you should also keep on updating the driver with this information > >> so the DTS for the SoC can use the correct SoC name. > > > > A quote from the above link: > > > > "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." > > Thanks for the quote. I can now see clearly about shmoble5678 and shmobile1234. > > I hate to be difficult, but I disagree. =) > > Perhaps my logic is flawed, but if I understand the above then we are > supposed to put DTS device nodes with compatible strings based on > current driver behaviour? > > I see one problem with that. Say that a certain new hardware feature > is included in one SoC, but the driver does not yet implement it. And > perhaps the driver developer does not understand the difference fully > when the device node is added to the DTS. Then won't we end up in a > situation where people may roll out DTBs in products and that would > not allow us to enable the feature later in the driver with a software > update? Now, if we always used shmobile5678 for shmobile5678 then we > would never have any problems. My preference still lies with specific per-feature driver bindings like in my original implementation. Adding compatible strings to every driver for every new SoC seems an absolute no-go to me. But with per-feature driver bindings you would have exactly the same problem: if you later add support for a new feature to the driver, and that feature is present on older SoCs, the only way to enable it would be to replace the DT. Thanks Guennadi > Of course, I prefer to use a version number for the hardware IP block > instead of the SoC. This since this is not a SoC property. But if we > now must be using SoC compatible strings then I think we should do it > in a sane way that would reduce or risk of shooting ourself it the > foot. > > As I pointed out earlier, the MMC drivers from other vendors seem to > use compatible strings with names per-SoC or per-SoC-family. This even > though they don't seem to handle any difference from a software > perspective. > > So it seems to me that exactly how to handle this varies from place to place. > > Cheers, > > / magnus > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi and Magnus, On Friday 21 June 2013 09:48:43 Guennadi Liakhovetski wrote: > On Fri, 21 Jun 2013, Magnus Damm wrote: > > On Fri, Jun 21, 2013 at 3:33 PM, Guennadi Liakhovetski wrote: > > > On Fri, 21 Jun 2013, Magnus Damm wrote: > > >> On Fri, Jun 21, 2013 at 6:10 AM, Guennadi Liakhovetski wrote: > > >> > On Wed, 19 Jun 2013, Magnus Damm wrote: > > >> >> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski wrote: > > >> >> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the > > >> >> > "disabled" state, those of them, available on APE6EVM, are then > > >> >> > added to the board DT. This version assignes fixed regulators to > > >> >> > all the interfaces, in future versions support for regulators > > >> >> > should be added. > > >> >> > > > >> >> > Signed-off-by: Guennadi Liakhovetski > > >> >> > <g.liakhovetski+renesas@gmail.com> > > >> >> > > >> >> Thanks for your work on this. In general I'm happy to see your > > >> >> > > >> >> progress but I wonder about the following: > > >> >> > --- a/arch/arm/boot/dts/r8a73a4.dtsi > > >> >> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi > > >> >> > @@ -98,4 +98,49 @@ > > >> >> > > > >> >> > gpio-controller; > > >> >> > #gpio-cells = <2>; > > >> >> > > > >> >> > }; > > >> >> > > > >> >> > + > > >> >> > + mmcif0: mmcif@ee200000 { > > >> >> > + compatible = "renesas,sh-mmcif"; > > >> >> > + reg = <0 0xee200000 0 0x100>; > > >> >> > + interrupt-parent = <&gic>; > > >> >> > + interrupts = <0 169 0x4>; > > >> >> > + reg-io-width = <4>; > > >> >> > + status = "disabled"; > > >> >> > + }; > > >> >> > + > > >> >> > + mmcif1: mmcif@ee220000 { > > >> >> > + compatible = "renesas,sh-mmcif"; > > >> >> > + reg = <0 0xee220000 0 0x100>; > > >> >> > + interrupt-parent = <&gic>; > > >> >> > + interrupts = <0 170 0x4>; > > >> >> > + reg-io-width = <4>; > > >> >> > + status = "disabled"; > > >> >> > + }; > > >> >> > + > > >> >> > + sdhi0: sdhi@ee100000 { > > >> >> > + compatible = "renesas,r8a7740-sdhi"; > > >> >> > + reg = <0 0xee100000 0 0x100>; > > >> >> > + interrupt-parent = <&gic>; > > >> >> > + interrupts = <0 165 4>; > > >> >> > + cap-sd-highspeed; > > >> >> > + status = "disabled"; > > >> >> > + }; > > >> >> > > >> >> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 > > >> >> SoC. Sorry to say this but to me this seems like the first steps > > >> >> toward a future binary compatibility mess. > > >> >> > > >> >> Others seem to handle this differently. For instance, in > > >> >> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings > > >> >> for omap2 and omap3. > > >> >> > > >> >> As I stated before, I prefer to see using a compatible string based > > >> >> on the version of the IP block, or if we have to use the SoC name > > >> >> then use the name of the actual SoC used. > > >> >> > > >> >> What is the common way to deal with the compatible strings? Just use > > >> >> an existing one if it happens to match? Seems a lot like copy-paste > > >> >> integration to me with the added benefit of forever binary > > >> >> compatibility. > > >> >> > > >> >> Perhaps we have agreed on something already? Care to remind me? =) > > >> > > > >> > Indeed this has been discussed extensively: > > >> > > > >> > http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus=19248 > > >> > > >> Yes, I know that, but did we also come to a conclusion how to handle > > >> the compatible string in case we have several SoCs that happen to have > > >> similar compatible hardware block versions? > > >> > > >> It seems that the SDHI driver has one compatible string for sh7372, > > >> and when we remove sh7372 SoC support in the future it will be easy to > > >> remove that too. But how about r8a7740? If we have multiple different > > >> SoCs that use same SoC compatible string (r8a7740 in this case) then > > >> how can we age out these strings? > > >> > > >> My take on this that if you use the SoC name for the compatible string > > >> then you should also keep on updating the driver with this information > > >> so the DTS for the SoC can use the correct SoC name. > > > > > > A quote from the above link: > > > > > > "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." > > > > Thanks for the quote. I can now see clearly about shmoble5678 and > > shmobile1234. > > > > I hate to be difficult, but I disagree. =) > > > > Perhaps my logic is flawed, but if I understand the above then we are > > supposed to put DTS device nodes with compatible strings based on > > current driver behaviour? > > > > I see one problem with that. Say that a certain new hardware feature > > is included in one SoC, but the driver does not yet implement it. And > > perhaps the driver developer does not understand the difference fully > > when the device node is added to the DTS. Then won't we end up in a > > situation where people may roll out DTBs in products and that would > > not allow us to enable the feature later in the driver with a software > > update? Now, if we always used shmobile5678 for shmobile5678 then we > > would never have any problems. > > My preference still lies with specific per-feature driver bindings like in > my original implementation. Adding compatible strings to every driver for > every new SoC seems an absolute no-go to me. Isn't it an existing practice used by many DT bindings in the upstream kernel ? > But with per-feature driver bindings you would have exactly the same > problem: if you later add support for a new feature to the driver, and that > feature is present on older SoCs, the only way to enable it would be to > replace the DT. > > > Of course, I prefer to use a version number for the hardware IP block > > instead of the SoC. Let's start with the obvious. Could we get those version numbers ? > > This since this is not a SoC property. Probably not directly applicable to the MMCIF and SDHI, when it comes to how IP are integrated in an SoC some of the IP-specific properties are actually SoC-specific. For instance on R9A7790 the Video Signal Processor can output directly to the Display Unit, the way this is configured on both IP cores depend not only on the IP version, but is specific to the R8A7790. I will thus use SoC-specific compatible strings for those drivers. > > But if we now must be using SoC compatible strings then I think we should > > do it in a sane way that would reduce or risk of shooting ourself it the > > foot. > > > > As I pointed out earlier, the MMC drivers from other vendors seem to > > use compatible strings with names per-SoC or per-SoC-family. This even > > though they don't seem to handle any difference from a software > > perspective. > > > > So it seems to me that exactly how to handle this varies from place to > > place.
On Friday 21 June 2013, Magnus Damm wrote: > > "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." > > Thanks for the quote. I can now see clearly about shmoble5678 and shmobile1234. > > I hate to be difficult, but I disagree. =) > > Perhaps my logic is flawed, but if I understand the above then we are > supposed to put DTS device nodes with compatible strings based on > current driver behaviour? > > I see one problem with that. Say that a certain new hardware feature > is included in one SoC, but the driver does not yet implement it. And > perhaps the driver developer does not understand the difference fully > when the device node is added to the DTS. Then won't we end up in a > situation where people may roll out DTBs in products and that would > not allow us to enable the feature later in the driver with a software > update? Now, if we always used shmobile5678 for shmobile5678 then we > would never have any problems. I said that you should use compatible="reneasas,shmobile1234-sdhi" if it is the /same/ block as the one used in 5678. If the new one is actually backwards compatible but has additional features (or may have them but you are not sure), then I would always list both, so 'compatible="reneasas,shmobile5678-sdhi", "reneasas,shmobile1234-sdhi", "renesas,shmobile-sdhi";'. You always start with the most specific model string (and in doubt you can be very specific here, up to a minor stepping release) and list the more generic strings to the end. This way you can have both: the driver only needs to match shmobile1234 if it doesn't use any of the newer features, but can check for shmobile5678 if it wants to. For chips that you don't already know about, it is not a big thing anyway, you can always use the most recent SoC string in the device tree and put that into the driver. However, we want to ideally support future SoCs with an unmodified driver, and that can only work if the future part claims compatibility with an older one. Using the same logic of putting the older 'compatible' strings into the DT for compatible parts at the time of the initial binding is not a technical necessity but is done for consistency with the way we have to do it later. > Of course, I prefer to use a version number for the hardware IP block > instead of the SoC. This since this is not a SoC property. But if we > now must be using SoC compatible strings then I think we should do it > in a sane way that would reduce or risk of shooting ourself it the > foot. Not sure I get this. Do you have to use the SoC identifier because you don't have access to the hardware IP block version, or because someone told you to? Using the hardware IP block version is defintely preferred in my opinion. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 21 June 2013, Guennadi Liakhovetski wrote: > My preference still lies with specific per-feature driver bindings like in > my original implementation. Adding compatible strings to every driver for > every new SoC seems an absolute no-go to me. But with per-feature driver > bindings you would have exactly the same problem: if you later add support > for a new feature to the driver, and that feature is present on older > SoCs, the only way to enable it would be to replace the DT. That is the main problem with the per-feature listing, but less of a problem if you list both the original part and the specific name of the new one in the compatible string. It's also not an either-or thing. For a number of features, having flags makes most sense, in particular when the feature is not a property of the IP block but of the way it is connected to the rest of the SoC. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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/arch/arm/boot/dts/r8a73a4-ape6evm.dts b/arch/arm/boot/dts/r8a73a4-ape6evm.dts index b5b24eb..63eaf5a 100644 --- a/arch/arm/boot/dts/r8a73a4-ape6evm.dts +++ b/arch/arm/boot/dts/r8a73a4-ape6evm.dts @@ -53,6 +53,37 @@ }; }; +&mmcif0 { + pinctrl-0 = <&mmcif0_pins>; + pinctrl-names = "default"; + + vmmc-supply = <&ape6evm_fixed_3v3>; + bus-width = <8>; + non-removable; + status = "okay"; +}; + +&sdhi0 { + pinctrl-0 = <&sdhi0_pins>; + pinctrl-names = "default"; + + vmmc-supply = <&ape6evm_fixed_3v3>; + bus-width = <4>; + toshiba,mmc-wrprotect-disable; + status = "okay"; +}; + +&sdhi1 { + pinctrl-0 = <&sdhi1_pins>; + pinctrl-names = "default"; + + vmmc-supply = <&ape6evm_fixed_3v3>; + bus-width = <4>; + broken-cd; + toshiba,mmc-wrprotect-disable; + status = "okay"; +}; + &pfc { pinctrl-0 = <&scifa0_pins>; pinctrl-names = "default"; @@ -61,4 +92,19 @@ renesas,groups = "scifa0_data"; renesas,function = "scifa0"; }; + + mmcif0_pins: mmcif0 { + renesas,groups = "mmc0_data8", "mmc0_ctrl"; + renesas,function = "mmc0"; + }; + + sdhi0_pins: sdhi0 { + renesas,groups = "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd", "sdhi0_wp"; + renesas,function = "sdhi0"; + }; + + sdhi1_pins: sdhi1 { + renesas,groups = "sdhi1_data4", "sdhi1_ctrl"; + renesas,function = "sdhi1"; + }; }; diff --git a/arch/arm/boot/dts/r8a73a4.dtsi b/arch/arm/boot/dts/r8a73a4.dtsi index 6e73367..33c9bac 100644 --- a/arch/arm/boot/dts/r8a73a4.dtsi +++ b/arch/arm/boot/dts/r8a73a4.dtsi @@ -98,4 +98,49 @@ gpio-controller; #gpio-cells = <2>; }; + + mmcif0: mmcif@ee200000 { + compatible = "renesas,sh-mmcif"; + reg = <0 0xee200000 0 0x100>; + interrupt-parent = <&gic>; + interrupts = <0 169 0x4>; + reg-io-width = <4>; + status = "disabled"; + }; + + mmcif1: mmcif@ee220000 { + compatible = "renesas,sh-mmcif"; + reg = <0 0xee220000 0 0x100>; + interrupt-parent = <&gic>; + interrupts = <0 170 0x4>; + reg-io-width = <4>; + status = "disabled"; + }; + + sdhi0: sdhi@ee100000 { + compatible = "renesas,r8a7740-sdhi"; + reg = <0 0xee100000 0 0x100>; + interrupt-parent = <&gic>; + interrupts = <0 165 4>; + cap-sd-highspeed; + status = "disabled"; + }; + + sdhi1: sdhi@ee120000 { + compatible = "renesas,r8a7740-sdhi"; + reg = <0 0xee120000 0 0x100>; + interrupt-parent = <&gic>; + interrupts = <0 166 4>; + cap-sd-highspeed; + status = "disabled"; + }; + + sdhi2: sdhi@ee140000 { + compatible = "renesas,r8a7740-sdhi"; + reg = <0 0xee140000 0 0x100>; + interrupt-parent = <&gic>; + interrupts = <0 167 4>; + cap-sd-highspeed; + status = "disabled"; + }; };
This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled" state, those of them, available on APE6EVM, are then added to the board DT. This version assignes fixed regulators to all the interfaces, in future versions support for regulators should be added. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> --- arch/arm/boot/dts/r8a73a4-ape6evm.dts | 46 +++++++++++++++++++++++++++++++++ arch/arm/boot/dts/r8a73a4.dtsi | 45 ++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 0 deletions(-)