Hi Philipp,
On Mon, Jul 30, 2018 at 12:21:52PM +0200, Philipp Zabel wrote:
> Hi Manivannan,
>
> Thank you for the patch, a few small comments below:
>
> On Sat, 2018-07-28 at 00:15 +0530, Manivannan Sadhasivam wrote:
> > Add Reset Management Unit (RMU) support for Actions Semi Owl SoCs.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasi...@linaro.org>
> > ---
> > drivers/clk/actions/Kconfig | 1 +
> > drivers/clk/actions/Makefile | 1 +
> > drivers/clk/actions/owl-common.h | 2 +
> > drivers/clk/actions/owl-reset.c | 72 ++++++++++++++++++++++++++++++++
> > drivers/clk/actions/owl-reset.h | 32 ++++++++++++++
> > 5 files changed, 108 insertions(+)
> > create mode 100644 drivers/clk/actions/owl-reset.c
> > create mode 100644 drivers/clk/actions/owl-reset.h
> >
> > diff --git a/drivers/clk/actions/Kconfig b/drivers/clk/actions/Kconfig
> > index dc38c85a4833..04f0a6355726 100644
> > --- a/drivers/clk/actions/Kconfig
> > +++ b/drivers/clk/actions/Kconfig
> > @@ -2,6 +2,7 @@ config CLK_ACTIONS
> > bool "Clock driver for Actions Semi SoCs"
> > depends on ARCH_ACTIONS || COMPILE_TEST
> > select REGMAP_MMIO
> > + select RESET_CONTROLLER
> > default ARCH_ACTIONS
> >
> > if CLK_ACTIONS
> > diff --git a/drivers/clk/actions/Makefile b/drivers/clk/actions/Makefile
> > index 78c17d56f991..ccfdf9781cef 100644
> > --- a/drivers/clk/actions/Makefile
> > +++ b/drivers/clk/actions/Makefile
> > @@ -7,6 +7,7 @@ clk-owl-y += owl-divider.o
> > clk-owl-y += owl-factor.o
> > clk-owl-y += owl-composite.o
> > clk-owl-y += owl-pll.o
> > +clk-owl-y += owl-reset.o
> >
> > # SoC support
> > obj-$(CONFIG_CLK_OWL_S700) += owl-s700.o
> > diff --git a/drivers/clk/actions/owl-common.h
> > b/drivers/clk/actions/owl-common.h
> > index 56f01f7774aa..4dc7f286831f 100644
> > --- a/drivers/clk/actions/owl-common.h
> > +++ b/drivers/clk/actions/owl-common.h
> > @@ -26,6 +26,8 @@ struct owl_clk_desc {
> > struct owl_clk_common **clks;
> > unsigned long num_clks;
> > struct clk_hw_onecell_data *hw_clks;
> > + struct owl_reset_map *resets;
>
> Could this be:
> const struct owl_reset_map *resets;
> ?
>
Ack.
> > + unsigned long num_resets;
> > struct regmap *regmap;
> > };
> >
> > diff --git a/drivers/clk/actions/owl-reset.c
> > b/drivers/clk/actions/owl-reset.c
> > new file mode 100644
> > index 000000000000..91b161cc68de
> > --- /dev/null
> > +++ b/drivers/clk/actions/owl-reset.c
> > @@ -0,0 +1,72 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +//
> > +// Actions Semi Owl SoCs Reset Management Unit driver
> > +//
> > +// Copyright (c) 2018 Linaro Ltd.
> > +// Author: Manivannan Sadhasivam <manivannan.sadhasi...@linaro.org>
> > +
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
>
> This seems unnecessary, since register access is done via regmap.
>
Will remove this header.
> > +#include <linux/regmap.h>
> > +#include <linux/reset-controller.h>
> > +
> > +#include "owl-reset.h"
> > +
> > +static int owl_reset_assert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + struct owl_reset *reset = to_owl_reset(rcdev);
> > + const struct owl_reset_map *map = &reset->reset_map[id];
> > + u32 reg;
> > +
> > + regmap_read(reset->regmap, map->reg, ®);
> > + regmap_write(reset->regmap, map->reg, reg & ~map->bit);
>
> This read-modify-write sequence needs locking against concurrent
> register access. Better use regmap_update_bits():
>
> return regmap_update_bits(reset->regmap, map->reg, map->bit, 0);
>
Ack.
> > +
> > + return 0;
> > +}
> > +
> > +static int owl_reset_deassert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + struct owl_reset *reset = to_owl_reset(rcdev);
> > + const struct owl_reset_map *map = &reset->reset_map[id];
> > + u32 reg;
> > +
> > + regmap_read(reset->regmap, map->reg, ®);
> > + regmap_write(reset->regmap, map->reg, reg | map->bit);
>
> Better:
>
> return regmap_update_bits(reset->regmap, map->reg, map->bit, map->bit);
>
Ack.
> > +
> > + return 0;
> > +}
> > +
> > +static int owl_reset_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + owl_reset_assert(rcdev, id);
> > + udelay(1);
>
> Is the delay valid for all IP cores on all SoCs variants?
>
It is valid for S900 and S700 for now but I'm not sure about S500. Will
make sure while adding S500 support.
> > + owl_reset_deassert(rcdev, id);
> > +
> > + return 0;
> > +}
> > +
> > +static int owl_reset_status(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + struct owl_reset *reset = to_owl_reset(rcdev);
> > + const struct owl_reset_map *map = &reset->reset_map[id];
> > + u32 reg;
> > +
> > + regmap_read(reset->regmap, map->reg, ®);
>
> If this could return an error, better report it.
>
Ack.
> > +
> > + /*
> > + * The reset control API expects 0 if reset is not asserted,
> > + * which is the opposite of what our hardware uses.
> > + */
> > + return !(map->bit & reg);
> > +}
> > +
> > +const struct reset_control_ops owl_reset_ops = {
> > + .assert = owl_reset_assert,
> > + .deassert = owl_reset_deassert,
> > + .reset = owl_reset_reset,
> > + .status = owl_reset_status,
> > +};
> > diff --git a/drivers/clk/actions/owl-reset.h
> > b/drivers/clk/actions/owl-reset.h
> > new file mode 100644
> > index 000000000000..1a5e987ba99b
> > --- /dev/null
> > +++ b/drivers/clk/actions/owl-reset.h
> > @@ -0,0 +1,32 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +//
> > +// Actions Semi Owl SoCs Reset Management Unit driver
> > +//
> > +// Copyright (c) 2018 Linaro Ltd.
> > +// Author: Manivannan Sadhasivam <manivannan.sadhasi...@linaro.org>
> > +
> > +#ifndef _OWL_RESET_H_
> > +#define _OWL_RESET_H_
> > +
> > +#include <linux/reset-controller.h>
> > +#include <linux/spinlock.h>
>
> spinlock?
>
Oops. Not needed, will remove that.
> > +
> > +struct owl_reset_map {
> > + u16 reg;
>
> Note that this will be aligned to 32-bits. If the intention was to save
> space, consider storing an u8 bit index instead of the mask.
>
Better to change it to u32, no need of memory saving here :)
> > + u32 bit;
> > +};
> > +
> > +struct owl_reset {
> > + struct reset_controller_dev rcdev;
> > + struct owl_reset_map *reset_map;
>
> Could this be
> const struct owl_reset_map *reset_map;
> ?
>
Ack.
Thanks,
Mani
> > + struct regmap *regmap;
> > +};
> > +
> > +static inline struct owl_reset *to_owl_reset(struct reset_controller_dev
> > *rcdev)
> > +{
> > + return container_of(rcdev, struct owl_reset, rcdev);
> > +}
> > +
> > +extern const struct reset_control_ops owl_reset_ops;
> > +
> > +#endif /* _OWL_RESET_H_ */
>
> regards
> Philipp
