| Message ID | 914978925d34cfb5bee10fe92603f98763af48b0.1730123575.git.andrea.porta@suse.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add support for RaspberryPi RP1 PCI device using a DT overlay | expand |
On 28/10/2024 15:07, Andrea della Porta wrote: > Add device tree bindings for the clock generator found in RP1 multi > function device, and relative entries in MAINTAINERS file. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- Please do not send new versions of patchsets while discussion is still going. I responded to you on v2, pretty next day I think. It took you two weeks to respond to my comments. That was on 21st of October, a week ago, so I still have some time to respond to you. Best regards, Krzysztof
On Mon, Oct 28, 2024 at 03:07:18PM +0100, Andrea della Porta wrote: > Add device tree bindings for the clock generator found in RP1 multi > function device, and relative entries in MAINTAINERS file. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > .../clock/raspberrypi,rp1-clocks.yaml | 62 +++++++++++++++++++ > MAINTAINERS | 6 ++ > .../clock/raspberrypi,rp1-clocks.h | 61 ++++++++++++++++++ > 3 files changed, 129 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > create mode 100644 include/dt-bindings/clock/raspberrypi,rp1-clocks.h > > diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > new file mode 100644 > index 000000000000..a123dd619f8e > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > @@ -0,0 +1,62 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/raspberrypi,rp1-clocks.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: RaspberryPi RP1 clock generator > + > +maintainers: > + - Andrea della Porta <andrea.porta@suse.com> > + > +description: | > + The RP1 contains a clock generator designed as three PLLs (CORE, AUDIO, > + VIDEO), and each PLL output can be programmed though dividers to generate > + the clocks to drive the sub-peripherals embedded inside the chipset. > + > + Link to datasheet: > + https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > + > +properties: > + compatible: > + const: raspberrypi,rp1-clocks > + > + reg: > + maxItems: 1 > + > + '#clock-cells': > + description: > + The index in the assigned-clocks is mapped to the output clock as per > + definitions in include/dt-bindings/clock/raspberrypi,rp1-clocks.h. You still describe how current driver matches assigned-clocks to your output clocks. That's not the property of clock-cells and that's not how assigned-clocks work. There are no assigned clocks in your DTS, so this is really irrelevant (or not correct, choose). > + const: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + const: xosc What is the purpose of clock-names if you do not use it? Drop. > + > +required: > + - compatible > + - reg > + - '#clock-cells' > + - clocks > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/raspberrypi,rp1-clocks.h> > + > + rp1 { > + #address-cells = <2>; > + #size-cells = <2>; > + > + clocks@c040018000 { > + compatible = "raspberrypi,rp1-clocks"; > + reg = <0xc0 0x40018000 0x0 0x10038>; > + #clock-cells = <1>; > + clocks = <&clk_rp1_xosc>; > + clock-names = "xosc"; Only one space after '='. Best regards, Krzysztof
Hi Krzysztof, On 08:23 Tue 29 Oct , Krzysztof Kozlowski wrote: > On Mon, Oct 28, 2024 at 03:07:18PM +0100, Andrea della Porta wrote: > > Add device tree bindings for the clock generator found in RP1 multi > > function device, and relative entries in MAINTAINERS file. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > --- > > .../clock/raspberrypi,rp1-clocks.yaml | 62 +++++++++++++++++++ > > MAINTAINERS | 6 ++ > > .../clock/raspberrypi,rp1-clocks.h | 61 ++++++++++++++++++ > > 3 files changed, 129 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > > create mode 100644 include/dt-bindings/clock/raspberrypi,rp1-clocks.h > > > > diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > > new file mode 100644 > > index 000000000000..a123dd619f8e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > > @@ -0,0 +1,62 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/clock/raspberrypi,rp1-clocks.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: RaspberryPi RP1 clock generator > > + > > +maintainers: > > + - Andrea della Porta <andrea.porta@suse.com> > > + > > +description: | > > + The RP1 contains a clock generator designed as three PLLs (CORE, AUDIO, > > + VIDEO), and each PLL output can be programmed though dividers to generate > > + the clocks to drive the sub-peripherals embedded inside the chipset. > > + > > + Link to datasheet: > > + https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > > + > > +properties: > > + compatible: > > + const: raspberrypi,rp1-clocks > > + > > + reg: > > + maxItems: 1 > > + > > + '#clock-cells': > > + description: > > + The index in the assigned-clocks is mapped to the output clock as per > > + definitions in include/dt-bindings/clock/raspberrypi,rp1-clocks.h. > > You still describe how current driver matches assigned-clocks to your > output clocks. That's not the property of clock-cells and that's not how > assigned-clocks work. This description is taken by another upstream binding, please see Documentation/devicetree/bindings/clock/renesas,5p35023.yaml Its purpose is to let the user know how clock-cell number specified in assigned-clocks is mapped to the clock provided by this generator. Since some of these clocks are shared among peripherals, their frequency cannot be set by consumers, so it's the provider itself (i.e. the clock device described with this binding) that should take care of them. The renesas example has assigned-clocks specified though, please see below. > > There are no assigned clocks in your DTS, so this is really irrelevant > (or not correct, choose). In the first revision of this patchset (please see [1] and following messages) I had the assigned-clocks setup in the example while trying to explain their purpose, but Conor said those didn't seem to be relevant, hence I dropped them. Maybe I had to be more incisive on that. So, I'd be inclined to retain the description as it is and reintroduce some assigned-clocks in the example as in the renesas one, would it be ok for you? > > > > + const: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + const: xosc > > What is the purpose of clock-names if you do not use it? Drop. Ack. > > > + > > +required: > > + - compatible > > + - reg > > + - '#clock-cells' > > + - clocks > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/raspberrypi,rp1-clocks.h> > > + > > + rp1 { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + clocks@c040018000 { > > + compatible = "raspberrypi,rp1-clocks"; > > + reg = <0xc0 0x40018000 0x0 0x10038>; > > + #clock-cells = <1>; > > + clocks = <&clk_rp1_xosc>; > > + clock-names = "xosc"; > > Only one space after '='. I will drop the name since the driver will not refer to it via clk_get() but will use clk_parent_data::index. Many thanks, Andrea [1] - https://lore.kernel.org/all/20240822-refutable-railroad-a3f111ab1e3f@spud/ > > Best regards, > Krzysztof >
Hi Krzysztof, On 10:16 Thu 31 Oct , Andrea della Porta wrote: > Hi Krzysztof, > > On 08:23 Tue 29 Oct , Krzysztof Kozlowski wrote: > > On Mon, Oct 28, 2024 at 03:07:18PM +0100, Andrea della Porta wrote: > > > Add device tree bindings for the clock generator found in RP1 multi > > > function device, and relative entries in MAINTAINERS file. > > > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > > --- > > > .../clock/raspberrypi,rp1-clocks.yaml | 62 +++++++++++++++++++ > > > MAINTAINERS | 6 ++ > > > .../clock/raspberrypi,rp1-clocks.h | 61 ++++++++++++++++++ > > > 3 files changed, 129 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > > > create mode 100644 include/dt-bindings/clock/raspberrypi,rp1-clocks.h > > > > > > diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > > > new file mode 100644 > > > index 000000000000..a123dd619f8e > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > > > @@ -0,0 +1,62 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/clock/raspberrypi,rp1-clocks.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: RaspberryPi RP1 clock generator > > > + > > > +maintainers: > > > + - Andrea della Porta <andrea.porta@suse.com> > > > + > > > +description: | > > > + The RP1 contains a clock generator designed as three PLLs (CORE, AUDIO, > > > + VIDEO), and each PLL output can be programmed though dividers to generate > > > + the clocks to drive the sub-peripherals embedded inside the chipset. > > > + > > > + Link to datasheet: > > > + https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > > > + > > > +properties: > > > + compatible: > > > + const: raspberrypi,rp1-clocks > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + '#clock-cells': > > > + description: > > > + The index in the assigned-clocks is mapped to the output clock as per > > > + definitions in include/dt-bindings/clock/raspberrypi,rp1-clocks.h. > > > > You still describe how current driver matches assigned-clocks to your > > output clocks. That's not the property of clock-cells and that's not how > > assigned-clocks work. > > This description is taken by another upstream binding, please see > Documentation/devicetree/bindings/clock/renesas,5p35023.yaml > > Its purpose is to let the user know how clock-cell number specified > in assigned-clocks is mapped to the clock provided by this generator. > Since some of these clocks are shared among peripherals, their frequency > cannot be set by consumers, so it's the provider itself (i.e. the clock > device described with this binding) that should take care of them. > The renesas example has assigned-clocks specified though, please see below. > > > > > There are no assigned clocks in your DTS, so this is really irrelevant > > (or not correct, choose). > > In the first revision of this patchset (please see [1] and following messages) > I had the assigned-clocks setup in the example while trying to explain their > purpose, but Conor said those didn't seem to be relevant, hence I dropped them. > Maybe I had to be more incisive on that. > So, I'd be inclined to retain the description as it is and reintroduce some > assigned-clocks in the example as in the renesas one, would it be ok for you? Since I'm on the verge of producing a new patchset revision, may I kindly ask some comments on this? Is it ok for you? Many thanks, Andrea > > > > > > > > + const: 1 > > > + > > > + clocks: > > > + maxItems: 1 > > > + > > > + clock-names: > > > + const: xosc > > > > What is the purpose of clock-names if you do not use it? Drop. > > Ack. > > > > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - '#clock-cells' > > > + - clocks > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/clock/raspberrypi,rp1-clocks.h> > > > + > > > + rp1 { > > > + #address-cells = <2>; > > > + #size-cells = <2>; > > > + > > > + clocks@c040018000 { > > > + compatible = "raspberrypi,rp1-clocks"; > > > + reg = <0xc0 0x40018000 0x0 0x10038>; > > > + #clock-cells = <1>; > > > + clocks = <&clk_rp1_xosc>; > > > + clock-names = "xosc"; > > > > Only one space after '='. > > I will drop the name since the driver will not refer to it via clk_get() > but will use clk_parent_data::index. > > Many thanks, > Andrea > > [1] - https://lore.kernel.org/all/20240822-refutable-railroad-a3f111ab1e3f@spud/ > > > > > Best regards, > > Krzysztof > >
Quoting Andrea della Porta (2024-11-15 03:31:45) > On 10:16 Thu 31 Oct , Andrea della Porta wrote: > > On 08:23 Tue 29 Oct , Krzysztof Kozlowski wrote: > > > > + '#clock-cells': > > > > + description: > > > > + The index in the assigned-clocks is mapped to the output clock as per > > > > + definitions in include/dt-bindings/clock/raspberrypi,rp1-clocks.h. > > > > > > You still describe how current driver matches assigned-clocks to your > > > output clocks. That's not the property of clock-cells and that's not how > > > assigned-clocks work. > > > > This description is taken by another upstream binding, please see > > Documentation/devicetree/bindings/clock/renesas,5p35023.yaml > > > > Its purpose is to let the user know how clock-cell number specified > > in assigned-clocks is mapped to the clock provided by this generator. > > Since some of these clocks are shared among peripherals, their frequency > > cannot be set by consumers, so it's the provider itself (i.e. the clock > > device described with this binding) that should take care of them. > > The renesas example has assigned-clocks specified though, please see below. > > > > > > > > There are no assigned clocks in your DTS, so this is really irrelevant > > > (or not correct, choose). > > > > In the first revision of this patchset (please see [1] and following messages) > > I had the assigned-clocks setup in the example while trying to explain their > > purpose, but Conor said those didn't seem to be relevant, hence I dropped them. > > Maybe I had to be more incisive on that. > > So, I'd be inclined to retain the description as it is and reintroduce some > > assigned-clocks in the example as in the renesas one, would it be ok for you? > > Since I'm on the verge of producing a new patchset revision, may I kindly ask > some comments on this? Is it ok for you? > Everyone knows how #clock-cells works. It shouldn't need a description about how it works. It should just point at the header file with the numbers if anything.
diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml new file mode 100644 index 000000000000..a123dd619f8e --- /dev/null +++ b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml @@ -0,0 +1,62 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/raspberrypi,rp1-clocks.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: RaspberryPi RP1 clock generator + +maintainers: + - Andrea della Porta <andrea.porta@suse.com> + +description: | + The RP1 contains a clock generator designed as three PLLs (CORE, AUDIO, + VIDEO), and each PLL output can be programmed though dividers to generate + the clocks to drive the sub-peripherals embedded inside the chipset. + + Link to datasheet: + https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf + +properties: + compatible: + const: raspberrypi,rp1-clocks + + reg: + maxItems: 1 + + '#clock-cells': + description: + The index in the assigned-clocks is mapped to the output clock as per + definitions in include/dt-bindings/clock/raspberrypi,rp1-clocks.h. + const: 1 + + clocks: + maxItems: 1 + + clock-names: + const: xosc + +required: + - compatible + - reg + - '#clock-cells' + - clocks + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/raspberrypi,rp1-clocks.h> + + rp1 { + #address-cells = <2>; + #size-cells = <2>; + + clocks@c040018000 { + compatible = "raspberrypi,rp1-clocks"; + reg = <0xc0 0x40018000 0x0 0x10038>; + #clock-cells = <1>; + clocks = <&clk_rp1_xosc>; + clock-names = "xosc"; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index c27f3190737f..75a66e3e34c9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19380,6 +19380,12 @@ F: Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml F: drivers/media/platform/raspberrypi/pisp_be/ F: include/uapi/linux/media/raspberrypi/ +RASPBERRY PI RP1 PCI DRIVER +M: Andrea della Porta <andrea.porta@suse.com> +S: Maintained +F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml +F: include/dt-bindings/clock/rp1.h + RC-CORE / LIRC FRAMEWORK M: Sean Young <sean@mess.org> L: linux-media@vger.kernel.org diff --git a/include/dt-bindings/clock/raspberrypi,rp1-clocks.h b/include/dt-bindings/clock/raspberrypi,rp1-clocks.h new file mode 100644 index 000000000000..248efb895f35 --- /dev/null +++ b/include/dt-bindings/clock/raspberrypi,rp1-clocks.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ +/* + * Copyright (C) 2021 Raspberry Pi Ltd. + */ + +#ifndef __DT_BINDINGS_CLOCK_RASPBERRYPI_RP1 +#define __DT_BINDINGS_CLOCK_RASPBERRYPI_RP1 + +#define RP1_PLL_SYS_CORE 0 +#define RP1_PLL_AUDIO_CORE 1 +#define RP1_PLL_VIDEO_CORE 2 + +#define RP1_PLL_SYS 3 +#define RP1_PLL_AUDIO 4 +#define RP1_PLL_VIDEO 5 + +#define RP1_PLL_SYS_PRI_PH 6 +#define RP1_PLL_SYS_SEC_PH 7 +#define RP1_PLL_AUDIO_PRI_PH 8 + +#define RP1_PLL_SYS_SEC 9 +#define RP1_PLL_AUDIO_SEC 10 +#define RP1_PLL_VIDEO_SEC 11 + +#define RP1_CLK_SYS 12 +#define RP1_CLK_SLOW_SYS 13 +#define RP1_CLK_DMA 14 +#define RP1_CLK_UART 15 +#define RP1_CLK_ETH 16 +#define RP1_CLK_PWM0 17 +#define RP1_CLK_PWM1 18 +#define RP1_CLK_AUDIO_IN 19 +#define RP1_CLK_AUDIO_OUT 20 +#define RP1_CLK_I2S 21 +#define RP1_CLK_MIPI0_CFG 22 +#define RP1_CLK_MIPI1_CFG 23 +#define RP1_CLK_PCIE_AUX 24 +#define RP1_CLK_USBH0_MICROFRAME 25 +#define RP1_CLK_USBH1_MICROFRAME 26 +#define RP1_CLK_USBH0_SUSPEND 27 +#define RP1_CLK_USBH1_SUSPEND 28 +#define RP1_CLK_ETH_TSU 29 +#define RP1_CLK_ADC 30 +#define RP1_CLK_SDIO_TIMER 31 +#define RP1_CLK_SDIO_ALT_SRC 32 +#define RP1_CLK_GP0 33 +#define RP1_CLK_GP1 34 +#define RP1_CLK_GP2 35 +#define RP1_CLK_GP3 36 +#define RP1_CLK_GP4 37 +#define RP1_CLK_GP5 38 +#define RP1_CLK_VEC 39 +#define RP1_CLK_DPI 40 +#define RP1_CLK_MIPI0_DPI 41 +#define RP1_CLK_MIPI1_DPI 42 + +/* Extra PLL output channels - RP1B0 only */ +#define RP1_PLL_VIDEO_PRI_PH 43 +#define RP1_PLL_AUDIO_TERN 44 + +#endif
Add device tree bindings for the clock generator found in RP1 multi function device, and relative entries in MAINTAINERS file. Signed-off-by: Andrea della Porta <andrea.porta@suse.com> --- .../clock/raspberrypi,rp1-clocks.yaml | 62 +++++++++++++++++++ MAINTAINERS | 6 ++ .../clock/raspberrypi,rp1-clocks.h | 61 ++++++++++++++++++ 3 files changed, 129 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml create mode 100644 include/dt-bindings/clock/raspberrypi,rp1-clocks.h