[PATCH 0/8] ipq5018: enable ethernet support

[PATCH 0/8] ipq5018: enable ethernet support

[Ziyang Huang]

Enable ethernet support for IPQ5018.

Praveenkumar I (1):
  clk: qcom: support for duplicate freq in RCG2 freq table

Ziyang Huang (7):
  net: phy: Introduce Qualcomm IPQ5018 internal PHY driver
  phy: Introduce Qualcomm ethernet uniphy driver
  net: stmmac: Introduce Qualcomm IPQ50xx DWMAC driver
  clk: qcom: gcc-ipq5018: correct gcc_gmac0_sys_clk reg
  net: mdio: ipq4019: support reset control
  arm64: dts: qcom: ipq5018: enable ethernet support
  arm64: dts: qcom: ipq5018-rdp432-c2: enable ethernet support

 .../arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts |  52 ++
 arch/arm64/boot/dts/qcom/ipq5018.dtsi         | 120 ++++-
 drivers/clk/qcom/clk-rcg2.c                   |  86 ++-
 drivers/clk/qcom/gcc-ipq5018.c                |   2 +-
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   7 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-ipq50xx.c   | 155 ++++++
 drivers/net/mdio/mdio-ipq4019.c               |  10 +
 drivers/net/phy/Kconfig                       |      +
 drivers/net/phy/Makefile                      |   1 +
 drivers/net/phy/ipq5018-internal.c            | 125 +++++
 drivers/phy/qualcomm/Kconfig                  |   7 +
 drivers/phy/qualcomm/Makefile                 |   2 +
 drivers/phy/qualcomm/phy-qcom-eth-uniphy.c    | 494 ++++++++++++++++++
 include/dt-bindings/phy/qcom-eth-uniphy.h     |  15 +
 15 files changed, 1057 insertions(+), 25 deletions(-)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c
 create mode 100644 drivers/net/phy/ipq5018-internal.c
 create mode 100644 drivers/phy/qualcomm/phy-qcom-eth-uniphy.c
 create mode 100644 include/dt-bindings/phy/qcom-eth-uniphy.h

-- 
2.40.1

[PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver

[Ziyang Huang]

Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
---
 drivers/net/phy/Kconfig            |   5 ++
 drivers/net/phy/Makefile           |   1 +
 drivers/net/phy/ipq5018-internal.c | 125 +++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 drivers/net/phy/ipq5018-internal.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 107880d13d21..2d068fea7008 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -202,6 +202,11 @@ config INTEL_XWAY_PHY
 	  PEF 7061, PEF 7071 and PEF 7072 or integrated into the Intel
 	  SoCs xRX200, xRX300, xRX330, xRX350 and xRX550.
 
+config IPQ5018_INTERNAL_PHY
+	tristate "Qualcomm IPQ5018 internal PHY"
+	help
+	  Supports for the Qualcomm IPQ5018 internal PHY.
+
 config LSI_ET1011C_PHY
 	tristate "LSI ET1011C PHY"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index c945ed9bd14b..16d65378ae34 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_DP83TD510_PHY)	+= dp83td510.o
 obj-$(CONFIG_FIXED_PHY)		+= fixed_phy.o
 obj-$(CONFIG_ICPLUS_PHY)	+= icplus.o
 obj-$(CONFIG_INTEL_XWAY_PHY)	+= intel-xway.o
+obj-$(CONFIG_IPQ5018_INTERNAL_PHY)	+= ipq5018-internal.o
 obj-$(CONFIG_LSI_ET1011C_PHY)	+= et1011c.o
 obj-$(CONFIG_LXT_PHY)		+= lxt.o
 obj-$(CONFIG_MARVELL_10G_PHY)	+= marvell10g.o
diff --git a/drivers/net/phy/ipq5018-internal.c b/drivers/net/phy/ipq5018-internal.c
new file mode 100644
index 000000000000..d1331951b4d8
--- /dev/null
+++ b/drivers/net/phy/ipq5018-internal.c
@@ -0,0 +1,125 @@
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/phy.h>
+#include <linux/reset.h>
+
+#define IPQ5018_PHY_ID			0x004dd0c0
+
+#define TX_RX_CLK_RATE			125000000 /* 125M */
+
+#define IPQ5018_PHY_FIFO_CONTROL	0x19
+#define  IPQ5018_PHY_FIFO_RESET		GENMASK(1, 0)
+
+struct ipq5018_phy {
+	int num_clks;
+	struct clk_bulk_data *clks;
+	struct reset_control *rst;
+
+	struct clk_hw *clk_rx, *clk_tx;
+	struct clk_hw_onecell_data *clk_data;
+};
+
+static int ipq5018_probe(struct phy_device *phydev)
+{
+	struct ipq5018_phy *priv;
+	struct device *dev = &phydev->mdio.dev;
+	char name[64];
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return dev_err_probe(dev, -ENOMEM,
+				     "failed to allocate priv\n");
+
+	priv->num_clks = devm_clk_bulk_get_all(dev, &priv->clks);
+	if (priv->num_clks < 0)
+		return dev_err_probe(dev, priv->num_clks,
+				     "failed to acquire clocks\n");
+
+	ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to enable clocks\n");
+
+	priv->rst = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR_OR_NULL(priv->rst))
+		return dev_err_probe(dev, PTR_ERR(priv->rst),
+				     "failed to acquire reset\n");
+
+	ret = reset_control_reset(priv->rst);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to reset\n");
+
+	snprintf(name, sizeof(name), "%s#rx", dev_name(dev));
+	priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0,
+						  TX_RX_CLK_RATE);
+	if (IS_ERR_OR_NULL(priv->clk_rx))
+		return dev_err_probe(dev, PTR_ERR(priv->clk_rx),
+				     "failed to register rx clock\n");
+
+	snprintf(name, sizeof(name), "%s#tx", dev_name(dev));
+	priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0,
+						  TX_RX_CLK_RATE);
+	if (IS_ERR_OR_NULL(priv->clk_tx))
+		return dev_err_probe(dev, PTR_ERR(priv->clk_tx),
+				     "failed to register tx clock\n");
+
+	priv->clk_data = devm_kzalloc(dev,
+				      struct_size(priv->clk_data, hws, 2),
+				      GFP_KERNEL);
+	if (!priv->clk_data)
+		return dev_err_probe(dev, -ENOMEM,
+				     "failed to allocate clk_data\n");
+
+	priv->clk_data->num = 2;
+	priv->clk_data->hws[0] = priv->clk_rx;
+	priv->clk_data->hws[1] = priv->clk_tx;
+	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
+				     priv->clk_data);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "fail to register clock provider\n");
+
+	return 0;
+}
+
+static int ipq5018_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL,
+			 IPQ5018_PHY_FIFO_RESET, 0);
+	if (ret < 0)
+		return ret;
+
+	msleep(50);
+
+	ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL,
+			 IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static struct phy_driver ipq5018_internal_phy_driver[] = {
+	{
+		PHY_ID_MATCH_EXACT(IPQ5018_PHY_ID),
+		.name		= "Qualcomm IPQ5018 internal PHY",
+		.flags		= PHY_IS_INTERNAL,
+		.probe		= ipq5018_probe,
+		.soft_reset	= ipq5018_soft_reset,
+	},
+};
+module_phy_driver(ipq5018_internal_phy_driver);
+
+static struct mdio_device_id __maybe_unused ipq5018_internal_phy_ids[] = {
+	{ PHY_ID_MATCH_EXACT(IPQ5018_PHY_ID) },
+	{ }
+};
+MODULE_DEVICE_TABLE(mdio, ipq5018_internal_phy_ids);
+
+MODULE_DESCRIPTION("Qualcomm IPQ5018 internal PHY driver");
+MODULE_AUTHOR("Ziyang Huang <hzyitc@outlook.com>");
-- 
2.40.1

Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver

[Andrew Lunn]

On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote:
> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>

You need to say something in the commit message. One obvious thing is
to justify not using the at803x driver, since 

> +#define IPQ5018_PHY_ID			0x004dd0c0

is in the Atheros OUI range.

> +static int ipq5018_probe(struct phy_device *phydev)
> +{
> +	struct ipq5018_phy *priv;
> +	struct device *dev = &phydev->mdio.dev;
> +	char name[64];
> +	int ret;

I guess you are new to mainline network. Please read:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

Section 1.6.4.

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "failed to allocate priv\n");

Please read the documentation of dev_err_probe() and this fix the
obvious problem with this.

> +	snprintf(name, sizeof(name), "%s#rx", dev_name(dev));
> +	priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0,
> +						  TX_RX_CLK_RATE);
> +	if (IS_ERR_OR_NULL(priv->clk_rx))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk_rx),
> +				     "failed to register rx clock\n");
> +
> +	snprintf(name, sizeof(name), "%s#tx", dev_name(dev));
> +	priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0,
> +						  TX_RX_CLK_RATE);
> +	if (IS_ERR_OR_NULL(priv->clk_tx))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk_tx),
> +				     "failed to register tx clock\n");
> +
> +	priv->clk_data = devm_kzalloc(dev,
> +				      struct_size(priv->clk_data, hws, 2),
> +				      GFP_KERNEL);
> +	if (!priv->clk_data)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "failed to allocate clk_data\n");
> +
> +	priv->clk_data->num = 2;
> +	priv->clk_data->hws[0] = priv->clk_rx;
> +	priv->clk_data->hws[1] = priv->clk_tx;
> +	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
> +				     priv->clk_data);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "fail to register clock provider\n");

This needs an explanation. Why register two fixed clocks, which you
never use? Why not put these two clocks in DT?

> +
> +	return 0;
> +}
> +
> +static int ipq5018_soft_reset(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL,
> +			 IPQ5018_PHY_FIFO_RESET, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(50);
> +
> +	ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL,
> +			 IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}

This needs an explanation. It is also somewhat like
qca808x_link_change_notify(). Is it really sufficient to only do this
reset FIFO during a soft reset, or is it needed when ever the link
changes?

You also appear to be missing device tree bindings.

    Andrew

---
pw-bot: cr

Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver

[Ziyang Huang]

在 2024/1/22 0:19, Andrew Lunn 写道:
> On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote:
>> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
> 
> You need to say something in the commit message. One obvious thing is
> to justify not using the at803x driver, since

I want add more descriptions here. But I have no idea what to write. 
This is a mininal driver for a special phy.

Here is the thing, at first, I was tring to add these into at803x 
driver, then I found that the IPQ5018 internel phy is a special device. 
The initialization sequence is initing GCC clock and reset control, then 
registering clocks providers, which is very different from other devices.

What's more, I remembered it *wrongly* and thought it need to be 
accessed through MMIO. After checking the vendor code again, this 
doesn't exist.

So it seem like that it's a good idea to move it back to at803x driver.


> 
>> +#define IPQ5018_PHY_ID			0x004dd0c0
> 
> is in the Atheros OUI range.
> 
>> +static int ipq5018_probe(struct phy_device *phydev)
>> +{
>> +	struct ipq5018_phy *priv;
>> +	struct device *dev = &phydev->mdio.dev;
>> +	char name[64];
>> +	int ret;
> 
> I guess you are new to mainline network. Please read:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> 
> Section 1.6.4.

Sorry for missing it, will read it later.

> 
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return dev_err_probe(dev, -ENOMEM,
>> +				     "failed to allocate priv\n");
> 
> Please read the documentation of dev_err_probe() and this fix the
> obvious problem with this.

I had read it and I had known this helper function is to resolve the 
duplicate code for EPROBE_DEFER.

But it also say "Note that it is deemed acceptable to use this function 
for error prints during probe even if the @err is known to never be 
-EPROBE_DEFER. The benefit compared to a normal dev_err() is the 
standardized format of the error code and the fact that the error code 
is returned."

And I can find the same code in other driver, so I thought it is a 
standard. Or should I just return -ENOMEM? Please let me known.

> 
>> +	snprintf(name, sizeof(name), "%s#rx", dev_name(dev));
>> +	priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0,
>> +						  TX_RX_CLK_RATE);
>> +	if (IS_ERR_OR_NULL(priv->clk_rx))
>> +		return dev_err_probe(dev, PTR_ERR(priv->clk_rx),
>> +				     "failed to register rx clock\n");
>> +
>> +	snprintf(name, sizeof(name), "%s#tx", dev_name(dev));
>> +	priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0,
>> +						  TX_RX_CLK_RATE);
>> +	if (IS_ERR_OR_NULL(priv->clk_tx))
>> +		return dev_err_probe(dev, PTR_ERR(priv->clk_tx),
>> +				     "failed to register tx clock\n");
>> +
>> +	priv->clk_data = devm_kzalloc(dev,
>> +				      struct_size(priv->clk_data, hws, 2),
>> +				      GFP_KERNEL);
>> +	if (!priv->clk_data)
>> +		return dev_err_probe(dev, -ENOMEM,
>> +				     "failed to allocate clk_data\n");
>> +
>> +	priv->clk_data->num = 2;
>> +	priv->clk_data->hws[0] = priv->clk_rx;
>> +	priv->clk_data->hws[1] = priv->clk_tx;
>> +	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
>> +				     priv->clk_data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "fail to register clock provider\n");
> 
> This needs an explanation. Why register two fixed clocks, which you
> never use? Why not put these two clocks in DT?

Without documentions, here is my guess:

This is required by GCC controller. GCC driver require TX and RX clocks 
from GEPHY/UNIPHY. Then throught some sel or div cells, output clocks to 
GEPHY/UNIPHY and MAC. So I need to register them to make them can be 
refered in GCC controller. Will add a figure describing the clock tree 
in UNIPHY driver.

The frequency of these clocks is depends on the mode. For GEPHY, it only 
supports SGMII ( Or something similar, this is a internal bus ) and the 
clock is fixed at 1.25G. But for UNIPHY, is supports more mode like 
SGMII+ which used 3.125G.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int ipq5018_soft_reset(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL,
>> +			 IPQ5018_PHY_FIFO_RESET, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	msleep(50);
>> +
>> +	ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL,
>> +			 IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
> 
> This needs an explanation. It is also somewhat like
> qca808x_link_change_notify(). Is it really sufficient to only do this
> reset FIFO during a soft reset, or is it needed when ever the link
> changes?

I'm not sure here, this is what u-boot does. But I guess, we can reset 
GCC_GEPHY_* serial reset_controls.

> 
> You also appear to be missing device tree bindings.

Sorry for forgeting to add a WiP tag. Will add dt-bindings documentions 
in next patches.

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

Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver

[Andrew Lunn]

On Mon, Jan 22, 2024 at 11:37:29PM +0800, Ziyang Huang wrote:
> 在 2024/1/22 0:19, Andrew Lunn 写道:
> > On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote:
> > > Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
> > 
> > You need to say something in the commit message. One obvious thing is
> > to justify not using the at803x driver, since
> 
> I want add more descriptions here. But I have no idea what to write. This is
> a mininal driver for a special phy.

So say how it is special. Indicate why it needs a minimal driver.

Does the hardware support cable test? WoL? Does it perform downshift
and you can read the actual speed from the AT803X_SPECIFIC_STATUS
registers?

What we want to avoid is that you start with a special driver, and
then start copying bits of the at803x driver to support the hardware
features. The at803x.c driver already supports a few internal PHYs:
"Qualcomm Atheros AR9331 built-in", "Qualcomm Atheros QCA9561 built-in
PHY", "Qualcomm Atheros 8337 internal PHY", "Qualcomm Atheros 8327-A
internal PHY", "Qualcomm Atheros 8327-B internal PHY", so please add
it to the driver and test what additional features work for it.

> Here is the thing, at first, I was tring to add these into at803x driver,
> then I found that the IPQ5018 internel phy is a special device. The
> initialization sequence is initing GCC clock and reset control, then
> registering clocks providers, which is very different from other devices.

That is a different story all together, and part of the problems we
had with Qualcomm patches. It might be you need to use ID values in
the compatible to get this driver loaded. The PHY driver can then
enable the clocks it needs and take itself out of reset. What is
important here is an accurate device tree representation. What clocks
and resets does this device consume.

> > > +	if (!priv)
> > > +		return dev_err_probe(dev, -ENOMEM,
> > > +				     "failed to allocate priv\n");
> > 
> > Please read the documentation of dev_err_probe() and this fix the
> > obvious problem with this.

> And I can find the same code in other driver, so I thought it is a standard.
> Or should I just return -ENOMEM? Please let me known.

-ENOMEM is one of the error codes you are unlikely to see. And if it
does happen, you are going to have cascading errors. So just return
-ENOMEM.

> > > +	snprintf(name, sizeof(name), "%s#rx", dev_name(dev));
> > > +	priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0,
> > > +						  TX_RX_CLK_RATE);
> > > +	if (IS_ERR_OR_NULL(priv->clk_rx))
> > > +		return dev_err_probe(dev, PTR_ERR(priv->clk_rx),
> > > +				     "failed to register rx clock\n");
> > > +
> > > +	snprintf(name, sizeof(name), "%s#tx", dev_name(dev));
> > > +	priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0,
> > > +						  TX_RX_CLK_RATE);
> > > +	if (IS_ERR_OR_NULL(priv->clk_tx))
> > > +		return dev_err_probe(dev, PTR_ERR(priv->clk_tx),
> > > +				     "failed to register tx clock\n");
> > > +
> > > +	priv->clk_data = devm_kzalloc(dev,
> > > +				      struct_size(priv->clk_data, hws, 2),
> > > +				      GFP_KERNEL);
> > > +	if (!priv->clk_data)
> > > +		return dev_err_probe(dev, -ENOMEM,
> > > +				     "failed to allocate clk_data\n");
> > > +
> > > +	priv->clk_data->num = 2;
> > > +	priv->clk_data->hws[0] = priv->clk_rx;
> > > +	priv->clk_data->hws[1] = priv->clk_tx;
> > > +	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
> > > +				     priv->clk_data);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret,
> > > +				     "fail to register clock provider\n");
> > 
> > This needs an explanation. Why register two fixed clocks, which you
> > never use? Why not put these two clocks in DT?
> 
> Without documentions, here is my guess:

So you don't have the data sheet? Are you working from the Qualcomm
vendor tree?

> This is required by GCC controller. GCC driver require TX and RX clocks from
> GEPHY/UNIPHY. Then throught some sel or div cells, output clocks to
> GEPHY/UNIPHY and MAC. So I need to register them to make them can be refered
> in GCC controller. Will add a figure describing the clock tree in UNIPHY
> driver.

So in this case, the GCC is a clock consumer and the PHY is a clock
provider. Does GCC use DT properties clocks/clock-names and phandles
to reference these clocks it is consuming? If so, you can just use
fixed-clocks in DT.

> > > +static int ipq5018_soft_reset(struct phy_device *phydev)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL,
> > > +			 IPQ5018_PHY_FIFO_RESET, 0);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	msleep(50);
> > > +
> > > +	ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL,
> > > +			 IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > 
> > This needs an explanation. It is also somewhat like
> > qca808x_link_change_notify(). Is it really sufficient to only do this
> > reset FIFO during a soft reset, or is it needed when ever the link
> > changes?
> 
> I'm not sure here, this is what u-boot does. But I guess, we can reset
> GCC_GEPHY_* serial reset_controls.

Please add a comment with your best guess what it is doing and why. Is
this vendor u-boot, or upstream u-boot? Maybe the git history will
give you more details.

> > You also appear to be missing device tree bindings.
> 
> Sorry for forgeting to add a WiP tag. Will add dt-bindings documentions in
> next patches.

The DT binding is just as important as the code. Often the DT binding
is so broken we don't even bother looking at the code...

   Andrew

Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver

[Ziyang Huang]

在 2024/1/23 1:18, Andrew Lunn 写道:
> On Mon, Jan 22, 2024 at 11:37:29PM +0800, Ziyang Huang wrote:
>> 在 2024/1/22 0:19, Andrew Lunn 写道:
>>> On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote:
>>>> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
>>>
>>> You need to say something in the commit message. One obvious thing is
>>> to justify not using the at803x driver, since
>>
>> I want add more descriptions here. But I have no idea what to write. This is
>> a mininal driver for a special phy.
> 
> So say how it is special. Indicate why it needs a minimal driver.
> 
> Does the hardware support cable test? WoL? Does it perform downshift
> and you can read the actual speed from the AT803X_SPECIFIC_STATUS
> registers?
> 
> What we want to avoid is that you start with a special driver, and
> then start copying bits of the at803x driver to support the hardware
> features. The at803x.c driver already supports a few internal PHYs:
> "Qualcomm Atheros AR9331 built-in", "Qualcomm Atheros QCA9561 built-in
> PHY", "Qualcomm Atheros 8337 internal PHY", "Qualcomm Atheros 8327-A
> internal PHY", "Qualcomm Atheros 8327-B internal PHY", so please add
> it to the driver and test what additional features work for it.

After rechecking the vendor code, you are right. The only special thing 
of this device is that it's a combined device of UNIPHY and at803x 
general phy. So it needs the UNIPHY initialization sequence. But for the 
PHY part, it's almost same as others, just has some special registers. I 
will merge it into at803x driver.

> 
>> Here is the thing, at first, I was tring to add these into at803x driver,
>> then I found that the IPQ5018 internel phy is a special device. The
>> initialization sequence is initing GCC clock and reset control, then
>> registering clocks providers, which is very different from other devices.
> 
> That is a different story all together, and part of the problems we
> had with Qualcomm patches. It might be you need to use ID values in
> the compatible to get this driver loaded. The PHY driver can then
> enable the clocks it needs and take itself out of reset. What is
> important here is an accurate device tree representation. What clocks
> and resets does this device consume.

Ok, will try to do this.

> 
>>>> +	if (!priv)
>>>> +		return dev_err_probe(dev, -ENOMEM,
>>>> +				     "failed to allocate priv\n");
>>>
>>> Please read the documentation of dev_err_probe() and this fix the
>>> obvious problem with this.
> 
>> And I can find the same code in other driver, so I thought it is a standard.
>> Or should I just return -ENOMEM? Please let me known.
> 
> -ENOMEM is one of the error codes you are unlikely to see. And if it
> does happen, you are going to have cascading errors. So just return
> -ENOMEM.

ok, got it. Thanks.

> 
>>>> +	snprintf(name, sizeof(name), "%s#rx", dev_name(dev));
>>>> +	priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0,
>>>> +						  TX_RX_CLK_RATE);
>>>> +	if (IS_ERR_OR_NULL(priv->clk_rx))
>>>> +		return dev_err_probe(dev, PTR_ERR(priv->clk_rx),
>>>> +				     "failed to register rx clock\n");
>>>> +
>>>> +	snprintf(name, sizeof(name), "%s#tx", dev_name(dev));
>>>> +	priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0,
>>>> +						  TX_RX_CLK_RATE);
>>>> +	if (IS_ERR_OR_NULL(priv->clk_tx))
>>>> +		return dev_err_probe(dev, PTR_ERR(priv->clk_tx),
>>>> +				     "failed to register tx clock\n");
>>>> +
>>>> +	priv->clk_data = devm_kzalloc(dev,
>>>> +				      struct_size(priv->clk_data, hws, 2),
>>>> +				      GFP_KERNEL);
>>>> +	if (!priv->clk_data)
>>>> +		return dev_err_probe(dev, -ENOMEM,
>>>> +				     "failed to allocate clk_data\n");
>>>> +
>>>> +	priv->clk_data->num = 2;
>>>> +	priv->clk_data->hws[0] = priv->clk_rx;
>>>> +	priv->clk_data->hws[1] = priv->clk_tx;
>>>> +	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
>>>> +				     priv->clk_data);
>>>> +	if (ret)
>>>> +		return dev_err_probe(dev, ret,
>>>> +				     "fail to register clock provider\n");
>>>
>>> This needs an explanation. Why register two fixed clocks, which you
>>> never use? Why not put these two clocks in DT?
>>
>> Without documentions, here is my guess:
> 
> So you don't have the data sheet? Are you working from the Qualcomm
> vendor tree?

Unfortunately, Yes. I couldn't find any documentions about this part. So 
I read the Qualcomm code, tried to realize the logic and guessed the
functions of registers. Base on my understand, I use MACRO to describe 
these registers for human-readable and examined them.

> 
>> This is required by GCC controller. GCC driver require TX and RX clocks from
>> GEPHY/UNIPHY. Then throught some sel or div cells, output clocks to
>> GEPHY/UNIPHY and MAC. So I need to register them to make them can be refered
>> in GCC controller. Will add a figure describing the clock tree in UNIPHY
>> driver.
> 
> So in this case, the GCC is a clock consumer and the PHY is a clock
> provider. Does GCC use DT properties clocks/clock-names and phandles
> to reference these clocks it is consuming? If so, you can just use
> fixed-clocks in DT.

Yes, GCC use DT handler to refer these clocks. Will try as your said.

> 
>>>> +static int ipq5018_soft_reset(struct phy_device *phydev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL,
>>>> +			 IPQ5018_PHY_FIFO_RESET, 0);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	msleep(50);
>>>> +
>>>> +	ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL,
>>>> +			 IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> This needs an explanation. It is also somewhat like
>>> qca808x_link_change_notify(). Is it really sufficient to only do this
>>> reset FIFO during a soft reset, or is it needed when ever the link
>>> changes?
>>
>> I'm not sure here, this is what u-boot does. But I guess, we can reset
>> GCC_GEPHY_* serial reset_controls.
> 
> Please add a comment with your best guess what it is doing and why. Is
> this vendor u-boot, or upstream u-boot? Maybe the git history will
> give you more details.

Ok, I will also try to replace them with the series of GCC_GEPHY_* 
reset_controls and check whether it work.

> 
>>> You also appear to be missing device tree bindings.
>>
>> Sorry for forgeting to add a WiP tag. Will add dt-bindings documentions in
>> next patches.
> 
> The DT binding is just as important as the code. Often the DT binding
> is so broken we don't even bother looking at the code...

Will write them.

> 
>     Andrew

Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver

[Andrew Lunn]

> After rechecking the vendor code, you are right. The only special thing of
> this device is that it's a combined device of UNIPHY and at803x general phy.
> So it needs the UNIPHY initialization sequence. But for the PHY part, it's
> almost same as others, just has some special registers. I will merge it into
> at803x driver.

The UNIPHY is a separate driver, its a generic PHY driver? Can we keep
them separate for this internal PHY as well?

The initialisation sequence is what is going to be most 'interesting'
here. How UNIPHY, this PHY and the GCC all come together to make it
work. But for the moment, i think its best the PHY driver controls its
own clock input and reset, using standard Linux APIs, once the driver
has probed via compatible IDs.

       Andrew

[PATCH 2/8] phy: Introduce Qualcomm ethernet uniphy driver

[Ziyang Huang]

Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
---
 drivers/phy/qualcomm/Kconfig               |   7 +
 drivers/phy/qualcomm/Makefile              |   2 +
 drivers/phy/qualcomm/phy-qcom-eth-uniphy.c | 494 +++++++++++++++++++++
 include/dt-bindings/phy/qcom-eth-uniphy.h  |  15 +
 4 files changed, 518 insertions(+)
 create mode 100644 drivers/phy/qualcomm/phy-qcom-eth-uniphy.c
 create mode 100644 include/dt-bindings/phy/qcom-eth-uniphy.h

diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
index 97ca5952e34e..1cbbfd196115 100644
--- a/drivers/phy/qualcomm/Kconfig
+++ b/drivers/phy/qualcomm/Kconfig
@@ -28,6 +28,13 @@ config PHY_QCOM_EDP
 	  Enable this driver to support the Qualcomm eDP PHY found in various
 	  Qualcomm chipsets.
 
+config PHY_QCOM_ETH_UNIPHY
+	tristate "Qualcomm ethernet uniphy driver"
+	depends on OF && COMMON_CLK && (ARCH_QCOM || COMPILE_TEST)
+	select GENERIC_PHY
+	help
+	  Support for the Qualcomm ethernet uniphy.
+
 config PHY_QCOM_IPQ4019_USB
 	tristate "Qualcomm IPQ4019 USB PHY driver"
 	depends on OF && (ARCH_QCOM || COMPILE_TEST)
diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
index b030858e0f8d..a9f01e688553 100644
--- a/drivers/phy/qualcomm/Makefile
+++ b/drivers/phy/qualcomm/Makefile
@@ -21,4 +21,6 @@ obj-$(CONFIG_PHY_QCOM_USB_HS_28NM)	+= phy-qcom-usb-hs-28nm.o
 obj-$(CONFIG_PHY_QCOM_USB_SS)		+= phy-qcom-usb-ss.o
 obj-$(CONFIG_PHY_QCOM_USB_SNPS_FEMTO_V2)+= phy-qcom-snps-femto-v2.o
 obj-$(CONFIG_PHY_QCOM_IPQ806X_USB)	+= phy-qcom-ipq806x-usb.o
+
+obj-$(CONFIG_PHY_QCOM_ETH_UNIPHY)	+= phy-qcom-eth-uniphy.o
 obj-$(CONFIG_PHY_QCOM_SGMII_ETH)	+= phy-qcom-sgmii-eth.o
diff --git a/drivers/phy/qualcomm/phy-qcom-eth-uniphy.c b/drivers/phy/qualcomm/phy-qcom-eth-uniphy.c
new file mode 100644
index 000000000000..71d4cefb8adb
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-eth-uniphy.c
@@ -0,0 +1,494 @@
+/*
+ * UNIPHY is the PCS between MAC and PHY which controls the mode of
+ * physical ports. Depends on different SoC, it can support
+ * SGMII/SGMII+/USXGMII. What's more, in some SoC it also support
+ * QSGMII/PSGMII which combine multi SGMII line into single physical port.
+ *
+ * =======================================================================
+ *        ________________________________
+ *       |  _______   IPQ807x             |
+ *       | | GMAC0 |__                    |
+ *       | |_______|  \                   |               _________
+ *       |  _______    \                  |          ____|   GPHY  |
+ *       | | GMAC1 |__  \     _________   |         /    | /Switch |
+ *       | |_______|  \  \___|         |  |     SGMII(+) |_________|
+ *       |  _______    \_____|         |  | P0    /
+ *       | | GMAC2 |_________| UNIPHY0 |--|-----or
+ *       | |_______|    _____|         |  |       \
+ *       |  _______    /   __|         |  |    (Q/P)SGMII __________
+ *       | | GMAC3 |__/   /  |_________|  |         \____| (Q/P)PHY |
+ *       | |_______|     /                |              |__________|
+ *       |  _______     /                 |
+ *       | | GMAC4 |--or                  |               _________
+ *       | |_______|    \     _________   | P1           | (X)GPHY |
+ *       |  ________     or--| UNIPHY1 |--|----SGMII(+)--| /Switch |
+ *       | | XGMAC0 |___/    |_________|  |    /USXGMII  |_________|
+ *       | |________|                     |
+ *       |  ________                      |
+ *       | | GMAC5  |___                  |               _________
+ *       | |________|   \     _________   | P2           | (X)GPHY |
+ *       |  ________     or--| UNIPHY2 |--|----SGMII(+)--| /Switch |
+ *       | | XGMAC1 |___/    |_________|  |    /USXGMII  |_________|
+ *       | |________|                     |
+ *       |________________________________|
+ *
+ * =======================================================================
+ *       _________________________________
+ *       |  _______   IPQ50xx   ______    | P0             ______
+ *       | | GMAC0 |___________| GPHY |---|------UTP------| RJ45 |
+ *       | |_______|           |______|   |               |______|
+ *       |  _______           _________   |               _________
+ *       | | GMAC1 |_________| UNIPHY0 |  | P1           |   GPHY  |
+ *       | |_______|         |_________|--|----SGMII(+)--| /Switch |
+ *       |________________________________|              |_________|
+ *
+ * =======================================================================
+ */
+
+#include <dt-bindings/phy/qcom-eth-uniphy.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/iopoll.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+
+#define TCSR_ETH_CMN				0x0
+#define  TCSR_ETH_CMN_ENABLE			BIT(0)
+
+
+#define CMN_PLL_REFCLK_SRC			0x28
+#define  CMN_PLL_REFCLK_SRC_FROM_MASK		GENMASK(9, 8)
+#define   CMN_PLL_REFCLK_SRC_FROM(x)		FIELD_PREP(CMN_PLL_REFCLK_SRC_FROM_MASK, (x))
+#define   CMN_PLL_REFCLK_SRC_FROM_REG		CMN_PLL_REFCLK_SRC_FROM(0)
+#define   CMN_PLL_REFCLK_SRC_FROM_LOGIC		CMN_PLL_REFCLK_SRC_FROM(1)
+#define   CMN_PLL_REFCLK_SRC_FROM_PCS		CMN_PLL_REFCLK_SRC_FROM(2)
+
+#define CMN_PLL_REFCLK				0x784
+#define  CMN_PLL_REFCLK_EXTERNAL		BIT(9)
+#define  CMN_PLL_REFCLK_DIV_MASK		GENMASK(8, 4)
+#define   CMN_PLL_REFCLK_DIV(x)			FIELD_PREP(CMN_PLL_REFCLK_DIV_MASK, (x))
+#define  CMN_PLL_REFCLK_FREQ_MASK		GENMASK(3, 0)
+#define   CMN_PLL_REFCLK_FREQ(x)		FIELD_PREP(CMN_PLL_REFCLK_FREQ_MASK, (x))
+#define   CMN_PLL_REFCLK_FREQ_25M		CMN_PLL_REFCLK_FREQ(3)
+#define   CMN_PLL_REFCLK_FREQ_31250K		CMN_PLL_REFCLK_FREQ(4)
+#define   CMN_PLL_REFCLK_FREQ_40M		CMN_PLL_REFCLK_FREQ(6)
+#define   CMN_PLL_REFCLK_FREQ_48M		CMN_PLL_REFCLK_FREQ(7)
+#define   CMN_PLL_REFCLK_FREQ_50M		CMN_PLL_REFCLK_FREQ(8)
+
+#define CMN_PLL_CTRL				0x780
+#define  CMN_PLL_CTRL_RST_N			BIT(6)
+
+#define CMN_PLL_STATUS				0x64
+#define  CMN_PLL_STATUS_LOCKED			BIT(2)
+
+
+#define IPQ50XX_UNIPHY_CLKOUT			0x74
+#define  IPQ50XX_UNIPHY_CLKOUT_DS_MASK		GENMASK(3, 2)
+#define   IPQ50XX_UNIPHY_CLKOUT_DS(x)		FIELD_PREP(IPQ50XX_UNIPHY_CLKOUT_DS_MASK, (x))
+#define   IPQ50XX_UNIPHY_CLKOUT_DS_2_8V		IPQ50XX_UNIPHY_CLKOUT_DS(0)
+#define   IPQ50XX_UNIPHY_CLKOUT_DS_1_5V		IPQ50XX_UNIPHY_CLKOUT_DS(1)
+#define  IPQ50XX_UNIPHY_CLKOUT_DIV_MASK		GENMASK(1, 1)
+#define   IPQ50XX_UNIPHY_CLKOUT_DIV(x)		FIELD_PREP(IPQ50XX_UNIPHY_CLKOUT_DIV_MASK, (x))
+#define   IPQ50XX_UNIPHY_CLKOUT_DIV_50M		IPQ50XX_UNIPHY_CLKOUT_DIV(0)
+#define   IPQ50XX_UNIPHY_CLKOUT_DIV_25M		IPQ50XX_UNIPHY_CLKOUT_DIV(1)
+#define  IPQ50XX_UNIPHY_CLKOUT_ENABLE		BIT(0)
+
+#define IPQ53XX_UNIPHY_CLKOUT			0x610
+#define  IPQ53XX_UNIPHY_CLKOUT_LDO_LEVEL_MASK	GENMASK(10, 8)
+#define  IPQ53XX_UNIPHY_CLKOUT_DIV_MASK		GENMASK(5, 5)
+#define   IPQ53XX_UNIPHY_CLKOUT_DIV(x)		FIELD_PREP(IPQ53XX_UNIPHY_CLKOUT_DIV_MASK, (x))
+#define   IPQ53XX_UNIPHY_CLKOUT_DIV_50M		IPQ53XX_UNIPHY_CLKOUT_DIV(0)
+#define   IPQ53XX_UNIPHY_CLKOUT_DIV_25M		IPQ53XX_UNIPHY_CLKOUT_DIV(1)
+#define  IPQ53XX_UNIPHY_CLKOUT_PULLDOWN		BIT(3)
+#define  IPQ53XX_UNIPHY_CLKOUT_DS_MASK		GENMASK(2, 1)
+#define   IPQ53XX_UNIPHY_CLKOUT_DS(x)		FIELD_PREP(IPQ53XX_UNIPHY_CLKOUT_DS_MASK, (x))
+#define   IPQ53XX_UNIPHY_CLKOUT_DS_2_8V		IPQ53XX_UNIPHY_CLKOUT_DS(0)
+#define   IPQ53XX_UNIPHY_CLKOUT_DS_1_5V		IPQ53XX_UNIPHY_CLKOUT_DS(1)
+#define  IPQ53XX_UNIPHY_CLKOUT_ENABLE		BIT(0)
+
+
+#define UNIPHY_MODE				0x46c
+#define  UNIPHY_MODE_USXG			BIT(13)
+#define  UNIPHY_MODE_XPCS			BIT(12)
+#define  UNIPHY_MODE_SGMIIPLUS			BIT(11)
+#define  UNIPHY_MODE_SGMII			BIT(10)
+#define  UNIPHY_MODE_PSGMII			BIT(9)
+#define  UNIPHY_MODE_QSGMII			BIT(8)
+#define  UNIPHY_MODE_CH0_MODE_MASK		GENMASK(6, 4)
+#define   UNIPHY_MODE_CH0_MODE(x)		FIELD_PREP(UNIPHY_MODE_CH0_MODE_MASK, (x))
+#define   UNIPHY_MODE_CH0_MODE_1000BASEX	UNIPHY_MODE_CH0_MODE(0)
+#define   UNIPHY_MODE_CH0_MODE_MAC		UNIPHY_MODE_CH0_MODE(2)
+#define  UNIPHY_MODE_SGMII_CHANNEL_MASK		GENMASK(2, 1)
+#define   UNIPHY_MODE_SGMII_CHANNEL(x)		FIELD_PREP(UNIPHY_MODE_SGMII_CHANNEL_MASK, (x))
+#define   UNIPHY_MODE_SGMII_CHANNEL_0		UNIPHY_MODE_SGMII_CHANNEL(0)
+#define   UNIPHY_MODE_SGMII_CHANNEL_1		UNIPHY_MODE_SGMII_CHANNEL(1)
+#define   UNIPHY_MODE_SGMII_CHANNEL_4		UNIPHY_MODE_SGMII_CHANNEL(2)
+#define  UNIPHY_MODE_AN_MODE_MASK		BIT(0)
+#define   UNIPHY_MODE_AN_MODE(x)		FIELD_PREP(UNIPHY_MODE_AN_MODE_MASK, (x))
+#define   UNIPHY_MODE_AN_MODE_ATHEROS		UNIPHY_MODE_AN_MODE(0)
+#define   UNIPHY_MODE_AN_MODE_STANDARD		UNIPHY_MODE_AN_MODE(1)
+
+#define UNIPHY_PLL_CTRL				0x780
+#define  UNIPHY_PLL_CTRL_RST_N			BIT(6)
+
+#define UNIPHY_CALIBRATION			0x1E0
+#define  UNIPHY_CALIBRATION_DONE		BIT(7)
+
+
+#define UNIPHY_CHANNEL(x)			(0x480 + 0x18 * (x))
+#define  UNIPHY_CHANNEL_RSTN			BIT(11)
+#define  UNIPHY_CHANNEL_FORCE_SPEED_25M		BIT(3)
+
+#define UNIPHY_SGMII				0x218
+#define  UNIPHY_SGMII_MODE_MASK			GENMASK(6, 4)
+#define   UNIPHY_SGMII_MODE(x)			FIELD_PREP(UNIPHY_SGMII_MODE_MASK, (x))
+#define   UNIPHY_SGMII_MODE_SGMII		UNIPHY_SGMII_MODE(3)
+#define   UNIPHY_SGMII_MODE_SGMIIPLUS		UNIPHY_SGMII_MODE(5)
+#define   UNIPHY_SGMII_MODE_USXGMII		UNIPHY_SGMII_MODE(7)
+#define  UNIPHY_SGMII_RATE_MASK			GENMASK(1, 0)
+#define   UNIPHY_SGMII_RATE(x)			FIELD_PREP(UNIPHY_SGMII_RATE_MASK, (x))
+
+
+#define SGMII_CLK_RATE				125000000 /* 125M */
+#define SGMII_PLUS_CLK_RATE			312500000 /* 312.5M */
+
+
+struct qcom_eth_uniphy {
+	struct device *dev;
+	void __iomem *base;
+	int num_clks;
+	struct clk_bulk_data *clks;
+	struct reset_control *rst;
+
+	int mode;
+
+	struct clk_hw *clk_rx, *clk_tx;
+	struct clk_hw_onecell_data *clk_data;
+};
+
+
+#define rmwl(addr, mask, val) \
+	writel(((readl(addr) & ~(mask)) | ((val) & (mask))), addr)
+
+static int cmn_init(struct platform_device *pdev)
+{
+	struct resource *res;
+	void __iomem *cmn_base;
+	void __iomem *tcsr_base;
+	u32 val;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn");
+	if (!res)
+		return 0;
+
+	cmn_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR_OR_NULL(cmn_base))
+		return PTR_ERR(cmn_base);
+
+	/* For IPQ50xx, tcsr is necessary to enable cmn block */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr");
+	if (res) {
+		tcsr_base = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR_OR_NULL(tcsr_base))
+			return PTR_ERR(tcsr_base);
+
+		rmwl((tcsr_base + TCSR_ETH_CMN), TCSR_ETH_CMN_ENABLE,
+		     TCSR_ETH_CMN_ENABLE);
+	}
+
+	rmwl((cmn_base + CMN_PLL_REFCLK_SRC),
+	     CMN_PLL_REFCLK_SRC_FROM_MASK,
+	     CMN_PLL_REFCLK_SRC_FROM_REG);
+	rmwl((cmn_base + CMN_PLL_REFCLK),
+	     (CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_FREQ_MASK
+	      | CMN_PLL_REFCLK_DIV_MASK),
+	     (CMN_PLL_REFCLK_FREQ_48M | CMN_PLL_REFCLK_DIV(2)));
+
+	rmwl((cmn_base + CMN_PLL_CTRL), CMN_PLL_CTRL_RST_N, 0);
+	msleep(1);
+	rmwl((cmn_base + CMN_PLL_CTRL), CMN_PLL_CTRL_RST_N,
+	     CMN_PLL_CTRL_RST_N);
+	msleep(1);
+
+	return read_poll_timeout(readl, val,
+				 (val & CMN_PLL_STATUS_LOCKED),
+				 100, 200000, false,
+				 (cmn_base + CMN_PLL_STATUS));
+}
+
+
+static void uniphy_write(struct qcom_eth_uniphy *uniphy, int addr, u32 val)
+{
+	writel(val, (uniphy->base + addr));
+}
+
+static u32 uniphy_read(struct qcom_eth_uniphy *uniphy, int addr)
+{
+	return readl((uniphy->base + addr));
+}
+
+static void uniphy_rmw(struct qcom_eth_uniphy *uniphy, int addr, u32 mask, u32 val)
+{
+	u32 v = uniphy_read(uniphy, addr);
+	v &= ~mask;
+	v |= val & mask;
+	uniphy_write(uniphy, addr, v);
+}
+
+static int uniphy_clkout_init(struct qcom_eth_uniphy *uniphy)
+{
+	u32 val;
+	int ret;
+
+	ret = of_property_read_u32(uniphy->dev->of_node, "clkout-frequency", &val);
+	if (ret == -EINVAL)
+		return 0;
+	else if (ret < 0)
+		return ret;
+
+	switch(val) {
+		case QCOM_ETH_UNIPHY_CLKOUT_FREQ_25M:
+			uniphy_rmw(uniphy, IPQ50XX_UNIPHY_CLKOUT,
+				   IPQ50XX_UNIPHY_CLKOUT_DIV_MASK,
+				   IPQ50XX_UNIPHY_CLKOUT_DIV_25M);
+			break;
+		case QCOM_ETH_UNIPHY_CLKOUT_FREQ_50M:
+			uniphy_rmw(uniphy, IPQ50XX_UNIPHY_CLKOUT,
+				   IPQ50XX_UNIPHY_CLKOUT_DIV_MASK,
+				   IPQ50XX_UNIPHY_CLKOUT_DIV_50M);
+			break;
+		default:
+			dev_err(uniphy->dev, "Unsupported clkout-frequency: %d\n", val);
+			return -EINVAL;
+	}
+
+	ret = of_property_read_u32(uniphy->dev->of_node, "clkout-drive-strength", &val);
+	if (ret != -EINVAL) {
+		if (ret < 0)
+			return ret;
+
+		switch(val) {
+			case QCOM_ETH_UNIPHY_CLKOUT_DS_1_5V:
+				uniphy_rmw(uniphy, IPQ50XX_UNIPHY_CLKOUT,
+					   IPQ50XX_UNIPHY_CLKOUT_DS_MASK,
+					   IPQ50XX_UNIPHY_CLKOUT_DS_1_5V);
+				break;
+			case QCOM_ETH_UNIPHY_CLKOUT_DS_2_8V:
+				uniphy_rmw(uniphy, IPQ50XX_UNIPHY_CLKOUT,
+					   IPQ50XX_UNIPHY_CLKOUT_DS_MASK,
+					   IPQ50XX_UNIPHY_CLKOUT_DS_2_8V);
+				break;
+			default:
+				dev_err(uniphy->dev, "Unsupported clkout-drive-strength: %d\n", val);
+				return -EINVAL;
+		}
+
+	}
+
+	uniphy_rmw(uniphy, IPQ50XX_UNIPHY_CLKOUT,
+		   IPQ50XX_UNIPHY_CLKOUT_ENABLE,
+		   IPQ50XX_UNIPHY_CLKOUT_ENABLE);
+
+	return 0;
+}
+
+static int uniphy_mode_set(struct qcom_eth_uniphy *uniphy)
+{
+	int ret;
+
+	ret = of_property_read_u32(uniphy->dev->of_node, "mode",
+				   &uniphy->mode);
+	if (ret < 0)
+		return ret;
+
+	switch(uniphy->mode) {
+		case QCOM_ETH_UNIPHY_MODE_SGMII:
+			uniphy_write(uniphy, UNIPHY_MODE,
+				     UNIPHY_MODE_SGMII);
+			uniphy_rmw(uniphy, UNIPHY_SGMII,
+				   UNIPHY_SGMII_MODE_MASK,
+				   UNIPHY_SGMII_MODE_SGMII);
+			break;
+		default:
+			dev_err(uniphy->dev, "Unsupported mode: %d\n",
+				uniphy->mode);
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int uniphy_calibrate(struct qcom_eth_uniphy *uniphy)
+{
+	u32 val;
+
+	uniphy_rmw(uniphy, UNIPHY_PLL_CTRL, UNIPHY_PLL_CTRL_RST_N, 0);
+	msleep(1);
+	uniphy_rmw(uniphy, UNIPHY_PLL_CTRL, UNIPHY_PLL_CTRL_RST_N,
+		   UNIPHY_PLL_CTRL_RST_N);
+	msleep(1);
+
+	return read_poll_timeout(uniphy_read, val,
+				 (val & UNIPHY_CALIBRATION_DONE),
+				 100, 200000, false,
+				 uniphy, UNIPHY_CALIBRATION);
+}
+
+static int uniphy_clk_register(struct qcom_eth_uniphy *uniphy)
+{
+	unsigned long rate;
+	char name[64];
+	int ret;
+
+	switch (uniphy->mode) {
+		case QCOM_ETH_UNIPHY_MODE_SGMII:
+			rate = SGMII_CLK_RATE;
+			break;
+	}
+
+	snprintf(name, sizeof(name), "%s#rx", dev_name(uniphy->dev));
+	uniphy->clk_rx = clk_hw_register_fixed_rate(uniphy->dev, name,
+						    NULL, 0, rate);
+	if (IS_ERR_OR_NULL(uniphy->clk_rx))
+		return dev_err_probe(uniphy->dev, PTR_ERR(uniphy->clk_rx),
+				     "failed to register rx clock\n");
+
+	snprintf(name, sizeof(name), "%s#tx", dev_name(uniphy->dev));
+	uniphy->clk_tx = clk_hw_register_fixed_rate(uniphy->dev, name,
+						    NULL, 0, rate);
+	if (IS_ERR_OR_NULL(uniphy->clk_tx))
+		return dev_err_probe(uniphy->dev, PTR_ERR(uniphy->clk_tx),
+				     "failed to register rx clock\n");
+
+	uniphy->clk_data = devm_kzalloc(uniphy->dev,
+					struct_size(uniphy->clk_data, hws, 2),
+					GFP_KERNEL);
+	if (!uniphy->clk_data)
+		return dev_err_probe(uniphy->dev, -ENOMEM,
+				     "failed to allocate clk_data\n");
+
+	uniphy->clk_data->num = 2;
+	uniphy->clk_data->hws[0] = uniphy->clk_rx;
+	uniphy->clk_data->hws[1] = uniphy->clk_tx;
+	ret = of_clk_add_hw_provider(uniphy->dev->of_node,
+				     of_clk_hw_onecell_get,
+				     uniphy->clk_data);
+	if (ret)
+		return dev_err_probe(uniphy->dev, ret,
+				     "fail to register clock provider\n");
+
+	return 0;
+}
+
+static int qcom_eth_uniphy_calibrate(struct phy *phy)
+{
+	struct qcom_eth_uniphy *uniphy = phy_get_drvdata(phy);
+	dev_info(uniphy->dev, "calibrating\n");
+	return uniphy_calibrate(uniphy);
+}
+
+static const struct phy_ops qcom_eth_uniphy_ops = {
+	.calibrate = qcom_eth_uniphy_calibrate,
+	.owner = THIS_MODULE,
+};
+
+static int qcom_eth_uniphy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qcom_eth_uniphy *uniphy;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	int ret;
+
+	ret = cmn_init(pdev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to init cmn block\n");
+
+	uniphy = devm_kzalloc(dev, sizeof(*uniphy), GFP_KERNEL);
+	if (!uniphy)
+		return dev_err_probe(dev, -ENOMEM,
+				     "failed to allocate priv\n");
+
+	uniphy->dev = dev;
+	uniphy->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(uniphy->base))
+		return dev_err_probe(dev, PTR_ERR(uniphy->base),
+				     "failed to ioremap base\n");
+
+	uniphy->num_clks = devm_clk_bulk_get_all(uniphy->dev, &uniphy->clks);
+	if (uniphy->num_clks < 0)
+		return dev_err_probe(uniphy->dev, uniphy->num_clks,
+				     "failed to acquire clocks\n");
+
+	ret = clk_bulk_prepare_enable(uniphy->num_clks, uniphy->clks);
+	if (ret)
+		return dev_err_probe(uniphy->dev, ret,
+				     "failed to enable clocks\n");
+
+	uniphy->rst = devm_reset_control_array_get_exclusive(uniphy->dev);
+	if (IS_ERR_OR_NULL(uniphy->rst))
+		return dev_err_probe(uniphy->dev, PTR_ERR(uniphy->rst),
+				     "failed to acquire reset\n");
+
+	ret = reset_control_reset(uniphy->rst);
+	if (ret)
+		return dev_err_probe(uniphy->dev, ret,
+				     "failed to reset\n");
+
+	ret = uniphy_clkout_init(uniphy);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to init clkout\n");
+
+	ret = uniphy_mode_set(uniphy);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to set mode\n");
+
+	ret = uniphy_calibrate(uniphy);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to calibrate\n");
+
+	ret = uniphy_clk_register(uniphy);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register clocks\n");
+
+	phy = devm_phy_create(dev, dev->of_node, &qcom_eth_uniphy_ops);
+	if (IS_ERR(phy))
+		return dev_err_probe(dev, PTR_ERR(phy),
+				     "failed to register phy\n");
+
+	phy_set_drvdata(phy, uniphy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(phy_provider))
+		return dev_err_probe(dev, PTR_ERR(phy_provider),
+				     "failed to register phy provider\n");
+
+	return 0;
+}
+
+static const struct of_device_id qcom_eth_uniphy_of_match[] = {
+	{ .compatible = "qcom,ipq5018-eth-uniphy" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, qcom_eth_uniphy_of_match);
+
+static struct platform_driver qcom_eth_uniphy_driver = {
+	.probe	= qcom_eth_uniphy_probe,
+	.driver	= {
+		.name		= "qcom-eth-uniphy",
+		.of_match_table	= qcom_eth_uniphy_of_match,
+	},
+};
+module_platform_driver(qcom_eth_uniphy_driver);
+
+MODULE_DESCRIPTION("Qualcomm ethernet uniphy driver");
+MODULE_AUTHOR("Ziyang Huang <hzyitc@outlook.com>");
diff --git a/include/dt-bindings/phy/qcom-eth-uniphy.h b/include/dt-bindings/phy/qcom-eth-uniphy.h
new file mode 100644
index 000000000000..038f82522ccd
--- /dev/null
+++ b/include/dt-bindings/phy/qcom-eth-uniphy.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _DT_BINDINGS_PHY_QCOM_ETH_UNIPHY
+#define _DT_BINDINGS_PHY_QCOM_ETH_UNIPHY
+
+#define QCOM_ETH_UNIPHY_MODE_SGMII		0
+#define QCOM_ETH_UNIPHY_MODE_SGMII_PLUS		1
+
+#define QCOM_ETH_UNIPHY_CLKOUT_FREQ_25M		25000000
+#define QCOM_ETH_UNIPHY_CLKOUT_FREQ_50M		50000000
+
+#define QCOM_ETH_UNIPHY_CLKOUT_DS_1_5V		1500
+#define QCOM_ETH_UNIPHY_CLKOUT_DS_2_8V		2800
+
+#endif
-- 
2.40.1

Re: [PATCH 2/8] phy: Introduce Qualcomm ethernet uniphy driver

[Ziyang Huang]

在 2024/1/21 20:42, Ziyang Huang 写道:
> +#define rmwl(addr, mask, val) \
> +	writel(((readl(addr) & ~(mask)) | ((val) & (mask))), addr)
> +
> +static int cmn_init(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	void __iomem *cmn_base;
> +	void __iomem *tcsr_base;
> +	u32 val;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn");
> +	if (!res)
> +		return 0;
> +
> +	cmn_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR_OR_NULL(cmn_base))
> +		return PTR_ERR(cmn_base);
> +
> +	/* For IPQ50xx, tcsr is necessary to enable cmn block */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr");
> +	if (res) {
> +		tcsr_base = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR_OR_NULL(tcsr_base))
> +			return PTR_ERR(tcsr_base);
> +
> +		rmwl((tcsr_base + TCSR_ETH_CMN), TCSR_ETH_CMN_ENABLE,
> +		     TCSR_ETH_CMN_ENABLE);
> +	}
> +
> +	rmwl((cmn_base + CMN_PLL_REFCLK_SRC),
> +	     CMN_PLL_REFCLK_SRC_FROM_MASK,
> +	     CMN_PLL_REFCLK_SRC_FROM_REG);
> +	rmwl((cmn_base + CMN_PLL_REFCLK),
> +	     (CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_FREQ_MASK
> +	      | CMN_PLL_REFCLK_DIV_MASK),
> +	     (CMN_PLL_REFCLK_FREQ_48M | CMN_PLL_REFCLK_DIV(2)));
> +
> +	rmwl((cmn_base + CMN_PLL_CTRL), CMN_PLL_CTRL_RST_N, 0);
> +	msleep(1);
> +	rmwl((cmn_base + CMN_PLL_CTRL), CMN_PLL_CTRL_RST_N,
> +	     CMN_PLL_CTRL_RST_N);
> +	msleep(1);
> +
> +	return read_poll_timeout(readl, val,
> +				 (val & CMN_PLL_STATUS_LOCKED),
> +				 100, 200000, false,
> +				 (cmn_base + CMN_PLL_STATUS));
> +}
> +

Hi Andrew,

Sorry to bother you. But I can't make a decision here.

The CMN block (Seem like the Abbreviation of "component") controls the 
entire network block. It need to be configured before uniphy, mdio, 
gmac, etc.. In the past, Qualcomm put it in mdio driver. But UNIPHY need 
to been initializated before mdio because some PHYs/switchs use the 
outclk provided by UNIPHY as their main clocks.

So it seem like that it should be described in a separate node. But I 
couldn't find a suitable driver directory for it. Can you please give me 
some suggestions? Thanks.

Re: [PATCH 2/8] phy: Introduce Qualcomm ethernet uniphy driver

[Andrew Lunn]

On Tue, Jan 23, 2024 at 11:58:26PM +0800, Ziyang Huang wrote:
> 在 2024/1/21 20:42, Ziyang Huang 写道:
> > +#define rmwl(addr, mask, val) \
> > +	writel(((readl(addr) & ~(mask)) | ((val) & (mask))), addr)
> > +
> > +static int cmn_init(struct platform_device *pdev)
> > +{
> > +	struct resource *res;
> > +	void __iomem *cmn_base;
> > +	void __iomem *tcsr_base;
> > +	u32 val;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn");
> > +	if (!res)
> > +		return 0;
> > +
> > +	cmn_base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR_OR_NULL(cmn_base))
> > +		return PTR_ERR(cmn_base);
> > +
> > +	/* For IPQ50xx, tcsr is necessary to enable cmn block */
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr");
> > +	if (res) {
> > +		tcsr_base = devm_ioremap_resource(&pdev->dev, res);
> > +		if (IS_ERR_OR_NULL(tcsr_base))
> > +			return PTR_ERR(tcsr_base);
> > +
> > +		rmwl((tcsr_base + TCSR_ETH_CMN), TCSR_ETH_CMN_ENABLE,
> > +		     TCSR_ETH_CMN_ENABLE);
> > +	}
> > +
> > +	rmwl((cmn_base + CMN_PLL_REFCLK_SRC),
> > +	     CMN_PLL_REFCLK_SRC_FROM_MASK,
> > +	     CMN_PLL_REFCLK_SRC_FROM_REG);
> > +	rmwl((cmn_base + CMN_PLL_REFCLK),
> > +	     (CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_FREQ_MASK
> > +	      | CMN_PLL_REFCLK_DIV_MASK),
> > +	     (CMN_PLL_REFCLK_FREQ_48M | CMN_PLL_REFCLK_DIV(2)));
> > +
> > +	rmwl((cmn_base + CMN_PLL_CTRL), CMN_PLL_CTRL_RST_N, 0);
> > +	msleep(1);
> > +	rmwl((cmn_base + CMN_PLL_CTRL), CMN_PLL_CTRL_RST_N,
> > +	     CMN_PLL_CTRL_RST_N);
> > +	msleep(1);
> > +
> > +	return read_poll_timeout(readl, val,
> > +				 (val & CMN_PLL_STATUS_LOCKED),
> > +				 100, 200000, false,
> > +				 (cmn_base + CMN_PLL_STATUS));
> > +}
> > +
> 
> Hi Andrew,
> 
> Sorry to bother you. But I can't make a decision here.
> 
> The CMN block (Seem like the Abbreviation of "component") controls the
> entire network block. It need to be configured before uniphy, mdio, gmac,
> etc.. In the past, Qualcomm put it in mdio driver. But UNIPHY need to been
> initializated before mdio because some PHYs/switchs use the outclk provided
> by UNIPHY as their main clocks.
> 
> So it seem like that it should be described in a separate node. But I
> couldn't find a suitable driver directory for it. Can you please give me
> some suggestions? Thanks.

Maybe drivers/soc/qcom.

Does it provide any resources to the uniphy, mdio, gmac, etc? Anything
which can be used to link all the bits together?

Looking at CMN_PLL_CTRL_RST_N, could it be considered as a reset
driver? Each of the other drivers have a phandle to it, and use the
reset API to take it out of reset when they probe? That should give
you some ordering guarantees.

    Andrew

[PATCH 3/8] net: stmmac: Introduce Qualcomm IPQ50xx DWMAC driver

[Ziyang Huang]

Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   7 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-ipq50xx.c   | 155 ++++++++++++++++++
 3 files changed, 163 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 06c6871f8788..baf6601d759c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -78,6 +78,13 @@ config DWMAC_INGENIC
 	  device driver. This driver is used on for the Ingenic SoCs
 	  MAC ethernet controller.
 
+config DWMAC_IPQ50XX
+	tristate "Qualcomm IPQ50xx DWMAC driver"
+	default ARCH_QCOM
+	depends on OF && (ARCH_QCOM || COMPILE_TEST)
+	help
+	  Support for the Qualcomm IPQ50xx DWMAC.
+
 config DWMAC_IPQ806X
 	tristate "QCA IPQ806x DWMAC support"
 	default ARCH_QCOM
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 5b57aee19267..eec4405d76d2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -15,6 +15,7 @@ stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o
 obj-$(CONFIG_STMMAC_PLATFORM)	+= stmmac-platform.o
 obj-$(CONFIG_DWMAC_ANARION)	+= dwmac-anarion.o
 obj-$(CONFIG_DWMAC_INGENIC)	+= dwmac-ingenic.o
+obj-$(CONFIG_DWMAC_IPQ50XX)	+= dwmac-ipq50xx.o
 obj-$(CONFIG_DWMAC_IPQ806X)	+= dwmac-ipq806x.o
 obj-$(CONFIG_DWMAC_LPC18XX)	+= dwmac-lpc18xx.o
 obj-$(CONFIG_DWMAC_MEDIATEK)	+= dwmac-mediatek.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c
new file mode 100644
index 000000000000..de8e9a0b2cb6
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c
@@ -0,0 +1,155 @@
+#include <linux/clk.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/stmmac.h>
+
+#include "stmmac_platform.h"
+
+enum {
+	IPQ50XX_GMAC_CLK_SYS,
+	IPQ50XX_GMAC_CLK_CFG,
+	IPQ50XX_GMAC_CLK_AHB,
+	IPQ50XX_GMAC_CLK_AXI,
+	IPQ50XX_GMAC_CLK_RX,
+	IPQ50XX_GMAC_CLK_TX,
+	IPQ50XX_GMAC_CLK_PTP,
+};
+
+static const struct clk_bulk_data ipq50xx_gmac_clks[] = {
+	[IPQ50XX_GMAC_CLK_SYS]	= { .id = "sys" },
+	[IPQ50XX_GMAC_CLK_CFG]	= { .id = "cfg" },
+	[IPQ50XX_GMAC_CLK_AHB]	= { .id = "ahb" },
+	[IPQ50XX_GMAC_CLK_AXI]	= { .id = "axi" },
+	[IPQ50XX_GMAC_CLK_RX]	= { .id = "rx" },
+	[IPQ50XX_GMAC_CLK_TX]	= { .id = "tx" },
+	[IPQ50XX_GMAC_CLK_PTP]	= { .id = "ptp" },
+};
+
+struct ipq50xx_gmac {
+	struct device *dev;
+	struct clk_bulk_data clks[ARRAY_SIZE(ipq50xx_gmac_clks)];
+	struct reset_control *rst;
+	struct phy *uniphy;
+};
+
+static int ipq50xx_gmac_powerup(struct net_device *ndev, void *priv)
+{
+	struct ipq50xx_gmac *gmac = priv;
+	int ret;
+
+	ret = phy_init(gmac->uniphy);
+	if (ret)
+		return ret;
+
+	ret = phy_power_on(gmac->uniphy);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void ipq50xx_gmac_fix_speed(void *priv, unsigned int speed, unsigned int mode)
+{
+	struct ipq50xx_gmac *gmac = priv;
+	unsigned long rate;
+
+	switch(speed) {
+		case SPEED_10:
+			rate = 2500000;
+			break;
+		case SPEED_100:
+			rate = 25000000;
+			break;
+		case SPEED_1000:
+			rate = 125000000;
+			break;
+		case SPEED_2500:
+			rate = 312500000;
+			break;
+		default:
+			dev_err(gmac->dev, "Unsupported speed: %d\n", speed);
+			rate = 125000000;
+			break;
+	}
+
+	clk_set_rate(gmac->clks[IPQ50XX_GMAC_CLK_RX].clk, rate);
+	clk_set_rate(gmac->clks[IPQ50XX_GMAC_CLK_TX].clk, rate);
+	phy_calibrate(gmac->uniphy);
+}
+
+static int ipq50xx_gmac_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct stmmac_resources stmmac_res;
+	struct plat_stmmacenet_data *plat_dat;
+	struct ipq50xx_gmac *gmac;
+	int ret;
+
+	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to get stmmac platform resources\n");
+
+	plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	if (IS_ERR_OR_NULL(plat_dat))
+		return dev_err_probe(dev, PTR_ERR(plat_dat),
+				     "failed to parse stmmac dt parameters\n");
+
+	gmac = devm_kzalloc(dev, sizeof(*gmac), GFP_KERNEL);
+	if (!gmac)
+		return dev_err_probe(dev, -ENOMEM,
+				     "failed to allocate priv\n");
+
+	gmac->dev = dev;
+
+	memcpy(gmac->clks, ipq50xx_gmac_clks, sizeof(gmac->clks));
+	ret = devm_clk_bulk_get_optional(dev, ARRAY_SIZE(gmac->clks), gmac->clks);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to acquire clocks\n");
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(gmac->clks), gmac->clks);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to enable clocks\n");
+
+	gmac->rst = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR_OR_NULL(gmac->rst))
+		return dev_err_probe(dev, PTR_ERR(gmac->rst),
+				     "failed to acquire reset\n");
+
+	ret = reset_control_reset(gmac->rst);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to reset\n");
+
+	gmac->uniphy = devm_phy_optional_get(dev, "uniphy");
+	if (IS_ERR(gmac->uniphy))
+		return dev_err_probe(dev, PTR_ERR(gmac->uniphy),
+				     "failed to acquire uniphy\n");
+
+	plat_dat->bsp_priv = gmac;
+	plat_dat->serdes_powerup = ipq50xx_gmac_powerup;
+	plat_dat->fix_mac_speed = ipq50xx_gmac_fix_speed;
+
+	return stmmac_dvr_probe(dev, plat_dat, &stmmac_res);
+}
+
+static const struct of_device_id ipq50xx_gmac_dwmac_match[] = {
+	{ .compatible = "qcom,ipq50xx-gmac" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ipq50xx_gmac_dwmac_match);
+
+static struct platform_driver ipq50xx_gmac_dwmac_driver = {
+	.probe = ipq50xx_gmac_probe,
+	.driver = {
+		.name		= "ipq50xx-gmac-dwmac",
+		.of_match_table	= ipq50xx_gmac_dwmac_match,
+	},
+};
+module_platform_driver(ipq50xx_gmac_dwmac_driver);
+
+MODULE_DESCRIPTION("Qualcomm IPQ50xx DWMAC driver");
+MODULE_AUTHOR("Ziyang Huang <hzyitc@outlook.com>");
-- 
2.40.1

Re: [PATCH 3/8] net: stmmac: Introduce Qualcomm IPQ50xx DWMAC driver

[kernel test robot]

Hi Ziyang,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on clk/clk-next pza/reset/next linus/master v6.8-rc1 next-20240123]
[cannot apply to pza/imx-drm/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ziyang-Huang/net-phy-Introduce-Qualcomm-IPQ5018-internal-PHY-driver/20240121-204840
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/TYZPR01MB5556B8833322A83632709631C9762%40TYZPR01MB5556.apcprd01.prod.exchangelabs.com
patch subject: [PATCH 3/8] net: stmmac: Introduce Qualcomm IPQ50xx DWMAC driver
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20240124/202401241342.SL4CiC8m-lkp@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project a31a60074717fc40887cfe132b77eec93bedd307)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240124/202401241342.SL4CiC8m-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401241342.SL4CiC8m-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c:94:13: error: call to undeclared function 'stmmac_probe_config_dt'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      94 |         plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
         |                    ^
   drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c:94:13: note: did you mean 'devm_stmmac_probe_config_dt'?
   drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h:15:1: note: 'devm_stmmac_probe_config_dt' declared here
      15 | devm_stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac);
         | ^
>> drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c:94:11: error: incompatible integer to pointer conversion assigning to 'struct plat_stmmacenet_data *' from 'int' [-Wint-conversion]
      94 |         plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
         |                  ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   2 errors generated.


vim +/stmmac_probe_config_dt +94 drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c

    80	
    81	static int ipq50xx_gmac_probe(struct platform_device *pdev)
    82	{
    83		struct device *dev = &pdev->dev;
    84		struct stmmac_resources stmmac_res;
    85		struct plat_stmmacenet_data *plat_dat;
    86		struct ipq50xx_gmac *gmac;
    87		int ret;
    88	
    89		ret = stmmac_get_platform_resources(pdev, &stmmac_res);
    90		if (ret)
    91			return dev_err_probe(dev, ret,
    92					     "failed to get stmmac platform resources\n");
    93	
  > 94		plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
    95		if (IS_ERR_OR_NULL(plat_dat))
    96			return dev_err_probe(dev, PTR_ERR(plat_dat),
    97					     "failed to parse stmmac dt parameters\n");
    98	
    99		gmac = devm_kzalloc(dev, sizeof(*gmac), GFP_KERNEL);
   100		if (!gmac)
   101			return dev_err_probe(dev, -ENOMEM,
   102					     "failed to allocate priv\n");
   103	
   104		gmac->dev = dev;
   105	
   106		memcpy(gmac->clks, ipq50xx_gmac_clks, sizeof(gmac->clks));
   107		ret = devm_clk_bulk_get_optional(dev, ARRAY_SIZE(gmac->clks), gmac->clks);
   108		if (ret)
   109			return dev_err_probe(dev, ret,
   110					     "failed to acquire clocks\n");
   111	
   112		ret = clk_bulk_prepare_enable(ARRAY_SIZE(gmac->clks), gmac->clks);
   113		if (ret)
   114			return dev_err_probe(dev, ret,
   115					     "failed to enable clocks\n");
   116	
   117		gmac->rst = devm_reset_control_array_get_exclusive(dev);
   118		if (IS_ERR_OR_NULL(gmac->rst))
   119			return dev_err_probe(dev, PTR_ERR(gmac->rst),
   120					     "failed to acquire reset\n");
   121	
   122		ret = reset_control_reset(gmac->rst);
   123		if (ret)
   124			return dev_err_probe(dev, ret,
   125					     "failed to reset\n");
   126	
   127		gmac->uniphy = devm_phy_optional_get(dev, "uniphy");
   128		if (IS_ERR(gmac->uniphy))
   129			return dev_err_probe(dev, PTR_ERR(gmac->uniphy),
   130					     "failed to acquire uniphy\n");
   131	
   132		plat_dat->bsp_priv = gmac;
   133		plat_dat->serdes_powerup = ipq50xx_gmac_powerup;
   134		plat_dat->fix_mac_speed = ipq50xx_gmac_fix_speed;
   135	
   136		return stmmac_dvr_probe(dev, plat_dat, &stmmac_res);
   137	}
   138	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/8] net: stmmac: Introduce Qualcomm IPQ50xx DWMAC driver

[kernel test robot]

Hi Ziyang,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on clk/clk-next pza/reset/next linus/master v6.8-rc1 next-20240123]
[cannot apply to pza/imx-drm/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ziyang-Huang/net-phy-Introduce-Qualcomm-IPQ5018-internal-PHY-driver/20240121-204840
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/TYZPR01MB5556B8833322A83632709631C9762%40TYZPR01MB5556.apcprd01.prod.exchangelabs.com
patch subject: [PATCH 3/8] net: stmmac: Introduce Qualcomm IPQ50xx DWMAC driver
config: parisc-allmodconfig (https://download.01.org/0day-ci/archive/20240124/202401241733.H4p1Af19-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240124/202401241733.H4p1Af19-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401241733.H4p1Af19-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c: In function 'ipq50xx_gmac_probe':
>> drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c:94:20: error: implicit declaration of function 'stmmac_probe_config_dt'; did you mean 'devm_stmmac_probe_config_dt'? [-Werror=implicit-function-declaration]
      94 |         plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
         |                    ^~~~~~~~~~~~~~~~~~~~~~
         |                    devm_stmmac_probe_config_dt
>> drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c:94:18: warning: assignment to 'struct plat_stmmacenet_data *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      94 |         plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
         |                  ^
   cc1: some warnings being treated as errors


vim +94 drivers/net/ethernet/stmicro/stmmac/dwmac-ipq50xx.c

    80	
    81	static int ipq50xx_gmac_probe(struct platform_device *pdev)
    82	{
    83		struct device *dev = &pdev->dev;
    84		struct stmmac_resources stmmac_res;
    85		struct plat_stmmacenet_data *plat_dat;
    86		struct ipq50xx_gmac *gmac;
    87		int ret;
    88	
    89		ret = stmmac_get_platform_resources(pdev, &stmmac_res);
    90		if (ret)
    91			return dev_err_probe(dev, ret,
    92					     "failed to get stmmac platform resources\n");
    93	
  > 94		plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
    95		if (IS_ERR_OR_NULL(plat_dat))
    96			return dev_err_probe(dev, PTR_ERR(plat_dat),
    97					     "failed to parse stmmac dt parameters\n");
    98	
    99		gmac = devm_kzalloc(dev, sizeof(*gmac), GFP_KERNEL);
   100		if (!gmac)
   101			return dev_err_probe(dev, -ENOMEM,
   102					     "failed to allocate priv\n");
   103	
   104		gmac->dev = dev;
   105	
   106		memcpy(gmac->clks, ipq50xx_gmac_clks, sizeof(gmac->clks));
   107		ret = devm_clk_bulk_get_optional(dev, ARRAY_SIZE(gmac->clks), gmac->clks);
   108		if (ret)
   109			return dev_err_probe(dev, ret,
   110					     "failed to acquire clocks\n");
   111	
   112		ret = clk_bulk_prepare_enable(ARRAY_SIZE(gmac->clks), gmac->clks);
   113		if (ret)
   114			return dev_err_probe(dev, ret,
   115					     "failed to enable clocks\n");
   116	
   117		gmac->rst = devm_reset_control_array_get_exclusive(dev);
   118		if (IS_ERR_OR_NULL(gmac->rst))
   119			return dev_err_probe(dev, PTR_ERR(gmac->rst),
   120					     "failed to acquire reset\n");
   121	
   122		ret = reset_control_reset(gmac->rst);
   123		if (ret)
   124			return dev_err_probe(dev, ret,
   125					     "failed to reset\n");
   126	
   127		gmac->uniphy = devm_phy_optional_get(dev, "uniphy");
   128		if (IS_ERR(gmac->uniphy))
   129			return dev_err_probe(dev, PTR_ERR(gmac->uniphy),
   130					     "failed to acquire uniphy\n");
   131	
   132		plat_dat->bsp_priv = gmac;
   133		plat_dat->serdes_powerup = ipq50xx_gmac_powerup;
   134		plat_dat->fix_mac_speed = ipq50xx_gmac_fix_speed;
   135	
   136		return stmmac_dvr_probe(dev, plat_dat, &stmmac_res);
   137	}
   138	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 4/8] clk: qcom: gcc-ipq5018: correct gcc_gmac0_sys_clk reg

[Ziyang Huang]

Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
---
 drivers/clk/qcom/gcc-ipq5018.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
index 19dc2b71cacf..3ba2fd53262d 100644
--- a/drivers/clk/qcom/gcc-ipq5018.c
+++ b/drivers/clk/qcom/gcc-ipq5018.c
@@ -1756,7 +1756,7 @@ static struct clk_branch gcc_gmac0_sys_clk = {
 	.halt_check = BRANCH_HALT_DELAY,
 	.halt_bit = 31,
 	.clkr = {
-		.enable_reg = 0x683190,
+		.enable_reg = 0x68190,
 		.enable_mask = BIT(0),
 		.hw.init = &(struct clk_init_data) {
 			.name = "gcc_gmac0_sys_clk",
-- 
2.40.1

Re: [PATCH 4/8] clk: qcom: gcc-ipq5018: correct gcc_gmac0_sys_clk reg

[Andrew Lunn]

On Sun, Jan 21, 2024 at 08:42:33PM +0800, Ziyang Huang wrote:

This, and all your patches need a Commit message.

What is the effect of this typo? What is broken because of it? Should
this be backported to stable? Does it need a Fixed: tag?

> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>


    Andrew

---
pw-bot: cr

Re: [PATCH 4/8] clk: qcom: gcc-ipq5018: correct gcc_gmac0_sys_clk reg

[Ziyang Huang]

在 2024/1/22 0:28, Andrew Lunn 写道:
> On Sun, Jan 21, 2024 at 08:42:33PM +0800, Ziyang Huang wrote:
> 
> This, and all your patches need a Commit message.
> 
> What is the effect of this typo? What is broken because of it? Should
> this be backported to stable? Does it need a Fixed: tag?
> 

Sorry for this, will add in upcoming patches.

>> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
> 
> 
>      Andrew
> 
> ---
> pw-bot: cr

[PATCH 5/8] clk: qcom: support for duplicate freq in RCG2 freq table

[Ziyang Huang]

From: Praveenkumar I <ipkumar@codeaurora.org>

Currently RCG code looks up the frequency table during set
rate and return the first available frequency greater than
requested rate. If CLK_SET_RATE_PARENT flag is set then the
set_rate request will go to its parent otherwise the clock
framework will configure pre-div, m and n according to the
returned frequency table entry. In this case, it is assuming
that parent clock will run in the same frequency with which
pre-div, m and n has been derived. But it may be possible
that the parent clock supports multiple frequency and the
same frequency can be derived with different pre-div, m and
n values depending upon current frequency.  Also, the same
frequency can be derived from different parent sources and
currently there is no option for having duplicate
frequencies in frequency table and choosing the best one
according to current rate.

Now this patch adds the support for having duplicate
frequencies in frequency table. During set rate, it will
compare the actual rate for each entry with requested rate
and will select the best entry in which the difference will
be less.

The existing functionality won’t be affected with this code
change since this code change will hit only if frequency
table has duplicate values.

Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
(cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>

Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad

Pick from https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/6dfb368bae130bee58e00ddf8330b55066e1c8c5

Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
---
 drivers/clk/qcom/clk-rcg2.c | 86 ++++++++++++++++++++++++++++---------
 1 file changed, 66 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index e22baf3a7112..6141e4991fbc 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -209,26 +209,82 @@ clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 	return __clk_rcg2_recalc_rate(hw, parent_rate, cfg);
 }
 
-static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f,
-				    struct clk_rate_request *req,
-				    enum freq_policy policy)
+static const struct freq_tbl *
+clk_rcg2_find_best_freq(struct clk_hw *hw, const struct freq_tbl *f,
+			unsigned long rate, enum freq_policy policy)
 {
-	unsigned long clk_flags, rate = req->rate;
-	struct clk_hw *p;
+	unsigned long req_rate = rate, best = 0, freq;
 	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
 	int index;
+	u64 tmp;
+	const struct freq_tbl *best_ftable = NULL;
 
 	switch (policy) {
 	case FLOOR:
-		f = qcom_find_freq_floor(f, rate);
+		f = qcom_find_freq_floor(rcg->freq_tbl, rate);
 		break;
 	case CEIL:
-		f = qcom_find_freq(f, rate);
+		f = qcom_find_freq(rcg->freq_tbl, rate);
 		break;
 	default:
-		return -EINVAL;
+		return best_ftable;
 	}
 
+	/*
+	 * Check for duplicate frequencies in frequency table if
+	 * CLK_SET_RATE_PARENT flag is not set
+	 */
+	if (!f || (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) ||
+	    ((f->freq && (f + 1)->freq != f->freq)))
+		return f;
+
+	/*
+	 * Check for all the duplicate entries in frequency table and
+	 * calculate the actual rate from current parent rate with each
+	 * entries pre_div, m and n values. The entry, which gives the
+	 * minimum difference in requested rate and actual rate, will be
+	 * selected as the best one.
+	 */
+	for (freq = f->freq; freq == f->freq; f++) {
+		index = qcom_find_src_index(hw, rcg->parent_map, f->src);
+		if (index < 0)
+			continue;
+
+		rate =  clk_hw_get_rate(clk_hw_get_parent_by_index(hw, index));
+		if (rcg->hid_width && f->pre_div) {
+			rate *= 2;
+			rate /= f->pre_div + 1;
+		}
+
+		if (rcg->mnd_width && f->n) {
+			tmp = rate;
+			tmp = tmp * f->n;
+			do_div(tmp, f->m);
+			rate = tmp;
+		}
+
+		if (abs(req_rate - rate) < abs(best - rate)) {
+			best_ftable = f;
+			best = rate;
+
+			if (req_rate == rate)
+				break;
+		}
+	}
+
+	return best_ftable;
+}
+
+static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f,
+				    struct clk_rate_request *req,
+				    enum freq_policy policy)
+{
+	unsigned long clk_flags, rate = req->rate;
+	struct clk_hw *p;
+	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+	int index;
+
+	f = clk_rcg2_find_best_freq(hw, f, rate, policy);
 	if (!f)
 		return -EINVAL;
 
@@ -360,17 +416,7 @@ static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate,
 	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
 	const struct freq_tbl *f;
 
-	switch (policy) {
-	case FLOOR:
-		f = qcom_find_freq_floor(rcg->freq_tbl, rate);
-		break;
-	case CEIL:
-		f = qcom_find_freq(rcg->freq_tbl, rate);
-		break;
-	default:
-		return -EINVAL;
-	}
-
+	f = clk_rcg2_find_best_freq(hw, rcg->freq_tbl, rate, policy);
 	if (!f)
 		return -EINVAL;
 
@@ -1032,7 +1078,7 @@ static int clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate,
 	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
 	const struct freq_tbl *f;
 
-	f = qcom_find_freq(rcg->freq_tbl, rate);
+	f = clk_rcg2_find_best_freq(hw, rcg->freq_tbl, rate, CEIL);
 	if (!f)
 		return -EINVAL;
 
-- 
2.40.1

Re: [PATCH 5/8] clk: qcom: support for duplicate freq in RCG2 freq table

[Andrew Lunn]

On Sun, Jan 21, 2024 at 08:42:34PM +0800, Ziyang Huang wrote:
> From: Praveenkumar I <ipkumar@codeaurora.org>
> 
> Currently RCG code looks up the frequency table during set
> rate and return the first available frequency greater than
> requested rate. If CLK_SET_RATE_PARENT flag is set then the
> set_rate request will go to its parent otherwise the clock
> framework will configure pre-div, m and n according to the
> returned frequency table entry. In this case, it is assuming
> that parent clock will run in the same frequency with which
> pre-div, m and n has been derived. But it may be possible
> that the parent clock supports multiple frequency and the
> same frequency can be derived with different pre-div, m and
> n values depending upon current frequency.  Also, the same
> frequency can be derived from different parent sources and
> currently there is no option for having duplicate
> frequencies in frequency table and choosing the best one
> according to current rate.
> 
> Now this patch adds the support for having duplicate
> frequencies in frequency table. During set rate, it will
> compare the actual rate for each entry with requested rate
> and will select the best entry in which the difference will
> be less.
> 
> The existing functionality won’t be affected with this code
> change since this code change will hit only if frequency
> table has duplicate values.

A good commit message for a change!

> 
> Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> (cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
> Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>
> 
> Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad
> 
> Pick from https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/6dfb368bae130bee58e00ddf8330b55066e1c8c5
> 
> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>

Please clean up these tags. These Change-ID tags are meaningless in
mainline. 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2 is not in mainline
either. The picked from might be interesting, but please put it into
the body of the commit message, not mixed in with the tags.

Who actually wrote this patch? The first Signed-off-by: is from
Abhishek Sahu. But you have a From of Praveenkumar I ?

    Andrew

---
pw-bot: cr

Re: [PATCH 5/8] clk: qcom: support for duplicate freq in RCG2 freq table

[Ziyang Huang]

在 2024/1/22 0:57, Andrew Lunn 写道:
> On Sun, Jan 21, 2024 at 08:42:34PM +0800, Ziyang Huang wrote:
>> From: Praveenkumar I <ipkumar@codeaurora.org>
>>
>> Currently RCG code looks up the frequency table during set
>> rate and return the first available frequency greater than
>> requested rate. If CLK_SET_RATE_PARENT flag is set then the
>> set_rate request will go to its parent otherwise the clock
>> framework will configure pre-div, m and n according to the
>> returned frequency table entry. In this case, it is assuming
>> that parent clock will run in the same frequency with which
>> pre-div, m and n has been derived. But it may be possible
>> that the parent clock supports multiple frequency and the
>> same frequency can be derived with different pre-div, m and
>> n values depending upon current frequency.  Also, the same
>> frequency can be derived from different parent sources and
>> currently there is no option for having duplicate
>> frequencies in frequency table and choosing the best one
>> according to current rate.
>>
>> Now this patch adds the support for having duplicate
>> frequencies in frequency table. During set rate, it will
>> compare the actual rate for each entry with requested rate
>> and will select the best entry in which the difference will
>> be less.
>>
>> The existing functionality won’t be affected with this code
>> change since this code change will hit only if frequency
>> table has duplicate values.
> 
> A good commit message for a change!
> 
>>
>> Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> (cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
>> Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>
>>
>> Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad
>>
>> Pick from https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/6dfb368bae130bee58e00ddf8330b55066e1c8c5
>>
>> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
> 
> Please clean up these tags. These Change-ID tags are meaningless in
> mainline. 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2 is not in mainline
> either. The picked from might be interesting, but please put it into
> the body of the commit message, not mixed in with the tags.
> 
> Who actually wrote this patch? The first Signed-off-by: is from
> Abhishek Sahu. But you have a From of Praveenkumar I ?

I have no idea about this. This patch is from Qualcomm vendor linux 
code. And it's necessary for choosing parent and calculating clock rate 
correctly.

What's more, I don't known how to deal with these commit message since 
I'm not the author and I'm not sure do I have right to edit them even 
though this is GPL code.

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

Re: [PATCH 5/8] clk: qcom: support for duplicate freq in RCG2 freq table

[Andrew Lunn]

> > > Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
> > > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> > > (cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
> > > Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>
> > > 
> > > Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad
> > > 
> > > Pick from https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/6dfb368bae130bee58e00ddf8330b55066e1c8c5
> > > 
> > > Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
> > 
> > Please clean up these tags. These Change-ID tags are meaningless in
> > mainline. 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2 is not in mainline
> > either. The picked from might be interesting, but please put it into
> > the body of the commit message, not mixed in with the tags.
> > 
> > Who actually wrote this patch? The first Signed-off-by: is from
> > Abhishek Sahu. But you have a From of Praveenkumar I ?
> 
> I have no idea about this. This patch is from Qualcomm vendor linux code.

O.K. Since this is direct from the vendor, who probably does not track
code authorship correctly, i would say the author in git is probably
wrong. I would set the author: to Abhishek Sahu <absahu@codeaurora.org>.

> What's more, I don't known how to deal with these commit message since I'm
> not the author and I'm not sure do I have right to edit them even though
> this is GPL code.

You should keep all the Signed-off-by, in the order they are. But the
Change-Id is meaningless, so there is no problem removing them.

	  Andrew

Re: [PATCH 5/8] clk: qcom: support for duplicate freq in RCG2 freq table

[Ziyang Huang]

在 2024/1/23 1:34, Andrew Lunn 写道:
>>>> Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
>>>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>>>> (cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
>>>> Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>
>>>>
>>>> Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad
>>>>
>>>> Pick from https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/6dfb368bae130bee58e00ddf8330b55066e1c8c5
>>>>
>>>> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
>>>
>>> Please clean up these tags. These Change-ID tags are meaningless in
>>> mainline. 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2 is not in mainline
>>> either. The picked from might be interesting, but please put it into
>>> the body of the commit message, not mixed in with the tags.
>>>
>>> Who actually wrote this patch? The first Signed-off-by: is from
>>> Abhishek Sahu. But you have a From of Praveenkumar I ?
>>
>> I have no idea about this. This patch is from Qualcomm vendor linux code.
> 
> O.K. Since this is direct from the vendor, who probably does not track
> code authorship correctly, i would say the author in git is probably
> wrong. I would set the author: to Abhishek Sahu <absahu@codeaurora.org>.
> 
>> What's more, I don't known how to deal with these commit message since I'm
>> not the author and I'm not sure do I have right to edit them even though
>> this is GPL code.
> 
> You should keep all the Signed-off-by, in the order they are. But the
> Change-Id is meaningless, so there is no problem removing them.
> 
> 	  Andrew

These days, I known a new knowledge: SGMII+ only support 2.5G. 1000M 
can't work with SGMII+ mode. So the 1000M frequency divided from 3.125G 
can be dropped. Then we don't have duplicated frequency and this patch 
is unnecessary. Will test my guess.

Re: [PATCH 5/8] clk: qcom: support for duplicate freq in RCG2 freq table

[Krzysztof Kozlowski]

On 21/01/2024 13:42, Ziyang Huang wrote:
> From: Praveenkumar I <ipkumar@codeaurora.org>
> 
> Currently RCG code looks up the frequency table during set
> rate and return the first available frequency greater than
> requested rate. If CLK_SET_RATE_PARENT flag is set then the
> set_rate request will go to its parent otherwise the clock
> framework will configure pre-div, m and n according to the
> returned frequency table entry. In this case, it is assuming
> that parent clock will run in the same frequency with which
> pre-div, m and n has been derived. But it may be possible
> that the parent clock supports multiple frequency and the
> same frequency can be derived with different pre-div, m and
> n values depending upon current frequency.  Also, the same
> frequency can be derived from different parent sources and
> currently there is no option for having duplicate
> frequencies in frequency table and choosing the best one
> according to current rate.
> 
> Now this patch adds the support for having duplicate
> frequencies in frequency table. During set rate, it will
> compare the actual rate for each entry with requested rate
> and will select the best entry in which the difference will
> be less.
> 
> The existing functionality won’t be affected with this code
> change since this code change will hit only if frequency
> table has duplicate values.
> 
> Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> (cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
> Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>
> 
> Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

Best regards,
Krzysztof

Re: [PATCH 5/8] clk: qcom: support for duplicate freq in RCG2 freq table

[Ziyang Huang]

在 2024/1/22 15:55, Krzysztof Kozlowski 写道:
> On 21/01/2024 13:42, Ziyang Huang wrote:
> [...]
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
> 
> Best regards,
> Krzysztof
> 

Hi Krzysztof,

Sorrr for my mistake. For easily comparison, I added these patches to 
git index, which mess up ./scripts/checkpatch.pl and 
./scripts/get_maintainer.pl and led to wrong results. After restored 
them, I noticed these issues, will fix them in next patches.

[PATCH 6/8] net: mdio: ipq4019: support reset control

[Ziyang Huang]

Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
---
 drivers/net/mdio/mdio-ipq4019.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 78b93de636f5..b52c5e1fb99b 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -12,6 +12,7 @@
 #include <linux/phy.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
+#include <linux/reset.h>
 
 #define MDIO_MODE_REG				0x40
 #define MDIO_ADDR_REG				0x44
@@ -40,6 +41,7 @@
 struct ipq4019_mdio_data {
 	void __iomem	*membase;
 	void __iomem *eth_ldo_rdy;
+	struct reset_control *rst;
 	struct clk *mdio_clk;
 };
 
@@ -219,6 +221,10 @@ static int ipq_mdio_reset(struct mii_bus *bus)
 		fsleep(IPQ_PHY_SET_DELAY_US);
 	}
 
+	ret = reset_control_reset(priv->rst);
+	if (ret)
+		return ret;
+
 	/* 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)
@@ -248,6 +254,10 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->membase))
 		return PTR_ERR(priv->membase);
 
+	priv->rst = devm_reset_control_array_get_optional_exclusive(&pdev->dev);
+	if (IS_ERR(priv->rst))
+		return PTR_ERR(priv->rst);
+
 	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);
-- 
2.40.1

Re: [PATCH 6/8] net: mdio: ipq4019: support reset control

[Andrew Lunn]

On Sun, Jan 21, 2024 at 08:42:35PM +0800, Ziyang Huang wrote:

In this case, you need to document why the MDIO core reset controller
code does not work for you.

You are also missing the device tree binding again.

> @@ -248,6 +254,10 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->membase))
>  		return PTR_ERR(priv->membase);
>  
> +	priv->rst = devm_reset_control_array_get_optional_exclusive(&pdev->dev);
> +	if (IS_ERR(priv->rst))
> +		return PTR_ERR(priv->rst);

How many resets does this MDIO controller have? Would
devm_reset_control_get_optional_exclusive() be sufficient.


    Andrew

---
pw-bot: cr

Re: [PATCH 6/8] net: mdio: ipq4019: support reset control

[Ziyang Huang]

在 2024/1/22 0:35, Andrew Lunn 写道:
> On Sun, Jan 21, 2024 at 08:42:35PM +0800, Ziyang Huang wrote:
> 
> In this case, you need to document why the MDIO core reset controller
> code does not work for you.
> 
> You are also missing the device tree binding again.

Sorry. Will write it.

> 
>> @@ -248,6 +254,10 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>>   	if (IS_ERR(priv->membase))
>>   		return PTR_ERR(priv->membase);
>>   
>> +	priv->rst = devm_reset_control_array_get_optional_exclusive(&pdev->dev);
>> +	if (IS_ERR(priv->rst))
>> +		return PTR_ERR(priv->rst);
> 
> How many resets does this MDIO controller have? Would
> devm_reset_control_get_optional_exclusive() be sufficient.
> 

Will explain in the following email.

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

[PATCH 7/8] arm64: dts: qcom: ipq5018: enable ethernet support

[Ziyang Huang]

Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
---
 arch/arm64/boot/dts/qcom/ipq5018.dtsi | 120 +++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
index e502a3ecf4b7..b36e5c2136b7 100644
--- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
@@ -94,6 +94,63 @@ soc: soc@0 {
 		#size-cells = <1>;
 		ranges = <0 0 0 0xffffffff>;
 
+		mdio0: mdio@88000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "qcom,ipq5018-mdio", "qcom,qca-mdio";
+			reg = <0x88000 0x64>;
+			resets = <&gcc GCC_GEPHY_MDC_SW_ARES>,
+				 <&gcc GCC_GEPHY_DSP_HW_ARES>;
+			clocks = <&gcc GCC_MDIO0_AHB_CLK>;
+			clock-names = "gcc_mdio_ahb_clk";
+			status = "disabled";
+
+			gephy: ethernet-phy@7 {
+				#clock-cells = <1>;
+				reg = <7>;
+				resets = <&gcc GCC_GEPHY_BCR>,
+					 <&gcc GCC_GEPHY_RX_ARES>,
+					 <&gcc GCC_GEPHY_TX_ARES>;
+				clocks = <&gcc GCC_GEPHY_RX_CLK>,
+					 <&gcc GCC_GEPHY_TX_CLK>;
+			};
+		};
+
+		mdio1: mdio@90000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "qcom,ipq5018-mdio";
+			reg = <0x90000 0x64>;
+			clocks = <&gcc GCC_MDIO1_AHB_CLK>;
+			clock-names = "gcc_mdio_ahb_clk";
+			status = "disabled";
+		};
+
+		uniphy0: eth-uniphy@98000 {
+			compatible = "qcom,ipq5018-eth-uniphy";
+			#clock-cells = <1>;
+			#phy-cells = <0>;
+			reg = <0x98000 0x800>,
+			      <0x9b000 0x800>,
+			      <0x19475c4 0x4>;
+			reg-names = "uniphy",
+				    "cmn",
+				    "tcsr";
+			clocks = <&gcc GCC_CMN_BLK_AHB_CLK>,
+				 <&gcc GCC_CMN_BLK_SYS_CLK>,
+				 <&gcc GCC_UNIPHY_AHB_CLK>,
+				 <&gcc GCC_UNIPHY_SYS_CLK>,
+				 <&gcc GCC_UNIPHY_RX_CLK>,
+				 <&gcc GCC_UNIPHY_TX_CLK>;
+			resets = <&gcc GCC_UNIPHY_BCR>,
+				 <&gcc GCC_UNIPHY_AHB_ARES>,
+				 <&gcc GCC_UNIPHY_SYS_ARES>,
+				 <&gcc GCC_UNIPHY_RX_ARES>,
+				 <&gcc GCC_UNIPHY_TX_ARES>;
+
+			status = "disabled";
+		};
+
 		tlmm: pinctrl@1000000 {
 			compatible = "qcom,ipq5018-tlmm";
 			reg = <0x01000000 0x300000>;
@@ -120,10 +177,10 @@ gcc: clock-controller@1800000 {
 				 <0>,
 				 <0>,
 				 <0>,
-				 <0>,
-				 <0>,
-				 <0>,
-				 <0>;
+				 <&gephy 0>,
+				 <&gephy 1>,
+				 <&uniphy0 0>,
+				 <&uniphy0 1>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;
@@ -244,6 +301,61 @@ frame@b128000 {
 				status = "disabled";
 			};
 		};
+
+		gmac0: ethernet@39c00000 {
+			compatible = "qcom,ipq50xx-gmac", "snps,dwmac";
+			reg = <0x39C00000 0x10000>;
+			clocks = <&gcc GCC_GMAC0_SYS_CLK>,
+				 <&gcc GCC_GMAC0_CFG_CLK>,
+				 <&gcc GCC_SNOC_GMAC0_AHB_CLK>,
+				 <&gcc GCC_SNOC_GMAC0_AXI_CLK>,
+				 <&gcc GCC_GMAC0_RX_CLK>,
+				 <&gcc GCC_GMAC0_TX_CLK>,
+				 <&gcc GCC_GMAC0_PTP_CLK>;
+			clock-names = "sys",
+				      "cfg",
+				      "ahb",
+				      "axi",
+				      "rx",
+				      "tx",
+				      "ptp";
+			resets = <&gcc GCC_GMAC0_BCR>;
+			interrupts = <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "macirq";
+
+			phy-handle = <&gephy>;
+			phy-mode = "sgmii";
+
+			status = "disabled";
+		};
+
+		gmac1: ethernet@39d00000 {
+			compatible = "qcom,ipq50xx-gmac", "snps,dwmac";
+			reg = <0x39D00000 0x10000>;
+			reg-names = "stmmaceth";
+			clocks = <&gcc GCC_GMAC1_SYS_CLK>,
+				 <&gcc GCC_GMAC1_CFG_CLK>,
+				 <&gcc GCC_SNOC_GMAC1_AHB_CLK>,
+				 <&gcc GCC_SNOC_GMAC1_AXI_CLK>,
+				 <&gcc GCC_GMAC1_RX_CLK>,
+				 <&gcc GCC_GMAC1_TX_CLK>,
+				 <&gcc GCC_GMAC1_PTP_CLK>;
+			clock-names = "sys",
+				      "cfg",
+				      "ahb",
+				      "axi",
+				      "rx",
+				      "tx",
+				      "ptp";
+			resets = <&gcc GCC_GMAC1_BCR>;
+			interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "macirq";
+
+			phys = <&uniphy0>;
+			phy-names = "uniphy";
+
+			status = "disabled";
+		};
 	};
 
 	timer {
-- 
2.40.1

Re: [PATCH 7/8] arm64: dts: qcom: ipq5018: enable ethernet support

[Andrew Lunn]

On Sun, Jan 21, 2024 at 08:42:36PM +0800, Ziyang Huang wrote:
> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5018.dtsi | 120 +++++++++++++++++++++++++-
>  1 file changed, 116 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> index e502a3ecf4b7..b36e5c2136b7 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> @@ -94,6 +94,63 @@ soc: soc@0 {
>  		#size-cells = <1>;
>  		ranges = <0 0 0 0xffffffff>;
>  
> +		mdio0: mdio@88000 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			compatible = "qcom,ipq5018-mdio", "qcom,qca-mdio";
> +			reg = <0x88000 0x64>;
> +			resets = <&gcc GCC_GEPHY_MDC_SW_ARES>,
> +				 <&gcc GCC_GEPHY_DSP_HW_ARES>;

What do these two resets do? An MDIO bus controller is unlikely to
have a DSP in it. That is something a PHY is more likely to have. An
MDIO bus controller does have an MDC line, but why is it
GCC_GEPHY_MDC_SW_ARES not GCC_MDIO_MDC_SW_ARES? So this again makes me
think this is a PHY reset, so should be in the PHY node.

A device tree binding will help sort this out.


    Andrew

---
pw-bot: cr

Re: [PATCH 7/8] arm64: dts: qcom: ipq5018: enable ethernet support

[Ziyang Huang]

在 2024/1/22 0:45, Andrew Lunn 写道:
> On Sun, Jan 21, 2024 at 08:42:36PM +0800, Ziyang Huang wrote:
>> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5018.dtsi | 120 +++++++++++++++++++++++++-
>>   1 file changed, 116 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> index e502a3ecf4b7..b36e5c2136b7 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> @@ -94,6 +94,63 @@ soc: soc@0 {
>>   		#size-cells = <1>;
>>   		ranges = <0 0 0 0xffffffff>;
>>   
>> +		mdio0: mdio@88000 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			compatible = "qcom,ipq5018-mdio", "qcom,qca-mdio";
>> +			reg = <0x88000 0x64>;
>> +			resets = <&gcc GCC_GEPHY_MDC_SW_ARES>,
>> +				 <&gcc GCC_GEPHY_DSP_HW_ARES>;
> 
> What do these two resets do? An MDIO bus controller is unlikely to
> have a DSP in it. That is something a PHY is more likely to have. An
> MDIO bus controller does have an MDC line, but why is it
> GCC_GEPHY_MDC_SW_ARES not GCC_MDIO_MDC_SW_ARES? So this again makes me
> think this is a PHY reset, so should be in the PHY node.
> 

IPQ5018 has two mdio bus. mdio0 is an internal bus which only connects 
to internal PHY while mdio1 is outgoing and can be used to connect PHY 
or switch.

So I thought GCC_GEPHY_MDC_SW_ARES is for mdio0 and GCC_MDIO_MDC_SW_ARES 
is for mdio1.

GCC_GEPHY_DSP_HW_ARES is seem like PHY reset. But if we don't deassert 
it, the phy node can't be scaned. So I add it here like what reset-gpios 
do - to reset PHY or switch.

> A device tree binding will help sort this out.

Base on the functions I explained, I can't write the accurate names and 
number of resets here.

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

Re: [PATCH 7/8] arm64: dts: qcom: ipq5018: enable ethernet support

[Andrew Lunn]

On Mon, Jan 22, 2024 at 11:52:30PM +0800, Ziyang Huang wrote:
> 在 2024/1/22 0:45, Andrew Lunn 写道:
> > On Sun, Jan 21, 2024 at 08:42:36PM +0800, Ziyang Huang wrote:
> > > Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
> > > ---
> > >   arch/arm64/boot/dts/qcom/ipq5018.dtsi | 120 +++++++++++++++++++++++++-
> > >   1 file changed, 116 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> > > index e502a3ecf4b7..b36e5c2136b7 100644
> > > --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> > > @@ -94,6 +94,63 @@ soc: soc@0 {
> > >   		#size-cells = <1>;
> > >   		ranges = <0 0 0 0xffffffff>;
> > > +		mdio0: mdio@88000 {
> > > +			#address-cells = <1>;
> > > +			#size-cells = <0>;
> > > +			compatible = "qcom,ipq5018-mdio", "qcom,qca-mdio";
> > > +			reg = <0x88000 0x64>;
> > > +			resets = <&gcc GCC_GEPHY_MDC_SW_ARES>,
> > > +				 <&gcc GCC_GEPHY_DSP_HW_ARES>;
> > 
> > What do these two resets do? An MDIO bus controller is unlikely to
> > have a DSP in it. That is something a PHY is more likely to have. An
> > MDIO bus controller does have an MDC line, but why is it
> > GCC_GEPHY_MDC_SW_ARES not GCC_MDIO_MDC_SW_ARES? So this again makes me
> > think this is a PHY reset, so should be in the PHY node.
> > 
> 
> IPQ5018 has two mdio bus. mdio0 is an internal bus which only connects to
> internal PHY while mdio1 is outgoing and can be used to connect PHY or
> switch.
> 
> So I thought GCC_GEPHY_MDC_SW_ARES is for mdio0 and GCC_MDIO_MDC_SW_ARES is
> for mdio1.
> 
> GCC_GEPHY_DSP_HW_ARES is seem like PHY reset. But if we don't deassert it,
> the phy node can't be scaned. So I add it here like what reset-gpios do - to
> reset PHY or switch.
> 
> > A device tree binding will help sort this out.
> 
> Base on the functions I explained, I can't write the accurate names and
> number of resets here.

Without a device tree binding, your patch will not be accepted. At
minimum, you need to describe your understanding of the hardware, what
you think the resets do.

Since this is an internal PHY, you know exactly what it is. So you can
use ID values in the compatible so the driver loads. The driver can
then deal with clocks and resets. With complex setups like this,
scanning is not always possible.

     Andrew

[PATCH 8/8] arm64: dts: qcom: ipq5018-rdp432-c2: enable ethernet support

[Ziyang Huang]

Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
---
 .../arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
index e636a1cb9b77..074b78d7939c 100644
--- a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
+++ b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
@@ -15,6 +15,9 @@ / {
 
 	aliases {
 		serial0 = &blsp1_uart1;
+
+		ethernet0 = &gmac0;
+		ethernet1 = &gmac1;
 	};
 
 	chosen {
@@ -43,6 +46,22 @@ &sleep_clk {
 };
 
 &tlmm {
+	mdio1_pins: mdio1_pins {
+		mdc {
+			pins = "gpio36";
+			function = "mdc";
+			drive-strength = <8>;
+			bias-pull-up;
+		};
+
+		mdio {
+			pins = "gpio37";
+			function = "mdio";
+			drive-strength = <8>;
+			bias-pull-up;
+		};
+	};
+
 	sdc_default_state: sdc-default-state {
 		clk-pins {
 			pins = "gpio9";
@@ -70,3 +89,36 @@ data-pins {
 &xo_board_clk {
 	clock-frequency = <24000000>;
 };
+
+&mdio0 {
+	status = "ok";
+};
+
+&mdio1 {
+	pinctrl-0 = <&mdio1_pins>;
+	pinctrl-names = "default";
+	status = "ok";
+
+	reset-gpios = <&tlmm 39 GPIO_ACTIVE_LOW>;
+
+	qca8081: ethernet-phy@28 {
+		reg = <28>;
+	};
+};
+
+&uniphy0 {
+	mode = <QCOM_ETH_UNIPHY_MODE_SGMII>;
+	clkout-frequency = <QCOM_ETH_UNIPHY_CLKOUT_FREQ_50M>;
+	clkout-drive-strength = <QCOM_ETH_UNIPHY_CLKOUT_DS_1_5V>;
+	status = "ok";
+};
+
+&gmac0 {
+	status = "ok";
+};
+
+&gmac1 {
+	phy-handle = <&qca8081>;
+	phy-mode = "sgmii";
+	status = "ok";
+};
-- 
2.40.1

Re: [PATCH 8/8] arm64: dts: qcom: ipq5018-rdp432-c2: enable ethernet support

[Krzysztof Kozlowski]

On 21/01/2024 13:42, Ziyang Huang wrote:
> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>

Missing commit msg.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts_getmaintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, use mainline), work on fork of kernel (don't, use
mainline) or you ignore some maintainers (really don't). Just use b4 and
all the problems go away.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.


> ---
>  .../arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> index e636a1cb9b77..074b78d7939c 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> +++ b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> @@ -15,6 +15,9 @@ / {
>  
>  	aliases {
>  		serial0 = &blsp1_uart1;
> +

Drop.

> +		ethernet0 = &gmac0;
> +		ethernet1 = &gmac1;

Keep alphabetical order.

>  	};
>  
>  	chosen {
> @@ -43,6 +46,22 @@ &sleep_clk {
>  };
>  
>  &tlmm {
> +	mdio1_pins: mdio1_pins {

This wasn't ever tested. NAK.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).


Best regards,
Krzysztof

Re: [PATCH 8/8] arm64: dts: qcom: ipq5018-rdp432-c2: enable ethernet support

[kernel test robot]

Hi Ziyang,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on clk/clk-next pza/reset/next]
[cannot apply to linus/master pza/imx-drm/next v6.8-rc1 next-20240123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ziyang-Huang/net-phy-Introduce-Qualcomm-IPQ5018-internal-PHY-driver/20240121-204840
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/TYZPR01MB5556D3E73D7F7242F810F915C9762%40TYZPR01MB5556.apcprd01.prod.exchangelabs.com
patch subject: [PATCH 8/8] arm64: dts: qcom: ipq5018-rdp432-c2: enable ethernet support
config: arm64-randconfig-001-20240124 (https://download.01.org/0day-ci/archive/20240124/202401240841.gfbyp3Lz-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240124/202401240841.gfbyp3Lz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401240841.gfbyp3Lz-lkp@intel.com/

All errors (new ones prefixed by >>):

>> Error: arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts:102.26-27 syntax error
   FATAL ERROR: Unable to parse input tree

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 0/8] ipq5018: enable ethernet support

[Andrew Lunn]

On Sun, Jan 21, 2024 at 08:40:39PM +0800, Ziyang Huang wrote:
> Enable ethernet support for IPQ5018.
> 
> Praveenkumar I (1):
>   clk: qcom: support for duplicate freq in RCG2 freq table
> 
> Ziyang Huang (7):
>   net: phy: Introduce Qualcomm IPQ5018 internal PHY driver
>   phy: Introduce Qualcomm ethernet uniphy driver
>   net: stmmac: Introduce Qualcomm IPQ50xx DWMAC driver
>   clk: qcom: gcc-ipq5018: correct gcc_gmac0_sys_clk reg
>   net: mdio: ipq4019: support reset control
>   arm64: dts: qcom: ipq5018: enable ethernet support
>   arm64: dts: qcom: ipq5018-rdp432-c2: enable ethernet support

Hi Ziyang

You failed to Cc: a number of Maintainers here. e.g for the PHY and
MDIO patches i would of expected the PHY and maintainers to be Cc:ed.

     Andrew

Re: [PATCH 0/8] ipq5018: enable ethernet support

[Ziyang Huang]

在 2024/1/21 23:51, Andrew Lunn 写道:
> On Sun, Jan 21, 2024 at 08:40:39PM +0800, Ziyang Huang wrote:
>> Enable ethernet support for IPQ5018.
>>
>> Praveenkumar I (1):
>>    clk: qcom: support for duplicate freq in RCG2 freq table
>>
>> Ziyang Huang (7):
>>    net: phy: Introduce Qualcomm IPQ5018 internal PHY driver
>>    phy: Introduce Qualcomm ethernet uniphy driver
>>    net: stmmac: Introduce Qualcomm IPQ50xx DWMAC driver
>>    clk: qcom: gcc-ipq5018: correct gcc_gmac0_sys_clk reg
>>    net: mdio: ipq4019: support reset control
>>    arm64: dts: qcom: ipq5018: enable ethernet support
>>    arm64: dts: qcom: ipq5018-rdp432-c2: enable ethernet support
> 
> Hi Ziyang
> 
> You failed to Cc: a number of Maintainers here. e.g for the PHY and
> MDIO patches i would of expected the PHY and maintainers to be Cc:ed.
> 
>       Andrew

Hi Andrew,

Sorrr for my mistake. For easily comparison, I added these patches to 
git index, which mess up ./scripts/checkpatch.pl and 
./scripts/get_maintainer.pl and led to wrong results. Will add correct 
Cc: in next patches.