On Fri, 22 Feb 2019 14:53:54 +0000,
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> When writing the driver, a hack was introduced to configure both SATA
> interrupts regardless of the port in use to overcome a limitation in
> the SATA core. Now that this limitation has been addressed, let's
> clean this driver section and move the hack into the (historically)
> responsible SATA driver: ahci_platform.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---
> drivers/ata/ahci_platform.c | 174 ++++++++++++++++++++++++++++++++
> drivers/irqchip/irq-mvebu-icu.c | 18 ----
> 2 files changed, 174 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index cf1e0e18a7a9..4401a137e6d5 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -17,6 +17,7 @@
> #include <linux/pm.h>
> #include <linux/device.h>
> #include <linux/of_device.h>
> +#include <linux/of_irq.h>
> #include <linux/platform_device.h>
> #include <linux/libata.h>
> #include <linux/ahci_platform.h>
> @@ -26,6 +27,9 @@
>
> #define DRV_NAME "ahci"
>
> +#define ICU_SATA0_ICU_ID 109
> +#define ICU_SATA1_ICU_ID 107
> +
> static const struct ata_port_info ahci_port_info = {
> .flags = AHCI_FLAG_COMMON,
> .pio_mask = ATA_PIO4,
> @@ -44,6 +48,170 @@ static struct scsi_host_template ahci_platform_sht = {
> AHCI_SHT(DRV_NAME),
> };
>
> +/*
> + * The Armada CP110 SATA unit (on A7k/A8k SoCs) has 2 ports, and a dedicated ICU
> + * entry per port. In the past, the AHCI SATA driver only supported one
> + * interrupt per SATA unit. To solve this, the 2 SATA wired interrupts in the
> + * South-Bridge got configured as 1 GIC interrupt in the North-Bridge,
> + * regardless of the number of SATA ports actually enabled/in use.
> + *
> + * Since then, this limitation has been addressed and the hack described
> + * above has been removed from the ICU driver. However, for compatibility
> + * it is needed to support DT with just one interrupt and no SATA ports.
> + * Instead of hacking everywhere in the ahci/libahci code, this quirk has been
> + * written to manually create the SATA port sub-nodes if they are missing,
> + * assign them the right port numbers through the "reg" properties and their
> + * respective "interrupts".
I don't think this write-up, however interesting, is useful here. That's the
kind of war story that makes perfect sense in a commit log, and not much in
the actual code. I'd suggest you keep the story short and only explain the
problem the workaround tries to paper over.
> + */
> +static int ahci_armada_8k_quirk(struct device *dev)
> +{
> + struct device_node *dn = dev->of_node;
> + struct property *interrupt = of_find_property(dn, "interrupts", NULL);
It'd make more sense if you checked the interrupt property once you actually
need it.
> + struct device_node *ports;
> + struct property *regs, *ints;
> + int rc, i;
> +
> + if (!(of_device_is_compatible(dn, "marvell,armada-8k-ahci") &&
Where is this compat string documented?
> + !of_get_child_count(dn)))
> + return 0;
> +
> + if (!of_node_get(dn))
> + return -EINVAL;
Shouldn't you do that first, before having started to mess with the node?
> +
> + /* Verify the main SATA node "interrupts" property validity */
> + if (!interrupt) {
> + rc = -EINVAL;
> + goto put_dn;
> + }
> +
> + /* Create the two sub-nodes */
> + ports = kzalloc(2 * sizeof(*ports), GFP_KERNEL);
> + if (!ports) {
> + rc = -ENOMEM;
> + goto put_dn;
> + }
> +
> + for (i = 0; i < 2; i++) {
> + of_node_set_flag(&ports[i], OF_DYNAMIC);
Do you really expect all users to now have to select OF_DYNAMIC to be able
to use their system? I'm not sure that's the right thing to do.
I'd rather see this workaround be handled at probe time, generating the
right data structures right away, rather than patching the DT from inside
the kernel to eventually generate these structures. Also, how does this work
for non-DT?
> + of_node_init(&ports[i]);
> + ports[i].parent = dn;
> + }
> +
> + ports[0].full_name = kstrdup("sata-port@0", GFP_KERNEL);
> + ports[1].full_name = kstrdup("sata-port@1", GFP_KERNEL);
> + if (!ports[0].full_name || !ports[1].full_name) {
> + rc = -ENOMEM;
> + goto free_ports_names;
> + }
> +
> + /* Create the two "reg" and "interrupts" property for the sub-nodes */
> + regs = kzalloc(2 * sizeof(*regs), GFP_KERNEL);
> + ints = kzalloc(2 * sizeof(*ints), GFP_KERNEL);
> + if (!regs || !ints) {
> + rc = -ENOMEM;
> + goto free_props;
> + }
> +
> + for (i = 0; i < 2; i++) {
> + regs[i].name = kstrdup("reg", GFP_KERNEL);
> + regs[i].length = sizeof(u32);
> + regs[i].value = kmalloc(sizeof(regs[i].length), GFP_KERNEL);
> +
> + ints[i].name = kstrdup("interrupts", GFP_KERNEL);
> + ints[i].length = interrupt->length;
> + ints[i].value = kmemdup(interrupt->value, interrupt->length,
> + GFP_KERNEL);
> +
> + if (!regs[i].name || !regs[i].value ||
> + !ints[i].name || !ints[i].value) {
> + rc = -ENOMEM;
> + goto free_props_allocs;
> + }
> + }
> +
> + /* Force the values of the ICU interrupts for both ports.
Comment formatting.
> + * In the past, "interrupts" properties had three values:
> + * 1/ ICU interrupt type, 2/ interrupt ID, 3/ interrupt type
I don't understand this comment. From what I can see in the last patch, the
interrupts property always had 2 cells.
> + * Now there are only two:
> + * 1/ interrupt ID, 2/ interrupt type
> + * In both case we want to access the penultimate.
> + */
> + for (i = 0; i < 2; i++) {
> + u32 *reg, *icu_int;
> + u8 *value;
> +
> + reg = regs[i].value;
> + *reg = cpu_to_be32(i);
> +
> + value = ints[i].value;
> + value = &value[ints[i].length - (2 * sizeof(u32))];
> + icu_int = (u32 *)value;
> + if (!i)
> + *icu_int = cpu_to_be32(ICU_SATA0_ICU_ID);
> + else
> + *icu_int = cpu_to_be32(ICU_SATA1_ICU_ID);
> + }
> +
> + for (i = 0; i < 2; i++) {
> + ports[i].properties = ®s[i];
> + regs[i].next = &ints[i];
> + ints[i].next = NULL;
> + }
> +
> + /* Create the two sub-nodes by attaching them */
> + rc = of_attach_node(&ports[0]);
> + if (rc) {
> + dev_err(dev, "Compat: cannot attach port0 (err %d)\n", rc);
> + goto free_props_allocs;
> + }
> +
> + rc = of_attach_node(&ports[1]);
> + if (rc) {
> + dev_err(dev, "Compat: cannot attach port1 (err %d)\n", rc);
> + goto detach_port0;
> + }
> +
> + /* Delete the "interrupts" property from the SATA host node */
> + rc = of_remove_property(dn, interrupt);
> + if (rc) {
> + dev_err(dev, "Compat: cannot delete SATA host interrupt\n");
> + goto detach_ports;
> + }
> +
> + of_node_put(dn);
> +
> + return 0;
> +
> +detach_ports:
> + of_detach_node(&ports[1]);
> +
> +detach_port0:
> + of_detach_node(&ports[0]);
> +
> +free_props_allocs:
> + for (i = 0; i < 2; i++) {
> + kfree(regs[i].name);
> + kfree(regs[i].value);
> + kfree(ints[i].name);
> + kfree(ints[i].value);
> + }
> +
> +free_props:
> + kfree(regs);
> + kfree(ints);
> +
> +free_ports_names:
> + for (i = 0; i < 2; i++)
> + kfree(ports[i].full_name);
> +
> + kfree(ports);
How about using devm allocations instead, since you're going to fail the
probe anyway?
> +
> +put_dn:
> + of_node_put(dn);
> +
> + return rc;
> + }
> +
> static int ahci_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -51,6 +219,12 @@ static int ahci_probe(struct platform_device *pdev)
> const struct ata_port_info *port;
> int rc;
>
> + rc = ahci_armada_8k_quirk(dev);
> + if (rc) {
> + dev_err(dev, "Problem with CP110 backward compatibility\n");
> + return rc;
> + }
> +
> hpriv = ahci_platform_get_resources(pdev,
> AHCI_PLATFORM_GET_RESETS);
> if (IS_ERR(hpriv))
> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> index 547045d89c4b..f3b43f63fe2e 100644
> --- a/drivers/irqchip/irq-mvebu-icu.c
> +++ b/drivers/irqchip/irq-mvebu-icu.c
> @@ -38,8 +38,6 @@
>
> /* ICU definitions */
> #define ICU_MAX_IRQS 207
> -#define ICU_SATA0_ICU_ID 109
> -#define ICU_SATA1_ICU_ID 107
>
> struct mvebu_icu_subset_data {
> unsigned int icu_group;
> @@ -111,22 +109,6 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
> }
>
> writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq));
> -
> - /*
> - * The SATA unit has 2 ports, and a dedicated ICU entry per
> - * port. The ahci sata driver supports only one irq interrupt
> - * per SATA unit. To solve this conflict, we configure the 2
> - * SATA wired interrupts in the south bridge into 1 GIC
> - * interrupt in the north bridge. Even if only a single port
> - * is enabled, if sata node is enabled, both interrupts are
> - * configured (regardless of which port is actually in use).
> - */
> - if (d->hwirq == ICU_SATA0_ICU_ID || d->hwirq == ICU_SATA1_ICU_ID) {
> - writel_relaxed(icu_int,
> - icu->base + ICU_INT_CFG(ICU_SATA0_ICU_ID));
> - writel_relaxed(icu_int,
> - icu->base + ICU_INT_CFG(ICU_SATA1_ICU_ID));
> - }
> }
>
> static struct irq_chip mvebu_icu_nsr_chip = {
Clearly, this is not much of an irqchip patch. I'd expect you either extract
the irqchip bit in a separate patch.
Overall, I'm not sure this patch can fly as is. The dependency on OF_DYNAMIC
is not exactly great, and the whole thing is likely to fail anyway, as you
don't even bother selecting that new dependency. I strongly suggest you
implement the quirk by generating the right structures at the right time.
Thanks,
M.
--
Jazz is not dead, it just smell funny.
