| Message ID | 20220406052459.10438-4-amhetre@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | memory: tegra: Add MC channels and error logging | expand |
06.04.2022 08:24, Ashish Mhetre пишет: > memory-controller@2c00000 { > compatible = "nvidia,tegra186-mc"; > - reg = <0x0 0x02c00000 0x0 0xb0000>; > + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ > + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast channel */ > + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ > + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ > + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ > + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ > + reg-names = "mc-sid", "mc-broadcast", "mc0", "mc1", "mc2", "mc3"; The "mc-" prefix feels redundant to me, I'd name the regs like this: "sid", "broadcast", "ch0", "ch1", "ch2", "ch3" You should also add validation of the regs/reg-names to the yaml based on SoC version. I.e. it's not enough to only bump the maxItems.
On 4/10/2022 7:51 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2022 08:24, Ashish Mhetre пишет: >> memory-controller@2c00000 { >> compatible = "nvidia,tegra186-mc"; >> - reg = <0x0 0x02c00000 0x0 0xb0000>; >> + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ >> + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast channel */ >> + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ >> + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ >> + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ >> + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ >> + reg-names = "mc-sid", "mc-broadcast", "mc0", "mc1", "mc2", "mc3"; > > The "mc-" prefix feels redundant to me, I'd name the regs like this: > > "sid", "broadcast", "ch0", "ch1", "ch2", "ch3" > Okay, I'll update the reg-names by omitting "mc-". > > You should also add validation of the regs/reg-names to the yaml based > on SoC version. I.e. it's not enough to only bump the maxItems. Okay, I'll add validation of reg-names as well.
On 4/10/2022 7:51 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 06.04.2022 08:24, Ashish Mhetre пишет: >> memory-controller@2c00000 { >> compatible = "nvidia,tegra186-mc"; >> - reg = <0x0 0x02c00000 0x0 0xb0000>; >> + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ >> + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast channel */ >> + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ >> + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ >> + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ >> + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ >> + reg-names = "mc-sid", "mc-broadcast", "mc0", "mc1", "mc2", "mc3"; > > The "mc-" prefix feels redundant to me, I'd name the regs like this: > > "sid", "broadcast", "ch0", "ch1", "ch2", "ch3" > > > You should also add validation of the regs/reg-names to the yaml based > on SoC version. I.e. it's not enough to only bump the maxItems. Okay, I will add validation of reg-names as following: reg-names: minItems: 0 maxItems: 6 items: - const: sid - const: broadcast - const: ch0 - const: ch1 - const: ch2 - const: ch3 We will have to keep minItems to 0 in order to make it compatible with old DT, right?
On 4/11/22 18:02, Ashish Mhetre wrote: > > > On 4/10/2022 7:51 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 06.04.2022 08:24, Ashish Mhetre пишет: >>> memory-controller@2c00000 { >>> compatible = "nvidia,tegra186-mc"; >>> - reg = <0x0 0x02c00000 0x0 0xb0000>; >>> + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ >>> + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast >>> channel */ >>> + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ >>> + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ >>> + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ >>> + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ >>> + reg-names = "mc-sid", "mc-broadcast", "mc0", "mc1", >>> "mc2", "mc3"; >> >> The "mc-" prefix feels redundant to me, I'd name the regs like this: >> >> "sid", "broadcast", "ch0", "ch1", "ch2", "ch3" >> >> >> You should also add validation of the regs/reg-names to the yaml based >> on SoC version. I.e. it's not enough to only bump the maxItems. > > Okay, I will add validation of reg-names as following: > > reg-names: > minItems: 0 > maxItems: 6 > items: > - const: sid > - const: broadcast > - const: ch0 > - const: ch1 > - const: ch2 > - const: ch3 > > > We will have to keep minItems to 0 in order to make it compatible with > old DT, right? Bindings are about the latest DTs. In general older dtbs must be updated and you must get error from the schema checker for older DTs. It's only drivers that should care about older dtbs.
On 4/11/2022 8:59 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > On 4/11/22 18:02, Ashish Mhetre wrote: >> >> >> On 4/10/2022 7:51 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 06.04.2022 08:24, Ashish Mhetre пишет: >>>> memory-controller@2c00000 { >>>> compatible = "nvidia,tegra186-mc"; >>>> - reg = <0x0 0x02c00000 0x0 0xb0000>; >>>> + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ >>>> + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast >>>> channel */ >>>> + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ >>>> + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ >>>> + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ >>>> + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ >>>> + reg-names = "mc-sid", "mc-broadcast", "mc0", "mc1", >>>> "mc2", "mc3"; >>> >>> The "mc-" prefix feels redundant to me, I'd name the regs like this: >>> >>> "sid", "broadcast", "ch0", "ch1", "ch2", "ch3" >>> >>> >>> You should also add validation of the regs/reg-names to the yaml based >>> on SoC version. I.e. it's not enough to only bump the maxItems. >> >> Okay, I will add validation of reg-names as following: >> >> reg-names: >> minItems: 0 >> maxItems: 6 >> items: >> - const: sid >> - const: broadcast >> - const: ch0 >> - const: ch1 >> - const: ch2 >> - const: ch3 >> >> >> We will have to keep minItems to 0 in order to make it compatible with >> old DT, right? > > Bindings are about the latest DTs. In general older dtbs must be updated > and you must get error from the schema checker for older DTs. It's only > drivers that should care about older dtbs. On v5 Krzysztof mentioned that old DTS will start failing with new bindings https://lkml.org/lkml/2022/3/22/907. So I just wanted to confirm whether it's fine if updated bindings start to fail with old DTS?
On 4/11/22 18:41, Ashish Mhetre wrote: > > > On 4/11/2022 8:59 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> On 4/11/22 18:02, Ashish Mhetre wrote: >>> >>> >>> On 4/10/2022 7:51 PM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 06.04.2022 08:24, Ashish Mhetre пишет: >>>>> memory-controller@2c00000 { >>>>> compatible = "nvidia,tegra186-mc"; >>>>> - reg = <0x0 0x02c00000 0x0 0xb0000>; >>>>> + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ >>>>> + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast >>>>> channel */ >>>>> + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ >>>>> + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ >>>>> + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ >>>>> + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ >>>>> + reg-names = "mc-sid", "mc-broadcast", "mc0", "mc1", >>>>> "mc2", "mc3"; >>>> >>>> The "mc-" prefix feels redundant to me, I'd name the regs like this: >>>> >>>> "sid", "broadcast", "ch0", "ch1", "ch2", "ch3" >>>> >>>> >>>> You should also add validation of the regs/reg-names to the yaml based >>>> on SoC version. I.e. it's not enough to only bump the maxItems. >>> >>> Okay, I will add validation of reg-names as following: >>> >>> reg-names: >>> minItems: 0 >>> maxItems: 6 >>> items: >>> - const: sid >>> - const: broadcast >>> - const: ch0 >>> - const: ch1 >>> - const: ch2 >>> - const: ch3 >>> >>> >>> We will have to keep minItems to 0 in order to make it compatible with >>> old DT, right? >> >> Bindings are about the latest DTs. In general older dtbs must be updated >> and you must get error from the schema checker for older DTs. It's only >> drivers that should care about older dtbs. > > On v5 Krzysztof mentioned that old DTS will start failing with new > bindings https://lkml.org/lkml/2022/3/22/907. > So I just wanted to confirm whether it's fine if updated bindings > start to fail with old DTS? Since the older DT was incorrect, it's fine that the DT check will fail for it.
On 4/12/2022 3:58 AM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > On 4/11/22 18:41, Ashish Mhetre wrote: >> >> >> On 4/11/2022 8:59 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On 4/11/22 18:02, Ashish Mhetre wrote: >>>> >>>> >>>> On 4/10/2022 7:51 PM, Dmitry Osipenko wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> 06.04.2022 08:24, Ashish Mhetre пишет: >>>>>> memory-controller@2c00000 { >>>>>> compatible = "nvidia,tegra186-mc"; >>>>>> - reg = <0x0 0x02c00000 0x0 0xb0000>; >>>>>> + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ >>>>>> + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast >>>>>> channel */ >>>>>> + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ >>>>>> + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ >>>>>> + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ >>>>>> + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ >>>>>> + reg-names = "mc-sid", "mc-broadcast", "mc0", "mc1", >>>>>> "mc2", "mc3"; >>>>> >>>>> The "mc-" prefix feels redundant to me, I'd name the regs like this: >>>>> >>>>> "sid", "broadcast", "ch0", "ch1", "ch2", "ch3" >>>>> >>>>> >>>>> You should also add validation of the regs/reg-names to the yaml based >>>>> on SoC version. I.e. it's not enough to only bump the maxItems. >>>> >>>> Okay, I will add validation of reg-names as following: >>>> >>>> reg-names: >>>> minItems: 0 >>>> maxItems: 6 >>>> items: >>>> - const: sid >>>> - const: broadcast >>>> - const: ch0 >>>> - const: ch1 >>>> - const: ch2 >>>> - const: ch3 >>>> >>>> >>>> We will have to keep minItems to 0 in order to make it compatible with >>>> old DT, right? >>> >>> Bindings are about the latest DTs. In general older dtbs must be updated >>> and you must get error from the schema checker for older DTs. It's only >>> drivers that should care about older dtbs. >> >> On v5 Krzysztof mentioned that old DTS will start failing with new >> bindings https://lkml.org/lkml/2022/3/22/907. >> So I just wanted to confirm whether it's fine if updated bindings >> start to fail with old DTS? > > Since the older DT was incorrect, it's fine that the DT check will fail > for it. Thanks for confirming Dmitry. I will incorporate all comments and send v7.
diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml index 13c4c82fd0d3..0fe396a2e162 100644 --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml @@ -35,7 +35,7 @@ properties: reg: minItems: 1 - maxItems: 3 + maxItems: 18 interrupts: items: @@ -142,7 +142,8 @@ allOf: then: properties: reg: - maxItems: 1 + maxItems: 6 + description: 5 memory controller channels and 1 for stream-id registers - if: properties: @@ -152,6 +153,7 @@ allOf: properties: reg: minItems: 3 + description: 17 memory controller channels and 1 for stream-id registers - if: properties: @@ -161,6 +163,7 @@ allOf: properties: reg: minItems: 3 + description: 17 memory controller channels and 1 for stream-id registers additionalProperties: false @@ -182,7 +185,13 @@ examples: memory-controller@2c00000 { compatible = "nvidia,tegra186-mc"; - reg = <0x0 0x02c00000 0x0 0xb0000>; + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast channel */ + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ + reg-names = "mc-sid", "mc-broadcast", "mc0", "mc1", "mc2", "mc3"; interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; #address-cells = <2>;
From tegra186 onwards, memory controller support multiple channels. Reg items are updated with address and size of these channels. Tegra186 has overall 5 memory controller channels. Tegra194 and tegra234 have overall 17 memory controller channels each. There is 1 reg item for memory controller stream-id registers. So update the reg maxItems to 18 in tegra186 devicetree documentation. Signed-off-by: Ashish Mhetre <amhetre@nvidia.com> --- .../memory-controllers/nvidia,tegra186-mc.yaml | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)