Question about SC16IS752 device tree.

Question about SC16IS752 device tree.

[Zhou Yanjie]

Hi folks,

I encountered a problem when using the SC16IS752 to expand the serial port.
I connected two Bluetooth modules to the two serial ports extended by the
SC16IS752. The device tree is as follows:

&ssi0 {
     status = "okay";

     num-cs = <2>;

     pinctrl-names = "default";
     pinctrl-0 = <&pins_ssi0>;

     sc16is752: expander@0 {
         compatible = "nxp,sc16is752";
         reg = <0>; /* CE0 */

         spi-rx-bus-width = <1>;
         spi-tx-bus-width = <1>;
         spi-max-frequency = <6000000>;

         clocks = <&exclk_sc16is752>;

         interrupt-parent = <&gpb>;
         interrupts = <18 IRQ_TYPE_EDGE_FALLING>;

         gpio-controller;
         #gpio-cells = <2>;

         bluetooth@0 {
             compatible = "brcm,bcm43438-bt";
             max-speed = <1000000>;

             device-wakeup-gpios = <&gpc 26 GPIO_ACTIVE_HIGH>;
             reset-gpios = <&gpb 17 GPIO_ACTIVE_LOW>;
         };

         bluetooth@1 {
             compatible = "brcm,bcm43438-bt";

             device-wakeup-gpios = <&gpc 28 GPIO_ACTIVE_HIGH>;
             reset-gpios = <&gpb 19 GPIO_ACTIVE_LOW>;
         };
     };
};



There are the following error messages after startup:

[    0.548417] serial serial0-0: controller busy
[    0.553572] serial serial0-0: failure adding device. status -EBUSY
[    0.559764] serial serial0: tty port ttySC0 registered
[    0.565545] spi0.0: ttySC1 at I/O 0x1 (irq = 18, base_baud = 3000000) 
is a SC16IS752
[    0.573987] serial serial1-0: controller busy
[    0.578351] serial serial1-0: failure adding device. status -EBUSY
[    0.585003] serial serial1: tty port ttySC1 registered

And only the module connected to the first serial port (ttySC0) can work 
normally.



If I change the device tree to:

&ssi0 {
     status = "okay";

     num-cs = <2>;

     pinctrl-names = "default";
     pinctrl-0 = <&pins_ssi0>;

     sc16is752: expander@0 {
         compatible = "nxp,sc16is752";
         reg = <0>; /* CE0 */

         spi-rx-bus-width = <1>;
         spi-tx-bus-width = <1>;
         spi-max-frequency = <6000000>;

         clocks = <&exclk_sc16is752>;

         interrupt-parent = <&gpb>;
         interrupts = <18 IRQ_TYPE_EDGE_FALLING>;

         gpio-controller;
         #gpio-cells = <2>;

         bluetooth@0 {
             compatible = "brcm,bcm43438-bt";
             max-speed = <1000000>;

             device-wakeup-gpios = <&gpc 26 GPIO_ACTIVE_HIGH>;
             reset-gpios = <&gpb 17 GPIO_ACTIVE_LOW>;
         };
     };
};

Then there will be no error message, and the module connected to the first
serial port (ttySC0) can also work normally.



After tracing, the problem seems to be in "serdev_device_add()" (line 
111) of
"drivers/tty/serdev/core.c":

int serdev_device_add(struct serdev_device *serdev)
{
     struct serdev_controller *ctrl = serdev->ctrl;
     struct device *parent = serdev->dev.parent;
     int err;

     dev_set_name(&serdev->dev, "%s-%d", dev_name(parent), serdev->nr);

     /* Only a single slave device is currently supported. */
     if (ctrl->serdev) {
         dev_err(&serdev->dev, "controller busy\n");
         return -EBUSY;
     }
     ctrl->serdev = serdev;

     err = device_add(&serdev->dev);
     if (err < 0) {
         dev_err(&serdev->dev, "Can't add %s, status %pe\n",
             dev_name(&serdev->dev), ERR_PTR(err));
         goto err_clear_serdev;
     }

     dev_dbg(&serdev->dev, "device %s registered\n", 
dev_name(&serdev->dev));

     return 0;

err_clear_serdev:
     ctrl->serdev = NULL;
     return err;
}
EXPORT_SYMBOL_GPL(serdev_device_add);



Is there any way to correctly describe the device mounted on the second
serial port (ttySC1) in the device tree? Or how do I need to modify the
"drivers/tty/serdev/core.c" to make the SC16IS752 still work properly
with two child nodes mounted?

Thanks and beset regards!

Re: Question about SC16IS752 device tree.

[Paul Cercueil]

Hi Zhou,

Le mar., mai 10 2022 at 02:08:28 +0800, Zhou Yanjie 
<zhouyanjie@wanyeetech.com> a écrit :
> Hi folks,
> 
> I encountered a problem when using the SC16IS752 to expand the serial 
> port.
> I connected two Bluetooth modules to the two serial ports extended by 
> the
> SC16IS752. The device tree is as follows:
> 
> &ssi0 {
>     status = "okay";
> 
>     num-cs = <2>;
> 
>     pinctrl-names = "default";
>     pinctrl-0 = <&pins_ssi0>;
> 
>     sc16is752: expander@0 {
>         compatible = "nxp,sc16is752";
>         reg = <0>; /* CE0 */
> 
>         spi-rx-bus-width = <1>;
>         spi-tx-bus-width = <1>;
>         spi-max-frequency = <6000000>;
> 
>         clocks = <&exclk_sc16is752>;
> 
>         interrupt-parent = <&gpb>;
>         interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
> 
>         gpio-controller;
>         #gpio-cells = <2>;
> 
>         bluetooth@0 {
>             compatible = "brcm,bcm43438-bt";
>             max-speed = <1000000>;
> 
>             device-wakeup-gpios = <&gpc 26 GPIO_ACTIVE_HIGH>;
>             reset-gpios = <&gpb 17 GPIO_ACTIVE_LOW>;
>         };
> 
>         bluetooth@1 {
>             compatible = "brcm,bcm43438-bt";
> 
>             device-wakeup-gpios = <&gpc 28 GPIO_ACTIVE_HIGH>;
>             reset-gpios = <&gpb 19 GPIO_ACTIVE_LOW>;
>         };
>     };
> };
> 
> 
> 
> There are the following error messages after startup:
> 
> [    0.548417] serial serial0-0: controller busy
> [    0.553572] serial serial0-0: failure adding device. status -EBUSY
> [    0.559764] serial serial0: tty port ttySC0 registered
> [    0.565545] spi0.0: ttySC1 at I/O 0x1 (irq = 18, base_baud = 
> 3000000) is a SC16IS752
> [    0.573987] serial serial1-0: controller busy
> [    0.578351] serial serial1-0: failure adding device. status -EBUSY
> [    0.585003] serial serial1: tty port ttySC1 registered
> 
> And only the module connected to the first serial port (ttySC0) can 
> work normally.

I can't say for sure that it's your problem, but your bluetooth nodes 
are missing "reg" properties.

Cheers,
-Paul

> 
> 
> 
> If I change the device tree to:
> 
> &ssi0 {
>     status = "okay";
> 
>     num-cs = <2>;
> 
>     pinctrl-names = "default";
>     pinctrl-0 = <&pins_ssi0>;
> 
>     sc16is752: expander@0 {
>         compatible = "nxp,sc16is752";
>         reg = <0>; /* CE0 */
> 
>         spi-rx-bus-width = <1>;
>         spi-tx-bus-width = <1>;
>         spi-max-frequency = <6000000>;
> 
>         clocks = <&exclk_sc16is752>;
> 
>         interrupt-parent = <&gpb>;
>         interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
> 
>         gpio-controller;
>         #gpio-cells = <2>;
> 
>         bluetooth@0 {
>             compatible = "brcm,bcm43438-bt";
>             max-speed = <1000000>;
> 
>             device-wakeup-gpios = <&gpc 26 GPIO_ACTIVE_HIGH>;
>             reset-gpios = <&gpb 17 GPIO_ACTIVE_LOW>;
>         };
>     };
> };
> 
> Then there will be no error message, and the module connected to the 
> first
> serial port (ttySC0) can also work normally.
> 
> 
> 
> After tracing, the problem seems to be in "serdev_device_add()" (line 
> 111) of
> "drivers/tty/serdev/core.c":
> 
> int serdev_device_add(struct serdev_device *serdev)
> {
>     struct serdev_controller *ctrl = serdev->ctrl;
>     struct device *parent = serdev->dev.parent;
>     int err;
> 
>     dev_set_name(&serdev->dev, "%s-%d", dev_name(parent), serdev->nr);
> 
>     /* Only a single slave device is currently supported. */
>     if (ctrl->serdev) {
>         dev_err(&serdev->dev, "controller busy\n");
>         return -EBUSY;
>     }
>     ctrl->serdev = serdev;
> 
>     err = device_add(&serdev->dev);
>     if (err < 0) {
>         dev_err(&serdev->dev, "Can't add %s, status %pe\n",
>             dev_name(&serdev->dev), ERR_PTR(err));
>         goto err_clear_serdev;
>     }
> 
>     dev_dbg(&serdev->dev, "device %s registered\n", 
> dev_name(&serdev->dev));
> 
>     return 0;
> 
> err_clear_serdev:
>     ctrl->serdev = NULL;
>     return err;
> }
> EXPORT_SYMBOL_GPL(serdev_device_add);
> 
> 
> 
> Is there any way to correctly describe the device mounted on the 
> second
> serial port (ttySC1) in the device tree? Or how do I need to modify 
> the
> "drivers/tty/serdev/core.c" to make the SC16IS752 still work properly
> with two child nodes mounted?
> 
> Thanks and beset regards!

Re: Question about SC16IS752 device tree.

[Zhou Yanjie]

Hi Paul,

On 2022/5/10 上午2:13, Paul Cercueil wrote:
> I can't say for sure that it's your problem, but your bluetooth nodes 
> are missing "reg" properties. 


Unfortunately it doesn't seem to be the problem here, I added "reg" and
the problem persists, and I've looked at other device trees that contain
"brcm,bcm43438-bt", none of them use "reg", and "reg" is not mentioned in
neither "Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt" nor
"Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml".


Best regards!

Re: Question about SC16IS752 device tree.

[H. Nikolaus Schaller]

Hi,

> Am 09.05.2022 um 20:41 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
> 
> Hi Paul,
> 
> On 2022/5/10 上午2:13, Paul Cercueil wrote:
>> I can't say for sure that it's your problem, but your bluetooth nodes are missing "reg" properties. 
> 
> 
> Unfortunately it doesn't seem to be the problem here, I added "reg" and
> the problem persists, and I've looked at other device trees that contain
> "brcm,bcm43438-bt", none of them use "reg", and "reg" is not mentioned in
> neither "Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt" nor
> "Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml".

what happens if you remove the serdev children from DTS? Does the driver create two separate /dev/tty ports? And do they work?

Maybe the sc16is752 driver does not separate them for child nodes, i.e. while "reg" should be added it may not be handled?

BR,
Nikolaus

Re: Question about SC16IS752 device tree.

[Zhou Yanjie]

Hi Nikolaus,

On 2022/5/10 上午4:19, H. Nikolaus Schaller wrote:
> Hi,
>
>> Am 09.05.2022 um 20:41 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
>>
>> Hi Paul,
>>
>> On 2022/5/10 上午2:13, Paul Cercueil wrote:
>>> I can't say for sure that it's your problem, but your bluetooth nodes are missing "reg" properties.
>>
>> Unfortunately it doesn't seem to be the problem here, I added "reg" and
>> the problem persists, and I've looked at other device trees that contain
>> "brcm,bcm43438-bt", none of them use "reg", and "reg" is not mentioned in
>> neither "Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt" nor
>> "Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml".
> what happens if you remove the serdev children from DTS? Does the driver create two separate /dev/tty ports? And do they work?


Yes, there will be two separate /dev/tty ports (ttySC0 and ttySC1), and
both ports can work normally, but at this time the two bluetooth modules
are not working.

I guess it is because the driver does not detect bluetooth module nodes,
so the inability to operate "reset-gpios" and "device-wakeup-gpios" causes
the bluetooth module to work incorrectly.


>
> Maybe the sc16is752 driver does not separate them for child nodes, i.e. while "reg" should be added it may not be handled?


I'm not too sure, I'm not very familiar with serial port systems.
If the truth is what you think, how should I improve it?


Best regards!


>
> BR,
> Nikolaus

Re: Question about SC16IS752 device tree.

[H. Nikolaus Schaller]

Hi,

> Am 10.05.2022 um 04:29 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
> 
> Hi Nikolaus,
> 
> On 2022/5/10 上午4:19, H. Nikolaus Schaller wrote:
>> Hi,
>> 
>>> Am 09.05.2022 um 20:41 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
>>> 
>>> Hi Paul,
>>> 
>>> On 2022/5/10 上午2:13, Paul Cercueil wrote:
>>>> I can't say for sure that it's your problem, but your bluetooth nodes are missing "reg" properties.
>>> 
>>> Unfortunately it doesn't seem to be the problem here, I added "reg" and
>>> the problem persists, and I've looked at other device trees that contain
>>> "brcm,bcm43438-bt", none of them use "reg", and "reg" is not mentioned in
>>> neither "Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt" nor
>>> "Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml".
>> what happens if you remove the serdev children from DTS? Does the driver create two separate /dev/tty ports? And do they work?
> 
> 
> Yes, there will be two separate /dev/tty ports (ttySC0 and ttySC1), and
> both ports can work normally, but at this time the two bluetooth modules
> are not working.
> 
> I guess it is because the driver does not detect bluetooth module nodes,
> so the inability to operate "reset-gpios" and "device-wakeup-gpios" causes
> the bluetooth module to work incorrectly.

I would assume that it is not prepared to handle two serdev subnodes and
assign the right gpios.

> 
> 
>> 
>> Maybe the sc16is752 driver does not separate them for child nodes, i.e. while "reg" should be added it may not be handled?
> 
> 
> I'm not too sure, I'm not very familiar with serial port systems.
> If the truth is what you think, how should I improve it?

Unfortunately I also don't know how the serdev implementation really works.

It was my nagging to make it happen by persistently proposing a non-universal
solutionsome years ago until one of the maintainers had mercy to write a general
solution. So I could switch my driver to simply use the serdev API. It was for a GPS
client device but not a tty side driver.

I think if you look up the first patches for the serdev interface this should
reveal the original author an he should be able to help.

BR,
Nikolaus

Re: Question about SC16IS752 device tree.

[Zhou Yanjie]

Hi,

On 2022/5/10 下午11:31, H. Nikolaus Schaller wrote:
> Hi,
>
>> Am 10.05.2022 um 04:29 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
>>
>> Hi Nikolaus,
>>
>> On 2022/5/10 上午4:19, H. Nikolaus Schaller wrote:
>>> Hi,
>>>
>>>> Am 09.05.2022 um 20:41 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
>>>>
>>>> Hi Paul,
>>>>
>>>> On 2022/5/10 上午2:13, Paul Cercueil wrote:
>>>>> I can't say for sure that it's your problem, but your bluetooth nodes are missing "reg" properties.
>>>> Unfortunately it doesn't seem to be the problem here, I added "reg" and
>>>> the problem persists, and I've looked at other device trees that contain
>>>> "brcm,bcm43438-bt", none of them use "reg", and "reg" is not mentioned in
>>>> neither "Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt" nor
>>>> "Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml".
>>> what happens if you remove the serdev children from DTS? Does the driver create two separate /dev/tty ports? And do they work?
>>
>> Yes, there will be two separate /dev/tty ports (ttySC0 and ttySC1), and
>> both ports can work normally, but at this time the two bluetooth modules
>> are not working.
>>
>> I guess it is because the driver does not detect bluetooth module nodes,
>> so the inability to operate "reset-gpios" and "device-wakeup-gpios" causes
>> the bluetooth module to work incorrectly.
> I would assume that it is not prepared to handle two serdev subnodes and
> assign the right gpios.


I found something new now, if I follow the practice in 
"fsl-ls1012a-frdm.dts"
and put the clock node inside the node of SC16IS752:

&ssi0 {
     status = "okay";

     num-cs = <2>;

     pinctrl-names = "default";
     pinctrl-0 = <&pins_ssi0>;

     sc16is752: expander@0 {
         compatible = "nxp,sc16is752";
         reg = <0>; /* CE0 */
         #address-cells = <1>;
         #size-cells = <0>;

         spi-rx-bus-width = <1>;
         spi-tx-bus-width = <1>;
         spi-max-frequency = <6000000>;

         clocks = <&exclk_sc16is752>;

         interrupt-parent = <&gpb>;
         interrupts = <18 IRQ_TYPE_EDGE_FALLING>;

         gpio-controller;
         #gpio-cells = <2>;

         exclk_sc16is752: sc16is752 {
             compatible = "fixed-clock";
             #clock-cells = <0>;
             clock-frequency = <48000000>;
         };

         bluetooth@0 {
             compatible = "brcm,bcm43438-bt";
             reg = <0>;
             max-speed = <1000000>;

             device-wakeup-gpios = <&gpc 26 GPIO_ACTIVE_HIGH>;
             reset-gpios = <&gpb 17 GPIO_ACTIVE_LOW>;
         };

         bluetooth@1 {
             compatible = "brcm,bcm43438-bt";
             reg = <1>;
             max-speed = <1000000>;

             device-wakeup-gpios = <&gpc 28 GPIO_ACTIVE_HIGH>;
             reset-gpios = <&gpb 19 GPIO_ACTIVE_LOW>;
         };
     };
};

This will cause all bluetooth modules to not work, and if the clock node 
is moved
to the end of the child node, the bluetooth module connected to ttySC0 
can work
normally, which seems to mean that only the first child node can work 
correctly.



And I found this patch:

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/tty/serdev?h=usb-next&id=08fcee289f341786eb3b44e5f2d1dc850943238e

It seems to mean that the SC16IS752 driver does not correctly 
distinguish between
the two serial ports, which makes the serdev driver think that the child 
nodes are
on the same serial device bus, which leads to the current problem.


>
>>
>>> Maybe the sc16is752 driver does not separate them for child nodes, i.e. while "reg" should be added it may not be handled?
>>
>> I'm not too sure, I'm not very familiar with serial port systems.
>> If the truth is what you think, how should I improve it?
> Unfortunately I also don't know how the serdev implementation really works.
>
> It was my nagging to make it happen by persistently proposing a non-universal
> solutionsome years ago until one of the maintainers had mercy to write a general
> solution. So I could switch my driver to simply use the serdev API. It was for a GPS
> client device but not a tty side driver.
>
> I think if you look up the first patches for the serdev interface this should
> reveal the original author an he should be able to help.


The original author of the serdev driver is Rob Herring, the original 
author of the
SC16IS752 is Jon Ringle, they are already on the CC list, I also added 
Johan Hovold
and the two authors Tomasz Moń and Lech Percza who sent patches to the 
sc16is7xx.c
driver in this year.

Hopefully they can guide us here.


Best regards!


>
> BR,
> Nikolaus

Re: Question about SC16IS752 device tree.

[Rob Herring]

On Tue, May 10, 2022 at 12:53 PM Zhou Yanjie <zhouyanjie@wanyeetech.com> wrote:
>
> Hi,
>
> On 2022/5/10 下午11:31, H. Nikolaus Schaller wrote:
> > Hi,
> >
> >> Am 10.05.2022 um 04:29 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
> >>
> >> Hi Nikolaus,
> >>
> >> On 2022/5/10 上午4:19, H. Nikolaus Schaller wrote:
> >>> Hi,
> >>>
> >>>> Am 09.05.2022 um 20:41 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
> >>>>
> >>>> Hi Paul,
> >>>>
> >>>> On 2022/5/10 上午2:13, Paul Cercueil wrote:
> >>>>> I can't say for sure that it's your problem, but your bluetooth nodes are missing "reg" properties.
> >>>> Unfortunately it doesn't seem to be the problem here, I added "reg" and
> >>>> the problem persists, and I've looked at other device trees that contain
> >>>> "brcm,bcm43438-bt", none of them use "reg", and "reg" is not mentioned in
> >>>> neither "Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt" nor
> >>>> "Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml".
> >>> what happens if you remove the serdev children from DTS? Does the driver create two separate /dev/tty ports? And do they work?
> >>
> >> Yes, there will be two separate /dev/tty ports (ttySC0 and ttySC1), and
> >> both ports can work normally, but at this time the two bluetooth modules
> >> are not working.
> >>
> >> I guess it is because the driver does not detect bluetooth module nodes,
> >> so the inability to operate "reset-gpios" and "device-wakeup-gpios" causes
> >> the bluetooth module to work incorrectly.
> > I would assume that it is not prepared to handle two serdev subnodes and
> > assign the right gpios.
>
>
> I found something new now, if I follow the practice in
> "fsl-ls1012a-frdm.dts"
> and put the clock node inside the node of SC16IS752:
>
> &ssi0 {
>      status = "okay";
>
>      num-cs = <2>;
>
>      pinctrl-names = "default";
>      pinctrl-0 = <&pins_ssi0>;
>
>      sc16is752: expander@0 {
>          compatible = "nxp,sc16is752";
>          reg = <0>; /* CE0 */
>          #address-cells = <1>;
>          #size-cells = <0>;
>
>          spi-rx-bus-width = <1>;
>          spi-tx-bus-width = <1>;
>          spi-max-frequency = <6000000>;
>
>          clocks = <&exclk_sc16is752>;
>
>          interrupt-parent = <&gpb>;
>          interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
>
>          gpio-controller;
>          #gpio-cells = <2>;
>
>          exclk_sc16is752: sc16is752 {
>              compatible = "fixed-clock";
>              #clock-cells = <0>;
>              clock-frequency = <48000000>;
>          };

That doesn't look right. This clock source is not part of or coming
from the sc16is752. This belongs at the top level.

>
>          bluetooth@0 {
>              compatible = "brcm,bcm43438-bt";
>              reg = <0>;
>              max-speed = <1000000>;
>
>              device-wakeup-gpios = <&gpc 26 GPIO_ACTIVE_HIGH>;
>              reset-gpios = <&gpb 17 GPIO_ACTIVE_LOW>;
>          };
>
>          bluetooth@1 {
>              compatible = "brcm,bcm43438-bt";
>              reg = <1>;
>              max-speed = <1000000>;
>
>              device-wakeup-gpios = <&gpc 28 GPIO_ACTIVE_HIGH>;
>              reset-gpios = <&gpb 19 GPIO_ACTIVE_LOW>;
>          };
>      };
> };
>
> This will cause all bluetooth modules to not work, and if the clock node
> is moved
> to the end of the child node, the bluetooth module connected to ttySC0
> can work
> normally, which seems to mean that only the first child node can work
> correctly.
>
>
>
> And I found this patch:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/tty/serdev?h=usb-next&id=08fcee289f341786eb3b44e5f2d1dc850943238e
>
> It seems to mean that the SC16IS752 driver does not correctly
> distinguish between
> the two serial ports, which makes the serdev driver think that the child
> nodes are
> on the same serial device bus, which leads to the current problem.
>
>
> >
> >>
> >>> Maybe the sc16is752 driver does not separate them for child nodes, i.e. while "reg" should be added it may not be handled?
> >>
> >> I'm not too sure, I'm not very familiar with serial port systems.
> >> If the truth is what you think, how should I improve it?
> > Unfortunately I also don't know how the serdev implementation really works.
> >
> > It was my nagging to make it happen by persistently proposing a non-universal
> > solutionsome years ago until one of the maintainers had mercy to write a general
> > solution. So I could switch my driver to simply use the serdev API. It was for a GPS
> > client device but not a tty side driver.
> >
> > I think if you look up the first patches for the serdev interface this should
> > reveal the original author an he should be able to help.
>
>
> The original author of the serdev driver is Rob Herring, the original
> author of the
> SC16IS752 is Jon Ringle, they are already on the CC list, I also added
> Johan Hovold
> and the two authors Tomasz Moń and Lech Percza who sent patches to the
> sc16is7xx.c
> driver in this year.
>
> Hopefully they can guide us here.

I think what needs to happen is of_serdev_register_devices() needs to
be passed the port index which can then be used to get the child with
a matching address/index.

There's not any DT binding that defines how this looks. It could be
either the slave devices are direct child nodes like you have or each
serial port should have a child node for the port and the grandchild
nodes are the slave device. I'd suppose it is possible to have
multiple devices muxed to a single port (that's what the comment is
about and handling muxed devices would require more work in serdev).
That binding would end up looking just like the former style and the
serdev core could have a hard time figuring out whether it is multiple
ports or multiple mux settings. I suppose we would be able to
distinguish that with the presence of mux-control binding or not. In
any case, all that needs to be considered before we change serdev.

Rob

Re: Question about SC16IS752 device tree.

[Zhou Yanjie]

Hi Rob,

On 2022/5/12 下午10:49, Rob Herring wrote:
> On Tue, May 10, 2022 at 12:53 PM Zhou Yanjie <zhouyanjie@wanyeetech.com> wrote:
>> Hi,
>>
>> On 2022/5/10 下午11:31, H. Nikolaus Schaller wrote:
>>> Hi,
>>>
>>>> Am 10.05.2022 um 04:29 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
>>>>
>>>> Hi Nikolaus,
>>>>
>>>> On 2022/5/10 上午4:19, H. Nikolaus Schaller wrote:
>>>>> Hi,
>>>>>
>>>>>> Am 09.05.2022 um 20:41 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
>>>>>>
>>>>>> Hi Paul,
>>>>>>
>>>>>> On 2022/5/10 上午2:13, Paul Cercueil wrote:
>>>>>>> I can't say for sure that it's your problem, but your bluetooth nodes are missing "reg" properties.
>>>>>> Unfortunately it doesn't seem to be the problem here, I added "reg" and
>>>>>> the problem persists, and I've looked at other device trees that contain
>>>>>> "brcm,bcm43438-bt", none of them use "reg", and "reg" is not mentioned in
>>>>>> neither "Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt" nor
>>>>>> "Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml".
>>>>> what happens if you remove the serdev children from DTS? Does the driver create two separate /dev/tty ports? And do they work?
>>>> Yes, there will be two separate /dev/tty ports (ttySC0 and ttySC1), and
>>>> both ports can work normally, but at this time the two bluetooth modules
>>>> are not working.
>>>>
>>>> I guess it is because the driver does not detect bluetooth module nodes,
>>>> so the inability to operate "reset-gpios" and "device-wakeup-gpios" causes
>>>> the bluetooth module to work incorrectly.
>>> I would assume that it is not prepared to handle two serdev subnodes and
>>> assign the right gpios.
>>
>> I found something new now, if I follow the practice in
>> "fsl-ls1012a-frdm.dts"
>> and put the clock node inside the node of SC16IS752:
>>
>> &ssi0 {
>>       status = "okay";
>>
>>       num-cs = <2>;
>>
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&pins_ssi0>;
>>
>>       sc16is752: expander@0 {
>>           compatible = "nxp,sc16is752";
>>           reg = <0>; /* CE0 */
>>           #address-cells = <1>;
>>           #size-cells = <0>;
>>
>>           spi-rx-bus-width = <1>;
>>           spi-tx-bus-width = <1>;
>>           spi-max-frequency = <6000000>;
>>
>>           clocks = <&exclk_sc16is752>;
>>
>>           interrupt-parent = <&gpb>;
>>           interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
>>
>>           gpio-controller;
>>           #gpio-cells = <2>;
>>
>>           exclk_sc16is752: sc16is752 {
>>               compatible = "fixed-clock";
>>               #clock-cells = <0>;
>>               clock-frequency = <48000000>;
>>           };
> That doesn't look right. This clock source is not part of or coming
> from the sc16is752. This belongs at the top level.


I saw in the "arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts" file 
that the clock
node was placed inside the SC16IS752 node, and I found that some 
RaspberryPi developers
did the same. I think the reason them do this may be because the clock 
of SC16IS752 is
provided by the oscillator circuit inside the chip (an external crystal 
oscillator is
required). If you feel this is inappropriate, I will leave the clock 
node alone.


>
>>           bluetooth@0 {
>>               compatible = "brcm,bcm43438-bt";
>>               reg = <0>;
>>               max-speed = <1000000>;
>>
>>               device-wakeup-gpios = <&gpc 26 GPIO_ACTIVE_HIGH>;
>>               reset-gpios = <&gpb 17 GPIO_ACTIVE_LOW>;
>>           };
>>
>>           bluetooth@1 {
>>               compatible = "brcm,bcm43438-bt";
>>               reg = <1>;
>>               max-speed = <1000000>;
>>
>>               device-wakeup-gpios = <&gpc 28 GPIO_ACTIVE_HIGH>;
>>               reset-gpios = <&gpb 19 GPIO_ACTIVE_LOW>;
>>           };
>>       };
>> };
>>
>> This will cause all bluetooth modules to not work, and if the clock node
>> is moved
>> to the end of the child node, the bluetooth module connected to ttySC0
>> can work
>> normally, which seems to mean that only the first child node can work
>> correctly.
>>
>>
>>
>> And I found this patch:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/tty/serdev?h=usb-next&id=08fcee289f341786eb3b44e5f2d1dc850943238e
>>
>> It seems to mean that the SC16IS752 driver does not correctly
>> distinguish between
>> the two serial ports, which makes the serdev driver think that the child
>> nodes are
>> on the same serial device bus, which leads to the current problem.
>>
>>
>>>>> Maybe the sc16is752 driver does not separate them for child nodes, i.e. while "reg" should be added it may not be handled?
>>>> I'm not too sure, I'm not very familiar with serial port systems.
>>>> If the truth is what you think, how should I improve it?
>>> Unfortunately I also don't know how the serdev implementation really works.
>>>
>>> It was my nagging to make it happen by persistently proposing a non-universal
>>> solutionsome years ago until one of the maintainers had mercy to write a general
>>> solution. So I could switch my driver to simply use the serdev API. It was for a GPS
>>> client device but not a tty side driver.
>>>
>>> I think if you look up the first patches for the serdev interface this should
>>> reveal the original author an he should be able to help.
>>
>> The original author of the serdev driver is Rob Herring, the original
>> author of the
>> SC16IS752 is Jon Ringle, they are already on the CC list, I also added
>> Johan Hovold
>> and the two authors Tomasz Moń and Lech Percza who sent patches to the
>> sc16is7xx.c
>> driver in this year.
>>
>> Hopefully they can guide us here.
> I think what needs to happen is of_serdev_register_devices() needs to
> be passed the port index which can then be used to get the child with
> a matching address/index.
>
> There's not any DT binding that defines how this looks. It could be
> either the slave devices are direct child nodes like you have or each
> serial port should have a child node for the port and the grandchild
> nodes are the slave device. I'd suppose it is possible to have
> multiple devices muxed to a single port (that's what the comment is
> about and handling muxed devices would require more work in serdev).
> That binding would end up looking just like the former style and the
> serdev core could have a hard time figuring out whether it is multiple
> ports or multiple mux settings. I suppose we would be able to
> distinguish that with the presence of mux-control binding or not. In
> any case, all that needs to be considered before we change serdev.


I think it seems that the grandchild node scheme should be more in line 
with the
current situation, since on further exploration I found these:

/sys/bus/platform/devices/10043000.spi/spi_master/spi0/spi0.0/serial0/serial0-0
/sys/bus/platform/devices/10043000.spi/spi_master/spi0/spi0.0/serial1/serial1-0

This means that for the SC16IS752 chip there are two serial device buses 
(one for
each serial port). The previous experimental results have proved that 
the current
driver does not seem to be able to correctly determine the 
correspondence between
two child nodes and two serial device buses, and when I removed the 2nd 
bluetooth
device (both module hardware and device tree node) and put the clock 
node alone,
I got these:

[    1.208848] Bluetooth: HCI UART driver ver 2.3
[    1.213302] Bluetooth: HCI UART protocol H4 registered
[    1.220201] hci_uart_bcm serial0-0: No reset resource, using default 
baud rate
[    1.227717] Bluetooth: HCI UART protocol Broadcom registered
[    1.240239] hci_uart_bcm: probe of serial1-0 failed with error -16

The device tree at this time looks like this:

&ssi0 {
     status = "okay";

     num-cs = <2>;

     pinctrl-names = "default";
     pinctrl-0 = <&pins_ssi0>;

     sc16is752: expander@0 {
         compatible = "nxp,sc16is752";
         reg = <0>; /* CE0 */

         spi-rx-bus-width = <1>;
         spi-tx-bus-width = <1>;
         spi-max-frequency = <4000000>;

         clocks = <&exclk_sc16is752>;

         interrupt-parent = <&gpb>;
         interrupts = <18 IRQ_TYPE_EDGE_FALLING>;

         gpio-controller;
         #gpio-cells = <2>;

         bluetooth {
          compatible = "brcm,bcm43438-bt";
          max-speed = <1000000>;

          device-wakeup-gpios = <&gpc 26 GPIO_ACTIVE_HIGH>;
          reset-gpios = <&gpb 17 GPIO_ACTIVE_LOW>;
         };
     };
};

I guess this is also because the current driver can't correctly 
determine the
correspondence between the child node and the two serial device buses:

Both serial device buses think that this child node corresponds to 
themselves,
so they both try to register it, but because now ttySC1 (corresponding 
to the
second serial device bus) is not connected to any bluetooth module, 
resulting
in registration failure.

If there are child nodes to represent each port, the correspondence between
the slave devices (grandchild node) and the serial device buses (child node)
will be very clear. But unfortunately, it seems that the current SC16IS752
driver does not support this form (at least there is no relevant information
in the "Example" given in nxp,sc16is7xx.txt).

I'm not too sure what kind of modifications need to be made to the SC16IS752
driver to achieve this, could you please give me some guidance (or 
examples)?


Thanks and best regards!


>
> Rob