| Message ID | 1596024483-21482-1-git-send-email-abel.vesa@nxp.com |
|---|---|
| Headers | show |
| Series | Add BLK_CTRL support for i.MX8MP | expand |
On 20-07-29 15:08:01, Abel Vesa wrote: > Some of the features of the audio_ctrl will be used by some > different drivers in a way those drivers will know best, so adding the > syscon compatible we allow those to do just that. Only the resets > and the clocks are registered bit the clk-blk-ctrl driver. > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > --- > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > index daa1769..b985875 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > @@ -736,6 +736,23 @@ > }; > }; > > + aips5: bus@30c00000 { > + compatible = "fsl,aips-bus", "simple-bus"; > + reg = <0x30c00000 0x400000>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + audio_blk_ctrl: audio-blk-ctrl@30e20000 { > + compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon"; > + reg = <0x30e20000 0x50C>; > + power-domains = <&audiomix_pd>; I forget to remote the power-domains property. Will remove in the next version. > + > + #clock-cells = <1>; > + #reset-cells = <1>; > + }; > + }; > + > gic: interrupt-controller@38800000 { > compatible = "arm,gic-v3"; > reg = <0x38800000 0x10000>, > -- > 2.7.4 >
On 20-07-29 15:08:02, Abel Vesa wrote: > Some of the features of the media_ctrl will be used by some > different drivers in a way those drivers will know best, so adding the > syscon compatible we allow those to do just that. Only the resets > and the clocks are registered bit the clk-blk-ctrl driver. > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > --- > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > index b985875..172c548 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > @@ -736,6 +736,23 @@ > }; > }; > > + aips4: bus@32c00000 { > + compatible = "simple-bus"; > + reg = <0x32c00000 0x400000>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + media_blk_ctrl: media-blk-ctrl@32ec0000 { > + compatible = "fsl,imx8mp-media-blk-ctrl", "syscon"; > + reg = <0x32ec0000 0x10000>; > + power-domains = <&mediamix_pd>; I forget to remove the power-domains property. Will remove in the next version. > + > + #clock-cells = <1>; > + #reset-cells = <1>; > + }; > + }; > + > aips5: bus@30c00000 { > compatible = "fsl,aips-bus", "simple-bus"; > reg = <0x30c00000 0x400000>; > -- > 2.7.4 >
On 20-07-29 15:08:03, Abel Vesa wrote: > Some of the features of the hdmi_ctrl will be used by some > different drivers in a way those drivers will know best, so adding the > syscon compatible we allow those to do just that. Only the resets > and the clocks are registered bit the clk-blk-ctrl driver. > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > --- > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > index 172c548..5a76c4d 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > @@ -751,6 +751,15 @@ > #clock-cells = <1>; > #reset-cells = <1>; > }; > + > + hdmi_blk_ctrl: hdmi-blk-ctrl@32fc0000 { > + compatible = "fsl,imx8mp-hdmi-blk-ctrl", "syscon"; > + reg = <0x32fc0000 0x1000>; > + power-domains = <&hdmimix_pd>; I forget to remove the power-domains property. Will remove in the next version. > + > + #clock-cells = <1>; > + #reset-cells = <1>; > + }; > }; > > aips5: bus@30c00000 { > -- > 2.7.4 >
Hi Abel, On Wed, 2020-07-29 at 15:07 +0300, Abel Vesa wrote: > On i.MX8MP, there is a new type of IP which is called BLK_CTRL in > RM and usually is comprised of some GPRs that are considered too > generic to be part of any dedicated IP from that specific subsystem. > > In general, some of the GPRs have some clock bits, some have reset bits, > so in order to be able to use the imx clock API, this needs to be > in a clock driver. From there it can use the reset controller API and > leave the rest to the syscon. > > This driver is intended to work with the following BLK_CTRL IPs found in > i.MX8MP (but it might be reused by the future i.MX platforms that > have this kind of IP in their design): > - Audio > - Media > - HDMI > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > --- > drivers/clk/imx/Makefile | 2 +- > drivers/clk/imx/clk-blk-ctrl.c | 318 +++++++++++++++++++++++++++++++++++++++++ > drivers/clk/imx/clk-blk-ctrl.h | 81 +++++++++++ > 3 files changed, 400 insertions(+), 1 deletion(-) > create mode 100644 drivers/clk/imx/clk-blk-ctrl.c > create mode 100644 drivers/clk/imx/clk-blk-ctrl.h > > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile > index 928f874c..7afe1df 100644 > --- a/drivers/clk/imx/Makefile > +++ b/drivers/clk/imx/Makefile > @@ -27,7 +27,7 @@ obj-$(CONFIG_MXC_CLK_SCU) += \ > > obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o > obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o > -obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o > +obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o clk-blk-ctrl.o > obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o > obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o > > diff --git a/drivers/clk/imx/clk-blk-ctrl.c b/drivers/clk/imx/clk-blk-ctrl.c > new file mode 100644 > index 00000000..a46e674 > --- /dev/null > +++ b/drivers/clk/imx/clk-blk-ctrl.c > @@ -0,0 +1,318 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright 2020 NXP. > + */ > + > +#include <linux/clk.h> > +#include <linux/reset-controller.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/slab.h> > +#include <linux/string.h> > +#include <linux/types.h> > + > +#include "clk.h" > +#include "clk-blk-ctrl.h" > + > +struct reset_hw { > + u32 offset; > + u32 shift; > + u32 mask; > +}; > + > +struct pm_safekeep_info { > + uint32_t *regs_values; > + uint32_t *regs_offsets; > + uint32_t regs_num; > +}; > + > +struct imx_blk_ctrl_drvdata { > + void __iomem *base; > + struct reset_controller_dev rcdev; > + struct reset_hw *rst_hws; > + struct pm_safekeep_info pm_info; > + > + spinlock_t lock; > +}; > + > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev, > + unsigned long id, bool assert) > +{ > + struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev, > + struct imx_blk_ctrl_drvdata, rcdev); > + unsigned int offset = drvdata->rst_hws[id].offset; > + unsigned int shift = drvdata->rst_hws[id].shift; > + unsigned int mask = drvdata->rst_hws[id].mask; > + void __iomem *reg_addr = drvdata->base + offset; > + unsigned long flags; > + u32 reg; > + > + if (assert) { > + pm_runtime_get_sync(rcdev->dev); > + spin_lock_irqsave(&drvdata->lock, flags); > + reg = readl(reg_addr); > + writel(reg & ~(mask << shift), reg_addr); > + spin_unlock_irqrestore(&drvdata->lock, flags); > + } else { > + spin_lock_irqsave(&drvdata->lock, flags); > + reg = readl(reg_addr); > + writel(reg | (mask << shift), reg_addr); > + spin_unlock_irqrestore(&drvdata->lock, flags); > + pm_runtime_put(rcdev->dev); This still has the issue of potentially letting exclusive reset control users break the device usage counter. Also shared reset control users start with deassert(), and you end probe with pm_runtime_put(), so the first shared reset control user that deasserts its reset will decrement the dev->power.usage_count to -1 ? For multiple resets being initially deasserted this would decrement multiple times. I think you'll have to track the (number of) asserted reset bits in this reset controller and limit when to call pm_runtime_get/put_sync(). > + } > + > + return 0; > +} > + > +static int imx_blk_ctrl_reset_reset(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + imx_blk_ctrl_reset_set(rcdev, id, true); > + return imx_blk_ctrl_reset_set(rcdev, id, false); Does this work for all peripherals? Are there none that require the reset line to be asserted for a certain number of bus clocks or similar? > +} > + > +static int imx_blk_ctrl_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return imx_blk_ctrl_reset_set(rcdev, id, true); > +} > + > +static int imx_blk_ctrl_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return imx_blk_ctrl_reset_set(rcdev, id, false); > +} > + > +static const struct reset_control_ops imx_blk_ctrl_reset_ops = { > + .reset = imx_blk_ctrl_reset_reset, > + .assert = imx_blk_ctrl_reset_assert, > + .deassert = imx_blk_ctrl_reset_deassert, > +}; > + > +static int imx_blk_ctrl_register_reset_controller(struct device *dev) > +{ > + struct imx_blk_ctrl_drvdata *drvdata = dev_get_drvdata(dev); > + const struct imx_blk_ctrl_dev_data *dev_data = of_device_get_match_data(dev); > + struct reset_hw *hws; > + int max = dev_data->resets_max; > + int i; > + > + spin_lock_init(&drvdata->lock); > + > + drvdata->rcdev.owner = THIS_MODULE; > + drvdata->rcdev.nr_resets = max; > + drvdata->rcdev.ops = &imx_blk_ctrl_reset_ops; > + drvdata->rcdev.of_node = dev->of_node; > + drvdata->rcdev.dev = dev; > + > + drvdata->rst_hws = devm_kzalloc(dev, sizeof(struct reset_hw) * max, > + GFP_KERNEL); I'd use devm_kcalloc() here. > + hws = drvdata->rst_hws; > + > + for (i = 0; i < dev_data->hws_num; i++) { > + struct imx_blk_ctrl_hw *hw = &dev_data->hws[i]; > + > + if (hw->type != BLK_CTRL_RESET) > + continue; > + > + hws[hw->id].offset = hw->offset; > + hws[hw->id].shift = hw->shift; > + hws[hw->id].mask = hw->mask; > + } > + > + return devm_reset_controller_register(dev, &drvdata->rcdev); > +} [...] regards Philipp
On 20-07-29 14:46:28, Philipp Zabel wrote: > Hi Abel, > > On Wed, 2020-07-29 at 15:07 +0300, Abel Vesa wrote: > > On i.MX8MP, there is a new type of IP which is called BLK_CTRL in [...] > > + > > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev, > > + unsigned long id, bool assert) > > +{ > > + struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev, > > + struct imx_blk_ctrl_drvdata, rcdev); > > + unsigned int offset = drvdata->rst_hws[id].offset; > > + unsigned int shift = drvdata->rst_hws[id].shift; > > + unsigned int mask = drvdata->rst_hws[id].mask; > > + void __iomem *reg_addr = drvdata->base + offset; > > + unsigned long flags; > > + u32 reg; > > + > > + if (assert) { > > + pm_runtime_get_sync(rcdev->dev); > > + spin_lock_irqsave(&drvdata->lock, flags); > > + reg = readl(reg_addr); > > + writel(reg & ~(mask << shift), reg_addr); > > + spin_unlock_irqrestore(&drvdata->lock, flags); > > + } else { > > + spin_lock_irqsave(&drvdata->lock, flags); > > + reg = readl(reg_addr); > > + writel(reg | (mask << shift), reg_addr); > > + spin_unlock_irqrestore(&drvdata->lock, flags); > > + pm_runtime_put(rcdev->dev); > > This still has the issue of potentially letting exclusive reset control > users break the device usage counter. > > Also shared reset control users start with deassert(), and you end probe > with pm_runtime_put(), so the first shared reset control user that > deasserts its reset will decrement the dev->power.usage_count to -1 ? > For multiple resets being initially deasserted this would decrement > multiple times. > > I think you'll have to track the (number of) asserted reset bits in this > reset controller and limit when to call pm_runtime_get/put_sync(). > Yes, you're right. I'll add a mask, and for each assert, the according bit will get set, and for each deasssert the same bit will get cleared. And when the mask has at least one bit set, the pm_runtime_get gets called and when the mask is 0, the pm_runtime_put_sync will be called. Does that sound OK ? > > + } > > + > > + return 0; > > +} > > + > > +static int imx_blk_ctrl_reset_reset(struct reset_controller_dev *rcdev, > > + unsigned long id) > > +{ > > + imx_blk_ctrl_reset_set(rcdev, id, true); > > + return imx_blk_ctrl_reset_set(rcdev, id, false); > > Does this work for all peripherals? Are there none that require the > reset line to be asserted for a certain number of bus clocks or similar? As of now, there is no user that calls reset. All the users call the assert and then deassert. As for the number of clocks for reset, I'll try to have a chat to the HW design team and then come back with the information. > > > +} > > +
On Thu, 2020-07-30 at 11:55 +0300, Abel Vesa wrote: > On 20-07-29 14:46:28, Philipp Zabel wrote: > > Hi Abel, > > > > On Wed, 2020-07-29 at 15:07 +0300, Abel Vesa wrote: > > > On i.MX8MP, there is a new type of IP which is called BLK_CTRL in > > [...] > > > > + > > > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev, > > > + unsigned long id, bool assert) > > > +{ > > > + struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev, > > > + struct imx_blk_ctrl_drvdata, rcdev); > > > + unsigned int offset = drvdata->rst_hws[id].offset; > > > + unsigned int shift = drvdata->rst_hws[id].shift; > > > + unsigned int mask = drvdata->rst_hws[id].mask; > > > + void __iomem *reg_addr = drvdata->base + offset; > > > + unsigned long flags; > > > + u32 reg; > > > + > > > + if (assert) { > > > + pm_runtime_get_sync(rcdev->dev); > > > + spin_lock_irqsave(&drvdata->lock, flags); > > > + reg = readl(reg_addr); > > > + writel(reg & ~(mask << shift), reg_addr); > > > + spin_unlock_irqrestore(&drvdata->lock, flags); > > > + } else { > > > + spin_lock_irqsave(&drvdata->lock, flags); > > > + reg = readl(reg_addr); > > > + writel(reg | (mask << shift), reg_addr); > > > + spin_unlock_irqrestore(&drvdata->lock, flags); > > > + pm_runtime_put(rcdev->dev); > > > > This still has the issue of potentially letting exclusive reset control > > users break the device usage counter. > > > > Also shared reset control users start with deassert(), and you end probe > > with pm_runtime_put(), so the first shared reset control user that > > deasserts its reset will decrement the dev->power.usage_count to -1 ? > > For multiple resets being initially deasserted this would decrement > > multiple times. > > > > I think you'll have to track the (number of) asserted reset bits in this > > reset controller and limit when to call pm_runtime_get/put_sync(). > > > > Yes, you're right. > > I'll add a mask, and for each assert, the according bit will get set, and > for each deasssert the same bit will get cleared. > And when the mask has at least one bit set, the pm_runtime_get gets called ^ When the mask was 0 before but now has a bit set. > and when the mask is 0, the pm_runtime_put_sync will be called. ^ When the mask had a bit set but now is 0. > Does that sound OK ? And the mask starts out as 0, as after the pm_runtime_put() in probe all reset lines are deasserted? > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int imx_blk_ctrl_reset_reset(struct reset_controller_dev *rcdev, > > > + unsigned long id) > > > +{ > > > + imx_blk_ctrl_reset_set(rcdev, id, true); > > > + return imx_blk_ctrl_reset_set(rcdev, id, false); > > > > Does this work for all peripherals? Are there none that require the > > reset line to be asserted for a certain number of bus clocks or similar? > > As of now, there is no user that calls reset. All the users call the assert > and then deassert. As for the number of clocks for reset, I'll try to have a > chat to the HW design team and then come back with the information. Ok. If this is not required or can't be guaranteed to work, it may be better to just leave it out. regards Philipp
On 20-07-30 11:39:22, Philipp Zabel wrote: > On Thu, 2020-07-30 at 11:55 +0300, Abel Vesa wrote: > > On 20-07-29 14:46:28, Philipp Zabel wrote: > > > Hi Abel, > > > > > > On Wed, 2020-07-29 at 15:07 +0300, Abel Vesa wrote: > > > > On i.MX8MP, there is a new type of IP which is called BLK_CTRL in > > > > [...] > > > > > > + > > > > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev, > > > > + unsigned long id, bool assert) > > > > +{ > > > > + struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev, > > > > + struct imx_blk_ctrl_drvdata, rcdev); > > > > + unsigned int offset = drvdata->rst_hws[id].offset; > > > > + unsigned int shift = drvdata->rst_hws[id].shift; > > > > + unsigned int mask = drvdata->rst_hws[id].mask; > > > > + void __iomem *reg_addr = drvdata->base + offset; > > > > + unsigned long flags; > > > > + u32 reg; > > > > + > > > > + if (assert) { > > > > + pm_runtime_get_sync(rcdev->dev); > > > > + spin_lock_irqsave(&drvdata->lock, flags); > > > > + reg = readl(reg_addr); > > > > + writel(reg & ~(mask << shift), reg_addr); > > > > + spin_unlock_irqrestore(&drvdata->lock, flags); > > > > + } else { > > > > + spin_lock_irqsave(&drvdata->lock, flags); > > > > + reg = readl(reg_addr); > > > > + writel(reg | (mask << shift), reg_addr); > > > > + spin_unlock_irqrestore(&drvdata->lock, flags); > > > > + pm_runtime_put(rcdev->dev); > > > > > > This still has the issue of potentially letting exclusive reset control > > > users break the device usage counter. > > > > > > Also shared reset control users start with deassert(), and you end probe > > > with pm_runtime_put(), so the first shared reset control user that > > > deasserts its reset will decrement the dev->power.usage_count to -1 ? > > > For multiple resets being initially deasserted this would decrement > > > multiple times. > > > > > > I think you'll have to track the (number of) asserted reset bits in this > > > reset controller and limit when to call pm_runtime_get/put_sync(). > > > > > > > Yes, you're right. > > > > I'll add a mask, and for each assert, the according bit will get set, and > > for each deasssert the same bit will get cleared. > > > And when the mask has at least one bit set, the pm_runtime_get gets called > > ^ When the mask was 0 before but now has a bit set. > > > and when the mask is 0, the pm_runtime_put_sync will be called. > > ^ When the mask had a bit set but now is 0. > > > Does that sound OK ? > > And the mask starts out as 0, as after the pm_runtime_put() in probe all > reset lines are deasserted? > Yes, that is correct. > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int imx_blk_ctrl_reset_reset(struct reset_controller_dev *rcdev, > > > > + unsigned long id) > > > > +{ > > > > + imx_blk_ctrl_reset_set(rcdev, id, true); > > > > + return imx_blk_ctrl_reset_set(rcdev, id, false); > > > > > > Does this work for all peripherals? Are there none that require the > > > reset line to be asserted for a certain number of bus clocks or similar? > > > > As of now, there is no user that calls reset. All the users call the assert > > and then deassert. As for the number of clocks for reset, I'll try to have a > > chat to the HW design team and then come back with the information. > > Ok. If this is not required or can't be guaranteed to work, it may be > better to just leave it out. > > regards > Philipp
The BLK_CTRL according to HW design is basically the wrapper of the entire function specific group of IPs and holds GPRs that usually cannot be placed into one specific IP from that group. Some of these GPRs are used to control some clocks, other some resets, others some very specific function that does not fit into clocks or resets. Since the clocks are registered using the i.MX clock subsystem API, the driver is placed into the clock subsystem, but it also registers the resets. For the other functionalities that other GPRs might have, the syscon is used. Abel Vesa (17): dt-bindings: clocks: imx8mp: Rename audiomix ids clocks to audio_blk_ctrl dt-bindings: reset: imx8mp: Add audio blk_ctrl reset IDs dt-bindings: clock: imx8mp: Add ids for the audio shared gate dt-bindings: clock: imx8mp: Add media blk_ctrl clock IDs dt-bindings: reset: imx8mp: Add media blk_ctrl reset IDs dt-bindings: clock: imx8mp: Add hdmi blk_ctrl clock IDs dt-bindings: reset: imx8mp: Add hdmi blk_ctrl reset IDs clk: imx8mp: Add audio shared gate arm64: dts: Remove imx-hdmimix-reset header file Documentation: bindings: clk: Add bindings for i.MX BLK_CTRL clk: imx: Add blk_ctrl combo driver clk: imx8mp: Add audio blk_ctrl clocks and resets clk: imx8mp: Add hdmi blk_ctrl clocks and resets clk: imx8mp: Add media blk_ctrl clocks and resets arm64: dts: imx8mp: Add audio_blk_ctrl node arm64: dts: imx8mp: Add media_blk_ctrl node arm64: dts: imx8mp: Add hdmi_blk_ctrl node .../bindings/clock/fsl,imx-blk-ctrl.yaml | 55 ++++ arch/arm64/boot/dts/freescale/imx8mp.dtsi | 44 +++ drivers/clk/imx/Makefile | 2 +- drivers/clk/imx/clk-blk-ctrl.c | 330 +++++++++++++++++++++ drivers/clk/imx/clk-blk-ctrl.h | 81 +++++ drivers/clk/imx/clk-imx8mp.c | 281 +++++++++++++++++- include/dt-bindings/clock/imx8mp-clock.h | 200 +++++++++---- include/dt-bindings/reset/imx8mp-reset.h | 45 +++ 8 files changed, 975 insertions(+), 63 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/fsl,imx-blk-ctrl.yaml create mode 100644 drivers/clk/imx/clk-blk-ctrl.c create mode 100644 drivers/clk/imx/clk-blk-ctrl.h