| Message ID | 20231011100707.1007417-3-takahiro.akashi@linaro.org |
|---|---|
| State | Accepted |
| Commit | 689204be9744db24fc8031229946f045fae02c07 |
| Headers | show |
| Series | firmware: scmi: add SCMI base protocol support | expand |
On Wed, 11 Oct 2023 at 03:07, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > SCMI specification allows any protocol to have its own channel for > the transport. While the current SCMI driver may assign its channel > from a device tree, the core function, devm_scmi_process_msg(), doesn't > use a protocol's channel, but always use an agent's channel. > > With this commit, devm_scmi_process_msg() tries to find and use > a protocol's channel. If it doesn't exist, use an agent's. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> > --- > v5 > * new commit (fixing a potential bug) > --- > drivers/firmware/scmi/mailbox_agent.c | 5 +++-- > drivers/firmware/scmi/optee_agent.c | 5 +++-- > drivers/firmware/scmi/scmi_agent-uclass.c | 7 ++++--- > drivers/firmware/scmi/smccc_agent.c | 5 +++-- > include/scmi_agent-uclass.h | 8 +++++--- > 5 files changed, 18 insertions(+), 12 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
Hi, On 10/11/23 12:06 PM, AKASHI Takahiro wrote: > SCMI specification allows any protocol to have its own channel for > the transport. While the current SCMI driver may assign its channel > from a device tree, the core function, devm_scmi_process_msg(), doesn't > use a protocol's channel, but always use an agent's channel. > > With this commit, devm_scmi_process_msg() tries to find and use > a protocol's channel. If it doesn't exist, use an agent's. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> > --- > v5 > * new commit (fixing a potential bug) > --- > drivers/firmware/scmi/mailbox_agent.c | 5 +++-- > drivers/firmware/scmi/optee_agent.c | 5 +++-- > drivers/firmware/scmi/scmi_agent-uclass.c | 7 ++++--- > drivers/firmware/scmi/smccc_agent.c | 5 +++-- > include/scmi_agent-uclass.h | 8 +++++--- > 5 files changed, 18 insertions(+), 12 deletions(-) > > diff --git a/drivers/firmware/scmi/mailbox_agent.c b/drivers/firmware/scmi/mailbox_agent.c > index 8277c1860606..7ad3e8da9f08 100644 > --- a/drivers/firmware/scmi/mailbox_agent.c > +++ b/drivers/firmware/scmi/mailbox_agent.c > @@ -94,13 +94,14 @@ static int setup_channel(struct udevice *dev, struct scmi_mbox_channel *chan) > } > > static int scmi_mbox_get_channel(struct udevice *dev, > + struct udevice *protocol, > struct scmi_channel **channel) > { > struct scmi_mbox_channel *base_chan = dev_get_plat(dev); > struct scmi_mbox_channel *chan; > int ret; > > - if (!dev_read_prop(dev, "shmem", NULL)) { > + if (!dev_read_prop(protocol, "shmem", NULL)) { > /* Uses agent base channel */ > *channel = container_of(base_chan, struct scmi_channel, ref); > > @@ -112,7 +113,7 @@ static int scmi_mbox_get_channel(struct udevice *dev, > return -ENOMEM; > > /* Setup a dedicated channel for the protocol */ > - ret = setup_channel(dev, chan); > + ret = setup_channel(protocol, chan); > if (ret) { > free(chan); > return ret; > diff --git a/drivers/firmware/scmi/optee_agent.c b/drivers/firmware/scmi/optee_agent.c > index db927fb21405..e3e462774045 100644 > --- a/drivers/firmware/scmi/optee_agent.c > +++ b/drivers/firmware/scmi/optee_agent.c > @@ -324,6 +324,7 @@ static int setup_channel(struct udevice *dev, struct scmi_optee_channel *chan) > } > > static int scmi_optee_get_channel(struct udevice *dev, > + struct udevice *protocol, > struct scmi_channel **channel) > { > struct scmi_optee_channel *base_chan = dev_get_plat(dev); > @@ -331,7 +332,7 @@ static int scmi_optee_get_channel(struct udevice *dev, > u32 channel_id; > int ret; > > - if (dev_read_u32(dev, "linaro,optee-channel-id", &channel_id)) { > + if (dev_read_u32(protocol, "linaro,optee-channel-id", &channel_id)) { > /* Uses agent base channel */ > *channel = container_of(base_chan, struct scmi_channel, ref); > > @@ -343,7 +344,7 @@ static int scmi_optee_get_channel(struct udevice *dev, > if (!chan) > return -ENOMEM; > > - ret = setup_channel(dev, chan); > + ret = setup_channel(protocol, chan); > if (ret) { > free(chan); > return ret; > diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c > index ec58ccd2bc5d..a28692f39f4d 100644 > --- a/drivers/firmware/scmi/scmi_agent-uclass.c > +++ b/drivers/firmware/scmi/scmi_agent-uclass.c > @@ -144,13 +144,14 @@ static const struct scmi_agent_ops *transport_dev_ops(struct udevice *dev) > * On return, @channel will be set. > * Return 0 on success and a negative errno on failure > */ > -static int scmi_of_get_channel(struct udevice *dev, struct scmi_channel **channel) > +static int scmi_of_get_channel(struct udevice *dev, struct udevice *protocol, > + struct scmi_channel **channel) > { > const struct scmi_agent_ops *ops; > > ops = transport_dev_ops(dev); > if (ops->of_get_channel) > - return ops->of_get_channel(dev, channel); > + return ops->of_get_channel(dev, protocol, channel); > else > return -EPROTONOSUPPORT; > } > @@ -166,7 +167,7 @@ int devm_scmi_of_get_channel(struct udevice *dev) > return -ENODEV; > > priv = dev_get_parent_priv(protocol); > - ret = scmi_of_get_channel(protocol->parent, &priv->channel); > + ret = scmi_of_get_channel(protocol->parent, protocol, &priv->channel); > if (ret == -EPROTONOSUPPORT) { > /* Drivers without a get_channel operator don't need a channel ref */ > priv->channel = NULL; > diff --git a/drivers/firmware/scmi/smccc_agent.c b/drivers/firmware/scmi/smccc_agent.c > index 6a52cd75d67b..972c6addde21 100644 > --- a/drivers/firmware/scmi/smccc_agent.c > +++ b/drivers/firmware/scmi/smccc_agent.c > @@ -81,6 +81,7 @@ static int setup_channel(struct udevice *dev, struct scmi_smccc_channel *chan) > } > > static int scmi_smccc_get_channel(struct udevice *dev, > + struct udevice *protocol, > struct scmi_channel **channel) > { > struct scmi_smccc_channel *base_chan = dev_get_plat(dev); > @@ -88,7 +89,7 @@ static int scmi_smccc_get_channel(struct udevice *dev, > u32 func_id; > int ret; > > - if (dev_read_u32(dev, "arm,smc-id", &func_id)) { > + if (dev_read_u32(protocol, "arm,smc-id", &func_id)) { This seems to trigger the assert in https://elixir.bootlin.com/u-boot/latest/source/drivers/core/ofnode.c#L393 when building with DM_DEBUG=y on my RK3588 board, making it fail to boot into U-Boot CLI: """ - found match at driver 'scmi-over-smccc' for 'arm,scmi-smc' ofnode_read_u32_index: arm,smc-id: 0x82000010 (2181038096) drivers/core/ofnode.c:393: ofnode_read_u32_index: Assertion `ofnode_valid(node)' failed. resetting ... """ Not sure what we should be doing here? Cheers, Quentin
diff --git a/drivers/firmware/scmi/mailbox_agent.c b/drivers/firmware/scmi/mailbox_agent.c index 8277c1860606..7ad3e8da9f08 100644 --- a/drivers/firmware/scmi/mailbox_agent.c +++ b/drivers/firmware/scmi/mailbox_agent.c @@ -94,13 +94,14 @@ static int setup_channel(struct udevice *dev, struct scmi_mbox_channel *chan) } static int scmi_mbox_get_channel(struct udevice *dev, + struct udevice *protocol, struct scmi_channel **channel) { struct scmi_mbox_channel *base_chan = dev_get_plat(dev); struct scmi_mbox_channel *chan; int ret; - if (!dev_read_prop(dev, "shmem", NULL)) { + if (!dev_read_prop(protocol, "shmem", NULL)) { /* Uses agent base channel */ *channel = container_of(base_chan, struct scmi_channel, ref); @@ -112,7 +113,7 @@ static int scmi_mbox_get_channel(struct udevice *dev, return -ENOMEM; /* Setup a dedicated channel for the protocol */ - ret = setup_channel(dev, chan); + ret = setup_channel(protocol, chan); if (ret) { free(chan); return ret; diff --git a/drivers/firmware/scmi/optee_agent.c b/drivers/firmware/scmi/optee_agent.c index db927fb21405..e3e462774045 100644 --- a/drivers/firmware/scmi/optee_agent.c +++ b/drivers/firmware/scmi/optee_agent.c @@ -324,6 +324,7 @@ static int setup_channel(struct udevice *dev, struct scmi_optee_channel *chan) } static int scmi_optee_get_channel(struct udevice *dev, + struct udevice *protocol, struct scmi_channel **channel) { struct scmi_optee_channel *base_chan = dev_get_plat(dev); @@ -331,7 +332,7 @@ static int scmi_optee_get_channel(struct udevice *dev, u32 channel_id; int ret; - if (dev_read_u32(dev, "linaro,optee-channel-id", &channel_id)) { + if (dev_read_u32(protocol, "linaro,optee-channel-id", &channel_id)) { /* Uses agent base channel */ *channel = container_of(base_chan, struct scmi_channel, ref); @@ -343,7 +344,7 @@ static int scmi_optee_get_channel(struct udevice *dev, if (!chan) return -ENOMEM; - ret = setup_channel(dev, chan); + ret = setup_channel(protocol, chan); if (ret) { free(chan); return ret; diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c index ec58ccd2bc5d..a28692f39f4d 100644 --- a/drivers/firmware/scmi/scmi_agent-uclass.c +++ b/drivers/firmware/scmi/scmi_agent-uclass.c @@ -144,13 +144,14 @@ static const struct scmi_agent_ops *transport_dev_ops(struct udevice *dev) * On return, @channel will be set. * Return 0 on success and a negative errno on failure */ -static int scmi_of_get_channel(struct udevice *dev, struct scmi_channel **channel) +static int scmi_of_get_channel(struct udevice *dev, struct udevice *protocol, + struct scmi_channel **channel) { const struct scmi_agent_ops *ops; ops = transport_dev_ops(dev); if (ops->of_get_channel) - return ops->of_get_channel(dev, channel); + return ops->of_get_channel(dev, protocol, channel); else return -EPROTONOSUPPORT; } @@ -166,7 +167,7 @@ int devm_scmi_of_get_channel(struct udevice *dev) return -ENODEV; priv = dev_get_parent_priv(protocol); - ret = scmi_of_get_channel(protocol->parent, &priv->channel); + ret = scmi_of_get_channel(protocol->parent, protocol, &priv->channel); if (ret == -EPROTONOSUPPORT) { /* Drivers without a get_channel operator don't need a channel ref */ priv->channel = NULL; diff --git a/drivers/firmware/scmi/smccc_agent.c b/drivers/firmware/scmi/smccc_agent.c index 6a52cd75d67b..972c6addde21 100644 --- a/drivers/firmware/scmi/smccc_agent.c +++ b/drivers/firmware/scmi/smccc_agent.c @@ -81,6 +81,7 @@ static int setup_channel(struct udevice *dev, struct scmi_smccc_channel *chan) } static int scmi_smccc_get_channel(struct udevice *dev, + struct udevice *protocol, struct scmi_channel **channel) { struct scmi_smccc_channel *base_chan = dev_get_plat(dev); @@ -88,7 +89,7 @@ static int scmi_smccc_get_channel(struct udevice *dev, u32 func_id; int ret; - if (dev_read_u32(dev, "arm,smc-id", &func_id)) { + if (dev_read_u32(protocol, "arm,smc-id", &func_id)) { /* Uses agent base channel */ *channel = container_of(base_chan, struct scmi_channel, ref); @@ -100,7 +101,7 @@ static int scmi_smccc_get_channel(struct udevice *dev, if (!chan) return -ENOMEM; - ret = setup_channel(dev, chan); + ret = setup_channel(protocol, chan); if (ret) { free(chan); return ret; diff --git a/include/scmi_agent-uclass.h b/include/scmi_agent-uclass.h index b1c93532c0ea..eee46c880a56 100644 --- a/include/scmi_agent-uclass.h +++ b/include/scmi_agent-uclass.h @@ -16,16 +16,18 @@ struct scmi_agent_ops { /* * of_get_channel - Get SCMI channel from SCMI agent device tree node * - * @dev: SCMI protocol device using the transport + * @dev: SCMI agent device using the transport + * @protocol: SCMI protocol device using the transport * @channel: Output reference to SCMI channel upon success * Return 0 upon success and a negative errno on failure */ - int (*of_get_channel)(struct udevice *dev, struct scmi_channel **channel); + int (*of_get_channel)(struct udevice *dev, struct udevice *protocol, + struct scmi_channel **channel); /* * process_msg - Request transport to get the SCMI message processed * - * @dev: SCMI protocol device using the transport + * @dev: SCMI agent device using the transport * @msg: SCMI message to be transmitted */ int (*process_msg)(struct udevice *dev, struct scmi_channel *channel,