Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Sep 12, 2009 at 5:55 PM, Jeff Garzik <jgarzik@xxxxxxxxx> wrote:
>
>
> General comment:
>
> * since you use iomap to map the region, you should use ioread{8,16,32} /
> iowrite{8,16,32} accessors.  Do not use inb/outb/inl/outl/etc.
.
I used them for runtime hot registers by separately mapping them
simply to avoid an extra overhead of ioread/iowrite, over the
portability.
I know it's not a good idea but in this case for these hot ports can
in/out be used?


>
> * run through scripts/checkpatch.pl
>

Weird. I don't see any WS issues you pointed below in my source code
or git diff file, except UT = T/4 below.

>
> On 09/12/2009 08:26 PM, Jung-Ik (John) Lee wrote:
>>
>> +/*
>> + * IO Registers
>> + * Note that all runtime hot ports are cached in ap private_data
>> + */
>> +
>> +#define ATP867X_IO_CHANNEL_OFFSET      0x10
>> +#define ATP867X_IOBASE(d)              pci_resource_start((d), 0)
>> +#define ATP867X_SYS_INFO(d)            (0x3F + ATP867X_IOBASE(d))
>> +
>> +#define ATP867X_IO_PORTBASE(d, port)   (0x00 + ATP867X_IOBASE(d) + \
>> +                                       (port) *
>> ATP867X_IO_CHANNEL_OFFSET)
>> +#define ATP867X_IO_DMABASE(d, port)    (0x40 + \
>> +                                       ATP867X_IO_PORTBASE((d), (port)))
>> +
>> +#define ATP867X_IO_STATUS(d, port)     (0x07 + \
>> +                                       ATP867X_IO_PORTBASE((d), (port)))
>> +#define ATP867X_IO_ALTSTATUS(d, port)  (0x0E + \
>> +                                       ATP867X_IO_PORTBASE((d), (port)))
>> +
>> +#define ATP867X_IO_MSTRPIOSPD(d, port) (0x08 + ATP867X_IO_DMABASE((d),
>> (port)))
>> +#define ATP867X_IO_SLAVPIOSPD(d, port) (0x09 + ATP867X_IO_DMABASE((d),
>> (port)))
>> +#define ATP867X_IO_8BPIOSPD(d, port)   (0x0A + ATP867X_IO_DMABASE((d),
>> (port)))
>> +#define ATP867X_IO_DMAMODE(d, port)    (0x0B + ATP867X_IO_DMABASE((d),
>> (port)))
>> +
>> +#define ATP867X_IO_PORTSPEED(d, port)  (0x4A + \
>> +                                       ATP867X_IO_PORTBASE((d), (port)))
>> +#define ATP867X_IO_PREREAD(d, port)    (0x4C + \
>> +                                       ATP867X_IO_PORTBASE((d), (port)))
>> +
>> +/*
>> + * IO Register Bitfields
>> + */
>> +
>> +#define ATP867X_IO_PIOSPD_ACTIVE_SHIFT 4
>> +#define ATP867X_IO_PIOSPD_RECOVER_SHIFT        0
>> +
>> +#define ATP867X_IO_DMAMODE_MSTR_SHIFT  0
>> +#define ATP867X_IO_DMAMODE_MSTR_MASK   0x07
>> +#define ATP867X_IO_DMAMODE_SLAVE_SHIFT 4
>> +#define ATP867X_IO_DMAMODE_SLAVE_MASK  0x70
>> +
>> +#define ATP867X_IO_DMAMODE_UDMA_6      0x07
>> +#define ATP867X_IO_DMAMODE_UDMA_5      0x06
>> +#define ATP867X_IO_DMAMODE_UDMA_4      0x05
>> +#define ATP867X_IO_DMAMODE_UDMA_3      0x04
>> +#define ATP867X_IO_DMAMODE_UDMA_2      0x03
>> +#define ATP867X_IO_DMAMODE_UDMA_1      0x02
>> +#define ATP867X_IO_DMAMODE_UDMA_0      0x01
>> +#define ATP867X_IO_DMAMODE_DISABLE     0x00
>> +
>> +#define ATP867X_IO_SYS_INFO_66MHZ      0x04
>> +#define ATP867X_IO_SYS_INFO_SLOW_UDMA5 0x02
>> +#define ATP867X_IO_SYS_MASK_RESERVED   (~0xf1)
>> +
>> +#define ATP867X_IO_PORTSPEED_VAL       0x1143
>> +#define ATP867X_PREREAD_VAL            0x0200
>> +
>> +#define ATP867X_NUM_PORTS              4
>> +#define ATP867X_BAR_IOBASE             0
>> +#define ATP867X_BAR_ROMBASE            6
>
> enums are the preferred method of creating named constants.  This
> potentially makes the code more readable in the debugger, imparts type
> information with the constant, and makes CPP output more readable.  eg.
>
> enum {
>        ATP867X_NUM_PORTS               = 4,
>        ATP867X_BAR_IOBASE              = 0,
> };

Sure.

>
>> +struct atp867x_priv {
>> +       unsigned long   dma_mode;
>> +       unsigned long   mstr_piospd;
>> +       unsigned long   slave_piospd;
>> +       unsigned long   eightb_piospd;
>> +       int             pci66mhz;
>> +};
>> +
>> +
>> +static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device
>> *adev)
>> +{
>> +       struct pci_dev *pdev    = to_pci_dev(ap->host->dev);
>> +       struct atp867x_priv *dp = ap->private_data;
>> +       u8 speed = adev->dma_mode;
>> +       u8 b;
>> +       u8 mode;
>> +
>> +
>> +       switch (speed) {
>> +       case XFER_UDMA_6:
>> +               mode = ATP867X_IO_DMAMODE_UDMA_6;
>> +               break;
>> +       case XFER_UDMA_5:
>> +               mode = ATP867X_IO_DMAMODE_UDMA_5;
>> +               break;
>> +       case XFER_UDMA_4:
>> +               mode = ATP867X_IO_DMAMODE_UDMA_4;
>> +               break;
>> +       case XFER_UDMA_3:
>> +               mode = ATP867X_IO_DMAMODE_UDMA_3;
>> +               break;
>> +       case XFER_UDMA_2:
>> +               mode = ATP867X_IO_DMAMODE_UDMA_2;
>> +               break;
>> +       case XFER_UDMA_1:
>> +               mode = ATP867X_IO_DMAMODE_UDMA_1;
>> +               break;
>> +       case XFER_UDMA_0:
>> +               mode = ATP867X_IO_DMAMODE_UDMA_0;
>> +               break;
>> +       default:
>> +               printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
>> +                       " Default to XFER_UDMA_0.\n", (unsigned)speed);
>> +               mode = ATP867X_IO_DMAMODE_UDMA_0;
>
> a table would be nice, preferred over a switch statement.  You may use
> ARRAY_SIZE() macro to generate a constant at compile time for number of
> elements in array.

OK. I had it in a pure math like mode = speed - XFER_UDMA_0 +1;

>
>
>> +
>> +       /*
>> +        * Doc 6.6.9: decrease the udma mode value by 1 for safer UDMA
>> speed
>> +        * on 66MHz bus
>> +        *   rev-A: UDMA_1~4 (5, 6 no change)
>> +        *   rev-B: all UDMA modes
>> +        *   UDMA_0 stays not to disable UDMA
>> +        */
>> +       if (dp->pci66mhz&&  mode>  ATP867X_IO_DMAMODE_UDMA_0&&
>> +          (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B ||
>> +           mode<  ATP867X_IO_DMAMODE_UDMA_5))
>> +               mode--;
>> +
>> +       b = inb(dp->dma_mode);
>> +       if (adev->devno&  1) {
>> +               b = (b&  ~ATP867X_IO_DMAMODE_SLAVE_MASK) |
>> +                       (mode<<  ATP867X_IO_DMAMODE_SLAVE_SHIFT);
>> +       } else {
>> +               b = (b&  ~ATP867X_IO_DMAMODE_MSTR_MASK) |
>> +                       (mode<<  ATP867X_IO_DMAMODE_MSTR_SHIFT);
>
> add whitespace between all tokens, eg. surrounding "&&" and ">" and "&"
> operators

Weird. I don;t these in my source/git diff file.

>
>
>> +       }
>> +       outb(b, dp->dma_mode);
>> +}
>> +
>> +static int atp867x_get_active_clocks_shifted(unsigned int clk)
>> +{
>> +       unsigned char clocks = clk;
>> +
>> +       switch (clocks) {
>> +       case 0:
>> +               clocks = 1;
>> +               break;
>> +       case 1 ... 7:
>> +               break;
>> +       case 8 ... 12:
>> +               clocks = 7;
>> +               break;
>> +       default:
>> +               printk(KERN_WARNING "ATP867X: active %dclk is invalid. "
>> +                       "Using default 8clk.\n", clk);
>> +               clocks = 0;     /* 8 clk */
>> +               break;
>> +       }
>> +       return clocks<<  ATP867X_IO_PIOSPD_ACTIVE_SHIFT;
>> +}
>> +
>> +static int atp867x_get_recover_clocks_shifted(unsigned int clk)
>> +{
>> +       unsigned char clocks = clk;
>> +
>> +       switch (clocks) {
>> +       case 0:
>> +               clocks = 1;
>> +               break;
>> +       case 1 ... 11:
>> +               break;
>> +       case 12:
>> +               clocks = 0;
>> +               break;
>> +       case 13: case 14:
>> +               --clocks;
>> +               break;
>> +       case 15:
>> +               break;
>> +       default:
>> +               printk(KERN_WARNING "ATP867X: recover %dclk is invalid. "
>> +                       "Using default 15clk.\n", clk);
>> +               clocks = 0;     /* 12 clk */
>> +               break;
>> +       }
>> +       return clocks<<  ATP867X_IO_PIOSPD_RECOVER_SHIFT;
>
> whitespace between all tokens

ditto

>
>> +static void atp867x_set_piomode(struct ata_port *ap, struct ata_device
>> *adev)
>> +{
>> +       struct ata_device *peer = ata_dev_pair(adev);
>> +       struct atp867x_priv *dp = ap->private_data;
>> +       u8 speed = adev->pio_mode;
>> +       struct ata_timing t, p;
>> +       int T, UT;
>> +       u8 b;
>> +
>> +       T = 1000000000 / 33333;
>> +       UT = T/4;
>
> ditto

Done.

>
>
>> +       switch (speed) {
>> +       case XFER_PIO_4:
>> +       case XFER_PIO_3:
>> +       case XFER_PIO_2:
>> +       case XFER_PIO_1:
>> +       case XFER_PIO_0:
>> +       case XFER_PIO_SLOW:
>> +               break;
>> +       default:
>> +               printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
>> +                       " Default to XFER_PIO_0.\n", (unsigned)speed);
>> +               speed = XFER_PIO_0;
>
> unsupported speeds are masked out via ata_port_info.  no need for additional
> checks.

Done.

>
>
>> +       ata_timing_compute(adev, speed,&t, T, UT);
>> +       if (peer&&  peer->pio_mode) {
>> +               ata_timing_compute(peer, peer->pio_mode,&p, T, UT);
>> +               ata_timing_merge(&p,&t,&t, ATA_TIMING_8BIT);
>> +       }
>> +
>> +       b = inb(dp->dma_mode);
>> +       if (adev->devno&  1)
>> +               b = (b&  ~ATP867X_IO_DMAMODE_SLAVE_MASK);
>> +       else
>> +               b = (b&  ~ATP867X_IO_DMAMODE_MSTR_MASK);
>> +       outb(b, dp->dma_mode);
>> +
>> +       b = atp867x_get_active_clocks_shifted(t.active) |
>> +               atp867x_get_recover_clocks_shifted(t.recover);
>> +       if (dp->pci66mhz)
>> +               b += 0x10;
>> +
>> +       if (adev->devno&  1)
>> +               outb(b, dp->slave_piospd);
>> +       else
>> +               outb(b, dp->mstr_piospd);
>> +
>> +       /*
>> +        * use the same value for comand timing as for PIO timimg
>> +        */
>> +       outb(b, dp->eightb_piospd);
>
> whitespace + ioread/iowrite

ditto.

>
>
>> +static int atp867x_cable_detect(struct ata_port *ap)
>> +{
>> +       return ATA_CBL_PATA40_SHORT;
>> +}
>> +
>> +static struct scsi_host_template atp867x_sht = {
>> +       ATA_BMDMA_SHT(DRV_NAME),
>> +};
>> +
>> +static struct ata_port_operations atp867x_ops = {
>> +       .inherits               =&ata_bmdma_port_ops,
>> +       .cable_detect           = atp867x_cable_detect,
>> +       .set_piomode            = atp867x_set_piomode,
>> +       .set_dmamode            = atp867x_set_dmamode,
>> +};
>> +
>> +
>> +#ifdef ATP867X_DEBUG
>> +static void atp867x_check_res(struct pci_dev *pdev)
>> +{
>> +       int i;
>> +       unsigned long start, len;
>> +
>> +       /* Check the PCI resources for this channel are enabled */
>> +       for (i = 0; i<  DEVICE_COUNT_RESOURCE; i++) {
>> +               start = pci_resource_start(pdev, i);
>> +               len   = pci_resource_len(pdev, i);
>> +               printk(KERN_DEBUG "ATP867X: resource start:len=%lx:%lx\n",
>> +                       start, len);
>> +       }
>> +}
>> +
>> +static void atp867x_check_ports(struct ata_port *ap, struct pci_dev
>> *pdev)
>> +{
>> +       struct ata_ioports *ioaddr =&ap->ioaddr;
>> +       struct atp867x_priv *dp = ap->private_data;
>> +       int port = ap->port_no;
>> +
>> +       printk(KERN_DEBUG "ATP867X: port[%d] addresses\n"
>> +               "  cmd_addr     =0x%llx, 0x%llx\n"
>> +               "  ctl_addr     =0x%llx, 0x%llx\n"
>> +               "  bmdma_addr   =0x%llx, 0x%llx\n"
>> +               "  data_addr    =0x%llx\n"
>> +               "  error_addr   =0x%llx\n"
>> +               "  feature_addr =0x%llx\n"
>> +               "  nsect_addr   =0x%llx\n"
>> +               "  lbal_addr    =0x%llx\n"
>> +               "  lbam_addr    =0x%llx\n"
>> +               "  lbah_addr    =0x%llx\n"
>> +               "  device_addr  =0x%llx\n"
>> +               "  status_addr  =0x%llx\n"
>> +               "  command_addr =0x%llx\n"
>> +               "  dp->dma_mode =0x%llx\n"
>> +               "  dp->mstr_piospd      =0x%llx\n"
>> +               "  dp->slave_piospd     =0x%llx\n"
>> +               "  dp->eightb_piospd    =0x%llx\n",
>> +               port,
>> +               (unsigned long long)ioaddr->cmd_addr,
>> +                       ATP867X_IO_PORTBASE(pdev, port),
>> +               (unsigned long long)ioaddr->ctl_addr,
>> +                       ATP867X_IO_ALTSTATUS(pdev, port),
>> +               (unsigned long long)ioaddr->bmdma_addr,
>> +                       ATP867X_IO_DMABASE(pdev, port),
>> +               (unsigned long long)ioaddr->data_addr,
>> +               (unsigned long long)ioaddr->error_addr,
>> +               (unsigned long long)ioaddr->feature_addr,
>> +               (unsigned long long)ioaddr->nsect_addr,
>> +               (unsigned long long)ioaddr->lbal_addr,
>> +               (unsigned long long)ioaddr->lbam_addr,
>> +               (unsigned long long)ioaddr->lbah_addr,
>> +               (unsigned long long)ioaddr->device_addr,
>> +               (unsigned long long)ioaddr->status_addr,
>> +               (unsigned long long)ioaddr->command_addr,
>> +               (unsigned long long)dp->dma_mode,
>> +               (unsigned long long)dp->mstr_piospd,
>> +               (unsigned long long)dp->slave_piospd,
>> +               (unsigned long long)dp->eightb_piospd);
>> +}
>> +#endif
>> +
>> +static inline void atp867x_get_ports(struct pci_dev *dev, unsigned int
>> port,
>> +       unsigned long *base, unsigned long *control)
>> +{
>> +       *base = ATP867X_IO_PORTBASE(dev, port);
>> +       *control = ATP867X_IO_ALTSTATUS(dev, port);
>> +}
>> +
>> +static inline unsigned long atp867x_get_dma_base(struct pci_dev *dev,
>> +       unsigned int port)
>> +{
>> +       return ATP867X_IO_DMABASE(dev, port);
>> +}
>> +
>> +static int atp867x_set_priv(struct ata_port *ap)
>> +{
>> +       struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>> +       struct atp867x_priv *dp;
>> +       int port = ap->port_no;
>> +
>> +       dp = ap->private_data =
>> +               devm_kzalloc(&pdev->dev, sizeof(*dp), GFP_KERNEL);
>> +       if (dp == NULL)
>> +               return -ENOMEM;
>> +
>> +       dp->dma_mode     = ATP867X_IO_DMAMODE(pdev, port);
>> +       dp->mstr_piospd  = ATP867X_IO_MSTRPIOSPD(pdev, port);
>> +       dp->slave_piospd = ATP867X_IO_SLAVPIOSPD(pdev, port);
>> +       dp->eightb_piospd = ATP867X_IO_8BPIOSPD(pdev, port);
>> +
>> +       dp->pci66mhz = inb(ATP867X_SYS_INFO(pdev))&
>>  ATP867X_IO_SYS_INFO_66MHZ;
>> +
>> +       return 0;
>> +}
>> +
>> +static int atp867x_ata_pci_sff_init_host(struct ata_host *host)
>> +{
>> +       struct device *gdev = host->dev;
>> +       struct pci_dev *pdev = to_pci_dev(gdev);
>> +       unsigned int mask = 0;
>> +       int i, rc;
>> +
>> +       /*
>> +        * do not map rombase
>> +        */
>> +       rc = pcim_iomap_regions(pdev, 1<<  ATP867X_BAR_IOBASE, DRV_NAME);
>> +       if (rc == -EBUSY)
>> +               pcim_pin_device(pdev);
>> +       if (rc)
>> +               return rc;
>> +       host->iomap = pcim_iomap_table(pdev);
>> +
>> +#ifdef ATP867X_DEBUG
>> +       atp867x_check_res(pdev);
>> +
>> +       for (i = 0; i<  PCI_ROM_RESOURCE; i++)
>> +               printk(KERN_DEBUG "ATP867X: iomap[%d]=0x%llx\n", i,
>> +                       (unsigned long long)(host->iomap[i]));
>> +#endif
>> +
>> +       /*
>> +        * request, iomap BARs and init port addresses accordingly
>> +        */
>> +       for (i = 0; i<  host->n_ports; i++) {
>> +               unsigned long base, control, dmabase;
>> +               struct ata_port *ap = host->ports[i];
>> +               struct ata_ioports *ioaddr =&ap->ioaddr;
>> +
>> +               atp867x_get_ports(pdev, i,&base,&control);
>> +               dmabase = atp867x_get_dma_base(pdev, i);
>> +
>> +               ioaddr->cmd_addr = devm_ioport_map(gdev, base, 8);
>> +               ioaddr->ctl_addr = ioaddr->altstatus_addr =
>> +                               devm_ioport_map(gdev, control, 1);
>> +               ioaddr->bmdma_addr = devm_ioport_map(gdev, dmabase, 8);
>
> why are you mapping separately from pcim_iomap_regions?
>
> Just do the mapping all at once, with pcim_iomap_regions, then simply
> calculate addresses in the n_ports loop.

OK.

>
>
>
>
>> +       rc = pcim_enable_device(pdev);
>> +       if (rc)
>> +               return rc;
>> +
>> +       /*
>> +        * Broken BIOS might not set latency high enough
>> +        */
>> +       pci_read_config_byte(pdev, PCI_LATENCY_TIMER,&v);
>> +       if (v<  0x80) {
>> +               v = 0x80;
>> +               pci_write_config_byte(pdev, PCI_LATENCY_TIMER, v);
>> +               printk(KERN_DEBUG "ATP867X: set latency timer of device
>> %s"
>> +                       " to %d\n", pci_name(pdev), v);
>> +       }
>
> this seems pointless - pci_set_master() already does this
>
pci_set_master won't re-set it if BIOS set it to somewhere between 16
and 256. This controller wants 0x80.
so, if BIOS set to less than 0x80, like 0x20, pci_set_master will keep
the value.
I could do this via pci fixup or quirks but that seems too much for
this simple setting.

Will send a new rev soon.
Thanks,
-John

>
--
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

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux