Hi Lee Jones,
Thanks for the review.
> Subject: Re: [PATCH v13 2/6] mfd: Add Renesas RZ/G2L MTU3a core driver
>
> On Thu, 16 Feb 2023, Biju Das wrote:
>
> > The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded in
> > the Renesas RZ/G2L family SoCs. It consists of eight 16-bit timer
> > channels and one 32-bit timer channel. It supports the following
> > functions
> > - Counter
> > - Timer
> > - PWM
> >
> > The 8/16/32 bit registers are mixed in each channel.
> >
> > Add MTU3a core driver for RZ/G2L SoC. The core driver shares the clk
> > and channel register access for the other child devices like Counter,
> > PWM and Clock event.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > Ref:
> >
> >
> > v12->v13:
> > * Moved RZ_MTU3_TMDR1_* macros from pwm driver to rz-mtu3.h.
> > v11->v2:
> > * Moved the core driver from timer to MFD.
> > * Moved header fine from clocksource/rz-mtu3.h->linux/mfd/rz-mtu3.h
> > * Removed Select MFD_CORE option from config.
> > v10->v11:
> > * No change.
> > v9->v10:
> > * No change.
> > v8->v9:
> > * No change.
> > v7->v8:
> > * Add locking for RMW on rz_mtu3_shared_reg_update_bit()
> > * Replaced enum rz_mtu3_functions with channel busy flag
> > * Added API for request and release a channel.
> > v6->v7:
> > * Added channel specific mutex to avoid races between child devices
> > (for eg: pwm and counter)
> > * Added rz_mtu3_shared_reg_update_bit() to update bit.
> > v5->v6:
> > * Updated commit and KConfig description
> > * Selected MFD_CORE to avoid build error if CONFIG_MFD_CORE not set.
> > * Improved error handling in probe().
> > * Updated MODULE_DESCRIPTION and title.
> > v4->v5:
> > * Moved core driver from MFD to timer
> > * Child devices instatiated using mfd_add_devices()
> > v3->v4:
> > * A single driver that registers both the counter and the pwm
> functionalities
> > that binds against "renesas,rz-mtu3".
> > * Moved PM handling from child devices to here.
> > * replaced include/linux/mfd/rz-mtu3.h->drivers/mfd/rz-mtu3.h
> > * Removed "remove" callback
> > v2->v3:
> > * removed unwanted header files
> > * Added LUT for 32 bit registers as it needed for 32-bit cascade
> counting.
> > * Exported 32 bit read/write functions.
> > v1->v2:
> > * Changed the compatible name
> > * Replaced devm_reset_control_get->devm_reset_control_get_exclusive
> > * Renamed function names rzg2l_mtu3->rz_mtu3 as this is generic IP
> > in RZ family SoC's.
> > ---
> > drivers/mfd/Kconfig | 10 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/rz-mtu3.c | 458 ++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/rz-mtu3.h | 243 +++++++++++++++++++
> > 4 files changed, 712 insertions(+)
> > create mode 100644 drivers/mfd/rz-mtu3.c create mode 100644
> > include/linux/mfd/rz-mtu3.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > fcc141e067b9..e16c550c5b05 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1308,6 +1308,16 @@ config MFD_SC27XX_PMIC
> > This driver provides common support for accessing the SC27xx PMICs,
> > and it also adds the irq_chip parts for handling the PMIC chip
> events.
> >
> > +config RZ_MTU3
> > + bool "Renesas RZ/G2L MTU3a core driver"
> > + depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> > + help
> > + Select this option to enable Renesas RZ/G2L MTU3a core driver for
> > + the Multi-Function Timer Pulse Unit 3 (MTU3a) hardware available
> > + on SoCs from Renesas. The core driver shares the clk and channel
> > + register access for the other child devices like Counter, PWM,
> > + Clock Source, and Clock event.
> > +
> > config ABX500_CORE
> > bool "ST-Ericsson ABX500 Mixed Signal Circuit register functions"
> > depends on ARCH_U8500 || COMPILE_TEST diff --git
> > a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > 2f6c89d1e277..1d2392f06f78 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -174,6 +174,7 @@ pcf50633-objs := pcf50633-core.o
> pcf50633-irq.o
> > obj-$(CONFIG_MFD_PCF50633) += pcf50633.o
> > obj-$(CONFIG_PCF50633_ADC) += pcf50633-adc.o
> > obj-$(CONFIG_PCF50633_GPIO) += pcf50633-gpio.o
> > +obj-$(CONFIG_RZ_MTU3) += rz-mtu3.o
> > obj-$(CONFIG_ABX500_CORE) += abx500-core.o
> > obj-$(CONFIG_MFD_DB8500_PRCMU) += db8500-prcmu.o
> > # ab8500-core need to come after db8500-prcmu (which provides the
> > channel) diff --git a/drivers/mfd/rz-mtu3.c b/drivers/mfd/rz-mtu3.c
> > new file mode 100644 index 000000000000..ad6bb6b28694
> > --- /dev/null
> > +++ b/drivers/mfd/rz-mtu3.c
> > @@ -0,0 +1,458 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L Multi-Function Timer Pulse Unit 3(MTU3a) Core
> > +driver
> > + *
> > + * Copyright (C) 2023 Renesas Electronics Corporation */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/rz-mtu3.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/reset.h>
> > +#include <linux/spinlock.h>
> > +
> > +static const unsigned long rz_mtu3_8bit_ch_reg_offs[][13] = {
> > + {
> > + [RZ_MTU3_TIER] = 0x4, [RZ_MTU3_NFCR] = 0x70,
> > + [RZ_MTU3_TCR] = 0x0, [RZ_MTU3_TCR2] = 0x28,
> > + [RZ_MTU3_TMDR1] = 0x1, [RZ_MTU3_TIORH] = 0x2,
> > + [RZ_MTU3_TIORL] = 0x3
>
> Instead of having this huge, fragile (ordered) slab list (which I will
> request to be amended to a single entry per line), please consider
> implementing a (3?) few helper MACROS, stored in a header, along the lines
> of:
>
> MTU_8BIT_<BLAH>(_tier, _nfcr, ... <blah>) \
> {
> [RZ_MTU3_TIER] = _tier,
> [RZ_MTU3_NFCR] = _nfcr,
> <blah>
> }
>
> Then in here, do:
>
> [MTU3_CHAN_x] = MTU_8BIT_<BLAH>(0x04, 0x70 ... <blah>)
>
> Notice how we're being explicit about the which values associate with which
> channel here too. This eradicates the fragility of an ordered structure.
OK, for 8/16 bits read/write this looks good.
static const unsigned long rz_mtu3_8bit_ch_reg_offs[][13] = {
[MTU3_CHAN_0] = MTU_8BIT_CH_0(0x04, 0x70, 0x00, 0x28, 0x01, 0x02, 0x03, 0x26),
[MTU3_CHAN_1] = MTU_8BIT_CH_1_2(0x04, 0xef, 0x05, 0x00, 0x14, 0x01, 0x02),
[MTU3_CHAN_2] = MTU_8BIT_CH_1_2(0x04, 0x16e, 0x05, 0x00, 0x0c, 0x01, 0x02),
[MTU3_CHAN_3] = MTU_8BIT_CH_3_4_6_7(0x08, 0x93, 0x2c, 0x00, 0x4c, 0x02, 0x04, 0x05, 0x38),
[MTU3_CHAN_4] = MTU_8BIT_CH_3_4_6_7(0x08, 0x93, 0x2c, 0x00, 0x4c, 0x02, 0x05, 0x06, 0x38),
[MTU3_CHAN_5] = MTU_8BIT_CH_5(0x32, 0x1eb, 0x34, 0x36, 0x04, 0x05, 0x06, 0x14, 0x15, 0x16, 0x24, 0x25, 0x26),
[MTU3_CHAN_6] = MTU_8BIT_CH_3_4_6_7(0x08, 0x93, 0x2c, 0x00, 0x4c, 0x02, 0x04, 0x05, 0x38),
[MTU3_CHAN_7] = MTU_8BIT_CH_3_4_6_7(0x08, 0x93, 0x2c, 0x00, 0x4c, 0x02, 0x05, 0x06, 0x38),
[MTU3_CHAN_8] = MTU_8BIT_CH_8(0x04, 0x368, 0x00, 0x06, 0x01, 0x02, 0x03)
};
static const unsigned long rz_mtu3_16bit_ch_reg_offs[][12] = {
[MTU3_CHAN_0] = MTU_16BIT_CH_0(0x6, 0x8, 0xa, 0xc, 0xe, 0x20, 0x22),
[MTU3_CHAN_1] = MTU_16BIT_CH_1_2(0x6, 0x8, 0xa),
[MTU3_CHAN_2] = MTU_16BIT_CH_1_2(0x6, 0x8, 0xa),
[MTU3_CHAN_3] = MTU_16BIT_CH_3_6(0x10, 0x18, 0x1a, 0x24, 0x26, 0x72),
[MTU3_CHAN_4] = MTU_16BIT_CH_4_7(0x11, 0x1b, 0x1d, 0x27, 0x29, 0x73, 0x75, 0x3f, 0x43, 0x45, 0x47, 0x49),
[MTU3_CHAN_5] = MTU_16BIT_CH_5(0x0, 0x2, 0x10, 0x12, 0x20, 0x22),
[MTU3_CHAN_6] = MTU_16BIT_CH_3_6(0x10, 0x18, 0x1a, 0x24, 0x26, 0x72),
[MTU3_CHAN_7] = MTU_16BIT_CH_4_7(0x11, 0x1b, 0x1d, 0x27, 0x29, 0x73, 0x75, 0x3f, 0x43, 0x45, 0x47, 0x49),
};
>
> > + },
> > + {
> > + [RZ_MTU3_TIER] = 0x4, [RZ_MTU3_NFCR] = 0xef,
> > + [RZ_MTU3_TSR] = 0x5, [RZ_MTU3_TCR] = 0x0,
> > + [RZ_MTU3_TCR2] = 0x14, [RZ_MTU3_TMDR1] = 0x1,
> > + [RZ_MTU3_TIOR] = 0x2
> > + },
> > + {
> > + [RZ_MTU3_TIER] = 0x4, [RZ_MTU3_NFCR] = 0x16e,
> > + [RZ_MTU3_TSR] = 0x5, [RZ_MTU3_TCR] = 0x0,
> > + [RZ_MTU3_TCR2] = 0xc, [RZ_MTU3_TMDR1] = 0x1,
> > + [RZ_MTU3_TIOR] = 0x2
> > + },
> > + {
> > + [RZ_MTU3_TIER] = 0x8, [RZ_MTU3_NFCR] = 0x93,
> > + [RZ_MTU3_TSR] = 0x2c, [RZ_MTU3_TCR] = 0x0,
> > + [RZ_MTU3_TCR2] = 0x4c, [RZ_MTU3_TMDR1] = 0x2,
> > + [RZ_MTU3_TIORH] = 0x4, [RZ_MTU3_TIORL] = 0x5,
> > + [RZ_MTU3_TBTM] = 0x38
> > + },
> > + {
> > + [RZ_MTU3_TIER] = 0x8, [RZ_MTU3_NFCR] = 0x93,
> > + [RZ_MTU3_TSR] = 0x2c, [RZ_MTU3_TCR] = 0x0,
> > + [RZ_MTU3_TCR2] = 0x4c, [RZ_MTU3_TMDR1] = 0x2,
> > + [RZ_MTU3_TIORH] = 0x5, [RZ_MTU3_TIORL] = 0x6,
> > + [RZ_MTU3_TBTM] = 0x38
> > + },
> > + {
> > + [RZ_MTU3_TIER] = 0x32, [RZ_MTU3_NFCR] = 0x1eb,
> > + [RZ_MTU3_TSTR] = 0x34, [RZ_MTU3_TCNTCMPCLR] = 0x36,
> > + [RZ_MTU3_TCRU] = 0x4, [RZ_MTU3_TCR2U] = 0x5,
> > + [RZ_MTU3_TIORU] = 0x6, [RZ_MTU3_TCRV] = 0x14,
> > + [RZ_MTU3_TCR2V] = 0x15, [RZ_MTU3_TIORV] = 0x16,
> > + [RZ_MTU3_TCRW] = 0x24, [RZ_MTU3_TCR2W] = 0x25,
> > + [RZ_MTU3_TIORW] = 0x26
> > + },
> > + {
> > + [RZ_MTU3_TIER] = 0x8, [RZ_MTU3_NFCR] = 0x93,
> > + [RZ_MTU3_TSR] = 0x2c, [RZ_MTU3_TCR] = 0x0,
> > + [RZ_MTU3_TCR2] = 0x4c, [RZ_MTU3_TMDR1] = 0x2,
> > + [RZ_MTU3_TIORH] = 0x4, [RZ_MTU3_TIORL] = 0x5,
> > + [RZ_MTU3_TBTM] = 0x38
> > + },
> > + {
> > + [RZ_MTU3_TIER] = 0x8, [RZ_MTU3_NFCR] = 0x93,
> > + [RZ_MTU3_TSR] = 0x2c, [RZ_MTU3_TCR] = 0x0,
> > + [RZ_MTU3_TCR2] = 0x4c, [RZ_MTU3_TMDR1] = 0x2,
> > + [RZ_MTU3_TIORH] = 0x5, [RZ_MTU3_TIORL] = 0x6,
> > + [RZ_MTU3_TBTM] = 0x38
> > + },
> > + {
> > + [RZ_MTU3_TIER] = 0x4, [RZ_MTU3_NFCR] = 0x368,
> > + [RZ_MTU3_TCR] = 0x0, [RZ_MTU3_TCR2] = 0x6,
> > + [RZ_MTU3_TMDR1] = 0x1, [RZ_MTU3_TIORH] = 0x2,
> > + [RZ_MTU3_TIORL] = 0x3
> > + }
> > +};
> > +
> > +static const unsigned long rz_mtu3_16bit_ch_reg_offs[][12] = {
> > + {
> > + [RZ_MTU3_TCNT] = 0x6, [RZ_MTU3_TGRA] = 0x8,
> > + [RZ_MTU3_TGRB] = 0xa, [RZ_MTU3_TGRC] = 0xc,
> > + [RZ_MTU3_TGRD] = 0xe, [RZ_MTU3_TGRE] = 0x20,
> > + [RZ_MTU3_TGRF] = 0x22
> > + },
> > + {
> > + [RZ_MTU3_TCNT] = 0x6, [RZ_MTU3_TGRA] = 0x8,
> > + [RZ_MTU3_TGRB] = 0xa
> > + },
> > + {
> > + [RZ_MTU3_TCNT] = 0x6, [RZ_MTU3_TGRA] = 0x8,
> > + [RZ_MTU3_TGRB] = 0xa
> > + },
> > + {
> > + [RZ_MTU3_TCNT] = 0x10, [RZ_MTU3_TGRA] = 0x18,
> > + [RZ_MTU3_TGRB] = 0x1a, [RZ_MTU3_TGRC] = 0x24,
> > + [RZ_MTU3_TGRD] = 0x26, [RZ_MTU3_TGRE] = 0x72
> > + },
> > + {
> > + [RZ_MTU3_TCNT] = 0x11, [RZ_MTU3_TGRA] = 0x1b,
> > + [RZ_MTU3_TGRB] = 0x1d, [RZ_MTU3_TGRC] = 0x27,
> > + [RZ_MTU3_TGRD] = 0x29, [RZ_MTU3_TGRE] = 0x73,
> > + [RZ_MTU3_TGRF] = 0x75, [RZ_MTU3_TADCR] = 0x3f,
> > + [RZ_MTU3_TADCORA] = 0x43, [RZ_MTU3_TADCORB] = 0x45,
> > + [RZ_MTU3_TADCOBRA] = 0x47,
> > + [RZ_MTU3_TADCOBRB] = 0x49
> > + },
> > + {
> > + [RZ_MTU3_TCNTU] = 0x0, [RZ_MTU3_TGRU] = 0x2,
> > + [RZ_MTU3_TCNTV] = 0x10, [RZ_MTU3_TGRV] = 0x12,
> > + [RZ_MTU3_TCNTW] = 0x20, [RZ_MTU3_TGRW] = 0x22
> > + },
> > + {
> > + [RZ_MTU3_TCNT] = 0x10, [RZ_MTU3_TGRA] = 0x18,
> > + [RZ_MTU3_TGRB] = 0x1a, [RZ_MTU3_TGRC] = 0x24,
> > + [RZ_MTU3_TGRD] = 0x26, [RZ_MTU3_TGRE] = 0x72
> > + },
> > + {
> > + [RZ_MTU3_TCNT] = 0x11, [RZ_MTU3_TGRA] = 0x1b,
> > + [RZ_MTU3_TGRB] = 0x1d, [RZ_MTU3_TGRC] = 0x27,
> > + [RZ_MTU3_TGRD] = 0x29, [RZ_MTU3_TGRE] = 0x73,
> > + [RZ_MTU3_TGRF] = 0x75, [RZ_MTU3_TADCR] = 0x3f,
> > + [RZ_MTU3_TADCORA] = 0x43, [RZ_MTU3_TADCORB] = 0x45,
> > + [RZ_MTU3_TADCOBRA] = 0x47,
> > + [RZ_MTU3_TADCOBRB] = 0x49
> > + },
> > +};
> > +
As for consistency, I will associate the channel here as well,
eventhough it consumes more memory.
2 * 5 = 10 vs 9 *5 = 45 bytes.
> > +static const unsigned long rz_mtu3_32bit_ch_reg_offs[][5] = {
> > + {
> > + [RZ_MTU3_TCNTLW] = 0x20, [RZ_MTU3_TGRALW] = 0x24,
> > + [RZ_MTU3_TGRBLW] = 0x28
> > + },
> > + { [RZ_MTU3_TCNT] = 0x8, [RZ_MTU3_TGRA] = 0xc,
> > + [RZ_MTU3_TGRB] = 0x10, [RZ_MTU3_TGRC] = 0x14,
> > + [RZ_MTU3_TGRD] = 0x18
> > + }
> > +};
> > +
> > +static bool rz_mtu3_is_16bit_shared_reg(u16 off) {
> > + return (off == RZ_MTU3_TDDRA || off == RZ_MTU3_TDDRB ||
> > + off == RZ_MTU3_TCDRA || off == RZ_MTU3_TCDRB ||
> > + off == RZ_MTU3_TCBRA || off == RZ_MTU3_TCBRB ||
> > + off == RZ_MTU3_TCNTSA || off == RZ_MTU3_TCNTSB); }
> > +
> > +u16 rz_mtu3_shared_reg_read(struct rz_mtu3_channel *ch, u16 off) {
> > + struct rz_mtu3 *mtu = dev_get_drvdata(c271667730h->dev->parent);
> > +
> > + if (rz_mtu3_is_16bit_shared_reg(off))
> > + return readw(mtu->mmio + off);
> > + else
> > + return readb(mtu->mmio + off);
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_shared_reg_read);
> > +
> > +u8 rz_mtu3_8bit_ch_read(struct rz_mtu3_channel *ch, u16 off)
>
> Any harm in s/off/offset/? I don't think it adds much bulk and will make it
> easier on the eye for the reader.
Ok, I tried to reduce number of columns. As you said will change it to offset.
>
> > +{
> > + u16 ch_offs;
> > +
> > + ch_offs = rz_mtu3_8bit_ch_reg_offs[ch->index][off];
> > + if (off != RZ_MTU3_TCR && ch_offs == 0)
> > + return -EINVAL;
> > +
> > + /*
> > + * NFCR register addresses on MTU{0,1,2,5,8} channels are smaller than
> > + * channel's base address.
> > + */
> > + if (off == RZ_MTU3_NFCR && (ch->index <= RZ_MTU2 ||
> > + ch->index == RZ_MTU5 ||
> > + ch->index == RZ_MTU8))
> > + return readb(ch->base - ch_offs);
> > + else
> > + return readb(ch->base + ch_offs);
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_8bit_ch_read);
> > +
> > +u16 rz_mtu3_16bit_ch_read(struct rz_mtu3_channel *ch, u16 off) {
> > + u16 ch_offs;
> > +
> > + /* MTU8 doesn't have 16-bit registers */
> > + if (ch->index == RZ_MTU8)
> > + return 0;
> > +
> > + ch_offs = rz_mtu3_16bit_ch_reg_offs[ch->index][off];
> > + if (ch->index != RZ_MTU5 && off != RZ_MTU3_TCNTU && ch_offs == 0)
> > + return 0;
> > +
> > + return readw(ch->base + ch_offs);
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_16bit_ch_read);
> > +
> > +u32 rz_mtu3_32bit_ch_read(struct rz_mtu3_channel *ch, u16 off) {
> > + u16 ch_offs;
> > +
> > + if (ch->index == RZ_MTU1)
> > + ch_offs = rz_mtu3_32bit_ch_reg_offs[0][off];
>
> When you define the channel numbers, as suggested above, you can swap out
> these magic numbers out for them instead.
Agreed.
>
> > + else if (ch->index == RZ_MTU8)
> > + ch_offs = rz_mtu3_32bit_ch_reg_offs[1][off];
> > +
> > + if (!ch_offs)
> > + return -EINVAL;
> > +
> > + return readl(ch->base + ch_offs);
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_32bit_ch_read);
> > +
> > +void rz_mtu3_8bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u8
> > +val) {
> > + u16 ch_offs;
> > +
> > + ch_offs = rz_mtu3_8bit_ch_reg_offs[ch->index][off];
> > + if (ch->index != RZ_MTU5 && off != RZ_MTU3_TCR && ch_offs == 0)
> > + return;
> > +
> > + /*
> > + * NFCR register addresses on MTU{0,1,2,5,8} channels are smaller than
> > + * channel's base address.
> > + */
> > + if (off == RZ_MTU3_NFCR && (ch->index <= RZ_MTU2 ||
> > + ch->index == RZ_MTU5 ||
> > + ch->index == RZ_MTU8))
> > + writeb(val, ch->base - ch_offs);
> > + else
> > + writeb(val, ch->base + ch_offs);
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_8bit_ch_write);
> > +
> > +void rz_mtu3_16bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u16
> > +val) {
> > + u16 ch_offs;
> > +
> > + /* MTU8 doesn't have 16-bit registers */
> > + if (ch->index == RZ_MTU8)
> > + return;
> > +
> > + ch_offs = rz_mtu3_16bit_ch_reg_offs[ch->index][off];
> > + if (ch->index != RZ_MTU5 && off != RZ_MTU3_TCNTU && ch_offs == 0)
> > + return;
> > +
> > + writew(val, ch->base + ch_offs);
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_16bit_ch_write);
> > +
> > +void rz_mtu3_32bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u32
> > +val) {
> > + u16 ch_offs;
> > +
> > + if (ch->index == RZ_MTU1)
> > + ch_offs = rz_mtu3_32bit_ch_reg_offs[0][off];
> > + else if (ch->index == RZ_MTU8)
> > + ch_offs = rz_mtu3_32bit_ch_reg_offs[1][off];
> > +
> > + if (!ch_offs)
> > + return;
> > +
> > + writel(val, ch->base + ch_offs);
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_32bit_ch_write);
> > +
> > +void rz_mtu3_shared_reg_write(struct rz_mtu3_channel *ch, u16 off,
> > +u16 value) {
> > + struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> > +
> > + if (rz_mtu3_is_16bit_shared_reg(off))
> > + writew(value, mtu->mmio + off);
> > + else
> > + writeb((u8)value, mtu->mmio + off); }
> > +EXPORT_SYMBOL_GPL(rz_mtu3_shared_reg_write);
> > +
> > +void rz_mtu3_shared_reg_update_bit(struct rz_mtu3_channel *ch, u16 off,
> > + u16 pos, u8 val)
> > +{
> > + struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> > + unsigned long tmdr, flags;
> > +
> > + raw_spin_lock_irqsave(&mtu->lock, flags);
> > + tmdr = rz_mtu3_shared_reg_read(ch, off);
> > + __assign_bit(pos, &tmdr, !!val);
> > + rz_mtu3_shared_reg_write(ch, off, tmdr);
> > + raw_spin_unlock_irqrestore(&mtu->lock, flags); }
> > +EXPORT_SYMBOL_GPL(rz_mtu3_shared_reg_update_bit);
> > +
> > +static void rz_mtu3_start_stop_ch(struct rz_mtu3_channel *ch, bool
> > +start) {
> > + struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> > + unsigned long flags, value;
> > + u8 offs;
> > +
> > + /* start stop register shared by multiple timer channels */
> > + raw_spin_lock_irqsave(&mtu->lock, flags);
> > +
> > + if (ch->index == RZ_MTU6 || ch->index == RZ_MTU7) {
> > + value = rz_mtu3_shared_reg_read(ch, RZ_MTU3_TSTRB);
> > + if (start)
> > + value |= 1 << ch->index;
> > + else
> > + value &= ~(1 << ch->index);
> > + rz_mtu3_shared_reg_write(ch, RZ_MTU3_TSTRB, value);
> > + } else if (ch->index != RZ_MTU5) {
> > + value = rz_mtu3_shared_reg_read(ch, RZ_MTU3_TSTRA);
> > + if (ch->index == RZ_MTU8)
> > + offs = 0x08;
> > + else if (ch->index < RZ_MTU3)
> > + offs = 1 << ch->index;
> > + else
> > + offs = 1 << (ch->index + 3);
> > + if (start)
> > + value |= offs;
> > + else
> > + value &= ~offs;
> > + rz_mtu3_shared_reg_write(ch, RZ_MTU3_TSTRA, value);
> > + }
> > +
> > + raw_spin_unlock_irqrestore(&mtu->lock, flags); }
> > +
> > +bool rz_mtu3_is_enabled(struct rz_mtu3_channel *ch) {
> > + struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> > + unsigned long flags, value;
> > + bool ret = false;
> > + u8 offs;
> > +
> > + /* start stop register shared by multiple timer channels */
> > + raw_spin_lock_irqsave(&mtu->lock, flags);
> > +
> > + if (ch->index == RZ_MTU6 || ch->index == RZ_MTU7) {
> > + value = rz_mtu3_shared_reg_read(ch, RZ_MTU3_TSTRB);
> > + ret = value & (1 << ch->index);
> > + } else if (ch->index != RZ_MTU5) {
> > + value = rz_mtu3_shared_reg_read(ch, RZ_MTU3_TSTRA);
> > + if (ch->index == RZ_MTU8)
> > + offs = 0x08;
> > + else if (ch->index < RZ_MTU3)
> > + offs = 1 << ch->index;
> > + else
> > + offs = 1 << (ch->index + 3);
> > +
> > + ret = value & offs;
> > + }
> > +
> > + raw_spin_unlock_irqrestore(&mtu->lock, flags);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_is_enabled);
> > +
> > +int rz_mtu3_enable(struct rz_mtu3_channel *ch) {
> > + /* enable channel */
> > + rz_mtu3_start_stop_ch(ch, true);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_enable);
> > +
> > +void rz_mtu3_disable(struct rz_mtu3_channel *ch) {
> > + /* disable channel */
> > + rz_mtu3_start_stop_ch(ch, false);
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_disable);
> > +
> > +static const unsigned int ch_reg_offsets[] = {
> > + 0x100, 0x180, 0x200, 0x000, 0x001, 0xa80, 0x800, 0x801, 0x400 };
> > +
> > +static void rz_mtu3_reset_assert(void *data) {
> > + struct rz_mtu3 *mtu = dev_get_drvdata(data);
> > +
> > + mfd_remove_devices(data);
> > + reset_control_assert(mtu->rstc);
> > +}
> > +
> > +static const struct mfd_cell rz_mtu3_devs[] = {
> > + {
> > + .name = "rz-mtu3-counter",
> > + },
> > + {
> > + .name = "pwm-rz-mtu3",
> > + },
> > +};
> > +
> > +static int rz_mtu3_probe(struct platform_device *pdev) {
> > + struct rz_mtu3 *ddata;
> > + unsigned int i;
> > + int ret;
> > +
> > + ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
> > + if (!ddata)
> > + return -ENOMEM;
> > +
> > + ddata->mmio = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(ddata->mmio))
> > + return PTR_ERR(ddata->mmio);
> > +
> > + ddata->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > + if (IS_ERR(ddata->rstc))
> > + return PTR_ERR(ddata->rstc);
> > +
> > + ddata->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(ddata->clk))
> > + return PTR_ERR(ddata->clk);
> > +
> > + reset_control_deassert(ddata->rstc);
> > + raw_spin_lock_init(&ddata->lock);
> > + platform_set_drvdata(pdev, ddata);
> > +
> > + for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) {
> > + ddata->channels[i].index = i;
> > + ddata->channels[i].is_busy = false;
> > + ddata->channels[i].base = ddata->mmio + ch_reg_offsets[i];
> > + mutex_init(&ddata->channels[i].lock);
> > + }
> > +
> > + ret = mfd_add_devices(&pdev->dev, 0, rz_mtu3_devs,
> > + ARRAY_SIZE(rz_mtu3_devs), NULL, 0, NULL);
> > + if (ret < 0)
> > + goto err_assert;
> > +
> > + return devm_add_action_or_reset(&pdev->dev, rz_mtu3_reset_assert,
> > + &pdev->dev);
> > +
> > +err_assert:
> > + reset_control_assert(ddata->rstc);
> > + return ret;
> > +}
> > +
> > +static const struct of_device_id rz_mtu3_of_match[] = {
> > + { .compatible = "renesas,rz-mtu3", },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rz_mtu3_of_match);
> > +
> > +static struct platform_driver rz_mtu3_driver = {
> > + .probe = rz_mtu3_probe,
> > + .driver = {
> > + .name = "rz-mtu3",
> > + .of_match_table = rz_mtu3_of_match,
> > + },
> > +};
> > +module_platform_driver(rz_mtu3_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@xxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a Core Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/rz-mtu3.h b/include/linux/mfd/rz-mtu3.h
> > new file mode 100644 index 000000000000..42e561a9603c
> > --- /dev/null
> > +++ b/include/linux/mfd/rz-mtu3.h
> > @@ -0,0 +1,243 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2022 Renesas Electronics Corporation */ #ifndef
> > +__LINUX_RZ_MTU3_H__ #define __LINUX_RZ_MTU3_H__
>
> __MFD_RZ_MTU3_H__
OK.
>
> > +#include <linux/clk.h>
>
> What about all the others?
It is not required here. Will remove it.
>
> > +/* 8-bit shared register offsets macros */
> > +#define RZ_MTU3_TSTRA 0x080 /* Timer start register A */
> > +#define RZ_MTU3_TSTRB 0x880 /* Timer start register B */
> > +
> > +/* 16-bit shared register offset macros */
> > +#define RZ_MTU3_TDDRA 0x016 /* Timer dead time data register A */
> > +#define RZ_MTU3_TDDRB 0x816 /* Timer dead time data register B */
> > +#define RZ_MTU3_TCDRA 0x014 /* Timer cycle data register A */
> > +#define RZ_MTU3_TCDRB 0x814 /* Timer cycle data register B */
> > +#define RZ_MTU3_TCBRA 0x022 /* Timer cycle buffer register A */
> > +#define RZ_MTU3_TCBRB 0x822 /* Timer cycle buffer register B */
> > +#define RZ_MTU3_TCNTSA 0x020 /* Timer subcounter A */
> > +#define RZ_MTU3_TCNTSB 0x820 /* Timer subcounter B */
> > +
> > +/*
> > + * MTU5 contains 3 timer counter registers and is totaly different
> > + * from other channels, so we must separate its offset */
> > +
> > +/* 8-bit register offset macros of MTU3 channels except MTU5 */
> > +#define RZ_MTU3_TIER 0 /* Timer interrupt register */
> > +#define RZ_MTU3_NFCR 1 /* Noise filter control register */
> > +#define RZ_MTU3_TSR 2 /* Timer status register */
> > +#define RZ_MTU3_TCR 3 /* Timer control register */
> > +#define RZ_MTU3_TCR2 4 /* Timer control register 2 */
> > +
> > +/* Timer mode register 1 */
> > +#define RZ_MTU3_TMDR1 5
> > +#define RZ_MTU3_TMDR1_MD GENMASK(3, 0)
> > +#define RZ_MTU3_TMDR1_MD_NORMAL FIELD_PREP(RZ_MTU3_TMDR1_MD, 0)
> > +#define RZ_MTU3_TMDR1_MD_PWMMODE1 FIELD_PREP(RZ_MTU3_TMDR1_MD, 2)
> > +
> > +#define RZ_MTU3_TIOR 6 /* Timer I/O control register */
> > +#define RZ_MTU3_TIORH 6 /* Timer I/O control register H */
> > +#define RZ_MTU3_TIORL 7 /* Timer I/O control register L */
> > +/* Only MTU3/4/6/7 have TBTM registers */
> > +#define RZ_MTU3_TBTM 8 /* Timer buffer operation transfer mode register
> */
> > +
> > +/* 8-bit MTU5 register offset macros */
> > +#define RZ_MTU3_TSTR 2 /* MTU5 Timer start register */
> > +#define RZ_MTU3_TCNTCMPCLR 3 /* MTU5 Timer compare match clear register
> */
> > +#define RZ_MTU3_TCRU 4 /* Timer control register U */
> > +#define RZ_MTU3_TCR2U 5 /* Timer control register 2U */
> > +#define RZ_MTU3_TIORU 6 /* Timer I/O control register U */
> > +#define RZ_MTU3_TCRV 7 /* Timer control register V */
> > +#define RZ_MTU3_TCR2V 8 /* Timer control register 2V */
> > +#define RZ_MTU3_TIORV 9 /* Timer I/O control register V */
> > +#define RZ_MTU3_TCRW 10 /* Timer control register W */
> > +#define RZ_MTU3_TCR2W 11 /* Timer control register 2W */
> > +#define RZ_MTU3_TIORW 12 /* Timer I/O control register W */
> > +
> > +/* 16-bit register offset macros of MTU3 channels except MTU5 */
> > +#define RZ_MTU3_TCNT 0 /* Timer counter */
> > +#define RZ_MTU3_TGRA 1 /* Timer general register A */
> > +#define RZ_MTU3_TGRB 2 /* Timer general register B */
> > +#define RZ_MTU3_TGRC 3 /* Timer general register C */
> > +#define RZ_MTU3_TGRD 4 /* Timer general register D */
> > +#define RZ_MTU3_TGRE 5 /* Timer general register E */
> > +#define RZ_MTU3_TGRF 6 /* Timer general register F */
> > +/* Timer A/D converter start request registers */
> > +#define RZ_MTU3_TADCR 7 /* control register */
> > +#define RZ_MTU3_TADCORA 8 /* cycle set register A */
> > +#define RZ_MTU3_TADCORB 9 /* cycle set register B */
> > +#define RZ_MTU3_TADCOBRA 10 /* cycle set buffer register A */
> > +#define RZ_MTU3_TADCOBRB 11 /* cycle set buffer register B */
> > +
> > +/* 16-bit MTU5 register offset macros */
> > +#define RZ_MTU3_TCNTU 0 /* MTU5 Timer counter U */
> > +#define RZ_MTU3_TGRU 1 /* MTU5 Timer general register U */
> > +#define RZ_MTU3_TCNTV 2 /* MTU5 Timer counter V */
> > +#define RZ_MTU3_TGRV 3 /* MTU5 Timer general register V */
> > +#define RZ_MTU3_TCNTW 4 /* MTU5 Timer counter W */
> > +#define RZ_MTU3_TGRW 5 /* MTU5 Timer general register W */
> > +
> > +/* 32-bit register offset */
> > +#define RZ_MTU3_TCNTLW 0 /* Timer longword counter */
> > +#define RZ_MTU3_TGRALW 1 /* Timer longword general register A
> */
> > +#define RZ_MTU3_TGRBLW 2 /* Timer longowrd general register B
> */
> > +
> > +#define RZ_MTU3_TMDR3 0x191 /* MTU1 Timer Mode Register 3 */
> > +
> > +/* Macros for setting registers */
> > +#define RZ_MTU3_TCR_CCLR_TGRA BIT(5)
> > +
> > +enum rz_mtu3_channels {
> > + RZ_MTU0,
> > + RZ_MTU1,
> > + RZ_MTU2,
> > + RZ_MTU3,
> > + RZ_MTU4,
> > + RZ_MTU5,
> > + RZ_MTU6,
> > + RZ_MTU7,
> > + RZ_MTU8,
> > + RZ_MTU_NUM_CHANNELS
> > +};
> > +
> > +/**
> > + * struct rz_mtu3_channel - MTU3 channel private data
> > + *
> > + * @dev: device handle
> > + * @index: channel index
>
> I'm generally against structures 'index'ing or 'id'ing themselves.
> There is usually a better way to solve the issue using pointers.
> However, I do believe this scenario is different (or I haven't spent long
> enough thinking about an alternative), so please work around this by
> changing the nomenclature to something like 'channel_number'.
Agreed, will use channel_number.
>
> > + * @base: channel base address
> > + * @lock: Lock to protect channel state
> > + * @is_busy: channel state
> > + */
> > +struct rz_mtu3_channel {
> > + struct device *dev;
> > + unsigned int index;
> > + void __iomem *base;
> > + struct mutex lock; /* Protect channel state */
>
> This comment is superfluous IMHO.
Basically it is for protecting is_busy variable(which is state of channel "idle vs busy")
I will remove it.
>
> > + bool is_busy;
>
> What's the purpose of this?
We should not allow consumers(PWM, counter) to acquire a channel, if it is busy.
The lock will avoid the race for concurrent access.
>
> > +};
> > +
> > +/**
> > + * struct rz_mtu3 - MTU3 core private data
> > + *
> > + * @clk: MTU3 module clock
> > + * @mmio: MTU3 module clock
> > + * @lock: Lock to protect shared register access
> > + * @rz_mtu3_channel: HW channels
> > + */
> > +struct rz_mtu3 {
> > + void *priv_rz_mtu3;
> > + void __iomem *mmio;
> > + struct clk *clk;
> > + struct reset_control *rstc;
> > + raw_spinlock_t lock; /* Protect the shared registers */
>
> As above. This is documented twice.
Agreed. Will remove it. I believe, to suppress check patch warning
I added it.
>
> > + struct rz_mtu3_channel channels[RZ_MTU_NUM_CHANNELS]; };
> > +
> > +#if IS_ENABLED(CONFIG_RZ_MTU3)
> > +static inline bool rz_mtu3_request_channel(struct rz_mtu3_channel
> > +*ch) {
> > + bool is_idle;
> > +
> > + mutex_lock(&ch->lock);
> > + is_idle = !ch->is_busy;
> > + if (is_idle)
> > + ch->is_busy = true;
>
> Perhaps I'd reading this all wrong, but ...
>
> What are you trying to do here?
It is to avoid race between counter and pwm to acquiring the same channel.
If a channel is free, then only they can access the channel.
Please let me know if any clarifications needed, or correct me if anything wrong.
Cheers,
Biju
>
> > + mutex_unlock(&ch->lock);
> > +
> > + return is_idle;
> > +}
> > +
> > +static inline void rz_mtu3_release_channel(struct rz_mtu3_channel
> > +*ch) {
> > + mutex_lock(&ch->lock);
> > + ch->is_busy = false;
> > + mutex_unlock(&ch->lock);
> > +}
> > +
> > +bool rz_mtu3_is_enabled(struct rz_mtu3_channel *ch); void
> > +rz_mtu3_disable(struct rz_mtu3_channel *ch); int
> > +rz_mtu3_enable(struct rz_mtu3_channel *ch);
> > +
> > +u8 rz_mtu3_8bit_ch_read(struct rz_mtu3_channel *ch, u16 off);
> > +u16 rz_mtu3_16bit_ch_read(struct rz_mtu3_channel *ch, u16 off);
> > +u32 rz_mtu3_32bit_ch_read(struct rz_mtu3_channel *ch, u16 off);
> > +u16 rz_mtu3_shared_reg_read(struct rz_mtu3_channel *ch, u16 off);
> > +
> > +void rz_mtu3_8bit_ch_write(struct rz_mtu3_channel *ch, u16 off, u8
> > +val); void rz_mtu3_16bit_ch_write(struct rz_mtu3_channel *ch, u16
> > +off, u16 val); void rz_mtu3_32bit_ch_write(struct rz_mtu3_channel
> > +*ch, u16 off, u32 val); void rz_mtu3_shared_reg_write(struct
> > +rz_mtu3_channel *ch, u16 off, u16 val); void
> rz_mtu3_shared_reg_update_bit(struct rz_mtu3_channel *ch, u16 off,
> > + u16 pos, u8 val);
> > +#else
> > +static inline bool rz_mtu3_request_channel(struct rz_mtu3_channel
> > +*ch) {
> > + return false;
> > +}
> > +
> > +static inline void rz_mtu3_release_channel(struct rz_mtu3_channel
> > +*ch) { }
> > +
> > +static inline bool rz_mtu3_is_enabled(struct rz_mtu3_channel *ch) {
> > + return false;
> > +}
> > +
> > +static inline void rz_mtu3_disable(struct rz_mtu3_channel *ch) { }
> > +
> > +static inline int rz_mtu3_enable(struct rz_mtu3_channel *ch) {
> > + return 0;
> > +}
> > +
> > +static inline u8 rz_mtu3_8bit_ch_read(struct rz_mtu3_channel *ch, u16
> > +off) {
> > + return 0;
> > +}
> > +
> > +static inline u16 rz_mtu3_16bit_ch_read(struct rz_mtu3_channel *ch,
> > +u16 off) {
> > + return 0;
> > +}
> > +
> > +static inline u32 rz_mtu3_32bit_ch_read(struct rz_mtu3_channel *ch,
> > +u16 off) {
> > + return 0;
> > +}
> > +
> > +static inline u16 rz_mtu3_shared_reg_read(struct rz_mtu3_channel *ch,
> > +u16 off) {
> > + return 0;
> > +}
> > +
> > +static inline void rz_mtu3_8bit_ch_write(struct rz_mtu3_channel *ch,
> > +u16 off, u8 val) { }
> > +
> > +static inline void rz_mtu3_16bit_ch_write(struct rz_mtu3_channel *ch,
> > +u16 off, u16 val) { }
> > +
> > +static inline void rz_mtu3_32bit_ch_write(struct rz_mtu3_channel *ch,
> > +u16 off, u32 val) { }
> > +
> > +static inline void rz_mtu3_shared_reg_write(struct rz_mtu3_channel
> > +*ch, u16 off, u16 val) { }
> > +
> > +static inline void rz_mtu3_shared_reg_update_bit(struct rz_mtu3_channel
> *ch,
> > + u16 off, u16 pos, u8 val)
> > +{
> > +}
> > +#endif
> > +
> > +#endif /* __LINUX_RZ_MTU3_H__ */
> > --
> > 2.25.1
> >
>
> --
> Lee Jones [李琼斯]
