Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

Hi Greg,

On 09:48 Tue 21 Jan     , Greg Kroah-Hartman wrote:

On Tue, Jan 21, 2025 at 09:43:37AM +0100, Andrea della Porta wrote:

quoted

Hi Greg,

On 12:47 Fri 17 Jan     , Greg Kroah-Hartman wrote:

quoted

On Mon, Jan 13, 2025 at 03:58:07PM +0100, Andrea della Porta wrote:

quoted

The RaspberryPi RP1 is a PCI multi function device containing
peripherals ranging from Ethernet to USB controller, I2C, SPI
and others.

Implement a bare minimum driver to operate the RP1, leveraging
actual OF based driver implementations for the on-board peripherals
by loading a devicetree overlay during driver probe.

The peripherals are accessed by mapping MMIO registers starting
from PCI BAR1 region.

With the overlay approach we can achieve more generic and agnostic
approach to managing this chipset, being that it is a PCI endpoint
and could possibly be reused in other hw implementations. The
presented approach is also used by Bootlin's Microchip LAN966x
patchset (see link) as well, for a similar chipset.

For reasons why this driver is contained in drivers/misc, please
check the links.

Links aren't always around all the time, please document it here why
this is needed, and then links can "add to" that summary.

Ack.

quoted

quoted

This driver is heavily based on downstream code from RaspberryPi
Foundation, and the original author is Phil Elwell.

Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf

...

quoted

quoted

diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
new file mode 100644
index 000000000000..3e8ba3fa7fd5
--- /dev/null
+++ b/drivers/misc/rp1/rp1_pci.c

@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018-24 Raspberry Pi Ltd.
+ * All rights reserved.
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+
+#include "rp1_pci.h"

Why does a self-contained .c file need a .h file?  Please put it all in
here.

I agree with you. Indeed, the very first version of this patch had the header
file placed inside the .c, but I received concerns about it and some advice to
do it differently, as you can see here:
https://lore.kernel.org/all/ZtWDpaqUG9d9yPPf@apocalypse/ (local)
so I've changed it accordingly in V2. So right now I'm not sure what the
acceptable behaviour should be ...

It's a pretty simple rule:
	Only use a .h file if multiple .c files need to see the symbol.

So no .h file is needed here.

Perfect, I'll revert back that two lines to V1 then. Please be aware
though that this will trigger the following checkpatch warning:

WARNING: externs should be avoided in .c files

quoted

quoted

quoted

+struct rp1_dev {
+	struct pci_dev *pdev;
+	struct irq_domain *domain;
+	struct irq_data *pcie_irqds[64];
+	void __iomem *bar1;
+	int ovcs_id;	/* overlay changeset id */
+	bool level_triggered_irq[RP1_INT_END];
+};
+
+static void msix_cfg_set(struct rp1_dev *rp1, unsigned int hwirq, u32 value)
+{
+	iowrite32(value, rp1->bar1 + RP1_PCIE_APBS_BASE + REG_SET + MSIX_CFG(hwirq));

Do your writes need a read to flush them properly?  Or can they handle
this automatically?

I had some thoughts with RaspberryPi foundation folks to double check it, and it
seems that there should be no need to readback the value (unless we want to go
really paranoid), so I would avoid that since in case of level handled interrupt
we would end up reading the register on every triggering interrupts.

Ok, if it passes testing, that's fine, hopefully it works properly, but
if not, you now have a trail to go and fix it in the future :)

Sure :)

Many thanks,
Andrea

thanks,

greg k-h