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 <pawell@xxxxxxxxxxx>
>>> ---
>>> 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.
>>> +
>>> /* Handle device/host interrupt */
>>> if (cdns->role != CDNS3_ROLE_END)
>>> ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
>>> @@ -176,11 +185,8 @@ static void cdns3_role_switch(struct work_struct *work)
>>>
>>> cdns = container_of(work, struct cdns3, role_switch_wq);
>>>
>>> - //TODO: implements this functions.
>>> - //host = cdns3_is_host(cdns);
>>> - //device = cdns3_is_device(cdns);
>>> - host = 1;
>>> - device = 0;
>>> + host = cdns3_is_host(cdns);
>>> + device = cdns3_is_device(cdns);
>>
>> What if there is a ID transition between the 2 functions so that
>> and both host and device become true?
>> Since you are checking the ID level separately in both the functions.
>>
>> How about instead having cdns3_get_id() and using
>> it to start/stop relevant roles if we are in OTG mode.
>>
>> Is this going to be used for a role switch even if we're not in OTG mode?
>> If not then it is a BUG if we get here.
>>
> Good point.
> User can change current mode by debugfs and then this function will also invoked.
> Probably I use cdns3_get_id as you suggest.
>
>>>
>>> if (host)
>>> role = CDNS3_ROLE_HOST;
>>> @@ -194,6 +200,12 @@ static void cdns3_role_switch(struct work_struct *work)
>>> pm_runtime_get_sync(cdns->dev);
>>> cdns3_role_stop(cdns);
>>>
>>> + if (cdns->desired_dr_mode != cdns->current_dr_mode) {
>>
>> This is about roles, why are we checking dr_mode here?
>
> Because after changing dr_mode by means of debugfs we need to update mode.
> Driver should do this after stopping the previous role. I will move this condition
> to cdns3_drd_update_mode and add comment in this place.
>
>>
>>> + cdns3_drd_update_mode(cdns);
>>> + host = cdns3_is_host(cdns);
>>> + device = cdns3_is_device(cdns);
>>> + }
>>> +
>>> if (host) {
>>> if (cdns->roles[CDNS3_ROLE_HOST])
>>> cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
>>> @@ -287,6 +299,7 @@ static int cdns3_probe(struct platform_device *pdev)
>>> if (ret)
>>> goto err2;
>>>
>>> + ret = cdns3_drd_init(cdns);
>>> if (ret)
>>> goto err2;
>>>
>>> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
>>> new file mode 100644
>>> index 000000000000..ac741c80e776
>>> --- /dev/null
>>> +++ b/drivers/usb/cdns3/drd.c
>>> @@ -0,0 +1,229 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Cadence USBSS DRD Driver.
>>> + *
>>> + * Copyright (C) 2018 Cadence.
>>> + *
>>> + * Author: Pawel Laszczak <pawell@xxxxxxxxxxx
>>> + *
>>> + */
>>> +#include <linux/kernel.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/usb/otg.h>
>>> +
>>> +#include "gadget.h"
>>> +#include "drd.h"
>>> +
>>> +/**
>>> + * cdns3_set_mode - change mode of OTG Core
>>> + * @cdns: pointer to context structure
>>> + * @mode: selected mode from cdns_role
>>> + */
>>> +void cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
>>> +{
>>> + u32 reg;
>>> +
>>> + cdns->current_dr_mode = mode;
>>> + switch (mode) {
>>> + case USB_DR_MODE_PERIPHERAL:
>>> + dev_info(cdns->dev, "Set controller to Gadget mode\n");
>>> + writel(OTGCMD_DEV_BUS_REQ | OTGCMD_OTG_DIS,
>>> + &cdns->otg_regs->cmd);
>>> + break;
>>> + case USB_DR_MODE_HOST:
>>> + dev_info(cdns->dev, "Set controller to Host mode\n");
>>> + writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
>>> + &cdns->otg_regs->cmd);
>>> + break;
>>> + case USB_DR_MODE_OTG:
>>> + dev_info(cdns->dev, "Set controller to OTG mode\n");
>>> + reg = readl(&cdns->otg_regs->ctrl1);
>>> + reg |= OTGCTRL1_IDPULLUP;
>>> + writel(reg, &cdns->otg_regs->ctrl1);
>>> +
>>> + /* wait until valid ID (ID_VALUE) can be sampled (50ms). */
>>> + mdelay(50);
>>> + break;
>>> + default:
>>> + cdns->current_dr_mode = USB_DR_MODE_UNKNOWN;
>>> + dev_err(cdns->dev, "Unsupported mode of operation %d\n", mode);
>>> + return;
>>> + }
>>> +}
>>> +
>>> +static int cdns3_otg_get_id(struct cdns3 *cdns)
>>> +{
>>> + int id;
>>> +
>>> + id = readl(&cdns->otg_regs->sts) & OTGSTS_ID_VALUE;
>>> + dev_dbg(cdns->dev, "OTG ID: %d", id);
>>> + return id;
>>> +}
>>> +
>>> +int cdns3_is_host(struct cdns3 *cdns)
>>> +{
>>> + if (cdns->current_dr_mode == USB_DR_MODE_HOST)
>>> + return 1;
>>
>> Why do you need this?
>
> I assumed that some SoC could have cut DRD /OTG and Device or Host part.
> In such case the driver cannot be based on ID pin.
> For only HOST it's not a problem because
> the standard XHCI driver will be used. Probably I will remove this fragment.
>>
>>> + else if (cdns->current_dr_mode == USB_DR_MODE_OTG)
>>> + if (!cdns3_otg_get_id(cdns))
>>> + return 1;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +int cdns3_is_device(struct cdns3 *cdns)
>>> +{
>>> + if (cdns->current_dr_mode == USB_DR_MODE_PERIPHERAL)
>>> + return 1;
>>> + else if (cdns->current_dr_mode == USB_DR_MODE_OTG)
>>> + if (cdns3_otg_get_id(cdns))
>>> + return 1;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * cdns3_otg_disable_irq - Disable all OTG interrupts
>>> + * @cdns: Pointer to controller context structure
>>> + */
>>> +static void cdns3_otg_disable_irq(struct cdns3 *cdns)
>>> +{
>>> + writel(0, &cdns->otg_regs->ien);
>>> +}
>>> +
>>> +/**
>>> + * cdns3_otg_enable_irq - enable id and sess_valid interrupts
>>> + * @cdns: Pointer to controller context structure
>>> + */
>>> +static void cdns3_otg_enable_irq(struct cdns3 *cdns)
>>> +{
>>> + writel(OTGIEN_ID_CHANGE_INT | OTGIEN_VBUSVALID_RISE_INT |
>>> + OTGIEN_VBUSVALID_FALL_INT, &cdns->otg_regs->ien);
>>> +}
>>> +
>>> +/**
>>> + * cdns3_init_otg_mode - initialize drd controller
>>> + * @cdns: Pointer to controller context structure
>>> + *
>>> + * Returns 0 on success otherwise negative errno
>>> + */
>>> +static void cdns3_init_otg_mode(struct cdns3 *cdns)
>>> +{
>>> + cdns3_otg_disable_irq(cdns);
>>> + /* clear all interrupts */
>>> + writel(~0, &cdns->otg_regs->ivect);
>>> +
>>> + cdns3_set_mode(cdns, USB_DR_MODE_OTG);
>>> +
>>> + cdns3_otg_enable_irq(cdns);
>>> +}
>>> +
>>> +/**
>>> + * cdns3_drd_update_mode - initialize mode of operation
>>
>> Looks like this will be called only once. How about calling it
>>
>> cdns3_drd_init_mode()?
>
> It will be also called after changing dr_mode from debugfs.
>
>>> + * @cdns: Pointer to controller context structure
>>> + *
>>> + * Returns 0 on success otherwise negative errno
>>> + */
>>> +int cdns3_drd_update_mode(struct cdns3 *cdns)
>>> +{
>>> + int ret = 0;
>>> +
>>> + switch (cdns->desired_dr_mode) {
>>
>> I think we can get rid of desired_dr_mode member in struct cdns.
>> Just pass the mode as an argument to cdns3_drd_init_mode()
>
> This will be used also in patch that introduce debugfs. This filed is also used
> during changing dr_mode from user space.
>
> My intention was:
> dr_mode - indicated what driver can support, this filed based on dr_mode from device tree,
> straps bits from otg register and kernel configuration.
> desired_ dr_mode - the next mode desired by user, changed by debugfs
> current_dr_mode - actually selected mode
>
OK, makes sense. But let's keep this patch simple. Add only the members you need right now.
Introduce the new one (desired_dr_mode) in the debugfs patch.
>>
>> And we already have cdns->dr_mode.
>>
>>> + case USB_DR_MODE_PERIPHERAL:
>>> + cdns3_set_mode(cdns, USB_DR_MODE_PERIPHERAL);
>>> + break;
>>> + case USB_DR_MODE_HOST:
>>> + cdns3_set_mode(cdns, USB_DR_MODE_HOST);
>>> + break;
>>> + case USB_DR_MODE_OTG:
>>> + cdns3_init_otg_mode(cdns);
>>> + break;
>>> + default:
>>> + dev_err(cdns->dev, "Unsupported mode of operation %d\n",
>>> + cdns->dr_mode);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return ret;
>>> +}
<snip>
cheers,
-roger
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
