TI Reset Driver adapted to the reset core
Subsystems (6)
This is a RFC on the TI reset adoption to the Reset framework.
The patchset was derived from work from Rajendra Nayak and Afzal Mohammed who have had similar code offerings. One of the major differences here is the SoC data has been broken out so that the data and code are independent.
There is currently no OMAP4 or prior support added to this patch series. Once the RFC is complete and the code infrastructure looks to be acceptable I will add these data sets into the patchset.
Dan
RFC 1/11 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 | 58 ++++++++++++ drivers/reset/ti/reset-ti.c | 195 ++++++++++++++++++++++++++++++++++++++ include/linux/reset_ti.h | 25 +++++ 7 files changed, 289 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 create mode 100644 include/linux/reset_ti.h
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 +58 -0
58 lines not on this page · raw message
drivers/reset/ti/reset-ti.c new file +195 -0
195 lines not on this page · raw message
include/linux/reset_ti.h new file +25 -0
@@ -0,0 +1,25 @@ +/* +* TI reset driver support +* +* 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. +* +* This program is distributed "as is" WITHOUT ANY WARRANTY of any +* kind, whether express or implied; without even the implied warranty +* of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef _RESET_TI_H_ +#define _RESET_TI_H_ + +#ifdef CONFIG_RESET_TI +void ti_dt_reset_init (void ); +#else +static inline void ti_dt_reset_init (void ){return ; }; +#endif + +#endif
Signature · 1 line
1.7.9.5
RFC 1/11 drivers: reset: TI: SoC reset controller support.
include/linux/reset_ti.h line 23
Quoted 8 lines · On Tuesday 29 April 2014 15:19:40 Dan Murphy wrote:
include/linux/reset_ti.h new file +8
-0+#ifndef _RESET_TI_H_ +#define _RESET_TI_H_ + +#ifdef CONFIG_RESET_TI +void ti_dt_reset_init (void ); +#else +static inline void ti_dt_reset_init (void ){return ; }; +#endif
Why can't this be a regular platform device driver that gets initialized through an initcall rather than get called from platform code?
Arnd
Philipp Zabel
maintainerRFC 1/11 drivers: reset: TI: SoC reset controller support.
In reply to Dan MurphyHi Dan,
Am Dienstag, den 29.04.2014, 15:19 -0500 schrieb Dan Murphy:
Quoted 108 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.
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.
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 | 58 ++++++++++++ drivers/reset/ti/reset-ti.c | 195 ++++++++++++++++++++++++++++++++++++++ include/linux/reset_ti.h | 25 +++++ 7 files changed, 289 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 create mode 100644 include/linux/reset_ti.hdrivers/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 DRA7drivers/reset/ti/Makefile new file +1
-0@@ -0,0 +1 @@ +obj-$(CONFIG_RESET_TI) += reset-ti.odrivers/reset/ti/reset-ti-data.h new file +27
-0@@ -0,0 +1,58 @@ +/* +* PRCM reset driver for TI SoC's +* +* Copyright 2014 Texas Instruments Inc. +* +* 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/of_device.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.
drivers/reset/ti/reset-ti-data.h line 37
Quoted 10 lines
drivers/reset/ti/reset-ti-data.h new file +10
-0+* @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. +*/ +struct ti_reset_reg_data { +u32 rstctrl_offs; +u32 rstst_offs; +u8 rstctrl_bit; +u8 rstst_bit;
You are only ever using these as (1 << rstctrl_bit) and as (1 << rstst_bit). You could store the mask here directly, like the regulator framework does.
drivers/reset/ti/reset-ti-data.h line 49
Quoted 12 lines
drivers/reset/ti/reset-ti-data.h new file +12
-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. +*
trailing whitespace
Quoted 36 lines
drivers/reset/ti/reset-ti-data.h new file +9
-0+*/ +struct ti_reset_data { +struct ti_reset_reg_data *reg_data; +struct reset_controller_dev rcdev; +void __iomem *reg_base; +u8 nr_resets; +}; + +#endif drivers/reset/ti/reset-ti.c new file +21
-0@@ -0,0 +1,195 @@ +/* +* PRCM reset driver for TI SoC's +* +* Copyright 2014 Texas Instruments Inc. +* +* 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. +*/ + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/slab.h> + +#include <linux/reset_ti.h>
drivers/reset/ti/reset-ti.c line 31
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+#include <linux/reset.h> + +#include "reset-ti-data.h" + +static void ti_reset_wait_on_reset (struct reset_controller_dev *rcdev, +unsigned long id) +{ +struct ti_reset_data *reset_data =container_of (rcdev, + struct ti_reset_data, + rcdev);
Please consider taking a few steps to the left.
Quoted 7 lines
drivers/reset/ti/reset-ti.c new file +7
-0+void __iomem *status_reg; +u32 val =0 ; +u8 status_bit =0 ; + +if (id <0 ) { +pr_err ("%s: ID passed is invalid\n" , __func__); +return ;
drivers/reset/ti/reset-ti.c line 48
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+ } + +/* Clear the reset status bit to reflect the current status */ + status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs; + status_bit = reset_data->reg_data[id].rstst_bit; +do { + val =readl (status_reg); +if (!(val & (1 << status_bit))) +break ; + }while (1 );
Is the status bit guaranteed to clear after a few cycles?
drivers/reset/ti/reset-ti.c line 56
Quoted 8 lines
drivers/reset/ti/reset-ti.c new file +8
-0+} + +static int ti_reset_assert (struct reset_controller_dev *rcdev, +unsigned long id) +{ +struct ti_reset_data *reset_data =container_of (rcdev, + struct ti_reset_data, + rcdev);
Very long lines again.
drivers/reset/ti/reset-ti.c line 63
Quoted 7 lines
drivers/reset/ti/reset-ti.c new file +7
-0+void __iomem *reg; +void __iomem *status_reg; +u32 val =0 ; +u8 bit =0 ; +u8 status_bit =0 ; + +if (id <0 ) {
The id parameter is _unsigned_ long.
drivers/reset/ti/reset-ti.c line 71
Quoted 8 lines
drivers/reset/ti/reset-ti.c new file +8
-0+pr_err ("%s: ID passed is invalid\n" , __func__); +return -ENODEV ; + } + +/* Clear the reset status bit to reflect the current status */ + status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs; + status_bit = reset_data->reg_data[id].rstst_bit; +writel (1 << status_bit, status_reg);
See the first comment, all the left shifts could be done by the compiler if you store the bit mask instead of the bit offset.
Quoted 13 lines
drivers/reset/ti/reset-ti.c new file +13
-0+ reg = reset_data->reg_base + reset_data->reg_data[id].rstctrl_offs; + bit = reset_data->reg_data[id].rstctrl_bit; + val =readl (reg); +if (!(val & (1 << bit))) { + val |= (1 << bit); +writel (val, reg); + } + +return 0 ; +} + +static int ti_reset_deassert (struct reset_controller_dev *rcdev, +unsigned long id)
drivers/reset/ti/reset-ti.c line 95
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+{ + +struct ti_reset_data *reset_data; +void __iomem *reg; +void __iomem *status_reg; +u32 val =0 ; +u8 bit =0 ; +u8 status_bit =0 ; + +if (id <0 ) {
Again, unsigned.
Quoted 47 lines
drivers/reset/ti/reset-ti.c new file +47
-0+pr_err ("%s: reset ID passed is invalid\n" , __func__); +return -ENODEV ; + } + + reset_data = container_of(rcdev, struct ti_reset_data, rcdev); + +/* Clear the reset status bit to reflect the current status */ + status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs; + status_bit = reset_data->reg_data[id].rstst_bit; +writel (1 << status_bit, status_reg); + + reg = reset_data->reg_base + reset_data->reg_data[id].rstctrl_offs; + bit = reset_data->reg_data[id].rstctrl_bit; + val =readl (reg); +if (val & (1 << bit)) { + val &= ~(1 << bit); +writel (val, reg); + } + +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 struct reset_control_ops ti_reset_ops = { + .reset = ti_reset_reset, + .assert = ti_reset_assert, + .deassert = ti_reset_deassert, +}; + +static const struct of_device_id ti_reset_of_match[] = { + {}, +}; + +const struct of_device_id *ti_reset_get_data (struct device_node *parent) +{ +const struct of_device_id *dev_node; + + dev_node =of_match_node (ti_reset_of_match, parent);
drivers/reset/ti/reset-ti.c line 152
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+if (!dev_node) { +pr_err ("%s: No compatible for resets for %s\n" , + __func__, parent->name); +return NULL ; + } + +return dev_node; +} + +void __init ti_dt_reset_init (void )
Is there a reason not to just register this as a platform device?
Quoted 64 lines
drivers/reset/ti/reset-ti.c new file +43
-0+{ +struct ti_reset_data *ti_data; +struct device_node *parent; +struct device_node *resets; +const struct of_device_id *dev_node; + + resets =of_find_node_by_name (NULL ,"resets" ); +if (!resets) { +pr_err ("%s: missing 'resets' child node.\n" , __func__); +return ; + } + + parent =of_get_parent (resets); +if (!parent) { +pr_err ("%s: Cannot find parent reset node\n" , __func__); +return ; + } + + ti_data =kzalloc (sizeof (*ti_data),GFP_KERNEL ); +if (!ti_data) +return ; + + dev_node =ti_reset_get_data (resets); +if (!dev_node) { +pr_err ("%s: Cannot find data for node\n" , __func__); +return ; + } + + ti_data = (struct ti_reset_data *) dev_node->data; + + ti_data->reg_base =of_iomap (parent,0 ); +if (!ti_data->reg_base) { +pr_err ("%s: Cannot map reset parent.\n" , __func__); +return ; + } + + ti_data->rcdev.owner =THIS_MODULE ; + ti_data->rcdev.nr_resets = ti_data->nr_resets; + ti_data->rcdev.of_node = resets; + ti_data->rcdev.ops = &ti_reset_ops; + +reset_controller_register (&ti_data->rcdev); +}include/linux/reset_ti.h new file +15
-0@@ -0,0 +1,25 @@ +/* +* TI reset driver support +* +* 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. +* +* This program is distributed "as is" WITHOUT ANY WARRANTY of any +* kind, whether express or implied; without even the implied warranty +* of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ +
include/linux/reset_ti.h line 25
Quoted 10 lines
include/linux/reset_ti.h new file +10
-0+#ifndef _RESET_TI_H_ +#define _RESET_TI_H_ + +#ifdef CONFIG_RESET_TI +void ti_dt_reset_init (void ); +#else +static inline void ti_dt_reset_init (void ){return ; }; +#endif + +#endif
regards
Philipp
RFC 1/11 drivers: reset: TI: SoC reset controller support.
Philipp and Arnd
Thank you for the comments
Quoted 116 lines · Philipp Zabel
Hi Dan,
Am Dienstag, den 29.04.2014, 15:19 -0500 schrieb Dan Murphy:
Quoted 113 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.
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.
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 | 58 ++++++++++++ drivers/reset/ti/reset-ti.c | 195 ++++++++++++++++++++++++++++++++++++++ include/linux/reset_ti.h | 25 +++++ 7 files changed, 289 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 create mode 100644 include/linux/reset_ti.hdrivers/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 DRA7drivers/reset/ti/Makefile new file +1
-0@@ -0,0 +1 @@ +obj-$(CONFIG_RESET_TI) += reset-ti.odrivers/reset/ti/reset-ti-data.h new file +32
-0@@ -0,0 +1,58 @@ +/* +* PRCM reset driver for TI SoC's +* +* Copyright 2014 Texas Instruments Inc. +* +* 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/of_device.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 9 lines
drivers/reset/ti/reset-ti-data.h line 37
Quoted 5 lines
drivers/reset/ti/reset-ti-data.h new file +5
-0+struct ti_reset_reg_data { +u32 rstctrl_offs; +u32 rstst_offs; +u8 rstctrl_bit; +u8 rstst_bit;You are only ever using these as (1 <<
rstctrl_bit) and as (1 <<rstst_bit). You could store the mask here directly, like the regulator framework does.
Yes we can store the mask as opposed to the bit.
I will look into it for v2.
Quoted 5 lines
Quoted 5 lines
drivers/reset/ti/reset-ti-data.h new file +5
-0+}; + +/** +* struct ti_reset_data - Structure that contains the reset register data +* as well as the total number of resets for a particular SoC.
Quoted 8 lines
drivers/reset/ti/reset-ti-data.h line 49
Quoted 7 lines
drivers/reset/ti/reset-ti-data.h new file +7
-0+* @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. +* trailing whitespace
Got it will fix v2.
Quoted 39 lines
Quoted 39 lines
drivers/reset/ti/reset-ti-data.h new file +9
-0+*/ +struct ti_reset_data { +struct ti_reset_reg_data *reg_data; +struct reset_controller_dev rcdev; +void __iomem *reg_base; +u8 nr_resets; +}; + +#endif drivers/reset/ti/reset-ti.c new file +24
-0@@ -0,0 +1,195 @@ +/* +* PRCM reset driver for TI SoC's +* +* Copyright 2014 Texas Instruments Inc. +* +* 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. +*/ + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/slab.h> + +#include <linux/reset_ti.h> +#include <linux/reset.h> + +#include "reset-ti-data.h"
Quoted 8 lines
drivers/reset/ti/reset-ti.c line 31
Quoted 7 lines
drivers/reset/ti/reset-ti.c new file +7
-0+ +static void ti_reset_wait_on_reset (struct reset_controller_dev *rcdev, +unsigned long id) +{ +struct ti_reset_data *reset_data =container_of (rcdev, + struct ti_reset_data, + rcdev);Please consider taking a few steps to the left.
Got it will fix v2.
Quoted 11 lines
Quoted 11 lines
drivers/reset/ti/reset-ti.c new file +10
-0+void __iomem *status_reg; +u32 val =0 ; +u8 status_bit =0 ; + +if (id <0 ) { +pr_err ("%s: ID passed is invalid\n" , __func__); +return ; + } + +/* Clear the reset status bit to reflect the current status */
Quoted 8 lines
drivers/reset/ti/reset-ti.c line 48
Quoted 7 lines
drivers/reset/ti/reset-ti.c new file +7
-0+ status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs; + status_bit = reset_data->reg_data[id].rstst_bit; +do { + val =readl (status_reg); +if (!(val & (1 << status_bit))) +break ; + }while (1 );Is the status bit guaranteed to clear after a few cycles?
I will look into this question
Quoted 9 lines
drivers/reset/ti/reset-ti.c line 56
Quoted 8 lines
drivers/reset/ti/reset-ti.c new file +8
-0+} + +static int ti_reset_assert (struct reset_controller_dev *rcdev, +unsigned long id) +{ +struct ti_reset_data *reset_data =container_of (rcdev, + struct ti_reset_data, + rcdev);Very long lines again.
Yep. Will fix v2 copy/paste problem
Quoted 9 lines
drivers/reset/ti/reset-ti.c line 63
Quoted 8 lines
drivers/reset/ti/reset-ti.c new file +7
-0+void __iomem *reg; +void __iomem *status_reg; +u32 val =0 ; +u8 bit =0 ; +u8 status_bit =0 ; + +if (id <0 ) {The id parameter is _unsigned_ long.
OK got it.
Quoted 11 lines
drivers/reset/ti/reset-ti.c line 71
Quoted 9 lines
drivers/reset/ti/reset-ti.c new file +8
-0+pr_err ("%s: ID passed is invalid\n" , __func__); +return -ENODEV ; + } + +/* Clear the reset status bit to reflect the current status */ + status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs; + status_bit = reset_data->reg_data[id].rstst_bit; +writel (1 << status_bit, status_reg);See the first comment, all the left shifts could be done by the compiler if you store the bit mask instead of the bit offset.
Per my reply I will look into this
Quoted 76 lines
Quoted 14 lines
drivers/reset/ti/reset-ti.c new file +13
-0+ reg = reset_data->reg_base + reset_data->reg_data[id].rstctrl_offs; + bit = reset_data->reg_data[id].rstctrl_bit; + val =readl (reg); +if (!(val & (1 << bit))) { + val |= (1 << bit); +writel (val, reg); + } + +return 0 ; +} + +static int ti_reset_deassert (struct reset_controller_dev *rcdev, +unsigned long id)drivers/reset/ti/reset-ti.c line 95
Quoted 10 lines
drivers/reset/ti/reset-ti.c new file +10
-0+{ + +struct ti_reset_data *reset_data; +void __iomem *reg; +void __iomem *status_reg; +u32 val =0 ; +u8 bit =0 ; +u8 status_bit =0 ; + +if (id <0 ) {Again, unsigned.
Quoted 50 lines
drivers/reset/ti/reset-ti.c new file +50
-0+pr_err ("%s: reset ID passed is invalid\n" , __func__); +return -ENODEV ; + } + + reset_data = container_of(rcdev, struct ti_reset_data, rcdev); + +/* Clear the reset status bit to reflect the current status */ + status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs; + status_bit = reset_data->reg_data[id].rstst_bit; +writel (1 << status_bit, status_reg); + + reg = reset_data->reg_base + reset_data->reg_data[id].rstctrl_offs; + bit = reset_data->reg_data[id].rstctrl_bit; + val =readl (reg); +if (val & (1 << bit)) { + val &= ~(1 << bit); +writel (val, reg); + } + +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 struct reset_control_ops ti_reset_ops = { + .reset = ti_reset_reset, + .assert = ti_reset_assert, + .deassert = ti_reset_deassert, +}; + +static const struct of_device_id ti_reset_of_match[] = { + {}, +}; + +const struct of_device_id *ti_reset_get_data (struct device_node *parent) +{ +const struct of_device_id *dev_node; + + dev_node =of_match_node (ti_reset_of_match, parent); +if (!dev_node) { +pr_err ("%s: No compatible for resets for %s\n" , + __func__, parent->name);
Quoted 8 lines
drivers/reset/ti/reset-ti.c line 152
Quoted 7 lines
drivers/reset/ti/reset-ti.c new file +7
-0+return NULL ; + } + +return dev_node; +} + +void __init ti_dt_reset_init (void )Is there a reason not to just register this as a platform device?
To answer this comment as well as Arnd's. To be honest this was a remnant of my development. You are both correct and this can be called during the init as a platform driver.
I will fix this in v2. Which will in turn remove one patch as well as a header file.
Dan
<snip>
Signature · 2 lines
------------------ Dan Murphy
RFC 2/11 drivers: reset: dra7: Add reset data for dra7xx
In reply to Dan MurphyAdd the reset register data for the dra7xx SoC.
Include the dt-bindings header to properly index
the right reset node.
--- drivers/reset/ti/Makefile | 1 + drivers/reset/ti/reset-ti-data.h | 1 + drivers/reset/ti/reset-ti-dra7xx.c | 61 ++++++++++++++++++++++++++++++++++++ drivers/reset/ti/reset-ti.c | 3 ++ 4 files changed, 66 insertions(+) create mode 100644 drivers/reset/ti/reset-ti-dra7xx.c
drivers/reset/ti/Makefile +1 -0
@@ -1 +1,2 @@ obj-$(CONFIG_RESET_TI) += reset-ti.o +obj-$(CONFIG_SOC_DRA7XX) += reset-ti-dra7xx.o
drivers/reset/ti/reset-ti-data.h +1 -0
@@ -55,4 +55,5 @@ struct ti_reset_data { u8 nr_resets; }; +extern struct ti_reset_data dra7_reset_data;#endif
drivers/reset/ti/reset-ti-dra7xx.c new file +61 -0
61 lines not on this page · raw message
drivers/reset/ti/reset-ti.c +3 -0
@@ -132,6 +132,9 @@ static struct reset_control_ops ti_reset_ops = { };static const struct of_device_id ti_reset_of_match[] = { +#ifdef CONFIG_SOC_DRA7XX + { .compatible ="ti,dra7-resets" , .data = &dra7_reset_data,}, +#endif {}, };
Signature · 1 line
1.7.9.5
RFC 3/11 drivers: reset: omap5: Add reset data for omap5
In reply to Dan MurphyAdd the reset register data for the omap5 SoC.
--- drivers/reset/ti/Makefile | 1 + drivers/reset/ti/reset-ti-data.h | 2 ++ drivers/reset/ti/reset-ti-omap5.c | 61 +++++++++++++++++++++++++++++++++++++ drivers/reset/ti/reset-ti.c | 3 ++ 4 files changed, 67 insertions(+) create mode 100644 drivers/reset/ti/reset-ti-omap5.c
drivers/reset/ti/Makefile +1 -0
@@ -1,2 +1,3 @@ obj-$(CONFIG_RESET_TI) += reset-ti.o obj-$(CONFIG_SOC_DRA7XX) += reset-ti-dra7xx.o +obj-$(CONFIG_SOC_OMAP5) += reset-ti-omap5.o
drivers/reset/ti/reset-ti-data.h +2 -0
@@ -56,4 +56,6 @@ struct ti_reset_data { };extern struct ti_reset_data dra7_reset_data; +extern struct ti_reset_data omap5_reset_data; +#endif
drivers/reset/ti/reset-ti-omap5.c new file +61 -0
61 lines not on this page · raw message
drivers/reset/ti/reset-ti.c +3 -0
@@ -135,6 +135,9 @@ static const struct of_device_id ti_reset_of_match[] = { #ifdef CONFIG_SOC_DRA7XX { .compatible = "ti,dra7-resets", .data = &dra7_reset_data,}, #endif +#ifdef CONFIG_SOC_OMAP5 + { .compatible = "ti,omap5-resets", .data = &omap5_reset_data,}, +#endif {}, };
Signature · 1 line
1.7.9.5
RFC 4/11 drivers: reset: am335: Add reset data for am335
In reply to Dan MurphyAdd the reset register data for the am335 SoC.
--- drivers/reset/ti/Makefile | 1 + drivers/reset/ti/reset-ti-am33xx.c | 37 ++++++++++++++++++++++++++++++++++++ drivers/reset/ti/reset-ti-data.h | 1 + drivers/reset/ti/reset-ti.c | 3 +++ 4 files changed, 42 insertions(+) create mode 100644 drivers/reset/ti/reset-ti-am33xx.c
drivers/reset/ti/Makefile +1 -0
@@ -1,3 +1,4 @@ obj-$(CONFIG_RESET_TI) += reset-ti.o obj-$(CONFIG_SOC_DRA7XX) += reset-ti-dra7xx.o obj-$(CONFIG_SOC_OMAP5) += reset-ti-omap5.o +obj-$(CONFIG_SOC_AM33XX) += reset-ti-am33xx.o
drivers/reset/ti/reset-ti-am33xx.c new file +37 -0
@@ -0,0 +1,37 @@ +/* +* am33xx reset data for PRCM Module +* +* Copyright 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. +*/ + +#include "reset-ti-data.h" + +static struct ti_reset_reg_data am335x_reset_reg_data[] = { + { + .rstctrl_offs =0xf00 , + .rstctrl_bit =0 , + .rstst_offs =0xf08 , + .rstst_bit =0 , + }, + { + .rstctrl_offs =0x1104 , + .rstctrl_bit =0 , + .rstst_offs =0x1114 , + .rstst_bit =0 , + }, + { + .rstctrl_offs =0x0D00 , + .rstctrl_bit =3 , + .rstst_offs =0x0D0C , + .rstst_bit =5 , + }, +}; + +struct ti_reset_data am335x_reset_data = { + .reg_data = am335x_reset_reg_data, + .nr_resets =ARRAY_SIZE (am335x_reset_reg_data), +};
drivers/reset/ti/reset-ti-data.h +1 -0
@@ -57,5 +57,6 @@ struct ti_reset_data {extern struct ti_reset_data dra7_reset_data;extern struct ti_reset_data omap5_reset_data; +extern struct ti_reset_data am335x_reset_data;#endif
drivers/reset/ti/reset-ti.c +3 -0
@@ -138,6 +138,9 @@ static const struct of_device_id ti_reset_of_match[] = { #ifdef CONFIG_SOC_OMAP5 { .compatible = "ti,omap5-resets", .data = &omap5_reset_data,}, #endif +#ifdef CONFIG_SOC_AM33XX + { .compatible = "ti,am335x-resets", .data = &am335x_reset_data,}, +#endif {}, };
Signature · 1 line
1.7.9.5
RFC 5/11 drivers: reset: am43xx: Add reset data for am43xx
In reply to Dan MurphyAdd the reset register data for the am43xx SoC.
--- drivers/reset/ti/Makefile | 1 + drivers/reset/ti/reset-ti-am43xx.c | 43 ++++++++++++++++++++++++++++++++++++ drivers/reset/ti/reset-ti-data.h | 1 + 3 files changed, 45 insertions(+) create mode 100644 drivers/reset/ti/reset-ti-am43xx.c
drivers/reset/ti/Makefile +1 -0
@@ -2,3 +2,4 @@ obj-$(CONFIG_RESET_TI) += reset-ti.o obj-$(CONFIG_SOC_DRA7XX) += reset-ti-dra7xx.o obj-$(CONFIG_SOC_OMAP5) += reset-ti-omap5.o obj-$(CONFIG_SOC_AM33XX) += reset-ti-am33xx.o +obj-$(CONFIG_SOC_AM43XX) += reset-ti-am43xx.o
drivers/reset/ti/reset-ti-am43xx.c new file +43 -0
43 lines not on this page · raw message
drivers/reset/ti/reset-ti-data.h +1 -0
@@ -58,5 +58,6 @@ struct ti_reset_data {extern struct ti_reset_data dra7_reset_data;extern struct ti_reset_data omap5_reset_data;extern struct ti_reset_data am335x_reset_data; +extern struct ti_reset_data am43x_reset_data;#endif
Signature · 1 line
1.7.9.5
RFC 6/11 ARM: OMAP: Add reset init to prcm file
In reply to Dan MurphyCall the reset init API to initialize the
reset framework and data for the related SoC.
--- arch/arm/mach-omap2/prm_common.c | 4 ++++ 1 file changed, 4 insertions(+)
arch/arm/mach-omap2/prm_common.c +4 -0
@@ -27,6 +27,7 @@#include <linux/of_address.h> #include <linux/clk-provider.h> #include <linux/clk/ti.h> +#include <linux/reset_ti.h> #include "soc.h" #include "prm2xxx_3xxx.h"
@@ -523,10 +524,13 @@ int __init of_prcm_init(void) mem =of_iomap (np,0 ); clk_memmaps[memmap_index] = mem;ti_dt_clk_init_provider (np, memmap_index); + memmap_index++; }ti_dt_clockdomains_setup (); +ti_dt_reset_init (); +return 0 ; }
Signature · 1 line
1.7.9.5
RFC 7/11 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 | 34 ++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/ti,reset.txt
Documentation/devicetree/bindings/reset/ti,reset.txt new file +34 -0
@@ -0,0 +1,34 @@ +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 Properties: +compatible - Should be "ti-<Soc>-resets + +DRA7xx: +Compatible - ti,dra7-resets +Reset indices - include/dt-bindings/reset/ti,dra7-resets.h + +OMAP5: +Compatible - ti,omap5-resets +Reset indices - include/dt-bindings/reset/ti,omap5-resets.h + +AM33xx: +Compatible - ti-am335x-resets +Reset indices - include/dt-bindings/reset/ti,am33xx-resets.h + +AM43xx: +Compatible - ti,am4372-resets +Reset indices - include/dt-bindings/reset/ti,am437x-resets.h
Signature · 1 line
1.7.9.5
RFC 8/11 ARM: dts: am33xx: Add prcm_resets node
In reply to Dan MurphyAdd the prcm_resets node to the prcm parent node.
Add the dt-bindings header to the DT file
--- arch/arm/boot/dts/am33xx.dtsi | 6 ++++++ include/dt-bindings/reset/ti,am33xx-resets.h | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 include/dt-bindings/reset/ti,am33xx-resets.h
arch/arm/boot/dts/am33xx.dtsi +6 -0
@@ -10,6 +10,7 @@ #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/pinctrl/am33xx.h> +#include <dt-bindings/reset/ti,am33xx-resets.h> #include "skeleton.dtsi"
@@ -117,6 +118,11 @@ prcm_clockdomains: clockdomains { }; + + prcm_resets: resets { + compatible = "ti,am335x-resets"; + #reset-cells = <1>; + }; }; scrm: scrm at 44e10000 {
include/dt-bindings/reset/ti,am33xx-resets.h new file +18 -0
@@ -0,0 +1,18 @@ +/* +* AM33xx reset index for PRCM Module +* +* Copyright 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. +*/ + +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H +#define _DT_BINDINGS_RESET_TI_AM33XX_H + +#define RESET_DEVICE_RESET 0 +#define RESET_GFX_RESET 1 +#define RESET_PER_RESET 2 + +#endif
Signature · 1 line
1.7.9.5
RFC 8/11 ARM: dts: am33xx: Add prcm_resets node
Quoted 8 lines · On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
+ * AM33xx reset index for PRCM Module
+ *
+ * Copyright 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.
+ */
include/dt-bindings/reset/ti,am33xx-resets.h line 18
Quoted 9 lines
include/dt-bindings/reset/ti,am33xx-resets.h new file +9
-0+ +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H +#define _DT_BINDINGS_RESET_TI_AM33XX_H + +#define RESET_DEVICE_RESET 0 +#define RESET_GFX_RESET 1 +#define RESET_PER_RESET 2 + +#endif
Interfaces like this should only be used if you can't use hardware numbers, in general. If these numbers are in the data sheet, just put them directly into the dts file, as we do for interrupt numbers, gpio numbers, register offsets etc.
If you have made them up to define an interface between the driver and DT because there is no usable hardare ID, I'd suggest just using a single file across all SoCs that have this driver, and have a unified name space.
Arnd
Tony Lindgren
maintainerRFC 8/11 ARM: dts: am33xx: Add prcm_resets node
Quoted 24 lines · Arnd Bergmann
Quoted 8 lines · On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
+ * AM33xx reset index for PRCM Module
+ *
+ * Copyright 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.
+ */include/dt-bindings/reset/ti,am33xx-resets.h line 18
Quoted 9 lines
include/dt-bindings/reset/ti,am33xx-resets.h new file +9
-0+ +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H +#define _DT_BINDINGS_RESET_TI_AM33XX_H + +#define RESET_DEVICE_RESET 0 +#define RESET_GFX_RESET 1 +#define RESET_PER_RESET 2 + +#endif Interfaces like this should only be used if you can't use hardware numbers, in general. If these numbers are in the data sheet, just put them directly into the dts file, as we do for interrupt numbers, gpio numbers, register offsets etc.
Quoted 4 lines
If you have made them up to define an interface between the driver and DT because there is no usable hardare ID, I'd suggest just using a single file across all SoCs that have this driver, and have a unified name space.
Also, it's a bit unclear how the reset controller phandle is used referenced and used by the consumer device.. Maybe setting that up first in a Linux generic way is a good point starting point.
Maybe something like this along the same way as clocks are set up (completely untested):
&reset1 {
iva_reset: reset1 {
reg = /bits/ 8 <0>;
};
gfx_reset: reset1 {
reg = /bits/ 8 <1>;
};
...
};&iva {
compatible = "ti,ivahd";
resets = <&reset1 1>;
...
};Regards,
Tony
RFC 8/11 ARM: dts: am33xx: Add prcm_resets node
Tony and Arnd
Thanks for the comments
Quoted 45 lines · Tony Lindgren
Quoted 23 lines · Arnd Bergmann
Quoted 8 lines · On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
+ * AM33xx reset index for PRCM Module
+ *
+ * Copyright 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.
+ */include/dt-bindings/reset/ti,am33xx-resets.h line 18
Quoted 9 lines
include/dt-bindings/reset/ti,am33xx-resets.h new file +9
-0+ +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H +#define _DT_BINDINGS_RESET_TI_AM33XX_H + +#define RESET_DEVICE_RESET 0 +#define RESET_GFX_RESET 1 +#define RESET_PER_RESET 2 + +#endif Interfaces like this should only be used if you can't use hardware numbers, in general. If these numbers are in the data sheet, just put them directly into the dts file, as we do for interrupt numbers, gpio numbers, register offsets etc.
Quoted 4 lines
If you have made them up to define an interface between the driver and DT because there is no usable hardare ID, I'd suggest just using a single file across all SoCs that have this driver, and have a unified name space.
Also, it's a bit unclear how the reset controller phandle is used referenced and used by the consumer device.. Maybe setting that up first in a Linux generic way is a good point starting point.
Maybe something like this along the same way as clocks are set up (completely untested):
&reset1 { iva_reset: reset1 { reg = /bits/ 8 <0>; }; gfx_reset: reset1 { reg = /bits/ 8 <1>; }; ... };
Quoted 5 lines
&iva {
compatible = "ti,ivahd";
resets = <&reset1 1>;
...
};I had something very similar to this when I was developing this driver but moved away from this.
Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so for each SoC.
&prcm_resets {
device_reset: device_reset {
rstctrl_offs = <0x1104>;
ctrl_bit-shift = <0>;
rstst_offs = <0x1114>;
sts_bit-shift = <0>;
};
gpu_reset: gpu_reset {
rstctrl_offs = <0x0D00>;
ctrl_bit-shift = <3>;
rstst_offs = <0x0D0C>;
sts_bit-shift = <5>;
};
};And then any client interested in a specific reset driver would include this
resets = <&prcm_resets &gpu_reset>;
reset-names = "gpu_reset";
Our reset code would then retrieve the register data through the phandle instead of an index.
Thoughts?
Dan
Quoted 3 lines
Regards,
Tony
Signature · 2 lines
------------------ Dan Murphy
Tony Lindgren
maintainerRFC 8/11 ARM: dts: am33xx: Add prcm_resets node
Quoted 77 lines · Dan Murphy
Tony and Arnd
Thanks for the comments
Quoted 45 lines · Tony Lindgren
Quoted 23 lines · Arnd Bergmann
Quoted 8 lines · On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
+ * AM33xx reset index for PRCM Module
+ *
+ * Copyright 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.
+ */include/dt-bindings/reset/ti,am33xx-resets.h line 18
Quoted 9 lines
include/dt-bindings/reset/ti,am33xx-resets.h new file +9
-0+ +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H +#define _DT_BINDINGS_RESET_TI_AM33XX_H + +#define RESET_DEVICE_RESET 0 +#define RESET_GFX_RESET 1 +#define RESET_PER_RESET 2 + +#endif Interfaces like this should only be used if you can't use hardware numbers, in general. If these numbers are in the data sheet, just put them directly into the dts file, as we do for interrupt numbers, gpio numbers, register offsets etc.
Quoted 4 lines
If you have made them up to define an interface between the driver and DT because there is no usable hardare ID, I'd suggest just using a single file across all SoCs that have this driver, and have a unified name space.
Also, it's a bit unclear how the reset controller phandle is used referenced and used by the consumer device.. Maybe setting that up first in a Linux generic way is a good point starting point.
Maybe something like this along the same way as clocks are set up (completely untested):
&reset1 { iva_reset: reset1 { reg = /bits/ 8 <0>; }; gfx_reset: reset1 { reg = /bits/ 8 <1>; }; ... };Quoted 5 lines
&iva { compatible = "ti,ivahd"; resets = <&reset1 1>; ... };I had something very similar to this when I was developing this driver but moved away from this.
Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so for each SoC.
&prcm_resets { device_reset: device_reset { rstctrl_offs = <0x1104>; ctrl_bit-shift = <0>; rstst_offs = <0x1114>; sts_bit-shift = <0>; }; gpu_reset: gpu_reset { rstctrl_offs = <0x0D00>; ctrl_bit-shift = <3>; rstst_offs = <0x0D0C>; sts_bit-shift = <5>; }; };
Quoted 8 lines
And then any client interested in a specific reset driver would include this
resets = <
&prcm_resets&gpu_reset>;
reset-names = "gpu_reset";Our reset code would then retrieve the register data through the phandle instead of an index.
Thoughts?
Using phandles makes sense here because it avoids the indexing. Indexing has a problem of needing to be in sync with the .dts files and the device driver(s). Using an index also easily causes misuse of virtual indexes being added that no longer describe the hardware at all.
Regards,
Tony
RFC 8/11 ARM: dts: am33xx: Add prcm_resets node
Tony
Quoted 82 lines · Tony Lindgren
Quoted 81 lines · Dan Murphy
Tony and Arnd
Thanks for the comments
Quoted 45 lines · Tony Lindgren
Quoted 23 lines · Arnd Bergmann
Quoted 8 lines · On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
+ * AM33xx reset index for PRCM Module
+ *
+ * Copyright 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.
+ */include/dt-bindings/reset/ti,am33xx-resets.h line 18
Quoted 9 lines
include/dt-bindings/reset/ti,am33xx-resets.h new file +9
-0+ +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H +#define _DT_BINDINGS_RESET_TI_AM33XX_H + +#define RESET_DEVICE_RESET 0 +#define RESET_GFX_RESET 1 +#define RESET_PER_RESET 2 + +#endif Interfaces like this should only be used if you can't use hardware numbers, in general. If these numbers are in the data sheet, just put them directly into the dts file, as we do for interrupt numbers, gpio numbers, register offsets etc.
Quoted 4 lines
If you have made them up to define an interface between the driver and DT because there is no usable hardare ID, I'd suggest just using a single file across all SoCs that have this driver, and have a unified name space.
Also, it's a bit unclear how the reset controller phandle is used referenced and used by the consumer device.. Maybe setting that up first in a Linux generic way is a good point starting point.
Maybe something like this along the same way as clocks are set up (completely untested):
&reset1 { iva_reset: reset1 { reg = /bits/ 8 <0>; }; gfx_reset: reset1 { reg = /bits/ 8 <1>; }; ... };Quoted 5 lines
&iva { compatible = "ti,ivahd"; resets = <&reset1 1>; ... };I had something very similar to this when I was developing this driver but moved away from this.
Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so for each SoC.
&prcm_resets { device_reset: device_reset { rstctrl_offs = <0x1104>; ctrl_bit-shift = <0>; rstst_offs = <0x1114>; sts_bit-shift = <0>; }; gpu_reset: gpu_reset { rstctrl_offs = <0x0D00>; ctrl_bit-shift = <3>; rstst_offs = <0x0D0C>; sts_bit-shift = <5>; }; };And then any client interested in a specific reset driver would include this
resets = <
&prcm_resets&gpu_reset>;
reset-names = "gpu_reset";
Quoted 7 lines
Quoted 3 lines
Our reset code would then retrieve the register data through the phandle instead of an index.
Thoughts?
Using phandles makes sense here because it avoids the indexing. Indexing has a problem of needing to be in sync with the .dts files and the device driver(s). Using an index also easily causes misuse of virtual indexes being added that no longer describe the hardware at all.
Thanks. What about placing register data in the dts files? Is there any issue with this concept?
Dan
Quoted 3 lines
Regards,
Tony
Signature · 2 lines
------------------ Dan Murphy
Tony Lindgren
maintainerRFC 8/11 ARM: dts: am33xx: Add prcm_resets node
Quoted 85 lines · Dan Murphy
Quoted 84 lines · Tony Lindgren
Quoted 83 lines · Dan Murphy
Tony and Arnd
Thanks for the comments
Quoted 45 lines · Tony Lindgren
Quoted 23 lines · Arnd Bergmann
Quoted 8 lines · On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
+ * AM33xx reset index for PRCM Module
+ *
+ * Copyright 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.
+ */include/dt-bindings/reset/ti,am33xx-resets.h line 18
Quoted 9 lines
include/dt-bindings/reset/ti,am33xx-resets.h new file +9
-0+ +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H +#define _DT_BINDINGS_RESET_TI_AM33XX_H + +#define RESET_DEVICE_RESET 0 +#define RESET_GFX_RESET 1 +#define RESET_PER_RESET 2 + +#endif Interfaces like this should only be used if you can't use hardware numbers, in general. If these numbers are in the data sheet, just put them directly into the dts file, as we do for interrupt numbers, gpio numbers, register offsets etc.
Quoted 4 lines
If you have made them up to define an interface between the driver and DT because there is no usable hardare ID, I'd suggest just using a single file across all SoCs that have this driver, and have a unified name space.
Also, it's a bit unclear how the reset controller phandle is used referenced and used by the consumer device.. Maybe setting that up first in a Linux generic way is a good point starting point.
Maybe something like this along the same way as clocks are set up (completely untested):
&reset1 { iva_reset: reset1 { reg = /bits/ 8 <0>; }; gfx_reset: reset1 { reg = /bits/ 8 <1>; }; ... };Quoted 5 lines
&iva { compatible = "ti,ivahd"; resets = <&reset1 1>; ... };I had something very similar to this when I was developing this driver but moved away from this.
Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so for each SoC.
&prcm_resets { device_reset: device_reset { rstctrl_offs = <0x1104>; ctrl_bit-shift = <0>; rstst_offs = <0x1114>; sts_bit-shift = <0>; }; gpu_reset: gpu_reset { rstctrl_offs = <0x0D00>; ctrl_bit-shift = <3>; rstst_offs = <0x0D0C>; sts_bit-shift = <5>; }; };And then any client interested in a specific reset driver would include this
resets = <
&prcm_resets&gpu_reset>;
reset-names = "gpu_reset";Our reset code would then retrieve the register data through the phandle instead of an index.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Thoughts?
Using phandles makes sense here because it avoids the indexing. Indexing has a problem of needing to be in sync with the .dts files and the device driver(s). Using an index also easily causes misuse of virtual indexes being added that no longer describe the hardware at all.
Thanks. What about placing register data in the dts files? Is there any issue with this concept?
I don't have issues with that but others may. In this case it seems like you should get away just defining few different types of reset controllers without adding any any custom properties?
In your example above, the rstctrl_offs should be just standard reg entry as an offset from the prcm_resets base address. Then you you only really need the bit shift and the type of the reset controller? And the type could be the compatible flag?
Regards,
Tony
RFC 8/11 ARM: dts: am33xx: Add prcm_resets node
Tony
Quoted 92 lines · Tony Lindgren
Quoted 83 lines · Dan Murphy
Quoted 82 lines · Tony Lindgren
Quoted 81 lines · Dan Murphy
Tony and Arnd
Thanks for the comments
Quoted 45 lines · Tony Lindgren
Quoted 23 lines · Arnd Bergmann
Quoted 8 lines · On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
+ * AM33xx reset index for PRCM Module
+ *
+ * Copyright 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.
+ */Review
Quoted 9 lines
(quoted diff) +9
-0+ +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H +#define _DT_BINDINGS_RESET_TI_AM33XX_H + +#define RESET_DEVICE_RESET 0 +#define RESET_GFX_RESET 1 +#define RESET_PER_RESET 2 + +#endifInterfaces like this should only be used if you can't use hardware numbers, in general. If these numbers are in the data sheet, just put them directly into the dts file, as we do for interrupt numbers, gpio numbers, register offsets etc.
Quoted 4 lines
If you have made them up to define an interface between the driver and DT because there is no usable hardare ID, I'd suggest just using a single file across all SoCs that have this driver, and have a unified name space.
Also, it's a bit unclear how the reset controller phandle is used referenced and used by the consumer device.. Maybe setting that up first in a Linux generic way is a good point starting point.
Maybe something like this along the same way as clocks are set up (completely untested):
&reset1 { iva_reset: reset1 { reg = /bits/ 8 <0>; }; gfx_reset: reset1 { reg = /bits/ 8 <1>; }; ... };Quoted 5 lines
&iva { compatible = "ti,ivahd"; resets = <&reset1 1>; ... };I had something very similar to this when I was developing this driver but moved away from this.
Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so for each SoC.
&prcm_resets { device_reset: device_reset { rstctrl_offs = <0x1104>; ctrl_bit-shift = <0>; rstst_offs = <0x1114>; sts_bit-shift = <0>; }; gpu_reset: gpu_reset { rstctrl_offs = <0x0D00>; ctrl_bit-shift = <3>; rstst_offs = <0x0D0C>; sts_bit-shift = <5>; }; };And then any client interested in a specific reset driver would include this
resets = <
&prcm_resets&gpu_reset>;
reset-names = "gpu_reset";Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
Our reset code would then retrieve the register data through the phandle instead of an index.
Thoughts?
Using phandles makes sense here because it avoids the indexing. Indexing has a problem of needing to be in sync with the .dts files and the device driver(s). Using an index also easily causes misuse of virtual indexes being added that no longer describe the hardware at all.
Thanks. What about placing register data in the dts files? Is there any issue with this concept?
Quoted 8 lines
I don't have issues with that but others may. In this case it seems like you should get away just defining few different types of reset controllers without adding any any custom properties?
In your example above, the
rstctrl_offsshould be just standard reg entry as an offset from theprcm_resetsbase address. Then you you only really need the bit shift and the type of the reset controller? And the type could be the compatible flag?
Well the only issue I have with declaring the reg with the standard dt entry is I have two register offsets here.
I will have to see if I can do this per the standard.
I may have to have children called control and status and define the reg and bits that way.
Dan
Quoted 4 lines
Regards,
Tony
Signature · 2 lines
------------------ Dan Murphy
RFC 9/11 ARM: dts: am4372: Add prcm_resets node
In reply to Dan MurphyAdd the prcm_resets node to the prcm parent node.
Add the dt-bindings header to the DT file
--- arch/arm/boot/dts/am4372.dtsi | 6 ++++++ include/dt-bindings/reset/ti,am437x-resets.h | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 include/dt-bindings/reset/ti,am437x-resets.h
arch/arm/boot/dts/am4372.dtsi +6 -0
@@ -10,6 +10,7 @@ #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/interrupt-controller/arm-gic.h> +#include <dt-bindings/reset/ti,am437x-resets.h> #include "skeleton.dtsi"
@@ -84,6 +85,11 @@ prcm_clockdomains: clockdomains { }; + + prcm_resets: resets { + compatible = "ti,am4372-resets"; + #reset-cells = <1>; + }; }; scrm: scrm at 44e10000 {
include/dt-bindings/reset/ti,am437x-resets.h new file +19 -0
@@ -0,0 +1,19 @@ +/* +* AM437x reset index for PRCM Module +* +* Copyright 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. +*/ + +#ifndef _DT_BINDINGS_RESET_TI_AM437X_H +#define _DT_BINDINGS_RESET_TI_AM437X_H + +#define RESET_DEVICE_RESET 0 +#define RESET_ICSS_RESET 1 +#define RESET_GFX_RESET 2 +#define RESET_PER_RESET 3 + +#endif
Signature · 1 line
1.7.9.5
RFC 10/11 ARM: dts: dra7: Add prm_resets node
In reply to Dan MurphyAdd the prcm_resets node to the prm parent node.
Add the dt-bindings header to the DT file
--- arch/arm/boot/dts/dra7.dtsi | 6 ++++++ include/dt-bindings/reset/ti,dra7-resets.h | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 include/dt-bindings/reset/ti,dra7-resets.h
arch/arm/boot/dts/dra7.dtsi +6 -0
@@ -9,6 +9,7 @@ #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/pinctrl/dra.h> +#include <dt-bindings/reset/ti,dra7-resets.h> #include "skeleton.dtsi"
@@ -120,6 +121,11 @@ prm_clockdomains: clockdomains { }; + + prm_resets: resets { + compatible = "ti,dra7-resets"; + #reset-cells = <1>; + }; }; cm_core_aon: cm_core_aon at 4a005000 {
include/dt-bindings/reset/ti,dra7-resets.h new file +22 -0
@@ -0,0 +1,22 @@ +/* +* DRA7xx reset index for PRCM Module +* +* Copyright 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. +*/ + +#ifndef _DT_BINDINGS_RESET_TI_DRA7_H +#define _DT_BINDINGS_RESET_TI_DRA7_H + +#define RESET_DEVICE_RESET 0 +#define RESET_DSP1_RESET 1 +#define RESET_IPU_CPU0_RESET 2 +#define RESET_IPU_CPU1_RESET 3 +#define RESET_IPU_MMU_CACHE_RESET 4 +#define RESET_IVA_RESET 5 +#define RESET_PCIE_RESET 6 + +#endif
Signature · 1 line
1.7.9.5
RFC 11/11 ARM: dts: omap5: Add prm_resets node
In reply to Dan MurphyAdd the prm_resets node to the prm parent node.
Add the dt-bindings header to the DT file
--- arch/arm/boot/dts/omap5.dtsi | 6 ++++++ include/dt-bindings/reset/ti,omap5-resets.h | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 include/dt-bindings/reset/ti,omap5-resets.h
arch/arm/boot/dts/omap5.dtsi +6 -0
@@ -10,6 +10,7 @@ #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/pinctrl/omap.h> +#include <dt-bindings/reset/ti,omap5-resets.h> #include "skeleton.dtsi"
@@ -134,6 +135,11 @@ prm_clockdomains: clockdomains { }; + + prm_resets: resets { + compatible = "ti,omap5-resets"; + #reset-cells = <1>; + }; }; cm_core_aon: cm_core_aon at 4a004000 {
include/dt-bindings/reset/ti,omap5-resets.h new file +22 -0
@@ -0,0 +1,22 @@ +/* +* OMAP5 reset index for PRCM Module +* +* Copyright 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. +*/ + +#ifndef _DT_BINDINGS_RESET_TI_OMAP5_H +#define _DT_BINDINGS_RESET_TI_OMAP5_H + +#define RESET_DEVICE_RESET 0 +#define RESET_DSP_RESET 1 +#define RESET_DSP_MMU_CACHE_RESET 2 +#define RESET_IPU_CPU0_RESET 3 +#define RESET_IPU_CPU1_RESET 4 +#define RESET_IPU_MMU_CACHE_RESET 5 +#define RESET_IVA_RESET 6 + +#endif
Signature · 1 line
1.7.9.5