Re: [PATCH v4 07/13] usb: otg: add OTG core

[Peter Chen]

On Wed, Sep 09, 2015 at 01:21:50PM +0300, Roger Quadros wrote:
> On 09/09/15 11:45, Peter Chen wrote:
> > On Wed, Sep 09, 2015 at 12:33:20PM +0300, Roger Quadros wrote:
> >> On 09/09/15 11:13, Peter Chen wrote:
> >>> On Wed, Sep 09, 2015 at 12:08:10PM +0300, Roger Quadros wrote:
> >>>> On 09/09/15 05:21, Peter Chen wrote:
> >>>>> On Tue, Sep 08, 2015 at 03:25:25PM +0300, Roger Quadros wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 08/09/15 11:31, Peter Chen wrote:
> >>>>>>> On Mon, Sep 07, 2015 at 01:23:01PM +0300, Roger Quadros wrote:
> >>>>>>>> On 07/09/15 04:23, Peter Chen wrote:
> >>>>>>>>> On Mon, Aug 24, 2015 at 04:21:18PM +0300, Roger Quadros wrote:
> >>>>>>>>>> + * This is used by the USB Host stack to register the Host controller
> >>>>>>>>>> + * to the OTG core. Host controller must not be started by the
> >>>>>>>>>> + * caller as it is left upto the OTG state machine to do so.
> >>>>>>>>>> + *
> >>>>>>>>>> + * Returns: 0 on success, error value otherwise.
> >>>>>>>>>> + */
> >>>>>>>>>> +int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
> >>>>>>>>>> +			 unsigned long irqflags, struct otg_hcd_ops *ops)
> >>>>>>>>>> +{
> >>>>>>>>>> +	struct usb_otg *otgd;
> >>>>>>>>>> +	struct device *hcd_dev = hcd->self.controller;
> >>>>>>>>>> +	struct device *otg_dev = usb_otg_get_device(hcd_dev);
> >>>>>>>>>> +
> >>>>>>>>>
> >>>>>>>>> One big problem here is: there are two designs for current (IP) driver
> >>>>>>>>> code, one creates dedicated hcd device as roothub's parent, like dwc3.
> >>>>>>>>> Another one doesn't do this, roothub's parent is core device (or otg device
> >>>>>>>>> in your patch), like chipidea and dwc2.
> >>>>>>>>>
> >>>>>>>>> Then, otg_dev will be glue layer device for chipidea after that.
> >>>>>>>>
> >>>>>>>> OK. Let's add a way for the otg controller driver to provide the host and gadget
> >>>>>>>> information to the otg core for such devices like chipidea and dwc2.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Roger, not only chipidea and dwc2, I think the musb uses the same
> >>>>>>> hierarchy. If the host, device, and otg share the same register
> >>>>>>> region, host part can't be a platform driver since we don't want
> >>>>>>> to remap the same register region again.
> >>>>>>>
> >>>>>>> So, in the design, we may need to consider both situations, one
> >>>>>>> is otg/host/device has its own register region, and host is a
> >>>>>>> separate platform device (A), the other is three parts share the
> >>>>>>> same register region, there is only one platform driver (B).
> >>>>>>>
> >>>>>>> A:
> >>>>>>>
> >>>>>>> 			IP core device 
> >>>>>>> 			    |
> >>>>>>> 			    |
> >>>>>>> 		      |-----|-----|
> >>>>>>> 		      gadget   host platform device	
> >>>>>>> 		      		|
> >>>>>>> 				roothub
> >>>>>>>
> >>>>>>> B:
> >>>>>>>
> >>>>>>> 			IP core device
> >>>>>>> 			    |
> >>>>>>> 			    |
> >>>>>>> 		      |-----|-----|
> >>>>>>> 		      gadget   	 roothub
> >>>>>>> 		      		
> >>>>>>>
> >>>>>>>> This API must be called before the hcd/gadget-driver is added so that the otg
> >>>>>>>> core knows it's linked to an OTG controller.
> >>>>>>>>
> >>>>>>>> Any better idea?
> >>>>>>>>
> >>>>>>>
> >>>>>>> A flag stands for this hcd controller is the same with otg controller
> >>>>>>> can be used, this flag can be stored at struct usb_otg_config.
> >>>>>>
> >>>>>> What if there is another architecture like so?
> >>>>>>
> >>>>>> C:
> >>>>>> 			[Parent]
> >>>>>> 			   |
> >>>>>> 			   |
> >>>>>> 		|------------------|--------------|
> >>>>>> 	[OTG core]		[gadget]	[host]
> >>>>>>
> >>>>>> We need a more flexible mechanism to link the gadget and
> >>>>>> host device to the otg core for non DT case.
> >>>>>>
> >>>>>> How about adding struct usb_otg parameter to usb_otg_register_hcd()?
> >>>>>>
> >>>>>> e.g.
> >>>>>> int usb_otg_register_hcd(struct usb_otg *otg, struct usb_hcd *hcd, ..)
> >>>>>>
> >>>>>> If otg is NULL it will try DT otg-controller property or parent to
> >>>>>> get the otg controller.
> >>>>>
> >>>>> How usb_otg_register_hcd get struct usb_otg, from where?
> >>>>
> >>>> This only works when the parent driver creating the hcd has registered the
> >>>> otg controller too.
> >>>>
> >>>
> >>> Sorry? So we need to find another way to solve this issue, right?
> >>
> >> For existing cases this is sufficient.
> >> The otg device is either the one supplied during usb_otg_register_hcd
> >> (cases B and C) or it is the parent device (case A).
> > 
> > How we differentiate case A and case B at usb_otg_register_hcd?
> > Would you show me the sample code?
> 
> Case A:
> 
> hcd platform driver doesn't know about otg device so it calls
> 
> 	usb_add_hcd(hcd,..)->usb_otg_register_hcd(NULL, hcd,..);
> 
> Case B:
> 
> core driver knows about both otg and hcd so it calls
> 	usb_otg_register_hcd(otg, hcd,...);
> 

Ok, Get your points, you mean invoke usb_otg_register_hcd at platform
driver directly instead of at hcd.c. It may be not a good solution
due to we use different otg APIs for two cases, it may confuse the
users, unless we can have some APIs (flags) are easy to read and well
documentation.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/