From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753112AbdCHMvx (ORCPT ); Wed, 8 Mar 2017 07:51:53 -0500 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:44630 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752095AbdCHMve (ORCPT ); Wed, 8 Mar 2017 07:51:34 -0500 Subject: Re: [PATCH 1/2] reset: add reset-simple to unify socfpga, stm32, and sunxi To: Andre Przywara , Philipp Zabel , References: <20170308095423.3948-1-p.zabel@pengutronix.de> <1ed8aac5-5945-c265-2d66-17a260938372@arm.com> CC: Maxime Coquelin , Maxime Ripard , Chen-Yu Tsai From: Alexandre Torgue Message-ID: Date: Wed, 8 Mar 2017 12:05:24 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <1ed8aac5-5945-c265-2d66-17a260938372@arm.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.75.127.51] X-ClientProxiedBy: SFHDAG5NODE2.st.com (10.75.127.14) To SFHDAG3NODE2.st.com (10.75.127.8) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-03-08_07:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Philipp, On 03/08/2017 11:19 AM, Andre Przywara wrote: > 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? I agree with Andre. It looks nice and it should be a good thing to have a common probe inside reset-simple.c Maybe only "nresets" and "inverted" DT properties are needed. Regards Alex > > Also .... > >> >> Signed-off-by: Philipp Zabel >> --- >> 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 >> + * >> + * 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 >> +#include >> +#include >> +#include >> + >> +#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 >> + * >> + * 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 >> +#include >> +#include >> + >> +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 >> #include >> >> -#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 >> #include >> >> -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 >> #include >> >> -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 = { >>