Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock

[Haojian Zhuang]

On 31 August 2013 04:06, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote:
> On 08/29/2013 07:45 PM, Haojian Zhuang wrote:
>> On 22 August 2013 13:53, Mike Turquette <mturquette@xxxxxxxxxx> wrote:
>>> Device Tree binding for the basic clock gate, plus the setup function to
>>> register the clock.  Based on the existing fixed-clock binding.
>>>
>>> A different approach to this was proposed in 2012[1] and a similar
>>> binding was proposed more recently[2] if anyone wants some extra
>>> reading.
>
>>> diff --git a/Documentation/devicetree/bindings/clock/gate-clock.txt b/Documentation/devicetree/bindings/clock/gate-clock.txt
>
>>> +Binding for simple gate clock.
> ...
>>> +       clock_foo: clock_foo@4a008100 {
>>> +               compatible = "gate-clock";
>>> +               #clock-cells = <0>;
>>> +               clocks = <&clock_bar>;
>>> +               reg = <0x4a008100 0x4>
>>> +               bit-shift = <3>
>>
>> There's some argument on my clock binding patch set of Hi3620.
>>
>> I defined each clock as one clock node and some of them are sharing one
>> register. Stephen attacked on this since it means multiple clock node sharing
>> one register.
>
> s/attacked/disagreed with/ I think:-)
>
>> Now the same thing also exists in Mike's patch. Mike's patch could also
>> support this behavior. And it's very common that one register is sharing among
>> multiple clocks in every SoC. Which one should I follow?
>
> I believe it's a matter of how the HW is structured.
>
> If the HW truly has segregated register regions for each individual
> clock, and is documented in a way that implies each individual clock is
> a separate HW module, then it makes sense to describe each clock as a
> separate DT node.
>
> However, if the HW simply has a "clock module" that provides many
> clocks, with inter-mingled registers all over the place, and is
> documented as a single module that generates lots of clocks, then it
> makes sense to describe that one module as a single DT node.
>
> In other words, the DT representation should match the HW design and
> documentation.
>
> This is exactly why we have the #clock-cells property in DT; so that a
> clock provider can provide multiple clocks if that's the way the HW is
> designed.
>
>>> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
>
>>> +void of_gate_clk_setup(struct device_node *node)
>>> +{
>>> +       struct clk *clk;
>>> +       const char *clk_name = node->name;
>>> +       void __iomem *reg;
>>> +       const char *parent_name;
>>> +       u8 clk_gate_flags = 0;
>>> +       u32 bit_idx = 0;
>>> +
>>> +       of_property_read_string(node, "clock-output-names", &clk_name);
>>> +
>>> +       parent_name = of_clk_get_parent_name(node, 0);
>>> +
>>> +       reg = of_iomap(node, 0);
>>
>> I suggest not using of_iomap for each clock node.
>>
>> If each clock is one node, it means hundreds of clock node existing in
>> device tree. Hundreds of mapping page only cost unnecessary mapping.
>
> The page table entries will get re-used. I'm not familiar with the mm
> code, but multiple of_iomap() for the exact same range probably just map
> down to incrementing a refcount on some kernel data structure, so
> actually has zero overhead?

I think it's not right. Let check the implemenation in ioremap().

__arm_ioremap_pfn_caller():

    /* try to reuse one of the static mapping whenever possible. */
    svm = find_static_vm_paddr();
    if (svm) {
         addr = svm->vm.addr;
         addr += paddr - svm->vm.phys_addr;
         return (void __iomem *)(offset + addr);
    }
    ...

    area = get_vm_area_caller(size, VM_IOREMAP, caller);
    addr = area->addr;
    ioremap_page_range(addr, addr+size, paddr, ..);

We can see that it'll try to find static mapping. What's the static mapping?
If we define iotable in machine driver, we have the static mapping, just like
debug_ll. If we parse everything from DTS file, it'll always get a new virtual
address from vm area. So it always create a new page mapping even for one
register.

>
>
>> Maybe we can resolve it by this way.
>>
>> DTS file:
>>         clock register bank {
>
> You need a compatible value here, in order to instantiate the top-level
> driver for the "clock generator" HW block.
>
>>                 reg = <{start} {size}>;
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;             /* each clock only
>> exists in one register */
>
> You would need a ranges property here, to map the child reg properties
> into the parent node's address space.
>
>>                 clock node {
>>                         compatible = "xxx";
>>                         #clock-cells = <0>;
>>                         clock-output-names = yyy";
>>                         reg = <{offset}>;
>>                         ... other properties ...
>>                 };
>>          };
>
> That could be a reasonable solution; the existence of a single "clock
> generator" HW block is clearly called out by the existing of the
> top-level DT node, yet the internals of that node are free to be
> whatever you want, since this is purely defined by the binding
> definition for that top-level "clock generator" node.
>
> That all said, I see almost zero value in having all these child nodes,
> since the top-level compatible value implies every single detail about
> the HW, so the list of clocks and their parameters could just as easily
> be a table in the driver for the HW, in order to avoid having to parse
> that data every boot just to end up with the same table...
>
> The only exception would be if the SoC designer truly had composed the
> top-level "clock generator" HW block out of many completely independent
> re-usable clock IP blocks, and many SoCs existed that used those
> individual clock blocks completely unchanged, without any SoC-specific
> differences such as additional SoC-specific clock block types, so that
> one could get greater re-use by parametrizing everything in DT. In my
> (perhaps limited) experience of SoCS, this seems like an /extremely/
> unlikely situation.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/