[PATCH v3] drivers/clocksource/mediatek: Ack and disable interrupts on suspend
Interrupts are disabled during suspend before this driver disables its timers. ARM trusted firmware will abort suspend if the timer irq is pending, so ack and disable the timer interrupt during suspend. Signed-off-by: Evan Benn --- Changes in v3: Move the ACK from the shutdown to the suspend function. Changes in v2: Remove the patch that splits the drivers into 2 files. drivers/clocksource/timer-mediatek.c | 23 +++ 1 file changed, 23 insertions(+) diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c index 9318edcd8963..1ae8fee639bf 100644 --- a/drivers/clocksource/timer-mediatek.c +++ b/drivers/clocksource/timer-mediatek.c @@ -241,6 +241,27 @@ static void mtk_gpt_enable_irq(struct timer_of *to, u8 timer) timer_of_base(to) + GPT_IRQ_EN_REG); } +static void mtk_gpt_resume(struct clock_event_device *clk) +{ + struct timer_of *to = to_timer_of(clk); + + mtk_gpt_enable_irq(to, TIMER_CLK_EVT); +} + +static void mtk_gpt_suspend(struct clock_event_device *clk) +{ + struct timer_of *to = to_timer_of(clk); + + /* Disable all interrupts */ + writel(0x0, timer_of_base(to) + GPT_IRQ_EN_REG); + + /* This is called with interrupts disabled, +* so we need to ack any interrupt that is pending +* Or for example ATF will prevent a suspend from completing. +*/ + writel(0x3f, timer_of_base(to) + GPT_IRQ_ACK_REG); +} + static struct timer_of to = { .flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK, @@ -286,6 +307,8 @@ static int __init mtk_gpt_init(struct device_node *node) to.clkevt.set_state_oneshot = mtk_gpt_clkevt_shutdown; to.clkevt.tick_resume = mtk_gpt_clkevt_shutdown; to.clkevt.set_next_event = mtk_gpt_clkevt_next_event; + to.clkevt.suspend = mtk_gpt_suspend; + to.clkevt.resume = mtk_gpt_resume; to.of_irq.handler = mtk_gpt_interrupt; ret = timer_of_init(node, &to); -- 2.31.1.295.g9ea45b61b8-goog
Re: [PATCH v2] drivers/clocksource/mediatek: Ack and disable interrupts on shutdown
On Sat, Apr 3, 2021 at 1:15 AM Daniel Lezcano wrote: > > > > That said I am happy to upload that version if people think it is better. > > IMO, it is not in the normal flow to disable/enable the interrupts. Hi Daniel, no problem. I will upload the suspend/resume version. Please note the similar fix to the other timer in this file: https://patchwork.kernel.org/project/linux-mediatek/patch/1614670085-26229-2-git-send-email-fengquan.c...@mediatek.com/ Is doing the same ack/disable. > > Does this timer belong to an always-on power domain ? > I'm not sure sorry, the datasheet does not seem to say. Maybe mediatek can answer.
Re: [PATCH v2] drivers/clocksource/mediatek: Ack and disable interrupts on shutdown
Hi Daniel, That is a good point, and I did try that at first and it works fine. I uploaded this version because the suspend/resume callbacks were undocumented and mostly not used by other clocksource drivers. I thought a smaller diff might be preferable. I also thought it would be better to shut off the interrupt as soon as it is not needed, avoiding any other potential bugs, instead of just fixing the one we know about with suspend. I'm not sure how the other driver / timer disable flows are intended to work for example (shutdown, detach, etc). That said I am happy to upload that version if people think it is better. https://elixir.bootlin.com/linux/latest/source/include/linux/clockchips.h#L120 On Thu, 25 Mar 2021 at 19:10, Daniel Lezcano wrote: > > On 25/03/2021 02:35, Evan Benn wrote: > > set_state_shutdown is called during system suspend after interrupts have > > been disabled. If the timer has fired in the meantime, there will be > > a pending IRQ. So we ack that now and disable the timer. Without this > > ARM trusted firmware will abort the suspend due to the pending > > interrupt. > > > > Now always disable the IRQ in state transitions, and re-enable in > > set_periodic and next_event. > > Why not put add the suspend/resume callbacks and put there the specific > code and let the irq untouched in the normal flow ? > > > Signed-off-by: Evan Benn > > --- > > > > Changes in v2: > > Remove the patch that splits the drivers into 2 files. > > > > drivers/clocksource/timer-mediatek.c | 49 +--- > > 1 file changed, 30 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/clocksource/timer-mediatek.c > > b/drivers/clocksource/timer-mediatek.c > > index 9318edcd8963..fba2f9494d90 100644 > > --- a/drivers/clocksource/timer-mediatek.c > > +++ b/drivers/clocksource/timer-mediatek.c > > @@ -132,13 +132,33 @@ static u64 notrace mtk_gpt_read_sched_clock(void) > > return readl_relaxed(gpt_sched_reg); > > } > > > > +static void mtk_gpt_disable_ack_interrupts(struct timer_of *to, u8 timer) > > +{ > > + u32 val; > > + > > + /* Disable interrupts */ > > + val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); > > + writel(val & ~GPT_IRQ_ENABLE(timer), timer_of_base(to) + > > +GPT_IRQ_EN_REG); > > + > > + /* Ack interrupts */ > > + writel(GPT_IRQ_ACK(timer), timer_of_base(to) + GPT_IRQ_ACK_REG); > > +} > > + > > static void mtk_gpt_clkevt_time_stop(struct timer_of *to, u8 timer) > > { > > u32 val; > > > > + /* Disable timer */ > > val = readl(timer_of_base(to) + GPT_CTRL_REG(timer)); > > writel(val & ~GPT_CTRL_ENABLE, timer_of_base(to) + > > GPT_CTRL_REG(timer)); > > + > > + /* This may be called with interrupts disabled, > > + * so we need to ack any interrupt that is pending > > + * Or for example ATF will prevent a suspend from completing. > > + */ > > + mtk_gpt_disable_ack_interrupts(to, timer); > > } > > > > static void mtk_gpt_clkevt_time_setup(struct timer_of *to, > > @@ -152,8 +172,10 @@ static void mtk_gpt_clkevt_time_start(struct timer_of > > *to, > > { > > u32 val; > > > > - /* Acknowledge interrupt */ > > - writel(GPT_IRQ_ACK(timer), timer_of_base(to) + GPT_IRQ_ACK_REG); > > + /* Enable interrupts */ > > + val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); > > + writel(val | GPT_IRQ_ENABLE(timer), > > +timer_of_base(to) + GPT_IRQ_EN_REG); > > > > val = readl(timer_of_base(to) + GPT_CTRL_REG(timer)); > > > > @@ -226,21 +248,6 @@ __init mtk_gpt_setup(struct timer_of *to, u8 timer, u8 > > option) > > timer_of_base(to) + GPT_CTRL_REG(timer)); > > } > > > > -static void mtk_gpt_enable_irq(struct timer_of *to, u8 timer) > > -{ > > - u32 val; > > - > > - /* Disable all interrupts */ > > - writel(0x0, timer_of_base(to) + GPT_IRQ_EN_REG); > > - > > - /* Acknowledge all spurious pending interrupts */ > > - writel(0x3f, timer_of_base(to) + GPT_IRQ_ACK_REG); > > - > > - val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); > > - writel(val | GPT_IRQ_ENABLE(timer), > > -timer_of_base(to) + GPT_IRQ_EN_REG); > > -} > > - > > static struct timer_of to = { > > .flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK, > > > > @@ -292,6 +299,12 @@ static int __init mtk_gpt_init
[PATCH v2] drivers/clocksource/mediatek: Ack and disable interrupts on shutdown
set_state_shutdown is called during system suspend after interrupts have been disabled. If the timer has fired in the meantime, there will be a pending IRQ. So we ack that now and disable the timer. Without this ARM trusted firmware will abort the suspend due to the pending interrupt. Now always disable the IRQ in state transitions, and re-enable in set_periodic and next_event. Signed-off-by: Evan Benn --- Changes in v2: Remove the patch that splits the drivers into 2 files. drivers/clocksource/timer-mediatek.c | 49 +--- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c index 9318edcd8963..fba2f9494d90 100644 --- a/drivers/clocksource/timer-mediatek.c +++ b/drivers/clocksource/timer-mediatek.c @@ -132,13 +132,33 @@ static u64 notrace mtk_gpt_read_sched_clock(void) return readl_relaxed(gpt_sched_reg); } +static void mtk_gpt_disable_ack_interrupts(struct timer_of *to, u8 timer) +{ + u32 val; + + /* Disable interrupts */ + val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); + writel(val & ~GPT_IRQ_ENABLE(timer), timer_of_base(to) + + GPT_IRQ_EN_REG); + + /* Ack interrupts */ + writel(GPT_IRQ_ACK(timer), timer_of_base(to) + GPT_IRQ_ACK_REG); +} + static void mtk_gpt_clkevt_time_stop(struct timer_of *to, u8 timer) { u32 val; + /* Disable timer */ val = readl(timer_of_base(to) + GPT_CTRL_REG(timer)); writel(val & ~GPT_CTRL_ENABLE, timer_of_base(to) + GPT_CTRL_REG(timer)); + + /* This may be called with interrupts disabled, +* so we need to ack any interrupt that is pending +* Or for example ATF will prevent a suspend from completing. +*/ + mtk_gpt_disable_ack_interrupts(to, timer); } static void mtk_gpt_clkevt_time_setup(struct timer_of *to, @@ -152,8 +172,10 @@ static void mtk_gpt_clkevt_time_start(struct timer_of *to, { u32 val; - /* Acknowledge interrupt */ - writel(GPT_IRQ_ACK(timer), timer_of_base(to) + GPT_IRQ_ACK_REG); + /* Enable interrupts */ + val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); + writel(val | GPT_IRQ_ENABLE(timer), + timer_of_base(to) + GPT_IRQ_EN_REG); val = readl(timer_of_base(to) + GPT_CTRL_REG(timer)); @@ -226,21 +248,6 @@ __init mtk_gpt_setup(struct timer_of *to, u8 timer, u8 option) timer_of_base(to) + GPT_CTRL_REG(timer)); } -static void mtk_gpt_enable_irq(struct timer_of *to, u8 timer) -{ - u32 val; - - /* Disable all interrupts */ - writel(0x0, timer_of_base(to) + GPT_IRQ_EN_REG); - - /* Acknowledge all spurious pending interrupts */ - writel(0x3f, timer_of_base(to) + GPT_IRQ_ACK_REG); - - val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); - writel(val | GPT_IRQ_ENABLE(timer), - timer_of_base(to) + GPT_IRQ_EN_REG); -} - static struct timer_of to = { .flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK, @@ -292,6 +299,12 @@ static int __init mtk_gpt_init(struct device_node *node) if (ret) return ret; + /* In case the firmware left the interrupts enabled +* disable and ack those now +*/ + mtk_gpt_disable_ack_interrupts(&to, TIMER_CLK_SRC); + mtk_gpt_disable_ack_interrupts(&to, TIMER_CLK_EVT); + /* Configure clock source */ mtk_gpt_setup(&to, TIMER_CLK_SRC, GPT_CTRL_OP_FREERUN); clocksource_mmio_init(timer_of_base(&to) + GPT_CNT_REG(TIMER_CLK_SRC), @@ -305,8 +318,6 @@ static int __init mtk_gpt_init(struct device_node *node) clockevents_config_and_register(&to.clkevt, timer_of_rate(&to), TIMER_SYNC_TICKS, 0x); - mtk_gpt_enable_irq(&to, TIMER_CLK_EVT); - return 0; } TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init); -- 2.31.0.291.g576ba9dcdaf-goog
Re: [PATCH 1/2] drivers/clocksource/mediatek: Split mediatek drivers into 2 files
On Mon, Mar 22, 2021 at 10:19 PM Daniel Lezcano wrote: > > > Hi Evan, > > On 18/03/2021 06:04, Evan Benn wrote: > > mtk_gpt and mtk_syst drivers for mt6577 and mt6765 devices were not > > sharing any code. So split them into separate files. > > For the sake of consistency, keeping all in one is better. Thanks Daniel, I think you are right. The other patch does apply cleanly without this one, so please just ignore this patch. > > Thanks > -- Daniel > > > Signed-off-by: Evan Benn > > --- > > > > arch/arm/mach-mediatek/Kconfig| 3 +- > > arch/arm64/Kconfig.platforms | 3 +- > > drivers/clocksource/Kconfig | 13 +- > > drivers/clocksource/Makefile | 3 +- > > ...mer-mediatek.c => timer-mediatek-mt6577.c} | 100 - > > drivers/clocksource/timer-mediatek-mt6765.c | 135 ++ > > 6 files changed, 151 insertions(+), 106 deletions(-) > > rename drivers/clocksource/{timer-mediatek.c => timer-mediatek-mt6577.c} > > (69%) > > create mode 100644 drivers/clocksource/timer-mediatek-mt6765.c > > > > diff --git a/arch/arm/mach-mediatek/Kconfig b/arch/arm/mach-mediatek/Kconfig > > index 9e0f592d87d8..8686f992c4b6 100644 > > --- a/arch/arm/mach-mediatek/Kconfig > > +++ b/arch/arm/mach-mediatek/Kconfig > > @@ -4,7 +4,8 @@ menuconfig ARCH_MEDIATEK > > depends on ARCH_MULTI_V7 > > select ARM_GIC > > select PINCTRL > > - select MTK_TIMER > > + select TIMER_MEDIATEK_MT6577 > > + select TIMER_MEDIATEK_MT6765 > > select MFD_SYSCON > > help > > Support for Mediatek MT65xx & MT81xx SoCs > > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms > > index cdfd5fed457f..d4752375ab0b 100644 > > --- a/arch/arm64/Kconfig.platforms > > +++ b/arch/arm64/Kconfig.platforms > > @@ -161,7 +161,8 @@ config ARCH_MEDIATEK > > bool "MediaTek SoC Family" > > select ARM_GIC > > select PINCTRL > > - select MTK_TIMER > > + select TIMER_MEDIATEK_MT6577 > > + select TIMER_MEDIATEK_MT6765 > > help > > This enables support for MediaTek MT27xx, MT65xx, MT76xx > > & MT81xx ARMv8 SoCs > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > index 39aa21d01e05..d697c799284e 100644 > > --- a/drivers/clocksource/Kconfig > > +++ b/drivers/clocksource/Kconfig > > @@ -438,13 +438,20 @@ config OXNAS_RPS_TIMER > > config SYS_SUPPORTS_SH_CMT > > bool > > > > -config MTK_TIMER > > - bool "Mediatek timer driver" if COMPILE_TEST > > +config TIMER_MEDIATEK_MT6577 > > + bool "Mediatek mt6577 timer driver" if COMPILE_TEST > > depends on HAS_IOMEM > > select TIMER_OF > > select CLKSRC_MMIO > > help > > - Support for Mediatek timer driver. > > + Enables clocksource and clockevent driver for Mediatek mt6577 timer. > > + > > +config TIMER_MEDIATEK_MT6765 > > + bool "Mediatek mt6765 timer driver" if COMPILE_TEST > > + depends on HAS_IOMEM > > + select TIMER_OF > > + help > > + Enables clockevent driver for Mediatek mt6765 timer. > > > > config SPRD_TIMER > > bool "Spreadtrum timer driver" if EXPERT > > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile > > index c17ee32a7151..b1f06ce114f9 100644 > > --- a/drivers/clocksource/Makefile > > +++ b/drivers/clocksource/Makefile > > @@ -49,7 +49,8 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)+= samsung_pwm_timer.o > > obj-$(CONFIG_FSL_FTM_TIMER) += timer-fsl-ftm.o > > obj-$(CONFIG_VF_PIT_TIMER) += timer-vf-pit.o > > obj-$(CONFIG_CLKSRC_QCOM)+= timer-qcom.o > > -obj-$(CONFIG_MTK_TIMER) += timer-mediatek.o > > +obj-$(CONFIG_TIMER_MEDIATEK_MT6577) += timer-mediatek-mt6577.o > > +obj-$(CONFIG_TIMER_MEDIATEK_MT6765) += timer-mediatek-mt6765.o > > obj-$(CONFIG_CLKSRC_PISTACHIO) += timer-pistachio.o > > obj-$(CONFIG_CLKSRC_TI_32K) += timer-ti-32k.o > > obj-$(CONFIG_OXNAS_RPS_TIMER)+= timer-oxnas-rps.o > > diff --git a/drivers/clocksource/timer-mediatek.c > > b/drivers/clocksource/timer-mediatek-mt6577.c > > similarity index 69% > > rename from drivers/clocksource/timer-mediatek.c > > rename to drivers/clocksource/timer-mediatek-mt6577.c > > index 9318edcd8963..9e5241d1876d 100644 > > --- a
Re: [PATCH] clocksource/drivers/timer-mediatek: optimize systimer irq clear flow on Mediatek Socs
On Thu, Mar 4, 2021 at 11:07 AM Fengquan Chen wrote: > > 1)ensure systimer is enabled before clear and disable interrupt, which only > for systimer in Mediatek Socs. Why does the timer need to be enabled before the interrupt can be disabled? The datasheet I have does not suggest that this is required. > > 2)clear any pending timer-irq when shutdown to keep suspend flow clean, > when use systimer as tick-broadcast timer > > Change-Id: Ia3eda83324af2fdaf5cbb3569a9bf020a11f8009 > Signed-off-by: Fengquan Chen > --- > drivers/clocksource/timer-mediatek.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/clocksource/timer-mediatek.c > b/drivers/clocksource/timer-mediatek.c > index 9318edc..9f1f095dc 100644 > --- a/drivers/clocksource/timer-mediatek.c > +++ b/drivers/clocksource/timer-mediatek.c > @@ -75,6 +75,7 @@ > static void mtk_syst_ack_irq(struct timer_of *to) This function seems to be mis-named. It does more than just ack the irq. > { > /* Clear and disable interrupt */ > + writel(SYST_CON_EN, SYST_CON_REG(to)); This line seems to enable the timer and disable the interrupt. > writel(SYST_CON_IRQ_CLR | SYST_CON_EN, SYST_CON_REG(to)); This line acks the interrupt and enables the timer and disables the interrupt. Are these lines both necessary? Maybe this function should just ack the interrupt without changing the other bits. > } > > @@ -111,6 +112,9 @@ static int mtk_syst_clkevt_next_event(unsigned long ticks, > > static int mtk_syst_clkevt_shutdown(struct clock_event_device *clkevt) > { > + /* Clear any irq */ > + mtk_syst_ack_irq(to_timer_of(clkevt)); > + > /* Disable timer */ > writel(0, SYST_CON_REG(to_timer_of(clkevt))); This is a third write to the same register, I believe all 3 writes can be combined into 1. Is that possible? > > -- > 1.8.1.1.dirty >
Re: [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0
On Sat, Mar 20, 2021 at 8:36 AM Alex Deucher wrote: > > On Fri, Mar 19, 2021 at 5:31 PM Evan Benn wrote: > > > > On Sat, 20 Mar 2021 at 02:10, Harry Wentland wrote: > > > On 2021-03-19 10:22 a.m., Alex Deucher wrote: > > > > On Fri, Mar 19, 2021 at 3:23 AM Evan Benn wrote: > > > >> > > > >> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT was set to the value of 12 > > > >> to ensure no display backlight will flicker at low user brightness > > > >> settings. However this value is quite bright, so for devices that do > > > >> not > > > >> implement the ACPI ATIF > > > >> ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS > > > >> functionality the user cannot set the brightness to a low level even if > > > >> the display would support such a low PWM. > > > >> > > > >> This ATIF feature is not implemented on for example AMD grunt > > > >> chromebooks. > > > >> > > > >> Signed-off-by: Evan Benn > > > >> > > > >> --- > > > >> I could not find a justification for the reason for the value. It has > > > >> caused some noticable regression for users: > > > >> https://bugzilla.kernel.org/show_bug.cgi?id=203439>>> > > > >> Maybe this can be either user controlled or userspace configured, but > > > >> preventing users from turning their backlight dim seems wrong. > > > > > > > > My understanding is that some panels flicker if you set the min to a > > > > value too low. This was a safe minimum if the platform didn't specify > > > > it's own safe minimum. I think we'd just be trading one bug for > > > > another (flickering vs not dim enough). Maybe a whitelist or > > > > blacklist would be a better solution? > > > > > > > > > > Yeah, this is a NACK from me as-is for the reasons Alex described. > > > > Thanks Harry + Alex, > > > > I agree this solution is not the best. > > > > > > > > I agree a whitelist approach might be best. > > > > Do you have any idea what an allowlist could be keyed on? > > Is the flickering you observed here a function of the panel or the gpu > > or some other component? > > Maybe we could move the minimum level into the logic for that hardware. > > > > Maybe the panel string from the EDID? Either that or something from > dmi data? Harry would probably have a better idea. One problem with keying from panel EDID is that for example the grunt chromebook platform has more than 100 different panels already shipped. Add to that that repair centers or people repairing their own device will use 'compatible' panels. I'm sure the AMD windows laptops have even more variety! > > Alex > > > > > > > Is this fix perhaps for OLED panels? If so we could use a different > > > min-value for OLED panels that don't do PWM, but use 12 for everything > > > else. > > > > All the chromebooks I have worked with LCD + LED backlight have been > > fine with a backlight set to 0. > > We do have OLED panels too, but I'm not aware of what they do. > > > > > Harry > > > > > > > Alex > > > > > > > > > > > >> > > > >> Also reviewed here: > > > >> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2748377>>> > > > >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >> > > > >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > >> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > >> index 573cf17262da..0129bd69b94e 100644 > > > >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > >> @@ -3151,7 +3151,7 @@ static int amdgpu_dm_mode_config_init(struct > > > >> amdgpu_device *adev) > > > >> return 0; > > > >> } > > > >> > > > >> -#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12 > > > >> +#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 0 > > > >> #define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255 > > > >> #define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50 > > > >> > > > >> -- > > > >> 2.31.0.291.g576ba9dcdaf-goog > > > >> > > > >> ___ > > > >> dri-devel mailing list > > > >> dri-de...@lists.freedesktop.org > > > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel>> > > > >> ___ > > > > dri-devel mailing list > > > > dri-de...@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel>> > > >
Re: [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0
On Sat, 20 Mar 2021 at 02:10, Harry Wentland wrote: > On 2021-03-19 10:22 a.m., Alex Deucher wrote: > > On Fri, Mar 19, 2021 at 3:23 AM Evan Benn wrote: > >> > >> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT was set to the value of 12 > >> to ensure no display backlight will flicker at low user brightness > >> settings. However this value is quite bright, so for devices that do not > >> implement the ACPI ATIF > >> ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS > >> functionality the user cannot set the brightness to a low level even if > >> the display would support such a low PWM. > >> > >> This ATIF feature is not implemented on for example AMD grunt chromebooks. > >> > >> Signed-off-by: Evan Benn > >> > >> --- > >> I could not find a justification for the reason for the value. It has > >> caused some noticable regression for users: > >> https://bugzilla.kernel.org/show_bug.cgi?id=203439>>> > >> Maybe this can be either user controlled or userspace configured, but > >> preventing users from turning their backlight dim seems wrong. > > > > My understanding is that some panels flicker if you set the min to a > > value too low. This was a safe minimum if the platform didn't specify > > it's own safe minimum. I think we'd just be trading one bug for > > another (flickering vs not dim enough). Maybe a whitelist or > > blacklist would be a better solution? > > > > Yeah, this is a NACK from me as-is for the reasons Alex described. Thanks Harry + Alex, I agree this solution is not the best. > > I agree a whitelist approach might be best. Do you have any idea what an allowlist could be keyed on? Is the flickering you observed here a function of the panel or the gpu or some other component? Maybe we could move the minimum level into the logic for that hardware. > > Is this fix perhaps for OLED panels? If so we could use a different > min-value for OLED panels that don't do PWM, but use 12 for everything else. All the chromebooks I have worked with LCD + LED backlight have been fine with a backlight set to 0. We do have OLED panels too, but I'm not aware of what they do. > Harry > > > Alex > > > > > >> > >> Also reviewed here: > >> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2748377>>> > >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> index 573cf17262da..0129bd69b94e 100644 > >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> @@ -3151,7 +3151,7 @@ static int amdgpu_dm_mode_config_init(struct > >> amdgpu_device *adev) > >> return 0; > >> } > >> > >> -#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12 > >> +#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 0 > >> #define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255 > >> #define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50 > >> > >> -- > >> 2.31.0.291.g576ba9dcdaf-goog > >> > >> ___ > >> dri-devel mailing list > >> dri-de...@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel>> > >> ___ > > dri-devel mailing list > > dri-de...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel>> >
[PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0
AMDGPU_DM_DEFAULT_MIN_BACKLIGHT was set to the value of 12 to ensure no display backlight will flicker at low user brightness settings. However this value is quite bright, so for devices that do not implement the ACPI ATIF ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS functionality the user cannot set the brightness to a low level even if the display would support such a low PWM. This ATIF feature is not implemented on for example AMD grunt chromebooks. Signed-off-by: Evan Benn --- I could not find a justification for the reason for the value. It has caused some noticable regression for users: https://bugzilla.kernel.org/show_bug.cgi?id=203439 Maybe this can be either user controlled or userspace configured, but preventing users from turning their backlight dim seems wrong. Also reviewed here: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2748377 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 573cf17262da..0129bd69b94e 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3151,7 +3151,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev) return 0; } -#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12 +#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 0 #define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255 #define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50 -- 2.31.0.291.g576ba9dcdaf-goog
[PATCH] scripts/coccinelle: Add script to detect sign extension
Hello, I am attempting to create a coccinelle script that will detect possibly buggy usage of the bitwise operators where integer promotion may result in bugs, usually due to sign extension. I know this script needs a lot more work, but I am just beginning to learn the syntax of coccinelle. At this stage I am mainly looking for advice if this is even worth continuing, or if I am on the wrong track entirely. Here is an example of the bug I hope to find: https://lore.kernel.org/lkml/20210317013758.ga134...@roeck-us.net/ Where ints and unsigned are mixed in bitwise operations, and the sizes differ. Thanks Evan Benn Signed-off-by: Evan Benn --- .../coccinelle/tests/int_sign_extend.cocci| 35 +++ 1 file changed, 35 insertions(+) create mode 100644 scripts/coccinelle/tests/int_sign_extend.cocci diff --git a/scripts/coccinelle/tests/int_sign_extend.cocci b/scripts/coccinelle/tests/int_sign_extend.cocci new file mode 100644 index ..bad61e37e4e7 --- /dev/null +++ b/scripts/coccinelle/tests/int_sign_extend.cocci @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: GPL-2.0-only +/// Mixing signed and unsigned types in bitwise operations risks problems when +/// the 'Usual arithmetic conversions' are applied. +/// For example: +/// https://lore.kernel.org/lkml/20210317013758.ga134...@roeck-us.net/ +/// When a signed int and an unsigned int are compared there is no problem. +/// But if the unsigned is changed to a unsigned long, for example by using BIT +/// the signed value will be sign-extended and could result in incorrect logic. +// Confidence: +// Copyright: (C) 2021 Evan Benn +// Comments: +// Options: + +virtual context +virtual org +virtual report + +@r@ +position p; +{int} s; +{unsigned long} u; +@@ +s@p & u + +@script:python depends on org@ +p << r.p; +@@ + +cocci.print_main("sign extension when comparing bits of signed and unsigned values", p) + +@script:python depends on report@ +p << r.p; +@@ + +coccilib.report.print_report(p[0],"sign extension when comparing bits of signed and unsigned values") -- 2.31.0.291.g576ba9dcdaf-goog
Re: [PATCH 2/2] drivers/clocksource/mediatek: Ack and disable interrupts on shutdown
Hello, There is a suspend failure on mt8173 chromebooks that use this timer. The failure shows as an errno: -95 failure with none device. I tracked this down to the arm trusted firmware aborting the suspend due to this timer having a pending IRQ, due to not being disabled during suspend / clockevents_shutdown. Also reviewed here vs the 4.19 chromeos kernel branch: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2766504 Thanks Evan Benn On Thu, Mar 18, 2021 at 4:05 PM Evan Benn wrote: > > set_state_shutdown is called during system suspend after interrupts have > been disabled. If the timer has fired in the meantime, there will be > a pending IRQ. So we ack that now and disable the timer. Without this > ARM trusted firmware will abort the suspend due to the pending > interrupt. > > Now always disable the IRQ in state transitions, and re-enable in > set_periodic and next_event. > > Signed-off-by: Evan Benn > --- > > drivers/clocksource/timer-mediatek-mt6577.c | 49 + > 1 file changed, 30 insertions(+), 19 deletions(-) > > diff --git a/drivers/clocksource/timer-mediatek-mt6577.c > b/drivers/clocksource/timer-mediatek-mt6577.c > index 9e5241d1876d..44598121585c 100644 > --- a/drivers/clocksource/timer-mediatek-mt6577.c > +++ b/drivers/clocksource/timer-mediatek-mt6577.c > @@ -54,13 +54,33 @@ static u64 notrace mtk_gpt_read_sched_clock(void) > return readl_relaxed(gpt_sched_reg); > } > > +static void mtk_gpt_disable_ack_interrupts(struct timer_of *to, u8 timer) > +{ > + u32 val; > + > + /* Disable interrupts */ > + val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); > + writel(val & ~GPT_IRQ_ENABLE(timer), timer_of_base(to) + > + GPT_IRQ_EN_REG); > + > + /* Ack interrupts */ > + writel(GPT_IRQ_ACK(timer), timer_of_base(to) + GPT_IRQ_ACK_REG); > +} > + > static void mtk_gpt_clkevt_time_stop(struct timer_of *to, u8 timer) > { > u32 val; > > + /* Disable timer */ > val = readl(timer_of_base(to) + GPT_CTRL_REG(timer)); > writel(val & ~GPT_CTRL_ENABLE, timer_of_base(to) + >GPT_CTRL_REG(timer)); > + > + /* This may be called with interrupts disabled, > +* so we need to ack any interrupt that is pending > +* Or for example ATF will prevent a suspend from completing. > +*/ > + mtk_gpt_disable_ack_interrupts(to, timer); > } > > static void mtk_gpt_clkevt_time_setup(struct timer_of *to, > @@ -74,8 +94,10 @@ static void mtk_gpt_clkevt_time_start(struct timer_of *to, > { > u32 val; > > - /* Acknowledge interrupt */ > - writel(GPT_IRQ_ACK(timer), timer_of_base(to) + GPT_IRQ_ACK_REG); > + /* Enable interrupts */ > + val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); > + writel(val | GPT_IRQ_ENABLE(timer), > + timer_of_base(to) + GPT_IRQ_EN_REG); > > val = readl(timer_of_base(to) + GPT_CTRL_REG(timer)); > > @@ -148,21 +170,6 @@ __init mtk_gpt_setup(struct timer_of *to, u8 timer, u8 > option) >timer_of_base(to) + GPT_CTRL_REG(timer)); > } > > -static void mtk_gpt_enable_irq(struct timer_of *to, u8 timer) > -{ > - u32 val; > - > - /* Disable all interrupts */ > - writel(0x0, timer_of_base(to) + GPT_IRQ_EN_REG); > - > - /* Acknowledge all spurious pending interrupts */ > - writel(0x3f, timer_of_base(to) + GPT_IRQ_ACK_REG); > - > - val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); > - writel(val | GPT_IRQ_ENABLE(timer), > - timer_of_base(to) + GPT_IRQ_EN_REG); > -} > - > static struct timer_of to = { > .flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK, > > @@ -193,6 +200,12 @@ static int __init mtk_gpt_init(struct device_node *node) > if (ret) > return ret; > > + /* In case the firmware left the interrupts enabled > +* disable and ack those now > +*/ > + mtk_gpt_disable_ack_interrupts(&to, TIMER_CLK_SRC); > + mtk_gpt_disable_ack_interrupts(&to, TIMER_CLK_EVT); > + > /* Configure clock source */ > mtk_gpt_setup(&to, TIMER_CLK_SRC, GPT_CTRL_OP_FREERUN); > clocksource_mmio_init(timer_of_base(&to) + GPT_CNT_REG(TIMER_CLK_SRC), > @@ -206,8 +219,6 @@ static int __init mtk_gpt_init(struct device_node *node) > clockevents_config_and_register(&to.clkevt, timer_of_rate(&to), > TIMER_SYNC_TICKS, 0x); > > - mtk_gpt_enable_irq(&to, TIMER_CLK_EVT); > - > return 0; > } > TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init); > -- > 2.31.0.rc2.261.g7f71774620-goog >
[PATCH 2/2] drivers/clocksource/mediatek: Ack and disable interrupts on shutdown
set_state_shutdown is called during system suspend after interrupts have been disabled. If the timer has fired in the meantime, there will be a pending IRQ. So we ack that now and disable the timer. Without this ARM trusted firmware will abort the suspend due to the pending interrupt. Now always disable the IRQ in state transitions, and re-enable in set_periodic and next_event. Signed-off-by: Evan Benn --- drivers/clocksource/timer-mediatek-mt6577.c | 49 + 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/drivers/clocksource/timer-mediatek-mt6577.c b/drivers/clocksource/timer-mediatek-mt6577.c index 9e5241d1876d..44598121585c 100644 --- a/drivers/clocksource/timer-mediatek-mt6577.c +++ b/drivers/clocksource/timer-mediatek-mt6577.c @@ -54,13 +54,33 @@ static u64 notrace mtk_gpt_read_sched_clock(void) return readl_relaxed(gpt_sched_reg); } +static void mtk_gpt_disable_ack_interrupts(struct timer_of *to, u8 timer) +{ + u32 val; + + /* Disable interrupts */ + val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); + writel(val & ~GPT_IRQ_ENABLE(timer), timer_of_base(to) + + GPT_IRQ_EN_REG); + + /* Ack interrupts */ + writel(GPT_IRQ_ACK(timer), timer_of_base(to) + GPT_IRQ_ACK_REG); +} + static void mtk_gpt_clkevt_time_stop(struct timer_of *to, u8 timer) { u32 val; + /* Disable timer */ val = readl(timer_of_base(to) + GPT_CTRL_REG(timer)); writel(val & ~GPT_CTRL_ENABLE, timer_of_base(to) + GPT_CTRL_REG(timer)); + + /* This may be called with interrupts disabled, +* so we need to ack any interrupt that is pending +* Or for example ATF will prevent a suspend from completing. +*/ + mtk_gpt_disable_ack_interrupts(to, timer); } static void mtk_gpt_clkevt_time_setup(struct timer_of *to, @@ -74,8 +94,10 @@ static void mtk_gpt_clkevt_time_start(struct timer_of *to, { u32 val; - /* Acknowledge interrupt */ - writel(GPT_IRQ_ACK(timer), timer_of_base(to) + GPT_IRQ_ACK_REG); + /* Enable interrupts */ + val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); + writel(val | GPT_IRQ_ENABLE(timer), + timer_of_base(to) + GPT_IRQ_EN_REG); val = readl(timer_of_base(to) + GPT_CTRL_REG(timer)); @@ -148,21 +170,6 @@ __init mtk_gpt_setup(struct timer_of *to, u8 timer, u8 option) timer_of_base(to) + GPT_CTRL_REG(timer)); } -static void mtk_gpt_enable_irq(struct timer_of *to, u8 timer) -{ - u32 val; - - /* Disable all interrupts */ - writel(0x0, timer_of_base(to) + GPT_IRQ_EN_REG); - - /* Acknowledge all spurious pending interrupts */ - writel(0x3f, timer_of_base(to) + GPT_IRQ_ACK_REG); - - val = readl(timer_of_base(to) + GPT_IRQ_EN_REG); - writel(val | GPT_IRQ_ENABLE(timer), - timer_of_base(to) + GPT_IRQ_EN_REG); -} - static struct timer_of to = { .flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK, @@ -193,6 +200,12 @@ static int __init mtk_gpt_init(struct device_node *node) if (ret) return ret; + /* In case the firmware left the interrupts enabled +* disable and ack those now +*/ + mtk_gpt_disable_ack_interrupts(&to, TIMER_CLK_SRC); + mtk_gpt_disable_ack_interrupts(&to, TIMER_CLK_EVT); + /* Configure clock source */ mtk_gpt_setup(&to, TIMER_CLK_SRC, GPT_CTRL_OP_FREERUN); clocksource_mmio_init(timer_of_base(&to) + GPT_CNT_REG(TIMER_CLK_SRC), @@ -206,8 +219,6 @@ static int __init mtk_gpt_init(struct device_node *node) clockevents_config_and_register(&to.clkevt, timer_of_rate(&to), TIMER_SYNC_TICKS, 0x); - mtk_gpt_enable_irq(&to, TIMER_CLK_EVT); - return 0; } TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init); -- 2.31.0.rc2.261.g7f71774620-goog
[PATCH 1/2] drivers/clocksource/mediatek: Split mediatek drivers into 2 files
mtk_gpt and mtk_syst drivers for mt6577 and mt6765 devices were not sharing any code. So split them into separate files. Signed-off-by: Evan Benn --- arch/arm/mach-mediatek/Kconfig| 3 +- arch/arm64/Kconfig.platforms | 3 +- drivers/clocksource/Kconfig | 13 +- drivers/clocksource/Makefile | 3 +- ...mer-mediatek.c => timer-mediatek-mt6577.c} | 100 - drivers/clocksource/timer-mediatek-mt6765.c | 135 ++ 6 files changed, 151 insertions(+), 106 deletions(-) rename drivers/clocksource/{timer-mediatek.c => timer-mediatek-mt6577.c} (69%) create mode 100644 drivers/clocksource/timer-mediatek-mt6765.c diff --git a/arch/arm/mach-mediatek/Kconfig b/arch/arm/mach-mediatek/Kconfig index 9e0f592d87d8..8686f992c4b6 100644 --- a/arch/arm/mach-mediatek/Kconfig +++ b/arch/arm/mach-mediatek/Kconfig @@ -4,7 +4,8 @@ menuconfig ARCH_MEDIATEK depends on ARCH_MULTI_V7 select ARM_GIC select PINCTRL - select MTK_TIMER + select TIMER_MEDIATEK_MT6577 + select TIMER_MEDIATEK_MT6765 select MFD_SYSCON help Support for Mediatek MT65xx & MT81xx SoCs diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms index cdfd5fed457f..d4752375ab0b 100644 --- a/arch/arm64/Kconfig.platforms +++ b/arch/arm64/Kconfig.platforms @@ -161,7 +161,8 @@ config ARCH_MEDIATEK bool "MediaTek SoC Family" select ARM_GIC select PINCTRL - select MTK_TIMER + select TIMER_MEDIATEK_MT6577 + select TIMER_MEDIATEK_MT6765 help This enables support for MediaTek MT27xx, MT65xx, MT76xx & MT81xx ARMv8 SoCs diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index 39aa21d01e05..d697c799284e 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -438,13 +438,20 @@ config OXNAS_RPS_TIMER config SYS_SUPPORTS_SH_CMT bool -config MTK_TIMER - bool "Mediatek timer driver" if COMPILE_TEST +config TIMER_MEDIATEK_MT6577 + bool "Mediatek mt6577 timer driver" if COMPILE_TEST depends on HAS_IOMEM select TIMER_OF select CLKSRC_MMIO help - Support for Mediatek timer driver. + Enables clocksource and clockevent driver for Mediatek mt6577 timer. + +config TIMER_MEDIATEK_MT6765 + bool "Mediatek mt6765 timer driver" if COMPILE_TEST + depends on HAS_IOMEM + select TIMER_OF + help + Enables clockevent driver for Mediatek mt6765 timer. config SPRD_TIMER bool "Spreadtrum timer driver" if EXPERT diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile index c17ee32a7151..b1f06ce114f9 100644 --- a/drivers/clocksource/Makefile +++ b/drivers/clocksource/Makefile @@ -49,7 +49,8 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM) += samsung_pwm_timer.o obj-$(CONFIG_FSL_FTM_TIMER)+= timer-fsl-ftm.o obj-$(CONFIG_VF_PIT_TIMER) += timer-vf-pit.o obj-$(CONFIG_CLKSRC_QCOM) += timer-qcom.o -obj-$(CONFIG_MTK_TIMER)+= timer-mediatek.o +obj-$(CONFIG_TIMER_MEDIATEK_MT6577)+= timer-mediatek-mt6577.o +obj-$(CONFIG_TIMER_MEDIATEK_MT6765)+= timer-mediatek-mt6765.o obj-$(CONFIG_CLKSRC_PISTACHIO) += timer-pistachio.o obj-$(CONFIG_CLKSRC_TI_32K)+= timer-ti-32k.o obj-$(CONFIG_OXNAS_RPS_TIMER) += timer-oxnas-rps.o diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek-mt6577.c similarity index 69% rename from drivers/clocksource/timer-mediatek.c rename to drivers/clocksource/timer-mediatek-mt6577.c index 9318edcd8963..9e5241d1876d 100644 --- a/drivers/clocksource/timer-mediatek.c +++ b/drivers/clocksource/timer-mediatek-mt6577.c @@ -47,86 +47,8 @@ #define GPT_CNT_REG(val)(0x08 + (0x10 * (val))) #define GPT_CMP_REG(val)(0x0C + (0x10 * (val))) -/* system timer */ -#define SYST_BASE (0x40) - -#define SYST_CON(SYST_BASE + 0x0) -#define SYST_VAL(SYST_BASE + 0x4) - -#define SYST_CON_REG(to)(timer_of_base(to) + SYST_CON) -#define SYST_VAL_REG(to)(timer_of_base(to) + SYST_VAL) - -/* - * SYST_CON_EN: Clock enable. Shall be set to - * - Start timer countdown. - * - Allow timeout ticks being updated. - * - Allow changing interrupt functions. - * - * SYST_CON_IRQ_EN: Set to allow interrupt. - * - * SYST_CON_IRQ_CLR: Set to clear interrupt. - */ -#define SYST_CON_EN BIT(0) -#define SYST_CON_IRQ_EN BIT(1) -#define SYST_CON_IRQ_CLR BIT(4) - static void __iomem *gpt_sched_reg __read_mostly; -static void mtk_syst_ack_irq(struct timer_of *to) -{ - /* Clear and disable interrupt */ - writel(SYST_CON_IRQ_CLR | SYST_CON_EN, SYST_CON_REG(to)); -} - -static irqreturn_t mtk_syst_handler(int irq, void *dev_id) -{ - struct
[PATCH 2/2] platform/chrome: cros_ec_proto: Add LID and BATTERY to default mask
After 'platform/chrome: cros_ec_proto: Use EC_HOST_EVENT_MASK not BIT' some of the flags are not quite correct. LID_CLOSED is used to suspend the device, so it makes sense to ignore that. BATTERY events are also frequent and causing spurious wakes on elm/hana mt8173 devices. Fixes: c214e564acb2ad9463293ab9c109bfdae91fbeaf Signed-off-by: Evan Benn --- drivers/platform/chrome/cros_ec_proto.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c index 4e442175612d..ea5149efcbea 100644 --- a/drivers/platform/chrome/cros_ec_proto.c +++ b/drivers/platform/chrome/cros_ec_proto.c @@ -526,9 +526,11 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev) * power), not wake up. */ ec_dev->host_event_wake_mask = U32_MAX & - ~(EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_DISCONNECTED) | + ~(EC_HOST_EVENT_MASK(EC_HOST_EVENT_LID_CLOSED) | + EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_DISCONNECTED) | EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_LOW) | EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_CRITICAL) | + EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY) | EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU) | EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_STATUS)); /* -- 2.20.1
[PATCH 1/2] platform/chrome: cros_ec_proto: Use EC_HOST_EVENT_MASK not BIT
The host_event_code enum is 1-based, use EC_HOST_EVENT_MASK not BIT to generate the intended mask. This patch changes the behaviour of the mask, a following patch will restore the intended behaviour: 'Add LID and BATTERY to default mask' Fixes: c214e564acb2ad9463293ab9c109bfdae91fbeaf Signed-off-by: Evan Benn --- drivers/platform/chrome/cros_ec_proto.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c index 0ecee8b8773d..4e442175612d 100644 --- a/drivers/platform/chrome/cros_ec_proto.c +++ b/drivers/platform/chrome/cros_ec_proto.c @@ -526,11 +526,11 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev) * power), not wake up. */ ec_dev->host_event_wake_mask = U32_MAX & - ~(BIT(EC_HOST_EVENT_AC_DISCONNECTED) | - BIT(EC_HOST_EVENT_BATTERY_LOW) | - BIT(EC_HOST_EVENT_BATTERY_CRITICAL) | - BIT(EC_HOST_EVENT_PD_MCU) | - BIT(EC_HOST_EVENT_BATTERY_STATUS)); + ~(EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_DISCONNECTED) | + EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_LOW) | + EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_CRITICAL) | + EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU) | + EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_STATUS)); /* * Old ECs may not support this command. Complain about all * other errors. -- 2.20.1
[PATCH] media: mtk-vcodec: Fix order of log arguments
Signed-off-by: Evan Benn --- drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c index a3c7a380c9308..785ec0df445ec 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c @@ -27,11 +27,11 @@ int mtk_vcodec_wait_for_done_ctx(struct mtk_vcodec_ctx *ctx, int command, if (!ret) { status = -1;/* timeout */ - mtk_v4l2_err("[%d] cmd=%d, ctx->type=%d, wait_event_interruptible_timeout time=%ums out %d %d!", + mtk_v4l2_err("[%d] ctx->type=%d, cmd=%d, wait_event_interruptible_timeout time=%ums out %d %d!", ctx->id, ctx->type, command, timeout_ms, ctx->int_cond, ctx->int_type); } else if (-ERESTARTSYS == ret) { - mtk_v4l2_err("[%d] cmd=%d, ctx->type=%d, wait_event_interruptible_timeout interrupted by a signal %d %d", + mtk_v4l2_err("[%d] ctx->type=%d, cmd=%d, wait_event_interruptible_timeout interrupted by a signal %d %d", ctx->id, ctx->type, command, ctx->int_cond, ctx->int_type); status = -1; -- 2.29.2.454.gaff20da3a2-goog
Re: [PATCH] MAINTAINERS: rectify entry in ARM SMC WATCHDOG DRIVER
AFAICT this has now been merged upstream, I'm not sure what action to take: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5c24a28b4eb842ad1256496be6ae01bab15f1dcb https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=72a9e7fea5866fc471fda78f05f166595c8c6ba6 On Wed, Jun 3, 2020 at 9:22 AM Evan Benn wrote: > > Apologies for that slip up. > > Reviewed-by: Evan Benn > > On Wed, Jun 3, 2020 at 6:16 AM Julius Werner wrote: > > > > Reviewed-by: Julius Werner
Re: [PATCH] MAINTAINERS: rectify entry in ARM SMC WATCHDOG DRIVER
Apologies for that slip up. Reviewed-by: Evan Benn On Wed, Jun 3, 2020 at 6:16 AM Julius Werner wrote: > > Reviewed-by: Julius Werner
[PATCH v6 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.
This is currently supported in firmware deployed on oak, hana and elm mt8173 chromebook devices. The kernel driver is written to be a generic SMC watchdog driver. Arm Trusted Firmware upstreaming review: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405 Patch to add oak, hana, elm device tree: https://lore.kernel.org/linux-arm-kernel/20200110073730.213789-1-hsi...@chromium.org/ I would like to add the device tree support after the above patch is accepted. Changes in v6: - Don't use dt default - Use default arm,smc-id value if non provided by dt Changes in v5: - Change compatible to arm,smc-wdt - Make timeleft return 0 on error - Use type punning on smc_func_id to save an alloc - Change compatible to arm,smc-wdt Changes in v4: - Add arm,smc-id property - Get smc-id from of property - Return a1 instead of a0 in timeleft Changes in v3: - Change name back to arm - Add optional get_timeleft op - change name to arm_smc_wdt Changes in v2: - Change name arm > mt8173 - use watchdog_stop_on_reboot - use watchdog_stop_on_unregister - use devm_watchdog_register_device - remove smcwd_shutdown, smcwd_remove - change error codes Evan Benn (1): dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog Julius Werner (1): watchdog: Add new arm_smc_wdt watchdog driver .../bindings/watchdog/arm-smc-wdt.yaml| 37 MAINTAINERS | 7 + arch/arm64/configs/defconfig | 1 + drivers/watchdog/Kconfig | 13 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/arm_smc_wdt.c| 188 ++ 6 files changed, 247 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml create mode 100644 drivers/watchdog/arm_smc_wdt.c -- 2.26.2.526.g744177e7f7-goog
[PATCH v6 2/2] watchdog: Add new arm_smc_wdt watchdog driver
From: Julius Werner This patch adds a watchdog driver that can be used on ARM systems with the appropriate watchdog implemented in Secure Monitor firmware. The driver communicates with firmware via a Secure Monitor Call. This may be useful for platforms using TrustZone that want the Secure Monitor firmware to have the final control over the watchdog. This is implemented on mt8173 chromebook devices oak, elm and hana in arm trusted firmware file plat/mediatek/mt8173/drivers/wdt/wdt.c. Signed-off-by: Julius Werner Signed-off-by: Evan Benn --- Changes in v6: - Use default arm,smc-id value if non provided by dt Changes in v5: - Make timeleft return 0 on error - Use type punning on smc_func_id to save an alloc - Change compatible to arm,smc-wdt Changes in v4: - Get smc-id from of property - Return a1 instead of a0 in timeleft Changes in v3: - Add optional get_timeleft op - change name to arm_smc_wdt Changes in v2: - use watchdog_stop_on_reboot - use watchdog_stop_on_unregister - use devm_watchdog_register_device - remove smcwd_shutdown, smcwd_remove - change error codes MAINTAINERS| 1 + arch/arm64/configs/defconfig | 1 + drivers/watchdog/Kconfig | 13 +++ drivers/watchdog/Makefile | 1 + drivers/watchdog/arm_smc_wdt.c | 188 + 5 files changed, 204 insertions(+) create mode 100644 drivers/watchdog/arm_smc_wdt.c diff --git a/MAINTAINERS b/MAINTAINERS index 0f2b39767bfa9..2b782bbff200a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1462,6 +1462,7 @@ M:Julius Werner R: Evan Benn S: Maintained F: devicetree/bindings/watchdog/arm-smc-wdt.yaml +F: drivers/watchdog/arm_smc_wdt.c ARM SMMU DRIVERS M: Will Deacon diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 24e534d850454..0619df80f7575 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -513,6 +513,7 @@ CONFIG_UNIPHIER_THERMAL=y CONFIG_WATCHDOG=y CONFIG_ARM_SP805_WATCHDOG=y CONFIG_ARM_SBSA_WATCHDOG=y +CONFIG_ARM_SMC_WATCHDOG=y CONFIG_S3C2410_WATCHDOG=y CONFIG_DW_WATCHDOG=y CONFIG_SUNXI_WATCHDOG=m diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 0663c604bd642..c440b576d23bf 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -867,6 +867,19 @@ config DIGICOLOR_WATCHDOG To compile this driver as a module, choose M here: the module will be called digicolor_wdt. +config ARM_SMC_WATCHDOG + tristate "ARM Secure Monitor Call based watchdog support" + depends on ARM || ARM64 + depends on OF + depends on HAVE_ARM_SMCCC + select WATCHDOG_CORE + help + Say Y here to include support for a watchdog timer + implemented by the EL3 Secure Monitor on ARM platforms. + Requires firmware support. + To compile this driver as a module, choose M here: the + module will be called arm_smc_wdt. + config LPC18XX_WATCHDOG tristate "LPC18xx/43xx Watchdog" depends on ARCH_LPC18XX || COMPILE_TEST diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 6de2e4ceef190..97bed1d3d97cb 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -94,6 +94,7 @@ obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o obj-$(CONFIG_RTD119X_WATCHDOG) += rtd119x_wdt.o obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o +obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o # X86 (i386 + ia64 + x86_64) Architecture obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c new file mode 100644 index 0..8f3d0c3a005fb --- /dev/null +++ b/drivers/watchdog/arm_smc_wdt.c @@ -0,0 +1,188 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * ARM Secure Monitor Call watchdog driver + * + * Copyright 2020 Google LLC. + * Julius Werner + * Based on mtk_wdt.c + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define DRV_NAME "arm_smc_wdt" +#define DRV_VERSION"1.0" + +enum smcwd_call { + SMCWD_INIT = 0, + SMCWD_SET_TIMEOUT = 1, + SMCWD_ENABLE= 2, + SMCWD_PET = 3, + SMCWD_GET_TIMELEFT = 4, +}; + +static bool nowayout = WATCHDOG_NOWAYOUT; +static unsigned int timeout; + +static int smcwd_call(struct watchdog_device *wdd, enum smcwd_call call, + unsigned long arg, struct arm_smccc_res *res) +{ + struct arm_smccc_res local_res; + + if (!res) + res = &local_res; + + arm_smccc_smc((u32)(uintptr_t)watchdog_get_drvdata(wdd), call, arg, 0, + 0, 0, 0, 0, res); + + if (res->a0 == PSCI_RET_NOT_SUPPORTED) + return -ENODEV; + if (res->a0 == PSCI_RET_INVALID_
[PATCH v6 1/2] dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog
This watchdog can be used on ARM systems with a Secure Monitor firmware to forward watchdog operations to firmware via a Secure Monitor Call. Signed-off-by: Evan Benn --- Changes in v6: - Don't use dt default Changes in v5: - Change compatible to arm,smc-wdt Changes in v4: - Add arm,smc-id property Changes in v3: - Change name back to arm Changes in v2: - Change name arm > mt8173 .../bindings/watchdog/arm-smc-wdt.yaml| 37 +++ MAINTAINERS | 6 +++ 2 files changed, 43 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml diff --git a/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml new file mode 100644 index 0..bec651541e0c8 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml @@ -0,0 +1,37 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/watchdog/arm-smc-wdt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ARM Secure Monitor Call based watchdog + +allOf: + - $ref: "watchdog.yaml#" + +maintainers: + - Julius Werner + +properties: + compatible: +enum: + - arm,smc-wdt + arm,smc-id: +allOf: + - $ref: /schemas/types.yaml#/definitions/uint32 +description: | + The ATF smc function id used by the firmware. + Defaults to 0x82003D06 if unset. + +required: + - compatible + +examples: + - | +watchdog { + compatible = "arm,smc-wdt"; + arm,smc-id = <0x82003D06>; + timeout-sec = <15>; +}; + +... diff --git a/MAINTAINERS b/MAINTAINERS index b816a453b10eb..0f2b39767bfa9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1457,6 +1457,12 @@ S: Maintained F: Documentation/devicetree/bindings/interrupt-controller/arm,vic.txt F: drivers/irqchip/irq-vic.c +ARM SMC WATCHDOG DRIVER +M: Julius Werner +R: Evan Benn +S: Maintained +F: devicetree/bindings/watchdog/arm-smc-wdt.yaml + ARM SMMU DRIVERS M: Will Deacon R: Robin Murphy -- 2.26.2.526.g744177e7f7-goog