RE: [RFC PATCH v2 05/15] usb:cdns3: Added DRD support

[Pawel Laszczak]

>
>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.

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. 

We doesn't know the order in which  the system will call interrupts function related to 
 the shared interrupt line.

>
>>>> +
>>>>  	/* 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.

Ok,  I will try to reorganize these two patches. 
>
>>>
>>> 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