Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
From: Paul Cercueil <paul@crapouillou.net>
To: Lee Jones <lee.jones@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Ralf Baechle <ralf@linux-mips.org>,
	Paul Burton <paul.burton@mips.com>,
	James Hogan <jhogan@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Jason Cooper <jason@lakedaemon.net>,
	Marc Zyngier <marc.zyngier@arm.com>,
	Mathieu Malaterre <malat@debian.org>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-mips@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-clk@vger.kernel.org, od@zcrc.me
Subject: Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver
Date: Wed, 26 Jun 2019 15:55:50 +0200
Message-ID: <1561557350.1872.0@crapouillou.net> (raw)
In-Reply-To: <20190626131850.GW21119@dell>

Hi Lee,

Le mer. 26 juin 2019 à 15:18, Lee Jones <lee.jones@linaro.org> a 
écrit :
> On Tue, 21 May 2019, Paul Cercueil wrote:
> 
>>  This driver will provide a regmap that can be retrieved very early 
>> in
>>  the boot process through the API function ingenic_tcu_get_regmap().
>> 
>>  Additionally, it will call devm_of_platform_populate() so that all 
>> the
>>  children devices will be probed.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>      v12: New patch
>> 
>>   drivers/mfd/Kconfig             |   8 +++
>>   drivers/mfd/Makefile            |   1 +
>>   drivers/mfd/ingenic-tcu.c       | 113 
>> ++++++++++++++++++++++++++++++++
>>   include/linux/mfd/ingenic-tcu.h |   8 +++
>>   4 files changed, 130 insertions(+)
>>   create mode 100644 drivers/mfd/ingenic-tcu.c
>> 
>>  diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>  index 294d9567cc71..a13544474e05 100644
>>  --- a/drivers/mfd/Kconfig
>>  +++ b/drivers/mfd/Kconfig
>>  @@ -494,6 +494,14 @@ config HTC_I2CPLD
>>   	  This device provides input and output GPIOs through an I2C
>>   	  interface to one or more sub-chips.
>> 
>>  +config INGENIC_TCU
>>  +	bool "Ingenic Timer/Counter Unit (TCU) support"
>>  +	depends on MIPS || COMPILE_TEST
>>  +	select REGMAP_MMIO
>>  +	help
>>  +	  Say yes here to support the Timer/Counter Unit (TCU) IP present
>>  +	  in the JZ47xx SoCs from Ingenic.
>>  +
>>   config MFD_INTEL_QUARK_I2C_GPIO
>>   	tristate "Intel Quark MFD I2C GPIO"
>>   	depends on PCI
>>  diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>  index 52b1a90ff515..fb89e131ae98 100644
>>  --- a/drivers/mfd/Makefile
>>  +++ b/drivers/mfd/Makefile
>>  @@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o 
>> ab8500-sysctrl.o
>>   obj-$(CONFIG_MFD_TIMBERDALE)    += timberdale.o
>>   obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
>>   obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
>>  +obj-$(CONFIG_INGENIC_TCU)	+= ingenic-tcu.o
>>   obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
>>   obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
>>   obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
>>  diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
>>  new file mode 100644
>>  index 000000000000..6c1d5e4310c1
>>  --- /dev/null
>>  +++ b/drivers/mfd/ingenic-tcu.c
>>  @@ -0,0 +1,113 @@
>>  +// SPDX-License-Identifier: GPL-2.0
>>  +/*
>>  + * JZ47xx SoCs TCU MFD driver
> 
> Nit: Another line here please.
> 
>>  + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
>>  + */
>>  +
>>  +#include <linux/mfd/ingenic-tcu.h>
>>  +#include <linux/of_address.h>
>>  +#include <linux/of_platform.h>
>>  +#include <linux/platform_device.h>
>>  +#include <linux/regmap.h>
>>  +
>>  +struct ingenic_soc_info {
>>  +	unsigned int num_channels;
>>  +};
>>  +
>>  +static struct regmap *tcu_regmap __initdata;
>>  +
>>  +static const struct regmap_config ingenic_tcu_regmap_config = {
>>  +	.reg_bits = 32,
>>  +	.val_bits = 32,
>>  +	.reg_stride = 4,
>>  +	.max_register = TCU_REG_OST_CNTHBUF,
>>  +};
>>  +
>>  +static const struct ingenic_soc_info jz4740_soc_info = {
>>  +	.num_channels = 8,
>>  +};
>>  +
>>  +static const struct ingenic_soc_info jz4725b_soc_info = {
>>  +	.num_channels = 6,
>>  +};
>>  +
>>  +static const struct of_device_id ingenic_tcu_of_match[] = {
>>  +	{ .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
>>  +	{ .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, 
>> },
>>  +	{ .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
>>  +	{ }
>>  +};
>>  +
>>  +static struct regmap * __init ingenic_tcu_create_regmap(struct 
>> device_node *np)
>>  +{
>>  +	struct resource res;
>>  +	void __iomem *base;
>>  +	struct regmap *map;
>>  +
>>  +	if (!of_match_node(ingenic_tcu_of_match, np))
>>  +		return ERR_PTR(-EINVAL);
>>  +
>>  +	base = of_io_request_and_map(np, 0, "TCU");
>>  +	if (IS_ERR(base))
>>  +		return ERR_PTR(PTR_ERR(base));
>>  +
>>  +	map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
>>  +	if (IS_ERR(map))
>>  +		goto err_iounmap;
>>  +
>>  +	return map;
>>  +
>>  +err_iounmap:
>>  +	iounmap(base);
>>  +	of_address_to_resource(np, 0, &res);
>>  +	release_mem_region(res.start, resource_size(&res));
>>  +
>>  +	return map;
>>  +}
> 
> Why does this need to be set-up earlier than probe()?

See the explanation below.

>>  +static int __init ingenic_tcu_probe(struct platform_device *pdev)
>>  +{
>>  +	struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
>>  +
>>  +	platform_set_drvdata(pdev, map);
>>  +
>>  +	regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
>>  +
>>  +	return devm_of_platform_populate(&pdev->dev);
>>  +}
>>  +
>>  +static struct platform_driver ingenic_tcu_driver = {
>>  +	.driver = {
>>  +		.name = "ingenic-tcu",
>>  +		.of_match_table = ingenic_tcu_of_match,
>>  +	},
>>  +};
>>  +
>>  +static int __init ingenic_tcu_platform_init(void)
>>  +{
>>  +	return platform_driver_probe(&ingenic_tcu_driver,
>>  +				     ingenic_tcu_probe);
> 
> What?  Why?

The device driver probed here will populate the children devices,
which will be able to retrieve the pointer to the regmap through
device_get_regmap(dev->parent).

The children devices are normal platform drivers that can be probed
the normal way. These are the PWM driver, the watchdog driver, and the
OST (OS Timer) clocksource driver, all part of the same hardware block
(the Timer/Counter Unit or TCU).

>>  +}
>>  +subsys_initcall(ingenic_tcu_platform_init);
>>  +
>>  +struct regmap * __init ingenic_tcu_get_regmap(struct device_node 
>> *np)
>>  +{
>>  +	if (!tcu_regmap)
>>  +		tcu_regmap = ingenic_tcu_create_regmap(np);
>>  +
>>  +	return tcu_regmap;
>>  +}
> 
> This makes me pretty uncomfortable.
> 
> What calls it?

The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]), and 
the
non-OST clocksource driver (patch [07/13]) all probe very early in the 
boot
process, and share the same devicetree node. They call this function to 
get
a pointer to the regmap.

>>  +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int 
>> channel)
>>  +{
>>  +	const struct ingenic_soc_info *soc = 
>> device_get_match_data(dev->parent);
>>  +
>>  +	/* Enable all TCU channels for PWM use by default except channels 
>> 0/1 */
>>  +	u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
>>  +
>>  +	device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
>>  +				 &pwm_channels_mask);
>>  +
>>  +	return !!(pwm_channels_mask & BIT(channel));
>>  +}
>>  +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>>  diff --git a/include/linux/mfd/ingenic-tcu.h 
>> b/include/linux/mfd/ingenic-tcu.h
>>  index 2083fa20821d..21df23916cd2 100644
>>  --- a/include/linux/mfd/ingenic-tcu.h
>>  +++ b/include/linux/mfd/ingenic-tcu.h
>>  @@ -6,6 +6,11 @@
>>   #define __LINUX_MFD_INGENIC_TCU_H_
>> 
>>   #include <linux/bitops.h>
>>  +#include <linux/init.h>
>>  +
>>  +struct device;
>>  +struct device_node;
>>  +struct regmap;
>> 
>>   #define TCU_REG_WDT_TDR		0x00
>>   #define TCU_REG_WDT_TCER	0x04
>>  @@ -53,4 +58,7 @@
>>   #define TCU_REG_TCNTc(c)	(TCU_REG_TCNT0 + ((c) * 
>> TCU_CHANNEL_STRIDE))
>>   #define TCU_REG_TCSRc(c)	(TCU_REG_TCSR0 + ((c) * 
>> TCU_CHANNEL_STRIDE))
>> 
>>  +struct regmap * __init ingenic_tcu_get_regmap(struct device_node 
>> *np);
>>  +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int 
>> channel);
>>  +
>>   #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
> 
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog



  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-21 14:51 Ingenic Timer/Counter Unit (TCU) patchset v12 Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 01/13] dt-bindings: ingenic: Add DT bindings for TCU clocks Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 02/13] doc: Add doc for the Ingenic TCU hardware Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 03/13] dt-bindings: Add doc for the Ingenic TCU drivers Paul Cercueil
2019-05-24 20:21   ` Rob Herring
2019-05-25 19:13     ` Paul Cercueil
2019-06-11 14:57       ` Rob Herring
2019-05-21 14:51 ` [PATCH v12 04/13] mfd: Add Ingenic TCU driver Paul Cercueil
2019-06-22 12:21   ` Paul Cercueil
2019-06-26 13:18   ` Lee Jones
2019-06-26 13:55     ` Paul Cercueil [this message]
2019-06-27  6:58       ` Lee Jones
2019-06-27  8:49         ` Paul Cercueil
2019-06-27  9:01           ` Lee Jones
2019-06-27  9:19             ` Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 05/13] clk: ingenic: Add driver for the TCU clocks Paul Cercueil
2019-06-07 21:28   ` Stephen Boyd
2019-06-07 21:59     ` Paul Cercueil
2019-06-07 22:50       ` Stephen Boyd
2019-05-21 14:51 ` [PATCH v12 06/13] irqchip: Add irq-ingenic-tcu driver Paul Cercueil
2019-06-22 12:22   ` Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 07/13] clocksource: Add a new timer-ingenic driver Paul Cercueil
2019-06-22 12:23   ` Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 08/13] clk: jz4740: Add TCU clock Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 09/13] MIPS: jz4740: Add DTS nodes for the TCU drivers Paul Cercueil
2019-05-22  9:21   ` Mathieu Malaterre
2019-05-22 11:15     ` Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 10/13] MIPS: qi_lb60: Reduce system timer and clocksource to 750 kHz Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 11/13] MIPS: CI20: Reduce system timer and clocksource to 3 MHz Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 12/13] MIPS: GCW0: Reduce system timer and clocksource to 750 kHz Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 13/13] MIPS: jz4740: Drop obsolete code Paul Cercueil
2019-05-27 11:39 ` Ingenic Timer/Counter Unit (TCU) patchset v12 Mathieu Malaterre

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1561557350.1872.0@crapouillou.net \
    --to=paul@crapouillou.net \
    --cc=corbet@lwn.net \
    --cc=daniel.lezcano@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=jason@lakedaemon.net \
    --cc=jhogan@kernel.org \
    --cc=lee.jones@linaro.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=malat@debian.org \
    --cc=marc.zyngier@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=od@zcrc.me \
    --cc=paul.burton@mips.com \
    --cc=ralf@linux-mips.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org linux-mips@archiver.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/ public-inbox