Re: [PATCH v1 0/7] MPFS system controller/mailbox fixes

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4686 bytes --]

On Wed, Jan 11, 2023 at 01:45:06PM +0000, Conor Dooley wrote:
> Hey Jassi, all,
> 
> 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.

I think, with how things currently are, the timeout is insufficient.
I've noticed some services taking longer (significantly) longer than what
I have provisioned for.

I'll try to find an upper bound and respin a v2. Questions below are
still valid either way!

Thanks,
Conor.

> 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?
> 
> Secondly, I have a question about what to do if a service does fail, but
> not due to a timeout - eg the above example where the "new" image for
> the FPGA is actually older than the one that currently exists.
> Ideally, if a service fails due to something other than the transaction
> timing out, I would go and read the status registers to see what the
> cause of failure was.
> I could not find a function in the mailbox framework that allows the
> client to request that sort of information from the client. Trying to
> do something with the auxiliary bus, or exporting some function to a
> device specific header seemed like a circumvention of the mailbox
> framework.
> Do you think it would be a good idea to implement something like
> mbox_client_peek_status(struct mbox_chan *chan, void *data) to allow
> clients to request this type of information?
> 
> It'd certainly allow me to report the actual errors to the drivers
> implementing the service & make better decisions there about how to
> proceed.
> Perhaps I have missed some way of doing this kind of thing that (should
> have been) staring me in the face!
> 
> Thanks,
> Conor.
> 
> CC: Conor Dooley <conor.dooley@microchip.com>
> CC: Daire McNamara <daire.mcnamara@microchip.com>
> CC: Jassi Brar <jassisinghbrar@gmail.com>
> CC: linux-riscv@lists.infradead.org
> CC: linux-kernel@vger.kernel.org
> 
> Conor Dooley (7):
>   mailbox: mpfs: fix an incorrect mask width
>   mailbox: mpfs: switch to txdone_poll
>   mailbox: mpfs: ditch a useless busy check
>   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              | 25 +++++++----
>  drivers/soc/microchip/mpfs-sys-controller.c | 48 +++++++++++++--------
>  2 files changed, 47 insertions(+), 26 deletions(-)
> 
> 
> base-commit: 88603b6dc419445847923fcb7fe5080067a30f98
> -- 
> 2.39.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]