Re: [PATCH v5 18/20] arm64: dts: exynos: google: Add initial Google gs101 SoC support

[Peter Griffin <peter.griffin@xxxxxxxxxx>]

Hi Sam,

On Sat, 2 Dec 2023 at 01:54, Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote:
>
> On Fri, Dec 1, 2023 at 10:11 AM Peter Griffin <peter.griffin@xxxxxxxxxx> wrote:
> >
> > Google gs101 SoC is ARMv8 mobile SoC found in the Pixel 6,
> > (oriole) Pixel 6a (bluejay) and Pixel 6 pro (raven) mobile
> > phones. It features:
> > * 4xA55 little cluster
> > * 2xA76 Mid cluster
> > * 2xX1 Big cluster
> >
> > This commit adds the basic device tree for gs101 (SoC).
> > Further platform support will be added over time.

[cut]

> > +       spi0_cs_func: spi0-cs-func-pins {
> > +               samsung,pins = "gpp20-3";
> > +               samsung,pin-function = <GS101_PIN_FUNC_3>;
> > +               samsung,pin-pud = <GS101_PIN_PULL_NONE>;
> > +               samsung,pin-drv = <GS101_PIN_DRV_2_5_MA>;
> > +       };
> > +};
> > +
>
> Nitpick: this empty line is not needed.

Ok will fix

[cut]

> > +
> > +       aliases {
> > +               pinctrl0 = &pinctrl_gpio_alive;
> > +               pinctrl1 = &pinctrl_far_alive;
> > +               pinctrl2 = &pinctrl_gsacore;
> > +               pinctrl3 = &pinctrl_gsactrl;
> > +               pinctrl4 = &pinctrl_peric0;
> > +               pinctrl5 = &pinctrl_peric1;
> > +               pinctrl6 = &pinctrl_hsi1;
> > +               pinctrl7 = &pinctrl_hsi2;
> > +               serial0 = &serial_0;
>
> Please check commit f4324583cd4d ("arm64: dts: exynos: move aliases to
> board in Exynos850"). At least for Exynos850 the serial alias was
> moved to the board dts by Krzysztof.

Ok will fix

>
> > +       };
> > +
> > +       pmu-0 {
> > +               compatible = "arm,cortex-a55-pmu";
> > +               interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH &ppi_cluster0>;
> > +       };
> > +
> > +       pmu-1 {
> > +               compatible = "arm,cortex-a76-pmu";
> > +               interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH &ppi_cluster1>;
> > +       };
> > +
> > +       pmu-2 {
> > +               compatible = "arm,cortex-x1-pmu";
> > +               interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH &ppi_cluster2>;
> > +       };
> > +
> > +       pmu-3 {
> > +               compatible = "arm,dsu-pmu";
> > +               interrupts = <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH 0>;
> > +               cpus = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>,
> > +                      <&cpu4>, <&cpu5>, <&cpu6>, <&cpu7>;
> > +       };
> > +
> > +       /* TODO replace with CCF clock */
> > +       dummy_clk: oscillator {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <12345>;
> > +               clock-output-names = "pclk";
> > +       };
>
> Don't you already have real USI/UART clocks implemented in your clock driver?

No, not yet, hence the dummy clock.

[cut]

> > +
> > +               usi_uart: usi@10a000c0 {
> > +                       compatible = "google,gs101-usi",
>
> I can't see this compatible in USI driver. Does it make sense to add it there?

It is not required at the moment, as it is compatible with
samsung,exynos850-usi. I don't want to keep adding more patches to
this series, and then having an endless cycle of nits.

>
> > +                                    "samsung,exynos850-usi";
> > +                       reg = <0x10a000c0 0x20>;
> > +                       samsung,sysreg = <&sysreg_peric0 0x1020>;
> > +                       samsung,mode = <USI_V2_UART>;
> > +                       #address-cells = <1>;
> > +                       #size-cells = <1>;
> > +                       ranges;
> > +                       clocks = <&dummy_clk>, <&dummy_clk>;
>
> The same concern as above. I think I saw those clocks already
> implemented in gs101 clock driver.

No, these clocks have not been implemented yet, hence the dummy clock.
There is no support for cmu_peric0 bank yet in the clock driver.

>
> > +                       clock-names = "pclk", "ipclk";
> > +                       status = "disabled";
> > +
> > +                       serial_0: serial@10a00000 {
> > +                               compatible = "google,gs101-uart";
> > +                               reg = <0x10a00000 0xc0>;
> > +                               reg-io-width = <4>;
> > +                               samsung,uart-fifosize = <256>;
> > +                               interrupts = <GIC_SPI 634
> > +                                             IRQ_TYPE_LEVEL_HIGH 0>;
> > +                               clocks = <&dummy_clk 0>, <&dummy_clk 0>;
>
> Ditto.

See above

>
> > +                               clock-names = "uart", "clk_uart_baud0";
> > +                               status = "disabled";
> > +                       };
> > +               };
> > +
> > +               pinctrl_peric1: pinctrl@10c40000 {
> > +                       compatible = "google,gs101-pinctrl";
> > +                       reg = <0x10C40000 0x00001000>;
> > +                       interrupts = <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH 0>;
> > +               };
> > +
> > +               sysreg_peric1: syscon@10c20000 {
> > +                       compatible = "google,gs101-peric1-sysreg", "syscon";
> > +                       reg = <0x10C20000 0x10000>;
> > +               };
> > +
> > +               pinctrl_hsi1: pinctrl@11840000 {
> > +                       compatible = "google,gs101-pinctrl";
> > +                       reg = <0x11840000 0x00001000>;
> > +                       interrupts = <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH 0>;
> > +               };
> > +
> > +               pinctrl_hsi2: pinctrl@14440000 {
> > +                       compatible = "google,gs101-pinctrl";
> > +                       reg = <0x14440000 0x00001000>;
> > +                       interrupts = <GIC_SPI 503 IRQ_TYPE_LEVEL_HIGH 0>;
> > +               };
> > +
> > +               cmu_apm: clock-controller@17400000 {
> > +                       compatible = "google,gs101-cmu-apm";
> > +                       reg = <0x17400000 0x8000>;
> > +                       #clock-cells = <1>;
> > +
> > +                       clocks = <&ext_24_5m>;
> > +                       clock-names = "oscclk";
>
> Doesn't CMU_APM take any clocks from CMU_TOP?

No it doesn't.

>
> > +               };
> > +
> > +               sysreg_apm: syscon@174204e0 {
> > +                       compatible = "google,gs101-apm-sysreg", "syscon";
> > +                       reg = <0x174204e0 0x1000>;
> > +               };
> > +
> > +               pmu_system_controller: system-controller@17460000 {
> > +                       compatible = "google,gs101-pmu", "syscon";
> > +                       reg = <0x17460000 0x10000>;
> > +               };
>
> Just a suggestion: it might be relatively simple to add syscon-reboot
> node in pmu_system_controller, and it might just work. One more
> feature for free! :)

Thanks for the suggestion. I tried that previously and it is not
included here deliberately because it relies on more than that to be
functional. Although the register offsets are the same, the PMU
registers are protected from the kernel and are only write accessible
via SMC call on this platform. I have patches ready to send out as a
RFC for that once this initial series is merged and we can discuss
that then.

regards,

Peter.