Hello,
On Sun, Apr 14, 2024 at 01:58:04PM +0200, Stefan Wahren wrote:
> 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.
I don't think it will help much, but it shouldn't hurt.
--
Regards,
Laurent Pinchart
