On 26/11/18 12:09, Pawel Laszczak wrote:
>>>>
>>>> Pawel,
>>>>
>>>> On 26/11/18 09:23, Pawel Laszczak wrote:
>>>>> Hi Roger,
>>>>>
>>>>>> On 18/11/18 12:09, Pawel Laszczak wrote:
>>>>>>> Patch adds supports for detecting Host/Device mode.
>>>>>>> Controller has additional OTG register that allow
>>>>>>> implement even whole OTG functionality.
>>>>>>> At this moment patch adds support only for detecting
>>>>>>> the appropriate mode based on strap pins and ID pin.
>>>>>>>
>>>>>>> Signed-off-by: Pawel Laszczak <paw...@cadence.com>
>>>>>>> ---
>>>>>>> drivers/usb/cdns3/Makefile | 2 +-
>>>>>>> drivers/usb/cdns3/core.c | 27 +++--
>>>>>>> drivers/usb/cdns3/drd.c | 229 +++++++++++++++++++++++++++++++++++++
>>>>>>> drivers/usb/cdns3/drd.h | 122 ++++++++++++++++++++
>>>>>>> 4 files changed, 372 insertions(+), 8 deletions(-)
>>>>>>> create mode 100644 drivers/usb/cdns3/drd.c
>>>>>>> create mode 100644 drivers/usb/cdns3/drd.h
>>>>>>>
>>>>>>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>>>>>>> index 02d25b23c5d3..e779b2a2f8eb 100644
>>>>>>> --- a/drivers/usb/cdns3/Makefile
>>>>>>> +++ b/drivers/usb/cdns3/Makefile
>>>>>>> @@ -1,5 +1,5 @@
>>>>>>> obj-$(CONFIG_USB_CDNS3) += cdns3.o
>>>>>>> obj-$(CONFIG_USB_CDNS3_PCI_WRAP) += cdns3-pci.o
>>>>>>>
>>>>>>> -cdns3-y := core.o
>>>>>>> +cdns3-y := core.o drd.o
>>>>>>> cdns3-pci-y := cdns3-pci-wrap.o
>>>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>>>> index f9055d4da67f..dbee4325da7f 100644
>>>>>>> --- a/drivers/usb/cdns3/core.c
>>>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>>>> @@ -17,6 +17,7 @@
>>>>>>>
>>>>>>> #include "gadget.h"
>>>>>>> #include "core.h"
>>>>>>> +#include "drd.h"
>>>>>>>
>>>>>>> static inline struct cdns3_role_driver
>>>>>>> *cdns3_get_current_role_driver(struct cdns3 *cdns)
>>>>>>> {
>>>>>>> @@ -57,8 +58,10 @@ static inline void cdns3_role_stop(struct cdns3
>>>>>>> *cdns)
>>>>>>> static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>>>>>>> {
>>>>>>> if (cdns->roles[CDNS3_ROLE_HOST] &&
>>>>>>> cdns->roles[CDNS3_ROLE_GADGET]) {
>>>>>>> - //TODO: implements selecting device/host mode
>>>>>>> - return CDNS3_ROLE_HOST;
>>>>>>> + if (cdns3_is_host(cdns))
>>>>>>> + return CDNS3_ROLE_HOST;
>>>>>>> + if (cdns3_is_device(cdns))
>>>>>>> + return CDNS3_ROLE_GADGET;
>>>>>>> }
>>>>>>> return cdns->roles[CDNS3_ROLE_HOST]
>>>>>>> ? CDNS3_ROLE_HOST
>>>>>>> @@ -124,6 +127,12 @@ static irqreturn_t cdns3_irq(int irq, void *data)
>>>>>>> struct cdns3 *cdns = data;
>>>>>>> irqreturn_t ret = IRQ_NONE;
>>>>>>>
>>>>>>> + if (cdns->dr_mode == USB_DR_MODE_OTG) {
>>>>>>> + ret = cdns3_drd_irq(cdns);
>>>>>>> + if (ret == IRQ_HANDLED)
>>>>>>> + return ret;
>>>>>>> + }
>>>>>>
>>>>>> The kernel's shared IRQ model takes care of sharing the same interrupt
>>>>>> between different devices and their drivers. You don't need to manually
>>>>>> handle it here. Just let all 3 drivers do a request_irq() and have
>>>>>> handlers check if the IRQ was theirs or not and return IRQ_HANDLED or
>>>>>> IRQ_NONE accordingly.
>>>>>>
>>>>>> Looks like you can do away with irq member of the role driver struct.
>>>>>
>>>>> Ok, I will split it into 3 separate part, but in this case, I
>>>>> additionally have to check the current
>>>>> role in ISR function. Driver can't read host side registers when
>>>>> controller works in device role
>>>>> and vice versa. One part of controller is kept in reset. Only DRD
>>>>> registers are common and are all accessible.
>>>>>
>>>>
>>>> In which ISR do you need to check current role?
>>>>
>>>> I'm not sure if we are on the same page.
>>>> Core (drd) driver shouldn't read host/device side registers. All 3 drivers,
>>>> i.e. DRD(core), Host (xhci) and device (cdns3) should do a request_irq()
>>>> and process their respective IRQ events.
>>>
>>> Yes, I understand.
>>> I need to check this in cdns3_irq_handler_thread and cdns3_host_irq.
>>>
>>> Core (drd) has register that are always accessible.
>>> Core (device) - registers are available also in device mode
>>> Core (host) - registers are available only in host mode
>>>
>>> So we can use separate request_irq for drd, device and host side, but
>>> we need ensure that in host side driver will not touch the device register.
>>
>> That should never happen as host side doesn't have visibility to device
>> registers.
>
> I meant the following scenario:
> Assume that controller work in Device role and it raise interrupt for Device
> part.
> Now host is kept in reset.
> 1. System receive interrupt
> 2. Kernel call drd_irq but this function return IRQ_NONE
> 3. Kernel call cdns3_host_irq and driver has to read some interrupt register
> to check if
> this event is addressed to him. In this moment we can have unknown
> behavior.
This is the problem. If device is not able to figure out its interrupt event
then
it must unregister the ISR.
> 4. Kernel call cdns3_irq_handler (device). This point probably will not
> happen because
> It could have stuck at point 3.
>
> Ok, I think I understood. After changing role, the driver unregisters the
> interrupt function
> used by previous role so point 3 never happen.
>
> I will try to check how it works.
>
Cool! Thanks.
>>>
>>> We doesn't know the order in which the system will call interrupts
>>> function related to
>>> the shared interrupt line.
>>
>> Order shouldn't matter. Each user will check its own events return IRQ_NONE
>> if they
>> didn't cause the IRQ.
>>>
>>
>> <snip>
>>
cheers,
-roger
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
