[PATCH 2/2] dsa: mv88e6131: support fixed PHYs

[PATCH 2/2] dsa: mv88e6131: support fixed PHYs

[Tobias Waldekranz]

Statically setup the PCS Control on the MAC to match the fixed PHY.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6131.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
index eadb27c..d2b2fcf 100644
--- a/drivers/net/dsa/mv88e6131.c
+++ b/drivers/net/dsa/mv88e6131.c
@@ -337,6 +337,48 @@ mv88e6131_phy_write(struct dsa_switch *ds,
 	return mv88e6xxx_phy_write_ppu(ds, addr, regnum, val);
 }
 
+static void mv88e6131_adjust_link(struct dsa_switch *ds, int port,
+				  struct phy_device *phy)
+{
+	u16 pcs_ctrl;
+
+	/* PPU will configure MACs with physical PHYs */
+	if (!fixed_phy_get_status(phy))
+		return;
+
+	pcs_ctrl = mv88e6xxx_reg_read(ds, REG_PORT(port), 1);
+
+	/* clear all forced settings */
+	pcs_ctrl &= ~0xff;
+	pcs_ctrl |= BIT(2) | BIT(4) | BIT(6);
+
+	switch (phy->speed) {
+	case 10:
+		break;
+	case 100:
+		pcs_ctrl |= BIT(0);
+		break;
+	case 1000:
+		pcs_ctrl |= BIT(1);
+		break;
+
+	default:
+		netdev_err(ds->ports[port], "unsupported speed: %d\n", phy->speed);
+	}
+
+	if (phy->duplex)
+		pcs_ctrl |= BIT(3);
+
+	if (phy->link)
+		pcs_ctrl |= BIT(5);
+
+	if (phy->pause)
+		pcs_ctrl |= BIT(7);
+
+	mv88e6xxx_reg_write(ds, REG_PORT(port), 1, pcs_ctrl);
+	return;
+}
+
 static struct mv88e6xxx_hw_stat mv88e6131_hw_stats[] = {
 	{ "in_good_octets", 8, 0x00, },
 	{ "in_bad_octets", 4, 0x02, },
@@ -398,6 +440,7 @@ struct dsa_switch_driver mv88e6131_switch_driver = {
 	.set_addr		= mv88e6xxx_set_addr_direct,
 	.phy_read		= mv88e6131_phy_read,
 	.phy_write		= mv88e6131_phy_write,
+	.adjust_link		= mv88e6131_adjust_link,
 	.poll_link		= mv88e6xxx_poll_link,
 	.get_strings		= mv88e6131_get_strings,
 	.get_ethtool_stats	= mv88e6131_get_ethtool_stats,
-- 
1.8.4.357.g8d83871.dirty

Re: [PATCH 2/2] dsa: mv88e6131: support fixed PHYs

[Florian Fainelli]

2015-02-12 6:13 GMT-08:00 Tobias Waldekranz <tobias@waldekranz.com>:
> Statically setup the PCS Control on the MAC to match the fixed PHY.

bcm_sf2 supports both fixed PHYs and regular PHYs, yet we do not need
to get access to the fixed PHY status from the adjust_link callback
because you could implement a separate fixed_link_update callback for
that purpose.

Did not that work for you?

>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  drivers/net/dsa/mv88e6131.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
> index eadb27c..d2b2fcf 100644
> --- a/drivers/net/dsa/mv88e6131.c
> +++ b/drivers/net/dsa/mv88e6131.c
> @@ -337,6 +337,48 @@ mv88e6131_phy_write(struct dsa_switch *ds,
>         return mv88e6xxx_phy_write_ppu(ds, addr, regnum, val);
>  }
>
> +static void mv88e6131_adjust_link(struct dsa_switch *ds, int port,
> +                                 struct phy_device *phy)
> +{
> +       u16 pcs_ctrl;
> +
> +       /* PPU will configure MACs with physical PHYs */
> +       if (!fixed_phy_get_status(phy))
> +               return;
> +
> +       pcs_ctrl = mv88e6xxx_reg_read(ds, REG_PORT(port), 1);
> +
> +       /* clear all forced settings */
> +       pcs_ctrl &= ~0xff;
> +       pcs_ctrl |= BIT(2) | BIT(4) | BIT(6);
> +
> +       switch (phy->speed) {
> +       case 10:
> +               break;
> +       case 100:
> +               pcs_ctrl |= BIT(0);
> +               break;
> +       case 1000:
> +               pcs_ctrl |= BIT(1);
> +               break;
> +
> +       default:
> +               netdev_err(ds->ports[port], "unsupported speed: %d\n", phy->speed);
> +       }
> +
> +       if (phy->duplex)
> +               pcs_ctrl |= BIT(3);
> +
> +       if (phy->link)
> +               pcs_ctrl |= BIT(5);
> +
> +       if (phy->pause)
> +               pcs_ctrl |= BIT(7);
> +
> +       mv88e6xxx_reg_write(ds, REG_PORT(port), 1, pcs_ctrl);
> +       return;
> +}
> +
>  static struct mv88e6xxx_hw_stat mv88e6131_hw_stats[] = {
>         { "in_good_octets", 8, 0x00, },
>         { "in_bad_octets", 4, 0x02, },
> @@ -398,6 +440,7 @@ struct dsa_switch_driver mv88e6131_switch_driver = {
>         .set_addr               = mv88e6xxx_set_addr_direct,
>         .phy_read               = mv88e6131_phy_read,
>         .phy_write              = mv88e6131_phy_write,
> +       .adjust_link            = mv88e6131_adjust_link,
>         .poll_link              = mv88e6xxx_poll_link,
>         .get_strings            = mv88e6131_get_strings,
>         .get_ethtool_stats      = mv88e6131_get_ethtool_stats,
> --
> 1.8.4.357.g8d83871.dirty
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Florian

Re: [PATCH 2/2] dsa: mv88e6131: support fixed PHYs

[Tobias Waldekranz]

On Thu, Feb 12, 2015 at 08:13:28AM -0800, Florian Fainelli wrote:
> 2015-02-12 6:13 GMT-08:00 Tobias Waldekranz <tobias@waldekranz.com>:
> > Statically setup the PCS Control on the MAC to match the fixed PHY.
> 
> bcm_sf2 supports both fixed PHYs and regular PHYs, yet we do not need
> to get access to the fixed PHY status from the adjust_link callback
> because you could implement a separate fixed_link_update callback for
> that purpose.
> 
> Did not that work for you?
> 

That was my first approach and it worked fine. The only issue I saw
was that the callback was continously called at each poll cycle even
though the link state had not changed.

So then I implemented the same check for updates that was in the
regular adjust_link callback. But before I submitted that version of
the patch I looked att the sf2 code, and it seemed as though this code
uses the callback to update the phy status based on the chip state and
not the other way around. Did I misunderstand the code?

Not wanting to break your code, I went with this approach instead. But
if you're fine with it, I'm more than happy to go with that version.

-- 
Thanks
 - wkz

Re: [PATCH 2/2] dsa: mv88e6131: support fixed PHYs

[Florian Fainelli]

2015-02-21 2:30 GMT-08:00 Tobias Waldekranz <tobias@waldekranz.com>:
> On Thu, Feb 12, 2015 at 08:13:28AM -0800, Florian Fainelli wrote:
>> 2015-02-12 6:13 GMT-08:00 Tobias Waldekranz <tobias@waldekranz.com>:
>> > Statically setup the PCS Control on the MAC to match the fixed PHY.
>>
>> bcm_sf2 supports both fixed PHYs and regular PHYs, yet we do not need
>> to get access to the fixed PHY status from the adjust_link callback
>> because you could implement a separate fixed_link_update callback for
>> that purpose.
>>
>> Did not that work for you?
>>
>
> That was my first approach and it worked fine. The only issue I saw
> was that the callback was continously called at each poll cycle even
> though the link state had not changed.

Right, we poll using this callback fairly often. Just like the PHY
libraries adjust_link, drivers are responsible for implementing
"caching" and avoiding the callback to be invoked too frequently.

>
> So then I implemented the same check for updates that was in the
> regular adjust_link callback. But before I submitted that version of
> the patch I looked att the sf2 code, and it seemed as though this code
> uses the callback to update the phy status based on the chip state and
> not the other way around. Did I misunderstand the code?

It is a two step process:

- fixed_link_update updates the fixed PHY's status member to reflect
the HW changes (link change mostly), updates the PHY device
speed/duplex/pause parameters

- adjust_link reads the PHY device speed/duplex/pause and applies
these to the HW registers

>
> Not wanting to break your code, I went with this approach instead. But
> if you're fine with it, I'm more than happy to go with that version.

I think it is a little cleaner since the adjust_link callback does not
need to know what kind of PHY device it is dealing with, while the
fixed_link_update() one only takes care of fixed PHYs.

Thanks
-- 
Florian

Re: [PATCH 2/2] dsa: mv88e6131: support fixed PHYs

[Tobias Waldekranz]

On Sat, Feb 21, 2015 at 10:56:25AM -0800, Florian Fainelli wrote:
> 2015-02-21 2:30 GMT-08:00 Tobias Waldekranz <tobias@waldekranz.com>:
> > On Thu, Feb 12, 2015 at 08:13:28AM -0800, Florian Fainelli wrote:
> >> 2015-02-12 6:13 GMT-08:00 Tobias Waldekranz <tobias@waldekranz.com>:
> >> > Statically setup the PCS Control on the MAC to match the fixed PHY.
> >>
> >> bcm_sf2 supports both fixed PHYs and regular PHYs, yet we do not need
> >> to get access to the fixed PHY status from the adjust_link callback
> >> because you could implement a separate fixed_link_update callback for
> >> that purpose.
> >>
> >> Did not that work for you?
> >>
> >
> > That was my first approach and it worked fine. The only issue I saw
> > was that the callback was continously called at each poll cycle even
> > though the link state had not changed.
> 
> Right, we poll using this callback fairly often. Just like the PHY
> libraries adjust_link, drivers are responsible for implementing
> "caching" and avoiding the callback to be invoked too frequently.
> 
> >
> > So then I implemented the same check for updates that was in the
> > regular adjust_link callback. But before I submitted that version of
> > the patch I looked att the sf2 code, and it seemed as though this code
> > uses the callback to update the phy status based on the chip state and
> > not the other way around. Did I misunderstand the code?
> 
> It is a two step process:
> 
> - fixed_link_update updates the fixed PHY's status member to reflect
> the HW changes (link change mostly), updates the PHY device
> speed/duplex/pause parameters
> 
> - adjust_link reads the PHY device speed/duplex/pause and applies
> these to the HW registers

Right, in my case I just need to do an initial config according to the
fixed settings which are read from the device-tree.

In the case where there is a real PHY attached to the switch, an
internal machine will poll the PHY and setup the MAC accordingly. So
HW will take care of step 2 for me.

> >
> > Not wanting to break your code, I went with this approach instead. But
> > if you're fine with it, I'm more than happy to go with that version.
> 
> I think it is a little cleaner since the adjust_link callback does not
> need to know what kind of PHY device it is dealing with, while the
> fixed_link_update() one only takes care of fixed PHYs.

In my case I do need to know, since I want the switch's phy polling
unit to do the work when possible. Maybe I should just rethink the
whole thing and do the setup at probe-time using some other method.

I will get back to you with a better solution :)

> Thanks
> -- 
> Florian

-- 
Thanks
 - wkz

Re: [PATCH 2/2] dsa: mv88e6131: support fixed PHYs

[Florian Fainelli]

On 23/02/15 03:13, Tobias Waldekranz wrote:
> On Sat, Feb 21, 2015 at 10:56:25AM -0800, Florian Fainelli wrote:
>> 2015-02-21 2:30 GMT-08:00 Tobias Waldekranz <tobias@waldekranz.com>:
>>> On Thu, Feb 12, 2015 at 08:13:28AM -0800, Florian Fainelli wrote:
>>>> 2015-02-12 6:13 GMT-08:00 Tobias Waldekranz <tobias@waldekranz.com>:
>>>>> Statically setup the PCS Control on the MAC to match the fixed PHY.
>>>>
>>>> bcm_sf2 supports both fixed PHYs and regular PHYs, yet we do not need
>>>> to get access to the fixed PHY status from the adjust_link callback
>>>> because you could implement a separate fixed_link_update callback for
>>>> that purpose.
>>>>
>>>> Did not that work for you?
>>>>
>>>
>>> That was my first approach and it worked fine. The only issue I saw
>>> was that the callback was continously called at each poll cycle even
>>> though the link state had not changed.
>>
>> Right, we poll using this callback fairly often. Just like the PHY
>> libraries adjust_link, drivers are responsible for implementing
>> "caching" and avoiding the callback to be invoked too frequently.
>>
>>>
>>> So then I implemented the same check for updates that was in the
>>> regular adjust_link callback. But before I submitted that version of
>>> the patch I looked att the sf2 code, and it seemed as though this code
>>> uses the callback to update the phy status based on the chip state and
>>> not the other way around. Did I misunderstand the code?
>>
>> It is a two step process:
>>
>> - fixed_link_update updates the fixed PHY's status member to reflect
>> the HW changes (link change mostly), updates the PHY device
>> speed/duplex/pause parameters
>>
>> - adjust_link reads the PHY device speed/duplex/pause and applies
>> these to the HW registers
> 
> Right, in my case I just need to do an initial config according to the
> fixed settings which are read from the device-tree.
> 
> In the case where there is a real PHY attached to the switch, an
> internal machine will poll the PHY and setup the MAC accordingly. So
> HW will take care of step 2 for me.

Even if this is not a real PHY device, but e.g: a MAC-to-MAC type of
connection? Does the PHY PPU rely on being able to issue MDIO accesses?

> 
>>>
>>> Not wanting to break your code, I went with this approach instead. But
>>> if you're fine with it, I'm more than happy to go with that version.
>>
>> I think it is a little cleaner since the adjust_link callback does not
>> need to know what kind of PHY device it is dealing with, while the
>> fixed_link_update() one only takes care of fixed PHYs.
> 
> In my case I do need to know, since I want the switch's phy polling
> unit to do the work when possible. Maybe I should just rethink the
> whole thing and do the setup at probe-time using some other method.

You could do that, but it might be a little cumbersome, something
simpler could be just to have an initialization flag that ensures that
both of these callbacks run for at least once, and then do nothing?

> 
> I will get back to you with a better solution :)
> 
>> Thanks
>> -- 
>> Florian
> 


-- 
Florian

Re: [PATCH 2/2] dsa: mv88e6131: support fixed PHYs

[David Miller]

From: Tobias Waldekranz <tobias@waldekranz.com>
Date: Thu, 12 Feb 2015 15:13:17 +0100

> Statically setup the PCS Control on the MAC to match the fixed PHY.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
 ...
> +	/* PPU will configure MACs with physical PHYs */
> +	if (!fixed_phy_get_status(phy))
> +		return;

I kind of agree with Florian that you probably don't need this.

And furthermore, this fixed_phy_get_status() interface you created
isn't even used for what it's designed for.

As coded, fixed_phy_get_status() returns the link status structure,
but here you are only interested in whether a phy is fixed or not
because you're simply making a boolean test upon whether you were
given a non-NULL fixed phy status or not.