Re: [PATCH 2/4] ARM: mstar: Add machine for MStar infinity family SoCs

From: Daniel Palmer <daniel@0x0f.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/4] ARM: mstar: Add machine for MStar infinity family SoCs
Date: Thu, 17 Oct 2019 05:32:20 +0900	[thread overview]
Message-ID: <20191016203219.GA5191@shiro> (raw)
In-Reply-To: <CAK8P3a2U7U31eF_POU2=eCU+E1DH-wnR2uHr-VZYWLy25hLjKg@mail.gmail.com>

> > +
> > +static void __init infinity_map_io(void)
> > +{
> > +       iotable_init(infinity_io_desc, ARRAY_SIZE(infinity_io_desc));
> > +       miu_flush = (void __iomem *)(infinity_io_desc[0].virtual
> > +                       + INFINITY_L3BRIDGE_FLUSH);
> > +       miu_status = (void __iomem *)(infinity_io_desc[0].virtual
> > +                       + INFINITY_L3BRIDGE_STATUS);
> > +}
> 
> If you do this a little later in .init_machine, you can use a simple ioremap()
> rather than picking a hardcoded physical address. It looks like nothing
> uses the mapping before you set soc_mb anyway.

I've moved this into infinity_barriers_init() using ioremap() as suggested.
I'd like to keep the fixed remap address for now as there are some
drivers in the vendor code that might be useful until rewrites are done but 
are littered with hard coded addresses.

> > +static DEFINE_SPINLOCK(infinity_mb_lock);
> > +
> > +static void infinity_mb(void)
> > +{
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&infinity_mb_lock, flags);
> > +       /* toggle the flush miu pipe fire bit */
> > +       writel_relaxed(0, miu_flush);
> > +       writel_relaxed(INFINITY_L3BRIDGE_FLUSH_TRIGGER, miu_flush);
> > +       while (!(readl_relaxed(miu_status) & INFINITY_L3BRIDGE_STATUS_DONE)) {
> > +               /* wait for flush to complete */
> > +       }
> > +       spin_unlock_irqrestore(&infinity_mb_lock, flags);
> > +}
> 
> Wow, this is a heavy barrier. From your description it doesn't sound like
> there is anything to be done about it unfortunately.

It's possible there is a better way once I can find out what the L3 bridge
actually is. There is a small amount of documentation for the miu (DDR
controller) that says it has an 8 or 4 operation configurable pipeline but
this flushing bit is in a totally different area that's only documented
by the comment about it in u-boot.

> Two possible issues I see here:
> 
> * It looks like it relies on CONFIG_ARM_HEAVY_BARRIER, but your Kconfig
>   entry does not select that. In many configurations, CACHE_L2X0 would
>   be set, but there is no need for yours on the Cortex-A7.

Fixed.

>    Not sure if it matters in practice, as almost nothing uses fiq any more.
>    OTOH, maybe the lock is not needed at all? AFAICT if the sequence
>    gets interrupted by a handler that also calls mb(), you would still
>    continue in the original thread while observing a full l3 barrier. ;-)

I've taken the lock out and tested that the ethernet isn't sending garbage
and everything looks good.

I'm still hoping for some feedback on the other parts of the series.
I'll post a v2 series in a few days.

Thanks!

Daniel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel