| Message ID | 20220504151755.11737-8-arun.ramadoss@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: DSA driver support for LAN937x | expand |
On Wed, May 04, 2022 at 08:47:49PM +0530, Arun Ramadoss wrote: > This patch add the SPI driver for the LAN937x switches. It uses the > lan937x_main.c and lan937x_dev.c functions. > > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> > --- > drivers/net/dsa/microchip/Makefile | 1 + > drivers/net/dsa/microchip/ksz_common.h | 1 + > drivers/net/dsa/microchip/lan937x_dev.c | 7 + > drivers/net/dsa/microchip/lan937x_spi.c | 236 ++++++++++++++++++++++++ > 4 files changed, 245 insertions(+) > create mode 100644 drivers/net/dsa/microchip/lan937x_spi.c > > diff --git a/drivers/net/dsa/microchip/Makefile b/drivers/net/dsa/microchip/Makefile > index d32ff38dc240..28d8eb62a795 100644 > --- a/drivers/net/dsa/microchip/Makefile > +++ b/drivers/net/dsa/microchip/Makefile > @@ -10,3 +10,4 @@ obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ8863_SMI) += ksz8863_smi.o > obj-$(CONFIG_NET_DSA_MICROCHIP_LAN937X) += lan937x.o > lan937x-objs := lan937x_dev.o > lan937x-objs += lan937x_main.o > +lan937x-objs += lan937x_spi.o > diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h > index 5671f580948d..fd9e0705d2d2 100644 > --- a/drivers/net/dsa/microchip/ksz_common.h > +++ b/drivers/net/dsa/microchip/ksz_common.h > @@ -151,6 +151,7 @@ void ksz_switch_remove(struct ksz_device *dev); > int ksz8_switch_register(struct ksz_device *dev); > int ksz9477_switch_register(struct ksz_device *dev); > int lan937x_switch_register(struct ksz_device *dev); > +int lan937x_check_device_id(struct ksz_device *dev); > > void ksz_update_port_member(struct ksz_device *dev, int port); > void ksz_init_mib_timer(struct ksz_device *dev); > diff --git a/drivers/net/dsa/microchip/lan937x_dev.c b/drivers/net/dsa/microchip/lan937x_dev.c > index 3f1797cc1d16..f430a8711775 100644 > --- a/drivers/net/dsa/microchip/lan937x_dev.c > +++ b/drivers/net/dsa/microchip/lan937x_dev.c > @@ -386,8 +386,15 @@ static int lan937x_mdio_register(struct ksz_device *dev) > > static int lan937x_switch_init(struct ksz_device *dev) > { > + int ret; > + > dev->ds->ops = &lan937x_switch_ops; > > + /* Check device tree */ > + ret = lan937x_check_device_id(dev); > + if (ret < 0) > + return ret; > + Can't this be called from lan937x_spi_probe() directly, why do you need to go through lan937x_switch_register() first? > dev->port_mask = (1 << dev->port_cnt) - 1; > > dev->ports = devm_kzalloc(dev->dev,
Hi Vladimir, Thanks for the comment. On Wed, 2022-05-04 at 23:07 +0300, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Wed, May 04, 2022 at 08:47:49PM +0530, Arun Ramadoss wrote: > > This patch add the SPI driver for the LAN937x switches. It uses the > > lan937x_main.c and lan937x_dev.c functions. > > > > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> > > --- > > drivers/net/dsa/microchip/Makefile | 1 + > > drivers/net/dsa/microchip/ksz_common.h | 1 + > > drivers/net/dsa/microchip/lan937x_dev.c | 7 + > > drivers/net/dsa/microchip/lan937x_spi.c | 236 > > ++++++++++++++++++++++++ > > 4 files changed, 245 insertions(+) > > create mode 100644 drivers/net/dsa/microchip/lan937x_spi.c > > > > diff --git a/drivers/net/dsa/microchip/Makefile > > b/drivers/net/dsa/microchip/Makefile > > index d32ff38dc240..28d8eb62a795 100644 > > --- a/drivers/net/dsa/microchip/Makefile > > +++ b/drivers/net/dsa/microchip/Makefile > > @@ -10,3 +10,4 @@ obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ8863_SMI) += > > ksz8863_smi.o > > obj-$(CONFIG_NET_DSA_MICROCHIP_LAN937X) += lan937x.o > > lan937x-objs := lan937x_dev.o > > lan937x-objs += lan937x_main.o > > +lan937x-objs += lan937x_spi.o > > diff --git a/drivers/net/dsa/microchip/ksz_common.h > > b/drivers/net/dsa/microchip/ksz_common.h > > index 5671f580948d..fd9e0705d2d2 100644 > > --- a/drivers/net/dsa/microchip/ksz_common.h > > +++ b/drivers/net/dsa/microchip/ksz_common.h > > @@ -151,6 +151,7 @@ void ksz_switch_remove(struct ksz_device *dev); > > int ksz8_switch_register(struct ksz_device *dev); > > int ksz9477_switch_register(struct ksz_device *dev); > > int lan937x_switch_register(struct ksz_device *dev); > > +int lan937x_check_device_id(struct ksz_device *dev); > > > > void ksz_update_port_member(struct ksz_device *dev, int port); > > void ksz_init_mib_timer(struct ksz_device *dev); > > diff --git a/drivers/net/dsa/microchip/lan937x_dev.c > > b/drivers/net/dsa/microchip/lan937x_dev.c > > index 3f1797cc1d16..f430a8711775 100644 > > --- a/drivers/net/dsa/microchip/lan937x_dev.c > > +++ b/drivers/net/dsa/microchip/lan937x_dev.c > > @@ -386,8 +386,15 @@ static int lan937x_mdio_register(struct > > ksz_device *dev) > > > > static int lan937x_switch_init(struct ksz_device *dev) > > { > > + int ret; > > + > > dev->ds->ops = &lan937x_switch_ops; > > > > + /* Check device tree */ > > + ret = lan937x_check_device_id(dev); > > + if (ret < 0) > > + return ret; > > + > > Can't this be called from lan937x_spi_probe() directly, why do you > need > to go through lan937x_switch_register() first? lan937x_check_device_id function compares the dev->chip_id with the lan937x_switch_chip array and populate the some of the parameters of struct ksz_dev. The dev->chip_id is populated using the dev->dev_ops- >detect in the ksz_switch_register function. If lan937x_check_device_id needs to be called in spi_probe, then chip_id has to be identified as part of spi_probe function. Since ksz_switch_register handles the identifying the chip_id, checking the device_id is part of switch_init. if (dev->dev_ops->detect(dev)) return -EINVAL; ret = dev->dev_ops->init(dev); if (ret) return ret; As per the comment, enable_spi_indirect_access function called twice https://lore.kernel.org/netdev/20220408232557.b62l3lksotq5vuvm@skbuf/ I have removed the enable_spi_indirect_access in the lan937x_setup function in v13 patch 6. But it actually failed our regression. The SPI indirect is required for accessing the Internal phy registers. We have enabled it in lan937x_init before registering the mdio_register. We need it for reading the phy id. And another place enabled in lan937x_setup after lan937x_switch_reset function. When I removed enabling in setup function, switch_reset disables the spi indirecting addressing. Because of that further phy register r/w fails. In Summary, we need to enable spi indirect access in both the places, one for mdio_register and another after switch_reset. Can I enable it both the places? Kindly suggest. > > > dev->port_mask = (1 << dev->port_cnt) - 1; > > > > dev->ports = devm_kzalloc(dev->dev,
On Thu, May 05, 2022 at 10:32:17AM +0000, Arun.Ramadoss@microchip.com wrote: > > > static int lan937x_switch_init(struct ksz_device *dev) > > > { > > > + int ret; > > > + > > > dev->ds->ops = &lan937x_switch_ops; > > > > > > + /* Check device tree */ > > > + ret = lan937x_check_device_id(dev); > > > + if (ret < 0) > > > + return ret; > > > + > > > > Can't this be called from lan937x_spi_probe() directly, why do you > > need > > to go through lan937x_switch_register() first? > > lan937x_check_device_id function compares the dev->chip_id with the > lan937x_switch_chip array and populate the some of the parameters of > struct ksz_dev. The dev->chip_id is populated using the dev->dev_ops- > >detect in the ksz_switch_register function. If lan937x_check_device_id > needs to be called in spi_probe, then chip_id has to be identified as > part of spi_probe function. Since ksz_switch_register handles the > identifying the chip_id, checking the device_id is part of switch_init. > > if (dev->dev_ops->detect(dev)) > return -EINVAL; > > ret = dev->dev_ops->init(dev); > if (ret) > return ret; Whatever you do, please use a common pattern for all of ksz9477, ksz8, and your lan937x. This includes validation of chip id, placement of the chip_data and dev_ops structures, and reuse as much logic as possible. The key is to limit the chip-specific information to structured data (tables) wherever possible and let common code deal with them. For example there is no reason why struct ksz_chip_data is redefined for every switch, why copying from "chip" to "dev" is duplicated for every switch, and yet, why every other switch copies from "chip" to "dev" in the "switch_init" function yet lan937x does it from "check_device_id". So much boilerplate, yet different in subtle ways, makes the code very unpleasant to review. I'm sure you'll find a straightforward way to code up a probing function. > As per the comment, enable_spi_indirect_access function called twice > https://lore.kernel.org/netdev/20220408232557.b62l3lksotq5vuvm@skbuf/ > I have removed the enable_spi_indirect_access in the lan937x_setup > function in v13 patch 6. But it actually failed our regression. > The SPI indirect is required for accessing the Internal phy registers. > We have enabled it in lan937x_init before registering the > mdio_register. We need it for reading the phy id. > And another place enabled in lan937x_setup after lan937x_switch_reset > function. When I removed enabling in setup function, switch_reset > disables the spi indirecting addressing. Because of that further phy > register r/w fails. In Summary, we need to enable spi indirect access > in both the places, one for mdio_register and another after > switch_reset. > > Can I enable it both the places? Kindly suggest. So you call lan937x_reset_switch() as the first thing in ds->ops->setup(), and this momentarily breaks the earlier MDIO bus setup done from probe -> ksz_switch_register() -> dev_ops->init(). So why don't you move the lan937x_enable_spi_indirect_access() and lan937x_mdio_register() calls to ds->ops->setup(), _after_ the switch soft reset, then?
diff --git a/drivers/net/dsa/microchip/Makefile b/drivers/net/dsa/microchip/Makefile index d32ff38dc240..28d8eb62a795 100644 --- a/drivers/net/dsa/microchip/Makefile +++ b/drivers/net/dsa/microchip/Makefile @@ -10,3 +10,4 @@ obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ8863_SMI) += ksz8863_smi.o obj-$(CONFIG_NET_DSA_MICROCHIP_LAN937X) += lan937x.o lan937x-objs := lan937x_dev.o lan937x-objs += lan937x_main.o +lan937x-objs += lan937x_spi.o diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h index 5671f580948d..fd9e0705d2d2 100644 --- a/drivers/net/dsa/microchip/ksz_common.h +++ b/drivers/net/dsa/microchip/ksz_common.h @@ -151,6 +151,7 @@ void ksz_switch_remove(struct ksz_device *dev); int ksz8_switch_register(struct ksz_device *dev); int ksz9477_switch_register(struct ksz_device *dev); int lan937x_switch_register(struct ksz_device *dev); +int lan937x_check_device_id(struct ksz_device *dev); void ksz_update_port_member(struct ksz_device *dev, int port); void ksz_init_mib_timer(struct ksz_device *dev); diff --git a/drivers/net/dsa/microchip/lan937x_dev.c b/drivers/net/dsa/microchip/lan937x_dev.c index 3f1797cc1d16..f430a8711775 100644 --- a/drivers/net/dsa/microchip/lan937x_dev.c +++ b/drivers/net/dsa/microchip/lan937x_dev.c @@ -386,8 +386,15 @@ static int lan937x_mdio_register(struct ksz_device *dev) static int lan937x_switch_init(struct ksz_device *dev) { + int ret; + dev->ds->ops = &lan937x_switch_ops; + /* Check device tree */ + ret = lan937x_check_device_id(dev); + if (ret < 0) + return ret; + dev->port_mask = (1 << dev->port_cnt) - 1; dev->ports = devm_kzalloc(dev->dev, diff --git a/drivers/net/dsa/microchip/lan937x_spi.c b/drivers/net/dsa/microchip/lan937x_spi.c new file mode 100644 index 000000000000..a50dfcf27aff --- /dev/null +++ b/drivers/net/dsa/microchip/lan937x_spi.c @@ -0,0 +1,236 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Microchip LAN937X switch driver register access through SPI + * Copyright (C) 2019-2021 Microchip Technology Inc. + */ +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/spi/spi.h> +#include <linux/of_device.h> + +#include "ksz_common.h" + +#define SPI_ADDR_SHIFT 24 +#define SPI_ADDR_ALIGN 3 +#define SPI_TURNAROUND_SHIFT 5 + +KSZ_REGMAP_TABLE(lan937x, 32, SPI_ADDR_SHIFT, SPI_TURNAROUND_SHIFT, + SPI_ADDR_ALIGN); + +struct lan937x_chip_data { + u32 chip_id; + const char *dev_name; + int num_vlans; + int num_alus; + int num_statics; + int cpu_ports; + int port_cnt; +}; + +static const struct of_device_id lan937x_dt_ids[]; + +static const struct lan937x_chip_data lan937x_switch_chips[] = { + { + .chip_id = 0x00937010, + .dev_name = "LAN9370", + .num_vlans = 4096, + .num_alus = 1024, + .num_statics = 256, + /* can be configured as cpu port */ + .cpu_ports = 0x10, + /* total port count */ + .port_cnt = 5, + }, + { + .chip_id = 0x00937110, + .dev_name = "LAN9371", + .num_vlans = 4096, + .num_alus = 1024, + .num_statics = 256, + /* can be configured as cpu port */ + .cpu_ports = 0x30, + /* total port count */ + .port_cnt = 6, + }, + { + .chip_id = 0x00937210, + .dev_name = "LAN9372", + .num_vlans = 4096, + .num_alus = 1024, + .num_statics = 256, + /* can be configured as cpu port */ + .cpu_ports = 0x30, + /* total port count */ + .port_cnt = 8, + }, + { + .chip_id = 0x00937310, + .dev_name = "LAN9373", + .num_vlans = 4096, + .num_alus = 1024, + .num_statics = 256, + /* can be configured as cpu port */ + .cpu_ports = 0x38, + /* total port count */ + .port_cnt = 5, + }, + { + .chip_id = 0x00937410, + .dev_name = "LAN9374", + .num_vlans = 4096, + .num_alus = 1024, + .num_statics = 256, + /* can be configured as cpu port */ + .cpu_ports = 0x30, + /* total port count */ + .port_cnt = 8, + }, +}; + +static int lan937x_spi_probe(struct spi_device *spi) +{ + struct regmap_config rc; + struct ksz_device *dev; + int i, ret; + + dev = ksz_switch_alloc(&spi->dev, spi); + if (!dev) + return -ENOMEM; + + for (i = 0; i < ARRAY_SIZE(lan937x_regmap_config); i++) { + rc = lan937x_regmap_config[i]; + rc.lock_arg = &dev->regmap_mutex; + dev->regmap[i] = devm_regmap_init_spi(spi, &rc); + + if (IS_ERR(dev->regmap[i])) { + ret = PTR_ERR(dev->regmap[i]); + dev_err(&spi->dev, + "Failed to initialize regmap%i: %d\n", + lan937x_regmap_config[i].val_bits, ret); + return ret; + } + } + + if (spi->dev.platform_data) + dev->pdata = spi->dev.platform_data; + + ret = lan937x_switch_register(dev); + /* Main DSA driver may not be started yet. */ + if (ret) + return ret; + + spi_set_drvdata(spi, dev); + + return 0; +} + +int lan937x_check_device_id(struct ksz_device *dev) +{ + const struct lan937x_chip_data *dt_chip_data; + const struct of_device_id *match; + int i; + + dt_chip_data = of_device_get_match_data(dev->dev); + + if (!dt_chip_data) + return -EINVAL; + + for (match = lan937x_dt_ids; match->compatible[0]; match++) { + const struct lan937x_chip_data *chip_data = match->data; + + /* Check for chip id */ + if (chip_data->chip_id != dev->chip_id) + continue; + + /* Check for Device Tree and Chip ID */ + if (dt_chip_data->chip_id != dev->chip_id) { + dev_err(dev->dev, + "Device tree specifies chip %s but found %s, please fix it!\n", + dt_chip_data->dev_name, chip_data->dev_name); + return -ENODEV; + } + + break; + } + + for (i = 0; i < ARRAY_SIZE(lan937x_switch_chips); i++) { + const struct lan937x_chip_data *chip = &lan937x_switch_chips[i]; + + if (dev->chip_id == chip->chip_id) { + dev->name = chip->dev_name; + dev->num_vlans = chip->num_vlans; + dev->num_alus = chip->num_alus; + dev->num_statics = chip->num_statics; + dev->port_cnt = chip->port_cnt; + dev->cpu_ports = chip->cpu_ports; + break; + } + } + + /* no switch found */ + if (!dev->port_cnt) + return -ENODEV; + + return 0; +} +EXPORT_SYMBOL(lan937x_check_device_id); + +static void lan937x_spi_remove(struct spi_device *spi) +{ + struct ksz_device *dev = spi_get_drvdata(spi); + + if (dev) + ksz_switch_remove(dev); + + spi_set_drvdata(spi, NULL); +} + +static void lan937x_spi_shutdown(struct spi_device *spi) +{ + struct ksz_device *dev = spi_get_drvdata(spi); + + if (dev) + dsa_switch_shutdown(dev->ds); + + spi_set_drvdata(spi, NULL); +} + +static const struct of_device_id lan937x_dt_ids[] = { + { .compatible = "microchip,lan9370", .data = &lan937x_switch_chips[0] }, + { .compatible = "microchip,lan9371", .data = &lan937x_switch_chips[1] }, + { .compatible = "microchip,lan9372", .data = &lan937x_switch_chips[2] }, + { .compatible = "microchip,lan9373", .data = &lan937x_switch_chips[3] }, + { .compatible = "microchip,lan9374", .data = &lan937x_switch_chips[4] }, + {}, +}; +MODULE_DEVICE_TABLE(of, lan937x_dt_ids); + +static const struct spi_device_id lan937x_spi_ids[] = { + { .name = "lan9370" }, + { .name = "lan9371" }, + { .name = "lan9372" }, + { .name = "lan9373" }, + { .name = "lan9374" }, + {}, +}; +MODULE_DEVICE_TABLE(spi, lan937x_spi_ids); + +static struct spi_driver lan937x_spi_driver = { + .driver = { + .name = "lan937x-switch", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(lan937x_dt_ids), + }, + .probe = lan937x_spi_probe, + .remove = lan937x_spi_remove, + .shutdown = lan937x_spi_shutdown, + .id_table = lan937x_spi_ids, +}; + +module_spi_driver(lan937x_spi_driver); + +MODULE_ALIAS("spi:lan937x"); + +MODULE_AUTHOR("Prasanna Vengateshan Varadharajan <Prasanna.Vengateshan@microchip.com>"); +MODULE_DESCRIPTION("Microchip LAN937x Series Switch SPI access Driver"); +MODULE_LICENSE("GPL");
This patch add the SPI driver for the LAN937x switches. It uses the lan937x_main.c and lan937x_dev.c functions. Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> --- drivers/net/dsa/microchip/Makefile | 1 + drivers/net/dsa/microchip/ksz_common.h | 1 + drivers/net/dsa/microchip/lan937x_dev.c | 7 + drivers/net/dsa/microchip/lan937x_spi.c | 236 ++++++++++++++++++++++++ 4 files changed, 245 insertions(+) create mode 100644 drivers/net/dsa/microchip/lan937x_spi.c