Re: [PATCH v2 3/8] firmware: arm_scmi: Add support for standalone transport drivers

Etienne CARRIERE
2024-07-23T13:40:04Z

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
>