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

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

[Stefan Wahren]

This series implement the initial Suspend-To-Idle 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 is a loose collection of fixes and improvements.
So cherry-picking 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 implementation isn't perfect and contains workarounds like
patch 4 and 9. So 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
suspend to idle      = 1.33 W

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

Known issues:
- currently it's not possible to power down the USB domain [3]
- there seems to be an issue with the DWC2 suspend handling [4]

[1] - https://github.com/lategoodbye/rpi-zero/issues/9
[2] - https://bugzilla.redhat.com/show_bug.cgi?id=2283978
[3] - https://github.com/raspberrypi/firmware/issues/1894
[4] - https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/T/

Stefan Wahren (11):
  firmware: raspberrypi: Improve timeout warning
  mailbox: bcm2835: Fix timeout during suspend mode
  pmdomain: raspberrypi-power: Adjust packet definition
  pmdomain: raspberrypi-power: Avoid powering down USB
  irqchip/bcm2835: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND
  drm/vc4: hdmi: Handle error case of pm_runtime_resume_and_get
  drm/vc4: hdmi: Disable connector status polling during suspend
  usb: dwc2: debugfs: Print parameter no_clock_gating
  usb: dwc2: Skip clock gating on Broadcom SoCs
  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_hdmi.c            | 18 +++++++++++++++++-
 drivers/irqchip/irq-bcm2835.c             |  4 +++-
 drivers/mailbox/bcm2835-mailbox.c         |  3 ++-
 drivers/pmdomain/bcm/raspberrypi-power.c  | 17 ++++++++++++-----
 drivers/tty/serial/8250/8250_bcm2835aux.c | 23 +++++++++++++++++++++++
 drivers/usb/dwc2/debugfs.c                |  1 +
 drivers/usb/dwc2/params.c                 |  1 +
 9 files changed, 61 insertions(+), 11 deletions(-)

--
2.34.1

[PATCH 01/11] 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>
---
 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 01/11] firmware: raspberrypi: Improve timeout warning

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 392 bytes --]



On 6/30/2024 4:36 PM, Stefan Wahren 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>
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

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

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2229 bytes --]



On 6/30/2024 4:36 PM, Stefan Wahren wrote:
> 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>
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH 03/11] 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>
---
 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

Re: [PATCH 03/11] pmdomain: raspberrypi-power: Adjust packet definition

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 549 bytes --]



On 6/30/2024 4:36 PM, Stefan Wahren wrote:
> 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>
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH 04/11] pmdomain: raspberrypi-power: Avoid powering down USB

[Stefan Wahren]

During supend to idle any request to power off the USB domain
leads to a timeout. As a temporary workaround don't register
the relevant power off handler.

Link: https://github.com/raspberrypi/firmware/issues/1894
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/pmdomain/bcm/raspberrypi-power.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pmdomain/bcm/raspberrypi-power.c b/drivers/pmdomain/bcm/raspberrypi-power.c
index 39d9a52200c3..3e7b84006acc 100644
--- a/drivers/pmdomain/bcm/raspberrypi-power.c
+++ b/drivers/pmdomain/bcm/raspberrypi-power.c
@@ -86,7 +86,14 @@ static void rpi_common_init_power_domain(struct rpi_power_domains *rpi_domains,

 	dom->base.name = name;
 	dom->base.power_on = rpi_domain_on;
-	dom->base.power_off = rpi_domain_off;
+
+	/*
+	 * During supend to idle any request to power off the USB domain
+	 * leads to a timeout. As a temporary workaround don't register
+	 * the relevant power off handler.
+	 */
+	if (strcmp("USB", name))
+		dom->base.power_off = rpi_domain_off;

 	/*
 	 * Treat all power domains as off at boot.
--
2.34.1

[PATCH 05/11] irqchip/bcm2835: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

[Stefan Wahren]

The BCM2835 ARMCTRL interrupt controller doesn't provide any facility
to configure the wakeup sources. That's the reason why this
implementation lacks the irq_set_wake implementation. But this prevent
us from properly entering power management states like "suspend to
idle".

So enable the flags IRQCHIP_SKIP_SET_WAKE and
IRQCHIP_MASK_ON_SUSPEND to let the irqchip core allows and handles
the power management.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/irqchip/irq-bcm2835.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c
index e94e2882286c..6c20604c2242 100644
--- a/drivers/irqchip/irq-bcm2835.c
+++ b/drivers/irqchip/irq-bcm2835.c
@@ -102,7 +102,9 @@ static void armctrl_unmask_irq(struct irq_data *d)
 static struct irq_chip armctrl_chip = {
 	.name = "ARMCTRL-level",
 	.irq_mask = armctrl_mask_irq,
-	.irq_unmask = armctrl_unmask_irq
+	.irq_unmask = armctrl_unmask_irq,
+	.flags = IRQCHIP_MASK_ON_SUSPEND |
+		 IRQCHIP_SKIP_SET_WAKE,
 };

 static int armctrl_xlate(struct irq_domain *d, struct device_node *ctrlr,
--
2.34.1

Re: [PATCH 05/11] irqchip/bcm2835: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]



On 6/30/2024 4:36 PM, 'Stefan Wahren' via BCM-KERNEL-FEEDBACK-LIST,PDL 
wrote:
> The BCM2835 ARMCTRL interrupt controller doesn't provide any facility
> to configure the wakeup sources. That's the reason why this
> implementation lacks the irq_set_wake implementation. But this prevent
> us from properly entering power management states like "suspend to
> idle".
> 
> So enable the flags IRQCHIP_SKIP_SET_WAKE and
> IRQCHIP_MASK_ON_SUSPEND to let the irqchip core allows and handles
> the power management.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH 06/11] 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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index d57c4a5948c8..b3a42b709718 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,11 @@ 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_ERROR("Failed to retain HDMI power domain: %d\n", ret);
+		return status;
+	}

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

On Sun, Jun 30, 2024 at 05:36:47PM GMT, 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>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index d57c4a5948c8..b3a42b709718 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,11 @@ 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_ERROR("Failed to retain HDMI power domain: %d\n", ret);

We need to use drm_err here

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Maíra Canal]

On 6/30/24 12:36, 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>

 From the docs, I see that `DRM_ERROR` was deprecated in favor of
`pr_err()` (although I'm seeing some drivers using `dev_err()`). So,
after this change, this is:

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

It would be nice to have a follow-up patch changing other vc4 files,
as they are using `DRM_ERROR` when returning the error from
`pm_runtime_resume_and_get()`.

Best Regards,
- Maíra

> ---
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index d57c4a5948c8..b3a42b709718 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,11 @@ 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_ERROR("Failed to retain HDMI power domain: %d\n", ret);
> +		return status;
> +	}
> 
>   	if (vc4_hdmi->hpd_gpio) {
>   		if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio))
> --
> 2.34.1
> 

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

[Stefan Wahren]

Hello,

Am 30.06.24 um 17:36 schrieb 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 | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index d57c4a5948c8..b3a42b709718 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,11 @@ 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_ERROR("Failed to retain HDMI power domain: %d\n", ret);
> +		return status;
I noticed today that the enum drm_connector_status also supports
connector_status_unknown. Wouldn't this be a more appropriate return
value in this error case?

Why isn't status initialized with connector_status_unknown at all?

Best regards
> +	}
>
>   	if (vc4_hdmi->hpd_gpio) {
>   		if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio))
> --
> 2.34.1
>

[PATCH 07/11] drm/vc4: hdmi: Disable connector status polling during suspend

[Stefan Wahren]

Suspend of VC4 HDMI will likely triggers a warning from
vc4_hdmi_connector_detect_ctx() during poll of connector status.
The power management will prevent the resume and keep the relevant
power domain disabled.

Since there is no reason to poll the connector status during
suspend, the polling should be disabled during this.

It not possible to use drm_mode_config_helper_suspend() here,
because the callbacks might be called during bind phase and not all
components are fully initialized.

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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index b3a42b709718..e80495cea6ac 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct drm_device *drm,
 static int vc4_hdmi_runtime_suspend(struct device *dev)
 {
 	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
+	struct drm_device *drm = vc4_hdmi->connector.dev;
+
+	/*
+	 * Don't disable polling if it was never initialized
+	 */
+	if (drm && drm->mode_config.poll_enabled)
+		drm_kms_helper_poll_disable(drm);

 	clk_disable_unprepare(vc4_hdmi->hsm_clock);

@@ -3115,6 +3122,7 @@ static int vc4_hdmi_runtime_suspend(struct device *dev)
 static int vc4_hdmi_runtime_resume(struct device *dev)
 {
 	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
+	struct drm_device *drm = vc4_hdmi->connector.dev;
 	unsigned long __maybe_unused flags;
 	u32 __maybe_unused value;
 	unsigned long rate;
@@ -3159,6 +3167,9 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
 	}
 #endif

+	if (drm && drm->mode_config.poll_enabled)
+		drm_kms_helper_poll_enable(drm);
+
 	return 0;

 err_disable_clk:
--
2.34.1

Re: [PATCH 07/11] drm/vc4: hdmi: Disable connector status polling during suspend

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

Hi,

On Sun, Jun 30, 2024 at 05:36:48PM GMT, Stefan Wahren wrote:
> Suspend of VC4 HDMI will likely triggers a warning from
> vc4_hdmi_connector_detect_ctx() during poll of connector status.
> The power management will prevent the resume and keep the relevant
> power domain disabled.
> 
> Since there is no reason to poll the connector status during
> suspend, the polling should be disabled during this.
> 
> It not possible to use drm_mode_config_helper_suspend() here,
> because the callbacks might be called during bind phase and not all
> components are fully initialized.
> 
> 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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index b3a42b709718..e80495cea6ac 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct drm_device *drm,
>  static int vc4_hdmi_runtime_suspend(struct device *dev)
>  {
>  	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> +	struct drm_device *drm = vc4_hdmi->connector.dev;
> +
> +	/*
> +	 * Don't disable polling if it was never initialized
> +	 */
> +	if (drm && drm->mode_config.poll_enabled)
> +		drm_kms_helper_poll_disable(drm);

Does it make sense to add it to runtime_suspend?

What if the board boots without a display connected, and only after a
while one is connected? Wouldn't that prevent the driver from detecting
it?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 07/11] drm/vc4: hdmi: Disable connector status polling during suspend

[Stefan Wahren]

Hi Maxime,

Am 02.07.24 um 15:48 schrieb Maxime Ripard:
> Hi,
>
> On Sun, Jun 30, 2024 at 05:36:48PM GMT, Stefan Wahren wrote:
>> Suspend of VC4 HDMI will likely triggers a warning from
>> vc4_hdmi_connector_detect_ctx() during poll of connector status.
>> The power management will prevent the resume and keep the relevant
>> power domain disabled.
>>
>> Since there is no reason to poll the connector status during
>> suspend, the polling should be disabled during this.
>>
>> It not possible to use drm_mode_config_helper_suspend() here,
>> because the callbacks might be called during bind phase and not all
>> components are fully initialized.
>>
>> 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 | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> index b3a42b709718..e80495cea6ac 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct drm_device *drm,
>>   static int vc4_hdmi_runtime_suspend(struct device *dev)
>>   {
>>   	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
>> +	struct drm_device *drm = vc4_hdmi->connector.dev;
>> +
>> +	/*
>> +	 * Don't disable polling if it was never initialized
>> +	 */
>> +	if (drm && drm->mode_config.poll_enabled)
>> +		drm_kms_helper_poll_disable(drm);
> Does it make sense to add it to runtime_suspend?
i saw that other drm drivers used drm_mode_config_helper_suspend() in
the RUNTIME_PM_OPS. But i agree, it should be better moved to
SYSTEM_SLEEP_PM_OPS.
> What if the board boots without a display connected, and only after a
> while one is connected? Wouldn't that prevent the driver from detecting
> it?
tbh I noticed that HDMI re-detection didn't worked in my setup 6.10-rcX
before this series. I need to investigate ...
>
> Maxime

Re: [PATCH 07/11] drm/vc4: hdmi: Disable connector status polling during suspend

[Stefan Wahren]

Am 03.07.24 um 12:28 schrieb Stefan Wahren:
> Hi Maxime,
>
> Am 02.07.24 um 15:48 schrieb Maxime Ripard:
>> Hi,
>>
>> On Sun, Jun 30, 2024 at 05:36:48PM GMT, Stefan Wahren wrote:
>>> Suspend of VC4 HDMI will likely triggers a warning from
>>> vc4_hdmi_connector_detect_ctx() during poll of connector status.
>>> The power management will prevent the resume and keep the relevant
>>> power domain disabled.
>>>
>>> Since there is no reason to poll the connector status during
>>> suspend, the polling should be disabled during this.
>>>
>>> It not possible to use drm_mode_config_helper_suspend() here,
>>> because the callbacks might be called during bind phase and not all
>>> components are fully initialized.
>>>
>>> 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 | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> index b3a42b709718..e80495cea6ac 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct
>>> drm_device *drm,
>>>   static int vc4_hdmi_runtime_suspend(struct device *dev)
>>>   {
>>>       struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
>>> +    struct drm_device *drm = vc4_hdmi->connector.dev;
>>> +
>>> +    /*
>>> +     * Don't disable polling if it was never initialized
>>> +     */
>>> +    if (drm && drm->mode_config.poll_enabled)
>>> +        drm_kms_helper_poll_disable(drm);
>> Does it make sense to add it to runtime_suspend?
> i saw that other drm drivers used drm_mode_config_helper_suspend() in
> the RUNTIME_PM_OPS. But i agree, it should be better moved to
> SYSTEM_SLEEP_PM_OPS.
>> What if the board boots without a display connected, and only after a
>> while one is connected? Wouldn't that prevent the driver from detecting
>> it?
> tbh I noticed that HDMI re-detection didn't worked in my setup
> 6.10-rcX before this series. I need to investigate ...
Okay, this patch breaks definitely HDMI re-detection. So please ignore
this patch. Sorry, about this mess.

There is another minor issue which already exists before that cause the
following log entry on HDMI reconnect:

[   74.078745] vc4-drm soc:gpu: [drm] User-defined mode not supported:
"1920x1200": 60 154000 1920 1968 2000 2080 1200 1203 1209 1235 0x68 0x9

But in this case HDMI works.

Regards
>>
>> Maxime
>

Re: [PATCH 07/11] drm/vc4: hdmi: Disable connector status polling during suspend

[Dave Stevenson]

Hi Stefan

On Wed, 3 Jul 2024 at 16:32, Stefan Wahren <wahrenst@gmx.net> wrote:
>
> Am 03.07.24 um 12:28 schrieb Stefan Wahren:
> > Hi Maxime,
> >
> > Am 02.07.24 um 15:48 schrieb Maxime Ripard:
> >> Hi,
> >>
> >> On Sun, Jun 30, 2024 at 05:36:48PM GMT, Stefan Wahren wrote:
> >>> Suspend of VC4 HDMI will likely triggers a warning from
> >>> vc4_hdmi_connector_detect_ctx() during poll of connector status.
> >>> The power management will prevent the resume and keep the relevant
> >>> power domain disabled.
> >>>
> >>> Since there is no reason to poll the connector status during
> >>> suspend, the polling should be disabled during this.
> >>>
> >>> It not possible to use drm_mode_config_helper_suspend() here,
> >>> because the callbacks might be called during bind phase and not all
> >>> components are fully initialized.
> >>>
> >>> 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 | 11 +++++++++++
> >>>   1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c
> >>> b/drivers/gpu/drm/vc4/vc4_hdmi.c
> >>> index b3a42b709718..e80495cea6ac 100644
> >>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> >>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> >>> @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct
> >>> drm_device *drm,
> >>>   static int vc4_hdmi_runtime_suspend(struct device *dev)
> >>>   {
> >>>       struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> >>> +    struct drm_device *drm = vc4_hdmi->connector.dev;
> >>> +
> >>> +    /*
> >>> +     * Don't disable polling if it was never initialized
> >>> +     */
> >>> +    if (drm && drm->mode_config.poll_enabled)
> >>> +        drm_kms_helper_poll_disable(drm);
> >> Does it make sense to add it to runtime_suspend?
> > i saw that other drm drivers used drm_mode_config_helper_suspend() in
> > the RUNTIME_PM_OPS. But i agree, it should be better moved to
> > SYSTEM_SLEEP_PM_OPS.
> >> What if the board boots without a display connected, and only after a
> >> while one is connected? Wouldn't that prevent the driver from detecting
> >> it?
> > tbh I noticed that HDMI re-detection didn't worked in my setup
> > 6.10-rcX before this series. I need to investigate ...
> Okay, this patch breaks definitely HDMI re-detection. So please ignore
> this patch. Sorry, about this mess.
>
> There is another minor issue which already exists before that cause the
> following log entry on HDMI reconnect:
>
> [   74.078745] vc4-drm soc:gpu: [drm] User-defined mode not supported:
> "1920x1200": 60 154000 1920 1968 2000 2080 1200 1203 1209 1235 0x68 0x9
>
> But in this case HDMI works.

That's saying the mode specified on the kernel command line via
"video=" is invalid. All other modes enumerated from the EDID should
still be present.

I don't immediately see anything wrong with the mode - it's just DMT
mode 0x44 aka 1920x1200@60Hz with reduced blanking. 154MHz clock is
less than the 162MHz limit for Pi0-3 so should be supported.
Setting the module parameter drm.debug to 0x4 should give you the
extra debug of the reason the mode was rejected via the log message at
[1].

At a guess the firmware has inserted the video= line based on the mode
it configured. Whilst that was useful, we've moved away from that now
by setting "disable_fw_kms_setup=1" in config.txt.

  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_modes.c#L1815-L1816

> Regards
> >>
> >> Maxime
> >
>

Re: [PATCH 07/11] drm/vc4: hdmi: Disable connector status polling during suspend

[Peter Robinson]

Hi Stefan,

> Suspend of VC4 HDMI will likely triggers a warning from
> vc4_hdmi_connector_detect_ctx() during poll of connector status.
> The power management will prevent the resume and keep the relevant
> power domain disabled.
>
> Since there is no reason to poll the connector status during
> suspend, the polling should be disabled during this.

What about HDMI-CEC? I don't know well enough how CEC integrates at
this level but CEC can wake up the device over HDMI from a TV display
for example so if this affects that, while it's maybe not required for
first pass, I know the rpi is used in a lot of media use cases so the
ability to wake up via CEC would certainly be welcomed.

> It not possible to use drm_mode_config_helper_suspend() here,
> because the callbacks might be called during bind phase and not all
> components are fully initialized.
>
> 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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index b3a42b709718..e80495cea6ac 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct drm_device *drm,
>  static int vc4_hdmi_runtime_suspend(struct device *dev)
>  {
>         struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> +       struct drm_device *drm = vc4_hdmi->connector.dev;
> +
> +       /*
> +        * Don't disable polling if it was never initialized
> +        */
> +       if (drm && drm->mode_config.poll_enabled)
> +               drm_kms_helper_poll_disable(drm);
>
>         clk_disable_unprepare(vc4_hdmi->hsm_clock);
>
> @@ -3115,6 +3122,7 @@ static int vc4_hdmi_runtime_suspend(struct device *dev)
>  static int vc4_hdmi_runtime_resume(struct device *dev)
>  {
>         struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> +       struct drm_device *drm = vc4_hdmi->connector.dev;
>         unsigned long __maybe_unused flags;
>         u32 __maybe_unused value;
>         unsigned long rate;
> @@ -3159,6 +3167,9 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
>         }
>  #endif
>
> +       if (drm && drm->mode_config.poll_enabled)
> +               drm_kms_helper_poll_enable(drm);
> +
>         return 0;
>
>  err_disable_clk:
> --
> 2.34.1
>

Re: [PATCH 07/11] drm/vc4: hdmi: Disable connector status polling during suspend

[Stefan Wahren]

Hi Peter,

Am 02.07.24 um 22:02 schrieb Peter Robinson:
> Hi Stefan,
>
>> Suspend of VC4 HDMI will likely triggers a warning from
>> vc4_hdmi_connector_detect_ctx() during poll of connector status.
>> The power management will prevent the resume and keep the relevant
>> power domain disabled.
>>
>> Since there is no reason to poll the connector status during
>> suspend, the polling should be disabled during this.
> What about HDMI-CEC? I don't know well enough how CEC integrates at
> this level but CEC can wake up the device over HDMI from a TV display
> for example so if this affects that, while it's maybe not required for
> first pass, I know the rpi is used in a lot of media use cases so the
> ability to wake up via CEC would certainly be welcomed.
unfortunately i don't know much about HDMI-CEC, too. The only thing I
noticed was that I currently cannot configure vc4 as a wakeup source.

[PATCH 08/11] 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>
---
 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

Re: [PATCH 08/11] usb: dwc2: debugfs: Print parameter no_clock_gating

[Minas Harutyunyan]

On 6/30/24 19:36, Stefan Wahren wrote:
> 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>

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

Re: [PATCH 08/11] usb: dwc2: debugfs: Print parameter no_clock_gating

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]



On 6/30/2024 4:36 PM, 'Stefan Wahren' via BCM-KERNEL-FEEDBACK-LIST,PDL 
wrote:
> 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH 09/11] 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 gatling 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/
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 5a1500d0bdd9..66580de52882 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

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Minas Harutyunyan]

On 6/30/24 19:36, Stefan Wahren wrote:
> 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 gatling workaround this issue.
> 
> Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
> Link: https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/T/__;!!A4F2R9G_pg!ct8iWVOAvVd4m_4YnYx7c3W3MN-1-zNmESEntpanapAXTL3FHFP3YXzzyBZCEdOsDLfQh-a_d-mJT5A$
> 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 5a1500d0bdd9..66580de52882 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
> 

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]



On 6/30/2024 4:36 PM, Stefan Wahren wrote:
> 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 gatling workaround this issue.

Typo: gatling/gating.

> 
> 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/
> 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 5a1500d0bdd9..66580de52882 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;

Could we set this depending upon whether the dwc2 host controller is a 
wake-up source for the system or not?
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Stefan Wahren]

Hi Florian,

Am 04.07.24 um 16:14 schrieb Florian Fainelli:
>
>
> On 6/30/2024 4:36 PM, Stefan Wahren wrote:
>> 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 gatling workaround this issue.
>
> Typo: gatling/gating.
>
>>
>> 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/
>> 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 5a1500d0bdd9..66580de52882 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;
>
> Could we set this depending upon whether the dwc2 host controller is a
> wake-up source for the system or not?

I would prefer to fix the suspend/resume behavior reported here [1]
instead of making tricky workarounds. But i don't have an idea how to
achieve this.

[1] -
https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Lukas Wunner]

On Thu, Jul 04, 2024 at 03:14:50PM +0100, Florian Fainelli wrote:
> On 6/30/2024 4:36 PM, Stefan Wahren wrote:
> > 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

A similar issue was reported for Agilex platforms back in 2021:

https://lore.kernel.org/all/5e8cbce0-3260-2971-484f-fc73a3b2bd28@synopsys.com/

It was fixed by commit 3d8d3504d233 ("usb: dwc2: Add platform specific
data for Intel's Agilex"), which sets the no_clock_gating flag on that
platform.

Looking at drivers/usb/dwc2/params.c, numerous other platforms need
the same flag.

Please amend the commit message to mention the Agilex issue and
resulting commit.


> > --- 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;
> 
> Could we set this depending upon whether the dwc2 host controller is a
> wake-up source for the system or not?

The flag seems to mean whether the platform is actually capable of
disabling the clock of the USB controller.  BCM2835 seems to be
incapable and as a result, even though dwc2_host_enter_clock_gating()
is called, the chip signals interrupts.

There doesn't seem to be a relation to using the controller as a
wakeup source, so your comment doesn't seem to make sense.
If the clock can't be gated, the chip can always serve as a
wakeup source.

The real question is whether BCM2848 platforms likewise cannot disable
the clock of the dwc2 controller or whether this is specific to the
BCM2835.  Right now dwc2_set_bcm_params() is applied to both the
BCM2848 and BCM2835.  If the BCM2848 behaves differently in this
regard, we'd have to duplicate dwc2_set_bcm_params() for the BCM2835.

Thanks,

Lukas

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Stefan Wahren]

Am 05.07.24 um 10:48 schrieb Lukas Wunner:
> On Thu, Jul 04, 2024 at 03:14:50PM +0100, Florian Fainelli wrote:
>> On 6/30/2024 4:36 PM, Stefan Wahren wrote:
>>> 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
> A similar issue was reported for Agilex platforms back in 2021:
>
> https://lore.kernel.org/all/5e8cbce0-3260-2971-484f-fc73a3b2bd28@synopsys.com/
>
> It was fixed by commit 3d8d3504d233 ("usb: dwc2: Add platform specific
> data for Intel's Agilex"), which sets the no_clock_gating flag on that
> platform.
>
> Looking at drivers/usb/dwc2/params.c, numerous other platforms need
> the same flag.
>
> Please amend the commit message to mention the Agilex issue and
> resulting commit.
 From my understanding Samsung noticed this issue at first and
introduced the no_clock_gating flag [1] and they referenced 0112b7ce68ea
("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.") as I did in
this patch. Later some platforms like Rockchip and Agilex followed.

Should i better refer to the Samsung bugfix instead of the Agilex bugfix?
>
>
>>> --- 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;
>> Could we set this depending upon whether the dwc2 host controller is a
>> wake-up source for the system or not?
> The flag seems to mean whether the platform is actually capable of
> disabling the clock of the USB controller.  BCM2835 seems to be
> incapable and as a result, even though dwc2_host_enter_clock_gating()
> is called, the chip signals interrupts.
That's why I was asking about this in the initial bug report. Since I
didn't get a reply, I submitted this workaround.
> There doesn't seem to be a relation to using the controller as a
> wakeup source, so your comment doesn't seem to make sense.
> If the clock can't be gated, the chip can always serve as a
> wakeup source.
I wouldn't conclude that the chip can always serve as a wakeup source
(e.g. power down the USB domain would also prevent this), but i agree
creating a relation between wakeup source and clock gating is a bad idea.
> The real question is whether BCM2848 platforms likewise cannot disable
> the clock of the dwc2 controller or whether this is specific to the
> BCM2835.  Right now dwc2_set_bcm_params() is applied to both the
> BCM2848 and BCM2835.  If the BCM2848 behaves differently in this
> regard, we'd have to duplicate dwc2_set_bcm_params() for the BCM2835.
 From my understand BCM2848 refers to the same SoC, but the ACPI
implementation uses a different ID [2]. So I think this is safe.
>
> Thanks,
>
> Lukas

[1] -
https://lore.kernel.org/linux-usb/20210716050127.4406-1-m.szyprowski@samsung.com/
[2] -
https://patches.linaro.org/project/linux-usb/patch/20210413215834.3126447-2-jeremy.linton@arm.com/

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Lukas Wunner]

On Fri, Jul 05, 2024 at 12:22:33PM +0200, Stefan Wahren wrote:
> Am 05.07.24 um 10:48 schrieb Lukas Wunner:
> > A similar issue was reported for Agilex platforms back in 2021:
> > 
> > https://lore.kernel.org/all/5e8cbce0-3260-2971-484f-fc73a3b2bd28@synopsys.com/
> > 
> > It was fixed by commit 3d8d3504d233 ("usb: dwc2: Add platform specific
> > data for Intel's Agilex"), which sets the no_clock_gating flag on that
> > platform.
> 
> From my understanding Samsung noticed this issue at first and
> introduced the no_clock_gating flag [1] and they referenced 0112b7ce68ea
> ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.") as I did in
> this patch. Later some platforms like Rockchip and Agilex followed.
> 
> Should i better refer to the Samsung bugfix instead of the Agilex bugfix?

I'd say mention both.  The Samsung one because it was the first
occurrence and the Agilex one because it specifically mentions
the interrupt storm which you've also witnessed on the Raspberry Pi.
Samsung's report mentions other symptoms than an interrupt storm.


> > The real question is whether BCM2848 platforms likewise cannot disable
> > the clock of the dwc2 controller or whether this is specific to the
> > BCM2835.  Right now dwc2_set_bcm_params() is applied to both the
> > BCM2848 and BCM2835.  If the BCM2848 behaves differently in this
> > regard, we'd have to duplicate dwc2_set_bcm_params() for the BCM2835.
> 
> From my understand BCM2848 refers to the same SoC, but the ACPI
> implementation uses a different ID [2]. So I think this is safe.
> [2] -
> https://patches.linaro.org/project/linux-usb/patch/20210413215834.3126447-2-jeremy.linton@arm.com/

Careful there, the patch vaguely says...

    With that added and identified as "BCM2848",
    an id in use by other OSs for this device, the dw2
    controller on the BCM2711 will work.

...which sounds like they copy-pasted the BCM2848 id from somewhere else.
I would assume that BCM2848 is really a different SoC and not just
a different name for the BCM2835, but hopefully BroadCom folks will
be able to confirm or deny this (and thus the necessity of the quirk
on BCM2848 and not just on BCM2835).

Thanks,

Lukas

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Stefan Wahren]

Hi Jeremy,

Am 05.07.24 um 17:03 schrieb Lukas Wunner:
> On Fri, Jul 05, 2024 at 12:22:33PM +0200, Stefan Wahren wrote:
>> Am 05.07.24 um 10:48 schrieb Lukas Wunner:
>>> The real question is whether BCM2848 platforms likewise cannot disable
>>> the clock of the dwc2 controller or whether this is specific to the
>>> BCM2835.  Right now dwc2_set_bcm_params() is applied to both the
>>> BCM2848 and BCM2835.  If the BCM2848 behaves differently in this
>>> regard, we'd have to duplicate dwc2_set_bcm_params() for the BCM2835.
>>  From my understand BCM2848 refers to the same SoC, but the ACPI
>> implementation uses a different ID [2]. So I think this is safe.
>> [2] -
>> https://patches.linaro.org/project/linux-usb/patch/20210413215834.3126447-2-jeremy.linton@arm.com/
> Careful there, the patch vaguely says...
>
>      With that added and identified as "BCM2848",
>      an id in use by other OSs for this device, the dw2
>      controller on the BCM2711 will work.
>
> ...which sounds like they copy-pasted the BCM2848 id from somewhere else.
> I would assume that BCM2848 is really a different SoC and not just
> a different name for the BCM2835, but hopefully BroadCom folks will
> be able to confirm or deny this (and thus the necessity of the quirk
> on BCM2848 and not just on BCM2835).
could you please clarify this situation?

Thanks
>
> Thanks,
>
> Lukas

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Jeremy Linton]

Hi,

On 7/5/24 10:21, Stefan Wahren wrote:
> Hi Jeremy,
> 
> Am 05.07.24 um 17:03 schrieb Lukas Wunner:
>> On Fri, Jul 05, 2024 at 12:22:33PM +0200, Stefan Wahren wrote:
>>> Am 05.07.24 um 10:48 schrieb Lukas Wunner:
>>>> The real question is whether BCM2848 platforms likewise cannot disable
>>>> the clock of the dwc2 controller or whether this is specific to the
>>>> BCM2835.  Right now dwc2_set_bcm_params() is applied to both the
>>>> BCM2848 and BCM2835.  If the BCM2848 behaves differently in this
>>>> regard, we'd have to duplicate dwc2_set_bcm_params() for the BCM2835.
>>>  From my understand BCM2848 refers to the same SoC, but the ACPI
>>> implementation uses a different ID [2]. So I think this is safe.
>>> [2] -
>>> https://patches.linaro.org/project/linux-usb/patch/20210413215834.3126447-2-jeremy.linton@arm.com/
>> Careful there, the patch vaguely says...
>>
>>      With that added and identified as "BCM2848",
>>      an id in use by other OSs for this device, the dw2
>>      controller on the BCM2711 will work.
>>
>> ...which sounds like they copy-pasted the BCM2848 id from somewhere else.
>> I would assume that BCM2848 is really a different SoC and not just
>> a different name for the BCM2835, but hopefully BroadCom folks will
>> be able to confirm or deny this (and thus the necessity of the quirk
>> on BCM2848 and not just on BCM2835).
> could you please clarify this situation?

This id comes from the edk2-platforms ACPI tables and is currently used 
by both the rpi3 and rpi4, and AFAIK nothing else as the rpi5-dev work 
is currently only exposing XHCI.

The ID is strictly the USB controller not the SoC. Its a bit confusingly 
named, but something we inherited from the much older windows/edk2 port, 
where it appears that the peripheral HID's were just picked in numerical 
order.

[0] 
https://github.com/tianocore/edk2-platforms/blob/12f68d29abdc9d703f67bd743fdec23ebb1e966e/Platform/RaspberryPi/AcpiTables/GpuDevs.asl#L15

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Lukas Wunner]

On Fri, Jul 05, 2024 at 12:16:14PM -0500, Jeremy Linton wrote:
> > Am 05.07.24 um 17:03 schrieb Lukas Wunner:
> > > Careful there, the patch vaguely says...
> > > 
> > >     With that added and identified as "BCM2848",
> > >     an id in use by other OSs for this device, the dw2
> > >     controller on the BCM2711 will work.
> > > 
> > > ...which sounds like they copy-pasted the BCM2848 id from somewhere else.
> > > I would assume that BCM2848 is really a different SoC and not just
> > > a different name for the BCM2835, but hopefully BroadCom folks will
> > > be able to confirm or deny this (and thus the necessity of the quirk
> > > on BCM2848 and not just on BCM2835).
> 
> This id comes from the edk2-platforms ACPI tables and is currently used by
> both the rpi3 and rpi4, and AFAIK nothing else as the rpi5-dev work is
> currently only exposing XHCI.
> 
> The ID is strictly the USB controller not the SoC. Its a bit confusingly
> named, but something we inherited from the much older windows/edk2 port,
> where it appears that the peripheral HID's were just picked in numerical
> order.
> 
> [0] https://github.com/tianocore/edk2-platforms/blob/12f68d29abdc9d703f67bd743fdec23ebb1e966e/Platform/RaspberryPi/AcpiTables/GpuDevs.asl#L15

So BCM2848, BCM2849, BCM2850 and so on are just made-up IDs
for a Windows/EDK2 port that got cargo-culted into the kernel?
Yikes!

Has anyone checked whether they collide with actual Broadcom products?

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Stefan Wahren]

Am 05.07.24 um 23:14 schrieb Lukas Wunner:
> On Fri, Jul 05, 2024 at 12:16:14PM -0500, Jeremy Linton wrote:
>>> Am 05.07.24 um 17:03 schrieb Lukas Wunner:
>>>> Careful there, the patch vaguely says...
>>>>
>>>>      With that added and identified as "BCM2848",
>>>>      an id in use by other OSs for this device, the dw2
>>>>      controller on the BCM2711 will work.
>>>>
>>>> ...which sounds like they copy-pasted the BCM2848 id from somewhere else.
>>>> I would assume that BCM2848 is really a different SoC and not just
>>>> a different name for the BCM2835, but hopefully BroadCom folks will
>>>> be able to confirm or deny this (and thus the necessity of the quirk
>>>> on BCM2848 and not just on BCM2835).
>> This id comes from the edk2-platforms ACPI tables and is currently used by
>> both the rpi3 and rpi4, and AFAIK nothing else as the rpi5-dev work is
>> currently only exposing XHCI.
>>
>> The ID is strictly the USB controller not the SoC. Its a bit confusingly
>> named, but something we inherited from the much older windows/edk2 port,
>> where it appears that the peripheral HID's were just picked in numerical
>> order.
>>
>> [0] https://github.com/tianocore/edk2-platforms/blob/12f68d29abdc9d703f67bd743fdec23ebb1e966e/Platform/RaspberryPi/AcpiTables/GpuDevs.asl#L15
> So BCM2848, BCM2849, BCM2850 and so on are just made-up IDs
> for a Windows/EDK2 port that got cargo-culted into the kernel?
> Yikes!
>
> Has anyone checked whether they collide with actual Broadcom products?
Using public available information like this [1], I wasn't able to find
any collision.

[1] - https://github.com/anholt/linux/wiki/Devices-with-Videocore-graphics

Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Jeremy Linton]

Hi,

On 7/5/24 16:14, Lukas Wunner wrote:
> On Fri, Jul 05, 2024 at 12:16:14PM -0500, Jeremy Linton wrote:
>>> Am 05.07.24 um 17:03 schrieb Lukas Wunner:
>>>> Careful there, the patch vaguely says...
>>>>
>>>>      With that added and identified as "BCM2848",
>>>>      an id in use by other OSs for this device, the dw2
>>>>      controller on the BCM2711 will work.
>>>>
>>>> ...which sounds like they copy-pasted the BCM2848 id from somewhere else.
>>>> I would assume that BCM2848 is really a different SoC and not just
>>>> a different name for the BCM2835, but hopefully BroadCom folks will
>>>> be able to confirm or deny this (and thus the necessity of the quirk
>>>> on BCM2848 and not just on BCM2835).
>>
>> This id comes from the edk2-platforms ACPI tables and is currently used by
>> both the rpi3 and rpi4, and AFAIK nothing else as the rpi5-dev work is
>> currently only exposing XHCI.
>>
>> The ID is strictly the USB controller not the SoC. Its a bit confusingly
>> named, but something we inherited from the much older windows/edk2 port,
>> where it appears that the peripheral HID's were just picked in numerical
>> order.
>>
>> [0] https://github.com/tianocore/edk2-platforms/blob/12f68d29abdc9d703f67bd743fdec23ebb1e966e/Platform/RaspberryPi/AcpiTables/GpuDevs.asl#L15
> 
> So BCM2848, BCM2849, BCM2850 and so on are just made-up IDs
> for a Windows/EDK2 port that got cargo-culted into the kernel?
> Yikes!

You could say that, but there was some due diligence a couple years ago 
to track down the owner of the pnp id/information at broadcom, and it 
didn't yield anything helpful. Whether they are legitimate seems to be 
lost in time. At this point they are widely/publicly known Ids, without 
apparent conflict.


> 
> Has anyone checked whether they collide with actual Broadcom products?

[PATCH RFT 10/11] 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>
---

Since i don't have a RS485 setup, any test feedback would be great.

 drivers/tty/serial/8250/8250_bcm2835aux.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index 121a5ce86050..cccd2a09cb6f 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -213,11 +213,34 @@ static const struct acpi_device_id bcm2835aux_serial_acpi_match[] = {
 };
 MODULE_DEVICE_TABLE(acpi, bcm2835aux_serial_acpi_match);

+static int __maybe_unused bcm2835aux_suspend(struct device *dev)
+{
+	struct bcm2835aux_data *data = dev_get_drvdata(dev);
+
+	serial8250_suspend_port(data->line);
+
+	return 0;
+}
+
+static int __maybe_unused bcm2835aux_resume(struct device *dev)
+{
+	struct bcm2835aux_data *data = dev_get_drvdata(dev);
+
+	serial8250_resume_port(data->line);
+
+	return 0;
+}
+
+static const struct dev_pm_ops bcm2835aux_dev_pm_ops = {
+	SET_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 = &bcm2835aux_dev_pm_ops,
 	},
 	.probe  = bcm2835aux_serial_probe,
 	.remove_new = bcm2835aux_serial_remove,
--
2.34.1

Re: [PATCH RFT 10/11] serial: 8250_bcm2835aux: add PM suspend/resume support

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]



On 6/30/2024 5:53 PM, Stefan Wahren 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>
> ---
> 
> Since i don't have a RS485 setup, any test feedback would be great.
> 
>   drivers/tty/serial/8250/8250_bcm2835aux.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
> index 121a5ce86050..cccd2a09cb6f 100644
> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
> @@ -213,11 +213,34 @@ static const struct acpi_device_id bcm2835aux_serial_acpi_match[] = {
>   };
>   MODULE_DEVICE_TABLE(acpi, bcm2835aux_serial_acpi_match);
> 
> +static int __maybe_unused bcm2835aux_suspend(struct device *dev)
> +{
> +	struct bcm2835aux_data *data = dev_get_drvdata(dev);
> +
> +	serial8250_suspend_port(data->line);

Don't you also need to disable the clock here, unless the device is a 
wake-up source, and conversely re-enable the clock upon resume?
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH RFT 10/11] serial: 8250_bcm2835aux: add PM suspend/resume support

[Stefan Wahren]

Hi Florian,

Am 04.07.24 um 16:12 schrieb Florian Fainelli:
>
>
> On 6/30/2024 5:53 PM, Stefan Wahren 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>
>> ---
>>
>> Since i don't have a RS485 setup, any test feedback would be great.
>>
>>   drivers/tty/serial/8250/8250_bcm2835aux.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c
>> b/drivers/tty/serial/8250/8250_bcm2835aux.c
>> index 121a5ce86050..cccd2a09cb6f 100644
>> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
>> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
>> @@ -213,11 +213,34 @@ static const struct acpi_device_id
>> bcm2835aux_serial_acpi_match[] = {
>>   };
>>   MODULE_DEVICE_TABLE(acpi, bcm2835aux_serial_acpi_match);
>>
>> +static int __maybe_unused bcm2835aux_suspend(struct device *dev)
>> +{
>> +    struct bcm2835aux_data *data = dev_get_drvdata(dev);
>> +
>> +    serial8250_suspend_port(data->line);
>
> Don't you also need to disable the clock here, unless the device is a
> wake-up source, and conversely re-enable the clock upon resume?
at first I experiment with the pm implementation from 8250_uniphier.c,
but this didn't work as soon as I drop "no_console_suspend" from the
Kernel cmdline. Maybe that's the wrong pattern.

[PATCH 11/11] 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>
---
 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 11/11] ARM: bcm2835_defconfig: Enable SUSPEND

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]



On 6/30/2024 6:19 PM, Stefan Wahren wrote:
> 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>
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH 00/11] ARM: bcm2835: Implement initial S2Idle for Raspberry Pi

[Peter Robinson]

Hi Stefan,

> This series implement the initial Suspend-To-Idle 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.

Firstly a big thank you for this work!

> The series is a loose collection of fixes and improvements.
> So cherry-picking 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 implementation isn't perfect and contains workarounds like
> patch 4 and 9. So there is still room for improvements, but
> at least the system won't freeze forever as before [2].

I've got a test kernel for Fedora that I'm going to test on
RPi3/Zero2W so let me know if you want me to test anything in
particular, I will do my best to give review/test on each individual
test over the next few days.

Cheer,
Peter

> 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
> suspend to idle      = 1.33 W
>
> The series has been tested on the following platforms:
> Raspberry Pi 1 B
> Raspberry Pi 3 A+
> Raspberry Pi 3 B+
>
> Known issues:
> - currently it's not possible to power down the USB domain [3]
> - there seems to be an issue with the DWC2 suspend handling [4]
>
> [1] - https://github.com/lategoodbye/rpi-zero/issues/9
> [2] - https://bugzilla.redhat.com/show_bug.cgi?id=2283978
> [3] - https://github.com/raspberrypi/firmware/issues/1894
> [4] - https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/T/
>
> Stefan Wahren (11):
>   firmware: raspberrypi: Improve timeout warning
>   mailbox: bcm2835: Fix timeout during suspend mode
>   pmdomain: raspberrypi-power: Adjust packet definition
>   pmdomain: raspberrypi-power: Avoid powering down USB
>   irqchip/bcm2835: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND
>   drm/vc4: hdmi: Handle error case of pm_runtime_resume_and_get
>   drm/vc4: hdmi: Disable connector status polling during suspend
>   usb: dwc2: debugfs: Print parameter no_clock_gating
>   usb: dwc2: Skip clock gating on Broadcom SoCs
>   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_hdmi.c            | 18 +++++++++++++++++-
>  drivers/irqchip/irq-bcm2835.c             |  4 +++-
>  drivers/mailbox/bcm2835-mailbox.c         |  3 ++-
>  drivers/pmdomain/bcm/raspberrypi-power.c  | 17 ++++++++++++-----
>  drivers/tty/serial/8250/8250_bcm2835aux.c | 23 +++++++++++++++++++++++
>  drivers/usb/dwc2/debugfs.c                |  1 +
>  drivers/usb/dwc2/params.c                 |  1 +
>  9 files changed, 61 insertions(+), 11 deletions(-)
>
> --
> 2.34.1
>