[PATCH v2] ata: add AMD Seattle platform driver

[PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

AMD Seattle SATA controller mostly conforms to AHCI interface with some
special register to control SGPIO interface. In the case of an AHCI
controller, the SGPIO feature is ideally implemented using the
"Enclosure Management" register of the AHCI controller, but those
registeres are not implemented in the Seattle SoC. Instead SoC
(Rev B0 onwards) provides a 32-bit SGPIO control register which should
be programmed to control the activity, locate and fault LEDs.

The driver is based on ahci_platform driver.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
CC: tj@kernel.org
CC: linux-ide@vger.kernel.org
---
v1 patch and discussion is available here [1].

Changes since v1:
* use intverted test (fixes per Joe Perches review feedback)
* remove the DT binding, the DT support will be implemented by
  extending the generic driver to use LED trigger frameworks
  (recommend by Arnd Bergmann)
  
[1] https://lkml.org/lkml/2016/1/7/681

 drivers/ata/Kconfig        |   8 ++
 drivers/ata/Makefile       |   1 +
 drivers/ata/ahci_seattle.c | 210 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+)
 create mode 100644 drivers/ata/ahci_seattle.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 861643ea..f2ba9c0 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -193,6 +193,14 @@ config SATA_FSL
 
 	  If unsure, say N.
 
+config SATA_AHCI_SEATTLE
+	tristate "AMD Seattle 6.0Gbps AHCI SATA host controller support"
+	depends on ARCH_SEATTLE
+	help
+	 This option enables support for AMD Seattle SATA host controller.
+
+	 If unsure, say N
+
 config SATA_INIC162X
 	tristate "Initio 162x SATA support (Very Experimental)"
 	depends on PCI
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index af45eff..9c0a96a 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_ATA)		+= libata.o
 # non-SFF interface
 obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
 obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
+obj-$(CONFIG_SATA_AHCI_SEATTLE)	+= ahci_seattle.o libahci.o libahci_platform.o
 obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o libahci_platform.o
 obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
 obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
diff --git a/drivers/ata/ahci_seattle.c b/drivers/ata/ahci_seattle.c
new file mode 100644
index 0000000..6e702ab
--- /dev/null
+++ b/drivers/ata/ahci_seattle.c
@@ -0,0 +1,210 @@
+/*
+ * AMD Seattle AHCI SATA driver
+ *
+ * Copyright (c) 2015, Advanced Micro Devices
+ * Author: Brijesh Singh <brijesh.singh@amd.com>
+ *
+ * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/device.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/libata.h>
+#include <linux/ahci_platform.h>
+#include <linux/acpi.h>
+#include <linux/pci_ids.h>
+#include "ahci.h"
+
+/* SGPIO Control Register definition
+ *
+ * Bit		Type		Description
+ * 31		RW		OD7.2 (activity)
+ * 30		RW		OD7.1 (locate)
+ * 29		RW		OD7.0 (fault)
+ * 28...8	RW		OD6.2...OD0.0 (3bits per port, 1 bit per LED)
+ * 7		RO		SGPIO feature flag
+ * 6:4		RO		Reserved
+ * 3:0		RO		Number of ports (0 means no port supported)
+ */
+#define ACTIVITY_BIT_POS(x)		(8 + (3 * x))
+#define LOCATE_BIT_POS(x)		(ACTIVITY_BIT_POS(x) + 1)
+#define FAULT_BIT_POS(x)		(LOCATE_BIT_POS(x) + 1)
+
+#define ACTIVITY_MASK			0x00010000
+#define LOCATE_MASK			0x00080000
+#define FAULT_MASK			0x00400000
+
+#define DRV_NAME "ahci-seattle"
+
+static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state,
+					    ssize_t size);
+
+struct seattle_plat_data {
+	void __iomem *sgpio_ctrl;
+};
+
+static struct ata_port_operations ahci_port_ops = {
+	.inherits		= &ahci_ops,
+};
+
+static const struct ata_port_info ahci_port_info = {
+	.flags		= AHCI_FLAG_COMMON,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_port_ops,
+};
+
+static struct ata_port_operations ahci_seattle_ops = {
+	.inherits		= &ahci_ops,
+	.transmit_led_message   = seattle_transmit_led_message,
+};
+
+static const struct ata_port_info ahci_port_seattle_info = {
+	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_EM | ATA_FLAG_SW_ACTIVITY,
+	.link_flags	= ATA_LFLAG_SW_ACTIVITY,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_seattle_ops,
+};
+
+static struct scsi_host_template ahci_platform_sht = {
+	AHCI_SHT(DRV_NAME),
+};
+
+static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state,
+					    ssize_t size)
+{
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	struct seattle_plat_data *plat_data = hpriv->plat_data;
+	unsigned long flags;
+	int pmp;
+	struct ahci_em_priv *emp;
+	u32 val;
+
+	/* get the slot number from the message */
+	pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8;
+	if (pmp >= EM_MAX_SLOTS)
+		return -EINVAL;
+	emp = &pp->em_priv[pmp];
+
+	val = ioread32(plat_data->sgpio_ctrl);
+	if (state & ACTIVITY_MASK)
+		val |= 1 << ACTIVITY_BIT_POS((ap->port_no));
+	else
+		val &= ~(1 << ACTIVITY_BIT_POS((ap->port_no)));
+
+	if (state & LOCATE_MASK)
+		val |= 1 << LOCATE_BIT_POS((ap->port_no));
+	else
+		val &= ~(1 << LOCATE_BIT_POS((ap->port_no)));
+
+	if (state & FAULT_MASK)
+		val |= 1 << FAULT_BIT_POS((ap->port_no));
+	else
+		val &= ~(1 << FAULT_BIT_POS((ap->port_no)));
+
+	iowrite32(val, plat_data->sgpio_ctrl);
+
+	spin_lock_irqsave(ap->lock, flags);
+
+	/* save off new led state for port/slot */
+	emp->led_state = state;
+
+	spin_unlock_irqrestore(ap->lock, flags);
+
+	return size;
+}
+
+static const struct ata_port_info *ahci_seattle_get_port_info(
+		struct platform_device *pdev, struct ahci_host_priv *hpriv)
+{
+	struct device *dev = &pdev->dev;
+	struct seattle_plat_data *plat_data;
+	u32 val;
+
+	plat_data = devm_kzalloc(dev, sizeof(*plat_data), GFP_KERNEL);
+	if (IS_ERR(plat_data))
+		return &ahci_port_info;
+
+	plat_data->sgpio_ctrl = devm_ioremap_resource(dev,
+			      platform_get_resource(pdev, IORESOURCE_MEM, 1));
+	if (IS_ERR(plat_data->sgpio_ctrl))
+		return &ahci_port_info;
+
+	val = ioread32(plat_data->sgpio_ctrl);
+
+	if (!(val & 0xf))
+		return &ahci_port_info;
+
+	hpriv->em_loc = 0;
+	hpriv->em_buf_sz = 4;
+	hpriv->em_msg_type = EM_MSG_TYPE_LED;
+	hpriv->plat_data = plat_data;
+
+	dev_info(dev, "SGPIO LED control is enabled.\n");
+	return &ahci_port_seattle_info;
+}
+
+static int ahci_seattle_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct ahci_host_priv *hpriv;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
+
+	rc = ahci_platform_enable_resources(hpriv);
+	if (rc)
+		return rc;
+
+	rc = ahci_platform_init_host(pdev, hpriv,
+				     ahci_seattle_get_port_info(pdev, hpriv),
+				     &ahci_platform_sht);
+	if (rc)
+		goto disable_resources;
+
+	return 0;
+disable_resources:
+	ahci_platform_disable_resources(hpriv);
+	return rc;
+}
+
+static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_platform_suspend,
+			 ahci_platform_resume);
+
+static const struct acpi_device_id ahci_acpi_match[] = {
+	{ "AMDI0600", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
+
+static struct platform_driver ahci_seattle_driver = {
+	.probe = ahci_seattle_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = DRV_NAME,
+		.acpi_match_table = ahci_acpi_match,
+		.pm = &ahci_pm_ops,
+	},
+};
+module_platform_driver(ahci_seattle_driver);
+
+MODULE_DESCRIPTION("Seattle AHCI SATA platform driver");
+MODULE_AUTHOR("Brijesh Singh <brijesh.singh@amd.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
1.9.1

[PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

AMD Seattle SATA controller mostly conforms to AHCI interface with some
special register to control SGPIO interface. In the case of an AHCI
controller, the SGPIO feature is ideally implemented using the
"Enclosure Management" register of the AHCI controller, but those
registeres are not implemented in the Seattle SoC. Instead SoC
(Rev B0 onwards) provides a 32-bit SGPIO control register which should
be programmed to control the activity, locate and fault LEDs.

The driver is based on ahci_platform driver.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
CC: tj@kernel.org
CC: linux-ide@vger.kernel.org
---
v1 patch and discussion is available here [1].

Changes since v1:
* use intverted test (fixes per Joe Perches review feedback)
* remove the DT binding, the DT support will be implemented by
  extending the generic driver to use LED trigger frameworks
  (recommend by Arnd Bergmann)
  
[1] https://lkml.org/lkml/2016/1/7/681

 drivers/ata/Kconfig        |   8 ++
 drivers/ata/Makefile       |   1 +
 drivers/ata/ahci_seattle.c | 210 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+)
 create mode 100644 drivers/ata/ahci_seattle.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 861643ea..f2ba9c0 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -193,6 +193,14 @@ config SATA_FSL
 
 	  If unsure, say N.
 
+config SATA_AHCI_SEATTLE
+	tristate "AMD Seattle 6.0Gbps AHCI SATA host controller support"
+	depends on ARCH_SEATTLE
+	help
+	 This option enables support for AMD Seattle SATA host controller.
+
+	 If unsure, say N
+
 config SATA_INIC162X
 	tristate "Initio 162x SATA support (Very Experimental)"
 	depends on PCI
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index af45eff..9c0a96a 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_ATA)		+= libata.o
 # non-SFF interface
 obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
 obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
+obj-$(CONFIG_SATA_AHCI_SEATTLE)	+= ahci_seattle.o libahci.o libahci_platform.o
 obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o libahci_platform.o
 obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
 obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
diff --git a/drivers/ata/ahci_seattle.c b/drivers/ata/ahci_seattle.c
new file mode 100644
index 0000000..6e702ab
--- /dev/null
+++ b/drivers/ata/ahci_seattle.c
@@ -0,0 +1,210 @@
+/*
+ * AMD Seattle AHCI SATA driver
+ *
+ * Copyright (c) 2015, Advanced Micro Devices
+ * Author: Brijesh Singh <brijesh.singh@amd.com>
+ *
+ * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/device.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/libata.h>
+#include <linux/ahci_platform.h>
+#include <linux/acpi.h>
+#include <linux/pci_ids.h>
+#include "ahci.h"
+
+/* SGPIO Control Register definition
+ *
+ * Bit		Type		Description
+ * 31		RW		OD7.2 (activity)
+ * 30		RW		OD7.1 (locate)
+ * 29		RW		OD7.0 (fault)
+ * 28...8	RW		OD6.2...OD0.0 (3bits per port, 1 bit per LED)
+ * 7		RO		SGPIO feature flag
+ * 6:4		RO		Reserved
+ * 3:0		RO		Number of ports (0 means no port supported)
+ */
+#define ACTIVITY_BIT_POS(x)		(8 + (3 * x))
+#define LOCATE_BIT_POS(x)		(ACTIVITY_BIT_POS(x) + 1)
+#define FAULT_BIT_POS(x)		(LOCATE_BIT_POS(x) + 1)
+
+#define ACTIVITY_MASK			0x00010000
+#define LOCATE_MASK			0x00080000
+#define FAULT_MASK			0x00400000
+
+#define DRV_NAME "ahci-seattle"
+
+static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state,
+					    ssize_t size);
+
+struct seattle_plat_data {
+	void __iomem *sgpio_ctrl;
+};
+
+static struct ata_port_operations ahci_port_ops = {
+	.inherits		= &ahci_ops,
+};
+
+static const struct ata_port_info ahci_port_info = {
+	.flags		= AHCI_FLAG_COMMON,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_port_ops,
+};
+
+static struct ata_port_operations ahci_seattle_ops = {
+	.inherits		= &ahci_ops,
+	.transmit_led_message   = seattle_transmit_led_message,
+};
+
+static const struct ata_port_info ahci_port_seattle_info = {
+	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_EM | ATA_FLAG_SW_ACTIVITY,
+	.link_flags	= ATA_LFLAG_SW_ACTIVITY,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_seattle_ops,
+};
+
+static struct scsi_host_template ahci_platform_sht = {
+	AHCI_SHT(DRV_NAME),
+};
+
+static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state,
+					    ssize_t size)
+{
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	struct seattle_plat_data *plat_data = hpriv->plat_data;
+	unsigned long flags;
+	int pmp;
+	struct ahci_em_priv *emp;
+	u32 val;
+
+	/* get the slot number from the message */
+	pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8;
+	if (pmp >= EM_MAX_SLOTS)
+		return -EINVAL;
+	emp = &pp->em_priv[pmp];
+
+	val = ioread32(plat_data->sgpio_ctrl);
+	if (state & ACTIVITY_MASK)
+		val |= 1 << ACTIVITY_BIT_POS((ap->port_no));
+	else
+		val &= ~(1 << ACTIVITY_BIT_POS((ap->port_no)));
+
+	if (state & LOCATE_MASK)
+		val |= 1 << LOCATE_BIT_POS((ap->port_no));
+	else
+		val &= ~(1 << LOCATE_BIT_POS((ap->port_no)));
+
+	if (state & FAULT_MASK)
+		val |= 1 << FAULT_BIT_POS((ap->port_no));
+	else
+		val &= ~(1 << FAULT_BIT_POS((ap->port_no)));
+
+	iowrite32(val, plat_data->sgpio_ctrl);
+
+	spin_lock_irqsave(ap->lock, flags);
+
+	/* save off new led state for port/slot */
+	emp->led_state = state;
+
+	spin_unlock_irqrestore(ap->lock, flags);
+
+	return size;
+}
+
+static const struct ata_port_info *ahci_seattle_get_port_info(
+		struct platform_device *pdev, struct ahci_host_priv *hpriv)
+{
+	struct device *dev = &pdev->dev;
+	struct seattle_plat_data *plat_data;
+	u32 val;
+
+	plat_data = devm_kzalloc(dev, sizeof(*plat_data), GFP_KERNEL);
+	if (IS_ERR(plat_data))
+		return &ahci_port_info;
+
+	plat_data->sgpio_ctrl = devm_ioremap_resource(dev,
+			      platform_get_resource(pdev, IORESOURCE_MEM, 1));
+	if (IS_ERR(plat_data->sgpio_ctrl))
+		return &ahci_port_info;
+
+	val = ioread32(plat_data->sgpio_ctrl);
+
+	if (!(val & 0xf))
+		return &ahci_port_info;
+
+	hpriv->em_loc = 0;
+	hpriv->em_buf_sz = 4;
+	hpriv->em_msg_type = EM_MSG_TYPE_LED;
+	hpriv->plat_data = plat_data;
+
+	dev_info(dev, "SGPIO LED control is enabled.\n");
+	return &ahci_port_seattle_info;
+}
+
+static int ahci_seattle_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct ahci_host_priv *hpriv;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
+
+	rc = ahci_platform_enable_resources(hpriv);
+	if (rc)
+		return rc;
+
+	rc = ahci_platform_init_host(pdev, hpriv,
+				     ahci_seattle_get_port_info(pdev, hpriv),
+				     &ahci_platform_sht);
+	if (rc)
+		goto disable_resources;
+
+	return 0;
+disable_resources:
+	ahci_platform_disable_resources(hpriv);
+	return rc;
+}
+
+static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_platform_suspend,
+			 ahci_platform_resume);
+
+static const struct acpi_device_id ahci_acpi_match[] = {
+	{ "AMDI0600", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
+
+static struct platform_driver ahci_seattle_driver = {
+	.probe = ahci_seattle_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = DRV_NAME,
+		.acpi_match_table = ahci_acpi_match,
+		.pm = &ahci_pm_ops,
+	},
+};
+module_platform_driver(ahci_seattle_driver);
+
+MODULE_DESCRIPTION("Seattle AHCI SATA platform driver");
+MODULE_AUTHOR("Brijesh Singh <brijesh.singh@amd.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
1.9.1

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Tejun,

Ping ?

-Brijesh 

On 01/14/2016 10:31 AM, Brijesh Singh wrote:
> AMD Seattle SATA controller mostly conforms to AHCI interface with some
> special register to control SGPIO interface. In the case of an AHCI
> controller, the SGPIO feature is ideally implemented using the
> "Enclosure Management" register of the AHCI controller, but those
> registeres are not implemented in the Seattle SoC. Instead SoC
> (Rev B0 onwards) provides a 32-bit SGPIO control register which should
> be programmed to control the activity, locate and fault LEDs.
> 
> The driver is based on ahci_platform driver.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> CC: tj@kernel.org
> CC: linux-ide@vger.kernel.org
> ---
> v1 patch and discussion is available here [1].
> 
> Changes since v1:
> * use intverted test (fixes per Joe Perches review feedback)
> * remove the DT binding, the DT support will be implemented by
>   extending the generic driver to use LED trigger frameworks
>   (recommend by Arnd Bergmann)
>   
> [1] https://lkml.org/lkml/2016/1/7/681
> 
>  drivers/ata/Kconfig        |   8 ++
>  drivers/ata/Makefile       |   1 +
>  drivers/ata/ahci_seattle.c | 210 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 219 insertions(+)
>  create mode 100644 drivers/ata/ahci_seattle.c
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 861643ea..f2ba9c0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -193,6 +193,14 @@ config SATA_FSL
>  
>  	  If unsure, say N.
>  
> +config SATA_AHCI_SEATTLE
> +	tristate "AMD Seattle 6.0Gbps AHCI SATA host controller support"
> +	depends on ARCH_SEATTLE
> +	help
> +	 This option enables support for AMD Seattle SATA host controller.
> +
> +	 If unsure, say N
> +
>  config SATA_INIC162X
>  	tristate "Initio 162x SATA support (Very Experimental)"
>  	depends on PCI
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index af45eff..9c0a96a 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_ATA)		+= libata.o
>  # non-SFF interface
>  obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
>  obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
> +obj-$(CONFIG_SATA_AHCI_SEATTLE)	+= ahci_seattle.o libahci.o libahci_platform.o
>  obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o libahci_platform.o
>  obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
>  obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
> diff --git a/drivers/ata/ahci_seattle.c b/drivers/ata/ahci_seattle.c
> new file mode 100644
> index 0000000..6e702ab
> --- /dev/null
> +++ b/drivers/ata/ahci_seattle.c
> @@ -0,0 +1,210 @@
> +/*
> + * AMD Seattle AHCI SATA driver
> + *
> + * Copyright (c) 2015, Advanced Micro Devices
> + * Author: Brijesh Singh <brijesh.singh@amd.com>
> + *
> + * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/device.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/libata.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/acpi.h>
> +#include <linux/pci_ids.h>
> +#include "ahci.h"
> +
> +/* SGPIO Control Register definition
> + *
> + * Bit		Type		Description
> + * 31		RW		OD7.2 (activity)
> + * 30		RW		OD7.1 (locate)
> + * 29		RW		OD7.0 (fault)
> + * 28...8	RW		OD6.2...OD0.0 (3bits per port, 1 bit per LED)
> + * 7		RO		SGPIO feature flag
> + * 6:4		RO		Reserved
> + * 3:0		RO		Number of ports (0 means no port supported)
> + */
> +#define ACTIVITY_BIT_POS(x)		(8 + (3 * x))
> +#define LOCATE_BIT_POS(x)		(ACTIVITY_BIT_POS(x) + 1)
> +#define FAULT_BIT_POS(x)		(LOCATE_BIT_POS(x) + 1)
> +
> +#define ACTIVITY_MASK			0x00010000
> +#define LOCATE_MASK			0x00080000
> +#define FAULT_MASK			0x00400000
> +
> +#define DRV_NAME "ahci-seattle"
> +
> +static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state,
> +					    ssize_t size);
> +
> +struct seattle_plat_data {
> +	void __iomem *sgpio_ctrl;
> +};
> +
> +static struct ata_port_operations ahci_port_ops = {
> +	.inherits		= &ahci_ops,
> +};
> +
> +static const struct ata_port_info ahci_port_info = {
> +	.flags		= AHCI_FLAG_COMMON,
> +	.pio_mask	= ATA_PIO4,
> +	.udma_mask	= ATA_UDMA6,
> +	.port_ops	= &ahci_port_ops,
> +};
> +
> +static struct ata_port_operations ahci_seattle_ops = {
> +	.inherits		= &ahci_ops,
> +	.transmit_led_message   = seattle_transmit_led_message,
> +};
> +
> +static const struct ata_port_info ahci_port_seattle_info = {
> +	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_EM | ATA_FLAG_SW_ACTIVITY,
> +	.link_flags	= ATA_LFLAG_SW_ACTIVITY,
> +	.pio_mask	= ATA_PIO4,
> +	.udma_mask	= ATA_UDMA6,
> +	.port_ops	= &ahci_seattle_ops,
> +};
> +
> +static struct scsi_host_template ahci_platform_sht = {
> +	AHCI_SHT(DRV_NAME),
> +};
> +
> +static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state,
> +					    ssize_t size)
> +{
> +	struct ahci_host_priv *hpriv = ap->host->private_data;
> +	struct ahci_port_priv *pp = ap->private_data;
> +	struct seattle_plat_data *plat_data = hpriv->plat_data;
> +	unsigned long flags;
> +	int pmp;
> +	struct ahci_em_priv *emp;
> +	u32 val;
> +
> +	/* get the slot number from the message */
> +	pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8;
> +	if (pmp >= EM_MAX_SLOTS)
> +		return -EINVAL;
> +	emp = &pp->em_priv[pmp];
> +
> +	val = ioread32(plat_data->sgpio_ctrl);
> +	if (state & ACTIVITY_MASK)
> +		val |= 1 << ACTIVITY_BIT_POS((ap->port_no));
> +	else
> +		val &= ~(1 << ACTIVITY_BIT_POS((ap->port_no)));
> +
> +	if (state & LOCATE_MASK)
> +		val |= 1 << LOCATE_BIT_POS((ap->port_no));
> +	else
> +		val &= ~(1 << LOCATE_BIT_POS((ap->port_no)));
> +
> +	if (state & FAULT_MASK)
> +		val |= 1 << FAULT_BIT_POS((ap->port_no));
> +	else
> +		val &= ~(1 << FAULT_BIT_POS((ap->port_no)));
> +
> +	iowrite32(val, plat_data->sgpio_ctrl);
> +
> +	spin_lock_irqsave(ap->lock, flags);
> +
> +	/* save off new led state for port/slot */
> +	emp->led_state = state;
> +
> +	spin_unlock_irqrestore(ap->lock, flags);
> +
> +	return size;
> +}
> +
> +static const struct ata_port_info *ahci_seattle_get_port_info(
> +		struct platform_device *pdev, struct ahci_host_priv *hpriv)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct seattle_plat_data *plat_data;
> +	u32 val;
> +
> +	plat_data = devm_kzalloc(dev, sizeof(*plat_data), GFP_KERNEL);
> +	if (IS_ERR(plat_data))
> +		return &ahci_port_info;
> +
> +	plat_data->sgpio_ctrl = devm_ioremap_resource(dev,
> +			      platform_get_resource(pdev, IORESOURCE_MEM, 1));
> +	if (IS_ERR(plat_data->sgpio_ctrl))
> +		return &ahci_port_info;
> +
> +	val = ioread32(plat_data->sgpio_ctrl);
> +
> +	if (!(val & 0xf))
> +		return &ahci_port_info;
> +
> +	hpriv->em_loc = 0;
> +	hpriv->em_buf_sz = 4;
> +	hpriv->em_msg_type = EM_MSG_TYPE_LED;
> +	hpriv->plat_data = plat_data;
> +
> +	dev_info(dev, "SGPIO LED control is enabled.\n");
> +	return &ahci_port_seattle_info;
> +}
> +
> +static int ahci_seattle_probe(struct platform_device *pdev)
> +{
> +	int rc;
> +	struct ahci_host_priv *hpriv;
> +
> +	hpriv = ahci_platform_get_resources(pdev);
> +	if (IS_ERR(hpriv))
> +		return PTR_ERR(hpriv);
> +
> +	rc = ahci_platform_enable_resources(hpriv);
> +	if (rc)
> +		return rc;
> +
> +	rc = ahci_platform_init_host(pdev, hpriv,
> +				     ahci_seattle_get_port_info(pdev, hpriv),
> +				     &ahci_platform_sht);
> +	if (rc)
> +		goto disable_resources;
> +
> +	return 0;
> +disable_resources:
> +	ahci_platform_disable_resources(hpriv);
> +	return rc;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_platform_suspend,
> +			 ahci_platform_resume);
> +
> +static const struct acpi_device_id ahci_acpi_match[] = {
> +	{ "AMDI0600", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
> +
> +static struct platform_driver ahci_seattle_driver = {
> +	.probe = ahci_seattle_probe,
> +	.remove = ata_platform_remove_one,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.acpi_match_table = ahci_acpi_match,
> +		.pm = &ahci_pm_ops,
> +	},
> +};
> +module_platform_driver(ahci_seattle_driver);
> +
> +MODULE_DESCRIPTION("Seattle AHCI SATA platform driver");
> +MODULE_AUTHOR("Brijesh Singh <brijesh.singh@amd.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Tejun,

Ping ?

-Brijesh 

On 01/14/2016 10:31 AM, Brijesh Singh wrote:
> AMD Seattle SATA controller mostly conforms to AHCI interface with some
> special register to control SGPIO interface. In the case of an AHCI
> controller, the SGPIO feature is ideally implemented using the
> "Enclosure Management" register of the AHCI controller, but those
> registeres are not implemented in the Seattle SoC. Instead SoC
> (Rev B0 onwards) provides a 32-bit SGPIO control register which should
> be programmed to control the activity, locate and fault LEDs.
> 
> The driver is based on ahci_platform driver.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> CC: tj@kernel.org
> CC: linux-ide@vger.kernel.org
> ---
> v1 patch and discussion is available here [1].
> 
> Changes since v1:
> * use intverted test (fixes per Joe Perches review feedback)
> * remove the DT binding, the DT support will be implemented by
>   extending the generic driver to use LED trigger frameworks
>   (recommend by Arnd Bergmann)
>   
> [1] https://lkml.org/lkml/2016/1/7/681
> 
>  drivers/ata/Kconfig        |   8 ++
>  drivers/ata/Makefile       |   1 +
>  drivers/ata/ahci_seattle.c | 210 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 219 insertions(+)
>  create mode 100644 drivers/ata/ahci_seattle.c
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 861643ea..f2ba9c0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -193,6 +193,14 @@ config SATA_FSL
>  
>  	  If unsure, say N.
>  
> +config SATA_AHCI_SEATTLE
> +	tristate "AMD Seattle 6.0Gbps AHCI SATA host controller support"
> +	depends on ARCH_SEATTLE
> +	help
> +	 This option enables support for AMD Seattle SATA host controller.
> +
> +	 If unsure, say N
> +
>  config SATA_INIC162X
>  	tristate "Initio 162x SATA support (Very Experimental)"
>  	depends on PCI
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index af45eff..9c0a96a 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_ATA)		+= libata.o
>  # non-SFF interface
>  obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
>  obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
> +obj-$(CONFIG_SATA_AHCI_SEATTLE)	+= ahci_seattle.o libahci.o libahci_platform.o
>  obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o libahci_platform.o
>  obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
>  obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
> diff --git a/drivers/ata/ahci_seattle.c b/drivers/ata/ahci_seattle.c
> new file mode 100644
> index 0000000..6e702ab
> --- /dev/null
> +++ b/drivers/ata/ahci_seattle.c
> @@ -0,0 +1,210 @@
> +/*
> + * AMD Seattle AHCI SATA driver
> + *
> + * Copyright (c) 2015, Advanced Micro Devices
> + * Author: Brijesh Singh <brijesh.singh@amd.com>
> + *
> + * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/device.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/libata.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/acpi.h>
> +#include <linux/pci_ids.h>
> +#include "ahci.h"
> +
> +/* SGPIO Control Register definition
> + *
> + * Bit		Type		Description
> + * 31		RW		OD7.2 (activity)
> + * 30		RW		OD7.1 (locate)
> + * 29		RW		OD7.0 (fault)
> + * 28...8	RW		OD6.2...OD0.0 (3bits per port, 1 bit per LED)
> + * 7		RO		SGPIO feature flag
> + * 6:4		RO		Reserved
> + * 3:0		RO		Number of ports (0 means no port supported)
> + */
> +#define ACTIVITY_BIT_POS(x)		(8 + (3 * x))
> +#define LOCATE_BIT_POS(x)		(ACTIVITY_BIT_POS(x) + 1)
> +#define FAULT_BIT_POS(x)		(LOCATE_BIT_POS(x) + 1)
> +
> +#define ACTIVITY_MASK			0x00010000
> +#define LOCATE_MASK			0x00080000
> +#define FAULT_MASK			0x00400000
> +
> +#define DRV_NAME "ahci-seattle"
> +
> +static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state,
> +					    ssize_t size);
> +
> +struct seattle_plat_data {
> +	void __iomem *sgpio_ctrl;
> +};
> +
> +static struct ata_port_operations ahci_port_ops = {
> +	.inherits		= &ahci_ops,
> +};
> +
> +static const struct ata_port_info ahci_port_info = {
> +	.flags		= AHCI_FLAG_COMMON,
> +	.pio_mask	= ATA_PIO4,
> +	.udma_mask	= ATA_UDMA6,
> +	.port_ops	= &ahci_port_ops,
> +};
> +
> +static struct ata_port_operations ahci_seattle_ops = {
> +	.inherits		= &ahci_ops,
> +	.transmit_led_message   = seattle_transmit_led_message,
> +};
> +
> +static const struct ata_port_info ahci_port_seattle_info = {
> +	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_EM | ATA_FLAG_SW_ACTIVITY,
> +	.link_flags	= ATA_LFLAG_SW_ACTIVITY,
> +	.pio_mask	= ATA_PIO4,
> +	.udma_mask	= ATA_UDMA6,
> +	.port_ops	= &ahci_seattle_ops,
> +};
> +
> +static struct scsi_host_template ahci_platform_sht = {
> +	AHCI_SHT(DRV_NAME),
> +};
> +
> +static ssize_t seattle_transmit_led_message(struct ata_port *ap, u32 state,
> +					    ssize_t size)
> +{
> +	struct ahci_host_priv *hpriv = ap->host->private_data;
> +	struct ahci_port_priv *pp = ap->private_data;
> +	struct seattle_plat_data *plat_data = hpriv->plat_data;
> +	unsigned long flags;
> +	int pmp;
> +	struct ahci_em_priv *emp;
> +	u32 val;
> +
> +	/* get the slot number from the message */
> +	pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8;
> +	if (pmp >= EM_MAX_SLOTS)
> +		return -EINVAL;
> +	emp = &pp->em_priv[pmp];
> +
> +	val = ioread32(plat_data->sgpio_ctrl);
> +	if (state & ACTIVITY_MASK)
> +		val |= 1 << ACTIVITY_BIT_POS((ap->port_no));
> +	else
> +		val &= ~(1 << ACTIVITY_BIT_POS((ap->port_no)));
> +
> +	if (state & LOCATE_MASK)
> +		val |= 1 << LOCATE_BIT_POS((ap->port_no));
> +	else
> +		val &= ~(1 << LOCATE_BIT_POS((ap->port_no)));
> +
> +	if (state & FAULT_MASK)
> +		val |= 1 << FAULT_BIT_POS((ap->port_no));
> +	else
> +		val &= ~(1 << FAULT_BIT_POS((ap->port_no)));
> +
> +	iowrite32(val, plat_data->sgpio_ctrl);
> +
> +	spin_lock_irqsave(ap->lock, flags);
> +
> +	/* save off new led state for port/slot */
> +	emp->led_state = state;
> +
> +	spin_unlock_irqrestore(ap->lock, flags);
> +
> +	return size;
> +}
> +
> +static const struct ata_port_info *ahci_seattle_get_port_info(
> +		struct platform_device *pdev, struct ahci_host_priv *hpriv)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct seattle_plat_data *plat_data;
> +	u32 val;
> +
> +	plat_data = devm_kzalloc(dev, sizeof(*plat_data), GFP_KERNEL);
> +	if (IS_ERR(plat_data))
> +		return &ahci_port_info;
> +
> +	plat_data->sgpio_ctrl = devm_ioremap_resource(dev,
> +			      platform_get_resource(pdev, IORESOURCE_MEM, 1));
> +	if (IS_ERR(plat_data->sgpio_ctrl))
> +		return &ahci_port_info;
> +
> +	val = ioread32(plat_data->sgpio_ctrl);
> +
> +	if (!(val & 0xf))
> +		return &ahci_port_info;
> +
> +	hpriv->em_loc = 0;
> +	hpriv->em_buf_sz = 4;
> +	hpriv->em_msg_type = EM_MSG_TYPE_LED;
> +	hpriv->plat_data = plat_data;
> +
> +	dev_info(dev, "SGPIO LED control is enabled.\n");
> +	return &ahci_port_seattle_info;
> +}
> +
> +static int ahci_seattle_probe(struct platform_device *pdev)
> +{
> +	int rc;
> +	struct ahci_host_priv *hpriv;
> +
> +	hpriv = ahci_platform_get_resources(pdev);
> +	if (IS_ERR(hpriv))
> +		return PTR_ERR(hpriv);
> +
> +	rc = ahci_platform_enable_resources(hpriv);
> +	if (rc)
> +		return rc;
> +
> +	rc = ahci_platform_init_host(pdev, hpriv,
> +				     ahci_seattle_get_port_info(pdev, hpriv),
> +				     &ahci_platform_sht);
> +	if (rc)
> +		goto disable_resources;
> +
> +	return 0;
> +disable_resources:
> +	ahci_platform_disable_resources(hpriv);
> +	return rc;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_platform_suspend,
> +			 ahci_platform_resume);
> +
> +static const struct acpi_device_id ahci_acpi_match[] = {
> +	{ "AMDI0600", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
> +
> +static struct platform_driver ahci_seattle_driver = {
> +	.probe = ahci_seattle_probe,
> +	.remove = ata_platform_remove_one,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.acpi_match_table = ahci_acpi_match,
> +		.pm = &ahci_pm_ops,
> +	},
> +};
> +module_platform_driver(ahci_seattle_driver);
> +
> +MODULE_DESCRIPTION("Seattle AHCI SATA platform driver");
> +MODULE_AUTHOR("Brijesh Singh <brijesh.singh@amd.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Tejun Heo]

On Thu, Jan 14, 2016 at 10:31:11AM -0600, Brijesh Singh wrote:
> AMD Seattle SATA controller mostly conforms to AHCI interface with some
> special register to control SGPIO interface. In the case of an AHCI
> controller, the SGPIO feature is ideally implemented using the
> "Enclosure Management" register of the AHCI controller, but those
> registeres are not implemented in the Seattle SoC. Instead SoC
> (Rev B0 onwards) provides a 32-bit SGPIO control register which should
> be programmed to control the activity, locate and fault LEDs.
> 
> The driver is based on ahci_platform driver.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> CC: tj@kernel.org
> CC: linux-ide@vger.kernel.org

Hans, can you please review the patch?

Thanks.

-- 
tejun

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Hans de Goede]

Hi,

On 25-01-16 21:43, Tejun Heo wrote:
> On Thu, Jan 14, 2016 at 10:31:11AM -0600, Brijesh Singh wrote:
>> AMD Seattle SATA controller mostly conforms to AHCI interface with some
>> special register to control SGPIO interface. In the case of an AHCI
>> controller, the SGPIO feature is ideally implemented using the
>> "Enclosure Management" register of the AHCI controller, but those
>> registeres are not implemented in the Seattle SoC. Instead SoC
>> (Rev B0 onwards) provides a 32-bit SGPIO control register which should
>> be programmed to control the activity, locate and fault LEDs.
>>
>> The driver is based on ahci_platform driver.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> Acked-by: Hans de Goede <hdegoede@redhat.com>
>> CC: tj@kernel.org
>> CC: linux-ide@vger.kernel.org
>
> Hans, can you please review the patch?

Done, driver looks good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Arnd Bergmann]

On Monday 25 January 2016 15:43:00 Tejun Heo wrote:
> On Thu, Jan 14, 2016 at 10:31:11AM -0600, Brijesh Singh wrote:
> > AMD Seattle SATA controller mostly conforms to AHCI interface with some
> > special register to control SGPIO interface. In the case of an AHCI
> > controller, the SGPIO feature is ideally implemented using the
> > "Enclosure Management" register of the AHCI controller, but those
> > registeres are not implemented in the Seattle SoC. Instead SoC
> > (Rev B0 onwards) provides a 32-bit SGPIO control register which should
> > be programmed to control the activity, locate and fault LEDs.
> > 
> > The driver is based on ahci_platform driver.
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > CC: tj@kernel.org
> > CC: linux-ide@vger.kernel.org
> 
> Hans, can you please review the patch?
> 

I think it needs more work: The changelog describes it as a normal
driver, but based on the previous discussion, this is just a hack
to work around broken BIOS versions that can no longer be fixed in
the field, and there has not been a decision what the proper
representation should be in ACPI.

The patch also fails to address the devicetree based case, even though
we did come to a conclusion that the current behavior is a regression
(compared to what we had in drivers/ide/) and that there is a relatively
simple fix to do it right.

I'd rather see this problem solved for DT first and then have a
discussion about what the ACPI binding should look like, with
a review from ACPI folks before this hack gets cemented in the kernel.

	Arnd

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Arnd,

On 01/26/2016 06:17 AM, Arnd Bergmann wrote:
> 
> I think it needs more work: The changelog describes it as a normal
> driver, but based on the previous discussion, this is just a hack
> to work around broken BIOS versions that can no longer be fixed in
> the field, and there has not been a decision what the proper
> representation should be in ACPI.
> 
I am not sure if we should label this driver as a hack to workaround the
broken BIOS. Unfortunately SoC did not implemented the enclosure management
per spec. Its not BIOS issue.

> The patch also fails to address the devicetree based case, even though
> we did come to a conclusion that the current behavior is a regression
> (compared to what we had in drivers/ide/) and that there is a relatively
> simple fix to do it right.
> 
I did looked at your recommendation for extending libahci to use ledtrig_ide_activity()
but as I pointed out in previous discussion this function is missing several key features
from EM (enclosure management) pov. e.g missing the slot number, missing the locate and fault led.
In case of EM, each port will have at least three leds (activity, locate and fault). 
Since these LED's are part of EM hence we need to ensure that tools like ledmon and ledctl (which uses libahci sysfs) works well.

The main question is, what is recommended approach to override libachi enclosure managements
transfer led messages function? A platform driver or something else.

Tejun and/or Hans do you have any recommendation ?


- Brijesh

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Arnd,

On 01/26/2016 06:17 AM, Arnd Bergmann wrote:
> 
> I think it needs more work: The changelog describes it as a normal
> driver, but based on the previous discussion, this is just a hack
> to work around broken BIOS versions that can no longer be fixed in
> the field, and there has not been a decision what the proper
> representation should be in ACPI.
> 
I am not sure if we should label this driver as a hack to workaround the
broken BIOS. Unfortunately SoC did not implemented the enclosure management
per spec. Its not BIOS issue.

> The patch also fails to address the devicetree based case, even though
> we did come to a conclusion that the current behavior is a regression
> (compared to what we had in drivers/ide/) and that there is a relatively
> simple fix to do it right.
> 
I did looked at your recommendation for extending libahci to use ledtrig_ide_activity()
but as I pointed out in previous discussion this function is missing several key features
from EM (enclosure management) pov. e.g missing the slot number, missing the locate and fault led.
In case of EM, each port will have at least three leds (activity, locate and fault). 
Since these LED's are part of EM hence we need to ensure that tools like ledmon and ledctl (which uses libahci sysfs) works well.

The main question is, what is recommended approach to override libachi enclosure managements
transfer led messages function? A platform driver or something else.

Tejun and/or Hans do you have any recommendation ?


- Brijesh

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Arnd Bergmann]

On Tuesday 26 January 2016 10:56:20 Brijesh Singh wrote:
> 
> On 01/26/2016 06:17 AM, Arnd Bergmann wrote:
> > 
> > I think it needs more work: The changelog describes it as a normal
> > driver, but based on the previous discussion, this is just a hack
> > to work around broken BIOS versions that can no longer be fixed in
> > the field, and there has not been a decision what the proper
> > representation should be in ACPI.
> > 
> I am not sure if we should label this driver as a hack to workaround the
> broken BIOS. Unfortunately SoC did not implemented the enclosure management
> per spec. Its not BIOS issue.

The BIOS issue is how the hardware workaround is represented. The
idea of ACPI is to hide this kind of glitch from the operating system
instead of forcing it to have a driver for each hardware variant.

> > The patch also fails to address the devicetree based case, even though
> > we did come to a conclusion that the current behavior is a regression
> > (compared to what we had in drivers/ide/) and that there is a relatively
> > simple fix to do it right.
> > 
> I did looked at your recommendation for extending libahci to use ledtrig_ide_activity()
> but as I pointed out in previous discussion this function is missing several key features
> from EM (enclosure management) pov. e.g missing the slot number, missing the locate and fault led.
> In case of EM, each port will have at least three leds (activity, locate and fault). 
> Since these LED's are part of EM hence we need to ensure that tools like ledmon and ledctl (which uses libahci sysfs) works well.

I'd assume this can be easily extended, we just need to come up with
a naming scheme for the LEDs so we can identify them in DT.

> The main question is, what is recommended approach to override libachi enclosure managements
> transfer led messages function? A platform driver or something else.
> 
> Tejun and/or Hans do you have any recommendation ?

I think for the DT case, a platform driver that registers itself to the LED
subsystem is the best way. You probably still want to put the hardware
register into a "syscon" device that contains the entire set of registers
around it (presumably more hacks for other hardware features), and then
just reference the register using a regmap from the LED driver. The generic
AHCI driver can then get extended with supports for the LED subsystem, to
look for specific LEDs by name based on lot number and type of LED.

Those can be present on any machine with a generic AHCI implementation,
and can be easily implemented using the GPIO-LED driver on machines that
don't have a special purpose register for them.

For the ACPI case, I still think that an AML call from the AHCI driver
is the most logical solution. You mentioned that you believe that calling
into the AML interpreter up to 100 times per second is a noticeable
overhead, but I doubt that and would like to see actual number backing
that up. Note that most of the time, the status of the LEDs won't even
change, so the driver does not have to call into the AML while I/O
is in progress, or while it is stopped, only for the transition or in
case of locate and fault events that should be extremely rare.

	Arnd

Re: [PATCH v2] ata: add AMD Seattle platform driver

[One Thousand Gnomes]

> For the ACPI case, I still think that an AML call from the AHCI driver
> is the most logical solution. You mentioned that you believe that calling
> into the AML interpreter up to 100 times per second is a noticeable
> overhead, but I doubt that and would like to see actual number backing
> that up. Note that most of the time, the status of the LEDs won't even
> change, so the driver does not have to call into the AML while I/O
> is in progress, or while it is stopped, only for the transition or in
> case of locate and fault events that should be extremely rare.

It can also be delayed - you don't need to adjust the LED at any great
rate - it's for a human not a machine to read.

Alan

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Arnd,

On 01/29/2016 03:22 PM, Arnd Bergmann wrote:
> 
> For the ACPI case, I still think that an AML call from the AHCI driver
> is the most logical solution. You mentioned that you believe that calling
> into the AML interpreter up to 100 times per second is a noticeable
> overhead, but I doubt that and would like to see actual number backing
> that up. Note that most of the time, the status of the LEDs won't even
> change, so the driver does not have to call into the AML while I/O
> is in progress, or while it is stopped, only for the transition or in
> case of locate and fault events that should be extremely rare.
>
During disk activity ahci_sw_activity_blink() is called based on timer expiration (~100ms).
I just enabled the function profiler for 'dd if=/dev/zero of=/root/tmp bs=1M count=4096' and see the output below. The function was called 37 times in 2.5s
 
#echo 1 > function_profile_enabled

#dd if=/dev/zero of=/root/tempfile bs=1M count=4096
4096+0 records in
4096+0 records out
4294967296 bytes (4.3 GB) copied, 2.57334 s, 1.7 GB/s

#echo 0 > function_profile_enabled

#cat trace_stat/*
Function                               Hit    Time            Avg             s^2
--------                               ---    ----            ---             ---
seattle_transmit_led_message            37    25.088 us       0.678 us        0.050 us    

I am not debating on your AML call recommendation, it sounds like a good idea however BIOS is already released hence its bit late to add AML methods for this. I am seeking guidance on what can be done in the given situation. I thought platform driver is one option to get this feature enabled in kernel.

- Brijesh

> 	Arnd
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Arnd,

On 01/29/2016 03:22 PM, Arnd Bergmann wrote:
> 
> For the ACPI case, I still think that an AML call from the AHCI driver
> is the most logical solution. You mentioned that you believe that calling
> into the AML interpreter up to 100 times per second is a noticeable
> overhead, but I doubt that and would like to see actual number backing
> that up. Note that most of the time, the status of the LEDs won't even
> change, so the driver does not have to call into the AML while I/O
> is in progress, or while it is stopped, only for the transition or in
> case of locate and fault events that should be extremely rare.
>
During disk activity ahci_sw_activity_blink() is called based on timer expiration (~100ms).
I just enabled the function profiler for 'dd if=/dev/zero of=/root/tmp bs=1M count=4096' and see the output below. The function was called 37 times in 2.5s
 
#echo 1 > function_profile_enabled

#dd if=/dev/zero of=/root/tempfile bs=1M count=4096
4096+0 records in
4096+0 records out
4294967296 bytes (4.3 GB) copied, 2.57334 s, 1.7 GB/s

#echo 0 > function_profile_enabled

#cat trace_stat/*
Function                               Hit    Time            Avg             s^2
--------                               ---    ----            ---             ---
seattle_transmit_led_message            37    25.088 us       0.678 us        0.050 us    

I am not debating on your AML call recommendation, it sounds like a good idea however BIOS is already released hence its bit late to add AML methods for this. I am seeking guidance on what can be done in the given situation. I thought platform driver is one option to get this feature enabled in kernel.

- Brijesh

> 	Arnd
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Arnd Bergmann]

On Monday 01 February 2016 12:56:06 Brijesh Singh wrote:
> On 01/29/2016 03:22 PM, Arnd Bergmann wrote:
> > 
> > For the ACPI case, I still think that an AML call from the AHCI driver
> > is the most logical solution. You mentioned that you believe that calling
> > into the AML interpreter up to 100 times per second is a noticeable
> > overhead, but I doubt that and would like to see actual number backing
> > that up. Note that most of the time, the status of the LEDs won't even
> > change, so the driver does not have to call into the AML while I/O
> > is in progress, or while it is stopped, only for the transition or in
> > case of locate and fault events that should be extremely rare.
> >
> During disk activity ahci_sw_activity_blink() is called based on timer expiration (~100ms).
> I just enabled the function profiler for 'dd if=/dev/zero of=/root/tmp bs=1M count=4096' and see the output below. The function was called 37 times in 2.5s
>  
> #echo 1 > function_profile_enabled
> 
> #dd if=/dev/zero of=/root/tempfile bs=1M count=4096
> 4096+0 records in
> 4096+0 records out
> 4294967296 bytes (4.3 GB) copied, 2.57334 s, 1.7 GB/s
> 
> #echo 0 > function_profile_enabled
> 
> #cat trace_stat/*
> Function                               Hit    Time            Avg             s^2
> --------                               ---    ----            ---             ---
> seattle_transmit_led_message            37    25.088 us       0.678 us        0.050 us    
> 
> I am not debating on your AML call recommendation, it sounds like a good idea however BIOS is already released hence its bit late to add AML methods for this. I am seeking guidance on what can be done in the given situation. I thought platform driver is one option to get this feature enabled in kernel.

This is where we really need the ACPI maintainers to explain the
general policy for dealing with firmware updates.

I would assume that adding the feature in a later firmware version
is a compatible change, and the feature is non-essential (the
device will work fine with the generic SATA driver, except
the LEDs don't blink), so it's not a big deal, it's just what
you get for having the firmware shipped before the driver is
reviewed (don't do that).

	Arnd

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi,

> 
> This is where we really need the ACPI maintainers to explain the
> general policy for dealing with firmware updates.
> 
> I would assume that adding the feature in a later firmware version
> is a compatible change, and the feature is non-essential (the
> device will work fine with the generic SATA driver, except
> the LEDs don't blink), so it's not a big deal, it's just what
> you get for having the firmware shipped before the driver is
> reviewed (don't do that).
> 

Agreed, the driver should have been reviewed earlier. And now changes in firmware will also require
them changing other OSes drivers.

Just speaking my mind, maybe server folks will be fine without activity LED but I think "locate" and "fault" LEDs is needed.

-Brijesh

> 	Arnd
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi,

> 
> This is where we really need the ACPI maintainers to explain the
> general policy for dealing with firmware updates.
> 
> I would assume that adding the feature in a later firmware version
> is a compatible change, and the feature is non-essential (the
> device will work fine with the generic SATA driver, except
> the LEDs don't blink), so it's not a big deal, it's just what
> you get for having the firmware shipped before the driver is
> reviewed (don't do that).
> 

Agreed, the driver should have been reviewed earlier. And now changes in firmware will also require
them changing other OSes drivers.

Just speaking my mind, maybe server folks will be fine without activity LED but I think "locate" and "fault" LEDs is needed.

-Brijesh

> 	Arnd
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Arnd Bergmann]

On Monday 01 February 2016 16:15:59 Brijesh Singh wrote:
> > 
> > This is where we really need the ACPI maintainers to explain the
> > general policy for dealing with firmware updates.
> > 
> > I would assume that adding the feature in a later firmware version
> > is a compatible change, and the feature is non-essential (the
> > device will work fine with the generic SATA driver, except
> > the LEDs don't blink), so it's not a big deal, it's just what
> > you get for having the firmware shipped before the driver is
> > reviewed (don't do that).
> > 
> 
> Agreed, the driver should have been reviewed earlier. And now changes in firmware will also require
> them changing other OSes drivers.

Can you explain that? I would expect the addition of some AML methods
to be a compatible change.

	Arnd

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi,

On 02/02/2016 08:08 AM, Arnd Bergmann wrote:
> On Monday 01 February 2016 16:15:59 Brijesh Singh wrote:
>>>
>>> This is where we really need the ACPI maintainers to explain the
>>> general policy for dealing with firmware updates.
>>>
>>> I would assume that adding the feature in a later firmware version
>>> is a compatible change, and the feature is non-essential (the
>>> device will work fine with the generic SATA driver, except
>>> the LEDs don't blink), so it's not a big deal, it's just what
>>> you get for having the firmware shipped before the driver is
>>> reviewed (don't do that).
>>>
>>
>> Agreed, the driver should have been reviewed earlier. And now changes in firmware will also require
>> them changing other OSes drivers.
> 
> Can you explain that? I would expect the addition of some AML methods
> to be a compatible change.
> 

current DSDT entry looks like this:

Device (SATA0)
{
.....

 Name(_CRS, ResourceTemplate()
 {
   Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)  /* SATA block address */
   Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
   Memory32Fixed(ReadWrite, 0xE00000078, 1)  /* SGPIO register */
}
  
......
}

Windows driver folks were okay to look at second resource field to map the SGPIO register and program the
registers to blink the LEDs. I think as per ACPI spec, its legal to pass more than one block in resource
template and since AML method is not mandatory for non standard enclosure management hence its entirely
possible that some BIOS vendors may not implement it at all. But if they implement and decide
to expose either AML method or register map but not both then Windows driver may break.

I believe most of BIOS vendors are using above DSDT block for SATA and by implementing the platform driver
we could enable this feature right away in Linux OS. We do prefer to use generic driver however given the current situation platform driver seems a reasonable choice. I wish SoC designer had implemented the full 
enclosure management per SATA spec.


> 	Arnd
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi,

On 02/02/2016 08:08 AM, Arnd Bergmann wrote:
> On Monday 01 February 2016 16:15:59 Brijesh Singh wrote:
>>>
>>> This is where we really need the ACPI maintainers to explain the
>>> general policy for dealing with firmware updates.
>>>
>>> I would assume that adding the feature in a later firmware version
>>> is a compatible change, and the feature is non-essential (the
>>> device will work fine with the generic SATA driver, except
>>> the LEDs don't blink), so it's not a big deal, it's just what
>>> you get for having the firmware shipped before the driver is
>>> reviewed (don't do that).
>>>
>>
>> Agreed, the driver should have been reviewed earlier. And now changes in firmware will also require
>> them changing other OSes drivers.
> 
> Can you explain that? I would expect the addition of some AML methods
> to be a compatible change.
> 

current DSDT entry looks like this:

Device (SATA0)
{
.....

 Name(_CRS, ResourceTemplate()
 {
   Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)  /* SATA block address */
   Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
   Memory32Fixed(ReadWrite, 0xE00000078, 1)  /* SGPIO register */
}
  
......
}

Windows driver folks were okay to look at second resource field to map the SGPIO register and program the
registers to blink the LEDs. I think as per ACPI spec, its legal to pass more than one block in resource
template and since AML method is not mandatory for non standard enclosure management hence its entirely
possible that some BIOS vendors may not implement it at all. But if they implement and decide
to expose either AML method or register map but not both then Windows driver may break.

I believe most of BIOS vendors are using above DSDT block for SATA and by implementing the platform driver
we could enable this feature right away in Linux OS. We do prefer to use generic driver however given the current situation platform driver seems a reasonable choice. I wish SoC designer had implemented the full 
enclosure management per SATA spec.


> 	Arnd
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Arnd Bergmann]

On Tuesday 02 February 2016 12:37:58 Brijesh Singh wrote:
> Hi,
> 
> On 02/02/2016 08:08 AM, Arnd Bergmann wrote:
> > On Monday 01 February 2016 16:15:59 Brijesh Singh wrote:
> >>>
> >>> This is where we really need the ACPI maintainers to explain the
> >>> general policy for dealing with firmware updates.
> >>>
> >>> I would assume that adding the feature in a later firmware version
> >>> is a compatible change, and the feature is non-essential (the
> >>> device will work fine with the generic SATA driver, except
> >>> the LEDs don't blink), so it's not a big deal, it's just what
> >>> you get for having the firmware shipped before the driver is
> >>> reviewed (don't do that).
> >>>
> >>
> >> Agreed, the driver should have been reviewed earlier. And now changes in firmware will also require
> >> them changing other OSes drivers.
> > 
> > Can you explain that? I would expect the addition of some AML methods
> > to be a compatible change.
> > 
> 
> current DSDT entry looks like this:
> 
> Device (SATA0)
> {
> .....
> 
>  Name(_CRS, ResourceTemplate()
>  {
>    Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)  /* SATA block address */
>    Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
>    Memory32Fixed(ReadWrite, 0xE00000078, 1)  /* SGPIO register */
> }
>   
> ......
> }
> 
> Windows driver folks were okay to look at second resource field to map the SGPIO register and program the
> registers to blink the LEDs. I think as per ACPI spec, its legal to pass more than one block in resource
> template and since AML method is not mandatory for non standard enclosure management hence its entirely
> possible that some BIOS vendors may not implement it at all. But if they implement and decide
> to expose either AML method or register map but not both then Windows driver may break.

I don't have access to the Windows source code. Is this in the
architecture-independent part of their kernel, or only done on ARM64?
How do they decide what the second memory range is for?

If this is now a de-facto extension to the PCI_CLASS_STORAGE_SATA_AHCI binding,
it should probably be put into the next version of the AHCI spec, and then
there is no problem using it.


	Arnd

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi,

>> }
>>
>> Windows driver folks were okay to look at second resource field to map the SGPIO register and program the
>> registers to blink the LEDs. I think as per ACPI spec, its legal to pass more than one block in resource
>> template and since AML method is not mandatory for non standard enclosure management hence its entirely
>> possible that some BIOS vendors may not implement it at all. But if they implement and decide
>> to expose either AML method or register map but not both then Windows driver may break.
> 
> I don't have access to the Windows source code. Is this in the
> architecture-independent part of their kernel, or only done on ARM64?
> How do they decide what the second memory range is for?
> 
> If this is now a de-facto extension to the PCI_CLASS_STORAGE_SATA_AHCI binding,
> it should probably be put into the next version of the AHCI spec, and then
> there is no problem using it.
> 
I don't have Windows code either and do not know the implementation details. I was told by the AMD folks 
working on Windows drivers for Seattle that they do not need any changes in BIOS DSDT to get the LEDs blinking.

This is not a de-facto extension of SATA_AHCI binding, you can call this method as a SoC hack to support the LEDs.
We are working with whatever BIOS is already available to enable the LEDs blinking.

> 
> 	Arnd
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi,

>> }
>>
>> Windows driver folks were okay to look at second resource field to map the SGPIO register and program the
>> registers to blink the LEDs. I think as per ACPI spec, its legal to pass more than one block in resource
>> template and since AML method is not mandatory for non standard enclosure management hence its entirely
>> possible that some BIOS vendors may not implement it at all. But if they implement and decide
>> to expose either AML method or register map but not both then Windows driver may break.
> 
> I don't have access to the Windows source code. Is this in the
> architecture-independent part of their kernel, or only done on ARM64?
> How do they decide what the second memory range is for?
> 
> If this is now a de-facto extension to the PCI_CLASS_STORAGE_SATA_AHCI binding,
> it should probably be put into the next version of the AHCI spec, and then
> there is no problem using it.
> 
I don't have Windows code either and do not know the implementation details. I was told by the AMD folks 
working on Windows drivers for Seattle that they do not need any changes in BIOS DSDT to get the LEDs blinking.

This is not a de-facto extension of SATA_AHCI binding, you can call this method as a SoC hack to support the LEDs.
We are working with whatever BIOS is already available to enable the LEDs blinking.

> 
> 	Arnd
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Arnd,

On 02/05/2016 11:23 AM, Brijesh Singh wrote:
> Hi,
> 
>>> }
>>>
>>> Windows driver folks were okay to look at second resource field to map the SGPIO register and program the
>>> registers to blink the LEDs. I think as per ACPI spec, its legal to pass more than one block in resource
>>> template and since AML method is not mandatory for non standard enclosure management hence its entirely
>>> possible that some BIOS vendors may not implement it at all. But if they implement and decide
>>> to expose either AML method or register map but not both then Windows driver may break.
>>
>> I don't have access to the Windows source code. Is this in the
>> architecture-independent part of their kernel, or only done on ARM64?
>> How do they decide what the second memory range is for?
>>
>> If this is now a de-facto extension to the PCI_CLASS_STORAGE_SATA_AHCI binding,
>> it should probably be put into the next version of the AHCI spec, and then
>> there is no problem using it.
>>
> I don't have Windows code either and do not know the implementation details. I was told by the AMD folks 
> working on Windows drivers for Seattle that they do not need any changes in BIOS DSDT to get the LEDs blinking.
> 
> This is not a de-facto extension of SATA_AHCI binding, you can call this method as a SoC hack to support the LEDs.
> We are working with whatever BIOS is already available to enable the LEDs blinking.
> 

I am not sure what I can do next, given the SoC and BIOS limitation it seems like platform driver is best choice to enable this feature.
Do you have any review feedback on driver itself ? if not, then can we get this patch in?

-Brijesh

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Arnd,

On 02/05/2016 11:23 AM, Brijesh Singh wrote:
> Hi,
> 
>>> }
>>>
>>> Windows driver folks were okay to look at second resource field to map the SGPIO register and program the
>>> registers to blink the LEDs. I think as per ACPI spec, its legal to pass more than one block in resource
>>> template and since AML method is not mandatory for non standard enclosure management hence its entirely
>>> possible that some BIOS vendors may not implement it at all. But if they implement and decide
>>> to expose either AML method or register map but not both then Windows driver may break.
>>
>> I don't have access to the Windows source code. Is this in the
>> architecture-independent part of their kernel, or only done on ARM64?
>> How do they decide what the second memory range is for?
>>
>> If this is now a de-facto extension to the PCI_CLASS_STORAGE_SATA_AHCI binding,
>> it should probably be put into the next version of the AHCI spec, and then
>> there is no problem using it.
>>
> I don't have Windows code either and do not know the implementation details. I was told by the AMD folks 
> working on Windows drivers for Seattle that they do not need any changes in BIOS DSDT to get the LEDs blinking.
> 
> This is not a de-facto extension of SATA_AHCI binding, you can call this method as a SoC hack to support the LEDs.
> We are working with whatever BIOS is already available to enable the LEDs blinking.
> 

I am not sure what I can do next, given the SoC and BIOS limitation it seems like platform driver is best choice to enable this feature.
Do you have any review feedback on driver itself ? if not, then can we get this patch in?

-Brijesh

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Tejun,


On 01/26/2016 03:36 AM, Hans de Goede wrote:
> Hi,
> 
> On 25-01-16 21:43, Tejun Heo wrote:
>> On Thu, Jan 14, 2016 at 10:31:11AM -0600, Brijesh Singh wrote:
>>> AMD Seattle SATA controller mostly conforms to AHCI interface with some
>>> special register to control SGPIO interface. In the case of an AHCI
>>> controller, the SGPIO feature is ideally implemented using the
>>> "Enclosure Management" register of the AHCI controller, but those
>>> registeres are not implemented in the Seattle SoC. Instead SoC
>>> (Rev B0 onwards) provides a 32-bit SGPIO control register which should
>>> be programmed to control the activity, locate and fault LEDs.
>>>
>>> The driver is based on ahci_platform driver.
>>>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>> Acked-by: Hans de Goede <hdegoede@redhat.com>
>>> CC: tj@kernel.org
>>> CC: linux-ide@vger.kernel.org
>>
>> Hans, can you please review the patch?
> 
> Done, driver looks good to me:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
>

Can we please pull this patch ? If needed then I can rebase to the tip. Given the current HW and BIOS limitation, platform driver seems like a right choice.

Thanks
Brijesh
 
> Regards,
> 
> Hans

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Tejun,


On 01/26/2016 03:36 AM, Hans de Goede wrote:
> Hi,
> 
> On 25-01-16 21:43, Tejun Heo wrote:
>> On Thu, Jan 14, 2016 at 10:31:11AM -0600, Brijesh Singh wrote:
>>> AMD Seattle SATA controller mostly conforms to AHCI interface with some
>>> special register to control SGPIO interface. In the case of an AHCI
>>> controller, the SGPIO feature is ideally implemented using the
>>> "Enclosure Management" register of the AHCI controller, but those
>>> registeres are not implemented in the Seattle SoC. Instead SoC
>>> (Rev B0 onwards) provides a 32-bit SGPIO control register which should
>>> be programmed to control the activity, locate and fault LEDs.
>>>
>>> The driver is based on ahci_platform driver.
>>>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>> Acked-by: Hans de Goede <hdegoede@redhat.com>
>>> CC: tj@kernel.org
>>> CC: linux-ide@vger.kernel.org
>>
>> Hans, can you please review the patch?
> 
> Done, driver looks good to me:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
>

Can we please pull this patch ? If needed then I can rebase to the tip. Given the current HW and BIOS limitation, platform driver seems like a right choice.

Thanks
Brijesh
 
> Regards,
> 
> Hans

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Tejun Heo]

Hello, Arnd.

On Mon, Feb 01, 2016 at 09:14:17PM +0100, Arnd Bergmann wrote:
>> I am not debating on your AML call recommendation, it sounds like
>> a good idea however BIOS is already released hence its bit late to
>> add AML methods for this. I am seeking guidance on what can be
>> done in the given situation. I thought platform driver is one
>> option to get this feature enabled in kernel.

> This is where we really need the ACPI maintainers to explain the
> general policy for dealing with firmware updates.
> 
> I would assume that adding the feature in a later firmware version
> is a compatible change, and the feature is non-essential (the
> device will work fine with the generic SATA driver, except
> the LEDs don't blink), so it's not a big deal, it's just what
> you get for having the firmware shipped before the driver is
> reviewed (don't do that).

So, if it were x86, I'd commit the custom driver without thinking too
much as ata drivers have always been working around bios issues (there
often wasn't any other recourse).  If the hardware is already out
there and it's not too easy to roll out bios updates, from libata
side, I'm okay with having a custom driver to work around that.  What
do you think?

Thanks.

-- 
tejun

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Arnd Bergmann]

On Wednesday 16 March 2016 14:07:13 Tejun Heo wrote:
> Hello, Arnd.
> 
> On Mon, Feb 01, 2016 at 09:14:17PM +0100, Arnd Bergmann wrote:
> >> I am not debating on your AML call recommendation, it sounds like
> >> a good idea however BIOS is already released hence its bit late to
> >> add AML methods for this. I am seeking guidance on what can be
> >> done in the given situation. I thought platform driver is one
> >> option to get this feature enabled in kernel.
> 
> > This is where we really need the ACPI maintainers to explain the
> > general policy for dealing with firmware updates.
> > 
> > I would assume that adding the feature in a later firmware version
> > is a compatible change, and the feature is non-essential (the
> > device will work fine with the generic SATA driver, except
> > the LEDs don't blink), so it's not a big deal, it's just what
> > you get for having the firmware shipped before the driver is
> > reviewed (don't do that).
> 
> So, if it were x86, I'd commit the custom driver without thinking too
> much as ata drivers have always been working around bios issues (there
> often wasn't any other recourse).  If the hardware is already out
> there and it's not too easy to roll out bios updates, from libata
> side, I'm okay with having a custom driver to work around that.  What
> do you think?


It's your call in the end. My main objection is to the fact that
I have suggested a clean implementation for the normal DT based
path that also fixes existing platforms that used to work in the
past and were broken by the (long-ago) move from drivers/ide to
drivers/ata, Brijesh has not implemented that but has instead
continued pushing the hack for the ACPI mode that is still
experimental on ARM64.

	Arnd

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Tejun,

On 03/17/2016 12:36 PM, Arnd Bergmann wrote:
> On Wednesday 16 March 2016 14:07:13 Tejun Heo wrote:
>> Hello, Arnd.
>>
>> On Mon, Feb 01, 2016 at 09:14:17PM +0100, Arnd Bergmann wrote:
>>>> I am not debating on your AML call recommendation, it sounds like
>>>> a good idea however BIOS is already released hence its bit late to
>>>> add AML methods for this. I am seeking guidance on what can be
>>>> done in the given situation. I thought platform driver is one
>>>> option to get this feature enabled in kernel.
>>
>>> This is where we really need the ACPI maintainers to explain the
>>> general policy for dealing with firmware updates.
>>>
>>> I would assume that adding the feature in a later firmware version
>>> is a compatible change, and the feature is non-essential (the
>>> device will work fine with the generic SATA driver, except
>>> the LEDs don't blink), so it's not a big deal, it's just what
>>> you get for having the firmware shipped before the driver is
>>> reviewed (don't do that).
>>
>> So, if it were x86, I'd commit the custom driver without thinking too
>> much as ata drivers have always been working around bios issues (there
>> often wasn't any other recourse).  If the hardware is already out
>> there and it's not too easy to roll out bios updates, from libata
>> side, I'm okay with having a custom driver to work around that.  What
>> do you think?
> 
> 
> It's your call in the end. My main objection is to the fact that
> I have suggested a clean implementation for the normal DT based
> path that also fixes existing platforms that used to work in the
> past and were broken by the (long-ago) move from drivers/ide to
> drivers/ata, Brijesh has not implemented that but has instead
> continued pushing the hack for the ACPI mode that is still
> experimental on ARM64.
>

I am helping a customer who want EM support in a distro (using ACPI mode). Since its difficult to update
the bios hence can I request to pull this driver. The driver solves the ACPI usecases.

As per DT is concerned, will look into driver/ide and led framework but since I am not very familiar with 
driver/ide and led framework hence it will take sometime to design and implement the DT cases.

> 	Arnd
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Tejun,

On 03/17/2016 12:36 PM, Arnd Bergmann wrote:
> On Wednesday 16 March 2016 14:07:13 Tejun Heo wrote:
>> Hello, Arnd.
>>
>> On Mon, Feb 01, 2016 at 09:14:17PM +0100, Arnd Bergmann wrote:
>>>> I am not debating on your AML call recommendation, it sounds like
>>>> a good idea however BIOS is already released hence its bit late to
>>>> add AML methods for this. I am seeking guidance on what can be
>>>> done in the given situation. I thought platform driver is one
>>>> option to get this feature enabled in kernel.
>>
>>> This is where we really need the ACPI maintainers to explain the
>>> general policy for dealing with firmware updates.
>>>
>>> I would assume that adding the feature in a later firmware version
>>> is a compatible change, and the feature is non-essential (the
>>> device will work fine with the generic SATA driver, except
>>> the LEDs don't blink), so it's not a big deal, it's just what
>>> you get for having the firmware shipped before the driver is
>>> reviewed (don't do that).
>>
>> So, if it were x86, I'd commit the custom driver without thinking too
>> much as ata drivers have always been working around bios issues (there
>> often wasn't any other recourse).  If the hardware is already out
>> there and it's not too easy to roll out bios updates, from libata
>> side, I'm okay with having a custom driver to work around that.  What
>> do you think?
> 
> 
> It's your call in the end. My main objection is to the fact that
> I have suggested a clean implementation for the normal DT based
> path that also fixes existing platforms that used to work in the
> past and were broken by the (long-ago) move from drivers/ide to
> drivers/ata, Brijesh has not implemented that but has instead
> continued pushing the hack for the ACPI mode that is still
> experimental on ARM64.
>

I am helping a customer who want EM support in a distro (using ACPI mode). Since its difficult to update
the bios hence can I request to pull this driver. The driver solves the ACPI usecases.

As per DT is concerned, will look into driver/ide and led framework but since I am not very familiar with 
driver/ide and led framework hence it will take sometime to design and implement the DT cases.

> 	Arnd
> 

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Tejun Heo]

Hello,

On Fri, Mar 18, 2016 at 01:36:40PM -0500, Brijesh Singh wrote:
> > It's your call in the end. My main objection is to the fact that
> > I have suggested a clean implementation for the normal DT based
> > path that also fixes existing platforms that used to work in the
> > past and were broken by the (long-ago) move from drivers/ide to
> > drivers/ata, Brijesh has not implemented that but has instead
> > continued pushing the hack for the ACPI mode that is still
> > experimental on ARM64.
> 
> I am helping a customer who want EM support in a distro (using ACPI
> mode). Since its difficult to update the bios hence can I request to
> pull this driver. The driver solves the ACPI usecases.
>
> As per DT is concerned, will look into driver/ide and led framework
> but since I am not very familiar with driver/ide and led framework
> hence it will take sometime to design and implement the DT cases.

Alright, will route it through for-4.6-fixes after the merge window
but please do look into the issue that Arnd raised.

Thanks.

-- 
tejun

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Tejun Heo]

On Thu, Jan 14, 2016 at 10:31:11AM -0600, Brijesh Singh wrote:
> AMD Seattle SATA controller mostly conforms to AHCI interface with some
> special register to control SGPIO interface. In the case of an AHCI
> controller, the SGPIO feature is ideally implemented using the
> "Enclosure Management" register of the AHCI controller, but those
> registeres are not implemented in the Seattle SoC. Instead SoC
> (Rev B0 onwards) provides a 32-bit SGPIO control register which should
> be programmed to control the activity, locate and fault LEDs.
> 
> The driver is based on ahci_platform driver.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> CC: tj@kernel.org
> CC: linux-ide@vger.kernel.org

Applied to libata/for-4.6-fixes.

Thanks.

-- 
tejun

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Matthias Brugger]

Hi Brijesh,

On 18/03/16 19:36, Brijesh Singh wrote:
> Hi Tejun,
>
> On 03/17/2016 12:36 PM, Arnd Bergmann wrote:
>> On Wednesday 16 March 2016 14:07:13 Tejun Heo wrote:
>>> Hello, Arnd.
>>>
>>> On Mon, Feb 01, 2016 at 09:14:17PM +0100, Arnd Bergmann wrote:
>>>>> I am not debating on your AML call recommendation, it sounds like
>>>>> a good idea however BIOS is already released hence its bit late to
>>>>> add AML methods for this. I am seeking guidance on what can be
>>>>> done in the given situation. I thought platform driver is one
>>>>> option to get this feature enabled in kernel.
>>>
>>>> This is where we really need the ACPI maintainers to explain the
>>>> general policy for dealing with firmware updates.
>>>>
>>>> I would assume that adding the feature in a later firmware version
>>>> is a compatible change, and the feature is non-essential (the
>>>> device will work fine with the generic SATA driver, except
>>>> the LEDs don't blink), so it's not a big deal, it's just what
>>>> you get for having the firmware shipped before the driver is
>>>> reviewed (don't do that).
>>>
>>> So, if it were x86, I'd commit the custom driver without thinking too
>>> much as ata drivers have always been working around bios issues (there
>>> often wasn't any other recourse).  If the hardware is already out
>>> there and it's not too easy to roll out bios updates, from libata
>>> side, I'm okay with having a custom driver to work around that.  What
>>> do you think?
>>
>>
>> It's your call in the end. My main objection is to the fact that
>> I have suggested a clean implementation for the normal DT based
>> path that also fixes existing platforms that used to work in the
>> past and were broken by the (long-ago) move from drivers/ide to
>> drivers/ata, Brijesh has not implemented that but has instead
>> continued pushing the hack for the ACPI mode that is still
>> experimental on ARM64.
>>
>
> I am helping a customer who want EM support in a distro (using ACPI mode). Since its difficult to update
> the bios hence can I request to pull this driver. The driver solves the ACPI usecases.
>
> As per DT is concerned, will look into driver/ide and led framework but since I am not very familiar with
> driver/ide and led framework hence it will take sometime to design and implement the DT cases.
>

Did you made any progress on the DT part?

Regards,
Matthias

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Matthias,

> 
> Did you made any progress on the DT part?
> 

I have not made much progress on DT part yet.

> Regards,
> Matthias

Re: [PATCH v2] ata: add AMD Seattle platform driver

[Brijesh Singh]

Hi Matthias,

> 
> Did you made any progress on the DT part?
> 

I have not made much progress on DT part yet.

> Regards,
> Matthias