[PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY
Tim Harvey
tharvey at gateworks.com
Tue Mar 29 00:23:02 CEST 2022
On Mon, Mar 28, 2022 at 2:26 AM Vladimir Oltean <vladimir.oltean at nxp.com> wrote:
>
> On Fri, Mar 25, 2022 at 02:03:56PM -0700, Tim Harvey wrote:
> > On Fri, Mar 25, 2022 at 11:07 AM Vladimir Oltean
> > <vladimir.oltean at nxp.com> wrote:
> > >
> > > Hi Tim,
> > >
> > > On Fri, Mar 25, 2022 at 09:53:20AM -0700, Tim Harvey wrote:
> > > > Vladimir,
> > > >
> > > > I came across this while looking for the best place to configure cpu
> > > > port interface mode (ie rgmii id) for the mv88e61xx dsa driver I'm
> > > > working on. Note that this patch causes port_probe to be called on the
> > > > cpu port 'before' the master device has been probed. I'm not sure if
> > > > this is intended or not?
> > > >
> > > > In my case I was looking to configure the cpu port interface mode when
> > > > the port was probed but I can't do that because it happens before the
> > > > switch is probed because of some things that need to happen there.
> > > > Best Regards,
> > > >
> > > > Tim
> > >
> > > You're past the DM_MDIO probing issues now? Glad to hear that.
> > > Probing the DSA CPU port before the master wasn't necessarily the
> > > intention, but then again, I'm not sure that there's a strict ordering
> > > guarantee between the two that needs to be satisfied?
> > >
> > > What do you need exactly? We could probably always reverse the
> > > device_probe(master) call with the probing of the CPU port if the
> > > ordering turns out to be a real problem. I can regression-test the
> > > change on my boards, but I'd like to understand the need you have, maybe
> > > even document it so that future changes are aware of it.
> >
> > Yes, I've got the mdio probing working now. The mv88e61xx driver
> > supports several chips that have different register offsets that need
> > to be known before indirect read/write and port read/write can be
> > used. That detection happens early on so I have it in the dsa_probe. I
> > would prefer to configure the cpu port interface mode (mac mode, link
> > speed/duplex etc) once instead of doing it every time the cpu port
> > gets enabled so I want to put that in the dsa_probe but at that time I
> > don't have the phy_device to determine interface mode (I suppose I
> > could get it from dt?). I noticed dsa_class calls ops->port_probe for
> > the cpu port early so that seemed like a great place to do all that,
> > but then I found my switch dsa_probe hadn't been called yet so I
> > haven't identified the switch and set the register offsets yet.
> >
> > I have worked around the fact that the port_probe is called for the
> > cpu port before the switch is probed I just wasn't sure if something
> > in this patch should be changed in case others fall into this trap in
> > the future. dsa_port_probe probes the master before calling
> > ops->port_probe with the comment 'We depend on the master device for
> > proper operation' so I just figured the same should be done for the
> > pre_probe.
>
> Sorry, that was quite a blunder, we should definitely ensure that
> U_BOOT_DRIVER :: probe gets called before dsa_ops :: port_probe.
> I've made a change moving the port_probe call for the CPU port to
> dsa_post_probe(), tested it in the sandbox, and it appears to work.
>
Ok, sounds good - did you submit this someplace? I haven't seen it.
> > I hope to send a series in the next few days but I do have a few items
> > I still need to fix:
> >
> > 1. my board currently uses the mv88e61xx_hw_reset weak override to
> > configure LEDs, GPIO's using direct register writes as I need one of
> > the GPIO's to be configured as a 125MHz RGMII_RCLK and configure MAC
> > interface mode(rgmii delays). I've moved the mac interface config into
> > the driver based on the dt props (phy-mode and fixed-link subnode) but
> > am still not sure how to go about dealing with the 'very custom' LED
> > and GPIO config without the hassle of defining new dt bindings. I was
> > hoping to use board_phy_config() but when that is called for the
> > switch the phy_id is a generic PHY_FIXED_ID and when called for the
> > ports the phydev->bus uses indirect register writes which can't be
> > used to configure the gpios.
>
> How are these configs handled in Linux?
Each of the 14 GPIO's for MV88E61xx can be configured as GPIO,
PTP_TRIG (PTP output trigger), PTP_EVREQ (PTP event request input),
PTP_EXTCLK (PTP ext clock input), SE_RCLK0 (SyncE RX clock 0 output),
SE_RCLK1 (SyncE RX clock 1 output), or CLK125 (125 MHz clock output).
Currently the only config handled in Linux is PTP_EVREQ for PTP
support.
Each port has 2 LED's which can be configured for 16 different
functions. The LED config is not handled at all the the Linux driver
(PHY LED config is spotty in general).
I think it makes sense to handle this in board_config_phy() and I can
do and avoid the issue of direct register access with something like:
int board_phy_config(struct phy_device *phydev)
{
unsigned short val;
/* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */
if (phydev->phy_id == 0xa5a55a5a &&
((board_type == GW5904) || (board_type == GW5909))) {
/* get the mdio parent bus so we have direct register access */
struct mii_dev *bus = miiphy_get_dev_by_name("mdio");
puts("MV88E61XX ");
/* GPIO[0] output CLK125 for RGMII_REFCLK:
* GPIO[7:0] Direction register is 0x62 of Scratch
registers: 1=input 0=output
* GPIO[0] Pin Control register is 0x68 of Scratch
registers: 7=CLK125 (Free running 125MHz clock output)
* Scratch register access is at 0x1a of GLOBAL2 register (0x1c)
*/
bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe);
bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7);
/* Port 0-3 LED configuration: Table 80/82
* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link)
* LED Control is register 0x16 of port registers
* port registers are at 0x10 + port
*/
bus->write(bus, 0x10, 0, 0x16, 0x8088);
bus->write(bus, 0x11, 0, 0x16, 0x8088);
bus->write(bus, 0x12, 0, 0x16, 0x8088);
bus->write(bus, 0x13, 0, 0x16, 0x8088);
}
}
>
> > 2. While my switch mdio bus is probing now as well as my fec_dm_mdio
> > bus I'm not clear how to properly get the struct mii_dev * associated
> > with the fec_dm_mdio from the dsa switch. Currently I'm using
> > miiphy_get_dev_by_name("mdio") which is horrible. How do I get the
> > mii_bus or even the udevice of an dm_mdio bus associated with a dm_eth
> > device?
>
> Hmm, isn't the MDIO bus udevice the dev->parent of the switch?
My dt is:
&fec {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_enet>;
phy-mode = "rgmii-id";
status = "okay";
fixed-link {
speed = <1000>;
full-duplex;
};
mdio {
#address-cells = <1>;
#size-cells = <0>;
switch at 0 {
compatible = "marvell,mv88e6085";
reg = <0>;
mdios {
#address-cells = <1>;
#size-cells = <0>;
sw_phy0: ethernet-phy at 0 {
reg = <0x0>;
};
sw_phy1: ethernet-phy at 1 {
reg = <0x1>;
};
sw_phy2: ethernet-phy at 2 {
reg = <0x2>;
};
sw_phy3: ethernet-phy at 3 {
reg = <0x3>;
};
};
ports {
#address-cells = <1>;
#size-cells = <0>;
port at 0 {
reg = <0>;
label = "lan4";
phy-mode = "internal";
phy-handle = <&sw_phy0>;
};
port at 1 {
reg = <1>;
label = "lan3";
phy-mode = "internal";
phy-handle = <&sw_phy1>;
};
port at 2 {
reg = <2>;
label = "lan2";
phy-mode = "internal";
phy-handle = <&sw_phy2>;
};
port at 3 {
reg = <3>;
label = "lan1";
phy-mode = "internal";
phy-handle = <&sw_phy3>;
};
port at 5 {
reg = <5>;
label = "cpu";
ethernet = <&fec>;
phy-mode = "rgmii-id";
fixed-link {
speed = <1000>;
full-duplex;
};
};
};
};
};
};
Assuming this looks correct to you the dm_fec_mdio UCLASS_MDIO gets
bound to the 'mdio' node and its parent is the fec eth device. So in
my switch probe the parent of the switch is the dm_fec_mdio device but
I'm not clear how to get to that udevice's strict mii_dev*.
> Assuming that the reason you need this is for MDIO input/output towards
> the switch. Although my suggestion would be to wrap this I/O behind
> dm_mdio_read(struct udevice *dev /* MDIO child device */) and
> dm_mdio_write(struct udevice *dev), rather than poking inside struct
> udevice from the mv88e61xx driver.
I'm not clear what you mean by the above?
My current pass at adding DSA support to the mv88e61xx driver is to
change the struct phy_device *phydev argument to struct udevice *dev
throughout (which actually isn't as messy as it seems) and I store the
struct mii_dev* in the switch priv structure to be used by the low
level functions needing mii.
Best Regards,
Tim
More information about the U-Boot
mailing list