Re: [PATCH v2 13/14] reset: starfive: Add StarFive JH7110 reset driver

[Hal Feng <hal.feng@starfivetech.com>]

On Sat, 19 Nov 2022 01:14:50 +0800, Emil Renner Berthing wrote:
> On Fri, 18 Nov 2022 at 02:06, Hal Feng <hal.feng@starfivetech.com> wrote:
>> diff --git a/drivers/reset/starfive/reset-starfive-jh7110.c b/drivers/reset/starfive/reset-starfive-jh7110.c
>> new file mode 100644
>> index 000000000000..00f3b4ecfb02
>> --- /dev/null
>> +++ b/drivers/reset/starfive/reset-starfive-jh7110.c
>> @@ -0,0 +1,67 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Reset driver for the StarFive JH7110 SoC
>> + *
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + * Copyright (C) 2022 Hal Feng <hal.feng@starfivetech.com>
>> + */
>> +
>> +#include <linux/auxiliary_bus.h>
>> +
>> +#include "reset-starfive-jh71x0.h"
>> +
>> +#include <dt-bindings/reset/starfive-jh7110.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 = 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,
>> +                                             info->asserted,
>> +                                             info->nr_resets,
>> +                                             false);
>> +}
>> +
>> +static const struct reset_info jh7110_sys_info = {
>> +       .nr_resets = JH7110_SYSRST_END,
>> +       .assert_offset = 0x2F8,
>> +       .status_offset = 0x308,
>> +       .asserted = NULL,
>> +};
>> +
>> +static const struct reset_info jh7110_aon_info = {
>> +       .nr_resets = JH7110_AONRST_END,
>> +       .assert_offset = 0x38,
>> +       .status_offset = 0x3C,
>> +       .asserted = NULL,
>> +};
> 
> It doesn't seem like syscrg, aoncrg or stgcrg have any inverted lines.
> Do you know if any other CRGs do? If not you can just leave out the
> .asserted member and always pass NULL.

All JH7110 CRGs don't have inverted lines, and it could be modified as
what you said.

> 
>> +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_DESCRIPTION("StarFive JH7110 Reset Driver");
>> +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/reset/starfive/reset-starfive-jh71x0.h b/drivers/reset/starfive/reset-starfive-jh71x0.h
>> index e6b27110de48..63a94ee1b395 100644
>> --- a/drivers/reset/starfive/reset-starfive-jh71x0.h
>> +++ b/drivers/reset/starfive/reset-starfive-jh71x0.h
>> @@ -6,6 +6,13 @@
>>  #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;
>> +       const u32 *asserted;
>> +};
> 
> This struct isn't used outside of reset-starfive-jh7110.c, so no need
> to define it in this header.
> Also consider calling it jh7110_reset_info so it blends in with the
> functions defined in that file.

Maybe it can be used in JH7100 audio reset?

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,