TI Reset Driver adapted to the reset core
Subsystems (2)
All
I have made some big changes to this patchset so I did not put all the changes in the patches themselves.
In brief
- I removed the object data files for each SoC and moved the data into the DT. I updated the binding document as well
- The DT implementation creates a parent reset node with the base offset for each reset and the children within that parent declares the bit mask
- I created a xlate function in the reset driver to verify that reset data and that the phandle exists. The core xlate checks for the nr_resets which we don't care about since we are not indexed based.
- Made the driver a platform driver as well so this removed the prm_common patch as well as the header file.
I will add other TI devices once I feel comfortable that the design is acceptable.
Dan
RFC PATCH v2 1/6 drivers: reset: TI: SoC reset controller support.
The TI SoC reset controller support utilizes the reset controller framework to give device drivers or function drivers a common set of APIs to call to reset a module.
The reset-ti is a common interface to the reset framework.
The register data is retrieved during initialization
of the reset driver through the reset-ti-data
file. The array of data is associated with the compatible from the respective DT entry.
Once the data is available then this is derefenced within the common interface.
The device driver has the ability to assert, deassert or perform a complete reset.
This code was derived from previous work by Rajendra Nayak and Afzal Mohammed. The code was changed to adopt to the reset core and abstract away the SoC information.
--- drivers/reset/Kconfig | 1 + drivers/reset/Makefile | 1 + drivers/reset/ti/Kconfig | 8 ++ drivers/reset/ti/Makefile | 1 + drivers/reset/ti/reset-ti-data.h | 56 ++++++++ drivers/reset/ti/reset-ti.c | 267 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 334 insertions(+) create mode 100644 drivers/reset/ti/Kconfig create mode 100644 drivers/reset/ti/Makefile create mode 100644 drivers/reset/ti/reset-ti-data.h create mode 100644 drivers/reset/ti/reset-ti.c
drivers/reset/Kconfig +1 -0
@@ -13,3 +13,4 @@ menuconfig RESET_CONTROLLER If unsure, say no. source "drivers/reset/sti/Kconfig" +source "drivers/reset/ti/Kconfig"
drivers/reset/Makefile +1 -0
@@ -1,3 +1,4 @@ obj-$(CONFIG_RESET_CONTROLLER) += core.o obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o obj-$(CONFIG_ARCH_STI) += sti/ +obj-$(CONFIG_RESET_TI) += ti/
drivers/reset/ti/Kconfig new file +8 -0
@@ -0,0 +1,8 @@ +config RESET_TI + depends on RESET_CONTROLLER + bool "TI reset controller" + help + Reset controller support for TI SoC's + + Reset controller found in TI's AM series of SoC's like + AM335x and AM43x and OMAP SoC's like OMAP5 and DRA7
drivers/reset/ti/Makefile new file +1 -0
@@ -0,0 +1 @@ +obj-$(CONFIG_RESET_TI) += reset-ti.o
drivers/reset/ti/reset-ti-data.h new file +56 -0
56 lines not on this page · raw message
drivers/reset/ti/reset-ti.c new file +267 -0
267 lines not on this page · raw message
Signature · 1 line
1.7.9.5
Felipe Balbi
NAKRFC PATCH v2 1/6 drivers: reset: TI: SoC reset controller support.
Hi,
Quoted 11 lines · Dan Murphy
The TI SoC reset controller support utilizes the reset controller framework to give device drivers or function drivers a common set of APIs to call to reset a module.
The reset-ti is a common interface to the reset framework.
The register data is retrieved during initialization
of the reset driver through the reset-ti-data
file. The array of data is associated with the compatible from the respective DT entry.
Quoted 8 lines
Once the data is available then this is derefenced within the common interface.
The device driver has the ability to assert, deassert or perform a complete reset.
This code was derived from previous work by Rajendra Nayak and Afzal Mohammed. The code was changed to adopt to the reset core and abstract away the SoC information.
you should break your commit log at around 72 characters at most.
Quoted 19 lines
Signed-off-by Dan Murphy <[email protected]>--- drivers/reset/Kconfig | 1 + drivers/reset/Makefile | 1 + drivers/reset/ti/Kconfig | 8 ++ drivers/reset/ti/Makefile | 1 + drivers/reset/ti/reset-ti-data.h | 56 ++++++++ drivers/reset/ti/reset-ti.c | 267 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 334 insertions(+) create mode 100644 drivers/reset/ti/Kconfig create mode 100644 drivers/reset/ti/Makefile create mode 100644 drivers/reset/ti/reset-ti-data.h create mode 100644 drivers/reset/ti/reset-ti.cdrivers/reset/Kconfig +0
-0
drivers/reset/Kconfig menuconfig RESET_CONTROLLER · line 16
Quoted 4 lines
drivers/reset/Kconfig +1
-0@@ -13,3 +13,4 @@ menuconfig RESET_CONTROLLER If unsure, say no. source "drivers/reset/sti/Kconfig" +source "drivers/reset/ti/Kconfig"
this driver is so small that I'm not sure you need a directory for it.
drivers/reset/ti/Kconfig line 2
Quoted 8 lines
drivers/reset/ti/Kconfig new file +2
-0@@ -0,0 +1,8 @@ +config RESET_TI + depends on RESET_CONTROLLER
don't you want to depend on ARCH_OMAP || COMPILE_TEST ?
drivers/reset/ti/reset-ti-data.h line 4
Quoted 10 lines
drivers/reset/ti/reset-ti-data.h new file +4
-0@@ -0,0 +1,56 @@ +/* +* PRCM reset driver for TI SoC's +* +* Copyright 2014 Texas Instruments Inc.
this is not the TI aproved way for copyright notices. Yeah, nit-picking...
Quoted 22 lines
drivers/reset/ti/reset-ti-data.h new file +22
-0+* Author: Dan Murphy <[email protected]> +* +* 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. +*/ + +#ifndef _RESET_TI_DATA_H_ +#define _RESET_TI_DATA_H_ + +#include <linux/kernel.h> +#include <linux/reset-controller.h> + +/** +* struct ti_reset_reg_data - Structure of the reset register information +* for a particular SoC. +* @rstctrl_offs: This is the reset control offset value from +* from the parent reset node. +* @rstst_offs: This is the reset status offset value from +* from the parent reset node. +* @rstctrl_bit: This is the reset control bit for the module. +* @rstst_bit: This is the reset status bit for the module.
drivers/reset/ti/reset-ti-data.h line 37
Quoted 10 lines
drivers/reset/ti/reset-ti-data.h new file +10
-0+* +* This structure describes the reset register control and status offsets. +* The bits are also defined for the same. +*/ +struct ti_reset_reg_data { +void __iomem *reg_base; +u32 rstctrl_offs; +u32 rstst_offs; +u32 rstctrl_bit; +u32 rstst_bit;
this structure can be folded into struct ti_reset_data and all of that can be initialized during probe.
Quoted 9 lines
drivers/reset/ti/reset-ti-data.h new file +9
-0+}; + +/** +* struct ti_reset_data - Structure that contains the reset register data +* as well as the total number of resets for a particular SoC. +* @reg_data: Pointer to the register data structure. +* @nr_resets: Total number of resets for the SoC in the reset array. +* +* This structure contains a pointer to the register data and the modules
drivers/reset/ti/reset-ti-data.h line 56
Quoted 10 lines
drivers/reset/ti/reset-ti-data.h new file +10
-0+* register base. The number of resets and reset controller device data is +* stored within this structure. +* +*/ +struct ti_reset_data { +struct ti_reset_reg_data *reg_data; +struct reset_controller_dev rcdev; +}; + +#endif
this entire file can be moved into the .c file. It's too small and the only user for these new types is the .c driver anyway.
drivers/reset/ti/reset-ti.c line 6
Quoted 12 lines
drivers/reset/ti/reset-ti.c new file +6
-0@@ -0,0 +1,267 @@ +/* +* PRCM reset driver for TI SoC's +* +* Copyright 2014 Texas Instruments Inc. +* +* Author: Dan Murphy <[email protected]>
fix copyright notice here too
Quoted 11 lines
drivers/reset/ti/reset-ti.c new file +11
-0+* 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. +*/ + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_address.h>
drivers/reset/ti/reset-ti.c line 28
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/slab.h> + +#include "reset-ti-data.h" + +#define DRIVER_NAME "prcm_reset_ti" + +static struct ti_reset_data *ti_data;
NAK, you *really* don't need and don't to have this global here. It only creates problems. And avoid arguing that "there's only one reset controller anyway" because that has bitten us in the past. Also, it's not a lot of work to make proper use of dev_set/get_drvdata() and container_of() to find your struct ti_reset_data pointer.
drivers/reset/ti/reset-ti.c line 41
Quoted 12 lines
drivers/reset/ti/reset-ti.c new file +12
-0+static int ti_reset_get_of_data (struct ti_reset_reg_data *reset_data, +unsigned long id) +{ +struct device_node *dev_node; +struct device_node *parent; +struct device_node *prcm_parent; +struct device_node *reset_parent; +int ret = -EINVAL ; + + dev_node =of_find_node_by_phandle ((phandle ) id); +if (!dev_node) { +pr_err ("%s: Cannot find phandle node\n" , __func__);
pass a struct device pointer as argument so you can convert these to dev_err().
Quoted 13 lines
drivers/reset/ti/reset-ti.c new file +13
-0+return ret; + } + +/* node parent */ + parent =of_get_parent (dev_node); +if (!parent) { +pr_err ("%s: Cannot find parent reset node\n" , __func__); +return ret; + } +/* prcm reset parent */ + reset_parent =of_get_next_parent (parent); +if (!reset_parent) { +pr_err ("%s: Cannot find parent reset node\n" , __func__);
drivers/reset/ti/reset-ti.c line 64
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+return ret; + } +/* PRCM Parent */ + reset_parent =of_get_parent (reset_parent); +if (!prcm_parent) { +pr_err ("%s: Cannot find parent reset node\n" , __func__); +return ret; + } + + reset_data->reg_base =of_iomap (reset_parent,0 );
please don't. of_iomap() is the stupidest helper in the kernel. Find your resource using platform_get_resource() and map it with devm_ioremap_resource() since that will properly request_mem_region() and correctly map the resource with or without caching.
Quoted 23 lines
drivers/reset/ti/reset-ti.c new file +23
-0+if (!reset_data->reg_base) { +pr_err ("%s: Cannot map reset parent.\n" , __func__); +return ret; + } + + ret =of_property_read_u32_index (parent,"reg" ,0 , + &reset_data->rstctrl_offs); +if (ret) +return ret; + + ret =of_property_read_u32_index (parent,"reg" ,1 , + &reset_data->rstst_offs); +if (ret) +return ret; + + ret =of_property_read_u32 (dev_node,"control-bit" , + &reset_data->rstctrl_bit); +if (ret <0 ) +pr_err ("%s: No entry in %s for rstst_offs\n" , __func__, + dev_node->name); + + ret =of_property_read_u32 (dev_node,"status-bit" , + &reset_data->rstst_bit);
drivers/reset/ti/reset-ti.c line 97
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+if (ret <0 ) +pr_err ("%s: No entry in %s for rstst_offs\n" , __func__, + dev_node->name); + +return 0 ; +} + +static void ti_reset_wait_on_reset (struct reset_controller_dev *rcdev, +unsigned long id) +{
right here you should have:
struct ti_reset_data *ti_data = container_of(rcdev, struct ti_reset_data, rcdev);
drivers/reset/ti/reset-ti.c line 103
Quoted 6 lines
drivers/reset/ti/reset-ti.c new file +6
-0+struct ti_reset_reg_data *temp_reg_data; +void __iomem *status_reg; +u32 bit_mask =0 ; +u32 val =0 ; + + temp_reg_data =kzalloc (sizeof (struct ti_reset_reg_data ),GFP_KERNEL );
let's think about this for a second:
user asks for a reset, then ->assert() method gets called, which will allocate memory, do some crap, free the allocated memory, then you call your ->deassert() method which will, allocate memory, do something, free memory, then this method is called which will allocate memory, do something, free memory.
Note that *all* of your allocations a) are unnecessary and b) will break resets from inside IRQ context...
drivers/reset/ti/reset-ti.c line 102
Quoted 1 line
drivers/reset/ti/reset-ti.c new file +1
-0+ti_reset_get_of_data (temp_reg_data, id);
this means that *every time* a reset is asserted/deasserted/waited on, you will parse *ONE MORE TIME* the DTB. Why ? Why don't you parse it once during probe() and cache the results ?
drivers/reset/ti/reset-ti.c line 113
Quoted 8 lines
drivers/reset/ti/reset-ti.c new file +8
-0+/* Clear the reset status bit to reflect the current status */ + status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs; + bit_mask = temp_reg_data->rstst_bit; +do { + val =readl (status_reg); +if (!(val & (1 << bit_mask))) +break ; + }while (1 );
also, none of these loops have a timeout. You are creating the possibility of hanging the platform completely if the HW misbehaves.
Quoted 17 lines
drivers/reset/ti/reset-ti.c new file +17
-0+static int ti_reset_assert (struct reset_controller_dev *rcdev, +unsigned long id) +{ +struct ti_reset_reg_data *temp_reg_data; +void __iomem *reg; +void __iomem *status_reg; +u32 status_bit =0 ; +u32 bit_mask =0 ; +u32 val =0 ; + + temp_reg_data =kzalloc (sizeof (struct ti_reset_reg_data ),GFP_KERNEL ); +ti_reset_get_of_data (temp_reg_data, id); + +/* Clear the reset status bit to reflect the current status */ + status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs; + status_bit = temp_reg_data->rstst_bit; +writel (1 << status_bit, status_reg);
drivers/reset/ti/reset-ti.c line 142
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+ + reg = temp_reg_data->reg_base + temp_reg_data->rstctrl_offs; + bit_mask = temp_reg_data->rstctrl_bit; + val =readl (reg); +if (!(val & bit_mask)) { + val |= bit_mask; +writel (val, reg); + } + +kfree (temp_reg_data);
same comments as previous callback
Quoted 19 lines
drivers/reset/ti/reset-ti.c new file +19
-0+return 0 ; +} + +static int ti_reset_deassert (struct reset_controller_dev *rcdev, +unsigned long id) +{ + +struct ti_reset_reg_data *temp_reg_data; +void __iomem *reg; +void __iomem *status_reg; +u32 status_bit =0 ; +u32 bit_mask =0 ; +u32 val =0 ; + + temp_reg_data =kzalloc (sizeof (struct ti_reset_reg_data ),GFP_KERNEL ); +ti_reset_get_of_data (temp_reg_data, id); + +/* Clear the reset status bit to reflect the current status */ + status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs;
drivers/reset/ti/reset-ti.c line 172
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+ status_bit = temp_reg_data->rstst_bit; +writel (1 << status_bit, status_reg); + + reg = temp_reg_data->reg_base + temp_reg_data->rstctrl_offs; + bit_mask = temp_reg_data->rstctrl_bit; + val =readl (reg); +if (val & bit_mask) { + val &= ~bit_mask; +writel (val, reg); + }
and here. Also, this is leaking temp_reg_data.
Quoted 31 lines
drivers/reset/ti/reset-ti.c new file +31
-0+ +return 0 ; +} + +static int ti_reset_reset (struct reset_controller_dev *rcdev, +unsigned long id) +{ +ti_reset_assert (rcdev, id); +ti_reset_deassert (rcdev, id); +ti_reset_wait_on_reset (rcdev, id); + +return 0 ; +} + +static int ti_reset_xlate (struct reset_controller_dev *rcdev, +const struct of_phandle_args *reset_spec) +{ +struct device_node *dev_node; + +if (WARN_ON (reset_spec->args_count != rcdev->of_reset_n_cells)) +return -EINVAL ; + +/* Verify that the phandle exists */ + dev_node =of_find_node_by_phandle ((phandle ) reset_spec->args[0 ]); +if (!dev_node) { +pr_err ("%s: Cannot find phandle node\n" , __func__); +return -EINVAL ; + } + +return reset_spec->args[0 ]; +}
drivers/reset/ti/reset-ti.c line 213
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+ +static struct reset_control_ops ti_reset_ops = { + .reset = ti_reset_reset, + .assert = ti_reset_assert, + .deassert = ti_reset_deassert, +}; + +static int ti_reset_probe (struct platform_device *pdev) +{ +struct device_node *resets;
here you should have something like:
struct ti_reset_data *ti_data; [...] ti_data = devm_kzalloc(sizeof(*ti_data), GFP_KERNEL); if (!ti_data) return -ENOMEM; platform_get_resource(...); ti_data->base = devm_ioremap_resource(res); if (IS_ERR(ti_data->base)) return PTR_ERR(ti_data->base); platform_set_drvdata(pdev, ti_data);
Quoted 14 lines
drivers/reset/ti/reset-ti.c new file +14
-0+ resets =of_find_node_by_name (NULL ,"resets" ); +if (!resets) { +pr_err ("%s: missing 'resets' child node.\n" , __func__); +return -EINVAL ; + } + + ti_data =kzalloc (sizeof (*ti_data),GFP_KERNEL ); +if (!ti_data) +return -ENOMEM ; + + ti_data->rcdev.owner =THIS_MODULE ; + ti_data->rcdev.of_node = resets; + ti_data->rcdev.ops = &ti_reset_ops; +
drivers/reset/ti/reset-ti.c line 238
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+ ti_data->rcdev.of_reset_n_cells =1 ; + ti_data->rcdev.of_xlate = &ti_reset_xlate; + +reset_controller_register (&ti_data->rcdev); + +return 0 ; +} + +static int ti_reset_remove (struct platform_device *pdev) +{
and here:
struct ti_reset_data *ti_data = platform_get_drvdata(pdev); reset_controller_unregister(&ti_data->rcdev);
Quoted 4 lines
drivers/reset/ti/reset-ti.c new file +4
-0+reset_controller_unregister (&ti_data->rcdev); + +return 0 ; +}
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140505/7a22ee09/attachment-0001.sig>
RFC PATCH v2 1/6 drivers: reset: TI: SoC reset controller support.
Felipe
Thanks for the comments
Quoted 17 lines · Felipe Balbi
Hi,
Quoted 14 lines · Dan Murphy
The TI SoC reset controller support utilizes the reset controller framework to give device drivers or function drivers a common set of APIs to call to reset a module.
The reset-ti is a common interface to the reset framework.
The register data is retrieved during initialization
of the reset driver through the reset-ti-data
file. The array of data is associated with the compatible from the respective DT entry.Once the data is available then this is derefenced within the common interface.
Quoted 6 lines
Quoted 5 lines
The device driver has the ability to assert, deassert or perform a complete reset.
This code was derived from previous work by Rajendra Nayak and Afzal Mohammed. The code was changed to adopt to the reset core and abstract away the SoC information.
you should break your commit log at around 72 characters at most.
Do you mean 72 characters per line?
Quoted 22 lines
Quoted 22 lines
Signed-off-by Dan Murphy <[email protected]>--- drivers/reset/Kconfig | 1 + drivers/reset/Makefile | 1 + drivers/reset/ti/Kconfig | 8 ++ drivers/reset/ti/Makefile | 1 + drivers/reset/ti/reset-ti-data.h | 56 ++++++++ drivers/reset/ti/reset-ti.c | 267 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 334 insertions(+) create mode 100644 drivers/reset/ti/Kconfig create mode 100644 drivers/reset/ti/Makefile create mode 100644 drivers/reset/ti/reset-ti-data.h create mode 100644 drivers/reset/ti/reset-ti.cdrivers/reset/Kconfig +0
-0@@ -13,3 +13,4 @@ menuconfig RESET_CONTROLLER If unsure, say no.
Quoted 3 lines
drivers/reset/Kconfig menuconfig RESET_CONTROLLER · line 16
Quoted 2 lines
drivers/reset/Kconfig +1
-0source "drivers/reset/sti/Kconfig" +source "drivers/reset/ti/Kconfig"this driver is so small that I'm not sure you need a directory for it.
Yeah. Carry over from v1 when we had the object data files which made things bigger I will put them back.
Quoted 10 lines
drivers/reset/ti/Kconfig line 2
Quoted 9 lines
drivers/reset/ti/Kconfig new file +2
-0@@ -0,0 +1,8 @@ +config RESET_TI + depends on RESET_CONTROLLERdon't you want to depend on
ARCH_OMAP||COMPILE_TEST?
Yes
Quoted 13 lines
drivers/reset/ti/reset-ti-data.h line 4
Quoted 11 lines
drivers/reset/ti/reset-ti-data.h new file +4
-0@@ -0,0 +1,56 @@ +/* +* PRCM reset driver for TI SoC's +* +* Copyright 2014 Texas Instruments Inc. this is not the TI aproved way for copyright notices. Yeah, nit-picking...
Hmm. Will need to look at other TI files to correct then.
Quoted 27 lines
Quoted 27 lines
drivers/reset/ti/reset-ti-data.h new file +26
-0+* Author: Dan Murphy <[email protected]> +* +* 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. +*/ + +#ifndef _RESET_TI_DATA_H_ +#define _RESET_TI_DATA_H_ + +#include <linux/kernel.h> +#include <linux/reset-controller.h> + +/** +* struct ti_reset_reg_data - Structure of the reset register information +* for a particular SoC. +* @rstctrl_offs: This is the reset control offset value from +* from the parent reset node. +* @rstst_offs: This is the reset status offset value from +* from the parent reset node. +* @rstctrl_bit: This is the reset control bit for the module. +* @rstst_bit: This is the reset status bit for the module. +* +* This structure describes the reset register control and status offsets. +* The bits are also defined for the same. +*/
Quoted 8 lines
drivers/reset/ti/reset-ti-data.h line 37
Quoted 6 lines
drivers/reset/ti/reset-ti-data.h new file +6
-0+struct ti_reset_reg_data { +void __iomem *reg_base; +u32 rstctrl_offs; +u32 rstst_offs; +u32 rstctrl_bit; +u32 rstst_bit;this structure can be folded into struct
ti_reset_dataand all of that can be initialized during probe.
I could do that. It will simplify the de-referencing as well.
Quoted 13 lines
Quoted 13 lines
drivers/reset/ti/reset-ti-data.h new file +13
-0+}; + +/** +* struct ti_reset_data - Structure that contains the reset register data +* as well as the total number of resets for a particular SoC. +* @reg_data: Pointer to the register data structure. +* @nr_resets: Total number of resets for the SoC in the reset array. +* +* This structure contains a pointer to the register data and the modules +* register base. The number of resets and reset controller device data is +* stored within this structure. +* +*/
Quoted 8 lines
drivers/reset/ti/reset-ti-data.h line 56
Quoted 6 lines
drivers/reset/ti/reset-ti-data.h new file +6
-0+struct ti_reset_data { +struct ti_reset_reg_data *reg_data; +struct reset_controller_dev rcdev; +}; + +#endif this entire file can be moved into the .c file. It's too small and the only user for these new types is the .c driver anyway.
This was another carry over from v1 when I had other data within. So I can collapse this now.
Quoted 32 lines
drivers/reset/ti/reset-ti.c line 6
Quoted 12 lines
drivers/reset/ti/reset-ti.c new file +6
-0@@ -0,0 +1,267 @@ +/* +* PRCM reset driver for TI SoC's +* +* Copyright 2014 Texas Instruments Inc. +* +* Author: Dan Murphy <[email protected]> fix copyright notice here too
Quoted 18 lines
drivers/reset/ti/reset-ti.c new file +18
-0+* 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. +*/ + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/slab.h> + +#include "reset-ti-data.h" +
Quoted 8 lines
drivers/reset/ti/reset-ti.c line 28
Quoted 3 lines
drivers/reset/ti/reset-ti.c new file +3
-0+#define DRIVER_NAME "prcm_reset_ti" + +static struct ti_reset_data *ti_data;NAK, you *really* don't need and don't to have this global here. It only creates problems. And avoid arguing that "there's only one reset controller anyway" because that has bitten us in the past. Also, it's not a lot of work to make proper use of dev_set/get_drvdata() and
container_of()to find your structti_reset_datapointer.
Agreed will fix
Quoted 15 lines
drivers/reset/ti/reset-ti.c line 41
Quoted 13 lines
drivers/reset/ti/reset-ti.c new file +12
-0+static int ti_reset_get_of_data (struct ti_reset_reg_data *reset_data, +unsigned long id) +{ +struct device_node *dev_node; +struct device_node *parent; +struct device_node *prcm_parent; +struct device_node *reset_parent; +int ret = -EINVAL ; + + dev_node =of_find_node_by_phandle ((phandle ) id); +if (!dev_node) { +pr_err ("%s: Cannot find phandle node\n" , __func__);pass a struct device pointer as argument so you can convert these to
dev_err().
Agreed
Quoted 20 lines
Quoted 20 lines
drivers/reset/ti/reset-ti.c new file +19
-0+return ret; + } + +/* node parent */ + parent =of_get_parent (dev_node); +if (!parent) { +pr_err ("%s: Cannot find parent reset node\n" , __func__); +return ret; + } +/* prcm reset parent */ + reset_parent =of_get_next_parent (parent); +if (!reset_parent) { +pr_err ("%s: Cannot find parent reset node\n" , __func__); +return ret; + } +/* PRCM Parent */ + reset_parent =of_get_parent (reset_parent); +if (!prcm_parent) { +pr_err ("%s: Cannot find parent reset node\n" , __func__);
Quoted 8 lines
drivers/reset/ti/reset-ti.c line 64
Quoted 4 lines
drivers/reset/ti/reset-ti.c new file +4
-0+return ret; + } + + reset_data->reg_base =of_iomap (reset_parent,0 );please don't.
of_iomap()is the stupidest helper in the kernel. Find your resource usingplatform_get_resource()and map it withdevm_ioremap_resource()since that will properlyrequest_mem_region()and correctly map the resource with or without caching.
Thanks for the information. The reason this is here so that if there are multiple PRCM modules I can just get the base address for the phandle of interest.
Quoted 36 lines
Quoted 24 lines
drivers/reset/ti/reset-ti.c new file +23
-0+if (!reset_data->reg_base) { +pr_err ("%s: Cannot map reset parent.\n" , __func__); +return ret; + } + + ret =of_property_read_u32_index (parent,"reg" ,0 , + &reset_data->rstctrl_offs); +if (ret) +return ret; + + ret =of_property_read_u32_index (parent,"reg" ,1 , + &reset_data->rstst_offs); +if (ret) +return ret; + + ret =of_property_read_u32 (dev_node,"control-bit" , + &reset_data->rstctrl_bit); +if (ret <0 ) +pr_err ("%s: No entry in %s for rstst_offs\n" , __func__, + dev_node->name); + + ret =of_property_read_u32 (dev_node,"status-bit" , + &reset_data->rstst_bit);drivers/reset/ti/reset-ti.c line 97
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+if (ret <0 ) +pr_err ("%s: No entry in %s for rstst_offs\n" , __func__, + dev_node->name); + +return 0 ; +} + +static void ti_reset_wait_on_reset (struct reset_controller_dev *rcdev, +unsigned long id) +{right here you should have:
Quoted 2 lines
struct ti_reset_data *ti_data = container_of(rcdev, struct ti_reset_data, rcdev);
I had that in v1. Should have kept that.
I will change
Quoted 9 lines
drivers/reset/ti/reset-ti.c line 103
Quoted 7 lines
drivers/reset/ti/reset-ti.c new file +6
-0+struct ti_reset_reg_data *temp_reg_data; +void __iomem *status_reg; +u32 bit_mask =0 ; +u32 val =0 ; + + temp_reg_data =kzalloc (sizeof (struct ti_reset_reg_data ),GFP_KERNEL );let's think about this for a second:
Quoted 8 lines
user asks for a reset, then ->assert() method gets called, which will allocate memory, do some crap, free the allocated memory, then you call your ->deassert() method which will, allocate memory, do something, free memory, then this method is called which will allocate memory, do something, free memory.
Note that *all* of your allocations a) are unnecessary and b) will break resets from inside IRQ context...
This made also think that this is not thread safe as this reset calls can be called from multiple callers so I think a semaphore is order for this function plus the other calls.
Quoted 5 lines
drivers/reset/ti/reset-ti.c line 102
Quoted 2 lines
drivers/reset/ti/reset-ti.c new file +1
-0+ti_reset_get_of_data (temp_reg_data, id);this means that *every time* a reset is asserted/deasserted/waited on, you will parse *ONE MORE TIME* the DTB. Why ? Why don't you parse it once during
probe()and cache the results ?
Cache it into what, a list?
I had implemented a list before and received comments do not use a list. Because you have to iterate through the list every time. And a list would need to contain some sort of indexing agent which defeats the purpose of a phandle. Then the DTB and kernel images would have to be in sync for the indexes.
If I put it into an array I run into issues with a bounded array and need to introduce the index anyway. So passing the phandle is futile which means I have to introduce the indexing from the V1 series again.
For my information why is parsing the DTB worse then iterating through a list? Is there a possibility that the DTB will be over written?
Quoted 11 lines
drivers/reset/ti/reset-ti.c line 113
Quoted 9 lines
drivers/reset/ti/reset-ti.c new file +8
-0+/* Clear the reset status bit to reflect the current status */ + status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs; + bit_mask = temp_reg_data->rstst_bit; +do { + val =readl (status_reg); +if (!(val & (1 << bit_mask))) +break ; + }while (1 );also, none of these loops have a timeout. You are creating the possibility of hanging the platform completely if the HW misbehaves.
Agreed. Philip commented on that on v1 I overlooked that comment.
Quoted 21 lines
Quoted 21 lines
drivers/reset/ti/reset-ti.c new file +20
-0+static int ti_reset_assert (struct reset_controller_dev *rcdev, +unsigned long id) +{ +struct ti_reset_reg_data *temp_reg_data; +void __iomem *reg; +void __iomem *status_reg; +u32 status_bit =0 ; +u32 bit_mask =0 ; +u32 val =0 ; + + temp_reg_data =kzalloc (sizeof (struct ti_reset_reg_data ),GFP_KERNEL ); +ti_reset_get_of_data (temp_reg_data, id); + +/* Clear the reset status bit to reflect the current status */ + status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs; + status_bit = temp_reg_data->rstst_bit; +writel (1 << status_bit, status_reg); + + reg = temp_reg_data->reg_base + temp_reg_data->rstctrl_offs; + bit_mask = temp_reg_data->rstctrl_bit;
Quoted 8 lines
drivers/reset/ti/reset-ti.c line 142
Quoted 7 lines
drivers/reset/ti/reset-ti.c new file +7
-0+ val =readl (reg); +if (!(val & bit_mask)) { + val |= bit_mask; +writel (val, reg); + } + +kfree (temp_reg_data);same comments as previous callback
Same answer here
Quoted 23 lines
Quoted 23 lines
drivers/reset/ti/reset-ti.c new file +22
-0+return 0 ; +} + +static int ti_reset_deassert (struct reset_controller_dev *rcdev, +unsigned long id) +{ + +struct ti_reset_reg_data *temp_reg_data; +void __iomem *reg; +void __iomem *status_reg; +u32 status_bit =0 ; +u32 bit_mask =0 ; +u32 val =0 ; + + temp_reg_data =kzalloc (sizeof (struct ti_reset_reg_data ),GFP_KERNEL ); +ti_reset_get_of_data (temp_reg_data, id); + +/* Clear the reset status bit to reflect the current status */ + status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs; + status_bit = temp_reg_data->rstst_bit; +writel (1 << status_bit, status_reg); +
Quoted 8 lines
drivers/reset/ti/reset-ti.c line 172
Quoted 7 lines
drivers/reset/ti/reset-ti.c new file +7
-0+ reg = temp_reg_data->reg_base + temp_reg_data->rstctrl_offs; + bit_mask = temp_reg_data->rstctrl_bit; + val =readl (reg); +if (val & bit_mask) { + val &= ~bit_mask; +writel (val, reg); + }and here. Also, this is leaking
temp_reg_data.
and here
Quoted 44 lines
Quoted 32 lines
drivers/reset/ti/reset-ti.c new file +31
-0+ +return 0 ; +} + +static int ti_reset_reset (struct reset_controller_dev *rcdev, +unsigned long id) +{ +ti_reset_assert (rcdev, id); +ti_reset_deassert (rcdev, id); +ti_reset_wait_on_reset (rcdev, id); + +return 0 ; +} + +static int ti_reset_xlate (struct reset_controller_dev *rcdev, +const struct of_phandle_args *reset_spec) +{ +struct device_node *dev_node; + +if (WARN_ON (reset_spec->args_count != rcdev->of_reset_n_cells)) +return -EINVAL ; + +/* Verify that the phandle exists */ + dev_node =of_find_node_by_phandle ((phandle ) reset_spec->args[0 ]); +if (!dev_node) { +pr_err ("%s: Cannot find phandle node\n" , __func__); +return -EINVAL ; + } + +return reset_spec->args[0 ]; +}drivers/reset/ti/reset-ti.c line 213
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+ +static struct reset_control_ops ti_reset_ops = { + .reset = ti_reset_reset, + .assert = ti_reset_assert, + .deassert = ti_reset_deassert, +}; + +static int ti_reset_probe (struct platform_device *pdev) +{ +struct device_node *resets;here you should have something like:
Quoted 15 lines
struct ti_reset_data *ti_data; [...] ti_data = devm_kzalloc(sizeof(*ti_data), GFP_KERNEL); if (!ti_data) return -ENOMEM; platform_get_resource(...); ti_data->base = devm_ioremap_resource(res); if (IS_ERR(ti_data->base)) return PTR_ERR(ti_data->base); platform_set_drvdata(pdev, ti_data);
agreed. Did not think of this when I made this a module.
Quoted 35 lines
Quoted 15 lines
drivers/reset/ti/reset-ti.c new file +14
-0+ resets =of_find_node_by_name (NULL ,"resets" ); +if (!resets) { +pr_err ("%s: missing 'resets' child node.\n" , __func__); +return -EINVAL ; + } + + ti_data =kzalloc (sizeof (*ti_data),GFP_KERNEL ); +if (!ti_data) +return -ENOMEM ; + + ti_data->rcdev.owner =THIS_MODULE ; + ti_data->rcdev.of_node = resets; + ti_data->rcdev.ops = &ti_reset_ops; +drivers/reset/ti/reset-ti.c line 238
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+ ti_data->rcdev.of_reset_n_cells =1 ; + ti_data->rcdev.of_xlate = &ti_reset_xlate; + +reset_controller_register (&ti_data->rcdev); + +return 0 ; +} + +static int ti_reset_remove (struct platform_device *pdev) +{and here:
struct ti_reset_data *ti_data = platform_get_drvdata(pdev); reset_controller_unregister(&ti_data->rcdev);Quoted 4 lines
drivers/reset/ti/reset-ti.c new file +4
-0+reset_controller_unregister (&ti_data->rcdev); + +return 0 ; +}
Signature · 2 lines
------------------ Dan Murphy
RFC PATCH v2 1/6 drivers: reset: TI: SoC reset controller support.
Hi,
[ I had to manually rewrap your email which came with long lines, please have a read on Documentation/email-clients.txt ]
Quoted 14 lines · Dan Murphy
Quoted 14 lines
Quoted 14 lines
The TI SoC reset controller support utilizes the reset controller framework to give device drivers or function drivers a common set of APIs to call to reset a module.
The reset-ti is a common interface to the reset framework.
The register data is retrieved during initialization
of the reset driver through the reset-ti-data
file. The array of data is associated with the compatible from the respective DT entry.Once the data is available then this is derefenced within the common interface.
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
The device driver has the ability to assert, deassert or perform a complete reset.
This code was derived from previous work by Rajendra Nayak and Afzal Mohammed. The code was changed to adopt to the reset core and abstract away the SoC information.
you should break your commit log at around 72 characters at most.
Do you mean 72 characters per line?
<sartalics> no, that's too much. The entire commit log </sartalics>
Yes, per-line :-)
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
drivers/reset/ti/reset-ti-data.h new file +0
-1--- /dev/nullQuoted 8 lines
drivers/reset/ti/reset-ti-data.h line 4
Quoted 6 lines
+++
b/drivers/reset/ti/reset-ti-data.hdrivers/reset/ti/reset-ti-data.h new file +4
-0@@ -0,0 +1,56 @@ +/* +* PRCM reset driver for TI SoC's +* +* Copyright 2014 Texas Instruments Inc. this is not the TI aproved way for copyright notices. Yeah, nit-picking...
Quoted 1 line
Hmm. Will need to look at other TI files to correct then.
/**
* file.c - Short Description
*
* Copyright (C) 2010-2011 Texas Instruments Incorporated - http://www.ti.com
*
* Author: Dan Murphy <[email protected]>
*
* GPL text goes here
*/
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
drivers/reset/ti/reset-ti-data.h new file +26
-0+* Author: Dan Murphy <[email protected]> +* +* 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. +*/ + +#ifndef _RESET_TI_DATA_H_ +#define _RESET_TI_DATA_H_ + +#include <linux/kernel.h> +#include <linux/reset-controller.h> + +/** +* struct ti_reset_reg_data - Structure of the reset register information +* for a particular SoC. +* @rstctrl_offs: This is the reset control offset value from +* from the parent reset node. +* @rstst_offs: This is the reset status offset value from +* from the parent reset node. +* @rstctrl_bit: This is the reset control bit for the module. +* @rstst_bit: This is the reset status bit for the module. +* +* This structure describes the reset register control and status offsets. +* The bits are also defined for the same. +*/ Quoted 8 lines
drivers/reset/ti/reset-ti-data.h line 37
Quoted 6 lines
drivers/reset/ti/reset-ti-data.h new file +6
-0+struct ti_reset_reg_data { +void __iomem *reg_base; +u32 rstctrl_offs; +u32 rstst_offs; +u32 rstctrl_bit; +u32 rstst_bit;this structure can be folded into struct
ti_reset_dataand all of that can be initialized during probe.
Quoted 1 line
I could do that. It will simplify the de-referencing as well.
yeah, and it will prevent constant alloc/free of this structure
Quoted 28 lines
Quoted 28 lines
drivers/reset/ti/reset-ti.c line 6
Quoted 12 lines
drivers/reset/ti/reset-ti.c new file +6
-0@@ -0,0 +1,267 @@ +/* +* PRCM reset driver for TI SoC's +* +* Copyright 2014 Texas Instruments Inc. +* +* Author: Dan Murphy <[email protected]> fix copyright notice here too
Quoted 14 lines
drivers/reset/ti/reset-ti.c new file +14
-0+* 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. +*/ + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/reset.h>
Quoted 8 lines
Quoted 8 lines
drivers/reset/ti/reset-ti.c line 28
Quoted 7 lines
drivers/reset/ti/reset-ti.c new file +7
-0+#include <linux/slab.h> + +#include "reset-ti-data.h" + +#define DRIVER_NAME "prcm_reset_ti" + +static struct ti_reset_data *ti_data;NAK, you *really* don't need and don't to have this global here. It only
don't _want_ to have, missed the verb
Quoted 28 lines
Quoted 19 lines
Quoted 19 lines
drivers/reset/ti/reset-ti.c new file +19
-0+return ret; + } + +/* node parent */ + parent =of_get_parent (dev_node); +if (!parent) { +pr_err ("%s: Cannot find parent reset node\n" , __func__); +return ret; + } +/* prcm reset parent */ + reset_parent =of_get_next_parent (parent); +if (!reset_parent) { +pr_err ("%s: Cannot find parent reset node\n" , __func__); +return ret; + } +/* PRCM Parent */ + reset_parent =of_get_parent (reset_parent); +if (!prcm_parent) { +pr_err ("%s: Cannot find parent reset node\n" , __func__);Quoted 8 lines
drivers/reset/ti/reset-ti.c line 64
Quoted 4 lines
drivers/reset/ti/reset-ti.c new file +4
-0+return ret; + } + + reset_data->reg_base =of_iomap (reset_parent,0 );please don't.
of_iomap()is the stupidest helper in the kernel. Find your resource usingplatform_get_resource()and map it withdevm_ioremap_resource()since that will properlyrequest_mem_region()and correctly map the resource with or without caching.
Quoted 2 lines
Thanks for the information. The reason this is here so that if there are multiple PRCM modules I can just get the base address for the phandle of interest.
yeah, you can also:
res = platform_get_resource(pdev, IORESOURCE_MEM, 0); [...] res = platform_get_resource(pdev, IORESOURCE_MEM, 1); [...]
or even:
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "my_resource"); [...] res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "my_other_resource"); [...]
Quoted 14 lines
Quoted 14 lines
drivers/reset/ti/reset-ti.c line 103
Quoted 6 lines
drivers/reset/ti/reset-ti.c new file +6
-0+struct ti_reset_reg_data *temp_reg_data; +void __iomem *status_reg; +u32 bit_mask =0 ; +u32 val =0 ; + + temp_reg_data =kzalloc (sizeof (struct ti_reset_reg_data ),GFP_KERNEL );let's think about this for a second:
user asks for a reset, then ->assert() method gets called, which will allocate memory, do some crap, free the allocated memory, then you call your ->deassert() method which will, allocate memory, do something, free memory, then this method is called which will allocate memory, do something, free memory.
Quoted 6 lines
Quoted 2 lines
Note that *all* of your allocations a) are unnecessary and b) will break resets from inside IRQ context...
This made also think that this is not thread safe as this reset calls can be called from multiple callers so I think a semaphore is order for this function plus the other calls.
right
Quoted 6 lines
Quoted 4 lines
drivers/reset/ti/reset-ti.c line 57
Quoted 1 line
drivers/reset/ti/reset-ti.c new file +1
-0+ti_reset_get_of_data (temp_reg_data, id);this means that *every time* a reset is asserted/deasserted/waited on, you will parse *ONE MORE TIME* the DTB. Why ? Why don't you parse it once during
probe()and cache the results ?Cache it into what, a list?
into your struct ti_reset_data:
of_get_property_u32(foo, "bar", &ti_data->bar);
following accesses you just need to read ti_data->bar.
Quoted 6 lines
I had implemented a list before and received comments do not use a list. Because you have to iterate through the list every time. And a list would need to contain some sort of indexing agent which defeats the purpose of a phandle. Then the DTB and kernel images would have to be in sync for the indexes.
Quoted 6 lines
If I put it into an array I run into issues with a bounded array and need to introduce the index anyway. So passing the phandle is futile which means I have to introduce the indexing from the V1 series again.
For my information why is parsing the DTB worse then iterating through a list? Is there a possibility that the DTB will be over written?
what are you talking about ?? Look at what how you're parsing your DTB:
+ ret = of_property_read_u32_index(parent, "reg", 0,
+ &reset_data->rstctrl_offs);
+ if (ret)
+ return ret;
+
+ ret = of_property_read_u32_index(parent, "reg", 1,
+ &reset_data->rstst_offs);
+ if (ret)
+ return ret;
+
+ ret = of_property_read_u32(dev_node, "control-bit",
+ &reset_data->rstctrl_bit);
+ if (ret < 0)
+ pr_err("%s: No entry in %s for rstst_offs\n", __func__,
+ dev_node->name);
+
+ ret = of_property_read_u32(dev_node, "status-bit",
+ &reset_data->rstst_bit);
+ if (ret < 0)
+ pr_err("%s: No entry in %s for rstst_offs\n", __func__,
+ dev_node->name);
why can't you do that from probe and cache the results inside struct ti_reset_data ? IOW:
probe()
{
[ ... ]
ret = of_property_read_u32_index(parent, "reg", 0,
&ti_data->rstctrl_offs);
[ ... ]
ret = of_property_read_u32_index(parent, "reg", 1,
&ti_data->rstst_offs);
[ ... ]
ret = of_property_read_u32(dev_node, "control-bit",
&ti_data->rstctrl_bit);
[ ... ]
ret = of_property_read_u32(dev_node, "status-bit",
&ti_data->rstst_bit);
[ ... ]
return 0;
}cheers
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140506/1915acc1/attachment.sig>
RFC PATCH v2 2/6 ARM: TI: Describe the ti reset DT entries
In reply to Dan MurphyDescribe the TI reset DT entries for TI SoC's.
--- .../devicetree/bindings/reset/ti,reset.txt | 103 ++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/ti,reset.txt
Documentation/devicetree/bindings/reset/ti,reset.txt new file +103 -0
@@ -0,0 +1,103 @@ +Texas Instruments Reset Controller +====================================== +Please also refer to reset.txt in this directory for common reset +controller binding usage. + +Specifying the reset entries for the IP module +============================================== +Parent module: +This is the module node that contains the reset registers and bits. + +example: + prcm_resets: resets { + compatible = "ti,dra7-resets"; + #reset-cells = <1>; + }; + +Required parent properties: +- compatible : Should be one of, + "ti,omap4-prm" for OMAP4 PRM instances + "ti,omap5-prm" for OMAP5 PRM instances + "ti,dra7-prm" for DRA7xx PRM instances + "ti,am4-prcm" for AM43xx PRCM instances + "ti,am3-prcm" for AM33xx PRCM instances + +Required child reset property: +- compatible : Should be + "resets" for All TI SoCs + +example: + prm: prm at 4ae06000 { + compatible = "ti,omap5-prm"; + reg = <0x4ae06000 0x3000>; + + prm_resets: resets { + #address-cells = <1>; + #size-cells = <1>; + #reset-cells = <1>; + }; + }; + + +Reset node declaration +============================================== +The reset node is declared in a parent child relationship. The main parent +is the PRCM module which contains the base address. The first child within +the reset parent declares the target modules reset name. This is followed by +the control and status offsets. + +Within the first reset child node is a secondary child node which declares the +reset signal of interest. Under this node the control and status bits +are declared. These bits declare the bit mask for the target reset. + + +Required properties: +reg - This is the register offset from the PRCM parent. + This must be declared as: + + reg = <control register offset>, + <status register offset>; + +control-bit - This is the bit within the register which controls the reset + of the target module. This is declared as a bit mask for the register. +status-bit - This is the bit within the register which contains the status of + the reset of the target module. + This is declared as a bit mask for the register. + +example: +&prm_resets { + dsp_rstctrl { + reg = <0x1c00>, + <0x1c04>; + + dsp_reset: dsp_reset { + control-bit = <0x01>; + status-bit = <0x01>; + }; + }; +}; + + + +Client Node Declaration +============================================== +This is the consumer of the parent node to declare what resets this +particular module is interested in. + +example: + src: src at 55082000 { + resets = <&reset_src phandle>; + reset-names = "<reset_name>"; + }; + +Required Properties: +reset_src - This is the parent DT entry for the reset controller +phandle - This is the phandle of the specific reset to be used by the clien + driver. +reset-names - This is the reset name of module that the device driver + needs to be able to reset. This value must correspond to a value within + the reset controller array. + +example: +resets = <&prm_resets &dsp_mmu_reset>; +reset-names = "dsp_mmu_reset";
Signature · 1 line
1.7.9.5
RFC PATCH v2 2/6 ARM: TI: Describe the ti reset DT entries
Quoted 14 lines · Dan Murphy
Documentation/devicetree/bindings/reset/ti,reset.txt new file +14
-0+ +Required parent properties: +- compatible : Should be one of, + "ti,omap4-prm" for OMAP4 PRM instances + "ti,omap5-prm" for OMAP5 PRM instances + "ti,dra7-prm" for DRA7xx PRM instances + "ti,am4-prcm" for AM43xx PRCM instances + "ti,am3-prcm" for AM33xx PRCM instances + +Required child reset property: +- compatible : Should be + "resets" for All TI SoCs + +example:
Documentation/devicetree/bindings/reset/ti,reset.txt line 39
Quoted 10 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +10
-0+ prm: prm at 4ae06000 { + compatible = "ti,omap5-prm"; + reg = <0x4ae06000 0x3000>; + + prm_resets: resets { + #address-cells = <1>; + #size-cells = <1>; + #reset-cells = <1>; + }; + };
The reg entries you have in the example below has different format compared to this?
Quoted 20 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +20
-0+Reset node declaration +============================================== +The reset node is declared in a parent child relationship. The main parent +is the PRCM module which contains the base address. The first child within +the reset parent declares the target modules reset name. This is followed by +the control and status offsets. + +Within the first reset child node is a secondary child node which declares the +reset signal of interest. Under this node the control and status bits +are declared. These bits declare the bit mask for the target reset. + + +Required properties: +reg - This is the register offset from the PRCM parent. + This must be declared as: + + reg = <control register offset>, + <status register offset>; + +control-bit - This is the bit within the register which controls the reset
Documentation/devicetree/bindings/reset/ti,reset.txt line 71
Quoted 10 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +10
-0+ of the target module. This is declared as a bit mask for the register. +status-bit - This is the bit within the register which contains the status of + the reset of the target module. + This is declared as a bit mask for the register. + +example: +&prm_resets { + dsp_rstctrl { + reg = <0x1c00>, + <0x1c04>;
Shouldn't this be start and size instead of start and end here?
Documentation/devicetree/bindings/reset/ti,reset.txt line 78
Quoted 6 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +6
-0+ dsp_reset: dsp_reset { + control-bit = <0x01>; + status-bit = <0x01>; + }; + }; +};
Are the control-bit and status-bit always the same? If so, you can keep that knowlede private to the the driver.
And maybe you can have the bit offset in a reg property here to avoid adding any custom properties? Something like:
dsp_reset: reset at 1 {
reg = 1;
};If reg is not suitable for that, it seems that some generic property to describe the bit offset is needed by quite a few drivers anyways, for things like clocks and regulators.
Quoted 12 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +12
-0+Client Node Declaration +============================================== +This is the consumer of the parent node to declare what resets this +particular module is interested in. + +example: + src: src at 55082000 { + resets = <&reset_src phandle>; + reset-names = "<reset_name>"; + }; + +Required Properties:
Documentation/devicetree/bindings/reset/ti,reset.txt line 103
Quoted 10 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +10
-0+reset_src - This is the parent DT entry for the reset controller +phandle - This is the phandle of the specific reset to be used by the clien + driver. +reset-names - This is the reset name of module that the device driver + needs to be able to reset. This value must correspond to a value within + the reset controller array. + +example: +resets = <&prm_resets &dsp_mmu_reset>; +reset-names = "dsp_mmu_reset";
This part looks OK to me, not sure if we need the reset-names property if we have one already why not.
Regards,
Tony
RFC PATCH v2 2/6 ARM: TI: Describe the ti reset DT entries
Tony
Thanks for the comments
Quoted 19 lines · Tony Lindgren
Quoted 18 lines · Dan Murphy
Documentation/devicetree/bindings/reset/ti,reset.txt new file +18
-0+ +Required parent properties: +- compatible : Should be one of, + "ti,omap4-prm" for OMAP4 PRM instances + "ti,omap5-prm" for OMAP5 PRM instances + "ti,dra7-prm" for DRA7xx PRM instances + "ti,am4-prcm" for AM43xx PRCM instances + "ti,am3-prcm" for AM33xx PRCM instances + +Required child reset property: +- compatible : Should be + "resets" for All TI SoCs + +example: + prm: prm at 4ae06000 { + compatible = "ti,omap5-prm"; + reg = <0x4ae06000 0x3000>; +
Quoted 8 lines
Documentation/devicetree/bindings/reset/ti,reset.txt line 39
Quoted 6 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +6
-0+ prm_resets: resets { + #address-cells = <1>; + #size-cells = <1>; + #reset-cells = <1>; + }; + };The reg entries you have in the example below has different format compared to this?
This is the parent to the resets. The reg entries below are for the child reset signals.
Quoted 24 lines
Quoted 24 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +23
-0+Reset node declaration +============================================== +The reset node is declared in a parent child relationship. The main parent +is the PRCM module which contains the base address. The first child within +the reset parent declares the target modules reset name. This is followed by +the control and status offsets. + +Within the first reset child node is a secondary child node which declares the +reset signal of interest. Under this node the control and status bits +are declared. These bits declare the bit mask for the target reset. + + +Required properties: +reg - This is the register offset from the PRCM parent. + This must be declared as: + + reg = <control register offset>, + <status register offset>; + +control-bit - This is the bit within the register which controls the reset + of the target module. This is declared as a bit mask for the register. +status-bit - This is the bit within the register which contains the status of + the reset of the target module.
Quoted 8 lines
Documentation/devicetree/bindings/reset/ti,reset.txt line 71
Quoted 7 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +7
-0+ This is declared as a bit mask for the register. + +example: +&prm_resets { + dsp_rstctrl { + reg = <0x1c00>, + <0x1c04>;Shouldn't this be start and size instead of start and end here?
I could do start and size. But the size will be the same for each register. AM33xx really hurts the consistency model here as the other patches in the series it appears that the control and status are consistent but AM33xx the registers are all over the place.
I have not looked at OMAP2->4 yet for control and status register offsets.
Quoted 8 lines
Documentation/devicetree/bindings/reset/ti,reset.txt line 78
Quoted 6 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +6
-0+ dsp_reset: dsp_reset { + control-bit = <0x01>; + status-bit = <0x01>; + }; + }; +};Are the control-bit and status-bit always the same? If so, you can keep that knowlede private to the the driver.
No there is a case in the am33xx resets file where the status and control bits are not the same.
Quoted 11 lines
And maybe you can have the bit offset in a reg property here to avoid adding any custom properties? Something like:
dsp_reset: reset at 1 { reg = 1; };If reg is not suitable for that, it seems that some generic property to describe the bit offset is needed by quite a few drivers anyways, for things like clocks and regulators.
I will have to look into this. Maybe a generic entry called bit-offset that defines the bit or a mask for the registers. It can mimic the reg entry.
Quoted 16 lines
Quoted 16 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +16
-0+Client Node Declaration +============================================== +This is the consumer of the parent node to declare what resets this +particular module is interested in. + +example: + src: src at 55082000 { + resets = <&reset_src phandle>; + reset-names = "<reset_name>"; + }; + +Required Properties: +reset_src - This is the parent DT entry for the reset controller +phandle - This is the phandle of the specific reset to be used by the clien + driver. +reset-names - This is the reset name of module that the device driver
Quoted 8 lines
Documentation/devicetree/bindings/reset/ti,reset.txt line 103
Quoted 6 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +6
-0+ needs to be able to reset. This value must correspond to a value within + the reset controller array. + +example: +resets = <&prm_resets &dsp_mmu_reset>; +reset-names = "dsp_mmu_reset";This part looks OK to me, not sure if we need the reset-names property if we have one already why not.
reset-names are part of the reset.txt file. I could probably drop the resets and reset-name information here and point to the reset.txt file for further explanation.
Quoted 4 lines
Regards,
Tony
Signature · 2 lines
------------------ Dan Murphy
RFC PATCH v2 2/6 ARM: TI: Describe the ti reset DT entries
Dan,
Quoted 32 lines · Dan
Tony
Thanks for the comments
Quoted 27 lines · Tony Lindgren
Quoted 14 lines · Dan Murphy
Documentation/devicetree/bindings/reset/ti,reset.txt new file +14
-0+ +Required parent properties: +- compatible : Should be one of, + "ti,omap4-prm" for OMAP4 PRM instances + "ti,omap5-prm" for OMAP5 PRM instances + "ti,dra7-prm" for DRA7xx PRM instances + "ti,am4-prcm" for AM43xx PRCM instances + "ti,am3-prcm" for AM33xx PRCM instances + +Required child reset property: +- compatible : Should be + "resets" for All TI SoCs + +example:Documentation/devicetree/bindings/reset/ti,reset.txt line 39
Quoted 10 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +10
-0+ prm: prm at 4ae06000 { + compatible = "ti,omap5-prm"; + reg = <0x4ae06000 0x3000>; + + prm_resets: resets { + #address-cells = <1>; + #size-cells = <1>; + #reset-cells = <1>; + }; + };The reg entries you have in the example below has different format compared to this?
Right, the #size-cells should have been 0, or each reg field updated with an additional field value of 4 as the registers are all of constant width - 4 bytes.
Quoted 36 lines
This is the parent to the resets. The reg entries below are for the child reset signals.
Quoted 32 lines
Quoted 21 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +20
-0+Reset node declaration +============================================== +The reset node is declared in a parent child relationship. The main parent +is the PRCM module which contains the base address. The first child within +the reset parent declares the target modules reset name. This is followed by +the control and status offsets. + +Within the first reset child node is a secondary child node which declares the +reset signal of interest. Under this node the control and status bits +are declared. These bits declare the bit mask for the target reset. + + +Required properties: +reg - This is the register offset from the PRCM parent. + This must be declared as: + + reg = <control register offset>, + <status register offset>; + +control-bit - This is the bit within the register which controls the resetDocumentation/devicetree/bindings/reset/ti,reset.txt line 71
Quoted 10 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +10
-0+ of the target module. This is declared as a bit mask for the register. +status-bit - This is the bit within the register which contains the status of + the reset of the target module. + This is declared as a bit mask for the register. + +example: +&prm_resets { + dsp_rstctrl { + reg = <0x1c00>, + <0x1c04>;Shouldn't this be start and size instead of start and end here?
These are two registers - one for control and one for status.
Quoted 17 lines
I could do start and size. But the size will be the same for each register. AM33xx really hurts the consistency model here as the other patches in the series it appears that the control and status are consistent but AM33xx the registers are all over the place.
I have not looked at OMAP2->4 yet for control and status register offsets.
Quoted 8 lines
Documentation/devicetree/bindings/reset/ti,reset.txt line 78
Quoted 6 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +6
-0+ dsp_reset: dsp_reset { + control-bit = <0x01>; + status-bit = <0x01>; + }; + }; +};Are the control-bit and status-bit always the same? If so, you can keep that knowlede private to the the driver.
Quoted 1 line
No there is a case in the am33xx resets file where the status and control bits are not the same.
Do we really need two separate properties to represent this, as these are essentially the relevant bits in the corresponding reg elements. I guess this is somewhat same as Tony's suggestion about using the reg field for these sub-nodes, in which case, you would have to add another #address-cells as 1 for each of the child reset nodes.
regards
Suman
Quoted 51 lines
Quoted 5 lines
And maybe you can have the bit offset in a reg property here to avoid adding any custom properties? Something like:
Quoted 7 lines
dsp_reset: reset at 1 { reg = 1; };If reg is not suitable for that, it seems that some generic property to describe the bit offset is needed by quite a few drivers anyways, for things like clocks and regulators.
I will have to look into this. Maybe a generic entry called bit-offset that defines the bit or a mask for the registers. It can mimic the reg entry.
Quoted 16 lines
Quoted 16 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +16
-0+Client Node Declaration +============================================== +This is the consumer of the parent node to declare what resets this +particular module is interested in. + +example: + src: src at 55082000 { + resets = <&reset_src phandle>; + reset-names = "<reset_name>"; + }; + +Required Properties: +reset_src - This is the parent DT entry for the reset controller +phandle - This is the phandle of the specific reset to be used by the clien + driver. +reset-names - This is the reset name of module that the device driverQuoted 8 lines
Documentation/devicetree/bindings/reset/ti,reset.txt line 103
Quoted 6 lines
Documentation/devicetree/bindings/reset/ti,reset.txt new file +6
-0+ needs to be able to reset. This value must correspond to a value within + the reset controller array. + +example: +resets = <&prm_resets &dsp_mmu_reset>; +reset-names = "dsp_mmu_reset";This part looks OK to me, not sure if we need the reset-names property if we have one already why not.
reset-names are part of the reset.txt file. I could probably drop the resets and reset-name information here and point to the reset.txt file for further explanation.
Quoted 4 lines
Regards,
Tony
RFC PATCH v2 3/6 ARM: dts: am33xx: Add prcm_resets node
In reply to Dan MurphyAdd the prcm_resets node to the prcm parent node.
Add the am33xx_resets file to define the
am33xx reset lines that are handled by this reset
framework.
--- arch/arm/boot/dts/am33xx-resets.dtsi | 42 ++++++++++++++++++++++++++++++++++ arch/arm/boot/dts/am33xx.dtsi | 7 ++++++ 2 files changed, 49 insertions(+) create mode 100644 arch/arm/boot/dts/am33xx-resets.dtsi
arch/arm/boot/dts/am33xx-resets.dtsi new file +42 -0
@@ -0,0 +1,42 @@ +/* + * Device Tree Source for AM33XX reset data + * + * Copyright (C) 2014 Texas Instruments, Inc. + * + * 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. + */ + +&prcm_resets { + gfx_rstctrl { + reg = <0x1104>, + <0x1114>; + + gfx_reset: gfx_reset { + control-bit = <0x01>; + status-bit = <0x01>; + }; + }; + + per_rstctrl { + reg = <0xD00>, + <0xD0C>; + + iva_reset: iva_reset { + control-bit = <0x04>; + status-bit = <0x10>; + }; + }; + + device_rstctrl { + reg = <0xf00>, + <0xf08>; + + device_reset: device_reset { + control-bit = <0x01>; + status-bit = <0x01>; + }; + }; + +};
arch/arm/boot/dts/am33xx.dtsi +7 -0
@@ -117,6 +117,12 @@ prcm_clockdomains: clockdomains { }; + + prcm_resets: resets { + #address-cells = <1>; + #size-cells = <1>; + #reset-cells = <1>; + }; }; scrm: scrm at 44e10000 {
@@ -833,3 +839,4 @@ }; /include/ "am33xx-clocks.dtsi" +/include/ "am33xx-resets.dtsi"
Signature · 1 line
1.7.9.5
RFC PATCH v2 4/6 ARM: dts: am4372: Add prcm_resets node
In reply to Dan MurphyAdd the prcm_resets node to the prcm parent node.
Add the am34xx_resets file to define the
am34xx reset lines that are handled by this reset
framework.
--- arch/arm/boot/dts/am4372.dtsi | 7 +++++ arch/arm/boot/dts/am43xx-resets.dtsi | 52 ++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 arch/arm/boot/dts/am43xx-resets.dtsi
arch/arm/boot/dts/am4372.dtsi +7 -0
@@ -84,6 +84,12 @@ prcm_clockdomains: clockdomains { }; + + prcm_resets: resets { + #address-cells = <1>; + #size-cells = <1>; + #reset-cells = <1>; + }; }; scrm: scrm at 44e10000 {
@@ -739,3 +745,4 @@ }; /include/ "am43xx-clocks.dtsi" +/include/ "am43xx-resets.dtsi"
arch/arm/boot/dts/am43xx-resets.dtsi new file +52 -0
52 lines not on this page · raw message
Signature · 1 line
1.7.9.5
RFC PATCH v2 5/6 ARM: dts: dra7: Add prm_resets node
In reply to Dan MurphyAdd the prcm_resets node to the prm parent node.
Add the draxx_resets file to define the
dra7xx reset lines that are handled by this reset
framework.
--- arch/arm/boot/dts/dra7.dtsi | 7 +++ arch/arm/boot/dts/dra7xx-resets.dtsi | 82 ++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 arch/arm/boot/dts/dra7xx-resets.dtsi
arch/arm/boot/dts/dra7.dtsi +7 -0
@@ -120,6 +120,12 @@ prm_clockdomains: clockdomains { }; + + prm_resets: resets { + #address-cells = <1>; + #size-cells = <1>; + #reset-cells = <1>; + }; }; cm_core_aon: cm_core_aon at 4a005000 {
@@ -793,3 +799,4 @@ }; /include/ "dra7xx-clocks.dtsi" +/include/ "dra7xx-resets.dtsi"
arch/arm/boot/dts/dra7xx-resets.dtsi new file +82 -0
82 lines not on this page · raw message
Signature · 1 line
1.7.9.5
RFC PATCH v2 6/6 ARM: dts: omap5: Add prm_resets node
In reply to Dan MurphyAdd the prm_resets node to the prm parent node.
Add the omap54xx_resets file to define the
omap5 reset lines that are handled by this reset
framework.
--- arch/arm/boot/dts/omap5.dtsi | 7 ++++ arch/arm/boot/dts/omap54xx-resets.dtsi | 66 ++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) create mode 100644 arch/arm/boot/dts/omap54xx-resets.dtsi
arch/arm/boot/dts/omap5.dtsi +7 -0
@@ -134,6 +134,12 @@ prm_clockdomains: clockdomains { }; + + prm_resets: resets { + #address-cells = <1>; + #size-cells = <1>; + #reset-cells = <1>; + }; }; cm_core_aon: cm_core_aon at 4a004000 {
@@ -873,3 +879,4 @@ }; /include/ "omap54xx-clocks.dtsi" +/include/ "omap54xx-resets.dtsi"
arch/arm/boot/dts/omap54xx-resets.dtsi new file +66 -0
66 lines not on this page · raw message
Signature · 1 line
1.7.9.5