Hi,
Here's two simple bugfixes for the Tegra memory controllers.
Tuomas Tynkkynen (2):
memory: tegra20-mc: Fix hang in IRQ handler.
memory: tegra30-mc: Fix IRQ handler.
drivers/memory/tegra20-mc.c | 5 ++++-
drivers/memory/tegra30-mc.c | 9 ++++++---
2 files changed, 10 insertions(+), 4 deletions(-)
--
1.8.1.5
In Tegra20 memory controller any MC interrupt would cause an
infinite loop in the IRQ handler.
Signed-off-by: Tuomas Tynkkynen <[email protected]>
---
drivers/memory/tegra20-mc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/memory/tegra20-mc.c b/drivers/memory/tegra20-mc.c
index 2ca5f28..bd5a553 100644
--- a/drivers/memory/tegra20-mc.c
+++ b/drivers/memory/tegra20-mc.c
@@ -193,8 +193,11 @@ static irqreturn_t tegra20_mc_isr(int irq, void *data)
mask &= stat;
if (!mask)
return IRQ_NONE;
- while ((bit = ffs(mask)) != 0)
+ while ((bit = ffs(mask)) != 0) {
tegra20_mc_decode(mc, bit - 1);
+ mask &= BIT(bit);
+ }
+
mc_writel(mc, stat, MC_INTSTATUS);
return IRQ_HANDLED;
}
--
1.8.1.5
In Tegra30 memory controller any MC interrupt would cause an infinite loop in
the IRQ handler. Additionally, a garbage pointer was used to read the MC
status registers, which causes wrong values to be printed if a MC error
occurred.
Signed-off-by: Tuomas Tynkkynen <[email protected]>
---
drivers/memory/tegra30-mc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/memory/tegra30-mc.c b/drivers/memory/tegra30-mc.c
index f4ae074..a6a05a2 100644
--- a/drivers/memory/tegra30-mc.c
+++ b/drivers/memory/tegra30-mc.c
@@ -218,7 +218,7 @@ static void tegra30_mc_decode(struct tegra30_mc *mc, int n)
return;
}
- err = readl(mc + MC_ERR_STATUS);
+ err = mc_readl(mc, MC_ERR_STATUS);
type = (err & MC_ERR_TYPE_MASK) >> MC_ERR_TYPE_SHIFT;
perm = (err & MC_ERR_INVALID_SMMU_PAGE_MASK) >>
@@ -235,7 +235,7 @@ static void tegra30_mc_decode(struct tegra30_mc *mc, int n)
if (cid < ARRAY_SIZE(tegra30_mc_client))
client = tegra30_mc_client[cid];
- addr = readl(mc + MC_ERR_ADR);
+ addr = mc_readl(mc, MC_ERR_ADR);
dev_err_ratelimited(mc->dev, "%s (0x%08x): 0x%08x %s (%s %s %s %s)\n",
mc_int_err[idx], err, addr, client,
@@ -313,8 +313,11 @@ static irqreturn_t tegra30_mc_isr(int irq, void *data)
mask &= stat;
if (!mask)
return IRQ_NONE;
- while ((bit = ffs(mask)) != 0)
+ while ((bit = ffs(mask)) != 0) {
tegra30_mc_decode(mc, bit - 1);
+ mask &= BIT(bit);
+ }
+
mc_writel(mc, stat, MC_INTSTATUS);
return IRQ_HANDLED;
}
--
1.8.1.5
On Mon, Jun 10, 2013 at 12:13:43PM +0300, Tuomas Tynkkynen wrote:
> In Tegra20 memory controller any MC interrupt would cause an
> infinite loop in the IRQ handler.
>
> Signed-off-by: Tuomas Tynkkynen <[email protected]>
> ---
> drivers/memory/tegra20-mc.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/memory/tegra20-mc.c b/drivers/memory/tegra20-mc.c
> index 2ca5f28..bd5a553 100644
> --- a/drivers/memory/tegra20-mc.c
> +++ b/drivers/memory/tegra20-mc.c
> @@ -193,8 +193,11 @@ static irqreturn_t tegra20_mc_isr(int irq, void *data)
> mask &= stat;
> if (!mask)
> return IRQ_NONE;
> - while ((bit = ffs(mask)) != 0)
> + while ((bit = ffs(mask)) != 0) {
> tegra20_mc_decode(mc, bit - 1);
> + mask &= BIT(bit);
Shouldn't this be "mask &= ~BIT(bit);"? The intent of the code is to
clear the bit which was handled by the loop body, right? The above
clears all other bits instead.
Thierry
On Mon, Jun 10, 2013 at 12:13:44PM +0300, Tuomas Tynkkynen wrote:
[...]
> diff --git a/drivers/memory/tegra30-mc.c b/drivers/memory/tegra30-mc.c
[...]
> @@ -313,8 +313,11 @@ static irqreturn_t tegra30_mc_isr(int irq, void *data)
> mask &= stat;
> if (!mask)
> return IRQ_NONE;
> - while ((bit = ffs(mask)) != 0)
> + while ((bit = ffs(mask)) != 0) {
> tegra30_mc_decode(mc, bit - 1);
> + mask &= BIT(bit);
Same comment as for patch 1/2.
Thierry
On 06/10/2013 11:36 PM, Thierry Reding wrote:
> On Mon, Jun 10, 2013 at 12:13:43PM +0300, Tuomas Tynkkynen wrote:
>> In Tegra20 memory controller any MC interrupt would cause an
>> infinite loop in the IRQ handler.
>>
>> Signed-off-by: Tuomas Tynkkynen <[email protected]>
>> ---
>> drivers/memory/tegra20-mc.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/memory/tegra20-mc.c b/drivers/memory/tegra20-mc.c
>> index 2ca5f28..bd5a553 100644
>> --- a/drivers/memory/tegra20-mc.c
>> +++ b/drivers/memory/tegra20-mc.c
>> @@ -193,8 +193,11 @@ static irqreturn_t tegra20_mc_isr(int irq, void *data)
>> mask &= stat;
>> if (!mask)
>> return IRQ_NONE;
>> - while ((bit = ffs(mask)) != 0)
>> + while ((bit = ffs(mask)) != 0) {
>> tegra20_mc_decode(mc, bit - 1);
>> + mask &= BIT(bit);
>
> Shouldn't this be "mask &= ~BIT(bit);"? The intent of the code is to
> clear the bit which was handled by the loop body, right? The above
> clears all other bits instead.
>
> Thierry
Whoops, yes it should be clearing just one bit. And since ffs() returned
a one-based bit-index, it seemed to work in practice.
I'll fix those & resend.
- Tuomas