[PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
Bartlomiej Zolnierkiewicz
b.zolnierkie at samsung.com
Tue Mar 14 09:36:05 PDT 2017
Hi,
On Sunday, March 12, 2017 08:28:43 PM Sergei Shtylyov wrote:
> Hello!
>
> On 03/09/2017 04:01 PM, Bartlomiej Zolnierkiewicz wrote:
>
> > Add Palmchip BK3710 PATA controller driver.
> >
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com>
> [...]
> > diff --git a/drivers/ata/pata_bk3710.c b/drivers/ata/pata_bk3710.c
> > new file mode 100644
> > index 0000000..65ee737
> > --- /dev/null
> > +++ b/drivers/ata/pata_bk3710.c
> > @@ -0,0 +1,395 @@
> > +/*
> > + * Palmchip BK3710 PATA controller driver
> > + *
> > + * Copyright (c) 2017 Samsung Electronics Co., Ltd.
> > + * http://www.samsung.com
> > + *
> > + * Based on palm_bk3710.c:
> > + *
> > + * Copyright (C) 2006 Texas Instruments.
> > + * Copyright (C) 2007 MontaVista Software, Inc., <source at mvista.com>
> > + *
> > + * This file is subject to the terms and conditions of the GNU General Public
> > + * License. See the file "COPYING" in the main directory of this archive
> > + * for more details.
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/ioport.h>
> > +#include <linux/ata.h>
> > +#include <linux/libata.h>
> > +#include <linux/delay.h>
> > +#include <linux/init.h>
> > +#include <linux/clk.h>
> > +#include <linux/platform_device.h>
>
> Probably a good idea to sort the #include's alphabetically...
Done.
> > +
> > +#define DRV_NAME "pata_bk3710"
> > +#define DRV_VERSION "0.1.0"
>
> This macro isn't used anywhere, do we really need it?
Removed.
> > +
> > +#define BK3710_REG_OFFSET 0x1F0
>
> I'd call it BK3710_TF_OFFSET or something of that sort...
> The DM644x manual calls these register command block (which seems to comply
> with ATA wording)...
Renamed.
> > +#define BK3710_CTL_OFFSET 0x3F6
> > +
> > +#define BK3710_BMISP 0x02
>
> Nothing other than the BMIDE status register, dunno why they made it 16-bit...
>
> > +#define BK3710_IDETIMP 0x40
> > +#define BK3710_UDMACTL 0x48
> > +#define BK3710_MISCCTL 0x50
> > +#define BK3710_REGSTB 0x54
> > +#define BK3710_REGRCVR 0x58
> > +#define BK3710_DATSTB 0x5C
> > +#define BK3710_DATRCVR 0x60
> > +#define BK3710_DMASTB 0x64
> > +#define BK3710_DMARCVR 0x68
> > +#define BK3710_UDMASTB 0x6C
> > +#define BK3710_UDMATRP 0x70
> > +#define BK3710_UDMAENV 0x74
> > +#define BK3710_IORDYTMP 0x78
>
> I'd keep all registers declared as in the IDE driver, for the purposes of
> documentation...
Don't see much point in it as the real documentation is available.
> [...]
> > +static void pata_bk3710_setudmamode(void __iomem *base, unsigned int dev,
> > + unsigned int mode)
> > +{
> > + u32 val32;
> > + u16 val16;
> > + u8 tenv, trp, t0;
>
> I think DaveM prefers reverse Christmas tree order with the declarations
> but maybe that's only for the networking tree... :-)
>
> > +
> > + /* DMA Data Setup */
> > + t0 = DIV_ROUND_UP(pata_bk3710_udmatimings[mode].cycletime,
> > + ideclk_period) - 1;
> > + tenv = DIV_ROUND_UP(20, ideclk_period) - 1;
> > + trp = DIV_ROUND_UP(pata_bk3710_udmatimings[mode].rptime,
> > + ideclk_period) - 1;
> > +
> > + /* udmastb Ultra DMA Access Strobe Width */
> > + val32 = ioread32(base + BK3710_UDMASTB) & (0xFF << (dev ? 0 : 8));
>
> I'd separate ioread32() and & to the different lines but as this is copied
> from the IDE driver verbatim, you can ignore me. :-)
Ignored. ;-)
> > + val32 |= (t0 << (dev ? 8 : 0));
>
> Outer parens not really needed.
Fixed.
> > + iowrite32(val32, base + BK3710_UDMASTB);
> > +
> > + /* udmatrp Ultra DMA Ready to Pause Time */
> > + val32 = ioread32(base + BK3710_UDMATRP) & (0xFF << (dev ? 0 : 8));
> > + val32 |= (trp << (dev ? 8 : 0));
>
> Here as well..
ditto
> > + iowrite32(val32, base + BK3710_UDMATRP);
> > +
> > + /* udmaenv Ultra DMA envelop Time */
> > + val32 = ioread32(base + BK3710_UDMAENV) & (0xFF << (dev ? 0 : 8));
> > + val32 |= (tenv << (dev ? 8 : 0));
>
> And here...
ditto
> [...]
> > +static void pata_bk3710_setdmamode(void __iomem *base, unsigned int dev,
>
> Maybe setmwdmamode()?
Renamed.
> > + unsigned short min_cycle,
> > + unsigned int mode)
> > +{
> > + const struct ata_timing *t;
> > + int cycletime;
> > + u32 val32;
> > + u16 val16;
> > + u8 td, tkw, t0;
> > +
> > + t = ata_timing_find_mode(mode);
> > + cycletime = max_t(int, t->cycle, min_cycle);
> > +
> > + /* DMA Data Setup */
> > + t0 = DIV_ROUND_UP(cycletime, ideclk_period);
> > + td = DIV_ROUND_UP(t->active, ideclk_period);
> > + tkw = t0 - td - 1;
> > + td -= 1;
>
> td--;
Fixed.
> > +
> > + val32 = ioread32(base + BK3710_DMASTB) & (0xFF << (dev ? 0 : 8));
> > + val32 |= (td << (dev ? 8 : 0));
>
> And here...
ditto
> > + iowrite32(val32, base + BK3710_DMASTB);
> > +
> > + val32 = ioread32(base + BK3710_DMARCVR) & (0xFF << (dev ? 0 : 8));
> > + val32 |= (tkw << (dev ? 8 : 0));
>
> And here...
ditto
> [...]
> > +static void pata_bk3710_setpiomode(void __iomem *base, struct ata_device *pair,
> > + unsigned int dev, unsigned int cycletime,
> > + unsigned int mode)
> > +{
> > + const struct ata_timing *t;
> > + u32 val32;
> > + u8 t2, t2i, t0;
> > +
> > + t = ata_timing_find_mode(XFER_PIO_0 + mode);
> > +
> > + /* PIO Data Setup */
> > + t0 = DIV_ROUND_UP(cycletime, ideclk_period);
> > + t2 = DIV_ROUND_UP(t->active, ideclk_period);
> > +
> > + t2i = t0 - t2 - 1;
> > + t2 -= 1;
>
> t2--;
ditto
> > +
> > + val32 = ioread32(base + BK3710_DATSTB) & (0xFF << (dev ? 0 : 8));
> > + val32 |= (t2 << (dev ? 8 : 0));
>
> Outer parens not needed.
ditto
> > + iowrite32(val32, base + BK3710_DATSTB);
> > +
> > + val32 = ioread32(base + BK3710_DATRCVR) & (0xFF << (dev ? 0 : 8));
> > + val32 |= (t2i << (dev ? 8 : 0));
>
> Here too..
ditto
> > + iowrite32(val32, base + BK3710_DATRCVR);
> > +
> > + /* FIXME: this is broken also in the old driver */
>
> What's wrong with this logic BTW?
Two things:
- it happens too late, after "mode" variable has been read
- we should probably just merge timings instead of downgrading PIO mode
> > + if (pair) {
> > + u8 mode2 = pair->pio_mode - XFER_PIO_0;
> > +
> > + if (mode2 < mode)
> > + mode = mode2;
> > + }
> > +
> > + /* TASKFILE Setup */
> > + t0 = DIV_ROUND_UP(t->cyc8b, ideclk_period);
> > + t2 = DIV_ROUND_UP(t->act8b, ideclk_period);
> > +
> > + t2i = t0 - t2 - 1;
> > + t2 -= 1;
>
> t2--;
Fixed.
> > +
> > + val32 = ioread32(base + BK3710_REGSTB) & (0xFF << (dev ? 0 : 8));
> > + val32 |= (t2 << (dev ? 8 : 0));
>
> Outer parens again...
ditto
> > + iowrite32(val32, base + BK3710_REGSTB);
> > +
> > + val32 = ioread32(base + BK3710_REGRCVR) & (0xFF << (dev ? 0 : 8));
> > + val32 |= (t2i << (dev ? 8 : 0));
>
> And again...
ditto
> > + iowrite32(val32, base + BK3710_REGRCVR);
> > +}
> > +
> > +static void pata_bk3710_set_piomode(struct ata_port *ap,
> > + struct ata_device *adev)
> > +{
> > + void __iomem *base = (void __iomem *)ap->ioaddr.bmdma_addr;
> > + struct ata_device *pair = ata_dev_pair(adev);
> > + const struct ata_timing *t = ata_timing_find_mode(adev->pio_mode);
> > + const u16 *id = adev->id;
> > + unsigned int cycle_time;
> > + int is_slave = adev->devno;
> > + const u8 pio = adev->pio_mode - XFER_PIO_0;
> > +
> > + if (id[ATA_ID_FIELD_VALID] & 2) {
> > + if (ata_id_has_iordy(id))
> > + cycle_time = id[ATA_ID_EIDE_PIO_IORDY];
> > + else
> > + cycle_time = id[ATA_ID_EIDE_PIO];
> > +
> > + /* conservative "downgrade" for all pre-ATA2 drives */
> > + if (pio < 3 && cycle_time < t->cycle)
> > + cycle_time = 0; /* use standard timing */
> > + }
> > +
> > + if (!cycle_time)
> > + cycle_time = t->cycle;
>
> This seems like a helper needed by libata in general but OK...
I need to think about this a bit more..
[...]
> > +static int __init pata_bk3710_probe(struct platform_device *pdev)
> > +{
> > + struct clk *clk;
> > + struct resource *mem, *irq;
> > + struct ata_host *host;
> > + struct ata_port *ap;
> > + void __iomem *base;
> > + unsigned long rate, mem_size;
> > +
> > + clk = clk_get(&pdev->dev, NULL);
>
> devm_clk_get()?
Fixed.
> > + if (IS_ERR(clk))
> > + return -ENODEV;
> > +
> > + clk_enable(clk);
> > + rate = clk_get_rate(clk);
> > + if (!rate)
> > + return -EINVAL;
> > +
> > + /* NOTE: round *down* to meet minimum timings; we count in clocks */
> > + ideclk_period = 1000000000UL / rate;
> > +
> > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (mem == NULL) {
> > + pr_err(DRV_NAME ": failed to get memory region resource\n");
> > + return -ENODEV;
> > + }
> > +
> > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> How about platform_get_irq()? I've fixed it... :-)
Converted.
> > + if (irq == NULL) {
> > + pr_err(DRV_NAME ": failed to get IRQ resource\n");
> > + return -ENODEV;
> > + }
> > +
> > + mem_size = resource_size(mem);
> > + if (!devm_request_mem_region(&pdev->dev, mem->start, mem_size,
> > + DRV_NAME)) {
> > + pr_err(DRV_NAME ": failed to request memory region\n");
> > + return -EBUSY;
> > + }
> > +
> > + base = ioremap(mem->start, mem_size);
>
> How about devm_ioremap_resource() instead of the above?
ditto
> > + if (!base) {
> > + pr_err(DRV_NAME ": failed to map IO memory\n");
> > + return -ENOMEM;
> > + }
> > +
> > + /* Configure the Palm Chip controller */
>
> It's Palmchip. :-)
Fixed.
[...]
Thanks for review!
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
More information about the linux-arm-kernel
mailing list