| Message ID | cover.1724159867.git.andrea.porta@suse.com |
|---|---|
| Headers | show |
| Series | Add support for RaspberryPi RP1 PCI device using a DT overlay | expand |
> +static unsigned int txdelay = 35; > +module_param(txdelay, uint, 0644); Networking does not like module parameters. This is also unused in this patch! So i suggest you just delete it. > + > /* This structure is only used for MACB on SiFive FU540 devices */ > struct sifive_fu540_macb_mgmt { > void __iomem *reg; > @@ -334,7 +337,7 @@ static int macb_mdio_wait_for_idle(struct macb *bp) > u32 val; > > return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_BIT(IDLE), > - 1, MACB_MDIO_TIMEOUT); > + 100, MACB_MDIO_TIMEOUT); > } Please take this patch out of the series, and break it up. This is one patch, with a good explanation why you need 1->100. > static int macb_mdio_read_c22(struct mii_bus *bus, int mii_id, int regnum) > @@ -493,6 +496,19 @@ static int macb_mdio_write_c45(struct mii_bus *bus, int mii_id, > return status; > } > > +static int macb_mdio_reset(struct mii_bus *bus) > +{ > + struct macb *bp = bus->priv; > + > + if (bp->phy_reset_gpio) { > + gpiod_set_value_cansleep(bp->phy_reset_gpio, 1); > + msleep(bp->phy_reset_ms); > + gpiod_set_value_cansleep(bp->phy_reset_gpio, 0); > + } > + > + return 0; > +} > + > static void macb_init_buffers(struct macb *bp) > { > struct macb_queue *queue; > @@ -969,6 +985,7 @@ static int macb_mii_init(struct macb *bp) > bp->mii_bus->write = &macb_mdio_write_c22; > bp->mii_bus->read_c45 = &macb_mdio_read_c45; > bp->mii_bus->write_c45 = &macb_mdio_write_c45; > + bp->mii_bus->reset = &macb_mdio_reset; This is one patch. > snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", > bp->pdev->name, bp->pdev->id); > bp->mii_bus->priv = bp; > @@ -1640,6 +1657,11 @@ static int macb_rx(struct macb_queue *queue, struct napi_struct *napi, > > macb_init_rx_ring(queue); > queue_writel(queue, RBQP, queue->rx_ring_dma); > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT > + if (bp->hw_dma_cap & HW_DMA_CAP_64B) > + macb_writel(bp, RBQPH, > + upper_32_bits(queue->rx_ring_dma)); > +#endif How does this affect a disto kernel? Do you actually need the #ifdef? What does bp->hw_dma_cap contain when CONFIG_ARCH_DMA_ADDR_T_64BIT is not defined? Again, this should be a patch of its own, with a good commit message. Interrupt coalescing should be a patch of its own, etc. Andrew --- pw-bot: cr
Hi Andrew, On 17:13 Tue 20 Aug , Andrew Lunn wrote: > > +static unsigned int txdelay = 35; > > +module_param(txdelay, uint, 0644); > > Networking does not like module parameters. > > This is also unused in this patch! So i suggest you just delete it. > > > + > > /* This structure is only used for MACB on SiFive FU540 devices */ > > struct sifive_fu540_macb_mgmt { > > void __iomem *reg; > > @@ -334,7 +337,7 @@ static int macb_mdio_wait_for_idle(struct macb *bp) > > u32 val; > > > > return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_BIT(IDLE), > > - 1, MACB_MDIO_TIMEOUT); > > + 100, MACB_MDIO_TIMEOUT); > > } > > Please take this patch out of the series, and break it up. This is one > patch, with a good explanation why you need 1->100. > > > static int macb_mdio_read_c22(struct mii_bus *bus, int mii_id, int regnum) > > @@ -493,6 +496,19 @@ static int macb_mdio_write_c45(struct mii_bus *bus, int mii_id, > > return status; > > } > > > > +static int macb_mdio_reset(struct mii_bus *bus) > > +{ > > + struct macb *bp = bus->priv; > > + > > + if (bp->phy_reset_gpio) { > > + gpiod_set_value_cansleep(bp->phy_reset_gpio, 1); > > + msleep(bp->phy_reset_ms); > > + gpiod_set_value_cansleep(bp->phy_reset_gpio, 0); > > + } > > + > > + return 0; > > +} > > + > > static void macb_init_buffers(struct macb *bp) > > { > > struct macb_queue *queue; > > @@ -969,6 +985,7 @@ static int macb_mii_init(struct macb *bp) > > bp->mii_bus->write = &macb_mdio_write_c22; > > bp->mii_bus->read_c45 = &macb_mdio_read_c45; > > bp->mii_bus->write_c45 = &macb_mdio_write_c45; > > + bp->mii_bus->reset = &macb_mdio_reset; > > This is one patch. > > > snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", > > bp->pdev->name, bp->pdev->id); > > bp->mii_bus->priv = bp; > > @@ -1640,6 +1657,11 @@ static int macb_rx(struct macb_queue *queue, struct napi_struct *napi, > > > > macb_init_rx_ring(queue); > > queue_writel(queue, RBQP, queue->rx_ring_dma); > > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT > > + if (bp->hw_dma_cap & HW_DMA_CAP_64B) > > + macb_writel(bp, RBQPH, > > + upper_32_bits(queue->rx_ring_dma)); > > +#endif > > How does this affect a disto kernel? Do you actually need the #ifdef? > What does bp->hw_dma_cap contain when CONFIG_ARCH_DMA_ADDR_T_64BIT is > not defined? > > Again, this should be a patch of its own, with a good commit message. > > Interrupt coalescing should be a patch of its own, etc. > > Andrew > Thanks for the feedback, I agree on all the observations. Please do note however that, as mentioned in the cover letter, this patch is not intended to be included upstream and is provided just as a quick way for anyone interested in testing the RP1 functionality using the Ethernet MAC. As such, this patch has not been polished nor splitted into manageable bits. Ii'm taknge note of your comments however and will come back to them in a future patch that deals specifically with macb. Many thanks, Andrea > --- > pw-bot: cr
On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > The RaspberryPi RP1 is ia 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-borad peripherals > by loading a devicetree overlay during driver probe. > The peripherals are accessed by mapping MMIO registers starting > from PCI BAR1 region. > As a minimum driver, the peripherals will not be added to the > dtbo here, but in following patches. > > Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > MAINTAINERS | 2 + > arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ Do not mix DTS with drivers. These MUST be separate. > drivers/misc/Kconfig | 1 + > drivers/misc/Makefile | 1 + > drivers/misc/rp1/Kconfig | 20 ++ > drivers/misc/rp1/Makefile | 3 + > drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ > drivers/misc/rp1/rp1-pci.dtso | 8 + > drivers/pci/quirks.c | 1 + > include/linux/pci_ids.h | 3 + > 10 files changed, 524 insertions(+) > create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso > create mode 100644 drivers/misc/rp1/Kconfig > create mode 100644 drivers/misc/rp1/Makefile > create mode 100644 drivers/misc/rp1/rp1-pci.c > create mode 100644 drivers/misc/rp1/rp1-pci.dtso > > diff --git a/MAINTAINERS b/MAINTAINERS > index 67f460c36ea1..1359538b76e8 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ > RASPBERRY PI RP1 PCI DRIVER > M: Andrea della Porta <andrea.porta@suse.com> > S: Maintained > +F: arch/arm64/boot/dts/broadcom/rp1.dtso > F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml > F: drivers/clk/clk-rp1.c > +F: drivers/misc/rp1/ > F: drivers/pinctrl/pinctrl-rp1.c > F: include/dt-bindings/clock/rp1.h > F: include/dt-bindings/misc/rp1.h > diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > new file mode 100644 > index 000000000000..d80178a278ee > --- /dev/null > +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > @@ -0,0 +1,152 @@ > +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > + > +#include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/interrupt-controller/irq.h> > +#include <dt-bindings/clock/rp1.h> > +#include <dt-bindings/misc/rp1.h> > + > +/dts-v1/; > +/plugin/; > + > +/ { > + fragment@0 { > + target-path=""; > + __overlay__ { > + #address-cells = <3>; > + #size-cells = <2>; > + > + rp1: rp1@0 { > + compatible = "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + interrupt-controller; > + interrupt-parent = <&rp1>; > + #interrupt-cells = <2>; > + > + // ranges and dma-ranges must be provided by the includer > + ranges = <0xc0 0x40000000 > + 0x01/*0x02000000*/ 0x00 0x00000000 > + 0x00 0x00400000>; Are you 100% sure you do not have here dtc W=1 warnings? > + > + dma-ranges = > + // inbound RP1 1x_xxxxxxxx -> PCIe 1x_xxxxxxxx > + <0x10 0x00000000 > + 0x43000000 0x10 0x00000000 > + 0x10 0x00000000>; > + > + clk_xosc: clk_xosc { Nope, switch to DTS coding style. > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-output-names = "xosc"; > + clock-frequency = <50000000>; > + }; > + > + macb_pclk: macb_pclk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-output-names = "pclk"; > + clock-frequency = <200000000>; > + }; > + > + macb_hclk: macb_hclk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-output-names = "hclk"; > + clock-frequency = <200000000>; > + }; > + > + rp1_clocks: clocks@c040018000 { Why do you mix MMIO with non-MMIO nodes? This really does not look correct. > + compatible = "raspberrypi,rp1-clocks"; > + #clock-cells = <1>; > + reg = <0xc0 0x40018000 0x0 0x10038>; Wrong order of properties - see DTS coding style. > + clocks = <&clk_xosc>; > + clock-names = "xosc"; Best regards, Krzysztof
On Tue, Aug 20, 2024 at 04:36:13PM +0200, Andrea della Porta wrote: > RaspberryPi RP1 is multi function PCI endpoint device that > exposes several subperipherals via PCI BAR. > Add an ethernet node for Cadence MACB to the RP1 dtso > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > arch/arm64/boot/dts/broadcom/rp1.dtso | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > index d80178a278ee..b40e203c28d5 100644 > --- a/arch/arm64/boot/dts/broadcom/rp1.dtso > +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > @@ -78,6 +78,29 @@ rp1_clocks: clocks@c040018000 { > <50000000>; // RP1_CLK_ETH_TSU > }; > > + rp1_eth: ethernet@c040100000 { > + reg = <0xc0 0x40100000 0x0 0x4000>; > + compatible = "cdns,macb"; Please start using DTS coding style... Best regards, Krzysztof
On Tue, Aug 20, 2024 at 04:36:09PM +0200, Andrea della Porta wrote: > The RP1 is an MFD supporting a gpio controller and /pinmux/pinctrl. > Add minimum support for the gpio only portion. The driver is in > pinctrl folder since upcoming patches will add the pinmux/pinctrl > support where the gpio part can be seen as an addition. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > MAINTAINERS | 1 + > drivers/pinctrl/Kconfig | 10 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-rp1.c | 719 ++++++++++++++++++++++++++++++++++ > 4 files changed, 731 insertions(+) > create mode 100644 drivers/pinctrl/pinctrl-rp1.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 4ce7b049d67e..67f460c36ea1 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19122,6 +19122,7 @@ S: Maintained > F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml > F: drivers/clk/clk-rp1.c > +F: drivers/pinctrl/pinctrl-rp1.c > F: include/dt-bindings/clock/rp1.h > F: include/dt-bindings/misc/rp1.h > > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 7e4f93a3bc7a..18bb1a8bd102 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -565,6 +565,16 @@ config PINCTRL_MLXBF3 > each pin. This driver can also be built as a module called > pinctrl-mlxbf3. > > +config PINCTRL_RP1 > + bool "Pinctrl driver for RP1" > + select PINMUX > + select PINCONF > + select GENERIC_PINCONF > + select GPIOLIB_IRQCHIP > + help > + Enable the gpio and pinctrl/mux driver for RaspberryPi RP1 > + multi function device. > + > source "drivers/pinctrl/actions/Kconfig" > source "drivers/pinctrl/aspeed/Kconfig" > source "drivers/pinctrl/bcm/Kconfig" > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > index cc809669405a..f1ca23b563f6 100644 > --- a/drivers/pinctrl/Makefile > +++ b/drivers/pinctrl/Makefile > @@ -45,6 +45,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o > obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o > obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o > obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o > +obj-$(CONFIG_PINCTRL_RP1) += pinctrl-rp1.o > obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o > obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o > obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o > diff --git a/drivers/pinctrl/pinctrl-rp1.c b/drivers/pinctrl/pinctrl-rp1.c > new file mode 100644 > index 000000000000..c035d2014505 > --- /dev/null > +++ b/drivers/pinctrl/pinctrl-rp1.c > @@ -0,0 +1,719 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for Raspberry Pi RP1 GPIO unit > + * > + * Copyright (C) 2023 Raspberry Pi Ltd. > + * > + * This driver is inspired by: > + * pinctrl-bcm2835.c, please see original file for copyright information > + */ > + > +#include <linux/bitmap.h> > +#include <linux/bitops.h> > +#include <linux/bug.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/gpio/driver.h> > +#include <linux/io.h> > +#include <linux/irq.h> > +#include <linux/irqdesc.h> > +#include <linux/init.h> > +#include <linux/of_address.h> > +#include <linux/of.h> > +#include <linux/of_irq.h> > +#include <linux/platform_device.h> > +#include <linux/seq_file.h> > +#include <linux/spinlock.h> > +#include <linux/types.h> Half of these headers are not used. Drop them. Best regards, Krzysztof
On Tue, Aug 20, 2024 at 04:36:11PM +0200, Andrea della Porta wrote: > Select the RP1 drivers needed to operate the PCI endpoint containing > several peripherals such as Ethernet and USB Controller. This chip is > present on RaspberryPi 5. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > arch/arm64/configs/defconfig | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index 7d32fca64996..e7615c464680 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -606,6 +606,7 @@ CONFIG_PINCTRL_QCM2290=y > CONFIG_PINCTRL_QCS404=y > CONFIG_PINCTRL_QDF2XXX=y > CONFIG_PINCTRL_QDU1000=y > +CONFIG_PINCTRL_RP1=y > CONFIG_PINCTRL_SA8775P=y > CONFIG_PINCTRL_SC7180=y > CONFIG_PINCTRL_SC7280=y > @@ -685,6 +686,7 @@ CONFIG_SENSORS_RASPBERRYPI_HWMON=m > CONFIG_SENSORS_SL28CPLD=m > CONFIG_SENSORS_INA2XX=m > CONFIG_SENSORS_INA3221=m > +CONFIG_MISC_RP1=y Module? > CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y > CONFIG_CPU_THERMAL=y > CONFIG_DEVFREQ_THERMAL=y > @@ -1259,6 +1261,7 @@ CONFIG_COMMON_CLK_CS2000_CP=y > CONFIG_COMMON_CLK_FSL_SAI=y > CONFIG_COMMON_CLK_S2MPS11=y > CONFIG_COMMON_CLK_PWM=y > +CONFIG_COMMON_CLK_RP1=y Module? > CONFIG_COMMON_CLK_RS9_PCIE=y > CONFIG_COMMON_CLK_VC3=y > CONFIG_COMMON_CLK_VC5=y > -- > 2.35.3 >
On Tue, Aug 20, 2024 at 04:36:12PM +0200, Andrea della Porta wrote: > RaspberryPi RP1 contains Cadence's MACB core. Implement the > changes to be able to operate the customization in the RP1. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > @@ -5100,6 +5214,11 @@ static int macb_probe(struct platform_device *pdev) > } > } > } > + > + device_property_read_u8(&pdev->dev, "cdns,aw2w-max-pipe", &bp->aw2w_max_pipe); > + device_property_read_u8(&pdev->dev, "cdns,ar2r-max-pipe", &bp->ar2r_max_pipe); Where are the bindings? > + bp->use_aw2b_fill = device_property_read_bool(&pdev->dev, "cdns,use-aw2b-fill"); > + > spin_lock_init(&bp->lock); > > /* setup capabilities */ > @@ -5155,6 +5274,21 @@ static int macb_probe(struct platform_device *pdev) > else > bp->phy_interface = interface; > > + /* optional PHY reset-related properties */ > + bp->phy_reset_gpio = devm_gpiod_get_optional(&pdev->dev, "phy-reset", Where is the binding? > + GPIOD_OUT_LOW); > + if (IS_ERR(bp->phy_reset_gpio)) { > + dev_err(&pdev->dev, "Failed to obtain phy-reset gpio\n"); > + err = PTR_ERR(bp->phy_reset_gpio); > + goto err_out_free_netdev; > + } > + > + bp->phy_reset_ms = 10; > + of_property_read_u32(np, "phy-reset-duration", &bp->phy_reset_ms); Where is the binding? > + /* A sane reset duration should not be longer than 1s */ > + if (bp->phy_reset_ms > 1000) > + bp->phy_reset_ms = 1000; > + > /* IP specific init */ > err = init(pdev); Best regards, Krzysztof
On Tue, Aug 20, 2024 at 04:36:08PM +0200, Andrea della Porta wrote: > RaspberryPi RP1 is an MFD providing, among other peripherals, several > clock generators and PLLs that drives the sub-peripherals. > Add the driver to support the clock providers. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> ... > diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c > new file mode 100644 > index 000000000000..d18e711c0623 > --- /dev/null > +++ b/drivers/clk/clk-rp1.c > @@ -0,0 +1,1655 @@ > +// SPDX-License-Identifier: GPL checkpatch says: WARNING: 'SPDX-License-Identifier: GPL' is not supported in LICENSES/... ... > +static int rp1_clock_set_parent(struct clk_hw *hw, u8 index) > +{ > + struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw); > + struct rp1_clockman *clockman = clock->clockman; > + const struct rp1_clock_data *data = clock->data; > + u32 ctrl, sel; > + > + spin_lock(&clockman->regs_lock); > + ctrl = clockman_read(clockman, data->ctrl_reg); > + > + if (index >= data->num_std_parents) { > + /* This is an aux source request */ > + if (index >= data->num_std_parents + data->num_aux_parents) It looks like &clockman->regs_lock needs to be unlocked here. Flagged by Smatch, Sparse. and Coccinelle. > + return -EINVAL; > + > + /* Select parent from aux list */ > + ctrl = set_register_field(ctrl, index - data->num_std_parents, > + CLK_CTRL_AUXSRC_MASK, > + CLK_CTRL_AUXSRC_SHIFT); > + /* Set src to aux list */ > + ctrl = set_register_field(ctrl, AUX_SEL, data->clk_src_mask, > + CLK_CTRL_SRC_SHIFT); > + } else { > + ctrl = set_register_field(ctrl, index, data->clk_src_mask, > + CLK_CTRL_SRC_SHIFT); > + } > + > + clockman_write(clockman, data->ctrl_reg, ctrl); > + spin_unlock(&clockman->regs_lock); > + > + sel = rp1_clock_get_parent(hw); > + WARN(sel != index, "(%s): Parent index req %u returned back %u\n", > + data->name, index, sel); > + > + return 0; > +} ...
On Tue, Aug 20, 2024 at 04:36:09PM +0200, Andrea della Porta wrote: > The RP1 is an MFD supporting a gpio controller and /pinmux/pinctrl. > Add minimum support for the gpio only portion. The driver is in > pinctrl folder since upcoming patches will add the pinmux/pinctrl > support where the gpio part can be seen as an addition. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> ... > diff --git a/drivers/pinctrl/pinctrl-rp1.c b/drivers/pinctrl/pinctrl-rp1.c ... > +const struct rp1_iobank_desc rp1_iobanks[RP1_NUM_BANKS] = { > + /* gpio inte ints rio pads */ > + { 0, 28, 0x0000, 0x011c, 0x0124, 0x0000, 0x0004 }, > + { 28, 6, 0x4000, 0x411c, 0x4124, 0x4000, 0x4004 }, > + { 34, 20, 0x8000, 0x811c, 0x8124, 0x8000, 0x8004 }, > +}; rp1_iobanks seems to only be used in this file. If so, it should be static. Flagged by Sparse. ...
On 20/08/2024 16:36, Andrea della Porta wrote: > RP1 is an MFD chipset that acts as a south-bridge PCIe endpoint sporting > a pletora of subdevices (i.e. Ethernet, USB host controller, I2C, PWM, > etc.) whose registers are all reachable starting from an offset from the > BAR address. The main point here is that while the RP1 as an endpoint > itself is discoverable via usual PCI enumeraiton, the devices it contains > are not discoverable and must be declared e.g. via the devicetree. > > This patchset is an attempt to provide a minimum infrastructure to allow > the RP1 chipset to be discovered and perpherals it contains to be added > from a devictree overlay loaded during RP1 PCI endpoint enumeration. > Followup patches should add support for the several peripherals contained > in RP1. > > This work is based upon dowstream drivers code and the proposal from RH > et al. (see [1] and [2]). A similar approach is also pursued in [3]. Looking briefly at findings it seems this was not really tested by automation and you expect reviewers to find issues which are pointed out by tools. That's not nice approach. Reviewer's time is limited, while tools do it for free. And the tools are free - you can use them without any effort. It does not look like you tested the DTS against bindings. Please run `make dtbs_check W=1` (see Documentation/devicetree/bindings/writing-schema.rst or https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ for instructions). Please run standard kernel tools for static analysis, like coccinelle, smatch and sparse, and fix reported warnings. Also please check for warnings when building with W=1. Most of these commands (checks or W=1 build) can build specific targets, like some directory, to narrow the scope to only your code. The code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Please run scripts/checkpatch.pl and fix reported warnings. Then please run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Best regards, Krzysztof
On 21/08/2024 10:38, Krzysztof Kozlowski wrote: > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: ... >> drivers/misc/Kconfig | 1 + >> drivers/misc/Makefile | 1 + >> drivers/misc/rp1/Kconfig | 20 ++ >> drivers/misc/rp1/Makefile | 3 + >> drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ >> drivers/misc/rp1/rp1-pci.dtso | 8 + >> drivers/pci/quirks.c | 1 + >> include/linux/pci_ids.h | 3 + >> 10 files changed, 524 insertions(+) >> create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso >> create mode 100644 drivers/misc/rp1/Kconfig >> create mode 100644 drivers/misc/rp1/Makefile >> create mode 100644 drivers/misc/rp1/rp1-pci.c >> create mode 100644 drivers/misc/rp1/rp1-pci.dtso >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 67f460c36ea1..1359538b76e8 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ >> RASPBERRY PI RP1 PCI DRIVER >> M: Andrea della Porta <andrea.porta@suse.com> >> S: Maintained >> +F: arch/arm64/boot/dts/broadcom/rp1.dtso >> F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml >> F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml >> F: drivers/clk/clk-rp1.c >> +F: drivers/misc/rp1/ >> F: drivers/pinctrl/pinctrl-rp1.c >> F: include/dt-bindings/clock/rp1.h >> F: include/dt-bindings/misc/rp1.h >> diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso >> new file mode 100644 >> index 000000000000..d80178a278ee >> --- /dev/null >> +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso >> @@ -0,0 +1,152 @@ >> +// SPDX-License-Identifier: (GPL-2.0 OR MIT) >> + >> +#include <dt-bindings/gpio/gpio.h> >> +#include <dt-bindings/interrupt-controller/irq.h> >> +#include <dt-bindings/clock/rp1.h> >> +#include <dt-bindings/misc/rp1.h> >> + >> +/dts-v1/; >> +/plugin/; >> + >> +/ { >> + fragment@0 { >> + target-path=""; >> + __overlay__ { >> + #address-cells = <3>; >> + #size-cells = <2>; >> + >> + rp1: rp1@0 { >> + compatible = "simple-bus"; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + interrupt-controller; >> + interrupt-parent = <&rp1>; >> + #interrupt-cells = <2>; >> + >> + // ranges and dma-ranges must be provided by the includer >> + ranges = <0xc0 0x40000000 >> + 0x01/*0x02000000*/ 0x00 0x00000000 >> + 0x00 0x00400000>; > > Are you 100% sure you do not have here dtc W=1 warnings? One more thing, I do not see this overlay applied to any target, which means it cannot be tested. You miss entry in Makefile. Best regards, Krzysztof
Hi Andrea, Am 20.08.24 um 16:36 schrieb Andrea della Porta: > The RaspberryPi RP1 is ia PCI multi function device containing > peripherals ranging from Ethernet to USB controller, I2C, SPI > and others. sorry, i cannot provide you a code review, but just some comments. multi function device suggests "mfd" subsystem or at least "soc" . I won't recommend misc driver here. > Implement a bare minimum driver to operate the RP1, leveraging > actual OF based driver implementations for the on-borad peripherals > by loading a devicetree overlay during driver probe. Can you please explain why this should be a DT overlay? The RP1 is assembled on the Raspberry Pi 5 PCB. DT overlays are typically for loose connections like displays or HATs. I think a DTSI just for the RP1 would fit better and is easier to read. > The peripherals are accessed by mapping MMIO registers starting > from PCI BAR1 region. > As a minimum driver, the peripherals will not be added to the > dtbo here, but in following patches. > > Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > MAINTAINERS | 2 + > arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ > drivers/misc/Kconfig | 1 + > drivers/misc/Makefile | 1 + > drivers/misc/rp1/Kconfig | 20 ++ > drivers/misc/rp1/Makefile | 3 + > drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ > drivers/misc/rp1/rp1-pci.dtso | 8 + > drivers/pci/quirks.c | 1 + > include/linux/pci_ids.h | 3 + > 10 files changed, 524 insertions(+) > create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso > create mode 100644 drivers/misc/rp1/Kconfig > create mode 100644 drivers/misc/rp1/Makefile > create mode 100644 drivers/misc/rp1/rp1-pci.c > create mode 100644 drivers/misc/rp1/rp1-pci.dtso > ... > + > + rp1_clocks: clocks@c040018000 { > + compatible = "raspberrypi,rp1-clocks"; > + #clock-cells = <1>; > + reg = <0xc0 0x40018000 0x0 0x10038>; > + clocks = <&clk_xosc>; > + clock-names = "xosc"; > + > + assigned-clocks = <&rp1_clocks RP1_PLL_SYS_CORE>, > + <&rp1_clocks RP1_PLL_AUDIO_CORE>, > + // RP1_PLL_VIDEO_CORE and dividers are now managed by VEC,DPI drivers > + <&rp1_clocks RP1_PLL_SYS>, > + <&rp1_clocks RP1_PLL_SYS_SEC>, > + <&rp1_clocks RP1_PLL_SYS_PRI_PH>, > + <&rp1_clocks RP1_CLK_ETH_TSU>; > + > + assigned-clock-rates = <1000000000>, // RP1_PLL_SYS_CORE > + <1536000000>, // RP1_PLL_AUDIO_CORE > + <200000000>, // RP1_PLL_SYS > + <125000000>, // RP1_PLL_SYS_SEC > + <100000000>, // RP1_PLL_SYS_PRI_PH > + <50000000>; // RP1_CLK_ETH_TSU > + }; > + > + rp1_gpio: pinctrl@c0400d0000 { > + reg = <0xc0 0x400d0000 0x0 0xc000>, > + <0xc0 0x400e0000 0x0 0xc000>, > + <0xc0 0x400f0000 0x0 0xc000>; > + compatible = "raspberrypi,rp1-gpio"; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <RP1_INT_IO_BANK0 IRQ_TYPE_LEVEL_HIGH>, > + <RP1_INT_IO_BANK1 IRQ_TYPE_LEVEL_HIGH>, > + <RP1_INT_IO_BANK2 IRQ_TYPE_LEVEL_HIGH>; > + gpio-line-names = > + "ID_SDA", // GPIO0 > + "ID_SCL", // GPIO1 > + "GPIO2", // GPIO2 > + "GPIO3", // GPIO3 > + "GPIO4", // GPIO4 > + "GPIO5", // GPIO5 > + "GPIO6", // GPIO6 > + "GPIO7", // GPIO7 > + "GPIO8", // GPIO8 > + "GPIO9", // GPIO9 > + "GPIO10", // GPIO10 > + "GPIO11", // GPIO11 > + "GPIO12", // GPIO12 > + "GPIO13", // GPIO13 > + "GPIO14", // GPIO14 > + "GPIO15", // GPIO15 > + "GPIO16", // GPIO16 > + "GPIO17", // GPIO17 > + "GPIO18", // GPIO18 > + "GPIO19", // GPIO19 > + "GPIO20", // GPIO20 > + "GPIO21", // GPIO21 > + "GPIO22", // GPIO22 > + "GPIO23", // GPIO23 > + "GPIO24", // GPIO24 > + "GPIO25", // GPIO25 > + "GPIO26", // GPIO26 > + "GPIO27", // GPIO27 > + "PCIE_RP1_WAKE", // GPIO28 > + "FAN_TACH", // GPIO29 > + "HOST_SDA", // GPIO30 > + "HOST_SCL", // GPIO31 > + "ETH_RST_N", // GPIO32 > + "", // GPIO33 > + "CD0_IO0_MICCLK", // GPIO34 > + "CD0_IO0_MICDAT0", // GPIO35 > + "RP1_PCIE_CLKREQ_N", // GPIO36 > + "", // GPIO37 > + "CD0_SDA", // GPIO38 > + "CD0_SCL", // GPIO39 > + "CD1_SDA", // GPIO40 > + "CD1_SCL", // GPIO41 > + "USB_VBUS_EN", // GPIO42 > + "USB_OC_N", // GPIO43 > + "RP1_STAT_LED", // GPIO44 > + "FAN_PWM", // GPIO45 > + "CD1_IO0_MICCLK", // GPIO46 > + "2712_WAKE", // GPIO47 > + "CD1_IO1_MICDAT1", // GPIO48 > + "EN_MAX_USB_CUR", // GPIO49 > + "", // GPIO50 > + "", // GPIO51 > + "", // GPIO52 > + ""; // GPIO53 GPIO line names are board specific, so this should go to the Raspberry Pi 5 file. > + }; > + }; > + }; > + }; > +}; > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 41c3d2821a78..02405209e6c4 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -618,4 +618,5 @@ source "drivers/misc/uacce/Kconfig" > source "drivers/misc/pvpanic/Kconfig" > source "drivers/misc/mchp_pci1xxxx/Kconfig" > source "drivers/misc/keba/Kconfig" > +source "drivers/misc/rp1/Kconfig" > endmenu > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index c2f990862d2b..84bfa866fbee 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -71,3 +71,4 @@ obj-$(CONFIG_TPS6594_PFSM) += tps6594-pfsm.o > obj-$(CONFIG_NSM) += nsm.o > obj-$(CONFIG_MARVELL_CN10K_DPI) += mrvl_cn10k_dpi.o > obj-y += keba/ > +obj-$(CONFIG_MISC_RP1) += rp1/ > diff --git a/drivers/misc/rp1/Kconfig b/drivers/misc/rp1/Kconfig > new file mode 100644 > index 000000000000..050417ee09ae > --- /dev/null > +++ b/drivers/misc/rp1/Kconfig > @@ -0,0 +1,20 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +# > +# RaspberryPi RP1 misc device > +# > + > +config MISC_RP1 > + tristate "RaspberryPi RP1 PCIe support" > + depends on PCI && PCI_QUIRKS > + select OF > + select OF_OVERLAY > + select IRQ_DOMAIN > + select PCI_DYNAMIC_OF_NODES > + help > + Support for the RP1 peripheral chip found on Raspberry Pi 5 board. > + This device supports several sub-devices including e.g. Ethernet controller, > + USB controller, I2C, SPI and UART. > + The driver is responsible for enabling the DT node once the PCIe endpoint > + has been configured, and handling interrupts. > + This driver uses an overlay to load other drivers to support for RP1 > + internal sub-devices. > diff --git a/drivers/misc/rp1/Makefile b/drivers/misc/rp1/Makefile > new file mode 100644 > index 000000000000..e83854b4ed2c > --- /dev/null > +++ b/drivers/misc/rp1/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +rp1-pci-objs := rp1-pci.o rp1-pci.dtbo.o > +obj-$(CONFIG_MISC_RP1) += rp1-pci.o > diff --git a/drivers/misc/rp1/rp1-pci.c b/drivers/misc/rp1/rp1-pci.c > new file mode 100644 > index 000000000000..a6093ba7e19a > --- /dev/null > +++ b/drivers/misc/rp1/rp1-pci.c > @@ -0,0 +1,333 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018-22 Raspberry Pi Ltd. > + * All rights reserved. > + */ > + > +#include <linux/clk.h> > +#include <linux/clkdev.h> > +#include <linux/clk-provider.h> > +#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 <linux/reset.h> > + > +#include <dt-bindings/misc/rp1.h> > + > +#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 RP1_ACTUAL_IRQS RP1_INT_END > +#define RP1_IRQS RP1_ACTUAL_IRQS > +#define RP1_HW_IRQ_MASK GENMASK(5, 0) > + > +#define RP1_SYSCLK_RATE 200000000 > +#define RP1_SYSCLK_FPGA_RATE 60000000 > + > +enum { > + SYSINFO_CHIP_ID_OFFSET = 0, > + SYSINFO_PLATFORM_OFFSET = 4, > +}; > + > +#define REG_SET 0x800 > +#define REG_CLR 0xc00 > + > +/* MSIX CFG registers start at 0x8 */ > +#define MSIX_CFG(x) (0x8 + (4 * (x))) > + > +#define MSIX_CFG_IACK_EN BIT(3) > +#define MSIX_CFG_IACK BIT(2) > +#define MSIX_CFG_TEST BIT(1) > +#define MSIX_CFG_ENABLE BIT(0) > + > +#define INTSTATL 0x108 > +#define INTSTATH 0x10c > + > +extern char __dtbo_rp1_pci_begin[]; > +extern char __dtbo_rp1_pci_end[]; > + > +struct rp1_dev { > + struct pci_dev *pdev; > + struct device *dev; > + struct clk *sys_clk; > + struct irq_domain *domain; > + struct irq_data *pcie_irqds[64]; > + void __iomem *bar1; > + int ovcs_id; > + bool level_triggered_irq[RP1_ACTUAL_IRQS]; > +}; > + > + ... > + > +static const struct pci_device_id dev_id_table[] = { > + { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RP1_C0), }, > + { 0, } > +}; > + > +static struct pci_driver rp1_driver = { > + .name = RP1_DRIVER_NAME, > + .id_table = dev_id_table, > + .probe = rp1_probe, > + .remove = rp1_remove, > +}; > + > +module_pci_driver(rp1_driver); > + > +MODULE_AUTHOR("Phil Elwell <phil@raspberrypi.com>"); Module author & Copyright doesn't seem to match with this patch author. Please clarify/fix
On 8/20/24 07:36, Andrea della Porta wrote: > RaspberryPi RP1 contains Cadence's MACB core. Implement the > changes to be able to operate the customization in the RP1. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> You are doing a lot of things, all at once, and you should consider extracting your change into a smaller subset with bug fixes first: - one commit which writes to the RBQPH the upper 32-bits of the RX ring DMA address, that looks like a bug fix - one commit which retriggers a buffer read, even though that appears to be RP1 specific maybe, if not, then this is also a bug fix - one commit that adds support for macb_shutdown() to kill DMA operations - one commit which adds support for a configurable PHY reset line + delay specified in milli seconds - one commit which adds support for controling the interrupt coalescing settings And then you can add all of the RP1 specific bits like the AXI bridge configuration. [snip] > @@ -1228,6 +1246,7 @@ struct macb_queue { > dma_addr_t tx_ring_dma; > struct work_struct tx_error_task; > bool txubr_pending; > + bool tx_pending; > struct napi_struct napi_tx; > > dma_addr_t rx_ring_dma; > @@ -1293,9 +1312,15 @@ struct macb { > > u32 caps; > unsigned int dma_burst_length; > + u8 aw2w_max_pipe; > + u8 ar2r_max_pipe; > + bool use_aw2b_fill; > > phy_interface_t phy_interface; > > + struct gpio_desc *phy_reset_gpio; > + int phy_reset_ms; The delay cannot be negative, so this needs to be unsigned int. > + > /* AT91RM9200 transmit queue (1 on wire + 1 queued) */ > struct macb_tx_skb rm9200_txq[2]; > unsigned int max_tx_length; > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 11665be3a22c..5eb5be6c96fc 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -41,6 +41,9 @@ > #include <linux/inetdevice.h> > #include "macb.h" > > +static unsigned int txdelay = 35; > +module_param(txdelay, uint, 0644); > + > /* This structure is only used for MACB on SiFive FU540 devices */ > struct sifive_fu540_macb_mgmt { > void __iomem *reg; > @@ -334,7 +337,7 @@ static int macb_mdio_wait_for_idle(struct macb *bp) > u32 val; > > return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_BIT(IDLE), > - 1, MACB_MDIO_TIMEOUT); > + 100, MACB_MDIO_TIMEOUT); Why do we need to increase how frequently we poll?
On 8/20/24 07:36, Andrea della Porta wrote: > RaspberryPi RP1 is multi function PCI endpoint device that > exposes several subperipherals via PCI BAR. > Add an ethernet node for Cadence MACB to the RP1 dtso > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > arch/arm64/boot/dts/broadcom/rp1.dtso | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > index d80178a278ee..b40e203c28d5 100644 > --- a/arch/arm64/boot/dts/broadcom/rp1.dtso > +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > @@ -78,6 +78,29 @@ rp1_clocks: clocks@c040018000 { > <50000000>; // RP1_CLK_ETH_TSU > }; > > + rp1_eth: ethernet@c040100000 { > + reg = <0xc0 0x40100000 0x0 0x4000>; > + compatible = "cdns,macb"; > + #address-cells = <1>; > + #size-cells = <0>; > + interrupts = <RP1_INT_ETH IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&macb_pclk &macb_hclk &rp1_clocks RP1_CLK_ETH_TSU>; > + clock-names = "pclk", "hclk", "tsu_clk"; > + phy-mode = "rgmii-id"; > + cdns,aw2w-max-pipe = /bits/ 8 <8>; > + cdns,ar2r-max-pipe = /bits/ 8 <8>; > + cdns,use-aw2b-fill; > + local-mac-address = [00 00 00 00 00 00]; > + phy-handle = <&phy1>; > + phy-reset-gpios = <&rp1_gpio 32 GPIO_ACTIVE_LOW>; > + phy-reset-duration = <5>; > + > + phy1: ethernet-phy@1 { > + reg = <0x1>; > + brcm,powerdown-enable; Undocumented property, and I would like to understand why this needs to be specified in the Device Tree? What model of Broadcom Ethernet PHY is being used here?
Hi Krzysztof, On 15:42 Wed 21 Aug , Krzysztof Kozlowski wrote: > On 20/08/2024 16:36, Andrea della Porta wrote: > > RP1 is an MFD chipset that acts as a south-bridge PCIe endpoint sporting > > a pletora of subdevices (i.e. Ethernet, USB host controller, I2C, PWM, > > etc.) whose registers are all reachable starting from an offset from the > > BAR address. The main point here is that while the RP1 as an endpoint > > itself is discoverable via usual PCI enumeraiton, the devices it contains > > are not discoverable and must be declared e.g. via the devicetree. > > > > This patchset is an attempt to provide a minimum infrastructure to allow > > the RP1 chipset to be discovered and perpherals it contains to be added > > from a devictree overlay loaded during RP1 PCI endpoint enumeration. > > Followup patches should add support for the several peripherals contained > > in RP1. > > > > This work is based upon dowstream drivers code and the proposal from RH > > et al. (see [1] and [2]). A similar approach is also pursued in [3]. > > Looking briefly at findings it seems this was not really tested by > automation and you expect reviewers to find issues which are pointed out > by tools. That's not nice approach. Reviewer's time is limited, while > tools do it for free. And the tools are free - you can use them without > any effort. Sorry if I gave you that impression, but this is not obviously the case. I've spent quite a bit of time in trying to deliver a patchset that ease your and others work, at least to the best I can. In fact, I've used many of the checking facilities you mentioned before sending it, solving all of the reported issues, except the ones for which there are strong reasons to leave untouched, as explained below. > > It does not look like you tested the DTS against bindings. Please run > `make dtbs_check W=1` (see > Documentation/devicetree/bindings/writing-schema.rst or > https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > for instructions). #> make W=1 dt_binding_check DT_SCHEMA_FILES=raspberrypi,rp1-gpio.yaml CHKDT Documentation/devicetree/bindings LINT Documentation/devicetree/bindings DTEX Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.example.dts DTC_CHK Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.example.dtb #> make W=1 dt_binding_check DT_SCHEMA_FILES=raspberrypi,rp1-clocks.yaml CHKDT Documentation/devicetree/bindings LINT Documentation/devicetree/bindings DTEX Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.example.dts DTC_CHK Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.example.dtb I see no issues here, in case you've found something different, I kindly ask you to post the results. #> make W=1 CHECK_DTBS=y broadcom/rp1.dtbo DTC arch/arm64/boot/dts/broadcom/rp1.dtbo arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property I believe that These warnings are unavoidable, and stem from the fact that this is quite a peculiar setup (PCI endpoint which dynamically loads platform driver addressable via BAR). The missing reg/ranges in the threee clocks are due to the simple-bus of the containing node to which I believe they should belong: I did a test to place those clocks in the same dtso under root or /clocks node but AFAIK it doesn't seems to work. I could move them in a separate dtso to be loaded before the main one but this is IMHO even more cumbersome than having a couple of warnings in CHECK_DTBS. Of course, if you have any suggestion on how to improve it I would be glad to discuss. About the last warning about the address/size-cells, if I drop those two lines in the _overlay_ node it generates even more warning, so again it's a "don't fix" one. > > Please run standard kernel tools for static analysis, like coccinelle, > smatch and sparse, and fix reported warnings. Also please check for > warnings when building with W=1. Most of these commands (checks or W=1 > build) can build specific targets, like some directory, to narrow the > scope to only your code. The code here looks like it needs a fix. Feel > free to get in touch if the warning is not clear. I didn't run those static analyzers since I've preferred a more "manual" aproach by carfeully checking the code, but I agree that something can escape even the more carefully executed code inspection so I will add them to my arsenal from now on. Thanks for the heads up. > > Please run scripts/checkpatch.pl and fix reported warnings. Then please > run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. > Some warnings can be ignored, especially from --strict run, but the code > here looks like it needs a fix. Feel free to get in touch if the warning > is not clear. > Again, most of checkpatch's complaints have been addressed, the remaining ones I deemed as not worth fixing, for example: #> scripts/checkpatch.pl --strict --codespell tmp/*.patch WARNING: please write a help paragraph that fully describes the config symbol #42: FILE: drivers/clk/Kconfig:91: +config COMMON_CLK_RP1 + tristate "Raspberry Pi RP1-based clock support" + depends on PCI || COMPILE_TEST + depends on COMMON_CLK + help + Enable common clock framework support for Raspberry Pi RP1. + This mutli-function device has 3 main PLLs and several clock + generators to drive the internal sub-peripherals. + I don't understand this warning, the paragraph is there and is more or less similar to many in the same file that are already upstream. Checkpatch bug? CHECK: Alignment should match open parenthesis #1541: FILE: drivers/clk/clk-rp1.c:1470: + if (WARN_ON_ONCE(clock_data->num_std_parents > AUX_SEL && + strcmp("-", clock_data->parents[AUX_SEL]))) This would have worsen the code readability. WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP #673: FILE: drivers/pinctrl/pinctrl-rp1.c:600: + return -ENOTSUPP; This I must investigate: I've already tried to fix it before sending the patchset but for some reason it wouldn't work, so I planned to fix it in the upcoming releases. WARNING: externs should be avoided in .c files #331: FILE: drivers/misc/rp1/rp1-pci.c:58: +extern char __dtbo_rp1_pci_begin[]; True, but in this case we don't have a symbol that should be exported to other translation units, it just needs to be referenced inside the driver and consumed locally. Hence it would be better to place the extern in .c file. Apologies for a couple of other warnings that I could have seen in the first place, but honestly they don't seems to be a big deal (one typo and on over 100 chars comment, that will be fixed in next patch version). > > Best regards, > Krzysztof > Many thanks, Andrea
On 22/08/2024 11:05, Andrea della Porta wrote: > Hi Krzysztof, > > On 15:42 Wed 21 Aug , Krzysztof Kozlowski wrote: >> On 20/08/2024 16:36, Andrea della Porta wrote: >>> RP1 is an MFD chipset that acts as a south-bridge PCIe endpoint sporting >>> a pletora of subdevices (i.e. Ethernet, USB host controller, I2C, PWM, >>> etc.) whose registers are all reachable starting from an offset from the >>> BAR address. The main point here is that while the RP1 as an endpoint >>> itself is discoverable via usual PCI enumeraiton, the devices it contains >>> are not discoverable and must be declared e.g. via the devicetree. >>> >>> This patchset is an attempt to provide a minimum infrastructure to allow >>> the RP1 chipset to be discovered and perpherals it contains to be added >>> from a devictree overlay loaded during RP1 PCI endpoint enumeration. >>> Followup patches should add support for the several peripherals contained >>> in RP1. >>> >>> This work is based upon dowstream drivers code and the proposal from RH >>> et al. (see [1] and [2]). A similar approach is also pursued in [3]. >> >> Looking briefly at findings it seems this was not really tested by >> automation and you expect reviewers to find issues which are pointed out >> by tools. That's not nice approach. Reviewer's time is limited, while >> tools do it for free. And the tools are free - you can use them without >> any effort. > > Sorry if I gave you that impression, but this is not obviously the case. Just look at number of reports... so many sparse reports that I wonder how it is not the case. And many kbuild reports. > I've spent quite a bit of time in trying to deliver a patchset that ease > your and others work, at least to the best I can. In fact, I've used many > of the checking facilities you mentioned before sending it, solving all > of the reported issues, except the ones for which there are strong reasons > to leave untouched, as explained below. > >> >> It does not look like you tested the DTS against bindings. Please run >> `make dtbs_check W=1` (see >> Documentation/devicetree/bindings/writing-schema.rst or >> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ >> for instructions). > > #> make W=1 dt_binding_check DT_SCHEMA_FILES=raspberrypi,rp1-gpio.yaml > CHKDT Documentation/devicetree/bindings > LINT Documentation/devicetree/bindings > DTEX Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.example.dts > DTC_CHK Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.example.dtb > > #> make W=1 dt_binding_check DT_SCHEMA_FILES=raspberrypi,rp1-clocks.yaml > CHKDT Documentation/devicetree/bindings > LINT Documentation/devicetree/bindings > DTEX Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.example.dts > DTC_CHK Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.example.dtb > > I see no issues here, in case you've found something different, I kindly ask you to post > the results. > > #> make W=1 CHECK_DTBS=y broadcom/rp1.dtbo > DTC arch/arm64/boot/dts/broadcom/rp1.dtbo > arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property > > I believe that These warnings are unavoidable, and stem from the fact that this > is quite a peculiar setup (PCI endpoint which dynamically loads platform driver > addressable via BAR). > The missing reg/ranges in the threee clocks are due to the simple-bus of the > containing node to which I believe they should belong: I did a test to place This is not the place where they belong. non-MMIO nodes should not be under simple-bus. > those clocks in the same dtso under root or /clocks node but AFAIK it doesn't > seems to work. I could move them in a separate dtso to be loaded before the main Well... who instantiates them? If they are in top-level, then CLK_OF_DECLARE which is not called at your point? You must instantiate clocks different way, since they are not part of "rp1". That's another bogus DT description... external oscilator is not part of RP1. > one but this is IMHO even more cumbersome than having a couple of warnings in > CHECK_DTBS. > Of course, if you have any suggestion on how to improve it I would be glad to > discuss. > About the last warning about the address/size-cells, if I drop those two lines > in the _overlay_ node it generates even more warning, so again it's a "don't fix" > one. > >> >> Please run standard kernel tools for static analysis, like coccinelle, >> smatch and sparse, and fix reported warnings. Also please check for >> warnings when building with W=1. Most of these commands (checks or W=1 >> build) can build specific targets, like some directory, to narrow the >> scope to only your code. The code here looks like it needs a fix. Feel >> free to get in touch if the warning is not clear. > > I didn't run those static analyzers since I've preferred a more "manual" aproach > by carfeully checking the code, but I agree that something can escape even the > more carefully executed code inspection so I will add them to my arsenal from > now on. Thanks for the heads up. I don't care if you do not run static analyzers if you produce good code. But if you produce bugs which could have been easily spotted with sparser, than it is different thing. Start running static checkers instead of asking reviewers to do that. > >> >> Please run scripts/checkpatch.pl and fix reported warnings. Then please >> run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. >> Some warnings can be ignored, especially from --strict run, but the code >> here looks like it needs a fix. Feel free to get in touch if the warning >> is not clear. >> > > Again, most of checkpatch's complaints have been addressed, the remaining > ones I deemed as not worth fixing, for example:> > #> scripts/checkpatch.pl --strict --codespell tmp/*.patch > > WARNING: please write a help paragraph that fully describes the config symbol > #42: FILE: drivers/clk/Kconfig:91: > +config COMMON_CLK_RP1 > + tristate "Raspberry Pi RP1-based clock support" > + depends on PCI || COMPILE_TEST > + depends on COMMON_CLK > + help > + Enable common clock framework support for Raspberry Pi RP1. > + This mutli-function device has 3 main PLLs and several clock > + generators to drive the internal sub-peripherals. > + > > I don't understand this warning, the paragraph is there and is more or less similar > to many in the same file that are already upstream. Checkpatch bug? > > > CHECK: Alignment should match open parenthesis > #1541: FILE: drivers/clk/clk-rp1.c:1470: > + if (WARN_ON_ONCE(clock_data->num_std_parents > AUX_SEL && > + strcmp("-", clock_data->parents[AUX_SEL]))) > > This would have worsen the code readability. > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > #673: FILE: drivers/pinctrl/pinctrl-rp1.c:600: > + return -ENOTSUPP; > > This I must investigate: I've already tried to fix it before sending the patchset > but for some reason it wouldn't work, so I planned to fix it in the upcoming > releases. > > > WARNING: externs should be avoided in .c files > #331: FILE: drivers/misc/rp1/rp1-pci.c:58: > +extern char __dtbo_rp1_pci_begin[]; > > True, but in this case we don't have a symbol that should be exported to other > translation units, it just needs to be referenced inside the driver and > consumed locally. Hence it would be better to place the extern in .c file. > > > Apologies for a couple of other warnings that I could have seen in the first > place, but honestly they don't seems to be a big deal (one typo and on over > 100 chars comment, that will be fixed in next patch version). Again, judging by number of reports from checkers that is a big deal, because it is your task to run the tools. Best regards, Krzysztof
Hi Simon, On 14:17 Wed 21 Aug , Simon Horman wrote: > On Tue, Aug 20, 2024 at 04:36:08PM +0200, Andrea della Porta wrote: > > RaspberryPi RP1 is an MFD providing, among other peripherals, several > > clock generators and PLLs that drives the sub-peripherals. > > Add the driver to support the clock providers. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > ... > > > diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c > > new file mode 100644 > > index 000000000000..d18e711c0623 > > --- /dev/null > > +++ b/drivers/clk/clk-rp1.c > > @@ -0,0 +1,1655 @@ > > +// SPDX-License-Identifier: GPL > > checkpatch says: > > WARNING: 'SPDX-License-Identifier: GPL' is not supported in LICENSES/... > Alas, the system on which I executed checkpatch was missing git python module, so spdxcheck.py wasn't working properly, sorry about that. Fixed in the next release. > ... > > > +static int rp1_clock_set_parent(struct clk_hw *hw, u8 index) > > +{ > > + struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw); > > + struct rp1_clockman *clockman = clock->clockman; > > + const struct rp1_clock_data *data = clock->data; > > + u32 ctrl, sel; > > + > > + spin_lock(&clockman->regs_lock); > > + ctrl = clockman_read(clockman, data->ctrl_reg); > > + > > + if (index >= data->num_std_parents) { > > + /* This is an aux source request */ > > + if (index >= data->num_std_parents + data->num_aux_parents) > > It looks like &clockman->regs_lock needs to be unlocked here. > > Flagged by Smatch, Sparse. and Coccinelle. Ack. Many thanks, Andrea > > > + return -EINVAL; > > + > > + /* Select parent from aux list */ > > + ctrl = set_register_field(ctrl, index - data->num_std_parents, > > + CLK_CTRL_AUXSRC_MASK, > > + CLK_CTRL_AUXSRC_SHIFT); > > + /* Set src to aux list */ > > + ctrl = set_register_field(ctrl, AUX_SEL, data->clk_src_mask, > > + CLK_CTRL_SRC_SHIFT); > > + } else { > > + ctrl = set_register_field(ctrl, index, data->clk_src_mask, > > + CLK_CTRL_SRC_SHIFT); > > + } > > + > > + clockman_write(clockman, data->ctrl_reg, ctrl); > > + spin_unlock(&clockman->regs_lock); > > + > > + sel = rp1_clock_get_parent(hw); > > + WARN(sel != index, "(%s): Parent index req %u returned back %u\n", > > + data->name, index, sel); > > + > > + return 0; > > +} > > ...
> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > #673: FILE: drivers/pinctrl/pinctrl-rp1.c:600: > + return -ENOTSUPP; > > This I must investigate: I've already tried to fix it before sending the patchset > but for some reason it wouldn't work, so I planned to fix it in the upcoming > releases. ENOTSUPP is an NFS error. It should not be used outside for NFS. You want EOPNOTSUPP. > WARNING: externs should be avoided in .c files > #331: FILE: drivers/misc/rp1/rp1-pci.c:58: > +extern char __dtbo_rp1_pci_begin[]; > > True, but in this case we don't have a symbol that should be exported to other > translation units, it just needs to be referenced inside the driver and > consumed locally. Hence it would be better to place the extern in .c file. Did you try making it static. Andrew
On 22/08/2024 16:33, Andrea della Porta wrote: > Hi Krzysztof, > > On 16:20 Wed 21 Aug , Krzysztof Kozlowski wrote: >> On 21/08/2024 10:38, Krzysztof Kozlowski wrote: >>> On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: >> >> ... >> >>>> drivers/misc/Kconfig | 1 + >>>> drivers/misc/Makefile | 1 + >>>> drivers/misc/rp1/Kconfig | 20 ++ >>>> drivers/misc/rp1/Makefile | 3 + >>>> drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ >>>> drivers/misc/rp1/rp1-pci.dtso | 8 + >>>> drivers/pci/quirks.c | 1 + >>>> include/linux/pci_ids.h | 3 + >>>> 10 files changed, 524 insertions(+) >>>> create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso >>>> create mode 100644 drivers/misc/rp1/Kconfig >>>> create mode 100644 drivers/misc/rp1/Makefile >>>> create mode 100644 drivers/misc/rp1/rp1-pci.c >>>> create mode 100644 drivers/misc/rp1/rp1-pci.dtso >>>> >>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>> index 67f460c36ea1..1359538b76e8 100644 >>>> --- a/MAINTAINERS >>>> +++ b/MAINTAINERS >>>> @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ >>>> RASPBERRY PI RP1 PCI DRIVER >>>> M: Andrea della Porta <andrea.porta@suse.com> >>>> S: Maintained >>>> +F: arch/arm64/boot/dts/broadcom/rp1.dtso >>>> F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml >>>> F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml >>>> F: drivers/clk/clk-rp1.c >>>> +F: drivers/misc/rp1/ >>>> F: drivers/pinctrl/pinctrl-rp1.c >>>> F: include/dt-bindings/clock/rp1.h >>>> F: include/dt-bindings/misc/rp1.h >>>> diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso >>>> new file mode 100644 >>>> index 000000000000..d80178a278ee >>>> --- /dev/null >>>> +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso >>>> @@ -0,0 +1,152 @@ >>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT) >>>> + >>>> +#include <dt-bindings/gpio/gpio.h> >>>> +#include <dt-bindings/interrupt-controller/irq.h> >>>> +#include <dt-bindings/clock/rp1.h> >>>> +#include <dt-bindings/misc/rp1.h> >>>> + >>>> +/dts-v1/; >>>> +/plugin/; >>>> + >>>> +/ { >>>> + fragment@0 { >>>> + target-path=""; >>>> + __overlay__ { >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + >>>> + rp1: rp1@0 { >>>> + compatible = "simple-bus"; >>>> + #address-cells = <2>; >>>> + #size-cells = <2>; >>>> + interrupt-controller; >>>> + interrupt-parent = <&rp1>; >>>> + #interrupt-cells = <2>; >>>> + >>>> + // ranges and dma-ranges must be provided by the includer >>>> + ranges = <0xc0 0x40000000 >>>> + 0x01/*0x02000000*/ 0x00 0x00000000 >>>> + 0x00 0x00400000>; >>> >>> Are you 100% sure you do not have here dtc W=1 warnings? >> >> One more thing, I do not see this overlay applied to any target, which >> means it cannot be tested. You miss entry in Makefile. >> > > The dtso is intended to be built from driver/misc/rp1/Makefile as it will > be included in the driver obj: > > --- /dev/null > +++ b/drivers/misc/rp1/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +rp1-pci-objs := rp1-pci.o rp1-pci.dtbo.o > +obj-$(CONFIG_MISC_RP1) += rp1-pci.o > > and not as part of the dtb system, hence it's m issing in > arch/arm64/boot/dts/broadcom/Makefile. > > On the other hand: > > #> make W=1 CHECK_DTBS=y broadcom/rp1.dtbo > DTC arch/arm64/boot/dts/broadcom/rp1.dtbo > arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property > > seems to do the checks, unless I'm missing something. Yeah, there is still no target which applies the overlay, so no one can tell whether it applies cleanly or not. You can only test single overlay, but it is expected to test each overlay being applied to chosen DTB. Best regards, Krzysztof
Hi Stefan, On 18:20 Wed 21 Aug , Stefan Wahren wrote: > Hi Andrea, > > Am 20.08.24 um 16:36 schrieb Andrea della Porta: > > The RaspberryPi RP1 is ia PCI multi function device containing > > peripherals ranging from Ethernet to USB controller, I2C, SPI > > and others. > sorry, i cannot provide you a code review, but just some comments. multi > function device suggests "mfd" subsystem or at least "soc" . I won't > recommend misc driver here. It's true that RP1 can be called an MFD but the reason for not placing it in mfd subsystem are twofold: - these discussions are quite clear about this matter: please see [1] and [2] - the current driver use no mfd API at all This RP1 driver is not currently addressing any aspect of ARM core in the SoC so I would say it should not stay in drivers/soc / either, as also condifirmed by [2] again and [3] (note that Microchip LAN966x is a very close fit to what we have here on RP1). > > Implement a bare minimum driver to operate the RP1, leveraging > > actual OF based driver implementations for the on-borad peripherals > > by loading a devicetree overlay during driver probe. > Can you please explain why this should be a DT overlay? The RP1 is > assembled on the Raspberry Pi 5 PCB. DT overlays are typically for loose > connections like displays or HATs. I think a DTSI just for the RP1 would > fit better and is easier to read. The dtsi solution you proposed is the one adopted downstream. It has its benefits of course, but there's more. 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 be possibly be reused in other hw implementations. I believe a similar reasoning could be applied to Bootlin's Microchip LAN966x patchset as well, and they also choose to approach the dtb overlay. Plus, a solution that can (althoguh proabbly in teh long run) cope with both DT or ACPI based system has been kindly requested, plase see [4] for details. IMHO the approach proposed from RH et al. of using dtbo for this 'special' kind of drivers makes a lot of sense (see [5]). > > The peripherals are accessed by mapping MMIO registers starting > > from PCI BAR1 region. > > As a minimum driver, the peripherals will not be added to the > > dtbo here, but in following patches. > > > > Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > MAINTAINERS | 2 + > > arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ > > drivers/misc/Kconfig | 1 + > > drivers/misc/Makefile | 1 + > > drivers/misc/rp1/Kconfig | 20 ++ > > drivers/misc/rp1/Makefile | 3 + > > drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ > > drivers/misc/rp1/rp1-pci.dtso | 8 + > > drivers/pci/quirks.c | 1 + > > include/linux/pci_ids.h | 3 + > > 10 files changed, 524 insertions(+) > > create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso > > create mode 100644 drivers/misc/rp1/Kconfig > > create mode 100644 drivers/misc/rp1/Makefile > > create mode 100644 drivers/misc/rp1/rp1-pci.c > > create mode 100644 drivers/misc/rp1/rp1-pci.dtso > > > ... > > + > > + rp1_clocks: clocks@c040018000 { > > + compatible = "raspberrypi,rp1-clocks"; > > + #clock-cells = <1>; > > + reg = <0xc0 0x40018000 0x0 0x10038>; > > + clocks = <&clk_xosc>; > > + clock-names = "xosc"; > > + > > + assigned-clocks = <&rp1_clocks RP1_PLL_SYS_CORE>, > > + <&rp1_clocks RP1_PLL_AUDIO_CORE>, > > + // RP1_PLL_VIDEO_CORE and dividers are now managed by VEC,DPI drivers > > + <&rp1_clocks RP1_PLL_SYS>, > > + <&rp1_clocks RP1_PLL_SYS_SEC>, > > + <&rp1_clocks RP1_PLL_SYS_PRI_PH>, > > + <&rp1_clocks RP1_CLK_ETH_TSU>; > > + > > + assigned-clock-rates = <1000000000>, // RP1_PLL_SYS_CORE > > + <1536000000>, // RP1_PLL_AUDIO_CORE > > + <200000000>, // RP1_PLL_SYS > > + <125000000>, // RP1_PLL_SYS_SEC > > + <100000000>, // RP1_PLL_SYS_PRI_PH > > + <50000000>; // RP1_CLK_ETH_TSU > > + }; > > + > > + rp1_gpio: pinctrl@c0400d0000 { > > + reg = <0xc0 0x400d0000 0x0 0xc000>, > > + <0xc0 0x400e0000 0x0 0xc000>, > > + <0xc0 0x400f0000 0x0 0xc000>; > > + compatible = "raspberrypi,rp1-gpio"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + interrupt-controller; > > + #interrupt-cells = <2>; > > + interrupts = <RP1_INT_IO_BANK0 IRQ_TYPE_LEVEL_HIGH>, > > + <RP1_INT_IO_BANK1 IRQ_TYPE_LEVEL_HIGH>, > > + <RP1_INT_IO_BANK2 IRQ_TYPE_LEVEL_HIGH>; > > + gpio-line-names = > > + "ID_SDA", // GPIO0 > > + "ID_SCL", // GPIO1 > > + "GPIO2", // GPIO2 > > + "GPIO3", // GPIO3 > > + "GPIO4", // GPIO4 > > + "GPIO5", // GPIO5 > > + "GPIO6", // GPIO6 > > + "GPIO7", // GPIO7 > > + "GPIO8", // GPIO8 > > + "GPIO9", // GPIO9 > > + "GPIO10", // GPIO10 > > + "GPIO11", // GPIO11 > > + "GPIO12", // GPIO12 > > + "GPIO13", // GPIO13 > > + "GPIO14", // GPIO14 > > + "GPIO15", // GPIO15 > > + "GPIO16", // GPIO16 > > + "GPIO17", // GPIO17 > > + "GPIO18", // GPIO18 > > + "GPIO19", // GPIO19 > > + "GPIO20", // GPIO20 > > + "GPIO21", // GPIO21 > > + "GPIO22", // GPIO22 > > + "GPIO23", // GPIO23 > > + "GPIO24", // GPIO24 > > + "GPIO25", // GPIO25 > > + "GPIO26", // GPIO26 > > + "GPIO27", // GPIO27 > > + "PCIE_RP1_WAKE", // GPIO28 > > + "FAN_TACH", // GPIO29 > > + "HOST_SDA", // GPIO30 > > + "HOST_SCL", // GPIO31 > > + "ETH_RST_N", // GPIO32 > > + "", // GPIO33 > > + "CD0_IO0_MICCLK", // GPIO34 > > + "CD0_IO0_MICDAT0", // GPIO35 > > + "RP1_PCIE_CLKREQ_N", // GPIO36 > > + "", // GPIO37 > > + "CD0_SDA", // GPIO38 > > + "CD0_SCL", // GPIO39 > > + "CD1_SDA", // GPIO40 > > + "CD1_SCL", // GPIO41 > > + "USB_VBUS_EN", // GPIO42 > > + "USB_OC_N", // GPIO43 > > + "RP1_STAT_LED", // GPIO44 > > + "FAN_PWM", // GPIO45 > > + "CD1_IO0_MICCLK", // GPIO46 > > + "2712_WAKE", // GPIO47 > > + "CD1_IO1_MICDAT1", // GPIO48 > > + "EN_MAX_USB_CUR", // GPIO49 > > + "", // GPIO50 > > + "", // GPIO51 > > + "", // GPIO52 > > + ""; // GPIO53 > GPIO line names are board specific, so this should go to the Raspberry > Pi 5 file. Could we instead just name them with generic GPIO'N' where N is the number of the gpio? Much like many of that pins already are... in this way we don't add a dependency in the board dts to the rp1_gpio node, which is not even there when the main dts is parsed at boot, since the dtbo will be added only on PCI enumeration of the RP1 device. Or even better: since we don't explicitly use the gpio names to address them (e.g. phy-reset-gpios in rp1_eth node is addressing the ETH_RST_N gpio by number), can we just get rid of the gpio-line-names property? Also Bootlin's Microchip gpio node seems to avoid naming them... > > + }; > > + }; > > + }; > > + }; > > +}; > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > > index 41c3d2821a78..02405209e6c4 100644 > > --- a/drivers/misc/Kconfig > > +++ b/drivers/misc/Kconfig > > @@ -618,4 +618,5 @@ source "drivers/misc/uacce/Kconfig" > > source "drivers/misc/pvpanic/Kconfig" > > source "drivers/misc/mchp_pci1xxxx/Kconfig" > > source "drivers/misc/keba/Kconfig" > > +source "drivers/misc/rp1/Kconfig" > > endmenu > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > > index c2f990862d2b..84bfa866fbee 100644 > > --- a/drivers/misc/Makefile > > +++ b/drivers/misc/Makefile > > @@ -71,3 +71,4 @@ obj-$(CONFIG_TPS6594_PFSM) += tps6594-pfsm.o > > obj-$(CONFIG_NSM) += nsm.o > > obj-$(CONFIG_MARVELL_CN10K_DPI) += mrvl_cn10k_dpi.o > > obj-y += keba/ > > +obj-$(CONFIG_MISC_RP1) += rp1/ > > diff --git a/drivers/misc/rp1/Kconfig b/drivers/misc/rp1/Kconfig > > new file mode 100644 > > index 000000000000..050417ee09ae > > --- /dev/null > > +++ b/drivers/misc/rp1/Kconfig > > @@ -0,0 +1,20 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +# > > +# RaspberryPi RP1 misc device > > +# > > + > > +config MISC_RP1 > > + tristate "RaspberryPi RP1 PCIe support" > > + depends on PCI && PCI_QUIRKS > > + select OF > > + select OF_OVERLAY > > + select IRQ_DOMAIN > > + select PCI_DYNAMIC_OF_NODES > > + help > > + Support for the RP1 peripheral chip found on Raspberry Pi 5 board. > > + This device supports several sub-devices including e.g. Ethernet controller, > > + USB controller, I2C, SPI and UART. > > + The driver is responsible for enabling the DT node once the PCIe endpoint > > + has been configured, and handling interrupts. > > + This driver uses an overlay to load other drivers to support for RP1 > > + internal sub-devices. > > diff --git a/drivers/misc/rp1/Makefile b/drivers/misc/rp1/Makefile > > new file mode 100644 > > index 000000000000..e83854b4ed2c > > --- /dev/null > > +++ b/drivers/misc/rp1/Makefile > > @@ -0,0 +1,3 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +rp1-pci-objs := rp1-pci.o rp1-pci.dtbo.o > > +obj-$(CONFIG_MISC_RP1) += rp1-pci.o > > diff --git a/drivers/misc/rp1/rp1-pci.c b/drivers/misc/rp1/rp1-pci.c > > new file mode 100644 > > index 000000000000..a6093ba7e19a > > --- /dev/null > > +++ b/drivers/misc/rp1/rp1-pci.c > > @@ -0,0 +1,333 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018-22 Raspberry Pi Ltd. > > + * All rights reserved. > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/clkdev.h> > > +#include <linux/clk-provider.h> > > +#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 <linux/reset.h> > > + > > +#include <dt-bindings/misc/rp1.h> > > + > > +#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 RP1_ACTUAL_IRQS RP1_INT_END > > +#define RP1_IRQS RP1_ACTUAL_IRQS > > +#define RP1_HW_IRQ_MASK GENMASK(5, 0) > > + > > +#define RP1_SYSCLK_RATE 200000000 > > +#define RP1_SYSCLK_FPGA_RATE 60000000 > > + > > +enum { > > + SYSINFO_CHIP_ID_OFFSET = 0, > > + SYSINFO_PLATFORM_OFFSET = 4, > > +}; > > + > > +#define REG_SET 0x800 > > +#define REG_CLR 0xc00 > > + > > +/* MSIX CFG registers start at 0x8 */ > > +#define MSIX_CFG(x) (0x8 + (4 * (x))) > > + > > +#define MSIX_CFG_IACK_EN BIT(3) > > +#define MSIX_CFG_IACK BIT(2) > > +#define MSIX_CFG_TEST BIT(1) > > +#define MSIX_CFG_ENABLE BIT(0) > > + > > +#define INTSTATL 0x108 > > +#define INTSTATH 0x10c > > + > > +extern char __dtbo_rp1_pci_begin[]; > > +extern char __dtbo_rp1_pci_end[]; > > + > > +struct rp1_dev { > > + struct pci_dev *pdev; > > + struct device *dev; > > + struct clk *sys_clk; > > + struct irq_domain *domain; > > + struct irq_data *pcie_irqds[64]; > > + void __iomem *bar1; > > + int ovcs_id; > > + bool level_triggered_irq[RP1_ACTUAL_IRQS]; > > +}; > > + > > + > ... > > + > > +static const struct pci_device_id dev_id_table[] = { > > + { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RP1_C0), }, > > + { 0, } > > +}; > > + > > +static struct pci_driver rp1_driver = { > > + .name = RP1_DRIVER_NAME, > > + .id_table = dev_id_table, > > + .probe = rp1_probe, > > + .remove = rp1_remove, > > +}; > > + > > +module_pci_driver(rp1_driver); > > + > > +MODULE_AUTHOR("Phil Elwell <phil@raspberrypi.com>"); > Module author & Copyright doesn't seem to match with this patch author. > Please clarify/fix My intention here is that, even if the code has been heavily modified by me, the core original code is still there so I just wanted to tribute it to the original author. I'll synchronize this with RaspberryPi guys and coem up with a unified solution. Just in case: would multiple MODULE_AUTHOR entries (one with my name and one with original authors name) be accepetd? Many thanks, Andrea References: - [1]: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ - [2]: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ - [3]: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ - [4]: https://lore.kernel.org/all/ba8cdf39-3ba3-4abc-98f5-d394d6867f95@gmx.net/ - [5]: https://lpc.events/event/17/contributions/1421/attachments/1337/2680/LPC2023%20Non-discoverable%20devices%20in%20PCI.pdf
Hi Andrea, Am 23.08.24 um 11:44 schrieb Andrea della Porta: > Hi Stefan, > > On 18:20 Wed 21 Aug , Stefan Wahren wrote: >> Hi Andrea, >> >> Am 20.08.24 um 16:36 schrieb Andrea della Porta: >>> The RaspberryPi RP1 is ia PCI multi function device containing >>> peripherals ranging from Ethernet to USB controller, I2C, SPI >>> and others. >> sorry, i cannot provide you a code review, but just some comments. multi >> function device suggests "mfd" subsystem or at least "soc" . I won't >> recommend misc driver here. > It's true that RP1 can be called an MFD but the reason for not placing > it in mfd subsystem are twofold: > > - these discussions are quite clear about this matter: please see [1] > and [2] > - the current driver use no mfd API at all > > This RP1 driver is not currently addressing any aspect of ARM core in the > SoC so I would say it should not stay in drivers/soc / either, as also > condifirmed by [2] again and [3] (note that Microchip LAN966x is a very > close fit to what we have here on RP1). thanks i was aware of these discussions. A pointer to them or at least a short statement in the cover letter would be great. > >>> Implement a bare minimum driver to operate the RP1, leveraging >>> actual OF based driver implementations for the on-borad peripherals >>> by loading a devicetree overlay during driver probe. >> Can you please explain why this should be a DT overlay? The RP1 is >> assembled on the Raspberry Pi 5 PCB. DT overlays are typically for loose >> connections like displays or HATs. I think a DTSI just for the RP1 would >> fit better and is easier to read. > The dtsi solution you proposed is the one adopted downstream. It has its > benefits of course, but there's more. > 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 be > possibly be reused in other hw implementations. I believe a similar > reasoning could be applied to Bootlin's Microchip LAN966x patchset as > well, and they also choose to approach the dtb overlay. Could please add this point in the commit message. Doesn't introduce (maintainence) issues in case U-Boot needs a RP1 driver, too? > Plus, a solution that can (althoguh proabbly in teh long run) cope > with both DT or ACPI based system has been kindly requested, plase see [4] > for details. > IMHO the approach proposed from RH et al. of using dtbo for this 'special' > kind of drivers makes a lot of sense (see [5]). > >>> The peripherals are accessed by mapping MMIO registers starting >>> from PCI BAR1 region. >>> As a minimum driver, the peripherals will not be added to the >>> dtbo here, but in following patches. >>> >>> Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf >>> Signed-off-by: Andrea della Porta <andrea.porta@suse.com> >>> --- >>> MAINTAINERS | 2 + >>> arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ >>> drivers/misc/Kconfig | 1 + >>> drivers/misc/Makefile | 1 + >>> drivers/misc/rp1/Kconfig | 20 ++ >>> drivers/misc/rp1/Makefile | 3 + >>> drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ >>> drivers/misc/rp1/rp1-pci.dtso | 8 + >>> drivers/pci/quirks.c | 1 + >>> include/linux/pci_ids.h | 3 + >>> 10 files changed, 524 insertions(+) >>> create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso >>> create mode 100644 drivers/misc/rp1/Kconfig >>> create mode 100644 drivers/misc/rp1/Makefile >>> create mode 100644 drivers/misc/rp1/rp1-pci.c >>> create mode 100644 drivers/misc/rp1/rp1-pci.dtso >>> >> ... >>> + >>> + rp1_clocks: clocks@c040018000 { >>> + compatible = "raspberrypi,rp1-clocks"; >>> + #clock-cells = <1>; >>> + reg = <0xc0 0x40018000 0x0 0x10038>; >>> + clocks = <&clk_xosc>; >>> + clock-names = "xosc"; >>> + >>> + assigned-clocks = <&rp1_clocks RP1_PLL_SYS_CORE>, >>> + <&rp1_clocks RP1_PLL_AUDIO_CORE>, >>> + // RP1_PLL_VIDEO_CORE and dividers are now managed by VEC,DPI drivers >>> + <&rp1_clocks RP1_PLL_SYS>, >>> + <&rp1_clocks RP1_PLL_SYS_SEC>, >>> + <&rp1_clocks RP1_PLL_SYS_PRI_PH>, >>> + <&rp1_clocks RP1_CLK_ETH_TSU>; >>> + >>> + assigned-clock-rates = <1000000000>, // RP1_PLL_SYS_CORE >>> + <1536000000>, // RP1_PLL_AUDIO_CORE >>> + <200000000>, // RP1_PLL_SYS >>> + <125000000>, // RP1_PLL_SYS_SEC >>> + <100000000>, // RP1_PLL_SYS_PRI_PH >>> + <50000000>; // RP1_CLK_ETH_TSU >>> + }; >>> + >>> + rp1_gpio: pinctrl@c0400d0000 { >>> + reg = <0xc0 0x400d0000 0x0 0xc000>, >>> + <0xc0 0x400e0000 0x0 0xc000>, >>> + <0xc0 0x400f0000 0x0 0xc000>; >>> + compatible = "raspberrypi,rp1-gpio"; >>> + gpio-controller; >>> + #gpio-cells = <2>; >>> + interrupt-controller; >>> + #interrupt-cells = <2>; >>> + interrupts = <RP1_INT_IO_BANK0 IRQ_TYPE_LEVEL_HIGH>, >>> + <RP1_INT_IO_BANK1 IRQ_TYPE_LEVEL_HIGH>, >>> + <RP1_INT_IO_BANK2 IRQ_TYPE_LEVEL_HIGH>; >>> + gpio-line-names = >>> + "ID_SDA", // GPIO0 >>> + "ID_SCL", // GPIO1 >>> + "GPIO2", // GPIO2 >>> + "GPIO3", // GPIO3 >>> + "GPIO4", // GPIO4 >>> + "GPIO5", // GPIO5 >>> + "GPIO6", // GPIO6 >>> + "GPIO7", // GPIO7 >>> + "GPIO8", // GPIO8 >>> + "GPIO9", // GPIO9 >>> + "GPIO10", // GPIO10 >>> + "GPIO11", // GPIO11 >>> + "GPIO12", // GPIO12 >>> + "GPIO13", // GPIO13 >>> + "GPIO14", // GPIO14 >>> + "GPIO15", // GPIO15 >>> + "GPIO16", // GPIO16 >>> + "GPIO17", // GPIO17 >>> + "GPIO18", // GPIO18 >>> + "GPIO19", // GPIO19 >>> + "GPIO20", // GPIO20 >>> + "GPIO21", // GPIO21 >>> + "GPIO22", // GPIO22 >>> + "GPIO23", // GPIO23 >>> + "GPIO24", // GPIO24 >>> + "GPIO25", // GPIO25 >>> + "GPIO26", // GPIO26 >>> + "GPIO27", // GPIO27 >>> + "PCIE_RP1_WAKE", // GPIO28 >>> + "FAN_TACH", // GPIO29 >>> + "HOST_SDA", // GPIO30 >>> + "HOST_SCL", // GPIO31 >>> + "ETH_RST_N", // GPIO32 >>> + "", // GPIO33 >>> + "CD0_IO0_MICCLK", // GPIO34 >>> + "CD0_IO0_MICDAT0", // GPIO35 >>> + "RP1_PCIE_CLKREQ_N", // GPIO36 >>> + "", // GPIO37 >>> + "CD0_SDA", // GPIO38 >>> + "CD0_SCL", // GPIO39 >>> + "CD1_SDA", // GPIO40 >>> + "CD1_SCL", // GPIO41 >>> + "USB_VBUS_EN", // GPIO42 >>> + "USB_OC_N", // GPIO43 >>> + "RP1_STAT_LED", // GPIO44 >>> + "FAN_PWM", // GPIO45 >>> + "CD1_IO0_MICCLK", // GPIO46 >>> + "2712_WAKE", // GPIO47 >>> + "CD1_IO1_MICDAT1", // GPIO48 >>> + "EN_MAX_USB_CUR", // GPIO49 >>> + "", // GPIO50 >>> + "", // GPIO51 >>> + "", // GPIO52 >>> + ""; // GPIO53 >> GPIO line names are board specific, so this should go to the Raspberry >> Pi 5 file. > Could we instead just name them with generic GPIO'N' where N is the number > of the gpio? Much like many of that pins already are... in this way we > don't add a dependency in the board dts to the rp1_gpio node, which is not > even there when the main dts is parsed at boot, since the dtbo will be > added only on PCI enumeration of the RP1 device. I think we should avoid user space incompatibilities with the vendor tree. > Or even better: since we don't explicitly use the gpio names to address > them (e.g. phy-reset-gpios in rp1_eth node is addressing the ETH_RST_N > gpio by number), can we just get rid of the gpio-line-names property? > Also Bootlin's Microchip gpio node seems to avoid naming them... As i said above the gpio lines are for user space, honestly nobody likes to go to cryptic interfaces of gpiochips and gpio numbers. Maybe ETH_RST_N isn't good example because this not interesting from user space. For example RP1_STAT_LED is a better one. Nobody can predict the future use cases of the RP1 and its pins. So i think we should have the flexibilty to specify the GPIOs on the board level for user friendliness. Isn't it possible to specify almost empty rp1 node with the gpio line names for the RPi 5 and apply the rp1 overlay on top? > >>> + }; >>> + }; >>> + }; >>> + }; >>> +}; >>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig >>> index 41c3d2821a78..02405209e6c4 100644 >>> --- a/drivers/misc/Kconfig >>> +++ b/drivers/misc/Kconfig >>> @@ -618,4 +618,5 @@ source "drivers/misc/uacce/Kconfig" >>> source "drivers/misc/pvpanic/Kconfig" >>> source "drivers/misc/mchp_pci1xxxx/Kconfig" >>> source "drivers/misc/keba/Kconfig" >>> +source "drivers/misc/rp1/Kconfig" >>> endmenu >>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile >>> index c2f990862d2b..84bfa866fbee 100644 >>> --- a/drivers/misc/Makefile >>> +++ b/drivers/misc/Makefile >>> @@ -71,3 +71,4 @@ obj-$(CONFIG_TPS6594_PFSM) += tps6594-pfsm.o >>> obj-$(CONFIG_NSM) += nsm.o >>> obj-$(CONFIG_MARVELL_CN10K_DPI) += mrvl_cn10k_dpi.o >>> obj-y += keba/ >>> +obj-$(CONFIG_MISC_RP1) += rp1/ >>> diff --git a/drivers/misc/rp1/Kconfig b/drivers/misc/rp1/Kconfig >>> new file mode 100644 >>> index 000000000000..050417ee09ae >>> --- /dev/null >>> +++ b/drivers/misc/rp1/Kconfig >>> @@ -0,0 +1,20 @@ >>> +# SPDX-License-Identifier: GPL-2.0-only >>> +# >>> +# RaspberryPi RP1 misc device >>> +# >>> + >>> +config MISC_RP1 >>> + tristate "RaspberryPi RP1 PCIe support" >>> + depends on PCI && PCI_QUIRKS >>> + select OF >>> + select OF_OVERLAY >>> + select IRQ_DOMAIN >>> + select PCI_DYNAMIC_OF_NODES >>> + help >>> + Support for the RP1 peripheral chip found on Raspberry Pi 5 board. >>> + This device supports several sub-devices including e.g. Ethernet controller, >>> + USB controller, I2C, SPI and UART. >>> + The driver is responsible for enabling the DT node once the PCIe endpoint >>> + has been configured, and handling interrupts. >>> + This driver uses an overlay to load other drivers to support for RP1 >>> + internal sub-devices. >>> diff --git a/drivers/misc/rp1/Makefile b/drivers/misc/rp1/Makefile >>> new file mode 100644 >>> index 000000000000..e83854b4ed2c >>> --- /dev/null >>> +++ b/drivers/misc/rp1/Makefile >>> @@ -0,0 +1,3 @@ >>> +# SPDX-License-Identifier: GPL-2.0-only >>> +rp1-pci-objs := rp1-pci.o rp1-pci.dtbo.o >>> +obj-$(CONFIG_MISC_RP1) += rp1-pci.o >>> diff --git a/drivers/misc/rp1/rp1-pci.c b/drivers/misc/rp1/rp1-pci.c >>> new file mode 100644 >>> index 000000000000..a6093ba7e19a >>> --- /dev/null >>> +++ b/drivers/misc/rp1/rp1-pci.c >>> @@ -0,0 +1,333 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Copyright (c) 2018-22 Raspberry Pi Ltd. >>> + * All rights reserved. >>> + */ >>> + >>> +#include <linux/clk.h> >>> +#include <linux/clkdev.h> >>> +#include <linux/clk-provider.h> >>> +#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 <linux/reset.h> >>> + >>> +#include <dt-bindings/misc/rp1.h> >>> + >>> +#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 RP1_ACTUAL_IRQS RP1_INT_END >>> +#define RP1_IRQS RP1_ACTUAL_IRQS >>> +#define RP1_HW_IRQ_MASK GENMASK(5, 0) >>> + >>> +#define RP1_SYSCLK_RATE 200000000 >>> +#define RP1_SYSCLK_FPGA_RATE 60000000 >>> + >>> +enum { >>> + SYSINFO_CHIP_ID_OFFSET = 0, >>> + SYSINFO_PLATFORM_OFFSET = 4, >>> +}; >>> + >>> +#define REG_SET 0x800 >>> +#define REG_CLR 0xc00 >>> + >>> +/* MSIX CFG registers start at 0x8 */ >>> +#define MSIX_CFG(x) (0x8 + (4 * (x))) >>> + >>> +#define MSIX_CFG_IACK_EN BIT(3) >>> +#define MSIX_CFG_IACK BIT(2) >>> +#define MSIX_CFG_TEST BIT(1) >>> +#define MSIX_CFG_ENABLE BIT(0) >>> + >>> +#define INTSTATL 0x108 >>> +#define INTSTATH 0x10c >>> + >>> +extern char __dtbo_rp1_pci_begin[]; >>> +extern char __dtbo_rp1_pci_end[]; >>> + >>> +struct rp1_dev { >>> + struct pci_dev *pdev; >>> + struct device *dev; >>> + struct clk *sys_clk; >>> + struct irq_domain *domain; >>> + struct irq_data *pcie_irqds[64]; >>> + void __iomem *bar1; >>> + int ovcs_id; >>> + bool level_triggered_irq[RP1_ACTUAL_IRQS]; >>> +}; >>> + >>> + >> ... >>> + >>> +static const struct pci_device_id dev_id_table[] = { >>> + { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RP1_C0), }, >>> + { 0, } >>> +}; >>> + >>> +static struct pci_driver rp1_driver = { >>> + .name = RP1_DRIVER_NAME, >>> + .id_table = dev_id_table, >>> + .probe = rp1_probe, >>> + .remove = rp1_remove, >>> +}; >>> + >>> +module_pci_driver(rp1_driver); >>> + >>> +MODULE_AUTHOR("Phil Elwell <phil@raspberrypi.com>"); >> Module author & Copyright doesn't seem to match with this patch author. >> Please clarify/fix > My intention here is that, even if the code has been heavily modified by me, > the core original code is still there so I just wanted to tribute it to the > original author. > I'll synchronize this with RaspberryPi guys and coem up with a unified solution. That would be nice to mention in the commit message and add your copyright. > Just in case: would multiple MODULE_AUTHOR entries (one with my name and one > with original authors name) be accepetd? Sure Best regards > > Many thanks, > Andrea > > References: > > - [1]: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ > - [2]: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ > - [3]: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ > - [4]: https://lore.kernel.org/all/ba8cdf39-3ba3-4abc-98f5-d394d6867f95@gmx.net/ > - [5]: https://lpc.events/event/17/contributions/1421/attachments/1337/2680/LPC2023%20Non-discoverable%20devices%20in%20PCI.pdf
Hi Stefan, On 12:23 Fri 23 Aug , Stefan Wahren wrote: > Hi Andrea, > > Am 23.08.24 um 11:44 schrieb Andrea della Porta: > > Hi Stefan, > > > > On 18:20 Wed 21 Aug , Stefan Wahren wrote: > > > Hi Andrea, > > > > > > Am 20.08.24 um 16:36 schrieb Andrea della Porta: > > > > The RaspberryPi RP1 is ia PCI multi function device containing > > > > peripherals ranging from Ethernet to USB controller, I2C, SPI > > > > and others. > > > sorry, i cannot provide you a code review, but just some comments. multi > > > function device suggests "mfd" subsystem or at least "soc" . I won't > > > recommend misc driver here. > > It's true that RP1 can be called an MFD but the reason for not placing > > it in mfd subsystem are twofold: > > > > - these discussions are quite clear about this matter: please see [1] > > and [2] > > - the current driver use no mfd API at all > > > > This RP1 driver is not currently addressing any aspect of ARM core in the > > SoC so I would say it should not stay in drivers/soc / either, as also > > condifirmed by [2] again and [3] (note that Microchip LAN966x is a very > > close fit to what we have here on RP1). > thanks i was aware of these discussions. A pointer to them or at least a > short statement in the cover letter would be great. Sure, consider it done. > > > > > > Implement a bare minimum driver to operate the RP1, leveraging > > > > actual OF based driver implementations for the on-borad peripherals > > > > by loading a devicetree overlay during driver probe. > > > Can you please explain why this should be a DT overlay? The RP1 is > > > assembled on the Raspberry Pi 5 PCB. DT overlays are typically for loose > > > connections like displays or HATs. I think a DTSI just for the RP1 would > > > fit better and is easier to read. > > The dtsi solution you proposed is the one adopted downstream. It has its > > benefits of course, but there's more. > > 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 be > > possibly be reused in other hw implementations. I believe a similar > > reasoning could be applied to Bootlin's Microchip LAN966x patchset as > > well, and they also choose to approach the dtb overlay. > Could please add this point in the commit message. Doesn't introduce Ack. > (maintainence) issues in case U-Boot needs a RP1 driver, too? Good point. Right now u-boot does not support RP1 nor PCIe (which is a prerequisite for RP1 to work) on Rpi5 and I'm quite sure that it will be so in the near future. Of course I cannot guarantee this will be the case far away in time. Since u-boot is lacking support for RP1 we cannot really produce some test results to check the compatibility versus kernel dtb overlay but we can speculate a little bit about it. AFAIK u-boot would probably place the rp1 node directly under its pcie@12000 node in DT while the dtb overlay will use dynamically created PCI endpoint node (dev@0) as parent for rp1 node. I would say it should work out of the box, the only minor drawback here should be the redundant rp1 node left from u-boot. And maybe some added checks to make sure the driver will be loaded only once from the dtb overlay and not from the u-boot node, but this change is locally to the RP1 linux driver code so it should not impact u-boot in any way. I'm inclined to consider the last one a minor issue. Please do keep in mind that, of course, this is just brainstorming and I cannot give 100% guarantee about that. > > Plus, a solution that can (althoguh proabbly in teh long run) cope > > with both DT or ACPI based system has been kindly requested, plase see [4] > > for details. > > IMHO the approach proposed from RH et al. of using dtbo for this 'special' > > kind of drivers makes a lot of sense (see [5]). > > > > > > The peripherals are accessed by mapping MMIO registers starting > > > > from PCI BAR1 region. > > > > As a minimum driver, the peripherals will not be added to the > > > > dtbo here, but in following patches. > > > > > > > > Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > > > --- > > > > MAINTAINERS | 2 + > > > > arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ > > > > drivers/misc/Kconfig | 1 + > > > > drivers/misc/Makefile | 1 + > > > > drivers/misc/rp1/Kconfig | 20 ++ > > > > drivers/misc/rp1/Makefile | 3 + > > > > drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ > > > > drivers/misc/rp1/rp1-pci.dtso | 8 + > > > > drivers/pci/quirks.c | 1 + > > > > include/linux/pci_ids.h | 3 + > > > > 10 files changed, 524 insertions(+) > > > > create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso > > > > create mode 100644 drivers/misc/rp1/Kconfig > > > > create mode 100644 drivers/misc/rp1/Makefile > > > > create mode 100644 drivers/misc/rp1/rp1-pci.c > > > > create mode 100644 drivers/misc/rp1/rp1-pci.dtso > > > > > > > ... > > > > + > > > > + rp1_clocks: clocks@c040018000 { > > > > + compatible = "raspberrypi,rp1-clocks"; > > > > + #clock-cells = <1>; > > > > + reg = <0xc0 0x40018000 0x0 0x10038>; > > > > + clocks = <&clk_xosc>; > > > > + clock-names = "xosc"; > > > > + > > > > + assigned-clocks = <&rp1_clocks RP1_PLL_SYS_CORE>, > > > > + <&rp1_clocks RP1_PLL_AUDIO_CORE>, > > > > + // RP1_PLL_VIDEO_CORE and dividers are now managed by VEC,DPI drivers > > > > + <&rp1_clocks RP1_PLL_SYS>, > > > > + <&rp1_clocks RP1_PLL_SYS_SEC>, > > > > + <&rp1_clocks RP1_PLL_SYS_PRI_PH>, > > > > + <&rp1_clocks RP1_CLK_ETH_TSU>; > > > > + > > > > + assigned-clock-rates = <1000000000>, // RP1_PLL_SYS_CORE > > > > + <1536000000>, // RP1_PLL_AUDIO_CORE > > > > + <200000000>, // RP1_PLL_SYS > > > > + <125000000>, // RP1_PLL_SYS_SEC > > > > + <100000000>, // RP1_PLL_SYS_PRI_PH > > > > + <50000000>; // RP1_CLK_ETH_TSU > > > > + }; > > > > + > > > > + rp1_gpio: pinctrl@c0400d0000 { > > > > + reg = <0xc0 0x400d0000 0x0 0xc000>, > > > > + <0xc0 0x400e0000 0x0 0xc000>, > > > > + <0xc0 0x400f0000 0x0 0xc000>; > > > > + compatible = "raspberrypi,rp1-gpio"; > > > > + gpio-controller; > > > > + #gpio-cells = <2>; > > > > + interrupt-controller; > > > > + #interrupt-cells = <2>; > > > > + interrupts = <RP1_INT_IO_BANK0 IRQ_TYPE_LEVEL_HIGH>, > > > > + <RP1_INT_IO_BANK1 IRQ_TYPE_LEVEL_HIGH>, > > > > + <RP1_INT_IO_BANK2 IRQ_TYPE_LEVEL_HIGH>; > > > > + gpio-line-names = > > > > + "ID_SDA", // GPIO0 > > > > + "ID_SCL", // GPIO1 > > > > + "GPIO2", // GPIO2 > > > > + "GPIO3", // GPIO3 > > > > + "GPIO4", // GPIO4 > > > > + "GPIO5", // GPIO5 > > > > + "GPIO6", // GPIO6 > > > > + "GPIO7", // GPIO7 > > > > + "GPIO8", // GPIO8 > > > > + "GPIO9", // GPIO9 > > > > + "GPIO10", // GPIO10 > > > > + "GPIO11", // GPIO11 > > > > + "GPIO12", // GPIO12 > > > > + "GPIO13", // GPIO13 > > > > + "GPIO14", // GPIO14 > > > > + "GPIO15", // GPIO15 > > > > + "GPIO16", // GPIO16 > > > > + "GPIO17", // GPIO17 > > > > + "GPIO18", // GPIO18 > > > > + "GPIO19", // GPIO19 > > > > + "GPIO20", // GPIO20 > > > > + "GPIO21", // GPIO21 > > > > + "GPIO22", // GPIO22 > > > > + "GPIO23", // GPIO23 > > > > + "GPIO24", // GPIO24 > > > > + "GPIO25", // GPIO25 > > > > + "GPIO26", // GPIO26 > > > > + "GPIO27", // GPIO27 > > > > + "PCIE_RP1_WAKE", // GPIO28 > > > > + "FAN_TACH", // GPIO29 > > > > + "HOST_SDA", // GPIO30 > > > > + "HOST_SCL", // GPIO31 > > > > + "ETH_RST_N", // GPIO32 > > > > + "", // GPIO33 > > > > + "CD0_IO0_MICCLK", // GPIO34 > > > > + "CD0_IO0_MICDAT0", // GPIO35 > > > > + "RP1_PCIE_CLKREQ_N", // GPIO36 > > > > + "", // GPIO37 > > > > + "CD0_SDA", // GPIO38 > > > > + "CD0_SCL", // GPIO39 > > > > + "CD1_SDA", // GPIO40 > > > > + "CD1_SCL", // GPIO41 > > > > + "USB_VBUS_EN", // GPIO42 > > > > + "USB_OC_N", // GPIO43 > > > > + "RP1_STAT_LED", // GPIO44 > > > > + "FAN_PWM", // GPIO45 > > > > + "CD1_IO0_MICCLK", // GPIO46 > > > > + "2712_WAKE", // GPIO47 > > > > + "CD1_IO1_MICDAT1", // GPIO48 > > > > + "EN_MAX_USB_CUR", // GPIO49 > > > > + "", // GPIO50 > > > > + "", // GPIO51 > > > > + "", // GPIO52 > > > > + ""; // GPIO53 > > > GPIO line names are board specific, so this should go to the Raspberry > > > Pi 5 file. > > Could we instead just name them with generic GPIO'N' where N is the number > > of the gpio? Much like many of that pins already are... in this way we > > don't add a dependency in the board dts to the rp1_gpio node, which is not > > even there when the main dts is parsed at boot, since the dtbo will be > > added only on PCI enumeration of the RP1 device. > I think we should avoid user space incompatibilities with the vendor tree. > > Or even better: since we don't explicitly use the gpio names to address > > them (e.g. phy-reset-gpios in rp1_eth node is addressing the ETH_RST_N > > gpio by number), can we just get rid of the gpio-line-names property? > > Also Bootlin's Microchip gpio node seems to avoid naming them... > As i said above the gpio lines are for user space, honestly nobody likes > to go to cryptic interfaces of gpiochips and gpio numbers. > You're right. > Maybe ETH_RST_N isn't good example because this not interesting from > user space. For example RP1_STAT_LED is a better one. Nobody can predict > the future use cases of the RP1 and its pins. So i think we should have > the flexibilty to specify the GPIOs on the board level for user > friendliness. > Agreed. > Isn't it possible to specify almost empty rp1 node with the gpio line > names for the RPi 5 and apply the rp1 overlay on top? Uhm, we can think of something like that, i.e. a secondary dtbo (populated with the gpio-line-names property only) to be added after the PCI enumeration has added the primary dtbo (i.e. the proposed rp1.dtso with gpio-line-names dropped) into devicetree. This implies loading this second dtbo from either: - the RP1 driver itself, since it's the one that is dynamically adding the RP1 node. I would say this is not the cleanest way unless we provide an elegant way to fed the customized dtbo to teh driver itself, but has the advantage that it would surely work and has no side effects that come to mind. - late at boot, directly from userspace. I see 2 problems here: 1) the gpio driver is already probed by the first dtbo so we should have a way to just add teh gpio names to the alredy existing one. Not sure how to accomplish that right now. 2) not sure whether how to prepare the dtbo, it should probably have an empty target-path since the dt parent tree is created dynamically and can be different for each system. This could be also helpful to customize things like the phy, that is external to teh ethernet MAC. I need to do some investigation, but would be helpful if you or others have some preference/objection on the two approaches above. > > > > > > + }; > > > > + }; > > > > + }; > > > > + }; > > > > +}; > > > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > > > > index 41c3d2821a78..02405209e6c4 100644 > > > > --- a/drivers/misc/Kconfig > > > > +++ b/drivers/misc/Kconfig > > > > @@ -618,4 +618,5 @@ source "drivers/misc/uacce/Kconfig" > > > > source "drivers/misc/pvpanic/Kconfig" > > > > source "drivers/misc/mchp_pci1xxxx/Kconfig" > > > > source "drivers/misc/keba/Kconfig" > > > > +source "drivers/misc/rp1/Kconfig" > > > > endmenu > > > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > > > > index c2f990862d2b..84bfa866fbee 100644 > > > > --- a/drivers/misc/Makefile > > > > +++ b/drivers/misc/Makefile > > > > @@ -71,3 +71,4 @@ obj-$(CONFIG_TPS6594_PFSM) += tps6594-pfsm.o > > > > obj-$(CONFIG_NSM) += nsm.o > > > > obj-$(CONFIG_MARVELL_CN10K_DPI) += mrvl_cn10k_dpi.o > > > > obj-y += keba/ > > > > +obj-$(CONFIG_MISC_RP1) += rp1/ > > > > diff --git a/drivers/misc/rp1/Kconfig b/drivers/misc/rp1/Kconfig > > > > new file mode 100644 > > > > index 000000000000..050417ee09ae > > > > --- /dev/null > > > > +++ b/drivers/misc/rp1/Kconfig > > > > @@ -0,0 +1,20 @@ > > > > +# SPDX-License-Identifier: GPL-2.0-only > > > > +# > > > > +# RaspberryPi RP1 misc device > > > > +# > > > > + > > > > +config MISC_RP1 > > > > + tristate "RaspberryPi RP1 PCIe support" > > > > + depends on PCI && PCI_QUIRKS > > > > + select OF > > > > + select OF_OVERLAY > > > > + select IRQ_DOMAIN > > > > + select PCI_DYNAMIC_OF_NODES > > > > + help > > > > + Support for the RP1 peripheral chip found on Raspberry Pi 5 board. > > > > + This device supports several sub-devices including e.g. Ethernet controller, > > > > + USB controller, I2C, SPI and UART. > > > > + The driver is responsible for enabling the DT node once the PCIe endpoint > > > > + has been configured, and handling interrupts. > > > > + This driver uses an overlay to load other drivers to support for RP1 > > > > + internal sub-devices. > > > > diff --git a/drivers/misc/rp1/Makefile b/drivers/misc/rp1/Makefile > > > > new file mode 100644 > > > > index 000000000000..e83854b4ed2c > > > > --- /dev/null > > > > +++ b/drivers/misc/rp1/Makefile > > > > @@ -0,0 +1,3 @@ > > > > +# SPDX-License-Identifier: GPL-2.0-only > > > > +rp1-pci-objs := rp1-pci.o rp1-pci.dtbo.o > > > > +obj-$(CONFIG_MISC_RP1) += rp1-pci.o > > > > diff --git a/drivers/misc/rp1/rp1-pci.c b/drivers/misc/rp1/rp1-pci.c > > > > new file mode 100644 > > > > index 000000000000..a6093ba7e19a > > > > --- /dev/null > > > > +++ b/drivers/misc/rp1/rp1-pci.c > > > > @@ -0,0 +1,333 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * Copyright (c) 2018-22 Raspberry Pi Ltd. > > > > + * All rights reserved. > > > > + */ > > > > + > > > > +#include <linux/clk.h> > > > > +#include <linux/clkdev.h> > > > > +#include <linux/clk-provider.h> > > > > +#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 <linux/reset.h> > > > > + > > > > +#include <dt-bindings/misc/rp1.h> > > > > + > > > > +#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 RP1_ACTUAL_IRQS RP1_INT_END > > > > +#define RP1_IRQS RP1_ACTUAL_IRQS > > > > +#define RP1_HW_IRQ_MASK GENMASK(5, 0) > > > > + > > > > +#define RP1_SYSCLK_RATE 200000000 > > > > +#define RP1_SYSCLK_FPGA_RATE 60000000 > > > > + > > > > +enum { > > > > + SYSINFO_CHIP_ID_OFFSET = 0, > > > > + SYSINFO_PLATFORM_OFFSET = 4, > > > > +}; > > > > + > > > > +#define REG_SET 0x800 > > > > +#define REG_CLR 0xc00 > > > > + > > > > +/* MSIX CFG registers start at 0x8 */ > > > > +#define MSIX_CFG(x) (0x8 + (4 * (x))) > > > > + > > > > +#define MSIX_CFG_IACK_EN BIT(3) > > > > +#define MSIX_CFG_IACK BIT(2) > > > > +#define MSIX_CFG_TEST BIT(1) > > > > +#define MSIX_CFG_ENABLE BIT(0) > > > > + > > > > +#define INTSTATL 0x108 > > > > +#define INTSTATH 0x10c > > > > + > > > > +extern char __dtbo_rp1_pci_begin[]; > > > > +extern char __dtbo_rp1_pci_end[]; > > > > + > > > > +struct rp1_dev { > > > > + struct pci_dev *pdev; > > > > + struct device *dev; > > > > + struct clk *sys_clk; > > > > + struct irq_domain *domain; > > > > + struct irq_data *pcie_irqds[64]; > > > > + void __iomem *bar1; > > > > + int ovcs_id; > > > > + bool level_triggered_irq[RP1_ACTUAL_IRQS]; > > > > +}; > > > > + > > > > + > > > ... > > > > + > > > > +static const struct pci_device_id dev_id_table[] = { > > > > + { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RP1_C0), }, > > > > + { 0, } > > > > +}; > > > > + > > > > +static struct pci_driver rp1_driver = { > > > > + .name = RP1_DRIVER_NAME, > > > > + .id_table = dev_id_table, > > > > + .probe = rp1_probe, > > > > + .remove = rp1_remove, > > > > +}; > > > > + > > > > +module_pci_driver(rp1_driver); > > > > + > > > > +MODULE_AUTHOR("Phil Elwell <phil@raspberrypi.com>"); > > > Module author & Copyright doesn't seem to match with this patch author. > > > Please clarify/fix > > My intention here is that, even if the code has been heavily modified by me, > > the core original code is still there so I just wanted to tribute it to the > > original author. > > I'll synchronize this with RaspberryPi guys and coem up with a unified solution. > That would be nice to mention in the commit message and add your copyright. Ack. > > Just in case: would multiple MODULE_AUTHOR entries (one with my name and one > > with original authors name) be accepetd? > Sure Many thanks, Andrea > > Best regards > > > > Many thanks, > > Andrea > > > > References: > > > > - [1]: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ > > - [2]: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ > > - [3]: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ > > - [4]: https://lore.kernel.org/all/ba8cdf39-3ba3-4abc-98f5-d394d6867f95@gmx.net/ > > - [5]: https://lpc.events/event/17/contributions/1421/attachments/1337/2680/LPC2023%20Non-discoverable%20devices%20in%20PCI.pdf >
On 14:27 Wed 21 Aug , Simon Horman wrote: > On Tue, Aug 20, 2024 at 04:36:09PM +0200, Andrea della Porta wrote: > > The RP1 is an MFD supporting a gpio controller and /pinmux/pinctrl. > > Add minimum support for the gpio only portion. The driver is in > > pinctrl folder since upcoming patches will add the pinmux/pinctrl > > support where the gpio part can be seen as an addition. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > ... > > > diff --git a/drivers/pinctrl/pinctrl-rp1.c b/drivers/pinctrl/pinctrl-rp1.c > > ... > > > +const struct rp1_iobank_desc rp1_iobanks[RP1_NUM_BANKS] = { > > + /* gpio inte ints rio pads */ > > + { 0, 28, 0x0000, 0x011c, 0x0124, 0x0000, 0x0004 }, > > + { 28, 6, 0x4000, 0x411c, 0x4124, 0x4000, 0x4004 }, > > + { 34, 20, 0x8000, 0x811c, 0x8124, 0x8000, 0x8004 }, > > +}; > > rp1_iobanks seems to only be used in this file. > If so, it should be static. Fixed, thanks. > > Flagged by Sparse. > > ...
On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -2610,6 +2610,9 @@ > #define PCI_VENDOR_ID_TEKRAM 0x1de1 > #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 > > +#define PCI_VENDOR_ID_RPI 0x1de4 > +#define PCI_DEVICE_ID_RP1_C0 0x0001 Minor thing, but please read the top of this file. As you aren't using these values anywhere outside of this one driver, there's no need to add these values to pci_ids.h. Just keep them local to the .c file itself. thanks, greg k-h
Hi Andrea, thanks for your patch! On Tue, Aug 20, 2024 at 4:36 PM Andrea della Porta <andrea.porta@suse.com> wrote: > The RP1 is an MFD supporting a gpio controller and /pinmux/pinctrl. > Add minimum support for the gpio only portion. The driver is in > pinctrl folder since upcoming patches will add the pinmux/pinctrl > support where the gpio part can be seen as an addition. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> (...) > +#include <linux/bitmap.h> > +#include <linux/bitops.h> (...) > +static void rp1_pad_update(struct rp1_pin_info *pin, u32 clr, u32 set) > +{ > + u32 padctrl = readl(pin->pad); > + > + padctrl &= ~clr; > + padctrl |= set; > + > + writel(padctrl, pin->pad); > +} Looks a bit like a reimplementation of regmap-mmio? If you want to do this why not use regmap-mmio? > +static void rp1_set_dir(struct rp1_pin_info *pin, bool is_input) > +{ > + int offset = is_input ? RP1_CLR_OFFSET : RP1_SET_OFFSET; > + > + writel(1 << pin->offset, pin->rio + RP1_RIO_OE + offset); If you include bitops.h what about: writel(BIT(pin->offset), pin->rio + RP1_RIO_OE + offset); > +static int rp1_get_value(struct rp1_pin_info *pin) > +{ > + return !!(readl(pin->rio + RP1_RIO_IN) & (1 << pin->offset)); > +} Also here > + > +static void rp1_set_value(struct rp1_pin_info *pin, int value) > +{ > + /* Assume the pin is already an output */ > + writel(1 << pin->offset, > + pin->rio + RP1_RIO_OUT + (value ? RP1_SET_OFFSET : RP1_CLR_OFFSET)); > +} And here > +static int rp1_gpio_set_config(struct gpio_chip *chip, unsigned int offset, > + unsigned long config) > +{ > + struct rp1_pin_info *pin = rp1_get_pin(chip, offset); > + unsigned long configs[] = { config }; > + > + return rp1_pinconf_set(pin, offset, configs, > + ARRAY_SIZE(configs)); > +} Nice that you implement this! > +static void rp1_gpio_irq_config(struct rp1_pin_info *pin, bool enable) > +{ > + writel(1 << pin->offset, > + pin->inte + (enable ? RP1_SET_OFFSET : RP1_CLR_OFFSET)); BIT() Yours, Linus Walleij
Hi Greg, On 09:53 Sat 24 Aug , Greg Kroah-Hartman wrote: > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > --- a/include/linux/pci_ids.h > > +++ b/include/linux/pci_ids.h > > @@ -2610,6 +2610,9 @@ > > #define PCI_VENDOR_ID_TEKRAM 0x1de1 > > #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 > > > > +#define PCI_VENDOR_ID_RPI 0x1de4 > > +#define PCI_DEVICE_ID_RP1_C0 0x0001 > > Minor thing, but please read the top of this file. As you aren't using > these values anywhere outside of this one driver, there's no need to add > these values to pci_ids.h. Just keep them local to the .c file itself. > Thanks, I've read the top part of that file. The reason I've declared those two macroes in pci_ids.h is that I'm using them both in the main driver (rp1-pci.c) and in drivers/pci/quirks.c. I suppose I could move DECLARE_PCI_FIXUP_FINAL() inside rp1-pci.c to keep those two defines local, but judging from the number of entries of DECLARE_PCI_FIXP_FINAL found in quirks.c versus the occurences found in respective driver, I assumed the preferred way was to place it in quirks.c. Many thanks, Andrea > thanks, > > greg k-h
On Mon, Aug 26, 2024 at 11:07:33AM +0200, Andrea della Porta wrote: > Hi Greg, > > On 09:53 Sat 24 Aug , Greg Kroah-Hartman wrote: > > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > > --- a/include/linux/pci_ids.h > > > +++ b/include/linux/pci_ids.h > > > @@ -2610,6 +2610,9 @@ > > > #define PCI_VENDOR_ID_TEKRAM 0x1de1 > > > #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 > > > > > > +#define PCI_VENDOR_ID_RPI 0x1de4 > > > +#define PCI_DEVICE_ID_RP1_C0 0x0001 > > > > Minor thing, but please read the top of this file. As you aren't using > > these values anywhere outside of this one driver, there's no need to add > > these values to pci_ids.h. Just keep them local to the .c file itself. > > > > Thanks, I've read the top part of that file. The reason I've declared those > two macroes in pci_ids.h is that I'm using them both in the > main driver (rp1-pci.c) and in drivers/pci/quirks.c. Ah, missed that, sorry, nevermind. greg k-h
Hi Florian, On 10:01 Wed 21 Aug , Florian Fainelli wrote: > On 8/20/24 07:36, Andrea della Porta wrote: > > RaspberryPi RP1 contains Cadence's MACB core. Implement the > > changes to be able to operate the customization in the RP1. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > You are doing a lot of things, all at once, and you should consider > extracting your change into a smaller subset with bug fixes first: > > - one commit which writes to the RBQPH the upper 32-bits of the RX ring DMA > address, that looks like a bug fix > > - one commit which retriggers a buffer read, even though that appears to be > RP1 specific maybe, if not, then this is also a bug fix > > - one commit that adds support for macb_shutdown() to kill DMA operations > > - one commit which adds support for a configurable PHY reset line + delay > specified in milli seconds > > - one commit which adds support for controling the interrupt coalescing > settings > > And then you can add all of the RP1 specific bits like the AXI bridge > configuration. > > [snip] > > > @@ -1228,6 +1246,7 @@ struct macb_queue { > > dma_addr_t tx_ring_dma; > > struct work_struct tx_error_task; > > bool txubr_pending; > > + bool tx_pending; > > struct napi_struct napi_tx; > > dma_addr_t rx_ring_dma; > > @@ -1293,9 +1312,15 @@ struct macb { > > u32 caps; > > unsigned int dma_burst_length; > > + u8 aw2w_max_pipe; > > + u8 ar2r_max_pipe; > > + bool use_aw2b_fill; > > phy_interface_t phy_interface; > > + struct gpio_desc *phy_reset_gpio; > > + int phy_reset_ms; > > The delay cannot be negative, so this needs to be unsigned int. > > > + > > /* AT91RM9200 transmit queue (1 on wire + 1 queued) */ > > struct macb_tx_skb rm9200_txq[2]; > > unsigned int max_tx_length; > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > > index 11665be3a22c..5eb5be6c96fc 100644 > > --- a/drivers/net/ethernet/cadence/macb_main.c > > +++ b/drivers/net/ethernet/cadence/macb_main.c > > @@ -41,6 +41,9 @@ > > #include <linux/inetdevice.h> > > #include "macb.h" > > +static unsigned int txdelay = 35; > > +module_param(txdelay, uint, 0644); > > + > > /* This structure is only used for MACB on SiFive FU540 devices */ > > struct sifive_fu540_macb_mgmt { > > void __iomem *reg; > > @@ -334,7 +337,7 @@ static int macb_mdio_wait_for_idle(struct macb *bp) > > u32 val; > > return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_BIT(IDLE), > > - 1, MACB_MDIO_TIMEOUT); > > + 100, MACB_MDIO_TIMEOUT); > > Why do we need to increase how frequently we poll? Thanks for your feedback, I will save all your precious suggestions for a future patch that will enable the macb ethernet. As stated in the cover letter, right now this specific patch is not intended to be upstreamed as is but it's just here for testing purposes, hence its 'raw' state. For sure the ethernet contained in RP1 will be one of the first device I will try to bring upstream, so I'll apply your comments there. Maybe the next time I will also add a better explanation about the state of a specific patch in the commit comment itself, and not only in the cover letter, just to be more explicit. Many thanks, Andrea > -- > Florian >
Hi Florian, On 10:02 Wed 21 Aug , Florian Fainelli wrote: > On 8/20/24 07:36, Andrea della Porta wrote: > > RaspberryPi RP1 is multi function PCI endpoint device that > > exposes several subperipherals via PCI BAR. > > Add an ethernet node for Cadence MACB to the RP1 dtso > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > arch/arm64/boot/dts/broadcom/rp1.dtso | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > > index d80178a278ee..b40e203c28d5 100644 > > --- a/arch/arm64/boot/dts/broadcom/rp1.dtso > > +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > > @@ -78,6 +78,29 @@ rp1_clocks: clocks@c040018000 { > > <50000000>; // RP1_CLK_ETH_TSU > > }; > > + rp1_eth: ethernet@c040100000 { > > + reg = <0xc0 0x40100000 0x0 0x4000>; > > + compatible = "cdns,macb"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + interrupts = <RP1_INT_ETH IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&macb_pclk &macb_hclk &rp1_clocks RP1_CLK_ETH_TSU>; > > + clock-names = "pclk", "hclk", "tsu_clk"; > > + phy-mode = "rgmii-id"; > > + cdns,aw2w-max-pipe = /bits/ 8 <8>; > > + cdns,ar2r-max-pipe = /bits/ 8 <8>; > > + cdns,use-aw2b-fill; > > + local-mac-address = [00 00 00 00 00 00]; > > + phy-handle = <&phy1>; > > + phy-reset-gpios = <&rp1_gpio 32 GPIO_ACTIVE_LOW>; > > + phy-reset-duration = <5>; > > + > > + phy1: ethernet-phy@1 { > > + reg = <0x1>; > > + brcm,powerdown-enable; > > Undocumented property, and I would like to understand why this needs to be > specified in the Device Tree? What model of Broadcom Ethernet PHY is being > used here? It's a Broadcom BCM5421 transceiver, and that property is intended to support the optional link-down powersave from DT. It will require slight changes in drivers/net/phy/broadcom.c too and is not really necessary for minimal support, so I will drop it in the next iteration. Many thanks, Andrea > -- > Florian >
Hi Linus, On 10:59 Mon 26 Aug , Linus Walleij wrote: > Hi Andrea, > > thanks for your patch! Thanks for your review! > > On Tue, Aug 20, 2024 at 4:36 PM Andrea della Porta > <andrea.porta@suse.com> wrote: > > > The RP1 is an MFD supporting a gpio controller and /pinmux/pinctrl. > > Add minimum support for the gpio only portion. The driver is in > > pinctrl folder since upcoming patches will add the pinmux/pinctrl > > support where the gpio part can be seen as an addition. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > (...) > > > +#include <linux/bitmap.h> > > +#include <linux/bitops.h> > (...) > > > +static void rp1_pad_update(struct rp1_pin_info *pin, u32 clr, u32 set) > > +{ > > + u32 padctrl = readl(pin->pad); > > + > > + padctrl &= ~clr; > > + padctrl |= set; > > + > > + writel(padctrl, pin->pad); > > +} > > Looks a bit like a reimplementation of regmap-mmio? If you want to do > this why not use regmap-mmio? Agreed. I can leverage regmail_field to get rid of the reimplemented code for the pin->pad register region. Do you think it could be worth using regmap-mmio also on pin->gpio, pin->inte, pin->ints and pin->rio even though they are not doing any special field manipulation as the pin->pad case? > > > +static void rp1_set_dir(struct rp1_pin_info *pin, bool is_input) > > +{ > > + int offset = is_input ? RP1_CLR_OFFSET : RP1_SET_OFFSET; > > + > > + writel(1 << pin->offset, pin->rio + RP1_RIO_OE + offset); > > If you include bitops.h what about: > > writel(BIT(pin->offset), pin->rio + RP1_RIO_OE + offset); Ack. > > > +static int rp1_get_value(struct rp1_pin_info *pin) > > +{ > > + return !!(readl(pin->rio + RP1_RIO_IN) & (1 << pin->offset)); > > +} > > Also here Ack. > > > + > > +static void rp1_set_value(struct rp1_pin_info *pin, int value) > > +{ > > + /* Assume the pin is already an output */ > > + writel(1 << pin->offset, > > + pin->rio + RP1_RIO_OUT + (value ? RP1_SET_OFFSET : RP1_CLR_OFFSET)); > > +} > > And here Ack. > > > +static int rp1_gpio_set_config(struct gpio_chip *chip, unsigned int offset, > > + unsigned long config) > > +{ > > + struct rp1_pin_info *pin = rp1_get_pin(chip, offset); > > + unsigned long configs[] = { config }; > > + > > + return rp1_pinconf_set(pin, offset, configs, > > + ARRAY_SIZE(configs)); > > +} > > Nice that you implement this! Thanks :) > > > +static void rp1_gpio_irq_config(struct rp1_pin_info *pin, bool enable) > > +{ > > + writel(1 << pin->offset, > > + pin->inte + (enable ? RP1_SET_OFFSET : RP1_CLR_OFFSET)); > > BIT() Ack. Many thanks, Andrea > > Yours, > Linus Walleij
Hi Andrew, On 15:04 Thu 22 Aug , Andrew Lunn wrote: > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > #673: FILE: drivers/pinctrl/pinctrl-rp1.c:600: > > + return -ENOTSUPP; > > > > This I must investigate: I've already tried to fix it before sending the patchset > > but for some reason it wouldn't work, so I planned to fix it in the upcoming > > releases. > > ENOTSUPP is an NFS error. It should not be used outside for NFS. You > want EOPNOTSUPP. Ack. > > > > WARNING: externs should be avoided in .c files > > #331: FILE: drivers/misc/rp1/rp1-pci.c:58: > > +extern char __dtbo_rp1_pci_begin[]; > > > > True, but in this case we don't have a symbol that should be exported to other > > translation units, it just needs to be referenced inside the driver and > > consumed locally. Hence it would be better to place the extern in .c file. > > Did you try making it static. The dtso is compiled into an obj and linked with the driver which is in a different transaltion unit. I'm not aware on other ways to include that symbol without declaring it extern (the exception being some hackery trick that compile the dtso into a .c file to be included into the driver main source file). Or probably I'm not seeing what you are proposing, could you please elaborate on that? Many thanks, Andrea > > Andrew
> > > WARNING: externs should be avoided in .c files > > > #331: FILE: drivers/misc/rp1/rp1-pci.c:58: > > > +extern char __dtbo_rp1_pci_begin[]; > > > > > > True, but in this case we don't have a symbol that should be exported to other > > > translation units, it just needs to be referenced inside the driver and > > > consumed locally. Hence it would be better to place the extern in .c file. > > > > Did you try making it static. > > The dtso is compiled into an obj and linked with the driver which is in > a different transaltion unit. I'm not aware on other ways to include that > symbol without declaring it extern (the exception being some hackery > trick that compile the dtso into a .c file to be included into the driver > main source file). > Or probably I'm not seeing what you are proposing, could you please elaborate > on that? Sorry, i jumped to the wrong conclusion. Often it is missing static keyword which causes warnings. However, you say it needs to be global scope. Reading the warning again: > > > WARNING: externs should be avoided in .c files It is wanting you to put it in a .h file, which then gets included by the two users. Andrew
Hi Krzysztof, On 11:50 Thu 22 Aug , Krzysztof Kozlowski wrote: > On 22/08/2024 11:05, Andrea della Porta wrote: > > Hi Krzysztof, > > > > On 15:42 Wed 21 Aug , Krzysztof Kozlowski wrote: > >> On 20/08/2024 16:36, Andrea della Porta wrote: > >>> RP1 is an MFD chipset that acts as a south-bridge PCIe endpoint sporting > >>> a pletora of subdevices (i.e. Ethernet, USB host controller, I2C, PWM, > >>> etc.) whose registers are all reachable starting from an offset from the > >>> BAR address. The main point here is that while the RP1 as an endpoint > >>> itself is discoverable via usual PCI enumeraiton, the devices it contains > >>> are not discoverable and must be declared e.g. via the devicetree. > >>> > >>> This patchset is an attempt to provide a minimum infrastructure to allow > >>> the RP1 chipset to be discovered and perpherals it contains to be added > >>> from a devictree overlay loaded during RP1 PCI endpoint enumeration. > >>> Followup patches should add support for the several peripherals contained > >>> in RP1. > >>> > >>> This work is based upon dowstream drivers code and the proposal from RH > >>> et al. (see [1] and [2]). A similar approach is also pursued in [3]. > >> > >> Looking briefly at findings it seems this was not really tested by > >> automation and you expect reviewers to find issues which are pointed out > >> by tools. That's not nice approach. Reviewer's time is limited, while > >> tools do it for free. And the tools are free - you can use them without > >> any effort. > > > > Sorry if I gave you that impression, but this is not obviously the case. > > Just look at number of reports... so many sparse reports that I wonder > how it is not the case. > > And many kbuild reports. Ack. > > > I've spent quite a bit of time in trying to deliver a patchset that ease > > your and others work, at least to the best I can. In fact, I've used many > > of the checking facilities you mentioned before sending it, solving all > > of the reported issues, except the ones for which there are strong reasons > > to leave untouched, as explained below. > > > >> > >> It does not look like you tested the DTS against bindings. Please run > >> `make dtbs_check W=1` (see > >> Documentation/devicetree/bindings/writing-schema.rst or > >> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > >> for instructions). > > > > #> make W=1 dt_binding_check DT_SCHEMA_FILES=raspberrypi,rp1-gpio.yaml > > CHKDT Documentation/devicetree/bindings > > LINT Documentation/devicetree/bindings > > DTEX Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.example.dts > > DTC_CHK Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.example.dtb > > > > #> make W=1 dt_binding_check DT_SCHEMA_FILES=raspberrypi,rp1-clocks.yaml > > CHKDT Documentation/devicetree/bindings > > LINT Documentation/devicetree/bindings > > DTEX Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.example.dts > > DTC_CHK Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.example.dtb > > > > I see no issues here, in case you've found something different, I kindly ask you to post > > the results. > > > > #> make W=1 CHECK_DTBS=y broadcom/rp1.dtbo > > DTC arch/arm64/boot/dts/broadcom/rp1.dtbo > > arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property > > arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property > > arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property > > arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property > > > > I believe that These warnings are unavoidable, and stem from the fact that this > > is quite a peculiar setup (PCI endpoint which dynamically loads platform driver > > addressable via BAR). > > The missing reg/ranges in the threee clocks are due to the simple-bus of the > > containing node to which I believe they should belong: I did a test to place > > This is not the place where they belong. non-MMIO nodes should not be > under simple-bus. Ack. > > > those clocks in the same dtso under root or /clocks node but AFAIK it doesn't > > seems to work. I could move them in a separate dtso to be loaded before the main > > Well... who instantiates them? If they are in top-level, then > CLK_OF_DECLARE which is not called at your point? > > You must instantiate clocks different way, since they are not part of > "rp1". That's another bogus DT description... external oscilator is not > part of RP1. > Ok, I'll dive into that and see what I can come up with. Many thanks for this feedback. > > > one but this is IMHO even more cumbersome than having a couple of warnings in > > CHECK_DTBS. > > Of course, if you have any suggestion on how to improve it I would be glad to > > discuss. > > About the last warning about the address/size-cells, if I drop those two lines > > in the _overlay_ node it generates even more warning, so again it's a "don't fix" > > one. > > > >> > >> Please run standard kernel tools for static analysis, like coccinelle, > >> smatch and sparse, and fix reported warnings. Also please check for > >> warnings when building with W=1. Most of these commands (checks or W=1 > >> build) can build specific targets, like some directory, to narrow the > >> scope to only your code. The code here looks like it needs a fix. Feel > >> free to get in touch if the warning is not clear. > > > > I didn't run those static analyzers since I've preferred a more "manual" aproach > > by carfeully checking the code, but I agree that something can escape even the > > more carefully executed code inspection so I will add them to my arsenal from > > now on. Thanks for the heads up. > > I don't care if you do not run static analyzers if you produce good > code. But if you produce bugs which could have been easily spotted with > sparser, than it is different thing. > > Start running static checkers instead of asking reviewers to do that. Ack. > > > > >> > >> Please run scripts/checkpatch.pl and fix reported warnings. Then please > >> run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. > >> Some warnings can be ignored, especially from --strict run, but the code > >> here looks like it needs a fix. Feel free to get in touch if the warning > >> is not clear. > >> > > > > Again, most of checkpatch's complaints have been addressed, the remaining > > ones I deemed as not worth fixing, for example:> > > #> scripts/checkpatch.pl --strict --codespell tmp/*.patch > > > > WARNING: please write a help paragraph that fully describes the config symbol > > #42: FILE: drivers/clk/Kconfig:91: > > +config COMMON_CLK_RP1 > > + tristate "Raspberry Pi RP1-based clock support" > > + depends on PCI || COMPILE_TEST > > + depends on COMMON_CLK > > + help > > + Enable common clock framework support for Raspberry Pi RP1. > > + This mutli-function device has 3 main PLLs and several clock > > + generators to drive the internal sub-peripherals. > > + > > > > I don't understand this warning, the paragraph is there and is more or less similar > > to many in the same file that are already upstream. Checkpatch bug? > > > > > > CHECK: Alignment should match open parenthesis > > #1541: FILE: drivers/clk/clk-rp1.c:1470: > > + if (WARN_ON_ONCE(clock_data->num_std_parents > AUX_SEL && > > + strcmp("-", clock_data->parents[AUX_SEL]))) > > > > This would have worsen the code readability. > > > > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > #673: FILE: drivers/pinctrl/pinctrl-rp1.c:600: > > + return -ENOTSUPP; > > > > This I must investigate: I've already tried to fix it before sending the patchset > > but for some reason it wouldn't work, so I planned to fix it in the upcoming > > releases. > > > > > > WARNING: externs should be avoided in .c files > > #331: FILE: drivers/misc/rp1/rp1-pci.c:58: > > +extern char __dtbo_rp1_pci_begin[]; > > > > True, but in this case we don't have a symbol that should be exported to other > > translation units, it just needs to be referenced inside the driver and > > consumed locally. Hence it would be better to place the extern in .c file. > > > > > > Apologies for a couple of other warnings that I could have seen in the first > > place, but honestly they don't seems to be a big deal (one typo and on over > > 100 chars comment, that will be fixed in next patch version). > > Again, judging by number of reports from checkers that is a big deal, > because it is your task to run the tools. Ack. Many thanks, Andrea > > Best regards, > Krzysztof >
Hi Andrew, On 15:04 Thu 29 Aug , Andrew Lunn wrote: > > > > WARNING: externs should be avoided in .c files > > > > #331: FILE: drivers/misc/rp1/rp1-pci.c:58: > > > > +extern char __dtbo_rp1_pci_begin[]; > > > > > > > > True, but in this case we don't have a symbol that should be exported to other > > > > translation units, it just needs to be referenced inside the driver and > > > > consumed locally. Hence it would be better to place the extern in .c file. > > > > > > Did you try making it static. > > > > The dtso is compiled into an obj and linked with the driver which is in > > a different transaltion unit. I'm not aware on other ways to include that > > symbol without declaring it extern (the exception being some hackery > > trick that compile the dtso into a .c file to be included into the driver > > main source file). > > Or probably I'm not seeing what you are proposing, could you please elaborate > > on that? > > Sorry, i jumped to the wrong conclusion. Often it is missing static > keyword which causes warnings. However, you say it needs to be global > scope. > > Reading the warning again: > > > > > WARNING: externs should be avoided in .c files > > It is wanting you to put it in a .h file, which then gets > included by the two users. Ah I see now what you were referring to, thanks. I'll put the extern into an header file, although there are no two users of that, the only one being rp1-pci.c. Many thanks, Andrea > > Andrew
Hi Andrew, On 15:04 Thu 29 Aug , Andrew Lunn wrote: > > > > WARNING: externs should be avoided in .c files > > > > #331: FILE: drivers/misc/rp1/rp1-pci.c:58: > > > > +extern char __dtbo_rp1_pci_begin[]; > > > > > > > > True, but in this case we don't have a symbol that should be exported to other > > > > translation units, it just needs to be referenced inside the driver and > > > > consumed locally. Hence it would be better to place the extern in .c file. > > > > > > Did you try making it static. > > > > The dtso is compiled into an obj and linked with the driver which is in > > a different transaltion unit. I'm not aware on other ways to include that > > symbol without declaring it extern (the exception being some hackery > > trick that compile the dtso into a .c file to be included into the driver > > main source file). > > Or probably I'm not seeing what you are proposing, could you please elaborate > > on that? > > Sorry, i jumped to the wrong conclusion. Often it is missing static > keyword which causes warnings. However, you say it needs to be global > scope. > > Reading the warning again: > > > > > WARNING: externs should be avoided in .c files > > It is wanting you to put it in a .h file, which then gets > included by the two users. On a second thought, are you really sure we want to proceed with the header file? After all the only line in it would be the extern declaration and the only one to include it would be rp1-dev.c. Moreover, an header file would convey the false premise that you can include it and use that symbol while in fact it should be only used inside the driver. OTOH, not creating that header file will continue to trigger the warning... Many thanks, Andrea > > Andrew
Hi Krzysztof, On 10:45 Wed 21 Aug , Krzysztof Kozlowski wrote: > On Tue, Aug 20, 2024 at 04:36:09PM +0200, Andrea della Porta wrote: > > The RP1 is an MFD supporting a gpio controller and /pinmux/pinctrl. > > Add minimum support for the gpio only portion. The driver is in > > pinctrl folder since upcoming patches will add the pinmux/pinctrl > > support where the gpio part can be seen as an addition. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > MAINTAINERS | 1 + > > drivers/pinctrl/Kconfig | 10 + > > drivers/pinctrl/Makefile | 1 + > > drivers/pinctrl/pinctrl-rp1.c | 719 ++++++++++++++++++++++++++++++++++ > > 4 files changed, 731 insertions(+) > > create mode 100644 drivers/pinctrl/pinctrl-rp1.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 4ce7b049d67e..67f460c36ea1 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -19122,6 +19122,7 @@ S: Maintained > > F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > > F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml > > F: drivers/clk/clk-rp1.c > > +F: drivers/pinctrl/pinctrl-rp1.c > > F: include/dt-bindings/clock/rp1.h > > F: include/dt-bindings/misc/rp1.h > > > > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > > index 7e4f93a3bc7a..18bb1a8bd102 100644 > > --- a/drivers/pinctrl/Kconfig > > +++ b/drivers/pinctrl/Kconfig > > @@ -565,6 +565,16 @@ config PINCTRL_MLXBF3 > > each pin. This driver can also be built as a module called > > pinctrl-mlxbf3. > > > > +config PINCTRL_RP1 > > + bool "Pinctrl driver for RP1" > > + select PINMUX > > + select PINCONF > > + select GENERIC_PINCONF > > + select GPIOLIB_IRQCHIP > > + help > > + Enable the gpio and pinctrl/mux driver for RaspberryPi RP1 > > + multi function device. > > + > > source "drivers/pinctrl/actions/Kconfig" > > source "drivers/pinctrl/aspeed/Kconfig" > > source "drivers/pinctrl/bcm/Kconfig" > > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > > index cc809669405a..f1ca23b563f6 100644 > > --- a/drivers/pinctrl/Makefile > > +++ b/drivers/pinctrl/Makefile > > @@ -45,6 +45,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o > > obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o > > obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o > > obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o > > +obj-$(CONFIG_PINCTRL_RP1) += pinctrl-rp1.o > > obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o > > obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o > > obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o > > diff --git a/drivers/pinctrl/pinctrl-rp1.c b/drivers/pinctrl/pinctrl-rp1.c > > new file mode 100644 > > index 000000000000..c035d2014505 > > --- /dev/null > > +++ b/drivers/pinctrl/pinctrl-rp1.c > > @@ -0,0 +1,719 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Driver for Raspberry Pi RP1 GPIO unit > > + * > > + * Copyright (C) 2023 Raspberry Pi Ltd. > > + * > > + * This driver is inspired by: > > + * pinctrl-bcm2835.c, please see original file for copyright information > > + */ > > + > > +#include <linux/bitmap.h> > > +#include <linux/bitops.h> > > +#include <linux/bug.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/err.h> > > +#include <linux/gpio/driver.h> > > +#include <linux/io.h> > > +#include <linux/irq.h> > > +#include <linux/irqdesc.h> > > +#include <linux/init.h> > > +#include <linux/of_address.h> > > +#include <linux/of.h> > > +#include <linux/of_irq.h> > > +#include <linux/platform_device.h> > > +#include <linux/seq_file.h> > > +#include <linux/spinlock.h> > > +#include <linux/types.h> > > Half of these headers are not used. Drop them. Ack. Many thanks, Andrea > > Best regards, > Krzysztof >
Hi Krzysztof, On 10:38 Wed 21 Aug , Krzysztof Kozlowski wrote: > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > The RaspberryPi RP1 is ia 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-borad peripherals > > by loading a devicetree overlay during driver probe. > > The peripherals are accessed by mapping MMIO registers starting > > from PCI BAR1 region. > > As a minimum driver, the peripherals will not be added to the > > dtbo here, but in following patches. > > > > Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > MAINTAINERS | 2 + > > arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ > > Do not mix DTS with drivers. > > These MUST be separate. Separating the dtso from the driver in two different patches would mean that the dtso patch would be ordered before the driver one. This is because the driver embeds the dtbo binary blob inside itself, at build time. So in order to build the driver, the dtso needs to be there also. This is not the standard approach used with 'normal' dtb/dtbo, where the dtb patch is ordered last wrt the driver it refers to. Are you sure you want to proceed in this way? > > > drivers/misc/Kconfig | 1 + > > drivers/misc/Makefile | 1 + > > drivers/misc/rp1/Kconfig | 20 ++ > > drivers/misc/rp1/Makefile | 3 + > > drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ > > drivers/misc/rp1/rp1-pci.dtso | 8 + > > drivers/pci/quirks.c | 1 + > > include/linux/pci_ids.h | 3 + > > 10 files changed, 524 insertions(+) > > create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso > > create mode 100644 drivers/misc/rp1/Kconfig > > create mode 100644 drivers/misc/rp1/Makefile > > create mode 100644 drivers/misc/rp1/rp1-pci.c > > create mode 100644 drivers/misc/rp1/rp1-pci.dtso > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 67f460c36ea1..1359538b76e8 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ > > RASPBERRY PI RP1 PCI DRIVER > > M: Andrea della Porta <andrea.porta@suse.com> > > S: Maintained > > +F: arch/arm64/boot/dts/broadcom/rp1.dtso > > F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > > F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml > > F: drivers/clk/clk-rp1.c > > +F: drivers/misc/rp1/ > > F: drivers/pinctrl/pinctrl-rp1.c > > F: include/dt-bindings/clock/rp1.h > > F: include/dt-bindings/misc/rp1.h > > diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > > new file mode 100644 > > index 000000000000..d80178a278ee > > --- /dev/null > > +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > > @@ -0,0 +1,152 @@ > > +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > > + > > +#include <dt-bindings/gpio/gpio.h> > > +#include <dt-bindings/interrupt-controller/irq.h> > > +#include <dt-bindings/clock/rp1.h> > > +#include <dt-bindings/misc/rp1.h> > > + > > +/dts-v1/; > > +/plugin/; > > + > > +/ { > > + fragment@0 { > > + target-path=""; > > + __overlay__ { > > + #address-cells = <3>; > > + #size-cells = <2>; > > + > > + rp1: rp1@0 { > > + compatible = "simple-bus"; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + interrupt-controller; > > + interrupt-parent = <&rp1>; > > + #interrupt-cells = <2>; > > + > > + // ranges and dma-ranges must be provided by the includer > > + ranges = <0xc0 0x40000000 > > + 0x01/*0x02000000*/ 0x00 0x00000000 > > + 0x00 0x00400000>; > > Are you 100% sure you do not have here dtc W=1 warnings? the W=1 warnings are: arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property I don't see anything related to the ranges line you mentioned. > > > + > > + dma-ranges = > > + // inbound RP1 1x_xxxxxxxx -> PCIe 1x_xxxxxxxx > > + <0x10 0x00000000 > > + 0x43000000 0x10 0x00000000 > > + 0x10 0x00000000>; > > + > > + clk_xosc: clk_xosc { > > Nope, switch to DTS coding style. Ack. > > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-output-names = "xosc"; > > + clock-frequency = <50000000>; > > + }; > > + > > + macb_pclk: macb_pclk { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-output-names = "pclk"; > > + clock-frequency = <200000000>; > > + }; > > + > > + macb_hclk: macb_hclk { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-output-names = "hclk"; > > + clock-frequency = <200000000>; > > + }; > > + > > + rp1_clocks: clocks@c040018000 { > > Why do you mix MMIO with non-MMIO nodes? This really does not look > correct. > Right. This is already under discussion here: https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by using CLK_OF_DECLARE. > > + compatible = "raspberrypi,rp1-clocks"; > > + #clock-cells = <1>; > > + reg = <0xc0 0x40018000 0x0 0x10038>; > > Wrong order of properties - see DTS coding style. Ack. Many thanks, Andrea > > > + clocks = <&clk_xosc>; > > + clock-names = "xosc"; > > Best regards, > Krzysztof >
> On a second thought, are you really sure we want to proceed with the header file? > After all the only line in it would be the extern declaration and the only one to > include it would be rp1-dev.c. Moreover, an header file would convey the false > premise that you can include it and use that symbol while in fact it should be > only used inside the driver. > OTOH, not creating that header file will continue to trigger the warning... The header file does not need to be in global scope. It could be in the driver source directory. As such, nothing outside of the driver can use it. Headers like this have multiple proposes. One is they make a symbol visible to the linker. But having two different .c files include the header enables type checking, which for long term maintenance is just as important. So a one line header is fine. Andrew
On Fri, Aug 30, 2024 at 03:49:04PM +0200, Andrea della Porta wrote: > Hi Krzysztof, > > On 10:38 Wed 21 Aug , Krzysztof Kozlowski wrote: > > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > > The RaspberryPi RP1 is ia 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-borad peripherals > > > by loading a devicetree overlay during driver probe. > > > The peripherals are accessed by mapping MMIO registers starting > > > from PCI BAR1 region. > > > As a minimum driver, the peripherals will not be added to the > > > dtbo here, but in following patches. > > > > > > Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > > --- > > > MAINTAINERS | 2 + > > > arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ > > > > Do not mix DTS with drivers. > > > > These MUST be separate. > > Separating the dtso from the driver in two different patches would mean > that the dtso patch would be ordered before the driver one. This is because > the driver embeds the dtbo binary blob inside itself, at build time. So > in order to build the driver, the dtso needs to be there also. This is not > the standard approach used with 'normal' dtb/dtbo, where the dtb patch is > ordered last wrt the driver it refers to. > Are you sure you want to proceed in this way? It is more about they are logically separate things. The .dtb/dtbo describes the hardware. It should be possible to review that as a standalone thing. The code them implements the binding. It makes no sense to review the code until the binding is correct, because changes to the binding will need changes to the code. Hence, we want the binding first, then the code which implements it. Andrew
On 30/08/2024 15:49, Andrea della Porta wrote: > Hi Krzysztof, > > On 10:38 Wed 21 Aug , Krzysztof Kozlowski wrote: >> On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: >>> The RaspberryPi RP1 is ia 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-borad peripherals >>> by loading a devicetree overlay during driver probe. >>> The peripherals are accessed by mapping MMIO registers starting >>> from PCI BAR1 region. >>> As a minimum driver, the peripherals will not be added to the >>> dtbo here, but in following patches. >>> >>> Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf >>> Signed-off-by: Andrea della Porta <andrea.porta@suse.com> >>> --- >>> MAINTAINERS | 2 + >>> arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ >> >> Do not mix DTS with drivers. >> >> These MUST be separate. > > Separating the dtso from the driver in two different patches would mean > that the dtso patch would be ordered before the driver one. This is because > the driver embeds the dtbo binary blob inside itself, at build time. So > in order to build the driver, the dtso needs to be there also. This is not Sure, in such case DTS will have to go through the same tree as driver as an exception. Please document it in patch changelog (---). > the standard approach used with 'normal' dtb/dtbo, where the dtb patch is > ordered last wrt the driver it refers to. It's not exactly the "ordered last" that matters, but lack of dependency and going through separate tree and branch - arm-soc/dts. Here there will be an exception how we handle patch, but still DTS is hardware description so should not be combined with driver code. > Are you sure you want to proceed in this way? > >> >>> drivers/misc/Kconfig | 1 + >>> drivers/misc/Makefile | 1 + >>> drivers/misc/rp1/Kconfig | 20 ++ >>> drivers/misc/rp1/Makefile | 3 + >>> drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ >>> drivers/misc/rp1/rp1-pci.dtso | 8 + >>> drivers/pci/quirks.c | 1 + >>> include/linux/pci_ids.h | 3 + >>> 10 files changed, 524 insertions(+) >>> create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso >>> create mode 100644 drivers/misc/rp1/Kconfig >>> create mode 100644 drivers/misc/rp1/Makefile >>> create mode 100644 drivers/misc/rp1/rp1-pci.c >>> create mode 100644 drivers/misc/rp1/rp1-pci.dtso >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index 67f460c36ea1..1359538b76e8 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ >>> RASPBERRY PI RP1 PCI DRIVER >>> M: Andrea della Porta <andrea.porta@suse.com> >>> S: Maintained >>> +F: arch/arm64/boot/dts/broadcom/rp1.dtso >>> F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml >>> F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml >>> F: drivers/clk/clk-rp1.c >>> +F: drivers/misc/rp1/ >>> F: drivers/pinctrl/pinctrl-rp1.c >>> F: include/dt-bindings/clock/rp1.h >>> F: include/dt-bindings/misc/rp1.h >>> diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso >>> new file mode 100644 >>> index 000000000000..d80178a278ee >>> --- /dev/null >>> +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso >>> @@ -0,0 +1,152 @@ >>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT) >>> + >>> +#include <dt-bindings/gpio/gpio.h> >>> +#include <dt-bindings/interrupt-controller/irq.h> >>> +#include <dt-bindings/clock/rp1.h> >>> +#include <dt-bindings/misc/rp1.h> >>> + >>> +/dts-v1/; >>> +/plugin/; >>> + >>> +/ { >>> + fragment@0 { >>> + target-path=""; >>> + __overlay__ { >>> + #address-cells = <3>; >>> + #size-cells = <2>; >>> + >>> + rp1: rp1@0 { >>> + compatible = "simple-bus"; >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + interrupt-controller; >>> + interrupt-parent = <&rp1>; >>> + #interrupt-cells = <2>; >>> + >>> + // ranges and dma-ranges must be provided by the includer >>> + ranges = <0xc0 0x40000000 >>> + 0x01/*0x02000000*/ 0x00 0x00000000 >>> + 0x00 0x00400000>; >> >> Are you 100% sure you do not have here dtc W=1 warnings? > > the W=1 warnings are: > > arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property > > I don't see anything related to the ranges line you mentioned. Hm, indeed, but I would expect warning about unit address not matching ranges/reg. > >> >>> + >>> + dma-ranges = >>> + // inbound RP1 1x_xxxxxxxx -> PCIe 1x_xxxxxxxx >>> + <0x10 0x00000000 >>> + 0x43000000 0x10 0x00000000 >>> + 0x10 0x00000000>; >>> + >>> + clk_xosc: clk_xosc { >> >> Nope, switch to DTS coding style. > > Ack. > >> >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-output-names = "xosc"; >>> + clock-frequency = <50000000>; >>> + }; >>> + >>> + macb_pclk: macb_pclk { >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-output-names = "pclk"; >>> + clock-frequency = <200000000>; >>> + }; >>> + >>> + macb_hclk: macb_hclk { >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-output-names = "hclk"; >>> + clock-frequency = <200000000>; >>> + }; >>> + >>> + rp1_clocks: clocks@c040018000 { >> >> Why do you mix MMIO with non-MMIO nodes? This really does not look >> correct. >> > > Right. This is already under discussion here: > https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ > > IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by > using CLK_OF_DECLARE. Depends. Where are these clocks? Naming suggests they might not be even part of this device. But if these are part of the device, then why this is not a clock controller (if they are controllable) or even removed (because we do not represent internal clock tree in DTS). Best regards, Krzysztof
On Fri, Aug 23, 2024 at 11:31 AM Andrea della Porta <andrea.porta@suse.com> wrote: > > Hi Stefan, > > On 12:23 Fri 23 Aug , Stefan Wahren wrote: > > Hi Andrea, > > > > Am 23.08.24 um 11:44 schrieb Andrea della Porta: > > > Hi Stefan, > > > > > > On 18:20 Wed 21 Aug , Stefan Wahren wrote: > > > > Hi Andrea, > > > > > > > > Am 20.08.24 um 16:36 schrieb Andrea della Porta: > > > > > The RaspberryPi RP1 is ia PCI multi function device containing > > > > > peripherals ranging from Ethernet to USB controller, I2C, SPI > > > > > and others. > > > > sorry, i cannot provide you a code review, but just some comments. multi > > > > function device suggests "mfd" subsystem or at least "soc" . I won't > > > > recommend misc driver here. > > > It's true that RP1 can be called an MFD but the reason for not placing > > > it in mfd subsystem are twofold: > > > > > > - these discussions are quite clear about this matter: please see [1] > > > and [2] > > > - the current driver use no mfd API at all > > > > > > This RP1 driver is not currently addressing any aspect of ARM core in the > > > SoC so I would say it should not stay in drivers/soc / either, as also > > > condifirmed by [2] again and [3] (note that Microchip LAN966x is a very > > > close fit to what we have here on RP1). > > thanks i was aware of these discussions. A pointer to them or at least a > > short statement in the cover letter would be great. > > Sure, consider it done. > > > > > > > > > Implement a bare minimum driver to operate the RP1, leveraging > > > > > actual OF based driver implementations for the on-borad peripherals > > > > > by loading a devicetree overlay during driver probe. > > > > Can you please explain why this should be a DT overlay? The RP1 is > > > > assembled on the Raspberry Pi 5 PCB. DT overlays are typically for loose > > > > connections like displays or HATs. I think a DTSI just for the RP1 would > > > > fit better and is easier to read. > > > The dtsi solution you proposed is the one adopted downstream. It has its > > > benefits of course, but there's more. > > > 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 be > > > possibly be reused in other hw implementations. I believe a similar > > > reasoning could be applied to Bootlin's Microchip LAN966x patchset as > > > well, and they also choose to approach the dtb overlay. > > Could please add this point in the commit message. Doesn't introduce > > Ack. > > > (maintainence) issues in case U-Boot needs a RP1 driver, too? > > Good point. Right now u-boot does not support RP1 nor PCIe (which is a > prerequisite for RP1 to work) on Rpi5 and I'm quite sure that it will be > so in the near future. Of course I cannot guarantee this will be the case > far away in time. > > Since u-boot is lacking support for RP1 we cannot really produce some test > results to check the compatibility versus kernel dtb overlay but we can > speculate a little bit about it. AFAIK u-boot would probably place the rp1 > node directly under its pcie@12000 node in DT while the dtb overlay will use > dynamically created PCI endpoint node (dev@0) as parent for rp1 node. u-boot could do that and it would not be following the 25+ year old PCI bus bindings. Some things may be argued about as "Linux bindings", but that isn't one of them. Rob
Quoting Andrea della Porta (2024-08-20 07:36:07) > The special section .dtb.init.rodata contains dtb and dtbo compiled > as objects inside the kernel and ends up in .init.data sectiion that > will be discarded early after the init phase. This is a problem for > builtin drivers that apply dtb overlay at runtime since this happens > later (e.g. during bus enumeration) and also for modules that should > be able to do it dynamically during runtime. > Move the dtb section to .data. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > include/asm-generic/vmlinux.lds.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > index ad6afc5c4918..3ae9097774b0 100644 > --- a/include/asm-generic/vmlinux.lds.h > +++ b/include/asm-generic/vmlinux.lds.h > @@ -365,6 +365,7 @@ > TRACE_PRINTKS() \ > BPF_RAW_TP() \ > TRACEPOINT_STR() \ > + KERNEL_DTB() \ The KERNEL_DTB() macro shows the section name is dtb.init.rodata. Can you remove the ".init." part if this isn't initdata anymore? And shouldn't it be in the RO_DATA() macro? It would be nice to keep the initdata properties when this isn't used after init as well. Perhaps we need another macro and/or filename to indicate that the DTB{O} can be thrown away after init/module init.
Hi Krzysztof, On 10:47 Wed 21 Aug , Krzysztof Kozlowski wrote: > On Tue, Aug 20, 2024 at 04:36:11PM +0200, Andrea della Porta wrote: > > Select the RP1 drivers needed to operate the PCI endpoint containing > > several peripherals such as Ethernet and USB Controller. This chip is > > present on RaspberryPi 5. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > arch/arm64/configs/defconfig | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > > index 7d32fca64996..e7615c464680 100644 > > --- a/arch/arm64/configs/defconfig > > +++ b/arch/arm64/configs/defconfig > > @@ -606,6 +606,7 @@ CONFIG_PINCTRL_QCM2290=y > > CONFIG_PINCTRL_QCS404=y > > CONFIG_PINCTRL_QDF2XXX=y > > CONFIG_PINCTRL_QDU1000=y > > +CONFIG_PINCTRL_RP1=y > > CONFIG_PINCTRL_SA8775P=y > > CONFIG_PINCTRL_SC7180=y > > CONFIG_PINCTRL_SC7280=y > > @@ -685,6 +686,7 @@ CONFIG_SENSORS_RASPBERRYPI_HWMON=m > > CONFIG_SENSORS_SL28CPLD=m > > CONFIG_SENSORS_INA2XX=m > > CONFIG_SENSORS_INA3221=m > > +CONFIG_MISC_RP1=y > > Module? Ack. > > > CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y > > CONFIG_CPU_THERMAL=y > > CONFIG_DEVFREQ_THERMAL=y > > @@ -1259,6 +1261,7 @@ CONFIG_COMMON_CLK_CS2000_CP=y > > CONFIG_COMMON_CLK_FSL_SAI=y > > CONFIG_COMMON_CLK_S2MPS11=y > > CONFIG_COMMON_CLK_PWM=y > > +CONFIG_COMMON_CLK_RP1=y > > Module? Ack. Many thanks, Andrea > > > CONFIG_COMMON_CLK_RS9_PCIE=y > > CONFIG_COMMON_CLK_VC3=y > > CONFIG_COMMON_CLK_VC5=y > > -- > > 2.35.3 > >
Hi Krzysztof, On 10:49 Wed 21 Aug , Krzysztof Kozlowski wrote: > On Tue, Aug 20, 2024 at 04:36:12PM +0200, Andrea della Porta wrote: > > RaspberryPi RP1 contains Cadence's MACB core. Implement the > > changes to be able to operate the customization in the RP1. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > > > @@ -5100,6 +5214,11 @@ static int macb_probe(struct platform_device *pdev) > > } > > } > > } > > + > > + device_property_read_u8(&pdev->dev, "cdns,aw2w-max-pipe", &bp->aw2w_max_pipe); > > + device_property_read_u8(&pdev->dev, "cdns,ar2r-max-pipe", &bp->ar2r_max_pipe); > > Where are the bindings? As stated in the cover letter, this patch (and the dtb patch #11 for macb) is completely unpolished and intended only for a quick test of the ethernet peripheral underneath the RP1. As such, it's not intended to be upstreamed yet. However, your feedback is really appreaciated and will be used in a future patch that will deal with ethernet mac support. > > > + bp->use_aw2b_fill = device_property_read_bool(&pdev->dev, "cdns,use-aw2b-fill"); > > + > > spin_lock_init(&bp->lock); > > > > /* setup capabilities */ > > @@ -5155,6 +5274,21 @@ static int macb_probe(struct platform_device *pdev) > > else > > bp->phy_interface = interface; > > > > + /* optional PHY reset-related properties */ > > + bp->phy_reset_gpio = devm_gpiod_get_optional(&pdev->dev, "phy-reset", > > Where is the binding? Ditto. > > > + GPIOD_OUT_LOW); > > + if (IS_ERR(bp->phy_reset_gpio)) { > > + dev_err(&pdev->dev, "Failed to obtain phy-reset gpio\n"); > > + err = PTR_ERR(bp->phy_reset_gpio); > > + goto err_out_free_netdev; > > + } > > + > > + bp->phy_reset_ms = 10; > > + of_property_read_u32(np, "phy-reset-duration", &bp->phy_reset_ms); > > Where is the binding? Ditto. Cheers, Andrea > > > + /* A sane reset duration should not be longer than 1s */ > > + if (bp->phy_reset_ms > 1000) > > + bp->phy_reset_ms = 1000; > > + > > /* IP specific init */ > > err = init(pdev); > > Best regards, > Krzysztof >
Hi Krzysztof, On 10:43 Wed 21 Aug , Krzysztof Kozlowski wrote: > On Tue, Aug 20, 2024 at 04:36:13PM +0200, Andrea della Porta wrote: > > RaspberryPi RP1 is multi function PCI endpoint device that > > exposes several subperipherals via PCI BAR. > > Add an ethernet node for Cadence MACB to the RP1 dtso > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > arch/arm64/boot/dts/broadcom/rp1.dtso | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > > index d80178a278ee..b40e203c28d5 100644 > > --- a/arch/arm64/boot/dts/broadcom/rp1.dtso > > +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > > @@ -78,6 +78,29 @@ rp1_clocks: clocks@c040018000 { > > <50000000>; // RP1_CLK_ETH_TSU > > }; > > > > + rp1_eth: ethernet@c040100000 { > > + reg = <0xc0 0x40100000 0x0 0x4000>; > > + compatible = "cdns,macb"; > > Please start using DTS coding style... Ack. Regards, Andrea > > Best regards, > Krzysztof >
On Wed, Aug 28, 2024 at 5:24 PM Andrea della Porta <andrea.porta@suse.com> wrote: > > Looks a bit like a reimplementation of regmap-mmio? If you want to do > > this why not use regmap-mmio? > > Agreed. I can leverage regmail_field to get rid of the reimplemented code > for the pin->pad register region. Do you think it could be worth using > regmap-mmio also on pin->gpio, pin->inte, pin->ints and pin->rio even > though they are not doing any special field manipulation as the pin->pad > case? Don't know without looking at the result, I bet you will see what looks best when you edit the patch, let's see what you come up with, I trust you on this. Yours, Linus Walleij
Hi Andrew, On 16:10 Fri 30 Aug , Andrew Lunn wrote: > > On a second thought, are you really sure we want to proceed with the header file? > > After all the only line in it would be the extern declaration and the only one to > > include it would be rp1-dev.c. Moreover, an header file would convey the false > > premise that you can include it and use that symbol while in fact it should be > > only used inside the driver. > > OTOH, not creating that header file will continue to trigger the warning... > > The header file does not need to be in global scope. It could be in > the driver source directory. As such, nothing outside of the driver > can use it. Ack. > > Headers like this have multiple proposes. One is they make a symbol > visible to the linker. But having two different .c files include the Hmm... not sure what second file is including it, since only rp1_pci.c needs it. > header enables type checking, which for long term maintenance is just > as important. So a one line header is fine. Done. Cheers, Andrea > > Andrew >
Hi Rob, On 13:27 Fri 30 Aug , Rob Herring wrote: > On Fri, Aug 23, 2024 at 11:31 AM Andrea della Porta > <andrea.porta@suse.com> wrote: > > ... > > > > Since u-boot is lacking support for RP1 we cannot really produce some test > > results to check the compatibility versus kernel dtb overlay but we can > > speculate a little bit about it. AFAIK u-boot would probably place the rp1 > > node directly under its pcie@12000 node in DT while the dtb overlay will use > > dynamically created PCI endpoint node (dev@0) as parent for rp1 node. > > u-boot could do that and it would not be following the 25+ year old > PCI bus bindings. Some things may be argued about as "Linux bindings", > but that isn't one of them. Indeed. It was just speculation, not something I would bet on. Regards, Andrea > > Rob
Hi Stephen, On 12:46 Fri 30 Aug , Stephen Boyd wrote: > Quoting Andrea della Porta (2024-08-20 07:36:07) > > The special section .dtb.init.rodata contains dtb and dtbo compiled > > as objects inside the kernel and ends up in .init.data sectiion that > > will be discarded early after the init phase. This is a problem for > > builtin drivers that apply dtb overlay at runtime since this happens > > later (e.g. during bus enumeration) and also for modules that should > > be able to do it dynamically during runtime. > > Move the dtb section to .data. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > include/asm-generic/vmlinux.lds.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > > index ad6afc5c4918..3ae9097774b0 100644 > > --- a/include/asm-generic/vmlinux.lds.h > > +++ b/include/asm-generic/vmlinux.lds.h > > @@ -365,6 +365,7 @@ > > TRACE_PRINTKS() \ > > BPF_RAW_TP() \ > > TRACEPOINT_STR() \ > > + KERNEL_DTB() \ > > The KERNEL_DTB() macro shows the section name is dtb.init.rodata. Can > you remove the ".init." part if this isn't initdata anymore? And > shouldn't it be in the RO_DATA() macro? Ack. > > It would be nice to keep the initdata properties when this isn't used > after init as well. Perhaps we need another macro and/or filename to > indicate that the DTB{O} can be thrown away after init/module init. We can certainly add some more filename extension that would place the relevant data in a droppable section. Throwing away the dtb/o after init is like the actual KERNEL_DTB macro that is adding teh data to section .init.data, but this would mean t would be useful only at very early init stage, just like for CONFIG_OF_UNITTEST. Throwing after module init could be more difficult though, I think, for example we're not sure when to discard the section in case of deferred modules probe. Many thanks, Andrea
Hi Andrew, On 16:21 Fri 30 Aug , Andrew Lunn wrote: > On Fri, Aug 30, 2024 at 03:49:04PM +0200, Andrea della Porta wrote: > > Hi Krzysztof, > > > > On 10:38 Wed 21 Aug , Krzysztof Kozlowski wrote: > > > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > > > The RaspberryPi RP1 is ia 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-borad peripherals > > > > by loading a devicetree overlay during driver probe. > > > > The peripherals are accessed by mapping MMIO registers starting > > > > from PCI BAR1 region. > > > > As a minimum driver, the peripherals will not be added to the > > > > dtbo here, but in following patches. > > > > > > > > Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > > > --- > > > > MAINTAINERS | 2 + > > > > arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ > > > > > > Do not mix DTS with drivers. > > > > > > These MUST be separate. > > > > Separating the dtso from the driver in two different patches would mean > > that the dtso patch would be ordered before the driver one. This is because > > the driver embeds the dtbo binary blob inside itself, at build time. So > > in order to build the driver, the dtso needs to be there also. This is not > > the standard approach used with 'normal' dtb/dtbo, where the dtb patch is > > ordered last wrt the driver it refers to. > > Are you sure you want to proceed in this way? > > It is more about they are logically separate things. The .dtb/dtbo > describes the hardware. It should be possible to review that as a > standalone thing. The code them implements the binding. It makes no > sense to review the code until the binding is correct, because changes > to the binding will need changes to the code. Hence, we want the > binding first, then the code which implements it. Ack. Cheers, Andrea > > Andrew
Hi Krzysztof, On 18:52 Fri 30 Aug , Krzysztof Kozlowski wrote: > On 30/08/2024 15:49, Andrea della Porta wrote: > > Hi Krzysztof, > > > > On 10:38 Wed 21 Aug , Krzysztof Kozlowski wrote: > >> On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > >>> The RaspberryPi RP1 is ia 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-borad peripherals > >>> by loading a devicetree overlay during driver probe. > >>> The peripherals are accessed by mapping MMIO registers starting > >>> from PCI BAR1 region. > >>> As a minimum driver, the peripherals will not be added to the > >>> dtbo here, but in following patches. > >>> > >>> Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > >>> Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > >>> --- > >>> MAINTAINERS | 2 + > >>> arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ > >> > >> Do not mix DTS with drivers. > >> > >> These MUST be separate. > > > > Separating the dtso from the driver in two different patches would mean > > that the dtso patch would be ordered before the driver one. This is because > > the driver embeds the dtbo binary blob inside itself, at build time. So > > in order to build the driver, the dtso needs to be there also. This is not > > Sure, in such case DTS will have to go through the same tree as driver > as an exception. Please document it in patch changelog (---). Ack. > > > the standard approach used with 'normal' dtb/dtbo, where the dtb patch is > > ordered last wrt the driver it refers to. > > It's not exactly the "ordered last" that matters, but lack of dependency > and going through separate tree and branch - arm-soc/dts. Here there > will be an exception how we handle patch, but still DTS is hardware > description so should not be combined with driver code. Ack. > > > Are you sure you want to proceed in this way? > > > > > >> > >>> drivers/misc/Kconfig | 1 + > >>> drivers/misc/Makefile | 1 + > >>> drivers/misc/rp1/Kconfig | 20 ++ > >>> drivers/misc/rp1/Makefile | 3 + > >>> drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ > >>> drivers/misc/rp1/rp1-pci.dtso | 8 + > >>> drivers/pci/quirks.c | 1 + > >>> include/linux/pci_ids.h | 3 + > >>> 10 files changed, 524 insertions(+) > >>> create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso > >>> create mode 100644 drivers/misc/rp1/Kconfig > >>> create mode 100644 drivers/misc/rp1/Makefile > >>> create mode 100644 drivers/misc/rp1/rp1-pci.c > >>> create mode 100644 drivers/misc/rp1/rp1-pci.dtso > >>> > >>> diff --git a/MAINTAINERS b/MAINTAINERS > >>> index 67f460c36ea1..1359538b76e8 100644 > >>> --- a/MAINTAINERS > >>> +++ b/MAINTAINERS > >>> @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ > >>> RASPBERRY PI RP1 PCI DRIVER > >>> M: Andrea della Porta <andrea.porta@suse.com> > >>> S: Maintained > >>> +F: arch/arm64/boot/dts/broadcom/rp1.dtso > >>> F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > >>> F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml > >>> F: drivers/clk/clk-rp1.c > >>> +F: drivers/misc/rp1/ > >>> F: drivers/pinctrl/pinctrl-rp1.c > >>> F: include/dt-bindings/clock/rp1.h > >>> F: include/dt-bindings/misc/rp1.h > >>> diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > >>> new file mode 100644 > >>> index 000000000000..d80178a278ee > >>> --- /dev/null > >>> +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > >>> @@ -0,0 +1,152 @@ > >>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > >>> + > >>> +#include <dt-bindings/gpio/gpio.h> > >>> +#include <dt-bindings/interrupt-controller/irq.h> > >>> +#include <dt-bindings/clock/rp1.h> > >>> +#include <dt-bindings/misc/rp1.h> > >>> + > >>> +/dts-v1/; > >>> +/plugin/; > >>> + > >>> +/ { > >>> + fragment@0 { > >>> + target-path=""; > >>> + __overlay__ { > >>> + #address-cells = <3>; > >>> + #size-cells = <2>; > >>> + > >>> + rp1: rp1@0 { > >>> + compatible = "simple-bus"; > >>> + #address-cells = <2>; > >>> + #size-cells = <2>; > >>> + interrupt-controller; > >>> + interrupt-parent = <&rp1>; > >>> + #interrupt-cells = <2>; > >>> + > >>> + // ranges and dma-ranges must be provided by the includer > >>> + ranges = <0xc0 0x40000000 > >>> + 0x01/*0x02000000*/ 0x00 0x00000000 > >>> + 0x00 0x00400000>; > >> > >> Are you 100% sure you do not have here dtc W=1 warnings? > > > > the W=1 warnings are: > > > > arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property > > arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property > > arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property > > arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property > > > > I don't see anything related to the ranges line you mentioned. > > Hm, indeed, but I would expect warning about unit address not matching > ranges/reg. > > > > >> > >>> + > >>> + dma-ranges = > >>> + // inbound RP1 1x_xxxxxxxx -> PCIe 1x_xxxxxxxx > >>> + <0x10 0x00000000 > >>> + 0x43000000 0x10 0x00000000 > >>> + 0x10 0x00000000>; > >>> + > >>> + clk_xosc: clk_xosc { > >> > >> Nope, switch to DTS coding style. > > > > Ack. > > > >> > >>> + compatible = "fixed-clock"; > >>> + #clock-cells = <0>; > >>> + clock-output-names = "xosc"; > >>> + clock-frequency = <50000000>; > >>> + }; > >>> + > >>> + macb_pclk: macb_pclk { > >>> + compatible = "fixed-clock"; > >>> + #clock-cells = <0>; > >>> + clock-output-names = "pclk"; > >>> + clock-frequency = <200000000>; > >>> + }; > >>> + > >>> + macb_hclk: macb_hclk { > >>> + compatible = "fixed-clock"; > >>> + #clock-cells = <0>; > >>> + clock-output-names = "hclk"; > >>> + clock-frequency = <200000000>; > >>> + }; > >>> + > >>> + rp1_clocks: clocks@c040018000 { > >> > >> Why do you mix MMIO with non-MMIO nodes? This really does not look > >> correct. > >> > > > > Right. This is already under discussion here: > > https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ > > > > IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by > > using CLK_OF_DECLARE. > > Depends. Where are these clocks? Naming suggests they might not be even > part of this device. But if these are part of the device, then why this > is not a clock controller (if they are controllable) or even removed > (because we do not represent internal clock tree in DTS). xosc is a crystal connected to the oscillator input of the RP1, so I would consider it an external fixed-clock. If we were in the entire dts, I would have put it in root under /clocks node, but here we're in the dtbo so I'm not sure where else should I put it. Regarding pclk and hclk, I'm still trying to understand where they come from. If they are external clocks (since they are fixed-clock too), they should be in the same node as xosc. CLK_OF_DECLARE does not seem to fit here because there's no special management of these clocks, so no new clock definition is needed. If they are internal tree, I cannot simply get rid of them because rp1_eth node references these two clocks (see clocks property), so they must be decalred somewhere. Any hint about this?. Many thanks, Andrea > > Best regards, > Krzysztof >
On 03/09/2024 17:15, Andrea della Porta wrote: >>>>> + >>>>> + rp1_clocks: clocks@c040018000 { >>>> >>>> Why do you mix MMIO with non-MMIO nodes? This really does not look >>>> correct. >>>> >>> >>> Right. This is already under discussion here: >>> https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ >>> >>> IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by >>> using CLK_OF_DECLARE. >> >> Depends. Where are these clocks? Naming suggests they might not be even >> part of this device. But if these are part of the device, then why this >> is not a clock controller (if they are controllable) or even removed >> (because we do not represent internal clock tree in DTS). > > xosc is a crystal connected to the oscillator input of the RP1, so I would > consider it an external fixed-clock. If we were in the entire dts, I would have > put it in root under /clocks node, but here we're in the dtbo so I'm not sure > where else should I put it. But physically, on which PCB, where is this clock located? > > Regarding pclk and hclk, I'm still trying to understand where they come from. > If they are external clocks (since they are fixed-clock too), they should be > in the same node as xosc. CLK_OF_DECLARE does not seem to fit here because There is no such node as "/clocks" so do not focus on that. That's just placeholder but useless and it is inconsistent with other cases (e.g. regulators). If this is external oscillator then it is not part of RP1 and you cannot put it inside just to satisfy your drivers. > there's no special management of these clocks, so no new clock definition is > needed. > If they are internal tree, I cannot simply get rid of them because rp1_eth node > references these two clocks (see clocks property), so they must be decalred > somewhere. Any hint about this?. > Describe the hardware. Show the diagram or schematics where is which device. Best regards, Krzysztof
Hi Krzysztof, On 20:27 Tue 03 Sep , Krzysztof Kozlowski wrote: > On 03/09/2024 17:15, Andrea della Porta wrote: > >>>>> + > >>>>> + rp1_clocks: clocks@c040018000 { > >>>> > >>>> Why do you mix MMIO with non-MMIO nodes? This really does not look > >>>> correct. > >>>> > >>> > >>> Right. This is already under discussion here: > >>> https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ > >>> > >>> IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by > >>> using CLK_OF_DECLARE. > >> > >> Depends. Where are these clocks? Naming suggests they might not be even > >> part of this device. But if these are part of the device, then why this > >> is not a clock controller (if they are controllable) or even removed > >> (because we do not represent internal clock tree in DTS). > > > > xosc is a crystal connected to the oscillator input of the RP1, so I would > > consider it an external fixed-clock. If we were in the entire dts, I would have > > put it in root under /clocks node, but here we're in the dtbo so I'm not sure > > where else should I put it. > > But physically, on which PCB, where is this clock located? xosc is a crystal, feeding the reference clock oscillator input pins of the RP1, please see page 12 of the following document: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf On Rpi5, the PCB is the very same as the one on which the BCM2712 (SoC) and RP1 are soldered. Would you consider it external (since the crystal is outside the RP1) or internal (since the oscillator feeded by the crystal is inside the RP1)? > > > > > Regarding pclk and hclk, I'm still trying to understand where they come from. > > If they are external clocks (since they are fixed-clock too), they should be > > in the same node as xosc. CLK_OF_DECLARE does not seem to fit here because > > There is no such node as "/clocks" so do not focus on that. That's just > placeholder but useless and it is inconsistent with other cases (e.g. > regulators). Fine, I beleve that the root node would be okay then, or some other carefully named node in root, if the clock is not internal to any chip. > > If this is external oscillator then it is not part of RP1 and you cannot > put it inside just to satisfy your drivers. Ack. > > > there's no special management of these clocks, so no new clock definition is > > needed. > > > If they are internal tree, I cannot simply get rid of them because rp1_eth node > > references these two clocks (see clocks property), so they must be decalred > > somewhere. Any hint about this?. > > > > Describe the hardware. Show the diagram or schematics where is which device. Unfortunately I don't have the documentation (schematics or other info) about how these two clocks (pclk and hclk) are arranged, but I'm trying to get some insight about that from various sources. While we're waiting for some (hopefully) more certain info, I'd like to speculate a bit. I would say that they both probably be either external (just like xosc), or generated internally to the RP1: If externals, I would place them in the same position as xosc, so root node or some other node under root (eg.: /rp1-clocks) If internals, I would leave them just where they are, i.e. inside the rp1 node Does it make sense? Many thnaks, Andrea > > Best regards, > Krzysztof >
On 05/09/2024 18:33, Andrea della Porta wrote: > Hi Krzysztof, > > On 20:27 Tue 03 Sep , Krzysztof Kozlowski wrote: >> On 03/09/2024 17:15, Andrea della Porta wrote: >>>>>>> + >>>>>>> + rp1_clocks: clocks@c040018000 { >>>>>> >>>>>> Why do you mix MMIO with non-MMIO nodes? This really does not look >>>>>> correct. >>>>>> >>>>> >>>>> Right. This is already under discussion here: >>>>> https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ >>>>> >>>>> IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by >>>>> using CLK_OF_DECLARE. >>>> >>>> Depends. Where are these clocks? Naming suggests they might not be even >>>> part of this device. But if these are part of the device, then why this >>>> is not a clock controller (if they are controllable) or even removed >>>> (because we do not represent internal clock tree in DTS). >>> >>> xosc is a crystal connected to the oscillator input of the RP1, so I would >>> consider it an external fixed-clock. If we were in the entire dts, I would have >>> put it in root under /clocks node, but here we're in the dtbo so I'm not sure >>> where else should I put it. >> >> But physically, on which PCB, where is this clock located? > > xosc is a crystal, feeding the reference clock oscillator input pins of the RP1, > please see page 12 of the following document: > https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf That's not the answer. Where is it physically located? > On Rpi5, the PCB is the very same as the one on which the BCM2712 (SoC) and RP1 > are soldered. Would you consider it external (since the crystal is outside the RP1) > or internal (since the oscillator feeded by the crystal is inside the RP1)? So it is on RPi 5 board? Just like every other SoC and every other vendor? Then just like every other SoC and every other vendor it is in board DTS file. > >> >>> >>> Regarding pclk and hclk, I'm still trying to understand where they come from. >>> If they are external clocks (since they are fixed-clock too), they should be >>> in the same node as xosc. CLK_OF_DECLARE does not seem to fit here because >> >> There is no such node as "/clocks" so do not focus on that. That's just >> placeholder but useless and it is inconsistent with other cases (e.g. >> regulators). > > Fine, I beleve that the root node would be okay then, or some other carefully named > node in root, if the clock is not internal to any chip. > >> >> If this is external oscillator then it is not part of RP1 and you cannot >> put it inside just to satisfy your drivers. > > Ack. > >> >>> there's no special management of these clocks, so no new clock definition is >>> needed. >> >>> If they are internal tree, I cannot simply get rid of them because rp1_eth node >>> references these two clocks (see clocks property), so they must be decalred >>> somewhere. Any hint about this?. >>> >> >> Describe the hardware. Show the diagram or schematics where is which device. > > Unfortunately I don't have the documentation (schematics or other info) about > how these two clocks (pclk and hclk) are arranged, but I'm trying to get > some insight about that from various sources. While we're waiting for some > (hopefully) more certain info, I'd like to speculate a bit. I would say that > they both probably be either external (just like xosc), or generated internally > to the RP1: > > If externals, I would place them in the same position as xosc, so root node > or some other node under root (eg.: /rp1-clocks) Just like /clocks, /rp1-clocks is not better. Neither /rp1-foo-clocks. I think there is some sort of big misunderstanding here. Is this RP1 co-processor on the RP board, connected over PCI to Broadcom SoC? > > If internals, I would leave them just where they are, i.e. inside the rp1 node > > Does it make sense? No, because you do not have xosc there, according to my knowledge. Best regards, Krzysztof
Hi Krzysztof, On 18:52 Thu 05 Sep , Krzysztof Kozlowski wrote: > On 05/09/2024 18:33, Andrea della Porta wrote: > > Hi Krzysztof, > > > > On 20:27 Tue 03 Sep , Krzysztof Kozlowski wrote: > >> On 03/09/2024 17:15, Andrea della Porta wrote: > >>>>>>> + > >>>>>>> + rp1_clocks: clocks@c040018000 { > >>>>>> > >>>>>> Why do you mix MMIO with non-MMIO nodes? This really does not look > >>>>>> correct. > >>>>>> > >>>>> > >>>>> Right. This is already under discussion here: > >>>>> https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ > >>>>> > >>>>> IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by > >>>>> using CLK_OF_DECLARE. > >>>> > >>>> Depends. Where are these clocks? Naming suggests they might not be even > >>>> part of this device. But if these are part of the device, then why this > >>>> is not a clock controller (if they are controllable) or even removed > >>>> (because we do not represent internal clock tree in DTS). > >>> > >>> xosc is a crystal connected to the oscillator input of the RP1, so I would > >>> consider it an external fixed-clock. If we were in the entire dts, I would have > >>> put it in root under /clocks node, but here we're in the dtbo so I'm not sure > >>> where else should I put it. > >> > >> But physically, on which PCB, where is this clock located? > > > > xosc is a crystal, feeding the reference clock oscillator input pins of the RP1, > > please see page 12 of the following document: > > https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > > That's not the answer. Where is it physically located? Please see below. > > > On Rpi5, the PCB is the very same as the one on which the BCM2712 (SoC) and RP1 > > are soldered. Would you consider it external (since the crystal is outside the RP1) > > or internal (since the oscillator feeded by the crystal is inside the RP1)? > > So it is on RPi 5 board? Just like every other SoC and every other > vendor? Then just like every other SoC and every other vendor it is in > board DTS file. Yes it's on the Rpi5 board. These are two separate thing, though: one is where to put it (DTS, DTSO) and another is in what target path relative to root. I was trying to understand the latter. The clock node should be put in the DTBO since we are loading this driver at runtime and we probably don't want to depend on some specific node name to be present in the DTS. This is also true because this driver should possibly work also on ACPI system and on hypothetical PCI card on which the RP1 could be mounted in the future, and in that case a DTS could be not even there. After all, those clocks must be in the immediate proximity to the RP1, and on the same board, which may or may not be the main board as the Rpi5 case. I think that, since this application is a little bit peculiar, maybe some compromises could be legit. > > > > >> > >>> > >>> Regarding pclk and hclk, I'm still trying to understand where they come from. > >>> If they are external clocks (since they are fixed-clock too), they should be > >>> in the same node as xosc. CLK_OF_DECLARE does not seem to fit here because > >> > >> There is no such node as "/clocks" so do not focus on that. That's just > >> placeholder but useless and it is inconsistent with other cases (e.g. > >> regulators). > > > > Fine, I beleve that the root node would be okay then, or some other carefully named > > node in root, if the clock is not internal to any chip. > > > >> > >> If this is external oscillator then it is not part of RP1 and you cannot > >> put it inside just to satisfy your drivers. > > > > Ack. > > > >> > >>> there's no special management of these clocks, so no new clock definition is > >>> needed. > >> > >>> If they are internal tree, I cannot simply get rid of them because rp1_eth node > >>> references these two clocks (see clocks property), so they must be decalred > >>> somewhere. Any hint about this?. > >>> > >> > >> Describe the hardware. Show the diagram or schematics where is which device. > > > > Unfortunately I don't have the documentation (schematics or other info) about > > how these two clocks (pclk and hclk) are arranged, but I'm trying to get > > some insight about that from various sources. While we're waiting for some > > (hopefully) more certain info, I'd like to speculate a bit. I would say that > > they both probably be either external (just like xosc), or generated internally > > to the RP1: > > > > If externals, I would place them in the same position as xosc, so root node > > or some other node under root (eg.: /rp1-clocks) > > Just like /clocks, /rp1-clocks is not better. Neither /rp1-foo-clocks. Right. So in this case, since xosc seems to be on the same level and on the same board of the RP1 and the SoC, and it's also external to the RP1, can I assume that placing xosc node in root is ok? > > I think there is some sort of big misunderstanding here. Is this RP1 > co-processor on the RP board, connected over PCI to Broadcom SoC? Yes. ---------------Rpi5 board--------------------- | | | SoC ==pci bus==> RP1 <== xosc crystal | | | ---------------------------------------------- > > > > > If internals, I would leave them just where they are, i.e. inside the rp1 node > > > > Does it make sense? > > No, because you do not have xosc there, according to my knowledge. Hmmm sorry, not sure what this negation was referring to... I was talking about hclk and pclk, not xosc here. Could you please add some details? Many thanks, Andrea > > Best regards, > Krzysztof >
On 05/09/2024 20:54, Andrea della Porta wrote: > Hi Krzysztof, > > On 18:52 Thu 05 Sep , Krzysztof Kozlowski wrote: >> On 05/09/2024 18:33, Andrea della Porta wrote: >>> Hi Krzysztof, >>> >>> On 20:27 Tue 03 Sep , Krzysztof Kozlowski wrote: >>>> On 03/09/2024 17:15, Andrea della Porta wrote: >>>>>>>>> + >>>>>>>>> + rp1_clocks: clocks@c040018000 { >>>>>>>> >>>>>>>> Why do you mix MMIO with non-MMIO nodes? This really does not look >>>>>>>> correct. >>>>>>>> >>>>>>> >>>>>>> Right. This is already under discussion here: >>>>>>> https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ >>>>>>> >>>>>>> IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by >>>>>>> using CLK_OF_DECLARE. >>>>>> >>>>>> Depends. Where are these clocks? Naming suggests they might not be even >>>>>> part of this device. But if these are part of the device, then why this >>>>>> is not a clock controller (if they are controllable) or even removed >>>>>> (because we do not represent internal clock tree in DTS). >>>>> >>>>> xosc is a crystal connected to the oscillator input of the RP1, so I would >>>>> consider it an external fixed-clock. If we were in the entire dts, I would have >>>>> put it in root under /clocks node, but here we're in the dtbo so I'm not sure >>>>> where else should I put it. >>>> >>>> But physically, on which PCB, where is this clock located? >>> >>> xosc is a crystal, feeding the reference clock oscillator input pins of the RP1, >>> please see page 12 of the following document: >>> https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf >> >> That's not the answer. Where is it physically located? > > Please see below. > >> >>> On Rpi5, the PCB is the very same as the one on which the BCM2712 (SoC) and RP1 >>> are soldered. Would you consider it external (since the crystal is outside the RP1) >>> or internal (since the oscillator feeded by the crystal is inside the RP1)? >> >> So it is on RPi 5 board? Just like every other SoC and every other >> vendor? Then just like every other SoC and every other vendor it is in >> board DTS file. > > Yes it's on the Rpi5 board. These are two separate thing, though: one is where Finally. > to put it (DTS, DTSO) and another is in what target path relative to root. I > was trying to understand the latter. It is already or should be part of DTS, not DTSO. You are duplicating device nodes. > The clock node should be put in the DTBO since we are loading this driver at > runtime and we probably don't want to depend on some specific node name to be > present in the DTS. This is also true because this driver should possibly work > also on ACPI system and on hypothetical PCI card on which the RP1 could be mounted Not really. ACPI and thus DT in such case will take it as clock input. Basically what you need here is to provide clock inputs to this device. It's then firmware independent. > in the future, and in that case a DTS could be not even there. No problem. Whatever firmware mechanism you have, it will provide you clock. > After all, those clocks must be in the immediate proximity to the RP1, and on the > same board, which may or may not be the main board as the Rpi5 case. > I think that, since this application is a little bit peculiar, maybe some > compromises could be legit. Application is not peculiar but completely standard. You have standard PCI device which has some inputs. One of these inputs, maybe on some reserved M.2 pins or whatver connector you have there, is the clock. ... >>> >>> If externals, I would place them in the same position as xosc, so root node >>> or some other node under root (eg.: /rp1-clocks) >> >> Just like /clocks, /rp1-clocks is not better. Neither /rp1-foo-clocks. > > Right. So in this case, since xosc seems to be on the same level and on the same > board of the RP1 and the SoC, and it's also external to the RP1, can I assume that > placing xosc node in root is ok? root node of the DTS yes. Root node of DTSO of course not, because it is not part of DTSO and you are duplicating DTS. It would not even work. That's why you need to apply the overlay to proper target as I asked long time ago. > >> >> I think there is some sort of big misunderstanding here. Is this RP1 >> co-processor on the RP board, connected over PCI to Broadcom SoC? > > Yes. > > ---------------Rpi5 board--------------------- > | | > | SoC ==pci bus==> RP1 <== xosc crystal | > | | > ---------------------------------------------- > >> >>> >>> If internals, I would leave them just where they are, i.e. inside the rp1 node >>> >>> Does it make sense? >> >> No, because you do not have xosc there, according to my knowledge. > > Hmmm sorry, not sure what this negation was referring to... I was talking about > hclk and pclk, not xosc here. Could you please add some details? If considering hclk and pclk, then depends where are they. If they come as inputs, then same as xosc. If they are not, then it is also obvious - we do not represent internal device clocks as fixed clocks in DTS, because it makes absolutely no sense at all. No benefits, no help, nothing at all. It's just device's internal clock. This is again nothing peculiar. Many other devices have some internal stuff. Do we add fixed clocks for these? Fixed regulators? No, of course not. Best regards, Krzysztof
Quoting Masahiro Yamada (2024-09-22 01:14:12) > > Rather, I'd modify my patch as follows: > > --- a/scripts/Makefile.dtbs > +++ b/scripts/Makefile.dtbs > @@ -34,12 +34,14 @@ $(obj)/dtbs-list: $(dtb-y) FORCE > # Assembly file to wrap dtb(o) > # --------------------------------------------------------------------------- > > +builtin-dtb-section = $(if $(filter arch/%, $(obj)),.dtb.init.rodata,.rodata) I think we want to free the empty root dtb that's always builtin. That is in drivers/of/ right? And I worry that an overlay could be in arch/ and then this breaks again. That's why it feels more correct to treat dtbo.o vs. dtb.o differently. Perhaps we can check $(obj) for dtbo vs dtb? Also, modpost code looks for .init* named sections and treats them as initdata already. Can we rename .dtb.init.rodata to .init.dtb.rodata so that modpost can find that?
On Tue, Sep 24, 2024 at 3:13 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Masahiro Yamada (2024-09-22 01:14:12) > > > > Rather, I'd modify my patch as follows: > > > > --- a/scripts/Makefile.dtbs > > +++ b/scripts/Makefile.dtbs > > @@ -34,12 +34,14 @@ $(obj)/dtbs-list: $(dtb-y) FORCE > > # Assembly file to wrap dtb(o) > > # --------------------------------------------------------------------------- > > > > +builtin-dtb-section = $(if $(filter arch/%, $(obj)),.dtb.init.rodata,.rodata) > > I think we want to free the empty root dtb that's always builtin. That > is in drivers/of/ right? drivers/of/empty_root.dts is really small. That is not a big deal even if empty_root.dtb remains in the .rodata section. > And I worry that an overlay could be in arch/ > and then this breaks again. That's why it feels more correct to treat > dtbo.o vs. dtb.o differently. Perhaps we can check $(obj) for dtbo vs > dtb? This is not a problem either. Checking $(obj)/ is temporary. See this later patch: https://lore.kernel.org/linux-kbuild/20240904234803.698424-16-masahiroy@kernel.org/T/#u After my work is completed, DTB and DTBO will go to the .rodata section unconditionally. > Also, modpost code looks for .init* named sections and treats them as > initdata already. Can we rename .dtb.init.rodata to .init.dtb.rodata so > that modpost can find that? My previous patch checked .dtb.init.rodata. I do not mind renaming it to .init.dtb.rodata.