Re: [PATCH 5/5] libata/drivers: Add pata_macio, driver Apple PowerMac/PowerBook IDE controller

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2009-12-01 at 17:00 +0900, Tejun Heo wrote:

> > +#define IDE_WAKEUP_DELAY	(1*HZ)
> 
> nitpick: In libata, it's common to use msecs for timing values so that
> might be a better option.

Yeah, that's cruft lifted from the old driver, I'll fix it.

> > +static const struct pata_macio_timing *pata_macio_find_timing(
> > +					    struct pata_macio_priv *priv,
> > +					    int mode)
> > +{
> > +	int i = 0;
> > +
> > +	while (priv->timings[i].mode > 0) {
> > +		if (priv->timings[i].mode == mode)
> > +			return &priv->timings[i];
> > +		i++;
> > +	}
> > +	return NULL;
> 
> Wouldn't for (i = 0; ...) look better?

Yeah, I suppose so :-)

> > +static void pata_macio_bmdma_start(struct ata_queued_cmd *qc)
> > +{
> > +	struct ata_port *ap = qc->ap;
> > +	struct pata_macio_priv *priv = ap->private_data;
> > +	struct dbdma_regs __iomem *dma_regs = ap->ioaddr.bmdma_addr;
> > +
> > +	dev_dbgdma(priv->dev, "%s: qc %p\n", __func__, qc);
> > +
> > +	writel((RUN << 16) | RUN, &dma_regs->control);
> > +	/* Make sure it gets to the controller right now */
> > +	(void)readl(&dma_regs->control);
> 
> Is flushing necessary here?  There's no ordering issue here, right?

That's also lifted pretty-much as-is from the old driver. Now I probably
wrote that part of the old driver too ages ago :-) I think I just want
to make sure the DMA starts asap and doesn't sit in some bridge write
posting queue for hundreds of cycles.

> > +static void pata_macio_bmdma_stop(struct ata_queued_cmd *qc)
> > +{
> > +	struct ata_port *ap = qc->ap;
> > +	struct pata_macio_priv *priv = ap->private_data;
> > +	struct dbdma_regs __iomem *dma_regs = ap->ioaddr.bmdma_addr;
> > +
> > +	dev_dbgdma(priv->dev, "%s: qc %p\n", __func__, qc);
> > +
> > +	/* Stop the DMA engine and wait for it to full halt */
> > +	writel (((RUN|WAKE|DEAD) << 16), &dma_regs->control);
> > +	while (readl(&dma_regs->status) & RUN)
> > +		udelay(1);
> 
> Heh... this is a scary looking loop.  It would be great if the above
> loop can be capped somehow.

Yeah, again, lifted from the old driver. We do that sort of loops in
various other drivers that use those DBDMA channels and I don't think
ever saw the loop hang, so at least those HW aren't -that- broken but I
suppose a little timeout wouldn't hurt. Not sure what to do if we hit it
tho.
 
> > +static u8 pata_macio_bmdma_status(struct ata_port *ap)
> > +{
> > +	struct pata_macio_priv *priv = ap->private_data;
> > +	struct dbdma_regs __iomem *dma_regs = ap->ioaddr.bmdma_addr;
> > +	u32 dstat, rstat = ATA_DMA_INTR;
> > +	unsigned long timeout = 0;
> > +
> > +	dstat = readl(&dma_regs->status);
> > +
> > +	dev_dbgdma(priv->dev, "%s: dstat=%x\n", __func__, dstat);
> > +
> > +	/* We have to things to deal with here:
>                    ^^
>                    two

Yup.

> > +/* port_start is when we allocate the DMA command list */
> > +static int pata_macio_port_start(struct ata_port *ap)
> > +{
> > +	struct pata_macio_priv *priv = ap->private_data;
> > +	struct dbdma_regs __iomem *dma_regs = ap->ioaddr.bmdma_addr;
> > +
> > +	if (dma_regs == NULL)
> > +		return 0;
> > +
> > +	/* Make sure DMA controller is stopped */
> > +	writel((RUN|PAUSE|FLUSH|WAKE|DEAD) << 16, &dma_regs->control);
> > +	while (readl(&dma_regs->status) & RUN)
> > +		udelay(1);
> 
> Hmmm.... this probably belongs to ->freeze() which is responsible for
> stopping any in-flight operations and masking IRQ and libata will call
> it during initialization before requesting IRQ, so you won't need to
> call it explicitly here.

Well, I'm not sure I really -need- the above thing. It's there mostly
for the sake of paranoia. I suppose in case the firmware left it in some
stange state. Anyway, will move to freeze.

> > +#ifdef CONFIG_PMAC_MEDIABAY
> > +static void pata_macio_mb_event(struct macio_dev* mdev, int mb_state)
> > +{
> > +	struct ata_host *host = macio_get_drvdata(mdev);
> > +	struct ata_port *ap;
> > +	struct ata_eh_info *ehi;
> > +	struct ata_device *dev;
> > +	unsigned long flags;
> > +
> > +	if (!host)
> > +		return;
> > +	ap = host->ports[0];
> > +	spin_lock_irqsave(ap->lock, flags);
> > +	ehi = &ap->link.eh_info;
> > +	if (mb_state == MB_CD) {
> > +		ata_ehi_push_desc(ehi, "mediabay plug");
> > +		ata_ehi_hotplugged(ehi);
> > +		ata_port_freeze(ap);
> > +	} else {
> > +		ata_ehi_push_desc(ehi, "mediabay unplug");
> > +		ata_for_each_dev(dev, &ap->link, ALL)
> > +			dev->flags |= ATA_DFLAG_DETACH;
> > +		ata_port_schedule_eh(ap);
> 
> I think you'll need an ata_port_freeze() or abort() here because at
> this point the drive is already gone and all in-flight commands need
> to be failed right away.  Holger, do you remember why
> ata_acpi_detach_device() is using ata_port_schedule_eh() instead?  Was
> it because ata_port_freeze() might end up poking registers after
> hotunplug happened?  ISTR reports where accessing any register there
> causing the whole system to lock up but then why can't it use
> ata_port_abort() instead?

I'll try. Which one ? freeze or abort tho ?

Cheers,
Ben.