[PATCH v2 0/3] ATA/ARM: convert ARM/DaVinci to use libata PATA drivers

[PATCH v2 0/3] ATA/ARM: convert ARM/DaVinci to use libata PATA drivers

[Bartlomiej Zolnierkiewicz]

Hi,

This patchset adds Palmchip BK3710 IDE controller driver to
libata and switches ARM/DaVinci to use it (instead of the old
IDE driver).

Sekhar, please check that it still works after changes, thanks.

Changes since v1
(https://www.spinics.net/lists/arm-kernel/msg567442.html):
- addressed review comments from Sergei Shtylyov
- fixed cycle_time unitialized variable issue

Changes since v0.1 draft patch version
(https://www.spinics.net/lists/arm-kernel/msg566932.html):
- fixed cycle_time build warning
- added platform support fixes from Sekhar
- added defconfig changes from Sekhar
- preserved platform support for the old IDE driver
- split it on 3 patches

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics


Bartlomiej Zolnierkiewicz (1):
  ata: add Palmchip BK3710 PATA controller driver

Sekhar Nori (2):
  ARM: davinci: add pata_bk3710 libata driver support
  ARM: davinci_all_defconfig: convert to use libata PATA

 arch/arm/configs/davinci_all_defconfig    |   4 +-
 arch/arm/mach-davinci/board-dm644x-evm.c  |   3 +-
 arch/arm/mach-davinci/board-dm646x-evm.c  |   3 +-
 arch/arm/mach-davinci/board-neuros-osd2.c |   3 +-
 drivers/ata/Kconfig                       |   9 +
 drivers/ata/Makefile                      |   1 +
 drivers/ata/pata_bk3710.c                 | 386 ++++++++++++++++++++++++++++++
 7 files changed, 403 insertions(+), 6 deletions(-)
 create mode 100644 drivers/ata/pata_bk3710.c

-- 
1.9.1

[PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver

[Bartlomiej Zolnierkiewicz]

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 | 386 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 396 insertions(+)
 create mode 100644 drivers/ata/pata_bk3710.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 70b57d2..38fa4ac 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -510,6 +510,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..6d77217
--- /dev/null
+++ b/drivers/ata/pata_bk3710.c
@@ -0,0 +1,386 @@
+/*
+ * 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/ata.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/libata.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#define DRV_NAME "pata_bk3710"
+
+#define BK3710_TF_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_setmwdmamode(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--;
+
+	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_setmwdmamode(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--;
+
+	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--;
+
+	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 = 0;
+	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;
+	struct ata_host *host;
+	struct ata_port *ap;
+	void __iomem *base;
+	unsigned long rate;
+	int irq;
+
+	clk = devm_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_irq(pdev, 0);
+	if (irq < 0) {
+		pr_err(DRV_NAME ": failed to get IRQ resource\n");
+		return irq;
+	}
+
+	base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	/* configure the Palmchip 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_TF_OFFSET;
+	ap->ioaddr.error_addr		= base + BK3710_TF_OFFSET + 1;
+	ap->ioaddr.feature_addr		= base + BK3710_TF_OFFSET + 1;
+	ap->ioaddr.nsect_addr		= base + BK3710_TF_OFFSET + 2;
+	ap->ioaddr.lbal_addr		= base + BK3710_TF_OFFSET + 3;
+	ap->ioaddr.lbam_addr		= base + BK3710_TF_OFFSET + 4;
+	ap->ioaddr.lbah_addr		= base + BK3710_TF_OFFSET + 5;
+	ap->ioaddr.device_addr		= base + BK3710_TF_OFFSET + 6;
+	ap->ioaddr.status_addr		= base + BK3710_TF_OFFSET + 7;
+	ap->ioaddr.command_addr		= base + BK3710_TF_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_TF_OFFSET,
+		      (unsigned long)base + BK3710_CTL_OFFSET);
+
+	/* activate */
+	return ata_host_activate(host, irq, 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

Re: [PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver

[Sergei Shtylyov]

Hello!

On 3/14/2017 7:36 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..6d77217
> --- /dev/null
> +++ b/drivers/ata/pata_bk3710.c
> @@ -0,0 +1,386 @@
[...]
> +static void pata_bk3710_chipinit(void __iomem *base)
> +{
[...]
> +	/*
> +	 * IORDYTMP IORDY Timer for Primary Register
> +	 * (ATA_IORDYTMP_IORDYTMP     , 0xffff  )
> +	 */
> +	iowrite32(0xFFFF, base + BK3710_IORDYTMP);

    As I've already said, this is useless as we don't handle the IORDY timeout 
interrupt anyway; writing 0 would be fine.

> +
> +	/*
> +	 * 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);

    Bits 0-3 cane only be cleared by writing 1, so this write can't clear 
them, contrary to what the comment says. Might be a material for a follow-up 
patch tho...

[...]
> +static int __init pata_bk3710_probe(struct platform_device *pdev)
> +{
> +	struct clk *clk;
> +	struct resource *mem;
> +	struct ata_host *host;
> +	struct ata_port *ap;
> +	void __iomem *base;
> +	unsigned long rate;
> +	int irq;
> +
> +	clk = devm_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;
> +	}

    NULL check not needed here, devm_ioremap_resource() checks this anyway.

> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		pr_err(DRV_NAME ": failed to get IRQ resource\n");
> +		return irq;
> +	}
> +
> +	base = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
[...]
> +/* work with hotplug and coldplug */
> +MODULE_ALIAS("platform:palm_bk3710");
> +
> +static struct platform_driver pata_bk3710_driver = {
> +	.driver = {
> +		.name = "palm_bk3710",

    Not DRV_NAME?

[...]

MBR, Sergei

Re: [PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver

[Bartlomiej Zolnierkiewicz]

Hi,

On Saturday, March 18, 2017 04:52:18 PM Sergei Shtylyov wrote:
> Hello!
> 
> On 3/14/2017 7:36 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..6d77217
> > --- /dev/null
> > +++ b/drivers/ata/pata_bk3710.c
> > @@ -0,0 +1,386 @@
> [...]
> > +static void pata_bk3710_chipinit(void __iomem *base)
> > +{
> [...]
> > +	/*
> > +	 * IORDYTMP IORDY Timer for Primary Register
> > +	 * (ATA_IORDYTMP_IORDYTMP     , 0xffff  )
> > +	 */
> > +	iowrite32(0xFFFF, base + BK3710_IORDYTMP);
> 
>     As I've already said, this is useless as we don't handle the IORDY timeout 
> interrupt anyway; writing 0 would be fine.

Will fix in v3, in the incremental patch (so it is easier to revert
if it turns out to cause problems later or port to palm_bk3710).

> > +
> > +	/*
> > +	 * 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);
> 
>     Bits 0-3 cane only be cleared by writing 1, so this write can't clear 

The documentation does say this about bits 1-3, bit 0 is handled in
a different way.

> them, contrary to what the comment says. Might be a material for a follow-up 
> patch tho...

Will fix in the incremental patch in v3.

> [...]
> > +static int __init pata_bk3710_probe(struct platform_device *pdev)
> > +{
> > +	struct clk *clk;
> > +	struct resource *mem;
> > +	struct ata_host *host;
> > +	struct ata_port *ap;
> > +	void __iomem *base;
> > +	unsigned long rate;
> > +	int irq;
> > +
> > +	clk = devm_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;
> > +	}
> 
>     NULL check not needed here, devm_ioremap_resource() checks this anyway.

Will be fixed in v3.

> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		pr_err(DRV_NAME ": failed to get IRQ resource\n");
> > +		return irq;
> > +	}
> > +
> > +	base = devm_ioremap_resource(&pdev->dev, mem);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> [...]
> > +/* work with hotplug and coldplug */
> > +MODULE_ALIAS("platform:palm_bk3710");
> > +
> > +static struct platform_driver pata_bk3710_driver = {
> > +	.driver = {
> > +		.name = "palm_bk3710",
> 
>     Not DRV_NAME?

DRV_NAME is "pata_bk3710" and the platform driver name needs to
match the old driver name for compatibility reasons (supporting
both drivers by the arch specific code).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver

[Sekhar Nori]

Hi,

On Tuesday 14 March 2017 10:06 PM, Bartlomiej Zolnierkiewicz wrote:
> Add Palmchip BK3710 PATA controller driver.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

There are some checkpatch suggestions that are worth fixing, I think.

[PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:277: CHECK: Alignment should match open parenthesis
[PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:320: CHECK: Alignment should match open parenthesis
[PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:489: CHECK: Comparison to NULL could be written "!mem"

Thanks,
Sekhar

Re: [PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, March 21, 2017 02:04:38 PM Sekhar Nori wrote:
> Hi,
> 
> On Tuesday 14 March 2017 10:06 PM, Bartlomiej Zolnierkiewicz wrote:
> > Add Palmchip BK3710 PATA controller driver.
> > 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> 
> There are some checkpatch suggestions that are worth fixing, I think.
> 
> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:277: CHECK: Alignment should match open parenthesis
> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:320: CHECK: Alignment should match open parenthesis
> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:489: CHECK: Comparison to NULL could be written "!mem"

Will be fixed in v3, thanks.

[ My scripts didn't run checkpatch with --strict so I haven't noticed them. ]

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver

[Sergei Shtylyov]

Hello!

On 03/22/2017 08:49 PM, Bartlomiej Zolnierkiewicz wrote:

>>> Add Palmchip BK3710 PATA controller driver.
>>>
>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>
>> There are some checkpatch suggestions that are worth fixing, I think.
>>
>> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:277: CHECK: Alignment should match open parenthesis
>> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:320: CHECK: Alignment should match open parenthesis
>> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:489: CHECK: Comparison to NULL could be written "!mem"
>
> Will be fixed in v3, thanks.
>
> [ My scripts didn't run checkpatch with --strict so I haven't noticed them. ]

   --strict'ly speaking, this switch is only forced on in the networking 
trees. :-)

> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics

MBR, Sergei

[PATCH v2 2/3] ARM: davinci: add pata_bk3710 libata driver support

[Bartlomiej Zolnierkiewicz]

From: Sekhar Nori <nsekhar@ti.com>

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
[b.zolnierkie: split from bigger patch + preserved old driver support]
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 arch/arm/mach-davinci/board-dm644x-evm.c  | 3 ++-
 arch/arm/mach-davinci/board-dm646x-evm.c  | 3 ++-
 arch/arm/mach-davinci/board-neuros-osd2.c | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c
index 023480b..20f1874 100644
--- a/arch/arm/mach-davinci/board-dm644x-evm.c
+++ b/arch/arm/mach-davinci/board-dm644x-evm.c
@@ -744,7 +744,8 @@ static int davinci_phy_fixup(struct phy_device *phydev)
 	return 0;
 }
 
-#define HAS_ATA		IS_ENABLED(CONFIG_BLK_DEV_PALMCHIP_BK3710)
+#define HAS_ATA		(IS_ENABLED(CONFIG_BLK_DEV_PALMCHIP_BK3710) || \
+			 IS_ENABLED(CONFIG_PATA_BK3710))
 
 #define HAS_NOR		IS_ENABLED(CONFIG_MTD_PHYSMAP)
 
diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
index f702d4f..cb17682 100644
--- a/arch/arm/mach-davinci/board-dm646x-evm.c
+++ b/arch/arm/mach-davinci/board-dm646x-evm.c
@@ -119,7 +119,8 @@
 	},
 };
 
-#define HAS_ATA		IS_ENABLED(CONFIG_BLK_DEV_PALMCHIP_BK3710)
+#define HAS_ATA		(IS_ENABLED(CONFIG_BLK_DEV_PALMCHIP_BK3710) || \
+			 IS_ENABLED(CONFIG_PATA_BK3710))
 
 #ifdef CONFIG_I2C
 /* CPLD Register 0 bits to control ATA */
diff --git a/arch/arm/mach-davinci/board-neuros-osd2.c b/arch/arm/mach-davinci/board-neuros-osd2.c
index 0a78388..0c02aaa 100644
--- a/arch/arm/mach-davinci/board-neuros-osd2.c
+++ b/arch/arm/mach-davinci/board-neuros-osd2.c
@@ -163,7 +163,8 @@ static void __init davinci_ntosd2_map_io(void)
 	.wires		= 4,
 };
 
-#define HAS_ATA		IS_ENABLED(CONFIG_BLK_DEV_PALMCHIP_BK3710)
+#define HAS_ATA		(IS_ENABLED(CONFIG_BLK_DEV_PALMCHIP_BK3710) || \
+			 IS_ENABLED(CONFIG_PATA_BK3710))
 
 #define HAS_NAND	IS_ENABLED(CONFIG_MTD_NAND_DAVINCI)
 
-- 
1.9.1

[PATCH v2 3/3] ARM: davinci_all_defconfig: convert to use libata PATA

[Bartlomiej Zolnierkiewicz]

From: Sekhar Nori <nsekhar@ti.com>

IDE subsystem has been deprecated since 2009 and the majority
(if not all) of Linux distributions have switched to use
libata for ATA support exclusively.  However there are still
some users (mostly old or/and embedded non-x86 systems) that
have not converted from using IDE subsystem to libata PATA
drivers.  This doesn't seem to be good thing in the long-term
for Linux as while there is less and less PATA systems left
in use:

* testing efforts are divided between two subsystems

* having duplicate drivers for same hardware confuses users

This patch converts davinci_all_defconfig to use libata PATA
drivers.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
[b.zolnierkie: split from bigger patch + added patch description]
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 arch/arm/configs/davinci_all_defconfig | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
index c8663ea..93aab3d 100644
--- a/arch/arm/configs/davinci_all_defconfig
+++ b/arch/arm/configs/davinci_all_defconfig
@@ -74,12 +74,10 @@ CONFIG_BLK_DEV_RAM=y
 CONFIG_BLK_DEV_RAM_COUNT=1
 CONFIG_BLK_DEV_RAM_SIZE=32768
 CONFIG_EEPROM_AT24=y
-CONFIG_IDE=m
-CONFIG_BLK_DEV_PALMCHIP_BK3710=m
-CONFIG_SCSI=m
 CONFIG_BLK_DEV_SD=m
 CONFIG_ATA=m
 CONFIG_AHCI_DA850=m
+CONFIG_PATA_BK3710=m
 CONFIG_NETDEVICES=y
 CONFIG_NETCONSOLE=y
 CONFIG_TUN=m
-- 
1.9.1

Re: [PATCH v2 0/3] ATA/ARM: convert ARM/DaVinci to use libata PATA drivers

[Tejun Heo]

On Tue, Mar 14, 2017 at 05:36:51PM +0100, Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> This patchset adds Palmchip BK3710 IDE controller driver to
> libata and switches ARM/DaVinci to use it (instead of the old
> IDE driver).
> 
> Sekhar, please check that it still works after changes, thanks.
> 
> Changes since v1
> (https://www.spinics.net/lists/arm-kernel/msg567442.html):
> - addressed review comments from Sergei Shtylyov
> - fixed cycle_time unitialized variable issue
> 
> Changes since v0.1 draft patch version
> (https://www.spinics.net/lists/arm-kernel/msg566932.html):
> - fixed cycle_time build warning
> - added platform support fixes from Sekhar
> - added defconfig changes from Sekhar
> - preserved platform support for the old IDE driver
> - split it on 3 patches

Looks fine to me from libata side.  Once it gets tested, how should
the patches be routed?  I don't think it'd make sense to route them
separately.

Thanks.

-- 
tejun

Re: [PATCH v2 0/3] ATA/ARM: convert ARM/DaVinci to use libata PATA drivers

[Sekhar Nori]

On Friday 17 March 2017 03:02 AM, Tejun Heo wrote:
> On Tue, Mar 14, 2017 at 05:36:51PM +0100, Bartlomiej Zolnierkiewicz wrote:
>> Hi,
>>
>> This patchset adds Palmchip BK3710 IDE controller driver to
>> libata and switches ARM/DaVinci to use it (instead of the old
>> IDE driver).
>>
>> Sekhar, please check that it still works after changes, thanks.
>>
>> Changes since v1
>> (https://www.spinics.net/lists/arm-kernel/msg567442.html):
>> - addressed review comments from Sergei Shtylyov
>> - fixed cycle_time unitialized variable issue
>>
>> Changes since v0.1 draft patch version
>> (https://www.spinics.net/lists/arm-kernel/msg566932.html):
>> - fixed cycle_time build warning
>> - added platform support fixes from Sekhar
>> - added defconfig changes from Sekhar
>> - preserved platform support for the old IDE driver
>> - split it on 3 patches
> 
> Looks fine to me from libata side.  Once it gets tested, how should
> the patches be routed?  I don't think it'd make sense to route them
> separately.

Hi Tejun, I can take the series through ARM-SoC tree with your ack.

Thanks,
Sekhar

Re: [PATCH v2 0/3] ATA/ARM: convert ARM/DaVinci to use libata PATA drivers

[Tejun Heo]

Hello,

On Tue, Mar 21, 2017 at 02:08:26PM +0530, Sekhar Nori wrote:
> > Looks fine to me from libata side.  Once it gets tested, how should
> > the patches be routed?  I don't think it'd make sense to route them
> > separately.
> 
> Hi Tejun, I can take the series through ARM-SoC tree with your ack.

Once the review points are addressed, please feel free to add

 Acked-by: Tejun Heo <tj@kernel.org>

and route through the arm tree.

Thanks.

-- 
tejun

Re: [PATCH v2 0/3] ATA/ARM: convert ARM/DaVinci to use libata PATA drivers

[Sekhar Nori]

On Tuesday 14 March 2017 10:06 PM, Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> This patchset adds Palmchip BK3710 IDE controller driver to
> libata and switches ARM/DaVinci to use it (instead of the old
> IDE driver).
> 
> Sekhar, please check that it still works after changes, thanks.

It looks like there are some comments pending. I would like to test
again after those are dispositioned. Hope thats fine.

Thanks,
Sekhar