When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac:
add backplane link mode support"), Ioana Ciornei said [1]:
> ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed
> by Linux (since the firmware is not touching these). That being said,
> DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can
> also have their PCS managed by Linux (no interraction from the
> firmware's part with the PCS, just the SerDes).
This implies that Linux only manages the SerDes when the link type is
backplane. Modify the condition in dpaa2_mac_connect to reflect this,
moving the existing conditions to more appropriate places.
[1] https://lore.kernel.org/netdev/20210120221900.i6esmk6uadgqpdtu@skbuf/
Fixes: f978fe85b8d1 ("dpaa2-mac: configure the SerDes phy on a protocol change")
Signed-off-by: Sean Anderson <[email protected]>
---
For v2 I tested a variety of setups to try and determine what the
behavior is. I evaluated the following branches on a variety of commits
on an LS1088ARDB:
- net/master
- this commit alone
- my lynx10g series [1] alone
- both of the above together
I also switched between MC firmware 10.30 (no protocol change support)
and 10.34 (with protocol change support), and I tried MAC link types of
of FIXED, PHY, and BACKPLANE. After loading the MC firmware, DPC,
kernel, and dtb, I booted up and ran
$ ls-addni dpmac.1
I had a 10G fiber SFP module plugged in and connected on the other end
to my computer.
My results are as follows:
- When the link type is FIXED, all configurations work.
- PHY and BACKPLANE do not work on net/master.
- I occasionally saw an ENOTSUPP error from dpmac_set_protocol with MC
version 10.30. I am not sure what the cause of this is, as I was
unable to reproduce it reliably.
- Occasionally, the link did not come up with my lynx10g series without
this commit. Like the above issue, this would persist across reboots,
but switching to another configuration and back would often fix this
issue.
Unfortunately, I was unable to pinpoint any "smoking gun" due to
difficulty in reproducing errors. However, I still think this commit is
correct, and should be applied. If Linux and the MC are out of sync,
most of the time things will work correctly but occasionally they won't.
[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
But with some additional changes for v10.
Changes in v2:
- Fix incorrect condition in dpaa2_mac_set_supported_interfaces
drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index c886f33f8c6f..9b40c862d807 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -179,9 +179,13 @@ static void dpaa2_mac_config(struct phylink_config *config, unsigned int mode,
if (err)
netdev_err(mac->net_dev, "dpmac_set_protocol() = %d\n", err);
- err = phy_set_mode_ext(mac->serdes_phy, PHY_MODE_ETHERNET, state->interface);
- if (err)
- netdev_err(mac->net_dev, "phy_set_mode_ext() = %d\n", err);
+ if (!phy_interface_mode_is_rgmii(mode)) {
+ err = phy_set_mode_ext(mac->serdes_phy, PHY_MODE_ETHERNET,
+ state->interface);
+ if (err)
+ netdev_err(mac->net_dev, "phy_set_mode_ext() = %d\n",
+ err);
+ }
}
static void dpaa2_mac_link_up(struct phylink_config *config,
@@ -317,7 +321,8 @@ static void dpaa2_mac_set_supported_interfaces(struct dpaa2_mac *mac)
}
}
- if (!mac->serdes_phy)
+ if (!(mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE) ||
+ !mac->serdes_phy)
return;
/* In case we have access to the SerDes phy/lane, then ask the SerDes
@@ -377,8 +382,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
return -EINVAL;
mac->if_mode = err;
- if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE &&
- !phy_interface_mode_is_rgmii(mac->if_mode) &&
+ if (mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE &&
is_of_node(dpmac_node)) {
serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL);
--
2.35.1.1320.gc452695387.dirty
On Fri, Mar 03, 2023 at 07:31:59PM -0500, Sean Anderson wrote:
> When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac:
> add backplane link mode support"), Ioana Ciornei said [1]:
>
> > ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed
> > by Linux (since the firmware is not touching these). That being said,
> > DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can
> > also have their PCS managed by Linux (no interraction from the
> > firmware's part with the PCS, just the SerDes).
>
> This implies that Linux only manages the SerDes when the link type is
> backplane. Modify the condition in dpaa2_mac_connect to reflect this,
> moving the existing conditions to more appropriate places.
I am not sure I understand why are you moving the conditions to
different places. Could you please explain?
Why not just append the existing condition from dpaa2_mac_connect() with
"mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE"?
This way, the serdes_phy is populated only if all the conditions pass
and you don't have to scatter them all around the driver.
>
> [1] https://lore.kernel.org/netdev/20210120221900.i6esmk6uadgqpdtu@skbuf/
>
> Fixes: f978fe85b8d1 ("dpaa2-mac: configure the SerDes phy on a protocol change")
> Signed-off-by: Sean Anderson <[email protected]>
> ---
> For v2 I tested a variety of setups to try and determine what the
> behavior is. I evaluated the following branches on a variety of commits
> on an LS1088ARDB:
>
> - net/master
> - this commit alone
> - my lynx10g series [1] alone
> - both of the above together
>
> I also switched between MC firmware 10.30 (no protocol change support)
> and 10.34 (with protocol change support), and I tried MAC link types of
> of FIXED, PHY, and BACKPLANE. After loading the MC firmware, DPC,
> kernel, and dtb, I booted up and ran
>
> $ ls-addni dpmac.1
>
> I had a 10G fiber SFP module plugged in and connected on the other end
> to my computer.
>
> My results are as follows:
>
> - When the link type is FIXED, all configurations work.
> - PHY and BACKPLANE do not work on net/master.
> - I occasionally saw an ENOTSUPP error from dpmac_set_protocol with MC
> version 10.30. I am not sure what the cause of this is, as I was
> unable to reproduce it reliably.
> - Occasionally, the link did not come up with my lynx10g series without
> this commit. Like the above issue, this would persist across reboots,
> but switching to another configuration and back would often fix this
> issue.
>
> Unfortunately, I was unable to pinpoint any "smoking gun" due to
> difficulty in reproducing errors. However, I still think this commit is
> correct, and should be applied. If Linux and the MC are out of sync,
> most of the time things will work correctly but occasionally they won't.
>
> [1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
> But with some additional changes for v10.
>
> Changes in v2:
> - Fix incorrect condition in dpaa2_mac_set_supported_interfaces
>
> drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> index c886f33f8c6f..9b40c862d807 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> @@ -179,9 +179,13 @@ static void dpaa2_mac_config(struct phylink_config *config, unsigned int mode,
> if (err)
> netdev_err(mac->net_dev, "dpmac_set_protocol() = %d\n", err);
>
> - err = phy_set_mode_ext(mac->serdes_phy, PHY_MODE_ETHERNET, state->interface);
> - if (err)
> - netdev_err(mac->net_dev, "phy_set_mode_ext() = %d\n", err);
> + if (!phy_interface_mode_is_rgmii(mode)) {
> + err = phy_set_mode_ext(mac->serdes_phy, PHY_MODE_ETHERNET,
> + state->interface);
> + if (err)
> + netdev_err(mac->net_dev, "phy_set_mode_ext() = %d\n",
> + err);
> + }
> }
>
> static void dpaa2_mac_link_up(struct phylink_config *config,
> @@ -317,7 +321,8 @@ static void dpaa2_mac_set_supported_interfaces(struct dpaa2_mac *mac)
> }
> }
>
> - if (!mac->serdes_phy)
> + if (!(mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE) ||
> + !mac->serdes_phy)
> return;
For example, you removed the check against
DPAA2_MAC_FEATURE_PROTOCOL_CHANGE from below in dpaa2_mac_connect() just
to put it here.
>
> /* In case we have access to the SerDes phy/lane, then ask the SerDes
> @@ -377,8 +382,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
> return -EINVAL;
> mac->if_mode = err;
>
> - if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE &&
> - !phy_interface_mode_is_rgmii(mac->if_mode) &&
> + if (mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE &&
> is_of_node(dpmac_node)) {
> serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL);
>
> --
On 3/6/23 03:09, Ioana Ciornei wrote:
> On Fri, Mar 03, 2023 at 07:31:59PM -0500, Sean Anderson wrote:
>> When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac:
>> add backplane link mode support"), Ioana Ciornei said [1]:
>>
>> > ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed
>> > by Linux (since the firmware is not touching these). That being said,
>> > DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can
>> > also have their PCS managed by Linux (no interraction from the
>> > firmware's part with the PCS, just the SerDes).
>>
>> This implies that Linux only manages the SerDes when the link type is
>> backplane. Modify the condition in dpaa2_mac_connect to reflect this,
>> moving the existing conditions to more appropriate places.
>
> I am not sure I understand why are you moving the conditions to
> different places. Could you please explain?
This is not (just) a movement of conditions, but a changing of what they
apply to.
There are two things which this patch changes: whether we manage the phy
and whether we say we support alternate interfaces. According to your
comment above (and roughly in-line with my testing), Linux manages the
phy *exactly* when the link type is BACKPLANE. In all other cases, the
firmware manages the phy. Similarly, alternate interfaces are supported
*exactly* when the firmware supports PROTOCOL_CHANGE. However, currently
the conditions do not match this.
> Why not just append the existing condition from dpaa2_mac_connect() with
> "mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE"?
>
> This way, the serdes_phy is populated only if all the conditions pass
> and you don't have to scatter them all around the driver.
If we have link type BACKPLANE, Linux manages the phy, even if the
firmware doesn't support changing the interface. Therefore, we need to
grab the phy, but not fill in alternate interfaces.
This does not scatter the conditions, but instead moves them to exactly
where they are needed. Currently, they are in the wrong places.
--Sean
>>
>> [1] https://lore.kernel.org/netdev/20210120221900.i6esmk6uadgqpdtu@skbuf/
>>
>> Fixes: f978fe85b8d1 ("dpaa2-mac: configure the SerDes phy on a protocol change")
>> Signed-off-by: Sean Anderson <[email protected]>
>> ---
>> For v2 I tested a variety of setups to try and determine what the
>> behavior is. I evaluated the following branches on a variety of commits
>> on an LS1088ARDB:
>>
>> - net/master
>> - this commit alone
>> - my lynx10g series [1] alone
>> - both of the above together
>>
>> I also switched between MC firmware 10.30 (no protocol change support)
>> and 10.34 (with protocol change support), and I tried MAC link types of
>> of FIXED, PHY, and BACKPLANE. After loading the MC firmware, DPC,
>> kernel, and dtb, I booted up and ran
>>
>> $ ls-addni dpmac.1
>>
>> I had a 10G fiber SFP module plugged in and connected on the other end
>> to my computer.
>>
>> My results are as follows:
>>
>> - When the link type is FIXED, all configurations work.
>> - PHY and BACKPLANE do not work on net/master.
>> - I occasionally saw an ENOTSUPP error from dpmac_set_protocol with MC
>> version 10.30. I am not sure what the cause of this is, as I was
>> unable to reproduce it reliably.
>> - Occasionally, the link did not come up with my lynx10g series without
>> this commit. Like the above issue, this would persist across reboots,
>> but switching to another configuration and back would often fix this
>> issue.
>>
>> Unfortunately, I was unable to pinpoint any "smoking gun" due to
>> difficulty in reproducing errors. However, I still think this commit is
>> correct, and should be applied. If Linux and the MC are out of sync,
>> most of the time things will work correctly but occasionally they won't.
>>
>> [1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
>> But with some additional changes for v10.
>>
>> Changes in v2:
>> - Fix incorrect condition in dpaa2_mac_set_supported_interfaces
>>
>> drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
>> index c886f33f8c6f..9b40c862d807 100644
>> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
>> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
>> @@ -179,9 +179,13 @@ static void dpaa2_mac_config(struct phylink_config *config, unsigned int mode,
>> if (err)
>> netdev_err(mac->net_dev, "dpmac_set_protocol() = %d\n", err);
>>
>> - err = phy_set_mode_ext(mac->serdes_phy, PHY_MODE_ETHERNET, state->interface);
>> - if (err)
>> - netdev_err(mac->net_dev, "phy_set_mode_ext() = %d\n", err);
>> + if (!phy_interface_mode_is_rgmii(mode)) {
>> + err = phy_set_mode_ext(mac->serdes_phy, PHY_MODE_ETHERNET,
>> + state->interface);
>> + if (err)
>> + netdev_err(mac->net_dev, "phy_set_mode_ext() = %d\n",
>> + err);
>> + }
>> }
>>
>> static void dpaa2_mac_link_up(struct phylink_config *config,
>> @@ -317,7 +321,8 @@ static void dpaa2_mac_set_supported_interfaces(struct dpaa2_mac *mac)
>> }
>> }
>>
>> - if (!mac->serdes_phy)
>> + if (!(mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE) ||
>> + !mac->serdes_phy)
>> return;
>
> For example, you removed the check against
> DPAA2_MAC_FEATURE_PROTOCOL_CHANGE from below in dpaa2_mac_connect() just
> to put it here.
>
>>
>> /* In case we have access to the SerDes phy/lane, then ask the SerDes
>> @@ -377,8 +382,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
>> return -EINVAL;
>> mac->if_mode = err;
>>
>> - if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE &&
>> - !phy_interface_mode_is_rgmii(mac->if_mode) &&
>> + if (mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE &&
>> is_of_node(dpmac_node)) {
>> serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL);
>>
>> --
Hi all,
On 3/6/23 11:13, Sean Anderson wrote:
> On 3/6/23 03:09, Ioana Ciornei wrote:
>> On Fri, Mar 03, 2023 at 07:31:59PM -0500, Sean Anderson wrote:
>>> When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac:
>>> add backplane link mode support"), Ioana Ciornei said [1]:
>>>
>>> > ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed
>>> > by Linux (since the firmware is not touching these). That being said,
>>> > DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can
>>> > also have their PCS managed by Linux (no interraction from the
>>> > firmware's part with the PCS, just the SerDes).
>>>
>>> This implies that Linux only manages the SerDes when the link type is
>>> backplane. Modify the condition in dpaa2_mac_connect to reflect this,
>>> moving the existing conditions to more appropriate places.
>>
>> I am not sure I understand why are you moving the conditions to
>> different places. Could you please explain?
>
> This is not (just) a movement of conditions, but a changing of what they
> apply to.
>
> There are two things which this patch changes: whether we manage the phy
> and whether we say we support alternate interfaces. According to your
> comment above (and roughly in-line with my testing), Linux manages the
> phy *exactly* when the link type is BACKPLANE. In all other cases, the
> firmware manages the phy. Similarly, alternate interfaces are supported
> *exactly* when the firmware supports PROTOCOL_CHANGE. However, currently
> the conditions do not match this.
>
>> Why not just append the existing condition from dpaa2_mac_connect() with
>> "mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE"?
>>
>> This way, the serdes_phy is populated only if all the conditions pass
>> and you don't have to scatter them all around the driver.
>
> If we have link type BACKPLANE, Linux manages the phy, even if the
> firmware doesn't support changing the interface. Therefore, we need to
> grab the phy, but not fill in alternate interfaces.
>
> This does not scatter the conditions, but instead moves them to exactly
> where they are needed. Currently, they are in the wrong places.
>
> --Sean
I see this is marked as "changes requested" on patchwork. However, as explained
above, I do not think that the requested changes are necessary. Please review the
patch status.
--Sean
On Wed, 22 Mar 2023 15:52:23 -0400 Sean Anderson wrote:
> I see this is marked as "changes requested" on patchwork. However, as explained
> above, I do not think that the requested changes are necessary. Please review the
> patch status.
Let me document this, it keeps coming up. Patch incoming..
On 3/22/23 19:10, Jakub Kicinski wrote:
> On Wed, 22 Mar 2023 15:52:23 -0400 Sean Anderson wrote:
>> I see this is marked as "changes requested" on patchwork. However,
>> as explained above, I do not think that the requested changes are
>> necessary. Please review the patch status.
>
> Let me document this, it keeps coming up. Patch incoming..
This is documented in the commit message. In fact, I got almost the same
response from Ioana the last time, made the same explanation, and
updated the commit message, but he did not respond to any further
emails. If I resend again, what will prevent that happening again? I am
doing my best to try and clarify this patch, but I am getting no
feedback. Nothing like "OK, Sean that makes sense to me, please put it
in the commit message" or "No, I don't think that's right because of
XYZ."
--Sean
On Mon, Mar 06, 2023 at 11:13:17AM -0500, Sean Anderson wrote:
> On 3/6/23 03:09, Ioana Ciornei wrote:
> > On Fri, Mar 03, 2023 at 07:31:59PM -0500, Sean Anderson wrote:
> >> When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac:
> >> add backplane link mode support"), Ioana Ciornei said [1]:
> >>
> >> > ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed
> >> > by Linux (since the firmware is not touching these). That being said,
> >> > DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can
> >> > also have their PCS managed by Linux (no interraction from the
> >> > firmware's part with the PCS, just the SerDes).
> >>
> >> This implies that Linux only manages the SerDes when the link type is
> >> backplane. Modify the condition in dpaa2_mac_connect to reflect this,
> >> moving the existing conditions to more appropriate places.
> >
> > I am not sure I understand why are you moving the conditions to
> > different places. Could you please explain?
>
> This is not (just) a movement of conditions, but a changing of what they
> apply to.
>
> There are two things which this patch changes: whether we manage the phy
> and whether we say we support alternate interfaces. According to your
> comment above (and roughly in-line with my testing), Linux manages the
> phy *exactly* when the link type is BACKPLANE. In all other cases, the
> firmware manages the phy. Similarly, alternate interfaces are supported
> *exactly* when the firmware supports PROTOCOL_CHANGE. However, currently
> the conditions do not match this.
>
> > Why not just append the existing condition from dpaa2_mac_connect() with
> > "mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE"?
> >
> > This way, the serdes_phy is populated only if all the conditions pass
> > and you don't have to scatter them all around the driver.
>
> If we have link type BACKPLANE, Linux manages the phy, even if the
> firmware doesn't support changing the interface. Therefore, we need to
> grab the phy, but not fill in alternate interfaces.
>
> This does not scatter the conditions, but instead moves them to exactly
> where they are needed. Currently, they are in the wrong places.
Sorry for not making my position clear from the first time which is:
there is no point in having a SerDes driver or a reference to the
SerDes PHY if the firmware does not actually support changing of
interfaces.
Why I think that is because the SerDes is configured at boot time
anyway for the interface type defined in the RCW (Reset Configuration
Word). If the firmware does not support any protocol change then why
trouble the dpaa2-eth driver with anything SerDes related?
This is why I am ok with only extending the condition from
dpaa2_mac_connect() with an additional check but not the exact patch
that you sent.
Ioana
On 3/23/23 11:27, Ioana Ciornei wrote:
> On Mon, Mar 06, 2023 at 11:13:17AM -0500, Sean Anderson wrote:
>> On 3/6/23 03:09, Ioana Ciornei wrote:
>> > On Fri, Mar 03, 2023 at 07:31:59PM -0500, Sean Anderson wrote:
>> >> When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac:
>> >> add backplane link mode support"), Ioana Ciornei said [1]:
>> >>
>> >> > ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed
>> >> > by Linux (since the firmware is not touching these). That being said,
>> >> > DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can
>> >> > also have their PCS managed by Linux (no interraction from the
>> >> > firmware's part with the PCS, just the SerDes).
>> >>
>> >> This implies that Linux only manages the SerDes when the link type is
>> >> backplane. Modify the condition in dpaa2_mac_connect to reflect this,
>> >> moving the existing conditions to more appropriate places.
>> >
>> > I am not sure I understand why are you moving the conditions to
>> > different places. Could you please explain?
>>
>> This is not (just) a movement of conditions, but a changing of what they
>> apply to.
>>
>> There are two things which this patch changes: whether we manage the phy
>> and whether we say we support alternate interfaces. According to your
>> comment above (and roughly in-line with my testing), Linux manages the
>> phy *exactly* when the link type is BACKPLANE. In all other cases, the
>> firmware manages the phy. Similarly, alternate interfaces are supported
>> *exactly* when the firmware supports PROTOCOL_CHANGE. However, currently
>> the conditions do not match this.
>>
>> > Why not just append the existing condition from dpaa2_mac_connect() with
>> > "mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE"?
>> >
>> > This way, the serdes_phy is populated only if all the conditions pass
>> > and you don't have to scatter them all around the driver.
>>
>> If we have link type BACKPLANE, Linux manages the phy, even if the
>> firmware doesn't support changing the interface. Therefore, we need to
>> grab the phy, but not fill in alternate interfaces.
>>
>> This does not scatter the conditions, but instead moves them to exactly
>> where they are needed. Currently, they are in the wrong places.
>
> Sorry for not making my position clear from the first time which is:
> there is no point in having a SerDes driver or a reference to the
> SerDes PHY if the firmware does not actually support changing of
> interfaces.
>
> Why I think that is because the SerDes is configured at boot time
> anyway for the interface type defined in the RCW (Reset Configuration
> Word). If the firmware does not support any protocol change then why
> trouble the dpaa2-eth driver with anything SerDes related?
It's actually the other way around. If the firmware is managing the phy,
why try to probe it? Consider a situation where the firmware supports
protocol change, but the link type is PHY. Then we will probe the
serdes, but we may confuse the firmware (or vice versa).
> This is why I am ok with only extending the condition from
> dpaa2_mac_connect() with an additional check but not the exact patch
> that you sent.
AIUI the condition there is correct because Linux controls the PCS in
both PHY and BACKPLANE modes (although the RGMII check is a bit
strange).
--Sean
On Thu, Mar 23, 2023 at 12:30:34PM -0400, Sean Anderson wrote:
> On 3/23/23 11:27, Ioana Ciornei wrote:
> > On Mon, Mar 06, 2023 at 11:13:17AM -0500, Sean Anderson wrote:
> >> On 3/6/23 03:09, Ioana Ciornei wrote:
> >> > On Fri, Mar 03, 2023 at 07:31:59PM -0500, Sean Anderson wrote:
> >> >> When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac:
> >> >> add backplane link mode support"), Ioana Ciornei said [1]:
> >> >>
> >> >> > ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed
> >> >> > by Linux (since the firmware is not touching these). That being said,
> >> >> > DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can
> >> >> > also have their PCS managed by Linux (no interraction from the
> >> >> > firmware's part with the PCS, just the SerDes).
> >> >>
> >> >> This implies that Linux only manages the SerDes when the link type is
> >> >> backplane. Modify the condition in dpaa2_mac_connect to reflect this,
> >> >> moving the existing conditions to more appropriate places.
> >> >
> >> > I am not sure I understand why are you moving the conditions to
> >> > different places. Could you please explain?
> >>
> >> This is not (just) a movement of conditions, but a changing of what they
> >> apply to.
> >>
> >> There are two things which this patch changes: whether we manage the phy
> >> and whether we say we support alternate interfaces. According to your
> >> comment above (and roughly in-line with my testing), Linux manages the
> >> phy *exactly* when the link type is BACKPLANE. In all other cases, the
> >> firmware manages the phy. Similarly, alternate interfaces are supported
> >> *exactly* when the firmware supports PROTOCOL_CHANGE. However, currently
> >> the conditions do not match this.
> >>
> >> > Why not just append the existing condition from dpaa2_mac_connect() with
> >> > "mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE"?
> >> >
> >> > This way, the serdes_phy is populated only if all the conditions pass
> >> > and you don't have to scatter them all around the driver.
> >>
> >> If we have link type BACKPLANE, Linux manages the phy, even if the
> >> firmware doesn't support changing the interface. Therefore, we need to
> >> grab the phy, but not fill in alternate interfaces.
> >>
> >> This does not scatter the conditions, but instead moves them to exactly
> >> where they are needed. Currently, they are in the wrong places.
> >
> > Sorry for not making my position clear from the first time which is:
> > there is no point in having a SerDes driver or a reference to the
> > SerDes PHY if the firmware does not actually support changing of
> > interfaces.
> >
> > Why I think that is because the SerDes is configured at boot time
> > anyway for the interface type defined in the RCW (Reset Configuration
> > Word). If the firmware does not support any protocol change then why
> > trouble the dpaa2-eth driver with anything SerDes related?
>
> It's actually the other way around. If the firmware is managing the phy,
> why try to probe it? Consider a situation where the firmware supports
> protocol change, but the link type is PHY. Then we will probe the
> serdes, but we may confuse the firmware (or vice versa).
And how is that conflicting with what I said?
Again, I agree that we don't want to manage the SerDes PHY in situations
in which the firmware also does it. And that means adding and extra
check in the driver so that the SerDes PHY is setup only in BACKPLANE
mode.
>
> > This is why I am ok with only extending the condition from
> > dpaa2_mac_connect() with an additional check but not the exact patch
> > that you sent.
>
> AIUI the condition there is correct because Linux controls the PCS in
> both PHY and BACKPLANE modes (although the RGMII check is a bit
> strange).
>
I am not sure if we talk about the same check.
I was referring to this check which has nothing to do with the PCS
(which is why I don't understand why you mentioned it).
if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE &&
!phy_interface_mode_is_rgmii(mac->if_mode) &&
is_of_node(dpmac_node)) {
serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL);
Also, why is the RGMII check strange? RGMII does not use a SerDes PHY.
Ioana
On 3/24/23 09:07, Ioana Ciornei wrote:
> On Thu, Mar 23, 2023 at 12:30:34PM -0400, Sean Anderson wrote:
>> On 3/23/23 11:27, Ioana Ciornei wrote:
>> > On Mon, Mar 06, 2023 at 11:13:17AM -0500, Sean Anderson wrote:
>> >> On 3/6/23 03:09, Ioana Ciornei wrote:
>> >> > On Fri, Mar 03, 2023 at 07:31:59PM -0500, Sean Anderson wrote:
>> >> >> When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac:
>> >> >> add backplane link mode support"), Ioana Ciornei said [1]:
>> >> >>
>> >> >> > ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed
>> >> >> > by Linux (since the firmware is not touching these). That being said,
>> >> >> > DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can
>> >> >> > also have their PCS managed by Linux (no interraction from the
>> >> >> > firmware's part with the PCS, just the SerDes).
>> >> >>
>> >> >> This implies that Linux only manages the SerDes when the link type is
>> >> >> backplane. Modify the condition in dpaa2_mac_connect to reflect this,
>> >> >> moving the existing conditions to more appropriate places.
>> >> >
>> >> > I am not sure I understand why are you moving the conditions to
>> >> > different places. Could you please explain?
>> >>
>> >> This is not (just) a movement of conditions, but a changing of what they
>> >> apply to.
>> >>
>> >> There are two things which this patch changes: whether we manage the phy
>> >> and whether we say we support alternate interfaces. According to your
>> >> comment above (and roughly in-line with my testing), Linux manages the
>> >> phy *exactly* when the link type is BACKPLANE. In all other cases, the
>> >> firmware manages the phy. Similarly, alternate interfaces are supported
>> >> *exactly* when the firmware supports PROTOCOL_CHANGE. However, currently
>> >> the conditions do not match this.
>> >>
>> >> > Why not just append the existing condition from dpaa2_mac_connect() with
>> >> > "mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE"?
>> >> >
>> >> > This way, the serdes_phy is populated only if all the conditions pass
>> >> > and you don't have to scatter them all around the driver.
>> >>
>> >> If we have link type BACKPLANE, Linux manages the phy, even if the
>> >> firmware doesn't support changing the interface. Therefore, we need to
>> >> grab the phy, but not fill in alternate interfaces.
>> >>
>> >> This does not scatter the conditions, but instead moves them to exactly
>> >> where they are needed. Currently, they are in the wrong places.
>> >
>> > Sorry for not making my position clear from the first time which is:
>> > there is no point in having a SerDes driver or a reference to the
>> > SerDes PHY if the firmware does not actually support changing of
>> > interfaces.
>> >
>> > Why I think that is because the SerDes is configured at boot time
>> > anyway for the interface type defined in the RCW (Reset Configuration
>> > Word). If the firmware does not support any protocol change then why
>> > trouble the dpaa2-eth driver with anything SerDes related?
>>
>> It's actually the other way around. If the firmware is managing the phy,
>> why try to probe it? Consider a situation where the firmware supports
>> protocol change, but the link type is PHY. Then we will probe the
>> serdes, but we may confuse the firmware (or vice versa).
>
> And how is that conflicting with what I said?
The existing checks let the above scenario happen.
> Again, I agree that we don't want to manage the SerDes PHY in situations
> in which the firmware also does it. And that means adding and extra
> check in the driver so that the SerDes PHY is setup only in BACKPLANE
> mode.
>
>>
>> > This is why I am ok with only extending the condition from
>> > dpaa2_mac_connect() with an additional check but not the exact patch
>> > that you sent.
>>
>> AIUI the condition there is correct because Linux controls the PCS in
>> both PHY and BACKPLANE modes (although the RGMII check is a bit
>> strange).
>>
>
> I am not sure if we talk about the same check.
Ah, sorry. I was referring to the check for dpaa2_pcs_create.
> I was referring to this check which has nothing to do with the PCS
> (which is why I don't understand why you mentioned it).
>
> if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE &&
> !phy_interface_mode_is_rgmii(mac->if_mode) &&
> is_of_node(dpmac_node)) {
> serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL);
Say we have DPAA2_MAC_FEATURE_PROTOCOL_CHANGE and DPMAC_LINK_TYPE_PHY.
Then this clause will be taken, even though we are not manging the phy.
> Also, why is the RGMII check strange? RGMII does not use a SerDes PHY.
The RGMII check for the PCS should be in place for both PHY and BACKPLANE.
--Sean