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

[PATCH 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 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                 | 395 ++++++++++++++++++++++++++++++
 7 files changed, 412 insertions(+), 6 deletions(-)
 create mode 100644 drivers/ata/pata_bk3710.c

-- 
1.9.1

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

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

[Sergei Shtylyov]

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

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

[Sergei Shtylyov]

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

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

[kbuild test robot]

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>

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

[Sergei Shtylyov]

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

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

[Sergei Shtylyov]

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

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

[Bartlomiej Zolnierkiewicz]

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

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