Re: [RFC PATCH v2 2/2] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve <hugo@xxxxxxxxxxx>]

On Fri, 19 May 2023 18:56:00 +0200
Lech Perczak <lech.perczak@xxxxxxxxxxxxxxx> wrote:

> 
> W dniu 19.05.2023 o 18:38, Hugo Villeneuve pisze:
> > On Fri, 19 May 2023 18:00:54 +0200
> > Lech Perczak <lech.perczak@xxxxxxxxxxxxxxx> wrote:
> >
> >> W dniu 19.05.2023 o 15:30, Hugo Villeneuve pisze:
> >>> On Fri, 19 May 2023 15:00:58 +0200
> >>> Lech Perczak <lech.perczak@xxxxxxxxxxxxxxx> wrote:
> >>>
> >>>> Hello Hugo,
> >>>>
> >>>> A couple of remarks inline.
> >>>>
> >>>> W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
> >>>>> From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> >>>>>
> >>>>> Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> >>>>> and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> >>>>> changed the function of the GPIOs pins to act as modem control
> >>>>> lines without any possibility of selecting GPIO function.
> >>>>>
> >>>>> As a consequence, applications that depends on GPIO lines configured
> >>>>> by default as GPIO pins no longer work as expected.
> >>>>>
> >>>>> Also, the change to select modem control lines function was done only
> >>>>> for channel A of dual UART variants (752/762). This was not documented
> >>>>> in the log message.
> >>>>>
> >>>>> This new patch allows to specify GPIO or modem control line function
> >>>>> in the device tree, and for each of the ports (A or B).
> >>>>>
> >>>>> This is done by using the new device-tree property named
> >>>>> "modem-control-line-ports" (property added in separate patch).
> >>>>>
> >>>>> Boards that need to have GPIOS configured as modem control lines
> >>>>> should add that property to their device tree. Here is a list of
> >>>>> boards using the sc16is7xx driver in their device tree and that may
> >>>>> need to be modified:
> >>>>>     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> >>>>>     mips/boot/dts/ingenic/cu1830-neo.dts
> >>>>>     mips/boot/dts/ingenic/cu1000-neo.dts
> >>>>>
> >>>>> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> >>>>> Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> >>>>> Signed-off-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> >>>>> ---
> >>>>> v1 -> v2: Rebase because original patch did not apply properly
> >>>>>           Add git base-commit
> >>>>>
> >>>>>  drivers/tty/serial/sc16is7xx.c | 42 ++++++++++++++++++++--------------
> >>>>>  1 file changed, 25 insertions(+), 17 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> >>>>> index abad091baeea..4e3d2325ef6e 100644
> >>>>> --- a/drivers/tty/serial/sc16is7xx.c
> >>>>> +++ b/drivers/tty/serial/sc16is7xx.c
> >>>>> @@ -236,7 +236,8 @@
> >>>>>
> >>>>>  /* IOControl register bits (Only 750/760) */
> >>>>>  #define SC16IS7XX_IOCONTROL_LATCH_BIT  (1 << 0) /* Enable input latching */
> >>>>> -#define SC16IS7XX_IOCONTROL_MODEM_BIT  (1 << 1) /* Enable GPIO[7:4] as modem pins */
> >>>>> +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT        (1 << 1) /* Enable GPIO[7:4] as modem A pins */
> >>>>> +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT        (1 << 2) /* Enable GPIO[3:0] as modem B pins */
> >>>>>  #define SC16IS7XX_IOCONTROL_SRESET_BIT (1 << 3) /* Software Reset */
> >>>>>
> >>>>>  /* EFCR register bits */
> >>>>> @@ -306,7 +307,6 @@ struct sc16is7xx_devtype {
> >>>>>         char    name[10];
> >>>>>         int     nr_gpio;
> >>>>>         int     nr_uart;
> >>>>> -       int     has_mctrl;
> >>>>>  };
> >>>>>
> >>>>>  #define SC16IS7XX_RECONF_MD            (1 << 0)
> >>>>> @@ -447,35 +447,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> >>>>>         .name           = "SC16IS74X",
> >>>>>         .nr_gpio        = 0,
> >>>>>         .nr_uart        = 1,
> >>>>> -       .has_mctrl      = 0,
> >>>>>  };
> >>>>>
> >>>>>  static const struct sc16is7xx_devtype sc16is750_devtype = {
> >>>>>         .name           = "SC16IS750",
> >>>>> -       .nr_gpio        = 4,
> >>>>> +       .nr_gpio        = 8,
> >>>>>         .nr_uart        = 1,
> >>>>> -       .has_mctrl      = 1,
> >>>>>  };
> >>>>>
> >>>>>  static const struct sc16is7xx_devtype sc16is752_devtype = {
> >>>>>         .name           = "SC16IS752",
> >>>>> -       .nr_gpio        = 0,
> >>>>> +       .nr_gpio        = 8,
> >>>>>         .nr_uart        = 2,
> >>>>> -       .has_mctrl      = 1,
> >>>>>  };
> >>>>>
> >>>>>  static const struct sc16is7xx_devtype sc16is760_devtype = {
> >>>>>         .name           = "SC16IS760",
> >>>>> -       .nr_gpio        = 4,
> >>>>> +       .nr_gpio        = 8,
> >>>>>         .nr_uart        = 1,
> >>>>> -       .has_mctrl      = 1,
> >>>>>  };
> >>>>>
> >>>>>  static const struct sc16is7xx_devtype sc16is762_devtype = {
> >>>>>         .name           = "SC16IS762",
> >>>>> -       .nr_gpio        = 0,
> >>>>> +       .nr_gpio        = 8,
> >>>>>         .nr_uart        = 2,
> >>>>> -       .has_mctrl      = 1,
> >>>>>  };
> >>>>>
> >>>>>  static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
> >>>>> @@ -1456,12 +1451,6 @@ static int sc16is7xx_probe(struct device *dev,
> >>>>>                                      SC16IS7XX_EFCR_RXDISABLE_BIT |
> >>>>>                                      SC16IS7XX_EFCR_TXDISABLE_BIT);
> >>>>>
> >>>>> -               /* Use GPIO lines as modem status registers */
> >>>>> -               if (devtype->has_mctrl)
> >>>>> -                       sc16is7xx_port_write(&s->p[i].port,
> >>>>> -                                            SC16IS7XX_IOCONTROL_REG,
> >>>>> -                                            SC16IS7XX_IOCONTROL_MODEM_BIT);
> >>>>> -
> >>>>>                 /* Initialize kthread work structs */
> >>>>>                 kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
> >>>>>                 kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
> >>>>> @@ -1497,6 +1486,25 @@ static int sc16is7xx_probe(struct device *dev,
> >>>>>                                          prop, p, u)
> >>>>>                         if (u < devtype->nr_uart)
> >>>>>                                 s->p[u].irda_mode = true;
> >>>>> +
> >>>>> +               val = 0;
> >>>>> +
> >>>>> +               of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> >>>>> +                                        prop, p, u)
> >>>> The general idea looks solid to me.
> >>>> I think, that with this property set, we should also reduce number of GPIOs exported by gpiochip.
> >>> OK, I will look into that.
> >>>
> >>>>> +                       if (u < devtype->nr_uart) {
> >>>>> +                               /* Use GPIO lines as modem control lines */
> >>>>> +                               if (u == 0)
> >>>>> +                                       val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> >>>>> +                               else if (u == 1)
> >>>>> +                                       val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> >>>>> +                       }
> >>>>> +
> >>>>> +               if (val)
> >>>>> +                       regmap_update_bits(
> >>>>> +                               s->regmap,
> >>>>> +                               SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> >>>>> +                               SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> >>>>> +                               SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
> >>>> I just had a chance to test this on my hardware (with SC16IS760), and something still doesn't work.
> >>>> When looking into /sys/kernel/debug/regmap/<device>/registers,
> >>>> it seems like the proper bits are set, also taking the regmap shift into account.
> >>>> For reference - I did my test basing on v6.4-rc2.
> >>>>
> >>>> I used a loopback plug and statserial to perform the test.
> >>>> Without your patch, when DTR is set, all other pins (DCD, DSR, RI) are set. With it - they are cleared, according to statserial.
> >>> It is strange, because the only conceptual change is the moment in time when we set the IOControl register. Before, IOControl was set within the for/loop for initializing each port. Now it is done after.
> >>>
> >>> Maybe you could try to put the section that reads the DT property just after resetting the device.
> >>>
> >>> While suggesting this, I just noticed that when the device is reset, we do not wait before initiating communication with the device. Thefore, I would suggest the following change (that I will submit as a separate patch):
> >>>
> >>>         regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> >>>                         SC16IS7XX_IOCONTROL_SRESET_BIT);
> >>>
> >>> +       /* After reset, the host must wait at least 3us before initializing a
> >>> +        * communication with the device: */
> >>> +       usleep_range(3, 5);
> >>>
> >>> Then like I said, insert the code that read the modem-control-line-port DT property right after that to see if it helps.
> >>>
> >>> Hugo.
> >> Hello,
> >>
> >> I tried those suggestions, together with the patch you mentioned in the other email - all to no avail.
> >> I added a debug print to confirm, that the property is detected, and regmap access is executed - and it is. This is so strange.
> >> Maybe the offsets in regmap are still wrong, for some reason?
> >> I also double checked if it was possible to alter and check GPIO states through GPIO subsystem - it is.
> > Then it seems that the remaining difference is the number of GPIOs that are configured, which for the moment is always the value set in constant nr_gpio (for each device).
> >
> > I already implemented and tested your suggestion of lowering the number of GPIOs exported when using some pins as modem status lines. It seems to be working good, altough I have only tested that the correct number of GPIOs are now exported.
> >
> > I will send you the test patch and let me know if it helps.
> >
> > Hugo.
> Hello Hugo,
> 
> I applied the patch marking IOControl register as volatile - and this one fixed the issue. Modem line states are now reported correctly.
> I think it'll be a good idea to bundle all the fixes in the series, when they become ready for submission.

Hi Lech,
glad to hear this.

I wanted to keep the patches separate, to not interfere with fixing the GPIO regression, but now I see that it caused way more headaches than anticipated :)

I will integrate all the sc16is7xx patches in a new serie and it should be easier to apply/test.

And let me know if the patch to reduce the number of exported GPIOs is working as expected for you.

Thank you,
Hugo.