Re: [PATCH v13 2/6] mfd: Add Renesas RZ/G2L MTU3a core driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 04 Mar 2023, Lee Jones wrote:

> 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:
> >  https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230113161753.1073706-3-biju.das.jz@xxxxxxxxxxxxxx/
> > 
> > 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.

Also, have you considered: devm_regmap_field_bulk_alloc()?

See: REG_FIELD()

> > +	},
> > +	{
> > +		[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
> > +	},
> > +};
> > +
> > +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.
> 
> > +{
> > +	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.
> 
> > +	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__
> 
> > +#include <linux/clk.h>
> 
> What about all the others?
> 
> > +/* 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'.
> 
> > + * @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.
> 
> > +	bool is_busy;
> 
> What's the purpose of this?
> 
> > +};
> > +
> > +/**
> > + * 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.
> 
> > +	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?
> 
> > +	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 [李琼斯]

-- 
Lee Jones [李琼斯]



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux