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

From: Ziyang Huang <hzyitc@outlook.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com,
	richardcochran@gmail.com, p.zabel@pengutronix.de,
	matthias.bgg@gmail.com, angelogioacchino.delregno@collabora.com,
	linux-kernel@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver
Date: Tue, 23 Jan 2024 23:38:01 +0800	[thread overview]
Message-ID: <TYZPR01MB55567CE79D7F08C738A81683C9742@TYZPR01MB5556.apcprd01.prod.exchangelabs.com> (raw)
In-Reply-To: <e1fd863a-6725-4180-8ad3-faeb44c09238@lunn.ch>

在 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