Messages in this thread |  | | Date | Tue, 23 Jan 2024 23:38:01 +0800 | | Subject | Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver | | From | 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
|  |