Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
> You can actually strap the 6390 and friends for a multi-chip mode where > they claim only a single address, instead of one per port, plus a couple > more for global registers. It vastly slows things down because of the > extra indirection, but it allows the switch to play nicely with other > MDIO devs. As you say, it slows things down a lot, so it is not used very often. In fact, i don't know of any recent board which actually uses a single address, for any DSA supported switch. If you need multiple devices, e.g. an odd PHY as well as a switch, i would use a couple of GPIO lines and do a bit-banging MDIO bus for the PHY, and let the switch have all the address of the hardware MDIO bus. This assumes you are using the kernel infrastructure, so you can connect the MAC to any arbitrary PHY. Andrew
Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
On 12/3/18 1:18 PM, Andrew Lunn wrote: >> Agreed, but I'd argue it's the same behavior we have today with the >> existing MII ioctls in this driver. That's not to say this is good, >> it's just not any less broken than the current state of things. > > Agreed. > > I actually would be happy with a warning in the commit message that > this code is not sufficient to make use of Linux PHY drivers, because > of the hardware polling. You can then leave fixing that to whoever > needs Linux PHY drivers. OK, will update for v3. >> AFAICT the polling hardware only pokes the device address that the >> driver stores in 'hw->phy.mdio.prtad', so the PHY polling unit would >> never see the switch, if it's even polling at all. Some of the MAC >> configurations will store MDIO_PRTAD_NONE, in which case I wouldn't >> expect the polling unit to be active. It's up to the board designer to >> ensure there's no address conflicts on the bus. > > Well, the 6390 does use address 0-10 for its port registers, and there > is something which looks like a PHYID in register 3. So for your use > case of DSA, it would be good to ensure it really is disabled. You can actually strap the 6390 and friends for a multi-chip mode where they claim only a single address, instead of one per port, plus a couple more for global registers. It vastly slows things down because of the extra indirection, but it allows the switch to play nicely with other MDIO devs. >> Then in the ioctl code, in addition to checking the mii_bus is >> available, also check that the requested address is one that phy_mask >> says mii_bus owns, otherwise we fall through to the old code. > > I'm not too bothered with the ioctl. It is there so you can shoot > yourself in the foot. The hardware polling unit just adds more > interesting weapons you can use to shoot yourself in the foot. Hooray for enhanced anti-foot artillery :)
Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
> Agreed, but I'd argue it's the same behavior we have today with the > existing MII ioctls in this driver. That's not to say this is good, > it's just not any less broken than the current state of things. Agreed. I actually would be happy with a warning in the commit message that this code is not sufficient to make use of Linux PHY drivers, because of the hardware polling. You can then leave fixing that to whoever needs Linux PHY drivers. > AFAICT the polling hardware only pokes the device address that the > driver stores in 'hw->phy.mdio.prtad', so the PHY polling unit would > never see the switch, if it's even polling at all. Some of the MAC > configurations will store MDIO_PRTAD_NONE, in which case I wouldn't > expect the polling unit to be active. It's up to the board designer to > ensure there's no address conflicts on the bus. Well, the 6390 does use address 0-10 for its port registers, and there is something which looks like a PHYID in register 3. So for your use case of DSA, it would be good to ensure it really is disabled. > Then in the ioctl code, in addition to checking the mii_bus is > available, also check that the requested address is one that phy_mask > says mii_bus owns, otherwise we fall through to the old code. I'm not too bothered with the ioctl. It is there so you can shoot yourself in the foot. The hardware polling unit just adds more interesting weapons you can use to shoot yourself in the foot. Andrew
Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
On 12/3/18 12:21 PM, Andrew Lunn wrote:
> On Mon, Dec 03, 2018 at 05:02:40PM +, Steve Douthit wrote:
>> On 12/3/18 11:54 AM, Andrew Lunn wrote:
+static s32 ixgbe_x550em_a_mii_bus_read(struct mii_bus *bus, int addr,
+ int regnum)
+{
+ struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
+ struct ixgbe_hw *hw = >hw;
+ u32 gssr = hw->phy.phy_semaphore_mask | IXGBE_GSSR_TOKEN_SM;
+
+ if (hw->bus.lan_id)
+ gssr |= IXGBE_GSSR_PHY1_SM;
+ else
+ gssr |= IXGBE_GSSR_PHY0_SM;
>>>
>>> Hi Steve
>>>
>>> If you only have one bus, do you still need this? One semaphore is all
>>> you need. And i'm not even sure you need that. The MDIO layer will
>>> perform locking, assuming everything goes through the MDIO layer.
>>
>> AFAIK I still need part of it. There's a PHY polling unit in the card
>> that we need to sync with independent of the locking in the MDIO layer.
>> I can drop the hw->bus.lan_id check though and unconditionally OR in the
>> IXGBE_GSSR_PHY0_SM since we always register on function 0 now.
>
> Hi Steve
>
> In general, this is not enough to make a PHY polling unit safe. At
> least not with C22 devices. They often have a register which can be
> used to select a different page. The software driver can do a write to
> swap the page at any time, e.g. to select the page with the
> temperature sensor. After it releases the semaphore for that write
> changing the page, the hardware could then poll the PHY, and read a
> temperature sensor register instead of the link status, and then bad
> things happen.
>
> When using Intel's PHY drivers embedded within the Intel MAC driver,
> this is not a problem. They can avoid swapping to different
> pages. However, you are on the path to allow the Linux PHY drivers to
> be used, and some of them will change the page. And with the embedded
> SOC you are working on, i would not be too surprised to see somebody
> build a system using a different PHY which the Intel PHY drivers don't
> support, but does have a Linux PHY driver.
Agreed, but I'd argue it's the same behavior we have today with the
existing MII ioctls in this driver. That's not to say this is good,
it's just not any less broken than the current state of things. I don't
know what the scope of the locking is for the software/firmware
semaphore on the firmware side. Maybe someone from Intel has more
details on the locking that would help find a clever locking solution?
> You also need to watch out for your use case of Marvell
> mv88e6390. Polling that makes no sense. Does the hardware actually
> recognise it is not a PHY?
AFAICT the polling hardware only pokes the device address that the
driver stores in 'hw->phy.mdio.prtad', so the PHY polling unit would
never see the switch, if it's even polling at all. Some of the MAC
configurations will store MDIO_PRTAD_NONE, in which case I wouldn't
expect the polling unit to be active. It's up to the board designer to
ensure there's no address conflicts on the bus.
I'm making some assumptions about the inner workings of the hardware
here, so someone from Intel would need to confirm those or set me
straight.
How about if I keep track of which PHY addresses the driver wants for
the x550em_a devices, then clear those bits from the phy_mask instead of
+ bus->phy_mask = GENMASK(31, 0);
Then in the ioctl code, in addition to checking the mii_bus is
available, also check that the requested address is one that phy_mask
says mii_bus owns, otherwise we fall through to the old code.
I think that keeps the dsa and PHY polling as separate as it is today
on the ioctl side, and hides the PHY(s) that the ixgbe driver wants to
manage exclusively from the mii_bus altogether.
Let me know what you think. It'll make the probing code a bit more
complicated, but I think it addresses your concerns.
Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
On Mon, Dec 03, 2018 at 05:02:40PM +, Steve Douthit wrote:
> On 12/3/18 11:54 AM, Andrew Lunn wrote:
> >> +static s32 ixgbe_x550em_a_mii_bus_read(struct mii_bus *bus, int addr,
> >> + int regnum)
> >> +{
> >> + struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
> >> + struct ixgbe_hw *hw = >hw;
> >> + u32 gssr = hw->phy.phy_semaphore_mask | IXGBE_GSSR_TOKEN_SM;
> >> +
> >> + if (hw->bus.lan_id)
> >> + gssr |= IXGBE_GSSR_PHY1_SM;
> >> + else
> >> + gssr |= IXGBE_GSSR_PHY0_SM;
> >
> > Hi Steve
> >
> > If you only have one bus, do you still need this? One semaphore is all
> > you need. And i'm not even sure you need that. The MDIO layer will
> > perform locking, assuming everything goes through the MDIO layer.
>
> AFAIK I still need part of it. There's a PHY polling unit in the card
> that we need to sync with independent of the locking in the MDIO layer.
> I can drop the hw->bus.lan_id check though and unconditionally OR in the
> IXGBE_GSSR_PHY0_SM since we always register on function 0 now.
Hi Steve
In general, this is not enough to make a PHY polling unit safe. At
least not with C22 devices. They often have a register which can be
used to select a different page. The software driver can do a write to
swap the page at any time, e.g. to select the page with the
temperature sensor. After it releases the semaphore for that write
changing the page, the hardware could then poll the PHY, and read a
temperature sensor register instead of the link status, and then bad
things happen.
When using Intel's PHY drivers embedded within the Intel MAC driver,
this is not a problem. They can avoid swapping to different
pages. However, you are on the path to allow the Linux PHY drivers to
be used, and some of them will change the page. And with the embedded
SOC you are working on, i would not be too surprised to see somebody
build a system using a different PHY which the Intel PHY drivers don't
support, but does have a Linux PHY driver.
You also need to watch out for your use case of Marvell
mv88e6390. Polling that makes no sense. Does the hardware actually
recognise it is not a PHY?
Andrew
Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
On 12/3/18 11:54 AM, Andrew Lunn wrote:
>> +static s32 ixgbe_x550em_a_mii_bus_read(struct mii_bus *bus, int addr,
>> + int regnum)
>> +{
>> +struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
>> +struct ixgbe_hw *hw = >hw;
>> +u32 gssr = hw->phy.phy_semaphore_mask | IXGBE_GSSR_TOKEN_SM;
>> +
>> +if (hw->bus.lan_id)
>> +gssr |= IXGBE_GSSR_PHY1_SM;
>> +else
>> +gssr |= IXGBE_GSSR_PHY0_SM;
>
> Hi Steve
>
> If you only have one bus, do you still need this? One semaphore is all
> you need. And i'm not even sure you need that. The MDIO layer will
> perform locking, assuming everything goes through the MDIO layer.
AFAIK I still need part of it. There's a PHY polling unit in the card
that we need to sync with independent of the locking in the MDIO layer.
I can drop the hw->bus.lan_id check though and unconditionally OR in the
IXGBE_GSSR_PHY0_SM since we always register on function 0 now.
Re: [PATCH net-next v2 1/2] ixgbe: register a mdiobus
> +static s32 ixgbe_x550em_a_mii_bus_read(struct mii_bus *bus, int addr,
> +int regnum)
> +{
> + struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
> + struct ixgbe_hw *hw = >hw;
> + u32 gssr = hw->phy.phy_semaphore_mask | IXGBE_GSSR_TOKEN_SM;
> +
> + if (hw->bus.lan_id)
> + gssr |= IXGBE_GSSR_PHY1_SM;
> + else
> + gssr |= IXGBE_GSSR_PHY0_SM;
Hi Steve
If you only have one bus, do you still need this? One semaphore is all
you need. And i'm not even sure you need that. The MDIO layer will
perform locking, assuming everything goes through the MDIO layer.
Andrew
