mbox series

[0/7] Add pin control driver for BCM2712 SoC

Message ID 20240731062814.215833-1-iivanov@suse.de
Headers show
Series Add pin control driver for BCM2712 SoC | expand

Message

Ivan T. Ivanov July 31, 2024, 6:28 a.m. UTC
Hi,

The following patches add a pin control driver for the BCM2712 SoC and few
pin/gpio Devicetree nodes for Raspberry Pi 5.

Device driver is follow up version on what Andrea posted in April [1].

It is based on sources from here [2]. I just made few cosmetic changes
and addressed review comments from earlier submission. I don't have
documentation for this controller.

Patch 3 was already posted by Andrea and it is in Broadcom integration tree[3].
Unfortunately it is still not in the Linus tree[4]. So I added it here as base
for my changes and to easy building and testing.

Patch 5 wire up power button on RPi5

Patch 6 adds WiFi Devicetree node for RPi5

Patch 7 adds Bluetooth Devicetree node for RPi5

They are few complaints from checkpatch.pl. Like few lines over 100 columns,
which I keep that way for better readability or usage of EOPNOTSUPP,
but it is according pin control API document.

All this have been tested as kernel was directly booted RPi5 via
kernel= config.txt option and cmdline.txt file with following content
(Note I am using Tumbleweed RPi raw images)

# cat /boot/efi/cmdline.txt
root=/dev/mmcblk0p3 rootwait rw systemd.show_status=1 console=tty ignore_loglevel earlycon console=ttyAMA10,115200

With all these patches Bluetooth and Wifi are working fine (tm) with
firmware files provided by openSUSE Tumbleweed.

All comments and suggestions are welcome!

Happy hacking!
Ivan

[1] https://lore.kernel.org/lkml/f6601f73-cb22-4ba3-88c5-241be8421fc3@broadcom.com/
[2] https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/pinctrl/bcm/pinctrl-bcm2712.c
[3] https://lore.kernel.org/lkml/20240605120712.3523290-1-florian.fainelli@broadcom.com/#t
[4] https://lore.kernel.org/all/bfc60a7e-54d2-48a6-a288-4fe76d66507a@gmx.net/


Andrea della Porta (1):
  arm64: dts: broadcom: Add support for BCM2712

Ivan T. Ivanov (6):
  dt-bindings: pinctrl: Add support for Broadcom STB pin controller
  pinctrl: bcm: Add STB family pin controller driver
  arm64: dts: broadcom: bcm2712: Add pin controller nodes
  arm64: dts: broadcom: bcm2712: Add one more GPIO node
  arm64: dts: broadcom: bcm2712: Add second SDHCI controller node
  arm64: dts: broadcom: bcm2712: Add UARTA controller node.

 .../pinctrl/brcm,brcmstb-pinctrl.yaml         |   73 +
 arch/arm64/boot/dts/broadcom/Makefile         |    1 +
 .../boot/dts/broadcom/bcm2712-rpi-5-b.dts     |  203 +++
 arch/arm64/boot/dts/broadcom/bcm2712.dtsi     |  357 +++++
 drivers/pinctrl/bcm/Kconfig                   |   13 +
 drivers/pinctrl/bcm/Makefile                  |    1 +
 drivers/pinctrl/bcm/pinctrl-brcmstb.c         | 1217 +++++++++++++++++
 7 files changed, 1865 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,brcmstb-pinctrl.yaml
 create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
 create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712.dtsi
 create mode 100644 drivers/pinctrl/bcm/pinctrl-brcmstb.c


base-commit: dc1c8034e31b14a2e5e212104ec508aec44ce1b9

Comments

Florian Fainelli July 31, 2024, 10:13 p.m. UTC | #1
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?
Ivan T. Ivanov Aug. 1, 2024, 7:19 a.m. UTC | #2
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.
 ... "
Ivan T. Ivanov Aug. 1, 2024, 7:50 a.m. UTC | #3
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.
    ..."
Krzysztof Kozlowski Aug. 1, 2024, 8:17 a.m. UTC | #4
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
Ivan T. Ivanov Aug. 1, 2024, 8:38 a.m. UTC | #5
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
Krzysztof Kozlowski Aug. 1, 2024, 2:19 p.m. UTC | #6
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
Stefan Wahren Aug. 2, 2024, 6:09 p.m. UTC | #7
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
> +
>
Stefan Wahren Aug. 2, 2024, 6:49 p.m. UTC | #8
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>;
Stefan Wahren Aug. 2, 2024, 7:08 p.m. UTC | #9
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
Stefan Wahren Aug. 2, 2024, 7:12 p.m. UTC | #10
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>;
Stefan Wahren Aug. 2, 2024, 7:16 p.m. UTC | #11
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>;
Ivan T. Ivanov Aug. 5, 2024, 9:27 a.m. UTC | #12
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
Ivan T. Ivanov Aug. 5, 2024, 9:37 a.m. UTC | #13
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
Dave Stevenson Aug. 12, 2024, 4:28 p.m. UTC | #14
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
> > +
> >
>
Stefan Wahren Aug. 22, 2024, 10:54 a.m. UTC | #15
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