Re: [PATCH v3 11/31] net: can: mscan: improve clock API use
[ trimming the CC: list a bit, as this is CAN and clock specific,
keeping Mark Brown and Greg KH for the UART and SPI part ]
On Tue, Jul 23, 2013 at 14:33 +0200, Marc Kleine-Budde wrote:
On 07/23/2013 01:53 PM, Gerhard Sittig wrote:
On Mon, Jul 22, 2013 at 14:31 +0200, Marc Kleine-Budde wrote:
On 07/22/2013 02:14 PM, Gerhard Sittig wrote:
the .get_clock() callback is run from probe() and might allocate
resources, introduce a .put_clock() callback that is run from remove()
to undo any allocation activities
looks good
use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put
upon driver unload
fine
assume that resources get prepared but not necessarily enabled in the
setup phase, make the open() and close() callbacks of the CAN network
device enable and disable a previously acquired and prepared clock
I think you should call prepare_enable and disable_unprepare in the
open/close functions.
After more local research, which totally eliminated the need to
pre-enable the CAN related clocks, but might need more discussion
as it touches the common gate support, I've learned something
more:
The CAN clock needs to get enabled during probe() already, since
registers get accessed between probe() for the driver and open()
for the network device -- while access to peripheral registers
crashes the kernel when clocks still are disabled (other hardware
may just hang or provide fake data, neither of this is OK).
Then call prepare_enable(); before and disable_unprepare(); after
accessing the registers. Have a look at the flexcan driver.
OK, your feedback made me notice that I mentally have mixed
peripheral access clocks ('ipg') and bitrate clocks ('per') in
the past versions of the driver.
Fixing this, telling 'ipg' and 'per' apart, in bypassing
eliminates the need for shared clock gates. Since the MCLK
subtree of the clock tree apply to both the CAN controller and
the PSC controller, I will have to adjust all of the following:
- the platform's clock driver, telling the gate for the registers
and the mux/div for the bitrate apart
- the CAN driver, acquiring both the 'ipg' clock item for
register access and the can clock for the bitrate, the latter
may get derived from either 'ips' or 'mclk', while 'mclk' may
be derived from either 'sys' or 'ref' (or 'ips' in this
hardware while the mscan(4) driver doesn't use this feature)
- the UART and SPI drivers, acquiring both the 'ipg' clock item
for register access and the 'mclk' item for the bitrate
This obsoletes the request for shared gates and eliminates
another pre-enable workaround in the clock driver backend.
It also is an improvement for the MPC512x platform, and remains
neutral to the MPC52xx platform. It's clearly desirable and
useful, and doesn't break anything. So I will do it.
[ the above applied to CAN, SPI, and UART; the remainder is
specific to CAN only ]
But I won't try to even further widen the scope of the series, I
won't try to address each and every potential for improvement
which drivers may have had for several years and which happened
to not have been addressed yet. This needs to stop at some
reasonable point. I'm not refusing to improve, but I'm asking to
check what is reasonable and what needs to get avoided.
I already introduced a bug in a recent version of the series
which went unnoticed during review (the unbalanced error path in
the network device open routine). I'd rather not mess with power
management aspects in bypassing in a driver that I'm unable to
test thoroughly. Not when I'm trying to work on something
totally different (introducing proper common clock support) and
try to minimize risk and avoid damage.
But I see the point in your suggestion to prepare _and_ enable
the clock during open() as well -- to have open() cope with
whatever probe() did, after all the driver is shared among
platforms, which may differ in what they do during probe().
If you enable a clock to access the registers before open() (and disable
it afterwards), it should not harm any architecture that doesn't need
this clock enabled.
You suggest to turn on the clock during initialization, and turn
it off until the network device actually gets used? I had a look
at the flexcan driver, saw that it used two clock items, as
outlined above for register access and for wired communication.
This is good. But I somehow doubt that the flexcan driver will
work if the ipg clock gets disabled (I assume it's a shared clock
that happens to remain enabled since others use it as well).
I'd rather not open that can of worms, too. My gut is telling me
that either the peripheral does weird things or will lose data
when its (register access) clock gets disabled. I won't try to
address power management and save/restore issues in that driver
now, and I won't try to hunt down and instrument any register
access in the shared code paths of a driver for multiple
Re: [PATCH v3 11/31] net: can: mscan: improve clock API use
On Mon, Jul 22, 2013 at 14:31 +0200, Marc Kleine-Budde wrote: On 07/22/2013 02:14 PM, Gerhard Sittig wrote: the .get_clock() callback is run from probe() and might allocate resources, introduce a .put_clock() callback that is run from remove() to undo any allocation activities looks good use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put upon driver unload fine assume that resources get prepared but not necessarily enabled in the setup phase, make the open() and close() callbacks of the CAN network device enable and disable a previously acquired and prepared clock I think you should call prepare_enable and disable_unprepare in the open/close functions. After more local research, which totally eliminated the need to pre-enable the CAN related clocks, but might need more discussion as it touches the common gate support, I've learned something more: The CAN clock needs to get enabled during probe() already, since registers get accessed between probe() for the driver and open() for the network device -- while access to peripheral registers crashes the kernel when clocks still are disabled (other hardware may just hang or provide fake data, neither of this is OK). But I see the point in your suggestion to prepare _and_ enable the clock during open() as well -- to have open() cope with whatever probe() did, after all the driver is shared among platforms, which may differ in what they do during probe(). So I will: - make open() of the network device prepare _and_ enable the clock for the peripheral (if acquired during probe()) - adjust open() because ATM it leaves the clock enabled when the network device operation fails (the error path is incomplete in v3) - make the MPC512x specific probe() time .get_clock() routine not just prepare but enable the clock as well - and of course address all the shutdown counter parts of the above setup paths This results in: - specific chip drivers only need to balance their private get and put clock routines which are called from probe and remove, common paths DTRT for all of them - correct operation for MPC512x, where common clock is used - still everything is neutral for MPC5200 where common clock isn't used, behaviour is identical to before the change - no assumptions are made about what occurs or doesn't occur during probe(), when the network device is used then the clock is fully setup and operational - when the CAN network device isn't setup (because device tree doesn't describe it, or disables that node), then its clock remains idle (neither gets setup nor enabled) - complete preparation for future improvement wrt power consumption, where potential changes remain isolated to the specific chip (probe() time setup, get_clock() routine) while the ndo part need not get touched any more So this is the most appropriate approach I can come up with. Removing unnecessary devm_put_clk() calls is orthogonal to that. Putting these in isn't totally wrong (they won't harm, and they do signal visual balance more clearly such that the next person won't stop and wonder), but it's true that they are redundant. Trained persons will wonder as much about their presence as untrained persons wonder about their absence. :) Apparently I'm not well trained yet. I thought that being explicit and cautious would be good, but the feedback I got suggests that encoding unnecessary instructions isn't desirable. So I will remove those devm_put_clk() in v4. To save us one more iteration, shall I remove those calls only from error paths during setup? Or shall I remove them from regular shutdown paths as well? How much pain does the community feel with harmless yet unnecessary instructions? :) virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 11/31] net: can: mscan: improve clock API use
On 07/23/2013 01:53 PM, Gerhard Sittig wrote: On Mon, Jul 22, 2013 at 14:31 +0200, Marc Kleine-Budde wrote: On 07/22/2013 02:14 PM, Gerhard Sittig wrote: the .get_clock() callback is run from probe() and might allocate resources, introduce a .put_clock() callback that is run from remove() to undo any allocation activities looks good use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put upon driver unload fine assume that resources get prepared but not necessarily enabled in the setup phase, make the open() and close() callbacks of the CAN network device enable and disable a previously acquired and prepared clock I think you should call prepare_enable and disable_unprepare in the open/close functions. After more local research, which totally eliminated the need to pre-enable the CAN related clocks, but might need more discussion as it touches the common gate support, I've learned something more: The CAN clock needs to get enabled during probe() already, since registers get accessed between probe() for the driver and open() for the network device -- while access to peripheral registers crashes the kernel when clocks still are disabled (other hardware may just hang or provide fake data, neither of this is OK). Then call prepare_enable(); before and disable_unprepare(); after accessing the registers. Have a look at the flexcan driver. But I see the point in your suggestion to prepare _and_ enable the clock during open() as well -- to have open() cope with whatever probe() did, after all the driver is shared among platforms, which may differ in what they do during probe(). If you enable a clock to access the registers before open() (and disable it afterwards), it should not harm any architecture that doesn't need this clock enabled. So I will: - make open() of the network device prepare _and_ enable the clock for the peripheral (if acquired during probe()) good - adjust open() because ATM it leaves the clock enabled when the network device operation fails (the error path is incomplete in v3) yes, clock should be disabled if open() fails. - make the MPC512x specific probe() time .get_clock() routine not just prepare but enable the clock as well If needed enable the clock, but disable after probe() has finished. - and of course address all the shutdown counter parts of the above setup paths This results in: - specific chip drivers only need to balance their private get and put clock routines which are called from probe and remove, common paths DTRT for all of them Yes, but clock should not stay enabled between probe() and open(). [...] Removing unnecessary devm_put_clk() calls is orthogonal to that. Putting these in isn't totally wrong (they won't harm, and they do signal visual balance more clearly such that the next person won't stop and wonder), but it's true that they are redundant. Trained persons will wonder as much about their presence as untrained persons wonder about their absence. :) Apparently I'm not well trained yet. The whole point about devm_* is to get rid of auto manually tear down functions. So please remove all devm_put_clk() calls, as it will be called automatically if a driver instance is removed. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v3 11/31] net: can: mscan: improve clock API use
the .get_clock() callback is run from probe() and might allocate
resources, introduce a .put_clock() callback that is run from remove()
to undo any allocation activities
use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put
upon driver unload
assume that resources get prepared but not necessarily enabled in the
setup phase, make the open() and close() callbacks of the CAN network
device enable and disable a previously acquired and prepared clock
store pointers to data structures upon successful allocation already
instead of deferral until complete setup, such that subroutines in the
setup sequence may access those data structures as well to track their
resource acquisition
since clock allocation remains optional, the release callback as well as
the enable/disable calls in open/close are optional as well
Signed-off-by: Gerhard Sittig g...@denx.de
---
drivers/net/can/mscan/mpc5xxx_can.c | 18 --
drivers/net/can/mscan/mscan.c |9 +
drivers/net/can/mscan/mscan.h |2 ++
3 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/net/can/mscan/mpc5xxx_can.c
b/drivers/net/can/mscan/mpc5xxx_can.c
index bc422ba..e59b3a3 100644
--- a/drivers/net/can/mscan/mpc5xxx_can.c
+++ b/drivers/net/can/mscan/mpc5xxx_can.c
@@ -40,6 +40,7 @@ struct mpc5xxx_can_data {
unsigned int type;
u32 (*get_clock)(struct platform_device *ofdev, const char *clock_name,
int *mscan_clksrc);
+ void (*put_clock)(struct platform_device *ofdev);
};
#ifdef CONFIG_PPC_MPC52xx
@@ -180,7 +181,7 @@ static u32 mpc512x_can_get_clock(struct platform_device
*ofdev,
clockdiv = 1;
if (!clock_name || !strcmp(clock_name, sys)) {
- sys_clk = clk_get(ofdev-dev, sys_clk);
+ sys_clk = devm_clk_get(ofdev-dev, sys_clk);
if (IS_ERR(sys_clk)) {
dev_err(ofdev-dev, couldn't get sys_clk\n);
goto exit_unmap;
@@ -203,7 +204,7 @@ static u32 mpc512x_can_get_clock(struct platform_device
*ofdev,
}
if (clocksrc 0) {
- ref_clk = clk_get(ofdev-dev, ref_clk);
+ ref_clk = devm_clk_get(ofdev-dev, ref_clk);
if (IS_ERR(ref_clk)) {
dev_err(ofdev-dev, couldn't get ref_clk\n);
goto exit_unmap;
@@ -280,6 +281,8 @@ static int mpc5xxx_can_probe(struct platform_device *ofdev)
dev = alloc_mscandev();
if (!dev)
goto exit_dispose_irq;
+ platform_set_drvdata(ofdev, dev);
+ SET_NETDEV_DEV(dev, ofdev-dev);
priv = netdev_priv(dev);
priv-reg_base = base;
@@ -296,8 +299,6 @@ static int mpc5xxx_can_probe(struct platform_device *ofdev)
goto exit_free_mscan;
}
- SET_NETDEV_DEV(dev, ofdev-dev);
-
err = register_mscandev(dev, mscan_clksrc);
if (err) {
dev_err(ofdev-dev, registering %s failed (err=%d)\n,
@@ -305,8 +306,6 @@ static int mpc5xxx_can_probe(struct platform_device *ofdev)
goto exit_free_mscan;
}
- platform_set_drvdata(ofdev, dev);
-
dev_info(ofdev-dev, MSCAN at 0x%p, irq %d, clock %d Hz\n,
priv-reg_base, dev-irq, priv-can.clock.freq);
@@ -324,10 +323,17 @@ exit_unmap_mem:
static int mpc5xxx_can_remove(struct platform_device *ofdev)
{
+ const struct of_device_id *match;
+ const struct mpc5xxx_can_data *data;
struct net_device *dev = platform_get_drvdata(ofdev);
struct mscan_priv *priv = netdev_priv(dev);
+ match = of_match_device(mpc5xxx_can_table, ofdev-dev);
+ data = match ? match-data : NULL;
+
unregister_mscandev(dev);
+ if (data data-put_clock)
+ data-put_clock(ofdev);
iounmap(priv-reg_base);
irq_dispose_mapping(dev-irq);
free_candev(dev);
diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
index e6b4095..1c08ffe 100644
--- a/drivers/net/can/mscan/mscan.c
+++ b/drivers/net/can/mscan/mscan.c
@@ -573,6 +573,12 @@ static int mscan_open(struct net_device *dev)
struct mscan_priv *priv = netdev_priv(dev);
struct mscan_regs __iomem *regs = priv-reg_base;
+ if (priv-clk_can) {
+ ret = clk_enable(priv-clk_can);
+ if (ret)
+ return ret;
+ }
+
/* common open */
ret = open_candev(dev);
if (ret)
@@ -621,6 +627,9 @@ static int mscan_close(struct net_device *dev)
close_candev(dev);
free_irq(dev-irq, dev);
+ if (priv-clk_can)
+ clk_disable(priv-clk_can);
+
return 0;
}
diff --git a/drivers/net/can/mscan/mscan.h b/drivers/net/can/mscan/mscan.h
index af2ed8b..f32e190 100644
---
Re: [PATCH v3 11/31] net: can: mscan: improve clock API use
On 07/22/2013 02:14 PM, Gerhard Sittig wrote: the .get_clock() callback is run from probe() and might allocate resources, introduce a .put_clock() callback that is run from remove() to undo any allocation activities looks good use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put upon driver unload fine assume that resources get prepared but not necessarily enabled in the setup phase, make the open() and close() callbacks of the CAN network device enable and disable a previously acquired and prepared clock I think you should call prepare_enable and disable_unprepare in the open/close functions. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
