diff mbox

[06/17] irqchip/irq-mvebu-icu: switch to regmap

Message ID 20180421135537.24716-7-miquel.raynal@bootlin.com
State New, archived
Headers show

Commit Message

Miquel Raynal April 21, 2018, 1:55 p.m. UTC
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(-)

Comments

Gregory CLEMENT April 30, 2018, 12:42 p.m. UTC | #1
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
>
Thomas Petazzoni April 30, 2018, 1:53 p.m. UTC | #2
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
Thomas Petazzoni April 30, 2018, 1:58 p.m. UTC | #3
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
Miquel Raynal May 3, 2018, 3:05 p.m. UTC | #4
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 mbox

Patch

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 =