| Message ID | 20240731062814.215833-1-iivanov@suse.de |
|---|---|
| Headers | show |
| Series | Add pin control driver for BCM2712 SoC | expand |
On 7/30/24 23:28, Ivan T. Ivanov wrote: > On RPi5 device Bluetooth chips is connected to UARTA > port. Add Bluetooth chips and related pin definitions. > With this and firmware already provided by distributions, > at least on openSUSE Tumbleweed, this is sufficient to make > Bluetooth operational on RPi5 \o/. > > Signed-off-by: Ivan T. Ivanov <iivanov@suse.de> > --- [snip] > sdio2_30_pins: sdio2-30-pins { > pin-clk { > function = "sd2"; > @@ -156,3 +184,20 @@ pins-dat { > }; > }; > }; > + > +/* uarta communicates with the BT module */ > +&uarta { > + uart-has-rtscts; > + auto-flow-control; > + status = "okay"; > + clock-frequency = <96000000>; Would not the "clock-frequency" belong to the .dtsi node instead? > + pinctrl-0 = <&uarta_24_pins &bt_shutdown_pins>; > + pinctrl-names = "default"; > + > + bluetooth: bluetooth { > + compatible = "brcm,bcm43438-bt"; > + max-speed = <3000000>; > + shutdown-gpios = <&gio 29 GPIO_ACTIVE_HIGH>; > + local-bd-address = [ 00 00 00 00 00 00 ]; > + }; > +}; > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > index 3c0663dc6712..e972f94d6828 100644 > --- a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > +++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > @@ -305,6 +305,17 @@ gio: gpio@7d508500 { > brcm,gpio-direct; > }; > > + uarta: serial@7d50c000 { > + compatible = "brcm,bcm7271-uart"; > + reg = <0x7d50c000 0x20>; > + reg-names = "uart"; > + reg-shift = <2>; > + reg-io-width = <4>; > + interrupts = <GIC_SPI 276 IRQ_TYPE_LEVEL_HIGH>; > + skip-init; Also an undocumented property upstream, what does it do? Is this to set UPF_SKIP_TEST?
On 07-31 15:11, Florian Fainelli wrote: > > + > > + gio: gpio@7d508500 { > > + compatible = "brcm,brcmstb-gpio"; > > + reg = <0x7d508500 0x40>; > > + interrupt-parent = <&main_irq>; > > + interrupts = <0>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + interrupt-controller; > > + #interrupt-cells = <2>; > > + brcm,gpio-bank-widths = <32 22>; > > + brcm,gpio-direct; > > Undocumented and unsupported property upstream, what does it do? Other than > that, LGTM. Doh. Something used for banging GPIO's from user-space via "/dev/gpiomem". I will remove it in next patch version. Thanks, Ivan [1] "... gpio: mmio: Add DIRECT mode for shared access The generic MMIO GPIO library uses shadow registers for efficiency, but this breaks attempts by raspi-gpio to change other GPIOs in the same bank. Add a DIRECT mode that makes fewer assumptions about the existing register contents, but note that genuinely simultaneous accesses are likely to lose updates. ... "
Hi, On 07-31 15:13, Florian Fainelli wrote: > > + > > +/* uarta communicates with the BT module */ > > +&uarta { > > + uart-has-rtscts; > > + auto-flow-control; > > + status = "okay"; > > + clock-frequency = <96000000>; > > Would not the "clock-frequency" belong to the .dtsi node instead? > Perhaps. > > + pinctrl-0 = <&uarta_24_pins &bt_shutdown_pins>; > > + pinctrl-names = "default"; > > + > > + bluetooth: bluetooth { > > + compatible = "brcm,bcm43438-bt"; > > + max-speed = <3000000>; > > + shutdown-gpios = <&gio 29 GPIO_ACTIVE_HIGH>; > > + local-bd-address = [ 00 00 00 00 00 00 ]; > > + }; > > +}; > > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > > index 3c0663dc6712..e972f94d6828 100644 > > --- a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > > +++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > > @@ -305,6 +305,17 @@ gio: gpio@7d508500 { > > brcm,gpio-direct; > > }; > > + uarta: serial@7d50c000 { > > + compatible = "brcm,bcm7271-uart"; > > + reg = <0x7d50c000 0x20>; > > + reg-names = "uart"; > > + reg-shift = <2>; > > + reg-io-width = <4>; > > + interrupts = <GIC_SPI 276 IRQ_TYPE_LEVEL_HIGH>; > > + skip-init; > > Also an undocumented property upstream, what does it do? Is this to set > UPF_SKIP_TEST? It is U-Boot thing [1]. I suppose I can drop it. Thanks, Ivan [1] ... " serial: pl01x: Add support for devices with the rate pre-configured. For Raspberry Pi, we had the input clock rate to the pl011 fixed in the rpi.c file, but it may be changed by firmware due to user changes to config.txt. Since the firmware always sets up the uart (default 115200 output unless the user changes it), we can just skip our own uart init to simplify the boot process and more reliably get serial output. ..."
On 31/07/2024 08:28, Ivan T. Ivanov wrote: > It looks like they are few revisions of this chip which varies > by number of pins. Perhaps not all of them are available on the > market. Perhaps some of them where early engineering samples, > I don't know. I decided to keep all of them just in case. > > Cc: Andrea della Porta <andrea.porta@suse.com> > Signed-off-by: Ivan T. Ivanov <iivanov@suse.de> > --- > .../pinctrl/brcm,brcmstb-pinctrl.yaml | 73 +++++++++++++++++++ > 1 file changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml > new file mode 100644 > index 000000000000..c5afdb49d784 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml bcm2712 is Rpi, so not really STB. The name is confusing. Please use compatible as filename, so: brcm,bcm2712-pinctrl.yaml > @@ -0,0 +1,73 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/brcm,brcmstb-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom STB family pin controller > + > +maintainers: > + - Ivan T. Ivanov <iivanov@suse.de> > + > +description: > + Broadcom's STB family memory-mapped pin controller. > + > +properties: > + compatible: > + enum: > + - brcm,bcm2712-pinctrl > + - brcm,bcm2712-aon-pinctrl > + - brcm,bcm2712c0-pinctrl > + - brcm,bcm2712c0-aon-pinctrl > + - brcm,bcm2712d0-pinctrl > + - brcm,bcm2712d0-aon-pinctrl > + > + reg: > + maxItems: 1 > + > +allOf: > + - $ref: pinctrl.yaml# "allOf:" block goes after "required:". > + > +required: > + - compatible > + - reg > + > +additionalProperties: > + anyOf: > + - type: object > + allOf: > + - $ref: pincfg-node.yaml# > + - $ref: pinmux-node.yaml# > + > + properties: > + function: > + enum: > + [ Unnecessary new line > + gpio, alt1, alt2, alt3, alt4, alt5, alt6, alt7, alt8, > + aon_cpu_standbyb, aon_fp_4sec_resetb, aon_gpclk, aon_pwm, > + arm_jtag, aud_fs_clk0, avs_pmu_bsc, bsc_m0, bsc_m1, bsc_m2, > + bsc_m3, clk_observe, ctl_hdmi_5v, enet0, enet0_mii, enet0_rgmii, > + ext_sc_clk, fl0, fl1, gpclk0, gpclk1, gpclk2, hdmi_tx0_auto_i2c, > + hdmi_tx0_bsc, hdmi_tx1_auto_i2c, hdmi_tx1_bsc, i2s_in, i2s_out, > + ir_in, mtsif, mtsif_alt, mtsif_alt1, pdm, pkt, pm_led_out, sc0, > + sd0, sd2, sd_card_a, sd_card_b, sd_card_c, sd_card_d, sd_card_e, > + sd_card_f, sd_card_g, spdif_out, spi_m, spi_s, sr_edm_sense, te0, > + te1, tsio, uart0, uart1, uart2, usb_pwr, usb_vbus, uui, vc_i2c0, > + vc_i2c3, vc_i2c4, vc_i2c5, vc_i2csl, vc_pcm, vc_pwm0, vc_pwm1, > + vc_spi0, vc_spi3, vc_spi4, vc_spi5, vc_uart0, vc_uart2, vc_uart3, > + vc_uart4, > + ] > + > + pins: > + items: > + pattern: "^((aon_)?s?gpio[0-6]?[0-9])|(emmc_(clk|cmd|dat[0-7]|ds))$" > + > + bias-disable: true > + bias-pull-down: true > + bias-pull-up: true > + additionalProperties: false > + > + - type: object > + additionalProperties: > + $ref: "#/additionalProperties/anyOf/0" I suggest going with patternProperties, fixed suffix for node names and $defs. See for example: Documentation/devicetree/bindings/pinctrl/qcom,x1e80100-tlmm.yaml Missing example. I don't see this being part of other complete device, so example is a requirement. Best regards, Krzysztof
Hi, On 08-01 10:17, Krzysztof Kozlowski wrote: > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml > > bcm2712 is Rpi, so not really STB. The name is confusing. Please use > compatible as filename, so: > brcm,bcm2712-pinctrl.yaml According Florian it is: https://lore.kernel.org/lkml/f6601f73-cb22-4ba3-88c5-241be8421fc3@broadcom.com/ > > > @@ -0,0 +1,73 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pinctrl/brcm,brcmstb-pinctrl.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Broadcom STB family pin controller > > + > > +maintainers: > > + - Ivan T. Ivanov <iivanov@suse.de> > > + > > +description: > > + Broadcom's STB family memory-mapped pin controller. > > + > > +properties: > > + compatible: > > + enum: > > + - brcm,bcm2712-pinctrl > > + - brcm,bcm2712-aon-pinctrl > > + - brcm,bcm2712c0-pinctrl > > + - brcm,bcm2712c0-aon-pinctrl > > + - brcm,bcm2712d0-pinctrl > > + - brcm,bcm2712d0-aon-pinctrl > > + > > + reg: > > + maxItems: 1 > > + > > +allOf: > > + - $ref: pinctrl.yaml# > > "allOf:" block goes after "required:". > > > + > > +required: > > + - compatible > > + - reg > > + > > +additionalProperties: > > + anyOf: > > + - type: object > > + allOf: > > + - $ref: pincfg-node.yaml# > > + - $ref: pinmux-node.yaml# > > + > > + properties: > > + function: > > + enum: > > + [ > > Unnecessary new line > > > + gpio, alt1, alt2, alt3, alt4, alt5, alt6, alt7, alt8, > > + aon_cpu_standbyb, aon_fp_4sec_resetb, aon_gpclk, aon_pwm, > > + arm_jtag, aud_fs_clk0, avs_pmu_bsc, bsc_m0, bsc_m1, bsc_m2, > > + bsc_m3, clk_observe, ctl_hdmi_5v, enet0, enet0_mii, enet0_rgmii, > > + ext_sc_clk, fl0, fl1, gpclk0, gpclk1, gpclk2, hdmi_tx0_auto_i2c, > > + hdmi_tx0_bsc, hdmi_tx1_auto_i2c, hdmi_tx1_bsc, i2s_in, i2s_out, > > + ir_in, mtsif, mtsif_alt, mtsif_alt1, pdm, pkt, pm_led_out, sc0, > > + sd0, sd2, sd_card_a, sd_card_b, sd_card_c, sd_card_d, sd_card_e, > > + sd_card_f, sd_card_g, spdif_out, spi_m, spi_s, sr_edm_sense, te0, > > + te1, tsio, uart0, uart1, uart2, usb_pwr, usb_vbus, uui, vc_i2c0, > > + vc_i2c3, vc_i2c4, vc_i2c5, vc_i2csl, vc_pcm, vc_pwm0, vc_pwm1, > > + vc_spi0, vc_spi3, vc_spi4, vc_spi5, vc_uart0, vc_uart2, vc_uart3, > > + vc_uart4, > > + ] > > + > > + pins: > > + items: > > + pattern: "^((aon_)?s?gpio[0-6]?[0-9])|(emmc_(clk|cmd|dat[0-7]|ds))$" > > + > > + bias-disable: true > > + bias-pull-down: true > > + bias-pull-up: true > > + additionalProperties: false > > + > > + - type: object > > + additionalProperties: > > + $ref: "#/additionalProperties/anyOf/0" > > I suggest going with patternProperties, fixed suffix for node names and > $defs. See for example: > Documentation/devicetree/bindings/pinctrl/qcom,x1e80100-tlmm.yaml > > Missing example. I don't see this being part of other complete device, > so example is a requirement. > Thanks, I will update and resend. Regards, Ivan
On 01/08/2024 10:38, Ivan T. Ivanov wrote: > Hi, > > On 08-01 10:17, Krzysztof Kozlowski wrote: >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml >> >> bcm2712 is Rpi, so not really STB. The name is confusing. Please use >> compatible as filename, so: >> brcm,bcm2712-pinctrl.yaml > > According Florian it is: > > https://lore.kernel.org/lkml/f6601f73-cb22-4ba3-88c5-241be8421fc3@broadcom.com/ OK, title can be like this, no problem, although then please expand what "STB" means. Bindings still are supposed to use compatible as filename. Best regards, Krzysztof
Hi, [add official Raspberry Pi kernel developer list] Am 31.07.24 um 08:28 schrieb Ivan T. Ivanov: > It looks like they are few revisions of this chip which varies > by number of pins. Perhaps not all of them are available on the > market. Perhaps some of them where early engineering samples, > I don't know. I decided to keep all of them just in case. The BCM2711 had also some revisions and we avoided successfully multiple versions of the RPi 4B DTS. So it would be nice if someone can explain if C0 & D0 are available in the market? Otherwise we may end up with multiple versions of the RPi 5 DTS. I'm missing an explanation in the commit message, what's the difference between brcm,bcm2712-pinctrl and brcm,bcm2712-aon-pinctrl? According to the driver brcm,bcm2712-pinctrl is the same as brcm,bcm2712c0-pinctrl. So the former is more a fallback? Thanks > > Cc: Andrea della Porta <andrea.porta@suse.com> > Signed-off-by: Ivan T. Ivanov <iivanov@suse.de> > --- > .../pinctrl/brcm,brcmstb-pinctrl.yaml | 73 +++++++++++++++++++ > 1 file changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml > new file mode 100644 > index 000000000000..c5afdb49d784 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml > @@ -0,0 +1,73 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/brcm,brcmstb-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom STB family pin controller > + > +maintainers: > + - Ivan T. Ivanov <iivanov@suse.de> > + > +description: > + Broadcom's STB family memory-mapped pin controller. > + > +properties: > + compatible: > + enum: > + - brcm,bcm2712-pinctrl > + - brcm,bcm2712-aon-pinctrl > + - brcm,bcm2712c0-pinctrl > + - brcm,bcm2712c0-aon-pinctrl > + - brcm,bcm2712d0-pinctrl > + - brcm,bcm2712d0-aon-pinctrl > + >
Hi, Am 31.07.24 um 08:28 schrieb Ivan T. Ivanov: > Add GPIO and related interrupt controller nodes and wire one > of the lines to power button. > > Signed-off-by: Ivan T. Ivanov <iivanov@suse.de> > --- > .../boot/dts/broadcom/bcm2712-rpi-5-b.dts | 21 +++++++++++++++++++ > arch/arm64/boot/dts/broadcom/bcm2712.dtsi | 21 +++++++++++++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > index 8a0d20afebfe..06e926af16b7 100644 > --- a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > +++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > @@ -2,6 +2,7 @@ > /dts-v1/; > > #include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/input/input.h> > #include "bcm2712.dtsi" > > / { > @@ -44,6 +45,21 @@ sd_vcc_reg: sd-vcc-reg { > enable-active-high; > gpios = <&gio_aon 4 GPIO_ACTIVE_HIGH>; > }; > + > + pwr-button { gpio-keys > + compatible = "gpio-keys"; > + Please drop the empty line > + pinctrl-names = "default"; > + pinctrl-0 = <&pwr_button_pins>; > + status = "okay"; > + > + pwr_key: pwr { power_button: power-button or do we need keep the reference name for compatibility? > + label = "pwr_button"; > + linux,code = <KEY_POWER>; > + gpios = <&gio 20 GPIO_ACTIVE_LOW>; > + debounce-interval = <50>; > + }; > + }; > }; > > /* The system UART */ > @@ -73,6 +89,11 @@ emmc_aon_cd_pins: emmc-aon-cd-pins { > }; > > &pinctrl { > + pwr_button_pins: pwr-button-pins { > + function = "gpio"; > + pins = "gpio20"; > + bias-pull-up; > + }; > > emmc_sd_pulls: emmc-sd-pulls { > pins = "emmc_cmd", "emmc_dat0", "emmc_dat1", "emmc_dat2", "emmc_dat3"; > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > index 1099171cd435..39d2419ffce2 100644 > --- a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > +++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > @@ -271,6 +271,27 @@ pinctrl: pinctrl@7d504100 { > reg = <0x7d504100 0x30>; > }; > > + main_irq: intc@7d508400 { > + compatible = "brcm,bcm7271-l2-intc"; > + reg = <0x7d508400 0x10>; > + interrupts = <GIC_SPI 244 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-controller; > + #interrupt-cells = <1>; > + }; > + > + gio: gpio@7d508500 { > + compatible = "brcm,brcmstb-gpio"; > + reg = <0x7d508500 0x40>; > + interrupt-parent = <&main_irq>; > + interrupts = <0>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-controller; > + #interrupt-cells = <2>; > + brcm,gpio-bank-widths = <32 22>; > + brcm,gpio-direct; > + }; > + > pinctrl_aon: pinctrl@7d510700 { > compatible = "brcm,bcm2712-aon-pinctrl"; > reg = <0x7d510700 0x20>;
Hi Ivan, Am 31.07.24 um 08:28 schrieb Ivan T. Ivanov: > Add GPIO and related interrupt controller nodes and wire one > of the lines to power button. > Since we enable the GPIO controller here, i think we should provide the relevant gpio-line-names for the Raspberry Pi 5 board. So gpioinfo works from the beginning. Doesn't need to be in this patch directly. Thanks
Hi Ivan, Am 31.07.24 um 08:28 schrieb Ivan T. Ivanov: > On RPi5 device Bluetooth chips is connected to UARTA > port. Add Bluetooth chips and related pin definitions. > With this and firmware already provided by distributions, > at least on openSUSE Tumbleweed, this is sufficient to make > Bluetooth operational on RPi5 \o/. > > Signed-off-by: Ivan T. Ivanov <iivanov@suse.de> > --- > .../boot/dts/broadcom/bcm2712-rpi-5-b.dts | 45 +++++++++++++++++++ > arch/arm64/boot/dts/broadcom/bcm2712.dtsi | 11 +++++ > 2 files changed, 56 insertions(+) > > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > index b6bfe0abb774..a557cbd8ba17 100644 > --- a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > +++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > @@ -133,11 +133,39 @@ wl_on_pins: wl-on-pins { > pins = "gpio28"; > }; > > + bt_shutdown_pins: bt-shutdown-pins { > + function = "gpio"; > + pins = "gpio29"; > + }; > + > emmc_sd_pulls: emmc-sd-pulls { > pins = "emmc_cmd", "emmc_dat0", "emmc_dat1", "emmc_dat2", "emmc_dat3"; > bias-pull-up; > }; > > + uarta_24_pins: uarta-24-pins { > + pin-rts { > + function = "uart0"; > + pins = "gpio24"; > + bias-disable; > + }; > + pin-cts { > + function = "uart0"; > + pins = "gpio25"; > + bias-pull-up; > + }; > + pin-txd { > + function = "uart0"; > + pins = "gpio26"; > + bias-disable; > + }; > + pin-rxd { > + function = "uart0"; > + pins = "gpio27"; > + bias-pull-up; > + }; > + }; > + > sdio2_30_pins: sdio2-30-pins { > pin-clk { > function = "sd2"; > @@ -156,3 +184,20 @@ pins-dat { > }; > }; > }; > + > +/* uarta communicates with the BT module */ > +&uarta { > + uart-has-rtscts; > + auto-flow-control; > + status = "okay"; > + clock-frequency = <96000000>; > + pinctrl-0 = <&uarta_24_pins &bt_shutdown_pins>; > + pinctrl-names = "default"; Please add status here > + > + bluetooth: bluetooth { > + compatible = "brcm,bcm43438-bt"; > + max-speed = <3000000>; > + shutdown-gpios = <&gio 29 GPIO_ACTIVE_HIGH>; > + local-bd-address = [ 00 00 00 00 00 00 ]; Can we drop this? Thanks > + }; > +}; > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > index 3c0663dc6712..e972f94d6828 100644 > --- a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > +++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > @@ -305,6 +305,17 @@ gio: gpio@7d508500 { > brcm,gpio-direct; > }; > > + uarta: serial@7d50c000 { > + compatible = "brcm,bcm7271-uart"; > + reg = <0x7d50c000 0x20>; > + reg-names = "uart"; > + reg-shift = <2>; > + reg-io-width = <4>; > + interrupts = <GIC_SPI 276 IRQ_TYPE_LEVEL_HIGH>; > + skip-init; > + status = "disabled"; > + }; > + > pinctrl_aon: pinctrl@7d510700 { > compatible = "brcm,bcm2712-aon-pinctrl"; > reg = <0x7d510700 0x20>;
Am 31.07.24 um 08:28 schrieb Ivan T. Ivanov: > Add pin-control devicetree nodes and used them to > explicitly define uSD card interface pin configuration. > > Signed-off-by: Ivan T. Ivanov <iivanov@suse.de> > --- > .../boot/dts/broadcom/bcm2712-rpi-5-b.dts | 20 +++++++++++++++++++ > arch/arm64/boot/dts/broadcom/bcm2712.dtsi | 10 ++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > index b5921437e09f..8a0d20afebfe 100644 > --- a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > +++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > @@ -53,10 +53,30 @@ &uart0 { > > /* SDIO1 is used to drive the SD card */ > &sdio1 { > + pinctrl-0 = <&emmc_sd_pulls>, <&emmc_aon_cd_pins>; > + pinctrl-names = "default"; > vqmmc-supply = <&sd_io_1v8_reg>; > vmmc-supply = <&sd_vcc_reg>; > bus-width = <4>; > sd-uhs-sdr50; > sd-uhs-ddr50; > sd-uhs-sdr104; > + cd-gpios = <&gio_aon 5 GPIO_ACTIVE_LOW>; > +}; > + > +&pinctrl_aon { > + emmc_aon_cd_pins: emmc-aon-cd-pins { > + function = "sd_card_g"; > + pins = "aon_gpio5"; > + bias-pull-up; > + }; > +}; > + > +&pinctrl { > + > + emmc_sd_pulls: emmc-sd-pulls { > + pins = "emmc_cmd", "emmc_dat0", "emmc_dat1", "emmc_dat2", "emmc_dat3"; > + bias-pull-up; > + }; > + > }; Please keep the references in alphabetical order (pinctrl comes before sdio). Except of this: Reviewed-by: Stefan Wahren <wahrenst@gmx.net> > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > index 398df13148bd..1099171cd435 100644 > --- a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > +++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > @@ -266,6 +266,16 @@ uart0: serial@7d001000 { > status = "disabled"; > }; > > + pinctrl: pinctrl@7d504100 { > + compatible = "brcm,bcm2712-pinctrl"; > + reg = <0x7d504100 0x30>; > + }; > + > + pinctrl_aon: pinctrl@7d510700 { > + compatible = "brcm,bcm2712-aon-pinctrl"; > + reg = <0x7d510700 0x20>; > + }; > + > interrupt-controller@7d517000 { > compatible = "brcm,bcm7271-l2-intc"; > reg = <0x7d517000 0x10>;
Hi, On 08-01 16:19, Krzysztof Kozlowski wrote: > > On 01/08/2024 10:38, Ivan T. Ivanov wrote: > > Hi, > > > > On 08-01 10:17, Krzysztof Kozlowski wrote: > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml > >> > >> bcm2712 is Rpi, so not really STB. The name is confusing. Please use > >> compatible as filename, so: > >> brcm,bcm2712-pinctrl.yaml > > > > According Florian it is: > > > > https://lore.kernel.org/lkml/f6601f73-cb22-4ba3-88c5-241be8421fc3@broadcom.com/ > > OK, title can be like this, no problem, although then please expand what > "STB" means. Ok. > > Bindings still are supposed to use compatible as filename. > IIUC, you suggest to rename compatible string from brcm,bcm2712-pinctrl to brcm,brcmstb-pinctrl?! Regards, Ivan
Hi, On 08-02 21:08, Stefan Wahren wrote: > > Hi Ivan, > > Am 31.07.24 um 08:28 schrieb Ivan T. Ivanov: > > Add GPIO and related interrupt controller nodes and wire one > > of the lines to power button. > > > Since we enable the GPIO controller here, i think we should provide the > relevant gpio-line-names for the Raspberry Pi 5 board. So gpioinfo works > from the beginning. Doesn't need to be in this patch directly. > Sure, I was planning to send this later to not make this patchest too large. Regards, Ivan
Hi Stefan Sorry for the delay in responding - I was on holiday last week. On Fri, 2 Aug 2024 at 19:10, Stefan Wahren <wahrenst@gmx.net> wrote: > > Hi, > > [add official Raspberry Pi kernel developer list] > > Am 31.07.24 um 08:28 schrieb Ivan T. Ivanov: > > It looks like they are few revisions of this chip which varies > > by number of pins. Perhaps not all of them are available on the > > market. Perhaps some of them where early engineering samples, > > I don't know. I decided to keep all of them just in case. > The BCM2711 had also some revisions and we avoided successfully multiple > versions of the RPi 4B DTS. So it would be nice if someone can explain > if C0 & D0 are available in the market? Otherwise we may end up with > multiple versions of the RPi 5 DTS. AFAIK A0 and B0 silicon were never commercialised. C0 is the current revision in use with Pi5. D0 will be in devices imminently. CM5 will use it from launch, but subsequently standard Pi5s will do as well. In addition to putting in the few fixes that were desired, some registers and DMA dreqs got shuffled around, hence some drivers will need additional compatible strings (vc4 certainly does) and other minor DT tweaks. Checking our downstream dt files, we have bcm2712d0-rpi-5-b.dts[1] that includes and patches the original (C0) bcm2712-rpi-5-b.dts[2]. The cleaner option would be to have a common bcm2712-rpi-5-b.dts(i) and separate bcm2712c0-rpi-5-b.dts bcm2712d0-rpi-5-b.dts which include the base and add the relevant customisations. Later a bcm2712d0-rpi-cm5.dts DT should be able to include that same base file as well. I'm not quite sure why the GPIO names are redefined in our d0 file - other than the unused ones using "-" instead of "", they appear identical. > I'm missing an explanation in the commit message, what's the difference > between brcm,bcm2712-pinctrl and brcm,bcm2712-aon-pinctrl? Two separate instantiations of the same IP block, but they differ in the number of pins that are associated and the pinmux functions for each of those pins. AFAIK there is no way from DT to specify those pinmux function names, so otherwise /sys/kernel/debug/pinctrl/<node>/pins will give the wrong function mappings. > According to the driver brcm,bcm2712-pinctrl is the same as > brcm,bcm2712c0-pinctrl. So the former is more a fallback? I'd need to check with Phil (who's on holiday this week) or Dom, but I believe you are correct that "brcm,bcm2712-pinctrl" is a fallback. Most likely due to our early DT files not having the c0 designation. Obviously for mainline that is irrelevant, so dropping the non-specific compatibles is fine. I hope that makes some more sense. Dave [1] https://github.com/raspberrypi/linux/blob/rpi-6.6.y/arch/arm64/boot/dts/broadcom/bcm2712d0-rpi-5-b.dts [2] https://github.com/raspberrypi/linux/blob/rpi-6.6.y/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > Thanks > > > > Cc: Andrea della Porta <andrea.porta@suse.com> > > Signed-off-by: Ivan T. Ivanov <iivanov@suse.de> > > --- > > .../pinctrl/brcm,brcmstb-pinctrl.yaml | 73 +++++++++++++++++++ > > 1 file changed, 73 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml > > new file mode 100644 > > index 000000000000..c5afdb49d784 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml > > @@ -0,0 +1,73 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pinctrl/brcm,brcmstb-pinctrl.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Broadcom STB family pin controller > > + > > +maintainers: > > + - Ivan T. Ivanov <iivanov@suse.de> > > + > > +description: > > + Broadcom's STB family memory-mapped pin controller. > > + > > +properties: > > + compatible: > > + enum: > > + - brcm,bcm2712-pinctrl > > + - brcm,bcm2712-aon-pinctrl > > + - brcm,bcm2712c0-pinctrl > > + - brcm,bcm2712c0-aon-pinctrl > > + - brcm,bcm2712d0-pinctrl > > + - brcm,bcm2712d0-aon-pinctrl > > + > > >
Hi Dave, Am 12.08.24 um 18:28 schrieb Dave Stevenson: > Hi Stefan > > Sorry for the delay in responding - I was on holiday last week. > no problem and thanks for the explanations. >> I'm missing an explanation in the commit message, what's the difference >> between brcm,bcm2712-pinctrl and brcm,bcm2712-aon-pinctrl? > Two separate instantiations of the same IP block, but they differ in > the number of pins that are associated and the pinmux functions for > each of those pins. AFAIK there is no way from DT to specify those > pinmux function names, so otherwise > /sys/kernel/debug/pinctrl/<node>/pins will give the wrong function > mappings. Yes, my request is that this or a similar explanation should go to the commit message, because not all reviewers know the IP block. It would be great to explain the aon part. Always on? >> According to the driver brcm,bcm2712-pinctrl is the same as >> brcm,bcm2712c0-pinctrl. So the former is more a fallback? > I'd need to check with Phil (who's on holiday this week) or Dom, but I > believe you are correct that "brcm,bcm2712-pinctrl" is a fallback. > Most likely due to our early DT files not having the c0 designation. > Obviously for mainline that is irrelevant, so dropping the > non-specific compatibles is fine. I agree. Regards > > I hope that makes some more sense. > > Dave > > [1] https://github.com/raspberrypi/linux/blob/rpi-6.6.y/arch/arm64/boot/dts/broadcom/bcm2712d0-rpi-5-b.dts > [2] https://github.com/raspberrypi/linux/blob/rpi-6.6.y/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts