[v2,4/4] mux: lan966: Add support for flexcom mux controller

Commit Message

Kavyasree Kotagiri May 9, 2022, 8:49 a.m. UTC

LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
For each chip select of each flexcom there is a configuration
register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
configuration register is 21 because there are 21 shared pins
on each of which the chip select can be mapped. Each bit of the
register represents a different FLEXCOM_SHARED pin.

Signed-off-by: Kavyasree Kotagiri <[e-mail]>
---
 arch/arm/mach-at91/Kconfig  |   2 +
 drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-
 drivers/mux/Kconfig         |  12 ++++
 drivers/mux/Makefile        |   2 +
 drivers/mux/lan966-flx.c    | 121 ++++++++++++++++++++++++++++++++++++
 5 files changed, 191 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mux/lan966-flx.c

Comments

Peter Rosin May 9, 2022, 11:37 a.m. UTC | #1

Hi!

2022-05-09 at 10:49, Kavyasree Kotagiri wrote:
> LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> For each chip select of each flexcom there is a configuration
> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> configuration register is 21 because there are 21 shared pins
> on each of which the chip select can be mapped. Each bit of the
> register represents a different FLEXCOM_SHARED pin.
> 
> Signed-off-by: Kavyasree Kotagiri <[e-mail]>
> ---
>  arch/arm/mach-at91/Kconfig  |   2 +
>  drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-
>  drivers/mux/Kconfig         |  12 ++++
>  drivers/mux/Makefile        |   2 +
>  drivers/mux/lan966-flx.c    | 121 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 191 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mux/lan966-flx.c
> 
> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> index 279810381256..26fb0f4e1b79 100644
> --- a/arch/arm/mach-at91/Kconfig
> +++ b/arch/arm/mach-at91/Kconfig
> @@ -74,6 +74,8 @@ config SOC_LAN966
>  	select DW_APB_TIMER_OF
>  	select ARM_GIC
>  	select MEMORY
> +	select MULTIPLEXER
> +	select MUX_LAN966
>  	help
>  	  This enables support for ARMv7 based Microchip LAN966 SoC family.
>  
> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> index 559eb4d352b6..7cfd0fc3f4f0 100644
> --- a/drivers/mfd/atmel-flexcom.c
> +++ b/drivers/mfd/atmel-flexcom.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/clk.h>
>  #include <dt-bindings/mfd/atmel-flexcom.h>
> +#include <linux/mux/consumer.h>
>  
>  /* I/O register offsets */
>  #define FLEX_MR		0x0	/* Mode Register */
> @@ -28,6 +29,10 @@
>  #define FLEX_MR_OPMODE(opmode)	(((opmode) << FLEX_MR_OPMODE_OFFSET) &	\
>  				 FLEX_MR_OPMODE_MASK)
>  
> +struct atmel_flex_caps {
> +	bool has_flx_mux;
> +};
> +
>  struct atmel_flexcom {
>  	void __iomem *base;
>  	u32 opmode;
> @@ -37,6 +42,7 @@ struct atmel_flexcom {
>  static int atmel_flexcom_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> +	const struct atmel_flex_caps *caps;
>  	struct resource *res;
>  	struct atmel_flexcom *ddata;
>  	int err;
> @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct platform_device *pdev)
>  	 */
>  	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
>  
> +	caps = of_device_get_match_data(&pdev->dev);
> +	if (!caps) {
> +		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Flexcom Mux */
> +	if (caps->has_flx_mux && of_property_read_bool(np, "mux-controls")) {
> +		struct mux_control *flx_mux;
> +		struct of_phandle_args args;
> +		int i, count;
> +
> +		flx_mux = devm_mux_control_get(&pdev->dev, NULL);
> +		if (IS_ERR(flx_mux))
> +			return PTR_ERR(flx_mux);
> +
> +		count = of_property_count_strings(np, "mux-control-names");
> +		for (i = 0; i < count; i++) {
> +			err = of_parse_phandle_with_fixed_args(np, "mux-controls", 1, i, &args);
> +			if (err)
> +				break;
> +
> +			err = mux_control_select(flx_mux, args.args[0]);
> +			if (!err) {
> +				mux_control_deselect(flx_mux);

This is suspect. When you deselect the mux you rely on the mux to be
configured with "as-is" as the idle state. But you do not document that.
This is also fragile in that you silently rely on noone else selecting
the mux to some unwanted state behind your back (mux controls are not
exclusive the way e.g. gpio pins or pwms are). The protocol is that
others may get a ref to the mux control and select it as long as noone
else has selected it.

The only sane thing to do is to keep the mux selected for the whole
duration when you rely on it to be in your desired state.

> +			} else {
> +				dev_err(&pdev->dev, "Failed to select FLEXCOM mux\n");
> +				return err;
> +			}
> +		}
> +	}
> +
>  	clk_disable_unprepare(ddata->clk);
>  
>  	return devm_of_platform_populate(&pdev->dev);
>  }
>  
> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> +
> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> +	.has_flx_mux = true,
> +};
> +
>  static const struct of_device_id atmel_flexcom_of_match[] = {
> -	{ .compatible = "atmel,sama5d2-flexcom" },
> +	{
> +		.compatible = "atmel,sama5d2-flexcom",
> +		.data = &atmel_flexcom_caps,
> +	},
> +
> +	{
> +		.compatible = "microchip,lan966-flexcom",
> +		.data = &lan966x_flexcom_caps,
> +	},
> +
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index e5c571fd232c..ea09f474bc2f 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -45,6 +45,18 @@ config MUX_GPIO
>  	  To compile the driver as a module, choose M here: the module will
>  	  be called mux-gpio.
>  
> +config MUX_LAN966
> +	tristate "LAN966 Flexcom multiplexer"
> +	depends on OF || COMPILE_TEST
> +	help
> +	Lan966 Flexcom Multiplexer controller.
> +
> +	The driver supports mapping 2 chip-selects of each of the lan966
> +	flexcoms to 21 flexcom shared pins.
> +
> +	To compile the driver as a module, choose M here: the module will
> +	be called mux-lan966.
> +
>  config MUX_MMIO
>  	tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
>  	depends on OF || COMPILE_TEST
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index 6e9fa47daf56..53a9840d96fa 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -7,10 +7,12 @@ mux-core-objs			:= core.o
>  mux-adg792a-objs		:= adg792a.o
>  mux-adgs1408-objs		:= adgs1408.o
>  mux-gpio-objs			:= gpio.o
> +mux-lan966-objs			:= lan966-flx.o
>  mux-mmio-objs			:= mmio.o
>  
>  obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
>  obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
>  obj-$(CONFIG_MUX_ADGS1408)	+= mux-adgs1408.o
>  obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
> +obj-$(CONFIG_MUX_LAN966)	+= mux-lan966.o
>  obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
> diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
> new file mode 100644
> index 000000000000..2c7dab616a6a
> --- /dev/null
> +++ b/drivers/mux/lan966-flx.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LAN966 Flexcom MUX driver
> + *
> + * Copyright (c) 2022 Microchip Inc.
> + *
> + * Author: Kavyasree Kotagiri <[e-mail]>

Looks like it is based on mmio.c?

> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/mux/driver.h>
> +#include <linux/io.h>
> +
> +#define FLEX_SHRD_MASK		0x1FFFFF
> +#define LAN966_MAX_CS		21
> +
> +static void __iomem *flx_shared_base;

I would much prefer to store the combined address (base+offset)
in the mux private data instead of only storing the offset and
then unnecessarily rely on some piece of global state (that
will be clobbered by other instances).

> +struct mux_lan966x {

Why is the file named lan966, but then everything inside lan966x?

> +	u32 offset;
> +	u32 ss_pin;
> +};
> +
> +static int mux_lan966x_set(struct mux_control *mux, int state)
> +{
> +	struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> +	u32 val;
> +
> +	val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
> +	writel(val, flx_shared_base + mux_lan966x[state].offset);

This reads memory you have not allocated, if you select a state
other than zero.

> +
> +	return 0;
> +}
> +
> +static const struct mux_control_ops mux_lan966x_ops = {
> +	.set = mux_lan966x_set,
> +};
> +
> +static const struct of_device_id mux_lan966x_dt_ids[] = {
> +	{ .compatible = "microchip,lan966-flx-mux", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
> +
> +static int mux_lan966x_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct mux_lan966x *mux_lan966x;
> +	struct mux_chip *mux_chip;
> +	int ret, num_fields, i;
> +
> +	ret = of_property_count_u32_elems(np, "mux-offset-pin");
> +	if (ret == 0 || ret % 2)
> +		ret = -EINVAL;
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "mux-offset-pin property missing or invalid");
> +	num_fields = ret / 2;
> +
> +	mux_chip = devm_mux_chip_alloc(dev, num_fields, sizeof(*mux_lan966x));

I might be thoroughly mistaken and confused by the code, but it seems
very strange that a subsequenct select is not undoing what a previous
select once did. Each state seems to write to its own register offset,
and there is nothing that restores the first register offset with you
switch states.

Care to explain how muxing works and what you are expected to do to
manipulate the mux? Is there some link to public documentation? I did
a quick search for lan966 but came up with nothing relevant.

> +	if (IS_ERR(mux_chip))
> +		return dev_err_probe(dev, PTR_ERR(mux_chip),
> +				     "failed to allocate mux_chips\n");
> +
> +	mux_lan966x = mux_chip_priv(mux_chip);
> +
> +	flx_shared_base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> +	if (IS_ERR(flx_shared_base))
> +		return dev_err_probe(dev, PTR_ERR(flx_shared_base),
> +				     "failed to get flexcom shared base address\n");
> +
> +	for (i = 0; i < num_fields; i++) {
> +		struct mux_control *mux = &mux_chip->mux[i];
> +		u32 offset, shared_pin;
> +
> +		ret = of_property_read_u32_index(np, "mux-offset-pin",
> +						 2 * i, &offset);
> +		if (ret == 0)
> +			ret = of_property_read_u32_index(np, "mux-offset-pin",
> +							 2 * i + 1,
> +							 &shared_pin);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to read mux-offset-pin property: %d", i);
> +
> +		if (shared_pin >= LAN966_MAX_CS)
> +			return -EINVAL;
> +
> +		mux_lan966x[i].offset = offset;
> +		mux_lan966x[i].ss_pin = shared_pin;

This clobbers memory you have not allocated, if num_fields >= 1.

Cheers,
Peter

> +
> +		mux->states = LAN966_MAX_CS;
> +	}
> +
> +	mux_chip->ops = &mux_lan966x_ops;
> +
> +	ret = devm_mux_chip_register(dev, mux_chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mux_lan966x_driver = {
> +	.driver = {
> +		.name = "lan966-mux",
> +		.of_match_table	= of_match_ptr(mux_lan966x_dt_ids),
> +	},
> +	.probe = mux_lan966x_probe,
> +};
> +
> +module_platform_driver(mux_lan966x_driver);
> +
> +MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
> +MODULE_AUTHOR("Kavyasree Kotagiri <[e-mail]>");
> +MODULE_LICENSE("GPL v2");
> +

Peter Rosin May 9, 2022, 11:41 a.m. UTC | #2

> This clobbers memory you have not allocated, if num_fields >= 1.

Oops, >= 2

Sorry for the noise.

Cheers,
Peter

kernel test robot May 10, 2022, 1:46 a.m. UTC | #3

Hi Kavyasree,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on lee-mfd/for-mfd-next]
[also build test ERROR on robh/for-next]
[cannot apply to soc/for-next linus/master v5.18-rc6 next-20220509]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Kavyasree-Kotagiri/Add-support-for-lan966-flexcom-multiplexer/20220509-171104
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: arm-at91_dt_defconfig (https://download.01.org/0day-ci/archive/20220510/[e-mail]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/6b77ec16441906d1aa067b60cf97807111abdd72
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kavyasree-Kotagiri/Add-support-for-lan966-flexcom-multiplexer/20220509-171104
        git checkout 6b77ec16441906d1aa067b60cf97807111abdd72
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/mfd/atmel-flexcom.o: in function `atmel_flexcom_probe':
   atmel-flexcom.c:(.text+0x160): undefined reference to `devm_mux_control_get'
>> arm-linux-gnueabi-ld: atmel-flexcom.c:(.text+0x1ec): undefined reference to `mux_control_select_delay'
>> arm-linux-gnueabi-ld: atmel-flexcom.c:(.text+0x1fc): undefined reference to `mux_control_deselect'

kernel test robot May 10, 2022, 3:28 a.m. UTC | #4

Hi Kavyasree,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on lee-mfd/for-mfd-next]
[also build test ERROR on robh/for-next]
[cannot apply to soc/for-next linus/master v5.18-rc6 next-20220509]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Kavyasree-Kotagiri/Add-support-for-lan966-flexcom-multiplexer/20220509-171104
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: arm64-randconfig-r013-20220509 (https://download.01.org/0day-ci/archive/20220510/[e-mail]/config)
compiler: aarch64-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/6b77ec16441906d1aa067b60cf97807111abdd72
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kavyasree-Kotagiri/Add-support-for-lan966-flexcom-multiplexer/20220509-171104
        git checkout 6b77ec16441906d1aa067b60cf97807111abdd72
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All errors (new ones prefixed by >>):

   aarch64-linux-ld: Unexpected GOT/PLT entries detected!
   aarch64-linux-ld: Unexpected run-time procedure linkages detected!
   aarch64-linux-ld: drivers/mfd/atmel-flexcom.o: in function `atmel_flexcom_probe':
   atmel-flexcom.c:(.text+0x178): undefined reference to `devm_mux_control_get'
>> aarch64-linux-ld: atmel-flexcom.c:(.text+0x1e0): undefined reference to `mux_control_select_delay'
>> aarch64-linux-ld: atmel-flexcom.c:(.text+0x1f4): undefined reference to `mux_control_deselect'

Kavyasree Kotagiri May 10, 2022, 2:59 p.m. UTC | #5

> > LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> > For each chip select of each flexcom there is a configuration
> > register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> > configuration register is 21 because there are 21 shared pins
> > on each of which the chip select can be mapped. Each bit of the
> > register represents a different FLEXCOM_SHARED pin.
> >
> > Signed-off-by: Kavyasree Kotagiri <[e-mail]>
> > ---
> >  arch/arm/mach-at91/Kconfig  |   2 +
> >  drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-
> >  drivers/mux/Kconfig         |  12 ++++
> >  drivers/mux/Makefile        |   2 +
> >  drivers/mux/lan966-flx.c    | 121
> ++++++++++++++++++++++++++++++++++++
> >  5 files changed, 191 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/mux/lan966-flx.c
> >
> > diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> > index 279810381256..26fb0f4e1b79 100644
> > --- a/arch/arm/mach-at91/Kconfig
> > +++ b/arch/arm/mach-at91/Kconfig
> > @@ -74,6 +74,8 @@ config SOC_LAN966
> >       select DW_APB_TIMER_OF
> >       select ARM_GIC
> >       select MEMORY
> > +     select MULTIPLEXER
> > +     select MUX_LAN966
> >       help
> >         This enables support for ARMv7 based Microchip LAN966 SoC family.
> >
> > diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> > index 559eb4d352b6..7cfd0fc3f4f0 100644
> > --- a/drivers/mfd/atmel-flexcom.c
> > +++ b/drivers/mfd/atmel-flexcom.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/io.h>
> >  #include <linux/clk.h>
> >  #include <dt-bindings/mfd/atmel-flexcom.h>
> > +#include <linux/mux/consumer.h>
> >
> >  /* I/O register offsets */
> >  #define FLEX_MR              0x0     /* Mode Register */
> > @@ -28,6 +29,10 @@
> >  #define FLEX_MR_OPMODE(opmode)       (((opmode) <<
> FLEX_MR_OPMODE_OFFSET) &  \
> >                                FLEX_MR_OPMODE_MASK)
> >
> > +struct atmel_flex_caps {
> > +     bool has_flx_mux;
> > +};
> > +
> >  struct atmel_flexcom {
> >       void __iomem *base;
> >       u32 opmode;
> > @@ -37,6 +42,7 @@ struct atmel_flexcom {
> >  static int atmel_flexcom_probe(struct platform_device *pdev)
> >  {
> >       struct device_node *np = pdev->dev.of_node;
> > +     const struct atmel_flex_caps *caps;
> >       struct resource *res;
> >       struct atmel_flexcom *ddata;
> >       int err;
> > @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct
> platform_device *pdev)
> >        */
> >       writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
> >
> > +     caps = of_device_get_match_data(&pdev->dev);
> > +     if (!caps) {
> > +             dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* Flexcom Mux */
> > +     if (caps->has_flx_mux && of_property_read_bool(np, "mux-controls"))
> {
> > +             struct mux_control *flx_mux;
> > +             struct of_phandle_args args;
> > +             int i, count;
> > +
> > +             flx_mux = devm_mux_control_get(&pdev->dev, NULL);
> > +             if (IS_ERR(flx_mux))
> > +                     return PTR_ERR(flx_mux);
> > +
> > +             count = of_property_count_strings(np, "mux-control-names");
> > +             for (i = 0; i < count; i++) {
> > +                     err = of_parse_phandle_with_fixed_args(np, "mux-controls",
> 1, i, &args);
> > +                     if (err)
> > +                             break;
> > +
> > +                     err = mux_control_select(flx_mux, args.args[0]);
> > +                     if (!err) {
> > +                             mux_control_deselect(flx_mux);
> 
> This is suspect. When you deselect the mux you rely on the mux to be
> configured with "as-is" as the idle state. But you do not document that.
> This is also fragile in that you silently rely on noone else selecting
> the mux to some unwanted state behind your back (mux controls are not
> exclusive the way e.g. gpio pins or pwms are). The protocol is that
> others may get a ref to the mux control and select it as long as noone
> else has selected it.
> 
> The only sane thing to do is to keep the mux selected for the whole
> duration when you rely on it to be in your desired state.
>

Yes, mux is kept selected until configuring register is done. Please see below log where
I added debug prints just for understanding:
# dmesg | grep KK
 [    0.779827] KK: Func: atmel_flexcom_probe ***** [START flx muxing] ********
[    0.779875] KK: Func: atmel_flexcom_probe i = 0 args[0] = 0
[    0.779890] KK: Func: mux_control_select [Entered]
[    0.779902] KK: Func: mux_lan966x_set [Entered] state = 0
[    0.779977] KK: Func: mux_lan966x_set [Read] = 0x1fffef   <<<----- setting mux_lan966x[state].ss_pin "4" which is passed from dts
[    0.779992] KK: Func: mux_lan966x_set [Exit]
[    0.780002] KK: Func: mux_control_select [Exit]
[    0.780011] KK: Func: mux_control_deselect [Entered]
[    0.780021] KK: Func: mux_control_deselect [Exit]


> > +                     } else {
> > +                             dev_err(&pdev->dev, "Failed to select FLEXCOM mux\n");
> > +                             return err;
> > +                     }
> > +             }
> > +     }
> > +
> >       clk_disable_unprepare(ddata->clk);
> >
> >       return devm_of_platform_populate(&pdev->dev);
> >  }
> >
> > +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> > +
> > +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> > +     .has_flx_mux = true,
> > +};
> > +
> >  static const struct of_device_id atmel_flexcom_of_match[] = {
> > -     { .compatible = "atmel,sama5d2-flexcom" },
> > +     {
> > +             .compatible = "atmel,sama5d2-flexcom",
> > +             .data = &atmel_flexcom_caps,
> > +     },
> > +
> > +     {
> > +             .compatible = "microchip,lan966-flexcom",
> > +             .data = &lan966x_flexcom_caps,
> > +     },
> > +
> >       { /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> > diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> > index e5c571fd232c..ea09f474bc2f 100644
> > --- a/drivers/mux/Kconfig
> > +++ b/drivers/mux/Kconfig
> > @@ -45,6 +45,18 @@ config MUX_GPIO
> >         To compile the driver as a module, choose M here: the module will
> >         be called mux-gpio.
> >
> > +config MUX_LAN966
> > +     tristate "LAN966 Flexcom multiplexer"
> > +     depends on OF || COMPILE_TEST
> > +     help
> > +     Lan966 Flexcom Multiplexer controller.
> > +
> > +     The driver supports mapping 2 chip-selects of each of the lan966
> > +     flexcoms to 21 flexcom shared pins.
> > +
> > +     To compile the driver as a module, choose M here: the module will
> > +     be called mux-lan966.
> > +
> >  config MUX_MMIO
> >       tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
> >       depends on OF || COMPILE_TEST
> > diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> > index 6e9fa47daf56..53a9840d96fa 100644
> > --- a/drivers/mux/Makefile
> > +++ b/drivers/mux/Makefile
> > @@ -7,10 +7,12 @@ mux-core-objs                       := core.o
> >  mux-adg792a-objs             := adg792a.o
> >  mux-adgs1408-objs            := adgs1408.o
> >  mux-gpio-objs                        := gpio.o
> > +mux-lan966-objs                      := lan966-flx.o
> >  mux-mmio-objs                        := mmio.o
> >
> >  obj-$(CONFIG_MULTIPLEXER)    += mux-core.o
> >  obj-$(CONFIG_MUX_ADG792A)    += mux-adg792a.o
> >  obj-$(CONFIG_MUX_ADGS1408)   += mux-adgs1408.o
> >  obj-$(CONFIG_MUX_GPIO)               += mux-gpio.o
> > +obj-$(CONFIG_MUX_LAN966)     += mux-lan966.o
> >  obj-$(CONFIG_MUX_MMIO)               += mux-mmio.o
> > diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
> > new file mode 100644
> > index 000000000000..2c7dab616a6a
> > --- /dev/null
> > +++ b/drivers/mux/lan966-flx.c
> > @@ -0,0 +1,121 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LAN966 Flexcom MUX driver
> > + *
> > + * Copyright (c) 2022 Microchip Inc.
> > + *
> > + * Author: Kavyasree Kotagiri <[e-mail]>
> 
> Looks like it is based on mmio.c?
> 
Yes, I took mmio.c  driver as reference.

> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/mux/driver.h>
> > +#include <linux/io.h>
> > +
> > +#define FLEX_SHRD_MASK               0x1FFFFF
> > +#define LAN966_MAX_CS                21
> > +
> > +static void __iomem *flx_shared_base;
> 
> I would much prefer to store the combined address (base+offset)
> in the mux private data instead of only storing the offset and
> then unnecessarily rely on some piece of global state (that
> will be clobbered by other instances).
> 
Ok. I will try to see if this is relevant and change accordingly.

> > +struct mux_lan966x {
> 
> Why is the file named lan966, but then everything inside lan966x?
> 
> > +     u32 offset;
> > +     u32 ss_pin;
> > +};
> > +
> > +static int mux_lan966x_set(struct mux_control *mux, int state)
> > +{
> > +     struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> > +     u32 val;
> > +
> > +     val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
> > +     writel(val, flx_shared_base + mux_lan966x[state].offset);
> 
> This reads memory you have not allocated, if you select a state
> other than zero.
> 
Ok. I will return error condition in case of trying to access none existing entry.
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct mux_control_ops mux_lan966x_ops = {
> > +     .set = mux_lan966x_set,
> > +};
> > +
> > +static const struct of_device_id mux_lan966x_dt_ids[] = {
> > +     { .compatible = "microchip,lan966-flx-mux", },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
> > +
> > +static int mux_lan966x_probe(struct platform_device *pdev)
> > +{
> > +     struct device_node *np = pdev->dev.of_node;
> > +     struct device *dev = &pdev->dev;
> > +     struct mux_lan966x *mux_lan966x;
> > +     struct mux_chip *mux_chip;
> > +     int ret, num_fields, i;
> > +
> > +     ret = of_property_count_u32_elems(np, "mux-offset-pin");
> > +     if (ret == 0 || ret % 2)
> > +             ret = -EINVAL;
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret,
> > +                                  "mux-offset-pin property missing or invalid");
> > +     num_fields = ret / 2;
> > +
> > +     mux_chip = devm_mux_chip_alloc(dev, num_fields,
> sizeof(*mux_lan966x));
> 
> I might be thoroughly mistaken and confused by the code, but it seems
> very strange that a subsequenct select is not undoing what a previous
> select once did. Each state seems to write to its own register offset,
> and there is nothing that restores the first register offset with you
> switch states.
> 
> Care to explain how muxing works and what you are expected to do to
> manipulate the mux? Is there some link to public documentation? I did
> a quick search for lan966 but came up with nothing relevant.
>
LAN966 has 5 flexcoms(which can be used as USART/SPI/I2C interface).
FLEXCOM has two chip-select I/O lines namely CS0 and CS1
in SPI mode, CTS and RTS in USART mode. These FLEXCOM pins are optional.
These chip-selects can be mapped to flexcom shared pin [0-21] which can be
done by configuring flexcom multiplexer register(FLEXCOM_SHARED[0-4]:SS_MASK[0-1])
Driver explanation:
"flx_shared_base" is used to get base address of Flexcom shared multiplexer
"mux-offset-pin" property is used to get cs0/cs1 offset and flexcom shared pin[0-21] of a flexcom.
state value passed from atmel-flexcom is used to configure respective 
FLEXCOM_SHARED[0-4]:SS_MASK[0-1] register with offset and flexcom shared pin.

> > +     if (IS_ERR(mux_chip))
> > +             return dev_err_probe(dev, PTR_ERR(mux_chip),
> > +                                  "failed to allocate mux_chips\n");
> > +
> > +     mux_lan966x = mux_chip_priv(mux_chip);
> > +
> > +     flx_shared_base = devm_platform_get_and_ioremap_resource(pdev,
> 0, NULL);
> > +     if (IS_ERR(flx_shared_base))
> > +             return dev_err_probe(dev, PTR_ERR(flx_shared_base),
> > +                                  "failed to get flexcom shared base address\n");
> > +
> > +     for (i = 0; i < num_fields; i++) {
> > +             struct mux_control *mux = &mux_chip->mux[i];
> > +             u32 offset, shared_pin;
> > +
> > +             ret = of_property_read_u32_index(np, "mux-offset-pin",
> > +                                              2 * i, &offset);
> > +             if (ret == 0)
> > +                     ret = of_property_read_u32_index(np, "mux-offset-pin",
> > +                                                      2 * i + 1,
> > +                                                      &shared_pin);
> > +             if (ret < 0)
> > +                     return dev_err_probe(dev, ret,
> > +                                          "failed to read mux-offset-pin property: %d", i);
> > +
> > +             if (shared_pin >= LAN966_MAX_CS)
> > +                     return -EINVAL;
> > +
> > +             mux_lan966x[i].offset = offset;
> > +             mux_lan966x[i].ss_pin = shared_pin;
> 
> This clobbers memory you have not allocated, if num_fields >= 1.
> 
> Cheers,
> Peter
> 
> > +
> > +             mux->states = LAN966_MAX_CS;
> > +     }
> > +
> > +     mux_chip->ops = &mux_lan966x_ops;
> > +
> > +     ret = devm_mux_chip_register(dev, mux_chip);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static struct platform_driver mux_lan966x_driver = {
> > +     .driver = {
> > +             .name = "lan966-mux",
> > +             .of_match_table = of_match_ptr(mux_lan966x_dt_ids),
> > +     },
> > +     .probe = mux_lan966x_probe,
> > +};
> > +
> > +module_platform_driver(mux_lan966x_driver);
> > +
> > +MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
> > +MODULE_AUTHOR("Kavyasree Kotagiri
> <[e-mail]>");
> > +MODULE_LICENSE("GPL v2");
> > +

Peter Rosin May 10, 2022, 7:38 p.m. UTC | #6

Hi!

2022-05-10 at 16:59, [e-mail] wrote:
>>> LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
>>> For each chip select of each flexcom there is a configuration
>>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
>>> configuration register is 21 because there are 21 shared pins
>>> on each of which the chip select can be mapped. Each bit of the
>>> register represents a different FLEXCOM_SHARED pin.
>>>
>>> Signed-off-by: Kavyasree Kotagiri <[e-mail]>
>>> ---
>>>  arch/arm/mach-at91/Kconfig  |   2 +
>>>  drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-
>>>  drivers/mux/Kconfig         |  12 ++++
>>>  drivers/mux/Makefile        |   2 +
>>>  drivers/mux/lan966-flx.c    | 121
>> ++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 191 insertions(+), 1 deletion(-)
>>>  create mode 100644 drivers/mux/lan966-flx.c
>>>
>>> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
>>> index 279810381256..26fb0f4e1b79 100644
>>> --- a/arch/arm/mach-at91/Kconfig
>>> +++ b/arch/arm/mach-at91/Kconfig
>>> @@ -74,6 +74,8 @@ config SOC_LAN966
>>>       select DW_APB_TIMER_OF
>>>       select ARM_GIC
>>>       select MEMORY
>>> +     select MULTIPLEXER
>>> +     select MUX_LAN966
>>>       help
>>>         This enables support for ARMv7 based Microchip LAN966 SoC family.
>>>
>>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
>>> index 559eb4d352b6..7cfd0fc3f4f0 100644
>>> --- a/drivers/mfd/atmel-flexcom.c
>>> +++ b/drivers/mfd/atmel-flexcom.c
>>> @@ -17,6 +17,7 @@
>>>  #include <linux/io.h>
>>>  #include <linux/clk.h>
>>>  #include <dt-bindings/mfd/atmel-flexcom.h>
>>> +#include <linux/mux/consumer.h>
>>>
>>>  /* I/O register offsets */
>>>  #define FLEX_MR              0x0     /* Mode Register */
>>> @@ -28,6 +29,10 @@
>>>  #define FLEX_MR_OPMODE(opmode)       (((opmode) <<
>> FLEX_MR_OPMODE_OFFSET) &  \
>>>                                FLEX_MR_OPMODE_MASK)
>>>
>>> +struct atmel_flex_caps {
>>> +     bool has_flx_mux;
>>> +};
>>> +
>>>  struct atmel_flexcom {
>>>       void __iomem *base;
>>>       u32 opmode;
>>> @@ -37,6 +42,7 @@ struct atmel_flexcom {
>>>  static int atmel_flexcom_probe(struct platform_device *pdev)
>>>  {
>>>       struct device_node *np = pdev->dev.of_node;
>>> +     const struct atmel_flex_caps *caps;
>>>       struct resource *res;
>>>       struct atmel_flexcom *ddata;
>>>       int err;
>>> @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct
>> platform_device *pdev)
>>>        */
>>>       writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
>>>
>>> +     caps = of_device_get_match_data(&pdev->dev);
>>> +     if (!caps) {
>>> +             dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     /* Flexcom Mux */
>>> +     if (caps->has_flx_mux && of_property_read_bool(np, "mux-controls"))
>> {
>>> +             struct mux_control *flx_mux;
>>> +             struct of_phandle_args args;
>>> +             int i, count;
>>> +
>>> +             flx_mux = devm_mux_control_get(&pdev->dev, NULL);
>>> +             if (IS_ERR(flx_mux))
>>> +                     return PTR_ERR(flx_mux);
>>> +
>>> +             count = of_property_count_strings(np, "mux-control-names");
>>> +             for (i = 0; i < count; i++) {
>>> +                     err = of_parse_phandle_with_fixed_args(np, "mux-controls",
>> 1, i, &args);
>>> +                     if (err)
>>> +                             break;
>>> +
>>> +                     err = mux_control_select(flx_mux, args.args[0]);
>>> +                     if (!err) {
>>> +                             mux_control_deselect(flx_mux);
>>
>> This is suspect. When you deselect the mux you rely on the mux to be
>> configured with "as-is" as the idle state. But you do not document that.
>> This is also fragile in that you silently rely on noone else selecting
>> the mux to some unwanted state behind your back (mux controls are not
>> exclusive the way e.g. gpio pins or pwms are). The protocol is that
>> others may get a ref to the mux control and select it as long as noone
>> else has selected it.
>>
>> The only sane thing to do is to keep the mux selected for the whole
>> duration when you rely on it to be in your desired state.
>>
> 
> Yes, mux is kept selected until configuring register is done. Please see below log where
> I added debug prints just for understanding:
> # dmesg | grep KK
>  [    0.779827] KK: Func: atmel_flexcom_probe ***** [START flx muxing] ********
> [    0.779875] KK: Func: atmel_flexcom_probe i = 0 args[0] = 0
> [    0.779890] KK: Func: mux_control_select [Entered]
> [    0.779902] KK: Func: mux_lan966x_set [Entered] state = 0
> [    0.779977] KK: Func: mux_lan966x_set [Read] = 0x1fffef   <<<----- setting mux_lan966x[state].ss_pin "4" which is passed from dts
> [    0.779992] KK: Func: mux_lan966x_set [Exit]
> [    0.780002] KK: Func: mux_control_select [Exit]
> [    0.780011] KK: Func: mux_control_deselect [Entered]
> [    0.780021] KK: Func: mux_control_deselect [Exit]

You misunderstand. The mux control is only "selected" between the call 
to mux_control_select() and the following call to 
mux_control_deselect().

After that, the mux control is "idle". However, in your case the 
"idle-state" is configured to "as-is" (which is the default), so the 
mux control (naturally) remains in the previously selected state while 
idle. But you are not documenting that limitation, and with this 
implementation you *must* have a mux control that uses "as-is" as its 
idle state. But that is an unexpected and broken limitation, and a 
much better solution is to simply keep the mux control "selected" for 
the complete duration when you rely on it to be in whatever state you 
need it to be in.

>>> +                     } else {
>>> +                             dev_err(&pdev->dev, "Failed to select FLEXCOM mux\n");
>>> +                             return err;
>>> +                     }
>>> +             }
>>> +     }
>>> +
>>>       clk_disable_unprepare(ddata->clk);
>>>
>>>       return devm_of_platform_populate(&pdev->dev);
>>>  }
>>>
>>> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
>>> +
>>> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
>>> +     .has_flx_mux = true,
>>> +};
>>> +
>>>  static const struct of_device_id atmel_flexcom_of_match[] = {
>>> -     { .compatible = "atmel,sama5d2-flexcom" },
>>> +     {
>>> +             .compatible = "atmel,sama5d2-flexcom",
>>> +             .data = &atmel_flexcom_caps,
>>> +     },
>>> +
>>> +     {
>>> +             .compatible = "microchip,lan966-flexcom",
>>> +             .data = &lan966x_flexcom_caps,
>>> +     },
>>> +
>>>       { /* sentinel */ }
>>>  };
>>>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
>>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>>> index e5c571fd232c..ea09f474bc2f 100644
>>> --- a/drivers/mux/Kconfig
>>> +++ b/drivers/mux/Kconfig
>>> @@ -45,6 +45,18 @@ config MUX_GPIO
>>>         To compile the driver as a module, choose M here: the module will
>>>         be called mux-gpio.
>>>
>>> +config MUX_LAN966
>>> +     tristate "LAN966 Flexcom multiplexer"
>>> +     depends on OF || COMPILE_TEST
>>> +     help
>>> +     Lan966 Flexcom Multiplexer controller.
>>> +
>>> +     The driver supports mapping 2 chip-selects of each of the lan966
>>> +     flexcoms to 21 flexcom shared pins.
>>> +
>>> +     To compile the driver as a module, choose M here: the module will
>>> +     be called mux-lan966.
>>> +
>>>  config MUX_MMIO
>>>       tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
>>>       depends on OF || COMPILE_TEST
>>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
>>> index 6e9fa47daf56..53a9840d96fa 100644
>>> --- a/drivers/mux/Makefile
>>> +++ b/drivers/mux/Makefile
>>> @@ -7,10 +7,12 @@ mux-core-objs                       := core.o
>>>  mux-adg792a-objs             := adg792a.o
>>>  mux-adgs1408-objs            := adgs1408.o
>>>  mux-gpio-objs                        := gpio.o
>>> +mux-lan966-objs                      := lan966-flx.o
>>>  mux-mmio-objs                        := mmio.o
>>>
>>>  obj-$(CONFIG_MULTIPLEXER)    += mux-core.o
>>>  obj-$(CONFIG_MUX_ADG792A)    += mux-adg792a.o
>>>  obj-$(CONFIG_MUX_ADGS1408)   += mux-adgs1408.o
>>>  obj-$(CONFIG_MUX_GPIO)               += mux-gpio.o
>>> +obj-$(CONFIG_MUX_LAN966)     += mux-lan966.o
>>>  obj-$(CONFIG_MUX_MMIO)               += mux-mmio.o
>>> diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
>>> new file mode 100644
>>> index 000000000000..2c7dab616a6a
>>> --- /dev/null
>>> +++ b/drivers/mux/lan966-flx.c
>>> @@ -0,0 +1,121 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * LAN966 Flexcom MUX driver
>>> + *
>>> + * Copyright (c) 2022 Microchip Inc.
>>> + *
>>> + * Author: Kavyasree Kotagiri <[e-mail]>
>>
>> Looks like it is based on mmio.c?
>>
> Yes, I took mmio.c  driver as reference.
> 

So, then the above copyright and authorship info is not complete.

>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/property.h>
>>> +#include <linux/mux/driver.h>
>>> +#include <linux/io.h>
>>> +
>>> +#define FLEX_SHRD_MASK               0x1FFFFF
>>> +#define LAN966_MAX_CS                21
>>> +
>>> +static void __iomem *flx_shared_base;
>>
>> I would much prefer to store the combined address (base+offset)
>> in the mux private data instead of only storing the offset and
>> then unnecessarily rely on some piece of global state (that
>> will be clobbered by other instances).
>>
> Ok. I will try to see if this is relevant and change accordingly.
> 
>>> +struct mux_lan966x {
>>
>> Why is the file named lan966, but then everything inside lan966x?
>>
>>> +     u32 offset;
>>> +     u32 ss_pin;
>>> +};
>>> +
>>> +static int mux_lan966x_set(struct mux_control *mux, int state)
>>> +{
>>> +     struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
>>> +     u32 val;
>>> +
>>> +     val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
>>> +     writel(val, flx_shared_base + mux_lan966x[state].offset);
>>
>> This reads memory you have not allocated, if you select a state
>> other than zero.
>>
> Ok. I will return error condition in case of trying to access none existing entry.
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct mux_control_ops mux_lan966x_ops = {
>>> +     .set = mux_lan966x_set,
>>> +};
>>> +
>>> +static const struct of_device_id mux_lan966x_dt_ids[] = {
>>> +     { .compatible = "microchip,lan966-flx-mux", },
>>> +     { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
>>> +
>>> +static int mux_lan966x_probe(struct platform_device *pdev)
>>> +{
>>> +     struct device_node *np = pdev->dev.of_node;
>>> +     struct device *dev = &pdev->dev;
>>> +     struct mux_lan966x *mux_lan966x;
>>> +     struct mux_chip *mux_chip;
>>> +     int ret, num_fields, i;
>>> +
>>> +     ret = of_property_count_u32_elems(np, "mux-offset-pin");
>>> +     if (ret == 0 || ret % 2)
>>> +             ret = -EINVAL;
>>> +     if (ret < 0)
>>> +             return dev_err_probe(dev, ret,
>>> +                                  "mux-offset-pin property missing or invalid");
>>> +     num_fields = ret / 2;
>>> +
>>> +     mux_chip = devm_mux_chip_alloc(dev, num_fields,
>> sizeof(*mux_lan966x));
>>
>> I might be thoroughly mistaken and confused by the code, but it seems
>> very strange that a subsequenct select is not undoing what a previous
>> select once did. Each state seems to write to its own register offset,
>> and there is nothing that restores the first register offset with you
>> switch states.
>>
>> Care to explain how muxing works and what you are expected to do to
>> manipulate the mux? Is there some link to public documentation? I did
>> a quick search for lan966 but came up with nothing relevant.
>>
> LAN966 has 5 flexcoms(which can be used as USART/SPI/I2C interface).
> FLEXCOM has two chip-select I/O lines namely CS0 and CS1
> in SPI mode, CTS and RTS in USART mode. These FLEXCOM pins are optional.
> These chip-selects can be mapped to flexcom shared pin [0-21] which can be
> done by configuring flexcom multiplexer register(FLEXCOM_SHARED[0-4]:SS_MASK[0-1])
> Driver explanation:
> "flx_shared_base" is used to get base address of Flexcom shared multiplexer
> "mux-offset-pin" property is used to get cs0/cs1 offset and flexcom shared pin[0-21] of a flexcom.
> state value passed from atmel-flexcom is used to configure respective 
> FLEXCOM_SHARED[0-4]:SS_MASK[0-1] register with offset and flexcom shared pin.

Ok, let me try to interpret that. You wish enable a "fan out" of these
two chip-selects for any of the 5 flexcoms (in SPI mode?) such that
these 10 internal chip-selects can be connected to any of 21 external
pins?

If that's correct and if you wish to interface with e.g. 20 chips,
then it would be possible to have 20 states for one mux control and
then reach the 20 chips using only CS0 from FLEXCOM0, or it would be
possible to have 2 states for 10 mux controls, one each for CS0/CS1 of
all five flexcoms and also reach 20 chips. Or any wild combo you
imagine is useful.

Is that correctly understood?

Assuming so, then you can have a maximum of 10 mux controls, and for
each mux control you need a variable number of legitimate states. Each
mux control would also need to know at what address/offset its SS_MASK
register is at and what pins/states are valid.

But your code does not implemnent the above. You allocate num_fields
mux controls, which is what confuses the hell out of me. num_fields is
the number of states, not the number of mux controls! And you also
need to specify an individual offset for each state, and that makes no
sense at all, at least not to me.

So, instead, I think you want to have:

struct mux_lan966x {
	u32 ss_mask;
	int num_pins;
	u32 ss_pin[];
};

And then do:

	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_lan966x) + num_pins * sizeof(u32));

(or however that size is best spelled, I haven't kept up)

The set operation would be along the lines of:

static int mux_lan966x_set(struct mux_control *mux, int state)
{
	struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
	u32 val;

	if (state < 0 || state >= mux_lan966x->num_pins)
		return -1;

	val = ~(1 << mux_lan966x->ss_pin[state]) & FLEX_SHRD_MASK;
	writel(val, flx_shared_base + mux_lan966x->ss_mask);

	return 0;
}

Because, one mux control should only ever need to know about one offset,
as it should only ever write to its own SS_MASK register. And you will
need some private space such that you can keep track of which states
are legit.

I would also separate out the SS_MASK offset from the mux-offset-pin
property and have one property for that value and then a straight
array for the valid pin numbers in another property (no longer named
mux-offset-pin of course).

Or something like that and assuming I understand how the FLEXCOMs work
and what you want to do etc.

Cheers,
Peter


>>> +     if (IS_ERR(mux_chip))
>>> +             return dev_err_probe(dev, PTR_ERR(mux_chip),
>>> +                                  "failed to allocate mux_chips\n");
>>> +
>>> +     mux_lan966x = mux_chip_priv(mux_chip);
>>> +
>>> +     flx_shared_base = devm_platform_get_and_ioremap_resource(pdev,
>> 0, NULL);
>>> +     if (IS_ERR(flx_shared_base))
>>> +             return dev_err_probe(dev, PTR_ERR(flx_shared_base),
>>> +                                  "failed to get flexcom shared base address\n");
>>> +
>>> +     for (i = 0; i < num_fields; i++) {
>>> +             struct mux_control *mux = &mux_chip->mux[i];
>>> +             u32 offset, shared_pin;
>>> +
>>> +             ret = of_property_read_u32_index(np, "mux-offset-pin",
>>> +                                              2 * i, &offset);
>>> +             if (ret == 0)
>>> +                     ret = of_property_read_u32_index(np, "mux-offset-pin",
>>> +                                                      2 * i + 1,
>>> +                                                      &shared_pin);
>>> +             if (ret < 0)
>>> +                     return dev_err_probe(dev, ret,
>>> +                                          "failed to read mux-offset-pin property: %d", i);
>>> +
>>> +             if (shared_pin >= LAN966_MAX_CS)
>>> +                     return -EINVAL;
>>> +
>>> +             mux_lan966x[i].offset = offset;
>>> +             mux_lan966x[i].ss_pin = shared_pin;
>>
>> This clobbers memory you have not allocated, if num_fields >= 1.
>>
>> Cheers,
>> Peter
>>
>>> +
>>> +             mux->states = LAN966_MAX_CS;
>>> +     }
>>> +
>>> +     mux_chip->ops = &mux_lan966x_ops;
>>> +
>>> +     ret = devm_mux_chip_register(dev, mux_chip);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static struct platform_driver mux_lan966x_driver = {
>>> +     .driver = {
>>> +             .name = "lan966-mux",
>>> +             .of_match_table = of_match_ptr(mux_lan966x_dt_ids),
>>> +     },
>>> +     .probe = mux_lan966x_probe,
>>> +};
>>> +
>>> +module_platform_driver(mux_lan966x_driver);
>>> +
>>> +MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
>>> +MODULE_AUTHOR("Kavyasree Kotagiri
>> <[e-mail]>");
>>> +MODULE_LICENSE("GPL v2");
>>> +

Claudiu Beznea May 11, 2022, 9:06 a.m. UTC | #7

Hi, Kavyasree,

On 09.05.2022 11:49, Kavyasree Kotagiri wrote:
> LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> For each chip select of each flexcom there is a configuration
> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> configuration register is 21 because there are 21 shared pins
> on each of which the chip select can be mapped. Each bit of the
> register represents a different FLEXCOM_SHARED pin.
> 
> Signed-off-by: Kavyasree Kotagiri <[e-mail]>
> ---
>  arch/arm/mach-at91/Kconfig  |   2 +
>  drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-
>  drivers/mux/Kconfig         |  12 ++++
>  drivers/mux/Makefile        |   2 +
>  drivers/mux/lan966-flx.c    | 121 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 191 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mux/lan966-flx.c
> 
> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> index 279810381256..26fb0f4e1b79 100644
> --- a/arch/arm/mach-at91/Kconfig
> +++ b/arch/arm/mach-at91/Kconfig
> @@ -74,6 +74,8 @@ config SOC_LAN966
>  	select DW_APB_TIMER_OF
>  	select ARM_GIC
>  	select MEMORY
> +	select MULTIPLEXER
> +	select MUX_LAN966
>  	help
>  	  This enables support for ARMv7 based Microchip LAN966 SoC family.
>  
> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> index 559eb4d352b6..7cfd0fc3f4f0 100644
> --- a/drivers/mfd/atmel-flexcom.c
> +++ b/drivers/mfd/atmel-flexcom.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/clk.h>
>  #include <dt-bindings/mfd/atmel-flexcom.h>
> +#include <linux/mux/consumer.h>
>  
>  /* I/O register offsets */
>  #define FLEX_MR		0x0	/* Mode Register */
> @@ -28,6 +29,10 @@
>  #define FLEX_MR_OPMODE(opmode)	(((opmode) << FLEX_MR_OPMODE_OFFSET) &	\
>  				 FLEX_MR_OPMODE_MASK)
>  
> +struct atmel_flex_caps {
> +	bool has_flx_mux;
> +};
> +
>  struct atmel_flexcom {
>  	void __iomem *base;
>  	u32 opmode;
> @@ -37,6 +42,7 @@ struct atmel_flexcom {
>  static int atmel_flexcom_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> +	const struct atmel_flex_caps *caps;
>  	struct resource *res;
>  	struct atmel_flexcom *ddata;
>  	int err;
> @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct platform_device *pdev)
>  	 */
>  	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
>  
> +	caps = of_device_get_match_data(&pdev->dev);
> +	if (!caps) {
> +		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Flexcom Mux */
> +	if (caps->has_flx_mux && of_property_read_bool(np, "mux-controls")) {
> +		struct mux_control *flx_mux;
> +		struct of_phandle_args args;
> +		int i, count;
> +
> +		flx_mux = devm_mux_control_get(&pdev->dev, NULL);
> +		if (IS_ERR(flx_mux))
> +			return PTR_ERR(flx_mux);
> +
> +		count = of_property_count_strings(np, "mux-control-names");
> +		for (i = 0; i < count; i++) {
> +			err = of_parse_phandle_with_fixed_args(np, "mux-controls", 1, i, &args);
> +			if (err)
> +				break;
> +
> +			err = mux_control_select(flx_mux, args.args[0]);
> +			if (!err) {
> +				mux_control_deselect(flx_mux);
> +			} else {
> +				dev_err(&pdev->dev, "Failed to select FLEXCOM mux\n");
> +				return err;
> +			}
> +		}
> +	}
> +

Can you move this in a separate function and take care of disabling the
clock on error cases?

>  	clk_disable_unprepare(ddata->clk);
>  
>  	return devm_of_platform_populate(&pdev->dev);
>  }
>  
> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> +
> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> +	.has_flx_mux = true,
> +};
> +
>  static const struct of_device_id atmel_flexcom_of_match[] = {
> -	{ .compatible = "atmel,sama5d2-flexcom" },
> +	{
> +		.compatible = "atmel,sama5d2-flexcom",
> +		.data = &atmel_flexcom_caps,
> +	},
> +
> +	{
> +		.compatible = "microchip,lan966-flexcom",
> +		.data = &lan966x_flexcom_caps,
> +	},
> +
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index e5c571fd232c..ea09f474bc2f 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -45,6 +45,18 @@ config MUX_GPIO
>  	  To compile the driver as a module, choose M here: the module will
>  	  be called mux-gpio.
>  
> +config MUX_LAN966
> +	tristate "LAN966 Flexcom multiplexer"
> +	depends on OF || COMPILE_TEST
> +	help
> +	Lan966 Flexcom Multiplexer controller.
> +
> +	The driver supports mapping 2 chip-selects of each of the lan966
> +	flexcoms to 21 flexcom shared pins.
> +
> +	To compile the driver as a module, choose M here: the module will
> +	be called mux-lan966.

Ussually the help message is aligned 2 spaces on right after help.

> +
>  config MUX_MMIO
>  	tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
>  	depends on OF || COMPILE_TEST
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index 6e9fa47daf56..53a9840d96fa 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -7,10 +7,12 @@ mux-core-objs			:= core.o
>  mux-adg792a-objs		:= adg792a.o
>  mux-adgs1408-objs		:= adgs1408.o
>  mux-gpio-objs			:= gpio.o
> +mux-lan966-objs			:= lan966-flx.o
>  mux-mmio-objs			:= mmio.o
>  
>  obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
>  obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
>  obj-$(CONFIG_MUX_ADGS1408)	+= mux-adgs1408.o
>  obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
> +obj-$(CONFIG_MUX_LAN966)	+= mux-lan966.o
>  obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
> diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
> new file mode 100644
> index 000000000000..2c7dab616a6a
> --- /dev/null
> +++ b/drivers/mux/lan966-flx.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LAN966 Flexcom MUX driver
> + *
> + * Copyright (c) 2022 Microchip Inc.
> + *
> + * Author: Kavyasree Kotagiri <[e-mail]>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/mux/driver.h>
> +#include <linux/io.h>
> +
> +#define FLEX_SHRD_MASK		0x1FFFFF

Or you can use GENMASK()

> +#define LAN966_MAX_CS		21
> +
> +static void __iomem *flx_shared_base;

I agree with Peter's point of view on this.

> +struct mux_lan966x {
> +	u32 offset;
> +	u32 ss_pin;
> +};
> +
> +static int mux_lan966x_set(struct mux_control *mux, int state)
> +{
> +	struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> +	u32 val;
> +
> +	val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
> +	writel(val, flx_shared_base + mux_lan966x[state].offset);

writel_relaxed should be enough here.

> +
> +	return 0;
> +}
> +
> +static const struct mux_control_ops mux_lan966x_ops = {
> +	.set = mux_lan966x_set,
> +};
> +
> +static const struct of_device_id mux_lan966x_dt_ids[] = {
> +	{ .compatible = "microchip,lan966-flx-mux", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
> +
> +static int mux_lan966x_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct mux_lan966x *mux_lan966x;
> +	struct mux_chip *mux_chip;
> +	int ret, num_fields, i;
> +
> +	ret = of_property_count_u32_elems(np, "mux-offset-pin");
> +	if (ret == 0 || ret % 2)
> +		ret = -EINVAL;
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "mux-offset-pin property missing or invalid");
> +	num_fields = ret / 2;
> +
> +	mux_chip = devm_mux_chip_alloc(dev, num_fields, sizeof(*mux_lan966x));
> +	if (IS_ERR(mux_chip))
> +		return dev_err_probe(dev, PTR_ERR(mux_chip),
> +				     "failed to allocate mux_chips\n");
> +
> +	mux_lan966x = mux_chip_priv(mux_chip);
> +
> +	flx_shared_base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> +	if (IS_ERR(flx_shared_base))
> +		return dev_err_probe(dev, PTR_ERR(flx_shared_base),
> +				     "failed to get flexcom shared base address\n");
> +
> +	for (i = 0; i < num_fields; i++) {
> +		struct mux_control *mux = &mux_chip->mux[i];
> +		u32 offset, shared_pin;
> +
> +		ret = of_property_read_u32_index(np, "mux-offset-pin",
> +						 2 * i, &offset);
> +		if (ret == 0)
> +			ret = of_property_read_u32_index(np, "mux-offset-pin",
> +							 2 * i + 1,
> +							 &shared_pin);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to read mux-offset-pin property: %d", i);
> +
> +		if (shared_pin >= LAN966_MAX_CS)
> +			return -EINVAL;
> +
> +		mux_lan966x[i].offset = offset;
> +		mux_lan966x[i].ss_pin = shared_pin;
> +
> +		mux->states = LAN966_MAX_CS;
> +	}
> +
> +	mux_chip->ops = &mux_lan966x_ops;
> +
> +	ret = devm_mux_chip_register(dev, mux_chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

you can just return ret; or return devm_mux_chip_register(dev, mux_chip);

> +}
> +
> +static struct platform_driver mux_lan966x_driver = {
> +	.driver = {
> +		.name = "lan966-mux",
> +		.of_match_table	= of_match_ptr(mux_lan966x_dt_ids),
> +	},
> +	.probe = mux_lan966x_probe,
> +};
> +
> +module_platform_driver(mux_lan966x_driver);
> +
> +MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
> +MODULE_AUTHOR("Kavyasree Kotagiri <[e-mail]>");
> +MODULE_LICENSE("GPL v2");

Last time I checked checkpatch.pl complained about using "GPL v2" string
here and suggested to use "GPL".

> +

Kavyasree Kotagiri May 11, 2022, 2:28 p.m. UTC | #8

> 2022-05-10 at 16:59, [e-mail] wrote:
> >>> LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> >>> For each chip select of each flexcom there is a configuration
> >>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> >>> configuration register is 21 because there are 21 shared pins
> >>> on each of which the chip select can be mapped. Each bit of the
> >>> register represents a different FLEXCOM_SHARED pin.
> >>>
> >>> Signed-off-by: Kavyasree Kotagiri <[e-mail]>
> >>> ---
> >>>  arch/arm/mach-at91/Kconfig  |   2 +
> >>>  drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-
> >>>  drivers/mux/Kconfig         |  12 ++++
> >>>  drivers/mux/Makefile        |   2 +
> >>>  drivers/mux/lan966-flx.c    | 121
> >> ++++++++++++++++++++++++++++++++++++
> >>>  5 files changed, 191 insertions(+), 1 deletion(-)
> >>>  create mode 100644 drivers/mux/lan966-flx.c
> >>>
> >>> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> >>> index 279810381256..26fb0f4e1b79 100644
> >>> --- a/arch/arm/mach-at91/Kconfig
> >>> +++ b/arch/arm/mach-at91/Kconfig
> >>> @@ -74,6 +74,8 @@ config SOC_LAN966
> >>>       select DW_APB_TIMER_OF
> >>>       select ARM_GIC
> >>>       select MEMORY
> >>> +     select MULTIPLEXER
> >>> +     select MUX_LAN966
> >>>       help
> >>>         This enables support for ARMv7 based Microchip LAN966 SoC
> family.
> >>>
> >>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> >>> index 559eb4d352b6..7cfd0fc3f4f0 100644
> >>> --- a/drivers/mfd/atmel-flexcom.c
> >>> +++ b/drivers/mfd/atmel-flexcom.c
> >>> @@ -17,6 +17,7 @@
> >>>  #include <linux/io.h>
> >>>  #include <linux/clk.h>
> >>>  #include <dt-bindings/mfd/atmel-flexcom.h>
> >>> +#include <linux/mux/consumer.h>
> >>>
> >>>  /* I/O register offsets */
> >>>  #define FLEX_MR              0x0     /* Mode Register */
> >>> @@ -28,6 +29,10 @@
> >>>  #define FLEX_MR_OPMODE(opmode)       (((opmode) <<
> >> FLEX_MR_OPMODE_OFFSET) &  \
> >>>                                FLEX_MR_OPMODE_MASK)
> >>>
> >>> +struct atmel_flex_caps {
> >>> +     bool has_flx_mux;
> >>> +};
> >>> +
> >>>  struct atmel_flexcom {
> >>>       void __iomem *base;
> >>>       u32 opmode;
> >>> @@ -37,6 +42,7 @@ struct atmel_flexcom {
> >>>  static int atmel_flexcom_probe(struct platform_device *pdev)
> >>>  {
> >>>       struct device_node *np = pdev->dev.of_node;
> >>> +     const struct atmel_flex_caps *caps;
> >>>       struct resource *res;
> >>>       struct atmel_flexcom *ddata;
> >>>       int err;
> >>> @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct
> >> platform_device *pdev)
> >>>        */
> >>>       writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base +
> FLEX_MR);
> >>>
> >>> +     caps = of_device_get_match_data(&pdev->dev);
> >>> +     if (!caps) {
> >>> +             dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> >>> +             return -EINVAL;
> >>> +     }
> >>> +
> >>> +     /* Flexcom Mux */
> >>> +     if (caps->has_flx_mux && of_property_read_bool(np, "mux-
> controls"))
> >> {
> >>> +             struct mux_control *flx_mux;
> >>> +             struct of_phandle_args args;
> >>> +             int i, count;
> >>> +
> >>> +             flx_mux = devm_mux_control_get(&pdev->dev, NULL);
> >>> +             if (IS_ERR(flx_mux))
> >>> +                     return PTR_ERR(flx_mux);
> >>> +
> >>> +             count = of_property_count_strings(np, "mux-control-names");
> >>> +             for (i = 0; i < count; i++) {
> >>> +                     err = of_parse_phandle_with_fixed_args(np, "mux-
> controls",
> >> 1, i, &args);
> >>> +                     if (err)
> >>> +                             break;
> >>> +
> >>> +                     err = mux_control_select(flx_mux, args.args[0]);
> >>> +                     if (!err) {
> >>> +                             mux_control_deselect(flx_mux);
> >>
> >> This is suspect. When you deselect the mux you rely on the mux to be
> >> configured with "as-is" as the idle state. But you do not document that.
> >> This is also fragile in that you silently rely on noone else selecting
> >> the mux to some unwanted state behind your back (mux controls are not
> >> exclusive the way e.g. gpio pins or pwms are). The protocol is that
> >> others may get a ref to the mux control and select it as long as noone
> >> else has selected it.
> >>
> >> The only sane thing to do is to keep the mux selected for the whole
> >> duration when you rely on it to be in your desired state.
> >>
> >
> > Yes, mux is kept selected until configuring register is done. Please see
> below log where
> > I added debug prints just for understanding:
> > # dmesg | grep KK
> >  [    0.779827] KK: Func: atmel_flexcom_probe ***** [START flx muxing]
> ********
> > [    0.779875] KK: Func: atmel_flexcom_probe i = 0 args[0] = 0
> > [    0.779890] KK: Func: mux_control_select [Entered]
> > [    0.779902] KK: Func: mux_lan966x_set [Entered] state = 0
> > [    0.779977] KK: Func: mux_lan966x_set [Read] = 0x1fffef   <<<----- setting
> mux_lan966x[state].ss_pin "4" which is passed from dts
> > [    0.779992] KK: Func: mux_lan966x_set [Exit]
> > [    0.780002] KK: Func: mux_control_select [Exit]
> > [    0.780011] KK: Func: mux_control_deselect [Entered]
> > [    0.780021] KK: Func: mux_control_deselect [Exit]
> 
> You misunderstand. The mux control is only "selected" between the call
> to mux_control_select() and the following call to
> mux_control_deselect().
> 
> After that, the mux control is "idle". However, in your case the
> "idle-state" is configured to "as-is" (which is the default), so the
> mux control (naturally) remains in the previously selected state while
> idle. But you are not documenting that limitation, and with this
> implementation you *must* have a mux control that uses "as-is" as its
> idle state. But that is an unexpected and broken limitation, and a
> much better solution is to simply keep the mux control "selected" for
> the complete duration when you rely on it to be in whatever state you
> need it to be in.
> 
I am new to mux drivers.
Let me try to explain why I have mux_control_deselect if there is no err from mux_control_select()
For example, 
1. When I have one only chip-select CS0 for flexcom3 to be mapped to flexcom shared pin 9:
As per previously shared log, FLEXCOM_SHARED[3]:SS_MASK[0] is being configured to expected value 
even before mux_control_deseletc().
2. When I have to map two chip-selects of flx3 - CS0 to flexcom shared 9
						CS1 to flexcom shared pin 7
FLEXCOM_SHARED[3]:SS_MASK[0] is set to expected value 0x1fffef
I see console hangs at mux_control_select() if I don’t call mux_control_deselect 
while mapping second chip-select FLEXCOM_SHARED[3]:SS_MASK[1].
After reading below description from mux_control_select() :
" * On successfully selecting the mux-control state, it will be locked until
 * there is a call to mux_control_deselect()."
Following this help text, I called mux_control_deselect() if there is no error from mux_control_select() 
and then it worked. FLEXCOM_SHARED[3]:SS_MASK[1] is now set to expected value 0x1fffbf.

Please explain me if I am missing something.

> >>> +                     } else {
> >>> +                             dev_err(&pdev->dev, "Failed to select FLEXCOM
> mux\n");
> >>> +                             return err;
> >>> +                     }
> >>> +             }
> >>> +     }
> >>> +
> >>>       clk_disable_unprepare(ddata->clk);
> >>>
> >>>       return devm_of_platform_populate(&pdev->dev);
> >>>  }
> >>>
> >>> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> >>> +
> >>> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> >>> +     .has_flx_mux = true,
> >>> +};
> >>> +
> >>>  static const struct of_device_id atmel_flexcom_of_match[] = {
> >>> -     { .compatible = "atmel,sama5d2-flexcom" },
> >>> +     {
> >>> +             .compatible = "atmel,sama5d2-flexcom",
> >>> +             .data = &atmel_flexcom_caps,
> >>> +     },
> >>> +
> >>> +     {
> >>> +             .compatible = "microchip,lan966-flexcom",
> >>> +             .data = &lan966x_flexcom_caps,
> >>> +     },
> >>> +
> >>>       { /* sentinel */ }
> >>>  };
> >>>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> >>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> >>> index e5c571fd232c..ea09f474bc2f 100644
> >>> --- a/drivers/mux/Kconfig
> >>> +++ b/drivers/mux/Kconfig
> >>> @@ -45,6 +45,18 @@ config MUX_GPIO
> >>>         To compile the driver as a module, choose M here: the module will
> >>>         be called mux-gpio.
> >>>
> >>> +config MUX_LAN966
> >>> +     tristate "LAN966 Flexcom multiplexer"
> >>> +     depends on OF || COMPILE_TEST
> >>> +     help
> >>> +     Lan966 Flexcom Multiplexer controller.
> >>> +
> >>> +     The driver supports mapping 2 chip-selects of each of the lan966
> >>> +     flexcoms to 21 flexcom shared pins.
> >>> +
> >>> +     To compile the driver as a module, choose M here: the module will
> >>> +     be called mux-lan966.
> >>> +
> >>>  config MUX_MMIO
> >>>       tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
> >>>       depends on OF || COMPILE_TEST
> >>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> >>> index 6e9fa47daf56..53a9840d96fa 100644
> >>> --- a/drivers/mux/Makefile
> >>> +++ b/drivers/mux/Makefile
> >>> @@ -7,10 +7,12 @@ mux-core-objs                       := core.o
> >>>  mux-adg792a-objs             := adg792a.o
> >>>  mux-adgs1408-objs            := adgs1408.o
> >>>  mux-gpio-objs                        := gpio.o
> >>> +mux-lan966-objs                      := lan966-flx.o
> >>>  mux-mmio-objs                        := mmio.o
> >>>
> >>>  obj-$(CONFIG_MULTIPLEXER)    += mux-core.o
> >>>  obj-$(CONFIG_MUX_ADG792A)    += mux-adg792a.o
> >>>  obj-$(CONFIG_MUX_ADGS1408)   += mux-adgs1408.o
> >>>  obj-$(CONFIG_MUX_GPIO)               += mux-gpio.o
> >>> +obj-$(CONFIG_MUX_LAN966)     += mux-lan966.o
> >>>  obj-$(CONFIG_MUX_MMIO)               += mux-mmio.o
> >>> diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
> >>> new file mode 100644
> >>> index 000000000000..2c7dab616a6a
> >>> --- /dev/null
> >>> +++ b/drivers/mux/lan966-flx.c
> >>> @@ -0,0 +1,121 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * LAN966 Flexcom MUX driver
> >>> + *
> >>> + * Copyright (c) 2022 Microchip Inc.
> >>> + *
> >>> + * Author: Kavyasree Kotagiri <[e-mail]>
> >>
> >> Looks like it is based on mmio.c?
> >>
> > Yes, I took mmio.c  driver as reference.
> >
> 
> So, then the above copyright and authorship info is not complete.
> 
> >>> + */
> >>> +
> >>> +#include <linux/err.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/of_platform.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/property.h>
> >>> +#include <linux/mux/driver.h>
> >>> +#include <linux/io.h>
> >>> +
> >>> +#define FLEX_SHRD_MASK               0x1FFFFF
> >>> +#define LAN966_MAX_CS                21
> >>> +
> >>> +static void __iomem *flx_shared_base;
> >>
> >> I would much prefer to store the combined address (base+offset)
> >> in the mux private data instead of only storing the offset and
> >> then unnecessarily rely on some piece of global state (that
> >> will be clobbered by other instances).
> >>
> > Ok. I will try to see if this is relevant and change accordingly.
> >
> >>> +struct mux_lan966x {
> >>
> >> Why is the file named lan966, but then everything inside lan966x?
> >>
> >>> +     u32 offset;
> >>> +     u32 ss_pin;
> >>> +};
> >>> +
> >>> +static int mux_lan966x_set(struct mux_control *mux, int state)
> >>> +{
> >>> +     struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> >>> +     u32 val;
> >>> +
> >>> +     val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
> >>> +     writel(val, flx_shared_base + mux_lan966x[state].offset);
> >>
> >> This reads memory you have not allocated, if you select a state
> >> other than zero.
> >>
> > Ok. I will return error condition in case of trying to access none existing
> entry.
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static const struct mux_control_ops mux_lan966x_ops = {
> >>> +     .set = mux_lan966x_set,
> >>> +};
> >>> +
> >>> +static const struct of_device_id mux_lan966x_dt_ids[] = {
> >>> +     { .compatible = "microchip,lan966-flx-mux", },
> >>> +     { /* sentinel */ }
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
> >>> +
> >>> +static int mux_lan966x_probe(struct platform_device *pdev)
> >>> +{
> >>> +     struct device_node *np = pdev->dev.of_node;
> >>> +     struct device *dev = &pdev->dev;
> >>> +     struct mux_lan966x *mux_lan966x;
> >>> +     struct mux_chip *mux_chip;
> >>> +     int ret, num_fields, i;
> >>> +
> >>> +     ret = of_property_count_u32_elems(np, "mux-offset-pin");
> >>> +     if (ret == 0 || ret % 2)
> >>> +             ret = -EINVAL;
> >>> +     if (ret < 0)
> >>> +             return dev_err_probe(dev, ret,
> >>> +                                  "mux-offset-pin property missing or invalid");
> >>> +     num_fields = ret / 2;
> >>> +
> >>> +     mux_chip = devm_mux_chip_alloc(dev, num_fields,
> >> sizeof(*mux_lan966x));
> >>
> >> I might be thoroughly mistaken and confused by the code, but it seems
> >> very strange that a subsequenct select is not undoing what a previous
> >> select once did. Each state seems to write to its own register offset,
> >> and there is nothing that restores the first register offset with you
> >> switch states.
> >>
> >> Care to explain how muxing works and what you are expected to do to
> >> manipulate the mux? Is there some link to public documentation? I did
> >> a quick search for lan966 but came up with nothing relevant.
> >>
> > LAN966 has 5 flexcoms(which can be used as USART/SPI/I2C interface).
> > FLEXCOM has two chip-select I/O lines namely CS0 and CS1
> > in SPI mode, CTS and RTS in USART mode. These FLEXCOM pins are
> optional.
> > These chip-selects can be mapped to flexcom shared pin [0-21] which can
> be
> > done by configuring flexcom multiplexer register(FLEXCOM_SHARED[0-
> 4]:SS_MASK[0-1])
> > Driver explanation:
> > "flx_shared_base" is used to get base address of Flexcom shared
> multiplexer
> > "mux-offset-pin" property is used to get cs0/cs1 offset and flexcom shared
> pin[0-21] of a flexcom.
> > state value passed from atmel-flexcom is used to configure respective
> > FLEXCOM_SHARED[0-4]:SS_MASK[0-1] register with offset and flexcom
> shared pin.
> 
> Ok, let me try to interpret that. You wish enable a "fan out" of these
> two chip-selects for any of the 5 flexcoms (in SPI mode?) such that
> these 10 internal chip-selects can be connected to any of 21 external
> pins?
> 
> If that's correct and if you wish to interface with e.g. 20 chips,
> then it would be possible to have 20 states for one mux control and
> then reach the 20 chips using only CS0 from FLEXCOM0, or it would be
> possible to have 2 states for 10 mux controls, one each for CS0/CS1 of
> all five flexcoms and also reach 20 chips. Or any wild combo you
> imagine is useful.
> 
> Is that correctly understood?
> 
> Assuming so, then you can have a maximum of 10 mux controls, and for
> each mux control you need a variable number of legitimate states. Each
> mux control would also need to know at what address/offset its SS_MASK
> register is at and what pins/states are valid.
> 
> But your code does not implemnent the above. You allocate num_fields
> mux controls, which is what confuses the hell out of me. num_fields is
> the number of states, not the number of mux controls! And you also
> need to specify an individual offset for each state, and that makes no
> sense at all, at least not to me.
> 
> So, instead, I think you want to have:
> 
> struct mux_lan966x {
>         u32 ss_mask;
>         int num_pins;
>         u32 ss_pin[];
> };
> 
> And then do:
> 
>         mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_lan966x) +
> num_pins * sizeof(u32));
> 
> (or however that size is best spelled, I haven't kept up)
> 
> The set operation would be along the lines of:
>
> static int mux_lan966x_set(struct mux_control *mux, int state)
> {
>         struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
>         u32 val;
> 
>         if (state < 0 || state >= mux_lan966x->num_pins)
>                 return -1;
> 
>         val = ~(1 << mux_lan966x->ss_pin[state]) & FLEX_SHRD_MASK;
>         writel(val, flx_shared_base + mux_lan966x->ss_mask);
> 
>         return 0;
> }
> 
> Because, one mux control should only ever need to know about one offset,
> as it should only ever write to its own SS_MASK register. And you will
> need some private space such that you can keep track of which states
> are legit.
> 
> I would also separate out the SS_MASK offset from the mux-offset-pin
> property and have one property for that value and then a straight
> array for the valid pin numbers in another property (no longer named
> mux-offset-pin of course).
> 
> Or something like that and assuming I understand how the FLEXCOMs work
> and what you want to do etc.
> 

Thank you for your comments
I agree, I will change number of mux controls to 1 in devm_mux_chip_alloc()
I would like to use mux-offset-pin property because 
each chip-select must be mapped to a unique flexcom shared pin.
For example, 
mux-offset-pin = <0x18 9>, /* 0: flexcom3 CS0 mapped to pin-9 */
                               <0x1c 7>, /* 1: flexcom3 CS1 mapped to pin-7 */
                               <0x20 4>; /* 2: flexcom4 CS0 mapped to pin-4 */};
I want to use mux-offset-pin property to be clear about which offset is mapped to which
flexcom shared pin. Here, 0, 1, 2.. entries represents state passed from mux_control_select(mux, state).

Please provide your comments.

> Cheers,
> Peter
> 
> 
> >>> +     if (IS_ERR(mux_chip))
> >>> +             return dev_err_probe(dev, PTR_ERR(mux_chip),
> >>> +                                  "failed to allocate mux_chips\n");
> >>> +
> >>> +     mux_lan966x = mux_chip_priv(mux_chip);
> >>> +
> >>> +     flx_shared_base =
> devm_platform_get_and_ioremap_resource(pdev,
> >> 0, NULL);
> >>> +     if (IS_ERR(flx_shared_base))
> >>> +             return dev_err_probe(dev, PTR_ERR(flx_shared_base),
> >>> +                                  "failed to get flexcom shared base address\n");
> >>> +
> >>> +     for (i = 0; i < num_fields; i++) {
> >>> +             struct mux_control *mux = &mux_chip->mux[i];
> >>> +             u32 offset, shared_pin;
> >>> +
> >>> +             ret = of_property_read_u32_index(np, "mux-offset-pin",
> >>> +                                              2 * i, &offset);
> >>> +             if (ret == 0)
> >>> +                     ret = of_property_read_u32_index(np, "mux-offset-pin",
> >>> +                                                      2 * i + 1,
> >>> +                                                      &shared_pin);
> >>> +             if (ret < 0)
> >>> +                     return dev_err_probe(dev, ret,
> >>> +                                          "failed to read mux-offset-pin property: %d", i);
> >>> +
> >>> +             if (shared_pin >= LAN966_MAX_CS)
> >>> +                     return -EINVAL;
> >>> +
> >>> +             mux_lan966x[i].offset = offset;
> >>> +             mux_lan966x[i].ss_pin = shared_pin;
> >>
> >> This clobbers memory you have not allocated, if num_fields >= 1.
> >>
> >> Cheers,
> >> Peter
> >>
> >>> +
> >>> +             mux->states = LAN966_MAX_CS;
> >>> +     }
> >>> +
> >>> +     mux_chip->ops = &mux_lan966x_ops;
> >>> +
> >>> +     ret = devm_mux_chip_register(dev, mux_chip);
> >>> +     if (ret < 0)
> >>> +             return ret;
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static struct platform_driver mux_lan966x_driver = {
> >>> +     .driver = {
> >>> +             .name = "lan966-mux",
> >>> +             .of_match_table = of_match_ptr(mux_lan966x_dt_ids),
> >>> +     },
> >>> +     .probe = mux_lan966x_probe,
> >>> +};
> >>> +
> >>> +module_platform_driver(mux_lan966x_driver);
> >>> +
> >>> +MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
> >>> +MODULE_AUTHOR("Kavyasree Kotagiri
> >> <[e-mail]>");
> >>> +MODULE_LICENSE("GPL v2");
> >>> +

Peter Rosin May 11, 2022, 3:43 p.m. UTC | #9

Hi!

2022-05-11 at 16:28, [e-mail] wrote:
>> 2022-05-10 at 16:59, [e-mail] wrote:
>>>>> LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
>>>>> For each chip select of each flexcom there is a configuration
>>>>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
>>>>> configuration register is 21 because there are 21 shared pins
>>>>> on each of which the chip select can be mapped. Each bit of the
>>>>> register represents a different FLEXCOM_SHARED pin.
>>>>>
>>>>> Signed-off-by: Kavyasree Kotagiri <[e-mail]>
>>>>> ---
>>>>>  arch/arm/mach-at91/Kconfig  |   2 +
>>>>>  drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-
>>>>>  drivers/mux/Kconfig         |  12 ++++
>>>>>  drivers/mux/Makefile        |   2 +
>>>>>  drivers/mux/lan966-flx.c    | 121
>>>> ++++++++++++++++++++++++++++++++++++
>>>>>  5 files changed, 191 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 drivers/mux/lan966-flx.c
>>>>>
>>>>> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
>>>>> index 279810381256..26fb0f4e1b79 100644
>>>>> --- a/arch/arm/mach-at91/Kconfig
>>>>> +++ b/arch/arm/mach-at91/Kconfig
>>>>> @@ -74,6 +74,8 @@ config SOC_LAN966
>>>>>       select DW_APB_TIMER_OF
>>>>>       select ARM_GIC
>>>>>       select MEMORY
>>>>> +     select MULTIPLEXER
>>>>> +     select MUX_LAN966
>>>>>       help
>>>>>         This enables support for ARMv7 based Microchip LAN966 SoC
>> family.
>>>>>
>>>>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
>>>>> index 559eb4d352b6..7cfd0fc3f4f0 100644
>>>>> --- a/drivers/mfd/atmel-flexcom.c
>>>>> +++ b/drivers/mfd/atmel-flexcom.c
>>>>> @@ -17,6 +17,7 @@
>>>>>  #include <linux/io.h>
>>>>>  #include <linux/clk.h>
>>>>>  #include <dt-bindings/mfd/atmel-flexcom.h>
>>>>> +#include <linux/mux/consumer.h>
>>>>>
>>>>>  /* I/O register offsets */
>>>>>  #define FLEX_MR              0x0     /* Mode Register */
>>>>> @@ -28,6 +29,10 @@
>>>>>  #define FLEX_MR_OPMODE(opmode)       (((opmode) <<
>>>> FLEX_MR_OPMODE_OFFSET) &  \
>>>>>                                FLEX_MR_OPMODE_MASK)
>>>>>
>>>>> +struct atmel_flex_caps {
>>>>> +     bool has_flx_mux;
>>>>> +};
>>>>> +
>>>>>  struct atmel_flexcom {
>>>>>       void __iomem *base;
>>>>>       u32 opmode;
>>>>> @@ -37,6 +42,7 @@ struct atmel_flexcom {
>>>>>  static int atmel_flexcom_probe(struct platform_device *pdev)
>>>>>  {
>>>>>       struct device_node *np = pdev->dev.of_node;
>>>>> +     const struct atmel_flex_caps *caps;
>>>>>       struct resource *res;
>>>>>       struct atmel_flexcom *ddata;
>>>>>       int err;
>>>>> @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct
>>>> platform_device *pdev)
>>>>>        */
>>>>>       writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base +
>> FLEX_MR);
>>>>>
>>>>> +     caps = of_device_get_match_data(&pdev->dev);
>>>>> +     if (!caps) {
>>>>> +             dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
>>>>> +             return -EINVAL;
>>>>> +     }
>>>>> +
>>>>> +     /* Flexcom Mux */
>>>>> +     if (caps->has_flx_mux && of_property_read_bool(np, "mux-
>> controls"))
>>>> {
>>>>> +             struct mux_control *flx_mux;
>>>>> +             struct of_phandle_args args;
>>>>> +             int i, count;
>>>>> +
>>>>> +             flx_mux = devm_mux_control_get(&pdev->dev, NULL);
>>>>> +             if (IS_ERR(flx_mux))
>>>>> +                     return PTR_ERR(flx_mux);
>>>>> +
>>>>> +             count = of_property_count_strings(np, "mux-control-names");
>>>>> +             for (i = 0; i < count; i++) {
>>>>> +                     err = of_parse_phandle_with_fixed_args(np, "mux-
>> controls",
>>>> 1, i, &args);
>>>>> +                     if (err)
>>>>> +                             break;
>>>>> +
>>>>> +                     err = mux_control_select(flx_mux, args.args[0]);
>>>>> +                     if (!err) {
>>>>> +                             mux_control_deselect(flx_mux);
>>>>
>>>> This is suspect. When you deselect the mux you rely on the mux to be
>>>> configured with "as-is" as the idle state. But you do not document that.
>>>> This is also fragile in that you silently rely on noone else selecting
>>>> the mux to some unwanted state behind your back (mux controls are not
>>>> exclusive the way e.g. gpio pins or pwms are). The protocol is that
>>>> others may get a ref to the mux control and select it as long as noone
>>>> else has selected it.
>>>>
>>>> The only sane thing to do is to keep the mux selected for the whole
>>>> duration when you rely on it to be in your desired state.
>>>>
>>>
>>> Yes, mux is kept selected until configuring register is done. Please see
>> below log where
>>> I added debug prints just for understanding:
>>> # dmesg | grep KK
>>>  [    0.779827] KK: Func: atmel_flexcom_probe ***** [START flx muxing]
>> ********
>>> [    0.779875] KK: Func: atmel_flexcom_probe i = 0 args[0] = 0
>>> [    0.779890] KK: Func: mux_control_select [Entered]
>>> [    0.779902] KK: Func: mux_lan966x_set [Entered] state = 0
>>> [    0.779977] KK: Func: mux_lan966x_set [Read] = 0x1fffef   <<<----- setting
>> mux_lan966x[state].ss_pin "4" which is passed from dts
>>> [    0.779992] KK: Func: mux_lan966x_set [Exit]
>>> [    0.780002] KK: Func: mux_control_select [Exit]
>>> [    0.780011] KK: Func: mux_control_deselect [Entered]
>>> [    0.780021] KK: Func: mux_control_deselect [Exit]
>>
>> You misunderstand. The mux control is only "selected" between the call
>> to mux_control_select() and the following call to
>> mux_control_deselect().
>>
>> After that, the mux control is "idle". However, in your case the
>> "idle-state" is configured to "as-is" (which is the default), so the
>> mux control (naturally) remains in the previously selected state while
>> idle. But you are not documenting that limitation, and with this
>> implementation you *must* have a mux control that uses "as-is" as its
>> idle state. But that is an unexpected and broken limitation, and a
>> much better solution is to simply keep the mux control "selected" for
>> the complete duration when you rely on it to be in whatever state you
>> need it to be in.
>>
> I am new to mux drivers.
> Let me try to explain why I have mux_control_deselect if there is no err from mux_control_select()
> For example, 
> 1. When I have one only chip-select CS0 for flexcom3 to be mapped to flexcom shared pin 9:
> As per previously shared log, FLEXCOM_SHARED[3]:SS_MASK[0] is being configured to expected value 
> even before mux_control_deseletc().
> 2. When I have to map two chip-selects of flx3 - CS0 to flexcom shared 9
> 						CS1 to flexcom shared pin 7
> FLEXCOM_SHARED[3]:SS_MASK[0] is set to expected value 0x1fffef
> I see console hangs at mux_control_select() if I don’t call mux_control_deselect 
> while mapping second chip-select FLEXCOM_SHARED[3]:SS_MASK[1].
> After reading below description from mux_control_select() :
> " * On successfully selecting the mux-control state, it will be locked until
>  * there is a call to mux_control_deselect()."
> Following this help text, I called mux_control_deselect() if there is no error from mux_control_select() 
> and then it worked. FLEXCOM_SHARED[3]:SS_MASK[1] is now set to expected value 0x1fffbf.
> 
> Please explain me if I am missing something.

You should wait with the deselect until you need to change the state
of the mux. I.e., on init/probe you set some variable to -1, like so:

	priv->mux_error = -1;

and when you later need to set/change the state of the mux, you do:

	if (!priv->mux_error)
		mux_control_deselect(...);
	priv->mux_error = mux_control_select(...);

and then you cleanup at the end of life:

	if (!priv->mux_error)
		mux_control_deselect(...);

Or something like that.

The mux api is obviously not a good fit for the use case of long
lived selects like you appear to have here. The api was designed
for short lived selects, along the lines of:

int
so_something(...)
{
	int err;

	err = mux_control_select(...);
	if (err)
		return err;

	do_it(...);

	mux_control_deselect(...);
}


> 
>>>>> +                     } else {
>>>>> +                             dev_err(&pdev->dev, "Failed to select FLEXCOM
>> mux\n");
>>>>> +                             return err;
>>>>> +                     }
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>>       clk_disable_unprepare(ddata->clk);
>>>>>
>>>>>       return devm_of_platform_populate(&pdev->dev);
>>>>>  }
>>>>>
>>>>> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
>>>>> +
>>>>> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
>>>>> +     .has_flx_mux = true,
>>>>> +};
>>>>> +
>>>>>  static const struct of_device_id atmel_flexcom_of_match[] = {
>>>>> -     { .compatible = "atmel,sama5d2-flexcom" },
>>>>> +     {
>>>>> +             .compatible = "atmel,sama5d2-flexcom",
>>>>> +             .data = &atmel_flexcom_caps,
>>>>> +     },
>>>>> +
>>>>> +     {
>>>>> +             .compatible = "microchip,lan966-flexcom",
>>>>> +             .data = &lan966x_flexcom_caps,
>>>>> +     },
>>>>> +
>>>>>       { /* sentinel */ }
>>>>>  };
>>>>>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
>>>>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>>>>> index e5c571fd232c..ea09f474bc2f 100644
>>>>> --- a/drivers/mux/Kconfig
>>>>> +++ b/drivers/mux/Kconfig
>>>>> @@ -45,6 +45,18 @@ config MUX_GPIO
>>>>>         To compile the driver as a module, choose M here: the module will
>>>>>         be called mux-gpio.
>>>>>
>>>>> +config MUX_LAN966
>>>>> +     tristate "LAN966 Flexcom multiplexer"
>>>>> +     depends on OF || COMPILE_TEST
>>>>> +     help
>>>>> +     Lan966 Flexcom Multiplexer controller.
>>>>> +
>>>>> +     The driver supports mapping 2 chip-selects of each of the lan966
>>>>> +     flexcoms to 21 flexcom shared pins.
>>>>> +
>>>>> +     To compile the driver as a module, choose M here: the module will
>>>>> +     be called mux-lan966.
>>>>> +
>>>>>  config MUX_MMIO
>>>>>       tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
>>>>>       depends on OF || COMPILE_TEST
>>>>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
>>>>> index 6e9fa47daf56..53a9840d96fa 100644
>>>>> --- a/drivers/mux/Makefile
>>>>> +++ b/drivers/mux/Makefile
>>>>> @@ -7,10 +7,12 @@ mux-core-objs                       := core.o
>>>>>  mux-adg792a-objs             := adg792a.o
>>>>>  mux-adgs1408-objs            := adgs1408.o
>>>>>  mux-gpio-objs                        := gpio.o
>>>>> +mux-lan966-objs                      := lan966-flx.o
>>>>>  mux-mmio-objs                        := mmio.o
>>>>>
>>>>>  obj-$(CONFIG_MULTIPLEXER)    += mux-core.o
>>>>>  obj-$(CONFIG_MUX_ADG792A)    += mux-adg792a.o
>>>>>  obj-$(CONFIG_MUX_ADGS1408)   += mux-adgs1408.o
>>>>>  obj-$(CONFIG_MUX_GPIO)               += mux-gpio.o
>>>>> +obj-$(CONFIG_MUX_LAN966)     += mux-lan966.o
>>>>>  obj-$(CONFIG_MUX_MMIO)               += mux-mmio.o
>>>>> diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
>>>>> new file mode 100644
>>>>> index 000000000000..2c7dab616a6a
>>>>> --- /dev/null
>>>>> +++ b/drivers/mux/lan966-flx.c
>>>>> @@ -0,0 +1,121 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * LAN966 Flexcom MUX driver
>>>>> + *
>>>>> + * Copyright (c) 2022 Microchip Inc.
>>>>> + *
>>>>> + * Author: Kavyasree Kotagiri <[e-mail]>
>>>>
>>>> Looks like it is based on mmio.c?
>>>>
>>> Yes, I took mmio.c  driver as reference.
>>>
>>
>> So, then the above copyright and authorship info is not complete.
>>
>>>>> + */
>>>>> +
>>>>> +#include <linux/err.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/of_platform.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/property.h>
>>>>> +#include <linux/mux/driver.h>
>>>>> +#include <linux/io.h>
>>>>> +
>>>>> +#define FLEX_SHRD_MASK               0x1FFFFF
>>>>> +#define LAN966_MAX_CS                21
>>>>> +
>>>>> +static void __iomem *flx_shared_base;
>>>>
>>>> I would much prefer to store the combined address (base+offset)
>>>> in the mux private data instead of only storing the offset and
>>>> then unnecessarily rely on some piece of global state (that
>>>> will be clobbered by other instances).
>>>>
>>> Ok. I will try to see if this is relevant and change accordingly.
>>>
>>>>> +struct mux_lan966x {
>>>>
>>>> Why is the file named lan966, but then everything inside lan966x?
>>>>
>>>>> +     u32 offset;
>>>>> +     u32 ss_pin;
>>>>> +};
>>>>> +
>>>>> +static int mux_lan966x_set(struct mux_control *mux, int state)
>>>>> +{
>>>>> +     struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
>>>>> +     u32 val;
>>>>> +
>>>>> +     val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
>>>>> +     writel(val, flx_shared_base + mux_lan966x[state].offset);
>>>>
>>>> This reads memory you have not allocated, if you select a state
>>>> other than zero.
>>>>
>>> Ok. I will return error condition in case of trying to access none existing
>> entry.
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct mux_control_ops mux_lan966x_ops = {
>>>>> +     .set = mux_lan966x_set,
>>>>> +};
>>>>> +
>>>>> +static const struct of_device_id mux_lan966x_dt_ids[] = {
>>>>> +     { .compatible = "microchip,lan966-flx-mux", },
>>>>> +     { /* sentinel */ }
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
>>>>> +
>>>>> +static int mux_lan966x_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +     struct device_node *np = pdev->dev.of_node;
>>>>> +     struct device *dev = &pdev->dev;
>>>>> +     struct mux_lan966x *mux_lan966x;
>>>>> +     struct mux_chip *mux_chip;
>>>>> +     int ret, num_fields, i;
>>>>> +
>>>>> +     ret = of_property_count_u32_elems(np, "mux-offset-pin");
>>>>> +     if (ret == 0 || ret % 2)
>>>>> +             ret = -EINVAL;
>>>>> +     if (ret < 0)
>>>>> +             return dev_err_probe(dev, ret,
>>>>> +                                  "mux-offset-pin property missing or invalid");
>>>>> +     num_fields = ret / 2;
>>>>> +
>>>>> +     mux_chip = devm_mux_chip_alloc(dev, num_fields,
>>>> sizeof(*mux_lan966x));
>>>>
>>>> I might be thoroughly mistaken and confused by the code, but it seems
>>>> very strange that a subsequenct select is not undoing what a previous
>>>> select once did. Each state seems to write to its own register offset,
>>>> and there is nothing that restores the first register offset with you
>>>> switch states.
>>>>
>>>> Care to explain how muxing works and what you are expected to do to
>>>> manipulate the mux? Is there some link to public documentation? I did
>>>> a quick search for lan966 but came up with nothing relevant.
>>>>
>>> LAN966 has 5 flexcoms(which can be used as USART/SPI/I2C interface).
>>> FLEXCOM has two chip-select I/O lines namely CS0 and CS1
>>> in SPI mode, CTS and RTS in USART mode. These FLEXCOM pins are
>> optional.
>>> These chip-selects can be mapped to flexcom shared pin [0-21] which can
>> be
>>> done by configuring flexcom multiplexer register(FLEXCOM_SHARED[0-
>> 4]:SS_MASK[0-1])
>>> Driver explanation:
>>> "flx_shared_base" is used to get base address of Flexcom shared
>> multiplexer
>>> "mux-offset-pin" property is used to get cs0/cs1 offset and flexcom shared
>> pin[0-21] of a flexcom.
>>> state value passed from atmel-flexcom is used to configure respective
>>> FLEXCOM_SHARED[0-4]:SS_MASK[0-1] register with offset and flexcom
>> shared pin.
>>
>> Ok, let me try to interpret that. You wish enable a "fan out" of these
>> two chip-selects for any of the 5 flexcoms (in SPI mode?) such that
>> these 10 internal chip-selects can be connected to any of 21 external
>> pins?
>>
>> If that's correct and if you wish to interface with e.g. 20 chips,
>> then it would be possible to have 20 states for one mux control and
>> then reach the 20 chips using only CS0 from FLEXCOM0, or it would be
>> possible to have 2 states for 10 mux controls, one each for CS0/CS1 of
>> all five flexcoms and also reach 20 chips. Or any wild combo you
>> imagine is useful.
>>
>> Is that correctly understood?
>>
>> Assuming so, then you can have a maximum of 10 mux controls, and for
>> each mux control you need a variable number of legitimate states. Each
>> mux control would also need to know at what address/offset its SS_MASK
>> register is at and what pins/states are valid.
>>
>> But your code does not implemnent the above. You allocate num_fields
>> mux controls, which is what confuses the hell out of me. num_fields is
>> the number of states, not the number of mux controls! And you also
>> need to specify an individual offset for each state, and that makes no
>> sense at all, at least not to me.
>>
>> So, instead, I think you want to have:
>>
>> struct mux_lan966x {
>>         u32 ss_mask;
>>         int num_pins;
>>         u32 ss_pin[];
>> };
>>
>> And then do:
>>
>>         mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_lan966x) +
>> num_pins * sizeof(u32));
>>
>> (or however that size is best spelled, I haven't kept up)
>>
>> The set operation would be along the lines of:
>>
>> static int mux_lan966x_set(struct mux_control *mux, int state)
>> {
>>         struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
>>         u32 val;
>>
>>         if (state < 0 || state >= mux_lan966x->num_pins)
>>                 return -1;
>>
>>         val = ~(1 << mux_lan966x->ss_pin[state]) & FLEX_SHRD_MASK;
>>         writel(val, flx_shared_base + mux_lan966x->ss_mask);
>>
>>         return 0;
>> }
>>
>> Because, one mux control should only ever need to know about one offset,
>> as it should only ever write to its own SS_MASK register. And you will
>> need some private space such that you can keep track of which states
>> are legit.
>>
>> I would also separate out the SS_MASK offset from the mux-offset-pin
>> property and have one property for that value and then a straight
>> array for the valid pin numbers in another property (no longer named
>> mux-offset-pin of course).
>>
>> Or something like that and assuming I understand how the FLEXCOMs work
>> and what you want to do etc.
>>
> 
> Thank you for your comments
> I agree, I will change number of mux controls to 1 in devm_mux_chip_alloc()
> I would like to use mux-offset-pin property because 
> each chip-select must be mapped to a unique flexcom shared pin.
> For example, 
> mux-offset-pin = <0x18 9>, /* 0: flexcom3 CS0 mapped to pin-9 */
>                                <0x1c 7>, /* 1: flexcom3 CS1 mapped to pin-7 */
>                                <0x20 4>; /* 2: flexcom4 CS0 mapped to pin-4 */};
> I want to use mux-offset-pin property to be clear about which offset is mapped to which
> flexcom shared pin. Here, 0, 1, 2.. entries represents state passed from mux_control_select(mux, state).
> 
> Please provide your comments.

It seems you want to just use the static info from the devicetree
to program your registers once and for all? That's not a problem
that a mux driver is aming at solving. My thought was that if you
have one SPI device with chip select on pin-7, another with chip-
select on pin-9 and a third on pin-4, then you /could/ use them all
from e.g. flexcom3/CS0 by reprogramming that SS_MASK register
at the time you want to access one of the three chips. You could
then of course not access the three chips in parallel, but the
muxing of the accesses to three chips like that is the problem
that the mux subsystem helps with.

I.e. pretty much exactly as is described in

	Documentation/devicetree/bindings/spi/spi-mux.yaml

but with a mux node bringing your new driver into the picture
instead of the "gpio-mux" in that example (and the "CS-X" signal
and the "Mux" block in that picture would be internal to the SoC).

If you are content with just programming a fixed set of values to
a couple of registers depending on how the board is wired, some
extra DT property on some node related to the flexcom seems like
a better fit than a mux driver.

Having said that, the mux solution above would solve this static
case too. You would just need to configure a mux with one state.
It would be a couple of extra nodes in the device tree, but it
would certainly work (and also cover the more complex case when
someone pops up and needs that).

Cheers,
Peter

>> Cheers,
>> Peter
>>
>>
>>>>> +     if (IS_ERR(mux_chip))
>>>>> +             return dev_err_probe(dev, PTR_ERR(mux_chip),
>>>>> +                                  "failed to allocate mux_chips\n");
>>>>> +
>>>>> +     mux_lan966x = mux_chip_priv(mux_chip);
>>>>> +
>>>>> +     flx_shared_base =
>> devm_platform_get_and_ioremap_resource(pdev,
>>>> 0, NULL);
>>>>> +     if (IS_ERR(flx_shared_base))
>>>>> +             return dev_err_probe(dev, PTR_ERR(flx_shared_base),
>>>>> +                                  "failed to get flexcom shared base address\n");
>>>>> +
>>>>> +     for (i = 0; i < num_fields; i++) {
>>>>> +             struct mux_control *mux = &mux_chip->mux[i];
>>>>> +             u32 offset, shared_pin;
>>>>> +
>>>>> +             ret = of_property_read_u32_index(np, "mux-offset-pin",
>>>>> +                                              2 * i, &offset);
>>>>> +             if (ret == 0)
>>>>> +                     ret = of_property_read_u32_index(np, "mux-offset-pin",
>>>>> +                                                      2 * i + 1,
>>>>> +                                                      &shared_pin);
>>>>> +             if (ret < 0)
>>>>> +                     return dev_err_probe(dev, ret,
>>>>> +                                          "failed to read mux-offset-pin property: %d", i);
>>>>> +
>>>>> +             if (shared_pin >= LAN966_MAX_CS)
>>>>> +                     return -EINVAL;
>>>>> +
>>>>> +             mux_lan966x[i].offset = offset;
>>>>> +             mux_lan966x[i].ss_pin = shared_pin;
>>>>
>>>> This clobbers memory you have not allocated, if num_fields >= 1.
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>> +
>>>>> +             mux->states = LAN966_MAX_CS;
>>>>> +     }
>>>>> +
>>>>> +     mux_chip->ops = &mux_lan966x_ops;
>>>>> +
>>>>> +     ret = devm_mux_chip_register(dev, mux_chip);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static struct platform_driver mux_lan966x_driver = {
>>>>> +     .driver = {
>>>>> +             .name = "lan966-mux",
>>>>> +             .of_match_table = of_match_ptr(mux_lan966x_dt_ids),
>>>>> +     },
>>>>> +     .probe = mux_lan966x_probe,
>>>>> +};
>>>>> +
>>>>> +module_platform_driver(mux_lan966x_driver);
>>>>> +
>>>>> +MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
>>>>> +MODULE_AUTHOR("Kavyasree Kotagiri
>>>> <[e-mail]>");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>> +

Michael Walle May 17, 2022, 11:53 a.m. UTC | #10

Hi,

>> +struct mux_lan966x {
>
> Why is the file named lan966, but then everything inside lan966x?

So I was about to reply to the bindings but since that question
came up here, too, I'll do it here.

IMHO the name "lan966" is super confusing and if I followed it
correctly, it was just invented because the DT guys don't want to
have a wildcard in the compatibles. But LAN966 isn't a real product,
just LAN9662 and LAN9668 is.

I'd really prefer to have a consistent naming. I've said it once
[1], having "lan966" (over lan966x) feels like cheating and is even
worse, because everyone would assume there is a thing named LAN966.
lan966x might lead the reader to think twice what the 'x' means.

So I'd prefer to have lan966x in the documentation and the drivers
and just "microchip,lan9668" or "microchip,lan9662" in the
compatibles.

-michael

[1] https://lore.kernel.org/linux-devicetree/[e-mail]/

Peter Rosin May 17, 2022, noon UTC | #11

2022-05-17 at 13:53, Michael Walle wrote:
> Hi,
> 
>>> +struct mux_lan966x {
>>
>> Why is the file named lan966, but then everything inside lan966x?
> 
> So I was about to reply to the bindings but since that question
> came up here, too, I'll do it here.
> 
> IMHO the name "lan966" is super confusing and if I followed it
> correctly, it was just invented because the DT guys don't want to
> have a wildcard in the compatibles. But LAN966 isn't a real product,
> just LAN9662 and LAN9668 is.

No wonder I failed when I searched the web for "lan966"...

So, as you were told in the thread you point at below, you name
stuff after one of them (and not some random thing that doesn't
exist), but then handle both in the same file(s). Like you would
have if one was introduced first and the other came later.

Cheers,
Peter

> 
> I'd really prefer to have a consistent naming. I've said it once
> [1], having "lan966" (over lan966x) feels like cheating and is even
> worse, because everyone would assume there is a thing named LAN966.
> lan966x might lead the reader to think twice what the 'x' means.
> 
> So I'd prefer to have lan966x in the documentation and the drivers
> and just "microchip,lan9668" or "microchip,lan9662" in the
> compatibles.
> 
> -michael
> 
> [1] https://lore.kernel.org/linux-devicetree/[e-mail]/

Lee Jones May 23, 2022, 2:52 p.m. UTC | #12

On Mon, 09 May 2022, Kavyasree Kotagiri wrote:

> LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> For each chip select of each flexcom there is a configuration
> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> configuration register is 21 because there are 21 shared pins
> on each of which the chip select can be mapped. Each bit of the
> register represents a different FLEXCOM_SHARED pin.
> 
> Signed-off-by: Kavyasree Kotagiri <[e-mail]>
> ---
>  arch/arm/mach-at91/Kconfig  |   2 +

>  drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-

Can this be separated into its own patch?

>  drivers/mux/Kconfig         |  12 ++++
>  drivers/mux/Makefile        |   2 +
>  drivers/mux/lan966-flx.c    | 121 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 191 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mux/lan966-flx.c
> 
> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> index 279810381256..26fb0f4e1b79 100644
> --- a/arch/arm/mach-at91/Kconfig
> +++ b/arch/arm/mach-at91/Kconfig
> @@ -74,6 +74,8 @@ config SOC_LAN966
>  	select DW_APB_TIMER_OF
>  	select ARM_GIC
>  	select MEMORY
> +	select MULTIPLEXER
> +	select MUX_LAN966
>  	help
>  	  This enables support for ARMv7 based Microchip LAN966 SoC family.
>  
> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> index 559eb4d352b6..7cfd0fc3f4f0 100644
> --- a/drivers/mfd/atmel-flexcom.c
> +++ b/drivers/mfd/atmel-flexcom.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/clk.h>
>  #include <dt-bindings/mfd/atmel-flexcom.h>
> +#include <linux/mux/consumer.h>
>  
>  /* I/O register offsets */
>  #define FLEX_MR		0x0	/* Mode Register */
> @@ -28,6 +29,10 @@
>  #define FLEX_MR_OPMODE(opmode)	(((opmode) << FLEX_MR_OPMODE_OFFSET) &	\
>  				 FLEX_MR_OPMODE_MASK)
>  
> +struct atmel_flex_caps {
> +	bool has_flx_mux;

Why does this need it's own struct?

> +};
> +
>  struct atmel_flexcom {
>  	void __iomem *base;
>  	u32 opmode;
> @@ -37,6 +42,7 @@ struct atmel_flexcom {
>  static int atmel_flexcom_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> +	const struct atmel_flex_caps *caps;
>  	struct resource *res;
>  	struct atmel_flexcom *ddata;
>  	int err;
> @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct platform_device *pdev)
>  	 */
>  	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
>  
> +	caps = of_device_get_match_data(&pdev->dev);
> +	if (!caps) {
> +		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");

dev_err() already prints out the device name, so you can drop the
"flexcom" part.  Also, please expand 'caps'.  I'm assuming that's
capabilities?

> +		return -EINVAL;
> +	}
> +
> +	/* Flexcom Mux */
> +	if (caps->has_flx_mux && of_property_read_bool(np, "mux-controls")) {
> +		struct mux_control *flx_mux;

What is 'flx'?

> +		struct of_phandle_args args;
> +		int i, count;
> +
> +		flx_mux = devm_mux_control_get(&pdev->dev, NULL);
> +		if (IS_ERR(flx_mux))
> +			return PTR_ERR(flx_mux);
> +
> +		count = of_property_count_strings(np, "mux-control-names");
> +		for (i = 0; i < count; i++) {
> +			err = of_parse_phandle_with_fixed_args(np, "mux-controls", 1, i, &args);
> +			if (err)
> +				break;

No mux_control_deselect() for previous iterations?

> +			err = mux_control_select(flx_mux, args.args[0]);
> +			if (!err) {
> +				mux_control_deselect(flx_mux);

Not sure I see the point in selecting then deselecting.

Is it just a test?

If so, why don't you wait until you need to select it, then apply
normal error handling protocols there instead?

> +			} else {
> +				dev_err(&pdev->dev, "Failed to select FLEXCOM mux\n");
> +				return err;
> +			}
> +		}
> +	}
> +
>  	clk_disable_unprepare(ddata->clk);
>  
>  	return devm_of_platform_populate(&pdev->dev);
>  }
>  
> +static const struct atmel_flex_caps atmel_flexcom_caps = {};

Why not just leave .data as NULL?

> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> +	.has_flx_mux = true,
> +};
> +
>  static const struct of_device_id atmel_flexcom_of_match[] = {
> -	{ .compatible = "atmel,sama5d2-flexcom" },
> +	{
> +		.compatible = "atmel,sama5d2-flexcom",
> +		.data = &atmel_flexcom_caps,

And this can't be a DT property?

> +	},
> +
> +	{
> +		.compatible = "microchip,lan966-flexcom",
> +		.data = &lan966x_flexcom_caps,
> +	},

This all seems like a lot of hoop-jumping.

Why not just do:

  if (of_device_is_compatible(np, "lan966x_flexcom_caps"))

> +
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);

Kavyasree Kotagiri May 27, 2022, 6 a.m. UTC | #13

Hi Peter,

Thanks for your comments.
> If you are content with just programming a fixed set of values to
> a couple of registers depending on how the board is wired, some
> extra DT property on some node related to the flexcom seems like
> a better fit than a mux driver.
Based on your inputs, I planned to send a new patch with new DT properties
introduced in atmel-flexcom.c driver rather than mux driver.

Thanks,
Kavya

> -----Original Message-----
> From: Peter Rosin [mailto:[e-mail]]
> Sent: Wednesday, May 11, 2022 9:14 PM
> To: Kavyasree Kotagiri - I30978 <[e-mail]>
> Cc: [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail];
> Manohar Puri - I30488 <[e-mail]>; UNGLinuxDriver
> <[e-mail]>; [e-mail];
> Nicolas Ferre - M43238 <[e-mail]>;
> [e-mail]; Claudiu Beznea - M18063
> <[e-mail]>
> Subject: Re: [PATCH v2 4/4] mux: lan966: Add support for flexcom mux
> controller
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi!
> 
> 2022-05-11 at 16:28, [e-mail] wrote:
> >> 2022-05-10 at 16:59, [e-mail] wrote:
> >>>>> LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> >>>>> For each chip select of each flexcom there is a configuration
> >>>>> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> >>>>> configuration register is 21 because there are 21 shared pins
> >>>>> on each of which the chip select can be mapped. Each bit of the
> >>>>> register represents a different FLEXCOM_SHARED pin.
> >>>>>
> >>>>> Signed-off-by: Kavyasree Kotagiri
> <[e-mail]>
> >>>>> ---
> >>>>>  arch/arm/mach-at91/Kconfig  |   2 +
> >>>>>  drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-
> >>>>>  drivers/mux/Kconfig         |  12 ++++
> >>>>>  drivers/mux/Makefile        |   2 +
> >>>>>  drivers/mux/lan966-flx.c    | 121
> >>>> ++++++++++++++++++++++++++++++++++++
> >>>>>  5 files changed, 191 insertions(+), 1 deletion(-)
> >>>>>  create mode 100644 drivers/mux/lan966-flx.c
> >>>>>
> >>>>> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-
> at91/Kconfig
> >>>>> index 279810381256..26fb0f4e1b79 100644
> >>>>> --- a/arch/arm/mach-at91/Kconfig
> >>>>> +++ b/arch/arm/mach-at91/Kconfig
> >>>>> @@ -74,6 +74,8 @@ config SOC_LAN966
> >>>>>       select DW_APB_TIMER_OF
> >>>>>       select ARM_GIC
> >>>>>       select MEMORY
> >>>>> +     select MULTIPLEXER
> >>>>> +     select MUX_LAN966
> >>>>>       help
> >>>>>         This enables support for ARMv7 based Microchip LAN966 SoC
> >> family.
> >>>>>
> >>>>> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-
> flexcom.c
> >>>>> index 559eb4d352b6..7cfd0fc3f4f0 100644
> >>>>> --- a/drivers/mfd/atmel-flexcom.c
> >>>>> +++ b/drivers/mfd/atmel-flexcom.c
> >>>>> @@ -17,6 +17,7 @@
> >>>>>  #include <linux/io.h>
> >>>>>  #include <linux/clk.h>
> >>>>>  #include <dt-bindings/mfd/atmel-flexcom.h>
> >>>>> +#include <linux/mux/consumer.h>
> >>>>>
> >>>>>  /* I/O register offsets */
> >>>>>  #define FLEX_MR              0x0     /* Mode Register */
> >>>>> @@ -28,6 +29,10 @@
> >>>>>  #define FLEX_MR_OPMODE(opmode)       (((opmode) <<
> >>>> FLEX_MR_OPMODE_OFFSET) &  \
> >>>>>                                FLEX_MR_OPMODE_MASK)
> >>>>>
> >>>>> +struct atmel_flex_caps {
> >>>>> +     bool has_flx_mux;
> >>>>> +};
> >>>>> +
> >>>>>  struct atmel_flexcom {
> >>>>>       void __iomem *base;
> >>>>>       u32 opmode;
> >>>>> @@ -37,6 +42,7 @@ struct atmel_flexcom {
> >>>>>  static int atmel_flexcom_probe(struct platform_device *pdev)
> >>>>>  {
> >>>>>       struct device_node *np = pdev->dev.of_node;
> >>>>> +     const struct atmel_flex_caps *caps;
> >>>>>       struct resource *res;
> >>>>>       struct atmel_flexcom *ddata;
> >>>>>       int err;
> >>>>> @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct
> >>>> platform_device *pdev)
> >>>>>        */
> >>>>>       writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base +
> >> FLEX_MR);
> >>>>>
> >>>>> +     caps = of_device_get_match_data(&pdev->dev);
> >>>>> +     if (!caps) {
> >>>>> +             dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> >>>>> +             return -EINVAL;
> >>>>> +     }
> >>>>> +
> >>>>> +     /* Flexcom Mux */
> >>>>> +     if (caps->has_flx_mux && of_property_read_bool(np, "mux-
> >> controls"))
> >>>> {
> >>>>> +             struct mux_control *flx_mux;
> >>>>> +             struct of_phandle_args args;
> >>>>> +             int i, count;
> >>>>> +
> >>>>> +             flx_mux = devm_mux_control_get(&pdev->dev, NULL);
> >>>>> +             if (IS_ERR(flx_mux))
> >>>>> +                     return PTR_ERR(flx_mux);
> >>>>> +
> >>>>> +             count = of_property_count_strings(np, "mux-control-
> names");
> >>>>> +             for (i = 0; i < count; i++) {
> >>>>> +                     err = of_parse_phandle_with_fixed_args(np, "mux-
> >> controls",
> >>>> 1, i, &args);
> >>>>> +                     if (err)
> >>>>> +                             break;
> >>>>> +
> >>>>> +                     err = mux_control_select(flx_mux, args.args[0]);
> >>>>> +                     if (!err) {
> >>>>> +                             mux_control_deselect(flx_mux);
> >>>>
> >>>> This is suspect. When you deselect the mux you rely on the mux to be
> >>>> configured with "as-is" as the idle state. But you do not document that.
> >>>> This is also fragile in that you silently rely on noone else selecting
> >>>> the mux to some unwanted state behind your back (mux controls are
> not
> >>>> exclusive the way e.g. gpio pins or pwms are). The protocol is that
> >>>> others may get a ref to the mux control and select it as long as noone
> >>>> else has selected it.
> >>>>
> >>>> The only sane thing to do is to keep the mux selected for the whole
> >>>> duration when you rely on it to be in your desired state.
> >>>>
> >>>
> >>> Yes, mux is kept selected until configuring register is done. Please see
> >> below log where
> >>> I added debug prints just for understanding:
> >>> # dmesg | grep KK
> >>>  [    0.779827] KK: Func: atmel_flexcom_probe ***** [START flx muxing]
> >> ********
> >>> [    0.779875] KK: Func: atmel_flexcom_probe i = 0 args[0] = 0
> >>> [    0.779890] KK: Func: mux_control_select [Entered]
> >>> [    0.779902] KK: Func: mux_lan966x_set [Entered] state = 0
> >>> [    0.779977] KK: Func: mux_lan966x_set [Read] = 0x1fffef   <<<-----
> setting
> >> mux_lan966x[state].ss_pin "4" which is passed from dts
> >>> [    0.779992] KK: Func: mux_lan966x_set [Exit]
> >>> [    0.780002] KK: Func: mux_control_select [Exit]
> >>> [    0.780011] KK: Func: mux_control_deselect [Entered]
> >>> [    0.780021] KK: Func: mux_control_deselect [Exit]
> >>
> >> You misunderstand. The mux control is only "selected" between the call
> >> to mux_control_select() and the following call to
> >> mux_control_deselect().
> >>
> >> After that, the mux control is "idle". However, in your case the
> >> "idle-state" is configured to "as-is" (which is the default), so the
> >> mux control (naturally) remains in the previously selected state while
> >> idle. But you are not documenting that limitation, and with this
> >> implementation you *must* have a mux control that uses "as-is" as its
> >> idle state. But that is an unexpected and broken limitation, and a
> >> much better solution is to simply keep the mux control "selected" for
> >> the complete duration when you rely on it to be in whatever state you
> >> need it to be in.
> >>
> > I am new to mux drivers.
> > Let me try to explain why I have mux_control_deselect if there is no err
> from mux_control_select()
> > For example,
> > 1. When I have one only chip-select CS0 for flexcom3 to be mapped to
> flexcom shared pin 9:
> > As per previously shared log, FLEXCOM_SHARED[3]:SS_MASK[0] is being
> configured to expected value
> > even before mux_control_deseletc().
> > 2. When I have to map two chip-selects of flx3 - CS0 to flexcom shared 9
> >                                               CS1 to flexcom shared pin 7
> > FLEXCOM_SHARED[3]:SS_MASK[0] is set to expected value 0x1fffef
> > I see console hangs at mux_control_select() if I don’t call
> mux_control_deselect
> > while mapping second chip-select FLEXCOM_SHARED[3]:SS_MASK[1].
> > After reading below description from mux_control_select() :
> > " * On successfully selecting the mux-control state, it will be locked until
> >  * there is a call to mux_control_deselect()."
> > Following this help text, I called mux_control_deselect() if there is no error
> from mux_control_select()
> > and then it worked. FLEXCOM_SHARED[3]:SS_MASK[1] is now set to
> expected value 0x1fffbf.
> >
> > Please explain me if I am missing something.
> 
> You should wait with the deselect until you need to change the state
> of the mux. I.e., on init/probe you set some variable to -1, like so:
> 
>         priv->mux_error = -1;
> 
> and when you later need to set/change the state of the mux, you do:
> 
>         if (!priv->mux_error)
>                 mux_control_deselect(...);
>         priv->mux_error = mux_control_select(...);
> 
> and then you cleanup at the end of life:
> 
>         if (!priv->mux_error)
>                 mux_control_deselect(...);
> 
> Or something like that.
> 
> The mux api is obviously not a good fit for the use case of long
> lived selects like you appear to have here. The api was designed
> for short lived selects, along the lines of:
> 
> int
> so_something(...)
> {
>         int err;
> 
>         err = mux_control_select(...);
>         if (err)
>                 return err;
> 
>         do_it(...);
> 
>         mux_control_deselect(...);
> }
> 
> 
> >
> >>>>> +                     } else {
> >>>>> +                             dev_err(&pdev->dev, "Failed to select FLEXCOM
> >> mux\n");
> >>>>> +                             return err;
> >>>>> +                     }
> >>>>> +             }
> >>>>> +     }
> >>>>> +
> >>>>>       clk_disable_unprepare(ddata->clk);
> >>>>>
> >>>>>       return devm_of_platform_populate(&pdev->dev);
> >>>>>  }
> >>>>>
> >>>>> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> >>>>> +
> >>>>> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> >>>>> +     .has_flx_mux = true,
> >>>>> +};
> >>>>> +
> >>>>>  static const struct of_device_id atmel_flexcom_of_match[] = {
> >>>>> -     { .compatible = "atmel,sama5d2-flexcom" },
> >>>>> +     {
> >>>>> +             .compatible = "atmel,sama5d2-flexcom",
> >>>>> +             .data = &atmel_flexcom_caps,
> >>>>> +     },
> >>>>> +
> >>>>> +     {
> >>>>> +             .compatible = "microchip,lan966-flexcom",
> >>>>> +             .data = &lan966x_flexcom_caps,
> >>>>> +     },
> >>>>> +
> >>>>>       { /* sentinel */ }
> >>>>>  };
> >>>>>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> >>>>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> >>>>> index e5c571fd232c..ea09f474bc2f 100644
> >>>>> --- a/drivers/mux/Kconfig
> >>>>> +++ b/drivers/mux/Kconfig
> >>>>> @@ -45,6 +45,18 @@ config MUX_GPIO
> >>>>>         To compile the driver as a module, choose M here: the module
> will
> >>>>>         be called mux-gpio.
> >>>>>
> >>>>> +config MUX_LAN966
> >>>>> +     tristate "LAN966 Flexcom multiplexer"
> >>>>> +     depends on OF || COMPILE_TEST
> >>>>> +     help
> >>>>> +     Lan966 Flexcom Multiplexer controller.
> >>>>> +
> >>>>> +     The driver supports mapping 2 chip-selects of each of the lan966
> >>>>> +     flexcoms to 21 flexcom shared pins.
> >>>>> +
> >>>>> +     To compile the driver as a module, choose M here: the module will
> >>>>> +     be called mux-lan966.
> >>>>> +
> >>>>>  config MUX_MMIO
> >>>>>       tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
> >>>>>       depends on OF || COMPILE_TEST
> >>>>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> >>>>> index 6e9fa47daf56..53a9840d96fa 100644
> >>>>> --- a/drivers/mux/Makefile
> >>>>> +++ b/drivers/mux/Makefile
> >>>>> @@ -7,10 +7,12 @@ mux-core-objs                       := core.o
> >>>>>  mux-adg792a-objs             := adg792a.o
> >>>>>  mux-adgs1408-objs            := adgs1408.o
> >>>>>  mux-gpio-objs                        := gpio.o
> >>>>> +mux-lan966-objs                      := lan966-flx.o
> >>>>>  mux-mmio-objs                        := mmio.o
> >>>>>
> >>>>>  obj-$(CONFIG_MULTIPLEXER)    += mux-core.o
> >>>>>  obj-$(CONFIG_MUX_ADG792A)    += mux-adg792a.o
> >>>>>  obj-$(CONFIG_MUX_ADGS1408)   += mux-adgs1408.o
> >>>>>  obj-$(CONFIG_MUX_GPIO)               += mux-gpio.o
> >>>>> +obj-$(CONFIG_MUX_LAN966)     += mux-lan966.o
> >>>>>  obj-$(CONFIG_MUX_MMIO)               += mux-mmio.o
> >>>>> diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
> >>>>> new file mode 100644
> >>>>> index 000000000000..2c7dab616a6a
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/mux/lan966-flx.c
> >>>>> @@ -0,0 +1,121 @@
> >>>>> +// SPDX-License-Identifier: GPL-2.0
> >>>>> +/*
> >>>>> + * LAN966 Flexcom MUX driver
> >>>>> + *
> >>>>> + * Copyright (c) 2022 Microchip Inc.
> >>>>> + *
> >>>>> + * Author: Kavyasree Kotagiri <[e-mail]>
> >>>>
> >>>> Looks like it is based on mmio.c?
> >>>>
> >>> Yes, I took mmio.c  driver as reference.
> >>>
> >>
> >> So, then the above copyright and authorship info is not complete.
> >>
> >>>>> + */
> >>>>> +
> >>>>> +#include <linux/err.h>
> >>>>> +#include <linux/module.h>
> >>>>> +#include <linux/of_platform.h>
> >>>>> +#include <linux/platform_device.h>
> >>>>> +#include <linux/property.h>
> >>>>> +#include <linux/mux/driver.h>
> >>>>> +#include <linux/io.h>
> >>>>> +
> >>>>> +#define FLEX_SHRD_MASK               0x1FFFFF
> >>>>> +#define LAN966_MAX_CS                21
> >>>>> +
> >>>>> +static void __iomem *flx_shared_base;
> >>>>
> >>>> I would much prefer to store the combined address (base+offset)
> >>>> in the mux private data instead of only storing the offset and
> >>>> then unnecessarily rely on some piece of global state (that
> >>>> will be clobbered by other instances).
> >>>>
> >>> Ok. I will try to see if this is relevant and change accordingly.
> >>>
> >>>>> +struct mux_lan966x {
> >>>>
> >>>> Why is the file named lan966, but then everything inside lan966x?
> >>>>
> >>>>> +     u32 offset;
> >>>>> +     u32 ss_pin;
> >>>>> +};
> >>>>> +
> >>>>> +static int mux_lan966x_set(struct mux_control *mux, int state)
> >>>>> +{
> >>>>> +     struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> >>>>> +     u32 val;
> >>>>> +
> >>>>> +     val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
> >>>>> +     writel(val, flx_shared_base + mux_lan966x[state].offset);
> >>>>
> >>>> This reads memory you have not allocated, if you select a state
> >>>> other than zero.
> >>>>
> >>> Ok. I will return error condition in case of trying to access none existing
> >> entry.
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static const struct mux_control_ops mux_lan966x_ops = {
> >>>>> +     .set = mux_lan966x_set,
> >>>>> +};
> >>>>> +
> >>>>> +static const struct of_device_id mux_lan966x_dt_ids[] = {
> >>>>> +     { .compatible = "microchip,lan966-flx-mux", },
> >>>>> +     { /* sentinel */ }
> >>>>> +};
> >>>>> +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
> >>>>> +
> >>>>> +static int mux_lan966x_probe(struct platform_device *pdev)
> >>>>> +{
> >>>>> +     struct device_node *np = pdev->dev.of_node;
> >>>>> +     struct device *dev = &pdev->dev;
> >>>>> +     struct mux_lan966x *mux_lan966x;
> >>>>> +     struct mux_chip *mux_chip;
> >>>>> +     int ret, num_fields, i;
> >>>>> +
> >>>>> +     ret = of_property_count_u32_elems(np, "mux-offset-pin");
> >>>>> +     if (ret == 0 || ret % 2)
> >>>>> +             ret = -EINVAL;
> >>>>> +     if (ret < 0)
> >>>>> +             return dev_err_probe(dev, ret,
> >>>>> +                                  "mux-offset-pin property missing or invalid");
> >>>>> +     num_fields = ret / 2;
> >>>>> +
> >>>>> +     mux_chip = devm_mux_chip_alloc(dev, num_fields,
> >>>> sizeof(*mux_lan966x));
> >>>>
> >>>> I might be thoroughly mistaken and confused by the code, but it seems
> >>>> very strange that a subsequenct select is not undoing what a previous
> >>>> select once did. Each state seems to write to its own register offset,
> >>>> and there is nothing that restores the first register offset with you
> >>>> switch states.
> >>>>
> >>>> Care to explain how muxing works and what you are expected to do to
> >>>> manipulate the mux? Is there some link to public documentation? I did
> >>>> a quick search for lan966 but came up with nothing relevant.
> >>>>
> >>> LAN966 has 5 flexcoms(which can be used as USART/SPI/I2C interface).
> >>> FLEXCOM has two chip-select I/O lines namely CS0 and CS1
> >>> in SPI mode, CTS and RTS in USART mode. These FLEXCOM pins are
> >> optional.
> >>> These chip-selects can be mapped to flexcom shared pin [0-21] which
> can
> >> be
> >>> done by configuring flexcom multiplexer register(FLEXCOM_SHARED[0-
> >> 4]:SS_MASK[0-1])
> >>> Driver explanation:
> >>> "flx_shared_base" is used to get base address of Flexcom shared
> >> multiplexer
> >>> "mux-offset-pin" property is used to get cs0/cs1 offset and flexcom
> shared
> >> pin[0-21] of a flexcom.
> >>> state value passed from atmel-flexcom is used to configure respective
> >>> FLEXCOM_SHARED[0-4]:SS_MASK[0-1] register with offset and flexcom
> >> shared pin.
> >>
> >> Ok, let me try to interpret that. You wish enable a "fan out" of these
> >> two chip-selects for any of the 5 flexcoms (in SPI mode?) such that
> >> these 10 internal chip-selects can be connected to any of 21 external
> >> pins?
> >>
> >> If that's correct and if you wish to interface with e.g. 20 chips,
> >> then it would be possible to have 20 states for one mux control and
> >> then reach the 20 chips using only CS0 from FLEXCOM0, or it would be
> >> possible to have 2 states for 10 mux controls, one each for CS0/CS1 of
> >> all five flexcoms and also reach 20 chips. Or any wild combo you
> >> imagine is useful.
> >>
> >> Is that correctly understood?
> >>
> >> Assuming so, then you can have a maximum of 10 mux controls, and for
> >> each mux control you need a variable number of legitimate states. Each
> >> mux control would also need to know at what address/offset its SS_MASK
> >> register is at and what pins/states are valid.
> >>
> >> But your code does not implemnent the above. You allocate num_fields
> >> mux controls, which is what confuses the hell out of me. num_fields is
> >> the number of states, not the number of mux controls! And you also
> >> need to specify an individual offset for each state, and that makes no
> >> sense at all, at least not to me.
> >>
> >> So, instead, I think you want to have:
> >>
> >> struct mux_lan966x {
> >>         u32 ss_mask;
> >>         int num_pins;
> >>         u32 ss_pin[];
> >> };
> >>
> >> And then do:
> >>
> >>         mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_lan966x) +
> >> num_pins * sizeof(u32));
> >>
> >> (or however that size is best spelled, I haven't kept up)
> >>
> >> The set operation would be along the lines of:
> >>
> >> static int mux_lan966x_set(struct mux_control *mux, int state)
> >> {
> >>         struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> >>         u32 val;
> >>
> >>         if (state < 0 || state >= mux_lan966x->num_pins)
> >>                 return -1;
> >>
> >>         val = ~(1 << mux_lan966x->ss_pin[state]) & FLEX_SHRD_MASK;
> >>         writel(val, flx_shared_base + mux_lan966x->ss_mask);
> >>
> >>         return 0;
> >> }
> >>
> >> Because, one mux control should only ever need to know about one
> offset,
> >> as it should only ever write to its own SS_MASK register. And you will
> >> need some private space such that you can keep track of which states
> >> are legit.
> >>
> >> I would also separate out the SS_MASK offset from the mux-offset-pin
> >> property and have one property for that value and then a straight
> >> array for the valid pin numbers in another property (no longer named
> >> mux-offset-pin of course).
> >>
> >> Or something like that and assuming I understand how the FLEXCOMs
> work
> >> and what you want to do etc.
> >>
> >
> > Thank you for your comments
> > I agree, I will change number of mux controls to 1 in
> devm_mux_chip_alloc()
> > I would like to use mux-offset-pin property because
> > each chip-select must be mapped to a unique flexcom shared pin.
> > For example,
> > mux-offset-pin = <0x18 9>, /* 0: flexcom3 CS0 mapped to pin-9 */
> >                                <0x1c 7>, /* 1: flexcom3 CS1 mapped to pin-7 */
> >                                <0x20 4>; /* 2: flexcom4 CS0 mapped to pin-4 */};
> > I want to use mux-offset-pin property to be clear about which offset is
> mapped to which
> > flexcom shared pin. Here, 0, 1, 2.. entries represents state passed from
> mux_control_select(mux, state).
> >
> > Please provide your comments.
> 
> It seems you want to just use the static info from the devicetree
> to program your registers once and for all? That's not a problem
> that a mux driver is aming at solving. My thought was that if you
> have one SPI device with chip select on pin-7, another with chip-
> select on pin-9 and a third on pin-4, then you /could/ use them all
> from e.g. flexcom3/CS0 by reprogramming that SS_MASK register
> at the time you want to access one of the three chips. You could
> then of course not access the three chips in parallel, but the
> muxing of the accesses to three chips like that is the problem
> that the mux subsystem helps with.
> 
> I.e. pretty much exactly as is described in
> 
>         Documentation/devicetree/bindings/spi/spi-mux.yaml
> 
> but with a mux node bringing your new driver into the picture
> instead of the "gpio-mux" in that example (and the "CS-X" signal
> and the "Mux" block in that picture would be internal to the SoC).
> 
> If you are content with just programming a fixed set of values to
> a couple of registers depending on how the board is wired, some
> extra DT property on some node related to the flexcom seems like
> a better fit than a mux driver.
> 
> Having said that, the mux solution above would solve this static
> case too. You would just need to configure a mux with one state.
> It would be a couple of extra nodes in the device tree, but it
> would certainly work (and also cover the more complex case when
> someone pops up and needs that).
> 
> Cheers,
> Peter
> 
> >> Cheers,
> >> Peter
> >>
> >>
> >>>>> +     if (IS_ERR(mux_chip))
> >>>>> +             return dev_err_probe(dev, PTR_ERR(mux_chip),
> >>>>> +                                  "failed to allocate mux_chips\n");
> >>>>> +
> >>>>> +     mux_lan966x = mux_chip_priv(mux_chip);
> >>>>> +
> >>>>> +     flx_shared_base =
> >> devm_platform_get_and_ioremap_resource(pdev,
> >>>> 0, NULL);
> >>>>> +     if (IS_ERR(flx_shared_base))
> >>>>> +             return dev_err_probe(dev, PTR_ERR(flx_shared_base),
> >>>>> +                                  "failed to get flexcom shared base address\n");
> >>>>> +
> >>>>> +     for (i = 0; i < num_fields; i++) {
> >>>>> +             struct mux_control *mux = &mux_chip->mux[i];
> >>>>> +             u32 offset, shared_pin;
> >>>>> +
> >>>>> +             ret = of_property_read_u32_index(np, "mux-offset-pin",
> >>>>> +                                              2 * i, &offset);
> >>>>> +             if (ret == 0)
> >>>>> +                     ret = of_property_read_u32_index(np, "mux-offset-pin",
> >>>>> +                                                      2 * i + 1,
> >>>>> +                                                      &shared_pin);
> >>>>> +             if (ret < 0)
> >>>>> +                     return dev_err_probe(dev, ret,
> >>>>> +                                          "failed to read mux-offset-pin property: %d",
> i);
> >>>>> +
> >>>>> +             if (shared_pin >= LAN966_MAX_CS)
> >>>>> +                     return -EINVAL;
> >>>>> +
> >>>>> +             mux_lan966x[i].offset = offset;
> >>>>> +             mux_lan966x[i].ss_pin = shared_pin;
> >>>>
> >>>> This clobbers memory you have not allocated, if num_fields >= 1.
> >>>>
> >>>> Cheers,
> >>>> Peter
> >>>>
> >>>>> +
> >>>>> +             mux->states = LAN966_MAX_CS;
> >>>>> +     }
> >>>>> +
> >>>>> +     mux_chip->ops = &mux_lan966x_ops;
> >>>>> +
> >>>>> +     ret = devm_mux_chip_register(dev, mux_chip);
> >>>>> +     if (ret < 0)
> >>>>> +             return ret;
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static struct platform_driver mux_lan966x_driver = {
> >>>>> +     .driver = {
> >>>>> +             .name = "lan966-mux",
> >>>>> +             .of_match_table = of_match_ptr(mux_lan966x_dt_ids),
> >>>>> +     },
> >>>>> +     .probe = mux_lan966x_probe,
> >>>>> +};
> >>>>> +
> >>>>> +module_platform_driver(mux_lan966x_driver);
> >>>>> +
> >>>>> +MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
> >>>>> +MODULE_AUTHOR("Kavyasree Kotagiri
> >>>> <[e-mail]>");
> >>>>> +MODULE_LICENSE("GPL v2");
> >>>>> +

Patch

diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
index 279810381256..26fb0f4e1b79 100644
--- a/arch/arm/mach-at91/Kconfig
+++ b/arch/arm/mach-at91/Kconfig
@@ -74,6 +74,8 @@  config SOC_LAN966
 	select DW_APB_TIMER_OF
 	select ARM_GIC
 	select MEMORY
+	select MULTIPLEXER
+	select MUX_LAN966
 	help
 	  This enables support for ARMv7 based Microchip LAN966 SoC family.
 
diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
index 559eb4d352b6..7cfd0fc3f4f0 100644
--- a/drivers/mfd/atmel-flexcom.c
+++ b/drivers/mfd/atmel-flexcom.c
@@ -17,6 +17,7 @@ 
 #include <linux/io.h>
 #include <linux/clk.h>
 #include <dt-bindings/mfd/atmel-flexcom.h>
+#include <linux/mux/consumer.h>
 
 /* I/O register offsets */
 #define FLEX_MR		0x0	/* Mode Register */
@@ -28,6 +29,10 @@ 
 #define FLEX_MR_OPMODE(opmode)	(((opmode) << FLEX_MR_OPMODE_OFFSET) &	\
 				 FLEX_MR_OPMODE_MASK)
 
+struct atmel_flex_caps {
+	bool has_flx_mux;
+};
+
 struct atmel_flexcom {
 	void __iomem *base;
 	u32 opmode;
@@ -37,6 +42,7 @@  struct atmel_flexcom {
 static int atmel_flexcom_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	const struct atmel_flex_caps *caps;
 	struct resource *res;
 	struct atmel_flexcom *ddata;
 	int err;
@@ -76,13 +82,60 @@  static int atmel_flexcom_probe(struct platform_device *pdev)
 	 */
 	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
 
+	caps = of_device_get_match_data(&pdev->dev);
+	if (!caps) {
+		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
+		return -EINVAL;
+	}
+
+	/* Flexcom Mux */
+	if (caps->has_flx_mux && of_property_read_bool(np, "mux-controls")) {
+		struct mux_control *flx_mux;
+		struct of_phandle_args args;
+		int i, count;
+
+		flx_mux = devm_mux_control_get(&pdev->dev, NULL);
+		if (IS_ERR(flx_mux))
+			return PTR_ERR(flx_mux);
+
+		count = of_property_count_strings(np, "mux-control-names");
+		for (i = 0; i < count; i++) {
+			err = of_parse_phandle_with_fixed_args(np, "mux-controls", 1, i, &args);
+			if (err)
+				break;
+
+			err = mux_control_select(flx_mux, args.args[0]);
+			if (!err) {
+				mux_control_deselect(flx_mux);
+			} else {
+				dev_err(&pdev->dev, "Failed to select FLEXCOM mux\n");
+				return err;
+			}
+		}
+	}
+
 	clk_disable_unprepare(ddata->clk);
 
 	return devm_of_platform_populate(&pdev->dev);
 }
 
+static const struct atmel_flex_caps atmel_flexcom_caps = {};
+
+static const struct atmel_flex_caps lan966x_flexcom_caps = {
+	.has_flx_mux = true,
+};
+
 static const struct of_device_id atmel_flexcom_of_match[] = {
-	{ .compatible = "atmel,sama5d2-flexcom" },
+	{
+		.compatible = "atmel,sama5d2-flexcom",
+		.data = &atmel_flexcom_caps,
+	},
+
+	{
+		.compatible = "microchip,lan966-flexcom",
+		.data = &lan966x_flexcom_caps,
+	},
+
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
index e5c571fd232c..ea09f474bc2f 100644
--- a/drivers/mux/Kconfig
+++ b/drivers/mux/Kconfig
@@ -45,6 +45,18 @@  config MUX_GPIO
 	  To compile the driver as a module, choose M here: the module will
 	  be called mux-gpio.
 
+config MUX_LAN966
+	tristate "LAN966 Flexcom multiplexer"
+	depends on OF || COMPILE_TEST
+	help
+	Lan966 Flexcom Multiplexer controller.
+
+	The driver supports mapping 2 chip-selects of each of the lan966
+	flexcoms to 21 flexcom shared pins.
+
+	To compile the driver as a module, choose M here: the module will
+	be called mux-lan966.
+
 config MUX_MMIO
 	tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
 	depends on OF || COMPILE_TEST
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
index 6e9fa47daf56..53a9840d96fa 100644
--- a/drivers/mux/Makefile
+++ b/drivers/mux/Makefile
@@ -7,10 +7,12 @@  mux-core-objs			:= core.o
 mux-adg792a-objs		:= adg792a.o
 mux-adgs1408-objs		:= adgs1408.o
 mux-gpio-objs			:= gpio.o
+mux-lan966-objs			:= lan966-flx.o
 mux-mmio-objs			:= mmio.o
 
 obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
 obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
 obj-$(CONFIG_MUX_ADGS1408)	+= mux-adgs1408.o
 obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
+obj-$(CONFIG_MUX_LAN966)	+= mux-lan966.o
 obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
new file mode 100644
index 000000000000..2c7dab616a6a
--- /dev/null
+++ b/drivers/mux/lan966-flx.c
@@ -0,0 +1,121 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LAN966 Flexcom MUX driver
+ *
+ * Copyright (c) 2022 Microchip Inc.
+ *
+ * Author: Kavyasree Kotagiri <[e-mail]>
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/mux/driver.h>
+#include <linux/io.h>
+
+#define FLEX_SHRD_MASK		0x1FFFFF
+#define LAN966_MAX_CS		21
+
+static void __iomem *flx_shared_base;
+struct mux_lan966x {
+	u32 offset;
+	u32 ss_pin;
+};
+
+static int mux_lan966x_set(struct mux_control *mux, int state)
+{
+	struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
+	u32 val;
+
+	val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
+	writel(val, flx_shared_base + mux_lan966x[state].offset);
+
+	return 0;
+}
+
+static const struct mux_control_ops mux_lan966x_ops = {
+	.set = mux_lan966x_set,
+};
+
+static const struct of_device_id mux_lan966x_dt_ids[] = {
+	{ .compatible = "microchip,lan966-flx-mux", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
+
+static int mux_lan966x_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct mux_lan966x *mux_lan966x;
+	struct mux_chip *mux_chip;
+	int ret, num_fields, i;
+
+	ret = of_property_count_u32_elems(np, "mux-offset-pin");
+	if (ret == 0 || ret % 2)
+		ret = -EINVAL;
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "mux-offset-pin property missing or invalid");
+	num_fields = ret / 2;
+
+	mux_chip = devm_mux_chip_alloc(dev, num_fields, sizeof(*mux_lan966x));
+	if (IS_ERR(mux_chip))
+		return dev_err_probe(dev, PTR_ERR(mux_chip),
+				     "failed to allocate mux_chips\n");
+
+	mux_lan966x = mux_chip_priv(mux_chip);
+
+	flx_shared_base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+	if (IS_ERR(flx_shared_base))
+		return dev_err_probe(dev, PTR_ERR(flx_shared_base),
+				     "failed to get flexcom shared base address\n");
+
+	for (i = 0; i < num_fields; i++) {
+		struct mux_control *mux = &mux_chip->mux[i];
+		u32 offset, shared_pin;
+
+		ret = of_property_read_u32_index(np, "mux-offset-pin",
+						 2 * i, &offset);
+		if (ret == 0)
+			ret = of_property_read_u32_index(np, "mux-offset-pin",
+							 2 * i + 1,
+							 &shared_pin);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "failed to read mux-offset-pin property: %d", i);
+
+		if (shared_pin >= LAN966_MAX_CS)
+			return -EINVAL;
+
+		mux_lan966x[i].offset = offset;
+		mux_lan966x[i].ss_pin = shared_pin;
+
+		mux->states = LAN966_MAX_CS;
+	}
+
+	mux_chip->ops = &mux_lan966x_ops;
+
+	ret = devm_mux_chip_register(dev, mux_chip);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static struct platform_driver mux_lan966x_driver = {
+	.driver = {
+		.name = "lan966-mux",
+		.of_match_table	= of_match_ptr(mux_lan966x_dt_ids),
+	},
+	.probe = mux_lan966x_probe,
+};
+
+module_platform_driver(mux_lan966x_driver);
+
+MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
+MODULE_AUTHOR("Kavyasree Kotagiri <[e-mail]>");
+MODULE_LICENSE("GPL v2");
+