diff mbox series

[06/10] memory: tegra: Parameterize interrupt handler

Message ID 20210420165237.3523732-7-thierry.reding@gmail.com (mailing list archive)
State New, archived
Headers show
Series memory: tegra: Driver unification | expand

Commit Message

Thierry Reding April 20, 2021, 4:52 p.m. UTC 
From: Thierry Reding <treding@nvidia.com>

Tegra20 requires a slightly different interrupt handler than Tegra30 and
later, so parameterize the handler, so that each SoC implementation can
provide its own.

While at it, also make IRQ support optional, which will help unify the
Tegra186 memory controller driver with this one.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/memory/tegra/mc.c      | 154 ++++++++-------------------------
 drivers/memory/tegra/mc.h      |   4 +
 drivers/memory/tegra/tegra20.c |  74 ++++++++++++++++
 include/soc/tegra/mc.h         |   2 +
 4 files changed, 117 insertions(+), 117 deletions(-)

Comments

Dmitry Osipenko April 21, 2021, 11:33 p.m. UTC | #1 
20.04.2021 19:52, Thierry Reding пишет:
> +const char *const status_names[32] = {
> +	[ 1] = "External interrupt",
> +	[ 6] = "EMEM address decode error",
> +	[ 7] = "GART page fault",
> +	[ 8] = "Security violation",
> +	[ 9] = "EMEM arbitration error",
> +	[10] = "Page fault",
> +	[11] = "Invalid APB ASID update",
> +	[12] = "VPR violation",
> +	[13] = "Secure carveout violation",
> +	[16] = "MTS carveout violation",
> +};
>  
> -	return IRQ_HANDLED;
> -}
> +const char *const error_names[8] = {
> +	[2] = "EMEM decode error",
> +	[3] = "TrustZone violation",
> +	[4] = "Carveout violation",
> +	[6] = "SMMU translation error",
> +};
>  

These struct names are too generic for a global names, what about
prefixing them with tegra_mc_ ?
Krzysztof Kozlowski April 26, 2021, 9:01 a.m. UTC | #2 
On 20/04/2021 18:52, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Tegra20 requires a slightly different interrupt handler than Tegra30 and
> later, so parameterize the handler, so that each SoC implementation can
> provide its own.
> 
> While at it, also make IRQ support optional, which will help unify the
> Tegra186 memory controller driver with this one.

Please split making it optional to separate patch. Looking at the code
should be possible and not affect bisectability.

Best regards,
Krzysztof
Thierry Reding April 26, 2021, 12:19 p.m. UTC | #3 
On Mon, Apr 26, 2021 at 11:01:00AM +0200, Krzysztof Kozlowski wrote:
> On 20/04/2021 18:52, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Tegra20 requires a slightly different interrupt handler than Tegra30 and
> > later, so parameterize the handler, so that each SoC implementation can
> > provide its own.
> > 
> > While at it, also make IRQ support optional, which will help unify the
> > Tegra186 memory controller driver with this one.
> 
> Please split making it optional to separate patch. Looking at the code
> should be possible and not affect bisectability.

Technically that means I'd have to add a check for valid callback,
otherwise it'd risks oopsing if for whatever reason that's passed in as
NULL. It shouldn't be a problem in practice because it'll be valid in
all cases, at least until the unification patch, so perhaps there's no
need for the check if it's made optional in a subsequent patch anyway.

Thierry
diff mbox series

Patch

diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index 36eac0a0df3c..515ffd948a37 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -492,32 +492,7 @@  int tegra30_mc_probe(struct tegra_mc *mc)
 	return 0;
 }
 
-const struct tegra_mc_ops tegra30_mc_ops = {
-	.probe = tegra30_mc_probe,
-};
-#endif
-
-static const char *const status_names[32] = {
-	[ 1] = "External interrupt",
-	[ 6] = "EMEM address decode error",
-	[ 7] = "GART page fault",
-	[ 8] = "Security violation",
-	[ 9] = "EMEM arbitration error",
-	[10] = "Page fault",
-	[11] = "Invalid APB ASID update",
-	[12] = "VPR violation",
-	[13] = "Secure carveout violation",
-	[16] = "MTS carveout violation",
-};
-
-static const char *const error_names[8] = {
-	[2] = "EMEM decode error",
-	[3] = "TrustZone violation",
-	[4] = "Carveout violation",
-	[6] = "SMMU translation error",
-};
-
-static irqreturn_t tegra_mc_irq(int irq, void *data)
+irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
 {
 	struct tegra_mc *mc = data;
 	unsigned long status;
@@ -614,78 +589,31 @@  static irqreturn_t tegra_mc_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data)
-{
-	struct tegra_mc *mc = data;
-	unsigned long status;
-	unsigned int bit;
-
-	/* mask all interrupts to avoid flooding */
-	status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
-	if (!status)
-		return IRQ_NONE;
-
-	for_each_set_bit(bit, &status, 32) {
-		const char *direction = "read", *secure = "";
-		const char *error = status_names[bit];
-		const char *client, *desc;
-		phys_addr_t addr;
-		u32 value, reg;
-		u8 id, type;
-
-		switch (BIT(bit)) {
-		case MC_INT_DECERR_EMEM:
-			reg = MC_DECERR_EMEM_OTHERS_STATUS;
-			value = mc_readl(mc, reg);
-
-			id = value & mc->soc->client_id_mask;
-			desc = error_names[2];
-
-			if (value & BIT(31))
-				direction = "write";
-			break;
-
-		case MC_INT_INVALID_GART_PAGE:
-			reg = MC_GART_ERROR_REQ;
-			value = mc_readl(mc, reg);
-
-			id = (value >> 1) & mc->soc->client_id_mask;
-			desc = error_names[2];
-
-			if (value & BIT(0))
-				direction = "write";
-			break;
-
-		case MC_INT_SECURITY_VIOLATION:
-			reg = MC_SECURITY_VIOLATION_STATUS;
-			value = mc_readl(mc, reg);
-
-			id = value & mc->soc->client_id_mask;
-			type = (value & BIT(30)) ? 4 : 3;
-			desc = error_names[type];
-			secure = "secure ";
-
-			if (value & BIT(31))
-				direction = "write";
-			break;
-
-		default:
-			continue;
-		}
-
-		client = mc->soc->clients[id].name;
-		addr = mc_readl(mc, reg + sizeof(u32));
-
-		dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s)\n",
-				    client, secure, direction, &addr, error,
-				    desc);
-	}
+const struct tegra_mc_ops tegra30_mc_ops = {
+	.probe = tegra30_mc_probe,
+	.handle_irq = tegra30_mc_handle_irq,
+};
+#endif
 
-	/* clear interrupts */
-	mc_writel(mc, status, MC_INTSTATUS);
+const char *const status_names[32] = {
+	[ 1] = "External interrupt",
+	[ 6] = "EMEM address decode error",
+	[ 7] = "GART page fault",
+	[ 8] = "Security violation",
+	[ 9] = "EMEM arbitration error",
+	[10] = "Page fault",
+	[11] = "Invalid APB ASID update",
+	[12] = "VPR violation",
+	[13] = "Secure carveout violation",
+	[16] = "MTS carveout violation",
+};
 
-	return IRQ_HANDLED;
-}
+const char *const error_names[8] = {
+	[2] = "EMEM decode error",
+	[3] = "TrustZone violation",
+	[4] = "Carveout violation",
+	[6] = "SMMU translation error",
+};
 
 /*
  * Memory Controller (MC) has few Memory Clients that are issuing memory
@@ -786,7 +714,6 @@  static int tegra_mc_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	struct tegra_mc *mc;
-	void *isr;
 	u64 mask;
 	int err;
 
@@ -823,29 +750,22 @@  static int tegra_mc_probe(struct platform_device *pdev)
 			return err;
 	}
 
-#ifdef CONFIG_ARCH_TEGRA_2x_SOC
-	if (mc->soc == &tegra20_mc_soc) {
-		isr = tegra20_mc_irq;
-	} else
-#endif
-	{
-		isr = tegra_mc_irq;
-	}
-
-	mc->irq = platform_get_irq(pdev, 0);
-	if (mc->irq < 0)
-		return mc->irq;
+	if (mc->soc->ops && mc->soc->ops->handle_irq) {
+		mc->irq = platform_get_irq(pdev, 0);
+		if (mc->irq < 0)
+			return mc->irq;
 
-	WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n");
+		WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n");
 
-	mc_writel(mc, mc->soc->intmask, MC_INTMASK);
+		mc_writel(mc, mc->soc->intmask, MC_INTMASK);
 
-	err = devm_request_irq(&pdev->dev, mc->irq, isr, 0,
-			       dev_name(&pdev->dev), mc);
-	if (err < 0) {
-		dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n", mc->irq,
-			err);
-		return err;
+		err = devm_request_irq(&pdev->dev, mc->irq, mc->soc->ops->handle_irq, 0,
+				       dev_name(&pdev->dev), mc);
+		if (err < 0) {
+			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n", mc->irq,
+				err);
+			return err;
+		}
 	}
 
 	err = tegra_mc_reset_setup(mc);
diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
index 731896169cf3..5459018541e1 100644
--- a/drivers/memory/tegra/mc.h
+++ b/drivers/memory/tegra/mc.h
@@ -135,9 +135,13 @@  extern const struct tegra_mc_soc tegra210_mc_soc;
     defined(CONFIG_ARCH_TEGRA_132_SOC) || \
     defined(CONFIG_ARCH_TEGRA_210_SOC)
 int tegra30_mc_probe(struct tegra_mc *mc);
+irqreturn_t tegra30_mc_handle_irq(int irq, void *data);
 extern const struct tegra_mc_ops tegra30_mc_ops;
 #endif
 
+extern const char * const status_names[32];
+extern const char * const error_names[8];
+
 /*
  * These IDs are for internal use of Tegra ICC drivers. The ID numbers are
  * chosen such that they don't conflict with the device-tree ICC node IDs.
diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c
index 2c86c0d70d59..300f99dd0077 100644
--- a/drivers/memory/tegra/tegra20.c
+++ b/drivers/memory/tegra/tegra20.c
@@ -713,10 +713,84 @@  static int tegra20_mc_resume(struct tegra_mc *mc)
 	return 0;
 }
 
+static irqreturn_t tegra20_mc_handle_irq(int irq, void *data)
+{
+	struct tegra_mc *mc = data;
+	unsigned long status;
+	unsigned int bit;
+
+	/* mask all interrupts to avoid flooding */
+	status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
+	if (!status)
+		return IRQ_NONE;
+
+	for_each_set_bit(bit, &status, 32) {
+		const char *direction = "read", *secure = "";
+		const char *error = status_names[bit];
+		const char *client, *desc;
+		phys_addr_t addr;
+		u32 value, reg;
+		u8 id, type;
+
+		switch (BIT(bit)) {
+		case MC_INT_DECERR_EMEM:
+			reg = MC_DECERR_EMEM_OTHERS_STATUS;
+			value = mc_readl(mc, reg);
+
+			id = value & mc->soc->client_id_mask;
+			desc = error_names[2];
+
+			if (value & BIT(31))
+				direction = "write";
+			break;
+
+		case MC_INT_INVALID_GART_PAGE:
+			reg = MC_GART_ERROR_REQ;
+			value = mc_readl(mc, reg);
+
+			id = (value >> 1) & mc->soc->client_id_mask;
+			desc = error_names[2];
+
+			if (value & BIT(0))
+				direction = "write";
+			break;
+
+		case MC_INT_SECURITY_VIOLATION:
+			reg = MC_SECURITY_VIOLATION_STATUS;
+			value = mc_readl(mc, reg);
+
+			id = value & mc->soc->client_id_mask;
+			type = (value & BIT(30)) ? 4 : 3;
+			desc = error_names[type];
+			secure = "secure ";
+
+			if (value & BIT(31))
+				direction = "write";
+			break;
+
+		default:
+			continue;
+		}
+
+		client = mc->soc->clients[id].name;
+		addr = mc_readl(mc, reg + sizeof(u32));
+
+		dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s)\n",
+				    client, secure, direction, &addr, error,
+				    desc);
+	}
+
+	/* clear interrupts */
+	mc_writel(mc, status, MC_INTSTATUS);
+
+	return IRQ_HANDLED;
+}
+
 static const struct tegra_mc_ops tegra20_mc_ops = {
 	.probe = tegra20_mc_probe,
 	.suspend = tegra20_mc_suspend,
 	.resume = tegra20_mc_resume,
+	.handle_irq = tegra20_mc_handle_irq,
 };
 
 const struct tegra_mc_soc tegra20_mc_soc = {
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index fd19df3eb529..ce3cdcc7b596 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -10,6 +10,7 @@ 
 #include <linux/debugfs.h>
 #include <linux/err.h>
 #include <linux/interconnect-provider.h>
+#include <linux/irq.h>
 #include <linux/reset-controller.h>
 #include <linux/types.h>
 
@@ -173,6 +174,7 @@  struct tegra_mc_ops {
 	int (*probe)(struct tegra_mc *mc);
 	int (*suspend)(struct tegra_mc *mc);
 	int (*resume)(struct tegra_mc *mc);
+	irqreturn_t (*handle_irq)(int irq, void *data);
 };
 
 struct tegra_mc_soc {