Re: [PATCH v2 2/2] usb: musb-new: sunxi: make compatible with UDC/DM gadget model

[Sam Edwards <cfsworks@gmail.com>]

On 6/27/24 09:06, Andre Przywara wrote:
> On Thu,  8 Jun 2023 13:56:31 -0600
> Sam Edwards <cfsworks@gmail.com> wrote:
> 
> Hi,
> 
> John asked me have a look at this.

Hi Andre, it's good to hear from you again,

I'd first like to make sure you're aware that the date on this patch is 
June *2023,* not June 2024. It's possible things have changed 
substantially in the past year. I do not know if this patch is still a 
necessity; though if John is nudging about it, it probably is.

> 
>> Since many sunxi boards do not implement a `board_usb_init`, it's
> 
> I am confused, what has this have to do with gadget support? *No* sunxi
> board build provides board_usb_init(), but apparently this works fine for
> now.
> I am all for this converting to DM part, but the rationale seems a bit
> off.

For context, board_usb_init() is (was?) the non-DM entry point for USB 
functionality; it is (was?) *the* implementation of 
usb_gadget_initialize() when !DM_USB_GADGET.

> 
> Also can you give some reason for this patch? What does this fix or
> improve? "it's better" is a bit thin, "complying with DM" would already be
> sufficient, but maybe there is more?

Eh, yeah, "better" is something of a question-begging word isn't it? :)

The main point is to be compatible with DM's view of UDC, which as you 
said is a worthy goal in itself. It's "better" because this allows using 
DM's all-purpose implementation of usb_gadget_initialize(), which is 
(was?) necessary for those targets lacking board_usb_init().

> 
>> better if we just make the sunxi USB driver compatible with the
>> DM gadget model, as many other musb-new variants already are.
>>
>> This change has been verified working on a T113s.
>>
>> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
>> ---
>>   drivers/usb/musb-new/sunxi.c | 50 +++++++++++++++++++++++-------------
>>   1 file changed, 32 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
>> index 510b254f7d..6658cd995d 100644
>> --- a/drivers/usb/musb-new/sunxi.c
>> +++ b/drivers/usb/musb-new/sunxi.c
>> @@ -444,6 +444,16 @@ static struct musb_hdrc_config musb_config_h3 = {
>>   	.ram_bits	= SUNXI_MUSB_RAM_BITS,
>>   };
>>   
>> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> 
> Please no more #ifdef's. Is there any reason to *not* force
> DM_USB_GADGET now, for all sunxi boards, in Kconfig?
> Either by "select"ing it in the USB Kconfig, or in arch/arm/Kconfig, like
> other platforms do.
> Then you don't need to care about the !DM_USB_GADGET definition of this
> function and can drop the #ifdef.

I wouldn't be the one to ask. I can't think of any such reason myself. 
But to me it sounds like since *no sunxi board provides 
board_usb_init()* the only way USB gadgets *could* work is with 
DM_USB_GADGET? That'd be reason enough to force it.

> 
>> +int dm_usb_gadget_handle_interrupts(struct udevice *dev) {
> 
> coding style

Sentence fragments are harder to understand. I am assuming you are 
saying, "Please put the opening '{' on its own line."

> 
>> +	struct sunxi_glue *glue = dev_get_priv(dev);
>> +	struct musb_host_data *host = &glue->mdata;
>> +
>> +	host->host->isr(0, host->host);
>> +	return 0;
>> +}
>> +#endif
>> +
>>   static int musb_usb_probe(struct udevice *dev)
>>   {
>>   	struct sunxi_glue *glue = dev_get_priv(dev);
>> @@ -452,10 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>>   	void *base = dev_read_addr_ptr(dev);
>>   	int ret;
>>   
>> -#ifdef CONFIG_USB_MUSB_HOST
>> -	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>> -#endif
>> -
>>   	if (!base)
>>   		return -EINVAL;
>>   
>> @@ -486,23 +492,31 @@ static int musb_usb_probe(struct udevice *dev)
>>   	pdata.platform_ops = &sunxi_musb_ops;
>>   	pdata.config = glue->cfg->config;
>>   
>> -#ifdef CONFIG_USB_MUSB_HOST
>> -	priv->desc_before_addr = true;
>> +	if (IS_ENABLED(CONFIG_USB_MUSB_HOST)) {
>> +		struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>> +		priv->desc_before_addr = true;
>>   
>> -	pdata.mode = MUSB_HOST;
>> -	host->host = musb_init_controller(&pdata, &glue->dev, base);
>> -	if (!host->host)
>> -		return -EIO;
>> +		pdata.mode = MUSB_HOST;
>> +		host->host = musb_init_controller(&pdata, &glue->dev, base);
>> +		if (!host->host)
>> +			return -EIO;
>>   
>> -	return musb_lowlevel_init(host);
>> -#else
>> -	pdata.mode = MUSB_PERIPHERAL;
>> -	host->host = musb_register(&pdata, &glue->dev, base);
>> -	if (IS_ERR_OR_NULL(host->host))
>> -		return -EIO;
>> +		return musb_lowlevel_init(host);
>> +	} else if (CONFIG_IS_ENABLED(DM_USB_GADGET)) {
>> +		pdata.mode = MUSB_PERIPHERAL;
>> +		host->host = musb_init_controller(&pdata, &glue->dev, base);
>> +		if (!host->host)
>> +			return -EIO;
>>   
>> -	return 0;
>> -#endif
>> +		return usb_add_gadget_udc(&glue->dev, &host->host->g);
>> +	} else {
>> +		pdata.mode = MUSB_PERIPHERAL;
>> +		host->host = musb_register(&pdata, &glue->dev, base);
>> +		if (IS_ERR_OR_NULL(host->host))
>> +			return -EIO;
>> +
>> +		return 0;
>> +	}
> 
> That looks like a good cleanup! Just need to test it briefly, but it seems
> like the gist of this patch is fine.

I think it would be wise to test it a little better than "briefly" given 
the age of the patch. I'm not well-equipped to do any testing myself 
right now or I'd volunteer.

> 
> Cheers,
> Andre

Likewise,
Sam

> 
> 
>>   }
>>   
>>   static int musb_usb_remove(struct udevice *dev)
>