Re: [RFC/Review] libata driver for Apple "macio" pata

[Tejun Heo <htejun@gmail.com>]

Benjamin Herrenschmidt wrote:
>>> +static void pata_macio_set_timings(struct ata_port *ap,
>>> +				   struct ata_device *adev)
>> Using single function for ->set_piomode and ->set_dmamode might not be
>> a good idea as libata core might call ->set_piomode with pio mode set
>> to PIO0 before reset but leaving the dma mode as is expecting the
>> transfer mode to be configured to PIO0.  However, this function would
>> configure considering the left-over DMA mode.
> 
> Would that be a problem as long as we use PIO ? Ie. the chipset will
> use the PIO timings when doing PIO transfers.

Some controllers share part of timing settings between PIO and DMA, so
the result might be different from expected.

> The reason I do both in
> one go is because of the shasta controller. For all the other one, I
> know quite precisely what all the bits are, and the PIO vs. DMA timing
> bits are clearly separate.
> 
> For shasta, I have no doc nor useful infos in the Darwin driver other
> than the timing tables, and they seem to have at least a one bit overlap
> between PIO and DMA timings. IE. This bit is set by the PIO timings and
> by some DMA timings, and it's unclear to me how to deal with that. IE,
> if you set a DMA timing where it's not set, and a PIO timing where it's
> set, should I set it or not ?
> 
> Thus I decided to do a single function that does both and OR them
> together, which corresponds to what Darwin does. I can still split them
> again, but there's still the question of that mysterious bit :-)

Yeah, exactly what I was talking about. :-) I think I'll just update the
core layer such that dma mode too is cleared when (re-)configuration starts.

>>> +static unsigned long pata_macio_mode_filter(struct ata_device *adev,
>>> +					    unsigned long xfer_mask)
>>> +{
>>> +	struct pata_macio_priv *priv = adev->link->ap->private_data;
>>> +
>>> +	if (priv->dma_regs == NULL)
>>> +		xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
>>> +	return xfer_mask;
>>> +}
>> Wouldn't it be better to clear these during initialization?
> 
> I could. Doesn't matter much where it's done, does it ? Doing there
> allows to deal with a failure in my port_start() callback, if the
> allocation of the DMA table fails, I clear dma_regs.

Functionally, it doesn't really matter but it's just customary to use
->mode_filter() to enforce dynamic restrictions - say ATA can go over
UDMA/66 but ATAPI can't or when both devices are present one transfer
mode affects the other kind of things.  Hmm... But then again,
ata_bmdma_mode_filter() is used to put the same kind of restriction as
yours.  Heh..  Never mind this suggestion.

>>> +	/* Convert the last command to an input/output */
>>> +	if (pi) {
>>> +		table--;
>>> +		st_le16(&table->command, write ? OUTPUT_LAST: INPUT_LAST);
>>> +		table++;
>>> +	}
>> libata guarantees that pi is not zero here.
> 
> Ok. Thanks. BTW. I do have another question about that though.
> 
> I set currently the max size of the sglist to half of my max of DMA
> commands a bit arbitrarily.
> 
> The reason is that I can only have 64K-4K per transfer (I don't think I
> can do 64K per DBDMA entry). So the above routine can potentially
> breakup, in the worst case scenario, the table into twice as many
> entries if they are all 64K precisely.
> 
> Is there a way I can set a max size for a given sglist segment ? I know
> the iommu can coalesce them back, but at least I know that breaking them
> up won't cause more than what the iommu had as an input .. I don't have
> an alignment restriction.

Maybe pci_set_dma_max_seg_size() which ends up in
blk_queue_max_segment_size().  sata_inic162x uses it and your
requirement seems similar.

>>> +	/* If dbdma didn't execute the STOP command yet, the
>>> +	 * active bit is still set. We consider that we aren't
>>> +	 * sharing interrupts (which is hopefully the case with
>> Wouldn't it be better to clear IRQF_SHARED for those?  Also, what
>> happens w/ irqpoll?
> 
> I don't think we ever use irqpoll on powerpc, but that's a good
> question. I don't know what the right answer is. In fact, I looked a bit
> at the libata core irq handling and it looks like if we return that the
> IRQ wasn't for us, after 1000 iteration, libata goes read the status and
> clear the IRQ anyway, which doesn't sound that a good idea if the IRQ is
> shared....

That code is activated only if ATA_IRQ_TRAP is set and it's not even in
config option.  I don't think anyone ever uses it.  Also, the worst
reading off status register and clearing BMDMA register can do is losing
an interrupt causing a timeout.  Weighed against the dreaded nobody
cared, it's not so bad.  libata definitely needs some improvements in
this area.

> I know some variant of the cell do have a register I can read to make
> sure the IRQ came from it (or not), but I have to figure out if it
> exists on the ohare variant or not. If it does, then I may be able to do
> something saner here.

Ah... if there's anything like that, please use it.  I have no idea why
the original TF and even BMDMA interface didn't include proper interrupt
pending bit.  Oh well, they were designed when floppy was popular after all.

> In the meantime, the above is the same as what I did for drivers/ide.
> 
> Another option would be to use the DMA channel IRQ (it's a separate IRQ
> I can use) and do like Darwin, that is, wait for both IRQs to happen
> before moving forward.

Hmm...

>>> +	/* Check that it can be enabled */
>>> +	if (pci_enable_device(pdev)) {
>>> +		dev_err(&pdev->dev,
>>> +			"Cannot enable controller PCI device\n");
>>> +		return -ENXIO;
>>> +	}
>> Maybe pcim_enable_device() can be used?  Or is it difficult because of
>> the macio stuff?
> 
> I wouldn't bother. There's no point in disabling it, this is just to
> make sure it's been properly enabled at boot.

pcim_enable_device() is a little bit more than managed pdev enable.  It
turns on resource management for other PCI resources like IO regions and
INTX setting.

Thanks.

-- 
tejun