[PATCH] media: cec: meson: ao-cec: enable the controller clock at probe

[PATCH] media: cec: meson: ao-cec: enable the controller clock at probe

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

The CEC core calls .adap_log_addr() before .adap_enable(), so the first
register access can happen while the controller is still in power-off
mode (CEC_GEN_CNTL clock field 0). The probe only pulses the reset bit
and leaves the clock field at zero.

On S4 an access to the powered-off controller never completes: it
stalls the AO bus and hard-hangs the SoC. This triggers on every HDMI
unplug, because that invalidates the physical address and makes the
core rewrite the logical address.

Leave the controller with its clock running after the reset at probe,
as .adap_enable() would.

Fixes: 7ec2c0f72cb1 ("media: platform: Add Amlogic Meson AO CEC Controller driver")
Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
---
Tool use (Documentation/process/generated-content.rst):
This work was done with the help of an AI coding assistant, Claude
(Anthropic), used through Claude Code over many sessions. It did a large
part of the hands-on work: it read the vendor kernel and the firmware
interfaces, probed registers on the board, drafted the code and the
changelogs, and ran the builds, checkpatch, dt_binding_check and the
tests on the hardware. I directed the work, reviewed every change and
the final patches, and take responsibility for them. There is no single
prompt to quote; the sessions were of the form "find out why X fails on
this board, fix it, test it on the box".

Tested on an Amediatech X98Q TV box (Amlogic S905W2, S4 family, 2 GiB),
vendor U-Boot and BL31/OP-TEE, Linux 7.3 plus this work.

 drivers/media/cec/platform/meson/ao-cec.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/media/cec/platform/meson/ao-cec.c b/drivers/media/cec/platform/meson/ao-cec.c
index 2df0d3743077..dd4d91d7b927 100644
--- a/drivers/media/cec/platform/meson/ao-cec.c
+++ b/drivers/media/cec/platform/meson/ao-cec.c
@@ -674,8 +674,25 @@ static int meson_ao_cec_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto out_probe_notify;
 
-	/* Setup Hardware */
-	writel_relaxed(CEC_GEN_CNTL_RESET,
+	/*
+	 * Setup Hardware.
+	 *
+	 * Leave the controller with its gated clock running. The CEC core
+	 * calls .adap_log_addr() before it ever calls .adap_enable(), so the
+	 * first register access happens while the block would otherwise still
+	 * be in power-off mode (CEC_GEN_CNTL clk field == 0). On Amlogic S4
+	 * that access never completes: it stalls the AO bus and hard-hangs the
+	 * SoC - every CPU stops, IRQs included - which happens on every HDMI
+	 * hot-unplug, because that is what invalidates the CEC physical
+	 * address.
+	 */
+	writel_relaxed(CEC_GEN_CNTL_RESET |
+		       FIELD_PREP(CEC_GEN_CNTL_CLK_CTRL_MASK,
+				  CEC_GEN_CNTL_CLK_ENABLE),
+		       ao_cec->base + CEC_GEN_CNTL_REG);
+	fsleep(100);
+	writel_relaxed(FIELD_PREP(CEC_GEN_CNTL_CLK_CTRL_MASK,
+				  CEC_GEN_CNTL_CLK_ENABLE),
 		       ao_cec->base + CEC_GEN_CNTL_REG);
 
 	return 0;

base-commit: 7be4bc52fd9e0e55dc9a2e0232b89427d2c4b614
-- 
2.56.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] media: cec: meson: ao-cec: enable the controller clock at probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: cec: meson: ao-cec: hardware reset and clock enable run after registration in meson_ao_cec_probe()
--

commit 217c22ea59bd67681b3e3f366c510df7ceb9177a
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

media: cec: meson: ao-cec: enable the controller clock at probe

This commit updates meson_ao_cec_probe() to enable the CEC controller's
clock in CEC_GEN_CNTL_REG while pulsing the reset bit and to leave the clock
enabled after reset. This is intended to prevent AO bus stalls on Amlogic S4
when .adap_log_addr() accesses the controller before .adap_enable() has been
called.

> diff --git a/drivers/media/cec/platform/meson/ao-cec.c b/drivers/media/cec/platform/meson/ao-cec.c
> index 2df0d37430779..dd4d91d7b927c 100644
> --- a/drivers/media/cec/platform/meson/ao-cec.c
> +++ b/drivers/media/cec/platform/meson/ao-cec.c
> @@ -674,8 +674,25 @@ static int meson_ao_cec_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto out_probe_notify;
>  
> -	/* Setup Hardware */
> -	writel_relaxed(CEC_GEN_CNTL_RESET,
> +	/*
> +	 * Setup Hardware.
> +	 *
> +	 * Leave the controller with its gated clock running. The CEC core
> +	 * calls .adap_log_addr() before it ever calls .adap_enable(), so the
> +	 * first register access happens while the block would otherwise still
> +	 * be in power-off mode (CEC_GEN_CNTL clk field == 0). On Amlogic S4
> +	 * that access never completes: it stalls the AO bus and hard-hangs the
> +	 * SoC - every CPU stops, IRQs included - which happens on every HDMI
> +	 * hot-unplug, because that is what invalidates the CEC physical
> +	 * address.
> +	 */
> +	writel_relaxed(CEC_GEN_CNTL_RESET |
> +		       FIELD_PREP(CEC_GEN_CNTL_CLK_CTRL_MASK,
> +				  CEC_GEN_CNTL_CLK_ENABLE),
> +		       ao_cec->base + CEC_GEN_CNTL_REG);
> +	fsleep(100);
> +	writel_relaxed(FIELD_PREP(CEC_GEN_CNTL_CLK_CTRL_MASK,
> +				  CEC_GEN_CNTL_CLK_ENABLE),
>  		       ao_cec->base + CEC_GEN_CNTL_REG);

[Severity: High]
Should this hardware reset and clock enable sequence be moved to run before
cec_notifier_cec_adap_register() and cec_register_adapter() in
meson_ao_cec_probe()?

In drivers/media/cec/platform/meson/ao-cec.c:meson_ao_cec_probe(), the
adapter is published to the HDMI notifier and registered with userspace
before CEC_GEN_CNTL_REG is written:

	ao_cec->notify = cec_notifier_cec_adap_register(hdmi_dev, NULL,
							ao_cec->adap);
	if (!ao_cec->notify) {
		ret = -ENOMEM;
		goto out_probe_clk;
	}

	ret = cec_register_adapter(ao_cec->adap, &pdev->dev);
	if (ret < 0)
		goto out_probe_notify;

	/* Setup Hardware. */
	writel_relaxed(CEC_GEN_CNTL_RESET | ...);
	fsleep(100);
	writel_relaxed(...);

Inside drivers/media/cec/core/cec-notifier.c:cec_notifier_cec_adap_register(),
n->cec_adap is assigned and cec_s_phys_addr() sets the initial physical
address:

	mutex_lock(&n->lock);
	n->cec_adap = adap;
	adap->conn_info = n->conn_info;
	adap->notifier = n;
	if (!adap->adap_controls_phys_addr)
		cec_s_phys_addr(adap, n->phys_addr, false);
	mutex_unlock(&n->lock);

If an HDMI hot-unplug or EDID update invokes cec_notifier_set_phys_addr()
after cec_notifier_cec_adap_register() releases n->lock but before
meson_ao_cec_probe() writes CEC_GEN_CNTL_REG, the call chain:

  cec_notifier_set_phys_addr()
    -> __cec_s_phys_addr()
      -> cec_adap_unconfigure()
        -> meson_ao_cec_set_log_addr()

will still access the controller while CEC_GEN_CNTL_REG has its clock field
at 0, triggering the AO bus stall on Amlogic S4.

Additionally, because /dev/cecX is already live after cec_register_adapter()
and the notifier is active while meson_ao_cec_probe() sleeps in fsleep(100)
with CEC_GEN_CNTL_RESET asserted, a concurrent call to
meson_ao_cec_adap_enable() or meson_ao_cec_set_log_addr() (for example from
udev or a CEC daemon opening the device and issuing CEC_ADAP_S_LOG_ADDRS)
can race with the unlocked reset and CEC_GEN_CNTL_REG writes in probe,
losing the controller configuration.

All prerequisites for the register writes (ao_cec->base, ao_cec->core clock
enable and rate setup, and device_reset_optional()) are already initialized
prior to cec_notifier_cec_adap_register().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192046.208710-1-christian.koever-draxl@student.uibk.ac.at?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic