irqchip/tegra: Remove everything related to COP
Subsystems (2)
PATCH 1/2 irqchip/tegra: Remove everything related to COP
There is no point in touching of the COP (ARM7TDMI auxiliary boot/firmware CPU) because COP's interrupts should be related only to an old multimedia firmware that is not applicable to the upstream kernel. Hence let's remove everything related to the COP, for consistency.
--- drivers/irqchip/irq-tegra.c | 17 ----------------- 1 file changed, 17 deletions(-)
drivers/irqchip/irq-tegra.c +0 -17
@@ -33,11 +33,6 @@#define ICTLR_CPU_IER_CLR 0x28#define ICTLR_CPU_IEP_CLASS 0x2C -#define ICTLR_COP_IER 0x30 -#define ICTLR_COP_IER_SET 0x34 -#define ICTLR_COP_IER_CLR 0x38 -#define ICTLR_COP_IEP_CLASS 0x3c -#define TEGRA_MAX_NUM_ICTLRS 6static unsigned int num_ictlrs;
@@ -68,8 +63,6 @@ static const struct of_device_id ictlr_matches[] = {struct tegra_ictlr_info {void __iomem *base[TEGRA_MAX_NUM_ICTLRS ];#ifdef CONFIG_PM_SLEEP -u32 cop_ier[TEGRA_MAX_NUM_ICTLRS ]; -u32 cop_iep[TEGRA_MAX_NUM_ICTLRS ];u32 cpu_ier[TEGRA_MAX_NUM_ICTLRS ];u32 cpu_iep[TEGRA_MAX_NUM_ICTLRS ];
@@ -144,11 +137,6 @@ static int tegra_ictlr_suspend(void)/* Save interrupt state */ lic->cpu_ier[i] =readl_relaxed (ictlr +ICTLR_CPU_IER ); lic->cpu_iep[i] =readl_relaxed (ictlr +ICTLR_CPU_IEP_CLASS ); - lic->cop_ier[i] =readl_relaxed (ictlr +ICTLR_COP_IER ); - lic->cop_iep[i] =readl_relaxed (ictlr +ICTLR_COP_IEP_CLASS ); - -/* Disable COP interrupts */ -writel_relaxed (~0ul , ictlr +ICTLR_COP_IER_CLR );/* Disable CPU interrupts */ writel_relaxed (~0ul , ictlr +ICTLR_CPU_IER_CLR );
@@ -175,11 +163,6 @@ static void tegra_ictlr_resume(void)writel_relaxed (~0ul , ictlr +ICTLR_CPU_IER_CLR );writel_relaxed (lic->cpu_ier[i], ictlr +ICTLR_CPU_IER_SET ); -writel_relaxed (lic->cop_iep[i], - ictlr +ICTLR_COP_IEP_CLASS ); -writel_relaxed (~0ul , ictlr +ICTLR_COP_IER_CLR ); -writel_relaxed (lic->cop_ier[i], - ictlr +ICTLR_COP_IER_SET ); }local_irq_restore (flags); }
Signature · 1 line
2.22.0
PATCH 2/2 irqchip/tegra: Clean up coding style
Make coding style to conform to the kernel's standard by fixing checkpatch warnings about "line over 80 characters".
--- drivers/irqchip/irq-tegra.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
drivers/irqchip/irq-tegra.c +5 -10
@@ -74,7 +74,7 @@ static struct tegra_ictlr_info *lic;static inline void tegra_ictlr_write_mask (struct irq_data *d,unsigned long reg) { -void __iomem *base = (void __iomem __force *)d->chip_data; +void __iomem *base = lic->base[d->hwirq /32 ];u32 mask; mask =BIT (d->hwirq %32 );
@@ -142,7 +142,8 @@ static int tegra_ictlr_suspend(void)writel_relaxed (~0ul , ictlr +ICTLR_CPU_IER_CLR );/* Enable the wakeup sources of ictlr */ -writel_relaxed (lic->ictlr_wake_mask[i], ictlr +ICTLR_CPU_IER_SET ); +writel_relaxed (lic->ictlr_wake_mask[i], + ictlr +ICTLR_CPU_IER_SET ); }local_irq_restore (flags);
@@ -222,7 +223,6 @@ static int tegra_ictlr_domain_alloc(struct irq_domain *domain, {struct irq_fwspec *fwspec = data;struct irq_fwspec parent_fwspec; -struct tegra_ictlr_info *info = domain->host_data;irq_hw_number_t hwirq;unsigned int i;
@@ -235,13 +235,9 @@ static int tegra_ictlr_domain_alloc(struct irq_domain *domain,if (hwirq >= (num_ictlrs *32 ))return -EINVAL ; -for (i =0 ; i < nr_irqs; i++) { -int ictlr = (hwirq + i) /32 ; - +for (i =0 ; i < nr_irqs; i++)irq_domain_set_hwirq_and_chip (domain, virq + i, hwirq + i, - &tegra_ictlr_chip, - (void __force *)info->base[ictlr]); - } + &tegra_ictlr_chip,NULL ); parent_fwspec = *fwspec; parent_fwspec.fwnode = domain->parent->fwnode;
@@ -312,7 +308,6 @@ static int __init tegra_ictlr_init(struct device_node *node,"%pOF: Found %u interrupt controllers in DT; expected %u.\n" , node, num_ictlrs, soc->num_ictlrs); - domain =irq_domain_add_hierarchy (parent_domain,0 , num_ictlrs *32 , node, &tegra_ictlr_domain_ops, lic);
Signature · 1 line
2.22.0
Re: [PATCH v1 2/2] irqchip/tegra: Clean up coding style
Quoted 3 lines · Dmitry Osipenko
Make coding style to conform to the kernel's standard by fixing checkpatch warnings about "line over 80 characters".
The last time I used a VT100 was about 30 years ago. I still think this was one of the most brilliant piece of equipment DEC ever produced, but I replaced it at the time with a Wyse 50 that had a 132 column mode. But even then, I could make my XTerm as wide as I wanted, and things haven't regressed much since.
More seriously, I don't consider the 80 column limit a hard one, and I'm pretty happy with code that spans more that 80 columns if that allows to read an expression without messing with the flow.
Quoted 11 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/irqchip/irq-tegra.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)drivers/irqchip/irq-tegra.c +0
-0
drivers/irqchip/irq-tegra.c static struct tegra_ictlr_info *lic; · line 77
Quoted 5 lines
drivers/irqchip/irq-tegra.c +1
-1@@ -74,7 +74,7 @@ static struct tegra_ictlr_info *lic;static inline void tegra_ictlr_write_mask (struct irq_data *d,unsigned long reg) { -void __iomem *base = (void __iomem __force *)d->chip_data; +void __iomem *base = lic->base[d->hwirq /32 ];
(1) This is an undocumented change
(2) Why do you think that moving from a per-interrupt base that is known at setup time to something that has to be recomputed on each and every access is a good thing?
Thanks,
M.
Quoted 48 lines
drivers/irqchip/irq-tegra.c +4
-9u32 mask; mask =BIT (d->hwirq %32 );@@ -142,7 +142,8 @@ static int tegra_ictlr_suspend(void)writel_relaxed (~0ul , ictlr +ICTLR_CPU_IER_CLR );/* Enable the wakeup sources of ictlr */ -writel_relaxed (lic->ictlr_wake_mask[i], ictlr +ICTLR_CPU_IER_SET ); +writel_relaxed (lic->ictlr_wake_mask[i], + ictlr +ICTLR_CPU_IER_SET ); }local_irq_restore (flags);@@ -222,7 +223,6 @@ static int tegra_ictlr_domain_alloc(struct irq_domain *domain, {struct irq_fwspec *fwspec = data;struct irq_fwspec parent_fwspec; -struct tegra_ictlr_info *info = domain->host_data;irq_hw_number_t hwirq;unsigned int i;@@ -235,13 +235,9 @@ static int tegra_ictlr_domain_alloc(struct irq_domain *domain,if (hwirq >= (num_ictlrs *32 ))return -EINVAL ; -for (i =0 ; i < nr_irqs; i++) { -int ictlr = (hwirq + i) /32 ; - +for (i =0 ; i < nr_irqs; i++)irq_domain_set_hwirq_and_chip (domain, virq + i, hwirq + i, - &tegra_ictlr_chip, - (void __force *)info->base[ictlr]); - } + &tegra_ictlr_chip,NULL ); parent_fwspec = *fwspec; parent_fwspec.fwnode = domain->parent->fwnode;@@ -312,7 +308,6 @@ static int __init tegra_ictlr_init(struct device_node *node,"%pOF: Found %u interrupt controllers in DT; expected %u.\n" , node, num_ictlrs, soc->num_ictlrs); - domain =irq_domain_add_hierarchy (parent_domain,0 , num_ictlrs *32 , node, &tegra_ictlr_domain_ops, lic);Signature · 1 line
2.22.0
Signature · 1 line
Jazz is not dead, it just smells funny.
Re: [PATCH v1 2/2] irqchip/tegra: Clean up coding style
13.08.2019 17:50, Marc Zyngier пишет:
Quoted 12 lines
Quoted 3 lines · Dmitry Osipenko
Make coding style to conform to the kernel's standard by fixing checkpatch warnings about "line over 80 characters".
The last time I used a VT100 was about 30 years ago. I still think this was one of the most brilliant piece of equipment DEC ever produced, but I replaced it at the time with a Wyse 50 that had a 132 column mode. But even then, I could make my XTerm as wide as I wanted, and things haven't regressed much since.
Quoted 3 lines
More seriously, I don't consider the 80 column limit a hard one, and I'm pretty happy with code that spans more that 80 columns if that allows to read an expression without messing with the flow.
Usually I have multiple source files opened side-by-side and the view sizes are tuned for 80 chars, it messes at least my flow when something goes over 80 chars.
Quoted 12 lines
Quoted 12 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/irqchip/irq-tegra.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)drivers/irqchip/irq-tegra.c +0
-0@@ -74,7 +74,7 @@ static struct tegra_ictlr_info *lic;
Quoted 6 lines
drivers/irqchip/irq-tegra.c static struct tegra_ictlr_info *lic; · line 77
Quoted 4 lines
drivers/irqchip/irq-tegra.c +1
-1static inline void tegra_ictlr_write_mask (struct irq_data *d,unsigned long reg) { -void __iomem *base = (void __iomem __force *)d->chip_data; +void __iomem *base = lic->base[d->hwirq /32 ];(1) This is an undocumented change
In my opinion this is a very trivial change and then the end result is absolutely the same, hence nothing to document here. Just read the code, I'd say.
Quoted 3 lines
(2) Why do you think that moving from a per-interrupt base that is known at setup time to something that has to be recomputed on each and every access is a good thing?
I think that there is no practical difference and the new variant is a bit more obvious and readable.
Quoted 50 lines
Quoted 49 lines
drivers/irqchip/irq-tegra.c +4
-9u32 mask; mask =BIT (d->hwirq %32 );@@ -142,7 +142,8 @@ static int tegra_ictlr_suspend(void)writel_relaxed (~0ul , ictlr +ICTLR_CPU_IER_CLR );/* Enable the wakeup sources of ictlr */ -writel_relaxed (lic->ictlr_wake_mask[i], ictlr +ICTLR_CPU_IER_SET ); +writel_relaxed (lic->ictlr_wake_mask[i], + ictlr +ICTLR_CPU_IER_SET ); }local_irq_restore (flags);@@ -222,7 +223,6 @@ static int tegra_ictlr_domain_alloc(struct irq_domain *domain, {struct irq_fwspec *fwspec = data;struct irq_fwspec parent_fwspec; -struct tegra_ictlr_info *info = domain->host_data;irq_hw_number_t hwirq;unsigned int i;@@ -235,13 +235,9 @@ static int tegra_ictlr_domain_alloc(struct irq_domain *domain,if (hwirq >= (num_ictlrs *32 ))return -EINVAL ; -for (i =0 ; i < nr_irqs; i++) { -int ictlr = (hwirq + i) /32 ; - +for (i =0 ; i < nr_irqs; i++)irq_domain_set_hwirq_and_chip (domain, virq + i, hwirq + i, - &tegra_ictlr_chip, - (void __force *)info->base[ictlr]); - } + &tegra_ictlr_chip,NULL ); parent_fwspec = *fwspec; parent_fwspec.fwnode = domain->parent->fwnode;@@ -312,7 +308,6 @@ static int __init tegra_ictlr_init(struct device_node *node,"%pOF: Found %u interrupt controllers in DT; expected %u.\n" , node, num_ictlrs, soc->num_ictlrs); - domain =irq_domain_add_hierarchy (parent_domain,0 , num_ictlrs *32 , node, &tegra_ictlr_domain_ops, lic);Signature · 1 line
2.22.0
Re: [PATCH v1 2/2] irqchip/tegra: Clean up coding style
Quoted 41 lines · Dmitry Osipenko
13.08.2019 17:50, Marc Zyngier пишет:
Quoted 12 lines
Quoted 3 lines · Dmitry Osipenko
Make coding style to conform to the kernel's standard by fixing checkpatch warnings about "line over 80 characters".
The last time I used a VT100 was about 30 years ago. I still think this was one of the most brilliant piece of equipment DEC ever produced, but I replaced it at the time with a Wyse 50 that had a 132 column mode. But even then, I could make my XTerm as wide as I wanted, and things haven't regressed much since.
Quoted 3 lines
More seriously, I don't consider the 80 column limit a hard one, and I'm pretty happy with code that spans more that 80 columns if that allows to read an expression without messing with the flow.
Usually I have multiple source files opened side-by-side and the view sizes are tuned for 80 chars, it messes at least my flow when something goes over 80 chars.
Quoted 12 lines
Quoted 12 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/irqchip/irq-tegra.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)drivers/irqchip/irq-tegra.c +0
-0@@ -74,7 +74,7 @@ static struct tegra_ictlr_info *lic;Quoted 6 lines
drivers/irqchip/irq-tegra.c static struct tegra_ictlr_info *lic; · line 77
Quoted 4 lines
drivers/irqchip/irq-tegra.c +1
-1static inline void tegra_ictlr_write_mask (struct irq_data *d,unsigned long reg) { -void __iomem *base = (void __iomem __force *)d->chip_data; +void __iomem *base = lic->base[d->hwirq /32 ];(1) This is an undocumented change
Quoted 3 lines
In my opinion this is a very trivial change and then the end result is absolutely the same, hence nothing to document here. Just read the code, I'd say.
And that is the very reason why I won't take any of your patches any time soon.
M.
Signature · 1 line
Jazz is not dead, it just smells funny.
Re: [PATCH v1 2/2] irqchip/tegra: Clean up coding style
13.08.2019 19:18, Marc Zyngier пишет:
Quoted 43 lines
Quoted 41 lines · Dmitry Osipenko
13.08.2019 17:50, Marc Zyngier пишет:
Quoted 12 lines
Quoted 3 lines · Dmitry Osipenko
Make coding style to conform to the kernel's standard by fixing checkpatch warnings about "line over 80 characters".
The last time I used a VT100 was about 30 years ago. I still think this was one of the most brilliant piece of equipment DEC ever produced, but I replaced it at the time with a Wyse 50 that had a 132 column mode. But even then, I could make my XTerm as wide as I wanted, and things haven't regressed much since.
Quoted 3 lines
More seriously, I don't consider the 80 column limit a hard one, and I'm pretty happy with code that spans more that 80 columns if that allows to read an expression without messing with the flow.
Usually I have multiple source files opened side-by-side and the view sizes are tuned for 80 chars, it messes at least my flow when something goes over 80 chars.
Quoted 18 lines
Quoted 11 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/irqchip/irq-tegra.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)drivers/irqchip/irq-tegra.c +0
-0drivers/irqchip/irq-tegra.c static struct tegra_ictlr_info *lic; · line 77
Quoted 5 lines
drivers/irqchip/irq-tegra.c +1
-1@@ -74,7 +74,7 @@ static struct tegra_ictlr_info *lic;static inline void tegra_ictlr_write_mask (struct irq_data *d,unsigned long reg) { -void __iomem *base = (void __iomem __force *)d->chip_data; +void __iomem *base = lic->base[d->hwirq /32 ];(1) This is an undocumented change
Quoted 6 lines
Quoted 3 lines
In my opinion this is a very trivial change and then the end result is absolutely the same, hence nothing to document here. Just read the code, I'd say.
And that is the very reason why I won't take any of your patches any time soon.
Sorry, the last part probably sounds a bit offending, but that's not what I meant (please note that english isn't my native language). I was just trying to say that the code is self-explanatory in my opinion. Anyway, it's not a bad excuse for me to get a break from it all. Sorry again.
Quoted 5 lines · Dmitry Osipenko
There is no point in touching of the COP (ARM7TDMI auxiliary boot/firmware CPU) because COP's interrupts should be related only to an old multimedia firmware that is not applicable to the upstream kernel. Hence let's remove everything related to the COP, for consistency.
We've had that exact discussion a few weeks ago, and I objected to this change as there is no possible way you can know for sure people don't use mainline with an old firmware. The whole point of the firmware is to be an abstraction for the kernel, for better or worse.
If you really want to do something useful here, consider detecting such firmware (even better, detect the new ones that don't require this stuff) and document the various requirements. At least we'll know where we stand, because the changes you're making are just as random as what we have so far.
Thanks,
M.
Signature · 1 line
Jazz is not dead, it just smells funny.
13.08.2019 17:25, Marc Zyngier пишет:
Quoted 8 lines
Quoted 5 lines · Dmitry Osipenko
There is no point in touching of the COP (ARM7TDMI auxiliary boot/firmware CPU) because COP's interrupts should be related only to an old multimedia firmware that is not applicable to the upstream kernel. Hence let's remove everything related to the COP, for consistency.
Quoted 4 lines
We've had that exact discussion a few weeks ago, and I objected to this change as there is no possible way you can know for sure people don't use mainline with an old firmware. The whole point of the firmware is to be an abstraction for the kernel, for better or worse.
I think you're misunderstanding what kind of firmware it is. It absolutely can't be used with upstream kernel without of porting the whole downstream driver stack (which is a very old crusty v3.1 kernel, a typical non-upstreamable downstream hackery). In upstream we now have a proper DRM graphics driver, video decoder driver and etc. Few years ago those downstream drivers were obsoleted and not supported by the NVIDIA anymore.
Quoted 5 lines
If you really want to do something useful here, consider detecting such firmware (even better, detect the new ones that don't require this stuff) and document the various requirements. At least we'll know where we stand, because the changes you're making are just as random as what we have so far.
As I just said in the above, that old firmware is not usable with the upstream in any shape or form. Hence there is nothing to detect, it is safe to assume that only newer hardware has something running in background. Peter De Schrijver suggested that kernel shouldn't touch the COP's interrupts for the newer firmware, so this patch should be fine if Peter or somebody else from NVIDIA will give an ACK.