[v4,13/19] reset: starfive: Add StarFive JH7110 reset driver

  Add auxiliary driver to support StarFive JH7110 system
and always-on resets.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 drivers/reset/starfive/Kconfig                |  8 +++
 drivers/reset/starfive/Makefile               |  1 +
 .../reset/starfive/reset-starfive-jh7110.c    | 64 +++++++++++++++++++
 .../reset/starfive/reset-starfive-jh71x0.h    |  6 ++
 4 files changed, 79 insertions(+)
 create mode 100644 drivers/reset/starfive/reset-starfive-jh7110.c
  

On Tue, 21 Feb 2023 at 03:47, Hal Feng <hal.feng@starfivetech.com> wrote:
>
> Add auxiliary driver to support StarFive JH7110 system
> and always-on resets.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  drivers/reset/starfive/Kconfig                |  8 +++
>  drivers/reset/starfive/Makefile               |  1 +
>  .../reset/starfive/reset-starfive-jh7110.c    | 64 +++++++++++++++++++
>  .../reset/starfive/reset-starfive-jh71x0.h    |  6 ++
>  4 files changed, 79 insertions(+)
>  create mode 100644 drivers/reset/starfive/reset-starfive-jh7110.c
>
> diff --git a/drivers/reset/starfive/Kconfig b/drivers/reset/starfive/Kconfig
> index 9d15c4110e40..fab1a081af17 100644
> --- a/drivers/reset/starfive/Kconfig
> +++ b/drivers/reset/starfive/Kconfig
> @@ -10,3 +10,11 @@ config RESET_STARFIVE_JH7100
>         default SOC_STARFIVE
>         help
>           This enables the reset controller driver for the StarFive JH7100 SoC.
> +
> +config RESET_STARFIVE_JH7110
> +       bool "StarFive JH7110 Reset Driver"
> +       depends on AUXILIARY_BUS && CLK_STARFIVE_JH7110_SYS
> +       select RESET_STARFIVE_JH71X0
> +       default CLK_STARFIVE_JH7110_SYS
> +       help
> +         This enables the reset controller driver for the StarFive JH7110 SoC.
> diff --git a/drivers/reset/starfive/Makefile b/drivers/reset/starfive/Makefile
> index f6aa12466fad..7a44b66fb9d5 100644
> --- a/drivers/reset/starfive/Makefile
> +++ b/drivers/reset/starfive/Makefile
> @@ -2,3 +2,4 @@
>  obj-$(CONFIG_RESET_STARFIVE_JH71X0)            += reset-starfive-jh71x0.o
>
>  obj-$(CONFIG_RESET_STARFIVE_JH7100)            += reset-starfive-jh7100.o
> +obj-$(CONFIG_RESET_STARFIVE_JH7110)            += reset-starfive-jh7110.o
> diff --git a/drivers/reset/starfive/reset-starfive-jh7110.c b/drivers/reset/starfive/reset-starfive-jh7110.c
> new file mode 100644
> index 000000000000..83577d1b7fc6
> --- /dev/null
> +++ b/drivers/reset/starfive/reset-starfive-jh7110.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Reset driver for the StarFive JH7110 SoC
> + *
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +
> +#include "reset-starfive-jh71x0.h"
> +
> +#include <dt-bindings/reset/starfive,jh7110-crg.h>
> +
> +static int jh7110_reset_probe(struct auxiliary_device *adev,
> +                             const struct auxiliary_device_id *id)
> +{
> +       struct reset_info *info = (struct reset_info *)(id->driver_data);
> +       void __iomem **base = (void __iomem **)dev_get_drvdata(adev->dev.parent);

Hi Hal,

I saw the kernel test robot complain about this, but I still wonder if
the extra level of indirection is really needed. Isn't it enough to
just add the explicit casts, so

dev_set_drvdata(priv->dev, (void *)priv->base);

in the clock drivers and here just

void __iomem *base = (void __iomem *)dev_get_drvdata(adev->dev.parent);

> +
> +       if (!info || !base)
> +               return -ENODEV;
> +
> +       return reset_starfive_jh71x0_register(&adev->dev, adev->dev.parent->of_node,
> +                                             *base + info->assert_offset,
> +                                             *base + info->status_offset,
> +                                             NULL,
> +                                             info->nr_resets,
> +                                             NULL);
> +}
> +
> +static const struct reset_info jh7110_sys_info = {
> +       .nr_resets = JH7110_SYSRST_END,
> +       .assert_offset = 0x2F8,
> +       .status_offset = 0x308,
> +};
> +
> +static const struct reset_info jh7110_aon_info = {
> +       .nr_resets = JH7110_AONRST_END,
> +       .assert_offset = 0x38,
> +       .status_offset = 0x3C,
> +};
> +
> +static const struct auxiliary_device_id jh7110_reset_ids[] = {
> +       {
> +               .name = "clk_starfive_jh71x0.reset-sys",
> +               .driver_data = (kernel_ulong_t)&jh7110_sys_info,
> +       },
> +       {
> +               .name = "clk_starfive_jh71x0.reset-aon",
> +               .driver_data = (kernel_ulong_t)&jh7110_aon_info,
> +       },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, jh7110_reset_ids);
> +
> +static struct auxiliary_driver jh7110_reset_driver = {
> +       .probe          = jh7110_reset_probe,
> +       .id_table       = jh7110_reset_ids,
> +};
> +module_auxiliary_driver(jh7110_reset_driver);
> +
> +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
> +MODULE_DESCRIPTION("StarFive JH7110 reset driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/reset/starfive/reset-starfive-jh71x0.h b/drivers/reset/starfive/reset-starfive-jh71x0.h
> index db7d39a87f87..e485472e1adc 100644
> --- a/drivers/reset/starfive/reset-starfive-jh71x0.h
> +++ b/drivers/reset/starfive/reset-starfive-jh71x0.h
> @@ -6,6 +6,12 @@
>  #ifndef __RESET_STARFIVE_JH71X0_H
>  #define __RESET_STARFIVE_JH71X0_H
>
> +struct reset_info {
> +       unsigned int nr_resets;
> +       unsigned int assert_offset;
> +       unsigned int status_offset;
> +};

As far as I can tell this struct isn't used anywhere but in
reset-starfive-jh7110.c. If so just move it there, and in any case
please call it something less generic like struct jh7110_reset_info.

> +
>  int reset_starfive_jh71x0_register(struct device *dev, struct device_node *of_node,
>                                    void __iomem *assert, void __iomem *status,
>                                    const u32 *asserted, unsigned int nr_resets,
> --
> 2.38.1
>
  

On Tue, Feb 21, 2023 at 04:33:09PM +0100, Emil Renner Berthing wrote:
> On Tue, 21 Feb 2023 at 03:47, Hal Feng <hal.feng@starfivetech.com> wrote:
> >
> > Add auxiliary driver to support StarFive JH7110 system
> > and always-on resets.
> >
> > Reported-by: kernel test robot <lkp@intel.com>

Drop the reported-by here too please Hal.

> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>

> > +static int jh7110_reset_probe(struct auxiliary_device *adev,
> > +                             const struct auxiliary_device_id *id)
> > +{
> > +       struct reset_info *info = (struct reset_info *)(id->driver_data);
> > +       void __iomem **base = (void __iomem **)dev_get_drvdata(adev->dev.parent);
> 
> Hi Hal,
> 
> I saw the kernel test robot complain about this, but I still wonder if
> the extra level of indirection is really needed. Isn't it enough to
> just add the explicit casts, so
> 
> dev_set_drvdata(priv->dev, (void *)priv->base);
> 
> in the clock drivers and here just
> 
> void __iomem *base = (void __iomem *)dev_get_drvdata(adev->dev.parent);

I *think* if you do that, sparse will complain that you cast away the
__iomem. The complaint is something like "cast removes address space
qualifier from expression".

The other option is, rather than set the base as the drvdata, just pass
the whole priv struct. That's what I did for mpfs at least & I thought I
had suggested it on v3, but must not have.
It looks prettier than the casting madness at least ;)

> > +
> > +       if (!info || !base)
> > +               return -ENODEV;
> > +
> > +       return reset_starfive_jh71x0_register(&adev->dev, adev->dev.parent->of_node,
> > +                                             *base + info->assert_offset,
> > +                                             *base + info->status_offset,
> > +                                             NULL,
> > +                                             info->nr_resets,
> > +                                             NULL);
> > +}
  

On Tue, 21 Feb 2023 16:33:09 +0100, Emil Renner Berthing wrote:
> On Tue, 21 Feb 2023 at 03:47, Hal Feng <hal.feng@starfivetech.com> wrote:
>>
>> Add auxiliary driver to support StarFive JH7110 system
>> and always-on resets.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> ---
>>  drivers/reset/starfive/Kconfig                |  8 +++
>>  drivers/reset/starfive/Makefile               |  1 +
>>  .../reset/starfive/reset-starfive-jh7110.c    | 64 +++++++++++++++++++
>>  .../reset/starfive/reset-starfive-jh71x0.h    |  6 ++
>>  4 files changed, 79 insertions(+)
>>  create mode 100644 drivers/reset/starfive/reset-starfive-jh7110.c
>>
[...]
>> diff --git a/drivers/reset/starfive/reset-starfive-jh71x0.h b/drivers/reset/starfive/reset-starfive-jh71x0.h
>> index db7d39a87f87..e485472e1adc 100644
>> --- a/drivers/reset/starfive/reset-starfive-jh71x0.h
>> +++ b/drivers/reset/starfive/reset-starfive-jh71x0.h
>> @@ -6,6 +6,12 @@
>>  #ifndef __RESET_STARFIVE_JH71X0_H
>>  #define __RESET_STARFIVE_JH71X0_H
>>
>> +struct reset_info {
>> +       unsigned int nr_resets;
>> +       unsigned int assert_offset;
>> +       unsigned int status_offset;
>> +};
> 
> As far as I can tell this struct isn't used anywhere but in
> reset-starfive-jh7110.c. If so just move it there, and in any case
> please call it something less generic like struct jh7110_reset_info.

OK, will fix it. Thanks.

Best regards,
Hal

> 
>> +
>>  int reset_starfive_jh71x0_register(struct device *dev, struct device_node *of_node,
>>                                    void __iomem *assert, void __iomem *status,
>>                                    const u32 *asserted, unsigned int nr_resets,
  

On Tue, 21 Feb 2023 16:34:11 +0000, Conor Dooley wrote:
> On Tue, Feb 21, 2023 at 04:33:09PM +0100, Emil Renner Berthing wrote:
>> On Tue, 21 Feb 2023 at 03:47, Hal Feng <hal.feng@starfivetech.com> wrote:
>> >
>> > Add auxiliary driver to support StarFive JH7110 system
>> > and always-on resets.
>> >
>> > Reported-by: kernel test robot <lkp@intel.com>
> 
> Drop the reported-by here too please Hal.

OK.

> 
>> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> 
>> > +static int jh7110_reset_probe(struct auxiliary_device *adev,
>> > +                             const struct auxiliary_device_id *id)
>> > +{
>> > +       struct reset_info *info = (struct reset_info *)(id->driver_data);
>> > +       void __iomem **base = (void __iomem **)dev_get_drvdata(adev->dev.parent);
>> 
>> Hi Hal,
>> 
>> I saw the kernel test robot complain about this, but I still wonder if
>> the extra level of indirection is really needed. Isn't it enough to
>> just add the explicit casts, so
>> 
>> dev_set_drvdata(priv->dev, (void *)priv->base);
>> 
>> in the clock drivers and here just
>> 
>> void __iomem *base = (void __iomem *)dev_get_drvdata(adev->dev.parent);
> 
> I *think* if you do that, sparse will complain that you cast away the
> __iomem. The complaint is something like "cast removes address space
> qualifier from expression".
> 
> The other option is, rather than set the base as the drvdata, just pass
> the whole priv struct. That's what I did for mpfs at least & I thought I
> had suggested it on v3, but must not have.
> It looks prettier than the casting madness at least ;)

I modified this just because we need to use container_of() to get some
struct in [1].

+struct isp_top_crg {
+	struct clk_bulk_data *top_clks;
+	struct reset_control *top_rsts;
+	int top_clks_num;
+	void __iomem *base;
+};

+static struct isp_top_crg *top_crg_from(void __iomem **base)
+{
+	return container_of(base, struct isp_top_crg, base);
+}

[1] https://lore.kernel.org/all/20230221083323.302471-7-xingyu.wu@starfivetech.com/

If we pass the whole priv struct, we need to make the priv struct
public. I think setting the address of "base" as the drvdata is
enough and easier.

Best regards,
Hal

> 
>> > +
>> > +       if (!info || !base)
>> > +               return -ENODEV;
>> > +
>> > +       return reset_starfive_jh71x0_register(&adev->dev, adev->dev.parent->of_node,
>> > +                                             *base + info->assert_offset,
>> > +                                             *base + info->status_offset,
>> > +                                             NULL,
>> > +                                             info->nr_resets,
>> > +                                             NULL);
>> > +}