Re: [Patch Resend v1 5/8] dt-bindings: arm: tegra: Add NVIDIA Tegra234 CBB2.0 binding

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

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

On Thu, Dec 16, 2021 at 08:36:11PM +0530, Sumit Gupta wrote:
> Hi Thierry,
> 
> > On Thu, Dec 09, 2021 at 10:52:03PM +0530, Sumit Gupta wrote:
> > > Add device-tree binding documentation to represent CBB2.0 (Control
> > > Backbone) error handling driver. The driver prints debug information
> > > about failed transaction on receiving interrupt from CBB2.0.
> > > 
> > > Signed-off-by: Sumit Gupta<sumitg@nvidia.com>
> > > ---
> > >   .../arm/tegra/nvidia,tegra234-cbb.yaml        | 80 +++++++++++++++++++
> > >   1 file changed, 80 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra234-cbb.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra234-cbb.yaml b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra234-cbb.yaml
> > > new file mode 100644
> > > index 000000000000..ad8177255e6c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra234-cbb.yaml
> > > @@ -0,0 +1,80 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +
> > > +$id:"http://devicetree.org/schemas/arm/tegra/tegra23_cbb.yaml#"
> > > +$schema:"http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: NVIDIA Tegra CBB2.0 Error handling driver device tree bindings
> > > +
> > > +maintainers:
> > > +  - Sumit Gupta<sumitg@nvidia.com>
> > > +
> > > +description: |+
> > > +  Control Backbone (CBB) comprises of the physical path from an
> > > +  initiator to a target's register configuration space.
> > > +  CBB2.0 consists of multiple sub-blocks connected to each other
> > > +  to create a topology.
> > > +  Tegra234 SOC has different fabrics based on CBB2.0 architecture
> > > +  which include cluster fabrics BPMP, AON, PSC, SCE, RCE, DCE, FSI
> > > +  and "CBB central fabric".
> > > +
> > > +  In CBB2.0, each initiator which can issue transactions connects to
> > > +  a Root Master Node (MN) before it connects to any other element of
> > > +  the fabric. Each Root MN contains a Error Monitor (EM) which detects
> > > +  and logs error. Interrupts from various EM blocks are collated by
> > > +  Error Notifier (EN) which is per fabric and presents a single
> > > +  interrupt from fabric to the SOC interrupt controller.
> > > +
> > > +  The driver handles errors from CBB due to illegal register accesses
> > > +  and prints debug information about failed transaction on receiving
> > > +  the interrupt from EN. Debug information includes Error Code, Error
> > > +  Description, MasterID, Fabric, SlaveID, Address, Cache, Protection,
> > > +  Security Group etc on receiving error notification.
> > > +
> > > +  If the Error Response Disable (ERD) is set/enabled for an initiator,
> > > +  then SError or Data abort exception error response is masked and an
> > > +  interrupt is used for reporting errors due to illegal accesses from
> > > +  that initiator. The value returned on read failures is '0xFFFFFFFF'
> > > +  for compatibility with PCIE.
> > > +
> > > +properties:
> > > +  $nodename:
> > > +    pattern: "^[a-f]+-en@[0-9a-f]+$"
> > > +
> > > +  compatible:
> > > +    enum:
> > > +      - nvidia,tegra234-aon-fabric
> > > +      - nvidia,tegra234-bpmp-fabric
> > > +      - nvidia,tegra234-cbb-fabric
> > > +      - nvidia,tegra234-dce-fabric
> > > +      - nvidia,tegra234-rce-fabric
> > > +      - nvidia,tegra234-sce-fabric
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +    items:
> > > +      - description: secure interrupt from error notifier.
> > > +
> > > +  nvidia,err-notifier-base:
> > > +    description: address of error notifier inside a fabric.
> > > +
> > > +  nvidia,off-mask-erd:
> > > +    description: offset of register having ERD bit.
> > I was wondering about these two properties. Do we really need them? I
> > see that they are set on a per-SoC basic and they only differ between
> > the various fabrics. If they don't need to be configured on a per-board
> > basis, then I don't think we need to specify these explicitly. Instead I
> > think we could derive them from the compatible string
> The CBB 2.0 based fabric's error handling driver remains same across
> different SOC's and their variants. Only these fields change.
> e.g: "off-mask-erd" value is different for T23x SOC variants.
> "err-notifier-base" also changed multiple times during simulator stage.
> So, keeping them in DT to avoid changing the driver code for different
> variants of an SOC and to change them during bring up stages with DT change
> only.

For different SoC variants I would expect this to be implied by a new
compatible string. A hypothetical Tegra235 SoC that is largely the same
as Tegra234 but required slight changes in these values would also get a
different set of compatible strings. So the fabrics in that case would
be called:

	- nvidia,tegra235-aon-fabric
	- nvidia,tegra235-bpmp-fabric
	- nvidia,tegra235-cbb-fabric
	...

and then that new value can be derived from that new compatible string.
In general we only want to provide data in device tree if it can't be
implied from the compatible string. Most of the time that's only for
things that are somehow dependent on the board design. Data that is
fixed for a given SoC can be derived from the compatible string.

Thierry

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