| Message ID | 20240710173153.4060457-4-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make SCMI transport as standalone drivers | expand |
> Subject: [PATCH v2 3/8] firmware: arm_scmi: Add support for > standalone transport drivers > > Extend the core SCMI stack with structures and methods to allow for > transports to be split out as standalone drivers, while still supporting > old style transports, defined as built into the SCMI core stack. > > No functional change. > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- > NOTE: old style transport support will be removed later in this series. > > v1 --> v2 > - fixed comit message > --- > drivers/firmware/arm_scmi/common.h | 84 > ++++++++++++++++++++++++++++++ > drivers/firmware/arm_scmi/driver.c | 44 +++++++++++++++- > drivers/firmware/arm_scmi/msg.c | 5 ++ > drivers/firmware/arm_scmi/shmem.c | 5 ++ > 4 files changed, 136 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/common.h > b/drivers/firmware/arm_scmi/common.h > index 8e5751aaa600..4af06810eb39 100644 > --- a/drivers/firmware/arm_scmi/common.h > +++ b/drivers/firmware/arm_scmi/common.h > @@ -349,6 +349,8 @@ struct scmi_shared_mem_operations { > bool tx, struct resource *res); }; > > +const struct scmi_shared_mem_operations > +*scmi_shared_mem_operations_get(void); > + > /* declarations for message passing transports */ struct > scmi_msg_payld; > > @@ -376,6 +378,88 @@ struct scmi_message_operations { > size_t max_len, struct scmi_xfer > *xfer); }; > > +const struct scmi_message_operations > +*scmi_message_operations_get(void); > + > +/** > + * struct scmi_transport_core_operations - Transpoert core > operations > + * > + * @bad_message_trace: An helper to report a > malformed/unexpected > +message > + * @rx_callback: Callback to report received messages > + * @shmem: Datagram operations for shared memory based > transports > + * @msg: Datagram operations for message based transports */ > struct > +scmi_transport_core_operations { > + void (*bad_message_trace)(struct scmi_chan_info *cinfo, > + u32 msg_hdr, enum scmi_bad_msg > err); > + void (*rx_callback)(struct scmi_chan_info *cinfo, u32 msg_hdr, > + void *priv); > + const struct scmi_shared_mem_operations *shmem; > + const struct scmi_message_operations *msg; }; > + > +/** > + * struct scmi_transport - A structure representing a configured > +transport > + * > + * @supplier: Device representimng the transport and acting as a > supplier for > + * the core SCMI stack > + * @desc: Transport descriptor > + * @core_ops: A pointer to a pointer used by the core SCMI stack to > make the > + * core transport operations accessible to the transports. > + */ > +struct scmi_transport { > + struct device *supplier; > + const struct scmi_desc *desc; > + struct scmi_transport_core_operations **core_ops; }; > + > +#define DEFINE_SCMI_TRANSPORT_DRIVER(__trans, __match_table, > __core_ptr)\ > +static int __trans##_probe(struct platform_device *pdev) \ > +{ > \ > + struct scmi_transport *scmi_trans; > \ > + struct platform_device *scmi_pdev; > \ > + struct device *dev = &pdev->dev; \ > + > \ > + scmi_trans = devm_kzalloc(dev, sizeof(*scmi_trans), > GFP_KERNEL);\ > + if (!scmi_trans) \ > + return -ENOMEM; > \ > + > \ > + scmi_pdev = devm_kzalloc(dev, sizeof(*scmi_pdev), > GFP_KERNEL); \ > + if (!scmi_pdev) > \ > + return -ENOMEM; > \ > + > \ > + scmi_trans->supplier = dev; > \ > + scmi_trans->desc = &__trans##_desc; > \ > + scmi_trans->core_ops = __core_ptr; > \ > + > \ > + scmi_pdev->name = "arm-scmi"; > \ > + scmi_pdev->id = PLATFORM_DEVID_AUTO; > \ > + scmi_pdev->dev.platform_data = scmi_trans; > \ > + > \ > + device_set_of_node_from_dev(&scmi_pdev->dev, dev); > \ > + > \ > + dev_set_drvdata(dev, scmi_pdev); > \ > + > \ > + return platform_device_register(scmi_pdev); > \ > +} > \ > + > \ > +static void __trans##_remove(struct platform_device *pdev) > \ > +{ > \ > + struct platform_device *scmi_pdev; > \ > + > \ > + scmi_pdev = dev_get_drvdata(&pdev->dev); > \ > + > \ > + platform_device_unregister(scmi_pdev); > \ > +} > \ > + > \ > +static struct platform_driver __trans##_driver = { \ > + .driver = { > \ > + .name = #__trans "_transport", \ > + .of_match_table = __match_table, > \ > + }, > \ > + .probe = __trans##_probe, > \ > + .remove_new = __trans##_remove, > \ > +} > + > extern const struct scmi_shared_mem_operations scmi_shmem_ops; > extern const struct scmi_message_operations scmi_msg_ops; > > diff --git a/drivers/firmware/arm_scmi/driver.c > b/drivers/firmware/arm_scmi/driver.c > index 6b6957f4743f..a1892d4d8c69 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -194,6 +194,11 @@ struct scmi_info { > #define bus_nb_to_scmi_info(nb) container_of(nb, struct > scmi_info, bus_nb) > #define req_nb_to_scmi_info(nb) container_of(nb, struct > scmi_info, dev_req_nb) > > +static struct scmi_transport_core_operations scmi_trans_core_ops = { > + .bad_message_trace = scmi_bad_message_trace, > + .rx_callback = scmi_rx_callback, > +}; > + > static unsigned long > scmi_vendor_protocol_signature(unsigned int protocol_id, char > *vendor_id, > char *sub_vendor_id, u32 impl_ver) @@ > -2950,6 +2955,28 @@ static int scmi_debugfs_raw_mode_setup(struct > scmi_info *info) > return ret; > } > > +static const struct scmi_desc *scmi_transport_lookup(struct device > +*dev) { > + struct scmi_transport *trans; > + > + trans = dev_get_platdata(dev); > + if (!trans || !trans->desc || !trans->supplier || !trans- > >core_ops) > + return NULL; > + > + if (!device_link_add(dev, trans->supplier, > DL_FLAG_AUTOREMOVE_CONSUMER)) { Just wonder why this is needed? > + dev_err(dev, > + "Adding link to supplier transport device > failed\n"); > + return NULL; > + } > + > + /* Provide core transport ops */ > + *trans->core_ops = &scmi_trans_core_ops; > + > + dev_info(dev, "Using %s\n", dev_driver_string(trans->supplier)); > + > + return trans->desc; > +} > + > static int scmi_probe(struct platform_device *pdev) { > int ret; > @@ -2962,8 +2989,14 @@ static int scmi_probe(struct > platform_device *pdev) > struct device_node *child, *np = dev->of_node; > > desc = of_device_get_match_data(dev); > - if (!desc) > - return -EINVAL; > + if (!desc) { > + desc = scmi_transport_lookup(dev); from the code, It is not actually a lookup operation. How about scmi_transport_setup? Regards, Peng.
On Thu, Jul 11, 2024 at 12:54:51PM +0000, Peng Fan wrote: > > Subject: [PATCH v2 3/8] firmware: arm_scmi: Add support for > > standalone transport drivers > > Hi Peng, > > Extend the core SCMI stack with structures and methods to allow for > > transports to be split out as standalone drivers, while still supporting > > old style transports, defined as built into the SCMI core stack. > > > > No functional change. > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > --- > > NOTE: old style transport support will be removed later in this series. > > > > v1 --> v2 > > - fixed comit message > > --- > > drivers/firmware/arm_scmi/common.h | 84 > > ++++++++++++++++++++++++++++++ > > drivers/firmware/arm_scmi/driver.c | 44 +++++++++++++++- > > drivers/firmware/arm_scmi/msg.c | 5 ++ > > drivers/firmware/arm_scmi/shmem.c | 5 ++ > > 4 files changed, 136 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/firmware/arm_scmi/common.h > > b/drivers/firmware/arm_scmi/common.h > > index 8e5751aaa600..4af06810eb39 100644 > > --- a/drivers/firmware/arm_scmi/common.h > > +++ b/drivers/firmware/arm_scmi/common.h > > @@ -349,6 +349,8 @@ struct scmi_shared_mem_operations { > > bool tx, struct resource *res); }; > > > > +const struct scmi_shared_mem_operations > > +*scmi_shared_mem_operations_get(void); > > + > > /* declarations for message passing transports */ struct > > scmi_msg_payld; > > > > @@ -376,6 +378,88 @@ struct scmi_message_operations { > > size_t max_len, struct scmi_xfer > > *xfer); }; > > > > +const struct scmi_message_operations > > +*scmi_message_operations_get(void); > > + > > +/** > > + * struct scmi_transport_core_operations - Transpoert core > > operations > > + * > > + * @bad_message_trace: An helper to report a > > malformed/unexpected > > +message > > + * @rx_callback: Callback to report received messages > > + * @shmem: Datagram operations for shared memory based > > transports > > + * @msg: Datagram operations for message based transports */ > > struct > > +scmi_transport_core_operations { > > + void (*bad_message_trace)(struct scmi_chan_info *cinfo, > > + u32 msg_hdr, enum scmi_bad_msg > > err); > > + void (*rx_callback)(struct scmi_chan_info *cinfo, u32 msg_hdr, > > + void *priv); > > + const struct scmi_shared_mem_operations *shmem; > > + const struct scmi_message_operations *msg; }; > > + > > +/** > > + * struct scmi_transport - A structure representing a configured > > +transport > > + * > > + * @supplier: Device representimng the transport and acting as a > > supplier for > > + * the core SCMI stack > > + * @desc: Transport descriptor > > + * @core_ops: A pointer to a pointer used by the core SCMI stack to > > make the > > + * core transport operations accessible to the transports. > > + */ > > +struct scmi_transport { > > + struct device *supplier; > > + const struct scmi_desc *desc; > > + struct scmi_transport_core_operations **core_ops; }; > > + > > +#define DEFINE_SCMI_TRANSPORT_DRIVER(__trans, __match_table, > > __core_ptr)\ > > +static int __trans##_probe(struct platform_device *pdev) \ > > +{ > > \ > > + struct scmi_transport *scmi_trans; > > \ > > + struct platform_device *scmi_pdev; > > \ > > + struct device *dev = &pdev->dev; \ > > + > > \ > > + scmi_trans = devm_kzalloc(dev, sizeof(*scmi_trans), > > GFP_KERNEL);\ > > + if (!scmi_trans) \ > > + return -ENOMEM; > > \ > > + > > \ > > + scmi_pdev = devm_kzalloc(dev, sizeof(*scmi_pdev), > > GFP_KERNEL); \ > > + if (!scmi_pdev) > > \ > > + return -ENOMEM; > > \ > > + > > \ > > + scmi_trans->supplier = dev; > > \ > > + scmi_trans->desc = &__trans##_desc; > > \ > > + scmi_trans->core_ops = __core_ptr; > > \ > > + > > \ > > + scmi_pdev->name = "arm-scmi"; > > \ > > + scmi_pdev->id = PLATFORM_DEVID_AUTO; > > \ > > + scmi_pdev->dev.platform_data = scmi_trans; > > \ > > + > > \ > > + device_set_of_node_from_dev(&scmi_pdev->dev, dev); > > \ > > + > > \ > > + dev_set_drvdata(dev, scmi_pdev); > > \ > > + > > \ > > + return platform_device_register(scmi_pdev); > > \ > > +} > > \ > > + > > \ > > +static void __trans##_remove(struct platform_device *pdev) > > \ > > +{ > > \ > > + struct platform_device *scmi_pdev; > > \ > > + > > \ > > + scmi_pdev = dev_get_drvdata(&pdev->dev); > > \ > > + > > \ > > + platform_device_unregister(scmi_pdev); > > \ > > +} > > \ > > + > > \ > > +static struct platform_driver __trans##_driver = { \ > > + .driver = { > > \ > > + .name = #__trans "_transport", \ > > + .of_match_table = __match_table, > > \ > > + }, > > \ > > + .probe = __trans##_probe, > > \ > > + .remove_new = __trans##_remove, > > \ > > +} > > + > > extern const struct scmi_shared_mem_operations scmi_shmem_ops; > > extern const struct scmi_message_operations scmi_msg_ops; > > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > b/drivers/firmware/arm_scmi/driver.c > > index 6b6957f4743f..a1892d4d8c69 100644 > > --- a/drivers/firmware/arm_scmi/driver.c > > +++ b/drivers/firmware/arm_scmi/driver.c > > @@ -194,6 +194,11 @@ struct scmi_info { > > #define bus_nb_to_scmi_info(nb) container_of(nb, struct > > scmi_info, bus_nb) > > #define req_nb_to_scmi_info(nb) container_of(nb, struct > > scmi_info, dev_req_nb) > > > > +static struct scmi_transport_core_operations scmi_trans_core_ops = { > > + .bad_message_trace = scmi_bad_message_trace, > > + .rx_callback = scmi_rx_callback, > > +}; > > + > > static unsigned long > > scmi_vendor_protocol_signature(unsigned int protocol_id, char > > *vendor_id, > > char *sub_vendor_id, u32 impl_ver) @@ > > -2950,6 +2955,28 @@ static int scmi_debugfs_raw_mode_setup(struct > > scmi_info *info) > > return ret; > > } > > > > +static const struct scmi_desc *scmi_transport_lookup(struct device > > +*dev) { > > + struct scmi_transport *trans; > > + > > + trans = dev_get_platdata(dev); > > + if (!trans || !trans->desc || !trans->supplier || !trans- > > >core_ops) > > + return NULL; > > + > > + if (!device_link_add(dev, trans->supplier, > > DL_FLAG_AUTOREMOVE_CONSUMER)) { > > Just wonder why this is needed? Oh, that is absolutely needed, because it unleashes the power of device_link ! (..and I jus discovered this myself only recently :P ...)..if I got your question right.... ...and that, together with the AUTOREMOVE_CONSUMER flag, means in other words that the transport device is linked as a supplier to the consumer SCMI core stack devices, and as a consequence you are absolutely free to abruptly unload/unbind the transport driver at any time and be sure that before that can happen the full SCMI stack will be FULLY UNBOUND too...so stopping any SCMI communication before the transport module is unloaded/unbound.... ...and this, more importantly, happens without ME having to write ANY line of code :P....just thanks to the device_link core magic... ...or at least this is my understanding and > > + dev_err(dev, > > + "Adding link to supplier transport device > > failed\n"); > > + return NULL; > > + } > > + > > + /* Provide core transport ops */ > > + *trans->core_ops = &scmi_trans_core_ops; > > + > > + dev_info(dev, "Using %s\n", dev_driver_string(trans->supplier)); > > + > > + return trans->desc; > > +} > > + > > static int scmi_probe(struct platform_device *pdev) { > > int ret; > > @@ -2962,8 +2989,14 @@ static int scmi_probe(struct > > platform_device *pdev) > > struct device_node *child, *np = dev->of_node; > > > > desc = of_device_get_match_data(dev); > > - if (!desc) > > - return -EINVAL; > > + if (!desc) { > > + desc = scmi_transport_lookup(dev); > > from the code, It is not actually a lookup operation. > How about scmi_transport_setup? > Oh, yes...funny thing is that it is how exactly I DID called this function initially....then I spotted that there was already a similar __scmi_trancport_setup() helper in the legacy code that I removed, so I tried to use another more distinct name....I could go back to scmi_transport_setup indeed.. Thanks, Cristian
Hi Cristian, Few nitpicking comments. On Wednesday, July 10, 2024, Cristian Marussi wrote: > Extend the core SCMI stack with structures and methods to allow for > transports to be split out as standalone drivers, while still supporting > old style transports, defined as built into the SCMI core stack. > > No functional change. > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- > NOTE: old style transport support will be removed later in this series. > > v1 --> v2 > - fixed comit message > --- > drivers/firmware/arm_scmi/common.h | 84 ++++++++++++++++++++++++++++++ > drivers/firmware/arm_scmi/driver.c | 44 +++++++++++++++- > drivers/firmware/arm_scmi/msg.c | 5 ++ > drivers/firmware/arm_scmi/shmem.c | 5 ++ > 4 files changed, 136 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > index 8e5751aaa600..4af06810eb39 100644 > --- a/drivers/firmware/arm_scmi/common.h > +++ b/drivers/firmware/arm_scmi/common.h > @@ -349,6 +349,8 @@ struct scmi_shared_mem_operations { > bool tx, struct resource *res); > }; > > +const struct scmi_shared_mem_operations *scmi_shared_mem_operations_get(void); > + > /* declarations for message passing transports */ > struct scmi_msg_payld; > > @@ -376,6 +378,88 @@ struct scmi_message_operations { > size_t max_len, struct scmi_xfer *xfer); > }; > > +const struct scmi_message_operations *scmi_message_operations_get(void); > + > +/** > + * struct scmi_transport_core_operations - Transpoert core operations > + * > + * @bad_message_trace: An helper to report a malformed/unexpected message > + * @rx_callback: Callback to report received messages > + * @shmem: Datagram operations for shared memory based transports > + * @msg: Datagram operations for message based transports > + */ > +struct scmi_transport_core_operations { > + void (*bad_message_trace)(struct scmi_chan_info *cinfo, > + u32 msg_hdr, enum scmi_bad_msg err); > + void (*rx_callback)(struct scmi_chan_info *cinfo, u32 msg_hdr, > + void *priv); > + const struct scmi_shared_mem_operations *shmem; > + const struct scmi_message_operations *msg; > +}; > + > +/** > + * struct scmi_transport - A structure representing a configured transport > + * > + * @supplier: Device representimng the transport and acting as a supplier for typo: s/representimng/representing/ > + * the core SCMI stack > + * @desc: Transport descriptor > + * @core_ops: A pointer to a pointer used by the core SCMI stack to make the > + * core transport operations accessible to the transports. > + */ > +struct scmi_transport { > + struct device *supplier; > + const struct scmi_desc *desc; > + struct scmi_transport_core_operations **core_ops; > +}; > + > +#define DEFINE_SCMI_TRANSPORT_DRIVER(__trans, __match_table, __core_ptr)\ > +static int __trans##_probe(struct platform_device *pdev) \ > +{ \ > + struct scmi_transport *scmi_trans; \ > + struct platform_device *scmi_pdev; \ > + struct device *dev = &pdev->dev; \ > + \ > + scmi_trans = devm_kzalloc(dev, sizeof(*scmi_trans), GFP_KERNEL);\ > + if (!scmi_trans) \ > + return -ENOMEM; \ > + \ > + scmi_pdev = devm_kzalloc(dev, sizeof(*scmi_pdev), GFP_KERNEL); \ > + if (!scmi_pdev) \ > + return -ENOMEM; \ > + \ > + scmi_trans->supplier = dev; \ > + scmi_trans->desc = &__trans##_desc; \ It's a bit weird the scmi_desc shall be specifically labeled __trans##_desc in the transport driver source file while match table and transport core operations instances references are passed as arguments. I think it's worth having the scmi_desc label also passed as an argument to DEFINE_SCMI_TRANSPORT_DRIVER() macro. > + scmi_trans->core_ops = __core_ptr; \ > + \ > + scmi_pdev->name = "arm-scmi"; \ > + scmi_pdev->id = PLATFORM_DEVID_AUTO; \ > + scmi_pdev->dev.platform_data = scmi_trans; \ > + \ > + device_set_of_node_from_dev(&scmi_pdev->dev, dev); \ > + \ > + dev_set_drvdata(dev, scmi_pdev); \ > + \ > + return platform_device_register(scmi_pdev); \ > +} \ > + \ > +static void __trans##_remove(struct platform_device *pdev) \ > +{ \ > + struct platform_device *scmi_pdev; \ > + \ > + scmi_pdev = dev_get_drvdata(&pdev->dev); \ > + \ > + platform_device_unregister(scmi_pdev); \ > +} \ > + \ > +static struct platform_driver __trans##_driver = { \ Same here. I think __trans##_driver label should be also explicitly passed as an argument to DEFINE_SCMI_TRANSPORT_DRIVER(). BR, Etienne > + .driver = { \ > + .name = #__trans "_transport", \ > + .of_match_table = __match_table, \ > + }, \ > + .probe = __trans##_probe, \ > + .remove_new = __trans##_remove, \ > +} > + > extern const struct scmi_shared_mem_operations scmi_shmem_ops; > extern const struct scmi_message_operations scmi_msg_ops; > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index 6b6957f4743f..a1892d4d8c69 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -194,6 +194,11 @@ struct scmi_info { > #define bus_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, bus_nb) > #define req_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, dev_req_nb) > > +static struct scmi_transport_core_operations scmi_trans_core_ops = { > + .bad_message_trace = scmi_bad_message_trace, > + .rx_callback = scmi_rx_callback, > +}; > + > static unsigned long > scmi_vendor_protocol_signature(unsigned int protocol_id, char *vendor_id, > char *sub_vendor_id, u32 impl_ver) > @@ -2950,6 +2955,28 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info) > return ret; > } > > +static const struct scmi_desc *scmi_transport_lookup(struct device *dev) > +{ > + struct scmi_transport *trans; > + > + trans = dev_get_platdata(dev); > + if (!trans || !trans->desc || !trans->supplier || !trans->core_ops) > + return NULL; > + > + if (!device_link_add(dev, trans->supplier, DL_FLAG_AUTOREMOVE_CONSUMER)) { > + dev_err(dev, > + "Adding link to supplier transport device failed\n"); > + return NULL; > + } > + > + /* Provide core transport ops */ > + *trans->core_ops = &scmi_trans_core_ops; > + > + dev_info(dev, "Using %s\n", dev_driver_string(trans->supplier)); > + > + return trans->desc; > +} > + > static int scmi_probe(struct platform_device *pdev) > { > int ret; > @@ -2962,8 +2989,14 @@ static int scmi_probe(struct platform_device *pdev) > struct device_node *child, *np = dev->of_node; > > desc = of_device_get_match_data(dev); > - if (!desc) > - return -EINVAL; > + if (!desc) { > + desc = scmi_transport_lookup(dev); > + if (!desc) { > + err_str = "transport invalid\n"; > + ret = -EINVAL; > + goto out_err; > + } > + } > > info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); > if (!info) > @@ -3130,6 +3163,7 @@ static int scmi_probe(struct platform_device *pdev) > clear_ida: > ida_free(&scmi_id, info->id); > > +out_err: > return dev_err_probe(dev, ret, "%s", err_str); > } > > @@ -3321,6 +3355,12 @@ static int __init scmi_driver_init(void) > if (ret) > return ret; > > + if (IS_ENABLED(CONFIG_ARM_SCMI_HAVE_SHMEM)) > + scmi_trans_core_ops.shmem = scmi_shared_mem_operations_get(); > + > + if (IS_ENABLED(CONFIG_ARM_SCMI_HAVE_MSG)) > + scmi_trans_core_ops.msg = scmi_message_operations_get(); > + > if (IS_ENABLED(CONFIG_ARM_SCMI_NEED_DEBUGFS)) > scmi_top_dentry = scmi_debugfs_init(); > > diff --git a/drivers/firmware/arm_scmi/msg.c b/drivers/firmware/arm_scmi/msg.c > index f4ba38afe0bb..0bed1d2825af 100644 > --- a/drivers/firmware/arm_scmi/msg.c > +++ b/drivers/firmware/arm_scmi/msg.c > @@ -118,3 +118,8 @@ const struct scmi_message_operations scmi_msg_ops = { > .fetch_response = msg_fetch_response, > .fetch_notification = msg_fetch_notification, > }; > + > +const struct scmi_message_operations *scmi_message_operations_get(void) > +{ > + return &scmi_msg_ops; > +} > diff --git a/drivers/firmware/arm_scmi/shmem.c b/drivers/firmware/arm_scmi/shmem.c > index 208a289642c3..b1fc0c31495b 100644 > --- a/drivers/firmware/arm_scmi/shmem.c > +++ b/drivers/firmware/arm_scmi/shmem.c > @@ -187,3 +187,8 @@ const struct scmi_shared_mem_operations scmi_shmem_ops = { > .channel_intr_enabled = shmem_channel_intr_enabled, > .setup_iomap = shmem_setup_iomap, > }; > + > +const struct scmi_shared_mem_operations *scmi_shared_mem_operations_get(void) > +{ > + return &scmi_shmem_ops; > +} > -- > 2.45.2 >
On Tue, Jul 23, 2024 at 01:39:41PM +0000, Etienne CARRIERE wrote: > Hi Cristian, > > Few nitpicking comments. > > On Wednesday, July 10, 2024, Cristian Marussi wrote: > > Extend the core SCMI stack with structures and methods to allow for > > transports to be split out as standalone drivers, while still supporting > > old style transports, defined as built into the SCMI core stack. > > > > No functional change. > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > --- > > NOTE: old style transport support will be removed later in this series. > > > > v1 --> v2 > > - fixed comit message > > --- > > drivers/firmware/arm_scmi/common.h | 84 ++++++++++++++++++++++++++++++ > > drivers/firmware/arm_scmi/driver.c | 44 +++++++++++++++- > > drivers/firmware/arm_scmi/msg.c | 5 ++ > > drivers/firmware/arm_scmi/shmem.c | 5 ++ > > 4 files changed, 136 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > > index 8e5751aaa600..4af06810eb39 100644 > > --- a/drivers/firmware/arm_scmi/common.h > > +++ b/drivers/firmware/arm_scmi/common.h > > @@ -349,6 +349,8 @@ struct scmi_shared_mem_operations { > > bool tx, struct resource *res); > > }; > > > > +const struct scmi_shared_mem_operations *scmi_shared_mem_operations_get(void); > > + > > /* declarations for message passing transports */ > > struct scmi_msg_payld; > > > > @@ -376,6 +378,88 @@ struct scmi_message_operations { > > size_t max_len, struct scmi_xfer *xfer); > > }; > > > > +const struct scmi_message_operations *scmi_message_operations_get(void); > > + > > +/** > > + * struct scmi_transport_core_operations - Transpoert core operations > > + * > > + * @bad_message_trace: An helper to report a malformed/unexpected message > > + * @rx_callback: Callback to report received messages > > + * @shmem: Datagram operations for shared memory based transports > > + * @msg: Datagram operations for message based transports > > + */ > > +struct scmi_transport_core_operations { > > + void (*bad_message_trace)(struct scmi_chan_info *cinfo, > > + u32 msg_hdr, enum scmi_bad_msg err); > > + void (*rx_callback)(struct scmi_chan_info *cinfo, u32 msg_hdr, > > + void *priv); > > + const struct scmi_shared_mem_operations *shmem; > > + const struct scmi_message_operations *msg; > > +}; > > + > > +/** > > + * struct scmi_transport - A structure representing a configured transport > > + * > > + * @supplier: Device representimng the transport and acting as a supplier for > > typo: s/representimng/representing/ > Fixed in V3. (...still to be posted) > > + * the core SCMI stack > > + * @desc: Transport descriptor > > + * @core_ops: A pointer to a pointer used by the core SCMI stack to make the > > + * core transport operations accessible to the transports. > > + */ > > +struct scmi_transport { > > + struct device *supplier; > > + const struct scmi_desc *desc; > > + struct scmi_transport_core_operations **core_ops; > > +}; > > + > > +#define DEFINE_SCMI_TRANSPORT_DRIVER(__trans, __match_table, __core_ptr)\ > > +static int __trans##_probe(struct platform_device *pdev) \ > > +{ \ > > + struct scmi_transport *scmi_trans; \ > > + struct platform_device *scmi_pdev; \ > > + struct device *dev = &pdev->dev; \ > > + \ > > + scmi_trans = devm_kzalloc(dev, sizeof(*scmi_trans), GFP_KERNEL);\ > > + if (!scmi_trans) \ > > + return -ENOMEM; \ > > + \ > > + scmi_pdev = devm_kzalloc(dev, sizeof(*scmi_pdev), GFP_KERNEL); \ > > + if (!scmi_pdev) \ > > + return -ENOMEM; \ > > + \ > > + scmi_trans->supplier = dev; \ > > + scmi_trans->desc = &__trans##_desc; \ > > It's a bit weird the scmi_desc shall be specifically labeled __trans##_desc > in the transport driver source file while match table and transport core > operations instances references are passed as arguments. I think it's > worth having the scmi_desc label also passed as an argument to > DEFINE_SCMI_TRANSPORT_DRIVER() macro. Yes, I agree, I was unsure about this so I have reworked all of these in V3 to pass as explicit parameter the driver name and desc name. > > > + scmi_trans->core_ops = __core_ptr; \ > > + \ > > + scmi_pdev->name = "arm-scmi"; \ > > + scmi_pdev->id = PLATFORM_DEVID_AUTO; \ > > + scmi_pdev->dev.platform_data = scmi_trans; \ > > + \ > > + device_set_of_node_from_dev(&scmi_pdev->dev, dev); \ > > + \ > > + dev_set_drvdata(dev, scmi_pdev); \ > > + \ > > + return platform_device_register(scmi_pdev); \ > > +} \ > > + \ > > +static void __trans##_remove(struct platform_device *pdev) \ > > +{ \ > > + struct platform_device *scmi_pdev; \ > > + \ > > + scmi_pdev = dev_get_drvdata(&pdev->dev); \ > > + \ > > + platform_device_unregister(scmi_pdev); \ > > +} \ > > + \ > > +static struct platform_driver __trans##_driver = { \ > > Same here. I think __trans##_driver label should be also explicitly > passed as an argument to DEFINE_SCMI_TRANSPORT_DRIVER(). > Fixed in V3. Thanks, Cristian
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 8e5751aaa600..4af06810eb39 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -349,6 +349,8 @@ struct scmi_shared_mem_operations { bool tx, struct resource *res); }; +const struct scmi_shared_mem_operations *scmi_shared_mem_operations_get(void); + /* declarations for message passing transports */ struct scmi_msg_payld; @@ -376,6 +378,88 @@ struct scmi_message_operations { size_t max_len, struct scmi_xfer *xfer); }; +const struct scmi_message_operations *scmi_message_operations_get(void); + +/** + * struct scmi_transport_core_operations - Transpoert core operations + * + * @bad_message_trace: An helper to report a malformed/unexpected message + * @rx_callback: Callback to report received messages + * @shmem: Datagram operations for shared memory based transports + * @msg: Datagram operations for message based transports + */ +struct scmi_transport_core_operations { + void (*bad_message_trace)(struct scmi_chan_info *cinfo, + u32 msg_hdr, enum scmi_bad_msg err); + void (*rx_callback)(struct scmi_chan_info *cinfo, u32 msg_hdr, + void *priv); + const struct scmi_shared_mem_operations *shmem; + const struct scmi_message_operations *msg; +}; + +/** + * struct scmi_transport - A structure representing a configured transport + * + * @supplier: Device representimng the transport and acting as a supplier for + * the core SCMI stack + * @desc: Transport descriptor + * @core_ops: A pointer to a pointer used by the core SCMI stack to make the + * core transport operations accessible to the transports. + */ +struct scmi_transport { + struct device *supplier; + const struct scmi_desc *desc; + struct scmi_transport_core_operations **core_ops; +}; + +#define DEFINE_SCMI_TRANSPORT_DRIVER(__trans, __match_table, __core_ptr)\ +static int __trans##_probe(struct platform_device *pdev) \ +{ \ + struct scmi_transport *scmi_trans; \ + struct platform_device *scmi_pdev; \ + struct device *dev = &pdev->dev; \ + \ + scmi_trans = devm_kzalloc(dev, sizeof(*scmi_trans), GFP_KERNEL);\ + if (!scmi_trans) \ + return -ENOMEM; \ + \ + scmi_pdev = devm_kzalloc(dev, sizeof(*scmi_pdev), GFP_KERNEL); \ + if (!scmi_pdev) \ + return -ENOMEM; \ + \ + scmi_trans->supplier = dev; \ + scmi_trans->desc = &__trans##_desc; \ + scmi_trans->core_ops = __core_ptr; \ + \ + scmi_pdev->name = "arm-scmi"; \ + scmi_pdev->id = PLATFORM_DEVID_AUTO; \ + scmi_pdev->dev.platform_data = scmi_trans; \ + \ + device_set_of_node_from_dev(&scmi_pdev->dev, dev); \ + \ + dev_set_drvdata(dev, scmi_pdev); \ + \ + return platform_device_register(scmi_pdev); \ +} \ + \ +static void __trans##_remove(struct platform_device *pdev) \ +{ \ + struct platform_device *scmi_pdev; \ + \ + scmi_pdev = dev_get_drvdata(&pdev->dev); \ + \ + platform_device_unregister(scmi_pdev); \ +} \ + \ +static struct platform_driver __trans##_driver = { \ + .driver = { \ + .name = #__trans "_transport", \ + .of_match_table = __match_table, \ + }, \ + .probe = __trans##_probe, \ + .remove_new = __trans##_remove, \ +} + extern const struct scmi_shared_mem_operations scmi_shmem_ops; extern const struct scmi_message_operations scmi_msg_ops; diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 6b6957f4743f..a1892d4d8c69 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -194,6 +194,11 @@ struct scmi_info { #define bus_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, bus_nb) #define req_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, dev_req_nb) +static struct scmi_transport_core_operations scmi_trans_core_ops = { + .bad_message_trace = scmi_bad_message_trace, + .rx_callback = scmi_rx_callback, +}; + static unsigned long scmi_vendor_protocol_signature(unsigned int protocol_id, char *vendor_id, char *sub_vendor_id, u32 impl_ver) @@ -2950,6 +2955,28 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info) return ret; } +static const struct scmi_desc *scmi_transport_lookup(struct device *dev) +{ + struct scmi_transport *trans; + + trans = dev_get_platdata(dev); + if (!trans || !trans->desc || !trans->supplier || !trans->core_ops) + return NULL; + + if (!device_link_add(dev, trans->supplier, DL_FLAG_AUTOREMOVE_CONSUMER)) { + dev_err(dev, + "Adding link to supplier transport device failed\n"); + return NULL; + } + + /* Provide core transport ops */ + *trans->core_ops = &scmi_trans_core_ops; + + dev_info(dev, "Using %s\n", dev_driver_string(trans->supplier)); + + return trans->desc; +} + static int scmi_probe(struct platform_device *pdev) { int ret; @@ -2962,8 +2989,14 @@ static int scmi_probe(struct platform_device *pdev) struct device_node *child, *np = dev->of_node; desc = of_device_get_match_data(dev); - if (!desc) - return -EINVAL; + if (!desc) { + desc = scmi_transport_lookup(dev); + if (!desc) { + err_str = "transport invalid\n"; + ret = -EINVAL; + goto out_err; + } + } info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); if (!info) @@ -3130,6 +3163,7 @@ static int scmi_probe(struct platform_device *pdev) clear_ida: ida_free(&scmi_id, info->id); +out_err: return dev_err_probe(dev, ret, "%s", err_str); } @@ -3321,6 +3355,12 @@ static int __init scmi_driver_init(void) if (ret) return ret; + if (IS_ENABLED(CONFIG_ARM_SCMI_HAVE_SHMEM)) + scmi_trans_core_ops.shmem = scmi_shared_mem_operations_get(); + + if (IS_ENABLED(CONFIG_ARM_SCMI_HAVE_MSG)) + scmi_trans_core_ops.msg = scmi_message_operations_get(); + if (IS_ENABLED(CONFIG_ARM_SCMI_NEED_DEBUGFS)) scmi_top_dentry = scmi_debugfs_init(); diff --git a/drivers/firmware/arm_scmi/msg.c b/drivers/firmware/arm_scmi/msg.c index f4ba38afe0bb..0bed1d2825af 100644 --- a/drivers/firmware/arm_scmi/msg.c +++ b/drivers/firmware/arm_scmi/msg.c @@ -118,3 +118,8 @@ const struct scmi_message_operations scmi_msg_ops = { .fetch_response = msg_fetch_response, .fetch_notification = msg_fetch_notification, }; + +const struct scmi_message_operations *scmi_message_operations_get(void) +{ + return &scmi_msg_ops; +} diff --git a/drivers/firmware/arm_scmi/shmem.c b/drivers/firmware/arm_scmi/shmem.c index 208a289642c3..b1fc0c31495b 100644 --- a/drivers/firmware/arm_scmi/shmem.c +++ b/drivers/firmware/arm_scmi/shmem.c @@ -187,3 +187,8 @@ const struct scmi_shared_mem_operations scmi_shmem_ops = { .channel_intr_enabled = shmem_channel_intr_enabled, .setup_iomap = shmem_setup_iomap, }; + +const struct scmi_shared_mem_operations *scmi_shared_mem_operations_get(void) +{ + return &scmi_shmem_ops; +}
Extend the core SCMI stack with structures and methods to allow for transports to be split out as standalone drivers, while still supporting old style transports, defined as built into the SCMI core stack. No functional change. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- NOTE: old style transport support will be removed later in this series. v1 --> v2 - fixed comit message --- drivers/firmware/arm_scmi/common.h | 84 ++++++++++++++++++++++++++++++ drivers/firmware/arm_scmi/driver.c | 44 +++++++++++++++- drivers/firmware/arm_scmi/msg.c | 5 ++ drivers/firmware/arm_scmi/shmem.c | 5 ++ 4 files changed, 136 insertions(+), 2 deletions(-)