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

[Tejun Heo <tj@kernel.org>]

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