[PATCH 0/5] ata: add support for Marvell Armada 7K/8K AHCI

[PATCH 0/5] ata: add support for Marvell Armada 7K/8K AHCI

[Thomas Petazzoni]

Hello,

The following small series adds support for the Marvell Armada 7K/8K
AHCI controller. This controller is a standard AHCI controller, so not
much is needed, except that it uses a non-default port offset and
length. Due to this, some preparatory patches are used to make it
possible to override the default port offset/length used by the AHCI
core.

Thanks!

Thomas

Thomas Petazzoni (5):
  ata: ahci: add support for non-standard port offset/length
  ata: acard-ahci: use the new port_{offset,length} members
  ata: ahci_ceva: use the new port_{offset,length} members
  ata: ahci_mvebu: add support for Armada 8K
  dt-bindings: ata: add compatible string for Armada 8K to ahci-platform

 Documentation/devicetree/bindings/ata/ahci-platform.txt | 1 +
 drivers/ata/acard-ahci.c                                | 3 ++-
 drivers/ata/ahci.c                                      | 5 +++--
 drivers/ata/ahci.h                                      | 7 ++++++-
 drivers/ata/ahci_ceva.c                                 | 5 ++---
 drivers/ata/ahci_mvebu.c                                | 5 +++++
 drivers/ata/libahci_platform.c                          | 7 ++++++-
 7 files changed, 25 insertions(+), 8 deletions(-)

-- 
2.6.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ata: ahci: add support for non-standard port offset/length

[Thomas Petazzoni]

By default, the AHCI registers start at a base of 0x100 bytes from the
base register address, and have a length of 0x80 bytes for each
port. Unfortunately, the ARM64 Marvell Armada 7K/8K deviates from this
default, with a base of 0x10000 bytes and a register length of 0x10000
bytes per port.

In order to support the Armada 7K/8K AHCI controller easily, this
commit extends the ahci_host_priv structure with two members:

 - port_offset, which contains the offset from the base, which
   defaults to AHCI_DEFAULT_PORT_OFFSET (0x100 bytes)

 - port_length, which contains the length of the per-port register
   area, which defaults to AHCI_DEFAULT_PORT_LENGTH (0x80 bytes)

This commit adds the structure members, initializes them to their
default value in ahci_platform_get_resources() and uses those
structure members were appropriate in the AHCI core.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/ata/ahci.c             | 5 +++--
 drivers/ata/ahci.h             | 7 ++++++-
 drivers/ata/libahci_platform.c | 7 ++++++-
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index a83bbcc..a695c17 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1714,10 +1714,11 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
+		unsigned int offset =
+			hpriv->port_offset + ap->port_no * hpriv->port_length;
 
 		ata_port_pbar_desc(ap, ahci_pci_bar, -1, "abar");
-		ata_port_pbar_desc(ap, ahci_pci_bar,
-				   0x100 + ap->port_no * 0x80, "port");
+		ata_port_pbar_desc(ap, ahci_pci_bar, offset, "port");
 
 		/* set enclosure management message type */
 		if (ap->flags & ATA_FLAG_EM)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 70b06bc..aca6bb2 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -326,6 +326,9 @@ struct ahci_port_priv {
 	char			*irq_desc;	/* desc in /proc/interrupts */
 };
 
+#define AHCI_DEFAULT_PORT_OFFSET	0x100
+#define AHCI_DEFAULT_PORT_LENGTH	0x80
+
 struct ahci_host_priv {
 	/* Input fields */
 	unsigned int		flags;		/* AHCI_HFLAG_* */
@@ -333,6 +336,8 @@ struct ahci_host_priv {
 	u32			mask_port_map;	/* mask out particular bits */
 
 	void __iomem *		mmio;		/* bus-independent mem map */
+	u32			port_offset;	/* offset of ports from base */
+	u32			port_length;	/* length of per-port area */
 	u32			cap;		/* cap to use */
 	u32			cap2;		/* cap2 to use */
 	u32			version;	/* cached version */
@@ -433,7 +438,7 @@ static inline void __iomem *__ahci_port_base(struct ata_host *host,
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->mmio;
 
-	return mmio + 0x100 + (port_no * 0x80);
+	return mmio + hpriv->port_offset + (port_no * hpriv->port_length);
 }
 
 static inline void __iomem *ahci_port_base(struct ata_port *ap)
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index aaa761b..727fe66 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -371,6 +371,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		goto err_out;
 	}
 
+	hpriv->port_offset = AHCI_DEFAULT_PORT_OFFSET;
+	hpriv->port_length = AHCI_DEFAULT_PORT_LENGTH;
+
 	for (i = 0; i < AHCI_MAX_CLKS; i++) {
 		/*
 		 * For now we must use clk_get(dev, NULL) for the first clock,
@@ -556,10 +559,12 @@ int ahci_platform_init_host(struct platform_device *pdev,
 
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
+		unsigned int offset =
+			hpriv->port_offset + ap->port_no * hpriv->port_length;
 
 		ata_port_desc(ap, "mmio %pR",
 			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
-		ata_port_desc(ap, "port 0x%x", 0x100 + ap->port_no * 0x80);
+		ata_port_desc(ap, "port 0x%x", offset);
 
 		/* set enclosure management message type */
 		if (ap->flags & ATA_FLAG_EM)
-- 
2.6.4

Re: [PATCH 1/5] ata: ahci: add support for non-standard port offset/length

[Tejun Heo]

Hello,

On Fri, Apr 22, 2016 at 04:32:37PM +0200, Thomas Petazzoni wrote:
> @@ -1714,10 +1714,11 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	for (i = 0; i < host->n_ports; i++) {
>  		struct ata_port *ap = host->ports[i];
> +		unsigned int offset =
> +			hpriv->port_offset + ap->port_no * hpriv->port_length;
>  
>  		ata_port_pbar_desc(ap, ahci_pci_bar, -1, "abar");
> -		ata_port_pbar_desc(ap, ahci_pci_bar,
> -				   0x100 + ap->port_no * 0x80, "port");
> +		ata_port_pbar_desc(ap, ahci_pci_bar, offset, "port");

Doesn't this just affect ahci_platform?  Why is ahci_init_one() being
updated?  Also, who's setting port_offet and port_length for
non-platform ahci drivers?

> @@ -556,10 +559,12 @@ int ahci_platform_init_host(struct platform_device *pdev,
>  
>  	for (i = 0; i < host->n_ports; i++) {
>  		struct ata_port *ap = host->ports[i];
> +		unsigned int offset =
> +			hpriv->port_offset + ap->port_no * hpriv->port_length;
>  
>  		ata_port_desc(ap, "mmio %pR",
>  			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
> -		ata_port_desc(ap, "port 0x%x", 0x100 + ap->port_no * 0x80);
> +		ata_port_desc(ap, "port 0x%x", offset);

If this needs to be configurable, wouldn't it be better to just let it
be specified per port?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] ata: ahci: add support for non-standard port offset/length

[Thomas Petazzoni]

Hello,

Thanks for your review and feedback!

On Fri, 22 Apr 2016 13:59:36 -0400, Tejun Heo wrote:

> On Fri, Apr 22, 2016 at 04:32:37PM +0200, Thomas Petazzoni wrote:
> > @@ -1714,10 +1714,11 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  
> >  	for (i = 0; i < host->n_ports; i++) {
> >  		struct ata_port *ap = host->ports[i];
> > +		unsigned int offset =
> > +			hpriv->port_offset + ap->port_no * hpriv->port_length;
> >  
> >  		ata_port_pbar_desc(ap, ahci_pci_bar, -1, "abar");
> > -		ata_port_pbar_desc(ap, ahci_pci_bar,
> > -				   0x100 + ap->port_no * 0x80, "port");
> > +		ata_port_pbar_desc(ap, ahci_pci_bar, offset, "port");
> 
> Doesn't this just affect ahci_platform?

No, because the main source of the problem is the ahci_port_base()
function, which is used all over the place in libahci.c (which is used
by all AHCI drivers, not only ahci_platform drivers). And this
ahci_port_base() currently hardcodes the 0x100 / 0x80 values:

429 static inline void __iomem *__ahci_port_base(struct ata_host *host,
430                                              unsigned int port_no)
431 {
432         struct ahci_host_priv *hpriv = host->private_data;
433         void __iomem *mmio = hpriv->mmio;
434 
435         return mmio + 0x100 + (port_no * 0x80);
436 }
437 
438 static inline void __iomem *ahci_port_base(struct ata_port *ap)
439 {
440         return __ahci_port_base(ap->host, ap->port_no);
441 }

In essence all what we want to change is the __ahci_port_base()
function so that it doesn't use hardcoded values.

>  Why is ahci_init_one() being updated?

The idea why ahci_init_one() was updated is also to remove hardcoded
values. Once we have the correct values in the ahci_host_priv, why not
use them everywhere instead of keeping some hardcoded values?

> Also, who's setting port_offet and port_length for non-platform ahci
> drivers?

This is obviously a mistake in my proposal. And the non-platform
drivers are allocating manually their ahci_host_priv structure, which
makes it a bit difficult/annoying to initialize those new fields.

I see a few possibilities:

 1/ Adjust all the drivers allocating manually an ahci_host_priv
    structure, and make sure they initialize the fields to their default
    values.

 2/ Add a new helper function that allows to allocate the
    ahci_host_priv structure and initialize it with sane default values,
    and use this helper in all drivers.

 3/ Add a flag in ahci_host_priv that tells whether non-standard port
    offset/length are to be used. This is the most minimal solution,
    but also maybe not the nicest one.

> If this needs to be configurable, wouldn't it be better to just let it
> be specified per port?

How could it be per-port? The base for a port is calculated as:

	base + <port_offset> + (port_no * <port_length>)

So, port_offset is clearly not per-port, there's only one of them. And
if we were to make <port_length> a per-port property, then the formula
would become a lot more complicated: we would have to iterate over each
port, and see what is the length of the register area for each of them,
to calculate the base address of the registers for the n-th port. This
is clearly a complexity that is not needed for my use case: all ports
have the same register area length, it's just that this length is
non-standard.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/5] ata: ahci: add support for non-standard port offset/length

[Tejun Heo]

Hello, Thomas.

On Wed, Apr 27, 2016 at 02:22:50PM +0200, Thomas Petazzoni wrote:
> I see a few possibilities:
> 
>  1/ Adjust all the drivers allocating manually an ahci_host_priv
>     structure, and make sure they initialize the fields to their default
>     values.
> 
>  2/ Add a new helper function that allows to allocate the
>     ahci_host_priv structure and initialize it with sane default values,
>     and use this helper in all drivers.
> 
>  3/ Add a flag in ahci_host_priv that tells whether non-standard port
>     offset/length are to be used. This is the most minimal solution,
>     but also maybe not the nicest one.

1 or 2 seems better to me.

> > If this needs to be configurable, wouldn't it be better to just let it
> > be specified per port?
> 
> How could it be per-port? The base for a port is calculated as:
> 
> 	base + <port_offset> + (port_no * <port_length>)
> 
> So, port_offset is clearly not per-port, there's only one of them. And
> if we were to make <port_length> a per-port property, then the formula
> would become a lot more complicated: we would have to iterate over each
> port, and see what is the length of the register area for each of them,
> to calculate the base address of the registers for the n-th port. This
> is clearly a complexity that is not needed for my use case: all ports
> have the same register area length, it's just that this length is
> non-standard.

Can't we just have hpriv->port_base?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] ata: acard-ahci: use the new port_{offset,length} members

[Thomas Petazzoni]

Now that the ahci_host_priv structure contains port_offset and
port_length members, this commit moves the acard-ahci driver to use
them rather than hardcoded values.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/ata/acard-ahci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
index ed6a30c..54c8ae4 100644
--- a/drivers/ata/acard-ahci.c
+++ b/drivers/ata/acard-ahci.c
@@ -476,7 +476,8 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
 
 		ata_port_pbar_desc(ap, AHCI_PCI_BAR, -1, "abar");
 		ata_port_pbar_desc(ap, AHCI_PCI_BAR,
-				   0x100 + ap->port_no * 0x80, "port");
+				   hpriv->port_offset +
+				   ap->port_no * hpriv->port_length, "port");
 
 		/* set initial link pm policy */
 		/*
-- 
2.6.4

[PATCH 3/5] ata: ahci_ceva: use the new port_{offset,length} members

[Thomas Petazzoni]

Now that the ahci_host_priv structure contains port_offset and
port_length members, this commit moves the ahci_ceva driver to use
them rather than hardcoded values.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/ata/ahci_ceva.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c
index 207649d..38611f2 100644
--- a/drivers/ata/ahci_ceva.c
+++ b/drivers/ata/ahci_ceva.c
@@ -79,8 +79,6 @@
 #define PORT_SCTL_SPD_GEN1	(0x1 << 4)
 #define PORT_SCTL_IPM		(0x3 << 8)
 
-#define PORT_BASE	0x100
-#define PORT_OFFSET	0x80
 #define NR_PORTS	2
 #define DRV_NAME	"ahci-ceva"
 #define CEVA_FLAG_BROKEN_GEN2	1
@@ -154,7 +152,8 @@ static void ahci_ceva_setup(struct ahci_host_priv *hpriv)
 		tmp = PORT_SCTL_SPD_GEN2 | PORT_SCTL_IPM;
 		if (cevapriv->flags & CEVA_FLAG_BROKEN_GEN2)
 			tmp = PORT_SCTL_SPD_GEN1 | PORT_SCTL_IPM;
-		writel(tmp, mmio + PORT_SCR_CTL + PORT_BASE + PORT_OFFSET * i);
+		writel(tmp, mmio + PORT_SCR_CTL + hpriv->port_offset +
+		       hpriv->port_length * i);
 	}
 }
 
-- 
2.6.4

[PATCH 4/5] ata: ahci_mvebu: add support for Armada 8K

[Thomas Petazzoni]

This commit adds support for the AHCI controller found in the Armada
7K/8K ARM64 SoCs. This controller uses non-standard port offset and
port length values, which explains why some special handling is
needed.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/ata/ahci_mvebu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index de7128d..b1d1d64 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -120,6 +120,10 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 
 		ahci_mvebu_mbus_config(hpriv, dram);
 		ahci_mvebu_regret_option(hpriv);
+	} else if (of_device_is_compatible(pdev->dev.of_node,
+					   "marvell,armada-8k-ahci")) {
+		hpriv->port_offset = 0x10000;
+		hpriv->port_length = 0x10000;
 	}
 
 	rc = ahci_platform_init_host(pdev, hpriv, &ahci_mvebu_port_info,
@@ -137,6 +141,7 @@ disable_resources:
 static const struct of_device_id ahci_mvebu_of_match[] = {
 	{ .compatible = "marvell,armada-380-ahci", },
 	{ .compatible = "marvell,armada-3700-ahci", },
+	{ .compatible = "marvell,armada-8k-ahci", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ahci_mvebu_of_match);
-- 
2.6.4

[PATCH 5/5] dt-bindings: ata: add compatible string for Armada 8K to ahci-platform

[Thomas Petazzoni]

In order to support the AHCI controller found in the Marvell Armada
7K/8K, a new compatible string is introduced in the Device Tree
binding for AHCI platform controllers.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 Documentation/devicetree/bindings/ata/ahci-platform.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 30df832..6b5acb7 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -15,6 +15,7 @@ Required properties:
   - "ibm,476gtr-ahci"
   - "marvell,armada-380-ahci"
   - "marvell,armada-3700-ahci"
+  - "marvell,armada-8k-ahci"
   - "snps,dwc-ahci"
   - "snps,exynos5440-ahci"
   - "snps,spear-ahci"
-- 
2.6.4

Re: [PATCH 5/5] dt-bindings: ata: add compatible string for Armada 8K to ahci-platform

[Rob Herring]

On Fri, Apr 22, 2016 at 04:32:41PM +0200, Thomas Petazzoni wrote:
> In order to support the AHCI controller found in the Marvell Armada
> 7K/8K, a new compatible string is introduced in the Device Tree
> binding for AHCI platform controllers.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/ata/ahci-platform.txt | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 0/5] ata: add support for Marvell Armada 7K/8K AHCI

[Thomas Petazzoni]

Tejun,

On Fri, 22 Apr 2016 16:32:36 +0200, Thomas Petazzoni wrote:

> The following small series adds support for the Marvell Armada 7K/8K
> AHCI controller. This controller is a standard AHCI controller, so not
> much is needed, except that it uses a non-default port offset and
> length. Due to this, some preparatory patches are used to make it
> possible to override the default port offset/length used by the AHCI
> core.

I recently learned that the non-standard port base/length is considered
as an HW issue, and it will be fixed in upcoming revisions of the SoC.
We are not interested in supporting the previous revisions that have
this bug, so I'm withdrawing this patch series.

Sorry for the noise and time spent there, I was not aware of the fact
that this issue would be fixed in HW.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com