[PATCH v3 0/6] Add SPI4 support for IPQ5424

[PATCH v3 0/6] Add SPI4 support for IPQ5424

[Manikanta Mylavarapu]

Add SPI4 node to the IPQ5424 device tree and update the relevant
bindings, GPIO pin mappings accordingly.

Changes in V3:
	- Rename SPI0 to SPI4 because SPI protocol runs on serial engine 4
	- Fixed all review comments from Konrad Dybico
	- Patch #1 to #4 added in V3 to rename SPI0 clocks, gpio pins to SPI4.  
	- Detailed change logs are added to the respective patches

V2 can be found at:
https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-1-quic_mmanikan@quicinc.com/

V1 can be found at:
https://lore.kernel.org/linux-arm-msm/20241122124505.1688436-1-quic_mmanikan@quicinc.com/

Manikanta Mylavarapu (6):
  dt-bindings: pinctrl: qcom: rename spi0 pins on IPQ5424
  dt-bindings: clock: qcom: gcc-ipq5424: add spi4 clocks
  pinctrl: qcom: ipq5424: rename spi0 pins
  clk: qcom: ipq5424: rename spi0 clocks
  arm64: dts: qcom: ipq5424: add spi4 node
  arm64: dts: qcom: ipq5424: configure spi4 node for rdp466

 .../bindings/pinctrl/qcom,ipq5424-tlmm.yaml   |  2 +-
 arch/arm64/boot/dts/qcom/ipq5424-rdp466.dts   | 43 +++++++++++++++++++
 arch/arm64/boot/dts/qcom/ipq5424.dtsi         | 11 +++++
 drivers/clk/qcom/gcc-ipq5424.c                | 20 ++++-----
 drivers/pinctrl/qcom/pinctrl-ipq5424.c        | 32 +++++++-------
 include/dt-bindings/clock/qcom,ipq5424-gcc.h  |  2 +
 6 files changed, 83 insertions(+), 27 deletions(-)


base-commit: 8155b4ef3466f0e289e8fcc9e6e62f3f4dceeac2
-- 
2.34.1

[PATCH v3 1/6] dt-bindings: pinctrl: qcom: rename spi0 pins on IPQ5424

[Manikanta Mylavarapu]

SPI protocol runs on serial engine 4. Hence rename
spi0 pins to spi4 like spi0_cs to spi4_cs etc.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
---
 .../devicetree/bindings/pinctrl/qcom,ipq5424-tlmm.yaml          | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,ipq5424-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,ipq5424-tlmm.yaml
index df284d3645c1..4e0be380caf6 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,ipq5424-tlmm.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,ipq5424-tlmm.yaml
@@ -79,7 +79,7 @@ $defs:
                 qdss_cti_trig_out_b0, qdss_cti_trig_in_b1, qdss_cti_trig_out_b1,
                 qdss_traceclk_a, qdss_tracectl_a, qdss_tracedata_a, qspi_clk,
                 qspi_cs, qspi_data, resout, rx0, rx1, rx2, sdc_clk, sdc_cmd,
-                sdc_data, spi0_cs, spi0_clk, spi0_miso, spi0_mosi, spi1, spi10,
+                sdc_data, spi4_cs, spi4_clk, spi4_miso, spi4_mosi, spi1, spi10,
                 spi11, tsens_max, uart0, uart1, wci_txd, wci_rxd, wsi_clk, wsi_data ]
 
     required:
-- 
2.34.1

[PATCH v3 2/6] dt-bindings: clock: qcom: gcc-ipq5424: add spi4 clocks

[Manikanta Mylavarapu]

SPI protocol runs on serial engine 4. Hence we need to
rename the spi0 clocks to spi4 clocks.

However, renaming spi0 to spi4 will result in the following
compilation error's.
drivers/clk/qcom/gcc-ipq5424.c:2865:3: error: ‘GCC_QUPV3_SPI0_CLK’
undeclared here
drivers/clk/qcom/gcc-ipq5424.c:2866:3: error: ‘GCC_QUPV3_SPI0_CLK_SRC’
undeclared here

To add spi4 clocks without compilation error's, do not
rename the spi0 clocks. Instead, duplicate the spi0 clock
macros and rename them to spi4.

After switching to spi4 clocks in the gcc-ipq5424 driver,
remove the spi0 clock macros.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
---
 include/dt-bindings/clock/qcom,ipq5424-gcc.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/dt-bindings/clock/qcom,ipq5424-gcc.h b/include/dt-bindings/clock/qcom,ipq5424-gcc.h
index 755ce7a71c7c..5dad45a8f614 100644
--- a/include/dt-bindings/clock/qcom,ipq5424-gcc.h
+++ b/include/dt-bindings/clock/qcom,ipq5424-gcc.h
@@ -123,6 +123,7 @@
 #define GCC_QUPV3_I2C0_CLK			113
 #define GCC_QUPV3_I2C1_CLK			114
 #define GCC_QUPV3_SPI0_CLK			115
+#define GCC_QUPV3_SPI4_CLK			GCC_QUPV3_SPI0_CLK
 #define GCC_QUPV3_SPI1_CLK			116
 #define GCC_QUPV3_UART0_CLK			117
 #define GCC_QUPV3_UART1_CLK			118
@@ -132,6 +133,7 @@
 #define GCC_QUPV3_I2C0_DIV_CLK_SRC              122
 #define GCC_QUPV3_I2C1_DIV_CLK_SRC              123
 #define GCC_QUPV3_SPI0_CLK_SRC			124
+#define GCC_QUPV3_SPI4_CLK_SRC			GCC_QUPV3_SPI0_CLK_SRC
 #define GCC_QUPV3_SPI1_CLK_SRC			125
 #define GCC_QUPV3_UART0_CLK_SRC			126
 #define GCC_QUPV3_UART1_CLK_SRC			127
-- 
2.34.1

[PATCH v3 3/6] pinctrl: qcom: ipq5424: rename spi0 pins

[Manikanta Mylavarapu]

SPI protocol runs on serial engine 4. Hence rename
spi0 pins to spi4 like spi0_cs to spi4_cs etc.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
---
 drivers/pinctrl/qcom/pinctrl-ipq5424.c | 32 +++++++++++++-------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-ipq5424.c b/drivers/pinctrl/qcom/pinctrl-ipq5424.c
index 0d610b076da3..05c45a115d7a 100644
--- a/drivers/pinctrl/qcom/pinctrl-ipq5424.c
+++ b/drivers/pinctrl/qcom/pinctrl-ipq5424.c
@@ -233,10 +233,10 @@ enum ipq5424_functions {
 	msm_mux_sdc_clk,
 	msm_mux_sdc_cmd,
 	msm_mux_sdc_data,
-	msm_mux_spi0_clk,
-	msm_mux_spi0_cs,
-	msm_mux_spi0_miso,
-	msm_mux_spi0_mosi,
+	msm_mux_spi4_clk,
+	msm_mux_spi4_cs,
+	msm_mux_spi4_miso,
+	msm_mux_spi4_mosi,
 	msm_mux_spi1,
 	msm_mux_spi10,
 	msm_mux_spi11,
@@ -300,7 +300,7 @@ static const char * const qspi_clk_groups[] = {
 	"gpio5",
 };
 
-static const char * const spi0_clk_groups[] = {
+static const char * const spi4_clk_groups[] = {
 	"gpio6",
 };
 
@@ -318,7 +318,7 @@ static const char * const qdss_tracedata_a_groups[] = {
 	"gpio38", "gpio39",
 };
 
-static const char * const spi0_cs_groups[] = {
+static const char * const spi4_cs_groups[] = {
 	"gpio7",
 };
 
@@ -326,7 +326,7 @@ static const char * const cri_trng1_groups[] = {
 	"gpio7",
 };
 
-static const char * const spi0_miso_groups[] = {
+static const char * const spi4_miso_groups[] = {
 	"gpio8",
 };
 
@@ -334,7 +334,7 @@ static const char * const cri_trng2_groups[] = {
 	"gpio8",
 };
 
-static const char * const spi0_mosi_groups[] = {
+static const char * const spi4_mosi_groups[] = {
 	"gpio9",
 };
 
@@ -695,10 +695,10 @@ static const struct pinfunction ipq5424_functions[] = {
 	MSM_PIN_FUNCTION(sdc_clk),
 	MSM_PIN_FUNCTION(sdc_cmd),
 	MSM_PIN_FUNCTION(sdc_data),
-	MSM_PIN_FUNCTION(spi0_clk),
-	MSM_PIN_FUNCTION(spi0_cs),
-	MSM_PIN_FUNCTION(spi0_miso),
-	MSM_PIN_FUNCTION(spi0_mosi),
+	MSM_PIN_FUNCTION(spi4_clk),
+	MSM_PIN_FUNCTION(spi4_cs),
+	MSM_PIN_FUNCTION(spi4_miso),
+	MSM_PIN_FUNCTION(spi4_mosi),
 	MSM_PIN_FUNCTION(spi1),
 	MSM_PIN_FUNCTION(spi10),
 	MSM_PIN_FUNCTION(spi11),
@@ -718,10 +718,10 @@ static const struct msm_pingroup ipq5424_groups[] = {
 	PINGROUP(3, sdc_data, qspi_data, pwm2, _, _, _, _, _, _),
 	PINGROUP(4, sdc_cmd, qspi_cs, _, _, _, _, _, _, _),
 	PINGROUP(5, sdc_clk, qspi_clk, _, _, _, _, _, _, _),
-	PINGROUP(6, spi0_clk, pwm1, _, cri_trng0, qdss_tracedata_a, _, _, _, _),
-	PINGROUP(7, spi0_cs, pwm1, _, cri_trng1, qdss_tracedata_a, _, _, _, _),
-	PINGROUP(8, spi0_miso, pwm1, wci_txd, wci_rxd, _, cri_trng2, qdss_tracedata_a, _, _),
-	PINGROUP(9, spi0_mosi, pwm1, _, cri_trng3, qdss_tracedata_a, _, _, _, _),
+	PINGROUP(6, spi4_clk, pwm1, _, cri_trng0, qdss_tracedata_a, _, _, _, _),
+	PINGROUP(7, spi4_cs, pwm1, _, cri_trng1, qdss_tracedata_a, _, _, _, _),
+	PINGROUP(8, spi4_miso, pwm1, wci_txd, wci_rxd, _, cri_trng2, qdss_tracedata_a, _, _),
+	PINGROUP(9, spi4_mosi, pwm1, _, cri_trng3, qdss_tracedata_a, _, _, _, _),
 	PINGROUP(10, uart0, pwm0, spi11, _, wci_txd, wci_rxd, _, qdss_tracedata_a, _),
 	PINGROUP(11, uart0, pwm0, spi1, _, wci_txd, wci_rxd, _, qdss_tracedata_a, _),
 	PINGROUP(12, uart0, pwm0, spi11, _, prng_rosc0, qdss_tracedata_a, _, _, _),
-- 
2.34.1

[PATCH v3 4/6] clk: qcom: ipq5424: rename spi0 clocks

[Manikanta Mylavarapu]

SPI protocol runs on serial engine 4. Hence rename spi0
clocks to spi4 like GCC_QUPV3_SPI0_CLK to GCC_QUPV3_SPI4_CLK.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
---
 drivers/clk/qcom/gcc-ipq5424.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/qcom/gcc-ipq5424.c b/drivers/clk/qcom/gcc-ipq5424.c
index 88a7d5b2e751..5fcf7d9ca390 100644
--- a/drivers/clk/qcom/gcc-ipq5424.c
+++ b/drivers/clk/qcom/gcc-ipq5424.c
@@ -531,7 +531,7 @@ static struct clk_rcg2 gcc_qupv3_i2c1_clk_src = {
 	},
 };
 
-static const struct freq_tbl ftbl_gcc_qupv3_spi0_clk_src[] = {
+static const struct freq_tbl ftbl_gcc_qupv3_spi4_clk_src[] = {
 	F(960000, P_XO, 10, 2, 5),
 	F(4800000, P_XO, 5, 0, 0),
 	F(9600000, P_XO, 2, 4, 5),
@@ -543,14 +543,14 @@ static const struct freq_tbl ftbl_gcc_qupv3_spi0_clk_src[] = {
 	{ }
 };
 
-static struct clk_rcg2 gcc_qupv3_spi0_clk_src = {
+static struct clk_rcg2 gcc_qupv3_spi4_clk_src = {
 	.cmd_rcgr = 0x4004,
 	.mnd_width = 8,
 	.hid_width = 5,
 	.parent_map = gcc_parent_map_0,
-	.freq_tbl = ftbl_gcc_qupv3_spi0_clk_src,
+	.freq_tbl = ftbl_gcc_qupv3_spi4_clk_src,
 	.clkr.hw.init = &(const struct clk_init_data) {
-		.name = "gcc_qupv3_spi0_clk_src",
+		.name = "gcc_qupv3_spi4_clk_src",
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.ops = &clk_rcg2_ops,
@@ -562,7 +562,7 @@ static struct clk_rcg2 gcc_qupv3_spi1_clk_src = {
 	.mnd_width = 8,
 	.hid_width = 5,
 	.parent_map = gcc_parent_map_0,
-	.freq_tbl = ftbl_gcc_qupv3_spi0_clk_src,
+	.freq_tbl = ftbl_gcc_qupv3_spi4_clk_src,
 	.clkr.hw.init = &(const struct clk_init_data) {
 		.name = "gcc_qupv3_spi1_clk_src",
 		.parent_data = gcc_parent_data_0,
@@ -2072,16 +2072,16 @@ static struct clk_branch gcc_qupv3_i2c1_clk = {
 	},
 };
 
-static struct clk_branch gcc_qupv3_spi0_clk = {
+static struct clk_branch gcc_qupv3_spi4_clk = {
 	.halt_reg = 0x4020,
 	.halt_check = BRANCH_HALT,
 	.clkr = {
 		.enable_reg = 0x4020,
 		.enable_mask = BIT(0),
 		.hw.init = &(const struct clk_init_data) {
-			.name = "gcc_qupv3_spi0_clk",
+			.name = "gcc_qupv3_spi4_clk",
 			.parent_hws = (const struct clk_hw*[]) {
-				&gcc_qupv3_spi0_clk_src.clkr.hw,
+				&gcc_qupv3_spi4_clk_src.clkr.hw,
 			},
 			.num_parents = 1,
 			.flags = CLK_SET_RATE_PARENT,
@@ -2862,8 +2862,8 @@ static struct clk_regmap *gcc_ipq5424_clocks[] = {
 	[GCC_QUPV3_I2C1_CLK] = &gcc_qupv3_i2c1_clk.clkr,
 	[GCC_QUPV3_I2C1_CLK_SRC] = &gcc_qupv3_i2c1_clk_src.clkr,
 	[GCC_QUPV3_I2C1_DIV_CLK_SRC] = &gcc_qupv3_i2c1_div_clk_src.clkr,
-	[GCC_QUPV3_SPI0_CLK] = &gcc_qupv3_spi0_clk.clkr,
-	[GCC_QUPV3_SPI0_CLK_SRC] = &gcc_qupv3_spi0_clk_src.clkr,
+	[GCC_QUPV3_SPI4_CLK] = &gcc_qupv3_spi4_clk.clkr,
+	[GCC_QUPV3_SPI4_CLK_SRC] = &gcc_qupv3_spi4_clk_src.clkr,
 	[GCC_QUPV3_SPI1_CLK] = &gcc_qupv3_spi1_clk.clkr,
 	[GCC_QUPV3_SPI1_CLK_SRC] = &gcc_qupv3_spi1_clk_src.clkr,
 	[GCC_QUPV3_UART0_CLK] = &gcc_qupv3_uart0_clk.clkr,
-- 
2.34.1

[PATCH v3 5/6] arm64: dts: qcom: ipq5424: add spi4 node

[Manikanta Mylavarapu]

Add SPI4 node for IPQ5424 SoC.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
---
Changes in V3:
	- Replace spi0 with spi4 in all applicable places such as
	  clocks, commit message, heading and dt node name.

 arch/arm64/boot/dts/qcom/ipq5424.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5424.dtsi b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
index 5e219f900412..d425298d0471 100644
--- a/arch/arm64/boot/dts/qcom/ipq5424.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
@@ -201,6 +201,17 @@ uart1: serial@1a84000 {
 				clock-names = "se";
 				interrupts = <GIC_SPI 340 IRQ_TYPE_LEVEL_HIGH>;
 			};
+
+			spi4: spi@1a90000 {
+				compatible = "qcom,geni-spi";
+				reg = <0 0x01a90000 0 0x4000>;
+				clocks = <&gcc GCC_QUPV3_SPI4_CLK>;
+				clock-names = "se";
+				interrupts = <GIC_SPI 343 IRQ_TYPE_LEVEL_HIGH>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+			};
 		};
 
 		sdhc: mmc@7804000 {
-- 
2.34.1

[PATCH v3 6/6] arm64: dts: qcom: ipq5424: configure spi4 node for rdp466

[Manikanta Mylavarapu]

Enable the SPI4 node and configure the associated gpio pins.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
---
Changes in V3:
	- Replace spi0 with spi4 in all applicable places such as
	  tlmm pin names, commit message, heading and dt node name.

 arch/arm64/boot/dts/qcom/ipq5424-rdp466.dts | 43 +++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5424-rdp466.dts b/arch/arm64/boot/dts/qcom/ipq5424-rdp466.dts
index d4d31026a026..1e7c7f73b21e 100644
--- a/arch/arm64/boot/dts/qcom/ipq5424-rdp466.dts
+++ b/arch/arm64/boot/dts/qcom/ipq5424-rdp466.dts
@@ -23,6 +23,36 @@ &sleep_clk {
 };
 
 &tlmm {
+	spi4_default_state: spi4-default-state {
+		clk-pins {
+			pins = "gpio6";
+			function = "spi4_clk";
+			drive-strength = <8>;
+			bias-pull-down;
+		};
+
+		cs-pins {
+			pins = "gpio7";
+			function = "spi4_cs";
+			drive-strength = <8>;
+			bias-pull-up;
+		};
+
+		miso-pins {
+			pins = "gpio8";
+			function = "spi4_miso";
+			drive-strength = <8>;
+			bias-pull-down;
+		};
+
+		mosi-pins {
+			pins = "gpio9";
+			function = "spi4_mosi";
+			drive-strength = <8>;
+			bias-pull-down;
+		};
+	};
+
 	sdc_default_state: sdc-default-state {
 		clk-pins {
 			pins = "gpio5";
@@ -57,3 +87,16 @@ &xo_board {
 	clock-frequency = <24000000>;
 };
 
+&spi4 {
+	pinctrl-0 = <&spi4_default_state>;
+	pinctrl-names = "default";
+	status = "okay";
+
+	flash@0 {
+		compatible = "micron,n25q128a11", "jedec,spi-nor";
+		reg = <0>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		spi-max-frequency = <50000000>;
+	};
+};
-- 
2.34.1

Re: [PATCH v3 1/6] dt-bindings: pinctrl: qcom: rename spi0 pins on IPQ5424

[Krzysztof Kozlowski]

On 27/12/2024 08:24, Manikanta Mylavarapu wrote:
> SPI protocol runs on serial engine 4. Hence rename
> spi0 pins to spi4 like spi0_cs to spi4_cs etc.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> ---


<form letter>
This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
of patchset, under or above your Signed-off-by tag, unless patch changed
significantly (e.g. new properties added to the DT bindings). Tag is
"received", when provided in a message replied to you on the mailing
list. Tools like b4 can help here. However, there's no need to repost
patches *only* to add the tags. The upstream maintainer will do that for
tags received on the version they apply.

Please read:
https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.
</form letter>

Best regards,
Krzysztof

Re: [PATCH v3 1/6] dt-bindings: pinctrl: qcom: rename spi0 pins on IPQ5424

[Manikanta Mylavarapu]

On 12/27/2024 1:06 PM, Krzysztof Kozlowski wrote:
> On 27/12/2024 08:24, Manikanta Mylavarapu wrote:
>> SPI protocol runs on serial engine 4. Hence rename
>> spi0 pins to spi4 like spi0_cs to spi4_cs etc.
>>
>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>> ---
> 
> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It looks like you received a tag and forgot to add it.
> 
> If you do not know the process, here is a short explanation:
> Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
> of patchset, under or above your Signed-off-by tag, unless patch changed
> significantly (e.g. new properties added to the DT bindings). Tag is
> "received", when provided in a message replied to you on the mailing
> list. Tools like b4 can help here. However, there's no need to repost
> patches *only* to add the tags. The upstream maintainer will do that for
> tags received on the version they apply.
> 
> Please read:
> https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577
> 
> If a tag was not added on purpose, please state why and what changed.
> </form letter>
> 

Hi Krzysztof,

	Patches #1 to #4 are newly added in V3 (to rename SPI0 to SPI4). Hence, there are no A-b/R-b
	tags associated with these patches. I mentioned this information in the cover letter.
	
	I assume you are referring to Patch #1 from the V2 series.
	Patch #1 [1] and #2 [2] from the V2 series have been merged into linux-next.
	[1] https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-2-quic_mmanikan@quicinc.com/
	[2] https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-3-quic_mmanikan@quicinc.com/

	Please let me know if i missed anything.

Thanks & Regards,
Manikanta.

Re: [PATCH v3 1/6] dt-bindings: pinctrl: qcom: rename spi0 pins on IPQ5424

[Krzysztof Kozlowski]

On 27/12/2024 10:18, Manikanta Mylavarapu wrote:
> 
> 
> On 12/27/2024 1:06 PM, Krzysztof Kozlowski wrote:
>> On 27/12/2024 08:24, Manikanta Mylavarapu wrote:
>>> SPI protocol runs on serial engine 4. Hence rename
>>> spi0 pins to spi4 like spi0_cs to spi4_cs etc.
>>>
>>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>>> ---
>>
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It looks like you received a tag and forgot to add it.
>>
>> If you do not know the process, here is a short explanation:
>> Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
>> of patchset, under or above your Signed-off-by tag, unless patch changed
>> significantly (e.g. new properties added to the DT bindings). Tag is
>> "received", when provided in a message replied to you on the mailing
>> list. Tools like b4 can help here. However, there's no need to repost
>> patches *only* to add the tags. The upstream maintainer will do that for
>> tags received on the version they apply.
>>
>> Please read:
>> https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577
>>
>> If a tag was not added on purpose, please state why and what changed.
>> </form letter>
>>
> 
> Hi Krzysztof,
> 
> 	Patches #1 to #4 are newly added in V3 (to rename SPI0 to SPI4). Hence, there are no A-b/R-b
> 	tags associated with these patches. I mentioned this information in the cover letter.
> 	
> 	I assume you are referring to Patch #1 from the V2 series.
> 	Patch #1 [1] and #2 [2] from the V2 series have been merged into linux-next.
> 	[1] https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-2-quic_mmanikan@quicinc.com/
> 	[2] https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-3-quic_mmanikan@quicinc.com/
> 
> 	Please let me know if i missed anything.

v3 mislead me here and three different subsystems in one patchset.

Anyway, if this is different patch then review follows - there is no ABI
impact explanation and this is an ABI break. What's more, entries are
not sorted anymore and why there is a gap? spi4, spi1 and spi10? Where
is spi3?

Not sure if this renaming is useful or correct, especially considering
not many arguments in commit msg (e.g. datasheet?).


Best regards,
Krzysztof

Re: [PATCH v3 2/6] dt-bindings: clock: qcom: gcc-ipq5424: add spi4 clocks

[Krzysztof Kozlowski]

On Fri, Dec 27, 2024 at 12:54:42PM +0530, Manikanta Mylavarapu wrote:
> SPI protocol runs on serial engine 4. Hence we need to
> rename the spi0 clocks to spi4 clocks.


No, you do not need. Why spi0 is incorrect?

> 
> However, renaming spi0 to spi4 will result in the following
> compilation error's.
> drivers/clk/qcom/gcc-ipq5424.c:2865:3: error: ‘GCC_QUPV3_SPI0_CLK’
> undeclared here
> drivers/clk/qcom/gcc-ipq5424.c:2866:3: error: ‘GCC_QUPV3_SPI0_CLK_SRC’
> undeclared here

Then do not rename... Sorry, but that part makes no sense. You must keep
ABI, so that's why you add new clocks.


> 
> To add spi4 clocks without compilation error's, do not
> rename the spi0 clocks. Instead, duplicate the spi0 clock
> macros and rename them to spi4.
> 
> After switching to spi4 clocks in the gcc-ipq5424 driver,
> remove the spi0 clock macros.

No. ABI.


Best regards,
Krzysztof

Re: [PATCH v3 4/6] clk: qcom: ipq5424: rename spi0 clocks

[Krzysztof Kozlowski]

On Fri, Dec 27, 2024 at 12:54:44PM +0530, Manikanta Mylavarapu wrote:
>  			.num_parents = 1,
>  			.flags = CLK_SET_RATE_PARENT,
> @@ -2862,8 +2862,8 @@ static struct clk_regmap *gcc_ipq5424_clocks[] = {
>  	[GCC_QUPV3_I2C1_CLK] = &gcc_qupv3_i2c1_clk.clkr,
>  	[GCC_QUPV3_I2C1_CLK_SRC] = &gcc_qupv3_i2c1_clk_src.clkr,
>  	[GCC_QUPV3_I2C1_DIV_CLK_SRC] = &gcc_qupv3_i2c1_div_clk_src.clkr,
> -	[GCC_QUPV3_SPI0_CLK] = &gcc_qupv3_spi0_clk.clkr,
> -	[GCC_QUPV3_SPI0_CLK_SRC] = &gcc_qupv3_spi0_clk_src.clkr,
> +	[GCC_QUPV3_SPI4_CLK] = &gcc_qupv3_spi4_clk.clkr,
> +	[GCC_QUPV3_SPI4_CLK_SRC] = &gcc_qupv3_spi4_clk_src.clkr,

ABI break without any explanation, real justification.

Best regards,
Krzysztof

Re: [PATCH v3 0/6] Add SPI4 support for IPQ5424

[Kathiravan Thirumoorthy]

On 12/27/2024 12:54 PM, Manikanta Mylavarapu wrote:
> Add SPI4 node to the IPQ5424 device tree and update the relevant
> bindings, GPIO pin mappings accordingly.
> 
> Changes in V3:
> 	- Rename SPI0 to SPI4 because SPI protocol runs on serial engine 4

Do we really need to do this? If so, it will not align with the HW 
documentation and will lead to the confusion down the line. IMHO, we 
should stick with the convention followed in the HW documentation.

Thanks,
Kathiravan T.

Re: [PATCH v3 1/6] dt-bindings: pinctrl: qcom: rename spi0 pins on IPQ5424

[Manikanta Mylavarapu]

On 12/27/2024 3:00 PM, Krzysztof Kozlowski wrote:
> On 27/12/2024 10:18, Manikanta Mylavarapu wrote:
>>
>>
>> On 12/27/2024 1:06 PM, Krzysztof Kozlowski wrote:
>>> On 27/12/2024 08:24, Manikanta Mylavarapu wrote:
>>>> SPI protocol runs on serial engine 4. Hence rename
>>>> spi0 pins to spi4 like spi0_cs to spi4_cs etc.
>>>>
>>>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>>>> ---
>>>
>>>
>>> <form letter>
>>> This is a friendly reminder during the review process.
>>>
>>> It looks like you received a tag and forgot to add it.
>>>
>>> If you do not know the process, here is a short explanation:
>>> Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
>>> of patchset, under or above your Signed-off-by tag, unless patch changed
>>> significantly (e.g. new properties added to the DT bindings). Tag is
>>> "received", when provided in a message replied to you on the mailing
>>> list. Tools like b4 can help here. However, there's no need to repost
>>> patches *only* to add the tags. The upstream maintainer will do that for
>>> tags received on the version they apply.
>>>
>>> Please read:
>>> https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577
>>>
>>> If a tag was not added on purpose, please state why and what changed.
>>> </form letter>
>>>
>>
>> Hi Krzysztof,
>>
>> 	Patches #1 to #4 are newly added in V3 (to rename SPI0 to SPI4). Hence, there are no A-b/R-b
>> 	tags associated with these patches. I mentioned this information in the cover letter.
>> 	
>> 	I assume you are referring to Patch #1 from the V2 series.
>> 	Patch #1 [1] and #2 [2] from the V2 series have been merged into linux-next.
>> 	[1] https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-2-quic_mmanikan@quicinc.com/
>> 	[2] https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-3-quic_mmanikan@quicinc.com/
>>
>> 	Please let me know if i missed anything.
> 
> v3 mislead me here and three different subsystems in one patchset.
> 
> Anyway, if this is different patch then review follows - there is no ABI
> impact explanation and this is an ABI break. What's more, entries are
> not sorted anymore and why there is a gap? spi4, spi1 and spi10? Where
> is spi3?
> 
> Not sure if this renaming is useful or correct, especially considering
> not many arguments in commit msg (e.g. datasheet?).
> 
> 

Hi Krzysztof,

	The IPQ5424 supports two SPI instances on serial engine 4 and 5.
	Previously, SPI clocks, gpio pins and DTS node names were named
	according to protocol instances like spi0 and spi1.

	As per the feedback received in
	https://lore.kernel.org/linux-arm-msm/ca0ecc07-fd45-4116-9927-8eb3e737505f@oss.qualcomm.com/,
	spi0 has been renamed to spi4 to align with the serial engine instance.

	Kindly advice if it's not acceptable.

Thanks & Regards,
Manikanta.

Re: [PATCH v3 1/6] dt-bindings: pinctrl: qcom: rename spi0 pins on IPQ5424

[Krzysztof Kozlowski]

On 30/12/2024 08:50, Manikanta Mylavarapu wrote:
> 
> 
> On 12/27/2024 3:00 PM, Krzysztof Kozlowski wrote:
>> On 27/12/2024 10:18, Manikanta Mylavarapu wrote:
>>>
>>>
>>> On 12/27/2024 1:06 PM, Krzysztof Kozlowski wrote:
>>>> On 27/12/2024 08:24, Manikanta Mylavarapu wrote:
>>>>> SPI protocol runs on serial engine 4. Hence rename
>>>>> spi0 pins to spi4 like spi0_cs to spi4_cs etc.
>>>>>
>>>>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>>>>> ---
>>>>
>>>>
>>>> <form letter>
>>>> This is a friendly reminder during the review process.
>>>>
>>>> It looks like you received a tag and forgot to add it.
>>>>
>>>> If you do not know the process, here is a short explanation:
>>>> Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
>>>> of patchset, under or above your Signed-off-by tag, unless patch changed
>>>> significantly (e.g. new properties added to the DT bindings). Tag is
>>>> "received", when provided in a message replied to you on the mailing
>>>> list. Tools like b4 can help here. However, there's no need to repost
>>>> patches *only* to add the tags. The upstream maintainer will do that for
>>>> tags received on the version they apply.
>>>>
>>>> Please read:
>>>> https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577
>>>>
>>>> If a tag was not added on purpose, please state why and what changed.
>>>> </form letter>
>>>>
>>>
>>> Hi Krzysztof,
>>>
>>> 	Patches #1 to #4 are newly added in V3 (to rename SPI0 to SPI4). Hence, there are no A-b/R-b
>>> 	tags associated with these patches. I mentioned this information in the cover letter.
>>> 	
>>> 	I assume you are referring to Patch #1 from the V2 series.
>>> 	Patch #1 [1] and #2 [2] from the V2 series have been merged into linux-next.
>>> 	[1] https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-2-quic_mmanikan@quicinc.com/
>>> 	[2] https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-3-quic_mmanikan@quicinc.com/
>>>
>>> 	Please let me know if i missed anything.
>>
>> v3 mislead me here and three different subsystems in one patchset.
>>
>> Anyway, if this is different patch then review follows - there is no ABI
>> impact explanation and this is an ABI break. What's more, entries are
>> not sorted anymore and why there is a gap? spi4, spi1 and spi10? Where
>> is spi3?
>>
>> Not sure if this renaming is useful or correct, especially considering
>> not many arguments in commit msg (e.g. datasheet?).
>>
>>
> 
> Hi Krzysztof,
> 
> 	The IPQ5424 supports two SPI instances on serial engine 4 and 5.
> 	Previously, SPI clocks, gpio pins and DTS node names were named
> 	according to protocol instances like spi0 and spi1.
> 
> 	As per the feedback received in
> 	https://lore.kernel.org/linux-arm-msm/ca0ecc07-fd45-4116-9927-8eb3e737505f@oss.qualcomm.com/,
> 	spi0 has been renamed to spi4 to align with the serial engine instance.
> 
> 	Kindly advice if it's not acceptable.

The advice was not about pins, though. My comments stands for commit
msg. Nothing about ABI, nothing about datasheet...

Best regards,
Krzysztof

Re: [PATCH v3 1/6] dt-bindings: pinctrl: qcom: rename spi0 pins on IPQ5424

[Manikanta Mylavarapu]

On 12/30/2024 1:46 PM, Krzysztof Kozlowski wrote:
> On 30/12/2024 08:50, Manikanta Mylavarapu wrote:
>>
>>
>> On 12/27/2024 3:00 PM, Krzysztof Kozlowski wrote:
>>> On 27/12/2024 10:18, Manikanta Mylavarapu wrote:
>>>>
>>>>
>>>> On 12/27/2024 1:06 PM, Krzysztof Kozlowski wrote:
>>>>> On 27/12/2024 08:24, Manikanta Mylavarapu wrote:
>>>>>> SPI protocol runs on serial engine 4. Hence rename
>>>>>> spi0 pins to spi4 like spi0_cs to spi4_cs etc.
>>>>>>
>>>>>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>>>>>> ---
>>>>>
>>>>>
>>>>> <form letter>
>>>>> This is a friendly reminder during the review process.
>>>>>
>>>>> It looks like you received a tag and forgot to add it.
>>>>>
>>>>> If you do not know the process, here is a short explanation:
>>>>> Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
>>>>> of patchset, under or above your Signed-off-by tag, unless patch changed
>>>>> significantly (e.g. new properties added to the DT bindings). Tag is
>>>>> "received", when provided in a message replied to you on the mailing
>>>>> list. Tools like b4 can help here. However, there's no need to repost
>>>>> patches *only* to add the tags. The upstream maintainer will do that for
>>>>> tags received on the version they apply.
>>>>>
>>>>> Please read:
>>>>> https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577
>>>>>
>>>>> If a tag was not added on purpose, please state why and what changed.
>>>>> </form letter>
>>>>>
>>>>
>>>> Hi Krzysztof,
>>>>
>>>> 	Patches #1 to #4 are newly added in V3 (to rename SPI0 to SPI4). Hence, there are no A-b/R-b
>>>> 	tags associated with these patches. I mentioned this information in the cover letter.
>>>> 	
>>>> 	I assume you are referring to Patch #1 from the V2 series.
>>>> 	Patch #1 [1] and #2 [2] from the V2 series have been merged into linux-next.
>>>> 	[1] https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-2-quic_mmanikan@quicinc.com/
>>>> 	[2] https://lore.kernel.org/linux-arm-msm/20241217091308.3253897-3-quic_mmanikan@quicinc.com/
>>>>
>>>> 	Please let me know if i missed anything.
>>>
>>> v3 mislead me here and three different subsystems in one patchset.
>>>
>>> Anyway, if this is different patch then review follows - there is no ABI
>>> impact explanation and this is an ABI break. What's more, entries are
>>> not sorted anymore and why there is a gap? spi4, spi1 and spi10? Where
>>> is spi3?
>>>
>>> Not sure if this renaming is useful or correct, especially considering
>>> not many arguments in commit msg (e.g. datasheet?).
>>>
>>>
>>
>> Hi Krzysztof,
>>
>> 	The IPQ5424 supports two SPI instances on serial engine 4 and 5.
>> 	Previously, SPI clocks, gpio pins and DTS node names were named
>> 	according to protocol instances like spi0 and spi1.
>>
>> 	As per the feedback received in
>> 	https://lore.kernel.org/linux-arm-msm/ca0ecc07-fd45-4116-9927-8eb3e737505f@oss.qualcomm.com/,
>> 	spi0 has been renamed to spi4 to align with the serial engine instance.
>>
>> 	Kindly advice if it's not acceptable.
> 
> The advice was not about pins, though. My comments stands for commit
> msg. Nothing about ABI, nothing about datasheet...
> 

I will update the commit message in the next version.

Thanks & Regards,
Manikanta.

Re: [PATCH v3 0/6] Add SPI4 support for IPQ5424

[Konrad Dybcio]

On 30.12.2024 7:51 AM, Kathiravan Thirumoorthy wrote:
> 
> 
> On 12/27/2024 12:54 PM, Manikanta Mylavarapu wrote:
>> Add SPI4 node to the IPQ5424 device tree and update the relevant
>> bindings, GPIO pin mappings accordingly.
>>
>> Changes in V3:
>>     - Rename SPI0 to SPI4 because SPI protocol runs on serial engine 4
> 
> Do we really need to do this? If so, it will not align with the HW documentation and will lead to the confusion down the line. IMHO, we should stick with the convention followed in the HW documentation.

+1, the clocks are called SPI0/SPI1 internally

Konrad

Re: [PATCH v3 0/6] Add SPI4 support for IPQ5424

[Konrad Dybcio]

On 30.12.2024 2:54 PM, Konrad Dybcio wrote:
> On 30.12.2024 7:51 AM, Kathiravan Thirumoorthy wrote:
>>
>>
>> On 12/27/2024 12:54 PM, Manikanta Mylavarapu wrote:
>>> Add SPI4 node to the IPQ5424 device tree and update the relevant
>>> bindings, GPIO pin mappings accordingly.
>>>
>>> Changes in V3:
>>>     - Rename SPI0 to SPI4 because SPI protocol runs on serial engine 4
>>
>> Do we really need to do this? If so, it will not align with the HW documentation and will lead to the confusion down the line. IMHO, we should stick with the convention followed in the HW documentation.
> 
> +1, the clocks are called SPI0/SPI1 internally

Ok, I looked at a bit more documentation and it looks like
somebody just had fun naming things..

SPI0 is on SE4 and SPI1 is on something else, with no more
clock provisions for that protocol.. Which is not usually the
case.

Let's just go with what you guys use internally, as this is
mighty spaghetti

Konrad

Re: [PATCH v3 0/6] Add SPI4 support for IPQ5424

[Kathiravan Thirumoorthy]

On 12/30/2024 7:28 PM, Konrad Dybcio wrote:
> On 30.12.2024 2:54 PM, Konrad Dybcio wrote:
>> On 30.12.2024 7:51 AM, Kathiravan Thirumoorthy wrote:
>>>
>>>
>>> On 12/27/2024 12:54 PM, Manikanta Mylavarapu wrote:
>>>> Add SPI4 node to the IPQ5424 device tree and update the relevant
>>>> bindings, GPIO pin mappings accordingly.
>>>>
>>>> Changes in V3:
>>>>      - Rename SPI0 to SPI4 because SPI protocol runs on serial engine 4
>>>
>>> Do we really need to do this? If so, it will not align with the HW documentation and will lead to the confusion down the line. IMHO, we should stick with the convention followed in the HW documentation.
>>
>> +1, the clocks are called SPI0/SPI1 internally
> 
> Ok, I looked at a bit more documentation and it looks like
> somebody just had fun naming things..
> 
> SPI0 is on SE4 and SPI1 is on something else, with no more
> clock provisions for that protocol.. Which is not usually the
> case.


IPQ5424 has one QUPV3 instance with 6 SEs. SE0-SE4 are Mini core and SE5 
is FW core.

SE0 and SE1 are for 4-wire UART and 2-wire UART respectively. SE2 and 
SE3 are for I2C protocol. SE4 is for SPI.

Since SE5 is FW based (some RDPs use this SE for I2C). In GCC block, 
clocks for this instance is named after SPI as SPI1.


> 
> Let's just go with what you guys use internally, as this is
> mighty spaghetti
> 
> Konrad

Re: [PATCH v3 0/6] Add SPI4 support for IPQ5424

[Konrad Dybcio]

On 30.12.2024 4:34 PM, Kathiravan Thirumoorthy wrote:
> 
> 
> On 12/30/2024 7:28 PM, Konrad Dybcio wrote:
>> On 30.12.2024 2:54 PM, Konrad Dybcio wrote:
>>> On 30.12.2024 7:51 AM, Kathiravan Thirumoorthy wrote:
>>>>
>>>>
>>>> On 12/27/2024 12:54 PM, Manikanta Mylavarapu wrote:
>>>>> Add SPI4 node to the IPQ5424 device tree and update the relevant
>>>>> bindings, GPIO pin mappings accordingly.
>>>>>
>>>>> Changes in V3:
>>>>>      - Rename SPI0 to SPI4 because SPI protocol runs on serial engine 4
>>>>
>>>> Do we really need to do this? If so, it will not align with the HW documentation and will lead to the confusion down the line. IMHO, we should stick with the convention followed in the HW documentation.
>>>
>>> +1, the clocks are called SPI0/SPI1 internally
>>
>> Ok, I looked at a bit more documentation and it looks like
>> somebody just had fun naming things..
>>
>> SPI0 is on SE4 and SPI1 is on something else, with no more
>> clock provisions for that protocol.. Which is not usually the
>> case.
> 
> 
> IPQ5424 has one QUPV3 instance with 6 SEs. SE0-SE4 are Mini core and SE5 is FW core.
> 
> SE0 and SE1 are for 4-wire UART and 2-wire UART respectively. SE2 and SE3 are for I2C protocol. SE4 is for SPI.
> 
> Since SE5 is FW based (some RDPs use this SE for I2C). In GCC block, clocks for this instance is named after SPI as SPI1.

Thanks for the explanation.

Manikanta, please refer to this in the commit message as well

Konrad

Re: [PATCH v3 0/6] Add SPI4 support for IPQ5424

[Manikanta Mylavarapu]

On 12/30/2024 9:06 PM, Konrad Dybcio wrote:
> On 30.12.2024 4:34 PM, Kathiravan Thirumoorthy wrote:
>>
>>
>> On 12/30/2024 7:28 PM, Konrad Dybcio wrote:
>>> On 30.12.2024 2:54 PM, Konrad Dybcio wrote:
>>>> On 30.12.2024 7:51 AM, Kathiravan Thirumoorthy wrote:
>>>>>
>>>>>
>>>>> On 12/27/2024 12:54 PM, Manikanta Mylavarapu wrote:
>>>>>> Add SPI4 node to the IPQ5424 device tree and update the relevant
>>>>>> bindings, GPIO pin mappings accordingly.
>>>>>>
>>>>>> Changes in V3:
>>>>>>      - Rename SPI0 to SPI4 because SPI protocol runs on serial engine 4
>>>>>
>>>>> Do we really need to do this? If so, it will not align with the HW documentation and will lead to the confusion down the line. IMHO, we should stick with the convention followed in the HW documentation.
>>>>
>>>> +1, the clocks are called SPI0/SPI1 internally
>>>
>>> Ok, I looked at a bit more documentation and it looks like
>>> somebody just had fun naming things..
>>>
>>> SPI0 is on SE4 and SPI1 is on something else, with no more
>>> clock provisions for that protocol.. Which is not usually the
>>> case.
>>
>>
>> IPQ5424 has one QUPV3 instance with 6 SEs. SE0-SE4 are Mini core and SE5 is FW core.
>>
>> SE0 and SE1 are for 4-wire UART and 2-wire UART respectively. SE2 and SE3 are for I2C protocol. SE4 is for SPI.
>>
>> Since SE5 is FW based (some RDPs use this SE for I2C). In GCC block, clocks for this instance is named after SPI as SPI1.
> 
> Thanks for the explanation.
> 
> Manikanta, please refer to this in the commit message as well
> 


Thank you, Konrad and Kathiravan, for your valuable insights.
I will incorporate the aforementioned information into the commit message, revert the 'renaming spi0 to spi4',
and include both spi0 and spi1 in the next version.

Thanks & Regards,
Manikanta.