* [PATCH 0/2] net: dsa: bcm_sf2: refactor & update RGMII regs
@ 2021-03-17 14:37 Rafał Miłecki
2021-03-17 14:37 ` [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register Rafał Miłecki
2021-03-17 14:37 ` [PATCH 2/2] net: dsa: bcm_sf2: fix BCM4908 RGMII reg(s) Rafał Miłecki
0 siblings, 2 replies; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-17 14:37 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
The first patch fixes some generic problem but it seems so device other
than BCM4908 SoC were affected. So this probably isn't critical.
I think it's OK / enough to take it for the net-next.git.
Rafał Miłecki (2):
net: dsa: bcm_sf2: add function finding RGMII register
net: dsa: bcm_sf2: fix BCM4908 RGMII reg(s)
drivers/net/dsa/bcm_sf2.c | 60 +++++++++++++++++++++++++++++-----
drivers/net/dsa/bcm_sf2_regs.h | 3 +-
2 files changed, 52 insertions(+), 11 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register
2021-03-17 14:37 [PATCH 0/2] net: dsa: bcm_sf2: refactor & update RGMII regs Rafał Miłecki
@ 2021-03-17 14:37 ` Rafał Miłecki
2021-03-17 21:20 ` Florian Fainelli
2021-03-17 14:37 ` [PATCH 2/2] net: dsa: bcm_sf2: fix BCM4908 RGMII reg(s) Rafał Miłecki
1 sibling, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-17 14:37 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
Simple macro like REG_RGMII_CNTRL_P() is insufficient as:
1. It doesn't validate port argument
2. It doesn't support chipsets with non-lineral RGMII regs layout
Missing port validation could result in getting register offset from out
of array. Random memory -> random offset -> random reads/writes. It
affected e.g. BCM4908 for REG_RGMII_CNTRL_P(7).
Fixes: a78e86ed586d ("net: dsa: bcm_sf2: Prepare for different register layouts")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
drivers/net/dsa/bcm_sf2.c | 51 ++++++++++++++++++++++++++++++----
drivers/net/dsa/bcm_sf2_regs.h | 2 --
2 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index ba5d546d06aa..942773bcb7e0 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -32,6 +32,30 @@
#include "b53/b53_priv.h"
#include "b53/b53_regs.h"
+static u16 bcm_sf2_reg_rgmii_cntrl(struct bcm_sf2_priv *priv, int port)
+{
+ switch (priv->type) {
+ case BCM4908_DEVICE_ID:
+ /* TODO */
+ break;
+ default:
+ switch (port) {
+ case 0:
+ return REG_RGMII_0_CNTRL;
+ case 1:
+ return REG_RGMII_1_CNTRL;
+ case 2:
+ return REG_RGMII_2_CNTRL;
+ default:
+ break;
+ }
+ }
+
+ WARN_ONCE(1, "Unsupported port %d\n", port);
+
+ return 0;
+}
+
/* Return the number of active ports, not counting the IMP (CPU) port */
static unsigned int bcm_sf2_num_active_ports(struct dsa_switch *ds)
{
@@ -647,6 +671,7 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
{
struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
u32 id_mode_dis = 0, port_mode;
+ u32 reg_rgmii_ctrl;
u32 reg;
if (port == core_readl(priv, CORE_IMP0_PRT_ID))
@@ -670,10 +695,14 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
return;
}
+ reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
+ if (!reg_rgmii_ctrl)
+ return;
+
/* Clear id_mode_dis bit, and the existing port mode, let
* RGMII_MODE_EN bet set by mac_link_{up,down}
*/
- reg = reg_readl(priv, REG_RGMII_CNTRL_P(port));
+ reg = reg_readl(priv, reg_rgmii_ctrl);
reg &= ~ID_MODE_DIS;
reg &= ~(PORT_MODE_MASK << PORT_MODE_SHIFT);
@@ -681,13 +710,14 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
if (id_mode_dis)
reg |= ID_MODE_DIS;
- reg_writel(priv, reg, REG_RGMII_CNTRL_P(port));
+ reg_writel(priv, reg, reg_rgmii_ctrl);
}
static void bcm_sf2_sw_mac_link_set(struct dsa_switch *ds, int port,
phy_interface_t interface, bool link)
{
struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
+ u32 reg_rgmii_ctrl;
u32 reg;
if (!phy_interface_mode_is_rgmii(interface) &&
@@ -695,13 +725,17 @@ static void bcm_sf2_sw_mac_link_set(struct dsa_switch *ds, int port,
interface != PHY_INTERFACE_MODE_REVMII)
return;
+ reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
+ if (!reg_rgmii_ctrl)
+ return;
+
/* If the link is down, just disable the interface to conserve power */
- reg = reg_readl(priv, REG_RGMII_CNTRL_P(port));
+ reg = reg_readl(priv, reg_rgmii_ctrl);
if (link)
reg |= RGMII_MODE_EN;
else
reg &= ~RGMII_MODE_EN;
- reg_writel(priv, reg, REG_RGMII_CNTRL_P(port));
+ reg_writel(priv, reg, reg_rgmii_ctrl);
}
static void bcm_sf2_sw_mac_link_down(struct dsa_switch *ds, int port,
@@ -735,8 +769,13 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
{
struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
struct ethtool_eee *p = &priv->dev->ports[port].eee;
+ u32 reg_rgmii_ctrl;
u32 reg, offset;
+ reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
+ if (!reg_rgmii_ctrl)
+ return;
+
bcm_sf2_sw_mac_link_set(ds, port, interface, true);
if (port != core_readl(priv, CORE_IMP0_PRT_ID)) {
@@ -750,7 +789,7 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
interface == PHY_INTERFACE_MODE_RGMII_TXID ||
interface == PHY_INTERFACE_MODE_MII ||
interface == PHY_INTERFACE_MODE_REVMII) {
- reg = reg_readl(priv, REG_RGMII_CNTRL_P(port));
+ reg = reg_readl(priv, reg_rgmii_ctrl);
reg &= ~(RX_PAUSE_EN | TX_PAUSE_EN);
if (tx_pause)
@@ -758,7 +797,7 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
if (rx_pause)
reg |= RX_PAUSE_EN;
- reg_writel(priv, reg, REG_RGMII_CNTRL_P(port));
+ reg_writel(priv, reg, reg_rgmii_ctrl);
}
reg = SW_OVERRIDE | LINK_STS;
diff --git a/drivers/net/dsa/bcm_sf2_regs.h b/drivers/net/dsa/bcm_sf2_regs.h
index 1d2d55c9f8aa..c7783cb45845 100644
--- a/drivers/net/dsa/bcm_sf2_regs.h
+++ b/drivers/net/dsa/bcm_sf2_regs.h
@@ -48,8 +48,6 @@ enum bcm_sf2_reg_offs {
#define PHY_PHYAD_SHIFT 8
#define PHY_PHYAD_MASK 0x1F
-#define REG_RGMII_CNTRL_P(x) (REG_RGMII_0_CNTRL + (x))
-
/* Relative to REG_RGMII_CNTRL */
#define RGMII_MODE_EN (1 << 0)
#define ID_MODE_DIS (1 << 1)
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] net: dsa: bcm_sf2: fix BCM4908 RGMII reg(s)
2021-03-17 14:37 [PATCH 0/2] net: dsa: bcm_sf2: refactor & update RGMII regs Rafał Miłecki
2021-03-17 14:37 ` [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register Rafał Miłecki
@ 2021-03-17 14:37 ` Rafał Miłecki
1 sibling, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-17 14:37 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
BCM4908 has only 1 RGMII reg for controlling port 7.
Fixes: 73b7a6047971 ("net: dsa: bcm_sf2: support BCM4908's integrated switch")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
drivers/net/dsa/bcm_sf2.c | 11 +++++++----
drivers/net/dsa/bcm_sf2_regs.h | 1 +
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 942773bcb7e0..5dc02dbb7094 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -36,7 +36,12 @@ static u16 bcm_sf2_reg_rgmii_cntrl(struct bcm_sf2_priv *priv, int port)
{
switch (priv->type) {
case BCM4908_DEVICE_ID:
- /* TODO */
+ switch (port) {
+ case 7:
+ return REG_RGMII_11_CNTRL;
+ default:
+ break;
+ }
break;
default:
switch (port) {
@@ -1183,9 +1188,7 @@ static const u16 bcm_sf2_4908_reg_offsets[] = {
[REG_PHY_REVISION] = 0x14,
[REG_SPHY_CNTRL] = 0x24,
[REG_CROSSBAR] = 0xc8,
- [REG_RGMII_0_CNTRL] = 0xe0,
- [REG_RGMII_1_CNTRL] = 0xec,
- [REG_RGMII_2_CNTRL] = 0xf8,
+ [REG_RGMII_11_CNTRL] = 0x014c,
[REG_LED_0_CNTRL] = 0x40,
[REG_LED_1_CNTRL] = 0x4c,
[REG_LED_2_CNTRL] = 0x58,
diff --git a/drivers/net/dsa/bcm_sf2_regs.h b/drivers/net/dsa/bcm_sf2_regs.h
index c7783cb45845..9e141d1a0b07 100644
--- a/drivers/net/dsa/bcm_sf2_regs.h
+++ b/drivers/net/dsa/bcm_sf2_regs.h
@@ -21,6 +21,7 @@ enum bcm_sf2_reg_offs {
REG_RGMII_0_CNTRL,
REG_RGMII_1_CNTRL,
REG_RGMII_2_CNTRL,
+ REG_RGMII_11_CNTRL,
REG_LED_0_CNTRL,
REG_LED_1_CNTRL,
REG_LED_2_CNTRL,
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register
2021-03-17 14:37 ` [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register Rafał Miłecki
@ 2021-03-17 21:20 ` Florian Fainelli
2021-03-18 7:30 ` Rafał Miłecki
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2021-03-17 21:20 UTC (permalink / raw)
To: Rafał Miłecki, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
On 3/17/2021 7:37 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Simple macro like REG_RGMII_CNTRL_P() is insufficient as:
> 1. It doesn't validate port argument
> 2. It doesn't support chipsets with non-lineral RGMII regs layout
>
> Missing port validation could result in getting register offset from out
> of array. Random memory -> random offset -> random reads/writes. It
> affected e.g. BCM4908 for REG_RGMII_CNTRL_P(7).
That is entirely fair, however as a bug fix this is not necessarily the
simplest way to approach this.
>
> Fixes: a78e86ed586d ("net: dsa: bcm_sf2: Prepare for different register layouts")
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> drivers/net/dsa/bcm_sf2.c | 51 ++++++++++++++++++++++++++++++----
> drivers/net/dsa/bcm_sf2_regs.h | 2 --
> 2 files changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index ba5d546d06aa..942773bcb7e0 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -32,6 +32,30 @@
> #include "b53/b53_priv.h"
> #include "b53/b53_regs.h"
>
> +static u16 bcm_sf2_reg_rgmii_cntrl(struct bcm_sf2_priv *priv, int port)
This is not meant to be used outside the file, so I would be keen on
removing the bcm_sf2_ prefix to make the name shorter and closer to the
original macro name.
> +{
> + switch (priv->type) {
> + case BCM4908_DEVICE_ID:
> + /* TODO */
> + break;
> + default:
> + switch (port) {
> + case 0:
> + return REG_RGMII_0_CNTRL;
> + case 1:
> + return REG_RGMII_1_CNTRL;
> + case 2:
> + return REG_RGMII_2_CNTRL;
> + default:
> + break;
> + }
> + }
> +
> + WARN_ONCE(1, "Unsupported port %d\n", port);
> +
> + return 0;
maybe return -1 or -EINVAL just in case 0 happens to be a valid offset
in the future. Checking the return value is not necessarily going to be
helpful as it needs immediate fixing, so what we could do is keep the
WARN_ON, and return the offset of REG_SWITCH_STATUS which is a read-only
register. This will trigger the bus arbiter logic to return an error
because a write was attempted from a read-only register.
What do you think?
> +}
> +
> /* Return the number of active ports, not counting the IMP (CPU) port */
> static unsigned int bcm_sf2_num_active_ports(struct dsa_switch *ds)
> {
> @@ -647,6 +671,7 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
> {
> struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
> u32 id_mode_dis = 0, port_mode;
> + u32 reg_rgmii_ctrl;
> u32 reg;
>
> if (port == core_readl(priv, CORE_IMP0_PRT_ID))
> @@ -670,10 +695,14 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
> return;
> }
>
> + reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
> + if (!reg_rgmii_ctrl)
> + return;
> +
> /* Clear id_mode_dis bit, and the existing port mode, let
> * RGMII_MODE_EN bet set by mac_link_{up,down}
> */
> - reg = reg_readl(priv, REG_RGMII_CNTRL_P(port));
> + reg = reg_readl(priv, reg_rgmii_ctrl);
> reg &= ~ID_MODE_DIS;
> reg &= ~(PORT_MODE_MASK << PORT_MODE_SHIFT);
>
> @@ -681,13 +710,14 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
> if (id_mode_dis)
> reg |= ID_MODE_DIS;
>
> - reg_writel(priv, reg, REG_RGMII_CNTRL_P(port));
> + reg_writel(priv, reg, reg_rgmii_ctrl);
> }
>
> static void bcm_sf2_sw_mac_link_set(struct dsa_switch *ds, int port,
> phy_interface_t interface, bool link)
> {
> struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
> + u32 reg_rgmii_ctrl;
> u32 reg;
>
> if (!phy_interface_mode_is_rgmii(interface) &&
> @@ -695,13 +725,17 @@ static void bcm_sf2_sw_mac_link_set(struct dsa_switch *ds, int port,
> interface != PHY_INTERFACE_MODE_REVMII)
> return;
>
> + reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
> + if (!reg_rgmii_ctrl)
> + return;
> +
> /* If the link is down, just disable the interface to conserve power */
> - reg = reg_readl(priv, REG_RGMII_CNTRL_P(port));
> + reg = reg_readl(priv, reg_rgmii_ctrl);
> if (link)
> reg |= RGMII_MODE_EN;
> else
> reg &= ~RGMII_MODE_EN;
> - reg_writel(priv, reg, REG_RGMII_CNTRL_P(port));
> + reg_writel(priv, reg, reg_rgmii_ctrl);
> }
>
> static void bcm_sf2_sw_mac_link_down(struct dsa_switch *ds, int port,
> @@ -735,8 +769,13 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
> {
> struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
> struct ethtool_eee *p = &priv->dev->ports[port].eee;
> + u32 reg_rgmii_ctrl;
> u32 reg, offset;
>
> + reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
> + if (!reg_rgmii_ctrl)
> + return;
> +
> bcm_sf2_sw_mac_link_set(ds, port, interface, true);
>
> if (port != core_readl(priv, CORE_IMP0_PRT_ID)) {
> @@ -750,7 +789,7 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
> interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> interface == PHY_INTERFACE_MODE_MII ||
> interface == PHY_INTERFACE_MODE_REVMII) {
> - reg = reg_readl(priv, REG_RGMII_CNTRL_P(port));
> + reg = reg_readl(priv, reg_rgmii_ctrl);
> reg &= ~(RX_PAUSE_EN | TX_PAUSE_EN);
>
> if (tx_pause)
> @@ -758,7 +797,7 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
> if (rx_pause)
> reg |= RX_PAUSE_EN;
>
> - reg_writel(priv, reg, REG_RGMII_CNTRL_P(port));
> + reg_writel(priv, reg, reg_rgmii_ctrl);
> }
>
> reg = SW_OVERRIDE | LINK_STS;
> diff --git a/drivers/net/dsa/bcm_sf2_regs.h b/drivers/net/dsa/bcm_sf2_regs.h
> index 1d2d55c9f8aa..c7783cb45845 100644
> --- a/drivers/net/dsa/bcm_sf2_regs.h
> +++ b/drivers/net/dsa/bcm_sf2_regs.h
> @@ -48,8 +48,6 @@ enum bcm_sf2_reg_offs {
> #define PHY_PHYAD_SHIFT 8
> #define PHY_PHYAD_MASK 0x1F
>
> -#define REG_RGMII_CNTRL_P(x) (REG_RGMII_0_CNTRL + (x))
> -
> /* Relative to REG_RGMII_CNTRL */
> #define RGMII_MODE_EN (1 << 0)
> #define ID_MODE_DIS (1 << 1)
>
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register
2021-03-17 21:20 ` Florian Fainelli
@ 2021-03-18 7:30 ` Rafał Miłecki
2021-03-18 16:16 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-18 7:30 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, Vivien Didelot, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
On 17.03.2021 22:20, Florian Fainelli wrote:
> On 3/17/2021 7:37 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Simple macro like REG_RGMII_CNTRL_P() is insufficient as:
>> 1. It doesn't validate port argument
>> 2. It doesn't support chipsets with non-lineral RGMII regs layout
>>
>> Missing port validation could result in getting register offset from out
>> of array. Random memory -> random offset -> random reads/writes. It
>> affected e.g. BCM4908 for REG_RGMII_CNTRL_P(7).
>
> That is entirely fair, however as a bug fix this is not necessarily the
> simplest way to approach this.
I'm not sure if I understand. Should I fix it in some totally different
way? Or should I just follow your inline suggestions?
>> Fixes: a78e86ed586d ("net: dsa: bcm_sf2: Prepare for different register layouts")
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> drivers/net/dsa/bcm_sf2.c | 51 ++++++++++++++++++++++++++++++----
>> drivers/net/dsa/bcm_sf2_regs.h | 2 --
>> 2 files changed, 45 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
>> index ba5d546d06aa..942773bcb7e0 100644
>> --- a/drivers/net/dsa/bcm_sf2.c
>> +++ b/drivers/net/dsa/bcm_sf2.c
>> @@ -32,6 +32,30 @@
>> #include "b53/b53_priv.h"
>> #include "b53/b53_regs.h"
>>
>> +static u16 bcm_sf2_reg_rgmii_cntrl(struct bcm_sf2_priv *priv, int port)
>
> This is not meant to be used outside the file, so I would be keen on
> removing the bcm_sf2_ prefix to make the name shorter and closer to the
> original macro name.
Most or all local functions use such a prefix. E.g.:
bcm_sf2_num_active_ports()
bcm_sf2_recalc_clock()
bcm_sf2_imp_setup()
bcm_sf2_gphy_enable_set()
bcm_sf2_port_intr_enable()
bcm_sf2_port_intr_disable()
bcm_sf2_port_setup()
bcm_sf2_port_disable()
It would be inconsistent to have RGMII reg function not follow that.
>> +{
>> + switch (priv->type) {
>> + case BCM4908_DEVICE_ID:
>> + /* TODO */
>> + break;
>> + default:
>> + switch (port) {
>> + case 0:
>> + return REG_RGMII_0_CNTRL;
>> + case 1:
>> + return REG_RGMII_1_CNTRL;
>> + case 2:
>> + return REG_RGMII_2_CNTRL;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> + WARN_ONCE(1, "Unsupported port %d\n", port);
>> +
>> + return 0;
>
> maybe return -1 or -EINVAL just in case 0 happens to be a valid offset
> in the future. Checking the return value is not necessarily going to be
> helpful as it needs immediate fixing, so what we could do is keep the
> WARN_ON, and return the offset of REG_SWITCH_STATUS which is a read-only
> register. This will trigger the bus arbiter logic to return an error
> because a write was attempted from a read-only register.
>
> What do you think?
Great, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register
2021-03-18 7:30 ` Rafał Miłecki
@ 2021-03-18 16:16 ` Florian Fainelli
0 siblings, 0 replies; 6+ messages in thread
From: Florian Fainelli @ 2021-03-18 16:16 UTC (permalink / raw)
To: Rafał Miłecki, Andrew Lunn, Vivien Didelot, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
On 3/18/2021 12:30 AM, Rafał Miłecki wrote:
> On 17.03.2021 22:20, Florian Fainelli wrote:
>> On 3/17/2021 7:37 AM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Simple macro like REG_RGMII_CNTRL_P() is insufficient as:
>>> 1. It doesn't validate port argument
>>> 2. It doesn't support chipsets with non-lineral RGMII regs layout
>>>
>>> Missing port validation could result in getting register offset from out
>>> of array. Random memory -> random offset -> random reads/writes. It
>>> affected e.g. BCM4908 for REG_RGMII_CNTRL_P(7).
>>
>> That is entirely fair, however as a bug fix this is not necessarily the
>> simplest way to approach this.
>
> I'm not sure if I understand. Should I fix it in some totally different
> way? Or should I just follow your inline suggestions?
What I meant is that for a bug fix you could just mangled the offset of
the register such that REG_RGMII_CNTRL_P(7) would resole to the right
offset. That would be lying a little bit, but for a bug fix, that would
work. Not that it matters since the changes are still fresh in net/net-next.
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-18 16:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-17 14:37 [PATCH 0/2] net: dsa: bcm_sf2: refactor & update RGMII regs Rafał Miłecki
2021-03-17 14:37 ` [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register Rafał Miłecki
2021-03-17 21:20 ` Florian Fainelli
2021-03-18 7:30 ` Rafał Miłecki
2021-03-18 16:16 ` Florian Fainelli
2021-03-17 14:37 ` [PATCH 2/2] net: dsa: bcm_sf2: fix BCM4908 RGMII reg(s) Rafał Miłecki
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).