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

[Arnd Bergmann]

On Wed, Oct 16, 2019 at 10:32 PM Daniel Palmer <daniel@xxxxxxxx> wrote:
>
> > > +
> > > +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.

Maybe keep the infinity_io_desc as an out-of-tree patch then? You can
simply do both, and ioremap() will return the hardcoded address.

> >    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 would not expect a missing spinlock to have an observable effect, the
question is more whether it's correct in all rare corner cases where
the barrier is interrupted and the interrupt handler uses another barrier.

I think it is, but I would recommend adding a comment to explain this if
you drop the spinlock. (or a comment about why this works with fiq if you
keep the lock).

     Arnd