| Message ID | 20180727184527.13287-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | Add Reset Controller support for Actions Semi Owl SoCs | expand |
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.sadhasivam@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; ? > + 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.sadhasivam@linaro.org> > + > +#include <linux/delay.h> > +#include <linux/io.h> This seems unnecessary, since register access is done via regmap. > +#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); > + > + 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); > + > + 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? > + 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. > + > + /* > + * 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.sadhasivam@linaro.org> > + > +#ifndef _OWL_RESET_H_ > +#define _OWL_RESET_H_ > + > +#include <linux/reset-controller.h> > +#include <linux/spinlock.h> spinlock? > + > +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. > + 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; ? > + 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mani, Am 27.07.2018 um 20:45 schrieb Manivannan Sadhasivam: > This patchset adds Reset Controller (RMU) support for Actions Semi > Owl SoCs, S900 and S700. For the Owl SoCs, RMU has been integrated into > the clock subsystem in hardware. Hence, in software we integrate RMU > support into common clock driver inorder to maintain compatibility. Can this not be placed into drivers/reset/ by using mfd-simple with a sub-node in DT? Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2018-07-30 at 20:41 +0530, Manivannan Sadhasivam wrote: > Hi Andreas, > > On Mon, Jul 30, 2018 at 12:26:07PM +0200, Andreas Färber wrote: > > Hi Mani, > > > > Am 27.07.2018 um 20:45 schrieb Manivannan Sadhasivam: > > > This patchset adds Reset Controller (RMU) support for Actions Semi > > > Owl SoCs, S900 and S700. For the Owl SoCs, RMU has been integrated into > > > the clock subsystem in hardware. Hence, in software we integrate RMU > > > support into common clock driver inorder to maintain compatibility. > > > > Can this not be placed into drivers/reset/ by using mfd-simple with a > > sub-node in DT? > > > > Actually I was not sure where to place this reset controller driver. When I > looked into other similar ones such as sunxi, they just integrated into the > clk subsystem. So I just chose that path. But yeah, this is hacky! > > But this RMU is not MFD by any means. Since the CMU (Clock) and RMU (Reset) > are two separate IPs inside SoC, we shouldn't describe it as a MFD driver. Since > RMU has only 2 registers, the HW designers decided to use up the CMU memory > map. So, maybe syscon would be best option I think. What is your opinion? Using syscon seems cleaner than stuffing the regmap into owl_clk_desc. > Even if we go for syscon, we should place the reset driver within clk > framework as I can see other SoCs like Mediatek are doing the same. But again > I'm not sure! Me neither. If the CMU and RMU are really separate and only share the memory map, a syscon driver could live in drivers/reset without problems. It's only when there are interactions between clocks and resets that you really want to have the reset driver integrated with clk. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Philipp, On Mon, Jul 30, 2018 at 05:38:31PM +0200, Philipp Zabel wrote: > On Mon, 2018-07-30 at 20:41 +0530, Manivannan Sadhasivam wrote: > > Hi Andreas, > > > > On Mon, Jul 30, 2018 at 12:26:07PM +0200, Andreas Färber wrote: > > > Hi Mani, > > > > > > Am 27.07.2018 um 20:45 schrieb Manivannan Sadhasivam: > > > > This patchset adds Reset Controller (RMU) support for Actions Semi > > > > Owl SoCs, S900 and S700. For the Owl SoCs, RMU has been integrated into > > > > the clock subsystem in hardware. Hence, in software we integrate RMU > > > > support into common clock driver inorder to maintain compatibility. > > > > > > Can this not be placed into drivers/reset/ by using mfd-simple with a > > > sub-node in DT? > > > > > > > Actually I was not sure where to place this reset controller driver. When I > > looked into other similar ones such as sunxi, they just integrated into the > > clk subsystem. So I just chose that path. But yeah, this is hacky! > > > > But this RMU is not MFD by any means. Since the CMU (Clock) and RMU (Reset) > > are two separate IPs inside SoC, we shouldn't describe it as a MFD driver. Since > > RMU has only 2 registers, the HW designers decided to use up the CMU memory > > map. So, maybe syscon would be best option I think. What is your opinion? > > Using syscon seems cleaner than stuffing the regmap into owl_clk_desc. > Okay. > > Even if we go for syscon, we should place the reset driver within clk > > framework as I can see other SoCs like Mediatek are doing the same. But again > > I'm not sure! > > Me neither. If the CMU and RMU are really separate and only share the > memory map, a syscon driver could live in drivers/reset without > problems. > It's only when there are interactions between clocks and resets that you > really want to have the reset driver integrated with clk. > Okay. Then I will add it as a syscon driver under drivers/reset. I hope that Andreas would also be happy with this. Thanks a lot for your response! Regards, Mani > regards > Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, On Tue, Aug 07, 2018 at 12:47:10PM -0600, Rob Herring wrote: > On Mon, Jul 30, 2018 at 08:41:31PM +0530, Manivannan Sadhasivam wrote: > > Hi Andreas, > > > > On Mon, Jul 30, 2018 at 12:26:07PM +0200, Andreas Färber wrote: > > > Hi Mani, > > > > > > Am 27.07.2018 um 20:45 schrieb Manivannan Sadhasivam: > > > > This patchset adds Reset Controller (RMU) support for Actions Semi > > > > Owl SoCs, S900 and S700. For the Owl SoCs, RMU has been integrated into > > > > the clock subsystem in hardware. Hence, in software we integrate RMU > > > > support into common clock driver inorder to maintain compatibility. > > > > > > Can this not be placed into drivers/reset/ by using mfd-simple with a > > > sub-node in DT? > > That is exactly what I tell folks not to do. Design the DT based on h/w > blocks, not current desired driver split for some OS. > > > Actually I was not sure where to place this reset controller driver. When I > > looked into other similar ones such as sunxi, they just integrated into the > > clk subsystem. So I just chose that path. But yeah, this is hacky! > > > > But this RMU is not MFD by any means. Since the CMU (Clock) and RMU (Reset) > > are two separate IPs inside SoC, we shouldn't describe it as a MFD driver. Since > > RMU has only 2 registers, the HW designers decided to use up the CMU memory > > map. So, maybe syscon would be best option I think. What is your opinion? > > If there's nothing shared then it is not a syscon. If you can create > separate address ranges, then 2 nodes is probably okay. If the registers > are all mixed up, then 1 node. > I don't quite understand the reason for not being syscon. The definition of syscon says that, "System controller node represents a register region containing a set of miscellaneous registers. The registers are not cohesive enough to represent as any specific type of device." which exactly fits this case. Only the registers of CMU & RMU are shared and not the HW block! Can you please clarify? Thanks, Mani > Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 8, 2018 at 11:30 AM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > Hi Rob, > > On Tue, Aug 07, 2018 at 12:47:10PM -0600, Rob Herring wrote: > > On Mon, Jul 30, 2018 at 08:41:31PM +0530, Manivannan Sadhasivam wrote: > > > Hi Andreas, > > > > > > On Mon, Jul 30, 2018 at 12:26:07PM +0200, Andreas Färber wrote: > > > > Hi Mani, > > > > > > > > Am 27.07.2018 um 20:45 schrieb Manivannan Sadhasivam: > > > > > This patchset adds Reset Controller (RMU) support for Actions Semi > > > > > Owl SoCs, S900 and S700. For the Owl SoCs, RMU has been integrated into > > > > > the clock subsystem in hardware. Hence, in software we integrate RMU > > > > > support into common clock driver inorder to maintain compatibility. > > > > > > > > Can this not be placed into drivers/reset/ by using mfd-simple with a > > > > sub-node in DT? > > > > That is exactly what I tell folks not to do. Design the DT based on h/w > > blocks, not current desired driver split for some OS. > > > > > Actually I was not sure where to place this reset controller driver. When I > > > looked into other similar ones such as sunxi, they just integrated into the > > > clk subsystem. So I just chose that path. But yeah, this is hacky! > > > > > > But this RMU is not MFD by any means. Since the CMU (Clock) and RMU (Reset) > > > are two separate IPs inside SoC, we shouldn't describe it as a MFD driver. Since > > > RMU has only 2 registers, the HW designers decided to use up the CMU memory > > > map. So, maybe syscon would be best option I think. What is your opinion? > > > > If there's nothing shared then it is not a syscon. If you can create > > separate address ranges, then 2 nodes is probably okay. If the registers > > are all mixed up, then 1 node. > > > > I don't quite understand the reason for not being syscon. The definition > of syscon says that, "System controller node represents a register region > containing a set of miscellaneous registers. The registers are not cohesive > enough to represent as any specific type of device." which exactly fits > this case. Only the registers of CMU & RMU are shared and not the HW block! > > Can you please clarify? IIRC, the original intent of 'syscon' was really for things that had no subsystem. Random bits all just dumped together. A block with clock and reset doesn't sounds pretty well defined functions. Plus that criteria doesn't work well because what does and doesn't have a subsystem (and DT binding) evolves. IMO, we should probably get rid of 'syscon'. Let me turn it around. Why do you want it do be a syscon? Simply to create a regmap I suppose because that is all that 'syscon' compatible is. A flag to create a regmap. Why do you need a regmap then? Do you need to protect concurrent accesses (a single register has both clock and reset bits). If so, you can't really call CMU and RMU 2 blocks. If not, you don't really need regmap. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This patchset adds Reset Controller (RMU) support for Actions Semi Owl SoCs, S900 and S700. For the Owl SoCs, RMU has been integrated into the clock subsystem in hardware. Hence, in software we integrate RMU support into common clock driver inorder to maintain compatibility. This patch series depends on the recently posted S700 clk series: "[PATCH v7 0/5] Add clock driver for Actions S700 SoC". For the S700 clk series, driver and bindings patches are applied through the clk tree. But the DTS patches are not yet picked up by the platform maintainer, Andreas. Hence, Andreas is expected to pick the DTS patches in this series once reviewed by the maintainers along with S700 clk DTS patches. Because of the absence of the S500 SoC clk support, the reset controller registration code is added to both S700 and S900 SoC clk drivers for now. But once S500 clk support is added, the reset controller registration part will be moved to Owl SoCs common clk code. Thanks, Mani Manivannan Sadhasivam (9): clk: actions: Cache regmap info in private clock descriptor dt-bindings: clock: Add reset controller bindings for Actions Semi Owl SoCs dt-bindings: reset: Add binding constants for Actions Semi S700 RMU dt-bindings: reset: Add binding constants for Actions Semi S900 RMU arm64: dts: actions: Add Reset Controller support for S700 SoC arm64: dts: actions: Add Reset Controller support for S900 SoC clk: actions: Add Actions Semi Owl SoCs Reset Management Unit support clk: actions: Add Actions Semi S700 SoC Reset Management Unit support clk: actions: Add Actions Semi S900 SoC Reset Management Unit support .../bindings/clock/actions,owl-cmu.txt | 2 + arch/arm64/boot/dts/actions/s700.dtsi | 2 + arch/arm64/boot/dts/actions/s900.dtsi | 2 + drivers/clk/actions/Kconfig | 1 + drivers/clk/actions/Makefile | 1 + drivers/clk/actions/owl-common.c | 3 +- drivers/clk/actions/owl-common.h | 5 +- drivers/clk/actions/owl-reset.c | 72 ++++++++++++++++ drivers/clk/actions/owl-reset.h | 32 +++++++ drivers/clk/actions/owl-s700.c | 55 +++++++++++- drivers/clk/actions/owl-s900.c | 86 ++++++++++++++++++- .../dt-bindings/reset/actions,s700-reset.h | 34 ++++++++ .../dt-bindings/reset/actions,s900-reset.h | 65 ++++++++++++++ 13 files changed, 354 insertions(+), 6 deletions(-) create mode 100644 drivers/clk/actions/owl-reset.c create mode 100644 drivers/clk/actions/owl-reset.h create mode 100644 include/dt-bindings/reset/actions,s700-reset.h create mode 100644 include/dt-bindings/reset/actions,s900-reset.h -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html