Re: [PATCH linux dev-5.3 2/2] fsi: aspeed: Add trace when error occurs

[Andrew Jeffery]

On Mon, 14 Oct 2019, at 23:51, Joel Stanley wrote:
> This prints out three registers in the FSI master when an error occurs.
> 
> Signed-off-by: Joel Stanley <joel at jms.id.au>
> ---
>  drivers/fsi/fsi-master-aspeed.c          |  7 +++++++
>  include/trace/events/fsi_master_aspeed.h | 18 ++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/fsi/fsi-master-aspeed.c 
> b/drivers/fsi/fsi-master-aspeed.c
> index d796c4012875..66657b7848de 100644
> --- a/drivers/fsi/fsi-master-aspeed.c
> +++ b/drivers/fsi/fsi-master-aspeed.c
> @@ -251,6 +251,13 @@ static int opb_read(void __iomem *base, uint32_t 
> addr, size_t size, u32 *out)
>  static int check_errors(struct fsi_master_aspeed *aspeed, int err)
>  {
>  	int ret;
> +	u32 mresp0, mstap0, mesrb0;
> +
> +	opb_read(aspeed->base, ctrl_base + FSI_MRESP0, 4, &mresp0);
> +	opb_read(aspeed->base, ctrl_base + FSI_MSTAP0, 4, &mstap0);
> +	opb_read(aspeed->base, ctrl_base + FSI_MESRB0, 4, &mesrb0);

check_errors() is done unconditionally in the read and write paths, and these
reads are unconditionally done on calls to check_error(). Really we only need
them if an error has occurred, so I think we should do:

if (trace_fsi_master_aspeed_opb_error_enabled()) {
    u32 mresp0, mstap0, mesrb0;
    opb_read(aspeed->base, ctrl_base + FSI_MRESP0, 4, &mresp0);
    ...
    trace_fsi_master_aspeed_opb_error(mresp0, mstap0, mesrb0);
}