Re: [DPU PATCH v5 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon

Thanks for reviews Stephen. Please find my comments according to new design.

On 2020-04-23 16:41, Stephen Boyd wrote:

Quoting Tanmay Shah (2020-03-31 17:30:27)

...

diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml new file mode 100644 index 0000000..761a01d --- /dev/null +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml @@ -0,0 +1,325 @@ +# SPDX-License-Identifier: GPL-2.0-only +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: Description of Qualcomm Display Port dt properties.

This title should be something like

"Qualcomm Display Port Controller"

Changed title as suggested.

...

+maintainers:

• • Chandan Uddaraju chandanu@codeaurora.org
• • Vara Reddy varar@codeaurora.org

+description: |

• Device tree bindings for MSM Display Port which supports DP host

controllers

• that are compatible with VESA Display Port interface specification.

+properties:

• "msm_dp":
• type: object
• description: |

•  Node containing Display port register address bases, clocks, 
  

power supplies.

• properties:

• compatible:
  

•   items:
  

•     - const: qcom,dp-display
  

• cell-index:
  

•   description: Specifies the controller instance.
  

• reg:
  

•   description: Physical base address and length of controller's
  

registers.

• reg-names:
  

•   description: |
  

•     Names for different register regions defined above. The 
  

required region

•     is mentioned below.
  

•   items:
  

•     - const: dp_ahb
  

•     - const: dp_aux
  

•     - const: dp_link
  

•     - const: dp_p0
  

•     - const: dp_phy
  

•     - const: dp_ln_tx0
  

•     - const: dp_ln_tx1
  

•     - const: afprom_physical
  

•     - const: dp_pll
  

•     - const: usb3_dp_com
  

•     - const: hdcp_physical
  

• interrupts:
  

•   description: The interrupt signal from the DP block.
  

• clocks:
  

•   description: List of clock specifiers for clocks needed by the
  

device.

• clock-names:
  

•   description: |
  

•     Device clock names in the same order as mentioned in clocks
  

property.

•     The required clocks are mentioned below.
  

•   items:
  

•     - const: core_aux_clk
  

•     - const: core_ref_clk_src
  

•     - const: core_usb_ref_clk
  

•     - const: core_usb_cfg_ahb_clk
  

•     - const: core_usb_pipe_clk
  

•     - const: ctrl_link_clk
  

•     - const: ctrl_link_iface_clk
  

•     - const: ctrl_pixel_clk
  

•     - const: crypto_clk
  

•     - const: pixel_clk_rcg
  

• pll-node:
  

•   description: phandle to DP PLL node.
  

• vdda-1p2-supply:
  

•   description: phandle to vdda 1.2V regulator node.
  

• vdda-0p9-supply:
  

•   description: phandle to vdda 0.9V regulator node.
  

• aux-cfg0-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 0 settings.
  

•     The first entry in this array corresponds to the register 
  

offset

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg1-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 1 settings.
  

•     The first entry in this array corresponds to the register 
  

offset

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg2-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 2 settings.
  

•     The first entry in this array corresponds to the register 
  

offset

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg3-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 3 settings.
  

•     The first entry in this array corresponds to the register 
  

offset

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg4-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 4 settings.
  

•     The first entry in this array corresponds to the register 
  

offset

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg5-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 5 settings.
  

•     The first entry in this array corresponds to the register 
  

offset

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg6-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 6 settings.
  

•     The first entry in this array corresponds to the register 
  

offset

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg7-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 7 settings.
  

•     The first entry in this array corresponds to the register 
  

offset

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg8-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 8 settings.
  

•     The first entry in this array corresponds to the register 
  

offset

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg9-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 9 settings.
  

•     The first entry in this array corresponds to the register 
  

offset

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• max-pclk-frequency-khz:
  

•   description: Maximum displayport pixel clock supported for the
  

chipset.

• data-lanes:
  

•   description: Maximum number of lanes that can be used for 
  

Display port.

This should be an array of cells, not a single cell indicating the number of lanes.

Done. Now data-lanes is array of integers and size of array represents maximum number of lanes supported.

...

• usbplug-cc-gpio:
  

•   maxItems: 1
  

•   description: Specifies the usbplug orientation gpio.
  

• aux-en-gpio:
  

•   maxItems: 1
  

•   description: Specifies the aux-channel enable gpio.
  

• aux-sel-gpio:
  

•   maxItems: 1
  

•   description: Specifies the sux-channel select gpio.
  

• ports:
  

•   description: |
  

•     Contains display port controller endpoint subnode.
  

•     remote-endpoint: |
  

•       For port@0, set to phandle of the connected panel/bridge's
  

•       input endpoint. For port@1, set to the DPU interface 
  

output.

•       Documentation/devicetree/bindings/graph.txt and
  

Documentation/devicetree/bindings/media/video-interfaces.txt.

• "dp_pll":

• type: object
  

• description: Node contains properties of Display port pll and 
  

phy driver.

• properties:
  

•   compatible:
  

•     items:
  

•       - const: qcom,dp-pll-10nm
  

•   cell-index:
  

•     description: Specifies the controller instance.
  

•   reg:
  

•     description: Physical base address and length of DP phy and 
  

pll registers.

•   reg-names:
  

•     description: |
  

•       Names for different register regions defined above. The
  

required region

•       is mentioned below.
  

•     items:
  

•       - const: pll_base
  

•       - const: phy_base
  

•       - const: ln_tx0_base
  

•       - const: ln_tx1_base
  

•       - const: gdsc_base
  

•   clocks:
  

•     description: List of clock specifiers for clocks needed by 
  

the device.

•   clock-names:
  

•     description: |
  

•       Device clock names in the same order as mentioned in 
  

clocks property.

•       The required clocks are mentioned below.
  

•     items:
  

•       - const: iface
  

•       - const: ref
  

•       - const: cfg_ahb
  

•       - const: pipe
  

+examples:

• • |
• #include <dt-bindings/interrupt-controller/arm-gic.h>
• #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
• #include <dt-bindings/clock/qcom,gcc-sdm845.h>
• #include <dt-bindings/clock/qcom,rpmh.h>
• #include <dt-bindings/gpio/gpio.h>
• msm_dp: displayport-controller@ae90000{

•            cell-index = <0>;
  

•    compatible = "qcom,dp-display";
  

•    reg =   <0 0xae90000 0 0x200>,
  

•            <0 0xae90200 0 0x200>,
  

•            <0 0xae90400 0 0xc00>,
  

•            <0 0xae91000 0 0x400>,
  

•            <0 0x88eaa00 0 0x200>,
  

•            <0 0x88ea200 0 0x200>,
  

•            <0 0x88ea600 0 0x200>,
  

•            <0 0x780000 0 0x6228>,
  

•            <0 0x088ea000 0 0x40>,
  

•            <0 0x88e8000 0 0x20>,
  

•            <0 0x0aee1000 0 0x034>;
  

This needs to be split up into at least two nodes. Any address above that starts in 88e needs to be put into a new node underneath the qmp phy. That is the "DP PHY" that lives in the power domain of the USB+DP combo phy. The qfprom_physical reg property should be removed from here and this binding should use the nvmem binding to reach into the qfprom to read out things (I guess there's some sort of HDCP key in the qfprom?).

After that I don't know why there are so many different reg properties for the DP controller here and why it needs to be split up. It looks like we should just map the register space from 0xae90000 up to 0xae91400 as one big register region and have the driver figure out how to operate on top of that. If it changes between SoC versions then we should have a more specific compatible that tells us what registers are in what place.

Done. Only one register region is specified here now in new bindings i.e. dp_controller starting from 0xae90000 upto 0xae91400. Removed rest of the module offsets. Driver will access each module using offset as required. Also PHY and USB3 DPCOM register bases are hard-coded in driver. Removed redundant qfprom and hdcp registers from bindings.

...

•    reg-names = "dp_ahb", "dp_aux", "dp_link",
  

•        "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
  

•        "qfprom_physical", "dp_pll",
  

•        "usb3_dp_com", "hdcp_physical";
  

•    interrupt-parent = <&display_subsystem>;
  

•    interrupts = <12 0>;
  

•    clocks = <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
  

•        <&rpmhcc RPMH_CXO_CLK>,
  

•        <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
  

•        <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
  

•        <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>,
  

•        <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
  

•        <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
  

•        <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>,
  

•        <&dispcc DISP_CC_MDSS_DP_CRYPTO_CLK>,
  

•        <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
  

•    clock-names = "core_aux_clk", "core_ref_clk_src",
  

•        "core_usb_ref_clk", "core_usb_cfg_ahb_clk",
  

•        "core_usb_pipe_clk", "ctrl_link_clk",
  

•        "ctrl_link_iface_clk", "ctrl_pixel_clk",
  

•        "crypto_clk", "pixel_clk_rcg";
  

•    pll-node = <&dp_pll>;
  

If the DP PLL and DP controller need to be controlled from two software entities, it may make sense to just combine that DP PLL into the controller node and have this node be a clk provider. The pll-node property is pretty ugly and should be removed.

Done. Removed PLL as separate node and combined PLL as module of DP driver. Removed pll-node property as well.

...

•    vdda-1p2-supply = <&vreg_l3c_1p2>;
  

•    vdda-0p9-supply = <&vreg_l4a_0p8>;
  

•    aux-cfg0-settings = [20 00];
  

•    aux-cfg1-settings = [24 13 23 1d];
  

•    aux-cfg2-settings = [28 24];
  

•    aux-cfg3-settings = [2c 00];
  

•    aux-cfg4-settings = [30 0a];
  

•    aux-cfg5-settings = [34 26];
  

•    aux-cfg6-settings = [38 0a];
  

•    aux-cfg7-settings = [3c 03];
  

•    aux-cfg8-settings = [40 bb];
  

•    aux-cfg9-settings = [44 03];
  

This pile of properties is board specific configuration tuning or something? Can this go into the driver? Or can it be made more human readable? I seem to recall that the USB phy had similar properties and we made them into human readable properties when board integrators needed to change them. The easiest approach there is to put everything in the driver for now and then when something has to change for a board it gets punted out to the DT and that overrides the "default" settings that are used in the driver.

Made aux-cfg[0-9]-settingsproperties optional in bindings. Added default configurations in driver and using them if these properties are not mentioned in dts.

...

•    max-pclk-frequency-khz = <67500>;
  

What is this? Why isn't this in the driver?

Done. Removed this property from bindings and setting default value in driver.

...

•    data-lanes = <2>;
  

•    aux-en-gpio = <&msmgpio 55 1>;
  

•    aux-sel-gpio = <&msmgpio 110 1>;
  

•    usbplug-cc-gpio = <&msmgpio 90 1>;
  

•    ports {
  

•        #address-cells = <1>;
  

•        #size-cells = <0>;
  

•        port@0 {
  

•            reg = <0>;
  

•            dp_in: endpoint {
  

•                remote-endpoint = <&dpu_intf0_out>;
  

•            };
  

•        };
  

•        port@1 {
  

•            reg = <1>;
  

•            dp_out: endpoint {
  

•            };
  

•        };
  

•    };
  

• };
• dp_pll: dp-pll@088ea000 {

•    compatible = "qcom,dp-pll-10nm";
  

•    label = "DP PLL";
  

•    cell-index = <0>;
  

•    #clock-cells = <1>;
  

•    reg = <0 0x088ea000 0 0x200>,
  

•          <0 0x088eaa00 0 0x200>,
  

•          <0 0x088ea200 0 0x200>,
  

•          <0 0x088ea600 0 0x200>,
  

•          <0 0x08803000 0 0x8>;
  

•    reg-names = "pll_base", "phy_base", "ln_tx0_base",
  

•        "ln_tx1_base", "gdsc_base";
  

I guess the DP_PLL lives inside the qmp combo phy? That would match how the USB phy binding has been done there. This whole node should be combined with the DP phy node that will be placed as a child of the qmp phy wrapper (i.e. qcom,sc7180-qmp-usb3-phy compatible node). Might as well change that compatible from qcom,sc7180-qmp-usb3-phy to be qcom,sc7180-qmp-usb3-dp-phy too so that it can create the DP phy bits too.

Done. Removed whole dp_pll node from here and added PLL as module of DP driver. This required hard coding of few register bases in driver for now.

...

•    clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
  

•         <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
  

•         <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
  

•         <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
  

•    clock-names = "iface_clk", "ref_clk",
  

•        "cfg_ahb_clk", "pipe_clk";
  

• };

diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt index 551ae26..7e99e45 100644 --- a/Documentation/devicetree/bindings/display/msm/dpu.txt +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt @@ -63,8 +63,9 @@ Required properties: Documentation/devicetree/bindings/graph.txt Documentation/devicetree/bindings/media/video-interfaces.txt

•   Port 0 -> DPU_INTF1 (DSI1)
  

•   Port 1 -> DPU_INTF2 (DSI2)
  

•   Port 0 -> DPU_INTF0 (DP)
  

•   Port 1 -> DPU_INTF1 (DSI1)
  

•   Port 2 -> DPU_INTF2 (DSI2)
  

DP should come last so that the port mapping doesn't have to change.

Done. Reverted Port mapping to old one i.e. Port0->DSI1, Port1->DSI2 and Added Port2->DP mapping.

...

Optional properties:

• assigned-clocks: list of clock specifiers for clocks needing rate

assignment