* [PATCH 0/2] reset: Add Broadcom STB RESCAL reset controller @ 2019-12-10 19:59 Florian Fainelli 2019-12-10 19:59 ` [PATCH 1/2] dt-bindings: reset: Document BCM7216 " Florian Fainelli 2019-12-10 19:59 ` [PATCH 2/2] reset: Add Broadcom STB " Florian Fainelli 0 siblings, 2 replies; 10+ messages in thread From: Florian Fainelli @ 2019-12-10 19:59 UTC (permalink / raw) To: linux-kernel Cc: Mark Rutland, Rob Herring, Florian Fainelli, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jim Quinlan, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Philipp Zabel, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE Hi Philipp, This patch series adds support for the BCM7216 RESCAL reset controller which is necessary to initialize SATA and PCIe0/1 on that chip. Please let us know if you have any comments. Thanks! Jim Quinlan (2): dt-bindings: reset: Document BCM7216 RESCAL reset controller reset: Add Broadcom STB RESCAL reset controller .../reset/brcm,bcm7216-pcie-sata-rescal.txt | 26 ++++ drivers/reset/Kconfig | 7 + drivers/reset/Makefile | 1 + drivers/reset/reset-brcmstb-rescal.c | 124 ++++++++++++++++++ 4 files changed, 158 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm7216-pcie-sata-rescal.txt create mode 100644 drivers/reset/reset-brcmstb-rescal.c -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] dt-bindings: reset: Document BCM7216 RESCAL reset controller 2019-12-10 19:59 [PATCH 0/2] reset: Add Broadcom STB RESCAL reset controller Florian Fainelli @ 2019-12-10 19:59 ` Florian Fainelli 2019-12-19 20:40 ` Rob Herring 2019-12-10 19:59 ` [PATCH 2/2] reset: Add Broadcom STB " Florian Fainelli 1 sibling, 1 reply; 10+ messages in thread From: Florian Fainelli @ 2019-12-10 19:59 UTC (permalink / raw) To: linux-kernel Cc: Mark Rutland, Rob Herring, Florian Fainelli, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jim Quinlan, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Philipp Zabel, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE From: Jim Quinlan <jim2101024@gmail.com> BCM7216 has a special purpose RESCAL reset controller for its SATA and PCIe0/1 instances. This is a simple reset controller with #reset-cells set to 0. Signed-off-by: Jim Quinlan <jim2101024@gmail.com> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- .../reset/brcm,bcm7216-pcie-sata-rescal.txt | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm7216-pcie-sata-rescal.txt diff --git a/Documentation/devicetree/bindings/reset/brcm,bcm7216-pcie-sata-rescal.txt b/Documentation/devicetree/bindings/reset/brcm,bcm7216-pcie-sata-rescal.txt new file mode 100644 index 000000000000..ceaf7ee58726 --- /dev/null +++ b/Documentation/devicetree/bindings/reset/brcm,bcm7216-pcie-sata-rescal.txt @@ -0,0 +1,26 @@ +BCM7216 RESCAL reset controller +=============================== + +This document describes the BCM7216 RESCAL reset controller which is +responsible for controlling the reset of the SATA and PCIe0/1 instances on +BCM7216. + +Please also refer to reset.txt in this directory for common reset controller +binding usage. + +Required properties: +- compatible: should be brcm,bcm7216-pcie-sata-rescal +- reg: register base and length +- #reset-cells: must be set to 0 + +Example: + +rescal: reset { + compatible = "brcm,bcm7216-pcie-sata-rescal"; + reg = <0x8b2c800>; + #reset-cells = <0>; +}; + +&pcie0 { + resets = <&rescal>; +}; -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] dt-bindings: reset: Document BCM7216 RESCAL reset controller 2019-12-10 19:59 ` [PATCH 1/2] dt-bindings: reset: Document BCM7216 " Florian Fainelli @ 2019-12-19 20:40 ` Rob Herring 2019-12-19 20:52 ` Florian Fainelli 0 siblings, 1 reply; 10+ messages in thread From: Rob Herring @ 2019-12-19 20:40 UTC (permalink / raw) To: Florian Fainelli Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Jim Quinlan, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Philipp Zabel, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE On Tue, Dec 10, 2019 at 11:59:02AM -0800, Florian Fainelli wrote: > From: Jim Quinlan <jim2101024@gmail.com> > > BCM7216 has a special purpose RESCAL reset controller for its SATA and > PCIe0/1 instances. This is a simple reset controller with #reset-cells > set to 0. > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > .../reset/brcm,bcm7216-pcie-sata-rescal.txt | 26 +++++++++++++++++++ > 1 file changed, 26 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm7216-pcie-sata-rescal.txt Can you make this DT schema format. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] dt-bindings: reset: Document BCM7216 RESCAL reset controller 2019-12-19 20:40 ` Rob Herring @ 2019-12-19 20:52 ` Florian Fainelli 0 siblings, 0 replies; 10+ messages in thread From: Florian Fainelli @ 2019-12-19 20:52 UTC (permalink / raw) To: Rob Herring, Florian Fainelli Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, Jim Quinlan, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Philipp Zabel, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE On 12/19/19 12:40 PM, Rob Herring wrote: > On Tue, Dec 10, 2019 at 11:59:02AM -0800, Florian Fainelli wrote: >> From: Jim Quinlan <jim2101024@gmail.com> >> >> BCM7216 has a special purpose RESCAL reset controller for its SATA and >> PCIe0/1 instances. This is a simple reset controller with #reset-cells >> set to 0. >> >> Signed-off-by: Jim Quinlan <jim2101024@gmail.com> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> .../reset/brcm,bcm7216-pcie-sata-rescal.txt | 26 +++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm7216-pcie-sata-rescal.txt > > Can you make this DT schema format. Yes, most certainly, I need to re-spin the reset driver as well. Thanks! -- Florian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] reset: Add Broadcom STB RESCAL reset controller 2019-12-10 19:59 [PATCH 0/2] reset: Add Broadcom STB RESCAL reset controller Florian Fainelli 2019-12-10 19:59 ` [PATCH 1/2] dt-bindings: reset: Document BCM7216 " Florian Fainelli @ 2019-12-10 19:59 ` Florian Fainelli 2019-12-11 9:48 ` Philipp Zabel 1 sibling, 1 reply; 10+ messages in thread From: Florian Fainelli @ 2019-12-10 19:59 UTC (permalink / raw) To: linux-kernel Cc: Mark Rutland, Rob Herring, Jim Quinlan, Florian Fainelli, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jim Quinlan, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Philipp Zabel, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE From: Jim Quinlan <jim2101024@gmail.com> On BCM7216 there is a special purpose reset controller named RESCAL (reset calibration) which is necessary for SATA and PCIe0/1 to operate correctly. This commit adds support for such a reset controller to be available. Signed-off-by: Jim Quinlan <im2101024@gmail.com> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/reset/Kconfig | 7 ++ drivers/reset/Makefile | 1 + drivers/reset/reset-brcmstb-rescal.c | 124 +++++++++++++++++++++++++++ 3 files changed, 132 insertions(+) create mode 100644 drivers/reset/reset-brcmstb-rescal.c diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index 12f5c897788d..b7cc0a2049d9 100644 --- a/drivers/reset/Kconfig +++ b/drivers/reset/Kconfig @@ -49,6 +49,13 @@ config RESET_BRCMSTB This enables the reset controller driver for Broadcom STB SoCs using a SUN_TOP_CTRL_SW_INIT style controller. +config RESET_BRCMSTB_RESCAL + bool "Broadcom STB RESCAL reset controller" + default ARCH_BRCMSTB || COMPILE_TEST + help + This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on + BCM7216. + config RESET_HSDK bool "Synopsys HSDK Reset Driver" depends on HAS_IOMEM diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index 00767c03f5f2..1e4291185c52 100644 --- a/drivers/reset/Makefile +++ b/drivers/reset/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_RESET_ATH79) += reset-ath79.o obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o +obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o obj-$(CONFIG_RESET_IMX7) += reset-imx7.o obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o diff --git a/drivers/reset/reset-brcmstb-rescal.c b/drivers/reset/reset-brcmstb-rescal.c new file mode 100644 index 000000000000..58a30e624a14 --- /dev/null +++ b/drivers/reset/reset-brcmstb-rescal.c @@ -0,0 +1,124 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2018 Broadcom */ + + +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/reset-controller.h> +#include <linux/types.h> + +#define BRCM_RESCAL_START 0 +#define BRCM_RESCAL_START_BIT BIT(0) +#define BRCM_RESCAL_CTRL 4 +#define BRCM_RESCAL_STATUS 8 +#define BRCM_RESCAL_STATUS_BIT BIT(0) + +struct brcm_rescal_reset { + void __iomem *base; + struct device *dev; + struct reset_controller_dev rcdev; +}; + +static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + return 0; +} + +static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct brcm_rescal_reset *data = + container_of(rcdev, struct brcm_rescal_reset, rcdev); + void __iomem *base = data->base; + const int NUM_RETRIES = 10; + u32 reg; + int i; + + reg = readl(base + BRCM_RESCAL_START); + writel(reg | BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); + reg = readl(base + BRCM_RESCAL_START); + if (!(reg & BRCM_RESCAL_START_BIT)) { + dev_err(data->dev, "failed to start sata/pcie rescal\n"); + return -EIO; + } + + reg = readl(base + BRCM_RESCAL_STATUS); + for (i = NUM_RETRIES; i >= 0 && !(reg & BRCM_RESCAL_STATUS_BIT); i--) { + udelay(100); + reg = readl(base + BRCM_RESCAL_STATUS); + } + if (!(reg & BRCM_RESCAL_STATUS_BIT)) { + dev_err(data->dev, "timedout on sata/pcie rescal\n"); + return -ETIMEDOUT; + } + + reg = readl(base + BRCM_RESCAL_START); + writel(reg ^ BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); + reg = readl(base + BRCM_RESCAL_START); + dev_dbg(data->dev, "sata/pcie rescal success\n"); + + return 0; +} + +static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev, + const struct of_phandle_args *reset_spec) +{ + /* This is needed if #reset-cells == 0. */ + return 0; +} + +static const struct reset_control_ops brcm_rescal_reset_ops = { + .assert = brcm_rescal_reset_assert, + .deassert = brcm_rescal_reset_deassert, +}; + +static int brcm_rescal_reset_probe(struct platform_device *pdev) +{ + struct brcm_rescal_reset *data; + struct resource *res; + + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + data->base = devm_ioremap_resource(&pdev->dev, res); + + if (IS_ERR(data->base)) + return PTR_ERR(data->base); + + platform_set_drvdata(pdev, data); + + data->rcdev.owner = THIS_MODULE; + data->rcdev.nr_resets = 1; + data->rcdev.ops = &brcm_rescal_reset_ops; + data->rcdev.of_node = pdev->dev.of_node; + data->rcdev.of_xlate = brcm_rescal_reset_xlate; + data->dev = &pdev->dev; + + return devm_reset_controller_register(&pdev->dev, &data->rcdev); +} + +static const struct of_device_id brcm_rescal_reset_of_match[] = { + { .compatible = "brcm,bcm7216-pcie-sata-rescal" }, + { }, +}; +MODULE_DEVICE_TABLE(of, brcm_rescal_reset_of_match); + +static struct platform_driver brcm_rescal_reset_driver = { + .probe = brcm_rescal_reset_probe, + .driver = { + .name = "brcm-rescal-reset", + .of_match_table = brcm_rescal_reset_of_match, + } +}; +module_platform_driver(brcm_rescal_reset_driver); + +MODULE_AUTHOR("Broadcom"); +MODULE_DESCRIPTION("Broadcom Sata/PCIe rescal reset controller"); +MODULE_LICENSE("GPL v2"); -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] reset: Add Broadcom STB RESCAL reset controller 2019-12-10 19:59 ` [PATCH 2/2] reset: Add Broadcom STB " Florian Fainelli @ 2019-12-11 9:48 ` Philipp Zabel 2019-12-11 18:12 ` Florian Fainelli 0 siblings, 1 reply; 10+ messages in thread From: Philipp Zabel @ 2019-12-11 9:48 UTC (permalink / raw) To: Florian Fainelli, linux-kernel Cc: Mark Rutland, Rob Herring, Jim Quinlan, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jim Quinlan, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE Hi Florian, Jim, On Tue, 2019-12-10 at 11:59 -0800, Florian Fainelli wrote: > From: Jim Quinlan <jim2101024@gmail.com> > > On BCM7216 there is a special purpose reset controller named RESCAL > (reset calibration) which is necessary for SATA and PCIe0/1 to operate > correctly. This commit adds support for such a reset controller to be > available. > > Signed-off-by: Jim Quinlan <im2101024@gmail.com> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/reset/Kconfig | 7 ++ > drivers/reset/Makefile | 1 + > drivers/reset/reset-brcmstb-rescal.c | 124 +++++++++++++++++++++++++++ > 3 files changed, 132 insertions(+) > create mode 100644 drivers/reset/reset-brcmstb-rescal.c > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 12f5c897788d..b7cc0a2049d9 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -49,6 +49,13 @@ config RESET_BRCMSTB > This enables the reset controller driver for Broadcom STB SoCs using > a SUN_TOP_CTRL_SW_INIT style controller. > > +config RESET_BRCMSTB_RESCAL > + bool "Broadcom STB RESCAL reset controller" > + default ARCH_BRCMSTB || COMPILE_TEST > + help > + This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on > + BCM7216. > + > config RESET_HSDK > bool "Synopsys HSDK Reset Driver" > depends on HAS_IOMEM > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index 00767c03f5f2..1e4291185c52 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -8,6 +8,7 @@ obj-$(CONFIG_RESET_ATH79) += reset-ath79.o > obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o > obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o > obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o > +obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o > obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o > obj-$(CONFIG_RESET_IMX7) += reset-imx7.o > obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o > diff --git a/drivers/reset/reset-brcmstb-rescal.c b/drivers/reset/reset-brcmstb-rescal.c > new file mode 100644 > index 000000000000..58a30e624a14 > --- /dev/null > +++ b/drivers/reset/reset-brcmstb-rescal.c > @@ -0,0 +1,124 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (C) 2018 Broadcom */ > + > + > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/reset-controller.h> > +#include <linux/types.h> > + > +#define BRCM_RESCAL_START 0 > +#define BRCM_RESCAL_START_BIT BIT(0) > +#define BRCM_RESCAL_CTRL 4 > +#define BRCM_RESCAL_STATUS 8 > +#define BRCM_RESCAL_STATUS_BIT BIT(0) Is there any reason the start bit is indented but the status bit is not? > + > +struct brcm_rescal_reset { > + void __iomem *base; > + struct device *dev; > + struct reset_controller_dev rcdev; > +}; > + > +static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return 0; > +} Please do not implement the assert operation if it doesn't cause a reset line to be asserted afterwards. The reset core will return 0 from reset_control_assert() for shared reset controls if .assert is not implemented. > + > +static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct brcm_rescal_reset *data = > + container_of(rcdev, struct brcm_rescal_reset, rcdev); > + void __iomem *base = data->base; > + const int NUM_RETRIES = 10; > + u32 reg; > + int i; > + > + reg = readl(base + BRCM_RESCAL_START); > + writel(reg | BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); > + reg = readl(base + BRCM_RESCAL_START); Are there any other fields beside the START_BIT in this register? > + if (!(reg & BRCM_RESCAL_START_BIT)) { > + dev_err(data->dev, "failed to start sata/pcie rescal\n"); > + return -EIO; > + } > + > + reg = readl(base + BRCM_RESCAL_STATUS); > + for (i = NUM_RETRIES; i >= 0 && !(reg & BRCM_RESCAL_STATUS_BIT); i--) { > + udelay(100); > + reg = readl(base + BRCM_RESCAL_STATUS); > + } This timeout loop should be replaced by a single readl_poll_timeout(). At 100 µs waits per iteration this could use the sleeping variant. > + if (!(reg & BRCM_RESCAL_STATUS_BIT)) { > + dev_err(data->dev, "timedout on sata/pcie rescal\n"); > + return -ETIMEDOUT; > + } > + > + reg = readl(base + BRCM_RESCAL_START); > + writel(reg ^ BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); Please use &= ~BRCM_RESCAL_START_BIT instead. > + reg = readl(base + BRCM_RESCAL_START); > + dev_dbg(data->dev, "sata/pcie rescal success\n"); > + > + return 0; > +} This whole function looks a lot like it doesn't just deassert a reset line, but actually issues a complete reset procedure of some kind. Do you have some insight on what actually happens in the hardware when the start bit is triggered? I suspect this should be implemented with the .reset operation. > + > +static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev, > + const struct of_phandle_args *reset_spec) > +{ > + /* This is needed if #reset-cells == 0. */ > + return 0; > +} > + > +static const struct reset_control_ops brcm_rescal_reset_ops = { > + .assert = brcm_rescal_reset_assert, > + .deassert = brcm_rescal_reset_deassert, > +}; > + > +static int brcm_rescal_reset_probe(struct platform_device *pdev) > +{ > + struct brcm_rescal_reset *data; > + struct resource *res; > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + data->base = devm_ioremap_resource(&pdev->dev, res); > + > + if (IS_ERR(data->base)) > + return PTR_ERR(data->base); > + > + platform_set_drvdata(pdev, data); > + > + data->rcdev.owner = THIS_MODULE; > + data->rcdev.nr_resets = 1; > + data->rcdev.ops = &brcm_rescal_reset_ops; > + data->rcdev.of_node = pdev->dev.of_node; > + data->rcdev.of_xlate = brcm_rescal_reset_xlate; > + data->dev = &pdev->dev; > + > + return devm_reset_controller_register(&pdev->dev, &data->rcdev); > +} > + > +static const struct of_device_id brcm_rescal_reset_of_match[] = { > + { .compatible = "brcm,bcm7216-pcie-sata-rescal" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, brcm_rescal_reset_of_match); > + > +static struct platform_driver brcm_rescal_reset_driver = { > + .probe = brcm_rescal_reset_probe, > + .driver = { > + .name = "brcm-rescal-reset", > + .of_match_table = brcm_rescal_reset_of_match, > + } > +}; > +module_platform_driver(brcm_rescal_reset_driver); > + > +MODULE_AUTHOR("Broadcom"); > +MODULE_DESCRIPTION("Broadcom Sata/PCIe rescal reset controller"); > +MODULE_LICENSE("GPL v2"); regards Philipp _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] reset: Add Broadcom STB RESCAL reset controller 2019-12-11 9:48 ` Philipp Zabel @ 2019-12-11 18:12 ` Florian Fainelli 2019-12-12 10:01 ` Philipp Zabel 0 siblings, 1 reply; 10+ messages in thread From: Florian Fainelli @ 2019-12-11 18:12 UTC (permalink / raw) To: Philipp Zabel, linux-kernel Cc: Mark Rutland, Rob Herring, Jim Quinlan, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jim Quinlan, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE On 12/11/2019 1:48 AM, Philipp Zabel wrote: >> +#define BRCM_RESCAL_START 0 >> +#define BRCM_RESCAL_START_BIT BIT(0) >> +#define BRCM_RESCAL_CTRL 4 >> +#define BRCM_RESCAL_STATUS 8 >> +#define BRCM_RESCAL_STATUS_BIT BIT(0) > > Is there any reason the start bit is indented but the status bit is not? This is a convention we have tried to adopt to denote the definition from a register word address/offset versus the definition for bits within that register word. > >> + >> +struct brcm_rescal_reset { >> + void __iomem *base; >> + struct device *dev; >> + struct reset_controller_dev rcdev; >> +}; >> + >> +static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + return 0; >> +} > > Please do not implement the assert operation if it doesn't cause a reset > line to be asserted afterwards. > The reset core will return 0 from reset_control_assert() for shared > reset controls if .assert is not implemented. OK, will drop it. > >> + >> +static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + struct brcm_rescal_reset *data = >> + container_of(rcdev, struct brcm_rescal_reset, rcdev); >> + void __iomem *base = data->base; >> + const int NUM_RETRIES = 10; >> + u32 reg; >> + int i; >> + >> + reg = readl(base + BRCM_RESCAL_START); >> + writel(reg | BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); >> + reg = readl(base + BRCM_RESCAL_START); > > Are there any other fields beside the START_BIT in this register? This is the only bit actually. > >> + if (!(reg & BRCM_RESCAL_START_BIT)) { >> + dev_err(data->dev, "failed to start sata/pcie rescal\n"); >> + return -EIO; >> + } >> + >> + reg = readl(base + BRCM_RESCAL_STATUS); >> + for (i = NUM_RETRIES; i >= 0 && !(reg & BRCM_RESCAL_STATUS_BIT); i--) { >> + udelay(100); >> + reg = readl(base + BRCM_RESCAL_STATUS); >> + } > > This timeout loop should be replaced by a single readl_poll_timeout(). > At 100 µs waits per iteration this could use the sleeping variant. OK, will do. > >> + if (!(reg & BRCM_RESCAL_STATUS_BIT)) { >> + dev_err(data->dev, "timedout on sata/pcie rescal\n"); >> + return -ETIMEDOUT; >> + } >> + >> + reg = readl(base + BRCM_RESCAL_START); >> + writel(reg ^ BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); > > Please use &= ~BRCM_RESCAL_START_BIT instead. > I think the idea was to avoid unconditionally clearing it, but based on the documentation, I don't see this being harmful, Jim? >> + reg = readl(base + BRCM_RESCAL_START); >> + dev_dbg(data->dev, "sata/pcie rescal success\n"); >> + >> + return 0; >> +} > > This whole function looks a lot like it doesn't just deassert a reset > line, but actually issues a complete reset procedure of some kind. Do > you have some insight on what actually happens in the hardware when the > start bit is triggered? I suspect this should be implemented with the > .reset operation. This hardware block is controlling the reset and calibration process of the SATA/PCIe combo PHY analog front end, but is not technically part of the PCIe or SATA PHY proper, it stands on its own, both functionally and from a register space perspective. The motivation for modelling this as a reset controller is that it does a reset (and a calibration) and this is a shared reset line among 2/3 instances of another block. If you think we should model this differently, please let us know. -- Florian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] reset: Add Broadcom STB RESCAL reset controller 2019-12-11 18:12 ` Florian Fainelli @ 2019-12-12 10:01 ` Philipp Zabel 2019-12-30 19:05 ` Florian Fainelli 0 siblings, 1 reply; 10+ messages in thread From: Philipp Zabel @ 2019-12-12 10:01 UTC (permalink / raw) To: Florian Fainelli, linux-kernel Cc: Mark Rutland, Rob Herring, Jim Quinlan, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jim Quinlan, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE On Wed, 2019-12-11 at 10:12 -0800, Florian Fainelli wrote: > > On 12/11/2019 1:48 AM, Philipp Zabel wrote: > > > +#define BRCM_RESCAL_START 0 > > > +#define BRCM_RESCAL_START_BIT BIT(0) > > > +#define BRCM_RESCAL_CTRL 4 > > > +#define BRCM_RESCAL_STATUS 8 > > > +#define BRCM_RESCAL_STATUS_BIT BIT(0) > > > > Is there any reason the start bit is indented but the status bit is not? > > This is a convention we have tried to adopt to denote the definition > from a register word address/offset versus the definition for bits > within that register word. That's fine, consider indenting BRCM_RESCAL_STATUS_BIT as well, then. [...] > > > + reg = readl(base + BRCM_RESCAL_START); > > > + writel(reg | BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); > > > + reg = readl(base + BRCM_RESCAL_START); > > > + if (!(reg & BRCM_RESCAL_START_BIT)) { > > > + dev_err(data->dev, "failed to start sata/pcie rescal\n"); Is this something that can actually happen? [...] > > > + reg = readl(base + BRCM_RESCAL_START); > > > + writel(reg ^ BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); > > > > Please use &= ~BRCM_RESCAL_START_BIT instead. > > I think the idea was to avoid unconditionally clearing it, but based on > the documentation, I don't see this being harmful, Jim? Unless the bit is self-clearing, I can't see how this XOR could ever set the bit instead of clearing it. And even if it would, I don't understand how that can be indented. Wouldn't that restart the reset/calibration sequence? > > > + reg = readl(base + BRCM_RESCAL_START); > > > + dev_dbg(data->dev, "sata/pcie rescal success\n"); > > > + > > > + return 0; > > > +} > > > > This whole function looks a lot like it doesn't just deassert a reset > > line, but actually issues a complete reset procedure of some kind. Do > > you have some insight on what actually happens in the hardware when the > > start bit is triggered? I suspect this should be implemented with the > > .reset operation. > > This hardware block is controlling the reset and calibration process of > the SATA/PCIe combo PHY analog front end, but is not technically part of > the PCIe or SATA PHY proper, it stands on its own, both functionally and > from a register space perspective. The motivation for modelling this as > a reset controller is that it does a reset (and a calibration) and this > is a shared reset line among 2/3 instances of another block. If you > think we should model this differently, please let us know. Thank you for the explanation. I agree the "reset and calibration sequence" property is close enough to a pure reset sequence to warrant describing this as as reset controller. The correct way would be to use the .reset callback though, if you can have the drivers use reset_control_reset(). regards Philipp _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] reset: Add Broadcom STB RESCAL reset controller 2019-12-12 10:01 ` Philipp Zabel @ 2019-12-30 19:05 ` Florian Fainelli 2020-01-02 11:23 ` Philipp Zabel 0 siblings, 1 reply; 10+ messages in thread From: Florian Fainelli @ 2019-12-30 19:05 UTC (permalink / raw) To: Philipp Zabel, Florian Fainelli, linux-kernel, Jim Quinlan, Jim Quinlan Cc: Mark Rutland, Rob Herring, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS On 12/12/19 2:01 AM, Philipp Zabel wrote: > On Wed, 2019-12-11 at 10:12 -0800, Florian Fainelli wrote: >> >> On 12/11/2019 1:48 AM, Philipp Zabel wrote: >>>> +#define BRCM_RESCAL_START 0 >>>> +#define BRCM_RESCAL_START_BIT BIT(0) >>>> +#define BRCM_RESCAL_CTRL 4 >>>> +#define BRCM_RESCAL_STATUS 8 >>>> +#define BRCM_RESCAL_STATUS_BIT BIT(0) >>> >>> Is there any reason the start bit is indented but the status bit is not? >> >> This is a convention we have tried to adopt to denote the definition >> from a register word address/offset versus the definition for bits >> within that register word. > > That's fine, consider indenting BRCM_RESCAL_STATUS_BIT as well, then. > > [...] >>>> + reg = readl(base + BRCM_RESCAL_START); >>>> + writel(reg | BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); >>>> + reg = readl(base + BRCM_RESCAL_START); >>>> + if (!(reg & BRCM_RESCAL_START_BIT)) { >>>> + dev_err(data->dev, "failed to start sata/pcie rescal\n"); > > Is this something that can actually happen? Have not seen it happen but if we have bogus hardware, we would rather get an informative log from the reset controller than a not so informative one from the consumer drivers about e.g.: SATA or PCIe link down (which could be for various other reasons). If you want this demoted to a debug print, let me know. > > [...] >>>> + reg = readl(base + BRCM_RESCAL_START); >>>> + writel(reg ^ BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); >>> >>> Please use &= ~BRCM_RESCAL_START_BIT instead. >> >> I think the idea was to avoid unconditionally clearing it, but based on >> the documentation, I don't see this being harmful, Jim? > > Unless the bit is self-clearing, I can't see how this XOR could ever set > the bit instead of clearing it. > And even if it would, I don't understand how that can be indented. > Wouldn't that restart the reset/calibration sequence? The bit is not self clearing, but it can be cleared when the reset/calibration procedure is successfully finished, so this seems to do what it is intended for, in that, if you read the bit as 1, XOR would let you clear it. If you read it as 0, XOR would leave it cleared. Would you want a comment above to explain that? > >>>> + reg = readl(base + BRCM_RESCAL_START); >>>> + dev_dbg(data->dev, "sata/pcie rescal success\n"); >>>> + >>>> + return 0; >>>> +} >>> >>> This whole function looks a lot like it doesn't just deassert a reset >>> line, but actually issues a complete reset procedure of some kind. Do >>> you have some insight on what actually happens in the hardware when the >>> start bit is triggered? I suspect this should be implemented with the >>> .reset operation. >> >> This hardware block is controlling the reset and calibration process of >> the SATA/PCIe combo PHY analog front end, but is not technically part of >> the PCIe or SATA PHY proper, it stands on its own, both functionally and >> from a register space perspective. The motivation for modelling this as >> a reset controller is that it does a reset (and a calibration) and this >> is a shared reset line among 2/3 instances of another block. If you >> think we should model this differently, please let us know. > > Thank you for the explanation. I agree the "reset and calibration > sequence" property is close enough to a pure reset sequence to warrant > describing this as as reset controller. > The correct way would be to use the .reset callback though, if you can > have the drivers use reset_control_reset(). This should be doable, let me try to update the drivers accordingly. It sounds a bit silly to have to have kind of knowledge pushed down to the consumer drivers though... -- Florian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] reset: Add Broadcom STB RESCAL reset controller 2019-12-30 19:05 ` Florian Fainelli @ 2020-01-02 11:23 ` Philipp Zabel 0 siblings, 0 replies; 10+ messages in thread From: Philipp Zabel @ 2020-01-02 11:23 UTC (permalink / raw) To: Florian Fainelli, linux-kernel, Jim Quinlan, Jim Quinlan Cc: Mark Rutland, Rob Herring, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS Hi Florian, On Mon, 2019-12-30 at 11:05 -0800, Florian Fainelli wrote: > On 12/12/19 2:01 AM, Philipp Zabel wrote: [...] > > > > > + reg = readl(base + BRCM_RESCAL_START); > > > > > + writel(reg | BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); > > > > > + reg = readl(base + BRCM_RESCAL_START); > > > > > + if (!(reg & BRCM_RESCAL_START_BIT)) { > > > > > + dev_err(data->dev, "failed to start sata/pcie rescal\n"); > > > > Is this something that can actually happen? > > Have not seen it happen but if we have bogus hardware, we would rather > get an informative log from the reset controller than a not so > informative one from the consumer drivers about e.g.: SATA or PCIe link > down (which could be for various other reasons). If you want this > demoted to a debug print, let me know. Ok, that is not necessary. I was just surprised that you don't trust register writes to work. > > [...] > > > > > + reg = readl(base + BRCM_RESCAL_START); > > > > > + writel(reg ^ BRCM_RESCAL_START_BIT, base + BRCM_RESCAL_START); > > > > > > > > Please use &= ~BRCM_RESCAL_START_BIT instead. > > > > > > I think the idea was to avoid unconditionally clearing it, but based on > > > the documentation, I don't see this being harmful, Jim? > > > > Unless the bit is self-clearing, I can't see how this XOR could ever set > > the bit instead of clearing it. > > And even if it would, I don't understand how that can be indented. > > Wouldn't that restart the reset/calibration sequence? > The bit is not self clearing, but it can be cleared when the > reset/calibration procedure is successfully finished, so this seems to > do what it is intended for, in that, if you read the bit as 1, XOR would > let you clear it. I'm with you so far ... > If you read it as 0, XOR would leave it cleared. ... but I don't understand this part. If BRCM_RESCAL_START is read as 0 at this point, we end up writing (0 ^ 1) == 1. > Would you want a comment above to explain that? Yes, please. > > > > > + reg = readl(base + BRCM_RESCAL_START); > > > > > + dev_dbg(data->dev, "sata/pcie rescal success\n"); > > > > > + > > > > > + return 0; > > > > > +} > > > > > > > > This whole function looks a lot like it doesn't just deassert a reset > > > > line, but actually issues a complete reset procedure of some kind. Do > > > > you have some insight on what actually happens in the hardware when the > > > > start bit is triggered? I suspect this should be implemented with the > > > > .reset operation. > > > > > > This hardware block is controlling the reset and calibration process of > > > the SATA/PCIe combo PHY analog front end, but is not technically part of > > > the PCIe or SATA PHY proper, it stands on its own, both functionally and > > > from a register space perspective. The motivation for modelling this as > > > a reset controller is that it does a reset (and a calibration) and this > > > is a shared reset line among 2/3 instances of another block. If you > > > think we should model this differently, please let us know. > > > > Thank you for the explanation. I agree the "reset and calibration > > sequence" property is close enough to a pure reset sequence to warrant > > describing this as as reset controller. > > The correct way would be to use the .reset callback though, if you can > > have the drivers use reset_control_reset(). > > This should be doable, let me try to update the drivers accordingly. It > sounds a bit silly to have to have kind of knowledge pushed down to the > consumer drivers though... There are cases where the abstraction doesn't fit perfectly, but usually it is the consumer driver that knows best whether the hardware just needs to be made responsive by deasserting the reset line, or whether triggering a reset procedure is required to initialize some internal state. regards Philipp _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-02 11:23 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-10 19:59 [PATCH 0/2] reset: Add Broadcom STB RESCAL reset controller Florian Fainelli 2019-12-10 19:59 ` [PATCH 1/2] dt-bindings: reset: Document BCM7216 " Florian Fainelli 2019-12-19 20:40 ` Rob Herring 2019-12-19 20:52 ` Florian Fainelli 2019-12-10 19:59 ` [PATCH 2/2] reset: Add Broadcom STB " Florian Fainelli 2019-12-11 9:48 ` Philipp Zabel 2019-12-11 18:12 ` Florian Fainelli 2019-12-12 10:01 ` Philipp Zabel 2019-12-30 19:05 ` Florian Fainelli 2020-01-02 11:23 ` Philipp Zabel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).