Re: [PATCH v4 09/15] memory: tegra: Squash tegra20-mc into common tegra-mc driver
On Mon, Apr 09, 2018 at 10:28:31PM +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 > in a preparation for the upcoming MC hot reset controls implementation, > avoiding code duplication. > > Signed-off-by: Dmitry Osipenko > --- > drivers/memory/Kconfig | 10 -- > drivers/memory/Makefile| 1 - > drivers/memory/tegra/Makefile | 1 + > drivers/memory/tegra/mc.c | 120 +-- > drivers/memory/tegra/mc.h | 11 ++ > drivers/memory/tegra/tegra20.c | 178 + > drivers/memory/tegra20-mc.c| 254 > - > include/soc/tegra/mc.h | 2 +- > 8 files changed, 299 insertions(+), 278 deletions(-) > create mode 100644 drivers/memory/tegra/tegra20.c > delete mode 100644 drivers/memory/tegra20-mc.c I've applied this now and squashed in patch "memory: tegra: Remove handling of MC_GART_ERROR_REQ on Tegra20". Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH v4 09/15] memory: tegra: Squash tegra20-mc into common tegra-mc driver
On 27.04.2018 13:24, Thierry Reding wrote:
> On Fri, Apr 27, 2018 at 01:13:47PM +0300, Dmitry Osipenko wrote:
>> On 27.04.2018 12:34, Thierry Reding wrote:
>>> On Mon, Apr 09, 2018 at 10:28:31PM +0300, Dmitry Osipenko wrote:
>>> [...]
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>>> [...]
+#define MC_GART_ERROR_REQ 0x30
+#define MC_DECERR_EMEM_OTHERS_STATUS 0x58
+#define MC_SECURITY_VIOLATION_STATUS 0x74
>>> [...]
diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
>>> [...]
@@ -21,19 +21,30 @@
#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)
static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
{
+ if (mc->regs2 && offset >= 0x24)
+ return readl(mc->regs2 + offset - 0x3c);
>>>
>>> I'm still not sure how this is supposed to work. If we pass in
>>> MC_GART_ERROR_REQ as offset into mc_readl(), then the condition above
>>> will be true (0x30 >= 0x24) but then the new offset will be computed
>>> and we end up with:
>>>
>>> return readl(mc->regs2 + 0x30 - 0x3c);
>>>
>>> which means we'll be adding a negative offset (or rather a very large
>>> offset because it will wrap around).
>>
>> Indeed! Thank you for pointing at it again, now I see the issue. It probably
>> works because actual registers mapping is aligned to page(?) size and adding
>> the
>> large offset with wraparound is equal to subtraction.
>>
>> That register belongs to the GART and we can't simply move interrupt
>> handling to
>> the GART driver because status register is within the MC in device tree. We
>> can
>> omit reading of MC_GART_ERROR_REQ and simply report GART page fault for the
>> starter and then reorganize drivers by making MC driver MFD and GART its
>> sub-device, what do you think?
>
> Sounds like a good idea. Can you send a fix on top of this that I can
> squash into this when applying?
Sure.
> As for integrating GART with MC, I'd prefer something that doesn't use
> MFD but rather does something similar to what we have for the SMMU. I
> think that's simpler to do and has less boilerplate. I think it's also
> warranted because the MC and GART are very tightly coupled, so an MFD
> would be slightly over-engineered, in my opinion.
Okay, I'll recap how SMMU is integrated with MC and then come up with something.
Re: [PATCH v4 09/15] memory: tegra: Squash tegra20-mc into common tegra-mc driver
On Fri, Apr 27, 2018 at 01:13:47PM +0300, Dmitry Osipenko wrote:
> On 27.04.2018 12:34, Thierry Reding wrote:
> > On Mon, Apr 09, 2018 at 10:28:31PM +0300, Dmitry Osipenko wrote:
> > [...]
> >> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> > [...]
> >> +#define MC_GART_ERROR_REQ 0x30
> >> +#define MC_DECERR_EMEM_OTHERS_STATUS 0x58
> >> +#define MC_SECURITY_VIOLATION_STATUS 0x74
> > [...]
> >> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
> > [...]
> >> @@ -21,19 +21,30 @@
> >> #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)
> >>
> >> static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
> >> {
> >> + if (mc->regs2 && offset >= 0x24)
> >> + return readl(mc->regs2 + offset - 0x3c);
> >
> > I'm still not sure how this is supposed to work. If we pass in
> > MC_GART_ERROR_REQ as offset into mc_readl(), then the condition above
> > will be true (0x30 >= 0x24) but then the new offset will be computed
> > and we end up with:
> >
> > return readl(mc->regs2 + 0x30 - 0x3c);
> >
> > which means we'll be adding a negative offset (or rather a very large
> > offset because it will wrap around).
>
> Indeed! Thank you for pointing at it again, now I see the issue. It probably
> works because actual registers mapping is aligned to page(?) size and adding
> the
> large offset with wraparound is equal to subtraction.
>
> That register belongs to the GART and we can't simply move interrupt handling
> to
> the GART driver because status register is within the MC in device tree. We
> can
> omit reading of MC_GART_ERROR_REQ and simply report GART page fault for the
> starter and then reorganize drivers by making MC driver MFD and GART its
> sub-device, what do you think?
Sounds like a good idea. Can you send a fix on top of this that I can
squash into this when applying?
As for integrating GART with MC, I'd prefer something that doesn't use
MFD but rather does something similar to what we have for the SMMU. I
think that's simpler to do and has less boilerplate. I think it's also
warranted because the MC and GART are very tightly coupled, so an MFD
would be slightly over-engineered, in my opinion.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v4 09/15] memory: tegra: Squash tegra20-mc into common tegra-mc driver
On 27.04.2018 12:34, Thierry Reding wrote:
> On Mon, Apr 09, 2018 at 10:28:31PM +0300, Dmitry Osipenko wrote:
> [...]
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> [...]
>> +#define MC_GART_ERROR_REQ 0x30
>> +#define MC_DECERR_EMEM_OTHERS_STATUS0x58
>> +#define MC_SECURITY_VIOLATION_STATUS0x74
> [...]
>> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
> [...]
>> @@ -21,19 +21,30 @@
>> #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)
>>
>> static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
>> {
>> +if (mc->regs2 && offset >= 0x24)
>> +return readl(mc->regs2 + offset - 0x3c);
>
> I'm still not sure how this is supposed to work. If we pass in
> MC_GART_ERROR_REQ as offset into mc_readl(), then the condition above
> will be true (0x30 >= 0x24) but then the new offset will be computed
> and we end up with:
>
> return readl(mc->regs2 + 0x30 - 0x3c);
>
> which means we'll be adding a negative offset (or rather a very large
> offset because it will wrap around).
Indeed! Thank you for pointing at it again, now I see the issue. It probably
works because actual registers mapping is aligned to page(?) size and adding the
large offset with wraparound is equal to subtraction.
That register belongs to the GART and we can't simply move interrupt handling to
the GART driver because status register is within the MC in device tree. We can
omit reading of MC_GART_ERROR_REQ and simply report GART page fault for the
starter and then reorganize drivers by making MC driver MFD and GART its
sub-device, what do you think?
Re: [PATCH v4 09/15] memory: tegra: Squash tegra20-mc into common tegra-mc driver
On Mon, Apr 09, 2018 at 10:28:31PM +0300, Dmitry Osipenko wrote:
[...]
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
[...]
> +#define MC_GART_ERROR_REQ0x30
> +#define MC_DECERR_EMEM_OTHERS_STATUS 0x58
> +#define MC_SECURITY_VIOLATION_STATUS 0x74
[...]
> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
[...]
> @@ -21,19 +21,30 @@
> #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)
>
> static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
> {
> + if (mc->regs2 && offset >= 0x24)
> + return readl(mc->regs2 + offset - 0x3c);
I'm still not sure how this is supposed to work. If we pass in
MC_GART_ERROR_REQ as offset into mc_readl(), then the condition above
will be true (0x30 >= 0x24) but then the new offset will be computed
and we end up with:
return readl(mc->regs2 + 0x30 - 0x3c);
which means we'll be adding a negative offset (or rather a very large
offset because it will wrap around).
Thierry
signature.asc
Description: PGP signature
[PATCH v4 09/15] memory: tegra: Squash tegra20-mc into common tegra-mc driver
Tegra30+ has some minor differences in registers / bits layout compared
to Tegra20. Let's squash Tegra20 driver into the common tegra-mc driver
in a preparation for the upcoming MC hot reset controls implementation,
avoiding code duplication.
Signed-off-by: Dmitry Osipenko
---
drivers/memory/Kconfig | 10 --
drivers/memory/Makefile| 1 -
drivers/memory/tegra/Makefile | 1 +
drivers/memory/tegra/mc.c | 120 +--
drivers/memory/tegra/mc.h | 11 ++
drivers/memory/tegra/tegra20.c | 178 +
drivers/memory/tegra20-mc.c| 254 -
include/soc/tegra/mc.h | 2 +-
8 files changed, 299 insertions(+), 278 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 60509f0a386b..5932ab33202a 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -37,6 +37,10 @@
#define MC_ERR_ADR 0x0c
+#define MC_GART_ERROR_REQ 0x30
+#define MC_DECERR_EMEM_OTHERS_STATUS 0x58
+#define MC_SECURITY_VIOLATION_STATUS 0x74
+
#define MC_EMEM_ARB_CFG 0x90
#define MC_EMEM_ARB_CFG_CYCLES_PER_UPDATE(x) (((x) & 0x1ff) << 0)
#define MC_EMEM_ARB_CFG_CYCLES_PER_UPDATE_MASK0x1ff
@@ -46,6 +50,9 @@
#define MC_EMEM_ADR_CFG_EMEM_NUMDEV BIT(0)
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
@@ -221,6 +228,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",
@@ -334,11 +342,85 @@ static irqreturn_t tegra_mc_irq(int irq, void *data)
return IRQ_HANDLED;
}
+static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data)
+{
+ struct tegra_mc *mc = data;
+ unsigned long status;
+ unsigned int bit;
+
+ /* mask all interrupts to avoid flooding */
+ status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
+ if (!status)
+ return IRQ_NONE;
+
+ for_each_set_bit(bit, &status, 32) {
+ const char *direction = "read", *secure = "";
+ const char *error = status_names[bit];
+ const char *client, *desc;
+ phys_addr_t addr;
+ u32 value, reg;
+ u8 id, type;
+
+ switch (BIT(bit)) {
+ case MC_INT_DECERR_EMEM:
+ reg = MC_DECERR_EMEM_OTHERS_STATUS;
+ value = mc_readl(mc, reg);
+
+ id = value & mc->soc->client_id_mask;
+ desc = error_names[2];
+
+ if (value &
