netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/3] arm64: dts: qcom: DTS updates
@ 2021-07-19 21:24 Alex Elder
  2021-07-19 21:24 ` [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional Alex Elder
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Alex Elder @ 2021-07-19 21:24 UTC (permalink / raw)
  To: bjorn.andersson, agross, robh+dt
  Cc: davem, kuba, evgreen, cpratapa, subashab, elder, linux-arm-msm,
	netdev, devicetree, linux-kernel

This series updates some IPA-related DT nodes.

Newer versions of IPA do not require an interconnect between IPA
and SoC internal memory.  The first patch updates the DT binding
to reflect this.

The second patch adds IPA information to "sc7280.dtsi", using only
two interconnects.  It includes the definition of the reserved
memory area used to hold IPA firmware.

The last patch defines the reserved IPA firmware memory area in
"sc7180.dtsi".

					-Alex

Alex Elder (3):
  dt-bindings: net: qcom,ipa: make imem interconnect optional
  arm64: dts: qcom: sc7280: add IPA information
  arm64: dts: qcom: sc7180: define ipa_fw_mem node

 .../devicetree/bindings/net/qcom,ipa.yaml     | 18 ++++----
 arch/arm64/boot/dts/qcom/sc7180.dtsi          |  5 +++
 arch/arm64/boot/dts/qcom/sc7280.dtsi          | 43 +++++++++++++++++++
 3 files changed, 58 insertions(+), 8 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional
  2021-07-19 21:24 [PATCH net-next 0/3] arm64: dts: qcom: DTS updates Alex Elder
@ 2021-07-19 21:24 ` Alex Elder
  2021-07-23 20:52   ` Rob Herring
  2021-07-19 21:24 ` [PATCH net-next 2/3] arm64: dts: qcom: sc7280: add IPA information Alex Elder
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Alex Elder @ 2021-07-19 21:24 UTC (permalink / raw)
  To: robh+dt, bjorn.andersson, agross
  Cc: davem, kuba, evgreen, cpratapa, subashab, elder, linux-arm-msm,
	netdev, devicetree, linux-kernel

On some newer SoCs, the interconnect between IPA and SoC internal
memory (imem) is not used.  Reflect this in the binding by moving
the definition of the "imem" interconnect to the end and defining
minItems to be 2 for both the interconnects and interconnect-names
properties.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 .../devicetree/bindings/net/qcom,ipa.yaml      | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
index ed88ba4b94df5..4853ab7017bd9 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
@@ -87,16 +87,18 @@ properties:
       - const: ipa-setup-ready
 
   interconnects:
+    minItems: 2
     items:
-      - description: Interconnect path between IPA and main memory
-      - description: Interconnect path between IPA and internal memory
-      - description: Interconnect path between IPA and the AP subsystem
+      - description: Path leading to system memory
+      - description: Path between the AP and IPA config space
+      - description: Path leading to internal memory
 
   interconnect-names:
+    minItems: 2
     items:
       - const: memory
-      - const: imem
       - const: config
+      - const: imem
 
   qcom,smem-states:
     $ref: /schemas/types.yaml#/definitions/phandle-array
@@ -207,11 +209,11 @@ examples:
 
                 interconnects =
                         <&rsc_hlos MASTER_IPA &rsc_hlos SLAVE_EBI1>,
-                        <&rsc_hlos MASTER_IPA &rsc_hlos SLAVE_IMEM>,
-                        <&rsc_hlos MASTER_APPSS_PROC &rsc_hlos SLAVE_IPA_CFG>;
+                        <&rsc_hlos MASTER_APPSS_PROC &rsc_hlos SLAVE_IPA_CFG>,
+                        <&rsc_hlos MASTER_IPA &rsc_hlos SLAVE_IMEM>;
                 interconnect-names = "memory",
-                                     "imem",
-                                     "config";
+                                     "config",
+                                     "imem";
 
                 qcom,smem-states = <&ipa_smp2p_out 0>,
                                    <&ipa_smp2p_out 1>;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 2/3] arm64: dts: qcom: sc7280: add IPA information
  2021-07-19 21:24 [PATCH net-next 0/3] arm64: dts: qcom: DTS updates Alex Elder
  2021-07-19 21:24 ` [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional Alex Elder
@ 2021-07-19 21:24 ` Alex Elder
  2021-07-19 21:24 ` [PATCH net-next 3/3] arm64: dts: qcom: sc7180: define ipa_fw_mem node Alex Elder
  2021-07-20 14:20 ` [PATCH net-next 0/3] arm64: dts: qcom: DTS updates patchwork-bot+netdevbpf
  3 siblings, 0 replies; 12+ messages in thread
From: Alex Elder @ 2021-07-19 21:24 UTC (permalink / raw)
  To: bjorn.andersson, agross
  Cc: robh+dt, davem, kuba, evgreen, cpratapa, subashab, elder,
	linux-arm-msm, netdev, devicetree, linux-kernel

Add IPA-related nodes and definitions to "sc7280.dtsi", including
the reserved memory area used for AP-based IPA firmware loading.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 43 ++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index a8c274ad74c47..5eb2b58ea23be 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -7,6 +7,7 @@
 
 #include <dt-bindings/clock/qcom,gcc-sc7280.h>
 #include <dt-bindings/clock/qcom,rpmh.h>
+#include <dt-bindings/interconnect/qcom,sc7280.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/mailbox/qcom-ipcc.h>
 #include <dt-bindings/power/qcom-aoss-qmp.h>
@@ -63,6 +64,11 @@ cpucp_mem: memory@80b00000 {
 			no-map;
 			reg = <0x0 0x80b00000 0x0 0x100000>;
 		};
+
+		ipa_fw_mem: memory@8b700000 {
+			reg = <0 0x8b700000 0 0x10000>;
+			no-map;
+		};
 	};
 
 	cpus {
@@ -508,6 +514,43 @@ mmss_noc: interconnect@1740000 {
 			qcom,bcm-voters = <&apps_bcm_voter>;
 		};
 
+		ipa: ipa@1e40000 {
+			compatible = "qcom,sc7280-ipa";
+
+			iommus = <&apps_smmu 0x480 0x0>,
+				 <&apps_smmu 0x482 0x0>;
+			reg = <0 0x1e40000 0 0x8000>,
+			      <0 0x1e50000 0 0x4ad0>,
+			      <0 0x1e04000 0 0x23000>;
+			reg-names = "ipa-reg",
+				    "ipa-shared",
+				    "gsi";
+
+			interrupts-extended = <&intc 0 654 IRQ_TYPE_EDGE_RISING>,
+					      <&intc 0 432 IRQ_TYPE_LEVEL_HIGH>,
+					      <&ipa_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
+					      <&ipa_smp2p_in 1 IRQ_TYPE_EDGE_RISING>;
+			interrupt-names = "ipa",
+					  "gsi",
+					  "ipa-clock-query",
+					  "ipa-setup-ready";
+
+			clocks = <&rpmhcc RPMH_IPA_CLK>;
+			clock-names = "core";
+
+			interconnects = <&aggre2_noc MASTER_IPA 0 &mc_virt SLAVE_EBI1 0>,
+					<&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_IPA_CFG 0>;
+			interconnect-names = "memory",
+					     "config";
+
+			qcom,smem-states = <&ipa_smp2p_out 0>,
+					   <&ipa_smp2p_out 1>;
+			qcom,smem-state-names = "ipa-clock-enabled-valid",
+						"ipa-clock-enabled";
+
+			status = "disabled";
+		};
+
 		tcsr_mutex: hwlock@1f40000 {
 			compatible = "qcom,tcsr-mutex", "syscon";
 			reg = <0 0x01f40000 0 0x40000>;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 3/3] arm64: dts: qcom: sc7180: define ipa_fw_mem node
  2021-07-19 21:24 [PATCH net-next 0/3] arm64: dts: qcom: DTS updates Alex Elder
  2021-07-19 21:24 ` [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional Alex Elder
  2021-07-19 21:24 ` [PATCH net-next 2/3] arm64: dts: qcom: sc7280: add IPA information Alex Elder
@ 2021-07-19 21:24 ` Alex Elder
  2021-07-20 14:20 ` [PATCH net-next 0/3] arm64: dts: qcom: DTS updates patchwork-bot+netdevbpf
  3 siblings, 0 replies; 12+ messages in thread
From: Alex Elder @ 2021-07-19 21:24 UTC (permalink / raw)
  To: bjorn.andersson, agross
  Cc: robh+dt, davem, kuba, evgreen, cpratapa, subashab, elder,
	linux-arm-msm, netdev, devicetree, linux-kernel

Define the reserved memory space used for IPA firmware for the
Qualcomm SC7180 SoC.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index a5d58eb928960..7af551a1fd904 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -110,6 +110,11 @@ tz_mem: memory@80b00000 {
 			no-map;
 		};
 
+		ipa_fw_mem: memory@8b700000 {
+			reg = <0 0x8b700000 0 0x10000>;
+			no-map;
+		};
+
 		rmtfs_mem: memory@94600000 {
 			compatible = "qcom,rmtfs-mem";
 			reg = <0x0 0x94600000 0x0 0x200000>;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 0/3] arm64: dts: qcom: DTS updates
  2021-07-19 21:24 [PATCH net-next 0/3] arm64: dts: qcom: DTS updates Alex Elder
                   ` (2 preceding siblings ...)
  2021-07-19 21:24 ` [PATCH net-next 3/3] arm64: dts: qcom: sc7180: define ipa_fw_mem node Alex Elder
@ 2021-07-20 14:20 ` patchwork-bot+netdevbpf
  2021-07-20 15:59   ` Bjorn Andersson
  3 siblings, 1 reply; 12+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-07-20 14:20 UTC (permalink / raw)
  To: Alex Elder
  Cc: bjorn.andersson, agross, robh+dt, davem, kuba, evgreen, cpratapa,
	subashab, elder, linux-arm-msm, netdev, devicetree, linux-kernel

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Mon, 19 Jul 2021 16:24:53 -0500 you wrote:
> This series updates some IPA-related DT nodes.
> 
> Newer versions of IPA do not require an interconnect between IPA
> and SoC internal memory.  The first patch updates the DT binding
> to reflect this.
> 
> The second patch adds IPA information to "sc7280.dtsi", using only
> two interconnects.  It includes the definition of the reserved
> memory area used to hold IPA firmware.
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional
    https://git.kernel.org/netdev/net-next/c/6a0eb6c9d934
  - [net-next,2/3] arm64: dts: qcom: sc7280: add IPA information
    https://git.kernel.org/netdev/net-next/c/f8bd3c82bf7d
  - [net-next,3/3] arm64: dts: qcom: sc7180: define ipa_fw_mem node
    https://git.kernel.org/netdev/net-next/c/fd0f72c34bd9

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 0/3] arm64: dts: qcom: DTS updates
  2021-07-20 14:20 ` [PATCH net-next 0/3] arm64: dts: qcom: DTS updates patchwork-bot+netdevbpf
@ 2021-07-20 15:59   ` Bjorn Andersson
  2021-07-26 15:44     ` Alex Elder
  0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Andersson @ 2021-07-20 15:59 UTC (permalink / raw)
  To: davem, kuba
  Cc: Alex Elder, agross, robh+dt, evgreen, cpratapa, subashab, elder,
	linux-arm-msm, netdev, devicetree, linux-kernel

On Tue 20 Jul 09:20 CDT 2021, patchwork-bot+netdevbpf@kernel.org wrote:

> Hello:
> 
> This series was applied to netdev/net-next.git (refs/heads/master):
> 

David, Jakub, can you please revert/drop the two "arm64: dts" patches
from the net-next tree?

DTS patches are generally merged through the qcom and ultimately soc
tree and I have a number of patches queued up in both sc7180 and sc7280
that will cause merge conflicts down the road, so I would prefer to pick
these up as well.

Regards,
Bjorn

> On Mon, 19 Jul 2021 16:24:53 -0500 you wrote:
> > This series updates some IPA-related DT nodes.
> > 
> > Newer versions of IPA do not require an interconnect between IPA
> > and SoC internal memory.  The first patch updates the DT binding
> > to reflect this.
> > 
> > The second patch adds IPA information to "sc7280.dtsi", using only
> > two interconnects.  It includes the definition of the reserved
> > memory area used to hold IPA firmware.
> > 
> > [...]
> 
> Here is the summary with links:
>   - [net-next,1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional
>     https://git.kernel.org/netdev/net-next/c/6a0eb6c9d934
>   - [net-next,2/3] arm64: dts: qcom: sc7280: add IPA information
>     https://git.kernel.org/netdev/net-next/c/f8bd3c82bf7d
>   - [net-next,3/3] arm64: dts: qcom: sc7180: define ipa_fw_mem node
>     https://git.kernel.org/netdev/net-next/c/fd0f72c34bd9
> 
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional
  2021-07-19 21:24 ` [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional Alex Elder
@ 2021-07-23 20:52   ` Rob Herring
  2021-07-26 15:59     ` Alex Elder
  0 siblings, 1 reply; 12+ messages in thread
From: Rob Herring @ 2021-07-23 20:52 UTC (permalink / raw)
  To: Alex Elder
  Cc: bjorn.andersson, agross, davem, kuba, evgreen, cpratapa,
	subashab, elder, linux-arm-msm, netdev, devicetree, linux-kernel

On Mon, Jul 19, 2021 at 04:24:54PM -0500, Alex Elder wrote:
> On some newer SoCs, the interconnect between IPA and SoC internal
> memory (imem) is not used.  Reflect this in the binding by moving
> the definition of the "imem" interconnect to the end and defining
> minItems to be 2 for both the interconnects and interconnect-names
> properties.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  .../devicetree/bindings/net/qcom,ipa.yaml      | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> index ed88ba4b94df5..4853ab7017bd9 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> @@ -87,16 +87,18 @@ properties:
>        - const: ipa-setup-ready
>  
>    interconnects:
> +    minItems: 2
>      items:
> -      - description: Interconnect path between IPA and main memory
> -      - description: Interconnect path between IPA and internal memory
> -      - description: Interconnect path between IPA and the AP subsystem
> +      - description: Path leading to system memory
> +      - description: Path between the AP and IPA config space
> +      - description: Path leading to internal memory
>  
>    interconnect-names:
> +    minItems: 2
>      items:
>        - const: memory
> -      - const: imem
>        - const: config
> +      - const: imem

What about existing users? This will generate warnings. Doing this for 
the 2nd item would avoid the need for .dts updates:

- enum: [ imem, config ]

Rob

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 0/3] arm64: dts: qcom: DTS updates
  2021-07-20 15:59   ` Bjorn Andersson
@ 2021-07-26 15:44     ` Alex Elder
  0 siblings, 0 replies; 12+ messages in thread
From: Alex Elder @ 2021-07-26 15:44 UTC (permalink / raw)
  To: Bjorn Andersson, davem, kuba
  Cc: Alex Elder, agross, robh+dt, evgreen, cpratapa, subashab, elder,
	linux-arm-msm, netdev, devicetree, linux-kernel

On 7/20/21 10:59 AM, Bjorn Andersson wrote:
> On Tue 20 Jul 09:20 CDT 2021, patchwork-bot+netdevbpf@kernel.org wrote:
> 
>> Hello:
>>
>> This series was applied to netdev/net-next.git (refs/heads/master):
>>
> 
> David, Jakub, can you please revert/drop the two "arm64: dts" patches
> from the net-next tree?

David, I intended for this series to go through the Qualcomm repository
rather than net-next, to avoid any conflicts with other updates to the
affected DTS file.  The only indication I made was by having you and
Jakub in the "Cc" list rather than "To" list; in the future I will be
more obvious in the cover page.

Would you please revert the entire merge, so that these commits can
go through the Qualcomm repository?  These are the commits (in order):
   6a0eb6c9d9341 dt-bindings: net: qcom,ipa: make imem interconnect optional
   f8bd3c82bf7d7 arm64: dts: qcom: sc7280: add IPA information
   fd0f72c34bd96 arm64: dts: qcom: sc7180: define ipa_fw_mem node
   b79c6fba6cd7c Merge branch 'qcom-dts-updates'

If there is another way you think this should be handled, please
explain.  Thanks.

					-Alex


> 
> DTS patches are generally merged through the qcom and ultimately soc
> tree and I have a number of patches queued up in both sc7180 and sc7280
> that will cause merge conflicts down the road, so I would prefer to pick
> these up as well.
> 
> Regards,
> Bjorn
> 
>> On Mon, 19 Jul 2021 16:24:53 -0500 you wrote:
>>> This series updates some IPA-related DT nodes.
>>>
>>> Newer versions of IPA do not require an interconnect between IPA
>>> and SoC internal memory.  The first patch updates the DT binding
>>> to reflect this.
>>>
>>> The second patch adds IPA information to "sc7280.dtsi", using only
>>> two interconnects.  It includes the definition of the reserved
>>> memory area used to hold IPA firmware.
>>>
>>> [...]
>>
>> Here is the summary with links:
>>    - [net-next,1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional
>>      https://git.kernel.org/netdev/net-next/c/6a0eb6c9d934
>>    - [net-next,2/3] arm64: dts: qcom: sc7280: add IPA information
>>      https://git.kernel.org/netdev/net-next/c/f8bd3c82bf7d
>>    - [net-next,3/3] arm64: dts: qcom: sc7180: define ipa_fw_mem node
>>      https://git.kernel.org/netdev/net-next/c/fd0f72c34bd9
>>
>> You are awesome, thank you!
>> --
>> Deet-doot-dot, I am a bot.
>> https://korg.docs.kernel.org/patchwork/pwbot.html
>>
>>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional
  2021-07-23 20:52   ` Rob Herring
@ 2021-07-26 15:59     ` Alex Elder
  2021-07-28 15:33       ` Rob Herring
  0 siblings, 1 reply; 12+ messages in thread
From: Alex Elder @ 2021-07-26 15:59 UTC (permalink / raw)
  To: Rob Herring, Alex Elder
  Cc: bjorn.andersson, agross, davem, kuba, evgreen, cpratapa,
	subashab, elder, linux-arm-msm, netdev, devicetree, linux-kernel

On 7/23/21 3:52 PM, Rob Herring wrote:
> On Mon, Jul 19, 2021 at 04:24:54PM -0500, Alex Elder wrote:
>> On some newer SoCs, the interconnect between IPA and SoC internal
>> memory (imem) is not used.  Reflect this in the binding by moving
>> the definition of the "imem" interconnect to the end and defining
>> minItems to be 2 for both the interconnects and interconnect-names
>> properties.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>   .../devicetree/bindings/net/qcom,ipa.yaml      | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>> index ed88ba4b94df5..4853ab7017bd9 100644
>> --- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>> +++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>> @@ -87,16 +87,18 @@ properties:
>>         - const: ipa-setup-ready
>>   
>>     interconnects:
>> +    minItems: 2
>>       items:
>> -      - description: Interconnect path between IPA and main memory
>> -      - description: Interconnect path between IPA and internal memory
>> -      - description: Interconnect path between IPA and the AP subsystem
>> +      - description: Path leading to system memory
>> +      - description: Path between the AP and IPA config space
>> +      - description: Path leading to internal memory
>>   
>>     interconnect-names:
>> +    minItems: 2
>>       items:
>>         - const: memory
>> -      - const: imem
>>         - const: config
>> +      - const: imem
> 
> What about existing users? This will generate warnings. Doing this for
> the 2nd item would avoid the need for .dts updates:
> 
> - enum: [ imem, config ]

If I understand correctly, the effect of this would be that
the second item can either be "imem" or "config", and the third
(if present) could only be "imem"?

And you're saying that otherwise, existing users (the only
one it applies to at the moment is "sdm845.dtsi") would
produce warnings, because the interconnects are listed
in an order different from what the binding specifies.

Is that correct?

If so, what you propose suggests "imem" could be listed twice.
It doesn't make sense, and maybe it's precluded in other ways
so that's OK.  But I'd be happy to update "sdm845.dtsi" to
address your concern.  (Maybe that's something you would rather
avoid?)

Also, I need to make a separate update to "sm8350.dtsi" because
that was defined before I understood what I do now about the 
interconnects.  It uses the wrong names, and should combine
its first two interconnects into just one.

					-Alex

> 
> Rob
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional
  2021-07-26 15:59     ` Alex Elder
@ 2021-07-28 15:33       ` Rob Herring
  2021-08-03 12:24         ` Alex Elder
  0 siblings, 1 reply; 12+ messages in thread
From: Rob Herring @ 2021-07-28 15:33 UTC (permalink / raw)
  To: Alex Elder
  Cc: Alex Elder, Bjorn Andersson, Gross, Andy, David Miller,
	Jakub Kicinski, Evan Green, cpratapa, subashab, Alex Elder,
	linux-arm-msm, netdev, devicetree, linux-kernel

On Mon, Jul 26, 2021 at 9:59 AM Alex Elder <elder@ieee.org> wrote:
>
> On 7/23/21 3:52 PM, Rob Herring wrote:
> > On Mon, Jul 19, 2021 at 04:24:54PM -0500, Alex Elder wrote:
> >> On some newer SoCs, the interconnect between IPA and SoC internal
> >> memory (imem) is not used.  Reflect this in the binding by moving
> >> the definition of the "imem" interconnect to the end and defining
> >> minItems to be 2 for both the interconnects and interconnect-names
> >> properties.
> >>
> >> Signed-off-by: Alex Elder <elder@linaro.org>
> >> ---
> >>   .../devicetree/bindings/net/qcom,ipa.yaml      | 18 ++++++++++--------
> >>   1 file changed, 10 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> >> index ed88ba4b94df5..4853ab7017bd9 100644
> >> --- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> >> +++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> >> @@ -87,16 +87,18 @@ properties:
> >>         - const: ipa-setup-ready
> >>
> >>     interconnects:
> >> +    minItems: 2
> >>       items:
> >> -      - description: Interconnect path between IPA and main memory
> >> -      - description: Interconnect path between IPA and internal memory
> >> -      - description: Interconnect path between IPA and the AP subsystem
> >> +      - description: Path leading to system memory
> >> +      - description: Path between the AP and IPA config space
> >> +      - description: Path leading to internal memory
> >>
> >>     interconnect-names:
> >> +    minItems: 2
> >>       items:
> >>         - const: memory
> >> -      - const: imem
> >>         - const: config
> >> +      - const: imem
> >
> > What about existing users? This will generate warnings. Doing this for
> > the 2nd item would avoid the need for .dts updates:
> >
> > - enum: [ imem, config ]
>
> If I understand correctly, the effect of this would be that
> the second item can either be "imem" or "config", and the third
> (if present) could only be "imem"?

Yes for the 2nd, but the 3rd item could only be 'config'.

>
> And you're saying that otherwise, existing users (the only
> one it applies to at the moment is "sdm845.dtsi") would
> produce warnings, because the interconnects are listed
> in an order different from what the binding specifies.
>
> Is that correct?

Yes.

> If so, what you propose suggests "imem" could be listed twice.
> It doesn't make sense, and maybe it's precluded in other ways
> so that's OK.

Good observation. There are generic checks that the strings are unique.

>  But I'd be happy to update "sdm845.dtsi" to
> address your concern.  (Maybe that's something you would rather
> avoid?)

Better to not change DT if you don't have to. You're probably okay if
all clients (consumers of the dtb) used names and didn't care about
the order. And I have no idea if all users of SDM845 are okay with a
DTB change being required. That's up to QCom maintainers. I only care
that ABI breakages are documented as such.

> Also, I need to make a separate update to "sm8350.dtsi" because
> that was defined before I understood what I do now about the
> interconnects.  It uses the wrong names, and should combine
> its first two interconnects into just one.

If the interconnects was ignored in that case, then the change doesn't matter.

Rob

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional
  2021-07-28 15:33       ` Rob Herring
@ 2021-08-03 12:24         ` Alex Elder
  2021-08-03 18:05           ` Rob Herring
  0 siblings, 1 reply; 12+ messages in thread
From: Alex Elder @ 2021-08-03 12:24 UTC (permalink / raw)
  To: Rob Herring
  Cc: Alex Elder, Bjorn Andersson, Gross, Andy, David Miller,
	Jakub Kicinski, Evan Green, cpratapa, subashab, Alex Elder,
	linux-arm-msm, netdev, devicetree, linux-kernel

On 7/28/21 10:33 AM, Rob Herring wrote:
> On Mon, Jul 26, 2021 at 9:59 AM Alex Elder <elder@ieee.org> wrote:
>>
>> On 7/23/21 3:52 PM, Rob Herring wrote:
>>> On Mon, Jul 19, 2021 at 04:24:54PM -0500, Alex Elder wrote:
>>>> On some newer SoCs, the interconnect between IPA and SoC internal
>>>> memory (imem) is not used.  Reflect this in the binding by moving
>>>> the definition of the "imem" interconnect to the end and defining
>>>> minItems to be 2 for both the interconnects and interconnect-names
>>>> properties.
>>>>
>>>> Signed-off-by: Alex Elder <elder@linaro.org>
>>>> ---
>>>>    .../devicetree/bindings/net/qcom,ipa.yaml      | 18 ++++++++++--------
>>>>    1 file changed, 10 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>>>> index ed88ba4b94df5..4853ab7017bd9 100644
>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>>>> @@ -87,16 +87,18 @@ properties:
>>>>          - const: ipa-setup-ready
>>>>
>>>>      interconnects:
>>>> +    minItems: 2
>>>>        items:
>>>> -      - description: Interconnect path between IPA and main memory
>>>> -      - description: Interconnect path between IPA and internal memory
>>>> -      - description: Interconnect path between IPA and the AP subsystem
>>>> +      - description: Path leading to system memory
>>>> +      - description: Path between the AP and IPA config space
>>>> +      - description: Path leading to internal memory
>>>>
>>>>      interconnect-names:
>>>> +    minItems: 2
>>>>        items:
>>>>          - const: memory
>>>> -      - const: imem
>>>>          - const: config
>>>> +      - const: imem
>>>
>>> What about existing users? This will generate warnings. Doing this for
>>> the 2nd item would avoid the need for .dts updates:
>>>
>>> - enum: [ imem, config ]

In other words:

   interconnect-names:
     minItems: 2
     items:
       - const: memory
       - enum: [ imem, config ]
       - const: imem

What do I do with the "interconnects" descriptions in that case?
How do I make the "interconnect-names" specified this way align
with the described interconnect values?  Is that necessary?

>> If I understand correctly, the effect of this would be that
>> the second item can either be "imem" or "config", and the third
>> (if present) could only be "imem"?
> 
> Yes for the 2nd, but the 3rd item could only be 'config'.

Sorry, yes, that's what I meant.  I might have misread the
diff output.

>> And you're saying that otherwise, existing users (the only
>> one it applies to at the moment is "sdm845.dtsi") would
>> produce warnings, because the interconnects are listed
>> in an order different from what the binding specifies.
>>
>> Is that correct?
> 
> Yes.
> 
>> If so, what you propose suggests "imem" could be listed twice.
>> It doesn't make sense, and maybe it's precluded in other ways
>> so that's OK.
> 
> Good observation. There are generic checks that the strings are unique.

I think I don't like that quite as much, because that
"no duplicates" rule is implied.  It also avoids any
confusion in the "respectively" relationship between
interconnects and interconnect-names.

I understand what you're suggesting though, and I would
be happy to update the binding in the way you suggest.
I'd like to hear what you say about my questions above
before doing so.

>>   But I'd be happy to update "sdm845.dtsi" to
>> address your concern.  (Maybe that's something you would rather
>> avoid?)
> 
> Better to not change DT if you don't have to. You're probably okay if
> all clients (consumers of the dtb) used names and didn't care about

In the IPA driver, wherever names are specified for things in DT,
names (only) are used to look them up.  So I'm "probably okay."

> the order. And I have no idea if all users of SDM845 are okay with a
> DTB change being required. That's up to QCom maintainers. I only care
> that ABI breakages are documented as such.
> 
>> Also, I need to make a separate update to "sm8350.dtsi" because
>> that was defined before I understood what I do now about the
>> interconnects.  It uses the wrong names, and should combine
>> its first two interconnects into just one.
> 
> If the interconnects was ignored in that case, then the change doesn't matter.

That platform is not yet fully supported by the IPA driver, thus
there is (so far) no instance where it is used.  Resolving this
is part of enabling support for that.

Thanks.

					-Alex


> Rob
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional
  2021-08-03 12:24         ` Alex Elder
@ 2021-08-03 18:05           ` Rob Herring
  0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2021-08-03 18:05 UTC (permalink / raw)
  To: Alex Elder
  Cc: Alex Elder, Bjorn Andersson, Gross, Andy, David Miller,
	Jakub Kicinski, Evan Green, cpratapa, subashab, Alex Elder,
	linux-arm-msm, netdev, devicetree, linux-kernel

On Tue, Aug 3, 2021 at 6:24 AM Alex Elder <elder@ieee.org> wrote:
>
> On 7/28/21 10:33 AM, Rob Herring wrote:
> > On Mon, Jul 26, 2021 at 9:59 AM Alex Elder <elder@ieee.org> wrote:
> >>
> >> On 7/23/21 3:52 PM, Rob Herring wrote:
> >>> On Mon, Jul 19, 2021 at 04:24:54PM -0500, Alex Elder wrote:
> >>>> On some newer SoCs, the interconnect between IPA and SoC internal
> >>>> memory (imem) is not used.  Reflect this in the binding by moving
> >>>> the definition of the "imem" interconnect to the end and defining
> >>>> minItems to be 2 for both the interconnects and interconnect-names
> >>>> properties.
> >>>>
> >>>> Signed-off-by: Alex Elder <elder@linaro.org>
> >>>> ---
> >>>>    .../devicetree/bindings/net/qcom,ipa.yaml      | 18 ++++++++++--------
> >>>>    1 file changed, 10 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> >>>> index ed88ba4b94df5..4853ab7017bd9 100644
> >>>> --- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> >>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> >>>> @@ -87,16 +87,18 @@ properties:
> >>>>          - const: ipa-setup-ready
> >>>>
> >>>>      interconnects:
> >>>> +    minItems: 2
> >>>>        items:
> >>>> -      - description: Interconnect path between IPA and main memory
> >>>> -      - description: Interconnect path between IPA and internal memory
> >>>> -      - description: Interconnect path between IPA and the AP subsystem
> >>>> +      - description: Path leading to system memory
> >>>> +      - description: Path between the AP and IPA config space
> >>>> +      - description: Path leading to internal memory
> >>>>
> >>>>      interconnect-names:
> >>>> +    minItems: 2
> >>>>        items:
> >>>>          - const: memory
> >>>> -      - const: imem
> >>>>          - const: config
> >>>> +      - const: imem
> >>>
> >>> What about existing users? This will generate warnings. Doing this for
> >>> the 2nd item would avoid the need for .dts updates:
> >>>
> >>> - enum: [ imem, config ]
>
> In other words:
>
>    interconnect-names:
>      minItems: 2
>      items:
>        - const: memory
>        - enum: [ imem, config ]
>        - const: imem
>
> What do I do with the "interconnects" descriptions in that case?
> How do I make the "interconnect-names" specified this way align
> with the described interconnect values?  Is that necessary?

The schema will only ever check or care that there are 2 or 3 entries.
You can put whatever you want for the description. Or use 'oneOf' to
have the 2 entry and 3 entry cases.

> >> If I understand correctly, the effect of this would be that
> >> the second item can either be "imem" or "config", and the third
> >> (if present) could only be "imem"?
> >
> > Yes for the 2nd, but the 3rd item could only be 'config'.
>
> Sorry, yes, that's what I meant.  I might have misread the
> diff output.
>
> >> And you're saying that otherwise, existing users (the only
> >> one it applies to at the moment is "sdm845.dtsi") would
> >> produce warnings, because the interconnects are listed
> >> in an order different from what the binding specifies.
> >>
> >> Is that correct?
> >
> > Yes.
> >
> >> If so, what you propose suggests "imem" could be listed twice.
> >> It doesn't make sense, and maybe it's precluded in other ways
> >> so that's OK.
> >
> > Good observation. There are generic checks that the strings are unique.
>
> I think I don't like that quite as much, because that
> "no duplicates" rule is implied.  It also avoids any
> confusion in the "respectively" relationship between
> interconnects and interconnect-names.

We could be verbose about it using the 'uniqueItems' keyword, but I
try to make the bindings follow the common case (for DT) rather than
the default for json-schema. If you think about the purpose of
'*-names', non-unique entries would never make sense as the purpose is
to identify what each entry is.

> I understand what you're suggesting though, and I would
> be happy to update the binding in the way you suggest.
> I'd like to hear what you say about my questions above
> before doing so.
>
> >>   But I'd be happy to update "sdm845.dtsi" to
> >> address your concern.  (Maybe that's something you would rather
> >> avoid?)
> >
> > Better to not change DT if you don't have to. You're probably okay if
> > all clients (consumers of the dtb) used names and didn't care about
>
> In the IPA driver, wherever names are specified for things in DT,
> names (only) are used to look them up.  So I'm "probably okay."

Yes, probably so. It's ultimately up to you and the Qcom maintainers
what to do. I'm just providing how you could avoid the change.

Rob

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-08-03 18:06 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-19 21:24 [PATCH net-next 0/3] arm64: dts: qcom: DTS updates Alex Elder
2021-07-19 21:24 ` [PATCH net-next 1/3] dt-bindings: net: qcom,ipa: make imem interconnect optional Alex Elder
2021-07-23 20:52   ` Rob Herring
2021-07-26 15:59     ` Alex Elder
2021-07-28 15:33       ` Rob Herring
2021-08-03 12:24         ` Alex Elder
2021-08-03 18:05           ` Rob Herring
2021-07-19 21:24 ` [PATCH net-next 2/3] arm64: dts: qcom: sc7280: add IPA information Alex Elder
2021-07-19 21:24 ` [PATCH net-next 3/3] arm64: dts: qcom: sc7180: define ipa_fw_mem node Alex Elder
2021-07-20 14:20 ` [PATCH net-next 0/3] arm64: dts: qcom: DTS updates patchwork-bot+netdevbpf
2021-07-20 15:59   ` Bjorn Andersson
2021-07-26 15:44     ` Alex Elder

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).