[PATCH]: [MPC5200] (v2) Add ATA DMA support

[Tim Yamin]

Hi Grant,

Thanks for the feedback. New version is attached.

> Is this a bug fix?  If so, please put it into a separate patch.

I suppose so, yes. If Ethernet has higher priority than ATA, you can
get a deadlock if you try and download a large file over a LAN to
disk, for example. But given that nothing other than this patch uses
BestComm for ATA do you have any specific reason to split it out into
another patch?

> Good, it can be turned off.  Do you think there is any risk to existing
> ATA users with this patch applied if this is turned off?

With the new version, the only risk if this is turned off is the
change I've made to bestcomm.h. Other than that, you should get no
risk because none of the new
code is executed (mwdma_mask and udma_mask are set to 0 if the option
is turned off).

> Can you find any way to avoid this?  This could be a performance drain.

Previous code had this, so I kept it. Things do seem to work OK
without it, so I've removed it...

> Is there any way to turn on/off DMA at runtime instead of CONFIG time?

You could use libata.dma=0 to force DMA off even if it's enabled at CONFIG time.

>>       priv->ipb_period = 1000000000 / (ipb_freq / 1000);
>>       priv->ata_regs = ata_regs;
>> +     priv->ata_regs_pa = (struct mpc52xx_ata __iomem *) res_mem.start;
>
> I'm not fond of this.  First off, it is *not* __iomem.  It is physical
> address.  It would be better to use the offset_of macro to add an offset
> to the physical base address.  Doing it this way forces you to cast and
> sidestep the compile time checks for incorrect dereferences.

I'm afraid I'm not quite sure what you have in mind here, could you
please provide a pointer?

Thanks,

Tim
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1050-mpc5200-add-ATA-DMA.patch
Type: text/x-patch
Size: 22903 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20080702/79a4a008/attachment.bin>