Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V3 2/6] hwmon: Add support for RPi voltage sensor
From: Stefan Wahren @ 2018-05-25 19:24 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1527276279-23876-1-git-send-email-stefan.wahren@i2se.com>

Currently there is no easy way to detect undervoltage conditions on a
remote Raspberry Pi. This hwmon driver retrieves the state of the
undervoltage sensor via mailbox interface. The handling based on
Noralf's modifications to the downstream firmware driver. In case of
an undervoltage condition only an entry is written to the kernel log.

CC: "Noralf Tr?nnes" <noralf@tronnes.org>
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 Documentation/hwmon/raspberrypi-hwmon |  22 +++++
 drivers/hwmon/Kconfig                 |  10 ++
 drivers/hwmon/Makefile                |   1 +
 drivers/hwmon/raspberrypi-hwmon.c     | 166 ++++++++++++++++++++++++++++++++++
 4 files changed, 199 insertions(+)
 create mode 100644 Documentation/hwmon/raspberrypi-hwmon
 create mode 100644 drivers/hwmon/raspberrypi-hwmon.c

diff --git a/Documentation/hwmon/raspberrypi-hwmon b/Documentation/hwmon/raspberrypi-hwmon
new file mode 100644
index 0000000..3c92e2c
--- /dev/null
+++ b/Documentation/hwmon/raspberrypi-hwmon
@@ -0,0 +1,22 @@
+Kernel driver raspberrypi-hwmon
+===============================
+
+Supported boards:
+  * Raspberry Pi A+ (via GPIO on SoC)
+  * Raspberry Pi B+ (via GPIO on SoC)
+  * Raspberry Pi 2 B (via GPIO on SoC)
+  * Raspberry Pi 3 B (via GPIO on port expander)
+  * Raspberry Pi 3 B+ (via PMIC)
+
+Author: Stefan Wahren <stefan.wahren@i2se.com>
+
+Description
+-----------
+
+This driver periodically polls a mailbox property of the VC4 firmware to detect
+undervoltage conditions.
+
+Sysfs entries
+-------------
+
+in0_lcrit_alarm		Undervoltage alarm
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index f10840a..fdaab82 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
 	  This driver can also be built as a module.  If so, the module
 	  will be called pwm-fan.
 
+config SENSORS_RASPBERRYPI_HWMON
+	tristate "Raspberry Pi voltage monitor"
+	depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
+	help
+	  If you say yes here you get support for voltage sensor on the
+	  Raspberry Pi.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called raspberrypi-hwmon.
+
 config SENSORS_SHT15
 	tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
 	depends on GPIOLIB || COMPILE_TEST
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e7d52a3..a929770 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
 obj-$(CONFIG_SENSORS_POWR1220)  += powr1220.o
 obj-$(CONFIG_SENSORS_PWM_FAN)	+= pwm-fan.o
+obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON)	+= raspberrypi-hwmon.o
 obj-$(CONFIG_SENSORS_S3C)	+= s3c-hwmon.o
 obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
 obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
new file mode 100644
index 0000000..fb4e4a6
--- /dev/null
+++ b/drivers/hwmon/raspberrypi-hwmon.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Raspberry Pi voltage sensor driver
+ *
+ * Based on firmware/raspberrypi.c by Noralf Tr?nnes
+ *
+ * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
+#define UNDERVOLTAGE_STICKY_BIT	BIT(16)
+
+struct rpi_hwmon_data {
+	struct device *hwmon_dev;
+	struct rpi_firmware *fw;
+	u32 last_throttled;
+	struct delayed_work get_values_poll_work;
+};
+
+static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
+{
+	u32 new_uv, old_uv, value;
+	int ret;
+
+	/* Request firmware to clear sticky bits */
+	value = 0xffff;
+
+	ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
+				    &value, sizeof(value));
+	if (ret) {
+		dev_err_once(data->hwmon_dev, "Failed to get throttled (%d)\n",
+			     ret);
+		return;
+	}
+
+	new_uv = value & UNDERVOLTAGE_STICKY_BIT;
+	old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
+	data->last_throttled = value;
+
+	if (new_uv == old_uv)
+		return;
+
+	if (new_uv)
+		dev_crit(data->hwmon_dev, "Undervoltage detected!\n");
+	else
+		dev_info(data->hwmon_dev, "Voltage normalised\n");
+
+	sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
+}
+
+static void get_values_poll(struct work_struct *work)
+{
+	struct rpi_hwmon_data *data;
+
+	data = container_of(work, struct rpi_hwmon_data,
+			    get_values_poll_work.work);
+
+	rpi_firmware_get_throttled(data);
+
+	/*
+	 * We can't run faster than the sticky shift (100ms) since we get
+	 * flipping in the sticky bits that are cleared.
+	 */
+	schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
+}
+
+static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
+		    u32 attr, int channel, long *val)
+{
+	struct rpi_hwmon_data *data = dev_get_drvdata(dev);
+
+	*val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
+	return 0;
+}
+
+static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
+			      u32 attr, int channel)
+{
+	return 0444;
+}
+
+static const u32 rpi_in_config[] = {
+	HWMON_I_LCRIT_ALARM,
+	0
+};
+
+static const struct hwmon_channel_info rpi_in = {
+	.type = hwmon_in,
+	.config = rpi_in_config,
+};
+
+static const struct hwmon_channel_info *rpi_info[] = {
+	&rpi_in,
+	NULL
+};
+
+static const struct hwmon_ops rpi_hwmon_ops = {
+	.is_visible = rpi_is_visible,
+	.read = rpi_read,
+};
+
+static const struct hwmon_chip_info rpi_chip_info = {
+	.ops = &rpi_hwmon_ops,
+	.info = rpi_info,
+};
+
+static int rpi_hwmon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rpi_hwmon_data *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	/* Parent driver assure that firmware is correct */
+	data->fw = dev_get_drvdata(dev->parent);
+
+	/* Init throttled */
+	ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
+				    &data->last_throttled,
+				    sizeof(data->last_throttled));
+
+	data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
+							       data,
+							       &rpi_chip_info,
+							       NULL);
+
+	INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
+	platform_set_drvdata(pdev, data);
+
+	if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
+		schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
+
+	return PTR_ERR_OR_ZERO(data->hwmon_dev);
+}
+
+static int rpi_hwmon_remove(struct platform_device *pdev)
+{
+	struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
+
+	cancel_delayed_work_sync(&data->get_values_poll_work);
+
+	return 0;
+}
+
+static struct platform_driver rpi_hwmon_driver = {
+	.probe = rpi_hwmon_probe,
+	.remove = rpi_hwmon_remove,
+	.driver = {
+		.name = "raspberrypi-hwmon",
+	},
+};
+module_platform_driver(rpi_hwmon_driver);
+
+MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

^ permalink raw reply related

* [PATCH V3 1/6] ARM: bcm2835: Add GET_THROTTLED firmware property
From: Stefan Wahren @ 2018-05-25 19:24 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1527276279-23876-1-git-send-email-stefan.wahren@i2se.com>

Recent Raspberry Pi firmware provides a mailbox property to detect
under-voltage conditions. Here is the current definition.

The u32 value returned by the firmware is divided into 2 parts:
  - lower 16-bits are the live value
  - upper 16-bits are the history or sticky value

  Bits:
  0: undervoltage
  1: arm frequency capped
  2: currently throttled
  16: undervoltage has occurred
  17: arm frequency capped has occurred
  18: throttling has occurred

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 include/soc/bcm2835/raspberrypi-firmware.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
index 8ee8991..c4a5c9e 100644
--- a/include/soc/bcm2835/raspberrypi-firmware.h
+++ b/include/soc/bcm2835/raspberrypi-firmware.h
@@ -75,6 +75,7 @@ enum rpi_firmware_property_tag {
 	RPI_FIRMWARE_GET_EDID_BLOCK =                         0x00030020,
 	RPI_FIRMWARE_GET_CUSTOMER_OTP =                       0x00030021,
 	RPI_FIRMWARE_GET_DOMAIN_STATE =                       0x00030030,
+	RPI_FIRMWARE_GET_THROTTLED =                          0x00030046,
 	RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
 	RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
 	RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,
-- 
2.7.4

^ permalink raw reply related

* [PATCH V3 0/6] hwmon: Add support for Raspberry Pi voltage sensor
From: Stefan Wahren @ 2018-05-25 19:24 UTC (permalink / raw)
  To: linux-arm-kernel

A common issue for the Raspberry Pi is an inadequate power supply. 
Noralf Tr?nnes started a discussion [1] about writing such undervoltage
conditions into the kernel log.

Changes in V3:
- rebase
- simplify probing

Changes in V2:
- simplified Kconfig dependency suggested by Robin Murphy
- replace dt-binding by probing from firmware driver
- add hwmon documentation
- minor improvements suggested by Guenter Roeck

[1] - https://github.com/raspberrypi/linux/issues/2367

Stefan Wahren (6):
  ARM: bcm2835: Add GET_THROTTLED firmware property
  hwmon: Add support for RPi voltage sensor
  firmware: raspberrypi: Register hwmon driver
  ARM: bcm2835_defconfig: Enable RPi voltage sensor
  ARM: multi_v7_defconfig: Enable RPi voltage sensor
  arm64: defconfig: Enable RPi voltage sensor

 Documentation/hwmon/raspberrypi-hwmon      |  22 ++++
 arch/arm/configs/bcm2835_defconfig         |   2 +-
 arch/arm/configs/multi_v7_defconfig        |   1 +
 arch/arm64/configs/defconfig               |   1 +
 drivers/firmware/raspberrypi.c             |  19 ++++
 drivers/hwmon/Kconfig                      |  10 ++
 drivers/hwmon/Makefile                     |   1 +
 drivers/hwmon/raspberrypi-hwmon.c          | 166 +++++++++++++++++++++++++++++
 include/soc/bcm2835/raspberrypi-firmware.h |   1 +
 9 files changed, 222 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/hwmon/raspberrypi-hwmon
 create mode 100644 drivers/hwmon/raspberrypi-hwmon.c

-- 
2.7.4

^ permalink raw reply

* [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control APIs
From: Jolly Shah @ 2018-05-25 19:23 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <6579fdce-6845-70d5-1dc1-64d52debef19@arm.com>

Hi Sudeep,

> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla at arm.com]
> Sent: Tuesday, May 15, 2018 1:58 AM
> To: Jolly Shah <JOLLYS@xilinx.com>; ard.biesheuvel at linaro.org;
> mingo at kernel.org; gregkh at linuxfoundation.org; matt at codeblueprint.co.uk;
> hkallweit1 at gmail.com; keescook at chromium.org;
> dmitry.torokhov at gmail.com; mturquette at baylibre.com;
> sboyd at codeaurora.org; michal.simek at xilinx.com; robh+dt at kernel.org;
> mark.rutland at arm.com; linux-clk at vger.kernel.org
> Cc: Sudeep Holla <sudeep.holla@arm.com>; Rajan Vaja <RAJANV@xilinx.com>;
> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org
> Subject: Re: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control
> APIs
> 
> 
> 
> On 14/05/18 20:18, Jolly Shah wrote:
> > Hi Sudeep,
> 
> [..]
> 
> >>
> >> As I mentioned in earlier patch, I don't see the need for this
> >> debugfs interface. Clock lay has read-only interface in debugfs
> >> already. Also if you want to debug clocks, you can do so using the
> >> driver which uses these clocks. Do you really want to manage clocks
> >> in user-space ? The whole idea of EEMI kind of interface is to
> >> abstract and hide the fine details even from non-trusted rich OS like
> >> Linux kernel, but by providing this you are doing exactly opposite.
> >
> > No we don't want to manage clocks in user-space. This debugfs is meant
> > for developer who wants to debug APIs behavior in case something
> > doesn't work as expected. Debugfs should be off by default in
> > production images.
> >
> 
> Good that it's not used in production image. The clock layer has
> *sufficient* debugfs support that will *help in debugging*. So please drop this
> Xilinx specific clock debugfs.
> 
> --
> Regards,
> Sudeep

Ok. Will remove them in next version. Let me know if rest changes look ok and I can submit final version with suggested minor changes.

Thanks,
Jolly Shah

^ permalink raw reply

* [PATCH 2/2] ARM: shmobile: only call secure_cntvoff_init on SMP builds
From: Geert Uytterhoeven @ 2018-05-25 19:12 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525161051.187324-2-arnd@arndb.de>

Hi Arnd,

On Fri, May 25, 2018 at 6:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> The secure_cntvoff_init() function is not available without CONFIG_SMP,
> leading to a link error on shmobile:
>
> arch/arm/mach-shmobile/setup-rcar-gen2.o: In function `rcar_gen2_timer_init':
> setup-rcar-gen2.c:(.init.text+0x18): undefined reference to `secure_cntvoff_init'
>
> From the description in commit 3fd45a136ff6 ("ARM: shmobile: rcar-gen2:
> Make sure CNTVOFF is initialized on CA7/15"), I understand that we
> don't need to call it on non-SMP builds because the boot CPU is always
> initialized by common code, so we can simply avoid the reference by
> checking for CONFIG_SMP.
>
> Fixes: cad160ed0a94 ("ARM: shmobile: Convert file to use cntvoff")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

NAKed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> @@ -71,7 +71,8 @@ void __init rcar_gen2_timer_init(void)
>         void __iomem *base;
>         u32 freq;
>
> -       secure_cntvoff_init();
> +       if (IS_ENABLED(CONFIG_SMP))
> +               secure_cntvoff_init();

The call here is for the boot CPU, since commit 9ce3fa6816c2fb59 ("ARM:
shmobile: rcar-gen2: Add CA7 arch_timer initialization for r8a7794"), and
modified and consolidated in commit 3fd45a136ff6 mentioned above.

The call for secondary CPUs is in arch/arm/mach-shmobile/headsmp-apmu.S.

>
>         if (of_machine_is_compatible("renesas,r8a7745") ||
>             of_machine_is_compatible("renesas,r8a77470") ||

So the proper fix is to build arch/arm/common/secure_cntvoff.o
unconditionally.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* [PATCH 1/6] coresight: remove CORESIGHT_LINKS_AND_SINKS dependencies and selections
From: Mathieu Poirier @ 2018-05-25 19:09 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525135227.ca286a1ba942bc7a28bb5686@arm.com>

On 25 May 2018 at 12:52, Kim Phillips <kim.phillips@arm.com> wrote:
> On Fri, 25 May 2018 09:27:09 -0600
> Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>
>> On 24 May 2018 at 17:30, Kim Phillips <kim.phillips@arm.com> wrote:
>> > On Thu, 24 May 2018 09:32:48 -0600
>> > Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>> >
>> >> On 23 May 2018 at 13:51, Kim Phillips <kim.phillips@arm.com> wrote:
>> >> > On Tue, 22 May 2018 11:31:40 -0600
>> >> > Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>> >> >
>> >> >> On Thu, May 17, 2018 at 08:20:19PM -0500, Kim Phillips wrote:
>> >> >> > A coresight topology doesn't need to include links, i.e., a source can
>> >> >> > be directly connected to a sink.  As such, selecting and/or depending on
>> >> >> > LINKS_AND_SINKS is no longer needed.
>> >> >>
>> >> >> I'm good with this patch but now the help text for CORESIGHT_LINKS_AND_SINKS no
>> >> >> longer match what the config does.  I see two ways to fix this:
>> >> >
>> >> > This patch doesn't change what the config does, it just changes what
>> >> > other config options depend on it.
>> >> >
>> >> >> 1) Rework the help text.
>> >> >
>> >> > I don't see how, given the above.  Here's the text:
>> >> >
>> >> > config CORESIGHT_LINKS_AND_SINKS
>> >> >         bool "CoreSight Link and Sink drivers"
>> >> >         help
>> >> >           This enables support for CoreSight link and sink drivers that are
>> >> >           responsible for transporting and collecting the trace data
>> >> >           respectively.  Link and sinks are dynamically aggregated with a trace
>> >> >           entity at run time to form a complete trace path.
>> >> >
>> >> > What part of that becomes invalid with this patch?
>> >>
>> >> Looking at the new Kconfig, what sink component depend on
>> >> CORESIGHT_LINKS_AND_SINKS?
>> >
>> > How does that affect the description text?  The description text
>> > doesn't insinuate any implicit dependencies or non-.
>>
>> Now that the depends are gone there is no correlation between this
>> config and sinks.
>
> There never was:  LINKS_AND_SINKS got introduced here:
>
> commit 6e21e3451556af6ada01e2206d5949fc654d75e1
> Author: Pratik Patel <pratikp@codeaurora.org>
> Date:   Mon Nov 3 11:07:39 2014 -0700
>
>     coresight-funnel: add CoreSight Funnel driver
>
>     This driver manages CoreSight Funnel which acts as a link.
>     Funnels have multiple input ports (typically 8) each of which
>     represents an input trace data stream. These multiple input trace
>     data streams are interleaved into a single output stream coming
>     out of the Funnel.
>
>     Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
>     Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> diff --git a/drivers/coresight/Makefile b/drivers/coresight/Makefile
> index 4c43dade4c14..05b8a1c75f73 100644
> --- a/drivers/coresight/Makefile
> +++ b/drivers/coresight/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_OF) += of_coresight.o
>  obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
>  obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
>  obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
> +obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o
>
> Then the replicator driver (again, not a sink), got tacked on to be
> built under LINKS_AND_SINKS here:
>
> commit ceacc1d9b7ae41e4be185596306be17537682fb1
> Author: Pratik Patel <pratikp@codeaurora.org>
> Date:   Mon Nov 3 11:07:40 2014 -0700
>
>     coresight-replicator: add CoreSight Replicator driver
>
>     This driver manages non-configurable CoreSight Replicator that
>     takes a single input trace data stream and replicates it to
>     produce two identical trace data output streams. Replicators
>     are typically used to route single interleaved trace data
>     stream to two or more sinks.
>
>     Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
>     Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> diff --git a/drivers/coresight/Makefile b/drivers/coresight/Makefile
> index 05b8a1c75f73..574d5fa496fa 100644
> --- a/drivers/coresight/Makefile
> +++ b/drivers/coresight/Makefile
> @@ -6,4 +6,5 @@ obj-$(CONFIG_OF) += of_coresight.o
>  obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
>  obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
>  obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
> -obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o
> +obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
> +                                          coresight-replicator.o
>
> Then, finally, the first 'depends on' came with the introduction of the
> first TMC driver, which used the config symbol
> CORESIGHT_LINK_AND_SINK_TMC, because the TMC can be configured as a
> link or as a sink:
>
> commit bc4bf7fe98daf4e64cc5ffc6cdc0e820f4d99c14
> Author: Pratik Patel <pratikp@codeaurora.org>
> Date:   Mon Nov 3 11:07:36 2014 -0700
>
>     coresight-tmc: add CoreSight TMC driver
>
>     This driver manages CoreSight TMC (Trace Memory Controller) which
>     can act as a link or a sink depending upon its configuration. It
>     can present itself as an ETF (Embedded Trace FIFO) or ETR
>     (Embedded Trace Router).
>
>     ETF when configured in circular buffer mode acts as a trace
>     collection sink. When configured in HW fifo mode it acts as link.
>     ETR always acts as a sink and can be used to route data to memory
>     allocated in RAM.
>
>     Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
>     Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index cd3890e3110e..092b6728af55 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -1340,4 +1340,24 @@ menuconfig CORESIGHT
>           a topological view of the CoreSight components based on a DT
>           specification and configure the right serie of components when a
>           trace source gets enabled.
> +
> +if CORESIGHT
> +config CORESIGHT_LINKS_AND_SINKS
> +       bool "CoreSight Link and Sink drivers"
> +       help
> +         This enables support for CoreSight link and sink drivers that are
> +         responsible for transporting and collecting the trace data
> +         respectively.  Link and sinks are dynamically aggregated with a trace
> +         entity at run time to form a complete trace path.
> +
> +config CORESIGHT_LINK_AND_SINK_TMC
> +       bool "Coresight generic TMC driver"
> +       depends on CORESIGHT_LINKS_AND_SINKS
> +       help
> +         This enables support for the Trace Memory Controller driver.  Depending
> +         on its configuration the device can act as a link (embedded trace router
> +         - ETR) or sink (embedded trace FIFO).  The driver complies with the
> +         generic implementation of the component without special enhancement or
> +         added features.
> +endif
>  endmenu
>
> So LINKS_AND_SINKS never actually built a sink driver by itself.  Since
> all the above commits appear to come from the same series, I'm guessing
> the CORESIGHT_LINKS_AND_SINKS name was from a decision made during
> development, where indeed sinks may have been built under the name.

Sinks were never built under that config.  It is there to highlight
the fact that sinks can't operate without links.  But that was under
previous designs where sources were never directly connected to sinks.

>
> We can argue semantics over the 'depends on' causality, but the original
> text description suggests that sink drivers were indeed being build
> under the name, because it never explicitly said that sink drivers need
> to depend on it.
>
> So I don't think the rename change is warranted due to the lift of the
> 'depends on' clauses.  I think the LINKS_AND_SINKS name was initially
> incorrect, and is a remnant of an oversight when cleaning up a
> development version for submission upstream.  A rename patch to fix
> it can be done, but I think it adds undue complication, and is
> completely unrelated to this modularization series.
>
>> >> config CORESIGHT_LINKS
>> >
>> > Please, not another gratuitous config name change, I've already
>> > experienced usage regressions from the CORESIGHT_QCOM_REPLICATOR =>
>> > CORESIGHT_DYNAMIC_REPLICATOR change:
>>
>> Defines within subsystems are bound to change as they evolves.  Most
>> of the CoreSight subsystem was developed on the OMAP3 based
>> beagleboard.  Since then new topologies have emerged and new IP blocks
>> came along.  It is only normal that we adjust configuration options to
>> reflect the reality of the HW the subsystem is managing.  I can guide
>> you through the history of the replicator config name change if you
>> want - it is quite logical.  Other than that and until this patchset,
>> we haven't modified a single configuration in the 5 years the
>> subsystem has existed.  Not bad for all the churn and new IP blocks
>> that came in.
>>
>> >
>> > https://patchwork.kernel.org/patch/10206023/
>> >
>> >>          bool "CoreSight Link drivers"
>> >>          help
>> >>            This enables support for CoreSight link drivers that are responsible
>> >>            for transporting trace data from source to sink.  Links are
>> >> dynamically
>> >>            aggregated with other traces entities at run time to form a
>> >> complete trace
>> >>            path.
>> >
>> > Oh, I see, so your point is that LINKS_AND_SINKS doesn't technically
>> > build any sink drivers?  That's completely orthogonal to removing a
>> > dependency chain:  that just tells me the name was a poor choice in the
>> > first place maybe?  I don't see where the Makefile may have built a
>> > sink, but it may be before the move to drivers/hwtracing/coresight, or
>> > some other reorganization.
>>
>> Because of the depends property carried by the sink drivers (which we
>> are now removing), defining CORESIGHT_LINKS_AND_SINKS was mandatory to
>> build sink drivers.  That was accurate 5 years ago with the topologies
>> that were available at that time.  Now there is no point in having the
>> define, which is why I'm asking you to make this modification.
>
> See above for my own analysis of the history; it should have been
> CORESIGHT_LINKS from the very beginning, and the description was
> inaccurate also from the beginning.  Lifting the 'depends on' doesn't
> necessitate globbing in a gratuitous naming and description fix in this
> patch.
>
>> >> >> 2) Rework CORESIGHT_LINKS_AND_SINKS to be CORESIGHT_FUNNEL and move
>> >> >> coresight-replicator.o under CORESIGHT_DYNAMIC_REPLICATOR in the Makefile. I
>> >> >> really liked your idea of making the replicator driver intelligent enough to
>> >> >> deal with both DT and platform declaration, which merges two driver into one.
>> >> >>
>> >> >> I'm obviously favouring the second option but recognise it doesn't have to be
>> >> >> part of this patchet.  So for this set please rework the help text for
>> >> >> CORESIGHT_LINKS_AND_SINKS.  Once we've dealt with this topic we can refactor the
>> >> >> replicator driver.
>> >> >
>> >> > I'd really like to just focus on getting CoreSight to load as modules,
>> >> > something for which this patch isn't technically required...
>> >>
>> >> The only thing I'm asking is that the config description and help text
>> >> reflect what the Makefile does.
>> >
>> > argh, wellll, it's a completely different change, and we're now
>> > completely off the modularization topic, and I'm uncomfortable doing
>>
>> I don't agree with you.  This is a very simple change and I even wrote
>> down what needed to be modified.
>>
>> > reorgs on things I don't understand, renaming CONFIG_s, esp. when
>> > others such as the REPLICATOR, since as far as I know, that's also a
>> > link??
>>
>> Correct, a replicator is a link and completely removed from this conversation.
>>
>> If this is so hard for you then simply don't make the modification - I
>> will do it myself, something that will take me about 10 minutes
>> (including writing the changelog).
>
> OK, cool, thanks and sorry, but, like I said, I don't think the rename
> belongs in either this 'depends on' lift one-off patch (that I'm already
> uncomfortable with), let alone in this modularization series.

Don't do anything, just leave things the way they are - I will deal with it.

>
> Because I consider it gratuitous, I think the rename ought to come at a
> time where another more purposeful rename occurs, i.e., in addition to
> e.g., a replicator driver reorg.
>
> Just my 2c.
>
> Kim

^ permalink raw reply

* [PATCH 1/6] coresight: remove CORESIGHT_LINKS_AND_SINKS dependencies and selections
From: Kim Phillips @ 2018-05-25 18:52 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CANLsYkwZtAptXtLK0HaDwmXbdAx24X+PEFVggakxe31nkH111g@mail.gmail.com>

On Fri, 25 May 2018 09:27:09 -0600
Mathieu Poirier <mathieu.poirier@linaro.org> wrote:

> On 24 May 2018 at 17:30, Kim Phillips <kim.phillips@arm.com> wrote:
> > On Thu, 24 May 2018 09:32:48 -0600
> > Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> >
> >> On 23 May 2018 at 13:51, Kim Phillips <kim.phillips@arm.com> wrote:
> >> > On Tue, 22 May 2018 11:31:40 -0600
> >> > Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> >> >
> >> >> On Thu, May 17, 2018 at 08:20:19PM -0500, Kim Phillips wrote:
> >> >> > A coresight topology doesn't need to include links, i.e., a source can
> >> >> > be directly connected to a sink.  As such, selecting and/or depending on
> >> >> > LINKS_AND_SINKS is no longer needed.
> >> >>
> >> >> I'm good with this patch but now the help text for CORESIGHT_LINKS_AND_SINKS no
> >> >> longer match what the config does.  I see two ways to fix this:
> >> >
> >> > This patch doesn't change what the config does, it just changes what
> >> > other config options depend on it.
> >> >
> >> >> 1) Rework the help text.
> >> >
> >> > I don't see how, given the above.  Here's the text:
> >> >
> >> > config CORESIGHT_LINKS_AND_SINKS
> >> >         bool "CoreSight Link and Sink drivers"
> >> >         help
> >> >           This enables support for CoreSight link and sink drivers that are
> >> >           responsible for transporting and collecting the trace data
> >> >           respectively.  Link and sinks are dynamically aggregated with a trace
> >> >           entity at run time to form a complete trace path.
> >> >
> >> > What part of that becomes invalid with this patch?
> >>
> >> Looking at the new Kconfig, what sink component depend on
> >> CORESIGHT_LINKS_AND_SINKS?
> >
> > How does that affect the description text?  The description text
> > doesn't insinuate any implicit dependencies or non-.
> 
> Now that the depends are gone there is no correlation between this
> config and sinks.

There never was:  LINKS_AND_SINKS got introduced here:

commit 6e21e3451556af6ada01e2206d5949fc654d75e1
Author: Pratik Patel <pratikp@codeaurora.org>
Date:   Mon Nov 3 11:07:39 2014 -0700

    coresight-funnel: add CoreSight Funnel driver
    
    This driver manages CoreSight Funnel which acts as a link.
    Funnels have multiple input ports (typically 8) each of which
    represents an input trace data stream. These multiple input trace
    data streams are interleaved into a single output stream coming
    out of the Funnel.
    
    Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
    Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/coresight/Makefile b/drivers/coresight/Makefile
index 4c43dade4c14..05b8a1c75f73 100644
--- a/drivers/coresight/Makefile
+++ b/drivers/coresight/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_OF) += of_coresight.o
 obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
 obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
 obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
+obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o

Then the replicator driver (again, not a sink), got tacked on to be
built under LINKS_AND_SINKS here:

commit ceacc1d9b7ae41e4be185596306be17537682fb1
Author: Pratik Patel <pratikp@codeaurora.org>
Date:   Mon Nov 3 11:07:40 2014 -0700

    coresight-replicator: add CoreSight Replicator driver
    
    This driver manages non-configurable CoreSight Replicator that
    takes a single input trace data stream and replicates it to
    produce two identical trace data output streams. Replicators
    are typically used to route single interleaved trace data
    stream to two or more sinks.
    
    Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
    Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/coresight/Makefile b/drivers/coresight/Makefile
index 05b8a1c75f73..574d5fa496fa 100644
--- a/drivers/coresight/Makefile
+++ b/drivers/coresight/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_OF) += of_coresight.o
 obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
 obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
 obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
-obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o
+obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
+                                          coresight-replicator.o

Then, finally, the first 'depends on' came with the introduction of the
first TMC driver, which used the config symbol
CORESIGHT_LINK_AND_SINK_TMC, because the TMC can be configured as a
link or as a sink:

commit bc4bf7fe98daf4e64cc5ffc6cdc0e820f4d99c14
Author: Pratik Patel <pratikp@codeaurora.org>
Date:   Mon Nov 3 11:07:36 2014 -0700

    coresight-tmc: add CoreSight TMC driver
    
    This driver manages CoreSight TMC (Trace Memory Controller) which
    can act as a link or a sink depending upon its configuration. It
    can present itself as an ETF (Embedded Trace FIFO) or ETR
    (Embedded Trace Router).
    
    ETF when configured in circular buffer mode acts as a trace
    collection sink. When configured in HW fifo mode it acts as link.
    ETR always acts as a sink and can be used to route data to memory
    allocated in RAM.
    
    Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
    Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index cd3890e3110e..092b6728af55 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -1340,4 +1340,24 @@ menuconfig CORESIGHT
          a topological view of the CoreSight components based on a DT
          specification and configure the right serie of components when a
          trace source gets enabled.
+
+if CORESIGHT
+config CORESIGHT_LINKS_AND_SINKS
+       bool "CoreSight Link and Sink drivers"
+       help
+         This enables support for CoreSight link and sink drivers that are
+         responsible for transporting and collecting the trace data
+         respectively.  Link and sinks are dynamically aggregated with a trace
+         entity at run time to form a complete trace path.
+
+config CORESIGHT_LINK_AND_SINK_TMC
+       bool "Coresight generic TMC driver"
+       depends on CORESIGHT_LINKS_AND_SINKS
+       help
+         This enables support for the Trace Memory Controller driver.  Depending
+         on its configuration the device can act as a link (embedded trace router
+         - ETR) or sink (embedded trace FIFO).  The driver complies with the
+         generic implementation of the component without special enhancement or
+         added features.
+endif
 endmenu

So LINKS_AND_SINKS never actually built a sink driver by itself.  Since
all the above commits appear to come from the same series, I'm guessing
the CORESIGHT_LINKS_AND_SINKS name was from a decision made during
development, where indeed sinks may have been built under the name.

We can argue semantics over the 'depends on' causality, but the original
text description suggests that sink drivers were indeed being build
under the name, because it never explicitly said that sink drivers need
to depend on it.

So I don't think the rename change is warranted due to the lift of the
'depends on' clauses.  I think the LINKS_AND_SINKS name was initially
incorrect, and is a remnant of an oversight when cleaning up a
development version for submission upstream.  A rename patch to fix
it can be done, but I think it adds undue complication, and is
completely unrelated to this modularization series. 

> >> config CORESIGHT_LINKS
> >
> > Please, not another gratuitous config name change, I've already
> > experienced usage regressions from the CORESIGHT_QCOM_REPLICATOR =>
> > CORESIGHT_DYNAMIC_REPLICATOR change:
> 
> Defines within subsystems are bound to change as they evolves.  Most
> of the CoreSight subsystem was developed on the OMAP3 based
> beagleboard.  Since then new topologies have emerged and new IP blocks
> came along.  It is only normal that we adjust configuration options to
> reflect the reality of the HW the subsystem is managing.  I can guide
> you through the history of the replicator config name change if you
> want - it is quite logical.  Other than that and until this patchset,
> we haven't modified a single configuration in the 5 years the
> subsystem has existed.  Not bad for all the churn and new IP blocks
> that came in.
> 
> >
> > https://patchwork.kernel.org/patch/10206023/
> >
> >>          bool "CoreSight Link drivers"
> >>          help
> >>            This enables support for CoreSight link drivers that are responsible
> >>            for transporting trace data from source to sink.  Links are
> >> dynamically
> >>            aggregated with other traces entities at run time to form a
> >> complete trace
> >>            path.
> >
> > Oh, I see, so your point is that LINKS_AND_SINKS doesn't technically
> > build any sink drivers?  That's completely orthogonal to removing a
> > dependency chain:  that just tells me the name was a poor choice in the
> > first place maybe?  I don't see where the Makefile may have built a
> > sink, but it may be before the move to drivers/hwtracing/coresight, or
> > some other reorganization.
> 
> Because of the depends property carried by the sink drivers (which we
> are now removing), defining CORESIGHT_LINKS_AND_SINKS was mandatory to
> build sink drivers.  That was accurate 5 years ago with the topologies
> that were available at that time.  Now there is no point in having the
> define, which is why I'm asking you to make this modification.

See above for my own analysis of the history; it should have been
CORESIGHT_LINKS from the very beginning, and the description was
inaccurate also from the beginning.  Lifting the 'depends on' doesn't
necessitate globbing in a gratuitous naming and description fix in this
patch.

> >> >> 2) Rework CORESIGHT_LINKS_AND_SINKS to be CORESIGHT_FUNNEL and move
> >> >> coresight-replicator.o under CORESIGHT_DYNAMIC_REPLICATOR in the Makefile. I
> >> >> really liked your idea of making the replicator driver intelligent enough to
> >> >> deal with both DT and platform declaration, which merges two driver into one.
> >> >>
> >> >> I'm obviously favouring the second option but recognise it doesn't have to be
> >> >> part of this patchet.  So for this set please rework the help text for
> >> >> CORESIGHT_LINKS_AND_SINKS.  Once we've dealt with this topic we can refactor the
> >> >> replicator driver.
> >> >
> >> > I'd really like to just focus on getting CoreSight to load as modules,
> >> > something for which this patch isn't technically required...
> >>
> >> The only thing I'm asking is that the config description and help text
> >> reflect what the Makefile does.
> >
> > argh, wellll, it's a completely different change, and we're now
> > completely off the modularization topic, and I'm uncomfortable doing
> 
> I don't agree with you.  This is a very simple change and I even wrote
> down what needed to be modified.
> 
> > reorgs on things I don't understand, renaming CONFIG_s, esp. when
> > others such as the REPLICATOR, since as far as I know, that's also a
> > link??
> 
> Correct, a replicator is a link and completely removed from this conversation.
> 
> If this is so hard for you then simply don't make the modification - I
> will do it myself, something that will take me about 10 minutes
> (including writing the changelog).

OK, cool, thanks and sorry, but, like I said, I don't think the rename
belongs in either this 'depends on' lift one-off patch (that I'm already
uncomfortable with), let alone in this modularization series.

Because I consider it gratuitous, I think the rename ought to come at a
time where another more purposeful rename occurs, i.e., in addition to
e.g., a replicator driver reorg.

Just my 2c.

Kim

^ permalink raw reply related

* [PATCH v2] arm64: dts: qcom: msm8996: Move UFS_GDSC to UFS HCD
From: Bjorn Andersson @ 2018-05-25 18:45 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180524223122.12601-1-bjorn.andersson@linaro.org>

The UFS_GDSC backs the resources needed by the UFS HCD, rather than the
PHY. This results in the UFS HCD occationally failing to enable some of
its clocks.

Move the UFS_GDSC reference to the UFS HCD node instead, to correct
this.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- Dropped UFS_GDSC from phy node

 arch/arm64/boot/dts/qcom/msm8996.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 380e14591686..8c7f9ca25b53 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -654,8 +654,6 @@
 			clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
 				 <&gcc GCC_UFS_CLKREF_CLK>;
 			status = "disabled";
-
-			power-domains = <&gcc UFS_GDSC>;
 		};
 
 		ufshc at 624000 {
@@ -674,6 +672,8 @@
 			vccq-max-microamp = <450000>;
 			vccq2-max-microamp = <450000>;
 
+			power-domains = <&gcc UFS_GDSC>;
+
 			clock-names =
 				"core_clk_src",
 				"core_clk",
-- 
2.17.0

^ permalink raw reply related

* [PATCH] arm64: dts: qcom: msm8996: Use UFS_GDSC for UFS
From: Bjorn Andersson @ 2018-05-25 18:35 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAFp+6iH5bS5rmqZ1oy09COb2Rzuz6zbN7pFwxz0WTJNOYTTbkg@mail.gmail.com>

On Fri 25 May 05:51 PDT 2018, Vivek Gautam wrote:

> Hi Bjorn,
> 
> On Fri, May 25, 2018 at 4:01 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > The UFS host controller occationally (20%) fails to enable
> > gcc_ufs_axi_clk because the UFS GDSC is not enabled. In most cases it's
> > enabled through the UFS phy driver, but to make sure it's enabled let's
> > enable it directly from the UFS host controller directly as well.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/msm8996.dtsi | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> > index 380e14591686..03c7904bda14 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> > @@ -674,6 +674,8 @@
> >                         vccq-max-microamp = <450000>;
> >                         vccq2-max-microamp = <450000>;
> >
> > +                       power-domains = <&gcc UFS_GDSC>;
> > +
> 
> We shouldn't need power-domain with the phy. UFS_GDSC should
> be attached to the controller, as the phy is powered up only after
> the controller is power-up, and during collapse too, we turn off
> the phy first.

Afaict you're right, it should only be needed by the resources for the
HCD.

> Can you try testing keeping UFS_GDSC only with ufs controller and
> remove it from the ufs-phy node? We are doing same on the 4.14 release
> branch too for db820.

I test booted this 10 times, with 100% success :)

> I apologize to have missed this in your patch for ufs-related dt nodes.
> Can we please fix this now?

I'll send a v2, that moves the power-domain to the HCD, rather than just
adding it there too. Thanks for the quick review!

Regards,
Bjorn

^ permalink raw reply

* Re: [PATCH v11 00/27] ARM: davinci: convert to common clock framework​
From: David Lechner @ 2018-05-25 18:21 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <850a0ec9-53aa-02a5-ecbd-e068b13b2764@ti.com>

On 05/22/2018 04:38 AM, Sekhar Nori wrote:
> Hi David,
> 
> On Friday 18 May 2018 10:18 PM, David Lechner wrote:
>> This series converts mach-davinci to use the common clock framework.
>>
>> The series works like this, the first 3 patches fix some issues with the clock
>> drivers that have already been accepted into the mainline kernel.
>>
>> Then, starting with "ARM: davinci: pass clock as parameter to
>> davinci_timer_init()", we get the mach code ready for the switch by adding the
>> code needed for the new clock drivers and adding #ifndef CONFIG_COMMON_CLK
>> around the legacy clocks so that we can switch easily between the old and the
>> new.
>>
>> "ARM: davinci: switch to common clock framework" actually flips the switch
>> to start using the new clock drivers. Then the next 8 patches remove all
>> of the old clock code.
>>
>> The final four patches add device tree clock support to the one SoC that
>> supports it.
>>
>> This series has been tested on TI OMAP-L138 LCDK (both device tree and legacy
>> board file).
> 
> If you do end up sending a v12, you can leave out the mach-davinci
> portions unless there are any changes you need to make. I will pick them
> up from this series once the driver dependencies are merged.
> 
> I do hope the drivers/clk/* changes can be merged from v4.18.
> 

I have resent all of the clk patches (including all of the ones I listed as
dependencies in addition to the three remaining in this series) under the
cover "clk: davinci: outstanding fixes?".

I also found that we need to add power-domains properties to the PWM nodes
in "ARM: dts: da850: Add clocks". I probably should just take your advice
and just globally added them even if they are not documented for some types
ofnodes.

^ permalink raw reply

* [PATCH 9/9] clk: davinci: Fix link errors when not all SoCs are enabled
From: David Lechner @ 2018-05-25 18:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525181150.17873-1-david@lechnology.com>

This fixes linker errors due to undefined symbols when one or more of
the TI DaVinci SoCs is not enabled in the kernel config.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/clk/davinci/pll.c   | 16 ++++++++++++++++
 drivers/clk/davinci/pll.h   | 11 ++++++++---
 drivers/clk/davinci/psc.c   | 14 ++++++++++++++
 drivers/clk/davinci/psc.h   | 12 ++++++++++++
 include/linux/clk/davinci.h | 19 +++++++++++++++----
 5 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/davinci/pll.c b/drivers/clk/davinci/pll.c
index 84a343060bc8..65abd371692d 100644
--- a/drivers/clk/davinci/pll.c
+++ b/drivers/clk/davinci/pll.c
@@ -860,25 +860,41 @@ static struct davinci_pll_platform_data *davinci_pll_get_pdata(struct device *de
 }
 
 /* needed in early boot for clocksource/clockevent */
+#ifdef CONFIG_ARCH_DAVINCI_DA850
 CLK_OF_DECLARE(da850_pll0, "ti,da850-pll0", of_da850_pll0_init);
+#endif
 
 static const struct of_device_id davinci_pll_of_match[] = {
+#ifdef CONFIG_ARCH_DAVINCI_DA850
 	{ .compatible = "ti,da850-pll1", .data = of_da850_pll1_init },
+#endif
 	{ }
 };
 
 static const struct platform_device_id davinci_pll_id_table[] = {
+#ifdef CONFIG_ARCH_DAVINCI_DA830
 	{ .name = "da830-pll",   .driver_data = (kernel_ulong_t)da830_pll_init   },
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DA850
 	{ .name = "da850-pll0",  .driver_data = (kernel_ulong_t)da850_pll0_init  },
 	{ .name = "da850-pll1",  .driver_data = (kernel_ulong_t)da850_pll1_init  },
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM355
 	{ .name = "dm355-pll1",  .driver_data = (kernel_ulong_t)dm355_pll1_init  },
 	{ .name = "dm355-pll2",  .driver_data = (kernel_ulong_t)dm355_pll2_init  },
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM365
 	{ .name = "dm365-pll1",  .driver_data = (kernel_ulong_t)dm365_pll1_init  },
 	{ .name = "dm365-pll2",  .driver_data = (kernel_ulong_t)dm365_pll2_init  },
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM644x
 	{ .name = "dm644x-pll1", .driver_data = (kernel_ulong_t)dm644x_pll1_init },
 	{ .name = "dm644x-pll2", .driver_data = (kernel_ulong_t)dm644x_pll2_init },
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM646x
 	{ .name = "dm646x-pll1", .driver_data = (kernel_ulong_t)dm646x_pll1_init },
 	{ .name = "dm646x-pll2", .driver_data = (kernel_ulong_t)dm646x_pll2_init },
+#endif
 	{ }
 };
 
diff --git a/drivers/clk/davinci/pll.h b/drivers/clk/davinci/pll.h
index b2e5c4496645..7cc354dd29e2 100644
--- a/drivers/clk/davinci/pll.h
+++ b/drivers/clk/davinci/pll.h
@@ -122,14 +122,19 @@ int of_davinci_pll_init(struct device *dev, struct device_node *node,
 
 /* Platform-specific callbacks */
 
+#ifdef CONFIG_ARCH_DAVINCI_DA850
 int da850_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 void of_da850_pll0_init(struct device_node *node);
 int of_da850_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
-
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM355
 int dm355_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
-
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM644x
 int dm644x_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
-
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM646x
 int dm646x_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+#endif
 
 #endif /* __CLK_DAVINCI_PLL_H___ */
diff --git a/drivers/clk/davinci/psc.c b/drivers/clk/davinci/psc.c
index 6326ba1fe3cc..fffbed5e263b 100644
--- a/drivers/clk/davinci/psc.c
+++ b/drivers/clk/davinci/psc.c
@@ -513,20 +513,34 @@ int of_davinci_psc_clk_init(struct device *dev,
 }
 
 static const struct of_device_id davinci_psc_of_match[] = {
+#ifdef CONFIG_ARCH_DAVINCI_DA850
 	{ .compatible = "ti,da850-psc0", .data = &of_da850_psc0_init_data },
 	{ .compatible = "ti,da850-psc1", .data = &of_da850_psc1_init_data },
+#endif
 	{ }
 };
 
 static const struct platform_device_id davinci_psc_id_table[] = {
+#ifdef CONFIG_ARCH_DAVINCI_DA830
 	{ .name = "da830-psc0", .driver_data = (kernel_ulong_t)&da830_psc0_init_data },
 	{ .name = "da830-psc1", .driver_data = (kernel_ulong_t)&da830_psc1_init_data },
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DA850
 	{ .name = "da850-psc0", .driver_data = (kernel_ulong_t)&da850_psc0_init_data },
 	{ .name = "da850-psc1", .driver_data = (kernel_ulong_t)&da850_psc1_init_data },
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM355
 	{ .name = "dm355-psc",  .driver_data = (kernel_ulong_t)&dm355_psc_init_data  },
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM365
 	{ .name = "dm365-psc",  .driver_data = (kernel_ulong_t)&dm365_psc_init_data  },
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM644x
 	{ .name = "dm644x-psc", .driver_data = (kernel_ulong_t)&dm644x_psc_init_data },
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM646x
 	{ .name = "dm646x-psc", .driver_data = (kernel_ulong_t)&dm646x_psc_init_data },
+#endif
 	{ }
 };
 
diff --git a/drivers/clk/davinci/psc.h b/drivers/clk/davinci/psc.h
index c2a7df6413fe..6a42529d31a9 100644
--- a/drivers/clk/davinci/psc.h
+++ b/drivers/clk/davinci/psc.h
@@ -94,15 +94,27 @@ struct davinci_psc_init_data {
 	int (*psc_init)(struct device *dev, void __iomem *base);
 };
 
+#ifdef CONFIG_ARCH_DAVINCI_DA830
 extern const struct davinci_psc_init_data da830_psc0_init_data;
 extern const struct davinci_psc_init_data da830_psc1_init_data;
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DA850
 extern const struct davinci_psc_init_data da850_psc0_init_data;
 extern const struct davinci_psc_init_data da850_psc1_init_data;
 extern const struct davinci_psc_init_data of_da850_psc0_init_data;
 extern const struct davinci_psc_init_data of_da850_psc1_init_data;
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM355
 extern const struct davinci_psc_init_data dm355_psc_init_data;
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM356
 extern const struct davinci_psc_init_data dm365_psc_init_data;
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM644x
 extern const struct davinci_psc_init_data dm644x_psc_init_data;
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM646x
 extern const struct davinci_psc_init_data dm646x_psc_init_data;
+#endif
 
 #endif /* __CLK_DAVINCI_PSC_H__ */
diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h
index 62764c5cc86e..8a7b5cd7eac0 100644
--- a/include/linux/clk/davinci.h
+++ b/include/linux/clk/davinci.h
@@ -13,17 +13,28 @@
 
 /* function for registering clocks in early boot */
 
+#ifdef CONFIG_ARCH_DAVINCI_DA830
 int da830_pll_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DA850
 int da850_pll0_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM355
 int dm355_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+int dm355_psc_init(struct device *dev, void __iomem *base);
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM365
 int dm365_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 int dm365_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
-int dm644x_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
-int dm646x_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
-
-int dm355_psc_init(struct device *dev, void __iomem *base);
 int dm365_psc_init(struct device *dev, void __iomem *base);
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM644x
+int dm644x_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 int dm644x_psc_init(struct device *dev, void __iomem *base);
+#endif
+#ifdef CONFIG_ARCH_DAVINCI_DM646x
+int dm646x_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 int dm646x_psc_init(struct device *dev, void __iomem *base);
+#endif
 
 #endif /* __LINUX_CLK_DAVINCI_PLL_H___ */
-- 
2.17.0

^ permalink raw reply related

* [PATCH 8/9] clk: davinci: psc: allow for dev == NULL
From: David Lechner @ 2018-05-25 18:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525181150.17873-1-david@lechnology.com>

On some davinci SoCs, we need to register the PSC clocks during early
boot because they are needed for clocksource/clockevent. These changes
allow for dev == NULL because in this case, we won't have a platform
device for the clocks.

Signed-off-by: David Lechner <david@lechnology.com>
Reviewed-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/clk/davinci/psc-dm355.c  |  3 +-
 drivers/clk/davinci/psc-dm365.c  |  3 +-
 drivers/clk/davinci/psc-dm644x.c |  3 +-
 drivers/clk/davinci/psc-dm646x.c |  3 +-
 drivers/clk/davinci/psc.c        | 58 ++++++++++++++++++++++++--------
 include/linux/clk/davinci.h      |  5 +++
 6 files changed, 57 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/davinci/psc-dm355.c b/drivers/clk/davinci/psc-dm355.c
index 128e7345b20c..ddd250107c4e 100644
--- a/drivers/clk/davinci/psc-dm355.c
+++ b/drivers/clk/davinci/psc-dm355.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/clk/davinci.h>
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 #include <linux/init.h>
@@ -68,7 +69,7 @@ static const struct davinci_lpsc_clk_info dm355_psc_info[] = {
 	{ }
 };
 
-static int dm355_psc_init(struct device *dev, void __iomem *base)
+int dm355_psc_init(struct device *dev, void __iomem *base)
 {
 	return davinci_psc_register_clocks(dev, dm355_psc_info, 42, base);
 }
diff --git a/drivers/clk/davinci/psc-dm365.c b/drivers/clk/davinci/psc-dm365.c
index 289af3913fb0..8c73086cc676 100644
--- a/drivers/clk/davinci/psc-dm365.c
+++ b/drivers/clk/davinci/psc-dm365.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/clk/davinci.h>
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 #include <linux/init.h>
@@ -86,7 +87,7 @@ static const struct davinci_lpsc_clk_info dm365_psc_info[] = {
 	{ }
 };
 
-static int dm365_psc_init(struct device *dev, void __iomem *base)
+int dm365_psc_init(struct device *dev, void __iomem *base)
 {
 	return davinci_psc_register_clocks(dev, dm365_psc_info, 52, base);
 }
diff --git a/drivers/clk/davinci/psc-dm644x.c b/drivers/clk/davinci/psc-dm644x.c
index c22367baa46f..fc0230e3a3d6 100644
--- a/drivers/clk/davinci/psc-dm644x.c
+++ b/drivers/clk/davinci/psc-dm644x.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/clk/davinci.h>
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 #include <linux/init.h>
@@ -63,7 +64,7 @@ static const struct davinci_lpsc_clk_info dm644x_psc_info[] = {
 	{ }
 };
 
-static int dm644x_psc_init(struct device *dev, void __iomem *base)
+int dm644x_psc_init(struct device *dev, void __iomem *base)
 {
 	return davinci_psc_register_clocks(dev, dm644x_psc_info, 41, base);
 }
diff --git a/drivers/clk/davinci/psc-dm646x.c b/drivers/clk/davinci/psc-dm646x.c
index 468ef86ea40b..c3f82ed70a80 100644
--- a/drivers/clk/davinci/psc-dm646x.c
+++ b/drivers/clk/davinci/psc-dm646x.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/clk/davinci.h>
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 #include <linux/init.h>
@@ -58,7 +59,7 @@ static const struct davinci_lpsc_clk_info dm646x_psc_info[] = {
 	{ }
 };
 
-static int dm646x_psc_init(struct device *dev, void __iomem *base)
+int dm646x_psc_init(struct device *dev, void __iomem *base)
 {
 	return davinci_psc_register_clocks(dev, dm646x_psc_info, 46, base);
 }
diff --git a/drivers/clk/davinci/psc.c b/drivers/clk/davinci/psc.c
index ce170e600f09..6326ba1fe3cc 100644
--- a/drivers/clk/davinci/psc.c
+++ b/drivers/clk/davinci/psc.c
@@ -15,6 +15,7 @@
 
 #include <linux/clk-provider.h>
 #include <linux/clk.h>
+#include <linux/clk/davinci.h>
 #include <linux/clkdev.h>
 #include <linux/err.h>
 #include <linux/of_address.h>
@@ -63,7 +64,7 @@ struct davinci_psc_data {
 
 /**
  * struct davinci_lpsc_clk - LPSC clock structure
- * @dev: the device that provides this LPSC
+ * @dev: the device that provides this LPSC or NULL
  * @hw: clk_hw for the LPSC
  * @pm_domain: power domain for the LPSC
  * @genpd_clk: clock reference owned by @pm_domain
@@ -221,6 +222,7 @@ static void davinci_psc_genpd_detach_dev(struct generic_pm_domain *pm_domain,
 
 /**
  * davinci_lpsc_clk_register - register LPSC clock
+ * @dev: the clocks's device or NULL
  * @name: name of this clock
  * @parent_name: name of clock's parent
  * @regmap: PSC MMIO region
@@ -238,7 +240,7 @@ davinci_lpsc_clk_register(struct device *dev, const char *name,
 	int ret;
 	bool is_on;
 
-	lpsc = devm_kzalloc(dev, sizeof(*lpsc), GFP_KERNEL);
+	lpsc = kzalloc(sizeof(*lpsc), GFP_KERNEL);
 	if (!lpsc)
 		return ERR_PTR(-ENOMEM);
 
@@ -261,9 +263,15 @@ davinci_lpsc_clk_register(struct device *dev, const char *name,
 	lpsc->pd = pd;
 	lpsc->flags = flags;
 
-	ret = devm_clk_hw_register(dev, &lpsc->hw);
-	if (ret < 0)
+	ret = clk_hw_register(dev, &lpsc->hw);
+	if (ret < 0) {
+		kfree(lpsc);
 		return ERR_PTR(ret);
+	}
+
+	/* for now, genpd is only registered when using device-tree */
+	if (!dev || !dev->of_node)
+		return lpsc;
 
 	/* genpd attach needs a way to look up this clock */
 	ret = clk_hw_register_clkdev(&lpsc->hw, name, best_dev_name(dev));
@@ -378,13 +386,15 @@ __davinci_psc_register_clocks(struct device *dev,
 	struct regmap *regmap;
 	int i, ret;
 
-	psc = devm_kzalloc(dev, sizeof(*psc), GFP_KERNEL);
+	psc = kzalloc(sizeof(*psc), GFP_KERNEL);
 	if (!psc)
 		return ERR_PTR(-ENOMEM);
 
-	clks = devm_kmalloc_array(dev, num_clks, sizeof(*clks), GFP_KERNEL);
-	if (!clks)
-		return ERR_PTR(-ENOMEM);
+	clks = kmalloc_array(num_clks, sizeof(*clks), GFP_KERNEL);
+	if (!clks) {
+		ret = -ENOMEM;
+		goto err_free_psc;
+	}
 
 	psc->clk_data.clks = clks;
 	psc->clk_data.clk_num = num_clks;
@@ -396,16 +406,20 @@ __davinci_psc_register_clocks(struct device *dev,
 	for (i = 0; i < num_clks; i++)
 		clks[i] = ERR_PTR(-ENOENT);
 
-	pm_domains = devm_kcalloc(dev, num_clks, sizeof(*pm_domains), GFP_KERNEL);
-	if (!pm_domains)
-		return ERR_PTR(-ENOMEM);
+	pm_domains = kcalloc(num_clks, sizeof(*pm_domains), GFP_KERNEL);
+	if (!pm_domains) {
+		ret = -ENOMEM;
+		goto err_free_clks;
+	}
 
 	psc->pm_data.domains = pm_domains;
 	psc->pm_data.num_domains = num_clks;
 
-	regmap = devm_regmap_init_mmio(dev, base, &davinci_psc_regmap_config);
-	if (IS_ERR(regmap))
-		return ERR_CAST(regmap);
+	regmap = regmap_init_mmio(dev, base, &davinci_psc_regmap_config);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		goto err_free_pm_domains;
+	}
 
 	for (; info->name; info++) {
 		struct davinci_lpsc_clk *lpsc;
@@ -423,6 +437,13 @@ __davinci_psc_register_clocks(struct device *dev,
 		pm_domains[info->md] = &lpsc->pm_domain;
 	}
 
+	/*
+	 * for now, a reset controller is only registered when there is a device
+	 * to associate it with.
+	 */
+	if (!dev)
+		return psc;
+
 	psc->rcdev.ops = &davinci_psc_reset_ops;
 	psc->rcdev.owner = THIS_MODULE;
 	psc->rcdev.dev = dev;
@@ -436,6 +457,15 @@ __davinci_psc_register_clocks(struct device *dev,
 		dev_warn(dev, "Failed to register reset controller (%d)\n", ret);
 
 	return psc;
+
+err_free_pm_domains:
+	kfree(pm_domains);
+err_free_clks:
+	kfree(clks);
+err_free_psc:
+	kfree(psc);
+
+	return ERR_PTR(ret);
 }
 
 int davinci_psc_register_clocks(struct device *dev,
diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h
index ebdd9df1c0ef..62764c5cc86e 100644
--- a/include/linux/clk/davinci.h
+++ b/include/linux/clk/davinci.h
@@ -21,4 +21,9 @@ int dm365_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgch
 int dm644x_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 int dm646x_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 
+int dm355_psc_init(struct device *dev, void __iomem *base);
+int dm365_psc_init(struct device *dev, void __iomem *base);
+int dm644x_psc_init(struct device *dev, void __iomem *base);
+int dm646x_psc_init(struct device *dev, void __iomem *base);
+
 #endif /* __LINUX_CLK_DAVINCI_PLL_H___ */
-- 
2.17.0

^ permalink raw reply related

* [PATCH 7/9] clk: davinci: da850-pll: change PLL0 to CLK_OF_DECLARE
From: David Lechner @ 2018-05-25 18:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525181150.17873-1-david@lechnology.com>

PLL0 on davinci/da850-type device needs to be registered early in boot
because it is needed for clocksource/clockevent. Change the driver
to use CLK_OF_DECLARE for this special case.

Reviewed-by: Sekhar Nori <nsekhar@ti.com>
Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/clk/davinci/pll-da850.c | 21 +++++++++++++++++----
 drivers/clk/davinci/pll.c       |  4 +++-
 drivers/clk/davinci/pll.h       |  2 +-
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/davinci/pll-da850.c b/drivers/clk/davinci/pll-da850.c
index 59cc2e3733f9..0f7198191ea2 100644
--- a/drivers/clk/davinci/pll-da850.c
+++ b/drivers/clk/davinci/pll-da850.c
@@ -13,6 +13,8 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/mfd/da8xx-cfgchip.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
 #include <linux/of.h>
 #include <linux/types.h>
 
@@ -136,11 +138,22 @@ static const struct davinci_pll_sysclk_info *da850_pll0_sysclk_info[] = {
 	NULL
 };
 
-int of_da850_pll0_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
+void of_da850_pll0_init(struct device_node *node)
 {
-	return of_davinci_pll_init(dev, dev->of_node, &da850_pll0_info,
-				   &da850_pll0_obsclk_info,
-				   da850_pll0_sysclk_info, 7, base, cfgchip);
+	void __iomem *base;
+	struct regmap *cfgchip;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s: ioremap failed\n", __func__);
+		return;
+	}
+
+	cfgchip = syscon_regmap_lookup_by_compatible("ti,da830-cfgchip");
+
+	of_davinci_pll_init(NULL, node, &da850_pll0_info,
+			    &da850_pll0_obsclk_info,
+			    da850_pll0_sysclk_info, 7, base, cfgchip);
 }
 
 static const struct davinci_pll_clk_info da850_pll1_info = {
diff --git a/drivers/clk/davinci/pll.c b/drivers/clk/davinci/pll.c
index 2eb981e61185..84a343060bc8 100644
--- a/drivers/clk/davinci/pll.c
+++ b/drivers/clk/davinci/pll.c
@@ -859,8 +859,10 @@ static struct davinci_pll_platform_data *davinci_pll_get_pdata(struct device *de
 	return pdata;
 }
 
+/* needed in early boot for clocksource/clockevent */
+CLK_OF_DECLARE(da850_pll0, "ti,da850-pll0", of_da850_pll0_init);
+
 static const struct of_device_id davinci_pll_of_match[] = {
-	{ .compatible = "ti,da850-pll0", .data = of_da850_pll0_init },
 	{ .compatible = "ti,da850-pll1", .data = of_da850_pll1_init },
 	{ }
 };
diff --git a/drivers/clk/davinci/pll.h b/drivers/clk/davinci/pll.h
index 562652fc0759..b2e5c4496645 100644
--- a/drivers/clk/davinci/pll.h
+++ b/drivers/clk/davinci/pll.h
@@ -123,7 +123,7 @@ int of_davinci_pll_init(struct device *dev, struct device_node *node,
 /* Platform-specific callbacks */
 
 int da850_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
-int of_da850_pll0_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+void of_da850_pll0_init(struct device_node *node);
 int of_da850_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 
 int dm355_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
-- 
2.17.0

^ permalink raw reply related

* [PATCH 6/9] clk: davinci: pll: allow dev == NULL
From: David Lechner @ 2018-05-25 18:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525181150.17873-1-david@lechnology.com>

This modifies the TI Davinci PLL clock driver to allow for the case
when dev == NULL. On some (most) SoCs that use this driver, the PLL
clock needs to be registered during early boot because it is used
for clocksource/clkevent and there will be no platform device available.

Signed-off-by: David Lechner <david@lechnology.com>
Reviewed-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/clk/davinci/pll-da830.c  |   5 +-
 drivers/clk/davinci/pll-da850.c  |  22 +--
 drivers/clk/davinci/pll-dm355.c  |   9 +-
 drivers/clk/davinci/pll-dm365.c  |   9 +-
 drivers/clk/davinci/pll-dm644x.c |   9 +-
 drivers/clk/davinci/pll-dm646x.c |   9 +-
 drivers/clk/davinci/pll.c        | 279 +++++++++++++++++++++----------
 drivers/clk/davinci/pll.h        |  30 ++--
 include/linux/clk/davinci.h      |  24 +++
 9 files changed, 259 insertions(+), 137 deletions(-)
 create mode 100644 include/linux/clk/davinci.h

diff --git a/drivers/clk/davinci/pll-da830.c b/drivers/clk/davinci/pll-da830.c
index 929a3d3a9adb..0a0d06fb25fd 100644
--- a/drivers/clk/davinci/pll-da830.c
+++ b/drivers/clk/davinci/pll-da830.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clkdev.h>
+#include <linux/clk/davinci.h>
 #include <linux/bitops.h>
 #include <linux/init.h>
 #include <linux/types.h>
@@ -36,11 +37,11 @@ SYSCLK(5, pll0_sysclk5, pll0_pllen, 5, 0);
 SYSCLK(6, pll0_sysclk6, pll0_pllen, 5, SYSCLK_FIXED_DIV);
 SYSCLK(7, pll0_sysclk7, pll0_pllen, 5, 0);
 
-int da830_pll_init(struct device *dev, void __iomem *base)
+int da830_pll_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
 	struct clk *clk;
 
-	davinci_pll_clk_register(dev, &da830_pll_info, "ref_clk", base);
+	davinci_pll_clk_register(dev, &da830_pll_info, "ref_clk", base, cfgchip);
 
 	clk = davinci_pll_sysclk_register(dev, &pll0_sysclk2, base);
 	clk_register_clkdev(clk, "pll0_sysclk2", "da830-psc0");
diff --git a/drivers/clk/davinci/pll-da850.c b/drivers/clk/davinci/pll-da850.c
index 2a038b7908cc..59cc2e3733f9 100644
--- a/drivers/clk/davinci/pll-da850.c
+++ b/drivers/clk/davinci/pll-da850.c
@@ -7,7 +7,9 @@
 
 #include <linux/bitops.h>
 #include <linux/clk-provider.h>
+#include <linux/clk/davinci.h>
 #include <linux/clkdev.h>
+#include <linux/device.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/mfd/da8xx-cfgchip.h>
@@ -81,11 +83,11 @@ static const struct davinci_pll_obsclk_info da850_pll0_obsclk_info = {
 	.ocsrc_mask = GENMASK(4, 0),
 };
 
-int da850_pll0_init(struct device *dev, void __iomem *base)
+int da850_pll0_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
 	struct clk *clk;
 
-	davinci_pll_clk_register(dev, &da850_pll0_info, "ref_clk", base);
+	davinci_pll_clk_register(dev, &da850_pll0_info, "ref_clk", base, cfgchip);
 
 	clk = davinci_pll_sysclk_register(dev, &pll0_sysclk1, base);
 	clk_register_clkdev(clk, "pll0_sysclk1", "da850-psc0");
@@ -134,11 +136,11 @@ static const struct davinci_pll_sysclk_info *da850_pll0_sysclk_info[] = {
 	NULL
 };
 
-int of_da850_pll0_init(struct device *dev, void __iomem *base)
+int of_da850_pll0_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
-	return of_davinci_pll_init(dev, &da850_pll0_info,
+	return of_davinci_pll_init(dev, dev->of_node, &da850_pll0_info,
 				   &da850_pll0_obsclk_info,
-				   da850_pll0_sysclk_info, 7, base);
+				   da850_pll0_sysclk_info, 7, base, cfgchip);
 }
 
 static const struct davinci_pll_clk_info da850_pll1_info = {
@@ -179,11 +181,11 @@ static const struct davinci_pll_obsclk_info da850_pll1_obsclk_info = {
 	.ocsrc_mask = GENMASK(4, 0),
 };
 
-int da850_pll1_init(struct device *dev, void __iomem *base)
+int da850_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
 	struct clk *clk;
 
-	davinci_pll_clk_register(dev, &da850_pll1_info, "oscin", base);
+	davinci_pll_clk_register(dev, &da850_pll1_info, "oscin", base, cfgchip);
 
 	davinci_pll_sysclk_register(dev, &pll1_sysclk1, base);
 
@@ -204,9 +206,9 @@ static const struct davinci_pll_sysclk_info *da850_pll1_sysclk_info[] = {
 	NULL
 };
 
-int of_da850_pll1_init(struct device *dev, void __iomem *base)
+int of_da850_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
-	return of_davinci_pll_init(dev, &da850_pll1_info,
+	return of_davinci_pll_init(dev, dev->of_node, &da850_pll1_info,
 				   &da850_pll1_obsclk_info,
-				   da850_pll1_sysclk_info, 3, base);
+				   da850_pll1_sysclk_info, 3, base, cfgchip);
 }
diff --git a/drivers/clk/davinci/pll-dm355.c b/drivers/clk/davinci/pll-dm355.c
index 93f4a53d6b44..505aed80be9a 100644
--- a/drivers/clk/davinci/pll-dm355.c
+++ b/drivers/clk/davinci/pll-dm355.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk/davinci.h>
 #include <linux/clkdev.h>
 #include <linux/init.h>
 #include <linux/types.h>
@@ -27,11 +28,11 @@ SYSCLK(2, pll1_sysclk2, pll1_pllen, 5, SYSCLK_FIXED_DIV | SYSCLK_ALWAYS_ENABLED)
 SYSCLK(3, pll1_sysclk3, pll1_pllen, 5, SYSCLK_ALWAYS_ENABLED);
 SYSCLK(4, pll1_sysclk4, pll1_pllen, 5, SYSCLK_ALWAYS_ENABLED);
 
-int dm355_pll1_init(struct device *dev, void __iomem *base)
+int dm355_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
 	struct clk *clk;
 
-	davinci_pll_clk_register(dev, &dm355_pll1_info, "ref_clk", base);
+	davinci_pll_clk_register(dev, &dm355_pll1_info, "ref_clk", base, cfgchip);
 
 	clk = davinci_pll_sysclk_register(dev, &pll1_sysclk1, base);
 	clk_register_clkdev(clk, "pll1_sysclk1", "dm355-psc");
@@ -64,9 +65,9 @@ static const struct davinci_pll_clk_info dm355_pll2_info = {
 
 SYSCLK(1, pll2_sysclk1, pll2_pllen, 5, SYSCLK_FIXED_DIV | SYSCLK_ALWAYS_ENABLED);
 
-int dm355_pll2_init(struct device *dev, void __iomem *base)
+int dm355_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
-	davinci_pll_clk_register(dev, &dm355_pll2_info, "oscin", base);
+	davinci_pll_clk_register(dev, &dm355_pll2_info, "oscin", base, cfgchip);
 
 	davinci_pll_sysclk_register(dev, &pll2_sysclk1, base);
 
diff --git a/drivers/clk/davinci/pll-dm365.c b/drivers/clk/davinci/pll-dm365.c
index 5f8d9f42d0f3..2d29712753a3 100644
--- a/drivers/clk/davinci/pll-dm365.c
+++ b/drivers/clk/davinci/pll-dm365.c
@@ -7,6 +7,7 @@
 
 #include <linux/bitops.h>
 #include <linux/clkdev.h>
+#include <linux/clk/davinci.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/types.h>
@@ -56,11 +57,11 @@ static const struct davinci_pll_obsclk_info dm365_pll1_obsclk_info = {
 	.ocsrc_mask = BIT(4),
 };
 
-int dm365_pll1_init(struct device *dev, void __iomem *base)
+int dm365_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
 	struct clk *clk;
 
-	davinci_pll_clk_register(dev, &dm365_pll1_info, "ref_clk", base);
+	davinci_pll_clk_register(dev, &dm365_pll1_info, "ref_clk", base, cfgchip);
 
 	clk = davinci_pll_sysclk_register(dev, &pll1_sysclk1, base);
 	clk_register_clkdev(clk, "pll1_sysclk1", "dm365-psc");
@@ -119,11 +120,11 @@ static const struct davinci_pll_obsclk_info dm365_pll2_obsclk_info = {
 	.ocsrc_mask = BIT(4),
 };
 
-int dm365_pll2_init(struct device *dev, void __iomem *base)
+int dm365_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
 	struct clk *clk;
 
-	davinci_pll_clk_register(dev, &dm365_pll2_info, "oscin", base);
+	davinci_pll_clk_register(dev, &dm365_pll2_info, "oscin", base, cfgchip);
 
 	davinci_pll_sysclk_register(dev, &pll2_sysclk1, base);
 
diff --git a/drivers/clk/davinci/pll-dm644x.c b/drivers/clk/davinci/pll-dm644x.c
index 69bf785377cf..7650fadfaac8 100644
--- a/drivers/clk/davinci/pll-dm644x.c
+++ b/drivers/clk/davinci/pll-dm644x.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk/davinci.h>
 #include <linux/clkdev.h>
 #include <linux/init.h>
 #include <linux/types.h>
@@ -27,11 +28,11 @@ SYSCLK(2, pll1_sysclk2, pll1_pllen, 4, SYSCLK_FIXED_DIV);
 SYSCLK(3, pll1_sysclk3, pll1_pllen, 4, SYSCLK_FIXED_DIV);
 SYSCLK(5, pll1_sysclk5, pll1_pllen, 4, SYSCLK_FIXED_DIV);
 
-int dm644x_pll1_init(struct device *dev, void __iomem *base)
+int dm644x_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
 	struct clk *clk;
 
-	davinci_pll_clk_register(dev, &dm644x_pll1_info, "ref_clk", base);
+	davinci_pll_clk_register(dev, &dm644x_pll1_info, "ref_clk", base, cfgchip);
 
 	clk = davinci_pll_sysclk_register(dev, &pll1_sysclk1, base);
 	clk_register_clkdev(clk, "pll1_sysclk1", "dm644x-psc");
@@ -66,9 +67,9 @@ static const struct davinci_pll_clk_info dm644x_pll2_info = {
 SYSCLK(1, pll2_sysclk1, pll2_pllen, 4, 0);
 SYSCLK(2, pll2_sysclk2, pll2_pllen, 4, 0);
 
-int dm644x_pll2_init(struct device *dev, void __iomem *base)
+int dm644x_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
-	davinci_pll_clk_register(dev, &dm644x_pll2_info, "oscin", base);
+	davinci_pll_clk_register(dev, &dm644x_pll2_info, "oscin", base, cfgchip);
 
 	davinci_pll_sysclk_register(dev, &pll2_sysclk1, base);
 
diff --git a/drivers/clk/davinci/pll-dm646x.c b/drivers/clk/davinci/pll-dm646x.c
index 0ae827e3ce80..26982970df0e 100644
--- a/drivers/clk/davinci/pll-dm646x.c
+++ b/drivers/clk/davinci/pll-dm646x.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/clk/davinci.h>
 #include <linux/clkdev.h>
 #include <linux/init.h>
 #include <linux/types.h>
@@ -29,11 +30,11 @@ SYSCLK(6, pll1_sysclk6, pll1_pllen, 4, 0);
 SYSCLK(8, pll1_sysclk8, pll1_pllen, 4, 0);
 SYSCLK(9, pll1_sysclk9, pll1_pllen, 4, 0);
 
-int dm646x_pll1_init(struct device *dev, void __iomem *base)
+int dm646x_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
 	struct clk *clk;
 
-	davinci_pll_clk_register(dev, &dm646x_pll1_info, "ref_clk", base);
+	davinci_pll_clk_register(dev, &dm646x_pll1_info, "ref_clk", base, cfgchip);
 
 	clk = davinci_pll_sysclk_register(dev, &pll1_sysclk1, base);
 	clk_register_clkdev(clk, "pll1_sysclk1", "dm646x-psc");
@@ -74,9 +75,9 @@ static const struct davinci_pll_clk_info dm646x_pll2_info = {
 
 SYSCLK(1, pll2_sysclk1, pll2_pllen, 4, SYSCLK_ALWAYS_ENABLED);
 
-int dm646x_pll2_init(struct device *dev, void __iomem *base)
+int dm646x_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip)
 {
-	davinci_pll_clk_register(dev, &dm646x_pll2_info, "oscin", base);
+	davinci_pll_clk_register(dev, &dm646x_pll2_info, "oscin", base, cfgchip);
 
 	davinci_pll_sysclk_register(dev, &pll2_sysclk1, base);
 
diff --git a/drivers/clk/davinci/pll.c b/drivers/clk/davinci/pll.c
index 23a24c944f1d..2eb981e61185 100644
--- a/drivers/clk/davinci/pll.c
+++ b/drivers/clk/davinci/pll.c
@@ -11,6 +11,7 @@
 
 #include <linux/clk-provider.h>
 #include <linux/clk.h>
+#include <linux/clk/davinci.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -223,6 +224,7 @@ static const struct clk_ops dm365_pll_ops = {
 
 /**
  * davinci_pll_div_register - common *DIV clock implementation
+ * @dev: The PLL platform device or NULL
  * @name: the clock name
  * @parent_name: the parent clock name
  * @reg: the *DIV register
@@ -240,17 +242,21 @@ static struct clk *davinci_pll_div_register(struct device *dev,
 	const struct clk_ops *divider_ops = &clk_divider_ops;
 	struct clk_gate *gate;
 	struct clk_divider *divider;
+	struct clk *clk;
+	int ret;
 
-	gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
+	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
 	if (!gate)
 		return ERR_PTR(-ENOMEM);
 
 	gate->reg = reg;
 	gate->bit_idx = DIV_ENABLE_SHIFT;
 
-	divider = devm_kzalloc(dev, sizeof(*divider), GFP_KERNEL);
-	if (!divider)
-		return ERR_PTR(-ENOMEM);
+	divider = kzalloc(sizeof(*divider), GFP_KERNEL);
+	if (!divider) {
+		ret = -ENOMEM;
+		goto err_free_gate;
+	}
 
 	divider->reg = reg;
 	divider->shift = DIV_RATIO_SHIFT;
@@ -261,9 +267,22 @@ static struct clk *davinci_pll_div_register(struct device *dev,
 		divider_ops = &clk_divider_ro_ops;
 	}
 
-	return clk_register_composite(dev, name, parent_names, num_parents,
-				      NULL, NULL, &divider->hw, divider_ops,
-				      &gate->hw, &clk_gate_ops, flags);
+	clk = clk_register_composite(dev, name, parent_names, num_parents,
+				     NULL, NULL, &divider->hw, divider_ops,
+				     &gate->hw, &clk_gate_ops, flags);
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		goto err_free_divider;
+	}
+
+	return clk;
+
+err_free_divider:
+	kfree(divider);
+err_free_gate:
+	kfree(gate);
+
+	return ERR_PTR(ret);
 }
 
 struct davinci_pllen_clk {
@@ -321,36 +340,17 @@ static int davinci_pllen_rate_change(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-static struct davinci_pll_platform_data *davinci_pll_get_pdata(struct device *dev)
-{
-	struct davinci_pll_platform_data *pdata = dev_get_platdata(dev);
-
-	/*
-	 * Platform data is optional, so allocate a new struct if one was not
-	 * provided. For device tree, this will always be the case.
-	 */
-	if (!pdata)
-		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
-	if (!pdata)
-		return NULL;
-
-	/* for device tree, we need to fill in the struct */
-	if (dev->of_node)
-		pdata->cfgchip =
-			syscon_regmap_lookup_by_compatible("ti,da830-cfgchip");
-
-	return pdata;
-}
-
 static struct notifier_block davinci_pllen_notifier = {
 	.notifier_call = davinci_pllen_rate_change,
 };
 
 /**
  * davinci_pll_clk_register - Register a PLL clock
+ * @dev: The PLL platform device or NULL
  * @info: The device-specific clock info
  * @parent_name: The parent clock name
  * @base: The PLL's memory region
+ * @cfgchip: CFGCHIP syscon regmap for info->unlock_reg or NULL
  *
  * This creates a series of clocks that represent the PLL.
  *
@@ -366,9 +366,9 @@ static struct notifier_block davinci_pllen_notifier = {
 struct clk *davinci_pll_clk_register(struct device *dev,
 				     const struct davinci_pll_clk_info *info,
 				     const char *parent_name,
-				     void __iomem *base)
+				     void __iomem *base,
+				     struct regmap *cfgchip)
 {
-	struct davinci_pll_platform_data *pdata;
 	char prediv_name[MAX_NAME_SIZE];
 	char pllout_name[MAX_NAME_SIZE];
 	char postdiv_name[MAX_NAME_SIZE];
@@ -376,11 +376,12 @@ struct clk *davinci_pll_clk_register(struct device *dev,
 	struct clk_init_data init;
 	struct davinci_pll_clk *pllout;
 	struct davinci_pllen_clk *pllen;
-	struct clk *pllout_clk, *clk;
-
-	pdata = davinci_pll_get_pdata(dev);
-	if (!pdata)
-		return ERR_PTR(-ENOMEM);
+	struct clk *oscin_clk = NULL;
+	struct clk *prediv_clk = NULL;
+	struct clk *pllout_clk;
+	struct clk *postdiv_clk = NULL;
+	struct clk *pllen_clk;
+	int ret;
 
 	if (info->flags & PLL_HAS_CLKMODE) {
 		/*
@@ -392,10 +393,10 @@ struct clk *davinci_pll_clk_register(struct device *dev,
 		 * a number of different things. In this driver we use it to
 		 * mean the signal after the PLLCTL[CLKMODE] switch.
 		 */
-		clk = clk_register_fixed_factor(dev, OSCIN_CLK_NAME,
-						parent_name, 0, 1, 1);
-		if (IS_ERR(clk))
-			return clk;
+		oscin_clk = clk_register_fixed_factor(dev, OSCIN_CLK_NAME,
+						      parent_name, 0, 1, 1);
+		if (IS_ERR(oscin_clk))
+			return oscin_clk;
 
 		parent_name = OSCIN_CLK_NAME;
 	}
@@ -411,30 +412,34 @@ struct clk *davinci_pll_clk_register(struct device *dev,
 
 		/* Some? DM355 chips don't correctly report the PREDIV value */
 		if (info->flags & PLL_PREDIV_FIXED8)
-			clk = clk_register_fixed_factor(dev, prediv_name,
-						parent_name, flags, 1, 8);
+			prediv_clk = clk_register_fixed_factor(dev, prediv_name,
+							parent_name, flags, 1, 8);
 		else
-			clk = davinci_pll_div_register(dev, prediv_name,
+			prediv_clk = davinci_pll_div_register(dev, prediv_name,
 				parent_name, base + PREDIV, fixed, flags);
-		if (IS_ERR(clk))
-			return clk;
+		if (IS_ERR(prediv_clk)) {
+			ret = PTR_ERR(prediv_clk);
+			goto err_unregister_oscin;
+		}
 
 		parent_name = prediv_name;
 	}
 
 	/* Unlock writing to PLL registers */
 	if (info->unlock_reg) {
-		if (IS_ERR_OR_NULL(pdata->cfgchip))
+		if (IS_ERR_OR_NULL(cfgchip))
 			dev_warn(dev, "Failed to get CFGCHIP (%ld)\n",
-				 PTR_ERR(pdata->cfgchip));
+				 PTR_ERR(cfgchip));
 		else
-			regmap_write_bits(pdata->cfgchip, info->unlock_reg,
+			regmap_write_bits(cfgchip, info->unlock_reg,
 					  info->unlock_mask, 0);
 	}
 
-	pllout = devm_kzalloc(dev, sizeof(*pllout), GFP_KERNEL);
-	if (!pllout)
-		return ERR_PTR(-ENOMEM);
+	pllout = kzalloc(sizeof(*pllout), GFP_KERNEL);
+	if (!pllout) {
+		ret = -ENOMEM;
+		goto err_unregister_prediv;
+	}
 
 	snprintf(pllout_name, MAX_NAME_SIZE, "%s_pllout", info->name);
 
@@ -456,9 +461,11 @@ struct clk *davinci_pll_clk_register(struct device *dev,
 	pllout->pllm_min = info->pllm_min;
 	pllout->pllm_max = info->pllm_max;
 
-	pllout_clk = devm_clk_register(dev, &pllout->hw);
-	if (IS_ERR(pllout_clk))
-		return pllout_clk;
+	pllout_clk = clk_register(dev, &pllout->hw);
+	if (IS_ERR(pllout_clk)) {
+		ret = PTR_ERR(pllout_clk);
+		goto err_free_pllout;
+	}
 
 	clk_hw_set_rate_range(&pllout->hw, info->pllout_min_rate,
 			      info->pllout_max_rate);
@@ -474,17 +481,21 @@ struct clk *davinci_pll_clk_register(struct device *dev,
 		if (info->flags & PLL_POSTDIV_ALWAYS_ENABLED)
 			flags |= CLK_IS_CRITICAL;
 
-		clk = davinci_pll_div_register(dev, postdiv_name, parent_name,
-					       base + POSTDIV, fixed, flags);
-		if (IS_ERR(clk))
-			return clk;
+		postdiv_clk = davinci_pll_div_register(dev, postdiv_name,
+				parent_name, base + POSTDIV, fixed, flags);
+		if (IS_ERR(postdiv_clk)) {
+			ret = PTR_ERR(postdiv_clk);
+			goto err_unregister_pllout;
+		}
 
 		parent_name = postdiv_name;
 	}
 
-	pllen = devm_kzalloc(dev, sizeof(*pllout), GFP_KERNEL);
-	if (!pllen)
-		return ERR_PTR(-ENOMEM);
+	pllen = kzalloc(sizeof(*pllout), GFP_KERNEL);
+	if (!pllen) {
+		ret = -ENOMEM;
+		goto err_unregister_postdiv;
+	}
 
 	snprintf(pllen_name, MAX_NAME_SIZE, "%s_pllen", info->name);
 
@@ -497,17 +508,35 @@ struct clk *davinci_pll_clk_register(struct device *dev,
 	pllen->hw.init = &init;
 	pllen->base = base;
 
-	clk = devm_clk_register(dev, &pllen->hw);
-	if (IS_ERR(clk))
-		return clk;
+	pllen_clk = clk_register(dev, &pllen->hw);
+	if (IS_ERR(pllen_clk)) {
+		ret = PTR_ERR(pllen_clk);
+		goto err_free_pllen;
+	}
 
-	clk_notifier_register(clk, &davinci_pllen_notifier);
+	clk_notifier_register(pllen_clk, &davinci_pllen_notifier);
 
 	return pllout_clk;
+
+err_free_pllen:
+	kfree(pllen);
+err_unregister_postdiv:
+	clk_unregister(postdiv_clk);
+err_unregister_pllout:
+	clk_unregister(pllout_clk);
+err_free_pllout:
+	kfree(pllout);
+err_unregister_prediv:
+	clk_unregister(prediv_clk);
+err_unregister_oscin:
+	clk_unregister(oscin_clk);
+
+	return ERR_PTR(ret);
 }
 
 /**
  * davinci_pll_auxclk_register - Register bypass clock (AUXCLK)
+ * @dev: The PLL platform device or NULL
  * @name: The clock name
  * @base: The PLL memory region
  */
@@ -521,6 +550,7 @@ struct clk *davinci_pll_auxclk_register(struct device *dev,
 
 /**
  * davinci_pll_sysclkbp_clk_register - Register bypass divider clock (SYSCLKBP)
+ * @dev: The PLL platform device or NULL
  * @name: The clock name
  * @base: The PLL memory region
  */
@@ -535,6 +565,7 @@ struct clk *davinci_pll_sysclkbp_clk_register(struct device *dev,
 
 /**
  * davinci_pll_obsclk_register - Register oscillator divider clock (OBSCLK)
+ * @dev: The PLL platform device or NULL
  * @info: The clock info
  * @base: The PLL memory region
  */
@@ -546,9 +577,11 @@ davinci_pll_obsclk_register(struct device *dev,
 	struct clk_mux *mux;
 	struct clk_gate *gate;
 	struct clk_divider *divider;
+	struct clk *clk;
 	u32 oscdiv;
+	int ret;
 
-	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
 	if (!mux)
 		return ERR_PTR(-ENOMEM);
 
@@ -556,16 +589,20 @@ davinci_pll_obsclk_register(struct device *dev,
 	mux->table = info->table;
 	mux->mask = info->ocsrc_mask;
 
-	gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
-	if (!gate)
-		return ERR_PTR(-ENOMEM);
+	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
+	if (!gate) {
+		ret = -ENOMEM;
+		goto err_free_mux;
+	}
 
 	gate->reg = base + CKEN;
 	gate->bit_idx = CKEN_OBSCLK_SHIFT;
 
-	divider = devm_kzalloc(dev, sizeof(*divider), GFP_KERNEL);
-	if (!divider)
-		return ERR_PTR(-ENOMEM);
+	divider = kzalloc(sizeof(*divider), GFP_KERNEL);
+	if (!divider) {
+		ret = -ENOMEM;
+		goto err_free_gate;
+	}
 
 	divider->reg = base + OSCDIV;
 	divider->shift = DIV_RATIO_SHIFT;
@@ -576,11 +613,27 @@ davinci_pll_obsclk_register(struct device *dev,
 	oscdiv |= BIT(DIV_ENABLE_SHIFT);
 	writel(oscdiv, base + OSCDIV);
 
-	return clk_register_composite(dev, info->name, info->parent_names,
-				      info->num_parents,
-				      &mux->hw, &clk_mux_ops,
-				      &divider->hw, &clk_divider_ops,
-				      &gate->hw, &clk_gate_ops, 0);
+	clk = clk_register_composite(dev, info->name, info->parent_names,
+				     info->num_parents,
+				     &mux->hw, &clk_mux_ops,
+				     &divider->hw, &clk_divider_ops,
+				     &gate->hw, &clk_gate_ops, 0);
+
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		goto err_free_divider;
+	}
+
+	return clk;
+
+err_free_divider:
+	kfree(divider);
+err_free_gate:
+	kfree(gate);
+err_free_mux:
+	kfree(mux);
+
+	return ERR_PTR(ret);
 }
 
 /* The PLL SYSCLKn clocks have a mechanism for synchronizing rate changes. */
@@ -616,6 +669,7 @@ static struct notifier_block davinci_pll_sysclk_notifier = {
 
 /**
  * davinci_pll_sysclk_register - Register divider clocks (SYSCLKn)
+ * @dev: The PLL platform device or NULL
  * @info: The clock info
  * @base: The PLL memory region
  */
@@ -630,6 +684,7 @@ davinci_pll_sysclk_register(struct device *dev,
 	struct clk *clk;
 	u32 reg;
 	u32 flags = 0;
+	int ret;
 
 	/* PLLDIVn registers are not entirely consecutive */
 	if (info->id < 4)
@@ -637,16 +692,18 @@ davinci_pll_sysclk_register(struct device *dev,
 	else
 		reg = PLLDIV4 + 4 * (info->id - 4);
 
-	gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
+	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
 	if (!gate)
 		return ERR_PTR(-ENOMEM);
 
 	gate->reg = base + reg;
 	gate->bit_idx = DIV_ENABLE_SHIFT;
 
-	divider = devm_kzalloc(dev, sizeof(*divider), GFP_KERNEL);
-	if (!divider)
-		return ERR_PTR(-ENOMEM);
+	divider = kzalloc(sizeof(*divider), GFP_KERNEL);
+	if (!divider) {
+		ret = -ENOMEM;
+		goto err_free_gate;
+	}
 
 	divider->reg = base + reg;
 	divider->shift = DIV_RATIO_SHIFT;
@@ -668,22 +725,31 @@ davinci_pll_sysclk_register(struct device *dev,
 	clk = clk_register_composite(dev, info->name, &info->parent_name, 1,
 				     NULL, NULL, &divider->hw, divider_ops,
 				     &gate->hw, &clk_gate_ops, flags);
-	if (IS_ERR(clk))
-		return clk;
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		goto err_free_divider;
+	}
 
 	clk_notifier_register(clk, &davinci_pll_sysclk_notifier);
 
 	return clk;
+
+err_free_divider:
+	kfree(divider);
+err_free_gate:
+	kfree(gate);
+
+	return ERR_PTR(ret);
 }
 
-int of_davinci_pll_init(struct device *dev,
+int of_davinci_pll_init(struct device *dev, struct device_node *node,
 			const struct davinci_pll_clk_info *info,
 			const struct davinci_pll_obsclk_info *obsclk_info,
 			const struct davinci_pll_sysclk_info **div_info,
 			u8 max_sysclk_id,
-			void __iomem *base)
+			void __iomem *base,
+			struct regmap *cfgchip)
 {
-	struct device_node *node = dev->of_node;
 	struct device_node *child;
 	const char *parent_name;
 	struct clk *clk;
@@ -693,7 +759,7 @@ int of_davinci_pll_init(struct device *dev,
 	else
 		parent_name = OSCIN_CLK_NAME;
 
-	clk = davinci_pll_clk_register(dev, info, parent_name, base);
+	clk = davinci_pll_clk_register(dev, info, parent_name, base, cfgchip);
 	if (IS_ERR(clk)) {
 		dev_err(dev, "failed to register %s\n", info->name);
 		return PTR_ERR(clk);
@@ -711,13 +777,15 @@ int of_davinci_pll_init(struct device *dev,
 		int n_clks =  max_sysclk_id + 1;
 		int i;
 
-		clk_data = devm_kzalloc(dev, sizeof(*clk_data), GFP_KERNEL);
+		clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
 		if (!clk_data)
 			return -ENOMEM;
 
-		clks = devm_kmalloc_array(dev, n_clks, sizeof(*clks), GFP_KERNEL);
-		if (!clks)
+		clks = kmalloc_array(n_clks, sizeof(*clks), GFP_KERNEL);
+		if (!clks) {
+			kfree(clk_data);
 			return -ENOMEM;
+		}
 
 		clk_data->clks = clks;
 		clk_data->clk_num = n_clks;
@@ -770,6 +838,27 @@ int of_davinci_pll_init(struct device *dev,
 	return 0;
 }
 
+static struct davinci_pll_platform_data *davinci_pll_get_pdata(struct device *dev)
+{
+	struct davinci_pll_platform_data *pdata = dev_get_platdata(dev);
+
+	/*
+	 * Platform data is optional, so allocate a new struct if one was not
+	 * provided. For device tree, this will always be the case.
+	 */
+	if (!pdata)
+		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	/* for device tree, we need to fill in the struct */
+	if (dev->of_node)
+		pdata->cfgchip =
+			syscon_regmap_lookup_by_compatible("ti,da830-cfgchip");
+
+	return pdata;
+}
+
 static const struct of_device_id davinci_pll_of_match[] = {
 	{ .compatible = "ti,da850-pll0", .data = of_da850_pll0_init },
 	{ .compatible = "ti,da850-pll1", .data = of_da850_pll1_init },
@@ -791,11 +880,13 @@ static const struct platform_device_id davinci_pll_id_table[] = {
 	{ }
 };
 
-typedef int (*davinci_pll_init)(struct device *dev, void __iomem *base);
+typedef int (*davinci_pll_init)(struct device *dev, void __iomem *base,
+				struct regmap *cfgchip);
 
 static int davinci_pll_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct davinci_pll_platform_data *pdata;
 	const struct of_device_id *of_id;
 	davinci_pll_init pll_init = NULL;
 	struct resource *res;
@@ -812,12 +903,18 @@ static int davinci_pll_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	pdata = davinci_pll_get_pdata(dev);
+	if (!pdata) {
+		dev_err(dev, "missing platform data\n");
+		return -EINVAL;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-	return pll_init(dev, base);
+	return pll_init(dev, base, pdata->cfgchip);
 }
 
 static struct platform_driver davinci_pll_driver = {
diff --git a/drivers/clk/davinci/pll.h b/drivers/clk/davinci/pll.h
index b1b6fb23f972..562652fc0759 100644
--- a/drivers/clk/davinci/pll.h
+++ b/drivers/clk/davinci/pll.h
@@ -11,6 +11,7 @@
 #include <linux/bitops.h>
 #include <linux/clk-provider.h>
 #include <linux/of.h>
+#include <linux/regmap.h>
 #include <linux/types.h>
 
 #define PLL_HAS_CLKMODE			BIT(0) /* PLL has PLLCTL[CLKMODE] */
@@ -94,7 +95,8 @@ struct davinci_pll_obsclk_info {
 struct clk *davinci_pll_clk_register(struct device *dev,
 				     const struct davinci_pll_clk_info *info,
 				     const char *parent_name,
-				     void __iomem *base);
+				     void __iomem *base,
+				     struct regmap *cfgchip);
 struct clk *davinci_pll_auxclk_register(struct device *dev,
 					const char *name,
 					void __iomem *base);
@@ -110,32 +112,24 @@ davinci_pll_sysclk_register(struct device *dev,
 			    const struct davinci_pll_sysclk_info *info,
 			    void __iomem *base);
 
-int of_davinci_pll_init(struct device *dev,
+int of_davinci_pll_init(struct device *dev, struct device_node *node,
 			const struct davinci_pll_clk_info *info,
 			const struct davinci_pll_obsclk_info *obsclk_info,
 			const struct davinci_pll_sysclk_info **div_info,
 			u8 max_sysclk_id,
-			void __iomem *base);
+			void __iomem *base,
+			struct regmap *cfgchip);
 
 /* Platform-specific callbacks */
 
-int da830_pll_init(struct device *dev, void __iomem *base);
+int da850_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+int of_da850_pll0_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+int of_da850_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 
-int da850_pll0_init(struct device *dev, void __iomem *base);
-int da850_pll1_init(struct device *dev, void __iomem *base);
-int of_da850_pll0_init(struct device *dev, void __iomem *base);
-int of_da850_pll1_init(struct device *dev, void __iomem *base);
+int dm355_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 
-int dm355_pll1_init(struct device *dev, void __iomem *base);
-int dm355_pll2_init(struct device *dev, void __iomem *base);
+int dm644x_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 
-int dm365_pll1_init(struct device *dev, void __iomem *base);
-int dm365_pll2_init(struct device *dev, void __iomem *base);
-
-int dm644x_pll1_init(struct device *dev, void __iomem *base);
-int dm644x_pll2_init(struct device *dev, void __iomem *base);
-
-int dm646x_pll1_init(struct device *dev, void __iomem *base);
-int dm646x_pll2_init(struct device *dev, void __iomem *base);
+int dm646x_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
 
 #endif /* __CLK_DAVINCI_PLL_H___ */
diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h
new file mode 100644
index 000000000000..ebdd9df1c0ef
--- /dev/null
+++ b/include/linux/clk/davinci.h
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Clock drivers for TI DaVinci PLL and PSC controllers
+ *
+ * Copyright (C) 2018 David Lechner <david@lechnology.com>
+ */
+
+#ifndef __LINUX_CLK_DAVINCI_PLL_H___
+#define __LINUX_CLK_DAVINCI_PLL_H___
+
+#include <linux/device.h>
+#include <linux/regmap.h>
+
+/* function for registering clocks in early boot */
+
+int da830_pll_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+int da850_pll0_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+int dm355_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+int dm365_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+int dm365_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+int dm644x_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+int dm646x_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
+
+#endif /* __LINUX_CLK_DAVINCI_PLL_H___ */
-- 
2.17.0

^ permalink raw reply related

* [PATCH 5/9] clk: davinci: psc-dm365: fix few clocks
From: David Lechner @ 2018-05-25 18:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525181150.17873-1-david@lechnology.com>

From: Sekhar Nori <nsekhar@ti.com>

Fix parent of EMAC and voice codec PSC clocks. Documentation is clear
on EMAC clock parent, but its not fully clear on parent of voice codec
clock. The implementation chosen is matches arch/arm/mach-davinci/dm365.c.
Add a comment explaining this for posterity.

There is only one power domain on DM365. Fix the power domain of voice
codec and vpss dac modules.

While at it, add a comment explaining how the parent of vpss dac clock was
derived. Note that this patch does not touch the parent of vpss dac clock.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
Reviewed-by: David Lechner <david@lechnology.com>
---
 drivers/clk/davinci/psc-dm365.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/davinci/psc-dm365.c b/drivers/clk/davinci/psc-dm365.c
index 3ad915f37376..289af3913fb0 100644
--- a/drivers/clk/davinci/psc-dm365.c
+++ b/drivers/clk/davinci/psc-dm365.c
@@ -65,9 +65,22 @@ static const struct davinci_lpsc_clk_info dm365_psc_info[] = {
 	LPSC(31, 0, arm,         pll2_sysclk2, NULL,               LPSC_ALWAYS_ENABLED),
 	LPSC(38, 0, spi3,        pll1_sysclk4, spi3_clkdev,        0),
 	LPSC(39, 0, spi4,        pll1_auxclk,  spi4_clkdev,        0),
-	LPSC(40, 0, emac,        pll2_sysclk4, emac_clkdev,        0),
-	LPSC(44, 1, voice_codec, pll1_sysclk3, voice_codec_clkdev, 0),
-	LPSC(46, 1, vpss_dac,    pll1_sysclk3, vpss_dac_clkdev,    0),
+	LPSC(40, 0, emac,        pll1_sysclk4, emac_clkdev,        0),
+	/*
+	 * The TRM (ARM Subsystem User's Guide) shows two clocks input into
+	 * voice codec module (PLL2 SYSCLK4 with a DIV2 and PLL1 SYSCLK4). Its
+	 * not fully clear from documentation which clock should be considered
+	 * as parent for PSC. The clock chosen here is to maintain
+	 * compatibility with existing code in arch/arm/mach-davinci/dm365.c
+	 */
+	LPSC(44, 0, voice_codec, pll2_sysclk4, voice_codec_clkdev, 0),
+	/*
+	 * Its not fully clear from TRM (ARM Subsystem User's Guide) as to what
+	 * the parent of VPSS DAC LPSC should actually be. PLL1 SYSCLK3 feeds
+	 * into HDVICP and MJCP. The clock chosen here is to remain compatible
+	 * with code existing in arch/arm/mach-davinci/dm365.c
+	 */
+	LPSC(46, 0, vpss_dac,    pll1_sysclk3, vpss_dac_clkdev,    0),
 	LPSC(47, 0, vpss_master, pll1_sysclk5, vpss_master_clkdev, 0),
 	LPSC(50, 0, mjcp,        pll1_sysclk3, NULL,               0),
 	{ }
-- 
2.17.0

^ permalink raw reply related

* [PATCH 4/9] clk: davinci: pll-dm646x: keep PLL2 SYSCLK1 always enabled
From: David Lechner @ 2018-05-25 18:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525181150.17873-1-david@lechnology.com>

From: Sekhar Nori <nsekhar@ti.com>

PLL2 SYSCLK1 on DM646x is connected to DDR2 PHY and cannot
be disabled. Mark it so to prevent unused clock disable
infrastructure from disabling it.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
Reviewed-by: David Lechner <david@lechnology.com>
---
 drivers/clk/davinci/pll-dm646x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/davinci/pll-dm646x.c b/drivers/clk/davinci/pll-dm646x.c
index a61cc3256418..0ae827e3ce80 100644
--- a/drivers/clk/davinci/pll-dm646x.c
+++ b/drivers/clk/davinci/pll-dm646x.c
@@ -72,7 +72,7 @@ static const struct davinci_pll_clk_info dm646x_pll2_info = {
 	.flags = 0,
 };
 
-SYSCLK(1, pll2_sysclk1, pll2_pllen, 4, 0);
+SYSCLK(1, pll2_sysclk1, pll2_pllen, 4, SYSCLK_ALWAYS_ENABLED);
 
 int dm646x_pll2_init(struct device *dev, void __iomem *base)
 {
-- 
2.17.0

^ permalink raw reply related

* [PATCH 3/9] clk: davinci: psc-dm355: fix ASP0/1 clkdev lookups
From: David Lechner @ 2018-05-25 18:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525181150.17873-1-david@lechnology.com>

The clkdev lookups for the ASP0/1 devices on TI DM355 were declared, but
not assigned to any LPSC. This assigns the clkdev lookups to the
correct LPSCs.

Reported-by: Sekhar Nori <nsekhar@ti.com>
Signed-off-by: David Lechner <david@lechnology.com>
Reviewed-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/clk/davinci/psc-dm355.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/davinci/psc-dm355.c b/drivers/clk/davinci/psc-dm355.c
index 6995ecea2677..128e7345b20c 100644
--- a/drivers/clk/davinci/psc-dm355.c
+++ b/drivers/clk/davinci/psc-dm355.c
@@ -41,14 +41,14 @@ static const struct davinci_lpsc_clk_info dm355_psc_info[] = {
 	LPSC(5,  0, timer3,      pll1_auxclk,  NULL,               0),
 	LPSC(6,  0, spi1,        pll1_sysclk2, spi1_clkdev,        0),
 	LPSC(7,  0, mmcsd1,      pll1_sysclk2, mmcsd1_clkdev,      0),
-	LPSC(8,  0, asp1,        pll1_sysclk2, NULL,               0),
+	LPSC(8,  0, asp1,        pll1_sysclk2, mcbsp1_clkdev,      0),
 	LPSC(9,  0, usb,         pll1_sysclk2, usb_clkdev,         0),
 	LPSC(10, 0, pwm3,        pll1_auxclk,  NULL,               0),
 	LPSC(11, 0, spi2,        pll1_sysclk2, spi2_clkdev,        0),
 	LPSC(12, 0, rto,         pll1_auxclk,  NULL,               0),
 	LPSC(14, 0, aemif,       pll1_sysclk2, aemif_clkdev,       0),
 	LPSC(15, 0, mmcsd0,      pll1_sysclk2, mmcsd0_clkdev,      0),
-	LPSC(17, 0, asp0,        pll1_sysclk2, NULL,               0),
+	LPSC(17, 0, asp0,        pll1_sysclk2, mcbsp0_clkdev,      0),
 	LPSC(18, 0, i2c,         pll1_auxclk,  i2c_clkdev,         0),
 	LPSC(19, 0, uart0,       pll1_auxclk,  uart0_clkdev,       0),
 	LPSC(20, 0, uart1,       pll1_auxclk,  uart1_clkdev,       0),
-- 
2.17.0

^ permalink raw reply related

* [PATCH 2/9] clk: davinci: pll-dm355: fix SYSCLKn parent names
From: David Lechner @ 2018-05-25 18:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525181150.17873-1-david@lechnology.com>

This fixes the parent clock names of the SYSCLKn clocks for the DM355
SoC in the TI DaVinici PLL clock driver.

It appears that this name just didn't get updated to the correct name
like the other SoCs during the driver's development.

Reported-by: Sekhar Nori <nsekhar@ti.com>
Signed-off-by: David Lechner <david@lechnology.com>
Acked-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/clk/davinci/pll-dm355.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/davinci/pll-dm355.c b/drivers/clk/davinci/pll-dm355.c
index 718d9bbbf30d..93f4a53d6b44 100644
--- a/drivers/clk/davinci/pll-dm355.c
+++ b/drivers/clk/davinci/pll-dm355.c
@@ -22,10 +22,10 @@ static const struct davinci_pll_clk_info dm355_pll1_info = {
 		 PLL_POSTDIV_ALWAYS_ENABLED | PLL_POSTDIV_FIXED_DIV,
 };
 
-SYSCLK(1, pll1_sysclk1, pll1, 5, SYSCLK_FIXED_DIV | SYSCLK_ALWAYS_ENABLED);
-SYSCLK(2, pll1_sysclk2, pll1, 5, SYSCLK_FIXED_DIV | SYSCLK_ALWAYS_ENABLED);
-SYSCLK(3, pll1_sysclk3, pll1, 5, SYSCLK_ALWAYS_ENABLED);
-SYSCLK(4, pll1_sysclk4, pll1, 5, SYSCLK_ALWAYS_ENABLED);
+SYSCLK(1, pll1_sysclk1, pll1_pllen, 5, SYSCLK_FIXED_DIV | SYSCLK_ALWAYS_ENABLED);
+SYSCLK(2, pll1_sysclk2, pll1_pllen, 5, SYSCLK_FIXED_DIV | SYSCLK_ALWAYS_ENABLED);
+SYSCLK(3, pll1_sysclk3, pll1_pllen, 5, SYSCLK_ALWAYS_ENABLED);
+SYSCLK(4, pll1_sysclk4, pll1_pllen, 5, SYSCLK_ALWAYS_ENABLED);
 
 int dm355_pll1_init(struct device *dev, void __iomem *base)
 {
@@ -62,7 +62,7 @@ static const struct davinci_pll_clk_info dm355_pll2_info = {
 		 PLL_POSTDIV_ALWAYS_ENABLED | PLL_POSTDIV_FIXED_DIV,
 };
 
-SYSCLK(1, pll2_sysclk1, pll2, 5, SYSCLK_FIXED_DIV | SYSCLK_ALWAYS_ENABLED);
+SYSCLK(1, pll2_sysclk1, pll2_pllen, 5, SYSCLK_FIXED_DIV | SYSCLK_ALWAYS_ENABLED);
 
 int dm355_pll2_init(struct device *dev, void __iomem *base)
 {
-- 
2.17.0

^ permalink raw reply related

* [PATCH 1/9] clk: davinci: pll-dm355: drop pll2_sysclk2
From: David Lechner @ 2018-05-25 18:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525181150.17873-1-david@lechnology.com>

This removes pll2_sysclk2 from the TI DM355 clock driver. This SoC
doesn't have such a clock. Also, SYSCLK_ALWAYS_ENABLED is transferred
to pll2_sysclk1 since it drives the DDR and doesn't have another
mechanism to keep it on.

Reported-by: Sekhar Nori <nsekhar@ti.com>
Signed-off-by: David Lechner <david@lechnology.com>
Acked-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/clk/davinci/pll-dm355.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/clk/davinci/pll-dm355.c b/drivers/clk/davinci/pll-dm355.c
index 5345f8286c50..718d9bbbf30d 100644
--- a/drivers/clk/davinci/pll-dm355.c
+++ b/drivers/clk/davinci/pll-dm355.c
@@ -62,8 +62,7 @@ static const struct davinci_pll_clk_info dm355_pll2_info = {
 		 PLL_POSTDIV_ALWAYS_ENABLED | PLL_POSTDIV_FIXED_DIV,
 };
 
-SYSCLK(1, pll2_sysclk1, pll2, 5, SYSCLK_FIXED_DIV);
-SYSCLK(2, pll2_sysclk2, pll2, 5, SYSCLK_FIXED_DIV | SYSCLK_ALWAYS_ENABLED);
+SYSCLK(1, pll2_sysclk1, pll2, 5, SYSCLK_FIXED_DIV | SYSCLK_ALWAYS_ENABLED);
 
 int dm355_pll2_init(struct device *dev, void __iomem *base)
 {
@@ -71,8 +70,6 @@ int dm355_pll2_init(struct device *dev, void __iomem *base)
 
 	davinci_pll_sysclk_register(dev, &pll2_sysclk1, base);
 
-	davinci_pll_sysclk_register(dev, &pll2_sysclk2, base);
-
 	davinci_pll_sysclkbp_clk_register(dev, "pll2_sysclkbp", base);
 
 	return 0;
-- 
2.17.0

^ permalink raw reply related

* [PATCH 0/9] clk: davinci: outstanding fixes
From: David Lechner @ 2018-05-25 18:11 UTC (permalink / raw)
  To: linux-arm-kernel

This is a resend of all of the outstanding DaVinci clock patches plus one new
patch. All of the patches (except the new one) have been reviewed and tested
by someone other than me.

The new patch ("clk: davinci: Fix link errors when not all SoCs are enabled")
should be fairly trivial.

I am resending them all in one series to hopefully make it easier to get them
picked up by having them in the correct order to avoid merge conflicts. This
series is based on the clk/clk-davinci-psc-da830 branch.

David Lechner (7):
  clk: davinci: pll-dm355: drop pll2_sysclk2
  clk: davinci: pll-dm355: fix SYSCLKn parent names
  clk: davinci: psc-dm355: fix ASP0/1 clkdev lookups
  clk: davinci: pll: allow dev == NULL
  clk: davinci: da850-pll: change PLL0 to CLK_OF_DECLARE
  clk: davinci: psc: allow for dev == NULL
  clk: davinci: Fix link errors when not all SoCs are enabled

Sekhar Nori (2):
  clk: davinci: pll-dm646x: keep PLL2 SYSCLK1 always enabled
  clk: davinci: psc-dm365: fix few clocks

 drivers/clk/davinci/pll-da830.c  |   5 +-
 drivers/clk/davinci/pll-da850.c  |  37 ++--
 drivers/clk/davinci/pll-dm355.c  |  22 ++-
 drivers/clk/davinci/pll-dm365.c  |   9 +-
 drivers/clk/davinci/pll-dm644x.c |   9 +-
 drivers/clk/davinci/pll-dm646x.c |  11 +-
 drivers/clk/davinci/pll.c        | 299 +++++++++++++++++++++----------
 drivers/clk/davinci/pll.h        |  41 +++--
 drivers/clk/davinci/psc-dm355.c  |   7 +-
 drivers/clk/davinci/psc-dm365.c  |  22 ++-
 drivers/clk/davinci/psc-dm644x.c |   3 +-
 drivers/clk/davinci/psc-dm646x.c |   3 +-
 drivers/clk/davinci/psc.c        |  72 ++++++--
 drivers/clk/davinci/psc.h        |  12 ++
 include/linux/clk/davinci.h      |  40 +++++
 15 files changed, 418 insertions(+), 174 deletions(-)
 create mode 100644 include/linux/clk/davinci.h

-- 
2.17.0

^ permalink raw reply

* [PATCH] PCI: Try to clean up resources via remove if shutdown doesn't exist
From: Sinan Kaya @ 2018-05-25 17:36 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1527257063-15843-1-git-send-email-okaya@codeaurora.org>

[+cc kexec]

On 5/25/2018 7:04 AM, Sinan Kaya wrote:
> It is up to a driver to implement shutdown() callback. If shutdown()
> callback is not implemented, PCI device can have pending interrupt and
> even do DMA transactions while the system is going down.
> 
> If kexec is in use, this can damage the newly booting kexec kernel
> or even prevent it from booting altogether. Fallback to calling the
> remove() callback if shutdown() isn't implemented for a given driver.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci-driver.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 6ace470..4ac72e3 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -475,8 +475,17 @@ static void pci_device_shutdown(struct device *dev)
>  
>  	pm_runtime_resume(dev);
>  
> +	/*
> +	 * Try shutdown callback if it exists, otherwise fallback to remove
> +	 * callback. PCI drivers can do DMA and have pending interrupts.
> +	 * Leaving the DMA and interrupts pending could damage the newly
> +	 * booting kexec kernel as well as prevent it from booting altogether
> +	 * if the pending interrupt is level.
> +	 */
>  	if (drv && drv->shutdown)
>  		drv->shutdown(pci_dev);
> +	else if (drv && drv->remove)
> +		drv->remove(pci_dev);
>  
>  	/*
>  	 * If this is a kexec reboot, turn off Bus Master bit on the
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply

* [PATCH v2 1/8] driver core: make deferring probe after init optional
From: Rob Herring @ 2018-05-25 17:35 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <92e3be10-e65a-99e9-6ef7-f11ded6a35f9@arm.com>

On Fri, May 25, 2018 at 7:20 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 24/05/18 21:57, Rob Herring wrote:
>>
>> On Thu, May 24, 2018 at 2:00 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>>
>>> On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote:
>>>>
>>>> Deferred probe will currently wait forever on dependent devices to
>>>> probe,
>>>> but sometimes a driver will never exist. It's also not always critical
>>>> for
>>>> a driver to exist. Platforms can rely on default configuration from the
>>>> bootloader or reset defaults for things such as pinctrl and power
>>>> domains.
>>>> This is often the case with initial platform support until various
>>>> drivers
>>>> get enabled. There's at least 2 scenarios where deferred probe can
>>>> render
>>>> a platform broken. Both involve using a DT which has more devices and
>>>> dependencies than the kernel supports. The 1st case is a driver may be
>>>> disabled in the kernel config. The 2nd case is the kernel version may
>>>> simply not have the dependent driver. This can happen if using a newer
>>>> DT
>>>> (provided by firmware perhaps) with a stable kernel version.
>>>>
>>>> Subsystems or drivers may opt-in to this behavior by calling
>>>> driver_deferred_probe_check_init_done() instead of just returning
>>>> -EPROBE_DEFER. They may use additional information from DT or kernel's
>>>> config to decide whether to continue to defer probe or not.
>>>>
>>>> Cc: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>>> ---
>>>>   drivers/base/dd.c      | 17 +++++++++++++++++
>>>>   include/linux/device.h |  2 ++
>>>>   2 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>>>> index c9f54089429b..d6034718da6f 100644
>>>> --- a/drivers/base/dd.c
>>>> +++ b/drivers/base/dd.c
>>>> @@ -226,6 +226,16 @@ void device_unblock_probing(void)
>>>>        driver_deferred_probe_trigger();
>>>>   }
>>>>
>>>> +int driver_deferred_probe_check_init_done(struct device *dev, bool
>>>> optional)
>>>> +{
>>>> +     if (optional && initcalls_done) {
>>>
>>>
>>> Wait, what's the "optional" mess here?
>>
>>
>> My intent was that subsystems just always call this function and never
>> return EPROBE_DEFER themselves. Then the driver core can make
>> decisions as to what to do (such as the timeout added in the next
>> patch). Or it can print common error/debug messages. So optional is a
>> hint to allow subsystems per device control.
>
>
> Maybe just driver_defer_probe() might be a more descriptive name? To me,
> calling "foo_check_x()" with a parameter that says "I don't actually care
> about x" is the really unintuitive bit.

All the other (though static or internal to driver core) functions are
prefixed driver_deferred_probe_* so I was trying to remain consistent
there. You're right though, with the timeout it's not just whether
initcalls are done. It's really "get the return value depending on the
core's deferred probe state". So perhaps one of these:

driver_deferred_probe_get_return_val()
driver_deferred_probe_handle_return()

The other option would be a more straight-forward functions that just
returns a bool on whether to continue deferring and leave the return
code handling to the caller:

if (driver_deferred_probe_enabled_for_builtin(dev))
  return -EPROBE_DEFER;
else
  return -ENODEV;

The pinctrl case would look like this:

builtin_only = of_property_read_bool(np_pctldev, "pinctrl-use-default");
if (builtin_only && driver_deferred_probe_enabled_for_builtin(dev))
  return -EPROBE_DEFER;
else if (!builtin_only && driver_deferred_probe_enabled(dev))
  return -EPROBE_DEFER;
else
  return -ENODEV;

I still prefer the former, picking the bike shed color is easier with
the latter.

>>>
>>> The caller knows this value, so why do you need to even pass it in here?
>>
>>
>> Because regardless of the value, we always stop deferring when/if we
>> hit the timeout and the caller doesn't know about the timeout. If we
>> get rid of it, we'd need functions for both init done and for deferred
>> timeout.
>>
>>> And bool values that are not obvious are horrid.  I had to go look this
>>> up when reading the later patches that just passed "true" in this
>>> variable as I had no idea what that meant.
>>
>>
>> Perhaps inverting it and calling it "keep_deferring" would be better.
>> However, the flag is ignored if we have timed out.
>
>
> Perhaps an enum (or bitmask of named flags) then? That would allow the most
> readability at callsites, plus it seems quite likely that we may want
> intermediate degrees of "deferral strictness" eventually.

A bitmask is just 32 booleans stuffed into one parameter which I can
guess Greg's opinion on.

I can't really think of other flags we might need here. If we added
some userspace trigger saying module loading is done, I don't think
we'd need that to be per caller.

Rob

^ permalink raw reply

* i2c-gpio and boards conversions to GPIO descriptors
From: Simon Guinot @ 2018-05-25 17:30 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180525154144.GC19100@kw.sim.vm.gnt>

On Fri, May 25, 2018 at 05:41:44PM +0200, Simon Guinot wrote:
> Hi Linus,

Forwarding to some mailing lists I missed in a first place.

> 
> I think your patch b2e63555592f "i2c: gpio: Convert to use descriptors"
> may have broken i2c-gpio support for some boards using old fashion
> platform_device declarations.
> 
> Indeed when an "i2c-gpio" platform_device is registered with a fixed id
> e.g. 0, then I think the device name becomes "i2c-gpio.0". And then this
> won't match a lookup table registered with an "i2c-gpio" dev_id.
> 
> Please double check this :)
> 
> Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180525/4bcae35d/attachment.sig>

^ permalink raw reply

* [PATCH 6/6] coresight: allow to build as modules
From: Mathieu Poirier @ 2018-05-25 17:27 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <4986a636-d5a5-387c-abf1-d68afe063f06@arm.com>

On 25 May 2018 at 11:12, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 18/05/18 02:20, Kim Phillips wrote:
>>
>> Allow to build coresight as modules.  This greatly enhances developer
>> efficiency by allowing the development to take place exclusively on the
>> target, and without needing to reboot in between changes.
>>
>> - Kconfig bools become tristates, to allow =m
>>
>> - use -objs to denote merge object directives in Makefile, adds a
>>    coresight-core nomenclature for the base module.
>>
>> - Export core functions so as to be able to be used by
>>    non-core modules.
>>
>> - add a coresight_exit() that unregisters the coresight bus, add
>>    remove fns for most others.
>>
>> - fix up modules with ID tables for autoloading on boot
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Randy Dunlap <rdunlap@infradead.org>
>> Signed-off-by: Kim Phillips <kim.phillips@arm.com>
>
>
> Kim,
>
> I see that you have addressed my comments on a previous version
> of this series posted in April. But I don't see the version number
> increased for this new version. Please add versioning to make it
> easier to make it more obvious.
>
> Also, generally it is a good idea to keep the people who reviewed
> and commented on your previous versions in the newer versions.
>
> Some comments below :
>
>> diff --git a/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
>> b/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
>> index fc742215ab05..bc42b8022556 100644
>> --- a/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
>> +++ b/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
>> @@ -37,7 +37,12 @@ struct replicator_state {
>>   static int replicator_enable(struct coresight_device *csdev, int inport,
>>                               int outport)
>>   {
>> -       struct replicator_state *drvdata =
>> dev_get_drvdata(csdev->dev.parent);
>> +       struct device *parent_dev = csdev->dev.parent;
>> +       struct replicator_state *drvdata = dev_get_drvdata(parent_dev);
>> +       struct module *module = parent_dev->driver->owner;
>> +
>> +       if (!try_module_get(module))
>> +               return -ENODEV;
>>         CS_UNLOCK(drvdata->base);
>
>
> What is the guarantee that the "csdev" is still available when we reach
> here ?
>
> A module could be unloaded "after the component was added to the path"
> (via coresight_build_path) and before we invoke the "enable" on each
> component in the path ?

Very good point - this is invariably racy.

>
> Also, it  is tedious to do module_get in "enable" and module_put in the
> disable call backs for each component.
>
> Instead, if we do a module_get() in build_path and module_put() in
> release path, we could solve all these problems and keep it the module
> refcount in a central place.

Good idea, it does streamline things a lot.

>
>> +MODULE_DEVICE_TABLE(amba, replicator_ids);
>> +
>>   static struct amba_driver replicator_driver = {
>>         .drv = {
>>                 .name   = "coresight-dynamic-replicator",
>> @@ -207,9 +226,10 @@ static struct amba_driver replicator_driver = {
>>                 .suppress_bind_attrs = true,
>>         },
>>         .probe          = replicator_probe,
>> +       .remove         = replicator_remove,
>>         .id_table       = replicator_ids,
>>   };
>
>
> Do we have the owner field set here for this driver ? I see that you
> added it for some components and not others. e.g, you have added it for
> etm4x, while not for replicator and others.
>
>
>> +MODULE_DEVICE_TABLE(amba, etm4_ids);
>> +
>>   static struct amba_driver etm4x_driver = {
>>         .drv = {
>>                 .name   = "coresight-etm4x",
>> +               .owner  = THIS_MODULE,
>>                 .suppress_bind_attrs = true,
>>         },
>>         .probe          = etm4_probe,
>> +       .remove         = etm4_remove,
>>         .id_table       = etm4_ids,
>>   };
>> -builtin_amba_driver(etm4x_driver);
>> +module_amba_driver(etm4x_driver);
>
>
>
> Suzuki

^ permalink raw reply

* [PATCH 6/6] coresight: allow to build as modules
From: Mathieu Poirier @ 2018-05-25 17:21 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180524184908.3648cf67456510261a1afe16@arm.com>

On 24 May 2018 at 17:49, Kim Phillips <kim.phillips@arm.com> wrote:
> On Tue, 22 May 2018 15:39:06 -0600
> Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>
>> On Thu, May 17, 2018 at 08:20:24PM -0500, Kim Phillips wrote:
>> > Allow to build coresight as modules.  This greatly enhances developer
>> > efficiency by allowing the development to take place exclusively on the
>> > target, and without needing to reboot in between changes.
>> >
>> > - Kconfig bools become tristates, to allow =m
>> >
>> > - use -objs to denote merge object directives in Makefile, adds a
>> >   coresight-core nomenclature for the base module.
>> >
>> > - Export core functions so as to be able to be used by
>> >   non-core modules.
>> >
>> > - add a coresight_exit() that unregisters the coresight bus, add
>> >   remove fns for most others.
>> >
>> > - fix up modules with ID tables for autoloading on boot
>> >
>> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> > Cc: Randy Dunlap <rdunlap@infradead.org>
>> > Signed-off-by: Kim Phillips <kim.phillips@arm.com>
>> > ---
>> >  drivers/hwtracing/coresight/Kconfig           | 48 +++++++++++++++----
>> >  drivers/hwtracing/coresight/Makefile          | 28 +++++++----
>> >  .../hwtracing/coresight/coresight-cpu-debug.c |  2 +
>> >  .../coresight/coresight-dynamic-replicator.c  | 26 ++++++++--
>> >  drivers/hwtracing/coresight/coresight-etb10.c | 27 +++++++++--
>> >  .../hwtracing/coresight/coresight-etm-perf.c  |  9 +++-
>> >  .../coresight/coresight-etm3x-sysfs.c         |  1 +
>> >  drivers/hwtracing/coresight/coresight-etm3x.c | 32 +++++++++++--
>> >  .../coresight/coresight-etm4x-sysfs.c         |  1 +
>> >  drivers/hwtracing/coresight/coresight-etm4x.c | 33 +++++++++++--
>> >  .../hwtracing/coresight/coresight-funnel.c    | 26 ++++++++--
>> >  drivers/hwtracing/coresight/coresight-priv.h  |  1 -
>> >  .../coresight/coresight-replicator.c          | 28 +++++++++--
>> >  drivers/hwtracing/coresight/coresight-stm.c   | 23 ++++++++-
>> >  drivers/hwtracing/coresight/coresight-tmc.c   | 18 ++++++-
>> >  drivers/hwtracing/coresight/coresight-tpiu.c  | 26 ++++++++--
>> >  drivers/hwtracing/coresight/coresight.c       | 14 ++++++
>> >  17 files changed, 299 insertions(+), 44 deletions(-)
>>
>> For the next revision please split the work based on files.
>
> If I read that literally, one file-by-one would break build
> bisectability.  Do you mean split by source files depending on the
> logical modules they belong to, e.g., etm3x, etm4x, etb10, etc.?  If

I meant to introduce all the needed changes - including the module
author, description and licences - per module.  That will break up
this patch considerably and allow us to concentrate on individual
component.  As a final step do the changes to Kconfig and the
Makefile.


> so, I think it would look like the coresight-core would be first,
> followed by the rest, but I also think there are cross-dependencies.
> Hmm, OK, I'll have a look, but there's also one more thing:  I think
> the Makefile  obj '-core' nomenclature was to change the name of the
> module to not be the same as the core source file, so what do you think
> about renaming the core source file instead of the module name?  e.g.:
>
> Instead of this:
>
> obj-$(CONFIG_CORESIGHT) += coresight-core.o
> coresight-core-objs := coresight.o \
>                        of_coresight.o
>
> we have this:
>
> obj-$(CONFIG_CORESIGHT) += coresight.o
> coresight-objs := coresight-core.o \
>                   of_coresight.o
>
> and e.g., instead of this:
>
> obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x-core.o
> coresight-etm3x-core-objs := coresight-etm3x.o \
>                              coresight-etm-cp14.o \
>                              coresight-etm3x-sysfs.o
>
> we have this:
>
> obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
> coresight-etm3x-objs := coresight-etm3x-core.o \
>                         coresight-etm-cp14.o \
>                         coresight-etm3x-sysfs.o
>
> ?

I think that is much better and avoid carrying the heave "-core"
appendage.  Renaming needs to be a patch on its own (but still part of
this set).

>
>> > +static int __exit tmc_remove(struct amba_device *adev)
>> > +{
>> > +   struct tmc_drvdata *drvdata = dev_get_drvdata(&adev->dev);
>> > +
>> > +   /* free ETB/ETF or ETR memory */
>> > +   tmc_read_unprepare(drvdata);
>> > +
>> > +   misc_deregister(&drvdata->miscdev);
>> > +   coresight_unregister(drvdata->csdev);
>> > +
>> > +   return 0;
>> > +}
>> > +
>>
>> Right now I can remove the module for a TMC link or sink when part of an active
>> session, something I pointed out during an earlier revision.
>
> Right, I missed that :(
>
> So would the first thing tmc_remove does is this:
>
> if (drvdata->reading)
>         return -EBUSY;
>
> work, or would we need to introduce a new sentinel?

The ->reading flag is to prevent concurrent access to the buffer from
sysFS and to avoid clobbering said buffer with new trace data.  The
TMC driver shouldn't be different from the other ones with regards to
the usage of the try_module_get()/module_put() functions.

>
>> I also think we need to deal with driver removal cases when the TMC buffer
>> (ETR or ETF) is being read from sysFS.
>
> OK, I thought the:
>
> struct file_operations tmc_fops = {
>         .owner = THIS_MODULE,
>
> would prevent module unload whilst sysfs access was being performed,
> but I'll double check.

Right, just check that it works as advertised.

>
> Thanks,
>
> Kim

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox