| Message ID | 20190508211330.19328-2-m.grzeschik@pengutronix.de |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | microchip: add support for ksz8863 driver family | expand |
On Wed, May 08, 2019 at 11:13:28PM +0200, Michael Grzeschik wrote: > Some microchip phys support the Serial Management Interface Protocol > (SMI) for the configuration of the extended register set. We add > MII_ADDR_SMI0 as an availabe interface to the mdiobb write and read > functions, as this interface can be easy realized using the bitbang mdio > driver. > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/net/phy/mdio-bitbang.c | 10 ++++++++++ > include/linux/phy.h | 12 ++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/net/phy/mdio-bitbang.c b/drivers/net/phy/mdio-bitbang.c > index 5136275c8e739..a978f8a9a172b 100644 > --- a/drivers/net/phy/mdio-bitbang.c > +++ b/drivers/net/phy/mdio-bitbang.c > @@ -22,6 +22,10 @@ > #define MDIO_READ 2 > #define MDIO_WRITE 1 > > +#define SMI0_RW_OPCODE 0 > +#define SMI0_READ_PHY (1 << 4) > +#define SMI0_WRITE_PHY (0 << 4) > + > #define MDIO_C45 (1<<15) > #define MDIO_C45_ADDR (MDIO_C45 | 0) > #define MDIO_C45_READ (MDIO_C45 | 3) > @@ -157,6 +161,9 @@ static int mdiobb_read(struct mii_bus *bus, int phy, int reg) > if (reg & MII_ADDR_C45) { > reg = mdiobb_cmd_addr(ctrl, phy, reg); > mdiobb_cmd(ctrl, MDIO_C45_READ, phy, reg); > + } else if (reg & MII_ADDR_SMI0) { > + mdiobb_cmd(ctrl, SMI0_RW_OPCODE, > + (reg & 0xE0) >> 5 | SMI0_READ_PHY, reg); > } else > mdiobb_cmd(ctrl, MDIO_READ, phy, reg); > > @@ -188,6 +195,9 @@ static int mdiobb_write(struct mii_bus *bus, int phy, int reg, u16 val) > if (reg & MII_ADDR_C45) { > reg = mdiobb_cmd_addr(ctrl, phy, reg); > mdiobb_cmd(ctrl, MDIO_C45_WRITE, phy, reg); > + } else if (reg & MII_ADDR_SMI0) { > + mdiobb_cmd(ctrl, SMI0_RW_OPCODE, > + (reg & 0xE0) >> 5 | SMI0_WRITE_PHY, reg); > } else > mdiobb_cmd(ctrl, MDIO_WRITE, phy, reg); > > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 073fb151b5a99..f011722fbd5c2 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -199,6 +199,18 @@ static inline const char *phy_modes(phy_interface_t interface) > IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy chips. */ > #define MII_ADDR_C45 (1<<30) > > +/* Serial Management Interface (SMI) uses the following frame format: > + * > + * preamble|start|Read/Write| PHY | REG |TA| Data bits | Idle > + * |frame| OP code |address |address| | | > + * read | 32x1´s | 01 | 00 | 1xRRR | RRRRR |Z0| 00000000DDDDDDDD | Z > + * write| 32x1´s | 01 | 00 | 0xRRR | RRRRR |10| xxxxxxxxDDDDDDDD | Z > + * > + * The register number is encoded with the 5 least significant bits in REG > + * and the 3 most significant bits in PHY > + */ > +#define MII_ADDR_SMI0 (1<<31) > + Michael This is a Micrel Proprietary protocol. So we should reflect this in the name. MII_ADDR_MICREL_SMI? Why the 0? Are there different versions? Maybe replace all SMI0 with MICREL_SMI in mdio-bitbang.c When i look at this, i don't see how a normal MDIO bus driver is going to support this. Only the bit banging driver, or maybe a Micrel MDIO bus master hardware driver. So i think the diagram should be placed into mdio-bitbang.c, and it would be nice to add a diagram of standard SMI. Andrew
On Thu, May 09, 2019 at 04:29:25PM +0200, Andrew Lunn wrote: > On Wed, May 08, 2019 at 11:13:28PM +0200, Michael Grzeschik wrote: > > Some microchip phys support the Serial Management Interface Protocol > > (SMI) for the configuration of the extended register set. We add > > MII_ADDR_SMI0 as an availabe interface to the mdiobb write and read > > functions, as this interface can be easy realized using the bitbang mdio > > driver. > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > --- > > drivers/net/phy/mdio-bitbang.c | 10 ++++++++++ > > include/linux/phy.h | 12 ++++++++++++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/drivers/net/phy/mdio-bitbang.c b/drivers/net/phy/mdio-bitbang.c > > index 5136275c8e739..a978f8a9a172b 100644 > > --- a/drivers/net/phy/mdio-bitbang.c > > +++ b/drivers/net/phy/mdio-bitbang.c > > @@ -22,6 +22,10 @@ > > #define MDIO_READ 2 > > #define MDIO_WRITE 1 > > > > +#define SMI0_RW_OPCODE 0 > > +#define SMI0_READ_PHY (1 << 4) > > +#define SMI0_WRITE_PHY (0 << 4) > > + > > #define MDIO_C45 (1<<15) > > #define MDIO_C45_ADDR (MDIO_C45 | 0) > > #define MDIO_C45_READ (MDIO_C45 | 3) > > @@ -157,6 +161,9 @@ static int mdiobb_read(struct mii_bus *bus, int phy, int reg) > > if (reg & MII_ADDR_C45) { > > reg = mdiobb_cmd_addr(ctrl, phy, reg); > > mdiobb_cmd(ctrl, MDIO_C45_READ, phy, reg); > > + } else if (reg & MII_ADDR_SMI0) { > > + mdiobb_cmd(ctrl, SMI0_RW_OPCODE, > > + (reg & 0xE0) >> 5 | SMI0_READ_PHY, reg); > > } else > > mdiobb_cmd(ctrl, MDIO_READ, phy, reg); > > > > @@ -188,6 +195,9 @@ static int mdiobb_write(struct mii_bus *bus, int phy, int reg, u16 val) > > if (reg & MII_ADDR_C45) { > > reg = mdiobb_cmd_addr(ctrl, phy, reg); > > mdiobb_cmd(ctrl, MDIO_C45_WRITE, phy, reg); > > + } else if (reg & MII_ADDR_SMI0) { > > + mdiobb_cmd(ctrl, SMI0_RW_OPCODE, > > + (reg & 0xE0) >> 5 | SMI0_WRITE_PHY, reg); > > } else > > mdiobb_cmd(ctrl, MDIO_WRITE, phy, reg); > > > > diff --git a/include/linux/phy.h b/include/linux/phy.h > > index 073fb151b5a99..f011722fbd5c2 100644 > > --- a/include/linux/phy.h > > +++ b/include/linux/phy.h > > @@ -199,6 +199,18 @@ static inline const char *phy_modes(phy_interface_t interface) > > IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy chips. */ > > #define MII_ADDR_C45 (1<<30) > > > > +/* Serial Management Interface (SMI) uses the following frame format: > > + * > > + * preamble|start|Read/Write| PHY | REG |TA| Data bits | Idle > > + * |frame| OP code |address |address| | | > > + * read | 32x1´s | 01 | 00 | 1xRRR | RRRRR |Z0| 00000000DDDDDDDD | Z > > + * write| 32x1´s | 01 | 00 | 0xRRR | RRRRR |10| xxxxxxxxDDDDDDDD | Z > > + * > > + * The register number is encoded with the 5 least significant bits in REG > > + * and the 3 most significant bits in PHY > > + */ > > +#define MII_ADDR_SMI0 (1<<31) > > + > > Michael > > This is a Micrel Proprietary protocol. So we should reflect this in > the name. MII_ADDR_MICREL_SMI? Why the 0? Are there different > versions? Maybe replace all SMI0 with MICREL_SMI in mdio-bitbang.c There are two variants of the SMI interface. The KSZ8863/73/93 Products use the above Variant described as "SMI0". The KSZ8864/95 Products use another layout: preamble|start|Read/Write| PHY | REG |TA| Data bits | Idle |frame| OP code |address |address| | | read | 32x1´s | 01 | 10 | RR11R | RRRRR |Z0| 00000000DDDDDDDD | Z write| 32x1´s | 01 | 01 | RR11R | RRRRR |10| xxxxxxxxDDDDDDDD | Z So they describe their write/read operation in the OP code rather then the PHY address. We could change the SMI index to SMI_KSZ88X3 for the current SMI0 to give it a more descriptive name. Beside we never know if Microchip will add the same protocol to other numbered switches. What naming would you prefer? > When i look at this, i don't see how a normal MDIO bus driver is going > to support this. Only the bit banging driver, or maybe a Micrel MDIO > bus master hardware driver. So i think the diagram should be placed > into mdio-bitbang.c, and it would be nice to add a diagram of standard > SMI. Right. I will move the description to mdio-bitbang.c. Thanks, Michael
> > > +/* Serial Management Interface (SMI) uses the following frame format: > > > + * > > > + * preamble|start|Read/Write| PHY | REG |TA| Data bits | Idle > > > + * |frame| OP code |address |address| | | > > > + * read | 32x1´s | 01 | 00 | 1xRRR | RRRRR |Z0| 00000000DDDDDDDD | Z > > > + * write| 32x1´s | 01 | 00 | 0xRRR | RRRRR |10| xxxxxxxxDDDDDDDD | Z > > > + * > > > + * The register number is encoded with the 5 least significant bits in REG > > > + * and the 3 most significant bits in PHY > > > + */ > > > +#define MII_ADDR_SMI0 (1<<31) > > > + > > > > Michael > > > > This is a Micrel Proprietary protocol. So we should reflect this in > > the name. MII_ADDR_MICREL_SMI? Why the 0? Are there different > > versions? Maybe replace all SMI0 with MICREL_SMI in mdio-bitbang.c > > There are two variants of the SMI interface. Hi Michael O.K, that explains the 0. > > The KSZ8863/73/93 Products use the above Variant described as "SMI0". > > The KSZ8864/95 Products use another layout: > > preamble|start|Read/Write| PHY | REG |TA| Data bits | Idle > |frame| OP code |address |address| | | > read | 32x1´s | 01 | 10 | RR11R | RRRRR |Z0| 00000000DDDDDDDD | Z > write| 32x1´s | 01 | 01 | RR11R | RRRRR |10| xxxxxxxxDDDDDDDD | Z > > So they describe their write/read operation in the OP code rather then > the PHY address. At a first look, i think a standard MDIO bus controller can do this? If so, we don't need a second define, just some code in the switch driver which shuffles bits around. > > We could change the SMI index to SMI_KSZ88X3 for the current SMI0 to > give it a more descriptive name. That seems sensible. In the mv88e6xxx driver, we name things based on the first device to introduce the feature. Andrew
diff --git a/drivers/net/phy/mdio-bitbang.c b/drivers/net/phy/mdio-bitbang.c index 5136275c8e739..a978f8a9a172b 100644 --- a/drivers/net/phy/mdio-bitbang.c +++ b/drivers/net/phy/mdio-bitbang.c @@ -22,6 +22,10 @@ #define MDIO_READ 2 #define MDIO_WRITE 1 +#define SMI0_RW_OPCODE 0 +#define SMI0_READ_PHY (1 << 4) +#define SMI0_WRITE_PHY (0 << 4) + #define MDIO_C45 (1<<15) #define MDIO_C45_ADDR (MDIO_C45 | 0) #define MDIO_C45_READ (MDIO_C45 | 3) @@ -157,6 +161,9 @@ static int mdiobb_read(struct mii_bus *bus, int phy, int reg) if (reg & MII_ADDR_C45) { reg = mdiobb_cmd_addr(ctrl, phy, reg); mdiobb_cmd(ctrl, MDIO_C45_READ, phy, reg); + } else if (reg & MII_ADDR_SMI0) { + mdiobb_cmd(ctrl, SMI0_RW_OPCODE, + (reg & 0xE0) >> 5 | SMI0_READ_PHY, reg); } else mdiobb_cmd(ctrl, MDIO_READ, phy, reg); @@ -188,6 +195,9 @@ static int mdiobb_write(struct mii_bus *bus, int phy, int reg, u16 val) if (reg & MII_ADDR_C45) { reg = mdiobb_cmd_addr(ctrl, phy, reg); mdiobb_cmd(ctrl, MDIO_C45_WRITE, phy, reg); + } else if (reg & MII_ADDR_SMI0) { + mdiobb_cmd(ctrl, SMI0_RW_OPCODE, + (reg & 0xE0) >> 5 | SMI0_WRITE_PHY, reg); } else mdiobb_cmd(ctrl, MDIO_WRITE, phy, reg); diff --git a/include/linux/phy.h b/include/linux/phy.h index 073fb151b5a99..f011722fbd5c2 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -199,6 +199,18 @@ static inline const char *phy_modes(phy_interface_t interface) IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy chips. */ #define MII_ADDR_C45 (1<<30) +/* Serial Management Interface (SMI) uses the following frame format: + * + * preamble|start|Read/Write| PHY | REG |TA| Data bits | Idle + * |frame| OP code |address |address| | | + * read | 32x1´s | 01 | 00 | 1xRRR | RRRRR |Z0| 00000000DDDDDDDD | Z + * write| 32x1´s | 01 | 00 | 0xRRR | RRRRR |10| xxxxxxxxDDDDDDDD | Z + * + * The register number is encoded with the 5 least significant bits in REG + * and the 3 most significant bits in PHY + */ +#define MII_ADDR_SMI0 (1<<31) + struct device; struct phylink; struct sk_buff;