Re: [PATCH 2/2] arm64: dts: exynos: Add initial support for Samsung Galaxy Watch FE

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Unaligned `no-map` simple-framebuffer region causes memory attribute aliasing.
> - [Medium] Invalid usage of `reserved-memory` for memory-mapped devices and regions outside system RAM.
> - [Low] Missing unit address in the `misc-rmem` device tree node.
> - [Low] Mathematical error in memory calculation in commit message and comments.
> --
>
> --- Patch [2]: [PATCH 2/2] arm64: dts: exynos: Add initial support for Samsung Galaxy Watch FE ---
> commit 6f30a8a96fda7068a12749f38925083c1838f92d
> Author: Aiden Isik <[email protected]>
>
> arm64: dts: exynos: Add initial support for Samsung Galaxy Watch FE
>
> This commit adds initial device tree support for the Samsung Galaxy Watch FE,
> including bootloader framebuffer and memory definitions.
>
> > Only the lowest 490MiB and highest 512MiB of memory are used for now.
> > The 488MiB between the addresses of 0x9ea00000 and 0xbe680000 is excluded
>
> [Severity: Low]
> This isn't a bug, but the math in the commit message seems slightly off.
>
> 0xbe680000 - 0x9ea00000 is 0x1fc80000, which is 508.5 MiB, not 488 MiB.

Will fix in v2.

>
> > diff --git a/arch/arm64/boot/dts/exynos/exynos5515-lucky7.dts b/arch/arm64/boot/dts/exynos/exynos5515-lucky7.dts
> > index 0000000000000..f91ea7e44cf86
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/exynos/exynos5515-lucky7.dts
> > @@ -0,0 +1,106 @@
> [ ... ]
> > +	memory@80000000 {
> > +		/*
> > +		 * Use the lowest 490MiB and highest 512MiB of memory.
> > +		 * The 488MiB between the addresses of 0x9ea00000 and
>
> [Severity: Low]
> This isn't a bug, but similarly to the commit message, the calculated memory
> size here is 508.5 MiB, not 488 MiB.

Will fix in v2.

>
> > +		 * 0xbe680000 is currently unused due to any memory accesses
> > +		 * in several ranges between those addresses causing the core
> > +		 * performing the accesses to stall.
> > +		 */
> > +		reg = <0x0 0x80000000 0x1ea00000>,
> [ ... ]
> > +	reserved-memory {
> > +		ranges;
> > +		#address-cells = <2>;
> > +		#size-cells = <1>;
> > +
> > +		framebuffer@9e0cf000 {
> > +			reg = <0x0 0x9e0cf000 (396 * 396 * 4)>;
> > +			no-map;
> > +		};
>
> [Severity: High]
> Does this unaligned size combined with no-map cause memory attribute aliasing
> on arm64?
>
> The region has an unaligned size of 0x99240 (627264 bytes). The no-map
> property drops this exact byte range from the kernel's linear map, leaving the
> remainder of the page (0x9e168240-0x9e169000) to be mapped as Normal Cacheable
> memory.
>
> When the simple-framebuffer driver calls ioremap_wc(), it maps the entire page
> at 0x9e168000 as Write-Combine (Normal Non-Cacheable), creating an attribute
> alias.

Possibly an issue? I haven't seen any other Exynos device trees do anything differently so I'll wait for a maintainer to weigh in.

>
> > +
> > +		misc-rmem {
> > +			/*
> > +			 * Some of these regions belong to memory-mapped
> > +			 * devices, and will be assigned to drivers as support
> > +			 * is added.
> > +			 *
> > +			 * Other regions belong to the TrustZone firmware
> > +			 * and should never be touched.
> > +			 */
> > +			reg = <0x0 0x87000000 0x200000>,
>
> [Severity: Medium]
> Is it valid to use reserved-memory for memory-mapped devices and regions
> outside system RAM?
>
> The Devicetree specification dictates that reserved-memory regions MUST be
> contained within a defined /memory node. The misc-rmem node includes
> addresses (e.g., 0xa0000000, 0xb0000000) that fall within the
> 0x9ea00000-0xc0000000 range explicitly omitted from the /memory node.
>
> Additionally, since these are MMIO regions, shouldn't they be described as
> device nodes on a bus rather than reserved RAM?
>

These addresses *are* part of RAM. It is just a temporarily disabled region.

> [Severity: Low]
> This isn't a bug, but this node is missing the unit address @87000000
> to match the first reg entry, which will generate a unit_address_vs_reg dtc
> compiler warning.
>

Will wait for a maintainer to weigh in on this one too, but I'll likely fix it in v2.

> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
>

Kind regards,
- Aiden Isik