| Message ID | 20180421135537.24716-7-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Miquel, On sam., avril 21 2018, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > The ICU DT nodes have now the 'syscon' compatible, we can switch to > regmap before splitting the code to support multiple platform devices to > be probed (one for the ICU, one per interrupt group). > How do you handle the case when you receive an old dtb (without the syscon compatible) ? Gregory > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/irqchip/irq-mvebu-icu.c | 37 ++++++++++++++++++++----------------- > 1 file changed, 20 insertions(+), 17 deletions(-) > > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c > index 5af2520445c4..580586240781 100644 > --- a/drivers/irqchip/irq-mvebu-icu.c > +++ b/drivers/irqchip/irq-mvebu-icu.c > @@ -18,6 +18,8 @@ > #include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/mfd/syscon.h> > > #include <dt-bindings/interrupt-controller/mvebu-icu.h> > > @@ -40,7 +42,7 @@ > > struct mvebu_icu { > struct irq_chip irq_chip; > - void __iomem *base; > + struct regmap *regmap; > struct irq_domain *domain; > struct device *dev; > }; > @@ -69,7 +71,7 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) > icu_int = 0; > } > > - writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq)); > + regmap_write(icu->regmap, ICU_INT_CFG(d->hwirq), icu_int); > > /* > * The SATA unit has 2 ports, and a dedicated ICU entry per > @@ -81,10 +83,10 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) > * 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)); > + regmap_write(icu->regmap, ICU_INT_CFG(ICU_SATA0_ICU_ID), > + icu_int); > + regmap_write(icu->regmap, ICU_INT_CFG(ICU_SATA1_ICU_ID), > + icu_int); > } > } > > @@ -208,12 +210,13 @@ static int mvebu_icu_probe(struct platform_device *pdev) > > icu->dev = &pdev->dev; > > + icu->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, NULL); > + if (IS_ERR(icu->regmap)) > + return PTR_ERR(icu->regmap); > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - icu->base = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(icu->base)) { > - dev_err(&pdev->dev, "Failed to map icu base address.\n"); > - return PTR_ERR(icu->base); > - } > + if (!res) > + return -ENODEV; > > icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > "ICU.%x", > @@ -247,10 +250,10 @@ static int mvebu_icu_probe(struct platform_device *pdev) > return ret; > > /* Set Clear/Set ICU SPI message address in AP */ > - writel_relaxed(upper_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AH); > - writel_relaxed(lower_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AL); > - writel_relaxed(upper_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AH); > - writel_relaxed(lower_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AL); > + regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, upper_32_bits(setspi)); > + regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, lower_32_bits(setspi)); > + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, upper_32_bits(clrspi)); > + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, lower_32_bits(clrspi)); > > /* > * Clean all ICU interrupts with type SPI_NSR, required to > @@ -259,11 +262,11 @@ static int mvebu_icu_probe(struct platform_device *pdev) > for (i = 0 ; i < ICU_MAX_IRQS ; i++) { > u32 icu_int, icu_grp; > > - icu_int = readl(icu->base + ICU_INT_CFG(i)); > + regmap_read(icu->regmap, ICU_INT_CFG(i), &icu_int); > icu_grp = icu_int >> ICU_GROUP_SHIFT; > > if (icu_grp == ICU_GRP_NSR) > - writel_relaxed(0x0, icu->base + ICU_INT_CFG(i)); > + regmap_write(icu->regmap, ICU_INT_CFG(i), 0); > } > > icu->domain = > -- > 2.14.1 >
Hello, On Sat, 21 Apr 2018 15:55:26 +0200, Miquel Raynal wrote: > The ICU DT nodes have now the 'syscon' compatible, we can switch to have now -> now have > regmap before splitting the code to support multiple platform devices to > be probed (one for the ICU, one per interrupt group). > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> As I explained in the review of PATCH 03/17, I think we could simply create the regmap in the ->probe() of the parent device, instead of using the "syscon" property, which is mainly useful when there is no parent device. The rest of the conversion to regmap looks good otherwise. Best regards, Thomas
Hello, On Sat, 21 Apr 2018 15:55:26 +0200, Miquel Raynal wrote: > + icu->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, NULL); > + if (IS_ERR(icu->regmap)) > + return PTR_ERR(icu->regmap); > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - icu->base = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(icu->base)) { > - dev_err(&pdev->dev, "Failed to map icu base address.\n"); > - return PTR_ERR(icu->base); > - } > + if (!res) > + return -ENODEV; Another issue with relying on the "syscon" compatible string: your ICU driver now *requires* that the DT has the "syscon" compatible string. Otherwise, no regmap is created, and your syscon_regmap_lookup_by_phandle() call will fail. In fact, there is a very good hint that something is wrong in terms of backward compatibility: your patch arm64: dts: marvell: add syscon compatible to CP110 ICU node comes early in the series, separated from other DT changes. It should come at the end of the series, with the rest of the DT changes. By doing this, you could test your series with the driver changes, but not the DT changes, and would have realized that the driver change in this patch (PATCH 06/17) breaks DT backward compatibility. This is IMO another good reason to not use the "syscon" property, but simply have the driver initialize the regmap by itself. This will work even with old DTs, as it will become just an internal implementation detail of the driver. Best regards, Thomas
Hi Thomas, Gregory, On Mon, 30 Apr 2018 15:53:52 +0200, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Hello, > > On Sat, 21 Apr 2018 15:55:26 +0200, Miquel Raynal wrote: > > The ICU DT nodes have now the 'syscon' compatible, we can switch to > > have now -> now have > > > regmap before splitting the code to support multiple platform devices to > > be probed (one for the ICU, one per interrupt group). > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > As I explained in the review of PATCH 03/17, I think we could simply > create the regmap in the ->probe() of the parent device, instead of > using the "syscon" property, which is mainly useful when there is no > parent device. This is a much better idea than adding the 'syscon' compatible. I will work on it. > > The rest of the conversion to regmap looks good otherwise. > > Best regards, > > Thomas Thanks, Miquèl
diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c index 5af2520445c4..580586240781 100644 --- a/drivers/irqchip/irq-mvebu-icu.c +++ b/drivers/irqchip/irq-mvebu-icu.c @@ -18,6 +18,8 @@ #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/mfd/syscon.h> #include <dt-bindings/interrupt-controller/mvebu-icu.h> @@ -40,7 +42,7 @@ struct mvebu_icu { struct irq_chip irq_chip; - void __iomem *base; + struct regmap *regmap; struct irq_domain *domain; struct device *dev; }; @@ -69,7 +71,7 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) icu_int = 0; } - writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq)); + regmap_write(icu->regmap, ICU_INT_CFG(d->hwirq), icu_int); /* * The SATA unit has 2 ports, and a dedicated ICU entry per @@ -81,10 +83,10 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) * 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)); + regmap_write(icu->regmap, ICU_INT_CFG(ICU_SATA0_ICU_ID), + icu_int); + regmap_write(icu->regmap, ICU_INT_CFG(ICU_SATA1_ICU_ID), + icu_int); } } @@ -208,12 +210,13 @@ static int mvebu_icu_probe(struct platform_device *pdev) icu->dev = &pdev->dev; + icu->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, NULL); + if (IS_ERR(icu->regmap)) + return PTR_ERR(icu->regmap); + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - icu->base = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(icu->base)) { - dev_err(&pdev->dev, "Failed to map icu base address.\n"); - return PTR_ERR(icu->base); - } + if (!res) + return -ENODEV; icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ICU.%x", @@ -247,10 +250,10 @@ static int mvebu_icu_probe(struct platform_device *pdev) return ret; /* Set Clear/Set ICU SPI message address in AP */ - writel_relaxed(upper_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AH); - writel_relaxed(lower_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AL); - writel_relaxed(upper_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AH); - writel_relaxed(lower_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AL); + regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, upper_32_bits(setspi)); + regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, lower_32_bits(setspi)); + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, upper_32_bits(clrspi)); + regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, lower_32_bits(clrspi)); /* * Clean all ICU interrupts with type SPI_NSR, required to @@ -259,11 +262,11 @@ static int mvebu_icu_probe(struct platform_device *pdev) for (i = 0 ; i < ICU_MAX_IRQS ; i++) { u32 icu_int, icu_grp; - icu_int = readl(icu->base + ICU_INT_CFG(i)); + regmap_read(icu->regmap, ICU_INT_CFG(i), &icu_int); icu_grp = icu_int >> ICU_GROUP_SHIFT; if (icu_grp == ICU_GRP_NSR) - writel_relaxed(0x0, icu->base + ICU_INT_CFG(i)); + regmap_write(icu->regmap, ICU_INT_CFG(i), 0); } icu->domain =
The ICU DT nodes have now the 'syscon' compatible, we can switch to regmap before splitting the code to support multiple platform devices to be probed (one for the ICU, one per interrupt group). Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/irqchip/irq-mvebu-icu.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-)