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

[Changhuang Liang]

Hi, Hal

> > 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.

The current series is quite large. To reduce the review burden of the current series,
I plan to release a separate series later for JH7110 to reduce these similar interfaces.

Best Regards,
Changhuang