Tegra GPIO: Minor code clean up
Subsystems (2)
cover Tegra GPIO: Minor code clean up
Hello,
I was investigating why CPU hangs during of GPIO driver suspend and in the end it turned out that it is a Broadcom WiFi driver problem because it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and this may cause a bit weird CPU hang on writing to INT_ENB register during of GPIO driver suspend. Meanwhile I also noticed that a few things could be improved in the driver, that's what this small series addresses.
Dmitry Osipenko (3):
gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
gpio: tegra: Properly handle irq_set_irq_wake() error
gpio: tegra: Use NOIRQ phase for suspend/resume
drivers/gpio/gpio-tegra.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
Signature · 1 line
2.24.0
PATCH 1/3 gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
There is no point in using old-style raw accessors, the generic accessors do the same thing and also take into account CPU endianness. Tegra SoCs do not support big-endian mode in the upstream kernel, but let's switch away from the outdated things anyway, just to keep code up-to-date.
--- drivers/gpio/gpio-tegra.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpio/gpio-tegra.c +2 -2
@@ -96,12 +96,12 @@ struct tegra_gpio_info {static inline void tegra_gpio_writel (struct tegra_gpio_info *tgi,u32 val,u32 reg) { -__raw_writel (val, tgi->regs + reg); +writel_relaxed (val, tgi->regs + reg); }static inline u32 tegra_gpio_readl (struct tegra_gpio_info *tgi,u32 reg) { -return __raw_readl (tgi->regs + reg); +return readl_relaxed (tgi->regs + reg); } static unsigned int tegra_gpio_compose(unsigned int bank, unsigned int port,
Signature · 1 line
2.24.0
Re: [PATCH v1 1/3] gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
niedz., 15 gru 2019 o 19:31 Dmitry Osipenko <[email protected]> napisał(a):
Quoted 31 lines
There is no point in using old-style raw accessors, the generic accessors do the same thing and also take into account CPU endianness. Tegra SoCs do not support big-endian mode in the upstream kernel, but let's switch away from the outdated things anyway, just to keep code up-to-date.
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpio/gpio-tegra.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/gpio/gpio-tegra.c +2
-3@@ -96,12 +96,12 @@ struct tegra_gpio_info {static inline void tegra_gpio_writel (struct tegra_gpio_info *tgi,u32 val,u32 reg) { -__raw_writel (val, tgi->regs + reg); +writel_relaxed (val, tgi->regs + reg); }static inline u32 tegra_gpio_readl (struct tegra_gpio_info *tgi,u32 reg) { -return __raw_readl (tgi->regs + reg); +return readl_relaxed (tgi->regs + reg); } static unsigned int tegra_gpio_compose(unsigned int bank, unsigned int port, --
Quoted 2 lines
2.24.0
The entire series looks good to me, but I'll wait for Thierry's acks just in case.
Bart
Re: [PATCH v1 1/3] gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
19.12.2019 14:01, Bartosz Golaszewski пишет:
Quoted 30 lines
niedz., 15 gru 2019 o 19:31 Dmitry Osipenko <[email protected]> napisał(a):
Quoted 29 lines
There is no point in using old-style raw accessors, the generic accessors do the same thing and also take into account CPU endianness. Tegra SoCs do not support big-endian mode in the upstream kernel, but let's switch away from the outdated things anyway, just to keep code up-to-date.
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpio/gpio-tegra.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/gpio/gpio-tegra.c +2
-2@@ -96,12 +96,12 @@ struct tegra_gpio_info {static inline void tegra_gpio_writel (struct tegra_gpio_info *tgi,u32 val,u32 reg) { -__raw_writel (val, tgi->regs + reg); +writel_relaxed (val, tgi->regs + reg); }static inline u32 tegra_gpio_readl (struct tegra_gpio_info *tgi,u32 reg) { -return __raw_readl (tgi->regs + reg); +return readl_relaxed (tgi->regs + reg); }
Quoted 7 lines
Quoted 4 lines
drivers/gpio/gpio-tegra.c +0
-1static unsigned int tegra_gpio_compose(unsigned int bank, unsigned int port, --2.24.0
The entire series looks good to me, but I'll wait for Thierry's acks just in case.
Thank you very much for taking a look at the patches!
Re: [PATCH v1 1/3] gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
In reply to Bartosz GolaszewskiQuoted 6 lines · Bartosz Golaszewski
niedz., 15 gru 2019 o 19:31 Dmitry Osipenko <[email protected]> napisał(a):
Quoted 5 lines
There is no point in using old-style raw accessors, the generic accessors do the same thing and also take into account CPU endianness. Tegra SoCs do not support big-endian mode in the upstream kernel, but let's switch away from the outdated things anyway, just to keep code up-to-date.
Good idea, I think I got most of the way to booting a tegra jetson board with a big endian image a few years ago, but never got time to finish the work.
Signature · 4 lines
Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html
Re: [PATCH v1 1/3] gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
19.12.2019 18:57, Ben Dooks пишет:
Quoted 12 lines · Bartosz Golaszewski
Quoted 10 lines
niedz., 15 gru 2019 o 19:31 Dmitry Osipenko <[email protected]> napisał(a):
Quoted 8 lines
There is no point in using old-style raw accessors, the generic accessors
do the same thing and also take into account CPU endianness. Tegra SoCs do
not support big-endian mode in the upstream kernel, but let's switch away
from the outdated things anyway, just to keep code up-to-date.
Quoted 4 lines
Good idea, I think I got most of the way to booting a tegra jetson board with a big endian image a few years ago, but never got time to finish the work.
Thanks, I remember yours effort in regards to the upstreaming big endian support for Jetson :) You were pretty close back then, but Thierry had a different opinion in regards to maintaining that support in terms of having extra testing burden.
PATCH 2/3 gpio: tegra: Properly handle irq_set_irq_wake() error
In reply to Dmitry OsipenkoTechnically upstream interrupt controller may fail changing of GPIO's bank wake-state and in this case the GPIO's wake-state shouldn't be changed.
--- drivers/gpio/gpio-tegra.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpio/gpio-tegra.c +6 -1
@@ -497,6 +497,11 @@ static int tegra_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)struct tegra_gpio_bank *bank =irq_data_get_irq_chip_data (d);unsigned int gpio = d->hwirq;u32 port, bit, mask; +int err; + + err =irq_set_irq_wake (bank->irq, enable); +if (err) +return err; port =GPIO_PORT (gpio); bit =GPIO_BIT (gpio);
@@ -507,7 +512,7 @@ static int tegra_gpio_irq_set_wake(struct irq_data *d, unsigned int enable) else bank->wake_enb[port] &= ~mask; - return irq_set_irq_wake(bank->irq, enable); + return 0; }#endif
Signature · 1 line
2.24.0
PATCH 3/3 gpio: tegra: Use NOIRQ phase for suspend/resume
In reply to Dmitry OsipenkoAll GPIO interrupts are disabled during of the NOIRQ suspend/resume phase, thus there is no need to manually disable the interrupts. This patch doesn't fix any problem, this is just a minor clean-up.
--- drivers/gpio/gpio-tegra.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
drivers/gpio/gpio-tegra.c +2 -8
@@ -416,11 +416,8 @@ static void tegra_gpio_irq_handler(struct irq_desc *desc)static int tegra_gpio_resume (struct device *dev) {struct tegra_gpio_info *tgi =dev_get_drvdata (dev); -unsigned long flags;unsigned int b, p; -local_irq_save (flags); -for (b =0 ; b < tgi->bank_count; b++) {struct tegra_gpio_bank *bank = &tgi->bank_info[b];
@@ -448,17 +445,14 @@ static int tegra_gpio_resume(struct device *dev) } } -local_irq_restore (flags);return 0 ; }static int tegra_gpio_suspend (struct device *dev) {struct tegra_gpio_info *tgi =dev_get_drvdata (dev); -unsigned long flags;unsigned int b, p; -local_irq_save (flags);for (b =0 ; b < tgi->bank_count; b++) {struct tegra_gpio_bank *bank = &tgi->bank_info[b];
@@ -488,7 +482,7 @@ static int tegra_gpio_suspend(struct device *dev) GPIO_INT_ENB(tgi, gpio)); } } -local_irq_restore (flags); +return 0 ; }
@@ -562,7 +556,7 @@ static inline void tegra_gpio_debuginit(struct tegra_gpio_info *tgi)#endif static const struct dev_pm_ops tegra_gpio_pm_ops = { -SET_SYSTEM_SLEEP_PM_OPS (tegra_gpio_suspend, tegra_gpio_resume) +SET_NOIRQ_SYSTEM_SLEEP_PM_OPS (tegra_gpio_suspend, tegra_gpio_resume) };static int tegra_gpio_probe (struct platform_device *pdev)
Signature · 1 line
2.24.0
15.12.2019 21:30, Dmitry Osipenko пишет:
Quoted 9 lines
Hello,
I was investigating why CPU hangs during of GPIO driver suspend and in the end it turned out that it is a Broadcom WiFi driver problem because it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and this may cause a bit weird CPU hang on writing to
INT_ENBregister during of GPIO driver suspend. Meanwhile I also noticed that a few things could be improved in the driver, that's what this small series addresses.
Quoted 8 lines
Dmitry Osipenko (3):
gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
gpio: tegra: Properly handleirq_set_irq_wake()error
gpio: tegra: Use NOIRQ phase for suspend/resumedrivers/gpio/gpio-tegra.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
For the reference, here is the WiFi driver fix:
Quoted 9 lines · Dmitry Osipenko
Hello,
I was investigating why CPU hangs during of GPIO driver suspend and in the end it turned out that it is a Broadcom WiFi driver problem because it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and this may cause a bit weird CPU hang on writing to
INT_ENBregister during of GPIO driver suspend. Meanwhile I also noticed that a few things could be improved in the driver, that's what this small series addresses.
Quoted 7 lines
Dmitry Osipenko (3):
gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
gpio: tegra: Properly handleirq_set_irq_wake()error
gpio: tegra: Use NOIRQ phase for suspend/resumedrivers/gpio/gpio-tegra.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
Patches look good:
I also applied this series on top of v5.5-rc1 and ran it through our test system:
Test results:
13 builds: 13 pass, 0 fail
22 boots: 22 pass, 0 fail
34 tests: 34 pass, 0 fail
Linux version: 5.5.0-rc1-g3d0b4fced39e
Boards tested: tegra124-jetson-tk1, tegra186-p2771-0000,
tegra194-p2972-0000, tegra20-ventana,
tegra210-p2371-2180, tegra30-cardhu-a04All tests passing, so:
19.12.2019 17:53, Thierry Reding пишет:
Quoted 35 lines · Dmitry Osipenko
Quoted 9 lines
Hello,
I was investigating why CPU hangs during of GPIO driver suspend and in the end it turned out that it is a Broadcom WiFi driver problem because it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and this may cause a bit weird CPU hang on writing to
INT_ENBregister during of GPIO driver suspend. Meanwhile I also noticed that a few things could be improved in the driver, that's what this small series addresses.Quoted 7 lines
Dmitry Osipenko (3):
gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
gpio: tegra: Properly handleirq_set_irq_wake()error
gpio: tegra: Use NOIRQ phase for suspend/resumedrivers/gpio/gpio-tegra.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)Patches look good:
Reviewed-by Thierry Reding <[email protected]>I also applied this series on top of v5.5-rc1 and ran it through our test system:
Test results: 13 builds: 13 pass, 0 fail 22 boots: 22 pass, 0 fail 34 tests: 34 pass, 0 fail Linux version: 5.5.0-rc1-g3d0b4fced39e Boards tested: tegra124-jetson-tk1, tegra186-p2771-0000, tegra194-p2972-0000, tegra20-ventana, tegra210-p2371-2180, tegra30-cardhu-a04
Quoted 4 lines
All tests passing, so:
Tested-by Thierry Reding <[email protected]>
Thank you very much!
Quoted 11 lines · Dmitry Osipenko
I was investigating why CPU hangs during of GPIO driver suspend and in the end it turned out that it is a Broadcom WiFi driver problem because it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and this may cause a bit weird CPU hang on writing to
INT_ENBregister during of GPIO driver suspend. Meanwhile I also noticed that a few things could be improved in the driver, that's what this small series addresses.Dmitry Osipenko (3):
gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
gpio: tegra: Properly handleirq_set_irq_wake()error
gpio: tegra: Use NOIRQ phase for suspend/resume
All three patches applied with Thierry's review/test tag.
Yours,
Linus Walleij
pon., 6 sty 2020 o 23:59 Linus Walleij <[email protected]> napisał(a):
Quoted 15 lines · Dmitry Osipenko
Quoted 8 lines
I was investigating why CPU hangs during of GPIO driver suspend and in the end it turned out that it is a Broadcom WiFi driver problem because it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and this may cause a bit weird CPU hang on writing to
INT_ENBregister during of GPIO driver suspend. Meanwhile I also noticed that a few things could be improved in the driver, that's what this small series addresses.Quoted 4 lines
Dmitry Osipenko (3):
gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
gpio: tegra: Properly handleirq_set_irq_wake()error
gpio: tegra: Use NOIRQ phase for suspend/resume
Quoted 4 lines
All three patches applied with Thierry's review/test tag.
Yours,
Linus Walleij
Ugh, I now noticed I responded to Thierry only after applying this to my tree.
Anyway, it shouldn't be a problem. I'll take more care next time.
Bart
Quoted 15 lines · Bartosz Golaszewski
pon., 6 sty 2020 o 23:59 Linus Walleij <[email protected]> napisał(a):
Quoted 14 lines · Dmitry Osipenko
Quoted 12 lines
I was investigating why CPU hangs during of GPIO driver suspend and in the end it turned out that it is a Broadcom WiFi driver problem because it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and this may cause a bit weird CPU hang on writing to
INT_ENBregister during of GPIO driver suspend. Meanwhile I also noticed that a few things could be improved in the driver, that's what this small series addresses.Dmitry Osipenko (3):
gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
gpio: tegra: Properly handleirq_set_irq_wake()error
gpio: tegra: Use NOIRQ phase for suspend/resume
Quoted 8 lines
Quoted 4 lines
All three patches applied with Thierry's review/test tag.
Yours,
Linus WalleijUgh, I now noticed I responded to Thierry only after applying this to my tree.
Anyway, it shouldn't be a problem. I'll take more care next time.
OK shall I drop the patches from my tree then? No big deal.
Thanks,
Linus Walleij
wt., 7 sty 2020 o 10:29 Linus Walleij <[email protected]> napisał(a):
Quoted 21 lines
Quoted 17 lines · Bartosz Golaszewski
pon., 6 sty 2020 o 23:59 Linus Walleij <[email protected]> napisał(a):
Quoted 16 lines · Dmitry Osipenko
Quoted 8 lines
I was investigating why CPU hangs during of GPIO driver suspend and in the end it turned out that it is a Broadcom WiFi driver problem because it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and this may cause a bit weird CPU hang on writing to
INT_ENBregister during of GPIO driver suspend. Meanwhile I also noticed that a few things could be improved in the driver, that's what this small series addresses.Quoted 4 lines
Dmitry Osipenko (3):
gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors
gpio: tegra: Properly handleirq_set_irq_wake()error
gpio: tegra: Use NOIRQ phase for suspend/resumeAll three patches applied with Thierry's review/test tag.
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
Yours,
Linus WalleijUgh, I now noticed I responded to Thierry only after applying this to my tree.
Anyway, it shouldn't be a problem. I'll take more care next time.
OK shall I drop the patches from my tree then? No big deal.
If you're fine with this, sure!
Thanks a lot,
Bart
Quoted 10 lines · Bartosz Golaszewski
wt., 7 sty 2020 o 10:29 Linus Walleij <[email protected]> napisał(a):
Quoted 6 lines
Quoted 3 lines
Ugh, I now noticed I responded to Thierry only after applying this to my tree.
Anyway, it shouldn't be a problem. I'll take more care next time.
OK shall I drop the patches from my tree then? No big deal.
If you're fine with this, sure!
OK dropped them, hadn't even pushed the branch out yet.
Soon reaching the top of my mailbox so I will be pushing the branches for the autobuilders and later tonight for-next.
Yours,
Linus Walleij