| Message ID | 20201110033113.31090-1-f.fainelli@gmail.com |
|---|---|
| Headers | show |
| Series | Broadcom b53 YAML bindings | expand |
10.11.2020 04:31, Florian Fainelli wrote: > Provide an empty 'ports' container node with the correct #address-cells > and #size-cells properties. This silences the following warning: > > arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dt.yaml: > ethernet-switch@18007000: 'oneOf' conditional failed, one must be fixed: > 'ports' is a required property > 'ethernet-ports' is a required property > From schema: > Documentation/devicetree/bindings/net/dsa/b53.yaml > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > arch/arm/boot/dts/bcm5301x.dtsi | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi > index 807580dd89f5..89993a8a6765 100644 > --- a/arch/arm/boot/dts/bcm5301x.dtsi > +++ b/arch/arm/boot/dts/bcm5301x.dtsi > @@ -489,6 +489,10 @@ srab: ethernet-switch@18007000 { > status = "disabled"; > > /* ports are defined in board DTS */ > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + }; You can drop those two lines from board files now I believe. grep "ports {" arch/arm/boot/dts/bcm470* + arch/arm/boot/dts/bcm953012er.dts
On 11/10/2020 1:31 AM, Rafał Miłecki wrote: > 10.11.2020 04:31, Florian Fainelli wrote: >> Provide an empty 'ports' container node with the correct #address-cells >> and #size-cells properties. This silences the following warning: >> >> arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dt.yaml: >> ethernet-switch@18007000: 'oneOf' conditional failed, one must be fixed: >> 'ports' is a required property >> 'ethernet-ports' is a required property >> From schema: >> Documentation/devicetree/bindings/net/dsa/b53.yaml >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> arch/arm/boot/dts/bcm5301x.dtsi | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi >> b/arch/arm/boot/dts/bcm5301x.dtsi >> index 807580dd89f5..89993a8a6765 100644 >> --- a/arch/arm/boot/dts/bcm5301x.dtsi >> +++ b/arch/arm/boot/dts/bcm5301x.dtsi >> @@ -489,6 +489,10 @@ srab: ethernet-switch@18007000 { >> status = "disabled"; >> /* ports are defined in board DTS */ >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + }; > > You can drop those two lines from board files now I believe. > > grep "ports {" arch/arm/boot/dts/bcm470* > + arch/arm/boot/dts/bcm953012er.dts Yes, indeed, thanks!
On Mon, Nov 09, 2020 at 07:31:06PM -0800, Florian Fainelli wrote: > Update the switch unit name from srab to ethernet-switch, allowing us to > fix warnings such as: > > CHECK arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dt.yaml > arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dt.yaml: > srab@18007000: $nodename:0: 'srab@18007000' does not match > '^(ethernet-)?switch(@.*)?$' > From schema: > Documentation/devicetree/bindings/net/dsa/b53.yaml > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- You're lucky that you don't have any firmware that fixes up DT properties by path. Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
On Mon, Nov 09, 2020 at 07:31:07PM -0800, Florian Fainelli wrote: > Provide a default compatible string which is based on the 53010 SRAB > compatible, this allows us to have sane defaults and silences the > following warnings: > > arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dt.yaml: > ethernet-switch@18007000: compatible: 'oneOf' conditional failed, one > must be fixed: > ['brcm,bcm5301x-srab'] is too short > 'brcm,bcm5325' was expected > 'brcm,bcm53115' was expected > 'brcm,bcm53125' was expected > 'brcm,bcm53128' was expected > 'brcm,bcm5365' was expected > 'brcm,bcm5395' was expected > 'brcm,bcm5389' was expected > 'brcm,bcm5397' was expected > 'brcm,bcm5398' was expected > 'brcm,bcm11360-srab' was expected > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm53010-srab', > 'brcm,bcm53011-srab', 'brcm,bcm53012-srab', 'brcm,bcm53018-srab', > 'brcm,bcm53019-srab'] > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm11404-srab', > 'brcm,bcm11407-srab', 'brcm,bcm11409-srab', 'brcm,bcm58310-srab', > 'brcm,bcm58311-srab', 'brcm,bcm58313-srab'] > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm58522-srab', > 'brcm,bcm58523-srab', 'brcm,bcm58525-srab', 'brcm,bcm58622-srab', > 'brcm,bcm58623-srab', 'brcm,bcm58625-srab', 'brcm,bcm88312-srab'] > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm3384-switch', > 'brcm,bcm6328-switch', 'brcm,bcm6368-switch'] > From schema: > Documentation/devicetree/bindings/net/dsa/b53.yaml > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- Nice, I didn't know DSA supported the switch inside this device. In the default AsusWRT, the switch is well hidden from the kernel :) Not that it makes any difference as far as I can see, but how do you know this a BCM53010 SRAB specifically? > arch/arm/boot/dts/bcm5301x.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi > index ee23c0841699..807580dd89f5 100644 > --- a/arch/arm/boot/dts/bcm5301x.dtsi > +++ b/arch/arm/boot/dts/bcm5301x.dtsi > @@ -483,7 +483,7 @@ thermal: thermal@1800c2c0 { > }; > > srab: ethernet-switch@18007000 { > - compatible = "brcm,bcm5301x-srab"; > + compatible = "brcm,bcm53010-srab", "brcm,bcm5301x-srab"; > reg = <0x18007000 0x1000>; > > status = "disabled"; > -- > 2.25.1 >
On 11/10/20 2:06 PM, Vladimir Oltean wrote: > On Mon, Nov 09, 2020 at 07:31:07PM -0800, Florian Fainelli wrote: >> Provide a default compatible string which is based on the 53010 SRAB >> compatible, this allows us to have sane defaults and silences the >> following warnings: >> >> arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dt.yaml: >> ethernet-switch@18007000: compatible: 'oneOf' conditional failed, one >> must be fixed: >> ['brcm,bcm5301x-srab'] is too short >> 'brcm,bcm5325' was expected >> 'brcm,bcm53115' was expected >> 'brcm,bcm53125' was expected >> 'brcm,bcm53128' was expected >> 'brcm,bcm5365' was expected >> 'brcm,bcm5395' was expected >> 'brcm,bcm5389' was expected >> 'brcm,bcm5397' was expected >> 'brcm,bcm5398' was expected >> 'brcm,bcm11360-srab' was expected >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm53010-srab', >> 'brcm,bcm53011-srab', 'brcm,bcm53012-srab', 'brcm,bcm53018-srab', >> 'brcm,bcm53019-srab'] >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm11404-srab', >> 'brcm,bcm11407-srab', 'brcm,bcm11409-srab', 'brcm,bcm58310-srab', >> 'brcm,bcm58311-srab', 'brcm,bcm58313-srab'] >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm58522-srab', >> 'brcm,bcm58523-srab', 'brcm,bcm58525-srab', 'brcm,bcm58622-srab', >> 'brcm,bcm58623-srab', 'brcm,bcm58625-srab', 'brcm,bcm88312-srab'] >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm3384-switch', >> 'brcm,bcm6328-switch', 'brcm,bcm6368-switch'] >> From schema: >> Documentation/devicetree/bindings/net/dsa/b53.yaml >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- > > Nice, I didn't know DSA supported the switch inside this device. In the > default AsusWRT, the switch is well hidden from the kernel :) Yeah right :) The YAML validation works even when the nodes are disabled, so we somehow need to account for all of these devices' DTS. > > Not that it makes any difference as far as I can see, but how do you > know this a BCM53010 SRAB specifically? The 4708 is effectively a 53010 which is why this is valid as a fallback. Any node that defines switch ports and uses a SoC different than the 53010 should use an appropriate "compatible" string though. That's what is done for NSP. -- Florian
On Mon, Nov 09, 2020 at 07:31:08PM -0800, Florian Fainelli wrote: > Provide an empty 'ports' container node with the correct #address-cells > and #size-cells properties. This silences the following warning: > > arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dt.yaml: > ethernet-switch@18007000: 'oneOf' conditional failed, one must be fixed: > 'ports' is a required property > 'ethernet-ports' is a required property > From schema: > Documentation/devicetree/bindings/net/dsa/b53.yaml > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > arch/arm/boot/dts/bcm5301x.dtsi | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi > index 807580dd89f5..89993a8a6765 100644 > --- a/arch/arm/boot/dts/bcm5301x.dtsi > +++ b/arch/arm/boot/dts/bcm5301x.dtsi > @@ -489,6 +489,10 @@ srab: ethernet-switch@18007000 { > status = "disabled"; > > /* ports are defined in board DTS */ > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + }; This look a bit 'lone wolf' here. Not sure how much time you intend to spend on this, but FWIW, others prefer to declare all ports in the SoC DTSI with status = "disabled", and just enable the ones used per-board, and add labels and PHY handles also per-board. Example: fsl-ls1028a.dtsi and fsl-ls1028a-rdb.dts. > }; > > rng: rng@18004000 { > -- > 2.25.1 >
On 11/10/20 2:12 PM, Vladimir Oltean wrote: > On Mon, Nov 09, 2020 at 07:31:08PM -0800, Florian Fainelli wrote: >> Provide an empty 'ports' container node with the correct #address-cells >> and #size-cells properties. This silences the following warning: >> >> arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dt.yaml: >> ethernet-switch@18007000: 'oneOf' conditional failed, one must be fixed: >> 'ports' is a required property >> 'ethernet-ports' is a required property >> From schema: >> Documentation/devicetree/bindings/net/dsa/b53.yaml >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> arch/arm/boot/dts/bcm5301x.dtsi | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi >> index 807580dd89f5..89993a8a6765 100644 >> --- a/arch/arm/boot/dts/bcm5301x.dtsi >> +++ b/arch/arm/boot/dts/bcm5301x.dtsi >> @@ -489,6 +489,10 @@ srab: ethernet-switch@18007000 { >> status = "disabled"; >> >> /* ports are defined in board DTS */ >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + }; > > This look a bit 'lone wolf' here. Not sure how much time you intend to > spend on this, but FWIW, others prefer to declare all ports in the SoC > DTSI with status = "disabled", and just enable the ones used per-board, > and add labels and PHY handles also per-board. Example: fsl-ls1028a.dtsi > and fsl-ls1028a-rdb.dts. That's a good suggestion, I could do that.
On Mon, Nov 09, 2020 at 07:31:09PM -0800, Florian Fainelli wrote: > Update the switch unit name from srab to ethernet-switch, allowing us > to fix warnings such as: > > CHECK arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dt.yaml > arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dt.yaml: > srab@18007000: $nodename:0: 'srab@18007000' does not match > '^(ethernet-)?switch(@.*)?$' > From schema: > Documentation/devicetree/bindings/net/dsa/b53.yaml > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > arch/arm/boot/dts/bcm-nsp.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi > index e895f7cb8c9f..e7d08959d5fe 100644 > --- a/arch/arm/boot/dts/bcm-nsp.dtsi > +++ b/arch/arm/boot/dts/bcm-nsp.dtsi > @@ -385,7 +385,7 @@ ccbtimer1: timer@35000 { > clock-names = "apb_pclk"; > }; > > - srab: srab@36000 { > + srab: ethernet-switch@36000 { > compatible = "brcm,nsp-srab"; > reg = <0x36000 0x1000>, > <0x3f308 0x8>, > -- > 2.25.1 >
On Mon, Nov 09, 2020 at 07:31:10PM -0800, Florian Fainelli wrote: > The register name should be "sgmii_config", not "sgmii", this is not a > functional change since no code is currently looking for that register > by name (or at all). > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- I.e. no one calls devm_platform_ioremap_resource(pdev, 2), and even if it did, the register name wouldn't matter. ...but at least it's documented that it should be called "sgmii_config". And the address is the same as the one that's documented for the SGMII register base in SoCs with "brcm,nsp-srab", so even without documentation, it is pretty clear to me that it was a mistake calling it simply "sgmii". And if that address is incorrect anyways, at least that would be a separate issue. Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > arch/arm/boot/dts/bcm-nsp.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi > index e7d08959d5fe..09fd7e55c069 100644 > --- a/arch/arm/boot/dts/bcm-nsp.dtsi > +++ b/arch/arm/boot/dts/bcm-nsp.dtsi > @@ -390,7 +390,7 @@ srab: ethernet-switch@36000 { > reg = <0x36000 0x1000>, > <0x3f308 0x8>, > <0x3f410 0xc>; > - reg-names = "srab", "mux_config", "sgmii"; > + reg-names = "srab", "mux_config", "sgmii_config"; > interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>, > -- > 2.25.1 >
On Mon, Nov 09, 2020 at 07:31:11PM -0800, Florian Fainelli wrote: > Provide a default compatible string which is based on the 58522 SRAB > compatible, this allows us to have sane defaults and silences the > following warnings: > > arch/arm/boot/dts/bcm958522er.dt.yaml: > ethernet-switch@36000: compatible: 'oneOf' conditional failed, > one > must be fixed: > ['brcm,bcm5301x-srab'] is too short > 'brcm,bcm5325' was expected > 'brcm,bcm53115' was expected > 'brcm,bcm53125' was expected > 'brcm,bcm53128' was expected > 'brcm,bcm5365' was expected > 'brcm,bcm5395' was expected > 'brcm,bcm5389' was expected > 'brcm,bcm5397' was expected > 'brcm,bcm5398' was expected > 'brcm,bcm11360-srab' was expected > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm53010-srab', > 'brcm,bcm53011-srab', 'brcm,bcm53012-srab', 'brcm,bcm53018-srab', > 'brcm,bcm53019-srab'] > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm11404-srab', > 'brcm,bcm11407-srab', 'brcm,bcm11409-srab', 'brcm,bcm58310-srab', > 'brcm,bcm58311-srab', 'brcm,bcm58313-srab'] > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm58522-srab', > 'brcm,bcm58523-srab', 'brcm,bcm58525-srab', 'brcm,bcm58622-srab', > 'brcm,bcm58623-srab', 'brcm,bcm58625-srab', 'brcm,bcm88312-srab'] > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm3384-switch', > 'brcm,bcm6328-switch', 'brcm,bcm6368-switch'] > From schema: > Documentation/devicetree/bindings/net/dsa/b53.yaml > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > arch/arm/boot/dts/bcm-nsp.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi > index 09fd7e55c069..8453865d1439 100644 > --- a/arch/arm/boot/dts/bcm-nsp.dtsi > +++ b/arch/arm/boot/dts/bcm-nsp.dtsi > @@ -386,7 +386,7 @@ ccbtimer1: timer@35000 { > }; > > srab: ethernet-switch@36000 { > - compatible = "brcm,nsp-srab"; > + compatible = "brcm,bcm58522-srab", "brcm,nsp-srab"; > reg = <0x36000 0x1000>, > <0x3f308 0x8>, > <0x3f410 0xc>; > -- > 2.25.1 > I am not getting this. The line: #include "bcm-nsp.dtsi" can be found in: arch/arm/boot/dts/bcm988312hr.dts arch/arm/boot/dts/bcm958625hr.dts arch/arm/boot/dts/bcm958622hr.dts arch/arm/boot/dts/bcm958625k.dts arch/arm/boot/dts/bcm958522er.dts arch/arm/boot/dts/bcm958525er.dts arch/arm/boot/dts/bcm958623hr.dts arch/arm/boot/dts/bcm958525xmc.dts The pattern for the other DTS files that include this seems to be to overwrite the compatible locally in bcm958522er.dts, like this: &srab { compatible = "brcm,bcm58522-srab", "brcm,nsp-srab"; }; Is there a reason why you are choosing to put an SoC specific compatible in the common bcm-nsp.dtsi?
On 11/10/20 2:37 PM, Vladimir Oltean wrote: > On Mon, Nov 09, 2020 at 07:31:11PM -0800, Florian Fainelli wrote: >> Provide a default compatible string which is based on the 58522 SRAB >> compatible, this allows us to have sane defaults and silences the >> following warnings: >> >> arch/arm/boot/dts/bcm958522er.dt.yaml: >> ethernet-switch@36000: compatible: 'oneOf' conditional failed, >> one >> must be fixed: >> ['brcm,bcm5301x-srab'] is too short >> 'brcm,bcm5325' was expected >> 'brcm,bcm53115' was expected >> 'brcm,bcm53125' was expected >> 'brcm,bcm53128' was expected >> 'brcm,bcm5365' was expected >> 'brcm,bcm5395' was expected >> 'brcm,bcm5389' was expected >> 'brcm,bcm5397' was expected >> 'brcm,bcm5398' was expected >> 'brcm,bcm11360-srab' was expected >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm53010-srab', >> 'brcm,bcm53011-srab', 'brcm,bcm53012-srab', 'brcm,bcm53018-srab', >> 'brcm,bcm53019-srab'] >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm11404-srab', >> 'brcm,bcm11407-srab', 'brcm,bcm11409-srab', 'brcm,bcm58310-srab', >> 'brcm,bcm58311-srab', 'brcm,bcm58313-srab'] >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm58522-srab', >> 'brcm,bcm58523-srab', 'brcm,bcm58525-srab', 'brcm,bcm58622-srab', >> 'brcm,bcm58623-srab', 'brcm,bcm58625-srab', 'brcm,bcm88312-srab'] >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm3384-switch', >> 'brcm,bcm6328-switch', 'brcm,bcm6368-switch'] >> From schema: >> Documentation/devicetree/bindings/net/dsa/b53.yaml >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> arch/arm/boot/dts/bcm-nsp.dtsi | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi >> index 09fd7e55c069..8453865d1439 100644 >> --- a/arch/arm/boot/dts/bcm-nsp.dtsi >> +++ b/arch/arm/boot/dts/bcm-nsp.dtsi >> @@ -386,7 +386,7 @@ ccbtimer1: timer@35000 { >> }; >> >> srab: ethernet-switch@36000 { >> - compatible = "brcm,nsp-srab"; >> + compatible = "brcm,bcm58522-srab", "brcm,nsp-srab"; >> reg = <0x36000 0x1000>, >> <0x3f308 0x8>, >> <0x3f410 0xc>; >> -- >> 2.25.1 >> > > I am not getting this. > The line: > #include "bcm-nsp.dtsi" > > can be found in: > > arch/arm/boot/dts/bcm988312hr.dts > arch/arm/boot/dts/bcm958625hr.dts > arch/arm/boot/dts/bcm958622hr.dts > arch/arm/boot/dts/bcm958625k.dts > arch/arm/boot/dts/bcm958522er.dts > arch/arm/boot/dts/bcm958525er.dts > arch/arm/boot/dts/bcm958623hr.dts > arch/arm/boot/dts/bcm958525xmc.dts > > > The pattern for the other DTS files that include this seems to be to > overwrite the compatible locally in bcm958522er.dts, like this: > > &srab { > compatible = "brcm,bcm58522-srab", "brcm,nsp-srab"; > }; > > Is there a reason why you are choosing to put an SoC specific compatible > in the common bcm-nsp.dtsi? It is necessary to silence the warnings provided in the commit message even when the srab node is disabled, since the dt_binding_check rule will check all of the nodes matching the pattern. If there is a better way to do this, I would gladly do it differently.
On Tue, Nov 10, 2020 at 02:40:43PM -0800, Florian Fainelli wrote: > On 11/10/20 2:37 PM, Vladimir Oltean wrote: > > On Mon, Nov 09, 2020 at 07:31:11PM -0800, Florian Fainelli wrote: > >> Provide a default compatible string which is based on the 58522 SRAB > >> compatible, this allows us to have sane defaults and silences the > >> following warnings: > >> > >> arch/arm/boot/dts/bcm958522er.dt.yaml: > >> ethernet-switch@36000: compatible: 'oneOf' conditional failed, > >> one > >> must be fixed: > >> ['brcm,bcm5301x-srab'] is too short > >> 'brcm,bcm5325' was expected > >> 'brcm,bcm53115' was expected > >> 'brcm,bcm53125' was expected > >> 'brcm,bcm53128' was expected > >> 'brcm,bcm5365' was expected > >> 'brcm,bcm5395' was expected > >> 'brcm,bcm5389' was expected > >> 'brcm,bcm5397' was expected > >> 'brcm,bcm5398' was expected > >> 'brcm,bcm11360-srab' was expected > >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm53010-srab', > >> 'brcm,bcm53011-srab', 'brcm,bcm53012-srab', 'brcm,bcm53018-srab', > >> 'brcm,bcm53019-srab'] > >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm11404-srab', > >> 'brcm,bcm11407-srab', 'brcm,bcm11409-srab', 'brcm,bcm58310-srab', > >> 'brcm,bcm58311-srab', 'brcm,bcm58313-srab'] > >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm58522-srab', > >> 'brcm,bcm58523-srab', 'brcm,bcm58525-srab', 'brcm,bcm58622-srab', > >> 'brcm,bcm58623-srab', 'brcm,bcm58625-srab', 'brcm,bcm88312-srab'] > >> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm3384-switch', > >> 'brcm,bcm6328-switch', 'brcm,bcm6368-switch'] > >> From schema: > >> Documentation/devicetree/bindings/net/dsa/b53.yaml > >> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- > >> arch/arm/boot/dts/bcm-nsp.dtsi | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi > >> index 09fd7e55c069..8453865d1439 100644 > >> --- a/arch/arm/boot/dts/bcm-nsp.dtsi > >> +++ b/arch/arm/boot/dts/bcm-nsp.dtsi > >> @@ -386,7 +386,7 @@ ccbtimer1: timer@35000 { > >> }; > >> > >> srab: ethernet-switch@36000 { > >> - compatible = "brcm,nsp-srab"; > >> + compatible = "brcm,bcm58522-srab", "brcm,nsp-srab"; > >> reg = <0x36000 0x1000>, > >> <0x3f308 0x8>, > >> <0x3f410 0xc>; > >> -- > >> 2.25.1 > >> > > > > I am not getting this. > > The line: > > #include "bcm-nsp.dtsi" > > > > can be found in: > > > > arch/arm/boot/dts/bcm988312hr.dts > > arch/arm/boot/dts/bcm958625hr.dts > > arch/arm/boot/dts/bcm958622hr.dts > > arch/arm/boot/dts/bcm958625k.dts > > arch/arm/boot/dts/bcm958522er.dts > > arch/arm/boot/dts/bcm958525er.dts > > arch/arm/boot/dts/bcm958623hr.dts > > arch/arm/boot/dts/bcm958525xmc.dts > > > > > > The pattern for the other DTS files that include this seems to be to > > overwrite the compatible locally in bcm958522er.dts, like this: > > > > &srab { > > compatible = "brcm,bcm58522-srab", "brcm,nsp-srab"; > > }; > > > > Is there a reason why you are choosing to put an SoC specific compatible > > in the common bcm-nsp.dtsi? > > It is necessary to silence the warnings provided in the commit message > even when the srab node is disabled, since the dt_binding_check rule > will check all of the nodes matching the pattern. If there is a better > way to do this, I would gladly do it differently. > -- > Florian I am still not getting it. The exact 3 lines from above will not change the "status" property from "disabled" to "okay", so I don't understand why it matters whether it's enabled or not. The dt_binding_check error isn't in the DTSI, it's in bcm958522er.dts. All that needs to be done is that the bcm958522er.dts needs to override the compatible from the DTSI and only the compatible, I believe. With no occurrence of an incomplete list of compatibles in any final DTS, the dt_binding_check should not complain about that single occurrence in the DTSI as far as I know (and I did not test this).
On 11/10/20 2:48 PM, Vladimir Oltean wrote: > On Tue, Nov 10, 2020 at 02:40:43PM -0800, Florian Fainelli wrote: >> On 11/10/20 2:37 PM, Vladimir Oltean wrote: >>> On Mon, Nov 09, 2020 at 07:31:11PM -0800, Florian Fainelli wrote: >>>> Provide a default compatible string which is based on the 58522 SRAB >>>> compatible, this allows us to have sane defaults and silences the >>>> following warnings: >>>> >>>> arch/arm/boot/dts/bcm958522er.dt.yaml: >>>> ethernet-switch@36000: compatible: 'oneOf' conditional failed, >>>> one >>>> must be fixed: >>>> ['brcm,bcm5301x-srab'] is too short >>>> 'brcm,bcm5325' was expected >>>> 'brcm,bcm53115' was expected >>>> 'brcm,bcm53125' was expected >>>> 'brcm,bcm53128' was expected >>>> 'brcm,bcm5365' was expected >>>> 'brcm,bcm5395' was expected >>>> 'brcm,bcm5389' was expected >>>> 'brcm,bcm5397' was expected >>>> 'brcm,bcm5398' was expected >>>> 'brcm,bcm11360-srab' was expected >>>> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm53010-srab', >>>> 'brcm,bcm53011-srab', 'brcm,bcm53012-srab', 'brcm,bcm53018-srab', >>>> 'brcm,bcm53019-srab'] >>>> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm11404-srab', >>>> 'brcm,bcm11407-srab', 'brcm,bcm11409-srab', 'brcm,bcm58310-srab', >>>> 'brcm,bcm58311-srab', 'brcm,bcm58313-srab'] >>>> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm58522-srab', >>>> 'brcm,bcm58523-srab', 'brcm,bcm58525-srab', 'brcm,bcm58622-srab', >>>> 'brcm,bcm58623-srab', 'brcm,bcm58625-srab', 'brcm,bcm88312-srab'] >>>> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm3384-switch', >>>> 'brcm,bcm6328-switch', 'brcm,bcm6368-switch'] >>>> From schema: >>>> Documentation/devicetree/bindings/net/dsa/b53.yaml >>>> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>> --- >>>> arch/arm/boot/dts/bcm-nsp.dtsi | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi >>>> index 09fd7e55c069..8453865d1439 100644 >>>> --- a/arch/arm/boot/dts/bcm-nsp.dtsi >>>> +++ b/arch/arm/boot/dts/bcm-nsp.dtsi >>>> @@ -386,7 +386,7 @@ ccbtimer1: timer@35000 { >>>> }; >>>> >>>> srab: ethernet-switch@36000 { >>>> - compatible = "brcm,nsp-srab"; >>>> + compatible = "brcm,bcm58522-srab", "brcm,nsp-srab"; >>>> reg = <0x36000 0x1000>, >>>> <0x3f308 0x8>, >>>> <0x3f410 0xc>; >>>> -- >>>> 2.25.1 >>>> >>> >>> I am not getting this. >>> The line: >>> #include "bcm-nsp.dtsi" >>> >>> can be found in: >>> >>> arch/arm/boot/dts/bcm988312hr.dts >>> arch/arm/boot/dts/bcm958625hr.dts >>> arch/arm/boot/dts/bcm958622hr.dts >>> arch/arm/boot/dts/bcm958625k.dts >>> arch/arm/boot/dts/bcm958522er.dts >>> arch/arm/boot/dts/bcm958525er.dts >>> arch/arm/boot/dts/bcm958623hr.dts >>> arch/arm/boot/dts/bcm958525xmc.dts >>> >>> >>> The pattern for the other DTS files that include this seems to be to >>> overwrite the compatible locally in bcm958522er.dts, like this: >>> >>> &srab { >>> compatible = "brcm,bcm58522-srab", "brcm,nsp-srab"; >>> }; >>> >>> Is there a reason why you are choosing to put an SoC specific compatible >>> in the common bcm-nsp.dtsi? >> >> It is necessary to silence the warnings provided in the commit message >> even when the srab node is disabled, since the dt_binding_check rule >> will check all of the nodes matching the pattern. If there is a better >> way to do this, I would gladly do it differently. >> -- >> Florian > > I am still not getting it. The exact 3 lines from above will not change > the "status" property from "disabled" to "okay", so I don't understand > why it matters whether it's enabled or not. The dt_binding_check error > isn't in the DTSI, it's in bcm958522er.dts. All that needs to be done is > that the bcm958522er.dts needs to override the compatible from the DTSI > and only the compatible, I believe. With no occurrence of an incomplete > list of compatibles in any final DTS, the dt_binding_check should not > complain about that single occurrence in the DTSI as far as I know (and > I did not test this). There is not a switch being enabled in arch/arm/boot/dts/bcm958522er.dts, so sure, I could add the 3 lines you quote above and that would silence the warning, but that does not scale at all across DTS files including bcm5301x.dtsi for instance, it sort of does for those including bcm-nsp.dtsi.
On Tue, Nov 10, 2020 at 02:52:57PM -0800, Florian Fainelli wrote: > On 11/10/20 2:48 PM, Vladimir Oltean wrote: > > On Tue, Nov 10, 2020 at 02:40:43PM -0800, Florian Fainelli wrote: > >> On 11/10/20 2:37 PM, Vladimir Oltean wrote: > >>> On Mon, Nov 09, 2020 at 07:31:11PM -0800, Florian Fainelli wrote: > >>>> Provide a default compatible string which is based on the 58522 SRAB > >>>> compatible, this allows us to have sane defaults and silences the > >>>> following warnings: > >>>> > >>>> arch/arm/boot/dts/bcm958522er.dt.yaml: > >>>> ethernet-switch@36000: compatible: 'oneOf' conditional failed, > >>>> one > >>>> must be fixed: > >>>> ['brcm,bcm5301x-srab'] is too short > >>>> 'brcm,bcm5325' was expected > >>>> 'brcm,bcm53115' was expected > >>>> 'brcm,bcm53125' was expected > >>>> 'brcm,bcm53128' was expected > >>>> 'brcm,bcm5365' was expected > >>>> 'brcm,bcm5395' was expected > >>>> 'brcm,bcm5389' was expected > >>>> 'brcm,bcm5397' was expected > >>>> 'brcm,bcm5398' was expected > >>>> 'brcm,bcm11360-srab' was expected > >>>> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm53010-srab', > >>>> 'brcm,bcm53011-srab', 'brcm,bcm53012-srab', 'brcm,bcm53018-srab', > >>>> 'brcm,bcm53019-srab'] > >>>> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm11404-srab', > >>>> 'brcm,bcm11407-srab', 'brcm,bcm11409-srab', 'brcm,bcm58310-srab', > >>>> 'brcm,bcm58311-srab', 'brcm,bcm58313-srab'] > >>>> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm58522-srab', > >>>> 'brcm,bcm58523-srab', 'brcm,bcm58525-srab', 'brcm,bcm58622-srab', > >>>> 'brcm,bcm58623-srab', 'brcm,bcm58625-srab', 'brcm,bcm88312-srab'] > >>>> 'brcm,bcm5301x-srab' is not one of ['brcm,bcm3384-switch', > >>>> 'brcm,bcm6328-switch', 'brcm,bcm6368-switch'] > >>>> From schema: > >>>> Documentation/devicetree/bindings/net/dsa/b53.yaml > >>>> > >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >>>> --- > >>>> arch/arm/boot/dts/bcm-nsp.dtsi | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi > >>>> index 09fd7e55c069..8453865d1439 100644 > >>>> --- a/arch/arm/boot/dts/bcm-nsp.dtsi > >>>> +++ b/arch/arm/boot/dts/bcm-nsp.dtsi > >>>> @@ -386,7 +386,7 @@ ccbtimer1: timer@35000 { > >>>> }; > >>>> > >>>> srab: ethernet-switch@36000 { > >>>> - compatible = "brcm,nsp-srab"; > >>>> + compatible = "brcm,bcm58522-srab", "brcm,nsp-srab"; > >>>> reg = <0x36000 0x1000>, > >>>> <0x3f308 0x8>, > >>>> <0x3f410 0xc>; > >>>> -- > >>>> 2.25.1 > >>>> > >>> > >>> I am not getting this. > >>> The line: > >>> #include "bcm-nsp.dtsi" > >>> > >>> can be found in: > >>> > >>> arch/arm/boot/dts/bcm988312hr.dts > >>> arch/arm/boot/dts/bcm958625hr.dts > >>> arch/arm/boot/dts/bcm958622hr.dts > >>> arch/arm/boot/dts/bcm958625k.dts > >>> arch/arm/boot/dts/bcm958522er.dts > >>> arch/arm/boot/dts/bcm958525er.dts > >>> arch/arm/boot/dts/bcm958623hr.dts > >>> arch/arm/boot/dts/bcm958525xmc.dts > >>> > >>> > >>> The pattern for the other DTS files that include this seems to be to > >>> overwrite the compatible locally in bcm958522er.dts, like this: > >>> > >>> &srab { > >>> compatible = "brcm,bcm58522-srab", "brcm,nsp-srab"; > >>> }; > >>> > >>> Is there a reason why you are choosing to put an SoC specific compatible > >>> in the common bcm-nsp.dtsi? > >> > >> It is necessary to silence the warnings provided in the commit message > >> even when the srab node is disabled, since the dt_binding_check rule > >> will check all of the nodes matching the pattern. If there is a better > >> way to do this, I would gladly do it differently. > >> -- > >> Florian > > > > I am still not getting it. The exact 3 lines from above will not change > > the "status" property from "disabled" to "okay", so I don't understand > > why it matters whether it's enabled or not. The dt_binding_check error > > isn't in the DTSI, it's in bcm958522er.dts. All that needs to be done is > > that the bcm958522er.dts needs to override the compatible from the DTSI > > and only the compatible, I believe. With no occurrence of an incomplete > > list of compatibles in any final DTS, the dt_binding_check should not > > complain about that single occurrence in the DTSI as far as I know (and > > I did not test this). > > There is not a switch being enabled in > arch/arm/boot/dts/bcm958522er.dts, so sure, I could add the 3 lines you > quote above and that would silence the warning, but that does not scale > at all across DTS files including bcm5301x.dtsi for instance, it sort of > does for those including bcm-nsp.dtsi. > -- > Florian I see only bcm47081.dtsi and bcm4708.dtsi to include bcm5301x.dtsi, and you did point out in the other email that the BCM4708* SoCs always contain a BCM53010 switch, so that would make the other patch not wrong. Either way, it's up to you and Rafal.
On 11/10/2020 2:13 PM, Florian Fainelli wrote: > On 11/10/20 2:12 PM, Vladimir Oltean wrote: >> On Mon, Nov 09, 2020 at 07:31:08PM -0800, Florian Fainelli wrote: >>> Provide an empty 'ports' container node with the correct #address-cells >>> and #size-cells properties. This silences the following warning: >>> >>> arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dt.yaml: >>> ethernet-switch@18007000: 'oneOf' conditional failed, one must be fixed: >>> 'ports' is a required property >>> 'ethernet-ports' is a required property >>> From schema: >>> Documentation/devicetree/bindings/net/dsa/b53.yaml >>> >>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>> --- >>> arch/arm/boot/dts/bcm5301x.dtsi | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi >>> index 807580dd89f5..89993a8a6765 100644 >>> --- a/arch/arm/boot/dts/bcm5301x.dtsi >>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi >>> @@ -489,6 +489,10 @@ srab: ethernet-switch@18007000 { >>> status = "disabled"; >>> >>> /* ports are defined in board DTS */ >>> + ports { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + }; >> >> This look a bit 'lone wolf' here. Not sure how much time you intend to >> spend on this, but FWIW, others prefer to declare all ports in the SoC >> DTSI with status = "disabled", and just enable the ones used per-board, >> and add labels and PHY handles also per-board. Example: fsl-ls1028a.dtsi >> and fsl-ls1028a-rdb.dts. > > That's a good suggestion, I could do that. There is quite a bit of variation between designs and how the ports are assigned and it would end up being quite verbose, so I will punt that for now.
On 11.11.2020 02:48, Florian Fainelli wrote: > On 11/10/2020 2:13 PM, Florian Fainelli wrote: >> On 11/10/20 2:12 PM, Vladimir Oltean wrote: >>> On Mon, Nov 09, 2020 at 07:31:08PM -0800, Florian Fainelli wrote: >>>> Provide an empty 'ports' container node with the correct #address-cells >>>> and #size-cells properties. This silences the following warning: >>>> >>>> arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dt.yaml: >>>> ethernet-switch@18007000: 'oneOf' conditional failed, one must be fixed: >>>> 'ports' is a required property >>>> 'ethernet-ports' is a required property >>>> From schema: >>>> Documentation/devicetree/bindings/net/dsa/b53.yaml >>>> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>> --- >>>> arch/arm/boot/dts/bcm5301x.dtsi | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi >>>> index 807580dd89f5..89993a8a6765 100644 >>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi >>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi >>>> @@ -489,6 +489,10 @@ srab: ethernet-switch@18007000 { >>>> status = "disabled"; >>>> >>>> /* ports are defined in board DTS */ >>>> + ports { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + }; >>> >>> This look a bit 'lone wolf' here. Not sure how much time you intend to >>> spend on this, but FWIW, others prefer to declare all ports in the SoC >>> DTSI with status = "disabled", and just enable the ones used per-board, >>> and add labels and PHY handles also per-board. Example: fsl-ls1028a.dtsi >>> and fsl-ls1028a-rdb.dts. >> >> That's a good suggestion, I could do that. > > There is quite a bit of variation between designs and how the ports are > assigned and it would end up being quite verbose, so I will punt that > for now. I agree with Florian, boards (vendors) use ports really randomly so pretty much every device needs that defined from the scratch.
On 10.11.2020 04:31, Florian Fainelli wrote: > Provide a default compatible string which is based on the 53010 SRAB > compatible, this allows us to have sane defaults and silences the > following warnings: > > arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dt.yaml: > ethernet-switch@18007000: compatible: 'oneOf' conditional failed, one > must be fixed: > ['brcm,bcm5301x-srab'] is too short > 'brcm,bcm5325' was expected > 'brcm,bcm53115' was expected > 'brcm,bcm53125' was expected > 'brcm,bcm53128' was expected > 'brcm,bcm5365' was expected > 'brcm,bcm5395' was expected > 'brcm,bcm5389' was expected > 'brcm,bcm5397' was expected > 'brcm,bcm5398' was expected > 'brcm,bcm11360-srab' was expected > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm53010-srab', > 'brcm,bcm53011-srab', 'brcm,bcm53012-srab', 'brcm,bcm53018-srab', > 'brcm,bcm53019-srab'] > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm11404-srab', > 'brcm,bcm11407-srab', 'brcm,bcm11409-srab', 'brcm,bcm58310-srab', > 'brcm,bcm58311-srab', 'brcm,bcm58313-srab'] > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm58522-srab', > 'brcm,bcm58523-srab', 'brcm,bcm58525-srab', 'brcm,bcm58622-srab', > 'brcm,bcm58623-srab', 'brcm,bcm58625-srab', 'brcm,bcm88312-srab'] > 'brcm,bcm5301x-srab' is not one of ['brcm,bcm3384-switch', > 'brcm,bcm6328-switch', 'brcm,bcm6368-switch'] > From schema: > Documentation/devicetree/bindings/net/dsa/b53.yaml > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > arch/arm/boot/dts/bcm5301x.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi > index ee23c0841699..807580dd89f5 100644 > --- a/arch/arm/boot/dts/bcm5301x.dtsi > +++ b/arch/arm/boot/dts/bcm5301x.dtsi > @@ -483,7 +483,7 @@ thermal: thermal@1800c2c0 { > }; > > srab: ethernet-switch@18007000 { > - compatible = "brcm,bcm5301x-srab"; > + compatible = "brcm,bcm53010-srab", "brcm,bcm5301x-srab"; I've never seen Northstar device with BCM53010, see below list. *** BCM47081 *** Buffalo WZR-600DHP2 [ 1.816948] b53_common: found switch: BCM53011, rev 2 Luxul XWR-1200 V1 [ 2.602445] b53_common: found switch: BCM53011, rev 5 TP-LINK Archer C5 V2 [ 0.606353] b53_common: found switch: BCM53011, rev 5 *** BCM4708 *** Buffalo WZR-1750DHP [ 1.961584] b53_common: found switch: BCM53011, rev 2 Netgear R6250 V1 [ 2.445594] b53_common: found switch: BCM53011, rev 2 SmartRG SR400ac [ 4.258116] b53_common: found switch: BCM53011, rev 5 *** BCM4709 *** TP-LINK Archer C9 V1 [ 0.640041] b53_common: found switch: BCM53012, rev 5 *** BCM47094 *** D-Link DIR-885L [ 1.373423] b53_common: found switch: BCM53012, rev 0 Luxul XWR-3150 V1 [ 5.893989] b53_common: found switch: BCM53012, rev 0 Luxul XAP-1610 V1 [ 0.761285] b53_common: found switch: BCM53012, rev 0