RE: [PATCH v3 21/22] reset: starfive: Add StarFive JHB100 reset driver

[Hal Feng]

> On 26.08.07 19:29, Changhuang Liang wrote:
> Add auxiliary reset driver to support StarFive JHB100 SoC.
> The StarFive JHB100 SoC has discontiguous reset IDs. A new function
> reset_starfive_register_with_info() is introduced to support both contiguous
> and discontiguous hardware designs.
> 
> Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> ---
>  MAINTAINERS                                   |   6 +
>  drivers/reset/starfive/Kconfig                |   9 +
>  drivers/reset/starfive/Makefile               |   1 +
>  .../reset/starfive/reset-starfive-common.c    |  99 +++++-
>  .../reset/starfive/reset-starfive-common.h    |  19 ++
>  .../reset/starfive/reset-starfive-jhb100.c    | 302 ++++++++++++++++++
>  6 files changed, 425 insertions(+), 11 deletions(-)  create mode 100644
> drivers/reset/starfive/reset-starfive-jhb100.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c48b56d0ab94..709600e80951 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26088,6 +26088,12 @@ S:	Supported
>  F:
> 	Documentation/devicetree/bindings/interrupt-controller/starfive,jhb100
> -intc.yaml
>  F:	drivers/irqchip/irq-starfive-jhb100-intc.c
> 
> +STARFIVE JHB100 RESET CONTROLLER DRIVERS
> +M:	Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> +S:	Maintained

'Supported' is better.

> +F:	drivers/reset/starfive/reset-starfive-jhb1*
> +F:	include/dt-bindings/reset/starfive,jhb1*.h
> +
>  STATIC BRANCH/CALL
>  M:	Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>  M:	Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> diff --git a/drivers/reset/starfive/Kconfig b/drivers/reset/starfive/Kconfig
> index 29fbcf1a7d83..ce00495be6ad 100644
> --- a/drivers/reset/starfive/Kconfig
> +++ b/drivers/reset/starfive/Kconfig
> @@ -19,3 +19,12 @@ config RESET_STARFIVE_JH7110
>  	default ARCH_STARFIVE
>  	help
>  	  This enables the reset controller driver for the StarFive JH7110 SoC.
> +
> +config RESET_STARFIVE_JHB100
> +	bool "StarFive JHB100 Reset Driver"
> +	depends on CLK_STARFIVE_COMMON || COMPILE_TEST
> +	select AUXILIARY_BUS
> +	select RESET_STARFIVE_COMMON
> +	default ARCH_STARFIVE
> +	help
> +	  This enables the reset controller driver for the StarFive JHB100 SoC.
> diff --git a/drivers/reset/starfive/Makefile b/drivers/reset/starfive/Makefile
> index 582e4c160bd4..217002302a9f 100644
> --- a/drivers/reset/starfive/Makefile
> +++ b/drivers/reset/starfive/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_RESET_STARFIVE_COMMON)		+=
> reset-starfive-common.o
> 
>  obj-$(CONFIG_RESET_STARFIVE_JH7100)		+= reset-starfive-jh7100.o
>  obj-$(CONFIG_RESET_STARFIVE_JH7110)		+= reset-starfive-jh7110.o
> +obj-$(CONFIG_RESET_STARFIVE_JHB100)		+= reset-starfive-jhb100.o
> diff --git a/drivers/reset/starfive/reset-starfive-common.c
> b/drivers/reset/starfive/reset-starfive-common.c
> index 772bdf6763d1..86dbb33bb216 100644
> --- a/drivers/reset/starfive/reset-starfive-common.c
> +++ b/drivers/reset/starfive/reset-starfive-common.c
> @@ -14,6 +14,8 @@
> 
>  #include "reset-starfive-common.h"
> 
> +#define STARFIVE_RESET_ID_INVALID	ULONG_MAX
> +
>  struct starfive_reset {
>  	struct reset_controller_dev rcdev;
>  	/* protect registers against concurrent read-modify-write */ @@ -21,6
> +23,11 @@ struct starfive_reset {
>  	void __iomem *assert;
>  	void __iomem *status;
>  	const u32 *asserted;
> +
> +	/* Only exists in reset controllers that use the
> +	 * reset_starfive_register_with_info helper.
> +	 */
> +	const struct starfive_reset_info *info;
>  };
> 
>  static inline struct starfive_reset *
> @@ -29,19 +36,40 @@ starfive_reset_from(struct reset_controller_dev
> *rcdev)
>  	return container_of(rcdev, struct starfive_reset, rcdev);  }
> 
> +static unsigned long
> +starfive_reset_id_to_hw_id(const struct starfive_reset_map *map, unsigned
> int nr_resets,
> +			   unsigned long reset_id)
> +{
> +	for (u32 i = 0; i < nr_resets; i++) {
> +		if (map[i].reset_id == reset_id)
> +			return map[i].hw_id;
> +	}
> +
> +	return STARFIVE_RESET_ID_INVALID;
> +}
> +
>  static int starfive_reset_update(struct reset_controller_dev *rcdev,
>  				 unsigned long id, bool assert)
>  {
>  	struct starfive_reset *data = starfive_reset_from(rcdev);
> -	unsigned long offset = id / 32;
> -	u32 mask = BIT(id % 32);
> -	void __iomem *reg_assert = data->assert + offset * sizeof(u32);
> -	void __iomem *reg_status = data->status + offset * sizeof(u32);
> -	u32 done = data->asserted ? data->asserted[offset] & mask : 0;
> -	u32 value;
> -	unsigned long flags;
> +	unsigned long offset, flags;
> +	void __iomem *reg_assert;
> +	void __iomem *reg_status;
> +	u32 mask, done, value;
>  	int ret;
> 
> +	if (data->info && data->info->discontiguous) {
> +		id = starfive_reset_id_to_hw_id(data->info->map,
> data->info->nr_resets, id);
> +		if (id == STARFIVE_RESET_ID_INVALID)
> +			return -EINVAL;
> +	}
> +
> +	offset = id / 32;
> +	mask = BIT(id % 32);
> +	reg_assert = data->assert + offset * sizeof(u32);
> +	reg_status = data->status + offset * sizeof(u32);
> +	done = data->asserted ? data->asserted[offset] & mask : 0;
> +
>  	if (!assert)
>  		done ^= mask;
> 
> @@ -89,10 +117,20 @@ static int starfive_reset_status(struct
> reset_controller_dev *rcdev,
>  				 unsigned long id)
>  {
>  	struct starfive_reset *data = starfive_reset_from(rcdev);
> -	unsigned long offset = id / 32;
> -	u32 mask = BIT(id % 32);
> -	void __iomem *reg_status = data->status + offset * sizeof(u32);
> -	u32 value = readl(reg_status);
> +	void __iomem *reg_status;
> +	unsigned long offset;
> +	u32 mask, value;
> +
> +	if (data->info && data->info->discontiguous) {
> +		id = starfive_reset_id_to_hw_id(data->info->map,
> data->info->nr_resets, id);
> +		if (id == STARFIVE_RESET_ID_INVALID)
> +			return -EINVAL;
> +	}
> +
> +	offset = id / 32;
> +	mask = BIT(id % 32);
> +	reg_status = data->status + offset * sizeof(u32);
> +	value = readl(reg_status);
> 
>  	if (!data->asserted)
>  		return !(value & mask);
> @@ -132,3 +170,42 @@ int reset_starfive_register(struct device *dev, struct
> device_node *of_node,
>  	return devm_reset_controller_register(dev, &data->rcdev);  }
> EXPORT_SYMBOL_GPL(reset_starfive_register);
> +
> +int reset_starfive_register_with_info(struct device *dev, struct device_node

Also, you add reset_starfive_register_with_info() but a very similar reset_starfive_register()
still remains. I think we can merge them.

Best regards,
Hal

> *of_node,
> +				      void __iomem *assert, void __iomem *status,
> +				      const u32 *asserted,
> +				      const struct starfive_reset_info *info,
> +				      struct module *owner)
> +{
> +	struct starfive_reset *data;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->rcdev.ops = &starfive_reset_ops;
> +	data->rcdev.owner = owner;
> +	data->rcdev.nr_resets = info->nr_resets;
> +	data->rcdev.dev = dev;
> +	data->rcdev.of_node = of_node;
> +
> +	spin_lock_init(&data->lock);
> +	data->assert = assert;
> +	data->status = status;
> +	data->asserted = asserted;
> +	data->info = info;
> +
> +	if (info->discontiguous && (!info->map || !info->nr_resets))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "discontiguous controller without a valid ID
> map\n");
> +
> +	ret = devm_reset_controller_register(dev, &data->rcdev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to register reset
> +controller");
> +
> +	dev_info(dev, "Registered %u resets", data->rcdev.nr_resets);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(reset_starfive_register_with_info);
> diff --git a/drivers/reset/starfive/reset-starfive-common.h
> b/drivers/reset/starfive/reset-starfive-common.h
> index 83461b22ee55..0d4e634de58e 100644
> --- a/drivers/reset/starfive/reset-starfive-common.h
> +++ b/drivers/reset/starfive/reset-starfive-common.h
> @@ -6,9 +6,28 @@
>  #ifndef __RESET_STARFIVE_COMMON_H
>  #define __RESET_STARFIVE_COMMON_H
> 
> +struct starfive_reset_map {
> +	unsigned long reset_id;
> +	unsigned long hw_id;
> +};
> +
> +struct starfive_reset_info {
> +	unsigned int nr_resets;
> +	unsigned int assert_offset;
> +	unsigned int status_offset;
> +	bool discontiguous;
> +	const struct starfive_reset_map *map;
> +};
> +
>  int reset_starfive_register(struct device *dev, struct device_node *of_node,
>  			    void __iomem *assert, void __iomem *status,
>  			    const u32 *asserted, unsigned int nr_resets,
>  			    struct module *owner);
> 
> +int reset_starfive_register_with_info(struct device *dev, struct device_node
> *of_node,
> +				      void __iomem *assert, void __iomem *status,
> +				      const u32 *asserted,
> +				      const struct starfive_reset_info *info,
> +				      struct module *owner);
> +
>  #endif /* __RESET_STARFIVE_COMMON_H */
...