| Message ID | 20220126222034.16889-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [V2] dt-bindings: watchdog: brcm,bcm7038: add more compatible strings | expand |
On Wed, Jan 26, 2022 at 11:20:34PM +0100, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This hardware block is used on almost all BCM63xx family chipsets and > BCM4908 which reuses a lot of BCM63xx parts. Add relevant compatible > strings and also include a generic one. > > The only SoC with a different block I found is BCM6838 (thus not included > in this change). > > It may be worth noting that BCM6338, BCM6345, BCM6348 and BCM63268 don't > include "SoftRst" register but that can be handled by drivers based on > precise compatible string. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > V2: Sort enum entries & update brcm,twd.yaml > --- > .../devicetree/bindings/mfd/brcm,twd.yaml | 2 +- > .../bindings/watchdog/brcm,bcm7038-wdt.yaml | 21 +++++++++++++++---- > 2 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml > index 634526f790b8..3f5db1990aba 100644 > --- a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml > +++ b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml > @@ -55,7 +55,7 @@ examples: > #size-cells = <1>; > > watchdog@28 { > - compatible = "brcm,bcm7038-wdt"; > + compatible = "brcm,bcm4908-wdt", "brcm,bcm63xx-wdt"; > reg = <0x28 0x8>; > }; > }; > diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml > index a926809352b8..4d848442913c 100644 > --- a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml > +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml > @@ -16,9 +16,22 @@ maintainers: > > properties: > compatible: > - enum: > - - brcm,bcm6345-wdt > - - brcm,bcm7038-wdt > + items: > + - enum: > + - brcm,bcm4908-wdt > + - brcm,bcm6338-wdt > + - brcm,bcm6345-wdt > + - brcm,bcm6348-wdt > + - brcm,bcm6848-wdt > + - brcm,bcm6858-wdt > + - brcm,bcm7038-wdt > + - brcm,bcm60333-wdt > + - brcm,bcm63138-wdt > + - brcm,bcm63148-wdt > + - brcm,bcm63268-wdt > + - brcm,bcm63381-wdt > + - brcm,bcm68360-wdt > + - const: brcm,bcm63xx-wdt Is it really worthwhile to update all these DTs?: arch/mips/boot/dts/brcm/bcm63268.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm6328.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm6358.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm6362.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm6368.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm7125.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm7346.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm7358.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm7360.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm7362.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm7420.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm7425.dtsi: compatible = "brcm,bcm7038-wdt"; arch/mips/boot/dts/brcm/bcm7435.dtsi: compatible = "brcm,bcm7038-wdt"; I don't think so. Rob
On 9.02.2022 20:09, Rob Herring wrote: > On Wed, Jan 26, 2022 at 11:20:34PM +0100, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> This hardware block is used on almost all BCM63xx family chipsets and >> BCM4908 which reuses a lot of BCM63xx parts. Add relevant compatible >> strings and also include a generic one. >> >> The only SoC with a different block I found is BCM6838 (thus not included >> in this change). >> >> It may be worth noting that BCM6338, BCM6345, BCM6348 and BCM63268 don't >> include "SoftRst" register but that can be handled by drivers based on >> precise compatible string. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> V2: Sort enum entries & update brcm,twd.yaml >> --- >> .../devicetree/bindings/mfd/brcm,twd.yaml | 2 +- >> .../bindings/watchdog/brcm,bcm7038-wdt.yaml | 21 +++++++++++++++---- >> 2 files changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml >> index 634526f790b8..3f5db1990aba 100644 >> --- a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml >> +++ b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml >> @@ -55,7 +55,7 @@ examples: >> #size-cells = <1>; >> >> watchdog@28 { >> - compatible = "brcm,bcm7038-wdt"; >> + compatible = "brcm,bcm4908-wdt", "brcm,bcm63xx-wdt"; >> reg = <0x28 0x8>; >> }; >> }; >> diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml >> index a926809352b8..4d848442913c 100644 >> --- a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml >> +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml >> @@ -16,9 +16,22 @@ maintainers: >> >> properties: >> compatible: >> - enum: >> - - brcm,bcm6345-wdt >> - - brcm,bcm7038-wdt >> + items: >> + - enum: >> + - brcm,bcm4908-wdt >> + - brcm,bcm6338-wdt >> + - brcm,bcm6345-wdt >> + - brcm,bcm6348-wdt >> + - brcm,bcm6848-wdt >> + - brcm,bcm6858-wdt >> + - brcm,bcm7038-wdt >> + - brcm,bcm60333-wdt >> + - brcm,bcm63138-wdt >> + - brcm,bcm63148-wdt >> + - brcm,bcm63268-wdt >> + - brcm,bcm63381-wdt >> + - brcm,bcm68360-wdt >> + - const: brcm,bcm63xx-wdt > > Is it really worthwhile to update all these DTs?: > > arch/mips/boot/dts/brcm/bcm63268.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm6328.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm6358.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm6362.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm6368.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm7125.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm7346.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm7358.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm7360.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm7362.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm7420.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm7425.dtsi: compatible = "brcm,bcm7038-wdt"; > arch/mips/boot/dts/brcm/bcm7435.dtsi: compatible = "brcm,bcm7038-wdt"; I don't have problem handling that. > I don't think so. So what's the policy for such bindings then? How to select SoCs that should have their own bindings? How can I tell which should /borrow/ a binding instead? I assumed we want binding per SoC based on the 17fffe91ba36 ("dt-bindings: watchdog: Add BCM6345 compatible to BCM7038 binding") https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=17fffe91ba36d11c7b17be154ecc7c1ed31527eb
On Wed, Feb 09, 2022 at 08:26:00PM +0100, Rafał Miłecki wrote: > On 9.02.2022 20:09, Rob Herring wrote: > > On Wed, Jan 26, 2022 at 11:20:34PM +0100, Rafał Miłecki wrote: > > > From: Rafał Miłecki <rafal@milecki.pl> > > > > > > This hardware block is used on almost all BCM63xx family chipsets and > > > BCM4908 which reuses a lot of BCM63xx parts. Add relevant compatible > > > strings and also include a generic one. > > > > > > The only SoC with a different block I found is BCM6838 (thus not included > > > in this change). > > > > > > It may be worth noting that BCM6338, BCM6345, BCM6348 and BCM63268 don't > > > include "SoftRst" register but that can be handled by drivers based on > > > precise compatible string. > > > > > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > > --- > > > V2: Sort enum entries & update brcm,twd.yaml > > > --- > > > .../devicetree/bindings/mfd/brcm,twd.yaml | 2 +- > > > .../bindings/watchdog/brcm,bcm7038-wdt.yaml | 21 +++++++++++++++---- > > > 2 files changed, 18 insertions(+), 5 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml > > > index 634526f790b8..3f5db1990aba 100644 > > > --- a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml > > > +++ b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml > > > @@ -55,7 +55,7 @@ examples: > > > #size-cells = <1>; > > > watchdog@28 { > > > - compatible = "brcm,bcm7038-wdt"; > > > + compatible = "brcm,bcm4908-wdt", "brcm,bcm63xx-wdt"; > > > reg = <0x28 0x8>; > > > }; > > > }; > > > diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml > > > index a926809352b8..4d848442913c 100644 > > > --- a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml > > > +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml > > > @@ -16,9 +16,22 @@ maintainers: > > > properties: > > > compatible: > > > - enum: > > > - - brcm,bcm6345-wdt > > > - - brcm,bcm7038-wdt > > > + items: > > > + - enum: > > > + - brcm,bcm4908-wdt > > > + - brcm,bcm6338-wdt > > > + - brcm,bcm6345-wdt > > > + - brcm,bcm6348-wdt > > > + - brcm,bcm6848-wdt > > > + - brcm,bcm6858-wdt > > > + - brcm,bcm7038-wdt > > > + - brcm,bcm60333-wdt > > > + - brcm,bcm63138-wdt > > > + - brcm,bcm63148-wdt > > > + - brcm,bcm63268-wdt > > > + - brcm,bcm63381-wdt > > > + - brcm,bcm68360-wdt > > > + - const: brcm,bcm63xx-wdt > > > > Is it really worthwhile to update all these DTs?: > > > > arch/mips/boot/dts/brcm/bcm63268.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm6328.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm6358.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm6362.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm6368.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm7125.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm7346.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm7358.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm7360.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm7362.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm7420.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm7425.dtsi: compatible = "brcm,bcm7038-wdt"; > > arch/mips/boot/dts/brcm/bcm7435.dtsi: compatible = "brcm,bcm7038-wdt"; > > I don't have problem handling that. Even if the dts files above are updated, the driver still has to support the above case. It's pointless to change this. We've already got 1000s of warnings to fix and 2000 bindings still to convert if you need something to do. > > I don't think so. > So what's the policy for such bindings then? How to select SoCs that > should have their own bindings? How can I tell which should /borrow/ a > binding instead? The way it is supposed to work is the first implementation gets 'soc1-block'. When the 2nd implementation comes about, it gets 'soc2-block'. If soc2 implementation is 'the same' or a superset, then it should have a fallback to 'soc1-block'. Another way to test that is, "I want my existing s/w to work as-is with this new h/w". The process repeats on the next SoC, but you could be backwards compatible with soc1, soc2, both or none. Is that what you are asking? If you are doing all this after the fact at once, then it can be a bit different in how you do compatibles. In this case, I would make "brcm,bcm7038-wdt" the fallback to the rest (granted, I know nothing about these chips or the relationship between them), but you have to keep supporting "brcm,bcm7038-wdt". 6345 is up to you I guess as there aren't any upstream dts files using it. Florian might care. Rob
On 9.02.2022 21:02, Rob Herring wrote: > On Wed, Feb 09, 2022 at 08:26:00PM +0100, Rafał Miłecki wrote: >> On 9.02.2022 20:09, Rob Herring wrote: >>> On Wed, Jan 26, 2022 at 11:20:34PM +0100, Rafał Miłecki wrote: >>>> From: Rafał Miłecki <rafal@milecki.pl> >>>> >>>> This hardware block is used on almost all BCM63xx family chipsets and >>>> BCM4908 which reuses a lot of BCM63xx parts. Add relevant compatible >>>> strings and also include a generic one. >>>> >>>> The only SoC with a different block I found is BCM6838 (thus not included >>>> in this change). >>>> >>>> It may be worth noting that BCM6338, BCM6345, BCM6348 and BCM63268 don't >>>> include "SoftRst" register but that can be handled by drivers based on >>>> precise compatible string. >>>> >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>>> --- >>>> V2: Sort enum entries & update brcm,twd.yaml >>>> --- >>>> .../devicetree/bindings/mfd/brcm,twd.yaml | 2 +- >>>> .../bindings/watchdog/brcm,bcm7038-wdt.yaml | 21 +++++++++++++++---- >>>> 2 files changed, 18 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml >>>> index 634526f790b8..3f5db1990aba 100644 >>>> --- a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml >>>> +++ b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml >>>> @@ -55,7 +55,7 @@ examples: >>>> #size-cells = <1>; >>>> watchdog@28 { >>>> - compatible = "brcm,bcm7038-wdt"; >>>> + compatible = "brcm,bcm4908-wdt", "brcm,bcm63xx-wdt"; >>>> reg = <0x28 0x8>; >>>> }; >>>> }; >>>> diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml >>>> index a926809352b8..4d848442913c 100644 >>>> --- a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml >>>> +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml >>>> @@ -16,9 +16,22 @@ maintainers: >>>> properties: >>>> compatible: >>>> - enum: >>>> - - brcm,bcm6345-wdt >>>> - - brcm,bcm7038-wdt >>>> + items: >>>> + - enum: >>>> + - brcm,bcm4908-wdt >>>> + - brcm,bcm6338-wdt >>>> + - brcm,bcm6345-wdt >>>> + - brcm,bcm6348-wdt >>>> + - brcm,bcm6848-wdt >>>> + - brcm,bcm6858-wdt >>>> + - brcm,bcm7038-wdt >>>> + - brcm,bcm60333-wdt >>>> + - brcm,bcm63138-wdt >>>> + - brcm,bcm63148-wdt >>>> + - brcm,bcm63268-wdt >>>> + - brcm,bcm63381-wdt >>>> + - brcm,bcm68360-wdt >>>> + - const: brcm,bcm63xx-wdt >>> >>> Is it really worthwhile to update all these DTs?: >>> >>> arch/mips/boot/dts/brcm/bcm63268.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm6328.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm6358.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm6362.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm6368.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm7125.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm7346.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm7358.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm7360.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm7362.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm7420.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm7425.dtsi: compatible = "brcm,bcm7038-wdt"; >>> arch/mips/boot/dts/brcm/bcm7435.dtsi: compatible = "brcm,bcm7038-wdt"; >> >> I don't have problem handling that. > > Even if the dts files above are updated, the driver still has to support > the above case. > > It's pointless to change this. We've already got 1000s of warnings to > fix and 2000 bindings still to convert if you need something to do. > >>> I don't think so. >> So what's the policy for such bindings then? How to select SoCs that >> should have their own bindings? How can I tell which should /borrow/ a >> binding instead? > > The way it is supposed to work is the first implementation gets > 'soc1-block'. When the 2nd implementation comes about, it gets > 'soc2-block'. If soc2 implementation is 'the same' or a superset, then > it should have a fallback to 'soc1-block'. Another way to test that is, > "I want my existing s/w to work as-is with this new h/w". The process > repeats on the next SoC, but you could be backwards compatible with > soc1, soc2, both or none. Is that what you are asking? > > If you are doing all this after the fact at once, then it can be a > bit different in how you do compatibles. > > In this case, I would make "brcm,bcm7038-wdt" the fallback to the rest > (granted, I know nothing about these chips or the relationship between > them), but you have to keep supporting "brcm,bcm7038-wdt". 6345 is up > to you I guess as there aren't any upstream dts files using it. Florian > might care. I think I got it now, thanks for explaining!
diff --git a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml index 634526f790b8..3f5db1990aba 100644 --- a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml +++ b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml @@ -55,7 +55,7 @@ examples: #size-cells = <1>; watchdog@28 { - compatible = "brcm,bcm7038-wdt"; + compatible = "brcm,bcm4908-wdt", "brcm,bcm63xx-wdt"; reg = <0x28 0x8>; }; }; diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml index a926809352b8..4d848442913c 100644 --- a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml @@ -16,9 +16,22 @@ maintainers: properties: compatible: - enum: - - brcm,bcm6345-wdt - - brcm,bcm7038-wdt + items: + - enum: + - brcm,bcm4908-wdt + - brcm,bcm6338-wdt + - brcm,bcm6345-wdt + - brcm,bcm6348-wdt + - brcm,bcm6848-wdt + - brcm,bcm6858-wdt + - brcm,bcm7038-wdt + - brcm,bcm60333-wdt + - brcm,bcm63138-wdt + - brcm,bcm63148-wdt + - brcm,bcm63268-wdt + - brcm,bcm63381-wdt + - brcm,bcm68360-wdt + - const: brcm,bcm63xx-wdt reg: maxItems: 1 @@ -37,7 +50,7 @@ required: examples: - | watchdog@f040a7e8 { - compatible = "brcm,bcm7038-wdt"; + compatible = "brcm,bcm7038-wdt", "brcm,bcm63xx-wdt"; reg = <0xf040a7e8 0x16>; clocks = <&upg_fixed>; };