Re: [PATCH net-next 04/14] net: dsa: mv88e6xxx: rework ATU Load/Purge
Hi Andrew, Andrew Lunn writes: > It does result in more patches, more to review, but it is much easier > to review, because it should be obviously correct. The overall lines > of code at the end is the same. So overall there is no harm is having > lots of small patches. Your concerns are dully noted. I'll do my best to split my upcoming changes in smaller, easier to review, patches. Thanks! Vivien
Re: [PATCH net-next 04/14] net: dsa: mv88e6xxx: rework ATU Load/Purge
> > I really wished you had moved the code, unmodified, into > > global1_atu.c. Then made lots of easy to review small changes. I > > cannot just look at this patch and know it is correct. What i need to > > compare against is not in this patch. So it is a lot harder to review. > > I've addressed all of your comments in this patchset except this one. Hi Vivien This time, i'm not going to push the issue further. But next time i will. The point is, we have working code. You don't just throw that away. You make lots of small changes to that working code to morph it into the code you want. These small changes are all very quick and easy to review, because they are all small and obvious. At each stage we have working code. If somehow it does break, it is easy to bisect it down to one small change. The actual likelyhood of breaking it is small, because the changes are all small and obviously correct. It does result in more patches, more to review, but it is much easier to review, because it should be obviously correct. The overall lines of code at the end is the same. So overall there is no harm is having lots of small patches. > A patch file cannot guarantee that a chunk of code moved around has not > been altered in the process. This will just generate more diff for no > value, that needs to be updated afterwards anyway. I don't need a guarantee. I would trust you have moved it, without editing it. Generating more diff is not a problem. The number of diffs is not important at all. What is important is lots of small, easy to review, obviously correct patches. > Plus you already complained in the first iteration I sent about > modifying lines that I previously added. I took care of logically > splitting the new ATU Load/Purge, GetNext, Flush and Remove operations > into incremental unmodified chunks in this series. Unfortunately, you are splitting in the wrong dimension. None of this is obviously correct. But it easily code of been. Andrew
Re: [PATCH net-next 04/14] net: dsa: mv88e6xxx: rework ATU Load/Purge
Hi Andrew, Andrew Lunn writes: > On Thu, Mar 09, 2017 at 06:33:14PM -0500, Vivien Didelot wrote: >> All Marvell switch chips have an ATU accessed using the same Global (1) >> register layout. Only the handling of the FID differs as more bits were >> necessary to support more and more databases. >> >> Add and use a fresh documented implementation of the ATU Load/Purge. > > This is not really the Linux way of doing something. You don't throw > something away and replace it. You incrementally modify what you have > into something better. > > I really wished you had moved the code, unmodified, into > global1_atu.c. Then made lots of easy to review small changes. I > cannot just look at this patch and know it is correct. What i need to > compare against is not in this patch. So it is a lot harder to review. I've addressed all of your comments in this patchset except this one. A patch file cannot guarantee that a chunk of code moved around has not been altered in the process. This will just generate more diff for no value, that needs to be updated afterwards anyway. Plus you already complained in the first iteration I sent about modifying lines that I previously added. I took care of logically splitting the new ATU Load/Purge, GetNext, Flush and Remove operations into incremental unmodified chunks in this series. I have updated the commit messages to detail the addition of the static helpers and their counterparts, but I will resend this patch as is. Thanks, Vivien
Re: [PATCH net-next 04/14] net: dsa: mv88e6xxx: rework ATU Load/Purge
On Thu, Mar 09, 2017 at 06:33:14PM -0500, Vivien Didelot wrote: > All Marvell switch chips have an ATU accessed using the same Global (1) > register layout. Only the handling of the FID differs as more bits were > necessary to support more and more databases. > > Add and use a fresh documented implementation of the ATU Load/Purge. This is not really the Linux way of doing something. You don't throw something away and replace it. You incrementally modify what you have into something better. I really wished you had moved the code, unmodified, into global1_atu.c. Then made lots of easy to review small changes. I cannot just look at this patch and know it is correct. What i need to compare against is not in this patch. So it is a lot harder to review. I will continue this review later... Andrew
[PATCH net-next 04/14] net: dsa: mv88e6xxx: rework ATU Load/Purge
All Marvell switch chips have an ATU accessed using the same Global (1)
register layout. Only the handling of the FID differs as more bits were
necessary to support more and more databases.
Add and use a fresh documented implementation of the ATU Load/Purge.
Signed-off-by: Vivien Didelot
---
drivers/net/dsa/mv88e6xxx/chip.c| 22 +--
drivers/net/dsa/mv88e6xxx/global1.h | 2 +
drivers/net/dsa/mv88e6xxx/global1_atu.c | 108
3 files changed, 111 insertions(+), 21 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 132bda7ca9a8..eceb9b61679e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2047,26 +2047,6 @@ static int _mv88e6xxx_atu_mac_read(struct mv88e6xxx_chip
*chip,
return 0;
}
-static int _mv88e6xxx_atu_load(struct mv88e6xxx_chip *chip,
- struct mv88e6xxx_atu_entry *entry)
-{
- int ret;
-
- ret = _mv88e6xxx_atu_wait(chip);
- if (ret < 0)
- return ret;
-
- ret = _mv88e6xxx_atu_mac_write(chip, entry->mac);
- if (ret < 0)
- return ret;
-
- ret = _mv88e6xxx_atu_data_write(chip, entry);
- if (ret < 0)
- return ret;
-
- return _mv88e6xxx_atu_cmd(chip, entry->fid, GLOBAL_ATU_OP_LOAD_DB);
-}
-
static int _mv88e6xxx_atu_getnext(struct mv88e6xxx_chip *chip, u16 fid,
struct mv88e6xxx_atu_entry *entry);
@@ -2134,7 +2114,7 @@ static int mv88e6xxx_port_db_load_purge(struct
mv88e6xxx_chip *chip, int port,
entry.state = state;
}
- return _mv88e6xxx_atu_load(chip, &entry);
+ return mv88e6xxx_g1_atu_loadpurge(chip, vlan.fid, &entry);
}
static int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/mv88e6xxx/global1.h
b/drivers/net/dsa/mv88e6xxx/global1.h
index 18322d05225a..2c03f2e04639 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.h
+++ b/drivers/net/dsa/mv88e6xxx/global1.h
@@ -41,5 +41,7 @@ int mv88e6390_g1_mgmt_rsvd2cpu(struct mv88e6xxx_chip *chip);
int mv88e6xxx_g1_atu_set_learn2all(struct mv88e6xxx_chip *chip, bool
learn2all);
int mv88e6xxx_g1_atu_set_age_time(struct mv88e6xxx_chip *chip,
unsigned int msecs);
+int mv88e6xxx_g1_atu_loadpurge(struct mv88e6xxx_chip *chip, u16 fid,
+ struct mv88e6xxx_atu_entry *entry);
#endif /* _MV88E6XXX_GLOBAL1_H */
diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c
b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index 843a21e05f7b..09190559178b 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -13,6 +13,13 @@
#include "mv88e6xxx.h"
#include "global1.h"
+/* Offset 0x01: ATU FID Register */
+
+static int mv88e6xxx_g1_atu_fid_write(struct mv88e6xxx_chip *chip, u16 fid)
+{
+ return mv88e6xxx_g1_write(chip, GLOBAL_ATU_FID, fid & 0xfff);
+}
+
/* Offset 0x0A: ATU Control Register */
int mv88e6xxx_g1_atu_set_learn2all(struct mv88e6xxx_chip *chip, bool learn2all)
@@ -58,3 +65,104 @@ int mv88e6xxx_g1_atu_set_age_time(struct mv88e6xxx_chip
*chip,
return mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val);
}
+
+/* Offset 0x0B: ATU Operation Register */
+
+static int mv88e6xxx_g1_atu_op_wait(struct mv88e6xxx_chip *chip)
+{
+ return mv88e6xxx_g1_wait(chip, GLOBAL_ATU_OP, GLOBAL_ATU_OP_BUSY);
+}
+
+static int mv88e6xxx_g1_atu_op(struct mv88e6xxx_chip *chip, u16 fid, u16 op)
+{
+ u16 val;
+ int err;
+
+ /* FID bits are dispatched all around gradually as more are supported */
+ if (mv88e6xxx_num_databases(chip) > 256) {
+ err = mv88e6xxx_g1_atu_fid_write(chip, fid);
+ if (err)
+ return err;
+ } else {
+ if (mv88e6xxx_num_databases(chip) > 16) {
+ /* ATU DBNum[7:4] are located in ATU Control 15:12 */
+ err = mv88e6xxx_g1_read(chip, GLOBAL_ATU_CONTROL, &val);
+ if (err)
+ return err;
+
+ val = (val & 0x0fff) | ((fid << 8) & 0xf000);
+ err = mv88e6xxx_g1_write(chip, GLOBAL_ATU_CONTROL, val);
+ if (err)
+ return err;
+ }
+
+ /* ATU DBNum[3:0] are located in ATU Operation 3:0 */
+ op |= fid & 0xf;
+ }
+
+ err = mv88e6xxx_g1_write(chip, GLOBAL_ATU_OP, op);
+ if (err)
+ return err;
+
+ return mv88e6xxx_g1_atu_op_wait(chip);
+}
+
+/* Offset 0x0C: ATU Data Register */
+
+static int mv88e6xxx_g1_atu_data_write(struct mv88e6xxx_chip *chip,
+ struct mv88e6xxx_atu_entry *entry)
+{
+ u16 data = entry->state & 0xf;
+
+ if (entry->state != GLOBAL_ATU_DATA_STATE_UNUSED) {
+
