Re: Not-yet-working inic162x driver

[Tejun Heo <htejun@gmail.com>]

Hello, Jeff.

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> Hello, guys.
>> +static struct ata_port_info inic_port_info = {
>> +    .sht            = &inic_sht,
>> +    /* XXX - for the time being, SATA_RESET fails faster... */
>> +    .host_flags        = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
>> +                  ATA_FLAG_MMIO | ATA_FLAG_SATA_RESET,
>> +    .pio_mask        = 0x1f,
>> +    .mwdma_mask        = 0x07,
>> +    .udma_mask        = 0x7f,
>> +    .port_ops        = &inic_ops,
>> +};
> 
> 
> does pio-only (dma_masks == 0) work?
> 

No, IDENTIFY doesn't work.  Even SRST and SATA_RESET don't work.

> 
>> +static u32 inic_scr_read(struct ata_port *ap, unsigned sc_reg)
>> +{
>> +    void __iomem *scr_addr = (void __iomem *)ap->ioaddr.scr_addr;
>> +
>> +    if (sc_reg <= SCR_ACTIVE)
>> +        return readl(scr_addr + sc_reg * 4);
>> +    return -1U;
> 
> 
> -1U is sorta a contradiction.
> 

I kind of prefer unsigned -1 over 0xf*, but no big deal.

> 
>> +static void inic_bmdma_setup(struct ata_queued_cmd *qc)
>> +{
>> +    struct ata_port *ap = qc->ap;
>> +    struct inic_port_priv *pp = ap->private_data;
>> +    void __iomem *base = AP_MMIO(ap);
>> +    int rw = qc->tf.flags & ATA_TFLAG_WRITE;
>> +    u32 len;
>> +
>> +    /* make sure device sees PRD table writes */
>> +    wmb();
>> +
>> +    /* load transfer length - is this necessary? */
>> +    if (is_atapi_taskfile(&qc->tf))
>> +        len = qc->nsect << 9;
>> +    else
>> +        len = qc->nbytes;
> 
> 
> In the about 'if atapi_taskfile() foo else bar', "foo" and "bar" appear 
> reversed.
> 

Yeap, you're right.  The test was 'if (qc->nsect)' initially and I 
forgot to swap the bodies after changing the condition.

> 
>> +static irqreturn_t inic_interrupt(int irq, void *dev_instance, struct 
>> pt_regs *regs)
>> +{
>> +    struct ata_host_set *host_set = dev_instance;
>> +    void __iomem *regs_base = host_set->mmio_base;
>> +    u16 host_irq_stat;
>> +    int i, handled = 0;;
>> +
>> +    host_irq_stat = readw(regs_base + HOST_IRQ_STAT);
>> +
>> +    if (unlikely(!(host_irq_stat & HIRQ_GLOBAL)))
>> +        goto out;
> 
> 
> check for both stat==0 (nothing to do) and stat=ffffffff (hw fault or 
> unplug)
> 

Sure.

> 
>> +static int inic_port_start(struct ata_port *ap)
>> +{
>> +    struct device *dev = ap->host_set->dev;
>> +    void __iomem *regs_base = ap->host_set->mmio_base;
>> +    void __iomem *base = AP_MMIO(ap);
>> +    struct inic_port_priv *pp = NULL;
> 
> 
> needless init
> 

Okay.

> 
>> +    struct inic_cmd_block *cb = NULL;
>> +    dma_addr_t cb_dma;
>> +    int rc;
>> +    u32 tmp;
>> +
>> +    /* Alloc resources */
>> +    rc = -ENOMEM;
>> +    pp = kzalloc(sizeof(*pp), GFP_KERNEL);
>> +    if (!pp)
>> +        goto err_out;
>> +
>> +    cb = dma_alloc_coherent(dev, sizeof(*cb), &cb_dma, GFP_KERNEL);
>> +    if (!cb)
>> +        goto err_out;
>> +    memset(cb, 0, sizeof(*cb));
>> +
>> +    rc = ata_pad_alloc(ap, dev);
>> +    if (rc)
>> +        goto err_out;
[[--snip--]]
>> +
>> + err_out:
>> +    if (cb)
>> +        dma_free_coherent(dev, sizeof(*cb), cb, cb_dma);
>> +    if (pp)
>> +        kfree(pp);
> 
> 
> 1) no need to check for NULL before calling kfree()

Right.

> 2) what about undoing ata_pad_alloc() ?

The last jump to err_out is after failure of ata_pad_alloc().  Also, 
ata_pad_free() doesn't check for ap->pad == NULL condition before 
freeing, so the test should be done explicitly and it seemed a bit out 
of place as pad handling is pretty much hidden from low level driver. 
How about adding ap->pad condition check in ata_pad_free()?

> 
>> +static int inic_init_one(struct pci_dev *pdev, const struct 
>> pci_device_id *ent)
>> +{
>> +    static int printed_version = 0;
>> +    int mmio_idx, i, rc;
>> +    struct ata_probe_ent *probe_ent = NULL;
>> +    void __iomem *regs_base = NULL;
>> +    struct ata_port_info *pinfo;
>> +    struct ata_ioports *port;
>> +    u16 ctrl;
>> +    u32 tmp;
>> +
>> +    if (!printed_version++)
>> +        dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
>> +
>> +    rc = pci_enable_device(pdev);
>> +    if (rc)
>> +        return rc;
>> +
>> +    rc = pci_request_regions(pdev, DRV_NAME);
>> +    if (rc)
>> +        goto out_disable;
>> +
>> +    /*
>> +     * If the controller is configured for Cardbus mode, BAR[0-1]
>> +     * map to BAR[4-5] of normal PCI mode and the rest are
>> +     * reserved.
>> +     */
>> +    mmio_idx = 5;
>> +    if (pci_resource_flags(pdev, 1) & IORESOURCE_MEM)
>> +        mmio_idx = 1;
>> +
>> +    if (!(pci_resource_flags(pdev, mmio_idx) & IORESOURCE_MEM)) {
>> +        dev_printk(KERN_ERR, &pdev->dev,
>> +               "invalid mmio area, flags %lx size %lu\n",
>> +               pci_resource_flags(pdev, mmio_idx),
>> +               pci_resource_len(pdev, mmio_idx));
> 
> 
> you should print out mmio_idx, as well as the PCI resources
> 

Yeap.

[[--snip--]]
>> +
>> +    /* Turn on global IRQ */
>> +    ctrl &= ~HCTRL_GINTDIS;
>> +    writew(ctrl, regs_base + HOST_CTRL);
>> +
>> +    pci_set_master(pdev);
> 
> 
> Missing a call to pci_intx(), most likely...
> 

I'll add it.

Thanks for reviewing it.  :-)

Do you happen to know anyone from Initio?

-- 
tejun