Re: [PATCH v5 4/8] dt-bindings: memory: tegra: Add external memory controller binding for Tegra210

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4643 bytes --]

On Tue, Mar 10, 2020 at 04:19:59PM +0100, Thierry Reding wrote:
> From: Joseph Lo <josephl@nvidia.com>
> 
> Add the binding document for the external memory controller (EMC) which
> communicates with external LPDDR4 devices. It includes the bindings of
> the EMC node and a sub-node of EMC table which under the reserved memory
> node. The EMC table contains the data of the rates that EMC supported.
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v5:
> - convert to dt-schema
> 
>  .../nvidia,tegra210-emc.yaml                  | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/nvidia,tegra210-emc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra210-emc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra210-emc.yaml
> new file mode 100644
> index 000000000000..caf21c08f9cc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra210-emc.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/nvidia,tegra210-emc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVIDIA Tegra210 SoC External Memory Controller
> +
> +maintainers:
> +  - Thierry Reding <thierry.reding@gmail.com>
> +  - Jon Hunter <jonathanh@nvidia.com>
> +
> +description: |
> +  The EMC interfaces with the off-chip SDRAM to service the request stream
> +  sent from the memory controller.
> +
> +properties:
> +  compatible:
> +    const: nvidia,tegra210-emc
> +
> +  reg:
> +    maxItems: 3
> +
> +  clocks:
> +    items:
> +      - description: external memory clock
> +
> +  clock-names:
> +    items:
> +      - const: emc
> +
> +  interrupts:
> +    items:
> +      - description: EMC general interrupt
> +
> +  memory-region:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to a reserved memory region describing the table of EMC
> +      frequencies trained by the firmware

Hi Rob,

the dt_binding_check error aside, do you have any feedback on this
particular property? This is a replacement for what we used to do on
earlier chips where each frequency had its own device tree node, and
each such node had a bunch of properties, which made it not very
readable and cumbersome to parse.

The reason I ask about this specifically is because there are two
levels of bootloaders involved here to pass the information to the
kernel and I'd like to get those patches merged into the bootloaders
while I'm finishing up the Linux kernel support.

Dmitry asked whether the format of this table would need to be
documented in the bindings. I'm on the fence about this. On one hand
we don't have this documented anywhere, but on the other hand, the table
has things like revision fields and so on, so it could technically
change, even though it's very unlikely that it will.

If you do want it formatted, do you have any suggestions on what that
should look like? Should I simply dump the C struct definition into the
bindings document?

Thierry

> +
> +  nvidia,memory-controller:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle of the memory controller node
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - nvidia,memory-controller
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/tegra210-car.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #
> +    reserved-memory {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        ranges;
> +
> +        emc_table: emc-table@83400000 {
> +            compatible = "nvidia,tegra210-emc-table";
> +            reg = <0x0 0x83400000 0x0 0x10000>;
> +            status = "okay";
> +        };
> +    };
> +
> +    external-memory-controller@7001b000 {
> +        compatible = "nvidia,tegra210-emc";
> +        reg = <0x0 0x7001b000 0x0 0x1000>,
> +              <0x0 0x7001e000 0x0 0x1000>,
> +              <0x0 0x7001f000 0x0 0x1000>;
> +        clocks = <&tegra_car TEGRA210_CLK_EMC>;
> +        clock-names = "emc";
> +        interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
> +        memory-region = <&emc_table>;
> +        nvidia,memory-controller = <&mc>;
> +    };
> -- 
> 2.24.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]