Re: Question about SC16IS752 device tree.

[Zhou Yanjie <zhouyanjie@wanyeetech.com>]

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