Messages in this thread | | | From | <> | Subject | Re: [RFC Patch net-next 03/10] net: dsa: microchip: add common 100/10Mbps selection function | Date | Tue, 19 Jul 2022 15:56:05 +0000 |
| |
Hi Vladimir, On Tue, 2022-07-19 at 13:48 +0300, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Tue, Jul 12, 2022 at 09:33:01PM +0530, Arun Ramadoss wrote: > > This patch adds the function for configuring the 100/10Mbps speed > > selection for the ksz switches. KSZ8795 switch uses Global control > > 4 > > register 0x06 bit 4 for choosing 100/10Mpbs. Other switches uses > > xMII > > control 1 0xN300 for it. > > For KSZ8795, if the bit is set then 10Mbps is chosen and if bit is > > clear then 100Mbps chosen. For all other switches it is other way > > around, if the bit is set then 100Mbps is chosen. > > So, this patch add the generic function for ksz switch to select > > the > > 100/10Mbps speed selection. While configuring, first it disables > > the > > gigabit functionality and then configure the respective speed. > > > > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> > > --- > > drivers/net/dsa/microchip/ksz9477_reg.h | 1 - > > drivers/net/dsa/microchip/ksz_common.c | 29 > > ++++++++++++++++++++++++ > > drivers/net/dsa/microchip/ksz_common.h | 6 +++++ > > drivers/net/dsa/microchip/lan937x_main.c | 14 ++++-------- > > drivers/net/dsa/microchip/lan937x_reg.h | 1 - > > 5 files changed, 40 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h > > b/drivers/net/dsa/microchip/ksz9477_reg.h > > index f23ed4809e47..2649fdf0bae1 100644 > > --- a/drivers/net/dsa/microchip/ksz9477_reg.h > > +++ b/drivers/net/dsa/microchip/ksz9477_reg.h > > @@ -1179,7 +1179,6 @@ > > > > #define PORT_SGMII_SEL BIT(7) > > #define PORT_MII_FULL_DUPLEX BIT(6) > > -#define PORT_MII_100MBIT BIT(4) > > #define PORT_GRXC_ENABLE BIT(0) > > > > #define REG_PORT_XMII_CTRL_1 0x0301 > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > > b/drivers/net/dsa/microchip/ksz_common.c > > index 5ebcd87fc531..f41cd2801210 100644 > > --- a/drivers/net/dsa/microchip/ksz_common.c > > +++ b/drivers/net/dsa/microchip/ksz_common.c > > @@ -256,6 +256,7 @@ static const u16 ksz8795_regs[] = { > > [S_START_CTRL] = 0x01, > > [S_BROADCAST_CTRL] = 0x06, > > [S_MULTICAST_CTRL] = 0x04, > > + [P_XMII_CTRL_0] = 0x06, > > [P_XMII_CTRL_1] = 0x56, > > }; > > > > @@ -284,6 +285,8 @@ static const u32 ksz8795_masks[] = { > > static const u8 ksz8795_values[] = { > > [P_MII_1GBIT] = 1, > > [P_MII_NOT_1GBIT] = 0, > > + [P_MII_100MBIT] = 0, > > + [P_MII_10MBIT] = 1, > > Can you namespace P_GMII_1GBIT/P_GMII_NOT_1GBIT separately from > P_MII_100MBIT/P_MII_10MBIT? Otherwise it's not obvious that the first > set writes to regs[P_XMII_CTRL_1] and the other to > regs[P_XMII_CTRL_0].
Yes, I can do it.
> > > }; > > > > static const u8 ksz8795_shifts[] = { > > @@ -356,6 +359,7 @@ static const u16 ksz9477_regs[] = { > > [S_START_CTRL] = 0x0300, > > [S_BROADCAST_CTRL] = 0x0332, > > [S_MULTICAST_CTRL] = 0x0331, > > + [P_XMII_CTRL_0] = 0x0300, > > [P_XMII_CTRL_1] = 0x0301, > > }; > > > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.h > > b/drivers/net/dsa/microchip/ksz_common.h > > index a76dfef6309c..f1fa6feca559 100644 > > --- a/drivers/net/dsa/microchip/ksz_common.h > > +++ b/drivers/net/dsa/microchip/ksz_common.h > > @@ -172,6 +172,7 @@ enum ksz_regs { > > S_START_CTRL, > > S_BROADCAST_CTRL, > > S_MULTICAST_CTRL, > > + P_XMII_CTRL_0, > > P_XMII_CTRL_1, > > }; > > > > > > /* Regmap tables generation */ > > diff --git a/drivers/net/dsa/microchip/lan937x_main.c > > b/drivers/net/dsa/microchip/lan937x_main.c > > index efca96b02e15..37f63110e5bb 100644 > > --- a/drivers/net/dsa/microchip/lan937x_main.c > > +++ b/drivers/net/dsa/microchip/lan937x_main.c > > @@ -346,21 +346,18 @@ static void lan937x_config_interface(struct > > ksz_device *dev, int port, > > int speed, int duplex, > > bool tx_pause, bool rx_pause) > > { > > - u8 xmii_ctrl0, xmii_ctrl1; > > + u8 xmii_ctrl0; > > > > ksz_pread8(dev, port, REG_PORT_XMII_CTRL_0, &xmii_ctrl0); > > - ksz_pread8(dev, port, REG_PORT_XMII_CTRL_1, &xmii_ctrl1); > > > > - xmii_ctrl0 &= ~(PORT_MII_100MBIT | PORT_MII_FULL_DUPLEX | > > - PORT_MII_TX_FLOW_CTRL | > > PORT_MII_RX_FLOW_CTRL); > > + xmii_ctrl0 &= ~(PORT_MII_FULL_DUPLEX | PORT_MII_TX_FLOW_CTRL > > | > > + PORT_MII_RX_FLOW_CTRL); > > > > if (speed == SPEED_1000) > > ksz_set_gbit(dev, port, true); > > - else > > - ksz_set_gbit(dev, port, false); > > > > - if (speed == SPEED_100) > > - xmii_ctrl0 |= PORT_MII_100MBIT; > > + if (speed == SPEED_100 || speed == SPEED_10) > > + ksz_set_100_10mbit(dev, port, speed); > > Why don't you create a single ksz_port_set_xmii_speed() entry point, > and > this decides whether to call ksz_set_gbit() with true or false > depending > on whether the speed was 1000, and ksz_set_100_10mbit() as > appropriate?
Ok, I will create new function.
> > > > if (duplex) > > xmii_ctrl0 |= PORT_MII_FULL_DUPLEX; > > @@ -372,7 +369,6 @@ static void lan937x_config_interface(struct > > ksz_device *dev, int port, > > xmii_ctrl0 |= PORT_MII_RX_FLOW_CTRL; > > > > ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_0, xmii_ctrl0); > > - ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_1, xmii_ctrl1); > > Will these remaining ksz_pwrite8 calls to REG_PORT_XMII_CTRL_0 not > overwrite what ksz_set_gbit() is doing?
Yes, this ksz_pwrite8 overwrites the ksz_set_gbit. But the next patch updates the duplex and flow control separately, I tested the phylink register configuration only after applying entire series. So I missed this. Thanks for pointing it. I will move the reading the xmii_ctrl0 after configuring the ksz_set_gbit().
> > > } > > > > void lan937x_phylink_get_caps(struct ksz_device *dev, int port, > > diff --git a/drivers/net/dsa/microchip/lan937x_reg.h > > b/drivers/net/dsa/microchip/lan937x_reg.h > > index 747295d34411..b9364f6a4f8f 100644 > > --- a/drivers/net/dsa/microchip/lan937x_reg.h > > +++ b/drivers/net/dsa/microchip/lan937x_reg.h > > @@ -135,7 +135,6 @@ > > #define PORT_SGMII_SEL BIT(7) > > #define PORT_MII_FULL_DUPLEX BIT(6) > > #define PORT_MII_TX_FLOW_CTRL BIT(5) > > -#define PORT_MII_100MBIT BIT(4) > > #define PORT_MII_RX_FLOW_CTRL BIT(3) > > #define PORT_GRXC_ENABLE BIT(0) > > > > -- > > 2.36.1 > >
| |