| Message ID | 1359559782-14552-5-git-send-email-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index 57cae1f..dcb650f 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -67,6 +67,8 @@ > #include <linux/usb/gadget.h> > #include <linux/usb/otg.h> > #include <linux/usb/chipidea.h> > +#include <linux/usb/of.h> > +#include <linux/phy.h> > > #include "ci.h" > #include "udc.h" > @@ -211,6 +213,42 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base) > return 0; > } > > +static void hw_phymode_configure(struct ci13xxx *ci) > +{ > + u32 portsc; > + > + /* > + * The lpm version has the corresponding bits in the devlc register. > + * Currently not implemented. > + */ > + if (ci->hw_bank.lpm) > + return; Why you don't implement it ? If you don't implement it, I believe you should add a warning in order to catch it when used with lpm devices. Matthieu
On Wed, Jan 30, 2013 at 05:54:54PM +0100, Matthieu CASTET wrote: > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > index 57cae1f..dcb650f 100644 > > --- a/drivers/usb/chipidea/core.c > > +++ b/drivers/usb/chipidea/core.c > > @@ -67,6 +67,8 @@ > > #include <linux/usb/gadget.h> > > #include <linux/usb/otg.h> > > #include <linux/usb/chipidea.h> > > +#include <linux/usb/of.h> > > +#include <linux/phy.h> > > > > #include "ci.h" > > #include "udc.h" > > @@ -211,6 +213,42 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base) > > return 0; > > } > > > > +static void hw_phymode_configure(struct ci13xxx *ci) > > +{ > > + u32 portsc; > > + > > + /* > > + * The lpm version has the corresponding bits in the devlc register. > > + * Currently not implemented. > > + */ > > + if (ci->hw_bank.lpm) > > + return; > Why you don't implement it ? > > If you don't implement it, I believe you should add a warning in order to catch > it when used with lpm devices. I'm against adding a warning because current users seem to go well without this setting. Adding a warning would lead to more confusion than it would help. I could try and implement it, though I'm unsure about the register layout. What I know from an earlier post from you is this: #define LPM_PTS(d) (((d)>>29)&7) #define LPM_STS BIT(28) /* serial transceiver select */ #define LPM_PTW BIT(27) /* parallel transceiver width */ Do you also know how LPM_PTS is decoded? Sascha
On Wed, Jan 30, 2013 at 04:29:40PM +0100, Sascha Hauer wrote: > From: Michael Grzeschik <m.grzeschik@pengutronix.de> > > +static void hw_phymode_configure(struct ci13xxx *ci) > +{ > + u32 portsc; > + > + /* > + * The lpm version has the corresponding bits in the devlc register. > + * Currently not implemented. > + */ > + if (ci->hw_bank.lpm) > + return; > + > + switch (ci->platdata->phy_mode) { > + case USBPHY_INTERFACE_MODE_UTMI: > + portsc = PORTSC_PTS_PTW_UTMI; > + break; > + case USBPHY_INTERFACE_MODE_UTMIW: > + portsc = PORTSC_PTS_PTW_UTMIW; > + break; > + case USBPHY_INTERFACE_MODE_ULPI: > + portsc = PORTSC_PTS_PTW_ULPI; > + break; > + case USBPHY_INTERFACE_MODE_SERIAL: > + portsc = PORTSC_PTS_PTW_SERIAL; > + break; > + case USBPHY_INTERFACE_MODE_HSIC: > + portsc = PORTSC_PTS_PTW_HSIC; > + break; > + default: > + return; > + } > + > + hw_write(ci, OP_PORTSC, PORTSC_PTS_PTW, portsc); > + > + mdelay(10); Please use usleep_range, can we recall which platform needs it? As we as I know, there is no such delay at FSL internal release code.
On Thu, Jan 31, 2013 at 11:08:54AM +0800, Peter Chen wrote: > On Wed, Jan 30, 2013 at 04:29:40PM +0100, Sascha Hauer wrote: > > From: Michael Grzeschik <m.grzeschik@pengutronix.de> > > > > +static void hw_phymode_configure(struct ci13xxx *ci) > > +{ > > + u32 portsc; > > + > > + /* > > + * The lpm version has the corresponding bits in the devlc register. > > + * Currently not implemented. > > + */ > > + if (ci->hw_bank.lpm) > > + return; > > + > > + switch (ci->platdata->phy_mode) { > > + case USBPHY_INTERFACE_MODE_UTMI: > > + portsc = PORTSC_PTS_PTW_UTMI; > > + break; > > + case USBPHY_INTERFACE_MODE_UTMIW: > > + portsc = PORTSC_PTS_PTW_UTMIW; > > + break; > > + case USBPHY_INTERFACE_MODE_ULPI: > > + portsc = PORTSC_PTS_PTW_ULPI; > > + break; > > + case USBPHY_INTERFACE_MODE_SERIAL: > > + portsc = PORTSC_PTS_PTW_SERIAL; > > + break; > > + case USBPHY_INTERFACE_MODE_HSIC: > > + portsc = PORTSC_PTS_PTW_HSIC; > > + break; > > + default: > > + return; > > + } > > + > > + hw_write(ci, OP_PORTSC, PORTSC_PTS_PTW, portsc); > > + > > + mdelay(10); > Please use usleep_range, can we recall which platform needs it? > As we as I know, there is no such delay at FSL internal release > code. Then let's drop it. People will complain when they find the platform that needs it and then we can decide whether we add this unconditionally or do something better. Sascha
Sascha Hauer a écrit : > On Wed, Jan 30, 2013 at 05:54:54PM +0100, Matthieu CASTET wrote: >>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c >>> index 57cae1f..dcb650f 100644 >>> --- a/drivers/usb/chipidea/core.c >>> +++ b/drivers/usb/chipidea/core.c >>> @@ -67,6 +67,8 @@ >>> #include <linux/usb/gadget.h> >>> #include <linux/usb/otg.h> >>> #include <linux/usb/chipidea.h> >>> +#include <linux/usb/of.h> >>> +#include <linux/phy.h> >>> >>> #include "ci.h" >>> #include "udc.h" >>> @@ -211,6 +213,42 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base) >>> return 0; >>> } >>> >>> +static void hw_phymode_configure(struct ci13xxx *ci) >>> +{ >>> + u32 portsc; >>> + >>> + /* >>> + * The lpm version has the corresponding bits in the devlc register. >>> + * Currently not implemented. >>> + */ >>> + if (ci->hw_bank.lpm) >>> + return; >> Why you don't implement it ? >> >> If you don't implement it, I believe you should add a warning in order to catch >> it when used with lpm devices. > > I'm against adding a warning because current users seem to go well > without this setting. Adding a warning would lead to more confusion than > it would help. > > I could try and implement it, though I'm unsure about the register > layout. > > What I know from an earlier post from you is this: > > #define LPM_PTS(d) (((d)>>29)&7) > #define LPM_STS BIT(28) /* serial transceiver select */ > #define LPM_PTW BIT(27) /* parallel transceiver width */ > > Do you also know how LPM_PTS is decoded? I will say the same as not lpm device : PTS is made up from PORTSCx bits 25, 30 and 31. PTS is made up from devlc bits 31, 30 and 29. Also in my datasheet, they give a way to check if the bits are read only or read write [1]. I don't know if it is worth the trouble to check it. Matthieu [1] PTS This register bit pair is used in conjunction with the configuration constant VUSB_HS_PHY_TYPE to control which parallel transceiver interface is selected. If VUSB_HS_PHY_TYPE is set for 0, 1, 2, 3, 8 or 10 then this bit is read only. If VUSB_HS_PHY_TYPE is 4, 5, 6, 7, 9 or 11 then this bit is read/write. This field is reset to: '000b' if VUSB_HS_PHY_TYPE = 0, 4 UTMI/UTMI+ '001b' if VUSB_HS_PHY_TYPE = 1, 5 ULPI DDR '010b' if VUSB_HS_PHY_TYPE = 2, 6 ULPI '011b' if VUSB_HS_PHY_TYPE = 3, 7, 8, 9 Serial/1.1 PHY/IC_USB (FS Only) '100b' if VUSB_HS_PHY_TYPE = 10, 11 UTMI for HSIC PHY
On Thu, Jan 31, 2013 at 10:15:54AM +0100, Matthieu CASTET wrote: > >> Why you don't implement it ? > >> > >> If you don't implement it, I believe you should add a warning in order to catch > >> it when used with lpm devices. > > > > I'm against adding a warning because current users seem to go well > > without this setting. Adding a warning would lead to more confusion than > > it would help. > > > > I could try and implement it, though I'm unsure about the register > > layout. > > > > What I know from an earlier post from you is this: > > > > #define LPM_PTS(d) (((d)>>29)&7) > > #define LPM_STS BIT(28) /* serial transceiver select */ > > #define LPM_PTW BIT(27) /* parallel transceiver width */ > > > > Do you also know how LPM_PTS is decoded? > > I will say the same as not lpm device : > > PTS is made up from PORTSCx bits 25, 30 and 31. Here it is PTS0 -> bit 30 PTS1 -> bit 31 PTS2 -> bit 25 > > PTS is made up from devlc bits 31, 30 and 29. In my new series I now assumed: PTS0 -> bit 29 PTS1 -> bit 30 PTS2 -> bit 31 > > > Also in my datasheet, they give a way to check if the bits are read only or read > write [1]. I don't know if it is worth the trouble to check it. > > > Matthieu > > > [1] > PTS > This register bit pair is used in conjunction with the configuration constant > VUSB_HS_PHY_TYPE to control which parallel transceiver interface is selected. If > VUSB_HS_PHY_TYPE is set for 0, 1, 2, 3, 8 or 10 then this bit is read only. If > VUSB_HS_PHY_TYPE is 4, 5, 6, 7, 9 or 11 then this bit is read/write. > > This field is reset to: > '000b' if VUSB_HS_PHY_TYPE = 0, 4 UTMI/UTMI+ > '001b' if VUSB_HS_PHY_TYPE = 1, 5 ULPI DDR > '010b' if VUSB_HS_PHY_TYPE = 2, 6 ULPI > '011b' if VUSB_HS_PHY_TYPE = 3, 7, 8, 9 Serial/1.1 PHY/IC_USB (FS Only) > '100b' if VUSB_HS_PHY_TYPE = 10, 11 UTMI for HSIC PHY Ok, this seems to match my assumption, except that our controller marks the 'ULPI DDR' setting as reserved. Sascha
diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt index 5778b9c..dd42ccd 100644 --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt @@ -5,6 +5,11 @@ Required properties: - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +Recommended properies: +- phy_type: the type of the phy connected to the core. Should be one + of "utmi", "utmi_wide", "ulpi", "serial" or "hsic". Without this + property the PORTSC register won't be touched + Optional properties: - fsl,usbphy: phandler of usb phy that connects to the only one port - fsl,usbmisc: phandler of non-core register device, with one argument diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h index 050de85..4817fd8 100644 --- a/drivers/usb/chipidea/bits.h +++ b/drivers/usb/chipidea/bits.h @@ -48,6 +48,13 @@ #define PORTSC_SUSP BIT(7) #define PORTSC_HSP BIT(9) #define PORTSC_PTC (0x0FUL << 16) +/* PTS and PTW for non lpm version only */ +#define PORTSC_PTS_PTW (BIT(31) | BIT(30) | BIT(28) | BIT(25)) +#define PORTSC_PTS_PTW_UTMI 0 +#define PORTSC_PTS_PTW_HSIC BIT(25) +#define PORTSC_PTS_PTW_UTMIW BIT(28) +#define PORTSC_PTS_PTW_ULPI BIT(31) +#define PORTSC_PTS_PTW_SERIAL (BIT(30) | BIT(31)) /* DEVLC */ #define DEVLC_PSPD (0x03UL << 25) diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c index 69024e0..ebc1148 100644 --- a/drivers/usb/chipidea/ci13xxx_imx.c +++ b/drivers/usb/chipidea/ci13xxx_imx.c @@ -21,6 +21,7 @@ #include <linux/clk.h> #include <linux/regulator/consumer.h> #include <linux/pinctrl/consumer.h> +#include <linux/usb/of.h> #include "ci.h" #include "ci13xxx_imx.h" @@ -112,6 +113,8 @@ static int ci13xxx_imx_probe(struct platform_device *pdev) CI13XXX_PULLUP_ON_VBUS | CI13XXX_DISABLE_STREAMING; + pdata->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node); + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); if (!data) { dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n"); diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 57cae1f..dcb650f 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -67,6 +67,8 @@ #include <linux/usb/gadget.h> #include <linux/usb/otg.h> #include <linux/usb/chipidea.h> +#include <linux/usb/of.h> +#include <linux/phy.h> #include "ci.h" #include "udc.h" @@ -211,6 +213,42 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base) return 0; } +static void hw_phymode_configure(struct ci13xxx *ci) +{ + u32 portsc; + + /* + * The lpm version has the corresponding bits in the devlc register. + * Currently not implemented. + */ + if (ci->hw_bank.lpm) + return; + + switch (ci->platdata->phy_mode) { + case USBPHY_INTERFACE_MODE_UTMI: + portsc = PORTSC_PTS_PTW_UTMI; + break; + case USBPHY_INTERFACE_MODE_UTMIW: + portsc = PORTSC_PTS_PTW_UTMIW; + break; + case USBPHY_INTERFACE_MODE_ULPI: + portsc = PORTSC_PTS_PTW_ULPI; + break; + case USBPHY_INTERFACE_MODE_SERIAL: + portsc = PORTSC_PTS_PTW_SERIAL; + break; + case USBPHY_INTERFACE_MODE_HSIC: + portsc = PORTSC_PTS_PTW_HSIC; + break; + default: + return; + } + + hw_write(ci, OP_PORTSC, PORTSC_PTS_PTW, portsc); + + mdelay(10); +} + /** * hw_device_reset: resets chip (execute without interruption) * @ci: the controller @@ -476,6 +514,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) : CI_ROLE_GADGET; } + hw_phymode_configure(ci); + ret = ci_role_start(ci, ci->role); if (ret) { dev_err(dev, "can't start %s role\n", ci_role(ci)->name); diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h index 544825d..1a2aa18 100644 --- a/include/linux/usb/chipidea.h +++ b/include/linux/usb/chipidea.h @@ -14,6 +14,7 @@ struct ci13xxx_platform_data { uintptr_t capoffset; unsigned power_budget; struct usb_phy *phy; + enum usb_phy_interface phy_mode; unsigned long flags; #define CI13XXX_REGS_SHARED BIT(0) #define CI13XXX_REQUIRE_TRANSCEIVER BIT(1)