[RFC PATCH] clk: add extension API

[RFC PATCH] clk: add extension API

[Peter De Schrijver]

Add an extension API for clocks. This allows clocktypes to provide extensions
for features which are uncommon and cannot be easily mapped onto normal clock
framework concecpts. eg: resetting blocks, configuring clock phase etc.

Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 drivers/clk/clk.c            |    8 ++++++++
 include/linux/clk-provider.h |    2 ++
 include/linux/clk.h          |    2 ++
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e5d5dc1..39bc458 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -956,6 +956,14 @@ struct clk *clk_get_parent(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_get_parent);
 
+int clk_ext(struct clk *clk, unsigned int op, unsigned long param)
+{
+	if (clk->ops->ext)
+		return clk->ops->ext(clk->hw, op, param);
+
+	return -EINVAL;
+}
+
 /*
  * .get_parent is mandatory for clocks with multiple possible parents.  It is
  * optional for single-parent clocks.  Always call .get_parent if it is
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 6803fb4..08025d3 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -109,6 +109,8 @@ struct clk_ops {
 	int		(*set_rate)(struct clk_hw *hw, unsigned long,
 				    unsigned long);
 	void		(*init)(struct clk_hw *hw);
+	int		(*ext)(struct clk_hw *hw, unsigned int op,
+					unsigned long param);
 };
 
 /**
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 0e078bd..74be656 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -278,4 +278,6 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
 int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
 			struct device *dev);
 
+int clk_ext(struct clk *clk, unsigned int op, unsigned long param);
+
 #endif
-- 
1.7.9.1

[RFC PATCH] clk: add extension API

[Stephen Boyd]

On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
> Add an extension API for clocks. This allows clocktypes to provide extensions
> for features which are uncommon and cannot be easily mapped onto normal clock
> framework concecpts. eg: resetting blocks, configuring clock phase etc.

This seems rather generic. Why not add more specific APIs/concepts like
clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
make them map.

At the least it should be documented so that people know what it should
be used for.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC PATCH] clk: add extension API

[Mike Turquette]

On 20120530-01:52, Stephen Boyd wrote:
> On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
> > Add an extension API for clocks. This allows clocktypes to provide extensions
> > for features which are uncommon and cannot be easily mapped onto normal clock
> > framework concecpts. eg: resetting blocks, configuring clock phase etc.
> 
> This seems rather generic. Why not add more specific APIs/concepts like
> clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
> make them map.
> 

I also wonder if exposing some of these knobs should be done in the
basic clock types.  Meaning that instead of having additional calls in
the clk.h API those calls could be exposed by the basic clock types that
map to the actions.

The question that needs to be answered is this: do generic drivers need
access to these additional functions (clk.h) or just the platform code
which implements some of the clock logic (basic clock types &
platform-speciic clock types).

Regards,
Mike

> At the least it should be documented so that people know what it should
> be used for.

[RFC PATCH] clk: add extension API

[Stephen Boyd]

On 05/30/12 12:40, Mike Turquette wrote:
> I also wonder if exposing some of these knobs should be done in the
> basic clock types.  Meaning that instead of having additional calls in
> the clk.h API those calls could be exposed by the basic clock types that
> map to the actions.

What do you mean by basic clock types that map to actions? Perhaps an
example?

>
> The question that needs to be answered is this: do generic drivers need
> access to these additional functions (clk.h) or just the platform code
> which implements some of the clock logic (basic clock types &
> platform-speciic clock types).

At least for tegra it looks like they need reset assertion and
deassertion in generic drivers.

$ git grep tegra_periph_reset_assert
arch/arm/mach-tegra/clock.c:void tegra_periph_reset_assert(struct clk *c)
arch/arm/mach-tegra/clock.c:EXPORT_SYMBOL(tegra_periph_reset_assert);
arch/arm/mach-tegra/include/mach/clk.h:void tegra_periph_reset_assert(struct clk *c);
arch/arm/mach-tegra/pcie.c:     tegra_periph_reset_assert(tegra_pcie.pcie_xclk);
arch/arm/mach-tegra/pcie.c:     tegra_periph_reset_assert(tegra_pcie.afi_clk);
arch/arm/mach-tegra/pcie.c:     tegra_periph_reset_assert(tegra_pcie.pex_clk);
arch/arm/mach-tegra/pcie.c:     tegra_periph_reset_assert(tegra_pcie.pcie_xclk);
arch/arm/mach-tegra/pcie.c:     tegra_periph_reset_assert(tegra_pcie.afi_clk);
arch/arm/mach-tegra/powergate.c:        tegra_periph_reset_assert(clk);
drivers/i2c/busses/i2c-tegra.c: tegra_periph_reset_assert(i2c_dev->clk);
drivers/input/keyboard/tegra-kbc.c:     tegra_periph_reset_assert(kbc->clk);
drivers/staging/nvec/nvec.c:    tegra_periph_reset_assert(nvec->i2c_clk);

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC PATCH] clk: add extension API

[Mike Turquette]

On 20120530-13:28, Stephen Boyd wrote:
> On 05/30/12 12:40, Mike Turquette wrote:
> > I also wonder if exposing some of these knobs should be done in the
> > basic clock types.  Meaning that instead of having additional calls in
> > the clk.h API those calls could be exposed by the basic clock types that
> > map to the actions.
> 
> What do you mean by basic clock types that map to actions? Perhaps an
> example?
> 

No exmaples to give, just tossing out ideas.

> >
> > The question that needs to be answered is this: do generic drivers need
> > access to these additional functions (clk.h) or just the platform code
> > which implements some of the clock logic (basic clock types &
> > platform-speciic clock types).
> 
> At least for tegra it looks like they need reset assertion and
> deassertion in generic drivers.
> 
> $ git grep tegra_periph_reset_assert
> arch/arm/mach-tegra/clock.c:void tegra_periph_reset_assert(struct clk *c)
> arch/arm/mach-tegra/clock.c:EXPORT_SYMBOL(tegra_periph_reset_assert);
> arch/arm/mach-tegra/include/mach/clk.h:void tegra_periph_reset_assert(struct clk *c);
> arch/arm/mach-tegra/pcie.c:     tegra_periph_reset_assert(tegra_pcie.pcie_xclk);
> arch/arm/mach-tegra/pcie.c:     tegra_periph_reset_assert(tegra_pcie.afi_clk);
> arch/arm/mach-tegra/pcie.c:     tegra_periph_reset_assert(tegra_pcie.pex_clk);
> arch/arm/mach-tegra/pcie.c:     tegra_periph_reset_assert(tegra_pcie.pcie_xclk);
> arch/arm/mach-tegra/pcie.c:     tegra_periph_reset_assert(tegra_pcie.afi_clk);
> arch/arm/mach-tegra/powergate.c:        tegra_periph_reset_assert(clk);
> drivers/i2c/busses/i2c-tegra.c: tegra_periph_reset_assert(i2c_dev->clk);
> drivers/input/keyboard/tegra-kbc.c:     tegra_periph_reset_assert(kbc->clk);
> drivers/staging/nvec/nvec.c:    tegra_periph_reset_assert(nvec->i2c_clk);
> 

Ok, this is good to know.  The same sort of thing is achieved via
runtime pm and the hwmod framework in OMAP code.  I had given similar
feedback to Andrew Lunn for using clk_prepare/clk_unprepare to power
down the PHY for some of his IP blocks.  I don't think that the clock
framework should be used for that and this clk_reset(...) stuff seems
similar.

Like Benoit, I am partial to associating these actions to module-level
APIs, not necessarily clock-level APIs.

A yardstick to determine whether or not the clock framework is the right
place for a _reset() function might be whether or not it will change the
values of struct clk's members.  If we had a clk_reset(...) call it
would clearly bang some bits in a register via clk->ops->reset ... but
what data would it change in struct clk?  Adjust the rate to 0?  Reset
prepare_count and enable_count to 0?

If it doesn't actually change any of the bookkeeping or accounting in
the clock framework then it might be a clue that the clock framework
isn't the best place for this API.

Thanks,
Mike

[RFC PATCH] clk: add extension API

[Saravana Kannan]

On 05/30/2012 12:40 PM, Mike Turquette wrote:
> On 20120530-01:52, Stephen Boyd wrote:
>> On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
>>> Add an extension API for clocks. This allows clocktypes to provide extensions
>>> for features which are uncommon and cannot be easily mapped onto normal clock
>>> framework concecpts. eg: resetting blocks, configuring clock phase etc.
>>
>> This seems rather generic. Why not add more specific APIs/concepts like
>> clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
>> make them map.
>>
>
> I also wonder if exposing some of these knobs should be done in the
> basic clock types.  Meaning that instead of having additional calls in
> the clk.h API those calls could be exposed by the basic clock types that
> map to the actions.
>
> The question that needs to be answered is this: do generic drivers need
> access to these additional functions (clk.h) or just the platform code
> which implements some of the clock logic (basic clock types&
> platform-speciic clock types).

One of the main reason for the common clock framework is so that each 
platform doesn't have it's own extension and have mostly similar code 
repeat all over the place. So, having clock APIs outside of clk.h 
doesn't make sense when we look at the direction we want the code base 
to proceed in.

-Saravana


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC PATCH] clk: add extension API

[Peter De Schrijver]

On Thu, May 31, 2012 at 05:29:54AM +0200, Saravana Kannan wrote:
> On 05/30/2012 12:40 PM, Mike Turquette wrote:
> > On 20120530-01:52, Stephen Boyd wrote:
> >> On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
> >>> Add an extension API for clocks. This allows clocktypes to provide extensions
> >>> for features which are uncommon and cannot be easily mapped onto normal clock
> >>> framework concecpts. eg: resetting blocks, configuring clock phase etc.
> >>
> >> This seems rather generic. Why not add more specific APIs/concepts like
> >> clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
> >> make them map.
> >>
> >
> > I also wonder if exposing some of these knobs should be done in the
> > basic clock types.  Meaning that instead of having additional calls in
> > the clk.h API those calls could be exposed by the basic clock types that
> > map to the actions.
> >
> > The question that needs to be answered is this: do generic drivers need
> > access to these additional functions (clk.h) or just the platform code
> > which implements some of the clock logic (basic clock types&
> > platform-speciic clock types).
> 
> One of the main reason for the common clock framework is so that each 
> platform doesn't have it's own extension and have mostly similar code 
> repeat all over the place. So, having clock APIs outside of clk.h 
> doesn't make sense when we look at the direction we want the code base 
> to proceed in.

I don't think this will lead to 'mostly similar code repeat all over the
place'. I don't know of any intree SoC which has a similar requirement.
So which code duplication would this cause?

Cheers,

Peter.

[RFC PATCH] clk: add extension API

[Saravana Kannan]

On 05/31/2012 01:23 AM, Peter De Schrijver wrote:
> On Thu, May 31, 2012 at 05:29:54AM +0200, Saravana Kannan wrote:
>> On 05/30/2012 12:40 PM, Mike Turquette wrote:
>>> On 20120530-01:52, Stephen Boyd wrote:
>>>> On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
>>>>> Add an extension API for clocks. This allows clocktypes to provide extensions
>>>>> for features which are uncommon and cannot be easily mapped onto normal clock
>>>>> framework concecpts. eg: resetting blocks, configuring clock phase etc.
>>>>
>>>> This seems rather generic. Why not add more specific APIs/concepts like
>>>> clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
>>>> make them map.
>>>>
>>>
>>> I also wonder if exposing some of these knobs should be done in the
>>> basic clock types.  Meaning that instead of having additional calls in
>>> the clk.h API those calls could be exposed by the basic clock types that
>>> map to the actions.
>>>
>>> The question that needs to be answered is this: do generic drivers need
>>> access to these additional functions (clk.h) or just the platform code
>>> which implements some of the clock logic (basic clock types&
>>> platform-speciic clock types).
>>
>> One of the main reason for the common clock framework is so that each
>> platform doesn't have it's own extension and have mostly similar code
>> repeat all over the place. So, having clock APIs outside of clk.h
>> doesn't make sense when we look at the direction we want the code base
>> to proceed in.
>
> I don't think this will lead to 'mostly similar code repeat all over the
> place'. I don't know of any intree SoC which has a similar requirement.
> So which code duplication would this cause?

It's not clear what you plan to use this API for. So, I can't really 
answer if any intree SoC needs it. But if this is about reset signals, 
it's definitely needed for MSM too. I was planning on bringing this up 
after the basic clock API implementation in the clock framework is 
usable for MSM.

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC PATCH] clk: add extension API

[Peter De Schrijver]

On Wed, May 30, 2012 at 10:52:31AM +0200, Stephen Boyd wrote:
> On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
> > Add an extension API for clocks. This allows clocktypes to provide extensions
> > for features which are uncommon and cannot be easily mapped onto normal clock
> > framework concecpts. eg: resetting blocks, configuring clock phase etc.
> 
> This seems rather generic. Why not add more specific APIs/concepts like
> clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
> make them map.
> 

Some of those might be very SoC specific. Eg OMAP doesn't need software
controlled modulereset. I don't think we should add a new function to the
clock framework for clock related features which only exist in a single
SoC or family. Ideally we could use inheritance to add methods to derived
clocktypes, but that's not really possible in C unfortunately.

Cheers,

Peter.

[RFC PATCH] clk: add extension API

[Felipe Balbi]

On Thu, May 31, 2012 at 10:51:25AM +0300, Peter De Schrijver wrote:
> On Wed, May 30, 2012 at 10:52:31AM +0200, Stephen Boyd wrote:
> > On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
> > > Add an extension API for clocks. This allows clocktypes to provide extensions
> > > for features which are uncommon and cannot be easily mapped onto normal clock
> > > framework concecpts. eg: resetting blocks, configuring clock phase etc.
> > 
> > This seems rather generic. Why not add more specific APIs/concepts like
> > clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
> > make them map.
> > 
> 
> Some of those might be very SoC specific. Eg OMAP doesn't need software
> controlled modulereset. I don't think we should add a new function to the

it depends on what you call modulereset. We have soft-reset logic hidden
under the hood, it's done before device creation, so drivers (most of
them) assume we're probe with the IP in reset state.

What I wonder most is if this should be done at the clock level or at
the device level. In the end you reset the IP block, not the clock,
right ?

> clock framework for clock related features which only exist in a single
> SoC or family. Ideally we could use inheritance to add methods to derived
> clocktypes, but that's not really possible in C unfortunately.

Well, it can be done... in a weird manner, but it can:

struct my_new_clk {
	struct clk	clk;

	int		(*reset)(struct my_new_clk *m);
};

int clk_reset(struct clk *clk)
{
	strut my_new_clk	*m = container_of(clk, struct my_new_clk, clk);

	return m->reset(m);
}

it doesn't look pretty, but it can be done.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120531/1acbf2af/attachment.sig>

[RFC PATCH] clk: add extension API

[Peter De Schrijver]

On Thu, May 31, 2012 at 10:18:42AM +0200, Felipe Balbi wrote:
> * PGP Signed by an unknown key
> 
> On Thu, May 31, 2012 at 10:51:25AM +0300, Peter De Schrijver wrote:
> > On Wed, May 30, 2012 at 10:52:31AM +0200, Stephen Boyd wrote:
> > > On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
> > > > Add an extension API for clocks. This allows clocktypes to provide extensions
> > > > for features which are uncommon and cannot be easily mapped onto normal clock
> > > > framework concecpts. eg: resetting blocks, configuring clock phase etc.
> > > 
> > > This seems rather generic. Why not add more specific APIs/concepts like
> > > clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
> > > make them map.
> > > 
> > 
> > Some of those might be very SoC specific. Eg OMAP doesn't need software
> > controlled modulereset. I don't think we should add a new function to the
> 
> it depends on what you call modulereset. We have soft-reset logic hidden
> under the hood, it's done before device creation, so drivers (most of
> them) assume we're probe with the IP in reset state.
> 
> What I wonder most is if this should be done at the clock level or at
> the device level. In the end you reset the IP block, not the clock,
> right ?

Yes. but, every block has at least 1 clock and thus the mapping is identical
down to the register level. Ie. we could do this outside the clockframework,
but then we would have the keep a list of IDs (1 per module) which the drivers
can use to call some tegra reset function which would in the end use registers
in the same memory area to cause a reset. (the registers controlling
modulereset are interleaved with those controlling the enable/disable of the
main moduleclock and bitpositions are identical)

Cheers,

Peter.

[RFC PATCH] clk: add extension API

[Felipe Balbi]

Hi,

On Thu, May 31, 2012 at 11:31:31AM +0300, Peter De Schrijver wrote:
> On Thu, May 31, 2012 at 10:18:42AM +0200, Felipe Balbi wrote:
> > * PGP Signed by an unknown key
> > 
> > On Thu, May 31, 2012 at 10:51:25AM +0300, Peter De Schrijver wrote:
> > > On Wed, May 30, 2012 at 10:52:31AM +0200, Stephen Boyd wrote:
> > > > On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
> > > > > Add an extension API for clocks. This allows clocktypes to provide extensions
> > > > > for features which are uncommon and cannot be easily mapped onto normal clock
> > > > > framework concecpts. eg: resetting blocks, configuring clock phase etc.
> > > > 
> > > > This seems rather generic. Why not add more specific APIs/concepts like
> > > > clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
> > > > make them map.
> > > > 
> > > 
> > > Some of those might be very SoC specific. Eg OMAP doesn't need software
> > > controlled modulereset. I don't think we should add a new function to the
> > 
> > it depends on what you call modulereset. We have soft-reset logic hidden
> > under the hood, it's done before device creation, so drivers (most of
> > them) assume we're probe with the IP in reset state.
> > 
> > What I wonder most is if this should be done at the clock level or at
> > the device level. In the end you reset the IP block, not the clock,
> > right ?
> 
> Yes. but, every block has at least 1 clock and thus the mapping is identical
> down to the register level. Ie. we could do this outside the clockframework,

What if a module needs two clocks and you drive the reset on both of the
clocks ? What would happen ?

> but then we would have the keep a list of IDs (1 per module) which the drivers
> can use to call some tegra reset function which would in the end use registers
> in the same memory area to cause a reset. (the registers controlling
> modulereset are interleaved with those controlling the enable/disable of the
> main moduleclock and bitpositions are identical)

Well, under a generic device-level API, you could just call an internal
clk_reset() function because you know which clocks feed into which
devices anyway. It could look something like:

on Tegra:

device_reset(dev)
	-> dev_pm_domain->reset()
		-> tegra_periph_reset()

on OMAP:

device_reset(dev)
	-> dev_pm_domain->reset()
		-> omap_hwmod_reset()


btw:

tegra_periph_reset(....)
{
	tegra_periph_reset_assert(...);
	udelay(2);
	tegra_periph_reset_deassert(...);
}

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120531/cc589cf2/attachment.sig>

[RFC PATCH] clk: add extension API

[Peter De Schrijver]

> 
> What if a module needs two clocks and you drive the reset on both of the
> clocks ? What would happen ?
> 

Only 'leave clocks' have this reset method and a module can only have 1 of
them.

> > but then we would have the keep a list of IDs (1 per module) which the drivers
> > can use to call some tegra reset function which would in the end use registers
> > in the same memory area to cause a reset. (the registers controlling
> > modulereset are interleaved with those controlling the enable/disable of the
> > main moduleclock and bitpositions are identical)
> 
> Well, under a generic device-level API, you could just call an internal
> clk_reset() function because you know which clocks feed into which
> devices anyway. It could look something like:
> 
> on Tegra:
> 
> device_reset(dev)
> 	-> dev_pm_domain->reset()
> 		-> tegra_periph_reset()
> 

These methods are also needed internally by the powergating code.

> on OMAP:
> 
> device_reset(dev)
> 	-> dev_pm_domain->reset()
> 		-> omap_hwmod_reset()
> 
> 
> btw:
> 
> tegra_periph_reset(....)
> {
> 	tegra_periph_reset_assert(...);
> 	udelay(2);
> 	tegra_periph_reset_deassert(...);
> }

which uses the clockframework currently.

Cheers,

Peter.

[RFC PATCH] clk: add extension API

[Felipe Balbi]

Hi,

On Thu, May 31, 2012 at 12:05:18PM +0300, Peter De Schrijver wrote:
> > 
> > What if a module needs two clocks and you drive the reset on both of the
> > clocks ? What would happen ?
> > 
> 
> Only 'leave clocks' have this reset method and a module can only have 1 of
> them.

fair enough... got mislead by you "every module has at least one clock"
statement.

> > > but then we would have the keep a list of IDs (1 per module) which the drivers
> > > can use to call some tegra reset function which would in the end use registers
> > > in the same memory area to cause a reset. (the registers controlling
> > > modulereset are interleaved with those controlling the enable/disable of the
> > > main moduleclock and bitpositions are identical)
> > 
> > Well, under a generic device-level API, you could just call an internal
> > clk_reset() function because you know which clocks feed into which
> > devices anyway. It could look something like:
> > 
> > on Tegra:
> > 
> > device_reset(dev)
> > 	-> dev_pm_domain->reset()
> > 		-> tegra_periph_reset()
> > 
> 
> These methods are also needed internally by the powergating code.

so ? Just call them when you need...

> > on OMAP:
> > 
> > device_reset(dev)
> > 	-> dev_pm_domain->reset()
> > 		-> omap_hwmod_reset()
> > 
> > 
> > btw:
> > 
> > tegra_periph_reset(....)
> > {
> > 	tegra_periph_reset_assert(...);
> > 	udelay(2);
> > 	tegra_periph_reset_deassert(...);
> > }
> 
> which uses the clockframework currently.

no problems there. The point is that you already know which clock feed
into which device, so if you have a device-based API for device
soft-reset, you can figure out which exact clock to toggle, right ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120531/20cd9415/attachment-0001.sig>

[RFC PATCH] clk: add extension API

[Peter De Schrijver]

> > > on Tegra:
> > > 
> > > device_reset(dev)
> > > 	-> dev_pm_domain->reset()
> > > 		-> tegra_periph_reset()
> > > 
> > 
> > These methods are also needed internally by the powergating code.
> 
> so ? Just call them when you need...
> 

the powergating code calls assert and deassert indepedently
ie:

tegra_periph_reset_assert()

do stuff

tegra_periph_reset_assert()

> > > on OMAP:
> > > 
> > > device_reset(dev)
> > > 	-> dev_pm_domain->reset()
> > > 		-> omap_hwmod_reset()
> > > 
> > > 
> > > btw:
> > > 
> > > tegra_periph_reset(....)
> > > {
> > > 	tegra_periph_reset_assert(...);
> > > 	udelay(2);
> > > 	tegra_periph_reset_deassert(...);
> > > }
> > 
> > which uses the clockframework currently.
> 
> no problems there. The point is that you already know which clock feed
> into which device, so if you have a device-based API for device
> soft-reset, you can figure out which exact clock to toggle, right ?

you have the struct clk, you could dive into that and grab clk_hw and call
some function directly. But sounds quite horrible to me.

Cheers,

Peter.

[RFC PATCH] clk: add extension API

[Felipe Balbi]

On Thu, May 31, 2012 at 12:43:01PM +0300, Peter De Schrijver wrote:
> > > > on Tegra:
> > > > 
> > > > device_reset(dev)
> > > > 	-> dev_pm_domain->reset()
> > > > 		-> tegra_periph_reset()
> > > > 
> > > 
> > > These methods are also needed internally by the powergating code.
> > 
> > so ? Just call them when you need...
> > 
> 
> the powergating code calls assert and deassert indepedently
> ie:
> 
> tegra_periph_reset_assert()
> 
> do stuff
> 
> tegra_periph_reset_assert()

I don't see the issue here. expose tegra_periph_reset() for the device
API only, internally you can do whatever you like. Call them
independently if you need.

> > > > on OMAP:
> > > > 
> > > > device_reset(dev)
> > > > 	-> dev_pm_domain->reset()
> > > > 		-> omap_hwmod_reset()
> > > > 
> > > > 
> > > > btw:
> > > > 
> > > > tegra_periph_reset(....)
> > > > {
> > > > 	tegra_periph_reset_assert(...);
> > > > 	udelay(2);
> > > > 	tegra_periph_reset_deassert(...);
> > > > }
> > > 
> > > which uses the clockframework currently.
> > 
> > no problems there. The point is that you already know which clock feed
> > into which device, so if you have a device-based API for device
> > soft-reset, you can figure out which exact clock to toggle, right ?
> 
> you have the struct clk, you could dive into that and grab clk_hw and call
> some function directly. But sounds quite horrible to me.

Well, that's just because struct device doesn't know about its own clock
providers, right ? Should that be patched too ?

Russell, as the orignal author of the clk API, what do you think ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120531/f2e30a7e/attachment.sig>

[RFC PATCH] clk: add extension API

[Peter De Schrijver]

> 
> Well, that's just because struct device doesn't know about its own clock
> providers, right ? Should that be patched too ?
> 
> Russell, as the orignal author of the clk API, what do you think ?

Even if it would know, how would that solve the problem? only the
clockframework is supposed to know about struct clk.

Cheers,

Peter.

[RFC PATCH] clk: add extension API

[Felipe Balbi]

On Thu, May 31, 2012 at 12:57:23PM +0300, Peter De Schrijver wrote:
> 
> > 
> > Well, that's just because struct device doesn't know about its own clock
> > providers, right ? Should that be patched too ?
> > 
> > Russell, as the orignal author of the clk API, what do you think ?
> 
> Even if it would know, how would that solve the problem? only the
> clockframework is supposed to know about struct clk.

you have the device and you have the clock, you need to toggle a bit on
the clock address space. I guess that pretty much says it, no ? Unless
your implementation of struct clk doesn't tell you clk register offset
which is rather unlikely.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120531/1cc895c9/attachment.sig>

[RFC PATCH] clk: add extension API

[Cousson, Benoit]

Hi Peter,

On 5/31/2012 11:05 AM, Peter De Schrijver wrote:
>>
>> What if a module needs two clocks and you drive the reset on both of the
>> clocks ? What would happen ?
>>
>
> Only 'leave clocks' have this reset method and a module can only have 1 of
> them.
>
>>> but then we would have the keep a list of IDs (1 per module) which the drivers
>>> can use to call some tegra reset function which would in the end use registers
>>> in the same memory area to cause a reset. (the registers controlling
>>> modulereset are interleaved with those controlling the enable/disable of the
>>> main moduleclock and bitpositions are identical)
>>
>> Well, under a generic device-level API, you could just call an internal
>> clk_reset() function because you know which clocks feed into which
>> devices anyway. It could look something like:
>>
>> on Tegra:
>>
>> device_reset(dev)
>> 	->  dev_pm_domain->reset()
>> 		->  tegra_periph_reset()
>>
>
> These methods are also needed internally by the powergating code.
>
>> on OMAP:
>>
>> device_reset(dev)
>> 	->  dev_pm_domain->reset()
>> 		->  omap_hwmod_reset()
>>
>>
>> btw:
>>
>> tegra_periph_reset(....)
>> {
>> 	tegra_periph_reset_assert(...);
>> 	udelay(2);
>> 	tegra_periph_reset_deassert(...);
>> }
>
> which uses the clockframework currently.

Hehe, that a little bit why we had to introduce hwmod for OMAP.
A HW IP cannot just be represented by a clock node like it used to be in 
the good old time.

Now the question is should we extend the Linux device structure to 
handle such HW IP, or should we extend the clock definition to handle 
this kind of extended clock node.

It looks to me that this kind of function does belong to the device more 
than to the clock node.

FWIW, we do have as well issue managing properly the reset for OMAP IPs. 
It is done using custom OMAP hooks today and hwmod whereas it looks like 
it is really a common problem for SoC in general.

Regards,
Benoit

[RFC PATCH] clk: add extension API

[Mark Brown]

On Thu, May 31, 2012 at 02:50:00PM +0200, Cousson, Benoit wrote:

> Now the question is should we extend the Linux device structure to
> handle such HW IP, or should we extend the clock definition to
> handle this kind of extended clock node.

> It looks to me that this kind of function does belong to the device
> more than to the clock node.

This is looking a lot like what power domains do to me...

[RFC PATCH] clk: add extension API

[Cousson, Benoit]

On 5/31/2012 3:04 PM, Mark Brown wrote:
> On Thu, May 31, 2012 at 02:50:00PM +0200, Cousson, Benoit wrote:
>
>> Now the question is should we extend the Linux device structure to
>> handle such HW IP, or should we extend the clock definition to
>> handle this kind of extended clock node.
>
>> It looks to me that this kind of function does belong to the device
>> more than to the clock node.
>
> This is looking a lot like what power domains do to me...

Do you mean the reset part?

[RFC PATCH] clk: add extension API

[Mark Brown]

On Thu, May 31, 2012 at 03:07:53PM +0200, Cousson, Benoit wrote:
> On 5/31/2012 3:04 PM, Mark Brown wrote:
> >On Thu, May 31, 2012 at 02:50:00PM +0200, Cousson, Benoit wrote:

> >>It looks to me that this kind of function does belong to the device
> >>more than to the clock node.

> >This is looking a lot like what power domains do to me...

> Do you mean the reset part?

Yes, the general "this block goes into an idle state and so we need to
rebuild/reset the state of the things in it afterwards" thing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120531/8a7de937/attachment.sig>

[RFC PATCH] clk: add extension API

[Mark Brown]

On Thu, May 31, 2012 at 11:31:31AM +0300, Peter De Schrijver wrote:

> > What I wonder most is if this should be done at the clock level or at
> > the device level. In the end you reset the IP block, not the clock,
> > right ?

> Yes. but, every block has at least 1 clock and thus the mapping is identical
> down to the register level. Ie. we could do this outside the clockframework,
> but then we would have the keep a list of IDs (1 per module) which the drivers
> can use to call some tegra reset function which would in the end use registers
> in the same memory area to cause a reset. (the registers controlling
> modulereset are interleaved with those controlling the enable/disable of the
> main moduleclock and bitpositions are identical)

One option which doesn't really need anything from the clock API is for
the module resets to restore these registers so the clock API never
notices anything changed.

[RFC PATCH] clk: add extension API

[Stephen Boyd]

On 5/31/2012 12:51 AM, Peter De Schrijver wrote:
> On Wed, May 30, 2012 at 10:52:31AM +0200, Stephen Boyd wrote:
>> On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
>>> Add an extension API for clocks. This allows clocktypes to provide extensions
>>> for features which are uncommon and cannot be easily mapped onto normal clock
>>> framework concecpts. eg: resetting blocks, configuring clock phase etc.
>> This seems rather generic. Why not add more specific APIs/concepts like
>> clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
>> make them map.
>>
> Some of those might be very SoC specific. Eg OMAP doesn't need software
> controlled modulereset. I don't think we should add a new function to the
> clock framework for clock related features which only exist in a single
> SoC or family. Ideally we could use inheritance to add methods to derived
> clocktypes, but that's not really possible in C unfortunately.

Most likely your hardware is not that special and the functionality you
think exists only on your SoC actually exists on 5 others. MSM needs
software resets too and we also associate them with each clock (there
can be many clocks for one device and one reset for each clock). We use
them mostly for the power domain code (looks like tegra and omap do the
same thing albeit slightly differently).

What do you plan to support under this API?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC PATCH] clk: add extension API

[Peter De Schrijver]

On Thu, May 31, 2012 at 11:04:36AM +0200, Stephen Boyd wrote:
> On 5/31/2012 12:51 AM, Peter De Schrijver wrote:
> > On Wed, May 30, 2012 at 10:52:31AM +0200, Stephen Boyd wrote:
> >> On 5/29/2012 2:58 AM, Peter De Schrijver wrote:
> >>> Add an extension API for clocks. This allows clocktypes to provide extensions
> >>> for features which are uncommon and cannot be easily mapped onto normal clock
> >>> framework concecpts. eg: resetting blocks, configuring clock phase etc.
> >> This seems rather generic. Why not add more specific APIs/concepts like
> >> clk_reset(), clk_set_phase(), etc.? If they don't map, maybe we should
> >> make them map.
> >>
> > Some of those might be very SoC specific. Eg OMAP doesn't need software
> > controlled modulereset. I don't think we should add a new function to the
> > clock framework for clock related features which only exist in a single
> > SoC or family. Ideally we could use inheritance to add methods to derived
> > clocktypes, but that's not really possible in C unfortunately.
> 
> Most likely your hardware is not that special and the functionality you
> think exists only on your SoC actually exists on 5 others. MSM needs
> software resets too and we also associate them with each clock (there
> can be many clocks for one device and one reset for each clock). We use
> them mostly for the power domain code (looks like tegra and omap do the
> same thing albeit slightly differently).
> 
> What do you plan to support under this API?

Anything which isn't available on other SoCs. If it is available, then we
could indeed add another method. I guess this will need to be decided on
a case by case basis.

Cheers,

Peter.

[RFC PATCH] clk: add extension API

[Rajendra Nayak]

Hi Peter,

On Tuesday 29 May 2012 03:28 PM, Peter De Schrijver wrote:
> Add an extension API for clocks. This allows clocktypes to provide extensions
> for features which are uncommon and cannot be easily mapped onto normal clock
> framework concecpts. eg: resetting blocks, configuring clock phase etc.

I was thinking on similar lines for OMAP wherein I need to control
'hardware-auto-gating' bits on clocks which are otherwise just normal
gates or dividers. We just disable all of them early at boot and
re-enable them late in the boot process once PM kicks in.

regards,
Rajendra

>
> Signed-off-by: Peter De Schrijver<pdeschrijver@nvidia.com>
> ---
>   drivers/clk/clk.c            |    8 ++++++++
>   include/linux/clk-provider.h |    2 ++
>   include/linux/clk.h          |    2 ++
>   3 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index e5d5dc1..39bc458 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -956,6 +956,14 @@ struct clk *clk_get_parent(struct clk *clk)
>   }
>   EXPORT_SYMBOL_GPL(clk_get_parent);
>
> +int clk_ext(struct clk *clk, unsigned int op, unsigned long param)
> +{
> +	if (clk->ops->ext)
> +		return clk->ops->ext(clk->hw, op, param);
> +
> +	return -EINVAL;
> +}
> +
>   /*
>    * .get_parent is mandatory for clocks with multiple possible parents.  It is
>    * optional for single-parent clocks.  Always call .get_parent if it is
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 6803fb4..08025d3 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -109,6 +109,8 @@ struct clk_ops {
>   	int		(*set_rate)(struct clk_hw *hw, unsigned long,
>   				    unsigned long);
>   	void		(*init)(struct clk_hw *hw);
> +	int		(*ext)(struct clk_hw *hw, unsigned int op,
> +					unsigned long param);
>   };
>
>   /**
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 0e078bd..74be656 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -278,4 +278,6 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
>   int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
>   			struct device *dev);
>
> +int clk_ext(struct clk *clk, unsigned int op, unsigned long param);
> +
>   #endif

[RFC PATCH] clk: add extension API

[Mike Turquette]

On 20120530-14:58, Rajendra Nayak wrote:
> Hi Peter,
> 
> On Tuesday 29 May 2012 03:28 PM, Peter De Schrijver wrote:
> >Add an extension API for clocks. This allows clocktypes to provide extensions
> >for features which are uncommon and cannot be easily mapped onto normal clock
> >framework concecpts. eg: resetting blocks, configuring clock phase etc.
> 
> I was thinking on similar lines for OMAP wherein I need to control
> 'hardware-auto-gating' bits on clocks which are otherwise just normal
> gates or dividers. We just disable all of them early at boot and
> re-enable them late in the boot process once PM kicks in.
> 

It would be helpful to get an idea of what these different functions
actually do.  This patch exposes a sort of poor man's ioctl/syscall
interface (which may indeed be necessary), but I would first prefer to
pool together everybody's needs and see if some new high-level APIs
should be added to clk.h.

Already a clk_reset/module reset call has been discussed in this thread.
The changelog discusses setting clock phase and Rajendra mentioned
controlling auto-gating behavior.

Do other folks have these same needs?  What about other needs that are
unlisted?  Please add them to the thread so we can get an idea of what
we're up against.

If I merge this patch in as-is then we'll never compare notes on what
could be common and everyone will just hide their code behind this new
clk_ext call ;-)

Regards, Mike

> regards,
> Rajendra
> 
> >
> >Signed-off-by: Peter De Schrijver<pdeschrijver@nvidia.com>
> >---
> >  drivers/clk/clk.c            |    8 ++++++++
> >  include/linux/clk-provider.h |    2 ++
> >  include/linux/clk.h          |    2 ++
> >  3 files changed, 12 insertions(+), 0 deletions(-)
> >
> >diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >index e5d5dc1..39bc458 100644
> >--- a/drivers/clk/clk.c
> >+++ b/drivers/clk/clk.c
> >@@ -956,6 +956,14 @@ struct clk *clk_get_parent(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_get_parent);
> >
> >+int clk_ext(struct clk *clk, unsigned int op, unsigned long param)
> >+{
> >+	if (clk->ops->ext)
> >+		return clk->ops->ext(clk->hw, op, param);
> >+
> >+	return -EINVAL;
> >+}
> >+
> >  /*
> >   * .get_parent is mandatory for clocks with multiple possible parents.  It is
> >   * optional for single-parent clocks.  Always call .get_parent if it is
> >diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> >index 6803fb4..08025d3 100644
> >--- a/include/linux/clk-provider.h
> >+++ b/include/linux/clk-provider.h
> >@@ -109,6 +109,8 @@ struct clk_ops {
> >  	int		(*set_rate)(struct clk_hw *hw, unsigned long,
> >  				    unsigned long);
> >  	void		(*init)(struct clk_hw *hw);
> >+	int		(*ext)(struct clk_hw *hw, unsigned int op,
> >+					unsigned long param);
> >  };
> >
> >  /**
> >diff --git a/include/linux/clk.h b/include/linux/clk.h
> >index 0e078bd..74be656 100644
> >--- a/include/linux/clk.h
> >+++ b/include/linux/clk.h
> >@@ -278,4 +278,6 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
> >  int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
> >  			struct device *dev);
> >
> >+int clk_ext(struct clk *clk, unsigned int op, unsigned long param);
> >+
> >  #endif
> 

[RFC PATCH] clk: add extension API

[Peter De Schrijver]

On Thu, May 31, 2012 at 09:19:16PM +0200, Mike Turquette wrote:
> On 20120530-14:58, Rajendra Nayak wrote:
> > Hi Peter,
> > 
> > On Tuesday 29 May 2012 03:28 PM, Peter De Schrijver wrote:
> > >Add an extension API for clocks. This allows clocktypes to provide extensions
> > >for features which are uncommon and cannot be easily mapped onto normal clock
> > >framework concecpts. eg: resetting blocks, configuring clock phase etc.
> > 
> > I was thinking on similar lines for OMAP wherein I need to control
> > 'hardware-auto-gating' bits on clocks which are otherwise just normal
> > gates or dividers. We just disable all of them early at boot and
> > re-enable them late in the boot process once PM kicks in.
> > 
> 
> It would be helpful to get an idea of what these different functions
> actually do.  This patch exposes a sort of poor man's ioctl/syscall
> interface (which may indeed be necessary), but I would first prefer to
> pool together everybody's needs and see if some new high-level APIs
> should be added to clk.h.
> 
> Already a clk_reset/module reset call has been discussed in this thread.
> The changelog discusses setting clock phase and Rajendra mentioned
> controlling auto-gating behavior.
> 
> Do other folks have these same needs?  What about other needs that are
> unlisted?  Please add them to the thread so we can get an idea of what
> we're up against.

* clock phase (invert or programmable delay)
* external request line configuration

Cheers,

Peter.

[RFC PATCH] clk: add extension API

[Stephen Warren]

On 05/29/2012 03:58 AM, Peter De Schrijver wrote:
> Add an extension API for clocks. This allows clocktypes to provide extensions
> for features which are uncommon and cannot be easily mapped onto normal clock
> framework concecpts. eg: resetting blocks, configuring clock phase etc.

I'm not sure that we should expose module reset as an operation on a clock.

In Tegra, there are resets that affect multiple clocks (well, they
affect portions of HW that use multiple clocks, not the clocks themselves).

Conversely, it's possible in general that there could be some clock
domains where different subsets of the clock domain are affected by
different reset domains.

Tieing the clock and reset domains together doesn't seem correct. A
separate reset API (and perhaps reset binding for DT) might make more sense.