Message-ID: <e18a490e-02cd-ae2a-37ac-e6731e149aa3@gmail.com>
Date:   Wed, 15 Mar 2023 16:23:57 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v2 net] net: dsa: microchip: fix RGMII delay configuration
 on KSZ8765/KSZ8794/KSZ8795
Content-Language: en-US
To:     Vladimir Oltean <vladimir.oltean@nxp.com>, netdev@vger.kernel.org
Cc:     Andrew Lunn <andrew@lunn.ch>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Woojung Huh <woojung.huh@microchip.com>,
        UNGLinuxDriver@microchip.com,
        Arun Ramadoss <arun.ramadoss@microchip.com>,
        Russell King <linux@armlinux.org.uk>,
        Marek Vasut <marex@denx.de>, linux-kernel@vger.kernel.org
References: <20230315231916.2998480-1-vladimir.oltean@nxp.com>
From:   Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <20230315231916.2998480-1-vladimir.oltean@nxp.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/15/23 16:19, Vladimir Oltean wrote:
> From: Marek Vasut <marex@denx.de>
> 
> The blamed commit has replaced a ksz_write8() call to address
> REG_PORT_5_CTRL_6 (0x56) with a ksz_set_xmii() -> ksz_pwrite8() call to
> regs[P_XMII_CTRL_1], which is also defined as 0x56 for ksz8795_regs[].
> 
> The trouble is that, when compared to ksz_write8(), ksz_pwrite8() also
> adjusts the register offset with the port base address. So in reality,
> ksz_pwrite8(offset=0x56) accesses register 0x56 + 0x50 = 0xa6, which in
> this switch appears to be unmapped, and the RGMII delay configuration on
> the CPU port does nothing.
> 
> So if the switch wasn't fine with the RGMII delay configuration done
> through pin strapping and relied on Linux to apply a different one in
> order to pass traffic, this is now broken.
> 
> Using the offset translation logic imposed by ksz_pwrite8(), the correct
> value for regs[P_XMII_CTRL_1] should have been 0x6 on ksz8795_regs[], in
> order to really end up accessing register 0x56.
> 
> Static code analysis shows that, despite there being multiple other
> accesses to regs[P_XMII_CTRL_1] in this driver, the only code path that
> is applicable to ksz8795_regs[] and ksz8_dev_ops is ksz_set_xmii().
> Therefore, the problem is isolated to RGMII delays.
> 
> In its current form, ksz8795_regs[] contains the same value for
> P_XMII_CTRL_0 and for P_XMII_CTRL_1, and this raises valid suspicions
> that writes made by the driver to regs[P_XMII_CTRL_0] might overwrite
> writes made to regs[P_XMII_CTRL_1] or vice versa.
> 
> Again, static analysis shows that the only accesses to P_XMII_CTRL_0
> from the driver are made from code paths which are not reachable with
> ksz8_dev_ops. So the accesses made by ksz_set_xmii() are safe for this
> switch family.
> 
> [ vladimiroltean: rewrote commit message ]
> 
> Fixes: c476bede4b0f ("net: dsa: microchip: ksz8795: use common xmii function")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian