On 18/05/16 16:12, Jun Li wrote:
> Hi
>
>> -----Original Message-----
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Wednesday, May 18, 2016 8:43 PM
>> To: Jun Li <jun...@nxp.com>; Peter Chen <hzpeterc...@gmail.com>
>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-usb@vger.kernel.org;
>> linux-o...@vger.kernel.org; linux-ker...@vger.kernel.org;
>> devicet...@vger.kernel.org
>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>
>> On 17/05/16 11:28, Jun Li wrote:
>>> Hi Roger,
>>>
>>>> -----Original Message-----
>>>> From: Roger Quadros [mailto:rog...@ti.com]
>>>> Sent: Tuesday, May 17, 2016 4:09 PM
>>>> To: Jun Li <jun...@nxp.com>; Peter Chen <hzpeterc...@gmail.com>
>>>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>>>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
>>>> linux-usb@vger.kernel.org; linux-o...@vger.kernel.org;
>>>> linux-ker...@vger.kernel.org; devicet...@vger.kernel.org
>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>
>>>> On 17/05/16 10:38, Jun Li wrote:
>>>>> Hi
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Roger Quadros [mailto:rog...@ti.com]
>>>>>> Sent: Monday, May 16, 2016 5:52 PM
>>>>>> To: Peter Chen <hzpeterc...@gmail.com>
>>>>>> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
>>>>>> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
>>>>>> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
>>>>>> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
>>>>>> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
>>>>>> r...@kernel.org; nsek...@ti.com; b-...@ti.com;
>>>>>> linux-usb@vger.kernel.org; linux-o...@vger.kernel.org;
>>>>>> linux-ker...@vger.kernel.org; devicet...@vger.kernel.org
>>>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>>>
>>>>>> On 16/05/16 12:23, Peter Chen wrote:
>>>>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 16/05/16 10:02, Peter Chen wrote:
>>>>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>>>>>>>>>> +
>>>>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget
>>>>>>>>>> +*gadget, bool connect) {
>>>>>>>>>> + struct usb_udc *udc;
>>>>>>>>>> +
>>>>>>>>>> + mutex_lock(&udc_lock);
>>>>>>>>>> + udc = usb_gadget_to_udc(gadget);
>>>>>>>>>> + if (!udc) {
>>>>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not
>>>>>> registered.\n",
>>>>>>>>>> + __func__);
>>>>>>>>>> + mutex_unlock(&udc_lock);
>>>>>>>>>> + return -EINVAL;
>>>>>>>>>> + }
>>>>>>>>>> +
>>>>>>>>>> + if (connect) {
>>>>>>>>>> + if (!gadget->connected)
>>>>>>>>>> + usb_gadget_connect(udc->gadget);
>>>>>>>>>> + } else {
>>>>>>>>>> + if (gadget->connected) {
>>>>>>>>>> + usb_gadget_disconnect(udc->gadget);
>>>>>>>>>> + udc->driver->disconnect(udc->gadget);
>>>>>>>>>> + }
>>>>>>>>>> + }
>>>>>>>>>> +
>>>>>>>>>> + mutex_unlock(&udc_lock);
>>>>>>>>>> +
>>>>>>>>>> + return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> Since this is called for vbus interrupt, why not using
>>>>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect
>>>>>>>>> at usb_gadget_stop.
>>>>>>>>
>>>>>>>> We can't assume that this is always called for vbus interrupt so
>>>>>>>> I decided not to call usb_udc_vbus_handler.
>>>>>>>>
>>>>>>>> udc->vbus is really pointless for us. We keep vbus states in our
>>>>>>>> state machine and leave udc->vbus as ture always.
>>>>>>>>
>>>>>>>> Why do you want to move udc->driver->disconnect() to stop?
>>>>>>>> If USB controller disconnected from bus then the gadget driver
>>>>>>>> must be notified about the disconnect immediately. The controller
>>>>>>>> may or may not be stopped by the core.
>>>>>>>>
>>>>>>>
>>>>>>> Then, would you give some comments when this API will be used?
>>>>>>> I was assumed it is only used for drd state machine.
>>>>>>
>>>>>> drd_state machine didn't even need this API in the first place :).
>>>>>> You guys wanted me to separate out start/stop and
>>>>>> connect/disconnect for full OTG case.
>>>>>> Won't full OTG state machine want to use this API? If not what
>>>>>> would it use?
>>>>>
>>>>> Instead create those new interfaces/symbol here and there just aim
>>>>> to address build problems in diff configures, Could we only allow
>>>>> meaningful combination of those 3 drivers configures?
>>>>>
>>>>> Hcd=y, gadget=y, otg=y or
>>>>> Hcd=m, gadget=m, otg=m
>>>>
>>>> This is still a limitation.
>>>>
>>>> It is perfectly fine to have
>>>> hcd=m, gadget=y
>>>> or
>>>> hcd=y, gadget=m
>>>
>>> I agree it makes sense to have above configs in non-otg case, that is,
>>> the 'y' driver can work without 'm' driver loaded.
>>>
>>> But,
>>> in otg enabled(y/m) case, the otherwise config of my list can't make
>>> any sense from my point view. That is: some driver is built-in, but it
>>> can't work at all if another 'm' driver is not loaded,
>>>
>>> in another words, the otg driver has to be 'm' if its dependent driver
>>> is 'm', correct?
>>
>> If both host and gadget are 'm' then otg can be 'm', but if either host or
>> gadget is built in then we have no choice but to make otg as built-in.
>>
>> I didn't want to have complex Kconfig so decided to have otg as built-in
>> only.
>> What do you want me to change in existing code? and why?
>
> Remove those stuff which only for pass diff driver config
> Like every controller driver need a duplicated
>
> static struct otg_hcd_ops ci_hcd_ops = {
> ...
> }
This is an exception only. Every controller driver doesn't need to implement
hcd_ops. It is implemented in the hcd core.
>
> And here is another example, for gadget connect, otg driver can
> directly call to usb_udc_vbus_handler() in drd state machine,
> but you create another interface:
>
> .connect_control = usb_gadget_connect_control,
>
> If the symbol is defined in one driver which is 'm', another driver
> reference it should be 'm' as well, then there is no this kind of problem
> as my understanding.
That is fine as long as all are 'm'. but how do you solve the case
when Gadget is built in and host is 'm'? OTG has to be built-in and
you will need an hcd to gadget interface.
Do you have any ideas to solve that case?
cheers,
-roger
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>> return 0;
>>>>>>>>>> @@ -660,9 +830,15 @@ static ssize_t
>>>>>>>>>> usb_udc_softconn_store(struct
>>>>>> device *dev,
>>>>>>>>>> return -EOPNOTSUPP;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> + /* In OTG mode we don't support softconnect, but b_bus_req */
>>>>>>>>>> + if (udc->gadget->otg_dev) {
>>>>>>>>>> + dev_err(dev, "soft-connect not supported in OTG
>>>>>>>>>> mode\n");
>>>>>>>>>> + return -EOPNOTSUPP;
>>>>>>>>>> + }
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> The soft-connect can be supported at dual-role mode currently,
>>>>>>>>> we can use b_bus_req entry once it is implemented later.
>>>>>>>>
>>>>>>>> Soft-connect should be done via sysfs handling within the OTG core.
>>>>>>>> This can be added later. I don't want anything outside the OTG
>>>>>>>> core to handle soft-connect behaviour as it will be hard to keep
>>>>>>>> things in sync.
>>>>>>>>
>>>>>>>> I can update the comment to something like this.
>>>>>>>>
>>>>>>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG
>>>>>>>> core */
>>>>>>>
>>>>>>> Ok, let's Felipe decide it.
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> if (sysfs_streq(buf, "connect")) {
>>>>>>>>>> usb_gadget_udc_start(udc);
>>>>>>>>>> - usb_gadget_connect(udc->gadget);
>>>>>>>>>> + usb_udc_connect_control(udc);
>>>>>>>>>
>>>>>>>>> This line seems to be not related with this patch.
>>>>>>>>>
>>>>>>>> Right. I'll remove it.
>>>>>>>>
>>>>>>>> cheers,
>>>>>>>> -roger
>>>>>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
