Re: [PATCH 1/2] USB: PHY: Add support for USB 3.0 phy for exynos5250
Hi, On Wed, Nov 7, 2012 at 5:10 AM, Sylwester Nawrocki sylvester.nawro...@gmail.com wrote: Hi, I have a few comments. Please see below... On 11/06/2012 04:36 PM, Vivek Gautam wrote: Adding support for USB3.0 phy for dwc3 controller on exynso5250 SOC. exynso - exynos Sure, will correct this. Signed-off-by: Vivek Gautamgautam.vi...@samsung.com --- drivers/usb/phy/samsung-usbphy.c| 337 +++ include/linux/usb/samsung_usb_phy.h |1 + 2 files changed, 338 insertions(+), 0 deletions(-) diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c index 3b4863d..e3b5fb1 100644 --- a/drivers/usb/phy/samsung-usbphy.c +++ b/drivers/usb/phy/samsung-usbphy.c @@ -167,6 +167,99 @@ #define EXYNOS5_PHY_OTG_TUNE (0x40) +/* USB 3.0: DRD */ +#define EXYNOS5_DRD_LINKSYSTEM (0x04) + +#define LINKSYSTEM_FLADJ_MASK (0x3f 1) +#define LINKSYSTEM_FLADJ(_x) ((_x) 1) +#define LINKSYSTEM_XHCI_VERSION_CONTROL(1 27) + +#define EXYNOS5_DRD_PHYUTMI(0x08) + +#define PHYUTMI_OTGDISABLE (1 6) +#define PHYUTMI_FORCESUSPEND (1 1) +#define PHYUTMI_FORCESLEEP (1 0) + +#define EXYNOS5_DRD_PHYPIPE(0x0C) Would be nice to put all hex numbers in lower case. Sure, will put the hex numbers in sync. + +#define EXYNOS5_DRD_PHYCLKRST (0x10) + +#define PHYCLKRST_SSC_REFCLKSEL_MASK (0xff 23) +#define PHYCLKRST_SSC_REFCLKSEL(_x)((_x) 23) + +#define PHYCLKRST_SSC_RANGE_MASK (0x03 21) +#define PHYCLKRST_SSC_RANGE(_x)((_x) 21) + +#define PHYCLKRST_SSC_EN (1 20) +#define PHYCLKRST_REF_SSP_EN (1 19) +#define PHYCLKRST_REF_CLKDIV2 (1 18) + +#define PHYCLKRST_MPLL_MULTIPLIER_MASK (0x7f 11) +#define PHYCLKRST_MPLL_MULTIPLIER(_x) ((_x) 11) Is this macro-definition going to be used anywhere else except the 5 definitions below ? Is this really helpful ? In anything else than forcing you to use questionable line breaking below ? +#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF \ How about simply defining it as #define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF(0x19 11) ? Yes, we can write the way as suggested. Will amend this. + PHYCLKRST_MPLL_MULTIPLIER(0x19) +#define PHYCLKRST_MPLL_MULTIPLIER_50M_REF \ + PHYCLKRST_MPLL_MULTIPLIER(0x02) +#define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF\ + PHYCLKRST_MPLL_MULTIPLIER(0x68) +#define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF\ + PHYCLKRST_MPLL_MULTIPLIER(0x7d) +#define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF \ + PHYCLKRST_MPLL_MULTIPLIER(0x02) + +#define PHYCLKRST_FSEL_MASK(0x3f 5) +#define PHYCLKRST_FSEL(_x) ((_x) 5) Ditto. Will amend this. +#define PHYCLKRST_FSEL_PAD_100MHZ \ + PHYCLKRST_FSEL(0x27) +#define PHYCLKRST_FSEL_PAD_24MHZ \ + PHYCLKRST_FSEL(0x2a) +#define PHYCLKRST_FSEL_PAD_20MHZ \ + PHYCLKRST_FSEL(0x31) +#define PHYCLKRST_FSEL_PAD_19_2MHZ \ + PHYCLKRST_FSEL(0x38) + +#define PHYCLKRST_RETENABLEN (1 4) + +#define PHYCLKRST_REFCLKSEL_MASK (0x03 2) +#define PHYCLKRST_REFCLKSEL(_x)((_x) 2) Ditto. Will amend this. +#define PHYCLKRST_REFCLKSEL_PAD_REFCLK \ + PHYCLKRST_REFCLKSEL(2) +#define PHYCLKRST_REFCLKSEL_EXT_REFCLK \ + PHYCLKRST_REFCLKSEL(3) + +#define PHYCLKRST_PORTRESET(1 1) +#define PHYCLKRST_COMMONONN(1 0) + +#define EXYNOS5_DRD_PHYREG0(0x14) +#define EXYNOS5_DRD_PHYREG1(0x18) + +#define EXYNOS5_DRD_PHYPARAM0 (0x1C) + +#define PHYPARAM0_REF_USE_PAD (0x1 31) +#define PHYPARAM0_REF_LOSLEVEL_MASK(0x1f 26) +#define PHYPARAM0_REF_LOSLEVEL (0x9 26) + +#define EXYNOS5_DRD_PHYPARAM1 (0x20) + +#define PHYPARAM1_PCS_TXDEEMPH_MASK(0x1f 0) +#define PHYPARAM1_PCS_TXDEEMPH (0x1C) + +#define EXYNOS5_DRD_PHYTERM(0x24) + +#define EXYNOS5_DRD_PHYTEST(0x28) + +#define PHYTEST_POWERDOWN_SSP (1 3) +#define PHYTEST_POWERDOWN_HSP (1 2) + +#define EXYNOS5_DRD_PHYADP (0x2C) + +#define EXYNOS5_DRD_PHYBATCHG (0x30) + +#define PHYBATCHG_UTMI_CLKSEL (0x1 2) + +#define EXYNOS5_DRD_PHYRESUME (0x34) +#define
Re: [PATCH 1/2] USB: PHY: Add support for USB 3.0 phy for exynos5250
On 11/07/2012 02:35 PM, Vivek Gautam wrote:
@@ -180,10 +273,12 @@ enum samsung_cpu_type {
/*
* struct samsung_usbphy - transceiver driver state
* @phy: transceiver structure
+ * @phy3: transceiver structure for USB 3.0
* @plat: platform data
* @dev: The parent device supplied to the probe function
* @clk: usb phy clock
* @regs: usb phy register memory base
+ * @regs_phy3: usb 3.0 phy register memory base
* @ref_clk_freq: reference clock frequency selection
* @cpu_type: machine identifier
* @phy_type: It keeps track of the PHY type.
@@ -191,10 +286,12 @@ enum samsung_cpu_type {
*/
struct samsung_usbphy {
struct usb_phy phy;
+ struct usb_phy phy3;
struct samsung_usbphy_data *plat;
struct device *dev;
struct clk *clk;
void __iomem*regs;
+ void __iomem*regs_phy3;
Wouldn't it be better to create a new data structure for USB 3.0
and embedding it here, rather than adding multiple fields with 3
suffix ? E.g.
struct {
void __iomem*phy_regs;
struct usb_phy phy;
} usb3;
?
Yes, thanks for suggesting. This way things will look clearer.
Will update this.
And why do you need to duplicate those fields in first place ?
I guess phy and phy3 are dependant and you can't register 2 PHYs
separately ?
Controllers like DWC3 needs two different PHYs. One of USB2 type and
one of USB3 type. So we needed to register two separate PHYs.
OK, I was just wondering if there is any dependency between those two phys
so you need to aggregate them in one struct samsung_usbphy, rather than
creating two separate struct samsung_usbphy objects for them.
+/*
+ * The function passed to the usb driver for phy initialization
+ */
static int samsung_usbphy_init(struct usb_phy *phy)
{
struct samsung_usbphy *sphy;
@@ -570,6 +872,8 @@ static int __devinit samsung_usbphy_probe(struct
platform_device *pdev)
struct device *dev =pdev-dev;
struct resource *phy_mem;
void __iomem*phy_base;
+ struct resource *phy3_mem;
+ void __iomem*phy3_base = NULL;
struct clk *clk;
int ret = 0;
@@ -618,7 +922,38 @@ static int __devinit samsung_usbphy_probe(struct
platform_device *pdev)
sphy-clk = clk;
+ if (sphy-cpu_type == TYPE_EXYNOS5250) {
+ phy3_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!phy3_mem) {
+ dev_err(dev, %s: missing mem resource\n, __func__);
+ return -ENODEV;
+ }
+
+ phy3_base = devm_request_and_ioremap(dev, phy3_mem);
+ if (!phy3_base) {
+ dev_err(dev, %s: register mapping failed\n, __func__);
+ return -ENXIO;
+ }
+ }
+
+ sphy-regs_phy3 = phy3_base;
+ sphy-phy3.dev = sphy-dev;
+ sphy-phy3.label= samsung-usbphy3;
+ sphy-phy3.init = samsung_usbphy3_init;
+ sphy-phy3.shutdown = samsung_usbphy3_shutdown;
+
ret = usb_add_phy(sphy-phy, USB_PHY_TYPE_USB2);
+ if (ret)
+ return ret;
+
+ if (sphy-cpu_type != TYPE_EXYNOS5250) {
+ dev_warn(dev, Not a valid cpu_type for USB 3.0\n);
+ } else {
+ ret = usb_add_phy(sphy-phy3, USB_PHY_TYPE_USB3);
+ if (ret)
+ return ret;
2 redundant lines here.
Will two returns under if return not error codes ? The last return
actually returns success.
If still it needs modification, will do that.
It's up to you how you structure it. The last return returns whatever
value ret has. I can't see what is an advantage of doing something like:
if (ret)
return ret;
return ret;
--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] USB: PHY: Add support for USB 3.0 phy for exynos5250
Adding support for USB3.0 phy for dwc3 controller on
exynso5250 SOC.
Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
---
drivers/usb/phy/samsung-usbphy.c| 337 +++
include/linux/usb/samsung_usb_phy.h |1 +
2 files changed, 338 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c
index 3b4863d..e3b5fb1 100644
--- a/drivers/usb/phy/samsung-usbphy.c
+++ b/drivers/usb/phy/samsung-usbphy.c
@@ -167,6 +167,99 @@
#define EXYNOS5_PHY_OTG_TUNE (0x40)
+/* USB 3.0: DRD */
+#define EXYNOS5_DRD_LINKSYSTEM (0x04)
+
+#define LINKSYSTEM_FLADJ_MASK (0x3f 1)
+#define LINKSYSTEM_FLADJ(_x) ((_x) 1)
+#define LINKSYSTEM_XHCI_VERSION_CONTROL(1 27)
+
+#define EXYNOS5_DRD_PHYUTMI(0x08)
+
+#define PHYUTMI_OTGDISABLE (1 6)
+#define PHYUTMI_FORCESUSPEND (1 1)
+#define PHYUTMI_FORCESLEEP (1 0)
+
+#define EXYNOS5_DRD_PHYPIPE(0x0C)
+
+#define EXYNOS5_DRD_PHYCLKRST (0x10)
+
+#define PHYCLKRST_SSC_REFCLKSEL_MASK (0xff 23)
+#define PHYCLKRST_SSC_REFCLKSEL(_x)((_x) 23)
+
+#define PHYCLKRST_SSC_RANGE_MASK (0x03 21)
+#define PHYCLKRST_SSC_RANGE(_x)((_x) 21)
+
+#define PHYCLKRST_SSC_EN (1 20)
+#define PHYCLKRST_REF_SSP_EN (1 19)
+#define PHYCLKRST_REF_CLKDIV2 (1 18)
+
+#define PHYCLKRST_MPLL_MULTIPLIER_MASK (0x7f 11)
+#define PHYCLKRST_MPLL_MULTIPLIER(_x) ((_x) 11)
+#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF \
+ PHYCLKRST_MPLL_MULTIPLIER(0x19)
+#define PHYCLKRST_MPLL_MULTIPLIER_50M_REF \
+ PHYCLKRST_MPLL_MULTIPLIER(0x02)
+#define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF\
+ PHYCLKRST_MPLL_MULTIPLIER(0x68)
+#define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF\
+ PHYCLKRST_MPLL_MULTIPLIER(0x7d)
+#define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF \
+ PHYCLKRST_MPLL_MULTIPLIER(0x02)
+
+#define PHYCLKRST_FSEL_MASK(0x3f 5)
+#define PHYCLKRST_FSEL(_x) ((_x) 5)
+#define PHYCLKRST_FSEL_PAD_100MHZ \
+ PHYCLKRST_FSEL(0x27)
+#define PHYCLKRST_FSEL_PAD_24MHZ \
+ PHYCLKRST_FSEL(0x2a)
+#define PHYCLKRST_FSEL_PAD_20MHZ \
+ PHYCLKRST_FSEL(0x31)
+#define PHYCLKRST_FSEL_PAD_19_2MHZ \
+ PHYCLKRST_FSEL(0x38)
+
+#define PHYCLKRST_RETENABLEN (1 4)
+
+#define PHYCLKRST_REFCLKSEL_MASK (0x03 2)
+#define PHYCLKRST_REFCLKSEL(_x)((_x) 2)
+#define PHYCLKRST_REFCLKSEL_PAD_REFCLK \
+ PHYCLKRST_REFCLKSEL(2)
+#define PHYCLKRST_REFCLKSEL_EXT_REFCLK \
+ PHYCLKRST_REFCLKSEL(3)
+
+#define PHYCLKRST_PORTRESET(1 1)
+#define PHYCLKRST_COMMONONN(1 0)
+
+#define EXYNOS5_DRD_PHYREG0(0x14)
+#define EXYNOS5_DRD_PHYREG1(0x18)
+
+#define EXYNOS5_DRD_PHYPARAM0 (0x1C)
+
+#define PHYPARAM0_REF_USE_PAD (0x1 31)
+#define PHYPARAM0_REF_LOSLEVEL_MASK(0x1f 26)
+#define PHYPARAM0_REF_LOSLEVEL (0x9 26)
+
+#define EXYNOS5_DRD_PHYPARAM1 (0x20)
+
+#define PHYPARAM1_PCS_TXDEEMPH_MASK(0x1f 0)
+#define PHYPARAM1_PCS_TXDEEMPH (0x1C)
+
+#define EXYNOS5_DRD_PHYTERM(0x24)
+
+#define EXYNOS5_DRD_PHYTEST(0x28)
+
+#define PHYTEST_POWERDOWN_SSP (1 3)
+#define PHYTEST_POWERDOWN_HSP (1 2)
+
+#define EXYNOS5_DRD_PHYADP (0x2C)
+
+#define EXYNOS5_DRD_PHYBATCHG (0x30)
+
+#define PHYBATCHG_UTMI_CLKSEL (0x1 2)
+
+#define EXYNOS5_DRD_PHYRESUME (0x34)
+#define EXYNOS5_DRD_LINKPORT (0x44)
+
#ifndef MHZ
#define MHZ (1000*1000)
#endif
@@ -180,10 +273,12 @@ enum samsung_cpu_type {
/*
* struct samsung_usbphy - transceiver driver state
* @phy: transceiver structure
+ * @phy3: transceiver structure for USB 3.0
* @plat: platform data
* @dev: The parent device supplied to the probe function
* @clk: usb phy clock
* @regs: usb phy register memory base
+ * @regs_phy3: usb 3.0 phy register memory base
* @ref_clk_freq: reference clock frequency selection
* @cpu_type: machine identifier
* @phy_type: It keeps track of the PHY type.
@@ -191,10 +286,12 @@ enum samsung_cpu_type {
*/
struct samsung_usbphy {
struct usb_phy phy;
+ struct usb_phy phy3;
struct samsung_usbphy_data *plat;
struct device *dev;
struct clk *clk;
void __iomem
Re: [PATCH 1/2] USB: PHY: Add support for USB 3.0 phy for exynos5250
Hi,
I have a few comments. Please see below...
On 11/06/2012 04:36 PM, Vivek Gautam wrote:
Adding support for USB3.0 phy for dwc3 controller on
exynso5250 SOC.
exynso - exynos
Signed-off-by: Vivek Gautamgautam.vi...@samsung.com
---
drivers/usb/phy/samsung-usbphy.c| 337 +++
include/linux/usb/samsung_usb_phy.h |1 +
2 files changed, 338 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c
index 3b4863d..e3b5fb1 100644
--- a/drivers/usb/phy/samsung-usbphy.c
+++ b/drivers/usb/phy/samsung-usbphy.c
@@ -167,6 +167,99 @@
#define EXYNOS5_PHY_OTG_TUNE (0x40)
+/* USB 3.0: DRD */
+#define EXYNOS5_DRD_LINKSYSTEM (0x04)
+
+#define LINKSYSTEM_FLADJ_MASK (0x3f 1)
+#define LINKSYSTEM_FLADJ(_x) ((_x) 1)
+#define LINKSYSTEM_XHCI_VERSION_CONTROL(1 27)
+
+#define EXYNOS5_DRD_PHYUTMI(0x08)
+
+#define PHYUTMI_OTGDISABLE (1 6)
+#define PHYUTMI_FORCESUSPEND (1 1)
+#define PHYUTMI_FORCESLEEP (1 0)
+
+#define EXYNOS5_DRD_PHYPIPE(0x0C)
Would be nice to put all hex numbers in lower case.
+
+#define EXYNOS5_DRD_PHYCLKRST (0x10)
+
+#define PHYCLKRST_SSC_REFCLKSEL_MASK (0xff 23)
+#define PHYCLKRST_SSC_REFCLKSEL(_x)((_x) 23)
+
+#define PHYCLKRST_SSC_RANGE_MASK (0x03 21)
+#define PHYCLKRST_SSC_RANGE(_x)((_x) 21)
+
+#define PHYCLKRST_SSC_EN (1 20)
+#define PHYCLKRST_REF_SSP_EN (1 19)
+#define PHYCLKRST_REF_CLKDIV2 (1 18)
+
+#define PHYCLKRST_MPLL_MULTIPLIER_MASK (0x7f 11)
+#define PHYCLKRST_MPLL_MULTIPLIER(_x) ((_x) 11)
Is this macro-definition going to be used anywhere else except the
5 definitions below ? Is this really helpful ? In anything else than
forcing you to use questionable line breaking below ?
+#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF \
How about simply defining it as
#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF(0x19 11)
?
+ PHYCLKRST_MPLL_MULTIPLIER(0x19)
+#define PHYCLKRST_MPLL_MULTIPLIER_50M_REF \
+ PHYCLKRST_MPLL_MULTIPLIER(0x02)
+#define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF\
+ PHYCLKRST_MPLL_MULTIPLIER(0x68)
+#define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF\
+ PHYCLKRST_MPLL_MULTIPLIER(0x7d)
+#define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF \
+ PHYCLKRST_MPLL_MULTIPLIER(0x02)
+
+#define PHYCLKRST_FSEL_MASK(0x3f 5)
+#define PHYCLKRST_FSEL(_x) ((_x) 5)
Ditto.
+#define PHYCLKRST_FSEL_PAD_100MHZ \
+ PHYCLKRST_FSEL(0x27)
+#define PHYCLKRST_FSEL_PAD_24MHZ \
+ PHYCLKRST_FSEL(0x2a)
+#define PHYCLKRST_FSEL_PAD_20MHZ \
+ PHYCLKRST_FSEL(0x31)
+#define PHYCLKRST_FSEL_PAD_19_2MHZ \
+ PHYCLKRST_FSEL(0x38)
+
+#define PHYCLKRST_RETENABLEN (1 4)
+
+#define PHYCLKRST_REFCLKSEL_MASK (0x03 2)
+#define PHYCLKRST_REFCLKSEL(_x)((_x) 2)
Ditto.
+#define PHYCLKRST_REFCLKSEL_PAD_REFCLK \
+ PHYCLKRST_REFCLKSEL(2)
+#define PHYCLKRST_REFCLKSEL_EXT_REFCLK \
+ PHYCLKRST_REFCLKSEL(3)
+
+#define PHYCLKRST_PORTRESET(1 1)
+#define PHYCLKRST_COMMONONN(1 0)
+
+#define EXYNOS5_DRD_PHYREG0(0x14)
+#define EXYNOS5_DRD_PHYREG1(0x18)
+
+#define EXYNOS5_DRD_PHYPARAM0 (0x1C)
+
+#define PHYPARAM0_REF_USE_PAD (0x1 31)
+#define PHYPARAM0_REF_LOSLEVEL_MASK(0x1f 26)
+#define PHYPARAM0_REF_LOSLEVEL (0x9 26)
+
+#define EXYNOS5_DRD_PHYPARAM1 (0x20)
+
+#define PHYPARAM1_PCS_TXDEEMPH_MASK(0x1f 0)
+#define PHYPARAM1_PCS_TXDEEMPH (0x1C)
+
+#define EXYNOS5_DRD_PHYTERM(0x24)
+
+#define EXYNOS5_DRD_PHYTEST(0x28)
+
+#define PHYTEST_POWERDOWN_SSP (1 3)
+#define PHYTEST_POWERDOWN_HSP (1 2)
+
+#define EXYNOS5_DRD_PHYADP (0x2C)
+
+#define EXYNOS5_DRD_PHYBATCHG (0x30)
+
+#define PHYBATCHG_UTMI_CLKSEL (0x1 2)
+
+#define EXYNOS5_DRD_PHYRESUME (0x34)
+#define EXYNOS5_DRD_LINKPORT (0x44)
+
#ifndef MHZ
#define MHZ (1000*1000)
#endif
@@ -180,10 +273,12 @@ enum samsung_cpu_type {
/*
* struct samsung_usbphy - transceiver driver state
* @phy: transceiver structure
+ * @phy3: transceiver structure for USB 3.0
* @plat: platform data
* @dev: The parent device supplied to the probe function
* @clk: usb phy
