Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1573435 > unrolled thread

[PATCH v2 1/2] net: ethernet: bgmac: init sequence bug

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug Jon Mason <jon.mason@broadcom.com> - 2017-02-03 22:10 +0100
    Re: [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug Jon Mason <jon.mason@broadcom.com> - 2017-02-03 23:40 +0100
    Re: [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug Rafał Miłecki <rafal@milecki.pl> - 2017-02-04 00:10 +0100

#1573435 — [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug

From: Zac Schroff <zschroff@broadcom.com>

Fix a bug in the 'bgmac' driver init sequence that blind writes for init
sequence where it should preserve most bits other than the ones it is
deliberately manipulating.

Signed-off-by: Zac Schroff <zschroff@broadcom.com>
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
Fixes: f6a95a24957 ("net: ethernet: bgmac: Add platform device support")
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 14 +++++++++-----
 drivers/net/ethernet/broadcom/bgmac.h          | 16 ++++++++++++++++
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 6f736c1..a626dce 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -51,8 +51,7 @@ static void platform_bgmac_idm_write(struct bgmac *bgmac, u16 offset, u32 value)
 
 static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
 {
-	if ((bgmac_idm_read(bgmac, BCMA_IOCTL) &
-	     (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC)) != BCMA_IOCTL_CLK)
+	if ((bgmac_idm_read(bgmac, BCMA_IOCTL) & BGMAC_CLK_EN) != BGMAC_CLK_EN)
 		return false;
 	if (bgmac_idm_read(bgmac, BCMA_RESET_CTL) & BCMA_RESET_CTL_RESET)
 		return false;
@@ -61,15 +60,20 @@ static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
 
 static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
 {
-	bgmac_idm_write(bgmac, BCMA_IOCTL,
-			(BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
+	u32 val;
+
+	val = bgmac_idm_read(bgmac, BCMA_IOCTL);
+	/* Some bits of BCMA_IOCTL set by HW/ATF and should not change */
+	val |= flags & ~(BGMAC_AWCACHE | BGMAC_ARCACHE | BGMAC_AWUSER |
+			 BGMAC_ARUSER);
+	val |= BGMAC_CLK_EN;
 	bgmac_idm_read(bgmac, BCMA_IOCTL);
 
 	bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
 	bgmac_idm_read(bgmac, BCMA_RESET_CTL);
 	udelay(1);
 
-	bgmac_idm_write(bgmac, BCMA_IOCTL, (BCMA_IOCTL_CLK | flags));
+	bgmac_idm_write(bgmac, BCMA_IOCTL, val);
 	bgmac_idm_read(bgmac, BCMA_IOCTL);
 	udelay(1);
 }
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index 71f493f..c8d33eb 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -213,6 +213,22 @@
 /* BCMA GMAC core specific IO Control (BCMA_IOCTL) flags */
 #define BGMAC_BCMA_IOCTL_SW_CLKEN		0x00000004	/* PHY Clock Enable */
 #define BGMAC_BCMA_IOCTL_SW_RESET		0x00000008	/* PHY Reset */
+/* The IOCTL values appear to be different in NS, NSP, and NS2, and do not match
+ * the values directly above
+ */
+#define BGMAC_CLK_EN				BIT(0)
+#define BGMAC_RESERVED_0			BIT(1)
+#define BGMAC_SOURCE_SYNC_MODE_EN		BIT(2)
+#define BGMAC_DEST_SYNC_MODE_EN			BIT(3)
+#define BGMAC_TX_CLK_OUT_INVERT_EN		BIT(4)
+#define BGMAC_DIRECT_GMII_MODE			BIT(5)
+#define BGMAC_CLK_250_SEL			BIT(6)
+#define BGMAC_AWCACHE				(0xf << 7)
+#define BGMAC_RESERVED_1			(0x1f << 11)
+#define BGMAC_ARCACHE				(0xf << 16)
+#define BGMAC_AWUSER				(0x3f << 20)
+#define BGMAC_ARUSER				(0x3f << 26)
+#define BGMAC_RESERVED				BIT(31)
 
 /* BCMA GMAC core specific IO status (BCMA_IOST) flags */
 #define BGMAC_BCMA_IOST_ATTACHED		0x00000800
-- 
2.7.4

[toc] | [next] | [standalone]

#1573489

On Fri, Feb 3, 2017 at 4:41 PM, Rafał Miłecki <rafal@milecki.pl> wrote:
> On 02/03/2017 10:08 PM, Jon Mason wrote:
>>
>> @@ -61,15 +60,20 @@ static bool platform_bgmac_clk_enabled(struct bgmac
>> *bgmac)
>>
>>  static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
>>  {
>> -       bgmac_idm_write(bgmac, BCMA_IOCTL,
>> -                       (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
>> +       u32 val;
>> +
>> +       val = bgmac_idm_read(bgmac, BCMA_IOCTL);
>> +       /* Some bits of BCMA_IOCTL set by HW/ATF and should not change */
>> +       val |= flags & ~(BGMAC_AWCACHE | BGMAC_ARCACHE | BGMAC_AWUSER |
>> +                        BGMAC_ARUSER);
>> +       val |= BGMAC_CLK_EN;
>>         bgmac_idm_read(bgmac, BCMA_IOCTL);
>
>
> This read was previously following write op most likely to flush it or
> something. I don't think it makes any sense to read after read.

Actually, that is sloppy coding on my part.  It should have a write
prior to the read to match what was there before.

I find it odd that it worked when I tested this patch.  It makes me
wonder if this "modify, reset, modify" series is really necessary
after all.  The docs indicate that writing a 0 to the reset brings it
out of reset.  I do not see any code that puts the HW in reset.  So,
unless the bootloader puts the HW in reset or it is in the reset state
by default, this seems like unnecessary code.  I can add some CYA
logic to read and see if it is in reset, toggle the bit, and then just
do the CLK enable.  Thoughts?

Thanks,
Jon

[toc] | [prev] | [next] | [standalone]

#1573510

On 02/03/2017 10:08 PM, Jon Mason wrote:
> @@ -61,15 +60,20 @@ static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
>
>  static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
>  {
> -	bgmac_idm_write(bgmac, BCMA_IOCTL,
> -			(BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
> +	u32 val;
> +
> +	val = bgmac_idm_read(bgmac, BCMA_IOCTL);
> +	/* Some bits of BCMA_IOCTL set by HW/ATF and should not change */
> +	val |= flags & ~(BGMAC_AWCACHE | BGMAC_ARCACHE | BGMAC_AWUSER |
> +			 BGMAC_ARUSER);
> +	val |= BGMAC_CLK_EN;
>  	bgmac_idm_read(bgmac, BCMA_IOCTL);

This read was previously following write op most likely to flush it or
something. I don't think it makes any sense to read after read.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web