Received-SPF: pass (google.com: domain of linux-kernel+bounces-14654-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 2 Jan 2024 17:24:45 +0000
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Luo Jie <quic_luoj@quicinc.com>
Cc: agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
	davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	andrew@lunn.ch, hkallweit1@gmail.com, robert.marko@sartura.hr,
	linux-arm-msm@vger.kernel.org, netdev@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	quic_srichara@quicinc.com
Subject: Re: [PATCH v4 1/5] net: mdio: ipq4019: move eth_ldo_rdy before MDIO
 bus register
Message-ID: <ZZRG3eZJM5QouR9+@shell.armlinux.org.uk>
References: <20231225084424.30986-1-quic_luoj@quicinc.com>
 <20231225084424.30986-2-quic_luoj@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231225084424.30986-2-quic_luoj@quicinc.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Mon, Dec 25, 2023 at 04:44:20PM +0800, Luo Jie wrote:
> +/* Maximum SOC PCS(uniphy) number on IPQ platform */
> +#define ETH_LDO_RDY_CNT				3
> +
>  struct ipq4019_mdio_data {
> -	void __iomem	*membase;
> -	void __iomem *eth_ldo_rdy;
> +	void __iomem *membase;
> +	void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];

Do you have any plans to make use of eth_ldo_rdy other than by code that
you touch in this patch? If you don't, what is the point of storing
these points in struct ipq4019_mdio_data ? You're using devm_ioremap()
which will already store the pointer internally to be able to free the
resource at the appropriate moment, so if your only use is shortly after
devm_ioremap(), you can use a local variable for this... meaning this
becomes (in addition to the other suggestion):

> +	/* This resource are optional */
> +	for (index = 0; index < ETH_LDO_RDY_CNT; index++) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1);
> +		if (res) {
> +			priv->eth_ldo_rdy[index] = devm_ioremap(&pdev->dev,
> +								res->start,
> +								resource_size(res));
> +
> +			/* The ethernet LDO enable is necessary to reset PHY
> +			 * by GPIO, some PHY(such as qca8084) GPIO reset uses
> +			 * the MDIO level reset, so this function should be
> +			 * called before the MDIO bus register.
> +			 */
> +			if (priv->eth_ldo_rdy[index]) {
> +				u32 val;
> +
> +				val = readl(priv->eth_ldo_rdy[index]);
> +				val |= BIT(0);
> +				writel(val, priv->eth_ldo_rdy[index]);
> +				fsleep(IPQ_PHY_SET_DELAY_US);
> +			}
> +		}

		void __iomem *eth_ldo_rdy;
		u32 val;

		res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1);
		if (!res)
			break;

		eth_ldo_rdy = devm_ioremap(&pdev->dev, res->start,
					   resource_size(res));
		if (!eth_ldo_rdy)
			continue;

		val = readl(eth_ldo_rdy) | BIT(0);
		writel(val, eth_ldo_rdy);
		... whatever sleep is deemed required ...

Other issues:

1. if devm_ioremap() fails (returns NULL) is it right that we should
   just ignore that resource?

2. Do you need to sleep after each write, or will sleeping after
   writing all of these do? It may be more efficient to detect when
   we have at least one eth_ldo_rdy and then sleep once.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!