From b5c506b163d305a8b6d5ead562699d3fc9935498 Mon Sep 17 00:00:00 2001 From: William Breathitt Gray Date: Mon, 20 Mar 2023 10:50:15 -0400 Subject: [PATCH 01/23] gpio: 104-dio-48e: Implement struct dio48e_gpio A private data structure struct dio48e_gpio is introduced to facilitate passage of the regmap and IRQ mask state for the device to the callback dio48e_handle_mask_sync(). This is in preparation for the removal of the handle_mask_sync() map parameter in a subsequent patch. Signed-off-by: William Breathitt Gray irq_mask; int err; unsigned int val; @@ -115,19 +125,19 @@ static int dio48e_handle_mask_sync(struct regmap *const map, const int index, return 0; /* remember the current mask for the next mask sync */ - *irq_mask = mask_buf; + dio48egpio->irq_mask = mask_buf; /* if all previously masked, enable interrupts when unmasking */ if (prev_mask == mask_buf_def) { - err = regmap_write(map, DIO48E_CLEAR_INTERRUPT, 0x00); + err = regmap_write(dio48egpio->map, DIO48E_CLEAR_INTERRUPT, 0x00); if (err) return err; - return regmap_write(map, DIO48E_ENABLE_INTERRUPT, 0x00); + return regmap_write(dio48egpio->map, DIO48E_ENABLE_INTERRUPT, 0x00); } /* if all are currently masked, disable interrupts */ if (mask_buf == mask_buf_def) - return regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val); + return regmap_read(dio48egpio->map, DIO48E_DISABLE_INTERRUPT, &val); return 0; } @@ -168,7 +178,7 @@ static int dio48e_probe(struct device *dev, unsigned int id) struct regmap *map; int err; struct regmap_irq_chip *chip; - unsigned int irq_mask; + struct dio48e_gpio *dio48egpio; struct regmap_irq_chip_data *chip_data; if (!devm_request_region(dev, base[id], DIO48E_EXTENT, name)) { @@ -186,12 +196,14 @@ static int dio48e_probe(struct device *dev, unsigned int id) return dev_err_probe(dev, PTR_ERR(map), "Unable to initialize register map\n"); - chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); - if (!chip) + dio48egpio = devm_kzalloc(dev, sizeof(*dio48egpio), GFP_KERNEL); + if (!dio48egpio) return -ENOMEM; - chip->irq_drv_data = devm_kzalloc(dev, sizeof(irq_mask), GFP_KERNEL); - if (!chip->irq_drv_data) + dio48egpio->map = map; + + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); + if (!chip) return -ENOMEM; chip->name = name; @@ -202,6 +214,7 @@ static int dio48e_probe(struct device *dev, unsigned int id) chip->irqs = dio48e_regmap_irqs; chip->num_irqs = ARRAY_SIZE(dio48e_regmap_irqs); chip->handle_mask_sync = dio48e_handle_mask_sync; + chip->irq_drv_data = dio48egpio; /* Initialize to prevent spurious interrupts before we're ready */ err = dio48e_irq_init_hw(map); From 69da5aa99ea67e86d3461fb281eadc952cc2914f Mon Sep 17 00:00:00 2001 From: William Breathitt Gray Date: Fri, 7 Apr 2023 07:47:33 -0400 Subject: [PATCH 02/23] regmap-irq: Drop map from handle_mask_sync() parameters Remove the map parameter from the struct regmap_irq_chip callback handle_mask_sync() because it can be passed via the irq_drv_data parameter instead. The gpio-104-dio-48e driver is the only consumer of this callback and is thus updated accordingly. Reviewed-by: Linus Walleij chip->num_regs; i++) { if (d->mask_base) { if (d->chip->handle_mask_sync) - d->chip->handle_mask_sync(d->map, i, - d->mask_buf_def[i], + d->chip->handle_mask_sync(i, d->mask_buf_def[i], d->mask_buf[i], d->chip->irq_drv_data); else { @@ -920,7 +919,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, if (d->mask_base) { if (chip->handle_mask_sync) { - ret = chip->handle_mask_sync(d->map, i, + ret = chip->handle_mask_sync(i, d->mask_buf_def[i], d->mask_buf[i], chip->irq_drv_data); diff --git a/drivers/gpio/gpio-104-dio-48e.c b/drivers/gpio/gpio-104-dio-48e.c index 63dce9532e975..8ff5f4ff5958b 100644 --- a/drivers/gpio/gpio-104-dio-48e.c +++ b/drivers/gpio/gpio-104-dio-48e.c @@ -110,7 +110,7 @@ struct dio48e_gpio { unsigned int irq_mask; }; -static int dio48e_handle_mask_sync(struct regmap *const map, const int index, +static int dio48e_handle_mask_sync(const int index, const unsigned int mask_buf_def, const unsigned int mask_buf, void *const irq_drv_data) diff --git a/include/linux/regmap.h b/include/linux/regmap.h index c2b9cc5db8241..f820bd44d16ff 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1658,8 +1658,7 @@ struct regmap_irq_chip { int (*handle_pre_irq)(void *irq_drv_data); int (*handle_post_irq)(void *irq_drv_data); - int (*handle_mask_sync)(struct regmap *map, int index, - unsigned int mask_buf_def, + int (*handle_mask_sync)(int index, unsigned int mask_buf_def, unsigned int mask_buf, void *irq_drv_data); int (*set_type_virt)(unsigned int **buf, unsigned int type, unsigned long hwirq, int reg); From 212bc1ce618dd7c734920a68988fe4b473f09f40 Mon Sep 17 00:00:00 2001 From: Aidan MacDonald Date: Tue, 9 May 2023 12:00:55 +0100 Subject: [PATCH 03/23] regmap-irq: Fix typo in documentation for .get_irq_reg() It refers to a non-existent "num_type_settings" value, which is an old name I'd used during development of config registers and later dropped because it wasn't very clear. The correct bound for the range is num_config_regs, which can be verified by checking the implementation. Signed-off-by: Aidan MacDonald Date: Tue, 9 May 2023 12:00:56 +0100 Subject: [PATCH 04/23] regmap-irq: Remove virtual registers No remaining users, and it's been replaced by config registers. Signed-off-by: Aidan MacDonald chip->num_virt_regs) { - for (i = 0; i < d->chip->num_virt_regs; i++) { - for (j = 0; j < d->chip->num_regs; j++) { - reg = d->get_irq_reg(d, d->chip->virt_reg_base[i], - j); - ret = regmap_write(map, reg, d->virt_buf[i][j]); - if (ret != 0) - dev_err(d->map->dev, - "Failed to write virt 0x%x: %d\n", - reg, ret); - } - } - } - for (i = 0; i < d->chip->num_config_bases; i++) { for (j = 0; j < d->chip->num_config_regs; j++) { reg = d->get_irq_reg(d, d->chip->config_base[i], j); @@ -320,13 +305,6 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type) return -EINVAL; } - if (d->chip->set_type_virt) { - ret = d->chip->set_type_virt(d->virt_buf, type, data->hwirq, - reg); - if (ret) - return ret; - } - if (d->chip->set_type_config) { ret = d->chip->set_type_config(d->config_buf, type, irq_data, reg, d->chip->irq_drv_data); @@ -758,9 +736,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, if (chip->num_type_reg) dev_warn(map->dev, "type registers are deprecated; use config registers instead"); - if (chip->num_virt_regs || chip->virt_reg_base || chip->set_type_virt) - dev_warn(map->dev, "virtual registers are deprecated; use config registers instead"); - if (irq_base) { irq_base = irq_alloc_descs(irq_base, 0, chip->num_irqs, 0); if (irq_base < 0) { @@ -824,24 +799,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, goto err_alloc; } - if (chip->num_virt_regs) { - /* - * Create virt_buf[chip->num_extra_config_regs][chip->num_regs] - */ - d->virt_buf = kcalloc(chip->num_virt_regs, sizeof(*d->virt_buf), - GFP_KERNEL); - if (!d->virt_buf) - goto err_alloc; - - for (i = 0; i < chip->num_virt_regs; i++) { - d->virt_buf[i] = kcalloc(chip->num_regs, - sizeof(**d->virt_buf), - GFP_KERNEL); - if (!d->virt_buf[i]) - goto err_alloc; - } - } - if (chip->num_config_bases && chip->num_config_regs) { /* * Create config_buf[num_config_bases][num_config_regs] @@ -1063,11 +1020,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, kfree(d->mask_buf); kfree(d->status_buf); kfree(d->status_reg_buf); - if (d->virt_buf) { - for (i = 0; i < chip->num_virt_regs; i++) - kfree(d->virt_buf[i]); - kfree(d->virt_buf); - } if (d->config_buf) { for (i = 0; i < chip->num_config_bases; i++) kfree(d->config_buf[i]); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 253f99fb282fe..2ad0e3d77b95e 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1544,8 +1544,6 @@ struct regmap_irq_chip_data; * @wake_base: Base address for wake enables. If zero unsupported. * @type_base: Base address for irq type. If zero unsupported. Deprecated, * use @config_base instead. - * @virt_reg_base: Base addresses for extra config regs. Deprecated, use - * @config_base instead. * @config_base: Base address for IRQ type config regs. If null unsupported. * @irq_reg_stride: Stride to use for chips where registers are not contiguous. * @init_ack_masked: Ack all masked interrupts once during initalization. @@ -1586,9 +1584,6 @@ struct regmap_irq_chip_data; * * @num_type_reg: Number of type registers. Deprecated, use config registers * instead. - * @num_virt_regs: Number of non-standard irq configuration registers. - * If zero unsupported. Deprecated, use config registers - * instead. * @num_config_bases: Number of config base registers. * @num_config_regs: Number of config registers for each config base register. * @@ -1598,9 +1593,6 @@ struct regmap_irq_chip_data; * after handling the interrupts in regmap_irq_handler(). * @handle_mask_sync: Callback used to handle IRQ mask syncs. The index will be * in the range [0, num_regs) - * @set_type_virt: Driver specific callback to extend regmap_irq_set_type() - * and configure virt regs. Deprecated, use @set_type_config - * callback and config registers instead. * @set_type_config: Callback used for configuring irq types. * @get_irq_reg: Callback for mapping (base register, index) pairs to register * addresses. The base register will be one of @status_base, @@ -1630,7 +1622,6 @@ struct regmap_irq_chip { unsigned int ack_base; unsigned int wake_base; unsigned int type_base; - unsigned int *virt_reg_base; const unsigned int *config_base; unsigned int irq_reg_stride; unsigned int init_ack_masked:1; @@ -1652,7 +1643,6 @@ struct regmap_irq_chip { int num_irqs; int num_type_reg; - int num_virt_regs; int num_config_bases; int num_config_regs; @@ -1660,8 +1650,6 @@ struct regmap_irq_chip { int (*handle_post_irq)(void *irq_drv_data); int (*handle_mask_sync)(int index, unsigned int mask_buf_def, unsigned int mask_buf, void *irq_drv_data); - int (*set_type_virt)(unsigned int **buf, unsigned int type, - unsigned long hwirq, int reg); int (*set_type_config)(unsigned int **buf, unsigned int type, const struct regmap_irq *irq_data, int idx, void *irq_drv_data); From f05cbadce7e409b38acdf21f0a05d4420afa1b11 Mon Sep 17 00:00:00 2001 From: Aidan MacDonald Date: Thu, 11 May 2023 10:13:39 +0100 Subject: [PATCH 05/23] regmap-irq: Remove type registers No remaining users, these have been replaced by config registers. Signed-off-by: Aidan MacDonald chip->type_in_mask) { - for (i = 0; i < d->chip->num_type_reg; i++) { - if (!d->type_buf_def[i]) - continue; - reg = d->get_irq_reg(d, d->chip->type_base, i); - ret = regmap_update_bits(d->map, reg, - d->type_buf_def[i], d->type_buf[i]); - if (ret != 0) - dev_err(d->map->dev, "Failed to sync type in %x\n", - reg); - } - } - for (i = 0; i < d->chip->num_config_bases; i++) { for (j = 0; j < d->chip->num_config_regs; j++) { reg = d->get_irq_reg(d, d->chip->config_base[i], j); @@ -273,36 +259,11 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type) reg = t->type_reg_offset / map->reg_stride; - if (t->type_reg_mask) - d->type_buf[reg] &= ~t->type_reg_mask; - else - d->type_buf[reg] &= ~(t->type_falling_val | - t->type_rising_val | - t->type_level_low_val | - t->type_level_high_val); - switch (type) { - case IRQ_TYPE_EDGE_FALLING: - d->type_buf[reg] |= t->type_falling_val; - break; - - case IRQ_TYPE_EDGE_RISING: - d->type_buf[reg] |= t->type_rising_val; - break; - - case IRQ_TYPE_EDGE_BOTH: - d->type_buf[reg] |= (t->type_falling_val | - t->type_rising_val); - break; - - case IRQ_TYPE_LEVEL_HIGH: - d->type_buf[reg] |= t->type_level_high_val; - break; - - case IRQ_TYPE_LEVEL_LOW: - d->type_buf[reg] |= t->type_level_low_val; - break; - default: - return -EINVAL; + if (d->chip->type_in_mask) { + ret = regmap_irq_set_type_config_simple(&d->type_buf, type, + irq_data, reg, d->chip->irq_drv_data); + if (ret) + return ret; } if (d->chip->set_type_config) { @@ -707,8 +668,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, struct regmap_irq_chip_data *d; int i; int ret = -ENOMEM; - int num_type_reg; - int num_regs; u32 reg; if (chip->num_regs <= 0) @@ -733,9 +692,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, return -EINVAL; } - if (chip->num_type_reg) - dev_warn(map->dev, "type registers are deprecated; use config registers instead"); - if (irq_base) { irq_base = irq_alloc_descs(irq_base, 0, chip->num_irqs, 0); if (irq_base < 0) { @@ -780,21 +736,13 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, goto err_alloc; } - /* - * Use num_config_regs if defined, otherwise fall back to num_type_reg - * to maintain backward compatibility. - */ - num_type_reg = chip->num_config_regs ? chip->num_config_regs - : chip->num_type_reg; - num_regs = chip->type_in_mask ? chip->num_regs : num_type_reg; - if (num_regs) { - d->type_buf_def = kcalloc(num_regs, + if (chip->type_in_mask) { + d->type_buf_def = kcalloc(chip->num_regs, sizeof(*d->type_buf_def), GFP_KERNEL); if (!d->type_buf_def) goto err_alloc; - d->type_buf = kcalloc(num_regs, sizeof(*d->type_buf), - GFP_KERNEL); + d->type_buf = kcalloc(chip->num_regs, sizeof(*d->type_buf), GFP_KERNEL); if (!d->type_buf) goto err_alloc; } @@ -970,20 +918,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, } } - if (chip->num_type_reg && !chip->type_in_mask) { - for (i = 0; i < chip->num_type_reg; ++i) { - reg = d->get_irq_reg(d, d->chip->type_base, i); - - ret = regmap_read(map, reg, &d->type_buf_def[i]); - - if (ret) { - dev_err(map->dev, "Failed to get type defaults at 0x%x: %d\n", - reg, ret); - goto err_alloc; - } - } - } - if (irq_base) d->domain = irq_domain_create_legacy(fwnode, chip->num_irqs, irq_base, 0, diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 2ad0e3d77b95e..0b4b9eca480d4 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1542,8 +1542,6 @@ struct regmap_irq_chip_data; * @ack_base: Base ack address. If zero then the chip is clear on read. * Using zero value is possible with @use_ack bit. * @wake_base: Base address for wake enables. If zero unsupported. - * @type_base: Base address for irq type. If zero unsupported. Deprecated, - * use @config_base instead. * @config_base: Base address for IRQ type config regs. If null unsupported. * @irq_reg_stride: Stride to use for chips where registers are not contiguous. * @init_ack_masked: Ack all masked interrupts once during initalization. @@ -1581,9 +1579,6 @@ struct regmap_irq_chip_data; * @irqs: Descriptors for individual IRQs. Interrupt numbers are * assigned based on the index in the array of the interrupt. * @num_irqs: Number of descriptors. - * - * @num_type_reg: Number of type registers. Deprecated, use config registers - * instead. * @num_config_bases: Number of config base registers. * @num_config_regs: Number of config registers for each config base register. * @@ -1621,7 +1616,6 @@ struct regmap_irq_chip { unsigned int unmask_base; unsigned int ack_base; unsigned int wake_base; - unsigned int type_base; const unsigned int *config_base; unsigned int irq_reg_stride; unsigned int init_ack_masked:1; @@ -1642,7 +1636,6 @@ struct regmap_irq_chip { const struct regmap_irq *irqs; int num_irqs; - int num_type_reg; int num_config_bases; int num_config_regs; From 72cc0f523babca3886421721aa662c7d352a6d32 Mon Sep 17 00:00:00 2001 From: Aidan MacDonald Date: Thu, 11 May 2023 10:13:40 +0100 Subject: [PATCH 06/23] regmap-irq: Remove support for not_fixed_stride No remaining users, use a custom .get_irq_reg() callback instead. Signed-off-by: Aidan MacDonald offset[i]; unsigned int index = offset / map->reg_stride; - if (chip->not_fixed_stride) - ret = regmap_read(map, - chip->status_base + offset, - &data->status_buf[b]); - else - ret = regmap_read(map, - chip->status_base + offset, - &data->status_buf[index]); - + ret = regmap_read(map, chip->status_base + offset, + &data->status_buf[index]); if (ret) break; } @@ -391,17 +384,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) * sake of simplicity. and add bulk reads only if needed */ for (i = 0; i < chip->num_main_regs; i++) { - /* - * For not_fixed_stride, don't use ->get_irq_reg(). - * It would produce an incorrect result. - */ - if (data->chip->not_fixed_stride) - reg = chip->main_status + - i * map->reg_stride * data->irq_reg_stride; - else - reg = data->get_irq_reg(data, - chip->main_status, i); - + reg = data->get_irq_reg(data, chip->main_status, i); ret = regmap_read(map, reg, &data->main_status_buf[i]); if (ret) { dev_err(map->dev, @@ -567,20 +550,8 @@ static const struct irq_domain_ops regmap_domain_ops = { unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data, unsigned int base, int index) { - const struct regmap_irq_chip *chip = data->chip; struct regmap *map = data->map; - /* - * FIXME: This is for backward compatibility and should be removed - * when not_fixed_stride is dropped (it's only used by qcom-pm8008). - */ - if (chip->not_fixed_stride && chip->sub_reg_offsets) { - struct regmap_irq_sub_irq_map *subreg; - - subreg = &chip->sub_reg_offsets[0]; - return base + subreg->offset[0]; - } - return base + index * map->reg_stride * data->irq_reg_stride; } EXPORT_SYMBOL_GPL(regmap_irq_get_irq_reg_linear); @@ -684,14 +655,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, return -EINVAL; } - if (chip->not_fixed_stride) { - dev_warn(map->dev, "not_fixed_stride is deprecated; use ->get_irq_reg() instead"); - - for (i = 0; i < chip->num_regs; i++) - if (chip->sub_reg_offsets[i].num_regs != 1) - return -EINVAL; - } - if (irq_base) { irq_base = irq_alloc_descs(irq_base, 0, chip->num_irqs, 0); if (irq_base < 0) { diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 0b4b9eca480d4..8fc0b3ebce446 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1528,9 +1528,6 @@ struct regmap_irq_chip_data; * status_base. Should contain num_regs arrays. * Can be provided for chips with more complex mapping than * 1.st bit to 1.st sub-reg, 2.nd bit to 2.nd sub-reg, ... - * When used with not_fixed_stride, each one-element array - * member contains offset calculated as address from each - * peripheral to first peripheral. * @num_main_regs: Number of 'main status' irq registers for chips which have * main_status set. * @@ -1567,11 +1564,6 @@ struct regmap_irq_chip_data; * registers before unmasking interrupts to clear any bits * set when they were masked. * @runtime_pm: Hold a runtime PM lock on the device when accessing it. - * @not_fixed_stride: Used when chip peripherals are not laid out with fixed - * stride. Must be used with sub_reg_offsets containing the - * offsets to each peripheral. Deprecated; the same thing - * can be accomplished with a @get_irq_reg callback, without - * the need for a @sub_reg_offsets table. * @no_status: No status register: all interrupts assumed generated by device. * * @num_regs: Number of registers in each control bank. @@ -1628,7 +1620,6 @@ struct regmap_irq_chip { unsigned int type_in_mask:1; unsigned int clear_on_unmask:1; unsigned int runtime_pm:1; - unsigned int not_fixed_stride:1; unsigned int no_status:1; int num_regs; From a240d23ee9dce2a9fe68d614f19a463d7029fb87 Mon Sep 17 00:00:00 2001 From: Aidan MacDonald Date: Thu, 11 May 2023 10:13:41 +0100 Subject: [PATCH 07/23] regmap-irq: Minor adjustments to .handle_mask_sync() If a .handle_mask_sync() callback is provided it supersedes all inbuilt handling of mask registers, and judging by the commit 69af4bcaa08d ("regmap-irq: Add handle_mask_sync() callback") it is intended to completely replace all default IRQ masking logic. The implementation has two minor inconsistencies, which can be fixed without breaking compatibility: (1) mask_base must be set to enable .handle_mask_sync(), even though mask_base is otherwise unused. This is easily fixed because mask_base is already optional. (2) Unmask registers aren't accounted for -- they are part of the default IRQ masking logic and are just a bit-inverted version of mask registers. It would be a bad idea to allow them to be used at the same time as .handle_mask_sync(), as the result would be confusing and unmaintainable, so make sure this can't happen. Signed-off-by: Aidan MacDonald chip->num_regs; i++) { - if (d->mask_base) { - if (d->chip->handle_mask_sync) - d->chip->handle_mask_sync(i, d->mask_buf_def[i], - d->mask_buf[i], - d->chip->irq_drv_data); - else { - reg = d->get_irq_reg(d, d->mask_base, i); - ret = regmap_update_bits(d->map, reg, - d->mask_buf_def[i], - d->mask_buf[i]); - if (ret) - dev_err(d->map->dev, "Failed to sync masks in %x\n", - reg); - } + if (d->chip->handle_mask_sync) + d->chip->handle_mask_sync(i, d->mask_buf_def[i], + d->mask_buf[i], + d->chip->irq_drv_data); + + if (d->mask_base && !d->chip->handle_mask_sync) { + reg = d->get_irq_reg(d, d->mask_base, i); + ret = regmap_update_bits(d->map, reg, + d->mask_buf_def[i], + d->mask_buf[i]); + if (ret) + dev_err(d->map->dev, "Failed to sync masks in %x\n", reg); } - if (d->unmask_base) { + if (d->unmask_base && !d->chip->handle_mask_sync) { reg = d->get_irq_reg(d, d->unmask_base, i); ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i], ~d->mask_buf[i]); @@ -785,28 +783,27 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, for (i = 0; i < chip->num_regs; i++) { d->mask_buf[i] = d->mask_buf_def[i]; - if (d->mask_base) { - if (chip->handle_mask_sync) { - ret = chip->handle_mask_sync(i, - d->mask_buf_def[i], - d->mask_buf[i], - chip->irq_drv_data); - if (ret) - goto err_alloc; - } else { - reg = d->get_irq_reg(d, d->mask_base, i); - ret = regmap_update_bits(d->map, reg, - d->mask_buf_def[i], - d->mask_buf[i]); - if (ret) { - dev_err(map->dev, "Failed to set masks in 0x%x: %d\n", - reg, ret); - goto err_alloc; - } + if (chip->handle_mask_sync) { + ret = chip->handle_mask_sync(i, d->mask_buf_def[i], + d->mask_buf[i], + chip->irq_drv_data); + if (ret) + goto err_alloc; + } + + if (d->mask_base && !chip->handle_mask_sync) { + reg = d->get_irq_reg(d, d->mask_base, i); + ret = regmap_update_bits(d->map, reg, + d->mask_buf_def[i], + d->mask_buf[i]); + if (ret) { + dev_err(map->dev, "Failed to set masks in 0x%x: %d\n", + reg, ret); + goto err_alloc; } } - if (d->unmask_base) { + if (d->unmask_base && !chip->handle_mask_sync) { reg = d->get_irq_reg(d, d->unmask_base, i); ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i], ~d->mask_buf[i]); From 0a3a56875500aaa5b0bc8f857ed46c8cd46d0775 Mon Sep 17 00:00:00 2001 From: Aidan MacDonald Date: Thu, 11 May 2023 10:13:42 +0100 Subject: [PATCH 08/23] regmap-irq: Drop backward compatibility for inverted mask/unmask All users must now specify .mask_unmask_non_inverted = true to ensure they are using the expected semantics: 1s disable IRQs in the mask registers, and enable IRQs in the unmask registers. Signed-off-by: Aidan MacDonald mask_buf[i], d->chip->irq_drv_data); - if (d->mask_base && !d->chip->handle_mask_sync) { - reg = d->get_irq_reg(d, d->mask_base, i); + if (d->chip->mask_base && !d->chip->handle_mask_sync) { + reg = d->get_irq_reg(d, d->chip->mask_base, i); ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i], d->mask_buf[i]); @@ -127,8 +124,8 @@ static void regmap_irq_sync_unlock(struct irq_data *data) dev_err(d->map->dev, "Failed to sync masks in %x\n", reg); } - if (d->unmask_base && !d->chip->handle_mask_sync) { - reg = d->get_irq_reg(d, d->unmask_base, i); + if (d->chip->unmask_base && !d->chip->handle_mask_sync) { + reg = d->get_irq_reg(d, d->chip->unmask_base, i); ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i], ~d->mask_buf[i]); if (ret) @@ -645,6 +642,9 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, if (chip->clear_on_unmask && (chip->ack_base || chip->use_ack)) return -EINVAL; + if (chip->mask_base && chip->unmask_base && !chip->mask_unmask_non_inverted) + return -EINVAL; + for (i = 0; i < chip->num_irqs; i++) { if (chip->irqs[i].reg_offset % map->reg_stride) return -EINVAL; @@ -733,28 +733,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, d->chip = chip; d->irq_base = irq_base; - if (chip->mask_base && chip->unmask_base && - !chip->mask_unmask_non_inverted) { - /* - * Chips that specify both mask_base and unmask_base used to - * get inverted mask behavior by default, with no way to ask - * for the normal, non-inverted behavior. This "inverted by - * default" behavior is deprecated, but we have to support it - * until existing drivers have been fixed. - * - * Existing drivers should be updated by swapping mask_base - * and unmask_base and setting mask_unmask_non_inverted=true. - * New drivers should always set the flag. - */ - dev_warn(map->dev, "mask_base and unmask_base are inverted, please fix it"); - - d->mask_base = chip->unmask_base; - d->unmask_base = chip->mask_base; - } else { - d->mask_base = chip->mask_base; - d->unmask_base = chip->unmask_base; - } - if (chip->irq_reg_stride) d->irq_reg_stride = chip->irq_reg_stride; else @@ -791,8 +769,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, goto err_alloc; } - if (d->mask_base && !chip->handle_mask_sync) { - reg = d->get_irq_reg(d, d->mask_base, i); + if (chip->mask_base && !chip->handle_mask_sync) { + reg = d->get_irq_reg(d, chip->mask_base, i); ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i], d->mask_buf[i]); @@ -803,8 +781,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, } } - if (d->unmask_base && !chip->handle_mask_sync) { - reg = d->get_irq_reg(d, d->unmask_base, i); + if (chip->unmask_base && !chip->handle_mask_sync) { + reg = d->get_irq_reg(d, chip->unmask_base, i); ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i], ~d->mask_buf[i]); if (ret) { From e12ff28764937dd58c8613f16065da60da149048 Mon Sep 17 00:00:00 2001 From: Maxime Chevallier Date: Thu, 11 May 2023 16:27:35 +0200 Subject: [PATCH 09/23] regmap: mmio: Allow passing an empty config->reg_stride Regmap's stride is used for MMIO regmaps to check the correctness of reg_width. However, it's acceptable to pass an empty config->reg_stride, in that case the actual stride used is 1. There are valid cases now to pass an empty stride, when using down/upshifting of register address. In this case, the stride value loses its sense, so ignore the reg_width when the stride isn't set. Signed-off-by: Maxime Chevallier reg_stride < min_stride) + if (config->reg_stride && config->reg_stride < min_stride) return ERR_PTR(-EINVAL); if (config->use_relaxed_mmio && config->io_port) From 3a48d2127f4dbd767d43bf8280b67d585e701f75 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Tue, 23 May 2023 22:22:27 +0100 Subject: [PATCH 10/23] regmap: Load register defaults in blocks rather than register by register Currently we use the normal single register write function to load the default values into the cache, resulting in a large number of reallocations when there are blocks of registers as we extend the memory region we are using to store the values. Instead scan through the list of defaults for blocks of adjacent registers and do a single allocation and insert for each such block. No functional change. We do not take advantage of the maple tree preallocation, this is purely at the regcache level. It is not clear to me yet if the maple tree level would help much here or if we'd have more overhead from overallocating and then freeing maple tree data. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230523-regcache-maple-load-defaults-v1-1-0c04336f005d@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regcache-maple.c | 58 +++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 6 deletions(-) diff --git a/drivers/base/regmap/regcache-maple.c b/drivers/base/regmap/regcache-maple.c index 9b1b559107ef4..2d2d5d7ee4473 100644 --- a/drivers/base/regmap/regcache-maple.c +++ b/drivers/base/regmap/regcache-maple.c @@ -239,11 +239,41 @@ static int regcache_maple_exit(struct regmap *map) return 0; } +static int regcache_maple_insert_block(struct regmap *map, int first, + int last) +{ + struct maple_tree *mt = map->cache; + MA_STATE(mas, mt, first, last); + unsigned long *entry; + int i, ret; + + entry = kcalloc(last - first + 1, sizeof(unsigned long), GFP_KERNEL); + if (!entry) + return -ENOMEM; + + for (i = 0; i < last - first + 1; i++) + entry[i] = map->reg_defaults[first + i].def; + + mas_lock(&mas); + + mas_set_range(&mas, map->reg_defaults[first].reg, + map->reg_defaults[last].reg); + ret = mas_store_gfp(&mas, entry, GFP_KERNEL); + + mas_unlock(&mas); + + if (ret) + kfree(entry); + + return ret; +} + static int regcache_maple_init(struct regmap *map) { struct maple_tree *mt; int i; int ret; + int range_start; mt = kmalloc(sizeof(*mt), GFP_KERNEL); if (!mt) @@ -252,14 +282,30 @@ static int regcache_maple_init(struct regmap *map) mt_init(mt); - for (i = 0; i < map->num_reg_defaults; i++) { - ret = regcache_maple_write(map, - map->reg_defaults[i].reg, - map->reg_defaults[i].def); - if (ret) - goto err; + if (!map->num_reg_defaults) + return 0; + + range_start = 0; + + /* Scan for ranges of contiguous registers */ + for (i = 1; i < map->num_reg_defaults; i++) { + if (map->reg_defaults[i].reg != + map->reg_defaults[i - 1].reg + 1) { + ret = regcache_maple_insert_block(map, range_start, + i - 1); + if (ret != 0) + goto err; + + range_start = i; + } } + /* Add the last block */ + ret = regcache_maple_insert_block(map, range_start, + map->num_reg_defaults - 1); + if (ret != 0) + goto err; + return 0; err: From 02534c8e967b51940ae7c0cd99befe216f1c2c8d Mon Sep 17 00:00:00 2001 From: Charles Keepax Date: Thu, 1 Jun 2023 11:10:35 +0100 Subject: [PATCH 11/23] regmap: regmap-irq: Move handle_post_irq to before pm_runtime_put Typically handle_post_irq is going to be used to manage some additional chip specific hardware operations required on each IRQ, these are very likely to want the chip to be resumed. For example the current in tree user max77620 uses this to toggle a global mask bit, which would obviously want the device resumed. It is worth noting this device does not specify the runtime_pm flag in regmap_irq_chip, so there is no actual issue. Move the callback to before the pm_runtime_put, so it will be called whilst the device is still resumed. Signed-off-by: Charles Keepax Link: https://lore.kernel.org/r/20230601101036.1499612-1-ckeepax@opensource.cirrus.com Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-irq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 330da5d6c8c3a..ced0dcf86e0bf 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -502,12 +502,12 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) } exit: - if (chip->runtime_pm) - pm_runtime_put(map->dev); - if (chip->handle_post_irq) chip->handle_post_irq(chip->irq_drv_data); + if (chip->runtime_pm) + pm_runtime_put(map->dev); + if (handled) return IRQ_HANDLED; else From 99e8dd39f34333d745e6c220be5d166e85214e6c Mon Sep 17 00:00:00 2001 From: Charles Keepax Date: Thu, 1 Jun 2023 11:10:36 +0100 Subject: [PATCH 12/23] regmap: Add missing cache_only checks The current behaviour around cache_only is slightly inconsistent, most paths will only check cache_only if cache_bypass is false, and will return -EBUSY if a read attempts to go to the hardware whilst cache_only is true. However, a couple of paths will not check cache_only at all. The most notable of these being regmap_raw_read which will check cache_only in the case it processes the transaction one register at a time, but not in the case it handles them as a block. In the typical case a device has been put into cache_only whilst powered down this can cause physical reads to happen whilst the device is unavailable. Add a check in regmap_raw_read and move the check in regmap_noinc_read, adding a check for cache_bypass, such that all paths are covered and consistent. Signed-off-by: Charles Keepax Link: https://lore.kernel.org/r/20230601101036.1499612-2-ckeepax@opensource.cirrus.com Signed-off-by: Mark Brown --- drivers/base/regmap/regmap.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index db7851f0e3b8c..f337c7930954e 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -2981,6 +2981,11 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val, size_t chunk_count, chunk_bytes; size_t chunk_regs = val_count; + if (!map->cache_bypass && map->cache_only) { + ret = -EBUSY; + goto out; + } + if (!map->read) { ret = -ENOTSUPP; goto out; @@ -3076,18 +3081,19 @@ int regmap_noinc_read(struct regmap *map, unsigned int reg, goto out_unlock; } + /* + * We have not defined the FIFO semantics for cache, as the + * cache is just one value deep. Should we return the last + * written value? Just avoid this by always reading the FIFO + * even when using cache. Cache only will not work. + */ + if (!map->cache_bypass && map->cache_only) { + ret = -EBUSY; + goto out_unlock; + } + /* Use the accelerated operation if we can */ if (map->bus->reg_noinc_read) { - /* - * We have not defined the FIFO semantics for cache, as the - * cache is just one value deep. Should we return the last - * written value? Just avoid this by always reading the FIFO - * even when using cache. Cache only will not work. - */ - if (map->cache_only) { - ret = -EBUSY; - goto out_unlock; - } ret = regmap_noinc_readwrite(map, reg, val, val_len, false); goto out_unlock; } From 65dd2f671875b1d97b6fa9bcf7677f5e1c55f776 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Sun, 11 Jun 2023 14:25:02 +0100 Subject: [PATCH 13/23] regmap: Provide a ram backed regmap with raw support Provide a simple, 16 bit only, RAM backed regmap which supports raw I/O for use in testing. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230610-regcache-raw-kunit-v1-1-583112cd28ac@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/Makefile | 2 +- drivers/base/regmap/internal.h | 8 ++ drivers/base/regmap/regmap-raw-ram.c | 133 +++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 drivers/base/regmap/regmap-raw-ram.c diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile index f6c6cb0172000..5fdd0845b45e5 100644 --- a/drivers/base/regmap/Makefile +++ b/drivers/base/regmap/Makefile @@ -8,7 +8,7 @@ obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o obj-$(CONFIG_REGMAP_KUNIT) += regmap-kunit.o obj-$(CONFIG_REGMAP_AC97) += regmap-ac97.o obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o -obj-$(CONFIG_REGMAP_RAM) += regmap-ram.o +obj-$(CONFIG_REGMAP_RAM) += regmap-ram.o regmap-raw-ram.o obj-$(CONFIG_REGMAP_SLIMBUS) += regmap-slimbus.o obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h index 9bd0dfd1e2596..d987ce182d220 100644 --- a/drivers/base/regmap/internal.h +++ b/drivers/base/regmap/internal.h @@ -312,6 +312,7 @@ struct regmap_ram_data { unsigned int *vals; /* Allocatd by caller */ bool *read; bool *written; + enum regmap_endian reg_endian; }; /* @@ -326,5 +327,12 @@ struct regmap *__regmap_init_ram(const struct regmap_config *config, #define regmap_init_ram(config, data) \ __regmap_lockdep_wrapper(__regmap_init_ram, #config, config, data) +struct regmap *__regmap_init_raw_ram(const struct regmap_config *config, + struct regmap_ram_data *data, + struct lock_class_key *lock_key, + const char *lock_name); + +#define regmap_init_raw_ram(config, data) \ + __regmap_lockdep_wrapper(__regmap_init_raw_ram, #config, config, data) #endif diff --git a/drivers/base/regmap/regmap-raw-ram.c b/drivers/base/regmap/regmap-raw-ram.c new file mode 100644 index 0000000000000..c9b800885f3ba --- /dev/null +++ b/drivers/base/regmap/regmap-raw-ram.c @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// Register map access API - Memory region with raw access +// +// This is intended for testing only +// +// Copyright (c) 2023, Arm Ltd + +#include +#include +#include +#include +#include +#include +#include + +#include "internal.h" + +static unsigned int decode_reg(enum regmap_endian endian, const void *reg) +{ + const u16 *r = reg; + + if (endian == REGMAP_ENDIAN_BIG) + return be16_to_cpu(*r); + else + return le16_to_cpu(*r); +} + +static int regmap_raw_ram_gather_write(void *context, + const void *reg, size_t reg_len, + const void *val, size_t val_len) +{ + struct regmap_ram_data *data = context; + unsigned int r; + u16 *our_buf = (u16 *)data->vals; + int i; + + if (reg_len != 2) + return -EINVAL; + if (val_len % 2) + return -EINVAL; + + r = decode_reg(data->reg_endian, reg); + memcpy(&our_buf[r], val, val_len); + + for (i = 0; i < val_len / 2; i++) + data->written[r + i] = true; + + return 0; +} + +static int regmap_raw_ram_write(void *context, const void *data, size_t count) +{ + return regmap_raw_ram_gather_write(context, data, 2, + data + 2, count - 2); +} + +static int regmap_raw_ram_read(void *context, + const void *reg, size_t reg_len, + void *val, size_t val_len) +{ + struct regmap_ram_data *data = context; + unsigned int r; + u16 *our_buf = (u16 *)data->vals; + int i; + + if (reg_len != 2) + return -EINVAL; + if (val_len % 2) + return -EINVAL; + + r = decode_reg(data->reg_endian, reg); + memcpy(val, &our_buf[r], val_len); + + for (i = 0; i < val_len / 2; i++) + data->read[r + i] = true; + + return 0; +} + +static void regmap_raw_ram_free_context(void *context) +{ + struct regmap_ram_data *data = context; + + kfree(data->vals); + kfree(data->read); + kfree(data->written); + kfree(data); +} + +static const struct regmap_bus regmap_raw_ram = { + .fast_io = true, + .write = regmap_raw_ram_write, + .gather_write = regmap_raw_ram_gather_write, + .read = regmap_raw_ram_read, + .free_context = regmap_raw_ram_free_context, +}; + +struct regmap *__regmap_init_raw_ram(const struct regmap_config *config, + struct regmap_ram_data *data, + struct lock_class_key *lock_key, + const char *lock_name) +{ + struct regmap *map; + + if (config->reg_bits != 16) + return ERR_PTR(-EINVAL); + + if (!config->max_register) { + pr_crit("No max_register specified for RAM regmap\n"); + return ERR_PTR(-EINVAL); + } + + data->read = kcalloc(sizeof(bool), config->max_register + 1, + GFP_KERNEL); + if (!data->read) + return ERR_PTR(-ENOMEM); + + data->written = kcalloc(sizeof(bool), config->max_register + 1, + GFP_KERNEL); + if (!data->written) + return ERR_PTR(-ENOMEM); + + data->reg_endian = config->reg_format_endian; + + map = __regmap_init(NULL, ®map_raw_ram, data, config, + lock_key, lock_name); + + return map; +} +EXPORT_SYMBOL_GPL(__regmap_init_raw_ram); + +MODULE_LICENSE("GPL v2"); From 155a6bd6375b584c8bdbf963b8ddef672ff9aca3 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Sun, 11 Jun 2023 14:25:03 +0100 Subject: [PATCH 14/23] regmap: Provide basic KUnit coverage for the raw register I/O Simple tests that cover basic raw I/O, plus basic coverage of cache sync since the caches generate bulk I/O with raw register maps. This could be more comprehensive but it is good for testing generic code. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230610-regcache-raw-kunit-v1-2-583112cd28ac@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-kunit.c | 327 +++++++++++++++++++++++++++++ 1 file changed, 327 insertions(+) diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c index f76d416881349..4ec5cbfc0ca04 100644 --- a/drivers/base/regmap/regmap-kunit.c +++ b/drivers/base/regmap/regmap-kunit.c @@ -712,6 +712,327 @@ static void cache_drop(struct kunit *test) regmap_exit(map); } +struct raw_test_types { + const char *name; + + enum regcache_type cache_type; + enum regmap_endian val_endian; +}; + +static void raw_to_desc(const struct raw_test_types *t, char *desc) +{ + strcpy(desc, t->name); +} + +static const struct raw_test_types raw_types_list[] = { + { "none-little", REGCACHE_NONE, REGMAP_ENDIAN_LITTLE }, + { "none-big", REGCACHE_NONE, REGMAP_ENDIAN_BIG }, + { "flat-little", REGCACHE_FLAT, REGMAP_ENDIAN_LITTLE }, + { "flat-big", REGCACHE_FLAT, REGMAP_ENDIAN_BIG }, + { "rbtree-little", REGCACHE_RBTREE, REGMAP_ENDIAN_LITTLE }, + { "rbtree-big", REGCACHE_RBTREE, REGMAP_ENDIAN_BIG }, + { "maple-little", REGCACHE_MAPLE, REGMAP_ENDIAN_LITTLE }, + { "maple-big", REGCACHE_MAPLE, REGMAP_ENDIAN_BIG }, +}; + +KUNIT_ARRAY_PARAM(raw_test_types, raw_types_list, raw_to_desc); + +static const struct raw_test_types raw_cache_types_list[] = { + { "flat-little", REGCACHE_FLAT, REGMAP_ENDIAN_LITTLE }, + { "flat-big", REGCACHE_FLAT, REGMAP_ENDIAN_BIG }, + { "rbtree-little", REGCACHE_RBTREE, REGMAP_ENDIAN_LITTLE }, + { "rbtree-big", REGCACHE_RBTREE, REGMAP_ENDIAN_BIG }, + { "maple-little", REGCACHE_MAPLE, REGMAP_ENDIAN_LITTLE }, + { "maple-big", REGCACHE_MAPLE, REGMAP_ENDIAN_BIG }, +}; + +KUNIT_ARRAY_PARAM(raw_test_cache_types, raw_cache_types_list, raw_to_desc); + +static const struct regmap_config raw_regmap_config = { + .max_register = BLOCK_TEST_SIZE, + + .reg_format_endian = REGMAP_ENDIAN_LITTLE, + .reg_bits = 16, + .val_bits = 16, +}; + +static struct regmap *gen_raw_regmap(struct regmap_config *config, + struct raw_test_types *test_type, + struct regmap_ram_data **data) +{ + u16 *buf; + struct regmap *ret; + size_t size = (config->max_register + 1) * config->reg_bits / 8; + int i; + struct reg_default *defaults; + + config->cache_type = test_type->cache_type; + config->val_format_endian = test_type->val_endian; + + buf = kmalloc(size, GFP_KERNEL); + if (!buf) + return ERR_PTR(-ENOMEM); + + get_random_bytes(buf, size); + + *data = kzalloc(sizeof(**data), GFP_KERNEL); + if (!(*data)) + return ERR_PTR(-ENOMEM); + (*data)->vals = (void *)buf; + + config->num_reg_defaults = config->max_register + 1; + defaults = kcalloc(config->num_reg_defaults, + sizeof(struct reg_default), + GFP_KERNEL); + if (!defaults) + return ERR_PTR(-ENOMEM); + config->reg_defaults = defaults; + + for (i = 0; i < config->num_reg_defaults; i++) { + defaults[i].reg = i; + switch (test_type->val_endian) { + case REGMAP_ENDIAN_LITTLE: + defaults[i].def = le16_to_cpu(buf[i]); + break; + case REGMAP_ENDIAN_BIG: + defaults[i].def = be16_to_cpu(buf[i]); + break; + default: + return ERR_PTR(-EINVAL); + } + } + + /* + * We use the defaults in the tests but they don't make sense + * to the core if there's no cache. + */ + if (config->cache_type == REGCACHE_NONE) + config->num_reg_defaults = 0; + + ret = regmap_init_raw_ram(config, *data); + if (IS_ERR(ret)) { + kfree(buf); + kfree(*data); + } + + return ret; +} + +static void raw_read_defaults_single(struct kunit *test) +{ + struct raw_test_types *t = (struct raw_test_types *)test->param_value; + struct regmap *map; + struct regmap_config config; + struct regmap_ram_data *data; + unsigned int rval; + int i; + + config = raw_regmap_config; + + map = gen_raw_regmap(&config, t, &data); + KUNIT_ASSERT_FALSE(test, IS_ERR(map)); + if (IS_ERR(map)) + return; + + /* Check that we can read the defaults via the API */ + for (i = 0; i < config.max_register + 1; i++) { + KUNIT_EXPECT_EQ(test, 0, regmap_read(map, i, &rval)); + KUNIT_EXPECT_EQ(test, config.reg_defaults[i].def, rval); + } + + regmap_exit(map); +} + +static void raw_read_defaults(struct kunit *test) +{ + struct raw_test_types *t = (struct raw_test_types *)test->param_value; + struct regmap *map; + struct regmap_config config; + struct regmap_ram_data *data; + u16 *rval; + u16 def; + size_t val_len; + int i; + + config = raw_regmap_config; + + map = gen_raw_regmap(&config, t, &data); + KUNIT_ASSERT_FALSE(test, IS_ERR(map)); + if (IS_ERR(map)) + return; + + val_len = sizeof(*rval) * (config.max_register + 1); + rval = kmalloc(val_len, GFP_KERNEL); + KUNIT_ASSERT_TRUE(test, rval != NULL); + if (!rval) + return; + + /* Check that we can read the defaults via the API */ + KUNIT_EXPECT_EQ(test, 0, regmap_raw_read(map, 0, rval, val_len)); + for (i = 0; i < config.max_register + 1; i++) { + def = config.reg_defaults[i].def; + if (config.val_format_endian == REGMAP_ENDIAN_BIG) { + KUNIT_EXPECT_EQ(test, def, be16_to_cpu(rval[i])); + } else { + KUNIT_EXPECT_EQ(test, def, le16_to_cpu(rval[i])); + } + } + + kfree(rval); + regmap_exit(map); +} + +static void raw_write_read_single(struct kunit *test) +{ + struct raw_test_types *t = (struct raw_test_types *)test->param_value; + struct regmap *map; + struct regmap_config config; + struct regmap_ram_data *data; + u16 val; + unsigned int rval; + + config = raw_regmap_config; + + map = gen_raw_regmap(&config, t, &data); + KUNIT_ASSERT_FALSE(test, IS_ERR(map)); + if (IS_ERR(map)) + return; + + get_random_bytes(&val, sizeof(val)); + + /* If we write a value to a register we can read it back */ + KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 0, val)); + KUNIT_EXPECT_EQ(test, 0, regmap_read(map, 0, &rval)); + KUNIT_EXPECT_EQ(test, val, rval); + + regmap_exit(map); +} + +static void raw_write(struct kunit *test) +{ + struct raw_test_types *t = (struct raw_test_types *)test->param_value; + struct regmap *map; + struct regmap_config config; + struct regmap_ram_data *data; + u16 *hw_buf; + u16 val[2]; + unsigned int rval; + int i; + + config = raw_regmap_config; + + map = gen_raw_regmap(&config, t, &data); + KUNIT_ASSERT_FALSE(test, IS_ERR(map)); + if (IS_ERR(map)) + return; + + hw_buf = (u16 *)data->vals; + + get_random_bytes(&val, sizeof(val)); + + /* Do a raw write */ + KUNIT_EXPECT_EQ(test, 0, regmap_raw_write(map, 2, val, sizeof(val))); + + /* We should read back the new values, and defaults for the rest */ + for (i = 0; i < config.max_register + 1; i++) { + KUNIT_EXPECT_EQ(test, 0, regmap_read(map, i, &rval)); + + switch (i) { + case 2: + case 3: + if (config.val_format_endian == REGMAP_ENDIAN_BIG) { + KUNIT_EXPECT_EQ(test, rval, + be16_to_cpu(val[i % 2])); + } else { + KUNIT_EXPECT_EQ(test, rval, + le16_to_cpu(val[i % 2])); + } + break; + default: + KUNIT_EXPECT_EQ(test, config.reg_defaults[i].def, rval); + break; + } + } + + /* The values should appear in the "hardware" */ + KUNIT_EXPECT_MEMEQ(test, &hw_buf[2], val, sizeof(val)); + + regmap_exit(map); +} + +static void raw_sync(struct kunit *test) +{ + struct raw_test_types *t = (struct raw_test_types *)test->param_value; + struct regmap *map; + struct regmap_config config; + struct regmap_ram_data *data; + u16 val[2]; + u16 *hw_buf; + unsigned int rval; + int i; + + config = raw_regmap_config; + + map = gen_raw_regmap(&config, t, &data); + KUNIT_ASSERT_FALSE(test, IS_ERR(map)); + if (IS_ERR(map)) + return; + + hw_buf = (u16 *)data->vals; + + get_random_bytes(&val, sizeof(val)); + + /* Do a regular write and a raw write in cache only mode */ + regcache_cache_only(map, true); + KUNIT_EXPECT_EQ(test, 0, regmap_raw_write(map, 2, val, sizeof(val))); + if (config.val_format_endian == REGMAP_ENDIAN_BIG) + KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 6, + be16_to_cpu(val[0]))); + else + KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 6, + le16_to_cpu(val[0]))); + + /* We should read back the new values, and defaults for the rest */ + for (i = 0; i < config.max_register + 1; i++) { + KUNIT_EXPECT_EQ(test, 0, regmap_read(map, i, &rval)); + + switch (i) { + case 2: + case 3: + case 6: + if (config.val_format_endian == REGMAP_ENDIAN_BIG) { + KUNIT_EXPECT_EQ(test, rval, + be16_to_cpu(val[i % 2])); + } else { + KUNIT_EXPECT_EQ(test, rval, + le16_to_cpu(val[i % 2])); + } + break; + default: + KUNIT_EXPECT_EQ(test, config.reg_defaults[i].def, rval); + break; + } + } + + /* The values should not appear in the "hardware" */ + KUNIT_EXPECT_MEMNEQ(test, &hw_buf[2], val, sizeof(val)); + KUNIT_EXPECT_MEMNEQ(test, &hw_buf[6], val, sizeof(u16)); + + for (i = 0; i < config.max_register + 1; i++) + data->written[i] = false; + + /* Do the sync */ + regcache_cache_only(map, false); + regcache_mark_dirty(map); + KUNIT_EXPECT_EQ(test, 0, regcache_sync(map)); + + /* The values should now appear in the "hardware" */ + KUNIT_EXPECT_MEMEQ(test, &hw_buf[2], val, sizeof(val)); + KUNIT_EXPECT_MEMEQ(test, &hw_buf[6], val, sizeof(u16)); + + regmap_exit(map); +} + static struct kunit_case regmap_test_cases[] = { KUNIT_CASE_PARAM(basic_read_write, regcache_types_gen_params), KUNIT_CASE_PARAM(bulk_write, regcache_types_gen_params), @@ -727,6 +1048,12 @@ static struct kunit_case regmap_test_cases[] = { KUNIT_CASE_PARAM(cache_sync_defaults, real_cache_types_gen_params), KUNIT_CASE_PARAM(cache_sync_patch, real_cache_types_gen_params), KUNIT_CASE_PARAM(cache_drop, sparse_cache_types_gen_params), + + KUNIT_CASE_PARAM(raw_read_defaults_single, raw_test_types_gen_params), + KUNIT_CASE_PARAM(raw_read_defaults, raw_test_types_gen_params), + KUNIT_CASE_PARAM(raw_write_read_single, raw_test_types_gen_params), + KUNIT_CASE_PARAM(raw_write, raw_test_types_gen_params), + KUNIT_CASE_PARAM(raw_sync, raw_test_cache_types_gen_params), {} }; From bfa0b38c148379c8a8c52e23bbdcb086414fb354 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Sun, 11 Jun 2023 13:06:07 +0100 Subject: [PATCH 15/23] regmap: maple: Implement block sync for the maple tree cache For register maps where we can write multiple values in a single bus operation it is generally much faster to do so. Improve the performance of maple tree cache syncs on such devices by identifying blocks of adjacent registers that need to be written out and combining them into a single operation. Combining writes does mean that we need to allocate a scratch buffer and format the data into it but it is expected that for most cases where caches are in use the cost of I/O will be much greater than the cost of doing the allocation and format. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230609-regcache-maple-sync-raw-v1-1-8ddeb4e2b9ab@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/internal.h | 2 + drivers/base/regmap/regcache-maple.c | 82 ++++++++++++++++++++++++++-- drivers/base/regmap/regcache.c | 4 +- 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h index 9bd0dfd1e2596..f993e2484f80b 100644 --- a/drivers/base/regmap/internal.h +++ b/drivers/base/regmap/internal.h @@ -257,6 +257,8 @@ int regcache_sync_block(struct regmap *map, void *block, unsigned long *cache_present, unsigned int block_base, unsigned int start, unsigned int end); +bool regcache_reg_needs_sync(struct regmap *map, unsigned int reg, + unsigned int val); static inline const void *regcache_get_val_addr(struct regmap *map, const void *base, diff --git a/drivers/base/regmap/regcache-maple.c b/drivers/base/regmap/regcache-maple.c index 14f6f49af097c..283c2e02a2985 100644 --- a/drivers/base/regmap/regcache-maple.c +++ b/drivers/base/regmap/regcache-maple.c @@ -186,6 +186,55 @@ static int regcache_maple_drop(struct regmap *map, unsigned int min, return ret; } +static int regcache_maple_sync_block(struct regmap *map, unsigned long *entry, + struct ma_state *mas, + unsigned int min, unsigned int max) +{ + void *buf; + unsigned long r; + size_t val_bytes = map->format.val_bytes; + int ret = 0; + + mas_pause(mas); + rcu_read_unlock(); + + /* + * Use a raw write if writing more than one register to a + * device that supports raw writes to reduce transaction + * overheads. + */ + if (max - min > 1 && regmap_can_raw_write(map)) { + buf = kmalloc(val_bytes * (max - min), map->alloc_flags); + if (!buf) { + ret = -ENOMEM; + goto out; + } + + /* Render the data for a raw write */ + for (r = min; r < max; r++) { + regcache_set_val(map, buf, r - min, + entry[r - mas->index]); + } + + ret = _regmap_raw_write(map, min, buf, (max - min) * val_bytes, + false); + + kfree(buf); + } else { + for (r = min; r < max; r++) { + ret = _regmap_write(map, r, + entry[r - mas->index]); + if (ret != 0) + goto out; + } + } + +out: + rcu_read_lock(); + + return ret; +} + static int regcache_maple_sync(struct regmap *map, unsigned int min, unsigned int max) { @@ -194,8 +243,9 @@ static int regcache_maple_sync(struct regmap *map, unsigned int min, MA_STATE(mas, mt, min, max); unsigned long lmin = min; unsigned long lmax = max; - unsigned int r; + unsigned int r, v, sync_start; int ret; + bool sync_needed = false; map->cache_bypass = true; @@ -203,18 +253,38 @@ static int regcache_maple_sync(struct regmap *map, unsigned int min, mas_for_each(&mas, entry, max) { for (r = max(mas.index, lmin); r <= min(mas.last, lmax); r++) { - mas_pause(&mas); - rcu_read_unlock(); - ret = regcache_sync_val(map, r, entry[r - mas.index]); + v = entry[r - mas.index]; + + if (regcache_reg_needs_sync(map, r, v)) { + if (!sync_needed) { + sync_start = r; + sync_needed = true; + } + continue; + } + + if (!sync_needed) + continue; + + ret = regcache_maple_sync_block(map, entry, &mas, + sync_start, r); if (ret != 0) goto out; - rcu_read_lock(); + sync_needed = false; + } + + if (sync_needed) { + ret = regcache_maple_sync_block(map, entry, &mas, + sync_start, r); + if (ret != 0) + goto out; + sync_needed = false; } } +out: rcu_read_unlock(); -out: map->cache_bypass = false; return ret; diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c index 029564695dbbb..c7d065f96a878 100644 --- a/drivers/base/regmap/regcache.c +++ b/drivers/base/regmap/regcache.c @@ -279,8 +279,8 @@ int regcache_write(struct regmap *map, return 0; } -static bool regcache_reg_needs_sync(struct regmap *map, unsigned int reg, - unsigned int val) +bool regcache_reg_needs_sync(struct regmap *map, unsigned int reg, + unsigned int val) { int ret; From d32758acbd4eee8ce95b705a6760526b4d26c2aa Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Sat, 10 Jun 2023 15:05:54 +0100 Subject: [PATCH 16/23] regmap: Don't check for changes in regcache_set_val() The only user of regcache_set_val() ignores the return value so we may as well not bother checking if the value we are trying to set is the same as the value already stored. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230609-regcache-set-val-no-ret-v1-1-9a6932760cf8@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/internal.h | 2 +- drivers/base/regmap/regcache.c | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h index f993e2484f80b..00848303b193f 100644 --- a/drivers/base/regmap/internal.h +++ b/drivers/base/regmap/internal.h @@ -269,7 +269,7 @@ static inline const void *regcache_get_val_addr(struct regmap *map, unsigned int regcache_get_val(struct regmap *map, const void *base, unsigned int idx); -bool regcache_set_val(struct regmap *map, void *base, unsigned int idx, +void regcache_set_val(struct regmap *map, void *base, unsigned int idx, unsigned int val); int regcache_lookup_reg(struct regmap *map, unsigned int reg); int regcache_sync_val(struct regmap *map, unsigned int reg, unsigned int val); diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c index c7d065f96a878..1f52646159df7 100644 --- a/drivers/base/regmap/regcache.c +++ b/drivers/base/regmap/regcache.c @@ -558,17 +558,14 @@ void regcache_cache_bypass(struct regmap *map, bool enable) } EXPORT_SYMBOL_GPL(regcache_cache_bypass); -bool regcache_set_val(struct regmap *map, void *base, unsigned int idx, +void regcache_set_val(struct regmap *map, void *base, unsigned int idx, unsigned int val) { - if (regcache_get_val(map, base, idx) == val) - return true; - /* Use device native format if possible */ if (map->format.format_val) { map->format.format_val(base + (map->cache_word_size * idx), val, 0); - return false; + return; } switch (map->cache_word_size) { @@ -601,7 +598,6 @@ bool regcache_set_val(struct regmap *map, void *base, unsigned int idx, default: BUG(); } - return false; } unsigned int regcache_get_val(struct regmap *map, const void *base, From b629c698eae745eb51b6d92395e2eee44bbf5f49 Mon Sep 17 00:00:00 2001 From: Waqar Hameed Date: Tue, 13 Jun 2023 13:42:27 +0200 Subject: [PATCH 17/23] regmap: Add debugfs file for forcing field writes `_regmap_update_bits()` checks if the current register value differs from the new value, and only writes to the register if they differ. When testing hardware drivers, it might be desirable to always force a register write, for example when writing to a `regmap_field`. This enables and simplifies testing and verification of the hardware interaction. For example, when using a hardware mock/simulation model, one can then more easily verify that the driver makes the correct expected register writes during certain events. Add a bool variable `force_write_field` and a corresponding debugfs entry to enable this. Since this feature could interfere with driver operation, guard it with a macro. Signed-off-by: Waqar Hameed Link: https://lore.kernel.org/r/pnd1qifa7sj.fsf@axis.com Signed-off-by: Mark Brown --- drivers/base/regmap/internal.h | 3 +++ drivers/base/regmap/regmap-debugfs.c | 11 +++++++++++ drivers/base/regmap/regmap.c | 2 +- 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h index 7211babbe4ee7..9a9ea514c2d81 100644 --- a/drivers/base/regmap/internal.h +++ b/drivers/base/regmap/internal.h @@ -125,6 +125,9 @@ struct regmap { int reg_stride; int reg_stride_order; + /* If set, will always write field to HW. */ + bool force_write_field; + /* regcache specific members */ const struct regcache_ops *cache_ops; enum regcache_type cache_type; diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c index c491fabe3617a..f36027591e1a8 100644 --- a/drivers/base/regmap/regmap-debugfs.c +++ b/drivers/base/regmap/regmap-debugfs.c @@ -636,6 +636,17 @@ void regmap_debugfs_init(struct regmap *map) ®map_cache_bypass_fops); } + /* + * This could interfere with driver operation. Therefore, don't provide + * any real compile time configuration option for this feature. One will + * have to modify the source code directly in order to use it. + */ +#undef REGMAP_ALLOW_FORCE_WRITE_FIELD_DEBUGFS +#ifdef REGMAP_ALLOW_FORCE_WRITE_FIELD_DEBUGFS + debugfs_create_bool("force_write_field", 0600, map->debugfs, + &map->force_write_field); +#endif + next = rb_first(&map->range_tree); while (next) { range_node = rb_entry(next, struct regmap_range_node, node); diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 627a767fa0470..89a7f1c459c1a 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -3279,7 +3279,7 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg, tmp = orig & ~mask; tmp |= val & mask; - if (force_write || (tmp != orig)) { + if (force_write || (tmp != orig) || map->force_write_field) { ret = _regmap_write(map, reg, tmp); if (ret == 0 && change) *change = true; From 180033061e203a7c5510e7c38bccd885657de517 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Tue, 13 Jun 2023 21:17:42 +0100 Subject: [PATCH 18/23] regmap: Add test that writes to write only registers are prevented We should have error checking that verifies that writes to write only registers are suppressed, verify that this happens as it should. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230613-regmap-kunit-read-write-v1-1-2db337c52827@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-kunit.c | 41 ++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c index 4ec5cbfc0ca04..f5c175022a477 100644 --- a/drivers/base/regmap/regmap-kunit.c +++ b/drivers/base/regmap/regmap-kunit.c @@ -92,6 +92,11 @@ static struct regmap *gen_regmap(struct regmap_config *config, return ret; } +static bool reg_5_false(struct device *context, unsigned int reg) +{ + return reg != 5; +} + static void basic_read_write(struct kunit *test) { struct regcache_types *t = (struct regcache_types *)test->param_value; @@ -191,6 +196,41 @@ static void bulk_read(struct kunit *test) regmap_exit(map); } +static void write_readonly(struct kunit *test) +{ + struct regcache_types *t = (struct regcache_types *)test->param_value; + struct regmap *map; + struct regmap_config config; + struct regmap_ram_data *data; + unsigned int val; + int i; + + config = test_regmap_config; + config.cache_type = t->type; + config.num_reg_defaults = BLOCK_TEST_SIZE; + config.writeable_reg = reg_5_false; + + map = gen_regmap(&config, &data); + KUNIT_ASSERT_FALSE(test, IS_ERR(map)); + if (IS_ERR(map)) + return; + + get_random_bytes(&val, sizeof(val)); + + for (i = 0; i < BLOCK_TEST_SIZE; i++) + data->written[i] = false; + + /* Change the value of all registers, readonly should fail */ + for (i = 0; i < BLOCK_TEST_SIZE; i++) + KUNIT_EXPECT_EQ(test, i != 5, regmap_write(map, i, val) == 0); + + /* Did that match what we see on the device? */ + for (i = 0; i < BLOCK_TEST_SIZE; i++) + KUNIT_EXPECT_EQ(test, i != 5, data->written[i]); + + regmap_exit(map); +} + static void reg_defaults(struct kunit *test) { struct regcache_types *t = (struct regcache_types *)test->param_value; @@ -1037,6 +1077,7 @@ static struct kunit_case regmap_test_cases[] = { KUNIT_CASE_PARAM(basic_read_write, regcache_types_gen_params), KUNIT_CASE_PARAM(bulk_write, regcache_types_gen_params), KUNIT_CASE_PARAM(bulk_read, regcache_types_gen_params), + KUNIT_CASE_PARAM(write_readonly, regcache_types_gen_params), KUNIT_CASE_PARAM(reg_defaults, regcache_types_gen_params), KUNIT_CASE_PARAM(reg_defaults_read_dev, regcache_types_gen_params), KUNIT_CASE_PARAM(register_patch, regcache_types_gen_params), From a07bff4054c9e2b3a5c5790312a4a45aaeca7afe Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Tue, 13 Jun 2023 21:17:43 +0100 Subject: [PATCH 19/23] regmap: Add a test case for write only registers Validate that attempts to read from write only registers fail and don't somehow trigger spurious hardware accesses. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230613-regmap-kunit-read-write-v1-2-2db337c52827@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-kunit.c | 32 ++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c index f5c175022a477..a1a1a9ce2e13b 100644 --- a/drivers/base/regmap/regmap-kunit.c +++ b/drivers/base/regmap/regmap-kunit.c @@ -231,6 +231,37 @@ static void write_readonly(struct kunit *test) regmap_exit(map); } +static void read_writeonly(struct kunit *test) +{ + struct regcache_types *t = (struct regcache_types *)test->param_value; + struct regmap *map; + struct regmap_config config; + struct regmap_ram_data *data; + unsigned int val; + int i; + + config = test_regmap_config; + config.cache_type = t->type; + config.readable_reg = reg_5_false; + + map = gen_regmap(&config, &data); + KUNIT_ASSERT_FALSE(test, IS_ERR(map)); + if (IS_ERR(map)) + return; + + for (i = 0; i < BLOCK_TEST_SIZE; i++) + data->read[i] = false; + + /* Try to read all the registers, the writeonly one should fail */ + for (i = 0; i < BLOCK_TEST_SIZE; i++) + KUNIT_EXPECT_EQ(test, i != 5, regmap_read(map, i, &val) == 0); + + /* Did we trigger a hardware access? */ + KUNIT_EXPECT_FALSE(test, data->read[5]); + + regmap_exit(map); +} + static void reg_defaults(struct kunit *test) { struct regcache_types *t = (struct regcache_types *)test->param_value; @@ -1078,6 +1109,7 @@ static struct kunit_case regmap_test_cases[] = { KUNIT_CASE_PARAM(bulk_write, regcache_types_gen_params), KUNIT_CASE_PARAM(bulk_read, regcache_types_gen_params), KUNIT_CASE_PARAM(write_readonly, regcache_types_gen_params), + KUNIT_CASE_PARAM(read_writeonly, regcache_types_gen_params), KUNIT_CASE_PARAM(reg_defaults, regcache_types_gen_params), KUNIT_CASE_PARAM(reg_defaults_read_dev, regcache_types_gen_params), KUNIT_CASE_PARAM(register_patch, regcache_types_gen_params), From 357a1ebd0c012f5421252547ebf4ee619b45733d Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Tue, 13 Jun 2023 21:17:44 +0100 Subject: [PATCH 20/23] regmap: Add test to make sure we don't sync to read only registers Ensure that a read only value in the register cache does not result in a write during regcache_sync(). Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230613-regmap-kunit-read-write-v1-3-2db337c52827@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-kunit.c | 42 ++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c index a1a1a9ce2e13b..6f444ac0ec490 100644 --- a/drivers/base/regmap/regmap-kunit.c +++ b/drivers/base/regmap/regmap-kunit.c @@ -680,6 +680,47 @@ static void cache_sync_defaults(struct kunit *test) regmap_exit(map); } +static void cache_sync_readonly(struct kunit *test) +{ + struct regcache_types *t = (struct regcache_types *)test->param_value; + struct regmap *map; + struct regmap_config config; + struct regmap_ram_data *data; + unsigned int val; + int i; + + config = test_regmap_config; + config.cache_type = t->type; + config.writeable_reg = reg_5_false; + + map = gen_regmap(&config, &data); + KUNIT_ASSERT_FALSE(test, IS_ERR(map)); + if (IS_ERR(map)) + return; + + /* Read all registers to fill the cache */ + for (i = 0; i < BLOCK_TEST_SIZE; i++) + KUNIT_EXPECT_EQ(test, 0, regmap_read(map, i, &val)); + + /* Change the value of all registers, readonly should fail */ + get_random_bytes(&val, sizeof(val)); + regcache_cache_only(map, true); + for (i = 0; i < BLOCK_TEST_SIZE; i++) + KUNIT_EXPECT_EQ(test, i != 5, regmap_write(map, i, val) == 0); + regcache_cache_only(map, false); + + /* Resync */ + for (i = 0; i < BLOCK_TEST_SIZE; i++) + data->written[i] = false; + KUNIT_EXPECT_EQ(test, 0, regcache_sync(map)); + + /* Did that match what we see on the device? */ + for (i = 0; i < BLOCK_TEST_SIZE; i++) + KUNIT_EXPECT_EQ(test, i != 5, data->written[i]); + + regmap_exit(map); +} + static void cache_sync_patch(struct kunit *test) { struct regcache_types *t = (struct regcache_types *)test->param_value; @@ -1119,6 +1160,7 @@ static struct kunit_case regmap_test_cases[] = { KUNIT_CASE_PARAM(cache_bypass, real_cache_types_gen_params), KUNIT_CASE_PARAM(cache_sync, real_cache_types_gen_params), KUNIT_CASE_PARAM(cache_sync_defaults, real_cache_types_gen_params), + KUNIT_CASE_PARAM(cache_sync_readonly, real_cache_types_gen_params), KUNIT_CASE_PARAM(cache_sync_patch, real_cache_types_gen_params), KUNIT_CASE_PARAM(cache_drop, sparse_cache_types_gen_params), From eab5abdeb79f0f694c007c3a76a97902705c86f0 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Tue, 13 Jun 2023 21:07:16 +0100 Subject: [PATCH 21/23] regmap: Check for register readability before checking cache during read Ensure that we don't return a spurious cache hit for unreadable registers (eg, with the flat cache which doesn't understand sparseness) by checking for readability before we do a cache lookup. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230613-b4-regmap-check-readability-before-cache-v1-1-b144c0b01ed9@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regmap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 89a7f1c459c1a..fad66b309ef98 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -2897,6 +2897,9 @@ static int _regmap_read(struct regmap *map, unsigned int reg, int ret; void *context = _regmap_map_get_context(map); + if (!regmap_readable(map, reg)) + return -EIO; + if (!map->cache_bypass) { ret = regcache_read(map, reg, val); if (ret == 0) @@ -2906,9 +2909,6 @@ static int _regmap_read(struct regmap *map, unsigned int reg, if (map->cache_only) return -EBUSY; - if (!regmap_readable(map, reg)) - return -EIO; - ret = map->reg_read(context, reg, val); if (ret == 0) { if (regmap_should_log(map)) From 3e47b8877d6c0f60943b00f3112756ca3b572cd6 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Fri, 16 Jun 2023 00:04:40 +0100 Subject: [PATCH 22/23] regmap: Drop early readability check We have some drivers that have a use case for cached write only registers, doing read/modify/writes on read only registers in order to work more easily with bitfields. Go back to trying the cache before we check if we can read from the device. Fixes: eab5abdeb79f0 ("regmap: Check for register readability before checking cache during read") Reported-by: Konrad Dybcio Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230615-regmap-drop-early-readability-v1-1-8135094362de@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regmap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index fad66b309ef98..89a7f1c459c1a 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -2897,9 +2897,6 @@ static int _regmap_read(struct regmap *map, unsigned int reg, int ret; void *context = _regmap_map_get_context(map); - if (!regmap_readable(map, reg)) - return -EIO; - if (!map->cache_bypass) { ret = regcache_read(map, reg, val); if (ret == 0) @@ -2909,6 +2906,9 @@ static int _regmap_read(struct regmap *map, unsigned int reg, if (map->cache_only) return -EBUSY; + if (!regmap_readable(map, reg)) + return -EIO; + ret = map->reg_read(context, reg, val); if (ret == 0) { if (regmap_should_log(map)) From d0c99ffe212679b338d12fe283964e6e43ce1501 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Sat, 17 Jun 2023 21:11:07 +0100 Subject: [PATCH 23/23] regmap: Allow reads from write only registers with the flat cache The flat cache is intended for devices that need the lowest overhead so doesn't track any sparseness. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20230617-regmap-kunit-read-writeonly-flat-v1-1-efd3ed66dec6@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-kunit.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c index 6f444ac0ec490..24257aa9004db 100644 --- a/drivers/base/regmap/regmap-kunit.c +++ b/drivers/base/regmap/regmap-kunit.c @@ -252,9 +252,18 @@ static void read_writeonly(struct kunit *test) for (i = 0; i < BLOCK_TEST_SIZE; i++) data->read[i] = false; - /* Try to read all the registers, the writeonly one should fail */ - for (i = 0; i < BLOCK_TEST_SIZE; i++) - KUNIT_EXPECT_EQ(test, i != 5, regmap_read(map, i, &val) == 0); + /* + * Try to read all the registers, the writeonly one should + * fail if we aren't using the flat cache. + */ + for (i = 0; i < BLOCK_TEST_SIZE; i++) { + if (t->type != REGCACHE_FLAT) { + KUNIT_EXPECT_EQ(test, i != 5, + regmap_read(map, i, &val) == 0); + } else { + KUNIT_EXPECT_EQ(test, 0, regmap_read(map, i, &val)); + } + } /* Did we trigger a hardware access? */ KUNIT_EXPECT_FALSE(test, data->read[5]);