| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150121085839.GA22716@kuha.fi.intel.com>
Date: Wed, 21 Jan 2015 10:58:39 +0200
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: Felipe Balbi <balbi@...com>
Cc: Kishon Vijay Abraham I <kishon@...com>,
Baolu Lu <baolu.lu@...ux.intel.com>, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] usb: dwc3: add ULPI interface support
On Tue, Jan 20, 2015 at 09:23:37AM -0600, Felipe Balbi wrote:
> Hi,
>
> On Tue, Jan 20, 2015 at 11:18:21AM +0200, Heikki Krogerus wrote:
> > Registers ULPI interface with the ULPI bus if HSPHY type is
> > ULPI.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > Cc: Felipe Balbi <balbi@...com>
>
> you're doing quite a bit in a single patch...
>
> > ---
> > drivers/usb/dwc3/Kconfig | 7 ++++
> > drivers/usb/dwc3/Makefile | 4 ++
> > drivers/usb/dwc3/core.c | 9 +++-
> > drivers/usb/dwc3/core.h | 22 ++++++++++
> > drivers/usb/dwc3/ulpi.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 143 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/usb/dwc3/ulpi.c
> >
> > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> > index 58b5b2c..6d0c5e6 100644
> > --- a/drivers/usb/dwc3/Kconfig
> > +++ b/drivers/usb/dwc3/Kconfig
> > @@ -11,6 +11,13 @@ config USB_DWC3
> >
> > if USB_DWC3
> >
> > +config USB_DWC3_ULPI
> > + bool "Provide ULPI PHY Interface"
> > + depends on ULPI_PHY=y || ULPI_PHY=USB_DWC3
> > + help
> > + Select this if you have ULPI type PHY attached to your DWC3
> > + controller.
> > +
> > choice
> > bool "DWC3 Mode Selection"
> > default USB_DWC3_DUAL_ROLE if (USB && USB_GADGET)
> > diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> > index bb34fbc..2fc44e0 100644
> > --- a/drivers/usb/dwc3/Makefile
> > +++ b/drivers/usb/dwc3/Makefile
> > @@ -16,6 +16,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) $(CONFIG_USB_DWC3_DUAL_ROLE)),)
> > dwc3-y += gadget.o ep0.o
> > endif
> >
> > +ifneq ($(CONFIG_USB_DWC3_ULPI),)
> > + dwc3-y += ulpi.o
> > +endif
> > +
> > ifneq ($(CONFIG_DEBUG_FS),)
> > dwc3-y += debugfs.o
> > endif
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 25ddc39..5219bc7 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -876,12 +876,17 @@ static int dwc3_probe(struct platform_device *pdev)
> > dwc->hird_threshold = hird_threshold
> > | (dwc->is_utmi_l1_suspend << 4);
> >
> > + platform_set_drvdata(pdev, dwc);
> > +
> > + ret = dwc3_ulpi_init(dwc);
> > + if (ret)
> > + return ret;
> > +
> > ret = dwc3_core_get_phy(dwc);
> > if (ret)
> > return ret;
> >
> > spin_lock_init(&dwc->lock);
> > - platform_set_drvdata(pdev, dwc);
>
> why do you need to move this ? Looks like this should be a cleanup and
> split into a single patch.
OK.
> it also appears that you need another patch moving dwc3_cache_hwparams()
> before all of these other calls, so you can use it from
> dwc3_ulpi_init().
OK.
> > @@ -965,6 +970,7 @@ err1:
> >
> > err0:
> > dwc3_free_event_buffers(dwc);
> > + dwc3_ulpi_exit(dwc);
> >
> > return ret;
> > }
> > @@ -984,6 +990,7 @@ static int dwc3_remove(struct platform_device *pdev)
> > phy_power_off(dwc->usb3_generic_phy);
> >
> > dwc3_core_exit(dwc);
> > + dwc3_ulpi_exit(dwc);
> >
> > pm_runtime_put_sync(&pdev->dev);
> > pm_runtime_disable(&pdev->dev);
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 0842aa8..f6881a6 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -32,6 +32,7 @@
> > #include <linux/usb/otg.h>
> >
> > #include <linux/phy/phy.h>
> > +#include <linux/phy/ulpi/interface.h>
> >
> > #define DWC3_MSG_MAX 500
> >
> > @@ -174,6 +175,14 @@
> > #define DWC3_GUSB2PHYCFG_PHYSOFTRST (1 << 31)
> > #define DWC3_GUSB2PHYCFG_SUSPHY (1 << 6)
> >
> > +/* Global USB2 PHY Vendor Control Register */
> > +#define DWC3_GUSB2PHYACC_NEWREGREQ (1 << 25)
> > +#define DWC3_GUSB2PHYACC_BUSY (1 << 23)
> > +#define DWC3_GUSB2PHYACC_WRITE (1 << 22)
> > +#define DWC3_GUSB2PHYACC_ADDR(n) (n << 16)
> > +#define DWC3_GUSB2PHYACC_EXTEND_ADDR(n) (n << 8)
> > +#define DWC3_GUSB2PHYACC_DATA(n) (n & 0xff)
>
> separate patch
OK.
> > @@ -590,6 +599,7 @@ struct dwc3_hwparams {
> > #define DWC3_NUM_INT(n) (((n) & (0x3f << 15)) >> 15)
> >
> > /* HWPARAMS3 */
> > +#define DWC3_ULPI_HSPHY (1 << 3)
>
> you also need a patch which defines this bit of HWPARAMS3. This is also
> the wrong definition. Which core revision do you have ? I can see that
> bit 3 is part of a 2 bit field called:
>
> DWC_USB3_HSPHY_INTERFACE
I have the same in my databook. I agree, it's not good like that.
> moreover, there are systems which have both ULPI and UTMI enabled and
> you can't really know which one the PHY is using.
>
> This needs a bit more thought.
Sure, I'll think of something better for this.
> > #define DWC3_NUM_IN_EPS_MASK (0x1f << 18)
> > #define DWC3_NUM_EPS_MASK (0x3f << 12)
> > #define DWC3_NUM_EPS(p) (((p)->hwparams3 & \
> > @@ -739,6 +749,8 @@ struct dwc3 {
> > struct phy *usb2_generic_phy;
> > struct phy *usb3_generic_phy;
> >
> > + struct ulpi *ulpi;
> > +
> > void __iomem *regs;
> > size_t regs_size;
> >
> > @@ -1035,4 +1047,14 @@ static inline int dwc3_gadget_resume(struct dwc3 *dwc)
> > }
> > #endif /* !IS_ENABLED(CONFIG_USB_DWC3_HOST) */
> >
> > +#if IS_ENABLED(CONFIG_USB_DWC3_ULPI)
> > +int dwc3_ulpi_init(struct dwc3 *dwc);
> > +void dwc3_ulpi_exit(struct dwc3 *dwc);
> > +#else
> > +static inline int dwc3_ulpi_init(struct dwc3 *dwc)
> > +{ return 0; }
> > +static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
> > +{ }
> > +#endif
> > +
> > #endif /* __DRIVERS_USB_DWC3_CORE_H */
> > diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
> > new file mode 100644
> > index 0000000..ee3ebbe
> > --- /dev/null
> > +++ b/drivers/usb/dwc3/ulpi.c
> > @@ -0,0 +1,102 @@
> > +/**
> > + * ulpi.c - DesignWare USB3 Controller's ULPI PHY interface
> > + *
> > + * Copyright (C) 2015 Intel Corporation
> > + *
> > + * Author: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/phy/ulpi/regs.h>
> > +
> > +#include "core.h"
> > +#include "io.h"
> > +
> > +#define DWC3_ULPI_ADDR(a) \
> > + ((a >= ULPI_EXT_VENDOR_SPECIFIC) ? \
> > + DWC3_GUSB2PHYACC_ADDR(ULPI_ACCESS_EXTENDED) | \
> > + DWC3_GUSB2PHYACC_EXTEND_ADDR(a) : DWC3_GUSB2PHYACC_ADDR(a))
> > +
> > +static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
> > +{
> > + unsigned count = 1000;
> > + u32 reg;
> > +
> > + while (count--) {
> > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
> > + if (!(reg & DWC3_GUSB2PHYACC_BUSY))
> > + return 0;
>
> this could use a cpu_relax();
Agreed.
> > + }
> > +
> > + return -ETIMEDOUT;
> > +}
> > +
> > +static int dwc3_ulpi_read(struct ulpi_ops *ops, u8 addr)
> > +{
> > + struct dwc3 *dwc = dev_get_drvdata(ops->dev);
> > + u32 reg;
> > + int ret;
> > +
> > + reg = DWC3_GUSB2PHYACC_NEWREGREQ | DWC3_ULPI_ADDR(addr);
> > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYACC(0), reg);
> > +
> > + ret = dwc3_ulpi_busyloop(dwc);
> > + if (ret)
> > + return ret;
> > +
> > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
> > +
> > + return DWC3_GUSB2PHYACC_DATA(reg);
> > +}
> > +
> > +static int dwc3_ulpi_write(struct ulpi_ops *ops, u8 addr, u8 val)
> > +{
> > + struct dwc3 *dwc = dev_get_drvdata(ops->dev);
> > + u32 reg;
> > + int ret;
> > +
> > + reg = DWC3_GUSB2PHYACC_NEWREGREQ | DWC3_ULPI_ADDR(addr);
> > + reg |= DWC3_GUSB2PHYACC_WRITE | val;
> > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYACC(0), reg);
> > +
> > + ret = dwc3_ulpi_busyloop(dwc);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static struct ulpi_ops dwc3_ulpi = {
> > + .read = dwc3_ulpi_read,
> > + .write = dwc3_ulpi_write,
> > +};
> > +
> > +int dwc3_ulpi_init(struct dwc3 *dwc)
> > +{
> > + int ret;
> > +
> > + /* First check if there is ULPI PHY */
> > + ret = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> > + if (!(ret & DWC3_ULPI_HSPHY))
> > + return 0;
>
> should use the cached structure.
Sure.
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists