On 24 February 2016 at 02:37, Mark Rutland <mark.rutland@xxxxxxx> wrote: Hi Mark, thanks for review. > On Tue, Feb 23, 2016 at 11:00:21AM +0800, Xinliang Liu wrote: >> Add ADE display controller binding doc. >> Add DesignWare DSI Host Controller v1.20a binding doc. >> >> v5: >> - Remove endpoint unit address of dsi output port. >> - Add "hisilicon,noc-syscon" property for ADE NOC QoS syscon. >> - Add "resets" property for ADE reset. >> v4: >> - Describe more specific of clocks and ports. >> - Fix indentation. >> v3: >> - Make ade as the drm master node. >> - Use assigned-clocks to set clock rate. >> - Use ports to connect display relavant nodes. >> v2: >> - Move dt binding docs to bindings/display/hisilicon directory. >> >> Signed-off-by: Xinwei Kong <kong.kongxinwei@xxxxxxxxxxxxx> >> Signed-off-by: Xinliang Liu <xinliang.liu@xxxxxxxxxx> >> --- >> .../bindings/display/hisilicon/dw-dsi.txt | 72 ++++++++++++++++++++++ >> .../bindings/display/hisilicon/hisi-ade.txt | 64 +++++++++++++++++++ >> 2 files changed, 136 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/display/hisilicon/dw-dsi.txt >> create mode 100644 Documentation/devicetree/bindings/display/hisilicon/hisi-ade.txt >> >> diff --git a/Documentation/devicetree/bindings/display/hisilicon/dw-dsi.txt b/Documentation/devicetree/bindings/display/hisilicon/dw-dsi.txt >> new file mode 100644 >> index 000000000000..0d234b5e19af >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/hisilicon/dw-dsi.txt >> @@ -0,0 +1,72 @@ >> +Device-Tree bindings for DesignWare DSI Host Controller v1.20a driver >> + >> +A DSI Host Controller resides in the middle of display controller and external >> +HDMI converter or panel. >> + >> +Required properties: >> +- compatible: value should be "hisilicon,hi6220-dsi". >> +- reg: physical base address and length of dsi controller's registers. >> +- clocks: the clocks needed. >> +- clock-names: the name of the clocks. > > You _must_ specify the precise set of clock names you expect here. > > Per the example, you seem to expect "pclk_dsi". > > Is that the name given in the DSI controller manual? Or is it just > "pclk"? It's already specific to the DSI controller... > I have read the SoC manual again, and it is the configuration clock. Maybe we can call it "cfg_clk". >> diff --git a/Documentation/devicetree/bindings/display/hisilicon/hisi-ade.txt b/Documentation/devicetree/bindings/display/hisilicon/hisi-ade.txt >> new file mode 100644 >> index 000000000000..c1844b3ff878 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/hisilicon/hisi-ade.txt >> @@ -0,0 +1,64 @@ >> +Device-Tree bindings for hisilicon ADE display controller driver >> + >> +ADE (Advanced Display Engine) is the display controller which grab image >> +data from memory, do composition, do post image processing, generate RGB >> +timing stream and transfer to DSI. >> + >> +Required properties: >> +- compatible: value should be "hisilicon,hi6220-ade". >> +- reg: physical base address and length of the ADE controller's registers. >> + Value should be "<0x0 0xf4100000 0x0 0x7800>". > > Get rid of the "Value should be ... " part. It is nonsensical to > describe this in the binding. Just describe what this is with relation > to _this_ IP block. OK, will fix all these parts in next version. > >> +- reg-names: name of physical base. Value should be "ade_base". > > That obviously doesn't apply to *-names propertiesm which must all be > specified in the binding (they're local to the device rather than > remote). > >> +- hisilicon,noc-syscon: ADE NOC QoS syscon. Value should be "<&medianoc_ade>" >> +- resets: The ADE reset controller node. Value should be "<&media_ctrl >> + MEDIA_ADE>". > > Likewise. > >> +- interrupt: the ldi vblank interrupt number used. >> +- clocks: the clocks needed. Three clocks are used in ADE driver: >> + ADE core clock, value should be "<&media_ctrl HI6220_ADE_CORE>"; >> + ADE pixel clok, value should be "<&media_ctrl HI6220_ADE_PIX_SRC>"; >> + media NOC QoS clock, value should be "<&media_ctrl HI6220_CODEC_JPEG>". >> +- clock-names: the name of the clocks. Values should be "clk_ade_core", >> + "clk_codec_jpeg" and "clk_ade_pix". > > Likewise, don't specify the value. > > Jsut define clocks in terms of clock-names, e.g. > > - clocks: a list of phandle + clock-specifier pairs, one for each entry > in clock-names. > - clock-names: should contain: > * "clk_ade_core" for the ADE ciore clock > * ... > * ... All right, got it. Thanks for teaching me how to to. > >> +- assigned-clocks: clocks to be assigned rate. >> +- assigned-clock-rates: clock rates which are assigned to assigned-clocks. >> + The rate of <&media_ctrl HI6220_ADE_CORE> could be "360000000" or >> + "180000000"; >> + The rate of <&media_ctrl HI6220_CODEC_JPEG> could be less than "1440000000". > > Is this strictly necessary? Why can the druiver not configure this? > > Does this have to match some pre-existing boot-time configuration? This is the maximum configuration value due to the parent clk and divider. And there is no pre-existing boot-time configuration about this. Driver could configure the value according to the performance and power consumption. Thanks, -xinliang > > Thanks, > Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
