* [PATCH v2 0/2] net: ethernet: bgmac: bug fixes @ 2017-02-03 21:08 Jon Mason 2017-02-03 21:08 ` [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug Jon Mason 2017-02-03 21:08 ` [PATCH v2 2/2] net: ethernet: bgmac: mac address change bug Jon Mason 0 siblings, 2 replies; 7+ messages in thread From: Jon Mason @ 2017-02-03 21:08 UTC (permalink / raw) To: David Miller; +Cc: rafal, bcm-kernel-feedback-list, netdev, linux-kernel Changes in v2: * Reworked the first match to make it more obvious what portions of the register were being preserved (Per Rafal Mileki) * Style change to reorder the function variables in patch 2 (per Sergei Shtylyov) Bug fixes for bgmac driver Hari Vyas (1): net: ethernet: bgmac: mac address change bug Zac Schroff (1): net: ethernet: bgmac: init sequence bug drivers/net/ethernet/broadcom/bgmac-platform.c | 14 +++++++++----- drivers/net/ethernet/broadcom/bgmac.c | 6 +++++- drivers/net/ethernet/broadcom/bgmac.h | 16 ++++++++++++++++ 3 files changed, 30 insertions(+), 6 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug 2017-02-03 21:08 [PATCH v2 0/2] net: ethernet: bgmac: bug fixes Jon Mason @ 2017-02-03 21:08 ` Jon Mason 2017-02-03 21:41 ` Rafał Miłecki 2017-02-03 21:08 ` [PATCH v2 2/2] net: ethernet: bgmac: mac address change bug Jon Mason 1 sibling, 1 reply; 7+ messages in thread From: Jon Mason @ 2017-02-03 21:08 UTC (permalink / raw) To: David Miller Cc: rafal, bcm-kernel-feedback-list, netdev, linux-kernel, Zac Schroff 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug 2017-02-03 21:08 ` [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug Jon Mason @ 2017-02-03 21:41 ` Rafał Miłecki 2017-02-03 22:34 ` Jon Mason 0 siblings, 1 reply; 7+ messages in thread From: Rafał Miłecki @ 2017-02-03 21:41 UTC (permalink / raw) To: Jon Mason, David Miller Cc: bcm-kernel-feedback-list, netdev, linux-kernel, Zac Schroff 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug 2017-02-03 21:41 ` Rafał Miłecki @ 2017-02-03 22:34 ` Jon Mason 0 siblings, 0 replies; 7+ messages in thread From: Jon Mason @ 2017-02-03 22:34 UTC (permalink / raw) To: Rafał Miłecki Cc: David Miller, BCM Kernel Feedback, Network Development, open list, Zac Schroff 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] net: ethernet: bgmac: mac address change bug 2017-02-03 21:08 [PATCH v2 0/2] net: ethernet: bgmac: bug fixes Jon Mason 2017-02-03 21:08 ` [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug Jon Mason @ 2017-02-03 21:08 ` Jon Mason 2017-02-03 21:44 ` Rafał Miłecki 1 sibling, 1 reply; 7+ messages in thread From: Jon Mason @ 2017-02-03 21:08 UTC (permalink / raw) To: David Miller Cc: rafal, bcm-kernel-feedback-list, netdev, linux-kernel, Hari Vyas From: Hari Vyas <hariv@broadcom.com> ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to bgmac_set_mac_address() but code assumed u8 *. This caused two bytes chopping and the wrong mac address was configured. Signed-off-by: Hari Vyas <hariv@broadcom.com> Signed-off-by: Jon Mason <jon.mason@broadcom.com> Fixes: 4e209001b86 ("bgmac: write mac address to hardware in ndo_set_mac_address") --- drivers/net/ethernet/broadcom/bgmac.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c index 0e066dc6..737be50 100644 --- a/drivers/net/ethernet/broadcom/bgmac.c +++ b/drivers/net/ethernet/broadcom/bgmac.c @@ -1221,12 +1221,16 @@ static netdev_tx_t bgmac_start_xmit(struct sk_buff *skb, static int bgmac_set_mac_address(struct net_device *net_dev, void *addr) { struct bgmac *bgmac = netdev_priv(net_dev); + struct sockaddr *sa = addr; int ret; ret = eth_prepare_mac_addr_change(net_dev, addr); if (ret < 0) return ret; - bgmac_write_mac_address(bgmac, (u8 *)addr); + + ether_addr_copy(bgmac->mac_addr, sa->sa_data); + bgmac_write_mac_address(bgmac, bgmac->mac_addr); + eth_commit_mac_addr_change(net_dev, addr); return 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] net: ethernet: bgmac: mac address change bug 2017-02-03 21:08 ` [PATCH v2 2/2] net: ethernet: bgmac: mac address change bug Jon Mason @ 2017-02-03 21:44 ` Rafał Miłecki 2017-02-03 21:49 ` Florian Fainelli 0 siblings, 1 reply; 7+ messages in thread From: Rafał Miłecki @ 2017-02-03 21:44 UTC (permalink / raw) To: Jon Mason, David Miller Cc: bcm-kernel-feedback-list, netdev, linux-kernel, Hari Vyas On 02/03/2017 10:08 PM, Jon Mason wrote: > From: Hari Vyas <hariv@broadcom.com> > > ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to > bgmac_set_mac_address() but code assumed u8 *. This caused two bytes > chopping and the wrong mac address was configured. > > Signed-off-by: Hari Vyas <hariv@broadcom.com> > Signed-off-by: Jon Mason <jon.mason@broadcom.com> > Fixes: 4e209001b86 ("bgmac: write mac address to hardware in ndo_set_mac_address") I think you were going to Cc stable? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] net: ethernet: bgmac: mac address change bug 2017-02-03 21:44 ` Rafał Miłecki @ 2017-02-03 21:49 ` Florian Fainelli 0 siblings, 0 replies; 7+ messages in thread From: Florian Fainelli @ 2017-02-03 21:49 UTC (permalink / raw) To: Rafał Miłecki, Jon Mason, David Miller Cc: bcm-kernel-feedback-list, netdev, linux-kernel, Hari Vyas On 02/03/2017 01:44 PM, Rafał Miłecki wrote: > On 02/03/2017 10:08 PM, Jon Mason wrote: >> From: Hari Vyas <hariv@broadcom.com> >> >> ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to >> bgmac_set_mac_address() but code assumed u8 *. This caused two bytes >> chopping and the wrong mac address was configured. >> >> Signed-off-by: Hari Vyas <hariv@broadcom.com> >> Signed-off-by: Jon Mason <jon.mason@broadcom.com> >> Fixes: 4e209001b86 ("bgmac: write mac address to hardware in >> ndo_set_mac_address") > > I think you were going to Cc stable? David takes care of queueing patches for -stable trees: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.txt#n114 -- Florian ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-02-03 23:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-03 21:08 [PATCH v2 0/2] net: ethernet: bgmac: bug fixes Jon Mason 2017-02-03 21:08 ` [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug Jon Mason 2017-02-03 21:41 ` Rafał Miłecki 2017-02-03 22:34 ` Jon Mason 2017-02-03 21:08 ` [PATCH v2 2/2] net: ethernet: bgmac: mac address change bug Jon Mason 2017-02-03 21:44 ` Rafał Miłecki 2017-02-03 21:49 ` Florian Fainelli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).