| Message ID | 20230929133754.857678-2-lcherian@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] dt-bindings: arm: coresight-tmc: Add "memory-region" property | expand |
Hi Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Tuesday, October 3, 2023 12:02 PM > To: Linu Cherian <lcherian@marvell.com>; suzuki.poulose@arm.com; > mike.leach@linaro.org; james.clark@arm.com; leo.yan@linaro.org > Cc: linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux- > kernel@vger.kernel.org; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; > devicetree@vger.kernel.org; Sunil Kovvuri Goutham > <sgoutham@marvell.com>; George Cherian <gcherian@marvell.com> > Subject: Re: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add > "memory-region" property > > On 03/10/2023 06:33, Linu Cherian wrote: > > Hi Krzysztof, > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Sent: Saturday, September 30, 2023 8:59 PM > >> To: Linu Cherian <lcherian@marvell.com>; suzuki.poulose@arm.com; > >> mike.leach@linaro.org; james.clark@arm.com; leo.yan@linaro.org > >> Cc: linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; > >> linux- kernel@vger.kernel.org; robh+dt@kernel.org; > >> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; > >> devicetree@vger.kernel.org; Sunil Kovvuri Goutham > >> <sgoutham@marvell.com>; George Cherian <gcherian@marvell.com> > >> Subject: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add > >> "memory-region" property > >> > >> External Email > >> > >> --------------------------------------------------------------------- > >> - On 29/09/2023 15:37, Linu Cherian wrote: > >>> memory-region 0: Reserved trace buffer memory > >>> > >>> TMC ETR: When available, use this reserved memory region for > >>> trace data capture. Same region is used for trace data > >>> retention after a panic or watchdog reset. > >>> > >>> TMC ETF: When available, use this reserved memory region for > >>> trace data retention synced from internal SRAM after a panic or > >>> watchdog reset. > >>> > >>> memory-region 1: Reserved meta data memory > >>> > >>> TMC ETR, ETF: When available, use this memory for register > >>> snapshot retention synced from hardware registers after a panic > >>> or watchdog reset. > >>> > >>> Signed-off-by: Linu Cherian <lcherian@marvell.com> > >>> --- > >> > >> Where is the changelog? This is supposed to be v4 or something later. > >> Please, keep proper versioning and changelog. > > > > Sure, will add the changelog from next version onwards. > > > > Yeah, the last version was RFC v3 and the RFC tag has been dropped from > this version onwards. > > Hence started this version with V1. > > v1 says it is the first version, but you already had three others. > Please keep continuous version log, regardless whether you call it RFC or RFT > or RFsomething. Okay. Will continue with Patch V5 from next series and add necessary notes in cover letter. > > > > >> > >>> .../bindings/arm/arm,coresight-tmc.yaml | 19 > +++++++++++++++++++ > >>> 1 file changed, 19 insertions(+) > >>> > > Best regards, > Krzysztof
Hi Linu On Fri, 29 Sept 2023 at 14:38, Linu Cherian <lcherian@marvell.com> wrote: > > memory-region 0: Reserved trace buffer memory > > TMC ETR: When available, use this reserved memory region for > trace data capture. Same region is used for trace data > retention after a panic or watchdog reset. > > TMC ETF: When available, use this reserved memory region for > trace data retention synced from internal SRAM after a panic or > watchdog reset. > > memory-region 1: Reserved meta data memory > > TMC ETR, ETF: When available, use this memory for register > snapshot retention synced from hardware registers after a panic > or watchdog reset. > > Signed-off-by: Linu Cherian <lcherian@marvell.com> > --- > .../bindings/arm/arm,coresight-tmc.yaml | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml > index cb8dceaca70e..45ca4d02d73e 100644 > --- a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml > +++ b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml > @@ -101,6 +101,22 @@ properties: > and ETF configurations. > $ref: /schemas/graph.yaml#/properties/port > > + memory-region: > + items: > + - description: Reserved trace buffer memory for ETR and ETF sinks. > + For ETR, this reserved memory region is used for trace data capture. > + Same region is used for trace data retention as well after a panic > + or watchdog reset. > + For ETF, this reserved memory region is used for retention of trace > + data synced from internal SRAM after a panic or watchdog reset. > + Is there a valid use case for ETR where we use these areas when there is not a panic/reset situation? Either way - the description should perhaps mention that these areas are only used if specifically selected by the driver - the default memory usage models for ETR / perf are otherwise unaltered. > + - description: Reserved meta data memory. Used for ETR and ETF sinks. > + > + memory-region-names: > + items: > + - const: trace-mem > + - const: metadata-mem > + Is there a constraint required here? If we are using the memory area for trace in a panic situation, then we must have the meta data memory area defined? Perhaps a set of names such as "etr-trace-mem", "panic-trace-mem" , "panic-metadata-mem", were the first is for general ETR trace in non-panic situation and then constrain the "panic-" areas to appear together. The "etr-trace-mem", "panic-trace-mem" could easily point to the same area. > required: > - compatible > - reg > @@ -115,6 +131,9 @@ examples: > etr@20070000 { > compatible = "arm,coresight-tmc", "arm,primecell"; > reg = <0x20070000 0x1000>; > + memory-region = <&etr_trace_mem_reserved>, > + <&etr_mdata_mem_reserved>; > + memory-region-names = "trace-mem", "metadata-mem"; > > clocks = <&oscclk6a>; > clock-names = "apb_pclk"; > -- > 2.34.1 >
Hi Mike, > -----Original Message----- > From: Mike Leach <mike.leach@linaro.org> > Sent: Friday, October 6, 2023 4:33 PM > To: Linu Cherian <lcherian@marvell.com> > Cc: suzuki.poulose@arm.com; james.clark@arm.com; leo.yan@linaro.org; > linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux- > kernel@vger.kernel.org; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; > devicetree@vger.kernel.org; Sunil Kovvuri Goutham > <sgoutham@marvell.com>; George Cherian <gcherian@marvell.com> > Subject: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add > "memory-region" property > > External Email > > ---------------------------------------------------------------------- > Hi Linu > > On Fri, 29 Sept 2023 at 14:38, Linu Cherian <lcherian@marvell.com> wrote: > > > > memory-region 0: Reserved trace buffer memory > > > > TMC ETR: When available, use this reserved memory region for > > trace data capture. Same region is used for trace data > > retention after a panic or watchdog reset. > > > > TMC ETF: When available, use this reserved memory region for > > trace data retention synced from internal SRAM after a panic or > > watchdog reset. > > > > memory-region 1: Reserved meta data memory > > > > TMC ETR, ETF: When available, use this memory for register > > snapshot retention synced from hardware registers after a panic > > or watchdog reset. > > > > Signed-off-by: Linu Cherian <lcherian@marvell.com> > > --- > > .../bindings/arm/arm,coresight-tmc.yaml | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml > > b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml > > index cb8dceaca70e..45ca4d02d73e 100644 > > --- a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml > > +++ b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml > > @@ -101,6 +101,22 @@ properties: > > and ETF configurations. > > $ref: /schemas/graph.yaml#/properties/port > > > > + memory-region: > > + items: > > + - description: Reserved trace buffer memory for ETR and ETF sinks. > > + For ETR, this reserved memory region is used for trace data capture. > > + Same region is used for trace data retention as well after a panic > > + or watchdog reset. > > + For ETF, this reserved memory region is used for retention of trace > > + data synced from internal SRAM after a panic or watchdog reset. > > + > > Is there a valid use case for ETR where we use these areas when there is not > a panic/reset situation? > Currently its meant to be used only for the panic/reset situations and gets enabled only with a special buffer mode. > Either way - the description should perhaps mention that these areas are > only used if specifically selected by the driver - the default memory usage > models for ETR / perf are otherwise unaltered. Ack. > > > + - description: Reserved meta data memory. Used for ETR and ETF sinks. > > + > > + memory-region-names: > > + items: > > + - const: trace-mem > > + - const: metadata-mem > > + > > Is there a constraint required here? If we are using the memory area for > trace in a panic situation, then we must have the meta data memory area > defined? > Perhaps a set of names such as "etr-trace-mem", "panic-trace-mem" , > "panic-metadata-mem", were the first is for general ETR trace in non-panic > situation and then constrain the "panic-" areas to appear together. > The "etr-trace-mem", "panic-trace-mem" could easily point to the same > area. > As noted above, we do not have other generic use case for these reserved regions now. Hence two regions/names, panic-trace-mem and panic-metadata-mem with constraints kept as minItems: 2 and maxItems: 2 would suffice ? > > required: > > - compatible > > - reg > > @@ -115,6 +131,9 @@ examples: > > etr@20070000 { > > compatible = "arm,coresight-tmc", "arm,primecell"; > > reg = <0x20070000 0x1000>; > > + memory-region = <&etr_trace_mem_reserved>, > > + <&etr_mdata_mem_reserved>; > > + memory-region-names = "trace-mem", "metadata-mem"; > > > > clocks = <&oscclk6a>; > > clock-names = "apb_pclk"; > > -- > > 2.34.1 > > > > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK
On Sat, Oct 14, 2023 at 11:36:37AM +0000, Linu Cherian wrote: > > > + - description: Reserved meta data memory. Used for ETR and ETF sinks. > > > + > > > + memory-region-names: > > > + items: > > > + - const: trace-mem > > > + - const: metadata-mem > > > + > > > > Is there a constraint required here? If we are using the memory area for > > trace in a panic situation, then we must have the meta data memory area > > defined? > > Perhaps a set of names such as "etr-trace-mem", "panic-trace-mem" , > > "panic-metadata-mem", were the first is for general ETR trace in non-panic > > situation and then constrain the "panic-" areas to appear together. > > The "etr-trace-mem", "panic-trace-mem" could easily point to the same > > area. > > > As noted above, we do not have other generic use case for these reserved regions now. > Hence two regions/names, panic-trace-mem and panic-metadata-mem with constraints kept as > minItems: 2 and maxItems: 2 would suffice ? Whatever you do, please delete the -mem suffix.
Hi Conor, > -----Original Message----- > From: Conor Dooley <conor@kernel.org> > Sent: Saturday, October 14, 2023 6:56 PM > To: Linu Cherian <lcherian@marvell.com> > Cc: Mike Leach <mike.leach@linaro.org>; suzuki.poulose@arm.com; > james.clark@arm.com; leo.yan@linaro.org; linux-arm- > kernel@lists.infradead.org; coresight@lists.linaro.org; linux- > kernel@vger.kernel.org; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; > devicetree@vger.kernel.org; Sunil Kovvuri Goutham > <sgoutham@marvell.com>; George Cherian <gcherian@marvell.com> > Subject: Re: [EXT] Re: [PATCH 1/7] dt-bindings: arm: coresight-tmc: Add > "memory-region" property > > On Sat, Oct 14, 2023 at 11:36:37AM +0000, Linu Cherian wrote: > > > > + - description: Reserved meta data memory. Used for ETR and ETF > sinks. > > > > + > > > > + memory-region-names: > > > > + items: > > > > + - const: trace-mem > > > > + - const: metadata-mem > > > > + > > > > > > Is there a constraint required here? If we are using the memory area > > > for trace in a panic situation, then we must have the meta data > > > memory area defined? > > > Perhaps a set of names such as "etr-trace-mem", "panic-trace-mem" , > > > "panic-metadata-mem", were the first is for general ETR trace in > > > non-panic situation and then constrain the "panic-" areas to appear > together. > > > The "etr-trace-mem", "panic-trace-mem" could easily point to the > > > same area. > > > > > As noted above, we do not have other generic use case for these reserved > regions now. > > Hence two regions/names, panic-trace-mem and panic-metadata-mem > with > > constraints kept as > > minItems: 2 and maxItems: 2 would suffice ? > > Whatever you do, please delete the -mem suffix. Ack.
diff --git a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml index cb8dceaca70e..45ca4d02d73e 100644 --- a/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml +++ b/Documentation/devicetree/bindings/arm/arm,coresight-tmc.yaml @@ -101,6 +101,22 @@ properties: and ETF configurations. $ref: /schemas/graph.yaml#/properties/port + memory-region: + items: + - description: Reserved trace buffer memory for ETR and ETF sinks. + For ETR, this reserved memory region is used for trace data capture. + Same region is used for trace data retention as well after a panic + or watchdog reset. + For ETF, this reserved memory region is used for retention of trace + data synced from internal SRAM after a panic or watchdog reset. + + - description: Reserved meta data memory. Used for ETR and ETF sinks. + + memory-region-names: + items: + - const: trace-mem + - const: metadata-mem + required: - compatible - reg @@ -115,6 +131,9 @@ examples: etr@20070000 { compatible = "arm,coresight-tmc", "arm,primecell"; reg = <0x20070000 0x1000>; + memory-region = <&etr_trace_mem_reserved>, + <&etr_mdata_mem_reserved>; + memory-region-names = "trace-mem", "metadata-mem"; clocks = <&oscclk6a>; clock-names = "apb_pclk";
memory-region 0: Reserved trace buffer memory TMC ETR: When available, use this reserved memory region for trace data capture. Same region is used for trace data retention after a panic or watchdog reset. TMC ETF: When available, use this reserved memory region for trace data retention synced from internal SRAM after a panic or watchdog reset. memory-region 1: Reserved meta data memory TMC ETR, ETF: When available, use this memory for register snapshot retention synced from hardware registers after a panic or watchdog reset. Signed-off-by: Linu Cherian <lcherian@marvell.com> --- .../bindings/arm/arm,coresight-tmc.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)