| Message ID | 8730960.DhLy2TFWpf@tool (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/2] power:reset: add driver for LinkStation power off | expand |
[please always add the maintainer to Cc or To, in this case sre@kernel.org] Hi, On Sat, Apr 25, 2020 at 03:00:43AM +0200, Daniel González Cabanelas wrote: > Some Buffalo LinkStations perform the power off operation, at restart > time, depending on the state of an output pin (LED) at the ethernet PHY. > > The driver is required by the Buffalo LinkStation LS421DE (ARM MVEBU), > and other models. Without it, the board remains forever halted if a > power off command is executed, unless the PSU is disconnected and > connected again. > > Signed-off-by: Daniel González Cabanelas <dgcbueu@gmail.com> > Reviewed-by: Álvaro Fernández Rojas <noltari@gmail.com> > --- Thanks for the patch. Looks like that hardware was designed by creative hardware engineers trying to impress software engineers with weird designs :) > drivers/power/reset/Kconfig | 10 ++ > drivers/power/reset/Makefile | 1 + > drivers/power/reset/linkstation-poweroff.c | 117 +++++++++++++++++++++ > 3 files changed, 128 insertions(+) > create mode 100644 drivers/power/reset/linkstation-poweroff.c > > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig > index 8903803020..de948835f8 100644 > --- a/drivers/power/reset/Kconfig > +++ b/drivers/power/reset/Kconfig > @@ -203,6 +203,16 @@ config POWER_RESET_KEYSTONE > help > Reboot support for the KEYSTONE SoCs. > > +config POWER_RESET_LINKSTATION > + tristate "Buffalo LinkStation power-off driver" > + depends on ARCH_MVEBU || COMPILE_TEST > + depends on OF_MDIO && PHYLIB > + help > + Some Buffalo LinkStations use a LED output at the ethernet PHY > + to inform the board that a power off operation must be performed > + after restarting. This driver sets the LED to the proper state > + for restarting or powering off. > + > config POWER_RESET_SYSCON > bool "Generic SYSCON regmap reset driver" > depends on OF > diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile > index da37f8b851..a8c83d2470 100644 > --- a/drivers/power/reset/Makefile > +++ b/drivers/power/reset/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_POWER_RESET_GEMINI_POWEROFF) += gemini-poweroff.o > obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o > obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o > obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o > +obj-${CONFIG_POWER_RESET_LINKSTATION} += linkstation-poweroff.o > obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o > obj-$(CONFIG_POWER_RESET_MT6323) += mt6323-poweroff.o > obj-$(CONFIG_POWER_RESET_QCOM_PON) += qcom-pon.o > diff --git a/drivers/power/reset/linkstation-poweroff.c b/drivers/power/reset/linkstation-poweroff.c > new file mode 100644 > index 0000000000..b5a4cc2c2b > --- /dev/null > +++ b/drivers/power/reset/linkstation-poweroff.c > @@ -0,0 +1,117 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * LinkStation power off restart driver > + * Copyright (C) 2020 Daniel González Cabanelas <dgcbueu@gmail.com> > + */ > + > +#include <linux/module.h> > +#include <linux/notifier.h> > +#include <linux/of.h> > +#include <linux/of_mdio.h> > +#include <linux/of_platform.h> > +#include <linux/reboot.h> > +#include <linux/phy.h> > + > +#define MII_MARVELL_LED_PAGE 3 > +#define MII_PHY_LED_CTRL 16 > + > +#define LED2_OFF (0x8 << 8) > +#define LED2_ON (0x9 << 8) > +#define LEDMASK GENMASK(11,8) > + > +static struct phy_device *phydev; > + > +static void mvphy_reg_led(u16 data) > +{ > + int rc; > + rc = phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, > + MII_PHY_LED_CTRL, LEDMASK, data); > + if (rc < 0) > + dev_err(&phydev->mdio.dev, > + "LED2 write register failed, %d\n", rc); > +} > + > +static int linkstation_reboot_notifier(struct notifier_block *nb, > + unsigned long action, void *unused) > +{ > + if (action == SYS_RESTART) > + mvphy_reg_led(LED2_ON); > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block linkstation_reboot_nb = { > + .notifier_call = linkstation_reboot_notifier, > +}; > + > +static void linkstation_poweroff(void) > +{ > + unregister_reboot_notifier(&linkstation_reboot_nb); > + mvphy_reg_led(LED2_OFF); > + > + kernel_restart("Power off"); > +} > + > +static int linkstation_poweroff_probe(struct platform_device *pdev) > +{ > + struct mii_bus *bus; > + struct device_node *dn; > + > + dn = of_parse_phandle(pdev->dev.of_node, "phy-handle,led", 0); > + > + if (dn) { > + phydev = of_phy_find_device(dn); > + of_node_put(dn); > + } else { > + dn = of_find_node_by_name(NULL, "mdio"); > + if (!dn) > + return -ENODEV; > + > + bus = of_mdio_find_bus(dn); > + of_node_put(dn); > + if (!bus) > + return -EPROBE_DEFER; > + > + phydev = phy_find_first(bus); do we need this code, or can we just make phy-handle,led mandatory and error out otherwise? > + } > + > + if (!phydev) > + return -ENODEV; Should this return -EPROBE_DEFER? > + register_reboot_notifier(&linkstation_reboot_nb); > + pm_power_off = linkstation_poweroff; > + > + dev_info(&pdev->dev, "PHY [%s]\n", phydev_name(phydev)); I don't think this deserves more than dev_dbg() > + return 0; > +} > + > +static int linkstation_poweroff_remove(struct platform_device *pdev) > +{ > + pm_power_off = NULL; > + unregister_reboot_notifier(&linkstation_reboot_nb); > + > + return 0; > +} > + > +static const struct of_device_id ls_poweroff_of_match[] = { > + { .compatible = "linkstation,power-off", }, > + { }, > +}; > + > +MODULE_DEVICE_TABLE(of, ls_poweroff_of_match); > + > +static struct platform_driver linkstation_poweroff_driver = { > + .probe = linkstation_poweroff_probe, > + .remove = linkstation_poweroff_remove, > + .driver = { > + .name = "linkstation_power_off", > + .of_match_table = ls_poweroff_of_match, > + }, > +}; > + > +module_platform_driver(linkstation_poweroff_driver); > + > +MODULE_AUTHOR("Daniel González Cabanelas <dgcbueu@gmail.com>"); > +MODULE_DESCRIPTION("LinkStation power off driver"); > +MODULE_LICENSE("GPL v2"); Otherwise looks good to me, -- Sebastian
Hi Sebastian. I''ll apply your suggestions. Thank you Daniel El jue., 30 abr. 2020 a las 0:53, Sebastian Reichel (<sebastian.reichel@collabora.com>) escribió: > > [please always add the maintainer to Cc or To, in this case sre@kernel.org] > > Hi, > > On Sat, Apr 25, 2020 at 03:00:43AM +0200, Daniel González Cabanelas wrote: > > Some Buffalo LinkStations perform the power off operation, at restart > > time, depending on the state of an output pin (LED) at the ethernet PHY. > > > > The driver is required by the Buffalo LinkStation LS421DE (ARM MVEBU), > > and other models. Without it, the board remains forever halted if a > > power off command is executed, unless the PSU is disconnected and > > connected again. > > > > Signed-off-by: Daniel González Cabanelas <dgcbueu@gmail.com> > > Reviewed-by: Álvaro Fernández Rojas <noltari@gmail.com> > > --- > > Thanks for the patch. Looks like that hardware was designed by > creative hardware engineers trying to impress software engineers > with weird designs :) > > > drivers/power/reset/Kconfig | 10 ++ > > drivers/power/reset/Makefile | 1 + > > drivers/power/reset/linkstation-poweroff.c | 117 +++++++++++++++++++++ > > 3 files changed, 128 insertions(+) > > create mode 100644 drivers/power/reset/linkstation-poweroff.c > > > > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig > > index 8903803020..de948835f8 100644 > > --- a/drivers/power/reset/Kconfig > > +++ b/drivers/power/reset/Kconfig > > @@ -203,6 +203,16 @@ config POWER_RESET_KEYSTONE > > help > > Reboot support for the KEYSTONE SoCs. > > > > +config POWER_RESET_LINKSTATION > > + tristate "Buffalo LinkStation power-off driver" > > + depends on ARCH_MVEBU || COMPILE_TEST > > + depends on OF_MDIO && PHYLIB > > + help > > + Some Buffalo LinkStations use a LED output at the ethernet PHY > > + to inform the board that a power off operation must be performed > > + after restarting. This driver sets the LED to the proper state > > + for restarting or powering off. > > + > > config POWER_RESET_SYSCON > > bool "Generic SYSCON regmap reset driver" > > depends on OF > > diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile > > index da37f8b851..a8c83d2470 100644 > > --- a/drivers/power/reset/Makefile > > +++ b/drivers/power/reset/Makefile > > @@ -10,6 +10,7 @@ obj-$(CONFIG_POWER_RESET_GEMINI_POWEROFF) += gemini-poweroff.o > > obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o > > obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o > > obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o > > +obj-${CONFIG_POWER_RESET_LINKSTATION} += linkstation-poweroff.o > > obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o > > obj-$(CONFIG_POWER_RESET_MT6323) += mt6323-poweroff.o > > obj-$(CONFIG_POWER_RESET_QCOM_PON) += qcom-pon.o > > diff --git a/drivers/power/reset/linkstation-poweroff.c b/drivers/power/reset/linkstation-poweroff.c > > new file mode 100644 > > index 0000000000..b5a4cc2c2b > > --- /dev/null > > +++ b/drivers/power/reset/linkstation-poweroff.c > > @@ -0,0 +1,117 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * LinkStation power off restart driver > > + * Copyright (C) 2020 Daniel González Cabanelas <dgcbueu@gmail.com> > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/notifier.h> > > +#include <linux/of.h> > > +#include <linux/of_mdio.h> > > +#include <linux/of_platform.h> > > +#include <linux/reboot.h> > > +#include <linux/phy.h> > > + > > +#define MII_MARVELL_LED_PAGE 3 > > +#define MII_PHY_LED_CTRL 16 > > + > > +#define LED2_OFF (0x8 << 8) > > +#define LED2_ON (0x9 << 8) > > +#define LEDMASK GENMASK(11,8) > > + > > +static struct phy_device *phydev; > > + > > +static void mvphy_reg_led(u16 data) > > +{ > > + int rc; > > + rc = phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, > > + MII_PHY_LED_CTRL, LEDMASK, data); > > + if (rc < 0) > > + dev_err(&phydev->mdio.dev, > > + "LED2 write register failed, %d\n", rc); > > +} > > + > > +static int linkstation_reboot_notifier(struct notifier_block *nb, > > + unsigned long action, void *unused) > > +{ > > + if (action == SYS_RESTART) > > + mvphy_reg_led(LED2_ON); > > + > > + return NOTIFY_DONE; > > +} > > + > > +static struct notifier_block linkstation_reboot_nb = { > > + .notifier_call = linkstation_reboot_notifier, > > +}; > > + > > +static void linkstation_poweroff(void) > > +{ > > + unregister_reboot_notifier(&linkstation_reboot_nb); > > + mvphy_reg_led(LED2_OFF); > > + > > + kernel_restart("Power off"); > > +} > > + > > +static int linkstation_poweroff_probe(struct platform_device *pdev) > > +{ > > + struct mii_bus *bus; > > + struct device_node *dn; > > + > > + dn = of_parse_phandle(pdev->dev.of_node, "phy-handle,led", 0); > > + > > + if (dn) { > > + phydev = of_phy_find_device(dn); > > + of_node_put(dn); > > + } else { > > + dn = of_find_node_by_name(NULL, "mdio"); > > + if (!dn) > > + return -ENODEV; > > + > > + bus = of_mdio_find_bus(dn); > > + of_node_put(dn); > > + if (!bus) > > + return -EPROBE_DEFER; > > + > > + phydev = phy_find_first(bus); > > do we need this code, or can we just make phy-handle,led mandatory > and error out otherwise? > > > + } > > + > > + if (!phydev) > > + return -ENODEV; > > Should this return -EPROBE_DEFER? > > > + register_reboot_notifier(&linkstation_reboot_nb); > > + pm_power_off = linkstation_poweroff; > > + > > + dev_info(&pdev->dev, "PHY [%s]\n", phydev_name(phydev)); > > I don't think this deserves more than dev_dbg() > > > + return 0; > > +} > > + > > +static int linkstation_poweroff_remove(struct platform_device *pdev) > > +{ > > + pm_power_off = NULL; > > + unregister_reboot_notifier(&linkstation_reboot_nb); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id ls_poweroff_of_match[] = { > > + { .compatible = "linkstation,power-off", }, > > + { }, > > +}; > > + > > +MODULE_DEVICE_TABLE(of, ls_poweroff_of_match); > > + > > +static struct platform_driver linkstation_poweroff_driver = { > > + .probe = linkstation_poweroff_probe, > > + .remove = linkstation_poweroff_remove, > > + .driver = { > > + .name = "linkstation_power_off", > > + .of_match_table = ls_poweroff_of_match, > > + }, > > +}; > > + > > +module_platform_driver(linkstation_poweroff_driver); > > + > > +MODULE_AUTHOR("Daniel González Cabanelas <dgcbueu@gmail.com>"); > > +MODULE_DESCRIPTION("LinkStation power off driver"); > > +MODULE_LICENSE("GPL v2"); > > Otherwise looks good to me, > > -- Sebastian
diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig index 8903803020..de948835f8 100644 --- a/drivers/power/reset/Kconfig +++ b/drivers/power/reset/Kconfig @@ -203,6 +203,16 @@ config POWER_RESET_KEYSTONE help Reboot support for the KEYSTONE SoCs. +config POWER_RESET_LINKSTATION + tristate "Buffalo LinkStation power-off driver" + depends on ARCH_MVEBU || COMPILE_TEST + depends on OF_MDIO && PHYLIB + help + Some Buffalo LinkStations use a LED output at the ethernet PHY + to inform the board that a power off operation must be performed + after restarting. This driver sets the LED to the proper state + for restarting or powering off. + config POWER_RESET_SYSCON bool "Generic SYSCON regmap reset driver" depends on OF diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile index da37f8b851..a8c83d2470 100644 --- a/drivers/power/reset/Makefile +++ b/drivers/power/reset/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_POWER_RESET_GEMINI_POWEROFF) += gemini-poweroff.o obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o +obj-${CONFIG_POWER_RESET_LINKSTATION} += linkstation-poweroff.o obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o obj-$(CONFIG_POWER_RESET_MT6323) += mt6323-poweroff.o obj-$(CONFIG_POWER_RESET_QCOM_PON) += qcom-pon.o diff --git a/drivers/power/reset/linkstation-poweroff.c b/drivers/power/reset/linkstation-poweroff.c new file mode 100644 index 0000000000..b5a4cc2c2b --- /dev/null +++ b/drivers/power/reset/linkstation-poweroff.c @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * LinkStation power off restart driver + * Copyright (C) 2020 Daniel González Cabanelas <dgcbueu@gmail.com> + */ + +#include <linux/module.h> +#include <linux/notifier.h> +#include <linux/of.h> +#include <linux/of_mdio.h> +#include <linux/of_platform.h> +#include <linux/reboot.h> +#include <linux/phy.h> + +#define MII_MARVELL_LED_PAGE 3 +#define MII_PHY_LED_CTRL 16 + +#define LED2_OFF (0x8 << 8) +#define LED2_ON (0x9 << 8) +#define LEDMASK GENMASK(11,8) + +static struct phy_device *phydev; + +static void mvphy_reg_led(u16 data) +{ + int rc; + rc = phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, + MII_PHY_LED_CTRL, LEDMASK, data); + if (rc < 0) + dev_err(&phydev->mdio.dev, + "LED2 write register failed, %d\n", rc); +} + +static int linkstation_reboot_notifier(struct notifier_block *nb, + unsigned long action, void *unused) +{ + if (action == SYS_RESTART) + mvphy_reg_led(LED2_ON); + + return NOTIFY_DONE; +} + +static struct notifier_block linkstation_reboot_nb = { + .notifier_call = linkstation_reboot_notifier, +}; + +static void linkstation_poweroff(void) +{ + unregister_reboot_notifier(&linkstation_reboot_nb); + mvphy_reg_led(LED2_OFF); + + kernel_restart("Power off"); +} + +static int linkstation_poweroff_probe(struct platform_device *pdev) +{ + struct mii_bus *bus; + struct device_node *dn; + + dn = of_parse_phandle(pdev->dev.of_node, "phy-handle,led", 0); + + if (dn) { + phydev = of_phy_find_device(dn); + of_node_put(dn); + } else { + dn = of_find_node_by_name(NULL, "mdio"); + if (!dn) + return -ENODEV; + + bus = of_mdio_find_bus(dn); + of_node_put(dn); + if (!bus) + return -EPROBE_DEFER; + + phydev = phy_find_first(bus); + } + + if (!phydev) + return -ENODEV; + + register_reboot_notifier(&linkstation_reboot_nb); + pm_power_off = linkstation_poweroff; + + dev_info(&pdev->dev, "PHY [%s]\n", phydev_name(phydev)); + + return 0; +} + +static int linkstation_poweroff_remove(struct platform_device *pdev) +{ + pm_power_off = NULL; + unregister_reboot_notifier(&linkstation_reboot_nb); + + return 0; +} + +static const struct of_device_id ls_poweroff_of_match[] = { + { .compatible = "linkstation,power-off", }, + { }, +}; + +MODULE_DEVICE_TABLE(of, ls_poweroff_of_match); + +static struct platform_driver linkstation_poweroff_driver = { + .probe = linkstation_poweroff_probe, + .remove = linkstation_poweroff_remove, + .driver = { + .name = "linkstation_power_off", + .of_match_table = ls_poweroff_of_match, + }, +}; + +module_platform_driver(linkstation_poweroff_driver); + +MODULE_AUTHOR("Daniel González Cabanelas <dgcbueu@gmail.com>"); +MODULE_DESCRIPTION("LinkStation power off driver"); +MODULE_LICENSE("GPL v2");