lkml.org 
[lkml]   [2015]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 1/8] Documentation: add DT binding for ARM System Control and Power Interface(SCPI) protocol
    > >> +Other required properties for all clocks(all from common clock binding):
    > >> +- #clock-cells : should be set to 1 as each of the SCPI clocks have multiple
    > >> + outputs. The clock specifier will be the index to an entry in the list
    > >> + of output clocks.
    > >
    > > Huh? That's somewhat a circular definition.
    > >
    > > What does that number correspond to in the HW? If it's just the number
    > > that the FW expects, that's a reasonable definition.
    > >
    >
    > Not exactly. The clock specifier are used by the consumers and they just
    > indicate the index into the list of clock outputs provided by the clock
    > provider. The consumers need not know the exact identifier used by the
    > provider to identify the clock(either via H/W or F/W)

    Currently the definition is circular because clock-indices is misued. If
    you sort that out then this should become grounded and well-defined.

    [...]

    > >> +scpi_protocol: scpi@2e000000 {
    > >> + compatible = "arm,scpi";
    > >> + mboxes = <&mailbox 0 &mailbox 1>;
    > >> + shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
    > >> +
    > >> + clocks {
    > >> + compatible = "arm,scpi-clocks";
    > >> +
    > >> + scpi_dvfs: scpi_clocks@0 {
    > >> + compatible = "arm,scpi-dvfs-clocks";
    > >> + #clock-cells = <1>;
    > >> + clock-indices = <0>, <1>, <2>;
    > >> + clock-output-names = "vbig", "vlittle", "vgpu";
    > >> + };
    > >> + scpi_clk: scpi_clocks@3 {
    > >> + compatible = "arm,scpi-variable-clocks";
    > >> + #clock-cells = <1>;
    > >> + clock-indices = <3>, <4>;
    > >> + clock-output-names = "pxlclk0", "pxlclk1";
    > >> + };
    > >> + };
    > >> +};
    > >> +
    > >> +cpu@0 {
    > >> + ...
    > >> + reg = <0 0>;
    > >> + clocks = <&scpi_dvfs 0>;
    > >> + clock-names = "vbig";
    > >> +};
    > >> +
    > >> +hdlcd@7ff60000 {
    > >> + ...
    > >> + reg = <0 0x7ff60000 0 0x1000>;
    > >> + clocks = <&scpi_clk 1>;
    > >> + clock-names = "pxlclk";
    > >> +};
    > >> +
    > >> +In the above example, the #clock-cells is set to 1 as required.
    > >> +scpi_dvfs has 3 output clocks namely: vbig, vlittle and vgpu with 0, 1
    > >> +and 2 as clock-indices. scpi_clk has 2 output clocks namely: pxlclk0 and
    > >> +pxlclk1 with 3 and 4 as clock-indices.
    > >> +
    > >> +The first consumer in the example is cpu@0 and it has vbig as input clock.
    > >> +The index '0' in the clock specifier here points to the first entry in the
    > >> +output clocks of scpi_dvfs for which clock_id asrequired by the firmware
    > >> +is 0.
    > >> +
    > >> +Similarly the second example is hdlcd@7ff60000 and it has pxlclk0 as input
    > >> +clock. The index '1' in the clock specifier here points to the second entry
    > >> +in the output clocks of scpi_clocks for which clock_id as required by the
    > >> +firmware is 4.
    > >
    > > To the best of my knowledge, this is wrong. Per the example in
    > > Documentation/devicetree/bindings/clock/clock-bindings.txt, the
    > > clock-indices apply to the logical value in the clock-specifier.
    > >
    > > So <&scpi_clk 3>, <&scpi_clk 4> exist, (and are named "pxlclk0",
    > > "pxlclk1" respectively), but <&scpi_clk 0>, <&scpi_clk 1> do not (or at
    > > least don't have names).
    > >
    >
    > That depends, if your clock provider provides a callback for decoding
    > clock and does this translation, then they can exist.

    Sure, hence the "(or at least don't have names)".

    > Since SCPI is using standard/default callback(of_clk_src_onecell_get),
    > only <&scpi_clk 0>, <&scpi_clk 1> in above example. For any value >=2,
    > of_clk_src_onecell_get will bail out as we have only 2 clocks
    > registered from that provider.

    That's in violation of the semantics of clock-indices, which was added
    to map from a non-contiguous set of clock-specifier values to a list of
    strings. Take a look at of_clk_get_parent_name (which this won't work
    with).

    Also see Documentation/devicetree/bindings/clock/clock-bindings.txt
    (relevant portion duplicated below):

    ----
    clock-indices: If the identifying number for the clocks in the node
    is not linear from zero, then this allows the mapping of
    identifiers into the clock-output-names array.

    For example, if we have two clocks <&oscillator 1> and <&oscillator 3>:

    oscillator {
    compatible = "myclocktype";
    #clock-cells = <1>;
    clock-indices = <1>, <3>;
    clock-output-names = "clka", "clkb";
    }

    This ensures we do not have any empty strings in clock-output-names
    ----

    Note that the indices are the clock-specifier values, not the raw HW/FW
    values.

    Either you should be using <&scpi_clk 3> and <&scpi_clk 4>, or you need
    a different property to map your logical indices to raw HW values.

    Thanks,
    Mark.


    \
     
     \ /
      Last update: 2015-07-22 18:41    [W:4.527 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site