[PATCH V2 00/16] ARM: bcm2835: Implement initial S2Idle for Raspberry Pi

[PATCH V2 00/16] 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-5, 15, 16) which allows S2Idle support for BCM2835
 2. drm vc4 patches which implement S2Idle support (mostly new and not
    really BCM2835 specific)
 3. dwc2 patches which handle BCM2835 specific issues (still in progress,
    but works fine)

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 of the implementation isn't complete yet (especially patch 14),
but much better than the first version. 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 [2].

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 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

Stefan Wahren (16):
  firmware: raspberrypi: Improve timeout warning
  mailbox: bcm2835: Fix timeout during suspend mode
  pmdomain: raspberrypi-power: Adjust packet definition
  pmdomain: raspberrypi-power: Add logging to rpi_firmware_set_power
  pmdomain: raspberrypi-power: set flag GENPD_FLAG_ACTIVE_WAKEUP
  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: debugfs: Print parameter no_clock_gating
  usb: dwc2: Add comment about BCM2848 ACPI ID
  usb: dwc2: Skip clock gating on Broadcom SoCs
  WIP: usb: dwc2: Implement recovery after PM domain off
  serial: 8250_bcm2835aux: add PM suspend/resume support
  ARM: bcm2835_defconfig: Enable SUSPEND

 arch/arm/configs/bcm2835_defconfig        |  2 -
 drivers/firmware/raspberrypi.c            |  3 +-
 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             | 21 +++----
 drivers/gpu/drm/vc4/vc4_validate.c        |  8 +--
 drivers/gpu/drm/vc4/vc4_vec.c             | 10 ++--
 drivers/mailbox/bcm2835-mailbox.c         |  3 +-
 drivers/pmdomain/bcm/raspberrypi-power.c  | 43 ++++++++------
 drivers/tty/serial/8250/8250_bcm2835aux.c | 37 ++++++++++++
 drivers/usb/dwc2/core.c                   | 16 ++++++
 drivers/usb/dwc2/core.h                   | 17 ++++++
 drivers/usb/dwc2/debugfs.c                |  1 +
 drivers/usb/dwc2/gadget.c                 | 49 ++++++++++++++++
 drivers/usb/dwc2/hcd.c                    | 49 ++++++++++++++++
 drivers/usb/dwc2/params.c                 |  2 +
 drivers/usb/dwc2/platform.c               | 32 +++++++++++
 22 files changed, 339 insertions(+), 101 deletions(-)

--
2.34.1

[PATCH V2 01/16] firmware: raspberrypi: Improve timeout warning

[Stefan Wahren]

Recent work on raspberry-power driver showed that even the
stacktrace on firmware property timeout doesn't provide
enough information. So add the first tag name to the warning
to be in line with a status error.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/firmware/raspberrypi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
index ac34876a97f8..18cc34987108 100644
--- a/drivers/firmware/raspberrypi.c
+++ b/drivers/firmware/raspberrypi.c
@@ -62,7 +62,6 @@ rpi_firmware_transaction(struct rpi_firmware *fw, u32 chan, u32 data)
 			ret = 0;
 		} else {
 			ret = -ETIMEDOUT;
-			WARN_ONCE(1, "Firmware transaction timeout");
 		}
 	} else {
 		dev_err(fw->cl.dev, "mbox_send_message returned %d\n", ret);
@@ -125,6 +124,8 @@ int rpi_firmware_property_list(struct rpi_firmware *fw,
 		dev_err(fw->cl.dev, "Request 0x%08x returned status 0x%08x\n",
 			buf[2], buf[1]);
 		ret = -EINVAL;
+	} else if (ret == -ETIMEDOUT) {
+		WARN_ONCE(1, "Firmware transaction 0x%08x timeout", buf[2]);
 	}

 	dma_free_coherent(fw->chan->mbox->dev, PAGE_ALIGN(size), buf, bus_addr);
--
2.34.1

Re: [PATCH V2 01/16] firmware: raspberrypi: Improve timeout warning

[Stefan Wahren]

Hi Florian,

Am 28.07.24 um 13:41 schrieb Stefan Wahren:
> Recent work on raspberry-power driver showed that even the
> stacktrace on firmware property timeout doesn't provide
> enough information. So add the first tag name to the warning
> to be in line with a status error.
>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Are there any concerns, because i assumed this patch would go via your tree?

Best regards
> ---
>   drivers/firmware/raspberrypi.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index ac34876a97f8..18cc34987108 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
> @@ -62,7 +62,6 @@ rpi_firmware_transaction(struct rpi_firmware *fw, u32 chan, u32 data)
>   			ret = 0;
>   		} else {
>   			ret = -ETIMEDOUT;
> -			WARN_ONCE(1, "Firmware transaction timeout");
>   		}
>   	} else {
>   		dev_err(fw->cl.dev, "mbox_send_message returned %d\n", ret);
> @@ -125,6 +124,8 @@ int rpi_firmware_property_list(struct rpi_firmware *fw,
>   		dev_err(fw->cl.dev, "Request 0x%08x returned status 0x%08x\n",
>   			buf[2], buf[1]);
>   		ret = -EINVAL;
> +	} else if (ret == -ETIMEDOUT) {
> +		WARN_ONCE(1, "Firmware transaction 0x%08x timeout", buf[2]);
>   	}
>
>   	dma_free_coherent(fw->chan->mbox->dev, PAGE_ALIGN(size), buf, bus_addr);
> --
> 2.34.1
>

Re: [PATCH V2 01/16] firmware: raspberrypi: Improve timeout warning

[Florian Fainelli]

From: Florian Fainelli <f.fainelli@gmail.com>

On Sun, 28 Jul 2024 13:41:45 +0200, Stefan Wahren <wahrenst@gmx.net> wrote:
> Recent work on raspberry-power driver showed that even the
> stacktrace on firmware property timeout doesn't provide
> enough information. So add the first tag name to the warning
> to be in line with a status error.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---

Applied to https://github.com/Broadcom/stblinux/commits/drivers/next, thanks!
--
Florian

[PATCH V2 02/16] 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 V2 02/16] mailbox: bcm2835: Fix timeout during suspend mode

[Stefan Wahren]

Hi Jassi,

Am 28.07.24 um 13:41 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>
Are there any objections with this patch?

I consider this solution as settled.

Best regards
> ---
>   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
>

[PATCH V2 03/16] pmdomain: raspberrypi-power: Adjust packet definition

[Stefan Wahren]

According to the official Mailbox property interface the second part
of RPI_FIRMWARE_SET_POWER_STATE ( and so on ...) is named state because
it represent u32 flags and just the lowest bit is for on/off. So rename it
to align with documentation and prepare the driver for further changes.

Link: https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/pmdomain/bcm/raspberrypi-power.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pmdomain/bcm/raspberrypi-power.c b/drivers/pmdomain/bcm/raspberrypi-power.c
index 06196ebfe03b..39d9a52200c3 100644
--- a/drivers/pmdomain/bcm/raspberrypi-power.c
+++ b/drivers/pmdomain/bcm/raspberrypi-power.c
@@ -41,7 +41,7 @@ struct rpi_power_domains {
  */
 struct rpi_power_domain_packet {
 	u32 domain;
-	u32 on;
+	u32 state;
 };

 /*
@@ -53,7 +53,7 @@ static int rpi_firmware_set_power(struct rpi_power_domain *rpi_domain, bool on)
 	struct rpi_power_domain_packet packet;

 	packet.domain = rpi_domain->domain;
-	packet.on = on;
+	packet.state = on;
 	return rpi_firmware_property(rpi_domain->fw,
 				     rpi_domain->old_interface ?
 				     RPI_FIRMWARE_SET_POWER_STATE :
@@ -142,13 +142,13 @@ rpi_has_new_domain_support(struct rpi_power_domains *rpi_domains)
 	int ret;

 	packet.domain = RPI_POWER_DOMAIN_ARM;
-	packet.on = ~0;
+	packet.state = ~0;

 	ret = rpi_firmware_property(rpi_domains->fw,
 				    RPI_FIRMWARE_GET_DOMAIN_STATE,
 				    &packet, sizeof(packet));

-	return ret == 0 && packet.on != ~0;
+	return ret == 0 && packet.state != ~0;
 }

 static int rpi_power_probe(struct platform_device *pdev)
--
2.34.1

[PATCH V2 04/16] pmdomain: raspberrypi-power: Add logging to rpi_firmware_set_power

[Stefan Wahren]

The Raspberry Pi power driver heavily relies on the logging of the
underlying firmware driver. This results in disadvantages like unspecific
error messages or limited debugging options. So implement the logging for
the most important function rpi_firmware_set_power.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/pmdomain/bcm/raspberrypi-power.c | 34 ++++++++++++++----------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/pmdomain/bcm/raspberrypi-power.c b/drivers/pmdomain/bcm/raspberrypi-power.c
index 39d9a52200c3..fadedfc9c645 100644
--- a/drivers/pmdomain/bcm/raspberrypi-power.c
+++ b/drivers/pmdomain/bcm/raspberrypi-power.c
@@ -48,33 +48,39 @@ struct rpi_power_domain_packet {
  * Asks the firmware to enable or disable power on a specific power
  * domain.
  */
-static int rpi_firmware_set_power(struct rpi_power_domain *rpi_domain, bool on)
+static int rpi_firmware_set_power(struct generic_pm_domain *domain, bool on)
 {
+	struct rpi_power_domain *rpi_domain =
+		container_of(domain, struct rpi_power_domain, base);
+	bool old_interface = rpi_domain->old_interface;
 	struct rpi_power_domain_packet packet;
+	int ret;

 	packet.domain = rpi_domain->domain;
 	packet.state = on;
-	return rpi_firmware_property(rpi_domain->fw,
-				     rpi_domain->old_interface ?
-				     RPI_FIRMWARE_SET_POWER_STATE :
-				     RPI_FIRMWARE_SET_DOMAIN_STATE,
-				     &packet, sizeof(packet));
+
+	ret = rpi_firmware_property(rpi_domain->fw, old_interface ?
+				    RPI_FIRMWARE_SET_POWER_STATE :
+				    RPI_FIRMWARE_SET_DOMAIN_STATE,
+				    &packet, sizeof(packet));
+	if (ret)
+		dev_err(&domain->dev, "Failed to set %s to %u (%d)\n",
+			old_interface ? "power" : "domain", on, ret);
+	else
+		dev_dbg(&domain->dev, "Set %s to %u\n",
+			old_interface ? "power" : "domain", on);
+
+	return ret;
 }

 static int rpi_domain_off(struct generic_pm_domain *domain)
 {
-	struct rpi_power_domain *rpi_domain =
-		container_of(domain, struct rpi_power_domain, base);
-
-	return rpi_firmware_set_power(rpi_domain, false);
+	return rpi_firmware_set_power(domain, false);
 }

 static int rpi_domain_on(struct generic_pm_domain *domain)
 {
-	struct rpi_power_domain *rpi_domain =
-		container_of(domain, struct rpi_power_domain, base);
-
-	return rpi_firmware_set_power(rpi_domain, true);
+	return rpi_firmware_set_power(domain, true);
 }

 static void rpi_common_init_power_domain(struct rpi_power_domains *rpi_domains,
--
2.34.1

[PATCH V2 05/16] pmdomain: raspberrypi-power: set flag GENPD_FLAG_ACTIVE_WAKEUP

[Stefan Wahren]

Set flag GENPD_FLAG_ACTIVE_WAKEUP to rpi_power genpd, then when a device
is set as wakeup source using device_set_wakeup_enable, the power
domain could be kept on to make sure the device could wakeup the system.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/pmdomain/bcm/raspberrypi-power.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pmdomain/bcm/raspberrypi-power.c b/drivers/pmdomain/bcm/raspberrypi-power.c
index fadedfc9c645..b87ea7adb7be 100644
--- a/drivers/pmdomain/bcm/raspberrypi-power.c
+++ b/drivers/pmdomain/bcm/raspberrypi-power.c
@@ -91,6 +91,7 @@ static void rpi_common_init_power_domain(struct rpi_power_domains *rpi_domains,
 	dom->fw = rpi_domains->fw;

 	dom->base.name = name;
+	dom->base.flags = GENPD_FLAG_ACTIVE_WAKEUP;
 	dom->base.power_on = rpi_domain_on;
 	dom->base.power_off = rpi_domain_off;

--
2.34.1

Re: [PATCH V2 05/16] pmdomain: raspberrypi-power: set flag GENPD_FLAG_ACTIVE_WAKEUP

[Stefan Wahren]

Hi,

Am 28.07.24 um 13:41 schrieb Stefan Wahren:
> Set flag GENPD_FLAG_ACTIVE_WAKEUP to rpi_power genpd, then when a device
> is set as wakeup source using device_set_wakeup_enable, the power
> domain could be kept on to make sure the device could wakeup the system.
>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
I know a lot developer are in holidays, but it would be nice to get a
review for the new pmdomain parts before i send V3.

Best regards
> ---
>   drivers/pmdomain/bcm/raspberrypi-power.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pmdomain/bcm/raspberrypi-power.c b/drivers/pmdomain/bcm/raspberrypi-power.c
> index fadedfc9c645..b87ea7adb7be 100644
> --- a/drivers/pmdomain/bcm/raspberrypi-power.c
> +++ b/drivers/pmdomain/bcm/raspberrypi-power.c
> @@ -91,6 +91,7 @@ static void rpi_common_init_power_domain(struct rpi_power_domains *rpi_domains,
>   	dom->fw = rpi_domains->fw;
>
>   	dom->base.name = name;
> +	dom->base.flags = GENPD_FLAG_ACTIVE_WAKEUP;
>   	dom->base.power_on = rpi_domain_on;
>   	dom->base.power_off = rpi_domain_off;
>
> --
> 2.34.1
>

Re: [PATCH V2 05/16] pmdomain: raspberrypi-power: set flag GENPD_FLAG_ACTIVE_WAKEUP

[Ulf Hansson]

On Sun, 28 Jul 2024 at 13:47, Stefan Wahren <wahrenst@gmx.net> wrote:
>
> Set flag GENPD_FLAG_ACTIVE_WAKEUP to rpi_power genpd, then when a device
> is set as wakeup source using device_set_wakeup_enable, the power
> domain could be kept on to make sure the device could wakeup the system.
>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

Patch 3 -> 5, applied for next to my pmdomain tree, thanks!

Kind regards
Uffe


> ---
>  drivers/pmdomain/bcm/raspberrypi-power.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pmdomain/bcm/raspberrypi-power.c b/drivers/pmdomain/bcm/raspberrypi-power.c
> index fadedfc9c645..b87ea7adb7be 100644
> --- a/drivers/pmdomain/bcm/raspberrypi-power.c
> +++ b/drivers/pmdomain/bcm/raspberrypi-power.c
> @@ -91,6 +91,7 @@ static void rpi_common_init_power_domain(struct rpi_power_domains *rpi_domains,
>         dom->fw = rpi_domains->fw;
>
>         dom->base.name = name;
> +       dom->base.flags = GENPD_FLAG_ACTIVE_WAKEUP;
>         dom->base.power_on = rpi_domain_on;
>         dom->base.power_off = rpi_domain_off;
>
> --
> 2.34.1
>
>

[PATCH V2 06/16] 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>
---
 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 V2 06/16] drm/vc4: hdmi: Handle error case of pm_runtime_resume_and_get

[Maíra Canal]

On 7/28/24 08:41, 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>

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
> 

Re: [PATCH V2 06/16] drm/vc4: hdmi: Handle error case of pm_runtime_resume_and_get

[Maxime Ripard]

On Sun, 28 Jul 2024 13:41:50 +0200, 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.
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH V2 07/16] 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>
---
 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..952953b4cdf8 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..688bfddbfb8f 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..b5f61da199c9 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..f12d572287f0 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..3f13ff692c28 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..1ede508a67d3 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..5474a36c5c9d 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 V2 07/16] drm/vc4: Get the rid of DRM_ERROR()

[Maxime Ripard]

On Sun, 28 Jul 2024 13:41:51 +0200, 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.
> 
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH V2 07/16] drm/vc4: Get the rid of DRM_ERROR()

[Maíra Canal]

On 7/28/24 08:41, 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>

I'd just double-check this patch with checkpatch.sh, as I feel a couple
of lines are not aligned to the parenthesis. That check, you can add my:

Reviewed-by: Maíra Canal <mcanal@igalia.com>

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..952953b4cdf8 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..688bfddbfb8f 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..b5f61da199c9 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..f12d572287f0 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..3f13ff692c28 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..1ede508a67d3 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..5474a36c5c9d 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 V2 08/16] 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>
---
 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 V2 08/16] drm/vc4: hdmi: add PM suspend/resume support

[Maxime Ripard]

On Sun, 28 Jul 2024 13:41:52 +0200, Stefan Wahren wrote:
> 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.
> 
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH V2 09/16] 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>
---
 drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 1ede508a67d3..4bf3a8d24770 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -441,20 +441,11 @@ 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);
+	v3d->clk = devm_clk_get_optional(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;
-		}
+		return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
 	}

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

[PATCH V2 10/16] 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 4bf3a8d24770..309c8af08fd0 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -511,6 +511,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 V2 11/16] usb: dwc2: debugfs: Print parameter no_clock_gating

[Stefan Wahren]

The commit c4a0f7a6ab54 ("usb: dwc2: Skip clock gating on Samsung
SoCs") introduced a parameter to skip enabling clock gating mode
even the hardware platform should supports it.

In order to make this more visible also print this in show
parameters of debugfs.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Acked-by: Minas Harutyunyan <hminas@synopsys.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/usb/dwc2/debugfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc2/debugfs.c b/drivers/usb/dwc2/debugfs.c
index 7c82ab590401..3116ac72747f 100644
--- a/drivers/usb/dwc2/debugfs.c
+++ b/drivers/usb/dwc2/debugfs.c
@@ -702,6 +702,7 @@ static int params_show(struct seq_file *seq, void *v)
 	print_param(seq, p, uframe_sched);
 	print_param(seq, p, external_id_pin_ctl);
 	print_param(seq, p, power_down);
+	print_param(seq, p, no_clock_gating);
 	print_param(seq, p, lpm);
 	print_param(seq, p, lpm_clock_gating);
 	print_param(seq, p, besl);
--
2.34.1

[PATCH V2 12/16] usb: dwc2: Add comment about BCM2848 ACPI ID

[Stefan Wahren]

During recent code review the different naming between ACPI and OF
IDs led to confusion. So add a clarifying comment.

Link: https://lore.kernel.org/linux-usb/38e46b44-6248-45e8-bdf9-66008a2fe290@arm.com/
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/usb/dwc2/params.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index a937eadbc9b3..4d73fae80b12 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -352,6 +352,7 @@ const struct of_device_id dwc2_of_match_table[] = {
 MODULE_DEVICE_TABLE(of, dwc2_of_match_table);

 const struct acpi_device_id dwc2_acpi_match[] = {
+	/* This ID refers to the same USB IP as of_device_id brcm,bcm2835-usb */
 	{ "BCM2848", (kernel_ulong_t)dwc2_set_bcm_params },
 	{ },
 };
--
2.34.1

[PATCH V2 13/16] usb: dwc2: Skip clock gating on Broadcom SoCs

[Stefan Wahren]

On resume of the Raspberry Pi the dwc2 driver fails to enable
HCD_FLAG_HW_ACCESSIBLE before re-enabling the interrupts.
This causes a situation where both handler ignore a incoming port
interrupt and force the upper layers to disable the dwc2 interrupt line.
This leaves the USB interface in a unusable state:

irq 66: nobody cared (try booting with the "irqpoll" option)
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W          6.10.0-rc3
Hardware name: BCM2835
Call trace:
unwind_backtrace from show_stack+0x10/0x14
show_stack from dump_stack_lvl+0x50/0x64
dump_stack_lvl from __report_bad_irq+0x38/0xc0
__report_bad_irq from note_interrupt+0x2ac/0x2f4
note_interrupt from handle_irq_event+0x88/0x8c
handle_irq_event from handle_level_irq+0xb4/0x1ac
handle_level_irq from generic_handle_domain_irq+0x24/0x34
generic_handle_domain_irq from bcm2836_chained_handle_irq+0x24/0x28
bcm2836_chained_handle_irq from generic_handle_domain_irq+0x24/0x34
generic_handle_domain_irq from generic_handle_arch_irq+0x34/0x44
generic_handle_arch_irq from __irq_svc+0x88/0xb0
Exception stack(0xc1b01f20 to 0xc1b01f68)
1f20: 0005c0d4 00000001 00000000 00000000 c1b09780 c1d6b32c c1b04e54 c1a5eae8
1f40: c1b04e90 00000000 00000000 00000000 c1d6a8a0 c1b01f70 c11d2da8 c11d4160
1f60: 60000013 ffffffff
__irq_svc from default_idle_call+0x1c/0xb0
default_idle_call from do_idle+0x21c/0x284
do_idle from cpu_startup_entry+0x28/0x2c
cpu_startup_entry from kernel_init+0x0/0x12c
handlers:
[<f539e0f4>] dwc2_handle_common_intr
[<75cd278b>] usb_hcd_irq
Disabling IRQ #66

Disabling clock gating workaround this issue.

Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
Link: https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/T/
Link: https://lore.kernel.org/all/5e8cbce0-3260-2971-484f-fc73a3b2bd28@synopsys.com/
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Acked-by: Minas Harutyunyan <hminas@synopsys.com>
---
 drivers/usb/dwc2/params.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 4d73fae80b12..68226defdc60 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -23,6 +23,7 @@ static void dwc2_set_bcm_params(struct dwc2_hsotg *hsotg)
 	p->max_transfer_size = 65535;
 	p->max_packet_count = 511;
 	p->ahbcfg = 0x10;
+	p->no_clock_gating = true;
 }

 static void dwc2_set_his_params(struct dwc2_hsotg *hsotg)
--
2.34.1

[PATCH V2 14/16] WIP: usb: dwc2: Implement recovery after PM domain off

[Stefan Wahren]

DO NOT MERGE

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.

So use the power on/off notifier in order to save & restore the USB
registers during system suspend.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---

Any feedback is appreciated.

 drivers/usb/dwc2/core.c     | 16 ++++++++++++
 drivers/usb/dwc2/core.h     | 17 +++++++++++++
 drivers/usb/dwc2/gadget.c   | 49 +++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/hcd.c      | 49 +++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/platform.c | 32 ++++++++++++++++++++++++
 5 files changed, 163 insertions(+)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 9919ab725d54..a3263cfdedac 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -391,6 +391,22 @@ int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, int rem_wakeup,
 		return dwc2_gadget_exit_hibernation(hsotg, rem_wakeup, reset);
 }

+int dwc2_enter_poweroff(struct dwc2_hsotg *hsotg)
+{
+	if (dwc2_is_host_mode(hsotg))
+		return dwc2_host_enter_poweroff(hsotg);
+	else
+		return dwc2_gadget_enter_poweroff(hsotg);
+}
+
+int dwc2_exit_poweroff(struct dwc2_hsotg *hsotg)
+{
+	if (dwc2_is_host_mode(hsotg))
+		return dwc2_host_exit_poweroff(hsotg);
+	else
+		return dwc2_gadget_exit_poweroff(hsotg);
+}
+
 /*
  * Do core a soft reset of the core.  Be careful with this because it
  * resets all the internal state machines of the core.
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 2bd74f3033ed..9ab755cc3081 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -9,6 +9,7 @@
 #define __DWC2_CORE_H__

 #include <linux/acpi.h>
+#include <linux/notifier.h>
 #include <linux/phy/phy.h>
 #include <linux/regulator/consumer.h>
 #include <linux/usb/gadget.h>
@@ -1080,6 +1081,8 @@ struct dwc2_hsotg {
 	struct regulator *vbus_supply;
 	struct regulator *usb33d;

+	struct notifier_block genpd_nb;
+
 	spinlock_t lock;
 	void *priv;
 	int     irq;
@@ -1316,6 +1319,8 @@ int dwc2_exit_partial_power_down(struct dwc2_hsotg *hsotg, int rem_wakeup,
 int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg, int is_host);
 int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, int rem_wakeup,
 		int reset, int is_host);
+int dwc2_enter_poweroff(struct dwc2_hsotg *hsotg);
+int dwc2_exit_poweroff(struct dwc2_hsotg *hsotg);
 void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg *hsotg);
 int dwc2_phy_init(struct dwc2_hsotg *hsotg, bool select_phy);

@@ -1435,6 +1440,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_enter_poweroff(struct dwc2_hsotg *hsotg);
+int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg);
 static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg)
 { hsotg->fifo_map = 0; }
 #else
@@ -1482,6 +1489,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_enter_poweroff(struct dwc2_hsotg *hsotg)
+{ return 0; }
+static inline int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg)
+{ return 0; }
 static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg) {}
 #endif

@@ -1505,6 +1516,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_enter_poweroff(struct dwc2_hsotg *hsotg);
+int dwc2_host_exit_poweroff(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 +1557,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_enter_poweroff(struct dwc2_hsotg *hsotg)
+{ return 0; }
+static inline int dwc2_host_exit_poweroff(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..38f0112970fe 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -5710,3 +5710,52 @@ void dwc2_gadget_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup)
 	hsotg->lx_state = DWC2_L0;
 	hsotg->bus_suspended = false;
 }
+
+int dwc2_gadget_enter_poweroff(struct dwc2_hsotg *hsotg)
+{
+	int ret;
+
+	dev_dbg(hsotg->dev, "Entering device power off.\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__);
+		return ret;
+	}
+
+	dev_dbg(hsotg->dev, "Entering device power off completed.\n");
+	return 0;
+}
+
+int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg)
+{
+	int ret;
+
+	dev_dbg(hsotg->dev, "Exiting device power off.\n");
+
+	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;
+	}
+
+	dev_dbg(hsotg->dev, "Exiting device power off completed.\n");
+	return 0;
+}
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index cb54390e7de4..22afdafb474e 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -5993,3 +5993,52 @@ void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup)
 			  jiffies + msecs_to_jiffies(71));
 	}
 }
+
+int dwc2_host_enter_poweroff(struct dwc2_hsotg *hsotg)
+{
+	int ret;
+
+	dev_dbg(hsotg->dev, "Entering host power off.\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__);
+		return ret;
+	}
+
+	dev_dbg(hsotg->dev, "Entering host power off completed.\n");
+	return 0;
+}
+
+int dwc2_host_exit_poweroff(struct dwc2_hsotg *hsotg)
+{
+	int ret;
+
+	dev_dbg(hsotg->dev, "Exiting host power off.\n");
+
+	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;
+	}
+
+	dev_dbg(hsotg->dev, "Exiting host power off completed.\n");
+	return 0;
+}
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 7b84416dfc2b..b97eefc18a6b 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_data/s3c-hsotg.h>
+#include <linux/pm_domain.h>
 #include <linux/reset.h>

 #include <linux/usb/of.h>
@@ -307,6 +308,8 @@ static void dwc2_driver_remove(struct platform_device *dev)
 	struct dwc2_gregs_backup *gr;
 	int ret = 0;

+	dev_pm_genpd_remove_notifier(&dev->dev);
+
 	gr = &hsotg->gr_backup;

 	/* Exit Hibernation when driver is removed. */
@@ -421,6 +424,31 @@ int dwc2_check_core_version(struct dwc2_hsotg *hsotg)
 	return 0;
 }

+static int dwc2_power_notifier(struct notifier_block *nb,
+			       unsigned long action, void *data)
+{
+	struct dwc2_hsotg *hsotg = container_of(nb, struct dwc2_hsotg,
+						genpd_nb);
+	int ret;
+
+	switch (action) {
+	case GENPD_NOTIFY_ON:
+		ret = dwc2_exit_poweroff(hsotg);
+		if (ret)
+			dev_err(hsotg->dev, "exit poweroff failed\n");
+		break;
+	case GENPD_NOTIFY_PRE_OFF:
+		ret = dwc2_enter_poweroff(hsotg);
+		if (ret)
+			dev_err(hsotg->dev, "enter poweroff failed\n");
+		break;
+	default:
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
 /**
  * dwc2_driver_probe() - Called when the DWC_otg core is bound to the DWC_otg
  * driver
@@ -620,6 +648,10 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		}
 	}
 #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */
+
+	hsotg->genpd_nb.notifier_call = dwc2_power_notifier;
+	dev_pm_genpd_add_notifier(&dev->dev, &hsotg->genpd_nb);
+
 	return 0;

 #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
--
2.34.1

Re: [PATCH V2 14/16] WIP: usb: dwc2: Implement recovery after PM domain off

[Stefan Wahren]

Hi Doug,

Am 28.07.24 um 15:00 schrieb Stefan Wahren:
> DO NOT MERGE
>
> 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.
>
> So use the power on/off notifier in order to save & restore the USB
> registers during system suspend.
sorry for bothering you with this DWC2 stuff, but it would great if you
can gave some feedback about this patch. I was working a lot to get
suspend to idle working on Raspberry Pi. And this patch is the most
complex part of the series.

Would you agree with this approach or did i miss something?

The problem is that the power domain driver acts independent from dwc2,
so we cannot prevent the USB domain power down except declaring a USB
device as wakeup source. So i decided to use the notifier approach. This
has been successful tested on some older Raspberry Pi boards.

Best regards
>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>
> Any feedback is appreciated.
>
>   drivers/usb/dwc2/core.c     | 16 ++++++++++++
>   drivers/usb/dwc2/core.h     | 17 +++++++++++++
>   drivers/usb/dwc2/gadget.c   | 49 +++++++++++++++++++++++++++++++++++++
>   drivers/usb/dwc2/hcd.c      | 49 +++++++++++++++++++++++++++++++++++++
>   drivers/usb/dwc2/platform.c | 32 ++++++++++++++++++++++++
>   5 files changed, 163 insertions(+)
>
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 9919ab725d54..a3263cfdedac 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -391,6 +391,22 @@ int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, int rem_wakeup,
>   		return dwc2_gadget_exit_hibernation(hsotg, rem_wakeup, reset);
>   }
>
> +int dwc2_enter_poweroff(struct dwc2_hsotg *hsotg)
> +{
> +	if (dwc2_is_host_mode(hsotg))
> +		return dwc2_host_enter_poweroff(hsotg);
> +	else
> +		return dwc2_gadget_enter_poweroff(hsotg);
> +}
> +
> +int dwc2_exit_poweroff(struct dwc2_hsotg *hsotg)
> +{
> +	if (dwc2_is_host_mode(hsotg))
> +		return dwc2_host_exit_poweroff(hsotg);
> +	else
> +		return dwc2_gadget_exit_poweroff(hsotg);
> +}
> +
>   /*
>    * Do core a soft reset of the core.  Be careful with this because it
>    * resets all the internal state machines of the core.
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 2bd74f3033ed..9ab755cc3081 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -9,6 +9,7 @@
>   #define __DWC2_CORE_H__
>
>   #include <linux/acpi.h>
> +#include <linux/notifier.h>
>   #include <linux/phy/phy.h>
>   #include <linux/regulator/consumer.h>
>   #include <linux/usb/gadget.h>
> @@ -1080,6 +1081,8 @@ struct dwc2_hsotg {
>   	struct regulator *vbus_supply;
>   	struct regulator *usb33d;
>
> +	struct notifier_block genpd_nb;
> +
>   	spinlock_t lock;
>   	void *priv;
>   	int     irq;
> @@ -1316,6 +1319,8 @@ int dwc2_exit_partial_power_down(struct dwc2_hsotg *hsotg, int rem_wakeup,
>   int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg, int is_host);
>   int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, int rem_wakeup,
>   		int reset, int is_host);
> +int dwc2_enter_poweroff(struct dwc2_hsotg *hsotg);
> +int dwc2_exit_poweroff(struct dwc2_hsotg *hsotg);
>   void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg *hsotg);
>   int dwc2_phy_init(struct dwc2_hsotg *hsotg, bool select_phy);
>
> @@ -1435,6 +1440,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_enter_poweroff(struct dwc2_hsotg *hsotg);
> +int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg);
>   static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg)
>   { hsotg->fifo_map = 0; }
>   #else
> @@ -1482,6 +1489,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_enter_poweroff(struct dwc2_hsotg *hsotg)
> +{ return 0; }
> +static inline int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg)
> +{ return 0; }
>   static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg) {}
>   #endif
>
> @@ -1505,6 +1516,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_enter_poweroff(struct dwc2_hsotg *hsotg);
> +int dwc2_host_exit_poweroff(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 +1557,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_enter_poweroff(struct dwc2_hsotg *hsotg)
> +{ return 0; }
> +static inline int dwc2_host_exit_poweroff(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..38f0112970fe 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -5710,3 +5710,52 @@ void dwc2_gadget_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup)
>   	hsotg->lx_state = DWC2_L0;
>   	hsotg->bus_suspended = false;
>   }
> +
> +int dwc2_gadget_enter_poweroff(struct dwc2_hsotg *hsotg)
> +{
> +	int ret;
> +
> +	dev_dbg(hsotg->dev, "Entering device power off.\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__);
> +		return ret;
> +	}
> +
> +	dev_dbg(hsotg->dev, "Entering device power off completed.\n");
> +	return 0;
> +}
> +
> +int dwc2_gadget_exit_poweroff(struct dwc2_hsotg *hsotg)
> +{
> +	int ret;
> +
> +	dev_dbg(hsotg->dev, "Exiting device power off.\n");
> +
> +	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;
> +	}
> +
> +	dev_dbg(hsotg->dev, "Exiting device power off completed.\n");
> +	return 0;
> +}
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index cb54390e7de4..22afdafb474e 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -5993,3 +5993,52 @@ void dwc2_host_exit_clock_gating(struct dwc2_hsotg *hsotg, int rem_wakeup)
>   			  jiffies + msecs_to_jiffies(71));
>   	}
>   }
> +
> +int dwc2_host_enter_poweroff(struct dwc2_hsotg *hsotg)
> +{
> +	int ret;
> +
> +	dev_dbg(hsotg->dev, "Entering host power off.\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__);
> +		return ret;
> +	}
> +
> +	dev_dbg(hsotg->dev, "Entering host power off completed.\n");
> +	return 0;
> +}
> +
> +int dwc2_host_exit_poweroff(struct dwc2_hsotg *hsotg)
> +{
> +	int ret;
> +
> +	dev_dbg(hsotg->dev, "Exiting host power off.\n");
> +
> +	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;
> +	}
> +
> +	dev_dbg(hsotg->dev, "Exiting host power off completed.\n");
> +	return 0;
> +}
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 7b84416dfc2b..b97eefc18a6b 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -16,6 +16,7 @@
>   #include <linux/platform_device.h>
>   #include <linux/phy/phy.h>
>   #include <linux/platform_data/s3c-hsotg.h>
> +#include <linux/pm_domain.h>
>   #include <linux/reset.h>
>
>   #include <linux/usb/of.h>
> @@ -307,6 +308,8 @@ static void dwc2_driver_remove(struct platform_device *dev)
>   	struct dwc2_gregs_backup *gr;
>   	int ret = 0;
>
> +	dev_pm_genpd_remove_notifier(&dev->dev);
> +
>   	gr = &hsotg->gr_backup;
>
>   	/* Exit Hibernation when driver is removed. */
> @@ -421,6 +424,31 @@ int dwc2_check_core_version(struct dwc2_hsotg *hsotg)
>   	return 0;
>   }
>
> +static int dwc2_power_notifier(struct notifier_block *nb,
> +			       unsigned long action, void *data)
> +{
> +	struct dwc2_hsotg *hsotg = container_of(nb, struct dwc2_hsotg,
> +						genpd_nb);
> +	int ret;
> +
> +	switch (action) {
> +	case GENPD_NOTIFY_ON:
> +		ret = dwc2_exit_poweroff(hsotg);
> +		if (ret)
> +			dev_err(hsotg->dev, "exit poweroff failed\n");
> +		break;
> +	case GENPD_NOTIFY_PRE_OFF:
> +		ret = dwc2_enter_poweroff(hsotg);
> +		if (ret)
> +			dev_err(hsotg->dev, "enter poweroff failed\n");
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
>   /**
>    * dwc2_driver_probe() - Called when the DWC_otg core is bound to the DWC_otg
>    * driver
> @@ -620,6 +648,10 @@ static int dwc2_driver_probe(struct platform_device *dev)
>   		}
>   	}
>   #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */
> +
> +	hsotg->genpd_nb.notifier_call = dwc2_power_notifier;
> +	dev_pm_genpd_add_notifier(&dev->dev, &hsotg->genpd_nb);
> +
>   	return 0;
>
>   #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
> --
> 2.34.1
>

Re: [PATCH V2 14/16] WIP: usb: dwc2: Implement recovery after PM domain off

[Doug Anderson]

Hi,

On Mon, Aug 12, 2024 at 4:48 PM Stefan Wahren <wahrenst@gmx.net> wrote:
>
> Hi Doug,
>
> Am 28.07.24 um 15:00 schrieb Stefan Wahren:
> > DO NOT MERGE
> >
> > 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.
> >
> > So use the power on/off notifier in order to save & restore the USB
> > registers during system suspend.
> sorry for bothering you with this DWC2 stuff, but it would great if you
> can gave some feedback about this patch.

Boy, it's been _ages_ since I looked at anything to do with dwc2, but
I still have some fondness in my heart for the crufty old driver :-P I
know I was involved with some of the patches to get
wakeup-from-suspend working on dwc2 host controllers in the past but,
if I remember correctly, I mostly shepherded / fixed patches from
Rockchip. Not sure I can spend the days trawling through the driver
and testing things with printk that really answering properly would
need, but let's see...


> I was working a lot to get
> suspend to idle working on Raspberry Pi. And this patch is the most
> complex part of the series.
>
> Would you agree with this approach or did i miss something?
>
> The problem is that the power domain driver acts independent from dwc2,
> so we cannot prevent the USB domain power down except declaring a USB
> device as wakeup source. So i decided to use the notifier approach. This
> has been successful tested on some older Raspberry Pi boards.

My genpd knowledge is probably not as good as it should be. Don't tell
anyone (aside from all the people and lists CCed here). ;-)

...so I guess you're relying on the fact that
dev_pm_genpd_add_notifier() will return an error if a power-domain
wasn't specified for dwc2 in the device tree, then you ignore that
error and your callback will never happen. You assume that the power
domain isn't specified then the dwc2 registers will be saved?

I guess one thing is that I'd wonder if that's really reliable. Maybe
some dwc2 controllers lose their registers over system suspend but
_don't_ specify a power domain? Maybe the USB controller just gets its
power yanked as part of system suspend. Maybe that's why the functions
for saving / restoring registers are already there? It looks like
there are ways for various platforms to specify that registers are
lost in some cases...

...but I guess you can't use the existing ways to say that registers
are lost because you're trying to be dynamic. You're saying that your
registers get saved _unless_ the power domain gets turned off, right?
...and the device core keeps power domains on for suspended devices if
they are wakeup sources, which makes sense.

So with that, your patch sounds like a plausible way to do it. I guess
one other way to do it would be some sort of "canary" approach. You
could _always_ save registers and then, at resume time, you could
detect if some "canary" register had reset to its power-on default. If
you see this then you can assume power was lost and re-init all the
registers. This could be pretty much any register that you know won't
be its power on default. In some ways a "canary" approach is uglier
but it also might be more reliable across more configurations?

I guess those would be my main thoughts on the topic. Is that roughly
the feedback you were looking for?

-Doug

Re: [PATCH V2 14/16] WIP: usb: dwc2: Implement recovery after PM domain off

[Ulf Hansson]

On Tue, 13 Aug 2024 at 21:57, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Aug 12, 2024 at 4:48 PM Stefan Wahren <wahrenst@gmx.net> wrote:
> >
> > Hi Doug,
> >
> > Am 28.07.24 um 15:00 schrieb Stefan Wahren:
> > > DO NOT MERGE
> > >
> > > 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.
> > >
> > > So use the power on/off notifier in order to save & restore the USB
> > > registers during system suspend.
> > sorry for bothering you with this DWC2 stuff, but it would great if you
> > can gave some feedback about this patch.
>
> Boy, it's been _ages_ since I looked at anything to do with dwc2, but
> I still have some fondness in my heart for the crufty old driver :-P I
> know I was involved with some of the patches to get
> wakeup-from-suspend working on dwc2 host controllers in the past but,
> if I remember correctly, I mostly shepherded / fixed patches from
> Rockchip. Not sure I can spend the days trawling through the driver
> and testing things with printk that really answering properly would
> need, but let's see...
>
>
> > I was working a lot to get
> > suspend to idle working on Raspberry Pi. And this patch is the most
> > complex part of the series.
> >
> > Would you agree with this approach or did i miss something?
> >
> > The problem is that the power domain driver acts independent from dwc2,
> > so we cannot prevent the USB domain power down except declaring a USB
> > device as wakeup source. So i decided to use the notifier approach. This
> > has been successful tested on some older Raspberry Pi boards.
>
> My genpd knowledge is probably not as good as it should be. Don't tell
> anyone (aside from all the people and lists CCed here). ;-)
>
> ...so I guess you're relying on the fact that
> dev_pm_genpd_add_notifier() will return an error if a power-domain
> wasn't specified for dwc2 in the device tree, then you ignore that
> error and your callback will never happen. You assume that the power
> domain isn't specified then the dwc2 registers will be saved?
>
> I guess one thing is that I'd wonder if that's really reliable. Maybe
> some dwc2 controllers lose their registers over system suspend but
> _don't_ specify a power domain? Maybe the USB controller just gets its
> power yanked as part of system suspend. Maybe that's why the functions
> for saving / restoring registers are already there? It looks like
> there are ways for various platforms to specify that registers are
> lost in some cases...
>
> ...but I guess you can't use the existing ways to say that registers
> are lost because you're trying to be dynamic. You're saying that your
> registers get saved _unless_ the power domain gets turned off, right?
> ...and the device core keeps power domains on for suspended devices if
> they are wakeup sources, which makes sense.
>
> So with that, your patch sounds like a plausible way to do it. I guess
> one other way to do it would be some sort of "canary" approach. You
> could _always_ save registers and then, at resume time, you could
> detect if some "canary" register had reset to its power-on default. If
> you see this then you can assume power was lost and re-init all the
> registers. This could be pretty much any register that you know won't
> be its power on default. In some ways a "canary" approach is uglier
> but it also might be more reliable across more configurations?
>
> I guess those would be my main thoughts on the topic. Is that roughly
> the feedback you were looking for?

Thanks Doug for sharing your thoughts. For the record, I agree with
these suggestions.

Using the genpd on/off notifiers is certainly fine, but doing a
save/restore unconditionally via some of the PM callbacks is usually
preferred - if it works.

Kind regards
Uffe

Re: [PATCH V2 14/16] WIP: usb: dwc2: Implement recovery after PM domain off

[Stefan Wahren]

Am 14.08.24 um 14:01 schrieb Ulf Hansson:
> On Tue, 13 Aug 2024 at 21:57, Doug Anderson <dianders@chromium.org> wrote:
>> Hi,
>>
>> On Mon, Aug 12, 2024 at 4:48 PM Stefan Wahren <wahrenst@gmx.net> wrote:
>>> Hi Doug,
>>>
>>> Am 28.07.24 um 15:00 schrieb Stefan Wahren:
>>>> DO NOT MERGE
>>>>
>>>> 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.
>>>>
>>>> So use the power on/off notifier in order to save & restore the USB
>>>> registers during system suspend.
>>> sorry for bothering you with this DWC2 stuff, but it would great if you
>>> can gave some feedback about this patch.
>> Boy, it's been _ages_ since I looked at anything to do with dwc2, but
>> I still have some fondness in my heart for the crufty old driver :-P I
>> know I was involved with some of the patches to get
>> wakeup-from-suspend working on dwc2 host controllers in the past but,
>> if I remember correctly, I mostly shepherded / fixed patches from
>> Rockchip. Not sure I can spend the days trawling through the driver
>> and testing things with printk that really answering properly would
>> need, but let's see...
Yes, this was more a cry for help, because i didn't get much feedback
yet here [1] [2]. So i searched for the most elegant solution via trial
& error and this patch is the outcome. One reason why this is still WIP,
is that i didn't test the gadget role path yet.
>>
>>> I was working a lot to get
>>> suspend to idle working on Raspberry Pi. And this patch is the most
>>> complex part of the series.
>>>
>>> Would you agree with this approach or did i miss something?
>>>
>>> The problem is that the power domain driver acts independent from dwc2,
>>> so we cannot prevent the USB domain power down except declaring a USB
>>> device as wakeup source. So i decided to use the notifier approach. This
>>> has been successful tested on some older Raspberry Pi boards.
>> My genpd knowledge is probably not as good as it should be. Don't tell
>> anyone (aside from all the people and lists CCed here). ;-)
>>
>> ...so I guess you're relying on the fact that
>> dev_pm_genpd_add_notifier() will return an error if a power-domain
>> wasn't specified for dwc2 in the device tree, then you ignore that
>> error and your callback will never happen. You assume that the power
>> domain isn't specified then the dwc2 registers will be saved?
Yes, on Raspberry Pi we needed to implement the power domain driver to
enable USB at the first place.
>> I guess one thing is that I'd wonder if that's really reliable. Maybe
>> some dwc2 controllers lose their registers over system suspend but
>> _don't_ specify a power domain? Maybe the USB controller just gets its
>> power yanked as part of system suspend. Maybe that's why the functions
>> for saving / restoring registers are already there? It looks like
>> there are ways for various platforms to specify that registers are
>> lost in some cases...
Yes, the DT property "snps,need-phy-for-wake" is such a case. But the
PHY on Raspberry Pi is currently modeled as a no-op.
>> ...but I guess you can't use the existing ways to say that registers
>> are lost because you're trying to be dynamic.
Yes, before this patch the DWC2 doesn't know if the USB domain is
powered down during suspend. So the notifier seems the most elegant
solution to me.
>> You're saying that your
>> registers get saved _unless_ the power domain gets turned off, right?
On BCM2835 there is no need to store the registers because there is no
power management supported by USB core except of the power domain. So
DWC2 don't expect a register loss.
>> ...and the device core keeps power domains on for suspended devices if
>> they are wakeup sources, which makes sense.
>>
>> So with that, your patch sounds like a plausible way to do it. I guess
>> one other way to do it would be some sort of "canary" approach. You
>> could _always_ save registers and then, at resume time, you could
>> detect if some "canary" register had reset to its power-on default. If
>> you see this then you can assume power was lost and re-init all the
>> registers. This could be pretty much any register that you know won't
>> be its power on default. In some ways a "canary" approach is uglier
>> but it also might be more reliable across more configurations?
I don't have enough knowledge about DWC2 and i also don't have the
databook to figure out if there is a magic register which could be used
for the canary approach. But all these different platforms, host vs
gadget role, different low modes let me think the resulting solution
would be also fragile and ugly.
>> I guess those would be my main thoughts on the topic. Is that roughly
>> the feedback you were looking for?
Yes, thank you. This was very helpful.
> Thanks Doug for sharing your thoughts. For the record, I agree with
> these suggestions.
>
> Using the genpd on/off notifiers is certainly fine, but doing a
> save/restore unconditionally via some of the PM callbacks is usually
> preferred - if it works.

I tried the latter one before without success. Because the DWC2 is aware
that the BCM2835 IP doesn't support any power saving mode, the USB core
stays fully functional in suspend and register restoring on resume
tramples newer registers values.

Best regards

[1] -
https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/
[2] -
https://lore.kernel.org/linux-usb/e4532055-c5d6-4ac9-8bbb-b9df18fa178b@gmx.net/
>
> Kind regards
> Uffe

Re: [PATCH V2 14/16] WIP: usb: dwc2: Implement recovery after PM domain off

[Doug Anderson]

Hi,

On Wed, Aug 14, 2024 at 2:48 PM Stefan Wahren <wahrenst@gmx.net> wrote:
>
> >> You're saying that your
> >> registers get saved _unless_ the power domain gets turned off, right?
> On BCM2835 there is no need to store the registers because there is no
> power management supported by USB core except of the power domain. So
> DWC2 don't expect a register loss.
> >> ...and the device core keeps power domains on for suspended devices if
> >> they are wakeup sources, which makes sense.
> >>
> >> So with that, your patch sounds like a plausible way to do it. I guess
> >> one other way to do it would be some sort of "canary" approach. You
> >> could _always_ save registers and then, at resume time, you could
> >> detect if some "canary" register had reset to its power-on default. If
> >> you see this then you can assume power was lost and re-init all the
> >> registers. This could be pretty much any register that you know won't
> >> be its power on default. In some ways a "canary" approach is uglier
> >> but it also might be more reliable across more configurations?
> I don't have enough knowledge about DWC2 and i also don't have the
> databook to figure out if there is a magic register which could be used
> for the canary approach. But all these different platforms, host vs
> gadget role, different low modes let me think the resulting solution
> would be also fragile and ugly.

I won't admit to having a DWC2 databook. ;-)

...but don't think it's too hard to find a good canary. What about
"GAHBCFG_GLBL_INTR_EN" ? From a quick glance it looks like the driver
seems to set that bit during driver startup and then it stays on until
driver shutdown. The databook that I definitely won't admit to having
almost certainly says that this register resets to 0 on all hardware
and it's applicable to both host and device. I think you could say
that if the register is 0 at resume time that registers must have been
lost and you can restore them.

I guess if that doesn't work then "GUSBCFG_TOUTCAL" could be used (I
think that resets to 0 but must be initted to non-0 by the driver).

Yet another register that could probably work as a canary would be
"GINTMSK". I believe that inits to all 0 (everything is masked) and
obviously to use the device we've got to unmask _some_ interrupts.

I can look for more, if need be.

-Doug

Re: [PATCH V2 14/16] WIP: usb: dwc2: Implement recovery after PM domain off

[Stefan Wahren]

Hi Doug,

Am 15.08.24 um 21:37 schrieb Doug Anderson:
> Hi,
>
> On Wed, Aug 14, 2024 at 2:48 PM Stefan Wahren <wahrenst@gmx.net> wrote:
>>>> You're saying that your
>>>> registers get saved _unless_ the power domain gets turned off, right?
>> On BCM2835 there is no need to store the registers because there is no
>> power management supported by USB core except of the power domain. So
>> DWC2 don't expect a register loss.
>>>> ...and the device core keeps power domains on for suspended devices if
>>>> they are wakeup sources, which makes sense.
>>>>
>>>> So with that, your patch sounds like a plausible way to do it. I guess
>>>> one other way to do it would be some sort of "canary" approach. You
>>>> could _always_ save registers and then, at resume time, you could
>>>> detect if some "canary" register had reset to its power-on default. If
>>>> you see this then you can assume power was lost and re-init all the
>>>> registers. This could be pretty much any register that you know won't
>>>> be its power on default. In some ways a "canary" approach is uglier
>>>> but it also might be more reliable across more configurations?
>> I don't have enough knowledge about DWC2 and i also don't have the
>> databook to figure out if there is a magic register which could be used
>> for the canary approach. But all these different platforms, host vs
>> gadget role, different low modes let me think the resulting solution
>> would be also fragile and ugly.
> I won't admit to having a DWC2 databook. ;-)
you convinced me of the canary approach. I missed one critical point the
whole time. The used power domain notifier can/will be called while the
USB clock is disabled. So this worked for the Raspberry Pi because the
clock is fixed.
> ...but don't think it's too hard to find a good canary. What about
> "GAHBCFG_GLBL_INTR_EN" ? From a quick glance it looks like the driver
> seems to set that bit during driver startup and then it stays on until
> driver shutdown. The databook that I definitely won't admit to having
> almost certainly says that this register resets to 0 on all hardware
> and it's applicable to both host and device. I think you could say
> that if the register is 0 at resume time that registers must have been
> lost and you can restore them.
There are several reason to not use the GAHBCFG_GLBL_INTR_EN. One is the
fact that the driver disabled this flag at several places, not just on
shutdown. Another reason is that the register layout of GAHBCFG on
BCM2835 is customized ( see last page of datasheet [1]).

I dumped the relevant registers with a unmodified dwc2 driver and the
outcome is a little bit unexpected (0 = PRE_POWER_OFF, 3 = POWER_ON):

[  169.101071] dwc2 20980000.usb: dwc2_suspend enter GAHBCFG = 00000031
[  169.101143] dwc2 20980000.usb: dwc2_suspend enter GUSBCFG = 20001707
[  169.101172] dwc2 20980000.usb: dwc2_suspend enter GINTMSK = f3000806
[  169.105888] dwc2 20980000.usb: dwc2_power_notifier: 0 GAHBCFG = 00000031
[  169.105962] dwc2 20980000.usb: dwc2_power_notifier: 0 GUSBCFG = 20001707
[  169.105994] dwc2 20980000.usb: dwc2_power_notifier: 0 GINTMSK = f3000806
[  174.248046] dwc2 20980000.usb: dwc2_power_notifier: 3 GAHBCFG = 0000001f
[  174.248118] dwc2 20980000.usb: dwc2_power_notifier: 3 GUSBCFG = 20402700
[  174.248148] dwc2 20980000.usb: dwc2_power_notifier: 3 GINTMSK = f3000806
[  174.253086] dwc2 20980000.usb: dwc2_resume enter: GAHBCFG = 0000001f
[  174.253162] dwc2 20980000.usb: dwc2_resume enter: GUSBCFG = 20402700
[  174.253190] dwc2 20980000.usb: dwc2_resume enter: GINTMSK = f3000806
> I guess if that doesn't work then "GUSBCFG_TOUTCAL" could be used (I
> think that resets to 0 but must be initted to non-0 by the driver).
Yes this looks good and match with the trace above. The driver seems to
initialize this once and a quick test seems to work so far. I will stick
to this.
> Yet another register that could probably work as a canary would be
> "GINTMSK". I believe that inits to all 0 (everything is masked) and
> obviously to use the device we've got to unmask _some_ interrupts.
I don't know why but this didn't worked according to trace, but i also
didn't noticed a interrupt after enabling of the power domain.Thanks [1]
-
https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
> I can look for more, if need be.
>
> -Doug
>

[PATCH V2 15/16] serial: 8250_bcm2835aux: add PM suspend/resume support

[Stefan Wahren]

This adds suspend/resume support for the 8250_bcm2835aux
driver to provide power management support on attached
devices.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/tty/serial/8250/8250_bcm2835aux.c | 37 +++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index 121a5ce86050..36e2bb34d82b 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -13,6 +13,7 @@
  */

 #include <linux/clk.h>
+#include <linux/console.h>
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -213,11 +214,47 @@ static const struct acpi_device_id bcm2835aux_serial_acpi_match[] = {
 };
 MODULE_DEVICE_TABLE(acpi, bcm2835aux_serial_acpi_match);

+static int bcm2835aux_suspend(struct device *dev)
+{
+	struct bcm2835aux_data *data = dev_get_drvdata(dev);
+	struct uart_8250_port *up = serial8250_get_port(data->line);
+
+	serial8250_suspend_port(data->line);
+
+	if (device_may_wakeup(dev))
+		return 0;
+
+	if (uart_console(&up->port) && !console_suspend_enabled)
+		return 0;
+
+	clk_disable_unprepare(data->clk);
+	return 0;
+}
+
+static int bcm2835aux_resume(struct device *dev)
+{
+	struct bcm2835aux_data *data = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
+
+	serial8250_resume_port(data->line);
+
+	return 0;
+}
+
+static const struct dev_pm_ops bcm2835aux_dev_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(bcm2835aux_suspend, bcm2835aux_resume)
+};
+
 static struct platform_driver bcm2835aux_serial_driver = {
 	.driver = {
 		.name = "bcm2835-aux-uart",
 		.of_match_table = bcm2835aux_serial_match,
 		.acpi_match_table = bcm2835aux_serial_acpi_match,
+		.pm = pm_ptr(&bcm2835aux_dev_pm_ops),
 	},
 	.probe  = bcm2835aux_serial_probe,
 	.remove_new = bcm2835aux_serial_remove,
--
2.34.1

Re: [PATCH V2 15/16] serial: 8250_bcm2835aux: add PM suspend/resume support

[Ulf Hansson]

On Sun, 28 Jul 2024 at 15:07, Stefan Wahren <wahrenst@gmx.net> wrote:
>
> This adds suspend/resume support for the 8250_bcm2835aux
> driver to provide power management support on attached
> devices.
>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  drivers/tty/serial/8250/8250_bcm2835aux.c | 37 +++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
> index 121a5ce86050..36e2bb34d82b 100644
> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
> @@ -13,6 +13,7 @@
>   */
>
>  #include <linux/clk.h>
> +#include <linux/console.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -213,11 +214,47 @@ static const struct acpi_device_id bcm2835aux_serial_acpi_match[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, bcm2835aux_serial_acpi_match);
>
> +static int bcm2835aux_suspend(struct device *dev)
> +{
> +       struct bcm2835aux_data *data = dev_get_drvdata(dev);
> +       struct uart_8250_port *up = serial8250_get_port(data->line);
> +
> +       serial8250_suspend_port(data->line);
> +
> +       if (device_may_wakeup(dev))
> +               return 0;
> +
> +       if (uart_console(&up->port) && !console_suspend_enabled)
> +               return 0;
> +
> +       clk_disable_unprepare(data->clk);
> +       return 0;
> +}
> +
> +static int bcm2835aux_resume(struct device *dev)
> +{
> +       struct bcm2835aux_data *data = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = clk_prepare_enable(data->clk);

Doesn't this create clk prepare/enable - unprepare/disable imbalance
problem when the uart is configured for system wakeup?

> +       if (ret)
> +               return ret;
> +
> +       serial8250_resume_port(data->line);
> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops bcm2835aux_dev_pm_ops = {
> +       SYSTEM_SLEEP_PM_OPS(bcm2835aux_suspend, bcm2835aux_resume)
> +};
> +
>  static struct platform_driver bcm2835aux_serial_driver = {
>         .driver = {
>                 .name = "bcm2835-aux-uart",
>                 .of_match_table = bcm2835aux_serial_match,
>                 .acpi_match_table = bcm2835aux_serial_acpi_match,
> +               .pm = pm_ptr(&bcm2835aux_dev_pm_ops),
>         },
>         .probe  = bcm2835aux_serial_probe,
>         .remove_new = bcm2835aux_serial_remove,
> --
> 2.34.1
>

Kind regards
Uffe

Re: [PATCH V2 15/16] serial: 8250_bcm2835aux: add PM suspend/resume support

[Stefan Wahren]

Am 14.08.24 um 14:18 schrieb Ulf Hansson:
> On Sun, 28 Jul 2024 at 15:07, Stefan Wahren <wahrenst@gmx.net> wrote:
>> This adds suspend/resume support for the 8250_bcm2835aux
>> driver to provide power management support on attached
>> devices.
>>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>>   drivers/tty/serial/8250/8250_bcm2835aux.c | 37 +++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
>> index 121a5ce86050..36e2bb34d82b 100644
>> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
>> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
>> @@ -13,6 +13,7 @@
>>    */
>>
>>   #include <linux/clk.h>
>> +#include <linux/console.h>
>>   #include <linux/io.h>
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>> @@ -213,11 +214,47 @@ static const struct acpi_device_id bcm2835aux_serial_acpi_match[] = {
>>   };
>>   MODULE_DEVICE_TABLE(acpi, bcm2835aux_serial_acpi_match);
>>
>> +static int bcm2835aux_suspend(struct device *dev)
>> +{
>> +       struct bcm2835aux_data *data = dev_get_drvdata(dev);
>> +       struct uart_8250_port *up = serial8250_get_port(data->line);
>> +
>> +       serial8250_suspend_port(data->line);
>> +
>> +       if (device_may_wakeup(dev))
>> +               return 0;
>> +
>> +       if (uart_console(&up->port) && !console_suspend_enabled)
>> +               return 0;
>> +
>> +       clk_disable_unprepare(data->clk);
>> +       return 0;
>> +}
>> +
>> +static int bcm2835aux_resume(struct device *dev)
>> +{
>> +       struct bcm2835aux_data *data = dev_get_drvdata(dev);
>> +       int ret;
>> +
>> +       ret = clk_prepare_enable(data->clk);
> Doesn't this create clk prepare/enable - unprepare/disable imbalance
> problem when the uart is configured for system wakeup?
Thanks, i will send a follow-up

Regards
>
>> +       if (ret)
>> +               return ret;
>> +
>> +       serial8250_resume_port(data->line);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct dev_pm_ops bcm2835aux_dev_pm_ops = {
>> +       SYSTEM_SLEEP_PM_OPS(bcm2835aux_suspend, bcm2835aux_resume)
>> +};
>> +
>>   static struct platform_driver bcm2835aux_serial_driver = {
>>          .driver = {
>>                  .name = "bcm2835-aux-uart",
>>                  .of_match_table = bcm2835aux_serial_match,
>>                  .acpi_match_table = bcm2835aux_serial_acpi_match,
>> +               .pm = pm_ptr(&bcm2835aux_dev_pm_ops),
>>          },
>>          .probe  = bcm2835aux_serial_probe,
>>          .remove_new = bcm2835aux_serial_remove,
>> --
>> 2.34.1
>>
> Kind regards
> Uffe
>

[PATCH V2 16/16] 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

Re: [PATCH V2 09/16] drm/vc4: v3d: simplify clock retrieval

[Maíra Canal]

On 7/28/24 10:00, 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>
> ---
>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>   1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> index 1ede508a67d3..4bf3a8d24770 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -441,20 +441,11 @@ 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);
> +	v3d->clk = devm_clk_get_optional(dev, NULL);
>   	if (IS_ERR(v3d->clk)) {
>   		int ret = PTR_ERR(v3d->clk);
> 

Super nit: you could delete this line ^

Reviewed-by: Maíra Canal <mcanal@igalia.com>

Best Regards,
- Maíra

> -		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;
> -		}
> +		return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
>   	}
> 
>   	ret = platform_get_irq(pdev, 0);
> --
> 2.34.1
> 

Re: [PATCH V2 09/16] drm/vc4: v3d: simplify clock retrieval

[Stefan Wahren]

Hi Maíra,

Am 30.07.24 um 13:23 schrieb Maíra Canal:
> On 7/28/24 10:00, 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>
>> ---
>>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c
>> b/drivers/gpu/drm/vc4/vc4_v3d.c
>> index 1ede508a67d3..4bf3a8d24770 100644
>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
>> @@ -441,20 +441,11 @@ 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);
>> +    v3d->clk = devm_clk_get_optional(dev, NULL);
>>       if (IS_ERR(v3d->clk)) {
>>           int ret = PTR_ERR(v3d->clk);
>>
>
> Super nit: you could delete this line ^
Can you please explain? ret is required for dev_err_probe or do you mean
the empty line after the declaration?
>
> Reviewed-by: Maíra Canal <mcanal@igalia.com>
>
> Best Regards,
> - Maíra
>
>> -        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;
>> -        }
>> +        return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
>>       }
>>
>>       ret = platform_get_irq(pdev, 0);
>> --
>> 2.34.1
>>
>

Re: [PATCH V2 09/16] drm/vc4: v3d: simplify clock retrieval

[Maíra Canal]

Hi Stefan,

On 7/31/24 13:41, Stefan Wahren wrote:
> Hi Maíra,
> 
> Am 30.07.24 um 13:23 schrieb Maíra Canal:
>> On 7/28/24 10:00, 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>
>>> ---
>>>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c
>>> b/drivers/gpu/drm/vc4/vc4_v3d.c
>>> index 1ede508a67d3..4bf3a8d24770 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
>>> @@ -441,20 +441,11 @@ 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);
>>> +    v3d->clk = devm_clk_get_optional(dev, NULL);
>>>       if (IS_ERR(v3d->clk)) {
>>>           int ret = PTR_ERR(v3d->clk);
>>>
>>
>> Super nit: you could delete this line ^
> Can you please explain? ret is required for dev_err_probe or do you mean
> the empty line after the declaration?

Just deleting the empty line after the declaration. It is a super small
nit indeed.

Best Regards,
- Maíra

>>
>> Reviewed-by: Maíra Canal <mcanal@igalia.com>
>>
>> Best Regards,
>> - Maíra
>>
>>> -        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;
>>> -        }
>>> +        return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
>>>       }
>>>
>>>       ret = platform_get_irq(pdev, 0);
>>> -- 
>>> 2.34.1
>>>
>>
> 

Re: [PATCH V2 09/16] drm/vc4: v3d: simplify clock retrieval

[Stefan Wahren]

Hi Maíra,

Am 02.08.24 um 14:56 schrieb Maíra Canal:
> Hi Stefan,
>
> On 7/31/24 13:41, Stefan Wahren wrote:
>> Hi Maíra,
>>
>> Am 30.07.24 um 13:23 schrieb Maíra Canal:
>>> On 7/28/24 10:00, 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>
>>>> ---
>>>>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>> b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>> index 1ede508a67d3..4bf3a8d24770 100644
>>>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>> @@ -441,20 +441,11 @@ 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);
>>>> +    v3d->clk = devm_clk_get_optional(dev, NULL);
>>>>       if (IS_ERR(v3d->clk)) {
>>>>           int ret = PTR_ERR(v3d->clk);
>>>>
>>>
>>> Super nit: you could delete this line ^
>> Can you please explain? ret is required for dev_err_probe or do you mean
>> the empty line after the declaration?
>
> Just deleting the empty line after the declaration. It is a super small
> nit indeed.
AFAIK an empty line after a declaration is part of the coding style. Or
is different in drm?

Best regards
>
> Best Regards,
> - Maíra
>
>>>
>>> Reviewed-by: Maíra Canal <mcanal@igalia.com>
>>>
>>> Best Regards,
>>> - Maíra
>>>
>>>> -        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;
>>>> -        }
>>>> +        return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
>>>>       }
>>>>
>>>>       ret = platform_get_irq(pdev, 0);
>>>> --
>>>> 2.34.1
>>>>
>>>
>>

Re: [PATCH V2 09/16] drm/vc4: v3d: simplify clock retrieval

[Maíra Canal]

Hi Stefan,

On 8/2/24 10:00, Stefan Wahren wrote:
> Hi Maíra,
> 
> Am 02.08.24 um 14:56 schrieb Maíra Canal:
>> Hi Stefan,
>>
>> On 7/31/24 13:41, Stefan Wahren wrote:
>>> Hi Maíra,
>>>
>>> Am 30.07.24 um 13:23 schrieb Maíra Canal:
>>>> On 7/28/24 10:00, 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>
>>>>> ---
>>>>>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>> b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>> index 1ede508a67d3..4bf3a8d24770 100644
>>>>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>> @@ -441,20 +441,11 @@ 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);
>>>>> +    v3d->clk = devm_clk_get_optional(dev, NULL);
>>>>>       if (IS_ERR(v3d->clk)) {
>>>>>           int ret = PTR_ERR(v3d->clk);
>>>>>
>>>>
>>>> Super nit: you could delete this line ^
>>> Can you please explain? ret is required for dev_err_probe or do you mean
>>> the empty line after the declaration?
>>
>> Just deleting the empty line after the declaration. It is a super small
>> nit indeed.
> AFAIK an empty line after a declaration is part of the coding style. Or
> is different in drm?

TBH I just checked the result of `git grep "dev_err_probe"` and I
noticed that most of the times, we don't add an empty line after the
declaration in this case or we don't even create a variable, something
like:

return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");

But it is a pretty small nit. Feel free to ignore it.

Also, let me know if you need me to apply any patches to drm-misc-next.

Best Regards,
- Maíra

> 
> Best regards
>>
>> Best Regards,
>> - Maíra
>>
>>>>
>>>> Reviewed-by: Maíra Canal <mcanal@igalia.com>
>>>>
>>>> Best Regards,
>>>> - Maíra
>>>>
>>>>> -        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;
>>>>> -        }
>>>>> +        return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
>>>>>       }
>>>>>
>>>>>       ret = platform_get_irq(pdev, 0);
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>>
>>>
> 

Re: [PATCH V2 09/16] drm/vc4: v3d: simplify clock retrieval

[Stefan Wahren]

Hi Maíra,

Am 07.08.24 um 16:31 schrieb Maíra Canal:
> Hi Stefan,
>
> On 8/2/24 10:00, Stefan Wahren wrote:
>> Hi Maíra,
>>
>> Am 02.08.24 um 14:56 schrieb Maíra Canal:
>>> Hi Stefan,
>>>
>>> On 7/31/24 13:41, Stefan Wahren wrote:
>>>> Hi Maíra,
>>>>
>>>> Am 30.07.24 um 13:23 schrieb Maíra Canal:
>>>>> On 7/28/24 10:00, 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>
>>>>>> ---
>>>>>>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>>>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>>> b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>>> index 1ede508a67d3..4bf3a8d24770 100644
>>>>>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>>> @@ -441,20 +441,11 @@ 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);
>>>>>> +    v3d->clk = devm_clk_get_optional(dev, NULL);
>>>>>>       if (IS_ERR(v3d->clk)) {
>>>>>>           int ret = PTR_ERR(v3d->clk);
>>>>>>
>>>>>
>>>>> Super nit: you could delete this line ^
>>>> Can you please explain? ret is required for dev_err_probe or do you
>>>> mean
>>>> the empty line after the declaration?
>>>
>>> Just deleting the empty line after the declaration. It is a super small
>>> nit indeed.
>> AFAIK an empty line after a declaration is part of the coding style. Or
>> is different in drm?
>
> TBH I just checked the result of `git grep "dev_err_probe"` and I
> noticed that most of the times, we don't add an empty line after the
> declaration in this case or we don't even create a variable, something
> like:
>
> return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
i will go for the latter variant.

I will send a new version which also addresses your comments regarding
patch 7, so they can be applied at once.

But i still need to wait for some feedback for patch 14 before sending
v3, which is the most important part of the series. But I also hope that
some of the firmware/mailbox/pmdomain patches at the beginning are also
applied before.

>
> But it is a pretty small nit. Feel free to ignore it.
>
> Also, let me know if you need me to apply any patches to drm-misc-next.

Yes, this would be nice to apply the vc4/v3d stuff in the next version,
so the series becomes shorter and easier to handle.

Best regards