Re: [PATCH v5 2/3] usb: cdns3: Add CIX Sky1 glue driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] usb: cdns3: unclocked MMIO access and host_dev race in cdnsp_sky1_shutdown()
- [High] usb: cdns3: controller reset in cdnsp_sky1_system_suspend() breaks system resume
- [Medium] usb: cdns3: positive probe return, pointer-sign mismatch, and DT mismatch
- [Medium] usb: cdns3: ignored D3 errors and broken D0 poll in cdns_sky1_platform_suspend()
- [Medium] usb: cdns3: cdnsp_sky1_system_suspend() disables wakeup clocks when wakeup is set
- [Medium] usb: cdns3: clock disable underflow on remove/shutdown after resume failure
- [Medium] usb: cdns3: raw ~AXI_HALT write in cdnsp_sky1_configure_controller()
- [Medium] usb: cdns3: 125us prescaler off-by-one in cdnsp_sky1_configure_controller()
- [Low] usb: cdns3: cdnsp_sky1_drd_init() disables clocks before devm_clk_get()
- [Low] usb: cdns3: dead fields, unused oc_gpio, and wrong compatible in Kconfig
--

commit fcb2f6f1fc38d7bbecb98bf69c74a74c7c5ceec3
Author: Hongliang Yang (杨宏亮) <hongliang.yang@cixtech.com>

usb: cdns3: Add CIX Sky1 glue driver

This commit adds a platform glue driver for the Cadence USBSSP dual-role
controller on the CIX Sky1 SoC, managing clocks, resets, S5 system
controller mode straps, and per-controller glue registers.

> diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig
> index 39ad23d1ada89..03d8e170a98af 100644
> --- a/drivers/usb/cdns3/Kconfig
> +++ b/drivers/usb/cdns3/Kconfig
> @@ -77,6 +77,19 @@ config USB_CDNSP_PCI
>  	  If you choose to build this driver as module it will
>  	  be dynamically linked and module will be called cdnsp-pci.ko
>  
> +config USB_CDNSP_SKY1
> +	tristate "Cadence USB3 support on CIX Sky1 SoC platforms"
> +	depends on USB_CDNS3
> +	depends on ARCH_CIX || COMPILE_TEST
> +	default USB_CDNS3
> +	help
> +	  Glue driver for the Cadence USB dual-role controllers on CIX Sky1
> +	  (device tree compatible cix,sky1-usb3). It enables clocks and resets
> +	  from the SoC, then uses the shared cdns3 platform core (cdns.ko).

[Severity: Low]
This isn't a bug, but the help text for USB_CDNSP_SKY1 mentions compatible
cix,sky1-usb3 whereas cdns_sky1_of_match and
Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml use
cix,sky1-usbssp, and the depends on USB_CDNS3 line is inside the enclosing if
USB_CDNS3 block. Also, in struct cdnsp_sky1 in drivers/usb/cdns3/cdnsp-sky1.h,
are the dr_base and cdnsp_pdev members, as well as oc_gpio in
cdnsp_sky1_probe() (where gpiod_direction_input() is called after requesting
with GPIOD_IN, and the descriptor is never referenced again), still needed?

> diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
> new file mode 100644
> index 0000000000000..1f7870289a35f
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-sky1.c

[ ... ]

> +static int cdns_sky1_platform_suspend(struct device *dev,
> +				      bool suspend, bool wakeup)
> +{
> +	struct cdns *cdns = dev_get_drvdata(dev);
> +	struct platform_device *xhci_dev = cdns->host_dev;
> +	struct usb_hcd  *hcd;
> +	struct device *parent = cdns->dev->parent;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(parent);
> +	u32 value;
> +	int poll;
> +	int ret = 0;
> +	int count = 3;
> +
> +	data->wakeup = wakeup;
> +
> +	if (cdns->role != USB_ROLE_HOST)
> +		return 0;
> +
> +	if (!xhci_dev) {
> +		dev_dbg(dev, "host controller has not been registered\n");
> +		return 0;
> +	}
> +
> +	hcd = dev_get_drvdata(&xhci_dev->dev);
> +	if (!hcd) {
> +		dev_dbg(dev, "host controller have not registered\n");
> +		return 0;
> +	}
> +
> +	if (suspend) {
> +		while (count--) {
> +			value = readl(hcd->regs + XECP_PM_PMCSR);
> +			value &= ~PS_MASK;
> +			value |= PS_D3 | PS_PME_En;
> +			writel(value, hcd->regs + XECP_PM_PMCSR);
> +			/* After controller enters D3, disable AXI and SOF
> +			 * until AXI valid flag changes to 0.
> +			 */
> +			if (sky1_rcsu_handshake(data, AXI_CLOCK_VALID, 0,
> +						100ULL * 1000)) {
> +				u32 ctst;
> +
> +				ctst = readl(data->rcsu_ctst_base);
> +				dev_dbg(dev, "enter D3 failed, register value:%x\n",
> +					ctst);
> +			} else {
> +				break;
> +			}
> +		}
> +		if (count < 0) {
> +			u32 ctst;
> +
> +			ctst = readl(data->rcsu_ctst_base);
> +			dev_err(dev, "enter D3 failed after retries, register value:%x\n",
> +				ctst);
> +		}
> +	} else {
> +		while (count--) {
> +			value = readl(hcd->regs + XECP_PM_PMCSR);
> +			value &= ~PS_MASK;
> +			value |= PS_D0;
> +			value &= ~PS_PME_En;
> +			writel(value, hcd->regs + XECP_PM_PMCSR);
> +			/* Wait power state back to D0 */
> +			poll = readl_poll_timeout(hcd->regs + XECP_PM_PMCSR,
> +						  value,
> +						  (value & PS_MASK) == 0 ||
> +						  value == U32_MAX,
> +						  10, 100ULL * 1000);
> +			if (poll) {
> +				dev_dbg(dev, "exit D3 timeout, power state=0x%lx\n",
> +					value & PS_MASK);
> +			} else {
> +				break;
> +			}
> +		}
> +		if (count < 0) {
> +			dev_err(dev, "exit D3 timeout after retries, power state=0x%lx\n",
> +				value & PS_MASK);
> +		}
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
In cdns_sky1_platform_suspend(), ret is initialized to 0 and never updated, so
when sky1_rcsu_handshake() or readl_poll_timeout() fails all retries (count <
0), the function logs an error but still returns 0 to
cdns3_controller_suspend() or cdns3_controller_resume().

In addition, in the resume (!suspend) branch, readl_poll_timeout() stops when
(value & PS_MASK) == 0 || value == U32_MAX, but unlike sky1_rcsu_handshake()
it does not treat value == U32_MAX as an error, so reading U32_MAX (where
value & PS_MASK is 3) is treated as a successful transition to D0.

Also, in the suspend branch, value |= PS_D3 | PS_PME_En enables PS_PME_En
even when wakeup is false, and when cdns->role != USB_ROLE_HOST the function
returns early without clearing AXI_CLOCK_VALID before
cdnsp_sky1_system_suspend() disables the sof and axi clocks. Should
cdns_sky1_platform_suspend() propagate an error code on failure, check for
U32_MAX on D0 polling, and only set PS_PME_En when wakeup is enabled?

> +static void cdnsp_sky1_configure_controller(struct cdnsp_sky1 *data)
> +{
> +	int clk;
> +	int v0, v1, v2;
> +	u32 val = 0;
> +
> +	if (data->u3_disable) {
> +		dev_dbg(data->dev, "disable u3 port\n");
> +		writel(D_XEC_CFG_3XPORT_MODE_VALUE, data->device_base
> +			+ D_XEC_CFG_3XPORT_MODE);
> +	}
> +	writel(AXI_HALT, data->device_base + D_XEC_AXI_CAP);
> +	writel(AXI_HALT, data->xhci_base + D_XEC_AXI_CAP);
> +	writel(data->axi_bmax_value, data->device_base + D_XEC_AXI_CTRL0);
> +	writel(data->axi_bmax_value, data->xhci_base + D_XEC_AXI_CTRL0);
> +	writel((~(u32)(AXI_HALT)), data->device_base + D_XEC_AXI_CAP);
> +	writel((~(u32)(AXI_HALT)), data->xhci_base + D_XEC_AXI_CAP);

[Severity: Medium]
In cdnsp_sky1_configure_controller(), AXI_HALT is defined as BIT(31). Writing
AXI_HALT and then (~(u32)(AXI_HALT)) directly to D_XEC_AXI_CAP first clears
bits [30:0] to 0 and then writes 0x7fffffff, setting all lower 31 bits to 1.
Should these accesses to D_XEC_AXI_CAP be read-modify-write operations using
readl() and writel() to set and clear only AXI_HALT?

> +	clk = data->sof_clk_freq;
> +	v0 =  25 * clk / 100000000;
> +	v1 = clk / 10000;
> +	v2 = clk / 10;
> +	writel(((v0 > 1) ? v0 - 1 : 1), data->device_base
> +		+ D_XEC_PRE_REG_250NS);
> +	writel((unsigned int)((v1 / 100 > 1) > 0 ? (v1 / 100) - 1 : 1),
> +	       data->device_base + D_XEC_PRE_REG_1US);
> +	writel((unsigned int)((v1 / 10 > 1) > 0 ? (v1 / 10) - 1 : 1),
> +	       data->device_base + D_XEC_PRE_REG_10US);
> +	writel(((v1) > 1 ? v1 - 1 : 1), data->device_base
> +		+ D_XEC_PRE_REG_100US);
> +	writel((unsigned int)((125 * clk / 1000000) > 1 ? (125 * clk / 1000000) : 1),
> +	       data->device_base + D_XEC_PRE_REG_125US);

[Severity: Medium]
In cdnsp_sky1_configure_controller(), all of the other prescaler registers
subtract 1 from the cycle count when the value is greater than 1 (for example,
for the 8 MHz SOF clock, 1us writes 7, 10us writes 79, 100us writes 799, and
1ms writes 7999). However, all four 125us register writes
(D_XEC_PRE_REG_125US, D_XEC_LPM_PRE_REG_125US, XEC_PRE_REG_125US, and
XEC_LPM_PRE_REG_125US) check ((125 * clk / 1000000) > 1) and write
(125 * clk / 1000000) without subtracting 1 (writing 1000 instead of 999 for
8 MHz, and 4 instead of 3 for 32 kHz).

Also, XEC_CFG_3XPORT_MODE on xhci_base handles both data->u3_disable and
data->ssp_disable, whereas D_XEC_CFG_3XPORT_MODE on device_base only checks
data->u3_disable. Should the four 125us prescaler calculations subtract 1, and
should data->ssp_disable also be applied to D_XEC_CFG_3XPORT_MODE?

[ ... ]

> +static int cdnsp_sky1_drd_init(struct cdnsp_sky1 *data)
> +{
> +	int ret = 0;
> +
> +	reset_control_assert(data->reset);
> +	reset_control_assert(data->preset);
> +	cdnsp_sky1_clk_disable_all(data->dev);
> +	ret = cdnsp_sky1_clk_enable_all(data->dev);

[Severity: Low]
In cdnsp_sky1_drd_init(), cdnsp_sky1_clk_disable_all() is called before
cdnsp_sky1_clk_enable_all(). Because devm_clk_get() is called inside
cdnsp_sky1_clk_enable_all() rather than once in cdnsp_sky1_probe(),
data->cix_usb_clks[] still contains NULL pointers when
cdnsp_sky1_clk_disable_all() runs during probe. Should the clocks be acquired
with devm_clk_get() in cdnsp_sky1_probe() before calling
cdnsp_sky1_drd_init()?

[ ... ]

> +static int cdnsp_sky1_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct of_dev_auxdata cdns_sky1_auxdata[] = {
> +		{ .compatible = "cdns,usb3", },
> +		{},
> +	};
> +	struct cdnsp_sky1 *data;
> +	struct cdns3_platform_data *cdns_sky1_pdata;
> +	int ret = 0;
> +

[ ... ]

> +	platform_set_drvdata(pdev, data);
> +	data->dev = dev;
> +	ret = of_alias_get_id(dev->of_node, "usb");
> +	if (ret == -ENODEV) {
> +		if (device_property_read_u32(dev, "id", &ret))
> +			ret = -ENODEV;
> +	}
> +	if (ret < 0 || ret >=  SKY1_USB_S5_NUM) {
> +		dev_err(dev, "get alias failed.\n");
> +		return ret;
> +	}
> +	data->id = ret;
> +	data->usb_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
> +							   "cix,usb-syscon");
> +	if (IS_ERR(data->usb_syscon)) {
> +		dev_err(dev, "Unable to get cix,usb-syscon regmap");
> +		return PTR_ERR(data->usb_syscon);
> +	}
> +	data->u3_disable = device_property_read_bool(dev, "u3-port-disable");
> +	data->ssp_disable = device_property_read_bool(dev, "ssp-disable");

[Severity: Medium]
In cdnsp_sky1_probe(), can this ID lookup and property parsing cause a few
issues?

1. ret is declared as int and &ret is passed to device_property_read_u32(),
which expects u32 * and triggers a -Wpointer-sign warning.

2. If ret >= SKY1_USB_S5_NUM, cdnsp_sky1_probe() returns a positive value to
really_probe() instead of a negative errno such as -EINVAL.

3. When CONFIG_OF is disabled (under COMPILE_TEST), of_alias_get_id() returns
-ENOSYS rather than -ENODEV, skipping the device_property_read_u32() fallback.

4. Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml sets
unevaluatedProperties: false and does not define id, u3-port-disable,
ssp-disable, or oc-gpios, nor does its example include a usb alias, so probing
the DT binding example will fail with -ENODEV.

[ ... ]

> +static void cdnsp_sky1_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	pm_runtime_get_sync(dev);
> +	of_platform_depopulate(dev);
> +	sky1_put_addr(data->xhci_base);
> +	sky1_put_addr(data->device_base);
> +	reset_control_assert(data->reset);
> +	reset_control_assert(data->preset);
> +	cdnsp_sky1_clk_disable_all(dev);
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_put_noidle(dev);
> +}
> +
> +/*
> + * Because the wake-up interrupt and host interrupt are the same interrupt,
> + * closing the axi and sof clock will result in the inability to generate
> + * port status change interrupt.
> + */
> +static int cdnsp_sky1_system_suspend(struct device *dev)
> +{
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	if (!data->wakeup) {
> +		reset_control_assert(data->reset);
> +		reset_control_assert(data->preset);
> +	}
> +	cdnsp_sky1_clk_disable_suspend(dev);
> +	return 0;
> +}

[Severity: Medium]
The comment above cdnsp_sky1_system_suspend() notes that closing the axi and
sof clocks prevents generating the port status change wakeup interrupt.
However, cdnsp_sky1_clk_disable_suspend(dev) (which disables the sof and axi
clocks) is called unconditionally outside the if (!data->wakeup) block. Should
cdnsp_sky1_clk_disable_suspend(dev) and the corresponding
cdnsp_sky1_clk_enable_resume(dev) in cdnsp_sky1_system_resume() also be
guarded by !data->wakeup, or should different clocks be gated when wakeup is
enabled?

> +
> +static int cdnsp_sky1_system_resume(struct device *dev)
> +{
> +	int ret = 0;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	ret = cdnsp_sky1_clk_enable_resume(dev);
> +	if (ret)
> +		return ret;
> +	if (!data->wakeup) {
> +		writel(CIX_USB_AXI_WR_CACHE_VALUE, data->rcsu_axi_base);
> +		cdnsp_sky1_set_mode_by_id(data->dev, MODE_STRAP_OTG);
> +		reset_control_deassert(data->preset);
> +		cdnsp_sky1_configure_controller(data);
> +		reset_control_deassert(data->reset);
> +	}
> +	return 0;
> +}

[Severity: High]
When !data->wakeup during system sleep, cdnsp_sky1_system_suspend() asserts
data->reset and data->preset, which resets all DRD/OTG, xHCI, and gadget
controller registers. On resume, cdnsp_sky1_system_resume() deasserts the
resets and runs cdnsp_sky1_configure_controller(), leaving the DRD block in
its post-reset state with host mode (OTGCMD_HOST_BUS_REQ) disabled.

When the child device then resumes in cdns3_plat_resume() ->
cdns3_controller_resume(), it first calls cdns_sky1_platform_suspend(dev,
false, false), which accesses hcd->regs + XECP_PM_PMCSR before
cdns_drd_host_on() has re-enabled the xHCI block, and then calls
cdns_resume() under spin_lock_irqsave(&cdns->lock, flags) (where
cdns_power_is_lost() for CDNSP_CONTROLLER_V2 either triggers sleeping calls
under the spinlock or skips reinitializing the DRD/host registers). How is the
child cdns3 controller expected to recover its DRD and host state after a full
reset in cdnsp_sky1_system_suspend()?

[Severity: Medium]
Also in cdnsp_sky1_system_resume(), if cdnsp_sky1_clk_enable_resume(dev) fails
to re-enable the first CIX_USB_CLK_OFF_NUM clocks (sof and axi), those two
clocks remain disabled while lpm and apb remain enabled. When
cdnsp_sky1_remove() or cdnsp_sky1_shutdown() later calls
cdnsp_sky1_clk_disable_all(dev), won't it call clk_disable_unprepare() on all
four clocks and underflow the enable/prepare counts for sof and axi?

[ ... ]

> +static void cdnsp_sky1_shutdown(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +	struct device *cdns_dev = NULL;
> +	struct device *gadget_dev = NULL;
> +	struct cdns *cdns = NULL;
> +
> +	if (!device_may_wakeup(dev)) {
> +		/*
> +		 * Find the cdns3 child device, then find its gadget device
> +		 * and release the function driver before disabling clocks.
> +		 * This ensures all register accesses in gadget_unbind_driver
> +		 * complete before clocks are turned off.
> +		 */
> +		device_for_each_child(dev, &cdns_dev, cdnsp_sky1_find_cdns_dev);
> +		if (cdns_dev)
> +			cdns = dev_get_drvdata(cdns_dev);
> +		if (cdns)
> +			device_for_each_child(cdns_dev, &gadget_dev,
> +					      cdnsp_sky1_find_gadget_match);
> +		if (gadget_dev)
> +			device_release_driver(gadget_dev);
> +		if (cdns && cdns->host_dev) {
> +			struct usb_hcd *hcd = platform_get_drvdata(cdns->host_dev);
> +
> +			if (hcd && hcd->irq > 0) {
> +				/*
> +				 * Clear HCD_FLAG_HW_ACCESSIBLE before disable_irq.
> +				 * This prevents usb_hcd_irq from calling xhci_irq
> +				 * (which reads USBSTS) after clocks are disabled.
> +				 */
> +				clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +				disable_irq(hcd->irq);
> +				synchronize_irq(hcd->irq);
> +			}
> +		}
> +		reset_control_assert(data->reset);
> +		reset_control_assert(data->preset);
> +		cdnsp_sky1_clk_disable_all(dev);
> +		put_device(gadget_dev);
> +		put_device(cdns_dev);
> +	}
> +}

[Severity: High]
Can cdnsp_sky1_shutdown() cause unclocked MMIO accesses or race with role
switching during system shutdown?

1. device_shutdown() does not runtime-resume devices before invoking
->shutdown(). If the controller is runtime-suspended when
device_release_driver(gadget_dev) is called, gadget_unbind_driver() ->
cdnsp_gadget_udc_stop() will access controller registers while the device is
in low-power state.

2. cdns->host_dev is accessed without holding cdns->mutex, which can race with
a concurrent role switch running cdns_host_exit() ->
platform_device_unregister(cdns->host_dev).

3. Releasing gadget_dev only unbinds the gadget function driver; cdns->otg_irq
and cdns->dev_irq remain enabled (and when cdns->role == USB_ROLE_DEVICE,
cdns->host_dev is NULL so the controller interrupt is not disabled via
hcd->irq either). After reset_control_assert() and
cdnsp_sky1_clk_disable_all(dev) turn off the APB and AXI clocks, any shared or
pending OTG or gadget interrupt will call cdns_drd_irq() or
cdnsp_irq_handler() and read MMIO registers while the controller is unclocked
and held in reset.

Would it be safer to runtime-resume and depopulate the child device (as in
cdnsp_sky1_remove()) so cdns3 tears down host, gadget, and OTG interrupts
cleanly before asserting resets and disabling clocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103074929.1971195-1-hongliang.yang@cixtech.com?part=2