> -----Original Message-----
> From: Niklas Cassel <cassel@xxxxxxxxxx>
> Sent: Monday, January 22, 2024 6:33 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx>
> Cc: dlemoal@xxxxxxxxxx; richardcochran@xxxxxxxxx;
> piyush.mehta@xxxxxxxxxx; axboe@xxxxxxxxx; Simek, Michal
> <michal.simek@xxxxxxx>; linux-ide@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>; Mehta, Piyush
> <piyush.mehta@xxxxxxx>
> Subject: Re: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY
> support
>
> Hello Radhey,
>
> On Fri, Jan 19, 2024 at 12:38:23AM +0530, Radhey Shyam Pandey wrote:
> > From: Piyush Mehta <piyush.mehta@xxxxxxx>
> >
> > Platform clock and phy error resources are not cleaned up in Xilinx GT
> > PHY error path. To fix introduce error label for
> > ahci_platform_disable_clks and phy_power_off/exit and call them in error
> path. No functional change.
> >
> > Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support
> > xilinx GT phy")
> > Signed-off-by: Piyush Mehta <piyush.mehta@xxxxxxx>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xxxxxxx>
> > ---
> > ---
> > drivers/ata/ahci_ceva.c | 47
> > +++++++++++++++++++++++++++++------------
> > 1 file changed, 33 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c index
> > 64f7f7d6ba84..bfc513f1d0b3 100644
> > --- a/drivers/ata/ahci_ceva.c
> > +++ b/drivers/ata/ahci_ceva.c
> > @@ -196,7 +196,7 @@ static int ceva_ahci_probe(struct platform_device
> *pdev)
> > struct ahci_host_priv *hpriv;
> > struct ceva_ahci_priv *cevapriv;
> > enum dev_dma_attr attr;
> > - int rc;
> > + int rc, i;
> >
> > cevapriv = devm_kzalloc(dev, sizeof(*cevapriv), GFP_KERNEL);
> > if (!cevapriv)
> > @@ -219,8 +219,6 @@ static int ceva_ahci_probe(struct platform_device
> *pdev)
> > if (rc)
> > return rc;
> > } else {
> > - int i;
> > -
> > rc = ahci_platform_enable_clks(hpriv);
> > if (rc)
> > return rc;
> > @@ -229,8 +227,11 @@ static int ceva_ahci_probe(struct platform_device
> > *pdev)
> >
> > for (i = 0; i < hpriv->nports; i++) {
> > rc = phy_init(hpriv->phys[i]);
> > - if (rc)
> > - return rc;
> > + if (rc) {
> > + while (--i >= 0)
> > + phy_exit(hpriv->phys[i]);
>
> It is ugly to have a loop both here and at the end of the function.
> Why don't you just goto disable_phys here?
>
> Just like how it is done in ahci_platform_enable_phys():
> https://github.com/torvalds/linux/blob/v6.7/drivers/ata/libahci_platform.c#
> L52-L54
>
>
> > + goto disable_clks;
> > + }
> > }
> >
> > /* De-assert the controller reset */ @@ -240,7 +241,7 @@
> static int
> > ceva_ahci_probe(struct platform_device *pdev)
> > rc = phy_power_on(hpriv->phys[i]);
> > if (rc) {
> > phy_exit(hpriv->phys[i]);
> > - return rc;
> > + goto disable_phys;
> > }
> > }
> > }
> > @@ -252,52 +253,60 @@ static int ceva_ahci_probe(struct
> platform_device *pdev)
> > if (of_property_read_u8_array(np, "ceva,p0-cominit-params",
> > (u8 *)&cevapriv->pp2c[0], 4) < 0) {
> > dev_warn(dev, "ceva,p0-cominit-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > if (of_property_read_u8_array(np, "ceva,p1-cominit-params",
> > (u8 *)&cevapriv->pp2c[1], 4) < 0) {
> > dev_warn(dev, "ceva,p1-cominit-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > /* Read OOB timing value for COMWAKE from device-tree*/
> > if (of_property_read_u8_array(np, "ceva,p0-comwake-params",
> > (u8 *)&cevapriv->pp3c[0], 4) < 0) {
> > dev_warn(dev, "ceva,p0-comwake-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > if (of_property_read_u8_array(np, "ceva,p1-comwake-params",
> > (u8 *)&cevapriv->pp3c[1], 4) < 0) {
> > dev_warn(dev, "ceva,p1-comwake-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > /* Read phy BURST timing value from device-tree */
> > if (of_property_read_u8_array(np, "ceva,p0-burst-params",
> > (u8 *)&cevapriv->pp4c[0], 4) < 0) {
> > dev_warn(dev, "ceva,p0-burst-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > if (of_property_read_u8_array(np, "ceva,p1-burst-params",
> > (u8 *)&cevapriv->pp4c[1], 4) < 0) {
> > dev_warn(dev, "ceva,p1-burst-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > /* Read phy RETRY interval timing value from device-tree */
> > if (of_property_read_u16_array(np, "ceva,p0-retry-params",
> > (u16 *)&cevapriv->pp5c[0], 2) < 0) {
> > dev_warn(dev, "ceva,p0-retry-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > if (of_property_read_u16_array(np, "ceva,p1-retry-params",
> > (u16 *)&cevapriv->pp5c[1], 2) < 0) {
> > dev_warn(dev, "ceva,p1-retry-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > /*
> > @@ -321,6 +330,16 @@ static int ceva_ahci_probe(struct platform_device
> > *pdev)
> >
> > disable_resources:
> > ahci_platform_disable_resources(hpriv);
>
> Looking at ahci_platform_disable_resources(),
> it calls ahci_platform_disable_phys(), which calls
> phy_power_off() and phy_exit().
>
> This means that if you jump to disable_resources, you will call
> phy_power_off() and phy_exit() twice.
> Looking at the phy code, it does not handle these functions being called
> twice.
>
>
> You already call ahci_platform_get_resources(), so why don't you just set
> AHCI_PLATFORM_GET_RESETS, that way you should be able to remove a
> bunch of duplicated code from this driver.
>
> One major difference seems to be that ahci_platform_enable_resources()
> does not assert reset before deasserting it.
> Can't we just add a reset_control_assert() + some small usleep (e.g.
> usleep_range(1000, 1500)) before the reset_control_deassert()?
> Have you tried doing that?
As I understand AHCI-platform library supports shared reset control
lines and AHCI SATA platform driver request exclusive reset lines.
So we need to move reset request out of ahci platform library and
make it driver specific as done in commit "9a9d3abe24bb
("ata: ahci: ceva: Update the driver to support xilinx GT phy")
Furthermore SATA IP programming mentioned in
Zynq UltraScale+ Device TRM mentions -
Assert SATA reset.
Set PS-GTR configuration (Part of phy_init)
Bring SATA by de-asserting the reset.
Now looking at ahci_platform_enable_resources(), it
enables all ahci_platform managed resources in the
following order
1) Regulator
2) Clocks (through ahci_platform_enable_clks)
3) Resets
4) Phys
Here reset is de-asserted before phy_init() which
is different from SATA Controller Programming Flow.
I assume because of above programming sequence
differences we earlier didn't use generic
ahci_platform_enable_resources() and instead used
a custom implementation derived from it in ceva
AHCI SATA platform driver.
Coming to reusing ahci_platform_enable_resources(),
if we have to do we have skip step-3. and let ceva
ahci driver request reset and then call
reset_control_assert()/deassert.
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.
Thanks,
Radhey
