From: Bartosz Golaszewski <[email protected]>
This is a follow to my series[1] the aim of which was to introduce device tree
support for early platform devices.
It was received rather negatively. Aside from using device tree to pass
implementation specific details to the system, two important concerns were
raised: no probe deferral support and the fact that currently the early devices
never get converted to actual platform drivers. This series is a
proof-of-concept that's trying to address those issues.
The only user of the current version of early platform drivers is the SuperH
architecture. If this series eventually gets merged, we could simply replace
the other solution.
The current implementation of early platform drivers is pretty much a hack
built on top of the early_param mechanism. The devices only look like platform
devices and use the same structures but never actually get registered with the
driver model.
The idea behind this series is to allow users to probe platform devices very
early in the boot sequence and then switch the early devices to actual platform
devices and the early drivers to platform drivers in during device_initcall.
If any of the early registration functions is called after device_initcall,
we'll just end up calling the normal platform_driver/device_register routines.
The user can specify if he wants the device to be probed the second time after
this conversion and the check if it's an early or a normal probe from the
driver code.
We also support a simple version of probe deferral: initially each new
registered device is added to the head of the early devices list. If it matches
an early driver, it will be probed. If probe return -EPROBE_DEFER, it will be
moved to the tail of the list and reprobed the next time we match a device
but after all other devices.
This implementation has certain shortcomings that will be addressed if the
feedback is at least somewhat positive. For instance: the driver registration
happens in early_initcall(). This may be too late for certain clocksource or
clk drivers. The solution for that would be defining a new section in which the
init callbacks of the drivers would reside and let the architecture call the
actual registration function whenever it's needed.
I also need to figure out if any locking is needed.
We don't support DT in this series either. The proposed approach would be to
walk over the devices nodes early in the boot sequence and allocate and probe
the matching early devices and then register them with the driver model later.
[1] https://lkml.org/lkml/2018/4/24/937
Bartosz Golaszewski (2):
earlydev: implement a new way to probe platform devices early
misc: implement a dummy early platform driver
drivers/base/Kconfig | 3 +
drivers/base/Makefile | 1 +
drivers/base/earlydev.c | 175 ++++++++++++++++++++++++++++++++
drivers/base/platform.c | 11 ++
drivers/misc/Kconfig | 8 ++
drivers/misc/Makefile | 1 +
drivers/misc/dummy-early.c | 46 +++++++++
include/linux/earlydev.h | 63 ++++++++++++
include/linux/platform_device.h | 4 +
9 files changed, 312 insertions(+)
create mode 100644 drivers/base/earlydev.c
create mode 100644 drivers/misc/dummy-early.c
create mode 100644 include/linux/earlydev.h
--
2.17.0
From: Bartosz Golaszewski <[email protected]>
The current implementation of early platform drivers is pretty much a
hack built on top of the early_param mechanism. The devices only look
like platform devices and use the same structures but never actually
get registered with the driver model.
The idea behind this series is to allow users to probe platform devices
very early in the boot sequence and then switch the early devices to
actual platform devices and the early drivers to platform drivers in
during device_initcall.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/base/Kconfig | 3 +
drivers/base/Makefile | 1 +
drivers/base/earlydev.c | 175 ++++++++++++++++++++++++++++++++
drivers/base/platform.c | 11 ++
include/linux/earlydev.h | 63 ++++++++++++
include/linux/platform_device.h | 4 +
6 files changed, 257 insertions(+)
create mode 100644 drivers/base/earlydev.c
create mode 100644 include/linux/earlydev.h
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 29b0eb452b3a..e05f96d626b3 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -170,6 +170,9 @@ config DEV_COREDUMP
default y if WANT_DEV_COREDUMP
depends on ALLOW_DEV_COREDUMP
+config EARLYDEV
+ def_bool n
+
config DEBUG_DRIVER
bool "Driver Core verbose debug messages"
depends on DEBUG_KERNEL
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index b074f242a435..ec47f86eac44 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -7,6 +7,7 @@ obj-y := component.o core.o bus.o dd.o syscore.o \
attribute_container.o transport_class.o \
topology.o container.o property.o cacheinfo.o \
devcon.o
+obj-$(CONFIG_EARLYDEV) += earlydev.o
obj-$(CONFIG_DEVTMPFS) += devtmpfs.o
obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
obj-y += power/
diff --git a/drivers/base/earlydev.c b/drivers/base/earlydev.c
new file mode 100644
index 000000000000..3da9e81031d2
--- /dev/null
+++ b/drivers/base/earlydev.c
@@ -0,0 +1,175 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Texas Instruments
+ * Author: Bartosz Golaszewski <[email protected]>
+ */
+
+#include <linux/earlydev.h>
+#include <linux/slab.h>
+
+#include "base.h"
+
+static bool early_done;
+static LIST_HEAD(early_drvs);
+static LIST_HEAD(early_devs);
+
+static void earlydev_pdev_set_name(struct platform_device *pdev)
+{
+ if (pdev->dev.init_name)
+ return;
+
+ if (!slab_is_available()) {
+ pr_warn("slab unavailable - not assigning name to early device\n");
+ return;
+ }
+
+ switch (pdev->id) {
+ case PLATFORM_DEVID_NONE:
+ pdev->dev.init_name = kasprintf(GFP_KERNEL, "%s", pdev->name);
+ break;
+ case PLATFORM_DEVID_AUTO:
+ pr_warn("auto device ID not supported in early devices\n");
+ break;
+ default:
+ pdev->dev.init_name = kasprintf(GFP_KERNEL, "%s.%d",
+ pdev->name, pdev->id);
+ break;
+ }
+
+ if (!pdev->dev.init_name)
+ pr_warn("error allocating the early device name\n");
+}
+
+static void earlydev_probe_devices(void)
+{
+ struct earlydev_driver *edrv, *ndrv;
+ struct earlydev_device *edev, *ndev;
+ int rv;
+
+ list_for_each_entry_safe(edev, ndev, &early_devs, list) {
+ if (edev->bound_to)
+ continue;
+
+ list_for_each_entry_safe(edrv, ndrv, &early_drvs, list) {
+ if (strcmp(edrv->plat_drv.driver.name,
+ edev->pdev.name) != 0)
+ continue;
+
+ earlydev_pdev_set_name(&edev->pdev);
+ rv = edrv->plat_drv.probe(&edev->pdev);
+ if (rv) {
+ if (rv == -EPROBE_DEFER) {
+ /*
+ * Move the device to the end of the
+ * list so that it'll be reprobed next
+ * time after all new devices.
+ */
+ list_move_tail(&edev->list,
+ &early_devs);
+ continue;
+ }
+
+ pr_err("error probing early device: %d\n", rv);
+ continue;
+ }
+
+ edev->bound_to = edrv;
+ edev->pdev.early_probed = true;
+ }
+ }
+}
+
+bool earlydev_probing_early(void)
+{
+ return !early_done;
+}
+
+bool earlydev_probe_late(struct platform_device *pdev)
+{
+ struct earlydev_device *edev;
+
+ edev = container_of(pdev, struct earlydev_device, pdev);
+
+ return edev->probe_late;
+}
+
+void __earlydev_driver_register(struct earlydev_driver *edrv,
+ struct module *owner)
+{
+ if (early_done) {
+ __platform_driver_register(&edrv->plat_drv, owner);
+ return;
+ }
+
+ pr_debug("registering early driver: %s\n", edrv->plat_drv.driver.name);
+
+ edrv->plat_drv.driver.owner = owner;
+
+ INIT_LIST_HEAD(&edrv->list);
+ list_add(&early_drvs, &edrv->list);
+
+ earlydev_probe_devices();
+}
+EXPORT_SYMBOL_GPL(__earlydev_driver_register);
+
+void earlydev_device_add(struct earlydev_device *edev)
+{
+ if (early_done) {
+ platform_device_register(&edev->pdev);
+ return;
+ }
+
+ pr_debug("adding early device: %s\n", edev->pdev.name);
+
+ INIT_LIST_HEAD(&edev->list);
+ list_add(&early_devs, &edev->list);
+
+ earlydev_probe_devices();
+}
+EXPORT_SYMBOL_GPL(earlydev_device_add);
+
+static void earlydev_drivers_to_platform(void)
+{
+ struct earlydev_driver *edrv, *n;
+ struct platform_driver *pdrv;
+ int rv;
+
+ list_for_each_entry_safe(edrv, n, &early_drvs, list) {
+ pdrv = &edrv->plat_drv;
+
+ rv = __platform_driver_register(pdrv, pdrv->driver.owner);
+ if (rv)
+ pr_err("error switching early to platform driver: %d\n",
+ rv);
+
+ list_del(&edrv->list);
+ }
+}
+
+static void earlydev_devices_to_platform(void)
+{
+ struct earlydev_device *edev, *n;
+ struct platform_device *pdev;
+ int rv;
+
+ list_for_each_entry_safe(edev, n, &early_devs, list) {
+ pdev = &edev->pdev;
+
+ rv = platform_device_register(pdev);
+ if (rv)
+ pr_err("error switching early to platform device: %d\n",
+ rv);
+ }
+}
+
+static int earlydev_switch_to_platform(void)
+{
+ pr_debug("switching early drivers and devices to platform\n");
+ early_done = true;
+
+ earlydev_drivers_to_platform();
+ earlydev_devices_to_platform();
+
+ return 0;
+}
+device_initcall(earlydev_switch_to_platform);
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 8075ddc70a17..fb51ce242f6c 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -26,6 +26,7 @@
#include <linux/clk/clk-conf.h>
#include <linux/limits.h>
#include <linux/property.h>
+#include <linux/earlydev.h>
#include "base.h"
#include "power/power.h"
@@ -574,7 +575,17 @@ static int platform_drv_probe(struct device *_dev)
ret = dev_pm_domain_attach(_dev, true);
if (ret != -EPROBE_DEFER) {
if (drv->probe) {
+#ifdef CONFIG_EARLYDEV
+ if (dev->early_probed && !earlydev_probe_late(dev)) {
+ /* Skip this probe if probed early. */
+ dev->early_probed = false;
+ ret = 0;
+ } else {
+ ret = drv->probe(dev);
+ }
+#else /* CONFIG_EARLYDEV */
ret = drv->probe(dev);
+#endif /* CONFIG_EARLYDEV */
if (ret)
dev_pm_domain_detach(_dev, true);
} else {
diff --git a/include/linux/earlydev.h b/include/linux/earlydev.h
new file mode 100644
index 000000000000..7f190a904405
--- /dev/null
+++ b/include/linux/earlydev.h
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Texast Instruments
+ * Author: Bartosz Golaszewski <[email protected]>
+ */
+
+#ifndef __EARLYDEV_H__
+#define __EARLYDEV_H__
+
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+struct earlydev_driver {
+ struct list_head list;
+ struct platform_driver plat_drv;
+};
+
+struct earlydev_device {
+ struct list_head list;
+ struct platform_device pdev;
+ struct earlydev_driver *bound_to;
+ bool probe_late;
+};
+
+#ifdef CONFIG_EARLYDEV
+extern bool earlydev_probing_early(void);
+extern bool earlydev_probe_late(struct platform_device *pdev);
+extern void __earlydev_driver_register(struct earlydev_driver *edrv,
+ struct module *owner);
+#define earlydev_driver_register(_driver) \
+ __earlydev_driver_register((_driver), THIS_MODULE)
+extern void earlydev_device_add(struct earlydev_device *edev);
+#else /* CONFIG_EARLYDEV */
+static inline void earlydev_probing_early(void)
+{
+ return false;
+}
+static inline bool earlydev_probe_late(struct platform_device *pdev)
+{
+ return true;
+}
+static inline void __earlydev_driver_register(struct earlydev_driver *drv,
+ struct module *owner) {}
+#define earlydev_driver_register(_driver)
+static inline void earlydev_device_add(struct earlydev_device *edev) {}
+#endif /* CONFIG_EARLYDEV */
+
+/*
+ * REVISIT: early_initcall may be still too late for some timers and critical
+ * clocks. We should probably have a separate section with callbacks that can
+ * be invoked at each architecture's discretion.
+ */
+#define earlydev_platform_driver(_drv) \
+ static int _drv##_register(void) \
+ { \
+ earlydev_driver_register(&(_drv)); \
+ return 0; \
+ } \
+ early_initcall(_drv##_register)
+
+#endif /* __EARLYDEV_H__ */
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 49f634d96118..5246f60f9aae 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -36,6 +36,10 @@ struct platform_device {
/* arch specific additions */
struct pdev_archdata archdata;
+
+#ifdef CONFIG_EARLYDEV
+ bool early_probed;
+#endif
};
#define platform_get_device_id(pdev) ((pdev)->id_entry)
--
2.17.0
From: Bartosz Golaszewski <[email protected]>
Implement a very simple early platform driver. Its purpose is to show
how such drivers can be registered and to emit a message when probed.
It can be then added to the device tree or machine code to verify that
the early platform devices work as expected.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/Kconfig | 8 +++++++
drivers/misc/Makefile | 1 +
drivers/misc/dummy-early.c | 46 ++++++++++++++++++++++++++++++++++++++
3 files changed, 55 insertions(+)
create mode 100644 drivers/misc/dummy-early.c
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 5d713008749b..99cde8aefdb0 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -91,6 +91,14 @@ config DUMMY_IRQ
The sole purpose of this module is to help with debugging of systems on
which spurious IRQs would happen on disabled IRQ vector.
+config DUMMY_EARLY
+ bool "Dummy early platform driver"
+ select EARLYDEV
+ default n
+ help
+ This module's only function is to register itself with the early
+ platform device framework and be probed early in the boot process.
+
config IBM_ASM
tristate "Device driver for IBM RSA service processor"
depends on X86 && PCI && INPUT
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 20be70c3f118..84ad0225eb14 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_INTEL_MID_PTI) += pti.o
obj-$(CONFIG_ATMEL_SSC) += atmel-ssc.o
obj-$(CONFIG_ATMEL_TCLIB) += atmel_tclib.o
obj-$(CONFIG_DUMMY_IRQ) += dummy-irq.o
+obj-$(CONFIG_DUMMY_EARLY) += dummy-early.o
obj-$(CONFIG_ICS932S401) += ics932s401.o
obj-$(CONFIG_LKDTM) += lkdtm/
obj-$(CONFIG_TIFM_CORE) += tifm_core.o
diff --git a/drivers/misc/dummy-early.c b/drivers/misc/dummy-early.c
new file mode 100644
index 000000000000..f00fb1fbd5fa
--- /dev/null
+++ b/drivers/misc/dummy-early.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Texas Instruments
+ *
+ * Author:
+ * Bartosz Golaszewski <[email protected]>
+ *
+ * Dummy testing driver whose only purpose is to be registered and probed
+ * using the early platform device mechanism.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/earlydev.h>
+
+static int dummy_early_probe(struct platform_device *pdev)
+{
+ if (earlydev_probing_early())
+ dev_notice(&pdev->dev, "dummy-early driver probed early!\n");
+ else
+ dev_notice(&pdev->dev, "dummy-early driver probed late!\n");
+
+ return 0;
+}
+
+static const struct of_device_id dummy_early_of_match[] = {
+ { .compatible = "none,dummy-early", },
+ { },
+};
+
+static struct earlydev_driver dummy_early_driver = {
+ .plat_drv = {
+ .probe = dummy_early_probe,
+ .driver = {
+ .name = "dummy-early",
+ .of_match_table = dummy_early_of_match,
+ },
+ }
+};
+earlydev_platform_driver(dummy_early_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <[email protected]>");
+MODULE_DESCRIPTION("Dummy early platform device driver");
+MODULE_LICENSE("GPL v2");
--
2.17.0
On Thu, Apr 26, 2018 at 05:29:18PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> This is a follow to my series[1] the aim of which was to introduce device tree
> support for early platform devices.
>
> It was received rather negatively. Aside from using device tree to pass
> implementation specific details to the system, two important concerns were
> raised: no probe deferral support and the fact that currently the early devices
> never get converted to actual platform drivers. This series is a
> proof-of-concept that's trying to address those issues.
>
> The only user of the current version of early platform drivers is the SuperH
> architecture. If this series eventually gets merged, we could simply replace
> the other solution.
Looking at a quick output of:
grep -r -A10 early_devices[[] arch/sh/kernel/
it looks like all of the existing early platform devices are serial
ports, clocks, and clocksources. The switch to device tree should pick
them all up from CLK_OF_DECLARE, TIMER_OF_DECLARE, and
EARLYCON_DECLARE. Until that's complete, the existing code works
as-is. I don't see what problem you're trying to solve.
FYI I'm (sometimes-somewhat-absent) arch/sh co-maintainer.
Rich
On 04/26/2018 12:31 PM, Rich Felker wrote:
> On Thu, Apr 26, 2018 at 05:29:18PM +0200, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <[email protected]>
>>
>> This is a follow to my series[1] the aim of which was to introduce device tree
>> support for early platform devices.
>>
>> It was received rather negatively. Aside from using device tree to pass
>> implementation specific details to the system, two important concerns were
>> raised: no probe deferral support and the fact that currently the early devices
>> never get converted to actual platform drivers. This series is a
>> proof-of-concept that's trying to address those issues.
>>
>> The only user of the current version of early platform drivers is the SuperH
>> architecture. If this series eventually gets merged, we could simply replace
>> the other solution.
>
> Looking at a quick output of:
>
> grep -r -A10 early_devices[[] arch/sh/kernel/
>
> it looks like all of the existing early platform devices are serial
> ports, clocks, and clocksources. The switch to device tree should pick
> them all up from CLK_OF_DECLARE, TIMER_OF_DECLARE, and
> EARLYCON_DECLARE. Until that's complete, the existing code works
> as-is. I don't see what problem you're trying to solve.
The problem for us is that clk maintainers don't want new drivers to use
CLK_OF_DECLARE and instead use platform devices. I have just written such
a new driver that is shared by 6 different SoCs. For some combinations of
SoCs and clocks, using a platform device is fine but on others we need to
register early, so the drivers now have to handle both cases, which is
kind of messy and fragile. If there is a generic way to register platform
devices early, then the code is simplified because we only have to handle
one method of registering the clocks instead of two.
>
> FYI I'm (sometimes-somewhat-absent) arch/sh co-maintainer.
>
> Rich
>
On Thu, Apr 26, 2018 at 09:28:39PM -0500, David Lechner wrote:
> On 04/26/2018 12:31 PM, Rich Felker wrote:
> >On Thu, Apr 26, 2018 at 05:29:18PM +0200, Bartosz Golaszewski wrote:
> >>From: Bartosz Golaszewski <[email protected]>
> >>
> >>This is a follow to my series[1] the aim of which was to introduce device tree
> >>support for early platform devices.
> >>
> >>It was received rather negatively. Aside from using device tree to pass
> >>implementation specific details to the system, two important concerns were
> >>raised: no probe deferral support and the fact that currently the early devices
> >>never get converted to actual platform drivers. This series is a
> >>proof-of-concept that's trying to address those issues.
> >>
> >>The only user of the current version of early platform drivers is the SuperH
> >>architecture. If this series eventually gets merged, we could simply replace
> >>the other solution.
> >
> >Looking at a quick output of:
> >
> > grep -r -A10 early_devices[[] arch/sh/kernel/
> >
> >it looks like all of the existing early platform devices are serial
> >ports, clocks, and clocksources. The switch to device tree should pick
> >them all up from CLK_OF_DECLARE, TIMER_OF_DECLARE, and
> >EARLYCON_DECLARE. Until that's complete, the existing code works
> >as-is. I don't see what problem you're trying to solve.
>
> The problem for us is that clk maintainers don't want new drivers to use
> CLK_OF_DECLARE and instead use platform devices. I have just written such
> a new driver that is shared by 6 different SoCs. For some combinations of
> SoCs and clocks, using a platform device is fine but on others we need to
> register early, so the drivers now have to handle both cases, which is
> kind of messy and fragile. If there is a generic way to register platform
> devices early, then the code is simplified because we only have to handle
> one method of registering the clocks instead of two.
Can you get them to explain why? This sounds wrong.
Rich
On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
> On 04/26/2018 12:31 PM, Rich Felker wrote:
>>
>> On Thu, Apr 26, 2018 at 05:29:18PM +0200, Bartosz Golaszewski wrote:
>>>
>>> From: Bartosz Golaszewski <[email protected]>
>>>
>>> This is a follow to my series[1] the aim of which was to introduce device
>>> tree
>>> support for early platform devices.
>>>
>>> It was received rather negatively. Aside from using device tree to pass
>>> implementation specific details to the system, two important concerns
>>> were
>>> raised: no probe deferral support and the fact that currently the early
>>> devices
>>> never get converted to actual platform drivers. This series is a
>>> proof-of-concept that's trying to address those issues.
>>>
>>> The only user of the current version of early platform drivers is the
>>> SuperH
>>> architecture. If this series eventually gets merged, we could simply
>>> replace
>>> the other solution.
>>
>>
>> Looking at a quick output of:
>>
>> grep -r -A10 early_devices[[] arch/sh/kernel/
>>
>> it looks like all of the existing early platform devices are serial
>> ports, clocks, and clocksources. The switch to device tree should pick
>> them all up from CLK_OF_DECLARE, TIMER_OF_DECLARE, and
>> EARLYCON_DECLARE. Until that's complete, the existing code works
>> as-is. I don't see what problem you're trying to solve.
>
>
> The problem for us is that clk maintainers don't want new drivers to use
> CLK_OF_DECLARE and instead use platform devices. I have just written such
> a new driver that is shared by 6 different SoCs. For some combinations of
> SoCs and clocks, using a platform device is fine but on others we need to
> register early, so the drivers now have to handle both cases, which is
> kind of messy and fragile. If there is a generic way to register platform
> devices early, then the code is simplified because we only have to handle
> one method of registering the clocks instead of two.
The early_platform code is certainly not a way to make things simpler,
it just adds one more way of doing the same thing that OF_CLK_DECLARE
already does. We removed the last early_platform users on ARM a few
years ago, and I would hope to leave it like that.
I haven't seen the discussion about your clock drivers, but I know that
usually only a very small subset of the clocks on an SoC are needed
that 'early', and you should use a regular platform driver for the rest.
Can you elaborate on which devices need to access your clocks before
you are able to initialize the clk driver through the regular platform_driver
framework? Do any of these need complex interactions with the clk
subsystem, or do you just need to ensure they are turned on?
Arnd
Hi Arnd,
On Friday 27 April 2018 01:22 PM, Arnd Bergmann wrote:
> On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
>> On 04/26/2018 12:31 PM, Rich Felker wrote:
>>>
>>> On Thu, Apr 26, 2018 at 05:29:18PM +0200, Bartosz Golaszewski wrote:
>>>>
>>>> From: Bartosz Golaszewski <[email protected]>
>>>>
>>>> This is a follow to my series[1] the aim of which was to introduce device
>>>> tree
>>>> support for early platform devices.
>>>>
>>>> It was received rather negatively. Aside from using device tree to pass
>>>> implementation specific details to the system, two important concerns
>>>> were
>>>> raised: no probe deferral support and the fact that currently the early
>>>> devices
>>>> never get converted to actual platform drivers. This series is a
>>>> proof-of-concept that's trying to address those issues.
>>>>
>>>> The only user of the current version of early platform drivers is the
>>>> SuperH
>>>> architecture. If this series eventually gets merged, we could simply
>>>> replace
>>>> the other solution.
>>>
>>>
>>> Looking at a quick output of:
>>>
>>> grep -r -A10 early_devices[[] arch/sh/kernel/
>>>
>>> it looks like all of the existing early platform devices are serial
>>> ports, clocks, and clocksources. The switch to device tree should pick
>>> them all up from CLK_OF_DECLARE, TIMER_OF_DECLARE, and
>>> EARLYCON_DECLARE. Until that's complete, the existing code works
>>> as-is. I don't see what problem you're trying to solve.
>>
>>
>> The problem for us is that clk maintainers don't want new drivers to use
>> CLK_OF_DECLARE and instead use platform devices. I have just written such
>> a new driver that is shared by 6 different SoCs. For some combinations of
>> SoCs and clocks, using a platform device is fine but on others we need to
>> register early, so the drivers now have to handle both cases, which is
>> kind of messy and fragile. If there is a generic way to register platform
>> devices early, then the code is simplified because we only have to handle
>> one method of registering the clocks instead of two.
>
> The early_platform code is certainly not a way to make things simpler,
> it just adds one more way of doing the same thing that OF_CLK_DECLARE
> already does. We removed the last early_platform users on ARM a few
> years ago, and I would hope to leave it like that.
>
> I haven't seen the discussion about your clock drivers, but I know that
> usually only a very small subset of the clocks on an SoC are needed
> that 'early', and you should use a regular platform driver for the rest.
Its true that the subset is small, but they are either PLL bypass clocks
or clocks derived out of the main clock gate controller on the Soc
(DaVinci PSC). So we need some non-platform-device based initialization
support in the two main clock drivers used on mach-davinci anyway.
> Can you elaborate on which devices need to access your clocks before
> you are able to initialize the clk driver through the regular platform_driver
> framework? Do any of these need complex interactions with the clk
> subsystem, or do you just need to ensure they are turned on?
Its just the timer IP. There is no complex interaction, just need to
ensure that the clock is registered with the framework and also switched
on when there is a gate clock.
The latest attempt by David for this was posted last night here:
https://lkml.org/lkml/2018/4/26/1093
Thanks,
Sekhar
On Fri, Apr 27, 2018 at 10:29 AM, Sekhar Nori <[email protected]> wrote:
> On Friday 27 April 2018 01:22 PM, Arnd Bergmann wrote:
>> On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
>>> On 04/26/2018 12:31 PM, Rich Felker wrote:
>>
>> I haven't seen the discussion about your clock drivers, but I know that
>> usually only a very small subset of the clocks on an SoC are needed
>> that 'early', and you should use a regular platform driver for the rest.
>
> Its true that the subset is small, but they are either PLL bypass clocks
> or clocks derived out of the main clock gate controller on the Soc
> (DaVinci PSC). So we need some non-platform-device based initialization
> support in the two main clock drivers used on mach-davinci anyway.
>
>> Can you elaborate on which devices need to access your clocks before
>> you are able to initialize the clk driver through the regular platform_driver
>> framework? Do any of these need complex interactions with the clk
>> subsystem, or do you just need to ensure they are turned on?
>
> Its just the timer IP. There is no complex interaction, just need to
> ensure that the clock is registered with the framework and also switched
> on when there is a gate clock.
>
> The latest attempt by David for this was posted last night here:
>
> https://lkml.org/lkml/2018/4/26/1093
Ok, so the workaround in that series is to set up the timer clk manually
in the SoC specific code (dm365_init_time etc) and register it to the
clk subsystem, right?
That seems to be a much more appropriate workaround than adding
back early_platform devices. We can always try to do something better
later, but for now I'm happy with that as a workaround.
Please clarify: do we have to set up the clk registers for the timer
here just because we can't rely on the bootloader to have it set up
initially, or is there some other reason? I think what some other
platforms do is to treat the timer clock as a fixed-rate clock that
is not managed by the actual clk driver but is set up by uboot
before we enter the kernel, and then the clk driver just makes sure
it doesn't turn that clk off later.
Arnd
2018-04-27 9:52 GMT+02:00 Arnd Bergmann <[email protected]>:
> On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
>> On 04/26/2018 12:31 PM, Rich Felker wrote:
>>>
>>> On Thu, Apr 26, 2018 at 05:29:18PM +0200, Bartosz Golaszewski wrote:
>>>>
>>>> From: Bartosz Golaszewski <[email protected]>
>>>>
>>>> This is a follow to my series[1] the aim of which was to introduce device
>>>> tree
>>>> support for early platform devices.
>>>>
>>>> It was received rather negatively. Aside from using device tree to pass
>>>> implementation specific details to the system, two important concerns
>>>> were
>>>> raised: no probe deferral support and the fact that currently the early
>>>> devices
>>>> never get converted to actual platform drivers. This series is a
>>>> proof-of-concept that's trying to address those issues.
>>>>
>>>> The only user of the current version of early platform drivers is the
>>>> SuperH
>>>> architecture. If this series eventually gets merged, we could simply
>>>> replace
>>>> the other solution.
>>>
>>>
>>> Looking at a quick output of:
>>>
>>> grep -r -A10 early_devices[[] arch/sh/kernel/
>>>
>>> it looks like all of the existing early platform devices are serial
>>> ports, clocks, and clocksources. The switch to device tree should pick
>>> them all up from CLK_OF_DECLARE, TIMER_OF_DECLARE, and
>>> EARLYCON_DECLARE. Until that's complete, the existing code works
>>> as-is. I don't see what problem you're trying to solve.
>>
>>
>> The problem for us is that clk maintainers don't want new drivers to use
>> CLK_OF_DECLARE and instead use platform devices. I have just written such
>> a new driver that is shared by 6 different SoCs. For some combinations of
>> SoCs and clocks, using a platform device is fine but on others we need to
>> register early, so the drivers now have to handle both cases, which is
>> kind of messy and fragile. If there is a generic way to register platform
>> devices early, then the code is simplified because we only have to handle
>> one method of registering the clocks instead of two.
>
> The early_platform code is certainly not a way to make things simpler,
> it just adds one more way of doing the same thing that OF_CLK_DECLARE
> already does. We removed the last early_platform users on ARM a few
> years ago, and I would hope to leave it like that.
>
> I haven't seen the discussion about your clock drivers, but I know that
> usually only a very small subset of the clocks on an SoC are needed
> that 'early', and you should use a regular platform driver for the rest.
>
> Can you elaborate on which devices need to access your clocks before
> you are able to initialize the clk driver through the regular platform_driver
> framework? Do any of these need complex interactions with the clk
> subsystem, or do you just need to ensure they are turned on?
>
> Arnd
The problem I'm trying to solve is the following:
We have platforms out there which still use both board files and
device tree. They are still comercially supported and are not going
anywhere anytime soon. Some of these platforms are being actively
maintained and cleaned-up. An example is the DaVinci platform: David
has recently converted all the SoCs and boards to using the common
clock framework. I'm cleaning up some other parts too.
The problem with the legacy board code is that a lot of things that
should be platform drivers ended up in arch/arm/mach-*. We're now
slowly moving this code to drivers/ but some initialization code
(timers, critical clocks, irqs) needs to be called early in the boot
sequence.
When you're saying that we already have all the OF_DECLARE macros, it
seems to me that you're forgetting that we also want to keep
supporting the board files. So without the early platform drivers we
need to use a mix of OF_DECLARE and handcrafted initialization in
arch/arm/mach-* since we can't call platform_device_register() that
early. This blocks us from completely moving the should-be-driver code
to drivers/, because these drivers *need* to support both cases.
The main problem with OF_DECLARE is that although we have
corresponding device nodes, we never actually register any real linux
devices. If we add to this the fact that current early platform
drivers implementation is broken (for reasons I mentioned in the cover
letter) the support gets really messy, since we can have up to three
entry points to the driver's code. Other issues come to mind as well:
if we're using OF_DECLARE we can't benefit from devm* routines.
My aim is to provide a clean, robust and generic way of probing
certain devices early and then converting them to actual platform
devices when we're advanced enough into the boot sequence. If we
merged such a framework, we could work towards removing both the
previous early platform devices (in favor of the new mechanism) and
maybe even deprecating and replacing OF_DECLARE(), since we could
simply early probe the DT drivers. Personally I see OF_DECLARE as a
bigger hack than early devices.
My patch tries to address exactly the use cases we're facing - for
example by providing means to probe devices twice (early and late) and
to check the state we're at in order for users to be able to just do
the critical initialization early on and then continue with regular
stuff later.
Best regards,
Bartosz Golaszewski
2018-04-27 10:55 GMT+02:00 Arnd Bergmann <[email protected]>:
> On Fri, Apr 27, 2018 at 10:29 AM, Sekhar Nori <[email protected]> wrote:
>> On Friday 27 April 2018 01:22 PM, Arnd Bergmann wrote:
>>> On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
>>>> On 04/26/2018 12:31 PM, Rich Felker wrote:
>
>>>
>>> I haven't seen the discussion about your clock drivers, but I know that
>>> usually only a very small subset of the clocks on an SoC are needed
>>> that 'early', and you should use a regular platform driver for the rest.
>>
>> Its true that the subset is small, but they are either PLL bypass clocks
>> or clocks derived out of the main clock gate controller on the Soc
>> (DaVinci PSC). So we need some non-platform-device based initialization
>> support in the two main clock drivers used on mach-davinci anyway.
>>
>>> Can you elaborate on which devices need to access your clocks before
>>> you are able to initialize the clk driver through the regular platform_driver
>>> framework? Do any of these need complex interactions with the clk
>>> subsystem, or do you just need to ensure they are turned on?
>>
>> Its just the timer IP. There is no complex interaction, just need to
>> ensure that the clock is registered with the framework and also switched
>> on when there is a gate clock.
>>
>> The latest attempt by David for this was posted last night here:
>>
>> https://lkml.org/lkml/2018/4/26/1093
>
> Ok, so the workaround in that series is to set up the timer clk manually
> in the SoC specific code (dm365_init_time etc) and register it to the
> clk subsystem, right?
>
> That seems to be a much more appropriate workaround than adding
> back early_platform devices. We can always try to do something better
> later, but for now I'm happy with that as a workaround.
>
Just to clarify: this is not bringing back the hacky early platform
devices from before. It's a new approach that actually uses the linux
platform driver model (unlike the current mechanism which only uses
the same data structures).
Thanks,
Bart
On Fri, Apr 27, 2018 at 10:57 AM, Bartosz Golaszewski
<[email protected]> wrote:
> 2018-04-27 9:52 GMT+02:00 Arnd Bergmann <[email protected]>:
>> On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
>>> On 04/26/2018 12:31 PM, Rich Felker wrote:
>
> We have platforms out there which still use both board files and
> device tree. They are still comercially supported and are not going
> anywhere anytime soon. Some of these platforms are being actively
> maintained and cleaned-up. An example is the DaVinci platform: David
> has recently converted all the SoCs and boards to using the common
> clock framework. I'm cleaning up some other parts too.
>
> The problem with the legacy board code is that a lot of things that
> should be platform drivers ended up in arch/arm/mach-*. We're now
> slowly moving this code to drivers/ but some initialization code
> (timers, critical clocks, irqs) needs to be called early in the boot
> sequence.
Right, and that's very good work.
> When you're saying that we already have all the OF_DECLARE macros, it
> seems to me that you're forgetting that we also want to keep
> supporting the board files. So without the early platform drivers we
> need to use a mix of OF_DECLARE and handcrafted initialization in
> arch/arm/mach-* since we can't call platform_device_register() that
> early. This blocks us from completely moving the should-be-driver code
> to drivers/, because these drivers *need* to support both cases.
The OF_DECLARE_* functions were initially added as a way to
remove board files that just consist of callbacks into early
initialization for some subsystems. As long as you still have
board files and you are looking for a way to reuse code between
OF_DECLARE_* functions and board files, why not just leave
those functions globally visible and call them from the non-DT
board files?
> The main problem with OF_DECLARE is that although we have
> corresponding device nodes, we never actually register any real linux
> devices. If we add to this the fact that current early platform
> drivers implementation is broken (for reasons I mentioned in the cover
> letter) the support gets really messy, since we can have up to three
> entry points to the driver's code. Other issues come to mind as well:
> if we're using OF_DECLARE we can't benefit from devm* routines.
Right, the devm_* problem has come up before.
> My aim is to provide a clean, robust and generic way of probing
> certain devices early and then converting them to actual platform
> devices when we're advanced enough into the boot sequence. If we
> merged such a framework, we could work towards removing both the
> previous early platform devices (in favor of the new mechanism) and
> maybe even deprecating and replacing OF_DECLARE(), since we could
> simply early probe the DT drivers. Personally I see OF_DECLARE as a
> bigger hack than early devices.
>
> My patch tries to address exactly the use cases we're facing - for
> example by providing means to probe devices twice (early and late) and
> to check the state we're at in order for users to be able to just do
> the critical initialization early on and then continue with regular
> stuff later.
Maybe the problem is reusing the name and some of the code from
an existing functionality that we've been trying to get rid of.
If what you want to do is completely different from the existing
early_platform implementation, how about starting by moving that
out of drivers/base/platform.c into something under arch/sh/
and renaming it to something with an sh_ prefix.
Let's just leave the non-DT part out of it by making it sh specific.
Then we can come up with improvements to the current
platform_device handling for DT based platforms that you can
use on DT-based davinci to replace what currently happens on
board-file based davinci systems, without mixing up those
two code paths too much in the base driver support.
Arnd
2018-04-27 12:18 GMT+02:00 Arnd Bergmann <[email protected]>:
> On Fri, Apr 27, 2018 at 10:57 AM, Bartosz Golaszewski
> <[email protected]> wrote:
>> 2018-04-27 9:52 GMT+02:00 Arnd Bergmann <[email protected]>:
>>> On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
>>>> On 04/26/2018 12:31 PM, Rich Felker wrote:
>>
>> We have platforms out there which still use both board files and
>> device tree. They are still comercially supported and are not going
>> anywhere anytime soon. Some of these platforms are being actively
>> maintained and cleaned-up. An example is the DaVinci platform: David
>> has recently converted all the SoCs and boards to using the common
>> clock framework. I'm cleaning up some other parts too.
>>
>> The problem with the legacy board code is that a lot of things that
>> should be platform drivers ended up in arch/arm/mach-*. We're now
>> slowly moving this code to drivers/ but some initialization code
>> (timers, critical clocks, irqs) needs to be called early in the boot
>> sequence.
>
> Right, and that's very good work.
>
>> When you're saying that we already have all the OF_DECLARE macros, it
>> seems to me that you're forgetting that we also want to keep
>> supporting the board files. So without the early platform drivers we
>> need to use a mix of OF_DECLARE and handcrafted initialization in
>> arch/arm/mach-* since we can't call platform_device_register() that
>> early. This blocks us from completely moving the should-be-driver code
>> to drivers/, because these drivers *need* to support both cases.
>
> The OF_DECLARE_* functions were initially added as a way to
> remove board files that just consist of callbacks into early
> initialization for some subsystems. As long as you still have
> board files and you are looking for a way to reuse code between
> OF_DECLARE_* functions and board files, why not just leave
> those functions globally visible and call them from the non-DT
> board files?
>
>> The main problem with OF_DECLARE is that although we have
>> corresponding device nodes, we never actually register any real linux
>> devices. If we add to this the fact that current early platform
>> drivers implementation is broken (for reasons I mentioned in the cover
>> letter) the support gets really messy, since we can have up to three
>> entry points to the driver's code. Other issues come to mind as well:
>> if we're using OF_DECLARE we can't benefit from devm* routines.
>
> Right, the devm_* problem has come up before.
>
>> My aim is to provide a clean, robust and generic way of probing
>> certain devices early and then converting them to actual platform
>> devices when we're advanced enough into the boot sequence. If we
>> merged such a framework, we could work towards removing both the
>> previous early platform devices (in favor of the new mechanism) and
>> maybe even deprecating and replacing OF_DECLARE(), since we could
>> simply early probe the DT drivers. Personally I see OF_DECLARE as a
>> bigger hack than early devices.
>>
>> My patch tries to address exactly the use cases we're facing - for
>> example by providing means to probe devices twice (early and late) and
>> to check the state we're at in order for users to be able to just do
>> the critical initialization early on and then continue with regular
>> stuff later.
>
> Maybe the problem is reusing the name and some of the code from
> an existing functionality that we've been trying to get rid of.
>
I'm not reusing the name - in fact I set the prefix to earlydev_
exactly in order to not confuse anyone. I'm also not reusing any code
in the second series.
> If what you want to do is completely different from the existing
> early_platform implementation, how about starting by moving that
> out of drivers/base/platform.c into something under arch/sh/
> and renaming it to something with an sh_ prefix.
>
Yes, this is a good idea, but what about the sh-specific drivers that
rely on it? Is including headers from arch/ in driver code still an
accepted practice?
> Let's just leave the non-DT part out of it by making it sh specific.
> Then we can come up with improvements to the current
> platform_device handling for DT based platforms that you can
> use on DT-based davinci to replace what currently happens on
> board-file based davinci systems, without mixing up those
> two code paths too much in the base driver support.
>
I don't see why we wouldn't want to unify these two cases. The best
solution to me seems having only one entry point into the driver code
- the probe() function stored in platform_driver - whether we're
probing it from DT, platform data, ACPI or early boot code.
Best regards,
Bartosz Golaszewski
On Fri, Apr 27, 2018 at 1:53 PM, Bartosz Golaszewski <[email protected]> wrote:
> 2018-04-27 12:18 GMT+02:00 Arnd Bergmann <[email protected]>:
>> On Fri, Apr 27, 2018 at 10:57 AM, Bartosz Golaszewski
>> <[email protected]> wrote:
>>> 2018-04-27 9:52 GMT+02:00 Arnd Bergmann <[email protected]>:
>>>> On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
>>> My patch tries to address exactly the use cases we're facing - for
>>> example by providing means to probe devices twice (early and late) and
>>> to check the state we're at in order for users to be able to just do
>>> the critical initialization early on and then continue with regular
>>> stuff later.
>>
>> Maybe the problem is reusing the name and some of the code from
>> an existing functionality that we've been trying to get rid of.
>>
>
> I'm not reusing the name - in fact I set the prefix to earlydev_
> exactly in order to not confuse anyone. I'm also not reusing any code
> in the second series.
Ok.
>> If what you want to do is completely different from the existing
>> early_platform implementation, how about starting by moving that
>> out of drivers/base/platform.c into something under arch/sh/
>> and renaming it to something with an sh_ prefix.
>>
>
> Yes, this is a good idea, but what about the sh-specific drivers that
> rely on it? Is including headers from arch/ in driver code still an
> accepted practice?
I think it's fine here, since we're just move it out of the way and
there are only very few drivers using it:
drivers/clocksource/sh_cmt.c:early_platform_init("earlytimer",
&sh_cmt_device_driver);
drivers/clocksource/sh_mtu2.c:early_platform_init("earlytimer",
&sh_mtu2_device_driver);
drivers/clocksource/sh_tmu.c:early_platform_init("earlytimer",
&sh_tmu_device_driver);
drivers/clocksource/timer-ti-dm.c:early_platform_init("earlytimer",
&omap_dm_timer_driver);
drivers/tty/serial/sh-sci.c:early_platform_init_buffer("earlyprintk",
&sci_driver,
For timer-ti-dm, it seems like a leftover from old times that can
be removed. The other four are shared between arch/sh and
arch/arm/mach-shmobile and already have some #ifdef
to handle those two cases.
>> Let's just leave the non-DT part out of it by making it sh specific.
>> Then we can come up with improvements to the current
>> platform_device handling for DT based platforms that you can
>> use on DT-based davinci to replace what currently happens on
>> board-file based davinci systems, without mixing up those
>> two code paths too much in the base driver support.
>
> I don't see why we wouldn't want to unify these two cases. The best
> solution to me seems having only one entry point into the driver code
> - the probe() function stored in platform_driver - whether we're
> probing it from DT, platform data, ACPI or early boot code.
I'd rather keep those separate and would prefer not to have
that many different ways of getting there instead. DT and
board files can already share most of the code through the
use of platform_device, especially when you start using
device properties instead of platform_data, and the other
two are rare corner cases and ideally left that way.
The early boot code is always special and instead of making
it easier to use, we should focus on using it as little as
possible: every line of code that we call before even
initializing timers and consoles means it gets harder to
debug when something goes wrong.
Arnd
On Fri, Apr 27, 2018 at 4:05 PM, Bartosz Golaszewski
<[email protected]> wrote:
> 2018-04-27 14:40 GMT+02:00 Arnd Bergmann <[email protected]>:
>> For timer-ti-dm, it seems like a leftover from old times that can
>> be removed. The other four are shared between arch/sh and
>> arch/arm/mach-shmobile and already have some #ifdef
>> to handle those two cases.
>>
>
> I'm also seeing that we also call early_platform_cleanup() from
> platform_bus_init(). Any ideas for this one?
My first idea would be to call it immediately after registering all
devices and drivers. It looks like it's only needed to make all
devm_ allocations persistent by removing them from the list,
so we have to call early_platform_cleanup() before getting
to the real platform code, but it could be done much earlier
if we want to, at least after both setup_arch() and sh_late_time_init()
are complete.
>> I'd rather keep those separate and would prefer not to have
>> that many different ways of getting there instead. DT and
>> board files can already share most of the code through the
>> use of platform_device, especially when you start using
>> device properties instead of platform_data, and the other
>> two are rare corner cases and ideally left that way.
>>
>> The early boot code is always special and instead of making
>> it easier to use, we should focus on using it as little as
>> possible: every line of code that we call before even
>> initializing timers and consoles means it gets harder to
>> debug when something goes wrong.
>>
>
> I'm afraid I don't quite understand your reasoning. I fully agree that
> devices that need to be initialized that early are a rare corner case.
> We should limit any such uses to the absolute minimum. But when we do
> need to go this way, we should do it right. Having a unified mechanism
> for early devices will allow maintainers to enforce good practices
> (using resources for register mapping, devres, reusing driver code for
> reading/writing to registers). Having initialization code in machine
> code will make everybody use different APIs and duplicate solutions. I
> normally assume that code consolidation is always good.
>
> If we add a way for DT-based platform devices to be probed early - it
> would be based on platform device/driver structures anyway. Why would
> we even want to not convert the board code into a simple call to
> early_platform_device_register() if we'll already offer this API for
> device tree?
I think we first need to define what we really want to achieve here.
It sounds like you still want to recreate a lot of what early_platform
devices do, but it seems more important to me to add the missing
functionality to the OF_DECLARE infrastructure. The most
important pieces that seem to be missing are solved by finding
a way to provide a platform_device pointer with the following
properties:
- allow being passed into dev_print()
- allow using the pointer as a token for devres unwinding
- access to device_private data that remains persistent
until real a platform_driver gets loaded
That can probably be done as an extension to the current
infrastructure.
However, I'd be very cautious about the resource portion:
filling the platform resources (registers, irqs, ...) the way
we do for regular devices is much harder and can introduce
additional (or circular) dependencies on other devices.
OTOH, not using those resources means you have a hard
time passing information from board files.
Arnd
2018-04-27 14:40 GMT+02:00 Arnd Bergmann <[email protected]>:
> On Fri, Apr 27, 2018 at 1:53 PM, Bartosz Golaszewski <[email protected]> wrote:
>> 2018-04-27 12:18 GMT+02:00 Arnd Bergmann <[email protected]>:
>>> On Fri, Apr 27, 2018 at 10:57 AM, Bartosz Golaszewski
>>> <[email protected]> wrote:
>>>> 2018-04-27 9:52 GMT+02:00 Arnd Bergmann <[email protected]>:
>>>>> On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
>>>> My patch tries to address exactly the use cases we're facing - for
>>>> example by providing means to probe devices twice (early and late) and
>>>> to check the state we're at in order for users to be able to just do
>>>> the critical initialization early on and then continue with regular
>>>> stuff later.
>>>
>>> Maybe the problem is reusing the name and some of the code from
>>> an existing functionality that we've been trying to get rid of.
>>>
>>
>> I'm not reusing the name - in fact I set the prefix to earlydev_
>> exactly in order to not confuse anyone. I'm also not reusing any code
>> in the second series.
>
> Ok.
>
>>> If what you want to do is completely different from the existing
>>> early_platform implementation, how about starting by moving that
>>> out of drivers/base/platform.c into something under arch/sh/
>>> and renaming it to something with an sh_ prefix.
>>>
>>
>> Yes, this is a good idea, but what about the sh-specific drivers that
>> rely on it? Is including headers from arch/ in driver code still an
>> accepted practice?
>
> I think it's fine here, since we're just move it out of the way and
> there are only very few drivers using it:
>
> drivers/clocksource/sh_cmt.c:early_platform_init("earlytimer",
> &sh_cmt_device_driver);
> drivers/clocksource/sh_mtu2.c:early_platform_init("earlytimer",
> &sh_mtu2_device_driver);
> drivers/clocksource/sh_tmu.c:early_platform_init("earlytimer",
> &sh_tmu_device_driver);
> drivers/clocksource/timer-ti-dm.c:early_platform_init("earlytimer",
> &omap_dm_timer_driver);
> drivers/tty/serial/sh-sci.c:early_platform_init_buffer("earlyprintk",
> &sci_driver,
>
> For timer-ti-dm, it seems like a leftover from old times that can
> be removed. The other four are shared between arch/sh and
> arch/arm/mach-shmobile and already have some #ifdef
> to handle those two cases.
>
I'm also seeing that we also call early_platform_cleanup() from
platform_bus_init(). Any ideas for this one?
>>> Let's just leave the non-DT part out of it by making it sh specific.
>>> Then we can come up with improvements to the current
>>> platform_device handling for DT based platforms that you can
>>> use on DT-based davinci to replace what currently happens on
>>> board-file based davinci systems, without mixing up those
>>> two code paths too much in the base driver support.
>>
>> I don't see why we wouldn't want to unify these two cases. The best
>> solution to me seems having only one entry point into the driver code
>> - the probe() function stored in platform_driver - whether we're
>> probing it from DT, platform data, ACPI or early boot code.
>
> I'd rather keep those separate and would prefer not to have
> that many different ways of getting there instead. DT and
> board files can already share most of the code through the
> use of platform_device, especially when you start using
> device properties instead of platform_data, and the other
> two are rare corner cases and ideally left that way.
>
> The early boot code is always special and instead of making
> it easier to use, we should focus on using it as little as
> possible: every line of code that we call before even
> initializing timers and consoles means it gets harder to
> debug when something goes wrong.
>
I'm afraid I don't quite understand your reasoning. I fully agree that
devices that need to be initialized that early are a rare corner case.
We should limit any such uses to the absolute minimum. But when we do
need to go this way, we should do it right. Having a unified mechanism
for early devices will allow maintainers to enforce good practices
(using resources for register mapping, devres, reusing driver code for
reading/writing to registers). Having initialization code in machine
code will make everybody use different APIs and duplicate solutions. I
normally assume that code consolidation is always good.
If we add a way for DT-based platform devices to be probed early - it
would be based on platform device/driver structures anyway. Why would
we even want to not convert the board code into a simple call to
early_platform_device_register() if we'll already offer this API for
device tree?
Best regards,
Bartosz Golaszewski
On Thu, Apr 26, 2018 at 5:29 PM, Bartosz Golaszewski <[email protected]> wrote:
> +/*
> + * REVISIT: early_initcall may be still too late for some timers and critical
> + * clocks. We should probably have a separate section with callbacks that can
> + * be invoked at each architecture's discretion.
> + */
> +#define earlydev_platform_driver(_drv) \
> + static int _drv##_register(void) \
> + { \
> + earlydev_driver_register(&(_drv)); \
> + return 0; \
> + } \
> + early_initcall(_drv##_register)
> +
> +#endif /* __EARLYDEV_H__ */
No full review, just one comment: this would really need to fit into the
existing callbacks for clk, timer, earlycon etc that we use with OF_DECLARE.
A lot of code makes assumptions about the order in which they are
called, and the current early_platform infrastructure does the same
thing using its "earlyprintk" and "earlytimer" classes.
Arnd
On Fri, Apr 27, 2018 at 02:40:34PM +0200, Arnd Bergmann wrote:
> On Fri, Apr 27, 2018 at 1:53 PM, Bartosz Golaszewski <[email protected]> wrote:
> > 2018-04-27 12:18 GMT+02:00 Arnd Bergmann <[email protected]>:
> >> On Fri, Apr 27, 2018 at 10:57 AM, Bartosz Golaszewski
> >> <[email protected]> wrote:
> >>> 2018-04-27 9:52 GMT+02:00 Arnd Bergmann <[email protected]>:
> >>>> On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
> >>> My patch tries to address exactly the use cases we're facing - for
> >>> example by providing means to probe devices twice (early and late) and
> >>> to check the state we're at in order for users to be able to just do
> >>> the critical initialization early on and then continue with regular
> >>> stuff later.
> >>
> >> Maybe the problem is reusing the name and some of the code from
> >> an existing functionality that we've been trying to get rid of.
> >>
> >
> > I'm not reusing the name - in fact I set the prefix to earlydev_
> > exactly in order to not confuse anyone. I'm also not reusing any code
> > in the second series.
>
> Ok.
>
> >> If what you want to do is completely different from the existing
> >> early_platform implementation, how about starting by moving that
> >> out of drivers/base/platform.c into something under arch/sh/
> >> and renaming it to something with an sh_ prefix.
> >>
> >
> > Yes, this is a good idea, but what about the sh-specific drivers that
> > rely on it? Is including headers from arch/ in driver code still an
> > accepted practice?
>
> I think it's fine here, since we're just move it out of the way and
> there are only very few drivers using it:
>
> drivers/clocksource/sh_cmt.c:early_platform_init("earlytimer",
> &sh_cmt_device_driver);
> drivers/clocksource/sh_mtu2.c:early_platform_init("earlytimer",
> &sh_mtu2_device_driver);
> drivers/clocksource/sh_tmu.c:early_platform_init("earlytimer",
> &sh_tmu_device_driver);
> drivers/clocksource/timer-ti-dm.c:early_platform_init("earlytimer",
> &omap_dm_timer_driver);
> drivers/tty/serial/sh-sci.c:early_platform_init_buffer("earlyprintk",
> &sci_driver,
>
> For timer-ti-dm, it seems like a leftover from old times that can
> be removed. The other four are shared between arch/sh and
> arch/arm/mach-shmobile and already have some #ifdef
> to handle those two cases.
FWIW if arch/sh is the only remaining user of the earlyprintk part,
please don't go to any trouble to preserve it. I want to move to
earlycon exclusively. Not sure if that is possible before moving the
boards that are using it to device tree; if so and it's easy, please
consider sending a patch or a sketch of what it should look like so I
can do it.
Rich
2018-04-27 16:48 GMT+02:00 Arnd Bergmann <[email protected]>:
> On Fri, Apr 27, 2018 at 4:05 PM, Bartosz Golaszewski
> <[email protected]> wrote:
>> 2018-04-27 14:40 GMT+02:00 Arnd Bergmann <[email protected]>:
>
>>> For timer-ti-dm, it seems like a leftover from old times that can
>>> be removed. The other four are shared between arch/sh and
>>> arch/arm/mach-shmobile and already have some #ifdef
>>> to handle those two cases.
>>>
>>
>> I'm also seeing that we also call early_platform_cleanup() from
>> platform_bus_init(). Any ideas for this one?
>
> My first idea would be to call it immediately after registering all
> devices and drivers. It looks like it's only needed to make all
> devm_ allocations persistent by removing them from the list,
> so we have to call early_platform_cleanup() before getting
> to the real platform code, but it could be done much earlier
> if we want to, at least after both setup_arch() and sh_late_time_init()
> are complete.
>
>>> I'd rather keep those separate and would prefer not to have
>>> that many different ways of getting there instead. DT and
>>> board files can already share most of the code through the
>>> use of platform_device, especially when you start using
>>> device properties instead of platform_data, and the other
>>> two are rare corner cases and ideally left that way.
>>>
>>> The early boot code is always special and instead of making
>>> it easier to use, we should focus on using it as little as
>>> possible: every line of code that we call before even
>>> initializing timers and consoles means it gets harder to
>>> debug when something goes wrong.
>>>
>>
>> I'm afraid I don't quite understand your reasoning. I fully agree that
>> devices that need to be initialized that early are a rare corner case.
>> We should limit any such uses to the absolute minimum. But when we do
>> need to go this way, we should do it right. Having a unified mechanism
>> for early devices will allow maintainers to enforce good practices
>> (using resources for register mapping, devres, reusing driver code for
>> reading/writing to registers). Having initialization code in machine
>> code will make everybody use different APIs and duplicate solutions. I
>> normally assume that code consolidation is always good.
>>
>> If we add a way for DT-based platform devices to be probed early - it
>> would be based on platform device/driver structures anyway. Why would
>> we even want to not convert the board code into a simple call to
>> early_platform_device_register() if we'll already offer this API for
>> device tree?
>
> I think we first need to define what we really want to achieve here.
> It sounds like you still want to recreate a lot of what early_platform
> devices do, but it seems more important to me to add the missing
> functionality to the OF_DECLARE infrastructure. The most
> important pieces that seem to be missing are solved by finding
> a way to provide a platform_device pointer with the following
> properties:
>
> - allow being passed into dev_print()
> - allow using the pointer as a token for devres unwinding
> - access to device_private data that remains persistent
> until real a platform_driver gets loaded
>
> That can probably be done as an extension to the current
> infrastructure.
>
> However, I'd be very cautious about the resource portion:
> filling the platform resources (registers, irqs, ...) the way
> we do for regular devices is much harder and can introduce
> additional (or circular) dependencies on other devices.
> OTOH, not using those resources means you have a hard
> time passing information from board files.
>
> Arnd
So speaking in pseudo-C we basically have two ways for an imaginary
future timer driver:
int foo_probe(struct platform_device *pdev)
{
struct clk *clk;
if (probing_early(pdev)) {
clk = devm_clk_get(dev, "earlyclock");
/* Do early stuff. */
return 0;
}
/* Do late stuff. */
return 0;
}
--- vs ---
int foo_probe(struct platform_device *pdev)
{
/* Do late stuff. */
return 0;
}
static int foo_init(struct device_node *np)
{
struct clk *clk;
struct device *dev = device_from_device_node(np);
/* Do early stuff. */
clk = devm_clk_get(dev, "earlyclock");
return 0;
}
TIMER_OF_DECLARE(foo, "bar,foo", foo_init);
I still believe the first approach is easier to implement and has the
added benefit of supporting board files.
I'll give it a thought and will be back at it next week.
Best regards,
Bartosz Golaszewski
On Fri, Apr 27, 2018 at 6:05 PM, Bartosz Golaszewski
<[email protected]> wrote:
> 2018-04-27 16:48 GMT+02:00 Arnd Bergmann <[email protected]>:
>> On Fri, Apr 27, 2018 at 4:05 PM, Bartosz Golaszewski
>
> So speaking in pseudo-C we basically have two ways for an imaginary
> future timer driver:
>
> int foo_probe(struct platform_device *pdev)
> {
> struct clk *clk;
>
> if (probing_early(pdev)) {
> clk = devm_clk_get(dev, "earlyclock");
>
> /* Do early stuff. */
> return 0;
> }
>
> /* Do late stuff. */
>
> return 0;
> }
>
> --- vs ---
>
> int foo_probe(struct platform_device *pdev)
> {
> /* Do late stuff. */
>
> return 0;
> }
>
> static int foo_init(struct device_node *np)
> {
> struct clk *clk;
> struct device *dev = device_from_device_node(np);
>
> /* Do early stuff. */
> clk = devm_clk_get(dev, "earlyclock");
>
> return 0;
> }
>
> TIMER_OF_DECLARE(foo, "bar,foo", foo_init);
>
> I still believe the first approach is easier to implement and has the
> added benefit of supporting board files.
Right. I still like the second approach better, since it avoids
multiplexing two very different code paths into a single
function, and because it's closer to what everyone is used
to at the moment.
Prototyping both is probably helpful to get a better idea
of the actual complexity this introduces.
> I'll give it a thought and will be back at it next week.
Ok. I'll be on vacation for three weeks so I wont' be able
to reply on the new patches.
Arnd
On Fri, Apr 27, 2018 at 2:40 PM, Arnd Bergmann <[email protected]> wrote:
> On Fri, Apr 27, 2018 at 1:53 PM, Bartosz Golaszewski <[email protected]> wrote:
>> 2018-04-27 12:18 GMT+02:00 Arnd Bergmann <[email protected]>:
>>> On Fri, Apr 27, 2018 at 10:57 AM, Bartosz Golaszewski
>>> <[email protected]> wrote:
>>>> 2018-04-27 9:52 GMT+02:00 Arnd Bergmann <[email protected]>:
>>>>> On Fri, Apr 27, 2018 at 4:28 AM, David Lechner <[email protected]> wrote:
>>>> My patch tries to address exactly the use cases we're facing - for
>>>> example by providing means to probe devices twice (early and late) and
>>>> to check the state we're at in order for users to be able to just do
>>>> the critical initialization early on and then continue with regular
>>>> stuff later.
>>>
>>> Maybe the problem is reusing the name and some of the code from
>>> an existing functionality that we've been trying to get rid of.
>>>
>>
>> I'm not reusing the name - in fact I set the prefix to earlydev_
>> exactly in order to not confuse anyone. I'm also not reusing any code
>> in the second series.
>
> Ok.
>
>>> If what you want to do is completely different from the existing
>>> early_platform implementation, how about starting by moving that
>>> out of drivers/base/platform.c into something under arch/sh/
>>> and renaming it to something with an sh_ prefix.
>>>
>>
>> Yes, this is a good idea, but what about the sh-specific drivers that
>> rely on it? Is including headers from arch/ in driver code still an
>> accepted practice?
>
> I think it's fine here, since we're just move it out of the way and
> there are only very few drivers using it:
>
> drivers/clocksource/sh_cmt.c:early_platform_init("earlytimer",
> &sh_cmt_device_driver);
> drivers/clocksource/sh_mtu2.c:early_platform_init("earlytimer",
> &sh_mtu2_device_driver);
> drivers/clocksource/sh_tmu.c:early_platform_init("earlytimer",
> &sh_tmu_device_driver);
> drivers/clocksource/timer-ti-dm.c:early_platform_init("earlytimer",
> &omap_dm_timer_driver);
> drivers/tty/serial/sh-sci.c:early_platform_init_buffer("earlyprintk",
> &sci_driver,
>
> For timer-ti-dm, it seems like a leftover from old times that can
> be removed. The other four are shared between arch/sh and
> arch/arm/mach-shmobile and already have some #ifdef
> to handle those two cases.
Sh-sci is also used on arm64, for R-Car Gen3 SoCs.
While the latter has CMT and TMU hardware blocks, so far we haven't enabled
support for it yet (just using the ARM arch timer is too convenient? ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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