| Message ID | 1489064496-6270-2-git-send-email-b.zolnierkie@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/09/2017 04:01 PM, Bartlomiej Zolnierkiewicz wrote: > Add Palmchip BK3710 PATA controller driver. > > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > --- > drivers/ata/Kconfig | 9 ++ > drivers/ata/Makefile | 1 + > drivers/ata/pata_bk3710.c | 395 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 405 insertions(+) > create mode 100644 drivers/ata/pata_bk3710.c > > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig > index 7e0fc98..c23ee65 100644 > --- a/drivers/ata/Kconfig > +++ b/drivers/ata/Kconfig > @@ -509,6 +509,15 @@ config PATA_BF54X > > If unsure, say N. > > +config PATA_BK3710 > + tristate "Palmchip BK3710 PATA support" > + depends on ARCH_DAVINCI > + help > + This option enables support for the integrated IDE controller on > + the TI DaVinci SoC. Strictly speaking, Palmchip BK3710 is Mentor Graphics' IDE core, used on the original DaVinci's. Mentor's documentation was never publicly available though... [...] MBR, Sergei
On 03/09/2017 08:05 PM, Sergei Shtylyov wrote: >> Add Palmchip BK3710 PATA controller driver. >> >> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> --- >> drivers/ata/Kconfig | 9 ++ >> drivers/ata/Makefile | 1 + >> drivers/ata/pata_bk3710.c | 395 ++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 405 insertions(+) >> create mode 100644 drivers/ata/pata_bk3710.c >> >> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig >> index 7e0fc98..c23ee65 100644 >> --- a/drivers/ata/Kconfig >> +++ b/drivers/ata/Kconfig >> @@ -509,6 +509,15 @@ config PATA_BF54X >> >> If unsure, say N. >> >> +config PATA_BK3710 >> + tristate "Palmchip BK3710 PATA support" >> + depends on ARCH_DAVINCI >> + help >> + This option enables support for the integrated IDE controller on >> + the TI DaVinci SoC. > > Strictly speaking, Palmchip BK3710 is Mentor Graphics' IDE core, used on > the original DaVinci's. Mentor's documentation was never publicly available > though... From googling I knew that Palmchip was a company Mentor probably bought... > [...] I'll try to review the whole driver on weekend. MBR, Sergei
Hi Bartlomiej,
[auto build test WARNING on tj-libata/for-next]
[also build test WARNING on v4.11-rc1 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Bartlomiej-Zolnierkiewicz/ata-add-Palmchip-BK3710-PATA-controller-driver/20170311-095152
base: https://git.kernel.org/pub/scm/linux/kernel/git/tj/libata.git for-next
config: arm-davinci_all_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
drivers/ata/pata_bk3710.c: In function 'pata_bk3710_set_piomode':
>> drivers/ata/pata_bk3710.c:223:5: warning: 'cycle_time' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (!cycle_time)
^
vim +/cycle_time +223 drivers/ata/pata_bk3710.c
207 const u16 *id = adev->id;
208 unsigned int cycle_time;
209 int is_slave = adev->devno;
210 const u8 pio = adev->pio_mode - XFER_PIO_0;
211
212 if (id[ATA_ID_FIELD_VALID] & 2) {
213 if (ata_id_has_iordy(id))
214 cycle_time = id[ATA_ID_EIDE_PIO_IORDY];
215 else
216 cycle_time = id[ATA_ID_EIDE_PIO];
217
218 /* conservative "downgrade" for all pre-ATA2 drives */
219 if (pio < 3 && cycle_time < t->cycle)
220 cycle_time = 0; /* use standard timing */
221 }
222
> 223 if (!cycle_time)
224 cycle_time = t->cycle;
225
226 pata_bk3710_setpiomode(base, pair, is_slave, cycle_time, pio);
227 }
228
229 static void pata_bk3710_chipinit(void __iomem *base)
230 {
231 /*
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hello! On 03/09/2017 04:01 PM, Bartlomiej Zolnierkiewicz wrote: > Add Palmchip BK3710 PATA controller driver. > > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@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@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... > + > +#define DRV_NAME "pata_bk3710" > +#define DRV_VERSION "0.1.0" This macro isn't used anywhere, do we really need it? > + > +#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)... > +#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... [...] > +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. :-) > + val32 |= (t0 << (dev ? 8 : 0)); Outer parens not really needed. > + 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.. > + 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... [...] > +static void pata_bk3710_setdmamode(void __iomem *base, unsigned int dev, Maybe setmwdmamode()? > + 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--; > + > + val32 = ioread32(base + BK3710_DMASTB) & (0xFF << (dev ? 0 : 8)); > + val32 |= (td << (dev ? 8 : 0)); And here... > + iowrite32(val32, base + BK3710_DMASTB); > + > + val32 = ioread32(base + BK3710_DMARCVR) & (0xFF << (dev ? 0 : 8)); > + val32 |= (tkw << (dev ? 8 : 0)); And here... [...] > +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--; > + > + val32 = ioread32(base + BK3710_DATSTB) & (0xFF << (dev ? 0 : 8)); > + val32 |= (t2 << (dev ? 8 : 0)); Outer parens not needed. > + iowrite32(val32, base + BK3710_DATSTB); > + > + val32 = ioread32(base + BK3710_DATRCVR) & (0xFF << (dev ? 0 : 8)); > + val32 |= (t2i << (dev ? 8 : 0)); Here too.. > + iowrite32(val32, base + BK3710_DATRCVR); > + > + /* FIXME: this is broken also in the old driver */ What's wrong with this logic BTW? > + 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--; > + > + val32 = ioread32(base + BK3710_REGSTB) & (0xFF << (dev ? 0 : 8)); > + val32 |= (t2 << (dev ? 8 : 0)); Outer parens again... > + iowrite32(val32, base + BK3710_REGSTB); > + > + val32 = ioread32(base + BK3710_REGRCVR) & (0xFF << (dev ? 0 : 8)); > + val32 |= (t2i << (dev ? 8 : 0)); And again... > + 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... > + > + pata_bk3710_setpiomode(base, pair, is_slave, cycle_time, pio); > +} > + > +static void pata_bk3710_chipinit(void __iomem *base) > +{ > + /* > + * REVISIT: the ATA reset signal needs to be managed through a > + * GPIO, which means it should come from platform_data. Until > + * we get and use such information, we have to trust that things > + * have been reset before we get here. > + */ > + > + /* > + * Program the IDETIMP Register Value based on the following assumptions > + * > + * (ATA_IDETIMP_IDEEN , ENABLE ) | > + * (ATA_IDETIMP_PREPOST1 , DISABLE) | > + * (ATA_IDETIMP_PREPOST0 , DISABLE) | > + * > + * DM6446 silicon rev 2.1 and earlier have no observed net benefit > + * from enabling prefetch/postwrite. > + */ > + iowrite16(BIT(15), base + BK3710_IDETIMP); The bit 15 is called IDEEN indeed... [...] > + /* > + * MISCCTL Miscellaneous Conrol Register > + * (ATA_MISCCTL_HWNHLD1P , 1 cycle) > + * (ATA_MISCCTL_HWNHLD0P , 1 cycle) > + * (ATA_MISCCTL_TIMORIDE , 1) > + */ > + iowrite32(0x001, base + BK3710_MISCCTL); Named TIMORIDE indeed; bits 1-15 reserved... > + > + /* > + * IORDYTMP IORDY Timer for Primary Register > + * (ATA_IORDYTMP_IORDYTMP , 0xffff ) > + */ > + iowrite32(0xFFFF, base + BK3710_IORDYTMP); We don't seem to handle the IORDY timeout interrupt, hence setting the IORDY timeout doesn't seem useful... > + > + /* > + * Configure BMISP Register > + * (ATA_BMISP_DMAEN1 , DISABLE ) | > + * (ATA_BMISP_DMAEN0 , DISABLE ) | These bits are documented as reserved anyway. > + * (ATA_BMISP_IORDYINT , CLEAR) | > + * (ATA_BMISP_INTRSTAT , CLEAR) | > + * (ATA_BMISP_DMAERROR , CLEAR) They forgot bit 0, IDEACT. :-) > + */ > + iowrite16(0, base + BK3710_BMISP); > + > + pata_bk3710_setpiomode(base, NULL, 0, 600, 0); > + pata_bk3710_setpiomode(base, NULL, 1, 600, 0); > +} > + > +static struct ata_port_operations pata_bk3710_ports_ops = { > + .inherits = &ata_bmdma_port_ops, Strictly speaking, the BMIDE control/status registers are 16-bit but they probably are OK with 8-bit accesses as well... [...] > +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()? > + 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... :-) > + 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? > + if (!base) { > + pr_err(DRV_NAME ": failed to map IO memory\n"); > + return -ENOMEM; > + } > + > + /* Configure the Palm Chip controller */ It's Palmchip. :-) [...] Well, no serious issues found, so you can stamp: Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> MBR, Sergei
On 03/12/2017 08:28 PM, Sergei Shtylyov wrote: >> +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... No, the build bot had already reported the problem with 'cycle_time' here, I just forgot about it. [...] > > Well, no serious issues found, so you can stamp: > > Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Too early, it seems... :-) MBR, Sergei
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@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@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
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index 7e0fc98..c23ee65 100644 --- a/drivers/ata/Kconfig +++ b/drivers/ata/Kconfig @@ -509,6 +509,15 @@ config PATA_BF54X If unsure, say N. +config PATA_BK3710 + tristate "Palmchip BK3710 PATA support" + depends on ARCH_DAVINCI + help + This option enables support for the integrated IDE controller on + the TI DaVinci SoC. + + If unsure, say N. + config PATA_CMD64X tristate "CMD64x PATA support" depends on PCI diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile index 89a0a19..9438db8 100644 --- a/drivers/ata/Makefile +++ b/drivers/ata/Makefile @@ -50,6 +50,7 @@ obj-$(CONFIG_PATA_ARTOP) += pata_artop.o obj-$(CONFIG_PATA_ATIIXP) += pata_atiixp.o obj-$(CONFIG_PATA_ATP867X) += pata_atp867x.o obj-$(CONFIG_PATA_BF54X) += pata_bf54x.o +obj-$(CONFIG_PATA_BK3710) += pata_bk3710.o obj-$(CONFIG_PATA_CMD64X) += pata_cmd64x.o obj-$(CONFIG_PATA_CS5520) += pata_cs5520.o obj-$(CONFIG_PATA_CS5530) += pata_cs5530.o 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@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> + +#define DRV_NAME "pata_bk3710" +#define DRV_VERSION "0.1.0" + +#define BK3710_REG_OFFSET 0x1F0 +#define BK3710_CTL_OFFSET 0x3F6 + +#define BK3710_BMISP 0x02 +#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 + +static struct scsi_host_template pata_bk3710_sht = { + ATA_BMDMA_SHT(DRV_NAME), +}; + +static unsigned int ideclk_period; /* in nanoseconds */ + +struct pata_bk3710_udmatiming { + unsigned int rptime; /* tRP -- Ready to pause time (nsec) */ + unsigned int cycletime; /* tCYCTYP2/2 -- avg Cycle Time (nsec) */ + /* tENV is always a minimum of 20 nsec */ +}; + +static const struct pata_bk3710_udmatiming pata_bk3710_udmatimings[6] = { + { 160, 240 / 2 }, /* UDMA Mode 0 */ + { 125, 160 / 2 }, /* UDMA Mode 1 */ + { 100, 120 / 2 }, /* UDMA Mode 2 */ + { 100, 90 / 2 }, /* UDMA Mode 3 */ + { 100, 60 / 2 }, /* UDMA Mode 4 */ + { 85, 40 / 2 }, /* UDMA Mode 5 */ +}; + +static void pata_bk3710_setudmamode(void __iomem *base, unsigned int dev, + unsigned int mode) +{ + u32 val32; + u16 val16; + u8 tenv, trp, t0; + + /* 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)); + val32 |= (t0 << (dev ? 8 : 0)); + 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)); + iowrite32(val32, base + BK3710_UDMATRP); + + /* udmaenv Ultra DMA envelop Time */ + val32 = ioread32(base + BK3710_UDMAENV) & (0xFF << (dev ? 0 : 8)); + val32 |= (tenv << (dev ? 8 : 0)); + iowrite32(val32, base + BK3710_UDMAENV); + + /* Enable UDMA for Device */ + val16 = ioread16(base + BK3710_UDMACTL) | (1 << dev); + iowrite16(val16, base + BK3710_UDMACTL); +} + +static void pata_bk3710_setdmamode(void __iomem *base, unsigned int dev, + 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; + + val32 = ioread32(base + BK3710_DMASTB) & (0xFF << (dev ? 0 : 8)); + val32 |= (td << (dev ? 8 : 0)); + iowrite32(val32, base + BK3710_DMASTB); + + val32 = ioread32(base + BK3710_DMARCVR) & (0xFF << (dev ? 0 : 8)); + val32 |= (tkw << (dev ? 8 : 0)); + iowrite32(val32, base + BK3710_DMARCVR); + + /* Disable UDMA for Device */ + val16 = ioread16(base + BK3710_UDMACTL) & ~(1 << dev); + iowrite16(val16, base + BK3710_UDMACTL); +} + +static void pata_bk3710_set_dmamode(struct ata_port *ap, + struct ata_device *adev) +{ + void __iomem *base = (void __iomem *)ap->ioaddr.bmdma_addr; + int is_slave = adev->devno; + const u8 xferspeed = adev->dma_mode; + + if (xferspeed >= XFER_UDMA_0) + pata_bk3710_setudmamode(base, is_slave, + xferspeed - XFER_UDMA_0); + else + pata_bk3710_setdmamode(base, is_slave, + adev->id[ATA_ID_EIDE_DMA_MIN], + xferspeed); +} + +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; + + val32 = ioread32(base + BK3710_DATSTB) & (0xFF << (dev ? 0 : 8)); + val32 |= (t2 << (dev ? 8 : 0)); + iowrite32(val32, base + BK3710_DATSTB); + + val32 = ioread32(base + BK3710_DATRCVR) & (0xFF << (dev ? 0 : 8)); + val32 |= (t2i << (dev ? 8 : 0)); + iowrite32(val32, base + BK3710_DATRCVR); + + /* FIXME: this is broken also in the old driver */ + 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; + + val32 = ioread32(base + BK3710_REGSTB) & (0xFF << (dev ? 0 : 8)); + val32 |= (t2 << (dev ? 8 : 0)); + iowrite32(val32, base + BK3710_REGSTB); + + val32 = ioread32(base + BK3710_REGRCVR) & (0xFF << (dev ? 0 : 8)); + val32 |= (t2i << (dev ? 8 : 0)); + 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; + + pata_bk3710_setpiomode(base, pair, is_slave, cycle_time, pio); +} + +static void pata_bk3710_chipinit(void __iomem *base) +{ + /* + * REVISIT: the ATA reset signal needs to be managed through a + * GPIO, which means it should come from platform_data. Until + * we get and use such information, we have to trust that things + * have been reset before we get here. + */ + + /* + * Program the IDETIMP Register Value based on the following assumptions + * + * (ATA_IDETIMP_IDEEN , ENABLE ) | + * (ATA_IDETIMP_PREPOST1 , DISABLE) | + * (ATA_IDETIMP_PREPOST0 , DISABLE) | + * + * DM6446 silicon rev 2.1 and earlier have no observed net benefit + * from enabling prefetch/postwrite. + */ + iowrite16(BIT(15), base + BK3710_IDETIMP); + + /* + * UDMACTL Ultra-ATA DMA Control + * (ATA_UDMACTL_UDMAP1 , 0 ) | + * (ATA_UDMACTL_UDMAP0 , 0 ) + * + */ + iowrite16(0, base + BK3710_UDMACTL); + + /* + * MISCCTL Miscellaneous Conrol Register + * (ATA_MISCCTL_HWNHLD1P , 1 cycle) + * (ATA_MISCCTL_HWNHLD0P , 1 cycle) + * (ATA_MISCCTL_TIMORIDE , 1) + */ + iowrite32(0x001, base + BK3710_MISCCTL); + + /* + * IORDYTMP IORDY Timer for Primary Register + * (ATA_IORDYTMP_IORDYTMP , 0xffff ) + */ + iowrite32(0xFFFF, base + BK3710_IORDYTMP); + + /* + * Configure BMISP Register + * (ATA_BMISP_DMAEN1 , DISABLE ) | + * (ATA_BMISP_DMAEN0 , DISABLE ) | + * (ATA_BMISP_IORDYINT , CLEAR) | + * (ATA_BMISP_INTRSTAT , CLEAR) | + * (ATA_BMISP_DMAERROR , CLEAR) + */ + iowrite16(0, base + BK3710_BMISP); + + pata_bk3710_setpiomode(base, NULL, 0, 600, 0); + pata_bk3710_setpiomode(base, NULL, 1, 600, 0); +} + +static struct ata_port_operations pata_bk3710_ports_ops = { + .inherits = &ata_bmdma_port_ops, + .cable_detect = ata_cable_80wire, + + .set_piomode = pata_bk3710_set_piomode, + .set_dmamode = pata_bk3710_set_dmamode, +}; + +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); + 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); + 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); + if (!base) { + pr_err(DRV_NAME ": failed to map IO memory\n"); + return -ENOMEM; + } + + /* Configure the Palm Chip controller */ + pata_bk3710_chipinit(base); + + /* allocate host */ + host = ata_host_alloc(&pdev->dev, 1); + if (!host) + return -ENOMEM; + ap = host->ports[0]; + + ap->ops = &pata_bk3710_ports_ops; + ap->pio_mask = ATA_PIO4; + ap->mwdma_mask = ATA_MWDMA2; + ap->udma_mask = rate < 100000000 ? ATA_UDMA4 : ATA_UDMA5; + ap->flags |= ATA_FLAG_SLAVE_POSS; + + ap->ioaddr.data_addr = base + BK3710_REG_OFFSET; + ap->ioaddr.error_addr = base + BK3710_REG_OFFSET + 1; + ap->ioaddr.feature_addr = base + BK3710_REG_OFFSET + 1; + ap->ioaddr.nsect_addr = base + BK3710_REG_OFFSET + 2; + ap->ioaddr.lbal_addr = base + BK3710_REG_OFFSET + 3; + ap->ioaddr.lbam_addr = base + BK3710_REG_OFFSET + 4; + ap->ioaddr.lbah_addr = base + BK3710_REG_OFFSET + 5; + ap->ioaddr.device_addr = base + BK3710_REG_OFFSET + 6; + ap->ioaddr.status_addr = base + BK3710_REG_OFFSET + 7; + ap->ioaddr.command_addr = base + BK3710_REG_OFFSET + 7; + + ap->ioaddr.altstatus_addr = base + BK3710_CTL_OFFSET; + ap->ioaddr.ctl_addr = base + BK3710_CTL_OFFSET; + + ap->ioaddr.bmdma_addr = base; + + ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", + (unsigned long)base + BK3710_REG_OFFSET, + (unsigned long)base + BK3710_CTL_OFFSET); + + /* activate */ + return ata_host_activate(host, irq->start, ata_sff_interrupt, 0, + &pata_bk3710_sht); +} + +/* work with hotplug and coldplug */ +MODULE_ALIAS("platform:palm_bk3710"); + +static struct platform_driver pata_bk3710_driver = { + .driver = { + .name = "palm_bk3710", + }, +}; + +static int __init pata_bk3710_init(void) +{ + return platform_driver_probe(&pata_bk3710_driver, pata_bk3710_probe); +} + +module_init(pata_bk3710_init); +MODULE_LICENSE("GPL");
Add Palmchip BK3710 PATA controller driver. Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> --- drivers/ata/Kconfig | 9 ++ drivers/ata/Makefile | 1 + drivers/ata/pata_bk3710.c | 395 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 405 insertions(+) create mode 100644 drivers/ata/pata_bk3710.c