Hi Torsten,
On 2026-09-25T10:54:32, Torsten Duwe <[email protected]> wrote:
> mfd: add support for RPi5 RP1 chip MFD
>
> MFD base driver for the RP1 chip, similar to Linux'
> drivers/misc/rp1/rp1_pci.c
>
> Signed-off-by: Torsten Duwe <[email protected]>
> Co-authored-by: Oleksii Moisieiev <[email protected]>
>
> configs/rpi_arm64_defconfig | 1 +
> drivers/mfd/Kconfig | 10 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/rp1.c | 127 ++++++++++++++++++++++
> include/dt-bindings/mfd/rp1.h | 239
> ++++++++++++++++++++++++++++++++++++++++++
> include/pci_ids.h | 3 +
> 6 files changed, 381 insertions(+)
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> @@ -2,3 +2,13 @@ config MFD_ATMEL_SMC
> +config MFD_RP1
> + tristate "RP1 MFD driver"
> + depends on PCI
> + help
> + Support for the RP1 peripheral chip.
> +
> + This driver provides support for the Raspberry Pi RP1 peripheral chip.
> + It is responsible for enabling the Device Tree node once the PCIe
> endpoint
> + has been configureds.
Please use 'bool' rather than 'tristate' - U-Boot does not build
loadable modules, and the surrounding entries are all bool. Also
'configureds' should be 'configured'.
> diff --git a/drivers/mfd/rp1.c b/drivers/mfd/rp1.c
> @@ -0,0 +1,127 @@
> +#define RP1_B0_CHIP_ID 0x10001927
> +#define RP1_C0_CHIP_ID 0x20001927
> +
> +#define RP1_PLATFORM_ASIC BIT(1)
> +#define RP1_PLATFORM_FPGA BIT(0)
> +
> +#define RP1_DRIVER_NAME "rp1"
> +
> +#define PCI_DEVICE_REV_RP1_C0 2
RP1_B0_CHIP_ID, RP1_PLATFORM_ASIC and PCI_DEVICE_REV_RP1_C0 are all
unused - please drop them, or actually use them (e.g. accept the B0
variant, or check the revision).
> diff --git a/drivers/mfd/rp1.c b/drivers/mfd/rp1.c
> @@ -0,0 +1,127 @@
> +static u32 rp1_reg_read(struct rp1_dev *rp1, unsigned int base_addr, u32
> offset)
> +{
> + resource_size_t phys = rp1_io_to_phys(rp1, base_addr);
> + void __iomem *regblock = ioremap(phys, 0x1000);
> + u32 value = readl(regblock + offset);
> +
> + iounmap(regblock);
> + return value;
> +}
Please map the BAR once in probe (store the virtual address in struct
rp1_dev) and just do readl() here. ioremap()/iounmap() around every
register access is unnecessary, and there is no check that ioremap()
succeeded before the readl().
> diff --git a/drivers/mfd/rp1.c b/drivers/mfd/rp1.c
> @@ -0,0 +1,127 @@
> +static int rp1_get_bar_region(struct udevice *dev, phys_addr_t *bar_start)
> +{
> + *bar_start = (phys_addr_t)dm_pci_map_bar(dev, PCI_BASE_ADDRESS_1, 0, 0,
> + PCI_REGION_TYPE, PCI_REGION_MEM);
> + return 0;
> +}
This wrapper adds nothing and always returns 0 without checking
whether dm_pci_map_bar() succeeded. Either fold it into rp1_probe()
and check the result, or make it return an actual error when the map
fails.
> diff --git a/drivers/mfd/rp1.c b/drivers/mfd/rp1.c
> @@ -0,0 +1,127 @@
> + /*
> + * HACK: Set bar address in the correct order.
> + * Required for correct U-Boot operation.
> + */
> + dm_pci_write_config32(dev, PCI_BASE_ADDRESS_1, 0);
> + dm_pci_write_config32(dev, PCI_BASE_ADDRESS_0, 0x40000);
A driver shouldn't need to poke BARs by hand - the PCI core is meant
to assign them. Please explain in the comment (and in the commit
message) why auto-assignment does the wrong thing here, and whether
the underlying issue in the PCI core / RC driver can be fixed instead
of open-coding it. 'HACK' with no reference to the root cause makes it
hard for something to understand / fix this later.
> diff --git a/drivers/mfd/rp1.c b/drivers/mfd/rp1.c
> @@ -0,0 +1,127 @@
> +static int rp1_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,
> + uint offset, ulong *valuep, enum pci_size_t
> size)
> +{
> + /*
> + * Leaving this call because pci subsystem calls for read_config
> + * and produces error then this callback is not set.
> + * Just return 0 here.s
> + */
> + *valuep = 0;
> + return 0;
> +}
A leaf UCLASS_PCI_GENERIC device shouldn't need dm_pci_ops at all -
those are for PCI controllers. If the core is complaining, that looks
like a real bug in the PCI uclass rather than something to paper over
with a stub that silently returns 0 for every config read. Please
investigate and fix the root cause. Also: typo 'here.s', and 'then'
should be 'when'.
> diff --git a/drivers/mfd/rp1.c b/drivers/mfd/rp1.c
> @@ -0,0 +1,127 @@
> +static int rp1_bind(struct udevice *dev)
> +{
> + device_set_name(dev, RP1_DRIVER_NAME);
> + return 0;
> +}
Is renaming the device really needed? The bound name normally comes
from the driver / DT node, and having every RP1 instance use the fixed
string 'rp1' will collide if more than one is ever probed. If this is
only cosmetic, please drop the bind hook.
> diff --git a/include/dt-bindings/mfd/rp1.h b/include/dt-bindings/mfd/rp1.h
> @@ -0,0 +1,239 @@
> +/*
> + * This header provides constants for the PY MFD.
> + *
> + * Copyright (c) 2024 EPAM Systems
> + *
> + * Derived from linux rp1.h.
> + */
How about 'RP1 MFD' instead of 'PY MFD' ?
> diff --git a/include/pci_ids.h b/include/pci_ids.h
> @@ -2555,6 +2555,9 @@
> +#define PCI_VENDOR_ID_RPI 0x1de4
> +#define PCI_DEVICE_ID_RPI_RP1_C0 0x0001
The extra tab on PCI_DEVICE_ID_RPI_RP1_C0 misaligns it with the
surrounding block - please drop it.
Tested-by: Simon Glass <[email protected]> # rpi5
Regards,
Simon
