Hi Jean-Jacques,
On Tue, 5 Nov 2019 at 04:50, Jean-Jacques Hiblot <jjhib...@ti.com> wrote:
>
> This adds a driver for mmio-based syscon multiplexers controlled by
> bitfields in a syscon register range.
> This is heavily based on the linux mmio-mux driver.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhib...@ti.com>
> ---
>
> Changes in v2: None
>
> drivers/mux/Kconfig | 15 +++++
> drivers/mux/Makefile | 1 +
> drivers/mux/mmio.c | 155 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 171 insertions(+)
> create mode 100644 drivers/mux/mmio.c
Reviewed-by: Simon Glass <s...@chromium.org>
Nits below.
>
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index ad0199c058..bda6a2d9f5 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -4,4 +4,19 @@ config MULTIPLEXER
> bool "Multiplexer Support"
> depends on DM
>
> +
> +if MULTIPLEXER
> +
> +config MUX_MMIO
> + bool "MMIO register bitfield-controlled Multiplexer"
> + depends on MULTIPLEXER && SYSCON
> + help
> + MMIO register bitfield-controlled Multiplexer controller.
> +
> + The driver builds multiplexer controllers for bitfields in a syscon
> + register. For N bit wide bitfields, there will be 2^N possible
> + multiplexer states.
> +
> +endif
> +
> endmenu
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index 351e4363d3..78ebf04c7a 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -4,3 +4,4 @@
> # Jean-Jacques Hiblot <jjhib...@ti.com>
>
> obj-$(CONFIG_$(SPL_)MULTIPLEXER) += mux-uclass.o
> +obj-$(CONFIG_$(SPL_)MUX_MMIO) += mmio.o
> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
> new file mode 100644
> index 0000000000..a9faaeb9fd
> --- /dev/null
> +++ b/drivers/mux/mmio.c
> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MMIO register bitfield-controlled multiplexer driver
> + * Based on the linux mmio multiplexer driver
> + *
> + * Copyright (C) 2017 Pengutronix, Philipp Zabel <ker...@pengutronix.de>
> + * Copyright (C) 2019 Texas Instrument, Jean-jacques Hiblot <jjhib...@ti.com>
> + */
> +#include <common.h>
> +#include <dm.h>
> +#include <mux-internal.h>
> +#include <regmap.h>
> +#include <syscon.h>
> +#include <dm/device.h>
> +#include <dm/read.h>
> +#include <dt-bindings/mux/mux.h>
> +
> +static int mux_mmio_set(struct mux_control *mux, int state)
> +{
> + struct regmap_field **fields = dev_get_priv(mux->dev);
> +
> + return regmap_field_write(fields[mux_control_get_index(mux)], state);
> +}
> +
> +static const struct mux_control_ops mux_mmio_ops = {
> + .set = mux_mmio_set,
> +};
> +
> +static const struct udevice_id mmio_mux_of_match[] = {
> + { .compatible = "mmio-mux" },
> + { /* sentinel */ },
> +};
> +
> +static int mmio_mux_probe(struct udevice *dev)
> +{
> + struct regmap_field **fields;
> + struct mux_chip *mux_chip = dev_get_uclass_priv(dev);
> + struct regmap *regmap;
> + u32 *mux_reg_masks;
> + u32 *idle_states;
> + int num_fields;
> + int ret;
> + int i;
> +
> + regmap = syscon_node_to_regmap(dev_ofnode(dev->parent));
> + if (IS_ERR(regmap)) {
> + ret = PTR_ERR(regmap);
> + dev_err(dev, "failed to get regmap: %d\n", ret);
> + return ret;
> + }
> +
> + num_fields = dev_read_size(dev, "mux-reg-masks");
> + if (num_fields < 0) {
> + dev_err(dev, "mux-reg-masks property missing or invalid:
> %d\n",
> + num_fields);
These seem like errors that should not happen in a real system.
You could use
return log_msg_ret("mux-reg-masks" ,-EINVAL)
which reduces to nothing unless CONFIG_LOG_ERROR_RETURN is set.
That gives a nice backtrace of all errors that happened with a little
string tag to explain each one.
> + return num_fields;
> + }
> + num_fields /= sizeof(u32);
> + if (num_fields == 0 || num_fields % 2)
> + ret = -EINVAL;
> + num_fields = num_fields / 2;
> +
> + ret = mux_alloc_controllers(dev, num_fields);
> + if (ret < 0) {
> + dev_err(dev, "failed to allocate mux controllers: %d\n",
> + ret);
Same here and below. Seems like a waste to include this string when we
are just out of memory.
> + return ret;
> + }
> +
> + fields = devm_kmalloc(dev, num_fields * sizeof(*fields), __GFP_ZERO);
> + if (!fields)
> + return -ENOMEM;
> + dev->priv = fields;
> +
> + mux_reg_masks = devm_kmalloc(dev, num_fields * 2 * sizeof(u32),
> + __GFP_ZERO);
> + if (!mux_reg_masks)
> + return -ENOMEM;
> +
> + ret = dev_read_u32_array(dev, "mux-reg-masks", mux_reg_masks,
> + num_fields * 2);
> + if (ret < 0) {
> + dev_err(dev, "failed to read mux-reg-masks property: %d\n",
> + ret);
> + return ret;
> + }
> +
> + idle_states = devm_kmalloc(dev, num_fields * sizeof(u32), __GFP_ZERO);
> + if (!idle_states)
> + return -ENOMEM;
> +
> + ret = dev_read_u32_array(dev, "idle-states", idle_states, num_fields);
> + if (ret < 0) {
> + dev_err(dev, "failed to read idle-states property: %d\n",
> + ret);
> + devm_kfree(dev, idle_states);
> + idle_states = NULL;
> + }
> +
> + for (i = 0; i < num_fields; i++) {
> + struct mux_control *mux = &mux_chip->mux[i];
> + struct reg_field field;
> + u32 reg, mask;
> + int bits;
> +
> + reg = mux_reg_masks[2 * i];
> + mask = mux_reg_masks[2 * i + 1];
> +
> + field.reg = reg;
> + field.msb = fls(mask) - 1;
> + field.lsb = ffs(mask) - 1;
> +
> + if (mask != GENMASK(field.msb, field.lsb)) {
> + dev_err(dev, "bitfield %d: invalid mask 0x%x\n",
> + i, mask);
> + return -EINVAL;
> + }
> +
> + fields[i] = devm_regmap_field_alloc(dev, regmap, field);
> + if (IS_ERR(fields[i])) {
> + ret = PTR_ERR(fields[i]);
> + dev_err(dev, "bitfield %d: failed allocate: %d\n",
> + i, ret);
> + return ret;
> + }
> +
> + bits = 1 + field.msb - field.lsb;
> + mux->states = 1 << bits;
> +
> + if (!idle_states)
> + continue;
> +
> + if (idle_states[i] != MUX_IDLE_AS_IS &&
> + idle_states[i] >= mux->states) {
> + dev_err(dev, "bitfield: %d: out of range idle state
> %d\n",
> + i, idle_states[i]);
> + return -EINVAL;
> + }
> + mux->idle_state = idle_states[i];
> + }
> +
> + devm_kfree(dev, mux_reg_masks);
> + if (idle_states)
> + devm_kfree(dev, idle_states);
> +
> + return 0;
> +}
> +
> +U_BOOT_DRIVER(mmio_mux) = {
> + .name = "mmio-mux",
> + .id = UCLASS_MUX,
> + .of_match = mmio_mux_of_match,
> + .probe = mmio_mux_probe,
> + .ops = &mux_mmio_ops,
> +};
> --
> 2.17.1
>
Regards,
Simon
