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 <[email protected]>
> ---
> 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.
> +
> +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, 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>> ---
>> 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, [email protected] 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 <[email protected]>
> > > ---
> > > 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