Re: [PATCH 09/21] ata: libahci_platform: Introduce reset assertion/deassertion methods

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 3/24/22 09:16, Serge Semin wrote:
> Currently the ACHI-platform library supports only the assert and deassert
> reset signals and ignores the platforms with self-deasserting reset lines.
> That prone to having the platforms with self-deasserting reset method
> misbehaviour when it comes to resuming from sleep state after the clocks
> have been fully disabled. For such cases the controller needs to be fully
> reset all over after the reference clocks are enabled and stable,
> otherwise the controller state machine might be in an undetermined state.
> 
> The best solution would be to auto-detect which reset method is supported
> by the particular platform and use it implicitly in the framework of the
> ahci_platform_enable_resources()/ahci_platform_disable_resources()
> methods. Alas it can't be implemented due to the AHCI-platform library
> already supporting the shared reset control lines. As [1] says in such
> case we have to use only one of the next methods:
> + reset_control_assert()/reset_control_deassert();
> + reset_control_reset()/reset_control_rearm().
> If the driver had an exclusive control over the reset lines we could have
> been able to manipulate the lines with no much limitation and just used
> the combination of the methods above to cover all the possible
> reset-control cases. Since the shared reset control has already been
> advertised and couldn't be changed with no risk to breaking the platforms
> relying on it, we have no choice but to make the platform drivers to
> determine which reset methods the platform reset system supports.
> 
> In order to implement both types of reset control support we suggest to
> introduce the new AHCI-platform flag: AHCI_PLATFORM_RST_TRIGGER, which
> when passed to the ahci_platform_get_resources() method together with the
> AHCI_PLATFORM_GET_RESETS flag will indicate that the reset lines are
> self-deasserting thus the reset_control_reset()/reset_control_rearm() will
> be used to control the reset state. Otherwise the
> reset_control_deassert()/reset_control_assert() methods will be utilized.
> 
> [1] Documentation/driver-api/reset.rst
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  drivers/ata/ahci.h             |  1 +
>  drivers/ata/libahci_platform.c | 47 ++++++++++++++++++++++++++++++----
>  include/linux/ahci_platform.h  |  5 +++-
>  3 files changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 1564c691094a..0b1d5c24cb8c 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -342,6 +342,7 @@ struct ahci_host_priv {
>  	bool			got_runtime_pm; /* Did we do pm_runtime_get? */
>  	unsigned int		n_clks;
>  	struct clk_bulk_data	*clks;		/* Optional */
> +	unsigned int		f_rsts;

Why ? using flags directly is not OK ?

>  	struct reset_control	*rsts;		/* Optional */
>  	struct regulator	**target_pwrs;	/* Optional */
>  	struct regulator	*ahci_regulator;/* Optional */
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 5998e735a813..febad33aa43c 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -150,6 +150,41 @@ void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
>  }
>  EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
>  
> +/**
> + * ahci_platform_deassert_rsts - Deassert/trigger platform resets
> + * @hpriv: host private area to store config values
> + *
> + * This function desserts or triggers all the reset lanes found for the AHCI

s/desserts/deasserts ?
s/lanes/lines ?

> + * device.
> + *
> + * RETURNS:
> + * 0 on success otherwise a negative error code
> + */
> +int ahci_platform_deassert_rsts(struct ahci_host_priv *hpriv)
> +{
> +	if (hpriv->f_rsts & AHCI_PLATFORM_RST_TRIGGER)
> +		return reset_control_reset(hpriv->rsts);
> +
> +	return reset_control_deassert(hpriv->rsts);
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_deassert_rsts);
> +
> +/**
> + * ahci_platform_assert_rsts - Assert/rearm platform resets
> + * @hpriv: host private area to store config values
> + *
> + * This function asserts or rearms (for self-deasserting resets) all the reset
> + * controls found for the AHCI device.
> + */
> +void ahci_platform_assert_rsts(struct ahci_host_priv *hpriv)
> +{
> +	if (hpriv->f_rsts & AHCI_PLATFORM_RST_TRIGGER)
> +		return (void)reset_control_rearm(hpriv->rsts);

return void in a void function ? How does this even compile ?
And what if reset_control_rearm() fails ? What happens ?

> +
> +	reset_control_assert(hpriv->rsts);
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_assert_rsts);
> +
>  /**
>   * ahci_platform_enable_regulators - Enable regulators
>   * @hpriv: host private area to store config values
> @@ -247,18 +282,18 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
>  	if (rc)
>  		goto disable_regulator;
>  
> -	rc = reset_control_deassert(hpriv->rsts);
> +	rc = ahci_platform_deassert_rsts(hpriv);
>  	if (rc)
>  		goto disable_clks;
>  
>  	rc = ahci_platform_enable_phys(hpriv);
>  	if (rc)
> -		goto disable_resets;
> +		goto disable_rsts;
>  
>  	return 0;
>  
> -disable_resets:
> -	reset_control_assert(hpriv->rsts);
> +disable_rsts:
> +	ahci_platform_assert_rsts(hpriv);
>  
>  disable_clks:
>  	ahci_platform_disable_clks(hpriv);
> @@ -285,7 +320,7 @@ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
>  {
>  	ahci_platform_disable_phys(hpriv);
>  
> -	reset_control_assert(hpriv->rsts);
> +	ahci_platform_assert_rsts(hpriv);
>  
>  	ahci_platform_disable_clks(hpriv);
>  
> @@ -468,6 +503,8 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>  			rc = PTR_ERR(hpriv->rsts);
>  			goto err_out;
>  		}
> +
> +		hpriv->f_rsts = flags & AHCI_PLATFORM_RST_TRIGGER;

Why not use hpriv->flags ?

>  	}
>  
>  	/*
> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> index fd964e6a68d6..57d25d30a9fa 100644
> --- a/include/linux/ahci_platform.h
> +++ b/include/linux/ahci_platform.h
> @@ -26,6 +26,8 @@ struct clk *
>  ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id);
>  int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
>  void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
> +int ahci_platform_deassert_rsts(struct ahci_host_priv *hpriv);
> +void ahci_platform_assert_rsts(struct ahci_host_priv *hpriv);
>  int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
>  void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv);
>  int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
> @@ -44,6 +46,7 @@ int ahci_platform_resume_host(struct device *dev);
>  int ahci_platform_suspend(struct device *dev);
>  int ahci_platform_resume(struct device *dev);
>  
> -#define AHCI_PLATFORM_GET_RESETS	0x01
> +#define AHCI_PLATFORM_GET_RESETS	BIT(0)
> +#define AHCI_PLATFORM_RST_TRIGGER	BIT(1)
>  
>  #endif /* _AHCI_PLATFORM_H */


-- 
Damien Le Moal
Western Digital Research