Re: [PATCH 6/8] power: reset: at91-reset: add reset_controller_dev support

[Claudiu.Beznea]

Hi, Philipp,

On 05.04.2022 14:47, Philipp Zabel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Claudiu,
> 
> On Di, 2022-04-05 at 14:27 +0300, Claudiu Beznea wrote:
>> SAMA7G5 reset controller has 5 extra lines that goes to different
>> devices
>> (3 lines to USB PHYs, 1 line to DDR controller, one line DDR PHY
>> controller). These reset lines could be requested by different
>> controller
>> drivers (e.g. USB PHY driver) and these controllers' drivers could
>> assert/deassert these lines when necessary. Thus add support for
>> reset_controller_dev which brings this functionality.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>> ---
>>  drivers/power/reset/at91-reset.c | 92
>> ++++++++++++++++++++++++++++++--
>>  1 file changed, 88 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/power/reset/at91-reset.c
>> b/drivers/power/reset/at91-reset.c
>> index 0d721e27f545..b04df54c15d2 100644
>> --- a/drivers/power/reset/at91-reset.c
>> +++ b/drivers/power/reset/at91-reset.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/of_address.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/reboot.h>
>> +#include <linux/reset-controller.h>
>>
>>  #include <soc/at91/at91sam9_ddrsdr.h>
>>  #include <soc/at91/at91sam9_sdramc.h>
>> @@ -53,12 +54,16 @@ enum reset_type {
>>  struct at91_reset {
>>         void __iomem *rstc_base;
>>         void __iomem *ramc_base[2];
>> +       void __iomem *dev_base;
>> +       struct reset_controller_dev rcdev;
>>         struct clk *sclk;
>>         struct notifier_block nb;
>>         u32 args;
>>         u32 ramc_lpr;
>>  };
>>
>> +#define to_at91_reset(r)       container_of(r, struct at91_reset, rcdev)
>> +
>>  struct at91_reset_data {
>>         u32 reset_args;
>>         u32 n_device_reset;
>> @@ -191,6 +196,79 @@ static const struct of_device_id
>> at91_reset_of_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, at91_reset_of_match);
>>
>> +static int at91_reset_update(struct reset_controller_dev *rcdev,
>> +                            unsigned long id, bool assert)
>> +{
>> +       struct at91_reset *reset = to_at91_reset(rcdev);
>> +       u32 val;
>> +
>> +       val = readl_relaxed(reset->dev_base);
>> +       if (assert)
>> +               val |= BIT(id);
>> +       else
>> +               val &= ~BIT(id);
>> +       writel_relaxed(val, reset->dev_base);
> 
> This read-modify-update should be protected by a spinlock.

Right, I missed it.

> 
>> +
>> +       return 0;
>> +}
>> +
>> +static int at91_reset_assert(struct reset_controller_dev *rcdev,
>> +                            unsigned long id)
>> +{
>> +       return at91_reset_update(rcdev, id, true);
>> +}
>> +
>> +static int at91_reset_deassert(struct reset_controller_dev *rcdev,
>> +                              unsigned long id)
>> +{
>> +       return at91_reset_update(rcdev, id, false);
>> +}
>> +
>> +static int at91_reset_dev_status(struct reset_controller_dev *rcdev,
>> +                                unsigned long id)
>> +{
>> +       struct at91_reset *reset = to_at91_reset(rcdev);
>> +       u32 val;
>> +
>> +       val = readl_relaxed(reset->dev_base);
>> +
>> +       return !!(val & BIT(id));
>> +}
>> +
>> +static const struct reset_control_ops at91_reset_ops = {
>> +       .assert = at91_reset_assert,
>> +       .deassert = at91_reset_deassert,
>> +       .status = at91_reset_dev_status,
>> +};
>> +
>> +static int at91_reset_of_xlate(struct reset_controller_dev *rcdev,
>> +                              const struct of_phandle_args *reset_spec)
>> +{
>> +       return reset_spec->args[0];
>> +}
> 
> For 1:1 mappings there is no need for a custom of_xlate handler. Just
> leave of_xlate and of_reset_n_cells empty.

I'll have a look on it.

> 
>> +
>> +static int at91_rcdev_init(struct at91_reset *reset,
>> +                          const struct at91_reset_data *data,
>> +                          struct platform_device *pdev)
>> +{
>> +       if (!data->n_device_reset)
>> +               return 0;
>> +
>> +       reset->dev_base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 1,
>> +                                       NULL);
>> +       if (IS_ERR(reset->rstc_base))
> 
> Should check reset->dev_base here.

That's true.

Thank you for your review,
Claudiu Beznea

> 
>> +               return -ENODEV;
>> +
>> +       reset->rcdev.ops = &at91_reset_ops;
>> +       reset->rcdev.owner = THIS_MODULE;
>> +       reset->rcdev.of_node = pdev->dev.of_node;
>> +       reset->rcdev.nr_resets = data->n_device_reset;
>> +       reset->rcdev.of_reset_n_cells = 1;
>> +       reset->rcdev.of_xlate = at91_reset_of_xlate;
>> +
>> +       return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>> +}
>> +
>>  static int __init at91_reset_probe(struct platform_device *pdev)
>>  {
>>         const struct of_device_id *match;
>> @@ -244,6 +322,10 @@ static int __init at91_reset_probe(struct
>> platform_device *pdev)
>>
>>         platform_set_drvdata(pdev, reset);
>>
>> +       ret = at91_rcdev_init(reset, data, pdev);
>> +       if (ret)
>> +               goto disable_clk;
>> +
>>         if (of_device_is_compatible(pdev->dev.of_node,
>> "microchip,sam9x60-rstc")) {
>>                 u32 val = readl(reset->rstc_base + AT91_RSTC_MR);
>>
>> @@ -252,14 +334,16 @@ static int __init at91_reset_probe(struct
>> platform_device *pdev)
>>         }
>>
>>         ret = register_restart_handler(&reset->nb);
>> -       if (ret) {
>> -               clk_disable_unprepare(reset->sclk);
>> -               return ret;
>> -       }
>> +       if (ret)
>> +               goto disable_clk;
>>
>>         at91_reset_status(pdev, reset->rstc_base);
>>
>>         return 0;
>> +
>> +disable_clk:
>> +       clk_disable_unprepare(reset->sclk);
>> +       return ret;
>>  }
>>
>>  static int __exit at91_reset_remove(struct platform_device *pdev)
> 
> regards
> Philipp