| Message ID | cover.1730123575.git.andrea.porta@suse.com |
|---|---|
| Headers | show |
| Series | Add support for RaspberryPi RP1 PCI device using a DT overlay | expand |
Quoting Andrea della Porta (2024-10-28 07:07:28) > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > index 6e5a984c1d4e..efdf9abf04c4 100644 > --- a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > +++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > @@ -38,6 +38,13 @@ clk_emmc2: clk-emmc2 { > clock-frequency = <200000000>; > clock-output-names = "emmc2-clock"; > }; > + > + clk_rp1_xosc: clock-rp1-xosc { The node name is preferred to be clock-50000000 now.
On Mon, Oct 28, 2024 at 3:07 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> This looks good to me. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Can this patch and the GPIO/pinctrl bindings simply be applied to the pinctrl tree once the bindings are finalized? Yours, Linus Walleij
On Mon, Oct 28, 2024 at 3:07 PM Andrea della Porta <andrea.porta@suse.com> wrote: > +config PINCTRL_RP1 > + bool "Pinctrl driver for RP1" > + select PINMUX > + select PINCONF > + select GENERIC_PINCONF > + select GPIOLIB > + select GPIOLIB_IRQCHIP Just a quick thing: You don't happen to want: depends on MISC_RP1 default MISC_RP1 So it will always come in tandem with MISC_RP1? Yours, Linus Walleij
Quoting Andrea della Porta (2024-10-28 07:07:24) > diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c > new file mode 100644 > index 000000000000..69b9cf037cb2 > --- /dev/null [...] > + > +struct rp1_clockman { > + struct device *dev; > + void __iomem *regs; Do you still need this if there's a regmap? > + struct regmap *regmap; > + spinlock_t regs_lock; /* spinlock for all clocks */ Do you need this or is the spinlock in the regmap sufficient? > + > + /* Must be last */ > + struct clk_hw_onecell_data onecell; > +}; > + > +struct rp1_pll_core_data { > + const char *name; These 'name' members can move to clk_init_data? > + u32 cs_reg; > + u32 pwr_reg; > + u32 fbdiv_int_reg; > + u32 fbdiv_frac_reg; > + unsigned long flags; And probably flags as well? It seems like clk_init_data should be declared at the same time as struct rp1_pll_core_data is. > + u32 fc0_src; > +}; > + > +struct rp1_pll_data { > + const char *name; > + u32 ctrl_reg; > + unsigned long flags; > + u32 fc0_src; > +}; > + > +struct rp1_pll_ph_data { > + const char *name; > + unsigned int phase; > + unsigned int fixed_divider; > + u32 ph_reg; > + unsigned long flags; > + u32 fc0_src; > +}; > + > +struct rp1_pll_divider_data { > + const char *name; > + u32 sec_reg; > + unsigned long flags; > + u32 fc0_src; > +}; > + > +struct rp1_clock_data { > + const char *name; > + int num_std_parents; > + int num_aux_parents; > + unsigned long flags; > + u32 oe_mask; > + u32 clk_src_mask; > + u32 ctrl_reg; > + u32 div_int_reg; > + u32 div_frac_reg; > + u32 sel_reg; > + u32 div_int_max; > + unsigned long max_freq; > + u32 fc0_src; > +}; > + > +struct rp1_clk_desc { > + struct clk_hw *(*clk_register)(struct rp1_clockman *clockman, > + struct rp1_clk_desc *desc); > + const void *data; > + struct clk_hw hw; > + struct rp1_clockman *clockman; > + unsigned long cached_rate; > + struct clk_divider div; > +}; > + > +#define FIELD_SET(_reg, _mask, _val) \ > +do { \ > + u32 mask = (_mask); \ > + (_reg) &= ~mask; \ > + (_reg) |= FIELD_PREP(mask, (_val)); \ Please just write reg &= ~mask reg |= FIELD_PREP(mask, val); instead of using this macro. > +} while (0) > + > + [...] > + > +static struct clk_hw *rp1_register_pll_core(struct rp1_clockman *clockman, > + struct rp1_clk_desc *desc) > +{ > + const struct rp1_pll_core_data *pll_core_data = desc->data; > + struct clk_init_data init = { }; > + int ret; > + > + /* All of the PLL cores derive from the external oscillator. */ > + init.parent_data = desc->hw.init->parent_data; > + init.num_parents = desc->hw.init->num_parents; > + init.name = pll_core_data->name; > + init.ops = &rp1_pll_core_ops; > + init.flags = pll_core_data->flags | CLK_IGNORE_UNUSED | CLK_IS_CRITICAL; > + > + desc->clockman = clockman; > + desc->hw.init = &init; > + > + ret = devm_clk_hw_register(clockman->dev, &desc->hw); > + > + if (ret) > + return ERR_PTR(ret); > + > + return &desc->hw; > +} > + > +static struct clk_hw *rp1_register_pll(struct rp1_clockman *clockman, > + struct rp1_clk_desc *desc) > +{ > + const struct rp1_pll_data *pll_data = desc->data; > + struct clk_init_data init = { }; > + int ret; > + > + init.parent_data = desc->hw.init->parent_data; > + init.num_parents = desc->hw.init->num_parents; > + init.name = pll_data->name; > + init.ops = &rp1_pll_ops; > + init.flags = pll_data->flags | CLK_IGNORE_UNUSED | CLK_IS_CRITICAL; > + > + desc->clockman = clockman; > + desc->hw.init = &init; > + > + ret = devm_clk_hw_register(clockman->dev, &desc->hw); > + > + if (ret) > + return ERR_PTR(ret); > + > + return &desc->hw; > +} > + > +static struct clk_hw *rp1_register_pll_ph(struct rp1_clockman *clockman, > + struct rp1_clk_desc *desc) > +{ > + const struct rp1_pll_ph_data *ph_data = desc->data; > + struct clk_init_data init = { }; > + int ret; > + > + init.parent_data = desc->hw.init->parent_data; > + init.num_parents = desc->hw.init->num_parents; > + init.name = ph_data->name; > + init.ops = &rp1_pll_ph_ops; > + init.flags = ph_data->flags | CLK_IGNORE_UNUSED; > + > + desc->clockman = clockman; > + desc->hw.init = &init; > + > + ret = devm_clk_hw_register(clockman->dev, &desc->hw); > + > + if (ret) > + return ERR_PTR(ret); > + > + return &desc->hw; > +} > + > +static struct clk_hw *rp1_register_pll_divider(struct rp1_clockman *clockman, > + struct rp1_clk_desc *desc) > +{ > + const struct rp1_pll_data *divider_data = desc->data; > + struct clk_init_data init = { }; > + int ret; > + > + init.parent_data = desc->hw.init->parent_data; > + init.num_parents = desc->hw.init->num_parents; > + init.name = divider_data->name; > + init.ops = &rp1_pll_divider_ops; > + init.flags = divider_data->flags | CLK_IGNORE_UNUSED | CLK_IS_CRITICAL; > + > + desc->div.reg = clockman->regs + divider_data->ctrl_reg; Why is 'regs' used here? Isn't everything using a regmap now so it's all offsets? > + desc->div.shift = PLL_SEC_DIV_SHIFT; > + desc->div.width = PLL_SEC_DIV_WIDTH; > + desc->div.flags = CLK_DIVIDER_ROUND_CLOSEST; > + desc->div.flags |= CLK_IS_CRITICAL; > + desc->div.lock = &clockman->regs_lock; > + desc->div.hw.init = &init; > + desc->div.table = pll_sec_div_table; > + > + desc->clockman = clockman; > + > + ret = devm_clk_hw_register(clockman->dev, &desc->div.hw); > + > + if (ret) > + return ERR_PTR(ret); > + > + return &desc->div.hw; > +} > + > +static struct clk_hw *rp1_register_clock(struct rp1_clockman *clockman, > + struct rp1_clk_desc *desc) > +{ > + const struct rp1_clock_data *clock_data = desc->data; > + struct clk_init_data init = { }; > + int ret; > + > + if (WARN_ON_ONCE(MAX_CLK_PARENTS < > + clock_data->num_std_parents + clock_data->num_aux_parents)) > + return NULL; > + > + /* There must be a gap for the AUX selector */ > + if (WARN_ON_ONCE(clock_data->num_std_parents > AUX_SEL && > + desc->hw.init->parent_data[AUX_SEL].index != -1)) > + return NULL; > + > + init.parent_data = desc->hw.init->parent_data; > + init.num_parents = desc->hw.init->num_parents; > + init.name = clock_data->name; > + init.flags = clock_data->flags | CLK_IGNORE_UNUSED; > + init.ops = &rp1_clk_ops; > + > + desc->clockman = clockman; > + desc->hw.init = &init; > + > + ret = devm_clk_hw_register(clockman->dev, &desc->hw); > + > + if (ret) > + return ERR_PTR(ret); > + > + return &desc->hw; > +} > + > +/* Assignment helper macros for different clock types. */ > +#define _REGISTER(f, ...) { .clk_register = f, __VA_ARGS__ } > + > +#define PARENT_CLK(pnum, ...) .hw.init = &(const struct clk_init_data) { \ Instead of this macro just use CLK_HW_INIT_HW() or CLK_HW_INIT_PARENTS_DATA()? > + .parent_data = (const struct \ > + clk_parent_data[]) { \ > + __VA_ARGS__ \ > + }, \ > + .num_parents = pnum } > + > +#define CLK_DATA(type, ...) .data = &(struct type) { __VA_ARGS__ } > + > +#define REGISTER_PLL_CORE(...) _REGISTER(&rp1_register_pll_core, \ > + __VA_ARGS__) > + > +#define REGISTER_PLL(...) _REGISTER(&rp1_register_pll, \ > + __VA_ARGS__) > + > +#define REGISTER_PLL_PH(...) _REGISTER(&rp1_register_pll_ph, \ > + __VA_ARGS__) > + > +#define REGISTER_PLL_DIV(...) _REGISTER(&rp1_register_pll_divider, \ > + __VA_ARGS__) > + > +#define REGISTER_CLK(...) _REGISTER(&rp1_register_clock, \ > + __VA_ARGS__) > + > +static struct rp1_clk_desc clk_desc_array[] = { > + [RP1_PLL_SYS_CORE] = REGISTER_PLL_CORE(PARENT_CLK(1, { .index = 0 }), > + CLK_DATA(rp1_pll_core_data, > + .name = "pll_sys_core", > + .cs_reg = PLL_SYS_CS, > + .pwr_reg = PLL_SYS_PWR, > + .fbdiv_int_reg = PLL_SYS_FBDIV_INT, > + .fbdiv_frac_reg = PLL_SYS_FBDIV_FRAC, > + )), > + > + [RP1_PLL_AUDIO_CORE] = REGISTER_PLL_CORE(PARENT_CLK(1, { .index = 0 }), > + CLK_DATA(rp1_pll_core_data, > + .name = "pll_audio_core", > + .cs_reg = PLL_AUDIO_CS, > + .pwr_reg = PLL_AUDIO_PWR, > + .fbdiv_int_reg = PLL_AUDIO_FBDIV_INT, > + .fbdiv_frac_reg = PLL_AUDIO_FBDIV_FRAC, > + )), > + > + [RP1_PLL_VIDEO_CORE] = REGISTER_PLL_CORE(PARENT_CLK(1, { .index = 0 }), > + CLK_DATA(rp1_pll_core_data, > + .name = "pll_video_core", > + .cs_reg = PLL_VIDEO_CS, > + .pwr_reg = PLL_VIDEO_PWR, > + .fbdiv_int_reg = PLL_VIDEO_FBDIV_INT, > + .fbdiv_frac_reg = PLL_VIDEO_FBDIV_FRAC, > + )), > + > + [RP1_PLL_SYS] = REGISTER_PLL(PARENT_CLK(1, > + { .hw = &clk_desc_array[RP1_PLL_SYS_CORE].hw } > + ), > + CLK_DATA(rp1_pll_data, > + .name = "pll_sys", > + .ctrl_reg = PLL_SYS_PRIM, > + .fc0_src = FC_NUM(0, 2), > + )), > + > + [RP1_CLK_ETH_TSU] = REGISTER_CLK(PARENT_CLK(1, { .index = 0 }), > + CLK_DATA(rp1_clock_data, > + .name = "clk_eth_tsu", > + .num_std_parents = 0, > + .num_aux_parents = 1, > + .ctrl_reg = CLK_ETH_TSU_CTRL, > + .div_int_reg = CLK_ETH_TSU_DIV_INT, > + .sel_reg = CLK_ETH_TSU_SEL, > + .div_int_max = DIV_INT_8BIT_MAX, > + .max_freq = 50 * HZ_PER_MHZ, > + .fc0_src = FC_NUM(5, 7), > + )), > + > + [RP1_CLK_SYS] = REGISTER_CLK(PARENT_CLK(3, > + { .index = 0 }, > + { .index = -1 }, > + { .hw = &clk_desc_array[RP1_PLL_SYS].hw } > + ), > + CLK_DATA(rp1_clock_data, > + .name = "clk_sys", > + .num_std_parents = 3, > + .num_aux_parents = 0, > + .ctrl_reg = CLK_SYS_CTRL, > + .div_int_reg = CLK_SYS_DIV_INT, > + .sel_reg = CLK_SYS_SEL, > + .div_int_max = DIV_INT_24BIT_MAX, > + .max_freq = 200 * HZ_PER_MHZ, > + .fc0_src = FC_NUM(0, 4), > + .clk_src_mask = 0x3, > + )), > + > + [RP1_PLL_SYS_PRI_PH] = REGISTER_PLL_PH(PARENT_CLK(1, > + { .hw = &clk_desc_array[RP1_PLL_SYS].hw } > + ), > + CLK_DATA(rp1_pll_ph_data, > + .name = "pll_sys_pri_ph", > + .ph_reg = PLL_SYS_PRIM, > + .fixed_divider = 2, > + .phase = RP1_PLL_PHASE_0, > + .fc0_src = FC_NUM(1, 2), > + )), > + > + [RP1_PLL_SYS_SEC] = REGISTER_PLL_DIV(PARENT_CLK(1, > + { .hw = &clk_desc_array[RP1_PLL_SYS_CORE].hw } > + ), > + CLK_DATA(rp1_pll_data, > + .name = "pll_sys_sec", > + .ctrl_reg = PLL_SYS_SEC, > + .fc0_src = FC_NUM(2, 2), > + )), > +}; > + > +static const struct regmap_range rp1_reg_ranges[] = { > + regmap_reg_range(PLL_SYS_CS, PLL_SYS_SEC), > + regmap_reg_range(PLL_AUDIO_CS, PLL_AUDIO_TERN), > + regmap_reg_range(PLL_VIDEO_CS, PLL_VIDEO_SEC), > + regmap_reg_range(GPCLK_OE_CTRL, GPCLK_OE_CTRL), > + regmap_reg_range(CLK_SYS_CTRL, CLK_SYS_DIV_INT), > + regmap_reg_range(CLK_SYS_SEL, CLK_SYS_SEL), > + regmap_reg_range(CLK_SLOW_SYS_CTRL, CLK_SLOW_SYS_DIV_INT), > + regmap_reg_range(CLK_SLOW_SYS_SEL, CLK_SLOW_SYS_SEL), > + regmap_reg_range(CLK_DMA_CTRL, CLK_DMA_DIV_INT), > + regmap_reg_range(CLK_DMA_SEL, CLK_DMA_SEL), > + regmap_reg_range(CLK_UART_CTRL, CLK_UART_DIV_INT), > + regmap_reg_range(CLK_UART_SEL, CLK_UART_SEL), > + regmap_reg_range(CLK_ETH_CTRL, CLK_ETH_DIV_INT), > + regmap_reg_range(CLK_ETH_SEL, CLK_ETH_SEL), > + regmap_reg_range(CLK_PWM0_CTRL, CLK_PWM0_SEL), > + regmap_reg_range(CLK_PWM1_CTRL, CLK_PWM1_SEL), > + regmap_reg_range(CLK_AUDIO_IN_CTRL, CLK_AUDIO_IN_DIV_INT), > + regmap_reg_range(CLK_AUDIO_IN_SEL, CLK_AUDIO_IN_SEL), > + regmap_reg_range(CLK_AUDIO_OUT_CTRL, CLK_AUDIO_OUT_DIV_INT), > + regmap_reg_range(CLK_AUDIO_OUT_SEL, CLK_AUDIO_OUT_SEL), > + regmap_reg_range(CLK_I2S_CTRL, CLK_I2S_DIV_INT), > + regmap_reg_range(CLK_I2S_SEL, CLK_I2S_SEL), > + regmap_reg_range(CLK_MIPI0_CFG_CTRL, CLK_MIPI0_CFG_DIV_INT), > + regmap_reg_range(CLK_MIPI0_CFG_SEL, CLK_MIPI0_CFG_SEL), > + regmap_reg_range(CLK_MIPI1_CFG_CTRL, CLK_MIPI1_CFG_DIV_INT), > + regmap_reg_range(CLK_MIPI1_CFG_SEL, CLK_MIPI1_CFG_SEL), > + regmap_reg_range(CLK_PCIE_AUX_CTRL, CLK_PCIE_AUX_DIV_INT), > + regmap_reg_range(CLK_PCIE_AUX_SEL, CLK_PCIE_AUX_SEL), > + regmap_reg_range(CLK_USBH0_MICROFRAME_CTRL, CLK_USBH0_MICROFRAME_DIV_INT), > + regmap_reg_range(CLK_USBH0_MICROFRAME_SEL, CLK_USBH0_MICROFRAME_SEL), > + regmap_reg_range(CLK_USBH1_MICROFRAME_CTRL, CLK_USBH1_MICROFRAME_DIV_INT), > + regmap_reg_range(CLK_USBH1_MICROFRAME_SEL, CLK_USBH1_MICROFRAME_SEL), > + regmap_reg_range(CLK_USBH0_SUSPEND_CTRL, CLK_USBH0_SUSPEND_DIV_INT), > + regmap_reg_range(CLK_USBH0_SUSPEND_SEL, CLK_USBH0_SUSPEND_SEL), > + regmap_reg_range(CLK_USBH1_SUSPEND_CTRL, CLK_USBH1_SUSPEND_DIV_INT), > + regmap_reg_range(CLK_USBH1_SUSPEND_SEL, CLK_USBH1_SUSPEND_SEL), > + regmap_reg_range(CLK_ETH_TSU_CTRL, CLK_ETH_TSU_DIV_INT), > + regmap_reg_range(CLK_ETH_TSU_SEL, CLK_ETH_TSU_SEL), > + regmap_reg_range(CLK_ADC_CTRL, CLK_ADC_DIV_INT), > + regmap_reg_range(CLK_ADC_SEL, CLK_ADC_SEL), > + regmap_reg_range(CLK_SDIO_TIMER_CTRL, CLK_SDIO_TIMER_DIV_INT), > + regmap_reg_range(CLK_SDIO_TIMER_SEL, CLK_SDIO_TIMER_SEL), > + regmap_reg_range(CLK_SDIO_ALT_SRC_CTRL, CLK_SDIO_ALT_SRC_DIV_INT), > + regmap_reg_range(CLK_SDIO_ALT_SRC_SEL, CLK_SDIO_ALT_SRC_SEL), > + regmap_reg_range(CLK_GP0_CTRL, CLK_GP0_SEL), > + regmap_reg_range(CLK_GP1_CTRL, CLK_GP1_SEL), > + regmap_reg_range(CLK_GP2_CTRL, CLK_GP2_SEL), > + regmap_reg_range(CLK_GP3_CTRL, CLK_GP3_SEL), > + regmap_reg_range(CLK_GP4_CTRL, CLK_GP4_SEL), > + regmap_reg_range(CLK_GP5_CTRL, CLK_GP5_SEL), > + regmap_reg_range(CLK_SYS_RESUS_CTRL, CLK_SYS_RESUS_CTRL), > + regmap_reg_range(CLK_SLOW_SYS_RESUS_CTRL, CLK_SLOW_SYS_RESUS_CTRL), > + regmap_reg_range(FC0_REF_KHZ, FC0_RESULT), > + regmap_reg_range(VIDEO_CLK_VEC_CTRL, VIDEO_CLK_VEC_DIV_INT), > + regmap_reg_range(VIDEO_CLK_VEC_SEL, VIDEO_CLK_DPI_DIV_INT), > + regmap_reg_range(VIDEO_CLK_DPI_SEL, VIDEO_CLK_MIPI1_DPI_SEL), > +}; > + > +static const struct regmap_access_table rp1_reg_table = { > + .yes_ranges = rp1_reg_ranges, > + .n_yes_ranges = ARRAY_SIZE(rp1_reg_ranges), > +}; > + > +static const struct regmap_config rp1_clk_regmap_cfg = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = PLL_VIDEO_SEC, > + .name = "rp1-clk", > + .rd_table = &rp1_reg_table, > +}; > + > +static int rp1_clk_probe(struct platform_device *pdev) > +{ > + const size_t asize = ARRAY_SIZE(clk_desc_array); > + struct rp1_clk_desc *desc; > + struct device *dev = &pdev->dev; > + struct rp1_clockman *clockman; > + struct clk_hw **hws; > + unsigned int i; > + > + clockman = devm_kzalloc(dev, struct_size(clockman, onecell.hws, asize), > + GFP_KERNEL); > + if (!clockman) > + return -ENOMEM; > + > + spin_lock_init(&clockman->regs_lock); > + clockman->dev = dev; > + > + clockman->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(clockman->regs)) > + return PTR_ERR(clockman->regs); > + > + clockman->regmap = devm_regmap_init_mmio(dev, clockman->regs, > + &rp1_clk_regmap_cfg); > + if (IS_ERR(clockman->regmap)) { > + dev_err(dev, "could not init clock regmap\n"); return dev_err_probe()? > + return PTR_ERR(clockman->regmap); > + } > + > + clockman->onecell.num = asize; > + hws = clockman->onecell.hws; > + > + for (i = 0; i < asize; i++) { > + desc = &clk_desc_array[i]; > + if (desc->clk_register && desc->data) { > + hws[i] = desc->clk_register(clockman, desc); > + if (IS_ERR_OR_NULL(hws[i])) > + dev_err_probe(dev, PTR_ERR(hws[i]), > + "Unable to register clock: %s\n", > + clk_hw_get_name(hws[i])); > + } > + } > + > + platform_set_drvdata(pdev, clockman); > + > + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > + &clockman->onecell); > +}
Hi Linus, On 23:18 Mon 28 Oct , Linus Walleij wrote: > On Mon, Oct 28, 2024 at 3:07 PM Andrea della Porta > <andrea.porta@suse.com> wrote: > > > +config PINCTRL_RP1 > > + bool "Pinctrl driver for RP1" > > + select PINMUX > > + select PINCONF > > + select GENERIC_PINCONF > > + select GPIOLIB > > + select GPIOLIB_IRQCHIP > > Just a quick thing: > > You don't happen to want: > depends on MISC_RP1 > default MISC_RP1 > > So it will always come in tandem with > MISC_RP1? You're right! Added. However I will postpone the "default MISC_RP1" line after checking why the pinctrl driver does not work when compiled as a module (right now it's bool, not tristate). Many thanks, Andrea > > Yours, > Linus Walleij
Hi Stephen, On 13:49 Mon 28 Oct , Stephen Boyd wrote: > Quoting Andrea della Porta (2024-10-28 07:07:28) > > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > > index 6e5a984c1d4e..efdf9abf04c4 100644 > > --- a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > > +++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > > @@ -38,6 +38,13 @@ clk_emmc2: clk-emmc2 { > > clock-frequency = <200000000>; > > clock-output-names = "emmc2-clock"; > > }; > > + > > + clk_rp1_xosc: clock-rp1-xosc { > > The node name is preferred to be clock-50000000 now. Ack. Many thanks, Andrea
On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > bridge, the window should instead be in PCI address space. Call > pci_bus_address() on the resource in order to obtain the PCI bus > address. > of_pci_prop_ranges() could be called for PCI devices also (not just PCI bridges), right? - Mani > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > drivers/pci/of_property.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c > index 5a0b98e69795..886c236e5de6 100644 > --- a/drivers/pci/of_property.c > +++ b/drivers/pci/of_property.c > @@ -126,7 +126,7 @@ static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs, > if (of_pci_get_addr_flags(&res[j], &flags)) > continue; > > - val64 = res[j].start; > + val64 = pci_bus_address(pdev, &res[j] - pdev->resource); > of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags, > false); > if (pci_is_bridge(pdev)) { > -- > 2.35.3 >
Hi Andrea, On Mon, 28 Oct 2024 15:07:22 +0100 Andrea della Porta <andrea.porta@suse.com> wrote: > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > bridge, the window should instead be in PCI address space. Call > pci_bus_address() on the resource in order to obtain the PCI bus > address. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> Tested ok with my LAN966x PCI device. Tested-by: Herve Codina <herve.codina@bootlin.com> Best regards, Hervé
Hi Herve, On 09:06 Mon 04 Nov , Herve Codina wrote: > Hi Andrea, > > On Mon, 28 Oct 2024 15:07:22 +0100 > Andrea della Porta <andrea.porta@suse.com> wrote: > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > bridge, the window should instead be in PCI address space. Call > > pci_bus_address() on the resource in order to obtain the PCI bus > > address. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > Tested ok with my LAN966x PCI device. > > Tested-by: Herve Codina <herve.codina@bootlin.com> Thanks for testing that! Regards, Andrea > > Best regards, > Hervé
Hi Manivannan, On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > bridge, the window should instead be in PCI address space. Call > > pci_bus_address() on the resource in order to obtain the PCI bus > > address. > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > bridges), right? Correct. Please note however that while the PCI-PCI bridge has the parent address in CPU space, an endpoint device has it in PCI space: here we're focusing on the bridge part. It probably used to work before since in many cases the CPU and PCI address are the same, but it breaks down when they differ. Many thanks, Andrea > > - Mani > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > drivers/pci/of_property.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c > > index 5a0b98e69795..886c236e5de6 100644 > > --- a/drivers/pci/of_property.c > > +++ b/drivers/pci/of_property.c > > @@ -126,7 +126,7 @@ static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs, > > if (of_pci_get_addr_flags(&res[j], &flags)) > > continue; > > > > - val64 = res[j].start; > > + val64 = pci_bus_address(pdev, &res[j] - pdev->resource); > > of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags, > > false); > > if (pci_is_bridge(pdev)) { > > -- > > 2.35.3 > > > > -- > மணிவண்ணன் சதாசிவம்
Hi Andrea, On 10/28/24 16:07, Andrea della Porta wrote: > RaspberryPi RP1 is a multi function PCI endpoint device that > exposes several subperipherals via PCI BAR. > Add a dtb overlay that will be compiled into a binary blob > and linked in the RP1 driver. > This overlay offers just minimal support to represent the > RP1 device itself, the sub-peripherals will be added by > future patches. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > NOTE: this patch should be taken by the same maintainer that will take > "[PATCH v3 10/12] misc: rp1: RaspberryPi RP1 misc driver", since they > are closely related in terms of compiling. > > MAINTAINERS | 1 + > arch/arm64/boot/dts/broadcom/rp1.dtso | 61 +++++++++++++++++++++++++++ > 2 files changed, 62 insertions(+) > create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso > > diff --git a/MAINTAINERS b/MAINTAINERS > index 06277969a522..510a071ede78 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19383,6 +19383,7 @@ 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/misc/pci1de4,1.yaml > F: Documentation/devicetree/bindings/pci/pci-ep-bus.yaml > diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > new file mode 100644 > index 000000000000..8d1bbf207a30 > --- /dev/null > +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > @@ -0,0 +1,61 @@ > +// 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/raspberrypi,rp1-clocks.h> > + > +/dts-v1/; > +/plugin/; > + > +/ { > + fragment@0 { > + target-path=""; > + __overlay__ { > + compatible = "pci1de4,1"; > + #address-cells = <3>; > + #size-cells = <2>; > + interrupt-controller; > + #interrupt-cells = <2>; > + > + pci_ep_bus: pci-ep-bus@1 { > + compatible = "simple-bus"; > + ranges = <0xc0 0x40000000 > + 0x01 0x00 0x00000000 > + 0x00 0x00400000>; > + dma-ranges = <0x10 0x00000000 > + 0x43000000 0x10 0x00000000 > + 0x10 0x00000000>; > + #address-cells = <2>; > + #size-cells = <2>; > + > + rp1_clocks: clocks@c040018000 { > + compatible = "raspberrypi,rp1-clocks"; > + reg = <0xc0 0x40018000 0x0 0x10038>; shouldn't this be: rp1_clocks: clocks@18000 { reg = <0x00 0x00018000 0x0 0x10038>; ... } ? And for other nodes too... ~Stan > + #clock-cells = <1>; > + clocks = <&clk_rp1_xosc>; > + clock-names = "xosc"; > + assigned-clocks = <&rp1_clocks RP1_PLL_SYS_CORE>, > + <&rp1_clocks RP1_PLL_SYS>, > + <&rp1_clocks RP1_CLK_SYS>; > + assigned-clock-rates = <1000000000>, // RP1_PLL_SYS_CORE > + <200000000>, // RP1_PLL_SYS > + <200000000>; // RP1_CLK_SYS > + }; > + > + rp1_gpio: pinctrl@c0400d0000 { > + compatible = "raspberrypi,rp1-gpio"; > + reg = <0xc0 0x400d0000 0x0 0xc000>, > + <0xc0 0x400e0000 0x0 0xc000>, > + <0xc0 0x400f0000 0x0 0xc000>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>, > + <1 IRQ_TYPE_LEVEL_HIGH>, > + <2 IRQ_TYPE_LEVEL_HIGH>; > + }; > + }; > + }; > + }; > +};
On 10/28/24 16:07, Andrea della Porta wrote: > The RaspberryPi RP1 is a PCI multi function device containing > peripherals ranging from Ethernet to USB controller, I2C, SPI > and others. > > Implement a bare minimum driver to operate the RP1, leveraging > actual OF based driver implementations for the on-board peripherals > by loading a devicetree overlay during driver probe. > > The peripherals are accessed by mapping MMIO registers starting > from PCI BAR1 region. > > With the overlay approach we can achieve more generic and agnostic > approach to managing this chipset, being that it is a PCI endpoint > and could possibly be reused in other hw implementations. The > presented approach is also used by Bootlin's Microchip LAN966x > patchset (see link) as well, for a similar chipset. > > Since the gpio line names should be provided by the user, there > is an interface through configfs that allows the userspace to > load a DT overlay that will provide gpio-line-names property. > The interface can be invoked like this: > > cat rpi-rp1-gpios-5-b.dtbo > /sys/kernel/config/rp1-cfg/gpio_set_names > > and is designed to be similar to what users are already used to > from distro with downstream kernel. > > For reasons why this driver is contained in drivers/misc, please > check the links. > > This driver is heavily based on downstream code from RaspberryPi > Foundation, and the original author is Phil Elwell. > > Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > Link: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ > Link: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ > Link: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ > Link: https://lore.kernel.org/all/20240808154658.247873-1-herve.codina@bootlin.com/ > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > MAINTAINERS | 1 + > drivers/misc/Kconfig | 1 + > drivers/misc/Makefile | 1 + > drivers/misc/rp1/Kconfig | 21 ++ > drivers/misc/rp1/Makefile | 3 + > drivers/misc/rp1/rp1-pci.dtso | 8 + > drivers/misc/rp1/rp1_pci.c | 357 ++++++++++++++++++++++++++++++++++ > drivers/misc/rp1/rp1_pci.h | 14 ++ > drivers/pci/quirks.c | 1 + > include/linux/pci_ids.h | 3 + > 10 files changed, 410 insertions(+) > create mode 100644 drivers/misc/rp1/Kconfig > create mode 100644 drivers/misc/rp1/Makefile > create mode 100644 drivers/misc/rp1/rp1-pci.dtso > create mode 100644 drivers/misc/rp1/rp1_pci.c > create mode 100644 drivers/misc/rp1/rp1_pci.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 510a071ede78..032678fb2470 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19389,6 +19389,7 @@ F: Documentation/devicetree/bindings/misc/pci1de4,1.yaml > F: Documentation/devicetree/bindings/pci/pci-ep-bus.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/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 3fe7e2a9bd29..ac85cb154100 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -628,4 +628,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 a9f94525e181..ae86d69997b4 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -72,3 +72,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..39c4aa4bf634 > --- /dev/null > +++ b/drivers/misc/rp1/Kconfig > @@ -0,0 +1,21 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +# > +# RaspberryPi RP1 misc device > +# > + > +config MISC_RP1 > + tristate "RaspberryPi RP1 PCIe support" > + depends on OF_IRQ && PCI_MSI && PCI_QUIRKS > + select OF_OVERLAY > + select PCI_DYNAMIC_OF_NODES > + help > + Support 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..508b4cb05627 > --- /dev/null > +++ b/drivers/misc/rp1/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +obj-$(CONFIG_MISC_RP1) += rp1-pci.o > +rp1-pci-objs := rp1_pci.o rp1-pci.dtbo.o > diff --git a/drivers/misc/rp1/rp1-pci.dtso b/drivers/misc/rp1/rp1-pci.dtso > new file mode 100644 > index 000000000000..0bf2f4bb18e6 > --- /dev/null > +++ b/drivers/misc/rp1/rp1-pci.dtso > @@ -0,0 +1,8 @@ > +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > + > +/* the dts overlay is included from the dts directory so > + * it can be possible to check it with CHECK_DTBS while > + * also compile it from the driver source directory. > + */ > + > +#include "arm64/broadcom/rp1.dtso" > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c > new file mode 100644 > index 000000000000..30d7a15dc7f1 > --- /dev/null > +++ b/drivers/misc/rp1/rp1_pci.c > @@ -0,0 +1,357 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018-24 Raspberry Pi Ltd. > + * All rights reserved. > + */ > + > +#include <linux/err.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/irqdomain.h> > +#include <linux/module.h> > +#include <linux/msi.h> > +#include <linux/of_platform.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > + > +#include "rp1_pci.h" > + > +#define RP1_DRIVER_NAME "rp1" > + > +#define RP1_HW_IRQ_MASK GENMASK(5, 0) > + > +#define REG_SET 0x800 > +#define REG_CLR 0xc00 > + > +/* MSI-X 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_ENABLE BIT(0) > + > +/* Address map */ > +#define RP1_PCIE_APBS_BASE 0x108000 > + > +/* Interrupts */ > +#define RP1_INT_IO_BANK0 0 > +#define RP1_INT_IO_BANK1 1 > +#define RP1_INT_IO_BANK2 2 > +#define RP1_INT_AUDIO_IN 3 > +#define RP1_INT_AUDIO_OUT 4 > +#define RP1_INT_PWM0 5 > +#define RP1_INT_ETH 6 > +#define RP1_INT_I2C0 7 > +#define RP1_INT_I2C1 8 > +#define RP1_INT_I2C2 9 > +#define RP1_INT_I2C3 10 > +#define RP1_INT_I2C4 11 > +#define RP1_INT_I2C5 12 > +#define RP1_INT_I2C6 13 > +#define RP1_INT_I2S0 14 > +#define RP1_INT_I2S1 15 > +#define RP1_INT_I2S2 16 > +#define RP1_INT_SDIO0 17 > +#define RP1_INT_SDIO1 18 > +#define RP1_INT_SPI0 19 > +#define RP1_INT_SPI1 20 > +#define RP1_INT_SPI2 21 > +#define RP1_INT_SPI3 22 > +#define RP1_INT_SPI4 23 > +#define RP1_INT_SPI5 24 > +#define RP1_INT_UART0 25 > +#define RP1_INT_TIMER_0 26 > +#define RP1_INT_TIMER_1 27 > +#define RP1_INT_TIMER_2 28 > +#define RP1_INT_TIMER_3 29 > +#define RP1_INT_USBHOST0 30 > +#define RP1_INT_USBHOST0_0 31 > +#define RP1_INT_USBHOST0_1 32 > +#define RP1_INT_USBHOST0_2 33 > +#define RP1_INT_USBHOST0_3 34 > +#define RP1_INT_USBHOST1 35 > +#define RP1_INT_USBHOST1_0 36 > +#define RP1_INT_USBHOST1_1 37 > +#define RP1_INT_USBHOST1_2 38 > +#define RP1_INT_USBHOST1_3 39 > +#define RP1_INT_DMA 40 > +#define RP1_INT_PWM1 41 > +#define RP1_INT_UART1 42 > +#define RP1_INT_UART2 43 > +#define RP1_INT_UART3 44 > +#define RP1_INT_UART4 45 > +#define RP1_INT_UART5 46 > +#define RP1_INT_MIPI0 47 > +#define RP1_INT_MIPI1 48 > +#define RP1_INT_VIDEO_OUT 49 > +#define RP1_INT_PIO_0 50 > +#define RP1_INT_PIO_1 51 > +#define RP1_INT_ADC_FIFO 52 > +#define RP1_INT_PCIE_OUT 53 > +#define RP1_INT_SPI6 54 > +#define RP1_INT_SPI7 55 > +#define RP1_INT_SPI8 56 > +#define RP1_INT_SYSCFG 58 > +#define RP1_INT_CLOCKS_DEFAULT 59 > +#define RP1_INT_VBUSCTRL 60 > +#define RP1_INT_PROC_MISC 57 > +#define RP1_INT_END 61 > + How those interrupts are supposed to be used by rp1-pci.dtso (or from rp1.dtso)? include/dt-bindings/interrupt-controller/raspberrypi,rp1-interrupts.h ? > +struct rp1_dev { > + struct pci_dev *pdev; > + struct device *dev; > + struct irq_domain *domain; > + struct irq_data *pcie_irqds[64]; > + void __iomem *bar1; > + int ovcs_id; > + bool level_triggered_irq[RP1_INT_END]; > +}; > + > +static void msix_cfg_set(struct rp1_dev *rp1, unsigned int hwirq, u32 value) > +{ > + iowrite32(value, rp1->bar1 + RP1_PCIE_APBS_BASE + REG_SET + MSIX_CFG(hwirq)); > +} > + > +static void msix_cfg_clr(struct rp1_dev *rp1, unsigned int hwirq, u32 value) > +{ > + iowrite32(value, rp1->bar1 + RP1_PCIE_APBS_BASE + REG_CLR + MSIX_CFG(hwirq)); > +} > + > +static void rp1_mask_irq(struct irq_data *irqd) > +{ > + struct rp1_dev *rp1 = irqd->domain->host_data; > + struct irq_data *pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; > + > + pci_msi_mask_irq(pcie_irqd); > +} > + > +static void rp1_unmask_irq(struct irq_data *irqd) > +{ > + struct rp1_dev *rp1 = irqd->domain->host_data; > + struct irq_data *pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; > + > + pci_msi_unmask_irq(pcie_irqd); > +} > + > +static int rp1_irq_set_type(struct irq_data *irqd, unsigned int type) > +{ > + struct rp1_dev *rp1 = irqd->domain->host_data; > + unsigned int hwirq = (unsigned int)irqd->hwirq; > + > + switch (type) { > + case IRQ_TYPE_LEVEL_HIGH: > + dev_dbg(rp1->dev, "MSIX IACK EN for irq %d\n", hwirq); > + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK_EN); > + rp1->level_triggered_irq[hwirq] = true; > + break; > + case IRQ_TYPE_EDGE_RISING: > + msix_cfg_clr(rp1, hwirq, MSIX_CFG_IACK_EN); > + rp1->level_triggered_irq[hwirq] = false; > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static struct irq_chip rp1_irq_chip = { > + .name = "rp1_irq_chip", tabs instead of spaces here and below. > + .irq_mask = rp1_mask_irq, > + .irq_unmask = rp1_unmask_irq, > + .irq_set_type = rp1_irq_set_type, > +}; > + > +static void rp1_chained_handle_irq(struct irq_desc *desc) > +{ > + unsigned int hwirq = desc->irq_data.hwirq & RP1_HW_IRQ_MASK; > + struct rp1_dev *rp1 = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + int virq; unsigned int > + > + chained_irq_enter(chip, desc); > + > + virq = irq_find_mapping(rp1->domain, hwirq); > + generic_handle_irq(virq); > + if (rp1->level_triggered_irq[hwirq]) > + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK); > + > + chained_irq_exit(chip, desc); > +} > + > +static int rp1_irq_xlate(struct irq_domain *d, struct device_node *node, > + const u32 *intspec, unsigned int intsize, > + unsigned long *out_hwirq, unsigned int *out_type) > +{ > + struct rp1_dev *rp1 = d->host_data; > + struct irq_data *pcie_irqd; > + unsigned long hwirq; > + int pcie_irq; > + int ret; > + > + ret = irq_domain_xlate_twocell(d, node, intspec, intsize, > + &hwirq, out_type); > + if (ret) > + return ret; > + > + pcie_irq = pci_irq_vector(rp1->pdev, hwirq); > + pcie_irqd = irq_get_irq_data(pcie_irq); > + rp1->pcie_irqds[hwirq] = pcie_irqd; > + *out_hwirq = hwirq; > + > + return 0; > +} > + > +static int rp1_irq_activate(struct irq_domain *d, struct irq_data *irqd, > + bool reserve) > +{ > + struct rp1_dev *rp1 = d->host_data; > + > + msix_cfg_set(rp1, (unsigned int)irqd->hwirq, MSIX_CFG_ENABLE); > + > + return 0; > +} > + > +static void rp1_irq_deactivate(struct irq_domain *d, struct irq_data *irqd) > +{ > + struct rp1_dev *rp1 = d->host_data; > + > + msix_cfg_clr(rp1, (unsigned int)irqd->hwirq, MSIX_CFG_ENABLE); > +} > + > +static const struct irq_domain_ops rp1_domain_ops = { > + .xlate = rp1_irq_xlate, > + .activate = rp1_irq_activate, > + .deactivate = rp1_irq_deactivate, > +}; > + > +static void rp1_unregister_interrupts(struct pci_dev *pdev) > +{ > + struct rp1_dev *rp1 = pci_get_drvdata(pdev); > + int irq, i; > + > + for (i = 0; i < RP1_INT_END; i++) { > + irq = irq_find_mapping(rp1->domain, i); > + irq_dispose_mapping(irq); > + } new line > + irq_domain_remove(rp1->domain); > + pci_free_irq_vectors(pdev); > +} > + > +static int rp1_probe(struct pci_dev *pdev, const struct pci_device_id *id) > +{ > + u32 dtbo_size = __dtbo_rp1_pci_end - __dtbo_rp1_pci_begin; > + void *dtbo_start = __dtbo_rp1_pci_begin; > + struct device *dev = &pdev->dev; > + struct device_node *rp1_node; > + struct rp1_dev *rp1; > + int err = 0; > + int i; > + > + rp1_node = dev_of_node(dev); > + if (!rp1_node) { > + dev_err(dev, "Missing of_node for device\n"); > + return -EINVAL; > + } > + > + rp1 = devm_kzalloc(&pdev->dev, sizeof(*rp1), GFP_KERNEL); > + if (!rp1) > + return -ENOMEM; > + > + rp1->pdev = pdev; > + rp1->dev = &pdev->dev; rp1->dev is used only once in dev_dbg macro. You could drop it and extract struct device from rp1->pdev. > + > + if (pci_resource_len(pdev, 1) <= 0x10000) { > + dev_err(&pdev->dev, > + "Not initialised - is the firmware running?\n"); no need of a new line > + return -EINVAL; > + } > + > + err = pcim_enable_device(pdev); > + if (err < 0) { > + dev_err(&pdev->dev, "Enabling PCI device has failed: %d", > + err); ditto > + return err; > + } > + > + rp1->bar1 = pcim_iomap(pdev, 1, 0); > + if (!rp1->bar1) { > + dev_err(&pdev->dev, "Cannot map PCI BAR\n"); > + return -EIO; > + } > + > + pci_set_master(pdev); > + > + err = pci_alloc_irq_vectors(pdev, RP1_INT_END, RP1_INT_END, > + PCI_IRQ_MSIX); ditto > + if (err != RP1_INT_END) { an 'err' could be smaller then RP1_INT_END but still positive, thus .probe will return positive error code. > + dev_err(&pdev->dev, "pci_alloc_irq_vectors failed - %d\n", err); > + return err; > + } > + > + pci_set_drvdata(pdev, rp1); > + rp1->domain = irq_domain_add_linear(rp1_node, RP1_INT_END, > + &rp1_domain_ops, rp1); irq_domain_add_linear() could fail, check for error ... > + > + for (i = 0; i < RP1_INT_END; i++) { > + int irq = irq_create_mapping(rp1->domain, i); irq_create_mapping() returns 'unsigned int', maybe something like this: unsigned int irq = irq_create_mapping(rp1->domain, i); if (!irq) { err = -EINVAL; (or -ENOMEM) ... } > + > + if (irq < 0) { > + dev_err(&pdev->dev, "failed to create irq mapping\n"); > + err = irq; > + goto err_unload_overlay; goto err_unregister_interrupts; ? > + } new line, please > + irq_set_chip_and_handler(irq, &rp1_irq_chip, handle_level_irq); > + irq_set_probe(irq); > + irq_set_chained_handler_and_data(pci_irq_vector(pdev, i), > + rp1_chained_handle_irq, rp1); > + } > + > + err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id, rp1_node); > + if (err) > + goto err_unregister_interrupts; > + > + err = of_platform_default_populate(rp1_node, NULL, dev); > + if (err) > + goto err_unload_overlay; > + > + return 0; > + > +err_unload_overlay: > + of_overlay_remove(&rp1->ovcs_id); > +err_unregister_interrupts: > + rp1_unregister_interrupts(pdev); > + > + return err; > +} > + > +static void rp1_remove(struct pci_dev *pdev) > +{ > + struct rp1_dev *rp1 = pci_get_drvdata(pdev); > + struct device *dev = &pdev->dev; > + > + of_platform_depopulate(dev); > + of_overlay_remove(&rp1->ovcs_id); > + rp1_unregister_interrupts(pdev); > +} > + > +static const struct pci_device_id dev_id_table[] = { > + { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0), }, > + { 0, } > +}; > +MODULE_DEVICE_TABLE(pci, dev_id_table); > + > +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("Andrea della Porta <andrea.porta@suse.com>"); > +MODULE_DESCRIPTION("RP1 wrapper"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/misc/rp1/rp1_pci.h b/drivers/misc/rp1/rp1_pci.h > new file mode 100644 > index 000000000000..7982f13bad9b > --- /dev/null > +++ b/drivers/misc/rp1/rp1_pci.h > @@ -0,0 +1,14 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +/* > + * Copyright (c) 2018-24 Raspberry Pi Ltd. > + * All rights reserved. > + */ > + > +#ifndef _RP1_EXTERN_H_ > +#define _RP1_EXTERN_H_ > + > +extern char __dtbo_rp1_pci_begin[]; > +extern char __dtbo_rp1_pci_end[]; Could you point me where those two variables are used outside of rp1_pci.c? ~Stan
On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: > Hi Manivannan, > > On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > > bridge, the window should instead be in PCI address space. Call > > > pci_bus_address() on the resource in order to obtain the PCI bus > > > address. > > > > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > > bridges), right? > > Correct. Please note however that while the PCI-PCI bridge has the parent > address in CPU space, an endpoint device has it in PCI space: here we're > focusing on the bridge part. It probably used to work before since in many > cases the CPU and PCI address are the same, but it breaks down when they > differ. > When you say 'focusing', you are specifically referring to the bridge part of this API I believe. But I don't see a check for the bridge in your change, which is what concerning me. Am I missing something? - Mani
On Mon, Nov 04, 2024 at 08:35:21PM +0530, Manivannan Sadhasivam wrote: > On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: > > On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > > > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > > > bridge, the window should instead be in PCI address space. Call > > > > pci_bus_address() on the resource in order to obtain the PCI bus > > > > address. > > > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > > > bridges), right? > > > > Correct. Please note however that while the PCI-PCI bridge has the parent > > address in CPU space, an endpoint device has it in PCI space: here we're > > focusing on the bridge part. It probably used to work before since in many > > cases the CPU and PCI address are the same, but it breaks down when they > > differ. > > When you say 'focusing', you are specifically referring to the > bridge part of this API I believe. But I don't see a check for the > bridge in your change, which is what concerning me. Am I missing > something? I think we want this change for all devices in the PCI address domain, including PCI-PCI bridges and endpoints, don't we? All those "ranges" addresses should be in the PCI domain. Bjorn
Hi Stephen, On 15:20 Mon 28 Oct , Stephen Boyd wrote: > Quoting Andrea della Porta (2024-10-28 07:07:24) > > diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c > > new file mode 100644 > > index 000000000000..69b9cf037cb2 > > --- /dev/null > [...] > > + > > +struct rp1_clockman { > > + struct device *dev; > > + void __iomem *regs; > > Do you still need this if there's a regmap? Unfortunately the clk_divider registered in rp1_register_pll_divider() mandates the use of a MMIO registeri, and yes, the divider and teh other clock shares some registers. AFAICT there were attempts to upstream generic regmap support for clk_divider, but right now there are just custom implementations (e.g. drivers/clk/qcom/clk-regmap.c). So, in order to use regmap, that clock divider shoulf be first augmented with regmap support. Please let me know if you think it's worth the effort. > > > + struct regmap *regmap; > > + spinlock_t regs_lock; /* spinlock for all clocks */ > > Do you need this or is the spinlock in the regmap sufficient? The original code wrapped multiple registers write/read inside the spinlock, so I suspect that using the internal regmap spinlock for each single transaciton (so to open up for interleaved register access, although I have no evidence of that) could have side-effects so I would prefer to stay safe and manage the lock from outside. I could easily use regmap_multi_reg_write() and friends to synchronize access across multiple registers but then we would have the problem above about missing regmap support in clk_divider, since now it's solved by passing the same spinlock to it too. I'm open to alternatives here... > > > + > > + /* Must be last */ > > + struct clk_hw_onecell_data onecell; > > +}; > > + > > +struct rp1_pll_core_data { > > + const char *name; > > These 'name' members can move to clk_init_data? You've read my mind, I was exactly planning that for the next revision > > > + u32 cs_reg; > > + u32 pwr_reg; > > + u32 fbdiv_int_reg; > > + u32 fbdiv_frac_reg; > > + unsigned long flags; > > And probably flags as well? It seems like clk_init_data should be > declared at the same time as struct rp1_pll_core_data is. Ditto. > > > + u32 fc0_src; > > +}; > > + > > +struct rp1_pll_data { > > + const char *name; > > + u32 ctrl_reg; > > + unsigned long flags; > > + u32 fc0_src; > > +}; > > + > > +struct rp1_pll_ph_data { > > + const char *name; > > + unsigned int phase; > > + unsigned int fixed_divider; > > + u32 ph_reg; > > + unsigned long flags; > > + u32 fc0_src; > > +}; > > + > > +struct rp1_pll_divider_data { > > + const char *name; > > + u32 sec_reg; > > + unsigned long flags; > > + u32 fc0_src; > > +}; > > + > > +struct rp1_clock_data { > > + const char *name; > > + int num_std_parents; > > + int num_aux_parents; > > + unsigned long flags; > > + u32 oe_mask; > > + u32 clk_src_mask; > > + u32 ctrl_reg; > > + u32 div_int_reg; > > + u32 div_frac_reg; > > + u32 sel_reg; > > + u32 div_int_max; > > + unsigned long max_freq; > > + u32 fc0_src; > > +}; > > + > > +struct rp1_clk_desc { > > + struct clk_hw *(*clk_register)(struct rp1_clockman *clockman, > > + struct rp1_clk_desc *desc); > > + const void *data; > > + struct clk_hw hw; > > + struct rp1_clockman *clockman; > > + unsigned long cached_rate; > > + struct clk_divider div; > > +}; > > + > > +#define FIELD_SET(_reg, _mask, _val) \ > > +do { \ > > + u32 mask = (_mask); \ > > + (_reg) &= ~mask; \ > > + (_reg) |= FIELD_PREP(mask, (_val)); \ > > Please just write > > reg &= ~mask > reg |= FIELD_PREP(mask, val); > > instead of using this macro. Ack. > > > +} while (0) > > + > > + > [...] > > + > > +static struct clk_hw *rp1_register_pll_core(struct rp1_clockman *clockman, > > + struct rp1_clk_desc *desc) > > +{ > > + const struct rp1_pll_core_data *pll_core_data = desc->data; > > + struct clk_init_data init = { }; > > + int ret; > > + > > + /* All of the PLL cores derive from the external oscillator. */ > > + init.parent_data = desc->hw.init->parent_data; > > + init.num_parents = desc->hw.init->num_parents; > > + init.name = pll_core_data->name; > > + init.ops = &rp1_pll_core_ops; > > + init.flags = pll_core_data->flags | CLK_IGNORE_UNUSED | CLK_IS_CRITICAL; > > + > > + desc->clockman = clockman; > > + desc->hw.init = &init; > > + > > + ret = devm_clk_hw_register(clockman->dev, &desc->hw); > > + > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return &desc->hw; > > +} > > + > > +static struct clk_hw *rp1_register_pll(struct rp1_clockman *clockman, > > + struct rp1_clk_desc *desc) > > +{ > > + const struct rp1_pll_data *pll_data = desc->data; > > + struct clk_init_data init = { }; > > + int ret; > > + > > + init.parent_data = desc->hw.init->parent_data; > > + init.num_parents = desc->hw.init->num_parents; > > + init.name = pll_data->name; > > + init.ops = &rp1_pll_ops; > > + init.flags = pll_data->flags | CLK_IGNORE_UNUSED | CLK_IS_CRITICAL; > > + > > + desc->clockman = clockman; > > + desc->hw.init = &init; > > + > > + ret = devm_clk_hw_register(clockman->dev, &desc->hw); > > + > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return &desc->hw; > > +} > > + > > +static struct clk_hw *rp1_register_pll_ph(struct rp1_clockman *clockman, > > + struct rp1_clk_desc *desc) > > +{ > > + const struct rp1_pll_ph_data *ph_data = desc->data; > > + struct clk_init_data init = { }; > > + int ret; > > + > > + init.parent_data = desc->hw.init->parent_data; > > + init.num_parents = desc->hw.init->num_parents; > > + init.name = ph_data->name; > > + init.ops = &rp1_pll_ph_ops; > > + init.flags = ph_data->flags | CLK_IGNORE_UNUSED; > > + > > + desc->clockman = clockman; > > + desc->hw.init = &init; > > + > > + ret = devm_clk_hw_register(clockman->dev, &desc->hw); > > + > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return &desc->hw; > > +} > > + > > +static struct clk_hw *rp1_register_pll_divider(struct rp1_clockman *clockman, > > + struct rp1_clk_desc *desc) > > +{ > > + const struct rp1_pll_data *divider_data = desc->data; > > + struct clk_init_data init = { }; > > + int ret; > > + > > + init.parent_data = desc->hw.init->parent_data; > > + init.num_parents = desc->hw.init->num_parents; > > + init.name = divider_data->name; > > + init.ops = &rp1_pll_divider_ops; > > + init.flags = divider_data->flags | CLK_IGNORE_UNUSED | CLK_IS_CRITICAL; > > + > > + desc->div.reg = clockman->regs + divider_data->ctrl_reg; > > Why is 'regs' used here? Isn't everything using a regmap now so it's all > offsets? Already explained above. > > > + desc->div.shift = PLL_SEC_DIV_SHIFT; > > + desc->div.width = PLL_SEC_DIV_WIDTH; > > + desc->div.flags = CLK_DIVIDER_ROUND_CLOSEST; > > + desc->div.flags |= CLK_IS_CRITICAL; > > + desc->div.lock = &clockman->regs_lock; > > + desc->div.hw.init = &init; > > + desc->div.table = pll_sec_div_table; > > + > > + desc->clockman = clockman; > > + > > + ret = devm_clk_hw_register(clockman->dev, &desc->div.hw); > > + > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return &desc->div.hw; > > +} > > + > > +static struct clk_hw *rp1_register_clock(struct rp1_clockman *clockman, > > + struct rp1_clk_desc *desc) > > +{ > > + const struct rp1_clock_data *clock_data = desc->data; > > + struct clk_init_data init = { }; > > + int ret; > > + > > + if (WARN_ON_ONCE(MAX_CLK_PARENTS < > > + clock_data->num_std_parents + clock_data->num_aux_parents)) > > + return NULL; > > + > > + /* There must be a gap for the AUX selector */ > > + if (WARN_ON_ONCE(clock_data->num_std_parents > AUX_SEL && > > + desc->hw.init->parent_data[AUX_SEL].index != -1)) > > + return NULL; > > + > > + init.parent_data = desc->hw.init->parent_data; > > + init.num_parents = desc->hw.init->num_parents; > > + init.name = clock_data->name; > > + init.flags = clock_data->flags | CLK_IGNORE_UNUSED; > > + init.ops = &rp1_clk_ops; > > + > > + desc->clockman = clockman; > > + desc->hw.init = &init; > > + > > + ret = devm_clk_hw_register(clockman->dev, &desc->hw); > > + > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return &desc->hw; > > +} > > + > > +/* Assignment helper macros for different clock types. */ > > +#define _REGISTER(f, ...) { .clk_register = f, __VA_ARGS__ } > > + > > +#define PARENT_CLK(pnum, ...) .hw.init = &(const struct clk_init_data) { \ > > Instead of this macro just use CLK_HW_INIT_HW() or > CLK_HW_INIT_PARENTS_DATA()? Ack. > > > + .parent_data = (const struct \ > > + clk_parent_data[]) { \ > > + __VA_ARGS__ \ > > + }, \ > > + .num_parents = pnum } > > + > > +#define CLK_DATA(type, ...) .data = &(struct type) { __VA_ARGS__ } > > + > > +#define REGISTER_PLL_CORE(...) _REGISTER(&rp1_register_pll_core, \ > > + __VA_ARGS__) > > + > > +#define REGISTER_PLL(...) _REGISTER(&rp1_register_pll, \ > > + __VA_ARGS__) > > + > > +#define REGISTER_PLL_PH(...) _REGISTER(&rp1_register_pll_ph, \ > > + __VA_ARGS__) > > + > > +#define REGISTER_PLL_DIV(...) _REGISTER(&rp1_register_pll_divider, \ > > + __VA_ARGS__) > > + > > +#define REGISTER_CLK(...) _REGISTER(&rp1_register_clock, \ > > + __VA_ARGS__) > > + > > +static struct rp1_clk_desc clk_desc_array[] = { > > + [RP1_PLL_SYS_CORE] = REGISTER_PLL_CORE(PARENT_CLK(1, { .index = 0 }), > > + CLK_DATA(rp1_pll_core_data, > > + .name = "pll_sys_core", > > + .cs_reg = PLL_SYS_CS, > > + .pwr_reg = PLL_SYS_PWR, > > + .fbdiv_int_reg = PLL_SYS_FBDIV_INT, > > + .fbdiv_frac_reg = PLL_SYS_FBDIV_FRAC, > > + )), > > + > > + [RP1_PLL_AUDIO_CORE] = REGISTER_PLL_CORE(PARENT_CLK(1, { .index = 0 }), > > + CLK_DATA(rp1_pll_core_data, > > + .name = "pll_audio_core", > > + .cs_reg = PLL_AUDIO_CS, > > + .pwr_reg = PLL_AUDIO_PWR, > > + .fbdiv_int_reg = PLL_AUDIO_FBDIV_INT, > > + .fbdiv_frac_reg = PLL_AUDIO_FBDIV_FRAC, > > + )), > > + > > + [RP1_PLL_VIDEO_CORE] = REGISTER_PLL_CORE(PARENT_CLK(1, { .index = 0 }), > > + CLK_DATA(rp1_pll_core_data, > > + .name = "pll_video_core", > > + .cs_reg = PLL_VIDEO_CS, > > + .pwr_reg = PLL_VIDEO_PWR, > > + .fbdiv_int_reg = PLL_VIDEO_FBDIV_INT, > > + .fbdiv_frac_reg = PLL_VIDEO_FBDIV_FRAC, > > + )), > > + > > + [RP1_PLL_SYS] = REGISTER_PLL(PARENT_CLK(1, > > + { .hw = &clk_desc_array[RP1_PLL_SYS_CORE].hw } > > + ), > > + CLK_DATA(rp1_pll_data, > > + .name = "pll_sys", > > + .ctrl_reg = PLL_SYS_PRIM, > > + .fc0_src = FC_NUM(0, 2), > > + )), > > + > > + [RP1_CLK_ETH_TSU] = REGISTER_CLK(PARENT_CLK(1, { .index = 0 }), > > + CLK_DATA(rp1_clock_data, > > + .name = "clk_eth_tsu", > > + .num_std_parents = 0, > > + .num_aux_parents = 1, > > + .ctrl_reg = CLK_ETH_TSU_CTRL, > > + .div_int_reg = CLK_ETH_TSU_DIV_INT, > > + .sel_reg = CLK_ETH_TSU_SEL, > > + .div_int_max = DIV_INT_8BIT_MAX, > > + .max_freq = 50 * HZ_PER_MHZ, > > + .fc0_src = FC_NUM(5, 7), > > + )), > > + > > + [RP1_CLK_SYS] = REGISTER_CLK(PARENT_CLK(3, > > + { .index = 0 }, > > + { .index = -1 }, > > + { .hw = &clk_desc_array[RP1_PLL_SYS].hw } > > + ), > > + CLK_DATA(rp1_clock_data, > > + .name = "clk_sys", > > + .num_std_parents = 3, > > + .num_aux_parents = 0, > > + .ctrl_reg = CLK_SYS_CTRL, > > + .div_int_reg = CLK_SYS_DIV_INT, > > + .sel_reg = CLK_SYS_SEL, > > + .div_int_max = DIV_INT_24BIT_MAX, > > + .max_freq = 200 * HZ_PER_MHZ, > > + .fc0_src = FC_NUM(0, 4), > > + .clk_src_mask = 0x3, > > + )), > > + > > + [RP1_PLL_SYS_PRI_PH] = REGISTER_PLL_PH(PARENT_CLK(1, > > + { .hw = &clk_desc_array[RP1_PLL_SYS].hw } > > + ), > > + CLK_DATA(rp1_pll_ph_data, > > + .name = "pll_sys_pri_ph", > > + .ph_reg = PLL_SYS_PRIM, > > + .fixed_divider = 2, > > + .phase = RP1_PLL_PHASE_0, > > + .fc0_src = FC_NUM(1, 2), > > + )), > > + > > + [RP1_PLL_SYS_SEC] = REGISTER_PLL_DIV(PARENT_CLK(1, > > + { .hw = &clk_desc_array[RP1_PLL_SYS_CORE].hw } > > + ), > > + CLK_DATA(rp1_pll_data, > > + .name = "pll_sys_sec", > > + .ctrl_reg = PLL_SYS_SEC, > > + .fc0_src = FC_NUM(2, 2), > > + )), > > +}; > > + > > +static const struct regmap_range rp1_reg_ranges[] = { > > + regmap_reg_range(PLL_SYS_CS, PLL_SYS_SEC), > > + regmap_reg_range(PLL_AUDIO_CS, PLL_AUDIO_TERN), > > + regmap_reg_range(PLL_VIDEO_CS, PLL_VIDEO_SEC), > > + regmap_reg_range(GPCLK_OE_CTRL, GPCLK_OE_CTRL), > > + regmap_reg_range(CLK_SYS_CTRL, CLK_SYS_DIV_INT), > > + regmap_reg_range(CLK_SYS_SEL, CLK_SYS_SEL), > > + regmap_reg_range(CLK_SLOW_SYS_CTRL, CLK_SLOW_SYS_DIV_INT), > > + regmap_reg_range(CLK_SLOW_SYS_SEL, CLK_SLOW_SYS_SEL), > > + regmap_reg_range(CLK_DMA_CTRL, CLK_DMA_DIV_INT), > > + regmap_reg_range(CLK_DMA_SEL, CLK_DMA_SEL), > > + regmap_reg_range(CLK_UART_CTRL, CLK_UART_DIV_INT), > > + regmap_reg_range(CLK_UART_SEL, CLK_UART_SEL), > > + regmap_reg_range(CLK_ETH_CTRL, CLK_ETH_DIV_INT), > > + regmap_reg_range(CLK_ETH_SEL, CLK_ETH_SEL), > > + regmap_reg_range(CLK_PWM0_CTRL, CLK_PWM0_SEL), > > + regmap_reg_range(CLK_PWM1_CTRL, CLK_PWM1_SEL), > > + regmap_reg_range(CLK_AUDIO_IN_CTRL, CLK_AUDIO_IN_DIV_INT), > > + regmap_reg_range(CLK_AUDIO_IN_SEL, CLK_AUDIO_IN_SEL), > > + regmap_reg_range(CLK_AUDIO_OUT_CTRL, CLK_AUDIO_OUT_DIV_INT), > > + regmap_reg_range(CLK_AUDIO_OUT_SEL, CLK_AUDIO_OUT_SEL), > > + regmap_reg_range(CLK_I2S_CTRL, CLK_I2S_DIV_INT), > > + regmap_reg_range(CLK_I2S_SEL, CLK_I2S_SEL), > > + regmap_reg_range(CLK_MIPI0_CFG_CTRL, CLK_MIPI0_CFG_DIV_INT), > > + regmap_reg_range(CLK_MIPI0_CFG_SEL, CLK_MIPI0_CFG_SEL), > > + regmap_reg_range(CLK_MIPI1_CFG_CTRL, CLK_MIPI1_CFG_DIV_INT), > > + regmap_reg_range(CLK_MIPI1_CFG_SEL, CLK_MIPI1_CFG_SEL), > > + regmap_reg_range(CLK_PCIE_AUX_CTRL, CLK_PCIE_AUX_DIV_INT), > > + regmap_reg_range(CLK_PCIE_AUX_SEL, CLK_PCIE_AUX_SEL), > > + regmap_reg_range(CLK_USBH0_MICROFRAME_CTRL, CLK_USBH0_MICROFRAME_DIV_INT), > > + regmap_reg_range(CLK_USBH0_MICROFRAME_SEL, CLK_USBH0_MICROFRAME_SEL), > > + regmap_reg_range(CLK_USBH1_MICROFRAME_CTRL, CLK_USBH1_MICROFRAME_DIV_INT), > > + regmap_reg_range(CLK_USBH1_MICROFRAME_SEL, CLK_USBH1_MICROFRAME_SEL), > > + regmap_reg_range(CLK_USBH0_SUSPEND_CTRL, CLK_USBH0_SUSPEND_DIV_INT), > > + regmap_reg_range(CLK_USBH0_SUSPEND_SEL, CLK_USBH0_SUSPEND_SEL), > > + regmap_reg_range(CLK_USBH1_SUSPEND_CTRL, CLK_USBH1_SUSPEND_DIV_INT), > > + regmap_reg_range(CLK_USBH1_SUSPEND_SEL, CLK_USBH1_SUSPEND_SEL), > > + regmap_reg_range(CLK_ETH_TSU_CTRL, CLK_ETH_TSU_DIV_INT), > > + regmap_reg_range(CLK_ETH_TSU_SEL, CLK_ETH_TSU_SEL), > > + regmap_reg_range(CLK_ADC_CTRL, CLK_ADC_DIV_INT), > > + regmap_reg_range(CLK_ADC_SEL, CLK_ADC_SEL), > > + regmap_reg_range(CLK_SDIO_TIMER_CTRL, CLK_SDIO_TIMER_DIV_INT), > > + regmap_reg_range(CLK_SDIO_TIMER_SEL, CLK_SDIO_TIMER_SEL), > > + regmap_reg_range(CLK_SDIO_ALT_SRC_CTRL, CLK_SDIO_ALT_SRC_DIV_INT), > > + regmap_reg_range(CLK_SDIO_ALT_SRC_SEL, CLK_SDIO_ALT_SRC_SEL), > > + regmap_reg_range(CLK_GP0_CTRL, CLK_GP0_SEL), > > + regmap_reg_range(CLK_GP1_CTRL, CLK_GP1_SEL), > > + regmap_reg_range(CLK_GP2_CTRL, CLK_GP2_SEL), > > + regmap_reg_range(CLK_GP3_CTRL, CLK_GP3_SEL), > > + regmap_reg_range(CLK_GP4_CTRL, CLK_GP4_SEL), > > + regmap_reg_range(CLK_GP5_CTRL, CLK_GP5_SEL), > > + regmap_reg_range(CLK_SYS_RESUS_CTRL, CLK_SYS_RESUS_CTRL), > > + regmap_reg_range(CLK_SLOW_SYS_RESUS_CTRL, CLK_SLOW_SYS_RESUS_CTRL), > > + regmap_reg_range(FC0_REF_KHZ, FC0_RESULT), > > + regmap_reg_range(VIDEO_CLK_VEC_CTRL, VIDEO_CLK_VEC_DIV_INT), > > + regmap_reg_range(VIDEO_CLK_VEC_SEL, VIDEO_CLK_DPI_DIV_INT), > > + regmap_reg_range(VIDEO_CLK_DPI_SEL, VIDEO_CLK_MIPI1_DPI_SEL), > > +}; > > + > > +static const struct regmap_access_table rp1_reg_table = { > > + .yes_ranges = rp1_reg_ranges, > > + .n_yes_ranges = ARRAY_SIZE(rp1_reg_ranges), > > +}; > > + > > +static const struct regmap_config rp1_clk_regmap_cfg = { > > + .reg_bits = 32, > > + .val_bits = 32, > > + .reg_stride = 4, > > + .max_register = PLL_VIDEO_SEC, > > + .name = "rp1-clk", > > + .rd_table = &rp1_reg_table, > > +}; > > + > > +static int rp1_clk_probe(struct platform_device *pdev) > > +{ > > + const size_t asize = ARRAY_SIZE(clk_desc_array); > > + struct rp1_clk_desc *desc; > > + struct device *dev = &pdev->dev; > > + struct rp1_clockman *clockman; > > + struct clk_hw **hws; > > + unsigned int i; > > + > > + clockman = devm_kzalloc(dev, struct_size(clockman, onecell.hws, asize), > > + GFP_KERNEL); > > + if (!clockman) > > + return -ENOMEM; > > + > > + spin_lock_init(&clockman->regs_lock); > > + clockman->dev = dev; > > + > > + clockman->regs = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(clockman->regs)) > > + return PTR_ERR(clockman->regs); > > + > > + clockman->regmap = devm_regmap_init_mmio(dev, clockman->regs, > > + &rp1_clk_regmap_cfg); > > + if (IS_ERR(clockman->regmap)) { > > + dev_err(dev, "could not init clock regmap\n"); > > return dev_err_probe()? Ack. Many thanks, Andrea > > > + return PTR_ERR(clockman->regmap); > > + } > > + > > + clockman->onecell.num = asize; > > + hws = clockman->onecell.hws; > > + > > + for (i = 0; i < asize; i++) { > > + desc = &clk_desc_array[i]; > > + if (desc->clk_register && desc->data) { > > + hws[i] = desc->clk_register(clockman, desc); > > + if (IS_ERR_OR_NULL(hws[i])) > > + dev_err_probe(dev, PTR_ERR(hws[i]), > > + "Unable to register clock: %s\n", > > + clk_hw_get_name(hws[i])); > > + } > > + } > > + > > + platform_set_drvdata(pdev, clockman); > > + > > + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > > + &clockman->onecell); > > +}
Hi Stan, On 15:29 Mon 04 Nov , Stanimir Varbanov wrote: > Hi Andrea, > > On 10/28/24 16:07, Andrea della Porta wrote: > > RaspberryPi RP1 is a multi function PCI endpoint device that > > exposes several subperipherals via PCI BAR. > > Add a dtb overlay that will be compiled into a binary blob > > and linked in the RP1 driver. > > This overlay offers just minimal support to represent the > > RP1 device itself, the sub-peripherals will be added by > > future patches. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > NOTE: this patch should be taken by the same maintainer that will take > > "[PATCH v3 10/12] misc: rp1: RaspberryPi RP1 misc driver", since they > > are closely related in terms of compiling. > > > > MAINTAINERS | 1 + > > arch/arm64/boot/dts/broadcom/rp1.dtso | 61 +++++++++++++++++++++++++++ > > 2 files changed, 62 insertions(+) > > create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 06277969a522..510a071ede78 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -19383,6 +19383,7 @@ 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/misc/pci1de4,1.yaml > > F: Documentation/devicetree/bindings/pci/pci-ep-bus.yaml > > diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > > new file mode 100644 > > index 000000000000..8d1bbf207a30 > > --- /dev/null > > +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > > @@ -0,0 +1,61 @@ > > +// 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/raspberrypi,rp1-clocks.h> > > + > > +/dts-v1/; > > +/plugin/; > > + > > +/ { > > + fragment@0 { > > + target-path=""; > > + __overlay__ { > > + compatible = "pci1de4,1"; > > + #address-cells = <3>; > > + #size-cells = <2>; > > + interrupt-controller; > > + #interrupt-cells = <2>; > > + > > + pci_ep_bus: pci-ep-bus@1 { > > + compatible = "simple-bus"; > > + ranges = <0xc0 0x40000000 > > + 0x01 0x00 0x00000000 > > + 0x00 0x00400000>; > > + dma-ranges = <0x10 0x00000000 > > + 0x43000000 0x10 0x00000000 > > + 0x10 0x00000000>; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + rp1_clocks: clocks@c040018000 { > > + compatible = "raspberrypi,rp1-clocks"; > > + reg = <0xc0 0x40018000 0x0 0x10038>; > > shouldn't this be: > > rp1_clocks: clocks@18000 { > reg = <0x00 0x00018000 0x0 0x10038>; > ... > } > > ? > > And for other nodes too... For that to be @18000 instead of @c040018000, you should also change the "ranges" entry in pci-ep-bus node, as follows: ranges = <0x00 0x00018000 //This was: 0xc0 0x40000000 0x01 0x00 0x00000000 0x00 0x00400000>; which is of course feasible, but I prefer to use addresses that resemble (at least to some extent) the ones in RP1 docs. Many thanks, Andrea > > ~Stan > > > + #clock-cells = <1>; > > + clocks = <&clk_rp1_xosc>; > > + clock-names = "xosc"; > > + assigned-clocks = <&rp1_clocks RP1_PLL_SYS_CORE>, > > + <&rp1_clocks RP1_PLL_SYS>, > > + <&rp1_clocks RP1_CLK_SYS>; > > + assigned-clock-rates = <1000000000>, // RP1_PLL_SYS_CORE > > + <200000000>, // RP1_PLL_SYS > > + <200000000>; // RP1_CLK_SYS > > + }; > > + > > + rp1_gpio: pinctrl@c0400d0000 { > > + compatible = "raspberrypi,rp1-gpio"; > > + reg = <0xc0 0x400d0000 0x0 0xc000>, > > + <0xc0 0x400e0000 0x0 0xc000>, > > + <0xc0 0x400f0000 0x0 0xc000>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + interrupt-controller; > > + #interrupt-cells = <2>; > > + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>, > > + <1 IRQ_TYPE_LEVEL_HIGH>, > > + <2 IRQ_TYPE_LEVEL_HIGH>; > > + }; > > + }; > > + }; > > + }; > > +}; >
Hi Stan, On 15:43 Mon 04 Nov , Stanimir Varbanov wrote: > On 10/28/24 16:07, Andrea della Porta wrote: > > The RaspberryPi RP1 is a PCI multi function device containing > > peripherals ranging from Ethernet to USB controller, I2C, SPI > > and others. > > > > Implement a bare minimum driver to operate the RP1, leveraging > > actual OF based driver implementations for the on-board peripherals > > by loading a devicetree overlay during driver probe. > > > > The peripherals are accessed by mapping MMIO registers starting > > from PCI BAR1 region. > > > > With the overlay approach we can achieve more generic and agnostic > > approach to managing this chipset, being that it is a PCI endpoint > > and could possibly be reused in other hw implementations. The > > presented approach is also used by Bootlin's Microchip LAN966x > > patchset (see link) as well, for a similar chipset. > > > > Since the gpio line names should be provided by the user, there > > is an interface through configfs that allows the userspace to > > load a DT overlay that will provide gpio-line-names property. > > The interface can be invoked like this: > > > > cat rpi-rp1-gpios-5-b.dtbo > /sys/kernel/config/rp1-cfg/gpio_set_names > > > > and is designed to be similar to what users are already used to > > from distro with downstream kernel. > > > > For reasons why this driver is contained in drivers/misc, please > > check the links. > > > > This driver is heavily based on downstream code from RaspberryPi > > Foundation, and the original author is Phil Elwell. > > > > Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > > Link: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ > > Link: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ > > Link: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ > > Link: https://lore.kernel.org/all/20240808154658.247873-1-herve.codina@bootlin.com/ > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > MAINTAINERS | 1 + > > drivers/misc/Kconfig | 1 + > > drivers/misc/Makefile | 1 + > > drivers/misc/rp1/Kconfig | 21 ++ > > drivers/misc/rp1/Makefile | 3 + > > drivers/misc/rp1/rp1-pci.dtso | 8 + > > drivers/misc/rp1/rp1_pci.c | 357 ++++++++++++++++++++++++++++++++++ > > drivers/misc/rp1/rp1_pci.h | 14 ++ > > drivers/pci/quirks.c | 1 + > > include/linux/pci_ids.h | 3 + > > 10 files changed, 410 insertions(+) > > create mode 100644 drivers/misc/rp1/Kconfig > > create mode 100644 drivers/misc/rp1/Makefile > > create mode 100644 drivers/misc/rp1/rp1-pci.dtso > > create mode 100644 drivers/misc/rp1/rp1_pci.c > > create mode 100644 drivers/misc/rp1/rp1_pci.h > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 510a071ede78..032678fb2470 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -19389,6 +19389,7 @@ F: Documentation/devicetree/bindings/misc/pci1de4,1.yaml > > F: Documentation/devicetree/bindings/pci/pci-ep-bus.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/drivers/misc/Kconfig b/drivers/misc/Kconfig > > index 3fe7e2a9bd29..ac85cb154100 100644 > > --- a/drivers/misc/Kconfig > > +++ b/drivers/misc/Kconfig > > @@ -628,4 +628,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 a9f94525e181..ae86d69997b4 100644 > > --- a/drivers/misc/Makefile > > +++ b/drivers/misc/Makefile > > @@ -72,3 +72,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..39c4aa4bf634 > > --- /dev/null > > +++ b/drivers/misc/rp1/Kconfig > > @@ -0,0 +1,21 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +# > > +# RaspberryPi RP1 misc device > > +# > > + > > +config MISC_RP1 > > + tristate "RaspberryPi RP1 PCIe support" > > + depends on OF_IRQ && PCI_MSI && PCI_QUIRKS > > + select OF_OVERLAY > > + select PCI_DYNAMIC_OF_NODES > > + help > > + Support 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..508b4cb05627 > > --- /dev/null > > +++ b/drivers/misc/rp1/Makefile > > @@ -0,0 +1,3 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +obj-$(CONFIG_MISC_RP1) += rp1-pci.o > > +rp1-pci-objs := rp1_pci.o rp1-pci.dtbo.o > > diff --git a/drivers/misc/rp1/rp1-pci.dtso b/drivers/misc/rp1/rp1-pci.dtso > > new file mode 100644 > > index 000000000000..0bf2f4bb18e6 > > --- /dev/null > > +++ b/drivers/misc/rp1/rp1-pci.dtso > > @@ -0,0 +1,8 @@ > > +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > > + > > +/* the dts overlay is included from the dts directory so > > + * it can be possible to check it with CHECK_DTBS while > > + * also compile it from the driver source directory. > > + */ > > + > > +#include "arm64/broadcom/rp1.dtso" > > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c > > new file mode 100644 > > index 000000000000..30d7a15dc7f1 > > --- /dev/null > > +++ b/drivers/misc/rp1/rp1_pci.c > > @@ -0,0 +1,357 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018-24 Raspberry Pi Ltd. > > + * All rights reserved. > > + */ > > + > > +#include <linux/err.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/irqchip/chained_irq.h> > > +#include <linux/irqdomain.h> > > +#include <linux/module.h> > > +#include <linux/msi.h> > > +#include <linux/of_platform.h> > > +#include <linux/pci.h> > > +#include <linux/platform_device.h> > > + > > +#include "rp1_pci.h" > > + > > +#define RP1_DRIVER_NAME "rp1" > > + > > +#define RP1_HW_IRQ_MASK GENMASK(5, 0) > > + > > +#define REG_SET 0x800 > > +#define REG_CLR 0xc00 > > + > > +/* MSI-X 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_ENABLE BIT(0) > > + > > +/* Address map */ > > +#define RP1_PCIE_APBS_BASE 0x108000 > > + > > +/* Interrupts */ > > +#define RP1_INT_IO_BANK0 0 > > +#define RP1_INT_IO_BANK1 1 > > +#define RP1_INT_IO_BANK2 2 > > +#define RP1_INT_AUDIO_IN 3 > > +#define RP1_INT_AUDIO_OUT 4 > > +#define RP1_INT_PWM0 5 > > +#define RP1_INT_ETH 6 > > +#define RP1_INT_I2C0 7 > > +#define RP1_INT_I2C1 8 > > +#define RP1_INT_I2C2 9 > > +#define RP1_INT_I2C3 10 > > +#define RP1_INT_I2C4 11 > > +#define RP1_INT_I2C5 12 > > +#define RP1_INT_I2C6 13 > > +#define RP1_INT_I2S0 14 > > +#define RP1_INT_I2S1 15 > > +#define RP1_INT_I2S2 16 > > +#define RP1_INT_SDIO0 17 > > +#define RP1_INT_SDIO1 18 > > +#define RP1_INT_SPI0 19 > > +#define RP1_INT_SPI1 20 > > +#define RP1_INT_SPI2 21 > > +#define RP1_INT_SPI3 22 > > +#define RP1_INT_SPI4 23 > > +#define RP1_INT_SPI5 24 > > +#define RP1_INT_UART0 25 > > +#define RP1_INT_TIMER_0 26 > > +#define RP1_INT_TIMER_1 27 > > +#define RP1_INT_TIMER_2 28 > > +#define RP1_INT_TIMER_3 29 > > +#define RP1_INT_USBHOST0 30 > > +#define RP1_INT_USBHOST0_0 31 > > +#define RP1_INT_USBHOST0_1 32 > > +#define RP1_INT_USBHOST0_2 33 > > +#define RP1_INT_USBHOST0_3 34 > > +#define RP1_INT_USBHOST1 35 > > +#define RP1_INT_USBHOST1_0 36 > > +#define RP1_INT_USBHOST1_1 37 > > +#define RP1_INT_USBHOST1_2 38 > > +#define RP1_INT_USBHOST1_3 39 > > +#define RP1_INT_DMA 40 > > +#define RP1_INT_PWM1 41 > > +#define RP1_INT_UART1 42 > > +#define RP1_INT_UART2 43 > > +#define RP1_INT_UART3 44 > > +#define RP1_INT_UART4 45 > > +#define RP1_INT_UART5 46 > > +#define RP1_INT_MIPI0 47 > > +#define RP1_INT_MIPI1 48 > > +#define RP1_INT_VIDEO_OUT 49 > > +#define RP1_INT_PIO_0 50 > > +#define RP1_INT_PIO_1 51 > > +#define RP1_INT_ADC_FIFO 52 > > +#define RP1_INT_PCIE_OUT 53 > > +#define RP1_INT_SPI6 54 > > +#define RP1_INT_SPI7 55 > > +#define RP1_INT_SPI8 56 > > +#define RP1_INT_SYSCFG 58 > > +#define RP1_INT_CLOCKS_DEFAULT 59 > > +#define RP1_INT_VBUSCTRL 60 > > +#define RP1_INT_PROC_MISC 57 > > +#define RP1_INT_END 61 > > + > > How those interrupts are supposed to be used by rp1-pci.dtso (or from > rp1.dtso)? Unfortunately they will not. See below. > > include/dt-bindings/interrupt-controller/raspberrypi,rp1-interrupts.h ? Please see: https://lore.kernel.org/all/e05705c1-95dc-4d77-8a0d-8c2a785b0b05@kernel.org/ > > > +struct rp1_dev { > > + struct pci_dev *pdev; > > + struct device *dev; > > + struct irq_domain *domain; > > + struct irq_data *pcie_irqds[64]; > > + void __iomem *bar1; > > + int ovcs_id; > > + bool level_triggered_irq[RP1_INT_END]; > > +}; > > + > > +static void msix_cfg_set(struct rp1_dev *rp1, unsigned int hwirq, u32 value) > > +{ > > + iowrite32(value, rp1->bar1 + RP1_PCIE_APBS_BASE + REG_SET + MSIX_CFG(hwirq)); > > +} > > + > > +static void msix_cfg_clr(struct rp1_dev *rp1, unsigned int hwirq, u32 value) > > +{ > > + iowrite32(value, rp1->bar1 + RP1_PCIE_APBS_BASE + REG_CLR + MSIX_CFG(hwirq)); > > +} > > + > > +static void rp1_mask_irq(struct irq_data *irqd) > > +{ > > + struct rp1_dev *rp1 = irqd->domain->host_data; > > + struct irq_data *pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; > > + > > + pci_msi_mask_irq(pcie_irqd); > > +} > > + > > +static void rp1_unmask_irq(struct irq_data *irqd) > > +{ > > + struct rp1_dev *rp1 = irqd->domain->host_data; > > + struct irq_data *pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; > > + > > + pci_msi_unmask_irq(pcie_irqd); > > +} > > + > > +static int rp1_irq_set_type(struct irq_data *irqd, unsigned int type) > > +{ > > + struct rp1_dev *rp1 = irqd->domain->host_data; > > + unsigned int hwirq = (unsigned int)irqd->hwirq; > > + > > + switch (type) { > > + case IRQ_TYPE_LEVEL_HIGH: > > + dev_dbg(rp1->dev, "MSIX IACK EN for irq %d\n", hwirq); > > + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK_EN); > > + rp1->level_triggered_irq[hwirq] = true; > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + msix_cfg_clr(rp1, hwirq, MSIX_CFG_IACK_EN); > > + rp1->level_triggered_irq[hwirq] = false; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +static struct irq_chip rp1_irq_chip = { > > + .name = "rp1_irq_chip", > > tabs instead of spaces here and below. Ack. > > > + .irq_mask = rp1_mask_irq, > > + .irq_unmask = rp1_unmask_irq, > > + .irq_set_type = rp1_irq_set_type, > > +}; > > + > > +static void rp1_chained_handle_irq(struct irq_desc *desc) > > +{ > > + unsigned int hwirq = desc->irq_data.hwirq & RP1_HW_IRQ_MASK; > > + struct rp1_dev *rp1 = irq_desc_get_handler_data(desc); > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > + int virq; > > unsigned int Ack. > > > + > > + chained_irq_enter(chip, desc); > > + > > + virq = irq_find_mapping(rp1->domain, hwirq); > > + generic_handle_irq(virq); > > + if (rp1->level_triggered_irq[hwirq]) > > + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK); > > + > > + chained_irq_exit(chip, desc); > > +} > > + > > +static int rp1_irq_xlate(struct irq_domain *d, struct device_node *node, > > + const u32 *intspec, unsigned int intsize, > > + unsigned long *out_hwirq, unsigned int *out_type) > > +{ > > + struct rp1_dev *rp1 = d->host_data; > > + struct irq_data *pcie_irqd; > > + unsigned long hwirq; > > + int pcie_irq; > > + int ret; > > + > > + ret = irq_domain_xlate_twocell(d, node, intspec, intsize, > > + &hwirq, out_type); > > + if (ret) > > + return ret; > > + > > + pcie_irq = pci_irq_vector(rp1->pdev, hwirq); > > + pcie_irqd = irq_get_irq_data(pcie_irq); > > + rp1->pcie_irqds[hwirq] = pcie_irqd; > > + *out_hwirq = hwirq; > > + > > + return 0; > > +} > > + > > +static int rp1_irq_activate(struct irq_domain *d, struct irq_data *irqd, > > + bool reserve) > > +{ > > + struct rp1_dev *rp1 = d->host_data; > > + > > + msix_cfg_set(rp1, (unsigned int)irqd->hwirq, MSIX_CFG_ENABLE); > > + > > + return 0; > > +} > > + > > +static void rp1_irq_deactivate(struct irq_domain *d, struct irq_data *irqd) > > +{ > > + struct rp1_dev *rp1 = d->host_data; > > + > > + msix_cfg_clr(rp1, (unsigned int)irqd->hwirq, MSIX_CFG_ENABLE); > > +} > > + > > +static const struct irq_domain_ops rp1_domain_ops = { > > + .xlate = rp1_irq_xlate, > > + .activate = rp1_irq_activate, > > + .deactivate = rp1_irq_deactivate, > > +}; > > + > > +static void rp1_unregister_interrupts(struct pci_dev *pdev) > > +{ > > + struct rp1_dev *rp1 = pci_get_drvdata(pdev); > > + int irq, i; > > + > > + for (i = 0; i < RP1_INT_END; i++) { > > + irq = irq_find_mapping(rp1->domain, i); > > + irq_dispose_mapping(irq); > > + } > > new line Ack. > > > + irq_domain_remove(rp1->domain); > > + pci_free_irq_vectors(pdev); > > +} > > + > > +static int rp1_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > +{ > > + u32 dtbo_size = __dtbo_rp1_pci_end - __dtbo_rp1_pci_begin; > > + void *dtbo_start = __dtbo_rp1_pci_begin; > > + struct device *dev = &pdev->dev; > > + struct device_node *rp1_node; > > + struct rp1_dev *rp1; > > + int err = 0; > > + int i; > > + > > + rp1_node = dev_of_node(dev); > > + if (!rp1_node) { > > + dev_err(dev, "Missing of_node for device\n"); > > + return -EINVAL; > > + } > > + > > + rp1 = devm_kzalloc(&pdev->dev, sizeof(*rp1), GFP_KERNEL); > > + if (!rp1) > > + return -ENOMEM; > > + > > + rp1->pdev = pdev; > > + rp1->dev = &pdev->dev; > > rp1->dev is used only once in dev_dbg macro. You could drop it and > extract struct device from rp1->pdev. Good catch, thanks! > > > + > > + if (pci_resource_len(pdev, 1) <= 0x10000) { > > + dev_err(&pdev->dev, > > + "Not initialised - is the firmware running?\n"); > > no need of a new line It goes past 80 columns, no? > > > + return -EINVAL; > > + } > > + > > + err = pcim_enable_device(pdev); > > + if (err < 0) { > > + dev_err(&pdev->dev, "Enabling PCI device has failed: %d", > > + err); > > ditto Ditto. > > > + return err; > > + } > > + > > + rp1->bar1 = pcim_iomap(pdev, 1, 0); > > + if (!rp1->bar1) { > > + dev_err(&pdev->dev, "Cannot map PCI BAR\n"); > > + return -EIO; > > + } > > + > > + pci_set_master(pdev); > > + > > + err = pci_alloc_irq_vectors(pdev, RP1_INT_END, RP1_INT_END, > > + PCI_IRQ_MSIX); > > ditto Ditto. > > > + if (err != RP1_INT_END) { > > an 'err' could be smaller then RP1_INT_END but still positive, thus > .probe will return positive error code. Ack. > > > + dev_err(&pdev->dev, "pci_alloc_irq_vectors failed - %d\n", err); > > + return err; > > + } > > + > > + pci_set_drvdata(pdev, rp1); > > + rp1->domain = irq_domain_add_linear(rp1_node, RP1_INT_END, > > + &rp1_domain_ops, rp1); > > irq_domain_add_linear() could fail, check for error ... Ack. > > > + > > + for (i = 0; i < RP1_INT_END; i++) { > > + int irq = irq_create_mapping(rp1->domain, i); > > irq_create_mapping() returns 'unsigned int', maybe something like this: > > unsigned int irq = irq_create_mapping(rp1->domain, i); > if (!irq) { > err = -EINVAL; (or -ENOMEM) > ... > } Right, thanks. > > > + > > + if (irq < 0) { > > + dev_err(&pdev->dev, "failed to create irq mapping\n"); > > + err = irq; > > + goto err_unload_overlay; > > goto err_unregister_interrupts; ? Ack. > > > + } > > new line, please Ack. > > > + irq_set_chip_and_handler(irq, &rp1_irq_chip, handle_level_irq); > > + irq_set_probe(irq); > > + irq_set_chained_handler_and_data(pci_irq_vector(pdev, i), > > + rp1_chained_handle_irq, rp1); > > + } > > + > > + err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id, rp1_node); > > + if (err) > > + goto err_unregister_interrupts; > > + > > + err = of_platform_default_populate(rp1_node, NULL, dev); > > + if (err) > > + goto err_unload_overlay; > > + > > + return 0; > > + > > +err_unload_overlay: > > + of_overlay_remove(&rp1->ovcs_id); > > +err_unregister_interrupts: > > + rp1_unregister_interrupts(pdev); > > + > > + return err; > > +} > > + > > +static void rp1_remove(struct pci_dev *pdev) > > +{ > > + struct rp1_dev *rp1 = pci_get_drvdata(pdev); > > + struct device *dev = &pdev->dev; > > + > > + of_platform_depopulate(dev); > > + of_overlay_remove(&rp1->ovcs_id); > > + rp1_unregister_interrupts(pdev); > > +} > > + > > +static const struct pci_device_id dev_id_table[] = { > > + { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0), }, > > + { 0, } > > +}; > > +MODULE_DEVICE_TABLE(pci, dev_id_table); > > + > > +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("Andrea della Porta <andrea.porta@suse.com>"); > > +MODULE_DESCRIPTION("RP1 wrapper"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/misc/rp1/rp1_pci.h b/drivers/misc/rp1/rp1_pci.h > > new file mode 100644 > > index 000000000000..7982f13bad9b > > --- /dev/null > > +++ b/drivers/misc/rp1/rp1_pci.h > > @@ -0,0 +1,14 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > + > > +/* > > + * Copyright (c) 2018-24 Raspberry Pi Ltd. > > + * All rights reserved. > > + */ > > + > > +#ifndef _RP1_EXTERN_H_ > > +#define _RP1_EXTERN_H_ > > + > > +extern char __dtbo_rp1_pci_begin[]; > > +extern char __dtbo_rp1_pci_end[]; > > Could you point me where those two variables are used outside of rp1_pci.c? Nowhere else. For the rationale behind this please see: https://lore.kernel.org/all/e6e6c230-370f-4b04-8cb7-4158dd51efdc@lunn.ch/ and followups. Many thanks, Andrea > > ~Stan >
Hi Stan, On 15:31 Tue 05 Nov , Andrea della Porta wrote: > Hi Stan, > > On 15:29 Mon 04 Nov , Stanimir Varbanov wrote: > > Hi Andrea, > > ... > > shouldn't this be: > > > > rp1_clocks: clocks@18000 { > > reg = <0x00 0x00018000 0x0 0x10038>; > > ... > > } > > > > ? > > > > And for other nodes too... > > For that to be @18000 instead of @c040018000, you should also change > the "ranges" entry in pci-ep-bus node, as follows: > > ranges = <0x00 0x00018000 //This was: 0xc0 0x40000000 > 0x01 0x00 0x00000000 > 0x00 0x00400000>; > > which is of course feasible, but I prefer to use addresses that > resemble (at least to some extent) the ones in RP1 docs. > > Many thanks, > Andrea Sorry, this should be like this ranges = <0x00 0x00000000 //This was: 0xc0 0x40000000 0x01 0x00 0x00000000 0x00 0x00400000>; i.e. the child address in the range should be 0 and not 0x18000. Anyway you got the point: in theory we can replace that address with whatever placeholder we want and from a functional perspective, it will work, as long as you change all the relevant mappings. After some brainstorming we've decided to choose a simpler scheme that will resemble more the internal address described in the RP1 reference manual. 0xC040000000 will become 0x40000000. Many thanks, Andrea ...
On Mon, Nov 04, 2024 at 05:49:37PM -0600, Bjorn Helgaas wrote: > On Mon, Nov 04, 2024 at 08:35:21PM +0530, Manivannan Sadhasivam wrote: > > On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: > > > On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > > > > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > > > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > > > > bridge, the window should instead be in PCI address space. Call > > > > > pci_bus_address() on the resource in order to obtain the PCI bus > > > > > address. > > > > > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > > > > bridges), right? > > > > > > Correct. Please note however that while the PCI-PCI bridge has the parent > > > address in CPU space, an endpoint device has it in PCI space: here we're > > > focusing on the bridge part. It probably used to work before since in many > > > cases the CPU and PCI address are the same, but it breaks down when they > > > differ. > > > > When you say 'focusing', you are specifically referring to the > > bridge part of this API I believe. But I don't see a check for the > > bridge in your change, which is what concerning me. Am I missing > > something? > > I think we want this change for all devices in the PCI address > domain, including PCI-PCI bridges and endpoints, don't we? All those > "ranges" addresses should be in the PCI domain. > Yeah, right. I was slightly confused by the commit message. Maybe including a sentence about how the change will work fine for endpoint devices would help. Also, why it went unnoticed till now (ie., both CPU and PCI addresses are same in many SoCs). Also there should be a fixes tag (also CC stable) since this is a potential bug fix. - Mani
Hi Manivannan, On 14:35 Wed 06 Nov , Manivannan Sadhasivam wrote: > On Mon, Nov 04, 2024 at 05:49:37PM -0600, Bjorn Helgaas wrote: > > On Mon, Nov 04, 2024 at 08:35:21PM +0530, Manivannan Sadhasivam wrote: > > > On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: > > > > On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > > > > > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > > > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > > > > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > > > > > bridge, the window should instead be in PCI address space. Call > > > > > > pci_bus_address() on the resource in order to obtain the PCI bus > > > > > > address. > > > > > > > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > > > > > bridges), right? > > > > > > > > Correct. Please note however that while the PCI-PCI bridge has the parent > > > > address in CPU space, an endpoint device has it in PCI space: here we're > > > > focusing on the bridge part. It probably used to work before since in many > > > > cases the CPU and PCI address are the same, but it breaks down when they > > > > differ. > > > > > > When you say 'focusing', you are specifically referring to the > > > bridge part of this API I believe. But I don't see a check for the > > > bridge in your change, which is what concerning me. Am I missing > > > something? > > > > I think we want this change for all devices in the PCI address > > domain, including PCI-PCI bridges and endpoints, don't we? All those > > "ranges" addresses should be in the PCI domain. > > > > Yeah, right. I was slightly confused by the commit message. Maybe including a > sentence about how the change will work fine for endpoint devices would help. > Also, why it went unnoticed till now (ie., both CPU and PCI addresses are same > in many SoCs). Sorry for the (admittedly) confusing explanation from my side. What I would have really liked to convey is that although the root complex (that is itself a bridge) is the ultimate 'translator' between CPU and PCI addresses, all the other entities are of course under PCI address space. In fact, any resource submitted to of_pci_set_address() is intended to be a PCI bus address, and this is valid for both sub-bridges and EPs. > > Also there should be a fixes tag (also CC stable) since this is a potential bug > fix. Sure. I think it could be better to resend this specific patch (and maybe also the patch "of: address: Preserve the flags portion on 1:1 dma-ranges mapping", which is also a kind of bugfix) as standalone ones instead of prerequisites for the RP1 patchset, if it's not a concern to anyone... Regards, Andrea > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On Thu, Nov 07, 2024 at 10:06:53AM +0100, Andrea della Porta wrote: > Hi Manivannan, > > On 14:35 Wed 06 Nov , Manivannan Sadhasivam wrote: > > On Mon, Nov 04, 2024 at 05:49:37PM -0600, Bjorn Helgaas wrote: > > > On Mon, Nov 04, 2024 at 08:35:21PM +0530, Manivannan Sadhasivam wrote: > > > > On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: > > > > > On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > > > > > > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > > > > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > > > > > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > > > > > > bridge, the window should instead be in PCI address space. Call > > > > > > > pci_bus_address() on the resource in order to obtain the PCI bus > > > > > > > address. > > > > > > > > > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > > > > > > bridges), right? > > > > > > > > > > Correct. Please note however that while the PCI-PCI bridge has the parent > > > > > address in CPU space, an endpoint device has it in PCI space: here we're > > > > > focusing on the bridge part. It probably used to work before since in many > > > > > cases the CPU and PCI address are the same, but it breaks down when they > > > > > differ. > > > > > > > > When you say 'focusing', you are specifically referring to the > > > > bridge part of this API I believe. But I don't see a check for the > > > > bridge in your change, which is what concerning me. Am I missing > > > > something? > > > > > > I think we want this change for all devices in the PCI address > > > domain, including PCI-PCI bridges and endpoints, don't we? All those > > > "ranges" addresses should be in the PCI domain. > > > > > > > Yeah, right. I was slightly confused by the commit message. Maybe including a > > sentence about how the change will work fine for endpoint devices would help. > > Also, why it went unnoticed till now (ie., both CPU and PCI addresses are same > > in many SoCs). > > Sorry for the (admittedly) confusing explanation from my side. What I would > have really liked to convey is that although the root complex (that is itself > a bridge) is the ultimate 'translator' between CPU and PCI addresses, all the > other entities are of course under PCI address space. In fact, any resource > submitted to of_pci_set_address() is intended to be a PCI bus address, > and this is valid for both sub-bridges and EPs. > Sounds good. We usually have empty ranges for PCI bridges (1:1 mapping), so that also lead to the confusion at my end. > > > > Also there should be a fixes tag (also CC stable) since this is a potential bug > > fix. > > Sure. I think it could be better to resend this specific patch (and maybe also the > patch "of: address: Preserve the flags portion on 1:1 dma-ranges mapping", which > is also a kind of bugfix) as standalone ones instead of prerequisites for the RP1 > patchset, if it's not a concern to anyone... > In fact, it is recommended to send fixes separately (or at the start of the series). So there should be no concerns. - Mani
Hi Mani, On 11/6/24 16:35, Manivannan Sadhasivam wrote: > On Mon, Nov 04, 2024 at 05:49:37PM -0600, Bjorn Helgaas wrote: >> On Mon, Nov 04, 2024 at 08:35:21PM +0530, Manivannan Sadhasivam wrote: >>> On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: >>>> On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: >>>>> On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: >>>>>> When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() >>>>>> incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI >>>>>> bridge, the window should instead be in PCI address space. Call >>>>>> pci_bus_address() on the resource in order to obtain the PCI bus >>>>>> address. >>>>> >>>>> of_pci_prop_ranges() could be called for PCI devices also (not just PCI >>>>> bridges), right? >>>> >>>> Correct. Please note however that while the PCI-PCI bridge has the parent >>>> address in CPU space, an endpoint device has it in PCI space: here we're >>>> focusing on the bridge part. It probably used to work before since in many >>>> cases the CPU and PCI address are the same, but it breaks down when they >>>> differ. >>> >>> When you say 'focusing', you are specifically referring to the >>> bridge part of this API I believe. But I don't see a check for the >>> bridge in your change, which is what concerning me. Am I missing >>> something? >> >> I think we want this change for all devices in the PCI address >> domain, including PCI-PCI bridges and endpoints, don't we? All those >> "ranges" addresses should be in the PCI domain. >> > > Yeah, right. I was slightly confused by the commit message. Maybe including a > sentence about how the change will work fine for endpoint devices would help. > Also, why it went unnoticed till now (ie., both CPU and PCI addresses are same > in many SoCs). Most probably it is unnoticed because until now nobody has enabled /selected CONFIG_PCI_DYNAMIC_OF_NODES ? ~Stan
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]. The patches are ordered as follows: -PATCHES 1 to 4: add binding schemas for clock, gpio and RP1 peripherals. They are needed to support the other peripherals, e.g. the ethernet mac depends on a clock generated by RP1 and the phy is reset though the on-board gpio controller. -PATCHES 5 and 6: preparatory patches that fix the address mapping translation (especially wrt dma-ranges). -PATCH 7 and 8: add clock and gpio device drivers. -PATCH 9: the devicetree overlay describing the RP1 chipset. Please note that this patch should be taken by the same maintainer that will also take patch 11, since txeieh dtso is compiled in as binary blob and is closely coupled to the driver. -PATCH 10: this is the main patch to support RP1 chipset and peripherals enabling through dtb overlay. The dtso since is intimately coupled with the driver and will be linked in as binary blob in the driver obj. The real dtso is in devicetree folder while the dtso in driver folder is just a placeholder to include the real dtso. In this way it is possible to check the dtso against dt-bindings. The reason why drivers/misc has been selected as containing folder for this driver can be seen in [6], [7] and [8]. -PATCH 11: add the external clock node (used by RP1) to the main dts. -PATCH 12: add the relevant kernel CONFIG_ options to defconfig. This patchset is also a first attempt to be more agnostic wrt hardware description standards such as OF devicetree and ACPI, where 'agnostic' means "using DT in coexistence with ACPI", as been already promoted by e.g. AL (see [4]). Although there's currently no evidence it will also run out of the box on purely ACPI system, it is a first step towards that direction. Please note that albeit this patchset has no prerequisites in order to be applied cleanly, it still depends on Stanimir's WIP patchset for BCM2712 PCIe controller (see [5]) in order to work at runtime. Many thanks, Andrea della Porta Link: - [1]: https://lpc.events/event/17/contributions/1421/attachments/1337/2680/LPC2023%20Non-discoverable%20devices%20in%20PCI.pdf - [2]: https://lore.kernel.org/lkml/20230419231155.GA899497-robh@kernel.org/t/ - [3]: https://lore.kernel.org/all/20240808154658.247873-1-herve.codina@bootlin.com/#t - [4]: https://lore.kernel.org/all/73e05c77-6d53-4aae-95ac-415456ff0ae4@lunn.ch/ - [5]: https://lore.kernel.org/all/20240626104544.14233-1-svarbanov@suse.de/ - [6]: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ - [7]: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ - [8]: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ CHANGES IN V3: NEW ADDITIONS ------------------------------------------------ - Fixed a bug in of_pci_prop_ranges() that was incorrectly using a CPU address instead of PCI bus address while assigning "ranges" properties to PCI-PCI bridge nodes. As a side effect, the patch "PCI: of_property: Sanitize 32 bit PCI address parsed from DT" has been dropped since it's no longer necessary. RP1 misc driver ----------------------------------- - Dropped -@ option while compiling the dtso. - Removed unused includes. - Dropped unused sys_clk references. - Got rid of dump_bar(). - Added the relevant unregister function on exit paths for mapped interrupts and domain. - Added missing MODULE_DEVICE_TABLE(). - reset_control no longer claimed in probe(). - dtbo_size and dtbo_start local vars definition moved at the beginnning of the probe() function. - the DTB overlay is now applied after the interrupt controller has been setup. - Reworked the Kconfig dependency list for CONFIG_MISC_RP1 to avoid a recursion. GPIO/PINCTRL -------------------------------------- - Gpio line names changes (and relative dtbo and preparatory patches) have been dropped. CLOCKS -------------------------------------- - raspberrypi,rp1-clocks.h: license adjusted. - Removed unused headers. - Replaced locally defined KHZ and MHZ with defines from linux/units.h - Added regmap support for registers which also has builtin support for showing regs via debugfs. Dropped the previous implementation of debugfs attributes. - Reworked the clock tree using clk_parent_data instead of strings. This also allowed to get rid of __clk_get_hw() and friends. - Split a couple of lines assigning to calc_rate into multi-lines for ease of understanding. - Dropped .set_rate function from rp1_pll_ph_ops since it does nothing. - Initialize struct clk_init_data init via = {} instead of memset. - Used dev_err_probe() instead of dev_err(). - Module init/exit declaration replaced by module_platform_driver(). - Kconfig: CONFIG_COMMON_CLOCK_RP1 now depends on CONFIG_MISC_RP1 instead of CONFIG_PCI. DTS ----------------------------------------- - "rp1-xosc" renamed to "xosc" BINDINGS ------------------------------------ - raspberrypi,rp1-gpio.yaml: removed a paragraph in the description of pinctrl node since it's already covered by pinctrl-bindings. - All paths to referenced bindings are now full-paths. - Uniformly using single quotes over double quotes on patterns and strings. - Amended some node names to adhere to DTS coding style. - Dropped unused labels in examples. - pci-ep-bus.yaml: simplified the definition of pci-ep-bus node. - pci-ep-bus.yaml: added additionalProperties: true. - pci1de4,1.yaml: interrupt-controller and #interrupt-cells moved from pci-ep-bus node to the main device. - pci1de4,1.yaml: @unit-number not optional anymore. - pci1de4,1.yaml: droppped pci-ep-bus redefinition (already inherited by pci-ep-bus.yaml). Also removed the internal SoC nodes. Andrea della Porta (12): dt-bindings: clock: Add RaspberryPi RP1 clock bindings dt-bindings: pinctrl: Add RaspberryPi RP1 gpio/pinctrl/pinmux bindings dt-bindings: pci: Add common schema for devices accessible through PCI BARs dt-bindings: misc: Add device specific bindings for RaspberryPi RP1 PCI: of_property: Assign PCI instead of CPU bus address to dynamic bridge nodes of: address: Preserve the flags portion on 1:1 dma-ranges mapping clk: rp1: Add support for clocks provided by RP1 pinctrl: rp1: Implement RaspberryPi RP1 gpio support arm64: dts: rp1: Add support for RaspberryPi's RP1 device misc: rp1: RaspberryPi RP1 misc driver arm64: dts: bcm2712: Add external clock for RP1 chipset on Rpi5 arm64: defconfig: Enable RP1 misc/clock/gpio drivers .../clock/raspberrypi,rp1-clocks.yaml | 62 + .../devicetree/bindings/misc/pci1de4,1.yaml | 80 + .../devicetree/bindings/pci/pci-ep-bus.yaml | 58 + .../pinctrl/raspberrypi,rp1-gpio.yaml | 163 ++ MAINTAINERS | 14 + arch/arm64/boot/dts/broadcom/bcm2712.dtsi | 7 + arch/arm64/boot/dts/broadcom/rp1.dtso | 61 + arch/arm64/configs/defconfig | 3 + drivers/clk/Kconfig | 8 + drivers/clk/Makefile | 1 + drivers/clk/clk-rp1.c | 1540 +++++++++++++++++ drivers/misc/Kconfig | 1 + drivers/misc/Makefile | 1 + drivers/misc/rp1/Kconfig | 21 + drivers/misc/rp1/Makefile | 3 + drivers/misc/rp1/rp1-pci.dtso | 8 + drivers/misc/rp1/rp1_pci.c | 357 ++++ drivers/misc/rp1/rp1_pci.h | 14 + drivers/of/address.c | 3 +- drivers/pci/of_property.c | 2 +- drivers/pci/quirks.c | 1 + drivers/pinctrl/Kconfig | 11 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-rp1.c | 801 +++++++++ .../clock/raspberrypi,rp1-clocks.h | 61 + include/linux/pci_ids.h | 3 + 26 files changed, 3283 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml create mode 100644 Documentation/devicetree/bindings/misc/pci1de4,1.yaml create mode 100644 Documentation/devicetree/bindings/pci/pci-ep-bus.yaml create mode 100644 Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso create mode 100644 drivers/clk/clk-rp1.c create mode 100644 drivers/misc/rp1/Kconfig create mode 100644 drivers/misc/rp1/Makefile create mode 100644 drivers/misc/rp1/rp1-pci.dtso create mode 100644 drivers/misc/rp1/rp1_pci.c create mode 100644 drivers/misc/rp1/rp1_pci.h create mode 100644 drivers/pinctrl/pinctrl-rp1.c create mode 100644 include/dt-bindings/clock/raspberrypi,rp1-clocks.h