| Message ID | 20180821171635.22740-1-paul@crapouillou.net |
|---|---|
| Headers | show |
| Series | TCU patchset v7 | expand |
Hi Daniel & Thomas, On Tue, Aug 21, 2018 at 07:16:16PM +0200, Paul Cercueil wrote: > This driver handles the TCU (Timer Counter Unit) present on the Ingenic > JZ47xx SoCs, and provides the kernel with a system timer, and optionally > with a clocksource and a sched_clock. > > It also provides clocks and interrupt handling to client drivers. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v2: Use SPDX identifier for the license > > v3: - Move documentation to its own patch > - Search the devicetree for PWM clients, and use all the TCU > channels that won't be used for PWM > > v4: - Add documentation about why we search for PWM clients > - Verify that the PWM clients are for the TCU PWM driver > > v5: Major overhaul. Too many changes to list. Consider it's a new > patch. > > v6: - Add two API functions ingenic_tcu_request_channel and > ingenic_tcu_release_channel. To be used by the PWM driver to > request the use of a TCU channel. The driver will now dynamically > move away the system timer or clocksource to a new TCU channel. > - The system timer now defaults to channel 0, the clocksource now > defaults to channel 1 and is no more optional. The > ingenic,timer-channel and ingenic,clocksource-channel devicetree > properties are now gone. > - Fix round_rate / set_rate not calculating the prescale divider > the same way. This caused problems when (parent_rate / div) would > give a non-integer result. The behaviour is correct now. > - The clocksource clock is turned off on suspend now. > > v7: Fix section mismatch by using builtin_platform_driver_probe() > > drivers/clocksource/Kconfig | 10 + > drivers/clocksource/Makefile | 1 + > drivers/clocksource/ingenic-timer.c | 1124 +++++++++++++++++++++++++++++++++++ > drivers/clocksource/ingenic-timer.h | 15 + > include/linux/mfd/ingenic-tcu.h | 3 + > 5 files changed, 1153 insertions(+) > create mode 100644 drivers/clocksource/ingenic-timer.c > create mode 100644 drivers/clocksource/ingenic-timer.h >% How is this & patch 6 of the series looking to you from a drivers/clocksource perspective? If you're happy with them it'd be great to get an ack so I can take this through the MIPS tree with the rest of the series. The alternative would be to get the drivers in first then the MIPS bits in the next release cycle. Thanks, Paul
Hi Thierry, On Tue, Aug 21, 2018 at 07:16:23PM +0200, Paul Cercueil wrote: > The ingenic-timer "TCU" driver provides us with a regmap, that we can > use to safely access the TCU registers. > > It also provides us with clocks, that can be (un)gated, reparented or > reclocked from devicetree, instead of having these settings hardcoded in > this driver. > > While this driver is devicetree-compatible, it is never (as of now) > probed from devicetree, so this change does not introduce a ABI problem > with current devicetree files. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v5: New patch > > v6: Drop requirement of probing from devicetree > > v7: No change > > drivers/pwm/Kconfig | 2 + > drivers/pwm/pwm-jz4740.c | 124 +++++++++++++++++++++++++++++++---------------- > 2 files changed, 83 insertions(+), 43 deletions(-) How do patches 12-16 of this series look to you from a drivers/pwm perspective? If you're happy with them it'd be great to get an ack so I can take this through the MIPS tree along with the rest of the series. Thanks, Paul
On 28/08/2018 19:23, Paul Burton wrote: > Hi Daniel & Thomas, > > On Tue, Aug 21, 2018 at 07:16:16PM +0200, Paul Cercueil wrote: >> This driver handles the TCU (Timer Counter Unit) present on the Ingenic >> JZ47xx SoCs, and provides the kernel with a system timer, and optionally >> with a clocksource and a sched_clock. >> >> It also provides clocks and interrupt handling to client drivers. >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> >> Notes: >> v2: Use SPDX identifier for the license >> >> v3: - Move documentation to its own patch >> - Search the devicetree for PWM clients, and use all the TCU >> channels that won't be used for PWM >> >> v4: - Add documentation about why we search for PWM clients >> - Verify that the PWM clients are for the TCU PWM driver >> >> v5: Major overhaul. Too many changes to list. Consider it's a new >> patch. >> >> v6: - Add two API functions ingenic_tcu_request_channel and >> ingenic_tcu_release_channel. To be used by the PWM driver to >> request the use of a TCU channel. The driver will now dynamically >> move away the system timer or clocksource to a new TCU channel. >> - The system timer now defaults to channel 0, the clocksource now >> defaults to channel 1 and is no more optional. The >> ingenic,timer-channel and ingenic,clocksource-channel devicetree >> properties are now gone. >> - Fix round_rate / set_rate not calculating the prescale divider >> the same way. This caused problems when (parent_rate / div) would >> give a non-integer result. The behaviour is correct now. >> - The clocksource clock is turned off on suspend now. >> >> v7: Fix section mismatch by using builtin_platform_driver_probe() >> >> drivers/clocksource/Kconfig | 10 + >> drivers/clocksource/Makefile | 1 + >> drivers/clocksource/ingenic-timer.c | 1124 +++++++++++++++++++++++++++++++++++ >> drivers/clocksource/ingenic-timer.h | 15 + >> include/linux/mfd/ingenic-tcu.h | 3 + >> 5 files changed, 1153 insertions(+) >> create mode 100644 drivers/clocksource/ingenic-timer.c >> create mode 100644 drivers/clocksource/ingenic-timer.h >> % > > How is this & patch 6 of the series looking to you from a > drivers/clocksource perspective? The presence of completion, mutexes, etc ... makes me think the driver is not going to the right direction. I have to review the drivers again but it will take some time because I'm returning from vacations and there are a trillion emails to sort out :/ > If you're happy with them it'd be great to get an ack so I can take this > through the MIPS tree with the rest of the series. The alternative would > be to get the drivers in first then the MIPS bits in the next release > cycle. > > Thanks, > Paul >
Hi Daniel, On Wed, Aug 29, 2018 at 11:10:42AM +0200, Daniel Lezcano wrote: > On 28/08/2018 19:23, Paul Burton wrote: > > On Tue, Aug 21, 2018 at 07:16:16PM +0200, Paul Cercueil wrote: > >> This driver handles the TCU (Timer Counter Unit) present on the Ingenic > >> JZ47xx SoCs, and provides the kernel with a system timer, and optionally > >> with a clocksource and a sched_clock. > >> > >> It also provides clocks and interrupt handling to client drivers. > >> > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> > >> --- > >> > >> Notes: > >> v2: Use SPDX identifier for the license > >> > >> v3: - Move documentation to its own patch > >> - Search the devicetree for PWM clients, and use all the TCU > >> channels that won't be used for PWM > >> > >> v4: - Add documentation about why we search for PWM clients > >> - Verify that the PWM clients are for the TCU PWM driver > >> > >> v5: Major overhaul. Too many changes to list. Consider it's a new > >> patch. > >> > >> v6: - Add two API functions ingenic_tcu_request_channel and > >> ingenic_tcu_release_channel. To be used by the PWM driver to > >> request the use of a TCU channel. The driver will now dynamically > >> move away the system timer or clocksource to a new TCU channel. > >> - The system timer now defaults to channel 0, the clocksource now > >> defaults to channel 1 and is no more optional. The > >> ingenic,timer-channel and ingenic,clocksource-channel devicetree > >> properties are now gone. > >> - Fix round_rate / set_rate not calculating the prescale divider > >> the same way. This caused problems when (parent_rate / div) would > >> give a non-integer result. The behaviour is correct now. > >> - The clocksource clock is turned off on suspend now. > >> > >> v7: Fix section mismatch by using builtin_platform_driver_probe() > >> > >> drivers/clocksource/Kconfig | 10 + > >> drivers/clocksource/Makefile | 1 + > >> drivers/clocksource/ingenic-timer.c | 1124 +++++++++++++++++++++++++++++++++++ > >> drivers/clocksource/ingenic-timer.h | 15 + > >> include/linux/mfd/ingenic-tcu.h | 3 + > >> 5 files changed, 1153 insertions(+) > >> create mode 100644 drivers/clocksource/ingenic-timer.c > >> create mode 100644 drivers/clocksource/ingenic-timer.h > >> % > > > > How is this & patch 6 of the series looking to you from a > > drivers/clocksource perspective? > > The presence of completion, mutexes, etc ... makes me think the driver > is not going to the right direction. > > I have to review the drivers again but it will take some time because > I'm returning from vacations and there are a trillion emails to sort out :/ OK no problem, thanks for the heads up! Paul
On Tue, 21 Aug 2018, Paul Cercueil wrote: > This header contains macros for the registers that are present in the > regmap shared by all the drivers related to the TCU (Timer Counter Unit) > of the Ingenic JZ47xx SoCs. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > Acked-by: Lee Jones <lee.jones@linaro.org> > --- > > Notes: > v2: Use SPDX identifier for the license > > v3: - Use macros instead of enum > - Add 'TCU_' at the beginning of each macro > - Remove useless include <linux/regmap.h> > > v4: No change > > v5: No change > > v6: Rename barrier macro from __LINUX_CLK_INGENIC_TCU_H__ to > __LINUX_MFD_INGENIC_TCU_H__ > > v7: No change > > include/linux/mfd/ingenic-tcu.h | 56 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > create mode 100644 include/linux/mfd/ingenic-tcu.h Applied, thanks.
On 21/08/2018 19:16, Paul Cercueil wrote: > This driver handles the TCU (Timer Counter Unit) present on the Ingenic > JZ47xx SoCs, and provides the kernel with a system timer, and optionally > with a clocksource and a sched_clock. > > It also provides clocks and interrupt handling to client drivers. Can you provide a much more complete description of the timer in order to make my life easier for the review of this patch? Thanks -- Daniel
On Tue, Aug 21, 2018 at 07:16:23PM +0200, Paul Cercueil wrote: > The ingenic-timer "TCU" driver provides us with a regmap, that we can > use to safely access the TCU registers. > > It also provides us with clocks, that can be (un)gated, reparented or > reclocked from devicetree, instead of having these settings hardcoded in > this driver. > > While this driver is devicetree-compatible, it is never (as of now) > probed from devicetree, so this change does not introduce a ABI problem > with current devicetree files. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v5: New patch > > v6: Drop requirement of probing from devicetree > > v7: No change > > drivers/pwm/Kconfig | 2 + > drivers/pwm/pwm-jz4740.c | 124 +++++++++++++++++++++++++++++++---------------- > 2 files changed, 83 insertions(+), 43 deletions(-) > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index a4d262db9945..58359bf22b96 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -202,6 +202,8 @@ config PWM_IMX > config PWM_JZ4740 > tristate "Ingenic JZ47xx PWM support" > depends on MACH_INGENIC > + depends on COMMON_CLK > + select INGENIC_TIMER > help > Generic PWM framework driver for Ingenic JZ47xx based > machines. > diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c > index a7b134af5e04..1bda8d8e9865 100644 > --- a/drivers/pwm/pwm-jz4740.c > +++ b/drivers/pwm/pwm-jz4740.c > @@ -14,21 +14,21 @@ > */ > > #include <linux/clk.h> > +#include <linux/clk-provider.h> Why do you need this? Thierry
On Tue, Aug 21, 2018 at 07:16:24PM +0200, Paul Cercueil wrote: > The TCU channels 0 and 1 were previously reserved for system tasks, and > thus unavailable for PWM. > > This commit uses the newly introduced API functions of the ingenic-timer > driver to request/release the TCU channels that should be used as PWM. > This allows all the TCU channels to be used as PWM. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v6: New patch > > v7: No change > > drivers/pwm/pwm-jz4740.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) Acked-by: Thierry Reding <treding@nvidia.com>
On Tue, Aug 21, 2018 at 07:16:25PM +0200, Paul Cercueil wrote: > Depending on MACH_INGENIC prevent us from creating a generic kernel that > works on more than one MIPS board. Instead, we just depend on MIPS being > set. > > On other architectures, this driver can still be built, thanks to > COMPILE_TEST. This is used by automated tools to find bugs, for > instance. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v5: New patch > > v6: No change > > v7: No change > > drivers/pwm/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Thierry Reding <treding@nvidia.com>
On Tue, Aug 21, 2018 at 07:16:26PM +0200, Paul Cercueil wrote: > Right now none of the Ingenic-based boards probe this driver from > devicetree. This driver defined three compatible strings for the exact > same behaviour. Before these strings are used, we can remove two of > them. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v5: New patch > > v6: No change > > v7: No change > > drivers/pwm/pwm-jz4740.c | 2 -- > 1 file changed, 2 deletions(-) Acked-by: Thierry Reding <thierry.reding@gmail.com>
On Tue, Aug 21, 2018 at 07:16:27PM +0200, Paul Cercueil wrote: > The PWM in the JZ4725B works the same as in the JZ4740, except that it > only has 6 channels available instead of 8. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v5: New patch > > v6: - Move of_device_id structure back at the bottom (less noise in > patch) > - Use device_get_match_data() instead of of_* variant > > v7: No change > > drivers/pwm/pwm-jz4740.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) Acked-by: Thierry Reding <thierry.reding@gmail.com>
On Fri, Oct 12, 2018 at 12:40:53PM +0200, Thierry Reding wrote: > On Tue, Aug 21, 2018 at 07:16:24PM +0200, Paul Cercueil wrote: > > The TCU channels 0 and 1 were previously reserved for system tasks, and > > thus unavailable for PWM. > > > > This commit uses the newly introduced API functions of the ingenic-timer > > driver to request/release the TCU channels that should be used as PWM. > > This allows all the TCU channels to be used as PWM. > > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > --- > > > > Notes: > > v6: New patch > > > > v7: No change > > > > drivers/pwm/pwm-jz4740.c | 28 ++++++++++++++++------------ > > 1 file changed, 16 insertions(+), 12 deletions(-) > > Acked-by: Thierry Reding <treding@nvidia.com> Technically this should be: Acked-by: Thierry Reding <thierry.reding@gmail.com>
On Fri, Oct 12, 2018 at 12:41:50PM +0200, Thierry Reding wrote: > On Tue, Aug 21, 2018 at 07:16:25PM +0200, Paul Cercueil wrote: > > Depending on MACH_INGENIC prevent us from creating a generic kernel that > > works on more than one MIPS board. Instead, we just depend on MIPS being > > set. > > > > On other architectures, this driver can still be built, thanks to > > COMPILE_TEST. This is used by automated tools to find bugs, for > > instance. > > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > --- > > > > Notes: > > v5: New patch > > > > v6: No change > > > > v7: No change > > > > drivers/pwm/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Acked-by: Thierry Reding <treding@nvidia.com> Again, technically: Acked-by: Thierry Reding <thierry.reding@gmail.com>
Hi, This is the v7 of my patchset that adds support for the Timer/Counter Unit in Ingenic JZ47xx SoCs. >From v6, only patches [05/24] and [06/24] were modified to fix a section mismatch. All the other patches are the same. Thanks, -Paul Cercueil