| Message ID | 20210920095218.1108151-1-horatiu.vultur@microchip.com |
|---|---|
| Headers | show |
| Series | Add lan966x driver | expand |
Hi, On 20/09/2021 11:52:08+0200, Horatiu Vultur wrote: > The LAN8804 SKY has same features as that of LAN8804 SKY except that it On of those part name should be different ;) > doesn't support 1588, SyncE or Q-USGMII. > > This PHY is found inside the LAN966X switches. > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > --- > drivers/net/phy/micrel.c | 73 ++++++++++++++++++++++++++++++++++++++ > include/linux/micrel_phy.h | 1 + > 2 files changed, 74 insertions(+) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 5c928f827173..34800b547004 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1537,6 +1537,65 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev, > return ret; > } > > +#define LAN_EXT_PAGE_ACCESS_CONTROL 0x16 > +#define LAN_EXT_PAGE_ACCESS_ADDRESS_DATA 0x17 > +#define LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC 0x4000 > + > +#define LAN8804_ALIGN_SWAP 0x4a > +#define LAN8804_ALIGN_TX_A_B_SWAP 0x1 > +#define LAN8804_ALIGN_TX_A_B_SWAP_MASK GENMASK(2, 0) > +#define LAN8814_CLOCK_MANAGEMENT 0xd > +#define LAN8814_LINK_QUALITY 0x8e > + > +static int lanphy_read_page_reg(struct phy_device *phydev, int page, u32 addr) > +{ > + u32 data; > + > + phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page); > + phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr); > + phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, > + (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC)); > + data = phy_read(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA); > + > + return data; > +} > + > +static int lanphy_write_page_reg(struct phy_device *phydev, int page, u16 addr, > + u16 val) > +{ > + phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page); > + phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr); > + phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, > + (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC)); > + > + val = phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, val); > + if (val) { > + phydev_err(phydev, "Error: phy_write has returned error %d\n", > + val); > + return val; > + } > + return 0; > +} > + > +static int lan8804_config_init(struct phy_device *phydev) > +{ > + int val; > + > + /* MDI-X setting for swap A,B transmit */ > + val = lanphy_read_page_reg(phydev, 2, LAN8804_ALIGN_SWAP); > + val &= ~LAN8804_ALIGN_TX_A_B_SWAP_MASK; > + val |= LAN8804_ALIGN_TX_A_B_SWAP; > + lanphy_write_page_reg(phydev, 2, LAN8804_ALIGN_SWAP, val); > + > + /* Make sure that the PHY will not stop generating the clock when the > + * link partner goes down > + */ > + lanphy_write_page_reg(phydev, 31, LAN8814_CLOCK_MANAGEMENT, 0x27e); > + lanphy_read_page_reg(phydev, 1, LAN8814_LINK_QUALITY); > + > + return 0; > +} > + > static struct phy_driver ksphy_driver[] = { > { > .phy_id = PHY_ID_KS8737, > @@ -1718,6 +1777,20 @@ static struct phy_driver ksphy_driver[] = { > .get_stats = kszphy_get_stats, > .suspend = genphy_suspend, > .resume = kszphy_resume, > +}, { > + .phy_id = PHY_ID_LAN8804, > + .phy_id_mask = MICREL_PHY_ID_MASK, > + .name = "Microchip LAN966X Gigabit PHY", > + .config_init = lan8804_config_init, > + .driver_data = &ksz9021_type, > + .probe = kszphy_probe, > + .soft_reset = genphy_soft_reset, > + .read_status = ksz9031_read_status, > + .get_sset_count = kszphy_get_sset_count, > + .get_strings = kszphy_get_strings, > + .get_stats = kszphy_get_stats, > + .suspend = genphy_suspend, > + .resume = kszphy_resume, > }, { > .phy_id = PHY_ID_KSZ9131, > .phy_id_mask = MICREL_PHY_ID_MASK, > diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h > index 3d43c60b49fa..1f7c33b2f5a3 100644 > --- a/include/linux/micrel_phy.h > +++ b/include/linux/micrel_phy.h > @@ -28,6 +28,7 @@ > #define PHY_ID_KSZ9031 0x00221620 > #define PHY_ID_KSZ9131 0x00221640 > #define PHY_ID_LAN8814 0x00221660 > +#define PHY_ID_LAN8804 0x00221670 > > #define PHY_ID_KSZ886X 0x00221430 > #define PHY_ID_KSZ8863 0x00221435 > -- > 2.31.1 >
On Mon, Sep 20, 2021 at 11:52:07AM +0200, Horatiu Vultur wrote: > According to the documentation the second resource is optional. But the > blamed commit ignores that and if the resource is not there it just > fails. > > This patch reverts that to still allow the second resource to be > optional because other SoC have the some MDIO controller and doesn't > need to second resource. > > Fixes: 672a1c394950 ("net: mdio: mscc-miim: Make use of the helper function devm_platform_ioremap_resource()") > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> Hi Moratiu The script kiddies might come long and 'fix' this again. Maybe consider adding devm_platform_ioremap_resource_optional(), following the pattern of other _optional() API calls. Otherwise add a comment. Andrew
On Mon, Sep 20, 2021 at 11:52:08AM +0200, Horatiu Vultur wrote: > The LAN8804 SKY has same features as that of LAN8804 SKY except that it > doesn't support 1588, SyncE or Q-USGMII. > > This PHY is found inside the LAN966X switches. Please add the new PHY to the micrel_tbl[]. Andrew
On Mon, Sep 20, 2021 at 11:52:11AM +0200, Horatiu Vultur wrote: > The lan966x switch SoC has a number of components that can be reset > indiviually, but at least the switch core needs to be in a well defined > state at power on, when any of the lan966x drivers starts to access the > switch core, this reset driver is available. > > The reset driver is loaded early via the postcore_initcall interface, and > will then be available for the other lan966x drivers (SGPIO, SwitchDev etc) > that are loaded next, and the first of them to be loaded can perform the > one-time switch core reset that is needed. A lot of this looks very similar to reset-microchip-sparx5.c. PROTECT_REG is 0x88 rather than 0x84, but actually using the value is the same. SOFT_RESET_REG is identical. So rather than adding a new driver, maybe you can generalize reset-microchip-sparx5.c, and add a second compatible string? Andrew
> +struct lan966x_reset_context { > + struct regmap *gcb_ctrl; > + struct regmap *cpu_ctrl; > + struct notifier_block restart_handler; > +}; > + > +#define PROTECT_REG 0x88 > +#define PROTECT_BIT BIT(5) > +#define SOFT_RESET_REG 0x00 > +#define SOFT_RESET_BIT BIT(1) > + > +static int lan966x_restart_handle(struct notifier_block *this, > + unsigned long mode, void *cmd) > +{ > + struct lan966x_reset_context *ctx = container_of(this, struct lan966x_reset_context, > + restart_handler); > + > + /* Make sure the core is not protected from reset */ > + regmap_update_bits(ctx->cpu_ctrl, PROTECT_REG, PROTECT_BIT, 0); This all looks familiar... Maybe yet another compatible added to reset-microchip-sparx5.c ? Andrew
On Mon, Sep 20, 2021 at 11:52:15AM +0200, Horatiu Vultur wrote: > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > new file mode 100644 > index 000000000000..2984f510ae27 > --- /dev/null > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > @@ -0,0 +1,350 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +#include <asm/memory.h> > +#include <linux/module.h> > +#include <linux/if_bridge.h> > +#include <linux/iopoll.h> > +#include <linux/of_platform.h> > +#include <linux/of_net.h> > +#include <linux/reset.h> > + > +#include "lan966x_main.h" > + > +#define READL_SLEEP_US 10 > +#define READL_TIMEOUT_US 100000000 > + > +#define IO_RANGES 2 > + > +static const struct of_device_id lan966x_match[] = { > + { .compatible = "microchip,lan966x-switch" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, mchp_lan966x_match); > + > +struct lan966x_main_io_resource { > + enum lan966x_target id; > + phys_addr_t offset; > + int range; > +}; > + > +static const struct lan966x_main_io_resource lan966x_main_iomap[] = { > + { TARGET_CPU, 0xc0000, 0 }, /* 0xe00c0000 */ > + { TARGET_ORG, 0, 1 }, /* 0xe2000000 */ > + { TARGET_GCB, 0x4000, 1 }, /* 0xe2004000 */ > + { TARGET_QS, 0x8000, 1 }, /* 0xe2008000 */ > + { TARGET_CHIP_TOP, 0x10000, 1 }, /* 0xe2010000 */ > + { TARGET_REW, 0x14000, 1 }, /* 0xe2014000 */ > + { TARGET_SYS, 0x28000, 1 }, /* 0xe2028000 */ > + { TARGET_HSIO, 0x2c000, 1 }, /* 0xe202c000 */ > + { TARGET_DEV, 0x34000, 1 }, /* 0xe2034000 */ > + { TARGET_DEV + 1, 0x38000, 1 }, /* 0xe2038000 */ > + { TARGET_DEV + 2, 0x3c000, 1 }, /* 0xe203c000 */ > + { TARGET_DEV + 3, 0x40000, 1 }, /* 0xe2040000 */ > + { TARGET_DEV + 4, 0x44000, 1 }, /* 0xe2044000 */ > + { TARGET_DEV + 5, 0x48000, 1 }, /* 0xe2048000 */ > + { TARGET_DEV + 6, 0x4c000, 1 }, /* 0xe204c000 */ > + { TARGET_DEV + 7, 0x50000, 1 }, /* 0xe2050000 */ > + { TARGET_QSYS, 0x100000, 1 }, /* 0xe2100000 */ > + { TARGET_AFI, 0x120000, 1 }, /* 0xe2120000 */ > + { TARGET_ANA, 0x140000, 1 }, /* 0xe2140000 */ > +}; > + > +static int lan966x_create_targets(struct platform_device *pdev, > + struct lan966x *lan966x) > +{ > + struct resource *iores[IO_RANGES]; > + void __iomem *iomem[IO_RANGES]; > + void __iomem *begin[IO_RANGES]; > + int idx; > + > + for (idx = 0; idx < IO_RANGES; idx++) { > + iores[idx] = platform_get_resource(pdev, IORESOURCE_MEM, > + idx); > + iomem[idx] = devm_ioremap(&pdev->dev, > + iores[idx]->start, > + resource_size(iores[idx])); This is buggy. If platform_get_resource() returns NULL, you will oops the kernel. In any case, this code will be ripe for janitor patching. Please consider using devm_platform_ioremap_resource() now, before someone converts your code to use this function.
On Mon, Sep 20, 2021 at 11:52:16AM +0200, Horatiu Vultur wrote: > +static void lan966x_cleanup_ports(struct lan966x *lan966x) > +{ > + struct lan966x_port *port; > + int portno; > + > + for (portno = 0; portno < lan966x->num_phys_ports; portno++) { > + port = lan966x->ports[portno]; > + if (!port) > + continue; > + > + if (port->phylink) { > + rtnl_lock(); > + lan966x_port_stop(port->dev); > + rtnl_unlock(); > + port->phylink = NULL; This leaks the phylink structure. You need to call phylink_destroy(). > static int lan966x_probe_port(struct lan966x *lan966x, u8 port, > phy_interface_t phy_mode) > { > struct lan966x_port *lan966x_port; > + struct phylink *phylink; > + struct net_device *dev; > + int err; > > if (port >= lan966x->num_phys_ports) > return -EINVAL; > > - lan966x_port = devm_kzalloc(lan966x->dev, sizeof(*lan966x_port), > - GFP_KERNEL); > + dev = devm_alloc_etherdev_mqs(lan966x->dev, > + sizeof(struct lan966x_port), 8, 1); > + if (!dev) > + return -ENOMEM; > > + SET_NETDEV_DEV(dev, lan966x->dev); > + lan966x_port = netdev_priv(dev); > + lan966x_port->dev = dev; > lan966x_port->lan966x = lan966x; > lan966x_port->chip_port = port; > lan966x_port->pvid = PORT_PVID; > lan966x->ports[port] = lan966x_port; > > + dev->max_mtu = ETH_MAX_MTU; > + > + dev->netdev_ops = &lan966x_port_netdev_ops; > + dev->needed_headroom = IFH_LEN * sizeof(u32); > + > + err = register_netdev(dev); > + if (err) { > + dev_err(lan966x->dev, "register_netdev failed\n"); > + goto err_register_netdev; > + } register_netdev() publishes the network device. > + > + lan966x_port->phylink_config.dev = &lan966x_port->dev->dev; > + lan966x_port->phylink_config.type = PHYLINK_NETDEV; > + lan966x_port->phylink_config.pcs_poll = true; > + > + phylink = phylink_create(&lan966x_port->phylink_config, > + lan966x_port->fwnode, > + phy_mode, > + &lan966x_phylink_mac_ops); phylink_create() should always be called _prior_ to the network device being published. In any case... > + if (IS_ERR(phylink)) > + return PTR_ERR(phylink); If this fails, this function returns an error, but leaves the network device published - which is a bug in itself. > +static void lan966x_phylink_mac_link_down(struct phylink_config *config, > + unsigned int mode, > + phy_interface_t interface) > +{ Hmm? Shouldn't this do something?
On Mon, 20 Sep 2021 11:52:18 +0200 Horatiu Vultur wrote: > +static void lan966x_get_eth_mac_stats(struct net_device *dev, > + struct ethtool_eth_mac_stats *mac_stats) Great to see this API used, please also consider adding RMON stats since the seem to be present.
The 09/20/2021 13:52, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Sep 20, 2021 at 11:52:07AM +0200, Horatiu Vultur wrote: > > According to the documentation the second resource is optional. But the > > blamed commit ignores that and if the resource is not there it just > > fails. > > > > This patch reverts that to still allow the second resource to be > > optional because other SoC have the some MDIO controller and doesn't > > need to second resource. > > > > Fixes: 672a1c394950 ("net: mdio: mscc-miim: Make use of the helper function devm_platform_ioremap_resource()") > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> Hi Andrew, > > Hi Moratiu > > The script kiddies might come long and 'fix' this again. Maybe > consider adding devm_platform_ioremap_resource_optional(), following > the pattern of other _optional() API calls. Otherwise add a comment. Initially I think I will go with the comment. Because this patch actually needs to go on net. > > Andrew
The 09/20/2021 13:59, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Sep 20, 2021 at 11:52:08AM +0200, Horatiu Vultur wrote: > > The LAN8804 SKY has same features as that of LAN8804 SKY except that it > > doesn't support 1588, SyncE or Q-USGMII. > > > > This PHY is found inside the LAN966X switches. > > Please add the new PHY to the micrel_tbl[]. Will do that. Thanks > > Andrew >
The 09/20/2021 14:11, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Sep 20, 2021 at 11:52:11AM +0200, Horatiu Vultur wrote: > > The lan966x switch SoC has a number of components that can be reset > > indiviually, but at least the switch core needs to be in a well defined > > state at power on, when any of the lan966x drivers starts to access the > > switch core, this reset driver is available. > > > > The reset driver is loaded early via the postcore_initcall interface, and > > will then be available for the other lan966x drivers (SGPIO, SwitchDev etc) > > that are loaded next, and the first of them to be loaded can perform the > > one-time switch core reset that is needed. > > A lot of this looks very similar to > reset-microchip-sparx5.c. PROTECT_REG is 0x88 rather than 0x84, but > actually using the value is the same. SOFT_RESET_REG is identical. So > rather than adding a new driver, maybe you can generalize > reset-microchip-sparx5.c, and add a second compatible string? You are right, they look similar. I will try to add a new compatible string. > > Andrew
The 09/20/2021 14:54, Russell King (Oracle) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe Hi Russell, > > On Mon, Sep 20, 2021 at 11:52:16AM +0200, Horatiu Vultur wrote: > > +static void lan966x_cleanup_ports(struct lan966x *lan966x) > > +{ > > + struct lan966x_port *port; > > + int portno; > > + > > + for (portno = 0; portno < lan966x->num_phys_ports; portno++) { > > + port = lan966x->ports[portno]; > > + if (!port) > > + continue; > > + > > + if (port->phylink) { > > + rtnl_lock(); > > + lan966x_port_stop(port->dev); > > + rtnl_unlock(); > > + port->phylink = NULL; > > This leaks the phylink structure. You need to call phylink_destroy(). > > > static int lan966x_probe_port(struct lan966x *lan966x, u8 port, > > phy_interface_t phy_mode) > > { > > struct lan966x_port *lan966x_port; > > + struct phylink *phylink; > > + struct net_device *dev; > > + int err; > > > > if (port >= lan966x->num_phys_ports) > > return -EINVAL; > > > > - lan966x_port = devm_kzalloc(lan966x->dev, sizeof(*lan966x_port), > > - GFP_KERNEL); > > + dev = devm_alloc_etherdev_mqs(lan966x->dev, > > + sizeof(struct lan966x_port), 8, 1); > > + if (!dev) > > + return -ENOMEM; > > > > + SET_NETDEV_DEV(dev, lan966x->dev); > > + lan966x_port = netdev_priv(dev); > > + lan966x_port->dev = dev; > > lan966x_port->lan966x = lan966x; > > lan966x_port->chip_port = port; > > lan966x_port->pvid = PORT_PVID; > > lan966x->ports[port] = lan966x_port; > > > > + dev->max_mtu = ETH_MAX_MTU; > > + > > + dev->netdev_ops = &lan966x_port_netdev_ops; > > + dev->needed_headroom = IFH_LEN * sizeof(u32); > > + > > + err = register_netdev(dev); > > + if (err) { > > + dev_err(lan966x->dev, "register_netdev failed\n"); > > + goto err_register_netdev; > > + } > > register_netdev() publishes the network device. > > > + > > + lan966x_port->phylink_config.dev = &lan966x_port->dev->dev; > > + lan966x_port->phylink_config.type = PHYLINK_NETDEV; > > + lan966x_port->phylink_config.pcs_poll = true; > > + > > + phylink = phylink_create(&lan966x_port->phylink_config, > > + lan966x_port->fwnode, > > + phy_mode, > > + &lan966x_phylink_mac_ops); > > phylink_create() should always be called _prior_ to the network device > being published. In any case... > > > + if (IS_ERR(phylink)) > > + return PTR_ERR(phylink); > > If this fails, this function returns an error, but leaves the network > device published - which is a bug in itself. If this fails it should eventually call lan966x_cleaup_ports where the net_device will be unregister. But first I will need to make phylink_create() be called prior the network device. > > > +static void lan966x_phylink_mac_link_down(struct phylink_config *config, > > + unsigned int mode, > > + phy_interface_t interface) > > +{ > > Hmm? Shouldn't this do something? I don't think I need to do anything here. The current setup is that there is a PHY in front of the MAC. So when the link partner goes down, the PHY will go down and the MAC will still be up. Is this a problem? When we force the port to be set down, then in the function lan966x_port_stop we actually shutdown the port. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!