lkml.org 
[lkml]   [2018]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC 2/2] reset: reset-zynqmp: Adding support for Xilinx zynqmp reset controller.
From
Date
Hi,

On Sun, 2018-03-25 at 00:41 +0530, chinnikishore369@gmail.com wrote:
> From: Nava kishore Manne <navam@xilinx.com>
>
> Add a reset controller driver for Xilinx Zynq UltraScale+ MPSoC.
> The zynqmp reset-controller has the ability to reset lines
> connected to different blocks and peripheral in the Soc.
>
> Signed-off-by: Nava kishore Manne <navam@xilinx.com>

Thank you for the patch, this driver looks mostly fine to me.
I just have a few nitpicks below:

> ---
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-zynqmp.c | 106 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 107 insertions(+)
> create mode 100644 drivers/reset/reset-zynqmp.c
>
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 132c24f5ddb5..4e1a94acf83f 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -20,4 +20,5 @@ obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
> obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
> +obj-$(CONFIG_ARCH_ZYNQMP) += reset-zynqmp.o
>
> diff --git a/drivers/reset/reset-zynqmp.c b/drivers/reset/reset-zynqmp.c
> new file mode 100644
> index 000000000000..c8510fa93ad9
> --- /dev/null
> +++ b/drivers/reset/reset-zynqmp.c
> @@ -0,0 +1,106 @@
[...]
> +#include <linux/firmware/xilinx/zynqmp/firmware.h>
> +
> +#define ZYNQMP_NR_RESETS (ZYNQMP_PM_RESET_END - ZYNQMP_PM_RESET_START - 2)
> +#define ZYNQMP_RESET_ID (ZYNQMP_PM_RESET_START + 1)
> +
> +static const struct zynqmp_eemi_ops *eemi_ops;

This seems to depend on some other patches for the EEMI API support.
Could you point them out to me? I'd just like to see whether the
ZYNQMP_PM_RESET_* defines agree with the zynqmp-reset.txt DT docs.

> +struct zynqmp_reset {
> + struct reset_controller_dev rcdev;
> +};
> +
> +static int zynqmp_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)

This is not a hard requirement, but it would be nice if you could align
with the open parentheses such that checkpatch.pl --strict doesn't warn.
Same for the others below.

> +{
> + return eemi_ops->reset_assert(ZYNQMP_RESET_ID + id,
> + PM_RESET_ACTION_ASSERT);
> +}
> +
> +static int zynqmp_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + return eemi_ops->reset_assert(ZYNQMP_RESET_ID + id,
> + PM_RESET_ACTION_RELEASE);
> +}
> +
> +static int zynqmp_reset_status(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + int val;
> +
> + eemi_ops->reset_get_status(ZYNQMP_RESET_ID + id, &val);

Can reset_get_status return an error? If so, please propagate it.

> + return val;
> +}
> +
> +static int zynqmp_reset_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + return eemi_ops->reset_assert(ZYNQMP_RESET_ID + id,
> + PM_RESET_ACTION_PULSE);
> +}
> +
> +static struct reset_control_ops zynqmp_reset_ops = {
> + .reset = zynqmp_reset_reset,
> + .assert = zynqmp_reset_assert,
> + .deassert = zynqmp_reset_deassert,
> + .status = zynqmp_reset_status,
> +};
> +
> +static int zynqmp_reset_probe(struct platform_device *pdev)
> +{
> + struct zynqmp_reset *zynqmp_reset;
> + int ret;
> +
> + zynqmp_reset = devm_kzalloc(&pdev->dev,
> + sizeof(*zynqmp_reset), GFP_KERNEL);
> + if (!zynqmp_reset)
> + return -ENOMEM;
> +
> + eemi_ops = zynqmp_pm_get_eemi_ops();

Not sure if this can ever happen, but if there are two xlnx,zynqmp-reset
in the DT by accident, the probing the second would presumably return an
error here, thereby overwriting eemi_ops for the first one.
Perhaps it is safer to either use a local variable and only assign on
success, or better: move eemi_ops into the zynqmp_reset structure.

> + if (!eemi_ops)
> + return -ENXIO;
> +
> + platform_set_drvdata(pdev, zynqmp_reset);
> +
> + zynqmp_reset->rcdev.ops = &zynqmp_reset_ops;
> + zynqmp_reset->rcdev.owner = THIS_MODULE;
> + zynqmp_reset->rcdev.of_node = pdev->dev.of_node;
> + zynqmp_reset->rcdev.of_reset_n_cells = 1;

This line is not needed. If of_xlate is not set, the core will use
of_reset_simple_xlate and set of_reset_n_cells to 1.

> + zynqmp_reset->rcdev.nr_resets = ZYNQMP_NR_RESETS;
> +
> + ret = reset_controller_register(&zynqmp_reset->rcdev);
> + if (!ret)
> + dev_info(&pdev->dev, "Xilinx zynqmp reset driver probed\n");
> +
> + return ret;
> +}
> +
> +static const struct of_device_id zynqmp_reset_dt_ids[] = {
> + { .compatible = "xlnx,zynqmp-reset", },
> + { },
> +};
> +
> +static struct platform_driver zynqmp_reset_driver = {
> + .probe = zynqmp_reset_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = zynqmp_reset_dt_ids,
> + },
> +};
> +
> +static int __init zynqmp_reset_init(void)
> +{
> + return platform_driver_register(&zynqmp_reset_driver);
> +}
> +
> +arch_initcall(zynqmp_reset_init);

regards
Philipp

\
 
 \ /
  Last update: 2018-03-26 11:02    [W:0.048 / U:6.608 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site