In a classic Greybus topology, there is an application processor, an SVC, and one or more modules, all connected to a UniPro bus. Most of the time, as the application processor doesn't have a UniPro interface, it is actually connected to a device acting as a bridge with the bus, and this bridge also acts as the SVC.
Sometimes, there is no UniPro bus at all, like for the BeaglePlay, which has the following topology:
+----+ +------------+ +--------+ | AP | <--- UART ---> | SVC/Bridge | <--- 802.15.4 ---> | Module | +----+ +------------+ +--------+ | | +--------+ `------------ 802.15.4 ---> | Module | +--------+
There are two main interesting aspects with Greybus: - the SVC protocol to monitor and configure the bus - other protocols, to expose module peripherals to the host
When the bus has a single module connected directly to the AP, then this module must also implement the SVC protocol:
+----+ +------------+ | AP | <--- bus ---> | SVC/Module | +----+ +------------+
The SVC doesn' really serve a purpose here, as there is no bus to manage, and adding its support only increase the complexity and the code size needed on memory-constrained devices.
The goal of this patchset is to let a single module expose some Greybus protocols without requiring the module to also implement SVC protocol. We call this mode "Point-To-Point". There are three main notable facts with the implementation of this patchset:
- most of the time, what this patchet does is just skipping calls that issue commands to the SVC, as they are not applicable without an SVC
- CPort ID allocation is a bit different as there is no SVC/Bridge to do translation between AP address space and interface address space, so the patchset forces allocation of AP CPort IDs that matches the ones found in interface's manifest
- enumeration of a module is normally started by a "Module Inserted" event issued by the SVC. As the SVC is absent, the host device driver must manually call a function to start the enumeration
We tested this patchset with the gb-beagleplay driver, slightly tweaked to only keep the HLDC over UART part of the driver, connected over UART to an EFR32MG24 running BeagleBoard's implementation of Greybus-Zephyr [1].
In the discussion to integrate this module into Zephyr [2] (it's currently as separate module not part of the main Zephyr code base), there seems to be interest in being able to have a single-node device on the bus without SVC [3]. If some features that were implemented by the SVC are missing, we can consider adding more callbacks to the gb_hd_driver structure at a later time, and let drivers decide how they want to support these features.
[1] https://github.com/beagleboard/greybus-zephyr [2] https://github.com/zephyrproject-rtos/zephyr/issues/98259 [3] https://github.com/zephyrproject-rtos/zephyr/issues/98259#issuecomment-35613...
Damien Riégel (8): greybus: add P2P interface type greybus: let gb_interface_create take additional p2p argument greybus: force CPort ID allocation in P2P mode greybus: split module creation function greybus: add function create module in P2P mode greybus: make host API work without SVC greybus: add function to create SVC-less host device greybus: add function to probe p2p module
drivers/greybus/connection.c | 15 ++++-- drivers/greybus/hd.c | 80 +++++++++++++++++++++++++++---- drivers/greybus/interface.c | 72 ++++++++++++++++++++-------- drivers/greybus/module.c | 55 ++++++++++++++++++--- include/linux/greybus/hd.h | 6 +++ include/linux/greybus/interface.h | 4 +- include/linux/greybus/module.h | 1 + 7 files changed, 193 insertions(+), 40 deletions(-)
Introduce a new Point-To-Point interface type. This type of interface is used to indicate that this interface belongs to a module that's connected in a point-to-point setup with a host device.
In this configuration, there is no SVC in the bus, so all code paths that use the SVC are skipped when dealing with this kind of interface.
Signed-off-by: Damien Riégel damien.riegel@silabs.com --- drivers/greybus/connection.c | 5 +++ drivers/greybus/interface.c | 52 +++++++++++++++++++++---------- include/linux/greybus/interface.h | 1 + 3 files changed, 42 insertions(+), 16 deletions(-)
diff --git a/drivers/greybus/connection.c b/drivers/greybus/connection.c index 9c88861986c..f9287f2f4c9 100644 --- a/drivers/greybus/connection.c +++ b/drivers/greybus/connection.c @@ -407,6 +407,8 @@ gb_connection_svc_connection_create(struct gb_connection *connection) return 0;
intf = connection->intf; + if (intf->type == GB_INTERFACE_TYPE_P2P) + return 0;
/* * Enable either E2EFC or CSD, unless no flow control is requested. @@ -441,6 +443,9 @@ gb_connection_svc_connection_destroy(struct gb_connection *connection) if (gb_connection_is_static(connection)) return;
+ if (connection->intf->type == GB_INTERFACE_TYPE_P2P) + return; + gb_svc_connection_destroy(connection->hd->svc, connection->hd->svc->ap_intf_id, connection->hd_cport_id, diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c index a0f3e942272..e96b58b211b 100644 --- a/drivers/greybus/interface.c +++ b/drivers/greybus/interface.c @@ -711,6 +711,9 @@ static int gb_interface_suspend(struct device *dev) if (ret) goto err_hibernate_abort;
+ if (intf->type == GB_INTERFACE_TYPE_P2P) + return 0; + ret = gb_interface_hibernate_link(intf); if (ret) return ret; @@ -733,16 +736,19 @@ static int gb_interface_suspend(struct device *dev) static int gb_interface_resume(struct device *dev) { struct gb_interface *intf = to_gb_interface(dev); - struct gb_svc *svc = intf->hd->svc; int ret;
- ret = gb_interface_refclk_set(intf, true); - if (ret) - return ret; + if (intf->type != GB_INTERFACE_TYPE_P2P) { + struct gb_svc *svc = intf->hd->svc;
- ret = gb_svc_intf_resume(svc, intf->interface_id); - if (ret) - return ret; + ret = gb_interface_refclk_set(intf, true); + if (ret) + return ret; + + ret = gb_svc_intf_resume(svc, intf->interface_id); + if (ret) + return ret; + }
ret = gb_control_resume(intf->control); if (ret) @@ -924,6 +930,11 @@ static int _gb_interface_activate(struct gb_interface *intf, if (intf->ejected || intf->removed) return -ENODEV;
+ if (intf->type == GB_INTERFACE_TYPE_P2P) { + *type = GB_INTERFACE_TYPE_P2P; + goto make_active; + } + ret = gb_interface_vsys_set(intf, true); if (ret) return ret; @@ -955,6 +966,7 @@ static int _gb_interface_activate(struct gb_interface *intf, if (ret) goto err_hibernate_link;
+make_active: intf->active = true;
trace_gb_interface_activate(intf); @@ -1012,6 +1024,7 @@ int gb_interface_activate(struct gb_interface *intf) case GB_INTERFACE_TYPE_GREYBUS: ret = _gb_interface_activate_es3_hack(intf, &type); break; + case GB_INTERFACE_TYPE_P2P: default: ret = _gb_interface_activate(intf, &type); } @@ -1049,11 +1062,13 @@ void gb_interface_deactivate(struct gb_interface *intf) if (intf->mode_switch) complete(&intf->mode_switch_completion);
- gb_interface_route_destroy(intf); - gb_interface_hibernate_link(intf); - gb_interface_unipro_set(intf, false); - gb_interface_refclk_set(intf, false); - gb_interface_vsys_set(intf, false); + if (intf->type != GB_INTERFACE_TYPE_P2P) { + gb_interface_route_destroy(intf); + gb_interface_hibernate_link(intf); + gb_interface_unipro_set(intf, false); + gb_interface_refclk_set(intf, false); + gb_interface_vsys_set(intf, false); + }
intf->active = false; } @@ -1072,10 +1087,12 @@ int gb_interface_enable(struct gb_interface *intf) int ret, size; void *manifest;
- ret = gb_interface_read_and_clear_init_status(intf); - if (ret) { - dev_err(&intf->dev, "failed to clear init status: %d\n", ret); - return ret; + if (intf->type != GB_INTERFACE_TYPE_P2P) { + ret = gb_interface_read_and_clear_init_status(intf); + if (ret) { + dev_err(&intf->dev, "failed to clear init status: %d\n", ret); + return ret; + } }
/* Establish control connection */ @@ -1230,6 +1247,9 @@ int gb_interface_add(struct gb_interface *intf) gb_interface_type_string(intf));
switch (intf->type) { + case GB_INTERFACE_TYPE_P2P: + dev_info(&intf->dev, "Added P2P interface\n"); + break; case GB_INTERFACE_TYPE_GREYBUS: dev_info(&intf->dev, "GMP VID=0x%08x, PID=0x%08x\n", intf->vendor_id, intf->product_id); diff --git a/include/linux/greybus/interface.h b/include/linux/greybus/interface.h index ce4def881e6..fed6356eb13 100644 --- a/include/linux/greybus/interface.h +++ b/include/linux/greybus/interface.h @@ -18,6 +18,7 @@ enum gb_interface_type { GB_INTERFACE_TYPE_DUMMY, GB_INTERFACE_TYPE_UNIPRO, GB_INTERFACE_TYPE_GREYBUS, + GB_INTERFACE_TYPE_P2P, };
#define GB_INTERFACE_QUIRK_NO_CPORT_FEATURES BIT(0)
Update the function that creates an interface to take an additional boolean argument to indicate if the interface is point to point or not.
If this argument is true, interface's type and ID are immediately set, and a different group of attributes is set on the interface device, as there is no way to fetch greybus attributes (vendor ID, product ID, serial number) or Unipro attributes (ddbl1 manufacturer and product ID) when SVC is absent, and so it doesn't make sense to expose them.
Signed-off-by: Damien Riégel damien.riegel@silabs.com --- drivers/greybus/interface.c | 20 ++++++++++++++++---- drivers/greybus/module.c | 2 +- include/linux/greybus/interface.h | 3 ++- 3 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c index e96b58b211b..aa2bd841977 100644 --- a/drivers/greybus/interface.c +++ b/drivers/greybus/interface.c @@ -687,6 +687,12 @@ static const struct attribute_group *interface_groups[] = { NULL };
+static const struct attribute_group *interface_groups_p2p[] = { + &interface_greybus_group, + &interface_common_group, + NULL +}; + static void gb_interface_release(struct device *dev) { struct gb_interface *intf = to_gb_interface(dev); @@ -790,7 +796,8 @@ const struct device_type greybus_interface_type = { * failure occurs due to memory exhaustion. */ struct gb_interface *gb_interface_create(struct gb_module *module, - u8 interface_id) + u8 interface_id, + bool p2p) { struct gb_host_device *hd = module->hd; struct gb_interface *intf; @@ -808,13 +815,18 @@ struct gb_interface *gb_interface_create(struct gb_module *module, INIT_WORK(&intf->mode_switch_work, gb_interface_mode_switch_work); init_completion(&intf->mode_switch_completion);
- /* Invalid device id to start with */ - intf->device_id = GB_INTERFACE_DEVICE_ID_BAD; + if (p2p) { + intf->device_id = GB_SVC_DEVICE_ID_MIN; + intf->type = GB_INTERFACE_TYPE_P2P; + } else { + /* Invalid device id to start with */ + intf->device_id = GB_INTERFACE_DEVICE_ID_BAD; + }
intf->dev.parent = &module->dev; intf->dev.bus = &greybus_bus_type; intf->dev.type = &greybus_interface_type; - intf->dev.groups = interface_groups; + intf->dev.groups = p2p ? interface_groups_p2p : interface_groups; intf->dev.dma_mask = module->dev.dma_mask; device_initialize(&intf->dev); dev_set_name(&intf->dev, "%s.%u", dev_name(&module->dev), diff --git a/drivers/greybus/module.c b/drivers/greybus/module.c index 7f7153a1dd6..4ed68550d32 100644 --- a/drivers/greybus/module.c +++ b/drivers/greybus/module.c @@ -113,7 +113,7 @@ struct gb_module *gb_module_create(struct gb_host_device *hd, u8 module_id, trace_gb_module_create(module);
for (i = 0; i < num_interfaces; ++i) { - intf = gb_interface_create(module, module_id + i); + intf = gb_interface_create(module, module_id + i, false); if (!intf) { dev_err(&module->dev, "failed to create interface %u\n", module_id + i); diff --git a/include/linux/greybus/interface.h b/include/linux/greybus/interface.h index fed6356eb13..dee42859ebe 100644 --- a/include/linux/greybus/interface.h +++ b/include/linux/greybus/interface.h @@ -70,7 +70,8 @@ struct gb_interface { #define to_gb_interface(d) container_of(d, struct gb_interface, dev)
struct gb_interface *gb_interface_create(struct gb_module *module, - u8 interface_id); + u8 interface_id, + bool p2p); int gb_interface_activate(struct gb_interface *intf); void gb_interface_deactivate(struct gb_interface *intf); int gb_interface_enable(struct gb_interface *intf);
In the classic Greybus topology, a connection is created by issuing a command to the SVC, with AP CPort ID, Interface ID, and Intf CPort ID, that way the SVC knows that when a message is issued by the host and targets "AP CPort ID", it must be routed to the "Intf CPort ID" of the right interface. The interface CPort ID is the ID as listed in the manifest.
For instance with GB BeaglePlay, the SVC does a kind of CPort ID translation: when it receives a message targeting "AP CPort ID", it forwards the message to the right interface and replaces the AP CPort ID by the Interface CPort ID. The same thing is done in gbridge, a userspace application that acts as an SVC.
When working in P2P mode, there is no SVC to translate CPort IDs, the host device speaks directly with an interface. For the interface to understand what the CPort ID means, the host device must use the same IDs as the ones present in the manifest. These changes to connection create functions force the CPort ID allocated by the host to match the interface.
Signed-off-by: Damien Riégel damien.riegel@silabs.com --- drivers/greybus/connection.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/greybus/connection.c b/drivers/greybus/connection.c index f9287f2f4c9..072b47cdd9b 100644 --- a/drivers/greybus/connection.c +++ b/drivers/greybus/connection.c @@ -235,7 +235,9 @@ gb_connection_create_static(struct gb_host_device *hd, u16 hd_cport_id, struct gb_connection * gb_connection_create_control(struct gb_interface *intf) { - return _gb_connection_create(intf->hd, -1, intf, NULL, 0, NULL, + int hd_cport_id = intf->type == GB_INTERFACE_TYPE_P2P ? 0 : -1; + + return _gb_connection_create(intf->hd, hd_cport_id, intf, NULL, 0, NULL, GB_CONNECTION_FLAG_CONTROL | GB_CONNECTION_FLAG_HIGH_PRIO); } @@ -245,8 +247,9 @@ gb_connection_create(struct gb_bundle *bundle, u16 cport_id, gb_request_handler_t handler) { struct gb_interface *intf = bundle->intf; + int hd_cport_id = intf->type == GB_INTERFACE_TYPE_P2P ? cport_id : -1;
- return _gb_connection_create(intf->hd, -1, intf, bundle, cport_id, + return _gb_connection_create(intf->hd, hd_cport_id, intf, bundle, cport_id, handler, 0); } EXPORT_SYMBOL_GPL(gb_connection_create); @@ -257,11 +260,12 @@ gb_connection_create_flags(struct gb_bundle *bundle, u16 cport_id, unsigned long flags) { struct gb_interface *intf = bundle->intf; + int hd_cport_id = intf->type == GB_INTERFACE_TYPE_P2P ? cport_id : -1;
if (WARN_ON_ONCE(flags & GB_CONNECTION_FLAG_CORE_MASK)) flags &= ~GB_CONNECTION_FLAG_CORE_MASK;
- return _gb_connection_create(intf->hd, -1, intf, bundle, cport_id, + return _gb_connection_create(intf->hd, hd_cport_id, intf, bundle, cport_id, handler, flags); } EXPORT_SYMBOL_GPL(gb_connection_create_flags);
The module creation function is split into two parts; one part for allocating and initializing the module structure itself, and a second part to create the interface that belong to this module. This is a preparatory step before introducing a function to create module in point-to-point mode.
Signed-off-by: Damien Riégel damien.riegel@silabs.com --- drivers/greybus/module.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/greybus/module.c b/drivers/greybus/module.c index 4ed68550d32..334aefb46b5 100644 --- a/drivers/greybus/module.c +++ b/drivers/greybus/module.c @@ -86,12 +86,12 @@ const struct device_type greybus_module_type = { .release = gb_module_release, };
-struct gb_module *gb_module_create(struct gb_host_device *hd, u8 module_id, - size_t num_interfaces) +static struct gb_module *__gb_module_create(struct gb_host_device *hd, + const struct attribute_group **groups, + u8 module_id, + size_t num_interfaces) { - struct gb_interface *intf; struct gb_module *module; - int i;
module = kzalloc(struct_size(module, interfaces, num_interfaces), GFP_KERNEL); @@ -105,13 +105,26 @@ struct gb_module *gb_module_create(struct gb_host_device *hd, u8 module_id, module->dev.parent = &hd->dev; module->dev.bus = &greybus_bus_type; module->dev.type = &greybus_module_type; - module->dev.groups = module_groups; + module->dev.groups = groups; module->dev.dma_mask = hd->dev.dma_mask; device_initialize(&module->dev); dev_set_name(&module->dev, "%d-%u", hd->bus_id, module_id);
trace_gb_module_create(module);
+ return module; +} + +struct gb_module *gb_module_create(struct gb_host_device *hd, u8 module_id, + size_t num_interfaces) +{ + struct gb_module *module = __gb_module_create(hd, module_groups, module_id, num_interfaces); + struct gb_interface *intf; + int i; + + if (!module) + return NULL; + for (i = 0; i < num_interfaces; ++i) { intf = gb_interface_create(module, module_id + i, false); if (!intf) {
Add a new function to a create a module in point-to-point mode. The number of interfaces is normally reported by the SVC in a "Module Inserted".
As there's no SVC in point-to-point mode, this function hardcodes the number of interface to 1. The "eject" attribute is also hidden, as "Module Eject" is an operation issued by the AP to the SVC to eject module with the given primary interface and doesn't make sense if SVC is absent.
Signed-off-by: Damien Riégel damien.riegel@silabs.com --- drivers/greybus/module.c | 30 ++++++++++++++++++++++++++++++ include/linux/greybus/module.h | 1 + 2 files changed, 31 insertions(+)
diff --git a/drivers/greybus/module.c b/drivers/greybus/module.c index 334aefb46b5..e628dc81b9a 100644 --- a/drivers/greybus/module.c +++ b/drivers/greybus/module.c @@ -72,6 +72,13 @@ static struct attribute *module_attrs[] = { }; ATTRIBUTE_GROUPS(module);
+static struct attribute *module_p2p_attrs[] = { + &dev_attr_module_id.attr, + &dev_attr_num_interfaces.attr, + NULL, +}; +ATTRIBUTE_GROUPS(module_p2p); + static void gb_module_release(struct device *dev) { struct gb_module *module = to_gb_module(dev); @@ -115,6 +122,29 @@ static struct gb_module *__gb_module_create(struct gb_host_device *hd, return module; }
+struct gb_module *gb_module_create_p2p(struct gb_host_device *hd) +{ + struct gb_module *module = __gb_module_create(hd, module_p2p_groups, 0, 1); + struct gb_interface *intf; + + if (!module) + return NULL; + + intf = gb_interface_create(module, 0, true); + if (!intf) { + dev_err(&module->dev, "failed to create P2P interface\n"); + put_device(&module->dev); + + module = NULL; + goto exit; + } + + module->interfaces[0] = intf; + +exit: + return module; +} + struct gb_module *gb_module_create(struct gb_host_device *hd, u8 module_id, size_t num_interfaces) { diff --git a/include/linux/greybus/module.h b/include/linux/greybus/module.h index 3efe2133acf..8fec21d6abf 100644 --- a/include/linux/greybus/module.h +++ b/include/linux/greybus/module.h @@ -27,6 +27,7 @@ struct gb_module { }; #define to_gb_module(d) container_of(d, struct gb_module, dev)
+struct gb_module *gb_module_create_p2p(struct gb_host_device *hd); struct gb_module *gb_module_create(struct gb_host_device *hd, u8 module_id, size_t num_interfaces); int gb_module_add(struct gb_module *module);
In preparation for an SVC-less topology, make host device API behaves nicely if an SVC is not allocated.
Signed-off-by: Damien Riégel damien.riegel@silabs.com --- drivers/greybus/hd.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/greybus/hd.c b/drivers/greybus/hd.c index 5de98d9177f..1e2f1f3a65f 100644 --- a/drivers/greybus/hd.c +++ b/drivers/greybus/hd.c @@ -45,11 +45,19 @@ static struct attribute *bus_attrs[] = { }; ATTRIBUTE_GROUPS(bus);
+static bool gb_hd_is_p2p(struct gb_host_device *hd) +{ + return !hd->svc; +} + int gb_hd_cport_reserve(struct gb_host_device *hd, u16 cport_id) { struct ida *id_map = &hd->cport_id_map; int ret;
+ if (gb_hd_is_p2p(hd)) + return -EPERM; + ret = ida_alloc_range(id_map, cport_id, cport_id, GFP_KERNEL); if (ret < 0) { dev_err(&hd->dev, "failed to reserve cport %u\n", cport_id); @@ -64,6 +72,9 @@ void gb_hd_cport_release_reserved(struct gb_host_device *hd, u16 cport_id) { struct ida *id_map = &hd->cport_id_map;
+ if (gb_hd_is_p2p(hd)) + return; + ida_free(id_map, cport_id); } EXPORT_SYMBOL_GPL(gb_hd_cport_release_reserved); @@ -205,10 +216,12 @@ int gb_hd_add(struct gb_host_device *hd) if (ret) return ret;
- ret = gb_svc_add(hd->svc); - if (ret) { - device_del(&hd->dev); - return ret; + if (!gb_hd_is_p2p(hd)) { + ret = gb_svc_add(hd->svc); + if (ret) { + device_del(&hd->dev); + return ret; + } }
trace_gb_hd_add(hd); @@ -225,7 +238,8 @@ void gb_hd_del(struct gb_host_device *hd) * Tear down the svc and flush any on-going hotplug processing before * removing the remaining interfaces. */ - gb_svc_del(hd->svc); + if (!gb_hd_is_p2p(hd)) + gb_svc_del(hd->svc);
device_del(&hd->dev); } @@ -233,7 +247,8 @@ EXPORT_SYMBOL_GPL(gb_hd_del);
void gb_hd_shutdown(struct gb_host_device *hd) { - gb_svc_del(hd->svc); + if (!gb_hd_is_p2p(hd)) + gb_svc_del(hd->svc); } EXPORT_SYMBOL_GPL(gb_hd_shutdown);
This exports a new symbol, gb_hd_create_p2p, which let users create a host device that doesn't have an SVC. As other gb_hd_* functions already support handling a host device with a NULL 'svc' pointer, this is safe to expose.
Signed-off-by: Damien Riégel damien.riegel@silabs.com --- drivers/greybus/hd.c | 23 +++++++++++++++++++---- include/linux/greybus/hd.h | 4 ++++ 2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/drivers/greybus/hd.c b/drivers/greybus/hd.c index 1e2f1f3a65f..2fc9fbe987f 100644 --- a/drivers/greybus/hd.c +++ b/drivers/greybus/hd.c @@ -132,10 +132,10 @@ const struct device_type greybus_hd_type = { .release = gb_hd_release, };
-struct gb_host_device *gb_hd_create(struct gb_hd_driver *driver, - struct device *parent, - size_t buffer_size_max, - size_t num_cports) +struct gb_host_device *gb_hd_create_p2p(struct gb_hd_driver *driver, + struct device *parent, + size_t buffer_size_max, + size_t num_cports) { struct gb_host_device *hd; int ret; @@ -197,6 +197,21 @@ struct gb_host_device *gb_hd_create(struct gb_hd_driver *driver,
trace_gb_hd_create(hd);
+ return hd; +} +EXPORT_SYMBOL_GPL(gb_hd_create_p2p); + +struct gb_host_device *gb_hd_create(struct gb_hd_driver *driver, + struct device *parent, + size_t buffer_size_max, + size_t num_cports) +{ + struct gb_host_device *hd; + + hd = gb_hd_create_p2p(driver, parent, buffer_size_max, num_cports); + if (IS_ERR(hd)) + return hd; + hd->svc = gb_svc_create(hd); if (!hd->svc) { dev_err(&hd->dev, "failed to create svc\n"); diff --git a/include/linux/greybus/hd.h b/include/linux/greybus/hd.h index 718e2857054..28ee7fca1d1 100644 --- a/include/linux/greybus/hd.h +++ b/include/linux/greybus/hd.h @@ -72,6 +72,10 @@ struct gb_host_device *gb_hd_create(struct gb_hd_driver *driver, struct device *parent, size_t buffer_size_max, size_t num_cports); +struct gb_host_device *gb_hd_create_p2p(struct gb_hd_driver *driver, + struct device *parent, + size_t buffer_size_max, + size_t num_cports); int gb_hd_add(struct gb_host_device *hd); void gb_hd_del(struct gb_host_device *hd); void gb_hd_shutdown(struct gb_host_device *hd);
In a normal Greybus topology, the enumeration of an interface would start with a "Module Inserted" event, issued by the SVC. In a point-to-point configuration, the SVC is absent and the host device must enumerate the module manually.
In order to do so, export a new function that lets the host device driver start the enumeration. Note that this function issues commands to the module in a blocking way and should not be called in a probe function, but should rather be called from a workqueue.
Signed-off-by: Damien Riégel damien.riegel@silabs.com --- drivers/greybus/hd.c | 30 ++++++++++++++++++++++++++++++ include/linux/greybus/hd.h | 2 ++ 2 files changed, 32 insertions(+)
diff --git a/drivers/greybus/hd.c b/drivers/greybus/hd.c index 2fc9fbe987f..165775b6b8b 100644 --- a/drivers/greybus/hd.c +++ b/drivers/greybus/hd.c @@ -132,6 +132,36 @@ const struct device_type greybus_hd_type = { .release = gb_hd_release, };
+int gb_hd_p2p_probe_module(struct gb_host_device *hd) +{ + struct gb_module *module; + int ret; + + if (!gb_hd_is_p2p(hd)) + return -EOPNOTSUPP; + + /* In P2P mode, only one module is supported. */ + if (!list_empty(&hd->modules)) + return -EBUSY; + + module = gb_module_create_p2p(hd); + if (!module) { + dev_err(&hd->dev, "failed to create module\n"); + return -ENOMEM; + } + + ret = gb_module_add(module); + if (ret) { + gb_module_put(module); + return ret; + } + + list_add(&module->hd_node, &hd->modules); + + return 0; +} +EXPORT_SYMBOL_GPL(gb_hd_p2p_probe_module); + struct gb_host_device *gb_hd_create_p2p(struct gb_hd_driver *driver, struct device *parent, size_t buffer_size_max, diff --git a/include/linux/greybus/hd.h b/include/linux/greybus/hd.h index 28ee7fca1d1..096b9642d11 100644 --- a/include/linux/greybus/hd.h +++ b/include/linux/greybus/hd.h @@ -83,6 +83,8 @@ void gb_hd_put(struct gb_host_device *hd); int gb_hd_output(struct gb_host_device *hd, void *req, u16 size, u8 cmd, bool in_irq);
+int gb_hd_p2p_probe_module(struct gb_host_device *hd); + int gb_hd_init(void); void gb_hd_exit(void);
On Tue, Dec 23, 2025 at 01:31:34PM -0500, Damien Riégel wrote:
In a classic Greybus topology, there is an application processor, an SVC, and one or more modules, all connected to a UniPro bus. Most of the time, as the application processor doesn't have a UniPro interface, it is actually connected to a device acting as a bridge with the bus, and this bridge also acts as the SVC.
Sometimes, there is no UniPro bus at all, like for the BeaglePlay, which has the following topology:
+----+ +------------+ +--------+ | AP | <--- UART ---> | SVC/Bridge | <--- 802.15.4 ---> | Module | +----+ +------------+ +--------+ | | +--------+ `------------ 802.15.4 ---> | Module | +--------+There are two main interesting aspects with Greybus:
- the SVC protocol to monitor and configure the bus
- other protocols, to expose module peripherals to the host
When the bus has a single module connected directly to the AP, then this module must also implement the SVC protocol:
+----+ +------------+ | AP | <--- bus ---> | SVC/Module | +----+ +------------+The SVC doesn' really serve a purpose here, as there is no bus to manage, and adding its support only increase the complexity and the code size needed on memory-constrained devices.
Exactly how much memory does a "single point" SVC driver take, vs. adding new P2P functionality everywhere in the code like you just did? Finding that out would be good first before worrying about adding another type of "bus" here please.
The goal of this patchset is to let a single module expose some Greybus protocols without requiring the module to also implement SVC protocol. We call this mode "Point-To-Point". There are three main notable facts with the implementation of this patchset:
- most of the time, what this patchet does is just skipping calls that issue commands to the SVC, as they are not applicable without an SVC
Great, make a SVC that just ignores them :)
- CPort ID allocation is a bit different as there is no SVC/Bridge to do translation between AP address space and interface address space, so the patchset forces allocation of AP CPort IDs that matches the ones found in interface's manifest
Again, a simple SCV would make this not needed.
- enumeration of a module is normally started by a "Module Inserted" event issued by the SVC. As the SVC is absent, the host device driver must manually call a function to start the enumeration
I'd prefer again, to have that in the SVC you are using.
We tested this patchset with the gb-beagleplay driver, slightly tweaked to only keep the HLDC over UART part of the driver, connected over UART to an EFR32MG24 running BeagleBoard's implementation of Greybus-Zephyr [1].
In the discussion to integrate this module into Zephyr [2] (it's currently as separate module not part of the main Zephyr code base), there seems to be interest in being able to have a single-node device on the bus without SVC [3]. If some features that were implemented by the SVC are missing, we can consider adding more callbacks to the gb_hd_driver structure at a later time, and let drivers decide how they want to support these features.
I can understand if you want to be a greybus host running zephyr that this might make sense, as it lets you not even have to write any SVC logic, but for Linux here, I think the simplicity makes more sense (i.e. everything goes through the same data paths, no multiple test paths that need to always be exercised.)
So I'd prefer not to do this, just try to make a simple svc module and see if that works instead.
thanks,
greg k-h
On Fri Jan 16, 2026 at 10:09 AM EST, Greg Kroah-Hartman wrote: [...]
The SVC doesn' really serve a purpose here, as there is no bus to manage, and adding its support only increase the complexity and the code size needed on memory-constrained devices.
Exactly how much memory does a "single point" SVC driver take, vs. adding new P2P functionality everywhere in the code like you just did? Finding that out would be good first before worrying about adding another type of "bus" here please.
Sorry for not giving numbers in the cover letter. We measured 10kB of code size difference (which may be on the higher end of the spectrum) and in Zephyr [1] they measured a 2kB difference (which is probably on the lower end considering the implementation is pretty barebone).
The goal of this patchset is to let a single module expose some Greybus protocols without requiring the module to also implement SVC protocol. We call this mode "Point-To-Point". There are three main notable facts with the implementation of this patchset:
- most of the time, what this patchet does is just skipping calls that issue commands to the SVC, as they are not applicable without an SVC
Great, make a SVC that just ignores them :)
We can't really *completely* ignore them, we need to respond in a way that lets Linux think this is a legit SVC, or it will stop probing. But that's mostly what we already do and what they do in Zephyr [2], only connection create/destroy really matters to map CPort ID between AP and Interface.
- CPort ID allocation is a bit different as there is no SVC/Bridge to do translation between AP address space and interface address space, so the patchset forces allocation of AP CPort IDs that matches the ones found in interface's manifest
Again, a simple SCV would make this not needed.
- enumeration of a module is normally started by a "Module Inserted" event issued by the SVC. As the SVC is absent, the host device driver must manually call a function to start the enumeration
I'd prefer again, to have that in the SVC you are using.
We tested this patchset with the gb-beagleplay driver, slightly tweaked to only keep the HLDC over UART part of the driver, connected over UART to an EFR32MG24 running BeagleBoard's implementation of Greybus-Zephyr [1].
In the discussion to integrate this module into Zephyr [2] (it's currently as separate module not part of the main Zephyr code base), there seems to be interest in being able to have a single-node device on the bus without SVC [3]. If some features that were implemented by the SVC are missing, we can consider adding more callbacks to the gb_hd_driver structure at a later time, and let drivers decide how they want to support these features.
I can understand if you want to be a greybus host running zephyr that this might make sense, as it lets you not even have to write any SVC logic, but for Linux here, I think the simplicity makes more sense (i.e. everything goes through the same data paths, no multiple test paths that need to always be exercised.)
So I'd prefer not to do this, just try to make a simple svc module and see if that works instead.
That's what we currently have and it works, but it comes at the cost of more flash usage. That being said, I understand the concern of wanting to keep the current implementation simple and have only one data path. I think the benefits are not big enough in terms of code size savings to justify pushing this patchset further, so I will just leave it as is. It's out there if someone wants to try and improve it to be less intrusive :)
thanks, damien
[1] https://github.com/zephyrproject-rtos/zephyr/issues/98259#issuecomment-37745... [2] https://github.com/beagleboard/greybus-zephyr/blob/main/subsys/greybus/svc.c
On Tue, Jan 27, 2026 at 03:23:44PM -0500, Damien Riégel wrote:
On Fri Jan 16, 2026 at 10:09 AM EST, Greg Kroah-Hartman wrote: [...]
The SVC doesn' really serve a purpose here, as there is no bus to manage, and adding its support only increase the complexity and the code size needed on memory-constrained devices.
Exactly how much memory does a "single point" SVC driver take, vs. adding new P2P functionality everywhere in the code like you just did? Finding that out would be good first before worrying about adding another type of "bus" here please.
Sorry for not giving numbers in the cover letter. We measured 10kB of code size difference (which may be on the higher end of the spectrum) and in Zephyr [1] they measured a 2kB difference (which is probably on the lower end considering the implementation is pretty barebone).
Please submit your "empty" SVC driver, 10kb seems too big to me, I think we can slim it down, or at the very least, see what is making it "large" and potentially work to reduce that.
For Zephyr, I thought it was 1kb in the github comments, but still 2kb seems reasonable :)
thanks,
greg k-h
On Wed Jan 28, 2026 at 1:04 AM EST, Greg Kroah-Hartman wrote:
On Tue, Jan 27, 2026 at 03:23:44PM -0500, Damien Riégel wrote:
On Fri Jan 16, 2026 at 10:09 AM EST, Greg Kroah-Hartman wrote: [...]
The SVC doesn' really serve a purpose here, as there is no bus to manage, and adding its support only increase the complexity and the code size needed on memory-constrained devices.
Exactly how much memory does a "single point" SVC driver take, vs. adding new P2P functionality everywhere in the code like you just did? Finding that out would be good first before worrying about adding another type of "bus" here please.
Sorry for not giving numbers in the cover letter. We measured 10kB of code size difference (which may be on the higher end of the spectrum) and in Zephyr [1] they measured a 2kB difference (which is probably on the lower end considering the implementation is pretty barebone).
Please submit your "empty" SVC driver, 10kb seems too big to me, I think we can slim it down, or at the very least, see what is making it "large" and potentially work to reduce that.
The comparison I did originally was pretty naive. When I tested without the SVC, the C files were not even compiled. What I didn't account for is that our SVC currently logs pretty extensively, and guess where these log strings are stored...
After removing the logs, I could compare apples to apples: - with SVC: 59,272 B - without SVC: 57,556 B
So not even 2kB difference.
For Zephyr, I thought it was 1kb in the github comments, but still 2kb seems reasonable :)
The last comment in the thread says 2kB, but it's in the same ballpark.
So I think we have our number, an empty SVC would be in the 1kB to 2kB range.