Re: [PATCH v2 03/22] usb: dwc3: glue: Allow more fine grained control over mode switches

[Sven Peter <sven@kernel.org>]

Hi,

On 19.09.25 23:40, Thinh Nguyen wrote:
> On Sat, Sep 06, 2025, Sven Peter wrote:
>> We need fine grained control over mode switched on the DWC3 controller
>> present on Apple Silicon. Export core, host and gadget init and exit,
>> ptrcap and susphy control functions. Also introduce an additional
>> parameter to probe_data that allows to skip the final initialization
>> step that would bring up host or gadget mode.
>>
>> Signed-off-by: Sven Peter <sven@kernel.org>
>> ---
>>   drivers/usb/dwc3/core.c   | 16 +++++++++++-----
>>   drivers/usb/dwc3/gadget.c |  2 ++
>>   drivers/usb/dwc3/glue.h   | 14 ++++++++++++++
>>   drivers/usb/dwc3/host.c   |  2 ++
>>   4 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 8002c23a5a02acb8f3e87b2662a53998a4cf4f5c..18056fac44c8732278a650ac2be8b493892c92dd 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -132,6 +132,7 @@ void dwc3_enable_susphy(struct dwc3 *dwc, bool enable)
>>   		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
>>   	}
>>   }
>> +EXPORT_SYMBOL_GPL(dwc3_enable_susphy);
>>   
>>   void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode, bool ignore_susphy)
>>   {
>> @@ -157,6 +158,7 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode, bool ignore_susphy)
>>   
>>   	dwc->current_dr_role = mode;
>>   }
>> +EXPORT_SYMBOL_GPL(dwc3_set_prtcap);
> 
> I'm hesitant to export this as is. This function may change the susphy
> bits and expect them to be restored later. It's not meant to be a
> standalone use. At least, we should document how it should be used along
> with the other newly added interfaces.


Sure, I can otherwise also open-code the susphy change inside 
dwc3_apple_phy_set_mode anyway if you prefer to keep this private to the 
dwc3 core. I should restore it there to the original value anyway I 
guess after phy_set_mode.



>>   
>>   static void __dwc3_set_mode(struct work_struct *work)

[...]

>>   int dwc3_gadget_suspend(struct dwc3 *dwc)
>>   {
>> diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h
>> index 2efd00e763be4fc51911f32d43054059e61fb43a..633268c76fe4c7fdc312c9705dfa7cf7ccf3544c 100644
>> --- a/drivers/usb/dwc3/glue.h
>> +++ b/drivers/usb/dwc3/glue.h
>> @@ -15,16 +15,30 @@
>>    * @res: resource for the DWC3 core mmio region
>>    * @ignore_clocks_and_resets: clocks and resets defined for the device should
>>    *		be ignored by the DWC3 core, as they are managed by the glue
>> + * @skip_core_init_mode: skip the finial initialization of the target mode, as
> 
> finial -> final?

Whoops, yes, I thought I ran a spell checker over this because I usually 
add a lot of typos but must've forgotten :-)


> 
>> + *		it must be managed by the glue
>>    */

[...]

>>   
>> +int dwc3_core_init(struct dwc3 *dwc);
>> +void dwc3_core_exit(struct dwc3 *dwc);
>> +
>> +int dwc3_host_init(struct dwc3 *dwc);
>> +void dwc3_host_exit(struct dwc3 *dwc);
>> +int dwc3_gadget_init(struct dwc3 *dwc);
>> +void dwc3_gadget_exit(struct dwc3 *dwc);
>> +
>> +void dwc3_enable_susphy(struct dwc3 *dwc, bool enable);
>> +void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode, bool ignore_susphy);
>> +
> 
> We should document these interfaces. The dwc3_core_probe() does all of
> the above in the proper order. It's not obvious why these are needed and
> how they should be used.

Very good point, I'll add documentation for all of these!


Thanks for the review,


Sven