Re: [PATCH V7 5/7] ata: ahci_tegra: disable devslp for t124

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3350 bytes --]

On Mon, Feb 12, 2018 at 10:56:44PM +0530, Preetham Chandru Ramchandra wrote:
> From: Preetham Ramchandra <pchandru@nvidia.com>
> 
> t124 does not support devslp and it should be
> disabled.

Please spell out t124 as Tegra124 in the subject and the commit message.

> 
> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
> ---
>  drivers/ata/ahci_tegra.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> index 6aaf8a4571c8..62f2afb54789 100644
> --- a/drivers/ata/ahci_tegra.c
> +++ b/drivers/ata/ahci_tegra.c
> @@ -145,6 +145,10 @@
>  #define FUSE_SATA_CALIB					0x124
>  #define FUSE_SATA_CALIB_MASK				0x3
>  
> +enum {
> +	NO_DEVSLP	= (1 << 0),
> +};
> +
>  struct sata_pad_calibration {
>  	u8 gen1_tx_amp;
>  	u8 gen1_tx_peak;
> @@ -166,12 +170,14 @@ struct tegra_ahci_ops {
>  struct tegra_ahci_soc {
>  	const char *const	*supply_names;
>  	u32			num_supplies;
> +	u32			quirks;
>  	struct tegra_ahci_ops	ops;
>  };

I'd prefer these to be simple booleans, which make the code easier to
read, in my opinion. This could be:

	bool supports_devslp;

>  
>  struct tegra_ahci_priv {
>  	struct platform_device	   *pdev;
>  	void __iomem		   *sata_regs;
> +	void __iomem		   *sata_aux_regs;
>  	struct reset_control	   *sata_rst;
>  	struct reset_control	   *sata_oob_rst;
>  	struct reset_control	   *sata_cold_rst;
> @@ -181,6 +187,18 @@ struct tegra_ahci_priv {
>  	struct tegra_ahci_soc	   *soc_data;
>  };
>  
> +static void tegra_ahci_handle_quirks(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	u32 val;
> +
> +	if (tegra->sata_aux_regs && (tegra->soc_data->quirks & NO_DEVSLP)) {

And then this becomes:

	if (tegra->sata_aux_regs && !tegra->soc->supports_devslp)

> +		val = readl(tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0);
> +		val &= ~SATA_AUX_MISC_CNTL_1_0_SDS_SUPPORT;
> +		writel(val, tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0);
> +	}
> +}
> +
>  static int tegra124_ahci_init(struct ahci_host_priv *hpriv)
>  {
>  	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> @@ -400,6 +418,7 @@ static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
>  	val &= ~SATA_CONFIGURATION_0_CLK_OVERRIDE;
>  	writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
>  
> +	tegra_ahci_handle_quirks(hpriv);
>  
>  	/* Unmask SATA interrupts */
>  
> @@ -441,6 +460,7 @@ static const char *const tegra124_supply_names[] = {
>  static const struct tegra_ahci_soc tegra124_ahci_soc_data = {
>  	.supply_names = tegra124_supply_names,
>  	.num_supplies = ARRAY_SIZE(tegra124_supply_names),
> +	.quirks = NO_DEVSLP,
>  	.ops = {
>  		.init = tegra124_ahci_init,
>  	},
> @@ -485,6 +505,15 @@ static int tegra_ahci_probe(struct platform_device *pdev)
>  	tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(tegra->sata_regs))
>  		return PTR_ERR(tegra->sata_regs);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	/*
> +	 * Aux register is optional.
> +	 */

"Aux register" -> "AUX registers". Also, I think it's more idiomatic to
place the comment above platform_get_resource() so that the assignment
and the check below are not separated.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]