[PATCH V3 0/9] ARM: bcm2835: Implement initial S2Idle for Raspberry Pi

[PATCH V3 0/9] ARM: bcm2835: Implement initial S2Idle for Raspberry Pi

[Stefan Wahren]

This series implement the initial S2Idle support for
the Raspberry Pi, which was a long time on my TODO list [1]. The
changes allow to suspend and resume the Raspberry Pi via debug UART.
The focus is on the BCM2835 SoC, because it's less complex than its
successors and have enough documentation.

The series can be roughly separated in 3 parts:
 1. base patches (1, 9) which allows S2Idle support for BCM2835
 2. drm vc4 patches (2 - 6)  which implement S2Idle support
 3. dwc2 patches (7, 8) which handle BCM2835 specific issues

Cherry-picking of patches should be fine.

Test steps:
- configure debug console (pl011 or mini UART) as wakeup source
- send system to idle state

  echo freeze > /sys/power/state

- wakeup system by console traffic

The DWC2 part based on an idea of Doug Anderson and its implementation
should be mostly finished now, but still RFC. The USB domain is now powered
down and the USB devices are still usable after resume. There is still room
for improvements, but at least the system won't freeze forever as before.

Here are some figures for the Raspberry Pi 1 (without any
devices connected except of a debug UART):

running but CPU idle = 1.67 W
S2Idle               = 1.33 W

In comparison with HDMI & USB keyboard connected (but neither active
nor wakeup source):

running but CPU idle = 1.82 W
S2Idle               = 1.33 W

The series has been successfully tested on the following platforms:
Raspberry Pi 1 B
Raspberry Pi 3 B+

Changes in V3:
- added Reviewed-by & Acked-by from Florian & Maíra
- dropped applied pmdomain & bcm2835aux patches
- address comments by Maíra (patch 3 & 5)
- replace old USB recovery patch with canary approach [3], which should
  work with other platforms

Changes in V2:
- rebased against todays mainline
- added Reviewed-by from Florian
- added Acked-by from Minas
- dropped "irqchip/bcm2835: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND"
  because it has been applied by Thomas Gleixner
- dropped "pmdomain: raspberrypi-power: Avoid powering down USB"
  because this workaround has been replaced by patch 14
- use drm_err_once instead of DRM_ERROR and return connector_status_unknown
  in patch 6
- add new patch in order to clean-up all DRM_ERROR
- add new patch to improve raspberrypi-power logging
- add new patch to simplify V3D clock retrieval
- add new patch 5 to avoid power down of wakeup devices
- add new patch 12 to avoid confusion about ACPI ID of BCM283x USB
- add new patch 8 & 10 which address the problem that HDMI
  is not functional after s2idle
- add more links and fix typo in patch 13
- add new WIP patch 14 which recover DWC2 register after power down
- take care of UART clock in patch 15 as commented by Florian
- use SYSTEM_SLEEP_PM_OPS in patch 15

[1] - https://github.com/lategoodbye/rpi-zero/issues/9
[2] - https://bugzilla.redhat.com/show_bug.cgi?id=2283978
[3] - https://lore.kernel.org/linux-usb/CAD=FV=W7sdi1+SHfhY6RrjK32r8iAGe4w+O_u5Sp982vgBU6EQ@mail.gmail.com/

Stefan Wahren (9):
  mailbox: bcm2835: Fix timeout during suspend mode
  drm/vc4: hdmi: Handle error case of pm_runtime_resume_and_get
  drm/vc4: Get the rid of DRM_ERROR()
  drm/vc4: hdmi: add PM suspend/resume support
  drm/vc4: v3d: simplify clock retrieval
  drm/vc4: v3d: add PM suspend/resume support
  usb: dwc2: Refactor backup/restore of registers
  usb: dwc2: Implement recovery after PM domain off
  ARM: bcm2835_defconfig: Enable SUSPEND

 arch/arm/configs/bcm2835_defconfig |   2 -
 drivers/gpu/drm/vc4/vc4_bo.c       |  14 ++--
 drivers/gpu/drm/vc4/vc4_dpi.c      |  14 ++--
 drivers/gpu/drm/vc4/vc4_dsi.c      |  32 +++++----
 drivers/gpu/drm/vc4/vc4_gem.c      |  11 ++--
 drivers/gpu/drm/vc4/vc4_hdmi.c     |  70 ++++++++++++++------
 drivers/gpu/drm/vc4/vc4_hvs.c      |   4 +-
 drivers/gpu/drm/vc4/vc4_irq.c      |   2 +-
 drivers/gpu/drm/vc4/vc4_v3d.c      |  26 +++-----
 drivers/gpu/drm/vc4/vc4_validate.c |   8 +--
 drivers/gpu/drm/vc4/vc4_vec.c      |  10 +--
 drivers/mailbox/bcm2835-mailbox.c  |   3 +-
 drivers/usb/dwc2/core.c            |   1 +
 drivers/usb/dwc2/core.h            |  14 ++++
 drivers/usb/dwc2/gadget.c          | 101 +++++++++++++++--------------
 drivers/usb/dwc2/hcd.c             |  99 ++++++++++++++--------------
 drivers/usb/dwc2/platform.c        |  38 +++++++++++
 17 files changed, 265 insertions(+), 184 deletions(-)

--
2.34.1

[PATCH V3 1/9] mailbox: bcm2835: Fix timeout during suspend mode

[Stefan Wahren]

During noirq suspend phase the Raspberry Pi power driver suffer of
firmware property timeouts. The reason is that the IRQ of the underlying
BCM2835 mailbox is disabled and rpi_firmware_property_list() will always
run into a timeout [1].

Since the VideoCore side isn't consider as a wakeup source, set the
IRQF_NO_SUSPEND flag for the mailbox IRQ in order to keep it enabled
during suspend-resume cycle.

[1]
PM: late suspend of devices complete after 1.754 msecs
WARNING: CPU: 0 PID: 438 at drivers/firmware/raspberrypi.c:128
 rpi_firmware_property_list+0x204/0x22c
Firmware transaction 0x00028001 timeout
Modules linked in:
CPU: 0 PID: 438 Comm: bash Tainted: G         C         6.9.3-dirty #17
Hardware name: BCM2835
Call trace:
unwind_backtrace from show_stack+0x18/0x1c
show_stack from dump_stack_lvl+0x34/0x44
dump_stack_lvl from __warn+0x88/0xec
__warn from warn_slowpath_fmt+0x7c/0xb0
warn_slowpath_fmt from rpi_firmware_property_list+0x204/0x22c
rpi_firmware_property_list from rpi_firmware_property+0x68/0x8c
rpi_firmware_property from rpi_firmware_set_power+0x54/0xc0
rpi_firmware_set_power from _genpd_power_off+0xe4/0x148
_genpd_power_off from genpd_sync_power_off+0x7c/0x11c
genpd_sync_power_off from genpd_finish_suspend+0xcc/0xe0
genpd_finish_suspend from dpm_run_callback+0x78/0xd0
dpm_run_callback from device_suspend_noirq+0xc0/0x238
device_suspend_noirq from dpm_suspend_noirq+0xb0/0x168
dpm_suspend_noirq from suspend_devices_and_enter+0x1b8/0x5ac
suspend_devices_and_enter from pm_suspend+0x254/0x2e4
pm_suspend from state_store+0xa8/0xd4
state_store from kernfs_fop_write_iter+0x154/0x1a0
kernfs_fop_write_iter from vfs_write+0x12c/0x184
vfs_write from ksys_write+0x78/0xc0
ksys_write from ret_fast_syscall+0x0/0x54
Exception stack(0xcc93dfa8 to 0xcc93dff0)
[...]
PM: noirq suspend of devices complete after 3095.584 msecs

Link: https://github.com/raspberrypi/firmware/issues/1894
Fixes: 0bae6af6d704 ("mailbox: Enable BCM2835 mailbox support")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/mailbox/bcm2835-mailbox.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
index fbfd0202047c..ea12fb8d2401 100644
--- a/drivers/mailbox/bcm2835-mailbox.c
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -145,7 +145,8 @@ static int bcm2835_mbox_probe(struct platform_device *pdev)
 	spin_lock_init(&mbox->lock);

 	ret = devm_request_irq(dev, irq_of_parse_and_map(dev->of_node, 0),
-			       bcm2835_mbox_irq, 0, dev_name(dev), mbox);
+			       bcm2835_mbox_irq, IRQF_NO_SUSPEND, dev_name(dev),
+			       mbox);
 	if (ret) {
 		dev_err(dev, "Failed to register a mailbox IRQ handler: %d\n",
 			ret);
--
2.34.1

Re: [PATCH V3 1/9] mailbox: bcm2835: Fix timeout during suspend mode

[Stefan Wahren]

Hi Jassi,

Am 21.08.24 um 23:40 schrieb Stefan Wahren:
> During noirq suspend phase the Raspberry Pi power driver suffer of
> firmware property timeouts. The reason is that the IRQ of the underlying
> BCM2835 mailbox is disabled and rpi_firmware_property_list() will always
> run into a timeout [1].
>
> Since the VideoCore side isn't consider as a wakeup source, set the
> IRQF_NO_SUSPEND flag for the mailbox IRQ in order to keep it enabled
> during suspend-resume cycle.
>
> [1]
> PM: late suspend of devices complete after 1.754 msecs
> WARNING: CPU: 0 PID: 438 at drivers/firmware/raspberrypi.c:128
>   rpi_firmware_property_list+0x204/0x22c
> Firmware transaction 0x00028001 timeout
> Modules linked in:
> CPU: 0 PID: 438 Comm: bash Tainted: G         C         6.9.3-dirty #17
> Hardware name: BCM2835
> Call trace:
> unwind_backtrace from show_stack+0x18/0x1c
> show_stack from dump_stack_lvl+0x34/0x44
> dump_stack_lvl from __warn+0x88/0xec
> __warn from warn_slowpath_fmt+0x7c/0xb0
> warn_slowpath_fmt from rpi_firmware_property_list+0x204/0x22c
> rpi_firmware_property_list from rpi_firmware_property+0x68/0x8c
> rpi_firmware_property from rpi_firmware_set_power+0x54/0xc0
> rpi_firmware_set_power from _genpd_power_off+0xe4/0x148
> _genpd_power_off from genpd_sync_power_off+0x7c/0x11c
> genpd_sync_power_off from genpd_finish_suspend+0xcc/0xe0
> genpd_finish_suspend from dpm_run_callback+0x78/0xd0
> dpm_run_callback from device_suspend_noirq+0xc0/0x238
> device_suspend_noirq from dpm_suspend_noirq+0xb0/0x168
> dpm_suspend_noirq from suspend_devices_and_enter+0x1b8/0x5ac
> suspend_devices_and_enter from pm_suspend+0x254/0x2e4
> pm_suspend from state_store+0xa8/0xd4
> state_store from kernfs_fop_write_iter+0x154/0x1a0
> kernfs_fop_write_iter from vfs_write+0x12c/0x184
> vfs_write from ksys_write+0x78/0xc0
> ksys_write from ret_fast_syscall+0x0/0x54
> Exception stack(0xcc93dfa8 to 0xcc93dff0)
> [...]
> PM: noirq suspend of devices complete after 3095.584 msecs
>
> Link: https://github.com/raspberrypi/firmware/issues/1894
> Fixes: 0bae6af6d704 ("mailbox: Enable BCM2835 mailbox support")
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
gentle ping
> ---
>   drivers/mailbox/bcm2835-mailbox.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
> index fbfd0202047c..ea12fb8d2401 100644
> --- a/drivers/mailbox/bcm2835-mailbox.c
> +++ b/drivers/mailbox/bcm2835-mailbox.c
> @@ -145,7 +145,8 @@ static int bcm2835_mbox_probe(struct platform_device *pdev)
>   	spin_lock_init(&mbox->lock);
>
>   	ret = devm_request_irq(dev, irq_of_parse_and_map(dev->of_node, 0),
> -			       bcm2835_mbox_irq, 0, dev_name(dev), mbox);
> +			       bcm2835_mbox_irq, IRQF_NO_SUSPEND, dev_name(dev),
> +			       mbox);
>   	if (ret) {
>   		dev_err(dev, "Failed to register a mailbox IRQ handler: %d\n",
>   			ret);
> --
> 2.34.1
>

Re: [PATCH V3 1/9] mailbox: bcm2835: Fix timeout during suspend mode

[Jassi Brar]

On Sat, Aug 31, 2024 at 4:19 AM Stefan Wahren <wahrenst@gmx.net> wrote:
>
> Hi Jassi,
>
> Am 21.08.24 um 23:40 schrieb Stefan Wahren:
> > During noirq suspend phase the Raspberry Pi power driver suffer of
> > firmware property timeouts. The reason is that the IRQ of the underlying
> > BCM2835 mailbox is disabled and rpi_firmware_property_list() will always
> > run into a timeout [1].
> >
> > Since the VideoCore side isn't consider as a wakeup source, set the
> > IRQF_NO_SUSPEND flag for the mailbox IRQ in order to keep it enabled
> > during suspend-resume cycle.
> >
> > [1]
> > PM: late suspend of devices complete after 1.754 msecs
> > WARNING: CPU: 0 PID: 438 at drivers/firmware/raspberrypi.c:128
> >   rpi_firmware_property_list+0x204/0x22c
> > Firmware transaction 0x00028001 timeout
> > Modules linked in:
> > CPU: 0 PID: 438 Comm: bash Tainted: G         C         6.9.3-dirty #17
> > Hardware name: BCM2835
> > Call trace:
> > unwind_backtrace from show_stack+0x18/0x1c
> > show_stack from dump_stack_lvl+0x34/0x44
> > dump_stack_lvl from __warn+0x88/0xec
> > __warn from warn_slowpath_fmt+0x7c/0xb0
> > warn_slowpath_fmt from rpi_firmware_property_list+0x204/0x22c
> > rpi_firmware_property_list from rpi_firmware_property+0x68/0x8c
> > rpi_firmware_property from rpi_firmware_set_power+0x54/0xc0
> > rpi_firmware_set_power from _genpd_power_off+0xe4/0x148
> > _genpd_power_off from genpd_sync_power_off+0x7c/0x11c
> > genpd_sync_power_off from genpd_finish_suspend+0xcc/0xe0
> > genpd_finish_suspend from dpm_run_callback+0x78/0xd0
> > dpm_run_callback from device_suspend_noirq+0xc0/0x238
> > device_suspend_noirq from dpm_suspend_noirq+0xb0/0x168
> > dpm_suspend_noirq from suspend_devices_and_enter+0x1b8/0x5ac
> > suspend_devices_and_enter from pm_suspend+0x254/0x2e4
> > pm_suspend from state_store+0xa8/0xd4
> > state_store from kernfs_fop_write_iter+0x154/0x1a0
> > kernfs_fop_write_iter from vfs_write+0x12c/0x184
> > vfs_write from ksys_write+0x78/0xc0
> > ksys_write from ret_fast_syscall+0x0/0x54
> > Exception stack(0xcc93dfa8 to 0xcc93dff0)
> > [...]
> > PM: noirq suspend of devices complete after 3095.584 msecs
> >
> > Link: https://github.com/raspberrypi/firmware/issues/1894
> > Fixes: 0bae6af6d704 ("mailbox: Enable BCM2835 mailbox support")
> > Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> gentle ping

This sounds like a fix but also a part of 9 patches update. Do you
want this merged as a bugfix now or into the next window.

thanks

Re: [PATCH V3 1/9] mailbox: bcm2835: Fix timeout during suspend mode

[Stefan Wahren]

Hi Jassi,

Am 01.09.24 um 22:26 schrieb Jassi Brar:
> On Sat, Aug 31, 2024 at 4:19 AM Stefan Wahren <wahrenst@gmx.net> wrote:
>> Hi Jassi,
>>
>> Am 21.08.24 um 23:40 schrieb Stefan Wahren:
>>> During noirq suspend phase the Raspberry Pi power driver suffer of
>>> firmware property timeouts. The reason is that the IRQ of the underlying
>>> BCM2835 mailbox is disabled and rpi_firmware_property_list() will always
>>> run into a timeout [1].
>>>
>>> Since the VideoCore side isn't consider as a wakeup source, set the
>>> IRQF_NO_SUSPEND flag for the mailbox IRQ in order to keep it enabled
>>> during suspend-resume cycle.
>>>
>>> [1]
>>> PM: late suspend of devices complete after 1.754 msecs
>>> WARNING: CPU: 0 PID: 438 at drivers/firmware/raspberrypi.c:128
>>>    rpi_firmware_property_list+0x204/0x22c
>>> Firmware transaction 0x00028001 timeout
>>> Modules linked in:
>>> CPU: 0 PID: 438 Comm: bash Tainted: G         C         6.9.3-dirty #17
>>> Hardware name: BCM2835
>>> Call trace:
>>> unwind_backtrace from show_stack+0x18/0x1c
>>> show_stack from dump_stack_lvl+0x34/0x44
>>> dump_stack_lvl from __warn+0x88/0xec
>>> __warn from warn_slowpath_fmt+0x7c/0xb0
>>> warn_slowpath_fmt from rpi_firmware_property_list+0x204/0x22c
>>> rpi_firmware_property_list from rpi_firmware_property+0x68/0x8c
>>> rpi_firmware_property from rpi_firmware_set_power+0x54/0xc0
>>> rpi_firmware_set_power from _genpd_power_off+0xe4/0x148
>>> _genpd_power_off from genpd_sync_power_off+0x7c/0x11c
>>> genpd_sync_power_off from genpd_finish_suspend+0xcc/0xe0
>>> genpd_finish_suspend from dpm_run_callback+0x78/0xd0
>>> dpm_run_callback from device_suspend_noirq+0xc0/0x238
>>> device_suspend_noirq from dpm_suspend_noirq+0xb0/0x168
>>> dpm_suspend_noirq from suspend_devices_and_enter+0x1b8/0x5ac
>>> suspend_devices_and_enter from pm_suspend+0x254/0x2e4
>>> pm_suspend from state_store+0xa8/0xd4
>>> state_store from kernfs_fop_write_iter+0x154/0x1a0
>>> kernfs_fop_write_iter from vfs_write+0x12c/0x184
>>> vfs_write from ksys_write+0x78/0xc0
>>> ksys_write from ret_fast_syscall+0x0/0x54
>>> Exception stack(0xcc93dfa8 to 0xcc93dff0)
>>> [...]
>>> PM: noirq suspend of devices complete after 3095.584 msecs
>>>
>>> Link: https://github.com/raspberrypi/firmware/issues/1894
>>> Fixes: 0bae6af6d704 ("mailbox: Enable BCM2835 mailbox support")
>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>>> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> gentle ping
> This sounds like a fix but also a part of 9 patches update. Do you
> want this merged as a bugfix now or into the next window.
there is no dependency to the rest of the series. Since this is late in
the 6.11 cycle, i'm fine with merging it for the next window.

Thanks
>
> thanks

[PATCH V3 2/9] drm/vc4: hdmi: Handle error case of pm_runtime_resume_and_get

[Stefan Wahren]

The commit 0f5251339eda ("drm/vc4: hdmi: Make sure the controller is
powered in detect") introduced the necessary power management handling
to avoid register access while controller is powered down.
Unfortunately it just print a warning if pm_runtime_resume_and_get()
fails and proceed anyway.

This could happen during suspend to idle. So we must assume it is unsafe
to access the HDMI register. So bail out properly.

Fixes: 0f5251339eda ("drm/vc4: hdmi: Make sure the controller is powered in detect")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
Acked-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index d57c4a5948c8..cb424604484f 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -429,6 +429,7 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector,
 {
 	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
 	enum drm_connector_status status = connector_status_disconnected;
+	int ret;

 	/*
 	 * NOTE: This function should really take vc4_hdmi->mutex, but
@@ -441,7 +442,12 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector,
 	 * the lock for now.
 	 */

-	WARN_ON(pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev));
+	ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);
+	if (ret) {
+		drm_err_once(connector->dev, "Failed to retain HDMI power domain: %d\n",
+			     ret);
+		return connector_status_unknown;
+	}

 	if (vc4_hdmi->hpd_gpio) {
 		if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio))
--
2.34.1

Re: [PATCH V3 2/9] drm/vc4: hdmi: Handle error case of pm_runtime_resume_and_get

[Maíra Canal]

Hi Stefan,

On 8/21/24 18:40, Stefan Wahren wrote:
> The commit 0f5251339eda ("drm/vc4: hdmi: Make sure the controller is
> powered in detect") introduced the necessary power management handling
> to avoid register access while controller is powered down.
> Unfortunately it just print a warning if pm_runtime_resume_and_get()
> fails and proceed anyway.
> 
> This could happen during suspend to idle. So we must assume it is unsafe
> to access the HDMI register. So bail out properly.
> 
> Fixes: 0f5251339eda ("drm/vc4: hdmi: Make sure the controller is powered in detect")
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> Reviewed-by: Maíra Canal <mcanal@igalia.com>
> Acked-by: Maxime Ripard <mripard@kernel.org>

Applied to drm/kernel/drm-misc-next!

Best Regards,
- Maíra

> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index d57c4a5948c8..cb424604484f 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -429,6 +429,7 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector,
>   {
>   	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
>   	enum drm_connector_status status = connector_status_disconnected;
> +	int ret;
> 
>   	/*
>   	 * NOTE: This function should really take vc4_hdmi->mutex, but
> @@ -441,7 +442,12 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector,
>   	 * the lock for now.
>   	 */
> 
> -	WARN_ON(pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev));
> +	ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);
> +	if (ret) {
> +		drm_err_once(connector->dev, "Failed to retain HDMI power domain: %d\n",
> +			     ret);
> +		return connector_status_unknown;
> +	}
> 
>   	if (vc4_hdmi->hpd_gpio) {
>   		if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio))
> --
> 2.34.1
> 

[PATCH V3 3/9] drm/vc4: Get the rid of DRM_ERROR()

[Stefan Wahren]

DRM_ERROR() has been deprecated in favor of pr_err(). However, we
should prefer to use drm_err() whenever possible so we get device-
specific output with the error message. In error case of kcalloc,
we can simply drop DRM_ERROR(), because kcalloc already logs errors.

Suggested-by: Maíra Canal <mcanal@igalia.com>
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vc4/vc4_bo.c       | 14 ++++++------
 drivers/gpu/drm/vc4/vc4_dpi.c      | 14 ++++++------
 drivers/gpu/drm/vc4/vc4_dsi.c      | 32 ++++++++++++++------------
 drivers/gpu/drm/vc4/vc4_gem.c      | 11 +++++----
 drivers/gpu/drm/vc4/vc4_hdmi.c     | 36 +++++++++++++++---------------
 drivers/gpu/drm/vc4/vc4_hvs.c      |  4 ++--
 drivers/gpu/drm/vc4/vc4_irq.c      |  2 +-
 drivers/gpu/drm/vc4/vc4_v3d.c      |  6 ++---
 drivers/gpu/drm/vc4/vc4_validate.c |  8 +++----
 drivers/gpu/drm/vc4/vc4_vec.c      | 10 ++++-----
 10 files changed, 70 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index 86d629e45307..3f72be7490d5 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -469,7 +469,7 @@ struct vc4_bo *vc4_bo_create(struct drm_device *dev, size_t unaligned_size,

 	if (IS_ERR(dma_obj)) {
 		struct drm_printer p = drm_info_printer(vc4->base.dev);
-		DRM_ERROR("Failed to allocate from GEM DMA helper:\n");
+		drm_err(dev, "Failed to allocate from GEM DMA helper:\n");
 		vc4_bo_stats_print(&p, vc4);
 		return ERR_PTR(-ENOMEM);
 	}
@@ -702,7 +702,7 @@ static struct dma_buf *vc4_prime_export(struct drm_gem_object *obj, int flags)
 	 */
 	ret = vc4_bo_inc_usecnt(bo);
 	if (ret) {
-		DRM_ERROR("Failed to increment BO usecnt\n");
+		drm_err(obj->dev, "Failed to increment BO usecnt\n");
 		return ERR_PTR(ret);
 	}

@@ -1050,10 +1050,10 @@ static void vc4_bo_cache_destroy(struct drm_device *dev, void *unused)

 	for (i = 0; i < vc4->num_labels; i++) {
 		if (vc4->bo_labels[i].num_allocated) {
-			DRM_ERROR("Destroying BO cache with %d %s "
-				  "BOs still allocated\n",
-				  vc4->bo_labels[i].num_allocated,
-				  vc4->bo_labels[i].name);
+			drm_err(dev, "Destroying BO cache with %d %s "
+				"BOs still allocated\n",
+				vc4->bo_labels[i].num_allocated,
+				vc4->bo_labels[i].name);
 		}

 		if (is_user_label(i))
@@ -1083,7 +1083,7 @@ int vc4_label_bo_ioctl(struct drm_device *dev, void *data,

 	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
 	if (!gem_obj) {
-		DRM_ERROR("Failed to look up GEM BO %d\n", args->handle);
+		drm_err(dev, "Failed to look up GEM BO %d\n", args->handle);
 		kfree(name);
 		return -ENOENT;
 	}
diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
index 39152e755a13..a382dc4654bd 100644
--- a/drivers/gpu/drm/vc4/vc4_dpi.c
+++ b/drivers/gpu/drm/vc4/vc4_dpi.c
@@ -199,8 +199,8 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder)
 						       DPI_FORMAT);
 				break;
 			default:
-				DRM_ERROR("Unknown media bus format %d\n",
-					  bus_format);
+				drm_err(dev, "Unknown media bus format %d\n",
+					bus_format);
 				break;
 			}
 		}
@@ -236,11 +236,11 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder)

 	ret = clk_set_rate(dpi->pixel_clock, mode->clock * 1000);
 	if (ret)
-		DRM_ERROR("Failed to set clock rate: %d\n", ret);
+		drm_err(dev, "Failed to set clock rate: %d\n", ret);

 	ret = clk_prepare_enable(dpi->pixel_clock);
 	if (ret)
-		DRM_ERROR("Failed to set clock rate: %d\n", ret);
+		drm_err(dev, "Failed to set clock rate: %d\n", ret);

 	drm_dev_exit(idx);
 }
@@ -339,7 +339,7 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data)
 	if (IS_ERR(dpi->core_clock)) {
 		ret = PTR_ERR(dpi->core_clock);
 		if (ret != -EPROBE_DEFER)
-			DRM_ERROR("Failed to get core clock: %d\n", ret);
+			drm_err(drm, "Failed to get core clock: %d\n", ret);
 		return ret;
 	}

@@ -347,13 +347,13 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data)
 	if (IS_ERR(dpi->pixel_clock)) {
 		ret = PTR_ERR(dpi->pixel_clock);
 		if (ret != -EPROBE_DEFER)
-			DRM_ERROR("Failed to get pixel clock: %d\n", ret);
+			drm_err(drm, "Failed to get pixel clock: %d\n", ret);
 		return ret;
 	}

 	ret = clk_prepare_enable(dpi->core_clock);
 	if (ret) {
-		DRM_ERROR("Failed to turn on core clock: %d\n", ret);
+		drm_err(drm, "Failed to turn on core clock: %d\n", ret);
 		return ret;
 	}

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 46f6c4ce61c5..f5ccc1bf7a63 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -613,6 +613,7 @@ struct vc4_dsi {
 static inline void
 dsi_dma_workaround_write(struct vc4_dsi *dsi, u32 offset, u32 val)
 {
+	struct drm_device *drm = dsi->bridge.dev;
 	struct dma_chan *chan = dsi->reg_dma_chan;
 	struct dma_async_tx_descriptor *tx;
 	dma_cookie_t cookie;
@@ -633,19 +634,19 @@ dsi_dma_workaround_write(struct vc4_dsi *dsi, u32 offset, u32 val)
 						  dsi->reg_dma_paddr,
 						  4, 0);
 	if (!tx) {
-		DRM_ERROR("Failed to set up DMA register write\n");
+		drm_err(drm, "Failed to set up DMA register write\n");
 		return;
 	}

 	cookie = tx->tx_submit(tx);
 	ret = dma_submit_error(cookie);
 	if (ret) {
-		DRM_ERROR("Failed to submit DMA: %d\n", ret);
+		drm_err(drm, "Failed to submit DMA: %d\n", ret);
 		return;
 	}
 	ret = dma_sync_wait(chan, cookie);
 	if (ret)
-		DRM_ERROR("Failed to wait for DMA: %d\n", ret);
+		drm_err(drm, "Failed to wait for DMA: %d\n", ret);
 }

 #define DSI_READ(offset)								\
@@ -893,7 +894,7 @@ static void vc4_dsi_bridge_pre_enable(struct drm_bridge *bridge,

 	ret = pm_runtime_resume_and_get(dev);
 	if (ret) {
-		DRM_ERROR("Failed to runtime PM enable on DSI%d\n", dsi->variant->port);
+		drm_err(bridge->dev, "Failed to runtime PM enable on DSI%d\n", dsi->variant->port);
 		return;
 	}

@@ -986,13 +987,14 @@ static void vc4_dsi_bridge_pre_enable(struct drm_bridge *bridge,

 	ret = clk_prepare_enable(dsi->escape_clock);
 	if (ret) {
-		DRM_ERROR("Failed to turn on DSI escape clock: %d\n", ret);
+		drm_err(bridge->dev, "Failed to turn on DSI escape clock: %d\n",
+			ret);
 		return;
 	}

 	ret = clk_prepare_enable(dsi->pll_phy_clock);
 	if (ret) {
-		DRM_ERROR("Failed to turn on DSI PLL: %d\n", ret);
+		drm_err(bridge->dev, "Failed to turn on DSI PLL: %d\n", ret);
 		return;
 	}

@@ -1014,7 +1016,7 @@ static void vc4_dsi_bridge_pre_enable(struct drm_bridge *bridge,

 	ret = clk_prepare_enable(dsi->pixel_clock);
 	if (ret) {
-		DRM_ERROR("Failed to turn on DSI pixel clock: %d\n", ret);
+		drm_err(bridge->dev, "Failed to turn on DSI pixel clock: %d\n", ret);
 		return;
 	}

@@ -1172,6 +1174,7 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
 				     const struct mipi_dsi_msg *msg)
 {
 	struct vc4_dsi *dsi = host_to_dsi(host);
+	struct drm_device *drm = dsi->bridge.dev;
 	struct mipi_dsi_packet packet;
 	u32 pkth = 0, pktc = 0;
 	int i, ret;
@@ -1303,8 +1306,8 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
 						  DSI_RXPKT1H_BC_PARAM);

 			if (rxlen != msg->rx_len) {
-				DRM_ERROR("DSI returned %db, expecting %db\n",
-					  rxlen, (int)msg->rx_len);
+				drm_err(drm, "DSI returned %db, expecting %db\n",
+					rxlen, (int)msg->rx_len);
 				ret = -ENXIO;
 				goto reset_fifo_and_return;
 			}
@@ -1326,7 +1329,7 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
 	return ret;

 reset_fifo_and_return:
-	DRM_ERROR("DSI transfer failed, resetting: %d\n", ret);
+	drm_err(drm, "DSI transfer failed, resetting: %d\n", ret);

 	DSI_PORT_WRITE(TXPKT1C, DSI_PORT_READ(TXPKT1C) & ~DSI_TXPKT1C_CMD_EN);
 	udelay(1);
@@ -1468,7 +1471,8 @@ static void dsi_handle_error(struct vc4_dsi *dsi,
 	if (!(stat & bit))
 		return;

-	DRM_ERROR("DSI%d: %s error\n", dsi->variant->port, type);
+	drm_err(dsi->bridge.dev, "DSI%d: %s error\n", dsi->variant->port,
+		type);
 	*ret = IRQ_HANDLED;
 }

@@ -1687,7 +1691,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 						      &dsi->reg_dma_paddr,
 						      GFP_KERNEL);
 		if (!dsi->reg_dma_mem) {
-			DRM_ERROR("Failed to get DMA memory\n");
+			drm_err(drm, "Failed to get DMA memory\n");
 			return -ENOMEM;
 		}

@@ -1702,8 +1706,8 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 		if (IS_ERR(dsi->reg_dma_chan)) {
 			ret = PTR_ERR(dsi->reg_dma_chan);
 			if (ret != -EPROBE_DEFER)
-				DRM_ERROR("Failed to get DMA channel: %d\n",
-					  ret);
+				drm_err(drm, "Failed to get DMA channel: %d\n",
+					ret);
 			return ret;
 		}

diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 03648f954985..24fb1b57e1dd 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -832,8 +832,8 @@ vc4_get_bcl(struct drm_device *dev, struct vc4_exec_info *exec)
 	 */
 	temp = kvmalloc_array(temp_size, 1, GFP_KERNEL);
 	if (!temp) {
-		DRM_ERROR("Failed to allocate storage for copying "
-			  "in bin/render CLs.\n");
+		drm_err(dev, "Failed to allocate storage for copying "
+			"in bin/render CLs.\n");
 		ret = -ENOMEM;
 		goto fail;
 	}
@@ -866,7 +866,7 @@ vc4_get_bcl(struct drm_device *dev, struct vc4_exec_info *exec)

 	bo = vc4_bo_create(dev, exec_size, true, VC4_BO_TYPE_BCL);
 	if (IS_ERR(bo)) {
-		DRM_ERROR("Couldn't allocate BO for binning\n");
+		drm_err(dev, "Couldn't allocate BO for binning\n");
 		ret = PTR_ERR(bo);
 		goto fail;
 	}
@@ -1153,10 +1153,9 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
 	}

 	exec = kcalloc(1, sizeof(*exec), GFP_KERNEL);
-	if (!exec) {
-		DRM_ERROR("malloc failure on exec struct\n");
+	if (!exec)
 		return -ENOMEM;
-	}
+
 	exec->dev = vc4;

 	ret = vc4_v3d_pm_get(vc4);
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index cb424604484f..6611ab7c26a6 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -704,7 +704,7 @@ static int vc4_hdmi_write_infoframe(struct drm_connector *connector,

 	ret = vc4_hdmi_stop_packet(vc4_hdmi, type, true);
 	if (ret) {
-		DRM_ERROR("Failed to wait for infoframe to go idle: %d\n", ret);
+		drm_err(drm, "Failed to wait for infoframe to go idle: %d\n", ret);
 		goto out;
 	}

@@ -740,7 +740,7 @@ static int vc4_hdmi_write_infoframe(struct drm_connector *connector,
 	ret = wait_for((HDMI_READ(HDMI_RAM_PACKET_STATUS) &
 			BIT(packet_id)), 100);
 	if (ret)
-		DRM_ERROR("Failed to wait for infoframe to start: %d\n", ret);
+		drm_err(drm, "Failed to wait for infoframe to start: %d\n", ret);

 out:
 	drm_dev_exit(idx);
@@ -901,7 +901,7 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder,

 	ret = pm_runtime_put(&vc4_hdmi->pdev->dev);
 	if (ret < 0)
-		DRM_ERROR("Failed to release power domain: %d\n", ret);
+		drm_err(drm, "Failed to release power domain: %d\n", ret);

 	drm_dev_exit(idx);

@@ -1443,7 +1443,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,

 	ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);
 	if (ret < 0) {
-		DRM_ERROR("Failed to retain power domain: %d\n", ret);
+		drm_err(drm, "Failed to retain power domain: %d\n", ret);
 		goto err_dev_exit;
 	}

@@ -1468,19 +1468,19 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 			 div_u64(tmds_char_rate, 100) * 101);
 	ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate);
 	if (ret) {
-		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
+		drm_err(drm, "Failed to set HSM clock rate: %d\n", ret);
 		goto err_put_runtime_pm;
 	}

 	ret = clk_set_rate(vc4_hdmi->pixel_clock, tmds_char_rate);
 	if (ret) {
-		DRM_ERROR("Failed to set pixel clock rate: %d\n", ret);
+		drm_err(drm, "Failed to set pixel clock rate: %d\n", ret);
 		goto err_put_runtime_pm;
 	}

 	ret = clk_prepare_enable(vc4_hdmi->pixel_clock);
 	if (ret) {
-		DRM_ERROR("Failed to turn on pixel clock: %d\n", ret);
+		drm_err(drm, "Failed to turn on pixel clock: %d\n", ret);
 		goto err_put_runtime_pm;
 	}

@@ -1496,13 +1496,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,

 	ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, bvb_rate);
 	if (ret) {
-		DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret);
+		drm_err(drm, "Failed to set pixel bvb clock rate: %d\n", ret);
 		goto err_disable_pixel_clock;
 	}

 	ret = clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
 	if (ret) {
-		DRM_ERROR("Failed to turn on pixel bvb clock: %d\n", ret);
+		drm_err(drm, "Failed to turn on pixel bvb clock: %d\n", ret);
 		goto err_disable_pixel_clock;
 	}

@@ -2951,13 +2951,13 @@ static int vc4_hdmi_init_resources(struct drm_device *drm,
 	if (IS_ERR(vc4_hdmi->pixel_clock)) {
 		ret = PTR_ERR(vc4_hdmi->pixel_clock);
 		if (ret != -EPROBE_DEFER)
-			DRM_ERROR("Failed to get pixel clock\n");
+			drm_err(drm, "Failed to get pixel clock\n");
 		return ret;
 	}

 	vc4_hdmi->hsm_clock = devm_clk_get(dev, "hdmi");
 	if (IS_ERR(vc4_hdmi->hsm_clock)) {
-		DRM_ERROR("Failed to get HDMI state machine clock\n");
+		drm_err(drm, "Failed to get HDMI state machine clock\n");
 		return PTR_ERR(vc4_hdmi->hsm_clock);
 	}
 	vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock;
@@ -3041,31 +3041,31 @@ static int vc5_hdmi_init_resources(struct drm_device *drm,

 	vc4_hdmi->hsm_clock = devm_clk_get(dev, "hdmi");
 	if (IS_ERR(vc4_hdmi->hsm_clock)) {
-		DRM_ERROR("Failed to get HDMI state machine clock\n");
+		drm_err(drm, "Failed to get HDMI state machine clock\n");
 		return PTR_ERR(vc4_hdmi->hsm_clock);
 	}

 	vc4_hdmi->pixel_bvb_clock = devm_clk_get(dev, "bvb");
 	if (IS_ERR(vc4_hdmi->pixel_bvb_clock)) {
-		DRM_ERROR("Failed to get pixel bvb clock\n");
+		drm_err(drm, "Failed to get pixel bvb clock\n");
 		return PTR_ERR(vc4_hdmi->pixel_bvb_clock);
 	}

 	vc4_hdmi->audio_clock = devm_clk_get(dev, "audio");
 	if (IS_ERR(vc4_hdmi->audio_clock)) {
-		DRM_ERROR("Failed to get audio clock\n");
+		drm_err(drm, "Failed to get audio clock\n");
 		return PTR_ERR(vc4_hdmi->audio_clock);
 	}

 	vc4_hdmi->cec_clock = devm_clk_get(dev, "cec");
 	if (IS_ERR(vc4_hdmi->cec_clock)) {
-		DRM_ERROR("Failed to get CEC clock\n");
+		drm_err(drm, "Failed to get CEC clock\n");
 		return PTR_ERR(vc4_hdmi->cec_clock);
 	}

 	vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
 	if (IS_ERR(vc4_hdmi->reset)) {
-		DRM_ERROR("Failed to get HDMI reset line\n");
+		drm_err(drm, "Failed to get HDMI reset line\n");
 		return PTR_ERR(vc4_hdmi->reset);
 	}

@@ -3221,14 +3221,14 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)

 	ddc_node = of_parse_phandle(dev->of_node, "ddc", 0);
 	if (!ddc_node) {
-		DRM_ERROR("Failed to find ddc node in device tree\n");
+		drm_err(drm, "Failed to find ddc node in device tree\n");
 		return -ENODEV;
 	}

 	vc4_hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
 	of_node_put(ddc_node);
 	if (!vc4_hdmi->ddc) {
-		DRM_DEBUG("Failed to get ddc i2c adapter by node\n");
+		drm_err(drm, "Failed to get ddc i2c adapter by node\n");
 		return -EPROBE_DEFER;
 	}

diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
index 04af672caacb..2a835a5cff9d 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -191,8 +191,8 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,

 	ret = drm_mm_insert_node(&hvs->dlist_mm, space, VC4_KERNEL_DWORDS);
 	if (ret) {
-		DRM_ERROR("Failed to allocate space for filter kernel: %d\n",
-			  ret);
+		drm_err(&hvs->vc4->base, "Failed to allocate space for filter kernel: %d\n",
+			ret);
 		return ret;
 	}

diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 563b3dfeb9b9..ef93d8e22a35 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -76,7 +76,7 @@ vc4_overflow_mem_work(struct work_struct *work)

 	bin_bo_slot = vc4_v3d_get_bin_slot(vc4);
 	if (bin_bo_slot < 0) {
-		DRM_ERROR("Couldn't allocate binner overflow mem\n");
+		drm_err(&vc4->base, "Couldn't allocate binner overflow mem\n");
 		goto complete;
 	}

diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 04ac7805e6d5..6e566584afbf 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -471,8 +471,8 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
 		return ret;

 	if (V3D_READ(V3D_IDENT0) != V3D_EXPECTED_IDENT0) {
-		DRM_ERROR("V3D_IDENT0 read 0x%08x instead of 0x%08x\n",
-			  V3D_READ(V3D_IDENT0), V3D_EXPECTED_IDENT0);
+		drm_err(drm, "V3D_IDENT0 read 0x%08x instead of 0x%08x\n",
+			V3D_READ(V3D_IDENT0), V3D_EXPECTED_IDENT0);
 		ret = -EINVAL;
 		goto err_put_runtime_pm;
 	}
@@ -485,7 +485,7 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)

 	ret = vc4_irq_install(drm, vc4->irq);
 	if (ret) {
-		DRM_ERROR("Failed to install IRQ handler\n");
+		drm_err(drm, "Failed to install IRQ handler\n");
 		goto err_put_runtime_pm;
 	}

diff --git a/drivers/gpu/drm/vc4/vc4_validate.c b/drivers/gpu/drm/vc4/vc4_validate.c
index 7dff3ca5af6b..0c17284bf6f5 100644
--- a/drivers/gpu/drm/vc4/vc4_validate.c
+++ b/drivers/gpu/drm/vc4/vc4_validate.c
@@ -65,7 +65,7 @@ utile_width(int cpp)
 	case 8:
 		return 2;
 	default:
-		DRM_ERROR("unknown cpp: %d\n", cpp);
+		pr_err("unknown cpp: %d\n", cpp);
 		return 1;
 	}
 }
@@ -82,7 +82,7 @@ utile_height(int cpp)
 	case 8:
 		return 4;
 	default:
-		DRM_ERROR("unknown cpp: %d\n", cpp);
+		pr_err("unknown cpp: %d\n", cpp);
 		return 1;
 	}
 }
@@ -390,8 +390,8 @@ validate_tile_binning_config(VALIDATE_ARGS)
 	bin_slot = vc4_v3d_get_bin_slot(vc4);
 	if (bin_slot < 0) {
 		if (bin_slot != -EINTR && bin_slot != -ERESTARTSYS) {
-			DRM_ERROR("Failed to allocate binner memory: %d\n",
-				  bin_slot);
+			drm_err(dev, "Failed to allocate binner memory: %d\n",
+				bin_slot);
 		}
 		return bin_slot;
 	}
diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index 070813b8aff8..eb64e881051e 100644
--- a/drivers/gpu/drm/vc4/vc4_vec.c
+++ b/drivers/gpu/drm/vc4/vc4_vec.c
@@ -557,7 +557,7 @@ static void vc4_vec_encoder_disable(struct drm_encoder *encoder,

 	ret = pm_runtime_put(&vec->pdev->dev);
 	if (ret < 0) {
-		DRM_ERROR("Failed to release power domain: %d\n", ret);
+		drm_err(drm, "Failed to release power domain: %d\n", ret);
 		goto err_dev_exit;
 	}

@@ -591,7 +591,7 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder,

 	ret = pm_runtime_resume_and_get(&vec->pdev->dev);
 	if (ret < 0) {
-		DRM_ERROR("Failed to retain power domain: %d\n", ret);
+		drm_err(drm, "Failed to retain power domain: %d\n", ret);
 		goto err_dev_exit;
 	}

@@ -604,13 +604,13 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder,
 	 */
 	ret = clk_set_rate(vec->clock, 108000000);
 	if (ret) {
-		DRM_ERROR("Failed to set clock rate: %d\n", ret);
+		drm_err(drm, "Failed to set clock rate: %d\n", ret);
 		goto err_put_runtime_pm;
 	}

 	ret = clk_prepare_enable(vec->clock);
 	if (ret) {
-		DRM_ERROR("Failed to turn on core clock: %d\n", ret);
+		drm_err(drm, "Failed to turn on core clock: %d\n", ret);
 		goto err_put_runtime_pm;
 	}

@@ -806,7 +806,7 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data)
 	if (IS_ERR(vec->clock)) {
 		ret = PTR_ERR(vec->clock);
 		if (ret != -EPROBE_DEFER)
-			DRM_ERROR("Failed to get clock: %d\n", ret);
+			drm_err(drm, "Failed to get clock: %d\n", ret);
 		return ret;
 	}

--
2.34.1

Re: [PATCH V3 3/9] drm/vc4: Get the rid of DRM_ERROR()

[Maíra Canal]

Hi Stefan,

On 8/21/24 18:40, Stefan Wahren wrote:
> DRM_ERROR() has been deprecated in favor of pr_err(). However, we
> should prefer to use drm_err() whenever possible so we get device-
> specific output with the error message. In error case of kcalloc,
> we can simply drop DRM_ERROR(), because kcalloc already logs errors.
> 
> Suggested-by: Maíra Canal <mcanal@igalia.com>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Reviewed-by: Maíra Canal <mcanal@igalia.com>

Applied to drm/kernel/drm-misc-next!

Best Regards,
- Maíra

> ---
>   drivers/gpu/drm/vc4/vc4_bo.c       | 14 ++++++------
>   drivers/gpu/drm/vc4/vc4_dpi.c      | 14 ++++++------
>   drivers/gpu/drm/vc4/vc4_dsi.c      | 32 ++++++++++++++------------
>   drivers/gpu/drm/vc4/vc4_gem.c      | 11 +++++----
>   drivers/gpu/drm/vc4/vc4_hdmi.c     | 36 +++++++++++++++---------------
>   drivers/gpu/drm/vc4/vc4_hvs.c      |  4 ++--
>   drivers/gpu/drm/vc4/vc4_irq.c      |  2 +-
>   drivers/gpu/drm/vc4/vc4_v3d.c      |  6 ++---
>   drivers/gpu/drm/vc4/vc4_validate.c |  8 +++----
>   drivers/gpu/drm/vc4/vc4_vec.c      | 10 ++++-----
>   10 files changed, 70 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 86d629e45307..3f72be7490d5 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -469,7 +469,7 @@ struct vc4_bo *vc4_bo_create(struct drm_device *dev, size_t unaligned_size,
> 
>   	if (IS_ERR(dma_obj)) {
>   		struct drm_printer p = drm_info_printer(vc4->base.dev);
> -		DRM_ERROR("Failed to allocate from GEM DMA helper:\n");
> +		drm_err(dev, "Failed to allocate from GEM DMA helper:\n");
>   		vc4_bo_stats_print(&p, vc4);
>   		return ERR_PTR(-ENOMEM);
>   	}
> @@ -702,7 +702,7 @@ static struct dma_buf *vc4_prime_export(struct drm_gem_object *obj, int flags)
>   	 */
>   	ret = vc4_bo_inc_usecnt(bo);
>   	if (ret) {
> -		DRM_ERROR("Failed to increment BO usecnt\n");
> +		drm_err(obj->dev, "Failed to increment BO usecnt\n");
>   		return ERR_PTR(ret);
>   	}
> 
> @@ -1050,10 +1050,10 @@ static void vc4_bo_cache_destroy(struct drm_device *dev, void *unused)
> 
>   	for (i = 0; i < vc4->num_labels; i++) {
>   		if (vc4->bo_labels[i].num_allocated) {
> -			DRM_ERROR("Destroying BO cache with %d %s "
> -				  "BOs still allocated\n",
> -				  vc4->bo_labels[i].num_allocated,
> -				  vc4->bo_labels[i].name);
> +			drm_err(dev, "Destroying BO cache with %d %s "
> +				"BOs still allocated\n",
> +				vc4->bo_labels[i].num_allocated,
> +				vc4->bo_labels[i].name);
>   		}
> 
>   		if (is_user_label(i))
> @@ -1083,7 +1083,7 @@ int vc4_label_bo_ioctl(struct drm_device *dev, void *data,
> 
>   	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
>   	if (!gem_obj) {
> -		DRM_ERROR("Failed to look up GEM BO %d\n", args->handle);
> +		drm_err(dev, "Failed to look up GEM BO %d\n", args->handle);
>   		kfree(name);
>   		return -ENOENT;
>   	}
> diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
> index 39152e755a13..a382dc4654bd 100644
> --- a/drivers/gpu/drm/vc4/vc4_dpi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dpi.c
> @@ -199,8 +199,8 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder)
>   						       DPI_FORMAT);
>   				break;
>   			default:
> -				DRM_ERROR("Unknown media bus format %d\n",
> -					  bus_format);
> +				drm_err(dev, "Unknown media bus format %d\n",
> +					bus_format);
>   				break;
>   			}
>   		}
> @@ -236,11 +236,11 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder)
> 
>   	ret = clk_set_rate(dpi->pixel_clock, mode->clock * 1000);
>   	if (ret)
> -		DRM_ERROR("Failed to set clock rate: %d\n", ret);
> +		drm_err(dev, "Failed to set clock rate: %d\n", ret);
> 
>   	ret = clk_prepare_enable(dpi->pixel_clock);
>   	if (ret)
> -		DRM_ERROR("Failed to set clock rate: %d\n", ret);
> +		drm_err(dev, "Failed to set clock rate: %d\n", ret);
> 
>   	drm_dev_exit(idx);
>   }
> @@ -339,7 +339,7 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data)
>   	if (IS_ERR(dpi->core_clock)) {
>   		ret = PTR_ERR(dpi->core_clock);
>   		if (ret != -EPROBE_DEFER)
> -			DRM_ERROR("Failed to get core clock: %d\n", ret);
> +			drm_err(drm, "Failed to get core clock: %d\n", ret);
>   		return ret;
>   	}
> 
> @@ -347,13 +347,13 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data)
>   	if (IS_ERR(dpi->pixel_clock)) {
>   		ret = PTR_ERR(dpi->pixel_clock);
>   		if (ret != -EPROBE_DEFER)
> -			DRM_ERROR("Failed to get pixel clock: %d\n", ret);
> +			drm_err(drm, "Failed to get pixel clock: %d\n", ret);
>   		return ret;
>   	}
> 
>   	ret = clk_prepare_enable(dpi->core_clock);
>   	if (ret) {
> -		DRM_ERROR("Failed to turn on core clock: %d\n", ret);
> +		drm_err(drm, "Failed to turn on core clock: %d\n", ret);
>   		return ret;
>   	}
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 46f6c4ce61c5..f5ccc1bf7a63 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -613,6 +613,7 @@ struct vc4_dsi {
>   static inline void
>   dsi_dma_workaround_write(struct vc4_dsi *dsi, u32 offset, u32 val)
>   {
> +	struct drm_device *drm = dsi->bridge.dev;
>   	struct dma_chan *chan = dsi->reg_dma_chan;
>   	struct dma_async_tx_descriptor *tx;
>   	dma_cookie_t cookie;
> @@ -633,19 +634,19 @@ dsi_dma_workaround_write(struct vc4_dsi *dsi, u32 offset, u32 val)
>   						  dsi->reg_dma_paddr,
>   						  4, 0);
>   	if (!tx) {
> -		DRM_ERROR("Failed to set up DMA register write\n");
> +		drm_err(drm, "Failed to set up DMA register write\n");
>   		return;
>   	}
> 
>   	cookie = tx->tx_submit(tx);
>   	ret = dma_submit_error(cookie);
>   	if (ret) {
> -		DRM_ERROR("Failed to submit DMA: %d\n", ret);
> +		drm_err(drm, "Failed to submit DMA: %d\n", ret);
>   		return;
>   	}
>   	ret = dma_sync_wait(chan, cookie);
>   	if (ret)
> -		DRM_ERROR("Failed to wait for DMA: %d\n", ret);
> +		drm_err(drm, "Failed to wait for DMA: %d\n", ret);
>   }
> 
>   #define DSI_READ(offset)								\
> @@ -893,7 +894,7 @@ static void vc4_dsi_bridge_pre_enable(struct drm_bridge *bridge,
> 
>   	ret = pm_runtime_resume_and_get(dev);
>   	if (ret) {
> -		DRM_ERROR("Failed to runtime PM enable on DSI%d\n", dsi->variant->port);
> +		drm_err(bridge->dev, "Failed to runtime PM enable on DSI%d\n", dsi->variant->port);
>   		return;
>   	}
> 
> @@ -986,13 +987,14 @@ static void vc4_dsi_bridge_pre_enable(struct drm_bridge *bridge,
> 
>   	ret = clk_prepare_enable(dsi->escape_clock);
>   	if (ret) {
> -		DRM_ERROR("Failed to turn on DSI escape clock: %d\n", ret);
> +		drm_err(bridge->dev, "Failed to turn on DSI escape clock: %d\n",
> +			ret);
>   		return;
>   	}
> 
>   	ret = clk_prepare_enable(dsi->pll_phy_clock);
>   	if (ret) {
> -		DRM_ERROR("Failed to turn on DSI PLL: %d\n", ret);
> +		drm_err(bridge->dev, "Failed to turn on DSI PLL: %d\n", ret);
>   		return;
>   	}
> 
> @@ -1014,7 +1016,7 @@ static void vc4_dsi_bridge_pre_enable(struct drm_bridge *bridge,
> 
>   	ret = clk_prepare_enable(dsi->pixel_clock);
>   	if (ret) {
> -		DRM_ERROR("Failed to turn on DSI pixel clock: %d\n", ret);
> +		drm_err(bridge->dev, "Failed to turn on DSI pixel clock: %d\n", ret);
>   		return;
>   	}
> 
> @@ -1172,6 +1174,7 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
>   				     const struct mipi_dsi_msg *msg)
>   {
>   	struct vc4_dsi *dsi = host_to_dsi(host);
> +	struct drm_device *drm = dsi->bridge.dev;
>   	struct mipi_dsi_packet packet;
>   	u32 pkth = 0, pktc = 0;
>   	int i, ret;
> @@ -1303,8 +1306,8 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
>   						  DSI_RXPKT1H_BC_PARAM);
> 
>   			if (rxlen != msg->rx_len) {
> -				DRM_ERROR("DSI returned %db, expecting %db\n",
> -					  rxlen, (int)msg->rx_len);
> +				drm_err(drm, "DSI returned %db, expecting %db\n",
> +					rxlen, (int)msg->rx_len);
>   				ret = -ENXIO;
>   				goto reset_fifo_and_return;
>   			}
> @@ -1326,7 +1329,7 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
>   	return ret;
> 
>   reset_fifo_and_return:
> -	DRM_ERROR("DSI transfer failed, resetting: %d\n", ret);
> +	drm_err(drm, "DSI transfer failed, resetting: %d\n", ret);
> 
>   	DSI_PORT_WRITE(TXPKT1C, DSI_PORT_READ(TXPKT1C) & ~DSI_TXPKT1C_CMD_EN);
>   	udelay(1);
> @@ -1468,7 +1471,8 @@ static void dsi_handle_error(struct vc4_dsi *dsi,
>   	if (!(stat & bit))
>   		return;
> 
> -	DRM_ERROR("DSI%d: %s error\n", dsi->variant->port, type);
> +	drm_err(dsi->bridge.dev, "DSI%d: %s error\n", dsi->variant->port,
> +		type);
>   	*ret = IRQ_HANDLED;
>   }
> 
> @@ -1687,7 +1691,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>   						      &dsi->reg_dma_paddr,
>   						      GFP_KERNEL);
>   		if (!dsi->reg_dma_mem) {
> -			DRM_ERROR("Failed to get DMA memory\n");
> +			drm_err(drm, "Failed to get DMA memory\n");
>   			return -ENOMEM;
>   		}
> 
> @@ -1702,8 +1706,8 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>   		if (IS_ERR(dsi->reg_dma_chan)) {
>   			ret = PTR_ERR(dsi->reg_dma_chan);
>   			if (ret != -EPROBE_DEFER)
> -				DRM_ERROR("Failed to get DMA channel: %d\n",
> -					  ret);
> +				drm_err(drm, "Failed to get DMA channel: %d\n",
> +					ret);
>   			return ret;
>   		}
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> index 03648f954985..24fb1b57e1dd 100644
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> @@ -832,8 +832,8 @@ vc4_get_bcl(struct drm_device *dev, struct vc4_exec_info *exec)
>   	 */
>   	temp = kvmalloc_array(temp_size, 1, GFP_KERNEL);
>   	if (!temp) {
> -		DRM_ERROR("Failed to allocate storage for copying "
> -			  "in bin/render CLs.\n");
> +		drm_err(dev, "Failed to allocate storage for copying "
> +			"in bin/render CLs.\n");
>   		ret = -ENOMEM;
>   		goto fail;
>   	}
> @@ -866,7 +866,7 @@ vc4_get_bcl(struct drm_device *dev, struct vc4_exec_info *exec)
> 
>   	bo = vc4_bo_create(dev, exec_size, true, VC4_BO_TYPE_BCL);
>   	if (IS_ERR(bo)) {
> -		DRM_ERROR("Couldn't allocate BO for binning\n");
> +		drm_err(dev, "Couldn't allocate BO for binning\n");
>   		ret = PTR_ERR(bo);
>   		goto fail;
>   	}
> @@ -1153,10 +1153,9 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
>   	}
> 
>   	exec = kcalloc(1, sizeof(*exec), GFP_KERNEL);
> -	if (!exec) {
> -		DRM_ERROR("malloc failure on exec struct\n");
> +	if (!exec)
>   		return -ENOMEM;
> -	}
> +
>   	exec->dev = vc4;
> 
>   	ret = vc4_v3d_pm_get(vc4);
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index cb424604484f..6611ab7c26a6 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -704,7 +704,7 @@ static int vc4_hdmi_write_infoframe(struct drm_connector *connector,
> 
>   	ret = vc4_hdmi_stop_packet(vc4_hdmi, type, true);
>   	if (ret) {
> -		DRM_ERROR("Failed to wait for infoframe to go idle: %d\n", ret);
> +		drm_err(drm, "Failed to wait for infoframe to go idle: %d\n", ret);
>   		goto out;
>   	}
> 
> @@ -740,7 +740,7 @@ static int vc4_hdmi_write_infoframe(struct drm_connector *connector,
>   	ret = wait_for((HDMI_READ(HDMI_RAM_PACKET_STATUS) &
>   			BIT(packet_id)), 100);
>   	if (ret)
> -		DRM_ERROR("Failed to wait for infoframe to start: %d\n", ret);
> +		drm_err(drm, "Failed to wait for infoframe to start: %d\n", ret);
> 
>   out:
>   	drm_dev_exit(idx);
> @@ -901,7 +901,7 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder,
> 
>   	ret = pm_runtime_put(&vc4_hdmi->pdev->dev);
>   	if (ret < 0)
> -		DRM_ERROR("Failed to release power domain: %d\n", ret);
> +		drm_err(drm, "Failed to release power domain: %d\n", ret);
> 
>   	drm_dev_exit(idx);
> 
> @@ -1443,7 +1443,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
> 
>   	ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);
>   	if (ret < 0) {
> -		DRM_ERROR("Failed to retain power domain: %d\n", ret);
> +		drm_err(drm, "Failed to retain power domain: %d\n", ret);
>   		goto err_dev_exit;
>   	}
> 
> @@ -1468,19 +1468,19 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
>   			 div_u64(tmds_char_rate, 100) * 101);
>   	ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate);
>   	if (ret) {
> -		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
> +		drm_err(drm, "Failed to set HSM clock rate: %d\n", ret);
>   		goto err_put_runtime_pm;
>   	}
> 
>   	ret = clk_set_rate(vc4_hdmi->pixel_clock, tmds_char_rate);
>   	if (ret) {
> -		DRM_ERROR("Failed to set pixel clock rate: %d\n", ret);
> +		drm_err(drm, "Failed to set pixel clock rate: %d\n", ret);
>   		goto err_put_runtime_pm;
>   	}
> 
>   	ret = clk_prepare_enable(vc4_hdmi->pixel_clock);
>   	if (ret) {
> -		DRM_ERROR("Failed to turn on pixel clock: %d\n", ret);
> +		drm_err(drm, "Failed to turn on pixel clock: %d\n", ret);
>   		goto err_put_runtime_pm;
>   	}
> 
> @@ -1496,13 +1496,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
> 
>   	ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, bvb_rate);
>   	if (ret) {
> -		DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret);
> +		drm_err(drm, "Failed to set pixel bvb clock rate: %d\n", ret);
>   		goto err_disable_pixel_clock;
>   	}
> 
>   	ret = clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
>   	if (ret) {
> -		DRM_ERROR("Failed to turn on pixel bvb clock: %d\n", ret);
> +		drm_err(drm, "Failed to turn on pixel bvb clock: %d\n", ret);
>   		goto err_disable_pixel_clock;
>   	}
> 
> @@ -2951,13 +2951,13 @@ static int vc4_hdmi_init_resources(struct drm_device *drm,
>   	if (IS_ERR(vc4_hdmi->pixel_clock)) {
>   		ret = PTR_ERR(vc4_hdmi->pixel_clock);
>   		if (ret != -EPROBE_DEFER)
> -			DRM_ERROR("Failed to get pixel clock\n");
> +			drm_err(drm, "Failed to get pixel clock\n");
>   		return ret;
>   	}
> 
>   	vc4_hdmi->hsm_clock = devm_clk_get(dev, "hdmi");
>   	if (IS_ERR(vc4_hdmi->hsm_clock)) {
> -		DRM_ERROR("Failed to get HDMI state machine clock\n");
> +		drm_err(drm, "Failed to get HDMI state machine clock\n");
>   		return PTR_ERR(vc4_hdmi->hsm_clock);
>   	}
>   	vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock;
> @@ -3041,31 +3041,31 @@ static int vc5_hdmi_init_resources(struct drm_device *drm,
> 
>   	vc4_hdmi->hsm_clock = devm_clk_get(dev, "hdmi");
>   	if (IS_ERR(vc4_hdmi->hsm_clock)) {
> -		DRM_ERROR("Failed to get HDMI state machine clock\n");
> +		drm_err(drm, "Failed to get HDMI state machine clock\n");
>   		return PTR_ERR(vc4_hdmi->hsm_clock);
>   	}
> 
>   	vc4_hdmi->pixel_bvb_clock = devm_clk_get(dev, "bvb");
>   	if (IS_ERR(vc4_hdmi->pixel_bvb_clock)) {
> -		DRM_ERROR("Failed to get pixel bvb clock\n");
> +		drm_err(drm, "Failed to get pixel bvb clock\n");
>   		return PTR_ERR(vc4_hdmi->pixel_bvb_clock);
>   	}
> 
>   	vc4_hdmi->audio_clock = devm_clk_get(dev, "audio");
>   	if (IS_ERR(vc4_hdmi->audio_clock)) {
> -		DRM_ERROR("Failed to get audio clock\n");
> +		drm_err(drm, "Failed to get audio clock\n");
>   		return PTR_ERR(vc4_hdmi->audio_clock);
>   	}
> 
>   	vc4_hdmi->cec_clock = devm_clk_get(dev, "cec");
>   	if (IS_ERR(vc4_hdmi->cec_clock)) {
> -		DRM_ERROR("Failed to get CEC clock\n");
> +		drm_err(drm, "Failed to get CEC clock\n");
>   		return PTR_ERR(vc4_hdmi->cec_clock);
>   	}
> 
>   	vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
>   	if (IS_ERR(vc4_hdmi->reset)) {
> -		DRM_ERROR("Failed to get HDMI reset line\n");
> +		drm_err(drm, "Failed to get HDMI reset line\n");
>   		return PTR_ERR(vc4_hdmi->reset);
>   	}
> 
> @@ -3221,14 +3221,14 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
> 
>   	ddc_node = of_parse_phandle(dev->of_node, "ddc", 0);
>   	if (!ddc_node) {
> -		DRM_ERROR("Failed to find ddc node in device tree\n");
> +		drm_err(drm, "Failed to find ddc node in device tree\n");
>   		return -ENODEV;
>   	}
> 
>   	vc4_hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
>   	of_node_put(ddc_node);
>   	if (!vc4_hdmi->ddc) {
> -		DRM_DEBUG("Failed to get ddc i2c adapter by node\n");
> +		drm_err(drm, "Failed to get ddc i2c adapter by node\n");
>   		return -EPROBE_DEFER;
>   	}
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 04af672caacb..2a835a5cff9d 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -191,8 +191,8 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
> 
>   	ret = drm_mm_insert_node(&hvs->dlist_mm, space, VC4_KERNEL_DWORDS);
>   	if (ret) {
> -		DRM_ERROR("Failed to allocate space for filter kernel: %d\n",
> -			  ret);
> +		drm_err(&hvs->vc4->base, "Failed to allocate space for filter kernel: %d\n",
> +			ret);
>   		return ret;
>   	}
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> index 563b3dfeb9b9..ef93d8e22a35 100644
> --- a/drivers/gpu/drm/vc4/vc4_irq.c
> +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> @@ -76,7 +76,7 @@ vc4_overflow_mem_work(struct work_struct *work)
> 
>   	bin_bo_slot = vc4_v3d_get_bin_slot(vc4);
>   	if (bin_bo_slot < 0) {
> -		DRM_ERROR("Couldn't allocate binner overflow mem\n");
> +		drm_err(&vc4->base, "Couldn't allocate binner overflow mem\n");
>   		goto complete;
>   	}
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> index 04ac7805e6d5..6e566584afbf 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -471,8 +471,8 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
>   		return ret;
> 
>   	if (V3D_READ(V3D_IDENT0) != V3D_EXPECTED_IDENT0) {
> -		DRM_ERROR("V3D_IDENT0 read 0x%08x instead of 0x%08x\n",
> -			  V3D_READ(V3D_IDENT0), V3D_EXPECTED_IDENT0);
> +		drm_err(drm, "V3D_IDENT0 read 0x%08x instead of 0x%08x\n",
> +			V3D_READ(V3D_IDENT0), V3D_EXPECTED_IDENT0);
>   		ret = -EINVAL;
>   		goto err_put_runtime_pm;
>   	}
> @@ -485,7 +485,7 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
> 
>   	ret = vc4_irq_install(drm, vc4->irq);
>   	if (ret) {
> -		DRM_ERROR("Failed to install IRQ handler\n");
> +		drm_err(drm, "Failed to install IRQ handler\n");
>   		goto err_put_runtime_pm;
>   	}
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_validate.c b/drivers/gpu/drm/vc4/vc4_validate.c
> index 7dff3ca5af6b..0c17284bf6f5 100644
> --- a/drivers/gpu/drm/vc4/vc4_validate.c
> +++ b/drivers/gpu/drm/vc4/vc4_validate.c
> @@ -65,7 +65,7 @@ utile_width(int cpp)
>   	case 8:
>   		return 2;
>   	default:
> -		DRM_ERROR("unknown cpp: %d\n", cpp);
> +		pr_err("unknown cpp: %d\n", cpp);
>   		return 1;
>   	}
>   }
> @@ -82,7 +82,7 @@ utile_height(int cpp)
>   	case 8:
>   		return 4;
>   	default:
> -		DRM_ERROR("unknown cpp: %d\n", cpp);
> +		pr_err("unknown cpp: %d\n", cpp);
>   		return 1;
>   	}
>   }
> @@ -390,8 +390,8 @@ validate_tile_binning_config(VALIDATE_ARGS)
>   	bin_slot = vc4_v3d_get_bin_slot(vc4);
>   	if (bin_slot < 0) {
>   		if (bin_slot != -EINTR && bin_slot != -ERESTARTSYS) {
> -			DRM_ERROR("Failed to allocate binner memory: %d\n",
> -				  bin_slot);
> +			drm_err(dev, "Failed to allocate binner memory: %d\n",
> +				bin_slot);
>   		}
>   		return bin_slot;
>   	}
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> index 070813b8aff8..eb64e881051e 100644
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> @@ -557,7 +557,7 @@ static void vc4_vec_encoder_disable(struct drm_encoder *encoder,
> 
>   	ret = pm_runtime_put(&vec->pdev->dev);
>   	if (ret < 0) {
> -		DRM_ERROR("Failed to release power domain: %d\n", ret);
> +		drm_err(drm, "Failed to release power domain: %d\n", ret);
>   		goto err_dev_exit;
>   	}
> 
> @@ -591,7 +591,7 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder,
> 
>   	ret = pm_runtime_resume_and_get(&vec->pdev->dev);
>   	if (ret < 0) {
> -		DRM_ERROR("Failed to retain power domain: %d\n", ret);
> +		drm_err(drm, "Failed to retain power domain: %d\n", ret);
>   		goto err_dev_exit;
>   	}
> 
> @@ -604,13 +604,13 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder,
>   	 */
>   	ret = clk_set_rate(vec->clock, 108000000);
>   	if (ret) {
> -		DRM_ERROR("Failed to set clock rate: %d\n", ret);
> +		drm_err(drm, "Failed to set clock rate: %d\n", ret);
>   		goto err_put_runtime_pm;
>   	}
> 
>   	ret = clk_prepare_enable(vec->clock);
>   	if (ret) {
> -		DRM_ERROR("Failed to turn on core clock: %d\n", ret);
> +		drm_err(drm, "Failed to turn on core clock: %d\n", ret);
>   		goto err_put_runtime_pm;
>   	}
> 
> @@ -806,7 +806,7 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data)
>   	if (IS_ERR(vec->clock)) {
>   		ret = PTR_ERR(vec->clock);
>   		if (ret != -EPROBE_DEFER)
> -			DRM_ERROR("Failed to get clock: %d\n", ret);
> +			drm_err(drm, "Failed to get clock: %d\n", ret);
>   		return ret;
>   	}
> 
> --
> 2.34.1
> 

[PATCH V3 4/9] drm/vc4: hdmi: add PM suspend/resume support

[Stefan Wahren]

Add suspend/resume support for the VC4 HDMI component in order
to handle suspend to idle properly. Since the HDMI power domain
is powered down during suspend, this makes connector status polling
pointless.

Link: https://lore.kernel.org/dri-devel/7003512d-7303-4f41-b0d6-a8af5bf8e497@gmx.net/
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Acked-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 6611ab7c26a6..f7a4ed16094e 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -3104,6 +3104,31 @@ static int vc5_hdmi_init_resources(struct drm_device *drm,
 	return 0;
 }

+static int vc4_hdmi_suspend(struct device *dev)
+{
+	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
+	struct drm_device *drm = vc4_hdmi->connector.dev;
+
+	if (drm && drm->mode_config.poll_enabled)
+		drm_kms_helper_poll_disable(drm);
+
+	return pm_runtime_force_suspend(dev);
+}
+
+static int vc4_hdmi_resume(struct device *dev)
+{
+	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
+	struct drm_device *drm = vc4_hdmi->connector.dev;
+	int ret;
+
+	ret = pm_runtime_force_resume(dev);
+
+	if (drm && drm->mode_config.poll_enabled)
+		drm_kms_helper_poll_enable(drm);
+
+	return ret;
+}
+
 static int vc4_hdmi_runtime_suspend(struct device *dev)
 {
 	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
@@ -3405,6 +3430,7 @@ static const struct dev_pm_ops vc4_hdmi_pm_ops = {
 	SET_RUNTIME_PM_OPS(vc4_hdmi_runtime_suspend,
 			   vc4_hdmi_runtime_resume,
 			   NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(vc4_hdmi_suspend, vc4_hdmi_resume)
 };

 struct platform_driver vc4_hdmi_driver = {
--
2.34.1

Re: [PATCH V3 4/9] drm/vc4: hdmi: add PM suspend/resume support

[kernel test robot]

Hi Stefan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on arm/for-next arm/fixes v6.11-rc4 next-20240822]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Wahren/mailbox-bcm2835-Fix-timeout-during-suspend-mode/20240822-063725
base:   linus/master
patch link:    https://lore.kernel.org/r/20240821214052.6800-5-wahrenst%40gmx.net
patch subject: [PATCH V3 4/9] drm/vc4: hdmi: add PM suspend/resume support
config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20240822/202408221940.t4pWjzvz-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240822/202408221940.t4pWjzvz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408221940.t4pWjzvz-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/vc4/vc4_hdmi.c:3118:12: warning: 'vc4_hdmi_resume' defined but not used [-Wunused-function]
    3118 | static int vc4_hdmi_resume(struct device *dev)
         |            ^~~~~~~~~~~~~~~
>> drivers/gpu/drm/vc4/vc4_hdmi.c:3107:12: warning: 'vc4_hdmi_suspend' defined but not used [-Wunused-function]
    3107 | static int vc4_hdmi_suspend(struct device *dev)
         |            ^~~~~~~~~~~~~~~~


vim +/vc4_hdmi_resume +3118 drivers/gpu/drm/vc4/vc4_hdmi.c

  3106	
> 3107	static int vc4_hdmi_suspend(struct device *dev)
  3108	{
  3109		struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
  3110		struct drm_device *drm = vc4_hdmi->connector.dev;
  3111	
  3112		if (drm && drm->mode_config.poll_enabled)
  3113			drm_kms_helper_poll_disable(drm);
  3114	
  3115		return pm_runtime_force_suspend(dev);
  3116	}
  3117	
> 3118	static int vc4_hdmi_resume(struct device *dev)
  3119	{
  3120		struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
  3121		struct drm_device *drm = vc4_hdmi->connector.dev;
  3122		int ret;
  3123	
  3124		ret = pm_runtime_force_resume(dev);
  3125	
  3126		if (drm && drm->mode_config.poll_enabled)
  3127			drm_kms_helper_poll_enable(drm);
  3128	
  3129		return ret;
  3130	}
  3131	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH V3 5/9] drm/vc4: v3d: simplify clock retrieval

[Stefan Wahren]

Common pattern of handling deferred probe can be simplified with
dev_err_probe() and devm_clk_get_optional(). This results in much
less code.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vc4/vc4_v3d.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 6e566584afbf..bf5c4e36c94e 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -441,21 +441,9 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
 	vc4->v3d = v3d;
 	v3d->vc4 = vc4;

-	v3d->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(v3d->clk)) {
-		int ret = PTR_ERR(v3d->clk);
-
-		if (ret == -ENOENT) {
-			/* bcm2835 didn't have a clock reference in the DT. */
-			ret = 0;
-			v3d->clk = NULL;
-		} else {
-			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "Failed to get V3D clock: %d\n",
-					ret);
-			return ret;
-		}
-	}
+	v3d->clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(v3d->clk))
+		return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");

 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0)
--
2.34.1

Re: [PATCH V3 5/9] drm/vc4: v3d: simplify clock retrieval

[Maíra Canal]

Hi Stefan,

On 8/21/24 18:40, Stefan Wahren wrote:
> Common pattern of handling deferred probe can be simplified with
> dev_err_probe() and devm_clk_get_optional(). This results in much
> less code.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> Reviewed-by: Maíra Canal <mcanal@igalia.com>

Applied to drm/misc/kernel/drm-misc-next!

Best Regards,
- Maíra

> ---
>   drivers/gpu/drm/vc4/vc4_v3d.c | 18 +++---------------
>   1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> index 6e566584afbf..bf5c4e36c94e 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -441,21 +441,9 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
>   	vc4->v3d = v3d;
>   	v3d->vc4 = vc4;
> 
> -	v3d->clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(v3d->clk)) {
> -		int ret = PTR_ERR(v3d->clk);
> -
> -		if (ret == -ENOENT) {
> -			/* bcm2835 didn't have a clock reference in the DT. */
> -			ret = 0;
> -			v3d->clk = NULL;
> -		} else {
> -			if (ret != -EPROBE_DEFER)
> -				dev_err(dev, "Failed to get V3D clock: %d\n",
> -					ret);
> -			return ret;
> -		}
> -	}
> +	v3d->clk = devm_clk_get_optional(dev, NULL);
> +	if (IS_ERR(v3d->clk))
> +		return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
> 
>   	ret = platform_get_irq(pdev, 0);
>   	if (ret < 0)
> --
> 2.34.1
> 

[PATCH V3 6/9] drm/vc4: v3d: add PM suspend/resume support

[Stefan Wahren]

Add suspend/resume support for the VC4 V3D component in order
to handle suspend to idle properly.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/gpu/drm/vc4/vc4_v3d.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index bf5c4e36c94e..03c790f7ffc6 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -508,6 +508,8 @@ static void vc4_v3d_unbind(struct device *dev, struct device *master,

 static const struct dev_pm_ops vc4_v3d_pm_ops = {
 	SET_RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, vc4_v3d_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
 };

 static const struct component_ops vc4_v3d_ops = {
--
2.34.1

[PATCH V3 7/9] usb: dwc2: Refactor backup/restore of registers

[Stefan Wahren]

The DWC2 runtime PM code reuses similar patterns to backup and
restore the registers. So consolidate them in USB mode specific
variants. This also has the advantage it is usable for further
PM improvements.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/usb/dwc2/core.h   |  12 +++++
 drivers/usb/dwc2/gadget.c | 101 +++++++++++++++++++-------------------
 drivers/usb/dwc2/hcd.c    |  99 +++++++++++++++++++------------------
 3 files changed, 114 insertions(+), 98 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 2bd74f3033ed..81e8632f29ed 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1435,6 +1435,8 @@ int dwc2_hsotg_tx_fifo_total_depth(struct dwc2_hsotg *hsotg);
 int dwc2_hsotg_tx_fifo_average_depth(struct dwc2_hsotg *hsotg);
 void dwc2_gadget_init_lpm(struct dwc2_hsotg *hsotg);
 void dwc2_gadget_program_ref_clk(struct dwc2_hsotg *hsotg);
+int dwc2_gadget_backup_critical_registers(struct dwc2_hsotg *hsotg);
+int dwc2_gadget_restore_critical_registers(struct dwc2_hsotg *hsotg);
 static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg)
 { hsotg->fifo_map = 0; }
 #else
@@ -1482,6 +1484,10 @@ static inline int dwc2_hsotg_tx_fifo_average_depth(struct dwc2_hsotg *hsotg)
 { return 0; }
 static inline void dwc2_gadget_init_lpm(struct dwc2_hsotg *hsotg) {}
 static inline void dwc2_gadget_program_ref_clk(struct dwc2_hsotg *hsotg) {}
+static inline int dwc2_gadget_backup_critical_registers(struct dwc2_hsotg *hsotg)
+{ return 0; }
+static inline int dwc2_gadget_restore_critical_registers(struct dwc2_hsotg *hsotg)
+{ return 0; }
 static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg) {}
 #endif

@@ -1505,6 +1511,8 @@ int dwc2_host_exit_partial_power_down(struct dwc2_hsotg *hsotg,
 void dwc2_host_enter_clock_gating(struct dwc2_hsotg *hsotg);
 void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup);
 bool dwc2_host_can_poweroff_phy(struct dwc2_hsotg *dwc2);
+int dwc2_host_backup_critical_registers(struct dwc2_hsotg *hsotg);
+int dwc2_host_restore_critical_registers(struct dwc2_hsotg *hsotg);
 static inline void dwc2_host_schedule_phy_reset(struct dwc2_hsotg *hsotg)
 { schedule_work(&hsotg->phy_reset_work); }
 #else
@@ -1544,6 +1552,10 @@ static inline void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg,
 					       int rem_wakeup) {}
 static inline bool dwc2_host_can_poweroff_phy(struct dwc2_hsotg *dwc2)
 { return false; }
+static inline int dwc2_host_backup_critical_registers(struct dwc2_hsotg *hsotg)
+{ return 0; }
+static inline int dwc2_host_restore_critical_registers(struct dwc2_hsotg *hsotg)
+{ return 0; }
 static inline void dwc2_host_schedule_phy_reset(struct dwc2_hsotg *hsotg) {}

 #endif
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index e7bf9cc635be..0bff748bcf74 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -5309,6 +5309,49 @@ void dwc2_gadget_program_ref_clk(struct dwc2_hsotg *hsotg)
 	dev_dbg(hsotg->dev, "GREFCLK=0x%08x\n", dwc2_readl(hsotg, GREFCLK));
 }

+int dwc2_gadget_backup_critical_registers(struct dwc2_hsotg *hsotg)
+{
+	int ret;
+
+	/* Backup all registers */
+	ret = dwc2_backup_global_registers(hsotg);
+	if (ret) {
+		dev_err(hsotg->dev, "%s: failed to backup global registers\n",
+			__func__);
+		return ret;
+	}
+
+	ret = dwc2_backup_device_registers(hsotg);
+	if (ret) {
+		dev_err(hsotg->dev, "%s: failed to backup device registers\n",
+			__func__);
+		return ret;
+	}
+
+	return 0;
+}
+
+int dwc2_gadget_restore_critical_registers(struct dwc2_hsotg *hsotg)
+{
+	int ret;
+
+	ret = dwc2_restore_global_registers(hsotg);
+	if (ret) {
+		dev_err(hsotg->dev, "%s: failed to restore registers\n",
+			__func__);
+		return ret;
+	}
+
+	ret = dwc2_restore_device_registers(hsotg, 0);
+	if (ret) {
+		dev_err(hsotg->dev, "%s: failed to restore device registers\n",
+			__func__);
+		return ret;
+	}
+
+	return 0;
+}
+
 /**
  * dwc2_gadget_enter_hibernation() - Put controller in Hibernation.
  *
@@ -5326,18 +5369,9 @@ int dwc2_gadget_enter_hibernation(struct dwc2_hsotg *hsotg)
 	/* Change to L2(suspend) state */
 	hsotg->lx_state = DWC2_L2;
 	dev_dbg(hsotg->dev, "Start of hibernation completed\n");
-	ret = dwc2_backup_global_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to backup global registers\n",
-			__func__);
-		return ret;
-	}
-	ret = dwc2_backup_device_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to backup device registers\n",
-			__func__);
+	ret = dwc2_gadget_backup_critical_registers(hsotg);
+	if (ret)
 		return ret;
-	}

 	gpwrdn = GPWRDN_PWRDNRSTN;
 	udelay(10);
@@ -5483,20 +5517,9 @@ int dwc2_gadget_exit_hibernation(struct dwc2_hsotg *hsotg,
 	dwc2_writel(hsotg, 0xffffffff, GINTSTS);

 	/* Restore global registers */
-	ret = dwc2_restore_global_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to restore registers\n",
-			__func__);
-		return ret;
-	}
-
-	/* Restore device registers */
-	ret = dwc2_restore_device_registers(hsotg, rem_wakeup);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to restore device registers\n",
-			__func__);
+	ret = dwc2_gadget_restore_critical_registers(hsotg);
+	if (ret)
 		return ret;
-	}

 	if (rem_wakeup) {
 		mdelay(10);
@@ -5530,19 +5553,9 @@ int dwc2_gadget_enter_partial_power_down(struct dwc2_hsotg *hsotg)
 	dev_dbg(hsotg->dev, "Entering device partial power down started.\n");

 	/* Backup all registers */
-	ret = dwc2_backup_global_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to backup global registers\n",
-			__func__);
-		return ret;
-	}
-
-	ret = dwc2_backup_device_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to backup device registers\n",
-			__func__);
+	ret = dwc2_gadget_backup_critical_registers(hsotg);
+	if (ret)
 		return ret;
-	}

 	/*
 	 * Clear any pending interrupts since dwc2 will not be able to
@@ -5610,21 +5623,9 @@ int dwc2_gadget_exit_partial_power_down(struct dwc2_hsotg *hsotg,

 	udelay(100);
 	if (restore) {
-		ret = dwc2_restore_global_registers(hsotg);
-		if (ret) {
-			dev_err(hsotg->dev, "%s: failed to restore registers\n",
-				__func__);
-			return ret;
-		}
-		/* Restore DCFG */
-		dwc2_writel(hsotg, dr->dcfg, DCFG);
-
-		ret = dwc2_restore_device_registers(hsotg, 0);
-		if (ret) {
-			dev_err(hsotg->dev, "%s: failed to restore device registers\n",
-				__func__);
+		ret = dwc2_gadget_restore_critical_registers(hsotg);
+		if (ret)
 			return ret;
-		}
 	}

 	/* Set the Power-On Programming done bit */
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index cb54390e7de4..32fa606e5d59 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -5477,6 +5477,49 @@ int dwc2_restore_host_registers(struct dwc2_hsotg *hsotg)
 	return 0;
 }

+int dwc2_host_backup_critical_registers(struct dwc2_hsotg *hsotg)
+{
+	int ret;
+
+	/* Backup all registers */
+	ret = dwc2_backup_global_registers(hsotg);
+	if (ret) {
+		dev_err(hsotg->dev, "%s: failed to backup global registers\n",
+			__func__);
+		return ret;
+	}
+
+	ret = dwc2_backup_host_registers(hsotg);
+	if (ret) {
+		dev_err(hsotg->dev, "%s: failed to backup host registers\n",
+			__func__);
+		return ret;
+	}
+
+	return 0;
+}
+
+int dwc2_host_restore_critical_registers(struct dwc2_hsotg *hsotg)
+{
+	int ret;
+
+	ret = dwc2_restore_global_registers(hsotg);
+	if (ret) {
+		dev_err(hsotg->dev, "%s: failed to restore registers\n",
+			__func__);
+		return ret;
+	}
+
+	ret = dwc2_restore_host_registers(hsotg);
+	if (ret) {
+		dev_err(hsotg->dev, "%s: failed to restore host registers\n",
+			__func__);
+		return ret;
+	}
+
+	return 0;
+}
+
 /**
  * dwc2_host_enter_hibernation() - Put controller in Hibernation.
  *
@@ -5492,18 +5535,9 @@ int dwc2_host_enter_hibernation(struct dwc2_hsotg *hsotg)
 	u32 gpwrdn;

 	dev_dbg(hsotg->dev, "Preparing host for hibernation\n");
-	ret = dwc2_backup_global_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to backup global registers\n",
-			__func__);
-		return ret;
-	}
-	ret = dwc2_backup_host_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to backup host registers\n",
-			__func__);
+	ret = dwc2_host_backup_critical_registers(hsotg);
+	if (ret)
 		return ret;
-	}

 	/* Enter USB Suspend Mode */
 	hprt0 = dwc2_readl(hsotg, HPRT0);
@@ -5697,20 +5731,9 @@ int dwc2_host_exit_hibernation(struct dwc2_hsotg *hsotg, int rem_wakeup,
 	dwc2_writel(hsotg, 0xffffffff, GINTSTS);

 	/* Restore global registers */
-	ret = dwc2_restore_global_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to restore registers\n",
-			__func__);
+	ret = dwc2_host_restore_critical_registers(hsotg);
+	if (ret)
 		return ret;
-	}
-
-	/* Restore host registers */
-	ret = dwc2_restore_host_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to restore host registers\n",
-			__func__);
-		return ret;
-	}

 	if (rem_wakeup) {
 		dwc2_hcd_rem_wakeup(hsotg);
@@ -5777,19 +5800,9 @@ int dwc2_host_enter_partial_power_down(struct dwc2_hsotg *hsotg)
 		dev_warn(hsotg->dev, "Suspend wasn't generated\n");

 	/* Backup all registers */
-	ret = dwc2_backup_global_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to backup global registers\n",
-			__func__);
-		return ret;
-	}
-
-	ret = dwc2_backup_host_registers(hsotg);
-	if (ret) {
-		dev_err(hsotg->dev, "%s: failed to backup host registers\n",
-			__func__);
+	ret = dwc2_host_backup_critical_registers(hsotg);
+	if (ret)
 		return ret;
-	}

 	/*
 	 * Clear any pending interrupts since dwc2 will not be able to
@@ -5858,19 +5871,9 @@ int dwc2_host_exit_partial_power_down(struct dwc2_hsotg *hsotg,

 	udelay(100);
 	if (restore) {
-		ret = dwc2_restore_global_registers(hsotg);
-		if (ret) {
-			dev_err(hsotg->dev, "%s: failed to restore registers\n",
-				__func__);
-			return ret;
-		}
-
-		ret = dwc2_restore_host_registers(hsotg);
-		if (ret) {
-			dev_err(hsotg->dev, "%s: failed to restore host registers\n",
-				__func__);
+		ret = dwc2_host_restore_critical_registers(hsotg);
+		if (ret)
 			return ret;
-		}
 	}

 	/* Drive resume signaling and exit suspend mode on the port. */
--
2.34.1

Re: [PATCH V3 7/9] usb: dwc2: Refactor backup/restore of registers

[kernel test robot]

Hi Stefan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on arm/for-next arm/fixes v6.11-rc4 next-20240822]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Wahren/mailbox-bcm2835-Fix-timeout-during-suspend-mode/20240822-063725
base:   linus/master
patch link:    https://lore.kernel.org/r/20240821214052.6800-8-wahrenst%40gmx.net
patch subject: [PATCH V3 7/9] usb: dwc2: Refactor backup/restore of registers
config: x86_64-randconfig-161-20240822 (https://download.01.org/0day-ci/archive/20240822/202408221629.jv9AgCrF-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240822/202408221629.jv9AgCrF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408221629.jv9AgCrF-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/usb/dwc2/gadget.c:5605:28: warning: variable 'dr' set but not used [-Wunused-but-set-variable]
    5605 |         struct dwc2_dregs_backup *dr;
         |                                   ^
   1 warning generated.


vim +/dr +5605 drivers/usb/dwc2/gadget.c

be2b960e57154a Artur Petrosyan 2021-04-08  5588  
be2b960e57154a Artur Petrosyan 2021-04-08  5589  /*
be2b960e57154a Artur Petrosyan 2021-04-08  5590   * dwc2_gadget_exit_partial_power_down() - Exit controller from device partial
be2b960e57154a Artur Petrosyan 2021-04-08  5591   * power down.
be2b960e57154a Artur Petrosyan 2021-04-08  5592   *
be2b960e57154a Artur Petrosyan 2021-04-08  5593   * @hsotg: Programming view of the DWC_otg controller
be2b960e57154a Artur Petrosyan 2021-04-08  5594   * @restore: indicates whether need to restore the registers or not.
be2b960e57154a Artur Petrosyan 2021-04-08  5595   *
be2b960e57154a Artur Petrosyan 2021-04-08  5596   * Return: non-zero if failed to exit device partial power down.
be2b960e57154a Artur Petrosyan 2021-04-08  5597   *
be2b960e57154a Artur Petrosyan 2021-04-08  5598   * This function is for exiting from device mode partial power down.
be2b960e57154a Artur Petrosyan 2021-04-08  5599   */
be2b960e57154a Artur Petrosyan 2021-04-08  5600  int dwc2_gadget_exit_partial_power_down(struct dwc2_hsotg *hsotg,
be2b960e57154a Artur Petrosyan 2021-04-08  5601  					bool restore)
be2b960e57154a Artur Petrosyan 2021-04-08  5602  {
be2b960e57154a Artur Petrosyan 2021-04-08  5603  	u32 pcgcctl;
be2b960e57154a Artur Petrosyan 2021-04-08  5604  	u32 dctl;
be2b960e57154a Artur Petrosyan 2021-04-08 @5605  	struct dwc2_dregs_backup *dr;
be2b960e57154a Artur Petrosyan 2021-04-08  5606  	int ret = 0;
be2b960e57154a Artur Petrosyan 2021-04-08  5607  
be2b960e57154a Artur Petrosyan 2021-04-08  5608  	dr = &hsotg->dr_backup;
be2b960e57154a Artur Petrosyan 2021-04-08  5609  
be2b960e57154a Artur Petrosyan 2021-04-08  5610  	dev_dbg(hsotg->dev, "Exiting device partial Power Down started.\n");
be2b960e57154a Artur Petrosyan 2021-04-08  5611  
be2b960e57154a Artur Petrosyan 2021-04-08  5612  	pcgcctl = dwc2_readl(hsotg, PCGCTL);
be2b960e57154a Artur Petrosyan 2021-04-08  5613  	pcgcctl &= ~PCGCTL_STOPPCLK;
be2b960e57154a Artur Petrosyan 2021-04-08  5614  	dwc2_writel(hsotg, pcgcctl, PCGCTL);
be2b960e57154a Artur Petrosyan 2021-04-08  5615  
be2b960e57154a Artur Petrosyan 2021-04-08  5616  	pcgcctl = dwc2_readl(hsotg, PCGCTL);
be2b960e57154a Artur Petrosyan 2021-04-08  5617  	pcgcctl &= ~PCGCTL_PWRCLMP;
be2b960e57154a Artur Petrosyan 2021-04-08  5618  	dwc2_writel(hsotg, pcgcctl, PCGCTL);
be2b960e57154a Artur Petrosyan 2021-04-08  5619  
be2b960e57154a Artur Petrosyan 2021-04-08  5620  	pcgcctl = dwc2_readl(hsotg, PCGCTL);
be2b960e57154a Artur Petrosyan 2021-04-08  5621  	pcgcctl &= ~PCGCTL_RSTPDWNMODULE;
be2b960e57154a Artur Petrosyan 2021-04-08  5622  	dwc2_writel(hsotg, pcgcctl, PCGCTL);
be2b960e57154a Artur Petrosyan 2021-04-08  5623  
be2b960e57154a Artur Petrosyan 2021-04-08  5624  	udelay(100);
be2b960e57154a Artur Petrosyan 2021-04-08  5625  	if (restore) {
78c09c66698a77 Stefan Wahren   2024-08-21  5626  		ret = dwc2_gadget_restore_critical_registers(hsotg);
78c09c66698a77 Stefan Wahren   2024-08-21  5627  		if (ret)
be2b960e57154a Artur Petrosyan 2021-04-08  5628  			return ret;
be2b960e57154a Artur Petrosyan 2021-04-08  5629  	}
be2b960e57154a Artur Petrosyan 2021-04-08  5630  
be2b960e57154a Artur Petrosyan 2021-04-08  5631  	/* Set the Power-On Programming done bit */
be2b960e57154a Artur Petrosyan 2021-04-08  5632  	dctl = dwc2_readl(hsotg, DCTL);
be2b960e57154a Artur Petrosyan 2021-04-08  5633  	dctl |= DCTL_PWRONPRGDONE;
be2b960e57154a Artur Petrosyan 2021-04-08  5634  	dwc2_writel(hsotg, dctl, DCTL);
be2b960e57154a Artur Petrosyan 2021-04-08  5635  
be2b960e57154a Artur Petrosyan 2021-04-08  5636  	/* Set in_ppd flag to 0 as here core exits from suspend. */
be2b960e57154a Artur Petrosyan 2021-04-08  5637  	hsotg->in_ppd = 0;
be2b960e57154a Artur Petrosyan 2021-04-08  5638  	hsotg->lx_state = DWC2_L0;
be2b960e57154a Artur Petrosyan 2021-04-08  5639  
be2b960e57154a Artur Petrosyan 2021-04-08  5640  	dev_dbg(hsotg->dev, "Exiting device partial Power Down completed.\n");
be2b960e57154a Artur Petrosyan 2021-04-08  5641  	return ret;
be2b960e57154a Artur Petrosyan 2021-04-08  5642  }
012466fc8ccc01 Artur Petrosyan 2021-04-13  5643  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH V3 7/9] usb: dwc2: Refactor backup/restore of registers

[Stefan Wahren]

Am 21.08.24 um 23:40 schrieb Stefan Wahren:
> The DWC2 runtime PM code reuses similar patterns to backup and
> restore the registers. So consolidate them in USB mode specific
> variants. This also has the advantage it is usable for further
> PM improvements.
>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>   drivers/usb/dwc2/core.h   |  12 +++++
>   drivers/usb/dwc2/gadget.c | 101 +++++++++++++++++++-------------------
>   drivers/usb/dwc2/hcd.c    |  99 +++++++++++++++++++------------------
>   3 files changed, 114 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 2bd74f3033ed..81e8632f29ed 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -1435,6 +1435,8 @@ int dwc2_hsotg_tx_fifo_total_depth(struct dwc2_hsotg *hsotg);
>   int dwc2_hsotg_tx_fifo_average_depth(struct dwc2_hsotg *hsotg);
>   void dwc2_gadget_init_lpm(struct dwc2_hsotg *hsotg);
>   void dwc2_gadget_program_ref_clk(struct dwc2_hsotg *hsotg);
> +int dwc2_gadget_backup_critical_registers(struct dwc2_hsotg *hsotg);
> +int dwc2_gadget_restore_critical_registers(struct dwc2_hsotg *hsotg);
>   static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg)
>   { hsotg->fifo_map = 0; }
>   #else
> @@ -1482,6 +1484,10 @@ static inline int dwc2_hsotg_tx_fifo_average_depth(struct dwc2_hsotg *hsotg)
>   { return 0; }
>   static inline void dwc2_gadget_init_lpm(struct dwc2_hsotg *hsotg) {}
>   static inline void dwc2_gadget_program_ref_clk(struct dwc2_hsotg *hsotg) {}
> +static inline int dwc2_gadget_backup_critical_registers(struct dwc2_hsotg *hsotg)
> +{ return 0; }
> +static inline int dwc2_gadget_restore_critical_registers(struct dwc2_hsotg *hsotg)
> +{ return 0; }
>   static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg) {}
>   #endif
>
> @@ -1505,6 +1511,8 @@ int dwc2_host_exit_partial_power_down(struct dwc2_hsotg *hsotg,
>   void dwc2_host_enter_clock_gating(struct dwc2_hsotg *hsotg);
>   void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup);
>   bool dwc2_host_can_poweroff_phy(struct dwc2_hsotg *dwc2);
> +int dwc2_host_backup_critical_registers(struct dwc2_hsotg *hsotg);
> +int dwc2_host_restore_critical_registers(struct dwc2_hsotg *hsotg);
>   static inline void dwc2_host_schedule_phy_reset(struct dwc2_hsotg *hsotg)
>   { schedule_work(&hsotg->phy_reset_work); }
>   #else
> @@ -1544,6 +1552,10 @@ static inline void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg,
>   					       int rem_wakeup) {}
>   static inline bool dwc2_host_can_poweroff_phy(struct dwc2_hsotg *dwc2)
>   { return false; }
> +static inline int dwc2_host_backup_critical_registers(struct dwc2_hsotg *hsotg)
> +{ return 0; }
> +static inline int dwc2_host_restore_critical_registers(struct dwc2_hsotg *hsotg)
> +{ return 0; }
>   static inline void dwc2_host_schedule_phy_reset(struct dwc2_hsotg *hsotg) {}
>
>   #endif
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index e7bf9cc635be..0bff748bcf74 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -5309,6 +5309,49 @@ void dwc2_gadget_program_ref_clk(struct dwc2_hsotg *hsotg)
>   	dev_dbg(hsotg->dev, "GREFCLK=0x%08x\n", dwc2_readl(hsotg, GREFCLK));
>   }
>
> +int dwc2_gadget_backup_critical_registers(struct dwc2_hsotg *hsotg)
> +{
> +	int ret;
> +
> +	/* Backup all registers */
> +	ret = dwc2_backup_global_registers(hsotg);
> +	if (ret) {
> +		dev_err(hsotg->dev, "%s: failed to backup global registers\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = dwc2_backup_device_registers(hsotg);
> +	if (ret) {
> +		dev_err(hsotg->dev, "%s: failed to backup device registers\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int dwc2_gadget_restore_critical_registers(struct dwc2_hsotg *hsotg)
> +{
> +	int ret;
> +
> +	ret = dwc2_restore_global_registers(hsotg);
> +	if (ret) {
> +		dev_err(hsotg->dev, "%s: failed to restore registers\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = dwc2_restore_device_registers(hsotg, 0);
> +	if (ret) {
> +		dev_err(hsotg->dev, "%s: failed to restore device registers\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * dwc2_gadget_enter_hibernation() - Put controller in Hibernation.
>    *
> @@ -5326,18 +5369,9 @@ int dwc2_gadget_enter_hibernation(struct dwc2_hsotg *hsotg)
>   	/* Change to L2(suspend) state */
>   	hsotg->lx_state = DWC2_L2;
>   	dev_dbg(hsotg->dev, "Start of hibernation completed\n");
> -	ret = dwc2_backup_global_registers(hsotg);
> -	if (ret) {
> -		dev_err(hsotg->dev, "%s: failed to backup global registers\n",
> -			__func__);
> -		return ret;
> -	}
> -	ret = dwc2_backup_device_registers(hsotg);
> -	if (ret) {
> -		dev_err(hsotg->dev, "%s: failed to backup device registers\n",
> -			__func__);
> +	ret = dwc2_gadget_backup_critical_registers(hsotg);
> +	if (ret)
>   		return ret;
> -	}
>
>   	gpwrdn = GPWRDN_PWRDNRSTN;
>   	udelay(10);
> @@ -5483,20 +5517,9 @@ int dwc2_gadget_exit_hibernation(struct dwc2_hsotg *hsotg,
>   	dwc2_writel(hsotg, 0xffffffff, GINTSTS);
>
>   	/* Restore global registers */
> -	ret = dwc2_restore_global_registers(hsotg);
> -	if (ret) {
> -		dev_err(hsotg->dev, "%s: failed to restore registers\n",
> -			__func__);
> -		return ret;
> -	}
> -
> -	/* Restore device registers */
> -	ret = dwc2_restore_device_registers(hsotg, rem_wakeup);
> -	if (ret) {
> -		dev_err(hsotg->dev, "%s: failed to restore device registers\n",
> -			__func__);
> +	ret = dwc2_gadget_restore_critical_registers(hsotg);
> +	if (ret)
>   		return ret;
> -	}
>
>   	if (rem_wakeup) {
>   		mdelay(10);
> @@ -5530,19 +5553,9 @@ int dwc2_gadget_enter_partial_power_down(struct dwc2_hsotg *hsotg)
>   	dev_dbg(hsotg->dev, "Entering device partial power down started.\n");
>
>   	/* Backup all registers */
> -	ret = dwc2_backup_global_registers(hsotg);
> -	if (ret) {
> -		dev_err(hsotg->dev, "%s: failed to backup global registers\n",
> -			__func__);
> -		return ret;
> -	}
> -
> -	ret = dwc2_backup_device_registers(hsotg);
> -	if (ret) {
> -		dev_err(hsotg->dev, "%s: failed to backup device registers\n",
> -			__func__);
> +	ret = dwc2_gadget_backup_critical_registers(hsotg);
> +	if (ret)
>   		return ret;
> -	}
>
>   	/*
>   	 * Clear any pending interrupts since dwc2 will not be able to
> @@ -5610,21 +5623,9 @@ int dwc2_gadget_exit_partial_power_down(struct dwc2_hsotg *hsotg,
>
>   	udelay(100);
>   	if (restore) {
> -		ret = dwc2_restore_global_registers(hsotg);
> -		if (ret) {
> -			dev_err(hsotg->dev, "%s: failed to restore registers\n",
> -				__func__);
> -			return ret;
> -		}
> -		/* Restore DCFG */
> -		dwc2_writel(hsotg, dr->dcfg, DCFG);
Oh dear, i accidentally dropped that. I will fix this.

Thanks goes to kernel test robot

[PATCH V3 RFC 8/9] usb: dwc2: Implement recovery after PM domain off

[Stefan Wahren]

According to the dt-bindings there are some platforms, which have a
dedicated USB power domain for DWC2 IP core supply. If the power domain
is switched off during system suspend then all USB register will lose
their settings.

Use GUSBCFG_TOUTCAL as a canary to detect that the power domain has
been powered off during suspend. Since the GOTGCTL_CURMODE_HOST doesn't
match on all platform with the current mode, additionally backup
GINTSTS. This works reliable to decide which registers should be
restored.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/usb/dwc2/core.c     |  1 +
 drivers/usb/dwc2/core.h     |  2 ++
 drivers/usb/dwc2/platform.c | 38 +++++++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

Hi, would be nice to get some feedback about the approach and
test results on other DWC2 platforms.

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 9919ab725d54..c3d24312db0f 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -43,6 +43,7 @@ int dwc2_backup_global_registers(struct dwc2_hsotg *hsotg)
 	/* Backup global regs */
 	gr = &hsotg->gr_backup;

+	gr->gintsts = dwc2_readl(hsotg, GINTSTS);
 	gr->gotgctl = dwc2_readl(hsotg, GOTGCTL);
 	gr->gintmsk = dwc2_readl(hsotg, GINTMSK);
 	gr->gahbcfg = dwc2_readl(hsotg, GAHBCFG);
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 81e8632f29ed..e65a74853bb0 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -667,6 +667,7 @@ struct dwc2_hw_params {
 /**
  * struct dwc2_gregs_backup - Holds global registers state before
  * entering partial power down
+ * @gintsts:		Backup of GINTSTS register
  * @gotgctl:		Backup of GOTGCTL register
  * @gintmsk:		Backup of GINTMSK register
  * @gahbcfg:		Backup of GAHBCFG register
@@ -683,6 +684,7 @@ struct dwc2_hw_params {
  * @valid:		True if registers values backuped.
  */
 struct dwc2_gregs_backup {
+	u32 gintsts;
 	u32 gotgctl;
 	u32 gintmsk;
 	u32 gahbcfg;
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 7b84416dfc2b..39e9064b6cfe 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -683,6 +683,14 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 		regulator_disable(dwc2->usb33d);
 	}

+	if (is_device_mode)
+		ret = dwc2_gadget_backup_critical_registers(dwc2);
+	else
+		ret = dwc2_host_backup_critical_registers(dwc2);
+
+	if (ret)
+		return ret;
+
 	if (dwc2->ll_hw_enabled &&
 	    (is_device_mode || dwc2_host_can_poweroff_phy(dwc2))) {
 		ret = __dwc2_lowlevel_hw_disable(dwc2);
@@ -692,6 +700,24 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 	return ret;
 }

+static int dwc2_restore_critical_registers(struct dwc2_hsotg *hsotg)
+{
+	struct dwc2_gregs_backup *gr;
+
+	gr = &hsotg->gr_backup;
+
+	if (!gr->valid) {
+		dev_err(hsotg->dev, "%s: no registers to restore\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	if (gr->gintsts & GINTSTS_CURMODE_HOST)
+		return dwc2_host_restore_critical_registers(hsotg);
+
+	return dwc2_gadget_restore_critical_registers(hsotg);
+}
+
 static int __maybe_unused dwc2_resume(struct device *dev)
 {
 	struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev);
@@ -704,6 +730,18 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 	}
 	dwc2->phy_off_for_suspend = false;

+	/*
+	 * During suspend it's possible that the power domain for the
+	 * DWC2 controller is disabled and all register values get lost.
+	 * In case the GUSBCFG register is not initialized, it's clear the
+	 * registers must be restored.
+	 */
+	if (!(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK)) {
+		ret = dwc2_restore_critical_registers(dwc2);
+		if (ret)
+			return ret;
+	}
+
 	if (dwc2->params.activate_stm_id_vb_detection) {
 		unsigned long flags;
 		u32 ggpio, gotgctl;
--
2.34.1

Re: [PATCH V3 RFC 8/9] usb: dwc2: Implement recovery after PM domain off

[Doug Anderson]

Hi,

On Wed, Aug 21, 2024 at 2:41 PM Stefan Wahren <wahrenst@gmx.net> wrote:
>
> According to the dt-bindings there are some platforms, which have a
> dedicated USB power domain for DWC2 IP core supply. If the power domain
> is switched off during system suspend then all USB register will lose
> their settings.
>
> Use GUSBCFG_TOUTCAL as a canary to detect that the power domain has
> been powered off during suspend. Since the GOTGCTL_CURMODE_HOST doesn't
> match on all platform with the current mode, additionally backup
> GINTSTS. This works reliable to decide which registers should be
> restored.
>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  drivers/usb/dwc2/core.c     |  1 +
>  drivers/usb/dwc2/core.h     |  2 ++
>  drivers/usb/dwc2/platform.c | 38 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+)
> +static int dwc2_restore_critical_registers(struct dwc2_hsotg *hsotg)
> +{
> +       struct dwc2_gregs_backup *gr;
> +
> +       gr = &hsotg->gr_backup;
> +
> +       if (!gr->valid) {
> +               dev_err(hsotg->dev, "%s: no registers to restore\n",
> +                       __func__);

nit: IMO "__func__" should not be in dev_err() messages. The message
plus the device should be enough. If __func__ should have been in
dev_err() messages then the Linux kernel would have automatically put
it there.

Aside from that, this looks reasonable to me and discussed previously.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH V3 9/9] ARM: bcm2835_defconfig: Enable SUSPEND

[Stefan Wahren]

Since the Raspberry Pi supports Suspend-To-Idle now, this option
should be enabled. This should make power management testing easier.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 arch/arm/configs/bcm2835_defconfig | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/configs/bcm2835_defconfig b/arch/arm/configs/bcm2835_defconfig
index b5f0bd8dd536..97632dee1ab3 100644
--- a/arch/arm/configs/bcm2835_defconfig
+++ b/arch/arm/configs/bcm2835_defconfig
@@ -38,8 +38,6 @@ CONFIG_CPU_FREQ_GOV_ONDEMAND=y
 CONFIG_CPUFREQ_DT=y
 CONFIG_ARM_RASPBERRYPI_CPUFREQ=y
 CONFIG_VFP=y
-# CONFIG_SUSPEND is not set
-CONFIG_PM=y
 CONFIG_JUMP_LABEL=y
 CONFIG_MODULES=y
 CONFIG_MODULE_UNLOAD=y
--
2.34.1