Re: [PATCH v5] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

[claudiu beznea]

Hi, Pavel,

On 9/8/26 13:52, Pavel Machek wrote:
> Hi!
>
> > From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >
> > To address this, release the spin lock before sleeping for 20 ms as
> > required by the HW manual and reacquire it afterwards. To avoid other
> > threads entering the critical section and configuring the HW while the
> > software is waiting for the OTG initialization to complete, introduce the
> > otg_initializing variable alongside the otg_init_done wait
> > queue. Any
>
> This is quite complex.

I agree. I tried to keep the current driver capabilities and adjust it for the 
long delay.

> How is this solved in mainline?

What do you mean by "How is this solved in mainline?" ? This patch is intended 
for mainline.

> > To avoid failures when multiple PHYs call struct
> > phy_ops::rcar_gen3_phy_usb2_init() simultaneously, and the PHY responsible
> > for initializing the OTG either fails or deinit quiqly and another PHY
> > takes over the PHY init role), the code waiting for the
> > channel->otg_init_done wait queue retries up to NUM_OF_PHYS times.
>
> And more complexity.
>
> Example of the code is quoted below, and we are returning EBUSY to
> userspace if it tries to change role at the wrong time. Not great.
>
> As far as I understand, the initialization on needs to be done
> once.

Yes.

> Instead of exposing /sys interfaces before hardware is ready,
> and then doing complex dance when /sys is accessed, could we
> initialize hardware in rcar_gen3_phy_usb2_probe or something?

It may be achievable, I haven't tried, but that would involve, at least, 
enabling PHY related stuff that consumes power at times this may not be needed.

> Looking at the code:
>
>          /* If current and new mode is the same, this returns the error */
>          if (cur_mode == new_mode)
>                  return -EINVAL;
>
> this should probably just return success? (EINVAL is certainly wrong
> error code here.)

Things could be improved, indeed. This is however code that was present in this 
driver before this patch.

Thank you,
Claudiu

> Best regards,
> 								Pavel
>
> > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > @@ -392,26 +408,58 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr,
> >   	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
> >   	bool is_b_device;
> >   	enum phy_mode cur_mode, new_mode;
> > +	int retries = NUM_OF_PHYS;
> > +	unsigned long flags;
> > +	int ret = -EIO;
> >   
> > -	guard(spinlock_irqsave)(&ch->lock);
> > +	spin_lock_irqsave(&ch->lock, flags);
> >   
> > -	if (!ch->is_otg_channel || !rcar_gen3_is_any_otg_rphy_initialized(ch))
> > -		return -EIO;
> > +	if (!ch->is_otg_channel)
> > +		goto unlock;
> > +
> > +	while (retries-- && ch->otg_initializing) {
> > +		spin_unlock_irqrestore(&ch->lock, flags);
> > +
> > +		ret = wait_event_timeout(ch->otg_init_done, !ch->otg_initializing,
> > +					 USB2_OTG_INIT_TIMEOUT);
> > +		ret = ret ? 0 : -ETIMEDOUT;
> > +		if (ret && !retries)
> > +			goto exit;
> > +
> > +		spin_lock_irqsave(&ch->lock, flags);
> > +	}
> > +
> > +	/* If another thread started a new initialization just return -EBUSY. */
> > +	if (ch->otg_initializing) {
> > +		ret = -EBUSY;
> > +		goto unlock;
> ...
> > @@ -1007,6 +1226,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
> >   		return ret;
> >   
> >   	spin_lock_init(&channel->lock);
> > +	init_waitqueue_head(&channel->otg_init_done);
> >   	for (i = 0; i < NUM_OF_PHYS; i++) {
> >   		channel->rphys[i].phy = devm_phy_create(dev, NULL,
> >   							channel->phy_data->phy_usb2_ops);