| Message ID | 20220405112724.2760905-1-claudiu.beznea@microchip.com |
|---|---|
| Headers | show |
| Series | power: reset: at91-reset: add support for sama7g5 | expand |
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@microchip.com> >> --- >> 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
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@microchip.com> >> --- >> 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. > >> + >> + 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've double checked that. This would work if reset ids are continuous from zero to rcdev.nr_resets. This the of_reset_simple_xlate: static int of_reset_simple_xlate(struct reset_controller_dev *rcdev, const struct of_phandle_args *reset_spec) { if (reset_spec->args[0] >= rcdev->nr_resets) return -EINVAL; return reset_spec->args[0]; } But in this driver's case we have 3 ids: 4, 5, 6. That is the reason I had this simple xlate function. Thank you, Claudiu Beznea > >> + >> +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. > >> + 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
Hi Claudio, On Di, 2022-04-05 at 14:47 +0000, Claudiu.Beznea@microchip.com wrote: > 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@microchip.com> > > > --- > > > 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. > > > > > + > > > + 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've double checked that. This would work if reset ids are continuous > from > zero to rcdev.nr_resets. This the of_reset_simple_xlate: > > static int of_reset_simple_xlate(struct reset_controller_dev *rcdev, > const struct of_phandle_args > *reset_spec) > { > if (reset_spec->args[0] >= rcdev->nr_resets) > return -EINVAL; > return reset_spec->args[0]; > } > > But in this driver's case we have 3 ids: 4, 5, 6. That is the reason > I had this simple xlate function. I see. In that case I'd say keep the custom of_xlate but let it return -EINVAL if the args[0] value is not 4, 5, or 6. Or you could set nr_resets to 7, but unless there are more resets at the lower bits, that wouldn't necessarily be better. regards Philipp
On 05.04.2022 18:15, Philipp Zabel wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Claudio, > > On Di, 2022-04-05 at 14:47 +0000, Claudiu.Beznea@microchip.com wrote: >> 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@microchip.com> >>>> --- >>>> 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. >>> >>>> + >>>> + 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've double checked that. This would work if reset ids are continuous >> from >> zero to rcdev.nr_resets. This the of_reset_simple_xlate: >> >> static int of_reset_simple_xlate(struct reset_controller_dev *rcdev, >> const struct of_phandle_args >> *reset_spec) >> { >> if (reset_spec->args[0] >= rcdev->nr_resets) >> return -EINVAL; >> return reset_spec->args[0]; >> } >> >> But in this driver's case we have 3 ids: 4, 5, 6. That is the reason >> I had this simple xlate function. > > I see. In that case I'd say keep the custom of_xlate but let it return > -EINVAL if the args[0] value is not 4, 5, or 6. I will go for this approach (I though of it initially but let aside after) to also protect the other 2 resets (DDR controller and DDR PHY controller) which are at bits 0 and 2 in register at rstc->dev_base. Thank you, Claudiu Beznea > > Or you could set nr_resets to 7, but unless there are more resets at > the lower bits, that wouldn't necessarily be better. > > regards > Philipp