Re: [PATCH 1/2] reset: add reset-simple to unify socfpga, stm32, and sunxi

From: Andre Przywara
Date: Wed Mar 08 2017 - 05:35:42 EST

Next message: Ingo Molnar: "Re: [PATCH 1/2] x86/platform: Add a low priority low frequency NMI call chain"
Previous message: Jiri Kosina: "Re: [PATCH v5 00/15] livepatch: hybrid consistency model"
In reply to: Andre Przywara: "Re: [PATCH 2/2] reset: simple: read back to make sure changes are applied"
Next in thread: Alexandre Torgue: "Re: [PATCH 1/2] reset: add reset-simple to unify socfpga, stm32, and sunxi"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 08/03/17 09:54, Philipp Zabel wrote:
> Reset operations for simple reset controllers with reset lines that can
> be controlled by toggling bits in (mostly) contiguous register ranges
> using read-modify-write cycles under a spinlock. So far this covers the
> socfpga, stm32, and sunxi drivers.

Wow, that looks nice, thanks for that.

But can't we go one step further and unify those driver into one file then?
And either have different probe functions to cover the different DT
requirements or to just have one unified probe checking for the super
set of all properties?

Also ....

>
> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> ---
> drivers/reset/Kconfig | 6 +++
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-simple.c | 92 +++++++++++++++++++++++++++++++++++++++++
> drivers/reset/reset-simple.h | 53 ++++++++++++++++++++++++
> drivers/reset/reset-socfpga.c | 96 +++++--------------------------------------
> drivers/reset/reset-stm32.c | 73 ++++----------------------------
> drivers/reset/reset-sunxi.c | 82 +++++-------------------------------
> 7 files changed, 181 insertions(+), 222 deletions(-)
> create mode 100644 drivers/reset/reset-simple.c
> create mode 100644 drivers/reset/reset-simple.h
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index d21c07ccc94e5..d968becd0474c 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -62,21 +62,27 @@ config RESET_PISTACHIO
> help
> This enables the reset driver for ImgTec Pistachio SoCs.
>
> +config RESET_SIMPLE
> + bool
> +
> config RESET_SOCFPGA
> bool "SoCFPGA Reset Driver" if COMPILE_TEST
> default ARCH_SOCFPGA
> + select RESET_SIMPLE
> help
> This enables the reset controller driver for Altera SoCFPGAs.
>
> config RESET_STM32
> bool "STM32 Reset Driver" if COMPILE_TEST
> default ARCH_STM32
> + select RESET_SIMPLE
> help
> This enables the RCC reset controller driver for STM32 MCUs.
>
> config RESET_SUNXI
> bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
> default ARCH_SUNXI
> + select RESET_SIMPLE
> help
> This enables the reset driver for Allwinner SoCs.
>
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 02a74db943397..f5cc015f20956 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
> obj-$(CONFIG_RESET_MESON) += reset-meson.o
> obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
> obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
> +obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> obj-$(CONFIG_RESET_STM32) += reset-stm32.o
> obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> new file mode 100644
> index 0000000000000..2160e84fe216b
> --- /dev/null
> +++ b/drivers/reset/reset-simple.c
> @@ -0,0 +1,92 @@
> +/*
> + * Simple Reset Controller ops
> + *
> + * Based on Allwinner SoCs Reset Controller driver
> + *
> + * Copyright 2013 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spinlock.h>
> +
> +#include "reset-simple.h"
> +
> +static inline struct reset_simple_data *
> +to_reset_simple_data(struct reset_controller_dev *rcdev)
> +{
> + return container_of(rcdev, struct reset_simple_data, rcdev);
> +}
> +
> +static int reset_simple_clear(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct reset_simple_data *data = to_reset_simple_data(rcdev);
> + int reg_width = sizeof(u32);
> + int bank = id / (reg_width * BITS_PER_BYTE);
> + int offset = id % (reg_width * BITS_PER_BYTE);
> + unsigned long flags;
> + u32 reg;
> +
> + spin_lock_irqsave(&data->lock, flags);
> +
> + reg = readl(data->membase + (bank * reg_width));
> + writel(reg & ~BIT(offset), data->membase + (bank * reg_width));
> +
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + return 0;
> +}
> +
> +static int reset_simple_set(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct reset_simple_data *data = to_reset_simple_data(rcdev);
> + int reg_width = sizeof(u32);
> + int bank = id / (reg_width * BITS_PER_BYTE);
> + int offset = id % (reg_width * BITS_PER_BYTE);
> + unsigned long flags;
> + u32 reg;
> +
> + spin_lock_irqsave(&data->lock, flags);
> +
> + reg = readl(data->membase + (bank * reg_width));
> + writel(reg | BIT(offset), data->membase + (bank * reg_width));
> +
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + return 0;
> +}

So these two functions look strikingly similar, apart from the OR / AND
NEG part.
What about a unified bit access function then and two tiny wrappers for
set/clear?

Cheers,
Andre.

> +
> +static int reset_simple_status(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct reset_simple_data *data = to_reset_simple_data(rcdev);
> + int reg_width = sizeof(u32);
> + int bank = id / (reg_width * BITS_PER_BYTE);
> + int offset = id % (reg_width * BITS_PER_BYTE);
> + u32 reg;
> +
> + reg = readl(data->membase + (bank * reg_width));
> +
> + return !(reg & BIT(offset));
> +}
> +
> +const struct reset_control_ops reset_simple_ops = {
> + .assert = reset_simple_set,
> + .deassert = reset_simple_clear,
> + .status = reset_simple_status,
> +};
> +
> +const struct reset_control_ops reset_simple_ops_inv = {
> + .assert = reset_simple_clear,
> + .deassert = reset_simple_set,
> +};
> diff --git a/drivers/reset/reset-simple.h b/drivers/reset/reset-simple.h
> new file mode 100644
> index 0000000000000..421a0795a2372
> --- /dev/null
> +++ b/drivers/reset/reset-simple.h
> @@ -0,0 +1,53 @@
> +/*
> + * Simple Reset Controller ops
> + *
> + * Based on Allwinner SoCs Reset Controller driver
> + *
> + * Copyright 2013 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __RESET_SIMPLE_H__
> +#define __RESET_SIMPLE_H__
> +
> +#include <linux/io.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spinlock.h>
> +
> +struct reset_simple_data {
> + spinlock_t lock;
> + void __iomem *membase;
> + struct reset_controller_dev rcdev;
> +};
> +
> +extern const struct reset_control_ops reset_simple_ops;
> +extern const struct reset_control_ops reset_simple_ops_inv;
> +
> +static inline int devm_reset_controller_register_simple(struct device *dev,
> + void __iomem *membase,
> + unsigned int nr_resets,
> + bool inverted)
> +{
> + struct reset_simple_data *data;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + spin_lock_init(&data->lock);
> +
> + data->rcdev.owner = THIS_MODULE;
> + data->rcdev.nr_resets = nr_resets;
> + data->rcdev.ops = inverted ? &reset_simple_ops_inv : &reset_simple_ops;
> + data->rcdev.of_node = dev->of_node;
> +
> + return devm_reset_controller_register(dev, &data->rcdev);
> +}
> +
> +#endif /* __RESET_SIMPLE_H__ */
> diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
> index 07224c0198920..6031a3873bc8f 100644
> --- a/drivers/reset/reset-socfpga.c
> +++ b/drivers/reset/reset-socfpga.c
> @@ -25,83 +25,16 @@
> #include <linux/spinlock.h>
> #include <linux/types.h>
>
> -#define BANK_INCREMENT 4
> -#define NR_BANKS 8
> -
> -struct socfpga_reset_data {
> - spinlock_t lock;
> - void __iomem *membase;
> - struct reset_controller_dev rcdev;
> -};
> -
> -static int socfpga_reset_assert(struct reset_controller_dev *rcdev,
> - unsigned long id)
> -{
> - struct socfpga_reset_data *data = container_of(rcdev,
> - struct socfpga_reset_data,
> - rcdev);
> - int bank = id / BITS_PER_LONG;
> - int offset = id % BITS_PER_LONG;
> - unsigned long flags;
> - u32 reg;
> -
> - spin_lock_irqsave(&data->lock, flags);
> -
> - reg = readl(data->membase + (bank * BANK_INCREMENT));
> - writel(reg | BIT(offset), data->membase + (bank * BANK_INCREMENT));
> - spin_unlock_irqrestore(&data->lock, flags);
> -
> - return 0;
> -}
> -
> -static int socfpga_reset_deassert(struct reset_controller_dev *rcdev,
> - unsigned long id)
> -{
> - struct socfpga_reset_data *data = container_of(rcdev,
> - struct socfpga_reset_data,
> - rcdev);
> -
> - int bank = id / BITS_PER_LONG;
> - int offset = id % BITS_PER_LONG;
> - unsigned long flags;
> - u32 reg;
> -
> - spin_lock_irqsave(&data->lock, flags);
> -
> - reg = readl(data->membase + (bank * BANK_INCREMENT));
> - writel(reg & ~BIT(offset), data->membase + (bank * BANK_INCREMENT));
> +#include "reset-simple.h"
>
> - spin_unlock_irqrestore(&data->lock, flags);
> -
> - return 0;
> -}
> -
> -static int socfpga_reset_status(struct reset_controller_dev *rcdev,
> - unsigned long id)
> -{
> - struct socfpga_reset_data *data = container_of(rcdev,
> - struct socfpga_reset_data, rcdev);
> - int bank = id / BITS_PER_LONG;
> - int offset = id % BITS_PER_LONG;
> - u32 reg;
> -
> - reg = readl(data->membase + (bank * BANK_INCREMENT));
> -
> - return !(reg & BIT(offset));
> -}
> -
> -static const struct reset_control_ops socfpga_reset_ops = {
> - .assert = socfpga_reset_assert,
> - .deassert = socfpga_reset_deassert,
> - .status = socfpga_reset_status,
> -};
> +#define NR_BANKS 8
>
> static int socfpga_reset_probe(struct platform_device *pdev)
> {
> - struct socfpga_reset_data *data;
> struct resource *res;
> struct device *dev = &pdev->dev;
> struct device_node *np = dev->of_node;
> + void __iomem *membase;
> u32 modrst_offset;
>
> /*
> @@ -114,29 +47,20 @@ static int socfpga_reset_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> -
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - data->membase = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(data->membase))
> - return PTR_ERR(data->membase);
> + membase = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(membase))
> + return PTR_ERR(membase);
>
> if (of_property_read_u32(np, "altr,modrst-offset", &modrst_offset)) {
> dev_warn(dev, "missing altr,modrst-offset property, assuming 0x10!\n");
> modrst_offset = 0x10;
> }
> - data->membase += modrst_offset;
> -
> - spin_lock_init(&data->lock);
> -
> - data->rcdev.owner = THIS_MODULE;
> - data->rcdev.nr_resets = NR_BANKS * BITS_PER_LONG;
> - data->rcdev.ops = &socfpga_reset_ops;
> - data->rcdev.of_node = pdev->dev.of_node;
> + membase += modrst_offset;
>
> - return devm_reset_controller_register(dev, &data->rcdev);
> + return devm_reset_controller_register_simple(&pdev->dev, membase,
> + NR_BANKS * BITS_PER_LONG,
> + false);
> }
>
> static const struct of_device_id socfpga_reset_dt_ids[] = {
> diff --git a/drivers/reset/reset-stm32.c b/drivers/reset/reset-stm32.c
> index 3a7c8527e66af..15d89d66863d8 100644
> --- a/drivers/reset/reset-stm32.c
> +++ b/drivers/reset/reset-stm32.c
> @@ -16,58 +16,7 @@
> #include <linux/spinlock.h>
> #include <linux/types.h>
>
> -struct stm32_reset_data {
> - spinlock_t lock;
> - void __iomem *membase;
> - struct reset_controller_dev rcdev;
> -};
> -
> -static int stm32_reset_assert(struct reset_controller_dev *rcdev,
> - unsigned long id)
> -{
> - struct stm32_reset_data *data = container_of(rcdev,
> - struct stm32_reset_data,
> - rcdev);
> - int bank = id / BITS_PER_LONG;
> - int offset = id % BITS_PER_LONG;
> - unsigned long flags;
> - u32 reg;
> -
> - spin_lock_irqsave(&data->lock, flags);
> -
> - reg = readl(data->membase + (bank * 4));
> - writel(reg | BIT(offset), data->membase + (bank * 4));
> -
> - spin_unlock_irqrestore(&data->lock, flags);
> -
> - return 0;
> -}
> -
> -static int stm32_reset_deassert(struct reset_controller_dev *rcdev,
> - unsigned long id)
> -{
> - struct stm32_reset_data *data = container_of(rcdev,
> - struct stm32_reset_data,
> - rcdev);
> - int bank = id / BITS_PER_LONG;
> - int offset = id % BITS_PER_LONG;
> - unsigned long flags;
> - u32 reg;
> -
> - spin_lock_irqsave(&data->lock, flags);
> -
> - reg = readl(data->membase + (bank * 4));
> - writel(reg & ~BIT(offset), data->membase + (bank * 4));
> -
> - spin_unlock_irqrestore(&data->lock, flags);
> -
> - return 0;
> -}
> -
> -static const struct reset_control_ops stm32_reset_ops = {
> - .assert = stm32_reset_assert,
> - .deassert = stm32_reset_deassert,
> -};
> +#include "reset-simple.h"
>
> static const struct of_device_id stm32_reset_dt_ids[] = {
> { .compatible = "st,stm32-rcc", },
> @@ -76,26 +25,22 @@ static const struct of_device_id stm32_reset_dt_ids[] = {
>
> static int stm32_reset_probe(struct platform_device *pdev)
> {
> - struct stm32_reset_data *data;
> + struct reset_simple_data *data;
> struct resource *res;
> + void __iomem *membase;
>
> data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> return -ENOMEM;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - data->membase = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(data->membase))
> - return PTR_ERR(data->membase);
> -
> - spin_lock_init(&data->lock);
> -
> - data->rcdev.owner = THIS_MODULE;
> - data->rcdev.nr_resets = resource_size(res) * 8;
> - data->rcdev.ops = &stm32_reset_ops;
> - data->rcdev.of_node = pdev->dev.of_node;
> + membase = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(membase))
> + return PTR_ERR(membase);
>
> - return devm_reset_controller_register(&pdev->dev, &data->rcdev);
> + return devm_reset_controller_register_simple(&pdev->dev, membase,
> + resource_size(res) * 8,
> + false);
> }
>
> static struct platform_driver stm32_reset_driver = {
> diff --git a/drivers/reset/reset-sunxi.c b/drivers/reset/reset-sunxi.c
> index cd585cd2f04dd..60745dd6f70a6 100644
> --- a/drivers/reset/reset-sunxi.c
> +++ b/drivers/reset/reset-sunxi.c
> @@ -22,64 +22,11 @@
> #include <linux/spinlock.h>
> #include <linux/types.h>
>
> -struct sunxi_reset_data {
> - spinlock_t lock;
> - void __iomem *membase;
> - struct reset_controller_dev rcdev;
> -};
> -
> -static int sunxi_reset_assert(struct reset_controller_dev *rcdev,
> - unsigned long id)
> -{
> - struct sunxi_reset_data *data = container_of(rcdev,
> - struct sunxi_reset_data,
> - rcdev);
> - int reg_width = sizeof(u32);
> - int bank = id / (reg_width * BITS_PER_BYTE);
> - int offset = id % (reg_width * BITS_PER_BYTE);
> - unsigned long flags;
> - u32 reg;
> -
> - spin_lock_irqsave(&data->lock, flags);
> -
> - reg = readl(data->membase + (bank * reg_width));
> - writel(reg & ~BIT(offset), data->membase + (bank * reg_width));
> -
> - spin_unlock_irqrestore(&data->lock, flags);
> -
> - return 0;
> -}
> -
> -static int sunxi_reset_deassert(struct reset_controller_dev *rcdev,
> - unsigned long id)
> -{
> - struct sunxi_reset_data *data = container_of(rcdev,
> - struct sunxi_reset_data,
> - rcdev);
> - int reg_width = sizeof(u32);
> - int bank = id / (reg_width * BITS_PER_BYTE);
> - int offset = id % (reg_width * BITS_PER_BYTE);
> - unsigned long flags;
> - u32 reg;
> -
> - spin_lock_irqsave(&data->lock, flags);
> -
> - reg = readl(data->membase + (bank * reg_width));
> - writel(reg | BIT(offset), data->membase + (bank * reg_width));
> -
> - spin_unlock_irqrestore(&data->lock, flags);
> -
> - return 0;
> -}
> -
> -static const struct reset_control_ops sunxi_reset_ops = {
> - .assert = sunxi_reset_assert,
> - .deassert = sunxi_reset_deassert,
> -};
> +#include "reset-simple.h"
>
> static int sunxi_reset_init(struct device_node *np)
> {
> - struct sunxi_reset_data *data;
> + struct reset_simple_data *data;
> struct resource res;
> resource_size_t size;
> int ret;
> @@ -108,7 +55,7 @@ static int sunxi_reset_init(struct device_node *np)
>
> data->rcdev.owner = THIS_MODULE;
> data->rcdev.nr_resets = size * 32;
> - data->rcdev.ops = &sunxi_reset_ops;
> + data->rcdev.ops = &reset_simple_ops_inv;
> data->rcdev.of_node = np;
>
> return reset_controller_register(&data->rcdev);
> @@ -147,26 +94,17 @@ static const struct of_device_id sunxi_reset_dt_ids[] = {
>
> static int sunxi_reset_probe(struct platform_device *pdev)
> {
> - struct sunxi_reset_data *data;
> struct resource *res;
> -
> - data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + void __iomem *membase;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - data->membase = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(data->membase))
> - return PTR_ERR(data->membase);
> -
> - spin_lock_init(&data->lock);
> -
> - data->rcdev.owner = THIS_MODULE;
> - data->rcdev.nr_resets = resource_size(res) * 32;
> - data->rcdev.ops = &sunxi_reset_ops;
> - data->rcdev.of_node = pdev->dev.of_node;
> + membase = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(membase))
> + return PTR_ERR(membase);
>
> - return devm_reset_controller_register(&pdev->dev, &data->rcdev);
> + return devm_reset_controller_register_simple(&pdev->dev, membase,
> + resource_size(res) * 32,
> + true);
> }
>
> static struct platform_driver sunxi_reset_driver = {
>

Next message: Ingo Molnar: "Re: [PATCH 1/2] x86/platform: Add a low priority low frequency NMI call chain"
Previous message: Jiri Kosina: "Re: [PATCH v5 00/15] livepatch: hybrid consistency model"
In reply to: Andre Przywara: "Re: [PATCH 2/2] reset: simple: read back to make sure changes are applied"
Next in thread: Alexandre Torgue: "Re: [PATCH 1/2] reset: add reset-simple to unify socfpga, stm32, and sunxi"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]