Message-ID: <2a7696ff-eceb-2feb-0cce-d9910b5ad81f@denx.de>
Date:   Thu, 23 Feb 2023 06:17:28 +0100
From:   Marek Vasut <marex@...x.de>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        Arun Ramadoss <arun.ramadoss@...rochip.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        UNGLinuxDriver@...rochip.com,
        Woojung Huh <woojung.huh@...rochip.com>, stable@...r.kernel.org
Subject: Re: [PATCH] net: dsa: microchip: Fix gigabit set and get function for
 KSZ87xx

On 2/23/23 01:22, Vladimir Oltean wrote:
> Hi Marek,
> 
> On Thu, Feb 23, 2023 at 12:55:08AM +0100, Marek Vasut wrote:
>> The old code, removed in:
>> c476bede4b0f0 ("net: dsa: microchip: ksz8795: use common xmii function")
>> used ksz_write8() (this part is important):
>> ksz_write8(dev, REG_PORT_5_CTRL_6, data8);
>> where:
>> drivers/net/dsa/microchip/ksz8795_reg.h:#define REG_PORT_5_CTRL_6
>> 0x56
>>
>> The new code, where the relevant part is added in (see Fixes tag)
>> 46f80fa8981bc ("net: dsa: microchip: add common gigabit set and get
>> function")
>> +++ b/drivers/net/dsa/microchip/ksz_common.c
>> @@ -257,6 +257,7 @@ static const u16 ksz8795_regs[] = {
>> +       [P_XMII_CTRL_1]                 = 0x56,
>> uses ksz_pwrite8() (with p in the function name, p means PORT):
>> ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8);
>> which per drivers/net/dsa/microchip/ksz_common.h translates to
>> ksz_write8(dev, dev->dev_ops->get_port_addr(port, offset), data);
>> and that dev->dev_ops->get_port_addr(port, offset) remapping function is per
>> drivers/net/dsa/microchip/ksz8795.c really call to the following macro:
>> PORT_CTRL_ADDR(port, offset)
>> which in turn from drivers/net/dsa/microchip/ksz8795_reg.h becomes
>> #define PORT_CTRL_ADDR(port, addr) ((addr) + REG_PORT_1_CTRL_0 + (port) * (REG_PORT_2_CTRL_0 - REG_PORT_1_CTRL_0))
>>
>> That means:
>> ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8)
>> writes register 0xa6 instead of register 0x56, because it calls the
>> PORT_CTRL_ADDR(port, 0x56)=0xa6, but in reality it should call
>> PORT_CTRL_ADDR(port, 0x06)=0x56, i.e. the remapping should happen ONCE, the
>> value 0x56 is already remapped .
> 
> I never had any objection to this part.
> 
>> All the call-sites which do
>> ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8)
>> or
>> ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8)
>> are affected by this, all six, that means the ksz_[gs]et_xmii() and the
>> ksz_[gs]et_gbit().
> 
> I'm going to say this with a very calm tone, please tell me where it's wrong
> and we can go from there.
> 
>   Not for the ksz_switch_chips[] elements which point to ksz8795_regs (which
>   had the incorrect value you're fixing), it isn't. You're making an argument
>   for code which never executes (5 out of 6 call paths), and basing your commit
>   message off of it. Your commit message reads as if you didn't even notice
>   that ksz_set_gbit() isn't called for ksz87xx, and that this is a bug in itself.
>   Moreover, the problem you're seeing (I may speculate what that is, but
>   I don't know) is surely not due to ksz_set_gbit() being called on the
>   wrong register, because it's not called at all *for that hardware*.
> 
> That gigabit bug was pointed out to you by reviewers, and you refuse to
> acknowledge this and keep bringing forth some other stuff which was never
> debated by anyone. The lack of acknowledgement plus your continuation to
> randomly keep singing another tune in another key completely is irritating
> to me on a very personal (non-technical) level. To respond to you, I am
> exercising some mental muscles which I wish I wouldn't have needed to,
> here, in this context. The same muscles I use when I need to identify
> manipulation on tass.com.
> 
> [ in case the message above is misinterpreted: I did not say that you
>    willingly manipulate. I said that your lack of acknowledgement to what
>    is being said to you is giving me the same kind of frustration ]
> 
> This is my feedback to the tone in your replies. I want you to give your
> feedback to my tone now too. You disregarded that.
> 
>> ...
>>
>> If all that should be changed in the commit message is "to access the
>> P_GMII_1GBIT_M, i.e. Is_1Gbps, bit" to something from the "ksz_set_xmii()"
>> function instead, then just say so.
>>
>> [...]
> 
> No, this is not all that I want.
> 
> The gigabit bug changes things in ways in which I'm curious how you're
> going to try to defend, with this attitude of responding to anything
> except to what was asked. Your commit says it fixes gigabit on KSZ87xx

No, it does not say it fixes gigabit on KSZ87xx, the commit message says 
it fixes gigabit accessor functions, but what it really fixes (and what 
is the bulk of the commit message) is the incorrectly double-remapped 
register 0x56 used in those gigabit accessor functions and which is also 
used in the ksz_[gs]et_xmii function.

> but the gigabit bug which *was pointed out to you by others* is still
> there. Your patch fixes something else, but *it says* it fixes a gigabit
> bug. What I want is for you to describe exactly what it fixes, or if you
> just don't know, say you noticed the bug during code review and you
> don't know what is its real life impact (considering pin strapping).

I believe I wrote what the problem is in my previous email, the 
incorrectly double-remapped XMII_1 register . The register wasn't 
updated in my case in ksz_set_xmii() with RGMII delays.

Why I picked the is_1Gbit bit for the commit message, probably was tired 
after lengthy debugging session of this problem.

> I don't want a patch to be merged which says it fixes something it doesn't
> fix, while leaving the exact thing it says it fixes unfixed.
> 
> I also don't want to entertain this game of "if it's just this small
> thing, why didn't you say so". I would be setting myself up for an
> endless time waste if I were to micromanage your commit message writing.
> 
> I am looking forward to a productive conversation with you, but if your
> next reply is going to have the same strategy of avoiding my key message
> and focusing on some other random thing, then I'm very sorry, but I'll
> just have to focus my attention somewhere else.

[...]

Powered by blists - more mailing lists