All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs
@ 2017-04-02 16:55 ` Rafał Miłecki
  0 siblings, 0 replies; 10+ messages in thread
From: Rafał Miłecki @ 2017-04-02 16:55 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Yendapally Reddy Dhananjaya Reddy, Jon Mason, Florian Fainelli,
	linux-arm-kernel, bcm-kernel-feedback-list, linux-kernel,
	Rafał Miłecki

From: Rafał Miłecki <rafal@milecki.pl>

So far all the PHY initialization was implemented using some totally
magic values. There was some pattern there but it wasn't clear what is
it about.

Thanks to the patch submitted by Broadcom:
[PATCH 5/6] phy: Add USB3 PHY support for Broadcom NSP SoC
and the upstream "iproc-mdio" driver we now know there is a MDIO bus
underneath with PHY(s) and their registers.

It allows us to clean the driver a bit by making all these values less
magical. The next step is switching to using a proper MDIO layer.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/phy/phy-bcm-ns-usb3.c | 69 ++++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/phy/phy-bcm-ns-usb3.c b/drivers/phy/phy-bcm-ns-usb3.c
index f420fa4bebfc..22b5e7047fa6 100644
--- a/drivers/phy/phy-bcm-ns-usb3.c
+++ b/drivers/phy/phy-bcm-ns-usb3.c
@@ -2,6 +2,7 @@
  * Broadcom Northstar USB 3.0 PHY Driver
  *
  * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
+ * Copyright (C) 2016 Broadcom
  *
  * All magic values used for initialization (and related comments) were obtained
  * from Broadcom's SDK:
@@ -23,6 +24,23 @@
 
 #define BCM_NS_USB3_MII_MNG_TIMEOUT_US	1000	/* usecs */
 
+#define BCM_NS_USB3_PHY_BASE_ADDR_REG	0x1f
+#define BCM_NS_USB3_PHY_PLL30_BLOCK	0x8000
+#define BCM_NS_USB3_PHY_TX_PMD_BLOCK	0x8040
+#define BCM_NS_USB3_PHY_PIPE_BLOCK	0x8060
+
+/* Registers of PLL30 block */
+#define BCM_NS_USB3_PLL_CONTROL		0x01
+#define BCM_NS_USB3_PLLA_CONTROL0	0x0a
+#define BCM_NS_USB3_PLLA_CONTROL1	0x0b
+
+/* Registers of TX PMD block */
+#define BCM_NS_USB3_TX_PMD_CONTROL1	0x01
+
+/* Registers of PIPE block */
+#define BCM_NS_USB3_LFPS_CMP		0x02
+#define BCM_NS_USB3_LFPS_DEGLITCH	0x03
+
 enum bcm_ns_family {
 	BCM_NS_UNKNOWN,
 	BCM_NS_AX,
@@ -76,8 +94,10 @@ static inline int bcm_ns_usb3_mii_mng_wait_idle(struct bcm_ns_usb3 *usb3)
 				    usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
 }
 
-static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
+static int bcm_ns_usb3_mdio_phy_write(struct bcm_ns_usb3 *usb3, u16 reg,
+				      u16 value)
 {
+	u32 tmp = 0;
 	int err;
 
 	err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
@@ -86,7 +106,11 @@ static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
 		return err;
 	}
 
-	writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
+	/* TODO: Use a proper MDIO bus layer */
+	tmp |= 0x58020000; /* Magic value for MDIO PHY write */
+	tmp |= reg << 18;
+	tmp |= value;
+	writel(tmp, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
 
 	return 0;
 }
@@ -102,21 +126,22 @@ static int bcm_ns_usb3_phy_init_ns_bx(struct bcm_ns_usb3 *usb3)
 	udelay(2);
 
 	/* USB3 PLL Block */
-	err = bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8000);
+	err = bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
+					 BCM_NS_USB3_PHY_PLL30_BLOCK);
 	if (err < 0)
 		return err;
 
 	/* Assert Ana_Pllseq start */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x58061000);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLL_CONTROL, 0x1000);
 
 	/* Assert CML Divider ratio to 26 */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x582a6400);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL0, 0x6400);
 
 	/* Asserting PLL Reset */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x582ec000);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL1, 0xc000);
 
 	/* Deaaserting PLL Reset */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x582e8000);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL1, 0x8000);
 
 	/* Waiting MII Mgt interface idle */
 	bcm_ns_usb3_mii_mng_wait_idle(usb3);
@@ -125,22 +150,24 @@ static int bcm_ns_usb3_phy_init_ns_bx(struct bcm_ns_usb3 *usb3)
 	writel(0, usb3->dmp + BCMA_RESET_CTL);
 
 	/* PLL frequency monitor enable */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x58069000);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLL_CONTROL, 0x9000);
 
 	/* PIPE Block */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8060);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
+				   BCM_NS_USB3_PHY_PIPE_BLOCK);
 
 	/* CMPMAX & CMPMINTH setting */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x580af30d);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_LFPS_CMP, 0xf30d);
 
 	/* DEGLITCH MIN & MAX setting */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x580e6302);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_LFPS_DEGLITCH, 0x6302);
 
 	/* TXPMD block */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8040);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
+				   BCM_NS_USB3_PHY_TX_PMD_BLOCK);
 
 	/* Enabling SSC */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x58061003);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_TX_PMD_CONTROL1, 0x1003);
 
 	/* Waiting MII Mgt interface idle */
 	bcm_ns_usb3_mii_mng_wait_idle(usb3);
@@ -159,22 +186,24 @@ static int bcm_ns_usb3_phy_init_ns_ax(struct bcm_ns_usb3 *usb3)
 	udelay(2);
 
 	/* PLL30 block */
-	err = bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8000);
+	err = bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
+					 BCM_NS_USB3_PHY_PLL30_BLOCK);
 	if (err < 0)
 		return err;
 
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x582a6400);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL0, 0x6400);
 
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x587e80e0);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG, 0x80e0);
 
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x580a009c);
+	bcm_ns_usb3_mdio_phy_write(usb3, 0x02, 0x009c);
 
 	/* Enable SSC */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8040);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
+				   BCM_NS_USB3_PHY_TX_PMD_BLOCK);
 
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x580a21d3);
+	bcm_ns_usb3_mdio_phy_write(usb3, 0x02, 0x21d3);
 
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x58061003);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_TX_PMD_CONTROL1, 0x1003);
 
 	/* Waiting MII Mgt interface idle */
 	bcm_ns_usb3_mii_mng_wait_idle(usb3);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs
@ 2017-04-02 16:55 ` Rafał Miłecki
  0 siblings, 0 replies; 10+ messages in thread
From: Rafał Miłecki @ 2017-04-02 16:55 UTC (permalink / raw)
  To: linux-arm-kernel

From: Rafa? Mi?ecki <rafal@milecki.pl>

So far all the PHY initialization was implemented using some totally
magic values. There was some pattern there but it wasn't clear what is
it about.

Thanks to the patch submitted by Broadcom:
[PATCH 5/6] phy: Add USB3 PHY support for Broadcom NSP SoC
and the upstream "iproc-mdio" driver we now know there is a MDIO bus
underneath with PHY(s) and their registers.

It allows us to clean the driver a bit by making all these values less
magical. The next step is switching to using a proper MDIO layer.

Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
---
 drivers/phy/phy-bcm-ns-usb3.c | 69 ++++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/phy/phy-bcm-ns-usb3.c b/drivers/phy/phy-bcm-ns-usb3.c
index f420fa4bebfc..22b5e7047fa6 100644
--- a/drivers/phy/phy-bcm-ns-usb3.c
+++ b/drivers/phy/phy-bcm-ns-usb3.c
@@ -2,6 +2,7 @@
  * Broadcom Northstar USB 3.0 PHY Driver
  *
  * Copyright (C) 2016 Rafa? Mi?ecki <rafal@milecki.pl>
+ * Copyright (C) 2016 Broadcom
  *
  * All magic values used for initialization (and related comments) were obtained
  * from Broadcom's SDK:
@@ -23,6 +24,23 @@
 
 #define BCM_NS_USB3_MII_MNG_TIMEOUT_US	1000	/* usecs */
 
+#define BCM_NS_USB3_PHY_BASE_ADDR_REG	0x1f
+#define BCM_NS_USB3_PHY_PLL30_BLOCK	0x8000
+#define BCM_NS_USB3_PHY_TX_PMD_BLOCK	0x8040
+#define BCM_NS_USB3_PHY_PIPE_BLOCK	0x8060
+
+/* Registers of PLL30 block */
+#define BCM_NS_USB3_PLL_CONTROL		0x01
+#define BCM_NS_USB3_PLLA_CONTROL0	0x0a
+#define BCM_NS_USB3_PLLA_CONTROL1	0x0b
+
+/* Registers of TX PMD block */
+#define BCM_NS_USB3_TX_PMD_CONTROL1	0x01
+
+/* Registers of PIPE block */
+#define BCM_NS_USB3_LFPS_CMP		0x02
+#define BCM_NS_USB3_LFPS_DEGLITCH	0x03
+
 enum bcm_ns_family {
 	BCM_NS_UNKNOWN,
 	BCM_NS_AX,
@@ -76,8 +94,10 @@ static inline int bcm_ns_usb3_mii_mng_wait_idle(struct bcm_ns_usb3 *usb3)
 				    usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
 }
 
-static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
+static int bcm_ns_usb3_mdio_phy_write(struct bcm_ns_usb3 *usb3, u16 reg,
+				      u16 value)
 {
+	u32 tmp = 0;
 	int err;
 
 	err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
@@ -86,7 +106,11 @@ static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
 		return err;
 	}
 
-	writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
+	/* TODO: Use a proper MDIO bus layer */
+	tmp |= 0x58020000; /* Magic value for MDIO PHY write */
+	tmp |= reg << 18;
+	tmp |= value;
+	writel(tmp, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
 
 	return 0;
 }
@@ -102,21 +126,22 @@ static int bcm_ns_usb3_phy_init_ns_bx(struct bcm_ns_usb3 *usb3)
 	udelay(2);
 
 	/* USB3 PLL Block */
-	err = bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8000);
+	err = bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
+					 BCM_NS_USB3_PHY_PLL30_BLOCK);
 	if (err < 0)
 		return err;
 
 	/* Assert Ana_Pllseq start */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x58061000);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLL_CONTROL, 0x1000);
 
 	/* Assert CML Divider ratio to 26 */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x582a6400);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL0, 0x6400);
 
 	/* Asserting PLL Reset */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x582ec000);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL1, 0xc000);
 
 	/* Deaaserting PLL Reset */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x582e8000);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL1, 0x8000);
 
 	/* Waiting MII Mgt interface idle */
 	bcm_ns_usb3_mii_mng_wait_idle(usb3);
@@ -125,22 +150,24 @@ static int bcm_ns_usb3_phy_init_ns_bx(struct bcm_ns_usb3 *usb3)
 	writel(0, usb3->dmp + BCMA_RESET_CTL);
 
 	/* PLL frequency monitor enable */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x58069000);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLL_CONTROL, 0x9000);
 
 	/* PIPE Block */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8060);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
+				   BCM_NS_USB3_PHY_PIPE_BLOCK);
 
 	/* CMPMAX & CMPMINTH setting */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x580af30d);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_LFPS_CMP, 0xf30d);
 
 	/* DEGLITCH MIN & MAX setting */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x580e6302);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_LFPS_DEGLITCH, 0x6302);
 
 	/* TXPMD block */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8040);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
+				   BCM_NS_USB3_PHY_TX_PMD_BLOCK);
 
 	/* Enabling SSC */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x58061003);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_TX_PMD_CONTROL1, 0x1003);
 
 	/* Waiting MII Mgt interface idle */
 	bcm_ns_usb3_mii_mng_wait_idle(usb3);
@@ -159,22 +186,24 @@ static int bcm_ns_usb3_phy_init_ns_ax(struct bcm_ns_usb3 *usb3)
 	udelay(2);
 
 	/* PLL30 block */
-	err = bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8000);
+	err = bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
+					 BCM_NS_USB3_PHY_PLL30_BLOCK);
 	if (err < 0)
 		return err;
 
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x582a6400);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL0, 0x6400);
 
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x587e80e0);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG, 0x80e0);
 
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x580a009c);
+	bcm_ns_usb3_mdio_phy_write(usb3, 0x02, 0x009c);
 
 	/* Enable SSC */
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8040);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
+				   BCM_NS_USB3_PHY_TX_PMD_BLOCK);
 
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x580a21d3);
+	bcm_ns_usb3_mdio_phy_write(usb3, 0x02, 0x21d3);
 
-	bcm_ns_usb3_mii_mng_write32(usb3, 0x58061003);
+	bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_TX_PMD_CONTROL1, 0x1003);
 
 	/* Waiting MII Mgt interface idle */
 	bcm_ns_usb3_mii_mng_wait_idle(usb3);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs
  2017-04-02 16:55 ` Rafał Miłecki
@ 2017-04-03  3:02   ` Jon Mason
  -1 siblings, 0 replies; 10+ messages in thread
From: Jon Mason @ 2017-04-03  3:02 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Kishon Vijay Abraham I, Yendapally Reddy Dhananjaya Reddy,
	Jon Mason, Florian Fainelli, linux-arm-kernel,
	BCM Kernel Feedback, open list, Rafał Miłecki

On Sun, Apr 2, 2017 at 12:55 PM, Rafał Miłecki <zajec5@gmail.com> wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> So far all the PHY initialization was implemented using some totally
> magic values. There was some pattern there but it wasn't clear what is
> it about.
>
> Thanks to the patch submitted by Broadcom:
> [PATCH 5/6] phy: Add USB3 PHY support for Broadcom NSP SoC
> and the upstream "iproc-mdio" driver we now know there is a MDIO bus
> underneath with PHY(s) and their registers.
>
> It allows us to clean the driver a bit by making all these values less
> magical. The next step is switching to using a proper MDIO layer.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Looks much better to me!

Acked-by: Jon Mason <jon.mason@broadcom.com>

> ---
>  drivers/phy/phy-bcm-ns-usb3.c | 69 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/phy/phy-bcm-ns-usb3.c b/drivers/phy/phy-bcm-ns-usb3.c
> index f420fa4bebfc..22b5e7047fa6 100644
> --- a/drivers/phy/phy-bcm-ns-usb3.c
> +++ b/drivers/phy/phy-bcm-ns-usb3.c
> @@ -2,6 +2,7 @@
>   * Broadcom Northstar USB 3.0 PHY Driver
>   *
>   * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
> + * Copyright (C) 2016 Broadcom
>   *
>   * All magic values used for initialization (and related comments) were obtained
>   * from Broadcom's SDK:
> @@ -23,6 +24,23 @@
>
>  #define BCM_NS_USB3_MII_MNG_TIMEOUT_US 1000    /* usecs */
>
> +#define BCM_NS_USB3_PHY_BASE_ADDR_REG  0x1f
> +#define BCM_NS_USB3_PHY_PLL30_BLOCK    0x8000
> +#define BCM_NS_USB3_PHY_TX_PMD_BLOCK   0x8040
> +#define BCM_NS_USB3_PHY_PIPE_BLOCK     0x8060
> +
> +/* Registers of PLL30 block */
> +#define BCM_NS_USB3_PLL_CONTROL                0x01
> +#define BCM_NS_USB3_PLLA_CONTROL0      0x0a
> +#define BCM_NS_USB3_PLLA_CONTROL1      0x0b
> +
> +/* Registers of TX PMD block */
> +#define BCM_NS_USB3_TX_PMD_CONTROL1    0x01
> +
> +/* Registers of PIPE block */
> +#define BCM_NS_USB3_LFPS_CMP           0x02
> +#define BCM_NS_USB3_LFPS_DEGLITCH      0x03
> +
>  enum bcm_ns_family {
>         BCM_NS_UNKNOWN,
>         BCM_NS_AX,
> @@ -76,8 +94,10 @@ static inline int bcm_ns_usb3_mii_mng_wait_idle(struct bcm_ns_usb3 *usb3)
>                                     usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
>  }
>
> -static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
> +static int bcm_ns_usb3_mdio_phy_write(struct bcm_ns_usb3 *usb3, u16 reg,
> +                                     u16 value)
>  {
> +       u32 tmp = 0;
>         int err;
>
>         err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
> @@ -86,7 +106,11 @@ static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
>                 return err;
>         }
>
> -       writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
> +       /* TODO: Use a proper MDIO bus layer */
> +       tmp |= 0x58020000; /* Magic value for MDIO PHY write */
> +       tmp |= reg << 18;
> +       tmp |= value;
> +       writel(tmp, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
>
>         return 0;
>  }
> @@ -102,21 +126,22 @@ static int bcm_ns_usb3_phy_init_ns_bx(struct bcm_ns_usb3 *usb3)
>         udelay(2);
>
>         /* USB3 PLL Block */
> -       err = bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8000);
> +       err = bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
> +                                        BCM_NS_USB3_PHY_PLL30_BLOCK);
>         if (err < 0)
>                 return err;
>
>         /* Assert Ana_Pllseq start */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x58061000);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLL_CONTROL, 0x1000);
>
>         /* Assert CML Divider ratio to 26 */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x582a6400);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL0, 0x6400);
>
>         /* Asserting PLL Reset */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x582ec000);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL1, 0xc000);
>
>         /* Deaaserting PLL Reset */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x582e8000);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL1, 0x8000);
>
>         /* Waiting MII Mgt interface idle */
>         bcm_ns_usb3_mii_mng_wait_idle(usb3);
> @@ -125,22 +150,24 @@ static int bcm_ns_usb3_phy_init_ns_bx(struct bcm_ns_usb3 *usb3)
>         writel(0, usb3->dmp + BCMA_RESET_CTL);
>
>         /* PLL frequency monitor enable */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x58069000);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLL_CONTROL, 0x9000);
>
>         /* PIPE Block */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8060);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
> +                                  BCM_NS_USB3_PHY_PIPE_BLOCK);
>
>         /* CMPMAX & CMPMINTH setting */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x580af30d);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_LFPS_CMP, 0xf30d);
>
>         /* DEGLITCH MIN & MAX setting */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x580e6302);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_LFPS_DEGLITCH, 0x6302);
>
>         /* TXPMD block */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8040);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
> +                                  BCM_NS_USB3_PHY_TX_PMD_BLOCK);
>
>         /* Enabling SSC */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x58061003);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_TX_PMD_CONTROL1, 0x1003);
>
>         /* Waiting MII Mgt interface idle */
>         bcm_ns_usb3_mii_mng_wait_idle(usb3);
> @@ -159,22 +186,24 @@ static int bcm_ns_usb3_phy_init_ns_ax(struct bcm_ns_usb3 *usb3)
>         udelay(2);
>
>         /* PLL30 block */
> -       err = bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8000);
> +       err = bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
> +                                        BCM_NS_USB3_PHY_PLL30_BLOCK);
>         if (err < 0)
>                 return err;
>
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x582a6400);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL0, 0x6400);
>
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x587e80e0);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG, 0x80e0);
>
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x580a009c);
> +       bcm_ns_usb3_mdio_phy_write(usb3, 0x02, 0x009c);
>
>         /* Enable SSC */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8040);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
> +                                  BCM_NS_USB3_PHY_TX_PMD_BLOCK);
>
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x580a21d3);
> +       bcm_ns_usb3_mdio_phy_write(usb3, 0x02, 0x21d3);
>
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x58061003);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_TX_PMD_CONTROL1, 0x1003);
>
>         /* Waiting MII Mgt interface idle */
>         bcm_ns_usb3_mii_mng_wait_idle(usb3);
> --
> 2.11.0
>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs
@ 2017-04-03  3:02   ` Jon Mason
  0 siblings, 0 replies; 10+ messages in thread
From: Jon Mason @ 2017-04-03  3:02 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, Apr 2, 2017 at 12:55 PM, Rafa? Mi?ecki <zajec5@gmail.com> wrote:
> From: Rafa? Mi?ecki <rafal@milecki.pl>
>
> So far all the PHY initialization was implemented using some totally
> magic values. There was some pattern there but it wasn't clear what is
> it about.
>
> Thanks to the patch submitted by Broadcom:
> [PATCH 5/6] phy: Add USB3 PHY support for Broadcom NSP SoC
> and the upstream "iproc-mdio" driver we now know there is a MDIO bus
> underneath with PHY(s) and their registers.
>
> It allows us to clean the driver a bit by making all these values less
> magical. The next step is switching to using a proper MDIO layer.
>
> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>

Looks much better to me!

Acked-by: Jon Mason <jon.mason@broadcom.com>

> ---
>  drivers/phy/phy-bcm-ns-usb3.c | 69 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/phy/phy-bcm-ns-usb3.c b/drivers/phy/phy-bcm-ns-usb3.c
> index f420fa4bebfc..22b5e7047fa6 100644
> --- a/drivers/phy/phy-bcm-ns-usb3.c
> +++ b/drivers/phy/phy-bcm-ns-usb3.c
> @@ -2,6 +2,7 @@
>   * Broadcom Northstar USB 3.0 PHY Driver
>   *
>   * Copyright (C) 2016 Rafa? Mi?ecki <rafal@milecki.pl>
> + * Copyright (C) 2016 Broadcom
>   *
>   * All magic values used for initialization (and related comments) were obtained
>   * from Broadcom's SDK:
> @@ -23,6 +24,23 @@
>
>  #define BCM_NS_USB3_MII_MNG_TIMEOUT_US 1000    /* usecs */
>
> +#define BCM_NS_USB3_PHY_BASE_ADDR_REG  0x1f
> +#define BCM_NS_USB3_PHY_PLL30_BLOCK    0x8000
> +#define BCM_NS_USB3_PHY_TX_PMD_BLOCK   0x8040
> +#define BCM_NS_USB3_PHY_PIPE_BLOCK     0x8060
> +
> +/* Registers of PLL30 block */
> +#define BCM_NS_USB3_PLL_CONTROL                0x01
> +#define BCM_NS_USB3_PLLA_CONTROL0      0x0a
> +#define BCM_NS_USB3_PLLA_CONTROL1      0x0b
> +
> +/* Registers of TX PMD block */
> +#define BCM_NS_USB3_TX_PMD_CONTROL1    0x01
> +
> +/* Registers of PIPE block */
> +#define BCM_NS_USB3_LFPS_CMP           0x02
> +#define BCM_NS_USB3_LFPS_DEGLITCH      0x03
> +
>  enum bcm_ns_family {
>         BCM_NS_UNKNOWN,
>         BCM_NS_AX,
> @@ -76,8 +94,10 @@ static inline int bcm_ns_usb3_mii_mng_wait_idle(struct bcm_ns_usb3 *usb3)
>                                     usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
>  }
>
> -static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
> +static int bcm_ns_usb3_mdio_phy_write(struct bcm_ns_usb3 *usb3, u16 reg,
> +                                     u16 value)
>  {
> +       u32 tmp = 0;
>         int err;
>
>         err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
> @@ -86,7 +106,11 @@ static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
>                 return err;
>         }
>
> -       writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
> +       /* TODO: Use a proper MDIO bus layer */
> +       tmp |= 0x58020000; /* Magic value for MDIO PHY write */
> +       tmp |= reg << 18;
> +       tmp |= value;
> +       writel(tmp, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
>
>         return 0;
>  }
> @@ -102,21 +126,22 @@ static int bcm_ns_usb3_phy_init_ns_bx(struct bcm_ns_usb3 *usb3)
>         udelay(2);
>
>         /* USB3 PLL Block */
> -       err = bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8000);
> +       err = bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
> +                                        BCM_NS_USB3_PHY_PLL30_BLOCK);
>         if (err < 0)
>                 return err;
>
>         /* Assert Ana_Pllseq start */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x58061000);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLL_CONTROL, 0x1000);
>
>         /* Assert CML Divider ratio to 26 */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x582a6400);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL0, 0x6400);
>
>         /* Asserting PLL Reset */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x582ec000);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL1, 0xc000);
>
>         /* Deaaserting PLL Reset */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x582e8000);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL1, 0x8000);
>
>         /* Waiting MII Mgt interface idle */
>         bcm_ns_usb3_mii_mng_wait_idle(usb3);
> @@ -125,22 +150,24 @@ static int bcm_ns_usb3_phy_init_ns_bx(struct bcm_ns_usb3 *usb3)
>         writel(0, usb3->dmp + BCMA_RESET_CTL);
>
>         /* PLL frequency monitor enable */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x58069000);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLL_CONTROL, 0x9000);
>
>         /* PIPE Block */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8060);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
> +                                  BCM_NS_USB3_PHY_PIPE_BLOCK);
>
>         /* CMPMAX & CMPMINTH setting */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x580af30d);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_LFPS_CMP, 0xf30d);
>
>         /* DEGLITCH MIN & MAX setting */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x580e6302);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_LFPS_DEGLITCH, 0x6302);
>
>         /* TXPMD block */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8040);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
> +                                  BCM_NS_USB3_PHY_TX_PMD_BLOCK);
>
>         /* Enabling SSC */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x58061003);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_TX_PMD_CONTROL1, 0x1003);
>
>         /* Waiting MII Mgt interface idle */
>         bcm_ns_usb3_mii_mng_wait_idle(usb3);
> @@ -159,22 +186,24 @@ static int bcm_ns_usb3_phy_init_ns_ax(struct bcm_ns_usb3 *usb3)
>         udelay(2);
>
>         /* PLL30 block */
> -       err = bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8000);
> +       err = bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
> +                                        BCM_NS_USB3_PHY_PLL30_BLOCK);
>         if (err < 0)
>                 return err;
>
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x582a6400);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PLLA_CONTROL0, 0x6400);
>
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x587e80e0);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG, 0x80e0);
>
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x580a009c);
> +       bcm_ns_usb3_mdio_phy_write(usb3, 0x02, 0x009c);
>
>         /* Enable SSC */
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8040);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_PHY_BASE_ADDR_REG,
> +                                  BCM_NS_USB3_PHY_TX_PMD_BLOCK);
>
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x580a21d3);
> +       bcm_ns_usb3_mdio_phy_write(usb3, 0x02, 0x21d3);
>
> -       bcm_ns_usb3_mii_mng_write32(usb3, 0x58061003);
> +       bcm_ns_usb3_mdio_phy_write(usb3, BCM_NS_USB3_TX_PMD_CONTROL1, 0x1003);
>
>         /* Waiting MII Mgt interface idle */
>         bcm_ns_usb3_mii_mng_wait_idle(usb3);
> --
> 2.11.0
>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs
  2017-04-02 16:55 ` Rafał Miłecki
@ 2017-04-05 10:10   ` Kishon Vijay Abraham I
  -1 siblings, 0 replies; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2017-04-05 10:10 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Yendapally Reddy Dhananjaya Reddy, Jon Mason, Florian Fainelli,
	linux-arm-kernel, bcm-kernel-feedback-list, linux-kernel,
	Rafał Miłecki

Hi,

On Sunday 02 April 2017 10:25 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> So far all the PHY initialization was implemented using some totally
> magic values. There was some pattern there but it wasn't clear what is
> it about.
> 
> Thanks to the patch submitted by Broadcom:
> [PATCH 5/6] phy: Add USB3 PHY support for Broadcom NSP SoC
> and the upstream "iproc-mdio" driver we now know there is a MDIO bus
> underneath with PHY(s) and their registers.
> 
> It allows us to clean the driver a bit by making all these values less
> magical. The next step is switching to using a proper MDIO layer.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/phy/phy-bcm-ns-usb3.c | 69 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/phy/phy-bcm-ns-usb3.c b/drivers/phy/phy-bcm-ns-usb3.c
> index f420fa4bebfc..22b5e7047fa6 100644
> --- a/drivers/phy/phy-bcm-ns-usb3.c
> +++ b/drivers/phy/phy-bcm-ns-usb3.c
> @@ -2,6 +2,7 @@
>   * Broadcom Northstar USB 3.0 PHY Driver
>   *
>   * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
> + * Copyright (C) 2016 Broadcom
>   *
>   * All magic values used for initialization (and related comments) were obtained
>   * from Broadcom's SDK:
> @@ -23,6 +24,23 @@
>  
>  #define BCM_NS_USB3_MII_MNG_TIMEOUT_US	1000	/* usecs */
>  
> +#define BCM_NS_USB3_PHY_BASE_ADDR_REG	0x1f
> +#define BCM_NS_USB3_PHY_PLL30_BLOCK	0x8000
> +#define BCM_NS_USB3_PHY_TX_PMD_BLOCK	0x8040
> +#define BCM_NS_USB3_PHY_PIPE_BLOCK	0x8060
> +
> +/* Registers of PLL30 block */
> +#define BCM_NS_USB3_PLL_CONTROL		0x01
> +#define BCM_NS_USB3_PLLA_CONTROL0	0x0a
> +#define BCM_NS_USB3_PLLA_CONTROL1	0x0b
> +
> +/* Registers of TX PMD block */
> +#define BCM_NS_USB3_TX_PMD_CONTROL1	0x01
> +
> +/* Registers of PIPE block */
> +#define BCM_NS_USB3_LFPS_CMP		0x02
> +#define BCM_NS_USB3_LFPS_DEGLITCH	0x03
> +
>  enum bcm_ns_family {
>  	BCM_NS_UNKNOWN,
>  	BCM_NS_AX,
> @@ -76,8 +94,10 @@ static inline int bcm_ns_usb3_mii_mng_wait_idle(struct bcm_ns_usb3 *usb3)
>  				    usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
>  }
>  
> -static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
> +static int bcm_ns_usb3_mdio_phy_write(struct bcm_ns_usb3 *usb3, u16 reg,
> +				      u16 value)
>  {
> +	u32 tmp = 0;
>  	int err;
>  
>  	err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
> @@ -86,7 +106,11 @@ static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
>  		return err;
>  	}
>  
> -	writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
> +	/* TODO: Use a proper MDIO bus layer */

Instead of using this intermediate patch, can we directly convert this to
mdio_driver or you see issues with converting it?

Thanks
Kishon

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs
@ 2017-04-05 10:10   ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2017-04-05 10:10 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

On Sunday 02 April 2017 10:25 PM, Rafa? Mi?ecki wrote:
> From: Rafa? Mi?ecki <rafal@milecki.pl>
> 
> So far all the PHY initialization was implemented using some totally
> magic values. There was some pattern there but it wasn't clear what is
> it about.
> 
> Thanks to the patch submitted by Broadcom:
> [PATCH 5/6] phy: Add USB3 PHY support for Broadcom NSP SoC
> and the upstream "iproc-mdio" driver we now know there is a MDIO bus
> underneath with PHY(s) and their registers.
> 
> It allows us to clean the driver a bit by making all these values less
> magical. The next step is switching to using a proper MDIO layer.
> 
> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
> ---
>  drivers/phy/phy-bcm-ns-usb3.c | 69 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/phy/phy-bcm-ns-usb3.c b/drivers/phy/phy-bcm-ns-usb3.c
> index f420fa4bebfc..22b5e7047fa6 100644
> --- a/drivers/phy/phy-bcm-ns-usb3.c
> +++ b/drivers/phy/phy-bcm-ns-usb3.c
> @@ -2,6 +2,7 @@
>   * Broadcom Northstar USB 3.0 PHY Driver
>   *
>   * Copyright (C) 2016 Rafa? Mi?ecki <rafal@milecki.pl>
> + * Copyright (C) 2016 Broadcom
>   *
>   * All magic values used for initialization (and related comments) were obtained
>   * from Broadcom's SDK:
> @@ -23,6 +24,23 @@
>  
>  #define BCM_NS_USB3_MII_MNG_TIMEOUT_US	1000	/* usecs */
>  
> +#define BCM_NS_USB3_PHY_BASE_ADDR_REG	0x1f
> +#define BCM_NS_USB3_PHY_PLL30_BLOCK	0x8000
> +#define BCM_NS_USB3_PHY_TX_PMD_BLOCK	0x8040
> +#define BCM_NS_USB3_PHY_PIPE_BLOCK	0x8060
> +
> +/* Registers of PLL30 block */
> +#define BCM_NS_USB3_PLL_CONTROL		0x01
> +#define BCM_NS_USB3_PLLA_CONTROL0	0x0a
> +#define BCM_NS_USB3_PLLA_CONTROL1	0x0b
> +
> +/* Registers of TX PMD block */
> +#define BCM_NS_USB3_TX_PMD_CONTROL1	0x01
> +
> +/* Registers of PIPE block */
> +#define BCM_NS_USB3_LFPS_CMP		0x02
> +#define BCM_NS_USB3_LFPS_DEGLITCH	0x03
> +
>  enum bcm_ns_family {
>  	BCM_NS_UNKNOWN,
>  	BCM_NS_AX,
> @@ -76,8 +94,10 @@ static inline int bcm_ns_usb3_mii_mng_wait_idle(struct bcm_ns_usb3 *usb3)
>  				    usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
>  }
>  
> -static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
> +static int bcm_ns_usb3_mdio_phy_write(struct bcm_ns_usb3 *usb3, u16 reg,
> +				      u16 value)
>  {
> +	u32 tmp = 0;
>  	int err;
>  
>  	err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
> @@ -86,7 +106,11 @@ static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
>  		return err;
>  	}
>  
> -	writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
> +	/* TODO: Use a proper MDIO bus layer */

Instead of using this intermediate patch, can we directly convert this to
mdio_driver or you see issues with converting it?

Thanks
Kishon

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs
  2017-04-05 10:10   ` Kishon Vijay Abraham I
@ 2017-04-05 10:52     ` Rafał Miłecki
  -1 siblings, 0 replies; 10+ messages in thread
From: Rafał Miłecki @ 2017-04-05 10:52 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Yendapally Reddy Dhananjaya Reddy, Jon Mason, Florian Fainelli,
	linux-arm-kernel, bcm-kernel-feedback-list, linux-kernel,
	Rafał Miłecki

On 04/05/2017 12:10 PM, Kishon Vijay Abraham I wrote:
> On Sunday 02 April 2017 10:25 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> So far all the PHY initialization was implemented using some totally
>> magic values. There was some pattern there but it wasn't clear what is
>> it about.
>>
>> Thanks to the patch submitted by Broadcom:
>> [PATCH 5/6] phy: Add USB3 PHY support for Broadcom NSP SoC
>> and the upstream "iproc-mdio" driver we now know there is a MDIO bus
>> underneath with PHY(s) and their registers.
>>
>> It allows us to clean the driver a bit by making all these values less
>> magical. The next step is switching to using a proper MDIO layer.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>  drivers/phy/phy-bcm-ns-usb3.c | 69 ++++++++++++++++++++++++++++++-------------
>>  1 file changed, 49 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/phy/phy-bcm-ns-usb3.c b/drivers/phy/phy-bcm-ns-usb3.c
>> index f420fa4bebfc..22b5e7047fa6 100644
>> --- a/drivers/phy/phy-bcm-ns-usb3.c
>> +++ b/drivers/phy/phy-bcm-ns-usb3.c
>> @@ -2,6 +2,7 @@
>>   * Broadcom Northstar USB 3.0 PHY Driver
>>   *
>>   * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
>> + * Copyright (C) 2016 Broadcom
>>   *
>>   * All magic values used for initialization (and related comments) were obtained
>>   * from Broadcom's SDK:
>> @@ -23,6 +24,23 @@
>>
>>  #define BCM_NS_USB3_MII_MNG_TIMEOUT_US	1000	/* usecs */
>>
>> +#define BCM_NS_USB3_PHY_BASE_ADDR_REG	0x1f
>> +#define BCM_NS_USB3_PHY_PLL30_BLOCK	0x8000
>> +#define BCM_NS_USB3_PHY_TX_PMD_BLOCK	0x8040
>> +#define BCM_NS_USB3_PHY_PIPE_BLOCK	0x8060
>> +
>> +/* Registers of PLL30 block */
>> +#define BCM_NS_USB3_PLL_CONTROL		0x01
>> +#define BCM_NS_USB3_PLLA_CONTROL0	0x0a
>> +#define BCM_NS_USB3_PLLA_CONTROL1	0x0b
>> +
>> +/* Registers of TX PMD block */
>> +#define BCM_NS_USB3_TX_PMD_CONTROL1	0x01
>> +
>> +/* Registers of PIPE block */
>> +#define BCM_NS_USB3_LFPS_CMP		0x02
>> +#define BCM_NS_USB3_LFPS_DEGLITCH	0x03
>> +
>>  enum bcm_ns_family {
>>  	BCM_NS_UNKNOWN,
>>  	BCM_NS_AX,
>> @@ -76,8 +94,10 @@ static inline int bcm_ns_usb3_mii_mng_wait_idle(struct bcm_ns_usb3 *usb3)
>>  				    usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
>>  }
>>
>> -static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
>> +static int bcm_ns_usb3_mdio_phy_write(struct bcm_ns_usb3 *usb3, u16 reg,
>> +				      u16 value)
>>  {
>> +	u32 tmp = 0;
>>  	int err;
>>
>>  	err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
>> @@ -86,7 +106,11 @@ static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
>>  		return err;
>>  	}
>>
>> -	writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
>> +	/* TODO: Use a proper MDIO bus layer */
>
> Instead of using this intermediate patch, can we directly convert this to
> mdio_driver or you see issues with converting it?

This is a bit more complex task with some possible issues to consider:
1) Backward compatibility - should we still support old DTB files?
2) Support for PHY DT node put in MDIO bus node - it requires reg = <phy num>; which is in conflict with current "reg" usage
3) Support for different reset reg - Northstar+ uses a different one - we'll probably need a syscon
4) My limited time (sadly)

The good point of this patch is that all changes made in:
bcm_ns_usb3_phy_init_ns_ax
bcm_ns_usb3_phy_init_ns_bx
will survive, so we're not going to change all the code back and forth.

If that's acceptable for you, I'd prefer to handle this with smaller patches.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs
@ 2017-04-05 10:52     ` Rafał Miłecki
  0 siblings, 0 replies; 10+ messages in thread
From: Rafał Miłecki @ 2017-04-05 10:52 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/05/2017 12:10 PM, Kishon Vijay Abraham I wrote:
> On Sunday 02 April 2017 10:25 PM, Rafa? Mi?ecki wrote:
>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>
>> So far all the PHY initialization was implemented using some totally
>> magic values. There was some pattern there but it wasn't clear what is
>> it about.
>>
>> Thanks to the patch submitted by Broadcom:
>> [PATCH 5/6] phy: Add USB3 PHY support for Broadcom NSP SoC
>> and the upstream "iproc-mdio" driver we now know there is a MDIO bus
>> underneath with PHY(s) and their registers.
>>
>> It allows us to clean the driver a bit by making all these values less
>> magical. The next step is switching to using a proper MDIO layer.
>>
>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>> ---
>>  drivers/phy/phy-bcm-ns-usb3.c | 69 ++++++++++++++++++++++++++++++-------------
>>  1 file changed, 49 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/phy/phy-bcm-ns-usb3.c b/drivers/phy/phy-bcm-ns-usb3.c
>> index f420fa4bebfc..22b5e7047fa6 100644
>> --- a/drivers/phy/phy-bcm-ns-usb3.c
>> +++ b/drivers/phy/phy-bcm-ns-usb3.c
>> @@ -2,6 +2,7 @@
>>   * Broadcom Northstar USB 3.0 PHY Driver
>>   *
>>   * Copyright (C) 2016 Rafa? Mi?ecki <rafal@milecki.pl>
>> + * Copyright (C) 2016 Broadcom
>>   *
>>   * All magic values used for initialization (and related comments) were obtained
>>   * from Broadcom's SDK:
>> @@ -23,6 +24,23 @@
>>
>>  #define BCM_NS_USB3_MII_MNG_TIMEOUT_US	1000	/* usecs */
>>
>> +#define BCM_NS_USB3_PHY_BASE_ADDR_REG	0x1f
>> +#define BCM_NS_USB3_PHY_PLL30_BLOCK	0x8000
>> +#define BCM_NS_USB3_PHY_TX_PMD_BLOCK	0x8040
>> +#define BCM_NS_USB3_PHY_PIPE_BLOCK	0x8060
>> +
>> +/* Registers of PLL30 block */
>> +#define BCM_NS_USB3_PLL_CONTROL		0x01
>> +#define BCM_NS_USB3_PLLA_CONTROL0	0x0a
>> +#define BCM_NS_USB3_PLLA_CONTROL1	0x0b
>> +
>> +/* Registers of TX PMD block */
>> +#define BCM_NS_USB3_TX_PMD_CONTROL1	0x01
>> +
>> +/* Registers of PIPE block */
>> +#define BCM_NS_USB3_LFPS_CMP		0x02
>> +#define BCM_NS_USB3_LFPS_DEGLITCH	0x03
>> +
>>  enum bcm_ns_family {
>>  	BCM_NS_UNKNOWN,
>>  	BCM_NS_AX,
>> @@ -76,8 +94,10 @@ static inline int bcm_ns_usb3_mii_mng_wait_idle(struct bcm_ns_usb3 *usb3)
>>  				    usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
>>  }
>>
>> -static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
>> +static int bcm_ns_usb3_mdio_phy_write(struct bcm_ns_usb3 *usb3, u16 reg,
>> +				      u16 value)
>>  {
>> +	u32 tmp = 0;
>>  	int err;
>>
>>  	err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
>> @@ -86,7 +106,11 @@ static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
>>  		return err;
>>  	}
>>
>> -	writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
>> +	/* TODO: Use a proper MDIO bus layer */
>
> Instead of using this intermediate patch, can we directly convert this to
> mdio_driver or you see issues with converting it?

This is a bit more complex task with some possible issues to consider:
1) Backward compatibility - should we still support old DTB files?
2) Support for PHY DT node put in MDIO bus node - it requires reg = <phy num>; which is in conflict with current "reg" usage
3) Support for different reset reg - Northstar+ uses a different one - we'll probably need a syscon
4) My limited time (sadly)

The good point of this patch is that all changes made in:
bcm_ns_usb3_phy_init_ns_ax
bcm_ns_usb3_phy_init_ns_bx
will survive, so we're not going to change all the code back and forth.

If that's acceptable for you, I'd prefer to handle this with smaller patches.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs
  2017-04-05 10:52     ` Rafał Miłecki
@ 2017-04-06 10:17       ` Kishon Vijay Abraham I
  -1 siblings, 0 replies; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2017-04-06 10:17 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Yendapally Reddy Dhananjaya Reddy, Jon Mason, Florian Fainelli,
	linux-arm-kernel, bcm-kernel-feedback-list, linux-kernel,
	Rafał Miłecki



On Wednesday 05 April 2017 04:22 PM, Rafał Miłecki wrote:
> On 04/05/2017 12:10 PM, Kishon Vijay Abraham I wrote:
>> On Sunday 02 April 2017 10:25 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> So far all the PHY initialization was implemented using some totally
>>> magic values. There was some pattern there but it wasn't clear what is
>>> it about.
>>>
>>> Thanks to the patch submitted by Broadcom:
>>> [PATCH 5/6] phy: Add USB3 PHY support for Broadcom NSP SoC
>>> and the upstream "iproc-mdio" driver we now know there is a MDIO bus
>>> underneath with PHY(s) and their registers.
>>>
>>> It allows us to clean the driver a bit by making all these values less
>>> magical. The next step is switching to using a proper MDIO layer.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>>  drivers/phy/phy-bcm-ns-usb3.c | 69 ++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 49 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/phy/phy-bcm-ns-usb3.c b/drivers/phy/phy-bcm-ns-usb3.c
>>> index f420fa4bebfc..22b5e7047fa6 100644
>>> --- a/drivers/phy/phy-bcm-ns-usb3.c
>>> +++ b/drivers/phy/phy-bcm-ns-usb3.c
>>> @@ -2,6 +2,7 @@
>>>   * Broadcom Northstar USB 3.0 PHY Driver
>>>   *
>>>   * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
>>> + * Copyright (C) 2016 Broadcom
>>>   *
>>>   * All magic values used for initialization (and related comments) were
>>> obtained
>>>   * from Broadcom's SDK:
>>> @@ -23,6 +24,23 @@
>>>
>>>  #define BCM_NS_USB3_MII_MNG_TIMEOUT_US    1000    /* usecs */
>>>
>>> +#define BCM_NS_USB3_PHY_BASE_ADDR_REG    0x1f
>>> +#define BCM_NS_USB3_PHY_PLL30_BLOCK    0x8000
>>> +#define BCM_NS_USB3_PHY_TX_PMD_BLOCK    0x8040
>>> +#define BCM_NS_USB3_PHY_PIPE_BLOCK    0x8060
>>> +
>>> +/* Registers of PLL30 block */
>>> +#define BCM_NS_USB3_PLL_CONTROL        0x01
>>> +#define BCM_NS_USB3_PLLA_CONTROL0    0x0a
>>> +#define BCM_NS_USB3_PLLA_CONTROL1    0x0b
>>> +
>>> +/* Registers of TX PMD block */
>>> +#define BCM_NS_USB3_TX_PMD_CONTROL1    0x01
>>> +
>>> +/* Registers of PIPE block */
>>> +#define BCM_NS_USB3_LFPS_CMP        0x02
>>> +#define BCM_NS_USB3_LFPS_DEGLITCH    0x03
>>> +
>>>  enum bcm_ns_family {
>>>      BCM_NS_UNKNOWN,
>>>      BCM_NS_AX,
>>> @@ -76,8 +94,10 @@ static inline int bcm_ns_usb3_mii_mng_wait_idle(struct
>>> bcm_ns_usb3 *usb3)
>>>                      usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
>>>  }
>>>
>>> -static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
>>> +static int bcm_ns_usb3_mdio_phy_write(struct bcm_ns_usb3 *usb3, u16 reg,
>>> +                      u16 value)
>>>  {
>>> +    u32 tmp = 0;
>>>      int err;
>>>
>>>      err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
>>> @@ -86,7 +106,11 @@ static int bcm_ns_usb3_mii_mng_write32(struct
>>> bcm_ns_usb3 *usb3, u32 value)
>>>          return err;
>>>      }
>>>
>>> -    writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
>>> +    /* TODO: Use a proper MDIO bus layer */
>>
>> Instead of using this intermediate patch, can we directly convert this to
>> mdio_driver or you see issues with converting it?
> 
> This is a bit more complex task with some possible issues to consider:
> 1) Backward compatibility - should we still support old DTB files?
> 2) Support for PHY DT node put in MDIO bus node - it requires reg = <phy num>;
> which is in conflict with current "reg" usage
> 3) Support for different reset reg - Northstar+ uses a different one - we'll
> probably need a syscon
> 4) My limited time (sadly)

Er..
> 
> The good point of this patch is that all changes made in:
> bcm_ns_usb3_phy_init_ns_ax
> bcm_ns_usb3_phy_init_ns_bx
> will survive, so we're not going to change all the code back and forth.
> 
> If that's acceptable for you, I'd prefer to handle this with smaller patches.

All right. I'll merge the patch as it is now.

Thanks
Kishon

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs
@ 2017-04-06 10:17       ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2017-04-06 10:17 UTC (permalink / raw)
  To: linux-arm-kernel



On Wednesday 05 April 2017 04:22 PM, Rafa? Mi?ecki wrote:
> On 04/05/2017 12:10 PM, Kishon Vijay Abraham I wrote:
>> On Sunday 02 April 2017 10:25 PM, Rafa? Mi?ecki wrote:
>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>
>>> So far all the PHY initialization was implemented using some totally
>>> magic values. There was some pattern there but it wasn't clear what is
>>> it about.
>>>
>>> Thanks to the patch submitted by Broadcom:
>>> [PATCH 5/6] phy: Add USB3 PHY support for Broadcom NSP SoC
>>> and the upstream "iproc-mdio" driver we now know there is a MDIO bus
>>> underneath with PHY(s) and their registers.
>>>
>>> It allows us to clean the driver a bit by making all these values less
>>> magical. The next step is switching to using a proper MDIO layer.
>>>
>>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>>> ---
>>>  drivers/phy/phy-bcm-ns-usb3.c | 69 ++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 49 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/phy/phy-bcm-ns-usb3.c b/drivers/phy/phy-bcm-ns-usb3.c
>>> index f420fa4bebfc..22b5e7047fa6 100644
>>> --- a/drivers/phy/phy-bcm-ns-usb3.c
>>> +++ b/drivers/phy/phy-bcm-ns-usb3.c
>>> @@ -2,6 +2,7 @@
>>>   * Broadcom Northstar USB 3.0 PHY Driver
>>>   *
>>>   * Copyright (C) 2016 Rafa? Mi?ecki <rafal@milecki.pl>
>>> + * Copyright (C) 2016 Broadcom
>>>   *
>>>   * All magic values used for initialization (and related comments) were
>>> obtained
>>>   * from Broadcom's SDK:
>>> @@ -23,6 +24,23 @@
>>>
>>>  #define BCM_NS_USB3_MII_MNG_TIMEOUT_US    1000    /* usecs */
>>>
>>> +#define BCM_NS_USB3_PHY_BASE_ADDR_REG    0x1f
>>> +#define BCM_NS_USB3_PHY_PLL30_BLOCK    0x8000
>>> +#define BCM_NS_USB3_PHY_TX_PMD_BLOCK    0x8040
>>> +#define BCM_NS_USB3_PHY_PIPE_BLOCK    0x8060
>>> +
>>> +/* Registers of PLL30 block */
>>> +#define BCM_NS_USB3_PLL_CONTROL        0x01
>>> +#define BCM_NS_USB3_PLLA_CONTROL0    0x0a
>>> +#define BCM_NS_USB3_PLLA_CONTROL1    0x0b
>>> +
>>> +/* Registers of TX PMD block */
>>> +#define BCM_NS_USB3_TX_PMD_CONTROL1    0x01
>>> +
>>> +/* Registers of PIPE block */
>>> +#define BCM_NS_USB3_LFPS_CMP        0x02
>>> +#define BCM_NS_USB3_LFPS_DEGLITCH    0x03
>>> +
>>>  enum bcm_ns_family {
>>>      BCM_NS_UNKNOWN,
>>>      BCM_NS_AX,
>>> @@ -76,8 +94,10 @@ static inline int bcm_ns_usb3_mii_mng_wait_idle(struct
>>> bcm_ns_usb3 *usb3)
>>>                      usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
>>>  }
>>>
>>> -static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 value)
>>> +static int bcm_ns_usb3_mdio_phy_write(struct bcm_ns_usb3 *usb3, u16 reg,
>>> +                      u16 value)
>>>  {
>>> +    u32 tmp = 0;
>>>      int err;
>>>
>>>      err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
>>> @@ -86,7 +106,11 @@ static int bcm_ns_usb3_mii_mng_write32(struct
>>> bcm_ns_usb3 *usb3, u32 value)
>>>          return err;
>>>      }
>>>
>>> -    writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
>>> +    /* TODO: Use a proper MDIO bus layer */
>>
>> Instead of using this intermediate patch, can we directly convert this to
>> mdio_driver or you see issues with converting it?
> 
> This is a bit more complex task with some possible issues to consider:
> 1) Backward compatibility - should we still support old DTB files?
> 2) Support for PHY DT node put in MDIO bus node - it requires reg = <phy num>;
> which is in conflict with current "reg" usage
> 3) Support for different reset reg - Northstar+ uses a different one - we'll
> probably need a syscon
> 4) My limited time (sadly)

Er..
> 
> The good point of this patch is that all changes made in:
> bcm_ns_usb3_phy_init_ns_ax
> bcm_ns_usb3_phy_init_ns_bx
> will survive, so we're not going to change all the code back and forth.
> 
> If that's acceptable for you, I'd prefer to handle this with smaller patches.

All right. I'll merge the patch as it is now.

Thanks
Kishon

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-04-06 10:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-02 16:55 [PATCH] phy: bcm-ns-usb3: split all writes into reg & val pairs Rafał Miłecki
2017-04-02 16:55 ` Rafał Miłecki
2017-04-03  3:02 ` Jon Mason
2017-04-03  3:02   ` Jon Mason
2017-04-05 10:10 ` Kishon Vijay Abraham I
2017-04-05 10:10   ` Kishon Vijay Abraham I
2017-04-05 10:52   ` Rafał Miłecki
2017-04-05 10:52     ` Rafał Miłecki
2017-04-06 10:17     ` Kishon Vijay Abraham I
2017-04-06 10:17       ` Kishon Vijay Abraham I

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.