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

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Pavel,

On 9/8/26 13:52, Pavel Machek wrote:
> Hi!
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> 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);
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy