| Message ID | 1374495298-22019-12-git-send-email-gsi@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
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
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
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
[ 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 platforms which is full of callbacks. That's just out of the scope of the series. It may be desirable to address this issue as well, but it shall be done in a separate action, not now "in bypassing". Thank you for understanding. :) And I do appreciate your feedback and desire for even better drivers, just disagree on what to do now in this very moment. What we already have is: - probe() and remove() for the driver, calling into clock setup and allocation and deallocation routines - no allocation for MPC52xx and thus no deallocation, keeping the status of how things used to be - allocation of a "can" clock for MPC512x and the respective deallocation - open() and close() for the network device, which prepare/enable and disable/unprepare the allocated clocks What I will add is: - allocation and release of both the 'ipg' and a "can" clock for the MPC512x case - handling of all allocated clocks in open() and close() (such that no assumption is made what occurs at probe() and remove() time) - permanently enabled 'ipg' clock when allocated, such that the driver may happily access the controller's registers and may assume things remain there - usually disabled 'can' clock, but enable/disable between open() and close() This shall result in: - no change in behaviour for MPC52xx - disabled clocks and hardware for MPC512x when CAN isn't probed (not listed in the device tree, or disabled) - enabled internal peripherals but wire disabled for MPC512x when CAN is probed and attached but not in use - only enabled wire related clock when the network device is open and in actual use The above goal of the next update in the series won't break any operation of peripherals, will be a clear improvement in that the driver finally does properly use the clock API, and will result in an appropriate use of hardware. There may be potential to conserve even more power, but it's not essential given the previous status of the driver, and it's out of the scope for the very series we are talking about. > > 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(). For this one I offered the compromise of only enabling the "can" clock during network device operation, but keeping the 'ipg' clock active over the driver's complete attachment period. This shall be acceptable. Anything else can be done later and independently. > [...] > > > 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. Ah, yes, the devm_*() feedback was processed as well. Release won't occur explicitly, as it's done implicitly by common code. virtually yours Gerhard Sittig
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 --- a/drivers/net/can/mscan/mscan.h +++ b/drivers/net/can/mscan/mscan.h @@ -21,6 +21,7 @@ #ifndef __MSCAN_H__ #define __MSCAN_H__ +#include <linux/clk.h> #include <linux/types.h> /* MSCAN control register 0 (CANCTL0) bits */ @@ -283,6 +284,7 @@ struct mscan_priv { unsigned int type; /* MSCAN type variants */ unsigned long flags; void __iomem *reg_base; /* ioremap'ed address to registers */ + struct clk *clk_can; /* clock for registers or bitrates */ u8 shadow_statflg; u8 shadow_canrier; u8 cur_pri;
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 <gsi@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(-)