Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver
From: Ziyang Huang
Date: Tue Jan 23 2024 - 10:47:00 EST
在 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@xxxxxxxxxxx>
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