[PATCH 3/3] arm64: dts: cix: add Sky1 USB/DP combo PHY
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Mon Nov 2 00:52:59 PST 2026
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] phy: cix: link rate unit mismatch and PLL disable in udphy_dp_phy_set_rate()
- [High] phy: cix: unclocked MMIO and lane check in udphy_dp_phy_configure()
- [High] phy: cix: clock leak and skipped re-init in cix_usbdp_phy_power_on()
- [Medium] phy: cix: per-lane voltages ignored in udphy_dp_phy_set_voltages()
- [Medium] dt-bindings: phy: cix,sky1-usbdp-phy: missing OF graph port properties
--
commit ffe7df98ad72d8c1a7362fd9f434e85648138040
Author: Gary Yang <gary.yang at cixtech.com>
arm64: dts: cix: add Sky1 USB/DP combo PHY
This series adds the device tree binding, driver, and device tree nodes for
the CIX Sky1 USB/DP combo PHY, supporting SuperSpeedPlus USB and DisplayPort
lane muxing and Type-C orientation switching via the S5 system controller.
> diff --git a/Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml b/Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml
> new file mode 100644
> index 0000000000000..a5b6943f84ebd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml
> @@ -0,0 +1,150 @@
[ ... ]
> + orientation-switch:
> + description:
> + The PHY handles Type-C plug orientation switching.
> + type: boolean
> +
> + mode-switch:
> + description:
> + The PHY handles multiplexing of the USB and DisplayPort lanes.
> + type: boolean
[Severity: Medium]
When a board device tree connects a usb-c-connector node to this PHY for
Type-C orientation and mode switching, the connector links to the provider
via OF graph port or ports endpoints (matched by fwnode_graph_devcon_matches()
in typec_switch_match() and typec_mux_match()).
Because this schema specifies additionalProperties: false without defining
port or ports properties, won't board device trees that wire a Type-C
connector to a cix,sky1-usbdp-phy node fail dtbs_check validation?
[ ... ]
> diff --git a/drivers/phy/cix/phy-cix-usbdp.c b/drivers/phy/cix/phy-cix-usbdp.c
> new file mode 100644
> index 0000000000000..d596e89c74f76
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-usbdp.c
> @@ -0,0 +1,1477 @@
[ ... ]
> +static int sky1_udphy_init(struct cix_udphy *udphy)
> +{
> + int ret;
> + u32 value;
> + const struct cix_udphy_con_dir *con_dir = &udphy->cfg->con_dir[udphy->id];
> +
> + regmap_update_bits(udphy->usbphy_syscon, con_dir->offset,
> + GENMASK(con_dir->bit, con_dir->bit),
> + udphy->flip << con_dir->bit);
> +
> + usleep_range(10, 20);
> +
> + regmap_read(udphy->usbphy_syscon, con_dir->offset, &value);
> +
> + /* usb rcsu reset is default deassert */
> + reset_control_assert(udphy->reset);
> + reset_control_assert(udphy->preset);
> +
> + ret = sky1_udphy_clk_enable(udphy->apb_clk);
> + if (ret) {
> + dev_err(udphy->dev, "Failed to prepare_enable udphy apb clock\n");
> + return ret;
> + }
[ ... ]
> +static int udphy_orien_sw_set(struct typec_switch_dev *sw,
> + enum typec_orientation orien)
> +{
> + struct cix_udphy *udphy = typec_switch_get_drvdata(sw);
> +
> + mutex_lock(&udphy->mutex);
> +
> + if (orien == TYPEC_ORIENTATION_NONE)
> + goto unlock_ret;
> +
> + udphy->flip = (orien == TYPEC_ORIENTATION_REVERSE) ? true : false;
> +
> +unlock_ret:
> + mutex_unlock(&udphy->mutex);
> + return 0;
> +}
[ ... ]
> +static int cix_usbdp_phy_power_on(struct cix_udphy *udphy)
> +{
> + const struct cix_udphy_cfg *phy_cfgs = udphy->cfg;
> + int ret;
> +
> + if (udphy->cur_mode == udphy->next_mode)
> + return 0;
> +
> + if ((udphy->cur_mode == UDPHY_MODE_USB || udphy->cur_mode == UDPHY_MODE_DP) &&
> + udphy->next_mode == UDPHY_MODE_DP_USB)
> + return 0;
> +
> + if (phy_cfgs->udphy_init) {
> + ret = phy_cfgs->udphy_init(udphy);
> + if (ret) {
> + dev_err(udphy->dev, "failed to init udphy\n");
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
[ ... ]
> +static int udphy_dp_phy_power_on(struct phy *phy)
> +{
> + struct cix_udphy *udphy = phy_get_drvdata(phy);
> + int ret = 0;
> +
> + mutex_lock(&udphy->mutex);
> + if (!(udphy->next_mode & UDPHY_MODE_DP))
> + goto unlock;
> +
> + ret = cix_usbdp_phy_power_on(udphy);
> + if (ret)
> + goto unlock;
> +
> + phy_set_bus_width(phy, udphy_dplane_get(udphy));
> + if (udphy->next_mode == UDPHY_MODE_DP)
> + udphy->cur_mode = UDPHY_MODE_DP;
> + else
> + udphy->cur_mode |= UDPHY_MODE_DP;
> +
> +unlock:
> + mutex_unlock(&udphy->mutex);
> + return ret;
> +}
[Severity: High]
How are udphy->apb_clk and the hardware lane configuration kept in sync when
transitioning between Type-C modes and orientations?
First, if USB powers on first (setting udphy->cur_mode = UDPHY_MODE_USB and
enabling udphy->apb_clk once in sky1_udphy_init()), and the Type-C mux then
selects 4-lane DP (udphy->next_mode = UDPHY_MODE_DP) followed by
udphy_dp_phy_power_on():
udphy_dp_phy_power_on()
cix_usbdp_phy_power_on()
sky1_udphy_init()
sky1_udphy_clk_enable(udphy->apb_clk);
udphy->cur_mode = UDPHY_MODE_DP;
sky1_udphy_init() calls clk_prepare_enable(udphy->apb_clk) a second time, and
udphy_dp_phy_power_on() overwrites udphy->cur_mode = UDPHY_MODE_DP, dropping
UDPHY_MODE_USB. When the PHYs later power off, sky1_udphy_exit() runs at most
once, leaking a prepare/enable reference on udphy->apb_clk.
Second, if DP powers on first in 4-lane mode (udphy->cur_mode = UDPHY_MODE_DP,
programmed with sky1_udphy_dp_conf) and udphy->next_mode later changes to
UDPHY_MODE_DP_USB, cix_usbdp_phy_power_on() hits the early return:
if ((udphy->cur_mode == UDPHY_MODE_USB || udphy->cur_mode == UDPHY_MODE_DP) &&
udphy->next_mode == UDPHY_MODE_DP_USB)
return 0;
Doesn't that skip reprogramming the PHY from 4-lane DP to the USB+DP combo
configuration when cix_u3phy_init() is called?
Third, if udphy_orien_sw_set() updates udphy->flip while one sub-PHY is
already active in udphy->cur_mode, cix_usbdp_phy_power_on() also returns 0
without updating udphy->usbphy_syscon or loading the flipped register table
(sky1_udphy_usbdp_conf_flip), while subsequent calls to
udphy_dp_phy_set_rate() and udphy_dp_phy_set_voltages() program the flipped
lanes.
[ ... ]
> +static int udphy_dp_phy_verify_config(struct cix_udphy *udphy,
> + struct phy_configure_opts_dp *dp)
> +{
> + int i;
> +
> + switch (dp->lanes) {
> + case 1:
> + case 2:
> + case 4:
> + break;
> + default:
> + return -EINVAL;
> + }
[ ... ]
> +static int udphy_dp_phy_set_rate(struct cix_udphy *udphy,
> + struct phy_configure_opts_dp *dp)
> +{
> + int ret;
> + const struct phy_link_cfg *conf = NULL;
> +
> + dev_dbg(udphy->dev, "lane %d, rate %x\n", dp->lanes, dp->link_rate);
> +
> + regmap_update_bits(udphy->phy_regmap, PHY_PMA_PLL_CTRL, BIT(1), 0x0);
> + usleep_range(10, 20);
> +
> + switch (dp->link_rate) {
> + case 0x6:
> + conf = &link_cfg[0];
> + ret = regmap_multi_reg_write(udphy->phy_regmap, sky1_udphy_dp_partial_conf,
> + ARRAY_SIZE(sky1_udphy_dp_partial_conf));
> + break;
> + case 0xa:
> + conf = &link_cfg[1];
> + ret = regmap_multi_reg_write(udphy->phy_regmap, sky1_udphy_dp_partial_conf,
> + ARRAY_SIZE(sky1_udphy_dp_partial_conf));
> + break;
> + case 0x14:
> + conf = &link_cfg[2];
> + ret = regmap_multi_reg_write(udphy->phy_regmap, sky1_udphy_dp_partial_conf,
> + ARRAY_SIZE(sky1_udphy_dp_partial_conf));
> + break;
> + case 0x1e:
> + conf = &link_cfg[3];
> + ret = regmap_multi_reg_write(udphy->phy_regmap, sky1_udphy_dp_8p1_conf,
> + ARRAY_SIZE(sky1_udphy_dp_8p1_conf));
> + break;
> + default:
> + return -EINVAL;
> + }
[Severity: High]
In include/linux/phy/phy-dp.h, struct phy_configure_opts_dp defines link_rate
in Mb/s (1620, 2700, 5400, 8100), whereas udphy_dp_phy_set_rate() matches
dp->link_rate against raw DPCD bandwidth codes (0x6, 0xa, 0x14, 0x1e).
Won't standard DisplayPort controller drivers passing link_rate in Mb/s to
phy_configure() -> udphy_dp_phy_configure() -> udphy_dp_phy_set_rate() always
hit the default case and fail with -EINVAL, while also leaving BIT(1) of
PHY_PMA_PLL_CTRL cleared?
[ ... ]
> +static int udphy_dp_phy_set_voltages(struct cix_udphy *udphy,
> + struct phy_configure_opts_dp *dp)
> +{
> + u32 index = dp->voltage[0] * 4 + dp->pre[0];
> + const struct phy_voltage_conf *cfg = volt_cfg;
> +
> + dev_dbg(udphy->dev, "lane %d, vs %d, pe %d\n", dp->lanes, dp->voltage[0],
> + dp->pre[0]);
[Severity: Medium]
udphy_dp_phy_verify_config() validates dp->voltage[i] and dp->pre[i] for each
lane i < dp->lanes, but udphy_dp_phy_set_voltages() computes index solely from
dp->voltage[0] and dp->pre[0] and writes that configuration to all active
lanes.
Should udphy_dp_phy_set_voltages() compute the volt_cfg index per lane using
each lane's own dp->voltage[i] and dp->pre[i] values so per-lane link training
adjustments are applied?
[ ... ]
> +static int udphy_dp_phy_configure(struct phy *phy,
> + union phy_configure_opts *opts)
> +{
> + struct cix_udphy *udphy = phy_get_drvdata(phy);
> + int ret;
> +
> + mutex_lock(&udphy->mutex);
> + ret = udphy_dp_phy_verify_config(udphy, &opts->dp);
> + if (ret)
> + goto unlock;
> +
> + if (opts->dp.set_rate) {
> + ret = udphy_dp_phy_set_rate(udphy, &opts->dp);
> + if (ret) {
> + dev_err(udphy->dev, "failed to set DP link rate\n");
> + goto unlock;
> + }
> + }
[Severity: High]
What happens if udphy_dp_phy_configure() is called when UDPHY_MODE_DP is not
active in udphy->cur_mode, or with more lanes than allocated to DP?
In udphy_dp_phy_power_on(), if !(udphy->next_mode & UDPHY_MODE_DP) (for
example when Type-C is in TYPEC_STATE_SAFE or USB-only mode, or when
cix,default-conf is 0 or 1), udphy_dp_phy_power_on() returns 0 without
enabling udphy->apb_clk or deasserting udphy->preset. A subsequent call to
udphy_dp_phy_configure() will then perform MMIO reads and writes through
udphy->phy_regmap while the APB clock is disabled and APB reset is asserted.
In addition, udphy_dp_phy_verify_config() accepts dp->lanes == 4 without
checking against udphy_dplane_get(udphy), so a 4-lane DP configuration in
2-lane UDPHY_MODE_DP_USB mode will overwrite the active USB lane registers in
udphy_dp_phy_set_rate() and udphy_dp_phy_set_voltages().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102082112.2306326-1-gary.yang@cixtech.com?part=3
More information about the linux-phy
mailing list