Hi Arnd,
Thanks for the changes, some minor change requested below.
> The new x-gene EDAC driver incorrectly tried to figure out the version of
> one of its IP blocks by looking at the version of the CPU core, which is
> only vagely related.
>
> This removes the incorrect code and instead uses the version of the IP
> block in the compatible string where it belongs.
>
> Found using build testing on x86, which does not provide the arm64
> cpuid interface.
>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
>
> How about this one? We should make sure this gets fixed before the binding
> gets set in stone with a kernel release.
>
> Loc, do you know if some of the other blocks might also have versions
> associated with them? Normally we try to use more specific product names,
> but in case of APM there don't seem to be any more specific names
> than X-Gene and X-Gene2 any more.
>
I already asked the designer. There is no version register in the PMD block.
> diff --git a/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
> index 480911c38ff9..e31b696ba939 100644
> --- a/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
> +++ b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
> @@ -25,7 +25,7 @@ Required properties for memory controller subnode:
> - memory-controller : Instance number of the memory controller.
>
> Required properties for PMD subnode:
> -- compatible : Shall be "apm,xgene-edac-pmd".
> +- compatible : Shall be "apm,xgene-edac-pmd" or "apm,xgene-v2-edac-pmd"
Can we change to "apm,xgene-edac-pmd-v2". I would like to associate
with the PMD instead with the family name. The PMD (processor module)
might be re-use in future chip as is such as Gen3, Gen4, and etc.
> - reg : First resource shall be the PMD resource.
> - pmd-controller : Instance number of the PMD controller.
>
> diff --git a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c
> index b5158572f6ab..8071c2223f92 100644
> --- a/drivers/edac/xgene_edac.c
> +++ b/drivers/edac/xgene_edac.c
> @@ -523,6 +523,7 @@ struct xgene_edac_pmd_ctx {
> struct edac_device_ctl_info *edac_dev;
> void __iomem *pmd_csr;
> u32 pmd;
> + int version;
> };
>
> static void xgene_edac_pmd_l1_check(struct edac_device_ctl_info *edac_dev,
> @@ -784,50 +785,6 @@ static void xgene_edac_pmd_cpu_hw_cfg(struct edac_device_ctl_info *edac_dev,
> writel(0x00000101, pg_f + MEMERR_CPU_MMUECR_PAGE_OFFSET);
> }
>
> -static bool xgene_edac_pmd_l2c_version1(void)
> -{
> - /* Check all chips with PMD L2C version 1 HW */
> - #define REVIDR_MINOR_REV(revidr) ((revidr) & 0x00000007)
> -
> - switch (MIDR_VARIANT(read_cpuid_id())) {
> - case 0:
> - switch (MIDR_REVISION(read_cpuid_id())) {
> - case 0:
> -
> - switch (REVIDR_MINOR_REV(read_cpuid(REVIDR_EL1))) {
> - case 1:
> - case 2:
> - return true;
> - };
> - break;
> - case 1:
> - if (REVIDR_MINOR_REV(read_cpuid(REVIDR_EL1)) == 1)
> - return true;
> - break;
> - }
> - break;
> - case 1:
> - switch (MIDR_REVISION(read_cpuid_id())) {
> - case 0:
> - switch (REVIDR_MINOR_REV(read_cpuid(REVIDR_EL1))) {
> - case 1:
> - return true;
> - };
> - break;
> - case 1:
> - switch (REVIDR_MINOR_REV(read_cpuid(REVIDR_EL1))) {
> - case 1:
> - case 0:
> - return true;
> - };
> - break;
> - }
> - break;
> - }
> -
> - return false;
> -}
> -
> static void xgene_edac_pmd_hw_cfg(struct edac_device_ctl_info *edac_dev)
> {
> struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info;
> @@ -837,7 +794,7 @@ static void xgene_edac_pmd_hw_cfg(struct edac_device_ctl_info *edac_dev)
> /* Enable PMD memory error - MEMERR_L2C_L2ECR and L2C_L2RTOCR */
> writel(0x00000703, pg_e + MEMERR_L2C_L2ECR_PAGE_OFFSET);
> /* Configure L2C HW request time out feature if supported */
> - if (!xgene_edac_pmd_l2c_version1())
> + if (ctx->version == 1)
The correct changes would be ctx->version > 1. So:
if (ctx->version > 1)
> writel(0x00000119, pg_d + CPUX_L2C_L2RTOCR_PAGE_OFFSET);
> }
>
> @@ -956,7 +913,7 @@ static int xgene_edac_pmd_available(u32 efuse, int pmd)
> return (efuse & (1 << pmd)) ? 0 : 1;
> }
>
> -static int xgene_edac_pmd_add(struct xgene_edac *edac, struct device_node *np)
> +static int xgene_edac_pmd_add(struct xgene_edac *edac, struct device_node *np, int version)
> {
> struct edac_device_ctl_info *edac_dev;
> struct xgene_edac_pmd_ctx *ctx;
> @@ -998,6 +955,7 @@ static int xgene_edac_pmd_add(struct xgene_edac *edac, struct device_node *np)
> ctx->edac = edac;
> ctx->edac_dev = edac_dev;
> ctx->ddev = *edac->dev;
> + ctx->version = version;
> edac_dev->dev = &ctx->ddev;
> edac_dev->ctl_name = ctx->name;
> edac_dev->dev_name = ctx->name;
> @@ -1169,7 +1127,9 @@ static int xgene_edac_probe(struct platform_device *pdev)
> if (of_device_is_compatible(child, "apm,xgene-edac-mc"))
> xgene_edac_mc_add(edac, child);
> if (of_device_is_compatible(child, "apm,xgene-edac-pmd"))
> - xgene_edac_pmd_add(edac, child);
> + xgene_edac_pmd_add(edac, child, 1);
> + if (of_device_is_compatible(child, "apm,xgene-v2-edac-pmd"))
Change to "apm,xgene-edac-pmd-v2"
> + xgene_edac_pmd_add(edac, child, 2);
> }
>
> return 0;
Again... Thanks for the changes.
-Loc
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
