* [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
@ 2017-03-09 13:01 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 25+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2017-03-09 13:01 UTC (permalink / raw)
To: linux-arm-kernel
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.
+
+ 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");
--
1.9.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
@ 2017-03-09 13:01 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 25+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2017-03-09 13:01 UTC (permalink / raw)
To: Tejun Heo
Cc: Sekhar Nori, Sergei Shtylyov, Kevin Hilman, Arnd Bergmann,
Russell King, Dmitry Eremin-Solenikov, linux-ide,
linux-arm-kernel, linux-kernel, b.zolnierkie
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.
+
+ 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");
--
1.9.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
2017-03-09 13:01 ` Bartlomiej Zolnierkiewicz
(?)
@ 2017-03-09 17:05 ` Sergei Shtylyov
-1 siblings, 0 replies; 25+ messages in thread
From: Sergei Shtylyov @ 2017-03-09 17:05 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Tejun Heo
Cc: Russell King, Arnd Bergmann, Dmitry Eremin-Solenikov,
Kevin Hilman, Sekhar Nori, linux-kernel, linux-ide,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
@ 2017-03-09 17:05 ` Sergei Shtylyov
0 siblings, 0 replies; 25+ messages in thread
From: Sergei Shtylyov @ 2017-03-09 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
@ 2017-03-09 17:05 ` Sergei Shtylyov
0 siblings, 0 replies; 25+ messages in thread
From: Sergei Shtylyov @ 2017-03-09 17:05 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Tejun Heo
Cc: Sekhar Nori, Kevin Hilman, Arnd Bergmann, Russell King,
Dmitry Eremin-Solenikov, linux-ide, linux-arm-kernel,
linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
2017-03-09 17:05 ` Sergei Shtylyov
@ 2017-03-09 20:45 ` Sergei Shtylyov
-1 siblings, 0 replies; 25+ messages in thread
From: Sergei Shtylyov @ 2017-03-09 20:45 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Tejun Heo
Cc: Sekhar Nori, Kevin Hilman, Arnd Bergmann, Russell King,
Dmitry Eremin-Solenikov, linux-ide, linux-arm-kernel,
linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
@ 2017-03-09 20:45 ` Sergei Shtylyov
0 siblings, 0 replies; 25+ messages in thread
From: Sergei Shtylyov @ 2017-03-09 20:45 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
2017-03-09 13:01 ` Bartlomiej Zolnierkiewicz
(?)
@ 2017-03-11 5:07 ` kbuild test robot
-1 siblings, 0 replies; 25+ messages in thread
From: kbuild test robot @ 2017-03-11 5:07 UTC (permalink / raw)
Cc: kbuild-all, Tejun Heo, Sekhar Nori, Sergei Shtylyov,
Kevin Hilman, Arnd Bergmann, Russell King,
Dmitry Eremin-Solenikov, linux-ide, linux-arm-kernel,
linux-kernel, b.zolnierkie
[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24160 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
@ 2017-03-11 5:07 ` kbuild test robot
0 siblings, 0 replies; 25+ messages in thread
From: kbuild test robot @ 2017-03-11 5:07 UTC (permalink / raw)
To: linux-arm-kernel
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 24160 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170311/76bb192a/attachment-0001.gz>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
@ 2017-03-11 5:07 ` kbuild test robot
0 siblings, 0 replies; 25+ messages in thread
From: kbuild test robot @ 2017-03-11 5:07 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: kbuild-all, Tejun Heo, Sekhar Nori, Sergei Shtylyov,
Kevin Hilman, Arnd Bergmann, Russell King,
Dmitry Eremin-Solenikov, linux-ide, linux-arm-kernel,
linux-kernel, b.zolnierkie
[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24160 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
2017-03-09 13:01 ` Bartlomiej Zolnierkiewicz
@ 2017-03-12 17:28 ` Sergei Shtylyov
-1 siblings, 0 replies; 25+ messages in thread
From: Sergei Shtylyov @ 2017-03-12 17:28 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Tejun Heo
Cc: Sekhar Nori, Kevin Hilman, Arnd Bergmann, Russell King,
Dmitry Eremin-Solenikov, linux-ide, linux-arm-kernel,
linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
@ 2017-03-12 17:28 ` Sergei Shtylyov
0 siblings, 0 replies; 25+ messages in thread
From: Sergei Shtylyov @ 2017-03-12 17:28 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
2017-03-12 17:28 ` Sergei Shtylyov
@ 2017-03-12 17:53 ` Sergei Shtylyov
-1 siblings, 0 replies; 25+ messages in thread
From: Sergei Shtylyov @ 2017-03-12 17:53 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Tejun Heo
Cc: Sekhar Nori, Kevin Hilman, Arnd Bergmann, Russell King,
Dmitry Eremin-Solenikov, linux-ide, linux-arm-kernel,
linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
@ 2017-03-12 17:53 ` Sergei Shtylyov
0 siblings, 0 replies; 25+ messages in thread
From: Sergei Shtylyov @ 2017-03-12 17:53 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <CGME20170314163607epcas1p42caadae15d32c0179f43df08d8a5224a@epcas1p4.samsung.com>]
* Re: [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
[not found] ` <CGME20170314163607epcas1p42caadae15d32c0179f43df08d8a5224a@epcas1p4.samsung.com>
@ 2017-03-14 16:36 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 25+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2017-03-14 16:36 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Tejun Heo, Sekhar Nori, Kevin Hilman, Arnd Bergmann,
Russell King, Dmitry Eremin-Solenikov, linux-ide,
linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 1/3] ata: add Palmchip BK3710 PATA controller driver
@ 2017-03-14 16:36 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 25+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2017-03-14 16:36 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread