[Freedreno] [DPU PATCH 1/3] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon 845
Stephen Boyd
swboyd at chromium.org
Tue Nov 6 23:45:34 UTC 2018
Quoting Chandan Uddaraju (2018-10-10 10:15:57)
> diff --git a/Documentation/devicetree/bindings/display/msm/dp.txt b/Documentation/devicetree/bindings/display/msm/dp.txt
> new file mode 100644
> index 0000000..0155266
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dp.txt
> @@ -0,0 +1,249 @@
> +Qualcomm Technologies, Inc.
> +DP is the master Display Port device which supports DP host controllers that are compatible with VESA Display Port interface specification.
> +DP Controller: Required properties:
> +- compatible: Should be "qcom,dp-display".
> +- reg: Base address and length of DP hardware's memory mapped regions.
> +- cell-index: Specifies the controller instance.
> +- reg-names: A list of strings that name the list of regs.
This is a lot of register regions! There are probably multiple devices
in here.
> + "dp_ahb" - DP controller memory region.
> + "dp_aux" - DP AUX memory region.
> + "dp_link" - DP link layer memory region.
> + "dp_p0" - DP pixel clock domain memory region.
> + "dp_phy" - DP PHY memory region.
So a phy driver? Should be different binding and device than the DP
controller?
> + "dp_ln_tx0" - USB3 DP PHY combo TX-0 lane memory region.
> + "dp_ln_tx1" - USB3 DP PHY combo TX-1 lane memory region.
Presumably part of the phy device. Again, different binding?
> + "dp_mmss_cc" - Display Clock Control memory region.
No. This shouldn't be reaching into the display clock controller region.
That should be it's own node and device driver.
> + "qfprom_physical" - QFPROM Phys memory region.
qfprom is accessed through nvmem framework, please use it instead of
remapping it and reading random bits directly from your driver.
> + "dp_pll" - USB3 DP combo PLL memory region.
> + "usb3_dp_com" - USB3 DP PHY combo memory region.
I guess this would go with the DP phy node?
> + "hdcp_physical" - DP HDCP memory region.
Goes with the main DP device?
Please drop the full stop from all of these lines.
> +- interrupt-parent phandle to the interrupt parent device node.
We don't need this sort of duplicate information. If it's part of
standard spec and not specific to this binding it can be left out.
> +- interrupts: The interrupt signal from the DP block.
> +- clocks: Clocks required for Display Port operation. See [1] for details on clock bindings.
> +- clock-names: Names of the clocks corresponding to handles. Following clocks are required:
> + "core_aux_clk", "core_usb_ref_clk_src","core_usb_ref_clk", "core_usb_cfg_ahb_clk",
> + "core_usb_pipe_clk", "ctrl_link_clk", "ctrl_link_iface_clk", "ctrl_crypto_clk",
> + "ctrl_pixel_clk", "pixel_clk_rcg", "pixel_parent".
Please remove "_clk" from all your clock-names. It's redundant and makes
things harder to read.
> +- pll-node: phandle to DP PLL node.
Huh?
> +- vdda-1p2-supply: phandle to vdda 1.2V regulator node.
> +- vdda-0p9-supply: phandle to vdda 0.9V regulator node.
Perhaps these have other names besides vdda? Maybe vdda-pll and vdda?
I'd also drop the 1p2 and 0p9 from them and just describe that in
freeform in the binding.
> +- qcom,aux-cfg0-settings: 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.
> +- qcom,aux-cfg1-settings: 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.
> +- qcom,aux-cfg2-settings: 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.
> +- qcom,aux-cfg3-settings: 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.
> +- qcom,aux-cfg4-settings: 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.
> +- qcom,aux-cfg5-settings: 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.
> +- qcom,aux-cfg6-settings: 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.
> +- qcom,aux-cfg7-settings: 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.
> +- qcom,aux-cfg8-settings: 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.
> +- qcom,aux-cfg9-settings: 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.
Look like magic bits. Can they just be thrown into the driver? Or
specified in some way that us mere mortals know what to put into these
bits.
> +- qcom,max-pclk-frequency-khz: An integer specifying the maximum. pixel clock in KHz supported by Display Port.
Why is this in the binding? It isn't known based on the device
compatible string?
> +- extcon: Phandle for the external connector class interface.
Is this for typec? Would be good to say what it's for.
> +- qcom,<type>-supply-entries: A node that lists the elements of the supply used by the a particular "type" of DP module. The module "types"
> + can be "core", "ctrl", and "phy". Within the same type,
> + there can be more than one instance of this binding,
> + in which case the entry would be appended with the
> + supply entry index.
> + e.g. qcom,ctrl-supply-entry at 0
> + -- qcom,supply-name: name of the supply (vdd/vdda/vddio)
> + -- qcom,supply-min-voltage: minimum voltage level (uV)
> + -- qcom,supply-max-voltage: maximum voltage level (uV)
> + -- qcom,supply-enable-load: load drawn (uA) from enabled supply
> + -- qcom,supply-disable-load: load drawn (uA) from disabled supply
> + -- qcom,supply-pre-on-sleep: time to sleep (ms) before turning on
> + -- qcom,supply-post-on-sleep: time to sleep (ms) after turning on
> + -- qcom,supply-pre-off-sleep: time to sleep (ms) before turning off
> + -- qcom,supply-post-off-sleep: time to sleep (ms) after turning off
All these properties should go away. min/max should be handled by board
constraints. Sleep times should be handled by regulator framework for
those particular regulators. Maybe the load would be important, but I'm
still sort of lost how that wouldn't be expressed through some SoC
compatible string for the device and then hardcoded into the driver.
> +- pinctrl-names: List of names to assign mdss pin states defined in pinctrl device node
> + Refer to pinctrl-bindings.txt
> +- pinctrl-<0..n>: Lists phandles each pointing to the pin configuration node within a pin
> + controller. These pin configurations are installed in the pinctrl
> + device node. Refer to pinctrl-bindings.txt
This isn't needed in the binding. Or at least, I don't see where it's
expressed in the example, so probably just noise?
> +DP Endpoint properties:
> + - remote-endpoint: For port at 0, set to phandle of the connected panel/bridge's
> + input endpoint. For port at 1, set to the DPU interface output. See [2] for
> + device graph info.
> +
> +Optional properties:
> +- qcom,aux-en-gpio: Specifies the aux-channel enable gpio.
> +- qcom,aux-sel-gpio: Specifies the aux-channel select gpio.
These should follow standard 'gpios' properties, like aux-enable-gpios
and aux-select-gpios. I guess these would need pinctrl muxing too.
> +
> +
> +DP PLL: Required properties:
> +- compatible: Should be "qcom,dp-pll-10nm".
> +- reg: Base address and length of DP hardware's memory mapped regions.
> +- cell-index: Specifies the PLL instance.
> +- reg-names: A list of strings that name the list of regs.
> + "pll_base" - DP PLL memory region.
> + "phy_base" - DP PHY memory region.
> + "ln_tx0_base" - USB3 DP PHY combo TX-0 lane memory region.
> + "ln_tx1_base" - USB3 DP PHY combo TX-1 lane memory region.
> + "gdsc_base" - gdsc memory region.
Why a gdsc_base? GDSCs should be handled by power domains?
> +- interrupt-parent phandle to the interrupt parent device node.
Again, drop this one.
> +- interrupts: The interrupt signal from the DP block.
> +- clocks: Clocks required for Display Port operation. See [1] for details on clock bindings.
> +- clock-names: Names of the clocks corresponding to handles. Following clocks are required:
> + "iface_clk", "ref_clk", cfg_ahb_clk", "pipe_clk".
Again, drop "_clk" please.
> +- clock-rate: Initial clock rate to be configured. For the shared clocks, the default value is set to zero so that minimum clock value is configured. Non-zero clock
> + value can be used to configure DP pixel clock.
We have assigned-clock-rates for this. Can you use that?
> +
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +[2] Documentation/devicetree/bindings/graph.txt
> +
> +Example:
> + msm_dp: dp_display at ae90000{
> + cell-index = <0>;
Is this needed?
> + compatible = "qcom,dp-display";
> +
> + reg = <0 0x90000 0x0dc>,
> + <0 0x90200 0x0c0>,
> + <0 0x90400 0x508>,
> + <0 0x90a00 0x094>,
Looks like one device with one reg property:
<0x90000 0x1000>
> + <1 0xeaa00 0x200>,
> + <1 0xea200 0x200>,
> + <1 0xea600 0x200>,
Looks like another device with one reg property:
<0xea000 0x1000>;
> + <2 0x02000 0x1a0>,
> + <3 0x00000 0x621c>,
> + <1 0xea000 0x180>,
This is part of the one above.
> + <1 0xe8000 0x20>,
Well maybe this is also part of the DP phy? Then it would start
somewhere a little earlier I supposed.
> + <4 0xe1000 0x034>;
> + reg-names = "dp_ahb", "dp_aux", "dp_link",
> + "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
> + "dp_mmss_cc", "qfprom_physical", "dp_pll",
> + "usb3_dp_com", "hdcp_physical";
> +
> + interrupt-parent = <&mdss>;
> + interrupts = <12 0>;
Why is this interrupt-parent stuff being done?
> +
> + extcon = <&usb_1_ssphy>;
> + 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>,
It's really odd that this device needs USB phy clocks. Is it really
necessary to do this?
> + <&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>;
> + qcom,aux-cfg0-settings = [20 00];
> + qcom,aux-cfg1-settings = [24 13 23 1d];
> + qcom,aux-cfg2-settings = [28 24];
> + qcom,aux-cfg3-settings = [2c 00];
> + qcom,aux-cfg4-settings = [30 0a];
> + qcom,aux-cfg5-settings = [34 26];
> + qcom,aux-cfg6-settings = [38 0a];
> + qcom,aux-cfg7-settings = [3c 03];
> + qcom,aux-cfg8-settings = [40 bb];
> + qcom,aux-cfg9-settings = [44 03];
> +
> + qcom,max-pclk-frequency-khz = <675000>;
> +
> + qcom,ctrl-supply-entries {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + qcom,ctrl-supply-entry at 0 {
> + reg = <0>;
> + qcom,supply-name = "vdda-1p2";
> + qcom,supply-min-voltage = <1200000>;
> + qcom,supply-max-voltage = <1200000>;
> + qcom,supply-enable-load = <21800>;
> + qcom,supply-disable-load = <4>;
> + };
> + };
> +
> + qcom,phy-supply-entries {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + qcom,phy-supply-entry at 0 {
> + reg = <0>;
> + qcom,supply-name = "vdda-0p9";
> + qcom,supply-min-voltage = <880000>;
> + qcom,supply-max-voltage = <880000>;
> + qcom,supply-enable-load = <36000>;
> + qcom,supply-disable-load = <32>;
> + };
> + };
NAK on the above two nodes.
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port at 0 {
> + reg = <0>;
> + dp_in: endpoint {
> + remote-endpoint = <&dpu_intf0_out>;
> + };
> + };
> +
> + port at 1 {
> + reg = <1>;
> + dp_out: endpoint {
> + };
> + };
> + };
> + };
> +
> + dp_pll: dp-pll at c011000 {
This doesn't even match reg property.
> + compatible = "qcom,dp-pll-10nm";
> + label = "DP PLL";
> + cell-index = <0>;
> + #clock-cells = <1>;
> +
> + reg = <1 0xea000 0x200>,
> + <1 0xeaa00 0x200>,
> + <1 0xea200 0x200>,
> + <1 0xea600 0x200>,
> + <2 0x03000 0x8>;
Why 1 and 2? What's going on in this example.
> + reg-names = "pll_base", "phy_base", "ln_tx0_base",
> + "ln_tx1_base", "gdsc_base";
Don't think we really need "_base" on these reg-names either. Seems
redundant.
> +
> + 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";
> + clock-rate = <0>;
> +
> + };
More information about the Freedreno
mailing list