[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ee921ae6-4501-8406-2deb-91e884c802d3@sigmadesigns.com>
Date: Fri, 21 Jul 2017 16:36:41 +0200
From: Marc Gonzalez <marc_gonzalez@...madesigns.com>
To: Timur Tabi <timur@...eaurora.org>,
Florian Fainelli <f.fainelli@...il.com>,
Andrew Lunn <andrew@...n.ch>, Mans Rullgard <mans@...sr.com>,
Martin Blumenstingl <martin.blumenstingl@...il.com>,
Grygorii Strashko <grygorii.strashko@...com>,
Fabio Estevam <fabio.estevam@....com>,
Zefir Kurtisi <zefir.kurtisi@...atec.com>,
"Daniel Mack" <zonque@...il.com>
CC: netdev <netdev@...r.kernel.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
"David S. Miller" <davem@...emloft.net>,
Thibaud Cornic <thibaud_cornic@...madesigns.com>,
Mason <slash.tmp@...e.fr>
Subject: Re: [PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock
delay issues
On 21/07/2017 16:06, Timur Tabi wrote:
> On 7/21/17 8:29 AM, Marc Gonzalez wrote:
>
>> I don't understand what you're saying.
>>
>> It is a correct observation that the code enabling
>> RGMII RX clock delay is a NOP, since that bit will
>> always be set at that point.
>>
>> The spec for the 8035 (I haven't checked for 8030 and 8031,
>> is that what you meant by "other systems"?) states that
>> Sel_clk125m_dsp, which is described as:
>> "Control bit for rgmii interface rx clock delay"
>> is 1 after HW reset, 1 after SW reset.
>>
>> So my statement "RX clock delay is enabled at reset"
>> is universally true. It's not just on some systems.
>
> Ok, taken out of context, the comment doesn't really explain why the
> code is the way it is. I'm not really happy about the word "assumes".
If a HW setting defaults to 0 at reset, and some init is called
right after reset, then you know the setting's value is 0.
If you need that value to be 1, all you need is a function
setting it to 1. This is the current situation.
Commit 2e5f9f281ee8 assumes 0 at reset, and provides a function
setting the value to 1.
Reality is different. The HW setting defaults to 1 at reset.
So it turns out that the function setting the value to 1
is pointless, because the value is already 1. There is
however no way to set the value to 0.
Does that explain why I wrote "assume"?
Also the commit message:
"The current code supports enabling RGMII RX and TX clock delays.
The unstated assumption is that these settings are disabled by
default at reset, which is not the case."
> Maybe you should add a sentence explaining when the code is NOT a no-op.
The code is *NEVER* NOT a no-op.
I.e. the code enabling RX clock delay is ALWAYS a no-op.
I don't understand what you think is unclear in my comment.
Regards.
Powered by blists - more mailing lists