LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [PATCH v2 2/2] dt-bindings: edac: arm-dmc520.txt
       [not found] ` <BN7PR08MB5572B3388B2D7DC8F6C7F285AE4C0@BN7PR08MB5572.namprd08.prod.outlook.com>
@ 2019-03-25 18:30   ` James Morse
  2019-05-16 19:36     ` Lei Wang (BSP)
  0 siblings, 1 reply; 2+ messages in thread
From: James Morse @ 2019-03-25 18:30 UTC (permalink / raw)
  To: Rui Zhao
  Cc: bp, robh+dt, mark.rutland, devicetree, linux-kernel, linux-edac,
	okaya, mchehab, will.deacon, sashal, hangl, lewan, Rui Zhao

Hi Rui,

On 07/03/2019 01:24, Rui Zhao wrote:
> From: Rui Zhao <ruizhao@microsoft.com>
> dt-bindings for new EDAC driver dmc520_edac.c.

(minor nit, the DT folk prefer the binding to come first in the series, this makes it
easier to review)


> diff --git a/Documentation/devicetree/bindings/edac/arm-dmc520.txt b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> new file mode 100644
> index 0000000..7bea7dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> @@ -0,0 +1,21 @@
> +* ARM DMC-520 EDAC node
> +
> +Required properties:
> +- compatible		: "arm,dmc-520".
> +- reg			: Address range of the DMC-520 registers.
> +- interrupts		: DMC-520 interrupt numbers.

Your example has two interrupts, what do they correspond to? (It needs to be clear from
the binding)

Because this thing has quite a few, it may be worth naming the ones you use. If someone
else's platform uses one of the others, they can add it without conflicting with DTs for
yours.

Looking through the TRM for things ending in _int, they seem to be:
* ram_ecc_erc
* ram_ecc_erd
* dram_ecc_erc
* dram_ecc_erd
* failed_access
* failed_prog
* link_err
* arch_fsm
* temperature_event
* phy_request
* combined_int

I think this is far too many to enumerate from day one, especially as your platform only
needs two. Could we name the two you need so that its clear which ones they are, and
others can be added when someone needs them.


> +- interrupt-mask	: Interrupts to be enabled, refer to interrupt_control
> +			  register in DMC-520 TRM for interrupt mapping.

This sounds like policy. It would be good to omit the interrupts that aren't wired up. If
there is a policy like 'use ram not dram on platform Y' we can get the edac driver to do
that based on of_machine_is_compatible() (as the altera edac driver already does).


> +Optional properties:
> +- interrupt-shared	: set this property if and only if all DMC-520
> +			  interrupts share the interrupt number.

What if some of them are combined, and some aren't?

(this shared-interrupts was my example of why we need a documented binding to work out
what is specific to your platofrm)

I'm not sure how this usually gets described in a DT binding ... couldn't we spot this
from duplicate entries in the interrupts property? If we register them with IRQF_SHARED,
would it matter?

(We can always tell its our device from the status register, so I think we should use
IRQF_SHARED regardless.)


Thanks,

James

^ permalink raw reply	[flat|nested] 2+ messages in thread

* RE: [PATCH v2 2/2] dt-bindings: edac: arm-dmc520.txt
  2019-03-25 18:30   ` [PATCH v2 2/2] dt-bindings: edac: arm-dmc520.txt James Morse
@ 2019-05-16 19:36     ` Lei Wang (BSP)
  0 siblings, 0 replies; 2+ messages in thread
From: Lei Wang (BSP) @ 2019-05-16 19:36 UTC (permalink / raw)
  To: James Morse, Rui Zhao
  Cc: bp, robh+dt, mark.rutland, devicetree, linux-kernel, linux-edac,
	okaya, mchehab, will.deacon, sashal, Hang Li, Rui Zhao

Hi James/Borislav,

I addressed your comments and sent out "[PATCH v3 1/2] dt-bindings: edac: arm-dmc520.txt" for review. Thanks!

-Lei

-----Original Message-----
From: James Morse <james.morse@arm.com> 
Sent: Monday, March 25, 2019 11:30 AM
To: Rui Zhao <ruizhao@outlook.com>
Cc: bp@alien8.de; robh+dt@kernel.org; mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org; okaya@kernel.org; mchehab@kernel.org; will.deacon@arm.com; sashal@kernel.org; Hang Li <hangl@microsoft.com>; Lei Wang (BSP) <Wang.Lei@microsoft.com>; Rui Zhao <ruizhao@microsoft.com>
Subject: Re: [PATCH v2 2/2] dt-bindings: edac: arm-dmc520.txt

Hi Rui,

On 07/03/2019 01:24, Rui Zhao wrote:
> From: Rui Zhao <ruizhao@microsoft.com> dt-bindings for new EDAC driver 
> dmc520_edac.c.

(minor nit, the DT folk prefer the binding to come first in the series, this makes it easier to review)


> diff --git a/Documentation/devicetree/bindings/edac/arm-dmc520.txt 
> b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> new file mode 100644
> index 0000000..7bea7dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> @@ -0,0 +1,21 @@
> +* ARM DMC-520 EDAC node
> +
> +Required properties:
> +- compatible		: "arm,dmc-520".
> +- reg			: Address range of the DMC-520 registers.
> +- interrupts		: DMC-520 interrupt numbers.

Your example has two interrupts, what do they correspond to? (It needs to be clear from the binding)

Because this thing has quite a few, it may be worth naming the ones you use. If someone else's platform uses one of the others, they can add it without conflicting with DTs for yours.

Looking through the TRM for things ending in _int, they seem to be:
* ram_ecc_erc
* ram_ecc_erd
* dram_ecc_erc
* dram_ecc_erd
* failed_access
* failed_prog
* link_err
* arch_fsm
* temperature_event
* phy_request
* combined_int

I think this is far too many to enumerate from day one, especially as your platform only needs two. Could we name the two you need so that its clear which ones they are, and others can be added when someone needs them.


> +- interrupt-mask	: Interrupts to be enabled, refer to interrupt_control
> +			  register in DMC-520 TRM for interrupt mapping.

This sounds like policy. It would be good to omit the interrupts that aren't wired up. If there is a policy like 'use ram not dram on platform Y' we can get the edac driver to do that based on of_machine_is_compatible() (as the altera edac driver already does).


> +Optional properties:
> +- interrupt-shared	: set this property if and only if all DMC-520
> +			  interrupts share the interrupt number.

What if some of them are combined, and some aren't?

(this shared-interrupts was my example of why we need a documented binding to work out what is specific to your platofrm)

I'm not sure how this usually gets described in a DT binding ... couldn't we spot this from duplicate entries in the interrupts property? If we register them with IRQF_SHARED, would it matter?

(We can always tell its our device from the status register, so I think we should use IRQF_SHARED regardless.)


Thanks,

James

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2019-05-16 19:36 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1551921818-2825-1-git-send-email-ruizhao@outlook.com>
     [not found] ` <BN7PR08MB5572B3388B2D7DC8F6C7F285AE4C0@BN7PR08MB5572.namprd08.prod.outlook.com>
2019-03-25 18:30   ` [PATCH v2 2/2] dt-bindings: edac: arm-dmc520.txt James Morse
2019-05-16 19:36     ` Lei Wang (BSP)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).