diff mbox series

[v4,1/5] net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register

Message ID 20231225084424.30986-2-quic_luoj@quicinc.com
State New
Headers
Series support ipq5332 platform |

Commit Message

Jie Luo Dec. 25, 2023, 8:44 a.m. UTC 
  The ethernet LDO provides the clock for the ethernet PHY that
is connected with PCS, each LDO enables the clock output to
each PCS, after the clock output enablement, the PHY GPIO reset
can take effect.

For the PHY taking the MDIO bus level GPIO reset, the ethernet
LDO should be enabled before the MDIO bus register.

For example, the qca8084 PHY takes the MDIO bus level GPIO
reset for quad PHYs, there is another reason for qca8084 PHY
using MDIO bus level GPIO reset instead of PHY level GPIO
reset as below.

The work sequence of qca8084:
1. enable ethernet LDO.
2. GPIO reset on quad PHYs.
3. register clock provider based on MDIO device of qca8084.
4. PHY probe function called for initializing common clocks.
5. PHY capabilities acquirement.

If qca8084 takes PHY level GPIO reset in the step 4, the clock
provider of qca8084 can't be registered correctly, since the
clock parent(reading the current qca8084 hardware registers in
step 3) of the registered clocks is deserted after GPIO reset.

There are two PCS(UNIPHY) supported in SOC side on ipq5332,
and three PCS(UNIPHY) supported on ipq9574.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 drivers/net/mdio/mdio-ipq4019.c | 51 +++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 19 deletions(-)

Comments

Konrad Dybcio Dec. 28, 2023, 9:49 a.m. UTC | #1 
On 25.12.2023 09:44, Luo Jie wrote:
> The ethernet LDO provides the clock for the ethernet PHY that
> is connected with PCS, each LDO enables the clock output to
> each PCS, after the clock output enablement, the PHY GPIO reset
> can take effect.
> 
> For the PHY taking the MDIO bus level GPIO reset, the ethernet
> LDO should be enabled before the MDIO bus register.
> 
> For example, the qca8084 PHY takes the MDIO bus level GPIO
> reset for quad PHYs, there is another reason for qca8084 PHY
> using MDIO bus level GPIO reset instead of PHY level GPIO
> reset as below.
> 
> The work sequence of qca8084:
> 1. enable ethernet LDO.
> 2. GPIO reset on quad PHYs.
> 3. register clock provider based on MDIO device of qca8084.
> 4. PHY probe function called for initializing common clocks.
> 5. PHY capabilities acquirement.
> 
> If qca8084 takes PHY level GPIO reset in the step 4, the clock
> provider of qca8084 can't be registered correctly, since the
> clock parent(reading the current qca8084 hardware registers in
> step 3) of the registered clocks is deserted after GPIO reset.
> 
> There are two PCS(UNIPHY) supported in SOC side on ipq5332,
> and three PCS(UNIPHY) supported on ipq9574.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  drivers/net/mdio/mdio-ipq4019.c | 51 +++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
> index abd8b508ec16..5273864fabb3 100644
> --- a/drivers/net/mdio/mdio-ipq4019.c
> +++ b/drivers/net/mdio/mdio-ipq4019.c
> @@ -37,9 +37,12 @@
>  
>  #define IPQ_PHY_SET_DELAY_US	100000
>  
> +/* 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];
>  	struct clk *mdio_clk;
>  };
>  
> @@ -206,19 +209,8 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
>  static int ipq_mdio_reset(struct mii_bus *bus)
>  {
>  	struct ipq4019_mdio_data *priv = bus->priv;
> -	u32 val;
>  	int ret;
>  
> -	/* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
> -	 * is specified in the device tree.
> -	 */
> -	if (priv->eth_ldo_rdy) {
> -		val = readl(priv->eth_ldo_rdy);
> -		val |= BIT(0);
> -		writel(val, priv->eth_ldo_rdy);
> -		fsleep(IPQ_PHY_SET_DELAY_US);
> -	}
> -
>  	/* Configure MDIO clock source frequency if clock is specified in the device tree */
>  	ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
>  	if (ret)
> @@ -236,7 +228,7 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>  	struct ipq4019_mdio_data *priv;
>  	struct mii_bus *bus;
>  	struct resource *res;
> -	int ret;
> +	int ret, index;
>  
>  	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
>  	if (!bus)
> @@ -252,11 +244,32 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->mdio_clk))
>  		return PTR_ERR(priv->mdio_clk);
>  
> -	/* The platform resource is provided on the chipset IPQ5018 */
> -	/* This resource is optional */
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -	if (res)
> -		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
> +	/* These platform resources are provided on the chipset IPQ5018 or
> +	 * IPQ5332.
> +	 */
> +	/* This resource are optional */
> +	for (index = 0; index < ETH_LDO_RDY_CNT; index++) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1);
> +		if (res) {
if (!res)
	break

> +			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);
fsleep should only be used when the argument is variable

Konrad
Jie Luo Dec. 30, 2023, 3:15 a.m. UTC | #2 
On 12/28/2023 5:49 PM, Konrad Dybcio wrote:
> On 25.12.2023 09:44, Luo Jie wrote:
>> The ethernet LDO provides the clock for the ethernet PHY that
>> is connected with PCS, each LDO enables the clock output to
>> each PCS, after the clock output enablement, the PHY GPIO reset
>> can take effect.
>>
>> For the PHY taking the MDIO bus level GPIO reset, the ethernet
>> LDO should be enabled before the MDIO bus register.
>>
>> For example, the qca8084 PHY takes the MDIO bus level GPIO
>> reset for quad PHYs, there is another reason for qca8084 PHY
>> using MDIO bus level GPIO reset instead of PHY level GPIO
>> reset as below.
>>
>> The work sequence of qca8084:
>> 1. enable ethernet LDO.
>> 2. GPIO reset on quad PHYs.
>> 3. register clock provider based on MDIO device of qca8084.
>> 4. PHY probe function called for initializing common clocks.
>> 5. PHY capabilities acquirement.
>>
>> If qca8084 takes PHY level GPIO reset in the step 4, the clock
>> provider of qca8084 can't be registered correctly, since the
>> clock parent(reading the current qca8084 hardware registers in
>> step 3) of the registered clocks is deserted after GPIO reset.
>>
>> There are two PCS(UNIPHY) supported in SOC side on ipq5332,
>> and three PCS(UNIPHY) supported on ipq9574.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   drivers/net/mdio/mdio-ipq4019.c | 51 +++++++++++++++++++++------------
>>   1 file changed, 32 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
>> index abd8b508ec16..5273864fabb3 100644
>> --- a/drivers/net/mdio/mdio-ipq4019.c
>> +++ b/drivers/net/mdio/mdio-ipq4019.c
>> @@ -37,9 +37,12 @@
>>   
>>   #define IPQ_PHY_SET_DELAY_US	100000
>>   
>> +/* 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];
>>   	struct clk *mdio_clk;
>>   };
>>   
>> @@ -206,19 +209,8 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
>>   static int ipq_mdio_reset(struct mii_bus *bus)
>>   {
>>   	struct ipq4019_mdio_data *priv = bus->priv;
>> -	u32 val;
>>   	int ret;
>>   
>> -	/* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
>> -	 * is specified in the device tree.
>> -	 */
>> -	if (priv->eth_ldo_rdy) {
>> -		val = readl(priv->eth_ldo_rdy);
>> -		val |= BIT(0);
>> -		writel(val, priv->eth_ldo_rdy);
>> -		fsleep(IPQ_PHY_SET_DELAY_US);
>> -	}
>> -
>>   	/* Configure MDIO clock source frequency if clock is specified in the device tree */
>>   	ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
>>   	if (ret)
>> @@ -236,7 +228,7 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>>   	struct ipq4019_mdio_data *priv;
>>   	struct mii_bus *bus;
>>   	struct resource *res;
>> -	int ret;
>> +	int ret, index;
>>   
>>   	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
>>   	if (!bus)
>> @@ -252,11 +244,32 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>>   	if (IS_ERR(priv->mdio_clk))
>>   		return PTR_ERR(priv->mdio_clk);
>>   
>> -	/* The platform resource is provided on the chipset IPQ5018 */
>> -	/* This resource is optional */
>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> -	if (res)
>> -		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
>> +	/* These platform resources are provided on the chipset IPQ5018 or
>> +	 * IPQ5332.
>> +	 */
>> +	/* This resource are optional */
>> +	for (index = 0; index < ETH_LDO_RDY_CNT; index++) {
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1);
>> +		if (res) {
> if (!res)
> 	break

will update this.

> 
>> +			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);
> fsleep should only be used when the argument is variable
> 
> Konrad

Ok, will update to use usleep_range, Thanks.
Russell King (Oracle) Jan. 2, 2024, 5:24 p.m. UTC | #3 
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.
Jie Luo Jan. 3, 2024, 1:27 p.m. UTC | #4 
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.
>
diff mbox series

Patch

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index abd8b508ec16..5273864fabb3 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -37,9 +37,12 @@ 
 
 #define IPQ_PHY_SET_DELAY_US	100000
 
+/* 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];
 	struct clk *mdio_clk;
 };
 
@@ -206,19 +209,8 @@  static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
 static int ipq_mdio_reset(struct mii_bus *bus)
 {
 	struct ipq4019_mdio_data *priv = bus->priv;
-	u32 val;
 	int ret;
 
-	/* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
-	 * is specified in the device tree.
-	 */
-	if (priv->eth_ldo_rdy) {
-		val = readl(priv->eth_ldo_rdy);
-		val |= BIT(0);
-		writel(val, priv->eth_ldo_rdy);
-		fsleep(IPQ_PHY_SET_DELAY_US);
-	}
-
 	/* Configure MDIO clock source frequency if clock is specified in the device tree */
 	ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
 	if (ret)
@@ -236,7 +228,7 @@  static int ipq4019_mdio_probe(struct platform_device *pdev)
 	struct ipq4019_mdio_data *priv;
 	struct mii_bus *bus;
 	struct resource *res;
-	int ret;
+	int ret, index;
 
 	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
 	if (!bus)
@@ -252,11 +244,32 @@  static int ipq4019_mdio_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->mdio_clk))
 		return PTR_ERR(priv->mdio_clk);
 
-	/* The platform resource is provided on the chipset IPQ5018 */
-	/* This resource is optional */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (res)
-		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
+	/* These platform resources are provided on the chipset IPQ5018 or
+	 * IPQ5332.
+	 */
+	/* 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);
+			}
+		}
+	}
 
 	bus->name = "ipq4019_mdio";
 	bus->read = ipq4019_mdio_read_c22;