[PATCH 0/9] add MDIO changes on ipq5332 platform

[PATCH 0/9] add MDIO changes on ipq5332 platform

[Luo Jie]

For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
connected with one of them, qca8084 supports to customize the
MDIO address by configuring security register, which also
includes GCC module configurable.

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

2. GCC uniphy AHB/SYS clocks need to be configured and the 
ethernet LDO needs be enabled to make the GPIO reset of phy
taking effect.

3. The MDIO address of qca8084 PHY can be programed with any
customized value by configuring the security register which
is accessed by the special MDIO sequence.

4. Before the qca8084 PHY probeable by MDIO bus, the related
clocks and reset sequence should be completed.

5. Add the example MDIO device tree node for IPQ5018.

Luo Jie (9):
  net: mdio: ipq4019: increase eth_ldo_rdy for ipq5332 platform
  net: mdio: ipq4019: Enable the clocks for ipq5332 platform
  net: mdio: ipq4019: Enable GPIO reset for ipq5332 platform
  net: mdio: ipq4019: configure CMN PLL clock for ipq5332
  net: mdio: ipq4019: support MDIO clock frequency divider
  net: mdio: ipq4019: Support qca8084 switch register access
  net: mdio: ipq4019: program phy address when "fixup" defined
  net: mdio: ipq4019: add qca8084 configurations
  dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

 .../bindings/net/qcom,ipq4019-mdio.yaml       | 138 ++++-
 drivers/net/mdio/mdio-ipq4019.c               | 557 +++++++++++++++++-
 2 files changed, 656 insertions(+), 39 deletions(-)


base-commit: bc962b35b139dd52319e6fc0f4bab00593bf38c9
-- 
2.42.0

[PATCH 1/9] net: mdio: ipq4019: increase eth_ldo_rdy for ipq5332 platform

[Luo Jie]

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 | 55 +++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index abd8b508ec16..9d444f5f7efb 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -18,28 +18,31 @@
 #define MDIO_DATA_WRITE_REG			0x48
 #define MDIO_DATA_READ_REG			0x4c
 #define MDIO_CMD_REG				0x50
-#define MDIO_CMD_ACCESS_BUSY		BIT(16)
-#define MDIO_CMD_ACCESS_START		BIT(8)
-#define MDIO_CMD_ACCESS_CODE_READ	0
-#define MDIO_CMD_ACCESS_CODE_WRITE	1
-#define MDIO_CMD_ACCESS_CODE_C45_ADDR	0
-#define MDIO_CMD_ACCESS_CODE_C45_WRITE	1
-#define MDIO_CMD_ACCESS_CODE_C45_READ	2
+#define MDIO_CMD_ACCESS_BUSY			BIT(16)
+#define MDIO_CMD_ACCESS_START			BIT(8)
+#define MDIO_CMD_ACCESS_CODE_READ		0
+#define MDIO_CMD_ACCESS_CODE_WRITE		1
+#define MDIO_CMD_ACCESS_CODE_C45_ADDR		0
+#define MDIO_CMD_ACCESS_CODE_C45_WRITE		1
+#define MDIO_CMD_ACCESS_CODE_C45_READ		2
 
 /* 0 = Clause 22, 1 = Clause 45 */
 #define MDIO_MODE_C45				BIT(8)
 
-#define IPQ4019_MDIO_TIMEOUT	10000
-#define IPQ4019_MDIO_SLEEP		10
+#define IPQ4019_MDIO_TIMEOUT			10000
+#define IPQ4019_MDIO_SLEEP			10
 
 /* MDIO clock source frequency is fixed to 100M */
-#define IPQ_MDIO_CLK_RATE	100000000
+#define IPQ_MDIO_CLK_RATE			100000000
 
-#define IPQ_PHY_SET_DELAY_US	100000
+#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;
 };
 
@@ -210,13 +213,15 @@ static int ipq_mdio_reset(struct mii_bus *bus)
 	int ret;
 
 	/* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
-	 * is specified in the device tree.
+	 * or more resource are 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);
+	for (ret = 0; ret < ETH_LDO_RDY_CNT; ret++) {
+		if (priv->eth_ldo_rdy[ret]) {
+			val = readl(priv->eth_ldo_rdy[ret]);
+			val |= BIT(0);
+			writel(val, priv->eth_ldo_rdy[ret]);
+			fsleep(IPQ_PHY_SET_DELAY_US);
+		}
 	}
 
 	/* Configure MDIO clock source frequency if clock is specified in the device tree */
@@ -252,11 +257,14 @@ 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 */
+	/* The platform resource is provided on the chipset IPQ5018/IPQ5332 */
 	/* This resource is optional */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (res)
-		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
+	for (ret = 0; ret < ETH_LDO_RDY_CNT; ret++) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, ret + 1);
+		if (res)
+			priv->eth_ldo_rdy[ret] = devm_ioremap(&pdev->dev,
+							      res->start, resource_size(res));
+	}
 
 	bus->name = "ipq4019_mdio";
 	bus->read = ipq4019_mdio_read_c22;
@@ -288,6 +296,7 @@ static void ipq4019_mdio_remove(struct platform_device *pdev)
 static const struct of_device_id ipq4019_mdio_dt_ids[] = {
 	{ .compatible = "qcom,ipq4019-mdio" },
 	{ .compatible = "qcom,ipq5018-mdio" },
+	{ .compatible = "qcom,ipq5332-mdio" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ipq4019_mdio_dt_ids);
-- 
2.42.0

Re: [PATCH 1/9] net: mdio: ipq4019: increase eth_ldo_rdy for ipq5332 platform

[Andrew Lunn]

> +	for (ret = 0; ret < ETH_LDO_RDY_CNT; ret++) {
> +		if (priv->eth_ldo_rdy[ret]) {
> +			val = readl(priv->eth_ldo_rdy[ret]);
> +			val |= BIT(0);
> +			writel(val, priv->eth_ldo_rdy[ret]);
> +			fsleep(IPQ_PHY_SET_DELAY_US);
> +		}

Please add a new variable, rather than use ret this way.

> +	for (ret = 0; ret < ETH_LDO_RDY_CNT; ret++) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, ret + 1);
> +		if (res)
> +			priv->eth_ldo_rdy[ret] = devm_ioremap(&pdev->dev,

same here.

    Andrew

---
pw-bot: cr

Re: [PATCH 1/9] net: mdio: ipq4019: increase eth_ldo_rdy for ipq5332 platform

[Jie Luo]

On 11/15/2023 9:44 PM, Andrew Lunn wrote:
>> +	for (ret = 0; ret < ETH_LDO_RDY_CNT; ret++) {
>> +		if (priv->eth_ldo_rdy[ret]) {
>> +			val = readl(priv->eth_ldo_rdy[ret]);
>> +			val |= BIT(0);
>> +			writel(val, priv->eth_ldo_rdy[ret]);
>> +			fsleep(IPQ_PHY_SET_DELAY_US);
>> +		}
> 
> Please add a new variable, rather than use ret this way.

OK, will add it in the next patch set.

> 
>> +	for (ret = 0; ret < ETH_LDO_RDY_CNT; ret++) {
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, ret + 1);
>> +		if (res)
>> +			priv->eth_ldo_rdy[ret] = devm_ioremap(&pdev->dev,
> 
> same here.

Ok.

> 
>      Andrew
> 
> ---
> pw-bot: cr

Re: [PATCH 1/9] net: mdio: ipq4019: increase eth_ldo_rdy for ipq5332 platform

[Krzysztof Kozlowski]

On 15/11/2023 04:25, Luo Jie wrote:
> 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 | 55 +++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
> index abd8b508ec16..9d444f5f7efb 100644
> --- a/drivers/net/mdio/mdio-ipq4019.c
> +++ b/drivers/net/mdio/mdio-ipq4019.c
> @@ -18,28 +18,31 @@
>  #define MDIO_DATA_WRITE_REG			0x48
>  #define MDIO_DATA_READ_REG			0x4c
>  #define MDIO_CMD_REG				0x50
> -#define MDIO_CMD_ACCESS_BUSY		BIT(16)
> -#define MDIO_CMD_ACCESS_START		BIT(8)
> -#define MDIO_CMD_ACCESS_CODE_READ	0
> -#define MDIO_CMD_ACCESS_CODE_WRITE	1
> -#define MDIO_CMD_ACCESS_CODE_C45_ADDR	0
> -#define MDIO_CMD_ACCESS_CODE_C45_WRITE	1
> -#define MDIO_CMD_ACCESS_CODE_C45_READ	2
> +#define MDIO_CMD_ACCESS_BUSY			BIT(16)
> +#define MDIO_CMD_ACCESS_START			BIT(8)
> +#define MDIO_CMD_ACCESS_CODE_READ		0
> +#define MDIO_CMD_ACCESS_CODE_WRITE		1
> +#define MDIO_CMD_ACCESS_CODE_C45_ADDR		0
> +#define MDIO_CMD_ACCESS_CODE_C45_WRITE		1
> +#define MDIO_CMD_ACCESS_CODE_C45_READ		2

Where is anything related to ipq5332 here?


..

>  	bus->name = "ipq4019_mdio";
>  	bus->read = ipq4019_mdio_read_c22;
> @@ -288,6 +296,7 @@ static void ipq4019_mdio_remove(struct platform_device *pdev)
>  static const struct of_device_id ipq4019_mdio_dt_ids[] = {
>  	{ .compatible = "qcom,ipq4019-mdio" },
>  	{ .compatible = "qcom,ipq5018-mdio" },
> +	{ .compatible = "qcom,ipq5332-mdio" },

How user comes before binding?

Best regards,
Krzysztof

Re: [PATCH 1/9] net: mdio: ipq4019: increase eth_ldo_rdy for ipq5332 platform

[Jie Luo]

On 11/16/2023 7:57 PM, Krzysztof Kozlowski wrote:
> On 15/11/2023 04:25, Luo Jie wrote:
>> 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 | 55 +++++++++++++++++++--------------
>>   1 file changed, 32 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
>> index abd8b508ec16..9d444f5f7efb 100644
>> --- a/drivers/net/mdio/mdio-ipq4019.c
>> +++ b/drivers/net/mdio/mdio-ipq4019.c
>> @@ -18,28 +18,31 @@
>>   #define MDIO_DATA_WRITE_REG			0x48
>>   #define MDIO_DATA_READ_REG			0x4c
>>   #define MDIO_CMD_REG				0x50
>> -#define MDIO_CMD_ACCESS_BUSY		BIT(16)
>> -#define MDIO_CMD_ACCESS_START		BIT(8)
>> -#define MDIO_CMD_ACCESS_CODE_READ	0
>> -#define MDIO_CMD_ACCESS_CODE_WRITE	1
>> -#define MDIO_CMD_ACCESS_CODE_C45_ADDR	0
>> -#define MDIO_CMD_ACCESS_CODE_C45_WRITE	1
>> -#define MDIO_CMD_ACCESS_CODE_C45_READ	2
>> +#define MDIO_CMD_ACCESS_BUSY			BIT(16)
>> +#define MDIO_CMD_ACCESS_START			BIT(8)
>> +#define MDIO_CMD_ACCESS_CODE_READ		0
>> +#define MDIO_CMD_ACCESS_CODE_WRITE		1
>> +#define MDIO_CMD_ACCESS_CODE_C45_ADDR		0
>> +#define MDIO_CMD_ACCESS_CODE_C45_WRITE		1
>> +#define MDIO_CMD_ACCESS_CODE_C45_READ		2
> 
> Where is anything related to ipq5332 here?

This is for alignment format, will keep it untouched in the next
patch set.

> 
> 
> ..
> 
>>   	bus->name = "ipq4019_mdio";
>>   	bus->read = ipq4019_mdio_read_c22;
>> @@ -288,6 +296,7 @@ static void ipq4019_mdio_remove(struct platform_device *pdev)
>>   static const struct of_device_id ipq4019_mdio_dt_ids[] = {
>>   	{ .compatible = "qcom,ipq4019-mdio" },
>>   	{ .compatible = "qcom,ipq5018-mdio" },
>> +	{ .compatible = "qcom,ipq5332-mdio" },
> 
> How user comes before binding?

The new added compatible is for the GCC uniphy AHB/SYS clocks configured
on the ipq5332 platform, will move this change into the following patch
that involves the ipq5332 to make it clear.

<net: mdio: ipq4019: Enable the clocks for ipq5332 platform>.

> 
> Best regards,
> Krzysztof
> 

[PATCH 2/9] net: mdio: ipq4019: Enable the clocks for ipq5332 platform

[Luo Jie]

For the platform ipq5332, the related GCC clocks need to be enabled
to make the GPIO reset of the MDIO slave devices taking effect.

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 9d444f5f7efb..a77982a1a1e1 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -34,16 +34,35 @@
 
 /* MDIO clock source frequency is fixed to 100M */
 #define IPQ_MDIO_CLK_RATE			100000000
+#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];
+};
+
+const char *const mdio_clk_name[] = {
+	"gcc_mdio_ahb_clk",
+	"gcc_uniphy0_ahb_clk",
+	"gcc_uniphy0_sys_clk",
+	"gcc_uniphy1_ahb_clk",
+	"gcc_uniphy1_sys_clk"
 };
 
 static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
@@ -212,6 +231,38 @@ static int ipq_mdio_reset(struct mii_bus *bus)
 	u32 val;
 	int ret;
 
+	/* For the platform ipq5332, there are two uniphy available to connect the
+	 * ethernet devices, the uniphy gcc clock should be enabled for resetting
+	 * the connected device such as qca8386 switch or qca8081 PHY effectively.
+	 */
+	if (of_device_is_compatible(bus->parent->of_node, "qcom,ipq5332-mdio")) {
+		int i;
+		unsigned long rate = 0;
+
+		for (i = MDIO_CLK_UNIPHY0_AHB; i < MDIO_CLK_CNT; i++) {
+			switch (i) {
+			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[i], rate);
+			if (ret)
+				return ret;
+
+			ret = clk_prepare_enable(priv->clk[i]);
+			if (ret)
+				return ret;
+		}
+	}
+
 	/* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
 	 * or more resource are specified in the device tree.
 	 */
@@ -225,11 +276,11 @@ static int ipq_mdio_reset(struct mii_bus *bus)
 	}
 
 	/* Configure MDIO clock source frequency if clock is specified in the device tree */
-	ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
+	ret = clk_set_rate(priv->clk[MDIO_CLK_MDIO_AHB], IPQ_MDIO_CLK_RATE);
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(priv->mdio_clk);
+	ret = clk_prepare_enable(priv->clk[MDIO_CLK_MDIO_AHB]);
 	if (ret == 0)
 		mdelay(10);
 
@@ -253,10 +304,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);
-
 	/* The platform resource is provided on the chipset IPQ5018/IPQ5332 */
 	/* This resource is optional */
 	for (ret = 0; ret < ETH_LDO_RDY_CNT; ret++) {
@@ -266,6 +313,12 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 							      res->start, resource_size(res));
 	}
 
+	for (ret = 0; ret < MDIO_CLK_CNT; ret++) {
+		priv->clk[ret] = devm_clk_get_optional(&pdev->dev, mdio_clk_name[ret]);
+		if (IS_ERR(priv->clk[ret]))
+			return PTR_ERR(priv->clk[ret]);
+	}
+
 	bus->name = "ipq4019_mdio";
 	bus->read = ipq4019_mdio_read_c22;
 	bus->write = ipq4019_mdio_write_c22;
-- 
2.42.0

Re: [PATCH 2/9] net: mdio: ipq4019: Enable the clocks for ipq5332 platform

[Konrad Dybcio]

On 15.11.2023 04:25, Luo Jie wrote:
> For the platform ipq5332, the related GCC clocks need to be enabled
> to make the GPIO reset of the MDIO slave devices taking effect.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
[...]

>  static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
> @@ -212,6 +231,38 @@ static int ipq_mdio_reset(struct mii_bus *bus)
>  	u32 val;
>  	int ret;
>  
> +	/* For the platform ipq5332, there are two uniphy available to connect the
> +	 * ethernet devices, the uniphy gcc clock should be enabled for resetting
> +	 * the connected device such as qca8386 switch or qca8081 PHY effectively.
> +	 */
> +	if (of_device_is_compatible(bus->parent->of_node, "qcom,ipq5332-mdio")) {
Would that not also be taken care of in the phy driver?

Konrad

Re: [PATCH 2/9] net: mdio: ipq4019: Enable the clocks for ipq5332 platform

[Jie Luo]

On 11/20/2023 10:22 PM, Konrad Dybcio wrote:
> On 15.11.2023 04:25, Luo Jie wrote:
>> For the platform ipq5332, the related GCC clocks need to be enabled
>> to make the GPIO reset of the MDIO slave devices taking effect.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> [...]
> 
>>   static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
>> @@ -212,6 +231,38 @@ static int ipq_mdio_reset(struct mii_bus *bus)
>>   	u32 val;
>>   	int ret;
>>   
>> +	/* For the platform ipq5332, there are two uniphy available to connect the
>> +	 * ethernet devices, the uniphy gcc clock should be enabled for resetting
>> +	 * the connected device such as qca8386 switch or qca8081 PHY effectively.
>> +	 */
>> +	if (of_device_is_compatible(bus->parent->of_node, "qcom,ipq5332-mdio")) {
> Would that not also be taken care of in the phy driver?
> 
> Konrad

Hi Konrad,
These clocks are the SOC clocks that is not related to the PHY type.
no matter what kind of PHY is connected, we also need to configure
these clocks.

Re: [PATCH 2/9] net: mdio: ipq4019: Enable the clocks for ipq5332 platform

[Andrew Lunn]

On Tue, Nov 21, 2023 at 06:28:54PM +0800, Jie Luo wrote:
> 
> 
> On 11/20/2023 10:22 PM, Konrad Dybcio wrote:
> > On 15.11.2023 04:25, Luo Jie wrote:
> > > For the platform ipq5332, the related GCC clocks need to be enabled
> > > to make the GPIO reset of the MDIO slave devices taking effect.
> > > 
> > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > [...]
> > 
> > >   static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
> > > @@ -212,6 +231,38 @@ static int ipq_mdio_reset(struct mii_bus *bus)
> > >   	u32 val;
> > >   	int ret;
> > > +	/* For the platform ipq5332, there are two uniphy available to connect the
> > > +	 * ethernet devices, the uniphy gcc clock should be enabled for resetting
> > > +	 * the connected device such as qca8386 switch or qca8081 PHY effectively.
> > > +	 */
> > > +	if (of_device_is_compatible(bus->parent->of_node, "qcom,ipq5332-mdio")) {
> > Would that not also be taken care of in the phy driver?
> > 
> > Konrad
> 
> Hi Konrad,
> These clocks are the SOC clocks that is not related to the PHY type.
> no matter what kind of PHY is connected, we also need to configure
> these clocks.

Hi Jie

You can avoid lots of these questions by making your commit message
better. Assume the reader does not know the clock tree for this
device. With a bit of experience, you can guess what reviewers are
going to ask, and answer those questions in the commit message.

      Andrew

Re: [PATCH 2/9] net: mdio: ipq4019: Enable the clocks for ipq5332 platform

[Jie Luo]

On 11/21/2023 10:04 PM, Andrew Lunn wrote:
> On Tue, Nov 21, 2023 at 06:28:54PM +0800, Jie Luo wrote:
>>
>>
>> On 11/20/2023 10:22 PM, Konrad Dybcio wrote:
>>> On 15.11.2023 04:25, Luo Jie wrote:
>>>> For the platform ipq5332, the related GCC clocks need to be enabled
>>>> to make the GPIO reset of the MDIO slave devices taking effect.
>>>>
>>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>> [...]
>>>
>>>>    static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
>>>> @@ -212,6 +231,38 @@ static int ipq_mdio_reset(struct mii_bus *bus)
>>>>    	u32 val;
>>>>    	int ret;
>>>> +	/* For the platform ipq5332, there are two uniphy available to connect the
>>>> +	 * ethernet devices, the uniphy gcc clock should be enabled for resetting
>>>> +	 * the connected device such as qca8386 switch or qca8081 PHY effectively.
>>>> +	 */
>>>> +	if (of_device_is_compatible(bus->parent->of_node, "qcom,ipq5332-mdio")) {
>>> Would that not also be taken care of in the phy driver?
>>>
>>> Konrad
>>
>> Hi Konrad,
>> These clocks are the SOC clocks that is not related to the PHY type.
>> no matter what kind of PHY is connected, we also need to configure
>> these clocks.
> 
> Hi Jie
> 
> You can avoid lots of these questions by making your commit message
> better. Assume the reader does not know the clock tree for this
> device. With a bit of experience, you can guess what reviewers are
> going to ask, and answer those questions in the commit message.
> 
>        Andrew

Hi Andrew,
Got it, will take more attention on the commit message to make the
code clearly in future patches, thanks for the suggestion.

Re: [PATCH 2/9] net: mdio: ipq4019: Enable the clocks for ipq5332 platform

[Simon Horman]

On Wed, Nov 15, 2023 at 11:25:08AM +0800, Luo Jie wrote:
> For the platform ipq5332, the related GCC clocks need to be enabled
> to make the GPIO reset of the MDIO slave devices taking effect.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  drivers/net/mdio/mdio-ipq4019.c | 67 +++++++++++++++++++++++++++++----
>  1 file changed, 60 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
> index 9d444f5f7efb..a77982a1a1e1 100644
> --- a/drivers/net/mdio/mdio-ipq4019.c
> +++ b/drivers/net/mdio/mdio-ipq4019.c

...

> +const char *const mdio_clk_name[] = {
> +	"gcc_mdio_ahb_clk",
> +	"gcc_uniphy0_ahb_clk",
> +	"gcc_uniphy0_sys_clk",
> +	"gcc_uniphy1_ahb_clk",
> +	"gcc_uniphy1_sys_clk"
>  };

Hi Luo Jie,

A minor nit from my side: It appears that mdio_clk_name is only
used in this file. If so it should be declared as static.

...

Re: [PATCH 2/9] net: mdio: ipq4019: Enable the clocks for ipq5332 platform

[Jie Luo]

On 11/27/2023 5:37 PM, Simon Horman wrote:
> On Wed, Nov 15, 2023 at 11:25:08AM +0800, Luo Jie wrote:
>> For the platform ipq5332, the related GCC clocks need to be enabled
>> to make the GPIO reset of the MDIO slave devices taking effect.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   drivers/net/mdio/mdio-ipq4019.c | 67 +++++++++++++++++++++++++++++----
>>   1 file changed, 60 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
>> index 9d444f5f7efb..a77982a1a1e1 100644
>> --- a/drivers/net/mdio/mdio-ipq4019.c
>> +++ b/drivers/net/mdio/mdio-ipq4019.c
> 
> ...
> 
>> +const char *const mdio_clk_name[] = {
>> +	"gcc_mdio_ahb_clk",
>> +	"gcc_uniphy0_ahb_clk",
>> +	"gcc_uniphy0_sys_clk",
>> +	"gcc_uniphy1_ahb_clk",
>> +	"gcc_uniphy1_sys_clk"
>>   };
> 
> Hi Luo Jie,
> 
> A minor nit from my side: It appears that mdio_clk_name is only
> used in this file. If so it should be declared as static.
> 
> ...

Hi Simon,
The mdio_clk_name is only used in this file, will update to use
static, thanks.

[PATCH 3/9] net: mdio: ipq4019: Enable GPIO reset for ipq5332 platform

[Luo Jie]

Before doing GPIO reset on the MDIO slave devices, the common clock
output to MDIO slave device should be enabled, and the related GCC
clocks also need to be configured.

Because of these extra configurations, the MDIO bus level GPIO and
PHY device level GPIO can't be leveraged. Need to add the device
tree property "phy-reset-gpio" of MDIO node to enable this special
GPIO reset.

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index a77982a1a1e1..93ae4684de31 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -12,6 +12,7 @@
 #include <linux/phy.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
+#include <linux/gpio/consumer.h>
 
 #define MDIO_MODE_REG				0x40
 #define MDIO_ADDR_REG				0x44
@@ -55,6 +56,7 @@ struct ipq4019_mdio_data {
 	void __iomem *membase;
 	void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
 	struct clk *clk[MDIO_CLK_CNT];
+	struct gpio_descs *reset_gpios;
 };
 
 const char *const mdio_clk_name[] = {
@@ -275,6 +277,24 @@ static int ipq_mdio_reset(struct mii_bus *bus)
 		}
 	}
 
+	/* Do the optional reset on the devices connected with MDIO bus */
+	if (priv->reset_gpios) {
+		unsigned long *values = bitmap_zalloc(priv->reset_gpios->ndescs, GFP_KERNEL);
+
+		if (!values)
+			return -ENOMEM;
+
+		bitmap_fill(values, priv->reset_gpios->ndescs);
+		gpiod_set_array_value_cansleep(priv->reset_gpios->ndescs, priv->reset_gpios->desc,
+					       priv->reset_gpios->info, values);
+
+		fsleep(IPQ_PHY_SET_DELAY_US);
+		bitmap_zero(values, priv->reset_gpios->ndescs);
+		gpiod_set_array_value_cansleep(priv->reset_gpios->ndescs, priv->reset_gpios->desc,
+					       priv->reset_gpios->info, values);
+		bitmap_free(values);
+	}
+
 	/* Configure MDIO clock source frequency if clock is specified in the device tree */
 	ret = clk_set_rate(priv->clk[MDIO_CLK_MDIO_AHB], IPQ_MDIO_CLK_RATE);
 	if (ret)
@@ -319,6 +339,19 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 			return PTR_ERR(priv->clk[ret]);
 	}
 
+	/* This GPIO reset is for qca8084 PHY, which is only probeable by MDIO bus
+	 * after the following steps completed.
+	 *
+	 * 1. Enable LDO to provide clock for qca8084 and enable SoC GCC uniphy related clocks.
+	 * 2. Do GPIO reset on the qca8084 PHY.
+	 * 3. Configure the PHY address that is customized according to device treee.
+	 * 4. Configure the related qca8084 GCC clock & reset.
+	 */
+	priv->reset_gpios = devm_gpiod_get_array_optional(&pdev->dev, "phy-reset", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->reset_gpios))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->reset_gpios),
+				     "mii_bus %s couldn't get reset GPIO\n", bus->id);
+
 	bus->name = "ipq4019_mdio";
 	bus->read = ipq4019_mdio_read_c22;
 	bus->write = ipq4019_mdio_write_c22;
-- 
2.42.0

Re: [PATCH 3/9] net: mdio: ipq4019: Enable GPIO reset for ipq5332 platform

[Andrew Lunn]

On Wed, Nov 15, 2023 at 11:25:09AM +0800, Luo Jie wrote:
> Before doing GPIO reset on the MDIO slave devices, the common clock
> output to MDIO slave device should be enabled, and the related GCC
> clocks also need to be configured.
> 
> Because of these extra configurations, the MDIO bus level GPIO and
> PHY device level GPIO can't be leveraged.

Its not clear to me why the normal reset cannot be used. The MBIO bus
driver can probe, setup the clocks, and then register the MDIO bus to
the core. The core can then use the GPIO resets.

What am i missing?

     Andrew

Re: [PATCH 3/9] net: mdio: ipq4019: Enable GPIO reset for ipq5332 platform

[Jie Luo]

On 11/15/2023 11:11 PM, Andrew Lunn wrote:
> On Wed, Nov 15, 2023 at 11:25:09AM +0800, Luo Jie wrote:
>> Before doing GPIO reset on the MDIO slave devices, the common clock
>> output to MDIO slave device should be enabled, and the related GCC
>> clocks also need to be configured.
>>
>> Because of these extra configurations, the MDIO bus level GPIO and
>> PHY device level GPIO can't be leveraged.
> 
> Its not clear to me why the normal reset cannot be used. The MBIO bus
> driver can probe, setup the clocks, and then register the MDIO bus to
> the core. The core can then use the GPIO resets.
> 
> What am i missing?
> 
>       Andrew

Hi Andrew,
Looks we can leverage the MDIO bus GPIO to reset qca8084 PHY, but the
mdio bus gpio only supports one GPIO number.

Here are the reasons i put the GPIO reset here.
1. Currently one MDIO bus instance only connects one qca8084 PHY as
MDIO slave device on IPQ5332 platform, since the MDIO address
occupied by qca8084. if the other type PHY also needs to use MDIO
bus GPIO reset, then we can't cover this case.

2. Before doing the GPIO reset on qca8084, we need to enable the clock
output to qca8084 by configuring eth_ldo_rdy register, and the mdio
bus->reset is called after the mdio bus level reset.

3. program the mdio address of qca8084 PHY and the initialization
configurations needed before the registers of qca8084 can be accessed.
if we take the PHY level GPIO reset for qca8084, there is no call point
to do the initialization configurations and programing PHY address in
the MDIO driver code.

i will check the feasibility of taking the PHY level GPIO reset and do
the initial configurations in the PHY probe function.

FYI, here is the sequence to bring up qca8084.
a. enable clock output to qca8084.
b. do gpio reset of qca8084.
c. customize MDIO address and initialization configurations.
d. the PHY ID can be acquired.


Thanks,
Jie.

Re: [PATCH 3/9] net: mdio: ipq4019: Enable GPIO reset for ipq5332 platform

[Robert Marko]

On Thu, Nov 16, 2023 at 12:14 PM Jie Luo <quic_luoj@quicinc.com> wrote:
>
>
>
> On 11/15/2023 11:11 PM, Andrew Lunn wrote:
> > On Wed, Nov 15, 2023 at 11:25:09AM +0800, Luo Jie wrote:
> >> Before doing GPIO reset on the MDIO slave devices, the common clock
> >> output to MDIO slave device should be enabled, and the related GCC
> >> clocks also need to be configured.
> >>
> >> Because of these extra configurations, the MDIO bus level GPIO and
> >> PHY device level GPIO can't be leveraged.
> >
> > Its not clear to me why the normal reset cannot be used. The MBIO bus
> > driver can probe, setup the clocks, and then register the MDIO bus to
> > the core. The core can then use the GPIO resets.
> >
> > What am i missing?
> >
> >       Andrew
>
> Hi Andrew,
> Looks we can leverage the MDIO bus GPIO to reset qca8084 PHY, but the
> mdio bus gpio only supports one GPIO number.

But, you can specify a PHY specific reset-gpio under the PHY subnode.
However, you must specify the PHY ID via compatible then, please look at:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/net/ethernet-phy.yaml?h=next-20231116#n36

I do this commonly when there are multiple reset GPIO-s for different ethernet
PHY-s.

Regards,
Robert
>
> Here are the reasons i put the GPIO reset here.
> 1. Currently one MDIO bus instance only connects one qca8084 PHY as
> MDIO slave device on IPQ5332 platform, since the MDIO address
> occupied by qca8084. if the other type PHY also needs to use MDIO
> bus GPIO reset, then we can't cover this case.
>
> 2. Before doing the GPIO reset on qca8084, we need to enable the clock
> output to qca8084 by configuring eth_ldo_rdy register, and the mdio
> bus->reset is called after the mdio bus level reset.
>
> 3. program the mdio address of qca8084 PHY and the initialization
> configurations needed before the registers of qca8084 can be accessed.
> if we take the PHY level GPIO reset for qca8084, there is no call point
> to do the initialization configurations and programing PHY address in
> the MDIO driver code.
>
> i will check the feasibility of taking the PHY level GPIO reset and do
> the initial configurations in the PHY probe function.
>
> FYI, here is the sequence to bring up qca8084.
> a. enable clock output to qca8084.
> b. do gpio reset of qca8084.
> c. customize MDIO address and initialization configurations.
> d. the PHY ID can be acquired.
>
>
> Thanks,
> Jie.



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH 3/9] net: mdio: ipq4019: Enable GPIO reset for ipq5332 platform

[Jie Luo]

On 11/16/2023 7:19 PM, Robert Marko wrote:
> On Thu, Nov 16, 2023 at 12:14 PM Jie Luo <quic_luoj@quicinc.com> wrote:
>>
>>
>>
>> On 11/15/2023 11:11 PM, Andrew Lunn wrote:
>>> On Wed, Nov 15, 2023 at 11:25:09AM +0800, Luo Jie wrote:
>>>> Before doing GPIO reset on the MDIO slave devices, the common clock
>>>> output to MDIO slave device should be enabled, and the related GCC
>>>> clocks also need to be configured.
>>>>
>>>> Because of these extra configurations, the MDIO bus level GPIO and
>>>> PHY device level GPIO can't be leveraged.
>>>
>>> Its not clear to me why the normal reset cannot be used. The MBIO bus
>>> driver can probe, setup the clocks, and then register the MDIO bus to
>>> the core. The core can then use the GPIO resets.
>>>
>>> What am i missing?
>>>
>>>        Andrew
>>
>> Hi Andrew,
>> Looks we can leverage the MDIO bus GPIO to reset qca8084 PHY, but the
>> mdio bus gpio only supports one GPIO number.
> 
> But, you can specify a PHY specific reset-gpio under the PHY subnode.
> However, you must specify the PHY ID via compatible then, please look at:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/net/ethernet-phy.yaml?h=next-20231116#n36
> 
> I do this commonly when there are multiple reset GPIO-s for different ethernet
> PHY-s.
> 
> Regards,
> Robert

Got it, thanks Robert for the information, i will try the GPIO reset of
PHY DT node, and update it in the next patch set.

>>
>> Here are the reasons i put the GPIO reset here.
>> 1. Currently one MDIO bus instance only connects one qca8084 PHY as
>> MDIO slave device on IPQ5332 platform, since the MDIO address
>> occupied by qca8084. if the other type PHY also needs to use MDIO
>> bus GPIO reset, then we can't cover this case.
>>
>> 2. Before doing the GPIO reset on qca8084, we need to enable the clock
>> output to qca8084 by configuring eth_ldo_rdy register, and the mdio
>> bus->reset is called after the mdio bus level reset.
>>
>> 3. program the mdio address of qca8084 PHY and the initialization
>> configurations needed before the registers of qca8084 can be accessed.
>> if we take the PHY level GPIO reset for qca8084, there is no call point
>> to do the initialization configurations and programing PHY address in
>> the MDIO driver code.
>>
>> i will check the feasibility of taking the PHY level GPIO reset and do
>> the initial configurations in the PHY probe function.
>>
>> FYI, here is the sequence to bring up qca8084.
>> a. enable clock output to qca8084.
>> b. do gpio reset of qca8084.
>> c. customize MDIO address and initialization configurations.
>> d. the PHY ID can be acquired.
>>
>>
>> Thanks,
>> Jie.
> 
> 
> 

Re: [PATCH 3/9] net: mdio: ipq4019: Enable GPIO reset for ipq5332 platform

[Andrew Lunn]

> FYI, here is the sequence to bring up qca8084.
> a. enable clock output to qca8084.
> b. do gpio reset of qca8084.
> c. customize MDIO address and initialization configurations.
> d. the PHY ID can be acquired.

This all sounds like it is specific to the qca8084, so it should be in
the driver for the qca8084.

Its been pointed out you can get the driver to load by using the PHY
ID in the compatible. You want the SoC clock driver to export a CCF
clock, which the PHY driver can use. The PHY driver should also be
able to get the GPIO. So i think the PHY driver can do all this.

     Andrew

Re: [PATCH 3/9] net: mdio: ipq4019: Enable GPIO reset for ipq5332 platform

[Jie Luo]

On 11/17/2023 1:20 AM, Andrew Lunn wrote:
>> FYI, here is the sequence to bring up qca8084.
>> a. enable clock output to qca8084.
>> b. do gpio reset of qca8084.
>> c. customize MDIO address and initialization configurations.
>> d. the PHY ID can be acquired.
> 
> This all sounds like it is specific to the qca8084, so it should be in
> the driver for the qca8084.
> 
> Its been pointed out you can get the driver to load by using the PHY
> ID in the compatible. You want the SoC clock driver to export a CCF
> clock, which the PHY driver can use. The PHY driver should also be
> able to get the GPIO. So i think the PHY driver can do all this.
> 
>       Andrew

Yes, Andrew, that is feasible, i will update the patches to move the
initialized clock configs in the PHY probe function.

Re: [PATCH 3/9] net: mdio: ipq4019: Enable GPIO reset for ipq5332 platform

[Jie Luo]

On 11/17/2023 1:20 AM, Andrew Lunn wrote:
>> FYI, here is the sequence to bring up qca8084.
>> a. enable clock output to qca8084.
>> b. do gpio reset of qca8084.
>> c. customize MDIO address and initialization configurations.
>> d. the PHY ID can be acquired.
> 
> This all sounds like it is specific to the qca8084, so it should be in
> the driver for the qca8084.
> 
> Its been pointed out you can get the driver to load by using the PHY
> ID in the compatible. You want the SoC clock driver to export a CCF
> clock, which the PHY driver can use. The PHY driver should also be
> able to get the GPIO. So i think the PHY driver can do all this.
> 
>       Andrew

Hi Andrew,
If i put the GPIO reset in the PHY device tree node, the PHY probe
function will be postponed to be called instead of being called
during the MDIO bus register, which leads to the PCS can't be
created correctly because of reading PHY capability failed before
the PHY probe function called.

my device tree nodes are as below.

ethernet_device {
	phy-handle = <&phy3>;
	phy-mode = "2500base-x";
	...
};

mdio@90000 {
	phy3: ethernet-phy@3 {
		compatible = "ethernet-phy-id004d.d180";
		reg = <4>;
		reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>;
		reset-assert-us = <100000>;
		reset-deassert-us = <100000>;
		clocks = <...>;
		clock-names = "...";
	};
};

Since the PHY probe function of phy3 is postponed instead of
called during the MDIO bus driver register, and the initialization
of qca8084 is not called when the ethernet_device driver is called
to create PCS, where the phy3 capability is checked, which is failed
since the qca8084 PHY probe is not called.

Any idea to resolve this call sequence issue?
Thanks.

[PATCH 4/9] 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 X 25MHZ
and 3 x 50MHZ output clocks available.

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 93ae4684de31..ca9cda98d1f8 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -43,6 +43,13 @@
 /* Maximum SOC PCS(uniphy) number on IPQ platform */
 #define ETH_LDO_RDY_CNT				3
 
+#define CMN_PLL_REFERENCE_CLOCK			0x784
+#define CMN_PLL_REFCLK_INDEX			GENMASK(3, 0)
+#define CMN_PLL_REFCLK_EXTERNAL			BIT(9)
+
+#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,
@@ -54,6 +61,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];
 	struct gpio_descs *reset_gpios;
@@ -227,12 +235,73 @@ 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 "cmn_ref_clk", the internal 48MHZ is used by default
+ * on the ipq533 platform.
+ *
+ * The output clock of CMN PLL block is provided to the MDIO slave devices,
+ * threre are 4 CMN PLL output clocks (1x25MHZ + 3x50MHZ) enabled by default.
+ *
+ * such as the output 50M clock for the qca8084 PHY.
+ */
+static void ipq_cmn_clock_config(struct mii_bus *bus)
+{
+	u32 reg_val;
+	const char *cmn_ref_clk;
+	struct ipq4019_mdio_data *priv = bus->priv;
+
+	if (priv && priv->cmn_membase) {
+		reg_val = readl(priv->cmn_membase + CMN_PLL_REFERENCE_CLOCK);
+		reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_INDEX);
+
+		/* Select reference clock source */
+		cmn_ref_clk = of_get_property(bus->parent->of_node, "cmn_ref_clk", NULL);
+		if (!cmn_ref_clk) {
+			/* Internal 48MHZ selected by default */
+			reg_val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
+		} else {
+			if (!strcmp(cmn_ref_clk, "external_25MHz"))
+				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 3));
+			else if (!strcmp(cmn_ref_clk, "external_31250KHz"))
+				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 4));
+			else if (!strcmp(cmn_ref_clk, "external_40MHz"))
+				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 6));
+			else if (!strcmp(cmn_ref_clk, "external_48MHz"))
+				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7));
+			else if (!strcmp(cmn_ref_clk, "external_50MHz"))
+				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8));
+			else
+				reg_val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
+		}
+
+		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);
+	}
+}
+
 static int ipq_mdio_reset(struct mii_bus *bus)
 {
 	struct ipq4019_mdio_data *priv = bus->priv;
 	u32 val;
 	int ret;
 
+	ipq_cmn_clock_config(bus);
+
 	/* For the platform ipq5332, there are two uniphy available to connect the
 	 * ethernet devices, the uniphy gcc clock should be enabled for resetting
 	 * the connected device such as qca8386 switch or qca8081 PHY effectively.
@@ -328,11 +397,21 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 	/* This resource is optional */
 	for (ret = 0; ret < ETH_LDO_RDY_CNT; ret++) {
 		res = platform_get_resource(pdev, IORESOURCE_MEM, ret + 1);
-		if (res)
+		if (res && strcmp(res->name, "cmn_blk"))
 			priv->eth_ldo_rdy[ret] = devm_ioremap(&pdev->dev,
 							      res->start, resource_size(res));
 	}
 
+	/* The CMN block resource is for providing clock source of 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 (ret = 0; ret < MDIO_CLK_CNT; ret++) {
 		priv->clk[ret] = devm_clk_get_optional(&pdev->dev, mdio_clk_name[ret]);
 		if (IS_ERR(priv->clk[ret]))
-- 
2.42.0

Re: [PATCH 4/9] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

[Andrew Lunn]

> +static void ipq_cmn_clock_config(struct mii_bus *bus)
> +{
> +	u32 reg_val;
> +	const char *cmn_ref_clk;
> +	struct ipq4019_mdio_data *priv = bus->priv;

Reverse christmass tree place.

> +
> +	if (priv && priv->cmn_membase) {

Can priv be NULL? Can cmn_membase be NULL?

> +		reg_val = readl(priv->cmn_membase + CMN_PLL_REFERENCE_CLOCK);
> +		reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_INDEX);
> +
> +		/* Select reference clock source */
> +		cmn_ref_clk = of_get_property(bus->parent->of_node, "cmn_ref_clk", NULL);
> +		if (!cmn_ref_clk) {
> +			/* Internal 48MHZ selected by default */
> +			reg_val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
> +		} else {
> +			if (!strcmp(cmn_ref_clk, "external_25MHz"))

Not strings, please use u32 values. You can then list the valid values
in the yaml file, and get te tools to verify the DT.

> +				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
> +					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 3));
> +			else if (!strcmp(cmn_ref_clk, "external_31250KHz"))
> +				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
> +					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 4));
> +			else if (!strcmp(cmn_ref_clk, "external_40MHz"))
> +				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
> +					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 6));
> +			else if (!strcmp(cmn_ref_clk, "external_48MHz"))
> +				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
> +					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7));
> +			else if (!strcmp(cmn_ref_clk, "external_50MHz"))
> +				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
> +					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8));
> +			else
> +				reg_val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);

If the value is not valid, return -EINVAL.

   Andrew

Re: [PATCH 4/9] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

[Jie Luo]

On 11/15/2023 11:19 PM, Andrew Lunn wrote:
>> +static void ipq_cmn_clock_config(struct mii_bus *bus)
>> +{
>> +	u32 reg_val;
>> +	const char *cmn_ref_clk;
>> +	struct ipq4019_mdio_data *priv = bus->priv;
> 
> Reverse christmass tree place.

will fix it in the next patch set.

> 
>> +
>> +	if (priv && priv->cmn_membase) {
> 
> Can priv be NULL? Can cmn_membase be NULL?

priv can't be NULL, cmn_membase is optional, the legacy chip does not
provide the cmn_membase in device node.

will remove the priv check here.

> 
>> +		reg_val = readl(priv->cmn_membase + CMN_PLL_REFERENCE_CLOCK);
>> +		reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_INDEX);
>> +
>> +		/* Select reference clock source */
>> +		cmn_ref_clk = of_get_property(bus->parent->of_node, "cmn_ref_clk", NULL);
>> +		if (!cmn_ref_clk) {
>> +			/* Internal 48MHZ selected by default */
>> +			reg_val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
>> +		} else {
>> +			if (!strcmp(cmn_ref_clk, "external_25MHz"))
> 
> Not strings, please use u32 values. You can then list the valid values
> in the yaml file, and get te tools to verify the DT.

will update this in the next patch.

> 
>> +				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
>> +					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 3));
>> +			else if (!strcmp(cmn_ref_clk, "external_31250KHz"))
>> +				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
>> +					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 4));
>> +			else if (!strcmp(cmn_ref_clk, "external_40MHz"))
>> +				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
>> +					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 6));
>> +			else if (!strcmp(cmn_ref_clk, "external_48MHz"))
>> +				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
>> +					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7));
>> +			else if (!strcmp(cmn_ref_clk, "external_50MHz"))
>> +				reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
>> +					    FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8));
>> +			else
>> +				reg_val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
> 
> If the value is not valid, return -EINVAL.

will add it in the next patch set.

> 
>     Andrew

Re: [PATCH 4/9] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

[Konrad Dybcio]

On 11/15/23 04:25, 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 X 25MHZ
> and 3 x 50MHZ output clocks available.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>   drivers/net/mdio/mdio-ipq4019.c | 81 ++++++++++++++++++++++++++++++++-
>   1 file changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
> index 93ae4684de31..ca9cda98d1f8 100644
> --- a/drivers/net/mdio/mdio-ipq4019.c
> +++ b/drivers/net/mdio/mdio-ipq4019.c
> @@ -43,6 +43,13 @@
>   /* Maximum SOC PCS(uniphy) number on IPQ platform */
>   #define ETH_LDO_RDY_CNT				3
>   
> +#define CMN_PLL_REFERENCE_CLOCK			0x784
> +#define CMN_PLL_REFCLK_INDEX			GENMASK(3, 0)
> +#define CMN_PLL_REFCLK_EXTERNAL			BIT(9)
> +
> +#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,
> @@ -54,6 +61,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];
>   	struct gpio_descs *reset_gpios;
> @@ -227,12 +235,73 @@ 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 "cmn_ref_clk", the internal 48MHZ is used by default
> + * on the ipq533 platform.
> + *
> + * The output clock of CMN PLL block is provided to the MDIO slave devices,
> + * threre are 4 CMN PLL output clocks (1x25MHZ + 3x50MHZ) enabled by default.
> + *
> + * such as the output 50M clock for the qca8084 PHY.
> + */
> +static void ipq_cmn_clock_config(struct mii_bus *bus)
> +{
> +	u32 reg_val;
> +	const char *cmn_ref_clk;
> +	struct ipq4019_mdio_data *priv = bus->priv;
> +
> +	if (priv && priv->cmn_membase) {
> +		reg_val = readl(priv->cmn_membase + CMN_PLL_REFERENCE_CLOCK);
> +		reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_INDEX);
> +
> +		/* Select reference clock source */
> +		cmn_ref_clk = of_get_property(bus->parent->of_node, "cmn_ref_clk", NULL);
> +		if (!cmn_ref_clk) {
> +			/* Internal 48MHZ selected by default */
> +			reg_val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
> +		} else {
> +			if (!strcmp(cmn_ref_clk, "external_25MHz"))
As pointed out by others, such string properties won't go through

Konrad

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

[Luo Jie]

The MDIO clock frequency can be divided according to the
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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index ca9cda98d1f8..44a8a866f8ee 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -30,6 +30,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
 
@@ -65,6 +68,7 @@ struct ipq4019_mdio_data {
 	void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
 	struct clk *clk[MDIO_CLK_CNT];
 	struct gpio_descs *reset_gpios;
+	int clk_div;
 };
 
 const char *const mdio_clk_name[] = {
@@ -98,6 +102,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);
 
@@ -139,6 +144,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);
 
@@ -171,6 +177,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);
 
@@ -214,6 +221,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);
 
@@ -431,6 +439,9 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(priv->reset_gpios),
 				     "mii_bus %s couldn't get reset GPIO\n", bus->id);
 
+	/* MDIO default frequency is 6.25MHz */
+	priv->clk_div = 0xf;
+
 	bus->name = "ipq4019_mdio";
 	bus->read = ipq4019_mdio_read_c22;
 	bus->write = ipq4019_mdio_write_c22;
-- 
2.42.0

Re: [PATCH 5/9] net: mdio: ipq4019: support MDIO clock frequency divider

[Andrew Lunn]

> +	/* MDIO default frequency is 6.25MHz */
> +	priv->clk_div = 0xf;

802.3 says MDC should be 2.5Mhz. Its O.K. to support faster clocks,
but it should be an optional DT property, clock-frequency as described
in the binding.

   Andrew

Re: [PATCH 5/9] net: mdio: ipq4019: support MDIO clock frequency divider

[Jie Luo]

On 11/15/2023 11:22 PM, Andrew Lunn wrote:
>> +	/* MDIO default frequency is 6.25MHz */
>> +	priv->clk_div = 0xf;
> 
> 802.3 says MDC should be 2.5Mhz. Its O.K. to support faster clocks,
> but it should be an optional DT property, clock-frequency as described
> in the binding.
> 
>     Andrew

Ok, Andrew, will add the DT property "clock-frequency" to support this
clock divider in the next patch set, thanks.

[PATCH 6/9] net: mdio: ipq4019: Support qca8084 switch register access

[Luo Jie]

For qca8084 chip, there are GCC, TLMM and security control
modules besides the PHY, these moudles are accessed with 32
bits value, which has the special MDIO sequences to read or
write this 32bit register.

There are initial configurations needed to make qca8084 PHY
probeable, and the PHY address of qca8084 can also be customized
before creating the PHY device on MDIO bus register, all these
configurations are located in switch modules that are accessed
by the 32bit register.

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 44a8a866f8ee..8dc611666c34 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -53,6 +53,14 @@
 #define CMN_PLL_POWER_ON_AND_RESET		0x780
 #define CMN_ANA_EN_SW_RSTN			BIT(6)
 
+/* QCA8084 includes the PHY chip, GCC/TLMM and the control modules,
+ * except for the PHY register, other registers are accessed by MDIO bus
+ * with 32bit value, which has the special MDIO sequences to access the
+ * switch modules register.
+ */
+#define IPQ_HIGH_ADDR_PREFIX			0x18
+#define IPQ_LOW_ADDR_PREFIX			0x10
+
 enum mdio_clk_id {
 	MDIO_CLK_MDIO_AHB,
 	MDIO_CLK_UNIPHY0_AHB,
@@ -243,6 +251,72 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
 	return 0;
 }
 
+static inline void split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page, u16 *sw_addr)
+{
+	*r1 = regaddr & 0x1c;
+
+	regaddr >>= 5;
+	*r2 = regaddr & 0x7;
+
+	regaddr >>= 3;
+	*page = regaddr & 0xffff;
+
+	regaddr >>= 16;
+	*sw_addr = regaddr & 0xff;
+}
+
+static int qca8084_set_page(struct mii_bus *bus, u16 sw_addr, u16 page)
+{
+	return bus->write(bus, IPQ_HIGH_ADDR_PREFIX | (sw_addr >> 5), sw_addr & 0x1f, page);
+}
+
+static int qca8084_mii_read(struct mii_bus *bus, u16 addr, u16 reg, u32 *val)
+{
+	int ret, data;
+
+	ret = bus->read(bus, addr, reg);
+	if (ret >= 0) {
+		data = ret;
+
+		ret = bus->read(bus, addr, reg | BIT(1));
+		if (ret >= 0)
+			*val =  data | ret << 16;
+	}
+
+	return ret < 0 ? ret : 0;
+}
+
+static int qca8084_mii_write(struct mii_bus *bus, u16 addr, u16 reg, u32 val)
+{
+	int ret;
+
+	ret = bus->write(bus, addr, reg, lower_16_bits(val));
+	if (!ret)
+		ret = bus->write(bus, addr, reg | BIT(1), upper_16_bits(val));
+
+	return ret;
+}
+
+static int qca8084_modify(struct mii_bus *bus, u32 regaddr, u32 clear, u32 set)
+{
+	u16 reg, addr, page, sw_addr;
+	u32 val;
+	int ret;
+
+	split_addr(regaddr, &reg, &addr, &page, &sw_addr);
+	ret = qca8084_set_page(bus, sw_addr, page);
+	if (ret < 0)
+		return ret;
+
+	ret = qca8084_mii_read(bus, IPQ_LOW_ADDR_PREFIX | addr, reg, &val);
+	if (ret < 0)
+		return ret;
+
+	val &= ~clear;
+	val |= set;
+	return qca8084_mii_write(bus, IPQ_LOW_ADDR_PREFIX | addr, reg, val);
+};
+
 /* For the CMN PLL block, the reference clock can be configured according to
  * the device tree property "cmn_ref_clk", the internal 48MHZ is used by default
  * on the ipq533 platform.
-- 
2.42.0

[PATCH 7/9] net: mdio: ipq4019: program phy address when "fixup" defined

[Luo Jie]

The PHY/PCS MDIO address can be programed when the property
"fixup" of phy node is defined.

The qca8084 PHY/PCS address configuration register is accessed
by MDIO bus with the special MDIO sequence.

The PHY address configuration register of IPQ5018 is accessed
by local bus.

Add the function ipq_mdio_preinit, which should be called before
the PHY device scanned and registered.

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 8dc611666c34..1c461c243ae0 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -61,6 +61,15 @@
 #define IPQ_HIGH_ADDR_PREFIX			0x18
 #define IPQ_LOW_ADDR_PREFIX			0x10
 
+/* QCA8084 PHY & PCS address can be customized, 4 PHYs and 3 PCSs are
+ * available.
+ */
+#define QCA8084_PHY_ADDR_LENGTH			5
+#define QCA8084_PHY_ADDR_NUM			4
+#define QCA8084_PCS_ADDR_NUM			3
+#define QCA8084_PHY_ADDR_MASK			GENMASK(19, 0)
+#define QCA8084_PCS_ADDR_MASK			GENMASK(14, 0)
+
 enum mdio_clk_id {
 	MDIO_CLK_MDIO_AHB,
 	MDIO_CLK_UNIPHY0_AHB,
@@ -317,6 +326,102 @@ static int qca8084_modify(struct mii_bus *bus, u32 regaddr, u32 clear, u32 set)
 	return qca8084_mii_write(bus, IPQ_LOW_ADDR_PREFIX | addr, reg, val);
 };
 
+/* The PHY/PCS MDIO address can be programed when the device tree property
+ * "fixup" of PHY node is specified.
+ */
+static int ipq_phy_addr_fixup(struct mii_bus *bus, struct device_node *mdio_node)
+{
+	const __be32 *phy_cfg;
+	u32 phy_addr_val, pcs_addr_val;
+	int ret, phy_index, pcs_index;
+	struct device_node *child;
+
+	phy_index = 0;
+	pcs_index = 0;
+	phy_addr_val = 0;
+	pcs_addr_val = 0;
+	for_each_available_child_of_node(mdio_node, child) {
+		ret = of_mdio_parse_addr(&bus->dev, child);
+		if (ret < 0)
+			continue;
+
+		if (!of_property_present(child, "fixup"))
+			continue;
+
+		if (of_property_present(child, "compatible")) {
+			pcs_addr_val |= ret << (QCA8084_PHY_ADDR_LENGTH * pcs_index);
+			pcs_index++;
+		} else {
+			phy_addr_val |= ret << (QCA8084_PHY_ADDR_LENGTH * phy_index);
+			phy_index++;
+		}
+	}
+
+	if (!phy_addr_val && !pcs_addr_val)
+		return 0;
+
+	if (phy_index > QCA8084_PHY_ADDR_NUM || pcs_index > QCA8084_PCS_ADDR_NUM) {
+		dev_err(&bus->dev,
+			"Too many MDIO address(phy number %d, pcs number %d) to be programed\n",
+			phy_index, pcs_index);
+		return -1;
+	}
+
+	phy_cfg = of_get_property(mdio_node, "phyaddr-fixup", &ret);
+
+	/* For MDIO access, phyaddr-fixup only provides the register address,
+	 * such as qca8084 PHY.
+	 *
+	 * As for local bus, the register length also needs to be provided,
+	 * such as the internal PHY of IPQ5018, only PHY address can be programed.
+	 */
+	if (!phy_cfg || (ret != (2 * sizeof(__be32)) && ret != sizeof(__be32)))
+		return 0;
+
+	if (ret == sizeof(__be32)) {
+		const __be32 *pcs_cfg;
+
+		/* MDIO access for customizing PHY address of qca8084 */
+		if (phy_addr_val != 0) {
+			ret = qca8084_modify(bus, be32_to_cpup(phy_cfg),
+					     QCA8084_PHY_ADDR_MASK, phy_addr_val);
+			if (ret)
+				return ret;
+		}
+
+		pcs_cfg = of_get_property(mdio_node, "pcsaddr-fixup", NULL);
+		/* Programe the PCS address if pcsaddr-fixup specified */
+		if (pcs_cfg && pcs_addr_val != 0) {
+			ret = qca8084_modify(bus, be32_to_cpup(pcs_cfg),
+					     QCA8084_PCS_ADDR_MASK, pcs_addr_val);
+			if (ret)
+				return ret;
+		}
+	} else {
+		void __iomem *ephy_cfg_base;
+
+		/* Local bus access for customizing internal PHY address of IPQ5018 */
+		ephy_cfg_base = ioremap(be32_to_cpup(phy_cfg), be32_to_cpup(phy_cfg + 1));
+		if (!ephy_cfg_base)
+			return -ENOMEM;
+
+		if (phy_addr_val != 0)
+			writel(phy_addr_val, ephy_cfg_base);
+	}
+
+	return 0;
+}
+
+static int ipq_mdio_preinit(struct mii_bus *bus)
+{
+	struct device_node *mdio_node = dev_of_node(&bus->dev);
+
+	if (!mdio_node)
+		return 0;
+
+	return ipq_phy_addr_fixup(bus, mdio_node);
+}
+
 /* For the CMN PLL block, the reference clock can be configured according to
  * the device tree property "cmn_ref_clk", the internal 48MHZ is used by default
  * on the ipq533 platform.
@@ -455,7 +560,7 @@ static int ipq_mdio_reset(struct mii_bus *bus)
 	if (ret == 0)
 		mdelay(10);
 
-	return ret;
+	return ipq_mdio_preinit(bus);
 }
 
 static int ipq4019_mdio_probe(struct platform_device *pdev)
-- 
2.42.0

Re: [PATCH 7/9] net: mdio: ipq4019: program phy address when "fixup" defined

[Andrew Lunn]

On Wed, Nov 15, 2023 at 11:25:13AM +0800, Luo Jie wrote:
> The PHY/PCS MDIO address can be programed when the property
> "fixup" of phy node is defined.
> 
> The qca8084 PHY/PCS address configuration register is accessed
> by MDIO bus with the special MDIO sequence.
> 
> The PHY address configuration register of IPQ5018 is accessed
> by local bus.
> 
> Add the function ipq_mdio_preinit, which should be called before
> the PHY device scanned and registered.

I'm not convinced this belongs in the MDIO bus driver. Its really a
PHY property, so i think all this should be in the PHY driver. If you
specify the PHY ID in the compatible string, you can get the driver
loaded and the probe function called. You can then set the PHY
address.

	Andrew

Re: [PATCH 7/9] net: mdio: ipq4019: program phy address when "fixup" defined

[Jie Luo]

On 11/16/2023 12:17 AM, Andrew Lunn wrote:
> On Wed, Nov 15, 2023 at 11:25:13AM +0800, Luo Jie wrote:
>> The PHY/PCS MDIO address can be programed when the property
>> "fixup" of phy node is defined.
>>
>> The qca8084 PHY/PCS address configuration register is accessed
>> by MDIO bus with the special MDIO sequence.
>>
>> The PHY address configuration register of IPQ5018 is accessed
>> by local bus.
>>
>> Add the function ipq_mdio_preinit, which should be called before
>> the PHY device scanned and registered.
> 
> I'm not convinced this belongs in the MDIO bus driver. Its really a
> PHY property, so i think all this should be in the PHY driver. If you
> specify the PHY ID in the compatible string, you can get the driver
> loaded and the probe function called. You can then set the PHY
> address.
> 
> 	Andrew

I will try to do the initialization configs in the PHY probe function,
Thanks Andrew for the suggestions.

[PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Luo Jie]

The PHY & PCS clocks need to be enabled and the reset
sequence needs to be completed to make qca8084 PHY
probeable by MDIO bus.

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 1c461c243ae0..9bdd49be2361 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -70,6 +70,30 @@
 #define QCA8084_PHY_ADDR_MASK			GENMASK(19, 0)
 #define QCA8084_PCS_ADDR_MASK			GENMASK(14, 0)
 
+/* QCA8084 GCC & security control registers */
+/* LDO control, BIT20 for PHY0 and PHY1, BIT21 for PHY2 and PHY3 */
+#define EPHY_CFG				0xC90F018
+#define EPHY_CFG_LDO_CTRL			GENMASK(21, 20)
+
+/* GEPHY TX&RX clock control register starts from GEPHY0_TX,
+ * end with GEPHY3_RX, the gap is 0x20.
+ */
+#define GEPHY0_TX_CBCR				0xC800058
+#define GEPHY3_RX_CBCR				0xC800138
+#define GEPHY_CBCR_GAP				0x20
+
+#define SRDS0_SYS_CBCR				0xC8001A8
+#define SRDS1_SYS_CBCR				0xC8001AC
+#define EPHY0_SYS_CBCR				0xC8001B0
+#define EPHY1_SYS_CBCR				0xC8001B4
+#define EPHY2_SYS_CBCR				0xC8001B8
+#define EPHY3_SYS_CBCR				0xC8001BC
+#define CLK_EN					BIT(0)
+#define CLK_RESET				BIT(2)
+
+#define GCC_GEPHY_MISC				0xC800304
+#define GCC_GEPHY_MISC_DSP_RESET		GENMASK(4, 0)
+
 enum mdio_clk_id {
 	MDIO_CLK_MDIO_AHB,
 	MDIO_CLK_UNIPHY0_AHB,
@@ -412,14 +436,121 @@ static int ipq_phy_addr_fixup(struct mii_bus *bus, struct device_node *mdio_node
 	return 0;
 }
 
+static inline int qca8084_clock_en_set(struct mii_bus *bus, u32 reg, bool enable)
+{
+	return qca8084_modify(bus, reg, CLK_EN, enable ? CLK_EN : 0);
+}
+
+static inline int qca8084_clock_reset(struct mii_bus *bus, u32 reg)
+{
+	int ret;
+
+	ret = qca8084_modify(bus, reg, CLK_RESET, CLK_RESET);
+	if (ret)
+		return ret;
+
+	usleep_range(20000, 21000);
+	return qca8084_modify(bus, reg, CLK_RESET, 0);
+}
+
+static int qca8084_clock_config(struct mii_bus *bus)
+{
+	u32 reg;
+	int ret;
+
+	/* Enable PCS */
+	ret = qca8084_clock_en_set(bus, SRDS0_SYS_CBCR, true);
+	if (ret)
+		return ret;
+
+	ret = qca8084_clock_en_set(bus, SRDS1_SYS_CBCR, true);
+	if (ret)
+		return ret;
+
+	/* Reset PCS */
+	ret = qca8084_clock_reset(bus, SRDS0_SYS_CBCR);
+	if (ret)
+		return ret;
+
+	ret = qca8084_clock_reset(bus, SRDS1_SYS_CBCR);
+	if (ret)
+		return ret;
+
+	/* Disable EPHY GMII RX & TX clock */
+	reg = GEPHY0_TX_CBCR;
+	while (reg <= GEPHY3_RX_CBCR) {
+		ret = qca8084_clock_en_set(bus, reg, false);
+		if (ret)
+			return ret;
+
+		reg += GEPHY_CBCR_GAP;
+	}
+
+	/* Enable EPHY */
+	ret = qca8084_clock_en_set(bus, EPHY0_SYS_CBCR, true);
+	if (ret)
+		return ret;
+
+	ret = qca8084_clock_en_set(bus, EPHY1_SYS_CBCR, true);
+	if (ret)
+		return ret;
+
+	ret = qca8084_clock_en_set(bus, EPHY2_SYS_CBCR, true);
+	if (ret)
+		return ret;
+
+	ret = qca8084_clock_en_set(bus, EPHY3_SYS_CBCR, true);
+	if (ret)
+		return ret;
+
+	/* Reset EPHY */
+	ret = qca8084_clock_reset(bus, EPHY0_SYS_CBCR);
+	if (ret)
+		return ret;
+
+	ret = qca8084_clock_reset(bus, EPHY1_SYS_CBCR);
+	if (ret)
+		return ret;
+
+	ret = qca8084_clock_reset(bus, EPHY2_SYS_CBCR);
+	if (ret)
+		return ret;
+
+	ret = qca8084_clock_reset(bus, EPHY3_SYS_CBCR);
+	if (ret)
+		return ret;
+
+	/* Deassert EPHY DSP */
+	ret = qca8084_modify(bus, GCC_GEPHY_MISC, GCC_GEPHY_MISC_DSP_RESET, 0);
+	if (ret)
+		return ret;
+
+	/* Enable efuse loading into analog circuit */
+	ret = qca8084_modify(bus, EPHY_CFG, EPHY_CFG_LDO_CTRL, 0);
+	if (ret)
+		return ret;
+
+	/* Sleep 10ms */
+	usleep_range(10000, 11000);
+	return 0;
+}
+
 static int ipq_mdio_preinit(struct mii_bus *bus)
 {
+	int ret;
 	struct device_node *mdio_node = dev_of_node(&bus->dev);
 
 	if (!mdio_node)
 		return 0;
 
-	return ipq_phy_addr_fixup(bus, mdio_node);
+	ret = ipq_phy_addr_fixup(bus, mdio_node);
+	if (ret)
+		return ret;
+
+	if (of_property_read_bool(mdio_node, "mdio-clk-fixup"))
+		ret = qca8084_clock_config(bus);
+
+	return ret;
 }
 
 /* For the CMN PLL block, the reference clock can be configured according to
-- 
2.42.0

Re: [PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Andrew Lunn]

On Wed, Nov 15, 2023 at 11:25:14AM +0800, Luo Jie wrote:
> The PHY & PCS clocks need to be enabled and the reset
> sequence needs to be completed to make qca8084 PHY
> probeable by MDIO bus.

Is all this guaranteed to be the same between different boards? Can
the board be wired differently and need a different configuration?

    Andrew

Re: [PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Konrad Dybcio]

On 11/15/23 17:20, Andrew Lunn wrote:
> On Wed, Nov 15, 2023 at 11:25:14AM +0800, Luo Jie wrote:
>> The PHY & PCS clocks need to be enabled and the reset
>> sequence needs to be completed to make qca8084 PHY
>> probeable by MDIO bus.
> 
> Is all this guaranteed to be the same between different boards?
No, this looks like a total subsystem overreach, these should be
taken care of from within clk framework and consumed with the clk
APIs.

Konrad

Re: [PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Robert Marko]

On Wed, Nov 15, 2023 at 6:01 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 11/15/23 17:20, Andrew Lunn wrote:
> > On Wed, Nov 15, 2023 at 11:25:14AM +0800, Luo Jie wrote:
> >> The PHY & PCS clocks need to be enabled and the reset
> >> sequence needs to be completed to make qca8084 PHY
> >> probeable by MDIO bus.
> >
> > Is all this guaranteed to be the same between different boards?
> No, this looks like a total subsystem overreach, these should be
> taken care of from within clk framework and consumed with the clk
> APIs.
>
> Konrad

There are patches for QCA8084 clocks:
https://patchwork.kernel.org/project/linux-arm-msm/cover/20231104034858.9159-1-quic_luoj@quicinc.com/

I guess all of the clocking should be done there, it isn't really a MDIO issue.

Regards,
Robert

-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Jie Luo]

On 11/16/2023 1:03 AM, Robert Marko wrote:
> On Wed, Nov 15, 2023 at 6:01 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>>
>>
>> On 11/15/23 17:20, Andrew Lunn wrote:
>>> On Wed, Nov 15, 2023 at 11:25:14AM +0800, Luo Jie wrote:
>>>> The PHY & PCS clocks need to be enabled and the reset
>>>> sequence needs to be completed to make qca8084 PHY
>>>> probeable by MDIO bus.
>>>
>>> Is all this guaranteed to be the same between different boards?
>> No, this looks like a total subsystem overreach, these should be
>> taken care of from within clk framework and consumed with the clk
>> APIs.
>>
>> Konrad
> 
> There are patches for QCA8084 clocks:
> https://patchwork.kernel.org/project/linux-arm-msm/cover/20231104034858.9159-1-quic_luoj@quicinc.com/
> 
> I guess all of the clocking should be done there, it isn't really a MDIO issue.
> 
> Regards,
> Robert
> 

Thanks Robert for the link of this patch.

Yes, the clock driver of qca8084 is probed as the MDIO device, the
configuration sequence here to lighten the qca8084 PHY need to
be completed before the clock APIs available to call.

Re: [PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Andrew Lunn]

> Yes, the clock driver of qca8084 is probed as the MDIO device, the
> configuration sequence here to lighten the qca8084 PHY need to
> be completed before the clock APIs available to call.

Please cleanly separate clock from MDIO. The MDIO driver should only
use the common clock framework API calls. If the clock driver is not
loaded yet, trying to get a clock should return -EPROBE_DEFER. The
MDIO driver should return that from its probe function. The driver
core will then try to probe the MDIO driver later, by which time the
clock driver should of loaded.

      Andrew

Re: [PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Jie Luo]

On 11/17/2023 1:08 AM, Andrew Lunn wrote:
>> Yes, the clock driver of qca8084 is probed as the MDIO device, the
>> configuration sequence here to lighten the qca8084 PHY need to
>> be completed before the clock APIs available to call.
> 
> Please cleanly separate clock from MDIO. The MDIO driver should only
> use the common clock framework API calls. If the clock driver is not
> loaded yet, trying to get a clock should return -EPROBE_DEFER. The
> MDIO driver should return that from its probe function. The driver
> core will then try to probe the MDIO driver later, by which time the
> clock driver should of loaded.
> 
>        Andrew
> 

Ok, will update the patches to take this solution using the clock
consume APIs. Thanks Andrew for the suggestion.

Re: [PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Jie Luo]

On 11/16/2023 1:01 AM, Konrad Dybcio wrote:
> 
> 
> On 11/15/23 17:20, Andrew Lunn wrote:
>> On Wed, Nov 15, 2023 at 11:25:14AM +0800, Luo Jie wrote:
>>> The PHY & PCS clocks need to be enabled and the reset
>>> sequence needs to be completed to make qca8084 PHY
>>> probeable by MDIO bus.
>>
>> Is all this guaranteed to be the same between different boards?
> No, this looks like a total subsystem overreach, these should be
> taken care of from within clk framework and consumed with the clk
> APIs.
> 
> Konrad

Hi Konrad,
As Robert shared the link of the clock provider driver, which is
registered as MDIO device and not available until to the qca8084
initializations completed done here, so i need to do raw read/write
the clock registers in this patch.

Re: [PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Jie Luo]

On 11/16/2023 12:20 AM, Andrew Lunn wrote:
> On Wed, Nov 15, 2023 at 11:25:14AM +0800, Luo Jie wrote:
>> The PHY & PCS clocks need to be enabled and the reset
>> sequence needs to be completed to make qca8084 PHY
>> probeable by MDIO bus.
> 
> Is all this guaranteed to be the same between different boards? Can
> the board be wired differently and need a different configuration?
> 
>      Andrew

Hi Andrew,
This configuration sequence is specified to the qca8084 chip,
not related with the platform(such as ipq5332).

All these configured registers are located in qca8084 chip, we need
to complete these configurations to make MDIO bus being able to
scan the qca8084 PHY(PHY registers can be accessed).

Re: [PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Andrew Lunn]

On Thu, Nov 16, 2023 at 06:47:08PM +0800, Jie Luo wrote:
> 
> 
> On 11/16/2023 12:20 AM, Andrew Lunn wrote:
> > On Wed, Nov 15, 2023 at 11:25:14AM +0800, Luo Jie wrote:
> > > The PHY & PCS clocks need to be enabled and the reset
> > > sequence needs to be completed to make qca8084 PHY
> > > probeable by MDIO bus.
> > 
> > Is all this guaranteed to be the same between different boards? Can
> > the board be wired differently and need a different configuration?
> > 
> >      Andrew
> 
> Hi Andrew,
> This configuration sequence is specified to the qca8084 chip,
> not related with the platform(such as ipq5332).
> 
> All these configured registers are located in qca8084 chip, we need
> to complete these configurations to make MDIO bus being able to
> scan the qca8084 PHY(PHY registers can be accessed).

So nothing here has anything to do with the actual PHYs on the bus?
The only clock exposed here is MDC, and that runs at the standard
2.5MHz? All the clock tree configuration is completely internal to the
SOC?

What we don't want is some hard coded configuration which only works
for one specific reference design.

	Andrew

Re: [PATCH 8/9] net: mdio: ipq4019: add qca8084 configurations

[Jie Luo]

On 11/17/2023 1:12 AM, Andrew Lunn wrote:
> On Thu, Nov 16, 2023 at 06:47:08PM +0800, Jie Luo wrote:
>>
>>
>> On 11/16/2023 12:20 AM, Andrew Lunn wrote:
>>> On Wed, Nov 15, 2023 at 11:25:14AM +0800, Luo Jie wrote:
>>>> The PHY & PCS clocks need to be enabled and the reset
>>>> sequence needs to be completed to make qca8084 PHY
>>>> probeable by MDIO bus.
>>>
>>> Is all this guaranteed to be the same between different boards? Can
>>> the board be wired differently and need a different configuration?
>>>
>>>       Andrew
>>
>> Hi Andrew,
>> This configuration sequence is specified to the qca8084 chip,
>> not related with the platform(such as ipq5332).
>>
>> All these configured registers are located in qca8084 chip, we need
>> to complete these configurations to make MDIO bus being able to
>> scan the qca8084 PHY(PHY registers can be accessed).
> 
> So nothing here has anything to do with the actual PHYs on the bus?
> The only clock exposed here is MDC, and that runs at the standard
> 2.5MHz? All the clock tree configuration is completely internal to the
> SOC?
> 
> What we don't want is some hard coded configuration which only works
> for one specific reference design.
> 
> 	Andrew

These configured registers are related with PHYs, which is located in
the qca8084 PHY chip, qca8084 PHY chip includes the GCC register that
is not from the SOC(ipq5332), is a internal part of qca8084 PHY.

qca8084 PHY works on 6.25MHZ and other clock rates below 6.25MHZ.

will move these clock configurations using the clock APIs into the PHY
probe function in the next patch set, since it is the internal configs
of qca8084 PHY.

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

[Luo Jie]

On platform IPQ5332, the MDIO address of qca8084 can be programed
when the device tree property "fixup" defined, the clock sequence
needs to be completed before the PHY probeable.

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

diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
index 3407e909e8a7..7ff92be14ee1 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
@@ -15,11 +15,13 @@ properties:
       - enum:
           - qcom,ipq4019-mdio
           - qcom,ipq5018-mdio
+          - qcom,ipq5332-mdio
 
       - items:
           - enum:
               - qcom,ipq6018-mdio
               - qcom,ipq8074-mdio
+              - qcom,ipq9574-mdio
           - const: qcom,ipq4019-mdio
 
   "#address-cells":
@@ -30,19 +32,47 @@ properties:
 
   reg:
     minItems: 1
-    maxItems: 2
+    maxItems: 5
     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.
+      the first Address and length of the register set for the MDIO controller,
+      the optional second, third and fourth address and length of the register
+      for ethernet LDO, these three address range are required by the platform
+      IPQ50xx/IPQ5332, the last address and length is for the CMN clock to
+      select the reference clock.
+
+  reg-names:
+    minItems: 1
+    maxItems: 5
 
   clocks:
-    items:
-      - description: MDIO clock source frequency fixed to 100MHZ
+    minItems: 1
+    maxItems: 5
+    description:
+      MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy
+      clocks enabled for resetting ethernet PHY.
 
   clock-names:
-    items:
-      - const: gcc_mdio_ahb_clk
+    minItems: 1
+    maxItems: 5
+
+  phy-reset-gpio:
+    minItems: 1
+    maxItems: 3
+    description:
+      GPIO used to reset the PHY, each GPIO is for resetting the connected
+      ethernet PHY device.
+
+  phyaddr-fixup:
+    description: Register address for programing MDIO address of PHY devices
+
+  pcsaddr-fixup:
+    description: Register address for programing MDIO address of PCS devices
+
+  mdio-clk-fixup:
+    description: The initialization clocks to be configured
+
+  fixup:
+    description: The MDIO address of PHY/PCS device to be programed
 
 required:
   - compatible
@@ -61,6 +91,8 @@ allOf:
               - qcom,ipq5018-mdio
               - qcom,ipq6018-mdio
               - qcom,ipq8074-mdio
+              - qcom,ipq5332-mdio
+              - qcom,ipq9574-mdio
     then:
       required:
         - clocks
@@ -70,6 +102,29 @@ allOf:
         clocks: false
         clock-names: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq5332-mdio
+    then:
+      properties:
+        clocks:
+          items:
+            - description: MDIO clock source frequency fixed to 100MHZ
+            - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
+            - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
+            - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
+            - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
+        clock-names:
+          items:
+            - const: gcc_mdio_ahb_clk
+            - const: gcc_uniphy0_ahb_clk
+            - const: gcc_uniphy0_sys_clk
+            - const: gcc_uniphy1_ahb_clk
+            - const: gcc_uniphy1_sys_clk
+
 unevaluatedProperties: false
 
 examples:
@@ -100,3 +155,70 @@ 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";
+      reg = <0x90000 0x64>, <0x7A00610 0x4>, <0x7A10610 0x4>, <0x9B000 0x800>;
+      reg-names = "mdio", "eth_ldo1", "eth_ldo2", "cmn_blk";
+
+      clocks = <&gcc GCC_MDIO_AHB_CLK>,
+               <&gcc GCC_UNIPHY0_AHB_CLK>,
+               <&gcc GCC_UNIPHY0_SYS_CLK>,
+               <&gcc GCC_UNIPHY1_AHB_CLK>,
+               <&gcc GCC_UNIPHY1_SYS_CLK>;
+
+      clock-names = "gcc_mdio_ahb_clk",
+                    "gcc_uniphy0_ahb_clk",
+                    "gcc_uniphy0_sys_clk",
+                    "gcc_uniphy1_ahb_clk",
+                    "gcc_uniphy1_sys_clk";
+
+      phy-reset-gpio = <&tlmm 51 GPIO_ACTIVE_LOW>;
+      phyaddr-fixup = <0xC90F018>;
+      pcsaddr-fixup = <0xC90F014>;
+      mdio-clk-fixup;
+
+      qca8kphy0: ethernet-phy@1 {
+        reg = <1>;
+        fixup;
+      };
+
+      qca8kphy1: ethernet-phy@2 {
+        reg = <2>;
+        fixup;
+      };
+
+      qca8kphy2: ethernet-phy@3 {
+        reg = <3>;
+        fixup;
+      };
+
+      qca8kphy3: ethernet-phy@4 {
+        reg = <4>;
+        fixup;
+      };
+
+      qca8kpcs0: pcsphy0@5 {
+        compatible = "qcom,qca8k_pcs";
+        reg = <5>;
+        fixup;
+        };
+
+      qca8kpcs1: pcsphy1@6 {
+        compatible = "qcom,qca8k_pcs";
+        reg = <6>;
+        fixup;
+        };
+
+      qca8kxpcs: xpcsphy@7 {
+        compatible = "qcom,qca8k_pcs";
+        reg = <7>;
+        fixup;
+        };
+    };
-- 
2.42.0

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

[Rob Herring]

On Wed, 15 Nov 2023 11:25:15 +0800, Luo Jie wrote:
> On platform IPQ5332, the MDIO address of qca8084 can be programed
> when the device tree property "fixup" defined, the clock sequence
> needs to be completed before the PHY probeable.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  .../bindings/net/qcom,ipq4019-mdio.yaml       | 138 +++++++++++++++++-
>  1 file changed, 130 insertions(+), 8 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml: phyaddr-fixup: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml: pcsaddr-fixup: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml: mdio-clk-fixup: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml: fixup: missing type definition
Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.example.dtb: /example-1/mdio@90000/pcsphy0@5: failed to match any schema with compatible: ['qcom,qca8k_pcs']
Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.example.dtb: /example-1/mdio@90000/pcsphy1@6: failed to match any schema with compatible: ['qcom,qca8k_pcs']
Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.example.dtb: /example-1/mdio@90000/xpcsphy@7: failed to match any schema with compatible: ['qcom,qca8k_pcs']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231115032515.4249-10-quic_luoj@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Andrew Lunn]

> +  phy-reset-gpio:
> +    minItems: 1
> +    maxItems: 3
> +    description:
> +      GPIO used to reset the PHY, each GPIO is for resetting the connected
> +      ethernet PHY device.

This is a PHY property, so should be in the PHY node. phylib should
then take care of fit.

> +
> +  phyaddr-fixup:
> +    description: Register address for programing MDIO address of PHY devices

Please give a better description of this and the next two properties.

> +
> +  pcsaddr-fixup:
> +    description: Register address for programing MDIO address of PCS devices
> +
> +  mdio-clk-fixup:
> +    description: The initialization clocks to be configured
> +
> +  fixup:
> +    description: The MDIO address of PHY/PCS device to be programed
> +        clocks:
> +          items:
> +            - description: MDIO clock source frequency fixed to 100MHZ
> +            - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
> +            - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
> +            - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
> +            - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ

The clock frequencies is not relevent here, the driver sets that up.

    Andrew

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

[Jie Luo]

On 11/15/2023 10:35 PM, Andrew Lunn wrote:
>> +  phy-reset-gpio:
>> +    minItems: 1
>> +    maxItems: 3
>> +    description:
>> +      GPIO used to reset the PHY, each GPIO is for resetting the connected
>> +      ethernet PHY device.
> 
> This is a PHY property, so should be in the PHY node. phylib should
> then take care of fit.

will check this and update in the next patch set.

> 
>> +
>> +  phyaddr-fixup:
>> +    description: Register address for programing MDIO address of PHY devices
> 
> Please give a better description of this and the next two properties.

these fixup flags are for customizing the MDIO address of qca8084 PHY &
PCS and doing the initialization configurations to bring up PHY.

will describe it more detail in the next patch set.

> 
>> +
>> +  pcsaddr-fixup:
>> +    description: Register address for programing MDIO address of PCS devices
>> +
>> +  mdio-clk-fixup:
>> +    description: The initialization clocks to be configured
>> +
>> +  fixup:
>> +    description: The MDIO address of PHY/PCS device to be programed
>> +        clocks:
>> +          items:
>> +            - description: MDIO clock source frequency fixed to 100MHZ
>> +            - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>> +            - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>> +            - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>> +            - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
> 
> The clock frequencies is not relevent here, the driver sets that up.

OK, will remove the clock frequency values in the next patch set.

> 
>      Andrew

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

[Krzysztof Kozlowski]

On 15/11/2023 04:25, Luo Jie wrote:
> On platform IPQ5332, the MDIO address of qca8084 can be programed
> when the device tree property "fixup" defined, the clock sequence
> needs to be completed before the PHY probeable.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  .../bindings/net/qcom,ipq4019-mdio.yaml       | 138 +++++++++++++++++-
>  1 file changed, 130 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> index 3407e909e8a7..7ff92be14ee1 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> @@ -15,11 +15,13 @@ properties:
>        - enum:
>            - qcom,ipq4019-mdio
>            - qcom,ipq5018-mdio
> +          - qcom,ipq5332-mdio
>  
>        - items:
>            - enum:
>                - qcom,ipq6018-mdio
>                - qcom,ipq8074-mdio
> +              - qcom,ipq9574-mdio
>            - const: qcom,ipq4019-mdio
>  
>    "#address-cells":
> @@ -30,19 +32,47 @@ properties:
>  
>    reg:
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 5
>      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.
> +      the first Address and length of the register set for the MDIO controller,
> +      the optional second, third and fourth address and length of the register
> +      for ethernet LDO, these three address range are required by the platform
> +      IPQ50xx/IPQ5332, the last address and length is for the CMN clock to
> +      select the reference clock.
> +
> +  reg-names:
> +    minItems: 1
> +    maxItems: 5

You must describe the items and constrain them per each variant.

>  
>    clocks:
> -    items:
> -      - description: MDIO clock source frequency fixed to 100MHZ
> +    minItems: 1
> +    maxItems: 5
> +    description:

Doesn't this make all other variants with incorrect constraints?

> +      MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy
> +      clocks enabled for resetting ethernet PHY.
>  
>    clock-names:
> -    items:
> -      - const: gcc_mdio_ahb_clk
> +    minItems: 1
> +    maxItems: 5
> +
> +  phy-reset-gpio:

No, for multiple reasons. It's gpios first of all. Where do you see such
property? Where is the existing definition?

Then it is "reset-gpios" if this is MDIO. Why do you put phy properties
in MDIO?

> +    minItems: 1
> +    maxItems: 3
> +    description:
> +      GPIO used to reset the PHY, each GPIO is for resetting the connected
> +      ethernet PHY device.
> +
> +  phyaddr-fixup:
> +    description: Register address for programing MDIO address of PHY devices

You did not test code which you sent.

> +
> +  pcsaddr-fixup:
> +    description: Register address for programing MDIO address of PCS devices
> +
> +  mdio-clk-fixup:
> +    description: The initialization clocks to be configured
> +
> +  fixup:
> +    description: The MDIO address of PHY/PCS device to be programed

Please do not send untested code.


...

> +
> +      qca8kphy2: ethernet-phy@3 {
> +        reg = <3>;
> +        fixup;
> +      };
> +
> +      qca8kphy3: ethernet-phy@4 {
> +        reg = <4>;
> +        fixup;
> +      };
> +
> +      qca8kpcs0: pcsphy0@5 {
> +        compatible = "qcom,qca8k_pcs";
> +        reg = <5>;
> +        fixup;
> +        };

Fix indentation.

Best regards,
Krzysztof

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

[Jie Luo]

On 11/16/2023 7:56 PM, Krzysztof Kozlowski wrote:
> On 15/11/2023 04:25, Luo Jie wrote:
>> On platform IPQ5332, the MDIO address of qca8084 can be programed
>> when the device tree property "fixup" defined, the clock sequence
>> needs to be completed before the PHY probeable.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 138 +++++++++++++++++-
>>   1 file changed, 130 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> index 3407e909e8a7..7ff92be14ee1 100644
>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> @@ -15,11 +15,13 @@ properties:
>>         - enum:
>>             - qcom,ipq4019-mdio
>>             - qcom,ipq5018-mdio
>> +          - qcom,ipq5332-mdio
>>   
>>         - items:
>>             - enum:
>>                 - qcom,ipq6018-mdio
>>                 - qcom,ipq8074-mdio
>> +              - qcom,ipq9574-mdio
>>             - const: qcom,ipq4019-mdio
>>   
>>     "#address-cells":
>> @@ -30,19 +32,47 @@ properties:
>>   
>>     reg:
>>       minItems: 1
>> -    maxItems: 2
>> +    maxItems: 5
>>       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.
>> +      the first Address and length of the register set for the MDIO controller,
>> +      the optional second, third and fourth address and length of the register
>> +      for ethernet LDO, these three address range are required by the platform
>> +      IPQ50xx/IPQ5332, the last address and length is for the CMN clock to
>> +      select the reference clock.
>> +
>> +  reg-names:
>> +    minItems: 1
>> +    maxItems: 5
> 
> You must describe the items and constrain them per each variant.

Ok, will describe these items one by one in the next patch set.

> 
>>   
>>     clocks:
>> -    items:
>> -      - description: MDIO clock source frequency fixed to 100MHZ
>> +    minItems: 1
>> +    maxItems: 5
>> +    description:
> 
> Doesn't this make all other variants with incorrect constraints?

There are 5 clock items, the first one is the legacy MDIO clock, the
other clocks are new added for ipq5332 platform, will describe it more
clearly in the next patch set.

> 
>> +      MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy
>> +      clocks enabled for resetting ethernet PHY.
>>   
>>     clock-names:
>> -    items:
>> -      - const: gcc_mdio_ahb_clk
>> +    minItems: 1
>> +    maxItems: 5
>> +
>> +  phy-reset-gpio:
> 
> No, for multiple reasons. It's gpios first of all. Where do you see such
> property? Where is the existing definition?

will remove this property, and update to use the exited PHY GPIO reset.

> 
> Then it is "reset-gpios" if this is MDIO. Why do you put phy properties
> in MDIO?
> 
>> +    minItems: 1
>> +    maxItems: 3
>> +    description:
>> +      GPIO used to reset the PHY, each GPIO is for resetting the connected
>> +      ethernet PHY device.
>> +
>> +  phyaddr-fixup:
>> +    description: Register address for programing MDIO address of PHY devices
> 
> You did not test code which you sent.

Hi Krzysztof,
This patch is passed with the following command in my workspace.
i will upgrade and install yamllint to make sure there is no
warning reported anymore.

make dt_bg_check 
DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml

> 
>> +
>> +  pcsaddr-fixup:
>> +    description: Register address for programing MDIO address of PCS devices
>> +
>> +  mdio-clk-fixup:
>> +    description: The initialization clocks to be configured
>> +
>> +  fixup:
>> +    description: The MDIO address of PHY/PCS device to be programed
> 
> Please do not send untested code.
> 

Ok, will complete the full test before uploading the patch.

> 
> ...
> 
>> +
>> +      qca8kphy2: ethernet-phy@3 {
>> +        reg = <3>;
>> +        fixup;
>> +      };
>> +
>> +      qca8kphy3: ethernet-phy@4 {
>> +        reg = <4>;
>> +        fixup;
>> +      };
>> +
>> +      qca8kpcs0: pcsphy0@5 {
>> +        compatible = "qcom,qca8k_pcs";
>> +        reg = <5>;
>> +        fixup;
>> +        };
> 
> Fix indentation.

Will Fix it in the next patch set, thanks.

> 
> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

On 17/11/2023 11:36, Jie Luo wrote:
>>>     clocks:
>>> -    items:
>>> -      - description: MDIO clock source frequency fixed to 100MHZ
>>> +    minItems: 1
>>> +    maxItems: 5
>>> +    description:
>>
>> Doesn't this make all other variants with incorrect constraints?
> 
> There are 5 clock items, the first one is the legacy MDIO clock, the
> other clocks are new added for ipq5332 platform, will describe it more
> clearly in the next patch set.

OTHER variants. Not this one.

> 
>>
>>> +      MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy
>>> +      clocks enabled for resetting ethernet PHY.
>>>   
>>>     clock-names:
>>> -    items:
>>> -      - const: gcc_mdio_ahb_clk
>>> +    minItems: 1
>>> +    maxItems: 5
>>> +
>>> +  phy-reset-gpio:
>>
>> No, for multiple reasons. It's gpios first of all. Where do you see such
>> property? Where is the existing definition?
> 
> will remove this property, and update to use the exited PHY GPIO reset.
> 
>>
>> Then it is "reset-gpios" if this is MDIO. Why do you put phy properties
>> in MDIO?
>>
>>> +    minItems: 1
>>> +    maxItems: 3
>>> +    description:
>>> +      GPIO used to reset the PHY, each GPIO is for resetting the connected
>>> +      ethernet PHY device.
>>> +
>>> +  phyaddr-fixup:
>>> +    description: Register address for programing MDIO address of PHY devices
>>
>> You did not test code which you sent.
> 
> Hi Krzysztof,
> This patch is passed with the following command in my workspace.
> i will upgrade and install yamllint to make sure there is no
> warning reported anymore.
> 
> make dt_bg_check 

No clue what's this, but no, I do not believe you tested it at all. It's
not about yamllint. It's was not tested. Look at errors reported on
mailing list.

> DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml


Best regards,
Krzysztof

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

[Jie Luo]

On 11/17/2023 6:40 PM, Krzysztof Kozlowski wrote:
> On 17/11/2023 11:36, Jie Luo wrote:
>>>>      clocks:
>>>> -    items:
>>>> -      - description: MDIO clock source frequency fixed to 100MHZ
>>>> +    minItems: 1
>>>> +    maxItems: 5
>>>> +    description:
>>>
>>> Doesn't this make all other variants with incorrect constraints?
>>
>> There are 5 clock items, the first one is the legacy MDIO clock, the
>> other clocks are new added for ipq5332 platform, will describe it more
>> clearly in the next patch set.
> 
> OTHER variants. Not this one.

The change here is for the clock number added for the ipq5332 platform,
the other platforms still use only legacy MDIO clock.

so i add minItems  and maxItems, i will check other .yaml files for the
reference.

> 
>>
>>>
>>>> +      MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy
>>>> +      clocks enabled for resetting ethernet PHY.
>>>>    
>>>>      clock-names:
>>>> -    items:
>>>> -      - const: gcc_mdio_ahb_clk
>>>> +    minItems: 1
>>>> +    maxItems: 5
>>>> +
>>>> +  phy-reset-gpio:
>>>
>>> No, for multiple reasons. It's gpios first of all. Where do you see such
>>> property? Where is the existing definition?
>>
>> will remove this property, and update to use the exited PHY GPIO reset.
>>
>>>
>>> Then it is "reset-gpios" if this is MDIO. Why do you put phy properties
>>> in MDIO?
>>>
>>>> +    minItems: 1
>>>> +    maxItems: 3
>>>> +    description:
>>>> +      GPIO used to reset the PHY, each GPIO is for resetting the connected
>>>> +      ethernet PHY device.
>>>> +
>>>> +  phyaddr-fixup:
>>>> +    description: Register address for programing MDIO address of PHY devices
>>>
>>> You did not test code which you sent.
>>
>> Hi Krzysztof,
>> This patch is passed with the following command in my workspace.
>> i will upgrade and install yamllint to make sure there is no
>> warning reported anymore.
>>
>> make dt_bg_check
> 
> No clue what's this, but no, I do not believe you tested it at all. It's
> not about yamllint. It's was not tested. Look at errors reported on
> mailing list.
> 
>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> 

Hi Krzysztof,
Here is the output when i run the dt check with this patch applied in my 
workspace, md64 is the alias for compiling the arm64 platform.

md64 dt_binding_check 
DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
warning: python package 'yamllint' not installed, skipping
   CHKDT   Documentation/devicetree/bindings/processed-schema.json
   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
   DTEX 
Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.example.dts
   DTC_CHK 
Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.example.dtb


> 
> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

On 17/11/2023 12:20, Jie Luo wrote:
> 
> 
> On 11/17/2023 6:40 PM, Krzysztof Kozlowski wrote:
>> On 17/11/2023 11:36, Jie Luo wrote:
>>>>>      clocks:
>>>>> -    items:
>>>>> -      - description: MDIO clock source frequency fixed to 100MHZ
>>>>> +    minItems: 1
>>>>> +    maxItems: 5
>>>>> +    description:
>>>>
>>>> Doesn't this make all other variants with incorrect constraints?
>>>
>>> There are 5 clock items, the first one is the legacy MDIO clock, the
>>> other clocks are new added for ipq5332 platform, will describe it more
>>> clearly in the next patch set.
>>
>> OTHER variants. Not this one.
> 
> The change here is for the clock number added for the ipq5332 platform,
> the other platforms still use only legacy MDIO clock.

Then your patch is wrong as I said. You now affect other variants. I
don't quite get your responses. Style of them suggests that you
disagree, but you are not providing any relevant argument.

> 
> so i add minItems  and maxItems, i will check other .yaml files for the
> reference.
> 
>>
>>>
>>>>
>>>>> +      MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy
>>>>> +      clocks enabled for resetting ethernet PHY.
>>>>>    
>>>>>      clock-names:
>>>>> -    items:
>>>>> -      - const: gcc_mdio_ahb_clk
>>>>> +    minItems: 1
>>>>> +    maxItems: 5
>>>>> +
>>>>> +  phy-reset-gpio:
>>>>
>>>> No, for multiple reasons. It's gpios first of all. Where do you see such
>>>> property? Where is the existing definition?
>>>
>>> will remove this property, and update to use the exited PHY GPIO reset.
>>>
>>>>
>>>> Then it is "reset-gpios" if this is MDIO. Why do you put phy properties
>>>> in MDIO?
>>>>
>>>>> +    minItems: 1
>>>>> +    maxItems: 3
>>>>> +    description:
>>>>> +      GPIO used to reset the PHY, each GPIO is for resetting the connected
>>>>> +      ethernet PHY device.
>>>>> +
>>>>> +  phyaddr-fixup:
>>>>> +    description: Register address for programing MDIO address of PHY devices
>>>>
>>>> You did not test code which you sent.
>>>
>>> Hi Krzysztof,
>>> This patch is passed with the following command in my workspace.
>>> i will upgrade and install yamllint to make sure there is no
>>> warning reported anymore.
>>>
>>> make dt_bg_check
>>
>> No clue what's this, but no, I do not believe you tested it at all. It's
>> not about yamllint. It's was not tested. Look at errors reported on
>> mailing list.
>>
>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>
> 
> Hi Krzysztof,
> Here is the output when i run the dt check with this patch applied in my 
> workspace, md64 is the alias for compiling the arm64 platform.

We still do not know your base and dtschema version.


Best regards,
Krzysztof

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

[Jie Luo]

On 11/17/2023 8:43 PM, Krzysztof Kozlowski wrote:
> On 17/11/2023 12:20, Jie Luo wrote:
>>
>>
>> On 11/17/2023 6:40 PM, Krzysztof Kozlowski wrote:
>>> On 17/11/2023 11:36, Jie Luo wrote:
>>>>>>       clocks:
>>>>>> -    items:
>>>>>> -      - description: MDIO clock source frequency fixed to 100MHZ
>>>>>> +    minItems: 1
>>>>>> +    maxItems: 5
>>>>>> +    description:
>>>>>
>>>>> Doesn't this make all other variants with incorrect constraints?
>>>>
>>>> There are 5 clock items, the first one is the legacy MDIO clock, the
>>>> other clocks are new added for ipq5332 platform, will describe it more
>>>> clearly in the next patch set.
>>>
>>> OTHER variants. Not this one.
>>
>> The change here is for the clock number added for the ipq5332 platform,
>> the other platforms still use only legacy MDIO clock.
> 
> Then your patch is wrong as I said. You now affect other variants. I
> don't quite get your responses. Style of them suggests that you
> disagree, but you are not providing any relevant argument.

The clock arguments are provided in the later part as below. i will also
provide more detail clock names for the new added clocks for the ipq5332
platform in description.

   - if: 

       properties: 

         compatible: 

           contains: 

             enum: 

               - qcom,ipq5332-mdio 

     then: 

       properties: 

         clocks: 

           items: 

             - description: MDIO clock source frequency fixed to 100MHZ 

             - description: UNIPHY0 AHB clock source frequency fixed to 
100MHZ
             - description: UNIPHY0 SYS clock source frequency fixed to 
24MHZ
             - description: UNIPHY1 AHB clock source frequency fixed to 
100MHZ
             - description: UNIPHY1 SYS clock source frequency fixed to 
24MHZ
         clock-names: 

           items: 

             - const: gcc_mdio_ahb_clk 

             - const: gcc_uniphy0_ahb_clk 

             - const: gcc_uniphy0_sys_clk 

             - const: gcc_uniphy1_ahb_clk 

             - const: gcc_uniphy1_sys_clk
> 
>>
>> so i add minItems  and maxItems, i will check other .yaml files for the
>> reference.
>>
>>>
>>>>
>>>>>
>>>>>> +      MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy
>>>>>> +      clocks enabled for resetting ethernet PHY.
>>>>>>     
>>>>>>       clock-names:
>>>>>> -    items:
>>>>>> -      - const: gcc_mdio_ahb_clk
>>>>>> +    minItems: 1
>>>>>> +    maxItems: 5
>>>>>> +
>>>>>> +  phy-reset-gpio:
>>>>>
>>>>> No, for multiple reasons. It's gpios first of all. Where do you see such
>>>>> property? Where is the existing definition?
>>>>
>>>> will remove this property, and update to use the exited PHY GPIO reset.
>>>>
>>>>>
>>>>> Then it is "reset-gpios" if this is MDIO. Why do you put phy properties
>>>>> in MDIO?
>>>>>
>>>>>> +    minItems: 1
>>>>>> +    maxItems: 3
>>>>>> +    description:
>>>>>> +      GPIO used to reset the PHY, each GPIO is for resetting the connected
>>>>>> +      ethernet PHY device.
>>>>>> +
>>>>>> +  phyaddr-fixup:
>>>>>> +    description: Register address for programing MDIO address of PHY devices
>>>>>
>>>>> You did not test code which you sent.
>>>>
>>>> Hi Krzysztof,
>>>> This patch is passed with the following command in my workspace.
>>>> i will upgrade and install yamllint to make sure there is no
>>>> warning reported anymore.
>>>>
>>>> make dt_bg_check
>>>
>>> No clue what's this, but no, I do not believe you tested it at all. It's
>>> not about yamllint. It's was not tested. Look at errors reported on
>>> mailing list.
>>>
>>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>
>>
>> Hi Krzysztof,
>> Here is the output when i run the dt check with this patch applied in my
>> workspace, md64 is the alias for compiling the arm64 platform.
> 
> We still do not know your base and dtschema version.

The code base is the commit id:
5ba73bec5e7b0494da7fdca3e003d8b97fa932cd
<Add linux-next specific files for 20231114>

The dtschema version is as below.
dt-doc-validate --version
2023.9


> 
> 
> Best regards,
> Krzysztof
> 

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

[Andrew Lunn]

> The clock arguments are provided in the later part as below. i will also
> provide more detail clock names for the new added clocks for the ipq5332
> platform in description.
> 
>   - if:
> 
>       properties:
> 
>         compatible:
> 
>           contains:
> 
>             enum:
> 
>               - qcom,ipq5332-mdio
> 
>     then:
> 
>       properties:
> 
>         clocks:
> 
>           items:
> 
>             - description: MDIO clock source frequency fixed to 100MHZ
> 
>             - description: UNIPHY0 AHB clock source frequency fixed to
> 100MHZ
>             - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>             - description: UNIPHY1 AHB clock source frequency fixed to
> 100MHZ
>             - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ

As i said before, the frequency of the clocks does not matter
here. That appears to be the drivers problem. I assume every board
design, with any sort of PHY, needs the same clock configuration?

      Andrew

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

[Jie Luo]

On 11/18/2023 11:36 PM, Andrew Lunn wrote:
>> The clock arguments are provided in the later part as below. i will also
>> provide more detail clock names for the new added clocks for the ipq5332
>> platform in description.
>>
>>    - if:
>>
>>        properties:
>>
>>          compatible:
>>
>>            contains:
>>
>>              enum:
>>
>>                - qcom,ipq5332-mdio
>>
>>      then:
>>
>>        properties:
>>
>>          clocks:
>>
>>            items:
>>
>>              - description: MDIO clock source frequency fixed to 100MHZ
>>
>>              - description: UNIPHY0 AHB clock source frequency fixed to
>> 100MHZ
>>              - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>              - description: UNIPHY1 AHB clock source frequency fixed to
>> 100MHZ
>>              - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
> 
> As i said before, the frequency of the clocks does not matter
> here. That appears to be the drivers problem. I assume every board
> design, with any sort of PHY, needs the same clock configuration?
> 
>        Andrew

Yes, Andrew, no matter what kind of PHY is connected, these clocks are
fix clocks, the clock rates are same as mentioned above, which are the 
SOC clock configurations.