Re: [PATCH v6 1/6] net: phy: at803x: Export at803x_debug_reg_mask()

Re: [PATCH v6 1/6] net: phy: at803x: Export at803x_debug_reg_mask()

[Andrew Lunn]

On Thu, May 10, 2018 at 04:16:52PM -0700, Paul Burton wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> On some boards, this PHY has a problem when it hibernates. Export this
> function to a board can register a PHY fixup to disable hibernation.

What do you know about the problem?

https://patchwork.ozlabs.org/patch/686371/

I don't remember how it was solved, but you should probably do the
same.

	Andrew

Re: [PATCH v6 6/6] MIPS: Boston: Adjust DT for pch_gbe PHY support

[Andrew Lunn]

> +					ethernet-phy@0 {
> +						compatible = "ethernet-phy-id001c.c915";

You only need to specify the compatible string like this if the PHY
has its own ID wrong. The AT802x gets this right, so you don't need
this.

	Andrew

Re: [PATCH v6 6/6] MIPS: Boston: Adjust DT for pch_gbe PHY support

[Andrew Lunn]

>  				eg20t_mac@2,0,1 {
>  					compatible = "pci8086,8802";
>  					reg = <0x00020100 0 0 0 0>;
> -					phy-reset-gpios = <&eg20t_gpio 6
> -							   GPIO_ACTIVE_LOW>;
> +
> +					#address-cells = <1>;
> +					#size-cells = <0>;

It is generally a good idea to put an 'mdio' container which the PHYs
are on. You then pass this container node to of_mdiobus_register().

> +
> +					ethernet-phy@0 {
> +						compatible = "ethernet-phy-id001c.c915";
> +						reg = <0>;
> +						reset-gpios = <&eg20t_gpio 6 GPIO_ACTIVE_LOW>;
> +						reset-assert-us = <25000>;
> +						reset-deassert-us = <25000>;
> +					};

  Andrew

Re: [PATCH v6 1/6] net: phy: at803x: Export at803x_debug_reg_mask()

[Andrew Lunn]

> I could reorder the probe function a little to initialize the PHY before
> performing the MAC reset, drop this patch and the AR803X hibernation
> stuff from patch 2 if you like. But again, I can't actually test the
> result on the affected hardware.

Hi Paul

I don't like a MAC driver poking around in PHY registers.

So if you can rearrange the code, that would be great.

   Thanks
	Andrew

Re: [PATCH v6 1/6] net: phy: at803x: Export at803x_debug_reg_mask()

[Andrew Lunn]

On Fri, May 11, 2018 at 03:22:39PM -0700, Paul Burton wrote:
> Hi Andrew,
> 
> On Fri, May 11, 2018 at 09:24:46PM +0200, Andrew Lunn wrote:
> > > I could reorder the probe function a little to initialize the PHY before
> > > performing the MAC reset, drop this patch and the AR803X hibernation
> > > stuff from patch 2 if you like. But again, I can't actually test the
> > > result on the affected hardware.
> > 
> > Hi Paul
> > 
> > I don't like a MAC driver poking around in PHY registers.
> > 
> > So if you can rearrange the code, that would be great.
> > 
> >    Thanks
> > 	Andrew
> 
> Sure, I'll give it a shot.
> 
> After digging into it I see 2 ways to go here:
> 
>   1) We could just always reset the PHY before we reset the MAC. That
>      would give us a window of however long the PHY takes to enter its
>      low power state & stop providing the RX clock during which we'd
>      need the MAC reset to complete. In the case of the AR8031 that's
>      "about 10 seconds" according to its data sheet. In this particular
>      case that feels like plenty, but it does also feel a bit icky to
>      rely on the timing chosen by the PHY manufacturer to line up with
>      that of the MAC reset.
> 
>   2) We could introduce a couple of new phy_* functions to disable &
>      enable low power states like the AR8031's hibernation feature, by
>      calling new function pointers in struct phy_driver. Then pch_gbe &
>      other MACs could call those to have the PHY driver disable
>      hibernation at times where we know we'll need the RX clock and
>      re-enable it afterwards.

Hi Paul

When there is no link, you don't need the MAC running. My assumption
is that the PHY is designed around that idea, you leave the MAC idle
until there is a link. When the phylib calls the link_change handler,
the MAC should then be started/stopped depending on the state of the
link. You are then guaranteed to have the clock when you need it.

I've no idea how easy this is to implement given the current code...

     Andrew