[PATCH 1/2] net: dsa: felix: allow the device to be disabled

[PATCH 1/2] net: dsa: felix: allow the device to be disabled

[Michael Walle]

If there is no specific configuration of the felix switch in the device
tree, but only the default configuration (ie. given by the SoCs dtsi
file), the probe fails because no CPU port has been set. On the other
hand you cannot set a default CPU port because that depends on the
actual board using the switch.

[    2.701300] DSA: tree 0 has no CPU port
[    2.705167] mscc_felix 0000:00:00.5: Failed to register DSA switch: -22
[    2.711844] mscc_felix: probe of 0000:00:00.5 failed with error -22

Thus let the device tree disable this device entirely, like it is also
done with the enetc driver of the same SoC.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/dsa/ocelot/felix.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 69546383a382..531c7710063f 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -699,6 +699,11 @@ static int felix_pci_probe(struct pci_dev *pdev,
 	struct felix *felix;
 	int err;
 
+	if (pdev->dev.of_node && !of_device_is_available(pdev->dev.of_node)) {
+		dev_info(&pdev->dev, "device is disabled, skipping\n");
+		return -ENODEV;
+	}
+
 	err = pci_enable_device(pdev);
 	if (err) {
 		dev_err(&pdev->dev, "device enable failed\n");
-- 
2.20.1

[PATCH 2/2] arm64: dts: ls1028a: disable the felix switch by default

[Michael Walle]

Disable the felix switch by default and enable it per board which are
actually using it.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts  | 4 ++++
 .../boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts      | 4 ++++
 arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts             | 4 ++++
 arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi                | 3 ++-
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
index a83a176cf18a..d4ca12b140b4 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
@@ -63,6 +63,10 @@
 	};
 };
 
+&mscc_felix {
+	status = "okay";
+};
+
 &mscc_felix_port0 {
 	label = "swp0";
 	managed = "in-band-status";
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
index 0a34ff682027..901b5b161def 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
@@ -48,6 +48,10 @@
 	status = "okay";
 };
 
+&mscc_felix {
+	status = "okay";
+};
+
 &mscc_felix_port0 {
 	label = "gbe0";
 	phy-handle = <&phy0>;
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
index 0d27b5667b8c..8294d364112e 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
@@ -228,6 +228,10 @@
 	status = "okay";
 };
 
+&mscc_felix {
+	status = "okay";
+};
+
 &mscc_felix_port0 {
 	label = "swp0";
 	managed = "in-band-status";
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
index c09279379723..70a10268bb83 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
@@ -933,10 +933,11 @@
 				fsl,extts-fifo;
 			};
 
-			ethernet-switch@0,5 {
+			mscc_felix: ethernet-switch@0,5 {
 				reg = <0x000500 0 0 0 0>;
 				/* IEP INT_B */
 				interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>;
+				status = "disabled";
 
 				ports {
 					#address-cells = <1>;
-- 
2.20.1

Re: [PATCH 2/2] arm64: dts: ls1028a: disable the felix switch by default

[Michael Walle]

Am 2020-03-12 17:43, schrieb Michael Walle:
> Disable the felix switch by default and enable it per board which are
> actually using it.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

I forgot to mention that this patch depends on the following series:
   
https://lore.kernel.org/linux-devicetree/20200311074929.19569-1-michael@walle.cc/

Sorry,
-michael

> ---
>  .../boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts  | 4 ++++
>  .../boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts      | 4 ++++
>  arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts             | 4 ++++
>  arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi                | 3 ++-
>  4 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git
> a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
> b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
> index a83a176cf18a..d4ca12b140b4 100644
> --- 
> a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
> +++ 
> b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
> @@ -63,6 +63,10 @@
>  	};
>  };
> 
> +&mscc_felix {
> +	status = "okay";
> +};
> +
>  &mscc_felix_port0 {
>  	label = "swp0";
>  	managed = "in-band-status";
> diff --git
> a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
> b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
> index 0a34ff682027..901b5b161def 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
> @@ -48,6 +48,10 @@
>  	status = "okay";
>  };
> 
> +&mscc_felix {
> +	status = "okay";
> +};
> +
>  &mscc_felix_port0 {
>  	label = "gbe0";
>  	phy-handle = <&phy0>;
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
> b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
> index 0d27b5667b8c..8294d364112e 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
> @@ -228,6 +228,10 @@
>  	status = "okay";
>  };
> 
> +&mscc_felix {
> +	status = "okay";
> +};
> +
>  &mscc_felix_port0 {
>  	label = "swp0";
>  	managed = "in-band-status";
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> index c09279379723..70a10268bb83 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> @@ -933,10 +933,11 @@
>  				fsl,extts-fifo;
>  			};
> 
> -			ethernet-switch@0,5 {
> +			mscc_felix: ethernet-switch@0,5 {
>  				reg = <0x000500 0 0 0 0>;
>  				/* IEP INT_B */
>  				interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> 
>  				ports {
>  					#address-cells = <1>;

Re: [PATCH 1/2] net: dsa: felix: allow the device to be disabled

[Vladimir Oltean]

On Thu, 12 Mar 2020 at 18:44, Michael Walle <michael@walle.cc> wrote:
>
> If there is no specific configuration of the felix switch in the device
> tree, but only the default configuration (ie. given by the SoCs dtsi
> file), the probe fails because no CPU port has been set. On the other
> hand you cannot set a default CPU port because that depends on the
> actual board using the switch.
>
> [    2.701300] DSA: tree 0 has no CPU port
> [    2.705167] mscc_felix 0000:00:00.5: Failed to register DSA switch: -22
> [    2.711844] mscc_felix: probe of 0000:00:00.5 failed with error -22
>
> Thus let the device tree disable this device entirely, like it is also
> done with the enetc driver of the same SoC.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/net/dsa/ocelot/felix.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index 69546383a382..531c7710063f 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -699,6 +699,11 @@ static int felix_pci_probe(struct pci_dev *pdev,
>         struct felix *felix;
>         int err;
>
> +       if (pdev->dev.of_node && !of_device_is_available(pdev->dev.of_node)) {
> +               dev_info(&pdev->dev, "device is disabled, skipping\n");
> +               return -ENODEV;
> +       }
> +

IMHO since DSA is already dependent on device tree for PHY bindings,
it would make more sense to move this there:

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index e7c30b472034..f7ca01d93928 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -878,7 +878,7 @@ static int dsa_switch_probe(struct dsa_switch *ds)
        if (!ds->num_ports)
                return -EINVAL;

-       if (np) {
+       if (np && of_device_is_available(np)) {
                err = dsa_switch_parse_of(ds, np);
                if (err)
                        dsa_switch_release_ports(ds);

so that we could enforce more uniform behavior across device drivers.
Then you might want to make felix shut up:

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 35124ef7e75b..fbd17fa94bff 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -712,10 +712,8 @@ static int felix_pci_probe(struct pci_dev *pdev,
        felix->ds = ds;

        err = dsa_register_switch(ds);
-       if (err) {
-               dev_err(&pdev->dev, "Failed to register DSA switch: %d\n", err);
+       if (err)
                goto err_register_ds;
-       }

        return 0;

This has the disadvantage of not printing the "nice" "device is
disabled, skipping" message (useless in my opinion), but the advantage
of also shutting up on -EPROBE_DEFER.

>         err = pci_enable_device(pdev);
>         if (err) {
>                 dev_err(&pdev->dev, "device enable failed\n");
> --
> 2.20.1
>

Thanks,
-Vladimir

Re: [PATCH 1/2] net: dsa: felix: allow the device to be disabled

[Florian Fainelli]

On 3/12/20 2:35 PM, Vladimir Oltean wrote:
> On Thu, 12 Mar 2020 at 18:44, Michael Walle <michael@walle.cc> wrote:
>>
>> If there is no specific configuration of the felix switch in the device
>> tree, but only the default configuration (ie. given by the SoCs dtsi
>> file), the probe fails because no CPU port has been set. On the other
>> hand you cannot set a default CPU port because that depends on the
>> actual board using the switch.
>>
>> [    2.701300] DSA: tree 0 has no CPU port
>> [    2.705167] mscc_felix 0000:00:00.5: Failed to register DSA switch: -22
>> [    2.711844] mscc_felix: probe of 0000:00:00.5 failed with error -22
>>
>> Thus let the device tree disable this device entirely, like it is also
>> done with the enetc driver of the same SoC.
>>
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/net/dsa/ocelot/felix.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
>> index 69546383a382..531c7710063f 100644
>> --- a/drivers/net/dsa/ocelot/felix.c
>> +++ b/drivers/net/dsa/ocelot/felix.c
>> @@ -699,6 +699,11 @@ static int felix_pci_probe(struct pci_dev *pdev,
>>         struct felix *felix;
>>         int err;
>>
>> +       if (pdev->dev.of_node && !of_device_is_available(pdev->dev.of_node)) {
>> +               dev_info(&pdev->dev, "device is disabled, skipping\n");
>> +               return -ENODEV;
>> +       }
>> +
> 
> IMHO since DSA is already dependent on device tree for PHY bindings,
> it would make more sense to move this there:

Michael's solution makes more sense, as this is a driver specific
problem whereby you have a pci_dev instance that is created and does not
honor the status property provided in Device Tree. If you were to look
for a proper solution it would likely be within the PCI core actually.

No other DSA switch has that problem because they use the
I2C/SPI/platform_device/GPIO/whatever entry points into the driver model.
-- 
Florian

Re: [PATCH 1/2] net: dsa: felix: allow the device to be disabled

[Vladimir Oltean]

On Thu, 12 Mar 2020 at 23:45, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 3/12/20 2:35 PM, Vladimir Oltean wrote:
> > On Thu, 12 Mar 2020 at 18:44, Michael Walle <michael@walle.cc> wrote:
> >>
> >> If there is no specific configuration of the felix switch in the device
> >> tree, but only the default configuration (ie. given by the SoCs dtsi
> >> file), the probe fails because no CPU port has been set. On the other
> >> hand you cannot set a default CPU port because that depends on the
> >> actual board using the switch.
> >>
> >> [    2.701300] DSA: tree 0 has no CPU port
> >> [    2.705167] mscc_felix 0000:00:00.5: Failed to register DSA switch: -22
> >> [    2.711844] mscc_felix: probe of 0000:00:00.5 failed with error -22
> >>
> >> Thus let the device tree disable this device entirely, like it is also
> >> done with the enetc driver of the same SoC.
> >>
> >> Signed-off-by: Michael Walle <michael@walle.cc>
> >> ---
> >>  drivers/net/dsa/ocelot/felix.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> >> index 69546383a382..531c7710063f 100644
> >> --- a/drivers/net/dsa/ocelot/felix.c
> >> +++ b/drivers/net/dsa/ocelot/felix.c
> >> @@ -699,6 +699,11 @@ static int felix_pci_probe(struct pci_dev *pdev,
> >>         struct felix *felix;
> >>         int err;
> >>
> >> +       if (pdev->dev.of_node && !of_device_is_available(pdev->dev.of_node)) {
> >> +               dev_info(&pdev->dev, "device is disabled, skipping\n");
> >> +               return -ENODEV;
> >> +       }
> >> +
> >
> > IMHO since DSA is already dependent on device tree for PHY bindings,
> > it would make more sense to move this there:
>
> Michael's solution makes more sense, as this is a driver specific
> problem whereby you have a pci_dev instance that is created and does not
> honor the status property provided in Device Tree. If you were to look
> for a proper solution it would likely be within the PCI core actually.
>
> No other DSA switch has that problem because they use the
> I2C/SPI/platform_device/GPIO/whatever entry points into the driver model.

True, my problem with doing it in the PCI core is that "the book" [0]
doesn't actually say anything about the "status" property, so this
patch might get some pushback from the PCI maintainers (but I don't
actually know):

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 512cb4312ddd..50c2b3da134a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2281,6 +2281,12 @@ static struct pci_dev *pci_scan_device(struct
pci_bus *bus, int devfn)

        pci_set_of_node(dev);

+       if (dev->dev.of_node && !of_device_is_available(dev->dev.of_node)) {
+               pci_bus_put(dev->bus);
+               kfree(dev);
+               return NULL;
+       }
+
        if (pci_setup_device(dev)) {
                pci_bus_put(dev->bus);
                kfree(dev);

OF bindings are completely optional with PCI, as you probably already
know. But there's nothing "driver" specific in disabling (i.e. not
probing) a device.

> --
>

[0] https://www.openfirmware.info/data/docs/bus.pci.pdf

Thanks,
-Vladimir

Re: [PATCH 1/2] net: dsa: felix: allow the device to be disabled

[Andrew Lunn]

e> IMHO since DSA is already dependent on device tree for PHY bindings,
> it would make more sense to move this there:

That is not really true. You can instantiate a marvell switch using a
platform device. So long any you only have C22 PHYs in a sane
configuration, it will just work. There are boards out there do this,
on x86 platforms without device tree.

	Andrew

Re: [PATCH 1/2] net: dsa: felix: allow the device to be disabled

[David Miller]

This series depends upon some devicetree tree changes, so why don't you
submit these changes there and add my:

Acked-by: David S. Miller <davem@davemloft.net>

Thank you.

Re: [PATCH 2/2] arm64: dts: ls1028a: disable the felix switch by default

[Shawn Guo]

On Thu, Mar 12, 2020 at 05:43:20PM +0100, Michael Walle wrote:
> Disable the felix switch by default and enable it per board which are
> actually using it.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Applied, thanks.

Re: [PATCH 1/2] net: dsa: felix: allow the device to be disabled

[Michael Walle]

Hi David, Hi Shawnguo,

Am 2020-03-15 04:53, schrieb David Miller:
> This series depends upon some devicetree tree changes, so why don't you
> submit these changes there and add my:
> 
> Acked-by: David S. Miller <davem@davemloft.net>
> 
> Thank you.

Patch 2/2 is already in linux-next, picked by Shawnguo. Who will
pick 1/2? I guess it doesn't matter through which tree it will go.

-michael