linux-kernel - Re: [PATCH v2 6/9] clk: clocking-wizard: add user clock monitor support

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] [day] [month] [year] [list]

Message-ID: <D3KYBAJXSWWV.3S8LKAX01TCAM@protonmail.com>
Date: Tue, 20 Aug 2024 18:30:33 +0000
From: Harry Austen <hpausten@...tonmail.com>
To: "Datta, Shubhrajyoti" <shubhrajyoti.datta@....com>, Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, "Simek, Michal" <michal.simek@....com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Dave Ertman <david.m.ertman@...el.com>, Ira Weiny <ira.weiny@...el.com>, "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 6/9] clk: clocking-wizard: add user clock monitor support

On Mon Aug 19, 2024 at 1:39 PM BST, Datta, Shubhrajyoti wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> > -----Original Message-----
...
> >
> > -static const struct versal_clk_data versal_data = {
> > -       .is_versal      = true,
> > +static const struct clk_wzrd_data version_6_0_data = {
> > +       .is_versal              = false,
> > +       .supports_monitor       = true,
> >  };
>
> The clocking wizard monitor support is a design choice.
> This will enable it for all the designs.

But the interrupt still has to be described in devicetree for the auxiliary/UIO
device to be registered. The interrupt is only used by the core for the clock
monitor functionality, so if that functionality is not built into the hardware,
then the interrupt description should be omitted. Does that not sound sensible
to you?

>
> >
> > +static const struct clk_wzrd_data versal_data = {
> > +       .is_versal              = true,
> > +       .supports_monitor       = true,
> > +};
>
>
> Same here.

Same reasoning as above.

Thanks for the review!
Harry

>
> > +
> > +static void clk_wzrd_unregister_adev(void *_adev) {
> > +       struct auxiliary_device *adev = _adev;
> > +
> > +       auxiliary_device_delete(adev);
> > +       auxiliary_device_uninit(adev);
> > +}
> > +
> > +static int clk_wzrd_setup_monitor(struct platform_device *pdev) {
> > +       struct device *dev = &pdev->dev;
> > +       const struct clk_wzrd_data *data = device_get_match_data(dev);
> > +       struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
> > +       struct auxiliary_device *adev = &clk_wzrd->adev;
> > +       int ret;
> > +
> > +       if (!data || !data->supports_monitor)
> > +               return 0;
> > +
> > +       adev->name = "clk-mon";
> > +       adev->dev.parent = dev;
> > +       adev->dev.platform_data = (__force void *)clk_wzrd->base;
> > +
> > +       ret = auxiliary_device_init(adev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = auxiliary_device_add(adev);
> > +       if (ret) {
> > +               auxiliary_device_uninit(adev);
> > +               return ret;
> > +       }
> > +
> > +       return devm_add_action_or_reset(dev, clk_wzrd_unregister_adev,
> > +adev); }
> > +
> >  static int clk_wzrd_register_output_clocks(struct device *dev, int
> > nr_outputs)  {
> >         const char *clkout_name, *clk_name, *clk_mul_name;
> >         struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
> >         u32 regl, regh, edge, regld, reghd, edged, div;
> > -       const struct versal_clk_data *data;
> > +       const struct clk_wzrd_data *data;
> >         unsigned long flags = 0;
> >         bool is_versal = false;
> >         void __iomem *ctrl_reg;
> > @@ -1170,6 +1216,10 @@ static int clk_wzrd_probe(struct platform_device
> > *pdev)
> >                 return -EINVAL;
> >         }
> >
> > +       ret = clk_wzrd_setup_monitor(pdev);
> > +       if (ret)
> > +               return dev_err_probe(&pdev->dev, ret, "failed to setup
> > + monitor\n");
> > +
> >         ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs);
> >         if (ret)
> >                 return ret;
> > @@ -1204,7 +1254,7 @@ static const struct of_device_id clk_wzrd_ids[] = {
> >         { .compatible = "xlnx,versal-clk-wizard", .data = &versal_data },
> >         { .compatible = "xlnx,clocking-wizard"   },
> >         { .compatible = "xlnx,clocking-wizard-v5.2"   },
> > -       { .compatible = "xlnx,clocking-wizard-v6.0"  },
> > +       { .compatible = "xlnx,clocking-wizard-v6.0", .data =
> > + &version_6_0_data },
> >         { },
> >  };
> >  MODULE_DEVICE_TABLE(of, clk_wzrd_ids);
> > --
> > 2.46.0
> >