On 1/3/2024 1:24 AM, Russell King (Oracle) wrote:
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):
I will remove the eth_lod_rdy from the structure, which is only for
enabling LDO shortly, will update to use the local variable for this.
Thanks for review comments.
+ /* 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?
Since each LDO is independent, each LDO is for controlling the
independent Ethernet device, that should be fine to ignore the
failed 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.
Yes, sleeping can be used after writing all of these, will update the
code as you suggest, thanks Russell.
Thanks.
