[PATCH 0/4] pata: imx: set timings for PIO modes up to PIO4

[PATCH 0/4] pata: imx: set timings for PIO modes up to PIO4

[Vladimir Zapolskiy]

The changeset adds support of PIO modes up to PIO4 by setting
necessary timings in the driver, before the change it is assumed
that the timings are always set by a bootloader once and thus
only one possible PIO mode has been supported (PIO0). With
this change the driver can be used on boards without ATA controller
configuration done by a bootloader.

The change is tested on a legacy i.MX31 board with an HDD connected
by a 40-pin flat cable.

Vladimir Zapolskiy (4):
  pata: imx: sort headers out
  pata: imx: set controller PIO mode with .set_piomode callback
  pata: imx: add support of setting timings for PIO modes
  pata: imx: support controller modes up to PIO4

 drivers/ata/pata_imx.c | 82 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 57 insertions(+), 25 deletions(-)

-- 
2.10.2

[PATCH 1/4] pata: imx: sort headers out

[Vladimir Zapolskiy]

Put headers in alphabetic order and remove redundant ones.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/ata/pata_imx.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/pata_imx.c b/drivers/ata/pata_imx.c
index 139d207..203e309 100644
--- a/drivers/ata/pata_imx.c
+++ b/drivers/ata/pata_imx.c
@@ -13,14 +13,12 @@
  * - dmaengine support
  * - check if timing stuff needed
  */
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/blkdev.h>
-#include <scsi/scsi_host.h>
+
 #include <linux/ata.h>
+#include <linux/clk.h>
 #include <linux/libata.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/clk.h>
 
 #define DRV_NAME "pata_imx"
 
-- 
2.10.2

[PATCH 2/4] pata: imx: set controller PIO mode with .set_piomode callback

[Vladimir Zapolskiy]

Convert .set_mode callback function to more specific .set_piomode,
the driver does not have support of DMA modes, thus a simpler version
of the callback is preferred.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/ata/pata_imx.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/ata/pata_imx.c b/drivers/ata/pata_imx.c
index 203e309..00df18b 100644
--- a/drivers/ata/pata_imx.c
+++ b/drivers/ata/pata_imx.c
@@ -38,28 +38,17 @@ struct pata_imx_priv {
 	u32 ata_ctl;
 };
 
-static int pata_imx_set_mode(struct ata_link *link, struct ata_device **unused)
+static void pata_imx_set_piomode(struct ata_port *ap, struct ata_device *adev)
 {
-	struct ata_device *dev;
-	struct ata_port *ap = link->ap;
 	struct pata_imx_priv *priv = ap->host->private_data;
 	u32 val;
 
-	ata_for_each_dev(dev, link, ENABLED) {
-		dev->pio_mode = dev->xfer_mode = XFER_PIO_0;
-		dev->xfer_shift = ATA_SHIFT_PIO;
-		dev->flags |= ATA_DFLAG_PIO;
-
-		val = __raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
-		if (ata_pio_need_iordy(dev))
-			val |= PATA_IMX_ATA_CTRL_IORDY_EN;
-		else
-			val &= ~PATA_IMX_ATA_CTRL_IORDY_EN;
-		__raw_writel(val, priv->host_regs + PATA_IMX_ATA_CONTROL);
-
-		ata_dev_info(dev, "configured for PIO\n");
-	}
-	return 0;
+	val = __raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
+	if (ata_pio_need_iordy(adev))
+		val |= PATA_IMX_ATA_CTRL_IORDY_EN;
+	else
+		val &= ~PATA_IMX_ATA_CTRL_IORDY_EN;
+	__raw_writel(val, priv->host_regs + PATA_IMX_ATA_CONTROL);
 }
 
 static struct scsi_host_template pata_imx_sht = {
@@ -70,7 +59,7 @@ static struct ata_port_operations pata_imx_port_ops = {
 	.inherits		= &ata_sff_port_ops,
 	.sff_data_xfer		= ata_sff_data_xfer_noirq,
 	.cable_detect		= ata_cable_unknown,
-	.set_mode		= pata_imx_set_mode,
+	.set_piomode		= pata_imx_set_piomode,
 };
 
 static void pata_imx_setup_port(struct ata_ioports *ioaddr)
-- 
2.10.2

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

[Vladimir Zapolskiy]

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(-)

diff --git a/drivers/ata/pata_imx.c b/drivers/ata/pata_imx.c
index 00df18b..8f13c9f 100644
--- a/drivers/ata/pata_imx.c
+++ b/drivers/ata/pata_imx.c
@@ -11,7 +11,6 @@
  *
  * TODO:
  * - dmaengine support
- * - check if timing stuff needed
  */
 
 #include <linux/ata.h>
@@ -22,6 +21,16 @@
 
 #define DRV_NAME "pata_imx"
 
+#define PATA_IMX_ATA_TIME_OFF		0x00
+#define PATA_IMX_ATA_TIME_ON		0x01
+#define PATA_IMX_ATA_TIME_1		0x02
+#define PATA_IMX_ATA_TIME_2W		0x03
+#define PATA_IMX_ATA_TIME_2R		0x04
+#define PATA_IMX_ATA_TIME_AX		0x05
+#define PATA_IMX_ATA_TIME_PIO_RDX	0x06
+#define PATA_IMX_ATA_TIME_4		0x07
+#define PATA_IMX_ATA_TIME_9		0x08
+
 #define PATA_IMX_ATA_CONTROL		0x24
 #define PATA_IMX_ATA_CTRL_FIFO_RST_B	(1<<7)
 #define PATA_IMX_ATA_CTRL_ATA_RST_B	(1<<6)
@@ -31,6 +40,10 @@
 #define PATA_IMX_DRIVE_DATA		0xA0
 #define PATA_IMX_DRIVE_CONTROL		0xD8
 
+static u32 pio_t4[] = { 30,  20,  15,  10,  10 };
+static u32 pio_t9[] = { 20,  15,  10,  10,  10 };
+static u32 pio_tA[] = { 35,  35,  35,  35,  35 };
+
 struct pata_imx_priv {
 	struct clk *clk;
 	/* timings/interrupt/control regs */
@@ -38,11 +51,43 @@ struct pata_imx_priv {
 	u32 ata_ctl;
 };
 
+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)
+		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);
+}
+
 static void pata_imx_set_piomode(struct ata_port *ap, struct ata_device *adev)
 {
 	struct pata_imx_priv *priv = ap->host->private_data;
 	u32 val;
 
+	pata_imx_set_timing(adev, priv);
+
 	val = __raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
 	if (ata_pio_need_iordy(adev))
 		val |= PATA_IMX_ATA_CTRL_IORDY_EN;
-- 
2.10.2

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

[Sergei Shtylyov]

Hello.

On 11/9/2016 3:56 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
                                                         ^^ not needed

> 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(-)
>
> diff --git a/drivers/ata/pata_imx.c b/drivers/ata/pata_imx.c
> index 00df18b..8f13c9f 100644
> --- a/drivers/ata/pata_imx.c
> +++ b/drivers/ata/pata_imx.c
[...]
> @@ -31,6 +40,10 @@
>  #define PATA_IMX_DRIVE_DATA		0xA0
>  #define PATA_IMX_DRIVE_CONTROL		0xD8
>
> +static u32 pio_t4[] = { 30,  20,  15,  10,  10 };
> +static u32 pio_t9[] = { 20,  15,  10,  10,  10 };
> +static u32 pio_tA[] = { 35,  35,  35,  35,  35 };

    Perhaps it makes sense to extend the 'struct ata_timing'...

[...]
> @@ -38,11 +51,43 @@ struct pata_imx_priv {
>  	u32 ata_ctl;
>  };
>
> +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)
> +		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);

    What do those registers mean?

> +	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);

    And this one?

> +
> +	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);

    DIV_ROUND_UP(x, T)?

[...]

MBR, Sergei

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

[Vladimir Zapolskiy]

Hi Sergei,

thank you for review, I see that Tejun has applied the changes,
anyway I'll answer your questions.

On 11/09/2016 11:39 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 11/9/2016 3:56 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
>                                                         ^^ not needed
>
>> 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(-)
>>
>> diff --git a/drivers/ata/pata_imx.c b/drivers/ata/pata_imx.c
>> index 00df18b..8f13c9f 100644
>> --- a/drivers/ata/pata_imx.c
>> +++ b/drivers/ata/pata_imx.c
> [...]
>> @@ -31,6 +40,10 @@
>>  #define PATA_IMX_DRIVE_DATA        0xA0
>>  #define PATA_IMX_DRIVE_CONTROL        0xD8
>>
>> +static u32 pio_t4[] = { 30,  20,  15,  10,  10 };
>> +static u32 pio_t9[] = { 20,  15,  10,  10,  10 };
>> +static u32 pio_tA[] = { 35,  35,  35,  35,  35 };
>
>    Perhaps it makes sense to extend the 'struct ata_timing'...
>
> [...]

As you guess the numbers are taken right from the ATAPI spec,
however I haven't found the second ATA controller driver sumbitted
upstream, which reuses these timings, so probably generalization
is not needed here. Anyway I would prefer if maintainers do it,
if they think that it makes sense.

>> @@ -38,11 +51,43 @@ struct pata_imx_priv {
>>      u32 ata_ctl;
>>  };
>>
>> +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)
>> +        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);
>
>    What do those registers mean?

You may find a better description from i.MX27 or i.MX31 Reference Manual
than my retelling, the docs are open.

toff/ton timings are used to avoid bus contention when switching
BUFFER_EN signal and data writing period. AFAIK these timings are
specific to the controller only.

>> +    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);
>
>    And this one?

This is trd timing from the ATA/ATAPI spec, "Read Data Valid to IORDY 
active", its minimal value is defined as 0.

>> +
>> +    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);
>
>    DIV_ROUND_UP(x, T)?

Yes, it is reasonable.

--
With best wishes,
Vladimir

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

[Tejun Heo]

Hello, Vladimir, Sergei.

On Thu, Nov 10, 2016 at 03:33:22AM +0200, Vladimir Zapolskiy wrote:
> thank you for review, I see that Tejun has applied the changes,
> anyway I'll answer your questions.

Oh, please submit incremental patches as necessary.

> > > @@ -31,6 +40,10 @@
> > >  #define PATA_IMX_DRIVE_DATA        0xA0
> > >  #define PATA_IMX_DRIVE_CONTROL        0xD8
> > > 
> > > +static u32 pio_t4[] = { 30,  20,  15,  10,  10 };
> > > +static u32 pio_t9[] = { 20,  15,  10,  10,  10 };
> > > +static u32 pio_tA[] = { 35,  35,  35,  35,  35 };
> > 
> >    Perhaps it makes sense to extend the 'struct ata_timing'...
> > 
> > [...]
> 
> As you guess the numbers are taken right from the ATAPI spec,
> however I haven't found the second ATA controller driver sumbitted
> upstream, which reuses these timings, so probably generalization
> is not needed here. Anyway I would prefer if maintainers do it,
> if they think that it makes sense.

Given that its usage isn't likely to be further expanded, I don't
think it matters that much either way, but it does make sense to put
them in ata_timing.  I'd be happy to apply such a patch.

> >    What do those registers mean?
> 
> You may find a better description from i.MX27 or i.MX31 Reference Manual
> than my retelling, the docs are open.
> 
> toff/ton timings are used to avoid bus contention when switching
> BUFFER_EN signal and data writing period. AFAIK these timings are
> specific to the controller only.
> 
> > > +    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);
> > 
> >    And this one?
> 
> This is trd timing from the ATA/ATAPI spec, "Read Data Valid to IORDY
> active", its minimal value is defined as 0.

Add comments for these explanations maybe?

> > > +
> > > +    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);
> > 
> >    DIV_ROUND_UP(x, T)?
> 
> Yes, it is reasonable.

And also for this cleanup?

Thanks.

-- 
tejun

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

[Bartlomiej Zolnierkiewicz]

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

[PATCH 4/4] pata: imx: support controller modes up to PIO4

[Vladimir Zapolskiy]

Having timing settings for all supported by the controller PIO modes
now it is possible to expand its PIO mask.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/ata/pata_imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/pata_imx.c b/drivers/ata/pata_imx.c
index 8f13c9f..d4caa23 100644
--- a/drivers/ata/pata_imx.c
+++ b/drivers/ata/pata_imx.c
@@ -160,7 +160,7 @@ static int pata_imx_probe(struct platform_device *pdev)
 	ap = host->ports[0];
 
 	ap->ops = &pata_imx_port_ops;
-	ap->pio_mask = ATA_PIO0;
+	ap->pio_mask = ATA_PIO4;
 	ap->flags |= ATA_FLAG_SLAVE_POSS;
 
 	io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
2.10.2

Re: [PATCH 0/4] pata: imx: set timings for PIO modes up to PIO4

[Tejun Heo]

On Wed, Nov 09, 2016 at 02:56:34AM +0200, Vladimir Zapolskiy wrote:
> The changeset adds support of PIO modes up to PIO4 by setting
> necessary timings in the driver, before the change it is assumed
> that the timings are always set by a bootloader once and thus
> only one possible PIO mode has been supported (PIO0). With
> this change the driver can be used on boards without ATA controller
> configuration done by a bootloader.
> 
> The change is tested on a legacy i.MX31 board with an HDD connected
> by a 40-pin flat cable.

Applied to libata/for-4.10.

Thanks.

-- 
tejun