| Message ID | 1392053002-19831-7-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 10, 2014 at 06:23:17PM +0100, Thomas Petazzoni wrote: > This commit adds support for the Armada 380/385 SoCs in the > system-controller driver. Since this SoC has the same system > controller registers layout than the Armada 370/XP at least for the > few features currently supported by the driver, this commit simply > adds a new compatible string that provides the same behavior than the > one provided for Armada 370/XP. > > Note that we intentionally do not use the same compatible string as > Armada 370/XP, as the current system-controller driver is far from > exploiting all the possibilities of the hardware, and we may in the > future discover differences between Armada 370/XP and Armada 380/385. I'd much prefer to add a new compatible string iff it accompanies incompatible changes. For now, I think it's best to use 'marvell,armada-370-xp-system-controller' in the dtsi file and change it when you introduce the incompatible features. thx, Jason. > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > --- > Documentation/devicetree/bindings/arm/mvebu-system-controller.txt | 3 ++- > arch/arm/mach-mvebu/system-controller.c | 8 ++++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt b/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt > index d24ab2e..3559972 100644 > --- a/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt > +++ b/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt > @@ -1,6 +1,6 @@ > MVEBU System Controller > ----------------------- > -MVEBU (Marvell SOCs: Armada 370/375/XP, Dove, mv78xx0, Kirkwood, Orion5x) > +MVEBU (Marvell SOCs: Armada 370/375/38x/XP, Dove, mv78xx0, Kirkwood, Orion5x) > > Required properties: > > @@ -8,6 +8,7 @@ Required properties: > - "marvell,orion-system-controller" > - "marvell,armada-370-xp-system-controller" > - "marvell,armada-375-system-controller" > + - "marvell,armada-380-system-controller" > - reg: Should contain system controller registers location and length. > > Example: > diff --git a/arch/arm/mach-mvebu/system-controller.c b/arch/arm/mach-mvebu/system-controller.c > index 1806187..b4e8bb2 100644 > --- a/arch/arm/mach-mvebu/system-controller.c > +++ b/arch/arm/mach-mvebu/system-controller.c > @@ -71,6 +71,14 @@ static struct of_device_id of_system_controller_table[] = { > }, { > .compatible = "marvell,armada-375-system-controller", > .data = (void *) &armada_375_system_controller, > + }, { > + /* > + * As far as RSTOUTn and System soft reset registers > + * are concerned, Armada 38x is similar to Armada > + * 370/XP > + */ > + .compatible = "marvell,armada-380-system-controller", > + .data = (void *) &armada_370_xp_system_controller, > }, > { /* end of list */ }, > }; > -- > 1.8.3.2 >
On Mon, Feb 10, 2014 at 06:47:26PM +0100, Thomas Petazzoni wrote: > Dear Jason Cooper, > > On Mon, 10 Feb 2014 12:39:57 -0500, Jason Cooper wrote: > > > > Note that we intentionally do not use the same compatible string as > > > Armada 370/XP, as the current system-controller driver is far from > > > exploiting all the possibilities of the hardware, and we may in the > > > future discover differences between Armada 370/XP and Armada 380/385. > > > > I'd much prefer to add a new compatible string iff it accompanies > > incompatible changes. > > > > For now, I think it's best to use 'marvell,armada-370-xp-system-controller' > > in the dtsi file and change it when you introduce the incompatible > > features. > > This doesn't work really well: if an user keeps an old DTB around, > which uses the old compatible string, then you're screwed because > there's no way a new kernel version can make the distinction between > Armada 370/XP and Armada 380/385. devicetree is designed to handle this. The dtb is _supposed_ to be separate from the kernel. > If we discover than Armada 380/385 need a special quirk to really work > reliably for example, but that this quirk doesn't apply to Armada > 370/XP, then you have a serious problem. No, you don't. It's the responsibility of the driver author to _not_ make incompatible changes and to sanely fall back to default behavior. Right now there are no differences, and both SoCs work with the same compatible string. If later there is support added for a feature for one of them, we add a compatible string to differentiate the two. When the driver encounters a potentially old dtb, it defaults to behaving as it does today. iow, no new feature. In this situation, the driver or SoC code could see the root compatible node is marvell,armada385 with a sys-con node using marvell,armada-370-xp-sys-con. It could then print a warning about potentially old dtb. Depending on the severity of the bug (which couldn't be too severe, otherwise you'd have code here today ;) ), you could even override the compatible string in the SoC code. > Therefore, I would like to really insist to have separate compatible > strings for different SOCs. As an example, we used to have the same > compatible string for the timer between Armada 370 and Armada XP, and > later discovered that it was not possible due to a bug affecting only > one of the two SOCs. Our experience clearly shows that sharing > compatible strings is a bad idea, and having separate compatible > strings from the beginning doesn't cost anything, and offers higher > flexibility for the future. Your argument is tempting, but I see a lot of over-quantization with little to no measurable gain other than "just in case." If I follow your logic and refer to the i2c transaction generator problem, should we go back and add compatible strings for all on-die IP blocks for every SoC and SoC revision? Even though we only wish we had something once? Can we anticipate every possible way an IP block will be broken? Ultimately, what I'm saying is that the devicetree process should be able to handle this kind of situation. And if it can't, the process is broken. No amount of bubblewrap and safety helmets will save us then. Now, wrt marvell,armada-38x-system-controller, if you can point out specific features you know about *today* that are different from the 370/xp sys-con, please spell them out at least in the commit log. I'm fine with just mentioning the features at this point, since we're dealing with initial SoC support. But "...we may in the future discover differences between..." doesn't inspire confidence. thx, Jason.
Dear Jason Cooper, On Mon, 10 Feb 2014 13:48:57 -0500, Jason Cooper wrote: > > This doesn't work really well: if an user keeps an old DTB around, > > which uses the old compatible string, then you're screwed because > > there's no way a new kernel version can make the distinction between > > Armada 370/XP and Armada 380/385. > > devicetree is designed to handle this. The dtb is _supposed_ to be > separate from the kernel. Exactly. Which is the whole point why your strategy doesn't work. Since the DTB is supposed to be separate from the kernel, we have no guarantee that the user will update the DTB when updating the kernel. Therefore, we have to keep compatibility for old DTBs, which forces us to be extra cautious when choosing compatible strings. > > If we discover than Armada 380/385 need a special quirk to really work > > reliably for example, but that this quirk doesn't apply to Armada > > 370/XP, then you have a serious problem. > > No, you don't. It's the responsibility of the driver author to _not_ > make incompatible changes and to sanely fall back to default behavior. This is not always possible. If we discover that the 38x system controller has a bug that needs to be worked around in a way that isn't suitable for Armada 370/XP, then there is *no* way for the driver author to keep backward compatibility. > Right now there are no differences, and both SoCs work with the same > compatible string. If later there is support added for a feature for > one of them, we add a compatible string to differentiate the two. When > the driver encounters a potentially old dtb, it defaults to behaving as > it does today. iow, no new feature. This only works if you *add* new features. Not if you have to get back to existing features and make changes that have to be different between the SOCs to fix bugs for existing features. > In this situation, the driver or SoC code could see the root compatible > node is marvell,armada385 with a sys-con node using > marvell,armada-370-xp-sys-con. It could then print a warning about > potentially old dtb. Depending on the severity of the bug (which > couldn't be too severe, otherwise you'd have code here today ;) ), you > could even override the compatible string in the SoC code. This seems overly complicated compared to the simple usage of different compatible strings per-SoC. At least, I'd like your policy to be baked by statements from the DT maintainers because I am not sure this policy is universally accepted. > > Therefore, I would like to really insist to have separate compatible > > strings for different SOCs. As an example, we used to have the same > > compatible string for the timer between Armada 370 and Armada XP, and > > later discovered that it was not possible due to a bug affecting only > > one of the two SOCs. Our experience clearly shows that sharing > > compatible strings is a bad idea, and having separate compatible > > strings from the beginning doesn't cost anything, and offers higher > > flexibility for the future. > > Your argument is tempting, but I see a lot of over-quantization with > little to no measurable gain other than "just in case." Did you see the very real example of "just in case" I described in the paragraph just above, with the Armada 370/XP timer? This "just in case" lead us to break compatibility with old DTs, and have a broken kernel until -rc4 or -rc5, due to issues in merging the necessary DT changes and driver changes. To me, your policy is simply ignoring the experience. > If I follow your logic and refer to the i2c transaction generator > problem, should we go back and add compatible strings for all on-die IP > blocks for every SoC and SoC revision? Even though we only wish we had > something once? Can we anticipate every possible way an IP block will > be broken? Hard to anticipate all the cases, but when we can, we should do it. Armada 370/XP and Armada 38x are *very* different SOCs. They don't use the same CPU core, the same cache controller, the same interrupt controller, a large number of peripherals have changed. The likely-hood of discovering differences is fairly high. > Ultimately, what I'm saying is that the devicetree process should be > able to handle this kind of situation. And if it can't, the process is > broken. No amount of bubblewrap and safety helmets will save us then. > > Now, wrt marvell,armada-38x-system-controller, if you can point out > specific features you know about *today* that are different from the > 370/xp sys-con, please spell them out at least in the commit log. I'm > fine with just mentioning the features at this point, since we're > dealing with initial SoC support. But "...we may in the future discover > differences between..." doesn't inspire confidence. Have you ever had a look at a very early datasheet? It seems like not. The datasheet details at this stage of the SoC development are very limited as the datasheet is still being written. Remember that we are talking about a SoC that has not even been announced publicly on the SoC vendor web site. We had to get back to Marvell many, many times to have enough details to understand various aspects of the SoC. So yes, we may very well discover differences in the future, simply because we have no way, today, to have a clear view of all aspects of the SoC. Best regards, Thomas
On Mon, Feb 10, 2014 at 5:39 PM, Jason Cooper <jason@lakedaemon.net> wrote: > On Mon, Feb 10, 2014 at 06:23:17PM +0100, Thomas Petazzoni wrote: >> This commit adds support for the Armada 380/385 SoCs in the >> system-controller driver. Since this SoC has the same system >> controller registers layout than the Armada 370/XP at least for the >> few features currently supported by the driver, this commit simply >> adds a new compatible string that provides the same behavior than the >> one provided for Armada 370/XP. >> >> Note that we intentionally do not use the same compatible string as >> Armada 370/XP, as the current system-controller driver is far from >> exploiting all the possibilities of the hardware, and we may in the >> future discover differences between Armada 370/XP and Armada 380/385. > > I'd much prefer to add a new compatible string iff it accompanies > incompatible changes. > > For now, I think it's best to use 'marvell,armada-370-xp-system-controller' > in the dtsi file and change it when you introduce the incompatible > features. Actually, they are doing the right thing here. It is completely appropriate to encode the SoC version into the compatible string, but then to retain compatibility with the older device driver by including the earlier compatible string in the compatible list so that no code changes are required. g. > > thx, > > Jason. > >> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> >> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >> --- >> Documentation/devicetree/bindings/arm/mvebu-system-controller.txt | 3 ++- >> arch/arm/mach-mvebu/system-controller.c | 8 ++++++++ >> 2 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt b/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt >> index d24ab2e..3559972 100644 >> --- a/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt >> +++ b/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt >> @@ -1,6 +1,6 @@ >> MVEBU System Controller >> ----------------------- >> -MVEBU (Marvell SOCs: Armada 370/375/XP, Dove, mv78xx0, Kirkwood, Orion5x) >> +MVEBU (Marvell SOCs: Armada 370/375/38x/XP, Dove, mv78xx0, Kirkwood, Orion5x) >> >> Required properties: >> >> @@ -8,6 +8,7 @@ Required properties: >> - "marvell,orion-system-controller" >> - "marvell,armada-370-xp-system-controller" >> - "marvell,armada-375-system-controller" >> + - "marvell,armada-380-system-controller" >> - reg: Should contain system controller registers location and length. >> >> Example: >> diff --git a/arch/arm/mach-mvebu/system-controller.c b/arch/arm/mach-mvebu/system-controller.c >> index 1806187..b4e8bb2 100644 >> --- a/arch/arm/mach-mvebu/system-controller.c >> +++ b/arch/arm/mach-mvebu/system-controller.c >> @@ -71,6 +71,14 @@ static struct of_device_id of_system_controller_table[] = { >> }, { >> .compatible = "marvell,armada-375-system-controller", >> .data = (void *) &armada_375_system_controller, >> + }, { >> + /* >> + * As far as RSTOUTn and System soft reset registers >> + * are concerned, Armada 38x is similar to Armada >> + * 370/XP >> + */ >> + .compatible = "marvell,armada-380-system-controller", >> + .data = (void *) &armada_370_xp_system_controller, However, this would not be the right thing to do. The better solution is to put the following into the .dts file: compatible = "marvell,armada-380-system-controller","marvell,armada-370-system-controller" So that the kernel has the option to override the 370 version if an errata or extra feature support ever needs to be added. g.
Grant, On Tue, 11 Feb 2014 14:22:04 +0000, Grant Likely wrote: > >> + }, { > >> + /* > >> + * As far as RSTOUTn and System soft reset registers > >> + * are concerned, Armada 38x is similar to Armada > >> + * 370/XP > >> + */ > >> + .compatible = "marvell,armada-380-system-controller", > >> + .data = (void *) &armada_370_xp_system_controller, > > However, this would not be the right thing to do. The better solution > is to put the following into the .dts file: > > compatible = "marvell,armada-380-system-controller","marvell,armada-370-system-controller" > > So that the kernel has the option to override the 370 version if an > errata or extra feature support ever needs to be added. Ah, right true. Makes sense. We are going to update our patch set to take this suggestion into account, and resubmit a v2 with this. This way, we don't need new to introduce in drivers new compatible strings for the clocksource driver, the mbus driver and the system-controller driver. Thanks Grant for your suggestion. Jason, are you ok with this? Best regards, Thomas
On Tue, Feb 11, 2014 at 04:24:36PM +0100, Thomas Petazzoni wrote: > Grant, > > On Tue, 11 Feb 2014 14:22:04 +0000, Grant Likely wrote: > > > >> + }, { > > >> + /* > > >> + * As far as RSTOUTn and System soft reset registers > > >> + * are concerned, Armada 38x is similar to Armada > > >> + * 370/XP > > >> + */ > > >> + .compatible = "marvell,armada-380-system-controller", > > >> + .data = (void *) &armada_370_xp_system_controller, > > > > However, this would not be the right thing to do. The better solution > > is to put the following into the .dts file: > > > > compatible = "marvell,armada-380-system-controller","marvell,armada-370-system-controller" > > > > So that the kernel has the option to override the 370 version if an > > errata or extra feature support ever needs to be added. > > Ah, right true. Makes sense. We are going to update our patch set to > take this suggestion into account, and resubmit a v2 with this. This > way, we don't need new to introduce in drivers new compatible strings > for the clocksource driver, the mbus driver and the system-controller > driver. > > Thanks Grant for your suggestion. > > Jason, are you ok with this? Yes. Thanks Grant for clearing this up. And thanks guys for tolerating my "This doesn't look right, but I'm having trouble explaining how" moment :) thx, Jason.
On 11/02/2014 16:30, Jason Cooper wrote: > On Tue, Feb 11, 2014 at 04:24:36PM +0100, Thomas Petazzoni wrote: >> Grant, >> >> On Tue, 11 Feb 2014 14:22:04 +0000, Grant Likely wrote: >> >>>>> + }, { >>>>> + /* >>>>> + * As far as RSTOUTn and System soft reset registers >>>>> + * are concerned, Armada 38x is similar to Armada >>>>> + * 370/XP >>>>> + */ >>>>> + .compatible = "marvell,armada-380-system-controller", >>>>> + .data = (void *) &armada_370_xp_system_controller, >>> >>> However, this would not be the right thing to do. The better solution >>> is to put the following into the .dts file: >>> >>> compatible = "marvell,armada-380-system-controller","marvell,armada-370-system-controller" >>> >>> So that the kernel has the option to override the 370 version if an >>> errata or extra feature support ever needs to be added. >> >> Ah, right true. Makes sense. We are going to update our patch set to >> take this suggestion into account, and resubmit a v2 with this. This >> way, we don't need new to introduce in drivers new compatible strings >> for the clocksource driver, the mbus driver and the system-controller >> driver. >> >> Thanks Grant for your suggestion. >> >> Jason, are you ok with this? > > Yes. Thanks Grant for clearing this up. And thanks guys for tolerating > my "This doesn't look right, but I'm having trouble explaining how" > moment :) Great, with this solution we have the better of the two options: the ability to deal with different behaviors of the IP without having to update the dts and no code added in the drivers files. > > thx, > > Jason. >
diff --git a/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt b/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt index d24ab2e..3559972 100644 --- a/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt +++ b/Documentation/devicetree/bindings/arm/mvebu-system-controller.txt @@ -1,6 +1,6 @@ MVEBU System Controller ----------------------- -MVEBU (Marvell SOCs: Armada 370/375/XP, Dove, mv78xx0, Kirkwood, Orion5x) +MVEBU (Marvell SOCs: Armada 370/375/38x/XP, Dove, mv78xx0, Kirkwood, Orion5x) Required properties: @@ -8,6 +8,7 @@ Required properties: - "marvell,orion-system-controller" - "marvell,armada-370-xp-system-controller" - "marvell,armada-375-system-controller" + - "marvell,armada-380-system-controller" - reg: Should contain system controller registers location and length. Example: diff --git a/arch/arm/mach-mvebu/system-controller.c b/arch/arm/mach-mvebu/system-controller.c index 1806187..b4e8bb2 100644 --- a/arch/arm/mach-mvebu/system-controller.c +++ b/arch/arm/mach-mvebu/system-controller.c @@ -71,6 +71,14 @@ static struct of_device_id of_system_controller_table[] = { }, { .compatible = "marvell,armada-375-system-controller", .data = (void *) &armada_375_system_controller, + }, { + /* + * As far as RSTOUTn and System soft reset registers + * are concerned, Armada 38x is similar to Armada + * 370/XP + */ + .compatible = "marvell,armada-380-system-controller", + .data = (void *) &armada_370_xp_system_controller, }, { /* end of list */ }, };