| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <441646671cd5becd1850ed9928e9776c01f188e0.camel@microchip.com>
Date: Tue, 19 Jul 2022 15:56:05 +0000
From: <Arun.Ramadoss@...rochip.com>
To: <olteanv@...il.com>
CC: <andrew@...n.ch>, <linux-kernel@...r.kernel.org>,
<UNGLinuxDriver@...rochip.com>, <vivien.didelot@...il.com>,
<linux@...linux.org.uk>, <f.fainelli@...il.com>, <kuba@...nel.org>,
<edumazet@...gle.com>, <pabeni@...hat.com>,
<netdev@...r.kernel.org>, <Woojung.Huh@...rochip.com>,
<davem@...emloft.net>
Subject: Re: [RFC Patch net-next 03/10] net: dsa: microchip: add common
100/10Mbps selection function
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@...rochip.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
> >
Powered by blists - more mailing lists