[PATCH 0/3] MPFS mailbox fixes

[PATCH 0/3] MPFS mailbox fixes

[Conor Dooley]

Hey all,

I spotted a couple of bugs in my mailbox driver while developing some
new features. None of the features these bugs relate to were in use so
they've gone unnoticed until now. The binding screwup is unfortunate
and I don't really know how I misread the register map so badly.

Jassi:
Not sure if you prefer developers to add a CC: stable or not to patches
so I have left them out, but I would like to see them backported.

Thanks,
Conor.

Conor Dooley (3):
  dt-bindings: mailbox: fix the mpfs' reg property
  mailbox: mpfs: fix handling of the reg property
  mailbox: mpfs: account for mbox offsets while sending

 .../mailbox/microchip,mpfs-mailbox.yaml       | 15 ++++++++---
 drivers/mailbox/mailbox-mpfs.c                | 25 +++++++++++--------
 2 files changed, 25 insertions(+), 15 deletions(-)

-- 
2.36.1

[PATCH 1/3] dt-bindings: mailbox: fix the mpfs' reg property

[Conor Dooley]

The "data" region of the PolarFire SoC's system controller mailbox is
not one continuous register space - the system controller's QSPI sits
between the control and data registers. Split the "data" reg into two
parts: "data" & "control".

Fixes: 213556235526 ("dt-bindings: soc/microchip: update syscontroller compatibles")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/mailbox/microchip,mpfs-mailbox.yaml  | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/microchip,mpfs-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/microchip,mpfs-mailbox.yaml
index 082d397d3e89..935937c67133 100644
--- a/Documentation/devicetree/bindings/mailbox/microchip,mpfs-mailbox.yaml
+++ b/Documentation/devicetree/bindings/mailbox/microchip,mpfs-mailbox.yaml
@@ -14,9 +14,15 @@ properties:
     const: microchip,mpfs-mailbox
 
   reg:
-    items:
-      - description: mailbox data registers
-      - description: mailbox interrupt registers
+    oneOf:
+      - items:
+          - description: mailbox control & data registers
+          - description: mailbox interrupt registers
+        deprecated: true
+      - items:
+          - description: mailbox control registers
+          - description: mailbox interrupt registers
+          - description: mailbox data registers
 
   interrupts:
     maxItems: 1
@@ -39,7 +45,8 @@ examples:
       #size-cells = <2>;
       mbox: mailbox@37020000 {
         compatible = "microchip,mpfs-mailbox";
-        reg = <0x0 0x37020000 0x0 0x1000>, <0x0 0x2000318c 0x0 0x40>;
+        reg = <0x0 0x37020000 0x0 0x58>, <0x0 0x2000318C 0x0 0x40>,
+              <0x0 0x37020800 0x0 0x100>;
         interrupt-parent = <&L1>;
         interrupts = <96>;
         #mbox-cells = <1>;
-- 
2.36.1

[PATCH 2/3] mailbox: mpfs: fix handling of the reg property

[Conor Dooley]

The "data" region of the PolarFire SoC's system controller mailbox is
not one continuous register space - the system controller's QSPI sits
between the control and data registers. Split the "data" reg into two
parts: "data" & "control". Optionally get the "data" register address
from the 3rd reg property in the devicetree & fall back to using the
old base + MAILBOX_REG_OFFSET that the current code uses.

Fixes: 83d7b1560810 ("mbox: add polarfire soc system controller mailbox")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/mailbox/mailbox-mpfs.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c
index 4e34854d1238..e432a8f0d148 100644
--- a/drivers/mailbox/mailbox-mpfs.c
+++ b/drivers/mailbox/mailbox-mpfs.c
@@ -62,6 +62,7 @@ struct mpfs_mbox {
 	struct mbox_controller controller;
 	struct device *dev;
 	int irq;
+	void __iomem *ctrl_base;
 	void __iomem *mbox_base;
 	void __iomem *int_reg;
 	struct mbox_chan chans[1];
@@ -73,7 +74,7 @@ static bool mpfs_mbox_busy(struct mpfs_mbox *mbox)
 {
 	u32 status;
 
-	status = readl_relaxed(mbox->mbox_base + SERVICES_SR_OFFSET);
+	status = readl_relaxed(mbox->ctrl_base + SERVICES_SR_OFFSET);
 
 	return status & SCB_STATUS_BUSY_MASK;
 }
@@ -99,14 +100,13 @@ static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data)
 
 		for (index = 0; index < (msg->cmd_data_size / 4); index++)
 			writel_relaxed(word_buf[index],
-				       mbox->mbox_base + MAILBOX_REG_OFFSET + index * 0x4);
+				       mbox->mbox_base + index * 0x4);
 		if (extra_bits) {
 			u8 i;
 			u8 byte_off = ALIGN_DOWN(msg->cmd_data_size, 4);
 			u8 *byte_buf = msg->cmd_data + byte_off;
 
-			val = readl_relaxed(mbox->mbox_base +
-					    MAILBOX_REG_OFFSET + index * 0x4);
+			val = readl_relaxed(mbox->mbox_base + index * 0x4);
 
 			for (i = 0u; i < extra_bits; i++) {
 				val &= ~(0xffu << (i * 8u));
@@ -114,14 +114,14 @@ static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data)
 			}
 
 			writel_relaxed(val,
-				       mbox->mbox_base + MAILBOX_REG_OFFSET + index * 0x4);
+				       mbox->mbox_base + index * 0x4);
 		}
 	}
 
 	opt_sel = ((msg->mbox_offset << 7u) | (msg->cmd_opcode & 0x7fu));
 	tx_trigger = (opt_sel << SCB_CTRL_POS) & SCB_CTRL_MASK;
 	tx_trigger |= SCB_CTRL_REQ_MASK | SCB_STATUS_NOTIFY_MASK;
-	writel_relaxed(tx_trigger, mbox->mbox_base + SERVICES_CR_OFFSET);
+	writel_relaxed(tx_trigger, mbox->ctrl_base + SERVICES_CR_OFFSET);
 
 	return 0;
 }
@@ -141,7 +141,7 @@ static void mpfs_mbox_rx_data(struct mbox_chan *chan)
 	if (!mpfs_mbox_busy(mbox)) {
 		for (i = 0; i < num_words; i++) {
 			response->resp_msg[i] =
-				readl_relaxed(mbox->mbox_base + MAILBOX_REG_OFFSET
+				readl_relaxed(mbox->mbox_base
 					      + mbox->resp_offset + i * 0x4);
 		}
 	}
@@ -200,14 +200,18 @@ static int mpfs_mbox_probe(struct platform_device *pdev)
 	if (!mbox)
 		return -ENOMEM;
 
-	mbox->mbox_base = devm_platform_get_and_ioremap_resource(pdev, 0, &regs);
-	if (IS_ERR(mbox->mbox_base))
-		return PTR_ERR(mbox->mbox_base);
+	mbox->ctrl_base = devm_platform_get_and_ioremap_resource(pdev, 0, &regs);
+	if (IS_ERR(mbox->ctrl_base))
+		return PTR_ERR(mbox->ctrl_base);
 
 	mbox->int_reg = devm_platform_get_and_ioremap_resource(pdev, 1, &regs);
 	if (IS_ERR(mbox->int_reg))
 		return PTR_ERR(mbox->int_reg);
 
+	mbox->mbox_base = devm_platform_get_and_ioremap_resource(pdev, 2, &regs);
+	if (IS_ERR(mbox->mbox_base)) // account for the old dt-binding w/ 2 regs
+		mbox->mbox_base = mbox->ctrl_base + MAILBOX_REG_OFFSET;
+
 	mbox->irq = platform_get_irq(pdev, 0);
 	if (mbox->irq < 0)
 		return mbox->irq;
-- 
2.36.1

[PATCH 3/3] mailbox: mpfs: account for mbox offsets while sending

[Conor Dooley]

The mailbox offset is not only used for receiving messages, but it is
also used by messages sent to the system controller by Linux that have a
payload, such as the "digital signature service". It is also overloaded
by certain other services (reprogramming of the FPGA fabric, see Link:)
to have a meaning other than the offset the system controller should
read from.
When the driver was written, no such services of the latter type were
in use & those of the former used an offset of zero so this has gone
un-noticed.

Link: https://www.microsemi.com/document-portal/doc_download/1245815-polarfire-fpga-and-polarfire-soc-fpga-system-services-user-guide # Section 5.2
Fixes: 83d7b1560810 ("mbox: add polarfire soc system controller mailbox")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/mailbox/mailbox-mpfs.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c
index e432a8f0d148..cfacb3f320a6 100644
--- a/drivers/mailbox/mailbox-mpfs.c
+++ b/drivers/mailbox/mailbox-mpfs.c
@@ -100,21 +100,20 @@ static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data)
 
 		for (index = 0; index < (msg->cmd_data_size / 4); index++)
 			writel_relaxed(word_buf[index],
-				       mbox->mbox_base + index * 0x4);
+				       mbox->mbox_base + msg->mbox_offset + index * 0x4);
 		if (extra_bits) {
 			u8 i;
 			u8 byte_off = ALIGN_DOWN(msg->cmd_data_size, 4);
 			u8 *byte_buf = msg->cmd_data + byte_off;
 
-			val = readl_relaxed(mbox->mbox_base + index * 0x4);
+			val = readl_relaxed(mbox->mbox_base + msg->mbox_offset + index * 0x4);
 
 			for (i = 0u; i < extra_bits; i++) {
 				val &= ~(0xffu << (i * 8u));
 				val |= (byte_buf[i] << (i * 8u));
 			}
 
-			writel_relaxed(val,
-				       mbox->mbox_base + index * 0x4);
+			writel_relaxed(val, mbox->mbox_base + msg->mbox_offset + index * 0x4);
 		}
 	}
 
-- 
2.36.1

Re: [PATCH 1/3] dt-bindings: mailbox: fix the mpfs' reg property

[Conor.Dooley]

On 05/08/2022 13:56, Conor Dooley wrote:
> The "data" region of the PolarFire SoC's system controller mailbox is
> not one continuous register space - the system controller's QSPI sits
> between the control and data registers. Split the "data" reg into two
> parts: "data" & "control".
> 
> Fixes: 213556235526 ("dt-bindings: soc/microchip: update syscontroller compatibles")

I omitted the second fixes tag:
Fixes: ed9543d6f2c4 ("dt-bindings: add bindings for polarfire soc mailbox")

> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>   .../bindings/mailbox/microchip,mpfs-mailbox.yaml  | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/microchip,mpfs-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/microchip,mpfs-mailbox.yaml
> index 082d397d3e89..935937c67133 100644
> --- a/Documentation/devicetree/bindings/mailbox/microchip,mpfs-mailbox.yaml
> +++ b/Documentation/devicetree/bindings/mailbox/microchip,mpfs-mailbox.yaml
> @@ -14,9 +14,15 @@ properties:
>       const: microchip,mpfs-mailbox
>   
>     reg:
> -    items:
> -      - description: mailbox data registers
> -      - description: mailbox interrupt registers
> +    oneOf:
> +      - items:
> +          - description: mailbox control & data registers
> +          - description: mailbox interrupt registers
> +        deprecated: true
> +      - items:
> +          - description: mailbox control registers
> +          - description: mailbox interrupt registers
> +          - description: mailbox data registers
>   
>     interrupts:
>       maxItems: 1
> @@ -39,7 +45,8 @@ examples:
>         #size-cells = <2>;
>         mbox: mailbox@37020000 {
>           compatible = "microchip,mpfs-mailbox";
> -        reg = <0x0 0x37020000 0x0 0x1000>, <0x0 0x2000318c 0x0 0x40>;
> +        reg = <0x0 0x37020000 0x0 0x58>, <0x0 0x2000318C 0x0 0x40>,
> +              <0x0 0x37020800 0x0 0x100>;
>           interrupt-parent = <&L1>;
>           interrupts = <96>;
>           #mbox-cells = <1>;

Re: [PATCH 1/3] dt-bindings: mailbox: fix the mpfs' reg property

[Krzysztof Kozlowski]

On 05/08/2022 15:01, Conor.Dooley@microchip.com wrote:
> On 05/08/2022 13:56, Conor Dooley wrote:
>> The "data" region of the PolarFire SoC's system controller mailbox is
>> not one continuous register space - the system controller's QSPI sits
>> between the control and data registers. Split the "data" reg into two
>> parts: "data" & "control".
>>
>> Fixes: 213556235526 ("dt-bindings: soc/microchip: update syscontroller compatibles")
> 
> I omitted the second fixes tag:
> Fixes: ed9543d6f2c4 ("dt-bindings: add bindings for polarfire soc mailbox")
> 
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>>   .../bindings/mailbox/microchip,mpfs-mailbox.yaml  | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 0/3] MPFS mailbox fixes

[Conor.Dooley]

On 05/08/2022 13:56, Conor Dooley wrote:
> Hey all,
> 
> I spotted a couple of bugs in my mailbox driver while developing some
> new features. None of the features these bugs relate to were in use so
> they've gone unnoticed until now. The binding screwup is unfortunate
> and I don't really know how I misread the register map so badly.
> 
> Jassi:
> Not sure if you prefer developers to add a CC: stable or not to patches
> so I have left them out, but I would like to see them backported.

Hey Jassi,
Have you just not had a chance to look at this yet, or are you waiting
for me to resend with the extra fixes tag applied?
Thanks,
Conor.

> 
> Thanks,
> Conor.
> 
> Conor Dooley (3):
>   dt-bindings: mailbox: fix the mpfs' reg property
>   mailbox: mpfs: fix handling of the reg property
>   mailbox: mpfs: account for mbox offsets while sending
> 
>  .../mailbox/microchip,mpfs-mailbox.yaml       | 15 ++++++++---
>  drivers/mailbox/mailbox-mpfs.c                | 25 +++++++++++--------
>  2 files changed, 25 insertions(+), 15 deletions(-)
> 

Re: [PATCH 0/3] MPFS mailbox fixes

[Jassi Brar]

On Tue, Aug 23, 2022 at 1:42 PM <Conor.Dooley@microchip.com> wrote:
>
> On 05/08/2022 13:56, Conor Dooley wrote:
> > Hey all,
> >
> > I spotted a couple of bugs in my mailbox driver while developing some
> > new features. None of the features these bugs relate to were in use so
> > they've gone unnoticed until now. The binding screwup is unfortunate
> > and I don't really know how I misread the register map so badly.
> >
> > Jassi:
> > Not sure if you prefer developers to add a CC: stable or not to patches
> > so I have left them out, but I would like to see them backported.
>
> Hey Jassi,
> Have you just not had a chance to look at this yet, or are you waiting
> for me to resend with the extra fixes tag applied?
>
Please resend with fixes tags.

thanks

Re: [PATCH 0/3] MPFS mailbox fixes

[Conor.Dooley]

On 23/08/2022 19:44, Jassi Brar wrote:
> On Tue, Aug 23, 2022 at 1:42 PM <Conor.Dooley@microchip.com> wrote:
>>
>> On 05/08/2022 13:56, Conor Dooley wrote:
>>> Hey all,
>>>
>>> I spotted a couple of bugs in my mailbox driver while developing some
>>> new features. None of the features these bugs relate to were in use so
>>> they've gone unnoticed until now. The binding screwup is unfortunate
>>> and I don't really know how I misread the register map so badly.
>>>
>>> Jassi:
>>> Not sure if you prefer developers to add a CC: stable or not to patches
>>> so I have left them out, but I would like to see them backported.
>>
>> Hey Jassi,
>> Have you just not had a chance to look at this yet, or are you waiting
>> for me to resend with the extra fixes tag applied?
>>
> Please resend with fixes tags.

Cool, will do tomorrow.
Thanks for the prompt reply!
Conor.