| Message ID | 20201029222509.27201-1-kabel@kernel.org |
|---|---|
| Headers | show |
| Series | Support for RollBall 10G copper SFP modules | expand |
A general point: please have me in the To: line rather than the Cc: line so that I can find emails better; I've just spent quite a while trying to find this series you posted last night amongst all the other emails that I'm Cc'd on. (The difference between To: and Cc: is that you expect those in the To: header to be the primary recipients who you expect action from, and those in the Cc: to be secondary recipients for information.) https://blog.thedigitalgroup.com/to-vs-cc-vs-bcc-how-to-use-them-correctly https://www.writebetteremails.com/to-cc.htm https://thinkproductive.co.uk/email-using-cc-bcc-to/ ... etc ... Thanks. On Thu, Oct 29, 2020 at 11:25:04PM +0100, Marek Behún wrote: > Hello, > > this is v2 of series adding support for RollBall/Hilink SFP modules. > > Checked with: > checkpatch.pl --max-line-length=80 > > Changes from v1: > - wrapped to 80 columns as per Russell's request > - initialization of RollBall MDIO I2C protocol moved from sfp.c to > mdio-i2c.c as per Russell's request > - second patch removes the 802.3z check also from phylink_sfp_config > as suggested by Russell > - creation/destruction of mdiobus for SFP now occurs before probing > for PHY/after releasing PHY (as suggested by Russell) > - the last patch became a little simpler after the above was done > > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Russell King <rmk+kernel@armlinux.org.uk> > > Marek Behún (5): > net: phy: mdio-i2c: support I2C MDIO protocol for RollBall SFP modules > net: phylink: allow attaching phy for SFP modules on 802.3z mode > net: sfp: create/destroy I2C mdiobus before PHY probe/after PHY > release > net: phy: marvell10g: change MACTYPE if underlying MAC does not > support it > net: sfp: add support for multigig RollBall transceivers > > drivers/net/mdio/mdio-i2c.c | 232 +++++++++++++++++++++++++++++++++- > drivers/net/phy/marvell10g.c | 31 +++++ > drivers/net/phy/phylink.c | 5 +- > drivers/net/phy/sfp.c | 67 ++++++++-- > include/linux/mdio/mdio-i2c.h | 8 +- > 5 files changed, 322 insertions(+), 21 deletions(-) > > > base-commit: cd29296fdfca919590e4004a7e4905544f4c4a32 > -- > 2.26.2 > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Oct 29, 2020 at 11:25:05PM +0100, Marek Behún wrote: > @@ -91,9 +94,210 @@ static int i2c_mii_write(struct mii_bus *bus, int phy_id, int reg, u16 val) > return ret < 0 ? ret : 0; > } > > -struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c) > +/* RollBall SFPs do not access internal PHY via I2C address 0x56, but > + * instead via address 0x51, when SFP page is set to 0x03 and password to > + * 0xffffffff: > + * > + * address size contents description > + * ------- ---- -------- ----------- > + * 0x80 1 CMD 0x01/0x02/0x04 for write/read/done > + * 0x81 1 DEV Clause 45 device > + * 0x82 2 REG Clause 45 register > + * 0x84 2 VAL Register value > + */ ... > +static int i2c_mii_init_rollball(struct i2c_adapter *i2c) > +{ > + u8 page_buf[2], pw_buf[5]; > + struct i2c_msg msgs[2]; > + int ret; > + > + page_buf[0] = SFP_PAGE; > + page_buf[1] = 3; > + > + msgs[0].addr = ROLLBALL_PHY_I2C_ADDR; > + msgs[0].flags = 0; > + msgs[0].len = sizeof(page_buf); > + msgs[0].buf = page_buf; > + > + pw_buf[0] = ROLLBALL_SFP_PASSWORD_ADDR; > + pw_buf[1] = 0xff; > + pw_buf[2] = 0xff; > + pw_buf[3] = 0xff; > + pw_buf[4] = 0xff; > + > + msgs[1].addr = ROLLBALL_PHY_I2C_ADDR; > + msgs[1].flags = 0; > + msgs[1].len = sizeof(pw_buf); > + msgs[1].buf = pw_buf; > + > + ret = i2c_transfer(i2c, msgs, ARRAY_SIZE(msgs)); > + if (ret < 0) > + return ret; > + else if (ret != ARRAY_SIZE(msgs)) > + return -EIO; > + > + return 0; > +} One of the points I raised in the previous review was: "Also, shouldn't we ensure that we are on page 1 before attempting any access?" I actually meant page 3 which I corrected when commenting on patch 5: "I think this needs to be done in the MDIO driver - if we have userspace or otherwise expand what we're doing, relying on page 3 remaining selected will be very fragile." I feel that point still stands; if the SFP page is changed after i2c_mii_init_rollball() and before a subsequent access, the subsequent access is not going to hit the correct page, and we will lose access to the PHY. Worse, we will end up writing to that other page. That said, I don't see anything in SFF8472 that would result in a clash, so I think I would like to see the comment at the beginning make explicit the expectation that SFP_PAGE will remain set to 3 throughout the lifetime of the module plugged into the system. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Oct 30, 2020 at 03:01:38PM +0000, Russell King - ARM Linux admin wrote:
> https://blog.thedigitalgroup.com/to-vs-cc-vs-bcc-how-to-use-them-correctly
I have to disagree about some of the information provided in this link:
------------------------------[cut here]------------------------------
Using the BCC Field:
BCC is for Blind Carbon Copy. It sends copies of the email to multiple
recipients, the only difference being that none of the recipients are
made aware of who else has received the email.
The BCC field is used when you want to send an email to multiple
recipients but do not want any of them to know about the other people
you have sent them to. There can be many scenarios where the BCC field
might be used, and the purpose might be a desire to keep the names of
the recipients a secret to one another and also protect the privacy of
recipients.
The most common application is for sending an email to a long list of
people who do not know each other, such as mailing lists. This protects
the privacy of the recipients as they are not able to view each other’s
email addresses.
------------------------------[cut here]------------------------------
It's plain stupid to put a mailing list in Bcc. I have filters that move
inbound emails from Inbox to separate folders based on the mailing list
from To: or CC:, except for emails where my address is also in To: or Cc:.
But when the mailing list is in Bcc, that email evades the filter and
arrives directly in my inbox, regardless of whether I'm even an intended
recipient or not.
On Thu, Oct 29, 2020 at 11:25:07PM +0100, Marek Behún wrote: > @@ -1936,8 +1950,10 @@ static void sfp_sm_main(struct sfp *sfp, unsigned int event) > sfp_sm_link_down(sfp); > if (sfp->sm_state > SFP_S_INIT) > sfp_module_stop(sfp->sfp_bus); > - if (sfp->mod_phy) > + if (sfp->mod_phy) { > sfp_sm_phy_detach(sfp); > + sfp_i2c_mdiobus_destroy(sfp); > + } if (sfp->mod_phy) sfp_sm_phy_detach(sfp); if (sfp->i2c_mii) sfp_i2c_mdiobus_destroy(sfp); would be better IMHO, in case we end up with the MDIO bus registered but don't discover a PHY. (which is entirely possible with Mikrotik SFPs where the PHY is not accessible.) Other than that, I don't see any obvious issues. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Oct 29, 2020 at 11:25:09PM +0100, Marek Behún wrote: > This adds support for multigig copper SFP modules from RollBall/Hilink. > These modules have a specific way to access clause 45 registers of the > internal PHY. > > We also need to wait at least 25 seconds after deasserting TX disable > before accessing the PHY. The code waits for 30 seconds just to be sure. > > Signed-off-by: Marek Behún <kabel@kernel.org> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Russell King <rmk+kernel@armlinux.org.uk> Reviewed-by: Russell King <rmk+kernel@armlinux.org.uk> Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hello, this is v2 of series adding support for RollBall/Hilink SFP modules. Checked with: checkpatch.pl --max-line-length=80 Changes from v1: - wrapped to 80 columns as per Russell's request - initialization of RollBall MDIO I2C protocol moved from sfp.c to mdio-i2c.c as per Russell's request - second patch removes the 802.3z check also from phylink_sfp_config as suggested by Russell - creation/destruction of mdiobus for SFP now occurs before probing for PHY/after releasing PHY (as suggested by Russell) - the last patch became a little simpler after the above was done Cc: Andrew Lunn <andrew@lunn.ch> Cc: Russell King <rmk+kernel@armlinux.org.uk> Marek Behún (5): net: phy: mdio-i2c: support I2C MDIO protocol for RollBall SFP modules net: phylink: allow attaching phy for SFP modules on 802.3z mode net: sfp: create/destroy I2C mdiobus before PHY probe/after PHY release net: phy: marvell10g: change MACTYPE if underlying MAC does not support it net: sfp: add support for multigig RollBall transceivers drivers/net/mdio/mdio-i2c.c | 232 +++++++++++++++++++++++++++++++++- drivers/net/phy/marvell10g.c | 31 +++++ drivers/net/phy/phylink.c | 5 +- drivers/net/phy/sfp.c | 67 ++++++++-- include/linux/mdio/mdio-i2c.h | 8 +- 5 files changed, 322 insertions(+), 21 deletions(-) base-commit: cd29296fdfca919590e4004a7e4905544f4c4a32