Re: [PATCH 1/1] arm64: dts: imx8mp: remove arm, primecell-periphid at etm nodes

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Robin,

Am Freitag, 7. Juli 2023, 10:50:31 CEST schrieb Robin Murphy:
> On 2023-07-07 06:34, Alexander Stein wrote:
> > Hi Frank,
> > 
> > Am Donnerstag, 6. Juli 2023, 16:39:07 CEST schrieb Frank Li:
> >> On Thu, Jul 06, 2023 at 12:06:19PM +0100, Robin Murphy wrote:
> >>>>> Am Mittwoch, 5. Juli 2023, 22:59:53 CEST schrieb Frank Li:
> >>>>>> The reg size of etm nodes is incorrectly set to 64k instead of 4k.
> >>>>>> This
> >>>>>> leads to a crash when calling amba_read_periphid().  After corrected
> >>>>>> reg
> >>>>>> size, amba_read_periphid() retrieve the correct periphid.
> >>>>>> arm,primecell-periphid were removed from the etm nodes.
> >>>>> 
> >>>>> So this means the reference manual is wrong here? It clearly states
> >>>>> the size is 64kiB. Reference Manual i.MX8MP Rev 1. 06/2021
> >>>>> On a side note: Is imx8mq affected by this as well? The DAP memory
> >>>>> table lists similar sizes in the RM .
> >>>> 
> >>>> Note that the 64K MMIO space per device is really an alignment thing.
> >>>> It's a recommendation from ARM to allow individual device MMIO regions
> >>>> to be mapped on kernels with 64K page size. Most of the time the real
> >>>> MMIO space occupied by the device is actually much smaller than 64K.
> >>> 
> >>> Indeed, it's quite common for TRM memory maps to be written in terms of
> >>> the
> >>> interconnect configuration, i.e. from the point of view of the
> >>> interconnect
> >>> itself, that whole range of address space is assigned to that
> >>> peripheral,
> >>> and it may even be true that the entire range is routed to the port
> >>> where
> >>> that peripheral is connected. However what's of more interest for DT is
> >>> how
> >>> much of that range the peripheral itself actually decodes.
> >> 
> >> Yes, there are not problem by mapping bigger space in most case.
> >> 
> >> amba bus's periphal use close to end of region to show device's identical
> >> information.
> > 
> > Ah, thanks for the explanation. This make things more clear.
> > But on the other is it sensible to assume the memory resource size to fit
> > the IP address space? It appears to me the size is fixed to 4kiB anyway.
> > Would it make more sense to read the values from the address "base + 4K -
> > x" instead of "base + size - x"?
> 
> The size of PrimeCell components in general isn't necessarily 4KB
> though, and the ID registers were defined relative to the *end* of the
> register space. The old PrimeCell standards evolved into the CoreSight
> spec, and from the oldest version of that I can easily link to[1]:
> 
> "Each component occupies one or more contiguous 4KB blocks of address
> space. Where a component occupies more than one 4KB block, these
> registers must appear in the highest 4KB block."
> 
> (FWIW the latest Coresight 3.0 spec relaxes this restriction, but we
> tend to model newer stuff as platform drivers with explicit DT/ACPI
> identifiers rather than amba drivers anyway)

Ah, I wasn't aware the register space for PrimeCells/CoreSight could be larger 
than 4k. So the exact size must be known and used in DT. Thanks for 
explanation.

Best regards,
Alexander

> Thanks,
> Robin.
> 
> [1] https://developer.arm.com/documentation/ihi0029/d/?lang=en
> 
> > Best regards,
> > Alexander
> > 
> >> In drivers/amba/bus.c,
> >> 
> >> amba_read_periphid()
> >> {
> >> 
> >> 	...
> >> 	size = resource_size(&dev->res);
> >> 	...
> >> 	for (pid = 0, i = 0; i < 4; i++)
> >> 	
> >> 		pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i *
> > 
> > 8);
> > 
> >> }
> >> 
> >> So the range in DTS for arm,primecell should be actual IP address space.
> >> 
> >>> Robin.
> >>> 
> >>>> _______________________________________________
> >>>> linux-arm-kernel mailing list
> >>>> linux-arm-kernel@lists.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/