| Message ID | 20170725161553.30147-2-privat@egil-hjelmeland.no |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Egil, Egil Hjelmeland <privat@egil-hjelmeland.no> writes: > Fixes after testing on actual HW: > > - lan9303_mdio_write()/_read() must multiply register number > by 4 to get offset > > - Indirect access (PMI) to phy register only work in I2C mode. In > MDIO mode phy registers must be accessed directly. Introduced > struct lan9303_phy_ops to handle the two modes. Renamed functions > to clarify. > > - lan9303_detect_phy_setup() : Failed MDIO read return 0xffff. > Handle that. Small patch series when possible are better. Bullet points in commit messages are likely to describe how a patch or series may be split up ;-) This patch seems to be the unique patch of the series resolving what is described in the cover letter as "Make the MDIO interface work". I'd suggest you to split up this one commit in several *atomic* and easy to review patches and send them separately as on thread named "net: dsa: lan9303: fix MDIO interface" (also note that imperative is prefered for subject lines, see: https://chris.beams.io/posts/git-commit/#imperative) <...> > -static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip) > +static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip) For instance you can have a first commit only renaming the functions. The reason for it is to separate the functional changes from cosmetic changes, which makes it easier for review. <...> > - if (reg != 0) > + if ((reg != 0) && (reg != 0xffff)) if (reg && reg != 0xffff) should be enough. > chip->phy_addr_sel_strap = 1; > else > chip->phy_addr_sel_strap = 0; <...> > +struct lan9303; > + > +struct lan9303_phy_ops { > + /* PHY 1 &2 access*/ The spacing is weird in the comment. "/* PHY 1 & 2 access */" maybe? <...> > +int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val) > +{ > + struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev); > + struct mdio_device *mdio = sw_dev->device; > + > + mutex_lock(&mdio->bus->mdio_lock); > + mdio->bus->write(mdio->bus, phy, regnum, val); > + mutex_unlock(&mdio->bus->mdio_lock); This is exactly what mdiobus_write(mdio->bus, phy, regnum, val) is doing. There are very few valid reasons to go play in the mii_bus structure, using generic APIs are strongly prefered. Plus you have checks and traces for free! > + > + return 0; > +} > + > +int lan9303_mdio_phy_read(struct lan9303 *chip, int phy, int reg) > +{ > + struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev); > + struct mdio_device *mdio = sw_dev->device; > + int val; > + > + mutex_lock(&mdio->bus->mdio_lock); > + val = mdio->bus->read(mdio->bus, phy, reg); > + mutex_unlock(&mdio->bus->mdio_lock); Same here, mdiobus_read(). Thanks, Vivien
On 25. juli 2017 21:15, Vivien Didelot wrote: > Hi Egil, > > Egil Hjelmeland <privat@egil-hjelmeland.no> writes: > >> Fixes after testing on actual HW: >> >> - lan9303_mdio_write()/_read() must multiply register number >> by 4 to get offset >> >> - Indirect access (PMI) to phy register only work in I2C mode. In >> MDIO mode phy registers must be accessed directly. Introduced >> struct lan9303_phy_ops to handle the two modes. Renamed functions >> to clarify. >> >> - lan9303_detect_phy_setup() : Failed MDIO read return 0xffff. >> Handle that. > > Small patch series when possible are better. Bullet points in commit > messages are likely to describe how a patch or series may be split up > ;-) > > This patch seems to be the unique patch of the series resolving what is > described in the cover letter as "Make the MDIO interface work". > > I'd suggest you to split up this one commit in several *atomic* and easy > to review patches and send them separately as on thread named "net: dsa: > lan9303: fix MDIO interface" (also note that imperative is prefered for > subject lines, see: https://chris.beams.io/posts/git-commit/#imperative) > > <...> > >> -static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip) >> +static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip) > > For instance you can have a first commit only renaming the functions. > The reason for it is to separate the functional changes from cosmetic > changes, which makes it easier for review. > > <...> Thank you for reviewing. I can split the first patch. I can also split the patch series to more digestible series. But since most of the patches touches the same file, I assume that each series must be completed and applied before starting on a new one. So I really want to group the patches into only a few series in order to not spend months on the process. >> + if ((reg != 0) && (reg != 0xffff)) > > if (reg && reg != 0xffff) should be enough. Of course. >> +struct lan9303_phy_ops { >> + /* PHY 1 &2 access*/ > > The spacing is weird in the comment. "/* PHY 1 & 2 access */" maybe? > Yes. >> +int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val) >> +{ >> + struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev); >> + struct mdio_device *mdio = sw_dev->device; >> + >> + mutex_lock(&mdio->bus->mdio_lock); >> + mdio->bus->write(mdio->bus, phy, regnum, val); >> + mutex_unlock(&mdio->bus->mdio_lock); > > This is exactly what mdiobus_write(mdio->bus, phy, regnum, val) is > doing. There are very few valid reasons to go play in the mii_bus > structure, using generic APIs are strongly prefered. Plus you have > checks and traces for free! > Lack of oversight was the only reason. I just adapted stuff from lan9303_mdio_phy_write above. Will switch to mdiobus_write of course. > Same here, mdiobus_read(). > Ditto. > > Thanks, > > Vivien > Appreciated, Egil
Hi Egil, Egil Hjelmeland <privat@egil-hjelmeland.no> writes: >> I'd suggest you to split up this one commit in several *atomic* and easy >> to review patches and send them separately as on thread named "net: dsa: >> lan9303: fix MDIO interface" (also note that imperative is prefered for >> subject lines, see: https://chris.beams.io/posts/git-commit/#imperative) > > I can split the first patch. > > I can also split the patch series to more digestible series. But > since most of the patches touches the same file, I assume that each > series must be completed and applied before starting on a new one. > So I really want to group the patches into only a few series in order > to not spend months on the process. I understand. But believe me, your patches are very likely to land mainline faster if you send them in small chunks. This might not be true for every subsystems, but netdev is very responsive. This is even more true since this series has no-no (such as the sysfs entries) which guarantees the whole patch series to be rejected. Sending portions of your local work branch then rebase it against net-next/master is a usual development process. Thanks, Vivien
On 26. juli 2017 16:30, Vivien Didelot wrote: > Hi Egil, > > Egil Hjelmeland <privat@egil-hjelmeland.no> writes: > >>> I'd suggest you to split up this one commit in several *atomic* and easy >>> to review patches and send them separately as on thread named "net: dsa: >>> lan9303: fix MDIO interface" (also note that imperative is prefered for >>> subject lines, see: https://chris.beams.io/posts/git-commit/#imperative) >> >> I can split the first patch. >> >> I can also split the patch series to more digestible series. But >> since most of the patches touches the same file, I assume that each >> series must be completed and applied before starting on a new one. >> So I really want to group the patches into only a few series in order >> to not spend months on the process. > > I understand. But believe me, your patches are very likely to land > mainline faster if you send them in small chunks. This might not be true > for every subsystems, but netdev is very responsive. This is even more > true since this series has no-no (such as the sysfs entries) which > guarantees the whole patch series to be rejected. > > Sending portions of your local work branch then rebase it against > net-next/master is a usual development process. > > > Thanks, > > Vivien > Thank you for the advice. I got some other NMIs today that I have to serve. Hope to come back with MDIO patch series soon. Egil
On Tue, Jul 25, 2017 at 06:15:44PM +0200, Egil Hjelmeland wrote: > Fixes after testing on actual HW: > > - lan9303_mdio_write()/_read() must multiply register number > by 4 to get offset > > - Indirect access (PMI) to phy register only work in I2C mode. In > MDIO mode phy registers must be accessed directly. Introduced > struct lan9303_phy_ops to handle the two modes. Renamed functions > to clarify. > > - lan9303_detect_phy_setup() : Failed MDIO read return 0xffff. > Handle that. > > Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no> > --- > drivers/net/dsa/lan9303-core.c | 42 +++++++++++++++++++++++++++--------------- > drivers/net/dsa/lan9303.h | 11 +++++++++++ > drivers/net/dsa/lan9303_i2c.c | 2 ++ > drivers/net/dsa/lan9303_mdio.c | 34 ++++++++++++++++++++++++++++++++++ > 4 files changed, 74 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > index cd76e61f1fca..e622db586c3d 100644 > --- a/drivers/net/dsa/lan9303-core.c > +++ b/drivers/net/dsa/lan9303-core.c > @@ -20,6 +20,9 @@ > > #include "lan9303.h" > > +/* 13.2 System Control and Status Registers > + * Multiply register number by 4 to get address offset. > + */ > #define LAN9303_CHIP_REV 0x14 > # define LAN9303_CHIP_ID 0x9303 > #define LAN9303_IRQ_CFG 0x15 > @@ -53,6 +56,9 @@ > #define LAN9303_VIRT_PHY_BASE 0x70 > #define LAN9303_VIRT_SPECIAL_CTRL 0x77 > > +/*13.4 Switch Fabric Control and Status Registers > + * Accessed indirectly via SWITCH_CSR_CMD, SWITCH_CSR_DATA. > + */ > #define LAN9303_SW_DEV_ID 0x0000 > #define LAN9303_SW_RESET 0x0001 > #define LAN9303_SW_RESET_RESET BIT(0) > @@ -242,7 +248,7 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val) > return regmap_write(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, val); > } > > -static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip) > +static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip) > { > int ret, i; > u32 reg; > @@ -262,7 +268,7 @@ static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip) > return -EIO; > } > > -static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum) > +static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum) > { > int ret; > u32 val; > @@ -272,7 +278,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum) > > mutex_lock(&chip->indirect_mutex); > > - ret = lan9303_port_phy_reg_wait_for_completion(chip); > + ret = lan9303_indirect_phy_wait_for_completion(chip); > if (ret) > goto on_error; > > @@ -281,7 +287,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum) > if (ret) > goto on_error; > > - ret = lan9303_port_phy_reg_wait_for_completion(chip); > + ret = lan9303_indirect_phy_wait_for_completion(chip); > if (ret) > goto on_error; > > @@ -299,8 +305,8 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum) > return ret; > } > > -static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum, > - unsigned int val) > +static int lan9303_indirect_phy_write(struct lan9303 *chip, int addr, > + int regnum, u16 val) > { > int ret; > u32 reg; > @@ -311,7 +317,7 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum, > > mutex_lock(&chip->indirect_mutex); > > - ret = lan9303_port_phy_reg_wait_for_completion(chip); > + ret = lan9303_indirect_phy_wait_for_completion(chip); > if (ret) > goto on_error; > > @@ -328,6 +334,11 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum, > return ret; > } > > +const struct lan9303_phy_ops lan9303_indirect_phy_ops = { > + .phy_read = lan9303_indirect_phy_read, > + .phy_write = lan9303_indirect_phy_write, > +}; > + > static int lan9303_switch_wait_for_completion(struct lan9303 *chip) > { > int ret, i; > @@ -427,14 +438,15 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip) > * Special reg 18 of phy 3 reads as 0x0000, if 'phy_addr_sel_strap' is 0 > * and the IDs are 0-1-2, else it contains something different from > * 0x0000, which means 'phy_addr_sel_strap' is 1 and the IDs are 1-2-3. > + * 0xffff is returned for failed MDIO access. Hi Egil 0xffff is not really a failure. It just means there is nothing on the bus at that address. The bus has a weak pull-up, so defaults to one. > */ > - reg = lan9303_port_phy_reg_read(chip, 3, MII_LAN911X_SPECIAL_MODES); > + reg = chip->ops->phy_read(chip, 3, MII_LAN911X_SPECIAL_MODES); > if (reg < 0) { > dev_err(chip->dev, "Failed to detect phy config: %d\n", reg); > return reg; > } > > - if (reg != 0) > + if ((reg != 0) && (reg != 0xffff)) > chip->phy_addr_sel_strap = 1; > else > chip->phy_addr_sel_strap = 0; > @@ -719,7 +731,7 @@ static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum) > if (phy > phy_base + 2) > return -ENODEV; > > - return lan9303_port_phy_reg_read(chip, phy, regnum); > + return chip->ops->phy_read(chip, phy, regnum); > } > > static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum, > @@ -733,7 +745,7 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum, > if (phy > phy_base + 2) > return -ENODEV; > > - return lan9303_phy_reg_write(chip, phy, regnum, val); > + return chip->ops->phy_write(chip, phy, regnum, val); > } > > static int lan9303_port_enable(struct dsa_switch *ds, int port, > @@ -766,13 +778,13 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port, > switch (port) { > case 1: > lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET); > - lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1, > - MII_BMCR, BMCR_PDOWN); > + lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1, > + MII_BMCR, BMCR_PDOWN); > break; > case 2: > lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET); > - lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 2, > - MII_BMCR, BMCR_PDOWN); > + lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2, > + MII_BMCR, BMCR_PDOWN); > break; > default: > dev_dbg(chip->dev, > diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h > index d1512dad2d90..444d00b460e1 100644 > --- a/drivers/net/dsa/lan9303.h > +++ b/drivers/net/dsa/lan9303.h > @@ -2,6 +2,15 @@ > #include <linux/device.h> > #include <net/dsa.h> > > +struct lan9303; > + > +struct lan9303_phy_ops { > + /* PHY 1 &2 access*/ A space would be nice here after the &. > + int (*phy_read)(struct lan9303 *chip, int port, int regnum); > + int (*phy_write)(struct lan9303 *chip, int port, > + int regnum, u16 val); > +}; > + > struct lan9303 { > struct device *dev; > struct regmap *regmap; > @@ -11,9 +20,11 @@ struct lan9303 { > bool phy_addr_sel_strap; > struct dsa_switch *ds; > struct mutex indirect_mutex; /* protect indexed register access */ > + const struct lan9303_phy_ops *ops; > }; > > extern const struct regmap_access_table lan9303_register_set; > +extern const struct lan9303_phy_ops lan9303_indirect_phy_ops; > > int lan9303_probe(struct lan9303 *chip, struct device_node *np); > int lan9303_remove(struct lan9303 *chip); > diff --git a/drivers/net/dsa/lan9303_i2c.c b/drivers/net/dsa/lan9303_i2c.c > index ab3ce0da5071..24ec20f7f444 100644 > --- a/drivers/net/dsa/lan9303_i2c.c > +++ b/drivers/net/dsa/lan9303_i2c.c > @@ -63,6 +63,8 @@ static int lan9303_i2c_probe(struct i2c_client *client, > i2c_set_clientdata(client, sw_dev); > sw_dev->chip.dev = &client->dev; > > + sw_dev->chip.ops = &lan9303_indirect_phy_ops; > + > ret = lan9303_probe(&sw_dev->chip, client->dev.of_node); > if (ret != 0) > return ret; > diff --git a/drivers/net/dsa/lan9303_mdio.c b/drivers/net/dsa/lan9303_mdio.c > index 93c36c0541cf..94df12c5362f 100644 > --- a/drivers/net/dsa/lan9303_mdio.c > +++ b/drivers/net/dsa/lan9303_mdio.c > @@ -39,6 +39,7 @@ static void lan9303_mdio_real_write(struct mdio_device *mdio, int reg, u16 val) > static int lan9303_mdio_write(void *ctx, uint32_t reg, uint32_t val) > { > struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx; > + reg <<= 2; /* reg num to offset */ > > mutex_lock(&sw_dev->device->bus->mdio_lock); > lan9303_mdio_real_write(sw_dev->device, reg, val & 0xffff); > @@ -56,6 +57,7 @@ static u16 lan9303_mdio_real_read(struct mdio_device *mdio, int reg) > static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val) > { > struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx; > + reg <<= 2; /* reg num to offset */ > > mutex_lock(&sw_dev->device->bus->mdio_lock); > *val = lan9303_mdio_real_read(sw_dev->device, reg); > @@ -65,6 +67,36 @@ static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val) > return 0; > } > > +int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val) > +{ > + struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev); > + struct mdio_device *mdio = sw_dev->device; > + > + mutex_lock(&mdio->bus->mdio_lock); > + mdio->bus->write(mdio->bus, phy, regnum, val); > + mutex_unlock(&mdio->bus->mdio_lock); It is better to use mdiobus_read/write or if you are nesting mdio busses, mdiobus_read_nested/mdiobus_write_nested. Please test this code with lockdep enabled. And as others have pointed out, there are too many changes in this one patch. Thanks Andrew
> > So I really want to group the patches into only a few series in order > > to not spend months on the process. I strongly agree with Vivien here. Good patches get accepted in about 3 days. You should expect feedback within a day or two. That allows you to have fast cycle times for getting patches in. Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: Wed, 26 Jul 2017 19:52:24 +0200 >> > So I really want to group the patches into only a few series in order >> > to not spend months on the process. > > I strongly agree with Vivien here. Good patches get accepted in about > 3 days. You should expect feedback within a day or two. That allows > you to have fast cycle times for getting patches in. +1 Small simple patches will get everything in 10 times fast than if you clump everything together into larger, harder to review ones.
Den 26. juli 2017 22:07, skrev David Miller: > From: Andrew Lunn <andrew@lunn.ch> > Date: Wed, 26 Jul 2017 19:52:24 +0200 > >>>> So I really want to group the patches into only a few series in order >>>> to not spend months on the process. >> >> I strongly agree with Vivien here. Good patches get accepted in about >> 3 days. You should expect feedback within a day or two. That allows >> you to have fast cycle times for getting patches in. > > +1 > > Small simple patches will get everything in 10 times fast than if > you clump everything together into larger, harder to review ones. > Good. Just one question about process. Could I have posted my work as a RFC? To get one round of initial feedback before chopping into small patch requests. As well as indicating where I am heading. Or is that just waste of human bandwidth? Egil
> Good. Just one question about process. Could I have posted my work > as a RFC? To get one round of initial feedback before chopping into > small patch requests. As well as indicating where I am heading. Or is > that just waste of human bandwidth? Depends. Post 100 RFC patches, i won't look at them. Post 21 with a cover note making it clear you are planning to submit them in blocks of 7, i might. But it is best to assume reviewers have small blocks of time. 21 patches take 3 times a long to review as 7. The block of time might not be enough for 21, so the review gets differed. 7 are more likely to fit in the available time, so it happens quickly. Andrew
Hi Egil,
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Egil-Hjelmeland/net-dsa-lan9303-unicast-offload-fdb-mdb-STP/20170727-074246
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> ERROR: "lan9303_indirect_phy_ops" [drivers/net/dsa/lan9303_i2c.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 26. juli 2017 18:55, Andrew Lunn wrote: > On Tue, Jul 25, 2017 at 06:15:44PM +0200, Egil Hjelmeland wrote: > It is better to use mdiobus_read/write or if you are nesting mdio > busses, mdiobus_read_nested/mdiobus_write_nested. Please test this > code with lockdep enabled. > I have CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_MUTEXES. Should I enable more? Egil
On Fri, Jul 28, 2017 at 01:08:25PM +0200, Egil Hjelmeland wrote: > On 26. juli 2017 18:55, Andrew Lunn wrote: > >On Tue, Jul 25, 2017 at 06:15:44PM +0200, Egil Hjelmeland wrote: > >It is better to use mdiobus_read/write or if you are nesting mdio > >busses, mdiobus_read_nested/mdiobus_write_nested. Please test this > >code with lockdep enabled. > > > > I have CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_MUTEXES. Should I enable > more? Hi Egil Enable CONFIG_LOCKDEP and CONFIG_PROVE_LOCKING. Any lockdep splat you get while accessing the mdio bus at this point are probably false positives, since it is a different mutex. Using the _nested() version should avoid these false positives. But you might find other places your locking is not right. Andrew
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index cd76e61f1fca..e622db586c3d 100644 --- a/drivers/net/dsa/lan9303-core.c +++ b/drivers/net/dsa/lan9303-core.c @@ -20,6 +20,9 @@ #include "lan9303.h" +/* 13.2 System Control and Status Registers + * Multiply register number by 4 to get address offset. + */ #define LAN9303_CHIP_REV 0x14 # define LAN9303_CHIP_ID 0x9303 #define LAN9303_IRQ_CFG 0x15 @@ -53,6 +56,9 @@ #define LAN9303_VIRT_PHY_BASE 0x70 #define LAN9303_VIRT_SPECIAL_CTRL 0x77 +/*13.4 Switch Fabric Control and Status Registers + * Accessed indirectly via SWITCH_CSR_CMD, SWITCH_CSR_DATA. + */ #define LAN9303_SW_DEV_ID 0x0000 #define LAN9303_SW_RESET 0x0001 #define LAN9303_SW_RESET_RESET BIT(0) @@ -242,7 +248,7 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val) return regmap_write(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, val); } -static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip) +static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip) { int ret, i; u32 reg; @@ -262,7 +268,7 @@ static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip) return -EIO; } -static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum) +static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum) { int ret; u32 val; @@ -272,7 +278,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum) mutex_lock(&chip->indirect_mutex); - ret = lan9303_port_phy_reg_wait_for_completion(chip); + ret = lan9303_indirect_phy_wait_for_completion(chip); if (ret) goto on_error; @@ -281,7 +287,7 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum) if (ret) goto on_error; - ret = lan9303_port_phy_reg_wait_for_completion(chip); + ret = lan9303_indirect_phy_wait_for_completion(chip); if (ret) goto on_error; @@ -299,8 +305,8 @@ static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum) return ret; } -static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum, - unsigned int val) +static int lan9303_indirect_phy_write(struct lan9303 *chip, int addr, + int regnum, u16 val) { int ret; u32 reg; @@ -311,7 +317,7 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum, mutex_lock(&chip->indirect_mutex); - ret = lan9303_port_phy_reg_wait_for_completion(chip); + ret = lan9303_indirect_phy_wait_for_completion(chip); if (ret) goto on_error; @@ -328,6 +334,11 @@ static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum, return ret; } +const struct lan9303_phy_ops lan9303_indirect_phy_ops = { + .phy_read = lan9303_indirect_phy_read, + .phy_write = lan9303_indirect_phy_write, +}; + static int lan9303_switch_wait_for_completion(struct lan9303 *chip) { int ret, i; @@ -427,14 +438,15 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip) * Special reg 18 of phy 3 reads as 0x0000, if 'phy_addr_sel_strap' is 0 * and the IDs are 0-1-2, else it contains something different from * 0x0000, which means 'phy_addr_sel_strap' is 1 and the IDs are 1-2-3. + * 0xffff is returned for failed MDIO access. */ - reg = lan9303_port_phy_reg_read(chip, 3, MII_LAN911X_SPECIAL_MODES); + reg = chip->ops->phy_read(chip, 3, MII_LAN911X_SPECIAL_MODES); if (reg < 0) { dev_err(chip->dev, "Failed to detect phy config: %d\n", reg); return reg; } - if (reg != 0) + if ((reg != 0) && (reg != 0xffff)) chip->phy_addr_sel_strap = 1; else chip->phy_addr_sel_strap = 0; @@ -719,7 +731,7 @@ static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum) if (phy > phy_base + 2) return -ENODEV; - return lan9303_port_phy_reg_read(chip, phy, regnum); + return chip->ops->phy_read(chip, phy, regnum); } static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum, @@ -733,7 +745,7 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum, if (phy > phy_base + 2) return -ENODEV; - return lan9303_phy_reg_write(chip, phy, regnum, val); + return chip->ops->phy_write(chip, phy, regnum, val); } static int lan9303_port_enable(struct dsa_switch *ds, int port, @@ -766,13 +778,13 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port, switch (port) { case 1: lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET); - lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1, - MII_BMCR, BMCR_PDOWN); + lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1, + MII_BMCR, BMCR_PDOWN); break; case 2: lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET); - lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 2, - MII_BMCR, BMCR_PDOWN); + lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2, + MII_BMCR, BMCR_PDOWN); break; default: dev_dbg(chip->dev, diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h index d1512dad2d90..444d00b460e1 100644 --- a/drivers/net/dsa/lan9303.h +++ b/drivers/net/dsa/lan9303.h @@ -2,6 +2,15 @@ #include <linux/device.h> #include <net/dsa.h> +struct lan9303; + +struct lan9303_phy_ops { + /* PHY 1 &2 access*/ + int (*phy_read)(struct lan9303 *chip, int port, int regnum); + int (*phy_write)(struct lan9303 *chip, int port, + int regnum, u16 val); +}; + struct lan9303 { struct device *dev; struct regmap *regmap; @@ -11,9 +20,11 @@ struct lan9303 { bool phy_addr_sel_strap; struct dsa_switch *ds; struct mutex indirect_mutex; /* protect indexed register access */ + const struct lan9303_phy_ops *ops; }; extern const struct regmap_access_table lan9303_register_set; +extern const struct lan9303_phy_ops lan9303_indirect_phy_ops; int lan9303_probe(struct lan9303 *chip, struct device_node *np); int lan9303_remove(struct lan9303 *chip); diff --git a/drivers/net/dsa/lan9303_i2c.c b/drivers/net/dsa/lan9303_i2c.c index ab3ce0da5071..24ec20f7f444 100644 --- a/drivers/net/dsa/lan9303_i2c.c +++ b/drivers/net/dsa/lan9303_i2c.c @@ -63,6 +63,8 @@ static int lan9303_i2c_probe(struct i2c_client *client, i2c_set_clientdata(client, sw_dev); sw_dev->chip.dev = &client->dev; + sw_dev->chip.ops = &lan9303_indirect_phy_ops; + ret = lan9303_probe(&sw_dev->chip, client->dev.of_node); if (ret != 0) return ret; diff --git a/drivers/net/dsa/lan9303_mdio.c b/drivers/net/dsa/lan9303_mdio.c index 93c36c0541cf..94df12c5362f 100644 --- a/drivers/net/dsa/lan9303_mdio.c +++ b/drivers/net/dsa/lan9303_mdio.c @@ -39,6 +39,7 @@ static void lan9303_mdio_real_write(struct mdio_device *mdio, int reg, u16 val) static int lan9303_mdio_write(void *ctx, uint32_t reg, uint32_t val) { struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx; + reg <<= 2; /* reg num to offset */ mutex_lock(&sw_dev->device->bus->mdio_lock); lan9303_mdio_real_write(sw_dev->device, reg, val & 0xffff); @@ -56,6 +57,7 @@ static u16 lan9303_mdio_real_read(struct mdio_device *mdio, int reg) static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val) { struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx; + reg <<= 2; /* reg num to offset */ mutex_lock(&sw_dev->device->bus->mdio_lock); *val = lan9303_mdio_real_read(sw_dev->device, reg); @@ -65,6 +67,36 @@ static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val) return 0; } +int lan9303_mdio_phy_write(struct lan9303 *chip, int phy, int regnum, u16 val) +{ + struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev); + struct mdio_device *mdio = sw_dev->device; + + mutex_lock(&mdio->bus->mdio_lock); + mdio->bus->write(mdio->bus, phy, regnum, val); + mutex_unlock(&mdio->bus->mdio_lock); + + return 0; +} + +int lan9303_mdio_phy_read(struct lan9303 *chip, int phy, int reg) +{ + struct lan9303_mdio *sw_dev = dev_get_drvdata(chip->dev); + struct mdio_device *mdio = sw_dev->device; + int val; + + mutex_lock(&mdio->bus->mdio_lock); + val = mdio->bus->read(mdio->bus, phy, reg); + mutex_unlock(&mdio->bus->mdio_lock); + + return val; +} + +static const struct lan9303_phy_ops lan9303_mdio_phy_ops = { + .phy_read = lan9303_mdio_phy_read, + .phy_write = lan9303_mdio_phy_write, +}; + static const struct regmap_config lan9303_mdio_regmap_config = { .reg_bits = 8, .val_bits = 32, @@ -106,6 +138,8 @@ static int lan9303_mdio_probe(struct mdio_device *mdiodev) dev_set_drvdata(&mdiodev->dev, sw_dev); sw_dev->chip.dev = &mdiodev->dev; + sw_dev->chip.ops = &lan9303_mdio_phy_ops; + ret = lan9303_probe(&sw_dev->chip, mdiodev->dev.of_node); if (ret != 0) return ret;
Fixes after testing on actual HW: - lan9303_mdio_write()/_read() must multiply register number by 4 to get offset - Indirect access (PMI) to phy register only work in I2C mode. In MDIO mode phy registers must be accessed directly. Introduced struct lan9303_phy_ops to handle the two modes. Renamed functions to clarify. - lan9303_detect_phy_setup() : Failed MDIO read return 0xffff. Handle that. Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no> --- drivers/net/dsa/lan9303-core.c | 42 +++++++++++++++++++++++++++--------------- drivers/net/dsa/lan9303.h | 11 +++++++++++ drivers/net/dsa/lan9303_i2c.c | 2 ++ drivers/net/dsa/lan9303_mdio.c | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 15 deletions(-)