Re: [PATCH] SATA: OCTEON: support SATA on OCTEON platform

From: David Daney
Date: Mon Jan 19 2015 - 14:16:38 EST

On 01/19/2015 07:43 AM, Mark Rutland wrote:
On Mon, Jan 19, 2015 at 03:23:58PM +0000, Aleksey Makarov wrote:
The OCTEON SATA controller is currently found on cn71XX devices.

Signed-off-by: David Daney <david.daney@xxxxxxxxxx>
Signed-off-by: Vinita Gupta <vgupta@xxxxxxxxxxxxxxxxxx>
[aleksey.makarov@xxxxxxxxxx: preparing for submission,
conflict resolution, fixes for the platform code]
Signed-off-by: Aleksey Makarov <aleksey.makarov@xxxxxxxxxx>
---
.../devicetree/bindings/ata/ahci-platform.txt | 1 +
.../devicetree/bindings/mips/cavium/sata-uctl.txt | 31 ++++++
arch/mips/cavium-octeon/octeon-platform.c | 1 +
drivers/ata/Kconfig | 9 ++
drivers/ata/Makefile | 1 +
drivers/ata/ahci_platform.c | 10 ++
drivers/ata/sata_octeon.c | 107 +++++++++++++++++++++
7 files changed, 160 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mips/cavium/sata-uctl.txt
create mode 100644 drivers/ata/sata_octeon.c

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 4ab09f2..1a5d3be 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -11,6 +11,7 @@ Required properties:
- compatible : compatible string, one of:
- "allwinner,sun4i-a10-ahci"
- "hisilicon,hisi-ahci"
+ - "cavium,octeon-7130-ahci"
- "ibm,476gtr-ahci"
- "marvell,armada-380-ahci"
- "snps,dwc-ahci"
diff --git a/Documentation/devicetree/bindings/mips/cavium/sata-uctl.txt b/Documentation/devicetree/bindings/mips/cavium/sata-uctl.txt
new file mode 100644
index 0000000..222e66e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mips/cavium/sata-uctl.txt
@@ -0,0 +1,31 @@
+* UCTL SATA controller glue

I'm not sure I follow. What does this mean?

Well, UCTL is the internal name of the hardware block. It functions to connect a standard AHCI controller to the internal busses of the OCTEON SoC.


+
+Properties:
+- compatible: "cavium,octeon-7130-sata-uctl"
+
+ Compatibility with the cn7130 SOC.
+
+- reg: The base address of the UCTL register bank.
+
+- #address-cells: Must be <2>.
+
+- #size-cells: Must be <2>.
+
+- ranges: Empty to signify direct mapping of the children.

Why can't these be any values which are sufficient to map children?

They can. It happens to be the case that it is always empty.



+
+Example:
+
+ uctl@118006c000000 {
+ compatible = "cavium,octeon-7130-sata-uctl";
+ reg = <0x00011800 0x6c000000 0x00000000 0x00000100>;
+ ranges;
+ #address-cells = <0x00000002>;
+ #size-cells = <0x00000002>;

No need for all the zero-padding on these, they aren't addresses.

OK.


+ sata@16c00 {
+ compatible = "cavium,octeon-7130-ahci";
+ reg = <0x00016c00 0x00000000 0x00000000 0x00000200>;
+ interrupt-parent = <0x0000000d>;
+ interrupts = <0x00000002 0x00000004>;
+ cavium,qlm-trim-alias = "sata";

What's this property for? It wasn't documented above, and doesn't exist
in mainline.

It is not a part of the public interface, we will remove this line.


[...]

diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index ae41107..4a0e5e3 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_AHCI_SUNXI) += ahci_sunxi.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_ST) += ahci_st.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_TEGRA) += ahci_tegra.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_XGENE) += ahci_xgene.o libahci.o libahci_platform.o
+obj-$(CONFIG_SATA_OCTEON) += sata_octeon.o

# SFF w/ custom DMA
obj-$(CONFIG_PDC_ADMA) += pdc_adma.o
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 18d5398..bb36396 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -22,6 +22,12 @@
#include <linux/ahci_platform.h>
#include "ahci.h"

+#if IS_ENABLED(CONFIG_SATA_OCTEON)
+void ahci_octeon_config(struct platform_device *pdev);
+#else
+static inline void ahci_octeon_config(struct platform_device *pdev) {}
+#endif
+
static const struct ata_port_info ahci_port_info = {
.flags = AHCI_FLAG_COMMON,
.pio_mask = ATA_PIO4,
@@ -46,6 +52,9 @@ static int ahci_probe(struct platform_device *pdev)
if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci"))
hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ;

+ if (of_device_is_compatible(dev->of_node, "cavium,octeon-7130-ahci"))
+ ahci_octeon_config(pdev);
+

If we really need this kind of thing, make a new struct and associate it
with of_device_id::data in the table below. Then we make this path free
from any device-specific code.


Good idea.

We will attempt to factor this into a separate driver module for the "cavium,octeon-7130-sata-uctl" block. If that turns out to be too ugly, I would like to keep the code in this file, but with the change you suggest.

rc = ahci_platform_init_host(pdev, hpriv, &ahci_port_info);
if (rc)
goto disable_resources;
@@ -67,6 +76,7 @@ static const struct of_device_id ahci_of_match[] = {
{ .compatible = "ibm,476gtr-ahci", },
{ .compatible = "snps,dwc-ahci", },
{ .compatible = "hisilicon,hisi-ahci", },
+ { .compatible = "cavium,octeon-7130-ahci", },
{},

I was under the impression that the strings other than "generic-ahci"
were only for compatibility with existing DTBs. Why do we need to add
new platform-specific strings here?

Because it is an "existing DTB", The device tree doesn't contain the compatible property of "generic-ahci", only "cavium,octeon-7130-ahci".


[...]

+void ahci_octeon_config(struct platform_device *pdev)
+{
+ union cvmx_sata_uctl_shim_cfg shim_cfg;
+
+ /* set-up endian mode */
+ shim_cfg.u64 = cvmx_read_csr(CVMX_SATA_UCTL_SHIM_CFG);
+#ifdef __BIG_ENDIAN
+ shim_cfg.s.dma_endian_mode = 1;
+ shim_cfg.s.csr_endian_mode = 1;
+#else
+ shim_cfg.s.dma_endian_mode = 0;
+ shim_cfg.s.csr_endian_mode = 0;
+#endif
+ shim_cfg.s.dma_read_cmd = 1; /* No allocate L2C */
+ cvmx_write_csr(CVMX_SATA_UCTL_SHIM_CFG, shim_cfg.u64);
+
+ /* Set a good dma_mask */
+ pdev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
+ pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;

I thought a dma-ranges property in the DT could be used to set up the
DMA mask appropriately?

The DT contains no dma-ranges property, and we know a priori, that it should be 64-bits.


Thanks,
Mark.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/