Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1648234 > unrolled thread

Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs Andrew Lunn <andrew@lunn.ch> - 2017-05-23 18:10 +0200
    RE: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs Bogdan Purcareata <bogdan.purcareata@nxp.com> - 2017-05-23 18:40 +0200
      Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs Andrew Lunn <andrew@lunn.ch> - 2017-05-23 19:00 +0200
        Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs Florian Fainelli <f.fainelli@gmail.com> - 2017-05-23 20:20 +0200
          RE: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs Bogdan Purcareata <bogdan.purcareata@nxp.com> - 2017-05-24 08:10 +0200

#1648234 — Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs

On Tue, May 23, 2017 at 03:53:18PM +0000, Bogdan Purcareata wrote:
> Complete the work started by the following patches
> 5f6c99e0 net: phy: fix a bug in get_phy_c45_ids
> da1da284 net/phy: tune get_phy_c45_ids to support more c45 phy
> 
> in order to properly get non-standard C45 10G PHY IDs (e.g. Cortina
> CS4340).

At a first look, this seems to be a hack to work around a broken phy.

Instead of this, can you use the ethernet-phy-id property in device
tree?

	Andrew

[toc] | [next] | [standalone]

#1648252

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Tuesday, May 23, 2017 7:04 PM
> To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> Cc: f.fainelli@gmail.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs
> 
> On Tue, May 23, 2017 at 03:53:18PM +0000, Bogdan Purcareata wrote:
> > Complete the work started by the following patches
> > 5f6c99e0 net: phy: fix a bug in get_phy_c45_ids
> > da1da284 net/phy: tune get_phy_c45_ids to support more c45 phy
> >
> > in order to properly get non-standard C45 10G PHY IDs (e.g. Cortina
> > CS4340).
> 
> At a first look, this seems to be a hack to work around a broken phy.
> 
> Instead of this, can you use the ethernet-phy-id property in device
> tree?

The patches mentioned in the commit message add _some_ support for the Cortina PHYs - mainly checking for devices at additional locations. Once they are found, the phy IDs must be read from custom locations. I followed the u-boot model [1].

I was aiming to patch the get_phy_c45_ids to fully handle the Cortina PHY scenario (right now, the support is only partial). 

[1] http://code.metager.de/source/xref/denx/u-boot/drivers/net/phy/cortina.c#288

Thank you!
Bogdan

[toc] | [prev] | [next] | [standalone]

#1648269

> The patches mentioned in the commit message add _some_ support for
> the Cortina PHYs - mainly checking for devices at additional
> locations. Once they are found, the phy IDs must be read from custom
> locations.
 
As a general principle, we don't add hacks in generic code to handle
broken devices. We add generic mechanisms to work around the
brokenness.

In this case, by using ethernet-phy-id in the device tree, we are
saying, this PHYs probing is totally borked, but we know it is there,
at this address. Just load the driver.

Please try to make ethernet-phy-id work.

       Andrew

[toc] | [prev] | [next] | [standalone]

#1648321

On 05/23/2017 09:55 AM, Andrew Lunn wrote:
>> The patches mentioned in the commit message add _some_ support for
>> the Cortina PHYs - mainly checking for devices at additional
>> locations. Once they are found, the phy IDs must be read from custom
>> locations.
>  
> As a general principle, we don't add hacks in generic code to handle
> broken devices. We add generic mechanisms to work around the
> brokenness.
> 
> In this case, by using ethernet-phy-id in the device tree, we are
> saying, this PHYs probing is totally borked, but we know it is there,
> at this address. Just load the driver.
> 
> Please try to make ethernet-phy-id work.

What Andrew is suggesting is to leverage the code in
drivers/of/of_mdio.c which does the following:

       is_c45 = of_device_is_compatible(child,
                                         "ethernet-phy-ieee802.3-c45");

        if (!is_c45 && !of_get_phy_id(child, &phy_id))
                phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
        else
                phy = get_phy_device(mdio, addr, is_c45);
        if (IS_ERR(phy))
                return;

If you know the PHY ID, and you did put it in the PHY node's compatible
string (in the format that of_get_phy_id() expects it to, and you also
did not add "ethernet-phy-ieee802.3-c45") then the PHY library will
directly create the PHY device, with the designated ID, at the specific
address.

While this works for clause 22 PHYs, I don't know if it also does for
clause 45 PHYs, but as Andrew is suggesting, I would be more inclined
into making this scheme work for all types (22 or 45) PHYs, rather than
hacking the core code that tries to identify devices in packages.

Can you give it a spin?
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1649137

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Tuesday, May 23, 2017 9:12 PM
> To: Andrew Lunn <andrew@lunn.ch>; Bogdan Purcareata
> <bogdan.purcareata@nxp.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs
> 
> On 05/23/2017 09:55 AM, Andrew Lunn wrote:
> >> The patches mentioned in the commit message add _some_ support for
> >> the Cortina PHYs - mainly checking for devices at additional
> >> locations. Once they are found, the phy IDs must be read from custom
> >> locations.
> >
> > As a general principle, we don't add hacks in generic code to handle
> > broken devices. We add generic mechanisms to work around the
> > brokenness.
> >
> > In this case, by using ethernet-phy-id in the device tree, we are
> > saying, this PHYs probing is totally borked, but we know it is there,
> > at this address. Just load the driver.
> >
> > Please try to make ethernet-phy-id work.
> 
> What Andrew is suggesting is to leverage the code in
> drivers/of/of_mdio.c which does the following:
> 
>        is_c45 = of_device_is_compatible(child,
>                                          "ethernet-phy-ieee802.3-c45");
> 
>         if (!is_c45 && !of_get_phy_id(child, &phy_id))
>                 phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
>         else
>                 phy = get_phy_device(mdio, addr, is_c45);
>         if (IS_ERR(phy))
>                 return;
> 
> If you know the PHY ID, and you did put it in the PHY node's compatible
> string (in the format that of_get_phy_id() expects it to, and you also
> did not add "ethernet-phy-ieee802.3-c45") then the PHY library will
> directly create the PHY device, with the designated ID, at the specific
> address.
> 
> While this works for clause 22 PHYs, I don't know if it also does for
> clause 45 PHYs, but as Andrew is suggesting, I would be more inclined
> into making this scheme work for all types (22 or 45) PHYs, rather than
> hacking the core code that tries to identify devices in packages.
> 
> Can you give it a spin?

Sure. My first thought was to latch onto the codepath that had some mention of Cortina PHYs.

I will check of_get_phy_id() and see if it does the job.

Thank you!
Bogdan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web