* U-Boot DSA driver for microchip KSZ9477 suggestions needed
@ 2021-06-21 23:10 Tim Harvey
2021-06-21 23:49 ` Vladimir Oltean
0 siblings, 1 reply; 8+ messages in thread
From: Tim Harvey @ 2021-06-21 23:10 UTC (permalink / raw)
To: vladimir.oltean, alexandru.marginean, claudiu.manoil,
Michael Walle, u-boot
Greetings,
I've written a U-Boot phy driver for a Microchip KSZ9477 ethernet
switch that I submitted some time ago as an RFC [1] which simply
enables the ports in the dt and puts them in forwarding mode with link
detect. However I think this would be much better suited as a DSA
driver now that UCLASS_DSA exists.
The KSZ9477 switch is used in the Gateworks GW7901 IMX8M Mini based
board and is hooked up via I2C (instead of MDIO) so the relevant dt
(already in Mainline Linux) looks like this:
/ {
aliases {
ethernet0 = &fec1;
ethernet1 = &lan1;
ethernet2 = &lan2;
ethernet3 = &lan3;
ethernet4 = &lan4;
usb0 = &usbotg1;
usb1 = &usbotg2;
};
};
&fec1 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_fec1>;
phy-mode = "rgmii-id";
local-mac-address = [00 00 00 00 00 00];
status = "okay";
fixed-link {
speed = <1000>;
full-duplex;
};
};
&i2c3 {
clock-frequency = <400000>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_i2c3>;
status = "okay";
switch: switch@5f {
compatible = "microchip,ksz9897";
reg = <0x5f>;
pinctrl-0 = <&pinctrl_ksz>;
interrupt-parent = <&gpio4>;
interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
phy-mode = "rgmii-id";
ports {
#address-cells = <1>;
#size-cells = <0>;
lan1: port@0 {
reg = <0>;
label = "lan1";
local-mac-address = [00 00 00 00 00 00];
};
lan2: port@1 {
reg = <1>;
label = "lan2";
local-mac-address = [00 00 00 00 00 00];
};
lan3: port@2 {
reg = <2>;
label = "lan3";
local-mac-address = [00 00 00 00 00 00];
};
lan4: port@3 {
reg = <3>;
label = "lan4";
local-mac-address = [00 00 00 00 00 00];
};
port@5 {
reg = <5>;
label = "cpu";
ethernet = <&fec1>;
phy-mode = "rgmii-id";
fixed-link {
speed = <1000>;
full-duplex;
};
};
};
};
};
I'm running into an issue trying to get a DSA driver working for this
switch on this board. My UCLASS_DSA driver is getting registered and
dsa_post_bind successfully sees the ports and the master_dev however
when eth_initialize() is called my dsa driver probes before the fec1
master because i2c comes before fec in the imx8mm.dtsi device-tree.
Any suggestions on how and where to properly resolve this (Likely
there is a way to force a probe)?
If I hack around that my moving i2c in imx8mm.dtsi after fec I see a
U-Boot net device for all the ports including the cpu port. I'm not
quite sure what the necessity of the cpu port is here because I assume
the idea is to set 'ethact' to one of the physical ports before
initiating a cmd that sends/receives traffic but I suppose the cpu
interface is there because that's what Linux dsa does.
Without adding any tagging and just doing a 'setenv ethact lan1; ping
<serverip>' I'm not seeing any response. I'm not exactly clear what to
be using here for tagging. The Linux ksz driver [3] appears to add a 2
byte tag on ingress packets but a 1 byte on egress packets.
I'm also not quite clear if I should be implementing link detect on the ports.
Any suggestions would be greatly appreciated. I would like to get this
dsa driver working and submitted and then perhaps move on to added a
DSA driver or DSA support to drivers/net/phy/mv88e61xx.c
Best regards,
Tim
[1] https://www.mail-archive.com/u-boot@lists.denx.de/msg389713.html
[2] https://elixir.bootlin.com/linux/latest/source/net/dsa/tag_ksz.c
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: U-Boot DSA driver for microchip KSZ9477 suggestions needed
2021-06-21 23:10 U-Boot DSA driver for microchip KSZ9477 suggestions needed Tim Harvey
@ 2021-06-21 23:49 ` Vladimir Oltean
2021-06-22 16:38 ` Tim Harvey
0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Oltean @ 2021-06-21 23:49 UTC (permalink / raw)
To: Tim Harvey
Cc: vladimir.oltean, alexandru.marginean, claudiu.manoil,
Michael Walle, u-boot
On Mon, Jun 21, 2021 at 04:10:51PM -0700, Tim Harvey wrote:
> Greetings,
>
> I've written a U-Boot phy driver for a Microchip KSZ9477 ethernet
> switch that I submitted some time ago as an RFC [1] which simply
> enables the ports in the dt and puts them in forwarding mode with link
> detect. However I think this would be much better suited as a DSA
> driver now that UCLASS_DSA exists.
Define 'link detect'. I don't know how the switch-as-PHY drivers work.
I suppose the DSA master is made to connect to a PHY device which is
implemented by the switch driver, but whose port's link status is being
reported?
> The KSZ9477 switch is used in the Gateworks GW7901 IMX8M Mini based
> board and is hooked up via I2C (instead of MDIO) so the relevant dt
> (already in Mainline Linux) looks like this:
>
> / {
> aliases {
> ethernet0 = &fec1;
> ethernet1 = &lan1;
> ethernet2 = &lan2;
> ethernet3 = &lan3;
> ethernet4 = &lan4;
> usb0 = &usbotg1;
> usb1 = &usbotg2;
> };
> };
>
> &fec1 {
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_fec1>;
> phy-mode = "rgmii-id";
Can you confirm that the FEC will not attempt to add internal RGMII
delays in this case? Either the KSZ switch or the FEC should add delays,
but not both (the link will not work otherwise).
> local-mac-address = [00 00 00 00 00 00];
> status = "okay";
>
> fixed-link {
> speed = <1000>;
> full-duplex;
> };
> };
>
> &i2c3 {
> clock-frequency = <400000>;
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_i2c3>;
> status = "okay";
>
> switch: switch@5f {
> compatible = "microchip,ksz9897";
> reg = <0x5f>;
> pinctrl-0 = <&pinctrl_ksz>;
> interrupt-parent = <&gpio4>;
> interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
> phy-mode = "rgmii-id";
This is strange. Why is phy-mode in the 'switch' node (it is a per-port
property)?
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
> lan1: port@0 {
> reg = <0>;
> label = "lan1";
> local-mac-address = [00 00 00 00 00 00];
> };
> lan2: port@1 {
> reg = <1>;
> label = "lan2";
> local-mac-address = [00 00 00 00 00 00];
> };
> lan3: port@2 {
> reg = <2>;
> label = "lan3";
> local-mac-address = [00 00 00 00 00 00];
> };
> lan4: port@3 {
> reg = <3>;
> label = "lan4";
> local-mac-address = [00 00 00 00 00 00];
> };
> port@5 {
> reg = <5>;
> label = "cpu";
> ethernet = <&fec1>;
> phy-mode = "rgmii-id";
>
> fixed-link {
> speed = <1000>;
> full-duplex;
> };
> };
> };
> };
> };
>
> I'm running into an issue trying to get a DSA driver working for this
> switch on this board. My UCLASS_DSA driver is getting registered and
> dsa_post_bind successfully sees the ports and the master_dev however
> when eth_initialize() is called my dsa driver probes before the fec1
> master because i2c comes before fec in the imx8mm.dtsi device-tree.
> Any suggestions on how and where to properly resolve this (Likely
> there is a way to force a probe)?
>
> If I hack around that my moving i2c in imx8mm.dtsi after fec I see a
> U-Boot net device for all the ports including the cpu port.
Do you have patch e5d7d119287e ("net: dsa: probe master device")? Does
that not work?
> I'm not quite sure what the necessity of the cpu port is here because
> I assume the idea is to set 'ethact' to one of the physical ports
> before initiating a cmd that sends/receives traffic but I suppose the
> cpu interface is there because that's what Linux dsa does.
I am quite unsure what you mean. 'CPU port' == 'DSA master', i.e. the FEC?
You're asking why does the FEC driver register a UCLASS_ETH device,
considering that it should know it's a DSA master and you should only
use the DSA UCLASS_ETH devices for RX/TX?
I mean, if you can make that change and keep the drivers unaware of the
fact that they are DSA masters, I guess patches are welcome...
> Without adding any tagging and just doing a 'setenv ethact lan1; ping
> <serverip>' I'm not seeing any response.
And are you expecting too? Is the switch configured to forward packets
received from the host port which have no tail tag? Does it do that? If
not, is it an electrical problem (see the RGMII delay question)?
> I'm not exactly clear what to be using here for tagging. The Linux ksz
> driver [3] appears to add a 2 byte tag on ingress packets but a 1 byte
> on egress packets.
That's the expectation, to use the tail tagger to be able to steer
packets towards the desired port, correct.
If you have problems with that you can always use VLANs too, like
sja1105 does:
https://patchwork.ozlabs.org/project/uboot/patch/20191215205302.13325-4-olteanv@gmail.com/
> I'm also not quite clear if I should be implementing link detect on the ports.
DSA assumes that every port has a phy-handle to a PHY driver. In Linux,
the integrated PHYs from the KSZ switches are implemented using an MDIO
bus registered by the DSA switch driver, with custom logic to access
each PHY register from the port memory space, because these switches are
kinda quirky and don't really expose a clause 22 MDIO register map. I'm
not sure how that would work out in U-Boot, but it is the starting point
I would say.
> Any suggestions would be greatly appreciated. I would like to get this
> dsa driver working and submitted and then perhaps move on to added a
> DSA driver or DSA support to drivers/net/phy/mv88e61xx.c
That would be nice. I also need to find some time to resend the sja1105
driver.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: U-Boot DSA driver for microchip KSZ9477 suggestions needed
2021-06-21 23:49 ` Vladimir Oltean
@ 2021-06-22 16:38 ` Tim Harvey
2021-06-23 10:37 ` Vladimir Oltean
0 siblings, 1 reply; 8+ messages in thread
From: Tim Harvey @ 2021-06-22 16:38 UTC (permalink / raw)
To: Vladimir Oltean
Cc: vladimir.oltean, alexandru.marginean, claudiu.manoil,
Michael Walle, u-boot
On Mon, Jun 21, 2021 at 4:49 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Jun 21, 2021 at 04:10:51PM -0700, Tim Harvey wrote:
> > Greetings,
> >
> > I've written a U-Boot phy driver for a Microchip KSZ9477 ethernet
> > switch that I submitted some time ago as an RFC [1] which simply
> > enables the ports in the dt and puts them in forwarding mode with link
> > detect. However I think this would be much better suited as a DSA
> > driver now that UCLASS_DSA exists.
>
> Define 'link detect'. I don't know how the switch-as-PHY drivers work.
> I suppose the DSA master is made to connect to a PHY device which is
> implemented by the switch driver, but whose port's link status is being
> reported?
Yes, each downward port's link status can be detected. Each port has
standard GbE PHY registers which can be indirectly read.
>
> > The KSZ9477 switch is used in the Gateworks GW7901 IMX8M Mini based
> > board and is hooked up via I2C (instead of MDIO) so the relevant dt
> > (already in Mainline Linux) looks like this:
> >
> > / {
> > aliases {
> > ethernet0 = &fec1;
> > ethernet1 = &lan1;
> > ethernet2 = &lan2;
> > ethernet3 = &lan3;
> > ethernet4 = &lan4;
> > usb0 = &usbotg1;
> > usb1 = &usbotg2;
> > };
> > };
> >
> > &fec1 {
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_fec1>;
> > phy-mode = "rgmii-id";
>
> Can you confirm that the FEC will not attempt to add internal RGMII
> delays in this case? Either the KSZ switch or the FEC should add delays,
> but not both (the link will not work otherwise).
Correct, the FEC does not configure delays, only the PHY side.
In the case of DSA for dt it is correct to have phy-mode defined both
in the MAC and the PHY (each cpu port) correct?
>
> > local-mac-address = [00 00 00 00 00 00];
> > status = "okay";
> >
> > fixed-link {
> > speed = <1000>;
> > full-duplex;
> > };
> > };
> >
> > &i2c3 {
> > clock-frequency = <400000>;
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_i2c3>;
> > status = "okay";
> >
> > switch: switch@5f {
> > compatible = "microchip,ksz9897";
> > reg = <0x5f>;
> > pinctrl-0 = <&pinctrl_ksz>;
> > interrupt-parent = <&gpio4>;
> > interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
> > phy-mode = "rgmii-id";
>
> This is strange. Why is phy-mode in the 'switch' node (it is a per-port
> property)?
Right, this is wrong and just did not get caught. I will remove it.
>
> >
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > lan1: port@0 {
> > reg = <0>;
> > label = "lan1";
> > local-mac-address = [00 00 00 00 00 00];
> > };
> > lan2: port@1 {
> > reg = <1>;
> > label = "lan2";
> > local-mac-address = [00 00 00 00 00 00];
> > };
> > lan3: port@2 {
> > reg = <2>;
> > label = "lan3";
> > local-mac-address = [00 00 00 00 00 00];
> > };
> > lan4: port@3 {
> > reg = <3>;
> > label = "lan4";
> > local-mac-address = [00 00 00 00 00 00];
> > };
> > port@5 {
> > reg = <5>;
> > label = "cpu";
> > ethernet = <&fec1>;
> > phy-mode = "rgmii-id";
> >
> > fixed-link {
> > speed = <1000>;
> > full-duplex;
> > };
> > };
> > };
> > };
> > };
> >
> > I'm running into an issue trying to get a DSA driver working for this
> > switch on this board. My UCLASS_DSA driver is getting registered and
> > dsa_post_bind successfully sees the ports and the master_dev however
> > when eth_initialize() is called my dsa driver probes before the fec1
> > master because i2c comes before fec in the imx8mm.dtsi device-tree.
> > Any suggestions on how and where to properly resolve this (Likely
> > there is a way to force a probe)?
> >
> > If I hack around that my moving i2c in imx8mm.dtsi after fec I see a
> > U-Boot net device for all the ports including the cpu port.
>
> Do you have patch e5d7d119287e ("net: dsa: probe master device")? Does
> that not work?
Yes, I do have that patch and it seems I did not understand the issue properly.
The issue I'm seeing looks like it has to do with the fec driver. Whe
fecmxc_probe is called it calls
fec_get_miibus()->fec_mii_setspeed()->fec_get_ckl_rate() which fails
because it uses 'uclass_get_device(UCLASS_ETH, 0, &dev) to find the
first eth device assuming its the fec and instead its the lan1 port.
It seems the fec_get_clk_rate() makes a poor assumption that the first
UCLASS_ETH is the fec device and I'll have to find a way to fix that.
I've also found that if I do not have a 'fixed-link' node in each of
my downstream rj45 ports (lan1/lan2/lan3/lan4 above) else
dsa_port_probe() will return -ENODEV due to
dm_eth_phy_connect()->dm_eth_connect_phy_handle() failing because it
does not see the link as fixed. Should I really need a 'fixed-link' in
my non-cpu port children above? Not only is it not needed for Linux
dsa to work if I add it to the Linux dt, the Linux ksz9477 driver
crashes (at least in 5.4). I feel like perhaps dm_eth_phy_connect()
should default to thinking of the ports as fixed links if they have no
phy-handle/phy/phy-device node.
>
> > I'm not quite sure what the necessity of the cpu port is here because
> > I assume the idea is to set 'ethact' to one of the physical ports
> > before initiating a cmd that sends/receives traffic but I suppose the
> > cpu interface is there because that's what Linux dsa does.
>
> I am quite unsure what you mean. 'CPU port' == 'DSA master', i.e. the FEC?
> You're asking why does the FEC driver register a UCLASS_ETH device,
> considering that it should know it's a DSA master and you should only
> use the DSA UCLASS_ETH devices for RX/TX?
> I mean, if you can make that change and keep the drivers unaware of the
> fact that they are DSA masters, I guess patches are welcome...
>
> > Without adding any tagging and just doing a 'setenv ethact lan1; ping
> > <serverip>' I'm not seeing any response.
>
> And are you expecting too? Is the switch configured to forward packets
> received from the host port which have no tail tag? Does it do that? If
> not, is it an electrical problem (see the RGMII delay question)?
I understand that tagging is needed here in order to determine the
port the packet was meant for. When I add tagging of any kind the fec
driver appears to crash (memory alignment issue?) so I started off by
not tagging (dsa_set_tagging(dev, 0, 0)) and only enabling lan1 but
found a ping fails. I know that I've got my RGMII delay configured
correctly for the switch's cpu port so I'm not sure where to look
next.
>
> > I'm not exactly clear what to be using here for tagging. The Linux ksz
> > driver [3] appears to add a 2 byte tag on ingress packets but a 1 byte
> > on egress packets.
>
> That's the expectation, to use the tail tagger to be able to steer
> packets towards the desired port, correct.
I can really use anything I want for tagging because the tag is only
present to/from master to cpu port right? So I can use bytes on head,
or bytes on tail, and any magic I want right?
> If you have problems with that you can always use VLANs too, like
> sja1105 does:
> https://patchwork.ozlabs.org/project/uboot/patch/20191215205302.13325-4-olteanv@gmail.com/
>
Thanks - I'll look that over. Currently the only dsa driver examples
in U-Boot are the felix_switch.c and dsa_sandbox.c
> > I'm also not quite clear if I should be implementing link detect on the ports.
>
> DSA assumes that every port has a phy-handle to a PHY driver. In Linux,
> the integrated PHYs from the KSZ switches are implemented using an MDIO
> bus registered by the DSA switch driver, with custom logic to access
> each PHY register from the port memory space, because these switches are
> kinda quirky and don't really expose a clause 22 MDIO register map. I'm
> not sure how that would work out in U-Boot, but it is the starting point
> I would say.
I can have my ksz dsa driver register an mdio bus for this and this is
probably the piece that I'm currently missing
>
> > Any suggestions would be greatly appreciated. I would like to get this
> > dsa driver working and submitted and then perhaps move on to added a
> > DSA driver or DSA support to drivers/net/phy/mv88e61xx.c
>
> That would be nice. I also need to find some time to resend the sja1105
> driver.
Thanks for the feedback!
Tim
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: U-Boot DSA driver for microchip KSZ9477 suggestions needed
2021-06-22 16:38 ` Tim Harvey
@ 2021-06-23 10:37 ` Vladimir Oltean
2021-06-24 0:28 ` Tim Harvey
0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Oltean @ 2021-06-23 10:37 UTC (permalink / raw)
To: tharvey
Cc: Vladimir Oltean, Alexandru Marginean, Claudiu Manoil,
Michael Walle, u-boot
On Tue, Jun 22, 2021 at 09:38:52AM -0700, Tim Harvey wrote:
> On Mon, Jun 21, 2021 at 4:49 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Mon, Jun 21, 2021 at 04:10:51PM -0700, Tim Harvey wrote:
> > > Greetings,
> > >
> > > I've written a U-Boot phy driver for a Microchip KSZ9477 ethernet
> > > switch that I submitted some time ago as an RFC [1] which simply
> > > enables the ports in the dt and puts them in forwarding mode with link
> > > detect. However I think this would be much better suited as a DSA
> > > driver now that UCLASS_DSA exists.
> >
> > Define 'link detect'. I don't know how the switch-as-PHY drivers work.
> > I suppose the DSA master is made to connect to a PHY device which is
> > implemented by the switch driver, but whose port's link status is being
> > reported?
>
> Yes, each downward port's link status can be detected. Each port has
> standard GbE PHY registers which can be indirectly read.
What do you mean by 'each port'?
With the switch-as-PHY driver, how many UCLASS_ETH devices are being
registered? One for the FEC and one for each front-facing switch port
(same as DSA would)? Only one, and that for the FEC? If the latter, how
does U-Boot know which of the front-facing switch ports to use as the
link status reporting for the FEC device?
> > Can you confirm that the FEC will not attempt to add internal RGMII
> > delays in this case? Either the KSZ switch or the FEC should add delays,
> > but not both (the link will not work otherwise).
>
> Correct, the FEC does not configure delays, only the PHY side.
>
> In the case of DSA for dt it is correct to have phy-mode defined both
> in the MAC and the PHY (each cpu port) correct?
It is, correct, but I'm not quite sure they're both supposed to be
"rgmii-id", more like one is "rgmii-id" and the other is plain "rgmii".
Anyhow.
> > > If I hack around that my moving i2c in imx8mm.dtsi after fec I see a
> > > U-Boot net device for all the ports including the cpu port.
> >
> > Do you have patch e5d7d119287e ("net: dsa: probe master device")? Does
> > that not work?
>
> Yes, I do have that patch and it seems I did not understand the issue properly.
>
> The issue I'm seeing looks like it has to do with the fec driver. Whe
> fecmxc_probe is called it calls
> fec_get_miibus()->fec_mii_setspeed()->fec_get_ckl_rate() which fails
> because it uses 'uclass_get_device(UCLASS_ETH, 0, &dev) to find the
> first eth device assuming its the fec and instead its the lan1 port.
> It seems the fec_get_clk_rate() makes a poor assumption that the first
> UCLASS_ETH is the fec device and I'll have to find a way to fix that.
I'm not familiar with the FEC driver internals, so you might need to
ping some of the driver authors for help if needed, but any assumption
about UCLASS_ETH device indices and then calling dev_get_priv() on them
looks wrong, yes.
> I've also found that if I do not have a 'fixed-link' node in each of
> my downstream rj45 ports (lan1/lan2/lan3/lan4 above) else
> dsa_port_probe() will return -ENODEV due to
> dm_eth_phy_connect()->dm_eth_connect_phy_handle() failing because it
> does not see the link as fixed. Should I really need a 'fixed-link' in
> my non-cpu port children above? Not only is it not needed for Linux
> dsa to work if I add it to the Linux dt, the Linux ksz9477 driver
> crashes (at least in 5.4). I feel like perhaps dm_eth_phy_connect()
> should default to thinking of the ports as fixed links if they have no
> phy-handle/phy/phy-device node.
See below.
> >
> > > I'm not quite sure what the necessity of the cpu port is here because
> > > I assume the idea is to set 'ethact' to one of the physical ports
> > > before initiating a cmd that sends/receives traffic but I suppose the
> > > cpu interface is there because that's what Linux dsa does.
> >
> > I am quite unsure what you mean. 'CPU port' == 'DSA master', i.e. the FEC?
> > You're asking why does the FEC driver register a UCLASS_ETH device,
> > considering that it should know it's a DSA master and you should only
> > use the DSA UCLASS_ETH devices for RX/TX?
> > I mean, if you can make that change and keep the drivers unaware of the
> > fact that they are DSA masters, I guess patches are welcome...
> >
> > > Without adding any tagging and just doing a 'setenv ethact lan1; ping
> > > <serverip>' I'm not seeing any response.
What do you mean 'response'? If you tcpdump on the other end do you see
the packet transmitted by U-Boot coming through? You might want to add a
debugging patch printing some port statistics.
> > And are you expecting too? Is the switch configured to forward packets
> > received from the host port which have no tail tag? Does it do that? If
> > not, is it an electrical problem (see the RGMII delay question)?
>
> I understand that tagging is needed here in order to determine the
> port the packet was meant for. When I add tagging of any kind the fec
> driver appears to crash (memory alignment issue?) so I started off by
> not tagging (dsa_set_tagging(dev, 0, 0)) and only enabling lan1 but
> found a ping fails. I know that I've got my RGMII delay configured
> correctly for the switch's cpu port so I'm not sure where to look
> next.
Again, this would need more debugging in fecmxc_send() to see what
crashes and why.
> >
> > > I'm not exactly clear what to be using here for tagging. The Linux ksz
> > > driver [3] appears to add a 2 byte tag on ingress packets but a 1 byte
> > > on egress packets.
> >
> > That's the expectation, to use the tail tagger to be able to steer
> > packets towards the desired port, correct.
>
> I can really use anything I want for tagging because the tag is only
> present to/from master to cpu port right? So I can use bytes on head,
> or bytes on tail, and any magic I want right?
You can use anything you want for tagging as long as the hardware
understands it, so in practice that means "not really anything"...
In particular, I think the KSZ switches use tail tags, not headers, so
that's what you need to work with.
I'm not sure how clear this is for you, but here's an image from the
Linux Documentation/networking/dsa/dsa.rst file:
Unaware application
opens and binds socket
| ^
| |
+-----------v--|--------------------+
|+------+ +------+ +------+ +------+|
|| swp0 | | swp1 | | swp2 | | swp3 ||
|+------+-+------+-+------+-+------+|
| DSA switch driver |
+-----------------------------------+
| ^
Tag added by | | Tag consumed by
switch driver | | switch driver
v |
+-----------------------------------+
| Unmodified host interface driver | Software
--------+-----------------------------------+------------
| Host interface (eth0) | Hardware
+-----------------------------------+
| ^
Tag consumed by | | Tag added by
switch hardware | | switch hardware
v |
+-----------------------------------+
| Switch |
|+------+ +------+ +------+ +------+|
|| swp0 | | swp1 | | swp2 | | swp3 ||
++------+-+------+-+------+-+------++
The reason why I suggested you can use VLANs is because switches can
typically be configured to understand VLANs. On ingress from the outside
world, you configure port i with pvid i so that it tags all untagged
packets with that VID (and you configure the CPU port as egress-tagged
in that VLAN, so that software can recover it).
On egress from the CPU, you set up one VLAN per each front-facing port,
which contains only the CPU port and the destination where you want that
packet to go (i.e. one front port). You configure the front port as
egress-untagged in that VLAN so that the fact a VLAN is being
transmitted between the host port and the switch (effectively a DSA
tagging protocol) is invisible to everybody on the wire.
Of course, you don't have to use VLANs, it was just a suggestion.
> > > I'm also not quite clear if I should be implementing link detect on the ports.
> >
> > DSA assumes that every port has a phy-handle to a PHY driver. In Linux,
> > the integrated PHYs from the KSZ switches are implemented using an MDIO
> > bus registered by the DSA switch driver, with custom logic to access
> > each PHY register from the port memory space, because these switches are
> > kinda quirky and don't really expose a clause 22 MDIO register map. I'm
> > not sure how that would work out in U-Boot, but it is the starting point
> > I would say.
>
> I can have my ksz dsa driver register an mdio bus for this and this is
> probably the piece that I'm currently missing
This is what I've been trying to tell you.
Linux DSA offers some helpers for accessing internal PHYs, which in my
opinion it should have not offered, see dsa_slave_mii_bus_init(). In
particular it assumes that port i will have a PHY at MDIO address i.
U-Boot DSA does not offer this helper, your only constructs are:
(a) fixed-link: this means there is no PHY and that the MAC operates at
a constant speed/duplex setting throughout the runtime of the device,
with no means of link detection. This is probably what you want for
the MAC-to-MAC link between the host port and the switch CPU port,
but not what you want for the front-facing RJ45 ports which can
switch speeds between 10/100/1000.
(b) phy-handle: this is a phandle to a device tree description of a PHY
device, be it an integrated PHY or a discrete one. Of course, your
big problem is that you don't have a struct phy_device for your PHYs
integrated in the KSZ switch, especially one with OF bindings.
For reference, the sja1105 Linux driver also supports switches with
internal PHYs, here's how it describes things in the device tree.
Because on this switch, pinmuxing is complex, and certain ports like
port 1 can either be connected to an internal 100base-TX PHY or to an
SGMII PCS, describing the phy-handle in the device tree is necessary
even if it is towards a struct phy_device registered by an MDIO bus
provided by the same driver (instead of making the assumption that the
pins are strapped one way or another in the driver itself).
&spi_bridge {
/* SW1 */
ethernet-switch@0 {
compatible = "nxp,sja1110a";
reg = <0>;
spi-max-frequency = <4000000>;
spi-cpol;
dsa,member = <0 0>;
ethernet-ports {
#address-cells = <1>;
#size-cells = <0>;
/* Microcontroller port */
port@0 {
reg = <0>;
status = "disabled";
};
/* SW1_P1 */
port@1 {
reg = <1>;
label = "con_2x20";
phy-mode = "internal";
phy-handle = <&sw1_port1_base_tx_phy>;
};
port@2 {
reg = <2>;
ethernet = <&dpmac17>;
phy-mode = "rgmii-id";
fixed-link {
speed = <1000>;
full-duplex;
};
};
port@3 {
reg = <3>;
label = "1ge_p1";
phy-mode = "rgmii-id";
phy-handle = <&sw1_mii3_phy>;
};
sw1p4: port@4 {
reg = <4>;
link = <&sw2p1>;
phy-mode = "sgmii";
fixed-link {
speed = <1000>;
full-duplex;
};
};
port@5 {
reg = <5>;
label = "trx1";
phy-mode = "internal";
phy-handle = <&sw1_port5_base_t1_phy>;
};
port@6 {
reg = <6>;
label = "trx2";
phy-mode = "internal";
phy-handle = <&sw1_port6_base_t1_phy>;
};
port@7 {
reg = <7>;
label = "trx3";
phy-mode = "internal";
phy-handle = <&sw1_port7_base_t1_phy>;
};
port@8 {
reg = <8>;
label = "trx4";
phy-mode = "internal";
phy-handle = <&sw1_port8_base_t1_phy>;
};
port@9 {
reg = <9>;
label = "trx5";
phy-mode = "internal";
phy-handle = <&sw1_port9_base_t1_phy>;
};
port@a {
reg = <10>;
label = "trx6";
phy-mode = "internal";
phy-handle = <&sw1_port10_base_t1_phy>;
};
};
mdios {
#address-cells = <1>;
#size-cells = <0>;
mdio@0 {
reg = <0>;
compatible = "nxp,sja1110-base-t1-mdio";
#address-cells = <1>;
#size-cells = <0>;
sw1_port5_base_t1_phy: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x1>;
};
sw1_port6_base_t1_phy: ethernet-phy@2 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x2>;
};
sw1_port7_base_t1_phy: ethernet-phy@3 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x3>;
};
sw1_port8_base_t1_phy: ethernet-phy@4 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x4>;
};
sw1_port9_base_t1_phy: ethernet-phy@5 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x5>;
};
sw1_port10_base_t1_phy: ethernet-phy@6 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x6>;
};
};
mdio@1 {
reg = <1>;
compatible = "nxp,sja1110-base-tx-mdio";
#address-cells = <1>;
#size-cells = <0>;
sw1_port1_base_tx_phy: ethernet-phy@1 {
reg = <0x1>;
};
};
};
};
};
[ please disregard the fact that there are 2 mdio buses, one for
100base-T1 and another for 100base-TX PHYs. There are reasons why that
is, but I think you should only have one "mdio" node under the switch ]
By following this example I believe you can model your ports with an
internal PHY in pretty much the same way, while keeping the simple model
that U-Boot DSA offers - and this should answer your other question too
(fixed-link - no, phy-handle - yes). Please, let's keep the U-Boot DSA
framework simple and without the bells and whistles from Linux, even if
this means that the OF bindings for the switch need to be more descriptive
and less free-form. This was a conscious design decision and I don't
think it is limiting.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: U-Boot DSA driver for microchip KSZ9477 suggestions needed
2021-06-23 10:37 ` Vladimir Oltean
@ 2021-06-24 0:28 ` Tim Harvey
2021-06-24 2:05 ` Vladimir Oltean
0 siblings, 1 reply; 8+ messages in thread
From: Tim Harvey @ 2021-06-24 0:28 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Vladimir Oltean, Alexandru Marginean, Claudiu Manoil,
Michael Walle, u-boot
On Wed, Jun 23, 2021 at 3:37 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Tue, Jun 22, 2021 at 09:38:52AM -0700, Tim Harvey wrote:
> > On Mon, Jun 21, 2021 at 4:49 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > On Mon, Jun 21, 2021 at 04:10:51PM -0700, Tim Harvey wrote:
> > > > Greetings,
> > > >
> > > > I've written a U-Boot phy driver for a Microchip KSZ9477 ethernet
> > > > switch that I submitted some time ago as an RFC [1] which simply
> > > > enables the ports in the dt and puts them in forwarding mode with link
> > > > detect. However I think this would be much better suited as a DSA
> > > > driver now that UCLASS_DSA exists.
> > >
> > > Define 'link detect'. I don't know how the switch-as-PHY drivers work.
> > > I suppose the DSA master is made to connect to a PHY device which is
> > > implemented by the switch driver, but whose port's link status is being
> > > reported?
> >
> > Yes, each downward port's link status can be detected. Each port has
> > standard GbE PHY registers which can be indirectly read.
>
> What do you mean by 'each port'?
> With the switch-as-PHY driver, how many UCLASS_ETH devices are being
> registered? One for the FEC and one for each front-facing switch port
> (same as DSA would)? Only one, and that for the FEC? If the latter, how
> does U-Boot know which of the front-facing switch ports to use as the
> link status reporting for the FEC device?
With my ksz9477 'switch-as-phy' driver
(https://www.mail-archive.com/u-boot@lists.denx.de/msg389714.html)
there is only 1 UCLASS_ETH device registered, just fec. The driver
provides a UCLASS_ETH_PHY and its probe parses the dt to find the
ports then registers an mdio bus for the indirect register access and
registers a phy driver that matches the id's of the switch port.
This only worked with another patch
(https://www.mail-archive.com/u-boot@lists.denx.de/msg389712.html) to
the UCLASS_ETH_PHY driver that allowed the phy driver to bind the the
fec mac. Therefore this acts as a single phy on the MAC and when
phy_config is called it sets up auto-negotiation on all the non-cpu
ports and when phy_startup is called it performs link detect on all
the non-cpu ports (if any port is linked its considered a link). The
phy_config configured all the ports in forwarding mode.
This was modeled after the drivers/net/phy/mv88e61xx.c (switch-as-phy)
driver and the only difference between that is that it attempted to
use driver-model and get its config from dt. The big downside of this
method is that the switch is configured in forwarding mode so you end
up with a bridge loop while active in U-Boot if you have more than one
port connected to your network.
I think if you look over that relatively simple driver it will make
sense how the ksz switch works in case I'm explaining it wrong or with
the wrong terminology.
>
> > > Can you confirm that the FEC will not attempt to add internal RGMII
> > > delays in this case? Either the KSZ switch or the FEC should add delays,
> > > but not both (the link will not work otherwise).
> >
> > Correct, the FEC does not configure delays, only the PHY side.
> >
> > In the case of DSA for dt it is correct to have phy-mode defined both
> > in the MAC and the PHY (each cpu port) correct?
>
> It is, correct, but I'm not quite sure they're both supposed to be
> "rgmii-id", more like one is "rgmii-id" and the other is plain "rgmii".
> Anyhow.
>
> > > > If I hack around that my moving i2c in imx8mm.dtsi after fec I see a
> > > > U-Boot net device for all the ports including the cpu port.
> > >
> > > Do you have patch e5d7d119287e ("net: dsa: probe master device")? Does
> > > that not work?
> >
> > Yes, I do have that patch and it seems I did not understand the issue properly.
> >
> > The issue I'm seeing looks like it has to do with the fec driver. Whe
> > fecmxc_probe is called it calls
> > fec_get_miibus()->fec_mii_setspeed()->fec_get_ckl_rate() which fails
> > because it uses 'uclass_get_device(UCLASS_ETH, 0, &dev) to find the
> > first eth device assuming its the fec and instead its the lan1 port.
> > It seems the fec_get_clk_rate() makes a poor assumption that the first
> > UCLASS_ETH is the fec device and I'll have to find a way to fix that.
>
> I'm not familiar with the FEC driver internals, so you might need to
> ping some of the driver authors for help if needed, but any assumption
> about UCLASS_ETH device indices and then calling dev_get_priv() on them
> looks wrong, yes.
>
> > I've also found that if I do not have a 'fixed-link' node in each of
> > my downstream rj45 ports (lan1/lan2/lan3/lan4 above) else
> > dsa_port_probe() will return -ENODEV due to
> > dm_eth_phy_connect()->dm_eth_connect_phy_handle() failing because it
> > does not see the link as fixed. Should I really need a 'fixed-link' in
> > my non-cpu port children above? Not only is it not needed for Linux
> > dsa to work if I add it to the Linux dt, the Linux ksz9477 driver
> > crashes (at least in 5.4). I feel like perhaps dm_eth_phy_connect()
> > should default to thinking of the ports as fixed links if they have no
> > phy-handle/phy/phy-device node.
>
> See below.
>
> > >
> > > > I'm not quite sure what the necessity of the cpu port is here because
> > > > I assume the idea is to set 'ethact' to one of the physical ports
> > > > before initiating a cmd that sends/receives traffic but I suppose the
> > > > cpu interface is there because that's what Linux dsa does.
> > >
> > > I am quite unsure what you mean. 'CPU port' == 'DSA master', i.e. the FEC?
> > > You're asking why does the FEC driver register a UCLASS_ETH device,
> > > considering that it should know it's a DSA master and you should only
> > > use the DSA UCLASS_ETH devices for RX/TX?
> > > I mean, if you can make that change and keep the drivers unaware of the
> > > fact that they are DSA masters, I guess patches are welcome...
> > >
> > > > Without adding any tagging and just doing a 'setenv ethact lan1; ping
> > > > <serverip>' I'm not seeing any response.
>
> What do you mean 'response'? If you tcpdump on the other end do you see
> the packet transmitted by U-Boot coming through? You might want to add a
> debugging patch printing some port statistics.
>
> > > And are you expecting too? Is the switch configured to forward packets
> > > received from the host port which have no tail tag? Does it do that? If
> > > not, is it an electrical problem (see the RGMII delay question)?
> >
> > I understand that tagging is needed here in order to determine the
> > port the packet was meant for. When I add tagging of any kind the fec
> > driver appears to crash (memory alignment issue?) so I started off by
> > not tagging (dsa_set_tagging(dev, 0, 0)) and only enabling lan1 but
> > found a ping fails. I know that I've got my RGMII delay configured
> > correctly for the switch's cpu port so I'm not sure where to look
> > next.
>
> Again, this would need more debugging in fecmxc_send() to see what
> crashes and why.
>
> > >
> > > > I'm not exactly clear what to be using here for tagging. The Linux ksz
> > > > driver [3] appears to add a 2 byte tag on ingress packets but a 1 byte
> > > > on egress packets.
> > >
> > > That's the expectation, to use the tail tagger to be able to steer
> > > packets towards the desired port, correct.
> >
> > I can really use anything I want for tagging because the tag is only
> > present to/from master to cpu port right? So I can use bytes on head,
> > or bytes on tail, and any magic I want right?
>
> You can use anything you want for tagging as long as the hardware
> understands it, so in practice that means "not really anything"...
> In particular, I think the KSZ switches use tail tags, not headers, so
> that's what you need to work with.
> I'm not sure how clear this is for you, but here's an image from the
> Linux Documentation/networking/dsa/dsa.rst file:
>
> Unaware application
> opens and binds socket
> | ^
> | |
> +-----------v--|--------------------+
> |+------+ +------+ +------+ +------+|
> || swp0 | | swp1 | | swp2 | | swp3 ||
> |+------+-+------+-+------+-+------+|
> | DSA switch driver |
> +-----------------------------------+
> | ^
> Tag added by | | Tag consumed by
> switch driver | | switch driver
> v |
> +-----------------------------------+
> | Unmodified host interface driver | Software
> --------+-----------------------------------+------------
> | Host interface (eth0) | Hardware
> +-----------------------------------+
> | ^
> Tag consumed by | | Tag added by
> switch hardware | | switch hardware
> v |
> +-----------------------------------+
> | Switch |
> |+------+ +------+ +------+ +------+|
> || swp0 | | swp1 | | swp2 | | swp3 ||
> ++------+-+------+-+------+-+------++
>
> The reason why I suggested you can use VLANs is because switches can
> typically be configured to understand VLANs. On ingress from the outside
> world, you configure port i with pvid i so that it tags all untagged
> packets with that VID (and you configure the CPU port as egress-tagged
> in that VLAN, so that software can recover it).
> On egress from the CPU, you set up one VLAN per each front-facing port,
> which contains only the CPU port and the destination where you want that
> packet to go (i.e. one front port). You configure the front port as
> egress-untagged in that VLAN so that the fact a VLAN is being
> transmitted between the host port and the switch (effectively a DSA
> tagging protocol) is invisible to everybody on the wire.
> Of course, you don't have to use VLANs, it was just a suggestion.
>
> > > > I'm also not quite clear if I should be implementing link detect on the ports.
> > >
> > > DSA assumes that every port has a phy-handle to a PHY driver. In Linux,
> > > the integrated PHYs from the KSZ switches are implemented using an MDIO
> > > bus registered by the DSA switch driver, with custom logic to access
> > > each PHY register from the port memory space, because these switches are
> > > kinda quirky and don't really expose a clause 22 MDIO register map. I'm
> > > not sure how that would work out in U-Boot, but it is the starting point
> > > I would say.
> >
> > I can have my ksz dsa driver register an mdio bus for this and this is
> > probably the piece that I'm currently missing
>
> This is what I've been trying to tell you.
> Linux DSA offers some helpers for accessing internal PHYs, which in my
> opinion it should have not offered, see dsa_slave_mii_bus_init(). In
> particular it assumes that port i will have a PHY at MDIO address i.
> U-Boot DSA does not offer this helper, your only constructs are:
> (a) fixed-link: this means there is no PHY and that the MAC operates at
> a constant speed/duplex setting throughout the runtime of the device,
> with no means of link detection. This is probably what you want for
> the MAC-to-MAC link between the host port and the switch CPU port,
> but not what you want for the front-facing RJ45 ports which can
> switch speeds between 10/100/1000.
yes, this is what I want for the FEC to cpu-port (port5) link.
> (b) phy-handle: this is a phandle to a device tree description of a PHY
> device, be it an integrated PHY or a discrete one. Of course, your
> big problem is that you don't have a struct phy_device for your PHYs
> integrated in the KSZ switch, especially one with OF bindings.
>
yes, this is what I need for the lan1/lan2/lan3/lan4 ports.
> For reference, the sja1105 Linux driver also supports switches with
> internal PHYs, here's how it describes things in the device tree.
> Because on this switch, pinmuxing is complex, and certain ports like
> port 1 can either be connected to an internal 100base-TX PHY or to an
> SGMII PCS, describing the phy-handle in the device tree is necessary
> even if it is towards a struct phy_device registered by an MDIO bus
> provided by the same driver (instead of making the assumption that the
> pins are strapped one way or another in the driver itself).
>
> &spi_bridge {
> /* SW1 */
> ethernet-switch@0 {
> compatible = "nxp,sja1110a";
> reg = <0>;
> spi-max-frequency = <4000000>;
> spi-cpol;
> dsa,member = <0 0>;
>
> ethernet-ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> /* Microcontroller port */
> port@0 {
> reg = <0>;
> status = "disabled";
> };
>
> /* SW1_P1 */
> port@1 {
> reg = <1>;
> label = "con_2x20";
> phy-mode = "internal";
> phy-handle = <&sw1_port1_base_tx_phy>;
> };
>
> port@2 {
> reg = <2>;
> ethernet = <&dpmac17>;
> phy-mode = "rgmii-id";
>
> fixed-link {
> speed = <1000>;
> full-duplex;
> };
> };
>
> port@3 {
> reg = <3>;
> label = "1ge_p1";
> phy-mode = "rgmii-id";
> phy-handle = <&sw1_mii3_phy>;
> };
>
> sw1p4: port@4 {
> reg = <4>;
> link = <&sw2p1>;
> phy-mode = "sgmii";
>
> fixed-link {
> speed = <1000>;
> full-duplex;
> };
> };
>
> port@5 {
> reg = <5>;
> label = "trx1";
> phy-mode = "internal";
> phy-handle = <&sw1_port5_base_t1_phy>;
> };
>
> port@6 {
> reg = <6>;
> label = "trx2";
> phy-mode = "internal";
> phy-handle = <&sw1_port6_base_t1_phy>;
> };
>
> port@7 {
> reg = <7>;
> label = "trx3";
> phy-mode = "internal";
> phy-handle = <&sw1_port7_base_t1_phy>;
> };
>
> port@8 {
> reg = <8>;
> label = "trx4";
> phy-mode = "internal";
> phy-handle = <&sw1_port8_base_t1_phy>;
> };
>
> port@9 {
> reg = <9>;
> label = "trx5";
> phy-mode = "internal";
> phy-handle = <&sw1_port9_base_t1_phy>;
> };
>
> port@a {
> reg = <10>;
> label = "trx6";
> phy-mode = "internal";
> phy-handle = <&sw1_port10_base_t1_phy>;
> };
> };
>
> mdios {
> #address-cells = <1>;
> #size-cells = <0>;
>
> mdio@0 {
> reg = <0>;
> compatible = "nxp,sja1110-base-t1-mdio";
> #address-cells = <1>;
> #size-cells = <0>;
>
> sw1_port5_base_t1_phy: ethernet-phy@1 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x1>;
> };
>
> sw1_port6_base_t1_phy: ethernet-phy@2 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x2>;
> };
>
> sw1_port7_base_t1_phy: ethernet-phy@3 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x3>;
> };
>
> sw1_port8_base_t1_phy: ethernet-phy@4 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x4>;
> };
>
> sw1_port9_base_t1_phy: ethernet-phy@5 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x5>;
> };
>
> sw1_port10_base_t1_phy: ethernet-phy@6 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x6>;
> };
> };
>
> mdio@1 {
> reg = <1>;
> compatible = "nxp,sja1110-base-tx-mdio";
> #address-cells = <1>;
> #size-cells = <0>;
>
> sw1_port1_base_tx_phy: ethernet-phy@1 {
> reg = <0x1>;
> };
> };
> };
> };
> };
>
> [ please disregard the fact that there are 2 mdio buses, one for
> 100base-T1 and another for 100base-TX PHYs. There are reasons why that
> is, but I think you should only have one "mdio" node under the switch ]
>
> By following this example I believe you can model your ports with an
> internal PHY in pretty much the same way, while keeping the simple model
> that U-Boot DSA offers - and this should answer your other question too
> (fixed-link - no, phy-handle - yes). Please, let's keep the U-Boot DSA
> framework simple and without the bells and whistles from Linux, even if
> this means that the OF bindings for the switch need to be more descriptive
> and less free-form. This was a conscious design decision and I don't
> think it is limiting.
Interesting. So you're telling me that even though my current dt
bindings work and are correct for 'Linux DSA' I need to change them
(specifically, augment them) for U-Boot? This really seems wrong
unless perhaps the augmentation needed for U-Boot DSA is not
'incorrect' for Linux DSA.
This does explain how I'm supposed to bind the non-cpu ports to a phy.
So now I have:
&fec1 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_fec1>;
phy-mode = "rgmii-id";
local-mac-address = [00 00 00 00 00 00];
status = "okay";
fixed-link {
speed = <1000>;
full-duplex;
};
};
&i2c3 {
clock-frequency = <400000>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_i2c3>;
status = "okay";
switch: switch@5f {
compatible = "microchip,ksz9897";
reg = <0x5f>;
pinctrl-0 = <&pinctrl_ksz>;
interrupt-parent = <&gpio4>;
interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
ports {
#address-cells = <1>;
#size-cells = <0>;
lan1: port@0 {
reg = <0>;
label = "lan1";
local-mac-address = [00 00 00 00 00 00];
phy-handle = <&sw_phy0>;
phy-mode = "internal";
};
lan2: port@1 {
reg = <1>;
label = "lan2";
local-mac-address = [00 00 00 00 00 00];
phy-handle = <&sw_phy1>; // added for uboot
phy-mode = "internal"; // added for uboot
};
lan3: port@2 {
reg = <2>;
label = "lan3";
local-mac-address = [00 00 00 00 00 00];
phy-handle = <&sw_phy2>; // added for uboot
phy-mode = "internal"; // added for uboot
};
lan4: port@3 {
reg = <3>;
label = "lan4";
local-mac-address = [00 00 00 00 00 00];
phy-handle = <&sw_phy3>; // added for uboot
phy-mode = "internal"; // added for uboot
};
port@5 {
reg = <5>;
label = "cpu";
ethernet = <&fec1>;
phy-mode = "rgmii-id";
fixed-link {
speed = <1000>;
full-duplex;
};
};
};
/* added for U-Boot */
mdios {
#address-cells = <1>;
#size-cells = <0>;
mdio@0 {
reg = <0>;
compatible = "microchip,ksz-mdio";
#address-cells = <1>;
#size-cells = <0>;
sw_phy0: ethernet-phy@0 {
compatible =
"ethernet-phy-ieee8023-c45";
reg = <0x0>;
};
sw_phy1: ethernet-phy@1 {
compatible =
"ethernet-phy-ieee8023-c45";
reg = <0x1>;
};
sw_phy2: ethernet-phy@2 {
compatible =
"ethernet-phy-ieee8023-c45";
reg = <0x2>;
};
sw_phy3: ethernet-phy@3 {
compatible =
"ethernet-phy-ieee8023-c45";
reg = <0x3>;
};
};
};
};
};
I've added UCLASS_MDIO driver with compatible matching
'microchip,ksz-mdio' and had to add some code to my switch probe func
to probe the mdios:
ofnode node, mdios;
mdios = dev_read_subnode(dev, "mdios");
if (ofnode_valid(mdios)) {
ofnode_for_each_subnode(node, mdios) {
const char *name = ofnode_get_name(node);
struct udevice *pdev;
ret = device_bind_driver_to_node(dev,
KSZ_MDIO_CHILD_DRV_NAME, name, node, &pdev);
if (ret)
dev_err(dev, "failed to probe %s:
%d\n", name, ret);
}
}
I didn't see anything like that in your driver but I assume this was correct.
So now I have a ksz-mdio driver which is getting probed and I see it
properly reading phy_id registers for the non-cpu ports. I also have a
ksz-phy-driver and its probe is called after the non-cpu port phys are
identified. However I never do see the phy driver's
config/startup/shutdown functions get called and am still digging into
that.
I'm also seeing packets go out the non-cpu ports properly but nothing
is received from the fec driver and I'm thinking its because the fec
driver is setting up a MAC addr filter only allowing packets for its
MAC which clearly needs to be removed when used with a DSA driver I
would think?
Thanks for your help!
Tim
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: U-Boot DSA driver for microchip KSZ9477 suggestions needed
2021-06-24 0:28 ` Tim Harvey
@ 2021-06-24 2:05 ` Vladimir Oltean
2021-06-24 19:26 ` Tim Harvey
0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Oltean @ 2021-06-24 2:05 UTC (permalink / raw)
To: Tim Harvey
Cc: Vladimir Oltean, Alexandru Marginean, Claudiu Manoil,
Michael Walle, u-boot
On Wed, Jun 23, 2021 at 05:28:42PM -0700, Tim Harvey wrote:
> On Wed, Jun 23, 2021 at 3:37 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >
> > On Tue, Jun 22, 2021 at 09:38:52AM -0700, Tim Harvey wrote:
> > > On Mon, Jun 21, 2021 at 4:49 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > >
> > > > On Mon, Jun 21, 2021 at 04:10:51PM -0700, Tim Harvey wrote:
> > > > > Greetings,
> > > > >
> > > > > I've written a U-Boot phy driver for a Microchip KSZ9477 ethernet
> > > > > switch that I submitted some time ago as an RFC [1] which simply
> > > > > enables the ports in the dt and puts them in forwarding mode with link
> > > > > detect. However I think this would be much better suited as a DSA
> > > > > driver now that UCLASS_DSA exists.
> > > >
> > > > Define 'link detect'. I don't know how the switch-as-PHY drivers work.
> > > > I suppose the DSA master is made to connect to a PHY device which is
> > > > implemented by the switch driver, but whose port's link status is being
> > > > reported?
> > >
> > > Yes, each downward port's link status can be detected. Each port has
> > > standard GbE PHY registers which can be indirectly read.
> >
> > What do you mean by 'each port'?
> > With the switch-as-PHY driver, how many UCLASS_ETH devices are being
> > registered? One for the FEC and one for each front-facing switch port
> > (same as DSA would)? Only one, and that for the FEC? If the latter, how
> > does U-Boot know which of the front-facing switch ports to use as the
> > link status reporting for the FEC device?
>
> With my ksz9477 'switch-as-phy' driver
> (https://www.mail-archive.com/u-boot@lists.denx.de/msg389714.html)
> there is only 1 UCLASS_ETH device registered, just fec. The driver
> provides a UCLASS_ETH_PHY and its probe parses the dt to find the
> ports then registers an mdio bus for the indirect register access and
> registers a phy driver that matches the id's of the switch port.
>
> This only worked with another patch
> (https://www.mail-archive.com/u-boot@lists.denx.de/msg389712.html) to
> the UCLASS_ETH_PHY driver that allowed the phy driver to bind the the
> fec mac. Therefore this acts as a single phy on the MAC and when
> phy_config is called it sets up auto-negotiation on all the non-cpu
> ports and when phy_startup is called it performs link detect on all
> the non-cpu ports (if any port is linked its considered a link). The
> phy_config configured all the ports in forwarding mode.
>
> This was modeled after the drivers/net/phy/mv88e61xx.c (switch-as-phy)
> driver and the only difference between that is that it attempted to
> use driver-model and get its config from dt. The big downside of this
> method is that the switch is configured in forwarding mode so you end
> up with a bridge loop while active in U-Boot if you have more than one
> port connected to your network.
>
> I think if you look over that relatively simple driver it will make
> sense how the ksz switch works in case I'm explaining it wrong or with
> the wrong terminology.
Ah, ok, so it enables switching. Basically my misunderstanding was, if
the strategy is simply 'if any of the switch internal PHYs has link then
the phy-device exposed to the host port has link', how can this work and
not confuse the user badly. Say a cable is plugged, but not into the
port you are trying to ping over.
But you've answered my question, the packets are replicated by the
switch to all user ports if the situation demands it (FDB entry not
present for that MAC DA), and once the destination is learned, packets
will flow only towards the desired port.
I should have known better.
Actually there's a third type of switch driver now in U-Boot, which is
drivers/net/vsc9953.c which uses some overly bloated struct ethsw_command_func
operations and a dedicated "ethsw" U-Boot command to manage/call them.
This seems to be completely missing the point of U-Boot networking (what
user in their right mind needs to manage VLANs, the FDB, link
aggregation groups, ingress filtering from U-Boot?). With this "ethsw"
framework, it's a bit of a mix between the switch-as-PHY model and the
DM DSA model.
The switch is managed through a plain U-Boot command, so there's no DM
concept to speak of. The host port uses a fixed-link (same as DM DSA)
but is otherwise completely unaware that there's a switch there, as it
doesn't have a phy-handle to it. That's also an advantage, the switch
might not be MDIO based, so a phy-handle to a memory-based device is
kind of strange, and as you've seen in the change you made for the FEC,
it can be quite limiting. Side note, I do happen to have the hardware
for it, so I guess one day I should take the time to convert vsc9953.c
to the DM DSA model as well.
DM DSA follows the Linux mentality that switches are just ports with
some optional hardware acceleration. They behave and are used just like
regular ports, up to the point where the hwaccel features are stripped
to the bone and switching is not even supported because as you say it is
dangerous to leave it enabled without a network stack properly prepared
to deal with switch control protocols like STP.
The one big use case for networking in U-Boot is TFTP/friends to load
the next stage image and that is about it, whoever needs more is IMO
doing it wrong. So DM DSA is modeled around this idea precisely.
> > [ please disregard the fact that there are 2 mdio buses, one for
> > 100base-T1 and another for 100base-TX PHYs. There are reasons why that
> > is, but I think you should only have one "mdio" node under the switch ]
^remember what I said here, I'll reference it here [1]
> > By following this example I believe you can model your ports with an
> > internal PHY in pretty much the same way, while keeping the simple model
> > that U-Boot DSA offers - and this should answer your other question too
> > (fixed-link - no, phy-handle - yes). Please, let's keep the U-Boot DSA
> > framework simple and without the bells and whistles from Linux, even if
> > this means that the OF bindings for the switch need to be more descriptive
> > and less free-form. This was a conscious design decision and I don't
> > think it is limiting.
>
> Interesting. So you're telling me that even though my current dt
> bindings work and are correct for 'Linux DSA' I need to change them
> (specifically, augment them) for U-Boot? This really seems wrong
> unless perhaps the augmentation needed for U-Boot DSA is not
> 'incorrect' for Linux DSA.
Yeah, you got the point.
It seems wrong until you consider that Linux DSA has 13 years of
baggage, while U-Boot DM DSA is fresh and doesn't have to make all the
same mistakes again. Having OF bindings, but not having a fixed-link or
a phy-handle is ambiguous as to what the author meant to say. When Linux
DSA transitioned from phylib to phylink on the CPU port a few years ago,
this was a big problem, because the CPU port in some device trees was in
the exact same situation (no phy-handle, no fixed-link), and phylink
rightfully didn't know what to make of it. The (biased) interpretation
given back then was "if you don't have a fixed-link on a port, then it
just has an implicit fixed-link for the highest supported speed. And oh
yeah, only if it's a CPU port btw!"
https://lore.kernel.org/netdev/20200213144615.GH18808@shell.armlinux.org.uk/
Now you are saying "if a front-facing port does not have a fixed-link or
a phy-handle specifier, then it is connected to an internal PHY".
Obviously...
It gets more and more ambiguous each time the meaning is overloaded.
How about we just say "no" and just go with the generic format, which
covers this particular case too, even if more needs to be spelled out in
the DT? Again, maybe for some switch drivers there isn't much information
to be gained (the configuration is fixed, there is nothing that could
have not been hardcoded in the driver), but if there is any sort of
flexibility in the pinmuxing at all (say port 1 can go either to an
internal PHY or to a SGMII SERDES lane which is fixed at gigabit), then
the way the link is being reported/negotiated changes drastically and
you do need to tell the driver what to do.
If your concern is that the bindings might diverge between Linux and
U-Boot, I might be somewhat open to that argument (in the sense that I
don't get why it is a big deal, but I might be able to help with reviews
if you want to make the Linux KSZ driver support the fully-OF method of
connecting to the internal PHY).
> This does explain how I'm supposed to bind the non-cpu ports to a phy.
> So now I have:
>
> &fec1 {
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_fec1>;
> phy-mode = "rgmii-id";
> local-mac-address = [00 00 00 00 00 00];
> status = "okay";
>
> fixed-link {
> speed = <1000>;
> full-duplex;
> };
> };
>
> &i2c3 {
> clock-frequency = <400000>;
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_i2c3>;
> status = "okay";
>
> switch: switch@5f {
> compatible = "microchip,ksz9897";
> reg = <0x5f>;
> pinctrl-0 = <&pinctrl_ksz>;
> interrupt-parent = <&gpio4>;
> interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> lan1: port@0 {
> reg = <0>;
> label = "lan1";
> local-mac-address = [00 00 00 00 00 00];
> phy-handle = <&sw_phy0>;
> phy-mode = "internal";
> };
>
> lan2: port@1 {
> reg = <1>;
> label = "lan2";
> local-mac-address = [00 00 00 00 00 00];
> phy-handle = <&sw_phy1>; // added for uboot
> phy-mode = "internal"; // added for uboot
> };
>
> lan3: port@2 {
> reg = <2>;
> label = "lan3";
> local-mac-address = [00 00 00 00 00 00];
> phy-handle = <&sw_phy2>; // added for uboot
> phy-mode = "internal"; // added for uboot
> };
>
> lan4: port@3 {
> reg = <3>;
> label = "lan4";
> local-mac-address = [00 00 00 00 00 00];
> phy-handle = <&sw_phy3>; // added for uboot
> phy-mode = "internal"; // added for uboot
> };
>
> port@5 {
> reg = <5>;
> label = "cpu";
> ethernet = <&fec1>;
> phy-mode = "rgmii-id";
>
> fixed-link {
> speed = <1000>;
> full-duplex;
> };
> };
> };
>
> /* added for U-Boot */
> mdios {
> #address-cells = <1>;
> #size-cells = <0>;
>
> mdio@0 {
> reg = <0>;
> compatible = "microchip,ksz-mdio";
> #address-cells = <1>;
> #size-cells = <0>;
[1] So if you are known to have a single MDIO bus, then as mentioned,
instead of going
ethernet-switch {
mdios {
mdio@0 {
ethernet-phy@0 {
};
};
};
};
you can probably just do
ethernet-switch {
mdio {
ethernet-phy@0 {
};
};
};
>
> sw_phy0: ethernet-phy@0 {
> compatible = "ethernet-phy-ieee8023-c45";
This compatible string is for clause 45 PHYs, are your PHYs clause 45
(is their addressing scheme two-layered, with an MMD and a register
address, or just a register address)? It's likely that this compatible
string is simply ignored by U-Boot, you should remove it.
> reg = <0x0>;
> };
>
> sw_phy1: ethernet-phy@1 {
> compatible = "ethernet-phy-ieee8023-c45";
> reg = <0x1>;
> };
> sw_phy2: ethernet-phy@2 {
> compatible = "ethernet-phy-ieee8023-c45";
> reg = <0x2>;
> };
>
> sw_phy3: ethernet-phy@3 {
> compatible = "ethernet-phy-ieee8023-c45";
> reg = <0x3>;
> };
> };
> };
> };
> };
>
> I've added UCLASS_MDIO driver with compatible matching
> 'microchip,ksz-mdio' and had to add some code to my switch probe func
> to probe the mdios:
>
> ofnode node, mdios;
> mdios = dev_read_subnode(dev, "mdios");
> if (ofnode_valid(mdios)) {
> ofnode_for_each_subnode(node, mdios) {
> const char *name = ofnode_get_name(node);
> struct udevice *pdev;
>
> ret = device_bind_driver_to_node(dev,
> KSZ_MDIO_CHILD_DRV_NAME, name, node, &pdev);
> if (ret)
> dev_err(dev, "failed to probe %s: %d\n", name, ret);
> }
> }
>
> I didn't see anything like that in your driver but I assume this was correct.
I should have assumed you were going to search the Dec 2019 submission
for it. The answer is that only the NXP SJA1110 has internal PHYs, and
the SJA1110 is not covered by that 2019 patch, only the older SJA1105 is.
It might be when I find some time to resend.
> So now I have a ksz-mdio driver which is getting probed and I see it
> properly reading phy_id registers for the non-cpu ports. I also have a
> ksz-phy-driver and its probe is called after the non-cpu port phys are
> identified. However I never do see the phy driver's
> config/startup/shutdown functions get called and am still digging into
> that.
Your switch driver needs to call these functions, does it do that?
See felix_port_enable().
> I'm also seeing packets go out the non-cpu ports properly but nothing
> is received from the fec driver and I'm thinking its because the fec
> driver is setting up a MAC addr filter only allowing packets for its
> MAC which clearly needs to be removed when used with a DSA driver I
> would think?
That seems highly plausible, but why? If you look inside dsa_port_probe()
you'll find there is logic specifically for that: by default, DSA ports
inherit the MAC address of their DSA master (FEC for you) if their env
variable is not populated. Does that not work for you? How does the DSA
port end up having a different MAC address compared to the FEC? Are you
working with CONFIG_NET_RANDOM_ETHADDR enabled? But it should work even
then. Have you manually changed the ethNaddr env variables?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: U-Boot DSA driver for microchip KSZ9477 suggestions needed
2021-06-24 2:05 ` Vladimir Oltean
@ 2021-06-24 19:26 ` Tim Harvey
2021-06-29 8:09 ` Vladimir Oltean
0 siblings, 1 reply; 8+ messages in thread
From: Tim Harvey @ 2021-06-24 19:26 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Vladimir Oltean, Alexandru Marginean, Claudiu Manoil,
Michael Walle, u-boot
On Wed, Jun 23, 2021 at 7:05 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Wed, Jun 23, 2021 at 05:28:42PM -0700, Tim Harvey wrote:
> > On Wed, Jun 23, 2021 at 3:37 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> > >
> > > On Tue, Jun 22, 2021 at 09:38:52AM -0700, Tim Harvey wrote:
> > > > On Mon, Jun 21, 2021 at 4:49 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jun 21, 2021 at 04:10:51PM -0700, Tim Harvey wrote:
> > > > > > Greetings,
> > > > > >
> > > > > > I've written a U-Boot phy driver for a Microchip KSZ9477 ethernet
> > > > > > switch that I submitted some time ago as an RFC [1] which simply
> > > > > > enables the ports in the dt and puts them in forwarding mode with link
> > > > > > detect. However I think this would be much better suited as a DSA
> > > > > > driver now that UCLASS_DSA exists.
> > > > >
> > > > > Define 'link detect'. I don't know how the switch-as-PHY drivers work.
> > > > > I suppose the DSA master is made to connect to a PHY device which is
> > > > > implemented by the switch driver, but whose port's link status is being
> > > > > reported?
> > > >
> > > > Yes, each downward port's link status can be detected. Each port has
> > > > standard GbE PHY registers which can be indirectly read.
> > >
> > > What do you mean by 'each port'?
> > > With the switch-as-PHY driver, how many UCLASS_ETH devices are being
> > > registered? One for the FEC and one for each front-facing switch port
> > > (same as DSA would)? Only one, and that for the FEC? If the latter, how
> > > does U-Boot know which of the front-facing switch ports to use as the
> > > link status reporting for the FEC device?
> >
> > With my ksz9477 'switch-as-phy' driver
> > (https://www.mail-archive.com/u-boot@lists.denx.de/msg389714.html)
> > there is only 1 UCLASS_ETH device registered, just fec. The driver
> > provides a UCLASS_ETH_PHY and its probe parses the dt to find the
> > ports then registers an mdio bus for the indirect register access and
> > registers a phy driver that matches the id's of the switch port.
> >
> > This only worked with another patch
> > (https://www.mail-archive.com/u-boot@lists.denx.de/msg389712.html) to
> > the UCLASS_ETH_PHY driver that allowed the phy driver to bind the the
> > fec mac. Therefore this acts as a single phy on the MAC and when
> > phy_config is called it sets up auto-negotiation on all the non-cpu
> > ports and when phy_startup is called it performs link detect on all
> > the non-cpu ports (if any port is linked its considered a link). The
> > phy_config configured all the ports in forwarding mode.
> >
> > This was modeled after the drivers/net/phy/mv88e61xx.c (switch-as-phy)
> > driver and the only difference between that is that it attempted to
> > use driver-model and get its config from dt. The big downside of this
> > method is that the switch is configured in forwarding mode so you end
> > up with a bridge loop while active in U-Boot if you have more than one
> > port connected to your network.
> >
> > I think if you look over that relatively simple driver it will make
> > sense how the ksz switch works in case I'm explaining it wrong or with
> > the wrong terminology.
>
> Ah, ok, so it enables switching. Basically my misunderstanding was, if
> the strategy is simply 'if any of the switch internal PHYs has link then
> the phy-device exposed to the host port has link', how can this work and
> not confuse the user badly. Say a cable is plugged, but not into the
> port you are trying to ping over.
agreed, this is confusing but this is how the mv88e61xx.c
switch-as-phy driver works which I modeled my switch-as-phy driver
from.
From a user perspective with this kind of switch-as-phy approach
network traffic works through any port with a link due to the switch
being in forwarding mode and there is no need to know what port is
active but personally if you are using DSA in Linux such that each
port has a network interface then I do like the model of doing the
same in U-Boot.
>
> But you've answered my question, the packets are replicated by the
> switch to all user ports if the situation demands it (FDB entry not
> present for that MAC DA), and once the destination is learned, packets
> will flow only towards the desired port.
>
> I should have known better.
>
> Actually there's a third type of switch driver now in U-Boot, which is
> drivers/net/vsc9953.c which uses some overly bloated struct ethsw_command_func
> operations and a dedicated "ethsw" U-Boot command to manage/call them.
> This seems to be completely missing the point of U-Boot networking (what
> user in their right mind needs to manage VLANs, the FDB, link
> aggregation groups, ingress filtering from U-Boot?). With this "ethsw"
> framework, it's a bit of a mix between the switch-as-PHY model and the
> DM DSA model.
>
> The switch is managed through a plain U-Boot command, so there's no DM
> concept to speak of. The host port uses a fixed-link (same as DM DSA)
> but is otherwise completely unaware that there's a switch there, as it
> doesn't have a phy-handle to it. That's also an advantage, the switch
> might not be MDIO based, so a phy-handle to a memory-based device is
> kind of strange, and as you've seen in the change you made for the FEC,
> it can be quite limiting. Side note, I do happen to have the hardware
> for it, so I guess one day I should take the time to convert vsc9953.c
> to the DM DSA model as well.
this seems like a hack that was simply done because it pre-dated U-Boot DSA.
>
> DM DSA follows the Linux mentality that switches are just ports with
> some optional hardware acceleration. They behave and are used just like
> regular ports, up to the point where the hwaccel features are stripped
> to the bone and switching is not even supported because as you say it is
> dangerous to leave it enabled without a network stack properly prepared
> to deal with switch control protocols like STP.
> The one big use case for networking in U-Boot is TFTP/friends to load
> the next stage image and that is about it, whoever needs more is IMO
> doing it wrong. So DM DSA is modeled around this idea precisely.
>
> > > [ please disregard the fact that there are 2 mdio buses, one for
> > > 100base-T1 and another for 100base-TX PHYs. There are reasons why that
> > > is, but I think you should only have one "mdio" node under the switch ]
>
> ^remember what I said here, I'll reference it here [1]
>
> > > By following this example I believe you can model your ports with an
> > > internal PHY in pretty much the same way, while keeping the simple model
> > > that U-Boot DSA offers - and this should answer your other question too
> > > (fixed-link - no, phy-handle - yes). Please, let's keep the U-Boot DSA
> > > framework simple and without the bells and whistles from Linux, even if
> > > this means that the OF bindings for the switch need to be more descriptive
> > > and less free-form. This was a conscious design decision and I don't
> > > think it is limiting.
> >
> > Interesting. So you're telling me that even though my current dt
> > bindings work and are correct for 'Linux DSA' I need to change them
> > (specifically, augment them) for U-Boot? This really seems wrong
> > unless perhaps the augmentation needed for U-Boot DSA is not
> > 'incorrect' for Linux DSA.
>
> Yeah, you got the point.
>
> It seems wrong until you consider that Linux DSA has 13 years of
> baggage, while U-Boot DM DSA is fresh and doesn't have to make all the
> same mistakes again. Having OF bindings, but not having a fixed-link or
> a phy-handle is ambiguous as to what the author meant to say. When Linux
> DSA transitioned from phylib to phylink on the CPU port a few years ago,
> this was a big problem, because the CPU port in some device trees was in
> the exact same situation (no phy-handle, no fixed-link), and phylink
> rightfully didn't know what to make of it. The (biased) interpretation
> given back then was "if you don't have a fixed-link on a port, then it
> just has an implicit fixed-link for the highest supported speed. And oh
> yeah, only if it's a CPU port btw!"
> https://lore.kernel.org/netdev/20200213144615.GH18808@shell.armlinux.org.uk/
> Now you are saying "if a front-facing port does not have a fixed-link or
> a phy-handle specifier, then it is connected to an internal PHY".
> Obviously...
>
> It gets more and more ambiguous each time the meaning is overloaded.
> How about we just say "no" and just go with the generic format, which
> covers this particular case too, even if more needs to be spelled out in
> the DT? Again, maybe for some switch drivers there isn't much information
> to be gained (the configuration is fixed, there is nothing that could
> have not been hardcoded in the driver), but if there is any sort of
> flexibility in the pinmuxing at all (say port 1 can go either to an
> internal PHY or to a SGMII SERDES lane which is fixed at gigabit), then
> the way the link is being reported/negotiated changes drastically and
> you do need to tell the driver what to do.
>
> If your concern is that the bindings might diverge between Linux and
> U-Boot, I might be somewhat open to that argument (in the sense that I
> don't get why it is a big deal, but I might be able to help with reviews
> if you want to make the Linux KSZ driver support the fully-OF method of
> connecting to the internal PHY).
>
I'm ok with it. The gw7901 dts is already upstream and working with
the Linux DSA drivers
(https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mm-venice-gw7901.dts?h=for-next).
The way I see it by adding the phy-handle and phy-mode props to the
non-cpu ports and adding an mdio device I'm only adding more clarity
to the dt for U-Boot and the binding changes would not affect Linux
DSA drivers if somehow they were used for Linux.
> > This does explain how I'm supposed to bind the non-cpu ports to a phy.
> > So now I have:
> >
> > &fec1 {
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_fec1>;
> > phy-mode = "rgmii-id";
> > local-mac-address = [00 00 00 00 00 00];
> > status = "okay";
> >
> > fixed-link {
> > speed = <1000>;
> > full-duplex;
> > };
> > };
> >
> > &i2c3 {
> > clock-frequency = <400000>;
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_i2c3>;
> > status = "okay";
> >
> > switch: switch@5f {
> > compatible = "microchip,ksz9897";
> > reg = <0x5f>;
> > pinctrl-0 = <&pinctrl_ksz>;
> > interrupt-parent = <&gpio4>;
> > interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
> >
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > lan1: port@0 {
> > reg = <0>;
> > label = "lan1";
> > local-mac-address = [00 00 00 00 00 00];
> > phy-handle = <&sw_phy0>;
> > phy-mode = "internal";
> > };
> >
> > lan2: port@1 {
> > reg = <1>;
> > label = "lan2";
> > local-mac-address = [00 00 00 00 00 00];
> > phy-handle = <&sw_phy1>; // added for uboot
> > phy-mode = "internal"; // added for uboot
> > };
> >
> > lan3: port@2 {
> > reg = <2>;
> > label = "lan3";
> > local-mac-address = [00 00 00 00 00 00];
> > phy-handle = <&sw_phy2>; // added for uboot
> > phy-mode = "internal"; // added for uboot
> > };
> >
> > lan4: port@3 {
> > reg = <3>;
> > label = "lan4";
> > local-mac-address = [00 00 00 00 00 00];
> > phy-handle = <&sw_phy3>; // added for uboot
> > phy-mode = "internal"; // added for uboot
> > };
> >
> > port@5 {
> > reg = <5>;
> > label = "cpu";
> > ethernet = <&fec1>;
> > phy-mode = "rgmii-id";
> >
> > fixed-link {
> > speed = <1000>;
> > full-duplex;
> > };
> > };
> > };
> >
> > /* added for U-Boot */
> > mdios {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > mdio@0 {
> > reg = <0>;
> > compatible = "microchip,ksz-mdio";
> > #address-cells = <1>;
> > #size-cells = <0>;
>
> [1] So if you are known to have a single MDIO bus, then as mentioned,
> instead of going
>
> ethernet-switch {
> mdios {
> mdio@0 {
> ethernet-phy@0 {
> };
> };
> };
> };
>
> you can probably just do
>
> ethernet-switch {
> mdio {
> ethernet-phy@0 {
> };
> };
> };
>
I wonder though if for consistency of U-Boot DSA dt's it would be best
to encapsulate the 1 or more mdio's in an 'mdios' child. The dsa
driver needs to specifically bind the mdio driver and at least that
code can look the same for various dsa drivers, or perhaps that code
can even be put in uclass-dsa.
Perhaps I made the binding of the mdio driver overly-complicated. This
is what I'm doing to bind any children matching a specific compatible
within the 'mdios' child of the switch:
#define KSZ_MDIO_CHILD_DRV_NAME "ksz_mdio"
static const struct udevice_id ksz_mdio_ids[] = {
{ .compatible = "microchip,ksz-mdio" },
{ }
};
U_BOOT_DRIVER(ksz_mdio) = {
.name = KSZ_MDIO_CHILD_DRV_NAME,
.id = UCLASS_MDIO,
.of_match = ksz_mdio_ids,
.bind = ksz_mdio_bind,
.probe = ksz_mdio_probe,
.ops = &ksz_mdio_ops,
.priv_auto = sizeof(struct ksz_mdio_priv),
.plat_auto = sizeof(struct mdio_perdev_priv),
};
static int ksz_i2c_probe(struct udevice *dev)
{
...
ofnode node, mdios;
mdios = dev_read_subnode(dev, "mdios");
if (ofnode_valid(mdios)) {
ofnode_for_each_subnode(node, mdios) {
const char *name = ofnode_get_name(node);
struct udevice *pdev;
ret = device_bind_driver_to_node(dev,
KSZ_MDIO_CHILD_DRV_NAME, name, node, &pdev);
if (ret)
dev_err(dev, "failed to probe %s:
%d\n", name, ret);
}
}
...
}
> >
> > sw_phy0: ethernet-phy@0 {
> > compatible = "ethernet-phy-ieee8023-c45";
>
> This compatible string is for clause 45 PHYs, are your PHYs clause 45
> (is their addressing scheme two-layered, with an MMD and a register
> address, or just a register address)? It's likely that this compatible
> string is simply ignored by U-Boot, you should remove it.
>
ok - when I remove it the genphy driver continues to work for per-port
link detect
> > reg = <0x0>;
> > };
> >
> > sw_phy1: ethernet-phy@1 {
> > compatible = "ethernet-phy-ieee8023-c45";
> > reg = <0x1>;
> > };
> > sw_phy2: ethernet-phy@2 {
> > compatible = "ethernet-phy-ieee8023-c45";
> > reg = <0x2>;
> > };
> >
> > sw_phy3: ethernet-phy@3 {
> > compatible = "ethernet-phy-ieee8023-c45";
> > reg = <0x3>;
> > };
> > };
> > };
> > };
> > };
> >
> > I've added UCLASS_MDIO driver with compatible matching
> > 'microchip,ksz-mdio' and had to add some code to my switch probe func
> > to probe the mdios:
> >
> > ofnode node, mdios;
> > mdios = dev_read_subnode(dev, "mdios");
> > if (ofnode_valid(mdios)) {
> > ofnode_for_each_subnode(node, mdios) {
> > const char *name = ofnode_get_name(node);
> > struct udevice *pdev;
> >
> > ret = device_bind_driver_to_node(dev,
> > KSZ_MDIO_CHILD_DRV_NAME, name, node, &pdev);
> > if (ret)
> > dev_err(dev, "failed to probe %s: %d\n", name, ret);
> > }
> > }
> >
> > I didn't see anything like that in your driver but I assume this was correct.
>
> I should have assumed you were going to search the Dec 2019 submission
> for it. The answer is that only the NXP SJA1110 has internal PHYs, and
> the SJA1110 is not covered by that 2019 patch, only the older SJA1105 is.
> It might be when I find some time to resend.
>
> > So now I have a ksz-mdio driver which is getting probed and I see it
> > properly reading phy_id registers for the non-cpu ports. I also have a
> > ksz-phy-driver and its probe is called after the non-cpu port phys are
> > identified. However I never do see the phy driver's
> > config/startup/shutdown functions get called and am still digging into
> > that.
>
> Your switch driver needs to call these functions, does it do that?
> See felix_port_enable().
Yes, that's what I needed - thanks for pointing that out!
Now per-port auto negotiation and link detect is working for me. I do
find that if a non-cpu port has no link the network transaction
continues on like it does have a link so there is likely a check that
should be added somewhere. I will look into that.
>
> > I'm also seeing packets go out the non-cpu ports properly but nothing
> > is received from the fec driver and I'm thinking its because the fec
> > driver is setting up a MAC addr filter only allowing packets for its
> > MAC which clearly needs to be removed when used with a DSA driver I
> > would think?
>
> That seems highly plausible, but why? If you look inside dsa_port_probe()
> you'll find there is logic specifically for that: by default, DSA ports
> inherit the MAC address of their DSA master (FEC for you) if their env
> variable is not populated. Does that not work for you? How does the DSA
> port end up having a different MAC address compared to the FEC? Are you
> working with CONFIG_NET_RANDOM_ETHADDR enabled? But it should work even
> then. Have you manually changed the ethNaddr env variables?
I do not use CONFIG_NET_RANDOM_ETHADDR but I do have env addr env for
each mac as for Linux DSA we want each non-cpu port to have a unique
MAC with the board mfg OID so this was the culprit. Setting the
eth*addr env vars and having a local-mac-address prop in the non-cpu
ports is a way of passing that mac address along from U-Boot to Linux
thus I would loose that capability if I didn't set that env. Now that
I understand the issue and realize that the non-cpu ports 'must' share
the MAC of the master, why wouldn't that just be enforced by
uclass-dsa? What is the reasoning behind allowing env to set them?
So now that all of my ports are working with auto-negotiation and link
detect I suppose the final thing I need to do is add tagging or vlanid
to be able to tell dsa what port the received packets came from.
Thanks,
Tim
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: U-Boot DSA driver for microchip KSZ9477 suggestions needed
2021-06-24 19:26 ` Tim Harvey
@ 2021-06-29 8:09 ` Vladimir Oltean
0 siblings, 0 replies; 8+ messages in thread
From: Vladimir Oltean @ 2021-06-29 8:09 UTC (permalink / raw)
To: tharvey
Cc: Vladimir Oltean, Alexandru Marginean, Claudiu Manoil,
Michael Walle, u-boot
On Thu, Jun 24, 2021 at 12:26:53PM -0700, Tim Harvey wrote:
> > [1] So if you are known to have a single MDIO bus, then as mentioned,
> > instead of going
> >
> > ethernet-switch {
> > mdios {
> > mdio@0 {
> > ethernet-phy@0 {
> > };
> > };
> > };
> > };
> >
> > you can probably just do
> >
> > ethernet-switch {
> > mdio {
> > ethernet-phy@0 {
> > };
> > };
> > };
> >
>
> I wonder though if for consistency of U-Boot DSA dt's it would be best
> to encapsulate the 1 or more mdio's in an 'mdios' child. The dsa
> driver needs to specifically bind the mdio driver and at least that
> code can look the same for various dsa drivers, or perhaps that code
> can even be put in uclass-dsa.
If it is a conscious decision then I don't disagree with it. Embedding
the potentially more than one MDIO controller under the same "mdios"
node will lead to more uniform bindings.
Andrew Lunn explained here that even "marvell,mv88e6xxx-mdio-external"
could have benefited from a better organization of multiple MDIO buses,
given some forethought:
https://patchwork.kernel.org/project/netdevbpf/patch/20210524232214.1378937-12-olteanv@gmail.com/
Nonetheless, now they have an "mdio" and an "mdio1" node under the
switch, with "mdio1" not going through the YAML validator. See
arm/boot/dts/vf610-zii-ssmb-dtu.dts in Linux.
> Perhaps I made the binding of the mdio driver overly-complicated. This
> is what I'm doing to bind any children matching a specific compatible
> within the 'mdios' child of the switch:
>
> #define KSZ_MDIO_CHILD_DRV_NAME "ksz_mdio"
>
> static const struct udevice_id ksz_mdio_ids[] = {
> { .compatible = "microchip,ksz-mdio" },
> { }
> };
>
> U_BOOT_DRIVER(ksz_mdio) = {
> .name = KSZ_MDIO_CHILD_DRV_NAME,
> .id = UCLASS_MDIO,
> .of_match = ksz_mdio_ids,
> .bind = ksz_mdio_bind,
> .probe = ksz_mdio_probe,
> .ops = &ksz_mdio_ops,
> .priv_auto = sizeof(struct ksz_mdio_priv),
> .plat_auto = sizeof(struct mdio_perdev_priv),
> };
>
> static int ksz_i2c_probe(struct udevice *dev)
> {
> ...
> ofnode node, mdios;
> mdios = dev_read_subnode(dev, "mdios");
> if (ofnode_valid(mdios)) {
> ofnode_for_each_subnode(node, mdios) {
> const char *name = ofnode_get_name(node);
> struct udevice *pdev;
>
> ret = device_bind_driver_to_node(dev,
> KSZ_MDIO_CHILD_DRV_NAME, name, node, &pdev);
Don't you have to know the drv_name beforehand, making this tricky to
implement centrally in DM DSA?
If you want it to be common I suppose what you can do is you can offer
it as a library service (driver can optionally call this function
provided by DSA with its drv_name as argument).
> if (ret)
> dev_err(dev, "failed to probe %s: %d\n", name, ret);
> }
> }
> ...
> }
>
>
> > >
> > > sw_phy0: ethernet-phy@0 {
> > > compatible = "ethernet-phy-ieee8023-c45";
> >
> > This compatible string is for clause 45 PHYs, are your PHYs clause 45
> > (is their addressing scheme two-layered, with an MMD and a register
> > address, or just a register address)? It's likely that this compatible
> > string is simply ignored by U-Boot, you should remove it.
> >
>
> ok - when I remove it the genphy driver continues to work for per-port
> link detect
>
> > > reg = <0x0>;
> > > };
> > >
> > > sw_phy1: ethernet-phy@1 {
> > > compatible = "ethernet-phy-ieee8023-c45";
> > > reg = <0x1>;
> > > };
> > > sw_phy2: ethernet-phy@2 {
> > > compatible = "ethernet-phy-ieee8023-c45";
> > > reg = <0x2>;
> > > };
> > >
> > > sw_phy3: ethernet-phy@3 {
> > > compatible = "ethernet-phy-ieee8023-c45";
> > > reg = <0x3>;
> > > };
> > > };
> > > };
> > > };
> > > };
> > >
> > > I've added UCLASS_MDIO driver with compatible matching
> > > 'microchip,ksz-mdio' and had to add some code to my switch probe func
> > > to probe the mdios:
> > >
> > > ofnode node, mdios;
> > > mdios = dev_read_subnode(dev, "mdios");
> > > if (ofnode_valid(mdios)) {
> > > ofnode_for_each_subnode(node, mdios) {
> > > const char *name = ofnode_get_name(node);
> > > struct udevice *pdev;
> > >
> > > ret = device_bind_driver_to_node(dev,
> > > KSZ_MDIO_CHILD_DRV_NAME, name, node, &pdev);
> > > if (ret)
> > > dev_err(dev, "failed to probe %s: %d\n", name, ret);
> > > }
> > > }
> > >
> > > I didn't see anything like that in your driver but I assume this was correct.
> >
> > I should have assumed you were going to search the Dec 2019 submission
> > for it. The answer is that only the NXP SJA1110 has internal PHYs, and
> > the SJA1110 is not covered by that 2019 patch, only the older SJA1105 is.
> > It might be when I find some time to resend.
> >
> > > So now I have a ksz-mdio driver which is getting probed and I see it
> > > properly reading phy_id registers for the non-cpu ports. I also have a
> > > ksz-phy-driver and its probe is called after the non-cpu port phys are
> > > identified. However I never do see the phy driver's
> > > config/startup/shutdown functions get called and am still digging into
> > > that.
> >
> > Your switch driver needs to call these functions, does it do that?
> > See felix_port_enable().
>
> Yes, that's what I needed - thanks for pointing that out!
>
> Now per-port auto negotiation and link detect is working for me. I do
> find that if a non-cpu port has no link the network transaction
> continues on like it does have a link so there is likely a check that
> should be added somewhere. I will look into that.
I suspect you will need a KSZ PHY specific driver and the genphy one
will not be enough.
> >
> > > I'm also seeing packets go out the non-cpu ports properly but nothing
> > > is received from the fec driver and I'm thinking its because the fec
> > > driver is setting up a MAC addr filter only allowing packets for its
> > > MAC which clearly needs to be removed when used with a DSA driver I
> > > would think?
> >
> > That seems highly plausible, but why? If you look inside dsa_port_probe()
> > you'll find there is logic specifically for that: by default, DSA ports
> > inherit the MAC address of their DSA master (FEC for you) if their env
> > variable is not populated. Does that not work for you? How does the DSA
> > port end up having a different MAC address compared to the FEC? Are you
> > working with CONFIG_NET_RANDOM_ETHADDR enabled? But it should work even
> > then. Have you manually changed the ethNaddr env variables?
>
> I do not use CONFIG_NET_RANDOM_ETHADDR but I do have env addr env for
> each mac as for Linux DSA we want each non-cpu port to have a unique
> MAC with the board mfg OID so this was the culprit. Setting the
> eth*addr env vars and having a local-mac-address prop in the non-cpu
> ports is a way of passing that mac address along from U-Boot to Linux
> thus I would loose that capability if I didn't set that env. Now that
> I understand the issue and realize that the non-cpu ports 'must' share
> the MAC of the master, why wouldn't that just be enforced by
> uclass-dsa? What is the reasoning behind allowing env to set them?
The reasoning behind allowing the env to set individual MAC addresses
per DSA port is to make people like you happy (don't you want the board
to send packets with the same source MAC address in U-Boot as in Linux?).
One key thing you need to understand is that there are various kinds of
DSA tagging protocols - basically places where the switch expects the
tag to be put, and how it looks like. For felix_switch.c (the only piece
of hardware supported by DM DSA today), the DSA tag is placed in front
of the Ethernet header, and it starts with 6 bytes of ff:ff:ff:ff:ff:ff.
The DSA master perceives this as being the MAC address of all ingress
packets, whereas the MAC address of the individual switch ports is
merely payload in this system (it is shifted to the right by an amount
of bytes equal to the DSA tag length). So the DSA master doesn't have
any RX filtering issues.
Whereas in your case, the MAC address that the DSA master sees is
precisely the MAC address of the switch port. In Linux, what we did is
we added a "bool promisc_on_master" in struct dsa_device_ops.
I recommend that you introduce a new "set_promisc" function pointer to
struct eth_ops, you implement it for the FEC driver, then you make DSA
call it on the DSA master if available. Managing address filters is not
really justified.
> So now that all of my ports are working with auto-negotiation and link
> detect I suppose the final thing I need to do is add tagging or vlanid
> to be able to tell dsa what port the received packets came from.
>
> Thanks,
>
> Tim
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-06-29 8:09 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-21 23:10 U-Boot DSA driver for microchip KSZ9477 suggestions needed Tim Harvey
2021-06-21 23:49 ` Vladimir Oltean
2021-06-22 16:38 ` Tim Harvey
2021-06-23 10:37 ` Vladimir Oltean
2021-06-24 0:28 ` Tim Harvey
2021-06-24 2:05 ` Vladimir Oltean
2021-06-24 19:26 ` Tim Harvey
2021-06-29 8:09 ` Vladimir Oltean
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.