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.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
