Re: [PATCH linux dev-5.4 1/2] usb: gadget: aspeed: add ast2600 vhub support

[Tao Ren <rentao.bupt@gmail.com>]

On Fri, Jan 17, 2020 at 10:13:57AM +1030, Andrew Jeffery wrote:
> 
> 
> On Fri, 17 Jan 2020, at 09:55, rentao.bupt@gmail.com wrote:
> > From: Tao Ren <rentao.bupt@gmail.com>
> > 
> > Add AST2600 support in aspeed-vhub driver.
> > 
> > There are 3 major differences between AST2500 and AST2600 vhub:
> >   - AST2600 supports 7 downstream devices while AST2500 supports 5.
> >   - AST2600 supports 21 programmable endpoints while AST2500 supports 15.
> >   - EP0 data buffer's 8-byte DMA alignment restriction is removed from
> >     AST2600.
> > 
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > ---
> >  drivers/usb/gadget/udc/aspeed-vhub/Kconfig |  4 +--
> >  drivers/usb/gadget/udc/aspeed-vhub/core.c  | 25 ++++++-----------
> >  drivers/usb/gadget/udc/aspeed-vhub/vhub.h  | 32 ++++++++++++++++------
> >  3 files changed, 35 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/Kconfig 
> > b/drivers/usb/gadget/udc/aspeed-vhub/Kconfig
> > index 83ba8a2eb6af..605500b19cf3 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/Kconfig
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/Kconfig
> > @@ -4,5 +4,5 @@ config USB_ASPEED_VHUB
> >  	depends on ARCH_ASPEED || COMPILE_TEST
> >  	depends on USB_LIBCOMPOSITE
> >  	help
> > -	  USB peripheral controller for the Aspeed AST2500 family
> > -	  SoCs supporting the "vHub" functionality and USB2.0
> > +	  USB peripheral controller for the Aspeed AST2400, AST2500 and
> > +	  AST2600 family SoCs supporting the "vHub" functionality and USB2.0
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c 
> > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > index 90b134d5dca9..5fafe91d3619 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > @@ -97,6 +97,7 @@ void ast_vhub_free_request(struct usb_ep *u_ep, 
> > struct usb_request *u_req)
> >  
> >  static irqreturn_t ast_vhub_irq(int irq, void *data)
> >  {
> > +	u32 i;
> >  	struct ast_vhub *vhub = data;
> >  	irqreturn_t iret = IRQ_NONE;
> >  	u32 istat;
> > @@ -121,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> >  
> >  	/* Handle generic EPs first */
> >  	if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> > -		u32 i, ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
> > +		u32 ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
> >  		writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
> >  
> >  		for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs; i++) {
> > @@ -134,21 +135,10 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> >  	}
> >  
> >  	/* Handle device interrupts */
> > -	if (istat & (VHUB_IRQ_DEVICE1 |
> > -		     VHUB_IRQ_DEVICE2 |
> > -		     VHUB_IRQ_DEVICE3 |
> > -		     VHUB_IRQ_DEVICE4 |
> > -		     VHUB_IRQ_DEVICE5)) {
> > -		if (istat & VHUB_IRQ_DEVICE1)
> > -			ast_vhub_dev_irq(&vhub->ports[0].dev);
> > -		if (istat & VHUB_IRQ_DEVICE2)
> > -			ast_vhub_dev_irq(&vhub->ports[1].dev);
> > -		if (istat & VHUB_IRQ_DEVICE3)
> > -			ast_vhub_dev_irq(&vhub->ports[2].dev);
> > -		if (istat & VHUB_IRQ_DEVICE4)
> > -			ast_vhub_dev_irq(&vhub->ports[3].dev);
> > -		if (istat & VHUB_IRQ_DEVICE5)
> > -			ast_vhub_dev_irq(&vhub->ports[4].dev);
> > +	for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > +		u32 dev_irq = VHUB_IRQ_DEVICE1 << i;
> > +		if (istat & dev_irq)
> > +			ast_vhub_dev_irq(&vhub->ports[i].dev);
> >  	}
> >  
> >  	/* Handle top-level vHub EP0 interrupts */
> > @@ -407,6 +397,9 @@ static const struct of_device_id ast_vhub_dt_ids[] 
> > = {
> >  	{
> >  		.compatible = "aspeed,ast2500-usb-vhub",
> >  	},
> > +	{
> > +		.compatible = "aspeed,ast2600-usb-vhub",
> > +	},
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h 
> > b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > index 761919e220d3..76935d02decf 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > @@ -2,6 +2,23 @@
> >  #ifndef __ASPEED_VHUB_H
> >  #define __ASPEED_VHUB_H
> >  
> > +/*****************************
> > + *                           *
> > + * Maximum devices/endpoints *
> > + *                           *
> > + *****************************/
> > +
> > +#ifdef CONFIG_MACH_ASPEED_G6
> 
> No, this prevents building a kernel supporting multiple AST generations. Please
> describe the differences in a platform data struct attached to struct of_device_id.

Got it. I took "#ifdef" approach just because it involves little
changes. Let me move to of_device_id direction then.

> Also, what's the plan for upstreaming these changes? It's okay to send them for
> inclusion in the openbmc tree if you're wanting them to bake there for some
> more widespread testing, but it's not clear what the intent is.
> 
> Cheers,
> 
> Andrew

My major goal was to get more testing and early feedback. Given we've
decided to go with of_device_id approach, let's ignore the patchs then.


Cheers,

Tao