[v9,3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding

This patch adds documentation for the APM X-Gene SoC EDAC DTS binding.

Signed-off-by: Loc Ho <lho@apm.com>
---
 .../devicetree/bindings/edac/apm-xgene-edac.txt    |   74 ++++++++++++++++++++
 1 files changed, 74 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/apm-xgene-edac.txt

On Thursday 14 May 2015 11:05:45 Loc Ho wrote:
> +       edac: edac@78800000 {
> +               compatible = "apm,xgene-edac";
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               reg = <0x0 0x78800000 0x0 0x100>;

This seens a 'ranges' property to make the address resolution
work. An empty property should be fine.

> +               interrupts = <0x0 0x20 0x4>,
> +                            <0x0 0x21 0x4>,
> +                            <0x0 0x27 0x4>;
> +
> +               edacmc0: edacmc0@7e800000 {
> +                       compatible = "apm,xgene-edac-mc";
> +                       regmap-csw = <&csw>;
> +                       regmap-mcba = <&mcba>;
> +                       regmap-mcbb = <&mcbb>;
> +                       reg = <0x0 0x7e800000 0x0 0x1000>;
> +               };
> +
> +               edacpmd0: edacpmd0@7c000000 {
> +                       compatible = "apm,xgene-edac-pmd";
> +                       regmap-efuse = <&efuse>;
> +                       reg = <0x0 0x7c000000 0x0 0x200000>;
> +               };

Why do you have labels for each one? If nothing references
these nodes, just remove the labels.

Regarding the names, remove the numbers for the nodes that are
uniquely identified by the register location, e.g. replace
edacmc0@7e800000 with edacmc@7e800000.

If you have multiple edac-mc devices, would they point to the
same syscon nodes, or different ones for each? If they are the
same, just move the references to the main node.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday 14 May 2015 11:05:46 Loc Ho wrote:
> This patch adds support for the APM X-Gene SoC EDAC driver.
> 
> Signed-off-by: Loc Ho <lho@apm.com>

Looks much better than before.

>  
> +config EDAC_XGENE
> +	tristate "APM X-Gene SoC"
> +	depends on EDAC_MM_EDAC && ARM64
> +	help
> +	  Support for error detection and correction on the
> +	  APM X-Gene family of SOCs.
> +
>  endif # EDAC

Maybe 'ARM64 || COMPILE_TEST', so we get build coverage on x86 with allmodconfig?

> +	rc = of_property_read_u32_array(np, "reg", addr_res, 4);
> +	if (rc < 0) {
> +		dev_err(edac->dev, "no MCU resource address\n");
> +		goto err_group;
> +	}
> +	tmp_ctx.mcu_csr = devm_ioremap(edac->dev,
> +				       MAKEU64(addr_res[0], addr_res[1]),
> +				       MAKEU64(addr_res[2], addr_res[3]));
> +	if (IS_ERR(tmp_ctx.mcu_csr)) {
> +		dev_err(edac->dev, "unable to map MCU resource\n");
> +		rc = PTR_ERR(tmp_ctx.mcu_csr);
> +		goto err_group;
> +	}

This is incorrect, you have to use of_get_address() or of_address_to_resource()
to compute the CPU-relative address: The address that is used within the
node may not be the same, depending on the parent's ranges properties.
See also my comment about the missing ranges in the DT example.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi,

>> +       edac: edac@78800000 {
>> +               compatible = "apm,xgene-edac";
>> +               #address-cells = <2>;
>> +               #size-cells = <2>;
>> +               reg = <0x0 0x78800000 0x0 0x100>;
>
> This seens a 'ranges' property to make the address resolution
> work. An empty property should be fine.
>
>> +               interrupts = <0x0 0x20 0x4>,
>> +                            <0x0 0x21 0x4>,
>> +                            <0x0 0x27 0x4>;
>> +
>> +               edacmc0: edacmc0@7e800000 {
>> +                       compatible = "apm,xgene-edac-mc";
>> +                       regmap-csw = <&csw>;
>> +                       regmap-mcba = <&mcba>;
>> +                       regmap-mcbb = <&mcbb>;
>> +                       reg = <0x0 0x7e800000 0x0 0x1000>;
>> +               };
>> +
>> +               edacpmd0: edacpmd0@7c000000 {
>> +                       compatible = "apm,xgene-edac-pmd";
>> +                       regmap-efuse = <&efuse>;
>> +                       reg = <0x0 0x7c000000 0x0 0x200000>;
>> +               };
>
> Why do you have labels for each one? If nothing references
> these nodes, just remove the labels.
>
> Regarding the names, remove the numbers for the nodes that are
> uniquely identified by the register location, e.g. replace
> edacmc0@7e800000 with edacmc@7e800000.

I will remove all label and get rip of the number before @.

>
> If you have multiple edac-mc devices, would they point to the
> same syscon nodes, or different ones for each? If they are the
> same, just move the references to the main node.

They all reference the same one. I will move it up one level.

-Loc
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html