[PATCH 3/4 v3] ata: Add driver for Faraday Technology FTIDE010

[Linus Walleij]

Hi Barlomiej,

thanks a *lot* for detailed review and for showing me some really
tricky corners of ATA support!

Most things I just fixed, will send v4 soon.

On Tue, May 30, 2017 at 4:31 PM, Bartlomiej Zolnierkiewicz
<b.zolnierkie at samsung.com> wrote:

>> +static const bool set_mdma_66_mhz[] = { true, true, true, true };
>
> 50 MHz MWDMA timings are never used by the driver and can be removed..

I think it's nice to keep around as documentation really, and someone may want
to patch it in and experiment with 50MHz mode if things are not
working for them.

> CLK_MOD_REG is shared between master and slave so the driver needs to
> make sure that right clock is used if master and slave devices require
> different clocks (i.e. master is using UDMA5 and slave is using UDMA6).
(...)
> Moreover MWDMA_TIMING_REG is also shared between devices.
(...)
> ->qc_issue method can be used to program the right tuning values, i.e.

Wow thanks a lot for clearing this up!

I was a bit confused about how to deal with this. Now it makes a
lot more sense!

> static unsigned int ftide010_qc_issue(struct ata_queued_cmd *qc)
> {
>         struct ata_port *ap = qc->ap;
>         struct ata_device *adev = qc->dev;
>
>         if (adev != ap->private_data && ata_dma_enabled(adev))
>                 ftide010_set_dmamode(ap, adev);
>
>         return ata_bmdma_qc_issue(qc);
> }
>
> [ set ap->private_data to adev at the end of ftide010_set_dmamode() ]

I implemented this exactly as above, with some extra comments
so it's clear what is going on.

>> +static int pata_ftide010_gemini_cable_detect(struct ata_port *ap)
>> +{
>> +     struct ftide010 *ftide = ap->host->private_data;
>> +
>> +     /*
>> +      * Return the master cable, I have no clue how to return a different
>> +      * cable for the slave than for the master.
>> +      */
>
> Seems like ->cable_detect needs to operate on ata_device * now?
>
> Tejun?

Yeah... it looks like something libata is not really made to handle.

This platform has one IRQ for each port bridge anyways.

>> +             case GEMINI_MUXMODE_3:
>> +                     ftide->master_cbl = ATA_CBL_PATA40;
>> +                     ftide->slave_cbl = ATA_CBL_PATA40;
>> +                     break;
>> +             }
>
> It seems that for PATA devices 80-wires cable will be never
> used, is this correct?

That is a simplification. I haven't seen any systems using the
PATA interface at all.

I have put in some comments that we assume the
40-wire interface, and if something else like 80-wire is in use
then that needs to be specified in the device tree since the
SoC and driver cannot detect it without special hardware.

I guess I could add DT properties for it but it seems a bit
speculative since I can't test it.

Yours,
Linus Walleij