[PATCH v2 0/5] support ipq5332 platform

[PATCH v2 0/5] support ipq5332 platform

[Luo Jie]

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

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

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

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

4. Support optional MDIO clock frequency config.

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

Changes in v2:
	* remove the PHY related features such as PHY address
	  program and clock initialization.

	* leverage the MDIO level GPIO reset for qca8084 PHY.


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

 .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++-
 drivers/net/mdio/mdio-ipq4019.c               | 296 ++++++++++++++++--
 2 files changed, 424 insertions(+), 29 deletions(-)


base-commit: abb240f7a2bd14567ab53e602db562bb683391e6
-- 
2.42.0

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

[Luo Jie]

The ethernet LDO provides the clock for the ethernet PHY that
is connected with PCS, each LDO enables the clock output to
each PCS, after the clock output enablement, the PHY GPIO reset
can take effect.

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

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

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

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

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

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index abd8b508ec16..5273864fabb3 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -37,9 +37,12 @@
 
 #define IPQ_PHY_SET_DELAY_US	100000
 
+/* Maximum SOC PCS(uniphy) number on IPQ platform */
+#define ETH_LDO_RDY_CNT				3
+
 struct ipq4019_mdio_data {
-	void __iomem	*membase;
-	void __iomem *eth_ldo_rdy;
+	void __iomem *membase;
+	void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
 	struct clk *mdio_clk;
 };
 
@@ -206,19 +209,8 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
 static int ipq_mdio_reset(struct mii_bus *bus)
 {
 	struct ipq4019_mdio_data *priv = bus->priv;
-	u32 val;
 	int ret;
 
-	/* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
-	 * is specified in the device tree.
-	 */
-	if (priv->eth_ldo_rdy) {
-		val = readl(priv->eth_ldo_rdy);
-		val |= BIT(0);
-		writel(val, priv->eth_ldo_rdy);
-		fsleep(IPQ_PHY_SET_DELAY_US);
-	}
-
 	/* Configure MDIO clock source frequency if clock is specified in the device tree */
 	ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
 	if (ret)
@@ -236,7 +228,7 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 	struct ipq4019_mdio_data *priv;
 	struct mii_bus *bus;
 	struct resource *res;
-	int ret;
+	int ret, index;
 
 	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
 	if (!bus)
@@ -252,11 +244,32 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->mdio_clk))
 		return PTR_ERR(priv->mdio_clk);
 
-	/* The platform resource is provided on the chipset IPQ5018 */
-	/* This resource is optional */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (res)
-		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
+	/* These platform resources are provided on the chipset IPQ5018 or
+	 * IPQ5332.
+	 */
+	/* This resource are optional */
+	for (index = 0; index < ETH_LDO_RDY_CNT; index++) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1);
+		if (res) {
+			priv->eth_ldo_rdy[index] = devm_ioremap(&pdev->dev,
+								res->start,
+								resource_size(res));
+
+			/* The ethernet LDO enable is necessary to reset PHY
+			 * by GPIO, some PHY(such as qca8084) GPIO reset uses
+			 * the MDIO level reset, so this function should be
+			 * called before the MDIO bus register.
+			 */
+			if (priv->eth_ldo_rdy[index]) {
+				u32 val;
+
+				val = readl(priv->eth_ldo_rdy[index]);
+				val |= BIT(0);
+				writel(val, priv->eth_ldo_rdy[index]);
+				fsleep(IPQ_PHY_SET_DELAY_US);
+			}
+		}
+	}
 
 	bus->name = "ipq4019_mdio";
 	bus->read = ipq4019_mdio_read_c22;
-- 
2.42.0

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

[Maxime Chevallier]

Hello,

On Tue, 12 Dec 2023 19:51:46 +0800
Luo Jie <quic_luoj@quicinc.com> wrote:

> The ethernet LDO provides the clock for the ethernet PHY that
> is connected with PCS, each LDO enables the clock output to
> each PCS, after the clock output enablement, the PHY GPIO reset
> can take effect.
> 
> For the PHY taking the MDIO bus level GPIO reset, the ethernet
> LDO should be enabled before the MDIO bus register.
> 
> For example, the qca8084 PHY takes the MDIO bus level GPIO
> reset for quad PHYs, there is another reason for qca8084 PHY
> using MDIO bus level GPIO reset instead of PHY level GPIO
> reset as below.
> 
> The work sequence of qca8084:
> 1. enable ethernet LDO.
> 2. GPIO reset on quad PHYs.
> 3. register clock provider based on MDIO device of qca8084.
> 4. PHY probe function called for initializing common clocks.
> 5. PHY capabilities acquirement.
> 
> If qca8084 takes PHY level GPIO reset in the step 4, the clock
> provider of qca8084 can't be registered correctly, since the
> clock parent(reading the current qca8084 hardware registers in
> step 3) of the registered clocks is deserted after GPIO reset.
> 
> There are two PCS(UNIPHY) supported in SOC side on ipq5332,
> and three PCS(UNIPHY) supported on ipq9574.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---

[...]

> @@ -252,11 +244,32 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->mdio_clk))
>  		return PTR_ERR(priv->mdio_clk);
>  
> -	/* The platform resource is provided on the chipset IPQ5018 */
> -	/* This resource is optional */
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -	if (res)
> -		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
> +	/* These platform resources are provided on the chipset IPQ5018 or
> +	 * IPQ5332.
> +	 */
> +	/* This resource are optional */
> +	for (index = 0; index < ETH_LDO_RDY_CNT; index++) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1);
> +		if (res) {
> +			priv->eth_ldo_rdy[index] = devm_ioremap(&pdev->dev,
> +								res->start,
> +								resource_size(res));

You can simplify that sequence by using
devm_platform_get_and_ioremap_resource(), which will do both the
platform_get_resource and the devm_ioremap at once for you.

Thanks,

Maxime

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

[Russell King (Oracle)]

On Tue, Dec 12, 2023 at 01:50:01PM +0100, Maxime Chevallier wrote:
> Hello,
> 
> On Tue, 12 Dec 2023 19:51:46 +0800
> Luo Jie <quic_luoj@quicinc.com> wrote:
> > @@ -252,11 +244,32 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
> >  	if (IS_ERR(priv->mdio_clk))
> >  		return PTR_ERR(priv->mdio_clk);
> >  
> > -	/* The platform resource is provided on the chipset IPQ5018 */
> > -	/* This resource is optional */
> > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > -	if (res)
> > -		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
> > +	/* These platform resources are provided on the chipset IPQ5018 or
> > +	 * IPQ5332.
> > +	 */
> > +	/* This resource are optional */
> > +	for (index = 0; index < ETH_LDO_RDY_CNT; index++) {
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1);
> > +		if (res) {
> > +			priv->eth_ldo_rdy[index] = devm_ioremap(&pdev->dev,
> > +								res->start,
> > +								resource_size(res));
> 
> You can simplify that sequence by using
> devm_platform_get_and_ioremap_resource(), which will do both the
> platform_get_resource and the devm_ioremap at once for you.

Sadly it can't if resources are optional. __devm_ioremap_resource()
which will be capped by devm_platform_get_and_ioremap_resource() will
be passed a NULL 'res', which will lead to:

        if (!res || resource_type(res) != IORESOURCE_MEM) {
                dev_err(dev, "invalid resource %pR\n", res);
                return IOMEM_ERR_PTR(-EINVAL);
        }

There isn't an "optional" version of
devm_platform_get_and_ioremap_resource().

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

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

[Maxime Chevallier]

Hello Russell,

On Tue, 12 Dec 2023 19:11:15 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Tue, Dec 12, 2023 at 01:50:01PM +0100, Maxime Chevallier wrote:
> > Hello,
> > 
> > On Tue, 12 Dec 2023 19:51:46 +0800
> > Luo Jie <quic_luoj@quicinc.com> wrote:  
> > > @@ -252,11 +244,32 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
> > >  	if (IS_ERR(priv->mdio_clk))
> > >  		return PTR_ERR(priv->mdio_clk);
> > >  
> > > -	/* The platform resource is provided on the chipset IPQ5018 */
> > > -	/* This resource is optional */
> > > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > -	if (res)
> > > -		priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
> > > +	/* These platform resources are provided on the chipset IPQ5018 or
> > > +	 * IPQ5332.
> > > +	 */
> > > +	/* This resource are optional */
> > > +	for (index = 0; index < ETH_LDO_RDY_CNT; index++) {
> > > +		res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1);
> > > +		if (res) {
> > > +			priv->eth_ldo_rdy[index] = devm_ioremap(&pdev->dev,
> > > +								res->start,
> > > +								resource_size(res));  
> > 
> > You can simplify that sequence by using
> > devm_platform_get_and_ioremap_resource(), which will do both the
> > platform_get_resource and the devm_ioremap at once for you.  
> 
> Sadly it can't if resources are optional. __devm_ioremap_resource()
> which will be capped by devm_platform_get_and_ioremap_resource() will
> be passed a NULL 'res', which will lead to:
> 
>         if (!res || resource_type(res) != IORESOURCE_MEM) {
>                 dev_err(dev, "invalid resource %pR\n", res);
>                 return IOMEM_ERR_PTR(-EINVAL);
>         }
> 
> There isn't an "optional" version of
> devm_platform_get_and_ioremap_resource().
> 

Ah right, I missed that part indeed. Sorry for the noise then, and
thanks for double-checking :)

Best regards,

Maxime

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

[Luo Jie]

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

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

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

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

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

[Maxime Chevallier]

Hello,

On Tue, 12 Dec 2023 19:51:47 +0800
Luo Jie <quic_luoj@quicinc.com> wrote:

> On the platform ipq5332, the related SoC uniphy GCC clocks need
> to be enabled for making the MDIO slave devices accessible.
> 
> These UNIPHY clocks are from the SoC platform GCC clock provider,
> which are enabled for the connected PHY devices working.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>

[...]

>  static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
> @@ -209,14 +230,43 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
>  static int ipq_mdio_reset(struct mii_bus *bus)
>  {
>  	struct ipq4019_mdio_data *priv = bus->priv;
> -	int ret;
> +	int ret, index;
> +	unsigned long rate;

Please remember to use reverse christmas-tree ordering, meaning longer
declaration lines go first :

	struct ipq4019_mdio_data *priv = bus->priv;
	unsigned long rate;
	int ret, index;

> +
> +	/* For the platform ipq5332, there are two SoC uniphies available
> +	 * for connecting with ethernet PHY, the SoC uniphy gcc clock
> +	 * should be enabled for resetting the connected device such
> +	 * as qca8386 switch, qca8081 PHY or other PHYs effectively.
> +	 *
> +	 * Configure MDIO/UNIPHY clock source frequency if clock instance
> +	 * is specified in the device tree.
> +	 */
> +	for (index = MDIO_CLK_MDIO_AHB; index < MDIO_CLK_CNT; index++) {
> +		switch (index) {
> +		case MDIO_CLK_MDIO_AHB:
> +			rate = IPQ_MDIO_CLK_RATE;
> +			break;
> +		case MDIO_CLK_UNIPHY0_AHB:
> +		case MDIO_CLK_UNIPHY1_AHB:
> +			rate = IPQ_UNIPHY_AHB_CLK_RATE;
> +			break;
> +		case MDIO_CLK_UNIPHY0_SYS:
> +		case MDIO_CLK_UNIPHY1_SYS:
> +			rate = IPQ_UNIPHY_SYS_CLK_RATE;
> +			break;
> +		default:
> +			break;
> +		}
>  
> -	/* Configure MDIO clock source frequency if clock is specified in the device tree */
> -	ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
> -	if (ret)
> -		return ret;
> +		ret = clk_set_rate(priv->clk[index], rate);
> +		if (ret)
> +			return ret;
> +
> +		ret = clk_prepare_enable(priv->clk[index]);
> +		if (ret)
> +			return ret;
> +	}
>  
> -	ret = clk_prepare_enable(priv->mdio_clk);
>  	if (ret == 0)
>  		mdelay(10);
>  
> @@ -240,10 +290,6 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->membase))
>  		return PTR_ERR(priv->membase);
>  
> -	priv->mdio_clk = devm_clk_get_optional(&pdev->dev, "gcc_mdio_ahb_clk");
> -	if (IS_ERR(priv->mdio_clk))
> -		return PTR_ERR(priv->mdio_clk);
> -
>  	/* These platform resources are provided on the chipset IPQ5018 or
>  	 * IPQ5332.
>  	 */
> @@ -271,6 +317,13 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	for (index = 0; index < MDIO_CLK_CNT; index++) {
> +		priv->clk[index] = devm_clk_get_optional(&pdev->dev,
> +							 mdio_clk_name[index]);
> +		if (IS_ERR(priv->clk[index]))
> +			return PTR_ERR(priv->clk[index]);
> +	}

You should be able to use devm_clk_bulk_get_optional(), to avoid that
loop.

Thanks,

Maxime

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

[Jie Luo]

On 12/12/2023 8:46 PM, Maxime Chevallier wrote:
> Hello,
> 
> On Tue, 12 Dec 2023 19:51:47 +0800
> Luo Jie <quic_luoj@quicinc.com> wrote:
> 
>> On the platform ipq5332, the related SoC uniphy GCC clocks need
>> to be enabled for making the MDIO slave devices accessible.
>>
>> These UNIPHY clocks are from the SoC platform GCC clock provider,
>> which are enabled for the connected PHY devices working.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> 
> [...]
> 
>>   static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
>> @@ -209,14 +230,43 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
>>   static int ipq_mdio_reset(struct mii_bus *bus)
>>   {
>>   	struct ipq4019_mdio_data *priv = bus->priv;
>> -	int ret;
>> +	int ret, index;
>> +	unsigned long rate;
> 
> Please remember to use reverse christmas-tree ordering, meaning longer
> declaration lines go first :
> 
> 	struct ipq4019_mdio_data *priv = bus->priv;
> 	unsigned long rate;
> 	int ret, index;

Thanks, i will update this.

> 
>> +
>> +	/* For the platform ipq5332, there are two SoC uniphies available
>> +	 * for connecting with ethernet PHY, the SoC uniphy gcc clock
>> +	 * should be enabled for resetting the connected device such
>> +	 * as qca8386 switch, qca8081 PHY or other PHYs effectively.
>> +	 *
>> +	 * Configure MDIO/UNIPHY clock source frequency if clock instance
>> +	 * is specified in the device tree.
>> +	 */
>> +	for (index = MDIO_CLK_MDIO_AHB; index < MDIO_CLK_CNT; index++) {
>> +		switch (index) {
>> +		case MDIO_CLK_MDIO_AHB:
>> +			rate = IPQ_MDIO_CLK_RATE;
>> +			break;
>> +		case MDIO_CLK_UNIPHY0_AHB:
>> +		case MDIO_CLK_UNIPHY1_AHB:
>> +			rate = IPQ_UNIPHY_AHB_CLK_RATE;
>> +			break;
>> +		case MDIO_CLK_UNIPHY0_SYS:
>> +		case MDIO_CLK_UNIPHY1_SYS:
>> +			rate = IPQ_UNIPHY_SYS_CLK_RATE;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>>   
>> -	/* Configure MDIO clock source frequency if clock is specified in the device tree */
>> -	ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
>> -	if (ret)
>> -		return ret;
>> +		ret = clk_set_rate(priv->clk[index], rate);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = clk_prepare_enable(priv->clk[index]);
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>> -	ret = clk_prepare_enable(priv->mdio_clk);
>>   	if (ret == 0)
>>   		mdelay(10);
>>   
>> @@ -240,10 +290,6 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>>   	if (IS_ERR(priv->membase))
>>   		return PTR_ERR(priv->membase);
>>   
>> -	priv->mdio_clk = devm_clk_get_optional(&pdev->dev, "gcc_mdio_ahb_clk");
>> -	if (IS_ERR(priv->mdio_clk))
>> -		return PTR_ERR(priv->mdio_clk);
>> -
>>   	/* These platform resources are provided on the chipset IPQ5018 or
>>   	 * IPQ5332.
>>   	 */
>> @@ -271,6 +317,13 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +	for (index = 0; index < MDIO_CLK_CNT; index++) {
>> +		priv->clk[index] = devm_clk_get_optional(&pdev->dev,
>> +							 mdio_clk_name[index]);
>> +		if (IS_ERR(priv->clk[index]))
>> +			return PTR_ERR(priv->clk[index]);
>> +	}
> 
> You should be able to use devm_clk_bulk_get_optional(), to avoid that
> loop.
> 
> Thanks,
> 
> Maxime

Thanks Maxime for the suggestion.
These clocks need to be configured the different clock rate, MDIO system
clock works on 100MHZ, but UNIPHY system clock works on 24MHZ.

For the clock rate set, i still need the loop to configure the different
clock rate on the different clock instance.

So i use the devm_clk_get_optional to acquire the exact clock ID here.

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

[Luo Jie]

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

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

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

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

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

[Maxime Chevallier]

Hello,

I have some more minor comments for yoi :)

On Tue, 12 Dec 2023 19:51:48 +0800
Luo Jie <quic_luoj@quicinc.com> wrote:

> The reference clock of CMN PLL block is selectable, the internal
> 48MHZ is used by default.
> 
> The output clock of CMN PLL block is for providing the clock
> source of ethernet device(such as qca8084), there are 1 * 25MHZ
> and 3 * 50MHZ output clocks available for the ethernet devices.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---

[...]

> +/* For the CMN PLL block, the reference clock can be configured according to
> + * the device tree property "cmn-reference-clock", the internal 48MHZ is used
> + * by default on the ipq533 platform.
> + *
> + * The output clock of CMN PLL block is provided to the ethernet devices,
> + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default.
> + *
> + * Such as the output 50M clock for the qca8084 ethernet PHY.
> + */
> +static int ipq_cmn_clock_config(struct mii_bus *bus)
> +{
> +	int ret;
> +	u32 reg_val, src_sel, ref_clk;
> +	struct ipq4019_mdio_data *priv;

Here you should also use reverse christmas-tree notation

[...]

> @@ -317,6 +441,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	/* The CMN block resource is for providing clock source to ethernet,
> +	 * which can be optionally configured on the platform ipq9574 and
> +	 * ipq5332.
> +	 */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk");
> +	if (res) {
> +		priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(priv->cmn_membase))
> +			return PTR_ERR(priv->cmn_membase);
> +	}
> +

And here you can simplify a bit by using
devm_platform_ioremap_resource_byname()

Thanks,

Maxime

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

[Russell King (Oracle)]

On Tue, Dec 12, 2023 at 01:54:17PM +0100, Maxime Chevallier wrote:
> Hello,
> 
> I have some more minor comments for yoi :)
> 
> On Tue, 12 Dec 2023 19:51:48 +0800
> Luo Jie <quic_luoj@quicinc.com> wrote:
> > +	/* The CMN block resource is for providing clock source to ethernet,
> > +	 * which can be optionally configured on the platform ipq9574 and
> > +	 * ipq5332.
> > +	 */
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk");
> > +	if (res) {
> > +		priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res);
> > +		if (IS_ERR(priv->cmn_membase))
> > +			return PTR_ERR(priv->cmn_membase);
> > +	}
> > +
> 
> And here you can simplify a bit by using
> devm_platform_ioremap_resource_byname()

Not if the resource is optional.

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

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

[Jie Luo]

On 12/12/2023 8:54 PM, Maxime Chevallier wrote:
> Hello,
> 
> I have some more minor comments for yoi :)
> 
> On Tue, 12 Dec 2023 19:51:48 +0800
> Luo Jie <quic_luoj@quicinc.com> wrote:
> 
>> The reference clock of CMN PLL block is selectable, the internal
>> 48MHZ is used by default.
>>
>> The output clock of CMN PLL block is for providing the clock
>> source of ethernet device(such as qca8084), there are 1 * 25MHZ
>> and 3 * 50MHZ output clocks available for the ethernet devices.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
> 
> [...]
> 
>> +/* For the CMN PLL block, the reference clock can be configured according to
>> + * the device tree property "cmn-reference-clock", the internal 48MHZ is used
>> + * by default on the ipq533 platform.
>> + *
>> + * The output clock of CMN PLL block is provided to the ethernet devices,
>> + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default.
>> + *
>> + * Such as the output 50M clock for the qca8084 ethernet PHY.
>> + */
>> +static int ipq_cmn_clock_config(struct mii_bus *bus)
>> +{
>> +	int ret;
>> +	u32 reg_val, src_sel, ref_clk;
>> +	struct ipq4019_mdio_data *priv;
> 
> Here you should also use reverse christmas-tree notation

Ok, will correct this, thanks.

> 
> [...]
> 
>> @@ -317,6 +441,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +	/* The CMN block resource is for providing clock source to ethernet,
>> +	 * which can be optionally configured on the platform ipq9574 and
>> +	 * ipq5332.
>> +	 */
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk");
>> +	if (res) {
>> +		priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res);
>> +		if (IS_ERR(priv->cmn_membase))
>> +			return PTR_ERR(priv->cmn_membase);
>> +	}
>> +
> 
> And here you can simplify a bit by using
> devm_platform_ioremap_resource_byname()
> 
> Thanks,
> 
> Maxime
> 
As Russell mentioned, since this resource is optional,
so devm_platform_ioremap_resource_byname can't be used here.

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

[Maxime Chevallier]

On Wed, 13 Dec 2023 16:09:53 +0800
Jie Luo <quic_luoj@quicinc.com> wrote:

> On 12/12/2023 8:54 PM, Maxime Chevallier wrote:
> > Hello,
> > 
> > I have some more minor comments for yoi :)
> > 
> > On Tue, 12 Dec 2023 19:51:48 +0800
> > Luo Jie <quic_luoj@quicinc.com> wrote:
> >   
> >> The reference clock of CMN PLL block is selectable, the internal
> >> 48MHZ is used by default.
> >>
> >> The output clock of CMN PLL block is for providing the clock
> >> source of ethernet device(such as qca8084), there are 1 * 25MHZ
> >> and 3 * 50MHZ output clocks available for the ethernet devices.
> >>
> >> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> >> ---  
> > 
> > [...]
> >   
> >> +/* For the CMN PLL block, the reference clock can be configured according to
> >> + * the device tree property "cmn-reference-clock", the internal 48MHZ is used
> >> + * by default on the ipq533 platform.
> >> + *
> >> + * The output clock of CMN PLL block is provided to the ethernet devices,
> >> + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default.
> >> + *
> >> + * Such as the output 50M clock for the qca8084 ethernet PHY.
> >> + */
> >> +static int ipq_cmn_clock_config(struct mii_bus *bus)
> >> +{
> >> +	int ret;
> >> +	u32 reg_val, src_sel, ref_clk;
> >> +	struct ipq4019_mdio_data *priv;  
> > 
> > Here you should also use reverse christmas-tree notation  
> 
> Ok, will correct this, thanks.
> 
> > 
> > [...]
> >   
> >> @@ -317,6 +441,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
> >>   		}
> >>   	}
> >>   
> >> +	/* The CMN block resource is for providing clock source to ethernet,
> >> +	 * which can be optionally configured on the platform ipq9574 and
> >> +	 * ipq5332.
> >> +	 */
> >> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk");
> >> +	if (res) {
> >> +		priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res);
> >> +		if (IS_ERR(priv->cmn_membase))
> >> +			return PTR_ERR(priv->cmn_membase);
> >> +	}
> >> +  
> > 
> > And here you can simplify a bit by using
> > devm_platform_ioremap_resource_byname()
> > 
> > Thanks,
> > 
> > Maxime
> >   
> As Russell mentioned, since this resource is optional,
> so devm_platform_ioremap_resource_byname can't be used here.
> 

Indeed, my bad I missed that point. Sorry for the noise :/

Thanks,

Maxime

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

[Luo Jie]

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

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

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

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 8d3c6bae379f..2c724d3cd513 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -29,6 +29,9 @@
 /* 0 = Clause 22, 1 = Clause 45 */
 #define MDIO_MODE_C45				BIT(8)
 
+/* MDC frequency is SYS_CLK/(MDIO_CLK_DIV + 1), SYS_CLK is 100MHz */
+#define MDIO_CLK_DIV_MASK			GENMASK(7, 0)
+
 #define IPQ4019_MDIO_TIMEOUT	10000
 #define IPQ4019_MDIO_SLEEP		10
 
@@ -77,6 +80,7 @@ struct ipq4019_mdio_data {
 	void __iomem *cmn_membase;
 	void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
 	struct clk *clk[MDIO_CLK_CNT];
+	int clk_div;
 };
 
 static const char *const mdio_clk_name[] = {
@@ -110,6 +114,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);
 
@@ -151,6 +156,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);
 
@@ -183,6 +189,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);
 
@@ -226,6 +233,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);
 
@@ -397,6 +405,39 @@ static int ipq_mdio_reset(struct mii_bus *bus)
 	return ret;
 }
 
+static int ipq_mdio_clk_set(struct platform_device *pdev, int *clk_div)
+{
+	int freq;
+
+	/* Keep the MDIO clock divider as the hardware default value 0xff if
+	 * the MDIO property "clock-frequency" is not specified.
+	 */
+	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency", &freq)) {
+		*clk_div = 0xff;
+		return 0;
+	}
+
+	/* MDC frequency is SYS_CLK/(MDIO_CLK_DIV + 1), SYS_CLK is fixed
+	 * to 100MHz, the MDIO_CLK_DIV can be only configured the valid
+	 * values, other values cause malfunction.
+	 */
+	switch (freq) {
+	case 12500000:
+	case 6250000:
+	case 3125000:
+	case 1562500:
+	case 781250:
+	case 390625:
+		*clk_div = DIV_ROUND_UP(IPQ_MDIO_CLK_RATE, freq) - 1;
+		break;
+	default:
+		dev_err(&pdev->dev, "Invalid clock frequency %dHZ\n", freq);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int ipq4019_mdio_probe(struct platform_device *pdev)
 {
 	struct ipq4019_mdio_data *priv;
@@ -459,6 +500,10 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 			return PTR_ERR(priv->clk[index]);
 	}
 
+	ret = ipq_mdio_clk_set(pdev, &priv->clk_div);
+	if (ret)
+		return ret;
+
 	bus->name = "ipq4019_mdio";
 	bus->read = ipq4019_mdio_read_c22;
 	bus->write = ipq4019_mdio_write_c22;
-- 
2.42.0

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

[Luo Jie]

Update the yaml file for the new DTS properties.

1. cmn-reference-clock for the CMN PLL source clock select.
2. clock-frequency for MDIO clock frequency config.
3. add uniphy AHB & SYS GCC clocks.
4. add reset-gpios for MDIO bus level reset.

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

diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
index 3407e909e8a7..9546a6ad7841 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
@@ -20,6 +20,8 @@ properties:
           - enum:
               - qcom,ipq6018-mdio
               - qcom,ipq8074-mdio
+              - qcom,ipq9574-mdio
+              - qcom,ipq5332-mdio
           - const: qcom,ipq4019-mdio
 
   "#address-cells":
@@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
+      to select the reference clock.
+
+  reg-names:
+    minItems: 1
+    maxItems: 5
 
   clocks:
+    minItems: 1
     items:
       - description: MDIO clock source frequency fixed to 100MHZ
+      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
+      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
+      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
+      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
 
   clock-names:
+    minItems: 1
     items:
       - const: gcc_mdio_ahb_clk
+      - const: gcc_uniphy0_ahb_clk
+      - const: gcc_uniphy1_ahb_clk
+      - const: gcc_uniphy0_sys_clk
+      - const: gcc_uniphy1_sys_clk
+
+  cmn-reference-clock:
+    oneOf:
+      - items:
+          - enum:
+              - 0   # CMN PLL reference internal 48MHZ
+              - 1   # CMN PLL reference external 25MHZ
+              - 2   # CMN PLL reference external 31250KHZ
+              - 3   # CMN PLL reference external 40MHZ
+              - 4   # CMN PLL reference external 48MHZ
+              - 5   # CMN PLL reference external 50MHZ
+              - 6   # CMN PLL reference internal 96MHZ
+
+  clock-frequency:
+    oneOf:
+      - items:
+          - enum:
+              - 12500000
+              - 6250000
+              - 3125000
+              - 1562500
+              - 781250
+              - 390625
+    description:
+      The MDIO bus clock that must be output by the MDIO bus hardware,
+      only the listed frequecies above can be configured, other frequency
+      will cause malfunction. If absent, the default hardware value is used.
+
+  reset-gpios:
+    maxItems: 1
+
+  reset-assert-us:
+    maxItems: 1
+
+  reset-deassert-us:
+    maxItems: 1
 
 required:
   - compatible
@@ -61,6 +115,8 @@ allOf:
               - qcom,ipq5018-mdio
               - qcom,ipq6018-mdio
               - qcom,ipq8074-mdio
+              - qcom,ipq5332-mdio
+              - qcom,ipq9574-mdio
     then:
       required:
         - clocks
@@ -70,6 +126,40 @@ allOf:
         clocks: false
         clock-names: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq5332-mdio
+    then:
+      properties:
+        clocks:
+          minItems: 5
+          maxItems: 5
+        reg-names:
+          items:
+            - const: mdio
+            - const: eth_ldo1
+            - const: eth_ldo2
+            - const: cmn_blk
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq9574-mdio
+    then:
+      properties:
+        reg-names:
+          items:
+            - const: mdio
+            - const: eth_ldo1
+            - const: eth_ldo2
+            - const: eth_ldo3
+            - const: cmn_blk
+
 unevaluatedProperties: false
 
 examples:
@@ -100,3 +190,62 @@ examples:
         reg = <4>;
       };
     };
+
+  - |
+    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    mdio@90000 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      compatible = "qcom,ipq5332-mdio",
+                   "qcom,ipq4019-mdio";
+      cmn-reference-clock = <0>;
+      clock-frequency = <6250000>;
+
+      reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>;
+      reset-assert-us = <100000>;
+      reset-deassert-us = <100000>;
+
+      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_UNIPHY1_AHB_CLK>,
+               <&gcc GCC_UNIPHY0_SYS_CLK>,
+               <&gcc GCC_UNIPHY1_SYS_CLK>;
+
+      clock-names = "gcc_mdio_ahb_clk",
+                    "gcc_uniphy0_ahb_clk",
+                    "gcc_uniphy1_ahb_clk",
+                    "gcc_uniphy0_sys_clk",
+                    "gcc_uniphy1_sys_clk";
+
+      qca8kphy0: ethernet-phy@1 {
+        compatible = "ethernet-phy-id004d.d180";
+        reg = <1>;
+      };
+
+      qca8kphy1: ethernet-phy@2 {
+        compatible = "ethernet-phy-id004d.d180";
+        reg = <2>;
+      };
+
+      qca8kphy2: ethernet-phy@3 {
+        compatible = "ethernet-phy-id004d.d180";
+        reg = <3>;
+      };
+
+      qca8kphy3: ethernet-phy@4 {
+        compatible = "ethernet-phy-id004d.d180";
+        reg = <4>;
+      };
+    };
-- 
2.42.0

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

[Rob Herring]

On Tue, 12 Dec 2023 19:51:50 +0800, Luo Jie wrote:
> Update the yaml file for the new DTS properties.
> 
> 1. cmn-reference-clock for the CMN PLL source clock select.
> 2. clock-frequency for MDIO clock frequency config.
> 3. add uniphy AHB & SYS GCC clocks.
> 4. add reset-gpios for MDIO bus level reset.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>  1 file changed, 153 insertions(+), 4 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: cmn-reference-clock: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231212115151.20016-6-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 v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Conor Dooley]

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

On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
> Update the yaml file for the new DTS properties.
> 
> 1. cmn-reference-clock for the CMN PLL source clock select.
> 2. clock-frequency for MDIO clock frequency config.
> 3. add uniphy AHB & SYS GCC clocks.
> 4. add reset-gpios for MDIO bus level reset.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>  1 file changed, 153 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> index 3407e909e8a7..9546a6ad7841 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> @@ -20,6 +20,8 @@ properties:
>            - enum:
>                - qcom,ipq6018-mdio
>                - qcom,ipq8074-mdio
> +              - qcom,ipq9574-mdio
> +              - qcom,ipq5332-mdio
>            - const: qcom,ipq4019-mdio
>  
>    "#address-cells":
> @@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
> +      to select the reference clock.
> +
> +  reg-names:
> +    minItems: 1
> +    maxItems: 5
>  
>    clocks:
> +    minItems: 1
>      items:
>        - description: MDIO clock source frequency fixed to 100MHZ
> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>  
>    clock-names:
> +    minItems: 1
>      items:
>        - const: gcc_mdio_ahb_clk
> +      - const: gcc_uniphy0_ahb_clk
> +      - const: gcc_uniphy1_ahb_clk
> +      - const: gcc_uniphy0_sys_clk
> +      - const: gcc_uniphy1_sys_clk

> +  cmn-reference-clock:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - 0   # CMN PLL reference internal 48MHZ
> +              - 1   # CMN PLL reference external 25MHZ
> +              - 2   # CMN PLL reference external 31250KHZ
> +              - 3   # CMN PLL reference external 40MHZ
> +              - 4   # CMN PLL reference external 48MHZ
> +              - 5   # CMN PLL reference external 50MHZ
> +              - 6   # CMN PLL reference internal 96MHZ

Why is this not represented by an element of the clocks property?

> +  clock-frequency:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - 12500000
> +              - 6250000
> +              - 3125000
> +              - 1562500
> +              - 781250
> +              - 390625
> +    description:
> +      The MDIO bus clock that must be output by the MDIO bus hardware,
> +      only the listed frequecies above can be configured, other frequency
> +      will cause malfunction. If absent, the default hardware value is used.

Likewise.

Your commit message contains a bullet point list of what you are doing,
but there's no explanation here for why custom properties are required
to provide clock information.

Thanks,
Conor.

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

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

[Jie Luo]

On 12/13/2023 12:06 AM, Conor Dooley wrote:
> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>> Update the yaml file for the new DTS properties.
>>
>> 1. cmn-reference-clock for the CMN PLL source clock select.
>> 2. clock-frequency for MDIO clock frequency config.
>> 3. add uniphy AHB & SYS GCC clocks.
>> 4. add reset-gpios for MDIO bus level reset.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>>   1 file changed, 153 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> index 3407e909e8a7..9546a6ad7841 100644
>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> @@ -20,6 +20,8 @@ properties:
>>             - enum:
>>                 - qcom,ipq6018-mdio
>>                 - qcom,ipq8074-mdio
>> +              - qcom,ipq9574-mdio
>> +              - qcom,ipq5332-mdio
>>             - const: qcom,ipq4019-mdio
>>   
>>     "#address-cells":
>> @@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
>> +      to select the reference clock.
>> +
>> +  reg-names:
>> +    minItems: 1
>> +    maxItems: 5
>>   
>>     clocks:
>> +    minItems: 1
>>       items:
>>         - description: MDIO clock source frequency fixed to 100MHZ
>> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>   
>>     clock-names:
>> +    minItems: 1
>>       items:
>>         - const: gcc_mdio_ahb_clk
>> +      - const: gcc_uniphy0_ahb_clk
>> +      - const: gcc_uniphy1_ahb_clk
>> +      - const: gcc_uniphy0_sys_clk
>> +      - const: gcc_uniphy1_sys_clk
> 
>> +  cmn-reference-clock:
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - 0   # CMN PLL reference internal 48MHZ
>> +              - 1   # CMN PLL reference external 25MHZ
>> +              - 2   # CMN PLL reference external 31250KHZ
>> +              - 3   # CMN PLL reference external 40MHZ
>> +              - 4   # CMN PLL reference external 48MHZ
>> +              - 5   # CMN PLL reference external 50MHZ
>> +              - 6   # CMN PLL reference internal 96MHZ
> 
> Why is this not represented by an element of the clocks property?

This property is for the reference clock source selection of CMN PLL,
CMN PLL generates the different clock rates for the different Ethernet
blocks, this CMN PLL configuration is not located in the GCC, so the
clock framework can't be used, which is the general hardware register
instead of RCG register for GCC.

> 
>> +  clock-frequency:
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - 12500000
>> +              - 6250000
>> +              - 3125000
>> +              - 1562500
>> +              - 781250
>> +              - 390625
>> +    description:
>> +      The MDIO bus clock that must be output by the MDIO bus hardware,
>> +      only the listed frequecies above can be configured, other frequency
>> +      will cause malfunction. If absent, the default hardware value is used.
> 
> Likewise.
> 
> Your commit message contains a bullet point list of what you are doing,
> but there's no explanation here for why custom properties are required
> to provide clock information.
> 
> Thanks,
> Conor.

Hi Conor,
This property clock-frequency is optional to configure the MDIO working
clock rate, and this is the MDIO general DT property, since the hardware
default clock rate is 390625HZ, there is requirement for higher clock 
rate in the normal working case, i will update this information in the
next patch set.

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

[Conor Dooley]

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

On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
> 
> 
> On 12/13/2023 12:06 AM, Conor Dooley wrote:
> > On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
> > > Update the yaml file for the new DTS properties.
> > > 
> > > 1. cmn-reference-clock for the CMN PLL source clock select.
> > > 2. clock-frequency for MDIO clock frequency config.
> > > 3. add uniphy AHB & SYS GCC clocks.
> > > 4. add reset-gpios for MDIO bus level reset.
> > > 
> > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > > ---
> > >   .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
> > >   1 file changed, 153 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> > > index 3407e909e8a7..9546a6ad7841 100644
> > > --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> > > +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> > > @@ -20,6 +20,8 @@ properties:
> > >             - enum:
> > >                 - qcom,ipq6018-mdio
> > >                 - qcom,ipq8074-mdio
> > > +              - qcom,ipq9574-mdio
> > > +              - qcom,ipq5332-mdio
> > >             - const: qcom,ipq4019-mdio
> > >     "#address-cells":
> > > @@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
> > > +      to select the reference clock.
> > > +
> > > +  reg-names:
> > > +    minItems: 1
> > > +    maxItems: 5
> > >     clocks:
> > > +    minItems: 1
> > >       items:
> > >         - description: MDIO clock source frequency fixed to 100MHZ
> > > +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
> > > +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
> > > +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
> > > +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
> > >     clock-names:
> > > +    minItems: 1
> > >       items:
> > >         - const: gcc_mdio_ahb_clk
> > > +      - const: gcc_uniphy0_ahb_clk
> > > +      - const: gcc_uniphy1_ahb_clk
> > > +      - const: gcc_uniphy0_sys_clk
> > > +      - const: gcc_uniphy1_sys_clk
> > 
> > > +  cmn-reference-clock:
> > > +    oneOf:
> > > +      - items:
> > > +          - enum:
> > > +              - 0   # CMN PLL reference internal 48MHZ
> > > +              - 1   # CMN PLL reference external 25MHZ
> > > +              - 2   # CMN PLL reference external 31250KHZ
> > > +              - 3   # CMN PLL reference external 40MHZ
> > > +              - 4   # CMN PLL reference external 48MHZ
> > > +              - 5   # CMN PLL reference external 50MHZ
> > > +              - 6   # CMN PLL reference internal 96MHZ
> > 
> > Why is this not represented by an element of the clocks property?
> 
> This property is for the reference clock source selection of CMN PLL,
> CMN PLL generates the different clock rates for the different Ethernet
> blocks, this CMN PLL configuration is not located in the GCC, so the
> clock framework can't be used, which is the general hardware register
> instead of RCG register for GCC.

I don't see how the clock being provided by the "GCC" (whatever that is)
or by some other clock controller or fixed clock makes a difference.
Why can't the other clock provider be represented in the devicetree?

> > > +  clock-frequency:
> > > +    oneOf:
> > > +      - items:
> > > +          - enum:
> > > +              - 12500000
> > > +              - 6250000
> > > +              - 3125000
> > > +              - 1562500
> > > +              - 781250
> > > +              - 390625
> > > +    description:
> > > +      The MDIO bus clock that must be output by the MDIO bus hardware,
> > > +      only the listed frequecies above can be configured, other frequency
> > > +      will cause malfunction. If absent, the default hardware value is used.
> > 
> > Likewise.
> > 
> > Your commit message contains a bullet point list of what you are doing,
> > but there's no explanation here for why custom properties are required
> > to provide clock information.

> This property clock-frequency is optional to configure the MDIO working
> clock rate, and this is the MDIO general DT property, since the hardware
> default clock rate is 390625HZ, there is requirement for higher clock rate
> in the normal working case, i will update this information in the
> next patch set.

I'm just realising that this particular one is not a custom property,
the unusual `oneOf: - items: - enum:` structure here threw me. This can
just be
  clock-frequency:
    enum:
      - 12500000
      - 6250000
      - 3125000
      - 1562500
      - 781250
      - 390625

but you're missing a default, given your commit about the last element
in that list being one.

Thanks,
Conor.

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

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

[Jie Luo]

On 12/15/2023 1:12 AM, Conor Dooley wrote:
> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>
>>
>> On 12/13/2023 12:06 AM, Conor Dooley wrote:
>>> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>>>> Update the yaml file for the new DTS properties.
>>>>
>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>> 2. clock-frequency for MDIO clock frequency config.
>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>
>>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>>> ---
>>>>    .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>>>>    1 file changed, 153 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>> index 3407e909e8a7..9546a6ad7841 100644
>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>> @@ -20,6 +20,8 @@ properties:
>>>>              - enum:
>>>>                  - qcom,ipq6018-mdio
>>>>                  - qcom,ipq8074-mdio
>>>> +              - qcom,ipq9574-mdio
>>>> +              - qcom,ipq5332-mdio
>>>>              - const: qcom,ipq4019-mdio
>>>>      "#address-cells":
>>>> @@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
>>>> +      to select the reference clock.
>>>> +
>>>> +  reg-names:
>>>> +    minItems: 1
>>>> +    maxItems: 5
>>>>      clocks:
>>>> +    minItems: 1
>>>>        items:
>>>>          - description: MDIO clock source frequency fixed to 100MHZ
>>>> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>>      clock-names:
>>>> +    minItems: 1
>>>>        items:
>>>>          - const: gcc_mdio_ahb_clk
>>>> +      - const: gcc_uniphy0_ahb_clk
>>>> +      - const: gcc_uniphy1_ahb_clk
>>>> +      - const: gcc_uniphy0_sys_clk
>>>> +      - const: gcc_uniphy1_sys_clk
>>>
>>>> +  cmn-reference-clock:
>>>> +    oneOf:
>>>> +      - items:
>>>> +          - enum:
>>>> +              - 0   # CMN PLL reference internal 48MHZ
>>>> +              - 1   # CMN PLL reference external 25MHZ
>>>> +              - 2   # CMN PLL reference external 31250KHZ
>>>> +              - 3   # CMN PLL reference external 40MHZ
>>>> +              - 4   # CMN PLL reference external 48MHZ
>>>> +              - 5   # CMN PLL reference external 50MHZ
>>>> +              - 6   # CMN PLL reference internal 96MHZ
>>>
>>> Why is this not represented by an element of the clocks property?
>>
>> This property is for the reference clock source selection of CMN PLL,
>> CMN PLL generates the different clock rates for the different Ethernet
>> blocks, this CMN PLL configuration is not located in the GCC, so the
>> clock framework can't be used, which is the general hardware register
>> instead of RCG register for GCC.
> 
> I don't see how the clock being provided by the "GCC" (whatever that is)
> or by some other clock controller or fixed clock makes a difference.
> Why can't the other clock provider be represented in the devicetree?
> 

cmn-reference-clock is for selecting the reference clock source for the
whole Ethernet block, which is just the standalone configure register.
however the clock provider has the logical register distribution, such
as for one clock tree, there is RCG, DIVIDER and branch registers in
the qcom soc chip.

The clock consumer defines the clock IDs of device tree to reference the
clocks provided by the clock controller, and these clock IDs are
provided by the header file of clock provider.

like this,
clocks = <&gcc GCC_MDIO_AHB_CLK>, 

          <&gcc GCC_UNIPHY0_AHB_CLK>, 

          <&gcc GCC_UNIPHY1_AHB_CLK>, 

          <&gcc GCC_UNIPHY0_SYS_CLK>, 

          <&gcc GCC_UNIPHY1_SYS_CLK>;

gcc is the device node of clock provider, and GCC_MDIO_AHB_CLK is the 
clock ID.


>>>> +  clock-frequency:
>>>> +    oneOf:
>>>> +      - items:
>>>> +          - enum:
>>>> +              - 12500000
>>>> +              - 6250000
>>>> +              - 3125000
>>>> +              - 1562500
>>>> +              - 781250
>>>> +              - 390625
>>>> +    description:
>>>> +      The MDIO bus clock that must be output by the MDIO bus hardware,
>>>> +      only the listed frequecies above can be configured, other frequency
>>>> +      will cause malfunction. If absent, the default hardware value is used.
>>>
>>> Likewise.
>>>
>>> Your commit message contains a bullet point list of what you are doing,
>>> but there's no explanation here for why custom properties are required
>>> to provide clock information.
> 
>> This property clock-frequency is optional to configure the MDIO working
>> clock rate, and this is the MDIO general DT property, since the hardware
>> default clock rate is 390625HZ, there is requirement for higher clock rate
>> in the normal working case, i will update this information in the
>> next patch set.
> 
> I'm just realising that this particular one is not a custom property,
> the unusual `oneOf: - items: - enum:` structure here threw me. This can
> just be
>    clock-frequency:
>      enum:
>        - 12500000
>        - 6250000
>        - 3125000
>        - 1562500
>        - 781250
>        - 390625
> 
> but you're missing a default, given your commit about the last element
> in that list being one.
> 
> Thanks,
> Conor.

Ok, will update this in the next patch set, thanks for the comments.

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

[Krzysztof Kozlowski]

On 15/12/2023 07:46, Jie Luo wrote:
> 
> 
> On 12/15/2023 1:12 AM, Conor Dooley wrote:
>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>>
>>>
>>> On 12/13/2023 12:06 AM, Conor Dooley wrote:
>>>> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>>>>> Update the yaml file for the new DTS properties.
>>>>>
>>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>>> 2. clock-frequency for MDIO clock frequency config.
>>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>>
>>>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>>>> ---
>>>>>    .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>>>>>    1 file changed, 153 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>> index 3407e909e8a7..9546a6ad7841 100644
>>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>> @@ -20,6 +20,8 @@ properties:
>>>>>              - enum:
>>>>>                  - qcom,ipq6018-mdio
>>>>>                  - qcom,ipq8074-mdio
>>>>> +              - qcom,ipq9574-mdio
>>>>> +              - qcom,ipq5332-mdio
>>>>>              - const: qcom,ipq4019-mdio
>>>>>      "#address-cells":
>>>>> @@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
>>>>> +      to select the reference clock.
>>>>> +
>>>>> +  reg-names:
>>>>> +    minItems: 1
>>>>> +    maxItems: 5
>>>>>      clocks:
>>>>> +    minItems: 1
>>>>>        items:
>>>>>          - description: MDIO clock source frequency fixed to 100MHZ
>>>>> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>>> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>>> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>>> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>>>      clock-names:
>>>>> +    minItems: 1
>>>>>        items:
>>>>>          - const: gcc_mdio_ahb_clk
>>>>> +      - const: gcc_uniphy0_ahb_clk
>>>>> +      - const: gcc_uniphy1_ahb_clk
>>>>> +      - const: gcc_uniphy0_sys_clk
>>>>> +      - const: gcc_uniphy1_sys_clk
>>>>
>>>>> +  cmn-reference-clock:
>>>>> +    oneOf:
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - 0   # CMN PLL reference internal 48MHZ
>>>>> +              - 1   # CMN PLL reference external 25MHZ
>>>>> +              - 2   # CMN PLL reference external 31250KHZ
>>>>> +              - 3   # CMN PLL reference external 40MHZ
>>>>> +              - 4   # CMN PLL reference external 48MHZ
>>>>> +              - 5   # CMN PLL reference external 50MHZ
>>>>> +              - 6   # CMN PLL reference internal 96MHZ
>>>>
>>>> Why is this not represented by an element of the clocks property?
>>>
>>> This property is for the reference clock source selection of CMN PLL,
>>> CMN PLL generates the different clock rates for the different Ethernet
>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>> clock framework can't be used, which is the general hardware register
>>> instead of RCG register for GCC.
>>
>> I don't see how the clock being provided by the "GCC" (whatever that is)
>> or by some other clock controller or fixed clock makes a difference.
>> Why can't the other clock provider be represented in the devicetree?
>>
> 
> cmn-reference-clock is for selecting the reference clock source for the
> whole Ethernet block, which is just the standalone configure register.

Sure, you are aware though that all clocks are just configure registers?

Which clocks are these mentioned in the property? From where do they come?

Anyway, property is in existing form is not correct - this is not a
generic property.


> however the clock provider has the logical register distribution, such
> as for one clock tree, there is RCG, DIVIDER and branch registers in
> the qcom soc chip.
> 
> The clock consumer defines the clock IDs of device tree to reference the
> clocks provided by the clock controller, and these clock IDs are
> provided by the header file of clock provider.
> 
> like this,
> clocks = <&gcc GCC_MDIO_AHB_CLK>, 
> 
>           <&gcc GCC_UNIPHY0_AHB_CLK>, 
> 
>           <&gcc GCC_UNIPHY1_AHB_CLK>, 
> 
>           <&gcc GCC_UNIPHY0_SYS_CLK>, 
> 
>           <&gcc GCC_UNIPHY1_SYS_CLK>;
> 
> gcc is the device node of clock provider, and GCC_MDIO_AHB_CLK is the 
> clock ID.



Best regards,
Krzysztof

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

[Jie Luo]

On 12/15/2023 3:29 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 07:46, Jie Luo wrote:
>>
>>
>> On 12/15/2023 1:12 AM, Conor Dooley wrote:
>>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>>>
>>>>
>>>> On 12/13/2023 12:06 AM, Conor Dooley wrote:
>>>>> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>>>>>> Update the yaml file for the new DTS properties.
>>>>>>
>>>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>>>> 2. clock-frequency for MDIO clock frequency config.
>>>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>>>
>>>>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>>>>> ---
>>>>>>     .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>>>>>>     1 file changed, 153 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>> index 3407e909e8a7..9546a6ad7841 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>> @@ -20,6 +20,8 @@ properties:
>>>>>>               - enum:
>>>>>>                   - qcom,ipq6018-mdio
>>>>>>                   - qcom,ipq8074-mdio
>>>>>> +              - qcom,ipq9574-mdio
>>>>>> +              - qcom,ipq5332-mdio
>>>>>>               - const: qcom,ipq4019-mdio
>>>>>>       "#address-cells":
>>>>>> @@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
>>>>>> +      to select the reference clock.
>>>>>> +
>>>>>> +  reg-names:
>>>>>> +    minItems: 1
>>>>>> +    maxItems: 5
>>>>>>       clocks:
>>>>>> +    minItems: 1
>>>>>>         items:
>>>>>>           - description: MDIO clock source frequency fixed to 100MHZ
>>>>>> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>>>> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>>>> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>>>> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>>>>       clock-names:
>>>>>> +    minItems: 1
>>>>>>         items:
>>>>>>           - const: gcc_mdio_ahb_clk
>>>>>> +      - const: gcc_uniphy0_ahb_clk
>>>>>> +      - const: gcc_uniphy1_ahb_clk
>>>>>> +      - const: gcc_uniphy0_sys_clk
>>>>>> +      - const: gcc_uniphy1_sys_clk
>>>>>
>>>>>> +  cmn-reference-clock:
>>>>>> +    oneOf:
>>>>>> +      - items:
>>>>>> +          - enum:
>>>>>> +              - 0   # CMN PLL reference internal 48MHZ
>>>>>> +              - 1   # CMN PLL reference external 25MHZ
>>>>>> +              - 2   # CMN PLL reference external 31250KHZ
>>>>>> +              - 3   # CMN PLL reference external 40MHZ
>>>>>> +              - 4   # CMN PLL reference external 48MHZ
>>>>>> +              - 5   # CMN PLL reference external 50MHZ
>>>>>> +              - 6   # CMN PLL reference internal 96MHZ
>>>>>
>>>>> Why is this not represented by an element of the clocks property?
>>>>
>>>> This property is for the reference clock source selection of CMN PLL,
>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>> clock framework can't be used, which is the general hardware register
>>>> instead of RCG register for GCC.
>>>
>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>> or by some other clock controller or fixed clock makes a difference.
>>> Why can't the other clock provider be represented in the devicetree?
>>>
>>
>> cmn-reference-clock is for selecting the reference clock source for the
>> whole Ethernet block, which is just the standalone configure register.
> 
> Sure, you are aware though that all clocks are just configure registers?
> 
> Which clocks are these mentioned in the property? From where do they come?
> 
> Anyway, property is in existing form is not correct - this is not a
> generic property.
> 

This property cmn-reference-clock is just the hardware register 
configuration, since the different IPQ platform needs to select
the different reference clock source for the CMN PLL block that
provides the various clock outputs to the all kinds of Ethernet
devices, which is not from GCC provider.

This is indeed not a generic property, which is the Ethernet
function configs same as clock-frequency.

> 
>> however the clock provider has the logical register distribution, such
>> as for one clock tree, there is RCG, DIVIDER and branch registers in
>> the qcom soc chip.
>>
>> The clock consumer defines the clock IDs of device tree to reference the
>> clocks provided by the clock controller, and these clock IDs are
>> provided by the header file of clock provider.
>>
>> like this,
>> clocks = <&gcc GCC_MDIO_AHB_CLK>,
>>
>>            <&gcc GCC_UNIPHY0_AHB_CLK>,
>>
>>            <&gcc GCC_UNIPHY1_AHB_CLK>,
>>
>>            <&gcc GCC_UNIPHY0_SYS_CLK>,
>>
>>            <&gcc GCC_UNIPHY1_SYS_CLK>;
>>
>> gcc is the device node of clock provider, and GCC_MDIO_AHB_CLK is the
>> clock ID.
> 
> 
> 
> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

On 15/12/2023 10:49, Jie Luo wrote:
> 
> 
> On 12/15/2023 3:29 PM, Krzysztof Kozlowski wrote:
>> On 15/12/2023 07:46, Jie Luo wrote:
>>>
>>>
>>> On 12/15/2023 1:12 AM, Conor Dooley wrote:
>>>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>>>>
>>>>>
>>>>> On 12/13/2023 12:06 AM, Conor Dooley wrote:
>>>>>> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>>>>>>> Update the yaml file for the new DTS properties.
>>>>>>>
>>>>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>>>>> 2. clock-frequency for MDIO clock frequency config.
>>>>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>>>>
>>>>>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>>>>>> ---
>>>>>>>     .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>>>>>>>     1 file changed, 153 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>> index 3407e909e8a7..9546a6ad7841 100644
>>>>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>> @@ -20,6 +20,8 @@ properties:
>>>>>>>               - enum:
>>>>>>>                   - qcom,ipq6018-mdio
>>>>>>>                   - qcom,ipq8074-mdio
>>>>>>> +              - qcom,ipq9574-mdio
>>>>>>> +              - qcom,ipq5332-mdio
>>>>>>>               - const: qcom,ipq4019-mdio
>>>>>>>       "#address-cells":
>>>>>>> @@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
>>>>>>> +      to select the reference clock.
>>>>>>> +
>>>>>>> +  reg-names:
>>>>>>> +    minItems: 1
>>>>>>> +    maxItems: 5
>>>>>>>       clocks:
>>>>>>> +    minItems: 1
>>>>>>>         items:
>>>>>>>           - description: MDIO clock source frequency fixed to 100MHZ
>>>>>>> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>>>>> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>>>>> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>>>>> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>>>>>       clock-names:
>>>>>>> +    minItems: 1
>>>>>>>         items:
>>>>>>>           - const: gcc_mdio_ahb_clk
>>>>>>> +      - const: gcc_uniphy0_ahb_clk
>>>>>>> +      - const: gcc_uniphy1_ahb_clk
>>>>>>> +      - const: gcc_uniphy0_sys_clk
>>>>>>> +      - const: gcc_uniphy1_sys_clk
>>>>>>
>>>>>>> +  cmn-reference-clock:
>>>>>>> +    oneOf:
>>>>>>> +      - items:
>>>>>>> +          - enum:
>>>>>>> +              - 0   # CMN PLL reference internal 48MHZ
>>>>>>> +              - 1   # CMN PLL reference external 25MHZ
>>>>>>> +              - 2   # CMN PLL reference external 31250KHZ
>>>>>>> +              - 3   # CMN PLL reference external 40MHZ
>>>>>>> +              - 4   # CMN PLL reference external 48MHZ
>>>>>>> +              - 5   # CMN PLL reference external 50MHZ
>>>>>>> +              - 6   # CMN PLL reference internal 96MHZ
>>>>>>
>>>>>> Why is this not represented by an element of the clocks property?
>>>>>
>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>> clock framework can't be used, which is the general hardware register
>>>>> instead of RCG register for GCC.
>>>>
>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>> or by some other clock controller or fixed clock makes a difference.
>>>> Why can't the other clock provider be represented in the devicetree?
>>>>
>>>
>>> cmn-reference-clock is for selecting the reference clock source for the
>>> whole Ethernet block, which is just the standalone configure register.
>>
>> Sure, you are aware though that all clocks are just configure registers?
>>
>> Which clocks are these mentioned in the property? From where do they come?
>>
>> Anyway, property is in existing form is not correct - this is not a
>> generic property.
>>
> 
> This property cmn-reference-clock is just the hardware register 
> configuration, since the different IPQ platform needs to select
> the different reference clock source for the CMN PLL block that
> provides the various clock outputs to the all kinds of Ethernet
> devices, which is not from GCC provider.

AGAIN: where do the clocks come from? Which device generates them?

> 
> This is indeed not a generic property, which is the Ethernet
> function configs same as clock-frequency.


Then it should not be made as a generic property...



Best regards,
Krzysztof

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

[Jie Luo]

On 12/15/2023 6:19 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 10:49, Jie Luo wrote:
>>
>>
>> On 12/15/2023 3:29 PM, Krzysztof Kozlowski wrote:
>>> On 15/12/2023 07:46, Jie Luo wrote:
>>>>
>>>>
>>>> On 12/15/2023 1:12 AM, Conor Dooley wrote:
>>>>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>>>>>
>>>>>>
>>>>>> On 12/13/2023 12:06 AM, Conor Dooley wrote:
>>>>>>> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>>>>>>>> Update the yaml file for the new DTS properties.
>>>>>>>>
>>>>>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>>>>>> 2. clock-frequency for MDIO clock frequency config.
>>>>>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>>>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>>>>>
>>>>>>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>>>>>>> ---
>>>>>>>>      .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>>>>>>>>      1 file changed, 153 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>>> index 3407e909e8a7..9546a6ad7841 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>>> @@ -20,6 +20,8 @@ properties:
>>>>>>>>                - enum:
>>>>>>>>                    - qcom,ipq6018-mdio
>>>>>>>>                    - qcom,ipq8074-mdio
>>>>>>>> +              - qcom,ipq9574-mdio
>>>>>>>> +              - qcom,ipq5332-mdio
>>>>>>>>                - const: qcom,ipq4019-mdio
>>>>>>>>        "#address-cells":
>>>>>>>> @@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
>>>>>>>> +      to select the reference clock.
>>>>>>>> +
>>>>>>>> +  reg-names:
>>>>>>>> +    minItems: 1
>>>>>>>> +    maxItems: 5
>>>>>>>>        clocks:
>>>>>>>> +    minItems: 1
>>>>>>>>          items:
>>>>>>>>            - description: MDIO clock source frequency fixed to 100MHZ
>>>>>>>> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>>>>>> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>>>>>> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>>>>>> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>>>>>>        clock-names:
>>>>>>>> +    minItems: 1
>>>>>>>>          items:
>>>>>>>>            - const: gcc_mdio_ahb_clk
>>>>>>>> +      - const: gcc_uniphy0_ahb_clk
>>>>>>>> +      - const: gcc_uniphy1_ahb_clk
>>>>>>>> +      - const: gcc_uniphy0_sys_clk
>>>>>>>> +      - const: gcc_uniphy1_sys_clk
>>>>>>>
>>>>>>>> +  cmn-reference-clock:
>>>>>>>> +    oneOf:
>>>>>>>> +      - items:
>>>>>>>> +          - enum:
>>>>>>>> +              - 0   # CMN PLL reference internal 48MHZ
>>>>>>>> +              - 1   # CMN PLL reference external 25MHZ
>>>>>>>> +              - 2   # CMN PLL reference external 31250KHZ
>>>>>>>> +              - 3   # CMN PLL reference external 40MHZ
>>>>>>>> +              - 4   # CMN PLL reference external 48MHZ
>>>>>>>> +              - 5   # CMN PLL reference external 50MHZ
>>>>>>>> +              - 6   # CMN PLL reference internal 96MHZ
>>>>>>>
>>>>>>> Why is this not represented by an element of the clocks property?
>>>>>>
>>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>>> clock framework can't be used, which is the general hardware register
>>>>>> instead of RCG register for GCC.
>>>>>
>>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>>> or by some other clock controller or fixed clock makes a difference.
>>>>> Why can't the other clock provider be represented in the devicetree?
>>>>>
>>>>
>>>> cmn-reference-clock is for selecting the reference clock source for the
>>>> whole Ethernet block, which is just the standalone configure register.
>>>
>>> Sure, you are aware though that all clocks are just configure registers?
>>>
>>> Which clocks are these mentioned in the property? From where do they come?
>>>
>>> Anyway, property is in existing form is not correct - this is not a
>>> generic property.
>>>
>>
>> This property cmn-reference-clock is just the hardware register
>> configuration, since the different IPQ platform needs to select
>> the different reference clock source for the CMN PLL block that
>> provides the various clock outputs to the all kinds of Ethernet
>> devices, which is not from GCC provider.
> 
> AGAIN: where do the clocks come from? Which device generates them?

Oh, OK, the reference clock is from wifi that provides 48MHZ to
Ethernet block.

> 
>>
>> This is indeed not a generic property, which is the Ethernet
>> function configs same as clock-frequency.
> 
> 
> Then it should not be made as a generic property...

sure, i saw your another comments, will prefix qcom_ on this property.

> 
> 
> 
> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

On 15/12/2023 11:33, Jie Luo wrote:
>>>>>>>>> +  cmn-reference-clock:
>>>>>>>>> +    oneOf:
>>>>>>>>> +      - items:
>>>>>>>>> +          - enum:
>>>>>>>>> +              - 0   # CMN PLL reference internal 48MHZ
>>>>>>>>> +              - 1   # CMN PLL reference external 25MHZ
>>>>>>>>> +              - 2   # CMN PLL reference external 31250KHZ
>>>>>>>>> +              - 3   # CMN PLL reference external 40MHZ
>>>>>>>>> +              - 4   # CMN PLL reference external 48MHZ
>>>>>>>>> +              - 5   # CMN PLL reference external 50MHZ
>>>>>>>>> +              - 6   # CMN PLL reference internal 96MHZ
>>>>>>>>
>>>>>>>> Why is this not represented by an element of the clocks property?
>>>>>>>
>>>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>>>> clock framework can't be used, which is the general hardware register
>>>>>>> instead of RCG register for GCC.
>>>>>>
>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>>>> or by some other clock controller or fixed clock makes a difference.
>>>>>> Why can't the other clock provider be represented in the devicetree?
>>>>>>
>>>>>
>>>>> cmn-reference-clock is for selecting the reference clock source for the
>>>>> whole Ethernet block, which is just the standalone configure register.
>>>>
>>>> Sure, you are aware though that all clocks are just configure registers?
>>>>
>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>
>>>> Anyway, property is in existing form is not correct - this is not a
>>>> generic property.
>>>>
>>>
>>> This property cmn-reference-clock is just the hardware register
>>> configuration, since the different IPQ platform needs to select
>>> the different reference clock source for the CMN PLL block that
>>> provides the various clock outputs to the all kinds of Ethernet
>>> devices, which is not from GCC provider.
>>
>> AGAIN: where do the clocks come from? Which device generates them?
> 
> Oh, OK, the reference clock is from wifi that provides 48MHZ to
> Ethernet block.

Then WiFi should be providing you the clock and this device should be
clock consumer, right?



Best regards,
Krzysztof

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

[Jie Luo]

On 12/15/2023 6:37 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 11:33, Jie Luo wrote:
>>>>>>>>>> +  cmn-reference-clock:
>>>>>>>>>> +    oneOf:
>>>>>>>>>> +      - items:
>>>>>>>>>> +          - enum:
>>>>>>>>>> +              - 0   # CMN PLL reference internal 48MHZ
>>>>>>>>>> +              - 1   # CMN PLL reference external 25MHZ
>>>>>>>>>> +              - 2   # CMN PLL reference external 31250KHZ
>>>>>>>>>> +              - 3   # CMN PLL reference external 40MHZ
>>>>>>>>>> +              - 4   # CMN PLL reference external 48MHZ
>>>>>>>>>> +              - 5   # CMN PLL reference external 50MHZ
>>>>>>>>>> +              - 6   # CMN PLL reference internal 96MHZ
>>>>>>>>>
>>>>>>>>> Why is this not represented by an element of the clocks property?
>>>>>>>>
>>>>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>>>>> clock framework can't be used, which is the general hardware register
>>>>>>>> instead of RCG register for GCC.
>>>>>>>
>>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>>>>> or by some other clock controller or fixed clock makes a difference.
>>>>>>> Why can't the other clock provider be represented in the devicetree?
>>>>>>>
>>>>>>
>>>>>> cmn-reference-clock is for selecting the reference clock source for the
>>>>>> whole Ethernet block, which is just the standalone configure register.
>>>>>
>>>>> Sure, you are aware though that all clocks are just configure registers?
>>>>>
>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>
>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>> generic property.
>>>>>
>>>>
>>>> This property cmn-reference-clock is just the hardware register
>>>> configuration, since the different IPQ platform needs to select
>>>> the different reference clock source for the CMN PLL block that
>>>> provides the various clock outputs to the all kinds of Ethernet
>>>> devices, which is not from GCC provider.
>>>
>>> AGAIN: where do the clocks come from? Which device generates them?
>>
>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>> Ethernet block.
> 
> Then WiFi should be providing you the clock and this device should be
> clock consumer, right?

Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
for this 48MHZ clock output, it is the hardware PIN connection.
> 
> 
> 
> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

On 15/12/2023 11:40, Jie Luo wrote:
> 
> 
> On 12/15/2023 6:37 PM, Krzysztof Kozlowski wrote:
>> On 15/12/2023 11:33, Jie Luo wrote:
>>>>>>>>>>> +  cmn-reference-clock:
>>>>>>>>>>> +    oneOf:
>>>>>>>>>>> +      - items:
>>>>>>>>>>> +          - enum:
>>>>>>>>>>> +              - 0   # CMN PLL reference internal 48MHZ
>>>>>>>>>>> +              - 1   # CMN PLL reference external 25MHZ
>>>>>>>>>>> +              - 2   # CMN PLL reference external 31250KHZ
>>>>>>>>>>> +              - 3   # CMN PLL reference external 40MHZ
>>>>>>>>>>> +              - 4   # CMN PLL reference external 48MHZ
>>>>>>>>>>> +              - 5   # CMN PLL reference external 50MHZ
>>>>>>>>>>> +              - 6   # CMN PLL reference internal 96MHZ
>>>>>>>>>>
>>>>>>>>>> Why is this not represented by an element of the clocks property?
>>>>>>>>>
>>>>>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>>>>>> clock framework can't be used, which is the general hardware register
>>>>>>>>> instead of RCG register for GCC.
>>>>>>>>
>>>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>>>>>> or by some other clock controller or fixed clock makes a difference.
>>>>>>>> Why can't the other clock provider be represented in the devicetree?
>>>>>>>>
>>>>>>>
>>>>>>> cmn-reference-clock is for selecting the reference clock source for the
>>>>>>> whole Ethernet block, which is just the standalone configure register.
>>>>>>
>>>>>> Sure, you are aware though that all clocks are just configure registers?
>>>>>>
>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>
>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>> generic property.
>>>>>>
>>>>>
>>>>> This property cmn-reference-clock is just the hardware register
>>>>> configuration, since the different IPQ platform needs to select
>>>>> the different reference clock source for the CMN PLL block that
>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>> devices, which is not from GCC provider.
>>>>
>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>
>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>> Ethernet block.
>>
>> Then WiFi should be providing you the clock and this device should be
>> clock consumer, right?
> 
> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
> for this 48MHZ clock output, it is the hardware PIN connection.

All clocks are some hardware pin connections.

Best regards,
Krzysztof

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

[Jie Luo]

On 12/15/2023 6:53 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 11:40, Jie Luo wrote:
>>
>>
>> On 12/15/2023 6:37 PM, Krzysztof Kozlowski wrote:
>>> On 15/12/2023 11:33, Jie Luo wrote:
>>>>>>>>>>>> +  cmn-reference-clock:
>>>>>>>>>>>> +    oneOf:
>>>>>>>>>>>> +      - items:
>>>>>>>>>>>> +          - enum:
>>>>>>>>>>>> +              - 0   # CMN PLL reference internal 48MHZ
>>>>>>>>>>>> +              - 1   # CMN PLL reference external 25MHZ
>>>>>>>>>>>> +              - 2   # CMN PLL reference external 31250KHZ
>>>>>>>>>>>> +              - 3   # CMN PLL reference external 40MHZ
>>>>>>>>>>>> +              - 4   # CMN PLL reference external 48MHZ
>>>>>>>>>>>> +              - 5   # CMN PLL reference external 50MHZ
>>>>>>>>>>>> +              - 6   # CMN PLL reference internal 96MHZ
>>>>>>>>>>>
>>>>>>>>>>> Why is this not represented by an element of the clocks property?
>>>>>>>>>>
>>>>>>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>>>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>>>>>>> clock framework can't be used, which is the general hardware register
>>>>>>>>>> instead of RCG register for GCC.
>>>>>>>>>
>>>>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>>>>>>> or by some other clock controller or fixed clock makes a difference.
>>>>>>>>> Why can't the other clock provider be represented in the devicetree?
>>>>>>>>>
>>>>>>>>
>>>>>>>> cmn-reference-clock is for selecting the reference clock source for the
>>>>>>>> whole Ethernet block, which is just the standalone configure register.
>>>>>>>
>>>>>>> Sure, you are aware though that all clocks are just configure registers?
>>>>>>>
>>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>>
>>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>>> generic property.
>>>>>>>
>>>>>>
>>>>>> This property cmn-reference-clock is just the hardware register
>>>>>> configuration, since the different IPQ platform needs to select
>>>>>> the different reference clock source for the CMN PLL block that
>>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>>> devices, which is not from GCC provider.
>>>>>
>>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>>
>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>>> Ethernet block.
>>>
>>> Then WiFi should be providing you the clock and this device should be
>>> clock consumer, right?
>>
>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
>> for this 48MHZ clock output, it is the hardware PIN connection.
> 
> All clocks are some hardware pin connections.
> 
> Best regards,
> Krzysztof
> 

Yes, all reference clocks here are from hardware pin connection.

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

[Krzysztof Kozlowski]

On 15/12/2023 12:42, Jie Luo wrote:
>>>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>>>
>>>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>>>> generic property.
>>>>>>>>
>>>>>>>
>>>>>>> This property cmn-reference-clock is just the hardware register
>>>>>>> configuration, since the different IPQ platform needs to select
>>>>>>> the different reference clock source for the CMN PLL block that
>>>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>>>> devices, which is not from GCC provider.
>>>>>>
>>>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>>>
>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>>>> Ethernet block.
>>>>
>>>> Then WiFi should be providing you the clock and this device should be
>>>> clock consumer, right?
>>>
>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
>>> for this 48MHZ clock output, it is the hardware PIN connection.
>>
>> All clocks are some hardware pin connections.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Yes, all reference clocks here are from hardware pin connection.

You keep answering with short sentences without touching the root of the
problem. I don't know exactly why, but I feel this discussion leads
nowhere. After long discussion you finally admitted that clocks came
from another device - Wifi. It took us like 6 emails?

So last statement: if you have clock provider and clock consumer, you
must represent it in the bindings or provide rationale why it should not
or must not be represented in the bindings. So far I do not see any of
such arguments.

If you use arguments like:
"My driver....": sorry, bindings are not about drivers
"I don't have clock driver for WiFi": sorry, it does not matter if you
can write one, right?

Please reach internally your colleagues to solve these problems and make
review process smoother.

Best regards,
Krzysztof

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

[Jie Luo]

On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 12:42, Jie Luo wrote:
>>>>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>>>>
>>>>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>>>>> generic property.
>>>>>>>>>
>>>>>>>>
>>>>>>>> This property cmn-reference-clock is just the hardware register
>>>>>>>> configuration, since the different IPQ platform needs to select
>>>>>>>> the different reference clock source for the CMN PLL block that
>>>>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>>>>> devices, which is not from GCC provider.
>>>>>>>
>>>>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>>>>
>>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>>>>> Ethernet block.
>>>>>
>>>>> Then WiFi should be providing you the clock and this device should be
>>>>> clock consumer, right?
>>>>
>>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
>>>> for this 48MHZ clock output, it is the hardware PIN connection.
>>>
>>> All clocks are some hardware pin connections.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Yes, all reference clocks here are from hardware pin connection.
> 
> You keep answering with short sentences without touching the root of the
> problem. I don't know exactly why, but I feel this discussion leads
> nowhere. After long discussion you finally admitted that clocks came
> from another device - Wifi. It took us like 6 emails?
> 
> So last statement: if you have clock provider and clock consumer, you
> must represent it in the bindings or provide rationale why it should not
> or must not be represented in the bindings. So far I do not see any of
> such arguments.
> 
> If you use arguments like:
> "My driver....": sorry, bindings are not about drivers
> "I don't have clock driver for WiFi": sorry, it does not matter if you
> can write one, right?
> 
> Please reach internally your colleagues to solve these problems and make
> review process smoother.
> 
> Best regards,
> Krzysztof
> 
> 

These reference clocks source do not need the hardware configuration,
that is the reason why the clock provider is not needed, some reference
clock source are even from external crystal.

There is also no enable control for the reference clocks since it is
inputted by the hardware PIN connection, i will update these description
in the DT to make it more clear.

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

[Andrew Lunn]

> > You keep answering with short sentences without touching the root of the
> > problem. I don't know exactly why, but I feel this discussion leads
> > nowhere. After long discussion you finally admitted that clocks came
> > from another device - Wifi. It took us like 6 emails?
> > 
> > So last statement: if you have clock provider and clock consumer, you
> > must represent it in the bindings or provide rationale why it should not
> > or must not be represented in the bindings. So far I do not see any of
> > such arguments.
> > 
> > If you use arguments like:
> > "My driver....": sorry, bindings are not about drivers
> > "I don't have clock driver for WiFi": sorry, it does not matter if you
> > can write one, right?
> > 
> > Please reach internally your colleagues to solve these problems and make
> > review process smoother.

Yes, i strongly agree with this. Its not our job as maintainers to
educate big companies like Qualcomm how to write Linux drivers. There
are more experienced driver writer within Qualcomm, you need to make
contact with them, and get them to help you. Or you need to outsource
the driver development to one of the companies which write mainline
Linux drivers.

	Andrew

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

[Jie Luo]

On 12/15/2023 9:39 PM, Andrew Lunn wrote:
>>> You keep answering with short sentences without touching the root of the
>>> problem. I don't know exactly why, but I feel this discussion leads
>>> nowhere. After long discussion you finally admitted that clocks came
>>> from another device - Wifi. It took us like 6 emails?
>>>
>>> So last statement: if you have clock provider and clock consumer, you
>>> must represent it in the bindings or provide rationale why it should not
>>> or must not be represented in the bindings. So far I do not see any of
>>> such arguments.
>>>
>>> If you use arguments like:
>>> "My driver....": sorry, bindings are not about drivers
>>> "I don't have clock driver for WiFi": sorry, it does not matter if you
>>> can write one, right?
>>>
>>> Please reach internally your colleagues to solve these problems and make
>>> review process smoother.
> 
> Yes, i strongly agree with this. Its not our job as maintainers to
> educate big companies like Qualcomm how to write Linux drivers. There
> are more experienced driver writer within Qualcomm, you need to make
> contact with them, and get them to help you. Or you need to outsource
> the driver development to one of the companies which write mainline
> Linux drivers.
> 
> 	Andrew

Understand this, sorry for some easy mistakes happens here.
i will sync with internal team before pushing the next patch set.

Thanks a lot.

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

[Conor Dooley]

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

On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
> 
> 
> On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
> > On 15/12/2023 12:42, Jie Luo wrote:
> > > > > > > > > > Which clocks are these mentioned in the property? From where do they come?
> > > > > > > > > > 
> > > > > > > > > > Anyway, property is in existing form is not correct - this is not a
> > > > > > > > > > generic property.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > This property cmn-reference-clock is just the hardware register
> > > > > > > > > configuration, since the different IPQ platform needs to select
> > > > > > > > > the different reference clock source for the CMN PLL block that
> > > > > > > > > provides the various clock outputs to the all kinds of Ethernet
> > > > > > > > > devices, which is not from GCC provider.
> > > > > > > > 
> > > > > > > > AGAIN: where do the clocks come from? Which device generates them?
> > > > > > > 
> > > > > > > Oh, OK, the reference clock is from wifi that provides 48MHZ to
> > > > > > > Ethernet block.
> > > > > > 
> > > > > > Then WiFi should be providing you the clock and this device should be
> > > > > > clock consumer, right?
> > > > > 
> > > > > Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
> > > > > for this 48MHZ clock output, it is the hardware PIN connection.
> > > > 
> > > > All clocks are some hardware pin connections.
> > > > 
> > > > Best regards,
> > > > Krzysztof
> > > > 
> > > 
> > > Yes, all reference clocks here are from hardware pin connection.
> > 
> > You keep answering with short sentences without touching the root of the
> > problem. I don't know exactly why, but I feel this discussion leads
> > nowhere. After long discussion you finally admitted that clocks came
> > from another device - Wifi. It took us like 6 emails?
> > 
> > So last statement: if you have clock provider and clock consumer, you
> > must represent it in the bindings or provide rationale why it should not
> > or must not be represented in the bindings. So far I do not see any of
> > such arguments.
> > 
> > If you use arguments like:
> > "My driver....": sorry, bindings are not about drivers
> > "I don't have clock driver for WiFi": sorry, it does not matter if you
> > can write one, right?
> > 
> > Please reach internally your colleagues to solve these problems and make
> > review process smoother.

> These reference clocks source do not need the hardware configuration,
> that is the reason why the clock provider is not needed, some reference
> clock source are even from external crystal.

I fail to understand how that makes this clock different to the clocks
on any other platform. Clocks from external crystals are present in many
many systems. See for example fixed-clock.yaml.

> There is also no enable control for the reference clocks since it is
> inputted by the hardware PIN connection, i will update these description
> in the DT to make it more clear.

Again, this does not justify having custom properties for this clock,
as it is no different to other platforms. As far as I can tell, the only
thing that a standard "clocks" property cannot convey here is the
internal reference. I would suggest that since there is only one
internal clock frequency, the absence of this particular clock in the
"clocks" property can be used to determine that the reference is the
internal one.

Thanks,
Conor.

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

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

[Jie Luo]

On 12/15/2023 9:41 PM, Conor Dooley wrote:
> On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
>>
>>
>> On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
>>> On 15/12/2023 12:42, Jie Luo wrote:
>>>>>>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>>>>>>
>>>>>>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>>>>>>> generic property.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This property cmn-reference-clock is just the hardware register
>>>>>>>>>> configuration, since the different IPQ platform needs to select
>>>>>>>>>> the different reference clock source for the CMN PLL block that
>>>>>>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>>>>>>> devices, which is not from GCC provider.
>>>>>>>>>
>>>>>>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>>>>>>
>>>>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>>>>>>> Ethernet block.
>>>>>>>
>>>>>>> Then WiFi should be providing you the clock and this device should be
>>>>>>> clock consumer, right?
>>>>>>
>>>>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
>>>>>> for this 48MHZ clock output, it is the hardware PIN connection.
>>>>>
>>>>> All clocks are some hardware pin connections.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>
>>>> Yes, all reference clocks here are from hardware pin connection.
>>>
>>> You keep answering with short sentences without touching the root of the
>>> problem. I don't know exactly why, but I feel this discussion leads
>>> nowhere. After long discussion you finally admitted that clocks came
>>> from another device - Wifi. It took us like 6 emails?
>>>
>>> So last statement: if you have clock provider and clock consumer, you
>>> must represent it in the bindings or provide rationale why it should not
>>> or must not be represented in the bindings. So far I do not see any of
>>> such arguments.
>>>
>>> If you use arguments like:
>>> "My driver....": sorry, bindings are not about drivers
>>> "I don't have clock driver for WiFi": sorry, it does not matter if you
>>> can write one, right?
>>>
>>> Please reach internally your colleagues to solve these problems and make
>>> review process smoother.
> 
>> These reference clocks source do not need the hardware configuration,
>> that is the reason why the clock provider is not needed, some reference
>> clock source are even from external crystal.
> 
> I fail to understand how that makes this clock different to the clocks
> on any other platform. Clocks from external crystals are present in many
> many systems. See for example fixed-clock.yaml.

Hi Conor,
The reference clock rate has no meaning to the CMN PLL block, since the
software can't control the behavior of CMN PLL, and various output
clocks of CMN PLL block are fixed, adding this custom property is just
for selecting the different reference clock source, since different
IPQ platform needs to be configured the different reference clock source
for the CMN PLL block.

let's say if we register 48MHZ reference clock as the fix clock, we
can't distinguish it is internal 48MHZ or external 48MHZ, for these
two reference clock sources, there are different hardware configuration
of CMN PLL block, and this reference clock selection is not applicable
for the IPQ4019 platform.

> 
>> There is also no enable control for the reference clocks since it is
>> inputted by the hardware PIN connection, i will update these description
>> in the DT to make it more clear.
> 
> Again, this does not justify having custom properties for this clock,
> as it is no different to other platforms. As far as I can tell, the only
> thing that a standard "clocks" property cannot convey here is the
> internal reference. I would suggest that since there is only one
> internal clock frequency, the absence of this particular clock in the
> "clocks" property can be used to determine that the reference is the
> internal one.
> 
> Thanks,
> Conor.

Yes, we can get the clock rate of the clocks property if we register
these as the fix clock to distinguish the different clock source.

Since the reference clock rate value has no matter with the CMN clock
configuration, it is just the reference clock source selection, so
i did not use the fix clock for this.

Thanks for this suggestion, i will verify the fix clock register solution.

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

[Conor Dooley]

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

On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote:
> 
> 
> On 12/15/2023 9:41 PM, Conor Dooley wrote:
> > On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
> > > 
> > > 
> > > On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
> > > > On 15/12/2023 12:42, Jie Luo wrote:
> > > > > > > > > > > > Which clocks are these mentioned in the property? From where do they come?
> > > > > > > > > > > > 
> > > > > > > > > > > > Anyway, property is in existing form is not correct - this is not a
> > > > > > > > > > > > generic property.
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > This property cmn-reference-clock is just the hardware register
> > > > > > > > > > > configuration, since the different IPQ platform needs to select
> > > > > > > > > > > the different reference clock source for the CMN PLL block that
> > > > > > > > > > > provides the various clock outputs to the all kinds of Ethernet
> > > > > > > > > > > devices, which is not from GCC provider.
> > > > > > > > > > 
> > > > > > > > > > AGAIN: where do the clocks come from? Which device generates them?
> > > > > > > > > 
> > > > > > > > > Oh, OK, the reference clock is from wifi that provides 48MHZ to
> > > > > > > > > Ethernet block.
> > > > > > > > 
> > > > > > > > Then WiFi should be providing you the clock and this device should be
> > > > > > > > clock consumer, right?
> > > > > > > 
> > > > > > > Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
> > > > > > > for this 48MHZ clock output, it is the hardware PIN connection.
> > > > > > 
> > > > > > All clocks are some hardware pin connections.
> > > > > > 
> > > > > > Best regards,
> > > > > > Krzysztof
> > > > > > 
> > > > > 
> > > > > Yes, all reference clocks here are from hardware pin connection.
> > > > 
> > > > You keep answering with short sentences without touching the root of the
> > > > problem. I don't know exactly why, but I feel this discussion leads
> > > > nowhere. After long discussion you finally admitted that clocks came
> > > > from another device - Wifi. It took us like 6 emails?
> > > > 
> > > > So last statement: if you have clock provider and clock consumer, you
> > > > must represent it in the bindings or provide rationale why it should not
> > > > or must not be represented in the bindings. So far I do not see any of
> > > > such arguments.
> > > > 
> > > > If you use arguments like:
> > > > "My driver....": sorry, bindings are not about drivers
> > > > "I don't have clock driver for WiFi": sorry, it does not matter if you
> > > > can write one, right?
> > > > 
> > > > Please reach internally your colleagues to solve these problems and make
> > > > review process smoother.
> > 
> > > These reference clocks source do not need the hardware configuration,
> > > that is the reason why the clock provider is not needed, some reference
> > > clock source are even from external crystal.
> > 
> > I fail to understand how that makes this clock different to the clocks
> > on any other platform. Clocks from external crystals are present in many
> > many systems. See for example fixed-clock.yaml.
> 
> The reference clock rate has no meaning to the CMN PLL block, since the
> software can't control the behavior of CMN PLL, and various output
> clocks of CMN PLL block are fixed, adding this custom property is just
> for selecting the different reference clock source, since different
> IPQ platform needs to be configured the different reference clock source
> for the CMN PLL block.

Many, many other systems are in the same situation, where clocks are
provided to a peripheral that has no control over the clock rate, but
has to pick internal dividers or set bits in a config register depending
on what clock rate is provided to it. That is not something special
about this particular platform and other systems are able to use the
clocks property for this purpose.

> let's say if we register 48MHZ reference clock as the fix clock, we
> can't distinguish it is internal 48MHZ or external 48MHZ, for these
> two reference clock sources, there are different hardware configuration
> of CMN PLL block

That's easy, if the reference is external, it is provided by the clocks
property. If it internal, then there will be no clocks property
providing it.

> and this reference clock selection is not applicable
> for the IPQ4019 platform.

Isn't this a patch for the IPQ4019? Why would it not be relevant?

> > > There is also no enable control for the reference clocks since it is
> > > inputted by the hardware PIN connection, i will update these description
> > > in the DT to make it more clear.
> > 
> > Again, this does not justify having custom properties for this clock,
> > as it is no different to other platforms. As far as I can tell, the only
> > thing that a standard "clocks" property cannot convey here is the
> > internal reference. I would suggest that since there is only one
> > internal clock frequency, the absence of this particular clock in the
> > "clocks" property can be used to determine that the reference is the
> > internal one.

I'm surprised you didn't pick up on this, but there are actually _2_
internal references, which I have just noticed while double checking the
binding patch.

What is the impact of using the 48 MHz or 96 MHz internal reference?

Thanks,
Conor.

> Yes, we can get the clock rate of the clocks property if we register
> these as the fix clock to distinguish the different clock source.
> 
> Since the reference clock rate value has no matter with the CMN clock
> configuration, it is just the reference clock source selection, so
> i did not use the fix clock for this.
>
> Thanks for this suggestion, i will verify the fix clock register solution.

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

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

[Jie Luo]

On 12/16/2023 10:16 PM, Conor Dooley wrote:
> On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote:
>>
>>
>> On 12/15/2023 9:41 PM, Conor Dooley wrote:
>>> On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
>>>>
>>>>
>>>> On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
>>>>> On 15/12/2023 12:42, Jie Luo wrote:
>>>>>>>>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>>>>>>>>> generic property.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> This property cmn-reference-clock is just the hardware register
>>>>>>>>>>>> configuration, since the different IPQ platform needs to select
>>>>>>>>>>>> the different reference clock source for the CMN PLL block that
>>>>>>>>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>>>>>>>>> devices, which is not from GCC provider.
>>>>>>>>>>>
>>>>>>>>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>>>>>>>>
>>>>>>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>>>>>>>>> Ethernet block.
>>>>>>>>>
>>>>>>>>> Then WiFi should be providing you the clock and this device should be
>>>>>>>>> clock consumer, right?
>>>>>>>>
>>>>>>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
>>>>>>>> for this 48MHZ clock output, it is the hardware PIN connection.
>>>>>>>
>>>>>>> All clocks are some hardware pin connections.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Krzysztof
>>>>>>>
>>>>>>
>>>>>> Yes, all reference clocks here are from hardware pin connection.
>>>>>
>>>>> You keep answering with short sentences without touching the root of the
>>>>> problem. I don't know exactly why, but I feel this discussion leads
>>>>> nowhere. After long discussion you finally admitted that clocks came
>>>>> from another device - Wifi. It took us like 6 emails?
>>>>>
>>>>> So last statement: if you have clock provider and clock consumer, you
>>>>> must represent it in the bindings or provide rationale why it should not
>>>>> or must not be represented in the bindings. So far I do not see any of
>>>>> such arguments.
>>>>>
>>>>> If you use arguments like:
>>>>> "My driver....": sorry, bindings are not about drivers
>>>>> "I don't have clock driver for WiFi": sorry, it does not matter if you
>>>>> can write one, right?
>>>>>
>>>>> Please reach internally your colleagues to solve these problems and make
>>>>> review process smoother.
>>>
>>>> These reference clocks source do not need the hardware configuration,
>>>> that is the reason why the clock provider is not needed, some reference
>>>> clock source are even from external crystal.
>>>
>>> I fail to understand how that makes this clock different to the clocks
>>> on any other platform. Clocks from external crystals are present in many
>>> many systems. See for example fixed-clock.yaml.
>>
>> The reference clock rate has no meaning to the CMN PLL block, since the
>> software can't control the behavior of CMN PLL, and various output
>> clocks of CMN PLL block are fixed, adding this custom property is just
>> for selecting the different reference clock source, since different
>> IPQ platform needs to be configured the different reference clock source
>> for the CMN PLL block.
> 
> Many, many other systems are in the same situation, where clocks are
> provided to a peripheral that has no control over the clock rate, but
> has to pick internal dividers or set bits in a config register depending
> on what clock rate is provided to it. That is not something special
> about this particular platform and other systems are able to use the
> clocks property for this purpose.

Sure, Thanks Conor for this information.
i will try to replace this custom property with clocks property and
verify the drive.

> 
>> let's say if we register 48MHZ reference clock as the fix clock, we
>> can't distinguish it is internal 48MHZ or external 48MHZ, for these
>> two reference clock sources, there are different hardware configuration
>> of CMN PLL block
> 
> That's easy, if the reference is external, it is provided by the clocks
> property. If it internal, then there will be no clocks property
> providing it.

Thanks for this detail.

> 
>> and this reference clock selection is not applicable
>> for the IPQ4019 platform.
> 
> Isn't this a patch for the IPQ4019? Why would it not be relevant?
IPQ4019 is the legacy chip, and the same MDIO bus driver is also
extended to support the new IPQ platform, since the MDIO hardware
is leveraged by the new IPQ platform.

For the CMN PLL block, which is not existed on the legacy platform
IPQ4019, but it does not matter, we can also distinguish it according
to the CMN register base defined or not, the CMN reference clocks is
configured only when the CMN register base defined in the reg  property.

> 
>>>> There is also no enable control for the reference clocks since it is
>>>> inputted by the hardware PIN connection, i will update these description
>>>> in the DT to make it more clear.
>>>
>>> Again, this does not justify having custom properties for this clock,
>>> as it is no different to other platforms. As far as I can tell, the only
>>> thing that a standard "clocks" property cannot convey here is the
>>> internal reference. I would suggest that since there is only one
>>> internal clock frequency, the absence of this particular clock in the
>>> "clocks" property can be used to determine that the reference is the
>>> internal on
> 
> I'm surprised you didn't pick up on this, but there are actually _2_
> internal references, which I have just noticed while double checking the
> binding patch.

i noticed this, the reference clock source can be supported by clocks as
you suggested here, it is really helpful.
> 
> What is the impact of using the 48 MHz or 96 MHz internal reference?
They works on the different IPQ platform, 96MHZ internal reference is
used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is
same as what you describe above, the different clock source rate is
selected as the different register value, then the PLL can do the
corresponding config to output the correct clock rate, the external
clock source is also same if the clock rate is same, just the different
hardware PIN is selected if the external reference source is configured.

Thanks.

> 
> Thanks,
> Conor.
> 
>> Yes, we can get the clock rate of the clocks property if we register
>> these as the fix clock to distinguish the different clock source.
>>
>> Since the reference clock rate value has no matter with the CMN clock
>> configuration, it is just the reference clock source selection, so
>> i did not use the fix clock for this.
>>
>> Thanks for this suggestion, i will verify the fix clock register solution.

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

[Conor Dooley]

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

On Sat, Dec 16, 2023 at 11:37:08PM +0800, Jie Luo wrote:
> On 12/16/2023 10:16 PM, Conor Dooley wrote:
> > On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote:
> > > On 12/15/2023 9:41 PM, Conor Dooley wrote:
> > > > On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
> > > > > On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
> > > > > > On 15/12/2023 12:42, Jie Luo wrote:

> > > > > There is also no enable control for the reference clocks since it is
> > > > > inputted by the hardware PIN connection, i will update these description
> > > > > in the DT to make it more clear.
> > > > 
> > > > Again, this does not justify having custom properties for this clock,
> > > > as it is no different to other platforms. As far as I can tell, the only
> > > > thing that a standard "clocks" property cannot convey here is the
> > > > internal reference. I would suggest that since there is only one
> > > > internal clock frequency, the absence of this particular clock in the
> > > > "clocks" property can be used to determine that the reference is the
> > > > internal on
> > 
> > I'm surprised you didn't pick up on this, but there are actually _2_
> > internal references, which I have just noticed while double checking the
> > binding patch.
> 
> i noticed this, the reference clock source can be supported by clocks as
> you suggested here, it is really helpful.
> 
> > What is the impact of using the 48 MHz or 96 MHz internal reference?
> They works on the different IPQ platform, 96MHZ internal reference is
> used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is
> same as what you describe above, the different clock source rate is
> selected as the different register value, then the PLL can do the
> corresponding config to output the correct clock rate, the external
> clock source is also same if the clock rate is same, just the different
> hardware PIN is selected if the external reference source is configured.


Ah, so there is only one internal reference frequency per device. Then
my suggestion to use the presence of the clock in the clocks property
should work, just the fallback to the internal reference is going to
depend on the compatible.

Thanks,
Conor.

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

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

[Jie Luo]

On 12/19/2023 11:47 PM, Conor Dooley wrote:
> On Sat, Dec 16, 2023 at 11:37:08PM +0800, Jie Luo wrote:
>> On 12/16/2023 10:16 PM, Conor Dooley wrote:
>>> On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote:
>>>> On 12/15/2023 9:41 PM, Conor Dooley wrote:
>>>>> On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
>>>>>> On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 15/12/2023 12:42, Jie Luo wrote:
> 
>>>>>> There is also no enable control for the reference clocks since it is
>>>>>> inputted by the hardware PIN connection, i will update these description
>>>>>> in the DT to make it more clear.
>>>>>
>>>>> Again, this does not justify having custom properties for this clock,
>>>>> as it is no different to other platforms. As far as I can tell, the only
>>>>> thing that a standard "clocks" property cannot convey here is the
>>>>> internal reference. I would suggest that since there is only one
>>>>> internal clock frequency, the absence of this particular clock in the
>>>>> "clocks" property can be used to determine that the reference is the
>>>>> internal on
>>>
>>> I'm surprised you didn't pick up on this, but there are actually _2_
>>> internal references, which I have just noticed while double checking the
>>> binding patch.
>>
>> i noticed this, the reference clock source can be supported by clocks as
>> you suggested here, it is really helpful.
>>
>>> What is the impact of using the 48 MHz or 96 MHz internal reference?
>> They works on the different IPQ platform, 96MHZ internal reference is
>> used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is
>> same as what you describe above, the different clock source rate is
>> selected as the different register value, then the PLL can do the
>> corresponding config to output the correct clock rate, the external
>> clock source is also same if the clock rate is same, just the different
>> hardware PIN is selected if the external reference source is configured.
> 
> 
> Ah, so there is only one internal reference frequency per device. Then
> my suggestion to use the presence of the clock in the clocks property
> should work, just the fallback to the internal reference is going to
> depend on the compatible.
> 
> Thanks,
> Conor.

The reference clock source is configurable, normally there is the fix
reference clock configured per each IPQ platform, but we should keep
the reference clock source configurable in case of the reference clock
source switch needed in the future.

you are right, the reference clock source can be distinguished by
checking the clock rate and the compatible string.

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

[Krzysztof Kozlowski]

On 16/12/2023 16:37, Jie Luo wrote:
>>
>> I'm surprised you didn't pick up on this, but there are actually _2_
>> internal references, which I have just noticed while double checking the
>> binding patch.
> 
> i noticed this, the reference clock source can be supported by clocks as
> you suggested here, it is really helpful.
>>
>> What is the impact of using the 48 MHz or 96 MHz internal reference?
> They works on the different IPQ platform, 96MHZ internal reference is
> used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is

So the binding is just incorrect. Why do you even consider configuring
96 MHz internal reference on IPQ5332?


Best regards,
Krzysztof

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

[Jie Luo]

On 12/20/2023 3:28 PM, Krzysztof Kozlowski wrote:
> On 16/12/2023 16:37, Jie Luo wrote:
>>>
>>> I'm surprised you didn't pick up on this, but there are actually _2_
>>> internal references, which I have just noticed while double checking the
>>> binding patch.
>>
>> i noticed this, the reference clock source can be supported by clocks as
>> you suggested here, it is really helpful.
>>>
>>> What is the impact of using the 48 MHz or 96 MHz internal reference?
>> They works on the different IPQ platform, 96MHZ internal reference is
>> used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is
> 
> So the binding is just incorrect. Why do you even consider configuring
> 96 MHz internal reference on IPQ5332?
> 
> 
> Best regards,
> Krzysztof
> 

Normally there is the fix reference clock source used per each IPQ
platform, but we should keep it configurable, the CMN PLL block is
same among the supported IPQ platforms, Maybe there is special case
to switch the reference clock in the future.
i will also update the dtbinding doc to limit the reference clock based
on the compatible string.

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

[Conor Dooley]

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

> > > This is indeed not a generic property, which is the Ethernet
> > > function configs same as clock-frequency.
> > 
> > 
> > Then it should not be made as a generic property...
> 
> sure, i saw your another comments, will prefix qcom_ on this property.

iff the property stays, that would be a prefix of "qcom," not "qcom_".

Cheers,
Conor.

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

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

[Jie Luo]

On 12/15/2023 6:42 PM, Conor Dooley wrote:
>>>> This is indeed not a generic property, which is the Ethernet
>>>> function configs same as clock-frequency.
>>>
>>>
>>> Then it should not be made as a generic property...
>>
>> sure, i saw your another comments, will prefix qcom_ on this property.
> 
> iff the property stays, that would be a prefix of "qcom," not "qcom_".
> 
> Cheers,
> Conor.

Ok, thanks.

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

[Rob Herring]

On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
> Update the yaml file for the new DTS properties.
> 
> 1. cmn-reference-clock for the CMN PLL source clock select.
> 2. clock-frequency for MDIO clock frequency config.
> 3. add uniphy AHB & SYS GCC clocks.
> 4. add reset-gpios for MDIO bus level reset.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>  1 file changed, 153 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> index 3407e909e8a7..9546a6ad7841 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> @@ -20,6 +20,8 @@ properties:
>            - enum:
>                - qcom,ipq6018-mdio
>                - qcom,ipq8074-mdio
> +              - qcom,ipq9574-mdio
> +              - qcom,ipq5332-mdio
>            - const: qcom,ipq4019-mdio

A driver can function without knowing about all these new registers and 
clocks? If not, then it can't be compatible with "qcom,ipq4019-mdio".

>  
>    "#address-cells":
> @@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
> +      to select the reference clock.
> +
> +  reg-names:
> +    minItems: 1
> +    maxItems: 5
>  
>    clocks:
> +    minItems: 1
>      items:
>        - description: MDIO clock source frequency fixed to 100MHZ
> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ

These are all clock inputs to this h/w block and not some other clocks 
you want to manage?

>  
>    clock-names:
> +    minItems: 1
>      items:
>        - const: gcc_mdio_ahb_clk
> +      - const: gcc_uniphy0_ahb_clk
> +      - const: gcc_uniphy1_ahb_clk
> +      - const: gcc_uniphy0_sys_clk
> +      - const: gcc_uniphy1_sys_clk

"gcc" is presumably the name of the clock controller in QCom chips. 
Well, the clock source should not be part of the binding. The names 
should be local for what they are for. So drop 'gcc_'. And '_clk' is 
also redundant, so drop it too. Unfortunately you are stuck with the 
name of the 1st entry.

> +
> +  cmn-reference-clock:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - 0   # CMN PLL reference internal 48MHZ
> +              - 1   # CMN PLL reference external 25MHZ
> +              - 2   # CMN PLL reference external 31250KHZ
> +              - 3   # CMN PLL reference external 40MHZ
> +              - 4   # CMN PLL reference external 48MHZ
> +              - 5   # CMN PLL reference external 50MHZ
> +              - 6   # CMN PLL reference internal 96MHZ
> +
> +  clock-frequency:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - 12500000
> +              - 6250000
> +              - 3125000
> +              - 1562500
> +              - 781250
> +              - 390625
> +    description:
> +      The MDIO bus clock that must be output by the MDIO bus hardware,
> +      only the listed frequecies above can be configured, other frequency
> +      will cause malfunction. If absent, the default hardware value is used.
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  reset-assert-us:
> +    maxItems: 1
> +
> +  reset-deassert-us:
> +    maxItems: 1
>  
>  required:
>    - compatible
> @@ -61,6 +115,8 @@ allOf:
>                - qcom,ipq5018-mdio
>                - qcom,ipq6018-mdio
>                - qcom,ipq8074-mdio
> +              - qcom,ipq5332-mdio
> +              - qcom,ipq9574-mdio
>      then:
>        required:
>          - clocks
> @@ -70,6 +126,40 @@ allOf:
>          clocks: false
>          clock-names: false
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq5332-mdio
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 5
> +          maxItems: 5
> +        reg-names:
> +          items:
> +            - const: mdio
> +            - const: eth_ldo1
> +            - const: eth_ldo2
> +            - const: cmn_blk

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


> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq9574-mdio
> +    then:
> +      properties:
> +        reg-names:
> +          items:
> +            - const: mdio
> +            - const: eth_ldo1
> +            - const: eth_ldo2
> +            - const: eth_ldo3
> +            - const: cmn_blk

And 'minItems: 5' here.

The ipq9574 adds the CMN block, but none of the clocks? Weird.

Rob

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

[Jie Luo]

On 12/13/2023 4:06 AM, Rob Herring wrote:
> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>> Update the yaml file for the new DTS properties.
>>
>> 1. cmn-reference-clock for the CMN PLL source clock select.
>> 2. clock-frequency for MDIO clock frequency config.
>> 3. add uniphy AHB & SYS GCC clocks.
>> 4. add reset-gpios for MDIO bus level reset.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>>   1 file changed, 153 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> index 3407e909e8a7..9546a6ad7841 100644
>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> @@ -20,6 +20,8 @@ properties:
>>             - enum:
>>                 - qcom,ipq6018-mdio
>>                 - qcom,ipq8074-mdio
>> +              - qcom,ipq9574-mdio
>> +              - qcom,ipq5332-mdio
>>             - const: qcom,ipq4019-mdio
> 
> A driver can function without knowing about all these new registers and
> clocks? If not, then it can't be compatible with "qcom,ipq4019-mdio".

Yes, the driver can work without knowing the compatible string.
the configuration is decided by the DT property defined or not.

> 
>>   
>>     "#address-cells":
>> @@ -30,19 +32,71 @@ 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/IPQ9574, the last address and length is for the CMN clock
>> +      to select the reference clock.
>> +
>> +  reg-names:
>> +    minItems: 1
>> +    maxItems: 5
>>   
>>     clocks:
>> +    minItems: 1
>>       items:
>>         - description: MDIO clock source frequency fixed to 100MHZ
>> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
> 
> These are all clock inputs to this h/w block and not some other clocks
> you want to manage?

Yes, for ipq5332, these 5 clocks are need to be managed, for the legacy 
platform such as ipq8074, only MDIO clock is needed.

No other more clock needs to be managed for the current IPQ platforms.

> 
>>   
>>     clock-names:
>> +    minItems: 1
>>       items:
>>         - const: gcc_mdio_ahb_clk
>> +      - const: gcc_uniphy0_ahb_clk
>> +      - const: gcc_uniphy1_ahb_clk
>> +      - const: gcc_uniphy0_sys_clk
>> +      - const: gcc_uniphy1_sys_clk
> 
> "gcc" is presumably the name of the clock controller in QCom chips.
> Well, the clock source should not be part of the binding. The names
> should be local for what they are for. So drop 'gcc_'. And '_clk' is
> also redundant, so drop it too. Unfortunately you are stuck with the
> name of the 1st entry.

Yes, gcc is the name of QCOM SOC clock controller.
will remove the "gcc_" and "_clk" for the new added clocks.

we should keep the existed DT gcc_mdio_ahb_clk unmodified, right?
since it has been used in the current device tree.

> 
>> +
>> +  cmn-reference-clock:
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - 0   # CMN PLL reference internal 48MHZ
>> +              - 1   # CMN PLL reference external 25MHZ
>> +              - 2   # CMN PLL reference external 31250KHZ
>> +              - 3   # CMN PLL reference external 40MHZ
>> +              - 4   # CMN PLL reference external 48MHZ
>> +              - 5   # CMN PLL reference external 50MHZ
>> +              - 6   # CMN PLL reference internal 96MHZ
>> +
>> +  clock-frequency:
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - 12500000
>> +              - 6250000
>> +              - 3125000
>> +              - 1562500
>> +              - 781250
>> +              - 390625
>> +    description:
>> +      The MDIO bus clock that must be output by the MDIO bus hardware,
>> +      only the listed frequecies above can be configured, other frequency
>> +      will cause malfunction. If absent, the default hardware value is used.
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +
>> +  reset-assert-us:
>> +    maxItems: 1
>> +
>> +  reset-deassert-us:
>> +    maxItems: 1
>>   
>>   required:
>>     - compatible
>> @@ -61,6 +115,8 @@ allOf:
>>                 - qcom,ipq5018-mdio
>>                 - qcom,ipq6018-mdio
>>                 - qcom,ipq8074-mdio
>> +              - qcom,ipq5332-mdio
>> +              - qcom,ipq9574-mdio
>>       then:
>>         required:
>>           - clocks
>> @@ -70,6 +126,40 @@ allOf:
>>           clocks: false
>>           clock-names: false
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq5332-mdio
>> +    then:
>> +      properties:
>> +        clocks:
>> +          minItems: 5
>> +          maxItems: 5
>> +        reg-names:
>> +          items:
>> +            - const: mdio
>> +            - const: eth_ldo1
>> +            - const: eth_ldo2
>> +            - const: cmn_blk
> 
> Perhaps cmn_blk should come 2nd, so all the variants have the same entry
> indices. Then you can move this to the top level and just say 'minItems:
> 4' here.

Thanks Rob for the suggestion, i will update to move cmn_blk to the 2nd
location.

> 
> 
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq9574-mdio
>> +    then:
>> +      properties:
>> +        reg-names:
>> +          items:
>> +            - const: mdio
>> +            - const: eth_ldo1
>> +            - const: eth_ldo2
>> +            - const: eth_ldo3
>> +            - const: cmn_blk
> 
> And 'minItems: 5' here.
> 
> The ipq9574 adds the CMN block, but none of the clocks? Weird.
> 
> Rob

For ipq9574, only mdio clock is needed, the uniphy ahb and sys clock is
not needed to configure.

Yes, there is some Ethernet design delta between ipq9574 and ipq5332.