| Message ID | 1457714822-5754-2-git-send-email-ddaney.cavm@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> + phy_np = of_parse_phandle(node, "phy-handle", 0); > + /* If there is no phy or defective firmware presents > + * this cortina phy, for which there is no driver > + * support, ignore it. > + */ > + if (phy_np && > + !of_device_is_compatible(phy_np, "cortina,cs4223-slice")) { Hi David What is a cortina,cs4223-slice, and why does it need to be handled differently? Thanks Andrew
On 03/11/2016 09:31 AM, Andrew Lunn wrote: >> + phy_np = of_parse_phandle(node, "phy-handle", 0); >> + /* If there is no phy or defective firmware presents >> + * this cortina phy, for which there is no driver >> + * support, ignore it. >> + */ >> + if (phy_np && >> + !of_device_is_compatible(phy_np, "cortina,cs4223-slice")) { > > Hi David > > What is a cortina,cs4223-slice, It is 1/4 of: https://www.inphi.com/products/cs4223.php > and why does it need to be handled differently? > $ ls drivers/net/phy/*cortina* ls: cannot access drivers/net/phy/*cortina*: No such file or directory For this configuration of thunder_bgx.c, the use of a Linux PHY driver is optional. The firmware should probably not specify a PHY here, but it does so we ignore it so we don't wait around forever for the non-existent driver to bind.
On Fri, Mar 11, 2016 at 09:41:06AM -0800, David Daney wrote: > On 03/11/2016 09:31 AM, Andrew Lunn wrote: > >>+ phy_np = of_parse_phandle(node, "phy-handle", 0); > >>+ /* If there is no phy or defective firmware presents > >>+ * this cortina phy, for which there is no driver > >>+ * support, ignore it. > >>+ */ > >>+ if (phy_np && > >>+ !of_device_is_compatible(phy_np, "cortina,cs4223-slice")) { > > > >Hi David > > > >What is a cortina,cs4223-slice, > > It is 1/4 of: > > https://www.inphi.com/products/cs4223.php > > >and why does it need to be handled differently? > > > > $ ls drivers/net/phy/*cortina* > ls: cannot access drivers/net/phy/*cortina*: No such file or directory > > For this configuration of thunder_bgx.c, the use of a Linux PHY > driver is optional. > > The firmware should probably not specify a PHY here, but it does so > we ignore it so we don't wait around forever for the non-existent > driver to bind. Hi David I don't see why it should wait around forever. I have boards with Marvell PHYs, yet if i don't build the Marvell driver, the Ethernet driver still loads, because the generic PHY driver is used instead. Why does this not work here? Also, Documentation/devicetree/bindings/net/phy.txt says: compatible: Compatible list, may contain "ethernet-phy-ieee802.3-c22" or "ethernet-phy-ieee802.3-c45" for PHYs that implement IEEE802.3 clause 22 or IEEE802.3 clause 45 specifications. If neither of these are specified, the default is to assume clause 22. If the phy's identifier is known then the list may contain an entry of the form: "ethernet-phy-idAAAA.BBBB" where AAAA - The value of the 16 bit Phy Identifier 1 register as 4 hex digits. This is the chip vendor OUI bits 3:18 BBBB - The value of the 16 bit Phy Identifier 2 register as 4 hex digits. This is the chip vendor OUI bits 19:24, followed by 10 bits of a vendor specific ID. The compatible list should not contain other values than those listed here. So having "cortina,cs4223-slice" in the compatible string goes against the binding. To make this work, you probably need to extend the whitelist_phys list in of_mdio.c. Andrew
On 03/11/2016 10:00 AM, Andrew Lunn wrote: > On Fri, Mar 11, 2016 at 09:41:06AM -0800, David Daney wrote: >> On 03/11/2016 09:31 AM, Andrew Lunn wrote: >>>> + phy_np = of_parse_phandle(node, "phy-handle", 0); >>>> + /* If there is no phy or defective firmware presents >>>> + * this cortina phy, for which there is no driver >>>> + * support, ignore it. >>>> + */ >>>> + if (phy_np && >>>> + !of_device_is_compatible(phy_np, "cortina,cs4223-slice")) { >>> >>> Hi David >>> >>> What is a cortina,cs4223-slice, >> >> It is 1/4 of: >> >> https://www.inphi.com/products/cs4223.php >> >>> and why does it need to be handled differently? >>> >> >> $ ls drivers/net/phy/*cortina* >> ls: cannot access drivers/net/phy/*cortina*: No such file or directory >> >> For this configuration of thunder_bgx.c, the use of a Linux PHY >> driver is optional. >> >> The firmware should probably not specify a PHY here, but it does so >> we ignore it so we don't wait around forever for the non-existent >> driver to bind. > > Hi David > > I don't see why it should wait around forever. I have boards with > Marvell PHYs, yet if i don't build the Marvell driver, the Ethernet > driver still loads, because the generic PHY driver is used instead. > Why does this not work here? As I said before, there is no driver for the device, so of_phy_find_device() will always return NULL. It appears that the architects of the cs4223 were not familiar with the various IEEE specifications for PHYs, so unsurprisingly, the device cannot be handled by any standards compliant generic drivers. The Marvell PHY architects, on the other hand, seem to have designed their devices with a keen eye for following the standards. So, also unsurprisingly, the Marvell devices work perfectly with the generic drivers. > > Also, Documentation/devicetree/bindings/net/phy.txt says: > > compatible: Compatible list, may contain > "ethernet-phy-ieee802.3-c22" or "ethernet-phy-ieee802.3-c45" for > PHYs that implement IEEE802.3 clause 22 or IEEE802.3 clause 45 > specifications. If neither of these are specified, the default is to > assume clause 22. > > If the phy's identifier is known then the list may contain an entry > of the form: "ethernet-phy-idAAAA.BBBB" where > AAAA - The value of the 16 bit Phy Identifier 1 register as > 4 hex digits. This is the chip vendor OUI bits 3:18 > BBBB - The value of the 16 bit Phy Identifier 2 register as > 4 hex digits. This is the chip vendor OUI bits 19:24, > followed by 10 bits of a vendor specific ID. > > The compatible list should not contain other values than those > listed here. > > So having "cortina,cs4223-slice" in the compatible string goes against > the binding. Yes, you are probably correct. That is why the comment in the patch explains the strategy of ignoring this node that is not complaint with the binding document. > To make this work, you probably need to extend the > whitelist_phys list in of_mdio.c. That wouldn't work. There is no driver for the PHY, putting it on a white list doesn't solve the missing driver problem.
> >I don't see why it should wait around forever. I have boards with > >Marvell PHYs, yet if i don't build the Marvell driver, the Ethernet > >driver still loads, because the generic PHY driver is used instead. > >Why does this not work here? > > As I said before, there is no driver for the device, so > of_phy_find_device() will always return NULL. I'm not yet convinced this is true. I really do expect that the generic PHY driver will bind to it. It might then go horribly wrong, because it is not standard compliant, but that is a different issue. The generic driver should probably have a black list for such devices. This is a PHY issue, not an MDIO issue, and the problem should be solved in the PHY layer, not in one MDIO driver. We should also consider what happens when somebody actually writes a driver for this PHY. Are you not going to use it? Before this patchset, you did not special case this compatible string. So at the very least, you need to split this into a separate patch, so the maintainers can ACK/NACK it, independent of the other change it is embedded in. Andrew
On 03/11/2016 11:06 AM, Andrew Lunn wrote: >>> I don't see why it should wait around forever. I have boards with >>> Marvell PHYs, yet if i don't build the Marvell driver, the Ethernet >>> driver still loads, because the generic PHY driver is used instead. >>> Why does this not work here? >> >> As I said before, there is no driver for the device, so >> of_phy_find_device() will always return NULL. > > I'm not yet convinced this is true. Which part don't you believe? Is it: - there is no driver for the device. or - for PHYs with no driver, of_phy_find_device() will return NULL > I really do expect that the > generic PHY driver will bind to it. It might then go horribly wrong, > because it is not standard compliant, but that is a different issue. > At a higher level, the way we handle either of: - Lack of a driver. - "Horribly wrong" driver is the same, we cannot use a PHY driver. > The generic driver should probably have a black list for such devices. > This is a PHY issue, not an MDIO issue, and the problem should be > solved in the PHY layer, not in one MDIO driver. This isn't an MDIO driver patch. This is more about handling a defective device tree in the only driver (a non-MDIO driver) that will ever see such a device tree node. > > We should also consider what happens when somebody actually writes a > driver for this PHY. Are you not going to use it? Easy to answer: We remove the "&& !of_device_is_compatible(phy_np, "cortina,cs4223-slice")" clause from this driver. > > Before this patchset, you did not special case this compatible > string. So at the very least, you need to split this into a separate > patch, so the maintainers can ACK/NACK it, independent of the other > change it is embedded in. > I can, and will, do that. Thanks, David Daney
On 11/03/16 11:06, Andrew Lunn wrote: >>> I don't see why it should wait around forever. I have boards with >>> Marvell PHYs, yet if i don't build the Marvell driver, the Ethernet >>> driver still loads, because the generic PHY driver is used instead. >>> Why does this not work here? >> >> As I said before, there is no driver for the device, so >> of_phy_find_device() will always return NULL. > > I'm not yet convinced this is true. I really do expect that the > generic PHY driver will bind to it. It might then go horribly wrong, > because it is not standard compliant, but that is a different issue. I concur with Andrew here, unless the PHY is guaranteed to return garbage when get_phy_id() is called, there is a good chance that the Generic PHY driver will be bound to this PHY device, or this is not happening for you for some reason? > > The generic driver should probably have a black list for such devices. > This is a PHY issue, not an MDIO issue, and the problem should be > solved in the PHY layer, not in one MDIO driver. I considered the possibility once of disabling the generic PHY driver, such that systems where the vendor-specific PHY driver is expected to be used could utilize that. That does not play well with the fixed PHYs using the generic PHY driver though, anyway, I am digressing. > > We should also consider what happens when somebody actually writes a > driver for this PHY. Are you not going to use it? > > Before this patchset, you did not special case this compatible > string. So at the very least, you need to split this into a separate > patch, so the maintainers can ACK/NACK it, independent of the other > change it is embedded in. > > Andrew >
On 03/11/2016 11:37 AM, Florian Fainelli wrote: > On 11/03/16 11:06, Andrew Lunn wrote: >>>> I don't see why it should wait around forever. I have boards with >>>> Marvell PHYs, yet if i don't build the Marvell driver, the Ethernet >>>> driver still loads, because the generic PHY driver is used instead. >>>> Why does this not work here? >>> >>> As I said before, there is no driver for the device, so >>> of_phy_find_device() will always return NULL. >> >> I'm not yet convinced this is true. I really do expect that the >> generic PHY driver will bind to it. It might then go horribly wrong, >> because it is not standard compliant, but that is a different issue. > > I concur with Andrew here, unless the PHY is guaranteed to return > garbage when get_phy_id() is called, there is a good chance that the > Generic PHY driver will be bound to this PHY device, or this is not > happening for you for some reason? > get_phy_id() is working a designed. For this phy, we have: compatible = "cortina,cs4223-slice"; Therefore get_phy_id() is being called with a is_c45 value of false. get_phy_id() is returning a value of 0, which means that it succeeds, but the returned phy_id is 0xffffffff, which causes get_phy_device() to not create a phy_device, and no driver can be bound. I know you are all skeptical, but I really think the best thing to do is not try to attach a phy driver when this compatible value is encountered. It is a defective device tree, and I am attempting to handle it in the specific site where it can cause problems. We are trying to distinguish between these two cases: - of_phy_find_device() returns NULL because driver is not yet bound - of_phy_find_device() returns NULL because "cortina,cs4223-slice" I don't think we need to build some sort of frame work to handle things like this in a general way. David Daney
> For this phy, we have: > > compatible = "cortina,cs4223-slice"; That actually means something else is happening, i think. of_mdiobus_register() looks at the children, and decides if each child is a phy or an mdio device, by calling of_mdiobus_child_is_phy(). Since this compatible string is not in whitelist_phys[], it will return false. of_mdiobus_register() will then do a of_mdiobus_register_device(). This compatible means it is an MDIO device, not a PHY. So when you later call of_phy_find_device() it is always going to return NULL, because there is no PHY there, only an MDIO device. How usable is the hardware without a PHY driver? Is a better solution that your write a very minimal PHY driver? Andrew
On 03/11/2016 01:35 PM, Andrew Lunn wrote: [...] > How usable is the hardware without a PHY driver? The hardware has always in the past, still does, and probably always will work fine without a PHY driver. Link up/down are correctly handled. > Is a better solution > that your write a very minimal PHY driver? No. Nothing would be gained. All we are trying to do, is allow for loading of 1G PHY drivers via the -EPROBE_DEFER mechanism while continuing to allow the 10G and 40G ports to function without a PHY driver. Specifically, we are *not* attempting to solve the problem of re-architecting the kernel phy_device infrastructure so that it would be possible to write a Cortina PHYs driver. Nor are we proposing that a Cortina PHY driver be written that would fit into the current infrastructure. To this end, I still think the current patch takes the best approach. Thanks, David Daney
diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c index f8abdff..928c02b 100644 --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c @@ -978,27 +978,31 @@ static int bgx_init_of_phy(struct bgx *bgx) const char *mac; device_for_each_child_node(&bgx->pdev->dev, fwn) { + struct phy_device *pd; struct device_node *phy_np; struct device_node *node = to_of_node(fwn); - /* If it is not an OF node we cannot handle it yet, so - * exit the loop. - */ - if (!node) - break; - - phy_np = of_parse_phandle(node, "phy-handle", 0); - if (!phy_np) - continue; - - bgx->lmac[lmac].phydev = of_phy_find_device(phy_np); - mac = of_get_mac_address(node); if (mac) ether_addr_copy(bgx->lmac[lmac].mac, mac); SET_NETDEV_DEV(&bgx->lmac[lmac].netdev, &bgx->pdev->dev); bgx->lmac[lmac].lmacid = lmac; + + phy_np = of_parse_phandle(node, "phy-handle", 0); + /* If there is no phy or defective firmware presents + * this cortina phy, for which there is no driver + * support, ignore it. + */ + if (phy_np && + !of_device_is_compatible(phy_np, "cortina,cs4223-slice")) { + /* Wait until the phy drivers are available */ + pd = of_phy_find_device(phy_np); + if (!pd) + return -EPROBE_DEFER; + bgx->lmac[lmac].phydev = pd; + } + lmac++; if (lmac == MAX_LMAC_PER_BGX) { of_node_put(node); @@ -1032,9 +1036,6 @@ static int bgx_probe(struct pci_dev *pdev, const struct pci_device_id *ent) struct bgx *bgx = NULL; u8 lmac; - /* Load octeon mdio driver */ - octeon_mdiobus_force_mod_depencency(); - bgx = devm_kzalloc(dev, sizeof(*bgx), GFP_KERNEL); if (!bgx) return -ENOMEM;