[PATCH v4 0/5] support ipq5332 platform

[PATCH v4 0/5] support ipq5332 platform

[Luo Jie]

For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
connected with one of them.

1. The Ethernet LDO needs to be enabled to make the PHY GPIO
   reset taking effect, which uses the MDIO bus level reset.

2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
   to make the ethernet PHY device accessible.

3. To provide the clock to the ethernet, the CMN clock needs
   to be initialized for selecting reference clock and enabling
   the output clock.

4. Support optional MDIO clock frequency config.

5. Update dt-bindings doc for the new added properties.

Changes in v2:
	* remove the PHY related features such as PHY address
	  program and clock initialization.
	* leverage the MDIO level GPIO reset for qca8084 PHY.

Changes in v3:
	* fix the christmas-tree format issue.
	* improve the dt-binding changes.

Changes in v4:
	* improve the CMN PLL reference clock config.
	* improve the dt-binding changes.

Luo Jie (5):
  net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
  net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform
  net: mdio: ipq4019: configure CMN PLL clock for ipq5332
  net: mdio: ipq4019: support MDIO clock frequency divider
  dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

 .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
 drivers/net/mdio/mdio-ipq4019.c               | 288 ++++++++++++++++--
 2 files changed, 399 insertions(+), 30 deletions(-)


base-commit: 3b83fa94cf316aaf9ad9a367ac8031a06d31649b
-- 
2.42.0

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

[Luo Jie]

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) {
+			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;
-- 
2.42.0

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

[Konrad Dybcio]

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

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

[Jie Luo]

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.

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

[Russell King (Oracle)]

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!

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

[Jie Luo]

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.
> 

[PATCH v4 2/5] net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform

[Luo Jie]

On the platform ipq5332, the related SoC uniphy GCC clocks need
to be enabled for making the MDIO slave devices accessible.

These UNIPHY clocks are from the SoC platform GCC clock provider,
which are enabled for the connected PHY devices working.

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 5273864fabb3..e24b0e688b10 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -35,15 +35,36 @@
 /* MDIO clock source frequency is fixed to 100M */
 #define IPQ_MDIO_CLK_RATE	100000000
 
+/* SoC UNIPHY fixed clock */
+#define IPQ_UNIPHY_AHB_CLK_RATE	100000000
+#define IPQ_UNIPHY_SYS_CLK_RATE	24000000
+
 #define IPQ_PHY_SET_DELAY_US	100000
 
 /* Maximum SOC PCS(uniphy) number on IPQ platform */
 #define ETH_LDO_RDY_CNT				3
 
+enum mdio_clk_id {
+	MDIO_CLK_MDIO_AHB,
+	MDIO_CLK_UNIPHY0_AHB,
+	MDIO_CLK_UNIPHY0_SYS,
+	MDIO_CLK_UNIPHY1_AHB,
+	MDIO_CLK_UNIPHY1_SYS,
+	MDIO_CLK_CNT
+};
+
 struct ipq4019_mdio_data {
 	void __iomem *membase;
 	void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
-	struct clk *mdio_clk;
+	struct clk *clk[MDIO_CLK_CNT];
+};
+
+static const char *const mdio_clk_name[] = {
+	"gcc_mdio_ahb_clk",
+	"uniphy0_ahb",
+	"uniphy0_sys",
+	"uniphy1_ahb",
+	"uniphy1_sys"
 };
 
 static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
@@ -209,14 +230,43 @@ 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;
-	int ret;
+	unsigned long rate;
+	int ret, index;
 
-	/* 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)
-		return ret;
+	/* For the platform ipq5332, there are two SoC uniphies available
+	 * for connecting with ethernet PHY, the SoC uniphy gcc clock
+	 * should be enabled for resetting the connected device such
+	 * as qca8386 switch, qca8081 PHY or other PHYs effectively.
+	 *
+	 * Configure MDIO/UNIPHY clock source frequency if clock instance
+	 * is specified in the device tree.
+	 */
+	for (index = MDIO_CLK_MDIO_AHB; index < MDIO_CLK_CNT; index++) {
+		switch (index) {
+		case MDIO_CLK_MDIO_AHB:
+			rate = IPQ_MDIO_CLK_RATE;
+			break;
+		case MDIO_CLK_UNIPHY0_AHB:
+		case MDIO_CLK_UNIPHY1_AHB:
+			rate = IPQ_UNIPHY_AHB_CLK_RATE;
+			break;
+		case MDIO_CLK_UNIPHY0_SYS:
+		case MDIO_CLK_UNIPHY1_SYS:
+			rate = IPQ_UNIPHY_SYS_CLK_RATE;
+			break;
+		default:
+			break;
+		}
+
+		ret = clk_set_rate(priv->clk[index], rate);
+		if (ret)
+			return ret;
+
+		ret = clk_prepare_enable(priv->clk[index]);
+		if (ret)
+			return ret;
+	}
 
-	ret = clk_prepare_enable(priv->mdio_clk);
 	if (ret == 0)
 		mdelay(10);
 
@@ -240,10 +290,6 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->membase))
 		return PTR_ERR(priv->membase);
 
-	priv->mdio_clk = devm_clk_get_optional(&pdev->dev, "gcc_mdio_ahb_clk");
-	if (IS_ERR(priv->mdio_clk))
-		return PTR_ERR(priv->mdio_clk);
-
 	/* These platform resources are provided on the chipset IPQ5018 or
 	 * IPQ5332.
 	 */
@@ -271,6 +317,13 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 		}
 	}
 
+	for (index = 0; index < MDIO_CLK_CNT; index++) {
+		priv->clk[index] = devm_clk_get_optional(&pdev->dev,
+							 mdio_clk_name[index]);
+		if (IS_ERR(priv->clk[index]))
+			return PTR_ERR(priv->clk[index]);
+	}
+
 	bus->name = "ipq4019_mdio";
 	bus->read = ipq4019_mdio_read_c22;
 	bus->write = ipq4019_mdio_write_c22;
-- 
2.42.0

Re: [PATCH v4 2/5] net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform

[Bryan O'Donoghue]

On 25/12/2023 08:44, Luo Jie wrote:
> On the platform ipq5332, the related SoC uniphy GCC clocks need
> to be enabled for making the MDIO slave devices accessible.
> 
> These UNIPHY clocks are from the SoC platform GCC clock provider,
> which are enabled for the connected PHY devices working.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>   drivers/net/mdio/mdio-ipq4019.c | 75 ++++++++++++++++++++++++++++-----
>   1 file changed, 64 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
> index 5273864fabb3..e24b0e688b10 100644
> --- a/drivers/net/mdio/mdio-ipq4019.c
> +++ b/drivers/net/mdio/mdio-ipq4019.c
> @@ -35,15 +35,36 @@
>   /* MDIO clock source frequency is fixed to 100M */
>   #define IPQ_MDIO_CLK_RATE	100000000
>   
> +/* SoC UNIPHY fixed clock */
> +#define IPQ_UNIPHY_AHB_CLK_RATE	100000000
> +#define IPQ_UNIPHY_SYS_CLK_RATE	24000000
> +
>   #define IPQ_PHY_SET_DELAY_US	100000
>   
>   /* Maximum SOC PCS(uniphy) number on IPQ platform */
>   #define ETH_LDO_RDY_CNT				3
>   
> +enum mdio_clk_id {
> +	MDIO_CLK_MDIO_AHB,
> +	MDIO_CLK_UNIPHY0_AHB,
> +	MDIO_CLK_UNIPHY0_SYS,
> +	MDIO_CLK_UNIPHY1_AHB,
> +	MDIO_CLK_UNIPHY1_SYS,
> +	MDIO_CLK_CNT
> +};
> +
>   struct ipq4019_mdio_data {
>   	void __iomem *membase;
>   	void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
> -	struct clk *mdio_clk;
> +	struct clk *clk[MDIO_CLK_CNT];
> +};
> +
> +static const char *const mdio_clk_name[] = {
> +	"gcc_mdio_ahb_clk",
> +	"uniphy0_ahb",
> +	"uniphy0_sys",
> +	"uniphy1_ahb",
> +	"uniphy1_sys"
>   };
>   
>   static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
> @@ -209,14 +230,43 @@ 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;
> -	int ret;
> +	unsigned long rate;
> +	int ret, index;
>   
> -	/* 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)
> -		return ret;
> +	/* For the platform ipq5332, there are two SoC uniphies available
> +	 * for connecting with ethernet PHY, the SoC uniphy gcc clock
> +	 * should be enabled for resetting the connected device such
> +	 * as qca8386 switch, qca8081 PHY or other PHYs effectively.
> +	 *
> +	 * Configure MDIO/UNIPHY clock source frequency if clock instance
> +	 * is specified in the device tree.
> +	 */
> +	for (index = MDIO_CLK_MDIO_AHB; index < MDIO_CLK_CNT; index++) {

you could do a

if (!priv->clk[index])
	continue;

here and save a few cycles executing code for absent clocks. ipq6018 has 
just 1/5 of the clocks you are checking for here.

Better still capture the number of clocks you find in probe() in a 
variable priv->num_clocks and only step through the array

for (i = 0; i < priv->num_clocks; i++) {}

---
bod

Re: [PATCH v4 2/5] net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform

[Jie Luo]

On 1/3/2024 5:48 PM, Bryan O'Donoghue wrote:
> On 25/12/2023 08:44, Luo Jie wrote:
>> On the platform ipq5332, the related SoC uniphy GCC clocks need
>> to be enabled for making the MDIO slave devices accessible.
>>
>> These UNIPHY clocks are from the SoC platform GCC clock provider,
>> which are enabled for the connected PHY devices working.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   drivers/net/mdio/mdio-ipq4019.c | 75 ++++++++++++++++++++++++++++-----
>>   1 file changed, 64 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/mdio/mdio-ipq4019.c 
>> b/drivers/net/mdio/mdio-ipq4019.c
>> index 5273864fabb3..e24b0e688b10 100644
>> --- a/drivers/net/mdio/mdio-ipq4019.c
>> +++ b/drivers/net/mdio/mdio-ipq4019.c
>> @@ -35,15 +35,36 @@
>>   /* MDIO clock source frequency is fixed to 100M */
>>   #define IPQ_MDIO_CLK_RATE    100000000
>> +/* SoC UNIPHY fixed clock */
>> +#define IPQ_UNIPHY_AHB_CLK_RATE    100000000
>> +#define IPQ_UNIPHY_SYS_CLK_RATE    24000000
>> +
>>   #define IPQ_PHY_SET_DELAY_US    100000
>>   /* Maximum SOC PCS(uniphy) number on IPQ platform */
>>   #define ETH_LDO_RDY_CNT                3
>> +enum mdio_clk_id {
>> +    MDIO_CLK_MDIO_AHB,
>> +    MDIO_CLK_UNIPHY0_AHB,
>> +    MDIO_CLK_UNIPHY0_SYS,
>> +    MDIO_CLK_UNIPHY1_AHB,
>> +    MDIO_CLK_UNIPHY1_SYS,
>> +    MDIO_CLK_CNT
>> +};
>> +
>>   struct ipq4019_mdio_data {
>>       void __iomem *membase;
>>       void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
>> -    struct clk *mdio_clk;
>> +    struct clk *clk[MDIO_CLK_CNT];
>> +};
>> +
>> +static const char *const mdio_clk_name[] = {
>> +    "gcc_mdio_ahb_clk",
>> +    "uniphy0_ahb",
>> +    "uniphy0_sys",
>> +    "uniphy1_ahb",
>> +    "uniphy1_sys"
>>   };
>>   static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
>> @@ -209,14 +230,43 @@ 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;
>> -    int ret;
>> +    unsigned long rate;
>> +    int ret, index;
>> -    /* 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)
>> -        return ret;
>> +    /* For the platform ipq5332, there are two SoC uniphies available
>> +     * for connecting with ethernet PHY, the SoC uniphy gcc clock
>> +     * should be enabled for resetting the connected device such
>> +     * as qca8386 switch, qca8081 PHY or other PHYs effectively.
>> +     *
>> +     * Configure MDIO/UNIPHY clock source frequency if clock instance
>> +     * is specified in the device tree.
>> +     */
>> +    for (index = MDIO_CLK_MDIO_AHB; index < MDIO_CLK_CNT; index++) {
> 
> you could do a
> 
> if (!priv->clk[index])
>      continue;

Thanks for the comments, will update it.
> 
> here and save a few cycles executing code for absent clocks. ipq6018 has 
> just 1/5 of the clocks you are checking for here.
> 
> Better still capture the number of clocks you find in probe() in a 
> variable priv->num_clocks and only step through the array

Ok, thanks bod for the comments to improve the code.

> 
> for (i = 0; i < priv->num_clocks; i++) {}
> 
> ---
> bod

[PATCH v4 3/5] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

[Luo Jie]

The reference clock of CMN PLL block is selectable, the internal
48MHZ is used by default.

The output clock of CMN PLL block is for providing the clock
source of ethernet device(such as qca8084), there are 1 * 25MHZ
and 3 * 50MHZ output clocks available for the ethernet devices.

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index e24b0e688b10..e4862ac02026 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -44,6 +44,17 @@
 /* Maximum SOC PCS(uniphy) number on IPQ platform */
 #define ETH_LDO_RDY_CNT				3
 
+#define CMN_PLL_REFERENCE_SOURCE_SEL		0x28
+#define CMN_PLL_REFCLK_SOURCE_DIV		GENMASK(9, 8)
+
+#define CMN_PLL_REFERENCE_CLOCK			0x784
+#define CMN_PLL_REFCLK_EXTERNAL			BIT(9)
+#define CMN_PLL_REFCLK_DIV			GENMASK(8, 4)
+#define CMN_PLL_REFCLK_INDEX			GENMASK(3, 0)
+
+#define CMN_PLL_POWER_ON_AND_RESET		0x780
+#define CMN_ANA_EN_SW_RSTN			BIT(6)
+
 enum mdio_clk_id {
 	MDIO_CLK_MDIO_AHB,
 	MDIO_CLK_UNIPHY0_AHB,
@@ -55,6 +66,7 @@ enum mdio_clk_id {
 
 struct ipq4019_mdio_data {
 	void __iomem *membase;
+	void __iomem *cmn_membase;
 	void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
 	struct clk *clk[MDIO_CLK_CNT];
 };
@@ -227,12 +239,116 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
 	return 0;
 }
 
+/* For the CMN PLL block, the reference clock can be configured according to
+ * the device tree property "qcom,cmn-ref-clock-frequency", the internal 48MHZ
+ * is used by default.
+ *
+ * The output clock of CMN PLL block is provided to the ethernet devices,
+ * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default.
+ *
+ * Such as the output 50M clock for the qca8084 ethernet PHY.
+ */
+static int ipq_cmn_clock_config(struct mii_bus *bus)
+{
+	struct ipq4019_mdio_data *priv;
+	u32 reg_val, src_sel, ref_clk;
+	int ret;
+
+	priv = bus->priv;
+	if (priv->cmn_membase) {
+		reg_val = readl(priv->cmn_membase + CMN_PLL_REFERENCE_CLOCK);
+
+		/* Select reference clock source of CMN PLL block, which can
+		 * be from wifi module or the external xtal.
+		 *
+		 * If absent, the wifi internal 48MHz is used as the reference
+		 * clock source of CMN PLL block, if the 48MHZ is specified,
+		 * which means the xtal 48MHZ is selected.
+		 */
+		ret = of_property_read_u32(bus->parent->of_node,
+					   "qcom,cmn-ref-clock-frequency",
+					   &ref_clk);
+		if (!ret) {
+			switch (ref_clk) {
+			case 25000000:
+				reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+					     CMN_PLL_REFCLK_INDEX);
+				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 3));
+				break;
+			case 31250000:
+				reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+					     CMN_PLL_REFCLK_INDEX);
+				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 4));
+				break;
+			case 40000000:
+				reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+					     CMN_PLL_REFCLK_INDEX);
+				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 6));
+				break;
+			case 48000000:
+				reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+					     CMN_PLL_REFCLK_INDEX);
+				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7));
+				break;
+			case 50000000:
+				reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+					     CMN_PLL_REFCLK_INDEX);
+				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8));
+				break;
+			case 96000000:
+				src_sel = readl(priv->cmn_membase +
+						CMN_PLL_REFERENCE_SOURCE_SEL);
+				src_sel &= ~CMN_PLL_REFCLK_SOURCE_DIV;
+				src_sel |= FIELD_PREP(CMN_PLL_REFCLK_SOURCE_DIV, 0);
+				writel(src_sel, priv->cmn_membase +
+				       CMN_PLL_REFERENCE_SOURCE_SEL);
+
+				reg_val &= ~CMN_PLL_REFCLK_DIV;
+				reg_val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2);
+				break;
+			default:
+				return -EINVAL;
+			}
+		} else if (ret == -EINVAL) {
+			/* the internal 48MHZ is selected by default. */
+			reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_INDEX);
+			reg_val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
+		} else {
+			return ret;
+		}
+
+		writel(reg_val, priv->cmn_membase + CMN_PLL_REFERENCE_CLOCK);
+
+		/* assert CMN PLL */
+		reg_val = readl(priv->cmn_membase + CMN_PLL_POWER_ON_AND_RESET);
+		reg_val &= ~CMN_ANA_EN_SW_RSTN;
+		writel(reg_val, priv->cmn_membase);
+		fsleep(IPQ_PHY_SET_DELAY_US);
+
+		/* deassert CMN PLL */
+		reg_val |= CMN_ANA_EN_SW_RSTN;
+		writel(reg_val, priv->cmn_membase + CMN_PLL_POWER_ON_AND_RESET);
+		fsleep(IPQ_PHY_SET_DELAY_US);
+	}
+
+	return 0;
+}
+
 static int ipq_mdio_reset(struct mii_bus *bus)
 {
 	struct ipq4019_mdio_data *priv = bus->priv;
 	unsigned long rate;
 	int ret, index;
 
+	ret = ipq_cmn_clock_config(bus);
+	if (ret)
+		return ret;
+
 	/* For the platform ipq5332, there are two SoC uniphies available
 	 * for connecting with ethernet PHY, the SoC uniphy gcc clock
 	 * should be enabled for resetting the connected device such
@@ -296,7 +412,7 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 	/* 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 && strcmp(res->name, "cmn_blk")) {
 			priv->eth_ldo_rdy[index] = devm_ioremap(&pdev->dev,
 								res->start,
 								resource_size(res));
@@ -317,6 +433,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 		}
 	}
 
+	/* The CMN block resource is for providing clock source to ethernet,
+	 * which can be optionally configured on the platform ipq9574 and
+	 * ipq5332.
+	 */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk");
+	if (res) {
+		priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(priv->cmn_membase))
+			return PTR_ERR(priv->cmn_membase);
+	}
+
 	for (index = 0; index < MDIO_CLK_CNT; index++) {
 		priv->clk[index] = devm_clk_get_optional(&pdev->dev,
 							 mdio_clk_name[index]);
-- 
2.42.0

Re: [PATCH v4 3/5] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

[Bryan O'Donoghue]

On 25/12/2023 08:44, Luo Jie wrote:
> The reference clock of CMN PLL block is selectable, the internal
> 48MHZ is used by default.
> 
> The output clock of CMN PLL block is for providing the clock
> source of ethernet device(such as qca8084), there are 1 * 25MHZ
> and 3 * 50MHZ output clocks available for the ethernet devices.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>   drivers/net/mdio/mdio-ipq4019.c | 129 +++++++++++++++++++++++++++++++-
>   1 file changed, 128 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
> index e24b0e688b10..e4862ac02026 100644
> --- a/drivers/net/mdio/mdio-ipq4019.c
> +++ b/drivers/net/mdio/mdio-ipq4019.c
> @@ -44,6 +44,17 @@
>   /* Maximum SOC PCS(uniphy) number on IPQ platform */
>   #define ETH_LDO_RDY_CNT				3
>   
> +#define CMN_PLL_REFERENCE_SOURCE_SEL		0x28
> +#define CMN_PLL_REFCLK_SOURCE_DIV		GENMASK(9, 8)
> +
> +#define CMN_PLL_REFERENCE_CLOCK			0x784
> +#define CMN_PLL_REFCLK_EXTERNAL			BIT(9)
> +#define CMN_PLL_REFCLK_DIV			GENMASK(8, 4)
> +#define CMN_PLL_REFCLK_INDEX			GENMASK(3, 0)
> +
> +#define CMN_PLL_POWER_ON_AND_RESET		0x780
> +#define CMN_ANA_EN_SW_RSTN			BIT(6)
> +
>   enum mdio_clk_id {
>   	MDIO_CLK_MDIO_AHB,
>   	MDIO_CLK_UNIPHY0_AHB,
> @@ -55,6 +66,7 @@ enum mdio_clk_id {
>   
>   struct ipq4019_mdio_data {
>   	void __iomem *membase;
> +	void __iomem *cmn_membase;
>   	void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
>   	struct clk *clk[MDIO_CLK_CNT];
>   };
> @@ -227,12 +239,116 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
>   	return 0;
>   }
>   
> +/* For the CMN PLL block, the reference clock can be configured according to
> + * the device tree property "qcom,cmn-ref-clock-frequency", the internal 48MHZ
> + * is used by default.
> + *
> + * The output clock of CMN PLL block is provided to the ethernet devices,
> + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default.
> + *
> + * Such as the output 50M clock for the qca8084 ethernet PHY.
> + */
> +static int ipq_cmn_clock_config(struct mii_bus *bus)
> +{
> +	struct ipq4019_mdio_data *priv;
> +	u32 reg_val, src_sel, ref_clk;
> +	int ret;
> +
> +	priv = bus->priv;
> +	if (priv->cmn_membase) {

if (!priv->cnm_membase)
	return 0;

then move the indentation here one tab left.

---
bod

Re: [PATCH v4 3/5] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

[Jie Luo]

On 1/3/2024 5:50 PM, Bryan O'Donoghue wrote:
> On 25/12/2023 08:44, Luo Jie wrote:
>> The reference clock of CMN PLL block is selectable, the internal
>> 48MHZ is used by default.
>>
>> The output clock of CMN PLL block is for providing the clock
>> source of ethernet device(such as qca8084), there are 1 * 25MHZ
>> and 3 * 50MHZ output clocks available for the ethernet devices.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   drivers/net/mdio/mdio-ipq4019.c | 129 +++++++++++++++++++++++++++++++-
>>   1 file changed, 128 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/mdio/mdio-ipq4019.c 
>> b/drivers/net/mdio/mdio-ipq4019.c
>> index e24b0e688b10..e4862ac02026 100644
>> --- a/drivers/net/mdio/mdio-ipq4019.c
>> +++ b/drivers/net/mdio/mdio-ipq4019.c
>> @@ -44,6 +44,17 @@
>>   /* Maximum SOC PCS(uniphy) number on IPQ platform */
>>   #define ETH_LDO_RDY_CNT                3
>> +#define CMN_PLL_REFERENCE_SOURCE_SEL        0x28
>> +#define CMN_PLL_REFCLK_SOURCE_DIV        GENMASK(9, 8)
>> +
>> +#define CMN_PLL_REFERENCE_CLOCK            0x784
>> +#define CMN_PLL_REFCLK_EXTERNAL            BIT(9)
>> +#define CMN_PLL_REFCLK_DIV            GENMASK(8, 4)
>> +#define CMN_PLL_REFCLK_INDEX            GENMASK(3, 0)
>> +
>> +#define CMN_PLL_POWER_ON_AND_RESET        0x780
>> +#define CMN_ANA_EN_SW_RSTN            BIT(6)
>> +
>>   enum mdio_clk_id {
>>       MDIO_CLK_MDIO_AHB,
>>       MDIO_CLK_UNIPHY0_AHB,
>> @@ -55,6 +66,7 @@ enum mdio_clk_id {
>>   struct ipq4019_mdio_data {
>>       void __iomem *membase;
>> +    void __iomem *cmn_membase;
>>       void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
>>       struct clk *clk[MDIO_CLK_CNT];
>>   };
>> @@ -227,12 +239,116 @@ static int ipq4019_mdio_write_c22(struct 
>> mii_bus *bus, int mii_id, int regnum,
>>       return 0;
>>   }
>> +/* For the CMN PLL block, the reference clock can be configured 
>> according to
>> + * the device tree property "qcom,cmn-ref-clock-frequency", the 
>> internal 48MHZ
>> + * is used by default.
>> + *
>> + * The output clock of CMN PLL block is provided to the ethernet 
>> devices,
>> + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by 
>> default.
>> + *
>> + * Such as the output 50M clock for the qca8084 ethernet PHY.
>> + */
>> +static int ipq_cmn_clock_config(struct mii_bus *bus)
>> +{
>> +    struct ipq4019_mdio_data *priv;
>> +    u32 reg_val, src_sel, ref_clk;
>> +    int ret;
>> +
>> +    priv = bus->priv;
>> +    if (priv->cmn_membase) {
> 
> if (!priv->cnm_membase)
>      return 0;
> 
> then move the indentation here one tab left.
> 
Ok, will update this, Thanks.

> ---
> bod

[PATCH v4 4/5] net: mdio: ipq4019: support MDIO clock frequency divider

[Luo Jie]

The MDIO clock frequency can be divided according to the
MDIO control register value.

The MDIO system clock is fixed to 100MHZ, the working
frequency is 100MHZ/(divider + 1), the divider value
is from the bit[7:0] of control register 0x40.

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index e4862ac02026..fd41dd7ff9cb 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -29,6 +29,9 @@
 /* 0 = Clause 22, 1 = Clause 45 */
 #define MDIO_MODE_C45				BIT(8)
 
+/* MDC frequency is SYS_CLK/(MDIO_CLK_DIV + 1), SYS_CLK is 100MHz */
+#define MDIO_CLK_DIV_MASK			GENMASK(7, 0)
+
 #define IPQ4019_MDIO_TIMEOUT	10000
 #define IPQ4019_MDIO_SLEEP		10
 
@@ -69,6 +72,7 @@ struct ipq4019_mdio_data {
 	void __iomem *cmn_membase;
 	void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
 	struct clk *clk[MDIO_CLK_CNT];
+	int clk_div;
 };
 
 static const char *const mdio_clk_name[] = {
@@ -102,6 +106,7 @@ static int ipq4019_mdio_read_c45(struct mii_bus *bus, int mii_id, int mmd,
 	data = readl(priv->membase + MDIO_MODE_REG);
 
 	data |= MDIO_MODE_C45;
+	data |= FIELD_PREP(MDIO_CLK_DIV_MASK, priv->clk_div);
 
 	writel(data, priv->membase + MDIO_MODE_REG);
 
@@ -143,6 +148,7 @@ static int ipq4019_mdio_read_c22(struct mii_bus *bus, int mii_id, int regnum)
 	data = readl(priv->membase + MDIO_MODE_REG);
 
 	data &= ~MDIO_MODE_C45;
+	data |= FIELD_PREP(MDIO_CLK_DIV_MASK, priv->clk_div);
 
 	writel(data, priv->membase + MDIO_MODE_REG);
 
@@ -175,6 +181,7 @@ static int ipq4019_mdio_write_c45(struct mii_bus *bus, int mii_id, int mmd,
 	data = readl(priv->membase + MDIO_MODE_REG);
 
 	data |= MDIO_MODE_C45;
+	data |= FIELD_PREP(MDIO_CLK_DIV_MASK, priv->clk_div);
 
 	writel(data, priv->membase + MDIO_MODE_REG);
 
@@ -218,6 +225,7 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
 	data = readl(priv->membase + MDIO_MODE_REG);
 
 	data &= ~MDIO_MODE_C45;
+	data |= FIELD_PREP(MDIO_CLK_DIV_MASK, priv->clk_div);
 
 	writel(data, priv->membase + MDIO_MODE_REG);
 
@@ -389,6 +397,39 @@ static int ipq_mdio_reset(struct mii_bus *bus)
 	return ret;
 }
 
+static int ipq_mdio_clk_set(struct platform_device *pdev, int *clk_div)
+{
+	int freq;
+
+	/* Keep the MDIO clock divider as the hardware default value 0xff if
+	 * the MDIO property "clock-frequency" is not specified.
+	 */
+	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency", &freq)) {
+		*clk_div = 0xff;
+		return 0;
+	}
+
+	/* MDC frequency is SYS_CLK/(MDIO_CLK_DIV + 1), SYS_CLK is fixed
+	 * to 100MHz, the MDIO_CLK_DIV can be only configured the valid
+	 * values, other values cause malfunction.
+	 */
+	switch (freq) {
+	case 390625:
+	case 781250:
+	case 1562500:
+	case 3125000:
+	case 6250000:
+	case 12500000:
+		*clk_div = DIV_ROUND_UP(IPQ_MDIO_CLK_RATE, freq) - 1;
+		break;
+	default:
+		dev_err(&pdev->dev, "Invalid clock frequency %dHZ\n", freq);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int ipq4019_mdio_probe(struct platform_device *pdev)
 {
 	struct ipq4019_mdio_data *priv;
@@ -451,6 +492,10 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 			return PTR_ERR(priv->clk[index]);
 	}
 
+	ret = ipq_mdio_clk_set(pdev, &priv->clk_div);
+	if (ret)
+		return ret;
+
 	bus->name = "ipq4019_mdio";
 	bus->read = ipq4019_mdio_read_c22;
 	bus->write = ipq4019_mdio_write_c22;
-- 
2.42.0

[PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Luo Jie]

Update the yaml file for the new DTS properties.

1. qcom,cmn-ref-clock-frequency for the CMN PLL source clock select.
2. clock-frequency for MDIO clock frequency config.
3. add uniphy AHB & SYS GCC clocks.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 +++++++++++++++++-
 1 file changed, 136 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
index 3407e909e8a7..205500cb1fd1 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
@@ -18,8 +18,10 @@ properties:
 
       - items:
           - enum:
+              - qcom,ipq5332-mdio
               - qcom,ipq6018-mdio
               - qcom,ipq8074-mdio
+              - qcom,ipq9574-mdio
           - const: qcom,ipq4019-mdio
 
   "#address-cells":
@@ -30,19 +32,76 @@ properties:
 
   reg:
     minItems: 1
-    maxItems: 2
-    description:
-      the first Address and length of the register set for the MDIO controller.
-      the second Address and length of the register for ethernet LDO, this second
-      address range is only required by the platform IPQ50xx.
+    maxItems: 5
+    description: |
+      The first address and length of the register set for the MDIO controller,
+      the optional second address and length of the register is for CMN block,
+      the optional third, fourth and fifth address and length of the register
+      for Ethernet LDO, the optional Ethernet LDO address range is required by
+      the platform IPQ50xx/IPQ5332.
+
+  reg-names:
+    minItems: 1
+    items:
+      - const: mdio
+      - const: cmn_blk
+      - const: eth_ldo1
+      - const: eth_ldo2
+      - const: eth_ldo3
 
   clocks:
+    minItems: 1
     items:
       - description: MDIO clock source frequency fixed to 100MHZ
+      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
+      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
+      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
+      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
 
   clock-names:
+    minItems: 1
     items:
       - const: gcc_mdio_ahb_clk
+      - const: uniphy0_ahb
+      - const: uniphy1_ahb
+      - const: uniphy0_sys
+      - const: uniphy1_sys
+
+  qcom,cmn-ref-clock-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum:
+      - 25000000
+      - 31250000
+      - 40000000
+      - 48000000
+      - 50000000
+      - 96000000
+    default: 48000000
+    description: |
+      The reference clock source of CMN PLL block is selectable, the
+      reference clock source can be from wifi module or the external
+      xtal, the reference clock frequency 48MHZ can be from internal
+      wifi or the external xtal, if absent, the internal 48MHZ is used,
+      if the 48MHZ is specified, which means the external 48Mhz is used.
+
+  clock-frequency:
+    enum:
+      - 390625
+      - 781250
+      - 1562500
+      - 3125000
+      - 6250000
+      - 12500000
+    default: 390625
+    description: |
+      The MDIO bus clock that must be output by the MDIO bus hardware,
+      only the listed frequencies above can be supported, other frequency
+      will cause malfunction. If absent, the default hardware value 0xff
+      is used, which means the default MDIO clock frequency 390625HZ, The
+      MDIO clock frequency is MDIO_SYS_CLK/(MDIO_CLK_DIV + 1), the SoC
+      MDIO_SYS_CLK is fixed to 100MHZ, the MDIO_CLK_DIV is from MDIO control
+      register, there is higher clock frequency requirement on the normal
+      working case where the MDIO slave devices support high clock frequency.
 
 required:
   - compatible
@@ -59,8 +118,10 @@ allOf:
           contains:
             enum:
               - qcom,ipq5018-mdio
+              - qcom,ipq5332-mdio
               - qcom,ipq6018-mdio
               - qcom,ipq8074-mdio
+              - qcom,ipq9574-mdio
     then:
       required:
         - clocks
@@ -70,6 +131,20 @@ allOf:
         clocks: false
         clock-names: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq5332-mdio
+    then:
+      properties:
+        clocks:
+          minItems: 5
+          maxItems: 5
+        reg-names:
+          minItems: 4
+
 unevaluatedProperties: false
 
 examples:
@@ -100,3 +175,59 @@ examples:
         reg = <4>;
       };
     };
+
+  - |
+    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    mdio@90000 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      compatible = "qcom,ipq5332-mdio",
+                   "qcom,ipq4019-mdio";
+
+      reg = <0x90000 0x64>,
+            <0x9b000 0x800>,
+            <0x7a00610 0x4>,
+            <0x7a10610 0x4>;
+
+      reg-names = "mdio",
+                  "cmn_blk",
+                  "eth_ldo1",
+                  "eth_ldo2";
+
+      clocks = <&gcc GCC_MDIO_AHB_CLK>,
+               <&gcc GCC_UNIPHY0_AHB_CLK>,
+               <&gcc GCC_UNIPHY1_AHB_CLK>,
+               <&gcc GCC_UNIPHY0_SYS_CLK>,
+               <&gcc GCC_UNIPHY1_SYS_CLK>;
+
+      clock-names = "gcc_mdio_ahb_clk",
+                    "uniphy0_ahb",
+                    "uniphy1_ahb",
+                    "uniphy0_sys",
+                    "uniphy1_sys";
+
+      clock-frequency = <6250000>;
+      reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>;
+
+      qca8kphy0: ethernet-phy@1 {
+        compatible = "ethernet-phy-id004d.d180";
+        reg = <1>;
+      };
+
+      qca8kphy1: ethernet-phy@2 {
+        compatible = "ethernet-phy-id004d.d180";
+        reg = <2>;
+      };
+
+      qca8kphy2: ethernet-phy@3 {
+        compatible = "ethernet-phy-id004d.d180";
+        reg = <3>;
+      };
+
+      qca8kphy3: ethernet-phy@4 {
+        compatible = "ethernet-phy-id004d.d180";
+        reg = <4>;
+      };
+    };
-- 
2.42.0

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Krzysztof Kozlowski]

On 25/12/2023 09:44, Luo Jie wrote:
> Update the yaml file for the new DTS properties.
> 
> 1. qcom,cmn-ref-clock-frequency for the CMN PLL source clock select.
> 2. clock-frequency for MDIO clock frequency config.
> 3. add uniphy AHB & SYS GCC clocks.

I see two new compatibles, so your list is missing main point.

> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 +++++++++++++++++-
>  1 file changed, 136 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> index 3407e909e8a7..205500cb1fd1 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> @@ -18,8 +18,10 @@ properties:
>  
>        - items:
>            - enum:
> +              - qcom,ipq5332-mdio
>                - qcom,ipq6018-mdio
>                - qcom,ipq8074-mdio
> +              - qcom,ipq9574-mdio
>            - const: qcom,ipq4019-mdio
>  
>    "#address-cells":
> @@ -30,19 +32,76 @@ properties:
>  
>    reg:
>      minItems: 1
> -    maxItems: 2
> -    description:
> -      the first Address and length of the register set for the MDIO controller.
> -      the second Address and length of the register for ethernet LDO, this second
> -      address range is only required by the platform IPQ50xx.
> +    maxItems: 5
> +    description: |
> +      The first address and length of the register set for the MDIO controller,
> +      the optional second address and length of the register is for CMN block,
> +      the optional third, fourth and fifth address and length of the register
> +      for Ethernet LDO, the optional Ethernet LDO address range is required by

Wait, required? You said in in response to Rob these are not required!

> +      the platform IPQ50xx/IPQ5332.

So these are valid for all platforms or not? Looks not, but nothing
narrows the list for other boards.

Anyway, why do you add entries in the middle? LDO was the second, so it
cannot be now fifth.

> +
> +  reg-names:
> +    minItems: 1
> +    items:
> +      - const: mdio
> +      - const: cmn_blk
> +      - const: eth_ldo1
> +      - const: eth_ldo2
> +      - const: eth_ldo3
>  
>    clocks:
> +    minItems: 1
>      items:
>        - description: MDIO clock source frequency fixed to 100MHZ
> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>  
>    clock-names:
> +    minItems: 1
>      items:
>        - const: gcc_mdio_ahb_clk
> +      - const: uniphy0_ahb
> +      - const: uniphy1_ahb
> +      - const: uniphy0_sys
> +      - const: uniphy1_sys
> +
> +  qcom,cmn-ref-clock-frequency:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum:
> +      - 25000000
> +      - 31250000
> +      - 40000000
> +      - 48000000
> +      - 50000000
> +      - 96000000
> +    default: 48000000
> +    description: |
> +      The reference clock source of CMN PLL block is selectable, the
> +      reference clock source can be from wifi module or the external
> +      xtal, the reference clock frequency 48MHZ can be from internal
> +      wifi or the external xtal, if absent, the internal 48MHZ is used,
> +      if the 48MHZ is specified, which means the external 48Mhz is used.

This does not resolve mine and Conor's concerns from previous version.
External clocks are defined as clock inputs.

> +
> +  clock-frequency:
> +    enum:
> +      - 390625
> +      - 781250
> +      - 1562500
> +      - 3125000
> +      - 6250000
> +      - 12500000
> +    default: 390625
> +    description: |
> +      The MDIO bus clock that must be output by the MDIO bus hardware,
> +      only the listed frequencies above can be supported, other frequency
> +      will cause malfunction. If absent, the default hardware value 0xff
> +      is used, which means the default MDIO clock frequency 390625HZ, The
> +      MDIO clock frequency is MDIO_SYS_CLK/(MDIO_CLK_DIV + 1), the SoC
> +      MDIO_SYS_CLK is fixed to 100MHZ, the MDIO_CLK_DIV is from MDIO control
> +      register, there is higher clock frequency requirement on the normal
> +      working case where the MDIO slave devices support high clock frequency.
>  
>  required:
>    - compatible
> @@ -59,8 +118,10 @@ allOf:
>            contains:
>              enum:
>                - qcom,ipq5018-mdio
> +              - qcom,ipq5332-mdio
>                - qcom,ipq6018-mdio
>                - qcom,ipq8074-mdio
> +              - qcom,ipq9574-mdio
>      then:
>        required:
>          - clocks
> @@ -70,6 +131,20 @@ allOf:
>          clocks: false
>          clock-names: false
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq5332-mdio
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 5
> +          maxItems: 5
> +        reg-names:
> +          minItems: 4

Why all other variants now have 5 clocks and 5 reg entries? Nothing of
it is explained in the commit msg.

> +
>  unevaluatedProperties: false
>  
>  examples:
> @@ -100,3 +175,59 @@ examples:
>          reg = <4>;
>        };
>      };
> +
> +  - |
> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    mdio@90000 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;

That's not the order of properties. compatible is always the first, reg
and reg-names follow. See DTS coding style.

> +      compatible = "qcom,ipq5332-mdio",
> +                   "qcom,ipq4019-mdio";
> +
> +      reg = <0x90000 0x64>,
> +            <0x9b000 0x800>,
> +            <0x7a00610 0x4>,
> +            <0x7a10610 0x4>;
> +

Drop blank line.

> +      reg-names = "mdio",
> +                  "cmn_blk",
> +                  "eth_ldo1",
> +                  "eth_ldo2";
> +
> +      clocks = <&gcc GCC_MDIO_AHB_CLK>,
> +               <&gcc GCC_UNIPHY0_AHB_CLK>,
> +               <&gcc GCC_UNIPHY1_AHB_CLK>,
> +               <&gcc GCC_UNIPHY0_SYS_CLK>,
> +               <&gcc GCC_UNIPHY1_SYS_CLK>;
> +

Drop blank line

> +      clock-names = "gcc_mdio_ahb_clk",
> +                    "uniphy0_ahb",
> +                    "uniphy1_ahb",
> +                    "uniphy0_sys",
> +                    "uniphy1_sys";
> +
> +      clock-frequency = <6250000>;
> +      reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>;
> +

Best regards,
Krzysztof

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Jie Luo]

On 12/25/2023 6:29 PM, Krzysztof Kozlowski wrote:
> On 25/12/2023 09:44, Luo Jie wrote:
>> Update the yaml file for the new DTS properties.
>>
>> 1. qcom,cmn-ref-clock-frequency for the CMN PLL source clock select.
>> 2. clock-frequency for MDIO clock frequency config.
>> 3. add uniphy AHB & SYS GCC clocks.
> 
> I see two new compatibles, so your list is missing main point.

will add the compatibles into the list, thanks.

> 
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 +++++++++++++++++-
>>   1 file changed, 136 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> index 3407e909e8a7..205500cb1fd1 100644
>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> @@ -18,8 +18,10 @@ properties:
>>   
>>         - items:
>>             - enum:
>> +              - qcom,ipq5332-mdio
>>                 - qcom,ipq6018-mdio
>>                 - qcom,ipq8074-mdio
>> +              - qcom,ipq9574-mdio
>>             - const: qcom,ipq4019-mdio
>>   
>>     "#address-cells":
>> @@ -30,19 +32,76 @@ properties:
>>   
>>     reg:
>>       minItems: 1
>> -    maxItems: 2
>> -    description:
>> -      the first Address and length of the register set for the MDIO controller.
>> -      the second Address and length of the register for ethernet LDO, this second
>> -      address range is only required by the platform IPQ50xx.
>> +    maxItems: 5
>> +    description: |
>> +      The first address and length of the register set for the MDIO controller,
>> +      the optional second address and length of the register is for CMN block,
>> +      the optional third, fourth and fifth address and length of the register
>> +      for Ethernet LDO, the optional Ethernet LDO address range is required by
> 
> Wait, required? You said in in response to Rob these are not required!

As for the response to Rob, i was saying the uniphy ahb and sys clocks
are not needed on ipq9574.
The LDO are needed on ipq5332 and ipq5018 currently.

> 
>> +      the platform IPQ50xx/IPQ5332.
> 
> So these are valid for all platforms or not? Looks not, but nothing
> narrows the list for other boards.

i add the limitation on the reg usage for the ipq5332 platform on the
following part "if condition" of this patch, i will update the patch
to narrow down for the other compatibles.

> 
> Anyway, why do you add entries in the middle? LDO was the second, so it
> cannot be now fifth.

As Rob's suggestion, i move the cmn_blk to second location for
simplifying the limitation description, i checked the upstream dts code,
the LDO is not used currently, so we can move cmn_blk to the second
location here.

> 
>> +
>> +  reg-names:
>> +    minItems: 1
>> +    items:
>> +      - const: mdio
>> +      - const: cmn_blk
>> +      - const: eth_ldo1
>> +      - const: eth_ldo2
>> +      - const: eth_ldo3
>>   
>>     clocks:
>> +    minItems: 1
>>       items:
>>         - description: MDIO clock source frequency fixed to 100MHZ
>> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>   
>>     clock-names:
>> +    minItems: 1
>>       items:
>>         - const: gcc_mdio_ahb_clk
>> +      - const: uniphy0_ahb
>> +      - const: uniphy1_ahb
>> +      - const: uniphy0_sys
>> +      - const: uniphy1_sys
>> +
>> +  qcom,cmn-ref-clock-frequency:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum:
>> +      - 25000000
>> +      - 31250000
>> +      - 40000000
>> +      - 48000000
>> +      - 50000000
>> +      - 96000000
>> +    default: 48000000
>> +    description: |
>> +      The reference clock source of CMN PLL block is selectable, the
>> +      reference clock source can be from wifi module or the external
>> +      xtal, the reference clock frequency 48MHZ can be from internal
>> +      wifi or the external xtal, if absent, the internal 48MHZ is used,
>> +      if the 48MHZ is specified, which means the external 48Mhz is used.
> 
> This does not resolve mine and Conor's concerns from previous version.
> External clocks are defined as clock inputs.

No matter the external or internal reference clock, they are the clock
source selection for CMN, there are only 48MHZ can be external or 
internal, other clocks have the different clock rate, so the internal
48MHZ reference clock can be implied when the 
"qcom,cmn-ref-clock-frequency" is not defined, which is suggested by 
Conor in the previous
comments.

> 
>> +
>> +  clock-frequency:
>> +    enum:
>> +      - 390625
>> +      - 781250
>> +      - 1562500
>> +      - 3125000
>> +      - 6250000
>> +      - 12500000
>> +    default: 390625
>> +    description: |
>> +      The MDIO bus clock that must be output by the MDIO bus hardware,
>> +      only the listed frequencies above can be supported, other frequency
>> +      will cause malfunction. If absent, the default hardware value 0xff
>> +      is used, which means the default MDIO clock frequency 390625HZ, The
>> +      MDIO clock frequency is MDIO_SYS_CLK/(MDIO_CLK_DIV + 1), the SoC
>> +      MDIO_SYS_CLK is fixed to 100MHZ, the MDIO_CLK_DIV is from MDIO control
>> +      register, there is higher clock frequency requirement on the normal
>> +      working case where the MDIO slave devices support high clock frequency.
>>   
>>   required:
>>     - compatible
>> @@ -59,8 +118,10 @@ allOf:
>>             contains:
>>               enum:
>>                 - qcom,ipq5018-mdio
>> +              - qcom,ipq5332-mdio
>>                 - qcom,ipq6018-mdio
>>                 - qcom,ipq8074-mdio
>> +              - qcom,ipq9574-mdio
>>       then:
>>         required:
>>           - clocks
>> @@ -70,6 +131,20 @@ allOf:
>>           clocks: false
>>           clock-names: false
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq5332-mdio
>> +    then:
>> +      properties:
>> +        clocks:
>> +          minItems: 5
>> +          maxItems: 5
>> +        reg-names:
>> +          minItems: 4
> 
> Why all other variants now have 5 clocks and 5 reg entries? Nothing of
> it is explained in the commit msg.

 From the condition above, only "qcom,ipq5332-mdio" has 5 clocks (mdio +
4 uniphy clocks) and 4 regs (mdio + cmn_blk + 2 LDOs) as the cmn_blk is
moved to the second location.

how it can gives the 5 clocks and 5 regs for other variants here?


> 
>> +
>>   unevaluatedProperties: false
>>   
>>   examples:
>> @@ -100,3 +175,59 @@ examples:
>>           reg = <4>;
>>         };
>>       };
>> +
>> +  - |
>> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    mdio@90000 {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
> 
> That's not the order of properties. compatible is always the first, reg
> and reg-names follow. See DTS coding style.

will correct this, thanks.

> 
>> +      compatible = "qcom,ipq5332-mdio",
>> +                   "qcom,ipq4019-mdio";
>> +
>> +      reg = <0x90000 0x64>,
>> +            <0x9b000 0x800>,
>> +            <0x7a00610 0x4>,
>> +            <0x7a10610 0x4>;
>> +
> 
> Drop blank line.
Ok.

> 
>> +      reg-names = "mdio",
>> +                  "cmn_blk",
>> +                  "eth_ldo1",
>> +                  "eth_ldo2";
>> +
>> +      clocks = <&gcc GCC_MDIO_AHB_CLK>,
>> +               <&gcc GCC_UNIPHY0_AHB_CLK>,
>> +               <&gcc GCC_UNIPHY1_AHB_CLK>,
>> +               <&gcc GCC_UNIPHY0_SYS_CLK>,
>> +               <&gcc GCC_UNIPHY1_SYS_CLK>;
>> +
> 
> Drop blank line
Ok.

> 
>> +      clock-names = "gcc_mdio_ahb_clk",
>> +                    "uniphy0_ahb",
>> +                    "uniphy1_ahb",
>> +                    "uniphy0_sys",
>> +                    "uniphy1_sys";
>> +
>> +      clock-frequency = <6250000>;
>> +      reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>;
>> +
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Krzysztof Kozlowski]

On 26/12/2023 08:25, Jie Luo wrote:
>>> -    description:
>>> -      the first Address and length of the register set for the MDIO controller.
>>> -      the second Address and length of the register for ethernet LDO, this second
>>> -      address range is only required by the platform IPQ50xx.
>>> +    maxItems: 5
>>> +    description: |
>>> +      The first address and length of the register set for the MDIO controller,
>>> +      the optional second address and length of the register is for CMN block,
>>> +      the optional third, fourth and fifth address and length of the register
>>> +      for Ethernet LDO, the optional Ethernet LDO address range is required by
>>
>> Wait, required? You said in in response to Rob these are not required!
> 
> As for the response to Rob, i was saying the uniphy ahb and sys clocks
> are not needed on ipq9574.
> The LDO are needed on ipq5332 and ipq5018 currently.

Clocks as well but:

"A driver can function without knowing about all these new registers and
..."

Anyway, this should be list ("items:") with descriptions, instead of one
big description listing things.


> 
>>
>>> +      the platform IPQ50xx/IPQ5332.
>>
>> So these are valid for all platforms or not? Looks not, but nothing
>> narrows the list for other boards.
> 
> i add the limitation on the reg usage for the ipq5332 platform on the
> following part "if condition" of this patch, i will update the patch
> to narrow down for the other compatibles.
> 
>>
>> Anyway, why do you add entries in the middle? LDO was the second, so it
>> cannot be now fifth.
> 
> As Rob's suggestion, i move the cmn_blk to second location for
> simplifying the limitation description, i checked the upstream dts code,
> the LDO is not used currently, so we can move cmn_blk to the second
> location here.

I cannot find his suggestion in the previous thread. Where did he
propose it?

...

>>> +  qcom,cmn-ref-clock-frequency:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum:
>>> +      - 25000000
>>> +      - 31250000
>>> +      - 40000000
>>> +      - 48000000
>>> +      - 50000000
>>> +      - 96000000
>>> +    default: 48000000
>>> +    description: |
>>> +      The reference clock source of CMN PLL block is selectable, the
>>> +      reference clock source can be from wifi module or the external
>>> +      xtal, the reference clock frequency 48MHZ can be from internal
>>> +      wifi or the external xtal, if absent, the internal 48MHZ is used,
>>> +      if the 48MHZ is specified, which means the external 48Mhz is used.
>>
>> This does not resolve mine and Conor's concerns from previous version.
>> External clocks are defined as clock inputs.
> 
> No matter the external or internal reference clock, they are the clock
> source selection for CMN, there are only 48MHZ can be external or 
> internal, other clocks have the different clock rate, so the internal
> 48MHZ reference clock can be implied when the 
> "qcom,cmn-ref-clock-frequency" is not defined, which is suggested by 
> Conor in the previous
> comments.

I don't think he proposed it, but maybe I missed some message (care to
point me to his message where he agreed on usage of
qcom,cmn-ref-clock-frequency?). I am pretty sure we both stayed on the
same page, that the presence of clocks defines choice of internal clock.
This property should go away.

It is tiring to keep discussing this.

> 
>>
>>> +
>>> +  clock-frequency:
>>> +    enum:
>>> +      - 390625
>>> +      - 781250
>>> +      - 1562500
>>> +      - 3125000
>>> +      - 6250000
>>> +      - 12500000
>>> +    default: 390625
>>> +    description: |
>>> +      The MDIO bus clock that must be output by the MDIO bus hardware,
>>> +      only the listed frequencies above can be supported, other frequency
>>> +      will cause malfunction. If absent, the default hardware value 0xff
>>> +      is used, which means the default MDIO clock frequency 390625HZ, The
>>> +      MDIO clock frequency is MDIO_SYS_CLK/(MDIO_CLK_DIV + 1), the SoC
>>> +      MDIO_SYS_CLK is fixed to 100MHZ, the MDIO_CLK_DIV is from MDIO control
>>> +      register, there is higher clock frequency requirement on the normal
>>> +      working case where the MDIO slave devices support high clock frequency.
>>>   
>>>   required:
>>>     - compatible
>>> @@ -59,8 +118,10 @@ allOf:
>>>             contains:
>>>               enum:
>>>                 - qcom,ipq5018-mdio
>>> +              - qcom,ipq5332-mdio
>>>                 - qcom,ipq6018-mdio
>>>                 - qcom,ipq8074-mdio
>>> +              - qcom,ipq9574-mdio
>>>       then:
>>>         required:
>>>           - clocks
>>> @@ -70,6 +131,20 @@ allOf:
>>>           clocks: false
>>>           clock-names: false
>>>   
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,ipq5332-mdio
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          minItems: 5
>>> +          maxItems: 5
>>> +        reg-names:
>>> +          minItems: 4
>>
>> Why all other variants now have 5 clocks and 5 reg entries? Nothing of
>> it is explained in the commit msg.
> 
>  From the condition above, only "qcom,ipq5332-mdio" has 5 clocks (mdio +
> 4 uniphy clocks) and 4 regs (mdio + cmn_blk + 2 LDOs) as the cmn_blk is
> moved to the second location.
> 
> how it can gives the 5 clocks and 5 regs for other variants here?

How? Just read the beginning of your patch. It clearly says everyone has
up to 5 reg entries and up to 5 clocks.



Best regards,
Krzysztof

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]

On Tue, Dec 26, 2023 at 10:28:09AM +0100, Krzysztof Kozlowski wrote:
> On 26/12/2023 08:25, Jie Luo wrote:

> >>> +  qcom,cmn-ref-clock-frequency:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    enum:
> >>> +      - 25000000
> >>> +      - 31250000
> >>> +      - 40000000
> >>> +      - 48000000
> >>> +      - 50000000
> >>> +      - 96000000
> >>> +    default: 48000000
> >>> +    description: |
> >>> +      The reference clock source of CMN PLL block is selectable, the
> >>> +      reference clock source can be from wifi module or the external
> >>> +      xtal, the reference clock frequency 48MHZ can be from internal
> >>> +      wifi or the external xtal, if absent, the internal 48MHZ is used,
> >>> +      if the 48MHZ is specified, which means the external 48Mhz is used.
> >>
> >> This does not resolve mine and Conor's concerns from previous version.
> >> External clocks are defined as clock inputs.
> > 
> > No matter the external or internal reference clock, they are the clock
> > source selection for CMN, there are only 48MHZ can be external or 
> > internal, other clocks have the different clock rate, so the internal
> > 48MHZ reference clock can be implied when the 
> > "qcom,cmn-ref-clock-frequency" is not defined, which is suggested by 
> > Conor in the previous
> > comments.
> 
> I don't think he proposed it, but maybe I missed some message (care to
> point me to his message where he agreed on usage of
> qcom,cmn-ref-clock-frequency?). I am pretty sure we both stayed on the
> same page, that the presence of clocks defines choice of internal clock.
> This property should go away.

Exactly, I wanted this property to be removed. My suggestion was about
defaulting to the internal clock when the "clocks" property did not
contain the cmn ref clock.

> It is tiring to keep discussing this.

Yup.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Jie Luo]

On 12/26/2023 8:21 PM, Conor Dooley wrote:
> On Tue, Dec 26, 2023 at 10:28:09AM +0100, Krzysztof Kozlowski wrote:
>> On 26/12/2023 08:25, Jie Luo wrote:
> 
>>>>> +  qcom,cmn-ref-clock-frequency:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    enum:
>>>>> +      - 25000000
>>>>> +      - 31250000
>>>>> +      - 40000000
>>>>> +      - 48000000
>>>>> +      - 50000000
>>>>> +      - 96000000
>>>>> +    default: 48000000
>>>>> +    description: |
>>>>> +      The reference clock source of CMN PLL block is selectable, the
>>>>> +      reference clock source can be from wifi module or the external
>>>>> +      xtal, the reference clock frequency 48MHZ can be from internal
>>>>> +      wifi or the external xtal, if absent, the internal 48MHZ is used,
>>>>> +      if the 48MHZ is specified, which means the external 48Mhz is used.
>>>>
>>>> This does not resolve mine and Conor's concerns from previous version.
>>>> External clocks are defined as clock inputs.
>>>
>>> No matter the external or internal reference clock, they are the clock
>>> source selection for CMN, there are only 48MHZ can be external or
>>> internal, other clocks have the different clock rate, so the internal
>>> 48MHZ reference clock can be implied when the
>>> "qcom,cmn-ref-clock-frequency" is not defined, which is suggested by
>>> Conor in the previous
>>> comments.
>>
>> I don't think he proposed it, but maybe I missed some message (care to
>> point me to his message where he agreed on usage of
>> qcom,cmn-ref-clock-frequency?). I am pretty sure we both stayed on the
>> same page, that the presence of clocks defines choice of internal clock.
>> This property should go away.
> 
> Exactly, I wanted this property to be removed. My suggestion was about
> defaulting to the internal clock when the "clocks" property did not
> contain the cmn ref clock.

There are two internal reference clock sources 48MHZ and 96MHZ.
The 96MHZ is used on ipq5018 currently as i said in the previous
message, but it is also possible to used on ipq9574 per double checked,
since the possible reference clock source should be kept as configurable
and the clock source should not be limited on the specific IPQ platform,
since the clock source is configurable, the different clock source maybe
required by the different board design.

As description above, the reference clock source has the different clock
rate except the 48MHZ, so it can imply that the internal 48MHZ used when
the property is absent.

> 
>> It is tiring to keep discussing this.
> 
> Yup.
> 

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Krzysztof Kozlowski]

On 26/12/2023 14:14, Jie Luo wrote:
> 
>>>>>
>>>>> This does not resolve mine and Conor's concerns from previous version.
>>>>> External clocks are defined as clock inputs.
>>>>
>>>> No matter the external or internal reference clock, they are the clock
>>>> source selection for CMN, there are only 48MHZ can be external or
>>>> internal, other clocks have the different clock rate, so the internal
>>>> 48MHZ reference clock can be implied when the
>>>> "qcom,cmn-ref-clock-frequency" is not defined, which is suggested by
>>>> Conor in the previous
>>>> comments.
>>>
>>> I don't think he proposed it, but maybe I missed some message (care to
>>> point me to his message where he agreed on usage of
>>> qcom,cmn-ref-clock-frequency?). I am pretty sure we both stayed on the
>>> same page, that the presence of clocks defines choice of internal clock.
>>> This property should go away.
>>
>> Exactly, I wanted this property to be removed. My suggestion was about
>> defaulting to the internal clock when the "clocks" property did not
>> contain the cmn ref clock.
> 
> There are two internal reference clock sources 48MHZ and 96MHZ.

On which devices? Paste entire picture, not half-baked descriptions.

> The 96MHZ is used on ipq5018 currently as i said in the previous
> message, but it is also possible to used on ipq9574 per double checked,
> since the possible reference clock source should be kept as configurable
> and the clock source should not be limited on the specific IPQ platform,
> since the clock source is configurable, the different clock source maybe
> required by the different board design.

I don't see how this answers anything about our suggestions.

Best regards,
Krzysztof

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Jie Luo]

On 12/26/2023 9:19 PM, Krzysztof Kozlowski wrote:
> On 26/12/2023 14:14, Jie Luo wrote:
>>
>>>>>>
>>>>>> This does not resolve mine and Conor's concerns from previous version.
>>>>>> External clocks are defined as clock inputs.
>>>>>
>>>>> No matter the external or internal reference clock, they are the clock
>>>>> source selection for CMN, there are only 48MHZ can be external or
>>>>> internal, other clocks have the different clock rate, so the internal
>>>>> 48MHZ reference clock can be implied when the
>>>>> "qcom,cmn-ref-clock-frequency" is not defined, which is suggested by
>>>>> Conor in the previous
>>>>> comments.
>>>>
>>>> I don't think he proposed it, but maybe I missed some message (care to
>>>> point me to his message where he agreed on usage of
>>>> qcom,cmn-ref-clock-frequency?). I am pretty sure we both stayed on the
>>>> same page, that the presence of clocks defines choice of internal clock.
>>>> This property should go away.
>>>
>>> Exactly, I wanted this property to be removed. My suggestion was about
>>> defaulting to the internal clock when the "clocks" property did not
>>> contain the cmn ref clock.
>>
>> There are two internal reference clock sources 48MHZ and 96MHZ.
> 
> On which devices? Paste entire picture, not half-baked descriptions.

On IPQ9574, the default reference clock source is internal 48MHZ, but
the internal 96MHZ is also configurable on IPQ9574.

If it imply the internal reference clock used by the combination of
the absent of the property and the compatible string, which can't
distinguish the internal 48MHZ and internal 96MHZ on ipq9574.

Since all the reference clock sources have the different clock rate
except for 48MHZ that has internal and external 48MHZ, we just needs
to imply the internal 48MHZ used when the property is absent, other
clock sources are decided by the frequency value of the property.

> 
>> The 96MHZ is used on ipq5018 currently as i said in the previous
>> message, but it is also possible to used on ipq9574 per double checked,
>> since the possible reference clock source should be kept as configurable
>> and the clock source should not be limited on the specific IPQ platform,
>> since the clock source is configurable, the different clock source maybe
>> required by the different board design.
> 
> I don't see how this answers anything about our suggestions.

The reference clock source can be distinguished by the frequency value
as described above.

Without limiting the reference clock source with the compatible string,
the reference clock sources can be kept configurable on IPQ platform,
which makes the MDIO driver code extensible.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Jie Luo]

On 12/26/2023 5:28 PM, Krzysztof Kozlowski wrote:
> On 26/12/2023 08:25, Jie Luo wrote:
>>>> -    description:
>>>> -      the first Address and length of the register set for the MDIO controller.
>>>> -      the second Address and length of the register for ethernet LDO, this second
>>>> -      address range is only required by the platform IPQ50xx.
>>>> +    maxItems: 5
>>>> +    description: |
>>>> +      The first address and length of the register set for the MDIO controller,
>>>> +      the optional second address and length of the register is for CMN block,
>>>> +      the optional third, fourth and fifth address and length of the register
>>>> +      for Ethernet LDO, the optional Ethernet LDO address range is required by
>>>
>>> Wait, required? You said in in response to Rob these are not required!
>>
>> As for the response to Rob, i was saying the uniphy ahb and sys clocks
>> are not needed on ipq9574.
>> The LDO are needed on ipq5332 and ipq5018 currently.
> 
> Clocks as well but:
> 
> "A driver can function without knowing about all these new registers and
> ..."

This comments are for compatible string in V2, the MDIO drive configures
the hardware according to the DTS property defined or not for the new
added IPQ platforms(ipq5332 and ipq9574) support.

> 
> Anyway, this should be list ("items:") with descriptions, instead of one
> big description listing things.

Ok, will update to use the list descriptions, thanks.

> 
> 
>>
>>>
>>>> +      the platform IPQ50xx/IPQ5332.
>>>
>>> So these are valid for all platforms or not? Looks not, but nothing
>>> narrows the list for other boards.
>>
>> i add the limitation on the reg usage for the ipq5332 platform on the
>> following part "if condition" of this patch, i will update the patch
>> to narrow down for the other compatibles.
>>
>>>
>>> Anyway, why do you add entries in the middle? LDO was the second, so it
>>> cannot be now fifth.
>>
>> As Rob's suggestion, i move the cmn_blk to second location for
>> simplifying the limitation description, i checked the upstream dts code,
>> the LDO is not used currently, so we can move cmn_blk to the second
>> location here.
> 
> I cannot find his suggestion in the previous thread. Where did he
> propose it?

Rob suggested this on the V2 as below.
"
Perhaps cmn_blk should come 2nd, so all the variants have the same entry
indices. Then you can move this to the top level and just say 'minItems:
4' here.
"

> 
> ...
> 
>>>> +  qcom,cmn-ref-clock-frequency:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum:
>>>> +      - 25000000
>>>> +      - 31250000
>>>> +      - 40000000
>>>> +      - 48000000
>>>> +      - 50000000
>>>> +      - 96000000
>>>> +    default: 48000000
>>>> +    description: |
>>>> +      The reference clock source of CMN PLL block is selectable, the
>>>> +      reference clock source can be from wifi module or the external
>>>> +      xtal, the reference clock frequency 48MHZ can be from internal
>>>> +      wifi or the external xtal, if absent, the internal 48MHZ is used,
>>>> +      if the 48MHZ is specified, which means the external 48Mhz is used.
>>>
>>> This does not resolve mine and Conor's concerns from previous version.
>>> External clocks are defined as clock inputs.
>>
>> No matter the external or internal reference clock, they are the clock
>> source selection for CMN, there are only 48MHZ can be external or
>> internal, other clocks have the different clock rate, so the internal
>> 48MHZ reference clock can be implied when the
>> "qcom,cmn-ref-clock-frequency" is not defined, which is suggested by
>> Conor in the previous
>> comments.
> 
> I don't think he proposed it, but maybe I missed some message (care to
> point me to his message where he agreed on usage of
> qcom,cmn-ref-clock-frequency?). I am pretty sure we both stayed on the
> same page, that the presence of clocks defines choice of internal clock.
> This property should go away.

Sorry for this confusion.
Rob said the internal reference source can be decided by the absence of
the property combined with compatible string, because i said the
internal 96MHZ is used on ipq5018 currently in the previous message.

per double checked the current IPQ platforms, the internal 96MHZ is also
possible on ipq9574, and the reference clock source should be kept as
configurable instead of limited by the compatible string, maybe the
different reference clock source is acquired in the future, even
currently it is not used on the special platform for now.

so i update the solution with a little bit of changes.

> 
> It is tiring to keep discussing this.
> 
>>
>>>
>>>> +
>>>> +  clock-frequency:
>>>> +    enum:
>>>> +      - 390625
>>>> +      - 781250
>>>> +      - 1562500
>>>> +      - 3125000
>>>> +      - 6250000
>>>> +      - 12500000
>>>> +    default: 390625
>>>> +    description: |
>>>> +      The MDIO bus clock that must be output by the MDIO bus hardware,
>>>> +      only the listed frequencies above can be supported, other frequency
>>>> +      will cause malfunction. If absent, the default hardware value 0xff
>>>> +      is used, which means the default MDIO clock frequency 390625HZ, The
>>>> +      MDIO clock frequency is MDIO_SYS_CLK/(MDIO_CLK_DIV + 1), the SoC
>>>> +      MDIO_SYS_CLK is fixed to 100MHZ, the MDIO_CLK_DIV is from MDIO control
>>>> +      register, there is higher clock frequency requirement on the normal
>>>> +      working case where the MDIO slave devices support high clock frequency.
>>>>    
>>>>    required:
>>>>      - compatible
>>>> @@ -59,8 +118,10 @@ allOf:
>>>>              contains:
>>>>                enum:
>>>>                  - qcom,ipq5018-mdio
>>>> +              - qcom,ipq5332-mdio
>>>>                  - qcom,ipq6018-mdio
>>>>                  - qcom,ipq8074-mdio
>>>> +              - qcom,ipq9574-mdio
>>>>        then:
>>>>          required:
>>>>            - clocks
>>>> @@ -70,6 +131,20 @@ allOf:
>>>>            clocks: false
>>>>            clock-names: false
>>>>    
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - qcom,ipq5332-mdio
>>>> +    then:
>>>> +      properties:
>>>> +        clocks:
>>>> +          minItems: 5
>>>> +          maxItems: 5
>>>> +        reg-names:
>>>> +          minItems: 4
>>>
>>> Why all other variants now have 5 clocks and 5 reg entries? Nothing of
>>> it is explained in the commit msg.
>>
>>   From the condition above, only "qcom,ipq5332-mdio" has 5 clocks (mdio +
>> 4 uniphy clocks) and 4 regs (mdio + cmn_blk + 2 LDOs) as the cmn_blk is
>> moved to the second location.
>>
>> how it can gives the 5 clocks and 5 regs for other variants here?
> 
> How? Just read the beginning of your patch. It clearly says everyone has
> up to 5 reg entries and up to 5 clocks.

Sorry for missing the limitation of the new added regs and clocks for
other platforms, will update the patch to add the limitation usage of
the reg and clocks on the other platform.

Thanks!
> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Krzysztof Kozlowski]

On 26/12/2023 14:06, Jie Luo wrote:
> 
>>
>>
>>>
>>>>
>>>>> +      the platform IPQ50xx/IPQ5332.
>>>>
>>>> So these are valid for all platforms or not? Looks not, but nothing
>>>> narrows the list for other boards.
>>>
>>> i add the limitation on the reg usage for the ipq5332 platform on the
>>> following part "if condition" of this patch, i will update the patch
>>> to narrow down for the other compatibles.
>>>
>>>>
>>>> Anyway, why do you add entries in the middle? LDO was the second, so it
>>>> cannot be now fifth.
>>>
>>> As Rob's suggestion, i move the cmn_blk to second location for
>>> simplifying the limitation description, i checked the upstream dts code,
>>> the LDO is not used currently, so we can move cmn_blk to the second
>>> location here.
>>
>> I cannot find his suggestion in the previous thread. Where did he
>> propose it?
> 
> Rob suggested this on the V2 as below.
> "
> Perhaps cmn_blk should come 2nd, so all the variants have the same entry
> indices. Then you can move this to the top level and just say 'minItems:
> 4' here.

Wasn't this for new devices? What about all existing which have LDO as
the second entry?

>>>>> +  qcom,cmn-ref-clock-frequency:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    enum:
>>>>> +      - 25000000
>>>>> +      - 31250000
>>>>> +      - 40000000
>>>>> +      - 48000000
>>>>> +      - 50000000
>>>>> +      - 96000000
>>>>> +    default: 48000000
>>>>> +    description: |
>>>>> +      The reference clock source of CMN PLL block is selectable, the
>>>>> +      reference clock source can be from wifi module or the external
>>>>> +      xtal, the reference clock frequency 48MHZ can be from internal
>>>>> +      wifi or the external xtal, if absent, the internal 48MHZ is used,
>>>>> +      if the 48MHZ is specified, which means the external 48Mhz is used.
>>>>
>>>> This does not resolve mine and Conor's concerns from previous version.
>>>> External clocks are defined as clock inputs.
>>>
>>> No matter the external or internal reference clock, they are the clock
>>> source selection for CMN, there are only 48MHZ can be external or
>>> internal, other clocks have the different clock rate, so the internal
>>> 48MHZ reference clock can be implied when the
>>> "qcom,cmn-ref-clock-frequency" is not defined, which is suggested by
>>> Conor in the previous
>>> comments.
>>
>> I don't think he proposed it, but maybe I missed some message (care to
>> point me to his message where he agreed on usage of
>> qcom,cmn-ref-clock-frequency?). I am pretty sure we both stayed on the
>> same page, that the presence of clocks defines choice of internal clock.
>> This property should go away.
> 
> Sorry for this confusion.
> Rob said the internal reference source can be decided by the absence of
> the property combined with compatible string, because i said the

So all your three DT maintainers agree that lack of property for
choosing clock, defines the usage of interrupt source.

Now we had huge amount of arguments that you do not represent properly
the clock relationships. Still.

> internal 96MHZ is used on ipq5018 currently in the previous message.
> 
> per double checked the current IPQ platforms, the internal 96MHZ is also
> possible on ipq9574, and the reference clock source should be kept as
> configurable instead of limited by the compatible string, maybe the
> different reference clock source is acquired in the future, even
> currently it is not used on the special platform for now.
> 
> so i update the solution with a little bit of changes.

You still do not want to implement our suggestions and I don't
understand your arguments. Nothing in above paragraph explains me why
you cannot use clock provider/consumer relationships.


Best regards,
Krzysztof

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Jie Luo]

On 12/26/2023 9:18 PM, Krzysztof Kozlowski wrote:
> On 26/12/2023 14:06, Jie Luo wrote:
>>
>>>
>>>
>>>>
>>>>>
>>>>>> +      the platform IPQ50xx/IPQ5332.
>>>>>
>>>>> So these are valid for all platforms or not? Looks not, but nothing
>>>>> narrows the list for other boards.
>>>>
>>>> i add the limitation on the reg usage for the ipq5332 platform on the
>>>> following part "if condition" of this patch, i will update the patch
>>>> to narrow down for the other compatibles.
>>>>
>>>>>
>>>>> Anyway, why do you add entries in the middle? LDO was the second, so it
>>>>> cannot be now fifth.
>>>>
>>>> As Rob's suggestion, i move the cmn_blk to second location for
>>>> simplifying the limitation description, i checked the upstream dts code,
>>>> the LDO is not used currently, so we can move cmn_blk to the second
>>>> location here.
>>>
>>> I cannot find his suggestion in the previous thread. Where did he
>>> propose it?
>>
>> Rob suggested this on the V2 as below.
>> "
>> Perhaps cmn_blk should come 2nd, so all the variants have the same entry
>> indices. Then you can move this to the top level and just say 'minItems:
>> 4' here.
> 
> Wasn't this for new devices? What about all existing which have LDO as
> the second entry?

In the current regs, which includes mdio and one LDO.
mdio is the reg address range for all IPQs.
one LDO is only for ipq5018.

the new added LDO is for ipq5332.
the cmn_blk is for ipq9574 and ipq5332.

i will update these limitations of dt-bindings in the next patch set.

> 
>>>>>> +  qcom,cmn-ref-clock-frequency:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    enum:
>>>>>> +      - 25000000
>>>>>> +      - 31250000
>>>>>> +      - 40000000
>>>>>> +      - 48000000
>>>>>> +      - 50000000
>>>>>> +      - 96000000
>>>>>> +    default: 48000000
>>>>>> +    description: |
>>>>>> +      The reference clock source of CMN PLL block is selectable, the
>>>>>> +      reference clock source can be from wifi module or the external
>>>>>> +      xtal, the reference clock frequency 48MHZ can be from internal
>>>>>> +      wifi or the external xtal, if absent, the internal 48MHZ is used,
>>>>>> +      if the 48MHZ is specified, which means the external 48Mhz is used.
>>>>>
>>>>> This does not resolve mine and Conor's concerns from previous version.
>>>>> External clocks are defined as clock inputs.
>>>>
>>>> No matter the external or internal reference clock, they are the clock
>>>> source selection for CMN, there are only 48MHZ can be external or
>>>> internal, other clocks have the different clock rate, so the internal
>>>> 48MHZ reference clock can be implied when the
>>>> "qcom,cmn-ref-clock-frequency" is not defined, which is suggested by
>>>> Conor in the previous
>>>> comments.
>>>
>>> I don't think he proposed it, but maybe I missed some message (care to
>>> point me to his message where he agreed on usage of
>>> qcom,cmn-ref-clock-frequency?). I am pretty sure we both stayed on the
>>> same page, that the presence of clocks defines choice of internal clock.
>>> This property should go away.
>>
>> Sorry for this confusion.
>> Rob said the internal reference source can be decided by the absence of
>> the property combined with compatible string, because i said the
> 
> So all your three DT maintainers agree that lack of property for
> choosing clock, defines the usage of interrupt source.

This is the reference clock source selection of CMN block, which
generates the clocks for the Ethernet devices.

> 
> Now we had huge amount of arguments that you do not represent properly
> the clock relationships. Still.

here is the clock topology.
reference clock sources ---> CMN PLL ---> various output clocks

the output clocks are provided to the Ethernet devices(such as the
qca808x PHY devices).

These information is also provided the commit message of the patch
<net: mdio: ipq4019: configure CMN PLL clock for ipq5332>.

> 
>> internal 96MHZ is used on ipq5018 currently in the previous message.
>>
>> per double checked the current IPQ platforms, the internal 96MHZ is also
>> possible on ipq9574, and the reference clock source should be kept as
>> configurable instead of limited by the compatible string, maybe the
>> different reference clock source is acquired in the future, even
>> currently it is not used on the special platform for now.
>>
>> so i update the solution with a little bit of changes.
> 
> You still do not want to implement our suggestions and I don't
> understand your arguments. Nothing in above paragraph explains me why
> you cannot use clock provider/consumer relationships.

Hi Krzysztof,

The reference clock source can be registered as the fix clock provider,
 From the current fix clock provider, the clock rate is useful for the
clock consumer, the fix clock rate is used to generate the output clocks
by the divider or multiplier.

For the CMN block to select reference clock, which is configuring the
clock source, we don't know the formula to get the output clock value
based on the reference clock value.

i also see there is an example in the upstream code, which is same as
the CMN block to select the reference clock source.

the property "ref-clock-frequency" is defined in the yaml file below.
Documentation/devicetree/bindings/net/wireless/ti,wlcore.yaml.
"
ref-clock-frequency: 

     $ref: /schemas/types.yaml#/definitions/uint32 

     description: Reference clock frequency.
"

the reference clock source is selected based on the table as below.
source code file drivers/net/wireless/ti/wl12xx/main.c
"
static const struct wl12xx_clock wl12xx_refclock_table[] = { 

         { 19200000,     false,  WL12XX_REFCLOCK_19      }, 

         { 26000000,     false,  WL12XX_REFCLOCK_26      }, 

         { 26000000,     true,   WL12XX_REFCLOCK_26_XTAL }, 

         { 38400000,     false,  WL12XX_REFCLOCK_38      }, 

         { 38400000,     true,   WL12XX_REFCLOCK_38_XTAL }, 

         { 52000000,     false,  WL12XX_REFCLOCK_52      }, 

         { 0,            false,  0 } 

};
"
Thanks.
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Krzysztof Kozlowski]

On 28/12/2023 08:38, Jie Luo wrote:
>>> Sorry for this confusion.
>>> Rob said the internal reference source can be decided by the absence of
>>> the property combined with compatible string, because i said the
>>
>> So all your three DT maintainers agree that lack of property for
>> choosing clock, defines the usage of interrupt source.
> 
> This is the reference clock source selection of CMN block, which
> generates the clocks for the Ethernet devices.
> 
>>
>> Now we had huge amount of arguments that you do not represent properly
>> the clock relationships. Still.
> 
> here is the clock topology.
> reference clock sources ---> CMN PLL ---> various output clocks

How do you guarantee that these clocks are enabled without proper
relationships described in DT? In current and future designs?

> 
> the output clocks are provided to the Ethernet devices(such as the
> qca808x PHY devices).
> 
> These information is also provided the commit message of the patch
> <net: mdio: ipq4019: configure CMN PLL clock for ipq5332>.
> 
>>
>>> internal 96MHZ is used on ipq5018 currently in the previous message.
>>>
>>> per double checked the current IPQ platforms, the internal 96MHZ is also
>>> possible on ipq9574, and the reference clock source should be kept as
>>> configurable instead of limited by the compatible string, maybe the
>>> different reference clock source is acquired in the future, even
>>> currently it is not used on the special platform for now.
>>>
>>> so i update the solution with a little bit of changes.
>>
>> You still do not want to implement our suggestions and I don't
>> understand your arguments. Nothing in above paragraph explains me why
>> you cannot use clock provider/consumer relationships.
> 
> Hi Krzysztof,
> 
> The reference clock source can be registered as the fix clock provider,
>  From the current fix clock provider, the clock rate is useful for the
> clock consumer, the fix clock rate is used to generate the output clocks
> by the divider or multiplier.
> 
> For the CMN block to select reference clock, which is configuring the
> clock source, we don't know the formula to get the output clock value
> based on the reference clock value.

I don't understand what does it mean. You do not know how to program CMN
block?

> 
> i also see there is an example in the upstream code, which is same as
> the CMN block to select the reference clock source.

Oh, the old argument. So if there is a bug in the code, you are going
for example to implement it as well?

> 
> the property "ref-clock-frequency" is defined in the yaml file below.
> Documentation/devicetree/bindings/net/wireless/ti,wlcore.yaml.

And how does the hardware look like there? It's TI, so how do you even know?



Best regards,
Krzysztof

Re: [PATCH v4 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Jie Luo]

On 1/4/2024 3:47 PM, Krzysztof Kozlowski wrote:
> On 28/12/2023 08:38, Jie Luo wrote:
>>>> Sorry for this confusion.
>>>> Rob said the internal reference source can be decided by the absence of
>>>> the property combined with compatible string, because i said the
>>>
>>> So all your three DT maintainers agree that lack of property for
>>> choosing clock, defines the usage of interrupt source.
>>
>> This is the reference clock source selection of CMN block, which
>> generates the clocks for the Ethernet devices.
>>
>>>
>>> Now we had huge amount of arguments that you do not represent properly
>>> the clock relationships. Still.
>>
>> here is the clock topology.
>> reference clock sources ---> CMN PLL ---> various output clocks
> 
> How do you guarantee that these clocks are enabled without proper
> relationships described in DT? In current and future designs?

Will update the patch to add the clock relationship in the DT, thanks.

> 
>>
>> the output clocks are provided to the Ethernet devices(such as the
>> qca808x PHY devices).
>>
>> These information is also provided the commit message of the patch
>> <net: mdio: ipq4019: configure CMN PLL clock for ipq5332>.
>>
>>>
>>>> internal 96MHZ is used on ipq5018 currently in the previous message.
>>>>
>>>> per double checked the current IPQ platforms, the internal 96MHZ is also
>>>> possible on ipq9574, and the reference clock source should be kept as
>>>> configurable instead of limited by the compatible string, maybe the
>>>> different reference clock source is acquired in the future, even
>>>> currently it is not used on the special platform for now.
>>>>
>>>> so i update the solution with a little bit of changes.
>>>
>>> You still do not want to implement our suggestions and I don't
>>> understand your arguments. Nothing in above paragraph explains me why
>>> you cannot use clock provider/consumer relationships.
>>
>> Hi Krzysztof,
>>
>> The reference clock source can be registered as the fix clock provider,
>>   From the current fix clock provider, the clock rate is useful for the
>> clock consumer, the fix clock rate is used to generate the output clocks
>> by the divider or multiplier.
>>
>> For the CMN block to select reference clock, which is configuring the
>> clock source, we don't know the formula to get the output clock value
>> based on the reference clock value.
> 
> I don't understand what does it mean. You do not know how to program CMN
> block?

The output clock value of CMN block is not related to the clock value of
the reference source clock, the output clocks of CMN block are fixed to
25M and 50M, which are provided to the different Ethernet devices, there
is no formula for the relationship of input clock value and output clock
value of CMN block.

> 
>>
>> i also see there is an example in the upstream code, which is same as
>> the CMN block to select the reference clock source.
> 
> Oh, the old argument. So if there is a bug in the code, you are going
> for example to implement it as well?

The reference source clock can be registered as fix clock, and we can
get the clock rate of reference source clock with the external or
internal flag to distinguish the reference clock source, then program
the CMN block corresponding.
> 
>>
>> the property "ref-clock-frequency" is defined in the yaml file below.
>> Documentation/devicetree/bindings/net/wireless/ti,wlcore.yaml.
> 
> And how does the hardware look like there? It's TI, so how do you even know?

According to the driver code and DT description, the example is also for
selecting the reference clock source according to the clock value of DT
properties.

Sure, we can also use the registered fix clock to identify the
reference clock source for CMN block.

> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v4 0/5] support ipq5332 platform

[Sergey Ryazanov]

Hi Luo,

I have a few questions regarding the high level design of 
implementation. I hope that clarifying these topics will help us to find 
a good model for the case and finally merge a supporting code. Please 
find the questions below.

On 25.12.2023 10:44, Luo Jie wrote:
> For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
> connected with one of them.
> 
> 1. The Ethernet LDO needs to be enabled to make the PHY GPIO
>     reset taking effect, which uses the MDIO bus level reset.
> 
> 2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
>     to make the ethernet PHY device accessible.
> 
> 3. To provide the clock to the ethernet, the CMN clock needs
>     to be initialized for selecting reference clock and enabling
>     the output clock.
> 
> 4. Support optional MDIO clock frequency config.
> 
> 5. Update dt-bindings doc for the new added properties.
> 
> Changes in v2:
> 	* remove the PHY related features such as PHY address
> 	  program and clock initialization.
> 	* leverage the MDIO level GPIO reset for qca8084 PHY.
> 
> Changes in v3:
> 	* fix the christmas-tree format issue.
> 	* improve the dt-binding changes.
> 
> Changes in v4:
> 	* improve the CMN PLL reference clock config.
> 	* improve the dt-binding changes.
> 
> Luo Jie (5):
>    net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
>    net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform
>    net: mdio: ipq4019: configure CMN PLL clock for ipq5332
>    net: mdio: ipq4019: support MDIO clock frequency divider
>    dt-bindings: net: ipq4019-mdio: Document ipq5332 platform
> 
>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
>   drivers/net/mdio/mdio-ipq4019.c               | 288 ++++++++++++++++--
>   2 files changed, 399 insertions(+), 30 deletions(-)

I'm asking these questions because after checking the patches and 
following the earlier discussion, the series is looks like an 
overloading of the MDIO driver with somehow but not strictly related 
functionality.


First, let me summarize the case. Feel free to correct me if I took 
something wrong. So, we have:
- a reference design contains IPQ5332 SoC + QCA8084 switch/Phy;
- QCA8084 requires a reference clock for normal functionality;
- IPQ5332, as a chip, is able to provide a set of reference clocks for 
external devices;
- you want to configure IPQ5332 to provide the reference clock for QCA8084.


So, the high level questions are:
1. Is QCA8084 capable to consume the clock from some other generator? Is 
it possible to clock QCA8084 from external XO/PLL/whatever?
2. Is IPQ5332 capable to provide reference clock to another switch model?
3. Is the reference clock generation subsystem part of the MDIO block of 
IPQ5332?


And there are some tiny questions to make sure that we are on the same page:
a. What is the mentioned Ethernet LDO? AFAIK, LDO is some kind of gate 
(or switch) that enables clock output through an IPQ5332 pin. Isn't it? 
And if it's true, then can you clarify, what exactly clock is outputted?
b. Is the Ethernet LDO part of the MDIO block of IPQ5332? According to 
iomem addresses that was used in the example reg property, the Ethernet 
LDO is not part of MDIO.
c. Is the CMN PLL part of the MDIO block of IPQ5332? Again, according to 
iomem address, the CMN PLL is not part of MDIO.
d. Are GCC AHB & SYS clocks really consumed by MDIO itself? Or are they 
need for the external reference clock generation?


Please answer these questions one by one and we will have a good basis 
to move forward.



To speed up the discussion, let me share my user's view of the reference 
clocks modeling. I would like to join the option that has already been 
suggested by the maintainers. It is better to implement reference clocks 
handling using the clocks API, and the clock subsystem will take care of 
enabling and configuring them.

And considering the expected answers to the above questions, I would 
like to suggest to implement the clock handling using a dedicated clock 
controlling driver. Or even using several of such tiny dedicated 
drivers. So DTS will become like this:

   ext_ref_clock: ext_ref_clock {
     compatible = "fixed-clock";
     clock-frequency = <48000000>;
   };

   eth_cmn_pll: clock-controller@9b000 {
     compatible = "qcom,eth-cmn-pll-ipq5223";
     reg = <0x9b000 0x800>;
     clocks = <&ext_ref_clock>; /* use external 48MHz clock */
   };

   phy0_ext_clk: clock-controller@7a00610 {
     compatible = "qcom,ipq-eth-ldo";
     reg = <0x7a00610 0x4>;
     clocks = <&eth_cmn_pll>;
   };

   mdio@90000 {
     compatible = "qcom,ipq4019-mdio";
     reg = <0x90000 0x64>;
     clocks = <&gcc GCC_MDIO_AHB_CLK>;

     ethernet-phy@1 {
       compatible = "...";
       reg = <1>;
       clocks = <&phy0_ext_clk>;
       reset-gpios = <&gcc ...>;
     };
   };

--
Sergey

Re: [PATCH v4 0/5] support ipq5332 platform

[Jie Luo]

On 1/2/2024 7:01 AM, Sergey Ryazanov wrote:
> Hi Luo,
> 
> I have a few questions regarding the high level design of 
> implementation. I hope that clarifying these topics will help us to find 
> a good model for the case and finally merge a supporting code. Please 
> find the questions below.
> 
> On 25.12.2023 10:44, Luo Jie wrote:
>> For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
>> connected with one of them.
>>
>> 1. The Ethernet LDO needs to be enabled to make the PHY GPIO
>>     reset taking effect, which uses the MDIO bus level reset.
>>
>> 2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
>>     to make the ethernet PHY device accessible.
>>
>> 3. To provide the clock to the ethernet, the CMN clock needs
>>     to be initialized for selecting reference clock and enabling
>>     the output clock.
>>
>> 4. Support optional MDIO clock frequency config.
>>
>> 5. Update dt-bindings doc for the new added properties.
>>
>> Changes in v2:
>>     * remove the PHY related features such as PHY address
>>       program and clock initialization.
>>     * leverage the MDIO level GPIO reset for qca8084 PHY.
>>
>> Changes in v3:
>>     * fix the christmas-tree format issue.
>>     * improve the dt-binding changes.
>>
>> Changes in v4:
>>     * improve the CMN PLL reference clock config.
>>     * improve the dt-binding changes.
>>
>> Luo Jie (5):
>>    net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
>>    net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform
>>    net: mdio: ipq4019: configure CMN PLL clock for ipq5332
>>    net: mdio: ipq4019: support MDIO clock frequency divider
>>    dt-bindings: net: ipq4019-mdio: Document ipq5332 platform
>>
>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
>>   drivers/net/mdio/mdio-ipq4019.c               | 288 ++++++++++++++++--
>>   2 files changed, 399 insertions(+), 30 deletions(-)
> 
> I'm asking these questions because after checking the patches and 
> following the earlier discussion, the series is looks like an 
> overloading of the MDIO driver with somehow but not strictly related 
> functionality.
> 
> 
> First, let me summarize the case. Feel free to correct me if I took 
> something wrong. So, we have:
> - a reference design contains IPQ5332 SoC + QCA8084 switch/Phy;

IPQ5322 SoC is currently connected with qca8386(switch that includes
QCA8084 PHY), the pure PHY chip qca8084 is currently connected on the
SoC IPQ9574.

> - QCA8084 requires a reference clock for normal functionality;

The reference clock is selected for the CMN PLL block, which outputs
the clocks to the Ethernet devices including the qca8084 PHY for normal
functionality, also for other connected Ethernet devices, the CMN PLL
block is located in SoC such as ipq5332 and ipq9574.

> - IPQ5332, as a chip, is able to provide a set of reference clocks for 
> external devices;

Yes, the CMN PLL block of IPQ5332 provides the output clocks as the
working clocks for the external Ethernet devices such as the QCA8386
(switch chip), the reference clocks we are discussing is as the
reference clock source of the CMN PLL block.

> - you want to configure IPQ5332 to provide the reference clock for QCA8084.

The reference clocks for CMN PLL block is configurable, and the output
clocks of CMN PLL are fixed, the output clocks are 50MHZ, which is given
to the external Ethernet devices.
here is the topology of clocks.
		     ---------
		    |	     |
reference clock --->| CMN PLL|--> output 50M clocks --> qca8084/qca8386
	            |        |
	            ---------
> 			   	
> 
> So, the high level questions are:
> 1. Is QCA8084 capable to consume the clock from some other generator? Is 
> it possible to clock QCA8084 from external XO/PLL/whatever?
No, the clock of qca8084/qca8386 is provided from the output clock of
CMN PLL as above.

> 2. Is IPQ5332 capable to provide reference clock to another switch model?

Yes, IPQ5332 can provide the reference clock to all connected Ethernet
devices, such as qca8386(switch), qca8081 phy and others.

> 3. Is the reference clock generation subsystem part of the MDIO block of 
> IPQ5332?

the reference clock of CMN PLL block can be from wifi and external xtal, 
the CMN PLL is integrated in the MDIO block, CMN PLL is the independent
block that generates the clocks for the connected Ethernet devices.

> 
> 
> And there are some tiny questions to make sure that we are on the same 
> page:
> a. What is the mentioned Ethernet LDO? AFAIK, LDO is some kind of gate 
> (or switch) that enables clock output through an IPQ5332 pin. Isn't it?

That's correct, the LDO is for enabling the output 50M clock of CMN PLL
to the connected Ethernet device, which is controlled by the hardware
register on the IPQ5332.

> And if it's true, then can you clarify, what exactly clock is outputted?

the 50M clock is outputted to the external Ethernet devices.

> b. Is the Ethernet LDO part of the MDIO block of IPQ5332? According to 
> iomem addresses that was used in the example reg property, the Ethernet 
> LDO is not part of MDIO.

LDO is not the part of MDIO block, LDO has the different register space
from MDIO, which is located in the independent Ethernet part.

> c. Is the CMN PLL part of the MDIO block of IPQ5332? Again, according to 
> iomem address, the CMN PLL is not part of MDIO.

No, CMN PLL is not the part of MDIO block, which is the independent
block, but it generates the clocks to the connected Ethernet devices
managed by MDIO bus, and the CMN PLL block needs to be configured
correctly to generate the clocks to make the MDIO devices(Ethernet
devices) working.

> d. Are GCC AHB & SYS clocks really consumed by MDIO itself? Or are they 
> need for the external reference clock generation?

GCC AHB & SYS clocks are consumed by the uniphy(PCS) that is connected
with the Ethernet devices, so we can say the GCC AHB & SYS clocks are
consumed by the Ethernet devices, which is not for the external
reference clock generation, external reference clock of CMN PLL are the
fix clock that are from wifi or external XO.

> 
> 
> Please answer these questions one by one and we will have a good basis 
> to move forward.
OK.
> 
> 
> 
> To speed up the discussion, let me share my user's view of the reference 
> clocks modeling. I would like to join the option that has already been 
> suggested by the maintainers. It is better to implement reference clocks 
> handling using the clocks API, and the clock subsystem will take care of 
> enabling and configuring them.
> 
> And considering the expected answers to the above questions, I would 
> like to suggest to implement the clock handling using a dedicated clock 
> controlling driver. Or even using several of such tiny dedicated 
> drivers. So DTS will become like this:
> 
>    ext_ref_clock: ext_ref_clock {
>      compatible = "fixed-clock";
>      clock-frequency = <48000000>;
>    };
> 
>    eth_cmn_pll: clock-controller@9b000 {
>      compatible = "qcom,eth-cmn-pll-ipq5223";
>      reg = <0x9b000 0x800>;
>      clocks = <&ext_ref_clock>; /* use external 48MHz clock */
>    };
> 
>    phy0_ext_clk: clock-controller@7a00610 {
>      compatible = "qcom,ipq-eth-ldo";
>      reg = <0x7a00610 0x4>;
>      clocks = <&eth_cmn_pll>;
>    };
> 
>    mdio@90000 {
>      compatible = "qcom,ipq4019-mdio";
>      reg = <0x90000 0x64>;
>      clocks = <&gcc GCC_MDIO_AHB_CLK>;
> 
>      ethernet-phy@1 {
>        compatible = "...";
>        reg = <1>;
>        clocks = <&phy0_ext_clk>;
>        reset-gpios = <&gcc ...>;
>      };
>    };
> 
> -- 
> Sergey

Thanks Sergey for the reference DTS.
Since the GPIO reset of qca8084/qca8386 is needed before configuring the
Ethernet device.

The configuration of and phy0_ext_clk(LDO) should be configured
firstly, which enables the clocks to the Ethernet devices, then the GPIO
reset of the connected Ethernet devices(such as qca8386) can take
effect, currently the GPIO reset takes the MDIO bus level reset.

So phy0_ext_clk can't be put in the PHY device tree node, one LDO
controls the clock output enabled to the connected Ethernet device such
as qca8386.

Re: [PATCH v4 0/5] support ipq5332 platform

[Sergey Ryazanov]

Hi Luo,

thank you for explaining the case in such details. I also have checked 
the related DTSs in the Linaro repository to be more familiar with the 
I/O mem layout. Specifically I checked these two, hope they are relevant 
to the discussion:
https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi

Please find my comments below.

On 03.01.2024 15:31, Jie Luo wrote:
> On 1/2/2024 7:01 AM, Sergey Ryazanov wrote:
>> Hi Luo,
>>
>> I have a few questions regarding the high level design of 
>> implementation. I hope that clarifying these topics will help us to 
>> find a good model for the case and finally merge a supporting code. 
>> Please find the questions below.
>>
>> On 25.12.2023 10:44, Luo Jie wrote:
>>> For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
>>> connected with one of them.
>>>
>>> 1. The Ethernet LDO needs to be enabled to make the PHY GPIO
>>>     reset taking effect, which uses the MDIO bus level reset.
>>>
>>> 2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
>>>     to make the ethernet PHY device accessible.
>>>
>>> 3. To provide the clock to the ethernet, the CMN clock needs
>>>     to be initialized for selecting reference clock and enabling
>>>     the output clock.
>>>
>>> 4. Support optional MDIO clock frequency config.
>>>
>>> 5. Update dt-bindings doc for the new added properties.
>>>
>>> Changes in v2:
>>>     * remove the PHY related features such as PHY address
>>>       program and clock initialization.
>>>     * leverage the MDIO level GPIO reset for qca8084 PHY.
>>>
>>> Changes in v3:
>>>     * fix the christmas-tree format issue.
>>>     * improve the dt-binding changes.
>>>
>>> Changes in v4:
>>>     * improve the CMN PLL reference clock config.
>>>     * improve the dt-binding changes.
>>>
>>> Luo Jie (5):
>>>    net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
>>>    net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform
>>>    net: mdio: ipq4019: configure CMN PLL clock for ipq5332
>>>    net: mdio: ipq4019: support MDIO clock frequency divider
>>>    dt-bindings: net: ipq4019-mdio: Document ipq5332 platform
>>>
>>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
>>>   drivers/net/mdio/mdio-ipq4019.c               | 288 ++++++++++++++++--
>>>   2 files changed, 399 insertions(+), 30 deletions(-)
>>
>> I'm asking these questions because after checking the patches and 
>> following the earlier discussion, the series is looks like an 
>> overloading of the MDIO driver with somehow but not strictly related 
>> functionality.
>>
>>
>> First, let me summarize the case. Feel free to correct me if I took 
>> something wrong. So, we have:
>> - a reference design contains IPQ5332 SoC + QCA8084 switch/Phy;
> 
> IPQ5322 SoC is currently connected with qca8386(switch that includes
> QCA8084 PHY), the pure PHY chip qca8084 is currently connected on the
> SoC IPQ9574.

As far as I understand these chips have standardized interfaces and 
QCA8386/QCA8084 can be reused with another SoC(s) in future. As well as 
IPQ5332 can be used with different phy. So now we are talking about some 
specific reference design. Isn't it?

>> - QCA8084 requires a reference clock for normal functionality;
> 
> The reference clock is selected for the CMN PLL block, which outputs
> the clocks to the Ethernet devices including the qca8084 PHY for normal
> functionality, also for other connected Ethernet devices, the CMN PLL
> block is located in SoC such as ipq5332 and ipq9574.
> 
>> - IPQ5332, as a chip, is able to provide a set of reference clocks for 
>> external devices;
> 
> Yes, the CMN PLL block of IPQ5332 provides the output clocks as the
> working clocks for the external Ethernet devices such as the QCA8386
> (switch chip), the reference clocks we are discussing is as the
> reference clock source of the CMN PLL block.

Ok, I feel we have some ambiguity regarding the reference clock term 
here. Sure, CMN PLL needs a reference clock to functioning. And in the 
same time, the output clock provided by CMN PLL is a reference clock for 
QCA8384.

So, when I was talking about IPQ5332, I meant the whole chip including 
CMN PLL block. So, I asked about CMN PLL output clock. But you already 
clarified the SoC capabilities below.

>> - you want to configure IPQ5332 to provide the reference clock for 
>> QCA8084.
> 
> The reference clocks for CMN PLL block is configurable, and the output
> clocks of CMN PLL are fixed, the output clocks are 50MHZ, which is given
> to the external Ethernet devices.
> here is the topology of clocks.
>                     ---------
>                     |        |
> reference clock --->| CMN PLL|--> output 50M clocks --> qca8084/qca8386
>                     |        |
>                     ---------
>>
>>
>> So, the high level questions are:
>> 1. Is QCA8084 capable to consume the clock from some other generator? 
>> Is it possible to clock QCA8084 from external XO/PLL/whatever?
> No, the clock of qca8084/qca8386 is provided from the output clock of
> CMN PLL as above.

Right, in case of pairing QCA8386 with IPQ5332, it is a good option to 
provide the clock from the SoC. But in general QCA8386 will be Ok with 
any 50 MHz clock. Right? I would like to say that thinking about this 
specific reference design being a single possible combination limits a 
scope of driver implementation options.

>> 2. Is IPQ5332 capable to provide reference clock to another switch model?
> 
> Yes, IPQ5332 can provide the reference clock to all connected Ethernet
> devices, such as qca8386(switch), qca8081 phy and others.

Ok. Thank you for clarifying this.

>> 3. Is the reference clock generation subsystem part of the MDIO block 
>> of IPQ5332?
> 
> the reference clock of CMN PLL block can be from wifi and external xtal, 
> the CMN PLL is integrated in the MDIO block, CMN PLL is the independent
> block that generates the clocks for the connected Ethernet devices.
> 
>>
>>
>> And there are some tiny questions to make sure that we are on the same 
>> page:
>> a. What is the mentioned Ethernet LDO? AFAIK, LDO is some kind of gate 
>> (or switch) that enables clock output through an IPQ5332 pin. Isn't it?
> 
> That's correct, the LDO is for enabling the output 50M clock of CMN PLL
> to the connected Ethernet device, which is controlled by the hardware
> register on the IPQ5332.
> 
>> And if it's true, then can you clarify, what exactly clock is outputted?
> 
> the 50M clock is outputted to the external Ethernet devices.
> 
>> b. Is the Ethernet LDO part of the MDIO block of IPQ5332? According to 
>> iomem addresses that was used in the example reg property, the 
>> Ethernet LDO is not part of MDIO.
> 
> LDO is not the part of MDIO block, LDO has the different register space
> from MDIO, which is located in the independent Ethernet part.

I have checked the Linaro's DTSs and noticed that mentioned LDO 
addresses belong to a node called 'ess-uniphy'. So these LDO(s) are part 
of UNIPHY block. So far, so good.

>> c. Is the CMN PLL part of the MDIO block of IPQ5332? Again, according 
>> to iomem address, the CMN PLL is not part of MDIO.
> 
> No, CMN PLL is not the part of MDIO block, which is the independent
> block, but it generates the clocks to the connected Ethernet devices
> managed by MDIO bus, and the CMN PLL block needs to be configured
> correctly to generate the clocks to make the MDIO devices(Ethernet
> devices) working.

I came to the same conclusion checking Linaro's DTS. So the CMN PLL 
block looks like a small block implemented outside of any other block. 
Now I am starting to understand, why everything was putted into the MDIO 
driver. This PLL is so small that it doesn't seem to deserve a dedicated 
driver. Am I got it right?

>> d. Are GCC AHB & SYS clocks really consumed by MDIO itself? Or are 
>> they need for the external reference clock generation?
> 
> GCC AHB & SYS clocks are consumed by the uniphy(PCS) that is connected
> with the Ethernet devices, so we can say the GCC AHB & SYS clocks are
> consumed by the Ethernet devices, which is not for the external
> reference clock generation, external reference clock of CMN PLL are the
> fix clock that are from wifi or external XO.

Again this UNIPHY block. The UNIPHY node was missed from the upstream 
DTS, so it was decided to assign AHB & SYS clocks to MDIO. Right?

What do you think about implementing this clocks handling functionality 
in a dedicated driver (e.g. uniphy) and create a dedicated DTS node for 
it? This driver could consume AHB & SYS clocks as well as consuming CMN 
PLL clock and be a clock provider for the Ethernet PHY (e.g. QCA8336).

And looks like CMN PLL should be implemented as a dedicated micro 
driver. A driver that consumes fixed reference clocks (XO or from WiFi) 
and provides the clock to UNIPHY, to be passed to the Ethernet PHY by 
means of LDO gate.

>> To speed up the discussion, let me share my user's view of the 
>> reference clocks modeling. I would like to join the option that has 
>> already been suggested by the maintainers. It is better to implement 
>> reference clocks handling using the clocks API, and the clock 
>> subsystem will take care of enabling and configuring them.
>>
>> And considering the expected answers to the above questions, I would 
>> like to suggest to implement the clock handling using a dedicated 
>> clock controlling driver. Or even using several of such tiny dedicated 
>> drivers. So DTS will become like this:
>>
>>    ext_ref_clock: ext_ref_clock {
>>      compatible = "fixed-clock";
>>      clock-frequency = <48000000>;
>>    };
>>
>>    eth_cmn_pll: clock-controller@9b000 {
>>      compatible = "qcom,eth-cmn-pll-ipq5223";
>>      reg = <0x9b000 0x800>;
>>      clocks = <&ext_ref_clock>; /* use external 48MHz clock */
>>    };
>>
>>    phy0_ext_clk: clock-controller@7a00610 {
>>      compatible = "qcom,ipq-eth-ldo";
>>      reg = <0x7a00610 0x4>;
>>      clocks = <&eth_cmn_pll>;
>>    };
>>
>>    mdio@90000 {
>>      compatible = "qcom,ipq4019-mdio";
>>      reg = <0x90000 0x64>;
>>      clocks = <&gcc GCC_MDIO_AHB_CLK>;
>>
>>      ethernet-phy@1 {
>>        compatible = "...";
>>        reg = <1>;
>>        clocks = <&phy0_ext_clk>;
>>        reset-gpios = <&gcc ...>;
>>      };
>>    };
> 
> Thanks Sergey for the reference DTS.
> Since the GPIO reset of qca8084/qca8386 is needed before configuring the
> Ethernet device.
> 
> The configuration of and phy0_ext_clk(LDO) should be configured
> firstly, which enables the clocks to the Ethernet devices, then the GPIO
> reset of the connected Ethernet devices(such as qca8386) can take
> effect, currently the GPIO reset takes the MDIO bus level reset.
> 
> So phy0_ext_clk can't be put in the PHY device tree node, one LDO
> controls the clock output enabled to the connected Ethernet device such
> as qca8386.

I still feel lost. Why it is impossible to specify clocks and resets in 
the PHY node and then implement the initialization sequence in the 
QCA8386 driver? I read the discussion of the QCA8386 driver submission. 
That driver modeling also looks a complex task. But it still puzzling 
me, why a part of the QCA8386 driver should be implemented inside the 
MDIO driver.

--
Sergey

Re: [PATCH v4 0/5] support ipq5332 platform

[Jie Luo]

On 1/5/2024 10:48 AM, Sergey Ryazanov wrote:
> Hi Luo,
> 
> thank you for explaining the case in such details. I also have checked 
> the related DTSs in the Linaro repository to be more familiar with the 
> I/O mem layout. Specifically I checked these two, hope they are relevant 
> to the discussion:
> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsiThanks Sergey for looking into this driver support.
> 
> Please find my comments below.
> 
> On 03.01.2024 15:31, Jie Luo wrote:
>> On 1/2/2024 7:01 AM, Sergey Ryazanov wrote:
>>> Hi Luo,
>>>
>>> I have a few questions regarding the high level design of 
>>> implementation. I hope that clarifying these topics will help us to 
>>> find a good model for the case and finally merge a supporting code. 
>>> Please find the questions below.
>>>
>>> On 25.12.2023 10:44, Luo Jie wrote:
>>>> For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
>>>> connected with one of them.
>>>>
>>>> 1. The Ethernet LDO needs to be enabled to make the PHY GPIO
>>>>     reset taking effect, which uses the MDIO bus level reset.
>>>>
>>>> 2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
>>>>     to make the ethernet PHY device accessible.
>>>>
>>>> 3. To provide the clock to the ethernet, the CMN clock needs
>>>>     to be initialized for selecting reference clock and enabling
>>>>     the output clock.
>>>>
>>>> 4. Support optional MDIO clock frequency config.
>>>>
>>>> 5. Update dt-bindings doc for the new added properties.
>>>>
>>>> Changes in v2:
>>>>     * remove the PHY related features such as PHY address
>>>>       program and clock initialization.
>>>>     * leverage the MDIO level GPIO reset for qca8084 PHY.
>>>>
>>>> Changes in v3:
>>>>     * fix the christmas-tree format issue.
>>>>     * improve the dt-binding changes.
>>>>
>>>> Changes in v4:
>>>>     * improve the CMN PLL reference clock config.
>>>>     * improve the dt-binding changes.
>>>>
>>>> Luo Jie (5):
>>>>    net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
>>>>    net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 
>>>> platform
>>>>    net: mdio: ipq4019: configure CMN PLL clock for ipq5332
>>>>    net: mdio: ipq4019: support MDIO clock frequency divider
>>>>    dt-bindings: net: ipq4019-mdio: Document ipq5332 platform
>>>>
>>>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
>>>>   drivers/net/mdio/mdio-ipq4019.c               | 288 
>>>> ++++++++++++++++--
>>>>   2 files changed, 399 insertions(+), 30 deletions(-)
>>>
>>> I'm asking these questions because after checking the patches and 
>>> following the earlier discussion, the series is looks like an 
>>> overloading of the MDIO driver with somehow but not strictly related 
>>> functionality.
>>>
>>>
>>> First, let me summarize the case. Feel free to correct me if I took 
>>> something wrong. So, we have:
>>> - a reference design contains IPQ5332 SoC + QCA8084 switch/Phy;
>>
>> IPQ5322 SoC is currently connected with qca8386(switch that includes
>> QCA8084 PHY), the pure PHY chip qca8084 is currently connected on the
>> SoC IPQ9574.
> 
> As far as I understand these chips have standardized interfaces and 
> QCA8386/QCA8084 can be reused with another SoC(s) in future. As well as 
> IPQ5332 can be used with different phy. So now we are talking about some 
> specific reference design. Isn't it?

Not the specific reference design.
You are right, these chips can be used with other SoC(s) with the
standardized interfaces supported.

> 
>>> - QCA8084 requires a reference clock for normal functionality;
>>
>> The reference clock is selected for the CMN PLL block, which outputs
>> the clocks to the Ethernet devices including the qca8084 PHY for normal
>> functionality, also for other connected Ethernet devices, the CMN PLL
>> block is located in SoC such as ipq5332 and ipq9574.
>>
>>> - IPQ5332, as a chip, is able to provide a set of reference clocks 
>>> for external devices;
>>
>> Yes, the CMN PLL block of IPQ5332 provides the output clocks as the
>> working clocks for the external Ethernet devices such as the QCA8386
>> (switch chip), the reference clocks we are discussing is as the
>> reference clock source of the CMN PLL block.
> 
> Ok, I feel we have some ambiguity regarding the reference clock term 
> here. Sure, CMN PLL needs a reference clock to functioning. And in the 
> same time, the output clock provided by CMN PLL is a reference clock for 
> QCA8384.
> 
> So, when I was talking about IPQ5332, I meant the whole chip including 
> CMN PLL block. So, I asked about CMN PLL output clock. But you already 
> clarified the SoC capabilities below.

Yes, Sergey.

> 
>>> - you want to configure IPQ5332 to provide the reference clock for 
>>> QCA8084.
>>
>> The reference clocks for CMN PLL block is configurable, and the output
>> clocks of CMN PLL are fixed, the output clocks are 50MHZ, which is given
>> to the external Ethernet devices.
>> here is the topology of clocks.
>>                     ---------
>>                     |        |
>> reference clock --->| CMN PLL|--> output 50M clocks --> qca8084/qca8386
>>                     |        |
>>                     ---------
>>>
>>>
>>> So, the high level questions are:
>>> 1. Is QCA8084 capable to consume the clock from some other generator? 
>>> Is it possible to clock QCA8084 from external XO/PLL/whatever?
>> No, the clock of qca8084/qca8386 is provided from the output clock of
>> CMN PLL as above.
> 
> Right, in case of pairing QCA8386 with IPQ5332, it is a good option to 
> provide the clock from the SoC. But in general QCA8386 will be Ok with 
> any 50 MHz clock. Right? I would like to say that thinking about this 
> specific reference design being a single possible combination limits a 
> scope of driver implementation options.

Yes, from the view of qca8386(qca8084), any input 50M reference clock
should be fine, but normally we should keep the same clock source for
qca8386 and the connected SoC to avoid any pps offset.

For example, we also tried the crystal 50M as the reference clock of
qca8386, since the SoC connected with qca8386 has the different clock
source from qca8386, which leads to some packet drop because of the
little bit of clock frequency shift.
Normally we should keep the clock source of qca8386 same as the clock
from the connected SoC.

> 
>>> 2. Is IPQ5332 capable to provide reference clock to another switch 
>>> model?
>>
>> Yes, IPQ5332 can provide the reference clock to all connected Ethernet
>> devices, such as qca8386(switch), qca8081 phy and others.
> 
> Ok. Thank you for clarifying this.
> 
>>> 3. Is the reference clock generation subsystem part of the MDIO block 
>>> of IPQ5332?
>>
>> the reference clock of CMN PLL block can be from wifi and external 
>> xtal, the CMN PLL is integrated in the MDIO block, CMN PLL is the 
>> independent
>> block that generates the clocks for the connected Ethernet devices.
>>
>>>
>>>
>>> And there are some tiny questions to make sure that we are on the 
>>> same page:
>>> a. What is the mentioned Ethernet LDO? AFAIK, LDO is some kind of 
>>> gate (or switch) that enables clock output through an IPQ5332 pin. 
>>> Isn't it?
>>
>> That's correct, the LDO is for enabling the output 50M clock of CMN PLL
>> to the connected Ethernet device, which is controlled by the hardware
>> register on the IPQ5332.
>>
>>> And if it's true, then can you clarify, what exactly clock is outputted?
>>
>> the 50M clock is outputted to the external Ethernet devices.
>>
>>> b. Is the Ethernet LDO part of the MDIO block of IPQ5332? According 
>>> to iomem addresses that was used in the example reg property, the 
>>> Ethernet LDO is not part of MDIO.
>>
>> LDO is not the part of MDIO block, LDO has the different register space
>> from MDIO, which is located in the independent Ethernet part.
> 
> I have checked the Linaro's DTSs and noticed that mentioned LDO 
> addresses belong to a node called 'ess-uniphy'. So these LDO(s) are part 
> of UNIPHY block. So far, so good.

Yes, LDO is a part of uniphy block on IPQ5332.

> 
>>> c. Is the CMN PLL part of the MDIO block of IPQ5332? Again, according 
>>> to iomem address, the CMN PLL is not part of MDIO.
>>
>> No, CMN PLL is not the part of MDIO block, which is the independent
>> block, but it generates the clocks to the connected Ethernet devices
>> managed by MDIO bus, and the CMN PLL block needs to be configured
>> correctly to generate the clocks to make the MDIO devices(Ethernet
>> devices) working.
> 
> I came to the same conclusion checking Linaro's DTS. So the CMN PLL 
> block looks like a small block implemented outside of any other block. 
> Now I am starting to understand, why everything was putted into the MDIO 
> driver. This PLL is so small that it doesn't seem to deserve a dedicated 
> driver. Am I got it right?

Yes, you are right. CMN block is a independent block, we just need to
configure this block for selecting the reference clock and then do a
reset, which is a simple configuration and the related output clocks
to the Ethernet devices, so it is put in the MDIO driver currently.

> 
>>> d. Are GCC AHB & SYS clocks really consumed by MDIO itself? Or are 
>>> they need for the external reference clock generation?
>>
>> GCC AHB & SYS clocks are consumed by the uniphy(PCS) that is connected
>> with the Ethernet devices, so we can say the GCC AHB & SYS clocks are
>> consumed by the Ethernet devices, which is not for the external
>> reference clock generation, external reference clock of CMN PLL are the
>> fix clock that are from wifi or external XO.
> 
> Again this UNIPHY block. The UNIPHY node was missed from the upstream 
> DTS, so it was decided to assign AHB & SYS clocks to MDIO. Right?

Right, currently there is no UNIPHY node defined in upstream.

> 
> What do you think about implementing this clocks handling functionality 
> in a dedicated driver (e.g. uniphy) and create a dedicated DTS node for 
> it? This driver could consume AHB & SYS clocks as well as consuming CMN 
> PLL clock and be a clock provider for the Ethernet PHY (e.g. QCA8336).

As for AHB & SYS clocks, that can be consumed by the dedicated in the
future uniphy driver, but it seem there is a sequence issue with
qca8386(qca8084) as mentioned in the reply to your comment below.

Maybe we can enable these uniphy clocks in the GCC(SoC) provider driver?
i am not sure whether it is acceptable by the GCC(SoC) provider driver.

> 
> And looks like CMN PLL should be implemented as a dedicated micro 
> driver. A driver that consumes fixed reference clocks (XO or from WiFi) 
> and provides the clock to UNIPHY, to be passed to the Ethernet PHY by 
> means of LDO gate.
the CMN PLL block can be realized as the independent driver.
maybe this CMN driver can be put in the directory drivers/clk/qcom?

> 
>>> To speed up the discussion, let me share my user's view of the 
>>> reference clocks modeling. I would like to join the option that has 
>>> already been suggested by the maintainers. It is better to implement 
>>> reference clocks handling using the clocks API, and the clock 
>>> subsystem will take care of enabling and configuring them.
>>>
>>> And considering the expected answers to the above questions, I would 
>>> like to suggest to implement the clock handling using a dedicated 
>>> clock controlling driver. Or even using several of such tiny 
>>> dedicated drivers. So DTS will become like this:
>>>
>>>    ext_ref_clock: ext_ref_clock {
>>>      compatible = "fixed-clock";
>>>      clock-frequency = <48000000>;
>>>    };
>>>
>>>    eth_cmn_pll: clock-controller@9b000 {
>>>      compatible = "qcom,eth-cmn-pll-ipq5223";
>>>      reg = <0x9b000 0x800>;
>>>      clocks = <&ext_ref_clock>; /* use external 48MHz clock */
>>>    };
>>>
>>>    phy0_ext_clk: clock-controller@7a00610 {
>>>      compatible = "qcom,ipq-eth-ldo";
>>>      reg = <0x7a00610 0x4>;
>>>      clocks = <&eth_cmn_pll>;
>>>    };
>>>
>>>    mdio@90000 {
>>>      compatible = "qcom,ipq4019-mdio";
>>>      reg = <0x90000 0x64>;
>>>      clocks = <&gcc GCC_MDIO_AHB_CLK>;
>>>
>>>      ethernet-phy@1 {
>>>        compatible = "...";
>>>        reg = <1>;
>>>        clocks = <&phy0_ext_clk>;
>>>        reset-gpios = <&gcc ...>;
>>>      };
>>>    };
>>
>> Thanks Sergey for the reference DTS.
>> Since the GPIO reset of qca8084/qca8386 is needed before configuring the
>> Ethernet device.
>>
>> The configuration of and phy0_ext_clk(LDO) should be configured
>> firstly, which enables the clocks to the Ethernet devices, then the GPIO
>> reset of the connected Ethernet devices(such as qca8386) can take
>> effect, currently the GPIO reset takes the MDIO bus level reset.
>>
>> So phy0_ext_clk can't be put in the PHY device tree node, one LDO
>> controls the clock output enabled to the connected Ethernet device such
>> as qca8386.
> 
> I still feel lost. Why it is impossible to specify clocks and resets in 
> the PHY node and then implement the initialization sequence in the 
> QCA8386 driver? I read the discussion of the QCA8386 driver submission. 
> That driver modeling also looks a complex task. But it still puzzling 
> me, why a part of the QCA8386 driver should be implemented inside the 
> MDIO driver.
> 
> -- 
> Sergey

Let me clarify the work sequence here.
1. configure CMN PLL to generate the reference clocks for qca8386(
same as qca8084).
2. enable LDO and configure the uniphy ahb & sys clocks.
3. do GPIO reset on qca8386(qca8084), the GPIO reset is for chip,
just need to do one GPIO reset on quad PHYs.
4. configure the initial clocks and resets, which are from NSSCC
clock provider driver, the NSSCC is also located in qca8386(qca8084),
these clocks and resets for all quad PHYs of qca8386(qca8084), which
just needs to be initialized one time.
5. then the qca8386(qca8084) PHY capability can be acquired correctly in
the PHY probe function.

Currently, The GPIO reset of qca8386(qca8084) takes use of the MDIO
level GPIO reset, so i put the LDO enable in the MDIO probe function
called before MDIO bus level reset.

To take your proposal, we can't use the MDIO bus level reset and MDIO
device level reset from the MDIO bus framework code, we need to do
reset in one PHY probe function, and the CMN driver and uniphy driver
needs to be initialized before PHY probe function, CMN driver is fine,
but it seems be not usual for uniphy(pcs) driver called before PHY probe
function.

Re: [PATCH v4 0/5] support ipq5332 platform

[Sergey Ryazanov]

On 05.01.2024 12:34, Jie Luo wrote:
> On 1/5/2024 10:48 AM, Sergey Ryazanov wrote:
>> thank you for explaining the case in such details. I also have checked 
>> the related DTSs in the Linaro repository to be more familiar with the 
>> I/O mem layout. Specifically I checked these two, hope they are 
>> relevant to the discussion:
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>
> Thanks Sergey for looking into this driver support.
>
>> Please find my comments below.
>>
>> On 03.01.2024 15:31, Jie Luo wrote:
>>> On 1/2/2024 7:01 AM, Sergey Ryazanov wrote:
>>>> Hi Luo,
>>>>
>>>> I have a few questions regarding the high level design of 
>>>> implementation. I hope that clarifying these topics will help us to 
>>>> find a good model for the case and finally merge a supporting code. 
>>>> Please find the questions below.
>>>>
>>>> On 25.12.2023 10:44, Luo Jie wrote:
>>>>> For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
>>>>> connected with one of them.
>>>>>
>>>>> 1. The Ethernet LDO needs to be enabled to make the PHY GPIO
>>>>>     reset taking effect, which uses the MDIO bus level reset.
>>>>>
>>>>> 2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
>>>>>     to make the ethernet PHY device accessible.
>>>>>
>>>>> 3. To provide the clock to the ethernet, the CMN clock needs
>>>>>     to be initialized for selecting reference clock and enabling
>>>>>     the output clock.
>>>>>
>>>>> 4. Support optional MDIO clock frequency config.
>>>>>
>>>>> 5. Update dt-bindings doc for the new added properties.
>>>>>
>>>>> Changes in v2:
>>>>>     * remove the PHY related features such as PHY address
>>>>>       program and clock initialization.
>>>>>     * leverage the MDIO level GPIO reset for qca8084 PHY.
>>>>>
>>>>> Changes in v3:
>>>>>     * fix the christmas-tree format issue.
>>>>>     * improve the dt-binding changes.
>>>>>
>>>>> Changes in v4:
>>>>>     * improve the CMN PLL reference clock config.
>>>>>     * improve the dt-binding changes.
>>>>>
>>>>> Luo Jie (5):
>>>>>    net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
>>>>>    net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 
>>>>> platform
>>>>>    net: mdio: ipq4019: configure CMN PLL clock for ipq5332
>>>>>    net: mdio: ipq4019: support MDIO clock frequency divider
>>>>>    dt-bindings: net: ipq4019-mdio: Document ipq5332 platform
>>>>>
>>>>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
>>>>>   drivers/net/mdio/mdio-ipq4019.c               | 288 
>>>>> ++++++++++++++++--
>>>>>   2 files changed, 399 insertions(+), 30 deletions(-)
>>>>
>>>> I'm asking these questions because after checking the patches and 
>>>> following the earlier discussion, the series is looks like an 
>>>> overloading of the MDIO driver with somehow but not strictly related 
>>>> functionality.
>>>>
>>>>
>>>> First, let me summarize the case. Feel free to correct me if I took 
>>>> something wrong. So, we have:
>>>> - a reference design contains IPQ5332 SoC + QCA8084 switch/Phy;
>>>
>>> IPQ5322 SoC is currently connected with qca8386(switch that includes
>>> QCA8084 PHY), the pure PHY chip qca8084 is currently connected on the
>>> SoC IPQ9574.
>>
>> As far as I understand these chips have standardized interfaces and 
>> QCA8386/QCA8084 can be reused with another SoC(s) in future. As well 
>> as IPQ5332 can be used with different phy. So now we are talking about 
>> some specific reference design. Isn't it?
> 
> Not the specific reference design.
> You are right, these chips can be used with other SoC(s) with the
> standardized interfaces supported.
> 
>>>> - QCA8084 requires a reference clock for normal functionality;
>>>
>>> The reference clock is selected for the CMN PLL block, which outputs
>>> the clocks to the Ethernet devices including the qca8084 PHY for normal
>>> functionality, also for other connected Ethernet devices, the CMN PLL
>>> block is located in SoC such as ipq5332 and ipq9574.
>>>
>>>> - IPQ5332, as a chip, is able to provide a set of reference clocks 
>>>> for external devices;
>>>
>>> Yes, the CMN PLL block of IPQ5332 provides the output clocks as the
>>> working clocks for the external Ethernet devices such as the QCA8386
>>> (switch chip), the reference clocks we are discussing is as the
>>> reference clock source of the CMN PLL block.
>>
>> Ok, I feel we have some ambiguity regarding the reference clock term 
>> here. Sure, CMN PLL needs a reference clock to functioning. And in the 
>> same time, the output clock provided by CMN PLL is a reference clock 
>> for QCA8384.
>>
>> So, when I was talking about IPQ5332, I meant the whole chip including 
>> CMN PLL block. So, I asked about CMN PLL output clock. But you already 
>> clarified the SoC capabilities below.
> 
> Yes, Sergey.
> 
>>
>>>> - you want to configure IPQ5332 to provide the reference clock for 
>>>> QCA8084.
>>>
>>> The reference clocks for CMN PLL block is configurable, and the output
>>> clocks of CMN PLL are fixed, the output clocks are 50MHZ, which is given
>>> to the external Ethernet devices.
>>> here is the topology of clocks.
>>>                     ---------
>>>                     |        |
>>> reference clock --->| CMN PLL|--> output 50M clocks --> qca8084/qca8386
>>>                     |        |
>>>                     ---------
>>>>
>>>>
>>>> So, the high level questions are:
>>>> 1. Is QCA8084 capable to consume the clock from some other 
>>>> generator? Is it possible to clock QCA8084 from external 
>>>> XO/PLL/whatever?
>>> No, the clock of qca8084/qca8386 is provided from the output clock of
>>> CMN PLL as above.
>>
>> Right, in case of pairing QCA8386 with IPQ5332, it is a good option to 
>> provide the clock from the SoC. But in general QCA8386 will be Ok with 
>> any 50 MHz clock. Right? I would like to say that thinking about this 
>> specific reference design being a single possible combination limits a 
>> scope of driver implementation options.
> 
> Yes, from the view of qca8386(qca8084), any input 50M reference clock
> should be fine, but normally we should keep the same clock source for
> qca8386 and the connected SoC to avoid any pps offset.
> 
> For example, we also tried the crystal 50M as the reference clock of
> qca8386, since the SoC connected with qca8386 has the different clock
> source from qca8386, which leads to some packet drop because of the
> little bit of clock frequency shift.
> Normally we should keep the clock source of qca8386 same as the clock
> from the connected SoC.

Great, so we have to deal with solution that more or less following 
standards. So far, so good.

>>>> 2. Is IPQ5332 capable to provide reference clock to another switch 
>>>> model?
>>>
>>> Yes, IPQ5332 can provide the reference clock to all connected Ethernet
>>> devices, such as qca8386(switch), qca8081 phy and others.
>>
>> Ok. Thank you for clarifying this.
>>
>>>> 3. Is the reference clock generation subsystem part of the MDIO 
>>>> block of IPQ5332?
>>>
>>> the reference clock of CMN PLL block can be from wifi and external 
>>> xtal, the CMN PLL is integrated in the MDIO block, CMN PLL is the 
>>> independent
>>> block that generates the clocks for the connected Ethernet devices.
>>>
>>>>
>>>>
>>>> And there are some tiny questions to make sure that we are on the 
>>>> same page:
>>>> a. What is the mentioned Ethernet LDO? AFAIK, LDO is some kind of 
>>>> gate (or switch) that enables clock output through an IPQ5332 pin. 
>>>> Isn't it?
>>>
>>> That's correct, the LDO is for enabling the output 50M clock of CMN PLL
>>> to the connected Ethernet device, which is controlled by the hardware
>>> register on the IPQ5332.
>>>
>>>> And if it's true, then can you clarify, what exactly clock is 
>>>> outputted?
>>>
>>> the 50M clock is outputted to the external Ethernet devices.
>>>
>>>> b. Is the Ethernet LDO part of the MDIO block of IPQ5332? According 
>>>> to iomem addresses that was used in the example reg property, the 
>>>> Ethernet LDO is not part of MDIO.
>>>
>>> LDO is not the part of MDIO block, LDO has the different register space
>>> from MDIO, which is located in the independent Ethernet part.
>>
>> I have checked the Linaro's DTSs and noticed that mentioned LDO 
>> addresses belong to a node called 'ess-uniphy'. So these LDO(s) are 
>> part of UNIPHY block. So far, so good.
> 
> Yes, LDO is a part of uniphy block on IPQ5332.
> 
>>>> c. Is the CMN PLL part of the MDIO block of IPQ5332? Again, 
>>>> according to iomem address, the CMN PLL is not part of MDIO.
>>>
>>> No, CMN PLL is not the part of MDIO block, which is the independent
>>> block, but it generates the clocks to the connected Ethernet devices
>>> managed by MDIO bus, and the CMN PLL block needs to be configured
>>> correctly to generate the clocks to make the MDIO devices(Ethernet
>>> devices) working.
>>
>> I came to the same conclusion checking Linaro's DTS. So the CMN PLL 
>> block looks like a small block implemented outside of any other block. 
>> Now I am starting to understand, why everything was putted into the 
>> MDIO driver. This PLL is so small that it doesn't seem to deserve a 
>> dedicated driver. Am I got it right?
> 
> Yes, you are right. CMN block is a independent block, we just need to
> configure this block for selecting the reference clock and then do a
> reset, which is a simple configuration and the related output clocks
> to the Ethernet devices, so it is put in the MDIO driver currently.
> 
>>>> d. Are GCC AHB & SYS clocks really consumed by MDIO itself? Or are 
>>>> they need for the external reference clock generation?
>>>
>>> GCC AHB & SYS clocks are consumed by the uniphy(PCS) that is connected
>>> with the Ethernet devices, so we can say the GCC AHB & SYS clocks are
>>> consumed by the Ethernet devices, which is not for the external
>>> reference clock generation, external reference clock of CMN PLL are the
>>> fix clock that are from wifi or external XO.
>>
>> Again this UNIPHY block. The UNIPHY node was missed from the upstream 
>> DTS, so it was decided to assign AHB & SYS clocks to MDIO. Right?
> 
> Right, currently there is no UNIPHY node defined in upstream.
> 
>> What do you think about implementing this clocks handling 
>> functionality in a dedicated driver (e.g. uniphy) and create a 
>> dedicated DTS node for it? This driver could consume AHB & SYS clocks 
>> as well as consuming CMN PLL clock and be a clock provider for the 
>> Ethernet PHY (e.g. QCA8336).
> 
> As for AHB & SYS clocks, that can be consumed by the dedicated in the
> future uniphy driver, but it seem there is a sequence issue with
> qca8386(qca8084) as mentioned in the reply to your comment below.
> 
> Maybe we can enable these uniphy clocks in the GCC(SoC) provider driver?
> i am not sure whether it is acceptable by the GCC(SoC) provider driver.

I do not think that idea of putting the UNIPHY clocks into GCC is any 
better than putting its handling into the MDIO driver. UNIPHY and GCC 
look like belong to different chip blocks.

I just realized that the UNIPHY block is a MII (probably SGMII) 
controller. Isn't it? And I expect that it responsible more then just 
for clock enabling. It should also activate and perform a basic 
configuration of MII for actual data transmission. If so, then it should 
placed somewhere under drivers/net/phy or drivers/net/pcs.

>> And looks like CMN PLL should be implemented as a dedicated micro 
>> driver. A driver that consumes fixed reference clocks (XO or from 
>> WiFi) and provides the clock to UNIPHY, to be passed to the Ethernet 
>> PHY by means of LDO gate.
>
> the CMN PLL block can be realized as the independent driver.
> maybe this CMN driver can be put in the directory drivers/clk/qcom?

Yep, exactly.

As far as I understand, we basically agree that clocks configuration can 
be implemented based on the clock API using a more specialized driver(s) 
than MDIO. The only obstacle is the PHY chip initialization issue 
explained below.

>>>> To speed up the discussion, let me share my user's view of the 
>>>> reference clocks modeling. I would like to join the option that has 
>>>> already been suggested by the maintainers. It is better to implement 
>>>> reference clocks handling using the clocks API, and the clock 
>>>> subsystem will take care of enabling and configuring them.
>>>>
>>>> And considering the expected answers to the above questions, I would 
>>>> like to suggest to implement the clock handling using a dedicated 
>>>> clock controlling driver. Or even using several of such tiny 
>>>> dedicated drivers. So DTS will become like this:
>>>>
>>>>    ext_ref_clock: ext_ref_clock {
>>>>      compatible = "fixed-clock";
>>>>      clock-frequency = <48000000>;
>>>>    };
>>>>
>>>>    eth_cmn_pll: clock-controller@9b000 {
>>>>      compatible = "qcom,eth-cmn-pll-ipq5223";
>>>>      reg = <0x9b000 0x800>;
>>>>      clocks = <&ext_ref_clock>; /* use external 48MHz clock */
>>>>    };
>>>>
>>>>    phy0_ext_clk: clock-controller@7a00610 {
>>>>      compatible = "qcom,ipq-eth-ldo";
>>>>      reg = <0x7a00610 0x4>;
>>>>      clocks = <&eth_cmn_pll>;
>>>>    };
>>>>
>>>>    mdio@90000 {
>>>>      compatible = "qcom,ipq4019-mdio";
>>>>      reg = <0x90000 0x64>;
>>>>      clocks = <&gcc GCC_MDIO_AHB_CLK>;
>>>>
>>>>      ethernet-phy@1 {
>>>>        compatible = "...";
>>>>        reg = <1>;
>>>>        clocks = <&phy0_ext_clk>;
>>>>        reset-gpios = <&gcc ...>;
>>>>      };
>>>>    };
>>>
>>> Thanks Sergey for the reference DTS.
>>> Since the GPIO reset of qca8084/qca8386 is needed before configuring the
>>> Ethernet device.
>>>
>>> The configuration of and phy0_ext_clk(LDO) should be configured
>>> firstly, which enables the clocks to the Ethernet devices, then the GPIO
>>> reset of the connected Ethernet devices(such as qca8386) can take
>>> effect, currently the GPIO reset takes the MDIO bus level reset.
>>>
>>> So phy0_ext_clk can't be put in the PHY device tree node, one LDO
>>> controls the clock output enabled to the connected Ethernet device such
>>> as qca8386.
>>
>> I still feel lost. Why it is impossible to specify clocks and resets 
>> in the PHY node and then implement the initialization sequence in the 
>> QCA8386 driver? I read the discussion of the QCA8386 driver 
>> submission. That driver modeling also looks a complex task. But it 
>> still puzzling me, why a part of the QCA8386 driver should be 
>> implemented inside the MDIO driver.
> 
> Let me clarify the work sequence here.
> 1. configure CMN PLL to generate the reference clocks for qca8386(
> same as qca8084).
> 2. enable LDO and configure the uniphy ahb & sys clocks.
> 3. do GPIO reset on qca8386(qca8084), the GPIO reset is for chip,
> just need to do one GPIO reset on quad PHYs.
> 4. configure the initial clocks and resets, which are from NSSCC
> clock provider driver, the NSSCC is also located in qca8386(qca8084),
> these clocks and resets for all quad PHYs of qca8386(qca8084), which
> just needs to be initialized one time.
> 5. then the qca8386(qca8084) PHY capability can be acquired correctly in
> the PHY probe function.
> 
> Currently, The GPIO reset of qca8386(qca8084) takes use of the MDIO
> level GPIO reset, so i put the LDO enable in the MDIO probe function
> called before MDIO bus level reset.
> 
> To take your proposal, we can't use the MDIO bus level reset and MDIO
> device level reset from the MDIO bus framework code, we need to do
> reset in one PHY probe function, and the CMN driver and uniphy driver
> needs to be initialized before PHY probe function, CMN driver is fine,
> but it seems be not usual for uniphy(pcs) driver called before PHY probe
> function.

Thank you for this compact yet detailed summary. Now it much more clear, 
what this phy chip requires to be initialized.

Looks like you need to implement at least two drivers:
1. chip (package) level driver that is responsible for basic "package" 
initialization;
2. phy driver to handle actual phy capabilities.

In case of DTS, device enumeration on the MDIO bus will work like this 
(see __of_mdiobus_register() function):
1. will be probed all mdio node childs with 'reg' property;
2. will be probed all child nodes without 'reg' property;
3. in both cases 1 & 2 nodes will be probed sequentially and synchronously.

So, using two drivers (package level + phy) you can declare a node 
describing the package first, and then add per-phy nodes.

   ext_ref_clock: ext_ref_clock {
     compatible = "fixed-clock";
     clock-frequency = <48000000>;
   };

   eth_cmn_pll: clock-controller@9b000 {
     compatible = "qcom,eth-cmn-pll-ipq5223";
     reg = <0x9b000 0x800>;
     clocks = <&ext_ref_clock>; /* use external 48MHz clock */
   };

   uniphy: mii-controller@7a00000 {
     compatible = "qcom,ipq5332-uniphy";
     reg = <0x7a00000 0x20000>;
     clocks = <&eth_cmn_pll>;
     #clock-cells = <1>;
   };

   mdio@90000 {
     compatible = "qcom,ipq4019-mdio";
     reg = <0x90000 0x64>;
     clocks = <&gcc GCC_MDIO_AHB_CLK>;

     /* Package level node must be first, so it will be probed first */
     phy_pkg: multi-phy-controller@0 {
       compatible = "qcom,qca8084";  /* matches package-level driver */
       reg = <0>;                    /* value can be any */
       clocks = <&uniphy 0>, <&uniphy 1>, ...;
       reset-gpios = <&gcc 123>;
       qcom,phy-addr-fixup = <1>, <2>, <3>, <4>;
       #clock-cells = <1>;
     };

     phy1: ethernet-phy@1 {
       compatible = "ethernet-phy-id004d.d180"; /* matches phy driver */
       reg = <1>;
       clocks = <&phy_pkg 321>;
     };

     phy2: ethernet-phy@2 {
       compatible = "ethernet-phy-id004d.d180";
       reg = <2>;
       clocks = <&phy_pkg 322>;
     };

     ...
   };


And inside the package-level driver ("qcom,qca8084") you can easily 
request clocks from the kernel using the regular clock API. Request GPIO 
line, etc. And then perform any initialization sequence required by 
QCA8084. As soon as common initialization is done and PHY addresses are 
configured, return from the probe callback.

Then MDIO devices enumeration code will continue the enumeration 
process, find a next node, what will be PHY1, PHY2, etc.

I hope I understand the case correctly and this brief description and 
example DT will give some clues as to a possible solution.

--
Sergey

Re: [PATCH v4 0/5] support ipq5332 platform

[Andrew Lunn]

> I just realized that the UNIPHY block is a MII (probably SGMII) controller.
> Isn't it? And I expect that it responsible more then just for clock
> enabling. It should also activate and perform a basic configuration of MII
> for actual data transmission. If so, then it should placed somewhere under
> drivers/net/phy or drivers/net/pcs.

Before we decide that, we need a description of what the UNIPHY
actually does, what registers it has, etc. Sometimes blocks like this
get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
etc. The SERDES parts go into a generic PHY driver, and the SGMII on
to of the SERDES is placed is a PCS driver.

The problem i have so far is that there is no usable description of
any of this hardware, and the developers trying to produce drivers for
this hardware don't actually seem to understand the Linux architecture
for things like this.

> As far as I understand, we basically agree that clocks configuration can be
> implemented based on the clock API using a more specialized driver(s) than
> MDIO. The only obstacle is the PHY chip initialization issue explained
> below.
> Thank you for this compact yet detailed summary. Now it much more clear,
> what this phy chip requires to be initialized.
> 
> Looks like you need to implement at least two drivers:
> 1. chip (package) level driver that is responsible for basic "package"
> initialization;
> 2. phy driver to handle actual phy capabilities.

Nope. As i keep saying, please look at the work Christian is
doing. phylib already has the concept of a PHY package, e.g. look at
the MSCC driver, and how it uses devm_phy_package_join(). What is
missing is a DT binding which allows package properties to be
expressed in DT. And this is what Christian is adding.

	  Andrew

Re: [PATCH v4 0/5] support ipq5332 platform

[Sergey Ryazanov]

On 06.01.2024 17:45, Andrew Lunn wrote:
>> I just realized that the UNIPHY block is a MII (probably SGMII) controller.
>> Isn't it? And I expect that it responsible more then just for clock
>> enabling. It should also activate and perform a basic configuration of MII
>> for actual data transmission. If so, then it should placed somewhere under
>> drivers/net/phy or drivers/net/pcs.
> 
> Before we decide that, we need a description of what the UNIPHY
> actually does, what registers it has, etc. Sometimes blocks like this
> get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
> would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
> etc. The SERDES parts go into a generic PHY driver, and the SGMII on
> to of the SERDES is placed is a PCS driver.

As far as I understand, UNIPHY only contains SGMII/PSGMII/whatever and a 
simple clock controller. PCIe & USB phys are implemented in other 
hardware blocks. See the lately merged USB support code for similar 
IPQ5018 SoC. But I can only speak to what I found searching online and 
checking the vendor's qca-ssdk "driver".

https://git.codelinaro.org/clo/qsdk/oss/lklm/qca-ssdk/-/tree/NHSS.QSDK.12.4.5.r3

I hope Luo can clarify with more confidence.

> The problem i have so far is that there is no usable description of
> any of this hardware, and the developers trying to produce drivers for
> this hardware don't actually seem to understand the Linux architecture
> for things like this.
>
>> As far as I understand, we basically agree that clocks configuration can be
>> implemented based on the clock API using a more specialized driver(s) than
>> MDIO. The only obstacle is the PHY chip initialization issue explained
>> below.
>> Thank you for this compact yet detailed summary. Now it much more clear,
>> what this phy chip requires to be initialized.
>>
>> Looks like you need to implement at least two drivers:
>> 1. chip (package) level driver that is responsible for basic "package"
>> initialization;
>> 2. phy driver to handle actual phy capabilities.
> 
> Nope. As i keep saying, please look at the work Christian is
> doing. phylib already has the concept of a PHY package, e.g. look at
> the MSCC driver, and how it uses devm_phy_package_join(). What is
> missing is a DT binding which allows package properties to be
> expressed in DT. And this is what Christian is adding.

Andrew, thank you so much for pointing me to that API and Christian's 
work. I have checked the DT change proposal and it fits this QCA8084 
case perfectly.

Am I right that all one has to do to solve this QCA8084 initialization 
case is wrap phys in a ethernet-phy-package node and use 
devm_phy_package_join() / phy_package_init_once() to do the basic 
initialization? So simple?

I came to put my 2c in and learnt a couple of new tricks. What a nice day :)

--
Sergey

Re: [PATCH v4 0/5] support ipq5332 platform

[Andrew Lunn]

> Andrew, thank you so much for pointing me to that API and Christian's work.
> I have checked the DT change proposal and it fits this QCA8084 case
> perfectly.

Not too surprising, since Christian is working on another Qualcomm PHY
which is very similar.

> Am I right that all one has to do to solve this QCA8084 initialization case
> is wrap phys in a ethernet-phy-package node and use devm_phy_package_join()
> / phy_package_init_once() to do the basic initialization? So simple?

I hope so. Once the correct kernel abstracts are used, it should be
reasonably straight forward. The clock stuff should be made into a
common clock driver, so all the consumer needs to do is enable the one
clock its needs and common clock driver core goes up the tree and
enables what ever needs enabling. It could be we need to use ID values
in the compatible get the PHY driver probed, rather than enumerate it.

Hopefully Lenaro can help get this all done correctly.

    Andrew

Re: [PATCH v4 0/5] support ipq5332 platform

[Jie Luo]

On 1/7/2024 6:03 AM, Sergey Ryazanov wrote:
> On 06.01.2024 17:45, Andrew Lunn wrote:
>>> I just realized that the UNIPHY block is a MII (probably SGMII) 
>>> controller.
>>> Isn't it? And I expect that it responsible more then just for clock
>>> enabling. It should also activate and perform a basic configuration 
>>> of MII
>>> for actual data transmission. If so, then it should placed somewhere 
>>> under
>>> drivers/net/phy or drivers/net/pcs.
>>
>> Before we decide that, we need a description of what the UNIPHY
>> actually does, what registers it has, etc. Sometimes blocks like this
>> get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
>> would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
>> etc. The SERDES parts go into a generic PHY driver, and the SGMII on
>> to of the SERDES is placed is a PCS driver.
> 
> As far as I understand, UNIPHY only contains SGMII/PSGMII/whatever and a 
> simple clock controller. PCIe & USB phys are implemented in other 
> hardware blocks. See the lately merged USB support code for similar 
> IPQ5018 SoC. But I can only speak to what I found searching online and 
> checking the vendor's qca-ssdk "driver".
> 
> https://git.codelinaro.org/clo/qsdk/oss/lklm/qca-ssdk/-/tree/NHSS.QSDK.12.4.5.r3
> 
> I hope Luo can clarify with more confidence.

Yes, Sergey. UNIPHY includes the interface mode controller(SGMII/UXGMII
PSGMII etc.) and the clock controller that provides the clocks to the
PPE(packet process engine) ports, which is the dedicated UNIPHY(PCS) for
connecting external PHY(such as qca8084 PHY) and located in the PPE
hardware block. The UNIPHY of PPE can't be used for PCIE & USB.

> 
>> The problem i have so far is that there is no usable description of
>> any of this hardware, and the developers trying to produce drivers for
>> this hardware don't actually seem to understand the Linux architecture
>> for things like this.
>>
>>> As far as I understand, we basically agree that clocks configuration 
>>> can be
>>> implemented based on the clock API using a more specialized driver(s) 
>>> than
>>> MDIO. The only obstacle is the PHY chip initialization issue explained
>>> below.
>>> Thank you for this compact yet detailed summary. Now it much more clear,
>>> what this phy chip requires to be initialized.
>>>
>>> Looks like you need to implement at least two drivers:
>>> 1. chip (package) level driver that is responsible for basic "package"
>>> initialization;
>>> 2. phy driver to handle actual phy capabilities.
>>
>> Nope. As i keep saying, please look at the work Christian is
>> doing. phylib already has the concept of a PHY package, e.g. look at
>> the MSCC driver, and how it uses devm_phy_package_join(). What is
>> missing is a DT binding which allows package properties to be
>> expressed in DT. And this is what Christian is adding.
> 
> Andrew, thank you so much for pointing me to that API and Christian's 
> work. I have checked the DT change proposal and it fits this QCA8084 
> case perfectly.
> 
> Am I right that all one has to do to solve this QCA8084 initialization 
> case is wrap phys in a ethernet-phy-package node and use 
> devm_phy_package_join() / phy_package_init_once() to do the basic 
> initialization? So simple?
> 
> I came to put my 2c in and learnt a couple of new tricks. What a nice 
> day :)
> 
> -- 
> Sergey

Re: [PATCH v4 0/5] support ipq5332 platform

[Jie Luo]

On 1/6/2024 11:45 PM, Andrew Lunn wrote:
>> I just realized that the UNIPHY block is a MII (probably SGMII) controller.
>> Isn't it? And I expect that it responsible more then just for clock
>> enabling. It should also activate and perform a basic configuration of MII
>> for actual data transmission. If so, then it should placed somewhere under
>> drivers/net/phy or drivers/net/pcs.

UNIPHY is located in PPE, which controls the interface mode for
connecting with external PHY, the hardware register(4 bytes) of UNIPHY
is accessed by local bus(ioremap).

> 
> Before we decide that, we need a description of what the UNIPHY
> actually does, what registers it has, etc. Sometimes blocks like this
> get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
> would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
> etc. The SERDES parts go into a generic PHY driver, and the SGMII on
> to of the SERDES is placed is a PCS driver.

Hi Andrew,
the UNIPHY is the hardware part of PPE(packet process engine) in IPQ
platform, which can't be used for USB, SATA serdes, the UNIPHY of
PPE is dedicated for connecting with external PHY CHIP.

> 
> The problem i have so far is that there is no usable description of
> any of this hardware, and the developers trying to produce drivers for
> this hardware don't actually seem to understand the Linux architecture
> for things like this.

Sorry for missing this description of UNIPHY, since the UNIPHY block is
the part of PPE, PPE driver will be posted as the independent driver for
review, so i did not give the description of UNIPHY.

The IPQ PPE includes MAC and UNIPHY integrated, the connection with
external PHY is as below.
MAC ---- UNIPHY(PCS) ---- (PCS)external PHY.

The UNIPHY here is the Ethernet dedicated SERDES for connecting with
external PHY.

> 
>> As far as I understand, we basically agree that clocks configuration can be
>> implemented based on the clock API using a more specialized driver(s) than
>> MDIO. The only obstacle is the PHY chip initialization issue explained
>> below.
>> Thank you for this compact yet detailed summary. Now it much more clear,
>> what this phy chip requires to be initialized.
>>
>> Looks like you need to implement at least two drivers:
>> 1. chip (package) level driver that is responsible for basic "package"
>> initialization;
>> 2. phy driver to handle actual phy capabilities.
> 
> Nope. As i keep saying, please look at the work Christian is
> doing. phylib already has the concept of a PHY package, e.g. look at
> the MSCC driver, and how it uses devm_phy_package_join(). What is
> missing is a DT binding which allows package properties to be
> expressed in DT. And this is what Christian is adding.
> 
> 	  Andrew

Thanks Andrew, the driver of qca8084 will be updated based on the
concept of PHY package.

Re: [PATCH v4 0/5] support ipq5332 platform

[Andrew Lunn]

> The IPQ PPE includes MAC and UNIPHY integrated, the connection with
> external PHY is as below.
> MAC ---- UNIPHY(PCS) ---- (PCS)external PHY.
> 
> The UNIPHY here is the Ethernet dedicated SERDES for connecting with
> external PHY.

You call it a PCS here. So does it implement clause 37 or 73 of the
802.3 standard? If it does, the driver for it belongs in
drivers/net/pcs.

	Andrew

Re: [PATCH v4 0/5] support ipq5332 platform

[Jie Luo]

On 1/8/2024 9:27 PM, Andrew Lunn wrote:
>> The IPQ PPE includes MAC and UNIPHY integrated, the connection with
>> external PHY is as below.
>> MAC ---- UNIPHY(PCS) ---- (PCS)external PHY.
>>
>> The UNIPHY here is the Ethernet dedicated SERDES for connecting with
>> external PHY.
> 
> You call it a PCS here. So does it implement clause 37 or 73 of the
> 802.3 standard? If it does, the driver for it belongs in
> drivers/net/pcs.
> 
> 	Andrew

Hi Andrew,
The PPE integrated PCSes support multiple interface modes such as SGMII,
UXSGMII, QSGMII, PSGMII and 10g-baser etc. which is configurable for
connecting the different PHY devices.

the SGMII and UXSGMII follows Cisco standard, which does not implement
the 802.3 standard in this interface modes.
PSGMII includes the qcom private protocol.

The PPE driver also including the integrated PCS driver will be
posted for the review in the near future, we can discuss in detail
based on that patch series raised.

Thanks for the suggestions.

Re: [PATCH v4 0/5] support ipq5332 platform

[Russell King (Oracle)]

On Sat, Jan 06, 2024 at 04:45:08PM +0100, Andrew Lunn wrote:
> > I just realized that the UNIPHY block is a MII (probably SGMII) controller.
> > Isn't it? And I expect that it responsible more then just for clock
> > enabling. It should also activate and perform a basic configuration of MII
> > for actual data transmission. If so, then it should placed somewhere under
> > drivers/net/phy or drivers/net/pcs.
> 
> Before we decide that, we need a description of what the UNIPHY
> actually does, what registers it has, etc. Sometimes blocks like this
> get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
> would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
> etc. The SERDES parts go into a generic PHY driver, and the SGMII on
> to of the SERDES is placed is a PCS driver.
> 
> The problem i have so far is that there is no usable description of
> any of this hardware, and the developers trying to produce drivers for
> this hardware don't actually seem to understand the Linux architecture
> for things like this.

+1. I think it's now more convoluted than ever, and someone needs to
take a step back, look at the hardware, look at the kernel model, and
work out how to implement this. It needs to be explained in a clear
and concise way in _one_ go, not spread over multiple emails. Probably
with ASCII art diagrams showing the structure.

If that isn't possible, then someone needs to provide a detailed
description of the hardware so that the subsystem maintainers get a
proper view of what this hardware is so they can advise. This is the
least preferable option due to the maintainer time it takes.

If neither of these two things happen, then I'm afraid all bets are
off for getting this into the kernel.

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

Re: [PATCH v4 0/5] support ipq5332 platform

[Jie Luo]

On 1/8/2024 11:53 PM, Russell King (Oracle) wrote:
> On Sat, Jan 06, 2024 at 04:45:08PM +0100, Andrew Lunn wrote:
>>> I just realized that the UNIPHY block is a MII (probably SGMII) controller.
>>> Isn't it? And I expect that it responsible more then just for clock
>>> enabling. It should also activate and perform a basic configuration of MII
>>> for actual data transmission. If so, then it should placed somewhere under
>>> drivers/net/phy or drivers/net/pcs.
>>
>> Before we decide that, we need a description of what the UNIPHY
>> actually does, what registers it has, etc. Sometimes blocks like this
>> get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
>> would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
>> etc. The SERDES parts go into a generic PHY driver, and the SGMII on
>> to of the SERDES is placed is a PCS driver.
>>
>> The problem i have so far is that there is no usable description of
>> any of this hardware, and the developers trying to produce drivers for
>> this hardware don't actually seem to understand the Linux architecture
>> for things like this.
> 
> +1. I think it's now more convoluted than ever, and someone needs to
> take a step back, look at the hardware, look at the kernel model, and
> work out how to implement this. It needs to be explained in a clear
> and concise way in _one_ go, not spread over multiple emails. Probably
> with ASCII art diagrams showing the structure.
> 
> If that isn't possible, then someone needs to provide a detailed
> description of the hardware so that the subsystem maintainers get a
> proper view of what this hardware is so they can advise. This is the
> least preferable option due to the maintainer time it takes.
> 
> If neither of these two things happen, then I'm afraid all bets are
> off for getting this into the kernel.
> 

Thanks Russell for suggestions.
I will provide the diagram of the hardware and the description in the
cover letter of new patch set, i know it is really important to for the
review smoothly.

Christian's work is designing the PHY package level DT, which is very
suitable for the complex clock configs of qca8084 PHY, i also provides
some descriptions of the qca8084 PHY, it will be very welcome to
review the clock DT model of qca8084 PHY.

Re: [PATCH v4 0/5] support ipq5332 platform

[Andrew Lunn]

On Fri, Jan 05, 2024 at 04:48:31AM +0200, Sergey Ryazanov wrote:
> Hi Luo,
> 
> thank you for explaining the case in such details. I also have checked the
> related DTSs in the Linaro repository to be more familiar with the I/O mem
> layout. Specifically I checked these two, hope they are relevant to the
> discussion:
> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> 
> Please find my comments below.

Hi Sergey

There is a second thread going on, focused around the quad PHY. See:

https://lore.kernel.org/netdev/60b9081c-76fa-4122-b7ae-5c3dcf7229f9@lunn.ch/

Since it is very hard to get consistent information out of Luo, he has
annoyed nearly all the PHY maintainers and all the DT maintainers, i'm
going back to baby steps, focusing on just the quad pure PHY, and
trying to get that understood and correctly described in DT.

However, does Linaro have any interest in just taking over this work,
or mentoring Luo?

	  Andrew

Re: [PATCH v4 0/5] support ipq5332 platform

[Alex Elder]

On 1/5/24 7:52 AM, Andrew Lunn wrote:
> On Fri, Jan 05, 2024 at 04:48:31AM +0200, Sergey Ryazanov wrote:
>> Hi Luo,
>>
>> thank you for explaining the case in such details. I also have checked the
>> related DTSs in the Linaro repository to be more familiar with the I/O mem
>> layout. Specifically I checked these two, hope they are relevant to the
>> discussion:
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>
>> Please find my comments below.
> 
> Hi Sergey
> 
> There is a second thread going on, focused around the quad PHY. See:
> 
> https://lore.kernel.org/netdev/60b9081c-76fa-4122-b7ae-5c3dcf7229f9@lunn.ch/
> 
> Since it is very hard to get consistent information out of Luo, he has
> annoyed nearly all the PHY maintainers and all the DT maintainers, i'm
> going back to baby steps, focusing on just the quad pure PHY, and
> trying to get that understood and correctly described in DT.
> 
> However, does Linaro have any interest in just taking over this work,
> or mentoring Luo?

I will reach out to Qualcomm to discuss options here.  We can
certainly offer to mentor, and I think we might even be able to
take over the work.  But I won't be able to get any resolution
on this until next week.

Jie Luo, please hold off on further posts on this for a little
while.

I will report back once I've been able to discuss this with
folks at Qualcomm.

					-Alex



> 
> 	  Andrew
> 

Re: [PATCH v4 0/5] support ipq5332 platform

[Sergey Ryazanov]

Hi Andrew,

On 05.01.2024 15:52, Andrew Lunn wrote:
> On Fri, Jan 05, 2024 at 04:48:31AM +0200, Sergey Ryazanov wrote:
>> Hi Luo,
>>
>> thank you for explaining the case in such details. I also have checked the
>> related DTSs in the Linaro repository to be more familiar with the I/O mem
>> layout. Specifically I checked these two, hope they are relevant to the
>> discussion:
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>
>> Please find my comments below.
> 
> Hi Sergey
> 
> There is a second thread going on, focused around the quad PHY. See:
> 
> https://lore.kernel.org/netdev/60b9081c-76fa-4122-b7ae-5c3dcf7229f9@lunn.ch/

Yeah. I had read your discussion yesterday before coming back to this 
clock discussion. It is a monster chip and looks like you have a hard 
time figuring out how it works and looking for a good code/DT model.

> Since it is very hard to get consistent information out of Luo, he has
> annoyed nearly all the PHY maintainers and all the DT maintainers, i'm
> going back to baby steps, focusing on just the quad pure PHY, and
> trying to get that understood and correctly described in DT.
> 
> However, does Linaro have any interest in just taking over this work,
> or mentoring Luo?

I should clarify here a bit. I found this discussion while looking for a 
way to port one open source firmware to my router based on previous IPQ 
generation. And since I am a bit familiar with this chip family, I chose 
to put my 2c to make implementation discussion more structured. Long 
story short, I have no idea about Linaro's plans :)

If I am allowed to speak, the chosen baby steps approach to focus on 
pure PHY seems to be the only sane method in that case. Considering 
Alex's promise, we can assume that the next release will support this PHY.

--
Sergey

Re: [PATCH v4 0/5] support ipq5332 platform

[Jie Luo]

On 1/6/2024 4:14 AM, Sergey Ryazanov wrote:
> Hi Andrew,
> 
> On 05.01.2024 15:52, Andrew Lunn wrote:
>> On Fri, Jan 05, 2024 at 04:48:31AM +0200, Sergey Ryazanov wrote:
>>> Hi Luo,
>>>
>>> thank you for explaining the case in such details. I also have 
>>> checked the
>>> related DTSs in the Linaro repository to be more familiar with the 
>>> I/O mem
>>> layout. Specifically I checked these two, hope they are relevant to the
>>> discussion:
>>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>
>>> Please find my comments below.
>>
>> Hi Sergey
>>
>> There is a second thread going on, focused around the quad PHY. See:
>>
>> https://lore.kernel.org/netdev/60b9081c-76fa-4122-b7ae-5c3dcf7229f9@lunn.ch/
> 
> Yeah. I had read your discussion yesterday before coming back to this 
> clock discussion. It is a monster chip and looks like you have a hard 
> time figuring out how it works and looking for a good code/DT model.

qca8084 is indeed a little complex, unlike other qcom PHY chips, qca8084
also includes the integrated clock controller that generates the
different clocks for the link of quad PHYs, which leads to some
misunderstanding of the clocks and resets used by qca8084.

i will refer to Christian's code and base on that to propose the DT
model of qca8084 for the review.

I am really sorry for the annoyance and misunderstanding caused by my
patches and replies.

> 
>> Since it is very hard to get consistent information out of Luo, he has
>> annoyed nearly all the PHY maintainers and all the DT maintainers, i'm
>> going back to baby steps, focusing on just the quad pure PHY, and
>> trying to get that understood and correctly described in DT.
>>
>> However, does Linaro have any interest in just taking over this work,
>> or mentoring Luo?
> 
> I should clarify here a bit. I found this discussion while looking for a 
> way to port one open source firmware to my router based on previous IPQ 
> generation. And since I am a bit familiar with this chip family, I chose 
> to put my 2c to make implementation discussion more structured. Long 
> story short, I have no idea about Linaro's plans :)
> 
> If I am allowed to speak, the chosen baby steps approach to focus on 
> pure PHY seems to be the only sane method in that case. Considering 
> Alex's promise, we can assume that the next release will support this PHY.
> 
> -- 
> Sergey

Thanks for help and guidance.