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. > static int of_serdev_register_devices(struct serdev_controller *ctrl) > { > struct device_node *port, *node; > struct serdev_device *serdev = NULL; > struct serport *serport = serdev_controller_get_drvdata(ctrl); > int err; > unsigned reg; > bool found = false; > > for_each_available_child_of_node(ctrl->dev.of_node, port) { > of_property_read_u32(port, "reg", ®); > > if (serport->tty_idx != reg) > continue; > > for_each_available_child_of_node(port, node) { > if (!of_get_property(node, "compatible", NULL)) > continue; > > dev_err(&ctrl->dev, "adding child %pOF\n", node); > > serdev = serdev_device_alloc(ctrl); > if (!serdev) > continue; > > serdev->dev.of_node = node; > > err = serdev_device_add(serdev); > if (err) { > dev_err(&serdev->dev, > "failure adding device. status %d\n", err); > serdev_device_put(serdev); > } else > found = true; > } > } > if (!found) > return -ENODEV; > > return 0; > } > > Anyway, these are the results from my testing. If anyone is interested > in proposing some better solutions or collaborating on some patches, I > am happy to contribute and to do some more testing. [1] https://www.spinics.net/lists/linux-serial/msg31354.html -- 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
