Hi Borislav,
>> +
>> +static void xgene_edac_l3_hw_init(struct edac_device_ctl_info *edac_dev,
>> + bool enable)
>
> arg alignment. It should be
>
> function name(arg1, arg2,
> arg3, arg4,
> arg5...
>
> Check your other functions too.
I will check and fix other functions if any. But this function is just
fine. It may be off just by the + extra character with patch
generation only.
>> +{
>> + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
>> + struct dentry *edac_debugfs;
>> + char name[30];
>> +
>> + if (!IS_ENABLED(CONFIG_EDAC_DEBUG))
>> + return;
>> +
>> + /*
>> + * Todo: Switch to common EDAC debug file system for edac device
>> + * when available.
>> + */
>
> What is that?
Debug folder node shows up at /sys/kernel/debug/<file here> while the
MC debug node shows up at /sys/kernel/debug/edac where <file here> is
the driver node name. It would be better if everything shows up at
/sys/kernel/debug/edac. For this to happen, I need EDAC core change to
provide an parent node.
>
>> + if (!ctx->edac->dfs) {
>> + ctx->edac->dfs = debugfs_create_dir(edac_dev->dev->kobj.name,
>> + NULL);
>> + if (!ctx->edac->dfs)
>> + return;
>> + }
>> + sprintf(name, "l3c%d", ctx->edac_idx);
>> + edac_debugfs = debugfs_create_dir(name, ctx->edac->dfs);
>> + if (!edac_debugfs)
>> + return;
>> +
>> + debugfs_create_file("l3_inject_ctrl", S_IWUSR, edac_debugfs, edac_dev,
>> + &xgene_edac_l3_debug_inject_fops);
>> +}
>> +
>> +static int xgene_edac_l3_add(struct xgene_edac *edac, struct device_node *np,
>> + int version)
>> +{
>> + struct edac_device_ctl_info *edac_dev;
>> + struct xgene_edac_dev_ctx *ctx;
>> + struct resource res;
>> + void __iomem *dev_csr;
>> + int edac_idx;
>> + int rc = 0;
>> +
>> + if (!devres_open_group(edac->dev, xgene_edac_l3_add, GFP_KERNEL))
>> + return -ENOMEM;
>> +
>> + rc = of_address_to_resource(np, 0, &res);
>> + if (rc < 0) {
>> + dev_err(edac->dev, "no L3 resource address\n");
>> + goto err;
>> + }
>> + dev_csr = devm_ioremap_resource(edac->dev, &res);
>> + if (IS_ERR(dev_csr)) {
>> + dev_err(edac->dev,
>> + "devm_ioremap_resource failed for L3 resource address\n");
>> + rc = PTR_ERR(dev_csr);
>> + goto err;
>> + }
>>
>> + edac_idx = edac_device_alloc_index();
>> + edac_dev = edac_device_alloc_ctl_info(sizeof(*ctx),
>> + "l3c", 1, "l3c", 1, 0, NULL, 0,
>> + edac_idx);
>> + if (!edac_dev) {
>> + rc = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + ctx = edac_dev->pvt_info;
>> + ctx->dev_csr = dev_csr;
>> + ctx->name = "xgene_l3_err";
>> + ctx->edac_idx = edac_idx;
>> + 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;
>> + edac_dev->mod_name = EDAC_MOD_STR;
>> +
>> + if (edac_op_state == EDAC_OPSTATE_POLL)
>> + edac_dev->edac_check = xgene_edac_l3_check;
>
> So depending on what SoC elements you detect, the last one's check
> function will win?
>
> If xgene_edac loads on multiple SoC elements, you need a global xgene
> check function for which l3, soc, mc, etc register...
No... each EDAC instance has its own context as we allocated via
function edac_device_alloc_ctl_info. Per type of instance will use the
same type function.
>> +
>> +static const char * const *xgene_edac_soc_mem_data(
>> + struct xgene_edac_dev_ctx *ctx)
>> +{
>> + switch (ctx->version) {
>> + case 1:
>> + return soc_mem_err_v1;
>> + default:
>> + return NULL;
>> + }
>> +}
>
> That's one badly formatted function used only once. Just move the logic
> there and kill it.
For format, code looks fine. In any case, I will roll this into the code.
>> +static int xgene_edac_soc_add(struct xgene_edac *edac, struct device_node *np,
>> + int version)
>> +{
>> + struct edac_device_ctl_info *edac_dev;
>> + struct xgene_edac_dev_ctx *ctx;
>> + void __iomem *dev_csr;
>> + struct resource res;
>> + int edac_idx;
>> + int rc = 0;
>> +
>> + if (!devres_open_group(edac->dev, xgene_edac_soc_add, GFP_KERNEL))
>> + return -ENOMEM;
>> +
>> + rc = of_address_to_resource(np, 0, &res);
>> + if (rc < 0) {
>> + dev_err(edac->dev, "no SoC resource address\n");
>> + goto err;
>> + }
>> + dev_csr = devm_ioremap_resource(edac->dev, &res);
>> + if (IS_ERR(dev_csr)) {
>> + dev_err(edac->dev,
>> + "devm_ioremap_resource failed for soc resource address\n");
>> + rc = PTR_ERR(dev_csr);
>> + goto err;
>> + }
>> +
>> + edac_idx = edac_device_alloc_index();
>> + edac_dev = edac_device_alloc_ctl_info(sizeof(*ctx),
>> + "SOC", 1, "SOC", 1, 2, NULL, 0,
>> + edac_idx);
>> + if (!edac_dev) {
>> + rc = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + ctx = edac_dev->pvt_info;
>> + ctx->dev_csr = dev_csr;
>> + ctx->name = "xgene_soc_err";
>> + ctx->edac_idx = edac_idx;
>> + 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;
>> + edac_dev->mod_name = EDAC_MOD_STR;
>> +
>> + if (edac_op_state == EDAC_OPSTATE_POLL)
>> + edac_dev->edac_check = xgene_edac_soc_check;
>
> Same here about the ->edac_check function.
Same comment as above.
-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
