Re: [PATCH v2 5/9] usb: chipidea: usb2: add SpacemiT K1 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Dynamic dual-role switching via `usb-role-switch` or `extcon` is completely broken when `CI_HDRC_DUAL_ROLE_NOT_OTG` is used.
- [Medium] Unsafe unconditional accesses to the OTGSC register on platforms without OTG registers.
--

commit 8804983329a666f72771103794460f9ba731c4b4
Author: Junzhong Pan <panjunzhong@linux.spacemit.com>

usb: chipidea: usb2: add SpacemiT K1 support

This commit adds a compatible string and platform data for the SpacemiT K1
USB2 controller. It configures the hardware with specific flags to account
for the lack of internal OTG register support.

> diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c
> index f81673edca3c1..606ca367343f3 100644
> --- a/drivers/usb/chipidea/ci_hdrc_usb2.c
> +++ b/drivers/usb/chipidea/ci_hdrc_usb2.c
> @@ -40,10 +40,18 @@ static const struct ci_hdrc_platform_data ci_zevio_pdata = {
>  	.flags		= CI_HDRC_REGS_SHARED | CI_HDRC_FORCE_FULLSPEED,
>  };
>  
> +static const struct ci_hdrc_platform_data ci_k1_pdata = {
> +	.capoffset	= DEF_CAPOFFSET,
> +	.flags		= CI_HDRC_DISABLE_STREAMING |
> +			  CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS |
> +			  CI_HDRC_DUAL_ROLE_NOT_OTG,

[Severity: Medium]
This isn't a bug introduced by this patch, but this flag configuration
exposes a pre-existing flaw in the core driver. Can the K1 SoC bus fabric
generate a slave error for accesses to unimplemented registers within the
USB IP?

During a USB resume following a wakeup event, ci_controller_resume()
unconditionally calls ci_extcon_wakeup_int(), which does a blind read to the
unsupported OTGSC register:

drivers/usb/chipidea/core.c:ci_extcon_wakeup_int() {
        ...
        u32 otgsc = hw_read_otgsc(ci, ~0);
        ...
}

Similarly, if power is lost during suspend while in host mode,
ci_power_lost_work() calls ci_handle_id_switch(), which unconditionally
calls ci_otg_role(), resulting in another read of OTGSC.

If the K1 hardware generates an access fault instead of returning 0 for
unimplemented register reads, could this cause a synchronous load access
fault on resume or wakeup?

[Severity: High]
This is a pre-existing issue, but does dynamic dual-role switching work on
this hardware when using this flag?

The CI_HDRC_DUAL_ROLE_NOT_OTG flag forces ci->is_otg to be false. However,
the role switch framework relies on ci_irq() to process changes. Looking at
ci_irq_handler(), the entire extcon and OTG event processing block is gated
by a check for ci->is_otg:

drivers/usb/chipidea/core.c:ci_irq_handler() {
        ...
        if (ci->is_otg) {
                otgsc = hw_read_otgsc(ci, ~0);
        ...
}

If a Type-C controller or extcon driver invokes usb_role_switch_set() or
ci_cable_notifier(), both call ci_irq(). Since ci->is_otg is false, will
these events be silently ignored by the handler, leaving the port stuck in
its default role despite the standard kernel APIs being registered?

> +};
> +
>  static const struct of_device_id ci_hdrc_usb2_of_match[] = {
>  	{ .compatible = "chipidea,usb2" },
>  	{ .compatible = "xlnx,zynq-usb-2.20a", .data = &ci_zynq_pdata },
>  	{ .compatible = "lsi,zevio-usb", .data = &ci_zevio_pdata },
> +	{ .compatible = "spacemit,k1-usb2", .data = &ci_k1_pdata },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ci_hdrc_usb2_of_match);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-b4-spacemit-k1-usb2-v2-0-f07342ffa051@linux.spacemit.com?part=5