mbox series

[net-next,0/4] DP83869 Enhancements

Message ID 20200519141813.28167-1-dmurphy@ti.com
Headers show
Series DP83869 Enhancements | expand

Message

Dan Murphy May 19, 2020, 2:18 p.m. UTC 
Hello

These are improvements to the DP83869 Ethernet PHY driver.  OP-mode and port
mirroring may be strapped on the device but the software only retrives these
settings from the device tree.  Reading the straps and initializing the
associated stored variables so when setting the PHY up and down the PHY's
configuration values will be retained.

The PHY also supports RGMII internal delays.  Implement this feature as it
was done in the DP83867 device.

Dan Murphy (4):
  net: phy: dp83869: Update port-mirroring to read straps
  net: phy: dp83869: Set opmode from straps
  dt-bindings: net: Add RGMII internal delay for DP83869
  net: dp83869: Add RGMII internal delay configuration

 .../devicetree/bindings/net/ti,dp83869.yaml   |  16 +++
 drivers/net/phy/dp83869.c                     | 120 +++++++++++++++++-
 include/dt-bindings/net/ti-dp83869.h          |  18 +++
 3 files changed, 150 insertions(+), 4 deletions(-)

Comments

Jakub Kicinski May 19, 2020, 4:58 p.m. UTC | #1 
On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
> If the op-mode for the device is not set in the device tree then set
> the strapped op-mode and store it for later configuration.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  171 |  if (val < 0)
      |          ^
Dan Murphy May 19, 2020, 5:40 p.m. UTC | #2 
Jakub

On 5/19/20 11:58 AM, Jakub Kicinski wrote:
> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
>> If the op-mode for the device is not set in the device tree then set
>> the strapped op-mode and store it for later configuration.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>    171 |  if (val < 0)
>        |          ^

This looks to be a false positive.

phy_read_mmd will return an errno or a value from 0->15

So if errno is returned then this will be true.

Unless I have to do IS_ERR.

I did not get that warning.  But I am using 9.2-gcc.

What compiler are you using?

Dan
Dan Murphy May 19, 2020, 5:56 p.m. UTC | #3 
Jakub

On 5/19/20 12:40 PM, Dan Murphy wrote:
> Jakub
>
> On 5/19/20 11:58 AM, Jakub Kicinski wrote:
>> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
>>> If the op-mode for the device is not set in the device tree then set
>>> the strapped op-mode and store it for later configuration.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
>> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always 
>> false due to limited range of data type [-Wtype-limits]
>>    171 |  if (val < 0)
>>        |          ^
>
> This looks to be a false positive.
>
> phy_read_mmd will return an errno or a value from 0->15
>
> So if errno is returned then this will be true.
>
> Unless I have to do IS_ERR.
>
> I did not get that warning.  But I am using 9.2-gcc.
>
> What compiler are you using?
>
I see what the issue is val needs to be an int not a u16

I will fix it

Dan


> Dan
>
Andrew Lunn May 19, 2020, 6:29 p.m. UTC | #4 
On Tue, May 19, 2020 at 09:58:18AM -0700, Jakub Kicinski wrote:
> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
> > If the op-mode for the device is not set in the device tree then set
> > the strapped op-mode and store it for later configuration.
> > 
> > Signed-off-by: Dan Murphy <dmurphy@ti.com>
> 
> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>   171 |  if (val < 0)
>       |          ^

Hi Jakub

This happens a lot with PHY drivers. The register being read is a u16,
so that is what people use.

Is this now a standard GCC warning? Or have you turned on extra
checking?

	Andrew
Dan Murphy May 19, 2020, 6:41 p.m. UTC | #5 
Andrew

On 5/19/20 1:29 PM, Andrew Lunn wrote:
> On Tue, May 19, 2020 at 09:58:18AM -0700, Jakub Kicinski wrote:
>> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
>>> If the op-mode for the device is not set in the device tree then set
>>> the strapped op-mode and store it for later configuration.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
>> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>>    171 |  if (val < 0)
>>        |          ^
> Hi Jakub
>
> This happens a lot with PHY drivers. The register being read is a u16,
> so that is what people use.

Yes this is what happened but phy_read_mmd returns an int so the 
declaration of val should be an int.

I will update that in v2


> Is this now a standard GCC warning? Or have you turned on extra
> checking?
I still was not able to reproduce this warning with gcc-9.2.  I would 
like to know the same


Dan
Jakub Kicinski May 19, 2020, 6:48 p.m. UTC | #6 
On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote:
> > Is this now a standard GCC warning? Or have you turned on extra
> > checking?  
> I still was not able to reproduce this warning with gcc-9.2.  I would 
> like to know the same

W=1 + gcc-10 here, also curious to know which one of the two makes 
the difference :)
Dan Murphy May 19, 2020, 6:59 p.m. UTC | #7 
Jakub

On 5/19/20 1:48 PM, Jakub Kicinski wrote:
> On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote:
>>> Is this now a standard GCC warning? Or have you turned on extra
>>> checking?
>> I still was not able to reproduce this warning with gcc-9.2.  I would
>> like to know the same
> W=1 + gcc-10 here, also curious to know which one of the two makes
> the difference :)

W=1 made the difference I got the warning with gcc-9.2

Dan
Andrew Lunn May 19, 2020, 7:03 p.m. UTC | #8 
On Tue, May 19, 2020 at 01:59:16PM -0500, Dan Murphy wrote:
> Jakub
> 
> On 5/19/20 1:48 PM, Jakub Kicinski wrote:
> > On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote:
> > > > Is this now a standard GCC warning? Or have you turned on extra
> > > > checking?
> > > I still was not able to reproduce this warning with gcc-9.2.  I would
> > > like to know the same
> > W=1 + gcc-10 here, also curious to know which one of the two makes
> > the difference :)
> 
> W=1 made the difference I got the warning with gcc-9.2

I wonder if we should turn on this specific warning by default in
drivers/net/phy? I keep making the same mistake, and it would be nice
if GCC actually told me.

   Andrew
Florian Fainelli May 19, 2020, 8:03 p.m. UTC | #9 
On 5/19/2020 7:18 AM, Dan Murphy wrote:
> The device tree may not have the property set for port mirroring
> because the hardware may have it strapped. If the property is not in the
> DT then check the straps and set the port mirroring bit appropriately.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

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