Re: [PATCH v6 1/2] dt-bindings: edac: arm-dmc520.txt

Re: [PATCH v6 1/2] dt-bindings: edac: arm-dmc520.txt

[Borislav Petkov]

On Thu, Sep 19, 2019 at 06:37:00PM +0000, Lei Wang wrote:
> This is the device tree bindings for new EDAC driver dmc520_edac.c.
> 
> Signed-off-by: Lei Wang <leiwang_git@outlook.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> 
> ---
>     No change in v6.
> ---
>  .../devicetree/bindings/edac/arm-dmc520.txt   | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/edac/arm-dmc520.txt
> 
> diff --git a/Documentation/devicetree/bindings/edac/arm-dmc520.txt b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> new file mode 100644
> index 000000000000..71e7aa32971a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> @@ -0,0 +1,26 @@
> +* ARM DMC-520 EDAC node
> +
> +Required properties:
> +- compatible		: "brcm,dmc-520", "arm,dmc-520".
> +- reg			: Address range of the DMC-520 registers.
> +- interrupts		: DMC-520 interrupt numbers. The example below specifies
> +			  two interrupt lines for dram_ecc_errc_int and
> +			  dram_ecc_errd_int.
> +- interrupt-config	: This is an array of interrupt masks. For each of the
> +			  above interrupt line, add one interrupt mask element to
> +			  it. That is, there is a 1:1 mapping from each interrupt
> +			  line to an interrupt mask. An interrupt mask can represent
> +			  multiple interrupts being enabled. Refer to interrupt_control
> +			  register in DMC-520 TRM for interrupt mapping. In the example
> +			  below, the interrupt configuration enables dram_ecc_errc_int
> +			  and dram_ecc_errd_int. And each interrupt is connected to
> +			  a separate interrupt line.
> +
> +Example:
> +
> +dmc0: dmc@200000 {
> +	compatible = "brcm,dmc-520", "arm,dmc-520";
> +	reg = <0x200000 0x80000>;
> +	interrupts = <0x0 0x349 0x4>, <0x0 0x34B 0x4>;
> +	interrupt-config = <0x4>, <0x8>;
> +};
> -- 

Still needs a DT person ACK.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v6 1/2] dt-bindings: edac: arm-dmc520.txt

[James Morse]

Hi Lei,

On 18/10/2019 21:08, Lei Wang wrote:
> This thread hasn't got traction from DT owners.

It looks like your patches didn't make it to the mailing list:
https://lore.kernel.org/r/BY5PR04MB6599EAA659A53B2331CB812586890@BY5PR04MB6599.namprd04.prod.outlook.com

You can search on https://lore.kernel.org/linux-edac/, I can only see the replies from the
people who received it directly.

I can't see anything obvious in the headers that would cause it to get rejected. Did you
get any bounces/errors from the list?

Depending on how the DT folk work, this may be why you haven't had a response yet. Tools
like patchwork will depend on the message reaching the list.


> James did give ACK before.
> Should that be good enough?

Nope! You need someone from the maintainer's entry for those files to review it.
I gave my R-B as what you'd done made sense from the TRM and the driver. The DT folk will
have a wider view and may have some advice.


Thanks,

James

Re: [PATCH v6 1/2] dt-bindings: edac: arm-dmc520.txt

[Borislav Petkov]

On Mon, Oct 21, 2019 at 05:36:09PM +0000, Lei Wang wrote:
> Is there some guideline on what email addresses are liked and not
> liked? Thanks!

We have a document which talks about mail clients and not so much about
mail servers:

https://www.kernel.org/doc/html/latest/process/email-clients.html

but in general, use a linux system to create and send your patches and
avoid Exchange for patches because it is notorious for mangling them.

Most kernel people I know use emacs or mutt with an imap account through
which they shuffle mail with fetchmail etc.

HTH.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v6 1/2] dt-bindings: edac: arm-dmc520.txt

[James Morse]

Hi Lei,

On 21/10/2019 18:36, Lei Wang wrote:
>> It looks like your patches didn't make it to the mailing list:
>> https://lore.kernel.org/r/BY5PR04MB6599EAA659A53B2331CB812586890@BY5PR04MB6599.namprd04.prod.outlook.com
>>
>> You can search on https://lore.kernel.org/linux-edac/, I can only see the replies from the
>> people who received it directly.
>>
>> I can't see anything obvious in the headers that would cause it to get rejected. Did you
>> get any bounces/errors from the list?
>>
>> Depending on how the DT folk work, this may be why you haven't had a response yet. Tools
>> like patchwork will depend on the message reaching the list.
> 
> Yes I got reject from
> 
> linux-edac@vger.kernel.org

> devicetree@vger.kernel.org

This is very likely the reason the DT folk haven't seen this.


> with the reason of
> 
> vger.kernel.org gave this error:
> Hello [40.92.9.51], for your MAIL FROM address <leiwang_git@outlook.com> 
> policy analysis reported: Your address is not liked source for email
> 
> I'll resend from another email address that hopefully the email list 
> likes. Is there some guideline on what email addresses are liked and not 
> liked?

There doesn't seem to be a list, I guess subscribe and see if it works.
(You should subscribe to a list before posting to it. It avoids issues like this)

outlook.com is mentioned here:
http://lists.kernelnewbies.org/pipermail/kernelnewbies/2018-January/018562.html

and a few more details on yahoo and AOL here:
https://kernelnewbies.kernelnewbies.narkive.com/F9mlJOst/why-the-email-of-yahoo-com-can-t-subscribe-majordomo-vger-kernel-org

Petition your employer for a working email setup?
(or use gmail...)


Thanks,

James

Re: [PATCH v6 1/2] dt-bindings: edac: arm-dmc520.txt

[Rob Herring]

On Wed, Oct 23, 2019 at 8:22 AM James Morse <james.morse@arm.com> wrote:
>
> Hi Lei,
>
> On 21/10/2019 18:36, Lei Wang wrote:
> >> It looks like your patches didn't make it to the mailing list:
> >> https://lore.kernel.org/r/BY5PR04MB6599EAA659A53B2331CB812586890@BY5PR04MB6599.namprd04.prod.outlook.com
> >>
> >> You can search on https://lore.kernel.org/linux-edac/, I can only see the replies from the
> >> people who received it directly.
> >>
> >> I can't see anything obvious in the headers that would cause it to get rejected. Did you
> >> get any bounces/errors from the list?
> >>
> >> Depending on how the DT folk work, this may be why you haven't had a response yet. Tools
> >> like patchwork will depend on the message reaching the list.
> >
> > Yes I got reject from
> >
> > linux-edac@vger.kernel.org
>
> > devicetree@vger.kernel.org
>
> This is very likely the reason the DT folk haven't seen this.

Yes, if it doesn't go to the DT list and into patchwork, then it goes
with the other 95K unread emails and maybe I see it.

Rob

Re: [PATCH v6 1/2] dt-bindings: edac: arm-dmc520.txt

[Rob Herring]

On Thu, Sep 19, 2019 at 1:37 PM Lei Wang <leiwang_git@outlook.com> wrote:
>
> This is the device tree bindings for new EDAC driver dmc520_edac.c.
>
> Signed-off-by: Lei Wang <leiwang_git@outlook.com>
> Reviewed-by: James Morse <james.morse@arm.com>
>
> ---
>     No change in v6.
> ---
>  .../devicetree/bindings/edac/arm-dmc520.txt   | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/edac/arm-dmc520.txt
>
> diff --git a/Documentation/devicetree/bindings/edac/arm-dmc520.txt b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> new file mode 100644
> index 000000000000..71e7aa32971a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> @@ -0,0 +1,26 @@
> +* ARM DMC-520 EDAC node
> +
> +Required properties:
> +- compatible           : "brcm,dmc-520", "arm,dmc-520".
> +- reg                  : Address range of the DMC-520 registers.
> +- interrupts           : DMC-520 interrupt numbers. The example below specifies
> +                         two interrupt lines for dram_ecc_errc_int and
> +                         dram_ecc_errd_int.
> +- interrupt-config     : This is an array of interrupt masks. For each of the

Not a standard property, so would need a vendor prefix...

> +                         above interrupt line, add one interrupt mask element to
> +                         it. That is, there is a 1:1 mapping from each interrupt
> +                         line to an interrupt mask. An interrupt mask can represent
> +                         multiple interrupts being enabled. Refer to interrupt_control
> +                         register in DMC-520 TRM for interrupt mapping. In the example
> +                         below, the interrupt configuration enables dram_ecc_errc_int
> +                         and dram_ecc_errd_int. And each interrupt is connected to
> +                         a separate interrupt line.

I've gone and read thru the TRM some. This binding doesn't seem to
correspond to the TRM at all. There are a bunch of interrupts and a
combined interrupt, and then there's the same set for 'overflow'
interrupts.

There's only one 'interrupt_control' reg. How do you have more that 1
32-bit value?

> +
> +Example:
> +
> +dmc0: dmc@200000 {
> +       compatible = "brcm,dmc-520", "arm,dmc-520";
> +       reg = <0x200000 0x80000>;
> +       interrupts = <0x0 0x349 0x4>, <0x0 0x34B 0x4>;
> +       interrupt-config = <0x4>, <0x8>;
> +};
> --
> 2.17.1
>

Re: [PATCH v6 1/2] dt-bindings: edac: arm-dmc520.txt

[Lei Wang]

Thanks James/Rob/Borislav for pointing out the email list issue. My work 
email does not work good either for this exercise. Going forward I'll 
switch to my gmail account.

And Thanks Rob for reviewing! Please see below.

>> +++ b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
>> @@ -0,0 +1,26 @@
>> +* ARM DMC-520 EDAC node
>> +
>> +Required properties:
>> +- compatible           : "brcm,dmc-520", "arm,dmc-520".
>> +- reg                  : Address range of the DMC-520 registers.
>> +- interrupts           : DMC-520 interrupt numbers. The example below specifies
>> +                         two interrupt lines for dram_ecc_errc_int and
>> +                         dram_ecc_errd_int.
>> +- interrupt-config     : This is an array of interrupt masks. For each of the
> 
> Not a standard property, so would need a vendor prefix...

Would dmc-interrupt-config as the property name work? Thanks!

> 
>> +                         above interrupt line, add one interrupt mask element to
>> +                         it. That is, there is a 1:1 mapping from each interrupt
>> +                         line to an interrupt mask. An interrupt mask can represent
>> +                         multiple interrupts being enabled. Refer to interrupt_control
>> +                         register in DMC-520 TRM for interrupt mapping. In the example
>> +                         below, the interrupt configuration enables dram_ecc_errc_int
>> +                         and dram_ecc_errd_int. And each interrupt is connected to
>> +                         a separate interrupt line.
> 
> I've gone and read thru the TRM some. This binding doesn't seem to
> correspond to the TRM at all. There are a bunch of interrupts and a
> combined interrupt, and then there's the same set for 'overflow'
> interrupts.
> 
> There's only one 'interrupt_control' reg. How do you have more that 1
> 32-bit value?

There is only one 'interrupt_control' register, for multiple interrupt 
sources. Then depending on platform hardware design, these interrupt 
sources can be wired to different physical interrupt lines.

That is, it is possible to mux interrupt sources into  interrupt lines 
for dmc520 in different ways. For example, in this particular brcm 
implementation,

Line 841: source dram_ecc_errc_int
Line 843: source dram_ecc_errd_int
Line 839: source dram_ecc_errc_int and dram_ecc_errd_int

There are two possibilities for implementing ecc counts for ce/ue. And 
we chose to use the single source line: as below, two interrupt lines 
0x349 and 0x34B, with interrupt masks 0x4 and 0x8 respectively.

Also, it's possible to implement using the combined-source line too: 
that would be one interrupt line 0x347, with interrupt mask 0xC.

This dt binding can support both by modifying the properties, without 
having to modify driver code.

 >> +
 >> +Example:
 >> +
 >> +dmc0: dmc@200000 {
 >> +       compatible = "brcm,dmc-520", "arm,dmc-520";
 >> +       reg = <0x200000 0x80000>;
 >> +       interrupts = <0x0 0x349 0x4>, <0x0 0x34B 0x4>;
 >> +       interrupt-config = <0x4>, <0x8>;
 >> +};
 >> --
 >> 2.17.1

Thanks!

-Lei

Re: [PATCH v6 1/2] dt-bindings: edac: arm-dmc520.txt

[Lei Wang]

Hi Rob,

Kindly ping... I addressed your comments in below, and with one 
question. Thanks!

-Lei

On 10/25/19 10:31 AM, Lei Wang wrote:
> Thanks James/Rob/Borislav for pointing out the email list issue. My work 
> email does not work good either for this exercise. Going forward I'll 
> switch to my gmail account.
> 
> And Thanks Rob for reviewing! Please see below.
> 
>>> +++ b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
>>> @@ -0,0 +1,26 @@
>>> +* ARM DMC-520 EDAC node
>>> +
>>> +Required properties:
>>> +- compatible           : "brcm,dmc-520", "arm,dmc-520".
>>> +- reg                  : Address range of the DMC-520 registers.
>>> +- interrupts           : DMC-520 interrupt numbers. The example 
>>> below specifies
>>> +                         two interrupt lines for dram_ecc_errc_int and
>>> +                         dram_ecc_errd_int.
>>> +- interrupt-config     : This is an array of interrupt masks. For 
>>> each of the
>>
>> Not a standard property, so would need a vendor prefix...
> 
> Would dmc-interrupt-config as the property name work? Thanks!
> 
>>
>>> +                         above interrupt line, add one interrupt 
>>> mask element to
>>> +                         it. That is, there is a 1:1 mapping from 
>>> each interrupt
>>> +                         line to an interrupt mask. An interrupt 
>>> mask can represent
>>> +                         multiple interrupts being enabled. Refer to 
>>> interrupt_control
>>> +                         register in DMC-520 TRM for interrupt 
>>> mapping. In the example
>>> +                         below, the interrupt configuration enables 
>>> dram_ecc_errc_int
>>> +                         and dram_ecc_errd_int. And each interrupt 
>>> is connected to
>>> +                         a separate interrupt line.
>>
>> I've gone and read thru the TRM some. This binding doesn't seem to
>> correspond to the TRM at all. There are a bunch of interrupts and a
>> combined interrupt, and then there's the same set for 'overflow'
>> interrupts.
>>
>> There's only one 'interrupt_control' reg. How do you have more that 1
>> 32-bit value?
> 
> There is only one 'interrupt_control' register, for multiple interrupt 
> sources. Then depending on platform hardware design, these interrupt 
> sources can be wired to different physical interrupt lines.
> 
> That is, it is possible to mux interrupt sources into  interrupt lines 
> for dmc520 in different ways. For example, in this particular brcm 
> implementation,
> 
> Line 841: source dram_ecc_errc_int
> Line 843: source dram_ecc_errd_int
> Line 839: source dram_ecc_errc_int and dram_ecc_errd_int
> 
> There are two possibilities for implementing ecc counts for ce/ue. And 
> we chose to use the single source line: as below, two interrupt lines 
> 0x349 and 0x34B, with interrupt masks 0x4 and 0x8 respectively.
> 
> Also, it's possible to implement using the combined-source line too: 
> that would be one interrupt line 0x347, with interrupt mask 0xC.
> 
> This dt binding can support both by modifying the properties, without 
> having to modify driver code.
> 
>  >> +
>  >> +Example:
>  >> +
>  >> +dmc0: dmc@200000 {
>  >> +       compatible = "brcm,dmc-520", "arm,dmc-520";
>  >> +       reg = <0x200000 0x80000>;
>  >> +       interrupts = <0x0 0x349 0x4>, <0x0 0x34B 0x4>;
>  >> +       interrupt-config = <0x4>, <0x8>;
>  >> +};
>  >> --
>  >> 2.17.1
> 
> Thanks!
> 
> -Lei
>