linux-kernel - Re: [PATCH 2/3] reset: mchp: sparx5: add switch reset driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <da90b66306269564b5e7dc0c4c5fde9c087bb1c4.camel@microchip.com>
Date:   Thu, 14 Jan 2021 09:30:50 +0100
From:   Steen Hegelund <steen.hegelund@...rochip.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Philipp Zabel <p.zabel@...gutronix.de>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        <linux-kernel@...r.kernel.org>,
        "Microchip Linux Driver Support" <UNGLinuxDriver@...rochip.com>,
        Gregory Clement <gregory.clement@...tlin.com>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/3] reset: mchp: sparx5: add switch reset driver

On Thu, 2021-01-14 at 00:23 +0100, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> > +static int sparx5_switch_reset(struct reset_controller_dev *rcdev,
> > +                                   unsigned long id)
> > +{
> > +     struct mchp_reset_context *ctx =
> > +             container_of(rcdev, struct mchp_reset_context,
> > reset_ctrl);
> > +     u32 val;
> > +
> > +     /* Make sure the core is PROTECTED from reset */
> > +     regmap_update_bits(ctx->cpu_ctrl, PROTECT_REG, PROTECT_BIT,
> > PROTECT_BIT);
> > +
> > +     dev_info(ctx->dev, "soft reset of switchcore\n");
> 
> dev_dbg()?

I will remove that.
> 
> > +
> > +     /* Start soft reset */
> > +     regmap_write(ctx->gcb_ctrl, SOFT_RESET_REG, SOFT_RESET_BIT);
> > +
> > +     /* Wait for soft reset done */
> > +     return read_poll_timeout(sparx5_read_soft_rst, val,
> > +                              (val & SOFT_RESET_BIT) == 0,
> > +                              1, 100, false,
> > +                              ctx);
> > +}
> 
> > +static int mchp_sparx5_reset_config(struct platform_device *pdev,
> > +                                 struct mchp_reset_context *ctx)
> > +{
> > +     struct device_node *dn = pdev->dev.of_node;
> > +     struct regmap *cpu_ctrl, *gcb_ctrl;
> > +     struct device_node *syscon_np;
> > +     int err;
> > +
> > +     syscon_np = of_parse_phandle(dn, "syscons", 0);
> > +     if (!syscon_np)
> > +             return -ENODEV;
> > +     cpu_ctrl = syscon_node_to_regmap(syscon_np);
> > +     if (IS_ERR(cpu_ctrl))
> > +             goto err_cpu;
> > +     of_node_put(syscon_np);
> > +
> > +     syscon_np = of_parse_phandle(dn, "syscons", 1);
> > +     if (!syscon_np)
> > +             return -ENODEV;
> > +     gcb_ctrl = syscon_node_to_regmap(syscon_np);
> > +     if (IS_ERR(gcb_ctrl))
> > +             goto err_gcb;
> > +     of_node_put(syscon_np);
> > +
> > +     ctx->cpu_ctrl = cpu_ctrl;
> > +     ctx->gcb_ctrl = gcb_ctrl;
> > +
> > +     ctx->reset_ctrl.owner = THIS_MODULE;
> > +     ctx->reset_ctrl.nr_resets = 1;
> > +     ctx->reset_ctrl.ops = &sparx5_reset_ops;
> > +     ctx->reset_ctrl.of_node = dn;
> > +
> > +     err = devm_reset_controller_register(&pdev->dev, &ctx-
> > >reset_ctrl);
> > +     if (err)
> > +             dev_err(&pdev->dev, "could not register reset
> > controller\n");
> > +     pr_info("%s:%d\n", __func__, __LINE__);
> > +     return err;
> > +err_cpu:
> > +     of_node_put(syscon_np);
> > +     dev_err(&pdev->dev, "No cpu syscon map\n");
> > +     return PTR_ERR(cpu_ctrl);
> > +err_gcb:
> > +     of_node_put(syscon_np);
> > +     dev_err(&pdev->dev, "No gcb syscon map\n");
> > +     return PTR_ERR(gcb_ctrl);
> 
> It would be normal to put the dev_err() before the goto, set err =
> PTR_ERR() and then goto out;

OK. I will change that.

> 
> 
> > +}
> > +
> > +static int mchp_sparx5_reset_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct mchp_reset_context *ctx;
> > +
> > +     pr_info("%s:%d\n", __func__, __LINE__);
> 
> More left over debug.

Yes. That will have to go.

> 
> > +     ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +     if (!ctx)
> > +             return -ENOMEM;
> > +     ctx->dev = dev;
> > +     return mchp_sparx5_reset_config(pdev, ctx);
> > +}
> > +
> > +static const struct of_device_id mchp_sparx5_reset_of_match[] = {
> > +     {
> > +             .compatible = "microchip,sparx5-switch-reset",
> > +     },
> > +     { /*sentinel*/ }
> > +};
> 
> > +static int __init mchp_sparx5_reset_init(void)
> > +{
> > +     return platform_driver_register(&mchp_sparx5_reset_driver);
> > +}
> > +
> > +postcore_initcall(mchp_sparx5_reset_init);
> 
> Does it actually need to be postcore? The users of the reset should
> look for -EPROBE_DEFER and try again later. And this then becomes
> just
> a normal driver.

I tried using that, but the SGPIO driver bailed out after 3 DEFER
attempts, so that is why I changed it to use the postcore_initcall.
Maybe it is because the SGPIO driver is a builtin_platform_driver?

> 
>   Andrew