* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-07 20:53 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-07 20:53 UTC (permalink / raw)
To: linux-arm-kernel
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>
CC: robh+dt at kernel.org
CC: pawel.moll at arm.com
CC: mark.rutland at arm.com
CC: ijc+devicetree at hellion.org.uk
CC: galak at codeaurora.org
CC: tj at kernel.org
CC: devicetree at vger.kernel.org
CC: linux-ide at vger.kernel.org
---
.../devicetree/bindings/ata/sata-seattle.txt | 34 ++++
drivers/ata/Kconfig | 8 +
drivers/ata/Makefile | 1 +
drivers/ata/ahci_seattle.c | 226 +++++++++++++++++++++
4 files changed, 269 insertions(+)
create mode 100644 Documentation/devicetree/bindings/ata/sata-seattle.txt
create mode 100644 drivers/ata/ahci_seattle.c
diff --git a/Documentation/devicetree/bindings/ata/sata-seattle.txt b/Documentation/devicetree/bindings/ata/sata-seattle.txt
new file mode 100644
index 0000000..5ad46b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/sata-seattle.txt
@@ -0,0 +1,34 @@
+* AHCI SATA Controller
+
+SATA nodes are defined to describe on-chip Serial ATA controllers.
+The AMD Seattle SATA controller mostly conforms to the AHCI interface
+with some special SGPIO register to contro activity LED interfaces.
+
+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.
+
+Each SATA controller should have its own node.
+
+
+Required properties:
+- compatible : compatible string should be "amd,seattle-ahci".
+- interrupts : <interrupt mapping for SATA IRQ>
+- reg : <registers mapping>
+
+Optional properties:
+- dma-coherent : Present if dma operations are coherent
+- clocks : a list of phandle + clock specifier pairs
+- target-supply : regulator for SATA target power
+- phys : reference to the SATA PHY node
+- phy-names : must be "sata-phy"
+
+Examples:
+ sata0 at e0300000 {
+ compatible = "amd,seattle-ahci";
+ reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
+ interrupts = <0x0 0x163 0x4>;
+ clocks = <0x2>
+ dma-coherent;
+ };
+
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 6aaa3f8..56f764d 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..8c9ab67
--- /dev/null
+++ b/drivers/ata/ahci_seattle.c
@@ -0,0 +1,226 @@
+/*
+ * 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)
+ emp = &pp->em_priv[pmp];
+ else
+ return -EINVAL;
+
+ 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);
+
+#ifdef CONFIG_OF
+static const struct of_device_id ahci_of_match[] = {
+ { .compatible = "amd,seattle-ahci", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ahci_of_match);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id ahci_acpi_match[] = {
+ { "AMDI0600", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
+#endif
+
+static struct platform_driver ahci_seattle_driver = {
+ .probe = ahci_seattle_probe,
+ .remove = ata_platform_remove_one,
+ .driver = {
+ .name = DRV_NAME,
+#ifdef CONFIG_OF
+ .of_match_table = ahci_of_match,
+#endif
+#ifdef CONFIG_ACPI
+ .acpi_match_table = ahci_acpi_match,
+#endif
+ .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
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-07 20:53 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-07 20:53 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Brijesh Singh, robh+dt, pawel.moll, mark.rutland, ijc+devicetree,
galak, tj, devicetree, linux-ide
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>
CC: robh+dt@kernel.org
CC: pawel.moll@arm.com
CC: mark.rutland@arm.com
CC: ijc+devicetree@hellion.org.uk
CC: galak@codeaurora.org
CC: tj@kernel.org
CC: devicetree@vger.kernel.org
CC: linux-ide@vger.kernel.org
---
.../devicetree/bindings/ata/sata-seattle.txt | 34 ++++
drivers/ata/Kconfig | 8 +
drivers/ata/Makefile | 1 +
drivers/ata/ahci_seattle.c | 226 +++++++++++++++++++++
4 files changed, 269 insertions(+)
create mode 100644 Documentation/devicetree/bindings/ata/sata-seattle.txt
create mode 100644 drivers/ata/ahci_seattle.c
diff --git a/Documentation/devicetree/bindings/ata/sata-seattle.txt b/Documentation/devicetree/bindings/ata/sata-seattle.txt
new file mode 100644
index 0000000..5ad46b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/sata-seattle.txt
@@ -0,0 +1,34 @@
+* AHCI SATA Controller
+
+SATA nodes are defined to describe on-chip Serial ATA controllers.
+The AMD Seattle SATA controller mostly conforms to the AHCI interface
+with some special SGPIO register to contro activity LED interfaces.
+
+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.
+
+Each SATA controller should have its own node.
+
+
+Required properties:
+- compatible : compatible string should be "amd,seattle-ahci".
+- interrupts : <interrupt mapping for SATA IRQ>
+- reg : <registers mapping>
+
+Optional properties:
+- dma-coherent : Present if dma operations are coherent
+- clocks : a list of phandle + clock specifier pairs
+- target-supply : regulator for SATA target power
+- phys : reference to the SATA PHY node
+- phy-names : must be "sata-phy"
+
+Examples:
+ sata0@e0300000 {
+ compatible = "amd,seattle-ahci";
+ reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
+ interrupts = <0x0 0x163 0x4>;
+ clocks = <0x2>
+ dma-coherent;
+ };
+
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 6aaa3f8..56f764d 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..8c9ab67
--- /dev/null
+++ b/drivers/ata/ahci_seattle.c
@@ -0,0 +1,226 @@
+/*
+ * 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)
+ emp = &pp->em_priv[pmp];
+ else
+ return -EINVAL;
+
+ 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);
+
+#ifdef CONFIG_OF
+static const struct of_device_id ahci_of_match[] = {
+ { .compatible = "amd,seattle-ahci", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ahci_of_match);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id ahci_acpi_match[] = {
+ { "AMDI0600", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
+#endif
+
+static struct platform_driver ahci_seattle_driver = {
+ .probe = ahci_seattle_probe,
+ .remove = ata_platform_remove_one,
+ .driver = {
+ .name = DRV_NAME,
+#ifdef CONFIG_OF
+ .of_match_table = ahci_of_match,
+#endif
+#ifdef CONFIG_ACPI
+ .acpi_match_table = ahci_acpi_match,
+#endif
+ .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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-07 20:53 ` Brijesh Singh
@ 2016-01-07 21:25 ` Arnd Bergmann
-1 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-07 21:25 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Brijesh Singh, linux-kernel, mark.rutland, devicetree,
pawel.moll, ijc+devicetree, linux-ide, robh+dt, galak, tj
On Thursday 07 January 2016 14:53:22 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>
> CC: robh+dt@kernel.org
> CC: pawel.moll@arm.com
> CC: mark.rutland@arm.com
> CC: ijc+devicetree@hellion.org.uk
> CC: galak@codeaurora.org
> CC: tj@kernel.org
> CC: devicetree@vger.kernel.org
> CC: linux-ide@vger.kernel.org
> ---
> .../devicetree/bindings/ata/sata-seattle.txt | 34 ++++
> drivers/ata/Kconfig | 8 +
> drivers/ata/Makefile | 1 +
> drivers/ata/ahci_seattle.c | 226 +++++++++++++++++++++
> 4 files changed, 269 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/ata/sata-seattle.txt
> create mode 100644 drivers/ata/ahci_seattle.c
>
> diff --git a/Documentation/devicetree/bindings/ata/sata-seattle.txt b/Documentation/devicetree/bindings/ata/sata-seattle.txt
> new file mode 100644
> index 0000000..5ad46b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/sata-seattle.txt
> @@ -0,0 +1,34 @@
> +* AHCI SATA Controller
> +
> +SATA nodes are defined to describe on-chip Serial ATA controllers.
> +The AMD Seattle SATA controller mostly conforms to the AHCI interface
> +with some special SGPIO register to contro activity LED interfaces.
> +
> +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.
> +
> +Each SATA controller should have its own node.
> +
> +
> +Required properties:
> +- compatible : compatible string should be "amd,seattle-ahci".
> +- interrupts : <interrupt mapping for SATA IRQ>
> +- reg : <registers mapping>
> +
> +Optional properties:
> +- dma-coherent : Present if dma operations are coherent
> +- clocks : a list of phandle + clock specifier pairs
> +- target-supply : regulator for SATA target power
> +- phys : reference to the SATA PHY node
> +- phy-names : must be "sata-phy"
> +
> +Examples:
> + sata0@e0300000 {
> + compatible = "amd,seattle-ahci";
> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
Looking at the register values, I doubt that the SGPIO is actually part of the
sata device. More likely, you are pointing in the middle of an actual
GPIO controller.
If so, please implement a GPIO driver for that device, and use the gpio-leds
driver to drive the LEDs. IIRC there is already a generic way to communicate
with the LEDs interface from libata, if not you can implement that in order
to keep the special case out of the platform driver.
> + interrupts = <0x0 0x163 0x4>;
> + clocks = <0x2>
This is not a valid property.
> +/* 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)
> + */
The 'reg' property in your example is only 8 bits wide, the above lists
32 bits.
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-07 21:25 ` Arnd Bergmann
0 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-07 21:25 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 07 January 2016 14:53:22 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>
> CC: robh+dt at kernel.org
> CC: pawel.moll at arm.com
> CC: mark.rutland at arm.com
> CC: ijc+devicetree at hellion.org.uk
> CC: galak at codeaurora.org
> CC: tj at kernel.org
> CC: devicetree at vger.kernel.org
> CC: linux-ide at vger.kernel.org
> ---
> .../devicetree/bindings/ata/sata-seattle.txt | 34 ++++
> drivers/ata/Kconfig | 8 +
> drivers/ata/Makefile | 1 +
> drivers/ata/ahci_seattle.c | 226 +++++++++++++++++++++
> 4 files changed, 269 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/ata/sata-seattle.txt
> create mode 100644 drivers/ata/ahci_seattle.c
>
> diff --git a/Documentation/devicetree/bindings/ata/sata-seattle.txt b/Documentation/devicetree/bindings/ata/sata-seattle.txt
> new file mode 100644
> index 0000000..5ad46b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/sata-seattle.txt
> @@ -0,0 +1,34 @@
> +* AHCI SATA Controller
> +
> +SATA nodes are defined to describe on-chip Serial ATA controllers.
> +The AMD Seattle SATA controller mostly conforms to the AHCI interface
> +with some special SGPIO register to contro activity LED interfaces.
> +
> +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.
> +
> +Each SATA controller should have its own node.
> +
> +
> +Required properties:
> +- compatible : compatible string should be "amd,seattle-ahci".
> +- interrupts : <interrupt mapping for SATA IRQ>
> +- reg : <registers mapping>
> +
> +Optional properties:
> +- dma-coherent : Present if dma operations are coherent
> +- clocks : a list of phandle + clock specifier pairs
> +- target-supply : regulator for SATA target power
> +- phys : reference to the SATA PHY node
> +- phy-names : must be "sata-phy"
> +
> +Examples:
> + sata0 at e0300000 {
> + compatible = "amd,seattle-ahci";
> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
Looking at the register values, I doubt that the SGPIO is actually part of the
sata device. More likely, you are pointing in the middle of an actual
GPIO controller.
If so, please implement a GPIO driver for that device, and use the gpio-leds
driver to drive the LEDs. IIRC there is already a generic way to communicate
with the LEDs interface from libata, if not you can implement that in order
to keep the special case out of the platform driver.
> + interrupts = <0x0 0x163 0x4>;
> + clocks = <0x2>
This is not a valid property.
> +/* 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)
> + */
The 'reg' property in your example is only 8 bits wide, the above lists
32 bits.
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-07 21:25 ` Arnd Bergmann
(?)
@ 2016-01-07 22:24 ` Brijesh Singh
-1 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-07 22:24 UTC (permalink / raw)
To: Arnd Bergmann, linux-arm-kernel
Cc: mark.rutland, devicetree, brijesh.singh, pawel.moll,
ijc+devicetree, linux-kernel, linux-ide, robh+dt, galak, tj
Hi,
>> +
>> +Examples:
>> + sata0@e0300000 {
>> + compatible = "amd,seattle-ahci";
>> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
>
> Looking at the register values, I doubt that the SGPIO is actually part of the
> sata device. More likely, you are pointing in the middle of an actual
> GPIO controller.
>
That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
A57 does not have access to GPIO's connected to backplane controller instead SoC has exposed two SGPIO
control registers (LSIOC_SGPIO_CONTROL0: 0xE000_0078 and LSIOC_SGPIO_CONTROL1: 0xE000_007C) to A57. All we
need to do is to program these registers based on the disk activity. The firmware running on A5 reads the values
and generate proper SGPIO timing and toggles the LEDs etc.
These registers are defined in SATA0/1 DSDT resource template and also documented in SoC BKDG. I just noticed that BKDG has wrong register definition so will ask documentation folks to fix that.
This driver is using SGPIO LED control similar to sata_highbank [1] except bit bang GPIO (which is done by firmware).
[1] http://lxr.free-electrons.com/source/drivers/ata/sata_highbank.c#L140
> If so, please implement a GPIO driver for that device, and use the gpio-leds
> driver to drive the LEDs. IIRC there is already a generic way to communicate
> with the LEDs interface from libata, if not you can implement that in order
> to keep the special case out of the platform driver.
>
>> + interrupts = <0x0 0x163 0x4>;
>> + clocks = <0x2>
>
> This is not a valid property.
>
>> +/* 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)
>> + */
>
> The 'reg' property in your example is only 8 bits wide, the above lists
> 32 bits.
>
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-07 22:24 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-07 22:24 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
>> +
>> +Examples:
>> + sata0 at e0300000 {
>> + compatible = "amd,seattle-ahci";
>> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
>
> Looking at the register values, I doubt that the SGPIO is actually part of the
> sata device. More likely, you are pointing in the middle of an actual
> GPIO controller.
>
That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
A57 does not have access to GPIO's connected to backplane controller instead SoC has exposed two SGPIO
control registers (LSIOC_SGPIO_CONTROL0: 0xE000_0078 and LSIOC_SGPIO_CONTROL1: 0xE000_007C) to A57. All we
need to do is to program these registers based on the disk activity. The firmware running on A5 reads the values
and generate proper SGPIO timing and toggles the LEDs etc.
These registers are defined in SATA0/1 DSDT resource template and also documented in SoC BKDG. I just noticed that BKDG has wrong register definition so will ask documentation folks to fix that.
This driver is using SGPIO LED control similar to sata_highbank [1] except bit bang GPIO (which is done by firmware).
[1] http://lxr.free-electrons.com/source/drivers/ata/sata_highbank.c#L140
> If so, please implement a GPIO driver for that device, and use the gpio-leds
> driver to drive the LEDs. IIRC there is already a generic way to communicate
> with the LEDs interface from libata, if not you can implement that in order
> to keep the special case out of the platform driver.
>
>> + interrupts = <0x0 0x163 0x4>;
>> + clocks = <0x2>
>
> This is not a valid property.
>
>> +/* 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)
>> + */
>
> The 'reg' property in your example is only 8 bits wide, the above lists
> 32 bits.
>
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-07 22:24 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-07 22:24 UTC (permalink / raw)
To: Arnd Bergmann, linux-arm-kernel
Cc: brijesh.singh, linux-kernel, mark.rutland, devicetree,
pawel.moll, ijc+devicetree, linux-ide, robh+dt, galak, tj
Hi,
>> +
>> +Examples:
>> + sata0@e0300000 {
>> + compatible = "amd,seattle-ahci";
>> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
>
> Looking at the register values, I doubt that the SGPIO is actually part of the
> sata device. More likely, you are pointing in the middle of an actual
> GPIO controller.
>
That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
A57 does not have access to GPIO's connected to backplane controller instead SoC has exposed two SGPIO
control registers (LSIOC_SGPIO_CONTROL0: 0xE000_0078 and LSIOC_SGPIO_CONTROL1: 0xE000_007C) to A57. All we
need to do is to program these registers based on the disk activity. The firmware running on A5 reads the values
and generate proper SGPIO timing and toggles the LEDs etc.
These registers are defined in SATA0/1 DSDT resource template and also documented in SoC BKDG. I just noticed that BKDG has wrong register definition so will ask documentation folks to fix that.
This driver is using SGPIO LED control similar to sata_highbank [1] except bit bang GPIO (which is done by firmware).
[1] http://lxr.free-electrons.com/source/drivers/ata/sata_highbank.c#L140
> If so, please implement a GPIO driver for that device, and use the gpio-leds
> driver to drive the LEDs. IIRC there is already a generic way to communicate
> with the LEDs interface from libata, if not you can implement that in order
> to keep the special case out of the platform driver.
>
>> + interrupts = <0x0 0x163 0x4>;
>> + clocks = <0x2>
>
> This is not a valid property.
>
>> +/* 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)
>> + */
>
> The 'reg' property in your example is only 8 bits wide, the above lists
> 32 bits.
>
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-07 22:24 ` Brijesh Singh
@ 2016-01-07 23:42 ` Arnd Bergmann
-1 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-07 23:42 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-arm-kernel, linux-kernel, mark.rutland, devicetree,
pawel.moll, ijc+devicetree, linux-ide, robh+dt, galak, tj
On Thursday 07 January 2016 16:24:22 Brijesh Singh wrote:
> >> +
> >> +Examples:
> >> + sata0@e0300000 {
> >> + compatible = "amd,seattle-ahci";
> >> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
> >
> > Looking at the register values, I doubt that the SGPIO is actually part of the
> > sata device. More likely, you are pointing in the middle of an actual
> > GPIO controller.
> >
>
> That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
Of course its a control register "for" SATA, what I meant is that it's
not part "of" the SATA IP block, which is hopefully a standard AHCI
compliant part as required by SBSA.
> A57 does not have access to GPIO's connected to backplane controller
> instead SoC has exposed two SGPIO control registers (LSIOC_SGPIO_CONTROL0:
> 0xE000_0078 and LSIOC_SGPIO_CONTROL1: 0xE000_007C) to A57. All we
> need to do is to program these registers based on the disk activity.
> The firmware running on A5 reads the values and generate proper SGPIO
> timing and toggles the LEDs etc.
It still sounds like SGPIO is not part of the AHCI standard spec, but
rather a subset of a device called LSIOC.
> These registers are defined in SATA0/1 DSDT resource template and also
> documented in SoC BKDG. I just noticed that BKDG has wrong register
> definition so will ask documentation folks to fix that.
>
> This driver is using SGPIO LED control similar to sata_highbank [1]
> except bit bang GPIO (which is done by firmware).
>
> [1] http://lxr.free-electrons.com/source/drivers/ata/sata_highbank.c#L140
This one is rather different: there is a single device that combines
registers for AHCI, the PHY attached to it and the LED. This is not
SBSA compliant of course, and it requires having a special driver.
What you have instead looks like a regular AHCI implementation that
should just work with the standard driver as long as you describe how
it gets its LEDs.
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-07 23:42 ` Arnd Bergmann
0 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-07 23:42 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 07 January 2016 16:24:22 Brijesh Singh wrote:
> >> +
> >> +Examples:
> >> + sata0 at e0300000 {
> >> + compatible = "amd,seattle-ahci";
> >> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
> >
> > Looking at the register values, I doubt that the SGPIO is actually part of the
> > sata device. More likely, you are pointing in the middle of an actual
> > GPIO controller.
> >
>
> That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
Of course its a control register "for" SATA, what I meant is that it's
not part "of" the SATA IP block, which is hopefully a standard AHCI
compliant part as required by SBSA.
> A57 does not have access to GPIO's connected to backplane controller
> instead SoC has exposed two SGPIO control registers (LSIOC_SGPIO_CONTROL0:
> 0xE000_0078 and LSIOC_SGPIO_CONTROL1: 0xE000_007C) to A57. All we
> need to do is to program these registers based on the disk activity.
> The firmware running on A5 reads the values and generate proper SGPIO
> timing and toggles the LEDs etc.
It still sounds like SGPIO is not part of the AHCI standard spec, but
rather a subset of a device called LSIOC.
> These registers are defined in SATA0/1 DSDT resource template and also
> documented in SoC BKDG. I just noticed that BKDG has wrong register
> definition so will ask documentation folks to fix that.
>
> This driver is using SGPIO LED control similar to sata_highbank [1]
> except bit bang GPIO (which is done by firmware).
>
> [1] http://lxr.free-electrons.com/source/drivers/ata/sata_highbank.c#L140
This one is rather different: there is a single device that combines
registers for AHCI, the PHY attached to it and the LED. This is not
SBSA compliant of course, and it requires having a special driver.
What you have instead looks like a regular AHCI implementation that
should just work with the standard driver as long as you describe how
it gets its LEDs.
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-07 23:42 ` Arnd Bergmann
(?)
@ 2016-01-08 1:46 ` Brijesh Singh
-1 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-08 1:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, linux-kernel, mark.rutland, devicetree,
pawel.moll, ijc+devicetree, linux-ide, robh+dt, galak, tj
Hi,
On 01/07/2016 05:42 PM, Arnd Bergmann wrote:
> On Thursday 07 January 2016 16:24:22 Brijesh Singh wrote:
>>>> +
>>>> +Examples:
>>>> + sata0@e0300000 {
>>>> + compatible = "amd,seattle-ahci";
>>>> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
>>>
>>> Looking at the register values, I doubt that the SGPIO is actually part of the
>>> sata device. More likely, you are pointing in the middle of an actual
>>> GPIO controller.
>>>
>>
>> That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
>
> Of course its a control register "for" SATA, what I meant is that it's
> not part "of" the SATA IP block, which is hopefully a standard AHCI
> compliant part as required by SBSA.
>
Yes, its not part of SATA IP block. We just need a method of pass SGPIO
control register address to driver.
Should I consider adding a property "sgpio-ctrl" to pass the register
address ?
e.g
sata0@e0300000 {
compatible = "amd,seattle-ahci";
reg = <0 0xe0300000 0 0x800>;
amd,sgpio-ctrl = <0xe0000078>;
interrupts = <0 355 4>;
clocks = <&sataclk_333mhz>;
dma-coherent;
};
>> A57 does not have access to GPIO's connected to backplane controller
>> instead SoC has exposed two SGPIO control registers (LSIOC_SGPIO_CONTROL0:
>> 0xE000_0078 and LSIOC_SGPIO_CONTROL1: 0xE000_007C) to A57. All we
>> need to do is to program these registers based on the disk activity.
>> The firmware running on A5 reads the values and generate proper SGPIO
>> timing and toggles the LEDs etc.
>
> It still sounds like SGPIO is not part of the AHCI standard spec, but
> rather a subset of a device called LSIOC.
>
>> These registers are defined in SATA0/1 DSDT resource template and also
>> documented in SoC BKDG. I just noticed that BKDG has wrong register
>> definition so will ask documentation folks to fix that.
>>
>> This driver is using SGPIO LED control similar to sata_highbank [1]
>> except bit bang GPIO (which is done by firmware).
>>
>> [1] http://lxr.free-electrons.com/source/drivers/ata/sata_highbank.c#L140
>
> This one is rather different: there is a single device that combines
> registers for AHCI, the PHY attached to it and the LED. This is not
> SBSA compliant of course, and it requires having a special driver.
>
> What you have instead looks like a regular AHCI implementation that
> should just work with the standard driver as long as you describe how
> it gets its LEDs.
>
Yes, its regular AHCI implementation and works well with ahci_platform
driver. In standard ahci_platform driver activity LEDs are blinked
through enclosure management interface. Given the current hardware
limitation it seems like creating a new driver would be cleaner. I am
open to suggestion.
Thanks for all your review feedbacks.
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-08 1:46 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-08 1:46 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 01/07/2016 05:42 PM, Arnd Bergmann wrote:
> On Thursday 07 January 2016 16:24:22 Brijesh Singh wrote:
>>>> +
>>>> +Examples:
>>>> + sata0 at e0300000 {
>>>> + compatible = "amd,seattle-ahci";
>>>> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
>>>
>>> Looking at the register values, I doubt that the SGPIO is actually part of the
>>> sata device. More likely, you are pointing in the middle of an actual
>>> GPIO controller.
>>>
>>
>> That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
>
> Of course its a control register "for" SATA, what I meant is that it's
> not part "of" the SATA IP block, which is hopefully a standard AHCI
> compliant part as required by SBSA.
>
Yes, its not part of SATA IP block. We just need a method of pass SGPIO
control register address to driver.
Should I consider adding a property "sgpio-ctrl" to pass the register
address ?
e.g
sata0 at e0300000 {
compatible = "amd,seattle-ahci";
reg = <0 0xe0300000 0 0x800>;
amd,sgpio-ctrl = <0xe0000078>;
interrupts = <0 355 4>;
clocks = <&sataclk_333mhz>;
dma-coherent;
};
>> A57 does not have access to GPIO's connected to backplane controller
>> instead SoC has exposed two SGPIO control registers (LSIOC_SGPIO_CONTROL0:
>> 0xE000_0078 and LSIOC_SGPIO_CONTROL1: 0xE000_007C) to A57. All we
>> need to do is to program these registers based on the disk activity.
>> The firmware running on A5 reads the values and generate proper SGPIO
>> timing and toggles the LEDs etc.
>
> It still sounds like SGPIO is not part of the AHCI standard spec, but
> rather a subset of a device called LSIOC.
>
>> These registers are defined in SATA0/1 DSDT resource template and also
>> documented in SoC BKDG. I just noticed that BKDG has wrong register
>> definition so will ask documentation folks to fix that.
>>
>> This driver is using SGPIO LED control similar to sata_highbank [1]
>> except bit bang GPIO (which is done by firmware).
>>
>> [1] http://lxr.free-electrons.com/source/drivers/ata/sata_highbank.c#L140
>
> This one is rather different: there is a single device that combines
> registers for AHCI, the PHY attached to it and the LED. This is not
> SBSA compliant of course, and it requires having a special driver.
>
> What you have instead looks like a regular AHCI implementation that
> should just work with the standard driver as long as you describe how
> it gets its LEDs.
>
Yes, its regular AHCI implementation and works well with ahci_platform
driver. In standard ahci_platform driver activity LEDs are blinked
through enclosure management interface. Given the current hardware
limitation it seems like creating a new driver would be cleaner. I am
open to suggestion.
Thanks for all your review feedbacks.
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-08 1:46 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-08 1:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, linux-kernel, mark.rutland, devicetree,
pawel.moll, ijc+devicetree, linux-ide, robh+dt, galak, tj
Hi,
On 01/07/2016 05:42 PM, Arnd Bergmann wrote:
> On Thursday 07 January 2016 16:24:22 Brijesh Singh wrote:
>>>> +
>>>> +Examples:
>>>> + sata0@e0300000 {
>>>> + compatible = "amd,seattle-ahci";
>>>> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
>>>
>>> Looking at the register values, I doubt that the SGPIO is actually part of the
>>> sata device. More likely, you are pointing in the middle of an actual
>>> GPIO controller.
>>>
>>
>> That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
>
> Of course its a control register "for" SATA, what I meant is that it's
> not part "of" the SATA IP block, which is hopefully a standard AHCI
> compliant part as required by SBSA.
>
Yes, its not part of SATA IP block. We just need a method of pass SGPIO
control register address to driver.
Should I consider adding a property "sgpio-ctrl" to pass the register
address ?
e.g
sata0@e0300000 {
compatible = "amd,seattle-ahci";
reg = <0 0xe0300000 0 0x800>;
amd,sgpio-ctrl = <0xe0000078>;
interrupts = <0 355 4>;
clocks = <&sataclk_333mhz>;
dma-coherent;
};
>> A57 does not have access to GPIO's connected to backplane controller
>> instead SoC has exposed two SGPIO control registers (LSIOC_SGPIO_CONTROL0:
>> 0xE000_0078 and LSIOC_SGPIO_CONTROL1: 0xE000_007C) to A57. All we
>> need to do is to program these registers based on the disk activity.
>> The firmware running on A5 reads the values and generate proper SGPIO
>> timing and toggles the LEDs etc.
>
> It still sounds like SGPIO is not part of the AHCI standard spec, but
> rather a subset of a device called LSIOC.
>
>> These registers are defined in SATA0/1 DSDT resource template and also
>> documented in SoC BKDG. I just noticed that BKDG has wrong register
>> definition so will ask documentation folks to fix that.
>>
>> This driver is using SGPIO LED control similar to sata_highbank [1]
>> except bit bang GPIO (which is done by firmware).
>>
>> [1] http://lxr.free-electrons.com/source/drivers/ata/sata_highbank.c#L140
>
> This one is rather different: there is a single device that combines
> registers for AHCI, the PHY attached to it and the LED. This is not
> SBSA compliant of course, and it requires having a special driver.
>
> What you have instead looks like a regular AHCI implementation that
> should just work with the standard driver as long as you describe how
> it gets its LEDs.
>
Yes, its regular AHCI implementation and works well with ahci_platform
driver. In standard ahci_platform driver activity LEDs are blinked
through enclosure management interface. Given the current hardware
limitation it seems like creating a new driver would be cleaner. I am
open to suggestion.
Thanks for all your review feedbacks.
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-08 1:46 ` Brijesh Singh
@ 2016-01-08 8:46 ` Arnd Bergmann
-1 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-08 8:46 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-arm-kernel, linux-kernel, mark.rutland, devicetree,
pawel.moll, ijc+devicetree, linux-ide, robh+dt, galak, tj
On Thursday 07 January 2016 19:46:08 Brijesh Singh wrote:
> Hi,
>
> On 01/07/2016 05:42 PM, Arnd Bergmann wrote:
> > On Thursday 07 January 2016 16:24:22 Brijesh Singh wrote:
> >>>> +
> >>>> +Examples:
> >>>> + sata0@e0300000 {
> >>>> + compatible = "amd,seattle-ahci";
> >>>> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
> >>>
> >>> Looking at the register values, I doubt that the SGPIO is actually part of the
> >>> sata device. More likely, you are pointing in the middle of an actual
> >>> GPIO controller.
> >>>
> >>
> >> That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
> >
> > Of course its a control register "for" SATA, what I meant is that it's
> > not part "of" the SATA IP block, which is hopefully a standard AHCI
> > compliant part as required by SBSA.
> >
> Yes, its not part of SATA IP block. We just need a method of pass SGPIO
> control register address to driver.
>
> Should I consider adding a property "sgpio-ctrl" to pass the register
> address ?
>
> e.g
>
> sata0@e0300000 {
> compatible = "amd,seattle-ahci";
> reg = <0 0xe0300000 0 0x800>;
> amd,sgpio-ctrl = <0xe0000078>;
> interrupts = <0 355 4>;
> clocks = <&sataclk_333mhz>;
> dma-coherent;
> };
We generally don't refer to register locations with properties other than
'reg', so that approach would be worse. What I'd suggest you do is to
have the sgpio registers in a separate device node, and use the LED
binding to access it, see
Documentation/devicetree/bindings/leds/common.txt
It seems that none of the drivers/ata/ drivers use the leds interface
today, but that can be added to libata-*.c whenever the appropriate
properties are there.
> > This one is rather different: there is a single device that combines
> > registers for AHCI, the PHY attached to it and the LED. This is not
> > SBSA compliant of course, and it requires having a special driver.
> >
> > What you have instead looks like a regular AHCI implementation that
> > should just work with the standard driver as long as you describe how
> > it gets its LEDs.
> >
> Yes, its regular AHCI implementation and works well with ahci_platform
> driver. In standard ahci_platform driver activity LEDs are blinked
> through enclosure management interface. Given the current hardware
> limitation it seems like creating a new driver would be cleaner. I am
> open to suggestion.
I'd say the code in drivers/ata should be kept completely generic, referring
only to the include/linux/leds.h interfaces and properties added to
Documentation/devicetree/bindings/ata/ahci-platform.txt (if any).
For the driver that actually owns the register, it depends a bit on how
the hardware is structured, and you'd need to look at the datasheet (or
talk to a hardware designer) for that.
I suspect we should have a node for the entire block of registers
around the SGPIO, presumably something like
syscon@0xe0000000 {
compatible = "amd,$SOC_ID"-lsioc", "syscon";
reg = <0xe0000000 0x1000>; /* find the actual length in the datasheet */
};
That way, any driver that ends up needing a register from this block
can use the syscon interface to get hold of a mapping, and/or you can
have a high-level driver to expose other functionalities. It's probably
best to start doing it either entirely using syscon references from
other drivers, or using no syscon references, and putting everything into
a driver for the register set, but as I said it depends a lot on what
else is in there.
Can you send me a register list of the 0xe0000000 segment for reference?
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-08 8:46 ` Arnd Bergmann
0 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-08 8:46 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 07 January 2016 19:46:08 Brijesh Singh wrote:
> Hi,
>
> On 01/07/2016 05:42 PM, Arnd Bergmann wrote:
> > On Thursday 07 January 2016 16:24:22 Brijesh Singh wrote:
> >>>> +
> >>>> +Examples:
> >>>> + sata0 at e0300000 {
> >>>> + compatible = "amd,seattle-ahci";
> >>>> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
> >>>
> >>> Looking at the register values, I doubt that the SGPIO is actually part of the
> >>> sata device. More likely, you are pointing in the middle of an actual
> >>> GPIO controller.
> >>>
> >>
> >> That address is SGPIO control register for SATA. The current hardware implementation to control activity LED is not ideal.
> >
> > Of course its a control register "for" SATA, what I meant is that it's
> > not part "of" the SATA IP block, which is hopefully a standard AHCI
> > compliant part as required by SBSA.
> >
> Yes, its not part of SATA IP block. We just need a method of pass SGPIO
> control register address to driver.
>
> Should I consider adding a property "sgpio-ctrl" to pass the register
> address ?
>
> e.g
>
> sata0 at e0300000 {
> compatible = "amd,seattle-ahci";
> reg = <0 0xe0300000 0 0x800>;
> amd,sgpio-ctrl = <0xe0000078>;
> interrupts = <0 355 4>;
> clocks = <&sataclk_333mhz>;
> dma-coherent;
> };
We generally don't refer to register locations with properties other than
'reg', so that approach would be worse. What I'd suggest you do is to
have the sgpio registers in a separate device node, and use the LED
binding to access it, see
Documentation/devicetree/bindings/leds/common.txt
It seems that none of the drivers/ata/ drivers use the leds interface
today, but that can be added to libata-*.c whenever the appropriate
properties are there.
> > This one is rather different: there is a single device that combines
> > registers for AHCI, the PHY attached to it and the LED. This is not
> > SBSA compliant of course, and it requires having a special driver.
> >
> > What you have instead looks like a regular AHCI implementation that
> > should just work with the standard driver as long as you describe how
> > it gets its LEDs.
> >
> Yes, its regular AHCI implementation and works well with ahci_platform
> driver. In standard ahci_platform driver activity LEDs are blinked
> through enclosure management interface. Given the current hardware
> limitation it seems like creating a new driver would be cleaner. I am
> open to suggestion.
I'd say the code in drivers/ata should be kept completely generic, referring
only to the include/linux/leds.h interfaces and properties added to
Documentation/devicetree/bindings/ata/ahci-platform.txt (if any).
For the driver that actually owns the register, it depends a bit on how
the hardware is structured, and you'd need to look at the datasheet (or
talk to a hardware designer) for that.
I suspect we should have a node for the entire block of registers
around the SGPIO, presumably something like
syscon at 0xe0000000 {
compatible = "amd,$SOC_ID"-lsioc", "syscon";
reg = <0xe0000000 0x1000>; /* find the actual length in the datasheet */
};
That way, any driver that ends up needing a register from this block
can use the syscon interface to get hold of a mapping, and/or you can
have a high-level driver to expose other functionalities. It's probably
best to start doing it either entirely using syscon references from
other drivers, or using no syscon references, and putting everything into
a driver for the register set, but as I said it depends a lot on what
else is in there.
Can you send me a register list of the 0xe0000000 segment for reference?
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-08 8:46 ` Arnd Bergmann
(?)
@ 2016-01-08 22:21 ` Brijesh Singh
-1 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-08 22:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: brijesh.singh, linux-arm-kernel, linux-kernel, mark.rutland,
devicetree, pawel.moll, ijc+devicetree, linux-ide, robh+dt,
galak, tj
Hi,
>> Should I consider adding a property "sgpio-ctrl" to pass the register
>> address ?
>>
>> e.g
>>
>> sata0@e0300000 {
>> compatible = "amd,seattle-ahci";
>> reg = <0 0xe0300000 0 0x800>;
>> amd,sgpio-ctrl = <0xe0000078>;
>> interrupts = <0 355 4>;
>> clocks = <&sataclk_333mhz>;
>> dma-coherent;
>> };
>
> We generally don't refer to register locations with properties other than
> 'reg', so that approach would be worse. What I'd suggest you do is to
> have the sgpio registers in a separate device node, and use the LED
> binding to access it, see
>
> Documentation/devicetree/bindings/leds/common.txt
>
> It seems that none of the drivers/ata/ drivers use the leds interface
> today, but that can be added to libata-*.c whenever the appropriate
> properties are there.
>
libata-*.c implements the "Enclosure management" style led messages but also has hooks
to register a custom led control callback. Since Seattle platform does not support
the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
to indicate that we can still handle the led messages by our registered callback. I see
that sata_highbank driver is doing something similar.
>> Yes, its regular AHCI implementation and works well with ahci_platform
>> driver. In standard ahci_platform driver activity LEDs are blinked
>> through enclosure management interface. Given the current hardware
>> limitation it seems like creating a new driver would be cleaner. I am
>> open to suggestion.
>
> I'd say the code in drivers/ata should be kept completely generic, referring
> only to the include/linux/leds.h interfaces and properties added to
> Documentation/devicetree/bindings/ata/ahci-platform.txt (if any).
>
> For the driver that actually owns the register, it depends a bit on how
> the hardware is structured, and you'd need to look at the datasheet (or
> talk to a hardware designer) for that.
>
> I suspect we should have a node for the entire block of registers
> around the SGPIO, presumably something like
>
> syscon@0xe0000000 {
> compatible = "amd,$SOC_ID"-lsioc", "syscon";
> reg = <0xe0000000 0x1000>; /* find the actual length in the datasheet */
> };
>
> That way, any driver that ends up needing a register from this block
> can use the syscon interface to get hold of a mapping, and/or you can
> have a high-level driver to expose other functionalities. It's probably
> best to start doing it either entirely using syscon references from
> other drivers, or using no syscon references, and putting everything into
> a driver for the register set, but as I said it depends a lot on what
> else is in there.
>
> Can you send me a register list of the 0xe0000000 segment for reference?
>
Thanks for pointing syncon, are you thinking something like this ?
sgpio0: syscon@e0000078 {
compatible = "amd, seattle-sgpio", syscon";
reg = <0x0 0xe0000078 0x0 0x1>;
};
sata@e0300000 {
compatible = "amd,seattle-ahci";
reg = <0x0 0xe0300000 0x0 0xf0000>;
interrupts = <0x0 0x163 0x4>;
amd,sgpio-ctrl = <&sgpio0>;
dma-coherent;
};
SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
its just extension of the IP block but it just happened to be way out of the IP block range.
Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
+ 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;
+
The above code works on ACPI and DT cases. But if we go with syncon approach then we need to handle DT and ACPI differently. Because sycon will provide struct regmap instead of struct resources and also make reading/writing a bit different. I was trying to minimize the DT vs ACPI changes in the driver (other than binding) hence I think defining two ranges in sata controller reg property was much cleaner and it aligns with DSDT.
Just for reference the SATA DSDT looks like this:
Device (SATA0)
{
.....
Name(_CRS, ResourceTemplate()
{
Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)
Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
Memory32Fixed(ReadWrite, 0xE00000078, 1)
}
......
}
>
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-08 22:21 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-08 22:21 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
>> Should I consider adding a property "sgpio-ctrl" to pass the register
>> address ?
>>
>> e.g
>>
>> sata0 at e0300000 {
>> compatible = "amd,seattle-ahci";
>> reg = <0 0xe0300000 0 0x800>;
>> amd,sgpio-ctrl = <0xe0000078>;
>> interrupts = <0 355 4>;
>> clocks = <&sataclk_333mhz>;
>> dma-coherent;
>> };
>
> We generally don't refer to register locations with properties other than
> 'reg', so that approach would be worse. What I'd suggest you do is to
> have the sgpio registers in a separate device node, and use the LED
> binding to access it, see
>
> Documentation/devicetree/bindings/leds/common.txt
>
> It seems that none of the drivers/ata/ drivers use the leds interface
> today, but that can be added to libata-*.c whenever the appropriate
> properties are there.
>
libata-*.c implements the "Enclosure management" style led messages but also has hooks
to register a custom led control callback. Since Seattle platform does not support
the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
to indicate that we can still handle the led messages by our registered callback. I see
that sata_highbank driver is doing something similar.
>> Yes, its regular AHCI implementation and works well with ahci_platform
>> driver. In standard ahci_platform driver activity LEDs are blinked
>> through enclosure management interface. Given the current hardware
>> limitation it seems like creating a new driver would be cleaner. I am
>> open to suggestion.
>
> I'd say the code in drivers/ata should be kept completely generic, referring
> only to the include/linux/leds.h interfaces and properties added to
> Documentation/devicetree/bindings/ata/ahci-platform.txt (if any).
>
> For the driver that actually owns the register, it depends a bit on how
> the hardware is structured, and you'd need to look at the datasheet (or
> talk to a hardware designer) for that.
>
> I suspect we should have a node for the entire block of registers
> around the SGPIO, presumably something like
>
> syscon at 0xe0000000 {
> compatible = "amd,$SOC_ID"-lsioc", "syscon";
> reg = <0xe0000000 0x1000>; /* find the actual length in the datasheet */
> };
>
> That way, any driver that ends up needing a register from this block
> can use the syscon interface to get hold of a mapping, and/or you can
> have a high-level driver to expose other functionalities. It's probably
> best to start doing it either entirely using syscon references from
> other drivers, or using no syscon references, and putting everything into
> a driver for the register set, but as I said it depends a lot on what
> else is in there.
>
> Can you send me a register list of the 0xe0000000 segment for reference?
>
Thanks for pointing syncon, are you thinking something like this ?
sgpio0: syscon at e0000078 {
compatible = "amd, seattle-sgpio", syscon";
reg = <0x0 0xe0000078 0x0 0x1>;
};
sata at e0300000 {
compatible = "amd,seattle-ahci";
reg = <0x0 0xe0300000 0x0 0xf0000>;
interrupts = <0x0 0x163 0x4>;
amd,sgpio-ctrl = <&sgpio0>;
dma-coherent;
};
SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
its just extension of the IP block but it just happened to be way out of the IP block range.
Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
+ 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;
+
The above code works on ACPI and DT cases. But if we go with syncon approach then we need to handle DT and ACPI differently. Because sycon will provide struct regmap instead of struct resources and also make reading/writing a bit different. I was trying to minimize the DT vs ACPI changes in the driver (other than binding) hence I think defining two ranges in sata controller reg property was much cleaner and it aligns with DSDT.
Just for reference the SATA DSDT looks like this:
Device (SATA0)
{
.....
Name(_CRS, ResourceTemplate()
{
Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)
Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
Memory32Fixed(ReadWrite, 0xE00000078, 1)
}
......
}
>
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-08 22:21 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-08 22:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: brijesh.singh, linux-arm-kernel, linux-kernel, mark.rutland,
devicetree, pawel.moll, ijc+devicetree, linux-ide, robh+dt,
galak, tj
Hi,
>> Should I consider adding a property "sgpio-ctrl" to pass the register
>> address ?
>>
>> e.g
>>
>> sata0@e0300000 {
>> compatible = "amd,seattle-ahci";
>> reg = <0 0xe0300000 0 0x800>;
>> amd,sgpio-ctrl = <0xe0000078>;
>> interrupts = <0 355 4>;
>> clocks = <&sataclk_333mhz>;
>> dma-coherent;
>> };
>
> We generally don't refer to register locations with properties other than
> 'reg', so that approach would be worse. What I'd suggest you do is to
> have the sgpio registers in a separate device node, and use the LED
> binding to access it, see
>
> Documentation/devicetree/bindings/leds/common.txt
>
> It seems that none of the drivers/ata/ drivers use the leds interface
> today, but that can be added to libata-*.c whenever the appropriate
> properties are there.
>
libata-*.c implements the "Enclosure management" style led messages but also has hooks
to register a custom led control callback. Since Seattle platform does not support
the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
to indicate that we can still handle the led messages by our registered callback. I see
that sata_highbank driver is doing something similar.
>> Yes, its regular AHCI implementation and works well with ahci_platform
>> driver. In standard ahci_platform driver activity LEDs are blinked
>> through enclosure management interface. Given the current hardware
>> limitation it seems like creating a new driver would be cleaner. I am
>> open to suggestion.
>
> I'd say the code in drivers/ata should be kept completely generic, referring
> only to the include/linux/leds.h interfaces and properties added to
> Documentation/devicetree/bindings/ata/ahci-platform.txt (if any).
>
> For the driver that actually owns the register, it depends a bit on how
> the hardware is structured, and you'd need to look at the datasheet (or
> talk to a hardware designer) for that.
>
> I suspect we should have a node for the entire block of registers
> around the SGPIO, presumably something like
>
> syscon@0xe0000000 {
> compatible = "amd,$SOC_ID"-lsioc", "syscon";
> reg = <0xe0000000 0x1000>; /* find the actual length in the datasheet */
> };
>
> That way, any driver that ends up needing a register from this block
> can use the syscon interface to get hold of a mapping, and/or you can
> have a high-level driver to expose other functionalities. It's probably
> best to start doing it either entirely using syscon references from
> other drivers, or using no syscon references, and putting everything into
> a driver for the register set, but as I said it depends a lot on what
> else is in there.
>
> Can you send me a register list of the 0xe0000000 segment for reference?
>
Thanks for pointing syncon, are you thinking something like this ?
sgpio0: syscon@e0000078 {
compatible = "amd, seattle-sgpio", syscon";
reg = <0x0 0xe0000078 0x0 0x1>;
};
sata@e0300000 {
compatible = "amd,seattle-ahci";
reg = <0x0 0xe0300000 0x0 0xf0000>;
interrupts = <0x0 0x163 0x4>;
amd,sgpio-ctrl = <&sgpio0>;
dma-coherent;
};
SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
its just extension of the IP block but it just happened to be way out of the IP block range.
Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
+ 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;
+
The above code works on ACPI and DT cases. But if we go with syncon approach then we need to handle DT and ACPI differently. Because sycon will provide struct regmap instead of struct resources and also make reading/writing a bit different. I was trying to minimize the DT vs ACPI changes in the driver (other than binding) hence I think defining two ranges in sata controller reg property was much cleaner and it aligns with DSDT.
Just for reference the SATA DSDT looks like this:
Device (SATA0)
{
.....
Name(_CRS, ResourceTemplate()
{
Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)
Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
Memory32Fixed(ReadWrite, 0xE00000078, 1)
}
......
}
>
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-08 22:21 ` Brijesh Singh
@ 2016-01-08 22:47 ` Arnd Bergmann
-1 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-08 22:47 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-arm-kernel, linux-kernel, mark.rutland, devicetree,
pawel.moll, ijc+devicetree, linux-ide, robh+dt, galak, tj
On Friday 08 January 2016 16:21:50 Brijesh Singh wrote:
> >> Should I consider adding a property "sgpio-ctrl" to pass the register
> >> address ?
> >>
> >> e.g
> >>
> >> sata0@e0300000 {
> >> compatible = "amd,seattle-ahci";
> >> reg = <0 0xe0300000 0 0x800>;
> >> amd,sgpio-ctrl = <0xe0000078>;
> >> interrupts = <0 355 4>;
> >> clocks = <&sataclk_333mhz>;
> >> dma-coherent;
> >> };
> >
> > We generally don't refer to register locations with properties other than
> > 'reg', so that approach would be worse. What I'd suggest you do is to
> > have the sgpio registers in a separate device node, and use the LED
> > binding to access it, see
> >
> > Documentation/devicetree/bindings/leds/common.txt
> >
> > It seems that none of the drivers/ata/ drivers use the leds interface
> > today, but that can be added to libata-*.c whenever the appropriate
> > properties are there.
> >
>
> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> to register a custom led control callback. Since Seattle platform does not support
> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
> to indicate that we can still handle the led messages by our registered callback. I see
> that sata_highbank driver is doing something similar.
But if the LEDs are the only thing that is special, I think it makes
more sense to extend the generic driver. This is similar to how the
ahci driver knows how to deal with external PHY, clk, regulator etc
resources. All of those are not part of the AHCI spec, but are common
enough that we want to have them done in a single driver.
We really should not need a special driver just for handling LEDs
when we already deal with more complex stuff, and we have a proper
abstraction for LEDs in the kernel.
> >
> > That way, any driver that ends up needing a register from this block
> > can use the syscon interface to get hold of a mapping, and/or you can
> > have a high-level driver to expose other functionalities. It's probably
> > best to start doing it either entirely using syscon references from
> > other drivers, or using no syscon references, and putting everything into
> > a driver for the register set, but as I said it depends a lot on what
> > else is in there.
> >
> > Can you send me a register list of the 0xe0000000 segment for reference?
> >
> Thanks for pointing syncon, are you thinking something like this ?
>
> sgpio0: syscon@e0000078 {
> compatible = "amd, seattle-sgpio", syscon";
> reg = <0x0 0xe0000078 0x0 0x1>;
> };
A syscon is defined as (roughly) a group of otherwise unrelated registers
that happen to be part of the same physical register area because the SoC
designer couldn't find a proper abstraction for them. When you define
a register area with a single byte in it, that is not a syscon.
>
> sata@e0300000 {
> compatible = "amd,seattle-ahci";
> reg = <0x0 0xe0300000 0x0 0xf0000>;
> interrupts = <0x0 0x163 0x4>;
> amd,sgpio-ctrl = <&sgpio0>;
> dma-coherent;
> };
>
The sata node should list "generic-ahci" so we can bind the normal
driver. You can leave the "amd,seattle-ahci" in addition in case we
ever need to know the difference to work around a bug, but it's really
not needed for the LED.
> SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
> They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
> its just extension of the IP block but it just happened to be way out of the IP block range.
> Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
That is quite normal, a lot of chips have register blocks where one
register is meant for one device only. E.g. the clock controller may have
one register for controlling the clocks of the AHCI device. In the old
days, we would have hacks like what you did to turn on the clocks by poking
the register from the SATA driver, but now we abstract those things using
generic subsystems.
I think you either want a special led controller device node that
refers to the syscon device and exports the LEDs so they can be
accessed by the AHCI device, or do it in the device that contains
the registers.
> But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
>
> + 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;
> +
>
> The above code works on ACPI and DT cases. But if we go with syncon approach
> then we need to handle DT and ACPI differently. Because sycon will provide
> struct regmap instead of struct resources and also make reading/writing a
> bit different. I was trying to minimize the DT vs ACPI changes in the driver
> (other than binding) hence I think defining two ranges in sata controller
> reg property was much cleaner and it aligns with DSDT.
Isn't this the thing that ACPI based firmware would handle using AML anyway?
I don't think the server people would be too happy to add a new driver
each time a SATA controller does the LEDs slightly differently, and this
is really the kind of platform specific hack that AML is meant for.
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-08 22:47 ` Arnd Bergmann
0 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-08 22:47 UTC (permalink / raw)
To: linux-arm-kernel
On Friday 08 January 2016 16:21:50 Brijesh Singh wrote:
> >> Should I consider adding a property "sgpio-ctrl" to pass the register
> >> address ?
> >>
> >> e.g
> >>
> >> sata0 at e0300000 {
> >> compatible = "amd,seattle-ahci";
> >> reg = <0 0xe0300000 0 0x800>;
> >> amd,sgpio-ctrl = <0xe0000078>;
> >> interrupts = <0 355 4>;
> >> clocks = <&sataclk_333mhz>;
> >> dma-coherent;
> >> };
> >
> > We generally don't refer to register locations with properties other than
> > 'reg', so that approach would be worse. What I'd suggest you do is to
> > have the sgpio registers in a separate device node, and use the LED
> > binding to access it, see
> >
> > Documentation/devicetree/bindings/leds/common.txt
> >
> > It seems that none of the drivers/ata/ drivers use the leds interface
> > today, but that can be added to libata-*.c whenever the appropriate
> > properties are there.
> >
>
> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> to register a custom led control callback. Since Seattle platform does not support
> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
> to indicate that we can still handle the led messages by our registered callback. I see
> that sata_highbank driver is doing something similar.
But if the LEDs are the only thing that is special, I think it makes
more sense to extend the generic driver. This is similar to how the
ahci driver knows how to deal with external PHY, clk, regulator etc
resources. All of those are not part of the AHCI spec, but are common
enough that we want to have them done in a single driver.
We really should not need a special driver just for handling LEDs
when we already deal with more complex stuff, and we have a proper
abstraction for LEDs in the kernel.
> >
> > That way, any driver that ends up needing a register from this block
> > can use the syscon interface to get hold of a mapping, and/or you can
> > have a high-level driver to expose other functionalities. It's probably
> > best to start doing it either entirely using syscon references from
> > other drivers, or using no syscon references, and putting everything into
> > a driver for the register set, but as I said it depends a lot on what
> > else is in there.
> >
> > Can you send me a register list of the 0xe0000000 segment for reference?
> >
> Thanks for pointing syncon, are you thinking something like this ?
>
> sgpio0: syscon at e0000078 {
> compatible = "amd, seattle-sgpio", syscon";
> reg = <0x0 0xe0000078 0x0 0x1>;
> };
A syscon is defined as (roughly) a group of otherwise unrelated registers
that happen to be part of the same physical register area because the SoC
designer couldn't find a proper abstraction for them. When you define
a register area with a single byte in it, that is not a syscon.
>
> sata at e0300000 {
> compatible = "amd,seattle-ahci";
> reg = <0x0 0xe0300000 0x0 0xf0000>;
> interrupts = <0x0 0x163 0x4>;
> amd,sgpio-ctrl = <&sgpio0>;
> dma-coherent;
> };
>
The sata node should list "generic-ahci" so we can bind the normal
driver. You can leave the "amd,seattle-ahci" in addition in case we
ever need to know the difference to work around a bug, but it's really
not needed for the LED.
> SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
> They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
> its just extension of the IP block but it just happened to be way out of the IP block range.
> Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
That is quite normal, a lot of chips have register blocks where one
register is meant for one device only. E.g. the clock controller may have
one register for controlling the clocks of the AHCI device. In the old
days, we would have hacks like what you did to turn on the clocks by poking
the register from the SATA driver, but now we abstract those things using
generic subsystems.
I think you either want a special led controller device node that
refers to the syscon device and exports the LEDs so they can be
accessed by the AHCI device, or do it in the device that contains
the registers.
> But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
>
> + 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;
> +
>
> The above code works on ACPI and DT cases. But if we go with syncon approach
> then we need to handle DT and ACPI differently. Because sycon will provide
> struct regmap instead of struct resources and also make reading/writing a
> bit different. I was trying to minimize the DT vs ACPI changes in the driver
> (other than binding) hence I think defining two ranges in sata controller
> reg property was much cleaner and it aligns with DSDT.
Isn't this the thing that ACPI based firmware would handle using AML anyway?
I don't think the server people would be too happy to add a new driver
each time a SATA controller does the LEDs slightly differently, and this
is really the kind of platform specific hack that AML is meant for.
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-08 22:47 ` Arnd Bergmann
(?)
@ 2016-01-11 18:56 ` Brijesh Singh
-1 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-11 18:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: brijesh.singh, linux-arm-kernel, linux-kernel, mark.rutland,
devicetree, pawel.moll, ijc+devicetree, linux-ide, robh+dt,
galak, tj
Hi Arnd,
Thanks for all your valuable feedbacks.
On 01/08/2016 04:47 PM, Arnd Bergmann wrote:
>>
>> libata-*.c implements the "Enclosure management" style led messages but also has hooks
>> to register a custom led control callback. Since Seattle platform does not support
>> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
>> to indicate that we can still handle the led messages by our registered callback. I see
>> that sata_highbank driver is doing something similar.
>
> But if the LEDs are the only thing that is special, I think it makes
> more sense to extend the generic driver. This is similar to how the
> ahci driver knows how to deal with external PHY, clk, regulator etc
> resources. All of those are not part of the AHCI spec, but are common
> enough that we want to have them done in a single driver.
>
> We really should not need a special driver just for handling LEDs
> when we already deal with more complex stuff, and we have a proper
> abstraction for LEDs in the kernel.
>
I took a quick look at LED class and saw ledtrig-ide-disk.c (ledtrig_ide_activity). It seems this approach was used
in deprecate ide-disk driver for blinking the activity led. Are you thinking that we implement something similar
in libahci to control the LED blinking? Looking at current libahci gives me feeling that LED's are considered as
part of AHCI enclosure management implementation and hence LED triggers are not exposed outside the library.
If I am missing something then please correct me.
>
> A syscon is defined as (roughly) a group of otherwise unrelated registers
> that happen to be part of the same physical register area because the SoC
> designer couldn't find a proper abstraction for them. When you define
> a register area with a single byte in it, that is not a syscon.
>
>>
>> sata@e0300000 {
>> compatible = "amd,seattle-ahci";
>> reg = <0x0 0xe0300000 0x0 0xf0000>;
>> interrupts = <0x0 0x163 0x4>;
>> amd,sgpio-ctrl = <&sgpio0>;
>> dma-coherent;
>> };
>>
>
>
> The sata node should list "generic-ahci" so we can bind the normal
> driver. You can leave the "amd,seattle-ahci" in addition in case we
> ever need to know the difference to work around a bug, but it's really
> not needed for the LED.
>
>> SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
>> They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
>> its just extension of the IP block but it just happened to be way out of the IP block range.
>> Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
>
> That is quite normal, a lot of chips have register blocks where one
> register is meant for one device only. E.g. the clock controller may have
> one register for controlling the clocks of the AHCI device. In the old
> days, we would have hacks like what you did to turn on the clocks by poking
> the register from the SATA driver, but now we abstract those things using
> generic subsystems.
>
> I think you either want a special led controller device node that
> refers to the syscon device and exports the LEDs so they can be
> accessed by the AHCI device, or do it in the device that contains
> the registers.
>
>> But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
>>
>> + 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;
>> +
>>
>> The above code works on ACPI and DT cases. But if we go with syncon approach
>> then we need to handle DT and ACPI differently. Because sycon will provide
>> struct regmap instead of struct resources and also make reading/writing a
>> bit different. I was trying to minimize the DT vs ACPI changes in the driver
>> (other than binding) hence I think defining two ranges in sata controller
>> reg property was much cleaner and it aligns with DSDT.
>
> Isn't this the thing that ACPI based firmware would handle using AML anyway?
> I don't think the server people would be too happy to add a new driver
> each time a SATA controller does the LEDs slightly differently, and this
> is really the kind of platform specific hack that AML is meant for.
>
Sorry I am not able understand your comment, Could you please explain me what you mean by AML is meant for this kind of platform specific hack ?
"SATA0" and "SATA1" DSDT Resource template contains two resources (#0 SATA block register and #1 SGPIO control register).
Device (SATA0)
{
Name(_CRS, ResourceTemplate()
{
Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)
Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
Memory32Fixed(ReadWrite, 0xE00000078, 1)
}
}
Device (SATA1)
{
Name(_CRS, ResourceTemplate()
{
Memory32Fixed(ReadWrite, 0xE0D100000, 0x000010000)
Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 393}
Memory32Fixed(ReadWrite, 0xE0000007C, 1)
}
}
The generic ahci platform driver calls ahci_platform_get_resources() to get the resources.
ahci_platform_get_resources() maps the resource #0. Since the SGPIO control resource is part of SATA device
resource hence I could simply ioremap resource #1 for both DT and ACPI cases. Since windows driver is making
use of same DSDT hence its going to be tricky to modify the DSDT to create a new device entry for SATA LED.
Before creating a new driver, I did attempt to extend the existing ahci_platform driver and was able to get it working but it was not looking good hence created a new driver. One of the issue was knowing when to map
the SGPIO control register in ACPI case. ahci_platform driver does binding based on _CLS mask so I was not sure
whether doing ioremap of resource#1 is safe and would not cause problem on non Seattle platform. If you want then
i can send you patch for quick comment and see if that make sense instead of adding a new driver.
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-11 18:56 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-11 18:56 UTC (permalink / raw)
To: linux-arm-kernel
Hi Arnd,
Thanks for all your valuable feedbacks.
On 01/08/2016 04:47 PM, Arnd Bergmann wrote:
>>
>> libata-*.c implements the "Enclosure management" style led messages but also has hooks
>> to register a custom led control callback. Since Seattle platform does not support
>> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
>> to indicate that we can still handle the led messages by our registered callback. I see
>> that sata_highbank driver is doing something similar.
>
> But if the LEDs are the only thing that is special, I think it makes
> more sense to extend the generic driver. This is similar to how the
> ahci driver knows how to deal with external PHY, clk, regulator etc
> resources. All of those are not part of the AHCI spec, but are common
> enough that we want to have them done in a single driver.
>
> We really should not need a special driver just for handling LEDs
> when we already deal with more complex stuff, and we have a proper
> abstraction for LEDs in the kernel.
>
I took a quick look at LED class and saw ledtrig-ide-disk.c (ledtrig_ide_activity). It seems this approach was used
in deprecate ide-disk driver for blinking the activity led. Are you thinking that we implement something similar
in libahci to control the LED blinking? Looking at current libahci gives me feeling that LED's are considered as
part of AHCI enclosure management implementation and hence LED triggers are not exposed outside the library.
If I am missing something then please correct me.
>
> A syscon is defined as (roughly) a group of otherwise unrelated registers
> that happen to be part of the same physical register area because the SoC
> designer couldn't find a proper abstraction for them. When you define
> a register area with a single byte in it, that is not a syscon.
>
>>
>> sata at e0300000 {
>> compatible = "amd,seattle-ahci";
>> reg = <0x0 0xe0300000 0x0 0xf0000>;
>> interrupts = <0x0 0x163 0x4>;
>> amd,sgpio-ctrl = <&sgpio0>;
>> dma-coherent;
>> };
>>
>
>
> The sata node should list "generic-ahci" so we can bind the normal
> driver. You can leave the "amd,seattle-ahci" in addition in case we
> ever need to know the difference to work around a bug, but it's really
> not needed for the LED.
>
>> SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
>> They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
>> its just extension of the IP block but it just happened to be way out of the IP block range.
>> Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
>
> That is quite normal, a lot of chips have register blocks where one
> register is meant for one device only. E.g. the clock controller may have
> one register for controlling the clocks of the AHCI device. In the old
> days, we would have hacks like what you did to turn on the clocks by poking
> the register from the SATA driver, but now we abstract those things using
> generic subsystems.
>
> I think you either want a special led controller device node that
> refers to the syscon device and exports the LEDs so they can be
> accessed by the AHCI device, or do it in the device that contains
> the registers.
>
>> But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
>>
>> + 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;
>> +
>>
>> The above code works on ACPI and DT cases. But if we go with syncon approach
>> then we need to handle DT and ACPI differently. Because sycon will provide
>> struct regmap instead of struct resources and also make reading/writing a
>> bit different. I was trying to minimize the DT vs ACPI changes in the driver
>> (other than binding) hence I think defining two ranges in sata controller
>> reg property was much cleaner and it aligns with DSDT.
>
> Isn't this the thing that ACPI based firmware would handle using AML anyway?
> I don't think the server people would be too happy to add a new driver
> each time a SATA controller does the LEDs slightly differently, and this
> is really the kind of platform specific hack that AML is meant for.
>
Sorry I am not able understand your comment, Could you please explain me what you mean by AML is meant for this kind of platform specific hack ?
"SATA0" and "SATA1" DSDT Resource template contains two resources (#0 SATA block register and #1 SGPIO control register).
Device (SATA0)
{
Name(_CRS, ResourceTemplate()
{
Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)
Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
Memory32Fixed(ReadWrite, 0xE00000078, 1)
}
}
Device (SATA1)
{
Name(_CRS, ResourceTemplate()
{
Memory32Fixed(ReadWrite, 0xE0D100000, 0x000010000)
Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 393}
Memory32Fixed(ReadWrite, 0xE0000007C, 1)
}
}
The generic ahci platform driver calls ahci_platform_get_resources() to get the resources.
ahci_platform_get_resources() maps the resource #0. Since the SGPIO control resource is part of SATA device
resource hence I could simply ioremap resource #1 for both DT and ACPI cases. Since windows driver is making
use of same DSDT hence its going to be tricky to modify the DSDT to create a new device entry for SATA LED.
Before creating a new driver, I did attempt to extend the existing ahci_platform driver and was able to get it working but it was not looking good hence created a new driver. One of the issue was knowing when to map
the SGPIO control register in ACPI case. ahci_platform driver does binding based on _CLS mask so I was not sure
whether doing ioremap of resource#1 is safe and would not cause problem on non Seattle platform. If you want then
i can send you patch for quick comment and see if that make sense instead of adding a new driver.
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-11 18:56 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-11 18:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: brijesh.singh, linux-arm-kernel, linux-kernel, mark.rutland,
devicetree, pawel.moll, ijc+devicetree, linux-ide, robh+dt,
galak, tj
Hi Arnd,
Thanks for all your valuable feedbacks.
On 01/08/2016 04:47 PM, Arnd Bergmann wrote:
>>
>> libata-*.c implements the "Enclosure management" style led messages but also has hooks
>> to register a custom led control callback. Since Seattle platform does not support
>> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
>> to indicate that we can still handle the led messages by our registered callback. I see
>> that sata_highbank driver is doing something similar.
>
> But if the LEDs are the only thing that is special, I think it makes
> more sense to extend the generic driver. This is similar to how the
> ahci driver knows how to deal with external PHY, clk, regulator etc
> resources. All of those are not part of the AHCI spec, but are common
> enough that we want to have them done in a single driver.
>
> We really should not need a special driver just for handling LEDs
> when we already deal with more complex stuff, and we have a proper
> abstraction for LEDs in the kernel.
>
I took a quick look at LED class and saw ledtrig-ide-disk.c (ledtrig_ide_activity). It seems this approach was used
in deprecate ide-disk driver for blinking the activity led. Are you thinking that we implement something similar
in libahci to control the LED blinking? Looking at current libahci gives me feeling that LED's are considered as
part of AHCI enclosure management implementation and hence LED triggers are not exposed outside the library.
If I am missing something then please correct me.
>
> A syscon is defined as (roughly) a group of otherwise unrelated registers
> that happen to be part of the same physical register area because the SoC
> designer couldn't find a proper abstraction for them. When you define
> a register area with a single byte in it, that is not a syscon.
>
>>
>> sata@e0300000 {
>> compatible = "amd,seattle-ahci";
>> reg = <0x0 0xe0300000 0x0 0xf0000>;
>> interrupts = <0x0 0x163 0x4>;
>> amd,sgpio-ctrl = <&sgpio0>;
>> dma-coherent;
>> };
>>
>
>
> The sata node should list "generic-ahci" so we can bind the normal
> driver. You can leave the "amd,seattle-ahci" in addition in case we
> ever need to know the difference to work around a bug, but it's really
> not needed for the LED.
>
>> SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
>> They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
>> its just extension of the IP block but it just happened to be way out of the IP block range.
>> Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
>
> That is quite normal, a lot of chips have register blocks where one
> register is meant for one device only. E.g. the clock controller may have
> one register for controlling the clocks of the AHCI device. In the old
> days, we would have hacks like what you did to turn on the clocks by poking
> the register from the SATA driver, but now we abstract those things using
> generic subsystems.
>
> I think you either want a special led controller device node that
> refers to the syscon device and exports the LEDs so they can be
> accessed by the AHCI device, or do it in the device that contains
> the registers.
>
>> But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
>>
>> + 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;
>> +
>>
>> The above code works on ACPI and DT cases. But if we go with syncon approach
>> then we need to handle DT and ACPI differently. Because sycon will provide
>> struct regmap instead of struct resources and also make reading/writing a
>> bit different. I was trying to minimize the DT vs ACPI changes in the driver
>> (other than binding) hence I think defining two ranges in sata controller
>> reg property was much cleaner and it aligns with DSDT.
>
> Isn't this the thing that ACPI based firmware would handle using AML anyway?
> I don't think the server people would be too happy to add a new driver
> each time a SATA controller does the LEDs slightly differently, and this
> is really the kind of platform specific hack that AML is meant for.
>
Sorry I am not able understand your comment, Could you please explain me what you mean by AML is meant for this kind of platform specific hack ?
"SATA0" and "SATA1" DSDT Resource template contains two resources (#0 SATA block register and #1 SGPIO control register).
Device (SATA0)
{
Name(_CRS, ResourceTemplate()
{
Memory32Fixed(ReadWrite, 0xE03000000, 0x000010000)
Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 387}
Memory32Fixed(ReadWrite, 0xE00000078, 1)
}
}
Device (SATA1)
{
Name(_CRS, ResourceTemplate()
{
Memory32Fixed(ReadWrite, 0xE0D100000, 0x000010000)
Interrupt(ResourceConsumer, Level, ActiveHigh Exclusive,,,) { 393}
Memory32Fixed(ReadWrite, 0xE0000007C, 1)
}
}
The generic ahci platform driver calls ahci_platform_get_resources() to get the resources.
ahci_platform_get_resources() maps the resource #0. Since the SGPIO control resource is part of SATA device
resource hence I could simply ioremap resource #1 for both DT and ACPI cases. Since windows driver is making
use of same DSDT hence its going to be tricky to modify the DSDT to create a new device entry for SATA LED.
Before creating a new driver, I did attempt to extend the existing ahci_platform driver and was able to get it working but it was not looking good hence created a new driver. One of the issue was knowing when to map
the SGPIO control register in ACPI case. ahci_platform driver does binding based on _CLS mask so I was not sure
whether doing ioremap of resource#1 is safe and would not cause problem on non Seattle platform. If you want then
i can send you patch for quick comment and see if that make sense instead of adding a new driver.
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread[parent not found: <5693FAC2.4070003-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-11 18:56 ` Brijesh Singh
(?)
@ 2016-01-12 14:24 ` Arnd Bergmann
-1 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-12 14:24 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA, pawel.moll-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
linux-ide-u79uwXL29TY76Z2rM5mHXA, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
galak-sgV2jX0FEOL9JmXXK+q4OQ, tj-DgEjT+Ai2ygdnm+yROfE0A
On Monday 11 January 2016 12:56:02 Brijesh Singh wrote:
> Hi Arnd,
>
> Thanks for all your valuable feedbacks.
>
> On 01/08/2016 04:47 PM, Arnd Bergmann wrote:
> >>
> >> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> >> to register a custom led control callback. Since Seattle platform does not support
> >> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
> >> to indicate that we can still handle the led messages by our registered callback. I see
> >> that sata_highbank driver is doing something similar.
> >
> > But if the LEDs are the only thing that is special, I think it makes
> > more sense to extend the generic driver. This is similar to how the
> > ahci driver knows how to deal with external PHY, clk, regulator etc
> > resources. All of those are not part of the AHCI spec, but are common
> > enough that we want to have them done in a single driver.
> >
> > We really should not need a special driver just for handling LEDs
> > when we already deal with more complex stuff, and we have a proper
> > abstraction for LEDs in the kernel.
> >
>
> I took a quick look at LED class and saw ledtrig-ide-disk.c (ledtrig_ide_activity). It seems this approach was used
> in deprecate ide-disk driver for blinking the activity led. Are you thinking that we implement something similar
> in libahci to control the LED blinking?
Yes. I'm not overly familiar with the LED framework, but it seems to me that
something along those lines is the right idea. Note that the driver predates
devicetree and it only handles a single LED, so you probably need to do
something a bit more sophisticated to handle each LED separately and connect
it do the right disk through DT.
We actually seem to have quite a number of platforms relying on this:
$ git grep -w "ide-disk"
Documentation/devicetree/bindings/leds/common.txt: "ide-disk" - LED indicates disk activity
Documentation/devicetree/bindings/leds/leds-gpio.txt: linux,default-trigger = "ide-disk";
arch/arm/boot/dts/am57xx-beagle-x15.dts: linux,default-trigger = "ide-disk";
arch/arm/boot/dts/kirkwood-ns2lite.dts: linux,default-trigger = "ide-disk";
arch/arm/boot/dts/kirkwood-topkick.dts: linux,default-trigger = "ide-disk";
arch/arm/mach-davinci/board-dm644x-evm.c: .default_trigger = "ide-disk", },
arch/arm/mach-omap1/board-osk.c: .default_trigger = "ide-disk", },
arch/arm/mach-pxa/spitz.c: .default_trigger = "ide-disk",
arch/mips/txx9/generic/setup.c: "ide-disk",
arch/mips/txx9/rbtx4939/setup.c: "ide-disk",
arch/powerpc/boot/dts/mpc8315erdb.dts: linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8377_rdb.dts: linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8378_rdb.dts: linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8379_rdb.dts: linux,default-trigger = "ide-disk";
arch/unicore32/kernel/gpio.c: .default_trigger = "ide-disk", },
drivers/leds/leds-hp6xx.c: .default_trigger = "ide-disk",
drivers/leds/trigger/Makefile:obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK) += ledtrig-ide-disk.o
drivers/leds/trigger/ledtrig-camera.c: * based on ledtrig-ide-disk.c
drivers/leds/trigger/ledtrig-ide-disk.c: led_trigger_register_simple("ide-disk", &ledtrig_ide);
drivers/macintosh/Kconfig: and the ide-disk LED trigger and configure appropriately through
drivers/macintosh/via-pmu-led.c: .default_trigger = "ide-disk",
I suspect this used to work in the past, but got broken when people moved
away from drivers/ide to drivers/ata, and nobody properly debugged the problem.
If you fix this right, the LEDs on all those platforms should light up again.
> Looking at current libahci gives me feeling that LED's are considered as
> part of AHCI enclosure management implementation and hence LED triggers
> are not exposed outside the library.
> If I am missing something then please correct me.
Yes, this sounds correct.
> > A syscon is defined as (roughly) a group of otherwise unrelated registers
> > that happen to be part of the same physical register area because the SoC
> > designer couldn't find a proper abstraction for them. When you define
> > a register area with a single byte in it, that is not a syscon.
> >
> >>
> >> sata@e0300000 {
> >> compatible = "amd,seattle-ahci";
> >> reg = <0x0 0xe0300000 0x0 0xf0000>;
> >> interrupts = <0x0 0x163 0x4>;
> >> amd,sgpio-ctrl = <&sgpio0>;
> >> dma-coherent;
> >> };
> >>
> >
> >
> > The sata node should list "generic-ahci" so we can bind the normal
> > driver. You can leave the "amd,seattle-ahci" in addition in case we
> > ever need to know the difference to work around a bug, but it's really
> > not needed for the LED.
> >
> >> SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
> >> They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
> >> its just extension of the IP block but it just happened to be way out of the IP block range.
> >> Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
> >
> > That is quite normal, a lot of chips have register blocks where one
> > register is meant for one device only. E.g. the clock controller may have
> > one register for controlling the clocks of the AHCI device. In the old
> > days, we would have hacks like what you did to turn on the clocks by poking
> > the register from the SATA driver, but now we abstract those things using
> > generic subsystems.
> >
> > I think you either want a special led controller device node that
> > refers to the syscon device and exports the LEDs so they can be
> > accessed by the AHCI device, or do it in the device that contains
> > the registers.
> >
> >> But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
> >>
> >> + 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;
> >> +
> >>
> >> The above code works on ACPI and DT cases. But if we go with syncon approach
> >> then we need to handle DT and ACPI differently. Because sycon will provide
> >> struct regmap instead of struct resources and also make reading/writing a
> >> bit different. I was trying to minimize the DT vs ACPI changes in the driver
> >> (other than binding) hence I think defining two ranges in sata controller
> >> reg property was much cleaner and it aligns with DSDT.
> >
> > Isn't this the thing that ACPI based firmware would handle using AML anyway?
> > I don't think the server people would be too happy to add a new driver
> > each time a SATA controller does the LEDs slightly differently, and this
> > is really the kind of platform specific hack that AML is meant for.
> >
> Sorry I am not able understand your comment, Could you please explain me what you mean by AML is meant for this kind of platform specific hack ?
>
I meant the sgpio register should not be exposed through a resource on
an ACPI based system but the access be hidden behind a call into an AML
method.
You basically extend the generic AHCI driver to understand three ways of
blinking the LEDS:
a) standard AHCI enclosure management
b) the Linux LED subsystem using whatever LED implementation the platform provides
c) calling into the ACPI interpreter to do platform specific hacks
Arnd
--
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
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-12 14:24 ` Arnd Bergmann
0 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-12 14:24 UTC (permalink / raw)
To: linux-arm-kernel
On Monday 11 January 2016 12:56:02 Brijesh Singh wrote:
> Hi Arnd,
>
> Thanks for all your valuable feedbacks.
>
> On 01/08/2016 04:47 PM, Arnd Bergmann wrote:
> >>
> >> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> >> to register a custom led control callback. Since Seattle platform does not support
> >> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
> >> to indicate that we can still handle the led messages by our registered callback. I see
> >> that sata_highbank driver is doing something similar.
> >
> > But if the LEDs are the only thing that is special, I think it makes
> > more sense to extend the generic driver. This is similar to how the
> > ahci driver knows how to deal with external PHY, clk, regulator etc
> > resources. All of those are not part of the AHCI spec, but are common
> > enough that we want to have them done in a single driver.
> >
> > We really should not need a special driver just for handling LEDs
> > when we already deal with more complex stuff, and we have a proper
> > abstraction for LEDs in the kernel.
> >
>
> I took a quick look at LED class and saw ledtrig-ide-disk.c (ledtrig_ide_activity). It seems this approach was used
> in deprecate ide-disk driver for blinking the activity led. Are you thinking that we implement something similar
> in libahci to control the LED blinking?
Yes. I'm not overly familiar with the LED framework, but it seems to me that
something along those lines is the right idea. Note that the driver predates
devicetree and it only handles a single LED, so you probably need to do
something a bit more sophisticated to handle each LED separately and connect
it do the right disk through DT.
We actually seem to have quite a number of platforms relying on this:
$ git grep -w "ide-disk"
Documentation/devicetree/bindings/leds/common.txt: "ide-disk" - LED indicates disk activity
Documentation/devicetree/bindings/leds/leds-gpio.txt: linux,default-trigger = "ide-disk";
arch/arm/boot/dts/am57xx-beagle-x15.dts: linux,default-trigger = "ide-disk";
arch/arm/boot/dts/kirkwood-ns2lite.dts: linux,default-trigger = "ide-disk";
arch/arm/boot/dts/kirkwood-topkick.dts: linux,default-trigger = "ide-disk";
arch/arm/mach-davinci/board-dm644x-evm.c: .default_trigger = "ide-disk", },
arch/arm/mach-omap1/board-osk.c: .default_trigger = "ide-disk", },
arch/arm/mach-pxa/spitz.c: .default_trigger = "ide-disk",
arch/mips/txx9/generic/setup.c: "ide-disk",
arch/mips/txx9/rbtx4939/setup.c: "ide-disk",
arch/powerpc/boot/dts/mpc8315erdb.dts: linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8377_rdb.dts: linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8378_rdb.dts: linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8379_rdb.dts: linux,default-trigger = "ide-disk";
arch/unicore32/kernel/gpio.c: .default_trigger = "ide-disk", },
drivers/leds/leds-hp6xx.c: .default_trigger = "ide-disk",
drivers/leds/trigger/Makefile:obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK) += ledtrig-ide-disk.o
drivers/leds/trigger/ledtrig-camera.c: * based on ledtrig-ide-disk.c
drivers/leds/trigger/ledtrig-ide-disk.c: led_trigger_register_simple("ide-disk", &ledtrig_ide);
drivers/macintosh/Kconfig: and the ide-disk LED trigger and configure appropriately through
drivers/macintosh/via-pmu-led.c: .default_trigger = "ide-disk",
I suspect this used to work in the past, but got broken when people moved
away from drivers/ide to drivers/ata, and nobody properly debugged the problem.
If you fix this right, the LEDs on all those platforms should light up again.
> Looking at current libahci gives me feeling that LED's are considered as
> part of AHCI enclosure management implementation and hence LED triggers
> are not exposed outside the library.
> If I am missing something then please correct me.
Yes, this sounds correct.
> > A syscon is defined as (roughly) a group of otherwise unrelated registers
> > that happen to be part of the same physical register area because the SoC
> > designer couldn't find a proper abstraction for them. When you define
> > a register area with a single byte in it, that is not a syscon.
> >
> >>
> >> sata at e0300000 {
> >> compatible = "amd,seattle-ahci";
> >> reg = <0x0 0xe0300000 0x0 0xf0000>;
> >> interrupts = <0x0 0x163 0x4>;
> >> amd,sgpio-ctrl = <&sgpio0>;
> >> dma-coherent;
> >> };
> >>
> >
> >
> > The sata node should list "generic-ahci" so we can bind the normal
> > driver. You can leave the "amd,seattle-ahci" in addition in case we
> > ever need to know the difference to work around a bug, but it's really
> > not needed for the LED.
> >
> >> SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
> >> They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
> >> its just extension of the IP block but it just happened to be way out of the IP block range.
> >> Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
> >
> > That is quite normal, a lot of chips have register blocks where one
> > register is meant for one device only. E.g. the clock controller may have
> > one register for controlling the clocks of the AHCI device. In the old
> > days, we would have hacks like what you did to turn on the clocks by poking
> > the register from the SATA driver, but now we abstract those things using
> > generic subsystems.
> >
> > I think you either want a special led controller device node that
> > refers to the syscon device and exports the LEDs so they can be
> > accessed by the AHCI device, or do it in the device that contains
> > the registers.
> >
> >> But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
> >>
> >> + 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;
> >> +
> >>
> >> The above code works on ACPI and DT cases. But if we go with syncon approach
> >> then we need to handle DT and ACPI differently. Because sycon will provide
> >> struct regmap instead of struct resources and also make reading/writing a
> >> bit different. I was trying to minimize the DT vs ACPI changes in the driver
> >> (other than binding) hence I think defining two ranges in sata controller
> >> reg property was much cleaner and it aligns with DSDT.
> >
> > Isn't this the thing that ACPI based firmware would handle using AML anyway?
> > I don't think the server people would be too happy to add a new driver
> > each time a SATA controller does the LEDs slightly differently, and this
> > is really the kind of platform specific hack that AML is meant for.
> >
> Sorry I am not able understand your comment, Could you please explain me what you mean by AML is meant for this kind of platform specific hack ?
>
I meant the sgpio register should not be exposed through a resource on
an ACPI based system but the access be hidden behind a call into an AML
method.
You basically extend the generic AHCI driver to understand three ways of
blinking the LEDS:
a) standard AHCI enclosure management
b) the Linux LED subsystem using whatever LED implementation the platform provides
c) calling into the ACPI interpreter to do platform specific hacks
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-12 14:24 ` Arnd Bergmann
0 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-12 14:24 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-arm-kernel, linux-kernel, mark.rutland, devicetree,
pawel.moll, ijc+devicetree, linux-ide, robh+dt, galak, tj
On Monday 11 January 2016 12:56:02 Brijesh Singh wrote:
> Hi Arnd,
>
> Thanks for all your valuable feedbacks.
>
> On 01/08/2016 04:47 PM, Arnd Bergmann wrote:
> >>
> >> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> >> to register a custom led control callback. Since Seattle platform does not support
> >> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
> >> to indicate that we can still handle the led messages by our registered callback. I see
> >> that sata_highbank driver is doing something similar.
> >
> > But if the LEDs are the only thing that is special, I think it makes
> > more sense to extend the generic driver. This is similar to how the
> > ahci driver knows how to deal with external PHY, clk, regulator etc
> > resources. All of those are not part of the AHCI spec, but are common
> > enough that we want to have them done in a single driver.
> >
> > We really should not need a special driver just for handling LEDs
> > when we already deal with more complex stuff, and we have a proper
> > abstraction for LEDs in the kernel.
> >
>
> I took a quick look at LED class and saw ledtrig-ide-disk.c (ledtrig_ide_activity). It seems this approach was used
> in deprecate ide-disk driver for blinking the activity led. Are you thinking that we implement something similar
> in libahci to control the LED blinking?
Yes. I'm not overly familiar with the LED framework, but it seems to me that
something along those lines is the right idea. Note that the driver predates
devicetree and it only handles a single LED, so you probably need to do
something a bit more sophisticated to handle each LED separately and connect
it do the right disk through DT.
We actually seem to have quite a number of platforms relying on this:
$ git grep -w "ide-disk"
Documentation/devicetree/bindings/leds/common.txt: "ide-disk" - LED indicates disk activity
Documentation/devicetree/bindings/leds/leds-gpio.txt: linux,default-trigger = "ide-disk";
arch/arm/boot/dts/am57xx-beagle-x15.dts: linux,default-trigger = "ide-disk";
arch/arm/boot/dts/kirkwood-ns2lite.dts: linux,default-trigger = "ide-disk";
arch/arm/boot/dts/kirkwood-topkick.dts: linux,default-trigger = "ide-disk";
arch/arm/mach-davinci/board-dm644x-evm.c: .default_trigger = "ide-disk", },
arch/arm/mach-omap1/board-osk.c: .default_trigger = "ide-disk", },
arch/arm/mach-pxa/spitz.c: .default_trigger = "ide-disk",
arch/mips/txx9/generic/setup.c: "ide-disk",
arch/mips/txx9/rbtx4939/setup.c: "ide-disk",
arch/powerpc/boot/dts/mpc8315erdb.dts: linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8377_rdb.dts: linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8378_rdb.dts: linux,default-trigger = "ide-disk";
arch/powerpc/boot/dts/mpc8379_rdb.dts: linux,default-trigger = "ide-disk";
arch/unicore32/kernel/gpio.c: .default_trigger = "ide-disk", },
drivers/leds/leds-hp6xx.c: .default_trigger = "ide-disk",
drivers/leds/trigger/Makefile:obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK) += ledtrig-ide-disk.o
drivers/leds/trigger/ledtrig-camera.c: * based on ledtrig-ide-disk.c
drivers/leds/trigger/ledtrig-ide-disk.c: led_trigger_register_simple("ide-disk", &ledtrig_ide);
drivers/macintosh/Kconfig: and the ide-disk LED trigger and configure appropriately through
drivers/macintosh/via-pmu-led.c: .default_trigger = "ide-disk",
I suspect this used to work in the past, but got broken when people moved
away from drivers/ide to drivers/ata, and nobody properly debugged the problem.
If you fix this right, the LEDs on all those platforms should light up again.
> Looking at current libahci gives me feeling that LED's are considered as
> part of AHCI enclosure management implementation and hence LED triggers
> are not exposed outside the library.
> If I am missing something then please correct me.
Yes, this sounds correct.
> > A syscon is defined as (roughly) a group of otherwise unrelated registers
> > that happen to be part of the same physical register area because the SoC
> > designer couldn't find a proper abstraction for them. When you define
> > a register area with a single byte in it, that is not a syscon.
> >
> >>
> >> sata@e0300000 {
> >> compatible = "amd,seattle-ahci";
> >> reg = <0x0 0xe0300000 0x0 0xf0000>;
> >> interrupts = <0x0 0x163 0x4>;
> >> amd,sgpio-ctrl = <&sgpio0>;
> >> dma-coherent;
> >> };
> >>
> >
> >
> > The sata node should list "generic-ahci" so we can bind the normal
> > driver. You can leave the "amd,seattle-ahci" in addition in case we
> > ever need to know the difference to work around a bug, but it's really
> > not needed for the LED.
> >
> >> SGPIO0 (0xe0000078) and SGPIO1 (0xe000007C) does not belong to any system control register set.
> >> They are meant to be used by SATA driver only and no other driver should every touch it. It almost feels
> >> its just extension of the IP block but it just happened to be way out of the IP block range.
> >> Other register near to 0xe000_0078 and 0xe000_007C should not be mapped by OS.
> >
> > That is quite normal, a lot of chips have register blocks where one
> > register is meant for one device only. E.g. the clock controller may have
> > one register for controlling the clocks of the AHCI device. In the old
> > days, we would have hacks like what you did to turn on the clocks by poking
> > the register from the SATA driver, but now we abstract those things using
> > generic subsystems.
> >
> > I think you either want a special led controller device node that
> > refers to the syscon device and exports the LEDs so they can be
> > accessed by the AHCI device, or do it in the device that contains
> > the registers.
> >
> >> But this syscon approach also brings another problem. Currently my code is pretty simple for mapping this regs
> >>
> >> + 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;
> >> +
> >>
> >> The above code works on ACPI and DT cases. But if we go with syncon approach
> >> then we need to handle DT and ACPI differently. Because sycon will provide
> >> struct regmap instead of struct resources and also make reading/writing a
> >> bit different. I was trying to minimize the DT vs ACPI changes in the driver
> >> (other than binding) hence I think defining two ranges in sata controller
> >> reg property was much cleaner and it aligns with DSDT.
> >
> > Isn't this the thing that ACPI based firmware would handle using AML anyway?
> > I don't think the server people would be too happy to add a new driver
> > each time a SATA controller does the LEDs slightly differently, and this
> > is really the kind of platform specific hack that AML is meant for.
> >
> Sorry I am not able understand your comment, Could you please explain me what you mean by AML is meant for this kind of platform specific hack ?
>
I meant the sgpio register should not be exposed through a resource on
an ACPI based system but the access be hidden behind a call into an AML
method.
You basically extend the generic AHCI driver to understand three ways of
blinking the LEDS:
a) standard AHCI enclosure management
b) the Linux LED subsystem using whatever LED implementation the platform provides
c) calling into the ACPI interpreter to do platform specific hacks
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-12 14:24 ` Arnd Bergmann
(?)
@ 2016-01-13 16:55 ` Brijesh Singh
-1 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-13 16:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: mark.rutland, devicetree, brijesh.singh, pawel.moll,
ijc+devicetree, linux-kernel, linux-ide, robh+dt, galak, tj,
linux-arm-kernel
Hi,
On 01/12/2016 08:24 AM, Arnd Bergmann wrote:
>>>
>>> Isn't this the thing that ACPI based firmware would handle using AML anyway?
>>> I don't think the server people would be too happy to add a new driver
>>> each time a SATA controller does the LEDs slightly differently, and this
>>> is really the kind of platform specific hack that AML is meant for.
>>>
>> Sorry I am not able understand your comment, Could you please explain me what you mean by AML is meant for this kind of platform specific hack ?
>>
>
> I meant the sgpio register should not be exposed through a resource on
> an ACPI based system but the access be hidden behind a call into an AML
> method.
>
> You basically extend the generic AHCI driver to understand three ways of
> blinking the LEDS:
>
> a) standard AHCI enclosure management
> b) the Linux LED subsystem using whatever LED implementation the platform provides
Yes I could look into exploring LED implementation for DT case.
> c) calling into the ACPI interpreter to do platform specific hacks
Thanks for explaining. Now I understood your comment on AML however we need to consider the following:
a) activity LED blinking routines are called very frequent (10 to 100ms based on emp->state) and executing AML method that often would introduces its own overhead.
b) all BIOS vendors need to implement a new methods in their DSDT and release a new BIOS.
c) other OS'es (mainly Windows) driver need to be updated.
We don't know how many other SoC's have similar hacky implementation which can take advantage of extending generic
AHCI driver to call into AML methods for LED blink. Given some of these causes I think having a platform driver is much cleaner. I can drop DT binding part from this driver and keep just ACPI binding.
Thoughts ?
-Brijesh
>
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-13 16:55 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-13 16:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 01/12/2016 08:24 AM, Arnd Bergmann wrote:
>>>
>>> Isn't this the thing that ACPI based firmware would handle using AML anyway?
>>> I don't think the server people would be too happy to add a new driver
>>> each time a SATA controller does the LEDs slightly differently, and this
>>> is really the kind of platform specific hack that AML is meant for.
>>>
>> Sorry I am not able understand your comment, Could you please explain me what you mean by AML is meant for this kind of platform specific hack ?
>>
>
> I meant the sgpio register should not be exposed through a resource on
> an ACPI based system but the access be hidden behind a call into an AML
> method.
>
> You basically extend the generic AHCI driver to understand three ways of
> blinking the LEDS:
>
> a) standard AHCI enclosure management
> b) the Linux LED subsystem using whatever LED implementation the platform provides
Yes I could look into exploring LED implementation for DT case.
> c) calling into the ACPI interpreter to do platform specific hacks
Thanks for explaining. Now I understood your comment on AML however we need to consider the following:
a) activity LED blinking routines are called very frequent (10 to 100ms based on emp->state) and executing AML method that often would introduces its own overhead.
b) all BIOS vendors need to implement a new methods in their DSDT and release a new BIOS.
c) other OS'es (mainly Windows) driver need to be updated.
We don't know how many other SoC's have similar hacky implementation which can take advantage of extending generic
AHCI driver to call into AML methods for LED blink. Given some of these causes I think having a platform driver is much cleaner. I can drop DT binding part from this driver and keep just ACPI binding.
Thoughts ?
-Brijesh
>
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-13 16:55 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-13 16:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: brijesh.singh, linux-arm-kernel, linux-kernel, mark.rutland,
devicetree, pawel.moll, ijc+devicetree, linux-ide, robh+dt,
galak, tj
Hi,
On 01/12/2016 08:24 AM, Arnd Bergmann wrote:
>>>
>>> Isn't this the thing that ACPI based firmware would handle using AML anyway?
>>> I don't think the server people would be too happy to add a new driver
>>> each time a SATA controller does the LEDs slightly differently, and this
>>> is really the kind of platform specific hack that AML is meant for.
>>>
>> Sorry I am not able understand your comment, Could you please explain me what you mean by AML is meant for this kind of platform specific hack ?
>>
>
> I meant the sgpio register should not be exposed through a resource on
> an ACPI based system but the access be hidden behind a call into an AML
> method.
>
> You basically extend the generic AHCI driver to understand three ways of
> blinking the LEDS:
>
> a) standard AHCI enclosure management
> b) the Linux LED subsystem using whatever LED implementation the platform provides
Yes I could look into exploring LED implementation for DT case.
> c) calling into the ACPI interpreter to do platform specific hacks
Thanks for explaining. Now I understood your comment on AML however we need to consider the following:
a) activity LED blinking routines are called very frequent (10 to 100ms based on emp->state) and executing AML method that often would introduces its own overhead.
b) all BIOS vendors need to implement a new methods in their DSDT and release a new BIOS.
c) other OS'es (mainly Windows) driver need to be updated.
We don't know how many other SoC's have similar hacky implementation which can take advantage of extending generic
AHCI driver to call into AML methods for LED blink. Given some of these causes I think having a platform driver is much cleaner. I can drop DT binding part from this driver and keep just ACPI binding.
Thoughts ?
-Brijesh
>
> Arnd
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-13 16:55 ` Brijesh Singh
@ 2016-01-13 20:39 ` Arnd Bergmann
-1 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-13 20:39 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-arm-kernel, linux-kernel, mark.rutland, devicetree,
pawel.moll, ijc+devicetree, linux-ide, robh+dt, galak, tj
On Wednesday 13 January 2016 10:55:04 Brijesh Singh wrote:
> On 01/12/2016 08:24 AM, Arnd Bergmann wrote:
> > c) calling into the ACPI interpreter to do platform specific hacks
>
> Thanks for explaining. Now I understood your comment on AML however we need to consider the following:
>
> a) activity LED blinking routines are called very frequent (10 to 100ms based on emp->state) and executing AML method that often would introduces its own overhead.
>
> b) all BIOS vendors need to implement a new methods in their DSDT and release a new BIOS.
>
> c) other OS'es (mainly Windows) driver need to be updated.
>
>
> We don't know how many other SoC's have similar hacky implementation which can take advantage of extending generic
> AHCI driver to call into AML methods for LED blink. Given some of these causes I think having a platform driver is much cleaner. I can drop DT binding part from this driver and keep just ACPI binding.
>
> Thoughts ?
I don't care what you do with ACPI, just make sure we can use the generic
driver without any platform specific hacks for the DT case.
For ACPI, please talk to the respective maintainers about whether they want
to have a custom driver for this case, or a proper abstraction in the generic
driver.
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-13 20:39 ` Arnd Bergmann
0 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2016-01-13 20:39 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 13 January 2016 10:55:04 Brijesh Singh wrote:
> On 01/12/2016 08:24 AM, Arnd Bergmann wrote:
> > c) calling into the ACPI interpreter to do platform specific hacks
>
> Thanks for explaining. Now I understood your comment on AML however we need to consider the following:
>
> a) activity LED blinking routines are called very frequent (10 to 100ms based on emp->state) and executing AML method that often would introduces its own overhead.
>
> b) all BIOS vendors need to implement a new methods in their DSDT and release a new BIOS.
>
> c) other OS'es (mainly Windows) driver need to be updated.
>
>
> We don't know how many other SoC's have similar hacky implementation which can take advantage of extending generic
> AHCI driver to call into AML methods for LED blink. Given some of these causes I think having a platform driver is much cleaner. I can drop DT binding part from this driver and keep just ACPI binding.
>
> Thoughts ?
I don't care what you do with ACPI, just make sure we can use the generic
driver without any platform specific hacks for the DT case.
For ACPI, please talk to the respective maintainers about whether they want
to have a custom driver for this case, or a proper abstraction in the generic
driver.
Arnd
^ permalink raw reply [flat|nested] 43+ messages in thread
[parent not found: <5690367E.8060609-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-08 22:21 ` Brijesh Singh
(?)
@ 2016-01-11 15:33 ` Mark Langsdorf
-1 siblings, 0 replies; 43+ messages in thread
From: Mark Langsdorf @ 2016-01-11 15:33 UTC (permalink / raw)
To: Brijesh Singh, Arnd Bergmann
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA, pawel.moll-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
linux-ide-u79uwXL29TY76Z2rM5mHXA, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
galak-sgV2jX0FEOL9JmXXK+q4OQ, tj-DgEjT+Ai2ygdnm+yROfE0A
On 01/08/2016 04:21 PM, Brijesh Singh wrote:
> Hi,
>> We generally don't refer to register locations with properties other than
>> 'reg', so that approach would be worse. What I'd suggest you do is to
>> have the sgpio registers in a separate device node, and use the LED
>> binding to access it, see
>>
>> Documentation/devicetree/bindings/leds/common.txt
>>
>> It seems that none of the drivers/ata/ drivers use the leds interface
>> today, but that can be added to libata-*.c whenever the appropriate
>> properties are there.
>>
>
> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> to register a custom led control callback. Since Seattle platform does not support
> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
> to indicate that we can still handle the led messages by our registered callback. I see
> that sata_highbank driver is doing something similar.
The sata_highbank driver is doing it wrong and shouldn't have been
accepted in its current condition. Enclosure management really should
be a separate device. Please don't use it as an example.
--Mark Langsdorf
--
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-11 15:33 ` Mark Langsdorf
0 siblings, 0 replies; 43+ messages in thread
From: Mark Langsdorf @ 2016-01-11 15:33 UTC (permalink / raw)
To: linux-arm-kernel
On 01/08/2016 04:21 PM, Brijesh Singh wrote:
> Hi,
>> We generally don't refer to register locations with properties other than
>> 'reg', so that approach would be worse. What I'd suggest you do is to
>> have the sgpio registers in a separate device node, and use the LED
>> binding to access it, see
>>
>> Documentation/devicetree/bindings/leds/common.txt
>>
>> It seems that none of the drivers/ata/ drivers use the leds interface
>> today, but that can be added to libata-*.c whenever the appropriate
>> properties are there.
>>
>
> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> to register a custom led control callback. Since Seattle platform does not support
> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
> to indicate that we can still handle the led messages by our registered callback. I see
> that sata_highbank driver is doing something similar.
The sata_highbank driver is doing it wrong and shouldn't have been
accepted in its current condition. Enclosure management really should
be a separate device. Please don't use it as an example.
--Mark Langsdorf
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-11 15:33 ` Mark Langsdorf
0 siblings, 0 replies; 43+ messages in thread
From: Mark Langsdorf @ 2016-01-11 15:33 UTC (permalink / raw)
To: Brijesh Singh, Arnd Bergmann
Cc: linux-arm-kernel, linux-kernel, mark.rutland, devicetree,
pawel.moll, ijc+devicetree, linux-ide, robh+dt, galak, tj
On 01/08/2016 04:21 PM, Brijesh Singh wrote:
> Hi,
>> We generally don't refer to register locations with properties other than
>> 'reg', so that approach would be worse. What I'd suggest you do is to
>> have the sgpio registers in a separate device node, and use the LED
>> binding to access it, see
>>
>> Documentation/devicetree/bindings/leds/common.txt
>>
>> It seems that none of the drivers/ata/ drivers use the leds interface
>> today, but that can be added to libata-*.c whenever the appropriate
>> properties are there.
>>
>
> libata-*.c implements the "Enclosure management" style led messages but also has hooks
> to register a custom led control callback. Since Seattle platform does not support
> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
> to indicate that we can still handle the led messages by our registered callback. I see
> that sata_highbank driver is doing something similar.
The sata_highbank driver is doing it wrong and shouldn't have been
accepted in its current condition. Enclosure management really should
be a separate device. Please don't use it as an example.
--Mark Langsdorf
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-11 15:33 ` Mark Langsdorf
(?)
@ 2016-01-11 16:55 ` Brijesh Singh
-1 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-11 16:55 UTC (permalink / raw)
To: Mark Langsdorf, Arnd Bergmann
Cc: brijesh.singh, linux-arm-kernel, linux-kernel, mark.rutland,
devicetree, pawel.moll, ijc+devicetree, linux-ide, robh+dt,
galak, tj
Hi,
On 01/11/2016 09:33 AM, Mark Langsdorf wrote:
> On 01/08/2016 04:21 PM, Brijesh Singh wrote:
>>
>> libata-*.c implements the "Enclosure management" style led messages but also has hooks
>> to register a custom led control callback. Since Seattle platform does not support
>> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
>> to indicate that we can still handle the led messages by our registered callback. I see
>> that sata_highbank driver is doing something similar.
>
> The sata_highbank driver is doing it wrong and shouldn't have been
> accepted in its current condition. Enclosure management really should
> be a separate device. Please don't use it as an example.
>
Okay. Thanks for letting me know.
Should I consider extending the generic driver to handle this Seattle platform specific feature or go with a new
platform driver approach (i.e the one I am doing right now) ?
-Brijesh
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-11 16:55 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-11 16:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 01/11/2016 09:33 AM, Mark Langsdorf wrote:
> On 01/08/2016 04:21 PM, Brijesh Singh wrote:
>>
>> libata-*.c implements the "Enclosure management" style led messages but also has hooks
>> to register a custom led control callback. Since Seattle platform does not support
>> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
>> to indicate that we can still handle the led messages by our registered callback. I see
>> that sata_highbank driver is doing something similar.
>
> The sata_highbank driver is doing it wrong and shouldn't have been
> accepted in its current condition. Enclosure management really should
> be a separate device. Please don't use it as an example.
>
Okay. Thanks for letting me know.
Should I consider extending the generic driver to handle this Seattle platform specific feature or go with a new
platform driver approach (i.e the one I am doing right now) ?
-Brijesh
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-11 16:55 ` Brijesh Singh
0 siblings, 0 replies; 43+ messages in thread
From: Brijesh Singh @ 2016-01-11 16:55 UTC (permalink / raw)
To: Mark Langsdorf, Arnd Bergmann
Cc: brijesh.singh, linux-arm-kernel, linux-kernel, mark.rutland,
devicetree, pawel.moll, ijc+devicetree, linux-ide, robh+dt,
galak, tj
Hi,
On 01/11/2016 09:33 AM, Mark Langsdorf wrote:
> On 01/08/2016 04:21 PM, Brijesh Singh wrote:
>>
>> libata-*.c implements the "Enclosure management" style led messages but also has hooks
>> to register a custom led control callback. Since Seattle platform does not support
>> the "Enclosure management" registers hence ata_port_info we are setting a ATA_FLAG_EM | ATA_FLAG_SW_ACIVITY
>> to indicate that we can still handle the led messages by our registered callback. I see
>> that sata_highbank driver is doing something similar.
>
> The sata_highbank driver is doing it wrong and shouldn't have been
> accepted in its current condition. Enclosure management really should
> be a separate device. Please don't use it as an example.
>
Okay. Thanks for letting me know.
Should I consider extending the generic driver to handle this Seattle platform specific feature or go with a new
platform driver approach (i.e the one I am doing right now) ?
-Brijesh
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-07 21:25 ` Arnd Bergmann
@ 2016-01-07 22:56 ` Rob Herring
-1 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2016-01-07 22:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, Brijesh Singh, linux-kernel, mark.rutland,
devicetree, pawel.moll, ijc+devicetree, linux-ide, galak, tj
On Thu, Jan 07, 2016 at 10:25:38PM +0100, Arnd Bergmann wrote:
> On Thursday 07 January 2016 14:53:22 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.
> > +Examples:
> > + sata0@e0300000 {
> > + compatible = "amd,seattle-ahci";
> > + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
>
> Looking at the register values, I doubt that the SGPIO is actually part of the
> sata device. More likely, you are pointing in the middle of an actual
> GPIO controller.
SGPIO is really a poor name as it has little to do with GPIO. It's a
serial protocol to set LED states. Still, I agree this should probably
be a separate node with perhaps a phandle to syscon.
> If so, please implement a GPIO driver for that device, and use the gpio-leds
> driver to drive the LEDs. IIRC there is already a generic way to communicate
> with the LEDs interface from libata, if not you can implement that in order
> to keep the special case out of the platform driver.
There is kernel support for activity LEDs, but the others you want to
control with ledmon/ledctl utilities rather than LED subsystem. Those
utilities use an enclosure management sysfs file IIRC. There's no
kernel support for SGPIO outside of the AHCI enclosure management
register (at least there wasn't 2 years ago when I last looked).
Rob
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-07 22:56 ` Rob Herring
0 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2016-01-07 22:56 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jan 07, 2016 at 10:25:38PM +0100, Arnd Bergmann wrote:
> On Thursday 07 January 2016 14:53:22 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.
> > +Examples:
> > + sata0 at e0300000 {
> > + compatible = "amd,seattle-ahci";
> > + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
>
> Looking at the register values, I doubt that the SGPIO is actually part of the
> sata device. More likely, you are pointing in the middle of an actual
> GPIO controller.
SGPIO is really a poor name as it has little to do with GPIO. It's a
serial protocol to set LED states. Still, I agree this should probably
be a separate node with perhaps a phandle to syscon.
> If so, please implement a GPIO driver for that device, and use the gpio-leds
> driver to drive the LEDs. IIRC there is already a generic way to communicate
> with the LEDs interface from libata, if not you can implement that in order
> to keep the special case out of the platform driver.
There is kernel support for activity LEDs, but the others you want to
control with ledmon/ledctl utilities rather than LED subsystem. Those
utilities use an enclosure management sysfs file IIRC. There's no
kernel support for SGPIO outside of the AHCI enclosure management
register (at least there wasn't 2 years ago when I last looked).
Rob
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-07 20:53 ` Brijesh Singh
@ 2016-01-08 19:59 ` Joe Perches
-1 siblings, 0 replies; 43+ messages in thread
From: Joe Perches @ 2016-01-08 19:59 UTC (permalink / raw)
To: Brijesh Singh, linux-arm-kernel, linux-kernel
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, tj,
devicetree, linux-ide
On Thu, 2016-01-07 at 14:53 -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.
trivia:
> diff --git a/drivers/ata/ahci_seattle.c b/drivers/ata/ahci_seattle.c
[]
> +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)
> + emp = &pp->em_priv[pmp];
> + else
> + return -EINVAL;
This is generally done using a inverted test that
returns immediately
if (pmp >= EMP_MAX_SLOTS)
return -EINVAL;
emp = &pp->em_priv[pmp];
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-08 19:59 ` Joe Perches
0 siblings, 0 replies; 43+ messages in thread
From: Joe Perches @ 2016-01-08 19:59 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2016-01-07 at 14:53 -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.
trivia:
> diff --git a/drivers/ata/ahci_seattle.c b/drivers/ata/ahci_seattle.c
[]
> +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)
> + emp = &pp->em_priv[pmp];
> + else
> + return -EINVAL;
This is generally done using a inverted test that
returns immediately
if (pmp >= EMP_MAX_SLOTS)
return -EINVAL;
emp = &pp->em_priv[pmp];
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] ata: add AMD Seattle platform driver
2016-01-07 20:53 ` Brijesh Singh
@ 2016-01-11 10:23 ` Hans de Goede
-1 siblings, 0 replies; 43+ messages in thread
From: Hans de Goede @ 2016-01-11 10:23 UTC (permalink / raw)
To: Brijesh Singh, linux-arm-kernel, linux-kernel
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, tj,
devicetree, linux-ide
Hi,
On 07-01-16 21:53, 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>
This driver looks good from a libahci_platform usage pov. So with the
2 remarks from other reviewers fixed this patch is:
Acked-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> CC: robh+dt@kernel.org
> CC: pawel.moll@arm.com
> CC: mark.rutland@arm.com
> CC: ijc+devicetree@hellion.org.uk
> CC: galak@codeaurora.org
> CC: tj@kernel.org
> CC: devicetree@vger.kernel.org
> CC: linux-ide@vger.kernel.org
> ---
> .../devicetree/bindings/ata/sata-seattle.txt | 34 ++++
> drivers/ata/Kconfig | 8 +
> drivers/ata/Makefile | 1 +
> drivers/ata/ahci_seattle.c | 226 +++++++++++++++++++++
> 4 files changed, 269 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/ata/sata-seattle.txt
> create mode 100644 drivers/ata/ahci_seattle.c
>
> diff --git a/Documentation/devicetree/bindings/ata/sata-seattle.txt b/Documentation/devicetree/bindings/ata/sata-seattle.txt
> new file mode 100644
> index 0000000..5ad46b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/sata-seattle.txt
> @@ -0,0 +1,34 @@
> +* AHCI SATA Controller
> +
> +SATA nodes are defined to describe on-chip Serial ATA controllers.
> +The AMD Seattle SATA controller mostly conforms to the AHCI interface
> +with some special SGPIO register to contro activity LED interfaces.
> +
> +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.
> +
> +Each SATA controller should have its own node.
> +
> +
> +Required properties:
> +- compatible : compatible string should be "amd,seattle-ahci".
> +- interrupts : <interrupt mapping for SATA IRQ>
> +- reg : <registers mapping>
> +
> +Optional properties:
> +- dma-coherent : Present if dma operations are coherent
> +- clocks : a list of phandle + clock specifier pairs
> +- target-supply : regulator for SATA target power
> +- phys : reference to the SATA PHY node
> +- phy-names : must be "sata-phy"
> +
> +Examples:
> + sata0@e0300000 {
> + compatible = "amd,seattle-ahci";
> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
> + interrupts = <0x0 0x163 0x4>;
> + clocks = <0x2>
> + dma-coherent;
> + };
> +
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 6aaa3f8..56f764d 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..8c9ab67
> --- /dev/null
> +++ b/drivers/ata/ahci_seattle.c
> @@ -0,0 +1,226 @@
> +/*
> + * 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)
> + emp = &pp->em_priv[pmp];
> + else
> + return -EINVAL;
> +
> + 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);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id ahci_of_match[] = {
> + { .compatible = "amd,seattle-ahci", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ahci_of_match);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id ahci_acpi_match[] = {
> + { "AMDI0600", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
> +#endif
> +
> +static struct platform_driver ahci_seattle_driver = {
> + .probe = ahci_seattle_probe,
> + .remove = ata_platform_remove_one,
> + .driver = {
> + .name = DRV_NAME,
> +#ifdef CONFIG_OF
> + .of_match_table = ahci_of_match,
> +#endif
> +#ifdef CONFIG_ACPI
> + .acpi_match_table = ahci_acpi_match,
> +#endif
> + .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);
>
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ata: add AMD Seattle platform driver
@ 2016-01-11 10:23 ` Hans de Goede
0 siblings, 0 replies; 43+ messages in thread
From: Hans de Goede @ 2016-01-11 10:23 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 07-01-16 21:53, 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>
This driver looks good from a libahci_platform usage pov. So with the
2 remarks from other reviewers fixed this patch is:
Acked-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> CC: robh+dt at kernel.org
> CC: pawel.moll at arm.com
> CC: mark.rutland at arm.com
> CC: ijc+devicetree at hellion.org.uk
> CC: galak at codeaurora.org
> CC: tj at kernel.org
> CC: devicetree at vger.kernel.org
> CC: linux-ide at vger.kernel.org
> ---
> .../devicetree/bindings/ata/sata-seattle.txt | 34 ++++
> drivers/ata/Kconfig | 8 +
> drivers/ata/Makefile | 1 +
> drivers/ata/ahci_seattle.c | 226 +++++++++++++++++++++
> 4 files changed, 269 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/ata/sata-seattle.txt
> create mode 100644 drivers/ata/ahci_seattle.c
>
> diff --git a/Documentation/devicetree/bindings/ata/sata-seattle.txt b/Documentation/devicetree/bindings/ata/sata-seattle.txt
> new file mode 100644
> index 0000000..5ad46b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/sata-seattle.txt
> @@ -0,0 +1,34 @@
> +* AHCI SATA Controller
> +
> +SATA nodes are defined to describe on-chip Serial ATA controllers.
> +The AMD Seattle SATA controller mostly conforms to the AHCI interface
> +with some special SGPIO register to contro activity LED interfaces.
> +
> +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.
> +
> +Each SATA controller should have its own node.
> +
> +
> +Required properties:
> +- compatible : compatible string should be "amd,seattle-ahci".
> +- interrupts : <interrupt mapping for SATA IRQ>
> +- reg : <registers mapping>
> +
> +Optional properties:
> +- dma-coherent : Present if dma operations are coherent
> +- clocks : a list of phandle + clock specifier pairs
> +- target-supply : regulator for SATA target power
> +- phys : reference to the SATA PHY node
> +- phy-names : must be "sata-phy"
> +
> +Examples:
> + sata0 at e0300000 {
> + compatible = "amd,seattle-ahci";
> + reg = <0x0 0xe0300000 0x0 0xf0000>, <0x0 0xe0000078 0x0 0x1>;
> + interrupts = <0x0 0x163 0x4>;
> + clocks = <0x2>
> + dma-coherent;
> + };
> +
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 6aaa3f8..56f764d 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..8c9ab67
> --- /dev/null
> +++ b/drivers/ata/ahci_seattle.c
> @@ -0,0 +1,226 @@
> +/*
> + * 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)
> + emp = &pp->em_priv[pmp];
> + else
> + return -EINVAL;
> +
> + 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);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id ahci_of_match[] = {
> + { .compatible = "amd,seattle-ahci", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ahci_of_match);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id ahci_acpi_match[] = {
> + { "AMDI0600", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
> +#endif
> +
> +static struct platform_driver ahci_seattle_driver = {
> + .probe = ahci_seattle_probe,
> + .remove = ata_platform_remove_one,
> + .driver = {
> + .name = DRV_NAME,
> +#ifdef CONFIG_OF
> + .of_match_table = ahci_of_match,
> +#endif
> +#ifdef CONFIG_ACPI
> + .acpi_match_table = ahci_acpi_match,
> +#endif
> + .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);
>
^ permalink raw reply [flat|nested] 43+ messages in thread