* [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property
2022-04-23 13:14 [PATCH 1/2] dt-bindings: net: dsa: marvell: Add single-chip-address property Nathan Rossi
@ 2022-04-23 13:14 ` Nathan Rossi
2022-04-23 14:07 ` Andrew Lunn
2022-05-02 22:32 ` [PATCH 1/2] dt-bindings: net: dsa: marvell: Add single-chip-address property Rob Herring
1 sibling, 1 reply; 9+ messages in thread
From: Nathan Rossi @ 2022-04-23 13:14 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: Nathan Rossi, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni
Handle the parsing and use of single chip addressing when the switch has
the single-chip-address property defined. This allows for specifying the
switch as using single chip addressing even when mdio address 0 is used
by another device on the bus. This is a feature of some switches (e.g.
the MV88E6341/MV88E6141) where the switch shares the bus only responding
to the higher 16 addresses.
Signed-off-by: Nathan Rossi <nathan@nathanrossi.com>
---
drivers/net/dsa/mv88e6xxx/smi.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mv88e6xxx/smi.c b/drivers/net/dsa/mv88e6xxx/smi.c
index a990271b74..1eb31c1563 100644
--- a/drivers/net/dsa/mv88e6xxx/smi.c
+++ b/drivers/net/dsa/mv88e6xxx/smi.c
@@ -171,9 +171,12 @@ static const struct mv88e6xxx_bus_ops mv88e6xxx_smi_indirect_ops = {
int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
struct mii_bus *bus, int sw_addr)
{
+ struct device_node *np = chip->dev->of_node;
+
if (chip->info->dual_chip)
chip->smi_ops = &mv88e6xxx_smi_dual_direct_ops;
- else if (sw_addr == 0)
+ else if (sw_addr == 0 ||
+ (np && of_property_read_bool(np, "single-chip-address")))
chip->smi_ops = &mv88e6xxx_smi_direct_ops;
else if (chip->info->multi_chip)
chip->smi_ops = &mv88e6xxx_smi_indirect_ops;
---
2.35.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 1/2] dt-bindings: net: dsa: marvell: Add single-chip-address property
@ 2022-04-23 13:14 Nathan Rossi
2022-04-23 13:14 ` [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property Nathan Rossi
2022-05-02 22:32 ` [PATCH 1/2] dt-bindings: net: dsa: marvell: Add single-chip-address property Rob Herring
0 siblings, 2 replies; 9+ messages in thread
From: Nathan Rossi @ 2022-04-23 13:14 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: Nathan Rossi, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski
Some Marvell DSA devices can be accessed in a single chip addressing
mode. This is currently configured by setting the address of the switch
to 0. However switches in this configuration do not respond to address
0, only responding to higher addresses (fixed addressed based on the
switch model) for the individual ports/etc. This is a feature to allow
for other phys to exist on the same mdio bus.
This change defines a 'single-chip-address' property in order to
explicitly define that the chip is accessed in this mode. This allows
for a switch to have an address defined other than 0, so that address
0 can be used for another mdio device.
Signed-off-by: Nathan Rossi <nathan@nathanrossi.com>
---
Documentation/devicetree/bindings/net/dsa/marvell.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt
index 2363b41241..5c7304274c 100644
--- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
+++ b/Documentation/devicetree/bindings/net/dsa/marvell.txt
@@ -46,6 +46,8 @@ Optional properties:
- mdio? : Container of PHYs and devices on the external MDIO
bus. The node must contains a compatible string of
"marvell,mv88e6xxx-mdio-external"
+- single-chip-address : Device is configured to use single chip addressing
+ mode.
Example:
---
2.35.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property
2022-04-23 13:14 ` [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property Nathan Rossi
@ 2022-04-23 14:07 ` Andrew Lunn
2022-04-23 14:41 ` Nathan Rossi
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Lunn @ 2022-04-23 14:07 UTC (permalink / raw)
To: Nathan Rossi
Cc: netdev, linux-kernel, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni
On Sat, Apr 23, 2022 at 01:14:27PM +0000, Nathan Rossi wrote:
> Handle the parsing and use of single chip addressing when the switch has
> the single-chip-address property defined. This allows for specifying the
> switch as using single chip addressing even when mdio address 0 is used
> by another device on the bus. This is a feature of some switches (e.g.
> the MV88E6341/MV88E6141) where the switch shares the bus only responding
> to the higher 16 addresses.
Hi Nathan
I think i'm missing something in this explanation:
smi.c says:
/* The switch ADDR[4:1] configuration pins define the chip SMI device address
* (ADDR[0] is always zero, thus only even SMI addresses can be strapped).
*
* When ADDR is all zero, the chip uses Single-chip Addressing Mode, assuming it
* is the only device connected to the SMI master. In this mode it responds to
* all 32 possible SMI addresses, and thus maps directly the internal devices.
*
* When ADDR is non-zero, the chip uses Multi-chip Addressing Mode, allowing
* multiple devices to share the SMI interface. In this mode it responds to only
* 2 registers, used to indirectly access the internal SMI devices.
*
* Some chips use a different scheme: Only the ADDR4 pin is used for
* configuration, and the device responds to 16 of the 32 SMI
* addresses, allowing two to coexist on the same SMI interface.
*/
So if ADDR = 0, it takes up the whole bus. And in this case reg = 0.
If ADDR != 0, it is in multi chip mode, and DT reg = ADDR.
int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
struct mii_bus *bus, int sw_addr)
{
if (chip->info->dual_chip)
chip->smi_ops = &mv88e6xxx_smi_dual_direct_ops;
else if (sw_addr == 0)
chip->smi_ops = &mv88e6xxx_smi_direct_ops;
else if (chip->info->multi_chip)
chip->smi_ops = &mv88e6xxx_smi_indirect_ops;
else
return -EINVAL;
This seems to implement what is above. smi_direct_ops == whole bus,
smi_indirect_ops == multi-chip mode.
In what situation do you see this not working? What device are you
using, what does you DT look like, and what at the ADDR value?
Thanks
Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property
2022-04-23 14:07 ` Andrew Lunn
@ 2022-04-23 14:41 ` Nathan Rossi
2022-04-23 15:16 ` Marek Behún
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Nathan Rossi @ 2022-04-23 14:41 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, linux-kernel, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni
On Sun, 24 Apr 2022 at 00:07, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sat, Apr 23, 2022 at 01:14:27PM +0000, Nathan Rossi wrote:
> > Handle the parsing and use of single chip addressing when the switch has
> > the single-chip-address property defined. This allows for specifying the
> > switch as using single chip addressing even when mdio address 0 is used
> > by another device on the bus. This is a feature of some switches (e.g.
> > the MV88E6341/MV88E6141) where the switch shares the bus only responding
> > to the higher 16 addresses.
>
> Hi Nathan
>
> I think i'm missing something in this explanation:
>
> smi.c says:
>
> /* The switch ADDR[4:1] configuration pins define the chip SMI device address
> * (ADDR[0] is always zero, thus only even SMI addresses can be strapped).
> *
> * When ADDR is all zero, the chip uses Single-chip Addressing Mode, assuming it
> * is the only device connected to the SMI master. In this mode it responds to
> * all 32 possible SMI addresses, and thus maps directly the internal devices.
> *
> * When ADDR is non-zero, the chip uses Multi-chip Addressing Mode, allowing
> * multiple devices to share the SMI interface. In this mode it responds to only
> * 2 registers, used to indirectly access the internal SMI devices.
> *
> * Some chips use a different scheme: Only the ADDR4 pin is used for
> * configuration, and the device responds to 16 of the 32 SMI
> * addresses, allowing two to coexist on the same SMI interface.
> */
>
> So if ADDR = 0, it takes up the whole bus. And in this case reg = 0.
> If ADDR != 0, it is in multi chip mode, and DT reg = ADDR.
>
> int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
> struct mii_bus *bus, int sw_addr)
> {
> if (chip->info->dual_chip)
> chip->smi_ops = &mv88e6xxx_smi_dual_direct_ops;
> else if (sw_addr == 0)
> chip->smi_ops = &mv88e6xxx_smi_direct_ops;
> else if (chip->info->multi_chip)
> chip->smi_ops = &mv88e6xxx_smi_indirect_ops;
> else
> return -EINVAL;
>
> This seems to implement what is above. smi_direct_ops == whole bus,
> smi_indirect_ops == multi-chip mode.
>
> In what situation do you see this not working? What device are you
> using, what does you DT look like, and what at the ADDR value?
The device I am using is the MV88E6141, it follows the second scheme
such that it only responds to the upper 16 of the 32 SMI addresses in
single chip addressing mode. I am able to define the switch at address
0, and everything works. However in the device I am using (Netgate
SG-3100) the ethernet phys for the non switch ethernet interfaces are
also on the same mdio bus as the switch. One of those phys is
configured with address 0. Defining both the ethernet-phy and switch
as address 0 does not work.
The device tree I have looks like:
&mdio {
status = "okay";
pinctrl-0 = <&mdio_pins>;
pinctrl-names = "default";
phy0: ethernet-phy@0 {
status = "okay";
reg = <0>;
};
phy1: ethernet-phy@1 {
status = "okay";
reg = <1>;
};
switch0: switch0@16 {
compatible = "marvell,mv88e6141", "marvell,mv88e6085";
single-chip-address;
reg = <16>; /* first port in single address mode */
dsa,member = <0 0>;
status = "okay";
... ports/mdio nodes ...
};
};
Thanks,
Nathan
>
> Thanks
> Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property
2022-04-23 14:41 ` Nathan Rossi
@ 2022-04-23 15:16 ` Marek Behún
2022-04-23 15:18 ` Marek Behún
2022-04-23 15:41 ` Andrew Lunn
2 siblings, 0 replies; 9+ messages in thread
From: Marek Behún @ 2022-04-23 15:16 UTC (permalink / raw)
To: Nathan Rossi
Cc: Andrew Lunn, netdev, linux-kernel, Vivien Didelot,
Florian Fainelli, Vladimir Oltean, David S. Miller,
Jakub Kicinski, Paolo Abeni
> switch0: switch0@16 {
> compatible = "marvell,mv88e6141", "marvell,mv88e6085";
Not relevant to your problem, but the node name should be switch@16,
not switch0@16. The 0 is redundant and should not be there.
Marek
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property
2022-04-23 14:41 ` Nathan Rossi
2022-04-23 15:16 ` Marek Behún
@ 2022-04-23 15:18 ` Marek Behún
2022-04-23 15:41 ` Andrew Lunn
2 siblings, 0 replies; 9+ messages in thread
From: Marek Behún @ 2022-04-23 15:18 UTC (permalink / raw)
To: Nathan Rossi
Cc: Andrew Lunn, netdev, linux-kernel, Vivien Didelot,
Florian Fainelli, Vladimir Oltean, David S. Miller,
Jakub Kicinski, Paolo Abeni
On Sun, 24 Apr 2022 00:41:22 +1000
Nathan Rossi <nathan@nathanrossi.com> wrote:
> On Sun, 24 Apr 2022 at 00:07, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Sat, Apr 23, 2022 at 01:14:27PM +0000, Nathan Rossi wrote:
> > > Handle the parsing and use of single chip addressing when the switch has
> > > the single-chip-address property defined. This allows for specifying the
> > > switch as using single chip addressing even when mdio address 0 is used
> > > by another device on the bus. This is a feature of some switches (e.g.
> > > the MV88E6341/MV88E6141) where the switch shares the bus only responding
> > > to the higher 16 addresses.
> >
> > Hi Nathan
> >
> > I think i'm missing something in this explanation:
> >
> > smi.c says:
> >
> > /* The switch ADDR[4:1] configuration pins define the chip SMI device address
> > * (ADDR[0] is always zero, thus only even SMI addresses can be strapped).
> > *
> > * When ADDR is all zero, the chip uses Single-chip Addressing Mode, assuming it
> > * is the only device connected to the SMI master. In this mode it responds to
> > * all 32 possible SMI addresses, and thus maps directly the internal devices.
> > *
> > * When ADDR is non-zero, the chip uses Multi-chip Addressing Mode, allowing
> > * multiple devices to share the SMI interface. In this mode it responds to only
> > * 2 registers, used to indirectly access the internal SMI devices.
> > *
> > * Some chips use a different scheme: Only the ADDR4 pin is used for
> > * configuration, and the device responds to 16 of the 32 SMI
> > * addresses, allowing two to coexist on the same SMI interface.
> > */
> >
> > So if ADDR = 0, it takes up the whole bus. And in this case reg = 0.
> > If ADDR != 0, it is in multi chip mode, and DT reg = ADDR.
> >
> > int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
> > struct mii_bus *bus, int sw_addr)
> > {
> > if (chip->info->dual_chip)
> > chip->smi_ops = &mv88e6xxx_smi_dual_direct_ops;
> > else if (sw_addr == 0)
> > chip->smi_ops = &mv88e6xxx_smi_direct_ops;
> > else if (chip->info->multi_chip)
> > chip->smi_ops = &mv88e6xxx_smi_indirect_ops;
> > else
> > return -EINVAL;
> >
> > This seems to implement what is above. smi_direct_ops == whole bus,
> > smi_indirect_ops == multi-chip mode.
> >
> > In what situation do you see this not working? What device are you
> > using, what does you DT look like, and what at the ADDR value?
>
> The device I am using is the MV88E6141, it follows the second scheme
> such that it only responds to the upper 16 of the 32 SMI addresses in
> single chip addressing mode. I am able to define the switch at address
> 0, and everything works. However in the device I am using (Netgate
> SG-3100) the ethernet phys for the non switch ethernet interfaces are
> also on the same mdio bus as the switch. One of those phys is
> configured with address 0. Defining both the ethernet-phy and switch
> as address 0 does not work.
This makes the need of new property reasonable. You can add my
Acked-by: Marek Behún <kabel@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property
2022-04-23 14:41 ` Nathan Rossi
2022-04-23 15:16 ` Marek Behún
2022-04-23 15:18 ` Marek Behún
@ 2022-04-23 15:41 ` Andrew Lunn
2022-04-24 12:57 ` Nathan Rossi
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Lunn @ 2022-04-23 15:41 UTC (permalink / raw)
To: Nathan Rossi
Cc: netdev, linux-kernel, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni
On Sun, Apr 24, 2022 at 12:41:22AM +1000, Nathan Rossi wrote:
> On Sun, 24 Apr 2022 at 00:07, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Sat, Apr 23, 2022 at 01:14:27PM +0000, Nathan Rossi wrote:
> > > Handle the parsing and use of single chip addressing when the switch has
> > > the single-chip-address property defined. This allows for specifying the
> > > switch as using single chip addressing even when mdio address 0 is used
> > > by another device on the bus. This is a feature of some switches (e.g.
> > > the MV88E6341/MV88E6141) where the switch shares the bus only responding
> > > to the higher 16 addresses.
> >
> > Hi Nathan
> >
> > I think i'm missing something in this explanation:
> >
> > smi.c says:
> >
> > /* The switch ADDR[4:1] configuration pins define the chip SMI device address
> > * (ADDR[0] is always zero, thus only even SMI addresses can be strapped).
> > *
> > * When ADDR is all zero, the chip uses Single-chip Addressing Mode, assuming it
> > * is the only device connected to the SMI master. In this mode it responds to
> > * all 32 possible SMI addresses, and thus maps directly the internal devices.
> > *
> > * When ADDR is non-zero, the chip uses Multi-chip Addressing Mode, allowing
> > * multiple devices to share the SMI interface. In this mode it responds to only
> > * 2 registers, used to indirectly access the internal SMI devices.
> > *
> > * Some chips use a different scheme: Only the ADDR4 pin is used for
> > * configuration, and the device responds to 16 of the 32 SMI
> > * addresses, allowing two to coexist on the same SMI interface.
> > */
> >
> > So if ADDR = 0, it takes up the whole bus. And in this case reg = 0.
> > If ADDR != 0, it is in multi chip mode, and DT reg = ADDR.
> >
> > int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
> > struct mii_bus *bus, int sw_addr)
> > {
> > if (chip->info->dual_chip)
> > chip->smi_ops = &mv88e6xxx_smi_dual_direct_ops;
> > else if (sw_addr == 0)
> > chip->smi_ops = &mv88e6xxx_smi_direct_ops;
> > else if (chip->info->multi_chip)
> > chip->smi_ops = &mv88e6xxx_smi_indirect_ops;
> > else
> > return -EINVAL;
> >
> > This seems to implement what is above. smi_direct_ops == whole bus,
> > smi_indirect_ops == multi-chip mode.
> >
> > In what situation do you see this not working? What device are you
> > using, what does you DT look like, and what at the ADDR value?
>
> The device I am using is the MV88E6141, it follows the second scheme
> such that it only responds to the upper 16 of the 32 SMI addresses in
> single chip addressing mode. I am able to define the switch at address
> 0, and everything works. However in the device I am using (Netgate
> SG-3100) the ethernet phys for the non switch ethernet interfaces are
> also on the same mdio bus as the switch. One of those phys is
> configured with address 0. Defining both the ethernet-phy and switch
> as address 0 does not work.
>
> The device tree I have looks like:
>
> &mdio {
> status = "okay";
> pinctrl-0 = <&mdio_pins>;
> pinctrl-names = "default";
>
> phy0: ethernet-phy@0 {
> status = "okay";
> reg = <0>;
> };
>
> phy1: ethernet-phy@1 {
> status = "okay";
> reg = <1>;
> };
So normally, we would have
switch0: switch0@16 {
compatible = "marvell,mv88e6141", "marvell,mv88e6085";
single-chip-address;
reg = <0>;
dsa,member = <0 0>;
status = "okay";
and then i guess you are seeing mdiobus_register_device() returning
-EBUSY because the PHY is also at address 0?
This is what is missing from your explanation. It is always better to
have more than less in the commit message.
So the chip is using addresses 0x10-0x1f, but in order to probe, you
need to put reg = 0, taking up slot 0, clashing with the PHY. Ideally
we want to take up one of the slots in the range 0x10-0x1f. reg=16 on
its own indicates multi-chip mode and the device is using address 16.
O.K, a bit more digging into the datasheet:
For multi-chip mode, for the 6341 family,
The SMI address that is used is determined by the ADDR[3:0]
configuration pins. ADDR[4] must be zero to select the device.
So it can only take the address range 0-f, since ADDR[4] == 0. So 16
is not even a valid multi-chip address. But it is valid for some other
chips.
So your DT property is says, ignore reg, i really am in single chip
mode.
This appears to be a general problem for any device with
.port_base_addr = 0x10.
I'm wondering if a better solution to this is special case
reg=16. First try mv88e6xxx_detect() in single chip mode. That will
read register 3. A read should be safe. If we get back a valid ID for
a switch, keep with single chip mode. Otherwise swap to multi-chip
mode. A multi-chip mv88e6xxx_detect() is more dangerous, because that
involves writes.
Looking at the existing DTs, there are only two using multi-chip mode
with reg=16:
arm/boot/dts/armada-370-rd.dts- reg = <0x10>;
arm/boot/dts/kirkwood-linksys-viper.dts- reg = <16>;
And i happen to have an armada-370-rd :-)
Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property
2022-04-23 15:41 ` Andrew Lunn
@ 2022-04-24 12:57 ` Nathan Rossi
0 siblings, 0 replies; 9+ messages in thread
From: Nathan Rossi @ 2022-04-24 12:57 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, linux-kernel, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S. Miller, Jakub Kicinski, Paolo Abeni
On Sun, 24 Apr 2022 at 01:41, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Apr 24, 2022 at 12:41:22AM +1000, Nathan Rossi wrote:
> > On Sun, 24 Apr 2022 at 00:07, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Sat, Apr 23, 2022 at 01:14:27PM +0000, Nathan Rossi wrote:
> > > > Handle the parsing and use of single chip addressing when the switch has
> > > > the single-chip-address property defined. This allows for specifying the
> > > > switch as using single chip addressing even when mdio address 0 is used
> > > > by another device on the bus. This is a feature of some switches (e.g.
> > > > the MV88E6341/MV88E6141) where the switch shares the bus only responding
> > > > to the higher 16 addresses.
> > >
> > > Hi Nathan
> > >
> > > I think i'm missing something in this explanation:
> > >
> > > smi.c says:
> > >
> > > /* The switch ADDR[4:1] configuration pins define the chip SMI device address
> > > * (ADDR[0] is always zero, thus only even SMI addresses can be strapped).
> > > *
> > > * When ADDR is all zero, the chip uses Single-chip Addressing Mode, assuming it
> > > * is the only device connected to the SMI master. In this mode it responds to
> > > * all 32 possible SMI addresses, and thus maps directly the internal devices.
> > > *
> > > * When ADDR is non-zero, the chip uses Multi-chip Addressing Mode, allowing
> > > * multiple devices to share the SMI interface. In this mode it responds to only
> > > * 2 registers, used to indirectly access the internal SMI devices.
> > > *
> > > * Some chips use a different scheme: Only the ADDR4 pin is used for
> > > * configuration, and the device responds to 16 of the 32 SMI
> > > * addresses, allowing two to coexist on the same SMI interface.
> > > */
> > >
> > > So if ADDR = 0, it takes up the whole bus. And in this case reg = 0.
> > > If ADDR != 0, it is in multi chip mode, and DT reg = ADDR.
> > >
> > > int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
> > > struct mii_bus *bus, int sw_addr)
> > > {
> > > if (chip->info->dual_chip)
> > > chip->smi_ops = &mv88e6xxx_smi_dual_direct_ops;
> > > else if (sw_addr == 0)
> > > chip->smi_ops = &mv88e6xxx_smi_direct_ops;
> > > else if (chip->info->multi_chip)
> > > chip->smi_ops = &mv88e6xxx_smi_indirect_ops;
> > > else
> > > return -EINVAL;
> > >
> > > This seems to implement what is above. smi_direct_ops == whole bus,
> > > smi_indirect_ops == multi-chip mode.
> > >
> > > In what situation do you see this not working? What device are you
> > > using, what does you DT look like, and what at the ADDR value?
> >
> > The device I am using is the MV88E6141, it follows the second scheme
> > such that it only responds to the upper 16 of the 32 SMI addresses in
> > single chip addressing mode. I am able to define the switch at address
> > 0, and everything works. However in the device I am using (Netgate
> > SG-3100) the ethernet phys for the non switch ethernet interfaces are
> > also on the same mdio bus as the switch. One of those phys is
> > configured with address 0. Defining both the ethernet-phy and switch
> > as address 0 does not work.
> >
> > The device tree I have looks like:
> >
> > &mdio {
> > status = "okay";
> > pinctrl-0 = <&mdio_pins>;
> > pinctrl-names = "default";
> >
> > phy0: ethernet-phy@0 {
> > status = "okay";
> > reg = <0>;
> > };
> >
> > phy1: ethernet-phy@1 {
> > status = "okay";
> > reg = <1>;
> > };
>
> So normally, we would have
>
>
> switch0: switch0@16 {
> compatible = "marvell,mv88e6141", "marvell,mv88e6085";
> single-chip-address;
> reg = <0>;
> dsa,member = <0 0>;
> status = "okay";
>
> and then i guess you are seeing mdiobus_register_device() returning
> -EBUSY because the PHY is also at address 0?
Correct, that is the issue I am trying to solve here.
>
> This is what is missing from your explanation. It is always better to
> have more than less in the commit message.
>
> So the chip is using addresses 0x10-0x1f, but in order to probe, you
> need to put reg = 0, taking up slot 0, clashing with the PHY. Ideally
> we want to take up one of the slots in the range 0x10-0x1f. reg=16 on
> its own indicates multi-chip mode and the device is using address 16.
>
> O.K, a bit more digging into the datasheet:
>
> For multi-chip mode, for the 6341 family,
>
> The SMI address that is used is determined by the ADDR[3:0]
> configuration pins. ADDR[4] must be zero to select the device.
>
> So it can only take the address range 0-f, since ADDR[4] == 0. So 16
> is not even a valid multi-chip address. But it is valid for some other
> chips.
>
> So your DT property is says, ignore reg, i really am in single chip
> mode.
>
> This appears to be a general problem for any device with
> .port_base_addr = 0x10.
I had initially thought of using the port_base_addr along with setting
up an of_match for the 6141 to provide compat_info which smi init
could use.
>
> I'm wondering if a better solution to this is special case
> reg=16. First try mv88e6xxx_detect() in single chip mode. That will
> read register 3. A read should be safe. If we get back a valid ID for
> a switch, keep with single chip mode. Otherwise swap to multi-chip
> mode. A multi-chip mv88e6xxx_detect() is more dangerous, because that
> involves writes.
I tested this idea and have sent out a patch for it
(https://lore.kernel.org/netdev/20220424125451.295435-1-nathan@nathanrossi.com/).
It works correctly for the single chip detection case and safely falls
through on other multi-chip addresses. It would be great if you could
test this on the armada 370 rd board with reg=16.
However just a side note, I had to move the reset gpio setup to occur
before the smi init. Interestingly I am not sure if there was a reason
for the reset to be unconfigured before setting up smi, it seems that
might cause issues with the multi-chip smi init ops?
Thanks,
Nathan
>
> Looking at the existing DTs, there are only two using multi-chip mode
> with reg=16:
>
> arm/boot/dts/armada-370-rd.dts- reg = <0x10>;
> arm/boot/dts/kirkwood-linksys-viper.dts- reg = <16>;
>
> And i happen to have an armada-370-rd :-)
>
> Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: net: dsa: marvell: Add single-chip-address property
2022-04-23 13:14 [PATCH 1/2] dt-bindings: net: dsa: marvell: Add single-chip-address property Nathan Rossi
2022-04-23 13:14 ` [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property Nathan Rossi
@ 2022-05-02 22:32 ` Rob Herring
1 sibling, 0 replies; 9+ messages in thread
From: Rob Herring @ 2022-05-02 22:32 UTC (permalink / raw)
To: Nathan Rossi
Cc: netdev, devicetree, linux-kernel, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean, David S. Miller,
Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski
On Sat, Apr 23, 2022 at 01:14:27PM +0000, Nathan Rossi wrote:
> Some Marvell DSA devices can be accessed in a single chip addressing
> mode. This is currently configured by setting the address of the switch
> to 0. However switches in this configuration do not respond to address
> 0, only responding to higher addresses (fixed addressed based on the
> switch model) for the individual ports/etc. This is a feature to allow
> for other phys to exist on the same mdio bus.
>
> This change defines a 'single-chip-address' property in order to
> explicitly define that the chip is accessed in this mode. This allows
> for a switch to have an address defined other than 0, so that address
> 0 can be used for another mdio device.
>
> Signed-off-by: Nathan Rossi <nathan@nathanrossi.com>
> ---
> Documentation/devicetree/bindings/net/dsa/marvell.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt
> index 2363b41241..5c7304274c 100644
> --- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/marvell.txt
> @@ -46,6 +46,8 @@ Optional properties:
> - mdio? : Container of PHYs and devices on the external MDIO
> bus. The node must contains a compatible string of
> "marvell,mv88e6xxx-mdio-external"
> +- single-chip-address : Device is configured to use single chip addressing
> + mode.
Doesn't sound like a common feature, it needs a vendor prefix.
Some of the commit message explanation of what 'single chip addressing'
is is needed here.
Rob
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-02 22:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-04-23 13:14 [PATCH 1/2] dt-bindings: net: dsa: marvell: Add single-chip-address property Nathan Rossi
2022-04-23 13:14 ` [PATCH 2/2] net: dsa: mv88e6xxx: Handle single-chip-address OF property Nathan Rossi
2022-04-23 14:07 ` Andrew Lunn
2022-04-23 14:41 ` Nathan Rossi
2022-04-23 15:16 ` Marek Behún
2022-04-23 15:18 ` Marek Behún
2022-04-23 15:41 ` Andrew Lunn
2022-04-24 12:57 ` Nathan Rossi
2022-05-02 22:32 ` [PATCH 1/2] dt-bindings: net: dsa: marvell: Add single-chip-address property Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).