Re: [PATCH v4 04/14] mfd: Add Samsung PWM/timer master driver
On Thu, Apr 11, 2013 at 12:35:47AM +0200, Arnd Bergmann wrote: Sorry for not replying earlier. My idea for the register level interface was to create a platform_device for each PWM, e.g. using the mfd_cell infrastructure. You can then pass a struct regmap as the platform data for each child of the timer node, and all the DT handling code can stay in the parent driver. Dunno if it helps or not but there's also dev_get_regmap() if you're passing the struct device around or can fish one out of thhe parent. signature.asc Description: Digital signature
Re: [PATCH v4 04/14] mfd: Add Samsung PWM/timer master driver
On Thursday 11 of April 2013 00:35:47 Arnd Bergmann wrote: On Monday 08 April 2013, Tomasz Figa wrote: On Saturday 06 of April 2013 00:24:18 Tomasz Figa wrote: On Friday 05 of April 2013 21:54:21 Arnd Bergmann wrote: On Friday 05 April 2013, Tomasz Figa wrote: I'm not sure what you mean by a register-level interface. Something like samsung_pwm_update_reg(reg, mask, val), which modifies bitfields according to the mask and value with appropriate synchronization? If yes, this solves only the problem of access to shared registers. The other problems that remain: - negotiation of PWM channels to use for clock source and clock events, because each board can use different channels for PWM outputs, - code duplication caused by both of the drivers doing mostly the same things and or having to parse the same data from device tree, - both non-DT and DT platforms must be supported, - how to keep the ability to load PWM driver as a module (or not load it at all when PWM is not used on particular board), while retaining everything that is needed for the clocksource driver in kernel, - some platforms can't use PWM timers as system clocksources, while on others this is the only timekeeping hardware available. Hmm. Does anybody have an idea of a better way of implementing this PWM timer support, which solves the above problems? This series is a dependency for moving Universal C210 board to DT-based description and it's already almost out of time to get this included for 3.10... Sorry for not replying earlier. My idea for the register level interface was to create a platform_device for each PWM, e.g. using the mfd_cell infrastructure. You can then pass a struct regmap as the platform data for each child of the timer node, and all the DT handling code can stay in the parent driver. Hmm. As I said, I'm completely fine with using a regmap for sharing registers between subdrivers. However the clocksource can not be registered as an MFD cell, because it's needed very early. I can imagine a solution alternative to my original one, where the MFD cells would be registered from the clocksource driver. This would mean that platforms that don't need (and can't use) the PWM clocksource would have to enable the driver anyway. Another thing is that I don't see a need to create one cell per PWM channel. The PWM core is designed in a way that supports multiple channels per PWM controller and so is the generic PWM DT specifier (controller channel period flags), so I'd rather see a single cell for all PWM channels. So, to summarize this alternative concept: - two drivers: 1) clocksource driver - registering clocksource and PWM MFD cell 2) PWM driver - handling the PWM MFD cell - both drivers would share registers using a regmap with custom lock/unlock callbacks (using spin_lock_irqsave/spin_unlock_irqrestore, because the clocksource needs to access PWM registers in IRQ context) - the clocksource/master driver would have a samsung_time_init function which would set up the driver early and initialize the clocksource - a platform driver would be registered by the clocksource/master driver which would register the PWM MFD cell in its probe - MFD cell platform data would contain struct regmap * and variant data (parsed from DT or received from platform code - as in my original version) This should indeed work, assuming that I find a solution to make clocksource_of_init not initialize the PWM clocksource (from PWM DT node) on platforms that can't use it. Best regards, -- Tomasz Figa Samsung Poland RD Center SW Solution Development, Kernel and System Framework -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] mfd: Add Samsung PWM/timer master driver
On Thursday 11 April 2013, Tomasz Figa wrote: On Thursday 11 of April 2013 00:35:47 Arnd Bergmann wrote: On Monday 08 April 2013, Tomasz Figa wrote: On Saturday 06 of April 2013 00:24:18 Tomasz Figa wrote: On Friday 05 of April 2013 21:54:21 Arnd Bergmann wrote: On Friday 05 April 2013, Tomasz Figa wrote: Sorry for not replying earlier. My idea for the register level interface was to create a platform_device for each PWM, e.g. using the mfd_cell infrastructure. You can then pass a struct regmap as the platform data for each child of the timer node, and all the DT handling code can stay in the parent driver. Hmm. As I said, I'm completely fine with using a regmap for sharing registers between subdrivers. However the clocksource can not be registered as an MFD cell, because it's needed very early. I can imagine a solution alternative to my original one, where the MFD cells would be registered from the clocksource driver. This would mean that platforms that don't need (and can't use) the PWM clocksource would have to enable the driver anyway. Sounds ok to me. The main part I want to avoid is having two separate device nodes in the DT for one physical device, but there are multiple ways to get there. Another thing is that I don't see a need to create one cell per PWM channel. The PWM core is designed in a way that supports multiple channels per PWM controller and so is the generic PWM DT specifier (controller channel period flags), so I'd rather see a single cell for all PWM channels. Sure, no problem with that. So, to summarize this alternative concept: - two drivers: 1) clocksource driver - registering clocksource and PWM MFD cell 2) PWM driver - handling the PWM MFD cell Ok. Don't be too tied to the MFD concept though if it causes problems. In many cases calling platform_device_register_resndata or similar is actually easier than MFD, especially if you only want a single child device. Either way is fine though. - both drivers would share registers using a regmap with custom lock/unlock callbacks (using spin_lock_irqsave/spin_unlock_irqrestore, because the clocksource needs to access PWM registers in IRQ context) Ok. - the clocksource/master driver would have a samsung_time_init function which would set up the driver early and initialize the clocksource Right. If you want to use the clocksource_of_init() infratstructure for this (which is probably a good idea), please base your series on top of the clksrc/cleanup branch in arm-soc. - a platform driver would be registered by the clocksource/master driver which would register the PWM MFD cell in its probe. This seems unnecessary, if the only purpose of the platform driver is to export the MFD cell. In that case, I would actually suggest a simpler model where the PWM driver registers the main platform driver directly and calls a function exported by the clocksource driver to get the regmap structure and anything else it may need (possibly initializing the driver in the case where it is not used as the clocksource but only provides the registers for PWM). - MFD cell platform data would contain struct regmap * and variant data (parsed from DT or received from platform code - as in my original version) Yes, either the MFD cell, or the exported symbol, whichever ends up easier. This should indeed work, assuming that I find a solution to make clocksource_of_init not initialize the PWM clocksource (from PWM DT node) on platforms that can't use it. Right. If anything else comes up, I'd suggest we discuss it on IRC (#armlinux on irc.freenode.net) tomorrow for faster round-trip, or I can call you on the phone if you like. I'm available all day tomorrow, just send me your (landline) phone number by private email. Arnd. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] mfd: Add Samsung PWM/timer master driver
On Monday 08 April 2013, Tomasz Figa wrote: On Saturday 06 of April 2013 00:24:18 Tomasz Figa wrote: On Friday 05 of April 2013 21:54:21 Arnd Bergmann wrote: On Friday 05 April 2013, Tomasz Figa wrote: I'm not sure what you mean by a register-level interface. Something like samsung_pwm_update_reg(reg, mask, val), which modifies bitfields according to the mask and value with appropriate synchronization? If yes, this solves only the problem of access to shared registers. The other problems that remain: - negotiation of PWM channels to use for clock source and clock events, because each board can use different channels for PWM outputs, - code duplication caused by both of the drivers doing mostly the same things and or having to parse the same data from device tree, - both non-DT and DT platforms must be supported, - how to keep the ability to load PWM driver as a module (or not load it at all when PWM is not used on particular board), while retaining everything that is needed for the clocksource driver in kernel, - some platforms can't use PWM timers as system clocksources, while on others this is the only timekeeping hardware available. Hmm. Does anybody have an idea of a better way of implementing this PWM timer support, which solves the above problems? This series is a dependency for moving Universal C210 board to DT-based description and it's already almost out of time to get this included for 3.10... Sorry for not replying earlier. My idea for the register level interface was to create a platform_device for each PWM, e.g. using the mfd_cell infrastructure. You can then pass a struct regmap as the platform data for each child of the timer node, and all the DT handling code can stay in the parent driver. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] mfd: Add Samsung PWM/timer master driver
On Saturday 06 of April 2013 00:24:18 Tomasz Figa wrote: On Friday 05 of April 2013 21:54:21 Arnd Bergmann wrote: On Friday 05 April 2013, Tomasz Figa wrote: I don't think anyone ever suggested using a private API though. I must have gotten the last paragraph of http://article.gmane.org/gmane.linux.kernel.samsung-soc/16638 wrong then. For me it seemed like the timer driver would expose a private API to the PWM driver. Yes, sorry I wasn't clear enough. I meant a register-level interface exposed from the base driver, not a high-level interface like you did. I'm not sure what you mean by a register-level interface. Something like samsung_pwm_update_reg(reg, mask, val), which modifies bitfields according to the mask and value with appropriate synchronization? If yes, this solves only the problem of access to shared registers. The other problems that remain: - negotiation of PWM channels to use for clock source and clock events, because each board can use different channels for PWM outputs, - code duplication caused by both of the drivers doing mostly the same things and or having to parse the same data from device tree, - both non-DT and DT platforms must be supported, - how to keep the ability to load PWM driver as a module (or not load it at all when PWM is not used on particular board), while retaining everything that is needed for the clocksource driver in kernel, - some platforms can't use PWM timers as system clocksources, while on others this is the only timekeeping hardware available. Hmm. Does anybody have an idea of a better way of implementing this PWM timer support, which solves the above problems? This series is a dependency for moving Universal C210 board to DT-based description and it's already almost out of time to get this included for 3.10... Best regards, Tomasz I think it's ok if the driver lives in drivers/mfd when it doesn't fit anywhere else easily, but you should really register it to the pwm subsystem to expose that functionality, not export functions that can be used by a pwm shim driver, which even seems to be missing from the series. Anyway, using PWM API in a clocksource driver that needs to be running very early (before any initcalls get called) seems a bit weird for me, especially when PWM API doesn't provide such fine grained control over PWM timers that is required in the clocksource drivers. Exactly, that's why you should have a regular PWM driver that gets loaded at a convenient time and just uses an interface exported by the base driver to access the common registers. Well, this is basically what my patches do, except that the PWM driver isn't reworked to use the base driver yet. The private API exported to the samsung-time and pwm-samsung drivers isn't maybe the most fortunate one, but it clearly has the advantage of solving all the problems I mentioned before. Same goes for calling this an MFD driver. It doesn't even use the MFD core, but there seems to be no better place to put it. Maybe drivers/platform/arm would be better, if it existed, just as currently available drivers/platform/x86? Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] mfd: Add Samsung PWM/timer master driver
Hi Tomasz, On Thu, Apr 04, 2013 at 06:37:01PM +0200, Tomasz Figa wrote: This patch adds master driver for PWM/timer block available on many Samsung SoCs providing clocksource and PWM output capabilities. Signed-off-by: Tomasz Figa t.f...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- .../devicetree/bindings/pwm/pwm-samsung.txt| 37 ++ drivers/clocksource/Kconfig| 1 + drivers/mfd/Kconfig| 3 + drivers/mfd/Makefile | 1 + drivers/mfd/samsung-pwm.c | 439 + drivers/pwm/Kconfig| 1 + include/linux/mfd/samsung-pwm.h| 49 +++ include/linux/platform_data/samsung-pwm.h | 28 ++ 8 files changed, 559 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-samsung.txt create mode 100644 drivers/mfd/samsung-pwm.c create mode 100644 include/linux/mfd/samsung-pwm.h create mode 100644 include/linux/platform_data/samsung-pwm.h Why is that an MFD driver, and why aren't you using the PWM APIs for it ? Also, you probably want to look at using the regmap APIs for your IO. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] mfd: Add Samsung PWM/timer master driver
Hi Samuel, On Friday 05 of April 2013 18:39:58 Samuel Ortiz wrote: Hi Tomasz, On Thu, Apr 04, 2013 at 06:37:01PM +0200, Tomasz Figa wrote: This patch adds master driver for PWM/timer block available on many Samsung SoCs providing clocksource and PWM output capabilities. Signed-off-by: Tomasz Figa t.f...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- .../devicetree/bindings/pwm/pwm-samsung.txt| 37 ++ drivers/clocksource/Kconfig| 1 + drivers/mfd/Kconfig| 3 + drivers/mfd/Makefile | 1 + drivers/mfd/samsung-pwm.c | 439 + drivers/pwm/Kconfig | 1 + include/linux/mfd/samsung-pwm.h| 49 +++ include/linux/platform_data/samsung-pwm.h | 28 ++ 8 files changed, 559 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-samsung.txt create mode 100644 drivers/mfd/samsung-pwm.c create mode 100644 include/linux/mfd/samsung-pwm.h create mode 100644 include/linux/platform_data/samsung-pwm.h Why is that an MFD driver, and why aren't you using the PWM APIs for it ? Also, you probably want to look at using the regmap APIs for your IO. The case of Samsung PWM timers is rather complicated. It is a hardware block that can be used at the same time to generate PWM signal and as a system clocksource. There was a discussion on how to solve the problem of sharing the hardware: http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=16500 (see the linked post and replies to it). Best regards, -- Tomasz Figa Samsung Poland RD Center SW Solution Development, Kernel and System Framework -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] mfd: Add Samsung PWM/timer master driver
On Friday 05 April 2013, Tomasz Figa wrote: I don't think anyone ever suggested using a private API though. I must have gotten the last paragraph of http://article.gmane.org/gmane.linux.kernel.samsung-soc/16638 wrong then. For me it seemed like the timer driver would expose a private API to the PWM driver. Yes, sorry I wasn't clear enough. I meant a register-level interface exposed from the base driver, not a high-level interface like you did. I think it's ok if the driver lives in drivers/mfd when it doesn't fit anywhere else easily, but you should really register it to the pwm subsystem to expose that functionality, not export functions that can be used by a pwm shim driver, which even seems to be missing from the series. Anyway, using PWM API in a clocksource driver that needs to be running very early (before any initcalls get called) seems a bit weird for me, especially when PWM API doesn't provide such fine grained control over PWM timers that is required in the clocksource drivers. Exactly, that's why you should have a regular PWM driver that gets loaded at a convenient time and just uses an interface exported by the base driver to access the common registers. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] mfd: Add Samsung PWM/timer master driver
On Friday 05 of April 2013 21:54:21 Arnd Bergmann wrote: On Friday 05 April 2013, Tomasz Figa wrote: I don't think anyone ever suggested using a private API though. I must have gotten the last paragraph of http://article.gmane.org/gmane.linux.kernel.samsung-soc/16638 wrong then. For me it seemed like the timer driver would expose a private API to the PWM driver. Yes, sorry I wasn't clear enough. I meant a register-level interface exposed from the base driver, not a high-level interface like you did. I'm not sure what you mean by a register-level interface. Something like samsung_pwm_update_reg(reg, mask, val), which modifies bitfields according to the mask and value with appropriate synchronization? If yes, this solves only the problem of access to shared registers. The other problems that remain: - negotiation of PWM channels to use for clock source and clock events, because each board can use different channels for PWM outputs, - code duplication caused by both of the drivers doing mostly the same things and or having to parse the same data from device tree, - both non-DT and DT platforms must be supported, - how to keep the ability to load PWM driver as a module (or not load it at all when PWM is not used on particular board), while retaining everything that is needed for the clocksource driver in kernel, - some platforms can't use PWM timers as system clocksources, while on others this is the only timekeeping hardware available. I think it's ok if the driver lives in drivers/mfd when it doesn't fit anywhere else easily, but you should really register it to the pwm subsystem to expose that functionality, not export functions that can be used by a pwm shim driver, which even seems to be missing from the series. Anyway, using PWM API in a clocksource driver that needs to be running very early (before any initcalls get called) seems a bit weird for me, especially when PWM API doesn't provide such fine grained control over PWM timers that is required in the clocksource drivers. Exactly, that's why you should have a regular PWM driver that gets loaded at a convenient time and just uses an interface exported by the base driver to access the common registers. Well, this is basically what my patches do, except that the PWM driver isn't reworked to use the base driver yet. The private API exported to the samsung-time and pwm-samsung drivers isn't maybe the most fortunate one, but it clearly has the advantage of solving all the problems I mentioned before. Same goes for calling this an MFD driver. It doesn't even use the MFD core, but there seems to be no better place to put it. Maybe drivers/platform/arm would be better, if it existed, just as currently available drivers/platform/x86? Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
