[PATCH 3/3] coresight etm4x: Add 32-bit read/write option to split 64-bit words

[Leo Yan]

On Wed, Mar 08, 2023 at 11:26:48AM +0000, Mike Leach wrote:

[...]

> > > > +static inline u64 etm4x_relaxed_read64(struct csdev_access *csa,
> > > > +unsigned int offset) {
> > > > +   if (csa->io_mem) {
> > > > +           if (csa->no_quad_mmio) {
> > > > +                   /* split 64-bit reads into 2 consecutive 32-bit reads */
> > > > +                   struct etm_quad_split container;
> > > > +
> > > > +                   container.lsw = etm4x_read32(csa, offset);
> > > > +                   container.msw = etm4x_read32(csa, offset +
> > > sizeof(u32));
> > > > +
> > > > +                   return *(u64 *) &container;
> > >
> > > To be honest, I am not familiar with this part, just want to remind two things.
> > >
> > > Firstly, here you could use hi_lo_readq_relaxed(), seems to me,
> > > hi_lo_readq_relaxed() does the same thing with above section and you don't
> > > need to define the structure etm_quad_split().
> > >
> > > Secondly, IIUC, a main problem with splitting 64-bit access into two 32-bit
> > > accesses is breaking atomicity.  If here have race condition between reading and
> > > writing 64-bit registers, we need to consider to use spinlock for register
> > > accessing.
> 
> The drivers ensure that:-
> a) the ETM is only accessed by the CPU it is associated with.
> b) there are appropriate locks in place while the ETM is programmed up.
> 
> Moreover, the 64 bit registers are programmed only when the device is
> disabled - as required by the spec, so there cannot be a case of using
> "half" an updated value as all the required registers are programmed
> before the device is finally enabled to begin tracing

Thanks a lot for confirmation, Al and Mike.  So the second issue is
not valid anymore and please ignore it.

Thanks,
Leo

> Regards
> 
> Mike
> 
> 
> >
> > That shouldn't be an issue. 32-bit memory-mapped interface is
> > normal for ETMs. The peripheral bus interface to ETMs (debug APB)
> > is maximum 32-bit. The ETM architecture defines that registers are
> > only single-copy atomic up to 32-bit. This is generally true of
> > CoreSight programming registers.
> >
> > What typically happens to a 64-bit read/write from a CPU is that it
> > is split into two 32-bit accesses by a downsizing bridge on the path
> > from the main system interconnect to the debug APB (see 4.3.5
> > in the ETM architecture spec). In this case, it sounds like there is no
> > downsizing bridge. But because there should be no existing code
> > relying on 64-bit atomic access to ETM registers, it should be safe
> > to split the accesses in software without worrying about atomicity.
> >
> > Overall the ETM configuration involves complicated interactions
> > between multiple registers, so if you've got one CPU reading out the
> > configuration while another CPU is writing it, you've likely got much
> > bigger problems than 64-bit atomicity.
> >
> > Al
> >
> >
> > >
> > > I'd like to leave the second issue to Suzuki/Mike/James for comfirmation in case
> > > I introduced complexity.
> > >
> > > > +           } else
> > > > +                   return readq_relaxed(csa->base + offset);
> > > > +   } else
> > > > +           return read_etm4x_sysreg_offset(offset, true);
> > >
> > > Here need to add brackets:
> > >
> > >         } else {
> > >                 return read_etm4x_sysreg_offset(offset, true);
> > >         }
> > >
> > > > +}
> > > > +
> > > > +static inline void etm4x_relaxed_write64(struct csdev_access *csa,
> > > > +u64 quad, unsigned int offset) {
> > > > +   if (csa->io_mem) {
> > > > +           /* split 64-bit writes into 2 consecutive 32-bit writes */
> > > > +           if (csa->no_quad_mmio) {
> > > > +                   struct etm_quad_split container;
> > > > +
> > > > +                   *(u64 *) &container = quad;
> > > > +
> > > > +                   etm4x_relaxed_write32(csa, container.lsw, offset);
> > > > +                   etm4x_relaxed_write32(csa, container.msw, offset +
> > > sizeof(u32));
> > > > +           } else
> > > > +                   writeq_relaxed(quad, csa->base + offset);
> > > > +   } else
> > > > +           write_etm4x_sysreg_offset(quad, offset, true);          \
> > >
> > > Ditto.  Please drop the character '\' at the end of the line.
> > >
> > > Thanks,
> > > Leo
> > >
> > > P.s. I have a ADLink AVA platform (Ampere Altra SoC with 32 CPUs), I am glad to
> > > give a test if you can confirm this patch set can apply on it (and please clarify if
> > > there have any prerequisite for firmwares).
> > >
> > > > +}
> > > > +
> > > >  static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata)  {
> > > >     return drvdata->arch >= ETM_ARCH_ETE;
> > > > --
> > > > 2.25.1
> > > >
> > > _______________________________________________
> > > CoreSight mailing list -- coresight at lists.linaro.org To unsubscribe send an email
> > > to coresight-leave at lists.linaro.org
> 
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK