| Message ID | 20240125013858.3986-4-semen.protsenko@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | arm64: exynos: Enable SPI for Exynos850 | expand |
On 1/25/24 01:38, Sam Protsenko wrote: > diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi > index 618bc674896e..ca257da74b50 100644 > --- a/arch/arm64/boot/dts/exynos/exynos850.dtsi > +++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi > @@ -738,6 +738,24 @@ usi_spi_0: usi@139400c0 { > <&cmu_peri CLK_GOUT_SPI0_IPCLK>; > clock-names = "pclk", "ipclk"; > status = "disabled"; > + > + spi_0: spi@13940000 { > + compatible = "samsung,exynos850-spi"; > + reg = <0x13940000 0x30>; > + interrupts = <GIC_SPI 221 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-0 = <&spi0_pins>; > + pinctrl-names = "default"; > + clocks = <&cmu_peri CLK_GOUT_SPI0_IPCLK>, > + <&cmu_peri CLK_GOUT_SPI0_PCLK>; > + clock-names = "spi_busclk0", "spi"; > + samsung,spi-src-clk = <0>; this optional property > + dmas = <&pdma0 5>, <&pdma0 4>; > + dma-names = "tx", "rx"; > + num-cs = <1>; and this one, are already defaults in the driver. Shall you remove them? > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; > + }; > };
On Mon, Jan 29, 2024 at 11:51 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > On 1/25/24 01:38, Sam Protsenko wrote: > > diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi > > index 618bc674896e..ca257da74b50 100644 > > --- a/arch/arm64/boot/dts/exynos/exynos850.dtsi > > +++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi > > @@ -738,6 +738,24 @@ usi_spi_0: usi@139400c0 { > > <&cmu_peri CLK_GOUT_SPI0_IPCLK>; > > clock-names = "pclk", "ipclk"; > > status = "disabled"; > > + > > + spi_0: spi@13940000 { > > + compatible = "samsung,exynos850-spi"; > > + reg = <0x13940000 0x30>; > > + interrupts = <GIC_SPI 221 IRQ_TYPE_LEVEL_HIGH>; > > + pinctrl-0 = <&spi0_pins>; > > + pinctrl-names = "default"; > > + clocks = <&cmu_peri CLK_GOUT_SPI0_IPCLK>, > > + <&cmu_peri CLK_GOUT_SPI0_PCLK>; > > + clock-names = "spi_busclk0", "spi"; > > + samsung,spi-src-clk = <0>; > > this optional property > The reason this property is provided here despite being optional, is to avoid corresponding dev_warn() message from spi-s3c64xx.c driver: if (of_property_read_u32(dev->of_node, "samsung,spi-src-clk", &temp)) { dev_warn(dev, "spi bus clock parent not specified, using clock at index 0 as parent\n"); The same usage (samsung,spi-src-clk = <0>) can be encountered in multiple other Exynos dts in arch/arm/ and arch/arm64/, and it's also used in bindings example. Probably for the same reason explained above. Even if dev_warn() is removed in the driver, I guess the older kernels will still print it if spi-src-clk is omitted. So I'd like to keep it here. > > + dmas = <&pdma0 5>, <&pdma0 4>; > > + dma-names = "tx", "rx"; > > + num-cs = <1>; > > and this one, are already defaults in the driver. Shall you remove them? > For exactly the same reasoning as stated above, I'd like to keep this here to keep dmesg clean and tidy. Otherwise it prints this warning: if (of_property_read_u32(dev->of_node, "num-cs", &temp)) { dev_warn(dev, "number of chip select lines not specified, assuming 1 chip select line\n"); And even if the warning is removed in the driver, older kernels will still print it. > > + #address-cells = <1>; > > + #size-cells = <0>; > > + status = "disabled"; > > + }; > > };
On 1/29/24 19:39, Sam Protsenko wrote: >>> + samsung,spi-src-clk = <0>; >> this optional property >> > The reason this property is provided here despite being optional, is > to avoid corresponding dev_warn() message from spi-s3c64xx.c driver: > > if (of_property_read_u32(dev->of_node, "samsung,spi-src-clk", &temp)) { > dev_warn(dev, "spi bus clock parent not specified, using > clock at index 0 as parent\n"); > > The same usage (samsung,spi-src-clk = <0>) can be encountered in > multiple other Exynos dts in arch/arm/ and arch/arm64/, and it's also > used in bindings example. Probably for the same reason explained > above. Even if dev_warn() is removed in the driver, I guess the older > kernels will still print it if spi-src-clk is omitted. So I'd like to > keep it here. Yeah, I know. I proposed a patch switching to dev_dbg. If it's so annoying and implies adding superfluous properties to DT, maybe it is worth to add a fixes tag to the dev_dbg patch and backport it to stable kernels? Your patch looks fine. I guess the vendor specific properties shall be last if you keep them, see: https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node If you remove the vendor properties or reorder them, one can add: Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
On 25/01/2024 02:38, Sam Protsenko wrote: > Some USI blocks can be configured as SPI controllers. Add corresponding > SPI nodes to Exynos850 SoC device tree. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > Changes in v2: > - Sorted pinctrl properties properly > > arch/arm64/boot/dts/exynos/exynos850.dtsi | 54 +++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi > index 618bc674896e..ca257da74b50 100644 > --- a/arch/arm64/boot/dts/exynos/exynos850.dtsi > +++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi > @@ -738,6 +738,24 @@ usi_spi_0: usi@139400c0 { > <&cmu_peri CLK_GOUT_SPI0_IPCLK>; > clock-names = "pclk", "ipclk"; > status = "disabled"; > + > + spi_0: spi@13940000 { > + compatible = "samsung,exynos850-spi"; > + reg = <0x13940000 0x30>; > + interrupts = <GIC_SPI 221 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-0 = <&spi0_pins>; > + pinctrl-names = "default"; > + clocks = <&cmu_peri CLK_GOUT_SPI0_IPCLK>, > + <&cmu_peri CLK_GOUT_SPI0_PCLK>; > + clock-names = "spi_busclk0", "spi"; > + samsung,spi-src-clk = <0>; > + dmas = <&pdma0 5>, <&pdma0 4>; > + dma-names = "tx", "rx"; > + num-cs = <1>; For the future: please keep properties sorted by name, so clocks+name, dmas+name, interrupts, pinctrl+name, more-or-less matching DTS coding style. address/size cells can go to the end. Best regards, Krzysztof
On Wed, 24 Jan 2024 19:38:58 -0600, Sam Protsenko wrote: > Some USI blocks can be configured as SPI controllers. Add corresponding > SPI nodes to Exynos850 SoC device tree. > > Applied, thanks! [3/3] arm64: dts: exynos: Add SPI nodes for Exynos850 https://git.kernel.org/krzk/linux/c/98473b0d78caa5502b7eee05553ee168f0b0b424 Best regards,
On 01/02/2024 11:36, Krzysztof Kozlowski wrote: > > On Wed, 24 Jan 2024 19:38:58 -0600, Sam Protsenko wrote: >> Some USI blocks can be configured as SPI controllers. Add corresponding >> SPI nodes to Exynos850 SoC device tree. >> >> > > Applied, thanks! > > [3/3] arm64: dts: exynos: Add SPI nodes for Exynos850 > https://git.kernel.org/krzk/linux/c/98473b0d78caa5502b7eee05553ee168f0b0b424 And dropped. You did not test it. For some time, all Samsung SoCs and its variants are expected not to introduce any new `dtbs_check W=1` warnings. Several platforms, like all ARM64 Samsung SoCs, have already zero warnings, thus for such platforms it is extra easy for the submitter to validate DTS before posting a patch. The patch briefly looks like it is not conforming to this rule. Please confirm that you tested your patch and it does not introduce any new warnings (linux-next is decisive here). Best regards, Krzysztof
On Thu, Feb 1, 2024 at 4:31 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 25/01/2024 02:38, Sam Protsenko wrote: > > Some USI blocks can be configured as SPI controllers. Add corresponding > > SPI nodes to Exynos850 SoC device tree. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > Changes in v2: > > - Sorted pinctrl properties properly > > > > arch/arm64/boot/dts/exynos/exynos850.dtsi | 54 +++++++++++++++++++++++ > > 1 file changed, 54 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi > > index 618bc674896e..ca257da74b50 100644 > > --- a/arch/arm64/boot/dts/exynos/exynos850.dtsi > > +++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi > > @@ -738,6 +738,24 @@ usi_spi_0: usi@139400c0 { > > <&cmu_peri CLK_GOUT_SPI0_IPCLK>; > > clock-names = "pclk", "ipclk"; > > status = "disabled"; > > + > > + spi_0: spi@13940000 { > > + compatible = "samsung,exynos850-spi"; > > + reg = <0x13940000 0x30>; > > + interrupts = <GIC_SPI 221 IRQ_TYPE_LEVEL_HIGH>; > > + pinctrl-0 = <&spi0_pins>; > > + pinctrl-names = "default"; > > + clocks = <&cmu_peri CLK_GOUT_SPI0_IPCLK>, > > + <&cmu_peri CLK_GOUT_SPI0_PCLK>; > > + clock-names = "spi_busclk0", "spi"; > > + samsung,spi-src-clk = <0>; > > + dmas = <&pdma0 5>, <&pdma0 4>; > > + dma-names = "tx", "rx"; > > + num-cs = <1>; > > For the future: please keep properties sorted by name, so clocks+name, > dmas+name, interrupts, pinctrl+name, more-or-less matching DTS coding > style. address/size cells can go to the end. > Noted, thanks! So IIUC, basically follow the order of properties described at [1], but keep the standard/common properties block sorted, and then keep vendor properties sorted, right? [1] Documentation/devicetree/bindings/dts-coding-style.rst > Best regards, > Krzysztof >
On Thu, Feb 1, 2024 at 5:56 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 01/02/2024 11:36, Krzysztof Kozlowski wrote: > > > > On Wed, 24 Jan 2024 19:38:58 -0600, Sam Protsenko wrote: > >> Some USI blocks can be configured as SPI controllers. Add corresponding > >> SPI nodes to Exynos850 SoC device tree. > >> > >> > > > > Applied, thanks! > > > > [3/3] arm64: dts: exynos: Add SPI nodes for Exynos850 > > https://git.kernel.org/krzk/linux/c/98473b0d78caa5502b7eee05553ee168f0b0b424 > > And dropped. You did not test it. > Right. Forgot to re-test it after re-shuffling the clocks. Sorry for the hustle, I'll send v3 shortly. > For some time, all Samsung SoCs and its variants are expected not to > introduce any new `dtbs_check W=1` warnings. Several platforms, like all > ARM64 Samsung SoCs, have already zero warnings, thus for such platforms > it is extra easy for the submitter to validate DTS before posting a > patch. The patch briefly looks like it is not conforming to this rule. > Please confirm that you tested your patch and it does not introduce any > new warnings (linux-next is decisive here). > > Best regards, > Krzysztof >
diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi index 618bc674896e..ca257da74b50 100644 --- a/arch/arm64/boot/dts/exynos/exynos850.dtsi +++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi @@ -738,6 +738,24 @@ usi_spi_0: usi@139400c0 { <&cmu_peri CLK_GOUT_SPI0_IPCLK>; clock-names = "pclk", "ipclk"; status = "disabled"; + + spi_0: spi@13940000 { + compatible = "samsung,exynos850-spi"; + reg = <0x13940000 0x30>; + interrupts = <GIC_SPI 221 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-0 = <&spi0_pins>; + pinctrl-names = "default"; + clocks = <&cmu_peri CLK_GOUT_SPI0_IPCLK>, + <&cmu_peri CLK_GOUT_SPI0_PCLK>; + clock-names = "spi_busclk0", "spi"; + samsung,spi-src-clk = <0>; + dmas = <&pdma0 5>, <&pdma0 4>; + dma-names = "tx", "rx"; + num-cs = <1>; + #address-cells = <1>; + #size-cells = <0>; + status = "disabled"; + }; }; usi_cmgp0: usi@11d000c0 { @@ -779,6 +797,24 @@ serial_1: serial@11d00000 { clock-names = "uart", "clk_uart_baud0"; status = "disabled"; }; + + spi_1: spi@11d00000 { + compatible = "samsung,exynos850-spi"; + reg = <0x11d00000 0x30>; + interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-0 = <&spi1_pins>; + pinctrl-names = "default"; + clocks = <&cmu_cmgp CLK_GOUT_CMGP_USI0_IPCLK>, + <&cmu_cmgp CLK_GOUT_CMGP_USI0_PCLK>; + clock-names = "spi_busclk0", "spi"; + samsung,spi-src-clk = <0>; + dmas = <&pdma0 12>, <&pdma0 13>; + dma-names = "tx", "rx"; + num-cs = <1>; + #address-cells = <1>; + #size-cells = <0>; + status = "disabled"; + }; }; usi_cmgp1: usi@11d200c0 { @@ -820,6 +856,24 @@ serial_2: serial@11d20000 { clock-names = "uart", "clk_uart_baud0"; status = "disabled"; }; + + spi_2: spi@11d20000 { + compatible = "samsung,exynos850-spi"; + reg = <0x11d20000 0x30>; + interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-0 = <&spi2_pins>; + pinctrl-names = "default"; + clocks = <&cmu_cmgp CLK_GOUT_CMGP_USI1_IPCLK>, + <&cmu_cmgp CLK_GOUT_CMGP_USI1_PCLK>; + clock-names = "spi_busclk0", "spi"; + samsung,spi-src-clk = <0>; + dmas = <&pdma0 14>, <&pdma0 15>; + dma-names = "tx", "rx"; + num-cs = <1>; + #address-cells = <1>; + #size-cells = <0>; + status = "disabled"; + }; }; }; };
Some USI blocks can be configured as SPI controllers. Add corresponding SPI nodes to Exynos850 SoC device tree. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- Changes in v2: - Sorted pinctrl properties properly arch/arm64/boot/dts/exynos/exynos850.dtsi | 54 +++++++++++++++++++++++ 1 file changed, 54 insertions(+)