Felipe Thanks for the comments On 05/05/2014 04:33 PM, Felipe Balbi wrote: > Hi, > > On Mon, May 05, 2014 at 03:09:22PM -0500, Dan Murphy wrote: >> 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. > you should break your commit log at around 72 characters at most. Do you mean 72 characters per line? > >> Signed-off-by: Dan Murphy <dmurphy@xxxxxx> >> --- >> 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 >> >> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig >> index 0615f50..a58d789 100644 >> --- a/drivers/reset/Kconfig >> +++ b/drivers/reset/Kconfig >> @@ -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. Yeah. Carry over from v1 when we had the object data files which made things bigger I will put them back. > >> diff --git a/drivers/reset/ti/Kconfig b/drivers/reset/ti/Kconfig >> new file mode 100644 >> index 0000000..dcdce90 >> --- /dev/null >> +++ b/drivers/reset/ti/Kconfig >> @@ -0,0 +1,8 @@ >> +config RESET_TI >> + depends on RESET_CONTROLLER > don't you want to depend on ARCH_OMAP || COMPILE_TEST ? Yes > >> diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h >> new file mode 100644 >> index 0000000..4d2a6d5 >> --- /dev/null >> +++ b/drivers/reset/ti/reset-ti-data.h >> @@ -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. > >> + * Author: Dan Murphy <dmurphy@xxxxxx> >> + * >> + * 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. >> + */ >> +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. I could do that. It will simplify the de-referencing as well. >> +}; >> + >> +/** >> + * 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. >> + * >> + */ >> +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. >> diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c >> new file mode 100644 >> index 0000000..349f4fb >> --- /dev/null >> +++ b/drivers/reset/ti/reset-ti.c >> @@ -0,0 +1,267 @@ >> +/* >> + * PRCM reset driver for TI SoC's >> + * >> + * Copyright 2014 Texas Instruments Inc. >> + * >> + * Author: Dan Murphy <dmurphy@xxxxxx> > fix copyright notice here too > >> + * 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" >> + >> +#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. Agreed will fix > >> +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 > >> + 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__); >> + 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. 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. > >> + 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); >> + 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); I had that in v1. Should have kept that. I will change > >> + 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... 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. > >> + 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? > >> + /* 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. > >> +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; >> + 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 > >> + 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); >> + >> + 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 > >> + >> + 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]; >> +} >> + >> +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); agreed. Did not think of this when I made this a module. > >> + 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; >> + >> + 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); > >> + reset_controller_unregister(&ti_data->rcdev); >> + >> + return 0; >> +} -- ------------------ Dan Murphy -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html