Re: [PATCH 06/12] dt-bindings: serial: snps-dw-apb-uart: Add RV1106 compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Invalid IOMUX register offsets for GPIO2 C and D pins cause memory corruption of GPIO1 A IOMUX settings.
- [Medium] The GRF MMC phase clock driver bypasses the syscon regmap, performing un-synchronized direct MMIO writes to the GRF.
--

commit 9eb9c18d0c1a4005a3ce42ce1798290561f0777d
Author: Simon Glass <sjg@chromium.org>

dt-bindings: serial: snps-dw-apb-uart: Add RV1106 compatible
    
Add the compatible for the UARTs of the Rockchip RV1106, which are
compatible with the Synopsys DesignWare APB UART.

> diff --git a/drivers/clk/rockchip/clk-rv1106.c b/drivers/clk/rockchip/clk-rv1106.c
> new file mode 100644
> index 0000000000000..88f043f34c233
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-rv1106.c
[ ... ]
> +static struct rockchip_clk_branch rv1106_grf_clk_branches[] __initdata = {
> +	MMC(SCLK_EMMC_DRV, "emmc_drv", "cclk_src_emmc", RV1106_EMMC_CON0, 1),
> +	MMC(SCLK_EMMC_SAMPLE, "emmc_sample", "cclk_src_emmc", RV1106_EMMC_CON1, 1),

[Severity: Medium]
Could this cause a regression by bypassing the syscon regmap and using direct
MMIO writes instead?

Registering phase clocks using the MMC macro rather than MMC_GRF passes a raw
MMIO pointer and a NULL regmap. While hardware-level data corruption might be
avoided due to the Rockchip GRF utilizing HIWORD_UPDATE bit masks, this
bypasses the syscon locking primitives and creates duplicate I/O mappings.

[ ... ]
> +static void __init rv1106_grf_clk_init(struct device_node *np)
> +{
> +	struct rockchip_clk_provider *ctx;
> +	struct device_node *parent_np;
> +	unsigned long clk_nr;
> +	void __iomem *reg_base;
> +
> +	clk_nr = rockchip_clk_find_max_clk_id(rv1106_grf_clk_branches,
> +					      ARRAY_SIZE(rv1106_grf_clk_branches)) + 1;
> +	parent_np = of_get_parent(np);
> +	reg_base = of_iomap(parent_np, 0);
> +	of_node_put(parent_np);

[Severity: Medium]
Is it safe to directly map the shared parent GRF syscon device here?

Mapping with of_iomap() circumvents the syscon regmap API instead of retrieving
its regmap via syscon_node_to_regmap(). This skips the expected synchronization
and creates duplicate I/O mappings.

> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index 7e0fcd45fd265..f9cbcb955853c 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
[ ... ]
> +static struct rockchip_pin_bank rv1106_pin_banks[] = {
> +	PIN_BANK_IOMUX_FLAGS(0, 32, "gpio0",
> +			     IOMUX_WIDTH_4BIT | IOMUX_SOURCE_PMU,
> +			     IOMUX_WIDTH_4BIT | IOMUX_SOURCE_PMU,
> +			     IOMUX_WIDTH_4BIT | IOMUX_SOURCE_PMU,
> +			     IOMUX_WIDTH_4BIT | IOMUX_SOURCE_PMU),
> +	PIN_BANK_IOMUX_FLAGS_OFFSET(1, 32, "gpio1",
> +				    IOMUX_WIDTH_4BIT,
> +				    IOMUX_WIDTH_4BIT,
> +				    IOMUX_WIDTH_4BIT,
> +				    IOMUX_WIDTH_4BIT,
> +				    0, 0x08, 0x10, 0x18),
> +	PIN_BANK_IOMUX_FLAGS_OFFSET(2, 32, "gpio2",
> +				    IOMUX_WIDTH_4BIT,
> +				    IOMUX_WIDTH_4BIT,
> +				    IOMUX_WIDTH_4BIT,
> +				    IOMUX_WIDTH_4BIT,
> +				    0x10020, 0x10028, 0, 0),

[Severity: High]
Does explicitly setting 0 as the offset for parts C and D cause memory
corruption of GPIO1 A IOMUX settings?

The gpio2 bank has 32 pins, but provides 0 as the explicit IOMUX offset for
parts C and D. During probe, this code evaluates iom->offset >= 0 (where 0 >= 0
is true):

drivers/pinctrl/pinctrl-rockchip.c:rockchip_pinctrl_get_soc_data() {
    ...
			/* preset iomux offset value, set new start value */
			if (iom->offset >= 0) {
    ...
}

This retains 0 as the offset for these pins rather than calculating them
sequentially. Since offset 0 of the IOC regmap is physically the IOMUX register
for GPIO1 A, any device tree configuration or runtime mux change for a GPIO2 C
or D pin will silently write to offset 0, corrupting the hardware IOMUX state
of GPIO1 A and B.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706195818.3906949-1-sjg@chromium.org?part=6