Re: [PATCH 3/4] pata: imx: add support of setting timings for PIO modes

[Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>]

Hi,

On Wednesday, November 09, 2016 02:56:37 AM Vladimir Zapolskiy wrote:
> The controller is capable to operate in up to PIO4 mode, however
> before the change the driver relies on timing settings done by
> a bootloader for PIO0 mode only. The change adds more flexibility
> in PIO mode selection at runtime and makes the driver to work even if
> bootloader does not preset ATA timings.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  drivers/ata/pata_imx.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)

[...]

> +static void pata_imx_set_timing(struct ata_device *adev,
> +				struct pata_imx_priv *priv)
> +{
> +	struct ata_timing timing;
> +	unsigned long clkrate;
> +	u32 T, mode;
> +
> +	clkrate = clk_get_rate(priv->clk);
> +
> +	if (adev->pio_mode < XFER_PIO_0 || adev->pio_mode > XFER_PIO_4 ||
> +	    !clkrate)

No need check for adev->pio_mode < XFER_PIO_0 || adev->pio_mode > XFER_PIO_4
as the libata core code guarantees that these conditions will never happen.

Also you should at least print an error on !clkrate condition. [ IMHO it
is actually better to BUG_ON() on this condition as the further operations
may be risky for the data integrity (wrong PIO timings may be used). ]

> +		return;
> +
> +	T = 1000000000 / clkrate;
> +	ata_timing_compute(adev, adev->pio_mode, &timing, T * 1000, 0);
> +
> +	mode = adev->pio_mode - XFER_PIO_0;
> +
> +	writeb(3, priv->host_regs + PATA_IMX_ATA_TIME_OFF);
> +	writeb(3, priv->host_regs + PATA_IMX_ATA_TIME_ON);
> +	writeb(timing.setup, priv->host_regs + PATA_IMX_ATA_TIME_1);
> +	writeb(timing.act8b, priv->host_regs + PATA_IMX_ATA_TIME_2W);
> +	writeb(timing.act8b, priv->host_regs + PATA_IMX_ATA_TIME_2R);
> +	writeb(1, priv->host_regs + PATA_IMX_ATA_TIME_PIO_RDX);
> +
> +	writeb(pio_t4[mode] / T + 1, priv->host_regs + PATA_IMX_ATA_TIME_4);
> +	writeb(pio_t9[mode] / T + 1, priv->host_regs + PATA_IMX_ATA_TIME_9);
> +	writeb(pio_tA[mode] / T + 1, priv->host_regs + PATA_IMX_ATA_TIME_AX);
> +}

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics