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

[Andrew Lunn <andrew@lunn.ch>]

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