Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock
Hi Tony, On Wednesday 04 of September 2013 10:59:09 Tony Lindgren wrote: > * Haojian Zhuang [130903 20:11]: > > 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. > > I may not follow you here.. But it seems that you've missing something > with the static mapping: It's found based on the physical address. So > if you create static mappings for your SoC with iotable_init(), those > mappings will be available everywhere including drivers when you do > ioremap(). The thing is that today we are moving in favour of fully dynamic mapping, based on data from device tree, with as little as possible (or even no) static mapping based on hardcoded values. So, back to the original problem, we end up doing multiple dynamic mappings of the same physical page, because there is no refcounting in ioremap and, if it doesn't find a static mapping containing the region we're interested in, it simply creates a new mapping. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock
Hi Tony, On Wednesday 04 of September 2013 10:59:09 Tony Lindgren wrote: * Haojian Zhuang haojian.zhu...@linaro.org [130903 20:11]: 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. I may not follow you here.. But it seems that you've missing something with the static mapping: It's found based on the physical address. So if you create static mappings for your SoC with iotable_init(), those mappings will be available everywhere including drivers when you do ioremap(). The thing is that today we are moving in favour of fully dynamic mapping, based on data from device tree, with as little as possible (or even no) static mapping based on hardcoded values. So, back to the original problem, we end up doing multiple dynamic mappings of the same physical page, because there is no refcounting in ioremap and, if it doesn't find a static mapping containing the region we're interested in, it simply creates a new mapping. Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock
* Haojian Zhuang [130903 20:11]: > > 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. I may not follow you here.. But it seems that you've missing something with the static mapping: It's found based on the physical address. So if you create static mappings for your SoC with iotable_init(), those mappings will be available everywhere including drivers when you do ioremap(). Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock
* Haojian Zhuang haojian.zhu...@linaro.org [130903 20:11]: 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. I may not follow you here.. But it seems that you've missing something with the static mapping: It's found based on the physical address. So if you create static mappings for your SoC with iotable_init(), those mappings will be available everywhere including drivers when you do ioremap(). Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock
On 31 August 2013 04:06, Stephen Warren wrote:
> On 08/29/2013 07:45 PM, Haojian Zhuang wrote:
>> On 22 August 2013 13:53, Mike Turquette 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 = <_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", _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
Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock
On 31 August 2013 04:06, Stephen Warren swar...@wwwdotorg.org wrote:
On 08/29/2013 07:45 PM, Haojian Zhuang wrote:
On 22 August 2013 13:53, Mike Turquette mturque...@linaro.org 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
Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock
On 08/29/2013 07:45 PM, Haojian Zhuang wrote:
> On 22 August 2013 13:53, Mike Turquette 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 = <_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", _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?
> 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
Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock
On 08/29/2013 07:45 PM, Haojian Zhuang wrote:
On 22 August 2013 13:53, Mike Turquette mturque...@linaro.org 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?
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
Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock
On 22 August 2013 13:53, Mike Turquette 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.
>
> [1] http://article.gmane.org/gmane.linux.documentation/5679
> [2]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137878.html
>
> Tero Kristo contributed helpful bug fixes to this patch.
>
> Signed-off-by: Mike Turquette
> Tested-by: Heiko Stuebner
> Reviewed-by: Heiko Stuebner
> ---
> Changes since v3:
> * replaced underscores with dashes in DT property names
> * bail from of clock setup function early if of_iomap fails
> * removed unecessary explict cast
>
> .../devicetree/bindings/clock/gate-clock.txt | 36 +
> drivers/clk/clk-gate.c | 47
> ++
> include/linux/clk-provider.h | 2 +
> 3 files changed, 85 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/gate-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/gate-clock.txt
> b/Documentation/devicetree/bindings/clock/gate-clock.txt
> new file mode 100644
> index 000..1c0d4d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/gate-clock.txt
> @@ -0,0 +1,36 @@
> +Binding for simple gate clock.
> +
> +This binding uses the common clock binding[1]. It assumes a
> +register-mapped clock gate controlled by a single bit that has only one
> +input clock or parent. By default setting the specified bit gates the
> +clock signal and clearing the bit ungates it.
> +
> +The binding must provide the register to control the gate and the bit
> +shift for the corresponding gate control bit. Some clocks set the bit to
> +gate the clock signal, and clear it to ungate the clock signal. The
> +optional "set-bit-to-disable" specifies this behavior.
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be "gate-clock".
> +- #clock-cells : from common clock binding; shall be set to 0.
> +- clocks : link to phandle of parent clock
> +- reg : base address for register controlling adjustable gate
> +- bit-shift : bit shift for programming the clock gate
> +
> +Optional properties:
> +- clock-output-names : from common clock binding.
> +- set-bit-to-disable : inverts default gate programming. Setting the bit
> + gates the clock and clearing the bit ungates the clock.
> +- hiword-mask : lower half of the register controls the gate, upper half
> + of the register indicates bits that were updated in the lower half
> +
> +Examples:
> + clock_foo: clock_foo@4a008100 {
> + compatible = "gate-clock";
> + #clock-cells = <0>;
> + clocks = <_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.
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?
> + };
> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
> index 2b28a00..63641c2 100644
> --- a/drivers/clk/clk-gate.c
> +++ b/drivers/clk/clk-gate.c
> @@ -15,6 +15,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> /**
> * DOC: basic gatable clock which can gate and ungate it's ouput
> @@ -162,3 +164,48 @@ struct clk *clk_register_gate(struct device *dev, const
> char *name,
> return clk;
> }
> EXPORT_SYMBOL_GPL(clk_register_gate);
> +
> +#ifdef CONFIG_OF
> +/**
> + * of_gate_clk_setup() - Setup function for simple gate rate clock
> + */
> +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", _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.
Maybe we can resolve it by this way.
DTS file:
clock register bank {
reg = <{start} {size}>;
#address-cells = <1>;
#size-cells = <0>; /* each clock only
exists in one register */
clock node {
compatible =
Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock
On 22 August 2013 13:53, Mike Turquette mturque...@linaro.org 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.
[1] http://article.gmane.org/gmane.linux.documentation/5679
[2]
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137878.html
Tero Kristo contributed helpful bug fixes to this patch.
Signed-off-by: Mike Turquette mturque...@linaro.org
Tested-by: Heiko Stuebner he...@sntech.de
Reviewed-by: Heiko Stuebner he...@sntech.de
---
Changes since v3:
* replaced underscores with dashes in DT property names
* bail from of clock setup function early if of_iomap fails
* removed unecessary explict cast
.../devicetree/bindings/clock/gate-clock.txt | 36 +
drivers/clk/clk-gate.c | 47
++
include/linux/clk-provider.h | 2 +
3 files changed, 85 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/gate-clock.txt
diff --git a/Documentation/devicetree/bindings/clock/gate-clock.txt
b/Documentation/devicetree/bindings/clock/gate-clock.txt
new file mode 100644
index 000..1c0d4d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/gate-clock.txt
@@ -0,0 +1,36 @@
+Binding for simple gate clock.
+
+This binding uses the common clock binding[1]. It assumes a
+register-mapped clock gate controlled by a single bit that has only one
+input clock or parent. By default setting the specified bit gates the
+clock signal and clearing the bit ungates it.
+
+The binding must provide the register to control the gate and the bit
+shift for the corresponding gate control bit. Some clocks set the bit to
+gate the clock signal, and clear it to ungate the clock signal. The
+optional set-bit-to-disable specifies this behavior.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be gate-clock.
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link to phandle of parent clock
+- reg : base address for register controlling adjustable gate
+- bit-shift : bit shift for programming the clock gate
+
+Optional properties:
+- clock-output-names : from common clock binding.
+- set-bit-to-disable : inverts default gate programming. Setting the bit
+ gates the clock and clearing the bit ungates the clock.
+- hiword-mask : lower half of the register controls the gate, upper half
+ of the register indicates bits that were updated in the lower half
+
+Examples:
+ 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.
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?
+ };
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 2b28a00..63641c2 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -15,6 +15,8 @@
#include linux/io.h
#include linux/err.h
#include linux/string.h
+#include linux/of.h
+#include linux/of_address.h
/**
* DOC: basic gatable clock which can gate and ungate it's ouput
@@ -162,3 +164,48 @@ struct clk *clk_register_gate(struct device *dev, const
char *name,
return clk;
}
EXPORT_SYMBOL_GPL(clk_register_gate);
+
+#ifdef CONFIG_OF
+/**
+ * of_gate_clk_setup() - Setup function for simple gate rate clock
+ */
+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.
Maybe we can resolve it by this way.
DTS file:
clock register bank {
reg = {start} {size};
#address-cells = 1;
#size-cells = 0; /* each clock only
exists in one register */
clock node {
[PATCH v4 5/5] clk: dt: binding for basic gate clock
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.
[1] http://article.gmane.org/gmane.linux.documentation/5679
[2]
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137878.html
Tero Kristo contributed helpful bug fixes to this patch.
Signed-off-by: Mike Turquette
Tested-by: Heiko Stuebner
Reviewed-by: Heiko Stuebner
---
Changes since v3:
* replaced underscores with dashes in DT property names
* bail from of clock setup function early if of_iomap fails
* removed unecessary explict cast
.../devicetree/bindings/clock/gate-clock.txt | 36 +
drivers/clk/clk-gate.c | 47 ++
include/linux/clk-provider.h | 2 +
3 files changed, 85 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/gate-clock.txt
diff --git a/Documentation/devicetree/bindings/clock/gate-clock.txt
b/Documentation/devicetree/bindings/clock/gate-clock.txt
new file mode 100644
index 000..1c0d4d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/gate-clock.txt
@@ -0,0 +1,36 @@
+Binding for simple gate clock.
+
+This binding uses the common clock binding[1]. It assumes a
+register-mapped clock gate controlled by a single bit that has only one
+input clock or parent. By default setting the specified bit gates the
+clock signal and clearing the bit ungates it.
+
+The binding must provide the register to control the gate and the bit
+shift for the corresponding gate control bit. Some clocks set the bit to
+gate the clock signal, and clear it to ungate the clock signal. The
+optional "set-bit-to-disable" specifies this behavior.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be "gate-clock".
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link to phandle of parent clock
+- reg : base address for register controlling adjustable gate
+- bit-shift : bit shift for programming the clock gate
+
+Optional properties:
+- clock-output-names : from common clock binding.
+- set-bit-to-disable : inverts default gate programming. Setting the bit
+ gates the clock and clearing the bit ungates the clock.
+- hiword-mask : lower half of the register controls the gate, upper half
+ of the register indicates bits that were updated in the lower half
+
+Examples:
+ clock_foo: clock_foo@4a008100 {
+ compatible = "gate-clock";
+ #clock-cells = <0>;
+ clocks = <_bar>;
+ reg = <0x4a008100 0x4>
+ bit-shift = <3>
+ };
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 2b28a00..63641c2 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -15,6 +15,8 @@
#include
#include
#include
+#include
+#include
/**
* DOC: basic gatable clock which can gate and ungate it's ouput
@@ -162,3 +164,48 @@ struct clk *clk_register_gate(struct device *dev, const
char *name,
return clk;
}
EXPORT_SYMBOL_GPL(clk_register_gate);
+
+#ifdef CONFIG_OF
+/**
+ * of_gate_clk_setup() - Setup function for simple gate rate clock
+ */
+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", _name);
+
+ parent_name = of_clk_get_parent_name(node, 0);
+
+ reg = of_iomap(node, 0);
+ if (!reg) {
+ pr_err("%s: no memory mapped for property reg\n", __func__);
+ return;
+ }
+
+ if (of_property_read_u32(node, "bit-shift", _idx)) {
+ pr_err("%s: missing bit-shift property for %s\n",
+ __func__, node->name);
+ return;
+ }
+
+ if (of_property_read_bool(node, "set-bit-to-disable"))
+ clk_gate_flags |= CLK_GATE_SET_TO_DISABLE;
+
+ if (of_property_read_bool(node, "hiword-mask"))
+ clk_gate_flags |= CLK_GATE_HIWORD_MASK;
+
+ clk = clk_register_gate(NULL, clk_name, parent_name, 0, reg, bit_idx,
+ clk_gate_flags, NULL);
+
+ if (!IS_ERR(clk))
+ of_clk_add_provider(node, of_clk_src_simple_get, clk);
+}
+EXPORT_SYMBOL_GPL(of_gate_clk_setup);
+CLK_OF_DECLARE(gate_clk, "gate-clock", of_gate_clk_setup);
+#endif
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 218d923..b471e37 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -240,6 +240,8 @@ struct clk *clk_register_gate(struct device *dev, const
char *name,
void
[PATCH v4 5/5] clk: dt: binding for basic gate clock
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.
[1] http://article.gmane.org/gmane.linux.documentation/5679
[2]
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137878.html
Tero Kristo contributed helpful bug fixes to this patch.
Signed-off-by: Mike Turquette mturque...@linaro.org
Tested-by: Heiko Stuebner he...@sntech.de
Reviewed-by: Heiko Stuebner he...@sntech.de
---
Changes since v3:
* replaced underscores with dashes in DT property names
* bail from of clock setup function early if of_iomap fails
* removed unecessary explict cast
.../devicetree/bindings/clock/gate-clock.txt | 36 +
drivers/clk/clk-gate.c | 47 ++
include/linux/clk-provider.h | 2 +
3 files changed, 85 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/gate-clock.txt
diff --git a/Documentation/devicetree/bindings/clock/gate-clock.txt
b/Documentation/devicetree/bindings/clock/gate-clock.txt
new file mode 100644
index 000..1c0d4d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/gate-clock.txt
@@ -0,0 +1,36 @@
+Binding for simple gate clock.
+
+This binding uses the common clock binding[1]. It assumes a
+register-mapped clock gate controlled by a single bit that has only one
+input clock or parent. By default setting the specified bit gates the
+clock signal and clearing the bit ungates it.
+
+The binding must provide the register to control the gate and the bit
+shift for the corresponding gate control bit. Some clocks set the bit to
+gate the clock signal, and clear it to ungate the clock signal. The
+optional set-bit-to-disable specifies this behavior.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be gate-clock.
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link to phandle of parent clock
+- reg : base address for register controlling adjustable gate
+- bit-shift : bit shift for programming the clock gate
+
+Optional properties:
+- clock-output-names : from common clock binding.
+- set-bit-to-disable : inverts default gate programming. Setting the bit
+ gates the clock and clearing the bit ungates the clock.
+- hiword-mask : lower half of the register controls the gate, upper half
+ of the register indicates bits that were updated in the lower half
+
+Examples:
+ clock_foo: clock_foo@4a008100 {
+ compatible = gate-clock;
+ #clock-cells = 0;
+ clocks = clock_bar;
+ reg = 0x4a008100 0x4
+ bit-shift = 3
+ };
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 2b28a00..63641c2 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -15,6 +15,8 @@
#include linux/io.h
#include linux/err.h
#include linux/string.h
+#include linux/of.h
+#include linux/of_address.h
/**
* DOC: basic gatable clock which can gate and ungate it's ouput
@@ -162,3 +164,48 @@ struct clk *clk_register_gate(struct device *dev, const
char *name,
return clk;
}
EXPORT_SYMBOL_GPL(clk_register_gate);
+
+#ifdef CONFIG_OF
+/**
+ * of_gate_clk_setup() - Setup function for simple gate rate clock
+ */
+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);
+ if (!reg) {
+ pr_err(%s: no memory mapped for property reg\n, __func__);
+ return;
+ }
+
+ if (of_property_read_u32(node, bit-shift, bit_idx)) {
+ pr_err(%s: missing bit-shift property for %s\n,
+ __func__, node-name);
+ return;
+ }
+
+ if (of_property_read_bool(node, set-bit-to-disable))
+ clk_gate_flags |= CLK_GATE_SET_TO_DISABLE;
+
+ if (of_property_read_bool(node, hiword-mask))
+ clk_gate_flags |= CLK_GATE_HIWORD_MASK;
+
+ clk = clk_register_gate(NULL, clk_name, parent_name, 0, reg, bit_idx,
+ clk_gate_flags, NULL);
+
+ if (!IS_ERR(clk))
+ of_clk_add_provider(node, of_clk_src_simple_get, clk);
+}
+EXPORT_SYMBOL_GPL(of_gate_clk_setup);
+CLK_OF_DECLARE(gate_clk, gate-clock, of_gate_clk_setup);
+#endif
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 218d923..b471e37 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -240,6 +240,8
