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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 2022-05-10 at 16:59, Kavyasree.Kotagiri@xxxxxxxxxxxxx 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 <kavyasree.kotagiri@xxxxxxxxxxxxx>
> >>> ---
> >>>  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 <kavyasree.kotagiri@xxxxxxxxxxxxx>
> >>
> >> 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
> >> <kavyasree.kotagiri@xxxxxxxxxxxxx>");
> >>> +MODULE_LICENSE("GPL v2");
> >>> +




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux