2020-07-14 06:24:09

by Sai Krishna Potthuri

[permalink] [raw]
Subject: [PATCH 2/2] reset: reset-zynqmp: Added support for Versal platform

Updated the reset driver to support Versal platform.
As part of adding Versal support
- Added Versal specific compatible string.
- Reset Id and number of resets are different for Versal and ZynqMP,
hence taken care of these two based on compatible string.

Signed-off-by: Sai Krishna Potthuri <[email protected]>
---
drivers/reset/reset-zynqmp.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/reset/reset-zynqmp.c b/drivers/reset/reset-zynqmp.c
index 373ea8d4f7a1..17aa4532ec5e 100644
--- a/drivers/reset/reset-zynqmp.c
+++ b/drivers/reset/reset-zynqmp.c
@@ -12,9 +12,11 @@

#define ZYNQMP_NR_RESETS (ZYNQMP_PM_RESET_END - ZYNQMP_PM_RESET_START)
#define ZYNQMP_RESET_ID ZYNQMP_PM_RESET_START
+#define VERSAL_NR_RESETS 95

struct zynqmp_reset_data {
struct reset_controller_dev rcdev;
+ u32 reset_id;
};

static inline struct zynqmp_reset_data *
@@ -26,23 +28,28 @@ to_zynqmp_reset_data(struct reset_controller_dev *rcdev)
static int zynqmp_reset_assert(struct reset_controller_dev *rcdev,
unsigned long id)
{
- return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
+ struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
+
+ return zynqmp_pm_reset_assert(priv->reset_id + id,
PM_RESET_ACTION_ASSERT);
}

static int zynqmp_reset_deassert(struct reset_controller_dev *rcdev,
unsigned long id)
{
- return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
+ struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
+
+ return zynqmp_pm_reset_assert(priv->reset_id + id,
PM_RESET_ACTION_RELEASE);
}

static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
unsigned long id)
{
+ struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
int val, err;

- err = zynqmp_pm_reset_get_status(ZYNQMP_RESET_ID + id, &val);
+ err = zynqmp_pm_reset_get_status(priv->reset_id + id, &val);
if (err)
return err;

@@ -52,7 +59,9 @@ static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
static int zynqmp_reset_reset(struct reset_controller_dev *rcdev,
unsigned long id)
{
- return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
+ struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
+
+ return zynqmp_pm_reset_assert(priv->reset_id + id,
PM_RESET_ACTION_PULSE);
}

@@ -76,13 +85,20 @@ static int zynqmp_reset_probe(struct platform_device *pdev)
priv->rcdev.ops = &zynqmp_reset_ops;
priv->rcdev.owner = THIS_MODULE;
priv->rcdev.of_node = pdev->dev.of_node;
+ priv->reset_id = ZYNQMP_RESET_ID;
priv->rcdev.nr_resets = ZYNQMP_NR_RESETS;
+ if (of_device_is_compatible(pdev->dev.of_node,
+ "xlnx,versal-reset")) {
+ priv->reset_id = 0;
+ priv->rcdev.nr_resets = VERSAL_NR_RESETS;
+ }

return devm_reset_controller_register(&pdev->dev, &priv->rcdev);
}

static const struct of_device_id zynqmp_reset_dt_ids[] = {
{ .compatible = "xlnx,zynqmp-reset", },
+ { .compatible = "xlnx,versal-reset", },
{ /* sentinel */ },
};

--
2.17.1


2020-07-20 09:51:27

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH 2/2] reset: reset-zynqmp: Added support for Versal platform

On Tue, 2020-07-14 at 11:59 +0530, Sai Krishna Potthuri wrote:
> Updated the reset driver to support Versal platform.
> As part of adding Versal support
> - Added Versal specific compatible string.
> - Reset Id and number of resets are different for Versal and ZynqMP,
> hence taken care of these two based on compatible string.
>
> Signed-off-by: Sai Krishna Potthuri <[email protected]>
> ---
> drivers/reset/reset-zynqmp.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/reset/reset-zynqmp.c b/drivers/reset/reset-zynqmp.c
> index 373ea8d4f7a1..17aa4532ec5e 100644
> --- a/drivers/reset/reset-zynqmp.c
> +++ b/drivers/reset/reset-zynqmp.c
> @@ -12,9 +12,11 @@
>
> #define ZYNQMP_NR_RESETS (ZYNQMP_PM_RESET_END - ZYNQMP_PM_RESET_START)
> #define ZYNQMP_RESET_ID ZYNQMP_PM_RESET_START
> +#define VERSAL_NR_RESETS 95
>
> struct zynqmp_reset_data {
> struct reset_controller_dev rcdev;
> + u32 reset_id;
> };
>
> static inline struct zynqmp_reset_data *
> @@ -26,23 +28,28 @@ to_zynqmp_reset_data(struct reset_controller_dev *rcdev)
> static int zynqmp_reset_assert(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> - return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> +
> + return zynqmp_pm_reset_assert(priv->reset_id + id,
> PM_RESET_ACTION_ASSERT);
> }
>
> static int zynqmp_reset_deassert(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> - return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> +
> + return zynqmp_pm_reset_assert(priv->reset_id + id,
> PM_RESET_ACTION_RELEASE);
> }
>
> static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> int val, err;
>
> - err = zynqmp_pm_reset_get_status(ZYNQMP_RESET_ID + id, &val);
> + err = zynqmp_pm_reset_get_status(priv->reset_id + id, &val);
> if (err)
> return err;
>
> @@ -52,7 +59,9 @@ static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> static int zynqmp_reset_reset(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> - return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
> + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> +
> + return zynqmp_pm_reset_assert(priv->reset_id + id,
> PM_RESET_ACTION_PULSE);
> }
>
> @@ -76,13 +85,20 @@ static int zynqmp_reset_probe(struct platform_device *pdev)
> priv->rcdev.ops = &zynqmp_reset_ops;
> priv->rcdev.owner = THIS_MODULE;
> priv->rcdev.of_node = pdev->dev.of_node;
> + priv->reset_id = ZYNQMP_RESET_ID;
> priv->rcdev.nr_resets = ZYNQMP_NR_RESETS;
> + if (of_device_is_compatible(pdev->dev.of_node,
> + "xlnx,versal-reset")) {

It would be better to use of_match_device and static const initalization
data for this.

> + priv->reset_id = 0;
> + priv->rcdev.nr_resets = VERSAL_NR_RESETS;

This won't work. All your reset ids are greater than 95, and this driver
is using the default of_xlate callback, so of_reset_simple_xlate will
fail all reset control requests with -EINVAL.

regards
Philipp

2020-07-20 14:01:15

by Sai Krishna Potthuri

[permalink] [raw]
Subject: RE: [PATCH 2/2] reset: reset-zynqmp: Added support for Versal platform

Hi Philipp,

Thanks for the review.

> -----Original Message-----
> From: Philipp Zabel <[email protected]>
> Sent: Monday, July 20, 2020 3:18 PM
> To: Sai Krishna Potthuri <[email protected]>; Rob Herring
> <[email protected]>; Michal Simek <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; git <[email protected]>; [email protected]
> Subject: Re: [PATCH 2/2] reset: reset-zynqmp: Added support for Versal
> platform
>
> On Tue, 2020-07-14 at 11:59 +0530, Sai Krishna Potthuri wrote:
> > Updated the reset driver to support Versal platform.
> > As part of adding Versal support
> > - Added Versal specific compatible string.
> > - Reset Id and number of resets are different for Versal and ZynqMP,
> > hence taken care of these two based on compatible string.
> >
> > Signed-off-by: Sai Krishna Potthuri
> > <[email protected]>
> > ---
> > drivers/reset/reset-zynqmp.c | 24 ++++++++++++++++++++----
> > 1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/reset/reset-zynqmp.c
> > b/drivers/reset/reset-zynqmp.c index 373ea8d4f7a1..17aa4532ec5e
> 100644
> > --- a/drivers/reset/reset-zynqmp.c
> > +++ b/drivers/reset/reset-zynqmp.c
> > @@ -12,9 +12,11 @@
> >
> > #define ZYNQMP_NR_RESETS (ZYNQMP_PM_RESET_END -
> > ZYNQMP_PM_RESET_START) #define ZYNQMP_RESET_ID
> ZYNQMP_PM_RESET_START
> > +#define VERSAL_NR_RESETS 95
> >
> > struct zynqmp_reset_data {
> > struct reset_controller_dev rcdev;
> > + u32 reset_id;
> > };
> >
> > static inline struct zynqmp_reset_data * @@ -26,23 +28,28 @@
> > to_zynqmp_reset_data(struct reset_controller_dev *rcdev) static int
> > zynqmp_reset_assert(struct reset_controller_dev *rcdev,
> > unsigned long id)
> > {
> > - return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
> > + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> > +
> > + return zynqmp_pm_reset_assert(priv->reset_id + id,
> > PM_RESET_ACTION_ASSERT);
> > }
> >
> > static int zynqmp_reset_deassert(struct reset_controller_dev *rcdev,
> > unsigned long id)
> > {
> > - return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
> > + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> > +
> > + return zynqmp_pm_reset_assert(priv->reset_id + id,
> > PM_RESET_ACTION_RELEASE);
> > }
> >
> > static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> > unsigned long id)
> > {
> > + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> > int val, err;
> >
> > - err = zynqmp_pm_reset_get_status(ZYNQMP_RESET_ID + id, &val);
> > + err = zynqmp_pm_reset_get_status(priv->reset_id + id, &val);
> > if (err)
> > return err;
> >
> > @@ -52,7 +59,9 @@ static int zynqmp_reset_status(struct
> > reset_controller_dev *rcdev, static int zynqmp_reset_reset(struct
> reset_controller_dev *rcdev,
> > unsigned long id)
> > {
> > - return zynqmp_pm_reset_assert(ZYNQMP_RESET_ID + id,
> > + struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
> > +
> > + return zynqmp_pm_reset_assert(priv->reset_id + id,
> > PM_RESET_ACTION_PULSE);
> > }
> >
> > @@ -76,13 +85,20 @@ static int zynqmp_reset_probe(struct
> platform_device *pdev)
> > priv->rcdev.ops = &zynqmp_reset_ops;
> > priv->rcdev.owner = THIS_MODULE;
> > priv->rcdev.of_node = pdev->dev.of_node;
> > + priv->reset_id = ZYNQMP_RESET_ID;
> > priv->rcdev.nr_resets = ZYNQMP_NR_RESETS;
> > + if (of_device_is_compatible(pdev->dev.of_node,
> > + "xlnx,versal-reset")) {
>
> It would be better to use of_match_device and static const initalization data
> for this.
Will create soc specific initialization data structure and assign based on
of_device_get_match_data().

>
> > + priv->reset_id = 0;
> > + priv->rcdev.nr_resets = VERSAL_NR_RESETS;
>
> This won't work. All your reset ids are greater than 95, and this driver is using
> the default of_xlate callback, so of_reset_simple_xlate will fail all reset
> control requests with -EINVAL.
Will create of_xlate callback that will simply return the reset line number
without any checks. We have underlying secure library which will validate
the reset line number.

Regards
Sai Krishna
>
> regards
> Philipp