| Message ID | 20230725142343.1724130-1-hugo@hugovil.com |
|---|---|
| Headers | show |
| Series | serial: sc16is7xx: fix GPIO regression and rs485 improvements | expand |
On Tue, Jul 25, 2023 at 10:23:34AM -0400, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Bit SRESET (3) is cleared when a reset operation is completed. Having > the IOCONTROL register as non-volatile will always read SRESET as 1, > which is incorrect. > > Also, if IOCONTROL register is not a volatile register, the upcoming > patch "serial: sc16is7xx: fix regression with GPIO configuration" > doesn't work when setting some shared GPIO lines as modem control > lines. > > Therefore mark IOCONTROL register as a volatile register. > > Cc: <stable@vger.kernel.org> # 6.1.x Why 6.1.y? What commit does this fix? > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com> > Tested-by: Lech Perczak <lech.perczak@camlingroup.com> > --- > drivers/tty/serial/sc16is7xx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index 8ae2afc76a9b..306ae512b38a 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -488,6 +488,7 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg) > case SC16IS7XX_TXLVL_REG: > case SC16IS7XX_RXLVL_REG: > case SC16IS7XX_IOSTATE_REG: > + case SC16IS7XX_IOCONTROL_REG: > return true; > default: > break; Is this the same as this change: https://lore.kernel.org/all/20230724034727.17335-1-hui.wang@canonical.com/ confused, greg k-h
On Tue, Jul 25, 2023 at 10:23:35AM -0400, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Commit c8f71b49ee4d ("serial: sc16is7xx: setup GPIO controller later > in probe") moved GPIO setup code later in probe function. Doing so > also required to move ports cleanup code (out_ports label) after the > GPIO cleanup code. > > After these moves, the out_thread label becomes misplaced and makes > part of the cleanup code illogical. > > This patch remove the now obsolete out_thread label and make GPIO > setup code jump to out_ports label if it fails. > > Fixes: c8f71b49ee4d ("serial: sc16is7xx: setup GPIO controller later in probe") > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com> > Tested-by: Lech Perczak <lech.perczak@camlingroup.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Why is this not ok for stable kernels yet it has a Fixes: tag? Please fix. thanks, greg k-h
On Tue, Jul 25, 2023 at 10:23:41AM -0400, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Make sure we wait at least 3us before initiating communication with > the device after reset. That says what you do, but not _why_ you do it? Please read the kernel documentation again for how to write a good changelog text. It's usually the hardest part of submitting a patch. > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com> > Tested-by: Lech Perczak <lech.perczak@camlingroup.com> > --- > drivers/tty/serial/sc16is7xx.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index 49213be60baf..718e982e1efe 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -1526,6 +1526,12 @@ static int sc16is7xx_probe(struct device *dev, > 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(5, 10); 5, 10 is NOT 3us. :(
On Tue, Jul 25, 2023 at 10:23:40AM -0400, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Add call to uart_get_rs485_mode() to probe for RS485 flags and > properties from device tree. Again, you are saying what you are doing, but not why. I have no hint as to if this is a bugfix, or a new features, or something else? thanks, greg k-h
On Mon, 31 Jul 2023 17:50:40 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Tue, Jul 25, 2023 at 10:23:34AM -0400, Hugo Villeneuve wrote: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Bit SRESET (3) is cleared when a reset operation is completed. Having > > the IOCONTROL register as non-volatile will always read SRESET as 1, > > which is incorrect. > > > > Also, if IOCONTROL register is not a volatile register, the upcoming > > patch "serial: sc16is7xx: fix regression with GPIO configuration" > > doesn't work when setting some shared GPIO lines as modem control > > lines. > > > > Therefore mark IOCONTROL register as a volatile register. > > > > Cc: <stable@vger.kernel.org> # 6.1.x > > Why 6.1.y? What commit does this fix? > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com> > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com> > > --- > > drivers/tty/serial/sc16is7xx.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > > index 8ae2afc76a9b..306ae512b38a 100644 > > --- a/drivers/tty/serial/sc16is7xx.c > > +++ b/drivers/tty/serial/sc16is7xx.c > > @@ -488,6 +488,7 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg) > > case SC16IS7XX_TXLVL_REG: > > case SC16IS7XX_RXLVL_REG: > > case SC16IS7XX_IOSTATE_REG: > > + case SC16IS7XX_IOCONTROL_REG: > > return true; > > default: > > break; > > Is this the same as this change: > https://lore.kernel.org/all/20230724034727.17335-1-hui.wang@canonical.com/ > > confused, Hi Greg, yes this is the same. You simply accepted an exact equivalent of my patch by someone else in your tree, no confusion there. I will remove this patch from my series and rebase it on your tree gregkh_tty/tty-next. Hugo.
On Mon, 31 Jul 2023 17:57:50 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Tue, Jul 25, 2023 at 10:23:41AM -0400, Hugo Villeneuve wrote: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Make sure we wait at least 3us before initiating communication with > > the device after reset. > > That says what you do, but not _why_ you do it? You are right, it is not clear. I will add a note that it is recommended to do so by the manufacturer in the device datasheet. > Please read the kernel documentation again for how to write a good > changelog text. It's usually the hardest part of submitting a patch. Yes. > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com> > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com> > > --- > > drivers/tty/serial/sc16is7xx.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > > index 49213be60baf..718e982e1efe 100644 > > --- a/drivers/tty/serial/sc16is7xx.c > > +++ b/drivers/tty/serial/sc16is7xx.c > > @@ -1526,6 +1526,12 @@ static int sc16is7xx_probe(struct device *dev, > > 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(5, 10); > > 5, 10 is NOT 3us. In v3, Andy Shevchenko suggested "I would put (5, 10) instead to relax a bit the scheduler." Should I add a comment to that effect: /* * After reset, the datasheet indicates that the host must wait at least * 3us before initializing a communication with the device. * Use (5, 10) range to relax the scheduler. */ ? Hugo.
On Mon, 31 Jul 2023 17:53:10 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Tue, Jul 25, 2023 at 10:23:35AM -0400, Hugo Villeneuve wrote: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Commit c8f71b49ee4d ("serial: sc16is7xx: setup GPIO controller later > > in probe") moved GPIO setup code later in probe function. Doing so > > also required to move ports cleanup code (out_ports label) after the > > GPIO cleanup code. > > > > After these moves, the out_thread label becomes misplaced and makes > > part of the cleanup code illogical. > > > > This patch remove the now obsolete out_thread label and make GPIO > > setup code jump to out_ports label if it fails. > > > > Fixes: c8f71b49ee4d ("serial: sc16is7xx: setup GPIO controller later in probe") > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com> > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Why is this not ok for stable kernels yet it has a Fixes: tag? > > Please fix. > > thanks, > > greg k-h Hi, this is a somewhat particular case. It is a change that "fixes" some previously unseen consequence in original commit, but that does not result in any binary change in the end. That is why I decided not to put in a "stable" tag. If you want, maybe it would be simpler to remove the "Fixes" tag? I originally put this tag to have a reference to the original commit, but since it is already mentioned in the commit log message body, it can be removed. Hugo.
On Thu, Aug 03, 2023 at 10:38:14AM -0400, Hugo Villeneuve wrote: > On Mon, 31 Jul 2023 17:59:14 +0200 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Tue, Jul 25, 2023 at 10:23:40AM -0400, Hugo Villeneuve wrote: > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > Add call to uart_get_rs485_mode() to probe for RS485 flags and > > > properties from device tree. > > > > Again, you are saying what you are doing, but not why. I have no hint > > as to if this is a bugfix, or a new features, or something else? > > > > thanks, > > > > greg k-h > > Hi Greg, > I could change the commit message to: > > --------- > serial: sc16is7xx: add missing support for rs485 devicetree properties > > Retrieve rs485 devicetree properties on registration of sc16is7xx ports > in case they are attached to an rs485 transceiver. > --------- > > I don't think that it should be considered as a bug fix, but maybe as a > missing feature. > > And does it mean that it should also go to older (stable) kernels then? > If yes, then do I need to add the "Fixes" tag? Does it fix a problem? If so, yes, it should go to older kernels. If not, then no. thanks, greg k-h
From: Hugo Villeneuve <hvilleneuve@dimonoff.com> Hello, this patch series mainly fixes a GPIO regression and improve RS485 flags and properties detection from DT. It now also includes various small fixes and improvements that were previously sent as separate patches, but that made testing everything difficult. Patch 1 fixes an issue with init of first port during probing. Patch 2 fixes an issue when debugging IOcontrol register, but it is also necessary for patch "fix regression with GPIO configuration" to work. Patch 3 fixes an incorrect label in sc16is7xx_probe() cleanup code. Patch 4 is a refactor of GPIO registration code in preparation for patch 5. Patches 5 and 6 fix a GPIO regression by (re)allowing to choose GPIO function for GPIO pins shared with modem status lines. Patch 7 fixes a bug with the output value when first setting the GPIO direction. Patch 8 allows to read common rs485 device-tree flags and properties. Patch 9 introduces a delay after a reset operation to respect datasheet timing recommandations. Patch 10 improves comments about chip variants. I have tested the changes on a custom board with two SC16IS752 DUART using a Variscite IMX8MN NANO SOM. 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 Thank you. Link: [v0] https://lkml.org/lkml/2023/5/15/923 [v1] https://lkml.org/lkml/2023/5/17/967 [v1] https://lkml.org/lkml/2023/5/17/777 [v1] https://lkml.org/lkml/2023/5/17/780 [v1] https://lkml.org/lkml/2023/5/17/785 [v1] https://lkml.org/lkml/2023/5/17/1311 [v2] https://lkml.org/lkml/2023/5/18/516 [v3] https://lkml.org/lkml/2023/5/25/7 [v4] https://lkml.org/lkml/2023/5/29/656 [v5] https://lkml.org/lkml/2023/6/1/1046 [v6] https://lkml.org/lkml/2023/6/1/1328 [v7] https://lkml.org/lkml/2023/6/2/861 [v8] https://lkml.org/lkml/2023/6/7/813 https://lkml.org/lkml/2023/7/21/1055 Changes for V1: - Abandonned the approach of trying to fix GPIO regression by reverting the original patches, because it created more problems than it solved (breaking existing users). Instead, add DT property to fix the GPIO behavior. Changes for V2: - Integrate multiple patches into this serie. Changes for V3: - Integrated all patches into single serie to facilitate debugging and tests. - Reduce number of exported GPIOs depending on new property nxp,modem-control-line-ports - Added additional example in DT bindings Changes for V4: - Increase reset post delay to relax scheduler. - Put comments patches at the end. - Remove Fixes tag for patch "mark IOCONTROL register as volatile". - Improve commit messages after reviews. - Fix coding style issues after reviews. - Change GPIO registration to always register the maximum number of GPIOs supported by the chip, but maks-out GPIOs declared as modem control lines. - Add patch to refactor GPIO registration. - Remove patch "serial: sc16is7xx: fix syntax error in comments". - Remove patch "add dump registers function" Changes for V5: - Change patch order to facilitate stable backport(s). - Change duplicate device addresses in DT binding examples. - Use GENMASK for bit masks. - Replace of_property_for_each_u32() with device_property_read_u32_array - Add "Cc: stable..." tags Changes for V6: - Fix compilation bug introduced by patch 3 Changes for V7: - Minor changes and coding style fixes after review for patch 5 "fix regression with GPIO configuration". Changes for V8: - Move mctrl_mask to "struct sc16is7xx_port" to avoid compiler warning when CONFIG_GPIOLIB is undefined. - Add "struct device" member to "struct sc16is7xx_port", in order to avoid passing a raw "struct device" to called functions from sc16is7xx_probe(). - Add new patch "serial: sc16is7xx: remove obsolete out_thread label" Changes for V9: - Change DT property name in commit message and move some comments to cover letter for patch "fix regression with GPIO configuration". - Add proper link to pre-v1 (v0) RFC patch. - Add changes log for V1 and V2 to this cover letter. Hugo Villeneuve (10): serial: sc16is7xx: fix broken port 0 uart init serial: sc16is7xx: mark IOCONTROL register as volatile serial: sc16is7xx: remove obsolete out_thread label serial: sc16is7xx: refactor GPIO controller registration dt-bindings: sc16is7xx: Add property to change GPIO function serial: sc16is7xx: fix regression with GPIO configuration serial: sc16is7xx: fix bug when first setting GPIO direction serial: sc16is7xx: add call to get rs485 DT flags and properties serial: sc16is7xx: add post reset delay serial: sc16is7xx: improve comments about variants .../bindings/serial/nxp,sc16is7xx.txt | 46 +++++ drivers/tty/serial/sc16is7xx.c | 177 +++++++++++++----- 2 files changed, 181 insertions(+), 42 deletions(-) base-commit: 0b5547c51827e053cc754db47d3ec3e6c2c451d2