| Message ID | 20240710162526.2341399-2-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reset: amlogic: move audio reset drivers out of CCF | expand |
On 7/10/24 19:25, Jerome Brunet wrote: > To allow using the same driver for the main reset controller and the > auxiliary ones embedded in the clock controllers, convert the > the Amlogic reset driver to regmap. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/reset/reset-meson.c | 80 ++++++++++++++++++++----------------- > 1 file changed, 44 insertions(+), 36 deletions(-) > > diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c > index f78be97898bc..8f3d6e9df235 100644 > --- a/drivers/reset/reset-meson.c > +++ b/drivers/reset/reset-meson.c > @@ -11,36 +11,44 @@ > #include <linux/of.h> > #include <linux/module.h> > #include <linux/platform_device.h> > +#include <linux/regmap.h> > #include <linux/reset-controller.h> > #include <linux/slab.h> > #include <linux/types.h> > > -#define BITS_PER_REG 32 > - > struct meson_reset_param { > int reg_count; > int level_offset; > }; > > struct meson_reset { > - void __iomem *reg_base; > const struct meson_reset_param *param; > struct reset_controller_dev rcdev; > - spinlock_t lock; > + struct regmap *map; > }; > > +static void meson_reset_offset_and_bit(struct meson_reset *data, > + unsigned long id, > + unsigned int *offset, > + unsigned int *bit) > +{ > + unsigned int stride = regmap_get_reg_stride(data->map); > + > + *offset = (id / (stride * BITS_PER_BYTE)) * stride; > + *bit = id % (stride * BITS_PER_BYTE); > +} > + > static int meson_reset_reset(struct reset_controller_dev *rcdev, > - unsigned long id) > + unsigned long id) > { > struct meson_reset *data = > container_of(rcdev, struct meson_reset, rcdev); > - unsigned int bank = id / BITS_PER_REG; > - unsigned int offset = id % BITS_PER_REG; > - void __iomem *reg_addr = data->reg_base + (bank << 2); > + unsigned int offset, bit; > > - writel(BIT(offset), reg_addr); > + meson_reset_offset_and_bit(data, id, &offset, &bit); > > - return 0; > + return regmap_update_bits(data->map, offset, > + BIT(bit), BIT(bit)); regmap_update_bits() is not equal to writel(), I suppose regmap_write() should be here. I'm still under testing, but I see unexpected behavior on A1 SoC: [ 1.482446] Call trace: [ 1.484860] dump_backtrace+0x94/0xec [ 1.488482] show_stack+0x18/0x24 [ 1.491754] dump_stack_lvl+0x78/0x90 [ 1.495377] dump_stack+0x18/0x24 [ 1.498654] __report_bad_irq+0x38/0xe4 [ 1.502453] note_interrupt+0x324/0x374 [ 1.506244] handle_irq_event+0x9c/0xac [ 1.510039] handle_fasteoi_irq+0xa4/0x238 [ 1.514093] generic_handle_domain_irq+0x2c/0x44 [ 1.518664] gic_handle_irq+0x40/0xc4 [ 1.522287] call_on_irq_stack+0x24/0x4c [ 1.526168] do_interrupt_handler+0x80/0x84 [ 1.530308] el1_interrupt+0x34/0x68 [ 1.533844] el1h_64_irq_handler+0x18/0x24 [ 1.537898] el1h_64_irq+0x64/0x68 [ 1.541262] default_idle_call+0x28/0x3c [ 1.545143] do_idle+0x208/0x260 [ 1.548334] cpu_startup_entry+0x38/0x3c [ 1.552220] kernel_init+0x0/0x1dc [ 1.555579] start_kernel+0x5ac/0x6f0 [ 1.559206] __primary_switched+0x80/0x88 and using of regmap_write() (apparently) fixes it. > } > > static int meson_reset_level(struct reset_controller_dev *rcdev, > @@ -48,25 +56,13 @@ static int meson_reset_level(struct reset_controller_dev *rcdev, > { > struct meson_reset *data = > container_of(rcdev, struct meson_reset, rcdev); > - unsigned int bank = id / BITS_PER_REG; > - unsigned int offset = id % BITS_PER_REG; > - void __iomem *reg_addr; > - unsigned long flags; > - u32 reg; > + unsigned int offset, bit; > > - reg_addr = data->reg_base + data->param->level_offset + (bank << 2); > + meson_reset_offset_and_bit(data, id, &offset, &bit); > + offset += data->param->level_offset; > > - spin_lock_irqsave(&data->lock, flags); > - > - reg = readl(reg_addr); > - if (assert) > - writel(reg & ~BIT(offset), reg_addr); > - else > - writel(reg | BIT(offset), reg_addr); > - > - spin_unlock_irqrestore(&data->lock, flags); > - > - return 0; > + return regmap_update_bits(data->map, offset, > + BIT(bit), assert ? 0 : BIT(bit)); > } > > static int meson_reset_assert(struct reset_controller_dev *rcdev, > @@ -113,30 +109,42 @@ static const struct of_device_id meson_reset_dt_ids[] = { > }; > MODULE_DEVICE_TABLE(of, meson_reset_dt_ids); > > +static const struct regmap_config regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > +}; > + > static int meson_reset_probe(struct platform_device *pdev) > { > + struct device *dev = &pdev->dev; > struct meson_reset *data; > + void __iomem *base; > > - data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > if (!data) > return -ENOMEM; > > - data->reg_base = devm_platform_ioremap_resource(pdev, 0); > - if (IS_ERR(data->reg_base)) > - return PTR_ERR(data->reg_base); > + base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(base)) > + return PTR_ERR(base); > > - data->param = of_device_get_match_data(&pdev->dev); > + data->param = of_device_get_match_data(dev); > if (!data->param) > return -ENODEV; > > - spin_lock_init(&data->lock); > + data->map = devm_regmap_init_mmio(dev, base, ®map_config); > + if (IS_ERR(data->map)) > + return dev_err_probe(dev, PTR_ERR(data->map), > + "can't init regmap mmio region\n"); > > data->rcdev.owner = THIS_MODULE; > - data->rcdev.nr_resets = data->param->reg_count * BITS_PER_REG; > + data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE > + * regmap_config.reg_stride; > data->rcdev.ops = &meson_reset_ops; > - data->rcdev.of_node = pdev->dev.of_node; > + data->rcdev.of_node = dev->of_node; > > - return devm_reset_controller_register(&pdev->dev, &data->rcdev); > + return devm_reset_controller_register(dev, &data->rcdev); > } > > static struct platform_driver meson_reset_driver = {
On Thu 18 Jul 2024 at 05:39, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > On 7/10/24 19:25, Jerome Brunet wrote: >> To allow using the same driver for the main reset controller and the >> auxiliary ones embedded in the clock controllers, convert the >> the Amlogic reset driver to regmap. >> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> drivers/reset/reset-meson.c | 80 ++++++++++++++++++++----------------- >> 1 file changed, 44 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c >> index f78be97898bc..8f3d6e9df235 100644 >> --- a/drivers/reset/reset-meson.c >> +++ b/drivers/reset/reset-meson.c >> @@ -11,36 +11,44 @@ >> #include <linux/of.h> >> #include <linux/module.h> >> #include <linux/platform_device.h> >> +#include <linux/regmap.h> >> #include <linux/reset-controller.h> >> #include <linux/slab.h> >> #include <linux/types.h> >> >> -#define BITS_PER_REG 32 >> - >> struct meson_reset_param { >> int reg_count; >> int level_offset; >> }; >> >> struct meson_reset { >> - void __iomem *reg_base; >> const struct meson_reset_param *param; >> struct reset_controller_dev rcdev; >> - spinlock_t lock; >> + struct regmap *map; >> }; >> >> +static void meson_reset_offset_and_bit(struct meson_reset *data, >> + unsigned long id, >> + unsigned int *offset, >> + unsigned int *bit) >> +{ >> + unsigned int stride = regmap_get_reg_stride(data->map); >> + >> + *offset = (id / (stride * BITS_PER_BYTE)) * stride; >> + *bit = id % (stride * BITS_PER_BYTE); >> +} >> + >> static int meson_reset_reset(struct reset_controller_dev *rcdev, >> - unsigned long id) >> + unsigned long id) >> { >> struct meson_reset *data = >> container_of(rcdev, struct meson_reset, rcdev); >> - unsigned int bank = id / BITS_PER_REG; >> - unsigned int offset = id % BITS_PER_REG; >> - void __iomem *reg_addr = data->reg_base + (bank << 2); >> + unsigned int offset, bit; >> >> - writel(BIT(offset), reg_addr); >> + meson_reset_offset_and_bit(data, id, &offset, &bit); >> >> - return 0; >> + return regmap_update_bits(data->map, offset, >> + BIT(bit), BIT(bit)); > > regmap_update_bits() is not equal to writel(), I suppose regmap_write() > should be here. > > I'm still under testing, but I see unexpected behavior on A1 SoC: > > [ 1.482446] Call trace: > [ 1.484860] dump_backtrace+0x94/0xec > [ 1.488482] show_stack+0x18/0x24 > [ 1.491754] dump_stack_lvl+0x78/0x90 > [ 1.495377] dump_stack+0x18/0x24 > [ 1.498654] __report_bad_irq+0x38/0xe4 > [ 1.502453] note_interrupt+0x324/0x374 > [ 1.506244] handle_irq_event+0x9c/0xac > [ 1.510039] handle_fasteoi_irq+0xa4/0x238 > [ 1.514093] generic_handle_domain_irq+0x2c/0x44 > [ 1.518664] gic_handle_irq+0x40/0xc4 > [ 1.522287] call_on_irq_stack+0x24/0x4c > [ 1.526168] do_interrupt_handler+0x80/0x84 > [ 1.530308] el1_interrupt+0x34/0x68 > [ 1.533844] el1h_64_irq_handler+0x18/0x24 > [ 1.537898] el1h_64_irq+0x64/0x68 > [ 1.541262] default_idle_call+0x28/0x3c > [ 1.545143] do_idle+0x208/0x260 > [ 1.548334] cpu_startup_entry+0x38/0x3c > [ 1.552220] kernel_init+0x0/0x1dc > [ 1.555579] start_kernel+0x5ac/0x6f0 > [ 1.559206] __primary_switched+0x80/0x88 That stack trace is not helping > > > and using of regmap_write() (apparently) fixes it. Nor does that conclusion. It is perfectly possible I have made mistake somewhere (I have already fixed one in v2), but sending incomplete report like this, with unargumented conclusion is just noise in the end. No, there is no situation in which `regmap_write` would solve a problem with `regmap_update_bits`. Either send a full analysis of the problem you found, if you did one, or at least send the full dump, explaining that you don't know what is happening. I'll also point that I did not send g12 clock along with v1 and that a1 support is not upstream or even reviewed. So this 'tests' obviously rely on an integration of v1, rfc and your custom develpment. That is hardly a reference at this point. This patches address g12 and sm1. If you want to contribute with tests, please test on these platforms. a1 will be addressed in due time. Testing is good but making complete report (trace and environment details) is equally important, especially for maintainers who might not be following Amlogic development as much as you do. > >> } >> >> static int meson_reset_level(struct reset_controller_dev *rcdev, >> @@ -48,25 +56,13 @@ static int meson_reset_level(struct reset_controller_dev *rcdev, >> { >> struct meson_reset *data = >> container_of(rcdev, struct meson_reset, rcdev); >> - unsigned int bank = id / BITS_PER_REG; >> - unsigned int offset = id % BITS_PER_REG; >> - void __iomem *reg_addr; >> - unsigned long flags; >> - u32 reg; >> + unsigned int offset, bit; >> >> - reg_addr = data->reg_base + data->param->level_offset + (bank << 2); >> + meson_reset_offset_and_bit(data, id, &offset, &bit); >> + offset += data->param->level_offset; >> >> - spin_lock_irqsave(&data->lock, flags); >> - >> - reg = readl(reg_addr); >> - if (assert) >> - writel(reg & ~BIT(offset), reg_addr); >> - else >> - writel(reg | BIT(offset), reg_addr); >> - >> - spin_unlock_irqrestore(&data->lock, flags); >> - >> - return 0; >> + return regmap_update_bits(data->map, offset, >> + BIT(bit), assert ? 0 : BIT(bit)); >> } >> >> static int meson_reset_assert(struct reset_controller_dev *rcdev, >> @@ -113,30 +109,42 @@ static const struct of_device_id meson_reset_dt_ids[] = { >> }; >> MODULE_DEVICE_TABLE(of, meson_reset_dt_ids); >> >> +static const struct regmap_config regmap_config = { >> + .reg_bits = 32, >> + .val_bits = 32, >> + .reg_stride = 4, >> +}; >> + >> static int meson_reset_probe(struct platform_device *pdev) >> { >> + struct device *dev = &pdev->dev; >> struct meson_reset *data; >> + void __iomem *base; >> >> - data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >> if (!data) >> return -ENOMEM; >> >> - data->reg_base = devm_platform_ioremap_resource(pdev, 0); >> - if (IS_ERR(data->reg_base)) >> - return PTR_ERR(data->reg_base); >> + base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> >> - data->param = of_device_get_match_data(&pdev->dev); >> + data->param = of_device_get_match_data(dev); >> if (!data->param) >> return -ENODEV; >> >> - spin_lock_init(&data->lock); >> + data->map = devm_regmap_init_mmio(dev, base, ®map_config); >> + if (IS_ERR(data->map)) >> + return dev_err_probe(dev, PTR_ERR(data->map), >> + "can't init regmap mmio region\n"); >> >> data->rcdev.owner = THIS_MODULE; >> - data->rcdev.nr_resets = data->param->reg_count * BITS_PER_REG; >> + data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE >> + * regmap_config.reg_stride; >> data->rcdev.ops = &meson_reset_ops; >> - data->rcdev.of_node = pdev->dev.of_node; >> + data->rcdev.of_node = dev->of_node; >> >> - return devm_reset_controller_register(&pdev->dev, &data->rcdev); >> + return devm_reset_controller_register(dev, &data->rcdev); >> } >> >> static struct platform_driver meson_reset_driver = {
On 7/18/24 10:19, Jerome Brunet wrote: > On Thu 18 Jul 2024 at 05:39, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> On 7/10/24 19:25, Jerome Brunet wrote: >>> To allow using the same driver for the main reset controller and the >>> auxiliary ones embedded in the clock controllers, convert the >>> the Amlogic reset driver to regmap. >>> >>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >>> --- >>> drivers/reset/reset-meson.c | 80 ++++++++++++++++++++----------------- >>> 1 file changed, 44 insertions(+), 36 deletions(-) >>> >>> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c >>> index f78be97898bc..8f3d6e9df235 100644 >>> --- a/drivers/reset/reset-meson.c >>> +++ b/drivers/reset/reset-meson.c >>> @@ -11,36 +11,44 @@ >>> #include <linux/of.h> >>> #include <linux/module.h> >>> #include <linux/platform_device.h> >>> +#include <linux/regmap.h> >>> #include <linux/reset-controller.h> >>> #include <linux/slab.h> >>> #include <linux/types.h> >>> >>> -#define BITS_PER_REG 32 >>> - >>> struct meson_reset_param { >>> int reg_count; >>> int level_offset; >>> }; >>> >>> struct meson_reset { >>> - void __iomem *reg_base; >>> const struct meson_reset_param *param; >>> struct reset_controller_dev rcdev; >>> - spinlock_t lock; >>> + struct regmap *map; >>> }; >>> >>> +static void meson_reset_offset_and_bit(struct meson_reset *data, >>> + unsigned long id, >>> + unsigned int *offset, >>> + unsigned int *bit) >>> +{ >>> + unsigned int stride = regmap_get_reg_stride(data->map); >>> + >>> + *offset = (id / (stride * BITS_PER_BYTE)) * stride; >>> + *bit = id % (stride * BITS_PER_BYTE); >>> +} >>> + >>> static int meson_reset_reset(struct reset_controller_dev *rcdev, >>> - unsigned long id) >>> + unsigned long id) >>> { >>> struct meson_reset *data = >>> container_of(rcdev, struct meson_reset, rcdev); >>> - unsigned int bank = id / BITS_PER_REG; >>> - unsigned int offset = id % BITS_PER_REG; >>> - void __iomem *reg_addr = data->reg_base + (bank << 2); >>> + unsigned int offset, bit; >>> >>> - writel(BIT(offset), reg_addr); >>> + meson_reset_offset_and_bit(data, id, &offset, &bit); >>> >>> - return 0; >>> + return regmap_update_bits(data->map, offset, >>> + BIT(bit), BIT(bit)); >> >> regmap_update_bits() is not equal to writel(), I suppose regmap_write() >> should be here. >> >> I'm still under testing, but I see unexpected behavior on A1 SoC: >> >> [ 1.482446] Call trace: >> [ 1.484860] dump_backtrace+0x94/0xec >> [ 1.488482] show_stack+0x18/0x24 >> [ 1.491754] dump_stack_lvl+0x78/0x90 >> [ 1.495377] dump_stack+0x18/0x24 >> [ 1.498654] __report_bad_irq+0x38/0xe4 >> [ 1.502453] note_interrupt+0x324/0x374 >> [ 1.506244] handle_irq_event+0x9c/0xac >> [ 1.510039] handle_fasteoi_irq+0xa4/0x238 >> [ 1.514093] generic_handle_domain_irq+0x2c/0x44 >> [ 1.518664] gic_handle_irq+0x40/0xc4 >> [ 1.522287] call_on_irq_stack+0x24/0x4c >> [ 1.526168] do_interrupt_handler+0x80/0x84 >> [ 1.530308] el1_interrupt+0x34/0x68 >> [ 1.533844] el1h_64_irq_handler+0x18/0x24 >> [ 1.537898] el1h_64_irq+0x64/0x68 >> [ 1.541262] default_idle_call+0x28/0x3c >> [ 1.545143] do_idle+0x208/0x260 >> [ 1.548334] cpu_startup_entry+0x38/0x3c >> [ 1.552220] kernel_init+0x0/0x1dc >> [ 1.555579] start_kernel+0x5ac/0x6f0 >> [ 1.559206] __primary_switched+0x80/0x88 > > That stack trace is not helping > >> >> >> and using of regmap_write() (apparently) fixes it. > > Nor does that conclusion. > > It is perfectly possible I have made mistake somewhere (I have already > fixed one in v2), but sending incomplete report like this, with > unargumented conclusion is just noise in the end. > > No, there is no situation in which `regmap_write` would solve a problem > with `regmap_update_bits`. > What is the default regs' value of this reset controller? The doc that I have doesn't clearly specifies it, but it tells that "the reset will auto-cover to 0 by HW". However pr_info() say that there is 0xffffffff before write (I am talking about A1). Also we know, that reset is triggered by writing 1 to specific bit. So, what will happen if 1 will be written where we did not intend to write it? > Either send a full analysis of the problem you found, if you did one, or > at least send the full dump, explaining that you don't know what is > happening. > Full analysis is following: - using regmap_update_bits() instead of writel() is incorrect because this changes the behavior of the driver - regmap_update_bits() should not be used here because default value of regs isn't taken into account and (_apparently_, the doc is terse) these regs could be updated by hw itself. > I'll also point that I did not send g12 clock along with v1 and that a1 > support is not upstream or even reviewed. So this 'tests' obviously rely > on an integration of v1, rfc and your custom develpment. That is hardly a > reference at this point. > > This patches address g12 and sm1. If you want to contribute with tests, > please test on these platforms. a1 will be addressed in due time. > Where did you find in my words any reference to not upstreamed functionality? Support of A1 SoC is here since commit bdb369e1e98a ("reset: add support for the Meson-A1 SoC Reset Controller"). > Testing is good but making complete report (trace and environment > details) is equally important, especially for maintainers who might not > be following Amlogic development as much as you do. > >> >>> } >>> >>> static int meson_reset_level(struct reset_controller_dev *rcdev, >>> @@ -48,25 +56,13 @@ static int meson_reset_level(struct reset_controller_dev *rcdev, >>> { >>> struct meson_reset *data = >>> container_of(rcdev, struct meson_reset, rcdev); >>> - unsigned int bank = id / BITS_PER_REG; >>> - unsigned int offset = id % BITS_PER_REG; >>> - void __iomem *reg_addr; >>> - unsigned long flags; >>> - u32 reg; >>> + unsigned int offset, bit; >>> >>> - reg_addr = data->reg_base + data->param->level_offset + (bank << 2); >>> + meson_reset_offset_and_bit(data, id, &offset, &bit); >>> + offset += data->param->level_offset; >>> >>> - spin_lock_irqsave(&data->lock, flags); >>> - >>> - reg = readl(reg_addr); >>> - if (assert) >>> - writel(reg & ~BIT(offset), reg_addr); >>> - else >>> - writel(reg | BIT(offset), reg_addr); >>> - >>> - spin_unlock_irqrestore(&data->lock, flags); >>> - >>> - return 0; >>> + return regmap_update_bits(data->map, offset, >>> + BIT(bit), assert ? 0 : BIT(bit)); >>> } >>> >>> static int meson_reset_assert(struct reset_controller_dev *rcdev, >>> @@ -113,30 +109,42 @@ static const struct of_device_id meson_reset_dt_ids[] = { >>> }; >>> MODULE_DEVICE_TABLE(of, meson_reset_dt_ids); >>> >>> +static const struct regmap_config regmap_config = { >>> + .reg_bits = 32, >>> + .val_bits = 32, >>> + .reg_stride = 4, >>> +}; >>> + >>> static int meson_reset_probe(struct platform_device *pdev) >>> { >>> + struct device *dev = &pdev->dev; >>> struct meson_reset *data; >>> + void __iomem *base; >>> >>> - data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >>> if (!data) >>> return -ENOMEM; >>> >>> - data->reg_base = devm_platform_ioremap_resource(pdev, 0); >>> - if (IS_ERR(data->reg_base)) >>> - return PTR_ERR(data->reg_base); >>> + base = devm_platform_ioremap_resource(pdev, 0); >>> + if (IS_ERR(base)) >>> + return PTR_ERR(base); >>> >>> - data->param = of_device_get_match_data(&pdev->dev); >>> + data->param = of_device_get_match_data(dev); >>> if (!data->param) >>> return -ENODEV; >>> >>> - spin_lock_init(&data->lock); >>> + data->map = devm_regmap_init_mmio(dev, base, ®map_config); >>> + if (IS_ERR(data->map)) >>> + return dev_err_probe(dev, PTR_ERR(data->map), >>> + "can't init regmap mmio region\n"); >>> >>> data->rcdev.owner = THIS_MODULE; >>> - data->rcdev.nr_resets = data->param->reg_count * BITS_PER_REG; >>> + data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE >>> + * regmap_config.reg_stride; >>> data->rcdev.ops = &meson_reset_ops; >>> - data->rcdev.of_node = pdev->dev.of_node; >>> + data->rcdev.of_node = dev->of_node; >>> >>> - return devm_reset_controller_register(&pdev->dev, &data->rcdev); >>> + return devm_reset_controller_register(dev, &data->rcdev); >>> } >>> >>> static struct platform_driver meson_reset_driver = { >
Quoting Jan Dakinevich (2024-07-18 04:01:28) > > > On 7/18/24 10:19, Jerome Brunet wrote: > > On Thu 18 Jul 2024 at 05:39, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> and using of regmap_write() (apparently) fixes it. > > > > Nor does that conclusion. > > > It is perfectly possible I have made mistake somewhere (I have already > > fixed one in v2), but sending incomplete report like this, with > > unargumented conclusion is just noise in the end. > > > > No, there is no situation in which `regmap_write` would solve a problem > > with `regmap_update_bits`. > > > > What is the default regs' value of this reset controller? The doc that I > have doesn't clearly specifies it, but it tells that "the reset will > auto-cover to 0 by HW". However pr_info() say that there is 0xffffffff > before write (I am talking about A1). > > Also we know, that reset is triggered by writing 1 to specific bit. So, > what will happen if 1 will be written where we did not intend to write it? > > > Either send a full analysis of the problem you found, if you did one, or > > at least send the full dump, explaining that you don't know what is > > happening. > > > > Full analysis is following: > - using regmap_update_bits() instead of writel() is incorrect because > this changes the behavior of the driver > - regmap_update_bits() should not be used here because default value of > regs isn't taken into account and (_apparently_, the doc is terse) these > regs could be updated by hw itself. Maybe use regmap_write_bits() instead.
On Thu 18 Jul 2024 at 12:29, Stephen Boyd <sboyd@kernel.org> wrote: >> >> Full analysis is following: >> - using regmap_update_bits() instead of writel() is incorrect because >> this changes the behavior of the driver >> - regmap_update_bits() should not be used here because default value of >> regs isn't taken into account and (_apparently_, the doc is terse) these >> regs could be updated by hw itself. > > Maybe use regmap_write_bits() instead. Actually regmap_write_bits() performs an update behind the scene. You'd still get the undefined read value making a mess AFAICT. I'll stick to the usual regmap_write().
diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c index f78be97898bc..8f3d6e9df235 100644 --- a/drivers/reset/reset-meson.c +++ b/drivers/reset/reset-meson.c @@ -11,36 +11,44 @@ #include <linux/of.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/reset-controller.h> #include <linux/slab.h> #include <linux/types.h> -#define BITS_PER_REG 32 - struct meson_reset_param { int reg_count; int level_offset; }; struct meson_reset { - void __iomem *reg_base; const struct meson_reset_param *param; struct reset_controller_dev rcdev; - spinlock_t lock; + struct regmap *map; }; +static void meson_reset_offset_and_bit(struct meson_reset *data, + unsigned long id, + unsigned int *offset, + unsigned int *bit) +{ + unsigned int stride = regmap_get_reg_stride(data->map); + + *offset = (id / (stride * BITS_PER_BYTE)) * stride; + *bit = id % (stride * BITS_PER_BYTE); +} + static int meson_reset_reset(struct reset_controller_dev *rcdev, - unsigned long id) + unsigned long id) { struct meson_reset *data = container_of(rcdev, struct meson_reset, rcdev); - unsigned int bank = id / BITS_PER_REG; - unsigned int offset = id % BITS_PER_REG; - void __iomem *reg_addr = data->reg_base + (bank << 2); + unsigned int offset, bit; - writel(BIT(offset), reg_addr); + meson_reset_offset_and_bit(data, id, &offset, &bit); - return 0; + return regmap_update_bits(data->map, offset, + BIT(bit), BIT(bit)); } static int meson_reset_level(struct reset_controller_dev *rcdev, @@ -48,25 +56,13 @@ static int meson_reset_level(struct reset_controller_dev *rcdev, { struct meson_reset *data = container_of(rcdev, struct meson_reset, rcdev); - unsigned int bank = id / BITS_PER_REG; - unsigned int offset = id % BITS_PER_REG; - void __iomem *reg_addr; - unsigned long flags; - u32 reg; + unsigned int offset, bit; - reg_addr = data->reg_base + data->param->level_offset + (bank << 2); + meson_reset_offset_and_bit(data, id, &offset, &bit); + offset += data->param->level_offset; - spin_lock_irqsave(&data->lock, flags); - - reg = readl(reg_addr); - if (assert) - writel(reg & ~BIT(offset), reg_addr); - else - writel(reg | BIT(offset), reg_addr); - - spin_unlock_irqrestore(&data->lock, flags); - - return 0; + return regmap_update_bits(data->map, offset, + BIT(bit), assert ? 0 : BIT(bit)); } static int meson_reset_assert(struct reset_controller_dev *rcdev, @@ -113,30 +109,42 @@ static const struct of_device_id meson_reset_dt_ids[] = { }; MODULE_DEVICE_TABLE(of, meson_reset_dt_ids); +static const struct regmap_config regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, +}; + static int meson_reset_probe(struct platform_device *pdev) { + struct device *dev = &pdev->dev; struct meson_reset *data; + void __iomem *base; - data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM; - data->reg_base = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(data->reg_base)) - return PTR_ERR(data->reg_base); + base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(base)) + return PTR_ERR(base); - data->param = of_device_get_match_data(&pdev->dev); + data->param = of_device_get_match_data(dev); if (!data->param) return -ENODEV; - spin_lock_init(&data->lock); + data->map = devm_regmap_init_mmio(dev, base, ®map_config); + if (IS_ERR(data->map)) + return dev_err_probe(dev, PTR_ERR(data->map), + "can't init regmap mmio region\n"); data->rcdev.owner = THIS_MODULE; - data->rcdev.nr_resets = data->param->reg_count * BITS_PER_REG; + data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE + * regmap_config.reg_stride; data->rcdev.ops = &meson_reset_ops; - data->rcdev.of_node = pdev->dev.of_node; + data->rcdev.of_node = dev->of_node; - return devm_reset_controller_register(&pdev->dev, &data->rcdev); + return devm_reset_controller_register(dev, &data->rcdev); } static struct platform_driver meson_reset_driver = {
To allow using the same driver for the main reset controller and the auxiliary ones embedded in the clock controllers, convert the the Amlogic reset driver to regmap. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/reset/reset-meson.c | 80 ++++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 36 deletions(-)