Re: [PATCH v2 08/14] clk: rp1: Add support for clocks provided by RP1

Re: [PATCH v2 08/14] clk: rp1: Add support for clocks provided by RP1

[Andrea della Porta]

Hi Stephen,

On 15:08 Wed 09 Oct     , Stephen Boyd wrote:
> Quoting Andrea della Porta (2024-10-07 05:39:51)
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 299bc678ed1b..537019987f0c 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -88,6 +88,15 @@ config COMMON_CLK_RK808
> >           These multi-function devices have two fixed-rate oscillators, clocked at 32KHz each.
> >           Clkout1 is always on, Clkout2 can off by control register.
> >  
> > +config COMMON_CLK_RP1
> > +       tristate "Raspberry Pi RP1-based clock support"
> > +       depends on PCI || COMPILE_TEST
> 
> A better limit would be some ARCH_* config.

I've avoided ARCH_BCM2835 since the original intention is for this driver
to work (in the future) also for custom PCI cards with RP1 on-board, and not
only for Rpi5.

> 
> > +       depends on COMMON_CLK
> 
> This is redundant as it's inside the 'if COMMON_CLK'. Please remove this
> line.

Ack.

> 
> > +       help
> > +         Enable common clock framework support for Raspberry Pi RP1.
> > +         This multi-function device has 3 main PLLs and several clock
> > +         generators to drive the internal sub-peripherals.
> > +
> >  config COMMON_CLK_HI655X
> >         tristate "Clock driver for Hi655x" if EXPERT
> >         depends on (MFD_HI655X_PMIC || COMPILE_TEST)
> > diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c
> > new file mode 100644
> > index 000000000000..9016666fb27d
> > --- /dev/null
> > +++ b/drivers/clk/clk-rp1.c
> > @@ -0,0 +1,1658 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2023 Raspberry Pi Ltd.
> > + *
> > + * Clock driver for RP1 PCIe multifunction chip.
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
> 
> Drop unused header.

Ack.

> 
> > +#include <linux/clk.h>
> 
> Preferably this include isn't included.

This include is currently needed by devm_clk_get_enabled() to retrieve
the xosc. Since that clock is based on a crystal (so it's fixed and
always enabled), I'm planning to hardcode it in the driver. This will
not only get rid of the devm_clk_get_enabled() call (and hence of the
clk.h include), but it'll also simplify the top devicetree. No promise
though, I need to check a couple of things first.

> 
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> 
> What is this include for? Should probably be mod_devicetable.h?

Yes.

> 
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#include <asm/div64.h>
> 
> Include math64.h instead?

Right, this is redundant, since there is already math64.h included a bunch
of lines above.

> 
> > +
> > +#include <dt-bindings/clock/raspberrypi,rp1-clocks.h>
> > +
> > +#define PLL_SYS_OFFSET                 0x08000
> > +#define PLL_SYS_CS                     (PLL_SYS_OFFSET + 0x00)
> > +#define PLL_SYS_PWR                    (PLL_SYS_OFFSET + 0x04)
> > +#define PLL_SYS_FBDIV_INT              (PLL_SYS_OFFSET + 0x08)
> > +#define PLL_SYS_FBDIV_FRAC             (PLL_SYS_OFFSET + 0x0c)
> > +#define PLL_SYS_PRIM                   (PLL_SYS_OFFSET + 0x10)
> > +#define PLL_SYS_SEC                    (PLL_SYS_OFFSET + 0x14)
> > +
> > +#define PLL_AUDIO_OFFSET               0x0c000
> > +#define PLL_AUDIO_CS                   (PLL_AUDIO_OFFSET + 0x00)
> > +#define PLL_AUDIO_PWR                  (PLL_AUDIO_OFFSET + 0x04)
> > +#define PLL_AUDIO_FBDIV_INT            (PLL_AUDIO_OFFSET + 0x08)
> > +#define PLL_AUDIO_FBDIV_FRAC           (PLL_AUDIO_OFFSET + 0x0c)
> > +#define PLL_AUDIO_PRIM                 (PLL_AUDIO_OFFSET + 0x10)
> > +#define PLL_AUDIO_SEC                  (PLL_AUDIO_OFFSET + 0x14)
> > +#define PLL_AUDIO_TERN                 (PLL_AUDIO_OFFSET + 0x18)
> > +
> > +#define PLL_VIDEO_OFFSET               0x10000
> > +#define PLL_VIDEO_CS                   (PLL_VIDEO_OFFSET + 0x00)
> > +#define PLL_VIDEO_PWR                  (PLL_VIDEO_OFFSET + 0x04)
> > +#define PLL_VIDEO_FBDIV_INT            (PLL_VIDEO_OFFSET + 0x08)
> > +#define PLL_VIDEO_FBDIV_FRAC           (PLL_VIDEO_OFFSET + 0x0c)
> > +#define PLL_VIDEO_PRIM                 (PLL_VIDEO_OFFSET + 0x10)
> > +#define PLL_VIDEO_SEC                  (PLL_VIDEO_OFFSET + 0x14)
> > +
> > +#define GPCLK_OE_CTRL                  0x00000
> > +
> > +#define CLK_SYS_OFFSET                 0x00014
> > +#define CLK_SYS_CTRL                   (CLK_SYS_OFFSET + 0x00)
> > +#define CLK_SYS_DIV_INT                        (CLK_SYS_OFFSET + 0x04)
> > +#define CLK_SYS_SEL                    (CLK_SYS_OFFSET + 0x0c)
> > +
> > +#define CLK_SLOW_OFFSET                        0x00024
> > +#define CLK_SLOW_SYS_CTRL              (CLK_SLOW_OFFSET + 0x00)
> > +#define CLK_SLOW_SYS_DIV_INT           (CLK_SLOW_OFFSET + 0x04)
> > +#define CLK_SLOW_SYS_SEL               (CLK_SLOW_OFFSET + 0x0c)
> > +
> > +#define CLK_DMA_OFFSET                 0x00044
> > +#define CLK_DMA_CTRL                   (CLK_DMA_OFFSET + 0x00)
> > +#define CLK_DMA_DIV_INT                        (CLK_DMA_OFFSET + 0x04)
> > +#define CLK_DMA_SEL                    (CLK_DMA_OFFSET + 0x0c)
> > +
> > +#define CLK_UART_OFFSET                        0x00054
> > +#define CLK_UART_CTRL                  (CLK_UART_OFFSET + 0x00)
> > +#define CLK_UART_DIV_INT               (CLK_UART_OFFSET + 0x04)
> > +#define CLK_UART_SEL                   (CLK_UART_OFFSET + 0x0c)
> > +
> > +#define CLK_ETH_OFFSET                 0x00064
> > +#define CLK_ETH_CTRL                   (CLK_ETH_OFFSET + 0x00)
> > +#define CLK_ETH_DIV_INT                        (CLK_ETH_OFFSET + 0x04)
> > +#define CLK_ETH_SEL                    (CLK_ETH_OFFSET + 0x0c)
> > +
> > +#define CLK_PWM0_OFFSET                        0x00074
> > +#define CLK_PWM0_CTRL                  (CLK_PWM0_OFFSET + 0x00)
> > +#define CLK_PWM0_DIV_INT               (CLK_PWM0_OFFSET + 0x04)
> > +#define CLK_PWM0_DIV_FRAC              (CLK_PWM0_OFFSET + 0x08)
> > +#define CLK_PWM0_SEL                   (CLK_PWM0_OFFSET + 0x0c)
> > +
> > +#define CLK_PWM1_OFFSET                        0x00084
> > +#define CLK_PWM1_CTRL                  (CLK_PWM1_OFFSET + 0x00)
> > +#define CLK_PWM1_DIV_INT               (CLK_PWM1_OFFSET + 0x04)
> > +#define CLK_PWM1_DIV_FRAC              (CLK_PWM1_OFFSET + 0x08)
> > +#define CLK_PWM1_SEL                   (CLK_PWM1_OFFSET + 0x0c)
> > +
> > +#define CLK_AUDIO_IN_OFFSET            0x00094
> > +#define CLK_AUDIO_IN_CTRL              (CLK_AUDIO_IN_OFFSET + 0x00)
> > +#define CLK_AUDIO_IN_DIV_INT           (CLK_AUDIO_IN_OFFSET + 0x04)
> > +#define CLK_AUDIO_IN_SEL               (CLK_AUDIO_IN_OFFSET + 0x0c)
> > +
> > +#define CLK_AUDIO_OUT_OFFSET           0x000a4
> > +#define CLK_AUDIO_OUT_CTRL             (CLK_AUDIO_OUT_OFFSET + 0x00)
> > +#define CLK_AUDIO_OUT_DIV_INT          (CLK_AUDIO_OUT_OFFSET + 0x04)
> > +#define CLK_AUDIO_OUT_SEL              (CLK_AUDIO_OUT_OFFSET + 0x0c)
> > +
> > +#define CLK_I2S_OFFSET                 0x000b4
> > +#define CLK_I2S_CTRL                   (CLK_I2S_OFFSET + 0x00)
> > +#define CLK_I2S_DIV_INT                        (CLK_I2S_OFFSET + 0x04)
> > +#define CLK_I2S_SEL                    (CLK_I2S_OFFSET + 0x0c)
> > +
> > +#define CLK_MIPI0_CFG_OFFSET           0x000c4
> > +#define CLK_MIPI0_CFG_CTRL             (CLK_MIPI0_CFG_OFFSET + 0x00)
> > +#define CLK_MIPI0_CFG_DIV_INT          (CLK_MIPI0_CFG_OFFSET + 0x04)
> > +#define CLK_MIPI0_CFG_SEL              (CLK_MIPI0_CFG_OFFSET + 0x0c)
> > +
> > +#define CLK_MIPI1_CFG_OFFSET           0x000d4
> > +#define CLK_MIPI1_CFG_CTRL             (CLK_MIPI1_CFG_OFFSET + 0x00)
> > +#define CLK_MIPI1_CFG_DIV_INT          (CLK_MIPI1_CFG_OFFSET + 0x04)
> > +#define CLK_MIPI1_CFG_SEL              (CLK_MIPI1_CFG_OFFSET + 0x0c)
> > +
> > +#define CLK_PCIE_AUX_OFFSET            0x000e4
> > +#define CLK_PCIE_AUX_CTRL              (CLK_PCIE_AUX_OFFSET + 0x00)
> > +#define CLK_PCIE_AUX_DIV_INT           (CLK_PCIE_AUX_OFFSET + 0x04)
> > +#define CLK_PCIE_AUX_SEL               (CLK_PCIE_AUX_OFFSET + 0x0c)
> > +
> > +#define CLK_USBH0_MICROFRAME_OFFSET    0x000f4
> > +#define CLK_USBH0_MICROFRAME_CTRL      (CLK_USBH0_MICROFRAME_OFFSET + 0x00)
> > +#define CLK_USBH0_MICROFRAME_DIV_INT   (CLK_USBH0_MICROFRAME_OFFSET + 0x04)
> > +#define CLK_USBH0_MICROFRAME_SEL       (CLK_USBH0_MICROFRAME_OFFSET + 0x0c)
> > +
> > +#define CLK_USBH1_MICROFRAME_OFFSET    0x00104
> > +#define CLK_USBH1_MICROFRAME_CTRL      (CLK_USBH1_MICROFRAME_OFFSET + 0x00)
> > +#define CLK_USBH1_MICROFRAME_DIV_INT   (CLK_USBH1_MICROFRAME_OFFSET + 0x04)
> > +#define CLK_USBH1_MICROFRAME_SEL       (CLK_USBH1_MICROFRAME_OFFSET + 0x0c)
> > +
> > +#define CLK_USBH0_SUSPEND_OFFSET       0x00114
> > +#define CLK_USBH0_SUSPEND_CTRL         (CLK_USBH0_SUSPEND_OFFSET + 0x00)
> > +#define CLK_USBH0_SUSPEND_DIV_INT      (CLK_USBH0_SUSPEND_OFFSET + 0x04)
> > +#define CLK_USBH0_SUSPEND_SEL          (CLK_USBH0_SUSPEND_OFFSET + 0x0c)
> > +
> > +#define CLK_USBH1_SUSPEND_OFFSET       0x00124
> > +#define CLK_USBH1_SUSPEND_CTRL         (CLK_USBH1_SUSPEND_OFFSET + 0x00)
> > +#define CLK_USBH1_SUSPEND_DIV_INT      (CLK_USBH1_SUSPEND_OFFSET + 0x04)
> > +#define CLK_USBH1_SUSPEND_SEL          (CLK_USBH1_SUSPEND_OFFSET + 0x0c)
> > +
> > +#define CLK_ETH_TSU_OFFSET             0x00134
> > +#define CLK_ETH_TSU_CTRL               (CLK_ETH_TSU_OFFSET + 0x00)
> > +#define CLK_ETH_TSU_DIV_INT            (CLK_ETH_TSU_OFFSET + 0x04)
> > +#define CLK_ETH_TSU_SEL                        (CLK_ETH_TSU_OFFSET + 0x0c)
> > +
> > +#define CLK_ADC_OFFSET                 0x00144
> > +#define CLK_ADC_CTRL                   (CLK_ADC_OFFSET + 0x00)
> > +#define CLK_ADC_DIV_INT                        (CLK_ADC_OFFSET + 0x04)
> > +#define CLK_ADC_SEL                    (CLK_ADC_OFFSET + 0x0c)
> > +
> > +#define CLK_SDIO_TIMER_OFFSET          0x00154
> > +#define CLK_SDIO_TIMER_CTRL            (CLK_SDIO_TIMER_OFFSET + 0x00)
> > +#define CLK_SDIO_TIMER_DIV_INT         (CLK_SDIO_TIMER_OFFSET + 0x04)
> > +#define CLK_SDIO_TIMER_SEL             (CLK_SDIO_TIMER_OFFSET + 0x0c)
> > +
> > +#define CLK_SDIO_ALT_SRC_OFFSET                0x00164
> > +#define CLK_SDIO_ALT_SRC_CTRL          (CLK_SDIO_ALT_SRC_OFFSET + 0x00)
> > +#define CLK_SDIO_ALT_SRC_DIV_INT       (CLK_SDIO_ALT_SRC_OFFSET + 0x04)
> > +#define CLK_SDIO_ALT_SRC_SEL           (CLK_SDIO_ALT_SRC_OFFSET + 0x0c)
> > +
> > +#define CLK_GP0_OFFSET                 0x00174
> > +#define CLK_GP0_CTRL                   (CLK_GP0_OFFSET + 0x00)
> > +#define CLK_GP0_DIV_INT                        (CLK_GP0_OFFSET + 0x04)
> > +#define CLK_GP0_DIV_FRAC               (CLK_GP0_OFFSET + 0x08)
> > +#define CLK_GP0_SEL                    (CLK_GP0_OFFSET + 0x0c)
> > +
> > +#define CLK_GP1_OFFSET                 0x00184
> > +#define CLK_GP1_CTRL                   (CLK_GP1_OFFSET + 0x00)
> > +#define CLK_GP1_DIV_INT                        (CLK_GP1_OFFSET + 0x04)
> > +#define CLK_GP1_DIV_FRAC               (CLK_GP1_OFFSET + 0x08)
> > +#define CLK_GP1_SEL                    (CLK_GP1_OFFSET + 0x0c)
> > +
> > +#define CLK_GP2_OFFSET                 0x00194
> > +#define CLK_GP2_CTRL                   (CLK_GP2_OFFSET + 0x00)
> > +#define CLK_GP2_DIV_INT                        (CLK_GP2_OFFSET + 0x04)
> > +#define CLK_GP2_DIV_FRAC               (CLK_GP2_OFFSET + 0x08)
> > +#define CLK_GP2_SEL                    (CLK_GP2_OFFSET + 0x0c)
> > +
> > +#define CLK_GP3_OFFSET                 0x001a4
> > +#define CLK_GP3_CTRL                   (CLK_GP3_OFFSET + 0x00)
> > +#define CLK_GP3_DIV_INT                        (CLK_GP3_OFFSET + 0x04)
> > +#define CLK_GP3_DIV_FRAC               (CLK_GP3_OFFSET + 0x08)
> > +#define CLK_GP3_SEL                    (CLK_GP3_OFFSET + 0x0c)
> > +
> > +#define CLK_GP4_OFFSET                 0x001b4
> > +#define CLK_GP4_CTRL                   (CLK_GP4_OFFSET + 0x00)
> > +#define CLK_GP4_DIV_INT                        (CLK_GP4_OFFSET + 0x04)
> > +#define CLK_GP4_DIV_FRAC               (CLK_GP4_OFFSET + 0x08)
> > +#define CLK_GP4_SEL                    (CLK_GP4_OFFSET + 0x0c)
> > +
> > +#define CLK_GP5_OFFSET                 0x001c4
> > +#define CLK_GP5_CTRL                   (CLK_GP5_OFFSET + 0x00)
> > +#define CLK_GP5_DIV_INT                        (CLK_GP5_OFFSET + 0x04)
> > +#define CLK_GP5_DIV_FRAC               (CLK_GP5_OFFSET + 0x08)
> > +#define CLK_GP5_SEL                    (CLK_GP5_OFFSET + 0x0c)
> > +
> > +#define CLK_SYS_RESUS_CTRL             0x0020c
> > +
> > +#define CLK_SLOW_SYS_RESUS_CTRL                0x00214
> > +
> > +#define FC0_OFFSET                     0x0021c
> > +#define FC0_REF_KHZ                    (FC0_OFFSET + 0x00)
> > +#define FC0_MIN_KHZ                    (FC0_OFFSET + 0x04)
> > +#define FC0_MAX_KHZ                    (FC0_OFFSET + 0x08)
> > +#define FC0_DELAY                      (FC0_OFFSET + 0x0c)
> > +#define FC0_INTERVAL                   (FC0_OFFSET + 0x10)
> > +#define FC0_SRC                                (FC0_OFFSET + 0x14)
> > +#define FC0_STATUS                     (FC0_OFFSET + 0x18)
> > +#define FC0_RESULT                     (FC0_OFFSET + 0x1c)
> > +#define FC_SIZE                                0x20
> > +#define FC_COUNT                       8
> > +#define FC_NUM(idx, off)               ((idx) * 32 + (off))
> > +
> > +#define AUX_SEL                                1
> > +
> > +#define VIDEO_CLOCKS_OFFSET            0x4000
> > +#define VIDEO_CLK_VEC_CTRL             (VIDEO_CLOCKS_OFFSET + 0x0000)
> > +#define VIDEO_CLK_VEC_DIV_INT          (VIDEO_CLOCKS_OFFSET + 0x0004)
> > +#define VIDEO_CLK_VEC_SEL              (VIDEO_CLOCKS_OFFSET + 0x000c)
> > +#define VIDEO_CLK_DPI_CTRL             (VIDEO_CLOCKS_OFFSET + 0x0010)
> > +#define VIDEO_CLK_DPI_DIV_INT          (VIDEO_CLOCKS_OFFSET + 0x0014)
> > +#define VIDEO_CLK_DPI_SEL              (VIDEO_CLOCKS_OFFSET + 0x001c)
> > +#define VIDEO_CLK_MIPI0_DPI_CTRL       (VIDEO_CLOCKS_OFFSET + 0x0020)
> > +#define VIDEO_CLK_MIPI0_DPI_DIV_INT    (VIDEO_CLOCKS_OFFSET + 0x0024)
> > +#define VIDEO_CLK_MIPI0_DPI_DIV_FRAC   (VIDEO_CLOCKS_OFFSET + 0x0028)
> > +#define VIDEO_CLK_MIPI0_DPI_SEL                (VIDEO_CLOCKS_OFFSET + 0x002c)
> > +#define VIDEO_CLK_MIPI1_DPI_CTRL       (VIDEO_CLOCKS_OFFSET + 0x0030)
> > +#define VIDEO_CLK_MIPI1_DPI_DIV_INT    (VIDEO_CLOCKS_OFFSET + 0x0034)
> > +#define VIDEO_CLK_MIPI1_DPI_DIV_FRAC   (VIDEO_CLOCKS_OFFSET + 0x0038)
> > +#define VIDEO_CLK_MIPI1_DPI_SEL                (VIDEO_CLOCKS_OFFSET + 0x003c)
> > +
> > +#define DIV_INT_8BIT_MAX               GENMASK(7, 0)   /* max divide for most clocks */
> > +#define DIV_INT_16BIT_MAX              GENMASK(15, 0)  /* max divide for GPx, PWM */
> > +#define DIV_INT_24BIT_MAX               GENMASK(23, 0) /* max divide for CLK_SYS */
> > +
> > +#define FC0_STATUS_DONE                        BIT(4)
> > +#define FC0_STATUS_RUNNING             BIT(8)
> > +#define FC0_RESULT_FRAC_SHIFT          5
> > +
> > +#define PLL_PRIM_DIV1_SHIFT            16
> > +#define PLL_PRIM_DIV1_WIDTH            3
> > +#define PLL_PRIM_DIV1_MASK             GENMASK(PLL_PRIM_DIV1_SHIFT + \
> > +                                               PLL_PRIM_DIV1_WIDTH - 1, \
> > +                                               PLL_PRIM_DIV1_SHIFT)
> > +#define PLL_PRIM_DIV2_SHIFT            12
> > +#define PLL_PRIM_DIV2_WIDTH            3
> > +#define PLL_PRIM_DIV2_MASK             GENMASK(PLL_PRIM_DIV2_SHIFT + \
> > +                                               PLL_PRIM_DIV2_WIDTH - 1, \
> > +                                               PLL_PRIM_DIV2_SHIFT)
> > +
> > +#define PLL_SEC_DIV_SHIFT              8
> > +#define PLL_SEC_DIV_WIDTH              5
> > +#define PLL_SEC_DIV_MASK               GENMASK(PLL_SEC_DIV_SHIFT + \
> > +                                               PLL_SEC_DIV_WIDTH - 1, \
> > +                                               PLL_SEC_DIV_SHIFT)
> > +
> > +#define PLL_CS_LOCK                    BIT(31)
> > +#define PLL_CS_REFDIV_SHIFT            0
> > +
> > +#define PLL_PWR_PD                     BIT(0)
> > +#define PLL_PWR_DACPD                  BIT(1)
> > +#define PLL_PWR_DSMPD                  BIT(2)
> > +#define PLL_PWR_POSTDIVPD              BIT(3)
> > +#define PLL_PWR_4PHASEPD               BIT(4)
> > +#define PLL_PWR_VCOPD                  BIT(5)
> > +#define PLL_PWR_MASK                   GENMASK(5, 0)
> > +
> > +#define PLL_SEC_RST                    BIT(16)
> > +#define PLL_SEC_IMPL                   BIT(31)
> > +
> > +/* PLL phase output for both PRI and SEC */
> > +#define PLL_PH_EN                      BIT(4)
> > +#define PLL_PH_PHASE_SHIFT             0
> > +
> > +#define RP1_PLL_PHASE_0                        0
> > +#define RP1_PLL_PHASE_90               1
> > +#define RP1_PLL_PHASE_180              2
> > +#define RP1_PLL_PHASE_270              3
> > +
> > +/* Clock fields for all clocks */
> > +#define CLK_CTRL_ENABLE                        BIT(11)
> > +#define CLK_CTRL_AUXSRC_SHIFT          5
> > +#define CLK_CTRL_AUXSRC_WIDTH          5
> > +#define CLK_CTRL_AUXSRC_MASK           GENMASK(CLK_CTRL_AUXSRC_SHIFT + \
> > +                                               CLK_CTRL_AUXSRC_WIDTH - 1, \
> > +                                               CLK_CTRL_AUXSRC_SHIFT)
> > +#define CLK_CTRL_SRC_SHIFT             0
> > +#define CLK_DIV_FRAC_BITS              16
> > +
> > +#define KHz                            1000
> > +#define MHz                            (KHz * KHz)
> 
> I think we have these macros now. See include/linux/units.h.

Ack.

> 
> > +#define LOCK_TIMEOUT_NS                        100000000
> > +#define FC_TIMEOUT_NS                  100000000
> > +
> > +#define MAX_CLK_PARENTS                        16
> > +
> > +/*
> > + * Secondary PLL channel output divider table.
> > + * Divider values range from 8 to 19.
> > + * Invalid values default to 19
> > + */
> > +static const struct clk_div_table pll_sec_div_table[] = {
> > +       { 0x00, 19 },
> > +       { 0x01, 19 },
> > +       { 0x02, 19 },
> > +       { 0x03, 19 },
> > +       { 0x04, 19 },
> > +       { 0x05, 19 },
> > +       { 0x06, 19 },
> > +       { 0x07, 19 },
> > +       { 0x08,  8 },
> > +       { 0x09,  9 },
> > +       { 0x0a, 10 },
> > +       { 0x0b, 11 },
> > +       { 0x0c, 12 },
> > +       { 0x0d, 13 },
> > +       { 0x0e, 14 },
> > +       { 0x0f, 15 },
> > +       { 0x10, 16 },
> > +       { 0x11, 17 },
> > +       { 0x12, 18 },
> > +       { 0x13, 19 },
> > +       { 0x14, 19 },
> > +       { 0x15, 19 },
> > +       { 0x16, 19 },
> > +       { 0x17, 19 },
> > +       { 0x18, 19 },
> > +       { 0x19, 19 },
> > +       { 0x1a, 19 },
> > +       { 0x1b, 19 },
> > +       { 0x1c, 19 },
> > +       { 0x1d, 19 },
> > +       { 0x1e, 19 },
> > +       { 0x1f, 19 },
> > +       { 0 }
> > +};
> > +
> > +struct rp1_clockman {
> > +       struct device *dev;
> > +       void __iomem *regs;
> > +       spinlock_t regs_lock; /* spinlock for all clocks */
> > +       struct clk_hw *hw_xosc; /* reference clock */
> > +
> > +       /* Must be last */
> > +       struct clk_hw_onecell_data onecell;
> > +};
> > +
> > +struct rp1_pll_core_data {
> > +       const char *name;
> > +       u32 cs_reg;
> > +       u32 pwr_reg;
> > +       u32 fbdiv_int_reg;
> > +       u32 fbdiv_frac_reg;
> > +       unsigned long flags;
> > +       u32 fc0_src;
> > +};
> > +
> > +struct rp1_pll_data {
> > +       const char *name;
> > +       const char *source_pll;
> > +       u32 ctrl_reg;
> > +       unsigned long flags;
> > +       u32 fc0_src;
> > +};
> > +
> > +struct rp1_pll_ph_data {
> > +       const char *name;
> > +       const char *source_pll;
> > +       unsigned int phase;
> > +       unsigned int fixed_divider;
> > +       u32 ph_reg;
> > +       unsigned long flags;
> > +       u32 fc0_src;
> > +};
> > +
> > +struct rp1_pll_divider_data {
> > +       const char *name;
> > +       const char *source_pll;
> > +       u32 sec_reg;
> > +       unsigned long flags;
> > +       u32 fc0_src;
> > +};
> > +
> > +struct rp1_clock_data {
> > +       const char *name;
> > +       const char *const parents[MAX_CLK_PARENTS];
> > +       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_pll_core {
> > +       struct clk_hw hw;
> > +       struct rp1_clockman *clockman;
> > +       unsigned long cached_rate;
> > +       const struct rp1_pll_core_data *data;
> > +};
> > +
> > +struct rp1_pll {
> > +       struct clk_hw hw;
> > +       struct rp1_clockman *clockman;
> > +       struct clk_divider div;
> > +       unsigned long cached_rate;
> > +       const struct rp1_pll_data *data;
> > +};
> > +
> > +struct rp1_pll_ph {
> > +       struct clk_hw hw;
> > +       struct rp1_clockman *clockman;
> > +       const struct rp1_pll_ph_data *data;
> > +};
> > +
> > +struct rp1_clock {
> > +       struct clk_hw hw;
> > +       struct rp1_clockman *clockman;
> > +       unsigned long cached_rate;
> > +       const struct rp1_clock_data *data;
> > +};
> > +
> > +struct rp1_clk_change {
> > +       struct clk_hw *hw;
> > +       unsigned long new_rate;
> > +};
> > +
> > +struct rp1_clk_desc {
> > +       struct clk_hw *(*clk_register)(struct rp1_clockman *clockman,
> > +                                      const void *data);
> > +       const void *data;
> > +};
> > +
> > +static void rp1_debugfs_regset(struct rp1_clockman *clockman, u32 base,
> > +                              const struct debugfs_reg32 *regs,
> > +                              size_t nregs, struct dentry *dentry)
> > +{
> > +       struct debugfs_regset32 *regset;
> > +
> > +       regset = devm_kzalloc(clockman->dev, sizeof(*regset), GFP_KERNEL);
> > +       if (!regset)
> > +               return;
> > +
> > +       regset->regs = regs;
> > +       regset->nregs = nregs;
> > +       regset->base = clockman->regs + base;
> > +
> > +       debugfs_create_regset32("regdump", 0444, dentry, regset);
> > +}
> > +
> > +static inline u32 set_register_field(u32 reg, u32 val, u32 mask, u32 shift)
> > +{
> > +       reg &= ~mask;
> > +       reg |= (val << shift) & mask;
> > +       return reg;
> > +}
> 
> Can you use FIELD_PREP() and friends?

Sure. Except for data->clk_src_mask which is non-constant.

> 
> > +
> > +static inline
> > +void clockman_write(struct rp1_clockman *clockman, u32 reg, u32 val)
> > +{
> > +       writel(val, clockman->regs + reg);
> > +}
> > +
> > +static inline u32 clockman_read(struct rp1_clockman *clockman, u32 reg)
> > +{
> > +       return readl(clockman->regs + reg);
> > +}
> > +
> > +static int rp1_pll_core_is_on(struct clk_hw *hw)
> > +{
> > +       struct rp1_pll_core *pll_core = container_of(hw, struct rp1_pll_core, hw);
> > +       struct rp1_clockman *clockman = pll_core->clockman;
> > +       const struct rp1_pll_core_data *data = pll_core->data;
> > +       u32 pwr = clockman_read(clockman, data->pwr_reg);
> > +
> > +       return (pwr & PLL_PWR_PD) || (pwr & PLL_PWR_POSTDIVPD);
> > +}
> > +
> > +static int rp1_pll_core_on(struct clk_hw *hw)
> > +{
> > +       struct rp1_pll_core *pll_core = container_of(hw, struct rp1_pll_core, hw);
> > +       struct rp1_clockman *clockman = pll_core->clockman;
> > +       const struct rp1_pll_core_data *data = pll_core->data;
> > +       u32 fbdiv_frac;
> > +       ktime_t timeout;
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +
> > +       if (!(clockman_read(clockman, data->cs_reg) & PLL_CS_LOCK)) {
> > +               /* Reset to a known state. */
> > +               clockman_write(clockman, data->pwr_reg, PLL_PWR_MASK);
> > +               clockman_write(clockman, data->fbdiv_int_reg, 20);
> > +               clockman_write(clockman, data->fbdiv_frac_reg, 0);
> > +               clockman_write(clockman, data->cs_reg, 1 << PLL_CS_REFDIV_SHIFT);
> > +       }
> > +
> > +       /* Come out of reset. */
> > +       fbdiv_frac = clockman_read(clockman, data->fbdiv_frac_reg);
> > +       clockman_write(clockman, data->pwr_reg, fbdiv_frac ? 0 : PLL_PWR_DSMPD);
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       /* Wait for the PLL to lock. */
> > +       timeout = ktime_add_ns(ktime_get(), LOCK_TIMEOUT_NS);
> > +       while (!(clockman_read(clockman, data->cs_reg) & PLL_CS_LOCK)) {
> > +               if (ktime_after(ktime_get(), timeout)) {
> > +                       dev_err(clockman->dev, "%s: can't lock PLL\n",
> > +                               clk_hw_get_name(hw));
> > +                       return -ETIMEDOUT;
> > +               }
> > +               cpu_relax();
> > +       }
> 
> Is this readl_poll_timeout()?

Yes. Since you suggested below to use regmap, this will be regmap_read_poll_timeout()
instead.

> 
> > +
> > +       return 0;
> > +}
> > +
> > +static void rp1_pll_core_off(struct clk_hw *hw)
> > +{
> > +       struct rp1_pll_core *pll_core = container_of(hw, struct rp1_pll_core, hw);
> > +       struct rp1_clockman *clockman = pll_core->clockman;
> > +       const struct rp1_pll_core_data *data = pll_core->data;
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       clockman_write(clockman, data->pwr_reg, 0);
> > +       spin_unlock(&clockman->regs_lock);
> > +}
> > +
> > +static inline unsigned long get_pll_core_divider(struct clk_hw *hw,
> > +                                                unsigned long rate,
> > +                                                unsigned long parent_rate,
> > +                                                u32 *div_int, u32 *div_frac)
> > +{
> > +       unsigned long calc_rate;
> > +       u32 fbdiv_int, fbdiv_frac;
> > +       u64 div_fp64; /* 32.32 fixed point fraction. */
> > +
> > +       /* Factor of reference clock to VCO frequency. */
> > +       div_fp64 = (u64)(rate) << 32;
> > +       div_fp64 = DIV_ROUND_CLOSEST_ULL(div_fp64, parent_rate);
> > +
> > +       /* Round the fractional component at 24 bits. */
> > +       div_fp64 += 1 << (32 - 24 - 1);
> > +
> > +       fbdiv_int = div_fp64 >> 32;
> > +       fbdiv_frac = (div_fp64 >> (32 - 24)) & 0xffffff;
> > +
> > +       calc_rate =
> > +               ((u64)parent_rate * (((u64)fbdiv_int << 24) + fbdiv_frac) + (1 << 23)) >> 24;
> > +
> > +       *div_int = fbdiv_int;
> > +       *div_frac = fbdiv_frac;
> > +
> > +       return calc_rate;
> > +}
> > +
> > +static int rp1_pll_core_set_rate(struct clk_hw *hw,
> > +                                unsigned long rate, unsigned long parent_rate)
> > +{
> > +       struct rp1_pll_core *pll_core = container_of(hw, struct rp1_pll_core, hw);
> > +       struct rp1_clockman *clockman = pll_core->clockman;
> > +       const struct rp1_pll_core_data *data = pll_core->data;
> > +       unsigned long calc_rate;
> > +       u32 fbdiv_int, fbdiv_frac;
> > +
> > +       /* Disable dividers to start with. */
> > +       spin_lock(&clockman->regs_lock);
> > +       clockman_write(clockman, data->fbdiv_int_reg, 0);
> > +       clockman_write(clockman, data->fbdiv_frac_reg, 0);
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       calc_rate = get_pll_core_divider(hw, rate, parent_rate,
> > +                                        &fbdiv_int, &fbdiv_frac);
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       clockman_write(clockman, data->pwr_reg, fbdiv_frac ? 0 : PLL_PWR_DSMPD);
> > +       clockman_write(clockman, data->fbdiv_int_reg, fbdiv_int);
> > +       clockman_write(clockman, data->fbdiv_frac_reg, fbdiv_frac);
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       /* Check that reference frequency is no greater than VCO / 16. */
> 
> Why is '16' special?

Adding Raspberry Pi guys to the mail loop hoping they can step in and
shed some light, since I don't have the necessary doc/insight to figure it
out.

> 
> > +       if (WARN_ON_ONCE(parent_rate > (rate / 16)))
> > +               return -ERANGE;
> > +
> > +       pll_core->cached_rate = calc_rate;
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       /* Don't need to divide ref unless parent_rate > (output freq / 16) */
> > +       clockman_write(clockman, data->cs_reg,
> > +                      clockman_read(clockman, data->cs_reg) |
> > +                                    (1 << PLL_CS_REFDIV_SHIFT));
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static unsigned long rp1_pll_core_recalc_rate(struct clk_hw *hw,
> > +                                             unsigned long parent_rate)
> > +{
> > +       struct rp1_pll_core *pll_core = container_of(hw, struct rp1_pll_core, hw);
> > +       struct rp1_clockman *clockman = pll_core->clockman;
> > +       const struct rp1_pll_core_data *data = pll_core->data;
> > +       u32 fbdiv_int, fbdiv_frac;
> > +       unsigned long calc_rate;
> > +
> > +       fbdiv_int = clockman_read(clockman, data->fbdiv_int_reg);
> > +       fbdiv_frac = clockman_read(clockman, data->fbdiv_frac_reg);
> > +       calc_rate =
> > +               ((u64)parent_rate * (((u64)fbdiv_int << 24) + fbdiv_frac) + (1 << 23)) >> 24;
> 
> Where does '24' come from? Can you simplify this line somehow? Maybe
> break it up into multiple lines?

Same as before, we'll need some info from the vendor to explain this.
It seems to me that it's some kind of fixed-point calculation about the
frequency that takes into account the fractional part in the addition and
round it on the result.
Regarding the multi-line split, consider it done.

> 
> > +
> > +       return calc_rate;
> > +}
> > +
> > +static long rp1_pll_core_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                                   unsigned long *parent_rate)
> > +{
> > +       u32 fbdiv_int, fbdiv_frac;
> > +       long calc_rate;
> > +
> > +       calc_rate = get_pll_core_divider(hw, rate, *parent_rate,
> > +                                        &fbdiv_int, &fbdiv_frac);
> > +       return calc_rate;
> 
> return get_pll_core_divider(...);

Ack.

> 
> > +}
> > +
> > +static void rp1_pll_core_debug_init(struct clk_hw *hw, struct dentry *dentry)
> > +{
> > +       struct rp1_pll_core *pll_core = container_of(hw, struct rp1_pll_core, hw);
> > +       struct rp1_clockman *clockman = pll_core->clockman;
> > +       const struct rp1_pll_core_data *data = pll_core->data;
> > +       struct debugfs_reg32 *regs;
> > +
> > +       regs = devm_kcalloc(clockman->dev, 4, sizeof(*regs), GFP_KERNEL);
> > +       if (!regs)
> > +               return;
> > +
> > +       regs[0].name = "cs";
> > +       regs[0].offset = data->cs_reg;
> > +       regs[1].name = "pwr";
> > +       regs[1].offset = data->pwr_reg;
> > +       regs[2].name = "fbdiv_int";
> > +       regs[2].offset = data->fbdiv_int_reg;
> > +       regs[3].name = "fbdiv_frac";
> > +       regs[3].offset = data->fbdiv_frac_reg;
> > +
> > +       rp1_debugfs_regset(clockman, 0, regs, 4, dentry);
> > +}
> 
> This can go behind CONFIG_DEBUG_FS so it isn't compiled unless debugfs
> is enabled.

Ack.

> 
> > +
> > +static void get_pll_prim_dividers(unsigned long rate, unsigned long parent_rate,
> > +                                 u32 *divider1, u32 *divider2)
> > +{
> > +       unsigned int div1, div2;
> > +       unsigned int best_div1 = 7, best_div2 = 7;
> > +       unsigned long best_rate_diff =
> > +               abs_diff(DIV_ROUND_CLOSEST(parent_rate, best_div1 * best_div2), rate);
> > +       unsigned long rate_diff, calc_rate;
> > +
> > +       for (div1 = 1; div1 <= 7; div1++) {
> > +               for (div2 = 1; div2 <= div1; div2++) {
> > +                       calc_rate = DIV_ROUND_CLOSEST(parent_rate, div1 * div2);
> > +                       rate_diff = abs_diff(calc_rate, rate);
> > +
> > +                       if (calc_rate == rate) {
> > +                               best_div1 = div1;
> > +                               best_div2 = div2;
> > +                               goto done;
> > +                       } else if (rate_diff < best_rate_diff) {
> > +                               best_div1 = div1;
> > +                               best_div2 = div2;
> > +                               best_rate_diff = rate_diff;
> > +                       }
> > +               }
> > +       }
> > +
> > +done:
> > +       *divider1 = best_div1;
> > +       *divider2 = best_div2;
> > +}
> > +
> > +static int rp1_pll_set_rate(struct clk_hw *hw,
> > +                           unsigned long rate, unsigned long parent_rate)
> > +{
> > +       struct rp1_pll *pll = container_of(hw, struct rp1_pll, hw);
> > +       struct rp1_clockman *clockman = pll->clockman;
> > +       const struct rp1_pll_data *data = pll->data;
> > +       u32 prim, prim_div1, prim_div2;
> > +
> > +       get_pll_prim_dividers(rate, parent_rate, &prim_div1, &prim_div2);
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       prim = clockman_read(clockman, data->ctrl_reg);
> > +       prim = set_register_field(prim, prim_div1, PLL_PRIM_DIV1_MASK,
> > +                                 PLL_PRIM_DIV1_SHIFT);
> > +       prim = set_register_field(prim, prim_div2, PLL_PRIM_DIV2_MASK,
> > +                                 PLL_PRIM_DIV2_SHIFT);
> > +       clockman_write(clockman, data->ctrl_reg, prim);
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static unsigned long rp1_pll_recalc_rate(struct clk_hw *hw,
> > +                                        unsigned long parent_rate)
> > +{
> > +       struct rp1_pll *pll = container_of(hw, struct rp1_pll, hw);
> > +       struct rp1_clockman *clockman = pll->clockman;
> > +       const struct rp1_pll_data *data = pll->data;
> > +       u32 prim, prim_div1, prim_div2;
> > +
> > +       prim = clockman_read(clockman, data->ctrl_reg);
> > +       prim_div1 = (prim & PLL_PRIM_DIV1_MASK) >> PLL_PRIM_DIV1_SHIFT;
> > +       prim_div2 = (prim & PLL_PRIM_DIV2_MASK) >> PLL_PRIM_DIV2_SHIFT;
> > +
> > +       if (!prim_div1 || !prim_div2) {
> > +               dev_err(clockman->dev, "%s: (%s) zero divider value\n",
> > +                       __func__, data->name);
> > +               return 0;
> > +       }
> > +
> > +       return DIV_ROUND_CLOSEST(parent_rate, prim_div1 * prim_div2);
> > +}
> > +
> > +static long rp1_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                              unsigned long *parent_rate)
> > +{
> > +       u32 div1, div2;
> > +
> > +       get_pll_prim_dividers(rate, *parent_rate, &div1, &div2);
> > +
> > +       return DIV_ROUND_CLOSEST(*parent_rate, div1 * div2);
> > +}
> > +
> > +static void rp1_pll_debug_init(struct clk_hw *hw,
> > +                              struct dentry *dentry)
> > +{
> > +       struct rp1_pll *pll = container_of(hw, struct rp1_pll, hw);
> > +       struct rp1_clockman *clockman = pll->clockman;
> > +       const struct rp1_pll_data *data = pll->data;
> > +       struct debugfs_reg32 *regs;
> > +
> > +       regs = devm_kcalloc(clockman->dev, 1, sizeof(*regs), GFP_KERNEL);
> > +       if (!regs)
> > +               return;
> > +
> > +       regs[0].name = "prim";
> > +       regs[0].offset = data->ctrl_reg;
> > +
> > +       rp1_debugfs_regset(clockman, 0, regs, 1, dentry);
> > +}
> > +
> > +static int rp1_pll_ph_is_on(struct clk_hw *hw)
> > +{
> > +       struct rp1_pll_ph *pll = container_of(hw, struct rp1_pll_ph, hw);
> > +       struct rp1_clockman *clockman = pll->clockman;
> > +       const struct rp1_pll_ph_data *data = pll->data;
> > +
> > +       return !!(clockman_read(clockman, data->ph_reg) & PLL_PH_EN);
> > +}
> > +
> > +static int rp1_pll_ph_on(struct clk_hw *hw)
> > +{
> > +       struct rp1_pll_ph *pll_ph = container_of(hw, struct rp1_pll_ph, hw);
> > +       struct rp1_clockman *clockman = pll_ph->clockman;
> > +       const struct rp1_pll_ph_data *data = pll_ph->data;
> > +       u32 ph_reg;
> > +
> > +       /* todo: ensure pri/sec is enabled! */
> 
> Capitalize TODO, or better yet do it and remove the comment.

Ack.

> 
> > +       spin_lock(&clockman->regs_lock);
> > +       ph_reg = clockman_read(clockman, data->ph_reg);
> > +       ph_reg |= data->phase << PLL_PH_PHASE_SHIFT;
> > +       ph_reg |= PLL_PH_EN;
> > +       clockman_write(clockman, data->ph_reg, ph_reg);
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static void rp1_pll_ph_off(struct clk_hw *hw)
> > +{
> > +       struct rp1_pll_ph *pll_ph = container_of(hw, struct rp1_pll_ph, hw);
> > +       struct rp1_clockman *clockman = pll_ph->clockman;
> > +       const struct rp1_pll_ph_data *data = pll_ph->data;
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       clockman_write(clockman, data->ph_reg,
> > +                      clockman_read(clockman, data->ph_reg) & ~PLL_PH_EN);
> > +       spin_unlock(&clockman->regs_lock);
> > +}
> > +
> > +static int rp1_pll_ph_set_rate(struct clk_hw *hw,
> > +                              unsigned long rate, unsigned long parent_rate)
> > +{
> > +       struct rp1_pll_ph *pll_ph = container_of(hw, struct rp1_pll_ph, hw);
> > +       const struct rp1_pll_ph_data *data = pll_ph->data;
> > +
> > +       /* Nothing really to do here! */
> 
> Is it read-only? Don't define a set_rate function then and make the rate
> determination function return the same value all the time.

Not 100% sure about it, maybe Raspberry Pi colleagues can explain.
By 'rate determination function' you're referring (in this case) to
rp1_pll_ph_recalc_rate(), right? If so, that clock type seems to have
a fixed divider but teh resulting clock depends on the parent rate, so
it has to be calculated.

> 
> > +       WARN_ON(data->fixed_divider != 1 && data->fixed_divider != 2);
> > +       WARN_ON(rate != parent_rate / data->fixed_divider);
> > +
> > +       return 0;
> > +}
> > +
> > +static unsigned long rp1_pll_ph_recalc_rate(struct clk_hw *hw,
> > +                                           unsigned long parent_rate)
> > +{
> > +       struct rp1_pll_ph *pll_ph = container_of(hw, struct rp1_pll_ph, hw);
> > +       const struct rp1_pll_ph_data *data = pll_ph->data;
> > +
> > +       return parent_rate / data->fixed_divider;
> > +}
> > +
> > +static long rp1_pll_ph_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                                 unsigned long *parent_rate)
> > +{
> > +       struct rp1_pll_ph *pll_ph = container_of(hw, struct rp1_pll_ph, hw);
> > +       const struct rp1_pll_ph_data *data = pll_ph->data;
> > +
> > +       return *parent_rate / data->fixed_divider;
> > +}
> > +
> > +static void rp1_pll_ph_debug_init(struct clk_hw *hw,
> > +                                 struct dentry *dentry)
> > +{
> > +       struct rp1_pll_ph *pll_ph = container_of(hw, struct rp1_pll_ph, hw);
> > +       const struct rp1_pll_ph_data *data = pll_ph->data;
> > +       struct rp1_clockman *clockman = pll_ph->clockman;
> > +       struct debugfs_reg32 *regs;
> > +
> > +       regs = devm_kcalloc(clockman->dev, 1, sizeof(*regs), GFP_KERNEL);
> > +       if (!regs)
> > +               return;
> > +
> > +       regs[0].name = "ph_reg";
> > +       regs[0].offset = data->ph_reg;
> > +
> > +       rp1_debugfs_regset(clockman, 0, regs, 1, dentry);
> > +}
> > +
> > +static int rp1_pll_divider_is_on(struct clk_hw *hw)
> > +{
> > +       struct rp1_pll *divider = container_of(hw, struct rp1_pll, div.hw);
> > +       struct rp1_clockman *clockman = divider->clockman;
> > +       const struct rp1_pll_data *data = divider->data;
> > +
> > +       return !(clockman_read(clockman, data->ctrl_reg) & PLL_SEC_RST);
> > +}
> > +
> > +static int rp1_pll_divider_on(struct clk_hw *hw)
> > +{
> > +       struct rp1_pll *divider = container_of(hw, struct rp1_pll, div.hw);
> > +       struct rp1_clockman *clockman = divider->clockman;
> > +       const struct rp1_pll_data *data = divider->data;
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       /* Check the implementation bit is set! */
> > +       WARN_ON(!(clockman_read(clockman, data->ctrl_reg) & PLL_SEC_IMPL));
> > +       clockman_write(clockman, data->ctrl_reg,
> > +                      clockman_read(clockman, data->ctrl_reg) & ~PLL_SEC_RST);
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static void rp1_pll_divider_off(struct clk_hw *hw)
> > +{
> > +       struct rp1_pll *divider = container_of(hw, struct rp1_pll, div.hw);
> > +       struct rp1_clockman *clockman = divider->clockman;
> > +       const struct rp1_pll_data *data = divider->data;
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       clockman_write(clockman, data->ctrl_reg, PLL_SEC_RST);
> > +       spin_unlock(&clockman->regs_lock);
> > +}
> > +
> > +static int rp1_pll_divider_set_rate(struct clk_hw *hw,
> > +                                   unsigned long rate,
> > +                                   unsigned long parent_rate)
> > +{
> > +       struct rp1_pll *divider = container_of(hw, struct rp1_pll, div.hw);
> > +       struct rp1_clockman *clockman = divider->clockman;
> > +       const struct rp1_pll_data *data = divider->data;
> > +       u32 div, sec;
> > +
> > +       div = DIV_ROUND_UP_ULL(parent_rate, rate);
> > +       div = clamp(div, 8u, 19u);
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       sec = clockman_read(clockman, data->ctrl_reg);
> > +       sec = set_register_field(sec, div, PLL_SEC_DIV_MASK, PLL_SEC_DIV_SHIFT);
> > +
> > +       /* Must keep the divider in reset to change the value. */
> > +       sec |= PLL_SEC_RST;
> > +       clockman_write(clockman, data->ctrl_reg, sec);
> > +
> > +       // todo: must sleep 10 pll vco cycles
> > +       sec &= ~PLL_SEC_RST;
> > +       clockman_write(clockman, data->ctrl_reg, sec);
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static unsigned long rp1_pll_divider_recalc_rate(struct clk_hw *hw,
> > +                                                unsigned long parent_rate)
> > +{
> > +       return clk_divider_ops.recalc_rate(hw, parent_rate);
> > +}
> > +
> > +static long rp1_pll_divider_round_rate(struct clk_hw *hw,
> > +                                      unsigned long rate,
> > +                                      unsigned long *parent_rate)
> > +{
> > +       return clk_divider_ops.round_rate(hw, rate, parent_rate);
> > +}
> > +
> > +static void rp1_pll_divider_debug_init(struct clk_hw *hw, struct dentry *dentry)
> > +{
> > +       struct rp1_pll *divider = container_of(hw, struct rp1_pll, div.hw);
> > +       struct rp1_clockman *clockman = divider->clockman;
> > +       const struct rp1_pll_data *data = divider->data;
> > +       struct debugfs_reg32 *regs;
> > +
> > +       regs = devm_kcalloc(clockman->dev, 1, sizeof(*regs), GFP_KERNEL);
> > +       if (!regs)
> > +               return;
> > +
> > +       regs[0].name = "sec";
> > +       regs[0].offset = data->ctrl_reg;
> > +
> > +       rp1_debugfs_regset(clockman, 0, regs, 1, dentry);
> > +}
> > +
> > +static int rp1_clock_is_on(struct clk_hw *hw)
> > +{
> > +       struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw);
> > +       struct rp1_clockman *clockman = clock->clockman;
> > +       const struct rp1_clock_data *data = clock->data;
> > +
> > +       return !!(clockman_read(clockman, data->ctrl_reg) & CLK_CTRL_ENABLE);
> > +}
> > +
> > +static unsigned long rp1_clock_recalc_rate(struct clk_hw *hw,
> > +                                          unsigned long parent_rate)
> > +{
> > +       struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw);
> > +       struct rp1_clockman *clockman = clock->clockman;
> > +       const struct rp1_clock_data *data = clock->data;
> > +       u64 calc_rate;
> > +       u64 div;
> > +
> > +       u32 frac;
> > +
> > +       div = clockman_read(clockman, data->div_int_reg);
> > +       frac = (data->div_frac_reg != 0) ?
> > +               clockman_read(clockman, data->div_frac_reg) : 0;
> > +
> > +       /* If the integer portion of the divider is 0, treat it as 2^16 */
> > +       if (!div)
> > +               div = 1 << 16;
> > +
> > +       div = (div << CLK_DIV_FRAC_BITS) | (frac >> (32 - CLK_DIV_FRAC_BITS));
> > +
> > +       calc_rate = (u64)parent_rate << CLK_DIV_FRAC_BITS;
> > +       calc_rate = div64_u64(calc_rate, div);
> > +
> > +       return calc_rate;
> > +}
> > +
> > +static int rp1_clock_on(struct clk_hw *hw)
> > +{
> > +       struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw);
> > +       struct rp1_clockman *clockman = clock->clockman;
> > +       const struct rp1_clock_data *data = clock->data;
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       clockman_write(clockman, data->ctrl_reg,
> > +                      clockman_read(clockman, data->ctrl_reg) | CLK_CTRL_ENABLE);
> > +       /* If this is a GPCLK, turn on the output-enable */
> > +       if (data->oe_mask)
> > +               clockman_write(clockman, GPCLK_OE_CTRL,
> > +                              clockman_read(clockman, GPCLK_OE_CTRL) | data->oe_mask);
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static void rp1_clock_off(struct clk_hw *hw)
> > +{
> > +       struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw);
> > +       struct rp1_clockman *clockman = clock->clockman;
> > +       const struct rp1_clock_data *data = clock->data;
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       clockman_write(clockman, data->ctrl_reg,
> > +                      clockman_read(clockman, data->ctrl_reg) & ~CLK_CTRL_ENABLE);
> > +       /* If this is a GPCLK, turn off the output-enable */
> > +       if (data->oe_mask)
> > +               clockman_write(clockman, GPCLK_OE_CTRL,
> > +                              clockman_read(clockman, GPCLK_OE_CTRL) & ~data->oe_mask);
> > +       spin_unlock(&clockman->regs_lock);
> > +}
> > +
> > +static u32 rp1_clock_choose_div(unsigned long rate, unsigned long parent_rate,
> > +                               const struct rp1_clock_data *data)
> > +{
> > +       u64 div;
> > +
> > +       /*
> > +        * Due to earlier rounding, calculated parent_rate may differ from
> > +        * expected value. Don't fail on a small discrepancy near unity divide.
> > +        */
> > +       if (!rate || rate > parent_rate + (parent_rate >> CLK_DIV_FRAC_BITS))
> > +               return 0;
> > +
> > +       /*
> > +        * Always express div in fixed-point format for fractional division;
> > +        * If no fractional divider is present, the fraction part will be zero.
> > +        */
> > +       if (data->div_frac_reg) {
> > +               div = (u64)parent_rate << CLK_DIV_FRAC_BITS;
> > +               div = DIV_ROUND_CLOSEST_ULL(div, rate);
> > +       } else {
> > +               div = DIV_ROUND_CLOSEST_ULL(parent_rate, rate);
> > +               div <<= CLK_DIV_FRAC_BITS;
> > +       }
> > +
> > +       div = clamp(div,
> > +                   1ull << CLK_DIV_FRAC_BITS,
> > +                   (u64)data->div_int_max << CLK_DIV_FRAC_BITS);
> > +
> > +       return div;
> > +}
> > +
> > +static u8 rp1_clock_get_parent(struct clk_hw *hw)
> > +{
> > +       struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw);
> > +       struct rp1_clockman *clockman = clock->clockman;
> > +       const struct rp1_clock_data *data = clock->data;
> > +       u32 sel, ctrl;
> > +       u8 parent;
> > +
> > +       /* Sel is one-hot, so find the first bit set */
> > +       sel = clockman_read(clockman, data->sel_reg);
> > +       parent = ffs(sel) - 1;
> > +
> > +       /* sel == 0 implies the parent clock is not enabled yet. */
> > +       if (!sel) {
> > +               /* Read the clock src from the CTRL register instead */
> > +               ctrl = clockman_read(clockman, data->ctrl_reg);
> > +               parent = (ctrl & data->clk_src_mask) >> CLK_CTRL_SRC_SHIFT;
> > +       }
> > +
> > +       if (parent >= data->num_std_parents)
> > +               parent = AUX_SEL;
> > +
> > +       if (parent == AUX_SEL) {
> > +               /*
> > +                * Clock parent is an auxiliary source, so get the parent from
> > +                * the AUXSRC register field.
> > +                */
> > +               ctrl = clockman_read(clockman, data->ctrl_reg);
> > +               parent = (ctrl & CLK_CTRL_AUXSRC_MASK) >> CLK_CTRL_AUXSRC_SHIFT;
> > +               parent += data->num_std_parents;
> > +       }
> > +
> > +       return parent;
> > +}
> > +
> > +static int rp1_clock_set_parent(struct clk_hw *hw, u8 index)
> > +{
> > +       struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw);
> > +       struct rp1_clockman *clockman = clock->clockman;
> > +       const struct rp1_clock_data *data = clock->data;
> > +       u32 ctrl, sel;
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +       ctrl = clockman_read(clockman, data->ctrl_reg);
> > +
> > +       if (index >= data->num_std_parents) {
> > +               /* This is an aux source request */
> > +               if (index >= data->num_std_parents + data->num_aux_parents) {
> > +                       spin_unlock(&clockman->regs_lock);
> > +                       return -EINVAL;
> > +               }
> > +
> > +               /* Select parent from aux list */
> > +               ctrl = set_register_field(ctrl, index - data->num_std_parents,
> > +                                         CLK_CTRL_AUXSRC_MASK,
> > +                                         CLK_CTRL_AUXSRC_SHIFT);
> > +               /* Set src to aux list */
> > +               ctrl = set_register_field(ctrl, AUX_SEL, data->clk_src_mask,
> > +                                         CLK_CTRL_SRC_SHIFT);
> > +       } else {
> > +               ctrl = set_register_field(ctrl, index, data->clk_src_mask,
> > +                                         CLK_CTRL_SRC_SHIFT);
> > +       }
> > +
> > +       clockman_write(clockman, data->ctrl_reg, ctrl);
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       sel = rp1_clock_get_parent(hw);
> > +       WARN(sel != index, "(%s): Parent index req %u returned back %u\n",
> > +            data->name, index, sel);
> > +
> > +       return 0;
> > +}
> > +
> > +static int rp1_clock_set_rate_and_parent(struct clk_hw *hw,
> > +                                        unsigned long rate,
> > +                                        unsigned long parent_rate,
> > +                                        u8 parent)
> > +{
> > +       struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw);
> > +       struct rp1_clockman *clockman = clock->clockman;
> > +       const struct rp1_clock_data *data = clock->data;
> > +       u32 div = rp1_clock_choose_div(rate, parent_rate, data);
> > +
> > +       WARN(rate > 4000000000ll, "rate is -ve (%d)\n", (int)rate);
> > +
> > +       if (WARN(!div,
> > +                "clk divider calculated as 0! (%s, rate %ld, parent rate %ld)\n",
> > +                data->name, rate, parent_rate))
> > +               div = 1 << CLK_DIV_FRAC_BITS;
> > +
> > +       spin_lock(&clockman->regs_lock);
> > +
> > +       clockman_write(clockman, data->div_int_reg, div >> CLK_DIV_FRAC_BITS);
> > +       if (data->div_frac_reg)
> > +               clockman_write(clockman, data->div_frac_reg, div << (32 - CLK_DIV_FRAC_BITS));
> > +
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       if (parent != 0xff)
> > +               rp1_clock_set_parent(hw, parent);
> > +
> > +       return 0;
> > +}
> > +
> > +static int rp1_clock_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                             unsigned long parent_rate)
> > +{
> > +       return rp1_clock_set_rate_and_parent(hw, rate, parent_rate, 0xff);
> > +}
> > +
> > +static void rp1_clock_choose_div_and_prate(struct clk_hw *hw,
> > +                                          int parent_idx,
> > +                                          unsigned long rate,
> > +                                          unsigned long *prate,
> > +                                          unsigned long *calc_rate)
> > +{
> > +       struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw);
> > +       const struct rp1_clock_data *data = clock->data;
> > +       struct clk_hw *parent;
> > +       u32 div;
> > +       u64 tmp;
> > +
> > +       parent = clk_hw_get_parent_by_index(hw, parent_idx);
> > +
> > +       *prate = clk_hw_get_rate(parent);
> > +       div = rp1_clock_choose_div(rate, *prate, data);
> > +
> > +       if (!div) {
> > +               *calc_rate = 0;
> > +               return;
> > +       }
> > +
> > +       /* Recalculate to account for rounding errors */
> > +       tmp = (u64)*prate << CLK_DIV_FRAC_BITS;
> > +       tmp = div_u64(tmp, div);
> > +
> > +       /*
> > +        * Prevent overclocks - if all parent choices result in
> > +        * a downstream clock in excess of the maximum, then the
> > +        * call to set the clock will fail.
> > +        */
> > +       if (tmp > clock->data->max_freq)
> > +               *calc_rate = 0;
> > +       else
> > +               *calc_rate = tmp;
> > +}
> > +
> > +static int rp1_clock_determine_rate(struct clk_hw *hw,
> > +                                   struct clk_rate_request *req)
> > +{
> > +       struct clk_hw *parent, *best_parent = NULL;
> > +       unsigned long best_rate = 0;
> > +       unsigned long best_prate = 0;
> > +       unsigned long best_rate_diff = ULONG_MAX;
> > +       unsigned long prate, calc_rate;
> > +       size_t i;
> > +
> > +       /*
> > +        * If the NO_REPARENT flag is set, try to use existing parent.
> > +        */
> > +       if ((clk_hw_get_flags(hw) & CLK_SET_RATE_NO_REPARENT)) {
> 
> Is this flag ever set?

Not right now, but it will be used as soon as I'll add the video clocks,
so I thought to leave it be to avoid adding it back in the future.
For this minimal support is not needed though, so let me know if you
want it removed.

> 
> > +               i = rp1_clock_get_parent(hw);
> > +               parent = clk_hw_get_parent_by_index(hw, i);
> > +               if (parent) {
> > +                       rp1_clock_choose_div_and_prate(hw, i, req->rate, &prate,
> > +                                                      &calc_rate);
> > +                       if (calc_rate > 0) {
> > +                               req->best_parent_hw = parent;
> > +                               req->best_parent_rate = prate;
> > +                               req->rate = calc_rate;
> > +                               return 0;
> > +                       }
> > +               }
> > +       }
> > +
> > +       /*
> > +        * Select parent clock that results in the closest rate (lower or
> > +        * higher)
> > +        */
> > +       for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
> > +               parent = clk_hw_get_parent_by_index(hw, i);
> > +               if (!parent)
> > +                       continue;
> > +
> > +               rp1_clock_choose_div_and_prate(hw, i, req->rate, &prate,
> > +                                              &calc_rate);
> > +
> > +               if (abs_diff(calc_rate, req->rate) < best_rate_diff) {
> > +                       best_parent = parent;
> > +                       best_prate = prate;
> > +                       best_rate = calc_rate;
> > +                       best_rate_diff = abs_diff(calc_rate, req->rate);
> > +
> > +                       if (best_rate_diff == 0)
> > +                               break;
> > +               }
> > +       }
> > +
> > +       if (best_rate == 0)
> > +               return -EINVAL;
> > +
> > +       req->best_parent_hw = best_parent;
> > +       req->best_parent_rate = best_prate;
> > +       req->rate = best_rate;
> > +
> > +       return 0;
> > +}
> > +
> > +static void rp1_clk_debug_init(struct clk_hw *hw, struct dentry *dentry)
> > +{
> > +       struct rp1_clock *clock = container_of(hw, struct rp1_clock, hw);
> > +       struct rp1_clockman *clockman = clock->clockman;
> > +       const struct rp1_clock_data *data = clock->data;
> > +       struct debugfs_reg32 *regs;
> > +       int i;
> > +
> > +       regs = devm_kcalloc(clockman->dev, 4, sizeof(*regs), GFP_KERNEL);
> > +       if (!regs)
> > +               return;
> > +
> > +       i = 0;
> > +       regs[i].name = "ctrl";
> > +       regs[i++].offset = data->ctrl_reg;
> > +       regs[i].name = "div_int";
> > +       regs[i++].offset = data->div_int_reg;
> > +       regs[i].name = "div_frac";
> > +       regs[i++].offset = data->div_frac_reg;
> > +       regs[i].name = "sel";
> > +       regs[i++].offset = data->sel_reg;
> 
> This time we get i but earlier it was hard-coded. Please be consistent.
> I suspect hard-coded is easier to follow so just do that.

Ack.

> 
> > +
> > +       rp1_debugfs_regset(clockman, 0, regs, i, dentry);
> 
> I also wonder if regmap could be used? That has debugfs suport to read
> registers builtin already.

Good idea.

> 
> > +}
> > +
> > +static const struct clk_ops rp1_pll_core_ops = {
> > +       .is_prepared = rp1_pll_core_is_on,
> > +       .prepare = rp1_pll_core_on,
> > +       .unprepare = rp1_pll_core_off,
> > +       .set_rate = rp1_pll_core_set_rate,
> > +       .recalc_rate = rp1_pll_core_recalc_rate,
> > +       .round_rate = rp1_pll_core_round_rate,
> > +       .debug_init = rp1_pll_core_debug_init,
> > +};
> > +
> > +static const struct clk_ops rp1_pll_ops = {
> > +       .set_rate = rp1_pll_set_rate,
> > +       .recalc_rate = rp1_pll_recalc_rate,
> > +       .round_rate = rp1_pll_round_rate,
> > +       .debug_init = rp1_pll_debug_init,
> > +};
> > +
> > +static const struct clk_ops rp1_pll_ph_ops = {
> > +       .is_prepared = rp1_pll_ph_is_on,
> > +       .prepare = rp1_pll_ph_on,
> > +       .unprepare = rp1_pll_ph_off,
> > +       .set_rate = rp1_pll_ph_set_rate,
> > +       .recalc_rate = rp1_pll_ph_recalc_rate,
> > +       .round_rate = rp1_pll_ph_round_rate,
> > +       .debug_init = rp1_pll_ph_debug_init,
> > +};
> > +
> > +static const struct clk_ops rp1_pll_divider_ops = {
> > +       .is_prepared = rp1_pll_divider_is_on,
> > +       .prepare = rp1_pll_divider_on,
> > +       .unprepare = rp1_pll_divider_off,
> > +       .set_rate = rp1_pll_divider_set_rate,
> > +       .recalc_rate = rp1_pll_divider_recalc_rate,
> > +       .round_rate = rp1_pll_divider_round_rate,
> > +       .debug_init = rp1_pll_divider_debug_init,
> > +};
> > +
> > +static const struct clk_ops rp1_clk_ops = {
> > +       .is_prepared = rp1_clock_is_on,
> > +       .prepare = rp1_clock_on,
> > +       .unprepare = rp1_clock_off,
> > +       .recalc_rate = rp1_clock_recalc_rate,
> > +       .get_parent = rp1_clock_get_parent,
> > +       .set_parent = rp1_clock_set_parent,
> > +       .set_rate_and_parent = rp1_clock_set_rate_and_parent,
> > +       .set_rate = rp1_clock_set_rate,
> > +       .determine_rate = rp1_clock_determine_rate,
> > +       .debug_init = rp1_clk_debug_init,
> > +};
> > +
> > +static struct clk_hw *rp1_register_pll_core(struct rp1_clockman *clockman,
> > +                                           const void *data)
> > +{
> > +       const char *ref_clk_name = clk_hw_get_name(clockman->hw_xosc);
> > +       const struct rp1_pll_core_data *pll_core_data = data;
> > +       struct rp1_pll_core *pll_core;
> > +       struct clk_init_data init;
> > +       int ret;
> > +
> > +       memset(&init, 0, sizeof(init));
> 
> I think struct clk_init_data init = { } is more the style in clk
> drivers. Please do that instead of calling memset().

Ack.

> 
> > +
> > +       /* All of the PLL cores derive from the external oscillator. */
> > +       init.parent_names = &ref_clk_name;
> > +       init.num_parents = 1;
> > +       init.name = pll_core_data->name;
> > +       init.ops = &rp1_pll_core_ops;
> > +       init.flags = pll_core_data->flags | CLK_IGNORE_UNUSED | CLK_IS_CRITICAL;
> > +
> > +       pll_core = devm_kzalloc(clockman->dev, sizeof(*pll_core), GFP_KERNEL);
> > +       if (!pll_core)
> > +               return NULL;
> > +
> > +       pll_core->clockman = clockman;
> > +       pll_core->data = pll_core_data;
> > +       pll_core->hw.init = &init;
> > +
> > +       ret = devm_clk_hw_register(clockman->dev, &pll_core->hw);
> > +       if (ret)
> > +               return ERR_PTR(ret);
> > +
> > +       return &pll_core->hw;
> > +}
> > +
> > +static struct clk_hw *rp1_register_pll(struct rp1_clockman *clockman,
> > +                                      const void *data)
> > +{
> > +       const struct rp1_pll_data *pll_data = data;
> > +       struct rp1_pll *pll;
> > +       struct clk_init_data init;
> > +       int ret;
> > +
> > +       memset(&init, 0, sizeof(init));
> > +
> > +       init.parent_names = &pll_data->source_pll;
> > +       init.num_parents = 1;
> > +       init.name = pll_data->name;
> > +       init.ops = &rp1_pll_ops;
> > +       init.flags = pll_data->flags | CLK_IGNORE_UNUSED | CLK_IS_CRITICAL;
> > +
> > +       pll = devm_kzalloc(clockman->dev, sizeof(*pll), GFP_KERNEL);
> > +       if (!pll)
> > +               return NULL;
> > +
> > +       pll->clockman = clockman;
> > +       pll->data = pll_data;
> > +       pll->hw.init = &init;
> > +
> > +       ret = devm_clk_hw_register(clockman->dev, &pll->hw);
> > +       if (ret)
> > +               return ERR_PTR(ret);
> > +
> > +       return &pll->hw;
> > +}
> > +
> > +static struct clk_hw *rp1_register_pll_ph(struct rp1_clockman *clockman,
> > +                                         const void *data)
> > +{
> > +       const struct rp1_pll_ph_data *ph_data = data;
> > +       struct rp1_pll_ph *ph;
> > +       struct clk_init_data init;
> > +       int ret;
> > +
> > +       memset(&init, 0, sizeof(init));
> > +
> > +       init.parent_names = &ph_data->source_pll;
> > +       init.num_parents = 1;
> > +       init.name = ph_data->name;
> > +       init.ops = &rp1_pll_ph_ops;
> > +       init.flags = ph_data->flags | CLK_IGNORE_UNUSED;
> > +
> > +       ph = devm_kzalloc(clockman->dev, sizeof(*ph), GFP_KERNEL);
> > +       if (!ph)
> > +               return NULL;
> > +
> > +       ph->clockman = clockman;
> > +       ph->data = ph_data;
> > +       ph->hw.init = &init;
> > +
> > +       ret = devm_clk_hw_register(clockman->dev, &ph->hw);
> > +       if (ret)
> > +               return ERR_PTR(ret);
> > +
> > +       return &ph->hw;
> > +}
> > +
> > +static struct clk_hw *rp1_register_pll_divider(struct rp1_clockman *clockman,
> > +                                              const void *data)
> > +{
> > +       const struct rp1_pll_data *divider_data = data;
> > +       struct rp1_pll *divider;
> > +       struct clk_init_data init;
> > +       int ret;
> > +
> > +       memset(&init, 0, sizeof(init));
> > +
> > +       init.parent_names = &divider_data->source_pll;
> > +       init.num_parents = 1;
> > +       init.name = divider_data->name;
> > +       init.ops = &rp1_pll_divider_ops;
> > +       init.flags = divider_data->flags | CLK_IGNORE_UNUSED | CLK_IS_CRITICAL;
> > +
> > +       divider = devm_kzalloc(clockman->dev, sizeof(*divider), GFP_KERNEL);
> > +       if (!divider)
> > +               return NULL;
> > +
> > +       divider->div.reg = clockman->regs + divider_data->ctrl_reg;
> > +       divider->div.shift = PLL_SEC_DIV_SHIFT;
> > +       divider->div.width = PLL_SEC_DIV_WIDTH;
> > +       divider->div.flags = CLK_DIVIDER_ROUND_CLOSEST;
> > +       divider->div.flags |= CLK_IS_CRITICAL;
> 
> Is everything critical? The usage of this flag and CLK_IGNORE_UNUSED is
> suspicious and likely working around some problems elsewhere.

Not sure... maybe they wanted to control the enablement through the parent
pll_sys_core only? RaspberryPi folks can you please step-in on this?

> 
> > +       divider->div.lock = &clockman->regs_lock;
> > +       divider->div.hw.init = &init;
> > +       divider->div.table = pll_sec_div_table;
> > +
> > +       divider->clockman = clockman;
> > +       divider->data = divider_data;
> > +
> > +       ret = devm_clk_hw_register(clockman->dev, &divider->div.hw);
> > +       if (ret)
> > +               return ERR_PTR(ret);
> > +
> > +       return &divider->div.hw;
> > +}
> > +
> > +static struct clk_hw *rp1_register_clock(struct rp1_clockman *clockman,
> > +                                        const void *data)
> > +{
> > +       const struct rp1_clock_data *clock_data = data;
> > +       struct rp1_clock *clock;
> > +       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 &&
> > +                        strcmp("-", clock_data->parents[AUX_SEL])))
> > +               return NULL;
> > +
> > +       memset(&init, 0, sizeof(init));
> > +       init.parent_names = clock_data->parents;
> > +       init.num_parents = clock_data->num_std_parents +
> > +                          clock_data->num_aux_parents;
> > +       init.name = clock_data->name;
> > +       init.flags = clock_data->flags | CLK_IGNORE_UNUSED;
> > +       init.ops = &rp1_clk_ops;
> > +
> > +       clock = devm_kzalloc(clockman->dev, sizeof(*clock), GFP_KERNEL);
> > +       if (!clock)
> > +               return NULL;
> > +
> > +       clock->clockman = clockman;
> > +       clock->data = clock_data;
> > +       clock->hw.init = &init;
> > +
> > +       ret = devm_clk_hw_register(clockman->dev, &clock->hw);
> > +       if (ret)
> > +               return ERR_PTR(ret);
> > +
> > +       return &clock->hw;
> > +}
> > +
> > +/* Assignment helper macros for different clock types. */
> > +#define _REGISTER(f, ...) { .clk_register = f, .data = __VA_ARGS__ }
> > +
> > +#define REGISTER_PLL_CORE(...) _REGISTER(&rp1_register_pll_core,       \
> > +                                         &(struct rp1_pll_core_data)   \
> > +                                         {__VA_ARGS__})
> > +
> > +#define REGISTER_PLL(...)      _REGISTER(&rp1_register_pll,            \
> > +                                         &(struct rp1_pll_data)        \
> > +                                         {__VA_ARGS__})
> > +
> > +#define REGISTER_PLL_PH(...)   _REGISTER(&rp1_register_pll_ph,         \
> > +                                         &(struct rp1_pll_ph_data)     \
> > +                                         {__VA_ARGS__})
> > +
> > +#define REGISTER_PLL_DIV(...)  _REGISTER(&rp1_register_pll_divider,    \
> > +                                         &(struct rp1_pll_data)        \
> > +                                         {__VA_ARGS__})
> > +
> > +#define REGISTER_CLK(...)      _REGISTER(&rp1_register_clock,          \
> > +                                         &(struct rp1_clock_data)      \
> > +                                         {__VA_ARGS__})
> > +
> > +static const struct rp1_clk_desc clk_desc_array[] = {
> > +       [RP1_PLL_SYS_CORE] = REGISTER_PLL_CORE(.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(.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(.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(.name = "pll_sys",
> > +                               .source_pll = "pll_sys_core",
> > +                               .ctrl_reg = PLL_SYS_PRIM,
> > +                               .fc0_src = FC_NUM(0, 2),
> > +                               ),
> > +
> > +       [RP1_CLK_ETH_TSU] = REGISTER_CLK(.name = "clk_eth_tsu",
> > +                               .parents = {"rp1-xosc"},
> > +                               .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 * MHz,
> > +                               .fc0_src = FC_NUM(5, 7),
> > +                               ),
> > +
> > +       [RP1_CLK_SYS] = REGISTER_CLK(.name = "clk_sys",
> > +                               .parents = {"rp1-xosc", "-", "pll_sys"},
> 
> Please use struct clk_parent_data or clk_hw directly. Don't use strings
> to describe parents.

Describing parents as as strings allows to directly assign it to struct
clk_init_data, as in rp1_register_clock():

const struct rp1_clock_data *clock_data = data;
struct clk_init_data init = { };
...
init.parent_names = clock_data->parents;

otherwise we should create an array and populate from clk_parent_data::name,
which is of course feasible but a bit less compact. Are you sure you want
to change it?

> 
> > +                               .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 * MHz,
> > +                               .fc0_src = FC_NUM(0, 4),
> > +                               .clk_src_mask = 0x3,
> > +                               ),
> > +
> > +       [RP1_PLL_SYS_PRI_PH] = REGISTER_PLL_PH(.name = "pll_sys_pri_ph",
> > +                               .source_pll = "pll_sys",
> > +                               .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(.name = "pll_sys_sec",
> > +                               .source_pll = "pll_sys_core",
> > +                               .ctrl_reg = PLL_SYS_SEC,
> > +                               .fc0_src = FC_NUM(2, 2),
> > +                               ),
> > +};
> > +
> > +static int rp1_clk_probe(struct platform_device *pdev)
> > +{
> > +       const size_t asize = ARRAY_SIZE(clk_desc_array);
> > +       const struct rp1_clk_desc *desc;
> > +       struct device *dev = &pdev->dev;
> > +       struct rp1_clockman *clockman;
> 
> I love the name 'clockman'!
> 
> > +       struct clk *clk_xosc;
> > +       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);
> > +
> > +       clk_xosc = devm_clk_get_enabled(dev, NULL);
> > +       if (IS_ERR(clk_xosc))
> > +               return PTR_ERR(clk_xosc);
> > +
> > +       clockman->hw_xosc = __clk_get_hw(clk_xosc);
> 
> Please use struct clk_parent_data::index instead.

Sorry, I didn't catch what you mean here. Can you please elaborate?

> 
> > +       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->data);
> > +                       if (IS_ERR_OR_NULL(hws[i]))
> > +                               dev_err(dev, "Unable to register clock: %s\n",
> 
> Use dev_err_probe() please.

Ack.

> 
> > +                                       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);
> > +}
> > +
> > +static const struct of_device_id rp1_clk_of_match[] = {
> > +       { .compatible = "raspberrypi,rp1-clocks" },
> > +       {}
> > +};
> > +MODULE_DEVICE_TABLE(of, rp1_clk_of_match);
> > +
> > +static struct platform_driver rp1_clk_driver = {
> > +       .driver = {
> > +               .name = "rp1-clk",
> > +               .of_match_table = rp1_clk_of_match,
> > +       },
> > +       .probe = rp1_clk_probe,
> > +};
> > +
> > +static int __init rp1_clk_driver_init(void)
> > +{
> > +       return platform_driver_register(&rp1_clk_driver);
> > +}
> > +postcore_initcall(rp1_clk_driver_init);
> > +
> > +static void __exit rp1_clk_driver_exit(void)
> > +{
> > +       platform_driver_unregister(&rp1_clk_driver);
> > +}
> > +module_exit(rp1_clk_driver_exit);
> 
> Can you use module_platform_driver()?

Sure.

Many thanks,
Andrea

Re: [PATCH v2 08/14] clk: rp1: Add support for clocks provided by RP1

[Herve Codina]

Hi Andrea,


On Wed, 23 Oct 2024 17:36:06 +0200
Andrea della Porta <andrea.porta@suse.com> wrote:

> Hi Stephen,
> 
> On 15:08 Wed 09 Oct     , Stephen Boyd wrote:
> > Quoting Andrea della Porta (2024-10-07 05:39:51)  
> > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > > index 299bc678ed1b..537019987f0c 100644
> > > --- a/drivers/clk/Kconfig
> > > +++ b/drivers/clk/Kconfig
> > > @@ -88,6 +88,15 @@ config COMMON_CLK_RK808
> > >           These multi-function devices have two fixed-rate oscillators, clocked at 32KHz each.
> > >           Clkout1 is always on, Clkout2 can off by control register.
> > >  
> > > +config COMMON_CLK_RP1
> > > +       tristate "Raspberry Pi RP1-based clock support"
> > > +       depends on PCI || COMPILE_TEST  
> > 
> > A better limit would be some ARCH_* config.  
> 
> I've avoided ARCH_BCM2835 since the original intention is for this driver
> to work (in the future) also for custom PCI cards with RP1 on-board, and not
> only for Rpi5.

Maybe depends on CONFIG_MISC_RP1 ?
CONFIG_MISC_RP1 enables the RP1 PCI driver

Best regards,
Hervé

Re: [PATCH v2 08/14] clk: rp1: Add support for clocks provided by RP1

[Andrea della Porta]

Hi Herve,

On 18:32 Wed 23 Oct     , Herve Codina wrote:
> Hi Andrea,
> 
> 
> On Wed, 23 Oct 2024 17:36:06 +0200
> Andrea della Porta <andrea.porta@suse.com> wrote:
> 
> > Hi Stephen,
> > 
> > On 15:08 Wed 09 Oct     , Stephen Boyd wrote:
> > > Quoting Andrea della Porta (2024-10-07 05:39:51)  
> > > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > > > index 299bc678ed1b..537019987f0c 100644
> > > > --- a/drivers/clk/Kconfig
> > > > +++ b/drivers/clk/Kconfig
> > > > @@ -88,6 +88,15 @@ config COMMON_CLK_RK808
> > > >           These multi-function devices have two fixed-rate oscillators, clocked at 32KHz each.
> > > >           Clkout1 is always on, Clkout2 can off by control register.
> > > >  
> > > > +config COMMON_CLK_RP1
> > > > +       tristate "Raspberry Pi RP1-based clock support"
> > > > +       depends on PCI || COMPILE_TEST  
> > > 
> > > A better limit would be some ARCH_* config.  
> > 
> > I've avoided ARCH_BCM2835 since the original intention is for this driver
> > to work (in the future) also for custom PCI cards with RP1 on-board, and not
> > only for Rpi5.
> 
> Maybe depends on CONFIG_MISC_RP1 ?
> CONFIG_MISC_RP1 enables the RP1 PCI driver

Yep, seems the most logical one, thanks.

Regards,
Andrea

> 
> Best regards,
> Hervé

Re: [PATCH v2 08/14] clk: rp1: Add support for clocks provided by RP1

[Stephen Boyd]

Quoting Andrea della Porta (2024-10-23 08:36:06)
> Hi Stephen,
> 
> On 15:08 Wed 09 Oct     , Stephen Boyd wrote:
> > Quoting Andrea della Porta (2024-10-07 05:39:51)
> > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > > index 299bc678ed1b..537019987f0c 100644
> > > --- a/drivers/clk/Kconfig
> > > +++ b/drivers/clk/Kconfig
> > > @@ -88,6 +88,15 @@ config COMMON_CLK_RK808
> > >           These multi-function devices have two fixed-rate oscillators, clocked at 32KHz each.
> > >           Clkout1 is always on, Clkout2 can off by control register.
> > >  
> > > +config COMMON_CLK_RP1
> > > +       tristate "Raspberry Pi RP1-based clock support"
> > > +       depends on PCI || COMPILE_TEST
> > 
> > A better limit would be some ARCH_* config.
> 
> I've avoided ARCH_BCM2835 since the original intention is for this driver
> to work (in the future) also for custom PCI cards with RP1 on-board, and not
> only for Rpi5.

How will that custom PCI card work? It will need this driver to probe?
Is the iomem going to be exposed through some PCI config space?

It's not great to depend on CONFIG_PCI because then the driver is forced
to be =m if PCI ever becomes tristate (unlikely, but still makes for bad
copy/pasta). I understand this line is trying to limit the availability
of the config symbol. Maybe it should simply depend on ARM or ARM64? Or
on nothing at all.

> 
> > > diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c
> > > new file mode 100644
> > > index 000000000000..9016666fb27d
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-rp1.c
> > 
> > > +#include <linux/clk.h>
> > 
> > Preferably this include isn't included.
> 
> This include is currently needed by devm_clk_get_enabled() to retrieve
> the xosc. Since that clock is based on a crystal (so it's fixed and
> always enabled), I'm planning to hardcode it in the driver. This will
> not only get rid of the devm_clk_get_enabled() call (and hence of the
> clk.h include), but it'll also simplify the top devicetree. No promise
> though, I need to check a couple of things first.

A clk provider (clk-provider.h) should ideally not be a clk consumer
(clk.h).

> 
> 
> > > +
> > > +static int rp1_pll_ph_set_rate(struct clk_hw *hw,
> > > +                              unsigned long rate, unsigned long parent_rate)
> > > +{
> > > +       struct rp1_pll_ph *pll_ph = container_of(hw, struct rp1_pll_ph, hw);
> > > +       const struct rp1_pll_ph_data *data = pll_ph->data;
> > > +
> > > +       /* Nothing really to do here! */
> > 
> > Is it read-only? Don't define a set_rate function then and make the rate
> > determination function return the same value all the time.
> 
> Not 100% sure about it, maybe Raspberry Pi colleagues can explain.
> By 'rate determination function' you're referring (in this case) to
> rp1_pll_ph_recalc_rate(), right?

Yes.

> If so, that clock type seems to have
> a fixed divider but teh resulting clock depends on the parent rate, so
> it has to be calculated.

Sure, it has to be calculated, but it will return the rate that causes
no change to the hardware. When that happens, the set_rate() op should
be skipped, and you can see that with clk_divider_ro_ops not having a
set_rate() function pointer.

> 
> > > +static int rp1_clock_determine_rate(struct clk_hw *hw,
> > > +                                   struct clk_rate_request *req)
> > > +{
> > > +       struct clk_hw *parent, *best_parent = NULL;
> > > +       unsigned long best_rate = 0;
> > > +       unsigned long best_prate = 0;
> > > +       unsigned long best_rate_diff = ULONG_MAX;
> > > +       unsigned long prate, calc_rate;
> > > +       size_t i;
> > > +
> > > +       /*
> > > +        * If the NO_REPARENT flag is set, try to use existing parent.
> > > +        */
> > > +       if ((clk_hw_get_flags(hw) & CLK_SET_RATE_NO_REPARENT)) {
> > 
> > Is this flag ever set?
> 
> Not right now, but it will be used as soon as I'll add the video clocks,
> so I thought to leave it be to avoid adding it back in the future.
> For this minimal support is not needed though, so let me know if you
> want it removed.
> 

Ok sure.

> 
> > > +
> > > +       [RP1_CLK_ETH_TSU] = REGISTER_CLK(.name = "clk_eth_tsu",
> > > +                               .parents = {"rp1-xosc"},
> > > +                               .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 * MHz,
> > > +                               .fc0_src = FC_NUM(5, 7),
> > > +                               ),
> > > +
> > > +       [RP1_CLK_SYS] = REGISTER_CLK(.name = "clk_sys",
> > > +                               .parents = {"rp1-xosc", "-", "pll_sys"},
> > 
> > Please use struct clk_parent_data or clk_hw directly. Don't use strings
> > to describe parents.
> 
> Describing parents as as strings allows to directly assign it to struct
> clk_init_data, as in rp1_register_clock():
> 
> const struct rp1_clock_data *clock_data = data;
> struct clk_init_data init = { };
> ...
> init.parent_names = clock_data->parents;
> 
> otherwise we should create an array and populate from clk_parent_data::name,
> which is of course feasible but a bit less compact. Are you sure you want
> to change it?
> 

Do not use strings to describe parents. That's the guiding principle
here. I agree using strings certainly makes it easy to describe things
but that doesn't mean it is acceptable.

> > > +       struct clk *clk_xosc;
> > > +       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);
> > > +
> > > +       clk_xosc = devm_clk_get_enabled(dev, NULL);
> > > +       if (IS_ERR(clk_xosc))
> > > +               return PTR_ERR(clk_xosc);
> > > +
> > > +       clockman->hw_xosc = __clk_get_hw(clk_xosc);
> > 
> > Please use struct clk_parent_data::index instead.
> 
> Sorry, I didn't catch what you mean here. Can you please elaborate?
> 

Don't use __clk_get_hw() at all. Also, don't use clk_get() and friends
in clk provider drivers. Use struct clk_parent_data so that the
framework can do the work for you at the right time.

Re: [PATCH v2 08/14] clk: rp1: Add support for clocks provided by RP1

[Andrea della Porta]

Hi Stephen,

On 14:52 Wed 23 Oct     , Stephen Boyd wrote:
> Quoting Andrea della Porta (2024-10-23 08:36:06)
> > Hi Stephen,
> > 
> > On 15:08 Wed 09 Oct     , Stephen Boyd wrote:
> > > Quoting Andrea della Porta (2024-10-07 05:39:51)
> > > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > > > index 299bc678ed1b..537019987f0c 100644
> > > > --- a/drivers/clk/Kconfig
> > > > +++ b/drivers/clk/Kconfig
> > > > @@ -88,6 +88,15 @@ config COMMON_CLK_RK808
> > > >           These multi-function devices have two fixed-rate oscillators, clocked at 32KHz each.
> > > >           Clkout1 is always on, Clkout2 can off by control register.
> > > >  
> > > > +config COMMON_CLK_RP1
> > > > +       tristate "Raspberry Pi RP1-based clock support"
> > > > +       depends on PCI || COMPILE_TEST
> > > 
> > > A better limit would be some ARCH_* config.
> > 
> > I've avoided ARCH_BCM2835 since the original intention is for this driver
> > to work (in the future) also for custom PCI cards with RP1 on-board, and not
> > only for Rpi5.
> 
> How will that custom PCI card work? It will need this driver to probe?

AFAICT there's no commercially available PCI card slot sporting an RP1 on-board,
but this driver should be used to probe and use that card too.

> Is the iomem going to be exposed through some PCI config space?

Yes, just as leverage in this driver through BAR1.

> 
> It's not great to depend on CONFIG_PCI because then the driver is forced
> to be =m if PCI ever becomes tristate (unlikely, but still makes for bad
> copy/pasta). I understand this line is trying to limit the availability
> of the config symbol. Maybe it should simply depend on ARM or ARM64? Or
> on nothing at all.

I see, Herve proposed CONFIG_MISC_RP1 that is enabled whenever this driver is
selected, and it makes a lot of sense to me.

> 
> > 
> > > > diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c
> > > > new file mode 100644
> > > > index 000000000000..9016666fb27d
> > > > --- /dev/null
> > > > +++ b/drivers/clk/clk-rp1.c
> > > 
> > > > +#include <linux/clk.h>
> > > 
> > > Preferably this include isn't included.
> > 
> > This include is currently needed by devm_clk_get_enabled() to retrieve
> > the xosc. Since that clock is based on a crystal (so it's fixed and
> > always enabled), I'm planning to hardcode it in the driver. This will
> > not only get rid of the devm_clk_get_enabled() call (and hence of the
> > clk.h include), but it'll also simplify the top devicetree. No promise
> > though, I need to check a couple of things first.
> 
> A clk provider (clk-provider.h) should ideally not be a clk consumer
> (clk.h).

Ack.

> 
> > 
> > 
> > > > +
> > > > +static int rp1_pll_ph_set_rate(struct clk_hw *hw,
> > > > +                              unsigned long rate, unsigned long parent_rate)
> > > > +{
> > > > +       struct rp1_pll_ph *pll_ph = container_of(hw, struct rp1_pll_ph, hw);
> > > > +       const struct rp1_pll_ph_data *data = pll_ph->data;
> > > > +
> > > > +       /* Nothing really to do here! */
> > > 
> > > Is it read-only? Don't define a set_rate function then and make the rate
> > > determination function return the same value all the time.
> > 
> > Not 100% sure about it, maybe Raspberry Pi colleagues can explain.
> > By 'rate determination function' you're referring (in this case) to
> > rp1_pll_ph_recalc_rate(), right?
> 
> Yes.
> 
> > If so, that clock type seems to have
> > a fixed divider but teh resulting clock depends on the parent rate, so
> > it has to be calculated.
> 
> Sure, it has to be calculated, but it will return the rate that causes
> no change to the hardware. When that happens, the set_rate() op should
> be skipped, and you can see that with clk_divider_ro_ops not having a
> set_rate() function pointer.

Ack.

> 
> > 
> > > > +static int rp1_clock_determine_rate(struct clk_hw *hw,
> > > > +                                   struct clk_rate_request *req)
> > > > +{
> > > > +       struct clk_hw *parent, *best_parent = NULL;
> > > > +       unsigned long best_rate = 0;
> > > > +       unsigned long best_prate = 0;
> > > > +       unsigned long best_rate_diff = ULONG_MAX;
> > > > +       unsigned long prate, calc_rate;
> > > > +       size_t i;
> > > > +
> > > > +       /*
> > > > +        * If the NO_REPARENT flag is set, try to use existing parent.
> > > > +        */
> > > > +       if ((clk_hw_get_flags(hw) & CLK_SET_RATE_NO_REPARENT)) {
> > > 
> > > Is this flag ever set?
> > 
> > Not right now, but it will be used as soon as I'll add the video clocks,
> > so I thought to leave it be to avoid adding it back in the future.
> > For this minimal support is not needed though, so let me know if you
> > want it removed.
> > 
> 
> Ok sure.
> 
> > 
> > > > +
> > > > +       [RP1_CLK_ETH_TSU] = REGISTER_CLK(.name = "clk_eth_tsu",
> > > > +                               .parents = {"rp1-xosc"},
> > > > +                               .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 * MHz,
> > > > +                               .fc0_src = FC_NUM(5, 7),
> > > > +                               ),
> > > > +
> > > > +       [RP1_CLK_SYS] = REGISTER_CLK(.name = "clk_sys",
> > > > +                               .parents = {"rp1-xosc", "-", "pll_sys"},
> > > 
> > > Please use struct clk_parent_data or clk_hw directly. Don't use strings
> > > to describe parents.
> > 
> > Describing parents as as strings allows to directly assign it to struct
> > clk_init_data, as in rp1_register_clock():
> > 
> > const struct rp1_clock_data *clock_data = data;
> > struct clk_init_data init = { };
> > ...
> > init.parent_names = clock_data->parents;
> > 
> > otherwise we should create an array and populate from clk_parent_data::name,
> > which is of course feasible but a bit less compact. Are you sure you want
> > to change it?
> > 
> 
> Do not use strings to describe parents. That's the guiding principle
> here. I agree using strings certainly makes it easy to describe things
> but that doesn't mean it is acceptable.

Ack.

> 
> > > > +       struct clk *clk_xosc;
> > > > +       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);
> > > > +
> > > > +       clk_xosc = devm_clk_get_enabled(dev, NULL);
> > > > +       if (IS_ERR(clk_xosc))
> > > > +               return PTR_ERR(clk_xosc);
> > > > +
> > > > +       clockman->hw_xosc = __clk_get_hw(clk_xosc);
> > > 
> > > Please use struct clk_parent_data::index instead.
> > 
> > Sorry, I didn't catch what you mean here. Can you please elaborate?
> > 
> 
> Don't use __clk_get_hw() at all. Also, don't use clk_get() and friends
> in clk provider drivers. Use struct clk_parent_data so that the
> framework can do the work for you at the right time.

Ack.

Many thanks,
Andrea

Re: [PATCH v2 08/14] clk: rp1: Add support for clocks provided by RP1

[Andrea della Porta]

Hi Stephen,

On 15:08 Wed 09 Oct     , Stephen Boyd wrote:
> Quoting Andrea della Porta (2024-10-07 05:39:51)
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 299bc678ed1b..537019987f0c 100644

Here's below the kind response from RaspberryPi guys...
...

> > +       clockman_write(clockman, data->pwr_reg, fbdiv_frac ? 0 : PLL_PWR_DSMPD);
> > +       clockman_write(clockman, data->fbdiv_int_reg, fbdiv_int);
> > +       clockman_write(clockman, data->fbdiv_frac_reg, fbdiv_frac);
> > +       spin_unlock(&clockman->regs_lock);
> > +
> > +       /* Check that reference frequency is no greater than VCO / 16. */
> 
> Why is '16' special?

16 is a hardware requirement.
The lowest feedback divisor in the PLL is 16, so the minimum output
frequency is ref_freq * 16.

...

> > +static unsigned long rp1_pll_core_recalc_rate(struct clk_hw *hw,
> > +                                             unsigned long parent_rate)
> > +{
> > +       struct rp1_pll_core *pll_core = container_of(hw, struct rp1_pll_core, hw);
> > +       struct rp1_clockman *clockman = pll_core->clockman;
> > +       const struct rp1_pll_core_data *data = pll_core->data;
> > +       u32 fbdiv_int, fbdiv_frac;
> > +       unsigned long calc_rate;
> > +
> > +       fbdiv_int = clockman_read(clockman, data->fbdiv_int_reg);
> > +       fbdiv_frac = clockman_read(clockman, data->fbdiv_frac_reg);
> > +       calc_rate =
> > +               ((u64)parent_rate * (((u64)fbdiv_int << 24) + fbdiv_frac) + (1 << 23)) >> 24;
> 
> Where does '24' come from? Can you simplify this line somehow? Maybe
> break it up into multiple lines?

The dividers have an 8 bit integer and (optional) 24 bit fractional
part to the divider value.
The two parts are split across two registers (int_reg and frac_reg),
with the value stored in the bottom bits of both.

...

> > +static int rp1_clock_determine_rate(struct clk_hw *hw,
> > +                                   struct clk_rate_request *req)
> > +{
> > +       struct clk_hw *parent, *best_parent = NULL;
> > +       unsigned long best_rate = 0;
> > +       unsigned long best_prate = 0;
> > +       unsigned long best_rate_diff = ULONG_MAX;
> > +       unsigned long prate, calc_rate;
> > +       size_t i;
> > +
> > +       /*
> > +        * If the NO_REPARENT flag is set, try to use existing parent.
> > +        */
> > +       if ((clk_hw_get_flags(hw) & CLK_SET_RATE_NO_REPARENT)) {
> 
> Is this flag ever set?

In future patches more clocks will be added (namely DPI, DSI (x2) and VEC).
All have the CLK_SET_RATE_NO_REPARENT flag set.
As those peripherals are sensitive to the accuracy of the clocks, the intent
is that the driver will have set the parent, and it isn't expected to change.

...

> > +       divider->div.reg = clockman->regs + divider_data->ctrl_reg;
> > +       divider->div.shift = PLL_SEC_DIV_SHIFT;
> > +       divider->div.width = PLL_SEC_DIV_WIDTH;
> > +       divider->div.flags = CLK_DIVIDER_ROUND_CLOSEST;
> > +       divider->div.flags |= CLK_IS_CRITICAL;
> 
> Is everything critical? The usage of this flag and CLK_IGNORE_UNUSED is
> suspicious and likely working around some problems elsewhere.

the next patchset revision will drop as many of those CRITICAL flags as possible,
and all of the IGNORE_UNUSED flags. That was legacy code needed on bcm-clk2835
since some clocks were enabled by the firmware, and therefore disabling them
had the potential for locking the firmware up. This does no longer apply to RP1.

...

Many thanks,
Andrea

Re: [PATCH v2 11/14] misc: rp1: RaspberryPi RP1 misc driver

[Dave Stevenson]

Hi Andrea

On Mon, 7 Oct 2024 at 14:07, Andrea della Porta <andrea.porta@suse.com> 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      |  24 +++
>  drivers/misc/rp1/Makefile     |   5 +
>  drivers/misc/rp1/rp1-pci.dtso |   8 +
>  drivers/misc/rp1/rp1_pci.c    | 365 ++++++++++++++++++++++++++++++++++
>  drivers/misc/rp1/rp1_pci.h    |  14 ++
>  drivers/pci/quirks.c          |   1 +
>  include/linux/pci_ids.h       |   3 +
>  10 files changed, 423 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..29b1fc2fc4af
> --- /dev/null
> +++ b/drivers/misc/rp1/Kconfig
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# RaspberryPi RP1 misc device
> +#
> +
> +config MISC_RP1
> +        tristate "RaspberryPi RP1 PCIe support"
> +        depends on PCI && PCI_QUIRKS
> +        select OF
> +        select OF_IRQ
> +        select OF_OVERLAY
> +        select IRQ_DOMAIN
> +        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..70384f5a7d7d
> --- /dev/null
> +++ b/drivers/misc/rp1/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_MISC_RP1)         += rp1-pci.o
> +rp1-pci-objs                   := rp1_pci.o rp1-pci.dtbo.o
> +
> +DTC_FLAGS_rp1-pci += -@
> 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..a1f7bf1804c0
> --- /dev/null
> +++ b/drivers/misc/rp1/rp1_pci.c
> @@ -0,0 +1,365 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018-24 Raspberry Pi Ltd.
> + * All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/of_platform.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +
> +#include "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
> +
> +struct rp1_dev {
> +       struct pci_dev *pdev;
> +       struct device *dev;
> +       struct clk *sys_clk;
> +       struct irq_domain *domain;
> +       struct irq_data *pcie_irqds[64];
> +       void __iomem *bar1;
> +       int ovcs_id;
> +       bool level_triggered_irq[RP1_INT_END];
> +};
> +
> +static void dump_bar(struct pci_dev *pdev, unsigned int bar)
> +{
> +       dev_info(&pdev->dev,
> +                "bar%d %pR\n",
> +                bar,
> +                pci_resource_n(pdev, bar));
> +}
> +
> +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",
> +       .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;
> +
> +       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 int rp1_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *rp1_node;
> +       struct reset_control *reset;
> +       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;
> +
> +       reset = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> +       if (IS_ERR(reset))
> +               return PTR_ERR(reset);
> +       reset_control_reset(reset);
> +
> +       dump_bar(pdev, 0);
> +       dump_bar(pdev, 1);
> +
> +       if (pci_resource_len(pdev, 1) <= 0x10000) {
> +               dev_err(&pdev->dev,
> +                       "Not initialised - is the firmware running?\n");
> +               return -EINVAL;
> +       }
> +
> +       err = pcim_enable_device(pdev);
> +       if (err < 0) {
> +               dev_err(&pdev->dev, "Enabling PCI device has failed: %d",
> +                       err);
> +               return err;
> +       }
> +
> +       rp1->bar1 = pcim_iomap(pdev, 1, 0);
> +       if (!rp1->bar1) {
> +               dev_err(&pdev->dev, "Cannot map PCI BAR\n");
> +               return -EIO;
> +       }
> +
> +       u32 dtbo_size = __dtbo_rp1_pci_end - __dtbo_rp1_pci_begin;
> +       void *dtbo_start = __dtbo_rp1_pci_begin;
> +
> +       err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id, rp1_node);
> +       if (err)
> +               return err;
> +
> +       pci_set_master(pdev);
> +
> +       err = pci_alloc_irq_vectors(pdev, RP1_INT_END, RP1_INT_END,
> +                                   PCI_IRQ_MSIX);
> +       if (err != RP1_INT_END) {
> +               dev_err(&pdev->dev, "pci_alloc_irq_vectors failed - %d\n", err);
> +               goto err_unload_overlay;
> +       }
> +
> +       pci_set_drvdata(pdev, rp1);
> +       rp1->domain = irq_domain_add_linear(of_find_node_by_name(NULL, "pci-ep-bus"), RP1_INT_END,
> +                                           &rp1_domain_ops, rp1);
> +
> +       for (i = 0; i < RP1_INT_END; i++) {
> +               int irq = irq_create_mapping(rp1->domain, i);
> +
> +               if (irq < 0) {
> +                       dev_err(&pdev->dev, "failed to create irq mapping\n");
> +                       err = irq;
> +                       goto err_unload_overlay;
> +               }
> +               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_platform_default_populate(rp1_node, NULL, dev);
> +       if (err)
> +               goto err_unload_overlay;
> +
> +       return 0;
> +
> +err_unload_overlay:
> +       of_overlay_remove(&rp1->ovcs_id);
> +
> +       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);
> +
> +       clk_unregister(rp1->sys_clk);
> +}
> +
> +static const struct pci_device_id dev_id_table[] = {
> +       { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0), },
> +       { 0, }
> +};

You need a

MODULE_DEVICE_TABLE(pci, dev_id_table);

here in order to load the module for the cases where it isn't
built-in. Otherwise you have to manually modprobe the module.

Cheers.
  Dave

> +
> +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[];
> +
> +#endif
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index dccb60c1d9cc..41e77d94ff73 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6266,6 +6266,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa76e, dpc_log_size);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0, of_pci_make_dev_node);
>
>  /*
>   * Devices known to require a longer delay before first config space access
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 4cf6aaed5f35..c7e6cd10ac52 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2611,6 +2611,9 @@
>  #define PCI_VENDOR_ID_TEKRAM           0x1de1
>  #define PCI_DEVICE_ID_TEKRAM_DC290     0xdc29
>
> +#define PCI_VENDOR_ID_RPI              0x1de4
> +#define PCI_DEVICE_ID_RPI_RP1_C0       0x0001
> +
>  #define PCI_VENDOR_ID_ALIBABA          0x1ded
>
>  #define PCI_VENDOR_ID_CXL              0x1e98
> --
> 2.35.3
>
>

Re: [PATCH v2 11/14] misc: rp1: RaspberryPi RP1 misc driver

[Andrea della Porta]

Hi Dave,

On 16:21 Thu 24 Oct     , Dave Stevenson wrote:
> Hi Andrea
> 
> On Mon, 7 Oct 2024 at 14:07, Andrea della Porta <andrea.porta@suse.com> wrote:
> >

...

> > +static const struct pci_device_id dev_id_table[] = {
> > +       { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0), },
> > +       { 0, }
> > +};
> 
> You need a
> 
> MODULE_DEVICE_TABLE(pci, dev_id_table);
> 
> here in order to load the module for the cases where it isn't
> built-in. Otherwise you have to manually modprobe the module.

Sure, thanks for the heads up!

Regards,
Andrea

> 
> Cheers.
>   Dave

...

Re: [PATCH v2 11/14] misc: rp1: RaspberryPi RP1 misc driver

[Andrea della Porta]

Hi Herve,

On 17:41 Mon 07 Oct     , Herve Codina wrote:
> Hi Andrea,
> 
> Nice to see that other PCI drivers will use the DT overlay over PCI feature!
> 
> On Mon,  7 Oct 2024 14:39:54 +0200
> Andrea della Porta <andrea.porta@suse.com> 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>
> 
> ...
> > --- /dev/null
> > +++ b/drivers/misc/rp1/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +obj-$(CONFIG_MISC_RP1)		+= rp1-pci.o
> > +rp1-pci-objs			:= rp1_pci.o rp1-pci.dtbo.o
> > +
> > +DTC_FLAGS_rp1-pci += -@
> Why do you need to add the symbol table (-@ option) in your dtbo ?

This was needed for loading the dtbo for gpio-line-names over the dtbo for
RP1, but now that I've dropped that patch it is not needed anymore, so I'll
remove that.

> 
> ...
> > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
> > new file mode 100644
> > index 000000000000..a1f7bf1804c0
> > --- /dev/null
> > +++ b/drivers/misc/rp1/rp1_pci.c
> > @@ -0,0 +1,365 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018-24 Raspberry Pi Ltd.
> > + * All rights reserved.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +
> > +#include "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
> > +
> > +struct rp1_dev {
> > +	struct pci_dev *pdev;
> > +	struct device *dev;
> > +	struct clk *sys_clk;
> > +	struct irq_domain *domain;
> > +	struct irq_data *pcie_irqds[64];
> > +	void __iomem *bar1;
> > +	int ovcs_id;
> > +	bool level_triggered_irq[RP1_INT_END];
> > +};
> > +
> > +static void dump_bar(struct pci_dev *pdev, unsigned int bar)
> > +{
> > +	dev_info(&pdev->dev,
> > +		 "bar%d %pR\n",
> > +		 bar,
> > +		 pci_resource_n(pdev, bar));
> > +}
> 
> Is this dump_bar() really needed ?

Nope. Removed.

> 
> ...
> > +static int rp1_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *rp1_node;
> > +	struct reset_control *reset;
> > +	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;
> > +
> > +	reset = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> > +	if (IS_ERR(reset))
> > +		return PTR_ERR(reset);
> > +	reset_control_reset(reset);
> 
> This device is a PCI device.
> Seems strange to get the reset control line for a PCI device.

And it isn't declared in teh node either, so right now seems a no-op.
Removed.

> 
> > +
> > +	dump_bar(pdev, 0);
> > +	dump_bar(pdev, 1);
> No sure those 2 dump_bar() calls are really needed.

Removed as well.

> > +
> > +	if (pci_resource_len(pdev, 1) <= 0x10000) {
> > +		dev_err(&pdev->dev,
> > +			"Not initialised - is the firmware running?\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = pcim_enable_device(pdev);
> > +	if (err < 0) {
> > +		dev_err(&pdev->dev, "Enabling PCI device has failed: %d",
> > +			err);
> > +		return err;
> > +	}
> > +
> > +	rp1->bar1 = pcim_iomap(pdev, 1, 0);
> > +	if (!rp1->bar1) {
> > +		dev_err(&pdev->dev, "Cannot map PCI BAR\n");
> > +		return -EIO;
> > +	}
> > +
> > +	u32 dtbo_size = __dtbo_rp1_pci_end - __dtbo_rp1_pci_begin;
> > +	void *dtbo_start = __dtbo_rp1_pci_begin;
> 
> Those declarations should be move at the beginning of the function.

Ack.

> 
> > +
> > +	err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id, rp1_node);
> > +	if (err)
> > +		return err;
> > +
> 
> Maybe applying the overlay should be done after the interrupt controller
> is ready.

Right.

> 
> > +	pci_set_master(pdev);
> > +
> > +	err = pci_alloc_irq_vectors(pdev, RP1_INT_END, RP1_INT_END,
> > +				    PCI_IRQ_MSIX);
> > +	if (err != RP1_INT_END) {
> > +		dev_err(&pdev->dev, "pci_alloc_irq_vectors failed - %d\n", err);
> > +		goto err_unload_overlay;
> > +	}
> > +
> > +	pci_set_drvdata(pdev, rp1);
> > +	rp1->domain = irq_domain_add_linear(of_find_node_by_name(NULL, "pci-ep-bus"), RP1_INT_END,
> > +					    &rp1_domain_ops, rp1);
> > +
> > +	for (i = 0; i < RP1_INT_END; i++) {
> > +		int irq = irq_create_mapping(rp1->domain, i);
> > +
> > +		if (irq < 0) {
> > +			dev_err(&pdev->dev, "failed to create irq mapping\n");
> > +			err = irq;
> > +			goto err_unload_overlay;
> > +		}
> > +		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_platform_default_populate(rp1_node, NULL, dev);
> > +	if (err)
> > +		goto err_unload_overlay;
> > +
> > +	return 0;
> > +
> > +err_unload_overlay:
> > +	of_overlay_remove(&rp1->ovcs_id);
> > +
> > +	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);
> > +
> > +	clk_unregister(rp1->sys_clk);
> 
> Unless I missed something, rp1->sys_clk is never set in probe().
> If this is correct, clk_unregister() should be removed and also
> the related clk header files.

Ack.

> 
> The interrupt controller created at probe() should be destroyed at remove().

Ack.

> 
> > +}
> > +
> 
> ...
> > 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[];
> > +
> > +#endif
> 
> These two symbols are only used by the rp1_pci.c file.
> Not sure that the rp1_pci.h is needed.
> Simply declare the symbols in the rp1_pci.c file.

I agree with you, and I did exactly as you suggest in the very first submission
of this patch, but I received a couple of comments against it, so I've created
the new header file.

Many thanks,
Andrea


> 
> 
> Best regards,
> Hervé

Re: [PATCH v2 09/14] pinctrl: rp1: Implement RaspberryPi RP1 gpio support

[Andrea della Porta]

Hi Linux,

On 11:03 Fri 11 Oct     , Linus Walleij wrote:
> On Mon, Oct 7, 2024 at 2:39 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 is a nice driver and I find no issues with it, what causes
> an issue is gpiochip_set_names() as pointed out by Bartosz.
> If you can live without the names you can remove that part for
> now and we can merge the driver, then you can add the names
> later when we sorted out how to share that function.

Sure, I'll drop the gpio-line-names patch for now.

Many thanks,
Andrea

> 
> Yours,
> Linus Walleij

Re: [PATCH v2 09/14] pinctrl: rp1: Implement RaspberryPi RP1 gpio support

[Andrea della Porta]

Hi Stefan,

On 12:08 Fri 11 Oct     , Stefan Wahren wrote:
> Am 11.10.24 um 11:03 schrieb Linus Walleij:
> > On Mon, Oct 7, 2024 at 2:39 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 is a nice driver and I find no issues with it, what causes
> > an issue is gpiochip_set_names() as pointed out by Bartosz.
> > If you can live without the names you can remove that part for
> > now and we can merge the driver, then you can add the names
> > later when we sorted out how to share that function.
> I raised the concerns about missing gpio line names in the first version
> of patch, without knowing the real efforts.
> 
> So I'm fine with Linus' suggestion, because I don't want to delay the
> upstreaming effort unnecessarily.

Perfect, thanks.

Regards,
Andrea

> 
> Regards
> > 
> > Yours,
> > Linus Walleij
> 

Re: [PATCH v2 10/14] arm64: dts: rp1: Add support for RaspberryPi's RP1 device

[Andrea della Porta]

Hi Herve,

On 16:57 Mon 07 Oct     , Herve Codina wrote:
> Hi Andrea,
> 
> On Mon,  7 Oct 2024 14:39:53 +0200
> Andrea della Porta <andrea.porta@suse.com> 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>
> > ---
> 
> ...
> > +/ {
> > +	fragment@0 {
> > +		target-path="";
> > +		__overlay__ {
> > +			compatible = "pci1de4,1";
> 
> The compatible is not needed here. Indeed, it will be added by the PCI core
> when it scans the bus and adds the missing nodes.
>   https://elixir.bootlin.com/linux/v6.12-rc2/source/drivers/pci/of_property.c#L383

Sure, but I've added it so that the dts could be validated.

> 
> > +			#address-cells = <3>;
> > +			#size-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>;
> > +				interrupt-controller;
> > +				interrupt-parent = <&pci_ep_bus>;
> > +				#interrupt-cells = <2>;
> 
> Not sure this node should be an interrupt controller.
> The interrupt controller is the PCI device itself (i.e.the node
> where the overlay is applied).

Right.

Many thanks,
Andrea

> 
> Best regards,
> Hervé
> 

Re: [PATCH v2 14/14] arm64: defconfig: Enable RP1 misc/clock/gpio drivers

[Andrea della Porta]

Hi Krzysztof,

On 08:32 Tue 08 Oct     , Krzysztof Kozlowski wrote:
> On Mon, Oct 07, 2024 at 02:39:57PM +0200, Andrea della Porta wrote:
> > Select the RP1 drivers needed to operate the PCI endpoint containing
> > several peripherals such as Ethernet and USB Controller. This chip is
> > present on RaspberryPi 5.
> > 
> > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> > ---
> >  arch/arm64/configs/defconfig | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> > index 5fdbfea7a5b2..5fcd9ae0d373 100644
> > --- a/arch/arm64/configs/defconfig
> > +++ b/arch/arm64/configs/defconfig
> > @@ -609,6 +609,7 @@ CONFIG_PINCTRL_QCM2290=y
> >  CONFIG_PINCTRL_QCS404=y
> >  CONFIG_PINCTRL_QDF2XXX=y
> >  CONFIG_PINCTRL_QDU1000=y
> > +CONFIG_PINCTRL_RP1=y
> 
> Module, that's not a SoC component.

This driver was born as a builtin only one, indeed it is bool and not
tristate in Kconfig and is registered through builtin_platform_driver().
Hence I've set it =y in defconfig. A quick check transforming it to be
dynamically loaded as a module confirm that something is not working, and
I agree with you that, in theory, this should also be a loadable module.
I need some investigation on that though...

Many thanks,
Andrea

> 
> >  CONFIG_PINCTRL_SA8775P=y
> >  CONFIG_PINCTRL_SC7180=y
> 
> Best regards,
> Krzysztof
>