lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Date:   Thu, 18 Nov 2021 15:20:26 +0300
From:   Dvorkin Dmitry <dvorkin@...bo.com>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Wells Lu <wellslutw@...il.com>
Cc:     linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        robh+dt@...nel.org, devicetree@...r.kernel.org, qinjian@...lus1.com
Subject: Re: [PATCH 3/3] devicetree: bindings: pinctrl: Add bindings doc for
 Sunplus SP7021.

Dear Linus!

I am the person who wrote this driver. Let me answer to your questions...

-----Original Message-----
>> From: Linus Walleij <linus.walleij@...aro.org>
>> Sent: Tuesday, November 9, 2021 12:00 PM
>> To: Wells Lu <wellslutw@...il.com>
>> Cc: linux-gpio@...r.kernel.org; linux-kernel@...r.kernel.org;
>> robh+dt@...nel.org; devicetree@...r.kernel.org; qinjian@...lus1.com;
>> dvorkin@...bo.com; Wells Lu 呂芳騰 <wells.lu@...plus.com>
>> Subject: Re: [PATCH 3/3] devicetree: bindings: pinctrl: Add bindings doc for
>> Sunplus SP7021.
>>
>>
>>> +        zero_func:
>>> +          description: |
>>> +            Disabled pins which are not used by pinctrl node's client
>> device.
>>> +          $ref: /schemas/types.yaml#/definitions/uint32-array
>> I have never seen this before. Can't you just use pin control hogs for this so the
>> pin controller just take care of these pins?

zero_func is required.

The bootloader may have different device tree (I am using general sp7021 
DTS in my u-boot setup, for example), while the kernel DTS may be 
changed between boots and specifies it more precisely - it is configured 
by user. So u-boot DTB and kernel DTB may be different -> result is that 
some pins may be muxed wrongly after u-boot starts kernel. Or even in 
pre-u-boot stage (we have the bootloader that starts u-boot, called 
XBoot). This XBoot also do some muxing. So we need this feature to get 
rid of possible unneded muxes done before kernel has been started.

There is the "group of pins" functions and individual pins that may 
intersect.

You may have "group of pins", say, emmc preconfigured before kernel 
started (in general DTS for u-boot) and you may want to have the pin 
from emmc group to be muxed as, say, SD card detect. You mux it in 
kernel DTS as GPIO, it will be in correct GPIO state, configured 
correctly, but while emmc group is enabled (nobody disabled it in kernel 
DTS!) the pin will belong to emmc function (preset group) and will not 
be functional.

I invented zero_func while has been debugging the problem like "why my 
Eth is not working when all pins are configured correctly and muxed to 
Eth". I spend some time to find that the pin I muxed to Eth has been 
muxed to SPI_FLASH GROUP in very early stage (in ROM boot). And I have 
no way to cleanup this mux group easily.

zero_func is the way to easily guarantee that you will successfully and 
correctly mux some pins / functions on kernel load even if somebody 
muxed other pins to this functions before kernel.

If I'd implement "automatic" mux cleanup before muxing some pin, the 
code would be more complex. I would like to keep code as simple as I can 
and give better control to user.


>>
>>> +      allOf:
>>> +        - if:
>>> +            properties:
>>> +              function:
>>> +                enum:
>>> +                  - SPI_FLASH
>>> +          then:
>>> +            properties:
>>> +              groups:
>>> +                enum:
>>> +                  - SPI_FLASH1
>>> +                  - SPI_FLASH2
>>> +        - if:
>>> +            properties:
>>> +              function:
>>> +                enum:
>>> +                  - SPI_FLASH_4BIT
>>> +          then:
>>> +            properties:
>>> +              groups:
>>> +                enum:
>>> +                  - SPI_FLASH_4BIT1
>>> +                  - SPI_FLASH_4BIT2
>>> +        - if:
>>> +            properties:
>>> +              function:
>>> +                enum:
>>> +                  - HDMI_TX
>>> +          then:
>>> +            properties:
>>> +              groups:
>>> +                enum:
>>> +                  - HDMI_TX1
>>> +                  - HDMI_TX2
>>> +                  - HDMI_TX3
>>> +        - if:
>>> +            properties:
>>> +              function:
>>> +                enum:
>>> +                  - LCDIF
>>> +          then:
>>> +            properties:
>>> +              groups:
>>> +                enum:
>>> +                  - LCDIF
>>>
>>> This looks complex to me, I need feedback from bindings people on this.

sp7021 supports two types of muxes:

1) group muxing (1-N sets of predefined pins for some function)

2) individual pin muxing

Some functions may be muxed only in group, like SPI_FLASH or HDMI.

That's why we have

pins = <...>;

and

function = <funcname>;

group = <funcsubname-group>;

second case could be cuted to

function = <funcsubname-group> only;

But I think, the syntax of a pair {function,group} fits SoC logic 
better. Especially if customer is reading possible muxes table for the chip.


>>>
>>> +        pins_uart0: pins_uart0 {
>>> +            function = "UA0";
>>> +            groups = "UA0";
>>> +        };
>>> +
>>> +        pins_uart1: pins_uart1 {
>>> +            pins = <
>>> +
>> SPPCTL_IOPAD(11,SPPCTL_PCTL_G_PMUX,MUXF_UA1_TX,0)
>>> +
>> SPPCTL_IOPAD(10,SPPCTL_PCTL_G_PMUX,MUXF_UA1_RX,0)
>>> +
>> SPPCTL_IOPAD(7,SPPCTL_PCTL_G_GPIO,0,SPPCTL_PCTL_L_OUT)
>>> +            >;
>>> +        };
>> This first looks like two ways to do the same thing?
>> UART0 uses strings for group + function and uart1 control individual pins.
>>
>> Is it possible to just do it one way?
>>
>> I think the pins = <...> scheme includes also multiplexing settings and then it
>> should be named pinmux = <...>:
No. Sorry. It is two different way of supported two different types of 
muxing, described above.
>>
>> Please read
>> Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml
>> closely.
>>
>> Yours,
>> Linus Walleij

View attachment "dvorkin.vcf" of type "text/x-vcard" (392 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ