* [PATCH v4 0/2] Add support for SYSCON reset @ 2016-06-20 18:46 Andrew F. Davis 2016-06-20 18:46 ` [PATCH v4 1/2] Documentation: dt: reset: Add TI syscon reset binding Andrew F. Davis 2016-06-20 18:46 ` [PATCH v4 2/2] reset: add TI SYSCON based reset driver Andrew F. Davis 0 siblings, 2 replies; 12+ messages in thread From: Andrew F. Davis @ 2016-06-20 18:46 UTC (permalink / raw) To: Philipp Zabel, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna Cc: devicetree, linux-kernel, Andrew F. Davis Some SoCs contain reset controls for modules that are memory-mapped to areas shared with other module configuration settings. This requires synchronization across all drivers accessing this memory area. This series adds a generic SYSCON reset driver to allow resets toggled by bits in memory-mapped registers through SYSCON. Changes from v3: - Add depends on HAS_IOMEM to Kconfig - Re-worked naming to be more TI specific for now - Changed DT binding to Philipp's suggestion Changes from v2: - Rebased on v4.7-rc1 - Removed the need to give reset specifier nodes an index address Changes from v1: - Reset control information is now described in the reset node, this keeps the reset information centralized for easy verification - Other small fixups Andrew F. Davis (2): Documentation: dt: reset: Add TI syscon reset binding reset: add TI SYSCON based reset driver .../devicetree/bindings/reset/ti-syscon-reset.txt | 83 +++++++ drivers/reset/Kconfig | 11 + drivers/reset/Makefile | 1 + drivers/reset/reset-ti-syscon.c | 260 +++++++++++++++++++++ include/dt-bindings/reset/ti-syscon.h | 30 +++ 5 files changed, 385 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/ti-syscon-reset.txt create mode 100644 drivers/reset/reset-ti-syscon.c create mode 100644 include/dt-bindings/reset/ti-syscon.h -- 2.9.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 1/2] Documentation: dt: reset: Add TI syscon reset binding 2016-06-20 18:46 [PATCH v4 0/2] Add support for SYSCON reset Andrew F. Davis @ 2016-06-20 18:46 ` Andrew F. Davis 2016-06-21 19:25 ` Rob Herring 2016-06-20 18:46 ` [PATCH v4 2/2] reset: add TI SYSCON based reset driver Andrew F. Davis 1 sibling, 1 reply; 12+ messages in thread From: Andrew F. Davis @ 2016-06-20 18:46 UTC (permalink / raw) To: Philipp Zabel, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna Cc: devicetree, linux-kernel, Andrew F. Davis Add TI syscon reset controller binding. This will hook to the reset framework and use syscon/regmap to set reset bits. This allows reset control of individual SoC subsytems and devices with memory-mapped reset registers in a common register memory space. Signed-off-by: Andrew F. Davis <afd@ti.com> [s-anna@ti.com: revise the binding format] Signed-off-by: Suman Anna <s-anna@ti.com> --- .../devicetree/bindings/reset/ti-syscon-reset.txt | 83 ++++++++++++++++++++++ include/dt-bindings/reset/ti-syscon.h | 30 ++++++++ 2 files changed, 113 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/ti-syscon-reset.txt create mode 100644 include/dt-bindings/reset/ti-syscon.h diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt new file mode 100644 index 0000000..eb95bb5 --- /dev/null +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt @@ -0,0 +1,83 @@ +TI SysCon Reset Controller +======================= + +Almost all SoCs have hardware modules that require reset control in addition +to clock and power control for their functionality. The reset control is +typically provided by means of memory-mapped I/O registers. These registers are +sometimes a part of a larger register space region implementing various +functionalities. This register range is best represented as a syscon node to +allow multiple entities to access their relevant registers in the common +register space. + +A SysCon Reset Controller node defines a device that uses a syscon node +and provides reset management functionality for various hardware modules +present on the SoC. + +SysCon Reset Controller Node +============================ +Each of the reset provider/controller nodes should be a child of a syscon +node and have the following properties. + +Required properties: +-------------------- + - compatible : Should be "syscon-reset" + - #reset-cells : Should be 1. Please see the reset consumer node below + for usage details + - ti,reset-bits : Contains the reset control register information + Should contain 5 cells for each reset exposed to + consumers, defined as: + Cell #1 : register offset of the reset control + register from the syscon register base + Cell #2 : bit shift value for the reset in the + respective reset control register + Cell #3 : register offset of the reset status + register from the syscon register base + Cell #4 : bit shift value for the reset in the + respective reset status register + Cell #5 : Flags used to control reset behavior, + availible flags defined in the DT include + file <dt-bindings/reset/ti-syscon.h> + +SysCon Reset Consumer Nodes +=========================== +Each of the reset consumer nodes should have the following properties, +in addition to their own properties. + +Required properties: +-------------------- + - resets : A phandle to the reset controller node and a phandle to a + reset specifier node as defined above. + +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for +common reset controller usage by consumers. + +Example: +-------- +The following example demonstrates a syscon node, the reset controller node +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2 +Edison SoC. + +/ { + soc { + psc: power-sleep-controller@02350000 { + compatible = "syscon", "simple-mfd"; + reg = <0x02350000 0x1000>; + + pscrst: psc-reset { + compatible = "ti,k2l-pscrst", "syscon-reset"; + #reset-cells = <1>; + + ti,reset-bits = < + 0xa3c 8 0x83c 8 RESET_ASSERT_CLEAR /* 0: pcrst-dsp0 */ + 0xa40 5 0 0 RESET_TRIGGER_SET /* 1: pcrst-example */ + >; + }; + }; + + dsp0: dsp0 { + ... + resets = <&pscrst 0>; + ... + }; + }; +}; diff --git a/include/dt-bindings/reset/ti-syscon.h b/include/dt-bindings/reset/ti-syscon.h new file mode 100644 index 0000000..fedcfb7 --- /dev/null +++ b/include/dt-bindings/reset/ti-syscon.h @@ -0,0 +1,30 @@ +/* + * TI Syscon Reset definitions + * + * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __DT_BINDINGS_RESET_TI_SYSCON_H__ +#define __DT_BINDINGS_RESET_TI_SYSCON_H__ + +/* The reset is asserted by setting (vs clearing) the described bit */ +#define RESET_SET (1 << 0) +/* This reset does not have a readable status bit */ +#define RESET_TRIGGER (1 << 1) + +#define RESET_ASSERT_CLEAR 0 +#define RESET_ASSERT_SET RESET_SET +#define RESET_TRIGGER_CLEAR RESET_TRIGGER +#define RESET_TRIGGER_SET (RESET_TRIGGER | RESET_SET) + +#endif -- 2.9.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] Documentation: dt: reset: Add TI syscon reset binding 2016-06-20 18:46 ` [PATCH v4 1/2] Documentation: dt: reset: Add TI syscon reset binding Andrew F. Davis @ 2016-06-21 19:25 ` Rob Herring 2016-06-21 20:06 ` [PATCH v5] " Andrew F. Davis 0 siblings, 1 reply; 12+ messages in thread From: Rob Herring @ 2016-06-21 19:25 UTC (permalink / raw) To: Andrew F. Davis Cc: Philipp Zabel, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna, devicetree, linux-kernel On Mon, Jun 20, 2016 at 01:46:06PM -0500, Andrew F. Davis wrote: > Add TI syscon reset controller binding. This will hook to the reset > framework and use syscon/regmap to set reset bits. This allows reset > control of individual SoC subsytems and devices with memory-mapped > reset registers in a common register memory space. > > Signed-off-by: Andrew F. Davis <afd@ti.com> > [s-anna@ti.com: revise the binding format] > Signed-off-by: Suman Anna <s-anna@ti.com> > --- > .../devicetree/bindings/reset/ti-syscon-reset.txt | 83 ++++++++++++++++++++++ > include/dt-bindings/reset/ti-syscon.h | 30 ++++++++ > 2 files changed, 113 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/ti-syscon-reset.txt > create mode 100644 include/dt-bindings/reset/ti-syscon.h > > diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt > new file mode 100644 > index 0000000..eb95bb5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt > @@ -0,0 +1,83 @@ > +TI SysCon Reset Controller > +======================= > + > +Almost all SoCs have hardware modules that require reset control in addition > +to clock and power control for their functionality. The reset control is > +typically provided by means of memory-mapped I/O registers. These registers are > +sometimes a part of a larger register space region implementing various > +functionalities. This register range is best represented as a syscon node to > +allow multiple entities to access their relevant registers in the common > +register space. > + > +A SysCon Reset Controller node defines a device that uses a syscon node > +and provides reset management functionality for various hardware modules > +present on the SoC. > + > +SysCon Reset Controller Node > +============================ > +Each of the reset provider/controller nodes should be a child of a syscon > +node and have the following properties. > + > +Required properties: > +-------------------- > + - compatible : Should be "syscon-reset" SoC specific compatible? You have it in the example. Otherwise looks fine. Rob ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5] Documentation: dt: reset: Add TI syscon reset binding 2016-06-21 19:25 ` Rob Herring @ 2016-06-21 20:06 ` Andrew F. Davis 2016-06-24 15:35 ` Rob Herring 0 siblings, 1 reply; 12+ messages in thread From: Andrew F. Davis @ 2016-06-21 20:06 UTC (permalink / raw) To: Philipp Zabel, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna Cc: devicetree, linux-kernel, Andrew F. Davis Add TI syscon reset controller binding. This will hook to the reset framework and use syscon/regmap to set reset bits. This allows reset control of individual SoC subsytems and devices with memory-mapped reset registers in a common register memory space. Signed-off-by: Andrew F. Davis <afd@ti.com> [s-anna@ti.com: revise the binding format] Signed-off-by: Suman Anna <s-anna@ti.com> --- .../devicetree/bindings/reset/ti-syscon-reset.txt | 87 ++++++++++++++++++++++ include/dt-bindings/reset/ti-syscon.h | 30 ++++++++ 2 files changed, 117 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/ti-syscon-reset.txt create mode 100644 include/dt-bindings/reset/ti-syscon.h diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt new file mode 100644 index 0000000..5a0b86c --- /dev/null +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt @@ -0,0 +1,87 @@ +TI SysCon Reset Controller +======================= + +Almost all SoCs have hardware modules that require reset control in addition +to clock and power control for their functionality. The reset control is +typically provided by means of memory-mapped I/O registers. These registers are +sometimes a part of a larger register space region implementing various +functionalities. This register range is best represented as a syscon node to +allow multiple entities to access their relevant registers in the common +register space. + +A SysCon Reset Controller node defines a device that uses a syscon node +and provides reset management functionality for various hardware modules +present on the SoC. + +SysCon Reset Controller Node +============================ +Each of the reset provider/controller nodes should be a child of a syscon +node and have the following properties. + +Required properties: +-------------------- + - compatible : Should be, + "ti,k2e-pscrst" + "ti,k2l-pscrst" + "ti,k2hk-pscrst" + "syscon-reset" + - #reset-cells : Should be 1. Please see the reset consumer node below + for usage details + - ti,reset-bits : Contains the reset control register information + Should contain 5 cells for each reset exposed to + consumers, defined as: + Cell #1 : register offset of the reset control + register from the syscon register base + Cell #2 : bit shift value for the reset in the + respective reset control register + Cell #3 : register offset of the reset status + register from the syscon register base + Cell #4 : bit shift value for the reset in the + respective reset status register + Cell #5 : Flags used to control reset behavior, + availible flags defined in the DT include + file <dt-bindings/reset/ti-syscon.h> + +SysCon Reset Consumer Nodes +=========================== +Each of the reset consumer nodes should have the following properties, +in addition to their own properties. + +Required properties: +-------------------- + - resets : A phandle to the reset controller node and a phandle to a + reset specifier node as defined above. + +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for +common reset controller usage by consumers. + +Example: +-------- +The following example demonstrates a syscon node, the reset controller node +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2 +Edison SoC. + +/ { + soc { + psc: power-sleep-controller@02350000 { + compatible = "syscon", "simple-mfd"; + reg = <0x02350000 0x1000>; + + pscrst: psc-reset { + compatible = "ti,k2e-pscrst", "syscon-reset"; + #reset-cells = <1>; + + ti,reset-bits = < + 0xa3c 8 0x83c 8 RESET_ASSERT_CLEAR /* 0: pcrst-dsp0 */ + 0xa40 5 0 0 RESET_TRIGGER_SET /* 1: pcrst-example */ + >; + }; + }; + + dsp0: dsp0 { + ... + resets = <&pscrst 0>; + ... + }; + }; +}; diff --git a/include/dt-bindings/reset/ti-syscon.h b/include/dt-bindings/reset/ti-syscon.h new file mode 100644 index 0000000..fedcfb7 --- /dev/null +++ b/include/dt-bindings/reset/ti-syscon.h @@ -0,0 +1,30 @@ +/* + * TI Syscon Reset definitions + * + * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __DT_BINDINGS_RESET_TI_SYSCON_H__ +#define __DT_BINDINGS_RESET_TI_SYSCON_H__ + +/* The reset is asserted by setting (vs clearing) the described bit */ +#define RESET_SET (1 << 0) +/* This reset does not have a readable status bit */ +#define RESET_TRIGGER (1 << 1) + +#define RESET_ASSERT_CLEAR 0 +#define RESET_ASSERT_SET RESET_SET +#define RESET_TRIGGER_CLEAR RESET_TRIGGER +#define RESET_TRIGGER_SET (RESET_TRIGGER | RESET_SET) + +#endif -- 2.9.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v5] Documentation: dt: reset: Add TI syscon reset binding 2016-06-21 20:06 ` [PATCH v5] " Andrew F. Davis @ 2016-06-24 15:35 ` Rob Herring 0 siblings, 0 replies; 12+ messages in thread From: Rob Herring @ 2016-06-24 15:35 UTC (permalink / raw) To: Andrew F. Davis Cc: Philipp Zabel, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna, devicetree, linux-kernel On Tue, Jun 21, 2016 at 03:06:45PM -0500, Andrew F. Davis wrote: > Add TI syscon reset controller binding. This will hook to the reset > framework and use syscon/regmap to set reset bits. This allows reset > control of individual SoC subsytems and devices with memory-mapped > reset registers in a common register memory space. > > Signed-off-by: Andrew F. Davis <afd@ti.com> > [s-anna@ti.com: revise the binding format] > Signed-off-by: Suman Anna <s-anna@ti.com> > --- > .../devicetree/bindings/reset/ti-syscon-reset.txt | 87 ++++++++++++++++++++++ > include/dt-bindings/reset/ti-syscon.h | 30 ++++++++ > 2 files changed, 117 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/ti-syscon-reset.txt > create mode 100644 include/dt-bindings/reset/ti-syscon.h Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 2/2] reset: add TI SYSCON based reset driver 2016-06-20 18:46 [PATCH v4 0/2] Add support for SYSCON reset Andrew F. Davis 2016-06-20 18:46 ` [PATCH v4 1/2] Documentation: dt: reset: Add TI syscon reset binding Andrew F. Davis @ 2016-06-20 18:46 ` Andrew F. Davis 2016-06-22 10:19 ` Philipp Zabel 1 sibling, 1 reply; 12+ messages in thread From: Andrew F. Davis @ 2016-06-20 18:46 UTC (permalink / raw) To: Philipp Zabel, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna Cc: devicetree, linux-kernel, Andrew F. Davis Add a reset-controller driver for performing reset management of various devices present on the SoC, with the reset registers shared between devices in a common register memory space. This driver uses the syscon/regmap frameworks to actually implement the various reset functionalities needed by the reset consumer devices. Signed-off-by: Andrew F. Davis <afd@ti.com> [s-anna@ti.com: add documentation, syscon name change] Signed-off-by: Suman Anna <s-anna@ti.com> --- drivers/reset/Kconfig | 11 ++ drivers/reset/Makefile | 1 + drivers/reset/reset-ti-syscon.c | 260 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 272 insertions(+) create mode 100644 drivers/reset/reset-ti-syscon.c diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index 0b2733d..60a1aed 100644 --- a/drivers/reset/Kconfig +++ b/drivers/reset/Kconfig @@ -15,5 +15,16 @@ menuconfig RESET_CONTROLLER config RESET_OXNAS bool +config TI_SYSCON_RESET + tristate "TI SYSCON Reset Driver" + depends on RESET_CONTROLLER + depends on HAS_IOMEM + select MFD_SYSCON + help + This enables the reset driver support for TI devices with + memory-mapped reset registers as part of a syscon device node. If + you wish to use the reset framework for such memory-mapped devices, + say Y here. Otherwise, say N. + source "drivers/reset/sti/Kconfig" source "drivers/reset/hisilicon/Kconfig" diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index f173fc3..5a9dc40 100644 --- a/drivers/reset/Makefile +++ b/drivers/reset/Makefile @@ -9,3 +9,4 @@ obj-$(CONFIG_ARCH_HISI) += hisilicon/ obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o obj-$(CONFIG_ATH79) += reset-ath79.o obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o +obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c new file mode 100644 index 0000000..229f876 --- /dev/null +++ b/drivers/reset/reset-ti-syscon.c @@ -0,0 +1,260 @@ +/* + * TI SYSCON regmap reset driver + * + * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/ + * Andrew F. Davis <afd@ti.com> + * Suman Anna <afd@ti.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/reset-controller.h> + +#include <dt-bindings/reset/ti-syscon.h> + +/** + * struct ti_syscon_reset_control - reset control structure + * @offset: reset control register offset from syscon base + * @reset_bit: reset bit in the reset control register + * @assert_high: flag to indicate if setting the bit high asserts the reset + * @status_offset: reset status register offset from syscon base + * @status_reset_bit: reset status bit in the reset status register + * @status_assert_high: flag to indicate if a set bit represents asserted state + * @toggle: flag to indicate this reset has no readable status register + */ +struct ti_syscon_reset_control { + unsigned int offset; + unsigned int reset_bit; + bool assert_high; + unsigned int status_offset; + unsigned int status_reset_bit; + bool status_assert_high; + bool toggle; +}; + +/** + * struct ti_syscon_reset_data - reset controller information structure + * @rcdev: reset controller entity + * @dev: reset controller device pointer + * @regmap: regmap handle containing the memory-mapped reset registers + * @controls: array of reset controls + * @nr_controls: number of controls in control array + */ +struct ti_syscon_reset_data { + struct reset_controller_dev rcdev; + struct device *dev; + struct regmap *regmap; + struct ti_syscon_reset_control *controls; + unsigned int nr_controls; +}; + +#define to_ti_syscon_reset_data(rcdev) \ + container_of(rcdev, struct ti_syscon_reset_data, rcdev) + +/** + * ti_syscon_reset_set() - program a device's reset + * @rcdev: reset controller entity + * @id: ID of the reset to toggle + * @assert: boolean flag to indicate assert or deassert + * + * This is a common internal function used to assert or deassert a device's + * reset using the regmap API. The device's reset is asserted if the @assert + * argument is true, or deasserted if the @assert argument is false. + * + * Return: 0 for successful request, else a corresponding error value + */ +static int ti_syscon_reset_set(struct reset_controller_dev *rcdev, + unsigned long id, bool assert) +{ + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev); + struct ti_syscon_reset_control *control; + unsigned int mask, value; + + if (id < 0 || id >= data->nr_controls) + return -EINVAL; + + control = &data->controls[id]; + + mask = BIT(control->reset_bit); + value = (assert == control->assert_high) ? mask : 0x0; + + return regmap_update_bits(data->regmap, control->offset, mask, value); +} + +/** + * ti_syscon_reset_assert() - assert device reset + * @rcdev: reset controller entity + * @id: ID of the reset to be asserted + * + * This function implements the reset driver op to assert a device's reset. + * This invokes the function ti_syscon_reset_set() with the corresponding + * parameters as passed in, but with the @assert argument set to true for + * asserting the reset. + * + * Return: 0 for successful request, else a corresponding error value + */ +static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + return ti_syscon_reset_set(rcdev, id, true); +} + +/** + * ti_syscon_reset_deassert() - deassert device reset + * @rcdev: reset controller entity + * @id: ID of reset to be deasserted + * + * This function implements the reset driver op to deassert a device's reset. + * This invokes the function ti_syscon_reset_set() with the corresponding + * parameters as passed in, but with the @assert argument set to false for + * deasserting the reset. + * + * Return: 0 for successful request, else a corresponding error value + */ +static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + return ti_syscon_reset_set(rcdev, id, false); +} + +/** + * ti_syscon_reset_status() - check device reset status + * @rcdev: reset controller entity + * @id: ID of the reset for which the status is being requested + * + * This function implements the reset driver op to return the status of a + * device's reset. + * + * Return: 0 if reset is deasserted, true if reset is asserted, else a + * corresponding error value + */ +static int ti_syscon_reset_status(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev); + struct ti_syscon_reset_control *control; + unsigned int reset_state; + int ret; + + if (id < 0 || id >= data->nr_controls) + return -EINVAL; + + control = &data->controls[id]; + + if (control->toggle) + return -ENOSYS; /* status not supported for this reset */ + + ret = regmap_read(data->regmap, control->status_offset, &reset_state); + if (ret) + return ret; + + return (reset_state & BIT(control->status_reset_bit)) == + control->status_assert_high; +} + +static struct reset_control_ops ti_syscon_reset_ops = { + .assert = ti_syscon_reset_assert, + .deassert = ti_syscon_reset_deassert, + .status = ti_syscon_reset_status, +}; + +static int ti_syscon_reset_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct ti_syscon_reset_data *data; + struct regmap *regmap; + const __be32 *list; + struct ti_syscon_reset_control *controls; + int size, nr_controls, i; + u32 flags; + + if (!np) + return -ENODEV; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + regmap = syscon_node_to_regmap(np->parent); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + list = of_get_property(np, "ti,reset-bits", &size); + if (!list || (size / sizeof(*list)) % 5 != 0) { + dev_err(dev, "invalid DT reset description\n"); + return -EINVAL; + } + + nr_controls = (size / sizeof(*list)) / 5; + controls = devm_kzalloc(dev, nr_controls * sizeof(*controls), GFP_KERNEL); + if (!controls) + return -ENOMEM; + + for (i = 0; i < nr_controls; i++) { + controls[i].offset = be32_to_cpup(list++); + controls[i].reset_bit = be32_to_cpup(list++); + controls[i].status_offset = be32_to_cpup(list++); + controls[i].status_reset_bit = be32_to_cpup(list++); + + flags = be32_to_cpup(list++); + controls[i].assert_high = !!(flags & RESET_SET); + controls[i].status_assert_high = !!(flags & RESET_SET); + controls[i].toggle = !!(flags & RESET_TRIGGER); + } + + data->rcdev.ops = &ti_syscon_reset_ops; + data->rcdev.owner = THIS_MODULE; + data->rcdev.of_node = np; + data->rcdev.nr_resets = nr_controls; + data->dev = dev; + data->regmap = regmap; + data->controls = controls; + data->nr_controls = nr_controls; + + platform_set_drvdata(pdev, data); + + return reset_controller_register(&data->rcdev); +} + +static int ti_syscon_reset_remove(struct platform_device *pdev) +{ + struct ti_syscon_reset_data *data = platform_get_drvdata(pdev); + + reset_controller_unregister(&data->rcdev); + + return 0; +} + +static const struct of_device_id ti_syscon_reset_of_match[] = { + { .compatible = "syscon-reset", }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, ti_syscon_reset_of_match); + +static struct platform_driver ti_syscon_reset_driver = { + .probe = ti_syscon_reset_probe, + .remove = ti_syscon_reset_remove, + .driver = { + .name = "ti-syscon-reset", + .of_match_table = ti_syscon_reset_of_match, + }, +}; +module_platform_driver(ti_syscon_reset_driver); + +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>"); +MODULE_AUTHOR("Suman Anna <s-anna@ti.com>"); +MODULE_DESCRIPTION("TI SYSCON Regmap Reset Driver"); +MODULE_LICENSE("GPL v2"); -- 2.9.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] reset: add TI SYSCON based reset driver 2016-06-20 18:46 ` [PATCH v4 2/2] reset: add TI SYSCON based reset driver Andrew F. Davis @ 2016-06-22 10:19 ` Philipp Zabel 2016-06-22 19:46 ` Andrew F. Davis 0 siblings, 1 reply; 12+ messages in thread From: Philipp Zabel @ 2016-06-22 10:19 UTC (permalink / raw) To: Andrew F. Davis Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna, devicetree, linux-kernel Am Montag, den 20.06.2016, 13:46 -0500 schrieb Andrew F. Davis: > Add a reset-controller driver for performing reset management of > various devices present on the SoC, with the reset registers shared > between devices in a common register memory space. This driver uses > the syscon/regmap frameworks to actually implement the various reset > functionalities needed by the reset consumer devices. > > Signed-off-by: Andrew F. Davis <afd@ti.com> > [s-anna@ti.com: add documentation, syscon name change] > Signed-off-by: Suman Anna <s-anna@ti.com> > --- > drivers/reset/Kconfig | 11 ++ > drivers/reset/Makefile | 1 + > drivers/reset/reset-ti-syscon.c | 260 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 272 insertions(+) > create mode 100644 drivers/reset/reset-ti-syscon.c > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 0b2733d..60a1aed 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -15,5 +15,16 @@ menuconfig RESET_CONTROLLER > config RESET_OXNAS > bool > > +config TI_SYSCON_RESET > + tristate "TI SYSCON Reset Driver" > + depends on RESET_CONTROLLER Should be inside an "if RESET_CONTROLLER" on reset/next, so the depends is not needed anymore. > + depends on HAS_IOMEM > + select MFD_SYSCON > + help > + This enables the reset driver support for TI devices with > + memory-mapped reset registers as part of a syscon device node. If > + you wish to use the reset framework for such memory-mapped devices, > + say Y here. Otherwise, say N. Actually, do you need the user configurable option at all? > source "drivers/reset/sti/Kconfig" > source "drivers/reset/hisilicon/Kconfig" > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index f173fc3..5a9dc40 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -9,3 +9,4 @@ obj-$(CONFIG_ARCH_HISI) += hisilicon/ > obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o > obj-$(CONFIG_ATH79) += reset-ath79.o > obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o > +obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o > diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c > new file mode 100644 > index 0000000..229f876 > --- /dev/null > +++ b/drivers/reset/reset-ti-syscon.c > @@ -0,0 +1,260 @@ > +/* > + * TI SYSCON regmap reset driver > + * > + * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/ > + * Andrew F. Davis <afd@ti.com> > + * Suman Anna <afd@ti.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/reset-controller.h> > + > +#include <dt-bindings/reset/ti-syscon.h> > + > +/** > + * struct ti_syscon_reset_control - reset control structure > + * @offset: reset control register offset from syscon base > + * @reset_bit: reset bit in the reset control register > + * @assert_high: flag to indicate if setting the bit high asserts the reset > + * @status_offset: reset status register offset from syscon base > + * @status_reset_bit: reset status bit in the reset status register > + * @status_assert_high: flag to indicate if a set bit represents asserted state > + * @toggle: flag to indicate this reset has no readable status register > + */ > +struct ti_syscon_reset_control { > + unsigned int offset; > + unsigned int reset_bit; > + bool assert_high; > + unsigned int status_offset; > + unsigned int status_reset_bit; > + bool status_assert_high; > + bool toggle; > +}; > + > +/** > + * struct ti_syscon_reset_data - reset controller information structure > + * @rcdev: reset controller entity > + * @dev: reset controller device pointer > + * @regmap: regmap handle containing the memory-mapped reset registers > + * @controls: array of reset controls > + * @nr_controls: number of controls in control array > + */ > +struct ti_syscon_reset_data { > + struct reset_controller_dev rcdev; > + struct device *dev; I don't see data->dev used anywhere. I think you can drop this. > + struct regmap *regmap; > + struct ti_syscon_reset_control *controls; > + unsigned int nr_controls; > +}; > + > +#define to_ti_syscon_reset_data(rcdev) \ > + container_of(rcdev, struct ti_syscon_reset_data, rcdev) > + > +/** > + * ti_syscon_reset_set() - program a device's reset > + * @rcdev: reset controller entity > + * @id: ID of the reset to toggle > + * @assert: boolean flag to indicate assert or deassert > + * > + * This is a common internal function used to assert or deassert a device's > + * reset using the regmap API. The device's reset is asserted if the @assert > + * argument is true, or deasserted if the @assert argument is false. > + * > + * Return: 0 for successful request, else a corresponding error value > + */ > +static int ti_syscon_reset_set(struct reset_controller_dev *rcdev, > + unsigned long id, bool assert) > +{ > + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev); > + struct ti_syscon_reset_control *control; > + unsigned int mask, value; > + > + if (id < 0 || id >= data->nr_controls) id is unsigned long, no need to check for negative values. > + return -EINVAL; > + > + control = &data->controls[id]; > + > + mask = BIT(control->reset_bit); > + value = (assert == control->assert_high) ? mask : 0x0; > + > + return regmap_update_bits(data->regmap, control->offset, mask, value); > +} > + > +/** > + * ti_syscon_reset_assert() - assert device reset > + * @rcdev: reset controller entity > + * @id: ID of the reset to be asserted > + * > + * This function implements the reset driver op to assert a device's reset. > + * This invokes the function ti_syscon_reset_set() with the corresponding > + * parameters as passed in, but with the @assert argument set to true for > + * asserting the reset. > + * > + * Return: 0 for successful request, else a corresponding error value > + */ > +static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return ti_syscon_reset_set(rcdev, id, true); > +} > + > +/** > + * ti_syscon_reset_deassert() - deassert device reset > + * @rcdev: reset controller entity > + * @id: ID of reset to be deasserted > + * > + * This function implements the reset driver op to deassert a device's reset. > + * This invokes the function ti_syscon_reset_set() with the corresponding > + * parameters as passed in, but with the @assert argument set to false for > + * deasserting the reset. > + * > + * Return: 0 for successful request, else a corresponding error value > + */ > +static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return ti_syscon_reset_set(rcdev, id, false); > +} > + > +/** > + * ti_syscon_reset_status() - check device reset status > + * @rcdev: reset controller entity > + * @id: ID of the reset for which the status is being requested > + * > + * This function implements the reset driver op to return the status of a > + * device's reset. > + * > + * Return: 0 if reset is deasserted, true if reset is asserted, else a > + * corresponding error value > + */ > +static int ti_syscon_reset_status(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev); > + struct ti_syscon_reset_control *control; > + unsigned int reset_state; > + int ret; > + > + if (id < 0 || id >= data->nr_controls) if (id >= data->nr_controls) > + return -EINVAL; > + > + control = &data->controls[id]; > + > + if (control->toggle) > + return -ENOSYS; /* status not supported for this reset */ That should be -ENOTSUPP. Are you sure that reading status is not supported for your trigger resets? On i.MX6 the triggered reset bits are self-clearing, for example, but only after the reset sequence is finished. So it is possible to read the reset status there. > + > + ret = regmap_read(data->regmap, control->status_offset, &reset_state); > + if (ret) > + return ret; > + > + return (reset_state & BIT(control->status_reset_bit)) == > + control->status_assert_high; If status_assert_high == 1 and status_reset_bit > 0 this will always return false. > +} > + > +static struct reset_control_ops ti_syscon_reset_ops = { > + .assert = ti_syscon_reset_assert, > + .deassert = ti_syscon_reset_deassert, > + .status = ti_syscon_reset_status, > +}; > + > +static int ti_syscon_reset_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct ti_syscon_reset_data *data; > + struct regmap *regmap; > + const __be32 *list; > + struct ti_syscon_reset_control *controls; > + int size, nr_controls, i; > + u32 flags; > + > + if (!np) > + return -ENODEV; This driver is probed via DT. Can this ever happen? > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + regmap = syscon_node_to_regmap(np->parent); > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); > + > + list = of_get_property(np, "ti,reset-bits", &size); > + if (!list || (size / sizeof(*list)) % 5 != 0) { > + dev_err(dev, "invalid DT reset description\n"); > + return -EINVAL; > + } > + > + nr_controls = (size / sizeof(*list)) / 5; > + controls = devm_kzalloc(dev, nr_controls * sizeof(*controls), GFP_KERNEL); > + if (!controls) > + return -ENOMEM; > + > + for (i = 0; i < nr_controls; i++) { > + controls[i].offset = be32_to_cpup(list++); > + controls[i].reset_bit = be32_to_cpup(list++); > + controls[i].status_offset = be32_to_cpup(list++); > + controls[i].status_reset_bit = be32_to_cpup(list++); > + > + flags = be32_to_cpup(list++); > + controls[i].assert_high = !!(flags & RESET_SET); > + controls[i].status_assert_high = !!(flags & RESET_SET); Why two variables if these are always the same. > + controls[i].toggle = !!(flags & RESET_TRIGGER); > + } > + > + data->rcdev.ops = &ti_syscon_reset_ops; > + data->rcdev.owner = THIS_MODULE; > + data->rcdev.of_node = np; > + data->rcdev.nr_resets = nr_controls; > + data->dev = dev; data->dev is not used. > + data->regmap = regmap; > + data->controls = controls; > + data->nr_controls = nr_controls; > + > + platform_set_drvdata(pdev, data); > + > + return reset_controller_register(&data->rcdev); Use devm_reset_controller_register here ... > +} > + > +static int ti_syscon_reset_remove(struct platform_device *pdev) > +{ > + struct ti_syscon_reset_data *data = platform_get_drvdata(pdev); > + > + reset_controller_unregister(&data->rcdev); > + > + return 0; > +} ... which allows you to remove the remove function entirely. > + > +static const struct of_device_id ti_syscon_reset_of_match[] = { > + { .compatible = "syscon-reset", }, "ti,syscon-reset" > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, ti_syscon_reset_of_match); > + > +static struct platform_driver ti_syscon_reset_driver = { > + .probe = ti_syscon_reset_probe, > + .remove = ti_syscon_reset_remove, > + .driver = { > + .name = "ti-syscon-reset", > + .of_match_table = ti_syscon_reset_of_match, > + }, > +}; > +module_platform_driver(ti_syscon_reset_driver); > + > +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>"); > +MODULE_AUTHOR("Suman Anna <s-anna@ti.com>"); > +MODULE_DESCRIPTION("TI SYSCON Regmap Reset Driver"); > +MODULE_LICENSE("GPL v2"); regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] reset: add TI SYSCON based reset driver 2016-06-22 10:19 ` Philipp Zabel @ 2016-06-22 19:46 ` Andrew F. Davis 2016-06-23 9:05 ` Philipp Zabel 0 siblings, 1 reply; 12+ messages in thread From: Andrew F. Davis @ 2016-06-22 19:46 UTC (permalink / raw) To: Philipp Zabel Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna, devicetree, linux-kernel On 06/22/2016 05:19 AM, Philipp Zabel wrote: > Am Montag, den 20.06.2016, 13:46 -0500 schrieb Andrew F. Davis: >> Add a reset-controller driver for performing reset management of >> various devices present on the SoC, with the reset registers shared >> between devices in a common register memory space. This driver uses >> the syscon/regmap frameworks to actually implement the various reset >> functionalities needed by the reset consumer devices. >> >> Signed-off-by: Andrew F. Davis <afd@ti.com> >> [s-anna@ti.com: add documentation, syscon name change] >> Signed-off-by: Suman Anna <s-anna@ti.com> >> --- >> drivers/reset/Kconfig | 11 ++ >> drivers/reset/Makefile | 1 + >> drivers/reset/reset-ti-syscon.c | 260 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 272 insertions(+) >> create mode 100644 drivers/reset/reset-ti-syscon.c >> >> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig >> index 0b2733d..60a1aed 100644 >> --- a/drivers/reset/Kconfig >> +++ b/drivers/reset/Kconfig >> @@ -15,5 +15,16 @@ menuconfig RESET_CONTROLLER >> config RESET_OXNAS >> bool >> >> +config TI_SYSCON_RESET >> + tristate "TI SYSCON Reset Driver" >> + depends on RESET_CONTROLLER > > Should be inside an "if RESET_CONTROLLER" on reset/next, so the depends > is not needed anymore. > Will fix. >> + depends on HAS_IOMEM >> + select MFD_SYSCON >> + help >> + This enables the reset driver support for TI devices with >> + memory-mapped reset registers as part of a syscon device node. If >> + you wish to use the reset framework for such memory-mapped devices, >> + say Y here. Otherwise, say N. > > Actually, do you need the user configurable option at all? > I'm not sure, right now it is selected by other things, but that is true for much of Kconfig, it is not platform dependent so it doesn't need to only be enabled by arch, it probably isn't hurting anything to leave it? >> source "drivers/reset/sti/Kconfig" >> source "drivers/reset/hisilicon/Kconfig" >> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile >> index f173fc3..5a9dc40 100644 >> --- a/drivers/reset/Makefile >> +++ b/drivers/reset/Makefile >> @@ -9,3 +9,4 @@ obj-$(CONFIG_ARCH_HISI) += hisilicon/ >> obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o >> obj-$(CONFIG_ATH79) += reset-ath79.o >> obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o >> +obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o >> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c >> new file mode 100644 >> index 0000000..229f876 >> --- /dev/null >> +++ b/drivers/reset/reset-ti-syscon.c >> @@ -0,0 +1,260 @@ >> +/* >> + * TI SYSCON regmap reset driver >> + * >> + * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/ >> + * Andrew F. Davis <afd@ti.com> >> + * Suman Anna <afd@ti.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any >> + * kind, whether express or implied; without even the implied warranty >> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include <linux/mfd/syscon.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> +#include <linux/reset-controller.h> >> + >> +#include <dt-bindings/reset/ti-syscon.h> >> + >> +/** >> + * struct ti_syscon_reset_control - reset control structure >> + * @offset: reset control register offset from syscon base >> + * @reset_bit: reset bit in the reset control register >> + * @assert_high: flag to indicate if setting the bit high asserts the reset >> + * @status_offset: reset status register offset from syscon base >> + * @status_reset_bit: reset status bit in the reset status register >> + * @status_assert_high: flag to indicate if a set bit represents asserted state >> + * @toggle: flag to indicate this reset has no readable status register >> + */ >> +struct ti_syscon_reset_control { >> + unsigned int offset; >> + unsigned int reset_bit; >> + bool assert_high; >> + unsigned int status_offset; >> + unsigned int status_reset_bit; >> + bool status_assert_high; >> + bool toggle; >> +}; >> + >> +/** >> + * struct ti_syscon_reset_data - reset controller information structure >> + * @rcdev: reset controller entity >> + * @dev: reset controller device pointer >> + * @regmap: regmap handle containing the memory-mapped reset registers >> + * @controls: array of reset controls >> + * @nr_controls: number of controls in control array >> + */ >> +struct ti_syscon_reset_data { >> + struct reset_controller_dev rcdev; >> + struct device *dev; > > I don't see data->dev used anywhere. I think you can drop this. > Will drop then. >> + struct regmap *regmap; >> + struct ti_syscon_reset_control *controls; >> + unsigned int nr_controls; >> +}; >> + >> +#define to_ti_syscon_reset_data(rcdev) \ >> + container_of(rcdev, struct ti_syscon_reset_data, rcdev) >> + >> +/** >> + * ti_syscon_reset_set() - program a device's reset >> + * @rcdev: reset controller entity >> + * @id: ID of the reset to toggle >> + * @assert: boolean flag to indicate assert or deassert >> + * >> + * This is a common internal function used to assert or deassert a device's >> + * reset using the regmap API. The device's reset is asserted if the @assert >> + * argument is true, or deasserted if the @assert argument is false. >> + * >> + * Return: 0 for successful request, else a corresponding error value >> + */ >> +static int ti_syscon_reset_set(struct reset_controller_dev *rcdev, >> + unsigned long id, bool assert) >> +{ >> + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev); >> + struct ti_syscon_reset_control *control; >> + unsigned int mask, value; >> + >> + if (id < 0 || id >= data->nr_controls) > > id is unsigned long, no need to check for negative values. > will fix. >> + return -EINVAL; >> + >> + control = &data->controls[id]; >> + >> + mask = BIT(control->reset_bit); >> + value = (assert == control->assert_high) ? mask : 0x0; >> + >> + return regmap_update_bits(data->regmap, control->offset, mask, value); >> +} >> + >> +/** >> + * ti_syscon_reset_assert() - assert device reset >> + * @rcdev: reset controller entity >> + * @id: ID of the reset to be asserted >> + * >> + * This function implements the reset driver op to assert a device's reset. >> + * This invokes the function ti_syscon_reset_set() with the corresponding >> + * parameters as passed in, but with the @assert argument set to true for >> + * asserting the reset. >> + * >> + * Return: 0 for successful request, else a corresponding error value >> + */ >> +static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + return ti_syscon_reset_set(rcdev, id, true); >> +} >> + >> +/** >> + * ti_syscon_reset_deassert() - deassert device reset >> + * @rcdev: reset controller entity >> + * @id: ID of reset to be deasserted >> + * >> + * This function implements the reset driver op to deassert a device's reset. >> + * This invokes the function ti_syscon_reset_set() with the corresponding >> + * parameters as passed in, but with the @assert argument set to false for >> + * deasserting the reset. >> + * >> + * Return: 0 for successful request, else a corresponding error value >> + */ >> +static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + return ti_syscon_reset_set(rcdev, id, false); >> +} >> + >> +/** >> + * ti_syscon_reset_status() - check device reset status >> + * @rcdev: reset controller entity >> + * @id: ID of the reset for which the status is being requested >> + * >> + * This function implements the reset driver op to return the status of a >> + * device's reset. >> + * >> + * Return: 0 if reset is deasserted, true if reset is asserted, else a >> + * corresponding error value >> + */ >> +static int ti_syscon_reset_status(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev); >> + struct ti_syscon_reset_control *control; >> + unsigned int reset_state; >> + int ret; >> + >> + if (id < 0 || id >= data->nr_controls) > > if (id >= data->nr_controls) > Will fix. >> + return -EINVAL; >> + >> + control = &data->controls[id]; >> + >> + if (control->toggle) >> + return -ENOSYS; /* status not supported for this reset */ > > That should be -ENOTSUPP. > Will fix. > Are you sure that reading status is not supported for your trigger > resets? > On i.MX6 the triggered reset bits are self-clearing, for example, but > only after the reset sequence is finished. So it is possible to read the > reset status there. > All the resets we have should have separate status bits, this trigger flag was added for systems that don't have and readable status bits (like some trigger resets), maybe the name should be changed? >> + >> + ret = regmap_read(data->regmap, control->status_offset, &reset_state); >> + if (ret) >> + return ret; >> + >> + return (reset_state & BIT(control->status_reset_bit)) == >> + control->status_assert_high; > > If status_assert_high == 1 and status_reset_bit > 0 this will always > return false. > ACK, will fix. >> +} >> + >> +static struct reset_control_ops ti_syscon_reset_ops = { >> + .assert = ti_syscon_reset_assert, >> + .deassert = ti_syscon_reset_deassert, >> + .status = ti_syscon_reset_status, >> +}; >> + >> +static int ti_syscon_reset_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + struct ti_syscon_reset_data *data; >> + struct regmap *regmap; >> + const __be32 *list; >> + struct ti_syscon_reset_control *controls; >> + int size, nr_controls, i; >> + u32 flags; >> + >> + if (!np) >> + return -ENODEV; > > This driver is probed via DT. Can this ever happen? > I don't think so, just sanity check, I'll remove it. >> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + regmap = syscon_node_to_regmap(np->parent); >> + if (IS_ERR(regmap)) >> + return PTR_ERR(regmap); >> + >> + list = of_get_property(np, "ti,reset-bits", &size); >> + if (!list || (size / sizeof(*list)) % 5 != 0) { >> + dev_err(dev, "invalid DT reset description\n"); >> + return -EINVAL; >> + } >> + >> + nr_controls = (size / sizeof(*list)) / 5; >> + controls = devm_kzalloc(dev, nr_controls * sizeof(*controls), GFP_KERNEL); >> + if (!controls) >> + return -ENOMEM; >> + >> + for (i = 0; i < nr_controls; i++) { >> + controls[i].offset = be32_to_cpup(list++); >> + controls[i].reset_bit = be32_to_cpup(list++); >> + controls[i].status_offset = be32_to_cpup(list++); >> + controls[i].status_reset_bit = be32_to_cpup(list++); >> + >> + flags = be32_to_cpup(list++); >> + controls[i].assert_high = !!(flags & RESET_SET); >> + controls[i].status_assert_high = !!(flags & RESET_SET); > > Why two variables if these are always the same. We have a status_ version for all fields, this way the status bits can be revere polarity from the regular control bit. This isn't used now but a flag can be added when this situation is needed. > >> + controls[i].toggle = !!(flags & RESET_TRIGGER); >> + } >> + >> + data->rcdev.ops = &ti_syscon_reset_ops; >> + data->rcdev.owner = THIS_MODULE; >> + data->rcdev.of_node = np; >> + data->rcdev.nr_resets = nr_controls; >> + data->dev = dev; > > data->dev is not used. > ACK >> + data->regmap = regmap; >> + data->controls = controls; >> + data->nr_controls = nr_controls; >> + >> + platform_set_drvdata(pdev, data); >> + >> + return reset_controller_register(&data->rcdev); > > Use devm_reset_controller_register here ... > ACK >> +} >> + >> +static int ti_syscon_reset_remove(struct platform_device *pdev) >> +{ >> + struct ti_syscon_reset_data *data = platform_get_drvdata(pdev); >> + >> + reset_controller_unregister(&data->rcdev); >> + >> + return 0; >> +} > > ... which allows you to remove the remove function entirely. > ACK >> + >> +static const struct of_device_id ti_syscon_reset_of_match[] = { >> + { .compatible = "syscon-reset", }, > > "ti,syscon-reset" > This matches the DT binding doc, if you would like that changed in the DT bindings I can do that too. Thanks, Andrew >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(of, ti_syscon_reset_of_match); >> + >> +static struct platform_driver ti_syscon_reset_driver = { >> + .probe = ti_syscon_reset_probe, >> + .remove = ti_syscon_reset_remove, >> + .driver = { >> + .name = "ti-syscon-reset", >> + .of_match_table = ti_syscon_reset_of_match, >> + }, >> +}; >> +module_platform_driver(ti_syscon_reset_driver); >> + >> +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>"); >> +MODULE_AUTHOR("Suman Anna <s-anna@ti.com>"); >> +MODULE_DESCRIPTION("TI SYSCON Regmap Reset Driver"); >> +MODULE_LICENSE("GPL v2"); > > regards > Philipp > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] reset: add TI SYSCON based reset driver 2016-06-22 19:46 ` Andrew F. Davis @ 2016-06-23 9:05 ` Philipp Zabel 2016-06-23 14:28 ` Andrew F. Davis 0 siblings, 1 reply; 12+ messages in thread From: Philipp Zabel @ 2016-06-23 9:05 UTC (permalink / raw) To: Andrew F. Davis Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna, devicetree, linux-kernel Am Mittwoch, den 22.06.2016, 14:46 -0500 schrieb Andrew F. Davis: [...] > >> + depends on HAS_IOMEM > >> + select MFD_SYSCON > >> + help > >> + This enables the reset driver support for TI devices with > >> + memory-mapped reset registers as part of a syscon device node. If > >> + you wish to use the reset framework for such memory-mapped devices, > >> + say Y here. Otherwise, say N. > > > > Actually, do you need the user configurable option at all? > > I'm not sure, right now it is selected by other things, but that is true > for much of Kconfig, it is not platform dependent so it doesn't need to > only be enabled by arch, it probably isn't hurting anything to leave it? No, that's okay. So the intention is to make it possible to enable it for COMPILE_TESTs on architectures other than TI? [...] > >> + if (control->toggle) > >> + return -ENOSYS; /* status not supported for this reset */ > > > > That should be -ENOTSUPP. > > > > Will fix. > > > Are you sure that reading status is not supported for your trigger > > resets? > > On i.MX6 the triggered reset bits are self-clearing, for example, but > > only after the reset sequence is finished. So it is possible to read the > > reset status there. > > All the resets we have should have separate status bits, this trigger > flag was added for systems that don't have and readable status bits > (like some trigger resets), maybe the name should be changed? I misunderstood. With "trigger" I mean a reset line that can't be controlled directly, so the driver should implement .reset() to trigger a reset sequence instead of .assert()/.deassert() to control the level. Whether or not the status can be read is something different. If you don't need it yet, you could just drop it for now, But if we want to make this as universally useful as possible, we should be sure that we cover most existing cases before defining the binding options. The hisilicon driver for example has just been changed to syscon. There the assert and deassert bits are in different registers and the status can't be read at all. To support that, too, we'd have to add a third register/bit pair to the binding... So far, I have seen the following variants. Depending on the hardware, a reset could be: - asserted by setting a bit - asserted by clearing a bit - deasserted by clearing/setting the same bit - deasserted by setting/clearing the same bit in another register - triggered to be asserted/deasserted automatically with some specific timing that the hardware knows about (in that case the manual assert/deassert is not available) The status of the reset line could be read via - the same bit that is used to assert/deassert - the same bit in another register - not at all What I've not yet seen but surely exists somewhere is the case where assert/deassert/status bits are at different bit positions either in the same register or in different registers. regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] reset: add TI SYSCON based reset driver 2016-06-23 9:05 ` Philipp Zabel @ 2016-06-23 14:28 ` Andrew F. Davis 2016-06-23 16:28 ` Philipp Zabel 0 siblings, 1 reply; 12+ messages in thread From: Andrew F. Davis @ 2016-06-23 14:28 UTC (permalink / raw) To: Philipp Zabel Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna, devicetree, linux-kernel On 06/23/2016 04:05 AM, Philipp Zabel wrote: > Am Mittwoch, den 22.06.2016, 14:46 -0500 schrieb Andrew F. Davis: > [...] >>>> + depends on HAS_IOMEM >>>> + select MFD_SYSCON >>>> + help >>>> + This enables the reset driver support for TI devices with >>>> + memory-mapped reset registers as part of a syscon device node. If >>>> + you wish to use the reset framework for such memory-mapped devices, >>>> + say Y here. Otherwise, say N. >>> >>> Actually, do you need the user configurable option at all? >> >> I'm not sure, right now it is selected by other things, but that is true >> for much of Kconfig, it is not platform dependent so it doesn't need to >> only be enabled by arch, it probably isn't hurting anything to leave it? > > No, that's okay. So the intention is to make it possible to enable it > for COMPILE_TESTs on architectures other than TI? > I was thinking more that it should be usable on non-TI architectures and so can be user enabled if needed. > [...] >>>> + if (control->toggle) >>>> + return -ENOSYS; /* status not supported for this reset */ >>> >>> That should be -ENOTSUPP. >>> >> >> Will fix. >> >>> Are you sure that reading status is not supported for your trigger >>> resets? >>> On i.MX6 the triggered reset bits are self-clearing, for example, but >>> only after the reset sequence is finished. So it is possible to read the >>> reset status there. >> >> All the resets we have should have separate status bits, this trigger >> flag was added for systems that don't have and readable status bits >> (like some trigger resets), maybe the name should be changed? > > I misunderstood. With "trigger" I mean a reset line that can't be > controlled directly, so the driver should implement .reset() to trigger > a reset sequence instead of .assert()/.deassert() to control the level. > Whether or not the status can be read is something different. > > If you don't need it yet, you could just drop it for now, But if we want > to make this as universally useful as possible, we should be sure that > we cover most existing cases before defining the binding options. > > The hisilicon driver for example has just been changed to syscon. There > the assert and deassert bits are in different registers and the status > can't be read at all. To support that, too, we'd have to add a third > register/bit pair to the binding... > > So far, I have seen the following variants. Depending on the hardware, a > reset could be: > - asserted by setting a bit > - asserted by clearing a bit > - deasserted by clearing/setting the same bit > - deasserted by setting/clearing the same bit in another register > - triggered to be asserted/deasserted automatically with some specific > timing that the hardware knows about (in that case the manual > assert/deassert is not available) > The status of the reset line could be read via > - the same bit that is used to assert/deassert > - the same bit in another register > - not at all > > What I've not yet seen but surely exists somewhere is the case where > assert/deassert/status bits are at different bit positions either in the > same register or in different registers. > Exactly why I was thinking having a node for the resets would be more future proof, we could add more properties later: pscrst-dsp: dsp { reset-control = <0xa3c 8 RESET_ASSERT_CLEAR>; reset-status = <0x83c 8 RESET_ASSERT_CLEAR>; + reset-deassert = <0x840 3 RESET_ASSERT_SET>; + reset-toggle-time-ms = <20>; }; While a fixed length array does make for a more condensed binding we don't get much flexibility. What we could also do is have a longer array then use macros to trim it down for simple cases: reset-bits = < SINGLE_BIT_RESET(0xa3c, 8) SINGLE_BIT_RESET(0xa40, 7) SINGLE_BIT_RESET(0xa44, 6) >; Each real entry could have 9 fields that the macro would expand into: (assert reg) (assert bit) (assert flag) (deassert reg) (deassert bit) (deassert flag) (status reg) (status bit) (status flag) This would be able to handle all of the above cases except the timing one, that case can always be handled by a dedicated driver for that system. My goal here, I would like to believe, aligns with the goals of DT in general, I would like to see one kernel work on many platforms without having to compile-in a bunch of SoC specific info. Some SoCs will need their own reset driver for when they do something unique (like this reset driver I will post next cycle which sends a message to a power management chip for its resets: http://git.ti.com/gitweb/?p=ti-linux-kernel/ti-linux-kernel.git;a=blob;f=drivers/reset/reset-ti-sci.c;h=3636dc176c8b5c4449eefc1fe13df7b912cec73e;hb=refs/heads/ti-lsk-linux-4.4.y ) but I see no reason a bunch of different drivers for simple register bit resets with the only difference being a #define offset. I hope that this effort will get ahead of these drivers and reduce the maintenance burden for you. So how much is handled by this driver is up to you, (hopefully without trying to handle every possible case :)). Thanks, Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] reset: add TI SYSCON based reset driver 2016-06-23 14:28 ` Andrew F. Davis @ 2016-06-23 16:28 ` Philipp Zabel 2016-06-27 15:23 ` Andrew F. Davis 0 siblings, 1 reply; 12+ messages in thread From: Philipp Zabel @ 2016-06-23 16:28 UTC (permalink / raw) To: Andrew F. Davis Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna, devicetree, linux-kernel Am Donnerstag, den 23.06.2016, 09:28 -0500 schrieb Andrew F. Davis: > On 06/23/2016 04:05 AM, Philipp Zabel wrote: > > Am Mittwoch, den 22.06.2016, 14:46 -0500 schrieb Andrew F. Davis: > > [...] > >>>> + depends on HAS_IOMEM > >>>> + select MFD_SYSCON > >>>> + help > >>>> + This enables the reset driver support for TI devices with > >>>> + memory-mapped reset registers as part of a syscon device node. If > >>>> + you wish to use the reset framework for such memory-mapped devices, > >>>> + say Y here. Otherwise, say N. > >>> > >>> Actually, do you need the user configurable option at all? > >> > >> I'm not sure, right now it is selected by other things, but that is true > >> for much of Kconfig, it is not platform dependent so it doesn't need to > >> only be enabled by arch, it probably isn't hurting anything to leave it? > > > > No, that's okay. So the intention is to make it possible to enable it > > for COMPILE_TESTs on architectures other than TI? > > I was thinking more that it should be usable on non-TI architectures and > so can be user enabled if needed. Those architectures could also just select it, then. Having the option might improve discoverability though. [...] > > So far, I have seen the following variants. Depending on the hardware, a > > reset could be: > > - asserted by setting a bit > > - asserted by clearing a bit > > - deasserted by clearing/setting the same bit > > - deasserted by setting/clearing the same bit in another register > > - triggered to be asserted/deasserted automatically with some specific > > timing that the hardware knows about (in that case the manual > > assert/deassert is not available) > > The status of the reset line could be read via > > - the same bit that is used to assert/deassert > > - the same bit in another register > > - not at all > > > > What I've not yet seen but surely exists somewhere is the case where > > assert/deassert/status bits are at different bit positions either in the > > same register or in different registers. > > Exactly why I was thinking having a node for the resets would be more > future proof, we could add more properties later: > > pscrst-dsp: dsp { > reset-control = <0xa3c 8 RESET_ASSERT_CLEAR>; > reset-status = <0x83c 8 RESET_ASSERT_CLEAR>; > + reset-deassert = <0x840 3 RESET_ASSERT_SET>; > + reset-toggle-time-ms = <20>; > }; > > While a fixed length array does make for a more condensed binding we > don't get much flexibility. > > What we could also do is have a longer array then use macros to trim it > down for simple cases: > > reset-bits = < > SINGLE_BIT_RESET(0xa3c, 8) > SINGLE_BIT_RESET(0xa40, 7) > SINGLE_BIT_RESET(0xa44, 6) > >; I think there has been opposition in the past against hiding things more complex than a single value behind macros. > Each real entry could have 9 fields that the macro would expand into: > (assert reg) (assert bit) (assert flag) > (deassert reg) (deassert bit) (deassert flag) > (status reg) (status bit) (status flag) > > This would be able to handle all of the above cases except the timing > one, that case can always be handled by a dedicated driver for that system. How about seven: (assert reg) (assert bit) (deassert reg) (deassert bit) (status reg) (status bit) (flags) reset-bits = < 0xa3c 8 0xa3c 8 0x83c 8 (ASSERT_CLEAR|DEASSERT_SET|STATUS_SET) >; > My goal here, I would like to believe, aligns with the goals of DT in > general, I would like to see one kernel work on many platforms without > having to compile-in a bunch of SoC specific info. Some SoCs will need > their own reset driver for when they do something unique (like this > reset driver I will post next cycle which sends a message to a power > management chip for its resets: > http://git.ti.com/gitweb/?p=ti-linux-kernel/ti-linux-kernel.git;a=blob;f=drivers/reset/reset-ti-sci.c;h=3636dc176c8b5c4449eefc1fe13df7b912cec73e;hb=refs/heads/ti-lsk-linux-4.4.y > ) > but I see no reason a bunch of different drivers for simple register bit > resets with the only difference being a #define offset. I hope that this > effort will get ahead of these drivers and reduce the maintenance burden > for you. The problem of reducing the amount of simple drivers is completely orthogonal to agreeing on a fitting DT binding. A common driver could just as well have static syscon_reset_control arrays per compatible compiled in. > So how much is handled by this driver is up to you, (hopefully without > trying to handle every possible case :)). It's also up to the DT maintainers to approve the bindings. I have no delusions that we have to find a compromise between driver/binding complexity and the number of supported common cases. The reason I find it difficult to make a decision is I don't feel like I have enough data. Should we support separate status reg? Obviously, you need it. Separate deassert reg? Questionable. Those devices usually have set/clear registers dedicated to resets and as such are not a good fit for this driver anyway. assert/deassert/status bits at different positions? Maybe not even needed. Support for triggered, self-clearing resets? Maybe better handled by a different driver, too. regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] reset: add TI SYSCON based reset driver 2016-06-23 16:28 ` Philipp Zabel @ 2016-06-27 15:23 ` Andrew F. Davis 0 siblings, 0 replies; 12+ messages in thread From: Andrew F. Davis @ 2016-06-27 15:23 UTC (permalink / raw) To: Philipp Zabel Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Suman Anna, devicetree, linux-kernel On 06/23/2016 11:28 AM, Philipp Zabel wrote: > Am Donnerstag, den 23.06.2016, 09:28 -0500 schrieb Andrew F. Davis: >> On 06/23/2016 04:05 AM, Philipp Zabel wrote: >>> Am Mittwoch, den 22.06.2016, 14:46 -0500 schrieb Andrew F. Davis: >>> [...] >>>>>> + depends on HAS_IOMEM >>>>>> + select MFD_SYSCON >>>>>> + help >>>>>> + This enables the reset driver support for TI devices with >>>>>> + memory-mapped reset registers as part of a syscon device node. If >>>>>> + you wish to use the reset framework for such memory-mapped devices, >>>>>> + say Y here. Otherwise, say N. >>>>> >>>>> Actually, do you need the user configurable option at all? >>>> >>>> I'm not sure, right now it is selected by other things, but that is true >>>> for much of Kconfig, it is not platform dependent so it doesn't need to >>>> only be enabled by arch, it probably isn't hurting anything to leave it? >>> >>> No, that's okay. So the intention is to make it possible to enable it >>> for COMPILE_TESTs on architectures other than TI? >> >> I was thinking more that it should be usable on non-TI architectures and >> so can be user enabled if needed. > > Those architectures could also just select it, then. Having the option > might improve discoverability though. > > [...] >>> So far, I have seen the following variants. Depending on the hardware, a >>> reset could be: >>> - asserted by setting a bit >>> - asserted by clearing a bit >>> - deasserted by clearing/setting the same bit >>> - deasserted by setting/clearing the same bit in another register >>> - triggered to be asserted/deasserted automatically with some specific >>> timing that the hardware knows about (in that case the manual >>> assert/deassert is not available) >>> The status of the reset line could be read via >>> - the same bit that is used to assert/deassert >>> - the same bit in another register >>> - not at all >>> >>> What I've not yet seen but surely exists somewhere is the case where >>> assert/deassert/status bits are at different bit positions either in the >>> same register or in different registers. >> >> Exactly why I was thinking having a node for the resets would be more >> future proof, we could add more properties later: >> >> pscrst-dsp: dsp { >> reset-control = <0xa3c 8 RESET_ASSERT_CLEAR>; >> reset-status = <0x83c 8 RESET_ASSERT_CLEAR>; >> + reset-deassert = <0x840 3 RESET_ASSERT_SET>; >> + reset-toggle-time-ms = <20>; >> }; >> >> While a fixed length array does make for a more condensed binding we >> don't get much flexibility. >> >> What we could also do is have a longer array then use macros to trim it >> down for simple cases: >> >> reset-bits = < >> SINGLE_BIT_RESET(0xa3c, 8) >> SINGLE_BIT_RESET(0xa40, 7) >> SINGLE_BIT_RESET(0xa44, 6) >>> ; > > I think there has been opposition in the past against hiding things more > complex than a single value behind macros. > >> Each real entry could have 9 fields that the macro would expand into: >> (assert reg) (assert bit) (assert flag) >> (deassert reg) (deassert bit) (deassert flag) >> (status reg) (status bit) (status flag) >> >> This would be able to handle all of the above cases except the timing >> one, that case can always be handled by a dedicated driver for that system. > > How about seven: > (assert reg) (assert bit) > (deassert reg) (deassert bit) > (status reg) (status bit) > (flags) > > reset-bits = < > 0xa3c 8 0xa3c 8 0x83c 8 (ASSERT_CLEAR|DEASSERT_SET|STATUS_SET) >> ; > I like this, we could also add a none flag: STATUS_SET STATUS_CLEAR STATUS_NONE to note that this reset doesn't support this and that the numbers given are not valid. >> My goal here, I would like to believe, aligns with the goals of DT in >> general, I would like to see one kernel work on many platforms without >> having to compile-in a bunch of SoC specific info. Some SoCs will need >> their own reset driver for when they do something unique (like this >> reset driver I will post next cycle which sends a message to a power >> management chip for its resets: >> http://git.ti.com/gitweb/?p=ti-linux-kernel/ti-linux-kernel.git;a=blob;f=drivers/reset/reset-ti-sci.c;h=3636dc176c8b5c4449eefc1fe13df7b912cec73e;hb=refs/heads/ti-lsk-linux-4.4.y >> ) >> but I see no reason a bunch of different drivers for simple register bit >> resets with the only difference being a #define offset. I hope that this >> effort will get ahead of these drivers and reduce the maintenance burden >> for you. > > The problem of reducing the amount of simple drivers is completely > orthogonal to agreeing on a fitting DT binding. A common driver could > just as well have static syscon_reset_control arrays per compatible > compiled in. > >> So how much is handled by this driver is up to you, (hopefully without >> trying to handle every possible case :)). > > It's also up to the DT maintainers to approve the bindings. > Looks like it got an ACK, almost makes me not want to touch it now :/ > I have no delusions that we have to find a compromise between > driver/binding complexity and the number of supported common cases. > The reason I find it difficult to make a decision is I don't feel like I > have enough data. > Should we support separate status reg? Obviously, you need it. Separate > deassert reg? Questionable. Those devices usually have set/clear > registers dedicated to resets and as such are not a good fit for this > driver anyway. assert/deassert/status bits at different positions? Maybe > not even needed. Support for triggered, self-clearing resets? Maybe > better handled by a different driver, too. > Well we can look at existing drivers, a quick looks seems to indicate the above scheme can handle the reset types currently handled by: hi6220_reset.c reset-ath79.c reset-meson.c reset-pistachio.c reset-socfpga.c reset-sunxi.c reset-zynq.c this is most of the current single bit reset drivers, the only ones that cannot be handled are ones with timing constraints and ones that need reset hardware setup. So I would say this is a good base set for covering many simple reset controllers. I'll push v5 with fixes from these comments and the DT changes here shortly. Thanks, Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-06-27 15:23 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-20 18:46 [PATCH v4 0/2] Add support for SYSCON reset Andrew F. Davis 2016-06-20 18:46 ` [PATCH v4 1/2] Documentation: dt: reset: Add TI syscon reset binding Andrew F. Davis 2016-06-21 19:25 ` Rob Herring 2016-06-21 20:06 ` [PATCH v5] " Andrew F. Davis 2016-06-24 15:35 ` Rob Herring 2016-06-20 18:46 ` [PATCH v4 2/2] reset: add TI SYSCON based reset driver Andrew F. Davis 2016-06-22 10:19 ` Philipp Zabel 2016-06-22 19:46 ` Andrew F. Davis 2016-06-23 9:05 ` Philipp Zabel 2016-06-23 14:28 ` Andrew F. Davis 2016-06-23 16:28 ` Philipp Zabel 2016-06-27 15:23 ` Andrew F. Davis
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).