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

From: Hal Feng
Date: Tue Nov 22 2022 - 00:56:28 EST


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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
>> + */
>> +
>> +#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@xxxxxxxxxxxxxxxx>");
>> +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,