add CNS3xxx AHCI support

From: Mac Lin <[email protected]>

Switch the driver to use module device table matching mechanism to add SoC-specific parts to the generic driver.

Signed-off-by Mac Lin <[email protected]>

---
 drivers/ata/ahci_pltfm.c |   14 +++++++++++++-
 drivers/ata/ahci_pltfm.h |   17 +++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletions(-)
 create mode 100644 drivers/ata/ahci_pltfm.h

From: Mac Lin <[email protected]>

CNS3xxx override the softreset function of ahci_platform ahci_softreset by cns3xxx_ahci_softreset, which would retry ahci_do_softreset again with pmp=0 if pmp=15 failed, for the controller has problem receiving D2H Reg FIS of the different PMP setting of the previous sent H2D Reg FIS.

Following describe the isssue with original ahci_platform driver on linux-2.6.37-rc3, arm/cns3xxx.

If CONFIG_SATA_PMP is enabled, while not using multiplier and connect the disks directly to the board, the disk cannot be found due to software reset always failed.

ahci ahci.0: forcing PORTS_IMPL to 0x3
ahci ahci.0: AHCI 0001.0100 32 slots 2 ports 3 Gbps 0x3 impl platform mode ahci ahci.0: flags: ncq sntf pm led clo only pmp pio slum part ccc scsi0 : ahci_platform
scsi1 : ahci_platform
ata1: SATA max UDMA/133 irq_stat 0x00400040, connection status changed irq 65
ata2: SATA max UDMA/133 mmio [mem 0x83000000-0x83ffffff] port 0x180 irq 65
ata2: SATA link down (SStatus 0 SControl 300)
ata1: link is slow to respond, please be patient (ready=0)
ata1: softreset failed (device not ready)
ata1: link is slow to respond, please be patient (ready=0)
ata1: softreset failed (device not ready)
ata1: link is slow to respond, please be patient (ready=0)
ata1: softreset failed (device not ready)
ata1: limiting SATA link speed to 1.5 Gbps
ata1: SATA link down (SStatus 1 SControl 310)

While using multiplier with CONFIG_SATA_PMP enabled, or using disks directly without CONFIG_SATA_PMP have no issue. It seems the device is sending D2H Reg FIS, but controller is not reflecting it on any known means.

Signed-off-by Mac Lin <[email protected]>

---
 arch/arm/mach-cns3xxx/devices.c |    2 +-
 drivers/ata/Kconfig             |   11 +++++++
 drivers/ata/Makefile            |    1 +
 drivers/ata/ahci_cns3xxx.c      |   62 +++++++++++++++++++++++++++++++++++++++
 drivers/ata/ahci_pltfm.c        |    3 ++
 drivers/ata/ahci_pltfm.h        |    2 +
 6 files changed, 80 insertions(+), 1 deletions(-)
 create mode 100644 drivers/ata/ahci_cns3xxx.c

arch/arm/mach-cns3xxx/devices.c +1 -1

index 50b4d31..b496f02 100644
@@ -40,7 +40,7 @@ static struct resource cns3xxx_ahci_resource[] = {
 static u64 cns3xxx_ahci_dmamask = DMA_BIT_MASK(32);
 
 static struct platform_device cns3xxx_ahci_pdev = {
-	.name		= "ahci",
+	.name		= "ahci-cns3xxx",
 	.id		= 0,
 	.resource	= cns3xxx_ahci_resource,
 	.num_resources	= ARRAY_SIZE(cns3xxx_ahci_resource),

drivers/ata/Kconfig +11 -0

index 36e2319..5d8b1a3 100644
@@ -75,6 +75,17 @@ config SATA_AHCI_PLATFORM
 
 	  If unsure, say N.
 
+config SATA_AHCI_CNS3XXX
+	bool "AHCI Support on the Cavium Networks CNS3xxx SOC"
+	depends on ARCH_CNS3XXX
+	depends on SATA_AHCI_PLATFORM
+	help
+	  This option enables AHCI platform driver to support CNS3xxx
+	  System-on-Chip devices. This is only needed when using CNS3xxx AHCI
+	  controller.
+
+	  If unsure, say N.
+
 config SATA_FSL
 	tristate "Freescale 3.0Gbps SATA support"
 	depends on FSL_SOC

drivers/ata/Makefile +1 -0

index 5b62be8..a0745e5 100644
@@ -100,6 +100,7 @@ obj-$(CONFIG_PATA_LEGACY)	+= pata_legacy.o
 
 obj-$(CONFIG_SATA_AHCI_PLATFORM) 		+= ahci_platform.o libahci.o
 ahci_platform-y					:= ahci_pltfm.o
+ahci_platform-$(CONFIG_SATA_AHCI_CNS3XXX) 	+= ahci_cns3xxx.o
 
 libata-y	:= libata-core.o libata-scsi.o libata-eh.o libata-transport.o
 libata-$(CONFIG_ATA_SFF)	+= libata-sff.o

drivers/ata/ahci_cns3xxx.c new file +14 -0

new file mode 100644
index 0000000..f7a238e
@@ -0,0 +1,62 @@
+/*
+ * AHCI support for CNS3xxx SoC
+ *
+ * Copyright 2010 MontaVista Software, LLC.
+ * Copyright 2010 Cavium Networks
+ *
+ * Authors: Anton Vorontsov <[email protected]>
+ *	    Mac Lin <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+

drivers/ata/ahci_cns3xxx.c new file +7 -0

+static struct ata_port_operations cns3xxx_ahci_ops = {
+	.inherits		= &ahci_ops,
+	.softreset		= cns3xxx_ahci_softreset,
+};
+
+static const struct ata_port_info cns3xxx_ata_port_info = {
+	.flags		= AHCI_FLAG_COMMON,

From: Mac Lin<[email protected]>

v2:
- Switch ahci_platform to module device table matching to add SoC specific support

v1: http://www.spinics.net/lists/arm-kernel/msg106236.html
- Add CNS3xxx SoC specific AHCI support

This patchset is based on linux-2.6.37-rc2

Mac Lin (3):
ahci_platform: rename to ahci_pltfm, but keep the original module name
ahci_pltfm: switch to module device table matching
ahci_platform: add support for CNS3xxx SoC devices

  arch/arm/mach-cns3xxx/devices.c |    2 +-
  drivers/ata/Kconfig             |   11 ++
  drivers/ata/Makefile            |    5 +-
  drivers/ata/ahci_cns3xxx.c      |   62 +++++++++++
  drivers/ata/ahci_platform.c     |  197 ------------------------------------
  drivers/ata/ahci_pltfm.c        |  212 +++++++++++++++++++++++++++++++++++++++
  drivers/ata/ahci_pltfm.h        |   19 ++++
  7 files changed, 309 insertions(+), 199 deletions(-)

create mode 100644 drivers/ata/ahci_cns3xxx.c
delete mode 100644 drivers/ata/ahci_platform.c
create mode 100644 drivers/ata/ahci_pltfm.c
create mode 100644 drivers/ata/ahci_pltfm.h

Quoted 23 lines

From: Mac Lin<[email protected]>

v2:
?- Switch ahci_platform to module device table matching to add SoC specific support

v1: http://www.spinics.net/lists/arm-kernel/msg106236.html
?- Add CNS3xxx SoC specific AHCI support

This patchset is based on linux-2.6.37-rc2

Mac Lin (3):
? ? ? ahci_platform: rename to ahci_pltfm, but keep the original module name
? ? ? ahci_pltfm: switch to module device table matching
? ? ? ahci_platform: add support for CNS3xxx SoC devices

?arch/arm/mach-cns3xxx/devices.c | ? ?2 +-
?drivers/ata/Kconfig ? ? ? ? ? ? | ? 11 ++
?drivers/ata/Makefile ? ? ? ? ? ?| ? ?5 +-
?drivers/ata/ahci_cns3xxx.c ? ? ?| ? 62 +++++++++++
?drivers/ata/ahci_platform.c ? ? | ?197

Quoted 9 lines

------------------------------------
?drivers/ata/ahci_pltfm.c ? ? ? ?| ?212
+++++++++++++++++++++++++++++++++++++++
?drivers/ata/ahci_pltfm.h ? ? ? ?| ? 19 ++++ ?7 files changed, 309 insertions(+), 199 deletions(-) ?create mode 100644 drivers/ata/ahci_cns3xxx.c ?delete mode 100644 drivers/ata/ahci_platform.c ?create mode 100644 drivers/ata/ahci_pltfm.c ?create mode 100644 drivers/ata/ahci_pltfm.h

It is overkill to rename the entirety of ahci_platform just for one override function.
This sort of thing I would have expected to be added directly to ahci_platform.c.

Quoted 4 lines

It is overkill to rename the entirety of ahci_platform just for one override function.
This sort of thing I would have expected to be added directly to ahci_platform.c.

It might be overkill for only one controller. but it is more clean and readable to have different SoC specific changes in separate files, especially when more SoCs need to make similar changes.

[...]

Quoted 7 lines

Quoted 4 lines

It is overkill to rename the entirety of ahci_platform just for one override function.
This sort of thing I would have expected to be added directly to ahci_platform.c.

It might be overkill for only one controller. but it is more clean and readable to have different SoC specific changes in separate files, especially when more SoCs need to make similar changes.

I think that renaming the file is not necessary. You can just rename the module in the makefile.

Personally I like the current approach more than putting controller-specific fixups directly into ahci_platform.

On Thu, Jan 06, 2011 at 02:43:08PM +0800, Lin Mac wrote:
[...]

Quoted 7 lines

Quoted 4 lines

It is overkill to rename the entirety of ahci_platform just for one override function.
This sort of thing I would have expected to be added directly to ahci_platform.c.

It might be overkill for only one controller. but it is more clean and readable to have different SoC specific changes in separate files, especially when more SoCs need to make similar changes.

I think that renaming the file is not necessary. You can just rename the module in the makefile.

Personally I like the current approach more than putting controller-specific fixups directly into ahci_platform.

Quoted 12 lines

[...]

Quoted 7 lines

Quoted 4 lines

It is overkill to rename the entirety of ahci_platform just for one override function.
This sort of thing I would have expected to be added directly to ahci_platform.c.

It might be overkill for only one controller. but it is more clean and readable to have different SoC specific changes in separate files, especially when more SoCs need to make similar changes.

I think that renaming the file is not necessary. You can just rename the module in the makefile.

Quoted 2 lines

Personally I like the current approach more than putting controller-specific fixups directly into ahci_platform.

My main objection is the renaming. If ahci_platform wants to export some symbols for SoC modules like ahci_platform_cns, that's ok.

In general, follow the library approach rather than linking modules together in strange ways.

Quoted 12 lines

Quoted 12 lines

[...]

Quoted 7 lines

Quoted 4 lines

It is overkill to rename the entirety of ahci_platform just for one override function.
This sort of thing I would have expected to be added directly to ahci_platform.c.

It might be overkill for only one controller. but it is more clean and readable to have different SoC specific changes in separate files, especially when more SoCs need to make similar changes.

I think that renaming the file is not necessary. You can just rename the module in the makefile.

Quoted 8 lines

Quoted 2 lines

Personally I like the current approach more than putting controller-specific fixups directly into ahci_platform.

My main objection is the renaming. ?If ahci_platform wants to export some symbols for SoC modules like ahci_platform_cns, that's ok.

In general, follow the library approach rather than linking modules together in strange ways.

In ahci_platform case there's almost nothing to factor out to a library. Each platform just needs its own small "fixups" that we normally pass via platform data.

We would happily pass them via platform data, and initially Mac did exactly this, see:

http://ns3.spinics.net/lists/linux-ide/msg39554.html

You may notice that all platform-specific quirks are in the platform code, which is neat. But there is a problem: once the platform code needs to use libata, then we have to build libata into the kernel (no modules possible), i.e.

http://ns3.spinics.net/lists/linux-ide/msg39656.html

It's not something new, we have the same "problem" with SDHCI, USB and plenty other drivers. But the solution is simple, for example see drivers/mmc/host/sdhci-pltfm.c and sdhci-cns3xxx.c (and sdhci-esdhc-imx.c as another example of platform-specific hooks for sdhci-pltfm).

Of course, we could make full-fledged platform driver just for CNS3xxx, but that's kind of overkill. IMO, it's better to make ahci_platform flexible and suitable for generic use.

As for renaming, I agree that ahci_platform.c file does not need to be renamed, I still think that we could just rename the module (so far it is used only on CNS3xxx platforms, so no big deal even if anybody rely on the module name, which I doubt.)