Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 22 Jul 2022 11:09:58 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Arun Ramadoss <arun.ramadoss@microchip.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        Woojung Huh <woojung.huh@microchip.com>,
        UNGLinuxDriver@microchip.com, Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>
Subject: Re: [Patch net-next v1 3/9] net: dsa: microchip: add common duplex
 and flow control function
Message-ID: <Ytp3dl5l9jkp94lU@shell.armlinux.org.uk>
References: <20220722092459.18653-1-arun.ramadoss@microchip.com>
 <20220722092459.18653-4-arun.ramadoss@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220722092459.18653-4-arun.ramadoss@microchip.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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!