by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [Patch net-next v1 3/9] net: dsa: microchip: add common duplex and flow control function

Hi,

On Fri, Jul 22, 2022 at 02:54:53PM +0530, Arun Ramadoss wrote:
> +void ksz_set_fullduplex(struct ksz_device *dev, int port, bool val)
> +{
> + const u8 *bitval = dev->info->xmii_ctrl0;
> + const u16 *regs = dev->info->regs;
> + u8 data8;
> +
> + ksz_pread8(dev, port, regs[P_XMII_CTRL_0], &data8);
> +
> + data8 &= ~P_MII_DUPLEX_M;
> +
> + if (val)
> + data8 |= FIELD_PREP(P_MII_DUPLEX_M,
> + bitval[P_MII_FULL_DUPLEX]);
> + else
> + data8 |= FIELD_PREP(P_MII_DUPLEX_M,
> + bitval[P_MII_HALF_DUPLEX]);
> +
> + ksz_pwrite8(dev, port, regs[P_XMII_CTRL_0], data8);
> +}
> +
> +void ksz_set_tx_pause(struct ksz_device *dev, int port, bool val)
> +{
> + const u32 *masks = dev->info->masks;
> + const u16 *regs = dev->info->regs;
> + u8 data8;
> +
> + ksz_pread8(dev, port, regs[P_XMII_CTRL_0], &data8);
> +
> + if (val)
> + data8 |= masks[P_MII_TX_FLOW_CTRL];
> + else
> + data8 &= ~masks[P_MII_TX_FLOW_CTRL];
> +
> + ksz_pwrite8(dev, port, regs[P_XMII_CTRL_0], data8);
> +}
> +
> +void ksz_set_rx_pause(struct ksz_device *dev, int port, bool val)
> +{
> + const u32 *masks = dev->info->masks;
> + const u16 *regs = dev->info->regs;
> + u8 data8;
> +
> + ksz_pread8(dev, port, regs[P_XMII_CTRL_0], &data8);
> +
> + if (val)
> + data8 |= masks[P_MII_RX_FLOW_CTRL];
> + else
> + data8 &= ~masks[P_MII_RX_FLOW_CTRL];
> +
> + ksz_pwrite8(dev, port, regs[P_XMII_CTRL_0], data8);
> +}
> +

Having looked through all the proposed patches and noticed that these
three functions are always called serially. What is the reason to make
these separate functions which all read the same register, modify a
single bit, and then write it back.

What we end up with is the following sequence:

- read P_XMII_CTRL_0
- udpate P_MII_HALF_DUPLEX bit
- write P_XMII_CTRL_0
- read P_XMII_CTRL_0
- update P_MII_TX_FLOW_CTRL bit
- write P_XMII_CTRL_0
- read P_XMII_CTRL_0
- update P_MII_RX_FLOW_CTRL bit
- write P_XMII_CTRL_0

whereas the original code did:

- read P_XMII_CTRL_0
- udpate P_MII_HALF_DUPLEX, P_MII_TX_FLOW_CTRL and P_MII_RX_FLOW_CTRL
bits
- write P_XMII_CTRL_0

which was much more efficient, not only in terms of CPU cycles, but also
IO cycles and code size.

You could do this instead:

u8 mask, val, ctrl0;

mask = P_MII_DUPLEX_M | masks[P_MII_TX_FLOW_CTRL] |
masks[P_MII_RX_FLOW_CTRL];

if (duplex == DUPLEX_FULL)
val = FIELD_PREP(P_MII_DUPLEX_M, bitval[P_MII_FULL_DUPLEX]);
else
val = FIELD_PREP(P_MII_DUPLEX_M, bitval[P_MII_HALF_DUPLEX]);

if (tx_pause)
val |= masks[P_MII_TX_FLOW_CTRL];

if (rx_pause)
val |= masks[P_MII_RX_FLOW_CTRL];

ksz_pread8(dev, port, REG_PORT_XMII_CTRL_0, &ctrl0);
ctrl0 = (ctrl0 & ~mask) | val;
ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_0, ctrl0);

and maybe convert that last three lines into a helper, ksz_pmodify8()
which could be useful in other parts of the driver where you do a
read-modify-write operation on a register.

Thanks.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!