On 04/12/18 10:50, Peter Chen wrote:

quoted

quoted

+ * Cadence USBSS DRD Driver.
+ *
+ * Copyright (C) 2018 Cadence.
+ *
+ * Author: Peter Chen [off-list ref]
+ *         Pawel Laszczak [off-list ref]
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+
+#include "gadget.h"
+#include "core.h"
+
+static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
+{
+     WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
+     return cdns->roles[cdns->role];
+}
+
+static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
+{
+     int ret;
+
+     if (role >= CDNS3_ROLE_END)

WARN_ON()?

quoted

+             return 0;
+
+     if (!cdns->roles[role])
+             return -ENXIO;
+
+     mutex_lock(&cdns->mutex);
+     cdns->role = role;
+     ret = cdns->roles[role]->start(cdns);
+     mutex_unlock(&cdns->mutex);
+     return ret;
+}
+
+static inline void cdns3_role_stop(struct cdns3 *cdns)
+{
+     enum cdns3_roles role = cdns->role;
+
+     if (role == CDNS3_ROLE_END)

WARN_ON(role >= CNDS3_ROLE_END) ?

quoted

+             return;
+
+     mutex_lock(&cdns->mutex);
+     cdns->roles[role]->stop(cdns);
+     cdns->role = CDNS3_ROLE_END;

Why change the role here? You are just stopping the role not changing it.
I think cdns->role should remain unchanged, so we can call cdns3_role_start()
if required without error.

The current version of this IP has some issues to detect vbus status correctly,
we have to force vbus status accordingly, so we need a status to indicate
vbus disconnection, and add some code to let controller know vbus
removal, in that case, the controller's state machine can be correct.
So, we increase one role 'CDNS3_ROLE_END' to for this purpose.

CDNS3_ROLE_GADGET: gadget mode and VBUS on
CDNS3_ROLE_HOST: host mode and VBUS on
CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.

So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
and need to set role as CDNS3_ROLE_END at ->stop for further handling at
role switch routine.

OK. but still this (changing to ROLE_END) must be moved to the role switch routine
and the explanation you just mentioned must be added there.

quoted

quoted

+     mutex_unlock(&cdns->mutex);
+}
+
+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;
+     }
+     return cdns->roles[CDNS3_ROLE_HOST]
+             ? CDNS3_ROLE_HOST
+             : CDNS3_ROLE_GADGET;

Why not just
        return cdns->role;

I'm wondering if we really need this function.

cdns->role gets from cdns3_get_role, and this API tells role at the runtime.
If both roles are supported, the role is decided by external
conditions, eg, vbus/id
or external connector. If only single role is supported, only one role structure
is allocated, cdns->roles[CDNS3_ROLE_HOST] or cdns->roles[CDNS3_ROLE_GADGET]

How about adding this description in function documentation.

quoted

quoted

+}

quoted

+
+/**
+ * cdns3_core_init_role - initialize role of operation
+ * @cdns: Pointer to cdns3 structure
+ *
+ * Returns 0 on success otherwise negative errno
+ */
+static int cdns3_core_init_role(struct cdns3 *cdns)
+{
+     struct device *dev = cdns->dev;
+     enum usb_dr_mode dr_mode;
+
+     dr_mode = usb_get_dr_mode(dev);
+     cdns->role = CDNS3_ROLE_END;
+
+     /*
+      * If driver can't read mode by means of usb_get_dr_mdoe function then
+      * chooses mode according with Kernel configuration. This setting
+      * can be restricted later depending on strap pin configuration.
+      */
+     if (dr_mode == USB_DR_MODE_UNKNOWN) {
+             if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
+                 IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
+                     dr_mode = USB_DR_MODE_OTG;
+             else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
+                     dr_mode = USB_DR_MODE_HOST;
+             else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
+                     dr_mode = USB_DR_MODE_PERIPHERAL;
+     }
+
+     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
+             //TODO: implements host initialization

                /* TODO: Add host role */ ?

quoted

+     }
+
+     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
+             //TODO: implements device initialization

                /* TODO: Add device role */ ?

Yes, it needs to allocate cdns->roles[CDNS3_ROLE_HOST] and
cdns->roles[CDNS3_ROLE_GADGET].

quoted

quoted

+     }
+
+     if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
+             dev_err(dev, "no supported roles\n");
+             return -ENODEV;
+     }
+
+     cdns->dr_mode = dr_mode;

Pawel, why dr_mode needs to be introduced?

quoted

quoted

+     return 0;
+}
+
+/**
+ * cdns3_irq - interrupt handler for cdns3 core device
+ *
+ * @irq: irq number for cdns3 core device
+ * @data: structure of cdns3
+ *
+ * Returns IRQ_HANDLED or IRQ_NONE
+ */
+static irqreturn_t cdns3_irq(int irq, void *data)
+{
+     struct cdns3 *cdns = data;
+     irqreturn_t ret = IRQ_NONE;
+
+     /* Handle device/host interrupt */
+     if (cdns->role != CDNS3_ROLE_END)

Is it because of this that you need to set role to END at role_stop?
I think it is better to add a state variable to struct cdns3_role_driver, so we can
check if it is active or stopped.

e.g.
        if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE)

quoted

+             ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
+
+     return ret;
+}
+

 CDNS3_ROLE_END is introduced from above comments, we don't
need another flag for it.
If cdns->role == CDNS3_ROLE_END, it handles VBUS and ID interrupt.

quoted

quoted

+static void cdns3_remove_roles(struct cdns3 *cdns)

Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()?

It is planed to called when at ->remove.

quoted

quoted

+{
+     //TODO: implements this function
+}

quoted

+
+static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
+{
+     enum cdns3_roles current_role;
+     int ret = 0;
+
+     current_role = cdns->role;
+
+     if (role == CDNS3_ROLE_END)
+             return 0;

role == END looks like error state. and it should never happen.
WARN here?

See my comments above.

quoted

quoted

+
+     dev_dbg(cdns->dev, "Switching role");
+

Don't you have to stop the previous role before starting the new role?

Yes, it is needed. Pawel may simply some flows to suit his platform.

quoted

quoted

+     ret = cdns3_role_start(cdns, role);
+     if (ret) {
+             /* Back to current role */
+             dev_err(cdns->dev, "set %d has failed, back to %d\n",
+                     role, current_role);
+             ret = cdns3_role_start(cdns, current_role);
+     }
+
+     return ret;
+}
+
+/**
+ * cdns3_role_switch - work queue handler for role switch
+ *
+ * @work: work queue item structure
+ *
+ * Handles below events:
+ * - Role switch for dual-role devices
+ * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
+ */
+static void cdns3_role_switch(struct work_struct *work)
+{
+     enum cdns3_roles role = CDNS3_ROLE_END;
+     struct cdns3 *cdns;
+     bool device, host;
+
+     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;
+
+     if (host)
+             role = CDNS3_ROLE_HOST;
+     else if (device)
+             role = CDNS3_ROLE_GADGET;
+
+     if (cdns->desired_dr_mode == cdns->current_dr_mode &&
+         cdns->role == role)
+             return;
+

I think all the below code can be moved to cdns3_do_role_switch().

quoted

+     pm_runtime_get_sync(cdns->dev);
+     cdns3_role_stop(cdns);
+
+     if (host) {
+             if (cdns->roles[CDNS3_ROLE_HOST])
+                     cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
+             pm_runtime_put_sync(cdns->dev);
+             return;
+     }
+
+     if (device)
+             cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
+     else
+             cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
+
+     pm_runtime_put_sync(cdns->dev);
+}
+
+/**
+ * cdns3_probe - probe for cdns3 core device
+ * @pdev: Pointer to cdns3 core platform device
+ *
+ * Returns 0 on success otherwise negative errno
+ */
+static int cdns3_probe(struct platform_device *pdev)
+{
+     struct device *dev = &pdev->dev;
+     struct resource *res;
+     struct cdns3 *cdns;
+     void __iomem *regs;
+     int ret;
+
+     cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
+     if (!cdns)
+             return -ENOMEM;
+
+     cdns->dev = dev;
+
+     platform_set_drvdata(pdev, cdns);
+
+     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+     if (!res) {
+             dev_err(dev, "missing IRQ\n");
+             return -ENODEV;
+     }
+     cdns->irq = res->start;
+
+     /*
+      * Request memory region
+      * region-0: xHCI
+      * region-1: Peripheral
+      * region-2: OTG registers
+      */

The memory region order is different from the dt-binding.
There it is OTG, host(xhci), device (peripheral).

quoted

+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     regs = devm_ioremap_resource(dev, res);
+
+     if (IS_ERR(regs))
+             return PTR_ERR(regs);
+     cdns->xhci_regs = regs;
+     cdns->xhci_res = res;
+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+     regs = devm_ioremap_resource(dev, res);
+     if (IS_ERR(regs))
+             return PTR_ERR(regs);
+     cdns->dev_regs  = regs;
+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+     regs = devm_ioremap_resource(dev, res);
+     if (IS_ERR(regs))
+             return PTR_ERR(regs);
+     cdns->otg_regs = regs;
+
+     mutex_init(&cdns->mutex);
+
+     cdns->phy = devm_phy_get(dev, "cdns3,usbphy");

"cdns3,usbphy" is not documented in dt-binding.

quoted

+     if (IS_ERR(cdns->phy)) {
+             dev_info(dev, "no generic phy found\n");
+             cdns->phy = NULL;
+             /*
+              * fall through here!
+              * if no generic phy found, phy init
+              * should be done under boot!
+              */

No you shouldn't fall through always if it is an error condition.
Something like this should work better.

        if (IS_ERR(cnds->phy)) {
                ret = PTR_ERR(cdns->phy);
                if (ret == -ENOSYS || ret == -ENODEV) {
                        cdns->phy = NULL;
                } else if (ret == -EPROBE_DEFER) {
                        return ret;
                } else {
                        dev_err(dev, "no phy found\n");
                        goto err0;
                }
        }

So if PHY was provided in DT, and PHY support/drivers is present
and error condition means something is wrong and we have to error out.

quoted

+     } else {
+             phy_init(cdns->phy);
+     }

You can do phy_init() outside the else.

quoted

+
+     ret = cdns3_core_init_role(cdns);
+     if (ret)
+             goto err1;
+
+     INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
+     if (ret)
+             goto err2;
+
+     if (ret)
+             goto err2;
+
+     cdns->role = cdns3_get_role(cdns);

I think this should move to cdns3_core_init_role().

I agree.

quoted

quoted

+
+     ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
+                            dev_name(dev), cdns);
+
+     if (ret)
+             goto err2;

How about moving request_irq to before cdsn3_core_init_role()?

Then you can move cdns3_role_start() as well to core_init_role().

Usually, we request irq after hardware initialization has finished, if not,
there may unexpected interrupt.

Doesn't kernel warn if interrupt happens and there is no handler?
To avoid that I was suggesting to request_irq first.

cheers,
-roger

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