RE: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Thanks for your review.

> 
> >
> > > > +- reg: Offset and length of the clock register set
> > > > +- clock-frequency: Indicates input clock frequency of clock block.
> > > > +       Will be set by u-boot
> > >
> > > Why does the fact this is set by u-boot matter to the binding?
> > >
> > OK, I will remove it.
> >
> > > > +
> > > > +Recommended properties:
> > > > +- #ddress-cells: Specifies the number of cells used to represent
> > > > +       physical base addresses.  Must be present if the device has
> > > > +       sub-nodes and set to 1 if present
> > >
> > > Typo: #address-cells
> > >
> > > In the example it looks like the address cells of child nodes are
> > > offsets within the unit, rather than absolute physical addresses.
> > > Could the code not treat them as absolute addresses? Then we'd only
> > > need to document that #address-cells, #size-cells and ranges must be
> > > present and have values suitable for mapping child nodes into the
> > > address space of the parent.
> > >
> > OK, thanks.
> >
> > > > +- #size-cells: Specifies the number of cells used to represent
> > > > +       the size of an address. Must be present if the device has
> > > > +       sub-nodes and set to 1 if present
> > >
> > > It's not really the size of an address, it's the size of a region
> > > identified by a reg entry.
> > >
> > > I think this can be simplified by my suggestion above.
> > >
> > Yes
> 
> Aah, I see that this is already in use, so it's a bit late to change
> this...
> 
I will modify the driver anyway once the binding gets done.

> >
> > > > +
> > > > +2. Clock Provider/Consumer Binding
> > > > +
> > > > +Most of the binding are from the common clock binding[1].
> > > > + [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > +
> > > > +Required properties:
> > > > +- compatible : Should include one or more of the following:
> 
> I didn't spot this earlier, but why "one or more"? are the 2.0 variants
> backwards compatible with the 1.0 variants.
> 
One or more for fixed-clock which is compatible with qoriq-sysclk-*.

> > > > +       - "fsl,qoriq-core-pll-[1,2].x": Indicates a core PLL clock
> > > device
> > > > +       - "fsl,qoriq-core-mux-[1,2].x": Indicates a core
> > > > + multiplexer
> > > clock
> > > > +               device; divided from the core PLL clock
> > >
> > > As above, I'd prefer a complete list of the basic strings we expect.
> > >
> > There is no list here, just *-mux-1.x and *-mux-2.x What kind of list
> > do you prefer?
> 
> Something like:
> 
> - compatible: Should include at least one of:
>     * "fsl,qoriq-core-pll-1.0" for core PLL clocks (v1.0)
>     * "fsl,qoriq-core-pll-2.0" for core PLL clocks (v2.0)
>     * "fsl,qoriq-core-mux-1.0" for core mux clocks (v1.0)
>     * "fsl,qoriq-core-mux-2.0" for core mux clocks (v2.0)
>   The *-2.0 variants are backwards compatible with the *-1.0 variants,
>   so for compatiblity a *-1.0 variant string should be in the list.
> 
See the comment by Scott wood.

> >
> > > > +       - "fixed-clock": From common clock binding; indicates
> > > > + output
> > > clock
> > > > +               of oscillator
> > > > +       - "fsl,qoriq-sysclk-[1,2].x": Indicates input system clock
> > >
> > > Here too.
> > >
> > > > +- #clock-cells: From common clock binding; indicates the number of
> > > > +       output clock. 0 is for one output clock; 1 for more than
> > > > +one clock
> > >
> > > If a clock source has multiple outputs, what those outputs are and
> > > what values in clock-cells they correspond to should be described
> here.
> > >
> > There is no way to describe the values of multiple outputs here.
> > This property is the type of BOOL. See the clock-bindings.txt.
> 
> Sorry? #clock-cells is most definitely _not_ a bool:
> 
ACTs like bool.

> 17: #clock-cells:      Number of cells in a clock specifier; Typically 0
> for nodes
> 18:                    with a single clock output and 1 for nodes with
> multiple
> 19:                    clock outputs.
> 
> And neither are the clock-specifiers encoded with those cells. Consider:
> 
>   pll0: pll0@800 {
>           #clock-cells = <1>;
>           reg = <0x800 0x4>;
>           compatible = "fsl,qoriq-core-pll-1.0";
>           clocks = <&sysclk>;
>           clock-output-names = "pll0", "pll0-div2";
>   };
> 
> Here the value of the cells in a clock-specifier seem to be:
> 0: pll0
> 1: pll0-div2
> 
> So in a consumer, the valid values of the cells in a clock-specifier
> are:
> 
>   consumer: device {
>           compatible = "vendor,some-device";
>           clocks = <&pll0 0>, /* pll0 */
>                    <&pll0 1>; /* pll0-div2 */
>   };
> 
> There must be some meaning assigned to the values of the cells in the
> clock-specifier (e.g. linear index of the clock output in the hardware).
> It would be good to describe this, other clock bindings do.
> 
Sorry, I still get what you mean. There is no INDEX at all.
0 is for one output, 1 for multiple output. Just like that.
What the " other clock bindings" did you refer to?

> >
> > > > +
> > > > +Recommended properties:
> > > > +- clocks: Should be the phandle of input parent clock
> > > > +- clock-names: From common clock binding, indicates the clock
> > > > +name
> > >
> > > That description's a bit opaque.
> > >
> > > What's the name of the clock input on these units? That's what
> > > clock- names should contain, and that should be documented.
> > >
> > Is it necessary to document these names since they are totally used by
> > clock provider and clock consumer has no idea about them?
> 
> I'm not sure I follow -- clocks and clock-names are used by consumers.
> They define which clocks are inputs to a consumer, and the names of the
> clock inputs on the consumer:
> 
>   consumer@0xffff0000 {
>           reg = <0xffff0000 0x1000>;
>           compatible = "vendor,some-consumer";
>           clocks = <&pl011 0>,
>                    <&otherclock 43 22>,
>                    <&pl011 1>;
>           clock-names = "apb_pclk",
>                         "pixel_clk",
>                         "scanout_clk";
>   };
> 
> Here the set of clock-names would be defined in the binding of the
> consumer, based on the clock input names in the IP documentation -- they
> tell the consumer which clock inputs on the consumer the clocks described
> in clocks are wired in to, and describe nothing about output of the
> provider. Using clock-names allows the set of clocks on an IP to change
> over revisions and for some clock inputs to be optional.
> 
OK, I get it. Will name the clock-names better, and add it if missed.
Thanks,

> >
> > > Do we not _always_ need the parent clock?
> > >
> > Not for fixed-clock that its parent clock is oscillator :)
> 
> Certainly fixed-clock doesn't need any parent clock, but I guess PLLs and
> muxes always do.
> 
> >
> > > If we have a clock do we need a clock-names entry for it?
> > >
> > Technically yes, but I don't bother to add it if the clock node has
> > only one clocks.
> 
> Ok. Where we only have one input clock, this doesn't need to be described.
> 
> Do the mux clocks always take 3 input clocks (judging by the examples
> they do)?
> 
> >
> > > > +- clock-output-names: From common clock binding, indicates the
> > > > +names
> > > of
> > > > +       output clocks
> > > > +- reg: Should be the offset and length of clock block base address.
> > > > +       The length should be 4.
> > > > +
> > > > +Example for clock block and clock provider:
> > > > +/ {
> > > > +       clockgen: global-utilities@e1000 {
> > > > +               compatible = "fsl,p5020-clockgen",
> > > > +"fsl,qoriq-clockgen-
> > > 1.0";
> > > > +               reg = <0xe1000 0x1000>;
> > > > +               clock-frequency = <0>;
> > >
> > > That looks odd.
> > >
> > Yes, but it has already existed here before this patch.
> > Can I move it to sysclk clock node since it is not used yet?
> 
> I hadn't realised there were DTS with this already. Why is there a clock
> with clock-frequency = <0> at all? Surely that isn't useful...
> 
See comments by Scott Wood, Thanks you Scoot.

> >
> > > > +               #address-cells = <1>;
> > > > +               #size-cells = <1>;
> > > > +
> > > > +               sysclk: sysclk {
> > > > +                       #clock-cells = <0>;
> > > > +                       compatible = "fsl,qoriq-sysclk-1.0",
> > > > + "fixed-clock";
> > >
> > > We didn't mention in the binding that "fsl,qoriq-sysclk-1.0" was
> > > compatible with "fixed-clock" and should have "fixed-clock" in the
> > > compatible list...
> > >
> > OK, will fix it.
> 
> Cheers. Also, doesn't a fixed-clock require a clock-frequency?
> 
Yes, it should be. But we got the clock-frequency from parent node because
There is a clock-frequency already there before this patch.

> >
> > > > +                       clock-output-names = "sysclk";
> > > > +               }
> > > > +
> > > > +               pll0: pll0@800 {
> > > > +                       #clock-cells = <1>;
> > > > +                       reg = <0x800 0x4>;
> > > > +                       compatible = "fsl,qoriq-core-pll-1.0";
> > > > +                       clocks = <&sysclk>;
> > > > +                       clock-output-names = "pll0", "pll0-div2";
> > > > +               };
> > > > +
> > > > +               pll1: pll1@820 {
> > > > +                       #clock-cells = <1>;
> > > > +                       reg = <0x820 0x4>;
> > > > +                       compatible = "fsl,qoriq-core-pll-1.0";
> > > > +                       clocks = <&sysclk>;
> > > > +                       clock-output-names = "pll1", "pll1-div2";
> > > > +               };
> > > > +
> > > > +               mux0: mux0@0 {
> > > > +                       #clock-cells = <0>;
> > > > +                       reg = <0x0 0x4>;
> > > > +                       compatible = "fsl,qoriq-core-mux-1.0";
> > > > +                       clocks = <&pll0 0>, <&pll0 1>, <&pll1 0>,
> > > <&pll1 1>;
> > > > +                       clock-names = "pll0_0", "pll0_1",
> > > > + "pll1_0",
> > > "pll1_1";
> > > > +                       clock-output-names = "cmux0";
> > > > +               };
> > > > +
> > > > +               mux1: mux1@20 {
> > > > +                       #clock-cells = <0>;
> > > > +                       reg = <0x20 0x4>;
> > > > +                       compatible = "fsl,qoriq-core-mux-1.0";
> > > > +                       clocks = <&pll0 0>, <&pll0 1>, <&pll1 0>,
> > > <&pll1 1>;
> > > > +                       clock-names = "pll0_0", "pll0_1",
> > > > + "pll1_0",
> > > "pll1_1";
> 
> I didn't spot this last time, but the clock-names here seem to be the
> names of the outputs from the provider, rather than the input names of
> the consumer. This goes against the intended purpose of clock-names.
> 
I am confused here with provider/consumer because some nodes can both be consumer and provider.
The clock-names are corresponding to clocks one by one, what's wrong with it?

Regards,
Yuantian

> > > > +                       clock-output-names = "cmux1";
> > >
> > > How does the mux choose which input clock to use at a point in time?
> > >
> > That is decided at runtime. Different input clock will lead to the
> > different Clock frequency that the CPUs work on.
> 
> Ok.
> 
> Cheers,
> Mark.

��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux