| Message ID | 20210403114848.30528-4-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ar9331: mainline some parts of switch functionality | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: rmk+kernel@armlinux.org.uk linux@rempel-privat.de |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: Possible repeated word: 'this' |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Hi Oleksij Maybe add a short comment about why the order is important. > - ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val); > + ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2, > + val >> 16); > if (ret < 0) > goto error; > > - ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2, > - val >> 16); > + ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val); > if (ret < 0) > goto error; > > return 0; > + > error: > dev_err_ratelimited(&sbus->dev, "Bus error. Failed to write register.\n"); > return ret; With that: Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew > -- > 2.29.2 >
On 4/3/2021 04:48, Oleksij Rempel wrote: > In case of this switch we work with 32bit registers on top of 16bit > bus. Some registers (for example access to forwarding database) have > trigger bit on the first 16bit half of request and the result + > configuration of request in the second half. Without this this patch, we would > trigger database operation and overwrite result in one run. > > To make it work properly, we should do the second part of transfer > before the first one is done. > > So far, this rule seems to work for all registers on this switch. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Seems like you could send this as a separate bugfix for the "net" tree along with a Fixes tag?
diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c index ca2ad77b71f1..9a5035b2f0ff 100644 --- a/drivers/net/dsa/qca/ar9331.c +++ b/drivers/net/dsa/qca/ar9331.c @@ -837,16 +837,17 @@ static int ar9331_mdio_write(void *ctx, u32 reg, u32 val) return 0; } - ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val); + ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2, + val >> 16); if (ret < 0) goto error; - ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2, - val >> 16); + ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val); if (ret < 0) goto error; return 0; + error: dev_err_ratelimited(&sbus->dev, "Bus error. Failed to write register.\n"); return ret;
In case of this switch we work with 32bit registers on top of 16bit bus. Some registers (for example access to forwarding database) have trigger bit on the first 16bit half of request and the result + configuration of request in the second half. Without this this patch, we would trigger database operation and overwrite result in one run. To make it work properly, we should do the second part of transfer before the first one is done. So far, this rule seems to work for all registers on this switch. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/dsa/qca/ar9331.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)