linux-kernel - Re: [PATCH v2 2/2] net: phy: tja11xx: add delayed registration of TJA1102 PHY1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Mar 2020 18:28:00 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        linux-kernel@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Marek Vasut <marex@...x.de>, David Jander <david@...tonic.nl>
Subject: Re: [PATCH v2 2/2] net: phy: tja11xx: add delayed registration of
 TJA1102 PHY1

On Mon, Mar 09, 2020 at 08:40:44AM +0100, Oleksij Rempel wrote:
> TJA1102 is a dual PHY package with PHY0 having proper PHYID and PHY1
> having no ID. On one hand it is possible to for PHY detection by
> compatible, on other hand we should be able to reset complete chip
> before PHY1 configured it, and we need to define dependencies for proper
> power management.
> 
> We can solve it by defining PHY1 as child of PHY0:
> 	tja1102_phy0: ethernet-phy@4 {
> 		reg = <0x4>;
> 
> 		interrupts-extended = <&gpio5 8 IRQ_TYPE_LEVEL_LOW>;
> 
> 		reset-gpios = <&gpio5 9 GPIO_ACTIVE_LOW>;
> 		reset-assert-us = <20>;
> 		reset-deassert-us = <2000>;
> 
> 		tja1102_phy1: ethernet-phy@5 {
> 			reg = <0x5>;
> 
> 			interrupts-extended = <&gpio5 8 IRQ_TYPE_LEVEL_LOW>;
> 		};
> 	};
> 
> The PHY1 should be a subnode of PHY0 and registered only after PHY0 was
> completely reset and initialized.

Hi Oleksij

Please add a binding document for this.

> +static void tja1102_p1_register(struct work_struct *work)
> +{
> +	struct tja11xx_priv *priv = container_of(work, struct tja11xx_priv,
> +						 phy_register_work);
> +
> +	struct phy_device *phydev_phy0 = priv->phydev;
> +        struct mii_bus *bus = phydev_phy0->mdio.bus;
> +	struct device *dev = &phydev_phy0->mdio.dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *child;
> +	int ret;
> +
> +	for_each_available_child_of_node(np, child) {
> +		struct phy_device *phy;
> +		int addr;
> +
> +		addr = of_mdio_parse_addr(dev, child);
> +		if (addr < 0) {
> +			dev_err(dev, "Can't parse addr\n");
> +			continue;
> +		}

It would also be good to check that addr is one more than the parent
device. That seems to be a silicon constraint.

> +
> +		/* skip already registered PHYs */
> +		if (mdiobus_is_registered_device(bus, addr)) {
> +			dev_err(dev, "device is already registred \n");
> +			continue;
> +		}
> +
> +		phy = phy_device_create(bus, addr, PHY_ID_TJA1102,
> +						false, NULL);
> +		if (IS_ERR(phy)) {
> +			dev_err(dev, "Can't register Port : %i\n", addr);

You are not registering at this step, just allocating.

> +			continue;
> +		}
> +
> +		ret = of_irq_get(child, 0);
> +		/* can we be deferred here? */

Yes.

commit 66bdede495c71da9c5ce18542976fae53642880b
Author: Geert Uytterhoeven <geert+renesas@...der.be>
Date:   Wed Oct 18 13:54:03 2017 +0200

    of_mdio: Fix broken PHY IRQ in case of probe deferral
    
    If an Ethernet PHY is initialized before the interrupt controller it is
    connected to, a message like the following is printed:
    
        irq: no irq domain found for /interrupt-controller@...c0000 !


> +		if (ret > 0) {
> +			phy->irq = ret;
> +			bus->irq[addr] = ret;
> +		} else {
> +			phy->irq = bus->irq[addr];
> +		}
> +
> +		/* overwrite parent phy_device_create() set parent to the
> +		 * mii_bus->dev
> +		 */
> +		phy->mdio.dev.parent = dev;
> +
> +		/* Associate the OF node with the device structure so it
> +		 * can be looked up later */
> +		of_node_get(child);
> +		phy->mdio.dev.of_node = child;
> +		phy->mdio.dev.fwnode = of_fwnode_handle(child);
> +
> +		/* All data is now stored in the phy struct;
> +		 * register it */
> +		ret = phy_device_register(phy);
> +		if (ret) {
> +			phy_device_free(phy);
> +			of_node_put(child);
> +		}
> +	}
> +}

This is a lot of of_mdiobus_register_phy(). I think it would be better
to refactor this code a bit, maybe make a helper out of
of_mdiobus_register_phy() for all the shared code.

	  Andrew