[V2,2/2] mailbox: introduce ARM SMC based mailbox

Commit Message

Peng Fan June 3, 2019, 8:30 a.m. UTC

From: Peng Fan <[e-mail]>

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 <[e-mail]>
Signed-off-by: Peng Fan <[e-mail]>
---

V2:
 Add interrupts notification support.

 drivers/mailbox/Kconfig                 |   7 ++
 drivers/mailbox/Makefile                |   2 +
 drivers/mailbox/arm-smc-mailbox.c       | 190 ++++++++++++++++++++++++++++++++
 include/linux/mailbox/arm-smc-mailbox.h |  10 ++
 4 files changed, 209 insertions(+)
 create mode 100644 drivers/mailbox/arm-smc-mailbox.c
 create mode 100644 include/linux/mailbox/arm-smc-mailbox.h

Comments

Florian Fainelli June 3, 2019, 4:32 p.m. UTC | #1

On 6/3/19 1:30 AM, [e-mail] wrote:
> From: Peng Fan <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Peng Fan <[e-mail]>
> ---

[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);
> +			return -EINVAL;
> +		}
> +	}

Should not the upper bound check be done against UINT_MAX since val is
an unsigned int?

> +
> +	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.

> +		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?

Peng Fan June 6, 2019, 3:35 a.m. UTC | #2

> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On 6/3/19 1:30 AM, [e-mail] wrote:
> > From: Peng Fan <[e-mail]>
> >
> > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%7
> Cpeng.fa
> >
> n%40nxp.com%7Caa396ba11ba244111fe408d6e8411fba%7C686ea1d3bc2b4
> c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C636951763738548621&amp;sdata=UlNESNg7I7
> 4TM9xp%2F
> > VMce4CSbMuJ95lh68cQw%2FnQMOw%3D&amp;reserved=0
> >
> > Cc: Andre Przywara <[e-mail]>
> > Signed-off-by: Peng Fan <[e-mail]>
> > ---
> 
> [snip]
> 
> +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> 
> That flag appears unused.

I'll remove this in V3.

> 
> > +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);
> > +			return -EINVAL;
> > +		}
> > +	}
> 
> Should not the upper bound check be done against UINT_MAX since val is an
> unsigned int?

Fix in V3.

> 
> > +
> > +	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.

Fix in V3.

> 
> > +		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?

When arm_smc_send_data, it will check ARM_SMC_MBOX_USE_HVC,
you mean there are other places needs this flag check?

> 
> [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.

Right.

> Since you use devm_* functions for everything, you may even remove that
> call.

You mean remove " platform_set_drvdata(pdev, mbox);" ?

> 
> [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?

This was to address comments here:
https://lore.kernel.org/patchwork/patch/812999/#1010433

Thanks,
Peng.

> --
> Florian

Andre Przywara June 6, 2019, 1:20 p.m. UTC | #3

On Mon, 3 Jun 2019 09:32:42 -0700
Florian Fainelli <[e-mail]> wrote:

Hi,

> On 6/3/19 1:30 AM, [e-mail] wrote:
> > From: Peng Fan <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Signed-off-by: Peng Fan <[e-mail]>
> > ---  
> 
> [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.

Peng Fan June 10, 2019, 1:32 a.m. UTC | #4

Hi Andre,
> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Mon, 3 Jun 2019 09:32:42 -0700
> Florian Fainelli <[e-mail]> wrote:
> 
> Hi,
> 
> > On 6/3/19 1:30 AM, [e-mail] wrote:
> > > From: Peng Fan <[e-mail]>
> > >
> > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> > >
> re.kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%
> 7Cpen
> > >
> g.fan%40nxp.com%7C15c4180b8fe5405d3de808d6ea81d5f1%7C686ea1d3bc
> 2b4c6
> > >
> fa92cd99c5c301635%7C0%7C0%7C636954240720601454&amp;sdata=1Cp
> WSgTH7lF
> > > cBKxJnLeIDw%2FDAQJJO%2FVypV1LUU1BRQA%3D&amp;reserved=0
> > >
> > > Cc: Andre Przywara <[e-mail]>
> > > Signed-off-by: Peng Fan <[e-mail]>
> > > ---
> >
> > [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?

I'll remove 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) {
> ...

make sense.

> 
> > > +
> > > +	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? 

If there is interrupt property, the interrupts should match channel counts.

Do we actually
> need one interrupt per channel?

I thought about this, provide one interrupt for all channels.
But there is no good way to let interrupt handlers know which
channel triggers the interrupt. So I use 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?

I could move it to driver code. Jassi, do you have any comments?

Thanks,
Peng.

> 
> Cheers,
> Andre.

Andre Przywara June 10, 2019, 10 a.m. UTC | #5

On Mon, 10 Jun 2019 01:32:49 +0000
Peng Fan <[e-mail]> wrote:

Hi Peng,

[ ... ]

> > > > +
> > > > +	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?   
> 
> If there is interrupt property, the interrupts should match channel counts.
> 
> Do we actually
> > need one interrupt per channel?  
> 
> I thought about this, provide one interrupt for all channels.
> But there is no good way to let interrupt handlers know which
> channel triggers the interrupt. So I use one interrupt per channel.

Yeah, I was wondering about this as well. Seems like we need this indeed.
Just sounds wasteful, but I guess we don't expect many channels anyway,
normally.

Cheers,
Andre.

Peng Fan June 12, 2019, 12:59 p.m. UTC | #6

Hi Andre,

> Subject: RE: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> Hi Andre,
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Mon, 3 Jun 2019 09:32:42 -0700
> > Florian Fainelli <[e-mail]> wrote:
> >
> > Hi,
> >
> > > On 6/3/19 1:30 AM, [e-mail] wrote:
> > > > From: Peng Fan <[e-mail]>
> > > >
> > > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > lo
> > > >
> >
> re.kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%
> > 7Cpen
> > > >
> >
> g.fan%40nxp.com%7C15c4180b8fe5405d3de808d6ea81d5f1%7C686ea1d3bc
> > 2b4c6
> > > >
> > fa92cd99c5c301635%7C0%7C0%7C636954240720601454&amp;sdata=1Cp
> > WSgTH7lF
> > > > cBKxJnLeIDw%2FDAQJJO%2FVypV1LUU1BRQA%3D&amp;reserved=0
> > > >
> > > > Cc: Andre Przywara <[e-mail]>
> > > > Signed-off-by: Peng Fan <[e-mail]>
> > > > ---
> > >
> > > [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?
> 
> I'll remove 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) {
> > ...
> 
> make sense.
> 
> >
> > > > +
> > > > +	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?
> 
> If there is interrupt property, the interrupts should match channel counts.
> 
> Do we actually
> > need one interrupt per channel?
> 
> I thought about this, provide one interrupt for all channels.
> But there is no good way to let interrupt handlers know which channel
> triggers the interrupt. So I use 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.

Long on ARM64 is 64bit, and 32bit on ARM32, so I use long.
Do you forsee any issues?

> >
> > Also, looking at the SMCCC, I only see six parameters in addition to
> > the function identifier. Shall we reflect this here?

a0 is used as function id, not no arm,func-ids provided in dts. a1-a7 are
also passed to smc.
If arm,func-ids is provided, a0 will be omitted just for consistency as above.

You mean write comments in the code for it?

Thanks,
Peng.

> 
> I could move it to driver code. Jassi, do you have any comments?
> 
> Thanks,
> Peng.
> 
> >
> > Cheers,
> > Andre.

Andre Przywara June 12, 2019, 5:18 p.m. UTC | #7

On Wed, 12 Jun 2019 12:59:04 +0000
Peng Fan <[e-mail]> wrote:

Hi,

> > Subject: RE: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > 
> > Hi Andre,  
> > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > >
> > > On Mon, 3 Jun 2019 09:32:42 -0700
> > > Florian Fainelli <[e-mail]> wrote:
> > >
> > > Hi,
> > >  
> > > > On 6/3/19 1:30 AM, [e-mail] wrote:  
> > > > > From: Peng Fan <[e-mail]>
> > > > >
> > > > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > > lo
> > > > >  
> > >  
> > re.kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%  
> > > 7Cpen  
> > > > >  
> > >  
> > g.fan%40nxp.com%7C15c4180b8fe5405d3de808d6ea81d5f1%7C686ea1d3bc  
> > > 2b4c6  
> > > > >  
> > > fa92cd99c5c301635%7C0%7C0%7C636954240720601454&amp;sdata=1Cp
> > > WSgTH7lF  
> > > > > cBKxJnLeIDw%2FDAQJJO%2FVypV1LUU1BRQA%3D&amp;reserved=0
> > > > >
> > > > > Cc: Andre Przywara <[e-mail]>
> > > > > Signed-off-by: Peng Fan <[e-mail]>
> > > > > ---  
> > > >
> > > > [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?  
> > 
> > I'll remove 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) {
> > > ...  
> > 
> > make sense.
> >   
> > >  
> > > > > +
> > > > > +	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?  
> > 
> > If there is interrupt property, the interrupts should match channel counts.
> > 
> > Do we actually  
> > > need one interrupt per channel?  
> > 
> > I thought about this, provide one interrupt for all channels.
> > But there is no good way to let interrupt handlers know which channel
> > triggers the interrupt. So I use 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.  
> 
> Long on ARM64 is 64bit, and 32bit on ARM32, so I use long.
> Do you forsee any issues?

Yes, because having different sizes depending on the underlying instruction set asks for trouble when talking about protocols. If I compile a kernel with this driver once for arm(32) and once for arm64 and run it on the same machine with the same firmware, does that behave differently? Not saying it's impossible to handle, but we should make sure there is no ambiguity.

> > > Also, looking at the SMCCC, I only see six parameters in addition to
> > > the function identifier. Shall we reflect this here?  
> 
> a0 is used as function id, not no arm,func-ids provided in dts. a1-a7 are
> also passed to smc.

Yes, so those are *7* parameters on top of the function IDs, whereas the SMCCC only speaks of 6:
"When the SMC64/HVC64 convention is used, the SMC or HVC instruction takes up to six 64-bit arguments in
registers ..."
"When the SMC32/HVC32 convention is used, an SMC or HVC instruction takes a Function Identifier and up to six
32-bit register values as arguments, ..."

> If arm,func-ids is provided, a0 will be omitted just for consistency as above.
> 
> You mean write comments in the code for it?

I think we should prevent people expecting anything useful to happen with the seventh argument.

Cheers,
Andre

Sudeep Holla June 20, 2019, 9:23 a.m. UTC | #8

On Mon, Jun 03, 2019 at 04:30:05PM +0800, [e-mail] wrote:
> From: Peng Fan <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Peng Fan <[e-mail]>
> ---
>
> V2:
>  Add interrupts notification support.
>
>  drivers/mailbox/Kconfig                 |   7 ++
>  drivers/mailbox/Makefile                |   2 +
>  drivers/mailbox/arm-smc-mailbox.c       | 190 ++++++++++++++++++++++++++++++++
>  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
>  4 files changed, 209 insertions(+)
>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>  create mode 100644 include/linux/mailbox/arm-smc-mailbox.h
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 595542bfae85..c3bd0f1ddcd8 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,13 @@ config ARM_MHU
>  	  The controller has 3 mailbox channels, the last of which can be
>  	  used in Secure mode only.
>
> +config ARM_SMC_MBOX
> +	tristate "Generic ARM smc mailbox"
> +	depends on OF && HAVE_ARM_SMCCC
> +	help
> +	  Generic mailbox driver which uses ARM smc calls to call into
> +	  firmware for triggering mailboxes.
> +
>  config IMX_MBOX
>  	tristate "i.MX Mailbox"
>  	depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index c22fad6f696b..93918a84c91b 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
>
>  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
>
> +obj-$(CONFIG_ARM_SMC_MBOX)	+= arm-smc-mailbox.o
> +
>  obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
>
>  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)	+= armada-37xx-rwtm-mailbox.o
> diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
> new file mode 100644
> index 000000000000..fef6e38d8b98
> --- /dev/null
> +++ b/drivers/mailbox/arm-smc-mailbox.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2016,2017 ARM Ltd.
> + * Copyright 2019 NXP
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox/arm-smc-mailbox.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define ARM_SMC_MBOX_USE_HVC	BIT(0)
> +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> +
> +struct arm_smc_chan_data {
> +	u32 function_id;
> +	u32 flags;
> +	int irq;
> +};
> +
> +static int arm_smc_send_data(struct mbox_chan *link, void *data)
> +{
> +	struct arm_smc_chan_data *chan_data = link->con_priv;
> +	struct arm_smccc_mbox_cmd *cmd = data;
> +	struct arm_smccc_res res;
> +	u32 function_id;
> +
> +	if (chan_data->function_id != UINT_MAX)
> +		function_id = chan_data->function_id;
> +	else
> +		function_id = cmd->a0;
> +
> +	if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> +		arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +			      cmd->a5, cmd->a6, cmd->a7, &res);
> +	else
> +		arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +			      cmd->a5, cmd->a6, cmd->a7, &res);
> +

So how will the SMC/HVC handler in EL3/2 find which mailbox is being referred
with this command ? I prefer 2nd argument to be the mailbox number.

--
Regards,
Sudeep

Peng Fan June 20, 2019, 10:21 a.m. UTC | #9

Hi Sudeep,

> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Mon, Jun 03, 2019 at 04:30:05PM +0800, [e-mail] wrote:
> > From: Peng Fan <[e-mail]>
> >
> > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%7
> Cpeng.fa
> >
> n%40nxp.com%7C6b37f78032e446be750e08d6f560e707%7C686ea1d3bc2b4
> c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C636966193913988679&amp;sdata=UNM4MTPs
> brqoMqWStEy
> > YzzwMEWTmX7hHO3TeNEz%2BOAw%3D&amp;reserved=0
> >
> > Cc: Andre Przywara <[e-mail]>
> > Signed-off-by: Peng Fan <[e-mail]>
> > ---
> >
> > V2:
> >  Add interrupts notification support.
> >
> >  drivers/mailbox/Kconfig                 |   7 ++
> >  drivers/mailbox/Makefile                |   2 +
> >  drivers/mailbox/arm-smc-mailbox.c       | 190
> ++++++++++++++++++++++++++++++++
> >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> >  4 files changed, 209 insertions(+)
> >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
> > 100644 include/linux/mailbox/arm-smc-mailbox.h
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > 595542bfae85..c3bd0f1ddcd8 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -15,6 +15,13 @@ config ARM_MHU
> >  	  The controller has 3 mailbox channels, the last of which can be
> >  	  used in Secure mode only.
> >
> > +config ARM_SMC_MBOX
> > +	tristate "Generic ARM smc mailbox"
> > +	depends on OF && HAVE_ARM_SMCCC
> > +	help
> > +	  Generic mailbox driver which uses ARM smc calls to call into
> > +	  firmware for triggering mailboxes.
> > +
> >  config IMX_MBOX
> >  	tristate "i.MX Mailbox"
> >  	depends on ARCH_MXC || COMPILE_TEST
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > c22fad6f696b..93918a84c91b 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> >
> >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> >
> > +obj-$(CONFIG_ARM_SMC_MBOX)	+= arm-smc-mailbox.o
> > +
> >  obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> >
> >  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)	+=
> armada-37xx-rwtm-mailbox.o
> > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > b/drivers/mailbox/arm-smc-mailbox.c
> > new file mode 100644
> > index 000000000000..fef6e38d8b98
> > --- /dev/null
> > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > @@ -0,0 +1,190 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2016,2017 ARM Ltd.
> > + * Copyright 2019 NXP
> > + */
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mailbox_controller.h> #include
> > +<linux/mailbox/arm-smc-mailbox.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define ARM_SMC_MBOX_USE_HVC	BIT(0)
> > +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> > +
> > +struct arm_smc_chan_data {
> > +	u32 function_id;
> > +	u32 flags;
> > +	int irq;
> > +};
> > +
> > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > +	struct arm_smc_chan_data *chan_data = link->con_priv;
> > +	struct arm_smccc_mbox_cmd *cmd = data;
> > +	struct arm_smccc_res res;
> > +	u32 function_id;
> > +
> > +	if (chan_data->function_id != UINT_MAX)
> > +		function_id = chan_data->function_id;
> > +	else
> > +		function_id = cmd->a0;
> > +
> > +	if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > +		arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3,
> cmd->a4,
> > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > +	else
> > +		arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3,
> cmd->a4,
> > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > +
> 
> So how will the SMC/HVC handler in EL3/2 find which mailbox is being
> referred with this command ? I prefer 2nd argument to be the mailbox
> number.
You mean channel number as following?

@@ -37,10 +38,10 @@ static int arm_smc_send_data(struct mbox_chan *link, void *data)
                function_id = cmd->a0;

        if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
-               arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
+               arm_smccc_hvc(function_id, chan_data->chan_id, cmd->a2, cmd->a3, cmd->a4,
                              cmd->a5, cmd->a6, cmd->a7, &res);
        else
-               arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
+               arm_smccc_smc(function_id, chan_data->chan_id, cmd->a2, cmd->a3, cmd->a4,
                              cmd->a5, cmd->a6, cmd->a7, &res);

Or should that be passed from firmware driver?

If not from firmware driver, just as above, I do not have a good idea which should be passed to smc,
from cmd->a1 to a5 or from cmd->a2 to a6.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep

Sudeep Holla June 20, 2019, 11:15 a.m. UTC | #10

On Thu, Jun 20, 2019 at 10:21:09AM +0000, Peng Fan wrote:
> Hi Sudeep,
>
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Mon, Jun 03, 2019 at 04:30:05PM +0800, [e-mail] wrote:
> > > From: Peng Fan <[e-mail]>
> > >
> > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%7
> > Cpeng.fa
> > >
> > n%40nxp.com%7C6b37f78032e446be750e08d6f560e707%7C686ea1d3bc2b4
> > c6fa92cd
> > >
> > 99c5c301635%7C0%7C0%7C636966193913988679&amp;sdata=UNM4MTPs
> > brqoMqWStEy
> > > YzzwMEWTmX7hHO3TeNEz%2BOAw%3D&amp;reserved=0
> > >
> > > Cc: Andre Przywara <[e-mail]>
> > > Signed-off-by: Peng Fan <[e-mail]>
> > > ---
> > >
> > > V2:
> > >  Add interrupts notification support.
> > >
> > >  drivers/mailbox/Kconfig                 |   7 ++
> > >  drivers/mailbox/Makefile                |   2 +
> > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > ++++++++++++++++++++++++++++++++
> > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > >  4 files changed, 209 insertions(+)
> > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
> > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > 595542bfae85..c3bd0f1ddcd8 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -15,6 +15,13 @@ config ARM_MHU
> > >  	  The controller has 3 mailbox channels, the last of which can be
> > >  	  used in Secure mode only.
> > >
> > > +config ARM_SMC_MBOX
> > > +	tristate "Generic ARM smc mailbox"
> > > +	depends on OF && HAVE_ARM_SMCCC
> > > +	help
> > > +	  Generic mailbox driver which uses ARM smc calls to call into
> > > +	  firmware for triggering mailboxes.
> > > +
> > >  config IMX_MBOX
> > >  	tristate "i.MX Mailbox"
> > >  	depends on ARCH_MXC || COMPILE_TEST
> > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > > c22fad6f696b..93918a84c91b 100644
> > > --- a/drivers/mailbox/Makefile
> > > +++ b/drivers/mailbox/Makefile
> > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> > >
> > >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> > >
> > > +obj-$(CONFIG_ARM_SMC_MBOX)	+= arm-smc-mailbox.o
> > > +
> > >  obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> > >
> > >  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)	+=
> > armada-37xx-rwtm-mailbox.o
> > > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > > b/drivers/mailbox/arm-smc-mailbox.c
> > > new file mode 100644
> > > index 000000000000..fef6e38d8b98
> > > --- /dev/null
> > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > @@ -0,0 +1,190 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2016,2017 ARM Ltd.
> > > + * Copyright 2019 NXP
> > > + */
> > > +
> > > +#include <linux/arm-smccc.h>
> > > +#include <linux/device.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/mailbox_controller.h> #include
> > > +<linux/mailbox/arm-smc-mailbox.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#define ARM_SMC_MBOX_USE_HVC	BIT(0)
> > > +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> > > +
> > > +struct arm_smc_chan_data {
> > > +	u32 function_id;
> > > +	u32 flags;
> > > +	int irq;
> > > +};
> > > +
> > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > +	struct arm_smc_chan_data *chan_data = link->con_priv;
> > > +	struct arm_smccc_mbox_cmd *cmd = data;
> > > +	struct arm_smccc_res res;
> > > +	u32 function_id;
> > > +
> > > +	if (chan_data->function_id != UINT_MAX)
> > > +		function_id = chan_data->function_id;
> > > +	else
> > > +		function_id = cmd->a0;
> > > +
> > > +	if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > > +		arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3,
> > cmd->a4,
> > > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > > +	else
> > > +		arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3,
> > cmd->a4,
> > > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > > +
> >
> > So how will the SMC/HVC handler in EL3/2 find which mailbox is being
> > referred with this command ? I prefer 2nd argument to be the mailbox
> > number.
> You mean channel number as following?
>
> @@ -37,10 +38,10 @@ static int arm_smc_send_data(struct mbox_chan *link, void *data)
>                 function_id = cmd->a0;
>
>         if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> -               arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +               arm_smccc_hvc(function_id, chan_data->chan_id, cmd->a2, cmd->a3, cmd->a4,
>                               cmd->a5, cmd->a6, cmd->a7, &res);
>         else
> -               arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +               arm_smccc_smc(function_id, chan_data->chan_id, cmd->a2, cmd->a3, cmd->a4,
>                               cmd->a5, cmd->a6, cmd->a7, &res);
>

Yes something like above. There's a brief description of the same in
latest SCMI specification though it's not related to SCMI, it more 
general note for SMC based mailbox.

"In case the doorbell is SMC/HVC based, it should follow the SMC Calling
Convention [SMCCC] and needs to provide the identifier of the Shared Memory
area that contains the payload. On return from the call, the Shared Memory
area which contained the payload is now updated with the SCMI return response.
The identifier of the Shared Memory area should be 32-bits and each identifier
should denote a distinct Shared Memory area."

> Or should that be passed from firmware driver?
>

No, we can't assume the id's in DT are 1-1 translation to mailbox ID used
though it may be the same most of the time.

> If not from firmware driver, just as above, I do not have a good idea which
> should be passed to smc, from cmd->a1 to a5 or from cmd->a2 to a6.
>

Also I found copying those registers may not be always needed and can
be sub-optimal. May be a way to indicate that this in DT whether
register based transfers are used or using memory. Just a thought.

--
Regards,
Sudeep

Jassi Brar June 20, 2019, 4:50 p.m. UTC | #11

On Mon, Jun 3, 2019 at 3:28 AM <[e-mail]> wrote:
>
> From: Peng Fan <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Peng Fan <[e-mail]>
> ---
>
> V2:
>  Add interrupts notification support.
>
>  drivers/mailbox/Kconfig                 |   7 ++
>  drivers/mailbox/Makefile                |   2 +
>  drivers/mailbox/arm-smc-mailbox.c       | 190 ++++++++++++++++++++++++++++++++
>  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
>  4 files changed, 209 insertions(+)
>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>  create mode 100644 include/linux/mailbox/arm-smc-mailbox.h
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 595542bfae85..c3bd0f1ddcd8 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,13 @@ config ARM_MHU
>           The controller has 3 mailbox channels, the last of which can be
>           used in Secure mode only.
>
> +config ARM_SMC_MBOX
> +       tristate "Generic ARM smc mailbox"
> +       depends on OF && HAVE_ARM_SMCCC
> +       help
> +         Generic mailbox driver which uses ARM smc calls to call into
> +         firmware for triggering mailboxes.
> +
>  config IMX_MBOX
>         tristate "i.MX Mailbox"
>         depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index c22fad6f696b..93918a84c91b 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)      += mailbox-test.o
>
>  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
>
> +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
> +
>  obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
>
>  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)    += armada-37xx-rwtm-mailbox.o
> diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
> new file mode 100644
> index 000000000000..fef6e38d8b98
> --- /dev/null
> +++ b/drivers/mailbox/arm-smc-mailbox.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2016,2017 ARM Ltd.
> + * Copyright 2019 NXP
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox/arm-smc-mailbox.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> +
IRQ bit is unused (and unnecessary IMO)

> +struct arm_smc_chan_data {
> +       u32 function_id;
> +       u32 flags;
> +       int irq;
> +};
> +
> +static int arm_smc_send_data(struct mbox_chan *link, void *data)
> +{
> +       struct arm_smc_chan_data *chan_data = link->con_priv;
> +       struct arm_smccc_mbox_cmd *cmd = data;
> +       struct arm_smccc_res res;
> +       u32 function_id;
> +
> +       if (chan_data->function_id != UINT_MAX)
> +               function_id = chan_data->function_id;
> +       else
> +               function_id = cmd->a0;
> +
Not sure about chan_data->function_id.  Why restrict from DT?
'a0' is the function_id register, let the user pass func-id via the
'a0' like other values via 'a[1-7]'


> +       if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> +               arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +                             cmd->a5, cmd->a6, cmd->a7, &res);
> +       else
> +               arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> +                             cmd->a5, cmd->a6, cmd->a7, &res);
> +
> +       if (chan_data->irq)
> +               return 0;
> +
This irq thing seems like oob signalling, that is, a protocol thing.
And then it provides lesser info via chan_irq_handler (returns NULL)
than res.a0 - which can always be ignored if not needed.
So the irq should be implemented in the upper layer if the protocol needs it.

> +       mbox_chan_received_data(link, (void *)res.a0);
> +
This is fine.

Peng Fan June 25, 2019, 7:20 a.m. UTC | #12

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:[e-mail]]
> Sent: 2019年6月21日 0:50
> To: Peng Fan <[e-mail]>
> Cc: Rob Herring <[e-mail]>; Mark Rutland
> <[e-mail]>; Sudeep Holla <[e-mail]>; Florian
> Fainelli <[e-mail]>; , Sascha Hauer <[e-mail]>;
> dl-linux-imx <[e-mail]>; Shawn Guo <[e-mail]>;
> [e-mail]; Devicetree List <[e-mail]>; Linux
> Kernel Mailing List <[e-mail]>;
> [e-mail]; Andre Przywara
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Mon, Jun 3, 2019 at 3:28 AM <[e-mail]> wrote:
> >
> > From: Peng Fan <[e-mail]>
> >
> > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%7
> Cpeng.fa
> >
> n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> ZkeRMtL8Bx
> > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> >
> > Cc: Andre Przywara <[e-mail]>
> > Signed-off-by: Peng Fan <[e-mail]>
> > ---
> >
> > V2:
> >  Add interrupts notification support.
> >
> >  drivers/mailbox/Kconfig                 |   7 ++
> >  drivers/mailbox/Makefile                |   2 +
> >  drivers/mailbox/arm-smc-mailbox.c       | 190
> ++++++++++++++++++++++++++++++++
> >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> >  4 files changed, 209 insertions(+)
> >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
> > 100644 include/linux/mailbox/arm-smc-mailbox.h
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > 595542bfae85..c3bd0f1ddcd8 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -15,6 +15,13 @@ config ARM_MHU
> >           The controller has 3 mailbox channels, the last of which can be
> >           used in Secure mode only.
> >
> > +config ARM_SMC_MBOX
> > +       tristate "Generic ARM smc mailbox"
> > +       depends on OF && HAVE_ARM_SMCCC
> > +       help
> > +         Generic mailbox driver which uses ARM smc calls to call into
> > +         firmware for triggering mailboxes.
> > +
> >  config IMX_MBOX
> >         tristate "i.MX Mailbox"
> >         depends on ARCH_MXC || COMPILE_TEST diff --git
> > a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > c22fad6f696b..93918a84c91b 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)      += mailbox-test.o
> >
> >  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
> >
> > +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
> > +
> >  obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> >
> >  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)    +=
> armada-37xx-rwtm-mailbox.o
> > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > b/drivers/mailbox/arm-smc-mailbox.c
> > new file mode 100644
> > index 000000000000..fef6e38d8b98
> > --- /dev/null
> > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > @@ -0,0 +1,190 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2016,2017 ARM Ltd.
> > + * Copyright 2019 NXP
> > + */
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mailbox_controller.h> #include
> > +<linux/mailbox/arm-smc-mailbox.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > +
> IRQ bit is unused (and unnecessary IMO)

This will be removed in next version.

> 
> > +struct arm_smc_chan_data {
> > +       u32 function_id;
> > +       u32 flags;
> > +       int irq;
> > +};
> > +
> > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > +       struct arm_smc_chan_data *chan_data = link->con_priv;
> > +       struct arm_smccc_mbox_cmd *cmd = data;
> > +       struct arm_smccc_res res;
> > +       u32 function_id;
> > +
> > +       if (chan_data->function_id != UINT_MAX)
> > +               function_id = chan_data->function_id;
> > +       else
> > +               function_id = cmd->a0;
> > +
> Not sure about chan_data->function_id.  Why restrict from DT?
> 'a0' is the function_id register, let the user pass func-id via the 'a0' like other
> values via 'a[1-7]'
> 
> 
> > +       if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > +               arm_smccc_hvc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +                             cmd->a5, cmd->a6, cmd->a7, &res);
> > +       else
> > +               arm_smccc_smc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +                             cmd->a5, cmd->a6, cmd->a7, &res);
> > +
> > +       if (chan_data->irq)
> > +               return 0;
> > +
> This irq thing seems like oob signalling, that is, a protocol thing.
> And then it provides lesser info via chan_irq_handler (returns NULL) than
> res.a0 - which can always be ignored if not needed.
> So the irq should be implemented in the upper layer if the protocol needs it.

The interrupts was added here because in v1, Florian suggest
"
I would just put a
provision in the binding to support an optional interrupt such that
asynchronism gets reasonably easy to plug in when it is available (and
desirable).
"

So I introduced interrupt in V2. In my testcase, after smc call done,
it means firmware->smc mailbox->firmware done. Interrupt notification
from firmware->Linux, means firmware has done the operation.

When using interrupts, we could not know res.a0 as smc sync call.

Interrupts is not a must in my testcase, Florian, Andre, do you have
any comments? Should I keep interrupts in V3 or drop it as Jassi comments?

Thanks,
Peng.

> 
> > +       mbox_chan_received_data(link, (void *)res.a0);
> > +
> This is fine.

Peng Fan June 25, 2019, 7:28 a.m. UTC | #13

Hi Sudeep,

> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Thu, Jun 20, 2019 at 10:21:09AM +0000, Peng Fan wrote:
> > Hi Sudeep,
> >
> > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > >
> > > On Mon, Jun 03, 2019 at 04:30:05PM +0800, [e-mail] wrote:
> > > > From: Peng Fan <[e-mail]>
> > > >
> > > > 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%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C0
> 1%7
> > > Cpeng.fa
> > > >
> > >
> n%40nxp.com%7C6b37f78032e446be750e08d6f560e707%7C686ea1d3bc2b4
> > > c6fa92cd
> > > >
> > >
> 99c5c301635%7C0%7C0%7C636966193913988679&amp;sdata=UNM4MTPs
> > > brqoMqWStEy
> > > > YzzwMEWTmX7hHO3TeNEz%2BOAw%3D&amp;reserved=0
> > > >
> > > > Cc: Andre Przywara <[e-mail]>
> > > > Signed-off-by: Peng Fan <[e-mail]>
> > > > ---
> > > >
> > > > V2:
> > > >  Add interrupts notification support.
> > > >
> > > >  drivers/mailbox/Kconfig                 |   7 ++
> > > >  drivers/mailbox/Makefile                |   2 +
> > > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > > ++++++++++++++++++++++++++++++++
> > > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > > >  4 files changed, 209 insertions(+)
> > > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create
> mode
> > > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > > >
> > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > 595542bfae85..c3bd0f1ddcd8 100644
> > > > --- a/drivers/mailbox/Kconfig
> > > > +++ b/drivers/mailbox/Kconfig
> > > > @@ -15,6 +15,13 @@ config ARM_MHU
> > > >  	  The controller has 3 mailbox channels, the last of which can be
> > > >  	  used in Secure mode only.
> > > >
> > > > +config ARM_SMC_MBOX
> > > > +	tristate "Generic ARM smc mailbox"
> > > > +	depends on OF && HAVE_ARM_SMCCC
> > > > +	help
> > > > +	  Generic mailbox driver which uses ARM smc calls to call into
> > > > +	  firmware for triggering mailboxes.
> > > > +
> > > >  config IMX_MBOX
> > > >  	tristate "i.MX Mailbox"
> > > >  	depends on ARCH_MXC || COMPILE_TEST
> > > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > > > c22fad6f696b..93918a84c91b 100644
> > > > --- a/drivers/mailbox/Makefile
> > > > +++ b/drivers/mailbox/Makefile
> > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> > > >
> > > >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> > > >
> > > > +obj-$(CONFIG_ARM_SMC_MBOX)	+= arm-smc-mailbox.o
> > > > +
> > > >  obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> > > >
> > > >  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)	+=
> > > armada-37xx-rwtm-mailbox.o
> > > > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > > > b/drivers/mailbox/arm-smc-mailbox.c
> > > > new file mode 100644
> > > > index 000000000000..fef6e38d8b98
> > > > --- /dev/null
> > > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > > @@ -0,0 +1,190 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (C) 2016,2017 ARM Ltd.
> > > > + * Copyright 2019 NXP
> > > > + */
> > > > +
> > > > +#include <linux/arm-smccc.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/mailbox_controller.h> #include
> > > > +<linux/mailbox/arm-smc-mailbox.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/platform_device.h>
> > > > +
> > > > +#define ARM_SMC_MBOX_USE_HVC	BIT(0)
> > > > +#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
> > > > +
> > > > +struct arm_smc_chan_data {
> > > > +	u32 function_id;
> > > > +	u32 flags;
> > > > +	int irq;
> > > > +};
> > > > +
> > > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > > +	struct arm_smc_chan_data *chan_data = link->con_priv;
> > > > +	struct arm_smccc_mbox_cmd *cmd = data;
> > > > +	struct arm_smccc_res res;
> > > > +	u32 function_id;
> > > > +
> > > > +	if (chan_data->function_id != UINT_MAX)
> > > > +		function_id = chan_data->function_id;
> > > > +	else
> > > > +		function_id = cmd->a0;
> > > > +
> > > > +	if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > > > +		arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3,
> > > cmd->a4,
> > > > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > > > +	else
> > > > +		arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3,
> > > cmd->a4,
> > > > +			      cmd->a5, cmd->a6, cmd->a7, &res);
> > > > +
> > >
> > > So how will the SMC/HVC handler in EL3/2 find which mailbox is being
> > > referred with this command ? I prefer 2nd argument to be the mailbox
> > > number.
> > You mean channel number as following?
> >
> > @@ -37,10 +38,10 @@ static int arm_smc_send_data(struct mbox_chan
> *link, void *data)
> >                 function_id = cmd->a0;
> >
> >         if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > -               arm_smccc_hvc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +               arm_smccc_hvc(function_id, chan_data->chan_id,
> cmd->a2, cmd->a3, cmd->a4,
> >                               cmd->a5, cmd->a6, cmd->a7, &res);
> >         else
> > -               arm_smccc_smc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +               arm_smccc_smc(function_id, chan_data->chan_id,
> cmd->a2, cmd->a3, cmd->a4,
> >                               cmd->a5, cmd->a6, cmd->a7, &res);
> >
> 
> Yes something like above. There's a brief description of the same in
> latest SCMI specification though it's not related to SCMI, it more
> general note for SMC based mailbox.
> 
> "In case the doorbell is SMC/HVC based, it should follow the SMC Calling
> Convention [SMCCC] and needs to provide the identifier of the Shared
> Memory
> area that contains the payload. On return from the call, the Shared Memory
> area which contained the payload is now updated with the SCMI return
> response.
> The identifier of the Shared Memory area should be 32-bits and each
> identifier
> should denote a distinct Shared Memory area."

Thanks for the info, it make sense to pass channel id to firmware.

> 
> > Or should that be passed from firmware driver?
> >
> 
> No, we can't assume the id's in DT are 1-1 translation to mailbox ID used
> though it may be the same most of the time.

Understand.

> 
> > If not from firmware driver, just as above, I do not have a good idea which
> > should be passed to smc, from cmd->a1 to a5 or from cmd->a2 to a6.
> >
> 
> Also I found copying those registers may not be always needed and can
> be sub-optimal. May be a way to indicate that this in DT whether
> register based transfers are used or using memory. Just a thought.

"reg-transport" or "mem-transport" should help here.

Thanks,
peng.

> 
> --
> Regards,
> Sudeep

Peng Fan June 25, 2019, 7:30 a.m. UTC | #14

Hi Jassi

> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Mon, Jun 3, 2019 at 3:28 AM <[e-mail]> wrote:
> >
> > From: Peng Fan <[e-mail]>
> >
> > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%7
> Cpeng.fa
> >
> n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> ZkeRMtL8Bx
> > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> >
> > Cc: Andre Przywara <[e-mail]>
> > Signed-off-by: Peng Fan <[e-mail]>
> > ---
> >
> > V2:
> >  Add interrupts notification support.
> >
> >  drivers/mailbox/Kconfig                 |   7 ++
> >  drivers/mailbox/Makefile                |   2 +
> >  drivers/mailbox/arm-smc-mailbox.c       | 190
> ++++++++++++++++++++++++++++++++
> >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> >  4 files changed, 209 insertions(+)
> >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
> > 100644 include/linux/mailbox/arm-smc-mailbox.h
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > 595542bfae85..c3bd0f1ddcd8 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -15,6 +15,13 @@ config ARM_MHU
> >           The controller has 3 mailbox channels, the last of which can be
> >           used in Secure mode only.
> >
> > +config ARM_SMC_MBOX
> > +       tristate "Generic ARM smc mailbox"
> > +       depends on OF && HAVE_ARM_SMCCC
> > +       help
> > +         Generic mailbox driver which uses ARM smc calls to call into
> > +         firmware for triggering mailboxes.
> > +
> >  config IMX_MBOX
> >         tristate "i.MX Mailbox"
> >         depends on ARCH_MXC || COMPILE_TEST diff --git
> > a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > c22fad6f696b..93918a84c91b 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)      += mailbox-test.o
> >
> >  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
> >
> > +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
> > +
> >  obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> >
> >  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)    +=
> armada-37xx-rwtm-mailbox.o
> > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > b/drivers/mailbox/arm-smc-mailbox.c
> > new file mode 100644
> > index 000000000000..fef6e38d8b98
> > --- /dev/null
> > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > @@ -0,0 +1,190 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2016,2017 ARM Ltd.
> > + * Copyright 2019 NXP
> > + */
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mailbox_controller.h> #include
> > +<linux/mailbox/arm-smc-mailbox.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > +
> IRQ bit is unused (and unnecessary IMO)
> 
> > +struct arm_smc_chan_data {
> > +       u32 function_id;
> > +       u32 flags;
> > +       int irq;
> > +};
> > +
> > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > +       struct arm_smc_chan_data *chan_data = link->con_priv;
> > +       struct arm_smccc_mbox_cmd *cmd = data;
> > +       struct arm_smccc_res res;
> > +       u32 function_id;
> > +
> > +       if (chan_data->function_id != UINT_MAX)
> > +               function_id = chan_data->function_id;
> > +       else
> > +               function_id = cmd->a0;
> > +
> Not sure about chan_data->function_id.  Why restrict from DT?
> 'a0' is the function_id register, let the user pass func-id via the 'a0' like other
> values via 'a[1-7]'

Missed to reply this comment.

The firmware driver might not have func-id, such as SCMI/SCPI.
So add an optional func-id to let smc mailbox driver could
use smc SiP func id.

Thanks,
Peng.

> 
> 
> > +       if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> > +               arm_smccc_hvc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +                             cmd->a5, cmd->a6, cmd->a7, &res);
> > +       else
> > +               arm_smccc_smc(function_id, cmd->a1, cmd->a2,
> cmd->a3, cmd->a4,
> > +                             cmd->a5, cmd->a6, cmd->a7, &res);
> > +
> > +       if (chan_data->irq)
> > +               return 0;
> > +
> This irq thing seems like oob signalling, that is, a protocol thing.
> And then it provides lesser info via chan_irq_handler (returns NULL) than
> res.a0 - which can always be ignored if not needed.
> So the irq should be implemented in the upper layer if the protocol needs it.
> 
> > +       mbox_chan_received_data(link, (void *)res.a0);
> > +
> This is fine.

Jassi Brar June 25, 2019, 2:36 p.m. UTC | #15

On Tue, Jun 25, 2019 at 2:30 AM Peng Fan <[e-mail]> wrote:
>
> Hi Jassi
>
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Mon, Jun 3, 2019 at 3:28 AM <[e-mail]> wrote:
> > >
> > > From: Peng Fan <[e-mail]>
> > >
> > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%7
> > Cpeng.fa
> > >
> > n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > c6fa92cd
> > >
> > 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> > ZkeRMtL8Bx
> > > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> > >
> > > Cc: Andre Przywara <[e-mail]>
> > > Signed-off-by: Peng Fan <[e-mail]>
> > > ---
> > >
> > > V2:
> > >  Add interrupts notification support.
> > >
> > >  drivers/mailbox/Kconfig                 |   7 ++
> > >  drivers/mailbox/Makefile                |   2 +
> > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > ++++++++++++++++++++++++++++++++
> > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > >  4 files changed, 209 insertions(+)
> > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
> > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > 595542bfae85..c3bd0f1ddcd8 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -15,6 +15,13 @@ config ARM_MHU
> > >           The controller has 3 mailbox channels, the last of which can be
> > >           used in Secure mode only.
> > >
> > > +config ARM_SMC_MBOX
> > > +       tristate "Generic ARM smc mailbox"
> > > +       depends on OF && HAVE_ARM_SMCCC
> > > +       help
> > > +         Generic mailbox driver which uses ARM smc calls to call into
> > > +         firmware for triggering mailboxes.
> > > +
> > >  config IMX_MBOX
> > >         tristate "i.MX Mailbox"
> > >         depends on ARCH_MXC || COMPILE_TEST diff --git
> > > a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > > c22fad6f696b..93918a84c91b 100644
> > > --- a/drivers/mailbox/Makefile
> > > +++ b/drivers/mailbox/Makefile
> > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)      += mailbox-test.o
> > >
> > >  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
> > >
> > > +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
> > > +
> > >  obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> > >
> > >  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)    +=
> > armada-37xx-rwtm-mailbox.o
> > > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > > b/drivers/mailbox/arm-smc-mailbox.c
> > > new file mode 100644
> > > index 000000000000..fef6e38d8b98
> > > --- /dev/null
> > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > @@ -0,0 +1,190 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2016,2017 ARM Ltd.
> > > + * Copyright 2019 NXP
> > > + */
> > > +
> > > +#include <linux/arm-smccc.h>
> > > +#include <linux/device.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/mailbox_controller.h> #include
> > > +<linux/mailbox/arm-smc-mailbox.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > > +
> > IRQ bit is unused (and unnecessary IMO)
> >
> > > +struct arm_smc_chan_data {
> > > +       u32 function_id;
> > > +       u32 flags;
> > > +       int irq;
> > > +};
> > > +
> > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > +       struct arm_smc_chan_data *chan_data = link->con_priv;
> > > +       struct arm_smccc_mbox_cmd *cmd = data;
> > > +       struct arm_smccc_res res;
> > > +       u32 function_id;
> > > +
> > > +       if (chan_data->function_id != UINT_MAX)
> > > +               function_id = chan_data->function_id;
> > > +       else
> > > +               function_id = cmd->a0;
> > > +
> > Not sure about chan_data->function_id.  Why restrict from DT?
> > 'a0' is the function_id register, let the user pass func-id via the 'a0' like other
> > values via 'a[1-7]'
>
> Missed to reply this comment.
>
> The firmware driver might not have func-id, such as SCMI/SCPI.
> So add an optional func-id to let smc mailbox driver could
> use smc SiP func id.
>
There is no end to conforming to protocols. Controller drivers should
be written having no particular client in mind.

Peng Fan June 26, 2019, 1:31 p.m. UTC | #16

Hi All,

> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Tue, Jun 25, 2019 at 2:30 AM Peng Fan <[e-mail]> wrote:
> >
> > Hi Jassi
> >
> > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > >
> > > On Mon, Jun 3, 2019 at 3:28 AM <[e-mail]> wrote:
> > > >
> > > > From: Peng Fan <[e-mail]>
> > > >
> > > > 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%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C0
> 1%7
> > > Cpeng.fa
> > > >
> > >
> n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > > c6fa92cd
> > > >
> > >
> 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> > > ZkeRMtL8Bx
> > > > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> > > >
> > > > Cc: Andre Przywara <[e-mail]>
> > > > Signed-off-by: Peng Fan <[e-mail]>
> > > > ---
> > > >
> > > > V2:
> > > >  Add interrupts notification support.
> > > >
> > > >  drivers/mailbox/Kconfig                 |   7 ++
> > > >  drivers/mailbox/Makefile                |   2 +
> > > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > > ++++++++++++++++++++++++++++++++
> > > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > > >  4 files changed, 209 insertions(+)
> > > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create
> mode
> > > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > > >
> > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > 595542bfae85..c3bd0f1ddcd8 100644
> > > > --- a/drivers/mailbox/Kconfig
> > > > +++ b/drivers/mailbox/Kconfig
> > > > @@ -15,6 +15,13 @@ config ARM_MHU
> > > >           The controller has 3 mailbox channels, the last of which can
> be
> > > >           used in Secure mode only.
> > > >
> > > > +config ARM_SMC_MBOX
> > > > +       tristate "Generic ARM smc mailbox"
> > > > +       depends on OF && HAVE_ARM_SMCCC
> > > > +       help
> > > > +         Generic mailbox driver which uses ARM smc calls to call into
> > > > +         firmware for triggering mailboxes.
> > > > +
> > > >  config IMX_MBOX
> > > >         tristate "i.MX Mailbox"
> > > >         depends on ARCH_MXC || COMPILE_TEST diff --git
> > > > a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > > > c22fad6f696b..93918a84c91b 100644
> > > > --- a/drivers/mailbox/Makefile
> > > > +++ b/drivers/mailbox/Makefile
> > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)      +=
> mailbox-test.o
> > > >
> > > >  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
> > > >
> > > > +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
> > > > +
> > > >  obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> > > >
> > > >  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)    +=
> > > armada-37xx-rwtm-mailbox.o
> > > > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > > > b/drivers/mailbox/arm-smc-mailbox.c
> > > > new file mode 100644
> > > > index 000000000000..fef6e38d8b98
> > > > --- /dev/null
> > > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > > @@ -0,0 +1,190 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (C) 2016,2017 ARM Ltd.
> > > > + * Copyright 2019 NXP
> > > > + */
> > > > +
> > > > +#include <linux/arm-smccc.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/mailbox_controller.h> #include
> > > > +<linux/mailbox/arm-smc-mailbox.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/platform_device.h>
> > > > +
> > > > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > > > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > > > +
> > > IRQ bit is unused (and unnecessary IMO)
> > >
> > > > +struct arm_smc_chan_data {
> > > > +       u32 function_id;
> > > > +       u32 flags;
> > > > +       int irq;
> > > > +};
> > > > +
> > > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > > +       struct arm_smc_chan_data *chan_data = link->con_priv;
> > > > +       struct arm_smccc_mbox_cmd *cmd = data;
> > > > +       struct arm_smccc_res res;
> > > > +       u32 function_id;
> > > > +
> > > > +       if (chan_data->function_id != UINT_MAX)
> > > > +               function_id = chan_data->function_id;
> > > > +       else
> > > > +               function_id = cmd->a0;
> > > > +
> > > Not sure about chan_data->function_id.  Why restrict from DT?
> > > 'a0' is the function_id register, let the user pass func-id via the 'a0' like
> other
> > > values via 'a[1-7]'
> >
> > Missed to reply this comment.
> >
> > The firmware driver might not have func-id, such as SCMI/SCPI.
> > So add an optional func-id to let smc mailbox driver could
> > use smc SiP func id.
> >
> There is no end to conforming to protocols. Controller drivers should
> be written having no particular client in mind.

If the func-id needs be passed from user, then the chan_id suggested
by Sudeep should also be passed from user, not in mailbox driver.

Jassi, so from your point, arm_smc_send_data just send a0 - a6
to firmware, right?

Sudeep, Andre, Florian,

What's your suggestion? SCMI not support, do you have
plan to add smc transport in SCMI?

Thanks,
Peng.

Jassi Brar June 26, 2019, 4:31 p.m. UTC | #17

On Wed, Jun 26, 2019 at 8:31 AM Peng Fan <[e-mail]> wrote:
>
>
> Hi All,
>
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Tue, Jun 25, 2019 at 2:30 AM Peng Fan <[e-mail]> wrote:
> > >
> > > Hi Jassi
> > >
> > > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > > >
> > > > On Mon, Jun 3, 2019 at 3:28 AM <[e-mail]> wrote:
> > > > >
> > > > > From: Peng Fan <[e-mail]>
> > > > >
> > > > > 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%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C0
> > 1%7
> > > > Cpeng.fa
> > > > >
> > > >
> > n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > > > c6fa92cd
> > > > >
> > > >
> > 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> > > > ZkeRMtL8Bx
> > > > > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> > > > >
> > > > > Cc: Andre Przywara <[e-mail]>
> > > > > Signed-off-by: Peng Fan <[e-mail]>
> > > > > ---
> > > > >
> > > > > V2:
> > > > >  Add interrupts notification support.
> > > > >
> > > > >  drivers/mailbox/Kconfig                 |   7 ++
> > > > >  drivers/mailbox/Makefile                |   2 +
> > > > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > > > ++++++++++++++++++++++++++++++++
> > > > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > > > >  4 files changed, 209 insertions(+)
> > > > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create
> > mode
> > > > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > > > >
> > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > > 595542bfae85..c3bd0f1ddcd8 100644
> > > > > --- a/drivers/mailbox/Kconfig
> > > > > +++ b/drivers/mailbox/Kconfig
> > > > > @@ -15,6 +15,13 @@ config ARM_MHU
> > > > >           The controller has 3 mailbox channels, the last of which can
> > be
> > > > >           used in Secure mode only.
> > > > >
> > > > > +config ARM_SMC_MBOX
> > > > > +       tristate "Generic ARM smc mailbox"
> > > > > +       depends on OF && HAVE_ARM_SMCCC
> > > > > +       help
> > > > > +         Generic mailbox driver which uses ARM smc calls to call into
> > > > > +         firmware for triggering mailboxes.
> > > > > +
> > > > >  config IMX_MBOX
> > > > >         tristate "i.MX Mailbox"
> > > > >         depends on ARCH_MXC || COMPILE_TEST diff --git
> > > > > a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > > > > c22fad6f696b..93918a84c91b 100644
> > > > > --- a/drivers/mailbox/Makefile
> > > > > +++ b/drivers/mailbox/Makefile
> > > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)      +=
> > mailbox-test.o
> > > > >
> > > > >  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
> > > > >
> > > > > +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
> > > > > +
> > > > >  obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> > > > >
> > > > >  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)    +=
> > > > armada-37xx-rwtm-mailbox.o
> > > > > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > > > > b/drivers/mailbox/arm-smc-mailbox.c
> > > > > new file mode 100644
> > > > > index 000000000000..fef6e38d8b98
> > > > > --- /dev/null
> > > > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > > > @@ -0,0 +1,190 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (C) 2016,2017 ARM Ltd.
> > > > > + * Copyright 2019 NXP
> > > > > + */
> > > > > +
> > > > > +#include <linux/arm-smccc.h>
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/mailbox_controller.h> #include
> > > > > +<linux/mailbox/arm-smc-mailbox.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +
> > > > > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > > > > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > > > > +
> > > > IRQ bit is unused (and unnecessary IMO)
> > > >
> > > > > +struct arm_smc_chan_data {
> > > > > +       u32 function_id;
> > > > > +       u32 flags;
> > > > > +       int irq;
> > > > > +};
> > > > > +
> > > > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > > > +       struct arm_smc_chan_data *chan_data = link->con_priv;
> > > > > +       struct arm_smccc_mbox_cmd *cmd = data;
> > > > > +       struct arm_smccc_res res;
> > > > > +       u32 function_id;
> > > > > +
> > > > > +       if (chan_data->function_id != UINT_MAX)
> > > > > +               function_id = chan_data->function_id;
> > > > > +       else
> > > > > +               function_id = cmd->a0;
> > > > > +
> > > > Not sure about chan_data->function_id.  Why restrict from DT?
> > > > 'a0' is the function_id register, let the user pass func-id via the 'a0' like
> > other
> > > > values via 'a[1-7]'
> > >
> > > Missed to reply this comment.
> > >
> > > The firmware driver might not have func-id, such as SCMI/SCPI.
> > > So add an optional func-id to let smc mailbox driver could
> > > use smc SiP func id.
> > >
> > There is no end to conforming to protocols. Controller drivers should
> > be written having no particular client in mind.
>
> If the func-id needs be passed from user, then the chan_id suggested
> by Sudeep should also be passed from user, not in mailbox driver.
>
Isn't it already so?

> Jassi, so from your point, arm_smc_send_data just send a0 - a6
> to firmware, right?
>
Yes.

> Sudeep, Andre, Florian,
>
> What's your suggestion? SCMI not support, do you have
> plan to add smc transport in SCMI?
>
Not replying on their behalf .... but SCMI should eventually support
more than MHU. And I can't see why that matters here?

thanks.

Florian Fainelli June 26, 2019, 4:44 p.m. UTC | #18

On 6/26/19 6:31 AM, Peng Fan wrote:
>>> The firmware driver might not have func-id, such as SCMI/SCPI.
>>> So add an optional func-id to let smc mailbox driver could
>>> use smc SiP func id.
>>>
>> There is no end to conforming to protocols. Controller drivers should
>> be written having no particular client in mind.
> 
> If the func-id needs be passed from user, then the chan_id suggested
> by Sudeep should also be passed from user, not in mailbox driver.
> 
> Jassi, so from your point, arm_smc_send_data just send a0 - a6
> to firmware, right?
> 
> Sudeep, Andre, Florian,
> 
> What's your suggestion? SCMI not support, do you have
> plan to add smc transport in SCMI?

On the platforms that I work with, we have taken the liberty of
implementing SCMI in our monitor firmware because the other MCU we use
for dynamic voltage and frequency scaling did not have enough memory to
support that and we still had the ability to make that firmware be
trusted enough we could give it power management responsibilities. I
would certainly feel more comfortable if the SCMI specification was
amended to indicate that the Agent could be such a software entity,
still residing on the same host CPU as the Platform(s), but if not,
that's fine.

This has lead us to implement a mailbox driver that uses a proprietary
SMC call for the P2A path ("tx" channel) and the return being done via
either that same SMC or through SGI. You can take a look at it in our
downstream tree here actually:

https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c

If we can get rid of our own driver and uses a standard SMC based
mailbox driver that supports our use case that involves interrupts (we
can always change their kind without our firmware/boot loader since FDT
is generated from that component), that would be great.

Sudeep Holla June 26, 2019, 5:02 p.m. UTC | #19

On Wed, Jun 26, 2019 at 01:31:15PM +0000, Peng Fan wrote:
> 
> Hi All,
> 
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > 
> > On Tue, Jun 25, 2019 at 2:30 AM Peng Fan <[e-mail]> wrote:
> > >
> > > Hi Jassi
> > >
> > > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > > >
> > > > On Mon, Jun 3, 2019 at 3:28 AM <[e-mail]> wrote:
> > > > >
> > > > > From: Peng Fan <[e-mail]>
> > > > >
> > > > > 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%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C0
> > 1%7
> > > > Cpeng.fa
> > > > >
> > > >
> > n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > > > c6fa92cd
> > > > >
> > > >
> > 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
> > > > ZkeRMtL8Bx
> > > > > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
> > > > >
> > > > > Cc: Andre Przywara <[e-mail]>
> > > > > Signed-off-by: Peng Fan <[e-mail]>
> > > > > ---
> > > > >
> > > > > V2:
> > > > >  Add interrupts notification support.
> > > > >
> > > > >  drivers/mailbox/Kconfig                 |   7 ++
> > > > >  drivers/mailbox/Makefile                |   2 +
> > > > >  drivers/mailbox/arm-smc-mailbox.c       | 190
> > > > ++++++++++++++++++++++++++++++++
> > > > >  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
> > > > >  4 files changed, 209 insertions(+)
> > > > >  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create
> > mode
> > > > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > > > >
> > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > > 595542bfae85..c3bd0f1ddcd8 100644
> > > > > --- a/drivers/mailbox/Kconfig
> > > > > +++ b/drivers/mailbox/Kconfig
> > > > > @@ -15,6 +15,13 @@ config ARM_MHU
> > > > >           The controller has 3 mailbox channels, the last of which can
> > be
> > > > >           used in Secure mode only.
> > > > >
> > > > > +config ARM_SMC_MBOX
> > > > > +       tristate "Generic ARM smc mailbox"
> > > > > +       depends on OF && HAVE_ARM_SMCCC
> > > > > +       help
> > > > > +         Generic mailbox driver which uses ARM smc calls to call into
> > > > > +         firmware for triggering mailboxes.
> > > > > +
> > > > >  config IMX_MBOX
> > > > >         tristate "i.MX Mailbox"
> > > > >         depends on ARCH_MXC || COMPILE_TEST diff --git
> > > > > a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > > > > c22fad6f696b..93918a84c91b 100644
> > > > > --- a/drivers/mailbox/Makefile
> > > > > +++ b/drivers/mailbox/Makefile
> > > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)      +=
> > mailbox-test.o
> > > > >
> > > > >  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
> > > > >
> > > > > +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
> > > > > +
> > > > >  obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> > > > >
> > > > >  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)    +=
> > > > armada-37xx-rwtm-mailbox.o
> > > > > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > > > > b/drivers/mailbox/arm-smc-mailbox.c
> > > > > new file mode 100644
> > > > > index 000000000000..fef6e38d8b98
> > > > > --- /dev/null
> > > > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > > > @@ -0,0 +1,190 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (C) 2016,2017 ARM Ltd.
> > > > > + * Copyright 2019 NXP
> > > > > + */
> > > > > +
> > > > > +#include <linux/arm-smccc.h>
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/mailbox_controller.h> #include
> > > > > +<linux/mailbox/arm-smc-mailbox.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +
> > > > > +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
> > > > > +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
> > > > > +
> > > > IRQ bit is unused (and unnecessary IMO)
> > > >
> > > > > +struct arm_smc_chan_data {
> > > > > +       u32 function_id;
> > > > > +       u32 flags;
> > > > > +       int irq;
> > > > > +};
> > > > > +
> > > > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > > > +       struct arm_smc_chan_data *chan_data = link->con_priv;
> > > > > +       struct arm_smccc_mbox_cmd *cmd = data;
> > > > > +       struct arm_smccc_res res;
> > > > > +       u32 function_id;
> > > > > +
> > > > > +       if (chan_data->function_id != UINT_MAX)
> > > > > +               function_id = chan_data->function_id;
> > > > > +       else
> > > > > +               function_id = cmd->a0;
> > > > > +
> > > > Not sure about chan_data->function_id.  Why restrict from DT?
> > > > 'a0' is the function_id register, let the user pass func-id via the 'a0' like
> > other
> > > > values via 'a[1-7]'
> > >
> > > Missed to reply this comment.
> > >
> > > The firmware driver might not have func-id, such as SCMI/SCPI.
> > > So add an optional func-id to let smc mailbox driver could
> > > use smc SiP func id.
> > >
> > There is no end to conforming to protocols. Controller drivers should
> > be written having no particular client in mind.
> 
> If the func-id needs be passed from user, then the chan_id suggested
> by Sudeep should also be passed from user, not in mailbox driver.
>

Why ? I understand SMC may have 1-1 mapping from DT to channel id, but
that may not be true for other mailbox controller. The client is provided
with mbox handle in DT and the mbox APIs are used to get the controller
handle. The client just uses the same and when it calls say send_data,
controller understands which channel the handle is mapped to and
client need not have remote idea on channel id. If by user you mean
DT yes but as described as it's indirect.

> Jassi, so from your point, arm_smc_send_data just send a0 - a6
> to firmware, right?
>
> Sudeep, Andre, Florian,
>
> What's your suggestion? SCMI not support, do you have
> plan to add smc transport in SCMI?
>

I *wish* we could abstract all the transport protocol behind the mailbox
APIs and SCMI just deals with message protocol as it is a message protocol
specification.

--
Regards,
Sudeep

Andre Przywara June 26, 2019, 5:05 p.m. UTC | #20

On 25/06/2019 08:20, Peng Fan wrote:
> Hi Jassi,
> 
>> -----Original Message-----
>> From: Jassi Brar [mailto:[e-mail]]
>> Sent: 2019年6月21日 0:50
>> To: Peng Fan <[e-mail]>
>> Cc: Rob Herring <[e-mail]>; Mark Rutland
>> <[e-mail]>; Sudeep Holla <[e-mail]>; Florian
>> Fainelli <[e-mail]>; , Sascha Hauer <[e-mail]>;
>> dl-linux-imx <[e-mail]>; Shawn Guo <[e-mail]>;
>> [e-mail]; Devicetree List <[e-mail]>; Linux
>> Kernel Mailing List <[e-mail]>;
>> [e-mail]; Andre Przywara
>> <[e-mail]>; [e-mail]
>> Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
>>
>> On Mon, Jun 3, 2019 at 3:28 AM <[e-mail]> wrote:
>>>
>>> From: Peng Fan <[e-mail]>
>>>
>>> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
>>> .kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&amp;data=02%7C01%7
>> Cpeng.fa
>>>
>> n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
>> c6fa92cd
>>>
>> 99c5c301635%7C0%7C0%7C636966462272457978&amp;sdata=Hzgeu43m5
>> ZkeRMtL8Bx
>>> gUm3%2B6FBObib1OPHPlSccE%2B0%3D&amp;reserved=0
>>>
>>> Cc: Andre Przywara <[e-mail]>
>>> Signed-off-by: Peng Fan <[e-mail]>
>>> ---
>>>
>>> V2:
>>>  Add interrupts notification support.
>>>
>>>  drivers/mailbox/Kconfig                 |   7 ++
>>>  drivers/mailbox/Makefile                |   2 +
>>>  drivers/mailbox/arm-smc-mailbox.c       | 190
>> ++++++++++++++++++++++++++++++++
>>>  include/linux/mailbox/arm-smc-mailbox.h |  10 ++
>>>  4 files changed, 209 insertions(+)
>>>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c  create mode
>>> 100644 include/linux/mailbox/arm-smc-mailbox.h
>>>
>>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
>>> 595542bfae85..c3bd0f1ddcd8 100644
>>> --- a/drivers/mailbox/Kconfig
>>> +++ b/drivers/mailbox/Kconfig
>>> @@ -15,6 +15,13 @@ config ARM_MHU
>>>           The controller has 3 mailbox channels, the last of which can be
>>>           used in Secure mode only.
>>>
>>> +config ARM_SMC_MBOX
>>> +       tristate "Generic ARM smc mailbox"
>>> +       depends on OF && HAVE_ARM_SMCCC
>>> +       help
>>> +         Generic mailbox driver which uses ARM smc calls to call into
>>> +         firmware for triggering mailboxes.
>>> +
>>>  config IMX_MBOX
>>>         tristate "i.MX Mailbox"
>>>         depends on ARCH_MXC || COMPILE_TEST diff --git
>>> a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
>>> c22fad6f696b..93918a84c91b 100644
>>> --- a/drivers/mailbox/Makefile
>>> +++ b/drivers/mailbox/Makefile
>>> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)      += mailbox-test.o
>>>
>>>  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
>>>
>>> +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
>>> +
>>>  obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
>>>
>>>  obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)    +=
>> armada-37xx-rwtm-mailbox.o
>>> diff --git a/drivers/mailbox/arm-smc-mailbox.c
>>> b/drivers/mailbox/arm-smc-mailbox.c
>>> new file mode 100644
>>> index 000000000000..fef6e38d8b98
>>> --- /dev/null
>>> +++ b/drivers/mailbox/arm-smc-mailbox.c
>>> @@ -0,0 +1,190 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2016,2017 ARM Ltd.
>>> + * Copyright 2019 NXP
>>> + */
>>> +
>>> +#include <linux/arm-smccc.h>
>>> +#include <linux/device.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/mailbox_controller.h> #include
>>> +<linux/mailbox/arm-smc-mailbox.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#define ARM_SMC_MBOX_USE_HVC   BIT(0)
>>> +#define ARM_SMC_MBOX_USB_IRQ   BIT(1)
>>> +
>> IRQ bit is unused (and unnecessary IMO)
> 
> This will be removed in next version.
> 
>>
>>> +struct arm_smc_chan_data {
>>> +       u32 function_id;
>>> +       u32 flags;
>>> +       int irq;
>>> +};
>>> +
>>> +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
>>> +       struct arm_smc_chan_data *chan_data = link->con_priv;
>>> +       struct arm_smccc_mbox_cmd *cmd = data;
>>> +       struct arm_smccc_res res;
>>> +       u32 function_id;
>>> +
>>> +       if (chan_data->function_id != UINT_MAX)
>>> +               function_id = chan_data->function_id;
>>> +       else
>>> +               function_id = cmd->a0;
>>> +
>> Not sure about chan_data->function_id.  Why restrict from DT?
>> 'a0' is the function_id register, let the user pass func-id via the 'a0' like other
>> values via 'a[1-7]'
>>
>>
>>> +       if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
>>> +               arm_smccc_hvc(function_id, cmd->a1, cmd->a2,
>> cmd->a3, cmd->a4,
>>> +                             cmd->a5, cmd->a6, cmd->a7, &res);
>>> +       else
>>> +               arm_smccc_smc(function_id, cmd->a1, cmd->a2,
>> cmd->a3, cmd->a4,
>>> +                             cmd->a5, cmd->a6, cmd->a7, &res);
>>> +
>>> +       if (chan_data->irq)
>>> +               return 0;
>>> +
>> This irq thing seems like oob signalling, that is, a protocol thing.
>> And then it provides lesser info via chan_irq_handler (returns NULL) than
>> res.a0 - which can always be ignored if not needed.
>> So the irq should be implemented in the upper layer if the protocol needs it.
> 
> The interrupts was added here because in v1, Florian suggest
> "
> I would just put a
> provision in the binding to support an optional interrupt such that
> asynchronism gets reasonably easy to plug in when it is available (and
> desirable).
> "
> 
> So I introduced interrupt in V2. In my testcase, after smc call done,
> it means firmware->smc mailbox->firmware done. Interrupt notification
> from firmware->Linux, means firmware has done the operation.
> 
> When using interrupts, we could not know res.a0 as smc sync call.
> 
> Interrupts is not a must in my testcase, Florian, Andre, do you have
> any comments? Should I keep interrupts in V3 or drop it as Jassi comments?

The smc mailbox is by its very design a one-way channel - and it's
synchronous. I think this is all the mailbox driver should be concerned
about. The fact that there is a protocol user that would benefit from a
return channel is a separate issue.
The SCMI binding explicitly mentions *two* mailboxes, one TX, one RX, so
the return channel could be very well implemented by a separate driver.
I am wondering if we get away without a functioning return channel, at
least for a subset of SCMI functionality? Can we use some dummy driver?
Or specify another smc channel with some unhandled/ignored channel ID
for that purpose?

So I would leave the IRQ return channel out for now, unless we
desperately need it.

Cheers,
Andre.

Florian Fainelli June 26, 2019, 5:07 p.m. UTC | #21

On 6/26/19 10:05 AM, André Przywara wrote:
>> So I introduced interrupt in V2. In my testcase, after smc call done,
>> it means firmware->smc mailbox->firmware done. Interrupt notification
>> from firmware->Linux, means firmware has done the operation.
>>
>> When using interrupts, we could not know res.a0 as smc sync call.
>>
>> Interrupts is not a must in my testcase, Florian, Andre, do you have
>> any comments? Should I keep interrupts in V3 or drop it as Jassi comments?
> 
> The smc mailbox is by its very design a one-way channel - and it's
> synchronous. I think this is all the mailbox driver should be concerned
> about. The fact that there is a protocol user that would benefit from a
> return channel is a separate issue.
> The SCMI binding explicitly mentions *two* mailboxes, one TX, one RX, so
> the return channel could be very well implemented by a separate driver.
> I am wondering if we get away without a functioning return channel, at
> least for a subset of SCMI functionality? Can we use some dummy driver?
> Or specify another smc channel with some unhandled/ignored channel ID
> for that purpose?
> 
> So I would leave the IRQ return channel out for now, unless we
> desperately need it.

That's fine, the initial point was specifically about the binding
already defining an optional interrupt property, but that can be removed
too if this is too much confusion or opens up the discussion beyond this
submission.

Sudeep Holla June 26, 2019, 5:09 p.m. UTC | #22

On Wed, Jun 26, 2019 at 09:44:06AM -0700, Florian Fainelli wrote:
> On 6/26/19 6:31 AM, Peng Fan wrote:
> >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> >>> So add an optional func-id to let smc mailbox driver could
> >>> use smc SiP func id.
> >>>
> >> There is no end to conforming to protocols. Controller drivers should
> >> be written having no particular client in mind.
> >
> > If the func-id needs be passed from user, then the chan_id suggested
> > by Sudeep should also be passed from user, not in mailbox driver.
> >
> > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > to firmware, right?
> >
> > Sudeep, Andre, Florian,
> >
> > What's your suggestion? SCMI not support, do you have
> > plan to add smc transport in SCMI?
>
> On the platforms that I work with, we have taken the liberty of
> implementing SCMI in our monitor firmware because the other MCU we use
> for dynamic voltage and frequency scaling did not have enough memory to
> support that and we still had the ability to make that firmware be
> trusted enough we could give it power management responsibilities. I
> would certainly feel more comfortable if the SCMI specification was
> amended to indicate that the Agent could be such a software entity,
> still residing on the same host CPU as the Platform(s), but if not,
> that's fine.
>

That's completely legal and there's nothing in the specification that
prohibits. I understand it's not explicitly not mentioned and I have
been trying to get such things clarified. But since it's main focus
is on the message protocol, the clarity on transport mechanism is very
thin and there's hesitation to add more details under the impression
that it may restrict the usage.

But as I mentioned, I understand what you need there :)

> This has lead us to implement a mailbox driver that uses a proprietary
> SMC call for the P2A path ("tx" channel) and the return being done via
> either that same SMC or through SGI. You can take a look at it in our
> downstream tree here actually:
>
> https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
>

Just curious, I see it's fast call and why do you still depend on
interrupt to indicate completion of the message. Will the return from
SMC not suffice ? Sorry if I am missing something obvious.

--
Regards,
Sudeep

Jassi Brar June 26, 2019, 6:27 p.m. UTC | #23

On Wed, Jun 26, 2019 at 11:44 AM Florian Fainelli <[e-mail]> wrote:
>
> On 6/26/19 6:31 AM, Peng Fan wrote:
> >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> >>> So add an optional func-id to let smc mailbox driver could
> >>> use smc SiP func id.
> >>>
> >> There is no end to conforming to protocols. Controller drivers should
> >> be written having no particular client in mind.
> >
> > If the func-id needs be passed from user, then the chan_id suggested
> > by Sudeep should also be passed from user, not in mailbox driver.
> >
> > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > to firmware, right?
> >
> > Sudeep, Andre, Florian,
> >
> > What's your suggestion? SCMI not support, do you have
> > plan to add smc transport in SCMI?
>
> On the platforms that I work with, we have taken the liberty of
> implementing SCMI in our monitor firmware because the other MCU we use
> for dynamic voltage and frequency scaling did not have enough memory to
> support that and we still had the ability to make that firmware be
> trusted enough we could give it power management responsibilities. I
> would certainly feel more comfortable if the SCMI specification was
> amended to indicate that the Agent could be such a software entity,
> still residing on the same host CPU as the Platform(s), but if not,
> that's fine.
>
> This has lead us to implement a mailbox driver that uses a proprietary
> SMC call for the P2A path ("tx" channel) and the return being done via
> either that same SMC or through SGI. You can take a look at it in our
> downstream tree here actually:
>
> https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
>
> If we can get rid of our own driver and uses a standard SMC based
> mailbox driver that supports our use case that involves interrupts (we
> can always change their kind without our firmware/boot loader since FDT
> is generated from that component), that would be great.
>
static irqreturn_t brcm_isr(void)
{
         mbox_chan_received_data(&chans[0], NULL);
         return IRQ_HANDLED;
}

Sorry, I fail to understand why the irq can't be moved inside the
client driver itself? There can't be more cost to it and there
definitely is no functionality lost.

Sudeep Holla June 27, 2019, 9:09 a.m. UTC | #24

On Wed, Jun 26, 2019 at 01:27:41PM -0500, Jassi Brar wrote:
> On Wed, Jun 26, 2019 at 11:44 AM Florian Fainelli <[e-mail]> wrote:
> >
> > On 6/26/19 6:31 AM, Peng Fan wrote:
> > >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> > >>> So add an optional func-id to let smc mailbox driver could
> > >>> use smc SiP func id.
> > >>>
> > >> There is no end to conforming to protocols. Controller drivers should
> > >> be written having no particular client in mind.
> > >
> > > If the func-id needs be passed from user, then the chan_id suggested
> > > by Sudeep should also be passed from user, not in mailbox driver.
> > >
> > > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > > to firmware, right?
> > >
> > > Sudeep, Andre, Florian,
> > >
> > > What's your suggestion? SCMI not support, do you have
> > > plan to add smc transport in SCMI?
> >
> > On the platforms that I work with, we have taken the liberty of
> > implementing SCMI in our monitor firmware because the other MCU we use
> > for dynamic voltage and frequency scaling did not have enough memory to
> > support that and we still had the ability to make that firmware be
> > trusted enough we could give it power management responsibilities. I
> > would certainly feel more comfortable if the SCMI specification was
> > amended to indicate that the Agent could be such a software entity,
> > still residing on the same host CPU as the Platform(s), but if not,
> > that's fine.
> >
> > This has lead us to implement a mailbox driver that uses a proprietary
> > SMC call for the P2A path ("tx" channel) and the return being done via
> > either that same SMC or through SGI. You can take a look at it in our
> > downstream tree here actually:
> >
> > https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
> >
> > If we can get rid of our own driver and uses a standard SMC based
> > mailbox driver that supports our use case that involves interrupts (we
> > can always change their kind without our firmware/boot loader since FDT
> > is generated from that component), that would be great.
> >
> static irqreturn_t brcm_isr(void)
> {
>          mbox_chan_received_data(&chans[0], NULL);
>          return IRQ_HANDLED;
> }
> 
> Sorry, I fail to understand why the irq can't be moved inside the
> client driver itself? There can't be more cost to it and there
> definitely is no functionality lost.

What if there are multiple clients ?
And I assume you are referring to case like this where IRQ is not tied
to the mailbox IP.

--
Regards,
Sudeep

Jassi Brar June 27, 2019, 3:32 p.m. UTC | #25

On Thu, Jun 27, 2019 at 4:09 AM Sudeep Holla <[e-mail]> wrote:
>
> On Wed, Jun 26, 2019 at 01:27:41PM -0500, Jassi Brar wrote:
> > On Wed, Jun 26, 2019 at 11:44 AM Florian Fainelli <[e-mail]> wrote:
> > >
> > > On 6/26/19 6:31 AM, Peng Fan wrote:
> > > >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> > > >>> So add an optional func-id to let smc mailbox driver could
> > > >>> use smc SiP func id.
> > > >>>
> > > >> There is no end to conforming to protocols. Controller drivers should
> > > >> be written having no particular client in mind.
> > > >
> > > > If the func-id needs be passed from user, then the chan_id suggested
> > > > by Sudeep should also be passed from user, not in mailbox driver.
> > > >
> > > > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > > > to firmware, right?
> > > >
> > > > Sudeep, Andre, Florian,
> > > >
> > > > What's your suggestion? SCMI not support, do you have
> > > > plan to add smc transport in SCMI?
> > >
> > > On the platforms that I work with, we have taken the liberty of
> > > implementing SCMI in our monitor firmware because the other MCU we use
> > > for dynamic voltage and frequency scaling did not have enough memory to
> > > support that and we still had the ability to make that firmware be
> > > trusted enough we could give it power management responsibilities. I
> > > would certainly feel more comfortable if the SCMI specification was
> > > amended to indicate that the Agent could be such a software entity,
> > > still residing on the same host CPU as the Platform(s), but if not,
> > > that's fine.
> > >
> > > This has lead us to implement a mailbox driver that uses a proprietary
> > > SMC call for the P2A path ("tx" channel) and the return being done via
> > > either that same SMC or through SGI. You can take a look at it in our
> > > downstream tree here actually:
> > >
> > > https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
> > >
> > > If we can get rid of our own driver and uses a standard SMC based
> > > mailbox driver that supports our use case that involves interrupts (we
> > > can always change their kind without our firmware/boot loader since FDT
> > > is generated from that component), that would be great.
> > >
> > static irqreturn_t brcm_isr(void)
> > {
> >          mbox_chan_received_data(&chans[0], NULL);
> >          return IRQ_HANDLED;
> > }
> >
> > Sorry, I fail to understand why the irq can't be moved inside the
> > client driver itself? There can't be more cost to it and there
> > definitely is no functionality lost.
>
> What if there are multiple clients ?
>
There is a flag IRQF_SHARED for such situations.
(good to see you considering multiple clients per channel as a legit scenario)

> And I assume you are referring to case like this where IRQ is not tied
> to the mailbox IP.
>
Yes, and that is the reason the irq should not be managed by the mailbox driver.

Sudeep Holla June 27, 2019, 5:07 p.m. UTC | #26

On Thu, Jun 27, 2019 at 10:32:27AM -0500, Jassi Brar wrote:
> On Thu, Jun 27, 2019 at 4:09 AM Sudeep Holla <[e-mail]> wrote:
> >
> > On Wed, Jun 26, 2019 at 01:27:41PM -0500, Jassi Brar wrote:
> > > On Wed, Jun 26, 2019 at 11:44 AM Florian Fainelli <[e-mail]> wrote:
> > > >
> > > > On 6/26/19 6:31 AM, Peng Fan wrote:
> > > > >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> > > > >>> So add an optional func-id to let smc mailbox driver could
> > > > >>> use smc SiP func id.
> > > > >>>
> > > > >> There is no end to conforming to protocols. Controller drivers should
> > > > >> be written having no particular client in mind.
> > > > >
> > > > > If the func-id needs be passed from user, then the chan_id suggested
> > > > > by Sudeep should also be passed from user, not in mailbox driver.
> > > > >
> > > > > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > > > > to firmware, right?
> > > > >
> > > > > Sudeep, Andre, Florian,
> > > > >
> > > > > What's your suggestion? SCMI not support, do you have
> > > > > plan to add smc transport in SCMI?
> > > >
> > > > On the platforms that I work with, we have taken the liberty of
> > > > implementing SCMI in our monitor firmware because the other MCU we use
> > > > for dynamic voltage and frequency scaling did not have enough memory to
> > > > support that and we still had the ability to make that firmware be
> > > > trusted enough we could give it power management responsibilities. I
> > > > would certainly feel more comfortable if the SCMI specification was
> > > > amended to indicate that the Agent could be such a software entity,
> > > > still residing on the same host CPU as the Platform(s), but if not,
> > > > that's fine.
> > > >
> > > > This has lead us to implement a mailbox driver that uses a proprietary
> > > > SMC call for the P2A path ("tx" channel) and the return being done via
> > > > either that same SMC or through SGI. You can take a look at it in our
> > > > downstream tree here actually:
> > > >
> > > > https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
> > > >
> > > > If we can get rid of our own driver and uses a standard SMC based
> > > > mailbox driver that supports our use case that involves interrupts (we
> > > > can always change their kind without our firmware/boot loader since FDT
> > > > is generated from that component), that would be great.
> > > >
> > > static irqreturn_t brcm_isr(void)
> > > {
> > >          mbox_chan_received_data(&chans[0], NULL);
> > >          return IRQ_HANDLED;
> > > }
> > >
> > > Sorry, I fail to understand why the irq can't be moved inside the
> > > client driver itself? There can't be more cost to it and there
> > > definitely is no functionality lost.
> >
> > What if there are multiple clients ?
> >
> There is a flag IRQF_SHARED for such situations.

Indeed, we can use it.

> (good to see you considering multiple clients per channel as a legit scenario)
>

Not single channel, but single IRQ shared by multiple channels.
We can have multiple SMC based mailbox but one shared IRQ.

> > And I assume you are referring to case like this where IRQ is not tied
> > to the mailbox IP.
> >
> Yes, and that is the reason the irq should not be manageid by the mailbox driver.

Thanks for confirmation.

--
Regards,
Sudeep

Florian Fainelli June 27, 2019, 6:10 p.m. UTC | #27

On 6/26/19 10:09 AM, Sudeep Holla wrote:
> On Wed, Jun 26, 2019 at 09:44:06AM -0700, Florian Fainelli wrote:
>> On 6/26/19 6:31 AM, Peng Fan wrote:
>>>>> The firmware driver might not have func-id, such as SCMI/SCPI.
>>>>> So add an optional func-id to let smc mailbox driver could
>>>>> use smc SiP func id.
>>>>>
>>>> There is no end to conforming to protocols. Controller drivers should
>>>> be written having no particular client in mind.
>>>
>>> If the func-id needs be passed from user, then the chan_id suggested
>>> by Sudeep should also be passed from user, not in mailbox driver.
>>>
>>> Jassi, so from your point, arm_smc_send_data just send a0 - a6
>>> to firmware, right?
>>>
>>> Sudeep, Andre, Florian,
>>>
>>> What's your suggestion? SCMI not support, do you have
>>> plan to add smc transport in SCMI?
>>
>> On the platforms that I work with, we have taken the liberty of
>> implementing SCMI in our monitor firmware because the other MCU we use
>> for dynamic voltage and frequency scaling did not have enough memory to
>> support that and we still had the ability to make that firmware be
>> trusted enough we could give it power management responsibilities. I
>> would certainly feel more comfortable if the SCMI specification was
>> amended to indicate that the Agent could be such a software entity,
>> still residing on the same host CPU as the Platform(s), but if not,
>> that's fine.
>>
> 
> That's completely legal and there's nothing in the specification that
> prohibits. I understand it's not explicitly not mentioned and I have
> been trying to get such things clarified. But since it's main focus
> is on the message protocol, the clarity on transport mechanism is very
> thin and there's hesitation to add more details under the impression
> that it may restrict the usage.
> 
> But as I mentioned, I understand what you need there :)
> 
>> This has lead us to implement a mailbox driver that uses a proprietary
>> SMC call for the P2A path ("tx" channel) and the return being done via
>> either that same SMC or through SGI. You can take a look at it in our
>> downstream tree here actually:
>>
>> https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
>>
> 
> Just curious, I see it's fast call and why do you still depend on
> interrupt to indicate completion of the message. Will the return from
> SMC not suffice ? Sorry if I am missing something obvious.

It is currently used for synchronous delayed responses where the SMC
call returns early, but the operation is carried out asynchronously by
e.g: the MCU that does voltage scaling a few milliseconds later. We'd
rather not block the caller for too long and that's where it stems from.

Patch

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 595542bfae85..c3bd0f1ddcd8 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -15,6 +15,13 @@  config ARM_MHU
 	  The controller has 3 mailbox channels, the last of which can be
 	  used in Secure mode only.
 
+config ARM_SMC_MBOX
+	tristate "Generic ARM smc mailbox"
+	depends on OF && HAVE_ARM_SMCCC
+	help
+	  Generic mailbox driver which uses ARM smc calls to call into
+	  firmware for triggering mailboxes.
+
 config IMX_MBOX
 	tristate "i.MX Mailbox"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index c22fad6f696b..93918a84c91b 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -7,6 +7,8 @@  obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
 
 obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
 
+obj-$(CONFIG_ARM_SMC_MBOX)	+= arm-smc-mailbox.o
+
 obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
 
 obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)	+= armada-37xx-rwtm-mailbox.o
diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
new file mode 100644
index 000000000000..fef6e38d8b98
--- /dev/null
+++ b/drivers/mailbox/arm-smc-mailbox.c
@@ -0,0 +1,190 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2016,2017 ARM Ltd.
+ * Copyright 2019 NXP
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/arm-smc-mailbox.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define ARM_SMC_MBOX_USE_HVC	BIT(0)
+#define ARM_SMC_MBOX_USB_IRQ	BIT(1)
+
+struct arm_smc_chan_data {
+	u32 function_id;
+	u32 flags;
+	int irq;
+};
+
+static int arm_smc_send_data(struct mbox_chan *link, void *data)
+{
+	struct arm_smc_chan_data *chan_data = link->con_priv;
+	struct arm_smccc_mbox_cmd *cmd = data;
+	struct arm_smccc_res res;
+	u32 function_id;
+
+	if (chan_data->function_id != UINT_MAX)
+		function_id = chan_data->function_id;
+	else
+		function_id = cmd->a0;
+
+	if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
+		arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
+			      cmd->a5, cmd->a6, cmd->a7, &res);
+	else
+		arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
+			      cmd->a5, cmd->a6, cmd->a7, &res);
+
+	if (chan_data->irq)
+		return 0;
+
+	mbox_chan_received_data(link, (void *)res.a0);
+
+	return 0;
+}
+
+static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
+	.send_data	= arm_smc_send_data,
+};
+
+static irqreturn_t chan_irq_handler(int irq, void *data)
+{
+	struct mbox_chan *chan = data;
+
+	mbox_chan_received_data(chan, NULL);
+
+	return IRQ_HANDLED;
+}
+
+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);
+			return -EINVAL;
+		}
+	}
+
+	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");
+		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;
+		}
+	}
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	mbox->num_chans = val;
+	mbox->chans = devm_kcalloc(dev, mbox->num_chans, sizeof(*mbox->chans),
+				   GFP_KERNEL);
+	if (!mbox->chans)
+		return -ENOMEM;
+
+	chan_data = devm_kcalloc(dev, mbox->num_chans, sizeof(*chan_data),
+				 GFP_KERNEL);
+	if (!chan_data)
+		return -ENOMEM;
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		u32 function_id;
+
+		ret = of_property_read_u32_index(dev->of_node,
+						 "arm,func-ids", i,
+						 &function_id);
+		if (ret)
+			chan_data[i].function_id = UINT_MAX;
+
+		else
+			chan_data[i].function_id = function_id;
+
+		if (use_hvc)
+			chan_data[i].flags |= ARM_SMC_MBOX_USE_HVC;
+		mbox->chans[i].con_priv = &chan_data[i];
+
+		if (irq_count) {
+			chan_data[i].irq = platform_get_irq(pdev, i);
+			if (chan_data[i].irq < 0)
+				return chan_data[i].irq;
+
+			ret = devm_request_irq(&pdev->dev, chan_data[i].irq,
+					       chan_irq_handler, 0, pdev->name,
+					       &mbox->chans[i]);
+			if (ret)
+				return ret;
+		}
+	}
+
+	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);
+	dev_info(dev, "ARM SMC mailbox enabled with %d chan%s.\n",
+		 mbox->num_chans, mbox->num_chans == 1 ? "" : "s");
+
+	return ret;
+}
+
+static int arm_smc_mbox_remove(struct platform_device *pdev)
+{
+	struct mbox_controller *mbox = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(mbox);
+	return 0;
+}
+
+static const struct of_device_id arm_smc_mbox_of_match[] = {
+	{ .compatible = "arm,smc-mbox", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, arm_smc_mbox_of_match);
+
+static struct platform_driver arm_smc_mbox_driver = {
+	.driver = {
+		.name = "arm-smc-mbox",
+		.of_match_table = arm_smc_mbox_of_match,
+	},
+	.probe		= arm_smc_mbox_probe,
+	.remove		= arm_smc_mbox_remove,
+};
+module_platform_driver(arm_smc_mbox_driver);
+
+MODULE_AUTHOR("Andre Przywara <[e-mail]>");
+MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mailbox/arm-smc-mailbox.h b/include/linux/mailbox/arm-smc-mailbox.h
new file mode 100644
index 000000000000..ca366fe491c3
--- /dev/null
+++ b/include/linux/mailbox/arm-smc-mailbox.h
@@ -0,0 +1,10 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#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;
+};
+
+#endif /* _LINUX_ARM_SMC_MAILBOX_H_ */