| Message ID | 1641926750-27544-2-git-send-email-amhetre@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | memory: tegra: Update mc interrupts | expand |
On 11/01/2022 19:45, Ashish Mhetre wrote: > Implement new structure for function related to mc interrupts. s/mc/MC/ > Move handle_irq into this structure. > Add support for clearing interrupts. The subject says you are adding support for MC interrupts, so before they were not supported at all? Here you also mention clearing of interrupts - another new feature. One commit for refactoring (adding new structure) which does not change functionality, second commit for adding new feature. Different question - why do you need new structure for just two function pointers? Why these different IRQ handling functions cannot be in tegra_mc_ops? To me, it's unnecessary code complexity (plus performance impact, but it's not that important). If this is really, really needed, please describe the rationale in the commit message. > > Signed-off-by: Ashish Mhetre <amhetre@nvidia.com> > --- > drivers/memory/tegra/mc.c | 14 +++++++++++--- > drivers/memory/tegra/mc.h | 1 + > drivers/memory/tegra/tegra114.c | 1 + > drivers/memory/tegra/tegra124.c | 2 ++ > drivers/memory/tegra/tegra186.c | 14 ++++++++++++++ > drivers/memory/tegra/tegra194.c | 12 ++++++++++++ > drivers/memory/tegra/tegra20.c | 6 +++++- > drivers/memory/tegra/tegra210.c | 1 + > drivers/memory/tegra/tegra30.c | 1 + > include/soc/tegra/mc.h | 7 ++++++- > 10 files changed, 54 insertions(+), 5 deletions(-) > Best regards, Krzysztof
11.01.2022 21:45, Ashish Mhetre пишет: > > @@ -765,16 +768,21 @@ static int tegra_mc_probe(struct platform_device *pdev) > return err; > } > > - if (mc->soc->ops && mc->soc->ops->handle_irq) { > + if (mc->soc->interrupt_ops && mc->soc->interrupt_ops->handle_irq) { > mc->irq = platform_get_irq(pdev, 0); > if (mc->irq < 0) > return mc->irq; > > WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n"); > > + /* clear any mc-errs that occurred before. */ s/mc-errs/Memory Controller errors/ > + if (mc->soc->interrupt_ops->clear_interrupt) > + mc->soc->interrupt_ops->clear_interrupt(mc); There is no explanation of this change neither in the code, nor in the commit message. Please always provide detailed descriptions for a non-trivial changes. Interrupts aren't cleared intentionally by the driver, otherwise you'll never know about early-boot memory faults which happened before the probe. Hence this change is incorrect.
On 1/12/2022 1:59 AM, Krzysztof Kozlowski wrote: > External email: Use caution opening links or attachments > > > On 11/01/2022 19:45, Ashish Mhetre wrote: >> Implement new structure for function related to mc interrupts. > > s/mc/MC/ > Okay, I'll update these. >> Move handle_irq into this structure. >> Add support for clearing interrupts. > > The subject says you are adding support for MC interrupts, so before > they were not supported at all? > Interrupt handling and error logging is not supported from Tegra186 onward. So the patches are for adding supported interrupts and logging them from Tegra186 onward. But you are right, subject/commit message is misleading. I'll update it in next version. > Here you also mention clearing of interrupts - another new feature. One > commit for refactoring (adding new structure) which does not change > functionality, second commit for adding new feature. > > Different question - why do you need new structure for just two function > pointers? Why these different IRQ handling functions cannot be in > tegra_mc_ops? To me, it's unnecessary code complexity (plus performance > impact, but it's not that important). If this is really, really needed, > please describe the rationale in the commit message. > clearing_interrupts() won't be needed. As pointed by Dmitry, we should be logging early boot MC interrupts as well instead of clearing them. Also, I'll keep handling irq inside tegra_mc_ops instead of adding new structure. >> >> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com> >> --- >> drivers/memory/tegra/mc.c | 14 +++++++++++--- >> drivers/memory/tegra/mc.h | 1 + >> drivers/memory/tegra/tegra114.c | 1 + >> drivers/memory/tegra/tegra124.c | 2 ++ >> drivers/memory/tegra/tegra186.c | 14 ++++++++++++++ >> drivers/memory/tegra/tegra194.c | 12 ++++++++++++ >> drivers/memory/tegra/tegra20.c | 6 +++++- >> drivers/memory/tegra/tegra210.c | 1 + >> drivers/memory/tegra/tegra30.c | 1 + >> include/soc/tegra/mc.h | 7 ++++++- >> 10 files changed, 54 insertions(+), 5 deletions(-) >> > > > Best regards, > Krzysztof
On 1/12/2022 1:43 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 11.01.2022 21:45, Ashish Mhetre пишет: >> >> @@ -765,16 +768,21 @@ static int tegra_mc_probe(struct platform_device *pdev) >> return err; >> } >> >> - if (mc->soc->ops && mc->soc->ops->handle_irq) { >> + if (mc->soc->interrupt_ops && mc->soc->interrupt_ops->handle_irq) { >> mc->irq = platform_get_irq(pdev, 0); >> if (mc->irq < 0) >> return mc->irq; >> >> WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n"); >> >> + /* clear any mc-errs that occurred before. */ > > s/mc-errs/Memory Controller errors/ > Sure, I'll update these in next version. >> + if (mc->soc->interrupt_ops->clear_interrupt) >> + mc->soc->interrupt_ops->clear_interrupt(mc); > > There is no explanation of this change neither in the code, nor in the > commit message. Please always provide detailed descriptions for a > non-trivial changes. > > Interrupts aren't cleared intentionally by the driver, otherwise you'll > never know about early-boot memory faults which happened before the > probe. Hence this change is incorrect. That's true, we should be logging early-boot memory faults as well. Ideally there shouldn't be any early-boot faults as all clients will be up after MC, right? But I agree that we should be checking and logging if any interrupt is present.
19.01.2022 11:47, Ashish Mhetre пишет: > > > On 1/12/2022 1:43 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 11.01.2022 21:45, Ashish Mhetre пишет: >>> >>> @@ -765,16 +768,21 @@ static int tegra_mc_probe(struct >>> platform_device *pdev) >>> return err; >>> } >>> >>> - if (mc->soc->ops && mc->soc->ops->handle_irq) { >>> + if (mc->soc->interrupt_ops && >>> mc->soc->interrupt_ops->handle_irq) { >>> mc->irq = platform_get_irq(pdev, 0); >>> if (mc->irq < 0) >>> return mc->irq; >>> >>> WARN(!mc->soc->client_id_mask, "missing client ID mask >>> for this SoC\n"); >>> >>> + /* clear any mc-errs that occurred before. */ >> >> s/mc-errs/Memory Controller errors/ >> > Sure, I'll update these in next version. >>> + if (mc->soc->interrupt_ops->clear_interrupt) >>> + mc->soc->interrupt_ops->clear_interrupt(mc); >> >> There is no explanation of this change neither in the code, nor in the >> commit message. Please always provide detailed descriptions for a >> non-trivial changes. >> >> Interrupts aren't cleared intentionally by the driver, otherwise you'll >> never know about early-boot memory faults which happened before the >> probe. Hence this change is incorrect. > That's true, we should be logging early-boot memory faults as well. > Ideally there shouldn't be any early-boot faults as all clients will > be up after MC, right? But I agree that we should be checking and > logging if any interrupt is present. Normally there won't be any errors during early boot, otherwise they need to be fixed.
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c index 3c5aae7..3b3f052 100644 --- a/drivers/memory/tegra/mc.c +++ b/drivers/memory/tegra/mc.c @@ -604,9 +604,12 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data) return IRQ_HANDLED; } +const struct tegra_mc_interrupt_ops tegra30_mc_interrupt_ops = { + .handle_irq = tegra30_mc_handle_irq, +}; + const struct tegra_mc_ops tegra30_mc_ops = { .probe = tegra30_mc_probe, - .handle_irq = tegra30_mc_handle_irq, }; #endif @@ -765,16 +768,21 @@ static int tegra_mc_probe(struct platform_device *pdev) return err; } - if (mc->soc->ops && mc->soc->ops->handle_irq) { + if (mc->soc->interrupt_ops && mc->soc->interrupt_ops->handle_irq) { mc->irq = platform_get_irq(pdev, 0); if (mc->irq < 0) return mc->irq; WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n"); + /* clear any mc-errs that occurred before. */ + if (mc->soc->interrupt_ops->clear_interrupt) + mc->soc->interrupt_ops->clear_interrupt(mc); + mc_writel(mc, mc->soc->intmask, MC_INTMASK); - err = devm_request_irq(&pdev->dev, mc->irq, mc->soc->ops->handle_irq, 0, + err = devm_request_irq(&pdev->dev, mc->irq, + mc->soc->interrupt_ops->handle_irq, 0, dev_name(&pdev->dev), mc); if (err < 0) { dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n", mc->irq, diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h index 1e49298..f1fd457 100644 --- a/drivers/memory/tegra/mc.h +++ b/drivers/memory/tegra/mc.h @@ -144,6 +144,7 @@ extern const struct tegra_mc_soc tegra194_mc_soc; defined(CONFIG_ARCH_TEGRA_210_SOC) int tegra30_mc_probe(struct tegra_mc *mc); extern const struct tegra_mc_ops tegra30_mc_ops; +extern const struct tegra_mc_interrupt_ops tegra30_mc_interrupt_ops; #endif #if defined(CONFIG_ARCH_TEGRA_186_SOC) || \ diff --git a/drivers/memory/tegra/tegra114.c b/drivers/memory/tegra/tegra114.c index 4135057..f7b8dd9 100644 --- a/drivers/memory/tegra/tegra114.c +++ b/drivers/memory/tegra/tegra114.c @@ -1114,4 +1114,5 @@ const struct tegra_mc_soc tegra114_mc_soc = { .resets = tegra114_mc_resets, .num_resets = ARRAY_SIZE(tegra114_mc_resets), .ops = &tegra30_mc_ops, + .interrupt_ops = &tegra30_mc_interrupt_ops, }; diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c index d780a84..8b704c1 100644 --- a/drivers/memory/tegra/tegra124.c +++ b/drivers/memory/tegra/tegra124.c @@ -1275,6 +1275,7 @@ const struct tegra_mc_soc tegra124_mc_soc = { .num_resets = ARRAY_SIZE(tegra124_mc_resets), .icc_ops = &tegra124_mc_icc_ops, .ops = &tegra30_mc_ops, + .interrupt_ops = &tegra30_mc_interrupt_ops, }; #endif /* CONFIG_ARCH_TEGRA_124_SOC */ @@ -1307,5 +1308,6 @@ const struct tegra_mc_soc tegra132_mc_soc = { .num_resets = ARRAY_SIZE(tegra124_mc_resets), .icc_ops = &tegra124_mc_icc_ops, .ops = &tegra30_mc_ops, + .interrupt_ops = &tegra30_mc_interrupt_ops, }; #endif /* CONFIG_ARCH_TEGRA_132_SOC */ diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c index e65eac5..b548b6a 100644 --- a/drivers/memory/tegra/tegra186.c +++ b/drivers/memory/tegra/tegra186.c @@ -12,6 +12,8 @@ #include <soc/tegra/mc.h> +#include "mc.h" + #if defined(CONFIG_ARCH_TEGRA_186_SOC) #include <dt-bindings/memory/tegra186-mc.h> #endif @@ -20,6 +22,8 @@ #define MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED BIT(16) #define MC_SID_STREAMID_SECURITY_OVERRIDE BIT(8) +#define MC_INTSTATUS_CLEAR 0x00033340 + static void tegra186_mc_program_sid(struct tegra_mc *mc) { unsigned int i; @@ -137,6 +141,15 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) return 0; } +static void tegra186_mc_clear_interrupt(struct tegra_mc *mc) +{ + mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS); +} + +const struct tegra_mc_interrupt_ops tegra186_mc_interrupt_ops = { + .clear_interrupt = tegra186_mc_clear_interrupt, +}; + const struct tegra_mc_ops tegra186_mc_ops = { .probe = tegra186_mc_probe, .remove = tegra186_mc_remove, @@ -874,5 +887,6 @@ const struct tegra_mc_soc tegra186_mc_soc = { .clients = tegra186_mc_clients, .num_address_bits = 40, .ops = &tegra186_mc_ops, + .interrupt_ops = &tegra186_mc_interrupt_ops, }; #endif diff --git a/drivers/memory/tegra/tegra194.c b/drivers/memory/tegra/tegra194.c index cab998b..19f135f 100644 --- a/drivers/memory/tegra/tegra194.c +++ b/drivers/memory/tegra/tegra194.c @@ -9,6 +9,17 @@ #include "mc.h" +#define MC_INTSTATUS_CLEAR 0x00133340 + +static void tegra194_mc_clear_interrupt(struct tegra_mc *mc) +{ + mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS); +} + +const struct tegra_mc_interrupt_ops tegra194_mc_interrupt_ops = { + .clear_interrupt = tegra194_mc_clear_interrupt, +}; + static const struct tegra_mc_client tegra194_mc_clients[] = { { .id = TEGRA194_MEMORY_CLIENT_PTCR, @@ -1348,4 +1359,5 @@ const struct tegra_mc_soc tegra194_mc_soc = { .clients = tegra194_mc_clients, .num_address_bits = 40, .ops = &tegra186_mc_ops, + .interrupt_ops = &tegra194_mc_interrupt_ops, }; diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c index fcd7738..3bcb2ca 100644 --- a/drivers/memory/tegra/tegra20.c +++ b/drivers/memory/tegra/tegra20.c @@ -786,11 +786,15 @@ static irqreturn_t tegra20_mc_handle_irq(int irq, void *data) return IRQ_HANDLED; } +static const struct tegra_mc_interrupt_ops tegra20_mc_interrupt_ops = { + .handle_irq = tegra20_mc_handle_irq, +}; + static const struct tegra_mc_ops tegra20_mc_ops = { .probe = tegra20_mc_probe, .suspend = tegra20_mc_suspend, .resume = tegra20_mc_resume, - .handle_irq = tegra20_mc_handle_irq, + .interrupt_ops = tegra20_mc_interrupt_ops, }; const struct tegra_mc_soc tegra20_mc_soc = { diff --git a/drivers/memory/tegra/tegra210.c b/drivers/memory/tegra/tegra210.c index 8ab6498..d7ed163 100644 --- a/drivers/memory/tegra/tegra210.c +++ b/drivers/memory/tegra/tegra210.c @@ -1287,4 +1287,5 @@ const struct tegra_mc_soc tegra210_mc_soc = { .resets = tegra210_mc_resets, .num_resets = ARRAY_SIZE(tegra210_mc_resets), .ops = &tegra30_mc_ops, + .interrupt_ops = &tegra30_mc_interrupt_ops, }; diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c index 8431635..bb5ff68 100644 --- a/drivers/memory/tegra/tegra30.c +++ b/drivers/memory/tegra/tegra30.c @@ -1400,4 +1400,5 @@ const struct tegra_mc_soc tegra30_mc_soc = { .num_resets = ARRAY_SIZE(tegra30_mc_resets), .icc_ops = &tegra30_mc_icc_ops, .ops = &tegra30_mc_ops, + .interrupt_ops = &tegra30_mc_interrupt_ops, }; diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h index 1066b11..debc47b 100644 --- a/include/soc/tegra/mc.h +++ b/include/soc/tegra/mc.h @@ -170,6 +170,11 @@ struct tegra_mc_icc_ops { void *data); }; +struct tegra_mc_interrupt_ops { + void (*clear_interrupt)(struct tegra_mc *mc); + irqreturn_t (*handle_irq)(int irq, void *data); +}; + struct tegra_mc_ops { /* * @probe: Callback to set up SoC-specific bits of the memory controller. This is called @@ -179,7 +184,6 @@ struct tegra_mc_ops { void (*remove)(struct tegra_mc *mc); int (*suspend)(struct tegra_mc *mc); int (*resume)(struct tegra_mc *mc); - irqreturn_t (*handle_irq)(int irq, void *data); int (*probe_device)(struct tegra_mc *mc, struct device *dev); }; @@ -205,6 +209,7 @@ struct tegra_mc_soc { const struct tegra_mc_icc_ops *icc_ops; const struct tegra_mc_ops *ops; + const struct tegra_mc_interrupt_ops *interrupt_ops; }; struct tegra_mc {
Implement new structure for function related to mc interrupts. Move handle_irq into this structure. Add support for clearing interrupts. Signed-off-by: Ashish Mhetre <amhetre@nvidia.com> --- drivers/memory/tegra/mc.c | 14 +++++++++++--- drivers/memory/tegra/mc.h | 1 + drivers/memory/tegra/tegra114.c | 1 + drivers/memory/tegra/tegra124.c | 2 ++ drivers/memory/tegra/tegra186.c | 14 ++++++++++++++ drivers/memory/tegra/tegra194.c | 12 ++++++++++++ drivers/memory/tegra/tegra20.c | 6 +++++- drivers/memory/tegra/tegra210.c | 1 + drivers/memory/tegra/tegra30.c | 1 + include/soc/tegra/mc.h | 7 ++++++- 10 files changed, 54 insertions(+), 5 deletions(-)