2022-03-21 21:19:25

by Sui Jingfeng

[permalink] [raw]
Subject: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller

From: suijingfeng <[email protected]>

Signed-off-by: suijingfeng <[email protected]>
Signed-off-by: Sui Jingfeng <[email protected]>
---
.../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
1 file changed, 230 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml

diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
new file mode 100644
index 000000000000..7be63346289e
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
@@ -0,0 +1,230 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
+
+maintainers:
+ - Sui Jingfeng <[email protected]>
+
+description: |+
+
+ Loongson display controllers are simple which require scanout buffers
+ to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
+ memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
+ with a dedicated video RAM which is 64MB or more, precise size can be
+ read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
+ chip.
+
+ LSDC has two display pipes, each way has a DVO interface which provide
+ RGB888 signals, vertical & horizontal synchronisations, data enable and
+ the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
+ 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
+
+ For LS7A1000, there are 4 dedicated GPIOs whose control register is
+ located at the DC register space. They are used to emulate two way i2c,
+ One for DVO0, another for DVO1.
+
+ LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
+ general purpose GPIO emulated i2c or hardware i2c in the SoC.
+
+ LSDC's display pipeline have several components as below description,
+
+ The display controller in LS7A1000:
+ ___________________ _________
+ | -------| | |
+ | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
+ | _ _ -------| ^ ^ |_________|
+ | | | | | -------| | |
+ | |_| |_| | i2c0 <--------+-------------+
+ | -------|
+ | DC IN LS7A1000 |
+ | _ _ -------|
+ | | | | | | i2c1 <--------+-------------+
+ | |_| |_| -------| | | _________
+ | -------| | | | |
+ | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
+ | -------| |_________|
+ |___________________|
+
+ Simple usage of LS7A1000 with LS3A4000 CPU:
+
+ +------+ +-----------------------------------+
+ | DDR4 | | +-------------------+ |
+ +------+ | | PCIe Root complex | LS7A1000 |
+ || MC0 | +--++---------++----+ |
+ +----------+ HT 3.0 | || || |
+ | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
+ | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
+ +----------+ | +--------+ +-+--+-+ +---------+ +------+
+ || MC1 +---------------|--|----------------+
+ +------+ | |
+ | DDR4 | +-------+ DVO0 | | DVO1 +------+
+ +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
+ +-------+ +------+
+
+ The display controller in LS2K1000/LS2K0500:
+ ___________________ _________
+ | -------| | |
+ | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
+ | _ _ -------| ^ ^ |_________|
+ | | | | | | | |
+ | |_| |_| | +------+ |
+ | <---->| i2c0 |<---------+
+ | DC IN LS2K1000 | +------+
+ | _ _ | +------+
+ | | | | | <---->| i2c1 |----------+
+ | |_| |_| | +------+ | _________
+ | -------| | | | |
+ | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
+ | -------| |_________|
+ |___________________|
+
+properties:
+ $nodename:
+ pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
+
+ compatible:
+ oneOf:
+ - items:
+ - enum:
+ - loongson,ls7a1000-dc
+ - loongson,ls2k1000-dc
+ - loongson,ls2k0500-dc
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ i2c-gpio@0:
+ description: |
+ Built-in GPIO emulate i2c exported for external display bridge
+ configuration, onitor detection and edid read back etc, for ls7a1000
+ only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
+ used to specify a I2c adapter bus number, if you don't specify one
+ i2c driver core will dynamically assign a bus number. Please specify
+ it only when its bus number matters. Bus number greater than 6 is safe
+ because ls7a1000 bridge have 6 hardware I2C controller integrated.
+
+ i2c-gpio@1:
+ description: |
+ Built-in GPIO emulate i2c exported for external display bridge
+ configuration, onitor detection and edid read back etc, for ls7a1000
+ only. Its compatible must be lsdc,i2c-gpio-1.
+
+ ports:
+ $ref: /schemas/graph.yaml#/properties/ports
+
+ properties:
+ port@0:
+ $ref: /schemas/graph.yaml#/properties/port
+ description: output port node connected with DPI panels or external encoders, with only one endpoint.
+
+ port@1:
+ $ref: /schemas/graph.yaml#/properties/port
+ description: output port node connected with DPI panels or external encoders, with only one endpoint.
+
+ required:
+ - port@0
+ - port@1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - ports
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ bus {
+
+ #address-cells = <3>;
+ #size-cells = <2>;
+ #interrupt-cells = <2>;
+
+ display-controller@6,1 {
+ compatible = "loongson,ls7a1000-dc";
+ reg = <0x3100 0x0 0x0 0x0 0x0>;
+ interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c-gpio@0 {
+ compatible = "lsdc,i2c-gpio-0";
+ reg = <6>;
+ sda = <0>;
+ scl = <1>;
+ };
+
+ i2c-gpio@1 {
+ compatible = "lsdc,i2c-gpio-1";
+ reg = <7>;
+ sda = <2>;
+ scl = <3>;
+ };
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ port@0 {
+ reg = <0>;
+ endpoint {
+ remote-endpoint = <&vga_encoder_in>;
+ };
+ };
+ port@1 {
+ reg = <1>;
+ endpoint {
+ remote-endpoint = <&dvi_encoder_in>;
+ };
+ };
+ };
+ };
+ };
+
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ bus {
+
+ #address-cells = <3>;
+ #size-cells = <2>;
+ #interrupt-cells = <2>;
+
+ display-controller@6,0 {
+ compatible = "loongson,ls2k1000-dc";
+ reg = <0x3100 0x0 0x0 0x0 0x0>;
+ interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ port@0 {
+ reg = <0>;
+ endpoint {
+ remote-endpoint = <&panel_in>;
+ };
+ };
+ port@1 {
+ reg = <1>;
+ endpoint {
+ remote-endpoint = <&hdmi_encoder_in>;
+ };
+ };
+ };
+ };
+ };
+...
--
2.25.1


2022-03-21 23:59:28

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller

On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
> From: suijingfeng <[email protected]>
>

Needs a commit message.

> Signed-off-by: suijingfeng <[email protected]>
> Signed-off-by: Sui Jingfeng <[email protected]>

Same person? Don't need both emails.

> ---
> .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
> 1 file changed, 230 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> new file mode 100644
> index 000000000000..7be63346289e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> @@ -0,0 +1,230 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
> +
> +maintainers:
> + - Sui Jingfeng <[email protected]>
> +
> +description: |+
> +
> + Loongson display controllers are simple which require scanout buffers
> + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
> + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
> + with a dedicated video RAM which is 64MB or more, precise size can be
> + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
> + chip.
> +
> + LSDC has two display pipes, each way has a DVO interface which provide
> + RGB888 signals, vertical & horizontal synchronisations, data enable and
> + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
> + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
> +
> + For LS7A1000, there are 4 dedicated GPIOs whose control register is
> + located at the DC register space. They are used to emulate two way i2c,
> + One for DVO0, another for DVO1.
> +
> + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
> + general purpose GPIO emulated i2c or hardware i2c in the SoC.
> +
> + LSDC's display pipeline have several components as below description,
> +
> + The display controller in LS7A1000:
> + ___________________ _________
> + | -------| | |
> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> + | _ _ -------| ^ ^ |_________|
> + | | | | | -------| | |
> + | |_| |_| | i2c0 <--------+-------------+
> + | -------|
> + | DC IN LS7A1000 |
> + | _ _ -------|
> + | | | | | | i2c1 <--------+-------------+
> + | |_| |_| -------| | | _________
> + | -------| | | | |
> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> + | -------| |_________|
> + |___________________|
> +
> + Simple usage of LS7A1000 with LS3A4000 CPU:
> +
> + +------+ +-----------------------------------+
> + | DDR4 | | +-------------------+ |
> + +------+ | | PCIe Root complex | LS7A1000 |
> + || MC0 | +--++---------++----+ |
> + +----------+ HT 3.0 | || || |
> + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
> + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
> + +----------+ | +--------+ +-+--+-+ +---------+ +------+
> + || MC1 +---------------|--|----------------+
> + +------+ | |
> + | DDR4 | +-------+ DVO0 | | DVO1 +------+
> + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
> + +-------+ +------+
> +
> + The display controller in LS2K1000/LS2K0500:
> + ___________________ _________
> + | -------| | |
> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> + | _ _ -------| ^ ^ |_________|
> + | | | | | | | |
> + | |_| |_| | +------+ |
> + | <---->| i2c0 |<---------+
> + | DC IN LS2K1000 | +------+
> + | _ _ | +------+
> + | | | | | <---->| i2c1 |----------+
> + | |_| |_| | +------+ | _________
> + | -------| | | | |
> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> + | -------| |_________|
> + |___________________|
> +
> +properties:
> + $nodename:
> + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
> +
> + compatible:
> + oneOf:
> + - items:
> + - enum:
> + - loongson,ls7a1000-dc
> + - loongson,ls2k1000-dc
> + - loongson,ls2k0500-dc
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> + i2c-gpio@0:
> + description: |
> + Built-in GPIO emulate i2c exported for external display bridge

If you have i2c-gpio, that belongs at the DT top-level, not here.

> + configuration, onitor detection and edid read back etc, for ls7a1000
> + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be

No, there's a defined i2c-gpio compatible already.

> + used to specify a I2c adapter bus number, if you don't specify one
> + i2c driver core will dynamically assign a bus number. Please specify

Bus numbers are a linux detail not relevant to DT binding.

> + it only when its bus number matters. Bus number greater than 6 is safe
> + because ls7a1000 bridge have 6 hardware I2C controller integrated.
> +
> + i2c-gpio@1:
> + description: |
> + Built-in GPIO emulate i2c exported for external display bridge
> + configuration, onitor detection and edid read back etc, for ls7a1000
> + only. Its compatible must be lsdc,i2c-gpio-1.
> +
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
> +
> + properties:
> + port@0:
> + $ref: /schemas/graph.yaml#/properties/port
> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> +
> + port@1:
> + $ref: /schemas/graph.yaml#/properties/port
> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> +
> + required:
> + - port@0
> + - port@1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + bus {
> +
> + #address-cells = <3>;
> + #size-cells = <2>;
> + #interrupt-cells = <2>;
> +
> + display-controller@6,1 {
> + compatible = "loongson,ls7a1000-dc";
> + reg = <0x3100 0x0 0x0 0x0 0x0>;
> + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + i2c-gpio@0 {
> + compatible = "lsdc,i2c-gpio-0";
> + reg = <6>;
> + sda = <0>;
> + scl = <1>;
> + };
> +
> + i2c-gpio@1 {
> + compatible = "lsdc,i2c-gpio-1";
> + reg = <7>;
> + sda = <2>;
> + scl = <3>;
> + };
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
> + reg = <0>;
> + endpoint {
> + remote-endpoint = <&vga_encoder_in>;
> + };
> + };
> + port@1 {
> + reg = <1>;
> + endpoint {
> + remote-endpoint = <&dvi_encoder_in>;
> + };
> + };
> + };
> + };
> + };
> +
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + bus {
> +
> + #address-cells = <3>;
> + #size-cells = <2>;
> + #interrupt-cells = <2>;
> +
> + display-controller@6,0 {
> + compatible = "loongson,ls2k1000-dc";
> + reg = <0x3100 0x0 0x0 0x0 0x0>;
> + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
> + reg = <0>;
> + endpoint {
> + remote-endpoint = <&panel_in>;
> + };
> + };
> + port@1 {
> + reg = <1>;
> + endpoint {
> + remote-endpoint = <&hdmi_encoder_in>;
> + };
> + };
> + };
> + };
> + };
> +...
> --
> 2.25.1
>
>

2022-03-22 03:00:53

by Sui Jingfeng

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller


On 2022/3/22 07:20, Rob Herring wrote:
> On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
>> From: suijingfeng <[email protected]>
>>
> Needs a commit message.
>
>> Signed-off-by: suijingfeng <[email protected]>
>> Signed-off-by: Sui Jingfeng <[email protected]>
> Same person? Don't need both emails.

Yes,  [email protected] is my company's email. But it can not be
used to send patches to dri-devel,

when send patches with this email, the patch will not be shown on patch
works.

Emails  are either blocked or got  rejected  by loongson's mail server. 
It can only receive emails

from you and other people, but not dri-devel. so have to use my personal
email([email protected]) to send patches.

>> ---
>> .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
>> 1 file changed, 230 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>> new file mode 100644
>> index 000000000000..7be63346289e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>> @@ -0,0 +1,230 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
>> +
>> +maintainers:
>> + - Sui Jingfeng <[email protected]>
>> +
>> +description: |+
>> +
>> + Loongson display controllers are simple which require scanout buffers
>> + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
>> + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
>> + with a dedicated video RAM which is 64MB or more, precise size can be
>> + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
>> + chip.
>> +
>> + LSDC has two display pipes, each way has a DVO interface which provide
>> + RGB888 signals, vertical & horizontal synchronisations, data enable and
>> + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
>> + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
>> +
>> + For LS7A1000, there are 4 dedicated GPIOs whose control register is
>> + located at the DC register space. They are used to emulate two way i2c,
>> + One for DVO0, another for DVO1.
>> +
>> + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
>> + general purpose GPIO emulated i2c or hardware i2c in the SoC.
>> +
>> + LSDC's display pipeline have several components as below description,
>> +
>> + The display controller in LS7A1000:
>> + ___________________ _________
>> + | -------| | |
>> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
>> + | _ _ -------| ^ ^ |_________|
>> + | | | | | -------| | |
>> + | |_| |_| | i2c0 <--------+-------------+
>> + | -------|
>> + | DC IN LS7A1000 |
>> + | _ _ -------|
>> + | | | | | | i2c1 <--------+-------------+
>> + | |_| |_| -------| | | _________
>> + | -------| | | | |
>> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
>> + | -------| |_________|
>> + |___________________|
>> +
>> + Simple usage of LS7A1000 with LS3A4000 CPU:
>> +
>> + +------+ +-----------------------------------+
>> + | DDR4 | | +-------------------+ |
>> + +------+ | | PCIe Root complex | LS7A1000 |
>> + || MC0 | +--++---------++----+ |
>> + +----------+ HT 3.0 | || || |
>> + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
>> + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
>> + +----------+ | +--------+ +-+--+-+ +---------+ +------+
>> + || MC1 +---------------|--|----------------+
>> + +------+ | |
>> + | DDR4 | +-------+ DVO0 | | DVO1 +------+
>> + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
>> + +-------+ +------+
>> +
>> + The display controller in LS2K1000/LS2K0500:
>> + ___________________ _________
>> + | -------| | |
>> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
>> + | _ _ -------| ^ ^ |_________|
>> + | | | | | | | |
>> + | |_| |_| | +------+ |
>> + | <---->| i2c0 |<---------+
>> + | DC IN LS2K1000 | +------+
>> + | _ _ | +------+
>> + | | | | | <---->| i2c1 |----------+
>> + | |_| |_| | +------+ | _________
>> + | -------| | | | |
>> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
>> + | -------| |_________|
>> + |___________________|
>> +
>> +properties:
>> + $nodename:
>> + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
>> +
>> + compatible:
>> + oneOf:
>> + - items:
>> + - enum:
>> + - loongson,ls7a1000-dc
>> + - loongson,ls2k1000-dc
>> + - loongson,ls2k0500-dc
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + '#address-cells':
>> + const: 1
>> +
>> + '#size-cells':
>> + const: 0
>> +
>> + i2c-gpio@0:
>> + description: |
>> + Built-in GPIO emulate i2c exported for external display bridge
> If you have i2c-gpio, that belongs at the DT top-level, not here.
>
>> + configuration, onitor detection and edid read back etc, for ls7a1000
>> + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
> No, there's a defined i2c-gpio compatible already.

This is different from the i2c-gpio already defined under drivers/i2c/busses/i2c-gpio.c,
By design, my i2c-gpio is vendor specific properties, lsdc device driver create the i2c
adapter at runtime. These are 4 dedicated GPIOs whose control register is located at the
LSDC register space, not general purpose GPIOs with separate control register resource.
So i think it is the child node of display-controller@6,1, it belongs to LSDC.
It seems that put it at the DT top-level break the hierarchy and relationship.

>> + used to specify a I2c adapter bus number, if you don't specify one
>> + i2c driver core will dynamically assign a bus number. Please specify
> Bus numbers are a linux detail not relevant to DT binding.
>
>> + it only when its bus number matters. Bus number greater than 6 is safe
>> + because ls7a1000 bridge have 6 hardware I2C controller integrated.
>> +
>> + i2c-gpio@1:
>> + description: |
>> + Built-in GPIO emulate i2c exported for external display bridge
>> + configuration, onitor detection and edid read back etc, for ls7a1000
>> + only. Its compatible must be lsdc,i2c-gpio-1.
>> +
>> + ports:
>> + $ref: /schemas/graph.yaml#/properties/ports
>> +
>> + properties:
>> + port@0:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
>> +
>> + port@1:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
>> +
>> + required:
>> + - port@0
>> + - port@1
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> + - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> + bus {
>> +
>> + #address-cells = <3>;
>> + #size-cells = <2>;
>> + #interrupt-cells = <2>;
>> +
>> + display-controller@6,1 {
>> + compatible = "loongson,ls7a1000-dc";
>> + reg = <0x3100 0x0 0x0 0x0 0x0>;
>> + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + i2c-gpio@0 {
>> + compatible = "lsdc,i2c-gpio-0";
>> + reg = <6>;
>> + sda = <0>;
>> + scl = <1>;
>> + };
>> +
>> + i2c-gpio@1 {
>> + compatible = "lsdc,i2c-gpio-1";
>> + reg = <7>;
>> + sda = <2>;
>> + scl = <3>;
>> + };
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + port@0 {
>> + reg = <0>;
>> + endpoint {
>> + remote-endpoint = <&vga_encoder_in>;
>> + };
>> + };
>> + port@1 {
>> + reg = <1>;
>> + endpoint {
>> + remote-endpoint = <&dvi_encoder_in>;
>> + };
>> + };
>> + };
>> + };
>> + };
>> +
>> + - |
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> + bus {
>> +
>> + #address-cells = <3>;
>> + #size-cells = <2>;
>> + #interrupt-cells = <2>;
>> +
>> + display-controller@6,0 {
>> + compatible = "loongson,ls2k1000-dc";
>> + reg = <0x3100 0x0 0x0 0x0 0x0>;
>> + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + port@0 {
>> + reg = <0>;
>> + endpoint {
>> + remote-endpoint = <&panel_in>;
>> + };
>> + };
>> + port@1 {
>> + reg = <1>;
>> + endpoint {
>> + remote-endpoint = <&hdmi_encoder_in>;
>> + };
>> + };
>> + };
>> + };
>> + };
>> +...
>> --
>> 2.25.1
>>
>>

2022-03-22 14:14:35

by Sui Jingfeng

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller


On 2022/3/22 21:08, Jiaxun Yang wrote:
>
>
> 在 2022/3/22 2:33, Sui Jingfeng 写道:
>>
>> On 2022/3/22 07:20, Rob Herring wrote:
>>> On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
>>>> From: suijingfeng <[email protected]>
>>>>
>>> Needs a commit message.
>>>
>>>> Signed-off-by: suijingfeng <[email protected]>
>>>> Signed-off-by: Sui Jingfeng <[email protected]>
>>> Same person? Don't need both emails.
>>
>> Yes,  [email protected] is my company's email. But it can not
>> be used to send patches to dri-devel,
>>
>> when send patches with this email, the patch will not be shown on
>> patch works.
>>
>> Emails  are either blocked or got  rejected  by loongson's mail
>> server.  It can only receive emails
>>
>> from you and other people, but not dri-devel. so have to use my
>> personal email([email protected]) to send patches.
> In this case you can just use your company's email to sign-off
> code and sending with your personal email. It's common practice.
>
> If you don't want to receiving kernel email in your company mailbox,
> you can add a entry in .mailmap .
>
|I'm using `git send-email -7 --cover-letter --annotate -v11` command to
send patches, it will automatically sign off patches with the my private
emails. |

> Thanks.
> - Jiaxun

2022-03-22 20:24:53

by Jiaxun Yang

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller



在 2022/3/22 2:33, Sui Jingfeng 写道:
>
> On 2022/3/22 07:20, Rob Herring wrote:
>> On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
>>> From: suijingfeng <[email protected]>
>>>
>> Needs a commit message.
>>
>>> Signed-off-by: suijingfeng <[email protected]>
>>> Signed-off-by: Sui Jingfeng <[email protected]>
>> Same person? Don't need both emails.
>
> Yes,  [email protected] is my company's email. But it can not be
> used to send patches to dri-devel,
>
> when send patches with this email, the patch will not be shown on
> patch works.
>
> Emails  are either blocked or got  rejected  by loongson's mail
> server.  It can only receive emails
>
> from you and other people, but not dri-devel. so have to use my
> personal email([email protected]) to send patches.
In this case you can just use your company's email to sign-off
code and sending with your personal email. It's common practice.

If you don't want to receiving kernel email in your company mailbox,
you can add a entry in .mailmap .

Thanks.
- Jiaxun

2022-03-22 23:54:52

by Jiaxun Yang

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller



在 2022/3/22 13:54, Sui Jingfeng 写道:
>
> On 2022/3/22 21:08, Jiaxun Yang wrote:
>>
>>
>> 在 2022/3/22 2:33, Sui Jingfeng 写道:
>>>
>>> On 2022/3/22 07:20, Rob Herring wrote:
>>>> On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
>>>>> From: suijingfeng <[email protected]>
>>>>>
>>>> Needs a commit message.
>>>>
>>>>> Signed-off-by: suijingfeng <[email protected]>
>>>>> Signed-off-by: Sui Jingfeng <[email protected]>
>>>> Same person? Don't need both emails.
>>>
>>> Yes,  [email protected] is my company's email. But it can not
>>> be used to send patches to dri-devel,
>>>
>>> when send patches with this email, the patch will not be shown on
>>> patch works.
>>>
>>> Emails  are either blocked or got  rejected  by loongson's mail
>>> server.  It can only receive emails
>>>
>>> from you and other people, but not dri-devel. so have to use my
>>> personal email([email protected]) to send patches.
>> In this case you can just use your company's email to sign-off
>> code and sending with your personal email. It's common practice.
>>
>> If you don't want to receiving kernel email in your company mailbox,
>> you can add a entry in .mailmap .
>>
> |I'm using `git send-email -7 --cover-letter --annotate -v11` command
> to send patches, it will automatically sign off patches with the my
> private emails. |
The alternative solution is:

git format-patch -7 -v11 --cover-letter
git send-email ./*.patch

Thanks.
- Jiaxun

>
>> Thanks.
>> - Jiaxun

2022-03-23 01:58:24

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller

On Tue, Mar 22, 2022 at 09:54:08PM +0800, Sui Jingfeng wrote:
>
> On 2022/3/22 21:08, Jiaxun Yang wrote:
> >
> >
> > 在 2022/3/22 2:33, Sui Jingfeng 写道:
> > >
> > > On 2022/3/22 07:20, Rob Herring wrote:
> > > > On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
> > > > > From: suijingfeng <[email protected]>
> > > > >
> > > > Needs a commit message.
> > > >
> > > > > Signed-off-by: suijingfeng <[email protected]>
> > > > > Signed-off-by: Sui Jingfeng <[email protected]>
> > > > Same person? Don't need both emails.
> > >
> > > Yes,  [email protected] is my company's email. But it can not
> > > be used to send patches to dri-devel,
> > >
> > > when send patches with this email, the patch will not be shown on
> > > patch works.
> > >
> > > Emails  are either blocked or got  rejected  by loongson's mail
> > > server.  It can only receive emails
> > >
> > > from you and other people, but not dri-devel. so have to use my
> > > personal email([email protected]) to send patches.
> > In this case you can just use your company's email to sign-off
> > code and sending with your personal email. It's common practice.
> >
> > If you don't want to receiving kernel email in your company mailbox,
> > you can add a entry in .mailmap .
> >
> |I'm using `git send-email -7 --cover-letter --annotate -v11` command to
> send patches, it will automatically sign off patches with the my private
> emails. |

I think that is only if you set your git config author to your private
email. Pretty much anything git might automatically do can be turned
off.

Rob

2022-03-23 13:41:23

by Sui Jingfeng

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller


On 2022/3/23 04:55, Rob Herring wrote:
> On Tue, Mar 22, 2022 at 10:33:45AM +0800, Sui Jingfeng wrote:
>> On 2022/3/22 07:20, Rob Herring wrote:
>>> On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
>>>> From: suijingfeng <[email protected]>
>>>>
>>> Needs a commit message.
>>>
>>>> Signed-off-by: suijingfeng <[email protected]>
>>>> Signed-off-by: Sui Jingfeng <[email protected]>
>>> Same person? Don't need both emails.
>> Yes,  [email protected] is my company's email. But it can not be used
>> to send patches to dri-devel,
>>
>> when send patches with this email, the patch will not be shown on patch
>> works.
>>
>> Emails  are either blocked or got  rejected  by loongson's mail server.  It
>> can only receive emails
>>
>> from you and other people, but not dri-devel. so have to use my personal
>> email([email protected]) to send patches.
>>
>>>> ---
>>>> .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
>>>> 1 file changed, 230 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>> new file mode 100644
>>>> index 000000000000..7be63346289e
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>> @@ -0,0 +1,230 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
>>>> +
>>>> +maintainers:
>>>> + - Sui Jingfeng <[email protected]>
>>>> +
>>>> +description: |+
>>>> +
>>>> + Loongson display controllers are simple which require scanout buffers
>>>> + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
>>>> + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
>>>> + with a dedicated video RAM which is 64MB or more, precise size can be
>>>> + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
>>>> + chip.
>>>> +
>>>> + LSDC has two display pipes, each way has a DVO interface which provide
>>>> + RGB888 signals, vertical & horizontal synchronisations, data enable and
>>>> + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
>>>> + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
>>>> +
>>>> + For LS7A1000, there are 4 dedicated GPIOs whose control register is
>>>> + located at the DC register space. They are used to emulate two way i2c,
>>>> + One for DVO0, another for DVO1.
>>>> +
>>>> + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
>>>> + general purpose GPIO emulated i2c or hardware i2c in the SoC.
>>>> +
>>>> + LSDC's display pipeline have several components as below description,
>>>> +
>>>> + The display controller in LS7A1000:
>>>> + ___________________ _________
>>>> + | -------| | |
>>>> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
>>>> + | _ _ -------| ^ ^ |_________|
>>>> + | | | | | -------| | |
>>>> + | |_| |_| | i2c0 <--------+-------------+
>>>> + | -------|
>>>> + | DC IN LS7A1000 |
>>>> + | _ _ -------|
>>>> + | | | | | | i2c1 <--------+-------------+
>>>> + | |_| |_| -------| | | _________
>>>> + | -------| | | | |
>>>> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
>>>> + | -------| |_________|
>>>> + |___________________|
>>>> +
>>>> + Simple usage of LS7A1000 with LS3A4000 CPU:
>>>> +
>>>> + +------+ +-----------------------------------+
>>>> + | DDR4 | | +-------------------+ |
>>>> + +------+ | | PCIe Root complex | LS7A1000 |
>>>> + || MC0 | +--++---------++----+ |
>>>> + +----------+ HT 3.0 | || || |
>>>> + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
>>>> + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
>>>> + +----------+ | +--------+ +-+--+-+ +---------+ +------+
>>>> + || MC1 +---------------|--|----------------+
>>>> + +------+ | |
>>>> + | DDR4 | +-------+ DVO0 | | DVO1 +------+
>>>> + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
>>>> + +-------+ +------+
>>>> +
>>>> + The display controller in LS2K1000/LS2K0500:
>>>> + ___________________ _________
>>>> + | -------| | |
>>>> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
>>>> + | _ _ -------| ^ ^ |_________|
>>>> + | | | | | | | |
>>>> + | |_| |_| | +------+ |
>>>> + | <---->| i2c0 |<---------+
>>>> + | DC IN LS2K1000 | +------+
>>>> + | _ _ | +------+
>>>> + | | | | | <---->| i2c1 |----------+
>>>> + | |_| |_| | +------+ | _________
>>>> + | -------| | | | |
>>>> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
>>>> + | -------| |_________|
>>>> + |___________________|
>>>> +
>>>> +properties:
>>>> + $nodename:
>>>> + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
>>>> +
>>>> + compatible:
>>>> + oneOf:
>>>> + - items:
>>>> + - enum:
>>>> + - loongson,ls7a1000-dc
>>>> + - loongson,ls2k1000-dc
>>>> + - loongson,ls2k0500-dc
>>>> +
>>>> + reg:
>>>> + maxItems: 1
>>>> +
>>>> + interrupts:
>>>> + maxItems: 1
>>>> +
>>>> + '#address-cells':
>>>> + const: 1
>>>> +
>>>> + '#size-cells':
>>>> + const: 0
>>>> +
>>>> + i2c-gpio@0:
>>>> + description: |
>>>> + Built-in GPIO emulate i2c exported for external display bridge
>>> If you have i2c-gpio, that belongs at the DT top-level, not here.
>>>
>>>> + configuration, onitor detection and edid read back etc, for ls7a1000
>>>> + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
>>> No, there's a defined i2c-gpio compatible already.
>> This is different from the i2c-gpio already defined under drivers/i2c/busses/i2c-gpio.c,
>> By design, my i2c-gpio is vendor specific properties, lsdc device driver create the i2c
>> adapter at runtime. These are 4 dedicated GPIOs whose control register is located at the
>> LSDC register space, not general purpose GPIOs with separate control register resource.
>> So i think it is the child node of display-controller@6,1, it belongs to LSDC.
>> It seems that put it at the DT top-level break the hierarchy and relationship.
> Okay, I see. Then just 'i2c' for the node names. You need a reference to
> i2c-controller.yaml for these nodes too.
>
> The compatible should not have an index in it.
OK, i will fix this at the next version. thanks.
>>>> + used to specify a I2c adapter bus number, if you don't specify one
>>>> + i2c driver core will dynamically assign a bus number. Please specify
>>> Bus numbers are a linux detail not relevant to DT binding.
>>>
>>>> + it only when its bus number matters. Bus number greater than 6 is safe
>>>> + because ls7a1000 bridge have 6 hardware I2C controller integrated.
>>>> +
>>>> + i2c-gpio@1:
>>>> + description: |
>>>> + Built-in GPIO emulate i2c exported for external display bridge
>>>> + configuration, onitor detection and edid read back etc, for ls7a1000
>>>> + only. Its compatible must be lsdc,i2c-gpio-1.
>>>> +
>>>> + ports:
>>>> + $ref: /schemas/graph.yaml#/properties/ports
>>>> +
>>>> + properties:
>>>> + port@0:
>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
>>>> +
>>>> + port@1:
>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
>>>> +
>>>> + required:
>>>> + - port@0
>>>> + - port@1
>>>> +
>>>> +required:
>>>> + - compatible
>>>> + - reg
>>>> + - interrupts
>>>> + - ports
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> + - |
>>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>>> + bus {
>>>> +
>>>> + #address-cells = <3>;
>>>> + #size-cells = <2>;
>>>> + #interrupt-cells = <2>;
>>>> +
>>>> + display-controller@6,1 {
>>>> + compatible = "loongson,ls7a1000-dc";
>>>> + reg = <0x3100 0x0 0x0 0x0 0x0>;
>>>> + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
>>>> +
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> +
>>>> + i2c-gpio@0 {
>>>> + compatible = "lsdc,i2c-gpio-0";
>>>> + reg = <6>;
> 'reg' needs to be documented with some description of what 6 and 7
> represent. If they are the control register offset, then make the
> address translatable (use 'ranges' and define the size).

By design, the reg property is used to specify a I2c adapter bus number,
if we don't specify one, i2c driver core will dynamically assign a bus number.
then the nr of the i2c adapter will started from 0. I want is start from 6
to avoid potential conflict feature hardware I2C driver.

Because LS7A1000 bridge chip have 6 hardware I2C controller integrated,
but its driver is not up-streamed yet. By default these hardware I2C controller's
nr is started from 0.

Even through i2c driver core can dynamically generate a number, i still want it
to be fixed and keep consistent and explicit. That is, i2c6 is for display pipe 0,
i2c7 is for display pipe 1. This follow the convention and flexible enough.

>>>> + sda = <0>;
>>>> + scl = <1>;
> These need a vendor prefix.
OK, i will fix this at the next version, thanks
>>>> + };
>>>> +
>>>> + i2c-gpio@1 {
>>>> + compatible = "lsdc,i2c-gpio-1";
>>>> + reg = <7>;
>>>> + sda = <2>;
>>>> + scl = <3>;
>>>> + };
>>>> +
>>>> + ports {
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + port@0 {
>>>> + reg = <0>;
>>>> + endpoint {
>>>> + remote-endpoint = <&vga_encoder_in>;
>>>> + };
>>>> + };
>>>> + port@1 {
>>>> + reg = <1>;
>>>> + endpoint {
>>>> + remote-endpoint = <&dvi_encoder_in>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + - |
>>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>>> + bus {
>>>> +
>>>> + #address-cells = <3>;
>>>> + #size-cells = <2>;
>>>> + #interrupt-cells = <2>;
>>>> +
>>>> + display-controller@6,0 {
>>>> + compatible = "loongson,ls2k1000-dc";
>>>> + reg = <0x3100 0x0 0x0 0x0 0x0>;
>>>> + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
>>>> +
>>>> + ports {
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + port@0 {
>>>> + reg = <0>;
>>>> + endpoint {
>>>> + remote-endpoint = <&panel_in>;
>>>> + };
>>>> + };
>>>> + port@1 {
>>>> + reg = <1>;
>>>> + endpoint {
>>>> + remote-endpoint = <&hdmi_encoder_in>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +...
>>>> --
>>>> 2.25.1
>>>>
>>>>

2022-03-24 02:06:03

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller

On Wed, Mar 23, 2022 at 11:38:55AM +0800, Sui Jingfeng wrote:
>
> On 2022/3/23 04:55, Rob Herring wrote:
> > On Tue, Mar 22, 2022 at 10:33:45AM +0800, Sui Jingfeng wrote:
> > > On 2022/3/22 07:20, Rob Herring wrote:
> > > > On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
> > > > > From: suijingfeng <[email protected]>
> > > > >
> > > > Needs a commit message.
> > > >
> > > > > Signed-off-by: suijingfeng <[email protected]>
> > > > > Signed-off-by: Sui Jingfeng <[email protected]>
> > > > Same person? Don't need both emails.
> > > Yes,? [email protected] is my company's email. But it can not be used
> > > to send patches to dri-devel,
> > >
> > > when send patches with this email, the patch will not be shown on patch
> > > works.
> > >
> > > Emails? are either blocked or got? rejected? by loongson's mail server.? It
> > > can only receive emails
> > >
> > > from you and other people, but not dri-devel. so have to use my personal
> > > email([email protected]) to send patches.
> > >
> > > > > ---
> > > > > .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
> > > > > 1 file changed, 230 insertions(+)
> > > > > create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..7be63346289e
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > @@ -0,0 +1,230 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
> > > > > +
> > > > > +maintainers:
> > > > > + - Sui Jingfeng <[email protected]>
> > > > > +
> > > > > +description: |+
> > > > > +
> > > > > + Loongson display controllers are simple which require scanout buffers
> > > > > + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
> > > > > + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
> > > > > + with a dedicated video RAM which is 64MB or more, precise size can be
> > > > > + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
> > > > > + chip.
> > > > > +
> > > > > + LSDC has two display pipes, each way has a DVO interface which provide
> > > > > + RGB888 signals, vertical & horizontal synchronisations, data enable and
> > > > > + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
> > > > > + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
> > > > > +
> > > > > + For LS7A1000, there are 4 dedicated GPIOs whose control register is
> > > > > + located at the DC register space. They are used to emulate two way i2c,
> > > > > + One for DVO0, another for DVO1.
> > > > > +
> > > > > + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
> > > > > + general purpose GPIO emulated i2c or hardware i2c in the SoC.
> > > > > +
> > > > > + LSDC's display pipeline have several components as below description,
> > > > > +
> > > > > + The display controller in LS7A1000:
> > > > > + ___________________ _________
> > > > > + | -------| | |
> > > > > + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> > > > > + | _ _ -------| ^ ^ |_________|
> > > > > + | | | | | -------| | |
> > > > > + | |_| |_| | i2c0 <--------+-------------+
> > > > > + | -------|
> > > > > + | DC IN LS7A1000 |
> > > > > + | _ _ -------|
> > > > > + | | | | | | i2c1 <--------+-------------+
> > > > > + | |_| |_| -------| | | _________
> > > > > + | -------| | | | |
> > > > > + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> > > > > + | -------| |_________|
> > > > > + |___________________|
> > > > > +
> > > > > + Simple usage of LS7A1000 with LS3A4000 CPU:
> > > > > +
> > > > > + +------+ +-----------------------------------+
> > > > > + | DDR4 | | +-------------------+ |
> > > > > + +------+ | | PCIe Root complex | LS7A1000 |
> > > > > + || MC0 | +--++---------++----+ |
> > > > > + +----------+ HT 3.0 | || || |
> > > > > + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
> > > > > + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
> > > > > + +----------+ | +--------+ +-+--+-+ +---------+ +------+
> > > > > + || MC1 +---------------|--|----------------+
> > > > > + +------+ | |
> > > > > + | DDR4 | +-------+ DVO0 | | DVO1 +------+
> > > > > + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
> > > > > + +-------+ +------+
> > > > > +
> > > > > + The display controller in LS2K1000/LS2K0500:
> > > > > + ___________________ _________
> > > > > + | -------| | |
> > > > > + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> > > > > + | _ _ -------| ^ ^ |_________|
> > > > > + | | | | | | | |
> > > > > + | |_| |_| | +------+ |
> > > > > + | <---->| i2c0 |<---------+
> > > > > + | DC IN LS2K1000 | +------+
> > > > > + | _ _ | +------+
> > > > > + | | | | | <---->| i2c1 |----------+
> > > > > + | |_| |_| | +------+ | _________
> > > > > + | -------| | | | |
> > > > > + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> > > > > + | -------| |_________|
> > > > > + |___________________|
> > > > > +
> > > > > +properties:
> > > > > + $nodename:
> > > > > + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
> > > > > +
> > > > > + compatible:
> > > > > + oneOf:
> > > > > + - items:
> > > > > + - enum:
> > > > > + - loongson,ls7a1000-dc
> > > > > + - loongson,ls2k1000-dc
> > > > > + - loongson,ls2k0500-dc
> > > > > +
> > > > > + reg:
> > > > > + maxItems: 1
> > > > > +
> > > > > + interrupts:
> > > > > + maxItems: 1
> > > > > +
> > > > > + '#address-cells':
> > > > > + const: 1
> > > > > +
> > > > > + '#size-cells':
> > > > > + const: 0
> > > > > +
> > > > > + i2c-gpio@0:
> > > > > + description: |
> > > > > + Built-in GPIO emulate i2c exported for external display bridge
> > > > If you have i2c-gpio, that belongs at the DT top-level, not here.
> > > >
> > > > > + configuration, onitor detection and edid read back etc, for ls7a1000
> > > > > + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
> > > > No, there's a defined i2c-gpio compatible already.
> > > This is different from the i2c-gpio already defined under drivers/i2c/busses/i2c-gpio.c,
> > > By design, my i2c-gpio is vendor specific properties, lsdc device driver create the i2c
> > > adapter at runtime. These are 4 dedicated GPIOs whose control register is located at the
> > > LSDC register space, not general purpose GPIOs with separate control register resource.
> > > So i think it is the child node of?display-controller@6,1,?it belongs to LSDC.
> > > It seems that put it at the DT top-level break the hierarchy and relationship.
> > Okay, I see. Then just 'i2c' for the node names. You need a reference to
> > i2c-controller.yaml for these nodes too.
> >
> > The compatible should not have an index in it.
> OK, i will fix this at the next version. thanks.
> > > > > + used to specify a I2c adapter bus number, if you don't specify one
> > > > > + i2c driver core will dynamically assign a bus number. Please specify
> > > > Bus numbers are a linux detail not relevant to DT binding.
> > > >
> > > > > + it only when its bus number matters. Bus number greater than 6 is safe
> > > > > + because ls7a1000 bridge have 6 hardware I2C controller integrated.
> > > > > +
> > > > > + i2c-gpio@1:
> > > > > + description: |
> > > > > + Built-in GPIO emulate i2c exported for external display bridge
> > > > > + configuration, onitor detection and edid read back etc, for ls7a1000
> > > > > + only. Its compatible must be lsdc,i2c-gpio-1.
> > > > > +
> > > > > + ports:
> > > > > + $ref: /schemas/graph.yaml#/properties/ports
> > > > > +
> > > > > + properties:
> > > > > + port@0:
> > > > > + $ref: /schemas/graph.yaml#/properties/port
> > > > > + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> > > > > +
> > > > > + port@1:
> > > > > + $ref: /schemas/graph.yaml#/properties/port
> > > > > + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> > > > > +
> > > > > + required:
> > > > > + - port@0
> > > > > + - port@1
> > > > > +
> > > > > +required:
> > > > > + - compatible
> > > > > + - reg
> > > > > + - interrupts
> > > > > + - ports
> > > > > +
> > > > > +additionalProperties: false
> > > > > +
> > > > > +examples:
> > > > > + - |
> > > > > + #include <dt-bindings/interrupt-controller/irq.h>
> > > > > + bus {
> > > > > +
> > > > > + #address-cells = <3>;
> > > > > + #size-cells = <2>;
> > > > > + #interrupt-cells = <2>;
> > > > > +
> > > > > + display-controller@6,1 {
> > > > > + compatible = "loongson,ls7a1000-dc";
> > > > > + reg = <0x3100 0x0 0x0 0x0 0x0>;
> > > > > + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
> > > > > +
> > > > > + #address-cells = <1>;
> > > > > + #size-cells = <0>;
> > > > > +
> > > > > + i2c-gpio@0 {
> > > > > + compatible = "lsdc,i2c-gpio-0";
> > > > > + reg = <6>;
> > 'reg' needs to be documented with some description of what 6 and 7
> > represent. If they are the control register offset, then make the
> > address translatable (use 'ranges' and define the size).
>
> By design, the reg property is used to specify a I2c adapter bus number,
> if we don't specify one, i2c driver core will dynamically assign a bus number.
> then the nr of the i2c adapter will started from 0. I want is start from 6
> to avoid potential conflict feature hardware I2C driver.
>
> Because LS7A1000 bridge chip have 6 hardware I2C controller integrated,
> but its driver is not up-streamed yet. By default these hardware I2C controller's
> nr is started from 0.

Linux's numbering doesn't belong in DT. So no, you can't use 'reg' in
that way.

> Even through i2c driver core can dynamically generate a number, i still want it
> to be fixed and keep consistent and explicit. That is, i2c6 is for display pipe 0,
> i2c7 is for display pipe 1. This follow the convention and flexible enough.

You may want that, but that is not how the kernel works. Specific
numbers are not guaranteed. I'm sure you've seen this for disks, network
interfaces, etc.

Rob

2022-03-25 19:34:42

by Sui Jingfeng

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller


On 2022/3/23 21:03, Rob Herring wrote:
> On Wed, Mar 23, 2022 at 11:38:55AM +0800, Sui Jingfeng wrote:
>> On 2022/3/23 04:55, Rob Herring wrote:
>>> On Tue, Mar 22, 2022 at 10:33:45AM +0800, Sui Jingfeng wrote:
>>>> On 2022/3/22 07:20, Rob Herring wrote:
>>>>> On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
>>>>>> From: suijingfeng <[email protected]>
>>>>>>
>>>>> Needs a commit message.
>>>>>
>>>>>> Signed-off-by: suijingfeng <[email protected]>
>>>>>> Signed-off-by: Sui Jingfeng <[email protected]>
>>>>> Same person? Don't need both emails.
>>>> Yes,  [email protected] is my company's email. But it can not be used
>>>> to send patches to dri-devel,
>>>>
>>>> when send patches with this email, the patch will not be shown on patch
>>>> works.
>>>>
>>>> Emails  are either blocked or got  rejected  by loongson's mail server.  It
>>>> can only receive emails
>>>>
>>>> from you and other people, but not dri-devel. so have to use my personal
>>>> email([email protected]) to send patches.
>>>>
>>>>>> ---
>>>>>> .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
>>>>>> 1 file changed, 230 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..7be63346289e
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>>>> @@ -0,0 +1,230 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
>>>>>> +
>>>>>> +maintainers:
>>>>>> + - Sui Jingfeng <[email protected]>
>>>>>> +
>>>>>> +description: |+
>>>>>> +
>>>>>> + Loongson display controllers are simple which require scanout buffers
>>>>>> + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
>>>>>> + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
>>>>>> + with a dedicated video RAM which is 64MB or more, precise size can be
>>>>>> + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
>>>>>> + chip.
>>>>>> +
>>>>>> + LSDC has two display pipes, each way has a DVO interface which provide
>>>>>> + RGB888 signals, vertical & horizontal synchronisations, data enable and
>>>>>> + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
>>>>>> + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
>>>>>> +
>>>>>> + For LS7A1000, there are 4 dedicated GPIOs whose control register is
>>>>>> + located at the DC register space. They are used to emulate two way i2c,
>>>>>> + One for DVO0, another for DVO1.
>>>>>> +
>>>>>> + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
>>>>>> + general purpose GPIO emulated i2c or hardware i2c in the SoC.
>>>>>> +
>>>>>> + LSDC's display pipeline have several components as below description,
>>>>>> +
>>>>>> + The display controller in LS7A1000:
>>>>>> + ___________________ _________
>>>>>> + | -------| | |
>>>>>> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
>>>>>> + | _ _ -------| ^ ^ |_________|
>>>>>> + | | | | | -------| | |
>>>>>> + | |_| |_| | i2c0 <--------+-------------+
>>>>>> + | -------|
>>>>>> + | DC IN LS7A1000 |
>>>>>> + | _ _ -------|
>>>>>> + | | | | | | i2c1 <--------+-------------+
>>>>>> + | |_| |_| -------| | | _________
>>>>>> + | -------| | | | |
>>>>>> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
>>>>>> + | -------| |_________|
>>>>>> + |___________________|
>>>>>> +
>>>>>> + Simple usage of LS7A1000 with LS3A4000 CPU:
>>>>>> +
>>>>>> + +------+ +-----------------------------------+
>>>>>> + | DDR4 | | +-------------------+ |
>>>>>> + +------+ | | PCIe Root complex | LS7A1000 |
>>>>>> + || MC0 | +--++---------++----+ |
>>>>>> + +----------+ HT 3.0 | || || |
>>>>>> + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
>>>>>> + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
>>>>>> + +----------+ | +--------+ +-+--+-+ +---------+ +------+
>>>>>> + || MC1 +---------------|--|----------------+
>>>>>> + +------+ | |
>>>>>> + | DDR4 | +-------+ DVO0 | | DVO1 +------+
>>>>>> + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
>>>>>> + +-------+ +------+
>>>>>> +
>>>>>> + The display controller in LS2K1000/LS2K0500:
>>>>>> + ___________________ _________
>>>>>> + | -------| | |
>>>>>> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
>>>>>> + | _ _ -------| ^ ^ |_________|
>>>>>> + | | | | | | | |
>>>>>> + | |_| |_| | +------+ |
>>>>>> + | <---->| i2c0 |<---------+
>>>>>> + | DC IN LS2K1000 | +------+
>>>>>> + | _ _ | +------+
>>>>>> + | | | | | <---->| i2c1 |----------+
>>>>>> + | |_| |_| | +------+ | _________
>>>>>> + | -------| | | | |
>>>>>> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
>>>>>> + | -------| |_________|
>>>>>> + |___________________|
>>>>>> +
>>>>>> +properties:
>>>>>> + $nodename:
>>>>>> + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
>>>>>> +
>>>>>> + compatible:
>>>>>> + oneOf:
>>>>>> + - items:
>>>>>> + - enum:
>>>>>> + - loongson,ls7a1000-dc
>>>>>> + - loongson,ls2k1000-dc
>>>>>> + - loongson,ls2k0500-dc
>>>>>> +
>>>>>> + reg:
>>>>>> + maxItems: 1
>>>>>> +
>>>>>> + interrupts:
>>>>>> + maxItems: 1
>>>>>> +
>>>>>> + '#address-cells':
>>>>>> + const: 1
>>>>>> +
>>>>>> + '#size-cells':
>>>>>> + const: 0
>>>>>> +
>>>>>> + i2c-gpio@0:
>>>>>> + description: |
>>>>>> + Built-in GPIO emulate i2c exported for external display bridge
>>>>> If you have i2c-gpio, that belongs at the DT top-level, not here.
>>>>>
>>>>>> + configuration, onitor detection and edid read back etc, for ls7a1000
>>>>>> + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
>>>>> No, there's a defined i2c-gpio compatible already.
>>>> This is different from the i2c-gpio already defined under drivers/i2c/busses/i2c-gpio.c,
>>>> By design, my i2c-gpio is vendor specific properties, lsdc device driver create the i2c
>>>> adapter at runtime. These are 4 dedicated GPIOs whose control register is located at the
>>>> LSDC register space, not general purpose GPIOs with separate control register resource.
>>>> So i think it is the child node of display-controller@6,1, it belongs to LSDC.
>>>> It seems that put it at the DT top-level break the hierarchy and relationship.
>>> Okay, I see. Then just 'i2c' for the node names. You need a reference to
>>> i2c-controller.yaml for these nodes too.
>>>
>>> The compatible should not have an index in it.
>> OK, i will fix this at the next version. thanks.
>>>>>> + used to specify a I2c adapter bus number, if you don't specify one
>>>>>> + i2c driver core will dynamically assign a bus number. Please specify
>>>>> Bus numbers are a linux detail not relevant to DT binding.
>>>>>
>>>>>> + it only when its bus number matters. Bus number greater than 6 is safe
>>>>>> + because ls7a1000 bridge have 6 hardware I2C controller integrated.
>>>>>> +
>>>>>> + i2c-gpio@1:
>>>>>> + description: |
>>>>>> + Built-in GPIO emulate i2c exported for external display bridge
>>>>>> + configuration, onitor detection and edid read back etc, for ls7a1000
>>>>>> + only. Its compatible must be lsdc,i2c-gpio-1.
>>>>>> +
>>>>>> + ports:
>>>>>> + $ref: /schemas/graph.yaml#/properties/ports
>>>>>> +
>>>>>> + properties:
>>>>>> + port@0:
>>>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>>>> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
>>>>>> +
>>>>>> + port@1:
>>>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>>>> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
>>>>>> +
>>>>>> + required:
>>>>>> + - port@0
>>>>>> + - port@1
>>>>>> +
>>>>>> +required:
>>>>>> + - compatible
>>>>>> + - reg
>>>>>> + - interrupts
>>>>>> + - ports
>>>>>> +
>>>>>> +additionalProperties: false
>>>>>> +
>>>>>> +examples:
>>>>>> + - |
>>>>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>>>>> + bus {
>>>>>> +
>>>>>> + #address-cells = <3>;
>>>>>> + #size-cells = <2>;
>>>>>> + #interrupt-cells = <2>;
>>>>>> +
>>>>>> + display-controller@6,1 {
>>>>>> + compatible = "loongson,ls7a1000-dc";
>>>>>> + reg = <0x3100 0x0 0x0 0x0 0x0>;
>>>>>> + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
>>>>>> +
>>>>>> + #address-cells = <1>;
>>>>>> + #size-cells = <0>;
>>>>>> +
>>>>>> + i2c-gpio@0 {
>>>>>> + compatible = "lsdc,i2c-gpio-0";
>>>>>> + reg = <6>;
>>> 'reg' needs to be documented with some description of what 6 and 7
>>> represent. If they are the control register offset, then make the
>>> address translatable (use 'ranges' and define the size).
>> By design, the reg property is used to specify a I2c adapter bus number,
>> if we don't specify one, i2c driver core will dynamically assign a bus number.
>> then the nr of the i2c adapter will started from 0. I want is start from 6
>> to avoid potential conflict feature hardware I2C driver.
>>
>> Because LS7A1000 bridge chip have 6 hardware I2C controller integrated,
>> but its driver is not up-streamed yet. By default these hardware I2C controller's
>> nr is started from 0.
> Linux's numbering doesn't belong in DT. So no, you can't use 'reg' in
> that way.
Then,  can i use something like lsdc,nr = <6> ?
>> Even through i2c driver core can dynamically generate a number, i still want it
>> to be fixed and keep consistent and explicit. That is, i2c6 is for display pipe 0,
>> i2c7 is for display pipe 1. This follow the convention and flexible enough.
> You may want that, but that is not how the kernel works. Specific
> numbers are not guaranteed. I'm sure you've seen this for disks, network
> interfaces, etc.
>
> Rob

2c_bit_add_numbered_bus() will guarantee it for you as long as If no
devices have pre-been declared for this bus.

you can read the comment of 2c_bit_add_numbered_bus() at
drivers/i2c/i2c-core-base.c

2022-03-25 20:03:39

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller

On Thu, Mar 24, 2022 at 09:48:19AM +0800, Sui Jingfeng wrote:
>
> On 2022/3/23 21:03, Rob Herring wrote:
> > On Wed, Mar 23, 2022 at 11:38:55AM +0800, Sui Jingfeng wrote:
> > > On 2022/3/23 04:55, Rob Herring wrote:
> > > > On Tue, Mar 22, 2022 at 10:33:45AM +0800, Sui Jingfeng wrote:
> > > > > On 2022/3/22 07:20, Rob Herring wrote:
> > > > > > On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
> > > > > > > From: suijingfeng <[email protected]>
> > > > > > >
> > > > > > Needs a commit message.
> > > > > >
> > > > > > > Signed-off-by: suijingfeng <[email protected]>
> > > > > > > Signed-off-by: Sui Jingfeng <[email protected]>
> > > > > > Same person? Don't need both emails.
> > > > > Yes,? [email protected] is my company's email. But it can not be used
> > > > > to send patches to dri-devel,
> > > > >
> > > > > when send patches with this email, the patch will not be shown on patch
> > > > > works.
> > > > >
> > > > > Emails? are either blocked or got? rejected? by loongson's mail server.? It
> > > > > can only receive emails
> > > > >
> > > > > from you and other people, but not dri-devel. so have to use my personal
> > > > > email([email protected]) to send patches.
> > > > >
> > > > > > > ---
> > > > > > > .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
> > > > > > > 1 file changed, 230 insertions(+)
> > > > > > > create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..7be63346289e
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > > > @@ -0,0 +1,230 @@
> > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > + - Sui Jingfeng <[email protected]>
> > > > > > > +
> > > > > > > +description: |+
> > > > > > > +
> > > > > > > + Loongson display controllers are simple which require scanout buffers
> > > > > > > + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
> > > > > > > + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
> > > > > > > + with a dedicated video RAM which is 64MB or more, precise size can be
> > > > > > > + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
> > > > > > > + chip.
> > > > > > > +
> > > > > > > + LSDC has two display pipes, each way has a DVO interface which provide
> > > > > > > + RGB888 signals, vertical & horizontal synchronisations, data enable and
> > > > > > > + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
> > > > > > > + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
> > > > > > > +
> > > > > > > + For LS7A1000, there are 4 dedicated GPIOs whose control register is
> > > > > > > + located at the DC register space. They are used to emulate two way i2c,
> > > > > > > + One for DVO0, another for DVO1.
> > > > > > > +
> > > > > > > + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
> > > > > > > + general purpose GPIO emulated i2c or hardware i2c in the SoC.
> > > > > > > +
> > > > > > > + LSDC's display pipeline have several components as below description,
> > > > > > > +
> > > > > > > + The display controller in LS7A1000:
> > > > > > > + ___________________ _________
> > > > > > > + | -------| | |
> > > > > > > + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> > > > > > > + | _ _ -------| ^ ^ |_________|
> > > > > > > + | | | | | -------| | |
> > > > > > > + | |_| |_| | i2c0 <--------+-------------+
> > > > > > > + | -------|
> > > > > > > + | DC IN LS7A1000 |
> > > > > > > + | _ _ -------|
> > > > > > > + | | | | | | i2c1 <--------+-------------+
> > > > > > > + | |_| |_| -------| | | _________
> > > > > > > + | -------| | | | |
> > > > > > > + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> > > > > > > + | -------| |_________|
> > > > > > > + |___________________|
> > > > > > > +
> > > > > > > + Simple usage of LS7A1000 with LS3A4000 CPU:
> > > > > > > +
> > > > > > > + +------+ +-----------------------------------+
> > > > > > > + | DDR4 | | +-------------------+ |
> > > > > > > + +------+ | | PCIe Root complex | LS7A1000 |
> > > > > > > + || MC0 | +--++---------++----+ |
> > > > > > > + +----------+ HT 3.0 | || || |
> > > > > > > + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
> > > > > > > + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
> > > > > > > + +----------+ | +--------+ +-+--+-+ +---------+ +------+
> > > > > > > + || MC1 +---------------|--|----------------+
> > > > > > > + +------+ | |
> > > > > > > + | DDR4 | +-------+ DVO0 | | DVO1 +------+
> > > > > > > + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
> > > > > > > + +-------+ +------+
> > > > > > > +
> > > > > > > + The display controller in LS2K1000/LS2K0500:
> > > > > > > + ___________________ _________
> > > > > > > + | -------| | |
> > > > > > > + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> > > > > > > + | _ _ -------| ^ ^ |_________|
> > > > > > > + | | | | | | | |
> > > > > > > + | |_| |_| | +------+ |
> > > > > > > + | <---->| i2c0 |<---------+
> > > > > > > + | DC IN LS2K1000 | +------+
> > > > > > > + | _ _ | +------+
> > > > > > > + | | | | | <---->| i2c1 |----------+
> > > > > > > + | |_| |_| | +------+ | _________
> > > > > > > + | -------| | | | |
> > > > > > > + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> > > > > > > + | -------| |_________|
> > > > > > > + |___________________|
> > > > > > > +
> > > > > > > +properties:
> > > > > > > + $nodename:
> > > > > > > + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
> > > > > > > +
> > > > > > > + compatible:
> > > > > > > + oneOf:
> > > > > > > + - items:
> > > > > > > + - enum:
> > > > > > > + - loongson,ls7a1000-dc
> > > > > > > + - loongson,ls2k1000-dc
> > > > > > > + - loongson,ls2k0500-dc
> > > > > > > +
> > > > > > > + reg:
> > > > > > > + maxItems: 1
> > > > > > > +
> > > > > > > + interrupts:
> > > > > > > + maxItems: 1
> > > > > > > +
> > > > > > > + '#address-cells':
> > > > > > > + const: 1
> > > > > > > +
> > > > > > > + '#size-cells':
> > > > > > > + const: 0
> > > > > > > +
> > > > > > > + i2c-gpio@0:
> > > > > > > + description: |
> > > > > > > + Built-in GPIO emulate i2c exported for external display bridge
> > > > > > If you have i2c-gpio, that belongs at the DT top-level, not here.
> > > > > >
> > > > > > > + configuration, onitor detection and edid read back etc, for ls7a1000
> > > > > > > + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
> > > > > > No, there's a defined i2c-gpio compatible already.
> > > > > This is different from the i2c-gpio already defined under drivers/i2c/busses/i2c-gpio.c,
> > > > > By design, my i2c-gpio is vendor specific properties, lsdc device driver create the i2c
> > > > > adapter at runtime. These are 4 dedicated GPIOs whose control register is located at the
> > > > > LSDC register space, not general purpose GPIOs with separate control register resource.
> > > > > So i think it is the child node of?display-controller@6,1,?it belongs to LSDC.
> > > > > It seems that put it at the DT top-level break the hierarchy and relationship.
> > > > Okay, I see. Then just 'i2c' for the node names. You need a reference to
> > > > i2c-controller.yaml for these nodes too.
> > > >
> > > > The compatible should not have an index in it.
> > > OK, i will fix this at the next version. thanks.
> > > > > > > + used to specify a I2c adapter bus number, if you don't specify one
> > > > > > > + i2c driver core will dynamically assign a bus number. Please specify
> > > > > > Bus numbers are a linux detail not relevant to DT binding.
> > > > > >
> > > > > > > + it only when its bus number matters. Bus number greater than 6 is safe
> > > > > > > + because ls7a1000 bridge have 6 hardware I2C controller integrated.
> > > > > > > +
> > > > > > > + i2c-gpio@1:
> > > > > > > + description: |
> > > > > > > + Built-in GPIO emulate i2c exported for external display bridge
> > > > > > > + configuration, onitor detection and edid read back etc, for ls7a1000
> > > > > > > + only. Its compatible must be lsdc,i2c-gpio-1.
> > > > > > > +
> > > > > > > + ports:
> > > > > > > + $ref: /schemas/graph.yaml#/properties/ports
> > > > > > > +
> > > > > > > + properties:
> > > > > > > + port@0:
> > > > > > > + $ref: /schemas/graph.yaml#/properties/port
> > > > > > > + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> > > > > > > +
> > > > > > > + port@1:
> > > > > > > + $ref: /schemas/graph.yaml#/properties/port
> > > > > > > + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> > > > > > > +
> > > > > > > + required:
> > > > > > > + - port@0
> > > > > > > + - port@1
> > > > > > > +
> > > > > > > +required:
> > > > > > > + - compatible
> > > > > > > + - reg
> > > > > > > + - interrupts
> > > > > > > + - ports
> > > > > > > +
> > > > > > > +additionalProperties: false
> > > > > > > +
> > > > > > > +examples:
> > > > > > > + - |
> > > > > > > + #include <dt-bindings/interrupt-controller/irq.h>
> > > > > > > + bus {
> > > > > > > +
> > > > > > > + #address-cells = <3>;
> > > > > > > + #size-cells = <2>;
> > > > > > > + #interrupt-cells = <2>;
> > > > > > > +
> > > > > > > + display-controller@6,1 {
> > > > > > > + compatible = "loongson,ls7a1000-dc";
> > > > > > > + reg = <0x3100 0x0 0x0 0x0 0x0>;
> > > > > > > + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > +
> > > > > > > + #address-cells = <1>;
> > > > > > > + #size-cells = <0>;
> > > > > > > +
> > > > > > > + i2c-gpio@0 {
> > > > > > > + compatible = "lsdc,i2c-gpio-0";
> > > > > > > + reg = <6>;
> > > > 'reg' needs to be documented with some description of what 6 and 7
> > > > represent. If they are the control register offset, then make the
> > > > address translatable (use 'ranges' and define the size).
> > > By design, the reg property is used to specify a I2c adapter bus number,
> > > if we don't specify one, i2c driver core will dynamically assign a bus number.
> > > then the nr of the i2c adapter will started from 0. I want is start from 6
> > > to avoid potential conflict feature hardware I2C driver.
> > >
> > > Because LS7A1000 bridge chip have 6 hardware I2C controller integrated,
> > > but its driver is not up-streamed yet. By default these hardware I2C controller's
> > > nr is started from 0.
> > Linux's numbering doesn't belong in DT. So no, you can't use 'reg' in
> > that way.
> Then,? can i use something like lsdc,nr = <6> ?
> > > Even through i2c driver core can dynamically generate a number, i still want it
> > > to be fixed and keep consistent and explicit. That is, i2c6 is for display pipe 0,
> > > i2c7 is for display pipe 1. This follow the convention and flexible enough.
> > You may want that, but that is not how the kernel works. Specific
> > numbers are not guaranteed. I'm sure you've seen this for disks, network
> > interfaces, etc.
> >
> > Rob
>
> 2c_bit_add_numbered_bus() will guarantee it for you as long as If no devices
> have pre-been declared for this bus.
>
> you can read the comment of 2c_bit_add_numbered_bus() at
> drivers/i2c/i2c-core-base.c

I didn't say it wasn't possible. It is not best practice. Grep
i2c_bit_add_numbered_bus and see how many users there are. Even if the
kernel allows specifying bus numbers, your Linux bus numbers don't
belong in DT.

Rob

2022-03-25 20:20:11

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller

On Tue, Mar 22, 2022 at 10:33:45AM +0800, Sui Jingfeng wrote:
>
> On 2022/3/22 07:20, Rob Herring wrote:
> > On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
> > > From: suijingfeng <[email protected]>
> > >
> > Needs a commit message.
> >
> > > Signed-off-by: suijingfeng <[email protected]>
> > > Signed-off-by: Sui Jingfeng <[email protected]>
> > Same person? Don't need both emails.
>
> Yes,? [email protected] is my company's email. But it can not be used
> to send patches to dri-devel,
>
> when send patches with this email, the patch will not be shown on patch
> works.
>
> Emails? are either blocked or got? rejected? by loongson's mail server.? It
> can only receive emails
>
> from you and other people, but not dri-devel. so have to use my personal
> email([email protected]) to send patches.
>
> > > ---
> > > .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
> > > 1 file changed, 230 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > new file mode 100644
> > > index 000000000000..7be63346289e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > @@ -0,0 +1,230 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
> > > +
> > > +maintainers:
> > > + - Sui Jingfeng <[email protected]>
> > > +
> > > +description: |+
> > > +
> > > + Loongson display controllers are simple which require scanout buffers
> > > + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
> > > + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
> > > + with a dedicated video RAM which is 64MB or more, precise size can be
> > > + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
> > > + chip.
> > > +
> > > + LSDC has two display pipes, each way has a DVO interface which provide
> > > + RGB888 signals, vertical & horizontal synchronisations, data enable and
> > > + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
> > > + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
> > > +
> > > + For LS7A1000, there are 4 dedicated GPIOs whose control register is
> > > + located at the DC register space. They are used to emulate two way i2c,
> > > + One for DVO0, another for DVO1.
> > > +
> > > + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
> > > + general purpose GPIO emulated i2c or hardware i2c in the SoC.
> > > +
> > > + LSDC's display pipeline have several components as below description,
> > > +
> > > + The display controller in LS7A1000:
> > > + ___________________ _________
> > > + | -------| | |
> > > + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> > > + | _ _ -------| ^ ^ |_________|
> > > + | | | | | -------| | |
> > > + | |_| |_| | i2c0 <--------+-------------+
> > > + | -------|
> > > + | DC IN LS7A1000 |
> > > + | _ _ -------|
> > > + | | | | | | i2c1 <--------+-------------+
> > > + | |_| |_| -------| | | _________
> > > + | -------| | | | |
> > > + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> > > + | -------| |_________|
> > > + |___________________|
> > > +
> > > + Simple usage of LS7A1000 with LS3A4000 CPU:
> > > +
> > > + +------+ +-----------------------------------+
> > > + | DDR4 | | +-------------------+ |
> > > + +------+ | | PCIe Root complex | LS7A1000 |
> > > + || MC0 | +--++---------++----+ |
> > > + +----------+ HT 3.0 | || || |
> > > + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
> > > + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
> > > + +----------+ | +--------+ +-+--+-+ +---------+ +------+
> > > + || MC1 +---------------|--|----------------+
> > > + +------+ | |
> > > + | DDR4 | +-------+ DVO0 | | DVO1 +------+
> > > + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
> > > + +-------+ +------+
> > > +
> > > + The display controller in LS2K1000/LS2K0500:
> > > + ___________________ _________
> > > + | -------| | |
> > > + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> > > + | _ _ -------| ^ ^ |_________|
> > > + | | | | | | | |
> > > + | |_| |_| | +------+ |
> > > + | <---->| i2c0 |<---------+
> > > + | DC IN LS2K1000 | +------+
> > > + | _ _ | +------+
> > > + | | | | | <---->| i2c1 |----------+
> > > + | |_| |_| | +------+ | _________
> > > + | -------| | | | |
> > > + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> > > + | -------| |_________|
> > > + |___________________|
> > > +
> > > +properties:
> > > + $nodename:
> > > + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
> > > +
> > > + compatible:
> > > + oneOf:
> > > + - items:
> > > + - enum:
> > > + - loongson,ls7a1000-dc
> > > + - loongson,ls2k1000-dc
> > > + - loongson,ls2k0500-dc
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + interrupts:
> > > + maxItems: 1
> > > +
> > > + '#address-cells':
> > > + const: 1
> > > +
> > > + '#size-cells':
> > > + const: 0
> > > +
> > > + i2c-gpio@0:
> > > + description: |
> > > + Built-in GPIO emulate i2c exported for external display bridge
> > If you have i2c-gpio, that belongs at the DT top-level, not here.
> >
> > > + configuration, onitor detection and edid read back etc, for ls7a1000
> > > + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
> > No, there's a defined i2c-gpio compatible already.
>
> This is different from the i2c-gpio already defined under drivers/i2c/busses/i2c-gpio.c,
> By design, my i2c-gpio is vendor specific properties, lsdc device driver create the i2c
> adapter at runtime. These are 4 dedicated GPIOs whose control register is located at the
> LSDC register space, not general purpose GPIOs with separate control register resource.
> So i think it is the child node of?display-controller@6,1,?it belongs to LSDC.
> It seems that put it at the DT top-level break the hierarchy and relationship.

Okay, I see. Then just 'i2c' for the node names. You need a reference to
i2c-controller.yaml for these nodes too.

The compatible should not have an index in it.


>
> > > + used to specify a I2c adapter bus number, if you don't specify one
> > > + i2c driver core will dynamically assign a bus number. Please specify
> > Bus numbers are a linux detail not relevant to DT binding.
> >
> > > + it only when its bus number matters. Bus number greater than 6 is safe
> > > + because ls7a1000 bridge have 6 hardware I2C controller integrated.
> > > +
> > > + i2c-gpio@1:
> > > + description: |
> > > + Built-in GPIO emulate i2c exported for external display bridge
> > > + configuration, onitor detection and edid read back etc, for ls7a1000
> > > + only. Its compatible must be lsdc,i2c-gpio-1.
> > > +
> > > + ports:
> > > + $ref: /schemas/graph.yaml#/properties/ports
> > > +
> > > + properties:
> > > + port@0:
> > > + $ref: /schemas/graph.yaml#/properties/port
> > > + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> > > +
> > > + port@1:
> > > + $ref: /schemas/graph.yaml#/properties/port
> > > + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> > > +
> > > + required:
> > > + - port@0
> > > + - port@1
> > > +
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - interrupts
> > > + - ports
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > + #include <dt-bindings/interrupt-controller/irq.h>
> > > + bus {
> > > +
> > > + #address-cells = <3>;
> > > + #size-cells = <2>;
> > > + #interrupt-cells = <2>;
> > > +
> > > + display-controller@6,1 {
> > > + compatible = "loongson,ls7a1000-dc";
> > > + reg = <0x3100 0x0 0x0 0x0 0x0>;
> > > + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
> > > +
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + i2c-gpio@0 {
> > > + compatible = "lsdc,i2c-gpio-0";
> > > + reg = <6>;

'reg' needs to be documented with some description of what 6 and 7
represent. If they are the control register offset, then make the
address translatable (use 'ranges' and define the size).

> > > + sda = <0>;
> > > + scl = <1>;

These need a vendor prefix.

> > > + };
> > > +
> > > + i2c-gpio@1 {
> > > + compatible = "lsdc,i2c-gpio-1";
> > > + reg = <7>;
> > > + sda = <2>;
> > > + scl = <3>;
> > > + };
> > > +
> > > + ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + port@0 {
> > > + reg = <0>;
> > > + endpoint {
> > > + remote-endpoint = <&vga_encoder_in>;
> > > + };
> > > + };
> > > + port@1 {
> > > + reg = <1>;
> > > + endpoint {
> > > + remote-endpoint = <&dvi_encoder_in>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + - |
> > > + #include <dt-bindings/interrupt-controller/irq.h>
> > > + bus {
> > > +
> > > + #address-cells = <3>;
> > > + #size-cells = <2>;
> > > + #interrupt-cells = <2>;
> > > +
> > > + display-controller@6,0 {
> > > + compatible = "loongson,ls2k1000-dc";
> > > + reg = <0x3100 0x0 0x0 0x0 0x0>;
> > > + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
> > > +
> > > + ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + port@0 {
> > > + reg = <0>;
> > > + endpoint {
> > > + remote-endpoint = <&panel_in>;
> > > + };
> > > + };
> > > + port@1 {
> > > + reg = <1>;
> > > + endpoint {
> > > + remote-endpoint = <&hdmi_encoder_in>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > + };
> > > +...
> > > --
> > > 2.25.1
> > >
> > >

2022-03-27 17:01:02

by Sui Jingfeng

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller


On 2022/3/24 21:26, Rob Herring wrote:
> On Thu, Mar 24, 2022 at 09:48:19AM +0800, Sui Jingfeng wrote:
>> On 2022/3/23 21:03, Rob Herring wrote:
>>> On Wed, Mar 23, 2022 at 11:38:55AM +0800, Sui Jingfeng wrote:
>>>> On 2022/3/23 04:55, Rob Herring wrote:
>>>>> On Tue, Mar 22, 2022 at 10:33:45AM +0800, Sui Jingfeng wrote:
>>>>>> On 2022/3/22 07:20, Rob Herring wrote:
>>>>>>> On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
>>>>>>>> From: suijingfeng <[email protected]>
>>>>>>>>
>>>>>>> Needs a commit message.
>>>>>>>
>>>>>>>> Signed-off-by: suijingfeng <[email protected]>
>>>>>>>> Signed-off-by: Sui Jingfeng <[email protected]>
>>>>>>> Same person? Don't need both emails.
>>>>>> Yes,  [email protected] is my company's email. But it can not be used
>>>>>> to send patches to dri-devel,
>>>>>>
>>>>>> when send patches with this email, the patch will not be shown on patch
>>>>>> works.
>>>>>>
>>>>>> Emails  are either blocked or got  rejected  by loongson's mail server.  It
>>>>>> can only receive emails
>>>>>>
>>>>>> from you and other people, but not dri-devel. so have to use my personal
>>>>>> email([email protected]) to send patches.
>>>>>>
>>>>>>>> ---
>>>>>>>> .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
>>>>>>>> 1 file changed, 230 insertions(+)
>>>>>>>> create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..7be63346289e
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>>>>>> @@ -0,0 +1,230 @@
>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>> +%YAML 1.2
>>>>>>>> +---
>>>>>>>> +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>> +
>>>>>>>> +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
>>>>>>>> +
>>>>>>>> +maintainers:
>>>>>>>> + - Sui Jingfeng <[email protected]>
>>>>>>>> +
>>>>>>>> +description: |+
>>>>>>>> +
>>>>>>>> + Loongson display controllers are simple which require scanout buffers
>>>>>>>> + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
>>>>>>>> + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
>>>>>>>> + with a dedicated video RAM which is 64MB or more, precise size can be
>>>>>>>> + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
>>>>>>>> + chip.
>>>>>>>> +
>>>>>>>> + LSDC has two display pipes, each way has a DVO interface which provide
>>>>>>>> + RGB888 signals, vertical & horizontal synchronisations, data enable and
>>>>>>>> + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
>>>>>>>> + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
>>>>>>>> +
>>>>>>>> + For LS7A1000, there are 4 dedicated GPIOs whose control register is
>>>>>>>> + located at the DC register space. They are used to emulate two way i2c,
>>>>>>>> + One for DVO0, another for DVO1.
>>>>>>>> +
>>>>>>>> + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
>>>>>>>> + general purpose GPIO emulated i2c or hardware i2c in the SoC.
>>>>>>>> +
>>>>>>>> + LSDC's display pipeline have several components as below description,
>>>>>>>> +
>>>>>>>> + The display controller in LS7A1000:
>>>>>>>> + ___________________ _________
>>>>>>>> + | -------| | |
>>>>>>>> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
>>>>>>>> + | _ _ -------| ^ ^ |_________|
>>>>>>>> + | | | | | -------| | |
>>>>>>>> + | |_| |_| | i2c0 <--------+-------------+
>>>>>>>> + | -------|
>>>>>>>> + | DC IN LS7A1000 |
>>>>>>>> + | _ _ -------|
>>>>>>>> + | | | | | | i2c1 <--------+-------------+
>>>>>>>> + | |_| |_| -------| | | _________
>>>>>>>> + | -------| | | | |
>>>>>>>> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
>>>>>>>> + | -------| |_________|
>>>>>>>> + |___________________|
>>>>>>>> +
>>>>>>>> + Simple usage of LS7A1000 with LS3A4000 CPU:
>>>>>>>> +
>>>>>>>> + +------+ +-----------------------------------+
>>>>>>>> + | DDR4 | | +-------------------+ |
>>>>>>>> + +------+ | | PCIe Root complex | LS7A1000 |
>>>>>>>> + || MC0 | +--++---------++----+ |
>>>>>>>> + +----------+ HT 3.0 | || || |
>>>>>>>> + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
>>>>>>>> + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
>>>>>>>> + +----------+ | +--------+ +-+--+-+ +---------+ +------+
>>>>>>>> + || MC1 +---------------|--|----------------+
>>>>>>>> + +------+ | |
>>>>>>>> + | DDR4 | +-------+ DVO0 | | DVO1 +------+
>>>>>>>> + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
>>>>>>>> + +-------+ +------+
>>>>>>>> +
>>>>>>>> + The display controller in LS2K1000/LS2K0500:
>>>>>>>> + ___________________ _________
>>>>>>>> + | -------| | |
>>>>>>>> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
>>>>>>>> + | _ _ -------| ^ ^ |_________|
>>>>>>>> + | | | | | | | |
>>>>>>>> + | |_| |_| | +------+ |
>>>>>>>> + | <---->| i2c0 |<---------+
>>>>>>>> + | DC IN LS2K1000 | +------+
>>>>>>>> + | _ _ | +------+
>>>>>>>> + | | | | | <---->| i2c1 |----------+
>>>>>>>> + | |_| |_| | +------+ | _________
>>>>>>>> + | -------| | | | |
>>>>>>>> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
>>>>>>>> + | -------| |_________|
>>>>>>>> + |___________________|
>>>>>>>> +
>>>>>>>> +properties:
>>>>>>>> + $nodename:
>>>>>>>> + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
>>>>>>>> +
>>>>>>>> + compatible:
>>>>>>>> + oneOf:
>>>>>>>> + - items:
>>>>>>>> + - enum:
>>>>>>>> + - loongson,ls7a1000-dc
>>>>>>>> + - loongson,ls2k1000-dc
>>>>>>>> + - loongson,ls2k0500-dc
>>>>>>>> +
>>>>>>>> + reg:
>>>>>>>> + maxItems: 1
>>>>>>>> +
>>>>>>>> + interrupts:
>>>>>>>> + maxItems: 1
>>>>>>>> +
>>>>>>>> + '#address-cells':
>>>>>>>> + const: 1
>>>>>>>> +
>>>>>>>> + '#size-cells':
>>>>>>>> + const: 0
>>>>>>>> +
>>>>>>>> + i2c-gpio@0:
>>>>>>>> + description: |
>>>>>>>> + Built-in GPIO emulate i2c exported for external display bridge
>>>>>>> If you have i2c-gpio, that belongs at the DT top-level, not here.
>>>>>>>
>>>>>>>> + configuration, onitor detection and edid read back etc, for ls7a1000
>>>>>>>> + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
>>>>>>> No, there's a defined i2c-gpio compatible already.
>>>>>> This is different from the i2c-gpio already defined under drivers/i2c/busses/i2c-gpio.c,
>>>>>> By design, my i2c-gpio is vendor specific properties, lsdc device driver create the i2c
>>>>>> adapter at runtime. These are 4 dedicated GPIOs whose control register is located at the
>>>>>> LSDC register space, not general purpose GPIOs with separate control register resource.
>>>>>> So i think it is the child node of display-controller@6,1, it belongs to LSDC.
>>>>>> It seems that put it at the DT top-level break the hierarchy and relationship.
>>>>> Okay, I see. Then just 'i2c' for the node names. You need a reference to
>>>>> i2c-controller.yaml for these nodes too.
>>>>>
>>>>> The compatible should not have an index in it.
>>>> OK, i will fix this at the next version. thanks.
>>>>>>>> + used to specify a I2c adapter bus number, if you don't specify one
>>>>>>>> + i2c driver core will dynamically assign a bus number. Please specify
>>>>>>> Bus numbers are a linux detail not relevant to DT binding.
>>>>>>>
>>>>>>>> + it only when its bus number matters. Bus number greater than 6 is safe
>>>>>>>> + because ls7a1000 bridge have 6 hardware I2C controller integrated.
>>>>>>>> +
>>>>>>>> + i2c-gpio@1:
>>>>>>>> + description: |
>>>>>>>> + Built-in GPIO emulate i2c exported for external display bridge
>>>>>>>> + configuration, onitor detection and edid read back etc, for ls7a1000
>>>>>>>> + only. Its compatible must be lsdc,i2c-gpio-1.
>>>>>>>> +
>>>>>>>> + ports:
>>>>>>>> + $ref: /schemas/graph.yaml#/properties/ports
>>>>>>>> +
>>>>>>>> + properties:
>>>>>>>> + port@0:
>>>>>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
>>>>>>>> +
>>>>>>>> + port@1:
>>>>>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
>>>>>>>> +
>>>>>>>> + required:
>>>>>>>> + - port@0
>>>>>>>> + - port@1
>>>>>>>> +
>>>>>>>> +required:
>>>>>>>> + - compatible
>>>>>>>> + - reg
>>>>>>>> + - interrupts
>>>>>>>> + - ports
>>>>>>>> +
>>>>>>>> +additionalProperties: false
>>>>>>>> +
>>>>>>>> +examples:
>>>>>>>> + - |
>>>>>>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>>>>>>> + bus {
>>>>>>>> +
>>>>>>>> + #address-cells = <3>;
>>>>>>>> + #size-cells = <2>;
>>>>>>>> + #interrupt-cells = <2>;
>>>>>>>> +
>>>>>>>> + display-controller@6,1 {
>>>>>>>> + compatible = "loongson,ls7a1000-dc";
>>>>>>>> + reg = <0x3100 0x0 0x0 0x0 0x0>;
>>>>>>>> + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
>>>>>>>> +
>>>>>>>> + #address-cells = <1>;
>>>>>>>> + #size-cells = <0>;
>>>>>>>> +
>>>>>>>> + i2c-gpio@0 {
>>>>>>>> + compatible = "lsdc,i2c-gpio-0";
>>>>>>>> + reg = <6>;
>>>>> 'reg' needs to be documented with some description of what 6 and 7
>>>>> represent. If they are the control register offset, then make the
>>>>> address translatable (use 'ranges' and define the size).
>>>> By design, the reg property is used to specify a I2c adapter bus number,
>>>> if we don't specify one, i2c driver core will dynamically assign a bus number.
>>>> then the nr of the i2c adapter will started from 0. I want is start from 6
>>>> to avoid potential conflict feature hardware I2C driver.
>>>>
>>>> Because LS7A1000 bridge chip have 6 hardware I2C controller integrated,
>>>> but its driver is not up-streamed yet. By default these hardware I2C controller's
>>>> nr is started from 0.
>>> Linux's numbering doesn't belong in DT. So no, you can't use 'reg' in
>>> that way.
>> Then,  can i use something like lsdc,nr = <6> ?
>>>> Even through i2c driver core can dynamically generate a number, i still want it
>>>> to be fixed and keep consistent and explicit. That is, i2c6 is for display pipe 0,
>>>> i2c7 is for display pipe 1. This follow the convention and flexible enough.
>>> You may want that, but that is not how the kernel works. Specific
>>> numbers are not guaranteed. I'm sure you've seen this for disks, network
>>> interfaces, etc.
>>>
>>> Rob
>> 2c_bit_add_numbered_bus() will guarantee it for you as long as If no devices
>> have pre-been declared for this bus.
>>
>> you can read the comment of 2c_bit_add_numbered_bus() at
>> drivers/i2c/i2c-core-base.c
> I didn't say it wasn't possible. It is not best practice. Grep
> i2c_bit_add_numbered_bus and see how many users there are.

i2c-gpio.c at drivers/i2c/busses/ just do the same thing.


+ nvidia,bpmp-bus-id: + $ref: /schemas/types.yaml#/definitions/uint32 +
description: Indicates the I2C bus number this DT node represents, + as
defined by the BPMP firmware.

> Even if the kernel allows specifying bus numbers,your Linux bus numbers don't
> belong in DT.

Again, Does does devicetree specification prohibit this?

Nvidia also put i2c bus number in the DT, we learn that from nvidia. [1][2]

[1] Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.yaml

[2]
https://lore.kernel.org/all/[email protected]/


>
> Rob

2022-03-28 16:44:40

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller

On Sat, Mar 26, 2022 at 06:04:46PM +0800, Sui Jingfeng wrote:
>
> On 2022/3/24 21:26, Rob Herring wrote:
> > On Thu, Mar 24, 2022 at 09:48:19AM +0800, Sui Jingfeng wrote:
> > > On 2022/3/23 21:03, Rob Herring wrote:
> > > > On Wed, Mar 23, 2022 at 11:38:55AM +0800, Sui Jingfeng wrote:
> > > > > On 2022/3/23 04:55, Rob Herring wrote:
> > > > > > On Tue, Mar 22, 2022 at 10:33:45AM +0800, Sui Jingfeng wrote:
> > > > > > > On 2022/3/22 07:20, Rob Herring wrote:
> > > > > > > > On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
> > > > > > > > > From: suijingfeng <[email protected]>
> > > > > > > > >
> > > > > > > > Needs a commit message.
> > > > > > > >
> > > > > > > > > Signed-off-by: suijingfeng <[email protected]>
> > > > > > > > > Signed-off-by: Sui Jingfeng <[email protected]>
> > > > > > > > Same person? Don't need both emails.
> > > > > > > Yes,? [email protected] is my company's email. But it can not be used
> > > > > > > to send patches to dri-devel,
> > > > > > >
> > > > > > > when send patches with this email, the patch will not be shown on patch
> > > > > > > works.
> > > > > > >
> > > > > > > Emails? are either blocked or got? rejected? by loongson's mail server.? It
> > > > > > > can only receive emails
> > > > > > >
> > > > > > > from you and other people, but not dri-devel. so have to use my personal
> > > > > > > email([email protected]) to send patches.
> > > > > > >
> > > > > > > > > ---
> > > > > > > > > .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
> > > > > > > > > 1 file changed, 230 insertions(+)
> > > > > > > > > create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000000..7be63346289e
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > > > > > @@ -0,0 +1,230 @@
> > > > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > > > > +%YAML 1.2
> > > > > > > > > +---
> > > > > > > > > +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
> > > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > > +
> > > > > > > > > +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
> > > > > > > > > +
> > > > > > > > > +maintainers:
> > > > > > > > > + - Sui Jingfeng <[email protected]>
> > > > > > > > > +
> > > > > > > > > +description: |+
> > > > > > > > > +
> > > > > > > > > + Loongson display controllers are simple which require scanout buffers
> > > > > > > > > + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
> > > > > > > > > + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
> > > > > > > > > + with a dedicated video RAM which is 64MB or more, precise size can be
> > > > > > > > > + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
> > > > > > > > > + chip.
> > > > > > > > > +
> > > > > > > > > + LSDC has two display pipes, each way has a DVO interface which provide
> > > > > > > > > + RGB888 signals, vertical & horizontal synchronisations, data enable and
> > > > > > > > > + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
> > > > > > > > > + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
> > > > > > > > > +
> > > > > > > > > + For LS7A1000, there are 4 dedicated GPIOs whose control register is
> > > > > > > > > + located at the DC register space. They are used to emulate two way i2c,
> > > > > > > > > + One for DVO0, another for DVO1.
> > > > > > > > > +
> > > > > > > > > + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
> > > > > > > > > + general purpose GPIO emulated i2c or hardware i2c in the SoC.
> > > > > > > > > +
> > > > > > > > > + LSDC's display pipeline have several components as below description,
> > > > > > > > > +
> > > > > > > > > + The display controller in LS7A1000:
> > > > > > > > > + ___________________ _________
> > > > > > > > > + | -------| | |
> > > > > > > > > + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> > > > > > > > > + | _ _ -------| ^ ^ |_________|
> > > > > > > > > + | | | | | -------| | |
> > > > > > > > > + | |_| |_| | i2c0 <--------+-------------+
> > > > > > > > > + | -------|
> > > > > > > > > + | DC IN LS7A1000 |
> > > > > > > > > + | _ _ -------|
> > > > > > > > > + | | | | | | i2c1 <--------+-------------+
> > > > > > > > > + | |_| |_| -------| | | _________
> > > > > > > > > + | -------| | | | |
> > > > > > > > > + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> > > > > > > > > + | -------| |_________|
> > > > > > > > > + |___________________|
> > > > > > > > > +
> > > > > > > > > + Simple usage of LS7A1000 with LS3A4000 CPU:
> > > > > > > > > +
> > > > > > > > > + +------+ +-----------------------------------+
> > > > > > > > > + | DDR4 | | +-------------------+ |
> > > > > > > > > + +------+ | | PCIe Root complex | LS7A1000 |
> > > > > > > > > + || MC0 | +--++---------++----+ |
> > > > > > > > > + +----------+ HT 3.0 | || || |
> > > > > > > > > + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
> > > > > > > > > + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
> > > > > > > > > + +----------+ | +--------+ +-+--+-+ +---------+ +------+
> > > > > > > > > + || MC1 +---------------|--|----------------+
> > > > > > > > > + +------+ | |
> > > > > > > > > + | DDR4 | +-------+ DVO0 | | DVO1 +------+
> > > > > > > > > + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
> > > > > > > > > + +-------+ +------+
> > > > > > > > > +
> > > > > > > > > + The display controller in LS2K1000/LS2K0500:
> > > > > > > > > + ___________________ _________
> > > > > > > > > + | -------| | |
> > > > > > > > > + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> > > > > > > > > + | _ _ -------| ^ ^ |_________|
> > > > > > > > > + | | | | | | | |
> > > > > > > > > + | |_| |_| | +------+ |
> > > > > > > > > + | <---->| i2c0 |<---------+
> > > > > > > > > + | DC IN LS2K1000 | +------+
> > > > > > > > > + | _ _ | +------+
> > > > > > > > > + | | | | | <---->| i2c1 |----------+
> > > > > > > > > + | |_| |_| | +------+ | _________
> > > > > > > > > + | -------| | | | |
> > > > > > > > > + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> > > > > > > > > + | -------| |_________|
> > > > > > > > > + |___________________|
> > > > > > > > > +
> > > > > > > > > +properties:
> > > > > > > > > + $nodename:
> > > > > > > > > + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
> > > > > > > > > +
> > > > > > > > > + compatible:
> > > > > > > > > + oneOf:
> > > > > > > > > + - items:
> > > > > > > > > + - enum:
> > > > > > > > > + - loongson,ls7a1000-dc
> > > > > > > > > + - loongson,ls2k1000-dc
> > > > > > > > > + - loongson,ls2k0500-dc
> > > > > > > > > +
> > > > > > > > > + reg:
> > > > > > > > > + maxItems: 1
> > > > > > > > > +
> > > > > > > > > + interrupts:
> > > > > > > > > + maxItems: 1
> > > > > > > > > +
> > > > > > > > > + '#address-cells':
> > > > > > > > > + const: 1
> > > > > > > > > +
> > > > > > > > > + '#size-cells':
> > > > > > > > > + const: 0
> > > > > > > > > +
> > > > > > > > > + i2c-gpio@0:
> > > > > > > > > + description: |
> > > > > > > > > + Built-in GPIO emulate i2c exported for external display bridge
> > > > > > > > If you have i2c-gpio, that belongs at the DT top-level, not here.
> > > > > > > >
> > > > > > > > > + configuration, onitor detection and edid read back etc, for ls7a1000
> > > > > > > > > + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
> > > > > > > > No, there's a defined i2c-gpio compatible already.
> > > > > > > This is different from the i2c-gpio already defined under drivers/i2c/busses/i2c-gpio.c,
> > > > > > > By design, my i2c-gpio is vendor specific properties, lsdc device driver create the i2c
> > > > > > > adapter at runtime. These are 4 dedicated GPIOs whose control register is located at the
> > > > > > > LSDC register space, not general purpose GPIOs with separate control register resource.
> > > > > > > So i think it is the child node of?display-controller@6,1,?it belongs to LSDC.
> > > > > > > It seems that put it at the DT top-level break the hierarchy and relationship.
> > > > > > Okay, I see. Then just 'i2c' for the node names. You need a reference to
> > > > > > i2c-controller.yaml for these nodes too.
> > > > > >
> > > > > > The compatible should not have an index in it.
> > > > > OK, i will fix this at the next version. thanks.
> > > > > > > > > + used to specify a I2c adapter bus number, if you don't specify one
> > > > > > > > > + i2c driver core will dynamically assign a bus number. Please specify
> > > > > > > > Bus numbers are a linux detail not relevant to DT binding.
> > > > > > > >
> > > > > > > > > + it only when its bus number matters. Bus number greater than 6 is safe
> > > > > > > > > + because ls7a1000 bridge have 6 hardware I2C controller integrated.
> > > > > > > > > +
> > > > > > > > > + i2c-gpio@1:
> > > > > > > > > + description: |
> > > > > > > > > + Built-in GPIO emulate i2c exported for external display bridge
> > > > > > > > > + configuration, onitor detection and edid read back etc, for ls7a1000
> > > > > > > > > + only. Its compatible must be lsdc,i2c-gpio-1.
> > > > > > > > > +
> > > > > > > > > + ports:
> > > > > > > > > + $ref: /schemas/graph.yaml#/properties/ports
> > > > > > > > > +
> > > > > > > > > + properties:
> > > > > > > > > + port@0:
> > > > > > > > > + $ref: /schemas/graph.yaml#/properties/port
> > > > > > > > > + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> > > > > > > > > +
> > > > > > > > > + port@1:
> > > > > > > > > + $ref: /schemas/graph.yaml#/properties/port
> > > > > > > > > + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> > > > > > > > > +
> > > > > > > > > + required:
> > > > > > > > > + - port@0
> > > > > > > > > + - port@1
> > > > > > > > > +
> > > > > > > > > +required:
> > > > > > > > > + - compatible
> > > > > > > > > + - reg
> > > > > > > > > + - interrupts
> > > > > > > > > + - ports
> > > > > > > > > +
> > > > > > > > > +additionalProperties: false
> > > > > > > > > +
> > > > > > > > > +examples:
> > > > > > > > > + - |
> > > > > > > > > + #include <dt-bindings/interrupt-controller/irq.h>
> > > > > > > > > + bus {
> > > > > > > > > +
> > > > > > > > > + #address-cells = <3>;
> > > > > > > > > + #size-cells = <2>;
> > > > > > > > > + #interrupt-cells = <2>;
> > > > > > > > > +
> > > > > > > > > + display-controller@6,1 {
> > > > > > > > > + compatible = "loongson,ls7a1000-dc";
> > > > > > > > > + reg = <0x3100 0x0 0x0 0x0 0x0>;
> > > > > > > > > + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > > > +
> > > > > > > > > + #address-cells = <1>;
> > > > > > > > > + #size-cells = <0>;
> > > > > > > > > +
> > > > > > > > > + i2c-gpio@0 {
> > > > > > > > > + compatible = "lsdc,i2c-gpio-0";
> > > > > > > > > + reg = <6>;
> > > > > > 'reg' needs to be documented with some description of what 6 and 7
> > > > > > represent. If they are the control register offset, then make the
> > > > > > address translatable (use 'ranges' and define the size).
> > > > > By design, the reg property is used to specify a I2c adapter bus number,
> > > > > if we don't specify one, i2c driver core will dynamically assign a bus number.
> > > > > then the nr of the i2c adapter will started from 0. I want is start from 6
> > > > > to avoid potential conflict feature hardware I2C driver.
> > > > >
> > > > > Because LS7A1000 bridge chip have 6 hardware I2C controller integrated,
> > > > > but its driver is not up-streamed yet. By default these hardware I2C controller's
> > > > > nr is started from 0.
> > > > Linux's numbering doesn't belong in DT. So no, you can't use 'reg' in
> > > > that way.
> > > Then,? can i use something like lsdc,nr = <6> ?
> > > > > Even through i2c driver core can dynamically generate a number, i still want it
> > > > > to be fixed and keep consistent and explicit. That is, i2c6 is for display pipe 0,
> > > > > i2c7 is for display pipe 1. This follow the convention and flexible enough.
> > > > You may want that, but that is not how the kernel works. Specific
> > > > numbers are not guaranteed. I'm sure you've seen this for disks, network
> > > > interfaces, etc.
> > > >
> > > > Rob
> > > 2c_bit_add_numbered_bus() will guarantee it for you as long as If no devices
> > > have pre-been declared for this bus.
> > >
> > > you can read the comment of 2c_bit_add_numbered_bus() at
> > > drivers/i2c/i2c-core-base.c
> > I didn't say it wasn't possible. It is not best practice. Grep
> > i2c_bit_add_numbered_bus and see how many users there are.
>
> i2c-gpio.c at drivers/i2c/busses/ just do the same thing.

No, the id for i2c-gpio nodes (any DT node without 'reg') will be -1
which means dynamically assigned.


> + nvidia,bpmp-bus-id: + $ref: /schemas/types.yaml#/definitions/uint32 +
> description: Indicates the I2C bus number this DT node represents, + as
> defined by the BPMP firmware.

The key difference is the numbering is defined by the BPMP firmware.


> > Even if the kernel allows specifying bus numbers,your Linux bus numbers don't
> > belong in DT.
>
> Again, Does does devicetree specification prohibit this?

No. The spec is not the last word on what's allowed or not. Lots of
patterns exist already which we can't change, but that doesn't mean they
should be copied by new users.

Rob

2022-03-29 02:17:27

by Sui Jingfeng

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller


On 2022/3/28 22:04, Rob Herring wrote:
> On Sat, Mar 26, 2022 at 06:04:46PM +0800, Sui Jingfeng wrote:
>> On 2022/3/24 21:26, Rob Herring wrote:
>>> On Thu, Mar 24, 2022 at 09:48:19AM +0800, Sui Jingfeng wrote:
>>>> On 2022/3/23 21:03, Rob Herring wrote:
>>>>> On Wed, Mar 23, 2022 at 11:38:55AM +0800, Sui Jingfeng wrote:
>>>>>> On 2022/3/23 04:55, Rob Herring wrote:
>>>>>>> On Tue, Mar 22, 2022 at 10:33:45AM +0800, Sui Jingfeng wrote:
>>>>>>>> On 2022/3/22 07:20, Rob Herring wrote:
>>>>>>>>> On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
>>>>>>>>>> From: suijingfeng <[email protected]>
>>>>>>>>>>
>>>>>>>>> Needs a commit message.
>>>>>>>>>
>>>>>>>>>> Signed-off-by: suijingfeng <[email protected]>
>>>>>>>>>> Signed-off-by: Sui Jingfeng <[email protected]>
>>>>>>>>> Same person? Don't need both emails.
>>>>>>>> Yes,  [email protected] is my company's email. But it can not be used
>>>>>>>> to send patches to dri-devel,
>>>>>>>>
>>>>>>>> when send patches with this email, the patch will not be shown on patch
>>>>>>>> works.
>>>>>>>>
>>>>>>>> Emails  are either blocked or got  rejected  by loongson's mail server.  It
>>>>>>>> can only receive emails
>>>>>>>>
>>>>>>>> from you and other people, but not dri-devel. so have to use my personal
>>>>>>>> email([email protected]) to send patches.
>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
>>>>>>>>>> 1 file changed, 230 insertions(+)
>>>>>>>>>> create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..7be63346289e
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>>>>>>>>> @@ -0,0 +1,230 @@
>>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>>>> +%YAML 1.2
>>>>>>>>>> +---
>>>>>>>>>> +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
>>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>>> +
>>>>>>>>>> +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
>>>>>>>>>> +
>>>>>>>>>> +maintainers:
>>>>>>>>>> + - Sui Jingfeng <[email protected]>
>>>>>>>>>> +
>>>>>>>>>> +description: |+
>>>>>>>>>> +
>>>>>>>>>> + Loongson display controllers are simple which require scanout buffers
>>>>>>>>>> + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
>>>>>>>>>> + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
>>>>>>>>>> + with a dedicated video RAM which is 64MB or more, precise size can be
>>>>>>>>>> + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
>>>>>>>>>> + chip.
>>>>>>>>>> +
>>>>>>>>>> + LSDC has two display pipes, each way has a DVO interface which provide
>>>>>>>>>> + RGB888 signals, vertical & horizontal synchronisations, data enable and
>>>>>>>>>> + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
>>>>>>>>>> + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
>>>>>>>>>> +
>>>>>>>>>> + For LS7A1000, there are 4 dedicated GPIOs whose control register is
>>>>>>>>>> + located at the DC register space. They are used to emulate two way i2c,
>>>>>>>>>> + One for DVO0, another for DVO1.
>>>>>>>>>> +
>>>>>>>>>> + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
>>>>>>>>>> + general purpose GPIO emulated i2c or hardware i2c in the SoC.
>>>>>>>>>> +
>>>>>>>>>> + LSDC's display pipeline have several components as below description,
>>>>>>>>>> +
>>>>>>>>>> + The display controller in LS7A1000:
>>>>>>>>>> + ___________________ _________
>>>>>>>>>> + | -------| | |
>>>>>>>>>> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
>>>>>>>>>> + | _ _ -------| ^ ^ |_________|
>>>>>>>>>> + | | | | | -------| | |
>>>>>>>>>> + | |_| |_| | i2c0 <--------+-------------+
>>>>>>>>>> + | -------|
>>>>>>>>>> + | DC IN LS7A1000 |
>>>>>>>>>> + | _ _ -------|
>>>>>>>>>> + | | | | | | i2c1 <--------+-------------+
>>>>>>>>>> + | |_| |_| -------| | | _________
>>>>>>>>>> + | -------| | | | |
>>>>>>>>>> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
>>>>>>>>>> + | -------| |_________|
>>>>>>>>>> + |___________________|
>>>>>>>>>> +
>>>>>>>>>> + Simple usage of LS7A1000 with LS3A4000 CPU:
>>>>>>>>>> +
>>>>>>>>>> + +------+ +-----------------------------------+
>>>>>>>>>> + | DDR4 | | +-------------------+ |
>>>>>>>>>> + +------+ | | PCIe Root complex | LS7A1000 |
>>>>>>>>>> + || MC0 | +--++---------++----+ |
>>>>>>>>>> + +----------+ HT 3.0 | || || |
>>>>>>>>>> + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
>>>>>>>>>> + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
>>>>>>>>>> + +----------+ | +--------+ +-+--+-+ +---------+ +------+
>>>>>>>>>> + || MC1 +---------------|--|----------------+
>>>>>>>>>> + +------+ | |
>>>>>>>>>> + | DDR4 | +-------+ DVO0 | | DVO1 +------+
>>>>>>>>>> + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
>>>>>>>>>> + +-------+ +------+
>>>>>>>>>> +
>>>>>>>>>> + The display controller in LS2K1000/LS2K0500:
>>>>>>>>>> + ___________________ _________
>>>>>>>>>> + | -------| | |
>>>>>>>>>> + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
>>>>>>>>>> + | _ _ -------| ^ ^ |_________|
>>>>>>>>>> + | | | | | | | |
>>>>>>>>>> + | |_| |_| | +------+ |
>>>>>>>>>> + | <---->| i2c0 |<---------+
>>>>>>>>>> + | DC IN LS2K1000 | +------+
>>>>>>>>>> + | _ _ | +------+
>>>>>>>>>> + | | | | | <---->| i2c1 |----------+
>>>>>>>>>> + | |_| |_| | +------+ | _________
>>>>>>>>>> + | -------| | | | |
>>>>>>>>>> + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
>>>>>>>>>> + | -------| |_________|
>>>>>>>>>> + |___________________|
>>>>>>>>>> +
>>>>>>>>>> +properties:
>>>>>>>>>> + $nodename:
>>>>>>>>>> + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
>>>>>>>>>> +
>>>>>>>>>> + compatible:
>>>>>>>>>> + oneOf:
>>>>>>>>>> + - items:
>>>>>>>>>> + - enum:
>>>>>>>>>> + - loongson,ls7a1000-dc
>>>>>>>>>> + - loongson,ls2k1000-dc
>>>>>>>>>> + - loongson,ls2k0500-dc
>>>>>>>>>> +
>>>>>>>>>> + reg:
>>>>>>>>>> + maxItems: 1
>>>>>>>>>> +
>>>>>>>>>> + interrupts:
>>>>>>>>>> + maxItems: 1
>>>>>>>>>> +
>>>>>>>>>> + '#address-cells':
>>>>>>>>>> + const: 1
>>>>>>>>>> +
>>>>>>>>>> + '#size-cells':
>>>>>>>>>> + const: 0
>>>>>>>>>> +
>>>>>>>>>> + i2c-gpio@0:
>>>>>>>>>> + description: |
>>>>>>>>>> + Built-in GPIO emulate i2c exported for external display bridge
>>>>>>>>> If you have i2c-gpio, that belongs at the DT top-level, not here.
>>>>>>>>>
>>>>>>>>>> + configuration, onitor detection and edid read back etc, for ls7a1000
>>>>>>>>>> + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
>>>>>>>>> No, there's a defined i2c-gpio compatible already.
>>>>>>>> This is different from the i2c-gpio already defined under drivers/i2c/busses/i2c-gpio.c,
>>>>>>>> By design, my i2c-gpio is vendor specific properties, lsdc device driver create the i2c
>>>>>>>> adapter at runtime. These are 4 dedicated GPIOs whose control register is located at the
>>>>>>>> LSDC register space, not general purpose GPIOs with separate control register resource.
>>>>>>>> So i think it is the child node of display-controller@6,1, it belongs to LSDC.
>>>>>>>> It seems that put it at the DT top-level break the hierarchy and relationship.
>>>>>>> Okay, I see. Then just 'i2c' for the node names. You need a reference to
>>>>>>> i2c-controller.yaml for these nodes too.
>>>>>>>
>>>>>>> The compatible should not have an index in it.
>>>>>> OK, i will fix this at the next version. thanks.
>>>>>>>>>> + used to specify a I2c adapter bus number, if you don't specify one
>>>>>>>>>> + i2c driver core will dynamically assign a bus number. Please specify
>>>>>>>>> Bus numbers are a linux detail not relevant to DT binding.
>>>>>>>>>
>>>>>>>>>> + it only when its bus number matters. Bus number greater than 6 is safe
>>>>>>>>>> + because ls7a1000 bridge have 6 hardware I2C controller integrated.
>>>>>>>>>> +
>>>>>>>>>> + i2c-gpio@1:
>>>>>>>>>> + description: |
>>>>>>>>>> + Built-in GPIO emulate i2c exported for external display bridge
>>>>>>>>>> + configuration, onitor detection and edid read back etc, for ls7a1000
>>>>>>>>>> + only. Its compatible must be lsdc,i2c-gpio-1.
>>>>>>>>>> +
>>>>>>>>>> + ports:
>>>>>>>>>> + $ref: /schemas/graph.yaml#/properties/ports
>>>>>>>>>> +
>>>>>>>>>> + properties:
>>>>>>>>>> + port@0:
>>>>>>>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>>>>>>>> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
>>>>>>>>>> +
>>>>>>>>>> + port@1:
>>>>>>>>>> + $ref: /schemas/graph.yaml#/properties/port
>>>>>>>>>> + description: output port node connected with DPI panels or external encoders, with only one endpoint.
>>>>>>>>>> +
>>>>>>>>>> + required:
>>>>>>>>>> + - port@0
>>>>>>>>>> + - port@1
>>>>>>>>>> +
>>>>>>>>>> +required:
>>>>>>>>>> + - compatible
>>>>>>>>>> + - reg
>>>>>>>>>> + - interrupts
>>>>>>>>>> + - ports
>>>>>>>>>> +
>>>>>>>>>> +additionalProperties: false
>>>>>>>>>> +
>>>>>>>>>> +examples:
>>>>>>>>>> + - |
>>>>>>>>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>>>>>>>>> + bus {
>>>>>>>>>> +
>>>>>>>>>> + #address-cells = <3>;
>>>>>>>>>> + #size-cells = <2>;
>>>>>>>>>> + #interrupt-cells = <2>;
>>>>>>>>>> +
>>>>>>>>>> + display-controller@6,1 {
>>>>>>>>>> + compatible = "loongson,ls7a1000-dc";
>>>>>>>>>> + reg = <0x3100 0x0 0x0 0x0 0x0>;
>>>>>>>>>> + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
>>>>>>>>>> +
>>>>>>>>>> + #address-cells = <1>;
>>>>>>>>>> + #size-cells = <0>;
>>>>>>>>>> +
>>>>>>>>>> + i2c-gpio@0 {
>>>>>>>>>> + compatible = "lsdc,i2c-gpio-0";
>>>>>>>>>> + reg = <6>;
>>>>>>> 'reg' needs to be documented with some description of what 6 and 7
>>>>>>> represent. If they are the control register offset, then make the
>>>>>>> address translatable (use 'ranges' and define the size).
>>>>>> By design, the reg property is used to specify a I2c adapter bus number,
>>>>>> if we don't specify one, i2c driver core will dynamically assign a bus number.
>>>>>> then the nr of the i2c adapter will started from 0. I want is start from 6
>>>>>> to avoid potential conflict feature hardware I2C driver.
>>>>>>
>>>>>> Because LS7A1000 bridge chip have 6 hardware I2C controller integrated,
>>>>>> but its driver is not up-streamed yet. By default these hardware I2C controller's
>>>>>> nr is started from 0.
>>>>> Linux's numbering doesn't belong in DT. So no, you can't use 'reg' in
>>>>> that way.
>>>> Then,  can i use something like lsdc,nr = <6> ?
>>>>>> Even through i2c driver core can dynamically generate a number, i still want it
>>>>>> to be fixed and keep consistent and explicit. That is, i2c6 is for display pipe 0,
>>>>>> i2c7 is for display pipe 1. This follow the convention and flexible enough.
>>>>> You may want that, but that is not how the kernel works. Specific
>>>>> numbers are not guaranteed. I'm sure you've seen this for disks, network
>>>>> interfaces, etc.
>>>>>
>>>>> Rob
>>>> 2c_bit_add_numbered_bus() will guarantee it for you as long as If no devices
>>>> have pre-been declared for this bus.
>>>>
>>>> you can read the comment of 2c_bit_add_numbered_bus() at
>>>> drivers/i2c/i2c-core-base.c
>>> I didn't say it wasn't possible. It is not best practice. Grep
>>> i2c_bit_add_numbered_bus and see how many users there are.
>> i2c-gpio.c at drivers/i2c/busses/ just do the same thing.
> No, the id for i2c-gpio nodes (any DT node without 'reg') will be -1
> which means dynamically assigned.
>
>
>> + nvidia,bpmp-bus-id: + $ref: /schemas/types.yaml#/definitions/uint32 +
>> description: Indicates the I2C bus number this DT node represents, + as
>> defined by the BPMP firmware.
> The key difference is the numbering is defined by the BPMP firmware.
>
Bus numbers are a linux detail, I am not sure it is relevant to BPMP firmware.
and BPMP firmware is not relevant here.

The point is you applied that patch, you are admit the fact that bus numbers
could be in DT.

>>> Even if the kernel allows specifying bus numbers,your Linux bus numbers don't
>>> belong in DT.
>> Again, Does does devicetree specification prohibit this?
> No. The spec is not the last word on what's allowed or not. Lots of
> patterns exist already which we can't change, but that doesn't mean they
> should be copied by new users.
>
> Rob

We develop that part by our own, only find that there are someone do the
same thing when it got push back.

We believe that it is very flexible actually, anyway if you still don't
change you mind we can rewrite it.

I have resend my patches,  see it at
https://patchwork.freedesktop.org/series/101843/

Please take spare time to review it if you would like, thanks you.

2022-03-30 10:31:48

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v11 5/7] dt-bindings: display: Add Loongson display controller

On Tue, Mar 29, 2022 at 10:02:11AM +0800, Sui Jingfeng wrote:
>
> On 2022/3/28 22:04, Rob Herring wrote:
> > On Sat, Mar 26, 2022 at 06:04:46PM +0800, Sui Jingfeng wrote:
> > > On 2022/3/24 21:26, Rob Herring wrote:
> > > > On Thu, Mar 24, 2022 at 09:48:19AM +0800, Sui Jingfeng wrote:
> > > > > On 2022/3/23 21:03, Rob Herring wrote:
> > > > > > On Wed, Mar 23, 2022 at 11:38:55AM +0800, Sui Jingfeng wrote:
> > > > > > > On 2022/3/23 04:55, Rob Herring wrote:
> > > > > > > > On Tue, Mar 22, 2022 at 10:33:45AM +0800, Sui Jingfeng wrote:
> > > > > > > > > On 2022/3/22 07:20, Rob Herring wrote:
> > > > > > > > > > On Tue, Mar 22, 2022 at 12:29:14AM +0800, Sui Jingfeng wrote:
> > > > > > > > > > > From: suijingfeng <[email protected]>
> > > > > > > > > > >
> > > > > > > > > > Needs a commit message.
> > > > > > > > > >
> > > > > > > > > > > Signed-off-by: suijingfeng <[email protected]>
> > > > > > > > > > > Signed-off-by: Sui Jingfeng <[email protected]>
> > > > > > > > > > Same person? Don't need both emails.
> > > > > > > > > Yes,? [email protected] is my company's email. But it can not be used
> > > > > > > > > to send patches to dri-devel,
> > > > > > > > >
> > > > > > > > > when send patches with this email, the patch will not be shown on patch
> > > > > > > > > works.
> > > > > > > > >
> > > > > > > > > Emails? are either blocked or got? rejected? by loongson's mail server.? It
> > > > > > > > > can only receive emails
> > > > > > > > >
> > > > > > > > > from you and other people, but not dri-devel. so have to use my personal
> > > > > > > > > email([email protected]) to send patches.
> > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > > .../loongson/loongson,display-controller.yaml | 230 ++++++++++++++++++
> > > > > > > > > > > 1 file changed, 230 insertions(+)
> > > > > > > > > > > create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > > > > > > > new file mode 100644
> > > > > > > > > > > index 000000000000..7be63346289e
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> > > > > > > > > > > @@ -0,0 +1,230 @@
> > > > > > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > > > > > > +%YAML 1.2
> > > > > > > > > > > +---
> > > > > > > > > > > +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
> > > > > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > > > > +
> > > > > > > > > > > +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree Bindings
> > > > > > > > > > > +
> > > > > > > > > > > +maintainers:
> > > > > > > > > > > + - Sui Jingfeng <[email protected]>
> > > > > > > > > > > +
> > > > > > > > > > > +description: |+
> > > > > > > > > > > +
> > > > > > > > > > > + Loongson display controllers are simple which require scanout buffers
> > > > > > > > > > > + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system
> > > > > > > > > > > + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped
> > > > > > > > > > > + with a dedicated video RAM which is 64MB or more, precise size can be
> > > > > > > > > > > + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge
> > > > > > > > > > > + chip.
> > > > > > > > > > > +
> > > > > > > > > > > + LSDC has two display pipes, each way has a DVO interface which provide
> > > > > > > > > > > + RGB888 signals, vertical & horizontal synchronisations, data enable and
> > > > > > > > > > > + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from
> > > > > > > > > > > + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers.
> > > > > > > > > > > +
> > > > > > > > > > > + For LS7A1000, there are 4 dedicated GPIOs whose control register is
> > > > > > > > > > > + located at the DC register space. They are used to emulate two way i2c,
> > > > > > > > > > > + One for DVO0, another for DVO1.
> > > > > > > > > > > +
> > > > > > > > > > > + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either
> > > > > > > > > > > + general purpose GPIO emulated i2c or hardware i2c in the SoC.
> > > > > > > > > > > +
> > > > > > > > > > > + LSDC's display pipeline have several components as below description,
> > > > > > > > > > > +
> > > > > > > > > > > + The display controller in LS7A1000:
> > > > > > > > > > > + ___________________ _________
> > > > > > > > > > > + | -------| | |
> > > > > > > > > > > + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> > > > > > > > > > > + | _ _ -------| ^ ^ |_________|
> > > > > > > > > > > + | | | | | -------| | |
> > > > > > > > > > > + | |_| |_| | i2c0 <--------+-------------+
> > > > > > > > > > > + | -------|
> > > > > > > > > > > + | DC IN LS7A1000 |
> > > > > > > > > > > + | _ _ -------|
> > > > > > > > > > > + | | | | | | i2c1 <--------+-------------+
> > > > > > > > > > > + | |_| |_| -------| | | _________
> > > > > > > > > > > + | -------| | | | |
> > > > > > > > > > > + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> > > > > > > > > > > + | -------| |_________|
> > > > > > > > > > > + |___________________|
> > > > > > > > > > > +
> > > > > > > > > > > + Simple usage of LS7A1000 with LS3A4000 CPU:
> > > > > > > > > > > +
> > > > > > > > > > > + +------+ +-----------------------------------+
> > > > > > > > > > > + | DDR4 | | +-------------------+ |
> > > > > > > > > > > + +------+ | | PCIe Root complex | LS7A1000 |
> > > > > > > > > > > + || MC0 | +--++---------++----+ |
> > > > > > > > > > > + +----------+ HT 3.0 | || || |
> > > > > > > > > > > + | LS3A4000 |<-------->| +---++---+ +--++--+ +---------+ +------+
> > > > > > > > > > > + | CPU |<-------->| | GC1000 | | LSDC |<-->| DDR3 MC |<->| VRAM |
> > > > > > > > > > > + +----------+ | +--------+ +-+--+-+ +---------+ +------+
> > > > > > > > > > > + || MC1 +---------------|--|----------------+
> > > > > > > > > > > + +------+ | |
> > > > > > > > > > > + | DDR4 | +-------+ DVO0 | | DVO1 +------+
> > > > > > > > > > > + +------+ VGA <--|ADV7125|<--------+ +-------->|TFP410|--> DVI/HDMI
> > > > > > > > > > > + +-------+ +------+
> > > > > > > > > > > +
> > > > > > > > > > > + The display controller in LS2K1000/LS2K0500:
> > > > > > > > > > > + ___________________ _________
> > > > > > > > > > > + | -------| | |
> > > > > > > > > > > + | CRTC0 --> | DVO0 ----> Encoder0 ---> Connector0 ---> | Monitor |
> > > > > > > > > > > + | _ _ -------| ^ ^ |_________|
> > > > > > > > > > > + | | | | | | | |
> > > > > > > > > > > + | |_| |_| | +------+ |
> > > > > > > > > > > + | <---->| i2c0 |<---------+
> > > > > > > > > > > + | DC IN LS2K1000 | +------+
> > > > > > > > > > > + | _ _ | +------+
> > > > > > > > > > > + | | | | | <---->| i2c1 |----------+
> > > > > > > > > > > + | |_| |_| | +------+ | _________
> > > > > > > > > > > + | -------| | | | |
> > > > > > > > > > > + | CRTC1 --> | DVO1 ----> Encoder1 ---> Connector1 ---> | Panel |
> > > > > > > > > > > + | -------| |_________|
> > > > > > > > > > > + |___________________|
> > > > > > > > > > > +
> > > > > > > > > > > +properties:
> > > > > > > > > > > + $nodename:
> > > > > > > > > > > + pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
> > > > > > > > > > > +
> > > > > > > > > > > + compatible:
> > > > > > > > > > > + oneOf:
> > > > > > > > > > > + - items:
> > > > > > > > > > > + - enum:
> > > > > > > > > > > + - loongson,ls7a1000-dc
> > > > > > > > > > > + - loongson,ls2k1000-dc
> > > > > > > > > > > + - loongson,ls2k0500-dc
> > > > > > > > > > > +
> > > > > > > > > > > + reg:
> > > > > > > > > > > + maxItems: 1
> > > > > > > > > > > +
> > > > > > > > > > > + interrupts:
> > > > > > > > > > > + maxItems: 1
> > > > > > > > > > > +
> > > > > > > > > > > + '#address-cells':
> > > > > > > > > > > + const: 1
> > > > > > > > > > > +
> > > > > > > > > > > + '#size-cells':
> > > > > > > > > > > + const: 0
> > > > > > > > > > > +
> > > > > > > > > > > + i2c-gpio@0:
> > > > > > > > > > > + description: |
> > > > > > > > > > > + Built-in GPIO emulate i2c exported for external display bridge
> > > > > > > > > > If you have i2c-gpio, that belongs at the DT top-level, not here.
> > > > > > > > > >
> > > > > > > > > > > + configuration, onitor detection and edid read back etc, for ls7a1000
> > > > > > > > > > > + only. Its compatible must be lsdc,i2c-gpio-0. The reg property can be
> > > > > > > > > > No, there's a defined i2c-gpio compatible already.
> > > > > > > > > This is different from the i2c-gpio already defined under drivers/i2c/busses/i2c-gpio.c,
> > > > > > > > > By design, my i2c-gpio is vendor specific properties, lsdc device driver create the i2c
> > > > > > > > > adapter at runtime. These are 4 dedicated GPIOs whose control register is located at the
> > > > > > > > > LSDC register space, not general purpose GPIOs with separate control register resource.
> > > > > > > > > So i think it is the child node of?display-controller@6,1,?it belongs to LSDC.
> > > > > > > > > It seems that put it at the DT top-level break the hierarchy and relationship.
> > > > > > > > Okay, I see. Then just 'i2c' for the node names. You need a reference to
> > > > > > > > i2c-controller.yaml for these nodes too.
> > > > > > > >
> > > > > > > > The compatible should not have an index in it.
> > > > > > > OK, i will fix this at the next version. thanks.
> > > > > > > > > > > + used to specify a I2c adapter bus number, if you don't specify one
> > > > > > > > > > > + i2c driver core will dynamically assign a bus number. Please specify
> > > > > > > > > > Bus numbers are a linux detail not relevant to DT binding.
> > > > > > > > > >
> > > > > > > > > > > + it only when its bus number matters. Bus number greater than 6 is safe
> > > > > > > > > > > + because ls7a1000 bridge have 6 hardware I2C controller integrated.
> > > > > > > > > > > +
> > > > > > > > > > > + i2c-gpio@1:
> > > > > > > > > > > + description: |
> > > > > > > > > > > + Built-in GPIO emulate i2c exported for external display bridge
> > > > > > > > > > > + configuration, onitor detection and edid read back etc, for ls7a1000
> > > > > > > > > > > + only. Its compatible must be lsdc,i2c-gpio-1.
> > > > > > > > > > > +
> > > > > > > > > > > + ports:
> > > > > > > > > > > + $ref: /schemas/graph.yaml#/properties/ports
> > > > > > > > > > > +
> > > > > > > > > > > + properties:
> > > > > > > > > > > + port@0:
> > > > > > > > > > > + $ref: /schemas/graph.yaml#/properties/port
> > > > > > > > > > > + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> > > > > > > > > > > +
> > > > > > > > > > > + port@1:
> > > > > > > > > > > + $ref: /schemas/graph.yaml#/properties/port
> > > > > > > > > > > + description: output port node connected with DPI panels or external encoders, with only one endpoint.
> > > > > > > > > > > +
> > > > > > > > > > > + required:
> > > > > > > > > > > + - port@0
> > > > > > > > > > > + - port@1
> > > > > > > > > > > +
> > > > > > > > > > > +required:
> > > > > > > > > > > + - compatible
> > > > > > > > > > > + - reg
> > > > > > > > > > > + - interrupts
> > > > > > > > > > > + - ports
> > > > > > > > > > > +
> > > > > > > > > > > +additionalProperties: false
> > > > > > > > > > > +
> > > > > > > > > > > +examples:
> > > > > > > > > > > + - |
> > > > > > > > > > > + #include <dt-bindings/interrupt-controller/irq.h>
> > > > > > > > > > > + bus {
> > > > > > > > > > > +
> > > > > > > > > > > + #address-cells = <3>;
> > > > > > > > > > > + #size-cells = <2>;
> > > > > > > > > > > + #interrupt-cells = <2>;
> > > > > > > > > > > +
> > > > > > > > > > > + display-controller@6,1 {
> > > > > > > > > > > + compatible = "loongson,ls7a1000-dc";
> > > > > > > > > > > + reg = <0x3100 0x0 0x0 0x0 0x0>;
> > > > > > > > > > > + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > > > > > +
> > > > > > > > > > > + #address-cells = <1>;
> > > > > > > > > > > + #size-cells = <0>;
> > > > > > > > > > > +
> > > > > > > > > > > + i2c-gpio@0 {
> > > > > > > > > > > + compatible = "lsdc,i2c-gpio-0";
> > > > > > > > > > > + reg = <6>;
> > > > > > > > 'reg' needs to be documented with some description of what 6 and 7
> > > > > > > > represent. If they are the control register offset, then make the
> > > > > > > > address translatable (use 'ranges' and define the size).
> > > > > > > By design, the reg property is used to specify a I2c adapter bus number,
> > > > > > > if we don't specify one, i2c driver core will dynamically assign a bus number.
> > > > > > > then the nr of the i2c adapter will started from 0. I want is start from 6
> > > > > > > to avoid potential conflict feature hardware I2C driver.
> > > > > > >
> > > > > > > Because LS7A1000 bridge chip have 6 hardware I2C controller integrated,
> > > > > > > but its driver is not up-streamed yet. By default these hardware I2C controller's
> > > > > > > nr is started from 0.
> > > > > > Linux's numbering doesn't belong in DT. So no, you can't use 'reg' in
> > > > > > that way.
> > > > > Then,? can i use something like lsdc,nr = <6> ?
> > > > > > > Even through i2c driver core can dynamically generate a number, i still want it
> > > > > > > to be fixed and keep consistent and explicit. That is, i2c6 is for display pipe 0,
> > > > > > > i2c7 is for display pipe 1. This follow the convention and flexible enough.
> > > > > > You may want that, but that is not how the kernel works. Specific
> > > > > > numbers are not guaranteed. I'm sure you've seen this for disks, network
> > > > > > interfaces, etc.
> > > > > >
> > > > > > Rob
> > > > > 2c_bit_add_numbered_bus() will guarantee it for you as long as If no devices
> > > > > have pre-been declared for this bus.
> > > > >
> > > > > you can read the comment of 2c_bit_add_numbered_bus() at
> > > > > drivers/i2c/i2c-core-base.c
> > > > I didn't say it wasn't possible. It is not best practice. Grep
> > > > i2c_bit_add_numbered_bus and see how many users there are.
> > > i2c-gpio.c at drivers/i2c/busses/ just do the same thing.
> > No, the id for i2c-gpio nodes (any DT node without 'reg') will be -1
> > which means dynamically assigned.
> >
> >
> > > + nvidia,bpmp-bus-id: + $ref: /schemas/types.yaml#/definitions/uint32 +
> > > description: Indicates the I2C bus number this DT node represents, + as
> > > defined by the BPMP firmware.
> > The key difference is the numbering is defined by the BPMP firmware.
> >
> Bus numbers are a linux detail, I am not sure it is relevant to BPMP firmware.
> and BPMP firmware is not relevant here.
>

Read the review of it[1]. The key part is this:

> +The BPMP firmware defines no single global name-/numbering-space for such
> +services. Put another way, the numbering scheme for I2C buses is distinct from
> +the numbering scheme for any other service the BPMP may provide (e.g. a future
> +hypothetical SPI bus service). As such, child device nodes will have no reg
> +property, and the BPMP node will have no #address-cells or #size-cells property.

> My understanding is that the I2C bus number is passed as part of the
> request to the BPMP firmware. Does that not count as addressing? Could
> we not represent that generically using a device tree hierarchy? I'm
> thinking something along these lines:


The bus numbers are NOT defined by Linux nor defined in the DT.

> The point is you applied that patch, you are admit the fact that bus numbers
> could be in DT.

That actually is not an argument for your patch. There are lots of
things we accepted in the past that now will be rejected. This case
however is not something that changed. You may still find examples as
bindings didn't always get reviewed (very well).


> > > > Even if the kernel allows specifying bus numbers,your Linux bus numbers don't
> > > > belong in DT.
> > > Again, Does does devicetree specification prohibit this?
> > No. The spec is not the last word on what's allowed or not. Lots of
> > patterns exist already which we can't change, but that doesn't mean they
> > should be copied by new users.
> >
> > Rob
>
> We develop that part by our own, only find that there are someone do the
> same thing when it got push back.
>
> We believe that it is very flexible actually, anyway if you still don't
> change you mind we can rewrite it.
>
> I have resend my patches,? see it at
> https://patchwork.freedesktop.org/series/101843/

Don't send new versions when the discussion on the prior one is ongoing.
Though in this case there is nothing more to discuss.

Rob

[1] https://lore.kernel.org/all/[email protected]/