Messages in this thread Patch in this message | | | Date | Wed, 16 Nov 2022 09:00:27 -0800 (PST) | From | matthew.gerlach@linux ... | Subject | Re: [PATCH 08/12] regmap: indirect: Add indirect regmap support |
| |
On Mon, 14 Nov 2022, Marco Pagani wrote:
> > On 2022-11-08 15:43, Ilpo Järvinen wrote: >> Add support for indirect register access via a regmap interface. >> >> Indirect register access is a generic way to access registers directly. >> One use case is accessing registers on Intel FPGA IPs with e.g. PMCI or >> HSSI. >> >> Co-developed-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> --- >> drivers/base/regmap/Kconfig | 3 + >> drivers/base/regmap/Makefile | 1 + >> drivers/base/regmap/regmap-indirect.c | 128 ++++++++++++++++++++++++++ >> include/linux/regmap.h | 55 +++++++++++ >> 4 files changed, 187 insertions(+) >> create mode 100644 drivers/base/regmap/regmap-indirect.c >> >> diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig >> index 159bac6c5046..94e5ca5434cf 100644 >> --- a/drivers/base/regmap/Kconfig >> +++ b/drivers/base/regmap/Kconfig >> @@ -65,3 +65,6 @@ config REGMAP_I3C >> config REGMAP_SPI_AVMM >> tristate >> depends on SPI >> + >> +config REGMAP_INDIRECT >> + tristate >> diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile >> index 11facb32a027..6221a4740806 100644 >> --- a/drivers/base/regmap/Makefile >> +++ b/drivers/base/regmap/Makefile >> @@ -20,3 +20,4 @@ obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o >> obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o >> obj-$(CONFIG_REGMAP_SPI_AVMM) += regmap-spi-avmm.o >> obj-$(CONFIG_REGMAP_MDIO) += regmap-mdio.o >> +obj-$(CONFIG_REGMAP_INDIRECT) += regmap-indirect.o >> diff --git a/drivers/base/regmap/regmap-indirect.c b/drivers/base/regmap/regmap-indirect.c >> new file mode 100644 >> index 000000000000..3ceb0c044c7c >> --- /dev/null >> +++ b/drivers/base/regmap/regmap-indirect.c >> @@ -0,0 +1,128 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Indirect Register Access. >> + * >> + * Copyright (C) 2020-2022 Intel Corporation, Inc. >> + */ >> +#include <linux/debugfs.h> >> +#include <linux/device.h> >> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/regmap.h> >> +#include <linux/seq_file.h> >> +#include <linux/slab.h> >> + >> +struct indirect_ctx { >> + void __iomem *base; >> + struct device *dev; >> + const struct regmap_indirect_cfg *indirect_cfg; >> +}; >> + >> +static int indirect_bus_idle_cmd(struct indirect_ctx *ctx) >> +{ >> + unsigned int cmd; >> + int ret; >> + >> + writel(ctx->indirect_cfg->idle_cmd, ctx->base + ctx->indirect_cfg->cmd_offset); >> + >> + ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->cmd_offset, cmd, >> + cmd == ctx->indirect_cfg->idle_cmd, >> + ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us); >> + if (ret) >> + dev_err(ctx->dev, "timed out waiting idle cmd (residual cmd=0x%x)\n", cmd); >> + >> + return ret; >> +} >> + >> +static int indirect_bus_reg_read(void *context, unsigned int reg, >> + unsigned int *val) >> +{ >> + struct indirect_ctx *ctx = context; >> + unsigned int cmd, ack, tmpval; >> + int ret; >> + >> + cmd = readl(ctx->base + ctx->indirect_cfg->cmd_offset); >> + if (cmd != ctx->indirect_cfg->idle_cmd) >> + dev_warn(ctx->dev, "residual cmd 0x%x on read entry\n", cmd); >> + >> + writel(reg, ctx->base + ctx->indirect_cfg->addr_offset); >> + writel(ctx->indirect_cfg->read_cmd, ctx->base + ctx->indirect_cfg->cmd_offset); >> + >> + ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->ack_offset, ack, >> + (ack & ctx->indirect_cfg->ack_mask) == ctx->indirect_cfg->ack_mask, >> + ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us); >> + if (ret) { >> + dev_err(ctx->dev, "read timed out on reg 0x%x ack 0x%x\n", reg, ack); >> + goto out;
This goto needs to be removed. The code needs to fall throuch and call indirect_bus_idle_cmd().
>> + } >> + >> + tmpval = readl(ctx->base + ctx->indirect_cfg->read_offset); >> + >> + if (indirect_bus_idle_cmd(ctx)) { >> + if (!ret) > > Could you please explain why it is necessary to insert an idle_cmd after each > bus read or write? Do the bus read and write methods assume there will be an > idle_cmd in the cmd register at initialization? Is this assumption safe if the > device detaches and reattaches? Isn't the "if (!ret)" check redundant since > ret is always == 0?
The idle_cmd ensures the RD or WR bits and the ACK bits are cleared before the next transaction. The if (!ret) doesn't override the error returned by readl_poll_timeout.
> >> + ret = -ETIMEDOUT; >> + goto out; >> + } >> + >> + *val = tmpval; >> +out: >> + return ret; >> +} >> + >> +static int indirect_bus_reg_write(void *context, unsigned int reg, >> + unsigned int val) >> +{ >> + struct indirect_ctx *ctx = context; >> + unsigned int cmd, ack; >> + int ret; >> + >> + cmd = readl(ctx->base + ctx->indirect_cfg->cmd_offset); >> + if (cmd != ctx->indirect_cfg->idle_cmd) >> + dev_warn(ctx->dev, "residual cmd 0x%x on write entry\n", cmd); >> + >> + writel(val, ctx->base + ctx->indirect_cfg->write_offset); >> + writel(reg, ctx->base + ctx->indirect_cfg->addr_offset); >> + writel(ctx->indirect_cfg->write_cmd, ctx->base + ctx->indirect_cfg->cmd_offset); >> + >> + ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->ack_offset, ack, >> + (ack & ctx->indirect_cfg->ack_mask) == ctx->indirect_cfg->ack_mask, >> + ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us); >> + if (ret) >> + dev_err(ctx->dev, "write timed out on reg 0x%x ack 0x%x\n", reg, ack); >> + >> + if (indirect_bus_idle_cmd(ctx)) { >> + if (!ret) >> + ret = -ETIMEDOUT; >> + } >> + >> + return ret; >> +} >> + >> +static const struct regmap_bus indirect_bus = { >> + .reg_write = indirect_bus_reg_write, >> + .reg_read = indirect_bus_reg_read, >> +}; >> + >> +struct regmap *__devm_regmap_init_indirect(struct device *dev, >> + void __iomem *base, >> + struct regmap_config *cfg, >> + struct lock_class_key *lock_key, >> + const char *lock_name) >> +{ >> + struct indirect_ctx *ctx; >> + >> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >> + if (!ctx) >> + return NULL; >> + >> + ctx->base = base; >> + ctx->dev = dev; >> + ctx->indirect_cfg = cfg->indirect_cfg; >> + >> + return __devm_regmap_init(dev, &indirect_bus, ctx, cfg, lock_key, lock_name); >> +} >> +EXPORT_SYMBOL_GPL(__devm_regmap_init_indirect); >> + >> +MODULE_DESCRIPTION("Indirect Register Access"); >> +MODULE_AUTHOR("Intel Corporation"); >> +MODULE_LICENSE("GPL"); >> diff --git a/include/linux/regmap.h b/include/linux/regmap.h >> index ca3434dca3a0..adaa7bca4f60 100644 >> --- a/include/linux/regmap.h >> +++ b/include/linux/regmap.h >> @@ -190,6 +190,41 @@ enum regmap_endian { >> REGMAP_ENDIAN_NATIVE, >> }; >> >> +/** >> + * struct regmap_indirect_cfg - A configuration for indirect register access >> + * >> + * @cmd_offset: Command register offset >> + * @idle_cmd: Idle command >> + * @read_cmd: Read command >> + * @write_cmd: Write command >> + * >> + * @ack_offset: Command acknowledgment register offset >> + * @ack_mask: Command acknowledgment bit mask >> + * >> + * @addr_offset: Address register offset >> + * @read_offset: Read register offset >> + * @write_offset: Write register offset >> + * >> + * @sleep_us: Command wait sleep (usecs) >> + * @timeout_us: Command timeout (usecs) >> + */ >> +struct regmap_indirect_cfg { >> + unsigned int cmd_offset; >> + u32 idle_cmd; >> + u32 read_cmd; >> + u32 write_cmd; >> + >> + unsigned int ack_offset; >> + u32 ack_mask; >> + >> + unsigned int addr_offset; >> + unsigned int read_offset; >> + unsigned int write_offset; >> + >> + unsigned long sleep_us; >> + unsigned long timeout_us; >> +}; >> + >> /** >> * struct regmap_range - A register range, used for access related checks >> * (readable/writeable/volatile/precious checks) >> @@ -431,6 +466,8 @@ struct regmap_config { >> const struct regmap_range_cfg *ranges; >> unsigned int num_ranges; >> >> + const struct regmap_indirect_cfg *indirect_cfg; >> + >> bool use_hwlock; >> bool use_raw_spinlock; >> unsigned int hwlock_id; >> @@ -693,6 +730,12 @@ struct regmap *__devm_regmap_init_spi_avmm(struct spi_device *spi, >> const struct regmap_config *config, >> struct lock_class_key *lock_key, >> const char *lock_name); >> +struct regmap *__devm_regmap_init_indirect(struct device *dev, >> + void __iomem *base, >> + struct regmap_config *cfg, >> + struct lock_class_key *lock_key, >> + const char *lock_name); >> + >> /* >> * Wrapper for regmap_init macros to include a unique lockdep key and name >> * for each call. No-op if CONFIG_LOCKDEP is not set. >> @@ -1148,6 +1191,18 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg); >> __regmap_lockdep_wrapper(__devm_regmap_init_spi_avmm, #config, \ >> spi, config) >> >> +/** >> + * devm_regmap_init_indirect - create a regmap for indirect register access >> + * @dev: device creating the regmap >> + * @base: __iomem point to base of memory with mailbox >> + * @cfg: regmap_config describing interface >> + * >> + * Return: 0 on success, negative error code otherwise. >> + */ >> +#define devm_regmap_init_indirect(dev, base, config) \ >> + __regmap_lockdep_wrapper(__devm_regmap_init_indirect, #config, \ >> + dev, base, config) >> + >> int regmap_mmio_attach_clk(struct regmap *map, struct clk *clk); >> void regmap_mmio_detach_clk(struct regmap *map); >> void regmap_exit(struct regmap *map); > > Thanks, > Marco > > >
| |