Am 26.07.2018 um 16:33 schrieb Rob Herring: > On Wed, Jul 25, 2018 at 2:55 AM Michael Allwright <allsey87@xxxxxxxxx> wrote: >> >> In some hardware I am developing, I have used the SC16IS7XX to connect >> two AVR microcontrollers back to my main processor running Linux. >> These microcontrollers implement an control interface which is >> modelled in Linux as a multi-function device. The set up is based on >> the driver for the RAVE Supervisory Processor: >> https://lwn.net/Articles/724765/ > > The SC16IS7XX is a popular topic suddenly[1]. > >> One issue during initialisation when using serdev with SC16IS7XX is >> that after uart_add_one_port has been called, the child driver may be >> started on a different kernel thread and may start calling the startup >> or set_termios functions of the serial driver before it has finished >> probing. I temporarily resolved this issue with a hack which involved >> adding a "ready" boolean to the driver. However, there are two proper >> solutions to this problem that I can think of: either serdev should >> somehow check that the probe was completed (successfully) before >> trying to load child drivers or the SC16IS7XX should have a mutex that >> better enforces the order in which its functions are called >> (particularly during initialization). My testing suggests that the >> latter may be the better solution, since even outside of the >> initialization context and when using the two ttys heavily at the same >> time, it is not hard to knock the SC16IS7XX device into a state where, >> at a minimum, the silicon stops sending interrupts. I think this >> ordering of function calls may also be partly responsible for >> "sc16is7xx 1-0048: ttySC0: Unexpected interrupt: 8" kernel messages >> that are mentioned in other threads. >> >> To support multiple ttys on a single device, the serdev code and its >> DT binding need to be slightly modified. My solution is to modify the >> code to parse the following: >> >> uart56: sc16is762@48 { >> compatible = "nxp,sc16is762"; >> reg = <0x48>; >> clocks = <&some_clock>; >> interrupt-parent = <&some_gpio_controller>; >> interrupts = <X IRQ_TYPE_EDGE_FALLING>; >> >> #address-cells = <1>; >> #size-cells = <0>; >> >> uart0: serial@0 { >> reg = <0>; >> uc0: microcontroller { > > This structure is what I suggested to Andreas. > >> compatible = "atmel,atmega328p"; >> current-speed = <57600>; >> }; >> }; >> >> uart1: serial@1 { >> reg = <1>; >> uc1: microcontroller { >> compatible = "atmel,atmega328p"; >> current-speed = <57600>; >> }; >> }; >> }; >> >> The code to parse this is as follows and uses the tty_idx to indicate >> which of the sc16is7xx ttys we are referring to. This works nicely >> with a single chip, however, when you have more than one sc16is7xx >> device, the reg value needs to be 2 and 3 for the second chip instead >> 0 and 1 as with the first chip. The second thing that is not so nice >> about this solution is that I needed to move struct serport from >> drivers/tty/serdev/serdev-ttyport.c to include/linux/serdev.h so that >> I could access tty_idx from drivers/tty/serdev/core.c. > > This is a problem. > > This needs to be solved by having the serdev ctrlr DT node be uart0/1 > nodes rather than the parent node. Then the current core code should > work and you don't need reg to equal the tty index which is wrong. I had stared at the sc16is7xx code without spotting an obvious way how to implement that. I know how to obtain a child of_node, but the uart_port actually stores a full dev pointer. Are you suggesting we have three different devices, 1 spi_device, N whatever for the ports? If so, how to create them? As long as dev_get_drvdata() still returns the expected struct I think it could work then. Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
