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.
> 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.
> 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 ?
> 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...
> + * 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.
> +};
> +
> +/**
> + * 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.
> 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.
> +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().
> + 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.
> + 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);
> + 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...
> + 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 ?
> + /* 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.
> +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
> + 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.
> +
> + 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);
> + 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;
> +}
--
balbi
Attachment:
signature.asc
Description: Digital signature
