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

From: Roger Quadros
Date: Fri Nov 23 2018 - 09:52:11 EST

Next message: Yangtao Li: "[PATCH] thermal/intel_powerclamp: Change to use DEFINE_SHOW_ATTRIBUTE macro"
Previous message: Yangtao Li: "[PATCH] thermal: tegra: soctherm: Change to use DEFINE_SHOW_ATTRIBUTE macro"
In reply to: Pawel Laszczak: "[RFC PATCH v2 05/15] usb:cdns3: Added DRD support"
Next in thread: Pawel Laszczak: "[RFC PATCH v2 09/15] usb:cdns3: EpX operations part of the API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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

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

> + 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?

> + 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()?

> + * @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()

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;
> +}
> +
> +irqreturn_t cdns3_drd_irq(struct cdns3 *cdns)
> +{
> + irqreturn_t ret = IRQ_NONE;
> + u32 reg;
> +
> + if (cdns->dr_mode != USB_DR_MODE_OTG)
> + return ret;
> +
> + reg = readl(&cdns->otg_regs->ivect);
> + if (!reg)
> + return ret;
> +
> + if (reg & OTGIEN_ID_CHANGE_INT) {
> + int id = cdns3_otg_get_id(cdns);
> +
> + dev_dbg(cdns->dev, "OTG IRQ: new ID: %d\n",
> + cdns3_otg_get_id(cdns));
> +
> + if (id)
> + cdns->role = CDNS3_ROLE_GADGET;
> + else
> + cdns->role = CDNS3_ROLE_HOST;

Why check ID and set role here? It might change by the time
the role_switch_wq starts up. Why not check the ID status there?

Also directly changing role here doesn't make sense as
there will be a mismatch between currently active role and cdns->role.

> +
> + queue_work(system_freezable_wq, &cdns->role_switch_wq);
> +
> + ret = IRQ_HANDLED;
> + }
> +
> + writel(~0, &cdns->otg_regs->ivect);
> + return IRQ_HANDLED;

return ret;

> +}
> +
> +int cdns3_drd_init(struct cdns3 *cdns)
> +{
> + enum usb_dr_mode dr_mode;
> + int ret = 0;
> + u32 state;
> +
> + state = OTGSTS_STRAP(readl(&cdns->otg_regs->sts));
> +
> + dr_mode = cdns->dr_mode;
> + if (state == OTGSTS_STRAP_HOST) {
> + dev_info(cdns->dev, "Controller strapped to HOST\n");
> + dr_mode = USB_DR_MODE_HOST;
> + if (cdns->dr_mode != USB_DR_MODE_HOST &&
> + cdns->dr_mode != USB_DR_MODE_OTG)
> + ret = -EINVAL;
> + } else if (state == OTGSTS_STRAP_GADGET) {
> + dev_info(cdns->dev, "Controller strapped to PERIPHERAL\n");
> + dr_mode = USB_DR_MODE_PERIPHERAL;
> + if (cdns->dr_mode != USB_DR_MODE_PERIPHERAL &&
> + cdns->dr_mode != USB_DR_MODE_OTG)
> + ret = -EINVAL;
> + }
> +
> + if (ret) {
> + dev_err(cdns->dev, "Incorrect DRD configuration\n");
> + return ret;
> + }
> +
> + //Updating DR mode according to strap.
> + cdns->dr_mode = dr_mode;
> + cdns->desired_dr_mode = dr_mode;
> + cdns->current_dr_mode = USB_DR_MODE_UNKNOWN;
> +
> + dev_info(cdns->dev, "Controller Device ID: %08lx, Revision ID: %08lx\n",
> + CDNS_RID(readl(&cdns->otg_regs->rid)),
> + CDNS_DID(readl(&cdns->otg_regs->did)));

dev_info should be moved at the end if cdns3_drd_update_mode() if it succeeded.

> +
> + state = readl(&cdns->otg_regs->sts);
> + if (OTGSTS_OTG_NRDY(state) != 0) {
> + dev_err(cdns->dev, "Cadence USB3 OTG device not ready\n");
> + return -ENODEV;
> + }
> +
> + ret = cdns3_drd_update_mode(cdns);
> +
> + return ret;
> +}
> diff --git a/drivers/usb/cdns3/drd.h b/drivers/usb/cdns3/drd.h
> new file mode 100644
> index 000000000000..0faa7520ecac
> --- /dev/null
> +++ b/drivers/usb/cdns3/drd.h
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Cadence USB3 DRD part of USBSS driver
> + *
> + * Copyright (C) 2018 Cadence.
> + *
> + * Author: Pawel Laszczak <pawell@xxxxxxxxxxx>
> + */
> +#ifndef __LINUX_CDNS3_DRD
> +#define __LINUX_CDNS3_DRD
> +
> +#include <linux/usb/otg.h>
> +#include <linux/phy/phy.h>
> +#include "core.h"
> +
> +/* DRD register interface. */
> +struct cdns3_otg_regs {
> + __le32 did;
> + __le32 rid;
> + __le32 capabilities;
> + __le32 reserved1;
> + __le32 cmd;
> + __le32 sts;
> + __le32 state;
> + __le32 reserved2;
> + __le32 ien;
> + __le32 ivect;
> + __le32 refclk;
> + __le32 tmr;
> + __le32 reserved3[4];
> + __le32 simulate;
> + __le32 override;
> + __le32 susp_ctrl;
> + __le32 reserved4;
> + __le32 anasts;
> + __le32 adp_ramp_time;
> + __le32 ctrl1;
> + __le32 ctrl2;
> +};
> +
> +/* CDNS_RID - bitmasks */
> +#define CDNS_RID(p) ((p) & GENMASK(15, 0))
> +
> +/* CDNS_VID - bitmasks */
> +#define CDNS_DID(p) ((p) & GENMASK(31, 0))
> +
> +/* OTGCMD - bitmasks */
> +/* "Request the bus for Device mode. */
> +#define OTGCMD_DEV_BUS_REQ BIT(0)
> +/* Request the bus for Host mode */
> +#define OTGCMD_HOST_BUS_REQ BIT(1)
> +/* Enable OTG mode. */
> +#define OTGCMD_OTG_EN BIT(2)
> +/* Disable OTG mode */
> +#define OTGCMD_OTG_DIS BIT(3)
> +/*"Configure OTG as A-Device. */
> +#define OTGCMD_A_DEV_EN BIT(4)
> +/*"Configure OTG as A-Device. */
> +#define OTGCMD_A_DEV_DIS BIT(5)
> +/* Drop the bus for Device mod e. */
> +#define OTGCMD_DEV_BUS_DROP BIT(8)
> +/* Drop the bus for Host mode*/
> +#define OTGCMD_HOST_BUS_DROP BIT(9)
> +/* Power Down USBSS-DEV. */
> +#define OTGCMD_DEV_POWER_OFF BIT(11)
> +/* Power Down CDNSXHCI. */
> +#define OTGCMD_HOST_POWER_OFF BIT(12)
> +
> +/* OTGIEN - bitmasks */
> +/* ID change interrupt enable */
> +#define OTGIEN_ID_CHANGE_INT BIT(0)
> +/* Vbusvalid fall detected interrupt enable.*/
> +#define OTGIEN_VBUSVALID_RISE_INT BIT(4)
> +/* Vbusvalid fall detected interrupt enable */
> +#define OTGIEN_VBUSVALID_FALL_INT BIT(5)
> +
> +/* OTGSTS - bitmasks */
> +/*
> + * Current value of the ID pin. It is only valid when idpullup in
> + * OTGCTRL1_TYPE register is set to '1'.
> + */
> +#define OTGSTS_ID_VALUE BIT(0)
> +/* Current value of the vbus_valid */
> +#define OTGSTS_VBUS_VALID BIT(1)
> +/* Current value of the b_sess_vld */
> +#define OTGSTS_SESSION_VALID BIT(2)
> +/*Device mode is active*/
> +#define OTGSTS_DEV_ACTIVE BIT(3)
> +/* Host mode is active. */
> +#define OTGSTS_HOST_ACTIVE BIT(4)
> +/* OTG Controller not ready. */
> +#define OTGSTS_OTG_NRDY_MASK BIT(11)
> +#define OTGSTS_OTG_NRDY(p) ((p) & OTGSTS_OTG_NRDY_MASK)
> +/*
> + * Value of the strap pins.
> + * 000 - no default configuration
> + * 010 - Controller initiall configured as Host
> + * 100 - Controller initially configured as Device
> + */
> +#define OTGSTS_STRAP(p) (((p) & GENMASK(14, 12)) >> 12)
> +#define OTGSTS_STRAP_NO_DEFAULT_CFG 0x00
> +#define OTGSTS_STRAP_HOST_OTG 0x01
> +#define OTGSTS_STRAP_HOST 0x02
> +#define OTGSTS_STRAP_GADGET 0x04
> +/* Host mode is turned on. */
> +#define OTGSTSE_XHCI_READYF BIT(26)
> +/* "Device mode is turned on .*/
> +#define OTGSTS_DEV_READY BIT(27)
> +
> +/* OTGREFCLK - bitmasks */
> +#define OTGREFCLK_STB_CLK_SWITCH_EN BIT(31)
> +
> +/* OTGCTRL1 - bitmasks */
> +#define OTGCTRL1_IDPULLUP BIT(24)
> +
> +int cdns3_is_host(struct cdns3 *cdns);
> +int cdns3_is_device(struct cdns3 *cdns);
> +int cdns3_drd_init(struct cdns3 *cdns);
> +int cdns3_drd_update_mode(struct cdns3 *cdns);
> +irqreturn_t cdns3_drd_irq(struct cdns3 *cdns);
> +
> +#endif /* __LINUX_CDNS3_DRD */
>

cheers,
-roger
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Next message: Yangtao Li: "[PATCH] thermal/intel_powerclamp: Change to use DEFINE_SHOW_ATTRIBUTE macro"
Previous message: Yangtao Li: "[PATCH] thermal: tegra: soctherm: Change to use DEFINE_SHOW_ATTRIBUTE macro"
In reply to: Pawel Laszczak: "[RFC PATCH v2 05/15] usb:cdns3: Added DRD support"
Next in thread: Pawel Laszczak: "[RFC PATCH v2 09/15] usb:cdns3: EpX operations part of the API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]