Re: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY support

[Niklas Cassel <cassel@kernel.org>]

Hello Radhey,

On Wed, Feb 07, 2024 at 06:28:25PM +0000, Pandey, Radhey Shyam wrote:

(snip)

> However for single controller IP programming sequence
> deviation I am less inclined to make modification in
> generic enable_resources() API . But if you strongly
> feel we should go ahead and make changes to the generic
> enable_resources API, please suggest your thoughts
> and then we can get started.

My first suggestion was to try to add a reset_control_assert() + msleep(1)
before the reset_control_deassert() in ahci_platform_enable_resources().
This should be safe for all platforms as in order to trigger a reset,
you actually need to pulse it. It is possible that your platform has
reset deasserted from boot, and in that case, since the generic version
currently only does a deassert, it is possible that no reset was done
at all on your platform.

If my first suggestion did not work, then my second suggestion was to
add a new flag which your platform driver sets
(e.g. HOLD_RESET_DURING_PHY_INIT), and implement that in a new function
in libahci_platform.c. The generic version could check for this flag at
the start of the function, and return the result from your new function
in libahci_platform.c instead.


However, since you have a TRM, let's ignore these two proposals and instead
re-spin your existing series, addressing the other outstanding issues:
1) The new kernel test robot build warnings introduced by your patch.

- The other review comments that are still applicable:
2) Your patch format is strange... There is one too many "---" line here.
3) Use goto disable_phys so you don't need to loop twice.
4) If you jump to disable_resources, you will call phy_power_off()
  and phy_exit() twice, which the PHY lib does not handle.
5) Please be a bit more specific in your commit message.
6) Describe your changes in imperative mood.
7) A new comment that I see when looking at your driver just now,
you should simply remove:

"""
	if (!cevapriv->rst) {
		rc = ahci_platform_enable_resources(hpriv);
		if (rc)
			return rc;
	} else {
"""

Either you want to use ahci_platform_enable_resources(),
or your own way, but you don't get to do both.

You call devm_reset_control_get_optional_exclusive(), which will
return NULL for platforms that do not supply a reset.

Both reset_control_assert() and reset_control_deassert() are no-ops
if you send in a NULL pointer. The are designed this way to work
with devm_reset_control_get_optional_exclusive() which will return
NULL for platforms that do not provide a reset, such that drivers,
such as yours, should not need
if () {
} else {
depending on if get_reset() returned non-NULL or not.

Just always call reset_control_assert()/reset_control_deassert(),
even for the !cevapriv->rst case, as they will be no-ops anyway.


Kind regards,
Niklas