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

[Hongliang Yang <hongliang.yang@cixtech.com>]

All five are real. Fixes go into v9; the binding patch is unaffected.

> [Low] missing MFD_SYSCON dependency for USB_CDNSP_SKY1

Confirmed. ARCH_CIX does not select MFD_SYSCON, and without it
syscon_regmap_lookup_by_phandle_args() returns -ENOTSUPP, so probe fails
with a minimal configuration. v9 adds "select MFD_SYSCON".

> [Medium] clock refcount underflow after resume failure

Confirmed, and it is not limited to a failed resume: shutting the system
down or removing the driver after system suspend, without an intervening
resume, reaches cdnsp_sky1_clk_disable_all() and disables the sof and axi
clocks a second time.

v9 records which clocks are on in a bitmap and routes every user through
one idempotent pair, cdnsp_sky1_clk_enable(data, mask) and
cdnsp_sky1_clk_disable(data, mask). A clock that is already on is skipped,
and the enable error path disables only the clocks that call enabled.
CIX_USB_CLK_OFF_NUM goes away; the suspend window is a mask instead.

> [Low] PS_PME_En not cleared in cdns_sky1_platform_suspend()

Confirmed, the read-modify-write keeps whatever PS_PME_En was left in the
register. v9 clears PS_PME_En together with PS_MASK before setting the new
power state.

You are also right that the core drops the return value. That means
-ETIMEDOUT does not prevent the half-suspended state, so v9 restores PS_D0
when the D3 entry times out, as the vendor driver does, and keeps the error
return for when the core propagates it.

> [Medium] cdns_sky1_pdata leak in cdnsp_sky1_probe()

Confirmed. of_platform_bus_create() returns 0 when the child device is not
created, so the if (ret) branch after of_platform_populate() never runs.

v9 checks the children twice. Before the populate it requires exactly one
enabled cdns,usb3 child node, so the auxdata platform data can never be
handed to more than one device, which would be a double free in
platform_device_release(). After the populate it looks for the created child
and, when there is none, frees the platform data and fails probe.

> [Medium] unconditional clock gating in cdnsp_sky1_system_suspend()

Confirmed, the function contradicts the comment above it. v9 gates the
clocks only when both conditions hold:

  - the child device, which owns the shared wakeup interrupt, does not have
    wakeup enabled;
  - AXI_CLOCK_VALID reads 0, that is the controller has stopped using AXI.

The second test answers the device/idle role and the D3 entry timeout cases
without depending on the role, and it is the same status bit that
cdns_sky1_platform_suspend() polls on a successful D3 entry.

Best regards,
Hongliang Yang