Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On Thu, Dec 18, 2014 at 1:17 AM, Dinh Nguyen wrote:
> +MODULE_DEVICE_TABLE(of, altera_mbox_match);
> +
> +static struct platform_driver altera_mbox_driver = {
> +.probe = altera_mbox_probe,
> +.remove = altera_mbox_remove,
> +.driver = {
> +.name = DRIVER_NAME,
> +.owner = THIS_MODULE,
> +.of_match_table = altera_mbox_match,
>>>
>>> of_match_ptr(altera_mbox_match).
>> Okay.
>
> This driver is DT-only, so of_match_ptr() is not needed.
>
Yes, this driver is DT-only. Okay, we don't need of_match_ptr.
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On 12/16/14, 12:33 AM, Ley Foon Tan wrote:
> On Tue, Dec 16, 2014 at 4:54 AM, Suman Anna wrote:
>> Hi Ley Foon,
>>
>> On 12/12/2014 08:38 AM, Dinh Nguyen wrote:
>>>
>>>
>>> On 12/12/14, 4:04 AM, Ley Foon Tan wrote:
The Altera mailbox allows for interprocessor communication. It supports
only one channel and work as either sender or receiver.
>>
>> I have a few more comments in addition to those that Dinh provided.
>>
Signed-off-by: Ley Foon Tan
---
.../devicetree/bindings/mailbox/altera-mailbox.txt | 49 +++
drivers/mailbox/Kconfig| 6 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-altera.c | 404
+
4 files changed, 461 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-altera.c
diff --git a/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
new file mode 100644
index 000..c261979
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
@@ -0,0 +1,49 @@
+Altera Mailbox Driver
+=
+
+Required properties:
+- compatible : "altr,mailbox-1.0".
>>
+static const struct of_device_id altera_mbox_match[] = {
+{ .compatible = "altr,mailbox-1.0" },
+{ /* Sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, altera_mbox_match);
+
+static struct platform_driver altera_mbox_driver = {
+.probe = altera_mbox_probe,
+.remove = altera_mbox_remove,
+.driver = {
+.name = DRIVER_NAME,
+.owner = THIS_MODULE,
+.of_match_table = altera_mbox_match,
>>
>> of_match_ptr(altera_mbox_match).
> Okay.
This driver is DT-only, so of_match_ptr() is not needed.
Dinh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On 12/16/14, 12:33 AM, Ley Foon Tan wrote:
On Tue, Dec 16, 2014 at 4:54 AM, Suman Anna s-a...@ti.com wrote:
Hi Ley Foon,
On 12/12/2014 08:38 AM, Dinh Nguyen wrote:
On 12/12/14, 4:04 AM, Ley Foon Tan wrote:
The Altera mailbox allows for interprocessor communication. It supports
only one channel and work as either sender or receiver.
I have a few more comments in addition to those that Dinh provided.
Signed-off-by: Ley Foon Tan lf...@altera.com
---
.../devicetree/bindings/mailbox/altera-mailbox.txt | 49 +++
drivers/mailbox/Kconfig| 6 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-altera.c | 404
+
4 files changed, 461 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-altera.c
diff --git a/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
new file mode 100644
index 000..c261979
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
@@ -0,0 +1,49 @@
+Altera Mailbox Driver
+=
+
+Required properties:
+- compatible : altr,mailbox-1.0.
snip
+static const struct of_device_id altera_mbox_match[] = {
+{ .compatible = altr,mailbox-1.0 },
+{ /* Sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, altera_mbox_match);
+
+static struct platform_driver altera_mbox_driver = {
+.probe = altera_mbox_probe,
+.remove = altera_mbox_remove,
+.driver = {
+.name = DRIVER_NAME,
+.owner = THIS_MODULE,
+.of_match_table = altera_mbox_match,
of_match_ptr(altera_mbox_match).
Okay.
This driver is DT-only, so of_match_ptr() is not needed.
Dinh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On Thu, Dec 18, 2014 at 1:17 AM, Dinh Nguyen dinh.li...@gmail.com wrote:
+MODULE_DEVICE_TABLE(of, altera_mbox_match);
+
+static struct platform_driver altera_mbox_driver = {
+.probe = altera_mbox_probe,
+.remove = altera_mbox_remove,
+.driver = {
+.name = DRIVER_NAME,
+.owner = THIS_MODULE,
+.of_match_table = altera_mbox_match,
of_match_ptr(altera_mbox_match).
Okay.
This driver is DT-only, so of_match_ptr() is not needed.
Yes, this driver is DT-only. Okay, we don't need of_match_ptr.
Ley Foon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On Wed, Dec 17, 2014 at 3:53 AM, Suman Anna wrote: >>> I think this logic in general will conflict between interrupt and poll >>> mode. send_data is supposed to return -EBUSY only in polling mode and >>> not in interrupt mode. >> What is the recommended error code for this case? BTW, omap-mailbox.c >> also return -EBUSY if mailbox is full. > > Looks like the mbox core doesn't care about the error, it's just > checking for non-success case. I made that comment based on the > documentation in last_tx_done. OMAP mailbox is always interrupt driven, > and it is very rare that we will ever hit the mbox full because of the > Tx-ready interrupt driven Tx ticker and also the h/w fifo. > > I see that your mailbox is very similar to OMAP mailbox though without > the h/w fifo, but you do have support for both polling and interrupt > modes in your code. OMAP mailbox can do that as well, but it's > inefficient to use polling so the driver is strictly interrupt driven. Yes, you are right. So, I think -EBUSY still can use it in interrupt case. > + > +mbox->mbox_base = devm_ioremap_resource(>dev, regs); > +if (IS_ERR(mbox->mbox_base)) > +return PTR_ERR(mbox->mbox_base); > + > +/* Check is it a sender or receiver? */ > +mbox->is_sender = altera_mbox_is_sender(mbox); > + > +mbox->irq = platform_get_irq(pdev, 0); > +if (mbox->irq >= 0) > +mbox->intr_mode = true; >>> >>> This seems to be a poor way of setting up the mode. Is it the same IP >>> block but different integration on different SoCs? Or on the same SoC, >>> and you can use it either by interrupt driven or by polling. >> Yes, it can use interrupt or polling mode depends on hardware configurations. >> It is a soft IP and can be configured with different hardware configurations. > > OK, the platform_get_irq suggests that this decision is made by having > or not having the interrupts property in the DT node, is that the only > way of differentiating this mode? Yes, we don't have other DTS property to identify the interrupt mode. Regards Ley Foon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
Hi Ley Foon,
On 12/16/2014 12:33 AM, Ley Foon Tan wrote:
> On Tue, Dec 16, 2014 at 4:54 AM, Suman Anna wrote:
>> Hi Ley Foon,
>>
>> On 12/12/2014 08:38 AM, Dinh Nguyen wrote:
>>>
>>>
>>> On 12/12/14, 4:04 AM, Ley Foon Tan wrote:
The Altera mailbox allows for interprocessor communication. It supports
only one channel and work as either sender or receiver.
>>
>> I have a few more comments in addition to those that Dinh provided.
>>
Signed-off-by: Ley Foon Tan
---
.../devicetree/bindings/mailbox/altera-mailbox.txt | 49 +++
drivers/mailbox/Kconfig| 6 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-altera.c | 404
+
4 files changed, 461 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-altera.c
diff --git a/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
new file mode 100644
index 000..c261979
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
@@ -0,0 +1,49 @@
+Altera Mailbox Driver
+=
+
+Required properties:
+- compatible : "altr,mailbox-1.0".
>>
>> I am not sure on this convention, sounds like IP version number. Please
>> check this with a DT maintainer.
> Our other soft IPs compatible strings all have 1.0 version number. Eg:
> altr,juart-1.0.
> So, we want to keep it as same format.
>
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c04fed9..84325f2 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -45,4 +45,10 @@ config PCC
states). Select this driver if your platform implements the
PCC clients mentioned above.
+config ALTERA_MBOX
+tristate "Altera Mailbox"
>>
>> You haven't used any depends on here, this driver is not applicable on
>> all platforms, right?
> It is a soft IP, so it is not limited to our platform only.
> Other soft IP drivers also don't have 'depend on', so I think should be fine.
Yeah, ok.
>
>
+
+static inline struct altera_mbox *to_altera_mbox(struct mbox_chan *chan)
+{
+if (!chan)
+return NULL;
+
+return container_of(chan, struct altera_mbox, chan);
+}
+
+static inline int altera_mbox_full(struct altera_mbox *mbox)
+{
+u32 status;
+
+status = __raw_readl(mbox->mbox_base + MAILBOX_STS_REG);
>>
>> You may want to replace all the __raw_readl/__raw_writel with regular
>> readl/writel or its equivalent relaxed versions depending on your needs.
> Okay.
>
>
+static int altera_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+struct altera_mbox *mbox = to_altera_mbox(chan);
+u32 *udata = (u32 *)data;
+
+if (!mbox || !data)
+return -EINVAL;
+if (!mbox->is_sender) {
+dev_warn(mbox->dev,
+"failed to send. This is receiver mailbox.\n");
+return -EINVAL;
+}
+
+if (altera_mbox_full(mbox))
+return -EBUSY;
>>
>> I think this logic in general will conflict between interrupt and poll
>> mode. send_data is supposed to return -EBUSY only in polling mode and
>> not in interrupt mode.
> What is the recommended error code for this case? BTW, omap-mailbox.c
> also return -EBUSY if mailbox is full.
Looks like the mbox core doesn't care about the error, it's just
checking for non-success case. I made that comment based on the
documentation in last_tx_done. OMAP mailbox is always interrupt driven,
and it is very rare that we will ever hit the mbox full because of the
Tx-ready interrupt driven Tx ticker and also the h/w fifo.
I see that your mailbox is very similar to OMAP mailbox though without
the h/w fifo, but you do have support for both polling and interrupt
modes in your code. OMAP mailbox can do that as well, but it's
inefficient to use polling so the driver is strictly interrupt driven.
>
+
+/* Enable interrupt before send */
+altera_mbox_tx_intmask(mbox, true);
>>
>> Is this true and required in Polling mode too?
> Add checking for interrupt mode here.
>
>
>>
>> Do you even need the chans variable, don't see it getting used
>> anywhere. You are directly using the mbox->chan during registration..
> Will update this.
>>
>>>
+struct resource *regs;
+int ret;
+
+mbox = devm_kzalloc(>dev, sizeof(struct altera_mbox),
>>
>> Use sizeof(*mbox)
> Okay.
>
>>
+GFP_KERNEL);
>>
>> You have a few "Alignment should match open parenthesis" checks
>> generated with the --strict option on checkpatch. I
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
Hi Ley Foon,
On 12/16/2014 12:33 AM, Ley Foon Tan wrote:
On Tue, Dec 16, 2014 at 4:54 AM, Suman Anna s-a...@ti.com wrote:
Hi Ley Foon,
On 12/12/2014 08:38 AM, Dinh Nguyen wrote:
On 12/12/14, 4:04 AM, Ley Foon Tan wrote:
The Altera mailbox allows for interprocessor communication. It supports
only one channel and work as either sender or receiver.
I have a few more comments in addition to those that Dinh provided.
Signed-off-by: Ley Foon Tan lf...@altera.com
---
.../devicetree/bindings/mailbox/altera-mailbox.txt | 49 +++
drivers/mailbox/Kconfig| 6 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-altera.c | 404
+
4 files changed, 461 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-altera.c
diff --git a/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
new file mode 100644
index 000..c261979
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
@@ -0,0 +1,49 @@
+Altera Mailbox Driver
+=
+
+Required properties:
+- compatible : altr,mailbox-1.0.
I am not sure on this convention, sounds like IP version number. Please
check this with a DT maintainer.
Our other soft IPs compatible strings all have 1.0 version number. Eg:
altr,juart-1.0.
So, we want to keep it as same format.
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c04fed9..84325f2 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -45,4 +45,10 @@ config PCC
states). Select this driver if your platform implements the
PCC clients mentioned above.
+config ALTERA_MBOX
+tristate Altera Mailbox
You haven't used any depends on here, this driver is not applicable on
all platforms, right?
It is a soft IP, so it is not limited to our platform only.
Other soft IP drivers also don't have 'depend on', so I think should be fine.
Yeah, ok.
+
+static inline struct altera_mbox *to_altera_mbox(struct mbox_chan *chan)
+{
+if (!chan)
+return NULL;
+
+return container_of(chan, struct altera_mbox, chan);
+}
+
+static inline int altera_mbox_full(struct altera_mbox *mbox)
+{
+u32 status;
+
+status = __raw_readl(mbox-mbox_base + MAILBOX_STS_REG);
You may want to replace all the __raw_readl/__raw_writel with regular
readl/writel or its equivalent relaxed versions depending on your needs.
Okay.
+static int altera_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+struct altera_mbox *mbox = to_altera_mbox(chan);
+u32 *udata = (u32 *)data;
+
+if (!mbox || !data)
+return -EINVAL;
+if (!mbox-is_sender) {
+dev_warn(mbox-dev,
+failed to send. This is receiver mailbox.\n);
+return -EINVAL;
+}
+
+if (altera_mbox_full(mbox))
+return -EBUSY;
I think this logic in general will conflict between interrupt and poll
mode. send_data is supposed to return -EBUSY only in polling mode and
not in interrupt mode.
What is the recommended error code for this case? BTW, omap-mailbox.c
also return -EBUSY if mailbox is full.
Looks like the mbox core doesn't care about the error, it's just
checking for non-success case. I made that comment based on the
documentation in last_tx_done. OMAP mailbox is always interrupt driven,
and it is very rare that we will ever hit the mbox full because of the
Tx-ready interrupt driven Tx ticker and also the h/w fifo.
I see that your mailbox is very similar to OMAP mailbox though without
the h/w fifo, but you do have support for both polling and interrupt
modes in your code. OMAP mailbox can do that as well, but it's
inefficient to use polling so the driver is strictly interrupt driven.
+
+/* Enable interrupt before send */
+altera_mbox_tx_intmask(mbox, true);
Is this true and required in Polling mode too?
Add checking for interrupt mode here.
Do you even need the chans variable, don't see it getting used
anywhere. You are directly using the mbox-chan during registration..
Will update this.
+struct resource *regs;
+int ret;
+
+mbox = devm_kzalloc(pdev-dev, sizeof(struct altera_mbox),
Use sizeof(*mbox)
Okay.
+GFP_KERNEL);
You have a few Alignment should match open parenthesis checks
generated with the --strict option on checkpatch. I am not sure if Jassi
is requiring them, but I would recommend that you fix all of them.
Okay, will check these.
+if (!mbox)
+return -ENOMEM;
+
+regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+if (!regs)
+return -ENXIO;
You can remove the check for regs, devm_ioremap_resource is capable
of handling the error.
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On Wed, Dec 17, 2014 at 3:53 AM, Suman Anna s-a...@ti.com wrote: I think this logic in general will conflict between interrupt and poll mode. send_data is supposed to return -EBUSY only in polling mode and not in interrupt mode. What is the recommended error code for this case? BTW, omap-mailbox.c also return -EBUSY if mailbox is full. Looks like the mbox core doesn't care about the error, it's just checking for non-success case. I made that comment based on the documentation in last_tx_done. OMAP mailbox is always interrupt driven, and it is very rare that we will ever hit the mbox full because of the Tx-ready interrupt driven Tx ticker and also the h/w fifo. I see that your mailbox is very similar to OMAP mailbox though without the h/w fifo, but you do have support for both polling and interrupt modes in your code. OMAP mailbox can do that as well, but it's inefficient to use polling so the driver is strictly interrupt driven. Yes, you are right. So, I think -EBUSY still can use it in interrupt case. + +mbox-mbox_base = devm_ioremap_resource(pdev-dev, regs); +if (IS_ERR(mbox-mbox_base)) +return PTR_ERR(mbox-mbox_base); + +/* Check is it a sender or receiver? */ +mbox-is_sender = altera_mbox_is_sender(mbox); + +mbox-irq = platform_get_irq(pdev, 0); +if (mbox-irq = 0) +mbox-intr_mode = true; This seems to be a poor way of setting up the mode. Is it the same IP block but different integration on different SoCs? Or on the same SoC, and you can use it either by interrupt driven or by polling. Yes, it can use interrupt or polling mode depends on hardware configurations. It is a soft IP and can be configured with different hardware configurations. OK, the platform_get_irq suggests that this decision is made by having or not having the interrupts property in the DT node, is that the only way of differentiating this mode? Yes, we don't have other DTS property to identify the interrupt mode. Regards Ley Foon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On Tue, Dec 16, 2014 at 4:54 AM, Suman Anna wrote:
> Hi Ley Foon,
>
> On 12/12/2014 08:38 AM, Dinh Nguyen wrote:
>>
>>
>> On 12/12/14, 4:04 AM, Ley Foon Tan wrote:
>>> The Altera mailbox allows for interprocessor communication. It supports
>>> only one channel and work as either sender or receiver.
>
> I have a few more comments in addition to those that Dinh provided.
>
>>>
>>> Signed-off-by: Ley Foon Tan
>>> ---
>>> .../devicetree/bindings/mailbox/altera-mailbox.txt | 49 +++
>>> drivers/mailbox/Kconfig| 6 +
>>> drivers/mailbox/Makefile | 2 +
>>> drivers/mailbox/mailbox-altera.c | 404
>>> +
>>> 4 files changed, 461 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
>>> create mode 100644 drivers/mailbox/mailbox-altera.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
>>> b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
>>> new file mode 100644
>>> index 000..c261979
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
>>> @@ -0,0 +1,49 @@
>>> +Altera Mailbox Driver
>>> +=
>>> +
>>> +Required properties:
>>> +- compatible : "altr,mailbox-1.0".
>
> I am not sure on this convention, sounds like IP version number. Please
> check this with a DT maintainer.
Our other soft IPs compatible strings all have 1.0 version number. Eg:
altr,juart-1.0.
So, we want to keep it as same format.
>>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>>> index c04fed9..84325f2 100644
>>> --- a/drivers/mailbox/Kconfig
>>> +++ b/drivers/mailbox/Kconfig
>>> @@ -45,4 +45,10 @@ config PCC
>>>states). Select this driver if your platform implements the
>>>PCC clients mentioned above.
>>>
>>> +config ALTERA_MBOX
>>> +tristate "Altera Mailbox"
>
> You haven't used any depends on here, this driver is not applicable on
> all platforms, right?
It is a soft IP, so it is not limited to our platform only.
Other soft IP drivers also don't have 'depend on', so I think should be fine.
>>> +
>>> +static inline struct altera_mbox *to_altera_mbox(struct mbox_chan *chan)
>>> +{
>>> +if (!chan)
>>> +return NULL;
>>> +
>>> +return container_of(chan, struct altera_mbox, chan);
>>> +}
>>> +
>>> +static inline int altera_mbox_full(struct altera_mbox *mbox)
>>> +{
>>> +u32 status;
>>> +
>>> +status = __raw_readl(mbox->mbox_base + MAILBOX_STS_REG);
>
> You may want to replace all the __raw_readl/__raw_writel with regular
> readl/writel or its equivalent relaxed versions depending on your needs.
Okay.
>>> +static int altera_mbox_send_data(struct mbox_chan *chan, void *data)
>>> +{
>>> +struct altera_mbox *mbox = to_altera_mbox(chan);
>>> +u32 *udata = (u32 *)data;
>>> +
>>> +if (!mbox || !data)
>>> +return -EINVAL;
>>> +if (!mbox->is_sender) {
>>> +dev_warn(mbox->dev,
>>> +"failed to send. This is receiver mailbox.\n");
>>> +return -EINVAL;
>>> +}
>>> +
>>> +if (altera_mbox_full(mbox))
>>> +return -EBUSY;
>
> I think this logic in general will conflict between interrupt and poll
> mode. send_data is supposed to return -EBUSY only in polling mode and
> not in interrupt mode.
What is the recommended error code for this case? BTW, omap-mailbox.c
also return -EBUSY if mailbox is full.
>>> +
>>> +/* Enable interrupt before send */
>>> +altera_mbox_tx_intmask(mbox, true);
>
> Is this true and required in Polling mode too?
Add checking for interrupt mode here.
>
> Do you even need the chans variable, don't see it getting used
> anywhere. You are directly using the mbox->chan during registration..
Will update this.
>
>>
>>> +struct resource *regs;
>>> +int ret;
>>> +
>>> +mbox = devm_kzalloc(>dev, sizeof(struct altera_mbox),
>
> Use sizeof(*mbox)
Okay.
>
>>> +GFP_KERNEL);
>
> You have a few "Alignment should match open parenthesis" checks
> generated with the --strict option on checkpatch. I am not sure if Jassi
> is requiring them, but I would recommend that you fix all of them.
Okay, will check these.
>
>>> +if (!mbox)
>>> +return -ENOMEM;
>>> +
>>> +regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +if (!regs)
>>> +return -ENXIO;
>
> You can remove the check for regs, devm_ioremap_resource is capable
> of handling the error.
Okay.
>>> +
>>> +mbox->mbox_base = devm_ioremap_resource(>dev, regs);
>>> +if (IS_ERR(mbox->mbox_base))
>>> +return PTR_ERR(mbox->mbox_base);
>>> +
>>> +/* Check is it a sender or receiver? */
>>> +mbox->is_sender = altera_mbox_is_sender(mbox);
>>> +
>>> +mbox->irq = platform_get_irq(pdev, 0);
>>> +if (mbox->irq >= 0)
>>> +mbox->intr_mode = true;
>
> This seems to be a poor way of setting
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On Mon, Dec 15, 2014 at 10:22 PM, Dinh Nguyen wrote:
>
>
> On 12/15/14, 12:22 AM, Ley Foon Tan wrote:
>> On Fri, Dec 12, 2014 at 10:38 PM, Dinh Nguyen wrote:
>>
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
>>>
>>> Alphabetize the headers.
>> Okay.
>>
>>
+static int altera_mbox_startup_sender(struct mbox_chan *chan)
+{
+ int ret;
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+
+ if (mbox->intr_mode) {
+ ret = request_irq(mbox->irq, altera_mbox_tx_interrupt, 0,
>>>
>>> Use devm_request_irq, since you didn't have and don't need use free_irq
>>> in the shutdown function.
>> We want to free_irq when it shutdown. That means nobody requests for
>> that mailbox (or channel) and request_irq() again if someone requests
>> for a mailbox.
>>
>
> But you didn't have any free_irq in the shutdown function. Use
> devm_request_irq here, so you don't have to add them.
Hi Dinh
free_irq() is in altera_mbox_shutdown(). This function will be called
when user free/release the mailbox (channel).
But, devm_free_irq() only will be called when unload the module.
+static void altera_mbox_shutdown(struct mbox_chan *chan)
+{
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+
+ if (WARN_ON(!mbox))
+ return;
+
+ if (mbox->intr_mode) {
+ /* Unmask all interrupt masks */
+ __raw_writel(~0, mbox->mbox_base + MAILBOX_INTMASK_REG);
+ free_irq(mbox->irq, chan);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
Hi Ley Foon,
On 12/12/2014 08:38 AM, Dinh Nguyen wrote:
>
>
> On 12/12/14, 4:04 AM, Ley Foon Tan wrote:
>> The Altera mailbox allows for interprocessor communication. It supports
>> only one channel and work as either sender or receiver.
I have a few more comments in addition to those that Dinh provided.
>>
>> Signed-off-by: Ley Foon Tan
>> ---
>> .../devicetree/bindings/mailbox/altera-mailbox.txt | 49 +++
>> drivers/mailbox/Kconfig| 6 +
>> drivers/mailbox/Makefile | 2 +
>> drivers/mailbox/mailbox-altera.c | 404
>> +
>> 4 files changed, 461 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
>> create mode 100644 drivers/mailbox/mailbox-altera.c
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
>> b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
>> new file mode 100644
>> index 000..c261979
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
>> @@ -0,0 +1,49 @@
>> +Altera Mailbox Driver
>> +=
>> +
>> +Required properties:
>> +- compatible : "altr,mailbox-1.0".
I am not sure on this convention, sounds like IP version number. Please
check this with a DT maintainer.
>> +- reg : physical base address of the mailbox and length of
>> +memory mapped region.
>> +- #mbox-cells: Common mailbox binding property to identify the number
>> +of cells required for the mailbox specifier. Should be 1.
>> +
>> +Optional properties:
>> +- interrupt-parent :interrupt source phandle.
>> +- interrupts : interrupt number. The interrupt specifier format
>> +depends on the interrupt controller parent.
>> +
>> +Example:
>> +mbox_tx: mailbox@0x100 {
>> +compatible = "altr,mailbox-1.0";
>> +reg = <0x100 0x8>;
>> +interrupt-parent = < _0 >;
>> +interrupts = <5>;
>> +#mbox-cells = <1>;
>> +};
>> +
>> +mbox_rx: mailbox@0x200 {
>> +compatible = "altr,mailbox-1.0";
>> +reg = <0x200 0x8>;
>> +interrupt-parent = < _0 >;
>> +interrupts = <6>;
>> +#mbox-cells = <1>;
>> +};
>> +
>> +Mailbox client
>> +===
>> +"mboxes" and the optional "mbox-names" (please see
>> +Documentation/devicetree/bindings/mailbox/mailbox.txt for details). Each
>> value
>> +of the mboxes property should contain a phandle to the mailbox controller
>> +device node and second argument is the channel index. It must be 0 (hardware
>> +support only one channel).The equivalent "mbox-names" property value can be
>> +used to give a name to the communication channel to be used by the client
>> user.
>> +
>> +Example:
>> +mclient0: mclient0@0x400 {
>> +compatible = "client-1.0";
>> +reg = <0x400 0x10>;
>> +mbox-names = "mbox-tx", "mbox-rx";
>> +mboxes = <_tx 0>,
>> + <_rx 0>;
>> +};
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> index c04fed9..84325f2 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -45,4 +45,10 @@ config PCC
>>states). Select this driver if your platform implements the
>>PCC clients mentioned above.
>>
>> +config ALTERA_MBOX
>> +tristate "Altera Mailbox"
You haven't used any depends on here, this driver is not applicable on
all platforms, right?
>> +help
>> + An implementation of the Altera Mailbox soft core. It is used
>> + to send message between processors. Say Y here if you want to use the
>> + Altera mailbox support.
>> endif
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>> index dd412c2..2e79231 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -7,3 +7,5 @@ obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o
>> obj-$(CONFIG_OMAP2PLUS_MBOX)+= omap-mailbox.o
>>
>> obj-$(CONFIG_PCC) += pcc.o
>> +
>> +obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o
>> diff --git a/drivers/mailbox/mailbox-altera.c
>> b/drivers/mailbox/mailbox-altera.c
>> new file mode 100644
>> index 000..9ed10de
>> --- /dev/null
>> +++ b/drivers/mailbox/mailbox-altera.c
>> @@ -0,0 +1,404 @@
>> +/*
>> + * Copyright Altera Corporation (C) 2013-2014. All rights reserved
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On 12/15/14, 12:22 AM, Ley Foon Tan wrote:
> On Fri, Dec 12, 2014 at 10:38 PM, Dinh Nguyen wrote:
>
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>
>> Alphabetize the headers.
> Okay.
>
>
>>> +static int altera_mbox_startup_sender(struct mbox_chan *chan)
>>> +{
>>> + int ret;
>>> + struct altera_mbox *mbox = to_altera_mbox(chan);
>>> +
>>> + if (mbox->intr_mode) {
>>> + ret = request_irq(mbox->irq, altera_mbox_tx_interrupt, 0,
>>
>> Use devm_request_irq, since you didn't have and don't need use free_irq
>> in the shutdown function.
> We want to free_irq when it shutdown. That means nobody requests for
> that mailbox (or channel) and request_irq() again if someone requests
> for a mailbox.
>
But you didn't have any free_irq in the shutdown function. Use
devm_request_irq here, so you don't have to add them.
Dinh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On 12/15/14, 12:22 AM, Ley Foon Tan wrote:
On Fri, Dec 12, 2014 at 10:38 PM, Dinh Nguyen dinh.li...@gmail.com wrote:
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/device.h
+#include linux/platform_device.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/of.h
+#include linux/mailbox_controller.h
Alphabetize the headers.
Okay.
+static int altera_mbox_startup_sender(struct mbox_chan *chan)
+{
+ int ret;
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+
+ if (mbox-intr_mode) {
+ ret = request_irq(mbox-irq, altera_mbox_tx_interrupt, 0,
Use devm_request_irq, since you didn't have and don't need use free_irq
in the shutdown function.
We want to free_irq when it shutdown. That means nobody requests for
that mailbox (or channel) and request_irq() again if someone requests
for a mailbox.
But you didn't have any free_irq in the shutdown function. Use
devm_request_irq here, so you don't have to add them.
Dinh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
Hi Ley Foon,
On 12/12/2014 08:38 AM, Dinh Nguyen wrote:
On 12/12/14, 4:04 AM, Ley Foon Tan wrote:
The Altera mailbox allows for interprocessor communication. It supports
only one channel and work as either sender or receiver.
I have a few more comments in addition to those that Dinh provided.
Signed-off-by: Ley Foon Tan lf...@altera.com
---
.../devicetree/bindings/mailbox/altera-mailbox.txt | 49 +++
drivers/mailbox/Kconfig| 6 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-altera.c | 404
+
4 files changed, 461 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-altera.c
diff --git a/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
new file mode 100644
index 000..c261979
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
@@ -0,0 +1,49 @@
+Altera Mailbox Driver
+=
+
+Required properties:
+- compatible : altr,mailbox-1.0.
I am not sure on this convention, sounds like IP version number. Please
check this with a DT maintainer.
+- reg : physical base address of the mailbox and length of
+memory mapped region.
+- #mbox-cells: Common mailbox binding property to identify the number
+of cells required for the mailbox specifier. Should be 1.
+
+Optional properties:
+- interrupt-parent :interrupt source phandle.
+- interrupts : interrupt number. The interrupt specifier format
+depends on the interrupt controller parent.
+
+Example:
+mbox_tx: mailbox@0x100 {
+compatible = altr,mailbox-1.0;
+reg = 0x100 0x8;
+interrupt-parent = gic_0 ;
+interrupts = 5;
+#mbox-cells = 1;
+};
+
+mbox_rx: mailbox@0x200 {
+compatible = altr,mailbox-1.0;
+reg = 0x200 0x8;
+interrupt-parent = gic_0 ;
+interrupts = 6;
+#mbox-cells = 1;
+};
+
+Mailbox client
+===
+mboxes and the optional mbox-names (please see
+Documentation/devicetree/bindings/mailbox/mailbox.txt for details). Each
value
+of the mboxes property should contain a phandle to the mailbox controller
+device node and second argument is the channel index. It must be 0 (hardware
+support only one channel).The equivalent mbox-names property value can be
+used to give a name to the communication channel to be used by the client
user.
+
+Example:
+mclient0: mclient0@0x400 {
+compatible = client-1.0;
+reg = 0x400 0x10;
+mbox-names = mbox-tx, mbox-rx;
+mboxes = mbox_tx 0,
+ mbox_rx 0;
+};
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c04fed9..84325f2 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -45,4 +45,10 @@ config PCC
states). Select this driver if your platform implements the
PCC clients mentioned above.
+config ALTERA_MBOX
+tristate Altera Mailbox
You haven't used any depends on here, this driver is not applicable on
all platforms, right?
+help
+ An implementation of the Altera Mailbox soft core. It is used
+ to send message between processors. Say Y here if you want to use the
+ Altera mailbox support.
endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index dd412c2..2e79231 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -7,3 +7,5 @@ obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o
obj-$(CONFIG_OMAP2PLUS_MBOX)+= omap-mailbox.o
obj-$(CONFIG_PCC) += pcc.o
+
+obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o
diff --git a/drivers/mailbox/mailbox-altera.c
b/drivers/mailbox/mailbox-altera.c
new file mode 100644
index 000..9ed10de
--- /dev/null
+++ b/drivers/mailbox/mailbox-altera.c
@@ -0,0 +1,404 @@
+/*
+ * Copyright Altera Corporation (C) 2013-2014. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
with
+ * this program. If not, see http://www.gnu.org/licenses/.
+ */
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/device.h
+#include linux/platform_device.h
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On Mon, Dec 15, 2014 at 10:22 PM, Dinh Nguyen dinh.li...@gmail.com wrote:
On 12/15/14, 12:22 AM, Ley Foon Tan wrote:
On Fri, Dec 12, 2014 at 10:38 PM, Dinh Nguyen dinh.li...@gmail.com wrote:
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/device.h
+#include linux/platform_device.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/of.h
+#include linux/mailbox_controller.h
Alphabetize the headers.
Okay.
+static int altera_mbox_startup_sender(struct mbox_chan *chan)
+{
+ int ret;
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+
+ if (mbox-intr_mode) {
+ ret = request_irq(mbox-irq, altera_mbox_tx_interrupt, 0,
Use devm_request_irq, since you didn't have and don't need use free_irq
in the shutdown function.
We want to free_irq when it shutdown. That means nobody requests for
that mailbox (or channel) and request_irq() again if someone requests
for a mailbox.
But you didn't have any free_irq in the shutdown function. Use
devm_request_irq here, so you don't have to add them.
Hi Dinh
free_irq() is in altera_mbox_shutdown(). This function will be called
when user free/release the mailbox (channel).
But, devm_free_irq() only will be called when unload the module.
+static void altera_mbox_shutdown(struct mbox_chan *chan)
+{
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+
+ if (WARN_ON(!mbox))
+ return;
+
+ if (mbox-intr_mode) {
+ /* Unmask all interrupt masks */
+ __raw_writel(~0, mbox-mbox_base + MAILBOX_INTMASK_REG);
+ free_irq(mbox-irq, chan);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On Tue, Dec 16, 2014 at 4:54 AM, Suman Anna s-a...@ti.com wrote:
Hi Ley Foon,
On 12/12/2014 08:38 AM, Dinh Nguyen wrote:
On 12/12/14, 4:04 AM, Ley Foon Tan wrote:
The Altera mailbox allows for interprocessor communication. It supports
only one channel and work as either sender or receiver.
I have a few more comments in addition to those that Dinh provided.
Signed-off-by: Ley Foon Tan lf...@altera.com
---
.../devicetree/bindings/mailbox/altera-mailbox.txt | 49 +++
drivers/mailbox/Kconfig| 6 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-altera.c | 404
+
4 files changed, 461 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-altera.c
diff --git a/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
new file mode 100644
index 000..c261979
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
@@ -0,0 +1,49 @@
+Altera Mailbox Driver
+=
+
+Required properties:
+- compatible : altr,mailbox-1.0.
I am not sure on this convention, sounds like IP version number. Please
check this with a DT maintainer.
Our other soft IPs compatible strings all have 1.0 version number. Eg:
altr,juart-1.0.
So, we want to keep it as same format.
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c04fed9..84325f2 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -45,4 +45,10 @@ config PCC
states). Select this driver if your platform implements the
PCC clients mentioned above.
+config ALTERA_MBOX
+tristate Altera Mailbox
You haven't used any depends on here, this driver is not applicable on
all platforms, right?
It is a soft IP, so it is not limited to our platform only.
Other soft IP drivers also don't have 'depend on', so I think should be fine.
+
+static inline struct altera_mbox *to_altera_mbox(struct mbox_chan *chan)
+{
+if (!chan)
+return NULL;
+
+return container_of(chan, struct altera_mbox, chan);
+}
+
+static inline int altera_mbox_full(struct altera_mbox *mbox)
+{
+u32 status;
+
+status = __raw_readl(mbox-mbox_base + MAILBOX_STS_REG);
You may want to replace all the __raw_readl/__raw_writel with regular
readl/writel or its equivalent relaxed versions depending on your needs.
Okay.
+static int altera_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+struct altera_mbox *mbox = to_altera_mbox(chan);
+u32 *udata = (u32 *)data;
+
+if (!mbox || !data)
+return -EINVAL;
+if (!mbox-is_sender) {
+dev_warn(mbox-dev,
+failed to send. This is receiver mailbox.\n);
+return -EINVAL;
+}
+
+if (altera_mbox_full(mbox))
+return -EBUSY;
I think this logic in general will conflict between interrupt and poll
mode. send_data is supposed to return -EBUSY only in polling mode and
not in interrupt mode.
What is the recommended error code for this case? BTW, omap-mailbox.c
also return -EBUSY if mailbox is full.
+
+/* Enable interrupt before send */
+altera_mbox_tx_intmask(mbox, true);
Is this true and required in Polling mode too?
Add checking for interrupt mode here.
Do you even need the chans variable, don't see it getting used
anywhere. You are directly using the mbox-chan during registration..
Will update this.
+struct resource *regs;
+int ret;
+
+mbox = devm_kzalloc(pdev-dev, sizeof(struct altera_mbox),
Use sizeof(*mbox)
Okay.
+GFP_KERNEL);
You have a few Alignment should match open parenthesis checks
generated with the --strict option on checkpatch. I am not sure if Jassi
is requiring them, but I would recommend that you fix all of them.
Okay, will check these.
+if (!mbox)
+return -ENOMEM;
+
+regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+if (!regs)
+return -ENXIO;
You can remove the check for regs, devm_ioremap_resource is capable
of handling the error.
Okay.
+
+mbox-mbox_base = devm_ioremap_resource(pdev-dev, regs);
+if (IS_ERR(mbox-mbox_base))
+return PTR_ERR(mbox-mbox_base);
+
+/* Check is it a sender or receiver? */
+mbox-is_sender = altera_mbox_is_sender(mbox);
+
+mbox-irq = platform_get_irq(pdev, 0);
+if (mbox-irq = 0)
+mbox-intr_mode = true;
This seems to be a poor way of setting up the mode. Is it the same IP
block but different integration on different SoCs? Or on the same SoC,
and you can use it either by interrupt driven or by polling.
Yes, it can use interrupt or polling mode depends on hardware configurations.
It is a soft IP and can be configured with different
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On Fri, Dec 12, 2014 at 10:38 PM, Dinh Nguyen wrote:
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>
> Alphabetize the headers.
Okay.
>> +static int altera_mbox_startup_sender(struct mbox_chan *chan)
>> +{
>> + int ret;
>> + struct altera_mbox *mbox = to_altera_mbox(chan);
>> +
>> + if (mbox->intr_mode) {
>> + ret = request_irq(mbox->irq, altera_mbox_tx_interrupt, 0,
>
> Use devm_request_irq, since you didn't have and don't need use free_irq
> in the shutdown function.
We want to free_irq when it shutdown. That means nobody requests for
that mailbox (or channel) and request_irq() again if someone requests
for a mailbox.
>
>> + DRIVER_NAME, chan);
>> + if (unlikely(ret)) {
>> + dev_err(mbox->dev,
>> + "failed to register mailbox interrupt:%d\n",
>> + ret);
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int altera_mbox_startup_receiver(struct mbox_chan *chan)
>> +{
>> + int ret;
>> + struct altera_mbox *mbox = to_altera_mbox(chan);
>> +
>> + if (mbox->intr_mode) {
>> + ret = request_irq(mbox->irq, altera_mbox_rx_interrupt, 0,
>> + DRIVER_NAME, chan);
>
> Use devm_request_irq
>
>
>> + if (unlikely(ret)) {
>> + dev_err(mbox->dev,
>> + "failed to register mailbox interrupt:%d\n",
>> + ret);
>> + return ret;
>> + }
>> + altera_mbox_rx_intmask(mbox, true);
>> + } else {
>> + /* Setup polling timer */
>> + setup_timer(>rxpoll_timer, altera_mbox_poll_rx,
>> + (unsigned long)chan);
>> + mod_timer(>rxpoll_timer,
>> + jiffies + msecs_to_jiffies(MBOX_POLLING_MS));
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int altera_mbox_send_data(struct mbox_chan *chan, void *data)
>> +{
>> + struct altera_mbox *mbox = to_altera_mbox(chan);
>> + u32 *udata = (u32 *)data;
>> +
>> + if (!mbox || !data)
>> + return -EINVAL;
>> + if (!mbox->is_sender) {
>> + dev_warn(mbox->dev,
>> + "failed to send. This is receiver mailbox.\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (altera_mbox_full(mbox))
>> + return -EBUSY;
>> +
>> + /* Enable interrupt before send */
>> + altera_mbox_tx_intmask(mbox, true);
>> +
>> + /* Pointer register must write before command register */
>> + __raw_writel(udata[MBOX_PTR], mbox->mbox_base + MAILBOX_PTR_REG);
>> + __raw_writel(udata[MBOX_CMD], mbox->mbox_base + MAILBOX_CMD_REG);
>> +
>> + return 0;
>> +}
>> +
>> +static bool altera_mbox_last_tx_done(struct mbox_chan *chan)
>> +{
>> + struct altera_mbox *mbox = to_altera_mbox(chan);
>> +
>> + if (WARN_ON(!mbox))
>> + return false;
>
> Are these checks necessary? Shouldn't the driver probe function have
> already failed?
Will removed.
>> +
>> + /* Return false if mailbox is full */
>> + return altera_mbox_full(mbox) ? false : true;
>> +}
>> +
>> +static bool altera_mbox_peek_data(struct mbox_chan *chan)
>> +{
>> + struct altera_mbox *mbox = to_altera_mbox(chan);
>> +
>> + if (WARN_ON(!mbox))
>> + return false;
>> +
>> + return altera_mbox_pending(mbox) ? true : false;
>> +}
>> +
>> +static int altera_mbox_startup(struct mbox_chan *chan)
>> +{
>> + struct altera_mbox *mbox = to_altera_mbox(chan);
>> + int ret = 0;
>> +
>> + if (!mbox)
>> + return -EINVAL;
>> +
>> + if (mbox->is_sender)
>> + ret = altera_mbox_startup_sender(chan);
>> + else
>> + ret = altera_mbox_startup_receiver(chan);
>> +
>> + return ret;
>> +}
>> +
>> +static void altera_mbox_shutdown(struct mbox_chan *chan)
>> +{
>> + struct altera_mbox *mbox = to_altera_mbox(chan);
>> +
>> + if (WARN_ON(!mbox))
>> + return;
>> +
>> + if (mbox->intr_mode) {
>> + /* Unmask all interrupt masks */
>> + __raw_writel(~0, mbox->mbox_base + MAILBOX_INTMASK_REG);
>> + free_irq(mbox->irq, chan);
>> + } else if (!mbox->is_sender)
>> + del_timer_sync(>rxpoll_timer);
>
> Need braces around the else as well.
Noted.
>
>> +}
>> +
>> +static struct mbox_chan_ops altera_mbox_ops = {
>> + .send_data = altera_mbox_send_data,
>> + .startup = altera_mbox_startup,
>> + .shutdown = altera_mbox_shutdown,
>> + .last_tx_done = altera_mbox_last_tx_done,
>> + .peek_data = altera_mbox_peek_data,
>> +};
>> +
>> +static int altera_mbox_probe(struct platform_device *pdev)
>> +{
>> + struct altera_mbox *mbox;
>> + struct mbox_chan *chans[1];
>
> It would be cleaner if you use
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On Fri, Dec 12, 2014 at 10:38 PM, Dinh Nguyen dinh.li...@gmail.com wrote:
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/device.h
+#include linux/platform_device.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/of.h
+#include linux/mailbox_controller.h
Alphabetize the headers.
Okay.
+static int altera_mbox_startup_sender(struct mbox_chan *chan)
+{
+ int ret;
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+
+ if (mbox-intr_mode) {
+ ret = request_irq(mbox-irq, altera_mbox_tx_interrupt, 0,
Use devm_request_irq, since you didn't have and don't need use free_irq
in the shutdown function.
We want to free_irq when it shutdown. That means nobody requests for
that mailbox (or channel) and request_irq() again if someone requests
for a mailbox.
+ DRIVER_NAME, chan);
+ if (unlikely(ret)) {
+ dev_err(mbox-dev,
+ failed to register mailbox interrupt:%d\n,
+ ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int altera_mbox_startup_receiver(struct mbox_chan *chan)
+{
+ int ret;
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+
+ if (mbox-intr_mode) {
+ ret = request_irq(mbox-irq, altera_mbox_rx_interrupt, 0,
+ DRIVER_NAME, chan);
Use devm_request_irq
+ if (unlikely(ret)) {
+ dev_err(mbox-dev,
+ failed to register mailbox interrupt:%d\n,
+ ret);
+ return ret;
+ }
+ altera_mbox_rx_intmask(mbox, true);
+ } else {
+ /* Setup polling timer */
+ setup_timer(mbox-rxpoll_timer, altera_mbox_poll_rx,
+ (unsigned long)chan);
+ mod_timer(mbox-rxpoll_timer,
+ jiffies + msecs_to_jiffies(MBOX_POLLING_MS));
+ }
+
+ return 0;
+}
+
+static int altera_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+ u32 *udata = (u32 *)data;
+
+ if (!mbox || !data)
+ return -EINVAL;
+ if (!mbox-is_sender) {
+ dev_warn(mbox-dev,
+ failed to send. This is receiver mailbox.\n);
+ return -EINVAL;
+ }
+
+ if (altera_mbox_full(mbox))
+ return -EBUSY;
+
+ /* Enable interrupt before send */
+ altera_mbox_tx_intmask(mbox, true);
+
+ /* Pointer register must write before command register */
+ __raw_writel(udata[MBOX_PTR], mbox-mbox_base + MAILBOX_PTR_REG);
+ __raw_writel(udata[MBOX_CMD], mbox-mbox_base + MAILBOX_CMD_REG);
+
+ return 0;
+}
+
+static bool altera_mbox_last_tx_done(struct mbox_chan *chan)
+{
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+
+ if (WARN_ON(!mbox))
+ return false;
Are these checks necessary? Shouldn't the driver probe function have
already failed?
Will removed.
+
+ /* Return false if mailbox is full */
+ return altera_mbox_full(mbox) ? false : true;
+}
+
+static bool altera_mbox_peek_data(struct mbox_chan *chan)
+{
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+
+ if (WARN_ON(!mbox))
+ return false;
+
+ return altera_mbox_pending(mbox) ? true : false;
+}
+
+static int altera_mbox_startup(struct mbox_chan *chan)
+{
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+ int ret = 0;
+
+ if (!mbox)
+ return -EINVAL;
+
+ if (mbox-is_sender)
+ ret = altera_mbox_startup_sender(chan);
+ else
+ ret = altera_mbox_startup_receiver(chan);
+
+ return ret;
+}
+
+static void altera_mbox_shutdown(struct mbox_chan *chan)
+{
+ struct altera_mbox *mbox = to_altera_mbox(chan);
+
+ if (WARN_ON(!mbox))
+ return;
+
+ if (mbox-intr_mode) {
+ /* Unmask all interrupt masks */
+ __raw_writel(~0, mbox-mbox_base + MAILBOX_INTMASK_REG);
+ free_irq(mbox-irq, chan);
+ } else if (!mbox-is_sender)
+ del_timer_sync(mbox-rxpoll_timer);
Need braces around the else as well.
Noted.
+}
+
+static struct mbox_chan_ops altera_mbox_ops = {
+ .send_data = altera_mbox_send_data,
+ .startup = altera_mbox_startup,
+ .shutdown = altera_mbox_shutdown,
+ .last_tx_done = altera_mbox_last_tx_done,
+ .peek_data = altera_mbox_peek_data,
+};
+
+static int altera_mbox_probe(struct platform_device *pdev)
+{
+ struct altera_mbox *mbox;
+ struct mbox_chan *chans[1];
It would be cleaner if you use devm_kzalloc to allocate the channel here.
Okay.
+ struct resource *regs;
+ int ret;
+
+ mbox = devm_kzalloc(pdev-dev, sizeof(struct altera_mbox),
+
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On 12/12/14, 4:04 AM, Ley Foon Tan wrote:
> The Altera mailbox allows for interprocessor communication. It supports
> only one channel and work as either sender or receiver.
>
> Signed-off-by: Ley Foon Tan
> ---
> .../devicetree/bindings/mailbox/altera-mailbox.txt | 49 +++
> drivers/mailbox/Kconfig| 6 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/mailbox-altera.c | 404
> +
> 4 files changed, 461 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
> create mode 100644 drivers/mailbox/mailbox-altera.c
>
> diff --git a/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
> b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
> new file mode 100644
> index 000..c261979
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
> @@ -0,0 +1,49 @@
> +Altera Mailbox Driver
> +=
> +
> +Required properties:
> +- compatible : "altr,mailbox-1.0".
> +- reg : physical base address of the mailbox and length of
> + memory mapped region.
> +- #mbox-cells: Common mailbox binding property to identify the number
> + of cells required for the mailbox specifier. Should be 1.
> +
> +Optional properties:
> +- interrupt-parent : interrupt source phandle.
> +- interrupts : interrupt number. The interrupt specifier format
> + depends on the interrupt controller parent.
> +
> +Example:
> + mbox_tx: mailbox@0x100 {
> + compatible = "altr,mailbox-1.0";
> + reg = <0x100 0x8>;
> + interrupt-parent = < _0 >;
> + interrupts = <5>;
> + #mbox-cells = <1>;
> + };
> +
> + mbox_rx: mailbox@0x200 {
> + compatible = "altr,mailbox-1.0";
> + reg = <0x200 0x8>;
> + interrupt-parent = < _0 >;
> + interrupts = <6>;
> + #mbox-cells = <1>;
> + };
> +
> +Mailbox client
> +===
> +"mboxes" and the optional "mbox-names" (please see
> +Documentation/devicetree/bindings/mailbox/mailbox.txt for details). Each
> value
> +of the mboxes property should contain a phandle to the mailbox controller
> +device node and second argument is the channel index. It must be 0 (hardware
> +support only one channel).The equivalent "mbox-names" property value can be
> +used to give a name to the communication channel to be used by the client
> user.
> +
> +Example:
> + mclient0: mclient0@0x400 {
> + compatible = "client-1.0";
> + reg = <0x400 0x10>;
> + mbox-names = "mbox-tx", "mbox-rx";
> + mboxes = <_tx 0>,
> + <_rx 0>;
> + };
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index c04fed9..84325f2 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -45,4 +45,10 @@ config PCC
> states). Select this driver if your platform implements the
> PCC clients mentioned above.
>
> +config ALTERA_MBOX
> + tristate "Altera Mailbox"
> + help
> + An implementation of the Altera Mailbox soft core. It is used
> + to send message between processors. Say Y here if you want to use the
> + Altera mailbox support.
> endif
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index dd412c2..2e79231 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,3 +7,5 @@ obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o
> obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o
>
> obj-$(CONFIG_PCC)+= pcc.o
> +
> +obj-$(CONFIG_ALTERA_MBOX)+= mailbox-altera.o
> diff --git a/drivers/mailbox/mailbox-altera.c
> b/drivers/mailbox/mailbox-altera.c
> new file mode 100644
> index 000..9ed10de
> --- /dev/null
> +++ b/drivers/mailbox/mailbox-altera.c
> @@ -0,0 +1,404 @@
> +/*
> + * Copyright Altera Corporation (C) 2013-2014. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> with
> + * this program. If not, see
Re: [PATCH (resend)] mailbox: Add Altera mailbox driver
On 12/12/14, 4:04 AM, Ley Foon Tan wrote:
The Altera mailbox allows for interprocessor communication. It supports
only one channel and work as either sender or receiver.
Signed-off-by: Ley Foon Tan lf...@altera.com
---
.../devicetree/bindings/mailbox/altera-mailbox.txt | 49 +++
drivers/mailbox/Kconfig| 6 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-altera.c | 404
+
4 files changed, 461 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-altera.c
diff --git a/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
new file mode 100644
index 000..c261979
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/altera-mailbox.txt
@@ -0,0 +1,49 @@
+Altera Mailbox Driver
+=
+
+Required properties:
+- compatible : altr,mailbox-1.0.
+- reg : physical base address of the mailbox and length of
+ memory mapped region.
+- #mbox-cells: Common mailbox binding property to identify the number
+ of cells required for the mailbox specifier. Should be 1.
+
+Optional properties:
+- interrupt-parent : interrupt source phandle.
+- interrupts : interrupt number. The interrupt specifier format
+ depends on the interrupt controller parent.
+
+Example:
+ mbox_tx: mailbox@0x100 {
+ compatible = altr,mailbox-1.0;
+ reg = 0x100 0x8;
+ interrupt-parent = gic_0 ;
+ interrupts = 5;
+ #mbox-cells = 1;
+ };
+
+ mbox_rx: mailbox@0x200 {
+ compatible = altr,mailbox-1.0;
+ reg = 0x200 0x8;
+ interrupt-parent = gic_0 ;
+ interrupts = 6;
+ #mbox-cells = 1;
+ };
+
+Mailbox client
+===
+mboxes and the optional mbox-names (please see
+Documentation/devicetree/bindings/mailbox/mailbox.txt for details). Each
value
+of the mboxes property should contain a phandle to the mailbox controller
+device node and second argument is the channel index. It must be 0 (hardware
+support only one channel).The equivalent mbox-names property value can be
+used to give a name to the communication channel to be used by the client
user.
+
+Example:
+ mclient0: mclient0@0x400 {
+ compatible = client-1.0;
+ reg = 0x400 0x10;
+ mbox-names = mbox-tx, mbox-rx;
+ mboxes = mbox_tx 0,
+ mbox_rx 0;
+ };
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c04fed9..84325f2 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -45,4 +45,10 @@ config PCC
states). Select this driver if your platform implements the
PCC clients mentioned above.
+config ALTERA_MBOX
+ tristate Altera Mailbox
+ help
+ An implementation of the Altera Mailbox soft core. It is used
+ to send message between processors. Say Y here if you want to use the
+ Altera mailbox support.
endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index dd412c2..2e79231 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -7,3 +7,5 @@ obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o
obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o
obj-$(CONFIG_PCC)+= pcc.o
+
+obj-$(CONFIG_ALTERA_MBOX)+= mailbox-altera.o
diff --git a/drivers/mailbox/mailbox-altera.c
b/drivers/mailbox/mailbox-altera.c
new file mode 100644
index 000..9ed10de
--- /dev/null
+++ b/drivers/mailbox/mailbox-altera.c
@@ -0,0 +1,404 @@
+/*
+ * Copyright Altera Corporation (C) 2013-2014. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
with
+ * this program. If not, see http://www.gnu.org/licenses/.
+ */
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/device.h
+#include linux/platform_device.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/of.h
+#include linux/mailbox_controller.h
Alphabetize the headers.
+
+#define DRIVER_NAME altera-mailbox
+
+#define MAILBOX_CMD_REG 0x00
+#define MAILBOX_PTR_REG 0x04
+#define
