U-Boot DSA driver for microchip KSZ9477 suggestions needed
Vladimir Oltean
vladimir.oltean at nxp.com
Wed Jun 23 12:37:05 CEST 2021
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 at 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 at 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 at 0 {
reg = <0>;
status = "disabled";
};
/* SW1_P1 */
port at 1 {
reg = <1>;
label = "con_2x20";
phy-mode = "internal";
phy-handle = <&sw1_port1_base_tx_phy>;
};
port at 2 {
reg = <2>;
ethernet = <&dpmac17>;
phy-mode = "rgmii-id";
fixed-link {
speed = <1000>;
full-duplex;
};
};
port at 3 {
reg = <3>;
label = "1ge_p1";
phy-mode = "rgmii-id";
phy-handle = <&sw1_mii3_phy>;
};
sw1p4: port at 4 {
reg = <4>;
link = <&sw2p1>;
phy-mode = "sgmii";
fixed-link {
speed = <1000>;
full-duplex;
};
};
port at 5 {
reg = <5>;
label = "trx1";
phy-mode = "internal";
phy-handle = <&sw1_port5_base_t1_phy>;
};
port at 6 {
reg = <6>;
label = "trx2";
phy-mode = "internal";
phy-handle = <&sw1_port6_base_t1_phy>;
};
port at 7 {
reg = <7>;
label = "trx3";
phy-mode = "internal";
phy-handle = <&sw1_port7_base_t1_phy>;
};
port at 8 {
reg = <8>;
label = "trx4";
phy-mode = "internal";
phy-handle = <&sw1_port8_base_t1_phy>;
};
port at 9 {
reg = <9>;
label = "trx5";
phy-mode = "internal";
phy-handle = <&sw1_port9_base_t1_phy>;
};
port at a {
reg = <10>;
label = "trx6";
phy-mode = "internal";
phy-handle = <&sw1_port10_base_t1_phy>;
};
};
mdios {
#address-cells = <1>;
#size-cells = <0>;
mdio at 0 {
reg = <0>;
compatible = "nxp,sja1110-base-t1-mdio";
#address-cells = <1>;
#size-cells = <0>;
sw1_port5_base_t1_phy: ethernet-phy at 1 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x1>;
};
sw1_port6_base_t1_phy: ethernet-phy at 2 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x2>;
};
sw1_port7_base_t1_phy: ethernet-phy at 3 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x3>;
};
sw1_port8_base_t1_phy: ethernet-phy at 4 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x4>;
};
sw1_port9_base_t1_phy: ethernet-phy at 5 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x5>;
};
sw1_port10_base_t1_phy: ethernet-phy at 6 {
compatible = "ethernet-phy-ieee802.3-c45";
reg = <0x6>;
};
};
mdio at 1 {
reg = <1>;
compatible = "nxp,sja1110-base-tx-mdio";
#address-cells = <1>;
#size-cells = <0>;
sw1_port1_base_tx_phy: ethernet-phy at 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.
More information about the U-Boot
mailing list