Messages in this thread | | | Subject | Re: [PATCH v2] ARM: imx: allow to disable board specific PHY fixups | From | Florian Fainelli <> | Date | Wed, 1 Apr 2020 10:10:57 -0700 |
| |
On 3/31/2020 11:33 PM, Oleksij Rempel wrote: > On Tue, Mar 31, 2020 at 09:19:18AM +0100, Russell King - ARM Linux admin wrote: >> On Tue, Mar 31, 2020 at 10:00:12AM +0200, Marc Kleine-Budde wrote: >>> On 3/31/20 9:54 AM, Russell King - ARM Linux admin wrote: >>>> On Tue, Mar 31, 2020 at 09:47:19AM +0200, Marc Kleine-Budde wrote: >>>>> On 3/30/20 7:41 PM, Russell King - ARM Linux admin wrote: >>>>>>>> arch/arm/mach-imx/mach-imx6q.c:167: phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK, >>>>>>>> arch/arm/mach-imx/mach-imx6q.c:169: phy_register_fixup_for_uid(PHY_ID_KSZ9031, MICREL_PHY_ID_MASK, >>>>>>>> arch/arm/mach-imx/mach-imx6q.c:171: phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef, >>>>>>>> arch/arm/mach-imx/mach-imx6q.c:173: phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef, >>>>>> >>>>>> As far as I'm concerned, the AR8035 fixup is there with good reason. >>>>>> It's not just "random" but is required to make the AR8035 usable with >>>>>> the iMX6 SoCs. Not because of a board level thing, but because it's >>>>>> required for the AR8035 to be usable with an iMX6 SoC. >>>>> >>>>> Is this still ture, if the AR8035 is attached to a switch behind an iMX6? >>>> >>>> Do you know of such a setup, or are you talking about theoretical >>>> situations? >>> >>> Granted, not for the AR8035, but for one of the KSZ Phys. This is why >>> Oleksij started looking into this issue in the first place. >> >> Maybe there's an easy solution to this - check whether the PHY being >> fixed up is connected to the iMX6 SoC: >> >> static bool phy_connected_to(struct phy_device *phydev, >> const struct of_device_id *matches) >> { >> struct device_node *np, *phy_np; >> >> for_each_matching_node(np, matches) { >> phy_np = of_parse_phandle(np, "phy-handle", 0); >> if (!phy_np) >> phy_np = of_parse_phandle(np, "phy", 0); >> if (!phy_np) >> phy_np = of_parse_phandle(np, "phy-device", 0); >> if (phy_np && phydev->mdio.dev.of_node == phy_np) { >> of_node_put(phy_np); >> of_node_put(np); >> return true; >> } >> of_node_put(phy_np); >> } >> return false; >> } >> >> static struct of_device_id imx_fec_ids[] = { >> { .compatible = "fsl,imx6q-fec", }, >> ... >> { }, >> }; >> >> static bool phy_connected_to_fec(struct phy_device *phydev) >> { >> return phy_connected_to(phydev, imx_fec_ids); >> } >> >> and then in the fixups: >> >> if (!phy_connected_to_fec(phydev)) >> return 0; >> > > Ok, i see. We will limit fixup impact to some specific devicetree nodes. > And if we wont to disable fixup completely, some special devicetree binding will > be needed. Correct? Is this acceptable mainline way? > For the usb ethernet fixups we will need some thing similar.
It looks like IMX is using phy_register_fixup_for_uid() which is not able to scope the fixup against a specific MDIO bus controller name, I would suggest we introduce one or two variants of that function in order to allow specifying the scope against a MDIO bus controller name, and another variant which can take a comparison function, such that the logic that Russell has suggested above could be passed a as callback to a new function: phy_register_fixup_cmp() or whatever is an appropriate name. Internally, those functions would ideal all be implemented by the same core function which is able to use any key/value as a match. -- Florian
| |