Message ID | 20190823212603.13456-9-marek.behun@nic.cz |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | net: dsa: mv88e6xxx: Peridot/Topaz SERDES changes | expand |
Hi Marek, On Fri, 23 Aug 2019 23:26:02 +0200, Marek Behún <marek.behun@nic.cz> wrote: > -int mv88e6xxx_port_hidden_write(struct mv88e6xxx_chip *chip, int port, int reg, > - u16 val); > +int mv88e6xxx_port_hidden_write(struct mv88e6xxx_chip *chip, int block, int port, > + int reg, u16 val); > int mv88e6xxx_port_hidden_wait(struct mv88e6xxx_chip *chip); > -int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int port, int reg, > - u16 *val); > +int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int block, int port, > + int reg, u16 *val); There's something I'm having trouble to follow here. This series keeps adding and modifying its own code. Wouldn't it be simpler for everyone if you directly implement the final mv88e6xxx_port_hidden_{read,write} functions taking this block argument, and update the code to switch to it? While at it, I don't really mind the "hidden" name, but is this the name used in the documentation, if any? Thank for you patience, Vivien
On Sat, 24 Aug 2019 16:13:28 -0400 Vivien Didelot <vivien.didelot@gmail.com> wrote: > Hi Marek, > > On Fri, 23 Aug 2019 23:26:02 +0200, Marek Behún <marek.behun@nic.cz> wrote: > > -int mv88e6xxx_port_hidden_write(struct mv88e6xxx_chip *chip, int port, int reg, > > - u16 val); > > +int mv88e6xxx_port_hidden_write(struct mv88e6xxx_chip *chip, int block, int port, > > + int reg, u16 val); > > int mv88e6xxx_port_hidden_wait(struct mv88e6xxx_chip *chip); > > -int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int port, int reg, > > - u16 *val); > > +int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int block, int port, > > + int reg, u16 *val); > > > There's something I'm having trouble to follow here. This series keeps > adding and modifying its own code. Wouldn't it be simpler for everyone > if you directly implement the final mv88e6xxx_port_hidden_{read,write} > functions taking this block argument, and update the code to switch to it? I wanted the commits to be atomic, in the sense that one commit does not do three different things at once. Renaming macros is cosmetic change, and moving functions to another file is a not a semantic change, while adding additional argument to functions is a semantic change. I can of course do all in one patch, but I though it would be better not to. > While at it, I don't really mind the "hidden" name, but is this the name > used in the documentation, if any? Yes, the registers are indeed named Hidden Registers in documentation.
Hi Marek, On Sat, 24 Aug 2019 22:52:16 +0200, Marek Behun <marek.behun@nic.cz> wrote: > > There's something I'm having trouble to follow here. This series keeps > > adding and modifying its own code. Wouldn't it be simpler for everyone > > if you directly implement the final mv88e6xxx_port_hidden_{read,write} > > functions taking this block argument, and update the code to switch to it? > > I wanted the commits to be atomic, in the sense that one commit does > not do three different things at once. Renaming macros is cosmetic > change, and moving functions to another file is a not a semantic > change, while adding additional argument to functions is a semantic > change. I can of course do all in one patch, but I though it would be > better not to. You add code, move it, rename it, then change it. It is hard to follow and read, especially in a series of 9 patches. I think you could do it the other way around. For example implement the .serdes_get_lane operation, its users, the mv88e6xxx_port_hidden_* API, its users, remove or convert old code, etc. Atomicity has nothing to do with it. > > While at it, I don't really mind the "hidden" name, but is this the name > > used in the documentation, if any? > > Yes, the registers are indeed named Hidden Registers in documentation. OK good to know, port_hidden_ makes sense indeed then. Thanks, Vivien
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 43cb48e2ef5f..202ccce65b1c 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -2325,7 +2325,7 @@ static bool mv88e6390_setup_errata_applied(struct mv88e6xxx_chip *chip) u16 val; for (port = 0; port < mv88e6xxx_num_ports(chip); port++) { - err = mv88e6xxx_port_hidden_read(chip, port, 0, &val); + err = mv88e6xxx_port_hidden_read(chip, 0xf, port, 0, &val); if (err) { dev_err(chip->dev, "Error reading hidden register: %d\n", err); @@ -2358,7 +2358,7 @@ static int mv88e6390_setup_errata(struct mv88e6xxx_chip *chip) } for (port = 0; port < mv88e6xxx_num_ports(chip); port++) { - err = mv88e6xxx_port_hidden_write(chip, port, 0, 0x01c0); + err = mv88e6xxx_port_hidden_write(chip, 0xf, port, 0, 0x01c0); if (err) return err; } diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h index cd7aa7392dfe..04550cb3c3b3 100644 --- a/drivers/net/dsa/mv88e6xxx/port.h +++ b/drivers/net/dsa/mv88e6xxx/port.h @@ -266,7 +266,7 @@ #define MV88E6XXX_PORT_RESERVED_1A_WRITE 0x4000 #define MV88E6XXX_PORT_RESERVED_1A_READ 0x0000 #define MV88E6XXX_PORT_RESERVED_1A_PORT_SHIFT 5 -#define MV88E6XXX_PORT_RESERVED_1A_BLOCK 0x3c00 +#define MV88E6XXX_PORT_RESERVED_1A_BLOCK_SHIFT 10 #define MV88E6XXX_PORT_RESERVED_1A_CTRL_PORT 0x04 #define MV88E6XXX_PORT_RESERVED_1A_DATA_PORT 0x05 @@ -353,10 +353,10 @@ int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port, int mv88e6xxx_port_disable_learn_limit(struct mv88e6xxx_chip *chip, int port); int mv88e6xxx_port_disable_pri_override(struct mv88e6xxx_chip *chip, int port); -int mv88e6xxx_port_hidden_write(struct mv88e6xxx_chip *chip, int port, int reg, - u16 val); +int mv88e6xxx_port_hidden_write(struct mv88e6xxx_chip *chip, int block, int port, + int reg, u16 val); int mv88e6xxx_port_hidden_wait(struct mv88e6xxx_chip *chip); -int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int port, int reg, - u16 *val); +int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int block, int port, + int reg, u16 *val); #endif /* _MV88E6XXX_PORT_H */ diff --git a/drivers/net/dsa/mv88e6xxx/port_hidden.c b/drivers/net/dsa/mv88e6xxx/port_hidden.c index 37520b6b8c89..fc0a45cb4f68 100644 --- a/drivers/net/dsa/mv88e6xxx/port_hidden.c +++ b/drivers/net/dsa/mv88e6xxx/port_hidden.c @@ -15,8 +15,8 @@ /* The mv88e6390 and mv88e6341 have some hidden registers used for debug and * development. The errata also makes use of them. */ -int mv88e6xxx_port_hidden_write(struct mv88e6xxx_chip *chip, int port, int reg, - u16 val) +int mv88e6xxx_port_hidden_write(struct mv88e6xxx_chip *chip, int block, int port, + int reg, u16 val) { u16 ctrl; int err; @@ -28,7 +28,7 @@ int mv88e6xxx_port_hidden_write(struct mv88e6xxx_chip *chip, int port, int reg, ctrl = MV88E6XXX_PORT_RESERVED_1A_BUSY | MV88E6XXX_PORT_RESERVED_1A_WRITE | - MV88E6XXX_PORT_RESERVED_1A_BLOCK | + block << MV88E6XXX_PORT_RESERVED_1A_BLOCK_SHIFT | port << MV88E6XXX_PORT_RESERVED_1A_PORT_SHIFT | reg; @@ -44,15 +44,15 @@ int mv88e6xxx_port_hidden_wait(struct mv88e6xxx_chip *chip) MV88E6XXX_PORT_RESERVED_1A, bit, 0); } -int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int port, int reg, - u16 *val) +int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int block, int port, + int reg, u16 *val) { u16 ctrl; int err; ctrl = MV88E6XXX_PORT_RESERVED_1A_BUSY | MV88E6XXX_PORT_RESERVED_1A_READ | - MV88E6XXX_PORT_RESERVED_1A_BLOCK | + block << MV88E6XXX_PORT_RESERVED_1A_BLOCK_SHIFT | port << MV88E6XXX_PORT_RESERVED_1A_PORT_SHIFT | reg;
Add support for setting the Block Address parameter when reading/writing hidden registers. Marvell's mdio examples for SERDES settings on Topaz use Block Address 0x7 when reading/writing hidden registers, although the specification says that block must be set to 0xf. Signed-off-by: Marek Behún <marek.behun@nic.cz> --- drivers/net/dsa/mv88e6xxx/chip.c | 4 ++-- drivers/net/dsa/mv88e6xxx/port.h | 10 +++++----- drivers/net/dsa/mv88e6xxx/port_hidden.c | 12 ++++++------ 3 files changed, 13 insertions(+), 13 deletions(-)