net: dsa: Add driver for the Microchip KSZ8463 3-port 10/100 Ethernet switch - #116253
net: dsa: Add driver for the Microchip KSZ8463 3-port 10/100 Ethernet switch#116253vengaer wants to merge 5 commits into
Conversation
|
@yangbolu1991 and @JiafeiPan, according to MAINTAINERS.yml, this driver would end up under your maintenance. Is that okay with you or should I add myself as a collaborator to assist in reviewing future changes to it? |
0e53992 to
ff1b5e6
Compare
|
Latest version should pass both style and sonarcloud checks |
|
| /* Pin configuration */ | ||
| const struct pinctrl_dev_config *pincfg; |
There was a problem hiding this comment.
why does this device has pinctrl? it is a external device.
There was a problem hiding this comment.
Thanks for the swift and thorough review.
The driver uses pinctrl to allow the pins in the int-gpios and reset-gpios properties to be muxed as GPIOs.
There was a problem hiding this comment.
that belongs in the gpio controllers pinctrl. All pins that are going to be used as GPIOs must be in the gpio controllers pinctrl. for this dsa driver they are just gpio pins. this driver don't has to configure them as gpios, they already are.
external chips typically don't have pinctrl, only if that external chip itself has a its own pinctrl internally if f.e. it integrates a spi, i2c and gpio controller and the pins are shared or dynamically assigned.
There was a problem hiding this comment.
or do yo have examples of other drivers where this is done in the zephyr tree?
There was a problem hiding this comment.
I don't have such an example, no. I suppose I've always done it this way simply because it made sense in my head and no one has ever called me out on it. You do make a compelling argument though and after greping for bindings using pinctrl, it would seem I have indeed used it incorrectly here. I appreciate the correction.
I'll remove anything pinctrl-related.
| - reg | ||
| - name: dsa.yaml | ||
| child-binding: | ||
| property-allowlist: |
There was a problem hiding this comment.
The driver doesn't use the phy-handle property defined for child-bindings in dsa.yaml. I figured allowing only the relevant properties was a good way of signifying that.
There was a problem hiding this comment.
just because we don't use a prop, doesn't mean we don't need to include it
There was a problem hiding this comment.
If you don't mind me asking, what purpose does it serve to include a property if it isn't, and likely never will be, used in this particular driver? I'm not trying to argue, I'd be happy to include all properties from dsa.yaml , I just want to know what I'm missing.
| - port | ||
| - reg | ||
| - dsa-tag-protocol | ||
| - name: pinctrl-device.yaml |
There was a problem hiding this comment.
As mentioned in the other comment, it's for muxing the int-gpios and reset-gpios pins. LMK if I'm missing something.
| microchip,autoneg-poll-interval: | ||
| type: int | ||
| default: 100 | ||
| description: | | ||
| Interval at which the PHY should be polled for auto-negotiation completion. In ms. | ||
|
|
||
| microchip,autoneg-timeout: | ||
| type: int | ||
| default: 5000 | ||
| description: | | ||
| Number of milliseconds after which auto-negotiation is assumed to have failed. On failure, | ||
| the PHY falls back on whatever is configured via microchip,fixed-link-speed. | ||
|
|
||
| The value must be greater than microchip,autoneg-poll-interval. |
There was a problem hiding this comment.
this is something for Kconfigs
| microchip,link-state-poll-interval: | ||
| type: int | ||
| default: 1000 | ||
| min: 10 | ||
| description: | | ||
| Interval at which the link state is polled should no int-gpios property be provided. | ||
| Given in ms. | ||
|
|
|
|
||
| pool = net_buf_pool_get(pkt->buffer->pool_id); | ||
| buf = net_buf_alloc_len(pool, sizeof(*tag), K_NO_WAIT); | ||
| if (!buf) { | ||
| NET_WARN("Could not allocate root for tag byte"); | ||
| return pkt; | ||
| } | ||
|
|
||
| tag = net_buf_simple_tail(&buf->b); | ||
| *tag = dsa_cfg->port_idx + 1; | ||
|
|
||
| net_buf_add(buf, sizeof(*tag)); | ||
| net_buf_frag_add(pkt->buffer, buf); |
There was a problem hiding this comment.
before allocation a new fragment, you should check if there is still space in the last fragment.
| static int ksz8463_spi_read_raw(const struct spi_dt_spec *spi, uint16_t addr, void *dst, size_t n) | ||
| { | ||
| int ret; | ||
| uint16_t cmd; | ||
| uint8_t buf[KSZ8463_SPI_CMD_MAX_SIZE]; | ||
|
|
||
| const struct spi_buf_set tx = { | ||
| .buffers = &(const struct spi_buf){.buf = buf, .len = sizeof(cmd)}, | ||
| .count = 1u, | ||
| }; | ||
|
|
||
| const struct spi_buf_set rx = { | ||
| .buffers = &(const struct spi_buf){.buf = buf, .len = sizeof(cmd) + n}, | ||
| .count = 1u, | ||
| }; | ||
|
|
||
| if (unlikely(n > KSZ8463_SPI_CMD_MAX_DATA_PH_SIZE)) { | ||
| return -ENOBUFS; | ||
| } | ||
|
|
||
| cmd = ksz8463_spi_cmd(addr, n); | ||
| sys_put_be16(cmd, buf); | ||
|
|
||
| ret = spi_transceive_dt(spi, &tx, &rx); | ||
| if (ret >= 0 && n) { | ||
| memcpy(dst, buf + sizeof(cmd), n); | ||
| } | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
you can put the original buffers directly in spi_buf_set, no need to combine them to one buffer. take a look at the spi api (spi.h)
There was a problem hiding this comment.
That's a good point, thank you.
| /* Like ksz8463_spi_read32_raw but with automatic lock acquisition */ | ||
| static inline int64_t ksz8463_spi_read32(const struct spi_dt_spec *spi, uint16_t addr) | ||
| { | ||
| int64_t ret = ksz8463_spi_lock(spi); |
There was a problem hiding this comment.
what is the lock for? the spi driver already has a internal lock, there is no need for a second.
There was a problem hiding this comment.
Certain operations require multiple subsequent reads and/or writes without risk the registers being changed in-between. The perhaps most obvious examples are the ksz8463_spi_update_bitsXX functions which read a particular register, alter the requested bits and write the updated value back to the device.
| * not that INT_MAX >= UINT16_MAX. See Section 5.2.4.2.1. | ||
| */ | ||
| BUILD_ASSERT(INT_MAX >= UINT16_MAX, "Potential overflow"); | ||
| return (int)be16; |
There was a problem hiding this comment.
use return values either for errors or data, not both. return the data via a pointer instead
| size_t n) | ||
| { | ||
| uint16_t cmd; | ||
| uint8_t buf[KSZ8463_SPI_CMD_MAX_SIZE]; |
There was a problem hiding this comment.
you don't need this intermediate buffer
yangbolu1991
left a comment
There was a problem hiding this comment.
Some comments added.
Is it possible to add board support to verify the driver?
At least CI build could be run.
Thanks.
| depends on $(dt_compat_any_has_prop,$(DSA_PORT_COMPAT),dsa-tag-protocol,1) | ||
| help | ||
| NXP NETC tag protocol. | ||
| rsource "Kconfig.netc" |
There was a problem hiding this comment.
Kconfig.nxp_imx_netc should be better to keep same name with driver.
There was a problem hiding this comment.
Sure, thanks for pointing that out.
| @@ -0,0 +1,33 @@ | |||
| # NXP NETC DSA configuration options | |||
|
|
||
| /* Max size of a complete SPI command */ | ||
| KSZ8463_SPI_CMD_MAX_SIZE = KSZ8463_SPI_CMD_PH_SIZE + KSZ8463_SPI_CMD_MAX_DATA_PH_SIZE, | ||
| }; |
There was a problem hiding this comment.
I think we should use #define instead for these size, offset ...
There was a problem hiding this comment.
Gladly, I'll update it.
|
|
||
| /* Advertise 10BASE-T half-duplex */ | ||
| KSZ8463_PxANAR_ADV_10BASE_T_HALF_DUPLEX = BIT(5), | ||
| }; |
There was a problem hiding this comment.
I think we should use #define for all the registers addresses and bitfield above.
There was a problem hiding this comment.
You'll hear no objection from me.
| if (!ret && cfg->pincfg) { | ||
| ret = pinctrl_apply_state(cfg->pincfg, PINCTRL_STATE_DEFAULT); | ||
| } | ||
| if (!ret && ksz8463_have_hard_reset(dev)) { |
There was a problem hiding this comment.
The style looks a little weird. Generally we check the error to return. Use explicit ret != 0.
Same comments for some other places.
if (ret != 0)
return ret;
if (ksz8463_have_hard_reset(dev))
ret = ksz8463_hard_reset(dev);
if (ret != 0)
return ret;
There was a problem hiding this comment.
Thanks, I'll go through the commits and update them.
| } | ||
|
|
||
| ret = gpio_pin_configure_dt(cfg->rst_gpio, GPIO_OUTPUT_ACTIVE); | ||
| if (!ret) { |
There was a problem hiding this comment.
Should check error to handle, but not run sequentially.
Same comments on some other places.
There was a problem hiding this comment.
Just to make sure I understand you correctly, you're referring to the same early return pattern you requested elsewhere here? So something like
ret = gpio_pin_configure_dt(cfg->rst_gpio, GPIO_OUTPUT_ACTIVE);
if (ret != 0) {
return ret;
}
k_sleep(K_MSEC(10));
ret = gpio_pin_set_dt(cfg->rst_gpio, 0);
if (ret != 0) {
return ret;
}
/* ... */If so, you got it.
| struct dsa_api ksz8463_dsa_api = { | ||
| .port_init = ksz8463_port_init, | ||
| .switch_setup = ksz8463_switch_setup, | ||
| .get_capabilies = ksz8463_get_capabilities, |
There was a problem hiding this comment.
Yikes, sorry you had to see that. Seems I must have messed up a rebase. I'll make sure to fix it for in the next version of the PR.
We are okay to maintain it. Thanks:) |
Much appreciated, thank you! |
Thank you very much for the review! The KSZ8463 is a standalone switch meaning it doesn't do much at all on its own. I do have a sample that configures the switch for use with a Nucleo H755ZI-Q board that I've used while developing the driver. The reason I didn't include it in this PR is that hooking the switch up to the Nucleo board requires that the PHY that comes with the latter first be removed. I don't know what the criteria for samples are but if requiring modifications to the board is not grounds for ruling one out, I'd happily contribute the one I have. With it, the driver ought at least be built as part of the CI. |
Keeping configuration options for disparate Ethernet all in one file would eventually result in the latter becoming cluttered. Move the NETC-specific options to a separate file. Signed-off-by: Vilhelm Engström <vilhelm.engstrom@tuta.io>
| /* NXP NETC switch tag protocol */ | ||
| #define DSA_TAG_PROTO_NETC 1 | ||
| /* Microchip KSZ8463 tag protocol */ | ||
| #define DSA_TAG_PROTO_KSZ8463 2 |
There was a problem hiding this comment.
define 'DSA_TAG_PROTO_KSZ8463' is missing Doxygen comments.
Is using Doxygen here desirable? If so, I'll convert the other comments in the file too.
There was a problem hiding this comment.
Actually I don't think we need that for dt-bindings header.
There was a problem hiding this comment.
Okay, thanks. Then I'll ignore the linter warning.
I would suggest to add platform support to verify it. Thanks. |
Nice, I'll include such a sample in the next version of the PR then. It should be up by this time tomorrow. |
| /* NXP NETC switch tag protocol */ | ||
| #define DSA_TAG_PROTO_NETC 1 | ||
| /* Microchip KSZ8463 tag protocol */ | ||
| #define DSA_TAG_PROTO_KSZ8463 2 |
There was a problem hiding this comment.
Okay, thanks. Then I'll ignore the linter warning.
ff1b5e6 to
77bea75
Compare
|
All comments should have been addressed in the latest version. There's also a sample showing how to configure the device for use with the Nucleo H755ZI-Q board that allows users to manage interfaces via the net shell. |
The KSZ8463 is a 3-port switch supporting Fast Ethernet originally developed by Micrel. The driver supports configuring the chip over SPI and, provided that an int-gpio is provided, reconfigures the internal PHYs of the user ports on link status change. If not such GPIO property is provided, the driver falls back on periodically polling the link state. Both of these approaches provide full hotplug support. The KSZ8463 driver manages not only the switch and its ports but also the internal PHYs of the user ports. Each of these "levels", i.e. the switch, the ports and the PHYs, are configured individually via the devicetree. See the bindings file included in this commit for an complete list of properties. While the switch does support a tag protocol, support for this is added as a separate commit to reduce the size of this changeset. The hardware used to develop the driver was graciously provided by Microchip free of charge. Signed-off-by: Vilhelm Engström <vilhelm.engstrom@tuta.io>
The KSZ8463 uses a single-byte tag inserted just after the payload of each Ethernet frame. Apart from designating on which user port the packet was either received or is to be sent, tags included in packets sent from the conduit port on the host to the CPU port on the KSZ8463 also include the frame priority as defined by 802.1p. This changeset implements software support for the KSZ8463 tag protocol. Additionally, it updates the related driver to enable tail tagging on switches for which the devicetree node representing the CPU port set the dsa-tag-protocol property appropriately. Signed-off-by: Vilhelm Engström <vilhelm.engstrom@tuta.io>
Mention the KSZ8463 DSA driver in the New Drivers section. Signed-off-by: Vilhelm Engström <vilhelm.engstrom@tuta.io>
Provide a sample showing how the KSZ8463 is configured for the Cortex-M7 CPU on ST's Nucleo H755ZI-Q. The KSZ8463MLI evaluation board is assumed to be connected to spi1. The primary focus of the sample is illustrating a proper devicetree configuration. The software included does nothing. Instead, users are expected to use the net shell to manage the DSA interfaces. Signed-off-by: Vilhelm Engström <vilhelm.engstrom@tuta.io>
77bea75 to
86c49ad
Compare
|
I'll figure out the rest of the linter warnings, and why I'm not seeing them when I run the scripts locally, this evening |



This PR introduces support for Microchip's KSZ8463 Ethernet switch, a 3-port switch featuring integrated PHYs in each of the two user ports. The driver supports the following features
INTRNline being connected to the CPU.int-gpiosbe specified in the devicetree.Hopefully, this will serve as a another example of how to employ the so far relatively sparingly used DSA API introduced in v4.4.
Given that most of the functionality introduced in this PR is entirely new, relatively few modifications are done to existing code. The only change is that Kconfig options for the NXP NETC driver are moved to a separate Kconfig file named Kconfig.netc to avoid cluttering the DSA top-level Kconfig.
The data sheet for the KSZ8463, along with other information, is available here.
I apologize for this PR being as large as it is. It's difficult to introduce changes such as these in smaller pieces.
A special thanks to Microchip for providing the evaluation kit used to develop this.