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:   Tue, 30 Aug 2022 13:33:40 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Jerry Ray <jerry.ray@microchip.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, UNGLinuxDriver@microchip.com
Subject: Re: [PATCH 1/2] net: dsa: LAN9303: Add basic support for LAN9354
Message-ID: <20220830103340.bqgzmcztb57m7jgd@skbuf>
References: <20220829180037.31078-1-jerry.ray@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220829180037.31078-1-jerry.ray@microchip.com>
Precedence: bulk

On Mon, Aug 29, 2022 at 01:00:36PM -0500, Jerry Ray wrote:
> Add initial BYTE_ORDER read to sync to improve driver robustness

Please don't post 2 different patches with the same commit message.
I think here, the first paragraph is what the commit message should
actually be.

> 
> The lan9303 expects two mdio read transactions back-to-back to read a 32-bit
> register. The first read transaction causes the other half of the 32-bit
> register to get latched.  The subsequent read returns the latched second half
> of the 32-bit read. The BYTE_ORDER register is an exception to this rule. As
> it is a constant value, there is no need to latch the second half. We read
> this register first in case there were reads during the boot loader process
> that might have occurred prior to this driver taking over ownership of
> accessing this device.
> 
> This patch has been tested on the SAMA5D3-EDS with a LAN9303 RMII daughter
> card.

Is this patch fixing a problem for any existing platforms supported by
this driver?

> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
>  drivers/net/dsa/lan9303-core.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index e03ff1f267bb..17ae02a56bfe 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -32,6 +32,7 @@
>  #define LAN9303_INT_EN 0x17
>  # define LAN9303_INT_EN_PHY_INT2_EN BIT(27)
>  # define LAN9303_INT_EN_PHY_INT1_EN BIT(26)
> +#define LAN9303_BYTE_ORDER 0x19
>  #define LAN9303_HW_CFG 0x1D
>  # define LAN9303_HW_CFG_READY BIT(27)
>  # define LAN9303_HW_CFG_AMDX_EN_PORT2 BIT(26)
> @@ -847,9 +848,10 @@ static int lan9303_check_device(struct lan9303 *chip)
>  	int ret;
>  	u32 reg;
>  
> -	ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
> +	// Dummy read to ensure MDIO access is in 32-bit sync.

C-style comments /* */ are more typical in the Linux kernel coding style.

> +	ret = lan9303_read(chip->regmap, LAN9303_BYTE_ORDER, &reg);

Pretty strange to see the dummy read in lan9303_check_device().
Bootloader leaving things in a messy state is only a problem if we don't
have a reset GPIO, right?

How about introducing the logic here, right in lan9303_probe():

	lan9303_handle_reset(chip);

	if (!chip->reset_gpio) {
		/* Dummy read to ensure MDIO access is in 32-bit sync. */
		ret = lan9303_read(chip->regmap, LAN9303_BYTE_ORDER, &reg);
		if (ret) {
			dev_err(chip->dev, "failed to access the device: %pe\n",
				ERR_PTR(ret));
			return ret;
		}
	}

	ret = lan9303_check_device(chip);

>  	if (ret) {
> -		dev_err(chip->dev, "failed to read chip revision register: %d\n",
> +		dev_err(chip->dev, "failed to access the device: %d\n",
>  			ret);
>  		if (!chip->reset_gpio) {
>  			dev_dbg(chip->dev,

The context here reads:
		if (!chip->reset_gpio) {
			dev_dbg(chip->dev,
				"hint: maybe failed due to missing reset GPIO\n");
		}

Is the comment still accurate after the change, or do you feel that it
can be removed? Looks like you are fixing a known issue.

> @@ -858,6 +860,13 @@ static int lan9303_check_device(struct lan9303 *chip)
>  		return ret;
>  	}
>  
> +	ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to read chip revision register: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
>  	if ((reg >> 16) != LAN9303_CHIP_ID) {
>  		dev_err(chip->dev, "expecting LAN9303 chip, but found: %X\n",
>  			reg >> 16);
> -- 
> 2.17.1
>