Re: [PATCH net-next v2 1/3] selftests: nic_basic_tests: Add selftest file for basic tests of NIC
On Thu, Sep 19, 2024 at 10:44:11AM +, mohan.pra...@microchip.com wrote:
> Hello Andrew,
>
> Thank you for the suggestion.
>
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > > > Since you have batteries included python:
> > > >
> > > > ethtool --json enp2s0
> > > > [sudo] password for andrew:
> > > > [ {
> > > > "ifname": "enp2s0",
> > > > "supported-ports": [ "TP","MII" ],
> > > > "supported-link-modes": [
> > > > "10baseT/Half","10baseT/Full","100baseT/Half","100baseT/Full","1000b
> > > > aseT/
> > > > Full" ],
> > > > "supported-pause-frame-use": "Symmetric Receive-only",
> > > > "supports-auto-negotiation": true,
> > > > "supported-fec-modes": [ ],
> > > > "advertised-link-modes": [
> > > > "10baseT/Half","10baseT/Full","100baseT/Half","100baseT/Full","1000b
> > > > aseT/
> > > > Full" ],
> > > > "advertised-pause-frame-use": "Symmetric Receive-only",
> > > > "advertised-auto-negotiation": true,
> > > > "advertised-fec-modes": [ ],
> > > > "auto-negotiation": false,
> > > > "master-slave-cfg": "preferred slave",
> > > > "master-slave-status": "unknown",
> > > > "port": "Twisted Pair",
> > > > "phyad": 0,
> > > > "transceiver": "external",
> > > > "supports-wake-on": "pumbg",
> > > > "wake-on": "d",
> > > > "link-detected": false
> > > > } ]
> > > >
> > > > You can use a json library to do all the parsing for you.
> > >
> > > I tried running the --json option with the ethtool ("ethtool --json
> > > enp9s0"),
> > however I am not getting the above output.
> > > Instead it always throws "ethtool: bad command line argument(s)"
> > > I am figuring out what might be missing (or any suggestions would be
> > helpful).
> >
> > Are you using real ethtool, or busybox? What version of ethtool? I'm using
> > 6.10, but it looks like JSON support was added somewhere around 5.10.
>
> I have been using ethtool 6.7, updating to ethtool 6.10 solved the problem.
It would be good to gracefully handle this. Have the test fail with a
human readable error indicating ethtool is too old, rather than just
throwing an exception etc.
Digging through the git history, it seems like 6.10 was actually the
first version that supported this:
commit bd1341cd2146bfb89e1239546299102339acbf4d
Author: Fabian Pfitzner
Date: Fri Jul 19 10:55:44 2024 +0200
add json support for base command
Most subcommands already implement json support for their output. The
base command (without supplying any subcommand) still lacks this
option. This patch implments the needed changes to get json output,
which is printed via "ethtool --json [iface]"
The following design decision were made during implementation:
- json values like Yes/No are printed as true/false
- values that are "Unknown" are not printed at all
- all other json values are not changed
- keys are printed in lowercase with dashes in between
Signed-off-by: Fabian Pfitzner
Andrew
Re: [PATCH net-next v2 1/3] selftests: nic_basic_tests: Add selftest file for basic tests of NIC
> > So i don't think this is a valid test. To really test autoneg off, you need > > to > > configure both ends of the link. > > I will change the implementation to configure both the ends of the link > appropriately in the next version. That would be good, but it does make the test and the test setup a lot more complex. I would suggest keeping such two target tests separate. Also, look to see if there are other such tests using two targets, and keep the basic configuration the same, IP address configuration, how to SSH between them etc. I'm not too familiar with the test framework. Maybe this is all a solved problem and there are helpers to use? Andrew
Re: [PATCH net-next v2 1/3] selftests: nic_basic_tests: Add selftest file for basic tests of NIC
> > Since you have batteries included python:
> >
> > ethtool --json enp2s0
> > [sudo] password for andrew:
> > [ {
> > "ifname": "enp2s0",
> > "supported-ports": [ "TP","MII" ],
> > "supported-link-modes": [
> > "10baseT/Half","10baseT/Full","100baseT/Half","100baseT/Full","1000baseT/
> > Full" ],
> > "supported-pause-frame-use": "Symmetric Receive-only",
> > "supports-auto-negotiation": true,
> > "supported-fec-modes": [ ],
> > "advertised-link-modes": [
> > "10baseT/Half","10baseT/Full","100baseT/Half","100baseT/Full","1000baseT/
> > Full" ],
> > "advertised-pause-frame-use": "Symmetric Receive-only",
> > "advertised-auto-negotiation": true,
> > "advertised-fec-modes": [ ],
> > "auto-negotiation": false,
> > "master-slave-cfg": "preferred slave",
> > "master-slave-status": "unknown",
> > "port": "Twisted Pair",
> > "phyad": 0,
> > "transceiver": "external",
> > "supports-wake-on": "pumbg",
> > "wake-on": "d",
> > "link-detected": false
> > } ]
> >
> > You can use a json library to do all the parsing for you.
>
> I tried running the --json option with the ethtool ("ethtool --json enp9s0"),
> however I am not getting the above output.
> Instead it always throws "ethtool: bad command line argument(s)"
> I am figuring out what might be missing (or any suggestions would be helpful).
Are you using real ethtool, or busybox? What version of ethtool? I'm
using 6.10, but it looks like JSON support was added somewhere around
5.10.
Andrew
Re: [PATCH net-next v2 1/3] selftests: nic_basic_tests: Add selftest file for basic tests of NIC
On Tue, Sep 17, 2024 at 08:04:07AM +0530, Mohan Prasad J wrote: > Add selftest file to test basic features of a NIC driver. > Tests for link modes, auto-negotiation are placed. > Selftest makes use of ksft modules and ethtool. > Add selftest file in the Makefile. > > Signed-off-by: Mohan Prasad J > --- > .../testing/selftests/drivers/net/hw/Makefile | 1 + > .../drivers/net/hw/nic_basic_tests.py | 145 ++ > 2 files changed, 146 insertions(+) > create mode 100644 tools/testing/selftests/drivers/net/hw/nic_basic_tests.py > > diff --git a/tools/testing/selftests/drivers/net/hw/Makefile > b/tools/testing/selftests/drivers/net/hw/Makefile > index c9f2f48fc..9f105227c 100644 > --- a/tools/testing/selftests/drivers/net/hw/Makefile > +++ b/tools/testing/selftests/drivers/net/hw/Makefile > @@ -10,6 +10,7 @@ TEST_PROGS = \ > hw_stats_l3.sh \ > hw_stats_l3_gre.sh \ > loopback.sh \ > + nic_basic_tests.py \ > pp_alloc_fail.py \ > rss_ctx.py \ > # > diff --git a/tools/testing/selftests/drivers/net/hw/nic_basic_tests.py > b/tools/testing/selftests/drivers/net/hw/nic_basic_tests.py > new file mode 100644 > index 0..27f780032 > --- /dev/null > +++ b/tools/testing/selftests/drivers/net/hw/nic_basic_tests.py > @@ -0,0 +1,145 @@ > +#!/usr/bin/env python3 > +# SPDX-License-Identifier: GPL-2.0 > + > +#Introduction: > +#This file has basic tests for generic NIC drivers. > +#The test comprises of auto-negotiation, speed and duplex checks. > +#Also has tests to check the throughput > +# > +#Setup: > +#Connect the DUT PC with NIC card to partner pc back via ethernet medium of > your choice(RJ45, T1) > +# > +#DUT PC Partner PC > +#┌───┐ > ┌──┐ > +#│ │ │ > │ > +#│ │ │ > │ > +#│ ┌───┐ │ > │ > +#│ │DUT NIC│ Eth │ > │ > +#│ │Interface ─┼─┼─any eth Interface > │ > +#│ └───┘ │ > │ > +#│ │ │ > │ > +#│ │ │ > │ > +#└───┘ > └──┘ > +# > +#Configurations: > +# Change the below configuration based on your hw needs. > +# """Default values""" > +sleep_time = 5 #time taken to wait for transitions to happen, in seconds. > +test_duration = 5 #performance test duration for the throughput check, in > seconds. > +throughput_threshold = 0.8 #percentage of throughput required to pass the > throughput > + > +import time > +import os > +import re > +import configparser > +import json > +from lib.py import ksft_run, ksft_exit, ksft_pr, ksft_eq > +from lib.py import KsftFailEx, KsftSkipEx > +from lib.py import NetDrvEpEnv > +from lib.py import cmd > +from lib.py import ethtool > + > +"""Global variables""" > +common_link_modes = [] Software engineers have a dislike for global variables. In this patch, it is not even used. Please consider replacing it by passing it as a parameter, or turn the code into a class and make it part of self. > +def test_link_modes(cfg) -> None: > +global common_link_modes > +link_modes = get_ethtool_content(cfg.ifname, "Supported link modes:") > +partner_link_modes = get_ethtool_content(cfg.ifname, "Link partner > advertised link modes:") > + > +if link_modes and partner_link_modes: > +for idx1 in range(len(link_modes)): > +for idx2 in range(len(partner_link_modes)): > +if link_modes[idx1] == partner_link_modes[idx2]: > +common_link_modes.append(link_modes[idx1]) You can use the power of python here. "supported-link-modes": [ "10baseT/Half","10baseT/Full","100baseT/Half","100baseT/Full","1000baseT/Full" ], "link-partner-modes": [ "10baseT/Half","10baseT/Full","100baseT/Half","100baseT/Full","1000baseT/Full" ], convert the list into a set, and then use 'and'. common_modes = set(josn['supported-link-modes']) and set(json['link-partner-modes']) Andrew
Re: [PATCH net-next v2 1/3] selftests: nic_basic_tests: Add selftest file for basic tests of NIC
> +def verify_link_up(ifname: str) -> None:
> +"""Verify whether the link is up"""
> +with open(f"/sys/class/net/{ifname}/operstate", "r") as fp:
> +link_state = fp.read().strip()
> +
> +if link_state == "down":
> +raise KsftSkipEx(f"Link state of interface {ifname} is DOWN")
> +
> +def set_autonegotiation_state(ifname: str, state: str) -> None:
> +content = get_ethtool_content(ifname, "Supported link modes:")
> +speeds, duplex_modes = get_speed_duplex(content)
> +speed = speeds[0]
> +duplex = duplex_modes[0]
> +if not speed or not duplex:
> +KsftSkipEx("No speed or duplex modes found")
> +"""Set the autonegotiation state for the interface"""
> +process = ethtool(f"-s {ifname} speed {speed} duplex {duplex} autoneg
> {state}")
> +def verify_autonegotiation(ifname: str, expected_state: str) -> None:
> +verify_link_up(ifname)
> +"""Verifying the autonegotiation state"""
> +output = get_ethtool_content(ifname, "Auto-negotiation:")
> +actual_state = output[0]
> +
> +ksft_eq(actual_state, expected_state)
> +
> +def test_link_modes(cfg) -> None:
> +global common_link_modes
> +link_modes = get_ethtool_content(cfg.ifname, "Supported link modes:")
> +partner_link_modes = get_ethtool_content(cfg.ifname, "Link partner
> advertised link modes:")
> +
> +if link_modes and partner_link_modes:
> +for idx1 in range(len(link_modes)):
> +for idx2 in range(len(partner_link_modes)):
> +if link_modes[idx1] == partner_link_modes[idx2]:
> +common_link_modes.append(link_modes[idx1])
> +break
> +else:
> +raise KsftFailEx("No link modes available")
> +
> +def test_autonegotiation(cfg) -> None:
> +autoneg = get_ethtool_content(cfg.ifname, "Supports auto-negotiation:")
> +if autoneg[0] == "Yes":
> +for state in ["off", "on"]:
> +set_autonegotiation_state(cfg.ifname, state)
> +time.sleep(sleep_time)
> +verify_autonegotiation(cfg.ifname, state)
If i'm understanding this correctly, you test with autoneg off, and
expect the link to come up. That only works reliably if the link peer
also has autoneg off, and is using the same speed/duplex.
What i guess is happening in your test setup is that the link peer is
failing autoneg and defaulting to 10/Half. But i don't think that is
guaranteed by 802.3. There are also a small number of devices which no
longer support 10/Half, they are likely to default to something
higher. This is especially true for datacenter NICs, they may start at
10G and go up from there.
So i don't think this is a valid test. To really test autoneg off, you
need to configure both ends of the link.
Andrew
Re: [PATCH net-next v2 1/3] selftests: nic_basic_tests: Add selftest file for basic tests of NIC
On Tue, Sep 17, 2024 at 08:04:07AM +0530, Mohan Prasad J wrote:
> Add selftest file to test basic features of a NIC driver.
> Tests for link modes, auto-negotiation are placed.
> Selftest makes use of ksft modules and ethtool.
> Add selftest file in the Makefile.
Thanks for reworking this.
> +++ b/tools/testing/selftests/drivers/net/hw/nic_basic_tests.py
> @@ -0,0 +1,145 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +#Introduction:
> +#This file has basic tests for generic NIC drivers.
> +#The test comprises of auto-negotiation, speed and duplex checks.
> +#Also has tests to check the throughput
> +#
> +#Setup:
> +#Connect the DUT PC with NIC card to partner pc back via ethernet medium of
> your choice(RJ45, T1)
> +#
> +#DUT PC Partner PC
> +#┌───┐
> ┌──┐
> +#│ │ │
> │
> +#│ │ │
> │
> +#│ ┌───┐ │
> │
> +#│ │DUT NIC│ Eth │
> │
> +#│ │Interface ─┼─┼─any eth Interface
> │
> +#│ └───┘ │
> │
> +#│ │ │
> │
> +#│ │ │
> │
> +#└───┘
> └──┘
> +#
> +#Configurations:
> +# Change the below configuration based on your hw needs.
> +# """Default values"""
> +sleep_time = 5 #time taken to wait for transitions to happen, in seconds.
> +test_duration = 5 #performance test duration for the throughput check, in
> seconds.
> +throughput_threshold = 0.8 #percentage of throughput required to pass the
> throughput
> +
> +import time
> +import os
> +import re
> +import configparser
> +import json
> +from lib.py import ksft_run, ksft_exit, ksft_pr, ksft_eq
> +from lib.py import KsftFailEx, KsftSkipEx
> +from lib.py import NetDrvEpEnv
> +from lib.py import cmd
> +from lib.py import ethtool
> +
> +"""Global variables"""
> +common_link_modes = []
> +
> +def get_ethtool_content(ifname: str, field: str):
> +capture = False
> +content = []
> +
> +"""Get the ethtool content for the interface"""
> +process = ethtool(f"{ifname}")
> +if process.ret != 0:
> +raise KsftSkipEx(f"Error while getting the ethtool content for
> interface {ifname}")
> +lines = process.stdout.splitlines()
> +
> +"""Retrieve the content of the field"""
> +for line in lines:
> +if field in line:
> +capture = True
> +data = line.split(":")[1].strip()
> +content.extend(data.split())
> +continue
Since you have batteries included python:
ethtool --json enp2s0
[sudo] password for andrew:
[ {
"ifname": "enp2s0",
"supported-ports": [ "TP","MII" ],
"supported-link-modes": [
"10baseT/Half","10baseT/Full","100baseT/Half","100baseT/Full","1000baseT/Full"
],
"supported-pause-frame-use": "Symmetric Receive-only",
"supports-auto-negotiation": true,
"supported-fec-modes": [ ],
"advertised-link-modes": [
"10baseT/Half","10baseT/Full","100baseT/Half","100baseT/Full","1000baseT/Full"
],
"advertised-pause-frame-use": "Symmetric Receive-only",
"advertised-auto-negotiation": true,
"advertised-fec-modes": [ ],
"auto-negotiation": false,
"master-slave-cfg": "preferred slave",
"master-slave-status": "unknown",
"port": "Twisted Pair",
"phyad": 0,
"transceiver": "external",
"supports-wake-on": "pumbg",
"wake-on": "d",
"link-detected": false
} ]
You can use a json library to do all the parsing for you.
> +def get_speed_duplex(content):
> +speed = []
> +duplex = []
> +"""Check the link modes"""
> +for data in content:
> +parts = data.split('/')
> +speed_value = re.match(r'\d+', parts[0])
> +if speed_value:
> +speed.append(speed_value.group())
> +else:
> +raise KsftSkipEx(f"No speed value found for interface {ifname}")
> +duplex.append(parts[1].lower())
> +return speed, duplex
> +
> +def verify_link_up(ifname: str) -> None:
> +"""Verify whether the link is up"""
> +with open(f"/sys/class/net/{ifname}/operstate", "r") as fp:
> +link_state = fp.read().strip()
> +
> +if link_state == "down":
> +raise KsftSkipEx(f"Link state of interface {ifname} is DOWN")
> +
> +def set_autonegotiation_state(ifname: str, state: str) -> None:
> +content = get_ethtool_content(ifname, "Supported link modes:")
> +
Re: [PATCH net-next 0/3] lan743x: This series of patches are for lan743x driver testing
> I am currently working on this and would rework as soon as possible. > The feedback that you provided is highly helpful and I will remodel the > implementation with these points in mind. > Hopefully you can see that in the next version. Great. Don't worry too much about link speeds you cannot test yourself. If your tests happen to fail on a 10G card, i would expect the Maintainer of the 10G card to debug if its the driver for the card or the test which is broken, and then help fix the test if its the test. Andrew
Re: [PATCH net-next 0/3] lan743x: This series of patches are for lan743x driver testing
On Fri, Sep 06, 2024 at 06:45:53AM +, mohan.pra...@microchip.com wrote: > Hello Andrew, > > Thank you for your review comments. > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > On Wed, Sep 04, 2024 at 03:45:46AM +0530, Mohan Prasad J wrote: > > > This series of patches are for testing the lan743x network driver. > > > Testing comprises autonegotiation, speed, duplex and throughput checks. > > > Tools such as ethtool, iperf3 are used in the testing process. > > > Performance test is done for TCP streams at different speeds. > > > > What is specific to lan743x? Why won't the autoneg test work for any > > interface which says it supports autoneg? Is duplex somehow special on the > > lan743x? > > > > Where possible, please try to make these tests generic, usable on any NIC. > > Or > > clearly document why they cannot be generic. > > > > As suggested, I will change the testcases to generic form and document them > accordingly in the next version. Great. How much time do you have? ethtool eth0 Settings for eth0: Supported ports: [ TPMII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Half 1000baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Half 1000baseT/Full Advertised pause frame use: Symmetric Receive-only Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full You can see that both the local device and the peer support auto-neg. You can see what link modes both the local and peer supports. The local device has 1000BaseT/Half where the peer does not, which is reasonably common. So you could use this as a basis for the test, ensurer 5 link modes do pass auto-neg, and one fails. If you can, please try to avoid hard coding any link modes. There will be some data centre NICs with a lowest speed to 10GBaseX, for example. There are some automotive devices with 10BaseT-1L which does not support autp-neg etc. It would be nice if the test could be used on any interface and the test will decide itself what can be tested, or if it should skip everything? And by the way, thanks for working on tests. We need more people like you contributing to them. Andrew
Re: [PATCH net-next 0/3] lan743x: This series of patches are for lan743x driver testing
On Wed, Sep 04, 2024 at 03:45:46AM +0530, Mohan Prasad J wrote: > This series of patches are for testing the lan743x network driver. > Testing comprises autonegotiation, speed, duplex and throughput checks. > Tools such as ethtool, iperf3 are used in the testing process. > Performance test is done for TCP streams at different speeds. What is specific to lan743x? Why won't the autoneg test work for any interface which says it supports autoneg? Is duplex somehow special on the lan743x? Where possible, please try to make these tests generic, usable on any NIC. Or clearly document why they cannot be generic. Andrew
Re: [RFC] Why is set_config not supported in mlx5_vnet?
On Mon, Aug 26, 2024 at 11:06:09AM +0200, Dragos Tatulea wrote: > > > On 23.08.24 18:54, Carlos Bilbao wrote: > > Hello, > > > > I'm debugging my vDPA setup, and when using ioctl to retrieve the > > configuration, I noticed that it's running in half duplex mode: > > > > Configuration data (24 bytes): > > MAC address: (Mac address) > > Status: 0x0001 > > Max virtqueue pairs: 8 > > MTU: 1500 > > Speed: 0 Mb > > Duplex: Half Duplex > > RSS max key size: 0 > > RSS max indirection table length: 0 > > Supported hash types: 0x > > > > I believe this might be contributing to the underperformance of vDPA. > mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX > feature which reports speed and duplex. You can check the state on the > PF. Then it should probably report DUPLEX_UNKNOWN. The speed of 0 also suggests SPEED_UNKNOWN is not being returned. So this just looks buggy in general. Andrew
Re: [RFC] Why is set_config not supported in mlx5_vnet?
On Fri, Aug 23, 2024 at 11:54:13AM -0500, Carlos Bilbao wrote: > Hello, > > I'm debugging my vDPA setup, and when using ioctl to retrieve the > configuration, I noticed that it's running in half duplex mode: > > Configuration data (24 bytes): > MAC address: (Mac address) > Status: 0x0001 > Max virtqueue pairs: 8 > MTU: 1500 > Speed: 0 Mb > Duplex: Half Duplex If the speed is 0, does duplex even matter? Andrew
Re: [PATCH v7 3/3] vdpa/mlx5: Add the support of set mac address
> +static int mlx5_vdpa_set_attr(struct vdpa_mgmt_dev *v_mdev, struct
> vdpa_device *dev,
> + const struct vdpa_dev_set_config *add_config)
> +{
> + struct virtio_net_config *config;
> + struct mlx5_core_dev *pfmdev;
> + struct mlx5_vdpa_dev *mvdev;
> + struct mlx5_vdpa_net *ndev;
> + struct mlx5_core_dev *mdev;
> + int err = -EINVAL;
I would say this should also be EOPNOTSUPP.
> +
> + mvdev = to_mvdev(dev);
> + ndev = to_mlx5_vdpa_ndev(mvdev);
> + mdev = mvdev->mdev;
> + config = &ndev->config;
> +
> + down_write(&ndev->reslock);
> + if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
> + pfmdev = pci_get_drvdata(pci_physfn(mdev->pdev));
> + err = mlx5_mpfs_add_mac(pfmdev, config->mac);
> + if (!err)
> + ether_addr_copy(config->mac, add_config->net.mac);
> + }
> +
> + up_write(&ndev->reslock);
> + return err;
Andrew
---
pw-bot: cr
Re: [PATCH v7 2/3] vdpa_sim_net: Add the support of set mac address
> +static int vdpasim_net_set_attr(struct vdpa_mgmt_dev *mdev, struct
> vdpa_device *dev,
> + const struct vdpa_dev_set_config *config)
> +{
> + struct vdpasim *vdpasim = container_of(dev, struct vdpasim, vdpa);
> + struct virtio_net_config *vio_config = vdpasim->config;
> +
> + mutex_lock(&vdpasim->mutex);
> +
> + if (config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
> + ether_addr_copy(vio_config->mac, config->net.mac);
> + mutex_unlock(&vdpasim->mutex);
> + return 0;
> + }
> +
> + mutex_unlock(&vdpasim->mutex);
> + return -EINVAL;
EOPNOTSUPP would be more appropriate.
Andrew
Re: [PATCH v7 1/3] vdpa: support set mac address from vdpa tool
> +static int vdpa_dev_net_device_attr_set(struct vdpa_device *vdev,
> + struct genl_info *info)
> +{
> + struct vdpa_dev_set_config set_config = {};
> + struct vdpa_mgmt_dev *mdev = vdev->mdev;
> + struct nlattr **nl_attrs = info->attrs;
> + const u8 *macaddr;
> + int err = -EINVAL;
> +
> + down_write(&vdev->cf_lock);
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> + set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
> +
> + if (is_valid_ether_addr(macaddr)) {
> + ether_addr_copy(set_config.net.mac, macaddr);
> + memcpy(set_config.net.mac, macaddr, ETH_ALEN);
ether_addr_copy() and memcpy()?
> + if (mdev->ops->dev_set_attr) {
> + err = mdev->ops->dev_set_attr(mdev, vdev,
> + &set_config);
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +"device does not support
> changing the MAC address");
You would generally return EOPNOTSUPP in this case, not EINVAL.
Also, the device does not support setting attributes. Given the
generic name, i assume you plan to set other attributes in the future,
at which point this error message will be wrong.
Andrew
Re: [PATH v5 2/3] vdpa_sim_net: Add the support of set mac address
> +static int vdpasim_net_set_attr(struct vdpa_mgmt_dev *mdev,
> + struct vdpa_device *dev,
> + const struct vdpa_dev_set_config *config)
> +{
> + struct vdpasim *vdpasim = container_of(dev, struct vdpasim, vdpa);
> + struct virtio_net_config *vio_config = vdpasim->config;
> +
> + mutex_lock(&vdpasim->mutex);
> +
> + if (config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
> + memcpy(vio_config->mac, config->net.mac, ETH_ALEN);
ether_addr_copy()
Andrew
Re: [PATH v5 1/3] vdpa: support set mac address from vdpa tool
On Tue, Jul 23, 2024 at 01:39:20PM +0800, Cindy Lu wrote:
> Add new UAPI to support the mac address from vdpa tool
> Function vdpa_nl_cmd_dev_attr_set_doit() will get the
> new MAC address from the vdpa tool and then set it to the device.
>
> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
>
> Here is example:
> root@L1# vdpa -jp dev config show vdpa0
> {
> "config": {
> "vdpa0": {
> "mac": "82:4d:e9:5d:d7:e6",
> "link ": "up",
> "link_announce ": false,
> "mtu": 1500
> }
> }
> }
>
> root@L1# vdpa dev set name vdpa0 mac 00:11:22:33:44:55
>
> root@L1# vdpa -jp dev config show vdpa0
> {
> "config": {
> "vdpa0": {
> "mac": "00:11:22:33:44:55",
> "link ": "up",
> "link_announce ": false,
> "mtu": 1500
> }
> }
> }
>
> Signed-off-by: Cindy Lu
> ---
> drivers/vdpa/vdpa.c | 84 +++
> include/linux/vdpa.h | 9 +
> include/uapi/linux/vdpa.h | 1 +
> 3 files changed, 94 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 8d391947eb8d..07d61ee62839 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -1361,6 +1361,85 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct
> sk_buff *skb, struct genl_info
> return err;
> }
>
> +static int vdpa_dev_net_device_attr_set(struct vdpa_device *vdev,
> + struct genl_info *info)
> +{
> + struct vdpa_dev_set_config set_config = {};
> + const u8 *macaddr;
> + struct vdpa_mgmt_dev *mdev = vdev->mdev;
> + struct nlattr **nl_attrs = info->attrs;
> + int err = -EINVAL;
> +
> + if (!vdev->mdev)
> + return -EINVAL;
> +
> + down_write(&vdev->cf_lock);
> + if ((mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC)) &&
> + nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> + set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
> +
> + if (is_valid_ether_addr(macaddr)) {
> + memcpy(set_config.net.mac, macaddr, ETH_ALEN);
> + if (mdev->ops->dev_set_attr) {
> + err = mdev->ops->dev_set_attr(mdev, vdev,
> + &set_config);
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +"device not supported");
> + }
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +"Invalid MAC address");
> + }
> + }
> + up_write(&vdev->cf_lock);
> + return err;
> +}
> +static int vdpa_nl_cmd_dev_attr_set_doit(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + const char *name;
> + int err = 0;
> + struct device *dev;
> + struct vdpa_device *vdev;
> + u64 classes;
> +
> + if (!info->attrs[VDPA_ATTR_DEV_NAME])
> + return -EINVAL;
> +
> + name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> +
> + down_write(&vdpa_dev_lock);
> + dev = bus_find_device(&vdpa_bus, NULL, name, vdpa_name_match);
> + if (!dev) {
> + NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> + err = -ENODEV;
> + goto dev_err;
> + }
> + vdev = container_of(dev, struct vdpa_device, dev);
> + if (!vdev->mdev) {
> + NL_SET_ERR_MSG_MOD(
> + info->extack,
> + "Fail to find the specified management device");
> + err = -EINVAL;
> + goto mdev_err;
> + }
> + classes = vdpa_mgmtdev_get_classes(vdev->mdev, NULL);
> + if (classes & BIT_ULL(VIRTIO_ID_NET)) {
> + err = vdpa_dev_net_device_attr_set(vdev, info);
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack, "%s device not supported",
> +name);
> + }
> +
> +mdev_err:
> + put_device(dev);
> +dev_err:
> + up_write(&vdpa_dev_lock);
> + return err;
> +}
> +
> static int vdpa_dev_config_dump(struct device *dev, void *data)
> {
> struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
> @@ -1497,6 +1576,11 @@ static const struct genl_ops vdpa_nl_ops[] = {
> .doit = vdpa_nl_cmd_dev_stats_get_doit,
> .flags = GENL_ADMIN_PERM,
> },
> + {
> + .cmd = VDPA_CMD_DEV_ATTR_SET,
> + .doit = vdpa_nl_cmd_dev_attr_set_doit,
> + .flags = GENL_ADMIN_PERM,
> + },
> };
>
> static struct genl_family vdpa_nl_family __ro_after_init = {
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 7977ca03ac7a..3511156c10db
Re: [PATH v5 0/3] vdpa: support set mac address from vdpa tool
On Tue, Jul 23, 2024 at 01:39:19PM +0800, Cindy Lu wrote: > Add support for setting the MAC address using the VDPA tool. > This feature will allow setting the MAC address using the VDPA tool. > For example, in vdpa_sim_net, the implementation sets the MAC address > to the config space. However, for other drivers, they can implement their > own function, not limited to the config space. > > Changelog v2 > - Changed the function name to prevent misunderstanding > - Added check for blk device > - Addressed the comments > Changelog v3 > - Split the function of the net device from vdpa_nl_cmd_dev_attr_set_doit > - Add a lock for the network device's dev_set_attr operation > - Address the comments > Changelog v4 > - Address the comments > - Add a lock for the vdap_sim?_net device's dev_set_attr operation > Changelog v5 > - Address the comments This history is to help reviewers of previous versions know if there comments have been addressed. Just saying 'Address the comments' is not useful. Please give a one line summary of each of the comment which has been addressed, maybe including how it was addressed. Andrew
Re: [PATCH] vdpa/mlx5: Add the support of set mac address
On Mon, Jul 08, 2024 at 02:55:49PM +0800, Cindy Lu wrote:
> Add the function to support setting the MAC address.
> For vdpa/mlx5, the function will use mlx5_mpfs_add_mac
> to set the mac address
>
> Tested in ConnectX-6 Dx device
>
> Signed-off-by: Cindy Lu
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 26ba7da6b410..f78701386690 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3616,10 +3616,33 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev
> *v_mdev, struct vdpa_device *
> destroy_workqueue(wq);
> mgtdev->ndev = NULL;
> }
> +static int mlx5_vdpa_set_attr_mac(struct vdpa_mgmt_dev *v_mdev,
> + struct vdpa_device *dev,
> + const struct vdpa_dev_set_config *add_config)
> +{
> + struct mlx5_vdpa_dev *mvdev = to_mvdev(dev);
> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> + struct mlx5_core_dev *mdev = mvdev->mdev;
> + struct virtio_net_config *config = &ndev->config;
> + int err;
> + struct mlx5_core_dev *pfmdev;
> +
> + if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
> + if (!is_zero_ether_addr(add_config->net.mac)) {
Is the core happy to call into the driver without validating the MAC
address? Will the core pass the broadcast address? That is not
zero. Or a multicast address? Should every driver repeat the same
validation, and probably get it just as wrong?
Andrew
---
pw-bot: cr
Re: [RFC PATCH v3 0/7] Add virtio_rtc module and related changes
> As long as it doesn't behave differently from the other RTC, I'm fine > with this. This is important because I don't want to carry any special > infrastructure for this driver or to have to special case this driver > later on because it is incompatible with some evolution of the > subsystem. Maybe deliberately throw away all the sub-second accuracy when accessing the time via the RTC API? That helps to make it look like an RTC. And when doing the rounding, add a constant offset of 10ms to emulate the virtual i2c bus it is hanging off, like most RTCs? Andrew
Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write
> Having worked with closed-source systems, especially VxWorks, for many > years (where the header files contain all the documentation), it just > seems strange to embed the documentation in the .c files. The key words here might be closed-source. With such black boxes, you don't have access the sources. You cannot look at the source to understand how a function works. In the open source world, the comments partially function as an introduction to reading the code and understanding what it does. You are also encouraged to change the code if needed, which in the closed source world you cannot do. Given this discussion, i now think putting the documentation in the .c file makes more sense. For the generated documentation it does not matter, but for the reader of the code, having it in the .c files does seem to make sense. Andrew
Re: [PATCH net-next v2 2/2] net: ethernet: mediatek: support custom GMAC label
On Mon, Apr 19, 2021 at 08:46:59AM -0700, Ilya Lipnitskiy wrote: > The MAC device name can now be set within DTS file instead of always > being "ethX". This is helpful for DSA to clearly label the DSA master > device and distinguish it from DSA slave ports. > > For example, some devices, such as the Ubiquiti EdgeRouter X, may have > ports labeled ethX. Labeling the master GMAC with a different prefix > than DSA ports helps with clarity. > > Suggested-by: René van Dorst > Signed-off-by: Ilya Lipnitskiy Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next 2/2] net: ethernet: mediatek: support custom GMAC label
On Sun, Apr 18, 2021 at 09:03:52PM -0700, Ilya Lipnitskiy wrote:
> The MAC device name can now be set within DTS file instead of always
> being "ethX". This is helpful for DSA to clearly label the DSA master
> device and distinguish it from DSA slave ports.
>
> For example, some devices, such as the Ubiquiti EdgeRouter X, may have
> ports labeled ethX. Labeling the master GMAC with a different prefix
> than DSA ports helps with clarity.
>
> Suggested-by: René van Dorst
> Signed-off-by: Ilya Lipnitskiy
> ---
> drivers/net/ethernet/mediatek/mtk_eth_soc.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 6b00c12c6c43..4c0ce4fb7735 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -2845,6 +2845,7 @@ static const struct net_device_ops mtk_netdev_ops = {
>
> static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np)
> {
> + const char *label = of_get_property(np, "label", NULL);
> const __be32 *_id = of_get_property(np, "reg", NULL);
> phy_interface_t phy_mode;
> struct phylink *phylink;
> @@ -2940,6 +2941,9 @@ static int mtk_add_mac(struct mtk_eth *eth, struct
> device_node *np)
> else
> eth->netdev[id]->max_mtu = MTK_MAX_RX_LENGTH_2K -
> MTK_RX_ETH_HLEN;
>
> + if (label)
> + strscpy(eth->netdev[id]->name, label, IFNAMSIZ);
It is better to use alloc_netdev_mqs() so you get validation the name
is unique.
Andrew
Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero
> Currently this code is implemented in pci_bus_find_domain_nr() function. > IIRC domain number is 16bit integer, so plain bitmap would consume 8 kB > of memory. I'm not sure if it is fine or some other tree-based structure > for allocated domain numbers is needed. Hi Pali Have a look at lib/idr.c Andrew
Re: [PATCH net-next,v2] net: ethernet: mediatek: ppe: fix busy wait loop
On Thu, Apr 15, 2021 at 05:11:48PM -0700, Ilya Lipnitskiy wrote:
> The intention is for the loop to timeout if the body does not succeed.
> The current logic calls time_is_before_jiffies(timeout) which is false
> until after the timeout, so the loop body never executes.
>
> Fix by using readl_poll_timeout as a more standard and less error-prone
> solution.
>
> Fixes: ba37b7caf1ed ("net: ethernet: mtk_eth_soc: add support for
> initializing the PPE")
> Signed-off-by: Ilya Lipnitskiy
> Cc: Felix Fietkau
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH v3 net-next 08/10] net: korina: Get mdio input clock via common clock framework
> @@ -1079,6 +1078,14 @@ static int korina_probe(struct platform_device *pdev) > eth_hw_addr_random(dev); > } > > + clk = devm_clk_get(&pdev->dev, NULL); You should use a name here. It makes future expansion of the binding easier. devm_clk_get_optional() is probably better. If there is a real error it will return an error. If the clock does not exist, you get a NULL. Real errors should cause the problem to fail, but with a NULL you can use the fallback value. You also need to document the device tree binding. Andrew
Re: [PATCH v3 net-next 07/10] net: korina: Add support for device tree
> - memcpy(dev->dev_addr, mac_addr, ETH_ALEN);
> + if (mac_addr) {
> + ether_addr_copy(dev->dev_addr, mac_addr);
> + } else {
> + u8 ofmac[ETH_ALEN];
> +
> + if (of_get_mac_address(pdev->dev.of_node, ofmac) == 0)
> + ether_addr_copy(dev->dev_addr, ofmac);
You should be able to skip the ether_addr_copy() by passing
dev->dev_addr directly to of_get_mac_address().
> + else
> + eth_hw_addr_random(dev);
> + }
>
> lp->rx_irq = platform_get_irq_byname(pdev, "korina_rx");
> lp->tx_irq = platform_get_irq_byname(pdev, "korina_tx");
> @@ -1146,8 +1157,21 @@ static int korina_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id korina_match[] = {
> + {
> + .compatible = "idt,3243x-emac",
You need to document this compatible somewhere under
Documentation/devicetree/binding
Andrew
Re: [PATCH v3 net-next 06/10] net: korina: Only pass mac address via platform data
On Thu, Apr 15, 2021 at 01:06:43AM +0200, Thomas Bogendoerfer wrote:
> Get rid of access to struct korina_device by just passing the mac
> address via platform data and use drvdata for passing netdev to remove
> function.
>
> Signed-off-by: Thomas Bogendoerfer
> ---
> arch/mips/rb532/devices.c | 5 +++--
> drivers/net/ethernet/korina.c | 11 ++-
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/arch/mips/rb532/devices.c b/arch/mips/rb532/devices.c
> index dd34f1b32b79..5fc3c8ee4f31 100644
> --- a/arch/mips/rb532/devices.c
> +++ b/arch/mips/rb532/devices.c
> @@ -105,6 +105,9 @@ static struct platform_device korina_dev0 = {
> .name = "korina",
> .resource = korina_dev0_res,
> .num_resources = ARRAY_SIZE(korina_dev0_res),
> + .dev = {
> + .platform_data = &korina_dev0_data.mac,
> + }
This is a bit unusual. Normally you define a structure in
include/linux/platform/data/koriana.h, and use that.
What about the name? "korina0" How is that passed?
Andrew
Re: [PATCH v3 net-next 05/10] net: korina: Use DMA API
On Thu, Apr 15, 2021 at 01:06:42AM +0200, Thomas Bogendoerfer wrote: > Instead of messing with MIPS specific macros use DMA API for mapping > descriptors and skbs. > > Signed-off-by: Thomas Bogendoerfer Reviewed-by: Andrew Lunn Andrew
Re: [PATCH v3 net-next 04/10] net: korina: Remove nested helpers
On Thu, Apr 15, 2021 at 01:06:41AM +0200, Thomas Bogendoerfer wrote: > Remove helpers, which are only used in one call site. > > Signed-off-by: Thomas Bogendoerfer Reviewed-by: Andrew Lunn Andrew
Re: [PATCH v3 net-next 03/10] net: korina: Remove not needed cache flushes
On Thu, Apr 15, 2021 at 01:06:40AM +0200, Thomas Bogendoerfer wrote: > Descriptors are mapped uncached so there is no need to do any cache > handling for them. > > Signed-off-by: Thomas Bogendoerfer Reviewed-by: Andrew Lunn Andrew
Re: [PATCH v3 net-next 02/10] net: korina: Use devres functions
On Thu, Apr 15, 2021 at 01:06:39AM +0200, Thomas Bogendoerfer wrote: > Simplify probe/remove code by using devm_ functions. > > Signed-off-by: Thomas Bogendoerfer Reviewed-by: Andrew Lunn Andrew
Re: [PATCH v3 net-next 01/10] net: korina: Fix MDIO functions
> +static int korina_mdio_wait(struct korina_private *lp)
> +{
> + u32 value;
> +
> + return readl_poll_timeout_atomic(&lp->eth_regs->miimind,
> + value, value & ETH_MII_IND_BSY,
> + 1, 1000);
> +}
> +
> +static int korina_mdio_read(struct net_device *dev, int phy, int reg)
> {
> struct korina_private *lp = netdev_priv(dev);
> int ret;
>
> - mii_id = ((lp->rx_irq == 0x2c ? 1 : 0) << 8);
> + if (korina_mdio_wait(lp))
> + return -ETIMEDOUT;
>
> - writel(0, &lp->eth_regs->miimcfg);
> - writel(0, &lp->eth_regs->miimcmd);
> - writel(mii_id | reg, &lp->eth_regs->miimaddr);
> - writel(ETH_MII_CMD_SCN, &lp->eth_regs->miimcmd);
> + writel(phy << 8 | reg, &lp->eth_regs->miimaddr);
> + writel(1, &lp->eth_regs->miimcmd);
> +
> + if (korina_mdio_wait(lp))
> + return -ETIMEDOUT;
Just return what readl_poll_timeout_atomic() returns. In general, you
should not change error codes.
>
> - ret = (int)(readl(&lp->eth_regs->miimrdd));
> + if (readl(&lp->eth_regs->miimind) & ETH_MII_IND_NV)
> + return -1;
Please use -ESOMETHING, not -1.
Andrew
Re: [PATCH net-next] net: ethernet: mediatek: ppe: fix busy wait loop
On Thu, Apr 15, 2021 at 04:02:34PM -0700, Ilya Lipnitskiy wrote:
> The intention is for the loop to timeout if the body does not succeed.
> The current logic calls time_is_before_jiffies(timeout) which is false
> until after the timeout, so the loop body never executes.
>
> time_is_after_jiffies(timeout) will return true until timeout is less
> than jiffies, which is the intended behavior here.
>
> Fixes: ba37b7caf1ed ("net: ethernet: mtk_eth_soc: add support for
> initializing the PPE")
> Signed-off-by: Ilya Lipnitskiy
> Cc: Felix Fietkau
> ---
> drivers/net/ethernet/mediatek/mtk_ppe.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.c
> b/drivers/net/ethernet/mediatek/mtk_ppe.c
> index 71e1ccea6e72..af3c266297aa 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe.c
> @@ -46,7 +46,7 @@ static int mtk_ppe_wait_busy(struct mtk_ppe *ppe)
> {
> unsigned long timeout = jiffies + HZ;
>
> - while (time_is_before_jiffies(timeout)) {
> + while (time_is_after_jiffies(timeout)) {
> if (!(ppe_r32(ppe, MTK_PPE_GLO_CFG) & MTK_PPE_GLO_CFG_BUSY))
> return 0;
Maybe see is iopoll.h can be used.
Andrew
>
> --
> 2.31.1
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH v2 1/2] net: phy: add genphy_c45_pma_suspend/resume
On Thu, Apr 15, 2021 at 12:25:37PM +0300, Radu Pirea (NXP OSS) wrote: > Add generic PMA suspend and resume callback functions for C45 PHYs. > > Signed-off-by: Radu Pirea (NXP OSS) Reviewed-by: Andrew Lunn Andrew
Re: [PATCH v2 2/2] phy: nxp-c45: add driver for tja1103
> +config NXP_C45_TJA11XX_PHY
> + tristate "NXP C45 TJA11XX PHYs"
> + help
> + Enable support for NXP C45 TJA11XX PHYs.
> + Currently supports only the TJA1103 PHY.
> +#define PHY_ID_BASE_T1 0x001BB010
It would be better to use PHY_ID_TJA_1103 here.
> +
> +#define PMAPMD_B100T1_PMAPMD_CTL 0x0834
> +#define B100T1_PMAPMD_CONFIG_EN BIT(15)
> +#define B100T1_PMAPMD_MASTER BIT(14)
> +#define MASTER_MODE (B100T1_PMAPMD_CONFIG_EN | \
> + B100T1_PMAPMD_MASTER)
You would normally align this with the B100T1_PMAPMD_CONFIG_EN
> +static int nxp_c45_reset_done(struct phy_device *phydev)
> +{
> + return !(phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_DEVICE_CONTROL) &
> + DEVICE_CONTROL_RESET);
> +}
> +
> +static int nxp_c45_reset_done_or_timeout(struct phy_device *phydev,
> + ktime_t timeout)
> +{
> + ktime_t cur = ktime_get();
> +
> + return nxp_c45_reset_done(phydev) || ktime_after(cur, timeout);
> +}
> +
> +static int nxp_c45_soft_reset(struct phy_device *phydev)
> +{
> + ktime_t timeout;
> + int ret;
> +
> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_DEVICE_CONTROL,
> + DEVICE_CONTROL_RESET);
> + if (ret)
> + return ret;
> +
> + timeout = ktime_add_ns(ktime_get(), RESET_POLL_NS);
> + spin_until_cond(nxp_c45_reset_done_or_timeout(phydev, timeout));
phy_read_mmd_poll_timeout() i think does what you need.
> + if (!nxp_c45_reset_done(phydev)) {
> + phydev_err(phydev, "reset fail\n");
> + return -EIO;
> + }
> + return 0;
> +}
> +static struct phy_driver nxp_c45_driver[] = {
> + {
> + PHY_ID_MATCH_MODEL(PHY_ID_BASE_T1),
> + .name = "NXP C45 BASE-T1",
"NXP C45 TJA1103"
Andrew
Re: [PATCH net-next 2/7] net: korina: Use devres functions
> + if (!p) {
> printk(KERN_ERR DRV_NAME ": cannot remap registers\n");
> - rc = -ENXIO;
> - goto probe_err_out;
> + return -ENOMEM;
> }
Hi Thomas
Another possible cleanup would be replacing printk(KERN_ERR with
dev_err(), or netdev_err() etc.
Andrew
Re: [PATCH net-next 1/7] net: korina: Fix MDIO functions
> +static int korina_mdio_wait(struct korina_private *lp)
> +{
> + int timeout = 1000;
> +
> + while ((readl(&lp->eth_regs->miimind) & 1) && timeout-- > 0)
> + udelay(1);
> +
> + if (timeout <= 0)
> + return -1;
> +
> + return 0;
Using readl_poll_timeout_atomic() would be better.
> +}
> +
> +static int korina_mdio_read(struct net_device *dev, int phy, int reg)
> {
> struct korina_private *lp = netdev_priv(dev);
> int ret;
>
> - mii_id = ((lp->rx_irq == 0x2c ? 1 : 0) << 8);
> + if (korina_mdio_wait(lp))
> + return -1;
This should really be -ETIMEDOUT
> dev->watchdog_timeo = TX_TIMEOUT;
> netif_napi_add(dev, &lp->napi, korina_poll, NAPI_POLL_WEIGHT);
>
> - lp->phy_addr = (((lp->rx_irq == 0x2c? 1:0) << 8) | 0x05);
> lp->mii_if.dev = dev;
> - lp->mii_if.mdio_read = mdio_read;
> - lp->mii_if.mdio_write = mdio_write;
> - lp->mii_if.phy_id = lp->phy_addr;
> + lp->mii_if.mdio_read = korina_mdio_read;
> + lp->mii_if.mdio_write = korina_mdio_write;
> + lp->mii_if.phy_id = 1;
> lp->mii_if.phy_id_mask = 0x1f;
> lp->mii_if.reg_num_mask = 0x1f;
You could also replace all the mii code with phylib.
Andrew
Re: [PATCH net-next 1/3] dt-bindings: net: add nvmem-mac-address-offset property
On Wed, Apr 14, 2021 at 05:26:55PM +0200, Michael Walle wrote: > It is already possible to read the MAC address via a NVMEM provider. But > there are boards, esp. with many ports, which only have a base MAC > address stored. Thus we need to have a way to provide an offset per > network device. We need to see what Rob thinks of this. There was recently a patchset to support swapping the byte order of the MAC address in a NVMEM. Rob said the NVMEM provider should have the property, not the MAC driver. This does seems more ethernet specific, so maybe it should be an Ethernet property? Andrew
Re: [PATCH] ARM: dts: imx6dl-yapp4: Fix RGMII connection to QCA8334 switch
On Tue, Apr 13, 2021 at 04:45:57PM +0200, Michal Vokáč wrote:
> The FEC does not have a PHY so it should not have a phy-handle. It is
> connected to the switch at RGMII level so we need a fixed-link sub-node
> on both ends.
>
> This was not a problem until the qca8k.c driver was converted to PHYLINK
> by commit b3591c2a3661 ("net: dsa: qca8k: Switch to PHYLINK instead of
> PHYLIB"). That commit revealed the FEC configuration was not correct.
>
> Fixes: 87489ec3a77f ("ARM: dts: imx: Add Y Soft IOTA Draco, Hydra and Ursa
> boards")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Michal Vokáč
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH] phy: nxp-c45: add driver for tja1103
> nxp-c45-tja11xx is acceptable from my point of view. Great. Enough bike shedding, nxp-c45-tja11xx it is. Andrew
Re: [PATCH] phy: nxp-c45: add driver for tja1103
> Ok, we can agree that there will not be a perfect naming. Would it be a > possibility to rename the existing TJA11xx driver to TJA1100-1-2 or is that > unwanted? It is generally a bad idea. It makes back porting fixing harder if the file changes name. > If nxp-c45.c is to generic (I take from your comments that' your > conclusion), we could at least lean towards nxp-c45-bt1.c? Unfortunately, > the product naming schemes are not sufficiently methodical to have a a good > driver name based on product names. And what does bt1 stand for? How about nxp-c45-tja11xx.c. It is not ideal, but it does at least give an indication of what devices it does cover, even if there is a big overlap with nxp-tja11xx.c, in terms of pattern matching. And if you do decide to have a major change of registers, your can call the device tja1201 and have a new driver nxp-c45-tja12xx. Andrew
Re: Re: [PATCH] phy: nxp-c45: add driver for tja1103
On Tue, Apr 13, 2021 at 08:56:30AM +0200, Christian Herber wrote: > Hi Andrew, > > On 4/12/2021 6:52 PM, Andrew Lunn wrote: > > > > So what you are say is, you don't care if the IP is completely > > different, it all goes in one driver. So lets put this driver into > > nxp-tja11xx.c. And then we avoid all the naming issues. > > > > Andrew > > > > As this seems to be a key question, let me try and shed some more light on > this. > The original series of BASE-T1 PHYs includes TJA110, TJA1101, and TJA1102. > They are covered by the existing driver, which has the unfortunate naming > TJA11xx. Unfortunate, because the use of wildcards is a bit to generous. Yes, that does happen. Naming is difficult. But i really think nxp-c45.c is much worse. It gives no idea at all what it supports. Or in the future, what it does not support, and you actually need nxp-c45-ng.c. Developers are going to look at a board, see a tja1XYZ chip, see the nxp-tja11xx.c and enable it. Does the chip have a big C45 symbol on it? Anything to give the idea it should use the nxp-c45 driver? Maybe we should actually swing the complete opposite direction. Name it npx-tja1103.c. There are lots of drivers which have a specific name, but actually support a lot more devices. The developer sees they have an tja1XYZ, there are two drivers which look about right, and enable them both? Andrew
Re: [PATCH net-next 1/2] net: phy: marvell-88x2222: check that link is operational
> Indeed - it should be a logical and operation - there is light present > _and_ the PHY recognises the signal. This is what the commit achieves, > although (iirc) doesn't cater for the case where there is no SFP cage > attached. Hi Russell Is there something like this in the marvell10 driver? Also, do you know when there is an SFP cage? Do we need a standardised DT property for this? Andrew
Re: [PATCH net-next 1/2] net: phy: marvell-88x2222: check that link is operational
On Tue, Apr 13, 2021 at 10:13:49AM +0300, Ivan Bornyakov wrote:
> On Tue, Apr 13, 2021 at 01:32:12AM +0200, Marek Behún wrote:
> > On Mon, 12 Apr 2021 15:16:59 +0300
> > Ivan Bornyakov wrote:
> >
> > > Some SFP modules uses RX_LOS for link indication. In such cases link
> > > will be always up, even without cable connected. RX_LOS changes will
> > > trigger link_up()/link_down() upstream operations. Thus, check that SFP
> > > link is operational before actual read link status.
> > >
> > > Signed-off-by: Ivan Bornyakov
> > > ---
> > > drivers/net/phy/marvell-88x.c | 26 ++
> > > 1 file changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/net/phy/marvell-88x.c
> > > b/drivers/net/phy/marvell-88x.c
> > > index eca8c2f20684..fb285ac741b2 100644
> > > --- a/drivers/net/phy/marvell-88x.c
> > > +++ b/drivers/net/phy/marvell-88x.c
> > > @@ -51,6 +51,7 @@
> > > struct mv_data {
> > > phy_interface_t line_interface;
> > > __ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> > > + bool sfp_link;
> > > };
> > >
> > > /* SFI PMA transmit enable */
> > > @@ -148,6 +149,9 @@ static int mv_read_status(struct phy_device
> > > *phydev)
> > > phydev->speed = SPEED_UNKNOWN;
> > > phydev->duplex = DUPLEX_UNKNOWN;
> > >
> > > + if (!priv->sfp_link)
> > > + return 0;
> > > +
> >
> > So if SFP is not used at all, if this PHY is used in a different
> > usecase, this function will always return 0? Is this correct?
> >
>
> Yes, probably. The thing is that I only have hardware with SFP cages, so
> I realy don't know if this driver work in other usecases.
It is O.K, to say you don't know if this will work for other setups,
but it is different thing to do something which could potentially
break those other setup. Somebody trying to use this without an SFP is
going to have a bad experience because of this change. And then they
are going to have to try to fix this, potentially breaking your setup.
if you truly need this, make it conditional on that you know you have
an SFP cage connected.
> > > +static void mv_sfp_link_down(void *upstream)
> > > +{
> > > + struct phy_device *phydev = upstream;
> > > + struct mv_data *priv;
> > > +
> > > + priv = (struct mv_data *)phydev->priv;
> >
> > This cast is redundant since the phydev->priv is (void*). You can cast
> > (void*) to (struct ... *).
> >
> > You can also just use
> > struct mv_data *priv = phydev->priv;
> >
>
> Yeah, I know, but reverse XMAS tree wouldn't line up.
Please move the assignment into the body of the function.
Andrew
Re: [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
> I guess this is depends whether the most usual case is to have all > these interrupts being actively in use or not. Most interrupts only > use a limited portion of their interrupt space at any given time. > Allocating all interrupts and creating mappings upfront is a waste of > memory. > > If the use case here is that all these interrupts will be wired and > used in most cases, then upfront allocation is probably not a problem. Hi Marc The interrupts are generally used. Since this is an Ethernet switch, generally the port is administratively up, even if there is no cable plugged in. Once/if a cable is plugged in and there is a link peer, the PHY will interrupt to indicate this. The only real case i can think of when the interrupts are not used is when the switch has more ports than connected to the front panel. This can happen in industrial settings, but not SOHO. Those ports which don't go anywhere are never configured up and so the interrupt is never used. Andrew
Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices
On Mon, Apr 12, 2021 at 07:47:18PM +0200, Michael Walle wrote: > of_get_mac_address() already supports fetching the MAC address by an > nvmem provider. But until now, it was just working for platform devices. > Esp. it was not working for DSA ports and PCI devices. It gets more > common that PCI devices have a device tree binding since SoCs contain > integrated root complexes. > > Use the nvmem of_* binding to fetch the nvmem cells by a struct > device_node. We still have to try to read the cell by device first > because there might be a nvmem_cell_lookup associated with that device. > > Signed-off-by: Michael Walle Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next v4 1/2] of: net: pass the dst buffer to of_get_mac_address()
On Mon, Apr 12, 2021 at 07:47:17PM +0200, Michael Walle wrote:
> of_get_mac_address() returns a "const void*" pointer to a MAC address.
> Lately, support to fetch the MAC address by an NVMEM provider was added.
> But this will only work with platform devices. It will not work with
> PCI devices (e.g. of an integrated root complex) and esp. not with DSA
> ports.
>
> There is an of_* variant of the nvmem binding which works without
> devices. The returned data of a nvmem_cell_read() has to be freed after
> use. On the other hand the return of_get_mac_address() points to some
> static data without a lifetime. The trick for now, was to allocate a
> device resource managed buffer which is then returned. This will only
> work if we have an actual device.
>
> Change it, so that the caller of of_get_mac_address() has to supply a
> buffer where the MAC address is written to. Unfortunately, this will
> touch all drivers which use the of_get_mac_address().
>
> Usually the code looks like:
>
> const char *addr;
> addr = of_get_mac_address(np);
> if (!IS_ERR(addr))
> ether_addr_copy(ndev->dev_addr, addr);
>
> This can then be simply rewritten as:
>
> of_get_mac_address(np, ndev->dev_addr);
>
> Sometimes is_valid_ether_addr() is used to test the MAC address.
> of_get_mac_address() already makes sure, it just returns a valid MAC
> address. Thus we can just test its return code. But we have to be
> careful if there are still other sources for the MAC address before the
> of_get_mac_address(). In this case we have to keep the
> is_valid_ether_addr() call.
>
> The following coccinelle patch was used to convert common cases to the
> new style. Afterwards, I've manually gone over the drivers and fixed the
> return code variable: either used a new one or if one was already
> available use that. Mansour Moufid, thanks for that coccinelle patch!
>
>
> @a@
> identifier x;
> expression y, z;
> @@
> - x = of_get_mac_address(y);
> + x = of_get_mac_address(y, z);
> <...
> - ether_addr_copy(z, x);
> ...>
>
> @@
> identifier a.x;
> @@
> - if (<+... x ...+>) {}
>
> @@
> identifier a.x;
> @@
> if (<+... x ...+>) {
> ...
> }
> - else {}
>
> @@
> identifier a.x;
> expression e;
> @@
> - if (<+... x ...+>@e)
> - {}
> - else
> + if (!(e))
> {...}
>
> @@
> expression x, y, z;
> @@
> - x = of_get_mac_address(y, z);
> + of_get_mac_address(y, z);
> ... when != x
>
>
> All drivers, except drivers/net/ethernet/aeroflex/greth.c, were
> compile-time tested.
>
> Suggested-by: Andrew Lunn
> Signed-off-by: Michael Walle
I cannot say i looked at all the changes, but the ones i did exam
seemed O.K.
Reviewed-by: Andrew Lunn
Andrew
Re: [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
> > > +static void
> > > +mt7530_setup_mdio_irq(struct mt7530_priv *priv)
> > > +{
> > > + struct dsa_switch *ds = priv->ds;
> > > + int p;
> > > +
> > > + for (p = 0; p < MT7530_NUM_PHYS; p++) {
> > > + if (BIT(p) & ds->phys_mii_mask) {
> > > + unsigned int irq;
> > > +
> > > + irq = irq_create_mapping(priv->irq_domain, p);
> >
> > This seems odd. Why aren't the MDIO IRQs allocated on demand as
> > endpoint attached to this interrupt controller are being probed
> > individually? In general, doing this allocation upfront is an
> > indication that there is some missing information in the DT to perform
> > the discovery.
>
> This is what Andrew's mv88e6xxx does, actually. In addition, I also check
> the phys_mii_mask to avoid creating mappings for unused ports.
It can be done via DT, using the standard interrupt property, so long
as you use of_mdiobus_register(np).
But when you have an 7 port switch, and a nice simple mapping, port 0
PHY using interrupt 0, you can save a lot of device tree boilerplate
by doing it in code. And when you have 4 of these switches, it gets
very boring adding all the DT to just wire up the interrupts 28
interrupts.
> Andrew, perhaps this can be done in DSA core?
Not easily. It is not always a simple mapping like this. Two of the
switches supported by mv88exxx offset the PHYs by 0x10. You really
need the switch driver involved, with its detailed knowledge of the
hardware.
Andrew
Re: [PATCH net-next] net: dsa: lantiq_gswip: Add support for dumping the registers
On Tue, Apr 13, 2021 at 12:24:49AM +0200, Martin Blumenstingl wrote: > Hi Andrew, > > On Mon, Apr 12, 2021 at 1:16 AM Andrew Lunn wrote: > > > > On Sun, Apr 11, 2021 at 10:55:11PM +0200, Martin Blumenstingl wrote: > > > Add support for .get_regs_len and .get_regs so it is easier to find out > > > about the state of the ports on the GSWIP hardware. For this we > > > specifically add the GSWIP_MAC_PSTATp(port) and GSWIP_MDIO_STATp(port) > > > register #defines as these contain the current port status (as well as > > > the result of the auto polling mechanism). Other global and per-port > > > registers which are also considered useful are included as well. > > > > Although this is O.K, there has been a trend towards using devlink > > regions for this, and other register sets in the switch. Take a look > > at drivers/net/dsa/mv88e6xxx/devlink.c. > > > > There is a userspace tool for the mv88e6xxx devlink regions here: > > > > https://github.com/lunn/mv88e6xxx_dump > > > > and a few people have forked it and modified it for other DSA > > switches. At some point we might want to try to merge the forks back > > together so we have one tool to dump any switch. > actually I was wondering if there is some way to make the registers > "easier to read" in userspace. You can add decoding to ethtool. The marvell chips have this, to some extent. But the ethtool API is limited to just port registers, and there can be a lot more registers which are not associated to a port. devlink gives you access to these additional registers. Andrew
Re: [PATCH net-next 1/2] net: phy: marvell-88x2222: check that link is operational
On Mon, Apr 12, 2021 at 03:16:59PM +0300, Ivan Bornyakov wrote: > Some SFP modules uses RX_LOS for link indication. In such cases link > will be always up, even without cable connected. RX_LOS changes will > trigger link_up()/link_down() upstream operations. Thus, check that SFP > link is operational before actual read link status. Sorry, but this is not making much sense to me. LOS just indicates some sort of light is coming into the device. You have no idea what sort of light. The transceiver might be able to decode that light and get sync, it might not. It is important that mv_read_status() returns the line side status. Has it been able to achieve sync? That should be independent of LOS. Or are you saying the transceiver is reporting sync, despite no light coming in? Andrew
Re: [PATCH] phy: nxp-c45: add driver for tja1103
> +static const struct nxp_c45_phy_stats nxp_c45_hw_stats[] = {
> + { "phy_symbol_error_cnt", MDIO_MMD_VEND1, SYMBOL_ERROR_COUNTER, 0,
> GENMASK(15, 0) },
> + { "phy_link_status_drop_cnt", MDIO_MMD_VEND1, LINK_DROP_COUNTER, 8,
> GENMASK(13, 8) },
> + { "phy_link_availability_drop_cnt", MDIO_MMD_VEND1, LINK_DROP_COUNTER,
> 0, GENMASK(5, 0) },
netdev tries to keep with the old 80 character limit. Please wrap the
long lines.
> +static void nxp_c45_set_delays(struct phy_device *phydev)
> +{
> + struct nxp_c45_phy *priv = phydev->priv;
> + u64 tx_delay = priv->tx_delay;
> + u64 rx_delay = priv->rx_delay;
> + u64 degree;
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> + degree = tx_delay / PS_PER_DEGREE;
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, VND1_TXID,
> + ID_ENABLE | nxp_c45_get_phase_shift(degree));
> + }
You are missing an else clause. You need to ensure the delay is 0 if
delays are not required. You have no idea what the bootloader has
done.
> +static int nxp_c45_get_delays(struct phy_device *phydev)
> +{
> + struct nxp_c45_phy *priv = phydev->priv;
> + int ret;
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> + ret = device_property_read_u32(&phydev->mdio.dev,
> "tx-internal-delay-ps",
> +&priv->tx_delay);
> + if (ret) {
> + phydev_err(phydev, "tx-internal-delay-ps property
> missing\n");
This is not normally mandatory. Default to 2ns if not specified in DT.
> +static int nxp_c45_set_phy_mode(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, ABILITIES);
> + phydev_dbg(phydev, "Clause 45 managed PHY abilities 0x%x\n", ret);
> +
> + switch (phydev->interface) {
> + case PHY_INTERFACE_MODE_RGMII:
> + if (!(ret & RGMII_ABILITY)) {
> + phydev_err(phydev, "rgmii mode not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG,
> MII_BASIC_CONFIG_RGMII);
> + break;
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + if (!(ret & RGMII_ID_ABILITY)) {
> + phydev_err(phydev, "rgmii-id, rgmii-txid, rgmii-rxid
> modes are not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG,
> MII_BASIC_CONFIG_RGMII);
> + ret = nxp_c45_get_delays(phydev);
> + if (ret)
> + return ret;
> +
> + nxp_c45_set_delays(phydev);
> + break;
Again, for PHY_INTERFACE_MODE_RGMII you need to ensure the hardware is
not inserting a delay.
> + case PHY_INTERFACE_MODE_SGMII:
> + if (!(ret & SGMII_ABILITY)) {
> + phydev_err(phydev, "sgmii mode not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG,
> MII_BASIC_CONFIG_SGMII);
> + break;
Interested. What gets reported over the inband signalling?
Andrew
Re: [PATCH v2] net: phy: marvell: fix detection of PHY on Topaz switches
On Mon, Apr 12, 2021 at 06:57:39PM +0200, Pali Rohár wrote:
> Since commit fee2d546414d ("net: phy: marvell: mv88e6390 temperature
> sensor reading"), Linux reports the temperature of Topaz hwmon as
> constant -75°C.
>
> This is because switches from the Topaz family (88E6141 / 88E6341) have
> the address of the temperature sensor register different from Peridot.
>
> This address is instead compatible with 88E1510 PHYs, as was used for
> Topaz before the above mentioned commit.
>
> Create a new mapping table between switch family and PHY ID for families
> which don't have a model number. And define PHY IDs for Topaz and Peridot
> families.
>
> Create a new PHY ID and a new PHY driver for Topaz's internal PHY.
> The only difference from Peridot's PHY driver is the HWMON probing
> method.
>
> Prior this change Topaz's internal PHY is detected by kernel as:
>
> PHY [...] driver [Marvell 88E6390] (irq=63)
>
> And afterwards as:
>
> PHY [...] driver [Marvell 88E6341 Family] (irq=63)
>
> Signed-off-by: Pali Rohár
> BugLink: https://github.com/globalscaletechnologies/linux/issues/1
> Fixes: fee2d546414d ("net: phy: marvell: mv88e6390 temperature sensor
> reading")
> Reviewed-by: Marek Behún
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH] phy: nxp-c45: add driver for tja1103
On Mon, Apr 12, 2021 at 05:49:04PM +0300, Radu Nicolae Pirea (NXP OSS) wrote: > On Mon, 2021-04-12 at 16:23 +0200, Andrew Lunn wrote: > > > It is purely a C45 device. > > > > > Even if the PHY will be based on the same IP or not, if it is a C45 > > > PHY, it will be supported by this driver. We are not talking about > > > 2 or > > > 3 PHYs. This driver will support all future C45 PHYs. That's why we > > > named it "NXP C45". > > > > So if in future you produce C45 multi-gige PHYs, which have nothing > > in > > common with the T1 automative PHY, it will still be in this driver? > Yes. C45 is robust and, if the PHY interface is standard, you can > support Base-T, Base-T1, and so on in the same register interface. So what you are say is, you don't care if the IP is completely different, it all goes in one driver. So lets put this driver into nxp-tja11xx.c. And then we avoid all the naming issues. Andrew
Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches
On Mon, Apr 12, 2021 at 05:52:39PM +0200, Pali Rohár wrote:
> On Monday 12 April 2021 17:32:33 Andrew Lunn wrote:
> > > Anyway, now I'm looking at phy/marvell.c driver again and it supports
> > > only 88E6341 and 88E6390 families from whole 88E63xxx range.
> > >
> > > So do we need to define for now table for more than
> > > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries?
> >
> > Probably not. I've no idea if the 6393 has an ID, so to be safe you
> > should add that. Assuming it has a family of its own.
>
> So what about just?
>
> if (reg == MII_PHYSID2 && !(val & 0x3f0)) {
> if (chip->info->family == MV88E6XXX_FAMILY_6341)
> val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6341 >> 4;
> else if (chip->info->family == MV88E6XXX_FAMILY_6390)
> val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
> }
As i said, i expect the 6393 also has no ID. And i recently found out
Marvell have some automotive switches, 88Q5xxx which are actually
based around the same IP and could be added to this driver. They also
might not have an ID. I suspect this list is going to get longer, so
having it table driven will make that simpler, less error prone.
Andrew
Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches
> Anyway, now I'm looking at phy/marvell.c driver again and it supports > only 88E6341 and 88E6390 families from whole 88E63xxx range. > > So do we need to define for now table for more than > MV88E6XXX_FAMILY_6341 and MV88E6XXX_FAMILY_6390 entries? Probably not. I've no idea if the 6393 has an ID, so to be safe you should add that. Assuming it has a family of its own. Andrew
Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches
On Mon, Apr 12, 2021 at 03:34:47PM +0200, Pali Rohár wrote:
> On Monday 12 April 2021 15:15:07 Andrew Lunn wrote:
> > > +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family);
> > > +
> >
> > No forward declaration please. Move the code around. It is often best
> > to do that in a patch which just moves code, no other changes. It
> > makes it easier to review.
>
> Avoiding forward declaration would mean to move about half of source
> code. mv88e6xxx_physid_for_family depends on mv88e6xxx_table which
> depends on all _ops structures which depends on all lot of other
> functions.
So this is basically what you are trying to do:
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 903d619e08ed..ef4dbcb052b7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3026,6 +3026,18 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
return err;
}
+static const enum mv88e6xxx_model family_model_table[] = {
+ [MV88E6XXX_FAMILY_6095] = MV88E6XXX_PORT_SWITCH_ID_PROD_6095,
+ [MV88E6XXX_FAMILY_6097] = MV88E6XXX_PORT_SWITCH_ID_PROD_6097,
+ [MV88E6XXX_FAMILY_6185] = MV88E6XXX_PORT_SWITCH_ID_PROD_6185,
+ [MV88E6XXX_FAMILY_6250] = MV88E6XXX_PORT_SWITCH_ID_PROD_6250,
+ [MV88E6XXX_FAMILY_6320] = MV88E6XXX_PORT_SWITCH_ID_PROD_6320,
+ [MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
+ [MV88E6XXX_FAMILY_6351] = MV88E6XXX_PORT_SWITCH_ID_PROD_6351,
+ [MV88E6XXX_FAMILY_6352] = MV88E6XXX_PORT_SWITCH_ID_PROD_6352,
+ [MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
+};
+
static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
{
struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
@@ -3056,7 +3068,7 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus, int
phy, int reg)
* a PHY,
*/
if (!(val & 0x3f0))
- val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
+ val |= family_model_table[chip->info->family]
>> 4;
}
and it compiles. No forward declarations needed. It is missing all the
error checking etc, but i don't see why that should change the
dependencies.
Andrew
Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches
> > > +/* This table contains representative model for every family */
> > > +static const enum mv88e6xxx_model family_model_table[] = {
> > > + [MV88E6XXX_FAMILY_6095] = MV88E6095,
> > > + [MV88E6XXX_FAMILY_6097] = MV88E6097,
> > > + [MV88E6XXX_FAMILY_6185] = MV88E6185,
> > > + [MV88E6XXX_FAMILY_6250] = MV88E6250,
> > > + [MV88E6XXX_FAMILY_6320] = MV88E6320,
> > > + [MV88E6XXX_FAMILY_6341] = MV88E6341,
> > > + [MV88E6XXX_FAMILY_6351] = MV88E6351,
> > > + [MV88E6XXX_FAMILY_6352] = MV88E6352,
> > > + [MV88E6XXX_FAMILY_6390] = MV88E6390,
> > > +};
> >
> > This table is wrong. MV88E6390 does not equal
> > MV88E6XXX_PORT_SWITCH_ID_PROD_6390. MV88E6XXX_PORT_SWITCH_ID_PROD_6390
> > was chosen because it is already an MDIO device ID, in register 2 and
> > 3. It probably will never clash with a real Marvell PHY ID. MV88E6390
> > is just a small integer, and there is a danger it will clash with a
> > real PHY.
>
> So... how to solve this issue? What should be in the mapping table?
You need to use MV88E6XXX_PORT_SWITCH_ID_PROD_6095,
MV88E6XXX_PORT_SWITCH_ID_PROD_6097,
...
MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
> > You cannot just replace the MARVELL_PHY_ID_88E6390. That will break
> > the 6390! You need to add the new PHY for the 88E6341.
>
> I have not replaced anything.
Yes, sorry. I read the diff wrong.
Andrew
Re: [PATCH] phy: nxp-c45: add driver for tja1103
> It is purely a C45 device. > Even if the PHY will be based on the same IP or not, if it is a C45 > PHY, it will be supported by this driver. We are not talking about 2 or > 3 PHYs. This driver will support all future C45 PHYs. That's why we > named it "NXP C45". So if in future you produce C45 multi-gige PHYs, which have nothing in common with the T1 automative PHY, it will still be in this driver? Andrew
Re: [PATCH] net: phy: marvell: fix detection of PHY on Topaz switches
> +static u16 mv88e6xxx_physid_for_family(enum mv88e6xxx_family family);
> +
No forward declaration please. Move the code around. It is often best
to do that in a patch which just moves code, no other changes. It
makes it easier to review.
> static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
> {
> struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
> @@ -3040,24 +3042,9 @@ static int mv88e6xxx_mdio_read(struct mii_bus *bus,
> int phy, int reg)
> err = chip->info->ops->phy_read(chip, bus, phy, reg, &val);
> mv88e6xxx_reg_unlock(chip);
>
> - if (reg == MII_PHYSID2) {
> - /* Some internal PHYs don't have a model number. */
> - if (chip->info->family != MV88E6XXX_FAMILY_6165)
> - /* Then there is the 6165 family. It gets is
> - * PHYs correct. But it can also have two
> - * SERDES interfaces in the PHY address
> - * space. And these don't have a model
> - * number. But they are not PHYs, so we don't
> - * want to give them something a PHY driver
> - * will recognise.
> - *
> - * Use the mv88e6390 family model number
> - * instead, for anything which really could be
> - * a PHY,
> - */
> - if (!(val & 0x3f0))
> - val |= MV88E6XXX_PORT_SWITCH_ID_PROD_6390 >> 4;
> - }
> + /* Some internal PHYs don't have a model number. */
> + if (reg == MII_PHYSID2 && !(val & 0x3f0))
> + val |= mv88e6xxx_physid_for_family(chip->info->family);
>
> return err ? err : val;
> }
> @@ -5244,6 +5231,39 @@ static const struct mv88e6xxx_info
> *mv88e6xxx_lookup_info(unsigned int prod_num)
> return NULL;
> }
>
> +/* This table contains representative model for every family */
> +static const enum mv88e6xxx_model family_model_table[] = {
> + [MV88E6XXX_FAMILY_6095] = MV88E6095,
> + [MV88E6XXX_FAMILY_6097] = MV88E6097,
> + [MV88E6XXX_FAMILY_6185] = MV88E6185,
> + [MV88E6XXX_FAMILY_6250] = MV88E6250,
> + [MV88E6XXX_FAMILY_6320] = MV88E6320,
> + [MV88E6XXX_FAMILY_6341] = MV88E6341,
> + [MV88E6XXX_FAMILY_6351] = MV88E6351,
> + [MV88E6XXX_FAMILY_6352] = MV88E6352,
> + [MV88E6XXX_FAMILY_6390] = MV88E6390,
> +};
This table is wrong. MV88E6390 does not equal
MV88E6XXX_PORT_SWITCH_ID_PROD_6390. MV88E6XXX_PORT_SWITCH_ID_PROD_6390
was chosen because it is already an MDIO device ID, in register 2 and
3. It probably will never clash with a real Marvell PHY ID. MV88E6390
is just a small integer, and there is a danger it will clash with a
real PHY.
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -3021,9 +3021,34 @@ static struct phy_driver marvell_drivers[] = {
> .get_stats = marvell_get_stats,
> },
> {
> - .phy_id = MARVELL_PHY_ID_88E6390,
> + .phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
> .phy_id_mask = MARVELL_PHY_ID_MASK,
> - .name = "Marvell 88E6390",
> + .name = "Marvell 88E6341 Family",
You cannot just replace the MARVELL_PHY_ID_88E6390. That will break
the 6390! You need to add the new PHY for the 88E6341.
Andrew
Re: [PATCH] phy: nxp-c45: add driver for tja1103
On Mon, Apr 12, 2021 at 01:02:07PM +0300, Radu Nicolae Pirea (NXP OSS) wrote: > On Fri, 2021-04-09 at 21:36 +0200, Andrew Lunn wrote: > > On Fri, Apr 09, 2021 at 09:41:06PM +0300, Radu Pirea (NXP OSS) wrote: > > > Add driver for tja1103 driver and for future NXP C45 PHYs. > > > > So apart from c45 vs c22, how does this differ to nxp-tja11xx.c? > > Do we really want two different drivers for the same hardware? > > Can we combine them somehow? > It looks like the PHYs are the same hardware, but that's not entirely > true. Just the naming is the same. TJA1103 is using a different IP and > is having timestamping support(I will add it later). Is the IP very different? You often see different generations of a PHY supported by the same driver, if the generations are similar. Does it support C22 or it is purely a C45 device? > TJA is also not an Ethernet PHY series, but a general prefix for media > interfaces including also CAN, LIN, etc. > > > > > +config NXP_C45_PHY > > > + tristate "NXP C45 PHYs" > > > > This is also very vague. So in the future it will support PHYs other > > than the TJA series? > Yes, in the future this driver will support other PHYs too. Based on the same IP? Or different IP? Are we talking about 2 more PHYs, so like the nxp-tja11xx.c will support 3 PHYs. And then the tja1106 will come along and need a new driver? What will you call that? I just don't like 'NXP C45 PHYs", it gives no clue as to what it actually supports, and it gives you problems when you need to add yet another driver. At minimum, there needs to be a patch to add tja1102 to the help for the nxp-tja11xx.c driver. And this driver needs to list tja1103. Andrew
Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
> +static void mana_gd_deregiser_irq(struct gdma_queue *queue)
> +{
> + struct gdma_dev *gd = queue->gdma_dev;
> + struct gdma_irq_context *gic;
> + struct gdma_context *gc;
> + struct gdma_resource *r;
> + unsigned int msix_index;
> + unsigned long flags;
> +
> + /* At most num_online_cpus() + 1 interrupts are used. */
> + msix_index = queue->eq.msix_index;
> + if (WARN_ON(msix_index > num_online_cpus()))
> + return;
Do you handle hot{un}plug of CPUs?
> +static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
> + struct gdma_event *event)
> +{
> + struct hw_channel_context *hwc = ctx;
> + struct gdma_dev *gd = hwc->gdma_dev;
> + union hwc_init_type_data type_data;
> + union hwc_init_eq_id_db eq_db;
> + u32 type, val;
> +
> + switch (event->type) {
> + case GDMA_EQE_HWC_INIT_EQ_ID_DB:
> + eq_db.as_uint32 = event->details[0];
> + hwc->cq->gdma_eq->id = eq_db.eq_id;
> + gd->doorbell = eq_db.doorbell;
> + break;
> +
> + case GDMA_EQE_HWC_INIT_DATA:
> +
> + type_data.as_uint32 = event->details[0];
> +
> + case GDMA_EQE_HWC_INIT_DONE:
> + complete(&hwc->hwc_init_eqe_comp);
> + break;
...
> + default:
> + WARN_ON(1);
> + break;
> + }
Are these events from the firmware? If you have newer firmware with an
older driver, are you going to spam the kernel log with WARN_ON dumps?
> +static int mana_move_wq_tail(struct gdma_queue *wq, u32 num_units)
> +{
> + u32 used_space_old;
> + u32 used_space_new;
> +
> + used_space_old = wq->head - wq->tail;
> + used_space_new = wq->head - (wq->tail + num_units);
> +
> + if (used_space_new > used_space_old) {
> + WARN_ON(1);
> + return -ERANGE;
> + }
You could replace the 1 by the condition. There are a couple of these.
Andrew
Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
> +static inline bool is_gdma_msg(const void *req)
> +{
> + struct gdma_req_hdr *hdr = (struct gdma_req_hdr *)req;
> +
> + if (hdr->req.hdr_type == GDMA_STANDARD_HEADER_TYPE &&
> + hdr->resp.hdr_type == GDMA_STANDARD_HEADER_TYPE &&
> + hdr->req.msg_size >= sizeof(struct gdma_req_hdr) &&
> + hdr->resp.msg_size >= sizeof(struct gdma_resp_hdr) &&
> + hdr->req.msg_type != 0 && hdr->resp.msg_type != 0)
> + return true;
> +
> + return false;
> +}
> +
> +static inline bool is_gdma_msg_len(const u32 req_len, const u32 resp_len,
> +const void *req)
> +{
> + struct gdma_req_hdr *hdr = (struct gdma_req_hdr *)req;
> +
> + if (req_len >= sizeof(struct gdma_req_hdr) &&
> + resp_len >= sizeof(struct gdma_resp_hdr) &&
> + req_len >= hdr->req.msg_size && resp_len >= hdr->resp.msg_size &&
> + is_gdma_msg(req)) {
> + return true;
> + }
> +
> + return false;
> +}
You missed adding the mana_ prefix here. There might be others.
> +#define CQE_POLLING_BUFFER 512
> +struct ana_eq {
> + struct gdma_queue *eq;
> + struct gdma_comp cqe_poll[CQE_POLLING_BUFFER];
> +};
> +static int ana_poll(struct napi_struct *napi, int budget)
> +{
You also have a few cases of ana_, not mana_. There might be others.
Andrew
Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
> > Currently the protocol versin is 0.1.1 You may ask why it's called > > "drv version" rather than "protocol version" -- it's because the PF driver > > calls it that way, so I think here the VF driver may as well use the same > > name. BTW, the "drv ver" info is passed to the PF driver in the below > > function: > > Ohh, yes, the "driver version" is not the ideal name for that. > > I already looked on it in previous patch, came to the conclusion about > the protocol and forgot :(. Which suggests it needs renaming. Andrew
Re: [PATCH net-next] net: dsa: lantiq_gswip: Add support for dumping the registers
On Sun, Apr 11, 2021 at 10:55:11PM +0200, Martin Blumenstingl wrote: > Add support for .get_regs_len and .get_regs so it is easier to find out > about the state of the ports on the GSWIP hardware. For this we > specifically add the GSWIP_MAC_PSTATp(port) and GSWIP_MDIO_STATp(port) > register #defines as these contain the current port status (as well as > the result of the auto polling mechanism). Other global and per-port > registers which are also considered useful are included as well. Although this is O.K, there has been a trend towards using devlink regions for this, and other register sets in the switch. Take a look at drivers/net/dsa/mv88e6xxx/devlink.c. There is a userspace tool for the mv88e6xxx devlink regions here: https://github.com/lunn/mv88e6xxx_dump and a few people have forked it and modified it for other DSA switches. At some point we might want to try to merge the forks back together so we have one tool to dump any switch. Andrew
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Sun, Apr 11, 2021 at 08:01:35PM +0200, Marek Behun wrote: > On Sat, 10 Apr 2021 15:34:46 +0200 > Ansuel Smith wrote: > > > Hi, > > this is a respin of the Marek series in hope that this time we can > > finally make some progress with dsa supporting multi-cpu port. > > > > This implementation is similar to the Marek series but with some tweaks. > > This adds support for multiple-cpu port but leave the driver the > > decision of the type of logic to use about assigning a CPU port to the > > various port. The driver can also provide no preference and the CPU port > > is decided using a round-robin way. > > In the last couple of months I have been giving some thought to this > problem, and came up with one important thing: if there are multiple > upstream ports, it would make a lot of sense to dynamically reallocate > them to each user port, based on which user port is actually used, and > at what speed. > > For example on Turris Omnia we have 2 CPU ports and 5 user ports. All > ports support at most 1 Gbps. Round-robin would assign: > CPU port 0 - Port 0 > CPU port 1 - Port 1 > CPU port 0 - Port 2 > CPU port 1 - Port 3 > CPU port 0 - Port 4 > > Now suppose that the user plugs ethernet cables only into ports 0 and 2, > with 1, 3 and 4 free: > CPU port 0 - Port 0 (plugged) > CPU port 1 - Port 1 (free) > CPU port 0 - Port 2 (plugged) > CPU port 1 - Port 3 (free) > CPU port 0 - Port 4 (free) > > We end up in a situation where ports 0 and 2 share 1 Gbps bandwidth to > CPU, and the second CPU port is not used at all. > > A mechanism for automatic reassignment of CPU ports would be ideal here. One thing you need to watch out for here source MAC addresses. I've not looked at the details, so this is more a heads up, it needs to be thought about. DSA slaves get there MAC address from the master interface. For a single CPU port, all the slaves have the same MAC address. What happens when you have multiple CPU ports? Does the slave interface get the MAC address from its CPU port? What happens when a slave moves from one CPU interface to another CPU interface? Does its MAC address change. ARP is going to be unhappy for a while? Also, how is the switch deciding on which CPU port to use? Some switches are probably learning the MAC address being used by the interface and doing forwarding based on that. So you might need unique slave MAC addresses, and when a slave moves, it takes it MAC address with it, and you hope the switch learns about the move. But considered trapped frames as opposed to forwarded frames. So BPDU, IGMP, etc. Generally, you only have the choice to send such trapped frames to one CPU port. So potentially, such frames are going to ingress on the wrong port. Does this matter? What about multicast? How do you control what port that ingresses on? What about RX filters on the master interfaces? Could it be we added it to the wrong master? For this series to make progress, we need to know what has been tested, and if all the more complex functionality works, not just basic pings. Andrew
Re: [PATCH] phy: nxp-c45: add driver for tja1103
On Fri, Apr 09, 2021 at 09:41:06PM +0300, Radu Pirea (NXP OSS) wrote: > Add driver for tja1103 driver and for future NXP C45 PHYs. So apart from c45 vs c22, how does this differ to nxp-tja11xx.c? Do we really want two different drivers for the same hardware? Can we combine them somehow? > +config NXP_C45_PHY > + tristate "NXP C45 PHYs" This is also very vague. So in the future it will support PHYs other than the TJA series? Andrew
Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
For the structs containing variables with the same sizes, or already size aligned > variables, we knew the __packed has no effect. And for these structs, it > doesn't > cause performance impact either, correct? > > But in the future, if different sized variables are added, the __packed may > become necessary again. To prevent anyone accidently forget to add __packed > when adding new variables to these structs, can we keep the __packed for all > messages going through the "wire"? It should not be a problem because anybody adding new variables should know packed is not liked in the kernel and will take care. If you want to be paranoid add a BUILD_BUG_ON(size(struct foo) != 42); Andrew
Re: [PATCH net v1] lan743x: fix ethernet frame cutoff issue
Hi Sven > Many thanks to Heiner Kallweit for suggesting this solution. Adding a Suggested-by: would be good. And it might sometime help Johnathan Corbet extract some interesting statistics from the commit messages if everybody uses the same format. Andrew
Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
> Linux kernel doesn't do namespaces in the code, so every new driver needs > to worry about global symbols clashing This driver is called mana, yet the code uses ana. It would be good to resolve this inconsistency as well. Ideally, you want to prefix everything with ana_ or mana_, depending on what you choose, so we have a clean namespace. Andrew
Re: [RFC v3 net-next 0/4] MT7530 interrupt support
On Thu, Apr 08, 2021 at 11:00:08PM +0800, DENG Qingfang wrote: > Hi René, > > On Thu, Apr 8, 2021 at 10:02 PM René van Dorst wrote: > > > > Tested on Ubiquiti ER-X-SFP (MT7621) with 1 external phy which uses > > irq=POLL. > > > > I wonder if the external PHY's IRQ can be registered in the devicetree. > Change MT7530_NUM_PHYS to 6, and add the following to ER-X-SFP dts PHY node: I don't know this platform. What is the PHYs interrupt pin connected to? A SoC GPIO? There is a generic mechanism to describe PHY interrupts in DT. That should be used, if it is a GPIO. Andrew
Re: [PATCH v2 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
> > > diff --git a/drivers/net/ethernet/microsoft/Kconfig > > b/drivers/net/ethernet/microsoft/Kconfig > > > new file mode 100644 > > > index ..12ef6b581566 > > > --- /dev/null > > > +++ b/drivers/net/ethernet/microsoft/Kconfig > > > @@ -0,0 +1,30 @@ > > > +# > > > +# Microsoft Azure network device configuration > > > +# > > > + > > > +config NET_VENDOR_MICROSOFT > > > + bool "Microsoft Azure Network Device" > > > > Seems to me that should be generalized, more like: > > > > bool "Microsoft Network Devices" > This device is planned for Azure cloud at this time. > We will update the wording if things change. This section is about the Vendor. Broadcom, Marvell, natsemi, toshiba, etc. Microsoft is the Vendor here and all Microsoft Ethernet drivers belong here. It does not matter what platform they are for. Andrew
Re: [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling
> If dropping the modifications to gswip_phylink_mac_config is my only change: > do you want me to keep or drop your Reviewed-by in v2? You can keep it. Andrew
Re: [RFC v2 net-next 4/4] staging: mt7621-dts: enable MT7530 interrupt controller
On Wed, Apr 07, 2021 at 12:50:38PM +0800, DENG Qingfang wrote: > Enable MT7530 interrupt controller in the MT7621 SoC. > > Signed-off-by: DENG Qingfang Reviewed-by: Andrew Lunn Andrew
Re: [RFC v2 net-next 3/4] dt-bindings: net: dsa: add MT7530 interrupt controller binding
On Wed, Apr 07, 2021 at 12:50:37PM +0800, DENG Qingfang wrote: > Add device tree binding to support MT7530 interrupt controller. > > Signed-off-by: DENG Qingfang Reviewed-by: Andrew Lunn Andrew
Re: [RFC v2 net-next 2/4] net: dsa: mt7530: add interrupt support
On Wed, Apr 07, 2021 at 12:50:36PM +0800, DENG Qingfang wrote: > Add support for MT7530 interrupt controller to handle internal PHYs. > In order to assign an IRQ number to each PHY, the registration of MDIO bus > is also done in this driver. > > Signed-off-by: DENG Qingfang > --- > RFC v1 -> RFC v2: > - Split MDIO and IRQ setup function Thanks for the split. It looks good. > +/* Forward declaration */ > +struct mt7530_priv; > +static int mt753x_phy_read(struct mii_bus *, int, int); > +static int mt753x_phy_write(struct mii_bus *, int, int, u16); It is better to move the functions to before mt7530_setup_mdio(). Andrew
Re: [RFC v2 net-next 1/4] net: phy: add MediaTek PHY driver
On Wed, Apr 07, 2021 at 12:50:35PM +0800, DENG Qingfang wrote: > Add support for MediaTek PHYs found in MT7530 and MT7531 switches. > The initialization procedure is from the vendor driver, but due to lack > of documentation, the function of some register values remains unknown. > > Signed-off-by: DENG Qingfang Reviewed-by: Andrew Lunn Andrew
Re: [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling
> For my own curiosity: is there a "recommended" way where to configure > link up/down, speed, duplex and flow control? currently I have the > logic in both, .phylink_mac_config and .phylink_mac_link_up. You probably want to read the documentation in include/linux/phylink.h Andrew
Re: [PATCH net-next 1/1] net: stmmac: Add support for external trigger timestamping
On Wed, Apr 07, 2021 at 10:15:37PM +0800, Wong Vee Khee wrote:
> From: Tan Tee Min
>
> The Synopsis MAC controller supports auxiliary snapshot feature that
> allows user to store a snapshot of the system time based on an external
> event.
>
> This patch add supports to the above mentioned feature. Users will be
> able to triggered capturing the time snapshot from user-space using
> application such as testptp or any other applications that uses the
> PTP_EXTTS_REQUEST ioctl request.
You forgot to Cc: the PTP maintainer.
> @@ -159,6 +163,37 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
>priv->systime_flags);
> spin_unlock_irqrestore(&priv->ptp_lock, flags);
> break;
> + case PTP_CLK_REQ_EXTTS:
> + priv->plat->ext_snapshot_en = on;
> + mutex_lock(&priv->aux_ts_lock);
> + acr_value = readl(ptpaddr + PTP_ACR);
> + acr_value &= ~PTP_ACR_MASK;
> + if (on) {
> + /* Enable External snapshot trigger */
> + acr_value |= priv->plat->ext_snapshot_num;
> + acr_value |= PTP_ACR_ATSFC;
> + pr_info("Auxiliary Snapshot %d enabled.\n",
> + priv->plat->ext_snapshot_num >>
> + PTP_ACR_ATSEN_SHIFT);
dev_dbg()?
> + /* Enable Timestamp Interrupt */
> + intr_value = readl(ioaddr + GMAC_INT_EN);
> + intr_value |= GMAC_INT_TSIE;
> + writel(intr_value, ioaddr + GMAC_INT_EN);
> +
> + } else {
> + pr_info("Auxiliary Snapshot %d disabled.\n",
> + priv->plat->ext_snapshot_num >>
> + PTP_ACR_ATSEN_SHIFT);
dev_dbg()?
Do you really want to spam the kernel log with this?
Re: [PATCH net-next v2 0/2] Enable 2.5Gbps speed for stmmac
> Intel mgbe is flexible to pair with any PHY. Only Aquantia/Marvell > multi-gige PHY can do rate adaption right? The Marvell/Marvell multi-gige PHY can also do rate adaptation. Marvell buying Aquantia made naming messy :-( I should probably use part numbers. > Hence, we still need to take care of others PHYs. Yes, it just makes working around the broken design harder if you want to get the most out of the hardware. Andrew
Re: [PATCH net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
> And, some variable defines can not follow the reverse christmas tree
> style due to dependency, e.g.
> static void hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
>struct gdma_event *event)
> {
> struct hw_channel_context *hwc = ctx;
> struct gdma_dev *gd = hwc->gdma_dev;
> struct gdma_context *gc = gdma_dev_to_context(gd);
>
> I failed to find the reverse christmas tree rule in the Documentation/
> folder. I knew the rule and I thought it was documented there,
> but it looks like it's not. So my understanding is that in general we
> should follow the style, but there can be exceptions due to
> dependencies or logical grouping.
I expect DaveM will tell you to move gdma_dev_to_context(gd) into the
body of the function. Or if you have this pattern a lot, add a macro
gdma_ctx_to_context().
Reverse Christmas tree is not in the main Coding Style documentation,
but it is expected for netdev.
Andrew
Re: [PATCH net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
> +static int gdma_query_max_resources(struct pci_dev *pdev)
> +{
> + struct gdma_context *gc = pci_get_drvdata(pdev);
> + struct gdma_general_req req = { 0 };
> + struct gdma_query_max_resources_resp resp = { 0 };
> + int err;
Network drivers need to use reverse christmas tree. I spotted a number
of functions getting this wrong. Please review the whole driver.
> +
> + gdma_init_req_hdr(&req.hdr, GDMA_QUERY_MAX_RESOURCES,
> + sizeof(req), sizeof(resp));
> +
> + err = gdma_send_request(gc, sizeof(req), &req, sizeof(resp), &resp);
> + if (err || resp.hdr.status) {
> + pr_err("%s, line %d: err=%d, err=0x%x\n", __func__, __LINE__,
> +err, resp.hdr.status);
I expect checkpatch complained about this. Don't use pr_err(), use
dev_err(pdev->dev, ... of netdev_err(ndev, ... You should always have
access to dev or ndev, so please change all pr_ calls.
> +static unsigned int num_queues = ANA_DEFAULT_NUM_QUEUE;
> +module_param(num_queues, uint, 0444);
No module parameters please.
Andrew
Re: [PATCH v2 2/2] drivers: net: dsa: qca8k: add support for multiple cpu port
On Tue, Apr 06, 2021 at 06:50:40AM +0200, Ansuel Smith wrote: > qca8k 83xx switch have 2 cpu ports. Rework the driver to support > multiple cpu port. All ports can access both cpu ports by default as > they support the same features. Do you have more information about how this actually works. How does the switch decide which port to use when sending a frame towards the CPU? Is there some sort of load balancing? How does Linux decide which CPU port to use towards the switch? Andrew
Re: [PATCH RFC net 2/2] net: dsa: lantiq_gswip: Configure all remaining GSWIP_MII_CFG bits
> case PHY_INTERFACE_MODE_RGMII: > case PHY_INTERFACE_MODE_RGMII_ID: > case PHY_INTERFACE_MODE_RGMII_RXID: > case PHY_INTERFACE_MODE_RGMII_TXID: > miicfg |= GSWIP_MII_CFG_MODE_RGMII; > + > + if (phylink_autoneg_inband(mode)) > + miicfg |= GSWIP_MII_CFG_RGMII_IBS; Is there any other MAC driver doing this? Are there any boards actually enabling it? Since it is so odd, if there is nothing using it, i would be tempted to leave this out. Andrew
Re: [PATCH RFC net 1/2] net: dsa: lantiq_gswip: Don't use PHY auto polling
On Tue, Apr 06, 2021 at 10:35:07PM +0200, Martin Blumenstingl wrote:
> PHY auto polling on the GSWIP hardware can be used so link changes
> (speed, link up/down, etc.) can be detected automatically. Internally
> GSWIP reads the PHY's registers for this functionality. Based on this
> automatic detection GSWIP can also automatically re-configure it's port
> settings. Unfortunately this auto polling (and configuration) mechanism
> seems to cause various issues observed by different people on different
> devices:
> - FritzBox 7360v2: the two Gbit/s ports (connected to the two internal
> PHY11G instances) are working fine but the two Fast Ethernet ports
> (using an AR8030 RMII PHY) are completely dead (neither RX nor TX are
> received). It turns out that the AR8030 PHY sets the BMSR_ESTATEN bit
> as well as the ESTATUS_1000_TFULL and ESTATUS_1000_XFULL bits. This
> makes the PHY auto polling state machine (rightfully?) think that the
> established link speed (when the other side is Gbit/s capable) is
> 1Gbit/s.
> - None of the Ethernet ports on the Zyxel P-2812HNU-F1 (two are
> connected to the internal PHY11G GPHYs while the other three are
> external RGMII PHYs) are working. Neither RX nor TX traffic was
> observed. It is not clear which part of the PHY auto polling state-
> machine caused this.
> - FritzBox 7412 (only one LAN port which is connected to one of the
> internal GPHYs running in PHY22F / Fast Ethernet mode) was seeing
> random disconnects (link down events could be seen). Sometimes all
> traffic would stop after such disconnect. It is not clear which part
> of the PHY auto polling state-machine cauased this.
> - TP-Link TD-W9980 (two ports are connected to the internal GPHYs
> running in PHY11G / Gbit/s mode, the other two are external RGMII
> PHYs) was affected by similar issues as the FritzBox 7412 just without
> the "link down" events
>
> Switch to software based configuration instead of PHY auto polling (and
> letting the GSWIP hardware configure the ports automatically) for the
> following link parameters:
> - link up/down
> - link speed
> - full/half duplex
> - flow control (RX / TX pause)
>
> After a big round of manual testing by various people (who helped test
> this on OpenWrt) it turns out that this fixes all reported issues.
>
> Additionally it can be considered more future proof because any
> "quirk" which is implemented for a PHY on the driver side can now be
> used with the GSWIP hardware as well because Linux is in control of the
> link parameters.
>
> As a nice side-effect this also solves a problem where fixed-links were
> not supported previously because we were relying on the PHY auto polling
> mechanism, which cannot work for fixed-links as there's no PHY from
> where it can read the registers. Configuring the link settings on the
> GSWIP ports means that we now use the settings from device-tree also for
> ports with fixed-links.
>
> Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> Fixes: 3e6fdeb28f4c33 ("net: dsa: lantiq_gswip: Let GSWIP automatically set
> the xMII clock")
> Cc: sta...@vger.kernel.org
> Acked-by: Hauke Mehrtens
> Signed-off-by: Martin Blumenstingl
Having the MAC polling the PHY is pretty much always a bad idea.
Reviewed-by: Andrew Lunn
Andrew
Re: [PATCH] include: net: add dsa_cpu_ports function
On Tue, Apr 06, 2021 at 05:49:03AM +0200, Ansuel Smith wrote:
> In preparation for the future when dsa will support multi cpu port,
> dsa_cpu_ports can be useful for switch that has multiple cpu port to
> retrieve the cpu mask for ACL and bridge table.
>
> Signed-off-by: Ansuel Smith
> ---
> include/net/dsa.h | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 83a933e563fe..d71b1acd9c3e 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -446,6 +446,18 @@ static inline u32 dsa_user_ports(struct dsa_switch *ds)
> return mask;
> }
>
> +static inline u32 dsa_cpu_ports(struct dsa_switch *ds)
> +{
> + u32 mask = 0;
> + int p;
> +
> + for (p = 0; p < ds->num_ports; p++)
> + if (dsa_is_cpu_port(ds, p))
> + mask |= BIT(p);
> +
> + return mask;
> +}
Hi Ansuel
We don't add a function unless it has a user. Please call it from somewhere.
Andrew
Re: [PATCH net-next v2 0/2] Enable 2.5Gbps speed for stmmac
> The limitation is not on the MAC, PCS or the PHY. For Intel mgbe, the > overclocking of 2.5 times clock rate to support 2.5G is only able to be > configured in the BIOS during boot time. Kernel driver has no access to > modify the clock rate for 1Gbps/2.5G mode. The way to determined the > current 1G/2.5G mode is by reading a dedicated adhoc register through mdio > bus. > In short, after the system boot up, it is either in 1G mode or 2.5G mode > which not able to be changed on the fly. Right. It would of been a lot easier if this was in the commit message from the beginning. Please ensure the next version does say this. > Since the stmmac MAC can pair with any PCS and PHY, I still prefer that we tie > this platform specific limitation with the of MAC. As stmmac does handle > platform > specific config/limitation. So yes, this needs to be somewhere in the intel specific stmmac code, with a nice comment explaining what is going on. What PHY are you using? The Aquantia/Marvell multi-gige phy can do rate adaptation. So you could fix the MAC-PHY link to 2500BaseX, and let the PHY internally handle the different line speeds. Andrew
Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver
On Tue, Apr 06, 2021 at 11:57:14PM +0800, DENG Qingfang wrote: > On Tue, Apr 6, 2021 at 11:47 PM Chun-Kuang Hu wrote: > > > > Hi, Qingfang: > > > > DENG Qingfang 於 2021年4月6日 週二 下午10:19寫道： > > > --- a/drivers/net/phy/Kconfig > > > +++ b/drivers/net/phy/Kconfig > > > @@ -207,6 +207,11 @@ config MARVELL_88X_PHY > > > Support for the Marvell 88X Dual-port Multi-speed Ethernet > > > Transceiver. > > > > > > +config MEDIATEK_PHY > > > > There are many Mediatek phy drivers in [1], so use a specific name. > > So "MEDIATEK_MT7530_PHY" should be okay? No. MEDIATEK_PHY is consistent with other PHY drivers. Please leave it as it is. And with time, we expect other devices will be supported by this driver, so having MT7530 in the name would be confusing. Andrew
Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver
On Tue, Apr 06, 2021 at 11:47:08PM +0800, Chun-Kuang Hu wrote: > Hi, Qingfang: > > DENG Qingfang 於 2021年4月6日 週二 下午10:19寫道： > > > > Add support for MediaTek PHYs found in MT7530 and MT7531 switches. > > The initialization procedure is from the vendor driver, but due to lack > > of documentation, the function of some register values remains unknown. > > > > Signed-off-by: DENG Qingfang > > --- > > drivers/net/phy/Kconfig| 5 ++ > > drivers/net/phy/Makefile | 1 + > > drivers/net/phy/mediatek.c | 109 + > > 3 files changed, 115 insertions(+) > > create mode 100644 drivers/net/phy/mediatek.c > > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > > index a615b3660b05..edd858cec9ec 100644 > > --- a/drivers/net/phy/Kconfig > > +++ b/drivers/net/phy/Kconfig > > @@ -207,6 +207,11 @@ config MARVELL_88X_PHY > > Support for the Marvell 88X Dual-port Multi-speed Ethernet > > Transceiver. > > > > +config MEDIATEK_PHY > > There are many Mediatek phy drivers in [1], so use a specific name. Those are generic PHY drivers, where as this patch is add a PHY driver. The naming used in this patch is consistent with other PHY drivers. So i'm happy with this patch in this respect. PHY drivers have been around a lot longer than generic PHY drivers. So i would actually say the generic PHY driver naming should make it clear they are generic PHYs, not PHYs. But lets not bike shed about this too much. Andrew
Re: [RFC net-next 2/4] net: dsa: mt7530: add interrupt support
On Tue, Apr 06, 2021 at 11:39:12PM +0800, DENG Qingfang wrote: > On Tue, Apr 6, 2021 at 11:30 PM Andrew Lunn wrote: > > > > On Tue, Apr 06, 2021 at 10:18:17PM +0800, DENG Qingfang wrote: > > > Add support for MT7530 interrupt controller to handle internal PHYs. > > > > Are the interrupts purely PHY interrupts? Or are there some switch > > operation interrupts, which are currently not used? > > There are other switch operations interrupts as well, such as VLAN > member violation, switch ACL hit. O.K. So that makes it similar to the mv88e6xxx. With that driver, i kept interrupt setup and mdio setup separate. I add the interrupt controller first, and then do mdio setup, calling a helper to map the PHY interrupts and assign them to bus->irq[]. That gives you a cleaner structure when you start using the other interrupts. Andrew
Re: [RFC net-next 2/4] net: dsa: mt7530: add interrupt support
On Tue, Apr 06, 2021 at 10:18:17PM +0800, DENG Qingfang wrote: > Add support for MT7530 interrupt controller to handle internal PHYs. Are the interrupts purely PHY interrupts? Or are there some switch operation interrupts, which are currently not used? I'm just wondering if it is correct to so closely tie interrupts and MDIO together. Andrew
Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver
On Tue, Apr 06, 2021 at 10:18:16PM +0800, DENG Qingfang wrote:
> Add support for MediaTek PHYs found in MT7530 and MT7531 switches.
Do you know if this PHY is available standalone?
> +static int mt7531_phy_config_init(struct phy_device *phydev)
> +{
> + mtk_phy_config_init(phydev);
> +
> + /* PHY link down power saving enable */
> + phy_set_bits(phydev, 0x17, BIT(4));
> + phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, 0xc6, 0x300);
> +
> + /* Set TX Pair delay selection */
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x13, 0x404);
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x14, 0x404);
This gets me worried about RGMII delays. We have had bad backwards
compatibility problems with PHY drivers which get RGMII delays wrong.
Since this is an internal PHY, i suggest you add a test to the
beginning of mt7531_phy_config_init():
if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
return -EINVAL;
We can then solve RGMII problems when somebody actually needs RGMII.
Andrew
Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO
> > Speed: 2500Mb/s and Duplex: Half is very unlikely. You really only > > ever see 10 Half and occasionally 100 Half. Anything above that will > > be full duplex. > > > > It is probably best to admit the truth and use DUPLEX_UNKNOWN. > > Agreed. I didn't notice this "lie" until I was writing the commit > message and wasn't sure off-hand how to fix it. Decided a follow on > patch could fix it up once this series lands. > > You are right that DUPLEX_UNKNOWN is the safest (and usually correct) default. > Additionally, if RX and TX speed are equal, I am willing to assume > this is DUPLEX_FULL. Is this same interface used by WiFi? Ethernet does not support different rates in each direction. So if RX and TX are different, i would actually say something is broken. 10 Half is still doing 10Mbps in each direction, it just cannot do both at the same time. WiFi can have asymmetric speeds. > I can propose something like this in a patch: > > grundler <1637>git diff > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index 86eb1d107433..a7ad9a0fb6ae 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -978,6 +978,11 @@ int usbnet_get_link_ksettings_internal(struct > net_device *net, > else > cmd->base.speed = SPEED_UNKNOWN; > > + if (dev->rx_speed == dev->tx_speed) > + cmd->base.duplex = DUPLEX_FULL; > + else > + cmd->base.duplex =DUPLEX_UNKNOWN; > + > return 0; > } > EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal); So i would say this is wrong. I would just set DUPLEX_UNKNOWN and be done. > I can send this out later once this series lands or you are welcome to > post this with additional checks if you like. Yes, this discussion should not prevent this patchset from being merged. > If we want to assume autoneg is always on (regardless of which type of > media cdc_ncm/cdc_ether are talking to), we could set both supported > and advertising to AUTO and lp_advertising to UNKNOWN. I pretty much agree autoneg has to be on. If it is not, and it is using a forced speed, there would need to be an additional API to set what it is forced to. There could be such proprietary calls, but the generic cdc_ncm/cdc_ether won't support them. But i also don't know how setting autoneg actually helps the user. Everybody just assumes it is supported. If you really know auto-neg is not supported and you can reliably indicate that autoneg is not supported, that would be useful. But i expect most users want to know if their USB 2.0 device is just doing 100Mbps, or if their USB 3.0 device can do 2.5G. For that, you need to see what is actually supported. Andrew
Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO
On Mon, Apr 05, 2021 at 04:13:40PM -0700, Grant Grundler wrote: > This series introduces support for USB network devices that report > speed as a part of their protocol, not emulating an MII to be accessed > over MDIO. > > v2: rebased on recent upstream changes > v3: incorporated hints on naming and comments > v4: fix misplaced hunks; reword some commit messages; > add same change for cdc_ether > v4-repost: added "net-next" to subject and Andrew Lunn's Reviewed-by > > I'm reposting Oliver Neukum's patch series with > fix ups for "misplaced hunks" (landed in the wrong patches). > Please fixup the "author" if "git am" fails to attribute the > patches 1-3 (of 4) to Oliver. > > I've tested v4 series with "5.12-rc3+" kernel on Intel NUC6i5SYB > and + Sabrent NT-S25G. Google Pixelbook Go (chromeos-4.4 kernel) > + Alpha Network AUE2500C were connected directly to the NT-S25G > to get 2.5Gbps link rate: > # ethtool enx002427880815 > Settings for enx002427880815: > Supported ports: [ ] > Supported link modes: Not reported > Supported pause frame use: No > Supports auto-negotiation: No > Supported FEC modes: Not reported > Advertised link modes: Not reported > Advertised pause frame use: No > Advertised auto-negotiation: No > Advertised FEC modes: Not reported > Speed: 2500Mb/s > Duplex: Half > Auto-negotiation: off > Port: Twisted Pair > PHYAD: 0 > Transceiver: internal > MDI-X: Unknown > Current message level: 0x0007 (7) >drv probe link > Link detected: yes > > > "Duplex" is a lie since we get no information about it. You can ask the PHY. At least those using mii or phylib. If you are using mii, then mii_ethtool_get_link_ksettings() should set it correctly. If you are using phylib, phy_ethtool_get_link_ksettings() will correctly set it. If you are not using either of these, you are on your own. Speed: 2500Mb/s and Duplex: Half is very unlikely. You really only ever see 10 Half and occasionally 100 Half. Anything above that will be full duplex. It is probably best to admit the truth and use DUPLEX_UNKNOWN. > I expect "Auto-Negotiation" is always true for cdc_ncm and > cdc_ether devices and perhaps someone knows offhand how > to have ethtool report "true" instead. ethtool_link_ksettings contains three bitmaps: supported: The capabilities of this device. advertising: What this device is telling the link peer it can do. lp_advertising: What the link peer is telling us it can do. So to get Supports auto-negotiation to be true you need to set bit ETHTOOL_LINK_MODE_Autoneg_BIT in supported. For Advertised auto-negotiation: you need to set the same bit in advertising. Auto-negotiation: off is i think from base.autoneg. Andrew
Re: [PATCH 1/2] of: net: pass the dst buffer to of_get_mac_address()
On Tue, Apr 06, 2021 at 03:19:11AM +0800, kernel test robot wrote: > Hi Michael, > > I love your patch! Yet something to improve: Looks correct. You missed the #else case for #ifdef CONFIG_OF in stmmac_platform.c Lets see what else 0-day finds before i start reviewing. Andrew
Re: gemini: sl3516: Mainlining of NS 2502
On Mon, Apr 05, 2021 at 08:39:54PM +0200, Corentin Labbe wrote:
> mdio0: ethernet-phy {
Not relevant to the brokennes, but this should not be called
ethernet-phy. The example given in
Documentation/devicetree/bindings/net/mdio-gpio.txt is
mdio0: mdio
> compatible = "virtual,mdio-gpio";
> gpios = <&gpio0 22 GPIO_ACTIVE_HIGH>, /* MDC */
> <&gpio0 21 GPIO_ACTIVE_HIGH>; /* MDIO */
> #address-cells = <1>;
> #size-cells = <0>;
> phy0: ethernet-phy@1 {
> reg = <1>;
> device_type = "ethernet-phy";
> };
> };
So you are using a bit-banging MDIO bus.
> gpio0: gpio@4d00 {
> pinctrl-names = "default";
> pinctrl-0 = <&gpio0_default_pins>;
> };
and here is the gpio controller.
> ethernet@6000 {
> status = "okay";
> ethernet-port@0 {
> phy-mode = "rgmii";
> phy-handle = <&phy0>;
> };
> ethernet-port@1 {
> /* Not used in this platform */
> };
> };
and this look O.K.
> libphy: Fixed MDIO Bus: probed
> mdio-gpio ethernet-phy: failed to get alias id
That does not look too good. But it is just a warning.
if (pdev->dev.of_node) {
bus_id = of_alias_get_id(pdev->dev.of_node, "mdio-gpio");
if (bus_id < 0) {
dev_warn(&pdev->dev, "failed to get alias id\n");
bus_id = 0;
}
If you look at the example in the documentation, it has
aliases {
mdio-gpio0 = &mdio0;
};
If you add that, i guess this will go away.
> libphy: GPIO Bitbanged MDIO: probed
> tun: Universal TUN/TAP device driver, 1.6
> gmac-gemini 6000.ethernet: Ethernet device ID: 0x000, revision 0x1
> gemini-ethernet-port 60008000.ethernet-port: probe 60008000.ethernet-port ID 0
> gemini-ethernet-port 60008000.ethernet-port: using a random ethernet address
> RTL8211B Gigabit Ethernet gpio-0:01: attached PHY driver
> (mii_bus:phy_addr=gpio-0:01, irq=POLL)
So a realtek PHY has been found on the MDIO bus. Good.
> gemini-ethernet-port 60008000.ethernet-port eth0: irq 30, DMA @ 0x0x60008000,
> GMAC @ 0x0x6000a000
and everything looks good.
> gemini-ethernet-port 6000c000.ethernet-port: probe 6000c000.ethernet-port ID 1
> gemini-ethernet-port 6000c000.ethernet-port: using a random ethernet address
> gemini-ethernet-port 6000c000.ethernet-port (unnamed net_device)
> (uninitialized): PHY init failed
And now it seems to of all gone horribly wrong :-(
This is the second Ethernet interface, the one you have commented as
/* Not used in this platform */
I _think_ you just need to delete the entry, otherwise it tried to
probe it. And then that probe fails, it looks like it also fails the
working interface :-(
> 1: lo: mtu 65536 qdisc noqueue qlen 1000
> link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> inet 127.0.0.1/8 scope host lo
>valid_lft forever preferred_lft forever
> inet6 ::1/128 scope host
>valid_lft forever preferred_lft forever
> 2: eth0: mtu 1500 qdisc noop qlen 1000
> link/ether b6:95:3c:18:98:62 brd ff:ff:ff:ff:ff:ff
> 3: sit0@NONE: mtu 1480 qdisc noop qlen 1000
> link/sit 0.0.0.0 brd 0.0.0.0
> # udhcpc -i eth0
> udhcpc: started, v1.33.0
> gmac-gemini 6000.ethernet: allocate 512 pages for queue
> gemini-ethernet-port 60008000.ethernet-port eth0: Unsupported PHY speed (-1)
> on gpio-0:01
> gemini-ethernet-port 60008000.ethernet-port eth0: Link is Down
> gemini-ethernet-port 60008000.ethernet-port eth0: link flow control: none
> udhcpc: socket: Address family not supported by protocol
> # gemini-ethernet-port 60008000.ethernet-port eth0: Link is Up - 1Gbps/Full -
> flow control rx/tx
> gemini-ethernet-port 60008000.ethernet-port eth0: link flow control: tx
> IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>
> # udhcpc -i eth0
> udhcpc: started, v1.33.0
> udhcpc: socket: Address family not supported by protocol
That suggests the kernel you have build does not have PF_PACKET.
Enable CONFIG_PACKET_DIAG.
Andrew
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
> Now, instead of encoding this information of the bus' capabilities at both > places, I'd propose just checking the mii_bus->capabilities field in the > mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having > two > places where this information is stored. What do you think about that? You will need to review all the MDIO bus drivers to make sure they correctly set the capabilities. There is something like 55 using of_mdiobus_register() and 45 using mdiobus_register(). So you have 100 drivers to review. Andrew
Re: [PATCH net-next v2 0/2] Enable 2.5Gbps speed for stmmac
> > You have a MAC and an PCS in the stmmac IP block. That then has > > some > > sort of SERDES interface, running 1000BaseX, SGMII, SGMII > > overclocked > > at 2.5G or 25000BaseX. Connected to the SERDES you have a PHY > > which > > converts to copper, giving you 2500BaseT. > > > > You said earlier, that the PHY can only do 2500BaseT. So it should > > be > > the PHY driver which sets supported to 2500BaseT and no other > > speeds. > > > > You should think about when somebody uses this MAC with a > > different > > PHY, one that can do the full range of 10/half through to 2.5G > > full. What generally happens is that the PHY performs auto-neg to > > determine the link speed. For 10M-1G speeds the PHY will > > configure its > > SERDES interface to SGMII and phylink will ask the PCS to also be > > configured to SGMII. If the PHY negotiates 2500BaseT, it will > > configure its side of the SERDES to 2500BaseX or SGMII > > overclocked at > > 2.5G. Again, phylink will ask the PCS to match what the PHY is > > doing. > > > > So, where exactly is the limitation in your hardware? PCS or PHY? > The limitation in the hardware is at the PCS side where it is either running > in SGMII 2.5G or SGMII 1G speeds. > When running on SGMII 2.5G speeds, we disable the in-band AN and use 2.5G > speed only So there is no actual limitation! The MAC should indicate it can do 10Half through to 2500BaseT. And you need to listen to PHYLINK and swap the PCS between SGMII to overclocked SGMII when it requests. PHYLINK will call stmmac_mac_config() and use state->interface to decide how to configure the PCS to match what the PHY is doing. Andrew
Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
On Sun, Apr 04, 2021 at 09:23:55PM +0200, Danilo Krummrich wrote: > On Fri, Apr 02, 2021 at 01:58:58PM +0100, Russell King - ARM Linux admin > wrote: > > On Fri, Apr 02, 2021 at 03:10:49AM +0200, Danilo Krummrich wrote: > > > On Thu, Apr 01, 2021 at 09:48:58AM +0100, Russell King - ARM Linux admin > > > wrote: > > > > One could also argue this is a feature, and it allows userspace to > > > > know whether C45 cycles are supported or not. > > > > > > > No, if the userspace requests such a transfer although the MDIO controller > > > does not support real c45 framing the kernel will call mdiobus_c45_addr() > > > to > > > join the devaddr and and regaddr in one parameter and pass it to > > > mdiobus_read() or mdiobus_write(). A bus driver not supporting c45 framing > > > will not care and just mask/shift the joined value and write it to the > > > particular register. Obviously, this will result into complete garbage > > > being > > > read or (even worse) written. > > > > > > We have established that MDIO drivers need to reject accesses for > > reads/writes that they do not support - this isn't something that > > they have historically checked for because it is only recent that > > phylib has really started to support clause 45 PHYs. > > > I see, that's why you consider it a feature - because it is. > What do you think about adding a flag MDIO_PHY_ID_MMD (or similar) analog to > MDIO_PHY_ID_C45 for phy_mii_ioctl() to check for, such that userspace can ask > for an indirect access in order to save userspace doing the indirect access > itself. A nice side effect would be saving 3 syscalls per request. We don't care about the performance of this IOCTL interface. It is for debug only, and you need to be very careful how you use it, because you can very easily shoot yourself in the foot. > So currently every driver should check for the flag MII_ADDR_C45 and report an > error in case it's unsupported. > > What do you think about checking the bus' capabilities instead in > mdiobus_c45_*()? This way the check if C45 is supported can even happen before > calling the driver at all. I think that would be a little cleaner than having > two places where information of the bus' capabilities are stored (return value > of read/write functions and the capabilities field). > > I think there are not too many drivers setting their capabilities though, but > it should be easy to derive this information from how and if they handle the > MII_ADDR_C45 flag. I actually don't think anything needs to change. The Marvell PHY probably probes due to its C22 IDs. The driver will then requests C45 access which automagically get converted into C45 over C22 for your hardware, but remain C45 access for bus drivers which support C45. Andrew
Re: [PATCH net-next v2 0/2] Enable 2.5Gbps speed for stmmac
On Mon, Apr 05, 2021 at 07:29:51PM +0800, Michael Sit Wei Hong wrote: > This patchset enables 2.5Gbps speed mode for stmmac. > Link speed mode is detected and configured at serdes power up sequence. > For 2.5G, we do not use SGMII in-band AN, we check the link speed mode > in the serdes and disable the in-band AN accordingly. > > Changes: > v1 -> v2 > patch 1/2 > -Remove MAC supported link speed masking > > patch 2/2 > -Add supported link speed masking in the PCS So there still some confusion here. |MAC - PCS |---serdes---| PHY |--- copper You have a MAC and an PCS in the stmmac IP block. That then has some sort of SERDES interface, running 1000BaseX, SGMII, SGMII overclocked at 2.5G or 25000BaseX. Connected to the SERDES you have a PHY which converts to copper, giving you 2500BaseT. You said earlier, that the PHY can only do 2500BaseT. So it should be the PHY driver which sets supported to 2500BaseT and no other speeds. You should think about when somebody uses this MAC with a different PHY, one that can do the full range of 10/half through to 2.5G full. What generally happens is that the PHY performs auto-neg to determine the link speed. For 10M-1G speeds the PHY will configure its SERDES interface to SGMII and phylink will ask the PCS to also be configured to SGMII. If the PHY negotiates 2500BaseT, it will configure its side of the SERDES to 2500BaseX or SGMII overclocked at 2.5G. Again, phylink will ask the PCS to match what the PHY is doing. So, where exactly is the limitation in your hardware? PCS or PHY? Andrew
Re: [PATCH net-next 1/2] net: stmmac: enable 2.5Gbps link speed
On Mon, Apr 05, 2021 at 09:07:34AM +, Voon, Weifeng wrote:
> > On Fri, Apr 02, 2021 at 07:45:04AM +, Voon, Weifeng wrote:
> > > > > + /* 2.5G mode only support 2500baseT full duplex only */
> > > > > + if (priv->plat->has_gmac4 && priv->plat->speed_2500_en) {
> > > > > + phylink_set(mac_supported, 2500baseT_Full);
> > > > > + phylink_set(mask, 10baseT_Half);
> > > > > + phylink_set(mask, 10baseT_Full);
> > > > > + phylink_set(mask, 100baseT_Half);
> > > > > + phylink_set(mask, 100baseT_Full);
> > > > > + phylink_set(mask, 1000baseT_Half);
> > > > > + phylink_set(mask, 1000baseT_Full);
> > > > > + phylink_set(mask, 1000baseKX_Full);
> > > >
> > > > Why? This seems at odds to the comment above?
> > >
> > > > What about 2500baseX_Full ?
> > >
> > > The comments explain that the PCS<->PHY link is in 2500BASE-X and why
> > > 10/100/1000 link speed is mutually exclusive with 2500.
> > > But the connected external PHY are twisted pair cable which only
> > > supports 2500baseT_full.
> >
> > The PHY should indicate what modes its supports. The PHY drivers
> > get_features() call should set supported to only 2500baseT_Full, if that is
> > all it supports.
> >
> > What modes are actually used should then be the intersect of what both the
> > MAC and the PHY indicate they can do.
>
> Noted Andrew. Instead of masking the 10/100/1000 mode support in the MAC, we
> will
> set the supported modes in the PCS.
PCS?
You said:
> > > But the connected external PHY are twisted pair cable which only
> > > supports 2500baseT_full.
So it should be the PHY, not the PCS, which indicates it only supports
2500baseT_full.
Andrew
