| Message ID | cover.1713036964.git.andrea.porta@suse.com |
|---|---|
| Headers | show |
| Series | Add support for BCM2712 SD card controller | expand |
On Sun, 14 Apr 2024 00:14:23 +0200, Andrea della Porta wrote: > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > .../pinctrl/brcm,bcm2712-pinctrl.yaml | 99 +++++++++++++++++++ > 1 file changed, 99 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:46:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:47:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:48:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:49:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:50:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:51:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:52:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:53:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:54:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:55:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:56:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:57:18: [warning] wrong indentation: expected 18 but found 17 (indentation) ./Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml:58:18: [warning] wrong indentation: expected 18 but found 17 (indentation) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/2d1272cad92ad618297a6683e9264e31b8f2df73.1713036964.git.andrea.porta@suse.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 14/04/2024 00:14, Andrea della Porta wrote: > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> Missing commit msg. We can't take empty commits. You have entire commit msg to describe the hardware, why not doing this? Plus, this wasn't even tested... > --- > .../pinctrl/brcm,bcm2712-pinctrl.yaml | 99 +++++++++++++++++++ > 1 file changed, 99 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml > new file mode 100644 > index 000000000000..2908dfe99f3e > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml > @@ -0,0 +1,99 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm2712-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom BCM2712 pin controller > + > +maintainers: > + - Andrea della Porta <andrea.porta@suse.com> > + > +description: > + Bindings for Broadcom's BCM2712 memory-mapped pin controller. Drop "Bindings for" and describe hardware. Bindings do not have to tell they are bindings, it's obvious. They cannot be anything else. > + > +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: > + items: > + - description: pin control registers > + > +allOf: > + - $ref: pinctrl.yaml# > + > +required: > + - compatible > + - reg > + > +additionalProperties: > + anyOf: > + - type: object > + allOf: > + - $ref: pincfg-node.yaml# > + - $ref: pinmux-node.yaml# > + > + properties: > + function: > + enum: [ 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" > + > +examples: > + - | > + pinctrl: pinctrl@7d504100 { > + compatible = "brcm,bcm2712-pinctrl"; > + reg = <0x7d504100 0x30>; > + > + uarta_24_pins: uarta_24_pins { Underscores are not allowed. Please observe the rules of DTS coding style (see docs). > + pin_rts { > + function = "uart0"; > + pins = "gpio24"; > + bias-disable; > + }; > + > + pin_cts { > + function = "uart0"; > + pins = "gpio25"; > + bias-pull-up; > + }; > + }; > + > + spi10_gpio2: spi10_gpio2 { > + function = "vc_spi0"; > + pins = "gpio2", "gpio3", "gpio4"; Messed indentation. Keep 4 space. > + bias-disable; > + }; > + }; > +... Best regards, Krzysztof
On 14/04/2024 00:14, Andrea della Porta wrote: > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > arch/arm64/boot/dts/broadcom/Makefile | 1 + > .../boot/dts/broadcom/bcm2712-rpi-5-b.dts | 313 +++++++ > arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi | 81 ++ > arch/arm64/boot/dts/broadcom/bcm2712.dtsi | 841 ++++++++++++++++++ > 4 files changed, 1236 insertions(+) > create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi > create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712.dtsi > > diff --git a/arch/arm64/boot/dts/broadcom/Makefile b/arch/arm64/boot/dts/broadcom/Makefile > index 8b4591ddd27c..92565e9781ad 100644 > --- a/arch/arm64/boot/dts/broadcom/Makefile > +++ b/arch/arm64/boot/dts/broadcom/Makefile > @@ -6,6 +6,7 @@ DTC_FLAGS := -@ > dtb-$(CONFIG_ARCH_BCM2835) += bcm2711-rpi-400.dtb \ > bcm2711-rpi-4-b.dtb \ > bcm2711-rpi-cm4-io.dtb \ > + bcm2712-rpi-5-b.dtb \ > bcm2837-rpi-3-a-plus.dtb \ > bcm2837-rpi-3-b.dtb \ > bcm2837-rpi-3-b-plus.dtb \ > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > new file mode 100644 > index 000000000000..2ce180a54e5b > --- /dev/null > +++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > @@ -0,0 +1,313 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/dts-v1/; > + > +#include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/interrupt-controller/irq.h> > +#include <dt-bindings/pwm/pwm.h> > +#include <dt-bindings/reset/raspberrypi,firmware-reset.h> > + > +#define spi0 _spi0 > +#define uart0 _uart0 > + > +#include "bcm2712.dtsi" > + > +#undef spi0 > +#undef uart0 > + > +/ { > + compatible = "raspberrypi,5-model-b", "brcm,bcm2712"; This patch did not pass basic tests. Like checkpatch. Please run scripts/checkpatch.pl and fix reported warnings. Then please run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. It does not look like you tested the DTS against bindings. Please run `make dtbs_check W=1` (see Documentation/devicetree/bindings/writing-schema.rst or https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ for instructions). > + model = "Raspberry Pi 5"; > + > + /* Will be filled by the bootloader */ > + memory@0 { > + device_type = "memory"; > + reg = <0 0 0x28000000>; > + }; > + > + leds: leds { > + compatible = "gpio-leds"; > + > + led_act: led-act { > + label = "ACT"; > + gpios = <&gio_aon 9 GPIO_ACTIVE_LOW>; > + default-state = "off"; > + linux,default-trigger = "mmc0"; > + }; > + }; > + > + sd_io_1v8_reg: sd_io_1v8_reg { Don't push to us downstream code. Please fix it first and adjust to match DTS coding style. Underscores are not allowed in node names. > + compatible = "regulator-gpio"; > + regulator-name = "vdd-sd-io"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <3300000>; > + regulator-boot-on; > + regulator-always-on; > + regulator-settling-time-us = <5000>; > + gpios = <&gio_aon 3 GPIO_ACTIVE_HIGH>; > + states = <1800000 0x1 > + 3300000 0x0>; Aren't these two tupples? > + status = "okay"; Why? Where is it disabled? > + }; > + > + sd_vcc_reg: sd_vcc_reg { Underscores... > + compatible = "regulator-fixed"; > + regulator-name = "vcc-sd"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + regulator-boot-on; > + enable-active-high; > + gpios = <&gio_aon 4 GPIO_ACTIVE_HIGH>; > + status = "okay"; Why? > + }; > + > + wl_on_reg: wl_on_reg { > + compatible = "regulator-fixed"; > + regulator-name = "wl-on-regulator"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + pinctrl-0 = <&wl_on_pins>; > + pinctrl-names = "default"; > + > + gpio = <&gio 28 GPIO_ACTIVE_HIGH>; > + > + startup-delay-us = <150000>; > + enable-active-high; > + }; > + > + clocks: clocks { > + }; Drop, useless. > +}; > + > +// Add some labels to 2712 device > + > +// The system UART > +uart10: &_uart0 { status = "okay"; }; > + > +// The system SPI for the bootloader EEPROM > +spi10: &_spi0 { status = "okay"; }; Use standard coding style. Look at other recent platforms how it is done. &spi { foo; }; > + > +#include "bcm2712-rpi.dtsi" This goes to the top. I must say this DTS is terrible to read. > + > +/* 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>; > + //no-1-8-v; Do not add dead code to the kernel. > + status = "okay"; > +}; > + > +&pinctrl_aon { > + emmc_aon_cd_pins: emmc_aon_cd_pins { Again, no underscores. > + function = "sd_card_g"; > + pins = "aon_gpio5"; > + bias-pull-up; > + }; > + > + /* Slight hack - only one PWM pin (status LED) is usable */ > + aon_pwm_1pin: aon_pwm_1pin { > + function = "aon_pwm"; > + pins = "aon_gpio9"; > + }; > +}; > + > +&pinctrl { > + pwr_button_pins: pwr_button_pins { > + function = "gpio"; > + pins = "gpio20"; > + bias-pull-up; > + }; > + > + wl_on_pins: wl_on_pins { > + function = "gpio"; > + 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; > + }; > +}; > + > +/ { Why the heck this appears in the middle? This is top level section. I am sorry, but this DTS looks really poor and not like existing coding style. Please do not introduce some entirely different coding styles. > + chosen: chosen { > + bootargs = "reboot=w coherent_pool=1M 8250.nr_uarts=1 pci=pcie_bus_safe snd_bcm2835.enable_compat_alsa=0 snd_bcm2835.enable_hdmi=1"; Not a DTS properties. Drop entire bootargs. > + stdout-path = "serial10:115200n8"; > + }; > + > + pwr_button { Srsly... > + compatible = "gpio-keys"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&pwr_button_pins>; > + status = "okay"; ??? > + > + pwr_key: pwr { OK, you definitely did not test it. The code looks worse and worse I keep looking, so I will stop. This did not pass basic internal review, checkpatch, basic tests. ... > + > + // Pad bank0 out to 32 entries > + "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", > + > + "HDMI0_SCL", // AON_SGPIO_00 > + "HDMI0_SDA", // AON_SGPIO_01 > + "HDMI1_SCL", // AON_SGPIO_02 > + "HDMI1_SDA", // AON_SGPIO_03 > + "PMIC_SCL", // AON_SGPIO_04 > + "PMIC_SDA"; // AON_SGPIO_05 > +}; > + > +/ { > + aliases { OK, now you are trolling us. It is third top-level node! Limited review follows. > + blconfig = &blconfig; > + blpubkey = &blpubkey; > + console = &uart10; > + mailbox = &mailbox; > + mmc0 = &sdio1; > + uart10 = &uart10; > + serial10 = &uart10; > + gpio1 = &gio; > + gpio2 = &gio_aon; > + gpio3 = &pinctrl; > + gpio4 = &pinctrl_aon; > + }; > + > + __overrides__ { ? Drop > + button_debounce = <&pwr_key>, "debounce-interval:0"; > + random = <&random>, "status"; > + sd_cqe = <&sdio1>, "supports-cqe?"; > + suspend = <&pwr_key>, "linux,code:0=205"; > + act_led_activelow = <&led_act>,"gpios:8"; > + act_led_trigger = <&led_act>, "linux,default-trigger"; > + }; > +}; > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi > new file mode 100644 > index 000000000000..d04e39b9c0b6 > --- /dev/null > +++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi What is this file for? > @@ -0,0 +1,81 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <dt-bindings/power/raspberrypi-power.h> > + > +&soc { > + firmware: firmware { > + compatible = "raspberrypi,bcm2835-firmware", "simple-mfd"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + mboxes = <&mailbox>; > + dma-ranges; > + > + firmware_clocks: clocks { > + compatible = "raspberrypi,firmware-clocks"; > + #clock-cells = <1>; > + }; > + > + reset: reset { > + compatible = "raspberrypi,firmware-reset"; > + #reset-cells = <1>; > + }; > + }; > + > + power: power { > + compatible = "raspberrypi,bcm2835-power"; > + firmware = <&firmware>; > + #power-domain-cells = <1>; > + }; > + > + /* Define these notional regulators for use by overlays, etc. */ > + vdd_3v3_reg: fixedregulator_3v3 { W=2 warnings. > + compatible = "regulator-fixed"; > + regulator-always-on; > + regulator-max-microvolt = <3300000>; > + regulator-min-microvolt = <3300000>; > + regulator-name = "3v3"; > + }; > + > + vdd_5v0_reg: fixedregulator_5v0 { W=2 warnings. > + compatible = "regulator-fixed"; > + regulator-always-on; > + regulator-max-microvolt = <5000000>; > + regulator-min-microvolt = <5000000>; > + regulator-name = "5v0"; > + }; > +}; > + > +/ { > + __overrides__ { > + arm_freq; NAK, drop. > + }; > +}; > + > +&rmem { > + /* > + * RPi5's co-processor will copy the board's bootloader configuration > + * into memory for the OS to consume. It'll also update this node with > + * its placement information. > + */ > + blconfig: nvram@0 { > + compatible = "raspberrypi,bootloader-config", "nvmem-rmem"; > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0x0 0x0 0x0>; > + no-map; > + status = "disabled"; > + }; > + /* > + * RPi5 will copy the binary public key blob (if present) from the bootloader > + * into memory for use by the OS. > + */ > + blpubkey: nvram@1 { > + compatible = "raspberrypi,bootloader-public-key", "nvmem-rmem"; > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0x0 0x0 0x0>; > + no-map; > + status = "disabled"; > + }; > +}; > diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > new file mode 100644 > index 000000000000..fd5a19f68b49 > --- /dev/null > +++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi > @@ -0,0 +1,841 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/soc/bcm2835-pm.h> > +#include <dt-bindings/phy/phy.h> > + > +/ { > + compatible = "brcm,bcm2712", "brcm,bcm2711"; Please run scripts/checkpatch.pl and fix reported warnings. Then please run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. > + model = "BCM2712"; Drop > + > + clk_27MHz: clk-27M { No upperscore letters. > + #clock-cells = <0>; > + compatible = "fixed-clock"; > + clock-frequency = <27000000>; > + clock-output-names = "27MHz-clock"; > + }; > + > + clk_108MHz: clk-108M { > + #clock-cells = <0>; > + compatible = "fixed-clock"; > + clock-frequency = <108000000>; > + clock-output-names = "108MHz-clock"; > + }; > + > + soc: soc { > + compatible = "simple-bus"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + ranges = <0x7c000000 0x10 0x7c000000 0x04000000>; > + /* Emulate a contiguous 30-bit address range for DMA */ > + dma-ranges = <0xc0000000 0x00 0x00000000 0x40000000>, > + <0x7c000000 0x10 0x7c000000 0x04000000>; > + > + system_timer: timer@7c003000 { > + compatible = "brcm,bcm2835-system-timer"; > + reg = <0x7c003000 0x1000>; > + interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; > + clock-frequency = <1000000>; > + }; > + > + mailbox: mailbox@7c013880 { > + compatible = "brcm,bcm2835-mbox"; > + reg = <0x7c013880 0x40>; > + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; > + #mbox-cells = <0>; > + }; > + > + disp_intr: interrupt-controller@7c502000 { > + compatible = "brcm,bcm2711-l2-intc", "brcm,l2-intc"; > + reg = <0x7c502000 0x30>; > + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-controller; > + #interrupt-cells = <1>; > + status = "disabled"; > + }; > + > + dvp: clock@7c700000 { > + compatible = "brcm,brcm2711-dvp"; > + reg = <0x7c700000 0x10>; > + clocks = <&clk_108MHz>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + }; > + > + /* > + * This node is the provider for the enable-method for > + * bringing up secondary cores. > + */ > + local_intc: local_intc@7cd00000 { You really need to clean this up... > + compatible = "brcm,bcm2836-l1-intc"; > + reg = <0x7cd00000 0x100>; > + }; > + > + uart0: serial@7d001000 { > + compatible = "arm,pl011", "arm,primecell"; > + reg = <0x7d001000 0x200>; > + interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk_uart>, > + <&clk_vpu>; > + clock-names = "uartclk", "apb_pclk"; > + arm,primecell-periphid = <0x00241011>; > + status = "disabled"; > + }; > + > + uart2: serial@7d001400 { > + compatible = "arm,pl011", "arm,primecell"; > + reg = <0x7d001400 0x200>; > + interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk_uart>, > + <&clk_vpu>; > + clock-names = "uartclk", "apb_pclk"; > + arm,primecell-periphid = <0x00241011>; > + status = "disabled"; > + }; > + > + uart5: serial@7d001a00 { > + compatible = "arm,pl011", "arm,primecell"; > + reg = <0x7d001a00 0x200>; > + interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk_uart>, > + <&clk_vpu>; > + clock-names = "uartclk", "apb_pclk"; > + arm,primecell-periphid = <0x00241011>; > + status = "disabled"; > + }; > + > + sdhost: mmc@7d002000 { > + compatible = "brcm,bcm2835-sdhost"; > + reg = <0x7d002000 0x100>; > + //interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>; No dead code. ... > + > + random: rng@7d208000 { > + compatible = "brcm,bcm2711-rng200"; > + reg = <0x7d208000 0x28>; > + status = "okay"; Drop. I just ignored the rest. Quality does not improve. This DTS is in very poor shape and not suitable for mainline submission. Please very carefully read DTS coding style and send DTS only after fixing all automation errors (all! so checkpatch, W=1, dtbs_check W=1) and after aligning this in 100% to DTS coding style. Best regards, Krzysztof
Le 14/04/2024 à 00:14, Andrea della Porta a écrit : > Add a pincontrol driver for BCM2712. BCM2712 allows muxing GPIOs > and setting configuration on pads. > > Originally-by: Jonathan Bell <jonathan@raspberrypi.com> > Originally-by: Phil Elwell <phil@raspberrypi.com> > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > drivers/pinctrl/bcm/Kconfig | 9 + > drivers/pinctrl/bcm/Makefile | 1 + > drivers/pinctrl/bcm/pinctrl-bcm2712.c | 1247 +++++++++++++++++++++++++ > 3 files changed, 1257 insertions(+) > create mode 100644 drivers/pinctrl/bcm/pinctrl-bcm2712.c ... > +static int bcm2712_pmx_get_function_groups(struct pinctrl_dev *pctldev, > + unsigned selector, > + const char * const **groups, > + unsigned * const num_groups) > +{ > + struct bcm2712_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev); Missing empty new line. > + /* every pin can do every function */ > + *groups = pc->gpio_groups; > + *num_groups = pc->pctl_desc.npins; > + > + return 0; > +} ... > +static int bcm2712_pinconf_get(struct pinctrl_dev *pctldev, > + unsigned pin, unsigned long *config) > +{ > + struct bcm2712_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev); > + enum pin_config_param param = pinconf_to_config_param(*config); > + u32 arg; > + > + switch (param) { > + case PIN_CONFIG_BIAS_DISABLE: > + arg = (bcm2712_pull_config_get(pc, pin) == BCM2712_PULL_NONE); > + break; > + case PIN_CONFIG_BIAS_PULL_DOWN: > + arg = (bcm2712_pull_config_get(pc, pin) == BCM2712_PULL_DOWN); > + break; > + case PIN_CONFIG_BIAS_PULL_UP: > + arg = (bcm2712_pull_config_get(pc, pin) == BCM2712_PULL_UP); > + break; > + default: > + return -ENOTSUPP; > + } > + > + *config = pinconf_to_config_packed(param, arg); > + > + return -ENOTSUPP; Strange. return 0; ? > +} > + > +static int bcm2712_pinconf_set(struct pinctrl_dev *pctldev, > + unsigned int pin, unsigned long *configs, > + unsigned int num_configs) > +{ > + struct bcm2712_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev); > + u32 param, arg; > + int i; > + > + for (i = 0; i < num_configs; i++) { > + param = pinconf_to_config_param(configs[i]); > + arg = pinconf_to_config_argument(configs[i]); > + > + switch (param) { > + case PIN_CONFIG_BIAS_DISABLE: > + bcm2712_pull_config_set(pc, pin, BCM2712_PULL_NONE); > + break; > + case PIN_CONFIG_BIAS_PULL_DOWN: > + bcm2712_pull_config_set(pc, pin, BCM2712_PULL_DOWN); > + break; > + case PIN_CONFIG_BIAS_PULL_UP: > + bcm2712_pull_config_set(pc, pin, BCM2712_PULL_UP); > + break; > + default: > + return -ENOTSUPP; > + } > + } /* for each config */ This comment is not really usefull, IMHO. > + > + return 0; > +} ... > +static int bcm2712_pinctrl_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + //struct device_node *np = dev->of_node; > + const struct bcm_plat_data *pdata; > + //const struct of_device_id *match; > + struct bcm2712_pinctrl *pc; > + const char **names; > + int num_pins, i; > + > + pdata = device_get_match_data(&pdev->dev); > + if (!pdata) > + return -EINVAL; > + > + pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL); > + if (!pc) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, pc); > + pc->dev = dev; > + spin_lock_init(&pc->lock); > + > + //pc->base = devm_of_iomap(dev, np, 0, NULL); Any use for this commented code? (and variable declarations above) CJ > + pc->base = devm_platform_ioremap_resource(pdev, 0); > + if (WARN_ON(IS_ERR(pc->base))) { > + //dev_err(dev, "could not get IO memory\n"); > + return PTR_ERR(pc->base); > + } > + > + pc->pctl_desc = *pdata->pctl_desc; > + num_pins = pc->pctl_desc.npins; > + names = devm_kmalloc_array(dev, num_pins, sizeof(const char *), > + GFP_KERNEL); > + if (!names) > + return -ENOMEM; > + for (i = 0; i < num_pins; i++) > + names[i] = pc->pctl_desc.pins[i].name; > + pc->gpio_groups = names; > + pc->pin_regs = pdata->pin_regs; > + pc->pin_funcs = pdata->pin_funcs; > + pc->pctl_dev = devm_pinctrl_register(dev, &pc->pctl_desc, pc); > + if (IS_ERR(pc->pctl_dev)) > + return PTR_ERR(pc->pctl_dev); > + > + pc->gpio_range = *pdata->gpio_range; > + pinctrl_add_gpio_range(pc->pctl_dev, &pc->gpio_range); > + > + return 0; > +} ...
Hi Andrea, Am 14.04.24 um 00:14 schrieb Andrea della Porta: > Hi, > > This patchset adds support for the SDHCI controller on Broadcom BCM2712 > SoC in order to make it possible to boot (particularly) Raspberry Pi 5 > from SD card. This work is heavily based on downstream contributions. since your goal is minimal Raspberry Pi 5 support, i suggest to use this as the subject for this patch. > Patch #1 and 2: introduce the dt binding definitions for, respectively, > the new pin cfg/mux controller and the SD host controller as a preparatory > step for the upcoming dts. > > Patch #3: add a somewhat reasonable (*almost* bare-minimum) dts to be used > to boot Rpi5 boards. Since till now there was no support at all for any > 2712 based chipset, both the SoC and board dts plus definitions for the > new Pin and SD host controller have been added. The patch still seems to contain a lot unnecessary stuff (Wifi, BT, SPI), please try to remove as much as possible for the minimal support (just boot via debug UART & SD card) in order to make review easier. Btw this patch must be after pinctrl & SDHCI support. > Patch #4: the driver supporting the pin controller. Based on [1] and > successive fix commits. > > Patch #5: add SDHCI support. Based on [2] and the next 2 fix commits. > Drop the SD Express implementation for now, that will be added by patch > #6. > > Patch #6: this patch offers SD Express support and can be considered totally > optional. The callback plumbing is slightly different w.r.t. the downstream > approach (see [3]), as explained in the patch comment. Not sure what is the best, > any comment is highly appreciated. I don't think this should be necessary for minimal Raspberry Pi 5 support. Maybe this should be addressed later. More important would be an additional patch which enables the necessary drivers in arm64/defconfig. > > Tested succesfully on Raspberry Pi 5 using an SDxC card as the boot device. > > Still untested: > - SD Express due to the lack of an Express capable card. > Also, it will need PCIe support first. > - card detection pin, since the sd was the booting and root fs device. > > Many thanks, > Andrea > > Links: > [1] - https://github.com/raspberrypi/linux/commit/d9b655314a826724538867bf9b6c229d04c25d84 > [2] - https://github.com/raspberrypi/linux/commit/e3aa070496e840e72a4dc384718690ea4125fa6a > [3] - https://github.com/raspberrypi/linux/commit/eb1df34db2a9a5b752eba40ee298c4ae87e26e87 > > Andrea della Porta (6): > dt-bindings: pinctrl: Add support for BCM2712 pin controller > dt-bindings: mmc: Add support for BCM2712 SD host controller > arm64: dts: broadcom: Add support for BCM2712 > pinctrl: bcm: Add pinconf/pinmux controller driver for BCM2712 > mmc: sdhci-brcmstb: Add BCM2712 support > mmc: sdhci-brcmstb: Add BCM2712 SD Express support > > .../bindings/mmc/brcm,sdhci-brcmstb.yaml | 51 +- > .../pinctrl/brcm,bcm2712-pinctrl.yaml | 99 ++ > arch/arm64/boot/dts/broadcom/Makefile | 1 + > .../boot/dts/broadcom/bcm2712-rpi-5-b.dts | 313 +++++ > arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi | 81 ++ > arch/arm64/boot/dts/broadcom/bcm2712.dtsi | 841 +++++++++++ > drivers/mmc/host/Kconfig | 1 + > drivers/mmc/host/sdhci-brcmstb.c | 275 ++++ > drivers/pinctrl/bcm/Kconfig | 9 + > drivers/pinctrl/bcm/Makefile | 1 + > drivers/pinctrl/bcm/pinctrl-bcm2712.c | 1247 +++++++++++++++++ > 11 files changed, 2918 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml > create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi > create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712.dtsi > create mode 100644 drivers/pinctrl/bcm/pinctrl-bcm2712.c >
On 4/13/2024 3:14 PM, Andrea della Porta wrote: > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > --- > .../pinctrl/brcm,bcm2712-pinctrl.yaml | 99 +++++++++++++++++++ > 1 file changed, 99 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml > new file mode 100644 > index 000000000000..2908dfe99f3e > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml > @@ -0,0 +1,99 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm2712-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom BCM2712 pin controller This is not strictly speaking BCM2712 specific, the pin controller you describe is a Broadcom STB product line pin controller. Please describe it as such as and make BCM2712 a specific instance of the chip using that pin controller, see more comments on patch #4.
Andrea, On 4/13/2024 3:14 PM, Andrea della Porta wrote: > Hi, > > This patchset adds support for the SDHCI controller on Broadcom BCM2712 > SoC in order to make it possible to boot (particularly) Raspberry Pi 5 > from SD card. This work is heavily based on downstream contributions. > > Patch #1 and 2: introduce the dt binding definitions for, respectively, > the new pin cfg/mux controller and the SD host controller as a preparatory > step for the upcoming dts. > > Patch #3: add a somewhat reasonable (*almost* bare-minimum) dts to be used > to boot Rpi5 boards. Since till now there was no support at all for any > 2712 based chipset, both the SoC and board dts plus definitions for the > new Pin and SD host controller have been added. > > Patch #4: the driver supporting the pin controller. Based on [1] and > successive fix commits. > > Patch #5: add SDHCI support. Based on [2] and the next 2 fix commits. > Drop the SD Express implementation for now, that will be added by patch > #6. > > Patch #6: this patch offers SD Express support and can be considered totally > optional. The callback plumbing is slightly different w.r.t. the downstream > approach (see [3]), as explained in the patch comment. Not sure what is the best, > any comment is highly appreciated. > > Tested succesfully on Raspberry Pi 5 using an SDxC card as the boot device. You really need to improve upon the quality of the patches you are submitting, they are not passing checkpatch.pl for coding style and this is seriously concerning. It sounds like these patches are sent out just to tick a box that an effort has been made to upstream them, that is not how upstream works, so please send some quality time on getting them in tip top shape. Thank you.
On 4/13/2024 3:14 PM, Andrea della Porta wrote:
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
No commit message given the amount of lines changed?
Please split this patch into multiple series that add basic 2712 support
to the mainline kernel.
On Sun, 14 Apr 2024 00:14:22 +0200, Andrea della Porta wrote: > Hi, > > This patchset adds support for the SDHCI controller on Broadcom BCM2712 > SoC in order to make it possible to boot (particularly) Raspberry Pi 5 > from SD card. This work is heavily based on downstream contributions. > > Patch #1 and 2: introduce the dt binding definitions for, respectively, > the new pin cfg/mux controller and the SD host controller as a preparatory > step for the upcoming dts. > > Patch #3: add a somewhat reasonable (*almost* bare-minimum) dts to be used > to boot Rpi5 boards. Since till now there was no support at all for any > 2712 based chipset, both the SoC and board dts plus definitions for the > new Pin and SD host controller have been added. > > Patch #4: the driver supporting the pin controller. Based on [1] and > successive fix commits. > > Patch #5: add SDHCI support. Based on [2] and the next 2 fix commits. > Drop the SD Express implementation for now, that will be added by patch > #6. > > Patch #6: this patch offers SD Express support and can be considered totally > optional. The callback plumbing is slightly different w.r.t. the downstream > approach (see [3]), as explained in the patch comment. Not sure what is the best, > any comment is highly appreciated. > > Tested succesfully on Raspberry Pi 5 using an SDxC card as the boot device. > > Still untested: > - SD Express due to the lack of an Express capable card. > Also, it will need PCIe support first. > - card detection pin, since the sd was the booting and root fs device. > > Many thanks, > Andrea > > Links: > [1] - https://github.com/raspberrypi/linux/commit/d9b655314a826724538867bf9b6c229d04c25d84 > [2] - https://github.com/raspberrypi/linux/commit/e3aa070496e840e72a4dc384718690ea4125fa6a > [3] - https://github.com/raspberrypi/linux/commit/eb1df34db2a9a5b752eba40ee298c4ae87e26e87 > > Andrea della Porta (6): > dt-bindings: pinctrl: Add support for BCM2712 pin controller > dt-bindings: mmc: Add support for BCM2712 SD host controller > arm64: dts: broadcom: Add support for BCM2712 > pinctrl: bcm: Add pinconf/pinmux controller driver for BCM2712 > mmc: sdhci-brcmstb: Add BCM2712 support > mmc: sdhci-brcmstb: Add BCM2712 SD Express support > > .../bindings/mmc/brcm,sdhci-brcmstb.yaml | 51 +- > .../pinctrl/brcm,bcm2712-pinctrl.yaml | 99 ++ > arch/arm64/boot/dts/broadcom/Makefile | 1 + > .../boot/dts/broadcom/bcm2712-rpi-5-b.dts | 313 +++++ > arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi | 81 ++ > arch/arm64/boot/dts/broadcom/bcm2712.dtsi | 841 +++++++++++ > drivers/mmc/host/Kconfig | 1 + > drivers/mmc/host/sdhci-brcmstb.c | 275 ++++ > drivers/pinctrl/bcm/Kconfig | 9 + > drivers/pinctrl/bcm/Makefile | 1 + > drivers/pinctrl/bcm/pinctrl-bcm2712.c | 1247 +++++++++++++++++ > 11 files changed, 2918 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml > create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts > create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712-rpi.dtsi > create mode 100644 arch/arm64/boot/dts/broadcom/bcm2712.dtsi > create mode 100644 drivers/pinctrl/bcm/pinctrl-bcm2712.c > > -- > 2.35.3 > > > My bot found new DTB warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade New warnings running 'make CHECK_DTBS=y broadcom/bcm2712-rpi-5-b.dtb' for cover.1713036964.git.andrea.porta@suse.com: arch/arm64/boot/dts/broadcom/bcm2712.dtsi:554.26-565.5: Warning (interrupt_provider): /soc/gpio@7d517c00: '#interrupt-cells' found, but node is not an interrupt provider also defined at arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts:201.10-206.3 also defined at arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts:259.10-288.3 arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: Warning (interrupt_map): Failed prerequisite 'interrupt_provider' arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /: failed to match any schema with compatible: ['raspberrypi,5-model-b', 'brcm,bcm2712'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /: failed to match any schema with compatible: ['raspberrypi,5-model-b', 'brcm,bcm2712'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: thermal-zones: cpu-thermal:trips:phandle: [[43]] is not of type 'object' from schema $id: http://devicetree.org/schemas/thermal/thermal-zones.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: soc: firmware: {'compatible': ['raspberrypi,bcm2835-firmware', 'simple-mfd'], '#address-cells': [[1]], '#size-cells': [[1]], 'mboxes': [[15]], 'dma-ranges': True, 'phandle': [[16]], 'clocks': {'compatible': ['raspberrypi,firmware-clocks'], '#clock-cells': [[1]], 'phandle': [[95]]}, 'reset': {'compatible': ['raspberrypi,firmware-reset'], '#reset-cells': [[1]], 'phandle': [[96]]}} should not be valid under {'type': 'object'} from schema $id: http://devicetree.org/schemas/simple-bus.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: soc: power: {'compatible': ['raspberrypi,bcm2835-power'], 'firmware': [[16]], '#power-domain-cells': [[1]], 'phandle': [[97]]} should not be valid under {'type': 'object'} from schema $id: http://devicetree.org/schemas/simple-bus.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: soc: fixedregulator_3v3: {'compatible': ['regulator-fixed'], 'regulator-always-on': True, 'regulator-max-microvolt': [[3300000]], 'regulator-min-microvolt': [[3300000]], 'regulator-name': ['3v3'], 'phandle': [[98]]} should not be valid under {'type': 'object'} from schema $id: http://devicetree.org/schemas/simple-bus.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: soc: fixedregulator_5v0: {'compatible': ['regulator-fixed'], 'regulator-always-on': True, 'regulator-max-microvolt': [[5000000]], 'regulator-min-microvolt': [[5000000]], 'regulator-name': ['5v0'], 'phandle': [[99]]} should not be valid under {'type': 'object'} from schema $id: http://devicetree.org/schemas/simple-bus.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/timer@7c003000: failed to match any schema with compatible: ['brcm,bcm2835-system-timer'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/local_intc@7cd00000: failed to match any schema with compatible: ['brcm,bcm2836-l1-intc'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/i2s@7d003000: failed to match any schema with compatible: ['brcm,bcm2835-i2s'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/spi@7d004000: failed to match any schema with compatible: ['brcm,bcm2835-spi'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/spi@7d004000/spidev@0: failed to match any schema with compatible: ['spidev'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/spi@7d004600: failed to match any schema with compatible: ['brcm,bcm2835-spi'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/spi@7d004800: failed to match any schema with compatible: ['brcm,bcm2835-spi'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/spi@7d004a00: failed to match any schema with compatible: ['brcm,bcm2835-spi'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/spi@7d004c00: failed to match any schema with compatible: ['brcm,bcm2835-spi'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: pwm@7d00c000: 'assigned-clocks' is a dependency of 'assigned-clock-rates' from schema $id: http://devicetree.org/schemas/clock/clock.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: pwm@7d00c800: 'assigned-clocks' is a dependency of 'assigned-clock-rates' from schema $id: http://devicetree.org/schemas/clock/clock.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/cprman@7d202000: failed to match any schema with compatible: ['brcm,bcm2711-cprman'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: intc@7d503000: $nodename:0: 'intc@7d503000' does not match '^interrupt-controller(@[0-9a-f,]+)*$' from schema $id: http://devicetree.org/schemas/interrupt-controller/brcm,l2-intc.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: intc@7d508380: $nodename:0: 'intc@7d508380' does not match '^interrupt-controller(@[0-9a-f,]+)*$' from schema $id: http://devicetree.org/schemas/interrupt-controller/brcm,l2-intc.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: intc@7d508400: $nodename:0: 'intc@7d508400' does not match '^interrupt-controller(@[0-9a-f,]+)*$' from schema $id: http://devicetree.org/schemas/interrupt-controller/brcm,l2-intc.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: gpio@7d508500: compatible:0: 'brcm,brcmstb-gpio' is not one of ['brcm,bcm7445-gpio'] from schema $id: http://devicetree.org/schemas/gpio/brcm,brcmstb-gpio.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: gpio@7d508500: compatible: ['brcm,brcmstb-gpio'] is too short from schema $id: http://devicetree.org/schemas/gpio/brcm,brcmstb-gpio.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: gpio@7d508500: 'brcm,gpio-direct', 'gpio-line-names' do not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/gpio/brcm,brcmstb-gpio.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: intc@7d517000: $nodename:0: 'intc@7d517000' does not match '^interrupt-controller(@[0-9a-f,]+)*$' from schema $id: http://devicetree.org/schemas/interrupt-controller/brcm,l2-intc.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: pwm@7d517a80: #pwm-cells:0:0: 2 was expected from schema $id: http://devicetree.org/schemas/pwm/brcm,bcm7038-pwm.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: intc@7d517ac0: $nodename:0: 'intc@7d517ac0' does not match '^interrupt-controller(@[0-9a-f,]+)*$' from schema $id: http://devicetree.org/schemas/interrupt-controller/brcm,l2-intc.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: intc@7d517b00: $nodename:0: 'intc@7d517b00' does not match '^interrupt-controller(@[0-9a-f,]+)*$' from schema $id: http://devicetree.org/schemas/interrupt-controller/brcm,l2-intc.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: gpio@7d517c00: compatible:0: 'brcm,brcmstb-gpio' is not one of ['brcm,bcm7445-gpio'] from schema $id: http://devicetree.org/schemas/gpio/brcm,brcmstb-gpio.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: gpio@7d517c00: compatible: ['brcm,brcmstb-gpio'] is too short from schema $id: http://devicetree.org/schemas/gpio/brcm,brcmstb-gpio.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: gpio@7d517c00: 'brcm,gpio-direct', 'gpio-line-names' do not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/gpio/brcm,brcmstb-gpio.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/avs-monitor@7d542000: failed to match any schema with compatible: ['brcm,bcm2711-avs-monitor', 'syscon', 'simple-mfd'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: firmware: '#address-cells', '#size-cells', 'dma-ranges' do not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: /soc/power: failed to match any schema with compatible: ['raspberrypi,bcm2835-power'] arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: l3-cache: 'cache-unified' is a dependency of 'cache-size' from schema $id: http://devicetree.org/schemas/cache.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: l3-cache: 'cache-unified' is a dependency of 'cache-sets' from schema $id: http://devicetree.org/schemas/cache.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: l3-cache: 'cache-unified' is a dependency of 'cache-line-size' from schema $id: http://devicetree.org/schemas/cache.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: l3-cache: 'cache-unified' is a required property from schema $id: http://devicetree.org/schemas/cache.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: l3-cache: Unevaluated properties are not allowed ('cache-level', 'cache-line-size', 'cache-sets', 'cache-size' were unexpected) from schema $id: http://devicetree.org/schemas/cache.yaml# arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtb: pwr_button: 'pwr' does not match any of the regexes: '^(button|event|key|switch|(button|event|key|switch)-[a-z0-9-]+|[a-z0-9-]+-(button|event|key|switch))$', 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/input/gpio-keys.yaml#
On Sun, Apr 14, 2024 at 5:45 PM Florian Fainelli <florian.fainelli@broadcom.com> wrote: > On 4/13/2024 3:14 PM, Andrea della Porta wrote: > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm2712-pinctrl.yaml (...) > > +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm2712-pinctrl.yaml# (...) > > +title: Broadcom BCM2712 pin controller > > This is not strictly speaking BCM2712 specific, the pin controller you > describe is a Broadcom STB product line pin controller. > > Please describe it as such as and make BCM2712 a specific instance of > the chip using that pin controller, see more comments on patch #4. So also the name of the bindings namespace should not be this one controller IMO but the name of the family, bcm-stb-pinctrl.yaml or so. Yours, Linus Walleij
On 12:07 Sun 14 Apr , Stefan Wahren wrote: > Hi Andrea, > > Am 14.04.24 um 00:14 schrieb Andrea della Porta: > > Hi, > > > > This patchset adds support for the SDHCI controller on Broadcom BCM2712 > > SoC in order to make it possible to boot (particularly) Raspberry Pi 5 > > from SD card. This work is heavily based on downstream contributions. > since your goal is minimal Raspberry Pi 5 support, i suggest to use this > as the subject for this patch. > > Patch #1 and 2: introduce the dt binding definitions for, respectively, > > the new pin cfg/mux controller and the SD host controller as a preparatory > > step for the upcoming dts. > > > > Patch #3: add a somewhat reasonable (*almost* bare-minimum) dts to be used > > to boot Rpi5 boards. Since till now there was no support at all for any > > 2712 based chipset, both the SoC and board dts plus definitions for the > > new Pin and SD host controller have been added. > The patch still seems to contain a lot unnecessary stuff (Wifi, BT, > SPI), please try to remove as much as possible for the minimal support > (just boot via debug UART & SD card) in order to make review easier. Btw > this patch must be after pinctrl & SDHCI support. > > Patch #4: the driver supporting the pin controller. Based on [1] and > > successive fix commits. > > > > Patch #5: add SDHCI support. Based on [2] and the next 2 fix commits. > > Drop the SD Express implementation for now, that will be added by patch > > #6. > > > > Patch #6: this patch offers SD Express support and can be considered totally > > optional. The callback plumbing is slightly different w.r.t. the downstream > > approach (see [3]), as explained in the patch comment. Not sure what is the best, > > any comment is highly appreciated. > I don't think this should be necessary for minimal Raspberry Pi 5 > support. Maybe this should be addressed later. Thanks for all the feedback. Just a quick note to let you know that I'm working on V2 patchset that will fix all coding-style and dts/binding issues. The new patchset will be significantly smaller and I managed to remove everything that is not strictly needed in order to be able to boot an rpi5 from sd card. Best regards, Andrea