diff mbox

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

Message ID 1489064496-6270-2-git-send-email-b.zolnierkie@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bartlomiej Zolnierkiewicz March 9, 2017, 1:01 p.m. UTC
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

Comments

Sergei Shtylyov March 9, 2017, 5:05 p.m. UTC | #1
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
Sergei Shtylyov March 9, 2017, 8:45 p.m. UTC | #2
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
kernel test robot March 11, 2017, 5:07 a.m. UTC | #3
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
Sergei Shtylyov March 12, 2017, 5:28 p.m. UTC | #4
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
Sergei Shtylyov March 12, 2017, 5:53 p.m. UTC | #5
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
Bartlomiej Zolnierkiewicz March 14, 2017, 4:36 p.m. UTC | #6
Hi,

On Sunday, March 12, 2017 08:28:43 PM Sergei Shtylyov wrote:
> Hello!
> 
> On 03/09/2017 04:01 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> > Add Palmchip BK3710 PATA controller driver.
> >
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> [...]
> > diff --git a/drivers/ata/pata_bk3710.c b/drivers/ata/pata_bk3710.c
> > new file mode 100644
> > index 0000000..65ee737
> > --- /dev/null
> > +++ b/drivers/ata/pata_bk3710.c
> > @@ -0,0 +1,395 @@
> > +/*
> > + * Palmchip BK3710 PATA controller driver
> > + *
> > + * Copyright (c) 2017 Samsung Electronics Co., Ltd.
> > + *		http://www.samsung.com
> > + *
> > + * Based on palm_bk3710.c:
> > + *
> > + * Copyright (C) 2006 Texas Instruments.
> > + * Copyright (C) 2007 MontaVista Software, Inc., <source@mvista.com>
> > + *
> > + * This file is subject to the terms and conditions of the GNU General Public
> > + * License.  See the file "COPYING" in the main directory of this archive
> > + * for more details.
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/ioport.h>
> > +#include <linux/ata.h>
> > +#include <linux/libata.h>
> > +#include <linux/delay.h>
> > +#include <linux/init.h>
> > +#include <linux/clk.h>
> > +#include <linux/platform_device.h>
> 
>     Probably a good idea to sort the #include's alphabetically...

Done.

> > +
> > +#define DRV_NAME "pata_bk3710"
> > +#define DRV_VERSION "0.1.0"
> 
>     This macro isn't used anywhere, do we really need it?

Removed.

> > +
> > +#define BK3710_REG_OFFSET	0x1F0
> 
>     I'd call it BK3710_TF_OFFSET or something of that sort...
> The DM644x manual calls these register command block (which seems to comply 
> with ATA wording)...

Renamed.

> > +#define BK3710_CTL_OFFSET	0x3F6
> > +
> > +#define BK3710_BMISP		0x02
> 
>     Nothing other than the BMIDE status register, dunno why they made it 16-bit...
> 
> > +#define BK3710_IDETIMP		0x40
> > +#define BK3710_UDMACTL		0x48
> > +#define BK3710_MISCCTL		0x50
> > +#define BK3710_REGSTB		0x54
> > +#define BK3710_REGRCVR		0x58
> > +#define BK3710_DATSTB		0x5C
> > +#define BK3710_DATRCVR		0x60
> > +#define BK3710_DMASTB		0x64
> > +#define BK3710_DMARCVR		0x68
> > +#define BK3710_UDMASTB		0x6C
> > +#define BK3710_UDMATRP		0x70
> > +#define BK3710_UDMAENV		0x74
> > +#define BK3710_IORDYTMP		0x78
> 
>     I'd keep all registers declared as in the IDE driver, for the purposes of 
> documentation...

Don't see much point in it as the real documentation is available.

> [...]
> > +static void pata_bk3710_setudmamode(void __iomem *base, unsigned int dev,
> > +				    unsigned int mode)
> > +{
> > +	u32 val32;
> > +	u16 val16;
> > +	u8 tenv, trp, t0;
> 
>     I think DaveM prefers reverse Christmas tree order with the declarations 
> but maybe that's only for the networking tree... :-)
> 
> > +
> > +	/* DMA Data Setup */
> > +	t0 = DIV_ROUND_UP(pata_bk3710_udmatimings[mode].cycletime,
> > +			  ideclk_period) - 1;
> > +	tenv = DIV_ROUND_UP(20, ideclk_period) - 1;
> > +	trp = DIV_ROUND_UP(pata_bk3710_udmatimings[mode].rptime,
> > +			   ideclk_period) - 1;
> > +
> > +	/* udmastb Ultra DMA Access Strobe Width */
> > +	val32 = ioread32(base + BK3710_UDMASTB) & (0xFF << (dev ? 0 : 8));
> 
>     I'd separate ioread32() and & to the different lines but as this is copied 
> from the IDE driver verbatim, you can ignore me. :-)

Ignored. ;-)

> > +	val32 |= (t0 << (dev ? 8 : 0));
> 
>     Outer parens not really needed.

Fixed.

> > +	iowrite32(val32, base + BK3710_UDMASTB);
> > +
> > +	/* udmatrp Ultra DMA Ready to Pause Time */
> > +	val32 = ioread32(base + BK3710_UDMATRP) & (0xFF << (dev ? 0 : 8));
> > +	val32 |= (trp << (dev ? 8 : 0));
> 
>     Here as well..

ditto

> > +	iowrite32(val32, base + BK3710_UDMATRP);
> > +
> > +	/* udmaenv Ultra DMA envelop Time */
> > +	val32 = ioread32(base + BK3710_UDMAENV) & (0xFF << (dev ? 0 : 8));
> > +	val32 |= (tenv << (dev ? 8 : 0));
> 
>     And here...

ditto

> [...]
> > +static void pata_bk3710_setdmamode(void __iomem *base, unsigned int dev,
> 
>     Maybe setmwdmamode()?

Renamed.

> > +				   unsigned short min_cycle,
> > +				   unsigned int mode)
> > +{
> > +	const struct ata_timing *t;
> > +	int cycletime;
> > +	u32 val32;
> > +	u16 val16;
> > +	u8 td, tkw, t0;
> > +
> > +	t = ata_timing_find_mode(mode);
> > +	cycletime = max_t(int, t->cycle, min_cycle);
> > +
> > +	/* DMA Data Setup */
> > +	t0 = DIV_ROUND_UP(cycletime, ideclk_period);
> > +	td = DIV_ROUND_UP(t->active, ideclk_period);
> > +	tkw = t0 - td - 1;
> > +	td -= 1;
> 
> 	td--;

Fixed.

> > +
> > +	val32 = ioread32(base + BK3710_DMASTB) & (0xFF << (dev ? 0 : 8));
> > +	val32 |= (td << (dev ? 8 : 0));
> 
>     And here...

ditto

> > +	iowrite32(val32, base + BK3710_DMASTB);
> > +
> > +	val32 = ioread32(base + BK3710_DMARCVR) & (0xFF << (dev ? 0 : 8));
> > +	val32 |= (tkw << (dev ? 8 : 0));
> 
>     And here...

ditto

> [...]
> > +static void pata_bk3710_setpiomode(void __iomem *base, struct ata_device *pair,
> > +				   unsigned int dev, unsigned int cycletime,
> > +				   unsigned int mode)
> > +{
> > +	const struct ata_timing *t;
> > +	u32 val32;
> > +	u8 t2, t2i, t0;
> > +
> > +	t = ata_timing_find_mode(XFER_PIO_0 + mode);
> > +
> > +	/* PIO Data Setup */
> > +	t0 = DIV_ROUND_UP(cycletime, ideclk_period);
> > +	t2 = DIV_ROUND_UP(t->active, ideclk_period);
> > +
> > +	t2i = t0 - t2 - 1;
> > +	t2 -= 1;
> 
> 	t2--;

ditto

> > +
> > +	val32 = ioread32(base + BK3710_DATSTB) & (0xFF << (dev ? 0 : 8));
> > +	val32 |= (t2 << (dev ? 8 : 0));
> 
>     Outer parens not needed.

ditto

> > +	iowrite32(val32, base + BK3710_DATSTB);
> > +
> > +	val32 = ioread32(base + BK3710_DATRCVR) & (0xFF << (dev ? 0 : 8));
> > +	val32 |= (t2i << (dev ? 8 : 0));
> 
>     Here too..

ditto

> > +	iowrite32(val32, base + BK3710_DATRCVR);
> > +
> > +	/* FIXME: this is broken also in the old driver */
> 
>    What's wrong with this logic BTW?

Two things:
- it happens too late, after "mode" variable has been read
- we should probably just merge timings instead of downgrading PIO mode

> > +	if (pair) {
> > +		u8 mode2 = pair->pio_mode - XFER_PIO_0;
> > +
> > +		if (mode2 < mode)
> > +			mode = mode2;
> > +	}
> > +
> > +	/* TASKFILE Setup */
> > +	t0 = DIV_ROUND_UP(t->cyc8b, ideclk_period);
> > +	t2 = DIV_ROUND_UP(t->act8b, ideclk_period);
> > +
> > +	t2i = t0 - t2 - 1;
> > +	t2 -= 1;
> 
> 	t2--;

Fixed.

> > +
> > +	val32 = ioread32(base + BK3710_REGSTB) & (0xFF << (dev ? 0 : 8));
> > +	val32 |= (t2 << (dev ? 8 : 0));
> 
>     Outer parens again...

ditto

> > +	iowrite32(val32, base + BK3710_REGSTB);
> > +
> > +	val32 = ioread32(base + BK3710_REGRCVR) & (0xFF << (dev ? 0 : 8));
> > +	val32 |= (t2i << (dev ? 8 : 0));
> 
>     And again...

ditto

> > +	iowrite32(val32, base + BK3710_REGRCVR);
> > +}
> > +
> > +static void pata_bk3710_set_piomode(struct ata_port *ap,
> > +				    struct ata_device *adev)
> > +{
> > +	void __iomem *base = (void __iomem *)ap->ioaddr.bmdma_addr;
> > +	struct ata_device *pair = ata_dev_pair(adev);
> > +	const struct ata_timing *t = ata_timing_find_mode(adev->pio_mode);
> > +	const u16 *id = adev->id;
> > +	unsigned int cycle_time;
> > +	int is_slave = adev->devno;
> > +	const u8 pio = adev->pio_mode - XFER_PIO_0;
> > +
> > +	if (id[ATA_ID_FIELD_VALID] & 2) {
> > +		if (ata_id_has_iordy(id))
> > +			cycle_time = id[ATA_ID_EIDE_PIO_IORDY];
> > +		else
> > +			cycle_time = id[ATA_ID_EIDE_PIO];
> > +
> > +		/* conservative "downgrade" for all pre-ATA2 drives */
> > +		if (pio < 3 && cycle_time < t->cycle)
> > +			cycle_time = 0; /* use standard timing */
> > +	}
> > +
> > +	if (!cycle_time)
> > +		cycle_time = t->cycle;
> 
>     This seems like a helper needed by libata in general but OK...

I need to think about this a bit more..

[...]

> > +static int __init pata_bk3710_probe(struct platform_device *pdev)
> > +{
> > +	struct clk *clk;
> > +	struct resource *mem, *irq;
> > +	struct ata_host *host;
> > +	struct ata_port *ap;
> > +	void __iomem *base;
> > +	unsigned long rate, mem_size;
> > +
> > +	clk = clk_get(&pdev->dev, NULL);
> 
>     devm_clk_get()?

Fixed.

> > +	if (IS_ERR(clk))
> > +		return -ENODEV;
> > +
> > +	clk_enable(clk);
> > +	rate = clk_get_rate(clk);
> > +	if (!rate)
> > +		return -EINVAL;
> > +
> > +	/* NOTE:  round *down* to meet minimum timings; we count in clocks */
> > +	ideclk_period = 1000000000UL / rate;
> > +
> > +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (mem == NULL) {
> > +		pr_err(DRV_NAME ": failed to get memory region resource\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> 
>     How about platform_get_irq()? I've fixed it... :-)

Converted.

> > +	if (irq == NULL) {
> > +		pr_err(DRV_NAME ": failed to get IRQ resource\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	mem_size = resource_size(mem);
> > +	if (!devm_request_mem_region(&pdev->dev, mem->start, mem_size,
> > +				     DRV_NAME)) {
> > +		pr_err(DRV_NAME ": failed to request memory region\n");
> > +		return -EBUSY;
> > +	}
> > +
> > +	base = ioremap(mem->start, mem_size);
> 
>     How about devm_ioremap_resource() instead of the above?

ditto

> > +	if (!base) {
> > +		pr_err(DRV_NAME ": failed to map IO memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* Configure the Palm Chip controller */
> 
>     It's Palmchip. :-)

Fixed.

[...]

Thanks for review!

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

Patch

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");