Re: [PATCH v3 0/8] Hey Jassi, all,

[Valentina.FernandezAlanis]

On 07/03/2023 20:22, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> 
> Here are some fixes for the system controller on PolarFire SoC that I
> ran into while implementing support for using the system controller to
> re-program the FPGA. A few are just minor bits that I fixed in passing,
> but the bulk of the patchset is changes to how the mailbox figures out
> if a "service" has completed.
> 
> Prior to implementing this particular functionality, the services
> requested from the system controller, via its mailbox interface, always
> triggered an interrupt when the system controller was finished with
> the service.
> 
> Unfortunately some of the services used to validate the FPGA images
> before programming them do not trigger an interrupt if they fail.
> For example, the service that checks whether an FPGA image is actually
> a newer version than what is already programmed, does not trigger an
> interrupt, unless the image is actually newer than the one currently
> programmed. If it has an earlier version, no interrupt is triggered
> and a status is set in the system controller's status register to
> signify the reason for the failure.
> 
> In order to differentiate between the service succeeding & the system
> controller being inoperative or otherwise unable to function, I had to
> switch the controller to poll a busy bit in the system controller's
> registers to see if it has completed a service.
> This makes sense anyway, as the interrupt corresponds to "data ready"
> rather than "tx done", so I have changed the mailbox controller driver
> to do that & left the interrupt solely for signalling data ready.
> It just so happened that all of the services that I had worked with and
> tested up to this point were "infallible" & did not set a status, so the
> particular code paths were never tested.
> 
> Jassi, the mailbox and soc patches depend on each other, as the change
> in what the interrupt is used for requires changing the client driver's
> behaviour too, as mbox_send_message() will now return when the system
> controller is no longer busy rather than when the data is ready.
> I'm happy to send the lot via the soc tree with your Ack and/or reivew,
> if that also works you?
> I've got some other bits that I'd like to change in the client driver,
> so via the soc tree would suit me better.
> 
> Thanks,
> Conor.
Hi Conor,

I tested this on the Icicle Kit board, looks good to me. So:
Tested-by: Valentina Fernandez <valentina.fernandezalanis@xxxxxxxxxxxxx>
> 
> Changes in v3:
> - check the service status in the .tx_done() callback rather than
>    mpfs_mbox_rx_data()
> - re-order the if/else bits in mpfs_blocking_transaction() to please my
>    eyes a bit more
> - expand on the comment in same
> 
> Changes in v2:
> - up the timeout to 30 seconds, as required for services like image
>    validation, which may vary significantly in execution time
> - fixed a typo!
> 
> CC: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> CC: Daire McNamara <daire.mcnamara@xxxxxxxxxxxxx>
> CC: Jassi Brar <jassisinghbrar@xxxxxxxxx>
> CC: linux-riscv@xxxxxxxxxxxxxxxxxxx
> CC: linux-kernel@xxxxxxxxxxxxxxx
> 
> Conor Dooley (8):
>    mailbox: mpfs: fix an incorrect mask width
>    mailbox: mpfs: switch to txdone_poll
>    mailbox: mpfs: ditch a useless busy check
>    mailbox: mpfs: check the service status in .tx_done()
>    soc: microchip: mpfs: fix some horrible alignment
>    soc: microchip: mpfs: use a consistent completion timeout
>    soc: microchip: mpfs: simplify error handling in
>      mpfs_blocking_transaction()
>    soc: microchip: mpfs: handle timeouts and failed services differently
> 
>   drivers/mailbox/mailbox-mpfs.c              | 55 ++++++++++++---------
>   drivers/soc/microchip/mpfs-sys-controller.c | 52 +++++++++++++------
>   2 files changed, 67 insertions(+), 40 deletions(-)
>