Message-ID: <Z3vlTwR+SiZQWVh7@linaro.org>
Date: Mon, 6 Jan 2025 16:14:39 +0200
From: Abel Vesa <abel.vesa@...aro.org>
To: Johan Hovold <johan@...nel.org>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	Rajendra Nayak <quic_rjendra@...cinc.com>,
	Sibi Sankar <quic_sibis@...cinc.com>,
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Trilok Soni <quic_tsoni@...cinc.com>,
	Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v5 2/6] usb: typec: Add support for Parade PS8830 Type-C
 Retimer

On 24-12-04 17:24:54, Johan Hovold wrote:
> On Tue, Nov 12, 2024 at 07:01:11PM +0200, Abel Vesa wrote:
> > The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
> > controlled over I2C. It usually sits between a USB/DisplayPort PHY
> > and the Type-C connector, and provides orientation and altmode handling.
> > 
> > The boards that use this retimer are the ones featuring the Qualcomm
> > Snapdragon X Elite SoCs.
> 
> > +static int ps883x_sw_set(struct typec_switch_dev *sw,
> > +			 enum typec_orientation orientation)
> > +{
> > +	struct ps883x_retimer *retimer = typec_switch_get_drvdata(sw);
> > +	int ret = 0;
> > +
> > +	ret = typec_switch_set(retimer->typec_switch, orientation);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&retimer->lock);
> > +
> > +	if (retimer->orientation != orientation) {
> > +		retimer->orientation = orientation;
> > +
> > +		ret = ps883x_set(retimer);
> > +	}
> > +
> > +	mutex_unlock(&retimer->lock);
> > +
> > +	return ret;
> > +}
> 
> This seems to indicate a bigger problem, but I see this function called
> during early resume while the i2c controller is suspended:
> 
> [   54.213900] ------------[ cut here ]------------
> [   54.213942] i2c i2c-2: Transfer while suspended
> [   54.214125] WARNING: CPU: 0 PID: 126 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0x874/0x968 [i2c_core]
> ...
> [   54.214833] CPU: 0 UID: 0 PID: 126 Comm: kworker/0:2 Not tainted 6.13.0-rc1 #11
> [   54.214844] Hardware name: Qualcomm CRD, BIOS 6.0.231221.BOOT.MXF.2.4-00348.1-HAMOA-1 12/21/2023
> [   54.214852] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
> ...
> [   54.215090] Call trace:
> [   54.215097]  __i2c_transfer+0x874/0x968 [i2c_core] (P)
> [   54.215112]  __i2c_transfer+0x874/0x968 [i2c_core] (L)
> [   54.215126]  i2c_transfer+0x94/0xf0 [i2c_core]
> [   54.215140]  i2c_transfer_buffer_flags+0x5c/0x90 [i2c_core]
> [   54.215153]  regmap_i2c_write+0x20/0x58 [regmap_i2c]
> [   54.215166]  _regmap_raw_write_impl+0x740/0x894
> [   54.215184]  _regmap_bus_raw_write+0x60/0x7c
> [   54.215192]  _regmap_write+0x60/0x1b4
> [   54.215200]  regmap_write+0x4c/0x78
> [   54.215207]  ps883x_set+0xb0/0x10c [ps883x]
> [   54.215219]  ps883x_sw_set+0x74/0x98 [ps883x]
> [   54.215227]  typec_switch_set+0x58/0x90 [typec]
> [   54.215248]  pmic_glink_altmode_worker+0x3c/0x23c [pmic_glink_altmode]
> [   54.215257]  process_one_work+0x20c/0x610
> [   54.215274]  worker_thread+0x23c/0x378
> [   54.215283]  kthread+0x124/0x128
> [   54.215291]  ret_from_fork+0x10/0x20
> [   54.215303] irq event stamp: 28140
> [   54.215309] hardirqs last  enabled at (28139): [<ffffd15e3bc2a434>] __up_console_sem+0x6c/0x80
> [   54.215325] hardirqs last disabled at (28140): [<ffffd15e3c596aa4>] el1_dbg+0x24/0x8c
> [   54.215341] softirqs last  enabled at (28120): [<ffffd15e3bb9b82c>] handle_softirqs+0x4c4/0x4dc
> [   54.215355] softirqs last disabled at (27961): [<ffffd15e3bb501ec>] __do_softirq+0x14/0x20
> [   54.215363] ---[ end trace 0000000000000000 ]---
> [   54.216889] Enabling non-boot CPUs ...
> 
> This can be reproduced on the CRD (or T14s) by disconnecting, for
> example, a mass storage device while the laptop is suspended.

Sorry for the late reply. 

According to Bjorn's reply, this needs to be fixed in qcom-glink-smem
driver due to the IRQF_NO_SUSPEND flag for the glink-smem interrupt.

TBF, this whole series is going to be delayed by that fix being needed anyway. 

> 
> > +static int ps883x_retimer_set(struct typec_retimer *rtmr,
> > +			      struct typec_retimer_state *state)
> > +{
> > +	struct ps883x_retimer *retimer = typec_retimer_get_drvdata(rtmr);
> > +	struct typec_mux_state mux_state;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&retimer->lock);
> > +
> > +	if (state->mode != retimer->mode) {
> > +		retimer->mode = state->mode;
> > +
> > +		if (state->alt)
> > +			retimer->svid = state->alt->svid;
> > +		else
> > +			retimer->svid = 0; // No SVID
> 
> Nit: I'd prefer if you avoid c99 comments for consistency.

Yes, will drop.

> 
> > +		ret = ps883x_set(retimer);
> > +	}
> > +
> > +	mutex_unlock(&retimer->lock);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	mux_state.alt = state->alt;
> > +	mux_state.data = state->data;
> > +	mux_state.mode = state->mode;
> > +
> > +	return typec_mux_set(retimer->typec_mux, &mux_state);
> > +}
> 
> > +static int ps883x_retimer_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct typec_switch_desc sw_desc = { };
> > +	struct typec_retimer_desc rtmr_desc = { };
> > +	struct ps883x_retimer *retimer;
> > +	int ret;
> > +
> > +	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
> > +	if (!retimer)
> > +		return -ENOMEM;
> > +
> > +	retimer->client = client;
> > +
> > +	mutex_init(&retimer->lock);
> > +
> > +	retimer->regmap = devm_regmap_init_i2c(client, &ps883x_retimer_regmap);
> > +	if (IS_ERR(retimer->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->regmap),
> > +				     "failed to allocate register map\n");
> > +
> > +	ret = ps883x_get_vregs(retimer);
> > +	if (ret)
> > +		return ret;
> > +
> > +	retimer->xo_clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(retimer->xo_clk))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> > +				     "failed to get xo clock\n");
> > +
> > +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
> 
> GPIOD_ASIS is documented as requiring you to later set the direction,
> but this does not happen unconditionally below.

Yes. Will do that after the read that figures out if the retimer is
already left configured or not.

> 
> > +	if (IS_ERR(retimer->reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->reset_gpio),
> > +				     "failed to get reset gpio\n");
> > +
> > +	retimer->typec_switch = typec_switch_get(dev);
> > +	if (IS_ERR(retimer->typec_switch))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->typec_switch),
> > +				     "failed to acquire orientation-switch\n");
> > +
> > +	retimer->typec_mux = typec_mux_get(dev);
> > +	if (IS_ERR(retimer->typec_mux)) {
> > +		ret = dev_err_probe(dev, PTR_ERR(retimer->typec_mux),
> > +				    "failed to acquire mode-mux\n");
> > +		goto err_switch_put;
> > +	}
> > +
> > +	ret = drm_aux_bridge_register(dev);
> > +	if (ret)
> > +		goto err_mux_put;
> > +
> > +	ret = ps883x_enable_vregs(retimer);
> > +	if (ret)
> > +		goto err_mux_put;
> > +
> > +	ret = clk_prepare_enable(retimer->xo_clk);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable XO: %d\n", ret);
> > +		goto err_vregs_disable;
> > +	}
> > +
> > +	sw_desc.drvdata = retimer;
> > +	sw_desc.fwnode = dev_fwnode(dev);
> > +	sw_desc.set = ps883x_sw_set;
> > +
> > +	retimer->sw = typec_switch_register(dev, &sw_desc);
> > +	if (IS_ERR(retimer->sw)) {
> > +		ret = dev_err_probe(dev, PTR_ERR(retimer->sw),
> > +				    "failed to register typec switch\n");
> > +		goto err_clk_disable;
> > +	}
> > +
> > +	rtmr_desc.drvdata = retimer;
> > +	rtmr_desc.fwnode = dev_fwnode(dev);
> > +	rtmr_desc.set = ps883x_retimer_set;
> > +
> > +	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
> > +	if (IS_ERR(retimer->retimer)) {
> > +		ret = dev_err_probe(dev, PTR_ERR(retimer->sw),
> > +				    "failed to register typec retimer\n");
> > +		goto err_switch_unregister;
> > +	}
> 
> The registration functions do not return -EPROBE_DEFER so I'd prefer if
> you switch back to dev_err() here as we already discussed. A driver must
> not probe defer after having registered child devices so it's important
> to document which functions can actually trigger a probe deferral. 
> 
> I know there's been a recent change to the dev_err_probe() suggesting
> that it could be used anyway, but I think that's a really bad idea and
> I'm considering sending a revert for that.

Makes sense to me. Will switch back to dev_err().

> 
> > +
> > +	/* skip resetting if already configured */
> > +	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> > +			     CONN_STATUS_0_CONNECTION_PRESENT))
> > +		return 0;
> 
> What if the device is held in reset? This looks like it only works if
> the boot firmware has already enabled the retimer. Otherwise you may
> return success from probe here with the retimer still in reset.

Please correct me if I'm wrong, but if the read above fails or reads
anything else than "connection present", then below we go through the
resetting sequence. If it reads "connection present", then retimer can't
be in reset.

And here is where the direction setting mentioned above would have to
happen if the "connection is present" as well, not just for when the
repeater is in reset, which is handled below. 

> 
> > +	gpiod_direction_output(retimer->reset_gpio, 1);
> > +
> > +	/* VDD IO supply enable to reset release delay */
> > +	usleep_range(4000, 14000);
> > +
> > +	gpiod_set_value(retimer->reset_gpio, 0);
> > +
> > +	/* firmware initialization delay */
> > +	msleep(60);
> > +
> > +	return 0;
> > +
> > +err_switch_unregister:
> > +	typec_switch_unregister(retimer->sw);
> > +err_vregs_disable:
> > +	ps883x_disable_vregs(retimer);
> > +err_clk_disable:
> > +	clk_disable_unprepare(retimer->xo_clk);
> > +err_mux_put:
> > +	typec_mux_put(retimer->typec_mux);
> > +err_switch_put:
> > +	typec_switch_put(retimer->typec_switch);
> > +
> > +	return ret;
> > +}
> > +
> > +static void ps883x_retimer_remove(struct i2c_client *client)
> > +{
> > +	struct ps883x_retimer *retimer = i2c_get_clientdata(client);
> > +
> > +	typec_retimer_unregister(retimer->retimer);
> > +	typec_switch_unregister(retimer->sw);
> > +
> > +	gpiod_set_value(retimer->reset_gpio, 1);
> > +
> > +	clk_disable_unprepare(retimer->xo_clk);
> > +
> > +	ps883x_disable_vregs(retimer);
> > +
> > +	typec_mux_put(retimer->typec_mux);
> > +	typec_switch_put(retimer->typec_switch);
> > +}
> 
> Johan

Thanks for reviewing,
Abel

Powered by blists - more mailing lists