| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e675a037-3c07-a8bb-19d4-781ab881c920@amd.com>
Date: Fri, 13 Jan 2023 10:04:04 -0800
From: Tanmay Shah <tanmays@....com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Tanmay Shah <tanmay.shah@....com>, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: sram: Tightly Coupled Memory (TCM) bindings
Hi Krzysztof Thanks for your reviews.
Please find my comments below.
On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
> On 13/01/2023 08:30, Tanmay Shah wrote:
>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>> driver. This bindings will help in defining TCM in device-tree and
>> make it's access platform agnostic and data-driven from the driver.
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@....com>
>> ---
>> .../devicetree/bindings/sram/xlnx,tcm.yaml | 137 ++++++++++++++++++
>> 1 file changed, 137 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>> new file mode 100644
>> index 000000000000..02d17026fb1f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>> @@ -0,0 +1,137 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/sram/xlnx,tcm.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Tightly Coupled Memory (TCM)
>> +
>> +maintainers:
>> + - Tanmay Shah <tanmay.shah@....com>
>> +
>> +description: |
>> + Tightly Coupled Memory(TCM) is available on AMD-Xilinx paltforms for ARM
>> + cortex remote processors to use. It is low-latency memory that provide
>> + predictable instruction execution and predictable data load/store timing.
>> + TCM can be configured in lockstep mode or split mode. In split mode
>> + configuration each RPU core has its own set of ATCM and BTCM memories and in
>> + lockstep mode redundant processor's TCM become available to lockstep
>> + processor. So In lockstep mode ATCM and BTCM size is increased.
>> +
>> +properties:
>> + $nodename:
>> + pattern: "sram-[0-9a-f]+$"
> Drop node name requirement.
> Why do you need sram node at all?
I will remove sram- node. However, it device-tree I was planning to put
all TCM nodes under single node for example:
tcm {
tcm-lockstep {
};
tcm-core@0 {
};
};
The top-most tcm node I assumed sram node. So I kept sram@...x
>> +
>> +patternProperties:
>> + "^tcm-[a-z]+@[0-9a-f]+$":
>> + type: object
>> + description: |
>> + During the split mode, each RPU core has its own set of ATCM and BTCM memory
>> +
>> + During the lock-step operation, the TCMs that are associated with the
>> + redundant processor become available to the lock-step processor.
>> + For example if each individual processor has 64KB ATCM, then in lockstep mode
>> + The size of ATCM become 128KB. Same for BTCM. tcm-lockstep node represents
>> + TCM address space in lockstep mode. tcm-core@x node specfies each core's
>> + TCM address space in split mode.
>> +
>> + properties:
>> + compatible:
>> + oneOf:
> This is not oneOf.
>
>> + - items:
> and you do not have more than one item.
>
>> + - enum:
>> + - xlnx,tcm-lockstep
>> + - xlnx,tcm-split
> compatible describes hardware, not configuration. What you encode here
> does not fit compatible.
I see. So, only xlnx,tcm is enough.
>
>> +
>> + "#address-cells":
> Use consistent quotes, either " or '
Ack.
>
>> + const: 1
>> +
>> + "#size-cells":
>> + const: 1
>> +
>> + reg:
>> + items:
>> + - description: |
>> + ATCM Memory address space. An ATCM typically holds interrupt or
>> + exception code that must be accessed at high speed, without any
>> + potential delay resulting from a cache miss.
>> + RPU on AMD-Xilinx platform can also fetch data from ATCM
>> + - description: |
>> + BTCM Memory address space. A BTCM typically holds a block of data
>> + for intensive processing, such as audio or video processing. RPU on
>> + AMD-Xilinx Platforms can also fetch Code (Instructions) from BTCM
>> +
>> + reg-names:
>> + items:
>> + - const: atcm
>> + - const: btcm
>> +
>> + ranges: true
>> +
>> + power-domains:
>> + maxItems: 8
>> + items:
>> + - description: list of ATCM Power domains
>> + - description: list of BTCM Power domains
>> + additionalItems: true
> And what are the rest?
As both items are list, we should be able to include more than one
power-domain I believe.
So first item I am trying to create list of ATCM power domains.
In split mode, first item is ATCM power-domain and second item is BTCM
power domain.
However, In lockstep mode, second core's TCM physically relocates and
two ATCM combines and
makes single region of ATCM. However, their power-domains remains same.
So, In lockstep mode, first two banks are ATCM and so, first two items
are ATCM power-domains.
I am not sure best way to represent this. But, first itmes is list.
So, I am assuming list of all ATCM power-domains possible.
>
>> +
>> + required:
>> + - compatible
>> + - '#address-cells'
>> + - '#size-cells'
>> + - reg
>> + - ranges
>> + - power-domains
>> + unevaluatedProperties: false
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/power/xlnx-zynqmp-power.h>
>> +
>> + amba {
> Drop.
ACK
>> + sram@...00000 {
> This does not match your bindings.
Ok. This was node-name. I will remove it from example.
>
>> + tcm-lockstep@...00000 {
>> + compatible = "xlnx,tcm-lockstep";
>> +
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + reg = <0xffe00000 0x20000>, <0xffe20000 0x20000>;
>> + reg-names = "atcm", "btcm";
>> + ranges = <0x0 0xffe00000 0x20000>, <0x20000 0xffe20000 0x20000>;
>> + power-domains = <&zynqmp_firmware PD_R5_0_ATCM>,
>> + <&zynqmp_firmware PD_R5_1_ATCM>,
> This is BTCM domain according to your binding. Your binding here is
> probably wrong and does not match real DTS.
As explained above, the first Item is list of all ATCM power-domains.
So, I kept both ATCM power-domains for lockstep mode.
We don't have dts nodes for TCM yet. We are using hard-coded address in
xlnx_r5_remoteproc.c driver.
As the bindings are new, I was hoping to introduce dts nodes once
bindings are designed right.
>
>> + <&zynqmp_firmware PD_R5_0_BTCM>,
>> + <&zynqmp_firmware PD_R5_1_BTCM>;
>> + };
>> +
>> + tcm-core@0 {
>> + compatible = "xlnx,tcm-split";
>> +
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + reg = <0xffe00000 0x10000>, <0xffe20000 0x10000>;
>> + reg-names = "atcm", "btcm";
>> + ranges = <0x0 0xffe00000 0x10000>, <0x20000 0xffe20000 0x10000>;
>> + power-domains = <&zynqmp_firmware PD_R5_0_ATCM>,
>> + <&zynqmp_firmware PD_R5_0_BTCM>;
>> + };
>> +
>> + tcm-core@1 {
>> + compatible = "xlnx,tcm-split";
>> +
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + reg = <0xffe90000 0x10000>, <0xffeb0000 0x10000>;
>> + reg-names = "atcm", "btcm";
>> + ranges = <0x0 0xffe90000 0x10000>, <0x20000 0xffeb0000 0x10000>;
>> + power-domains = <&zynqmp_firmware PD_R5_1_ATCM>,
>> + <&zynqmp_firmware PD_R5_1_BTCM>;
>> + };
>> + };
>> + };
>> +...
>>
>> base-commit: 6b31ffe9c8b9947d6d3552d6e10752fd96d0f80f
> Best regards,
> Krzysztof
>
Powered by blists - more mailing lists