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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> .../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
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 <[email protected]>
>> ---
>> .../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
>
On Mon, Jul 26, 2021 at 9:59 AM Alex Elder <[email protected]> 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 <[email protected]>
> >> ---
> >> .../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
On 7/28/21 10:33 AM, Rob Herring wrote:
> On Mon, Jul 26, 2021 at 9:59 AM Alex Elder <[email protected]> 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 <[email protected]>
>>>> ---
>>>> .../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
>
On Tue, Aug 3, 2021 at 6:24 AM Alex Elder <[email protected]> wrote:
>
> On 7/28/21 10:33 AM, Rob Herring wrote:
> > On Mon, Jul 26, 2021 at 9:59 AM Alex Elder <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> .../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