* [PATCH v2 0/2] Memory controller hot reset
@ 2018-02-12 17:06 Dmitry Osipenko
2018-02-12 17:06 ` [PATCH v2 1/2] memory: tegra: Squash tegra20-mc into common tegra-mc driver Dmitry Osipenko
2018-02-12 17:06 ` [PATCH v2 2/2] memory: tegra: Introduce memory client hot reset API Dmitry Osipenko
0 siblings, 2 replies; 11+ messages in thread
From: Dmitry Osipenko @ 2018-02-12 17:06 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter; +Cc: Philipp Zabel, linux-tegra, linux-kernel
Hello,
Tegra's memory controller has a "memory hot reset" functionality that
blocks all memory transactions for the memory client. During reset of HW,
it could perform DMA and resetting HW during DMA could lead to a system
hang or memory corruption, so here comes the memory hot reset that blocks
all interactions of HW with memory so that it could be reset safely. Memory
hot reset is urgently required for multimedia, like GPU or video decoder,
as it is quite easy to get into trouble without a proper HW reset.
The short series introduces a custom Tegra-specific memory hot reset API.
Since first revision of this patchset haven't got any comment, I'm trying
again with a V2 that has one minor correction compared to V1, I've added
terga_mc_hotreset_assert()/deassert() functions as in V1 there was only
tegra_mc_hot_reset() and turned out it is not enough.
Please review, thanks.
Dmitry Osipenko (2):
memory: tegra: Squash tegra20-mc into common tegra-mc driver
memory: tegra: Introduce memory client hot reset API
drivers/memory/Kconfig | 10 -
drivers/memory/Makefile | 1 -
drivers/memory/tegra/Makefile | 1 +
drivers/memory/tegra/mc.c | 433 ++++++++++++++++++++++++++++++++++------
drivers/memory/tegra/mc.h | 10 +
drivers/memory/tegra/tegra114.c | 25 +++
drivers/memory/tegra/tegra124.c | 32 +++
drivers/memory/tegra/tegra20.c | 95 +++++++++
drivers/memory/tegra/tegra210.c | 27 +++
drivers/memory/tegra/tegra30.c | 25 +++
drivers/memory/tegra20-mc.c | 254 -----------------------
include/soc/tegra/mc.h | 81 +++++++-
12 files changed, 669 insertions(+), 325 deletions(-)
create mode 100644 drivers/memory/tegra/tegra20.c
delete mode 100644 drivers/memory/tegra20-mc.c
--
2.15.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] memory: tegra: Squash tegra20-mc into common tegra-mc driver
2018-02-12 17:06 [PATCH v2 0/2] Memory controller hot reset Dmitry Osipenko
@ 2018-02-12 17:06 ` Dmitry Osipenko
2018-02-13 10:30 ` Thierry Reding
2018-02-12 17:06 ` [PATCH v2 2/2] memory: tegra: Introduce memory client hot reset API Dmitry Osipenko
1 sibling, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2018-02-12 17:06 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter; +Cc: Philipp Zabel, linux-tegra, linux-kernel
Tegra30+ has some minor differences in registers / bits layout compared
to Tegra20. Let's squash Tegra20 driver into the common tegra-mc driver
to reduce code a tad, this also will be useful for the upcoming Tegra's MC
reset API.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/memory/Kconfig | 10 --
drivers/memory/Makefile | 1 -
drivers/memory/tegra/Makefile | 1 +
drivers/memory/tegra/mc.c | 184 +++++++++++++++++++----------
drivers/memory/tegra/mc.h | 10 ++
drivers/memory/tegra/tegra20.c | 72 ++++++++++++
drivers/memory/tegra20-mc.c | 254 -----------------------------------------
include/soc/tegra/mc.h | 4 +-
8 files changed, 211 insertions(+), 325 deletions(-)
create mode 100644 drivers/memory/tegra/tegra20.c
delete mode 100644 drivers/memory/tegra20-mc.c
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 19a0e83f260d..8d731d6c3e54 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -104,16 +104,6 @@ config MVEBU_DEVBUS
Armada 370 and Armada XP. This controller allows to handle flash
devices such as NOR, NAND, SRAM, and FPGA.
-config TEGRA20_MC
- bool "Tegra20 Memory Controller(MC) driver"
- default y
- depends on ARCH_TEGRA_2x_SOC
- help
- This driver is for the Memory Controller(MC) module available
- in Tegra20 SoCs, mainly for a address translation fault
- analysis, especially for IOMMU/GART(Graphics Address
- Relocation Table) module.
-
config FSL_CORENET_CF
tristate "Freescale CoreNet Error Reporting"
depends on FSL_SOC_BOOKE
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 66f55240830e..a01ab3e22f94 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -16,7 +16,6 @@ obj-$(CONFIG_OMAP_GPMC) += omap-gpmc.o
obj-$(CONFIG_FSL_CORENET_CF) += fsl-corenet-cf.o
obj-$(CONFIG_FSL_IFC) += fsl_ifc.o
obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o
-obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o
obj-$(CONFIG_JZ4780_NEMC) += jz4780-nemc.o
obj-$(CONFIG_MTK_SMI) += mtk-smi.o
obj-$(CONFIG_DA8XX_DDRCTL) += da8xx-ddrctl.o
diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
index ce87a9470034..94ab16ba075b 100644
--- a/drivers/memory/tegra/Makefile
+++ b/drivers/memory/tegra/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
tegra-mc-y := mc.o
+tegra-mc-$(CONFIG_ARCH_TEGRA_2x_SOC) += tegra20.o
tegra-mc-$(CONFIG_ARCH_TEGRA_3x_SOC) += tegra30.o
tegra-mc-$(CONFIG_ARCH_TEGRA_114_SOC) += tegra114.o
tegra-mc-$(CONFIG_ARCH_TEGRA_124_SOC) += tegra124.o
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index a4803ac192bb..187a9005351b 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -27,6 +27,7 @@
#define MC_INT_INVALID_SMMU_PAGE (1 << 10)
#define MC_INT_ARBITRATION_EMEM (1 << 9)
#define MC_INT_SECURITY_VIOLATION (1 << 8)
+#define MC_INT_INVALID_GART_PAGE (1 << 7)
#define MC_INT_DECERR_EMEM (1 << 6)
#define MC_INTMASK 0x004
@@ -53,7 +54,14 @@
#define MC_EMEM_ADR_CFG 0x54
#define MC_EMEM_ADR_CFG_EMEM_NUMDEV BIT(0)
+#define MC_GART_ERROR_REQ 0x30
+#define MC_DECERR_EMEM_OTHERS_STATUS 0x58
+#define MC_SECURITY_VIOLATION_STATUS 0x74
+
static const struct of_device_id tegra_mc_of_match[] = {
+#ifdef CONFIG_ARCH_TEGRA_2x_SOC
+ { .compatible = "nvidia,tegra20-mc", .data = &tegra20_mc_soc },
+#endif
#ifdef CONFIG_ARCH_TEGRA_3x_SOC
{ .compatible = "nvidia,tegra30-mc", .data = &tegra30_mc_soc },
#endif
@@ -79,6 +87,9 @@ static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
unsigned int i;
u32 value;
+ if (mc->soc->tegra20)
+ return 0;
+
/* compute the number of MC clock cycles per tick */
tick = mc->tick * clk_get_rate(mc->clk);
do_div(tick, NSEC_PER_SEC);
@@ -229,6 +240,7 @@ static int tegra_mc_setup_timings(struct tegra_mc *mc)
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",
@@ -257,78 +269,124 @@ static irqreturn_t tegra_mc_irq(int irq, void *data)
for_each_set_bit(bit, &status, 32) {
const char *error = status_names[bit] ?: "unknown";
- const char *client = "unknown", *desc;
- const char *direction, *secure;
+ const char *client = "unknown", *desc = "";
+ const char *direction = "read", *secure = "";
phys_addr_t addr = 0;
unsigned int i;
- char perm[7];
+ char perm[7] = { 0 };
u8 id, type;
- u32 value;
+ u32 value, reg;
- value = mc_readl(mc, MC_ERR_STATUS);
+ if (mc->soc->tegra20) {
+ switch (bit) {
+ case 6:
+ reg = MC_DECERR_EMEM_OTHERS_STATUS;
+ value = mc_readl(mc, reg);
-#ifdef CONFIG_PHYS_ADDR_T_64BIT
- if (mc->soc->num_address_bits > 32) {
- addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
- MC_ERR_STATUS_ADR_HI_MASK);
- addr <<= 32;
- }
-#endif
+ id = value & mc->soc->client_id_mask;
+ desc = error_names[2];
- if (value & MC_ERR_STATUS_RW)
- direction = "write";
- else
- direction = "read";
+ if (value & BIT(31))
+ direction = "write";
+ break;
- if (value & MC_ERR_STATUS_SECURITY)
- secure = "secure ";
- else
- secure = "";
+ case 7:
+ reg = MC_GART_ERROR_REQ;
+ value = mc_readl(mc, reg);
- id = value & mc->soc->client_id_mask;
+ id = (value >> 1) & mc->soc->client_id_mask;
+ desc = error_names[2];
- for (i = 0; i < mc->soc->num_clients; i++) {
- if (mc->soc->clients[i].id == id) {
- client = mc->soc->clients[i].name;
+ if (value & BIT(0))
+ direction = "write";
+ break;
+
+ case 8:
+ 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:
+ reg = 0;
+ direction = "";
+ id = mc->soc->num_clients;
break;
}
- }
- type = (value & MC_ERR_STATUS_TYPE_MASK) >>
- MC_ERR_STATUS_TYPE_SHIFT;
- desc = error_names[type];
+ if (id < mc->soc->num_clients)
+ client = mc->soc->clients[id].name;
- switch (value & MC_ERR_STATUS_TYPE_MASK) {
- case MC_ERR_STATUS_TYPE_INVALID_SMMU_PAGE:
- perm[0] = ' ';
- perm[1] = '[';
+ if (reg)
+ addr = mc_readl(mc, reg + sizeof(u32));
+ } else {
+ value = mc_readl(mc, MC_ERR_STATUS);
- if (value & MC_ERR_STATUS_READABLE)
- perm[2] = 'R';
- else
- perm[2] = '-';
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+ if (mc->soc->num_address_bits > 32) {
+ addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
+ MC_ERR_STATUS_ADR_HI_MASK);
+ addr <<= 32;
+ }
+#endif
+ if (value & MC_ERR_STATUS_RW)
+ direction = "write";
- if (value & MC_ERR_STATUS_WRITABLE)
- perm[3] = 'W';
- else
- perm[3] = '-';
+ if (value & MC_ERR_STATUS_SECURITY)
+ secure = "secure ";
- if (value & MC_ERR_STATUS_NONSECURE)
- perm[4] = '-';
- else
- perm[4] = 'S';
+ id = value & mc->soc->client_id_mask;
- perm[5] = ']';
- perm[6] = '\0';
- break;
+ for (i = 0; i < mc->soc->num_clients; i++) {
+ if (mc->soc->clients[i].id == id) {
+ client = mc->soc->clients[i].name;
+ break;
+ }
+ }
- default:
- perm[0] = '\0';
- break;
- }
+ type = (value & MC_ERR_STATUS_TYPE_MASK) >>
+ MC_ERR_STATUS_TYPE_SHIFT;
+ desc = error_names[type];
+
+ switch (value & MC_ERR_STATUS_TYPE_MASK) {
+ case MC_ERR_STATUS_TYPE_INVALID_SMMU_PAGE:
+ perm[0] = ' ';
+ perm[1] = '[';
+
+ if (value & MC_ERR_STATUS_READABLE)
+ perm[2] = 'R';
+ else
+ perm[2] = '-';
+
+ if (value & MC_ERR_STATUS_WRITABLE)
+ perm[3] = 'W';
+ else
+ perm[3] = '-';
- value = mc_readl(mc, MC_ERR_ADR);
- addr |= value;
+ if (value & MC_ERR_STATUS_NONSECURE)
+ perm[4] = '-';
+ else
+ perm[4] = 'S';
+
+ perm[5] = ']';
+ perm[6] = '\0';
+ break;
+
+ default:
+ perm[0] = '\0';
+ break;
+ }
+
+ value = mc_readl(mc, MC_ERR_ADR);
+ addr |= value;
+ }
dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s%s)\n",
client, secure, direction, &addr, error,
@@ -369,11 +427,18 @@ static int tegra_mc_probe(struct platform_device *pdev)
if (IS_ERR(mc->regs))
return PTR_ERR(mc->regs);
- mc->clk = devm_clk_get(&pdev->dev, "mc");
- if (IS_ERR(mc->clk)) {
- dev_err(&pdev->dev, "failed to get MC clock: %ld\n",
- PTR_ERR(mc->clk));
- return PTR_ERR(mc->clk);
+ if (mc->soc->tegra20) {
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ mc->regs2 = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(mc->regs2))
+ return PTR_ERR(mc->regs2);
+ } else {
+ mc->clk = devm_clk_get(&pdev->dev, "mc");
+ if (IS_ERR(mc->clk)) {
+ dev_err(&pdev->dev, "failed to get MC clock: %ld\n",
+ PTR_ERR(mc->clk));
+ return PTR_ERR(mc->clk);
+ }
}
err = tegra_mc_setup_latency_allowance(mc);
@@ -416,7 +481,8 @@ static int tegra_mc_probe(struct platform_device *pdev)
value = MC_INT_DECERR_MTS | MC_INT_SECERR_SEC | MC_INT_DECERR_VPR |
MC_INT_INVALID_APB_ASID_UPDATE | MC_INT_INVALID_SMMU_PAGE |
- MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM;
+ MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM |
+ MC_INT_INVALID_GART_PAGE;
mc_writel(mc, value, MC_INTMASK);
diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
index ddb16676c3af..1642fbea5ce3 100644
--- a/drivers/memory/tegra/mc.h
+++ b/drivers/memory/tegra/mc.h
@@ -16,15 +16,25 @@
static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
{
+ if (mc->soc->tegra20 && offset >= 0x24)
+ return readl(mc->regs2 + offset - 0x3c);
+
return readl(mc->regs + offset);
}
static inline void mc_writel(struct tegra_mc *mc, u32 value,
unsigned long offset)
{
+ if (mc->soc->tegra20 && offset >= 0x24)
+ return writel(value, mc->regs2 + offset - 0x3c);
+
writel(value, mc->regs + offset);
}
+#ifdef CONFIG_ARCH_TEGRA_2x_SOC
+extern const struct tegra_mc_soc tegra20_mc_soc;
+#endif
+
#ifdef CONFIG_ARCH_TEGRA_3x_SOC
extern const struct tegra_mc_soc tegra30_mc_soc;
#endif
diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c
new file mode 100644
index 000000000000..81a082bdba19
--- /dev/null
+++ b/drivers/memory/tegra/tegra20.c
@@ -0,0 +1,72 @@
+/*
+ * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "mc.h"
+
+static const struct tegra_mc_client tegra20_mc_clients[] = {
+ { .name = "display0a" },
+ { .name = "display0ab" },
+ { .name = "display0b" },
+ { .name = "display0bb" },
+ { .name = "display0c" },
+ { .name = "display0cb" },
+ { .name = "display1b" },
+ { .name = "display1bb" },
+ { .name = "eppup" },
+ { .name = "g2pr" },
+ { .name = "g2sr" },
+ { .name = "mpeunifbr" },
+ { .name = "viruv" },
+ { .name = "avpcarm7r" },
+ { .name = "displayhc" },
+ { .name = "displayhcb" },
+ { .name = "fdcdrd" },
+ { .name = "g2dr" },
+ { .name = "host1xdmar" },
+ { .name = "host1xr" },
+ { .name = "idxsrd" },
+ { .name = "mpcorer" },
+ { .name = "mpe_ipred" },
+ { .name = "mpeamemrd" },
+ { .name = "mpecsrd" },
+ { .name = "ppcsahbdmar" },
+ { .name = "ppcsahbslvr" },
+ { .name = "texsrd" },
+ { .name = "vdebsevr" },
+ { .name = "vdember" },
+ { .name = "vdemcer" },
+ { .name = "vdetper" },
+ { .name = "eppu" },
+ { .name = "eppv" },
+ { .name = "eppy" },
+ { .name = "mpeunifbw" },
+ { .name = "viwsb" },
+ { .name = "viwu" },
+ { .name = "viwv" },
+ { .name = "viwy" },
+ { .name = "g2dw" },
+ { .name = "avpcarm7w" },
+ { .name = "fdcdwr" },
+ { .name = "host1xw" },
+ { .name = "ispw" },
+ { .name = "mpcorew" },
+ { .name = "mpecswr" },
+ { .name = "ppcsahbdmaw" },
+ { .name = "ppcsahbslvw" },
+ { .name = "vdebsevw" },
+ { .name = "vdembew" },
+ { .name = "vdetpmw" },
+};
+
+const struct tegra_mc_soc tegra20_mc_soc = {
+ .clients = tegra20_mc_clients,
+ .num_clients = ARRAY_SIZE(tegra20_mc_clients),
+ .num_address_bits = 32,
+ .client_id_mask = 0x3f,
+ .tegra20 = true,
+};
diff --git a/drivers/memory/tegra20-mc.c b/drivers/memory/tegra20-mc.c
deleted file mode 100644
index cc309a05289a..000000000000
--- a/drivers/memory/tegra20-mc.c
+++ /dev/null
@@ -1,254 +0,0 @@
-/*
- * Tegra20 Memory Controller
- *
- * Copyright (c) 2012, NVIDIA CORPORATION. All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope 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.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
- */
-
-#include <linux/err.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/ratelimit.h>
-#include <linux/platform_device.h>
-#include <linux/interrupt.h>
-#include <linux/io.h>
-
-#define DRV_NAME "tegra20-mc"
-
-#define MC_INTSTATUS 0x0
-#define MC_INTMASK 0x4
-
-#define MC_INT_ERR_SHIFT 6
-#define MC_INT_ERR_MASK (0x1f << MC_INT_ERR_SHIFT)
-#define MC_INT_DECERR_EMEM BIT(MC_INT_ERR_SHIFT)
-#define MC_INT_INVALID_GART_PAGE BIT(MC_INT_ERR_SHIFT + 1)
-#define MC_INT_SECURITY_VIOLATION BIT(MC_INT_ERR_SHIFT + 2)
-#define MC_INT_ARBITRATION_EMEM BIT(MC_INT_ERR_SHIFT + 3)
-
-#define MC_GART_ERROR_REQ 0x30
-#define MC_DECERR_EMEM_OTHERS_STATUS 0x58
-#define MC_SECURITY_VIOLATION_STATUS 0x74
-
-#define SECURITY_VIOLATION_TYPE BIT(30) /* 0=TRUSTZONE, 1=CARVEOUT */
-
-#define MC_CLIENT_ID_MASK 0x3f
-
-#define NUM_MC_REG_BANKS 2
-
-struct tegra20_mc {
- void __iomem *regs[NUM_MC_REG_BANKS];
- struct device *dev;
-};
-
-static inline u32 mc_readl(struct tegra20_mc *mc, u32 offs)
-{
- u32 val = 0;
-
- if (offs < 0x24)
- val = readl(mc->regs[0] + offs);
- else if (offs < 0x400)
- val = readl(mc->regs[1] + offs - 0x3c);
-
- return val;
-}
-
-static inline void mc_writel(struct tegra20_mc *mc, u32 val, u32 offs)
-{
- if (offs < 0x24)
- writel(val, mc->regs[0] + offs);
- else if (offs < 0x400)
- writel(val, mc->regs[1] + offs - 0x3c);
-}
-
-static const char * const tegra20_mc_client[] = {
- "cbr_display0a",
- "cbr_display0ab",
- "cbr_display0b",
- "cbr_display0bb",
- "cbr_display0c",
- "cbr_display0cb",
- "cbr_display1b",
- "cbr_display1bb",
- "cbr_eppup",
- "cbr_g2pr",
- "cbr_g2sr",
- "cbr_mpeunifbr",
- "cbr_viruv",
- "csr_avpcarm7r",
- "csr_displayhc",
- "csr_displayhcb",
- "csr_fdcdrd",
- "csr_g2dr",
- "csr_host1xdmar",
- "csr_host1xr",
- "csr_idxsrd",
- "csr_mpcorer",
- "csr_mpe_ipred",
- "csr_mpeamemrd",
- "csr_mpecsrd",
- "csr_ppcsahbdmar",
- "csr_ppcsahbslvr",
- "csr_texsrd",
- "csr_vdebsevr",
- "csr_vdember",
- "csr_vdemcer",
- "csr_vdetper",
- "cbw_eppu",
- "cbw_eppv",
- "cbw_eppy",
- "cbw_mpeunifbw",
- "cbw_viwsb",
- "cbw_viwu",
- "cbw_viwv",
- "cbw_viwy",
- "ccw_g2dw",
- "csw_avpcarm7w",
- "csw_fdcdwr",
- "csw_host1xw",
- "csw_ispw",
- "csw_mpcorew",
- "csw_mpecswr",
- "csw_ppcsahbdmaw",
- "csw_ppcsahbslvw",
- "csw_vdebsevw",
- "csw_vdembew",
- "csw_vdetpmw",
-};
-
-static void tegra20_mc_decode(struct tegra20_mc *mc, int n)
-{
- u32 addr, req;
- const char *client = "Unknown";
- int idx, cid;
- const struct reg_info {
- u32 offset;
- u32 write_bit; /* 0=READ, 1=WRITE */
- int cid_shift;
- char *message;
- } reg[] = {
- {
- .offset = MC_DECERR_EMEM_OTHERS_STATUS,
- .write_bit = 31,
- .message = "MC_DECERR",
- },
- {
- .offset = MC_GART_ERROR_REQ,
- .cid_shift = 1,
- .message = "MC_GART_ERR",
-
- },
- {
- .offset = MC_SECURITY_VIOLATION_STATUS,
- .write_bit = 31,
- .message = "MC_SECURITY_ERR",
- },
- };
-
- idx = n - MC_INT_ERR_SHIFT;
- if ((idx < 0) || (idx >= ARRAY_SIZE(reg))) {
- dev_err_ratelimited(mc->dev, "Unknown interrupt status %08lx\n",
- BIT(n));
- return;
- }
-
- req = mc_readl(mc, reg[idx].offset);
- cid = (req >> reg[idx].cid_shift) & MC_CLIENT_ID_MASK;
- if (cid < ARRAY_SIZE(tegra20_mc_client))
- client = tegra20_mc_client[cid];
-
- addr = mc_readl(mc, reg[idx].offset + sizeof(u32));
-
- dev_err_ratelimited(mc->dev, "%s (0x%08x): 0x%08x %s (%s %s)\n",
- reg[idx].message, req, addr, client,
- (req & BIT(reg[idx].write_bit)) ? "write" : "read",
- (reg[idx].offset == MC_SECURITY_VIOLATION_STATUS) ?
- ((req & SECURITY_VIOLATION_TYPE) ?
- "carveout" : "trustzone") : "");
-}
-
-static const struct of_device_id tegra20_mc_of_match[] = {
- { .compatible = "nvidia,tegra20-mc", },
- {},
-};
-
-static irqreturn_t tegra20_mc_isr(int irq, void *data)
-{
- u32 stat, mask, bit;
- struct tegra20_mc *mc = data;
-
- stat = mc_readl(mc, MC_INTSTATUS);
- mask = mc_readl(mc, MC_INTMASK);
- mask &= stat;
- if (!mask)
- return IRQ_NONE;
- while ((bit = ffs(mask)) != 0) {
- tegra20_mc_decode(mc, bit - 1);
- mask &= ~BIT(bit - 1);
- }
-
- mc_writel(mc, stat, MC_INTSTATUS);
- return IRQ_HANDLED;
-}
-
-static int tegra20_mc_probe(struct platform_device *pdev)
-{
- struct resource *irq;
- struct tegra20_mc *mc;
- int i, err;
- u32 intmask;
-
- mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
- if (!mc)
- return -ENOMEM;
- mc->dev = &pdev->dev;
-
- for (i = 0; i < ARRAY_SIZE(mc->regs); i++) {
- struct resource *res;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, i);
- mc->regs[i] = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(mc->regs[i]))
- return PTR_ERR(mc->regs[i]);
- }
-
- irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!irq)
- return -ENODEV;
- err = devm_request_irq(&pdev->dev, irq->start, tegra20_mc_isr,
- IRQF_SHARED, dev_name(&pdev->dev), mc);
- if (err)
- return -ENODEV;
-
- platform_set_drvdata(pdev, mc);
-
- intmask = MC_INT_INVALID_GART_PAGE |
- MC_INT_DECERR_EMEM | MC_INT_SECURITY_VIOLATION;
- mc_writel(mc, intmask, MC_INTMASK);
- return 0;
-}
-
-static struct platform_driver tegra20_mc_driver = {
- .probe = tegra20_mc_probe,
- .driver = {
- .name = DRV_NAME,
- .of_match_table = tegra20_mc_of_match,
- },
-};
-module_platform_driver(tegra20_mc_driver);
-
-MODULE_AUTHOR("Hiroshi DOYU <hdoyu@nvidia.com>");
-MODULE_DESCRIPTION("Tegra20 MC driver");
-MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 233bae954970..6cfc1dfa3a40 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -108,12 +108,14 @@ struct tegra_mc_soc {
u8 client_id_mask;
const struct tegra_smmu_soc *smmu;
+
+ bool tegra20;
};
struct tegra_mc {
struct device *dev;
struct tegra_smmu *smmu;
- void __iomem *regs;
+ void __iomem *regs, *regs2;
struct clk *clk;
int irq;
--
2.15.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] memory: tegra: Introduce memory client hot reset API
2018-02-12 17:06 [PATCH v2 0/2] Memory controller hot reset Dmitry Osipenko
2018-02-12 17:06 ` [PATCH v2 1/2] memory: tegra: Squash tegra20-mc into common tegra-mc driver Dmitry Osipenko
@ 2018-02-12 17:06 ` Dmitry Osipenko
2018-02-13 11:24 ` Thierry Reding
1 sibling, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2018-02-12 17:06 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter; +Cc: Philipp Zabel, linux-tegra, linux-kernel
In order to reset busy HW properly, memory controller needs to be
involved, otherwise it possible to get corrupted memory if HW was reset
during DMA. Introduce memory client 'hot reset' API that will be used
for resetting busy HW. The primary users are memory clients related to
video (decoder/encoder/camera) and graphics (2d/3d).
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/memory/tegra/mc.c | 249 ++++++++++++++++++++++++++++++++++++++++
drivers/memory/tegra/tegra114.c | 25 ++++
drivers/memory/tegra/tegra124.c | 32 ++++++
drivers/memory/tegra/tegra20.c | 23 ++++
drivers/memory/tegra/tegra210.c | 27 +++++
drivers/memory/tegra/tegra30.c | 25 ++++
include/soc/tegra/mc.h | 77 +++++++++++++
7 files changed, 458 insertions(+)
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index 187a9005351b..9838f588d64d 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -7,11 +7,13 @@
*/
#include <linux/clk.h>
+#include <linux/delay.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/reset.h>
#include <linux/slab.h>
#include <linux/sort.h>
@@ -81,6 +83,172 @@ static const struct of_device_id tegra_mc_of_match[] = {
};
MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
+static int terga_mc_flush_dma(struct tegra_mc *mc, unsigned int id)
+{
+ unsigned int hw_id = mc->soc->modules[id].hw_id;
+ u32 value, reg_poll = mc->soc->reg_client_flush_status;
+ int retries = 3;
+
+ value = mc_readl(mc, mc->soc->reg_client_ctrl);
+
+ if (mc->soc->tegra20)
+ value &= ~BIT(hw_id);
+ else
+ value |= BIT(hw_id);
+
+ /* block clients DMA requests */
+ mc_writel(mc, value, mc->soc->reg_client_ctrl);
+
+ /* wait for completion of the outstanding DMA requests */
+ if (mc->soc->tegra20) {
+ while (mc_readl(mc, reg_poll + hw_id * sizeof(u32)) != 0) {
+ if (!retries--)
+ return -EBUSY;
+
+ usleep_range(1000, 2000);
+ }
+ } else {
+ while ((mc_readl(mc, reg_poll) & BIT(hw_id)) == 0) {
+ if (!retries--)
+ return -EBUSY;
+
+ usleep_range(1000, 2000);
+ }
+ }
+
+ return 0;
+}
+
+static int terga_mc_unblock_dma(struct tegra_mc *mc, unsigned int id)
+{
+ unsigned int hw_id = mc->soc->modules[id].hw_id;
+ u32 value;
+
+ value = mc_readl(mc, mc->soc->reg_client_ctrl);
+
+ if (mc->soc->tegra20)
+ value |= BIT(hw_id);
+ else
+ value &= ~BIT(hw_id);
+
+ mc_writel(mc, value, mc->soc->reg_client_ctrl);
+
+ return 0;
+}
+
+static int terga_mc_hotreset_assert(struct tegra_mc *mc, unsigned int id)
+{
+ unsigned int hw_id = mc->soc->modules[id].hw_id;
+ u32 value;
+
+ if (mc->soc->tegra20) {
+ value = mc_readl(mc, mc->soc->reg_client_hotresetn);
+
+ mc_writel(mc, value & ~BIT(hw_id),
+ mc->soc->reg_client_hotresetn);
+ }
+
+ return 0;
+}
+
+static int terga_mc_hotreset_deassert(struct tegra_mc *mc, unsigned int id)
+{
+ unsigned int hw_id = mc->soc->modules[id].hw_id;
+ u32 value;
+
+ if (mc->soc->tegra20) {
+ value = mc_readl(mc, mc->soc->reg_client_hotresetn);
+
+ mc_writel(mc, value | BIT(hw_id),
+ mc->soc->reg_client_hotresetn);
+ }
+
+ return 0;
+}
+
+static int tegra_mc_hot_reset_assert(struct tegra_mc *mc, unsigned int id,
+ struct reset_control *rst)
+{
+ int err;
+
+ /*
+ * Block clients DMA requests and wait for completion of the
+ * outstanding requests.
+ */
+ err = terga_mc_flush_dma(mc, id);
+ if (err) {
+ dev_err(mc->dev, "Failed to flush DMA: %d\n", err);
+ return err;
+ }
+
+ /* put in reset HW that corresponds to the memory client */
+ err = reset_control_assert(rst);
+ if (err) {
+ dev_err(mc->dev, "Failed to assert HW reset: %d\n", err);
+ return err;
+ }
+
+ /* clear the client requests sitting before arbitration */
+ err = terga_mc_hotreset_assert(mc, id);
+ if (err) {
+ dev_err(mc->dev, "Failed to hot reset client: %d\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static int tegra_mc_hot_reset_deassert(struct tegra_mc *mc, unsigned int id,
+ struct reset_control *rst)
+{
+ int err;
+
+ /* take out client from hot reset */
+ err = terga_mc_hotreset_deassert(mc, id);
+ if (err) {
+ dev_err(mc->dev, "Failed to deassert hot reset: %d\n", err);
+ return err;
+ }
+
+ /* take out from reset corresponding clients HW */
+ err = reset_control_deassert(rst);
+ if (err) {
+ dev_err(mc->dev, "Failed to deassert HW reset: %d\n", err);
+ return err;
+ }
+
+ /* allow new DMA requests to proceed to arbitration */
+ err = terga_mc_unblock_dma(mc, id);
+ if (err) {
+ dev_err(mc->dev, "Failed to unblock client: %d\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static int tegra_mc_hot_reset(struct tegra_mc *mc, unsigned int id,
+ struct reset_control *rst, unsigned long usecs)
+{
+ int err;
+
+ err = tegra_mc_hot_reset_assert(mc, id, rst);
+ if (err)
+ return err;
+
+ /* make sure that reset is propagated */
+ if (usecs < 15)
+ udelay(usecs);
+ else
+ usleep_range(usecs, usecs + 500);
+
+ err = tegra_mc_hot_reset_deassert(mc, id, rst);
+ if (err)
+ return err;
+
+ return 0;
+}
+
static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
{
unsigned long long tick;
@@ -416,6 +584,7 @@ static int tegra_mc_probe(struct platform_device *pdev)
return -ENOMEM;
platform_set_drvdata(pdev, mc);
+ mutex_init(&mc->lock);
mc->soc = match->data;
mc->dev = &pdev->dev;
@@ -499,6 +668,86 @@ static struct platform_driver tegra_mc_driver = {
.probe = tegra_mc_probe,
};
+static int tegra_mc_match(struct device *dev, void *data)
+{
+ return of_match_node(tegra_mc_of_match, dev->of_node) != NULL;
+}
+
+static struct tegra_mc *tegra_mc_find_device(void)
+{
+ struct device *dev;
+
+ dev = driver_find_device(&tegra_mc_driver.driver, NULL, NULL,
+ tegra_mc_match);
+ if (!dev)
+ return NULL;
+
+ return dev_get_drvdata(dev);
+}
+
+int tegra_memory_client_hot_reset(unsigned int id, struct reset_control *rst,
+ unsigned long usecs)
+{
+ struct tegra_mc *mc;
+ int ret;
+
+ mc = tegra_mc_find_device();
+ if (!mc)
+ return -ENODEV;
+
+ if (id >= mc->soc->num_modules || !mc->soc->modules[id].valid)
+ return -EINVAL;
+
+ mutex_lock(&mc->lock);
+ ret = tegra_mc_hot_reset(mc, id, rst, usecs);
+ mutex_unlock(&mc->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tegra_memory_client_hot_reset);
+
+int tegra_memory_client_hot_reset_assert(unsigned int id,
+ struct reset_control *rst)
+{
+ struct tegra_mc *mc;
+ int ret;
+
+ mc = tegra_mc_find_device();
+ if (!mc)
+ return -ENODEV;
+
+ if (id >= mc->soc->num_modules || !mc->soc->modules[id].valid)
+ return -EINVAL;
+
+ mutex_lock(&mc->lock);
+ ret = tegra_mc_hot_reset_assert(mc, id, rst);
+ mutex_unlock(&mc->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tegra_memory_client_hot_reset_assert);
+
+int tegra_memory_client_hot_reset_deassert(unsigned int id,
+ struct reset_control *rst)
+{
+ struct tegra_mc *mc;
+ int ret;
+
+ mc = tegra_mc_find_device();
+ if (!mc)
+ return -ENODEV;
+
+ if (id >= mc->soc->num_modules || !mc->soc->modules[id].valid)
+ return -EINVAL;
+
+ mutex_lock(&mc->lock);
+ ret = tegra_mc_hot_reset_deassert(mc, id, rst);
+ mutex_unlock(&mc->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tegra_memory_client_hot_reset_deassert);
+
static int tegra_mc_init(void)
{
return platform_driver_register(&tegra_mc_driver);
diff --git a/drivers/memory/tegra/tegra114.c b/drivers/memory/tegra/tegra114.c
index b20e6e3e208e..d8ad269b6ff5 100644
--- a/drivers/memory/tegra/tegra114.c
+++ b/drivers/memory/tegra/tegra114.c
@@ -938,6 +938,27 @@ static const struct tegra_smmu_soc tegra114_smmu_soc = {
.num_asids = 4,
};
+static const struct tegra_mc_module tegra114_mc_modules[] = {
+ [TEGRA_MEMORY_CLIENT_AFI] = { .hw_id = 0, .valid = true },
+ [TEGRA_MEMORY_CLIENT_AVP] = { .hw_id = 1, .valid = true },
+ [TEGRA_MEMORY_CLIENT_DC] = { .hw_id = 2, .valid = true },
+ [TEGRA_MEMORY_CLIENT_DCB] = { .hw_id = 3, .valid = true },
+ [TEGRA_MEMORY_CLIENT_EPP] = { .hw_id = 4, .valid = true },
+ [TEGRA_MEMORY_CLIENT_2D] = { .hw_id = 5, .valid = true },
+ [TEGRA_MEMORY_CLIENT_HOST1X] = { .hw_id = 6, .valid = true },
+ [TEGRA_MEMORY_CLIENT_HDA] = { .hw_id = 7, .valid = true },
+ [TEGRA_MEMORY_CLIENT_ISP] = { .hw_id = 8, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPCORE] = { .hw_id = 9, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPCORELP] = { .hw_id = 10, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPE] = { .hw_id = 11, .valid = true },
+ [TEGRA_MEMORY_CLIENT_3D0] = { .hw_id = 12, .valid = true },
+ [TEGRA_MEMORY_CLIENT_3D1] = { .hw_id = 13, .valid = true },
+ [TEGRA_MEMORY_CLIENT_PPCS] = { .hw_id = 14, .valid = true },
+ [TEGRA_MEMORY_CLIENT_SATA] = { .hw_id = 15, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VDE] = { .hw_id = 16, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VI] = { .hw_id = 17, .valid = true },
+};
+
const struct tegra_mc_soc tegra114_mc_soc = {
.clients = tegra114_mc_clients,
.num_clients = ARRAY_SIZE(tegra114_mc_clients),
@@ -945,4 +966,8 @@ const struct tegra_mc_soc tegra114_mc_soc = {
.atom_size = 32,
.client_id_mask = 0x7f,
.smmu = &tegra114_smmu_soc,
+ .modules = tegra114_mc_modules,
+ .num_modules = ARRAY_SIZE(tegra114_mc_modules),
+ .reg_client_ctrl = 0x200,
+ .reg_client_flush_status = 0x204,
};
diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
index 8b6360eabb8a..135012c74358 100644
--- a/drivers/memory/tegra/tegra124.c
+++ b/drivers/memory/tegra/tegra124.c
@@ -1012,6 +1012,30 @@ static const struct tegra_smmu_group_soc tegra124_groups[] = {
},
};
+static const struct tegra_mc_module tegra124_mc_modules[] = {
+ [TEGRA_MEMORY_CLIENT_AFI] = { .hw_id = 0, .valid = true },
+ [TEGRA_MEMORY_CLIENT_AVP] = { .hw_id = 1, .valid = true },
+ [TEGRA_MEMORY_CLIENT_DC] = { .hw_id = 2, .valid = true },
+ [TEGRA_MEMORY_CLIENT_DCB] = { .hw_id = 3, .valid = true },
+ [TEGRA_MEMORY_CLIENT_HOST1X] = { .hw_id = 6, .valid = true },
+ [TEGRA_MEMORY_CLIENT_HDA] = { .hw_id = 7, .valid = true },
+ [TEGRA_MEMORY_CLIENT_ISP2] = { .hw_id = 8, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPCORE] = { .hw_id = 9, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPCORELP] = { .hw_id = 10, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MSENC] = { .hw_id = 11, .valid = true },
+ [TEGRA_MEMORY_CLIENT_PPCS] = { .hw_id = 14, .valid = true },
+ [TEGRA_MEMORY_CLIENT_SATA] = { .hw_id = 15, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VDE] = { .hw_id = 16, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VI] = { .hw_id = 17, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VIC] = { .hw_id = 18, .valid = true },
+ [TEGRA_MEMORY_CLIENT_XUSB_HOST] = { .hw_id = 19, .valid = true },
+ [TEGRA_MEMORY_CLIENT_XUSB_DEV] = { .hw_id = 20, .valid = true },
+ [TEGRA_MEMORY_CLIENT_TSEC] = { .hw_id = 22, .valid = true },
+ [TEGRA_MEMORY_CLIENT_SDMMC1] = { .hw_id = 29, .valid = true },
+ [TEGRA_MEMORY_CLIENT_SDMMC2] = { .hw_id = 30, .valid = true },
+ [TEGRA_MEMORY_CLIENT_SDMMC3] = { .hw_id = 31, .valid = true },
+};
+
#ifdef CONFIG_ARCH_TEGRA_124_SOC
static const struct tegra_smmu_soc tegra124_smmu_soc = {
.clients = tegra124_mc_clients,
@@ -1035,6 +1059,10 @@ const struct tegra_mc_soc tegra124_mc_soc = {
.smmu = &tegra124_smmu_soc,
.emem_regs = tegra124_mc_emem_regs,
.num_emem_regs = ARRAY_SIZE(tegra124_mc_emem_regs),
+ .modules = tegra124_mc_modules,
+ .num_modules = ARRAY_SIZE(tegra124_mc_modules),
+ .reg_client_ctrl = 0x200,
+ .reg_client_flush_status = 0x204,
};
#endif /* CONFIG_ARCH_TEGRA_124_SOC */
@@ -1059,5 +1087,9 @@ const struct tegra_mc_soc tegra132_mc_soc = {
.atom_size = 32,
.client_id_mask = 0x7f,
.smmu = &tegra132_smmu_soc,
+ .modules = tegra124_mc_modules,
+ .num_modules = ARRAY_SIZE(tegra124_mc_modules),
+ .reg_client_ctrl = 0x200,
+ .reg_client_flush_status = 0x204,
};
#endif /* CONFIG_ARCH_TEGRA_132_SOC */
diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c
index 81a082bdba19..4825013b948a 100644
--- a/drivers/memory/tegra/tegra20.c
+++ b/drivers/memory/tegra/tegra20.c
@@ -63,10 +63,33 @@ static const struct tegra_mc_client tegra20_mc_clients[] = {
{ .name = "vdetpmw" },
};
+static const struct tegra_mc_module tegra20_mc_modules[] = {
+ [TEGRA_MEMORY_CLIENT_AVP] = { .hw_id = 0, .valid = true },
+ [TEGRA_MEMORY_CLIENT_DC] = { .hw_id = 1, .valid = true },
+ [TEGRA_MEMORY_CLIENT_DCB] = { .hw_id = 2, .valid = true },
+ [TEGRA_MEMORY_CLIENT_EPP] = { .hw_id = 3, .valid = true },
+ [TEGRA_MEMORY_CLIENT_2D] = { .hw_id = 4, .valid = true },
+ [TEGRA_MEMORY_CLIENT_HOST1X] = { .hw_id = 5, .valid = true },
+ [TEGRA_MEMORY_CLIENT_ISP] = { .hw_id = 6, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPCORE] = { .hw_id = 7, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPEA] = { .hw_id = 8, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPEB] = { .hw_id = 9, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPEC] = { .hw_id = 10, .valid = true },
+ [TEGRA_MEMORY_CLIENT_3D] = { .hw_id = 11, .valid = true },
+ [TEGRA_MEMORY_CLIENT_PPCS] = { .hw_id = 12, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VDE] = { .hw_id = 13, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VI] = { .hw_id = 14, .valid = true },
+};
+
const struct tegra_mc_soc tegra20_mc_soc = {
.clients = tegra20_mc_clients,
.num_clients = ARRAY_SIZE(tegra20_mc_clients),
.num_address_bits = 32,
.client_id_mask = 0x3f,
.tegra20 = true,
+ .modules = tegra20_mc_modules,
+ .num_modules = ARRAY_SIZE(tegra20_mc_modules),
+ .reg_client_ctrl = 0x100,
+ .reg_client_hotresetn = 0x104,
+ .reg_client_flush_status = 0x140,
};
diff --git a/drivers/memory/tegra/tegra210.c b/drivers/memory/tegra/tegra210.c
index d398bcd3fc57..dfae0e72b632 100644
--- a/drivers/memory/tegra/tegra210.c
+++ b/drivers/memory/tegra/tegra210.c
@@ -1072,6 +1072,29 @@ static const struct tegra_smmu_group_soc tegra210_groups[] = {
},
};
+static const struct tegra_mc_module tegra210_mc_modules[] = {
+ [TEGRA_MEMORY_CLIENT_AFI] = { .hw_id = 0, .valid = true },
+ [TEGRA_MEMORY_CLIENT_AVP] = { .hw_id = 1, .valid = true },
+ [TEGRA_MEMORY_CLIENT_DC] = { .hw_id = 2, .valid = true },
+ [TEGRA_MEMORY_CLIENT_DCB] = { .hw_id = 3, .valid = true },
+ [TEGRA_MEMORY_CLIENT_HOST1X] = { .hw_id = 6, .valid = true },
+ [TEGRA_MEMORY_CLIENT_HDA] = { .hw_id = 7, .valid = true },
+ [TEGRA_MEMORY_CLIENT_ISP2] = { .hw_id = 8, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPCORE] = { .hw_id = 9, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPCORELP] = { .hw_id = 10, .valid = true },
+ [TEGRA_MEMORY_CLIENT_NVENC] = { .hw_id = 11, .valid = true },
+ [TEGRA_MEMORY_CLIENT_PPCS] = { .hw_id = 14, .valid = true },
+ [TEGRA_MEMORY_CLIENT_SATA] = { .hw_id = 15, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VI] = { .hw_id = 17, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VIC] = { .hw_id = 18, .valid = true },
+ [TEGRA_MEMORY_CLIENT_XUSB_HOST] = { .hw_id = 19, .valid = true },
+ [TEGRA_MEMORY_CLIENT_XUSB_DEV] = { .hw_id = 20, .valid = true },
+ [TEGRA_MEMORY_CLIENT_TSEC] = { .hw_id = 22, .valid = true },
+ [TEGRA_MEMORY_CLIENT_SDMMC1] = { .hw_id = 29, .valid = true },
+ [TEGRA_MEMORY_CLIENT_SDMMC2] = { .hw_id = 30, .valid = true },
+ [TEGRA_MEMORY_CLIENT_SDMMC3] = { .hw_id = 31, .valid = true },
+};
+
static const struct tegra_smmu_soc tegra210_smmu_soc = {
.clients = tegra210_mc_clients,
.num_clients = ARRAY_SIZE(tegra210_mc_clients),
@@ -1092,4 +1115,8 @@ const struct tegra_mc_soc tegra210_mc_soc = {
.atom_size = 64,
.client_id_mask = 0xff,
.smmu = &tegra210_smmu_soc,
+ .modules = tegra210_mc_modules,
+ .num_modules = ARRAY_SIZE(tegra210_mc_modules),
+ .reg_client_ctrl = 0x200,
+ .reg_client_flush_status = 0x204,
};
diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
index d756c837f23e..10a90ae91e31 100644
--- a/drivers/memory/tegra/tegra30.c
+++ b/drivers/memory/tegra/tegra30.c
@@ -960,6 +960,27 @@ static const struct tegra_smmu_soc tegra30_smmu_soc = {
.num_asids = 4,
};
+static const struct tegra_mc_module tegra30_mc_modules[] = {
+ [TEGRA_MEMORY_CLIENT_AFI] = { .hw_id = 0, .valid = true },
+ [TEGRA_MEMORY_CLIENT_AVP] = { .hw_id = 1, .valid = true },
+ [TEGRA_MEMORY_CLIENT_DC] = { .hw_id = 2, .valid = true },
+ [TEGRA_MEMORY_CLIENT_DCB] = { .hw_id = 3, .valid = true },
+ [TEGRA_MEMORY_CLIENT_EPP] = { .hw_id = 4, .valid = true },
+ [TEGRA_MEMORY_CLIENT_2D] = { .hw_id = 5, .valid = true },
+ [TEGRA_MEMORY_CLIENT_HOST1X] = { .hw_id = 6, .valid = true },
+ [TEGRA_MEMORY_CLIENT_HDA] = { .hw_id = 7, .valid = true },
+ [TEGRA_MEMORY_CLIENT_ISP] = { .hw_id = 8, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPCORE] = { .hw_id = 9, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPCORELP] = { .hw_id = 10, .valid = true },
+ [TEGRA_MEMORY_CLIENT_MPE] = { .hw_id = 11, .valid = true },
+ [TEGRA_MEMORY_CLIENT_3D0] = { .hw_id = 12, .valid = true },
+ [TEGRA_MEMORY_CLIENT_3D1] = { .hw_id = 13, .valid = true },
+ [TEGRA_MEMORY_CLIENT_PPCS] = { .hw_id = 14, .valid = true },
+ [TEGRA_MEMORY_CLIENT_SATA] = { .hw_id = 15, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VDE] = { .hw_id = 16, .valid = true },
+ [TEGRA_MEMORY_CLIENT_VI] = { .hw_id = 17, .valid = true },
+};
+
const struct tegra_mc_soc tegra30_mc_soc = {
.clients = tegra30_mc_clients,
.num_clients = ARRAY_SIZE(tegra30_mc_clients),
@@ -967,4 +988,8 @@ const struct tegra_mc_soc tegra30_mc_soc = {
.atom_size = 16,
.client_id_mask = 0x7f,
.smmu = &tegra30_smmu_soc,
+ .modules = tegra30_mc_modules,
+ .num_modules = ARRAY_SIZE(tegra30_mc_modules),
+ .reg_client_ctrl = 0x200,
+ .reg_client_flush_status = 0x204,
};
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 6cfc1dfa3a40..2d36db3ac659 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -9,11 +9,13 @@
#ifndef __SOC_TEGRA_MC_H__
#define __SOC_TEGRA_MC_H__
+#include <linux/mutex.h>
#include <linux/types.h>
struct clk;
struct device;
struct page;
+struct reset_control;
struct tegra_smmu_enable {
unsigned int reg;
@@ -95,6 +97,11 @@ static inline void tegra_smmu_remove(struct tegra_smmu *smmu)
}
#endif
+struct tegra_mc_module {
+ unsigned int hw_id;
+ bool valid;
+};
+
struct tegra_mc_soc {
const struct tegra_mc_client *clients;
unsigned int num_clients;
@@ -110,6 +117,13 @@ struct tegra_mc_soc {
const struct tegra_smmu_soc *smmu;
bool tegra20;
+
+ const struct tegra_mc_module *modules;
+ unsigned int num_modules;
+
+ u32 reg_client_ctrl;
+ u32 reg_client_hotresetn;
+ u32 reg_client_flush_status;
};
struct tegra_mc {
@@ -124,9 +138,72 @@ struct tegra_mc {
struct tegra_mc_timing *timings;
unsigned int num_timings;
+
+ struct mutex lock;
};
void tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
+#define TEGRA_MEMORY_CLIENT_AVP 0
+#define TEGRA_MEMORY_CLIENT_DC 1
+#define TEGRA_MEMORY_CLIENT_DCB 2
+#define TEGRA_MEMORY_CLIENT_EPP 3
+#define TEGRA_MEMORY_CLIENT_2D 4
+#define TEGRA_MEMORY_CLIENT_HOST1X 5
+#define TEGRA_MEMORY_CLIENT_ISP 6
+#define TEGRA_MEMORY_CLIENT_MPCORE 7
+#define TEGRA_MEMORY_CLIENT_MPCORELP 8
+#define TEGRA_MEMORY_CLIENT_MPEA 9
+#define TEGRA_MEMORY_CLIENT_MPEB 10
+#define TEGRA_MEMORY_CLIENT_MPEC 11
+#define TEGRA_MEMORY_CLIENT_3D 12
+#define TEGRA_MEMORY_CLIENT_3D1 13
+#define TEGRA_MEMORY_CLIENT_PPCS 14
+#define TEGRA_MEMORY_CLIENT_VDE 15
+#define TEGRA_MEMORY_CLIENT_VI 16
+#define TEGRA_MEMORY_CLIENT_AFI 17
+#define TEGRA_MEMORY_CLIENT_HDA 18
+#define TEGRA_MEMORY_CLIENT_SATA 19
+#define TEGRA_MEMORY_CLIENT_MSENC 20
+#define TEGRA_MEMORY_CLIENT_VIC 21
+#define TEGRA_MEMORY_CLIENT_XUSB_HOST 22
+#define TEGRA_MEMORY_CLIENT_XUSB_DEV 23
+#define TEGRA_MEMORY_CLIENT_TSEC 24
+#define TEGRA_MEMORY_CLIENT_SDMMC1 25
+#define TEGRA_MEMORY_CLIENT_SDMMC2 26
+#define TEGRA_MEMORY_CLIENT_SDMMC3 27
+#define TEGRA_MEMORY_CLIENT_MAX TEGRA_MEMORY_CLIENT_SDMMC3
+
+#define TEGRA_MEMORY_CLIENT_3D0 TEGRA_MEMORY_CLIENT_3D
+#define TEGRA_MEMORY_CLIENT_MPE TEGRA_MEMORY_CLIENT_MPEA
+#define TEGRA_MEMORY_CLIENT_NVENC TEGRA_MEMORY_CLIENT_MSENC
+#define TEGRA_MEMORY_CLIENT_ISP2 TEGRA_MEMORY_CLIENT_ISP
+
+#ifdef CONFIG_ARCH_TEGRA
+int tegra_memory_client_hot_reset(unsigned int id, struct reset_control *rst,
+ unsigned long usecs);
+int tegra_memory_client_hot_reset_assert(unsigned int id,
+ struct reset_control *rst);
+int tegra_memory_client_hot_reset_deassert(unsigned int id,
+ struct reset_control *rst);
+#else
+int tegra_memory_client_hot_reset(unsigned int id, struct reset_control *rst)
+{
+ return -ENOSYS;
+}
+
+int tegra_memory_client_hot_reset_assert(unsigned int id,
+ struct reset_control *rst)
+{
+ return -ENOSYS;
+}
+
+int tegra_memory_client_hot_reset_deassert(unsigned int id,
+ struct reset_control *rst)
+{
+ return -ENOSYS;
+}
+#endif /* CONFIG_ARCH_TEGRA */
+
#endif /* __SOC_TEGRA_MC_H__ */
--
2.15.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] memory: tegra: Squash tegra20-mc into common tegra-mc driver
2018-02-12 17:06 ` [PATCH v2 1/2] memory: tegra: Squash tegra20-mc into common tegra-mc driver Dmitry Osipenko
@ 2018-02-13 10:30 ` Thierry Reding
2018-02-14 11:15 ` Peter De Schrijver
2018-02-19 2:04 ` Dmitry Osipenko
0 siblings, 2 replies; 11+ messages in thread
From: Thierry Reding @ 2018-02-13 10:30 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Jonathan Hunter, Philipp Zabel, Peter De Schrijver, linux-tegra,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 16167 bytes --]
On Mon, Feb 12, 2018 at 08:06:30PM +0300, Dmitry Osipenko wrote:
> Tegra30+ has some minor differences in registers / bits layout compared
> to Tegra20. Let's squash Tegra20 driver into the common tegra-mc driver
> to reduce code a tad, this also will be useful for the upcoming Tegra's MC
> reset API.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/memory/Kconfig | 10 --
> drivers/memory/Makefile | 1 -
> drivers/memory/tegra/Makefile | 1 +
> drivers/memory/tegra/mc.c | 184 +++++++++++++++++++----------
> drivers/memory/tegra/mc.h | 10 ++
> drivers/memory/tegra/tegra20.c | 72 ++++++++++++
> drivers/memory/tegra20-mc.c | 254 -----------------------------------------
> include/soc/tegra/mc.h | 4 +-
> 8 files changed, 211 insertions(+), 325 deletions(-)
> create mode 100644 drivers/memory/tegra/tegra20.c
> delete mode 100644 drivers/memory/tegra20-mc.c
I generally like the idea of unifying the drivers, but I think this case
is somewhat borderline because the changes don't come naturally. That is
the parameterizations here seem overly heavy with a lot of special cases
for Tegra20. To me that indicates that Tegra20 is conceptually too much
apart from Tegra30 and later to make unification reasonable.
However, I'd still very much like to see them unified, so let's go
through the remainder in more detail.
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index 19a0e83f260d..8d731d6c3e54 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -104,16 +104,6 @@ config MVEBU_DEVBUS
> Armada 370 and Armada XP. This controller allows to handle flash
> devices such as NOR, NAND, SRAM, and FPGA.
>
> -config TEGRA20_MC
> - bool "Tegra20 Memory Controller(MC) driver"
> - default y
> - depends on ARCH_TEGRA_2x_SOC
> - help
> - This driver is for the Memory Controller(MC) module available
> - in Tegra20 SoCs, mainly for a address translation fault
> - analysis, especially for IOMMU/GART(Graphics Address
> - Relocation Table) module.
> -
> config FSL_CORENET_CF
> tristate "Freescale CoreNet Error Reporting"
> depends on FSL_SOC_BOOKE
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index 66f55240830e..a01ab3e22f94 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -16,7 +16,6 @@ obj-$(CONFIG_OMAP_GPMC) += omap-gpmc.o
> obj-$(CONFIG_FSL_CORENET_CF) += fsl-corenet-cf.o
> obj-$(CONFIG_FSL_IFC) += fsl_ifc.o
> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o
> -obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o
> obj-$(CONFIG_JZ4780_NEMC) += jz4780-nemc.o
> obj-$(CONFIG_MTK_SMI) += mtk-smi.o
> obj-$(CONFIG_DA8XX_DDRCTL) += da8xx-ddrctl.o
> diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
> index ce87a9470034..94ab16ba075b 100644
> --- a/drivers/memory/tegra/Makefile
> +++ b/drivers/memory/tegra/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> tegra-mc-y := mc.o
>
> +tegra-mc-$(CONFIG_ARCH_TEGRA_2x_SOC) += tegra20.o
> tegra-mc-$(CONFIG_ARCH_TEGRA_3x_SOC) += tegra30.o
> tegra-mc-$(CONFIG_ARCH_TEGRA_114_SOC) += tegra114.o
> tegra-mc-$(CONFIG_ARCH_TEGRA_124_SOC) += tegra124.o
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index a4803ac192bb..187a9005351b 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -27,6 +27,7 @@
> #define MC_INT_INVALID_SMMU_PAGE (1 << 10)
> #define MC_INT_ARBITRATION_EMEM (1 << 9)
> #define MC_INT_SECURITY_VIOLATION (1 << 8)
> +#define MC_INT_INVALID_GART_PAGE (1 << 7)
> #define MC_INT_DECERR_EMEM (1 << 6)
>
> #define MC_INTMASK 0x004
> @@ -53,7 +54,14 @@
> #define MC_EMEM_ADR_CFG 0x54
> #define MC_EMEM_ADR_CFG_EMEM_NUMDEV BIT(0)
>
> +#define MC_GART_ERROR_REQ 0x30
> +#define MC_DECERR_EMEM_OTHERS_STATUS 0x58
> +#define MC_SECURITY_VIOLATION_STATUS 0x74
> +
> static const struct of_device_id tegra_mc_of_match[] = {
> +#ifdef CONFIG_ARCH_TEGRA_2x_SOC
> + { .compatible = "nvidia,tegra20-mc", .data = &tegra20_mc_soc },
> +#endif
> #ifdef CONFIG_ARCH_TEGRA_3x_SOC
> { .compatible = "nvidia,tegra30-mc", .data = &tegra30_mc_soc },
> #endif
> @@ -79,6 +87,9 @@ static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
> unsigned int i;
> u32 value;
>
> + if (mc->soc->tegra20)
> + return 0;
Test for feature flags rather than chip generation. This could be
swapped for:
if (mc->soc->supports_latency_allowance)
return 0;
> +
> /* compute the number of MC clock cycles per tick */
> tick = mc->tick * clk_get_rate(mc->clk);
> do_div(tick, NSEC_PER_SEC);
> @@ -229,6 +240,7 @@ static int tegra_mc_setup_timings(struct tegra_mc *mc)
> 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",
> @@ -257,78 +269,124 @@ static irqreturn_t tegra_mc_irq(int irq, void *data)
>
> for_each_set_bit(bit, &status, 32) {
> const char *error = status_names[bit] ?: "unknown";
> - const char *client = "unknown", *desc;
> - const char *direction, *secure;
> + const char *client = "unknown", *desc = "";
> + const char *direction = "read", *secure = "";
> phys_addr_t addr = 0;
> unsigned int i;
> - char perm[7];
> + char perm[7] = { 0 };
> u8 id, type;
> - u32 value;
> + u32 value, reg;
>
> - value = mc_readl(mc, MC_ERR_STATUS);
> + if (mc->soc->tegra20) {
> + switch (bit) {
> + case 6:
Can we have symbolic names for this (and other bits)?
> + reg = MC_DECERR_EMEM_OTHERS_STATUS;
> + value = mc_readl(mc, reg);
>
> -#ifdef CONFIG_PHYS_ADDR_T_64BIT
> - if (mc->soc->num_address_bits > 32) {
> - addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
> - MC_ERR_STATUS_ADR_HI_MASK);
> - addr <<= 32;
> - }
> -#endif
> + id = value & mc->soc->client_id_mask;
> + desc = error_names[2];
>
> - if (value & MC_ERR_STATUS_RW)
> - direction = "write";
> - else
> - direction = "read";
> + if (value & BIT(31))
> + direction = "write";
> + break;
>
> - if (value & MC_ERR_STATUS_SECURITY)
> - secure = "secure ";
> - else
> - secure = "";
> + case 7:
> + reg = MC_GART_ERROR_REQ;
> + value = mc_readl(mc, reg);
>
> - id = value & mc->soc->client_id_mask;
> + id = (value >> 1) & mc->soc->client_id_mask;
> + desc = error_names[2];
>
> - for (i = 0; i < mc->soc->num_clients; i++) {
> - if (mc->soc->clients[i].id == id) {
> - client = mc->soc->clients[i].name;
> + if (value & BIT(0))
> + direction = "write";
> + break;
> +
> + case 8:
> + 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:
> + reg = 0;
> + direction = "";
This makes no sense to me. Why reset direction here if you already
explicitly set direction to "read". Why not just leave it unset until
you know exactly what it's going to be? Why do we even continue in a
case where we know nothing of the error status?
> + id = mc->soc->num_clients;
> break;
> }
> - }
>
> - type = (value & MC_ERR_STATUS_TYPE_MASK) >>
> - MC_ERR_STATUS_TYPE_SHIFT;
> - desc = error_names[type];
> + if (id < mc->soc->num_clients)
> + client = mc->soc->clients[id].name;
>
> - switch (value & MC_ERR_STATUS_TYPE_MASK) {
> - case MC_ERR_STATUS_TYPE_INVALID_SMMU_PAGE:
> - perm[0] = ' ';
> - perm[1] = '[';
> + if (reg)
> + addr = mc_readl(mc, reg + sizeof(u32));
> + } else {
> + value = mc_readl(mc, MC_ERR_STATUS);
>
> - if (value & MC_ERR_STATUS_READABLE)
> - perm[2] = 'R';
> - else
> - perm[2] = '-';
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> + if (mc->soc->num_address_bits > 32) {
> + addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
> + MC_ERR_STATUS_ADR_HI_MASK);
> + addr <<= 32;
> + }
> +#endif
> + if (value & MC_ERR_STATUS_RW)
> + direction = "write";
>
> - if (value & MC_ERR_STATUS_WRITABLE)
> - perm[3] = 'W';
> - else
> - perm[3] = '-';
> + if (value & MC_ERR_STATUS_SECURITY)
> + secure = "secure ";
>
> - if (value & MC_ERR_STATUS_NONSECURE)
> - perm[4] = '-';
> - else
> - perm[4] = 'S';
> + id = value & mc->soc->client_id_mask;
>
> - perm[5] = ']';
> - perm[6] = '\0';
> - break;
> + for (i = 0; i < mc->soc->num_clients; i++) {
> + if (mc->soc->clients[i].id == id) {
> + client = mc->soc->clients[i].name;
> + break;
> + }
> + }
>
> - default:
> - perm[0] = '\0';
> - break;
> - }
> + type = (value & MC_ERR_STATUS_TYPE_MASK) >>
> + MC_ERR_STATUS_TYPE_SHIFT;
> + desc = error_names[type];
> +
> + switch (value & MC_ERR_STATUS_TYPE_MASK) {
> + case MC_ERR_STATUS_TYPE_INVALID_SMMU_PAGE:
> + perm[0] = ' ';
> + perm[1] = '[';
> +
> + if (value & MC_ERR_STATUS_READABLE)
> + perm[2] = 'R';
> + else
> + perm[2] = '-';
> +
> + if (value & MC_ERR_STATUS_WRITABLE)
> + perm[3] = 'W';
> + else
> + perm[3] = '-';
>
> - value = mc_readl(mc, MC_ERR_ADR);
> - addr |= value;
> + if (value & MC_ERR_STATUS_NONSECURE)
> + perm[4] = '-';
> + else
> + perm[4] = 'S';
> +
> + perm[5] = ']';
> + perm[6] = '\0';
> + break;
> +
> + default:
> + perm[0] = '\0';
> + break;
> + }
> +
> + value = mc_readl(mc, MC_ERR_ADR);
> + addr |= value;
> + }
>
> dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s%s)\n",
> client, secure, direction, &addr, error,
I'd prefer if we completely separated the Tegra20 implementation of this
handler from the Tegra30+ implementation. Both don't end up sharing very
much in the end but this variant is very difficult to read, in my
opinion.
> @@ -369,11 +427,18 @@ static int tegra_mc_probe(struct platform_device *pdev)
> if (IS_ERR(mc->regs))
> return PTR_ERR(mc->regs);
>
> - mc->clk = devm_clk_get(&pdev->dev, "mc");
> - if (IS_ERR(mc->clk)) {
> - dev_err(&pdev->dev, "failed to get MC clock: %ld\n",
> - PTR_ERR(mc->clk));
> - return PTR_ERR(mc->clk);
> + if (mc->soc->tegra20) {
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + mc->regs2 = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(mc->regs2))
> + return PTR_ERR(mc->regs2);
Ugh... this is really ugly. In retrospect we really should've left the
memory-controller and iommu in the same device tree node. There's really
no reason for them to be separate, other than perhaps the Linux driver
model, which we could easily workaround by just instancing the IOMMU
device from the memory controller driver. That way we could simply share
the I/O mapping between both and avoid these games with two regions.
> + } else {
> + mc->clk = devm_clk_get(&pdev->dev, "mc");
> + if (IS_ERR(mc->clk)) {
> + dev_err(&pdev->dev, "failed to get MC clock: %ld\n",
> + PTR_ERR(mc->clk));
> + return PTR_ERR(mc->clk);
> + }
> }
It's odd that we don't have an MC clock on Tegra2. I wonder if perhaps
we just never implemented one, or it uses one which is always on by
default. Cc Peter to see if he knows.
>
> err = tegra_mc_setup_latency_allowance(mc);
> @@ -416,7 +481,8 @@ static int tegra_mc_probe(struct platform_device *pdev)
>
> value = MC_INT_DECERR_MTS | MC_INT_SECERR_SEC | MC_INT_DECERR_VPR |
> MC_INT_INVALID_APB_ASID_UPDATE | MC_INT_INVALID_SMMU_PAGE |
> - MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM;
> + MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM |
> + MC_INT_INVALID_GART_PAGE;
This should be conditionalized on a feature flag such as "has_gart". For
most generations of Tegra this would probably work, but newer versions
have become quite picky about these kinds of things, so in some cases an
access to a reserved register or field can cause an exception.
>
> mc_writel(mc, value, MC_INTMASK);
>
> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
> index ddb16676c3af..1642fbea5ce3 100644
> --- a/drivers/memory/tegra/mc.h
> +++ b/drivers/memory/tegra/mc.h
> @@ -16,15 +16,25 @@
>
> static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
> {
> + if (mc->soc->tegra20 && offset >= 0x24)
> + return readl(mc->regs2 + offset - 0x3c);
Erm... this is weird. Shouldn't the check be offset >= 0x3c? Otherwise
you could turn the offset into a very large number and access memory
outside of the mapping. At least technically.
Again, it'd be so much easier if MC and IOMMU were a single device as
they are in actual hardware. I'm sure we could argue the case that the
current DTS is buggy and that it's reasonable to break backwards-
compatibility.
> +
> return readl(mc->regs + offset);
> }
>
> static inline void mc_writel(struct tegra_mc *mc, u32 value,
> unsigned long offset)
> {
> + if (mc->soc->tegra20 && offset >= 0x24)
> + return writel(value, mc->regs2 + offset - 0x3c);
> +
> writel(value, mc->regs + offset);
> }
>
> +#ifdef CONFIG_ARCH_TEGRA_2x_SOC
> +extern const struct tegra_mc_soc tegra20_mc_soc;
> +#endif
> +
> #ifdef CONFIG_ARCH_TEGRA_3x_SOC
> extern const struct tegra_mc_soc tegra30_mc_soc;
> #endif
> diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c
> new file mode 100644
> index 000000000000..81a082bdba19
> --- /dev/null
> +++ b/drivers/memory/tegra/tegra20.c
> @@ -0,0 +1,72 @@
> +/*
> + * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include "mc.h"
> +
> +static const struct tegra_mc_client tegra20_mc_clients[] = {
> + { .name = "display0a" },
> + { .name = "display0ab" },
> + { .name = "display0b" },
> + { .name = "display0bb" },
> + { .name = "display0c" },
> + { .name = "display0cb" },
> + { .name = "display1b" },
> + { .name = "display1bb" },
> + { .name = "eppup" },
> + { .name = "g2pr" },
> + { .name = "g2sr" },
> + { .name = "mpeunifbr" },
> + { .name = "viruv" },
> + { .name = "avpcarm7r" },
> + { .name = "displayhc" },
> + { .name = "displayhcb" },
> + { .name = "fdcdrd" },
> + { .name = "g2dr" },
> + { .name = "host1xdmar" },
> + { .name = "host1xr" },
> + { .name = "idxsrd" },
> + { .name = "mpcorer" },
> + { .name = "mpe_ipred" },
> + { .name = "mpeamemrd" },
> + { .name = "mpecsrd" },
> + { .name = "ppcsahbdmar" },
> + { .name = "ppcsahbslvr" },
> + { .name = "texsrd" },
> + { .name = "vdebsevr" },
> + { .name = "vdember" },
> + { .name = "vdemcer" },
> + { .name = "vdetper" },
> + { .name = "eppu" },
> + { .name = "eppv" },
> + { .name = "eppy" },
> + { .name = "mpeunifbw" },
> + { .name = "viwsb" },
> + { .name = "viwu" },
> + { .name = "viwv" },
> + { .name = "viwy" },
> + { .name = "g2dw" },
> + { .name = "avpcarm7w" },
> + { .name = "fdcdwr" },
> + { .name = "host1xw" },
> + { .name = "ispw" },
> + { .name = "mpcorew" },
> + { .name = "mpecswr" },
> + { .name = "ppcsahbdmaw" },
> + { .name = "ppcsahbslvw" },
> + { .name = "vdebsevw" },
> + { .name = "vdembew" },
> + { .name = "vdetpmw" },
> +};
Can you please initialize the .id field for these? I know they aren't
technically necessary because the Tegra20 code doesn't actually look up
the client by ID (because the ID happens to match the array index), but
I'd like this to be consistent across all generations.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] memory: tegra: Introduce memory client hot reset API
2018-02-12 17:06 ` [PATCH v2 2/2] memory: tegra: Introduce memory client hot reset API Dmitry Osipenko
@ 2018-02-13 11:24 ` Thierry Reding
2018-02-19 12:35 ` Dmitry Osipenko
0 siblings, 1 reply; 11+ messages in thread
From: Thierry Reding @ 2018-02-13 11:24 UTC (permalink / raw)
To: Dmitry Osipenko; +Cc: Jonathan Hunter, Philipp Zabel, linux-tegra, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 17779 bytes --]
On Mon, Feb 12, 2018 at 08:06:31PM +0300, Dmitry Osipenko wrote:
> In order to reset busy HW properly, memory controller needs to be
> involved, otherwise it possible to get corrupted memory if HW was reset
> during DMA. Introduce memory client 'hot reset' API that will be used
> for resetting busy HW. The primary users are memory clients related to
> video (decoder/encoder/camera) and graphics (2d/3d).
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/memory/tegra/mc.c | 249 ++++++++++++++++++++++++++++++++++++++++
> drivers/memory/tegra/tegra114.c | 25 ++++
> drivers/memory/tegra/tegra124.c | 32 ++++++
> drivers/memory/tegra/tegra20.c | 23 ++++
> drivers/memory/tegra/tegra210.c | 27 +++++
> drivers/memory/tegra/tegra30.c | 25 ++++
> include/soc/tegra/mc.h | 77 +++++++++++++
> 7 files changed, 458 insertions(+)
As discussed on IRC, I typed up a variant of this in an attempt to fix
an unrelated bug report. The code is here:
https://github.com/thierryreding/linux/commit/e104e703cb75dfe742b2e051194a0af8ddefcd03
I think we can make this without adding a custom API. The reset control
API should work just fine. The above version doesn't take into account
some of Tegra20's quirks, but I think it should still work for Tegra20
with just slightly different implementations for ->assert() and
->deassert().
A couple of more specific comments below.
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index 187a9005351b..9838f588d64d 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -7,11 +7,13 @@
> */
>
> #include <linux/clk.h>
> +#include <linux/delay.h>
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/reset.h>
> #include <linux/slab.h>
> #include <linux/sort.h>
>
> @@ -81,6 +83,172 @@ static const struct of_device_id tegra_mc_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
>
> +static int terga_mc_flush_dma(struct tegra_mc *mc, unsigned int id)
> +{
> + unsigned int hw_id = mc->soc->modules[id].hw_id;
> + u32 value, reg_poll = mc->soc->reg_client_flush_status;
> + int retries = 3;
> +
> + value = mc_readl(mc, mc->soc->reg_client_ctrl);
> +
> + if (mc->soc->tegra20)
> + value &= ~BIT(hw_id);
> + else
> + value |= BIT(hw_id);
> +
> + /* block clients DMA requests */
> + mc_writel(mc, value, mc->soc->reg_client_ctrl);
> +
> + /* wait for completion of the outstanding DMA requests */
> + if (mc->soc->tegra20) {
> + while (mc_readl(mc, reg_poll + hw_id * sizeof(u32)) != 0) {
> + if (!retries--)
> + return -EBUSY;
> +
> + usleep_range(1000, 2000);
> + }
> + } else {
> + while ((mc_readl(mc, reg_poll) & BIT(hw_id)) == 0) {
> + if (!retries--)
> + return -EBUSY;
> +
> + usleep_range(1000, 2000);
> + }
> + }
> +
> + return 0;
> +}
I think this suffers from too much unification. The programming model is
too different to stash this into a single function implementation and as
a result this becomes very difficult to read. In my experience it's more
readable to split this into separate implementations and pass around
pointers to them.
> +
> +static int terga_mc_unblock_dma(struct tegra_mc *mc, unsigned int id)
> +{
> + unsigned int hw_id = mc->soc->modules[id].hw_id;
> + u32 value;
> +
> + value = mc_readl(mc, mc->soc->reg_client_ctrl);
> +
> + if (mc->soc->tegra20)
> + value |= BIT(hw_id);
> + else
> + value &= ~BIT(hw_id);
> +
> + mc_writel(mc, value, mc->soc->reg_client_ctrl);
> +
> + return 0;
> +}
> +
> +static int terga_mc_hotreset_assert(struct tegra_mc *mc, unsigned int id)
> +{
> + unsigned int hw_id = mc->soc->modules[id].hw_id;
> + u32 value;
> +
> + if (mc->soc->tegra20) {
> + value = mc_readl(mc, mc->soc->reg_client_hotresetn);
> +
> + mc_writel(mc, value & ~BIT(hw_id),
> + mc->soc->reg_client_hotresetn);
> + }
> +
> + return 0;
> +}
> +
> +static int terga_mc_hotreset_deassert(struct tegra_mc *mc, unsigned int id)
> +{
> + unsigned int hw_id = mc->soc->modules[id].hw_id;
> + u32 value;
> +
> + if (mc->soc->tegra20) {
> + value = mc_readl(mc, mc->soc->reg_client_hotresetn);
> +
> + mc_writel(mc, value | BIT(hw_id),
> + mc->soc->reg_client_hotresetn);
> + }
> +
> + return 0;
> +}
The same goes for these. I think we can do this much more easily by
providing reset controller API ->assert() and ->deassert()
implementations for Tegra20 and Tegra30+, and then register the reset
controller device using the ops stored in the MC SoC structure.
> +static int tegra_mc_hot_reset_assert(struct tegra_mc *mc, unsigned int id,
> + struct reset_control *rst)
> +{
> + int err;
> +
> + /*
> + * Block clients DMA requests and wait for completion of the
> + * outstanding requests.
> + */
> + err = terga_mc_flush_dma(mc, id);
> + if (err) {
> + dev_err(mc->dev, "Failed to flush DMA: %d\n", err);
> + return err;
> + }
> +
> + /* put in reset HW that corresponds to the memory client */
> + err = reset_control_assert(rst);
> + if (err) {
> + dev_err(mc->dev, "Failed to assert HW reset: %d\n", err);
> + return err;
> + }
> +
> + /* clear the client requests sitting before arbitration */
> + err = terga_mc_hotreset_assert(mc, id);
> + if (err) {
> + dev_err(mc->dev, "Failed to hot reset client: %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
This is very strictly according to the TRM, but I don't see a reason why
you couldn't stash the DMA blocking and the hot reset into a ->assert()
implementation...
> +
> +static int tegra_mc_hot_reset_deassert(struct tegra_mc *mc, unsigned int id,
> + struct reset_control *rst)
> +{
> + int err;
> +
> + /* take out client from hot reset */
> + err = terga_mc_hotreset_deassert(mc, id);
> + if (err) {
> + dev_err(mc->dev, "Failed to deassert hot reset: %d\n", err);
> + return err;
> + }
> +
> + /* take out from reset corresponding clients HW */
> + err = reset_control_deassert(rst);
> + if (err) {
> + dev_err(mc->dev, "Failed to deassert HW reset: %d\n", err);
> + return err;
> + }
> +
> + /* allow new DMA requests to proceed to arbitration */
> + err = terga_mc_unblock_dma(mc, id);
> + if (err) {
> + dev_err(mc->dev, "Failed to unblock client: %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
... and the hot reset deassertion and the DMA unblocking into a
->deassert() implementation.
I think the important part is that DMA is blocked and the requests are
cleared before the module reset, and similarily that the hot reset is
released and DMA is unblocked after the module reset.
> +
> +static int tegra_mc_hot_reset(struct tegra_mc *mc, unsigned int id,
> + struct reset_control *rst, unsigned long usecs)
> +{
> + int err;
> +
> + err = tegra_mc_hot_reset_assert(mc, id, rst);
> + if (err)
> + return err;
> +
> + /* make sure that reset is propagated */
> + if (usecs < 15)
> + udelay(usecs);
> + else
> + usleep_range(usecs, usecs + 500);
> +
> + err = tegra_mc_hot_reset_deassert(mc, id, rst);
> + if (err)
> + return err;
> +
> + return 0;
> +}
Do you really need this helper? It seems like a marginal gain in terms
of boilerplate while obviously some (or maybe even most?) drivers can't
use this because they need more explicit control over the sequence.
The only case where I could see this be useful is during some error
recovery mechanism, in which case perhaps a runtime suspend/resume might
be more useful.
> +
> static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
> {
> unsigned long long tick;
> @@ -416,6 +584,7 @@ static int tegra_mc_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> platform_set_drvdata(pdev, mc);
> + mutex_init(&mc->lock);
> mc->soc = match->data;
> mc->dev = &pdev->dev;
>
> @@ -499,6 +668,86 @@ static struct platform_driver tegra_mc_driver = {
> .probe = tegra_mc_probe,
> };
>
> +static int tegra_mc_match(struct device *dev, void *data)
> +{
> + return of_match_node(tegra_mc_of_match, dev->of_node) != NULL;
> +}
> +
> +static struct tegra_mc *tegra_mc_find_device(void)
> +{
> + struct device *dev;
> +
> + dev = driver_find_device(&tegra_mc_driver.driver, NULL, NULL,
> + tegra_mc_match);
> + if (!dev)
> + return NULL;
> +
> + return dev_get_drvdata(dev);
> +}
Another benefit of the reset controller API is that you don't have to
look up the MC device like this. Instead you can just upcast the pointer
to the reset controller device.
> +
> +int tegra_memory_client_hot_reset(unsigned int id, struct reset_control *rst,
> + unsigned long usecs)
> +{
> + struct tegra_mc *mc;
> + int ret;
> +
> + mc = tegra_mc_find_device();
> + if (!mc)
> + return -ENODEV;
> +
> + if (id >= mc->soc->num_modules || !mc->soc->modules[id].valid)
> + return -EINVAL;
One of the problems with sparse arrays is that you need to explicitly
mark each of the entries as valid. This is error prone and tedious in
my opinion.
> +
> + mutex_lock(&mc->lock);
> + ret = tegra_mc_hot_reset(mc, id, rst, usecs);
> + mutex_unlock(&mc->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tegra_memory_client_hot_reset);
> +
> +int tegra_memory_client_hot_reset_assert(unsigned int id,
> + struct reset_control *rst)
> +{
> + struct tegra_mc *mc;
> + int ret;
> +
> + mc = tegra_mc_find_device();
> + if (!mc)
> + return -ENODEV;
> +
> + if (id >= mc->soc->num_modules || !mc->soc->modules[id].valid)
> + return -EINVAL;
> +
> + mutex_lock(&mc->lock);
> + ret = tegra_mc_hot_reset_assert(mc, id, rst);
> + mutex_unlock(&mc->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tegra_memory_client_hot_reset_assert);
> +
> +int tegra_memory_client_hot_reset_deassert(unsigned int id,
> + struct reset_control *rst)
> +{
> + struct tegra_mc *mc;
> + int ret;
> +
> + mc = tegra_mc_find_device();
> + if (!mc)
> + return -ENODEV;
> +
> + if (id >= mc->soc->num_modules || !mc->soc->modules[id].valid)
> + return -EINVAL;
There's a lot of repetition in the code here. If you look at my
prototype, I think this is simpler to deal with if you get a reference
to the reset and then just use it. All of the special case handling is
done in the lookup function, and then you get back NULL or a valid
pointer that you can immediately use.
> +
> + mutex_lock(&mc->lock);
> + ret = tegra_mc_hot_reset_deassert(mc, id, rst);
> + mutex_unlock(&mc->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tegra_memory_client_hot_reset_deassert);
> +
> static int tegra_mc_init(void)
> {
> return platform_driver_register(&tegra_mc_driver);
[...]
> diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
> index 8b6360eabb8a..135012c74358 100644
> --- a/drivers/memory/tegra/tegra124.c
> +++ b/drivers/memory/tegra/tegra124.c
> @@ -1012,6 +1012,30 @@ static const struct tegra_smmu_group_soc tegra124_groups[] = {
> },
> };
>
> +static const struct tegra_mc_module tegra124_mc_modules[] = {
> + [TEGRA_MEMORY_CLIENT_AFI] = { .hw_id = 0, .valid = true },
> + [TEGRA_MEMORY_CLIENT_AVP] = { .hw_id = 1, .valid = true },
> + [TEGRA_MEMORY_CLIENT_DC] = { .hw_id = 2, .valid = true },
> + [TEGRA_MEMORY_CLIENT_DCB] = { .hw_id = 3, .valid = true },
> + [TEGRA_MEMORY_CLIENT_HOST1X] = { .hw_id = 6, .valid = true },
> + [TEGRA_MEMORY_CLIENT_HDA] = { .hw_id = 7, .valid = true },
> + [TEGRA_MEMORY_CLIENT_ISP2] = { .hw_id = 8, .valid = true },
> + [TEGRA_MEMORY_CLIENT_MPCORE] = { .hw_id = 9, .valid = true },
> + [TEGRA_MEMORY_CLIENT_MPCORELP] = { .hw_id = 10, .valid = true },
> + [TEGRA_MEMORY_CLIENT_MSENC] = { .hw_id = 11, .valid = true },
> + [TEGRA_MEMORY_CLIENT_PPCS] = { .hw_id = 14, .valid = true },
> + [TEGRA_MEMORY_CLIENT_SATA] = { .hw_id = 15, .valid = true },
> + [TEGRA_MEMORY_CLIENT_VDE] = { .hw_id = 16, .valid = true },
> + [TEGRA_MEMORY_CLIENT_VI] = { .hw_id = 17, .valid = true },
> + [TEGRA_MEMORY_CLIENT_VIC] = { .hw_id = 18, .valid = true },
> + [TEGRA_MEMORY_CLIENT_XUSB_HOST] = { .hw_id = 19, .valid = true },
> + [TEGRA_MEMORY_CLIENT_XUSB_DEV] = { .hw_id = 20, .valid = true },
> + [TEGRA_MEMORY_CLIENT_TSEC] = { .hw_id = 22, .valid = true },
> + [TEGRA_MEMORY_CLIENT_SDMMC1] = { .hw_id = 29, .valid = true },
> + [TEGRA_MEMORY_CLIENT_SDMMC2] = { .hw_id = 30, .valid = true },
> + [TEGRA_MEMORY_CLIENT_SDMMC3] = { .hw_id = 31, .valid = true },
> +};
This list is incomplete. The same is true for any later generation.
There are also quite a few holes in them. I think a better use of this
space is to make the array compact and instead have more explicit
information in the array.
> diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
> index 6cfc1dfa3a40..2d36db3ac659 100644
> --- a/include/soc/tegra/mc.h
> +++ b/include/soc/tegra/mc.h
> @@ -9,11 +9,13 @@
> #ifndef __SOC_TEGRA_MC_H__
> #define __SOC_TEGRA_MC_H__
>
> +#include <linux/mutex.h>
> #include <linux/types.h>
>
> struct clk;
> struct device;
> struct page;
> +struct reset_control;
>
> struct tegra_smmu_enable {
> unsigned int reg;
> @@ -95,6 +97,11 @@ static inline void tegra_smmu_remove(struct tegra_smmu *smmu)
> }
> #endif
>
> +struct tegra_mc_module {
> + unsigned int hw_id;
> + bool valid;
> +};
> +
> struct tegra_mc_soc {
> const struct tegra_mc_client *clients;
> unsigned int num_clients;
> @@ -110,6 +117,13 @@ struct tegra_mc_soc {
> const struct tegra_smmu_soc *smmu;
>
> bool tegra20;
> +
> + const struct tegra_mc_module *modules;
> + unsigned int num_modules;
> +
> + u32 reg_client_ctrl;
> + u32 reg_client_hotresetn;
> + u32 reg_client_flush_status;
That's not enough to cover all clients on Tegra124 and later. We need at
least two registers. I'd also prefer to move away from the assumption
that the ID is somehow linked to the bit position. The ID is in fact
completely arbitrary and in this case only chosen to match the bit
position.
This has multiple disadvantages, some of which I've already listed. One
of them is that we inevitably make arrays sparse as the SoC evolves, so
we need to work around this in code and data tables using a valid flag.
Checking for validity also becomes non-trivial and we move part of that
burden into drivers.
I think a much better and robust solution is to completely separate the
ID from the hardware registers. This is implemented in my prototype on
github. The ID is essentially only used as a way to identify the reset
via device tree. The MC driver will use the ID to look up the reset in
its per-SoC table and will only work with the reset object after that.
The object itself contains all the information needed to program the
registers.
Currently the tables in that implementation don't take into account the
per-client "outstanding requests" registers, but they could be easily
extended to do so.
> };
>
> struct tegra_mc {
> @@ -124,9 +138,72 @@ struct tegra_mc {
>
> struct tegra_mc_timing *timings;
> unsigned int num_timings;
> +
> + struct mutex lock;
> };
>
> void tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
> unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
>
> +#define TEGRA_MEMORY_CLIENT_AVP 0
> +#define TEGRA_MEMORY_CLIENT_DC 1
> +#define TEGRA_MEMORY_CLIENT_DCB 2
> +#define TEGRA_MEMORY_CLIENT_EPP 3
> +#define TEGRA_MEMORY_CLIENT_2D 4
> +#define TEGRA_MEMORY_CLIENT_HOST1X 5
> +#define TEGRA_MEMORY_CLIENT_ISP 6
> +#define TEGRA_MEMORY_CLIENT_MPCORE 7
> +#define TEGRA_MEMORY_CLIENT_MPCORELP 8
> +#define TEGRA_MEMORY_CLIENT_MPEA 9
> +#define TEGRA_MEMORY_CLIENT_MPEB 10
> +#define TEGRA_MEMORY_CLIENT_MPEC 11
> +#define TEGRA_MEMORY_CLIENT_3D 12
> +#define TEGRA_MEMORY_CLIENT_3D1 13
> +#define TEGRA_MEMORY_CLIENT_PPCS 14
> +#define TEGRA_MEMORY_CLIENT_VDE 15
> +#define TEGRA_MEMORY_CLIENT_VI 16
> +#define TEGRA_MEMORY_CLIENT_AFI 17
> +#define TEGRA_MEMORY_CLIENT_HDA 18
> +#define TEGRA_MEMORY_CLIENT_SATA 19
> +#define TEGRA_MEMORY_CLIENT_MSENC 20
> +#define TEGRA_MEMORY_CLIENT_VIC 21
> +#define TEGRA_MEMORY_CLIENT_XUSB_HOST 22
> +#define TEGRA_MEMORY_CLIENT_XUSB_DEV 23
> +#define TEGRA_MEMORY_CLIENT_TSEC 24
> +#define TEGRA_MEMORY_CLIENT_SDMMC1 25
> +#define TEGRA_MEMORY_CLIENT_SDMMC2 26
> +#define TEGRA_MEMORY_CLIENT_SDMMC3 27
> +#define TEGRA_MEMORY_CLIENT_MAX TEGRA_MEMORY_CLIENT_SDMMC3
> +
> +#define TEGRA_MEMORY_CLIENT_3D0 TEGRA_MEMORY_CLIENT_3D
> +#define TEGRA_MEMORY_CLIENT_MPE TEGRA_MEMORY_CLIENT_MPEA
> +#define TEGRA_MEMORY_CLIENT_NVENC TEGRA_MEMORY_CLIENT_MSENC
> +#define TEGRA_MEMORY_CLIENT_ISP2 TEGRA_MEMORY_CLIENT_ISP
> +
> +#ifdef CONFIG_ARCH_TEGRA
> +int tegra_memory_client_hot_reset(unsigned int id, struct reset_control *rst,
> + unsigned long usecs);
> +int tegra_memory_client_hot_reset_assert(unsigned int id,
> + struct reset_control *rst);
> +int tegra_memory_client_hot_reset_deassert(unsigned int id,
> + struct reset_control *rst);
> +#else
I *really* don't want yet another custom API. We've had a number of
these in the past and it's always been extremely painful to get rid of
them. And we probably won't ever be able to completely get rid of the
powergate API, which means additional headaches everytime we need to
touch code in that area. I don't want to repeat that mistake.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] memory: tegra: Squash tegra20-mc into common tegra-mc driver
2018-02-13 10:30 ` Thierry Reding
@ 2018-02-14 11:15 ` Peter De Schrijver
2018-02-14 15:42 ` Dmitry Osipenko
2018-02-19 2:04 ` Dmitry Osipenko
1 sibling, 1 reply; 11+ messages in thread
From: Peter De Schrijver @ 2018-02-14 11:15 UTC (permalink / raw)
To: Thierry Reding
Cc: Dmitry Osipenko, Jonathan Hunter, Philipp Zabel, linux-tegra,
linux-kernel
On Tue, Feb 13, 2018 at 11:30:39AM +0100, Thierry Reding wrote:
> > }
>
> It's odd that we don't have an MC clock on Tegra2. I wonder if perhaps
> we just never implemented one, or it uses one which is always on by
> default. Cc Peter to see if he knows.
We do, it has DT ID TEGRA20_CLK_MC. However, it looks like the modeling is
incorrect for Tegra20. Unlike on Tegra30 where the MC clock can be either
half or the same as the EMC clock, it is always half the EMC clock on
Tegra20. This happens to work because we likely never change the MC clock
and the non-existing bit just reads as 0, which means half the MC clock.
Peter.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] memory: tegra: Squash tegra20-mc into common tegra-mc driver
2018-02-14 11:15 ` Peter De Schrijver
@ 2018-02-14 15:42 ` Dmitry Osipenko
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Osipenko @ 2018-02-14 15:42 UTC (permalink / raw)
To: Peter De Schrijver, Thierry Reding
Cc: Jonathan Hunter, Philipp Zabel, linux-tegra, linux-kernel
On 14.02.2018 14:15, Peter De Schrijver wrote:
> On Tue, Feb 13, 2018 at 11:30:39AM +0100, Thierry Reding wrote:
>>> }
>>
>> It's odd that we don't have an MC clock on Tegra2. I wonder if perhaps
>> we just never implemented one, or it uses one which is always on by
>> default. Cc Peter to see if he knows.
>
> We do, it has DT ID TEGRA20_CLK_MC. However, it looks like the modeling is
> incorrect for Tegra20. Unlike on Tegra30 where the MC clock can be either
> half or the same as the EMC clock, it is always half the EMC clock on
> Tegra20. This happens to work because we likely never change the MC clock
> and the non-existing bit just reads as 0, which means half the MC clock.
Yes, and also that clock isn't specified in DT for MC. So probably we would have
to use clk_get_sys even if it was implemented correctly.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] memory: tegra: Squash tegra20-mc into common tegra-mc driver
2018-02-13 10:30 ` Thierry Reding
2018-02-14 11:15 ` Peter De Schrijver
@ 2018-02-19 2:04 ` Dmitry Osipenko
2018-02-19 2:16 ` Dmitry Osipenko
1 sibling, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2018-02-19 2:04 UTC (permalink / raw)
To: Thierry Reding
Cc: Jonathan Hunter, Philipp Zabel, Peter De Schrijver, linux-tegra,
linux-kernel
On 13.02.2018 13:30, Thierry Reding wrote:
> On Mon, Feb 12, 2018 at 08:06:30PM +0300, Dmitry Osipenko wrote:
>> Tegra30+ has some minor differences in registers / bits layout compared
>> to Tegra20. Let's squash Tegra20 driver into the common tegra-mc driver
>> to reduce code a tad, this also will be useful for the upcoming Tegra's MC
>> reset API.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> drivers/memory/Kconfig | 10 --
>> drivers/memory/Makefile | 1 -
>> drivers/memory/tegra/Makefile | 1 +
>> drivers/memory/tegra/mc.c | 184 +++++++++++++++++++----------
>> drivers/memory/tegra/mc.h | 10 ++
>> drivers/memory/tegra/tegra20.c | 72 ++++++++++++
>> drivers/memory/tegra20-mc.c | 254 -----------------------------------------
>> include/soc/tegra/mc.h | 4 +-
>> 8 files changed, 211 insertions(+), 325 deletions(-)
>> create mode 100644 drivers/memory/tegra/tegra20.c
>> delete mode 100644 drivers/memory/tegra20-mc.c
>
> I generally like the idea of unifying the drivers, but I think this case
> is somewhat borderline because the changes don't come naturally. That is
> the parameterizations here seem overly heavy with a lot of special cases
> for Tegra20. To me that indicates that Tegra20 is conceptually too much
> apart from Tegra30 and later to make unification reasonable.
>
> However, I'd still very much like to see them unified, so let's go
> through the remainder in more detail.
>
>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
>> index 19a0e83f260d..8d731d6c3e54 100644
>> --- a/drivers/memory/Kconfig
>> +++ b/drivers/memory/Kconfig
>> @@ -104,16 +104,6 @@ config MVEBU_DEVBUS
>> Armada 370 and Armada XP. This controller allows to handle flash
>> devices such as NOR, NAND, SRAM, and FPGA.
>>
>> -config TEGRA20_MC
>> - bool "Tegra20 Memory Controller(MC) driver"
>> - default y
>> - depends on ARCH_TEGRA_2x_SOC
>> - help
>> - This driver is for the Memory Controller(MC) module available
>> - in Tegra20 SoCs, mainly for a address translation fault
>> - analysis, especially for IOMMU/GART(Graphics Address
>> - Relocation Table) module.
>> -
>> config FSL_CORENET_CF
>> tristate "Freescale CoreNet Error Reporting"
>> depends on FSL_SOC_BOOKE
>> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
>> index 66f55240830e..a01ab3e22f94 100644
>> --- a/drivers/memory/Makefile
>> +++ b/drivers/memory/Makefile
>> @@ -16,7 +16,6 @@ obj-$(CONFIG_OMAP_GPMC) += omap-gpmc.o
>> obj-$(CONFIG_FSL_CORENET_CF) += fsl-corenet-cf.o
>> obj-$(CONFIG_FSL_IFC) += fsl_ifc.o
>> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o
>> -obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o
>> obj-$(CONFIG_JZ4780_NEMC) += jz4780-nemc.o
>> obj-$(CONFIG_MTK_SMI) += mtk-smi.o
>> obj-$(CONFIG_DA8XX_DDRCTL) += da8xx-ddrctl.o
>> diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
>> index ce87a9470034..94ab16ba075b 100644
>> --- a/drivers/memory/tegra/Makefile
>> +++ b/drivers/memory/tegra/Makefile
>> @@ -1,6 +1,7 @@
>> # SPDX-License-Identifier: GPL-2.0
>> tegra-mc-y := mc.o
>>
>> +tegra-mc-$(CONFIG_ARCH_TEGRA_2x_SOC) += tegra20.o
>> tegra-mc-$(CONFIG_ARCH_TEGRA_3x_SOC) += tegra30.o
>> tegra-mc-$(CONFIG_ARCH_TEGRA_114_SOC) += tegra114.o
>> tegra-mc-$(CONFIG_ARCH_TEGRA_124_SOC) += tegra124.o
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index a4803ac192bb..187a9005351b 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -27,6 +27,7 @@
>> #define MC_INT_INVALID_SMMU_PAGE (1 << 10)
>> #define MC_INT_ARBITRATION_EMEM (1 << 9)
>> #define MC_INT_SECURITY_VIOLATION (1 << 8)
>> +#define MC_INT_INVALID_GART_PAGE (1 << 7)
>> #define MC_INT_DECERR_EMEM (1 << 6)
>>
>> #define MC_INTMASK 0x004
>> @@ -53,7 +54,14 @@
>> #define MC_EMEM_ADR_CFG 0x54
>> #define MC_EMEM_ADR_CFG_EMEM_NUMDEV BIT(0)
>>
>> +#define MC_GART_ERROR_REQ 0x30
>> +#define MC_DECERR_EMEM_OTHERS_STATUS 0x58
>> +#define MC_SECURITY_VIOLATION_STATUS 0x74
>> +
>> static const struct of_device_id tegra_mc_of_match[] = {
>> +#ifdef CONFIG_ARCH_TEGRA_2x_SOC
>> + { .compatible = "nvidia,tegra20-mc", .data = &tegra20_mc_soc },
>> +#endif
>> #ifdef CONFIG_ARCH_TEGRA_3x_SOC
>> { .compatible = "nvidia,tegra30-mc", .data = &tegra30_mc_soc },
>> #endif
>> @@ -79,6 +87,9 @@ static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
>> unsigned int i;
>> u32 value;
>>
>> + if (mc->soc->tegra20)
>> + return 0;
>
> Test for feature flags rather than chip generation. This could be
> swapped for:
>
> if (mc->soc->supports_latency_allowance)
> return 0;
I'll try to do it other way in V2, without introducing any new flag at all.
>> +
>> /* compute the number of MC clock cycles per tick */
>> tick = mc->tick * clk_get_rate(mc->clk);
>> do_div(tick, NSEC_PER_SEC);
>> @@ -229,6 +240,7 @@ static int tegra_mc_setup_timings(struct tegra_mc *mc)
>> 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",
>> @@ -257,78 +269,124 @@ static irqreturn_t tegra_mc_irq(int irq, void *data)
>>
>> for_each_set_bit(bit, &status, 32) {
>> const char *error = status_names[bit] ?: "unknown";
>> - const char *client = "unknown", *desc;
>> - const char *direction, *secure;
>> + const char *client = "unknown", *desc = "";
>> + const char *direction = "read", *secure = "";
>> phys_addr_t addr = 0;
>> unsigned int i;
>> - char perm[7];
>> + char perm[7] = { 0 };
>> u8 id, type;
>> - u32 value;
>> + u32 value, reg;
>>
>> - value = mc_readl(mc, MC_ERR_STATUS);
>> + if (mc->soc->tegra20) {
>> + switch (bit) {
>> + case 6:
>
> Can we have symbolic names for this (and other bits)?
Sure.
>> + reg = MC_DECERR_EMEM_OTHERS_STATUS;
>> + value = mc_readl(mc, reg);
>>
>> -#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> - if (mc->soc->num_address_bits > 32) {
>> - addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
>> - MC_ERR_STATUS_ADR_HI_MASK);
>> - addr <<= 32;
>> - }
>> -#endif
>> + id = value & mc->soc->client_id_mask;
>> + desc = error_names[2];
>>
>> - if (value & MC_ERR_STATUS_RW)
>> - direction = "write";
>> - else
>> - direction = "read";
>> + if (value & BIT(31))
>> + direction = "write";
>> + break;
>>
>> - if (value & MC_ERR_STATUS_SECURITY)
>> - secure = "secure ";
>> - else
>> - secure = "";
>> + case 7:
>> + reg = MC_GART_ERROR_REQ;
>> + value = mc_readl(mc, reg);
>>
>> - id = value & mc->soc->client_id_mask;
>> + id = (value >> 1) & mc->soc->client_id_mask;
>> + desc = error_names[2];
>>
>> - for (i = 0; i < mc->soc->num_clients; i++) {
>> - if (mc->soc->clients[i].id == id) {
>> - client = mc->soc->clients[i].name;
>> + if (value & BIT(0))
>> + direction = "write";
>> + break;
>> +
>> + case 8:
>> + 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:
>> + reg = 0;
>> + direction = "";
>
> This makes no sense to me. Why reset direction here if you already
> explicitly set direction to "read". Why not just leave it unset until
> you know exactly what it's going to be? Why do we even continue in a
> case where we know nothing of the error status?
I decided to re-do the "invalid" bits handling in other way and this "default"
case will be thrown away.
We continue because it's the point of handling _ALL_ bits here, including
erroneous. I don't understand what's the question because you made code that
way, I just added bits handling for T20.
>> + id = mc->soc->num_clients;
>> break;
>> }
>> - }
>>
>> - type = (value & MC_ERR_STATUS_TYPE_MASK) >>
>> - MC_ERR_STATUS_TYPE_SHIFT;
>> - desc = error_names[type];
>> + if (id < mc->soc->num_clients)
>> + client = mc->soc->clients[id].name;
>>
>> - switch (value & MC_ERR_STATUS_TYPE_MASK) {
>> - case MC_ERR_STATUS_TYPE_INVALID_SMMU_PAGE:
>> - perm[0] = ' ';
>> - perm[1] = '[';
>> + if (reg)
>> + addr = mc_readl(mc, reg + sizeof(u32));
>> + } else {
>> + value = mc_readl(mc, MC_ERR_STATUS);
>>
>> - if (value & MC_ERR_STATUS_READABLE)
>> - perm[2] = 'R';
>> - else
>> - perm[2] = '-';
>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> + if (mc->soc->num_address_bits > 32) {
>> + addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
>> + MC_ERR_STATUS_ADR_HI_MASK);
>> + addr <<= 32;
>> + }
>> +#endif
>> + if (value & MC_ERR_STATUS_RW)
>> + direction = "write";
>>
>> - if (value & MC_ERR_STATUS_WRITABLE)
>> - perm[3] = 'W';
>> - else
>> - perm[3] = '-';
>> + if (value & MC_ERR_STATUS_SECURITY)
>> + secure = "secure ";
>>
>> - if (value & MC_ERR_STATUS_NONSECURE)
>> - perm[4] = '-';
>> - else
>> - perm[4] = 'S';
>> + id = value & mc->soc->client_id_mask;
>>
>> - perm[5] = ']';
>> - perm[6] = '\0';
>> - break;
>> + for (i = 0; i < mc->soc->num_clients; i++) {
>> + if (mc->soc->clients[i].id == id) {
>> + client = mc->soc->clients[i].name;
>> + break;
>> + }
>> + }
>>
>> - default:
>> - perm[0] = '\0';
>> - break;
>> - }
>> + type = (value & MC_ERR_STATUS_TYPE_MASK) >>
>> + MC_ERR_STATUS_TYPE_SHIFT;
>> + desc = error_names[type];
>> +
>> + switch (value & MC_ERR_STATUS_TYPE_MASK) {
>> + case MC_ERR_STATUS_TYPE_INVALID_SMMU_PAGE:
>> + perm[0] = ' ';
>> + perm[1] = '[';
>> +
>> + if (value & MC_ERR_STATUS_READABLE)
>> + perm[2] = 'R';
>> + else
>> + perm[2] = '-';
>> +
>> + if (value & MC_ERR_STATUS_WRITABLE)
>> + perm[3] = 'W';
>> + else
>> + perm[3] = '-';
>>
>> - value = mc_readl(mc, MC_ERR_ADR);
>> - addr |= value;
>> + if (value & MC_ERR_STATUS_NONSECURE)
>> + perm[4] = '-';
>> + else
>> + perm[4] = 'S';
>> +
>> + perm[5] = ']';
>> + perm[6] = '\0';
>> + break;
>> +
>> + default:
>> + perm[0] = '\0';
>> + break;
>> + }
>> +
>> + value = mc_readl(mc, MC_ERR_ADR);
>> + addr |= value;
>> + }
>>
>> dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s%s)\n",
>> client, secure, direction, &addr, error,
>
> I'd prefer if we completely separated the Tegra20 implementation of this
> handler from the Tegra30+ implementation. Both don't end up sharing very
> much in the end but this variant is very difficult to read, in my
> opinion.
I don't mind, although it is difficult to read because patch diff is formed that
way, maybe format-patch options could be tweaked to make it readable. The actual
code is "if (mc->soc->tegra20) {...} else {original code}".
Actually it is good that you asked to do it, because I've spotted couple issues
in this (and relative) code.
>> @@ -369,11 +427,18 @@ static int tegra_mc_probe(struct platform_device *pdev)
>> if (IS_ERR(mc->regs))
>> return PTR_ERR(mc->regs);
>>
>> - mc->clk = devm_clk_get(&pdev->dev, "mc");
>> - if (IS_ERR(mc->clk)) {
>> - dev_err(&pdev->dev, "failed to get MC clock: %ld\n",
>> - PTR_ERR(mc->clk));
>> - return PTR_ERR(mc->clk);
>> + if (mc->soc->tegra20) {
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + mc->regs2 = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(mc->regs2))
>> + return PTR_ERR(mc->regs2);
>
> Ugh... this is really ugly. In retrospect we really should've left the
> memory-controller and iommu in the same device tree node. There's really
> no reason for them to be separate, other than perhaps the Linux driver
> model, which we could easily workaround by just instancing the IOMMU
> device from the memory controller driver. That way we could simply share
> the I/O mapping between both and avoid these games with two regions.
Probably MC should have been an MFD and GART its sub-device from the start.
Anyway I'll try to make this code a bit nicer.
>> + } else {
>> + mc->clk = devm_clk_get(&pdev->dev, "mc");
>> + if (IS_ERR(mc->clk)) {
>> + dev_err(&pdev->dev, "failed to get MC clock: %ld\n",
>> + PTR_ERR(mc->clk));
>> + return PTR_ERR(mc->clk);
>> + }
>> }
>
> It's odd that we don't have an MC clock on Tegra2. I wonder if perhaps
> we just never implemented one, or it uses one which is always on by
> default. Cc Peter to see if he knows.
>
>>
>> err = tegra_mc_setup_latency_allowance(mc);
>> @@ -416,7 +481,8 @@ static int tegra_mc_probe(struct platform_device *pdev)
>>
>> value = MC_INT_DECERR_MTS | MC_INT_SECERR_SEC | MC_INT_DECERR_VPR |
>> MC_INT_INVALID_APB_ASID_UPDATE | MC_INT_INVALID_SMMU_PAGE |
>> - MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM;
>> + MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM |
>> + MC_INT_INVALID_GART_PAGE;
>
> This should be conditionalized on a feature flag such as "has_gart". For
> most generations of Tegra this would probably work, but newer versions
> have become quite picky about these kinds of things, so in some cases an
> access to a reserved register or field can cause an exception.
I noticed that some of these flags also don't apply to T30/T40, so for now I
decided to add "intr_mask" per SoC instead of adding the "has_gart" flag.
>>
>> mc_writel(mc, value, MC_INTMASK);
>>
>> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
>> index ddb16676c3af..1642fbea5ce3 100644
>> --- a/drivers/memory/tegra/mc.h
>> +++ b/drivers/memory/tegra/mc.h
>> @@ -16,15 +16,25 @@
>>
>> static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
>> {
>> + if (mc->soc->tegra20 && offset >= 0x24)
>> + return readl(mc->regs2 + offset - 0x3c);
>
> Erm... this is weird. Shouldn't the check be offset >= 0x3c? Otherwise
> you could turn the offset into a very large number and access memory
> outside of the mapping. At least technically.
Either way it would be access outside of the mapping. Probably it should be
better if the mapping address is completely invalid, because there is a better
chance that it won't be unnoticed.
> Again, it'd be so much easier if MC and IOMMU were a single device as
> they are in actual hardware. I'm sure we could argue the case that the
> current DTS is buggy and that it's reasonable to break backwards-
> compatibility.
>
>> +
>> return readl(mc->regs + offset);
>> }
>>
>> static inline void mc_writel(struct tegra_mc *mc, u32 value,
>> unsigned long offset)
>> {
>> + if (mc->soc->tegra20 && offset >= 0x24)
>> + return writel(value, mc->regs2 + offset - 0x3c);
>> +
>> writel(value, mc->regs + offset);
>> }
>>
>> +#ifdef CONFIG_ARCH_TEGRA_2x_SOC
>> +extern const struct tegra_mc_soc tegra20_mc_soc;
>> +#endif
>> +
>> #ifdef CONFIG_ARCH_TEGRA_3x_SOC
>> extern const struct tegra_mc_soc tegra30_mc_soc;
>> #endif
>> diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c
>> new file mode 100644
>> index 000000000000..81a082bdba19
>> --- /dev/null
>> +++ b/drivers/memory/tegra/tegra20.c
>> @@ -0,0 +1,72 @@
>> +/*
>> + * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include "mc.h"
>> +
>> +static const struct tegra_mc_client tegra20_mc_clients[] = {
>> + { .name = "display0a" },
>> + { .name = "display0ab" },
>> + { .name = "display0b" },
>> + { .name = "display0bb" },
>> + { .name = "display0c" },
>> + { .name = "display0cb" },
>> + { .name = "display1b" },
>> + { .name = "display1bb" },
>> + { .name = "eppup" },
>> + { .name = "g2pr" },
>> + { .name = "g2sr" },
>> + { .name = "mpeunifbr" },
>> + { .name = "viruv" },
>> + { .name = "avpcarm7r" },
>> + { .name = "displayhc" },
>> + { .name = "displayhcb" },
>> + { .name = "fdcdrd" },
>> + { .name = "g2dr" },
>> + { .name = "host1xdmar" },
>> + { .name = "host1xr" },
>> + { .name = "idxsrd" },
>> + { .name = "mpcorer" },
>> + { .name = "mpe_ipred" },
>> + { .name = "mpeamemrd" },
>> + { .name = "mpecsrd" },
>> + { .name = "ppcsahbdmar" },
>> + { .name = "ppcsahbslvr" },
>> + { .name = "texsrd" },
>> + { .name = "vdebsevr" },
>> + { .name = "vdember" },
>> + { .name = "vdemcer" },
>> + { .name = "vdetper" },
>> + { .name = "eppu" },
>> + { .name = "eppv" },
>> + { .name = "eppy" },
>> + { .name = "mpeunifbw" },
>> + { .name = "viwsb" },
>> + { .name = "viwu" },
>> + { .name = "viwv" },
>> + { .name = "viwy" },
>> + { .name = "g2dw" },
>> + { .name = "avpcarm7w" },
>> + { .name = "fdcdwr" },
>> + { .name = "host1xw" },
>> + { .name = "ispw" },
>> + { .name = "mpcorew" },
>> + { .name = "mpecswr" },
>> + { .name = "ppcsahbdmaw" },
>> + { .name = "ppcsahbslvw" },
>> + { .name = "vdebsevw" },
>> + { .name = "vdembew" },
>> + { .name = "vdetpmw" },
>> +};
>
> Can you please initialize the .id field for these? I know they aren't
> technically necessary because the Tegra20 code doesn't actually look up
> the client by ID (because the ID happens to match the array index), but
> I'd like this to be consistent across all generations.
Okay.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] memory: tegra: Squash tegra20-mc into common tegra-mc driver
2018-02-19 2:04 ` Dmitry Osipenko
@ 2018-02-19 2:16 ` Dmitry Osipenko
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Osipenko @ 2018-02-19 2:16 UTC (permalink / raw)
To: Thierry Reding
Cc: Jonathan Hunter, Philipp Zabel, Peter De Schrijver, linux-tegra,
linux-kernel
On 19.02.2018 05:04, Dmitry Osipenko wrote:
> On 13.02.2018 13:30, Thierry Reding wrote:
>> On Mon, Feb 12, 2018 at 08:06:30PM +0300, Dmitry Osipenko wrote:
>>> Tegra30+ has some minor differences in registers / bits layout compared
>>> to Tegra20. Let's squash Tegra20 driver into the common tegra-mc driver
>>> to reduce code a tad, this also will be useful for the upcoming Tegra's MC
>>> reset API.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>> drivers/memory/Kconfig | 10 --
>>> drivers/memory/Makefile | 1 -
>>> drivers/memory/tegra/Makefile | 1 +
>>> drivers/memory/tegra/mc.c | 184 +++++++++++++++++++----------
>>> drivers/memory/tegra/mc.h | 10 ++
>>> drivers/memory/tegra/tegra20.c | 72 ++++++++++++
>>> drivers/memory/tegra20-mc.c | 254 -----------------------------------------
>>> include/soc/tegra/mc.h | 4 +-
>>> 8 files changed, 211 insertions(+), 325 deletions(-)
>>> create mode 100644 drivers/memory/tegra/tegra20.c
>>> delete mode 100644 drivers/memory/tegra20-mc.c
>>
>> I generally like the idea of unifying the drivers, but I think this case
>> is somewhat borderline because the changes don't come naturally. That is
>> the parameterizations here seem overly heavy with a lot of special cases
>> for Tegra20. To me that indicates that Tegra20 is conceptually too much
>> apart from Tegra30 and later to make unification reasonable.
>>
>> However, I'd still very much like to see them unified, so let's go
>> through the remainder in more detail.
>>
>>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
>>> index 19a0e83f260d..8d731d6c3e54 100644
>>> --- a/drivers/memory/Kconfig
>>> +++ b/drivers/memory/Kconfig
>>> @@ -104,16 +104,6 @@ config MVEBU_DEVBUS
>>> Armada 370 and Armada XP. This controller allows to handle flash
>>> devices such as NOR, NAND, SRAM, and FPGA.
>>>
>>> -config TEGRA20_MC
>>> - bool "Tegra20 Memory Controller(MC) driver"
>>> - default y
>>> - depends on ARCH_TEGRA_2x_SOC
>>> - help
>>> - This driver is for the Memory Controller(MC) module available
>>> - in Tegra20 SoCs, mainly for a address translation fault
>>> - analysis, especially for IOMMU/GART(Graphics Address
>>> - Relocation Table) module.
>>> -
>>> config FSL_CORENET_CF
>>> tristate "Freescale CoreNet Error Reporting"
>>> depends on FSL_SOC_BOOKE
>>> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
>>> index 66f55240830e..a01ab3e22f94 100644
>>> --- a/drivers/memory/Makefile
>>> +++ b/drivers/memory/Makefile
>>> @@ -16,7 +16,6 @@ obj-$(CONFIG_OMAP_GPMC) += omap-gpmc.o
>>> obj-$(CONFIG_FSL_CORENET_CF) += fsl-corenet-cf.o
>>> obj-$(CONFIG_FSL_IFC) += fsl_ifc.o
>>> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o
>>> -obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o
>>> obj-$(CONFIG_JZ4780_NEMC) += jz4780-nemc.o
>>> obj-$(CONFIG_MTK_SMI) += mtk-smi.o
>>> obj-$(CONFIG_DA8XX_DDRCTL) += da8xx-ddrctl.o
>>> diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
>>> index ce87a9470034..94ab16ba075b 100644
>>> --- a/drivers/memory/tegra/Makefile
>>> +++ b/drivers/memory/tegra/Makefile
>>> @@ -1,6 +1,7 @@
>>> # SPDX-License-Identifier: GPL-2.0
>>> tegra-mc-y := mc.o
>>>
>>> +tegra-mc-$(CONFIG_ARCH_TEGRA_2x_SOC) += tegra20.o
>>> tegra-mc-$(CONFIG_ARCH_TEGRA_3x_SOC) += tegra30.o
>>> tegra-mc-$(CONFIG_ARCH_TEGRA_114_SOC) += tegra114.o
>>> tegra-mc-$(CONFIG_ARCH_TEGRA_124_SOC) += tegra124.o
>>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>>> index a4803ac192bb..187a9005351b 100644
>>> --- a/drivers/memory/tegra/mc.c
>>> +++ b/drivers/memory/tegra/mc.c
>>> @@ -27,6 +27,7 @@
>>> #define MC_INT_INVALID_SMMU_PAGE (1 << 10)
>>> #define MC_INT_ARBITRATION_EMEM (1 << 9)
>>> #define MC_INT_SECURITY_VIOLATION (1 << 8)
>>> +#define MC_INT_INVALID_GART_PAGE (1 << 7)
>>> #define MC_INT_DECERR_EMEM (1 << 6)
>>>
>>> #define MC_INTMASK 0x004
>>> @@ -53,7 +54,14 @@
>>> #define MC_EMEM_ADR_CFG 0x54
>>> #define MC_EMEM_ADR_CFG_EMEM_NUMDEV BIT(0)
>>>
>>> +#define MC_GART_ERROR_REQ 0x30
>>> +#define MC_DECERR_EMEM_OTHERS_STATUS 0x58
>>> +#define MC_SECURITY_VIOLATION_STATUS 0x74
>>> +
>>> static const struct of_device_id tegra_mc_of_match[] = {
>>> +#ifdef CONFIG_ARCH_TEGRA_2x_SOC
>>> + { .compatible = "nvidia,tegra20-mc", .data = &tegra20_mc_soc },
>>> +#endif
>>> #ifdef CONFIG_ARCH_TEGRA_3x_SOC
>>> { .compatible = "nvidia,tegra30-mc", .data = &tegra30_mc_soc },
>>> #endif
>>> @@ -79,6 +87,9 @@ static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
>>> unsigned int i;
>>> u32 value;
>>>
>>> + if (mc->soc->tegra20)
>>> + return 0;
>>
>> Test for feature flags rather than chip generation. This could be
>> swapped for:
>>
>> if (mc->soc->supports_latency_allowance)
>> return 0;
>
> I'll try to do it other way in V2, without introducing any new flag at all.
>
>>> +
>>> /* compute the number of MC clock cycles per tick */
>>> tick = mc->tick * clk_get_rate(mc->clk);
>>> do_div(tick, NSEC_PER_SEC);
>>> @@ -229,6 +240,7 @@ static int tegra_mc_setup_timings(struct tegra_mc *mc)
>>> 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",
>>> @@ -257,78 +269,124 @@ static irqreturn_t tegra_mc_irq(int irq, void *data)
>>>
>>> for_each_set_bit(bit, &status, 32) {
>>> const char *error = status_names[bit] ?: "unknown";
>>> - const char *client = "unknown", *desc;
>>> - const char *direction, *secure;
>>> + const char *client = "unknown", *desc = "";
>>> + const char *direction = "read", *secure = "";
>>> phys_addr_t addr = 0;
>>> unsigned int i;
>>> - char perm[7];
>>> + char perm[7] = { 0 };
>>> u8 id, type;
>>> - u32 value;
>>> + u32 value, reg;
>>>
>>> - value = mc_readl(mc, MC_ERR_STATUS);
>>> + if (mc->soc->tegra20) {
>>> + switch (bit) {
>>> + case 6:
>>
>> Can we have symbolic names for this (and other bits)?
>
> Sure.
>
>>> + reg = MC_DECERR_EMEM_OTHERS_STATUS;
>>> + value = mc_readl(mc, reg);
>>>
>>> -#ifdef CONFIG_PHYS_ADDR_T_64BIT
>>> - if (mc->soc->num_address_bits > 32) {
>>> - addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
>>> - MC_ERR_STATUS_ADR_HI_MASK);
>>> - addr <<= 32;
>>> - }
>>> -#endif
>>> + id = value & mc->soc->client_id_mask;
>>> + desc = error_names[2];
>>>
>>> - if (value & MC_ERR_STATUS_RW)
>>> - direction = "write";
>>> - else
>>> - direction = "read";
>>> + if (value & BIT(31))
>>> + direction = "write";
>>> + break;
>>>
>>> - if (value & MC_ERR_STATUS_SECURITY)
>>> - secure = "secure ";
>>> - else
>>> - secure = "";
>>> + case 7:
>>> + reg = MC_GART_ERROR_REQ;
>>> + value = mc_readl(mc, reg);
>>>
>>> - id = value & mc->soc->client_id_mask;
>>> + id = (value >> 1) & mc->soc->client_id_mask;
>>> + desc = error_names[2];
>>>
>>> - for (i = 0; i < mc->soc->num_clients; i++) {
>>> - if (mc->soc->clients[i].id == id) {
>>> - client = mc->soc->clients[i].name;
>>> + if (value & BIT(0))
>>> + direction = "write";
>>> + break;
>>> +
>>> + case 8:
>>> + 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:
>>> + reg = 0;
>>> + direction = "";
>>
>> This makes no sense to me. Why reset direction here if you already
>> explicitly set direction to "read". Why not just leave it unset until
>> you know exactly what it's going to be? Why do we even continue in a
>> case where we know nothing of the error status?
>
> I decided to re-do the "invalid" bits handling in other way and this "default"
> case will be thrown away.
>
> We continue because it's the point of handling _ALL_ bits here, including
> erroneous. I don't understand what's the question because you made code that
> way, I just added bits handling for T20.
>
>>> + id = mc->soc->num_clients;
>>> break;
>>> }
>>> - }
>>>
>>> - type = (value & MC_ERR_STATUS_TYPE_MASK) >>
>>> - MC_ERR_STATUS_TYPE_SHIFT;
>>> - desc = error_names[type];
>>> + if (id < mc->soc->num_clients)
>>> + client = mc->soc->clients[id].name;
>>>
>>> - switch (value & MC_ERR_STATUS_TYPE_MASK) {
>>> - case MC_ERR_STATUS_TYPE_INVALID_SMMU_PAGE:
>>> - perm[0] = ' ';
>>> - perm[1] = '[';
>>> + if (reg)
>>> + addr = mc_readl(mc, reg + sizeof(u32));
>>> + } else {
>>> + value = mc_readl(mc, MC_ERR_STATUS);
>>>
>>> - if (value & MC_ERR_STATUS_READABLE)
>>> - perm[2] = 'R';
>>> - else
>>> - perm[2] = '-';
>>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>>> + if (mc->soc->num_address_bits > 32) {
>>> + addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
>>> + MC_ERR_STATUS_ADR_HI_MASK);
>>> + addr <<= 32;
>>> + }
>>> +#endif
>>> + if (value & MC_ERR_STATUS_RW)
>>> + direction = "write";
>>>
>>> - if (value & MC_ERR_STATUS_WRITABLE)
>>> - perm[3] = 'W';
>>> - else
>>> - perm[3] = '-';
>>> + if (value & MC_ERR_STATUS_SECURITY)
>>> + secure = "secure ";
>>>
>>> - if (value & MC_ERR_STATUS_NONSECURE)
>>> - perm[4] = '-';
>>> - else
>>> - perm[4] = 'S';
>>> + id = value & mc->soc->client_id_mask;
>>>
>>> - perm[5] = ']';
>>> - perm[6] = '\0';
>>> - break;
>>> + for (i = 0; i < mc->soc->num_clients; i++) {
>>> + if (mc->soc->clients[i].id == id) {
>>> + client = mc->soc->clients[i].name;
>>> + break;
>>> + }
>>> + }
>>>
>>> - default:
>>> - perm[0] = '\0';
>>> - break;
>>> - }
>>> + type = (value & MC_ERR_STATUS_TYPE_MASK) >>
>>> + MC_ERR_STATUS_TYPE_SHIFT;
>>> + desc = error_names[type];
>>> +
>>> + switch (value & MC_ERR_STATUS_TYPE_MASK) {
>>> + case MC_ERR_STATUS_TYPE_INVALID_SMMU_PAGE:
>>> + perm[0] = ' ';
>>> + perm[1] = '[';
>>> +
>>> + if (value & MC_ERR_STATUS_READABLE)
>>> + perm[2] = 'R';
>>> + else
>>> + perm[2] = '-';
>>> +
>>> + if (value & MC_ERR_STATUS_WRITABLE)
>>> + perm[3] = 'W';
>>> + else
>>> + perm[3] = '-';
>>>
>>> - value = mc_readl(mc, MC_ERR_ADR);
>>> - addr |= value;
>>> + if (value & MC_ERR_STATUS_NONSECURE)
>>> + perm[4] = '-';
>>> + else
>>> + perm[4] = 'S';
>>> +
>>> + perm[5] = ']';
>>> + perm[6] = '\0';
>>> + break;
>>> +
>>> + default:
>>> + perm[0] = '\0';
>>> + break;
>>> + }
>>> +
>>> + value = mc_readl(mc, MC_ERR_ADR);
>>> + addr |= value;
>>> + }
>>>
>>> dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s%s)\n",
>>> client, secure, direction, &addr, error,
>>
>> I'd prefer if we completely separated the Tegra20 implementation of this
>> handler from the Tegra30+ implementation. Both don't end up sharing very
>> much in the end but this variant is very difficult to read, in my
>> opinion.
>
> I don't mind, although it is difficult to read because patch diff is formed that
> way, maybe format-patch options could be tweaked to make it readable. The actual
> code is "if (mc->soc->tegra20) {...} else {original code}".
>
> Actually it is good that you asked to do it, because I've spotted couple issues
> in this (and relative) code.
>
>>> @@ -369,11 +427,18 @@ static int tegra_mc_probe(struct platform_device *pdev)
>>> if (IS_ERR(mc->regs))
>>> return PTR_ERR(mc->regs);
>>>
>>> - mc->clk = devm_clk_get(&pdev->dev, "mc");
>>> - if (IS_ERR(mc->clk)) {
>>> - dev_err(&pdev->dev, "failed to get MC clock: %ld\n",
>>> - PTR_ERR(mc->clk));
>>> - return PTR_ERR(mc->clk);
>>> + if (mc->soc->tegra20) {
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> + mc->regs2 = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(mc->regs2))
>>> + return PTR_ERR(mc->regs2);
>>
>> Ugh... this is really ugly. In retrospect we really should've left the
>> memory-controller and iommu in the same device tree node. There's really
>> no reason for them to be separate, other than perhaps the Linux driver
>> model, which we could easily workaround by just instancing the IOMMU
>> device from the memory controller driver. That way we could simply share
>> the I/O mapping between both and avoid these games with two regions.
>
> Probably MC should have been an MFD and GART its sub-device from the start.
> Anyway I'll try to make this code a bit nicer.
>
>>> + } else {
>>> + mc->clk = devm_clk_get(&pdev->dev, "mc");
>>> + if (IS_ERR(mc->clk)) {
>>> + dev_err(&pdev->dev, "failed to get MC clock: %ld\n",
>>> + PTR_ERR(mc->clk));
>>> + return PTR_ERR(mc->clk);
>>> + }
>>> }
>>
>> It's odd that we don't have an MC clock on Tegra2. I wonder if perhaps
>> we just never implemented one, or it uses one which is always on by
>> default. Cc Peter to see if he knows.
>>
>>>
>>> err = tegra_mc_setup_latency_allowance(mc);
>>> @@ -416,7 +481,8 @@ static int tegra_mc_probe(struct platform_device *pdev)
>>>
>>> value = MC_INT_DECERR_MTS | MC_INT_SECERR_SEC | MC_INT_DECERR_VPR |
>>> MC_INT_INVALID_APB_ASID_UPDATE | MC_INT_INVALID_SMMU_PAGE |
>>> - MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM;
>>> + MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM |
>>> + MC_INT_INVALID_GART_PAGE;
>>
>> This should be conditionalized on a feature flag such as "has_gart". For
>> most generations of Tegra this would probably work, but newer versions
>> have become quite picky about these kinds of things, so in some cases an
>> access to a reserved register or field can cause an exception.
>
> I noticed that some of these flags also don't apply to T30/T40, so for now I
> decided to add "intr_mask" per SoC instead of adding the "has_gart" flag.
s/these flags/interrupt status bits/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] memory: tegra: Introduce memory client hot reset API
2018-02-13 11:24 ` Thierry Reding
@ 2018-02-19 12:35 ` Dmitry Osipenko
2018-02-19 12:44 ` Dmitry Osipenko
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2018-02-19 12:35 UTC (permalink / raw)
To: Thierry Reding; +Cc: Jonathan Hunter, Philipp Zabel, linux-tegra, linux-kernel
On 13.02.2018 14:24, Thierry Reding wrote:
> On Mon, Feb 12, 2018 at 08:06:31PM +0300, Dmitry Osipenko wrote:
>> In order to reset busy HW properly, memory controller needs to be
>> involved, otherwise it possible to get corrupted memory if HW was reset
>> during DMA. Introduce memory client 'hot reset' API that will be used
>> for resetting busy HW. The primary users are memory clients related to
>> video (decoder/encoder/camera) and graphics (2d/3d).
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> drivers/memory/tegra/mc.c | 249 ++++++++++++++++++++++++++++++++++++++++
>> drivers/memory/tegra/tegra114.c | 25 ++++
>> drivers/memory/tegra/tegra124.c | 32 ++++++
>> drivers/memory/tegra/tegra20.c | 23 ++++
>> drivers/memory/tegra/tegra210.c | 27 +++++
>> drivers/memory/tegra/tegra30.c | 25 ++++
>> include/soc/tegra/mc.h | 77 +++++++++++++
>> 7 files changed, 458 insertions(+)
>
> As discussed on IRC, I typed up a variant of this in an attempt to fix
> an unrelated bug report. The code is here:
>
> https://github.com/thierryreding/linux/commit/e104e703cb75dfe742b2e051194a0af8ddefcd03
>
> I think we can make this without adding a custom API. The reset control
> API should work just fine. The above version doesn't take into account
> some of Tegra20's quirks, but I think it should still work for Tegra20
> with just slightly different implementations for ->assert() and
> ->deassert().
>
> A couple of more specific comments below.
>
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index 187a9005351b..9838f588d64d 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -7,11 +7,13 @@
>> */
>>
>> #include <linux/clk.h>
>> +#include <linux/delay.h>
>> #include <linux/interrupt.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> #include <linux/slab.h>
>> #include <linux/sort.h>
>>
>> @@ -81,6 +83,172 @@ static const struct of_device_id tegra_mc_of_match[] = {
>> };
>> MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
>>
>> +static int terga_mc_flush_dma(struct tegra_mc *mc, unsigned int id)
>> +{
>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>> + u32 value, reg_poll = mc->soc->reg_client_flush_status;
>> + int retries = 3;
>> +
>> + value = mc_readl(mc, mc->soc->reg_client_ctrl);
>> +
>> + if (mc->soc->tegra20)
>> + value &= ~BIT(hw_id);
>> + else
>> + value |= BIT(hw_id);
>> +
>> + /* block clients DMA requests */
>> + mc_writel(mc, value, mc->soc->reg_client_ctrl);
>> +
>> + /* wait for completion of the outstanding DMA requests */
>> + if (mc->soc->tegra20) {
>> + while (mc_readl(mc, reg_poll + hw_id * sizeof(u32)) != 0) {
>> + if (!retries--)
>> + return -EBUSY;
>> +
>> + usleep_range(1000, 2000);
>> + }
>> + } else {
>> + while ((mc_readl(mc, reg_poll) & BIT(hw_id)) == 0) {
>> + if (!retries--)
>> + return -EBUSY;
>> +
>> + usleep_range(1000, 2000);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>
> I think this suffers from too much unification. The programming model is
> too different to stash this into a single function implementation and as
> a result this becomes very difficult to read. In my experience it's more
> readable to split this into separate implementations and pass around
> pointers to them.
>
>> +
>> +static int terga_mc_unblock_dma(struct tegra_mc *mc, unsigned int id)
>> +{
>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>> + u32 value;
>> +
>> + value = mc_readl(mc, mc->soc->reg_client_ctrl);
>> +
>> + if (mc->soc->tegra20)
>> + value |= BIT(hw_id);
>> + else
>> + value &= ~BIT(hw_id);
>> +
>> + mc_writel(mc, value, mc->soc->reg_client_ctrl);
>> +
>> + return 0;
>> +}
>> +
>> +static int terga_mc_hotreset_assert(struct tegra_mc *mc, unsigned int id)
>> +{
>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>> + u32 value;
>> +
>> + if (mc->soc->tegra20) {
>> + value = mc_readl(mc, mc->soc->reg_client_hotresetn);
>> +
>> + mc_writel(mc, value & ~BIT(hw_id),
>> + mc->soc->reg_client_hotresetn);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int terga_mc_hotreset_deassert(struct tegra_mc *mc, unsigned int id)
>> +{
>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>> + u32 value;
>> +
>> + if (mc->soc->tegra20) {
>> + value = mc_readl(mc, mc->soc->reg_client_hotresetn);
>> +
>> + mc_writel(mc, value | BIT(hw_id),
>> + mc->soc->reg_client_hotresetn);
>> + }
>> +
>> + return 0;
>> +}
>
> The same goes for these. I think we can do this much more easily by
> providing reset controller API ->assert() and ->deassert()
> implementations for Tegra20 and Tegra30+, and then register the reset
> controller device using the ops stored in the MC SoC structure.
>
>> +static int tegra_mc_hot_reset_assert(struct tegra_mc *mc, unsigned int id,
>> + struct reset_control *rst)
>> +{
>> + int err;
>> +
>> + /*
>> + * Block clients DMA requests and wait for completion of the
>> + * outstanding requests.
>> + */
>> + err = terga_mc_flush_dma(mc, id);
>> + if (err) {
>> + dev_err(mc->dev, "Failed to flush DMA: %d\n", err);
>> + return err;
>> + }
>> +
>> + /* put in reset HW that corresponds to the memory client */
>> + err = reset_control_assert(rst);
>> + if (err) {
>> + dev_err(mc->dev, "Failed to assert HW reset: %d\n", err);
>> + return err;
>> + }
>> +
>> + /* clear the client requests sitting before arbitration */
>> + err = terga_mc_hotreset_assert(mc, id);
>> + if (err) {
>> + dev_err(mc->dev, "Failed to hot reset client: %d\n", err);
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>
> This is very strictly according to the TRM, but I don't see a reason why
> you couldn't stash the DMA blocking and the hot reset into a ->assert()
> implementation...
>
>> +
>> +static int tegra_mc_hot_reset_deassert(struct tegra_mc *mc, unsigned int id,
>> + struct reset_control *rst)
>> +{
>> + int err;
>> +
>> + /* take out client from hot reset */
>> + err = terga_mc_hotreset_deassert(mc, id);
>> + if (err) {
>> + dev_err(mc->dev, "Failed to deassert hot reset: %d\n", err);
>> + return err;
>> + }
>> +
>> + /* take out from reset corresponding clients HW */
>> + err = reset_control_deassert(rst);
>> + if (err) {
>> + dev_err(mc->dev, "Failed to deassert HW reset: %d\n", err);
>> + return err;
>> + }
>> +
>> + /* allow new DMA requests to proceed to arbitration */
>> + err = terga_mc_unblock_dma(mc, id);
>> + if (err) {
>> + dev_err(mc->dev, "Failed to unblock client: %d\n", err);
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>
> ... and the hot reset deassertion and the DMA unblocking into a
> ->deassert() implementation.
>
> I think the important part is that DMA is blocked and the requests are
> cleared before the module reset, and similarily that the hot reset is
> released and DMA is unblocked after the module reset.
Okay, let's try the way you're suggesting and see if anything breaks..
>> +
>> +static int tegra_mc_hot_reset(struct tegra_mc *mc, unsigned int id,
>> + struct reset_control *rst, unsigned long usecs)
>> +{
>> + int err;
>> +
>> + err = tegra_mc_hot_reset_assert(mc, id, rst);
>> + if (err)
>> + return err;
>> +
>> + /* make sure that reset is propagated */
>> + if (usecs < 15)
>> + udelay(usecs);
>> + else
>> + usleep_range(usecs, usecs + 500);
>> +
>> + err = tegra_mc_hot_reset_deassert(mc, id, rst);
>> + if (err)
>> + return err;
>> +
>> + return 0;
>> +}
>
> Do you really need this helper? It seems like a marginal gain in terms
> of boilerplate while obviously some (or maybe even most?) drivers can't
> use this because they need more explicit control over the sequence.
>
> The only case where I could see this be useful is during some error
> recovery mechanism, in which case perhaps a runtime suspend/resume might
> be more useful.
>
>> +
>> static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
>> {
>> unsigned long long tick;
>> @@ -416,6 +584,7 @@ static int tegra_mc_probe(struct platform_device *pdev)
>> return -ENOMEM;
>>
>> platform_set_drvdata(pdev, mc);
>> + mutex_init(&mc->lock);
>> mc->soc = match->data;
>> mc->dev = &pdev->dev;
>>
>> @@ -499,6 +668,86 @@ static struct platform_driver tegra_mc_driver = {
>> .probe = tegra_mc_probe,
>> };
>>
>> +static int tegra_mc_match(struct device *dev, void *data)
>> +{
>> + return of_match_node(tegra_mc_of_match, dev->of_node) != NULL;
>> +}
>> +
>> +static struct tegra_mc *tegra_mc_find_device(void)
>> +{
>> + struct device *dev;
>> +
>> + dev = driver_find_device(&tegra_mc_driver.driver, NULL, NULL,
>> + tegra_mc_match);
>> + if (!dev)
>> + return NULL;
>> +
>> + return dev_get_drvdata(dev);
>> +}
>
> Another benefit of the reset controller API is that you don't have to
> look up the MC device like this. Instead you can just upcast the pointer
> to the reset controller device.
>
>> +
>> +int tegra_memory_client_hot_reset(unsigned int id, struct reset_control *rst,
>> + unsigned long usecs)
>> +{
>> + struct tegra_mc *mc;
>> + int ret;
>> +
>> + mc = tegra_mc_find_device();
>> + if (!mc)
>> + return -ENODEV;
>> +
>> + if (id >= mc->soc->num_modules || !mc->soc->modules[id].valid)
>> + return -EINVAL;
>
> One of the problems with sparse arrays is that you need to explicitly
> mark each of the entries as valid. This is error prone and tedious in
> my opinion.
>
>> +
>> + mutex_lock(&mc->lock);
>> + ret = tegra_mc_hot_reset(mc, id, rst, usecs);
>> + mutex_unlock(&mc->lock);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(tegra_memory_client_hot_reset);
>> +
>> +int tegra_memory_client_hot_reset_assert(unsigned int id,
>> + struct reset_control *rst)
>> +{
>> + struct tegra_mc *mc;
>> + int ret;
>> +
>> + mc = tegra_mc_find_device();
>> + if (!mc)
>> + return -ENODEV;
>> +
>> + if (id >= mc->soc->num_modules || !mc->soc->modules[id].valid)
>> + return -EINVAL;
>> +
>> + mutex_lock(&mc->lock);
>> + ret = tegra_mc_hot_reset_assert(mc, id, rst);
>> + mutex_unlock(&mc->lock);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(tegra_memory_client_hot_reset_assert);
>> +
>> +int tegra_memory_client_hot_reset_deassert(unsigned int id,
>> + struct reset_control *rst)
>> +{
>> + struct tegra_mc *mc;
>> + int ret;
>> +
>> + mc = tegra_mc_find_device();
>> + if (!mc)
>> + return -ENODEV;
>> +
>> + if (id >= mc->soc->num_modules || !mc->soc->modules[id].valid)
>> + return -EINVAL;
>
> There's a lot of repetition in the code here. If you look at my
> prototype, I think this is simpler to deal with if you get a reference
> to the reset and then just use it. All of the special case handling is
> done in the lookup function, and then you get back NULL or a valid
> pointer that you can immediately use.
>
>> +
>> + mutex_lock(&mc->lock);
>> + ret = tegra_mc_hot_reset_deassert(mc, id, rst);
>> + mutex_unlock(&mc->lock);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(tegra_memory_client_hot_reset_deassert);
>> +
>> static int tegra_mc_init(void)
>> {
>> return platform_driver_register(&tegra_mc_driver);
> [...]
>> diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
>> index 8b6360eabb8a..135012c74358 100644
>> --- a/drivers/memory/tegra/tegra124.c
>> +++ b/drivers/memory/tegra/tegra124.c
>> @@ -1012,6 +1012,30 @@ static const struct tegra_smmu_group_soc tegra124_groups[] = {
>> },
>> };
>>
>> +static const struct tegra_mc_module tegra124_mc_modules[] = {
>> + [TEGRA_MEMORY_CLIENT_AFI] = { .hw_id = 0, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_AVP] = { .hw_id = 1, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_DC] = { .hw_id = 2, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_DCB] = { .hw_id = 3, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_HOST1X] = { .hw_id = 6, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_HDA] = { .hw_id = 7, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_ISP2] = { .hw_id = 8, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_MPCORE] = { .hw_id = 9, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_MPCORELP] = { .hw_id = 10, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_MSENC] = { .hw_id = 11, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_PPCS] = { .hw_id = 14, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_SATA] = { .hw_id = 15, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_VDE] = { .hw_id = 16, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_VI] = { .hw_id = 17, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_VIC] = { .hw_id = 18, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_XUSB_HOST] = { .hw_id = 19, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_XUSB_DEV] = { .hw_id = 20, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_TSEC] = { .hw_id = 22, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_SDMMC1] = { .hw_id = 29, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_SDMMC2] = { .hw_id = 30, .valid = true },
>> + [TEGRA_MEMORY_CLIENT_SDMMC3] = { .hw_id = 31, .valid = true },
>> +};
>
> This list is incomplete. The same is true for any later generation.
> There are also quite a few holes in them. I think a better use of this
> space is to make the array compact and instead have more explicit
> information in the array.
>
>> diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
>> index 6cfc1dfa3a40..2d36db3ac659 100644
>> --- a/include/soc/tegra/mc.h
>> +++ b/include/soc/tegra/mc.h
>> @@ -9,11 +9,13 @@
>> #ifndef __SOC_TEGRA_MC_H__
>> #define __SOC_TEGRA_MC_H__
>>
>> +#include <linux/mutex.h>
>> #include <linux/types.h>
>>
>> struct clk;
>> struct device;
>> struct page;
>> +struct reset_control;
>>
>> struct tegra_smmu_enable {
>> unsigned int reg;
>> @@ -95,6 +97,11 @@ static inline void tegra_smmu_remove(struct tegra_smmu *smmu)
>> }
>> #endif
>>
>> +struct tegra_mc_module {
>> + unsigned int hw_id;
>> + bool valid;
>> +};
>> +
>> struct tegra_mc_soc {
>> const struct tegra_mc_client *clients;
>> unsigned int num_clients;
>> @@ -110,6 +117,13 @@ struct tegra_mc_soc {
>> const struct tegra_smmu_soc *smmu;
>>
>> bool tegra20;
>> +
>> + const struct tegra_mc_module *modules;
>> + unsigned int num_modules;
>> +
>> + u32 reg_client_ctrl;
>> + u32 reg_client_hotresetn;
>> + u32 reg_client_flush_status;
>
> That's not enough to cover all clients on Tegra124 and later. We need at
> least two registers. I'd also prefer to move away from the assumption
> that the ID is somehow linked to the bit position. The ID is in fact
> completely arbitrary and in this case only chosen to match the bit
> position.
>
> This has multiple disadvantages, some of which I've already listed. One
> of them is that we inevitably make arrays sparse as the SoC evolves, so
> we need to work around this in code and data tables using a valid flag.
> Checking for validity also becomes non-trivial and we move part of that
> burden into drivers.
>
> I think a much better and robust solution is to completely separate the
> ID from the hardware registers. This is implemented in my prototype on
> github. The ID is essentially only used as a way to identify the reset
> via device tree. The MC driver will use the ID to look up the reset in
> its per-SoC table and will only work with the reset object after that.
> The object itself contains all the information needed to program the
> registers.
>
> Currently the tables in that implementation don't take into account the
> per-client "outstanding requests" registers, but they could be easily
> extended to do so.
>
>> };
>>
>> struct tegra_mc {
>> @@ -124,9 +138,72 @@ struct tegra_mc {
>>
>> struct tegra_mc_timing *timings;
>> unsigned int num_timings;
>> +
>> + struct mutex lock;
>> };
>>
>> void tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
>> unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
>>
>> +#define TEGRA_MEMORY_CLIENT_AVP 0
>> +#define TEGRA_MEMORY_CLIENT_DC 1
>> +#define TEGRA_MEMORY_CLIENT_DCB 2
>> +#define TEGRA_MEMORY_CLIENT_EPP 3
>> +#define TEGRA_MEMORY_CLIENT_2D 4
>> +#define TEGRA_MEMORY_CLIENT_HOST1X 5
>> +#define TEGRA_MEMORY_CLIENT_ISP 6
>> +#define TEGRA_MEMORY_CLIENT_MPCORE 7
>> +#define TEGRA_MEMORY_CLIENT_MPCORELP 8
>> +#define TEGRA_MEMORY_CLIENT_MPEA 9
>> +#define TEGRA_MEMORY_CLIENT_MPEB 10
>> +#define TEGRA_MEMORY_CLIENT_MPEC 11
>> +#define TEGRA_MEMORY_CLIENT_3D 12
>> +#define TEGRA_MEMORY_CLIENT_3D1 13
>> +#define TEGRA_MEMORY_CLIENT_PPCS 14
>> +#define TEGRA_MEMORY_CLIENT_VDE 15
>> +#define TEGRA_MEMORY_CLIENT_VI 16
>> +#define TEGRA_MEMORY_CLIENT_AFI 17
>> +#define TEGRA_MEMORY_CLIENT_HDA 18
>> +#define TEGRA_MEMORY_CLIENT_SATA 19
>> +#define TEGRA_MEMORY_CLIENT_MSENC 20
>> +#define TEGRA_MEMORY_CLIENT_VIC 21
>> +#define TEGRA_MEMORY_CLIENT_XUSB_HOST 22
>> +#define TEGRA_MEMORY_CLIENT_XUSB_DEV 23
>> +#define TEGRA_MEMORY_CLIENT_TSEC 24
>> +#define TEGRA_MEMORY_CLIENT_SDMMC1 25
>> +#define TEGRA_MEMORY_CLIENT_SDMMC2 26
>> +#define TEGRA_MEMORY_CLIENT_SDMMC3 27
>> +#define TEGRA_MEMORY_CLIENT_MAX TEGRA_MEMORY_CLIENT_SDMMC3
>> +
>> +#define TEGRA_MEMORY_CLIENT_3D0 TEGRA_MEMORY_CLIENT_3D
>> +#define TEGRA_MEMORY_CLIENT_MPE TEGRA_MEMORY_CLIENT_MPEA
>> +#define TEGRA_MEMORY_CLIENT_NVENC TEGRA_MEMORY_CLIENT_MSENC
>> +#define TEGRA_MEMORY_CLIENT_ISP2 TEGRA_MEMORY_CLIENT_ISP
>> +
>> +#ifdef CONFIG_ARCH_TEGRA
>> +int tegra_memory_client_hot_reset(unsigned int id, struct reset_control *rst,
>> + unsigned long usecs);
>> +int tegra_memory_client_hot_reset_assert(unsigned int id,
>> + struct reset_control *rst);
>> +int tegra_memory_client_hot_reset_deassert(unsigned int id,
>> + struct reset_control *rst);
>> +#else
>
> I *really* don't want yet another custom API. We've had a number of
> these in the past and it's always been extremely painful to get rid of
> them. And we probably won't ever be able to completely get rid of the
> powergate API, which means additional headaches everytime we need to
> touch code in that area. I don't want to repeat that mistake.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] memory: tegra: Introduce memory client hot reset API
2018-02-19 12:35 ` Dmitry Osipenko
@ 2018-02-19 12:44 ` Dmitry Osipenko
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Osipenko @ 2018-02-19 12:44 UTC (permalink / raw)
To: Thierry Reding; +Cc: Jonathan Hunter, Philipp Zabel, linux-tegra, linux-kernel
On 19.02.2018 15:35, Dmitry Osipenko wrote:
> On 13.02.2018 14:24, Thierry Reding wrote:
>> On Mon, Feb 12, 2018 at 08:06:31PM +0300, Dmitry Osipenko wrote:
>>> In order to reset busy HW properly, memory controller needs to be
>>> involved, otherwise it possible to get corrupted memory if HW was reset
>>> during DMA. Introduce memory client 'hot reset' API that will be used
>>> for resetting busy HW. The primary users are memory clients related to
>>> video (decoder/encoder/camera) and graphics (2d/3d).
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>> drivers/memory/tegra/mc.c | 249 ++++++++++++++++++++++++++++++++++++++++
>>> drivers/memory/tegra/tegra114.c | 25 ++++
>>> drivers/memory/tegra/tegra124.c | 32 ++++++
>>> drivers/memory/tegra/tegra20.c | 23 ++++
>>> drivers/memory/tegra/tegra210.c | 27 +++++
>>> drivers/memory/tegra/tegra30.c | 25 ++++
>>> include/soc/tegra/mc.h | 77 +++++++++++++
>>> 7 files changed, 458 insertions(+)
>>
>> As discussed on IRC, I typed up a variant of this in an attempt to fix
>> an unrelated bug report. The code is here:
>>
>> https://github.com/thierryreding/linux/commit/e104e703cb75dfe742b2e051194a0af8ddefcd03
>>
>> I think we can make this without adding a custom API. The reset control
>> API should work just fine. The above version doesn't take into account
>> some of Tegra20's quirks, but I think it should still work for Tegra20
>> with just slightly different implementations for ->assert() and
>> ->deassert().
>>
>> A couple of more specific comments below.
>>
>>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>>> index 187a9005351b..9838f588d64d 100644
>>> --- a/drivers/memory/tegra/mc.c
>>> +++ b/drivers/memory/tegra/mc.c
>>> @@ -7,11 +7,13 @@
>>> */
>>>
>>> #include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> #include <linux/interrupt.h>
>>> #include <linux/kernel.h>
>>> #include <linux/module.h>
>>> #include <linux/of.h>
>>> #include <linux/platform_device.h>
>>> +#include <linux/reset.h>
>>> #include <linux/slab.h>
>>> #include <linux/sort.h>
>>>
>>> @@ -81,6 +83,172 @@ static const struct of_device_id tegra_mc_of_match[] = {
>>> };
>>> MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
>>>
>>> +static int terga_mc_flush_dma(struct tegra_mc *mc, unsigned int id)
>>> +{
>>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>>> + u32 value, reg_poll = mc->soc->reg_client_flush_status;
>>> + int retries = 3;
>>> +
>>> + value = mc_readl(mc, mc->soc->reg_client_ctrl);
>>> +
>>> + if (mc->soc->tegra20)
>>> + value &= ~BIT(hw_id);
>>> + else
>>> + value |= BIT(hw_id);
>>> +
>>> + /* block clients DMA requests */
>>> + mc_writel(mc, value, mc->soc->reg_client_ctrl);
>>> +
>>> + /* wait for completion of the outstanding DMA requests */
>>> + if (mc->soc->tegra20) {
>>> + while (mc_readl(mc, reg_poll + hw_id * sizeof(u32)) != 0) {
>>> + if (!retries--)
>>> + return -EBUSY;
>>> +
>>> + usleep_range(1000, 2000);
>>> + }
>>> + } else {
>>> + while ((mc_readl(mc, reg_poll) & BIT(hw_id)) == 0) {
>>> + if (!retries--)
>>> + return -EBUSY;
>>> +
>>> + usleep_range(1000, 2000);
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> I think this suffers from too much unification. The programming model is
>> too different to stash this into a single function implementation and as
>> a result this becomes very difficult to read. In my experience it's more
>> readable to split this into separate implementations and pass around
>> pointers to them.
>>
>>> +
>>> +static int terga_mc_unblock_dma(struct tegra_mc *mc, unsigned int id)
>>> +{
>>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>>> + u32 value;
>>> +
>>> + value = mc_readl(mc, mc->soc->reg_client_ctrl);
>>> +
>>> + if (mc->soc->tegra20)
>>> + value |= BIT(hw_id);
>>> + else
>>> + value &= ~BIT(hw_id);
>>> +
>>> + mc_writel(mc, value, mc->soc->reg_client_ctrl);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int terga_mc_hotreset_assert(struct tegra_mc *mc, unsigned int id)
>>> +{
>>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>>> + u32 value;
>>> +
>>> + if (mc->soc->tegra20) {
>>> + value = mc_readl(mc, mc->soc->reg_client_hotresetn);
>>> +
>>> + mc_writel(mc, value & ~BIT(hw_id),
>>> + mc->soc->reg_client_hotresetn);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int terga_mc_hotreset_deassert(struct tegra_mc *mc, unsigned int id)
>>> +{
>>> + unsigned int hw_id = mc->soc->modules[id].hw_id;
>>> + u32 value;
>>> +
>>> + if (mc->soc->tegra20) {
>>> + value = mc_readl(mc, mc->soc->reg_client_hotresetn);
>>> +
>>> + mc_writel(mc, value | BIT(hw_id),
>>> + mc->soc->reg_client_hotresetn);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> The same goes for these. I think we can do this much more easily by
>> providing reset controller API ->assert() and ->deassert()
>> implementations for Tegra20 and Tegra30+, and then register the reset
>> controller device using the ops stored in the MC SoC structure.
>>
>>> +static int tegra_mc_hot_reset_assert(struct tegra_mc *mc, unsigned int id,
>>> + struct reset_control *rst)
>>> +{
>>> + int err;
>>> +
>>> + /*
>>> + * Block clients DMA requests and wait for completion of the
>>> + * outstanding requests.
>>> + */
>>> + err = terga_mc_flush_dma(mc, id);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to flush DMA: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + /* put in reset HW that corresponds to the memory client */
>>> + err = reset_control_assert(rst);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to assert HW reset: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + /* clear the client requests sitting before arbitration */
>>> + err = terga_mc_hotreset_assert(mc, id);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to hot reset client: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> This is very strictly according to the TRM, but I don't see a reason why
>> you couldn't stash the DMA blocking and the hot reset into a ->assert()
>> implementation...
>>
>>> +
>>> +static int tegra_mc_hot_reset_deassert(struct tegra_mc *mc, unsigned int id,
>>> + struct reset_control *rst)
>>> +{
>>> + int err;
>>> +
>>> + /* take out client from hot reset */
>>> + err = terga_mc_hotreset_deassert(mc, id);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to deassert hot reset: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + /* take out from reset corresponding clients HW */
>>> + err = reset_control_deassert(rst);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to deassert HW reset: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + /* allow new DMA requests to proceed to arbitration */
>>> + err = terga_mc_unblock_dma(mc, id);
>>> + if (err) {
>>> + dev_err(mc->dev, "Failed to unblock client: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> ... and the hot reset deassertion and the DMA unblocking into a
>> ->deassert() implementation.
>>
>> I think the important part is that DMA is blocked and the requests are
>> cleared before the module reset, and similarily that the hot reset is
>> released and DMA is unblocked after the module reset.
>
> Okay, let's try the way you're suggesting and see if anything breaks..
Although, it would be awesome if you could ask somebody who is familiar with the
actual HW implementation. I can imagine that HW is simplified and expecting SW
to take that into account, there could be hazards.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-02-19 12:44 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-12 17:06 [PATCH v2 0/2] Memory controller hot reset Dmitry Osipenko
2018-02-12 17:06 ` [PATCH v2 1/2] memory: tegra: Squash tegra20-mc into common tegra-mc driver Dmitry Osipenko
2018-02-13 10:30 ` Thierry Reding
2018-02-14 11:15 ` Peter De Schrijver
2018-02-14 15:42 ` Dmitry Osipenko
2018-02-19 2:04 ` Dmitry Osipenko
2018-02-19 2:16 ` Dmitry Osipenko
2018-02-12 17:06 ` [PATCH v2 2/2] memory: tegra: Introduce memory client hot reset API Dmitry Osipenko
2018-02-13 11:24 ` Thierry Reding
2018-02-19 12:35 ` Dmitry Osipenko
2018-02-19 12:44 ` Dmitry Osipenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).