Re: [PATCH v5 07/11] staging: vc04_services: Move global memory mapped pointer

[Stefan Wahren <wahrenst@xxxxxxx>]

Hi Umang,

Am 12.04.24 um 09:57 schrieb Umang Jain:
> g_regs stores the remapped memory pointer for the vchiq platform.
> It can be moved to struct vchiq_drv_mgmt instead of being global.
>
> Adjust the affected functions accordingly. Pass vchiq_state pointer
> wherever necessary to access struct vchiq_drv_mgmt.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> ---
>   .../interface/vchiq_arm/vchiq_arm.c           | 19 +++++++++++--------
>   .../interface/vchiq_arm/vchiq_arm.h           |  2 ++
>   .../interface/vchiq_arm/vchiq_core.c          | 10 +++++-----
>   .../interface/vchiq_arm/vchiq_core.h          |  2 +-
>   4 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 29f9affe5304..9fc98411a2b8 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -129,8 +129,6 @@ struct vchiq_pagelist_info {
>   	unsigned int scatterlist_mapped;
>   };
>
> -static void __iomem *g_regs;
> -
>   static int
>   vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
>   			     unsigned int size, enum vchiq_bulk_dir dir);
> @@ -139,11 +137,14 @@ static irqreturn_t
>   vchiq_doorbell_irq(int irq, void *dev_id)
>   {
>   	struct vchiq_state *state = dev_id;
> +	struct vchiq_drv_mgmt *mgmt;
>   	irqreturn_t ret = IRQ_NONE;
>   	unsigned int status;
>
> +	mgmt = dev_get_drvdata(state->dev);
Was there a reason against moving it in the declaration?
> +
>   	/* Read (and clear) the doorbell */
> -	status = readl(g_regs + BELL0);
> +	status = readl(mgmt->regs + BELL0);
>
>   	if (status & ARM_DS_ACTIVE) {  /* Was the doorbell rung? */
>   		remote_event_pollall(state);
> @@ -556,9 +557,9 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>   	if (err)
>   		return err;
>
> -	g_regs = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(g_regs))
> -		return PTR_ERR(g_regs);
> +	drv_mgmt->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(drv_mgmt->regs))
> +		return PTR_ERR(drv_mgmt->regs);
>
>   	irq = platform_get_irq(pdev, 0);
>   	if (irq <= 0)
> @@ -641,8 +642,10 @@ static struct vchiq_arm_state *vchiq_platform_get_arm_state(struct vchiq_state *
>   }
>
>   void
> -remote_event_signal(struct remote_event *event)
> +remote_event_signal(struct vchiq_state *state, struct remote_event *event)
>   {
> +	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(state->dev);
> +
>   	/*
>   	 * Ensure that all writes to shared data structures have completed
>   	 * before signalling the peer.
> @@ -654,7 +657,7 @@ remote_event_signal(struct remote_event *event)
>   	dsb(sy);         /* data barrier operation */
>
>   	if (event->armed)
> -		writel(0, g_regs + BELL2); /* trigger vc interrupt */
> +		writel(0, mgmt->regs + BELL2); /* trigger vc interrupt */
>   }
>
>   int
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> index 52013bbc5171..10c1bdc50faf 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -50,6 +50,8 @@ struct vchiq_drv_mgmt {
>   	char *fragments_base;
>   	char *free_fragments;
>   	unsigned int fragments_size;
> +
> +	void __iomem *regs;
I'm not a expert about cache line alignment, but maybe this should be
moved at the beginning of struct vchiq_drv_mgmt?

Not sure this would have a performance impact.