* [PATCH v8 7/7] of/platform: Don't create device links for default busses
From: Saravana Kannan @ 2019-07-29 22:11 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190729221101.228240-1-saravanak@google.com>
Default busses also have devices created for them. But there's no point
in creating device links for them. It's especially wasteful as it'll
cause the traversal of the entire device tree and also spend a lot of
time checking and figuring out that creating those links isn't allowed.
So check for default busses and skip trying to create device links for
them.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/of/platform.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 41499ddc8d95..676b2f730d1b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -682,6 +682,8 @@ static int of_link_to_suppliers(struct device *dev)
return 0;
if (unlikely(!dev->of_node))
return 0;
+ if (of_match_node(of_default_bus_match_table, dev->of_node))
+ return 0;
return __of_link_to_suppliers(dev, dev->of_node);
}
--
2.22.0.709.g102302147b-goog
^ permalink raw reply related
* [PATCH v8 6/7] of/platform: Create device links for all child-supplier depencencies
From: Saravana Kannan @ 2019-07-29 22:11 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190729221101.228240-1-saravanak@google.com>
A parent device can have child devices that it adds when it probes. But
this probing of the parent device can happen way after kernel init is done
-- for example, when the parent device's driver is loaded as a module.
In such cases, if the child devices depend on a supplier in the system, we
need to make sure the supplier gets the sync_state() callback only after
these child devices are added and probed.
To achieve this, when creating device links for a device by looking at its
DT node, don't just look at DT references at the top node level. Look at DT
references in all the descendant nodes too and create device links from the
ancestor device to all these supplier devices.
This way, when the parent device probes and adds child devices, the child
devices can then create their own device links to the suppliers and further
delay the supplier's sync_state() callback to after the child devices are
probed.
Example:
In this illustration, -> denotes DT references and indentation
represents child status.
Device node A
Device node B -> D
Device node C -> B, D
Device node D
Assume all these devices have their drivers loaded as modules.
Without this patch, this is the sequence of events:
1. D is added.
2. A is added.
3. Device D probes.
4. Device D gets its sync_state() callback.
5. Device B and C might malfunction because their resources got
altered/turned off before they can make active requests for them.
With this patch, this is the sequence of events:
1. D is added.
2. A is added and creates device links to D.
3. Device link from A to B is not added because A is a parent of B.
4. Device D probes.
5. Device D does not get it's sync_state() callback because consumer A
hasn't probed yet.
5. Device A probes.
5. a. Devices B and C are added.
5. b. Device links from B and C to D are added.
5. c. Device A's probe completes.
6. Device D does not get it's sync_state() callback because consumer A
has probed but consumers B and C haven't probed yet.
7. Device B and C probe.
8. Device D gets it's sync_state() callback because all its consumers
have probed.
9. None of the devices malfunction.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/of/platform.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 71d6138698ec..41499ddc8d95 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -655,24 +655,35 @@ static bool of_link_property(struct device *dev, struct device_node *con_np,
return done ? 0 : -ENODEV;
}
+static int __of_link_to_suppliers(struct device *dev,
+ struct device_node *con_np)
+{
+ struct device_node *child;
+ struct property *p;
+ bool done = true;
+
+ for_each_property_of_node(con_np, p)
+ if (of_link_property(dev, con_np, p->name))
+ done = false;
+
+ for_each_child_of_node(con_np, child)
+ if (__of_link_to_suppliers(dev, child))
+ done = false;
+
+ return done ? 0 : -ENODEV;
+}
+
static bool of_devlink;
core_param(of_devlink, of_devlink, bool, 0);
static int of_link_to_suppliers(struct device *dev)
{
- struct property *p;
- bool done = true;
-
if (!of_devlink)
return 0;
if (unlikely(!dev->of_node))
return 0;
- for_each_property_of_node(dev->of_node, p)
- if (of_link_property(dev, dev->of_node, p->name))
- done = false;
-
- return done ? 0 : -ENODEV;
+ return __of_link_to_suppliers(dev, dev->of_node);
}
#ifndef CONFIG_PPC
--
2.22.0.709.g102302147b-goog
^ permalink raw reply related
* [PATCH v8 5/7] of/platform: Pause/resume sync state during init and of_platform_populate()
From: Saravana Kannan @ 2019-07-29 22:10 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190729221101.228240-1-saravanak@google.com>
When all the top level devices are populated from DT during kernel
init, the supplier devices could be added and probed before the
consumer devices are added and linked to the suppliers. To avoid the
sync_state() callback from being called prematurely, pause the
sync_state() callbacks before populating the devices and resume them
at late_initcall_sync().
Similarly, when children devices are populated after kernel init using
of_platform_populate(), there could be supplier-consumer dependencies
between the children devices that are populated. To avoid the same
problem with sync_state() being called prematurely, pause and resume
sync_state() callbacks across of_platform_populate().
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/of/platform.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 4344419a26fc..71d6138698ec 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -485,6 +485,7 @@ int of_platform_populate(struct device_node *root,
pr_debug("%s()\n", __func__);
pr_debug(" starting at: %pOF\n", root);
+ device_links_supplier_sync_state_pause();
for_each_child_of_node(root, child) {
rc = of_platform_bus_create(child, matches, lookup, parent, true);
if (rc) {
@@ -492,6 +493,8 @@ int of_platform_populate(struct device_node *root,
break;
}
}
+ device_links_supplier_sync_state_resume();
+
of_node_set_flag(root, OF_POPULATED_BUS);
of_node_put(root);
@@ -688,6 +691,7 @@ static int __init of_platform_default_populate_init(void)
return -ENODEV;
platform_bus_type.add_links = of_link_to_suppliers;
+ device_links_supplier_sync_state_pause();
/*
* Handle certain compatibles explicitly, since we don't want to create
* platform_devices for every node in /reserved-memory with a
@@ -708,6 +712,13 @@ static int __init of_platform_default_populate_init(void)
return 0;
}
arch_initcall_sync(of_platform_default_populate_init);
+
+static int __init of_platform_sync_state_init(void)
+{
+ device_links_supplier_sync_state_resume();
+ return 0;
+}
+late_initcall_sync(of_platform_sync_state_init);
#endif
int of_platform_device_destroy(struct device *dev, void *data)
--
2.22.0.709.g102302147b-goog
^ permalink raw reply related
* [PATCH v8 4/7] driver core: Add sync_state driver/bus callback
From: Saravana Kannan @ 2019-07-29 22:10 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190729221101.228240-1-saravanak@google.com>
This sync_state driver/bus callback is called once all the consumers
of a supplier have probed successfully.
This allows the supplier device's driver/bus to sync the supplier
device's state to the software state with the guarantee that all the
consumers are actively managing the resources provided by the supplier
device.
To maintain backwards compatibility and ease transition from existing
frameworks and resource cleanup schemes, late_initcall_sync is the
earliest when the sync_state callback might be called.
There is no upper bound on the time by which the sync_state callback
has to be called. This is because if a consumer device never probes,
the supplier has to maintain its resources in the state left by the
bootloader. For example, if the bootloader leaves the display
backlight at a fixed voltage and the backlight driver is never probed,
you don't want the backlight to ever be turned off after boot up.
Also, when multiple devices are added after kernel init, some
suppliers could be added before their consumer devices get added. In
these instances, the supplier devices could get their sync_state
callback called right after they probe because the consumers devices
haven't had a chance to create device links to the suppliers.
To handle this correctly, this change also provides APIs to
pause/resume sync state callbacks so that when multiple devices are
added, their sync_state callback evaluation can be postponed to happen
after all of them are added.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/core.c | 65 ++++++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 23 +++++++++++++++
2 files changed, 88 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index fec2e8ae75fe..8528b5298e14 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -46,6 +46,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
/* Device links support. */
static LIST_HEAD(wait_for_suppliers);
static DEFINE_MUTEX(wfs_lock);
+static LIST_HEAD(deferred_sync);
+static unsigned int supplier_sync_state_disabled;
#ifdef CONFIG_SRCU
static DEFINE_MUTEX(device_links_lock);
@@ -649,6 +651,62 @@ int device_links_check_suppliers(struct device *dev)
return ret;
}
+static void __device_links_supplier_sync_state(struct device *dev)
+{
+ struct device_link *link;
+
+ if (dev->state_synced)
+ return;
+
+ list_for_each_entry(link, &dev->links.consumers, s_node) {
+ if (!(link->flags & DL_FLAG_MANAGED))
+ continue;
+ if (link->status != DL_STATE_ACTIVE)
+ return;
+ }
+
+ if (dev->bus->sync_state)
+ dev->bus->sync_state(dev);
+ else if (dev->driver && dev->driver->sync_state)
+ dev->driver->sync_state(dev);
+
+ dev->state_synced = true;
+}
+
+void device_links_supplier_sync_state_pause(void)
+{
+ device_links_write_lock();
+ supplier_sync_state_disabled++;
+ device_links_write_unlock();
+}
+
+void device_links_supplier_sync_state_resume(void)
+{
+ struct device *dev, *tmp;
+
+ device_links_write_lock();
+ if (!supplier_sync_state_disabled) {
+ WARN(true, "Unmatched sync_state pause/resume!");
+ goto out;
+ }
+ supplier_sync_state_disabled--;
+ if (supplier_sync_state_disabled)
+ goto out;
+
+ list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_sync) {
+ __device_links_supplier_sync_state(dev);
+ list_del_init(&dev->links.defer_sync);
+ }
+out:
+ device_links_write_unlock();
+}
+
+static void __device_links_supplier_defer_sync(struct device *sup)
+{
+ if (list_empty(&sup->links.defer_sync))
+ list_add_tail(&sup->links.defer_sync, &deferred_sync);
+}
+
/**
* device_links_driver_bound - Update device links after probing its driver.
* @dev: Device to update the links for.
@@ -693,6 +751,11 @@ void device_links_driver_bound(struct device *dev)
WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
WRITE_ONCE(link->status, DL_STATE_ACTIVE);
+
+ if (supplier_sync_state_disabled)
+ __device_links_supplier_defer_sync(link->supplier);
+ else
+ __device_links_supplier_sync_state(link->supplier);
}
dev->links.status = DL_DEV_DRIVER_BOUND;
@@ -809,6 +872,7 @@ void device_links_driver_cleanup(struct device *dev)
WRITE_ONCE(link->status, DL_STATE_DORMANT);
}
+ list_del_init(&dev->links.defer_sync);
__device_links_no_driver(dev);
device_links_write_unlock();
@@ -1783,6 +1847,7 @@ void device_initialize(struct device *dev)
INIT_LIST_HEAD(&dev->links.consumers);
INIT_LIST_HEAD(&dev->links.suppliers);
INIT_LIST_HEAD(&dev->links.needs_suppliers);
+ INIT_LIST_HEAD(&dev->links.defer_sync);
dev->links.status = DL_DEV_NO_DRIVER;
}
EXPORT_SYMBOL_GPL(device_initialize);
diff --git a/include/linux/device.h b/include/linux/device.h
index 65ac1fa40939..224d0126badc 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -84,6 +84,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
* available at the time this function is called. As in, the
* function should NOT stop at the first failed device link if
* other unlinked supplier devices are present in the system.
+ * This is necessary for the sync_state() callback to work
+ * correctly.
*
* Return 0 if device links have been successfully created to all
* the suppliers of this device. Return an error if some of the
@@ -91,6 +93,13 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
* reattempted in the future.
* @probe: Called when a new device or driver add to this bus, and callback
* the specific driver's probe to initial the matched device.
+ * @sync_state: Called to sync device state to software state after all the
+ * state tracking consumers linked to this device (present at
+ * the time of late_initcall) have successfully bound to a
+ * driver. If the device has no consumers, this function will
+ * be called at late_initcall_sync level. If the device has
+ * consumers that are never bound to a driver, this function
+ * will never get called until they do.
* @remove: Called when a device removed from this bus.
* @shutdown: Called at shut-down time to quiesce the device.
*
@@ -135,6 +144,7 @@ struct bus_type {
int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
int (*add_links)(struct device *dev);
int (*probe)(struct device *dev);
+ void (*sync_state)(struct device *dev);
int (*remove)(struct device *dev);
void (*shutdown)(struct device *dev);
@@ -280,6 +290,13 @@ enum probe_type {
* @probe: Called to query the existence of a specific device,
* whether this driver can work with it, and bind the driver
* to a specific device.
+ * @sync_state: Called to sync device state to software state after all the
+ * state tracking consumers linked to this device (present at
+ * the time of late_initcall) have successfully bound to a
+ * driver. If the device has no consumers, this function will
+ * be called at late_initcall_sync level. If the device has
+ * consumers that are never bound to a driver, this function
+ * will never get called until they do.
* @remove: Called when the device is removed from the system to
* unbind a device from this driver.
* @shutdown: Called at shut-down time to quiesce the device.
@@ -318,6 +335,7 @@ struct device_driver {
int (*edit_links)(struct device *dev);
int (*probe) (struct device *dev);
+ void (*sync_state)(struct device *dev);
int (*remove) (struct device *dev);
void (*shutdown) (struct device *dev);
int (*suspend) (struct device *dev, pm_message_t state);
@@ -923,12 +941,14 @@ enum dl_dev_state {
* @suppliers: List of links to supplier devices.
* @consumers: List of links to consumer devices.
* @needs_suppliers: Hook to global list of devices waiting for suppliers.
+ * @defer_sync: Hook to global list of devices that have deferred sync_state.
* @status: Driver status information.
*/
struct dev_links_info {
struct list_head suppliers;
struct list_head consumers;
struct list_head needs_suppliers;
+ struct list_head defer_sync;
enum dl_dev_state status;
};
@@ -1101,6 +1121,7 @@ struct device {
bool offline:1;
bool of_node_reused:1;
bool has_edit_links:1;
+ bool state_synced:1;
#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
@@ -1445,6 +1466,8 @@ struct device_link *device_link_add(struct device *consumer,
void device_link_del(struct device_link *link);
void device_link_remove(void *consumer, struct device *supplier);
void device_link_remove_from_wfs(struct device *consumer);
+void device_links_supplier_sync_state_pause(void);
+void device_links_supplier_sync_state_resume(void);
#ifndef dev_fmt
#define dev_fmt(fmt) fmt
--
2.22.0.709.g102302147b-goog
^ permalink raw reply related
* [PATCH v8 3/7] of/platform: Add functional dependency link from DT bindings
From: Saravana Kannan @ 2019-07-29 22:10 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand, Jonathan Corbet
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team, linux-doc
In-Reply-To: <20190729221101.228240-1-saravanak@google.com>
Add device-links after the devices are created (but before they are
probed) by looking at common DT bindings like clocks and
interconnects.
Automatically adding device-links for functional dependencies at the
framework level provides the following benefits:
- Optimizes device probe order and avoids the useless work of
attempting probes of devices that will not probe successfully
(because their suppliers aren't present or haven't probed yet).
For example, in a commonly available mobile SoC, registering just
one consumer device's driver at an initcall level earlier than the
supplier device's driver causes 11 failed probe attempts before the
consumer device probes successfully. This was with a kernel with all
the drivers statically compiled in. This problem gets a lot worse if
all the drivers are loaded as modules without direct symbol
dependencies.
- Supplier devices like clock providers, interconnect providers, etc
need to keep the resources they provide active and at a particular
state(s) during boot up even if their current set of consumers don't
request the resource to be active. This is because the rest of the
consumers might not have probed yet and turning off the resource
before all the consumers have probed could lead to a hang or
undesired user experience.
Some frameworks (Eg: regulator) handle this today by turning off
"unused" resources at late_initcall_sync and hoping all the devices
have probed by then. This is not a valid assumption for systems with
loadable modules. Other frameworks (Eg: clock) just don't handle
this due to the lack of a clear signal for when they can turn off
resources. This leads to downstream hacks to handle cases like this
that can easily be solved in the upstream kernel.
By linking devices before they are probed, we give suppliers a clear
count of the number of dependent consumers. Once all of the
consumers are active, the suppliers can turn off the unused
resources without making assumptions about the number of consumers.
By default we just add device-links to track "driver presence" (probe
succeeded) of the supplier device. If any other functionality provided
by device-links are needed, it is left to the consumer/supplier
devices to change the link when they probe.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
.../admin-guide/kernel-parameters.txt | 5 +
drivers/of/platform.c | 165 ++++++++++++++++++
2 files changed, 170 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 7ccd158b3894..dba3200d3516 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3170,6 +3170,11 @@
This can be set from sysctl after boot.
See Documentation/admin-guide/sysctl/vm.rst for details.
+ of_devlink [KNL] Make device links from common DT bindings. Useful
+ for optimizing probe order and making sure resources
+ aren't turned off before the consumer devices have
+ probed.
+
ohci1394_dma=early [HW] enable debugging via the ohci1394 driver.
See Documentation/debugging-via-ohci1394.txt for more
info.
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 7801e25e6895..4344419a26fc 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -508,6 +508,170 @@ int of_platform_default_populate(struct device_node *root,
}
EXPORT_SYMBOL_GPL(of_platform_default_populate);
+bool of_link_is_valid(struct device_node *con, struct device_node *sup)
+{
+ of_node_get(sup);
+ /*
+ * Don't allow linking a device node as a consumer of one of its
+ * descendant nodes. By definition, a child node can't be a functional
+ * dependency for the parent node.
+ */
+ while (sup) {
+ if (sup == con) {
+ of_node_put(sup);
+ return false;
+ }
+ sup = of_get_next_parent(sup);
+ }
+ return true;
+}
+
+static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
+{
+ struct platform_device *sup_dev;
+ u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
+ int ret = 0;
+
+ /*
+ * Since we are trying to create device links, we need to find
+ * the actual device node that owns this supplier phandle.
+ * Often times it's the same node, but sometimes it can be one
+ * of the parents. So walk up the parent till you find a
+ * device.
+ */
+ while (sup_np && !of_find_property(sup_np, "compatible", NULL))
+ sup_np = of_get_next_parent(sup_np);
+ if (!sup_np)
+ return 0;
+
+ if (!of_link_is_valid(dev->of_node, sup_np)) {
+ of_node_put(sup_np);
+ return 0;
+ }
+ sup_dev = of_find_device_by_node(sup_np);
+ of_node_put(sup_np);
+ if (!sup_dev)
+ return -ENODEV;
+ if (!device_link_add(dev, &sup_dev->dev, dl_flags))
+ ret = -ENODEV;
+ put_device(&sup_dev->dev);
+ return ret;
+}
+
+static struct device_node *parse_prop_cells(struct device_node *np,
+ const char *prop, int index,
+ const char *binding,
+ const char *cell)
+{
+ struct of_phandle_args sup_args;
+
+ /* Don't need to check property name for every index. */
+ if (!index && strcmp(prop, binding))
+ return NULL;
+
+ if (of_parse_phandle_with_args(np, binding, cell, index, &sup_args))
+ return NULL;
+
+ return sup_args.np;
+}
+
+static struct device_node *parse_clocks(struct device_node *np,
+ const char *prop, int index)
+{
+ return parse_prop_cells(np, prop, index, "clocks", "#clock-cells");
+}
+
+static struct device_node *parse_interconnects(struct device_node *np,
+ const char *prop, int index)
+{
+ return parse_prop_cells(np, prop, index, "interconnects",
+ "#interconnect-cells");
+}
+
+static int strcmp_suffix(const char *str, const char *suffix)
+{
+ unsigned int len, suffix_len;
+
+ len = strlen(str);
+ suffix_len = strlen(suffix);
+ if (len <= suffix_len)
+ return -1;
+ return strcmp(str + len - suffix_len, suffix);
+}
+
+static struct device_node *parse_regulators(struct device_node *np,
+ const char *prop, int index)
+{
+ if (index || strcmp_suffix(prop, "-supply"))
+ return NULL;
+
+ return of_parse_phandle(np, prop, 0);
+}
+
+/**
+ * struct supplier_bindings - Information for parsing supplier DT binding
+ *
+ * @parse_prop: If the function cannot parse the property, return NULL.
+ * Otherwise, return the phandle listed in the property
+ * that corresponds to the index.
+ */
+struct supplier_bindings {
+ struct device_node *(*parse_prop)(struct device_node *np,
+ const char *name, int index);
+};
+
+static const struct supplier_bindings bindings[] = {
+ { .parse_prop = parse_clocks, },
+ { .parse_prop = parse_interconnects, },
+ { .parse_prop = parse_regulators, },
+ { },
+};
+
+static bool of_link_property(struct device *dev, struct device_node *con_np,
+ const char *prop)
+{
+ struct device_node *phandle;
+ struct supplier_bindings *s = bindings;
+ unsigned int i = 0;
+ bool done = true, matched = false;
+
+ while (!matched && s->parse_prop) {
+ while ((phandle = s->parse_prop(con_np, prop, i))) {
+ matched = true;
+ i++;
+ if (of_link_to_phandle(dev, phandle))
+ /*
+ * Don't stop at the first failure. See
+ * Documentation for bus_type.add_links for
+ * more details.
+ */
+ done = false;
+ }
+ s++;
+ }
+ return done ? 0 : -ENODEV;
+}
+
+static bool of_devlink;
+core_param(of_devlink, of_devlink, bool, 0);
+
+static int of_link_to_suppliers(struct device *dev)
+{
+ struct property *p;
+ bool done = true;
+
+ if (!of_devlink)
+ return 0;
+ if (unlikely(!dev->of_node))
+ return 0;
+
+ for_each_property_of_node(dev->of_node, p)
+ if (of_link_property(dev, dev->of_node, p->name))
+ done = false;
+
+ return done ? 0 : -ENODEV;
+}
+
#ifndef CONFIG_PPC
static const struct of_device_id reserved_mem_matches[] = {
{ .compatible = "qcom,rmtfs-mem" },
@@ -523,6 +687,7 @@ static int __init of_platform_default_populate_init(void)
if (!of_have_populated_dt())
return -ENODEV;
+ platform_bus_type.add_links = of_link_to_suppliers;
/*
* Handle certain compatibles explicitly, since we don't want to create
* platform_devices for every node in /reserved-memory with a
--
2.22.0.709.g102302147b-goog
^ permalink raw reply related
* [PATCH v8 2/7] driver core: Add edit_links() callback for drivers
From: Saravana Kannan @ 2019-07-29 22:10 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190729221101.228240-1-saravanak@google.com>
The driver core/bus adding supplier-consumer dependencies by default
enables functional dependencies to be tracked correctly even when the
consumer devices haven't had their drivers registered or loaded (if they
are modules).
However, when the bus incorrectly adds dependencies that it shouldn't
have added, the devices might never probe.
For example, if device-C is a consumer of device-S and they have
phandles to each other in DT, the following could happen:
1. Device-S get added first.
2. The bus add_links() callback will (incorrectly) try to link it as
a consumer of device-C.
3. Since device-C isn't present, device-S will be put in
"waiting-for-supplier" list.
4. Device-C gets added next.
5. All devices in "waiting-for-supplier" list are retried for linking.
6. Device-S gets linked as consumer to Device-C.
7. The bus add_links() callback will (correctly) try to link it as
a consumer of device-S.
8. This isn't allowed because it would create a cyclic device links.
Neither devices will get probed since the supplier is marked as
dependent on the consumer. And the consumer will never probe because the
consumer can't get resources from the supplier.
Without this patch, things stay in this broken state. However, with this
patch, the execution will continue like this:
9. Device-C's driver is loaded.
10. Device-C's driver removes Device-S as a consumer of Device-C.
11. Device-C's driver adds Device-C as a consumer of Device-S.
12. Device-S probes.
14. Device-C probes.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/core.c | 24 ++++++++++++++++++++++--
drivers/base/dd.c | 29 +++++++++++++++++++++++++++++
include/linux/device.h | 18 ++++++++++++++++++
3 files changed, 69 insertions(+), 2 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 62d416e667bd..fec2e8ae75fe 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -437,6 +437,19 @@ static void device_link_wait_for_supplier(struct device *consumer)
mutex_unlock(&wfs_lock);
}
+/**
+ * device_link_remove_from_wfs - Unmark device as waiting for supplier
+ * @consumer: Consumer device
+ *
+ * Unmark the consumer device as waiting for suppliers to become available.
+ */
+void device_link_remove_from_wfs(struct device *consumer)
+{
+ mutex_lock(&wfs_lock);
+ list_del_init(&consumer->links.needs_suppliers);
+ mutex_unlock(&wfs_lock);
+}
+
/**
* device_link_check_waiting_consumers - Try to unmark waiting consumers
*
@@ -454,12 +467,19 @@ static void device_link_wait_for_supplier(struct device *consumer)
static void device_link_check_waiting_consumers(void)
{
struct device *dev, *tmp;
+ int ret;
mutex_lock(&wfs_lock);
list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
- links.needs_suppliers)
- if (!dev->bus->add_links(dev))
+ links.needs_suppliers) {
+ ret = 0;
+ if (dev->has_edit_links)
+ ret = driver_edit_links(dev);
+ else if (dev->bus->add_links)
+ ret = dev->bus->add_links(dev);
+ if (!ret)
list_del_init(&dev->links.needs_suppliers);
+ }
mutex_unlock(&wfs_lock);
}
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 994a90747420..5e7041ede0d7 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -698,6 +698,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
drv->bus->name, __func__, dev_name(dev), drv->name);
+ if (drv->edit_links) {
+ if (drv->edit_links(dev))
+ dev->has_edit_links = true;
+ else
+ device_link_remove_from_wfs(dev);
+ }
pm_runtime_get_suppliers(dev);
if (dev->parent)
pm_runtime_get_sync(dev->parent);
@@ -786,6 +792,29 @@ struct device_attach_data {
bool have_async;
};
+static int __driver_edit_links(struct device_driver *drv, void *data)
+{
+ struct device *dev = data;
+
+ if (!drv->edit_links)
+ return 0;
+
+ if (driver_match_device(drv, dev) <= 0)
+ return 0;
+
+ return drv->edit_links(dev);
+}
+
+int driver_edit_links(struct device *dev)
+{
+ int ret;
+
+ device_lock(dev);
+ ret = bus_for_each_drv(dev->bus, NULL, dev, __driver_edit_links);
+ device_unlock(dev);
+ return ret;
+}
+
static int __device_attach_driver(struct device_driver *drv, void *_data)
{
struct device_attach_data *data = _data;
diff --git a/include/linux/device.h b/include/linux/device.h
index dbcf1de5e9fa..65ac1fa40939 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -263,6 +263,20 @@ enum probe_type {
* @probe_type: Type of the probe (synchronous or asynchronous) to use.
* @of_match_table: The open firmware table.
* @acpi_match_table: The ACPI match table.
+ * @edit_links: Called to allow a matched driver to edit the device links the
+ * bus might have added incorrectly. This will be useful to handle
+ * cases where the bus incorrectly adds functional dependencies
+ * that aren't true or tries to create cyclic dependencies. But
+ * doesn't correctly handle functional dependencies that are
+ * missed by the bus as the supplier's sync_state might get to
+ * execute before the driver for a missing consumer is loaded and
+ * gets to edit the device links for the consumer.
+ *
+ * This function might be called multiple times after a new device
+ * is added. The function is expected to create all the device
+ * links for the new device and return 0 if it was completed
+ * successfully or return an error if it needs to be reattempted
+ * in the future.
* @probe: Called to query the existence of a specific device,
* whether this driver can work with it, and bind the driver
* to a specific device.
@@ -302,6 +316,7 @@ struct device_driver {
const struct of_device_id *of_match_table;
const struct acpi_device_id *acpi_match_table;
+ int (*edit_links)(struct device *dev);
int (*probe) (struct device *dev);
int (*remove) (struct device *dev);
void (*shutdown) (struct device *dev);
@@ -1085,6 +1100,7 @@ struct device {
bool offline_disabled:1;
bool offline:1;
bool of_node_reused:1;
+ bool has_edit_links:1;
#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
@@ -1336,6 +1352,7 @@ extern int __must_check device_attach(struct device *dev);
extern int __must_check driver_attach(struct device_driver *drv);
extern void device_initial_probe(struct device *dev);
extern int __must_check device_reprobe(struct device *dev);
+extern int driver_edit_links(struct device *dev);
extern bool device_is_bound(struct device *dev);
@@ -1427,6 +1444,7 @@ struct device_link *device_link_add(struct device *consumer,
struct device *supplier, u32 flags);
void device_link_del(struct device_link *link);
void device_link_remove(void *consumer, struct device *supplier);
+void device_link_remove_from_wfs(struct device *consumer);
#ifndef dev_fmt
#define dev_fmt(fmt) fmt
--
2.22.0.709.g102302147b-goog
^ permalink raw reply related
* [PATCH v8 1/7] driver core: Add support for linking devices during device addition
From: Saravana Kannan @ 2019-07-29 22:10 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190729221101.228240-1-saravanak@google.com>
When devices are added, the bus might want to create device links to track
functional dependencies between supplier and consumer devices. This
tracking of supplier-consumer relationship allows optimizing device probe
order and tracking whether all consumers of a supplier are active. The
add_links bus callback is added to support this.
However, when consumer devices are added, they might not have a supplier
device to link to despite needing mandatory resources/functionality from
one or more suppliers. A waiting_for_suppliers list is created to track
such consumers and retry linking them when new devices get added.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/core.c | 83 ++++++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 14 +++++++
2 files changed, 97 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 950e3bd0f45c..62d416e667bd 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -44,6 +44,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
#endif
/* Device links support. */
+static LIST_HEAD(wait_for_suppliers);
+static DEFINE_MUTEX(wfs_lock);
#ifdef CONFIG_SRCU
static DEFINE_MUTEX(device_links_lock);
@@ -416,6 +418,51 @@ struct device_link *device_link_add(struct device *consumer,
}
EXPORT_SYMBOL_GPL(device_link_add);
+/**
+ * device_link_wait_for_supplier - Mark device as waiting for supplier
+ * @consumer: Consumer device
+ *
+ * Marks the consumer device as waiting for suppliers to become available. The
+ * consumer device will never be probed until it's unmarked as waiting for
+ * suppliers. The caller is responsible for adding the link to the supplier
+ * once the supplier device is present.
+ *
+ * This function is NOT meant to be called from the probe function of the
+ * consumer but rather from code that creates/adds the consumer device.
+ */
+static void device_link_wait_for_supplier(struct device *consumer)
+{
+ mutex_lock(&wfs_lock);
+ list_add_tail(&consumer->links.needs_suppliers, &wait_for_suppliers);
+ mutex_unlock(&wfs_lock);
+}
+
+/**
+ * device_link_check_waiting_consumers - Try to unmark waiting consumers
+ *
+ * Loops through all consumers waiting on suppliers and tries to add all their
+ * supplier links. If that succeeds, the consumer device is unmarked as waiting
+ * for suppliers. Otherwise, they are left marked as waiting on suppliers,
+ *
+ * The add_links bus callback is expected to return 0 if it has found and added
+ * all the supplier links for the consumer device. It should return an error if
+ * it isn't able to do so.
+ *
+ * The caller of device_link_wait_for_supplier() is expected to call this once
+ * it's aware of potential suppliers becoming available.
+ */
+static void device_link_check_waiting_consumers(void)
+{
+ struct device *dev, *tmp;
+
+ mutex_lock(&wfs_lock);
+ list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
+ links.needs_suppliers)
+ if (!dev->bus->add_links(dev))
+ list_del_init(&dev->links.needs_suppliers);
+ mutex_unlock(&wfs_lock);
+}
+
static void device_link_free(struct device_link *link)
{
while (refcount_dec_not_one(&link->rpm_active))
@@ -550,6 +597,19 @@ int device_links_check_suppliers(struct device *dev)
struct device_link *link;
int ret = 0;
+ /*
+ * If a device is waiting for one or more suppliers (in
+ * wait_for_suppliers list), it is not ready to probe yet. So just
+ * return -EPROBE_DEFER without having to check the links with existing
+ * suppliers.
+ */
+ mutex_lock(&wfs_lock);
+ if (!list_empty(&dev->links.needs_suppliers)) {
+ mutex_unlock(&wfs_lock);
+ return -EPROBE_DEFER;
+ }
+ mutex_unlock(&wfs_lock);
+
device_links_write_lock();
list_for_each_entry(link, &dev->links.suppliers, c_node) {
@@ -834,6 +894,10 @@ static void device_links_purge(struct device *dev)
{
struct device_link *link, *ln;
+ mutex_lock(&wfs_lock);
+ list_del(&dev->links.needs_suppliers);
+ mutex_unlock(&wfs_lock);
+
/*
* Delete all of the remaining links from this device to any other
* devices (either consumers or suppliers).
@@ -1698,6 +1762,7 @@ void device_initialize(struct device *dev)
#endif
INIT_LIST_HEAD(&dev->links.consumers);
INIT_LIST_HEAD(&dev->links.suppliers);
+ INIT_LIST_HEAD(&dev->links.needs_suppliers);
dev->links.status = DL_DEV_NO_DRIVER;
}
EXPORT_SYMBOL_GPL(device_initialize);
@@ -2133,6 +2198,24 @@ int device_add(struct device *dev)
BUS_NOTIFY_ADD_DEVICE, dev);
kobject_uevent(&dev->kobj, KOBJ_ADD);
+
+ /*
+ * Check if any of the other devices (consumers) have been waiting for
+ * this device (supplier) to be added so that they can create a device
+ * link to it.
+ *
+ * This needs to happen after device_pm_add() because device_link_add()
+ * requires the supplier be registered before it's called.
+ *
+ * But this also needs to happe before bus_probe_device() to make sure
+ * waiting consumers can link to it before the driver is bound to the
+ * device and the driver sync_state callback is called for this device.
+ */
+ device_link_check_waiting_consumers();
+
+ if (dev->bus && dev->bus->add_links && dev->bus->add_links(dev))
+ device_link_wait_for_supplier(dev);
+
bus_probe_device(dev);
if (parent)
klist_add_tail(&dev->p->knode_parent,
diff --git a/include/linux/device.h b/include/linux/device.h
index 5093691f56e7..dbcf1de5e9fa 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -78,6 +78,17 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
* -EPROBE_DEFER it will queue the device for deferred probing.
* @uevent: Called when a device is added, removed, or a few other things
* that generate uevents to add the environment variables.
+ * @add_links: Called, perhaps multiple times per device, after a device is
+ * added to this bus. The function is expected to create device
+ * links to all the suppliers of the input device that are
+ * available at the time this function is called. As in, the
+ * function should NOT stop at the first failed device link if
+ * other unlinked supplier devices are present in the system.
+ *
+ * Return 0 if device links have been successfully created to all
+ * the suppliers of this device. Return an error if some of the
+ * suppliers are not yet available and this function needs to be
+ * reattempted in the future.
* @probe: Called when a new device or driver add to this bus, and callback
* the specific driver's probe to initial the matched device.
* @remove: Called when a device removed from this bus.
@@ -122,6 +133,7 @@ struct bus_type {
int (*match)(struct device *dev, struct device_driver *drv);
int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
+ int (*add_links)(struct device *dev);
int (*probe)(struct device *dev);
int (*remove)(struct device *dev);
void (*shutdown)(struct device *dev);
@@ -895,11 +907,13 @@ enum dl_dev_state {
* struct dev_links_info - Device data related to device links.
* @suppliers: List of links to supplier devices.
* @consumers: List of links to consumer devices.
+ * @needs_suppliers: Hook to global list of devices waiting for suppliers.
* @status: Driver status information.
*/
struct dev_links_info {
struct list_head suppliers;
struct list_head consumers;
+ struct list_head needs_suppliers;
enum dl_dev_state status;
};
--
2.22.0.709.g102302147b-goog
^ permalink raw reply related
* [PATCH v8 0/7] Solve postboot supplier cleanup and optimize probe ordering
From: Saravana Kannan @ 2019-07-29 22:10 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
Add device-links to track functional dependencies between devices
after they are created (but before they are probed) by looking at
their common DT bindings like clocks, interconnects, etc.
Having functional dependencies automatically added before the devices
are probed, provides the following benefits:
- Optimizes device probe order and avoids the useless work of
attempting probes of devices that will not probe successfully
(because their suppliers aren't present or haven't probed yet).
For example, in a commonly available mobile SoC, registering just
one consumer device's driver at an initcall level earlier than the
supplier device's driver causes 11 failed probe attempts before the
consumer device probes successfully. This was with a kernel with all
the drivers statically compiled in. This problem gets a lot worse if
all the drivers are loaded as modules without direct symbol
dependencies.
- Supplier devices like clock providers, interconnect providers, etc
need to keep the resources they provide active and at a particular
state(s) during boot up even if their current set of consumers don't
request the resource to be active. This is because the rest of the
consumers might not have probed yet and turning off the resource
before all the consumers have probed could lead to a hang or
undesired user experience.
Some frameworks (Eg: regulator) handle this today by turning off
"unused" resources at late_initcall_sync and hoping all the devices
have probed by then. This is not a valid assumption for systems with
loadable modules. Other frameworks (Eg: clock) just don't handle
this due to the lack of a clear signal for when they can turn off
resources. This leads to downstream hacks to handle cases like this
that can easily be solved in the upstream kernel.
By linking devices before they are probed, we give suppliers a clear
count of the number of dependent consumers. Once all of the
consumers are active, the suppliers can turn off the unused
resources without making assumptions about the number of consumers.
By default we just add device-links to track "driver presence" (probe
succeeded) of the supplier device. If any other functionality provided
by device-links are needed, it is left to the consumer/supplier
devices to change the link when they probe.
v1 -> v2:
- Drop patch to speed up of_find_device_by_node()
- Drop depends-on property and use existing bindings
v2 -> v3:
- Refactor the code to have driver core initiate the linking of devs
- Have driver core link consumers to supplier before it's probed
- Add support for drivers to edit the device links before probing
v3 -> v4:
- Tested edit_links() on system with cyclic dependency. Works.
- Added some checks to make sure device link isn't attempted from
parent device node to child device node.
- Added way to pause/resume sync_state callbacks across
of_platform_populate().
- Recursively parse DT node to create device links from parent to
suppliers of parent and all child nodes.
v4 -> v5:
- Fixed copy-pasta bugs with linked list handling
- Walk up the phandle reference till I find an actual device (needed
for regulators to work)
- Added support for linking devices from regulator DT bindings
- Tested the whole series again to make sure cyclic dependencies are
broken with edit_links() and regulator links are created properly.
v5 -> v6:
- Split, squashed and reordered some of the patches.
- Refactored the device linking code to follow the same code pattern for
any property.
v6 -> v7:
- No functional changes.
- Renamed i to index
- Added comment to clarify not having to check property name for every
index
- Added "matched" variable to clarify code. No functional change.
- Added comments to include/linux/device.h for add_links()
v7 -> v8:
- Rebased on top of linux-next to handle device link changes in [1]
[1] - https://lore.kernel.org/lkml/2305283.AStDPdUUnE@kreacher/
-Saravana
Saravana Kannan (7):
driver core: Add support for linking devices during device addition
driver core: Add edit_links() callback for drivers
of/platform: Add functional dependency link from DT bindings
driver core: Add sync_state driver/bus callback
of/platform: Pause/resume sync state during init and
of_platform_populate()
of/platform: Create device links for all child-supplier depencencies
of/platform: Don't create device links for default busses
.../admin-guide/kernel-parameters.txt | 5 +
drivers/base/core.c | 168 ++++++++++++++++
drivers/base/dd.c | 29 +++
drivers/of/platform.c | 189 ++++++++++++++++++
include/linux/device.h | 55 +++++
5 files changed, 446 insertions(+)
--
2.22.0.709.g102302147b-goog
^ permalink raw reply
* Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
From: Jernej Škrabec @ 2019-07-29 22:04 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Chen-Yu Tsai, Mark Rutland,
linux-pwm-u79uwXL29TY76Z2rM5mHXA, devicetree, linux-kernel,
Maxime Ripard, Rob Herring, Thierry Reding,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel
In-Reply-To: <20190729185108.tpilwoooxvi2z72e-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König
napisal(a):
> On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote:
> > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König
> >
> > napisal(a):
> > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > > >
> > > > napisal(a):
> > > > > Hello,
> > > > >
> > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > >
> > > > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > > > Kleine-König
> > > > > > > >
> > > > > > > > napisal(a):
> > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec
wrote:
> > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > > > >
> > > > > > > > > > .npwm = 1,
> > > > > > > > > >
> > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > > > = {
> > > > > > > > > > + .has_bus_clock = true,
> > > > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > > > + .has_reset = true,
> > > > > > > > > > + .npwm = 2,
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > >
> > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > > > >
> > > > > > > > > > {
> > > > > > > > > >
> > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > > > >
> > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > > > {
> > > > > > > > > >
> > > > > > > > > > }, {
> > > > > > > > > >
> > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > > > > .data = &sun4i_pwm_single_bypass,
> > > > > > > > > >
> > > > > > > > > > + }, {
> > > > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > > > >
> > > > > > > > > If you follow my suggestion for the two previous patches,
> > > > > > > > > you
> > > > > > > > > can
> > > > > > > > > just
> > > > > > > > >
> > > > > > > > > use:
> > > > > > > > > compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > > "allwinner,sun5i-a10s-pwm";
> > > > > > > > >
> > > > > > > > > and drop this patch.
> > > > > > > >
> > > > > > > > Maxime found out that it's not compatible with A10s due to
> > > > > > > > difference
> > > > > > > > in bypass bit, but yes, I know what you mean.
> > > > > > > >
> > > > > > > > Since H6 requires reset line and bus clock to be specified,
> > > > > > > > it's
> > > > > > > > not
> > > > > > > > compatible from DT binding side. New yaml based binding must
> > > > > > > > somehow
> > > > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > > > standalone compatible. However, depending on conclusions of
> > > > > > > > other
> > > > > > > > discussions, this new compatible can be associated with
> > > > > > > > already
> > > > > > > > available quirks structure or have it's own.> >
> > > > > > >
> > > > > > > I cannot follow. You should be able to specify in the binding
> > > > > > > that
> > > > > > > the
> > > > > > > reset line and bus clock is optional. Then
> > > > > > > allwinner,sun50i-h6-pwm
> > > > > > > without a reset line and bus clock also verifies, but this
> > > > > > > doesn't
> > > > > > > really hurt (and who knows, maybe the next allwinner chip needs
> > > > > > > exactly
> > > > > > > this).
> > > > > >
> > > > > > It is not optional. It will not work if either the clocks or reset
> > > > > > controls
> > > > > > are missing. How would these be optional anyway? Either it's
> > > > > > connected
> > > > > > and
> > > > > > thus required, or it's not and therefore should be omitted from
> > > > > > the
> > > > > > description.
> > > > >
> > > > > [Just arguing about the clock here, the argumentation is analogous
> > > > > for
> > > > > the reset control.]
> > > > >
> > > > > From the driver's perspective it's optional: There are devices with
> > > > > and
> > > > > without a bus clock. This doesn't mean that you can just ignore this
> > > > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > > > specify it, then assume this is a device that doesn't have it and so
> > > > > you
> > > > > don't need to handle it." but not in the sense "it doesn't matter if
> > > > > you handle it or not.".
> > > > >
> > > > > Other than that I'm on your side. So for example I think it's not
> > > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > > > > because this hides exactly the kind of problem you point out here.
> > > >
> > > > I think there's misunderstanding. I only argued that we can't use
> > > >
> > > > compatible = "allwinner,sun50i-h6-pwm",
> > > >
> > > > "allwinner,sun5i-a10s-pwm";
> > > >
> > > > as you suggested and only
> > > >
> > > > compatible = "allwinner,sun50i-h6-pwm";
> > > >
> > > > will work. Not because of driver itself (it can still use _optional()
> > > > variants), but because of DT binding, which should be able to validate
> > > > H6
> > > > PWM node - reset and bus clock references are required in this case.
> > >
> > > I think I understood. In my eyes there is no need to let validation of
> > > the DT bindings catch a missing "optional" property that is needed on
> > > H6.
> > >
> > > You have to draw the line somewhere which information the driver has
> > > hard-coded and what is only provided by the device tree and just assumed
> > > to be correct by the driver. You argue the driver should know that
> >
> > No, in this thread I argue that DT validation tool, executed by
> >
> > make ARCH=arm64 dtbs_check
> >
> > should catch that. This is not a driver, but DT binding described in YAML.
>
> The argumentation is the same. dtbs_check doesn't notice if the base
> address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should
> it catch a missing reset controller phandle?
Of course checking actual values of node properties doesn't make sense in
dtbs_check, otherwise we would have million DT bindings. If you have 10 copies
of the same IP core, of course you would use same compatible, but actual
register ranges, interrupts, etc. would be different in DT nodes.
At this point I would make same argument as were made before, but there is no
point going in circles. I'm interested what have DT maintainers to say.
Best regards,
Jernej
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/2452836.v7ux4bnEjb%40jernej-laptop.
^ permalink raw reply
* Re: [v5 1/2] dt-bindings: gpio: aspeed: Add SGPIO support
From: Linus Walleij @ 2019-07-29 21:57 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Hongwei Zhang, open list:GPIO SUBSYSTEM, Joel Stanley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-aspeed, Bartosz Golaszewski, Rob Herring, Mark Rutland,
linux-kernel@vger.kernel.org, Linux ARM
In-Reply-To: <f2875111-9ba9-43b7-b2a4-d00c8725f5a0@www.fastmail.com>
On Mon, Jul 29, 2019 at 2:19 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> The behaviour is to periodically emit the state of all enabled GPIOs
> (i.e. the ngpios value), one per bus clock cycle. There's no explicit
> addressing scheme, the protocol encodes the value for a given GPIO
> by its position in the data stream relative to a pulse on the "load data"
> (LD) line, whose envelope covers the clock cycle for the last GPIO in
> the sequence. Similar to SPI the bus has both out and in lines, which
> cater to output/input GPIOs.
>
> A rough timing diagram for a 16-GPIO configuration looks like what
> I've pasted here:
>
> https://gist.github.com/amboar/c9543af1957854474b8c05ab357f0675
OK that is complex. I agree we need to keep this driver together.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 5/5] MIPS: dts: lantiq: danube: easy50712: route the PCI_INTA IRQ through EBU
From: Hauke Mehrtens @ 2019-07-29 21:55 UTC (permalink / raw)
To: Marc Zyngier, Martin Blumenstingl
Cc: tglx, jason, ralf, paul.burton, jhogan, robh+dt, linux-mips,
devicetree, linux-kernel, mark.rutland, john
In-Reply-To: <86wog2mq6k.wl-marc.zyngier@arm.com>
[-- Attachment #1.1: Type: text/plain, Size: 1803 bytes --]
On 7/28/19 12:03 PM, Marc Zyngier wrote:
> On Sat, 27 Jul 2019 18:53:15 +0100,
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>>
>> EBU provides an interrupt line for the PCI_INTA interrupt. Route
>> easy50712's PCI interrupt to EBU so the interrupt line is configured
>> correctly (using IRQ_TYPE_LEVEL_LOW, this was previously hardcoded in
>> the PCI driver) and ACKed properly.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Acked-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>> arch/mips/boot/dts/lantiq/easy50712.dts | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/boot/dts/lantiq/easy50712.dts b/arch/mips/boot/dts/lantiq/easy50712.dts
>> index 1ce20b7d05cb..33c26b93cfc9 100644
>> --- a/arch/mips/boot/dts/lantiq/easy50712.dts
>> +++ b/arch/mips/boot/dts/lantiq/easy50712.dts
>> @@ -1,6 +1,8 @@
>> // SPDX-License-Identifier: GPL-2.0
>> /dts-v1/;
>>
>> +#include <dt-bindings/interrupt-controller/irq.h>
>> +
>> /include/ "danube.dtsi"
>>
>> / {
>> @@ -105,7 +107,7 @@
>> lantiq,bus-clock = <33333333>;
>> interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
>> interrupt-map = <
>> - 0x7000 0 0 1 &icu0 29 1 // slot 14, irq 29
>> + 0x7000 0 0 1 &ebu0 0 IRQ_TYPE_LEVEL_LOW // slot 14
>> >;
>> gpios-reset = <&gpio 21 0>;
>> req-mask = <0x1>; /* GNT1 */
>> --
>> 2.22.0
>>
>
> Are you OK with breaking compatibility between kernel and DT? It
> usually isn't very nice for users...
I am fine with such changes. I am not aware of any board using this SoC
which ships the kernel and the device tree file as different binaries,
it is always either attached or patched into the kernel and never in the
boot loader.
Hauke
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [RFC-ish PATCH 00/17] Clean up ASPEED devicetree warnings
From: Linus Walleij @ 2019-07-29 21:55 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-aspeed, Rob Herring, Mark Rutland, Joel Stanley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, linux-kernel@vger.kernel.org, anoo, a.filippov,
Arnd Bergmann, yang.brianc.w, Corey Minyard, Greg KH, haiyue.wang,
wangzqbj, chen.kenyy, open list:GPIO SUBSYSTEM,
openipmi-developer, Patrick Venture, sschaeck, Tao
In-Reply-To: <20190726053959.2003-1-andrew@aj.id.au>
On Fri, Jul 26, 2019 at 7:40 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> The aim of this series is to minimise/eliminate all the warnings from the
> ASPEED devicetrees. It mostly achieves its goal, as outlined below.
I suppose it will all be merged in the Aspeed tree?
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 0/3] ARM: dts: aspeed: Deprecate g[45]-style compatibles
From: Linus Walleij @ 2019-07-29 21:53 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-aspeed, Lee Jones, Rob Herring, Mark Rutland, Joel Stanley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, linux-kernel@vger.kernel.org, open list:GPIO SUBSYSTEM
In-Reply-To: <20190724081313.12934-1-andrew@aj.id.au>
On Wed, Jul 24, 2019 at 10:13 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> It's probably best if we push the three patches all through one tree rather
> than fragmenting. Is everyone happy if Joel applies them to the aspeed tree?
If you are sure it will not collide with parallell work in the
pinctrl tree, yes.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
(If it does collide I'd prefer to take the pinctrl patches and fix the
conflicts in my tree.)
Yours,
Linus Walleij
^ permalink raw reply
* Re: Ingenic pinctrl patchs.
From: Linus Walleij @ 2019-07-29 21:42 UTC (permalink / raw)
To: Zhou Yanjie
Cc: linux-mips, linux-kernel@vger.kernel.org,
open list:GPIO SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Paul Burton, Rob Herring, Mark Rutland
In-Reply-To: <1563076436-5338-1-git-send-email-zhouyanjie@zoho.com>
On Sun, Jul 14, 2019 at 8:55 AM Zhou Yanjie <zhouyanjie@zoho.com> wrote:
> Add support for Ingenic JZ4760, JZ4760B, X1000, X1000E and X1500.
All 6 patches applied. Seems very straight-forward thanks for fixing this!
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: aspeed: Strip unnecessary quotes
From: Linus Walleij @ 2019-07-29 21:37 UTC (permalink / raw)
To: Andrew Jeffery
Cc: open list:GPIO SUBSYSTEM, Rob Herring, Mark Rutland, Joel Stanley,
linux-aspeed,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, linux-kernel@vger.kernel.org
In-Reply-To: <20190712061721.26645-1-andrew@aj.id.au>
On Fri, Jul 12, 2019 at 8:17 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> Rob pointed out that we didn't need the noise of the quotes in a
> separate review[1], so strip them out for consistency and avoid setting
> a bad example.
>
> [1] https://lists.ozlabs.org/pipermail/linux-aspeed/2019-July/002009.html
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Patch applied.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2] pinctrl: msm8998: Squash TSIF pins together
From: Linus Walleij @ 2019-07-29 21:30 UTC (permalink / raw)
To: Marc Gonzalez
Cc: DT, Jeffrey Hugo, MSM, Jonathan Neuschäfer, Bjorn Andersson,
gpio, Rob Herring, Linux ARM
In-Reply-To: <503b2ae8-ead6-70cd-7b21-ce5f5166a23a@free.fr>
On Thu, Jul 4, 2019 at 10:57 AM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote:
> TSIF is the Transport Stream Interface.
> First, rename tsif1 to tsif0, and tsif2 to tsif1.
> Then squash all 5 tsif0 pins into a single function.
> Same for tsif1.
>
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> ---
> Changes from v1:
> - Reword commit message for clarity (hopefully)
> - Drop unrelated change in qcom,msm8998-pinctrl.txt
> - CC DT
Patch applied with the ACKs.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2 3/6] pwm: jz4740: Apply configuration atomically
From: Paul Cercueil @ 2019-07-29 21:19 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Thierry Reding, Rob Herring, Mark Rutland, od, linux-pwm,
devicetree, linux-kernel, kernel
In-Reply-To: <20190724064745.7ghecdpg3gmxsiim@pengutronix.de>
Hi Uwe,
Le mer. 24 juil. 2019 à 2:47, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
>
> On Tue, Jul 23, 2019 at 04:46:40PM -0400, Paul Cercueil wrote:
>> Le lun. 22 juil. 2019 à 15:34, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>> <u.kleine-koenig@pengutronix.de> a écrit :
>> > On Fri, Jun 07, 2019 at 05:44:07PM +0200, Paul Cercueil wrote:
>> > > - is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
>> > > - if (is_enabled)
>> > > - jz4740_pwm_disable(chip, pwm);
>> > > + jz4740_pwm_disable(chip, pwm);
>> >
>> > I assume this stops the PWM. Does this complete the currently
>> running
>> > period? How does the PWM behave then? (Does it still drive the
>> output?
>> > If so, on which level?)
>>
>> Some PWM channels work in one mode "TCU1" and others work in
>> "TCU2". The
>> mode in which channels work depends on the version of the SoC.
>>
>> When stopped, the pins of TCU1 channels will be driven to the
>> inactive
>> level (which depends on the polarity). It is unknown whether or not
>> the
>> currently running period is completed. We set a bit to configure for
>> "abrupt shutdown", so I expect that it's not, but somebody would
>> need
>> to hook up a logic analyzer to see what's the exact behaviour with
>> and without that bit.
>
> This might be done even without a logic analyzer. Just do something
> like:
>
> pwm_apply_state(pwm, { .enabled = 1, .period = 5s })
> pwm_apply_state(pwm, { .enabled = 1, .period = 5s, .duty = 5s })
>
> and if that takes less then 5s the period is not completed.
>
> And note that "abrupt shutdown" is a bug.
I remember you asked that already in an older patchset.
The result of this test is that the period is never completed,
independently of the "abrupt shutdown" bit.
Cheers,
-Paul
>> TCU2 channels on the other hand will stop in the middle of a period,
>> leaving the pin hanging at whatever level it was before the stop.
>> That's the rationale behind the trick in commit 6580fd173070 ("pwm:
>> jz4740: Force TCU2 channels to return to their init level").
>
> Strange, but ok.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König
> |
> Industrial Linux Solutions |
> http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH] dt-bindings: clk: allwinner, sun4i-a10-ccu: Correct path in $id
From: Stephen Boyd @ 2019-07-29 21:12 UTC (permalink / raw)
To: Rob Herring, devicetree
Cc: Maxime Ripard, Michael Turquette, Chen-Yu Tsai, linux-arm-kernel,
linux-clk
In-Reply-To: <20190723230843.19922-1-robh@kernel.org>
Quoting Rob Herring (2019-07-23 16:08:43)
> The path in the schema '$id' value is wrong. Fix it.
>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
Acked-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply
* [v5 2/2] gpio: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-29 20:56 UTC (permalink / raw)
To: Andrew Jeffery, Linus Walleij, linux-gpio
Cc: Hongwei Zhang, Joel Stanley, devicetree, linux-aspeed,
Bartosz Golaszewski, linux-kernel, linux-arm-kernel
In-Reply-To: <1563564291-9692-3-git-send-email-hongweiz@ami.com>
Add SGPIO driver support for Aspeed AST2500 SoC.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
drivers/gpio/sgpio-aspeed.c | 521 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 521 insertions(+)
create mode 100644 drivers/gpio/sgpio-aspeed.c
diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
new file mode 100644
index 0000000..9a17b1a
--- /dev/null
+++ b/drivers/gpio/sgpio-aspeed.c
@@ -0,0 +1,521 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2019 American Megatrends International LLC.
+ *
+ * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/gpio/driver.h>
+#include <linux/hashtable.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+
+#define MAX_NR_SGPIO 80
+
+#define ASPEED_SGPIO_CTRL 0x54
+
+#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
+#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
+#define ASPEED_SGPIO_ENABLE BIT(0)
+
+struct aspeed_sgpio {
+ struct gpio_chip chip;
+ struct clk *pclk;
+ spinlock_t lock;
+ void __iomem *base;
+ uint32_t dir_in[3];
+ int irq;
+};
+
+struct aspeed_sgpio_bank {
+ uint16_t val_regs;
+ uint16_t rdata_reg;
+ uint16_t irq_regs;
+ const char names[4][3];
+};
+
+/*
+ * Note: The "value" register returns the input value when the GPIO is
+ * configured as an input.
+ *
+ * The "rdata" register returns the output value when the GPIO is
+ * configured as an output.
+ */
+static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
+ {
+ .val_regs = 0x0000,
+ .rdata_reg = 0x0070,
+ .irq_regs = 0x0004,
+ .names = { "A", "B", "C", "D" },
+ },
+ {
+ .val_regs = 0x001C,
+ .rdata_reg = 0x0074,
+ .irq_regs = 0x0020,
+ .names = { "E", "F", "G", "H" },
+ },
+ {
+ .val_regs = 0x0038,
+ .rdata_reg = 0x0078,
+ .irq_regs = 0x003C,
+ .names = { "I", "J" },
+ },
+};
+
+enum aspeed_sgpio_reg {
+ reg_val,
+ reg_rdata,
+ reg_irq_enable,
+ reg_irq_type0,
+ reg_irq_type1,
+ reg_irq_type2,
+ reg_irq_status,
+};
+
+#define GPIO_VAL_VALUE 0x00
+#define GPIO_IRQ_ENABLE 0x00
+#define GPIO_IRQ_TYPE0 0x04
+#define GPIO_IRQ_TYPE1 0x08
+#define GPIO_IRQ_TYPE2 0x0C
+#define GPIO_IRQ_STATUS 0x10
+
+static void __iomem *bank_reg(struct aspeed_sgpio *gpio,
+ const struct aspeed_sgpio_bank *bank,
+ const enum aspeed_sgpio_reg reg)
+{
+ switch (reg) {
+ case reg_val:
+ return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
+ case reg_rdata:
+ return gpio->base + bank->rdata_reg;
+ case reg_irq_enable:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
+ case reg_irq_type0:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
+ case reg_irq_type1:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
+ case reg_irq_type2:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
+ case reg_irq_status:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
+ default:
+ /* acturally if code runs to here, it's an error case */
+ BUG_ON(1);
+ }
+}
+
+#define GPIO_BANK(x) ((x) >> 5)
+#define GPIO_OFFSET(x) ((x) & 0x1f)
+#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
+
+static const struct aspeed_sgpio_bank *to_bank(unsigned int offset)
+{
+ unsigned int bank = GPIO_BANK(offset);
+
+ WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
+ return &aspeed_sgpio_banks[bank];
+}
+
+static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ const struct aspeed_sgpio_bank *bank = to_bank(offset);
+ unsigned long flags;
+ enum aspeed_sgpio_reg reg;
+ bool is_input;
+ int rc = 0;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ is_input = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
+ reg = is_input ? reg_val : reg_rdata;
+ rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset));
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return rc;
+}
+
+static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ const struct aspeed_sgpio_bank *bank = to_bank(offset);
+ unsigned long flags;
+ void __iomem *addr;
+ u32 reg = 0;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_val);
+
+ if (val)
+ reg |= GPIO_BIT(offset);
+ else
+ reg &= ~GPIO_BIT(offset);
+
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ gpio->dir_in[GPIO_BANK(offset)] |= GPIO_BIT(offset);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return 0;
+}
+
+static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ aspeed_sgpio_set(gc, offset, val);
+
+ return 0;
+}
+
+static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ int dir_status;
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ dir_status = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return dir_status;
+
+}
+
+static void irqd_to_aspeed_sgpio_data(struct irq_data *d,
+ struct aspeed_sgpio **gpio,
+ const struct aspeed_sgpio_bank **bank,
+ u32 *bit, int *offset)
+{
+ struct aspeed_sgpio *internal;
+
+ *offset = irqd_to_hwirq(d);
+ internal = irq_data_get_irq_chip_data(d);
+ WARN_ON(!internal);
+
+ *gpio = internal;
+ *bank = to_bank(*offset);
+ *bit = GPIO_BIT(*offset);
+}
+
+static void aspeed_sgpio_irq_ack(struct irq_data *d)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *status_addr;
+ int offset;
+ u32 bit;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+ status_addr = bank_reg(gpio, bank, reg_irq_status);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ iowrite32(bit, status_addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ u32 reg, bit;
+ void __iomem *addr;
+ int offset;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+ addr = bank_reg(gpio, bank, reg_irq_enable);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ reg = ioread32(addr);
+ if (set)
+ reg |= bit;
+ else
+ reg &= ~bit;
+
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_mask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, false);
+}
+
+static void aspeed_sgpio_irq_unmask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, true);
+}
+
+static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
+{
+ u32 type0 = 0;
+ u32 type1 = 0;
+ u32 type2 = 0;
+ u32 bit, reg;
+ const struct aspeed_sgpio_bank *bank;
+ irq_flow_handler_t handler;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *addr;
+ int offset;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+ switch (type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_EDGE_BOTH:
+ type2 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_RISING:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_FALLING:
+ handler = handle_edge_irq;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_LEVEL_LOW:
+ type1 |= bit;
+ handler = handle_level_irq;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_irq_type0);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type0;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type1);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type1;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type2);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type2;
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ irq_set_handler_locked(d, handler);
+
+ return 0;
+}
+
+static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
+{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *ic = irq_desc_get_chip(desc);
+ struct aspeed_sgpio *data = gpiochip_get_data(gc);
+ unsigned int i, p, girq;
+ unsigned long reg;
+
+ chained_irq_enter(ic, desc);
+
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
+
+ reg = ioread32(bank_reg(data, bank, reg_irq_status));
+
+ for_each_set_bit(p, ®, 32) {
+ girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
+ generic_handle_irq(girq);
+ }
+
+ }
+
+ chained_irq_exit(ic, desc);
+}
+
+static struct irq_chip aspeed_sgpio_irqchip = {
+ .name = "aspeed-sgpio",
+ .irq_ack = aspeed_sgpio_irq_ack,
+ .irq_mask = aspeed_sgpio_irq_mask,
+ .irq_unmask = aspeed_sgpio_irq_unmask,
+ .irq_set_type = aspeed_sgpio_set_type,
+};
+
+static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
+ struct platform_device *pdev)
+{
+ int rc, i;
+ const struct aspeed_sgpio_bank *bank;
+
+ rc = platform_get_irq(pdev, 0);
+ if (rc < 0)
+ return rc;
+
+ gpio->irq = rc;
+
+ /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* disable irq enable bits */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
+ /* clear status bits */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
+ }
+
+ rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
+ 0, handle_bad_irq, IRQ_TYPE_NONE);
+ if (rc) {
+ dev_info(&pdev->dev, "Could not add irqchip\n");
+ return rc;
+ }
+
+ gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
+ gpio->irq, aspeed_sgpio_irq_handler);
+
+ /* set IRQ settings and Enable Interrupt */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* set falling or level-low irq */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
+ /* trigger type is edge */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
+ /* dual edge trigger mode. */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
+ /* enable irq */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
+ }
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_sgpio_of_table[] = {
+ { .compatible = "aspeed,ast2400-sgpio" },
+ { .compatible = "aspeed,ast2500-sgpio" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
+
+static int __init aspeed_sgpio_probe(struct platform_device *pdev)
+{
+ struct aspeed_sgpio *gpio;
+ u32 nr_gpios, sgpio_freq, sgpio_clk_div;
+ int rc;
+ unsigned long apb_freq;
+
+ gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ gpio->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(gpio->base))
+ return PTR_ERR(gpio->base);
+
+ rc = of_property_read_u32(pdev->dev.of_node, "ngpios", &nr_gpios);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Could not read ngpios property\n");
+ return -EINVAL;
+ } else if (nr_gpios > MAX_NR_SGPIO) {
+ dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: %d\n",
+ MAX_NR_SGPIO, nr_gpios);
+ return -EINVAL;
+ }
+
+ rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Could not read bus-frequency property\n");
+ return -EINVAL;
+ }
+
+ gpio->pclk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(gpio->pclk)) {
+ dev_err(&pdev->dev, "devm_clk_get failed\n");
+ return PTR_ERR(gpio->pclk);
+ }
+
+ apb_freq = clk_get_rate(gpio->pclk);
+
+ /*
+ * From the datasheet,
+ * SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
+ * period = 2 * (GPIO254[31:16] + 1) / PCLK
+ * frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
+ * frequency = PCLK / (2 * (GPIO254[31:16] + 1))
+ * frequency * 2 * (GPIO254[31:16] + 1) = PCLK
+ * GPIO254[31:16] = PCLK / (frequency * 2) - 1
+ */
+ if (sgpio_freq == 0)
+ return -EINVAL;
+
+ sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
+
+ if (sgpio_clk_div > (1 << 16) - 1)
+ return -EINVAL;
+
+ iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
+ FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
+ ASPEED_SGPIO_ENABLE,
+ gpio->base + ASPEED_SGPIO_CTRL);
+
+ spin_lock_init(&gpio->lock);
+
+ gpio->chip.parent = &pdev->dev;
+ gpio->chip.ngpio = nr_gpios;
+ gpio->chip.direction_input = aspeed_sgpio_dir_in;
+ gpio->chip.direction_output = aspeed_sgpio_dir_out;
+ gpio->chip.get_direction = aspeed_sgpio_get_direction;
+ gpio->chip.request = NULL;
+ gpio->chip.free = NULL;
+ gpio->chip.get = aspeed_sgpio_get;
+ gpio->chip.set = aspeed_sgpio_set;
+ gpio->chip.set_config = NULL;
+ gpio->chip.label = dev_name(&pdev->dev);
+ gpio->chip.base = -1;
+
+ /* set all SGPIO pins as input (1). */
+ memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in));
+
+ rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+ if (rc < 0)
+ return rc;
+
+ return aspeed_sgpio_setup_irqs(gpio, pdev);
+}
+
+static struct platform_driver aspeed_sgpio_driver = {
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = aspeed_sgpio_of_table,
+ },
+};
+
+module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
+MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related
* [v6 2/2] gpio: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-29 20:43 UTC (permalink / raw)
To: Linus Walleij, Andrew Jeffery
Cc: devicetree, linux-aspeed, linux-kernel, Bartosz Golaszewski,
Joel Stanley, Hongwei Zhang, linux-arm-kernel
In-Reply-To: <1563564291-9692-3-git-send-email-hongweiz@ami.com>
Hello Linus,
Thanks for your detailed comments.
We just submitted a v6 of sgpio-aspeed.c, it includes the updates based on
your initial feedback:
1. fix a bug in aspeed_sgpio_dir_out()
2. some comments clean up.
Regards,
--Hongwei
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Sunday, July 28, 2019 7:38 PM
> To: Hongwei Zhang
> Cc: Andrew Jeffery; linux-gpio; Joel Stanley; linux-aspeed; Bartosz Golaszewski; linux-kernel; linux-
> arm-kernel
> Subject: Re: [v5 2/2] gpio: aspeed: Add SGPIO driver
>
> On Mon, Jul 22, 2019 at 10:37 PM Hongwei Zhang <hongweiz@ami.com> wrote:
>
> > As you suspected it correctly, AST2500 utilizes all the 32 bits of the
> > registers (data value, interrupt, etc...), such that using 8-bit bands
> > [7:0]/[15:8]/23:16]/[31:24] of GPIO_200H for SGPIO_A/B/C/D .
> > so registering 10 gpiochip drivers separately will make code more
> > complicated, for example gpio_200 register (data_value reg) has to be
> > shared by 4 gpiochip instances, and the same is true for gpio204
> > (interrupt reg), and other more registers.
> > So we would prefer to keeping current implementation.
>
> OK this is a pretty good argument. My review assumed one 32-bit register was not shared between
> banks but it is, I see.
>
> The above situation can be managed by regmap, but that will just a different complexity so go with this
> approach then.
>
> Yours,
> Linus Walleij
^ permalink raw reply
* [v6 2/2] gpio: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-29 20:29 UTC (permalink / raw)
To: Andrew Jeffery, Linus Walleij, linux-gpio
Cc: Hongwei Zhang, Joel Stanley, devicetree, linux-aspeed,
Bartosz Golaszewski, linux-kernel, linux-arm-kernel
In-Reply-To: <1563564291-9692-3-git-send-email-hongweiz@ami.com>
Add SGPIO driver support for Aspeed AST2500 SoC.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
drivers/gpio/sgpio-aspeed.c | 521 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 521 insertions(+)
create mode 100644 drivers/gpio/sgpio-aspeed.c
diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
new file mode 100644
index 0000000..9a17b1a
--- /dev/null
+++ b/drivers/gpio/sgpio-aspeed.c
@@ -0,0 +1,521 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2019 American Megatrends International LLC.
+ *
+ * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/gpio/driver.h>
+#include <linux/hashtable.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+
+#define MAX_NR_SGPIO 80
+
+#define ASPEED_SGPIO_CTRL 0x54
+
+#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
+#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
+#define ASPEED_SGPIO_ENABLE BIT(0)
+
+struct aspeed_sgpio {
+ struct gpio_chip chip;
+ struct clk *pclk;
+ spinlock_t lock;
+ void __iomem *base;
+ uint32_t dir_in[3];
+ int irq;
+};
+
+struct aspeed_sgpio_bank {
+ uint16_t val_regs;
+ uint16_t rdata_reg;
+ uint16_t irq_regs;
+ const char names[4][3];
+};
+
+/*
+ * Note: The "value" register returns the input value when the GPIO is
+ * configured as an input.
+ *
+ * The "rdata" register returns the output value when the GPIO is
+ * configured as an output.
+ */
+static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
+ {
+ .val_regs = 0x0000,
+ .rdata_reg = 0x0070,
+ .irq_regs = 0x0004,
+ .names = { "A", "B", "C", "D" },
+ },
+ {
+ .val_regs = 0x001C,
+ .rdata_reg = 0x0074,
+ .irq_regs = 0x0020,
+ .names = { "E", "F", "G", "H" },
+ },
+ {
+ .val_regs = 0x0038,
+ .rdata_reg = 0x0078,
+ .irq_regs = 0x003C,
+ .names = { "I", "J" },
+ },
+};
+
+enum aspeed_sgpio_reg {
+ reg_val,
+ reg_rdata,
+ reg_irq_enable,
+ reg_irq_type0,
+ reg_irq_type1,
+ reg_irq_type2,
+ reg_irq_status,
+};
+
+#define GPIO_VAL_VALUE 0x00
+#define GPIO_IRQ_ENABLE 0x00
+#define GPIO_IRQ_TYPE0 0x04
+#define GPIO_IRQ_TYPE1 0x08
+#define GPIO_IRQ_TYPE2 0x0C
+#define GPIO_IRQ_STATUS 0x10
+
+static void __iomem *bank_reg(struct aspeed_sgpio *gpio,
+ const struct aspeed_sgpio_bank *bank,
+ const enum aspeed_sgpio_reg reg)
+{
+ switch (reg) {
+ case reg_val:
+ return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
+ case reg_rdata:
+ return gpio->base + bank->rdata_reg;
+ case reg_irq_enable:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
+ case reg_irq_type0:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
+ case reg_irq_type1:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
+ case reg_irq_type2:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
+ case reg_irq_status:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
+ default:
+ /* acturally if code runs to here, it's an error case */
+ BUG_ON(1);
+ }
+}
+
+#define GPIO_BANK(x) ((x) >> 5)
+#define GPIO_OFFSET(x) ((x) & 0x1f)
+#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
+
+static const struct aspeed_sgpio_bank *to_bank(unsigned int offset)
+{
+ unsigned int bank = GPIO_BANK(offset);
+
+ WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
+ return &aspeed_sgpio_banks[bank];
+}
+
+static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ const struct aspeed_sgpio_bank *bank = to_bank(offset);
+ unsigned long flags;
+ enum aspeed_sgpio_reg reg;
+ bool is_input;
+ int rc = 0;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ is_input = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
+ reg = is_input ? reg_val : reg_rdata;
+ rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset));
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return rc;
+}
+
+static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ const struct aspeed_sgpio_bank *bank = to_bank(offset);
+ unsigned long flags;
+ void __iomem *addr;
+ u32 reg = 0;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_val);
+
+ if (val)
+ reg |= GPIO_BIT(offset);
+ else
+ reg &= ~GPIO_BIT(offset);
+
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ gpio->dir_in[GPIO_BANK(offset)] |= GPIO_BIT(offset);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return 0;
+}
+
+static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ aspeed_sgpio_set(gc, offset, val);
+
+ return 0;
+}
+
+static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ int dir_status;
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ dir_status = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return dir_status;
+
+}
+
+static void irqd_to_aspeed_sgpio_data(struct irq_data *d,
+ struct aspeed_sgpio **gpio,
+ const struct aspeed_sgpio_bank **bank,
+ u32 *bit, int *offset)
+{
+ struct aspeed_sgpio *internal;
+
+ *offset = irqd_to_hwirq(d);
+ internal = irq_data_get_irq_chip_data(d);
+ WARN_ON(!internal);
+
+ *gpio = internal;
+ *bank = to_bank(*offset);
+ *bit = GPIO_BIT(*offset);
+}
+
+static void aspeed_sgpio_irq_ack(struct irq_data *d)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *status_addr;
+ int offset;
+ u32 bit;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+ status_addr = bank_reg(gpio, bank, reg_irq_status);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ iowrite32(bit, status_addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ u32 reg, bit;
+ void __iomem *addr;
+ int offset;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+ addr = bank_reg(gpio, bank, reg_irq_enable);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ reg = ioread32(addr);
+ if (set)
+ reg |= bit;
+ else
+ reg &= ~bit;
+
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_mask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, false);
+}
+
+static void aspeed_sgpio_irq_unmask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, true);
+}
+
+static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
+{
+ u32 type0 = 0;
+ u32 type1 = 0;
+ u32 type2 = 0;
+ u32 bit, reg;
+ const struct aspeed_sgpio_bank *bank;
+ irq_flow_handler_t handler;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *addr;
+ int offset;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+ switch (type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_EDGE_BOTH:
+ type2 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_RISING:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_FALLING:
+ handler = handle_edge_irq;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_LEVEL_LOW:
+ type1 |= bit;
+ handler = handle_level_irq;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_irq_type0);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type0;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type1);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type1;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type2);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type2;
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ irq_set_handler_locked(d, handler);
+
+ return 0;
+}
+
+static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
+{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *ic = irq_desc_get_chip(desc);
+ struct aspeed_sgpio *data = gpiochip_get_data(gc);
+ unsigned int i, p, girq;
+ unsigned long reg;
+
+ chained_irq_enter(ic, desc);
+
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
+
+ reg = ioread32(bank_reg(data, bank, reg_irq_status));
+
+ for_each_set_bit(p, ®, 32) {
+ girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
+ generic_handle_irq(girq);
+ }
+
+ }
+
+ chained_irq_exit(ic, desc);
+}
+
+static struct irq_chip aspeed_sgpio_irqchip = {
+ .name = "aspeed-sgpio",
+ .irq_ack = aspeed_sgpio_irq_ack,
+ .irq_mask = aspeed_sgpio_irq_mask,
+ .irq_unmask = aspeed_sgpio_irq_unmask,
+ .irq_set_type = aspeed_sgpio_set_type,
+};
+
+static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
+ struct platform_device *pdev)
+{
+ int rc, i;
+ const struct aspeed_sgpio_bank *bank;
+
+ rc = platform_get_irq(pdev, 0);
+ if (rc < 0)
+ return rc;
+
+ gpio->irq = rc;
+
+ /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* disable irq enable bits */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
+ /* clear status bits */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
+ }
+
+ rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
+ 0, handle_bad_irq, IRQ_TYPE_NONE);
+ if (rc) {
+ dev_info(&pdev->dev, "Could not add irqchip\n");
+ return rc;
+ }
+
+ gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
+ gpio->irq, aspeed_sgpio_irq_handler);
+
+ /* set IRQ settings and Enable Interrupt */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* set falling or level-low irq */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
+ /* trigger type is edge */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
+ /* dual edge trigger mode. */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
+ /* enable irq */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
+ }
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_sgpio_of_table[] = {
+ { .compatible = "aspeed,ast2400-sgpio" },
+ { .compatible = "aspeed,ast2500-sgpio" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
+
+static int __init aspeed_sgpio_probe(struct platform_device *pdev)
+{
+ struct aspeed_sgpio *gpio;
+ u32 nr_gpios, sgpio_freq, sgpio_clk_div;
+ int rc;
+ unsigned long apb_freq;
+
+ gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ gpio->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(gpio->base))
+ return PTR_ERR(gpio->base);
+
+ rc = of_property_read_u32(pdev->dev.of_node, "ngpios", &nr_gpios);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Could not read ngpios property\n");
+ return -EINVAL;
+ } else if (nr_gpios > MAX_NR_SGPIO) {
+ dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: %d\n",
+ MAX_NR_SGPIO, nr_gpios);
+ return -EINVAL;
+ }
+
+ rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Could not read bus-frequency property\n");
+ return -EINVAL;
+ }
+
+ gpio->pclk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(gpio->pclk)) {
+ dev_err(&pdev->dev, "devm_clk_get failed\n");
+ return PTR_ERR(gpio->pclk);
+ }
+
+ apb_freq = clk_get_rate(gpio->pclk);
+
+ /*
+ * From the datasheet,
+ * SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
+ * period = 2 * (GPIO254[31:16] + 1) / PCLK
+ * frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
+ * frequency = PCLK / (2 * (GPIO254[31:16] + 1))
+ * frequency * 2 * (GPIO254[31:16] + 1) = PCLK
+ * GPIO254[31:16] = PCLK / (frequency * 2) - 1
+ */
+ if (sgpio_freq == 0)
+ return -EINVAL;
+
+ sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
+
+ if (sgpio_clk_div > (1 << 16) - 1)
+ return -EINVAL;
+
+ iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
+ FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
+ ASPEED_SGPIO_ENABLE,
+ gpio->base + ASPEED_SGPIO_CTRL);
+
+ spin_lock_init(&gpio->lock);
+
+ gpio->chip.parent = &pdev->dev;
+ gpio->chip.ngpio = nr_gpios;
+ gpio->chip.direction_input = aspeed_sgpio_dir_in;
+ gpio->chip.direction_output = aspeed_sgpio_dir_out;
+ gpio->chip.get_direction = aspeed_sgpio_get_direction;
+ gpio->chip.request = NULL;
+ gpio->chip.free = NULL;
+ gpio->chip.get = aspeed_sgpio_get;
+ gpio->chip.set = aspeed_sgpio_set;
+ gpio->chip.set_config = NULL;
+ gpio->chip.label = dev_name(&pdev->dev);
+ gpio->chip.base = -1;
+
+ /* set all SGPIO pins as input (1). */
+ memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in));
+
+ rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+ if (rc < 0)
+ return rc;
+
+ return aspeed_sgpio_setup_irqs(gpio, pdev);
+}
+
+static struct platform_driver aspeed_sgpio_driver = {
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = aspeed_sgpio_of_table,
+ },
+};
+
+module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
+MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2 4/7] ASoC: dt-bindings: Document dl-mask property
From: Daniel Baluta @ 2019-07-29 20:27 UTC (permalink / raw)
To: Nicolin Chen
Cc: Daniel Baluta, Mark Brown, Lucas Stach, Mihai Serban, Linux-ALSA,
Viorel Suman, Timur Tabi, S.j. Wang, Angus Ainslie (Purism),
Takashi Iwai, dl-linux-imx, Pengutronix Kernel Team,
Fabio Estevam, Linux Kernel Mailing List, Devicetree List,
Rob Herring
In-Reply-To: <20190729201508.GB20594@Asurada-Nvidia.nvidia.com>
On Mon, Jul 29, 2019 at 11:15 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Sun, Jul 28, 2019 at 10:24:26PM +0300, Daniel Baluta wrote:
> > SAI supports up to 8 data lines. This property let the user
> > configure how many data lines should be used per transfer
> > direction (Tx/Rx).
>
> This sounds a bit less persuasive to me as we are adding a
> DT property that's used to describe a hardware connections
> and it would be probably better to mention that the mapping
> between the mask and the data lines could be more flexible
> than consecutive active data lines as you said previously.
>
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> > Documentation/devicetree/bindings/sound/fsl-sai.txt | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> > index 2e726b983845..2b38036a4883 100644
> > --- a/Documentation/devicetree/bindings/sound/fsl-sai.txt
> > +++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> > @@ -49,6 +49,13 @@ Optional properties:
> >
> > - big-endian : Boolean property, required if all the SAI
> > registers are big-endian rather than little-endian.
> > + - fsl,dl-mask : list of two integers (bitmask, first for RX, second
>
> I am leaving this naming to DT maintainer.
>
> > + for TX) representing enabled datalines. Bit 0
> > + represents first data line, bit 1 represents second
> > + data line and so on. Data line is enabled if
> > + corresponding bit is set to 1. By default, if property
> > + not present, only dataline 0 is enabled for both
> > + directions.
>
> To make this patch more convincing, could we add an example
> as well in the Example section of this binding file? Like:
> /* RX data lines 0/1 and TX data lines 0/2 are connected */
> fsl,dl-mask = <0x3 0x5>;
Sure, will add an example.
^ permalink raw reply
* Re: [PATCH v2 3/7] ASoC: fsl_sai: Add support to enable multiple data lines
From: Nicolin Chen @ 2019-07-29 20:21 UTC (permalink / raw)
To: Daniel Baluta
Cc: broonie, l.stach, mihai.serban, alsa-devel, viorel.suman, timur,
shengjiu.wang, angus, tiwai, linux-imx, kernel, festevam,
linux-kernel, devicetree, robh
In-Reply-To: <20190728192429.1514-4-daniel.baluta@nxp.com>
On Sun, Jul 28, 2019 at 10:24:25PM +0300, Daniel Baluta wrote:
> SAI supports up to 8 Rx/Tx data lines which can be enabled
> using TCE/RCE bits of TCR3/RCR3 registers.
>
> Data lines to be enabled are read from DT fsl,dl-mask property.
> By default (if no DT entry is provided) only data line 0 is enabled.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
> sound/soc/fsl/fsl_sai.c | 11 ++++++++++-
> sound/soc/fsl/fsl_sai.h | 4 +++-
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index 637b1d12a575..5e7cb7fd29f5 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -601,7 +601,7 @@ static int fsl_sai_startup(struct snd_pcm_substream *substream,
>
> regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx),
> FSL_SAI_CR3_TRCE_MASK,
> - FSL_SAI_CR3_TRCE);
> + FSL_SAI_CR3_TRCE(sai->soc_data->dl_mask[tx]);
>
> ret = snd_pcm_hw_constraint_list(substream->runtime, 0,
> SNDRV_PCM_HW_PARAM_RATE, &fsl_sai_rate_constraints);
> @@ -888,6 +888,15 @@ static int fsl_sai_probe(struct platform_device *pdev)
> }
> }
>
> + /*
> + * active data lines mask for TX/RX, defaults to 1 (only the first
> + * data line is enabled
> + */
> + sai->dl_mask[RX] = 1;
> + sai->dl_mask[TX] = 1;
> + of_property_read_u32_index(np, "fsl,dl-mask", RX, &sai->dl_mask[RX]);
> + of_property_read_u32_index(np, "fsl,dl-mask", TX, &sai->dl_mask[TX]);
Just curious what if we enable 8 data lines through DT bindings
while an audio file only has 1 or 2 channels. Will TRCE bits be
okay to stay with 8 data channels configurations? Btw, how does
DMA work for the data registers? ESAI has one entry at a fixed
address for all data channels while SAI seems to have different
data registers.
^ permalink raw reply
* Re: [alsa-devel] [PATCH v2 1/7] ASoC: fsl_sai: Add registers definition for multiple datalines
From: Mark Brown @ 2019-07-29 20:20 UTC (permalink / raw)
To: Daniel Baluta
Cc: Nicolin Chen, Daniel Baluta, Devicetree List, Linux-ALSA,
Pengutronix Kernel Team, Timur Tabi, Rob Herring, S.j. Wang,
Angus Ainslie (Purism), Takashi Iwai, Linux Kernel Mailing List,
dl-linux-imx, Viorel Suman, Fabio Estevam, Mihai Serban,
Lucas Stach
In-Reply-To: <CAEnQRZDmnAmgUkRGv3V5S7b5EnYTd2BO5NFuME2CfGb1=nAHzQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]
On Mon, Jul 29, 2019 at 10:57:43PM +0300, Daniel Baluta wrote:
> On Mon, Jul 29, 2019 at 10:42 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> > On Sun, Jul 28, 2019 at 10:24:23PM +0300, Daniel Baluta wrote:
> > > @@ -704,7 +711,14 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
> > > case FSL_SAI_TCR3:
> > > case FSL_SAI_TCR4:
> > > case FSL_SAI_TCR5:
> > > - case FSL_SAI_TFR:
> > > + case FSL_SAI_TFR0:
> > A tricky thing here is that those SAI instances on older SoC don't
> > support multi data lines physically, while seemly having registers
> > pre-defined. So your change doesn't sound doing anything wrong to
> > them at all, I am still wondering if it is necessary to apply them
> > to newer compatible only though, as for older compatibles of SAI,
> > these registers would be useless and confusing if being exposed.
> > What do you think?
> Yes, I thought about this too. But, I tried to keep the code as short
> as possible and technically it is not wrong. When 1 data line is supported
> for example application will only care about TDR0, TFR0, etc.
So long as it's safe to read the registers (you don't get a bus error or
anything) I'd say it's more trouble than it's worth to have separate
regmap configuations just for this. The main reasons for restricting
readability are where there's physical problems with doing the reads or
to keep the size of the debugfs files under control for usability and
performance reasons.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v4 0/3] Introduce Bandwidth OPPs for interconnects
From: Saravana Kannan @ 2019-07-29 20:16 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rob Herring, Mark Rutland, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Rafael J. Wysocki, Georgi Djakov, Vincent Guittot,
Sweeney, Sean, David Dai, adharmap, Rajendra Nayak, Sibi Sankar,
Bjorn Andersson, Evan Green, Android Kernel Team, Linux PM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
In-Reply-To: <20190729093545.kvnqxjkyx4nogddk@vireshk-i7>
On Mon, Jul 29, 2019 at 2:35 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 26-07-19, 16:15, Saravana Kannan wrote:
> > Interconnects and interconnect paths quantify their performance levels in
> > terms of bandwidth and not in terms of frequency. So similar to how we have
> > frequency based OPP tables in DT and in the OPP framework, we need
> > bandwidth OPP table support in DT and in the OPP framework.
> >
> > So with the DT bindings added in this patch series, the DT for a GPU
> > that does bandwidth voting from GPU to Cache and GPU to DDR would look
> > something like this:
> >
> > gpu_cache_opp_table: gpu_cache_opp_table {
> > compatible = "operating-points-v2";
> >
> > gpu_cache_3000: opp-3000 {
> > opp-peak-KBps = <3000000>;
> > opp-avg-KBps = <1000000>;
> > };
> > gpu_cache_6000: opp-6000 {
> > opp-peak-KBps = <6000000>;
> > opp-avg-KBps = <2000000>;
> > };
> > gpu_cache_9000: opp-9000 {
> > opp-peak-KBps = <9000000>;
> > opp-avg-KBps = <9000000>;
> > };
> > };
> >
> > gpu_ddr_opp_table: gpu_ddr_opp_table {
> > compatible = "operating-points-v2";
> >
> > gpu_ddr_1525: opp-1525 {
> > opp-peak-KBps = <1525000>;
> > opp-avg-KBps = <452000>;
> > };
> > gpu_ddr_3051: opp-3051 {
> > opp-peak-KBps = <3051000>;
> > opp-avg-KBps = <915000>;
> > };
> > gpu_ddr_7500: opp-7500 {
> > opp-peak-KBps = <7500000>;
> > opp-avg-KBps = <3000000>;
> > };
> > };
> >
> > gpu_opp_table: gpu_opp_table {
> > compatible = "operating-points-v2";
> > opp-shared;
> >
> > opp-200000000 {
> > opp-hz = /bits/ 64 <200000000>;
> > };
> > opp-400000000 {
> > opp-hz = /bits/ 64 <400000000>;
> > };
> > };
> >
> > gpu@7864000 {
> > ...
> > operating-points-v2 = <&gpu_opp_table>, <&gpu_cache_opp_table>, <&gpu_ddr_opp_table>;
> > ...
> > };
>
> One feedback I missed giving earlier. Will it be possible to get some
> user code merged along with this ? I want to make sure anything we add
> ends up getting used.
Sibi might be working on doing that for the SDM845 CPUfreq driver.
Georgi could also change his GPU driver use case to use this BW OPP
table and required-opps.
The problem is that people don't want to start using this until we
decide on the DT representation. So it's like a chicken and egg
situation.
-Saravana
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox