From: Andre Przywara <andre.przywara@arm.com> To: Florian Fainelli <f.fainelli@gmail.com> Cc: peng.fan@nxp.com, robh+dt@kernel.org, mark.rutland@arm.com, jassisinghbrar@gmail.com, sudeep.holla@arm.com, kernel@pengutronix.de, linux-imx@nxp.com, shawnguo@kernel.org, festevam@gmail.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, van.freenix@gmail.com Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox Date: Thu, 6 Jun 2019 14:20:56 +0100 [thread overview] Message-ID: <20190606142056.68272dc0@donnerap.cambridge.arm.com> (raw) In-Reply-To: <866db682-785a-e0a6-b394-bb65c7a694c6@gmail.com> On Mon, 3 Jun 2019 09:32:42 -0700 Florian Fainelli <f.fainelli@gmail.com> wrote: Hi, > On 6/3/19 1:30 AM, peng.fan@nxp.com wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > This mailbox driver implements a mailbox which signals transmitted data > > via an ARM smc (secure monitor call) instruction. The mailbox receiver > > is implemented in firmware and can synchronously return data when it > > returns execution to the non-secure world again. > > An asynchronous receive path is not implemented. > > This allows the usage of a mailbox to trigger firmware actions on SoCs > > which either don't have a separate management processor or on which such > > a core is not available. A user of this mailbox could be the SCP > > interface. > > > > Modified from Andre Przywara's v2 patch > > https://lore.kernel.org/patchwork/patch/812999/ > > > > Cc: Andre Przywara <andre.przywara@arm.com> > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > [snip] > > +#define ARM_SMC_MBOX_USB_IRQ BIT(1) > > That flag appears unused. > > > +static int arm_smc_mbox_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct mbox_controller *mbox; > > + struct arm_smc_chan_data *chan_data; > > + const char *method; > > + bool use_hvc = false; > > + int ret, irq_count, i; > > + u32 val; > > + > > + if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) { > > + if (val < 1 || val > INT_MAX) { > > + dev_err(dev, "invalid arm,num-chans value %u of %pOFn\n", val, pdev->dev.of_node); Isn't the of_node parameter redundant, because dev_err() already takes care of that? > > + return -EINVAL; > > + } > > + } > > Should not the upper bound check be done against UINT_MAX since val is > an unsigned int? But wouldn't that be somewhat pointless, given that val is a u32? So I guess we could just condense this down to: ... if (!val) { ... > > + > > + irq_count = platform_irq_count(pdev); > > + if (irq_count == -EPROBE_DEFER) > > + return irq_count; > > + > > + if (irq_count && irq_count != val) { > > + dev_err(dev, "Interrupts not match num-chans\n"); > > Interrupts property does not match \"arm,num-chans\" would be more correct. Given that interrupts are optional, do we have to rely on this? Do we actually need one interrupt per channel? > > + return -EINVAL; > > + } > > + > > + if (!of_property_read_string(dev->of_node, "method", &method)) { > > + if (!strcmp("hvc", method)) { > > + use_hvc = true; > > + } else if (!strcmp("smc", method)) { > > + use_hvc = false; > > + } else { > > + dev_warn(dev, "invalid \"method\" property: %s\n", > > + method); > > + > > + return -EINVAL; > > + } > > Having at least one method specified does not seem to be checked later > on in the code, so if I omitted to specify that property, we would still > register the mailbox and default to use "smc" since the > ARM_SMC_MBOX_USE_HVC flag would not be set, would not we want to make > sure that we do have in fact a valid method specified given the binding > documents that property as mandatory? > > [snip] > > > + mbox->txdone_poll = false; > > + mbox->txdone_irq = false; > > + mbox->ops = &arm_smc_mbox_chan_ops; > > + mbox->dev = dev; > > + > > + ret = mbox_controller_register(mbox); > > + if (ret) > > + return ret; > > + > > + platform_set_drvdata(pdev, mbox); > > I would move this above mbox_controller_register() that way there is no > room for race conditions in case another part of the driver expects to > have pdev->dev.drvdata set before the mbox controller is registered. > Since you use devm_* functions for everything, you may even remove that > call. > > [snip] > > > +#ifndef _LINUX_ARM_SMC_MAILBOX_H_ > > +#define _LINUX_ARM_SMC_MAILBOX_H_ > > + > > +struct arm_smccc_mbox_cmd { > > + unsigned long a0, a1, a2, a3, a4, a5, a6, a7; > > +}; > > Do you expect this to be used by other in-kernel users? If so, it might > be good to document how a0 can have a special meaning and be used as a > substitute for the function_id? I don't think we should really expose this outside of the driver. From a mailbox point of view this is just the payload, transported according to the SMCCC. Also using "long" here sounds somewhat troublesome. Also, looking at the SMCCC, I only see six parameters in addition to the function identifier. Shall we reflect this here? Cheers, Andre.
WARNING: multiple messages have this Message-ID (diff)
From: Andre Przywara <andre.przywara@arm.com> To: Florian Fainelli <f.fainelli@gmail.com> Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, peng.fan@nxp.com, festevam@gmail.com, jassisinghbrar@gmail.com, linux-kernel@vger.kernel.org, robh+dt@kernel.org, linux-imx@nxp.com, kernel@pengutronix.de, sudeep.holla@arm.com, van.freenix@gmail.com, shawnguo@kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox Date: Thu, 6 Jun 2019 14:20:56 +0100 [thread overview] Message-ID: <20190606142056.68272dc0@donnerap.cambridge.arm.com> (raw) In-Reply-To: <866db682-785a-e0a6-b394-bb65c7a694c6@gmail.com> On Mon, 3 Jun 2019 09:32:42 -0700 Florian Fainelli <f.fainelli@gmail.com> wrote: Hi, > On 6/3/19 1:30 AM, peng.fan@nxp.com wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > This mailbox driver implements a mailbox which signals transmitted data > > via an ARM smc (secure monitor call) instruction. The mailbox receiver > > is implemented in firmware and can synchronously return data when it > > returns execution to the non-secure world again. > > An asynchronous receive path is not implemented. > > This allows the usage of a mailbox to trigger firmware actions on SoCs > > which either don't have a separate management processor or on which such > > a core is not available. A user of this mailbox could be the SCP > > interface. > > > > Modified from Andre Przywara's v2 patch > > https://lore.kernel.org/patchwork/patch/812999/ > > > > Cc: Andre Przywara <andre.przywara@arm.com> > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > [snip] > > +#define ARM_SMC_MBOX_USB_IRQ BIT(1) > > That flag appears unused. > > > +static int arm_smc_mbox_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct mbox_controller *mbox; > > + struct arm_smc_chan_data *chan_data; > > + const char *method; > > + bool use_hvc = false; > > + int ret, irq_count, i; > > + u32 val; > > + > > + if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) { > > + if (val < 1 || val > INT_MAX) { > > + dev_err(dev, "invalid arm,num-chans value %u of %pOFn\n", val, pdev->dev.of_node); Isn't the of_node parameter redundant, because dev_err() already takes care of that? > > + return -EINVAL; > > + } > > + } > > Should not the upper bound check be done against UINT_MAX since val is > an unsigned int? But wouldn't that be somewhat pointless, given that val is a u32? So I guess we could just condense this down to: ... if (!val) { ... > > + > > + irq_count = platform_irq_count(pdev); > > + if (irq_count == -EPROBE_DEFER) > > + return irq_count; > > + > > + if (irq_count && irq_count != val) { > > + dev_err(dev, "Interrupts not match num-chans\n"); > > Interrupts property does not match \"arm,num-chans\" would be more correct. Given that interrupts are optional, do we have to rely on this? Do we actually need one interrupt per channel? > > + return -EINVAL; > > + } > > + > > + if (!of_property_read_string(dev->of_node, "method", &method)) { > > + if (!strcmp("hvc", method)) { > > + use_hvc = true; > > + } else if (!strcmp("smc", method)) { > > + use_hvc = false; > > + } else { > > + dev_warn(dev, "invalid \"method\" property: %s\n", > > + method); > > + > > + return -EINVAL; > > + } > > Having at least one method specified does not seem to be checked later > on in the code, so if I omitted to specify that property, we would still > register the mailbox and default to use "smc" since the > ARM_SMC_MBOX_USE_HVC flag would not be set, would not we want to make > sure that we do have in fact a valid method specified given the binding > documents that property as mandatory? > > [snip] > > > + mbox->txdone_poll = false; > > + mbox->txdone_irq = false; > > + mbox->ops = &arm_smc_mbox_chan_ops; > > + mbox->dev = dev; > > + > > + ret = mbox_controller_register(mbox); > > + if (ret) > > + return ret; > > + > > + platform_set_drvdata(pdev, mbox); > > I would move this above mbox_controller_register() that way there is no > room for race conditions in case another part of the driver expects to > have pdev->dev.drvdata set before the mbox controller is registered. > Since you use devm_* functions for everything, you may even remove that > call. > > [snip] > > > +#ifndef _LINUX_ARM_SMC_MAILBOX_H_ > > +#define _LINUX_ARM_SMC_MAILBOX_H_ > > + > > +struct arm_smccc_mbox_cmd { > > + unsigned long a0, a1, a2, a3, a4, a5, a6, a7; > > +}; > > Do you expect this to be used by other in-kernel users? If so, it might > be good to document how a0 can have a special meaning and be used as a > substitute for the function_id? I don't think we should really expose this outside of the driver. From a mailbox point of view this is just the payload, transported according to the SMCCC. Also using "long" here sounds somewhat troublesome. Also, looking at the SMCCC, I only see six parameters in addition to the function identifier. Shall we reflect this here? Cheers, Andre. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-06-06 13:21 UTC|newest] Thread overview: 109+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-03 8:30 [PATCH V2 0/2] mailbox: arm: introduce smc triggered mailbox peng.fan 2019-06-03 8:30 ` peng.fan 2019-06-03 8:30 ` [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox peng.fan 2019-06-03 8:30 ` peng.fan 2019-06-03 16:22 ` Florian Fainelli 2019-06-03 16:22 ` Florian Fainelli 2019-06-03 16:56 ` Sudeep Holla 2019-06-03 16:56 ` Sudeep Holla 2019-06-03 17:18 ` Andre Przywara 2019-06-03 17:18 ` Andre Przywara 2019-06-03 17:18 ` Andre Przywara 2019-06-06 2:51 ` Florian Fainelli 2019-06-06 2:51 ` Florian Fainelli 2019-06-06 3:24 ` Peng Fan 2019-06-06 3:24 ` Peng Fan 2019-06-06 3:24 ` Peng Fan 2019-06-20 9:22 ` Sudeep Holla 2019-06-20 9:22 ` Sudeep Holla 2019-06-20 16:13 ` Andre Przywara 2019-06-20 16:13 ` Andre Przywara 2019-06-20 16:13 ` Andre Przywara 2019-06-20 16:27 ` Jassi Brar 2019-06-20 16:27 ` Jassi Brar 2019-07-08 22:19 ` Rob Herring 2019-07-08 22:19 ` Rob Herring 2019-07-09 1:40 ` Peng Fan 2019-07-09 1:40 ` Peng Fan 2019-07-09 1:40 ` Peng Fan 2019-07-09 13:31 ` Rob Herring 2019-07-09 13:31 ` Rob Herring 2019-07-09 13:31 ` Rob Herring 2019-06-03 8:30 ` [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox peng.fan 2019-06-03 8:30 ` peng.fan 2019-06-03 16:32 ` Florian Fainelli 2019-06-03 16:32 ` Florian Fainelli 2019-06-06 3:35 ` Peng Fan 2019-06-06 3:35 ` Peng Fan 2019-06-06 3:35 ` Peng Fan 2019-06-06 13:20 ` Andre Przywara [this message] 2019-06-06 13:20 ` Andre Przywara 2019-06-10 1:32 ` Peng Fan 2019-06-10 1:32 ` Peng Fan 2019-06-10 1:32 ` Peng Fan 2019-06-10 10:00 ` Andre Przywara 2019-06-10 10:00 ` Andre Przywara 2019-06-10 10:00 ` Andre Przywara 2019-06-12 12:59 ` Peng Fan 2019-06-12 12:59 ` Peng Fan 2019-06-12 12:59 ` Peng Fan 2019-06-12 17:18 ` Andre Przywara 2019-06-12 17:18 ` Andre Przywara 2019-06-12 17:18 ` Andre Przywara 2019-06-20 9:23 ` Sudeep Holla 2019-06-20 9:23 ` Sudeep Holla 2019-06-20 10:21 ` Peng Fan 2019-06-20 10:21 ` Peng Fan 2019-06-20 10:21 ` Peng Fan 2019-06-20 11:15 ` Sudeep Holla 2019-06-20 11:15 ` Sudeep Holla 2019-06-20 11:15 ` Sudeep Holla 2019-06-25 7:28 ` Peng Fan 2019-06-25 7:28 ` Peng Fan 2019-06-25 7:28 ` Peng Fan 2019-06-20 16:50 ` Jassi Brar 2019-06-20 16:50 ` Jassi Brar 2019-06-25 7:20 ` Peng Fan 2019-06-25 7:20 ` Peng Fan 2019-06-26 17:05 ` André Przywara 2019-06-26 17:05 ` André Przywara 2019-06-26 17:05 ` André Przywara 2019-06-26 17:07 ` Florian Fainelli 2019-06-26 17:07 ` Florian Fainelli 2019-06-26 17:07 ` Florian Fainelli 2019-06-25 7:30 ` Peng Fan 2019-06-25 7:30 ` Peng Fan 2019-06-25 7:30 ` Peng Fan 2019-06-25 14:36 ` Jassi Brar 2019-06-25 14:36 ` Jassi Brar 2019-06-25 14:36 ` Jassi Brar 2019-06-26 13:31 ` Peng Fan 2019-06-26 13:31 ` Peng Fan 2019-06-26 13:31 ` Peng Fan 2019-06-26 16:31 ` Jassi Brar 2019-06-26 16:31 ` Jassi Brar 2019-06-26 16:31 ` Jassi Brar 2019-06-26 16:44 ` Florian Fainelli 2019-06-26 16:44 ` Florian Fainelli 2019-06-26 16:44 ` Florian Fainelli 2019-06-26 17:09 ` Sudeep Holla 2019-06-26 17:09 ` Sudeep Holla 2019-06-26 17:09 ` Sudeep Holla 2019-06-27 18:10 ` Florian Fainelli 2019-06-27 18:10 ` Florian Fainelli 2019-06-27 18:10 ` Florian Fainelli 2019-06-26 18:27 ` Jassi Brar 2019-06-26 18:27 ` Jassi Brar 2019-06-26 18:27 ` Jassi Brar 2019-06-27 9:09 ` Sudeep Holla 2019-06-27 9:09 ` Sudeep Holla 2019-06-27 9:09 ` Sudeep Holla 2019-06-27 15:32 ` Jassi Brar 2019-06-27 15:32 ` Jassi Brar 2019-06-27 15:32 ` Jassi Brar 2019-06-27 17:07 ` Sudeep Holla 2019-06-27 17:07 ` Sudeep Holla 2019-06-27 17:07 ` Sudeep Holla 2019-06-26 17:02 ` Sudeep Holla 2019-06-26 17:02 ` Sudeep Holla 2019-06-26 17:02 ` Sudeep Holla
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190606142056.68272dc0@donnerap.cambridge.arm.com \ --to=andre.przywara@arm.com \ --cc=devicetree@vger.kernel.org \ --cc=f.fainelli@gmail.com \ --cc=festevam@gmail.com \ --cc=jassisinghbrar@gmail.com \ --cc=kernel@pengutronix.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=peng.fan@nxp.com \ --cc=robh+dt@kernel.org \ --cc=shawnguo@kernel.org \ --cc=sudeep.holla@arm.com \ --cc=van.freenix@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.