Re: [PATCH v2 3/4] drivers: mux: mmio-based syscon mux controller

Hi Jean-Jacques,

On Tue, 5 Nov 2019 at 04:50, Jean-Jacques Hiblot jjhiblot@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 jjhiblot@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 sjg@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 jjhiblot@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 kernel@pengutronix.de
• • Copyright (C) 2019 Texas Instrument, Jean-jacques Hiblot jjhiblot@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