| Message ID | 1565164139-21886-1-git-send-email-t-kristo@ti.com |
|---|---|
| Headers | show |
| Series | soc: ti: Add OMAP PRM driver | expand |
On 07/08/19 1:18 PM, Tero Kristo wrote: > Add new binding for OMAP PRM (Power and Reset Manager) instances. Each > of these will act as a power domain controller and potentially as a reset > provider. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > .../devicetree/bindings/arm/omap/prm-inst.txt | 24 ++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/omap/prm-inst.txt > > diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt > new file mode 100644 > index 0000000..e0ae87b > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt > @@ -0,0 +1,24 @@ > +OMAP PRM instance bindings > + > +Power and Reset Manager is an IP block on OMAP family of devices which > +handle the power domains and their current state, and provide reset > +handling for the domains and/or separate IP blocks under the power domain > +hierarchy. > + > +Required properties: > +- compatible: Must be one of: > + "ti,am3-prm-inst" > + "ti,am4-prm-inst" > + "ti,omap4-prm-inst" > + "ti,omap5-prm-inst" > + "ti,dra7-prm-inst" > +- reg: Contains PRM instance register address range > + (base address and length) How about reset-cells property, Isn't that a mandatory property? > + > +Example: > + > +prm_dsp2: prm@1b00 { > + compatible = "ti,dra7-prm-inst"; > + reg = <0x1b00 0x40>; > + #reset-cells = <1>; > +}; >
On 07/08/19 1:18 PM, Tero Kristo wrote: > Add initial PRM (Power and Reset Management) driver for TI OMAP class > SoCs. Initially this driver only supports reset control, but can be > extended to support rest of the functionality, like powerdomain > control, PRCM irq support etc. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > arch/arm/mach-omap2/Kconfig | 1 + > drivers/soc/ti/Makefile | 1 + > drivers/soc/ti/omap_prm.c | 216 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 218 insertions(+) > create mode 100644 drivers/soc/ti/omap_prm.c > > diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig > index fdb6743..42ad063 100644 > --- a/arch/arm/mach-omap2/Kconfig > +++ b/arch/arm/mach-omap2/Kconfig > @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS > select TI_SYSC > select OMAP_IRQCHIP > select CLKSRC_TI_32K > + select RESET_CONTROLLER > help > Systems based on OMAP2, OMAP3, OMAP4 or OMAP5 > > diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile > index b3868d3..788b5cd 100644 > --- a/drivers/soc/ti/Makefile > +++ b/drivers/soc/ti/Makefile > @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS) += knav_qmss.o > knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o > obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA) += knav_dma.o > obj-$(CONFIG_AMX3_PM) += pm33xx.o > +obj-$(CONFIG_ARCH_OMAP2PLUS) += omap_prm.o > obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o > obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o > obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN) += ti_sci_inta_msi.o > diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c > new file mode 100644 > index 0000000..7c89eb8 > --- /dev/null > +++ b/drivers/soc/ti/omap_prm.c > @@ -0,0 +1,216 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * OMAP2+ PRM driver > + * > + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/ > + * Tero Kristo <t-kristo@ti.com> > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/reset-controller.h> > +#include <linux/delay.h> > + > +struct omap_rst_map { > + s8 rst; > + s8 st; > +}; > + > +struct omap_prm_data { > + u32 base; > + const char *name; > + u16 pwstctrl; > + u16 pwstst; > + u16 rstctl; > + u16 rstst; > + struct omap_rst_map *rstmap; > + u8 flags; > +}; > + > +struct omap_prm { > + const struct omap_prm_data *data; > + void __iomem *base; > +}; > + > +struct omap_reset_data { > + struct reset_controller_dev rcdev; > + struct omap_prm *prm; > +}; > + > +#define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev) > + > +#define OMAP_MAX_RESETS 8 > +#define OMAP_RESET_MAX_WAIT 10000 > + > +#define OMAP_PRM_NO_RSTST BIT(0) > + > +static const struct of_device_id omap_prm_id_table[] = { > + { }, > +}; This table is blank and we are doing of_match_device against it. > + > +static int omap_reset_status(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct omap_reset_data *reset = to_omap_reset_data(rcdev); > + u32 v; > + > + v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); > + v &= 1 << id; > + v >>= id; > + > + return v; > +} > + > +static int omap_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct omap_reset_data *reset = to_omap_reset_data(rcdev); > + u32 v; > + > + /* assert the reset control line */ > + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); > + v |= 1 << id; > + writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl); > + > + return 0; > +} > + > +static int omap_reset_get_st_bit(struct omap_reset_data *reset, > + unsigned long id) > +{ > + struct omap_rst_map *map = reset->prm->data->rstmap; > + > + while (map && map->rst >= 0) { > + if (map->rst == id) > + return map->st; > + > + map++; > + } > + > + return id; > +} > + > +/* > + * Note that status will not change until clocks are on, and clocks cannot be > + * enabled until reset is deasserted. Consumer drivers must check status > + * separately after enabling clocks. > + */ > +static int omap_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct omap_reset_data *reset = to_omap_reset_data(rcdev); > + u32 v; > + int st_bit = id; > + bool has_rstst; > + > + /* check the current status to avoid de-asserting the line twice */ > + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); > + if (!(v & BIT(id))) > + return -EEXIST; > + > + has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST); > + > + if (has_rstst) { > + st_bit = omap_reset_get_st_bit(reset, id); > + > + /* Clear the reset status by writing 1 to the status bit */ > + v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); > + v |= 1 << st_bit; > + writel_relaxed(v, reset->prm->base + reset->prm->data->rstst); > + } > + > + /* de-assert the reset control line */ > + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); > + v &= ~(1 << id); > + writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl); > + > + return 0; > +} > + > +static const struct reset_control_ops omap_reset_ops = { > + .assert = omap_reset_assert, > + .deassert = omap_reset_deassert, > + .status = omap_reset_status, > +}; > + > +static int omap_prm_reset_probe(struct platform_device *pdev, > + struct omap_prm *prm) > +{ > + struct omap_reset_data *reset; > + > + /* > + * Check if we have resets. If either rstctl or rstst is > + * non-zero, we have reset registers in place. Additionally > + * the flag OMAP_PRM_NO_RSTST implies that we have resets. > + */ > + if (!prm->data->rstctl && !prm->data->rstst && > + !(prm->data->flags & OMAP_PRM_NO_RSTST)) > + return 0; > + > + reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL); > + if (!reset) > + return -ENOMEM; > + > + reset->rcdev.owner = THIS_MODULE; > + reset->rcdev.ops = &omap_reset_ops; > + reset->rcdev.of_node = pdev->dev.of_node; > + reset->rcdev.nr_resets = OMAP_MAX_RESETS; > + > + reset->prm = prm; > + > + return devm_reset_controller_register(&pdev->dev, &reset->rcdev); > +} > + > +static int omap_prm_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + const struct omap_prm_data *data; > + struct omap_prm *prm; > + const struct of_device_id *match; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; > + > + match = of_match_device(omap_prm_id_table, &pdev->dev); > + if (!match) > + return -ENOTSUPP; > + > + prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL); > + if (!prm) > + return -ENOMEM; > + > + data = match->data; > + > + while (data->base != res->start) { > + if (!data->base) > + return -EINVAL; > + data++; > + } > + > + prm->data = data; > + > + prm->base = devm_ioremap_resource(&pdev->dev, res); > + if (!prm->base) > + return -ENOMEM; > + > + return omap_prm_reset_probe(pdev, prm); > +} > + > +static struct platform_driver omap_prm_driver = { > + .probe = omap_prm_probe, > + .driver = { > + .name = KBUILD_MODNAME, > + .of_match_table = omap_prm_id_table, > + }, > +}; > +builtin_platform_driver(omap_prm_driver); > + > +MODULE_ALIAS("platform:prm"); > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("omap2+ prm driver"); It is a builtin_platform_driver so do we need the MODULE*? >
On 08/08/2019 07:35, Keerthy wrote: > > > On 07/08/19 1:18 PM, Tero Kristo wrote: >> Add new binding for OMAP PRM (Power and Reset Manager) instances. Each >> of these will act as a power domain controller and potentially as a reset >> provider. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> .../devicetree/bindings/arm/omap/prm-inst.txt | 24 >> ++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/arm/omap/prm-inst.txt >> >> diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt >> b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt >> new file mode 100644 >> index 0000000..e0ae87b >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt >> @@ -0,0 +1,24 @@ >> +OMAP PRM instance bindings >> + >> +Power and Reset Manager is an IP block on OMAP family of devices which >> +handle the power domains and their current state, and provide reset >> +handling for the domains and/or separate IP blocks under the power >> domain >> +hierarchy. >> + >> +Required properties: >> +- compatible: Must be one of: >> + "ti,am3-prm-inst" >> + "ti,am4-prm-inst" >> + "ti,omap4-prm-inst" >> + "ti,omap5-prm-inst" >> + "ti,dra7-prm-inst" >> +- reg: Contains PRM instance register address range >> + (base address and length) > > How about reset-cells property, Isn't that a mandatory property? It is optional, but you are right, should be added to this. -Tero > >> + >> +Example: >> + >> +prm_dsp2: prm@1b00 { >> + compatible = "ti,dra7-prm-inst"; >> + reg = <0x1b00 0x40>; >> + #reset-cells = <1>; >> +}; >> -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 08/08/2019 08:26, Keerthy wrote: > > > On 07/08/19 1:18 PM, Tero Kristo wrote: >> Add initial PRM (Power and Reset Management) driver for TI OMAP class >> SoCs. Initially this driver only supports reset control, but can be >> extended to support rest of the functionality, like powerdomain >> control, PRCM irq support etc. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> arch/arm/mach-omap2/Kconfig | 1 + >> drivers/soc/ti/Makefile | 1 + >> drivers/soc/ti/omap_prm.c | 216 >> ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 218 insertions(+) >> create mode 100644 drivers/soc/ti/omap_prm.c >> >> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig >> index fdb6743..42ad063 100644 >> --- a/arch/arm/mach-omap2/Kconfig >> +++ b/arch/arm/mach-omap2/Kconfig >> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS >> select TI_SYSC >> select OMAP_IRQCHIP >> select CLKSRC_TI_32K >> + select RESET_CONTROLLER >> help >> Systems based on OMAP2, OMAP3, OMAP4 or OMAP5 >> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile >> index b3868d3..788b5cd 100644 >> --- a/drivers/soc/ti/Makefile >> +++ b/drivers/soc/ti/Makefile >> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS) += knav_qmss.o >> knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o >> obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA) += knav_dma.o >> obj-$(CONFIG_AMX3_PM) += pm33xx.o >> +obj-$(CONFIG_ARCH_OMAP2PLUS) += omap_prm.o >> obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o >> obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o >> obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN) += ti_sci_inta_msi.o >> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c >> new file mode 100644 >> index 0000000..7c89eb8 >> --- /dev/null >> +++ b/drivers/soc/ti/omap_prm.c >> @@ -0,0 +1,216 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * OMAP2+ PRM driver >> + * >> + * Copyright (C) 2019 Texas Instruments Incorporated - >> http://www.ti.com/ >> + * Tero Kristo <t-kristo@ti.com> >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/device.h> >> +#include <linux/io.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/reset-controller.h> >> +#include <linux/delay.h> >> + >> +struct omap_rst_map { >> + s8 rst; >> + s8 st; >> +}; >> + >> +struct omap_prm_data { >> + u32 base; >> + const char *name; >> + u16 pwstctrl; >> + u16 pwstst; >> + u16 rstctl; >> + u16 rstst; >> + struct omap_rst_map *rstmap; >> + u8 flags; >> +}; >> + >> +struct omap_prm { >> + const struct omap_prm_data *data; >> + void __iomem *base; >> +}; >> + >> +struct omap_reset_data { >> + struct reset_controller_dev rcdev; >> + struct omap_prm *prm; >> +}; >> + >> +#define to_omap_reset_data(p) container_of((p), struct >> omap_reset_data, rcdev) >> + >> +#define OMAP_MAX_RESETS 8 >> +#define OMAP_RESET_MAX_WAIT 10000 >> + >> +#define OMAP_PRM_NO_RSTST BIT(0) >> + >> +static const struct of_device_id omap_prm_id_table[] = { >> + { }, >> +}; > > This table is blank and we are doing of_match_device against it. Yes, it gets populated with other patches in series, one entry per soc. > >> + >> +static int omap_reset_status(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >> + u32 v; >> + >> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); >> + v &= 1 << id; >> + v >>= id; >> + >> + return v; >> +} >> + >> +static int omap_reset_assert(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >> + u32 v; >> + >> + /* assert the reset control line */ >> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >> + v |= 1 << id; >> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl); >> + >> + return 0; >> +} >> + >> +static int omap_reset_get_st_bit(struct omap_reset_data *reset, >> + unsigned long id) >> +{ >> + struct omap_rst_map *map = reset->prm->data->rstmap; >> + >> + while (map && map->rst >= 0) { >> + if (map->rst == id) >> + return map->st; >> + >> + map++; >> + } >> + >> + return id; >> +} >> + >> +/* >> + * Note that status will not change until clocks are on, and clocks >> cannot be >> + * enabled until reset is deasserted. Consumer drivers must check status >> + * separately after enabling clocks. >> + */ >> +static int omap_reset_deassert(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >> + u32 v; >> + int st_bit = id; >> + bool has_rstst; >> + >> + /* check the current status to avoid de-asserting the line twice */ >> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >> + if (!(v & BIT(id))) >> + return -EEXIST; >> + >> + has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST); >> + >> + if (has_rstst) { >> + st_bit = omap_reset_get_st_bit(reset, id); >> + >> + /* Clear the reset status by writing 1 to the status bit */ >> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); >> + v |= 1 << st_bit; >> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstst); >> + } >> + >> + /* de-assert the reset control line */ >> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >> + v &= ~(1 << id); >> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl); >> + >> + return 0; >> +} >> + >> +static const struct reset_control_ops omap_reset_ops = { >> + .assert = omap_reset_assert, >> + .deassert = omap_reset_deassert, >> + .status = omap_reset_status, >> +}; >> + >> +static int omap_prm_reset_probe(struct platform_device *pdev, >> + struct omap_prm *prm) >> +{ >> + struct omap_reset_data *reset; >> + >> + /* >> + * Check if we have resets. If either rstctl or rstst is >> + * non-zero, we have reset registers in place. Additionally >> + * the flag OMAP_PRM_NO_RSTST implies that we have resets. >> + */ >> + if (!prm->data->rstctl && !prm->data->rstst && >> + !(prm->data->flags & OMAP_PRM_NO_RSTST)) >> + return 0; >> + >> + reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL); >> + if (!reset) >> + return -ENOMEM; >> + >> + reset->rcdev.owner = THIS_MODULE; >> + reset->rcdev.ops = &omap_reset_ops; >> + reset->rcdev.of_node = pdev->dev.of_node; >> + reset->rcdev.nr_resets = OMAP_MAX_RESETS; >> + >> + reset->prm = prm; >> + >> + return devm_reset_controller_register(&pdev->dev, &reset->rcdev); >> +} >> + >> +static int omap_prm_probe(struct platform_device *pdev) >> +{ >> + struct resource *res; >> + const struct omap_prm_data *data; >> + struct omap_prm *prm; >> + const struct of_device_id *match; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) >> + return -ENODEV; >> + >> + match = of_match_device(omap_prm_id_table, &pdev->dev); >> + if (!match) >> + return -ENOTSUPP; >> + >> + prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL); >> + if (!prm) >> + return -ENOMEM; >> + >> + data = match->data; >> + >> + while (data->base != res->start) { >> + if (!data->base) >> + return -EINVAL; >> + data++; >> + } >> + >> + prm->data = data; >> + >> + prm->base = devm_ioremap_resource(&pdev->dev, res); >> + if (!prm->base) >> + return -ENOMEM; >> + >> + return omap_prm_reset_probe(pdev, prm); >> +} >> + >> +static struct platform_driver omap_prm_driver = { >> + .probe = omap_prm_probe, >> + .driver = { >> + .name = KBUILD_MODNAME, >> + .of_match_table = omap_prm_id_table, >> + }, >> +}; >> +builtin_platform_driver(omap_prm_driver); >> + >> +MODULE_ALIAS("platform:prm"); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_DESCRIPTION("omap2+ prm driver"); > > It is a builtin_platform_driver so do we need the MODULE*? Well, thats debatable, however some existing drivers do introduce this even if they are builtin. -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tero, On 8/19/19 4:28 AM, Tero Kristo wrote: > On 08/08/2019 07:35, Keerthy wrote: >> >> >> On 07/08/19 1:18 PM, Tero Kristo wrote: >>> Add new binding for OMAP PRM (Power and Reset Manager) instances. Each >>> of these will act as a power domain controller and potentially as a >>> reset >>> provider. >>> >>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>> --- >>> .../devicetree/bindings/arm/omap/prm-inst.txt | 24 >>> ++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/arm/omap/prm-inst.txt >>> >>> diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt >>> b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt >>> new file mode 100644 >>> index 0000000..e0ae87b >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt >>> @@ -0,0 +1,24 @@ >>> +OMAP PRM instance bindings >>> + >>> +Power and Reset Manager is an IP block on OMAP family of devices which >>> +handle the power domains and their current state, and provide reset >>> +handling for the domains and/or separate IP blocks under the power >>> domain >>> +hierarchy. So, I see that you are adding these as flat nodes directly under the prm nodes where we have the clocks and clock_domains. Are you anticipating a single DT node and/or driver managing both power-domains and resets? >>> + >>> +Required properties: >>> +- compatible: Must be one of: >>> + "ti,am3-prm-inst" >>> + "ti,am4-prm-inst" >>> + "ti,omap4-prm-inst" >>> + "ti,omap5-prm-inst" >>> + "ti,dra7-prm-inst" What about OMAP2, OMAP3, DM814x, DM816x? regards Suman >>> +- reg: Contains PRM instance register address range >>> + (base address and length) >> >> How about reset-cells property, Isn't that a mandatory property? > > It is optional, but you are right, should be added to this. > > -Tero > >> >>> + >>> +Example: >>> + >>> +prm_dsp2: prm@1b00 { >>> + compatible = "ti,dra7-prm-inst"; >>> + reg = <0x1b00 0x40>; >>> + #reset-cells = <1>; >>> +}; >>> > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tero, On 8/19/19 4:32 AM, Tero Kristo wrote: > On 08/08/2019 08:26, Keerthy wrote: >> >> >> On 07/08/19 1:18 PM, Tero Kristo wrote: >>> Add initial PRM (Power and Reset Management) driver for TI OMAP class >>> SoCs. Initially this driver only supports reset control, but can be >>> extended to support rest of the functionality, like powerdomain >>> control, PRCM irq support etc. >>> >>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>> --- >>> arch/arm/mach-omap2/Kconfig | 1 + >>> drivers/soc/ti/Makefile | 1 + >>> drivers/soc/ti/omap_prm.c | 216 >>> ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 218 insertions(+) >>> create mode 100644 drivers/soc/ti/omap_prm.c >>> >>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig >>> index fdb6743..42ad063 100644 >>> --- a/arch/arm/mach-omap2/Kconfig >>> +++ b/arch/arm/mach-omap2/Kconfig >>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS >>> select TI_SYSC >>> select OMAP_IRQCHIP >>> select CLKSRC_TI_32K >>> + select RESET_CONTROLLER Use ARCH_HAS_RESET_CONTROLLER instead. >>> help >>> Systems based on OMAP2, OMAP3, OMAP4 or OMAP5 >>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile >>> index b3868d3..788b5cd 100644 >>> --- a/drivers/soc/ti/Makefile >>> +++ b/drivers/soc/ti/Makefile >>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS) += knav_qmss.o >>> knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o >>> obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA) += knav_dma.o >>> obj-$(CONFIG_AMX3_PM) += pm33xx.o >>> +obj-$(CONFIG_ARCH_OMAP2PLUS) += omap_prm.o >>> obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o >>> obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o >>> obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN) += ti_sci_inta_msi.o >>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c >>> new file mode 100644 >>> index 0000000..7c89eb8 >>> --- /dev/null >>> +++ b/drivers/soc/ti/omap_prm.c >>> @@ -0,0 +1,216 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * OMAP2+ PRM driver >>> + * >>> + * Copyright (C) 2019 Texas Instruments Incorporated - >>> http://www.ti.com/ >>> + * Tero Kristo <t-kristo@ti.com> >>> + */ >>> + >>> +#include <linux/kernel.h> >>> +#include <linux/module.h> >>> +#include <linux/device.h> >>> +#include <linux/io.h> >>> +#include <linux/of.h> >>> +#include <linux/of_device.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/reset-controller.h> >>> +#include <linux/delay.h> >>> + >>> +struct omap_rst_map { >>> + s8 rst; >>> + s8 st; >>> +}; >>> + >>> +struct omap_prm_data { >>> + u32 base; >>> + const char *name; >>> + u16 pwstctrl; >>> + u16 pwstst; >>> + u16 rstctl; Minor nit, can you use rstctrl instead here so that it is in sync with the other variables and with the register names used in TRM. >>> + u16 rstst; >>> + struct omap_rst_map *rstmap; >>> + u8 flags; >>> +}; >>> + >>> +struct omap_prm { >>> + const struct omap_prm_data *data; >>> + void __iomem *base; >>> +}; >>> + >>> +struct omap_reset_data { >>> + struct reset_controller_dev rcdev; >>> + struct omap_prm *prm; >>> +}; >>> + >>> +#define to_omap_reset_data(p) container_of((p), struct >>> omap_reset_data, rcdev) >>> + >>> +#define OMAP_MAX_RESETS 8 >>> +#define OMAP_RESET_MAX_WAIT 10000 >>> + >>> +#define OMAP_PRM_NO_RSTST BIT(0) >>> + >>> +static const struct of_device_id omap_prm_id_table[] = { >>> + { }, >>> +}; >> >> This table is blank and we are doing of_match_device against it. > > Yes, it gets populated with other patches in series, one entry per soc. > >> >>> + >>> +static int omap_reset_status(struct reset_controller_dev *rcdev, >>> + unsigned long id) >>> +{ >>> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >>> + u32 v; >>> + >>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); >>> + v &= 1 << id; >>> + v >>= id; >>> + >>> + return v; >>> +} >>> + >>> +static int omap_reset_assert(struct reset_controller_dev *rcdev, >>> + unsigned long id) >>> +{ >>> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >>> + u32 v; >>> + >>> + /* assert the reset control line */ >>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >>> + v |= 1 << id; >>> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl); >>> + >>> + return 0; >>> +} >>> + >>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset, >>> + unsigned long id) >>> +{ >>> + struct omap_rst_map *map = reset->prm->data->rstmap; >>> + >>> + while (map && map->rst >= 0) { >>> + if (map->rst == id) >>> + return map->st; >>> + >>> + map++; >>> + } >>> + >>> + return id; >>> +} >>> + >>> +/* >>> + * Note that status will not change until clocks are on, and clocks >>> cannot be >>> + * enabled until reset is deasserted. Consumer drivers must check >>> status >>> + * separately after enabling clocks. >>> + */ >>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev, >>> + unsigned long id) >>> +{ >>> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >>> + u32 v; >>> + int st_bit = id; No need to initialize this, will always get overwritten below. >>> + bool has_rstst; >>> + >>> + /* check the current status to avoid de-asserting the line twice */ >>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >>> + if (!(v & BIT(id))) >>> + return -EEXIST; >>> + >>> + has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST); >>> + >>> + if (has_rstst) { >>> + st_bit = omap_reset_get_st_bit(reset, id); >>> + >>> + /* Clear the reset status by writing 1 to the status bit */ >>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); >>> + v |= 1 << st_bit; >>> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstst); >>> + } >>> + >>> + /* de-assert the reset control line */ >>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >>> + v &= ~(1 << id); >>> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct reset_control_ops omap_reset_ops = { >>> + .assert = omap_reset_assert, >>> + .deassert = omap_reset_deassert, >>> + .status = omap_reset_status, >>> +}; >>> + >>> +static int omap_prm_reset_probe(struct platform_device *pdev, >>> + struct omap_prm *prm) Call this omap_prm_reset_init or something similar to avoid confusion. >>> +{ >>> + struct omap_reset_data *reset; >>> + >>> + /* >>> + * Check if we have resets. If either rstctl or rstst is >>> + * non-zero, we have reset registers in place. Additionally >>> + * the flag OMAP_PRM_NO_RSTST implies that we have resets. >>> + */ >>> + if (!prm->data->rstctl && !prm->data->rstst && >>> + !(prm->data->flags & OMAP_PRM_NO_RSTST)) >>> + return 0; >>> + >>> + reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL); >>> + if (!reset) >>> + return -ENOMEM; >>> + >>> + reset->rcdev.owner = THIS_MODULE; >>> + reset->rcdev.ops = &omap_reset_ops; >>> + reset->rcdev.of_node = pdev->dev.of_node; >>> + reset->rcdev.nr_resets = OMAP_MAX_RESETS; Suggest adding a number of resets to prm->data, and using it so that we don't even entertain any resets beyond the actual number of resets. You actually seem to be using the bit-position directly in client data instead of a reset number. I am not sure if this is accepted practice with reset controllers, do you incur any memory wastage? >>> + >>> + reset->prm = prm; >>> + >>> + return devm_reset_controller_register(&pdev->dev, &reset->rcdev); >>> +} >>> + >>> +static int omap_prm_probe(struct platform_device *pdev) >>> +{ >>> + struct resource *res; >>> + const struct omap_prm_data *data; >>> + struct omap_prm *prm; >>> + const struct of_device_id *match; >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + if (!res) >>> + return -ENODEV; >>> + >>> + match = of_match_device(omap_prm_id_table, &pdev->dev); >>> + if (!match) >>> + return -ENOTSUPP; Use of_device_get_match_data() instead to do both match and get the data. That can perhaps be the first block. >>> + >>> + prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL); >>> + if (!prm) >>> + return -ENOMEM; Perhaps move the allocate after the match check to streamline. >>> + >>> + data = match->data; >>> + >>> + while (data->base != res->start) { >>> + if (!data->base) >>> + return -EINVAL; >>> + data++; >>> + } >>> + >>> + prm->data = data; >>> + >>> + prm->base = devm_ioremap_resource(&pdev->dev, res); >>> + if (!prm->base) >>> + return -ENOMEM; >>> + >>> + return omap_prm_reset_probe(pdev, prm); >>> +} >>> + >>> +static struct platform_driver omap_prm_driver = { >>> + .probe = omap_prm_probe, >>> + .driver = { >>> + .name = KBUILD_MODNAME, >>> + .of_match_table = omap_prm_id_table, >>> + }, >>> +}; >>> +builtin_platform_driver(omap_prm_driver); >>> + >>> +MODULE_ALIAS("platform:prm"); Drop this and use MODULE_DEVICE_TABLE instead on omap_prm_id_table if retaining, but I don't think these need to be defined. regards Suman >>> +MODULE_LICENSE("GPL v2"); >>> +MODULE_DESCRIPTION("omap2+ prm driver"); >> >> It is a builtin_platform_driver so do we need the MODULE*? > > Well, thats debatable, however some existing drivers do introduce this > even if they are builtin. > > -Tero > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tero, On 8/7/19 2:48 AM, Tero Kristo wrote: > Hi, > > This series adds OMAP PRM driver which initially supports only reset > handling. Later on, power domain support can be added to this to get > rid of the current OMAP power domain handling code which resides > under the mach-omap2 platform directory. Initially, reset data is > added for AM3, OMAP4 and DRA7 SoCs. Wakeup M3 remoteproc driver is fully upstream, so we should be able to test that driver as well if you can add the AM4 data. That will also unblock my PRUSS. If you can add the data to others as well, it will help in easier migration of the individual drivers, otherwise the ti-sysc interconnect, hwmod, and hwmod reset data combinations will all have to be supported in code. regards Suman > > I've been testing the reset handling logic with OMAP remoteproc > driver which has been converted to use generic reset framework. This > part is a work in progress, so will be posting patches from that part > later on. > > -Tero > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >
On 20.8.2019 2.01, Suman Anna wrote: > Hi Tero, > > On 8/19/19 4:32 AM, Tero Kristo wrote: >> On 08/08/2019 08:26, Keerthy wrote: >>> >>> >>> On 07/08/19 1:18 PM, Tero Kristo wrote: >>>> Add initial PRM (Power and Reset Management) driver for TI OMAP class >>>> SoCs. Initially this driver only supports reset control, but can be >>>> extended to support rest of the functionality, like powerdomain >>>> control, PRCM irq support etc. >>>> >>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>> --- >>>> arch/arm/mach-omap2/Kconfig | 1 + >>>> drivers/soc/ti/Makefile | 1 + >>>> drivers/soc/ti/omap_prm.c | 216 >>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 218 insertions(+) >>>> create mode 100644 drivers/soc/ti/omap_prm.c >>>> >>>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig >>>> index fdb6743..42ad063 100644 >>>> --- a/arch/arm/mach-omap2/Kconfig >>>> +++ b/arch/arm/mach-omap2/Kconfig >>>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS >>>> select TI_SYSC >>>> select OMAP_IRQCHIP >>>> select CLKSRC_TI_32K >>>> + select RESET_CONTROLLER > > Use ARCH_HAS_RESET_CONTROLLER instead. Ok. > >>>> help >>>> Systems based on OMAP2, OMAP3, OMAP4 or OMAP5 >>>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile >>>> index b3868d3..788b5cd 100644 >>>> --- a/drivers/soc/ti/Makefile >>>> +++ b/drivers/soc/ti/Makefile >>>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS) += knav_qmss.o >>>> knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o >>>> obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA) += knav_dma.o >>>> obj-$(CONFIG_AMX3_PM) += pm33xx.o >>>> +obj-$(CONFIG_ARCH_OMAP2PLUS) += omap_prm.o >>>> obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o >>>> obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o >>>> obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN) += ti_sci_inta_msi.o >>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c >>>> new file mode 100644 >>>> index 0000000..7c89eb8 >>>> --- /dev/null >>>> +++ b/drivers/soc/ti/omap_prm.c >>>> @@ -0,0 +1,216 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * OMAP2+ PRM driver >>>> + * >>>> + * Copyright (C) 2019 Texas Instruments Incorporated - >>>> http://www.ti.com/ >>>> + * Tero Kristo <t-kristo@ti.com> >>>> + */ >>>> + >>>> +#include <linux/kernel.h> >>>> +#include <linux/module.h> >>>> +#include <linux/device.h> >>>> +#include <linux/io.h> >>>> +#include <linux/of.h> >>>> +#include <linux/of_device.h> >>>> +#include <linux/platform_device.h> >>>> +#include <linux/reset-controller.h> >>>> +#include <linux/delay.h> >>>> + >>>> +struct omap_rst_map { >>>> + s8 rst; >>>> + s8 st; >>>> +}; >>>> + >>>> +struct omap_prm_data { >>>> + u32 base; >>>> + const char *name; >>>> + u16 pwstctrl; >>>> + u16 pwstst; >>>> + u16 rstctl; > > Minor nit, can you use rstctrl instead here so that it is in sync with > the other variables and with the register names used in TRM. Ok. > >>>> + u16 rstst; >>>> + struct omap_rst_map *rstmap; >>>> + u8 flags; >>>> +}; >>>> + >>>> +struct omap_prm { >>>> + const struct omap_prm_data *data; >>>> + void __iomem *base; >>>> +}; >>>> + >>>> +struct omap_reset_data { >>>> + struct reset_controller_dev rcdev; >>>> + struct omap_prm *prm; >>>> +}; >>>> + >>>> +#define to_omap_reset_data(p) container_of((p), struct >>>> omap_reset_data, rcdev) >>>> + >>>> +#define OMAP_MAX_RESETS 8 >>>> +#define OMAP_RESET_MAX_WAIT 10000 >>>> + >>>> +#define OMAP_PRM_NO_RSTST BIT(0) >>>> + >>>> +static const struct of_device_id omap_prm_id_table[] = { >>>> + { }, >>>> +}; >>> >>> This table is blank and we are doing of_match_device against it. >> >> Yes, it gets populated with other patches in series, one entry per soc. >> >>> >>>> + >>>> +static int omap_reset_status(struct reset_controller_dev *rcdev, >>>> + unsigned long id) >>>> +{ >>>> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >>>> + u32 v; >>>> + >>>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); >>>> + v &= 1 << id; >>>> + v >>= id; >>>> + >>>> + return v; >>>> +} >>>> + >>>> +static int omap_reset_assert(struct reset_controller_dev *rcdev, >>>> + unsigned long id) >>>> +{ >>>> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >>>> + u32 v; >>>> + >>>> + /* assert the reset control line */ >>>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >>>> + v |= 1 << id; >>>> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset, >>>> + unsigned long id) >>>> +{ >>>> + struct omap_rst_map *map = reset->prm->data->rstmap; >>>> + >>>> + while (map && map->rst >= 0) { >>>> + if (map->rst == id) >>>> + return map->st; >>>> + >>>> + map++; >>>> + } >>>> + >>>> + return id; >>>> +} >>>> + >>>> +/* >>>> + * Note that status will not change until clocks are on, and clocks >>>> cannot be >>>> + * enabled until reset is deasserted. Consumer drivers must check >>>> status >>>> + * separately after enabling clocks. >>>> + */ >>>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev, >>>> + unsigned long id) >>>> +{ >>>> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >>>> + u32 v; >>>> + int st_bit = id; > > No need to initialize this, will always get overwritten below. Hmm right, must be a leftover from some earlier code. > >>>> + bool has_rstst; >>>> + >>>> + /* check the current status to avoid de-asserting the line twice */ >>>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >>>> + if (!(v & BIT(id))) >>>> + return -EEXIST; >>>> + >>>> + has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST); >>>> + >>>> + if (has_rstst) { >>>> + st_bit = omap_reset_get_st_bit(reset, id); >>>> + >>>> + /* Clear the reset status by writing 1 to the status bit */ >>>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); >>>> + v |= 1 << st_bit; >>>> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstst); >>>> + } >>>> + >>>> + /* de-assert the reset control line */ >>>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >>>> + v &= ~(1 << id); >>>> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct reset_control_ops omap_reset_ops = { >>>> + .assert = omap_reset_assert, >>>> + .deassert = omap_reset_deassert, >>>> + .status = omap_reset_status, >>>> +}; >>>> + >>>> +static int omap_prm_reset_probe(struct platform_device *pdev, >>>> + struct omap_prm *prm) > > Call this omap_prm_reset_init or something similar to avoid confusion. Ok, can change this. > >>>> +{ >>>> + struct omap_reset_data *reset; >>>> + >>>> + /* >>>> + * Check if we have resets. If either rstctl or rstst is >>>> + * non-zero, we have reset registers in place. Additionally >>>> + * the flag OMAP_PRM_NO_RSTST implies that we have resets. >>>> + */ >>>> + if (!prm->data->rstctl && !prm->data->rstst && >>>> + !(prm->data->flags & OMAP_PRM_NO_RSTST)) >>>> + return 0; >>>> + >>>> + reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL); >>>> + if (!reset) >>>> + return -ENOMEM; >>>> + >>>> + reset->rcdev.owner = THIS_MODULE; >>>> + reset->rcdev.ops = &omap_reset_ops; >>>> + reset->rcdev.of_node = pdev->dev.of_node; >>>> + reset->rcdev.nr_resets = OMAP_MAX_RESETS; > > Suggest adding a number of resets to prm->data, and using it so that we > don't even entertain any resets beyond the actual number of resets. Hmm why bother? Accessing a stale reset bit will just cause access to a reserved bit in the reset register, doing basically nothing. Also, this would not work for am3/am4 wkup, as there is a single reset bit at an arbitrary position. > > You actually seem to be using the bit-position directly in client data > instead of a reset number. I am not sure if this is accepted practice > with reset controllers, do you incur any memory wastage? Reset numbering almost always seems to start from 0, I think the only exception to this is wkup_m3 on am3/am4. Introducing an additional arbitrary mapping for this doesn't seem to make any sense. Also, resets are allocated on-need-basis, so it only allocates one instance for the reset control whatever the index is. > >>>> + >>>> + reset->prm = prm; >>>> + >>>> + return devm_reset_controller_register(&pdev->dev, &reset->rcdev); >>>> +} >>>> + >>>> +static int omap_prm_probe(struct platform_device *pdev) >>>> +{ >>>> + struct resource *res; >>>> + const struct omap_prm_data *data; >>>> + struct omap_prm *prm; >>>> + const struct of_device_id *match; >>>> + >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> + if (!res) >>>> + return -ENODEV; >>>> + >>>> + match = of_match_device(omap_prm_id_table, &pdev->dev); >>>> + if (!match) >>>> + return -ENOTSUPP; > > Use of_device_get_match_data() instead to do both match and get the > data. That can perhaps be the first block. > >>>> + >>>> + prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL); >>>> + if (!prm) >>>> + return -ENOMEM; > > Perhaps move the allocate after the match check to streamline. Ok, will check these two out. > >>>> + >>>> + data = match->data; >>>> + >>>> + while (data->base != res->start) { >>>> + if (!data->base) >>>> + return -EINVAL; >>>> + data++; >>>> + } >>>> + >>>> + prm->data = data; >>>> + >>>> + prm->base = devm_ioremap_resource(&pdev->dev, res); >>>> + if (!prm->base) >>>> + return -ENOMEM; >>>> + >>>> + return omap_prm_reset_probe(pdev, prm); >>>> +} >>>> + >>>> +static struct platform_driver omap_prm_driver = { >>>> + .probe = omap_prm_probe, >>>> + .driver = { >>>> + .name = KBUILD_MODNAME, >>>> + .of_match_table = omap_prm_id_table, >>>> + }, >>>> +}; >>>> +builtin_platform_driver(omap_prm_driver); >>>> + >>>> +MODULE_ALIAS("platform:prm"); > > Drop this and use MODULE_DEVICE_TABLE instead on omap_prm_id_table if > retaining, but I don't think these need to be defined. Ok, will ditch them. -Tero > > regards > Suman > >>>> +MODULE_LICENSE("GPL v2"); >>>> +MODULE_DESCRIPTION("omap2+ prm driver"); >>> >>> It is a builtin_platform_driver so do we need the MODULE*? >> >> Well, thats debatable, however some existing drivers do introduce this >> even if they are builtin. >> >> -Tero >> -- > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 20.8.2019 0.28, Suman Anna wrote: > Hi Tero, > > On 8/19/19 4:28 AM, Tero Kristo wrote: >> On 08/08/2019 07:35, Keerthy wrote: >>> >>> >>> On 07/08/19 1:18 PM, Tero Kristo wrote: >>>> Add new binding for OMAP PRM (Power and Reset Manager) instances. Each >>>> of these will act as a power domain controller and potentially as a >>>> reset >>>> provider. >>>> >>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>> --- >>>> .../devicetree/bindings/arm/omap/prm-inst.txt | 24 >>>> ++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> create mode 100644 >>>> Documentation/devicetree/bindings/arm/omap/prm-inst.txt >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt >>>> b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt >>>> new file mode 100644 >>>> index 0000000..e0ae87b >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt >>>> @@ -0,0 +1,24 @@ >>>> +OMAP PRM instance bindings >>>> + >>>> +Power and Reset Manager is an IP block on OMAP family of devices which >>>> +handle the power domains and their current state, and provide reset >>>> +handling for the domains and/or separate IP blocks under the power >>>> domain >>>> +hierarchy. > > So, I see that you are adding these as flat nodes directly under the prm > nodes where we have the clocks and clock_domains. Are you anticipating a > single DT node and/or driver managing both power-domains and resets? Single DT node + driver for both power-domains / resets. You can probably see from the driver code that the reset probing is called separately from the main probe function; power domain support should be added in similar manner under it. > >>>> + >>>> +Required properties: >>>> +- compatible: Must be one of: >>>> + "ti,am3-prm-inst" >>>> + "ti,am4-prm-inst" >>>> + "ti,omap4-prm-inst" >>>> + "ti,omap5-prm-inst" >>>> + "ti,dra7-prm-inst" > > What about OMAP2, OMAP3, DM814x, DM816x? OMAP2/3 architectures are slightly different beast to tackle, so planning to work on those later on. -Tero > > regards > Suman > >>>> +- reg: Contains PRM instance register address range >>>> + (base address and length) >>> >>> How about reset-cells property, Isn't that a mandatory property? >> >> It is optional, but you are right, should be added to this. >> >> -Tero >> >>> >>>> + >>>> +Example: >>>> + >>>> +prm_dsp2: prm@1b00 { >>>> + compatible = "ti,dra7-prm-inst"; >>>> + reg = <0x1b00 0x40>; >>>> + #reset-cells = <1>; >>>> +}; >>>> >> >> -- > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 20.8.2019 2.20, Suman Anna wrote: > Hi Tero, > > On 8/7/19 2:48 AM, Tero Kristo wrote: >> Hi, >> >> This series adds OMAP PRM driver which initially supports only reset >> handling. Later on, power domain support can be added to this to get >> rid of the current OMAP power domain handling code which resides >> under the mach-omap2 platform directory. Initially, reset data is >> added for AM3, OMAP4 and DRA7 SoCs. > > Wakeup M3 remoteproc driver is fully upstream, so we should be able to > test that driver as well if you can add the AM4 data. That will also > unblock my PRUSS. > > If you can add the data to others as well, it will help in easier > migration of the individual drivers, otherwise the ti-sysc interconnect, > hwmod, and hwmod reset data combinations will all have to be supported > in code. Ok, so you are saying you would need the PRM data for am4 in addition? I can generate that one also. -Tero > > regards > Suman > >> >> I've been testing the reset handling logic with OMAP remoteproc >> driver which has been converted to use generic reset framework. This >> part is a work in progress, so will be posting patches from that part >> later on. >> >> -Tero >> >> -- >> > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 8/20/19 2:37 AM, Tero Kristo wrote: > On 20.8.2019 2.01, Suman Anna wrote: >> Hi Tero, >> >> On 8/19/19 4:32 AM, Tero Kristo wrote: >>> On 08/08/2019 08:26, Keerthy wrote: >>>> >>>> >>>> On 07/08/19 1:18 PM, Tero Kristo wrote: >>>>> Add initial PRM (Power and Reset Management) driver for TI OMAP class >>>>> SoCs. Initially this driver only supports reset control, but can be >>>>> extended to support rest of the functionality, like powerdomain >>>>> control, PRCM irq support etc. >>>>> >>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>>> --- >>>>> arch/arm/mach-omap2/Kconfig | 1 + >>>>> drivers/soc/ti/Makefile | 1 + >>>>> drivers/soc/ti/omap_prm.c | 216 >>>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 218 insertions(+) >>>>> create mode 100644 drivers/soc/ti/omap_prm.c >>>>> >>>>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig >>>>> index fdb6743..42ad063 100644 >>>>> --- a/arch/arm/mach-omap2/Kconfig >>>>> +++ b/arch/arm/mach-omap2/Kconfig >>>>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS >>>>> select TI_SYSC >>>>> select OMAP_IRQCHIP >>>>> select CLKSRC_TI_32K >>>>> + select RESET_CONTROLLER >> >> Use ARCH_HAS_RESET_CONTROLLER instead. > > Ok. > >> >>>>> help >>>>> Systems based on OMAP2, OMAP3, OMAP4 or OMAP5 >>>>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile >>>>> index b3868d3..788b5cd 100644 >>>>> --- a/drivers/soc/ti/Makefile >>>>> +++ b/drivers/soc/ti/Makefile >>>>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS) += >>>>> knav_qmss.o >>>>> knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o >>>>> obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA) += knav_dma.o >>>>> obj-$(CONFIG_AMX3_PM) += pm33xx.o >>>>> +obj-$(CONFIG_ARCH_OMAP2PLUS) += omap_prm.o >>>>> obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o >>>>> obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o >>>>> obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN) += ti_sci_inta_msi.o >>>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c >>>>> new file mode 100644 >>>>> index 0000000..7c89eb8 >>>>> --- /dev/null >>>>> +++ b/drivers/soc/ti/omap_prm.c >>>>> @@ -0,0 +1,216 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> +/* >>>>> + * OMAP2+ PRM driver >>>>> + * >>>>> + * Copyright (C) 2019 Texas Instruments Incorporated - >>>>> http://www.ti.com/ >>>>> + * Tero Kristo <t-kristo@ti.com> >>>>> + */ >>>>> + >>>>> +#include <linux/kernel.h> >>>>> +#include <linux/module.h> >>>>> +#include <linux/device.h> >>>>> +#include <linux/io.h> >>>>> +#include <linux/of.h> >>>>> +#include <linux/of_device.h> >>>>> +#include <linux/platform_device.h> >>>>> +#include <linux/reset-controller.h> >>>>> +#include <linux/delay.h> >>>>> + >>>>> +struct omap_rst_map { >>>>> + s8 rst; >>>>> + s8 st; >>>>> +}; >>>>> + >>>>> +struct omap_prm_data { >>>>> + u32 base; >>>>> + const char *name; >>>>> + u16 pwstctrl; >>>>> + u16 pwstst; >>>>> + u16 rstctl; >> >> Minor nit, can you use rstctrl instead here so that it is in sync with >> the other variables and with the register names used in TRM. > > Ok. > >> >>>>> + u16 rstst; >>>>> + struct omap_rst_map *rstmap; >>>>> + u8 flags; >>>>> +}; >>>>> + >>>>> +struct omap_prm { >>>>> + const struct omap_prm_data *data; >>>>> + void __iomem *base; >>>>> +}; >>>>> + >>>>> +struct omap_reset_data { >>>>> + struct reset_controller_dev rcdev; >>>>> + struct omap_prm *prm; >>>>> +}; >>>>> + >>>>> +#define to_omap_reset_data(p) container_of((p), struct >>>>> omap_reset_data, rcdev) >>>>> + >>>>> +#define OMAP_MAX_RESETS 8 >>>>> +#define OMAP_RESET_MAX_WAIT 10000 >>>>> + >>>>> +#define OMAP_PRM_NO_RSTST BIT(0) >>>>> + >>>>> +static const struct of_device_id omap_prm_id_table[] = { >>>>> + { }, >>>>> +}; >>>> >>>> This table is blank and we are doing of_match_device against it. >>> >>> Yes, it gets populated with other patches in series, one entry per soc. >>> >>>> >>>>> + >>>>> +static int omap_reset_status(struct reset_controller_dev *rcdev, >>>>> + unsigned long id) >>>>> +{ >>>>> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >>>>> + u32 v; >>>>> + >>>>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); >>>>> + v &= 1 << id; >>>>> + v >>= id; >>>>> + >>>>> + return v; >>>>> +} >>>>> + >>>>> +static int omap_reset_assert(struct reset_controller_dev *rcdev, >>>>> + unsigned long id) >>>>> +{ >>>>> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >>>>> + u32 v; >>>>> + >>>>> + /* assert the reset control line */ >>>>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >>>>> + v |= 1 << id; >>>>> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset, >>>>> + unsigned long id) >>>>> +{ >>>>> + struct omap_rst_map *map = reset->prm->data->rstmap; >>>>> + >>>>> + while (map && map->rst >= 0) { >>>>> + if (map->rst == id) >>>>> + return map->st; >>>>> + >>>>> + map++; >>>>> + } >>>>> + >>>>> + return id; >>>>> +} >>>>> + >>>>> +/* >>>>> + * Note that status will not change until clocks are on, and clocks >>>>> cannot be >>>>> + * enabled until reset is deasserted. Consumer drivers must check >>>>> status >>>>> + * separately after enabling clocks. >>>>> + */ >>>>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev, >>>>> + unsigned long id) >>>>> +{ >>>>> + struct omap_reset_data *reset = to_omap_reset_data(rcdev); >>>>> + u32 v; >>>>> + int st_bit = id; >> >> No need to initialize this, will always get overwritten below. > > Hmm right, must be a leftover from some earlier code. > >> >>>>> + bool has_rstst; >>>>> + >>>>> + /* check the current status to avoid de-asserting the line >>>>> twice */ >>>>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >>>>> + if (!(v & BIT(id))) >>>>> + return -EEXIST; >>>>> + >>>>> + has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST); >>>>> + >>>>> + if (has_rstst) { >>>>> + st_bit = omap_reset_get_st_bit(reset, id); >>>>> + >>>>> + /* Clear the reset status by writing 1 to the status bit */ >>>>> + v = readl_relaxed(reset->prm->base + >>>>> reset->prm->data->rstst); >>>>> + v |= 1 << st_bit; >>>>> + writel_relaxed(v, reset->prm->base + >>>>> reset->prm->data->rstst); >>>>> + } >>>>> + >>>>> + /* de-assert the reset control line */ >>>>> + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl); >>>>> + v &= ~(1 << id); >>>>> + writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static const struct reset_control_ops omap_reset_ops = { >>>>> + .assert = omap_reset_assert, >>>>> + .deassert = omap_reset_deassert, >>>>> + .status = omap_reset_status, >>>>> +}; >>>>> + >>>>> +static int omap_prm_reset_probe(struct platform_device *pdev, >>>>> + struct omap_prm *prm) >> >> Call this omap_prm_reset_init or something similar to avoid confusion. > > Ok, can change this. > >> >>>>> +{ >>>>> + struct omap_reset_data *reset; >>>>> + >>>>> + /* >>>>> + * Check if we have resets. If either rstctl or rstst is >>>>> + * non-zero, we have reset registers in place. Additionally >>>>> + * the flag OMAP_PRM_NO_RSTST implies that we have resets. >>>>> + */ >>>>> + if (!prm->data->rstctl && !prm->data->rstst && >>>>> + !(prm->data->flags & OMAP_PRM_NO_RSTST)) >>>>> + return 0; >>>>> + >>>>> + reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL); >>>>> + if (!reset) >>>>> + return -ENOMEM; >>>>> + >>>>> + reset->rcdev.owner = THIS_MODULE; >>>>> + reset->rcdev.ops = &omap_reset_ops; >>>>> + reset->rcdev.of_node = pdev->dev.of_node; >>>>> + reset->rcdev.nr_resets = OMAP_MAX_RESETS; >> >> Suggest adding a number of resets to prm->data, and using it so that we >> don't even entertain any resets beyond the actual number of resets. > > Hmm why bother? Accessing a stale reset bit will just cause access to a > reserved bit in the reset register, doing basically nothing. Also, this > would not work for am3/am4 wkup, as there is a single reset bit at an > arbitrary position. The generic convention seems to be defining a reset id value defined from include/dt-bindings/reset/ that can be used to match between the dt-nodes and the reset-controller driver. Philipp, Any comments? regards Suman > >> >> You actually seem to be using the bit-position directly in client data >> instead of a reset number. I am not sure if this is accepted practice >> with reset controllers, do you incur any memory wastage? > > Reset numbering almost always seems to start from 0, I think the only > exception to this is wkup_m3 on am3/am4. Introducing an additional > arbitrary mapping for this doesn't seem to make any sense. > > Also, resets are allocated on-need-basis, so it only allocates one > instance for the reset control whatever the index is. > >> >>>>> + >>>>> + reset->prm = prm; >>>>> + >>>>> + return devm_reset_controller_register(&pdev->dev, &reset->rcdev); >>>>> +} >>>>> + >>>>> +static int omap_prm_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct resource *res; >>>>> + const struct omap_prm_data *data; >>>>> + struct omap_prm *prm; >>>>> + const struct of_device_id *match; >>>>> + >>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> + if (!res) >>>>> + return -ENODEV; >>>>> + >>>>> + match = of_match_device(omap_prm_id_table, &pdev->dev); >>>>> + if (!match) >>>>> + return -ENOTSUPP; >> >> Use of_device_get_match_data() instead to do both match and get the >> data. That can perhaps be the first block. >> >>>>> + >>>>> + prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL); >>>>> + if (!prm) >>>>> + return -ENOMEM; >> >> Perhaps move the allocate after the match check to streamline. > > Ok, will check these two out. > >> >>>>> + >>>>> + data = match->data; >>>>> + >>>>> + while (data->base != res->start) { >>>>> + if (!data->base) >>>>> + return -EINVAL; >>>>> + data++; >>>>> + } >>>>> + >>>>> + prm->data = data; >>>>> + >>>>> + prm->base = devm_ioremap_resource(&pdev->dev, res); >>>>> + if (!prm->base) >>>>> + return -ENOMEM; >>>>> + >>>>> + return omap_prm_reset_probe(pdev, prm); >>>>> +} >>>>> + >>>>> +static struct platform_driver omap_prm_driver = { >>>>> + .probe = omap_prm_probe, >>>>> + .driver = { >>>>> + .name = KBUILD_MODNAME, >>>>> + .of_match_table = omap_prm_id_table, >>>>> + }, >>>>> +}; >>>>> +builtin_platform_driver(omap_prm_driver); >>>>> + >>>>> +MODULE_ALIAS("platform:prm"); >> >> Drop this and use MODULE_DEVICE_TABLE instead on omap_prm_id_table if >> retaining, but I don't think these need to be defined. > > Ok, will ditch them. > > -Tero > >> >> regards >> Suman >> >>>>> +MODULE_LICENSE("GPL v2"); >>>>> +MODULE_DESCRIPTION("omap2+ prm driver"); >>>> >>>> It is a builtin_platform_driver so do we need the MODULE*? >>> >>> Well, thats debatable, however some existing drivers do introduce this >>> even if they are builtin. >>> >>> -Tero >>> -- >> > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 8/20/19 2:54 AM, Tero Kristo wrote: > On 20.8.2019 2.20, Suman Anna wrote: >> Hi Tero, >> >> On 8/7/19 2:48 AM, Tero Kristo wrote: >>> Hi, >>> >>> This series adds OMAP PRM driver which initially supports only reset >>> handling. Later on, power domain support can be added to this to get >>> rid of the current OMAP power domain handling code which resides >>> under the mach-omap2 platform directory. Initially, reset data is >>> added for AM3, OMAP4 and DRA7 SoCs. >> >> Wakeup M3 remoteproc driver is fully upstream, so we should be able to >> test that driver as well if you can add the AM4 data. That will also >> unblock my PRUSS. >> >> If you can add the data to others as well, it will help in easier >> migration of the individual drivers, otherwise the ti-sysc interconnect, >> hwmod, and hwmod reset data combinations will all have to be supported >> in code. > > Ok, so you are saying you would need the PRM data for am4 in addition? I > can generate that one also. Yes, if you can include the data for AM4 and OMAP5 as well, then we can convert all the SoCs other than OMAP2/OMAP3 at the same time as per your comment on bindings. Almost all of the active remoteprocs will be covered by these. OMAP3 ISP driver is also fully upstream, so we would have to manage its legacy compatibility. regards Suman > > -Tero > >> >> regards >> Suman >> >>> >>> I've been testing the reset handling logic with OMAP remoteproc >>> driver which has been converted to use generic reset framework. This >>> part is a work in progress, so will be posting patches from that part >>> later on. >>> >>> -Tero >>> >>> -- >>> >> > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 8/21/19 10:10 AM, Philipp Zabel wrote: > On Tue, 2019-08-20 at 11:47 -0500, Suman Anna wrote: >> On 8/20/19 2:37 AM, Tero Kristo wrote: >>> On 20.8.2019 2.01, Suman Anna wrote: >>>> Hi Tero, >>>> >>>> On 8/19/19 4:32 AM, Tero Kristo wrote: > [...] >>>>>>> +{ >>>>>>> + struct omap_reset_data *reset; >>>>>>> + >>>>>>> + /* >>>>>>> + * Check if we have resets. If either rstctl or rstst is >>>>>>> + * non-zero, we have reset registers in place. Additionally >>>>>>> + * the flag OMAP_PRM_NO_RSTST implies that we have resets. >>>>>>> + */ >>>>>>> + if (!prm->data->rstctl && !prm->data->rstst && >>>>>>> + !(prm->data->flags & OMAP_PRM_NO_RSTST)) >>>>>>> + return 0; >>>>>>> + >>>>>>> + reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL); >>>>>>> + if (!reset) >>>>>>> + return -ENOMEM; >>>>>>> + >>>>>>> + reset->rcdev.owner = THIS_MODULE; >>>>>>> + reset->rcdev.ops = &omap_reset_ops; >>>>>>> + reset->rcdev.of_node = pdev->dev.of_node; >>>>>>> + reset->rcdev.nr_resets = OMAP_MAX_RESETS; >>>> >>>> Suggest adding a number of resets to prm->data, and using it so that we >>>> don't even entertain any resets beyond the actual number of resets. >>> >>> Hmm why bother? Accessing a stale reset bit will just cause access to a >>> reserved bit in the reset register, doing basically nothing. Also, this >>> would not work for am3/am4 wkup, as there is a single reset bit at an >>> arbitrary position. >> >> The generic convention seems to be defining a reset id value defined >> from include/dt-bindings/reset/ that can be used to match between the >> dt-nodes and the reset-controller driver. >> >> Philipp, >> Any comments? > > Are there only reset bits and reserved bits in the range accessible by > [0..OMAP_MAX_RESETS] or are ther bits with another function as well? Thanks Philipp, these are just reset bits and reserved bits. > If the latter is the case, I would prefer enumerating the resets in a > dt-bindings header, with the driver containing an enum -> reg/bit > position lookup table. > > In general, assuming the device tree contains no errors, this should not > matter much, but I think it is nice if the reset driver, even with a > misconfigured device tree, can't write into arbitrary bit fields. Tero, Can you add a check for this if possible? regards Suman
On 21.8.2019 18.45, Suman Anna wrote: > On 8/21/19 10:10 AM, Philipp Zabel wrote: >> On Tue, 2019-08-20 at 11:47 -0500, Suman Anna wrote: >>> On 8/20/19 2:37 AM, Tero Kristo wrote: >>>> On 20.8.2019 2.01, Suman Anna wrote: >>>>> Hi Tero, >>>>> >>>>> On 8/19/19 4:32 AM, Tero Kristo wrote: >> [...] >>>>>>>> +{ >>>>>>>> + struct omap_reset_data *reset; >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * Check if we have resets. If either rstctl or rstst is >>>>>>>> + * non-zero, we have reset registers in place. Additionally >>>>>>>> + * the flag OMAP_PRM_NO_RSTST implies that we have resets. >>>>>>>> + */ >>>>>>>> + if (!prm->data->rstctl && !prm->data->rstst && >>>>>>>> + !(prm->data->flags & OMAP_PRM_NO_RSTST)) >>>>>>>> + return 0; >>>>>>>> + >>>>>>>> + reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL); >>>>>>>> + if (!reset) >>>>>>>> + return -ENOMEM; >>>>>>>> + >>>>>>>> + reset->rcdev.owner = THIS_MODULE; >>>>>>>> + reset->rcdev.ops = &omap_reset_ops; >>>>>>>> + reset->rcdev.of_node = pdev->dev.of_node; >>>>>>>> + reset->rcdev.nr_resets = OMAP_MAX_RESETS; >>>>> >>>>> Suggest adding a number of resets to prm->data, and using it so that we >>>>> don't even entertain any resets beyond the actual number of resets. >>>> >>>> Hmm why bother? Accessing a stale reset bit will just cause access to a >>>> reserved bit in the reset register, doing basically nothing. Also, this >>>> would not work for am3/am4 wkup, as there is a single reset bit at an >>>> arbitrary position. >>> >>> The generic convention seems to be defining a reset id value defined >>> from include/dt-bindings/reset/ that can be used to match between the >>> dt-nodes and the reset-controller driver. >>> >>> Philipp, >>> Any comments? >> >> Are there only reset bits and reserved bits in the range accessible by >> [0..OMAP_MAX_RESETS] or are ther bits with another function as well? > > Thanks Philipp, these are just reset bits and reserved bits. > >> If the latter is the case, I would prefer enumerating the resets in a >> dt-bindings header, with the driver containing an enum -> reg/bit >> position lookup table. >> >> In general, assuming the device tree contains no errors, this should not >> matter much, but I think it is nice if the reset driver, even with a >> misconfigured device tree, can't write into arbitrary bit fields. > > Tero, > Can you add a check for this if possible? Well, I can enforce the usage of reset bit mapping, which I have already implemented for some SoCs like am33xx. If the specific ID is not found, I can bail out. So, basically in this example requesting reset at index 3 would succeed, but it would fail for any other ID; this would be direct HW bit mapping. -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki