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@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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
