| Message ID | 20241219084638.960253-1-andre.werner@systec-electronic.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: sc16is7xx: Add polling feature if no IRQ usage possible | expand |
Dear Greg: On Thu, 19 Dec 2024, Greg KH wrote: > On Thu, Dec 19, 2024 at 10:22:40AM +0100, Andre Werner wrote: > > Dear Greg, > > On Thu, 19 Dec 2024, Greg KH wrote: > > > > > On Thu, Dec 19, 2024 at 09:46:38AM +0100, Andre Werner wrote: > > > > Fall back to polling mode if no interrupt is configured because not > > > > possible. If "interrupts" property is missing in devicetree the driver > > > > uses a delayed worker to pull state of interrupt status registers. > > > > > > > > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> > > > > --- > > > > This driver was tested on Linux 5.10. We had a custom board that was not > > > > able to connect the interrupt port. Only I2C was available. > > > > > > Could you not test this on the latest tree? 5.10 is _VERY_ old now. > > > > I will try it on devboard with a 6.1 Kernel. Is that okay for you? > > 6.1 was released in December of 2022, 2 full years and hundreds of > thousands of changes ago. Please work off of Linus's latest tree, we > can't go back in time :) > > > > > @@ -1537,7 +1564,13 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > > > > > > > > /* Always ask for fixed clock rate from a property. */ > > > > device_property_read_u32(dev, "clock-frequency", &uartclk); > > > > + s->polling = !device_property_present(dev, "interrupts"); > > > > > > > > + if (s->polling) { > > > > + dev_warn(dev, > > > > + "No interrupt definition found. Falling back to polling mode.\n"); > > > > > > What is a user supposed to do with this message? And why would a device > > > NOT have any interrupts? This feels like it is just going to pound on > > > the device and cause a lot of power drain for just a simple little uart. > > > > I thought it could be interesting to know that the device has missing > > interrupt support. > > Maybe, but as you are now warning a user about this, what are they > supposed to do to fix it? > > > > Why can't your system provide a valid irq line? > > > > > > > In our case we have only an I2C available in a connection cable and the > > GPIOs are linked through a two way level shifter. > > It was a very special situation in our case because target platform and > > sensor platform are provided. > > The IRQ from the sensor war not able to drive the two way level shifter low so > > we always detect outgoing traffic and the IRQ signal but at the target > > board after the level shifter the signal remains stable. So > > communication failed with a timeout. So we need to force polling the > > interrupt status register because > > both HW solution should not be changed in any way. > > Again, you are burning a TON of power just for a simple little uart, > with your system never being able to go to sleep, are you sure this is > something that you want others to emulate and support? I got your point and I'm fully with. This caused me to print a warning in Kernel log because it should not be the general working method. In our special case we do not have any other option because the sensor module using the SC16IS7xx and the hardware with the MCU running Linux OS are fixed. We had no possibilities to move any GPIO or such. This was the only chance to support the dedicated sensor platform and I may be the case that someone else faces the same problems. I thought that someone else may benefit from this workaround too. But as I got your point I'm also fine if it is not merged into main Linux Kernel sources. > > thanks, > > greg k-h >
Hello Greg and Andre, > From: Andre Werner <andre.werner@systec-electronic.com> > > Dear Greg: > > On Thu, 19 Dec 2024, Greg KH wrote: > > > On Thu, Dec 19, 2024 at 10:22:40AM +0100, Andre Werner wrote: > > > Dear Greg, > > > On Thu, 19 Dec 2024, Greg KH wrote: > > > > > > > On Thu, Dec 19, 2024 at 09:46:38AM +0100, Andre Werner wrote: > > > > > Fall back to polling mode if no interrupt is configured because not > > > > > possible. If "interrupts" property is missing in devicetree the driver > > > > > uses a delayed worker to pull state of interrupt status registers. > > > > > > > > > > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> > > > > > --- > > > > > This driver was tested on Linux 5.10. We had a custom board that was not > > > > > able to connect the interrupt port. Only I2C was available. > > > > > > > > Could you not test this on the latest tree? 5.10 is _VERY_ old now. > > > > > > I will try it on devboard with a 6.1 Kernel. Is that okay for you? > > > > 6.1 was released in December of 2022, 2 full years and hundreds of > > thousands of changes ago. Please work off of Linus's latest tree, we > > can't go back in time :) I agree with Greg that such a new feature only belongs in the current development. > > > > > @@ -1537,7 +1564,13 @@ int sc16is7xx_probe(struct device *dev, const > struct sc16is7xx_devtype *devtype, > > > > > > > > > > /* Always ask for fixed clock rate from a property. */ > > > > > device_property_read_u32(dev, "clock-frequency", &uartclk); > > > > > + s->polling = !device_property_present(dev, "interrupts"); > > > > > > > > > > + if (s->polling) { > > > > > + dev_warn(dev, > > > > > + "No interrupt definition found. Falling back to > polling mode.\n"); > > > > > > > > What is a user supposed to do with this message? And why would a device > > > > NOT have any interrupts? This feels like it is just going to pound on > > > > the device and cause a lot of power drain for just a simple little uart. > > > > > > I thought it could be interesting to know that the device has missing > > > interrupt support. > > > > Maybe, but as you are now warning a user about this, what are they > > supposed to do to fix it? Again, I agree with Greg, this message is appropriate to the developer only. I suggest to change the log level. > > > > Why can't your system provide a valid irq line? Here, I am more with Andre. This just happens sometimes, esp. in embedded solutions. > > > In our case we have only an I2C available in a connection cable and the > > > GPIOs are linked through a two way level shifter. > > > It was a very special situation in our case because target platform and > > > sensor platform are provided. > > > The IRQ from the sensor war not able to drive the two way level shifter low so > > > we always detect outgoing traffic and the IRQ signal but at the target > > > board after the level shifter the signal remains stable. So > > > communication failed with a timeout. So we need to force polling the > > > interrupt status register because > > > both HW solution should not be changed in any way. > > > > Again, you are burning a TON of power just for a simple little uart, > > with your system never being able to go to sleep, are you sure this is > > something that you want others to emulate and support? But the alternative is even worse: a non-functioning uart. Something that works but consumes too much power is better than something that doesn't even work at all. > I got your point and I'm fully with. This caused me to print a warning > in Kernel log because it should not be the general working method. > In our special case we do not have any other option because the sensor > module using the SC16IS7xx and the hardware with the MCU running Linux OS > are fixed. We had no possibilities to move any GPIO or such. This was > the only chance to support the dedicated sensor platform and I may be > the case that someone else faces the same problems. I thought that > someone else may benefit from this workaround too. But as I got your > point I'm also fine if it is not merged into main Linux Kernel sources. I vote this patch to go in (after modification). Kind regards, Maarten
On Mon, Dec 23, 2024 at 02:27:22PM +0000, Maarten Brock wrote: > > I got your point and I'm fully with. This caused me to print a warning > > in Kernel log because it should not be the general working method. > > In our special case we do not have any other option because the sensor > > module using the SC16IS7xx and the hardware with the MCU running Linux OS > > are fixed. We had no possibilities to move any GPIO or such. This was > > the only chance to support the dedicated sensor platform and I may be > > the case that someone else faces the same problems. I thought that > > someone else may benefit from this workaround too. But as I got your > > point I'm also fine if it is not merged into main Linux Kernel sources. > > I vote this patch to go in (after modification). Ok, let's see a cleaned up version of this and we'll review it again. thanks, greg k-h
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index a3093e09309f..31962fdca178 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -314,6 +314,7 @@ #define SC16IS7XX_FIFO_SIZE (64) #define SC16IS7XX_GPIOS_PER_BANK 4 +#define SC16IS7XX_POLL_PERIOD 10 /*ms*/ #define SC16IS7XX_RECONF_MD BIT(0) #define SC16IS7XX_RECONF_IER BIT(1) #define SC16IS7XX_RECONF_RS485 BIT(2) @@ -348,6 +349,9 @@ struct sc16is7xx_port { u8 mctrl_mask; struct kthread_worker kworker; struct task_struct *kworker_task; + struct kthread_delayed_work poll_work; + bool polling; + bool shutdown; struct sc16is7xx_one p[]; }; @@ -861,6 +865,19 @@ static irqreturn_t sc16is7xx_irq(int irq, void *dev_id) return IRQ_HANDLED; } +static void sc16is7xx_transmission_poll(struct kthread_work *work) +{ + struct sc16is7xx_port *s = container_of(work, struct sc16is7xx_port, poll_work.work); + + /* Resuse standard IRQ handler. Interrupt ID is unused in this context. */ + sc16is7xx_irq(0, s); + + /* setup delay based on SC16IS7XX_POLL_PERIOD */ + if (!s->shutdown) + kthread_queue_delayed_work(&s->kworker, &s->poll_work, + msecs_to_jiffies(SC16IS7XX_POLL_PERIOD)); +} + static void sc16is7xx_tx_proc(struct kthread_work *ws) { struct uart_port *port = &(to_sc16is7xx_one(ws, tx_work)->port); @@ -1149,6 +1166,7 @@ static int sc16is7xx_config_rs485(struct uart_port *port, struct ktermios *termi static int sc16is7xx_startup(struct uart_port *port) { struct sc16is7xx_one *one = to_sc16is7xx_one(port, port); + struct sc16is7xx_port *s = dev_get_drvdata(port->dev); unsigned int val; unsigned long flags; @@ -1210,6 +1228,11 @@ static int sc16is7xx_startup(struct uart_port *port) uart_port_lock_irqsave(port, &flags); sc16is7xx_enable_ms(port); uart_port_unlock_irqrestore(port, flags); + if (s->polling) { + s->shutdown = false; + kthread_queue_delayed_work(&s->kworker, &s->poll_work, + msecs_to_jiffies(SC16IS7XX_POLL_PERIOD)); + } return 0; } @@ -1232,6 +1255,10 @@ static void sc16is7xx_shutdown(struct uart_port *port) sc16is7xx_power(port, 0); + if (s->polling) { + s->shutdown = true; + kthread_cancel_delayed_work_sync(&s->poll_work); + } kthread_flush_worker(&s->kworker); } @@ -1537,7 +1564,13 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, /* Always ask for fixed clock rate from a property. */ device_property_read_u32(dev, "clock-frequency", &uartclk); + s->polling = !device_property_present(dev, "interrupts"); + if (s->polling) { + dev_warn(dev, + "No interrupt definition found. Falling back to polling mode.\n"); + irq = 0; + } s->clk = devm_clk_get_optional(dev, NULL); if (IS_ERR(s->clk)) return PTR_ERR(s->clk); @@ -1664,6 +1697,11 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, if (ret) goto out_ports; #endif + if (s->polling) { + /* Initialize Kernel timer for polling */ + kthread_init_delayed_work(&s->poll_work, sc16is7xx_transmission_poll); + return 0; + } /* * Setup interrupt. We first try to acquire the IRQ line as level IRQ. @@ -1724,6 +1762,8 @@ void sc16is7xx_remove(struct device *dev) sc16is7xx_power(&s->p[i].port, 0); } + if (s->polling) + kthread_cancel_delayed_work_sync(&s->poll_work); kthread_flush_worker(&s->kworker); kthread_stop(s->kworker_task);
Fall back to polling mode if no interrupt is configured because not possible. If "interrupts" property is missing in devicetree the driver uses a delayed worker to pull state of interrupt status registers. Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> --- This driver was tested on Linux 5.10. We had a custom board that was not able to connect the interrupt port. Only I2C was available. --- --- drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)