| Message ID | 20240913095622.72377-9-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hello Dario, On 13.09.24 11:55, Dario Binacchi wrote: > From: Michael Trimarchi <michael@amarulasolutions.com> > > Add iMX8 block ctrl driver for displaymix on iMX8MM/iMX8MN and > mediamix on iMX8MP. > > To support blk ctrl driver, the power domain driver on iMX8M needs > update to add relevant PGC domains > > Signed-off-by: Ye Li <ye.li@nxp.com> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > --- > > drivers/power/domain/Kconfig | 6 + > drivers/power/domain/Makefile | 1 + > drivers/power/domain/imx8m-blk-ctrl.c | 438 ++++++++++++++++++++++ > drivers/power/domain/imx8m-power-domain.c | 213 ++++++++++- > 4 files changed, 656 insertions(+), 2 deletions(-) > create mode 100644 drivers/power/domain/imx8m-blk-ctrl.c > > diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig > index bd82d2f7044b..fb006b6e8e28 100644 > --- a/drivers/power/domain/Kconfig > +++ b/drivers/power/domain/Kconfig > @@ -40,6 +40,12 @@ config IMX8M_POWER_DOMAIN > Enable support for manipulating NXP i.MX8M on-SoC power domains via > requests to the ATF. > > +config IMX8M_BLK_CTRL > + bool "Enable i.MX8M block control driver" > + depends on POWER_DOMAIN && ARCH_IMX8M > + help > + Enable support for manipulating NXP i.MX8M on-SoC block control driver > + > config IMX8MP_HSIOMIX_BLKCTRL > bool "Enable i.MX8MP HSIOMIX domain driver" > depends on POWER_DOMAIN && IMX8MP > diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile > index 2daab73eb758..46849fd2a4db 100644 > --- a/drivers/power/domain/Makefile > +++ b/drivers/power/domain/Makefile > @@ -8,6 +8,7 @@ obj-$(CONFIG_APPLE_PMGR_POWER_DOMAIN) += apple-pmgr.o > obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o > obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o > obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o > +obj-$(CONFIG_IMX8M_BLK_CTRL) += imx8m-blk-ctrl.o > obj-$(CONFIG_IMX8MP_HSIOMIX_BLKCTRL) += imx8mp-hsiomix.o > obj-$(CONFIG_MTK_POWER_DOMAIN) += mtk-power-domain.o > obj-$(CONFIG_MESON_GX_VPU_POWER_DOMAIN) += meson-gx-pwrc-vpu.o > diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c > new file mode 100644 > index 000000000000..4c89078b991b > --- /dev/null > +++ b/drivers/power/domain/imx8m-blk-ctrl.c > @@ -0,0 +1,438 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2023 NXP > + */ > + > +#include <dm.h> > +#include <malloc.h> > +#include <power-domain-uclass.h> > +#include <asm/io.h> > +#include <dm/device-internal.h> > +#include <dm/device.h> > +#include <dt-bindings/power/imx8mm-power.h> > +#include <dt-bindings/power/imx8mn-power.h> > +#include <dt-bindings/power/imx8mp-power.h> > +#include <clk.h> > +#include <linux/delay.h> > + > +#define BLK_SFT_RSTN 0x0 > +#define BLK_CLK_EN 0x4 > +#define BLK_MIPI_RESET_DIV 0x8 /* Mini/Nano/Plus DISPLAY_BLK_CTRL only */ > + > +#define DOMAIN_MAX_CLKS 4 > + > +struct imx8m_blk_ctrl_domain { > + struct clk clks[DOMAIN_MAX_CLKS]; > + struct power_domain power_dev; > +}; > + > +struct imx8m_blk_ctrl { > + void __iomem *base; > + struct power_domain bus_power_dev; > + struct imx8m_blk_ctrl_domain *domains; > +}; > + > +struct imx8m_blk_ctrl_domain_data { > + const char *name; > + const char * const *clk_names; > + const char *gpc_name; > + int num_clks; > + u32 rst_mask; > + u32 clk_mask; > + u32 mipi_phy_rst_mask; > +}; > + > +struct imx8m_blk_ctrl_data { > + int max_reg; > + const struct imx8m_blk_ctrl_domain_data *domains; > + int num_domains; > + u32 bus_rst_mask; > + u32 bus_clk_mask; > +}; > + > +static int imx8m_blk_ctrl_request(struct power_domain *power_domain) > +{ > + return 0; > +} > + > +static int imx8m_blk_ctrl_free(struct power_domain *power_domain) > +{ > + return 0; > +} > + > +static int imx8m_blk_ctrl_enable_domain_clk(struct udevice *dev, ulong domain_id, bool enable) > +{ > + int ret, i; > + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); > + struct imx8m_blk_ctrl_data *drv_data = > + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); > + > + debug("%s num_clk %u\n", __func__, drv_data->domains[domain_id].num_clks); > + > + for (i = 0; i < drv_data->domains[domain_id].num_clks; i++) { > + debug("%s clk %s\n", __func__, drv_data->domains[domain_id].clk_names[i]); > + if (enable) > + ret = clk_enable(&priv->domains[domain_id].clks[i]); > + else > + ret = clk_disable(&priv->domains[domain_id].clks[i]); > + if (ret && ret != -ENOENT) { > + printf("Failed to %s domain clk %s\n", enable ? "enable" : "disable", > + drv_data->domains[domain_id].clk_names[i]); > + return ret; > + } > + } > + > + return 0; > +} > + > +static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) > +{ > + struct udevice *dev = power_domain->dev; > + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); > + struct imx8m_blk_ctrl_data *drv_data = > + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); > + int ret; > + > + debug("%s, id %lu\n", __func__, power_domain->id); > + > + if (!priv->domains[power_domain->id].power_dev.dev) > + return -ENODEV; > + > + ret = power_domain_on(&priv->bus_power_dev); > + if (ret < 0) { > + printf("Failed to power up bus domain %d\n", ret); > + return ret; > + } > + > + /* Enable bus clock and deassert bus reset */ > + setbits_le32(priv->base + BLK_CLK_EN, drv_data->bus_clk_mask); > + setbits_le32(priv->base + BLK_SFT_RSTN, drv_data->bus_rst_mask); > + > + /* wait for reset to propagate */ > + udelay(5); > + > + /* put devices into reset */ > + clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); > + if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) > + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d > + rv_data->domains[power_domain->id].mipi_phy_rst_mask); Does this build for you? I needed the following fix: diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c index 4c89078b99..b772d50480 100644 --- a/drivers/power/domain/imx8m-blk-ctrl.c +++ b/drivers/power/domain/imx8m-blk-ctrl.c @@ -114,8 +114,8 @@ static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) /* put devices into reset */ clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) - clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d - rv_data->domains[power_domain->id].mipi_phy_rst_mask); + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, + drv_data->domains[power_domain->id].mipi_phy_rst_mask); /* enable upstream and blk-ctrl clocks to allow reset to propagate */ ret = imx8m_blk_ctrl_enable_domain_clk(dev, power_domain->id, true); to get it building. BTW: Just also working on bootlogo support for an imx8mp based board! I now applied your patches: clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present for gate and mux clk: clk-uclass: Implement CLK_OPS_PARENT_ENABLE And added in my adapted /drivers/clk/imx/clk-imx8mp.c (imported from linux) clk_dm(IMX8MP_CLK_MEDIA_AXI, imx8m_clk_composite_flags("media_axi", imx8mp_media_axi_sels, ARRAY_SIZE(imx8mp_media_axi_sels), base + 0x8a00, CLK_IS_CRITICAL)); instead of using imx8m_clk_composite, and dropped my approach to get clocks up and working. Also dropped my similiar approach for mediablock and used your power: Add iMX8M block ctrl driver for dispmix And with this 3 patches, bootlogo works also/still fine for me! I did not applied/need your patches: clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate clk: imx8mm: Prevent clock critical path from disabling during reparent and set_rate clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled clk: imx8mn: Mark IMX8MN_SYS_PLL2 and IMX8MN_SYS_PLL3 as enabled Of course, they are for imx8mm, but I mean I do not need similiar patches for imx8mp! Also not applied (as for imx8mm) clk: imx8mn: add video clocks support but as said, have similiar patch for clk-imx8mp.c May you check if using imx8m_clk_composite_flags() is working for you? I did not applied your patches 09/26 and the following patches from your series, as I made a video bridge driver based on linux driver drivers/gpu/drm/bridge/fsl-ldb.c and a lcdif driver based on linux:/drivers/gpu/drm/mxsfb/lcdif_drv.c bye, Heiko
Hello Heiko, On Tue, Sep 24, 2024 at 11:07 AM Heiko Schocher <hs@denx.de> wrote: > > Hello Dario, > > On 13.09.24 11:55, Dario Binacchi wrote: > > From: Michael Trimarchi <michael@amarulasolutions.com> > > > > Add iMX8 block ctrl driver for displaymix on iMX8MM/iMX8MN and > > mediamix on iMX8MP. > > > > To support blk ctrl driver, the power domain driver on iMX8M needs > > update to add relevant PGC domains > > > > Signed-off-by: Ye Li <ye.li@nxp.com> > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > --- > > > > drivers/power/domain/Kconfig | 6 + > > drivers/power/domain/Makefile | 1 + > > drivers/power/domain/imx8m-blk-ctrl.c | 438 ++++++++++++++++++++++ > > drivers/power/domain/imx8m-power-domain.c | 213 ++++++++++- > > 4 files changed, 656 insertions(+), 2 deletions(-) > > create mode 100644 drivers/power/domain/imx8m-blk-ctrl.c > > > > diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig > > index bd82d2f7044b..fb006b6e8e28 100644 > > --- a/drivers/power/domain/Kconfig > > +++ b/drivers/power/domain/Kconfig > > @@ -40,6 +40,12 @@ config IMX8M_POWER_DOMAIN > > Enable support for manipulating NXP i.MX8M on-SoC power domains via > > requests to the ATF. > > > > +config IMX8M_BLK_CTRL > > + bool "Enable i.MX8M block control driver" > > + depends on POWER_DOMAIN && ARCH_IMX8M > > + help > > + Enable support for manipulating NXP i.MX8M on-SoC block control driver > > + > > config IMX8MP_HSIOMIX_BLKCTRL > > bool "Enable i.MX8MP HSIOMIX domain driver" > > depends on POWER_DOMAIN && IMX8MP > > diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile > > index 2daab73eb758..46849fd2a4db 100644 > > --- a/drivers/power/domain/Makefile > > +++ b/drivers/power/domain/Makefile > > @@ -8,6 +8,7 @@ obj-$(CONFIG_APPLE_PMGR_POWER_DOMAIN) += apple-pmgr.o > > obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o > > obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o > > obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o > > +obj-$(CONFIG_IMX8M_BLK_CTRL) += imx8m-blk-ctrl.o > > obj-$(CONFIG_IMX8MP_HSIOMIX_BLKCTRL) += imx8mp-hsiomix.o > > obj-$(CONFIG_MTK_POWER_DOMAIN) += mtk-power-domain.o > > obj-$(CONFIG_MESON_GX_VPU_POWER_DOMAIN) += meson-gx-pwrc-vpu.o > > diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c > > new file mode 100644 > > index 000000000000..4c89078b991b > > --- /dev/null > > +++ b/drivers/power/domain/imx8m-blk-ctrl.c > > @@ -0,0 +1,438 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright 2023 NXP > > + */ > > + > > +#include <dm.h> > > +#include <malloc.h> > > +#include <power-domain-uclass.h> > > +#include <asm/io.h> > > +#include <dm/device-internal.h> > > +#include <dm/device.h> > > +#include <dt-bindings/power/imx8mm-power.h> > > +#include <dt-bindings/power/imx8mn-power.h> > > +#include <dt-bindings/power/imx8mp-power.h> > > +#include <clk.h> > > +#include <linux/delay.h> > > + > > +#define BLK_SFT_RSTN 0x0 > > +#define BLK_CLK_EN 0x4 > > +#define BLK_MIPI_RESET_DIV 0x8 /* Mini/Nano/Plus DISPLAY_BLK_CTRL only */ > > + > > +#define DOMAIN_MAX_CLKS 4 > > + > > +struct imx8m_blk_ctrl_domain { > > + struct clk clks[DOMAIN_MAX_CLKS]; > > + struct power_domain power_dev; > > +}; > > + > > +struct imx8m_blk_ctrl { > > + void __iomem *base; > > + struct power_domain bus_power_dev; > > + struct imx8m_blk_ctrl_domain *domains; > > +}; > > + > > +struct imx8m_blk_ctrl_domain_data { > > + const char *name; > > + const char * const *clk_names; > > + const char *gpc_name; > > + int num_clks; > > + u32 rst_mask; > > + u32 clk_mask; > > + u32 mipi_phy_rst_mask; > > +}; > > + > > +struct imx8m_blk_ctrl_data { > > + int max_reg; > > + const struct imx8m_blk_ctrl_domain_data *domains; > > + int num_domains; > > + u32 bus_rst_mask; > > + u32 bus_clk_mask; > > +}; > > + > > +static int imx8m_blk_ctrl_request(struct power_domain *power_domain) > > +{ > > + return 0; > > +} > > + > > +static int imx8m_blk_ctrl_free(struct power_domain *power_domain) > > +{ > > + return 0; > > +} > > + > > +static int imx8m_blk_ctrl_enable_domain_clk(struct udevice *dev, ulong domain_id, bool enable) > > +{ > > + int ret, i; > > + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); > > + struct imx8m_blk_ctrl_data *drv_data = > > + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); > > + > > + debug("%s num_clk %u\n", __func__, drv_data->domains[domain_id].num_clks); > > + > > + for (i = 0; i < drv_data->domains[domain_id].num_clks; i++) { > > + debug("%s clk %s\n", __func__, drv_data->domains[domain_id].clk_names[i]); > > + if (enable) > > + ret = clk_enable(&priv->domains[domain_id].clks[i]); > > + else > > + ret = clk_disable(&priv->domains[domain_id].clks[i]); > > + if (ret && ret != -ENOENT) { > > + printf("Failed to %s domain clk %s\n", enable ? "enable" : "disable", > > + drv_data->domains[domain_id].clk_names[i]); > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) > > +{ > > + struct udevice *dev = power_domain->dev; > > + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); > > + struct imx8m_blk_ctrl_data *drv_data = > > + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); > > + int ret; > > + > > + debug("%s, id %lu\n", __func__, power_domain->id); > > + > > + if (!priv->domains[power_domain->id].power_dev.dev) > > + return -ENODEV; > > + > > + ret = power_domain_on(&priv->bus_power_dev); > > + if (ret < 0) { > > + printf("Failed to power up bus domain %d\n", ret); > > + return ret; > > + } > > + > > + /* Enable bus clock and deassert bus reset */ > > + setbits_le32(priv->base + BLK_CLK_EN, drv_data->bus_clk_mask); > > + setbits_le32(priv->base + BLK_SFT_RSTN, drv_data->bus_rst_mask); > > + > > + /* wait for reset to propagate */ > > + udelay(5); > > + > > + /* put devices into reset */ > > + clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); > > + if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) > > + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d > > + rv_data->domains[power_domain->id].mipi_phy_rst_mask); > > Does this build for you? > > I needed the following fix: > > diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c > index 4c89078b99..b772d50480 100644 > --- a/drivers/power/domain/imx8m-blk-ctrl.c > +++ b/drivers/power/domain/imx8m-blk-ctrl.c > @@ -114,8 +114,8 @@ static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) > /* put devices into reset */ > clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); > if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) > - clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d > - rv_data->domains[power_domain->id].mipi_phy_rst_mask); > + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, > + drv_data->domains[power_domain->id].mipi_phy_rst_mask); > > /* enable upstream and blk-ctrl clocks to allow reset to propagate */ > ret = imx8m_blk_ctrl_enable_domain_clk(dev, power_domain->id, true); > > to get it building. Yes, you're right. I made some changes suggested by patman, and before applying the patch, I didn't recompile, so I didn't notice. > > BTW: Just also working on bootlogo support for an imx8mp based board! > > I now applied your patches: > > clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present for gate and mux > clk: clk-uclass: Implement CLK_OPS_PARENT_ENABLE > > And added in my adapted /drivers/clk/imx/clk-imx8mp.c (imported from linux) > > clk_dm(IMX8MP_CLK_MEDIA_AXI, imx8m_clk_composite_flags("media_axi", imx8mp_media_axi_sels, > ARRAY_SIZE(imx8mp_media_axi_sels), base + 0x8a00, CLK_IS_CRITICAL)); > > instead of using imx8m_clk_composite, and dropped my approach to > get clocks up and working. > > Also dropped my similiar approach for mediablock and used your > > power: Add iMX8M block ctrl driver for dispmix > > And with this 3 patches, bootlogo works also/still fine for me! > > I did not applied/need your patches: > > clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate > clk: imx8mm: Prevent clock critical path from disabling during reparent and set_rate > clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled > clk: imx8mn: Mark IMX8MN_SYS_PLL2 and IMX8MN_SYS_PLL3 as enabled > > Of course, they are for imx8mm, but I mean I do not need similiar patches > for imx8mp! > > Also not applied (as for imx8mm) > clk: imx8mn: add video clocks support > > but as said, have similiar patch for clk-imx8mp.c > > May you check if using imx8m_clk_composite_flags() is working for you? I will do it > > I did not applied your patches 09/26 and the following patches from > your series, as I made a video bridge driver based on > > linux driver drivers/gpu/drm/bridge/fsl-ldb.c > > and a lcdif driver based on linux:/drivers/gpu/drm/mxsfb/lcdif_drv.c > Thank you for you feedback Heiko Regards, Dario > bye, > Heiko > -- > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
Hi Dario, On 30.09.24 15:20, Dario Binacchi wrote: > Hello Heiko, > > On Tue, Sep 24, 2024 at 11:07 AM Heiko Schocher <hs@denx.de> wrote: >> >> Hello Dario, >> >> On 13.09.24 11:55, Dario Binacchi wrote: >>> From: Michael Trimarchi <michael@amarulasolutions.com> >>> >>> Add iMX8 block ctrl driver for displaymix on iMX8MM/iMX8MN and >>> mediamix on iMX8MP. >>> >>> To support blk ctrl driver, the power domain driver on iMX8M needs >>> update to add relevant PGC domains >>> >>> Signed-off-by: Ye Li <ye.li@nxp.com> >>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> >>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> >>> --- >>> >>> drivers/power/domain/Kconfig | 6 + >>> drivers/power/domain/Makefile | 1 + >>> drivers/power/domain/imx8m-blk-ctrl.c | 438 ++++++++++++++++++++++ >>> drivers/power/domain/imx8m-power-domain.c | 213 ++++++++++- >>> 4 files changed, 656 insertions(+), 2 deletions(-) >>> create mode 100644 drivers/power/domain/imx8m-blk-ctrl.c >>> >>> diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig >>> index bd82d2f7044b..fb006b6e8e28 100644 >>> --- a/drivers/power/domain/Kconfig >>> +++ b/drivers/power/domain/Kconfig >>> @@ -40,6 +40,12 @@ config IMX8M_POWER_DOMAIN >>> Enable support for manipulating NXP i.MX8M on-SoC power domains via >>> requests to the ATF. >>> >>> +config IMX8M_BLK_CTRL >>> + bool "Enable i.MX8M block control driver" >>> + depends on POWER_DOMAIN && ARCH_IMX8M >>> + help >>> + Enable support for manipulating NXP i.MX8M on-SoC block control driver >>> + >>> config IMX8MP_HSIOMIX_BLKCTRL >>> bool "Enable i.MX8MP HSIOMIX domain driver" >>> depends on POWER_DOMAIN && IMX8MP >>> diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile >>> index 2daab73eb758..46849fd2a4db 100644 >>> --- a/drivers/power/domain/Makefile >>> +++ b/drivers/power/domain/Makefile >>> @@ -8,6 +8,7 @@ obj-$(CONFIG_APPLE_PMGR_POWER_DOMAIN) += apple-pmgr.o >>> obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o >>> obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o >>> obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o >>> +obj-$(CONFIG_IMX8M_BLK_CTRL) += imx8m-blk-ctrl.o >>> obj-$(CONFIG_IMX8MP_HSIOMIX_BLKCTRL) += imx8mp-hsiomix.o >>> obj-$(CONFIG_MTK_POWER_DOMAIN) += mtk-power-domain.o >>> obj-$(CONFIG_MESON_GX_VPU_POWER_DOMAIN) += meson-gx-pwrc-vpu.o >>> diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c >>> new file mode 100644 >>> index 000000000000..4c89078b991b >>> --- /dev/null >>> +++ b/drivers/power/domain/imx8m-blk-ctrl.c >>> @@ -0,0 +1,438 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Copyright 2023 NXP >>> + */ >>> + >>> +#include <dm.h> >>> +#include <malloc.h> >>> +#include <power-domain-uclass.h> >>> +#include <asm/io.h> >>> +#include <dm/device-internal.h> >>> +#include <dm/device.h> >>> +#include <dt-bindings/power/imx8mm-power.h> >>> +#include <dt-bindings/power/imx8mn-power.h> >>> +#include <dt-bindings/power/imx8mp-power.h> >>> +#include <clk.h> >>> +#include <linux/delay.h> >>> + >>> +#define BLK_SFT_RSTN 0x0 >>> +#define BLK_CLK_EN 0x4 >>> +#define BLK_MIPI_RESET_DIV 0x8 /* Mini/Nano/Plus DISPLAY_BLK_CTRL only */ >>> + >>> +#define DOMAIN_MAX_CLKS 4 >>> + >>> +struct imx8m_blk_ctrl_domain { >>> + struct clk clks[DOMAIN_MAX_CLKS]; >>> + struct power_domain power_dev; >>> +}; >>> + >>> +struct imx8m_blk_ctrl { >>> + void __iomem *base; >>> + struct power_domain bus_power_dev; >>> + struct imx8m_blk_ctrl_domain *domains; >>> +}; >>> + >>> +struct imx8m_blk_ctrl_domain_data { >>> + const char *name; >>> + const char * const *clk_names; >>> + const char *gpc_name; >>> + int num_clks; >>> + u32 rst_mask; >>> + u32 clk_mask; >>> + u32 mipi_phy_rst_mask; >>> +}; >>> + >>> +struct imx8m_blk_ctrl_data { >>> + int max_reg; >>> + const struct imx8m_blk_ctrl_domain_data *domains; >>> + int num_domains; >>> + u32 bus_rst_mask; >>> + u32 bus_clk_mask; >>> +}; >>> + >>> +static int imx8m_blk_ctrl_request(struct power_domain *power_domain) >>> +{ >>> + return 0; >>> +} >>> + >>> +static int imx8m_blk_ctrl_free(struct power_domain *power_domain) >>> +{ >>> + return 0; >>> +} >>> + >>> +static int imx8m_blk_ctrl_enable_domain_clk(struct udevice *dev, ulong domain_id, bool enable) >>> +{ >>> + int ret, i; >>> + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); >>> + struct imx8m_blk_ctrl_data *drv_data = >>> + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); >>> + >>> + debug("%s num_clk %u\n", __func__, drv_data->domains[domain_id].num_clks); >>> + >>> + for (i = 0; i < drv_data->domains[domain_id].num_clks; i++) { >>> + debug("%s clk %s\n", __func__, drv_data->domains[domain_id].clk_names[i]); >>> + if (enable) >>> + ret = clk_enable(&priv->domains[domain_id].clks[i]); >>> + else >>> + ret = clk_disable(&priv->domains[domain_id].clks[i]); >>> + if (ret && ret != -ENOENT) { >>> + printf("Failed to %s domain clk %s\n", enable ? "enable" : "disable", >>> + drv_data->domains[domain_id].clk_names[i]); >>> + return ret; >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) >>> +{ >>> + struct udevice *dev = power_domain->dev; >>> + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); >>> + struct imx8m_blk_ctrl_data *drv_data = >>> + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); >>> + int ret; >>> + >>> + debug("%s, id %lu\n", __func__, power_domain->id); >>> + >>> + if (!priv->domains[power_domain->id].power_dev.dev) >>> + return -ENODEV; >>> + >>> + ret = power_domain_on(&priv->bus_power_dev); >>> + if (ret < 0) { >>> + printf("Failed to power up bus domain %d\n", ret); >>> + return ret; >>> + } >>> + >>> + /* Enable bus clock and deassert bus reset */ >>> + setbits_le32(priv->base + BLK_CLK_EN, drv_data->bus_clk_mask); >>> + setbits_le32(priv->base + BLK_SFT_RSTN, drv_data->bus_rst_mask); >>> + >>> + /* wait for reset to propagate */ >>> + udelay(5); >>> + >>> + /* put devices into reset */ >>> + clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); >>> + if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) >>> + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d >>> + rv_data->domains[power_domain->id].mipi_phy_rst_mask); >> >> Does this build for you? >> >> I needed the following fix: >> >> diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c >> index 4c89078b99..b772d50480 100644 >> --- a/drivers/power/domain/imx8m-blk-ctrl.c >> +++ b/drivers/power/domain/imx8m-blk-ctrl.c >> @@ -114,8 +114,8 @@ static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) >> /* put devices into reset */ >> clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); >> if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) >> - clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d >> - rv_data->domains[power_domain->id].mipi_phy_rst_mask); >> + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, >> + drv_data->domains[power_domain->id].mipi_phy_rst_mask); >> >> /* enable upstream and blk-ctrl clocks to allow reset to propagate */ >> ret = imx8m_blk_ctrl_enable_domain_clk(dev, power_domain->id, true); >> >> to get it building. > > Yes, you're right. I made some changes suggested by patman, and before > applying the > patch, I didn't recompile, so I didn't notice. > >> >> BTW: Just also working on bootlogo support for an imx8mp based board! >> >> I now applied your patches: >> >> clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present for gate and mux >> clk: clk-uclass: Implement CLK_OPS_PARENT_ENABLE >> >> And added in my adapted /drivers/clk/imx/clk-imx8mp.c (imported from linux) >> >> clk_dm(IMX8MP_CLK_MEDIA_AXI, imx8m_clk_composite_flags("media_axi", imx8mp_media_axi_sels, >> ARRAY_SIZE(imx8mp_media_axi_sels), base + 0x8a00, CLK_IS_CRITICAL)); >> >> instead of using imx8m_clk_composite, and dropped my approach to >> get clocks up and working. >> >> Also dropped my similiar approach for mediablock and used your >> >> power: Add iMX8M block ctrl driver for dispmix >> >> And with this 3 patches, bootlogo works also/still fine for me! >> >> I did not applied/need your patches: >> >> clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate >> clk: imx8mm: Prevent clock critical path from disabling during reparent and set_rate >> clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled >> clk: imx8mn: Mark IMX8MN_SYS_PLL2 and IMX8MN_SYS_PLL3 as enabled >> >> Of course, they are for imx8mm, but I mean I do not need similiar patches >> for imx8mp! >> >> Also not applied (as for imx8mm) >> clk: imx8mn: add video clocks support >> >> but as said, have similiar patch for clk-imx8mp.c >> >> May you check if using imx8m_clk_composite_flags() is working for you? > > I will do it > >> >> I did not applied your patches 09/26 and the following patches from >> your series, as I made a video bridge driver based on >> >> linux driver drivers/gpu/drm/bridge/fsl-ldb.c >> >> and a lcdif driver based on linux:/drivers/gpu/drm/mxsfb/lcdif_drv.c >> > > Thank you for you feedback Heiko You are welcome! Hmm.. unfortunately ... I had applied your 2 clock patches, which fixed a problem with enabling parent clocks ... but they broke booting on a carrier which has fec ethernet! After "Net: " output the board hang... I reverted your 2 clock patches and it bootet again ... so there is a problem ... try to get some more time to look into... Do you have fec ethernet on your board? Thanks! bye, Heiko > > Regards, > Dario > >> bye, >> Heiko >> -- >> DENX Software Engineering GmbH, Managing Director: Erika Unter >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de > > >
Hi Heko On Tue, Oct 1, 2024 at 6:23 AM Heiko Schocher <hs@denx.de> wrote: > > Hi Dario, > > On 30.09.24 15:20, Dario Binacchi wrote: > > Hello Heiko, > > > > On Tue, Sep 24, 2024 at 11:07 AM Heiko Schocher <hs@denx.de> wrote: > >> > >> Hello Dario, > >> > >> On 13.09.24 11:55, Dario Binacchi wrote: > >>> From: Michael Trimarchi <michael@amarulasolutions.com> > >>> > >>> Add iMX8 block ctrl driver for displaymix on iMX8MM/iMX8MN and > >>> mediamix on iMX8MP. > >>> > >>> To support blk ctrl driver, the power domain driver on iMX8M needs > >>> update to add relevant PGC domains > >>> > >>> Signed-off-by: Ye Li <ye.li@nxp.com> > >>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > >>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > >>> --- > >>> > >>> drivers/power/domain/Kconfig | 6 + > >>> drivers/power/domain/Makefile | 1 + > >>> drivers/power/domain/imx8m-blk-ctrl.c | 438 ++++++++++++++++++++++ > >>> drivers/power/domain/imx8m-power-domain.c | 213 ++++++++++- > >>> 4 files changed, 656 insertions(+), 2 deletions(-) > >>> create mode 100644 drivers/power/domain/imx8m-blk-ctrl.c > >>> > >>> diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig > >>> index bd82d2f7044b..fb006b6e8e28 100644 > >>> --- a/drivers/power/domain/Kconfig > >>> +++ b/drivers/power/domain/Kconfig > >>> @@ -40,6 +40,12 @@ config IMX8M_POWER_DOMAIN > >>> Enable support for manipulating NXP i.MX8M on-SoC power domains via > >>> requests to the ATF. > >>> > >>> +config IMX8M_BLK_CTRL > >>> + bool "Enable i.MX8M block control driver" > >>> + depends on POWER_DOMAIN && ARCH_IMX8M > >>> + help > >>> + Enable support for manipulating NXP i.MX8M on-SoC block control driver > >>> + > >>> config IMX8MP_HSIOMIX_BLKCTRL > >>> bool "Enable i.MX8MP HSIOMIX domain driver" > >>> depends on POWER_DOMAIN && IMX8MP > >>> diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile > >>> index 2daab73eb758..46849fd2a4db 100644 > >>> --- a/drivers/power/domain/Makefile > >>> +++ b/drivers/power/domain/Makefile > >>> @@ -8,6 +8,7 @@ obj-$(CONFIG_APPLE_PMGR_POWER_DOMAIN) += apple-pmgr.o > >>> obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o > >>> obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o > >>> obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o > >>> +obj-$(CONFIG_IMX8M_BLK_CTRL) += imx8m-blk-ctrl.o > >>> obj-$(CONFIG_IMX8MP_HSIOMIX_BLKCTRL) += imx8mp-hsiomix.o > >>> obj-$(CONFIG_MTK_POWER_DOMAIN) += mtk-power-domain.o > >>> obj-$(CONFIG_MESON_GX_VPU_POWER_DOMAIN) += meson-gx-pwrc-vpu.o > >>> diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c > >>> new file mode 100644 > >>> index 000000000000..4c89078b991b > >>> --- /dev/null > >>> +++ b/drivers/power/domain/imx8m-blk-ctrl.c > >>> @@ -0,0 +1,438 @@ > >>> +// SPDX-License-Identifier: GPL-2.0 > >>> +/* > >>> + * Copyright 2023 NXP > >>> + */ > >>> + > >>> +#include <dm.h> > >>> +#include <malloc.h> > >>> +#include <power-domain-uclass.h> > >>> +#include <asm/io.h> > >>> +#include <dm/device-internal.h> > >>> +#include <dm/device.h> > >>> +#include <dt-bindings/power/imx8mm-power.h> > >>> +#include <dt-bindings/power/imx8mn-power.h> > >>> +#include <dt-bindings/power/imx8mp-power.h> > >>> +#include <clk.h> > >>> +#include <linux/delay.h> > >>> + > >>> +#define BLK_SFT_RSTN 0x0 > >>> +#define BLK_CLK_EN 0x4 > >>> +#define BLK_MIPI_RESET_DIV 0x8 /* Mini/Nano/Plus DISPLAY_BLK_CTRL only */ > >>> + > >>> +#define DOMAIN_MAX_CLKS 4 > >>> + > >>> +struct imx8m_blk_ctrl_domain { > >>> + struct clk clks[DOMAIN_MAX_CLKS]; > >>> + struct power_domain power_dev; > >>> +}; > >>> + > >>> +struct imx8m_blk_ctrl { > >>> + void __iomem *base; > >>> + struct power_domain bus_power_dev; > >>> + struct imx8m_blk_ctrl_domain *domains; > >>> +}; > >>> + > >>> +struct imx8m_blk_ctrl_domain_data { > >>> + const char *name; > >>> + const char * const *clk_names; > >>> + const char *gpc_name; > >>> + int num_clks; > >>> + u32 rst_mask; > >>> + u32 clk_mask; > >>> + u32 mipi_phy_rst_mask; > >>> +}; > >>> + > >>> +struct imx8m_blk_ctrl_data { > >>> + int max_reg; > >>> + const struct imx8m_blk_ctrl_domain_data *domains; > >>> + int num_domains; > >>> + u32 bus_rst_mask; > >>> + u32 bus_clk_mask; > >>> +}; > >>> + > >>> +static int imx8m_blk_ctrl_request(struct power_domain *power_domain) > >>> +{ > >>> + return 0; > >>> +} > >>> + > >>> +static int imx8m_blk_ctrl_free(struct power_domain *power_domain) > >>> +{ > >>> + return 0; > >>> +} > >>> + > >>> +static int imx8m_blk_ctrl_enable_domain_clk(struct udevice *dev, ulong domain_id, bool enable) > >>> +{ > >>> + int ret, i; > >>> + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); > >>> + struct imx8m_blk_ctrl_data *drv_data = > >>> + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); > >>> + > >>> + debug("%s num_clk %u\n", __func__, drv_data->domains[domain_id].num_clks); > >>> + > >>> + for (i = 0; i < drv_data->domains[domain_id].num_clks; i++) { > >>> + debug("%s clk %s\n", __func__, drv_data->domains[domain_id].clk_names[i]); > >>> + if (enable) > >>> + ret = clk_enable(&priv->domains[domain_id].clks[i]); > >>> + else > >>> + ret = clk_disable(&priv->domains[domain_id].clks[i]); > >>> + if (ret && ret != -ENOENT) { > >>> + printf("Failed to %s domain clk %s\n", enable ? "enable" : "disable", > >>> + drv_data->domains[domain_id].clk_names[i]); > >>> + return ret; > >>> + } > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) > >>> +{ > >>> + struct udevice *dev = power_domain->dev; > >>> + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); > >>> + struct imx8m_blk_ctrl_data *drv_data = > >>> + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); > >>> + int ret; > >>> + > >>> + debug("%s, id %lu\n", __func__, power_domain->id); > >>> + > >>> + if (!priv->domains[power_domain->id].power_dev.dev) > >>> + return -ENODEV; > >>> + > >>> + ret = power_domain_on(&priv->bus_power_dev); > >>> + if (ret < 0) { > >>> + printf("Failed to power up bus domain %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + /* Enable bus clock and deassert bus reset */ > >>> + setbits_le32(priv->base + BLK_CLK_EN, drv_data->bus_clk_mask); > >>> + setbits_le32(priv->base + BLK_SFT_RSTN, drv_data->bus_rst_mask); > >>> + > >>> + /* wait for reset to propagate */ > >>> + udelay(5); > >>> + > >>> + /* put devices into reset */ > >>> + clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); > >>> + if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) > >>> + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d > >>> + rv_data->domains[power_domain->id].mipi_phy_rst_mask); > >> > >> Does this build for you? > >> > >> I needed the following fix: > >> > >> diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c > >> index 4c89078b99..b772d50480 100644 > >> --- a/drivers/power/domain/imx8m-blk-ctrl.c > >> +++ b/drivers/power/domain/imx8m-blk-ctrl.c > >> @@ -114,8 +114,8 @@ static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) > >> /* put devices into reset */ > >> clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); > >> if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) > >> - clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d > >> - rv_data->domains[power_domain->id].mipi_phy_rst_mask); > >> + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, > >> + drv_data->domains[power_domain->id].mipi_phy_rst_mask); > >> > >> /* enable upstream and blk-ctrl clocks to allow reset to propagate */ > >> ret = imx8m_blk_ctrl_enable_domain_clk(dev, power_domain->id, true); > >> > >> to get it building. > > > > Yes, you're right. I made some changes suggested by patman, and before > > applying the > > patch, I didn't recompile, so I didn't notice. > > > >> > >> BTW: Just also working on bootlogo support for an imx8mp based board! > >> > >> I now applied your patches: > >> > >> clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present for gate and mux > >> clk: clk-uclass: Implement CLK_OPS_PARENT_ENABLE > >> > >> And added in my adapted /drivers/clk/imx/clk-imx8mp.c (imported from linux) > >> > >> clk_dm(IMX8MP_CLK_MEDIA_AXI, imx8m_clk_composite_flags("media_axi", imx8mp_media_axi_sels, > >> ARRAY_SIZE(imx8mp_media_axi_sels), base + 0x8a00, CLK_IS_CRITICAL)); > >> > >> instead of using imx8m_clk_composite, and dropped my approach to > >> get clocks up and working. > >> > >> Also dropped my similiar approach for mediablock and used your > >> > >> power: Add iMX8M block ctrl driver for dispmix > >> > >> And with this 3 patches, bootlogo works also/still fine for me! > >> > >> I did not applied/need your patches: > >> > >> clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate > >> clk: imx8mm: Prevent clock critical path from disabling during reparent and set_rate > >> clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled > >> clk: imx8mn: Mark IMX8MN_SYS_PLL2 and IMX8MN_SYS_PLL3 as enabled > >> > >> Of course, they are for imx8mm, but I mean I do not need similiar patches > >> for imx8mp! > >> > >> Also not applied (as for imx8mm) > >> clk: imx8mn: add video clocks support > >> > >> but as said, have similiar patch for clk-imx8mp.c > >> > >> May you check if using imx8m_clk_composite_flags() is working for you? > > > > I will do it > > > >> > >> I did not applied your patches 09/26 and the following patches from > >> your series, as I made a video bridge driver based on > >> > >> linux driver drivers/gpu/drm/bridge/fsl-ldb.c > >> > >> and a lcdif driver based on linux:/drivers/gpu/drm/mxsfb/lcdif_drv.c > >> > > > > Thank you for you feedback Heiko > > You are welcome! > > Hmm.. unfortunately ... I had applied your 2 clock patches, which > fixed a problem with enabling parent clocks ... but they broke booting > on a carrier which has fec ethernet! After "Net: " output the board hang... > > I reverted your 2 clock patches and it bootet again ... so there is > a problem ... try to get some more time to look into... > We have a fec, but we had I think two patches more on it. I forget to answer Marek about them because I don't have my board now and because he is partially right (or maybe right). Anyway when we boot we could have and we have clocks that are enabled by bootrom or SPL that we need to declare as enable like PLL2/PLL3 those clocks are parts or could be part of reparent so, you need to have a reference counter on them that allow to not disable during the down chain disable and up chain enable. I think that what happens to your ethernet it's that you disable some clock that is critical to the board to survive. I had a patch merged by Tom that prints the clock name so if you enable the debug of the clock you will find that your board stops working during one of this reparinting. Can you check it? Michael > Do you have fec ethernet on your board? > > Thanks! > > bye, > Heiko > > > > Regards, > > Dario > > > >> bye, > >> Heiko > >> -- > >> DENX Software Engineering GmbH, Managing Director: Erika Unter > >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > >> Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de > > > > > > > > -- > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
Hi Micahel, On 01.10.24 07:01, Michael Nazzareno Trimarchi wrote: > Hi Heko > > On Tue, Oct 1, 2024 at 6:23 AM Heiko Schocher <hs@denx.de> wrote: >> >> Hi Dario, >> >> On 30.09.24 15:20, Dario Binacchi wrote: >>> Hello Heiko, >>> >>> On Tue, Sep 24, 2024 at 11:07 AM Heiko Schocher <hs@denx.de> wrote: >>>> >>>> Hello Dario, >>>> >>>> On 13.09.24 11:55, Dario Binacchi wrote: >>>>> From: Michael Trimarchi <michael@amarulasolutions.com> >>>>> >>>>> Add iMX8 block ctrl driver for displaymix on iMX8MM/iMX8MN and >>>>> mediamix on iMX8MP. >>>>> >>>>> To support blk ctrl driver, the power domain driver on iMX8M needs >>>>> update to add relevant PGC domains >>>>> >>>>> Signed-off-by: Ye Li <ye.li@nxp.com> >>>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> >>>>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> >>>>> --- >>>>> >>>>> drivers/power/domain/Kconfig | 6 + >>>>> drivers/power/domain/Makefile | 1 + >>>>> drivers/power/domain/imx8m-blk-ctrl.c | 438 ++++++++++++++++++++++ >>>>> drivers/power/domain/imx8m-power-domain.c | 213 ++++++++++- >>>>> 4 files changed, 656 insertions(+), 2 deletions(-) >>>>> create mode 100644 drivers/power/domain/imx8m-blk-ctrl.c >>>>> >>>>> diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig >>>>> index bd82d2f7044b..fb006b6e8e28 100644 >>>>> --- a/drivers/power/domain/Kconfig >>>>> +++ b/drivers/power/domain/Kconfig >>>>> @@ -40,6 +40,12 @@ config IMX8M_POWER_DOMAIN >>>>> Enable support for manipulating NXP i.MX8M on-SoC power domains via >>>>> requests to the ATF. >>>>> >>>>> +config IMX8M_BLK_CTRL >>>>> + bool "Enable i.MX8M block control driver" >>>>> + depends on POWER_DOMAIN && ARCH_IMX8M >>>>> + help >>>>> + Enable support for manipulating NXP i.MX8M on-SoC block control driver >>>>> + >>>>> config IMX8MP_HSIOMIX_BLKCTRL >>>>> bool "Enable i.MX8MP HSIOMIX domain driver" >>>>> depends on POWER_DOMAIN && IMX8MP >>>>> diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile >>>>> index 2daab73eb758..46849fd2a4db 100644 >>>>> --- a/drivers/power/domain/Makefile >>>>> +++ b/drivers/power/domain/Makefile >>>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_APPLE_PMGR_POWER_DOMAIN) += apple-pmgr.o >>>>> obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o >>>>> obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o >>>>> obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o >>>>> +obj-$(CONFIG_IMX8M_BLK_CTRL) += imx8m-blk-ctrl.o >>>>> obj-$(CONFIG_IMX8MP_HSIOMIX_BLKCTRL) += imx8mp-hsiomix.o >>>>> obj-$(CONFIG_MTK_POWER_DOMAIN) += mtk-power-domain.o >>>>> obj-$(CONFIG_MESON_GX_VPU_POWER_DOMAIN) += meson-gx-pwrc-vpu.o >>>>> diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c >>>>> new file mode 100644 >>>>> index 000000000000..4c89078b991b >>>>> --- /dev/null >>>>> +++ b/drivers/power/domain/imx8m-blk-ctrl.c >>>>> @@ -0,0 +1,438 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> +/* >>>>> + * Copyright 2023 NXP >>>>> + */ >>>>> + >>>>> +#include <dm.h> >>>>> +#include <malloc.h> >>>>> +#include <power-domain-uclass.h> >>>>> +#include <asm/io.h> >>>>> +#include <dm/device-internal.h> >>>>> +#include <dm/device.h> >>>>> +#include <dt-bindings/power/imx8mm-power.h> >>>>> +#include <dt-bindings/power/imx8mn-power.h> >>>>> +#include <dt-bindings/power/imx8mp-power.h> >>>>> +#include <clk.h> >>>>> +#include <linux/delay.h> >>>>> + >>>>> +#define BLK_SFT_RSTN 0x0 >>>>> +#define BLK_CLK_EN 0x4 >>>>> +#define BLK_MIPI_RESET_DIV 0x8 /* Mini/Nano/Plus DISPLAY_BLK_CTRL only */ >>>>> + >>>>> +#define DOMAIN_MAX_CLKS 4 >>>>> + >>>>> +struct imx8m_blk_ctrl_domain { >>>>> + struct clk clks[DOMAIN_MAX_CLKS]; >>>>> + struct power_domain power_dev; >>>>> +}; >>>>> + >>>>> +struct imx8m_blk_ctrl { >>>>> + void __iomem *base; >>>>> + struct power_domain bus_power_dev; >>>>> + struct imx8m_blk_ctrl_domain *domains; >>>>> +}; >>>>> + >>>>> +struct imx8m_blk_ctrl_domain_data { >>>>> + const char *name; >>>>> + const char * const *clk_names; >>>>> + const char *gpc_name; >>>>> + int num_clks; >>>>> + u32 rst_mask; >>>>> + u32 clk_mask; >>>>> + u32 mipi_phy_rst_mask; >>>>> +}; >>>>> + >>>>> +struct imx8m_blk_ctrl_data { >>>>> + int max_reg; >>>>> + const struct imx8m_blk_ctrl_domain_data *domains; >>>>> + int num_domains; >>>>> + u32 bus_rst_mask; >>>>> + u32 bus_clk_mask; >>>>> +}; >>>>> + >>>>> +static int imx8m_blk_ctrl_request(struct power_domain *power_domain) >>>>> +{ >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int imx8m_blk_ctrl_free(struct power_domain *power_domain) >>>>> +{ >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int imx8m_blk_ctrl_enable_domain_clk(struct udevice *dev, ulong domain_id, bool enable) >>>>> +{ >>>>> + int ret, i; >>>>> + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); >>>>> + struct imx8m_blk_ctrl_data *drv_data = >>>>> + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); >>>>> + >>>>> + debug("%s num_clk %u\n", __func__, drv_data->domains[domain_id].num_clks); >>>>> + >>>>> + for (i = 0; i < drv_data->domains[domain_id].num_clks; i++) { >>>>> + debug("%s clk %s\n", __func__, drv_data->domains[domain_id].clk_names[i]); >>>>> + if (enable) >>>>> + ret = clk_enable(&priv->domains[domain_id].clks[i]); >>>>> + else >>>>> + ret = clk_disable(&priv->domains[domain_id].clks[i]); >>>>> + if (ret && ret != -ENOENT) { >>>>> + printf("Failed to %s domain clk %s\n", enable ? "enable" : "disable", >>>>> + drv_data->domains[domain_id].clk_names[i]); >>>>> + return ret; >>>>> + } >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) >>>>> +{ >>>>> + struct udevice *dev = power_domain->dev; >>>>> + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); >>>>> + struct imx8m_blk_ctrl_data *drv_data = >>>>> + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); >>>>> + int ret; >>>>> + >>>>> + debug("%s, id %lu\n", __func__, power_domain->id); >>>>> + >>>>> + if (!priv->domains[power_domain->id].power_dev.dev) >>>>> + return -ENODEV; >>>>> + >>>>> + ret = power_domain_on(&priv->bus_power_dev); >>>>> + if (ret < 0) { >>>>> + printf("Failed to power up bus domain %d\n", ret); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + /* Enable bus clock and deassert bus reset */ >>>>> + setbits_le32(priv->base + BLK_CLK_EN, drv_data->bus_clk_mask); >>>>> + setbits_le32(priv->base + BLK_SFT_RSTN, drv_data->bus_rst_mask); >>>>> + >>>>> + /* wait for reset to propagate */ >>>>> + udelay(5); >>>>> + >>>>> + /* put devices into reset */ >>>>> + clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); >>>>> + if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) >>>>> + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d >>>>> + rv_data->domains[power_domain->id].mipi_phy_rst_mask); >>>> >>>> Does this build for you? >>>> >>>> I needed the following fix: >>>> >>>> diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c >>>> index 4c89078b99..b772d50480 100644 >>>> --- a/drivers/power/domain/imx8m-blk-ctrl.c >>>> +++ b/drivers/power/domain/imx8m-blk-ctrl.c >>>> @@ -114,8 +114,8 @@ static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) >>>> /* put devices into reset */ >>>> clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); >>>> if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) >>>> - clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d >>>> - rv_data->domains[power_domain->id].mipi_phy_rst_mask); >>>> + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, >>>> + drv_data->domains[power_domain->id].mipi_phy_rst_mask); >>>> >>>> /* enable upstream and blk-ctrl clocks to allow reset to propagate */ >>>> ret = imx8m_blk_ctrl_enable_domain_clk(dev, power_domain->id, true); >>>> >>>> to get it building. >>> >>> Yes, you're right. I made some changes suggested by patman, and before >>> applying the >>> patch, I didn't recompile, so I didn't notice. >>> >>>> >>>> BTW: Just also working on bootlogo support for an imx8mp based board! >>>> >>>> I now applied your patches: >>>> >>>> clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present for gate and mux >>>> clk: clk-uclass: Implement CLK_OPS_PARENT_ENABLE >>>> >>>> And added in my adapted /drivers/clk/imx/clk-imx8mp.c (imported from linux) >>>> >>>> clk_dm(IMX8MP_CLK_MEDIA_AXI, imx8m_clk_composite_flags("media_axi", imx8mp_media_axi_sels, >>>> ARRAY_SIZE(imx8mp_media_axi_sels), base + 0x8a00, CLK_IS_CRITICAL)); >>>> >>>> instead of using imx8m_clk_composite, and dropped my approach to >>>> get clocks up and working. >>>> >>>> Also dropped my similiar approach for mediablock and used your >>>> >>>> power: Add iMX8M block ctrl driver for dispmix >>>> >>>> And with this 3 patches, bootlogo works also/still fine for me! >>>> >>>> I did not applied/need your patches: >>>> >>>> clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate >>>> clk: imx8mm: Prevent clock critical path from disabling during reparent and set_rate >>>> clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled >>>> clk: imx8mn: Mark IMX8MN_SYS_PLL2 and IMX8MN_SYS_PLL3 as enabled >>>> >>>> Of course, they are for imx8mm, but I mean I do not need similiar patches >>>> for imx8mp! >>>> >>>> Also not applied (as for imx8mm) >>>> clk: imx8mn: add video clocks support >>>> >>>> but as said, have similiar patch for clk-imx8mp.c >>>> >>>> May you check if using imx8m_clk_composite_flags() is working for you? >>> >>> I will do it >>> >>>> >>>> I did not applied your patches 09/26 and the following patches from >>>> your series, as I made a video bridge driver based on >>>> >>>> linux driver drivers/gpu/drm/bridge/fsl-ldb.c >>>> >>>> and a lcdif driver based on linux:/drivers/gpu/drm/mxsfb/lcdif_drv.c >>>> >>> >>> Thank you for you feedback Heiko >> >> You are welcome! >> >> Hmm.. unfortunately ... I had applied your 2 clock patches, which >> fixed a problem with enabling parent clocks ... but they broke booting >> on a carrier which has fec ethernet! After "Net: " output the board hang... >> >> I reverted your 2 clock patches and it bootet again ... so there is >> a problem ... try to get some more time to look into... >> > > We have a fec, but we had I think two patches more on it. I forget to > answer Marek > about them because I don't have my board now and because he is > partially right (or maybe right). > Anyway when we boot we could have and we have clocks that are enabled > by bootrom or SPL that > we need to declare as enable like PLL2/PLL3 those clocks are parts or > could be part of reparent so, > you need to have a reference counter on them that allow to not disable > during the down chain disable > and up chain enable. I think that what happens to your ethernet it's > that you disable some clock that is > critical to the board to survive. I had a patch merged by Tom that Yep, thats what I think too! If you access registers in a block for which the clock is not enabled ... it just hang... > prints the clock name so if you enable > the debug of the clock you will find that your board stops working > during one of this reparinting. I currently work on 2024.07... will rebase if 2024.10 is out... Ah, you mean: commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08 Author: Michael Trimarchi <michael@amarulasolutions.com> Date: Tue Jul 9 08:28:13 2024 +0200 clk: clk-uclass: Print clk name in clk_enable/clk_disable Print clk name in clk_enable and clk_disable. Make sense to know what clock get disabled/enabled before a system crash or system hang. Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> I have the same change when I debug :-P IIRC I did not see the clock names ... but I will recheck! Thanks! > Can you check it? Yep, will do. bye, Heiko > > Michael > > >> Do you have fec ethernet on your board? >> >> Thanks! >> >> bye, >> Heiko >>> >>> Regards, >>> Dario >>> >>>> bye, >>>> Heiko >>>> -- >>>> DENX Software Engineering GmbH, Managing Director: Erika Unter >>>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >>>> Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de >>> >>> >>> >> >> -- >> DENX Software Engineering GmbH, Managing Director: Erika Unter >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de > > >
Hello Michael, On 01.10.24 07:14, Heiko Schocher wrote: > Hi Micahel, > > On 01.10.24 07:01, Michael Nazzareno Trimarchi wrote: >> Hi Heko >> >> On Tue, Oct 1, 2024 at 6:23 AM Heiko Schocher <hs@denx.de> wrote: >>> >>> Hi Dario, >>> >>> On 30.09.24 15:20, Dario Binacchi wrote: >>>> Hello Heiko, >>>> >>>> On Tue, Sep 24, 2024 at 11:07 AM Heiko Schocher <hs@denx.de> wrote: >>>>> >>>>> Hello Dario, >>>>> >>>>> On 13.09.24 11:55, Dario Binacchi wrote: >>>>>> From: Michael Trimarchi <michael@amarulasolutions.com> >>>>>> >>>>>> Add iMX8 block ctrl driver for displaymix on iMX8MM/iMX8MN and >>>>>> mediamix on iMX8MP. >>>>>> >>>>>> To support blk ctrl driver, the power domain driver on iMX8M needs >>>>>> update to add relevant PGC domains >>>>>> >>>>>> Signed-off-by: Ye Li <ye.li@nxp.com> >>>>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> >>>>>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> >>>>>> --- >>>>>> >>>>>> drivers/power/domain/Kconfig | 6 + >>>>>> drivers/power/domain/Makefile | 1 + >>>>>> drivers/power/domain/imx8m-blk-ctrl.c | 438 ++++++++++++++++++++++ >>>>>> drivers/power/domain/imx8m-power-domain.c | 213 ++++++++++- >>>>>> 4 files changed, 656 insertions(+), 2 deletions(-) >>>>>> create mode 100644 drivers/power/domain/imx8m-blk-ctrl.c >>>>>> >>>>>> diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig >>>>>> index bd82d2f7044b..fb006b6e8e28 100644 >>>>>> --- a/drivers/power/domain/Kconfig >>>>>> +++ b/drivers/power/domain/Kconfig >>>>>> @@ -40,6 +40,12 @@ config IMX8M_POWER_DOMAIN >>>>>> Enable support for manipulating NXP i.MX8M on-SoC power domains via >>>>>> requests to the ATF. >>>>>> >>>>>> +config IMX8M_BLK_CTRL >>>>>> + bool "Enable i.MX8M block control driver" >>>>>> + depends on POWER_DOMAIN && ARCH_IMX8M >>>>>> + help >>>>>> + Enable support for manipulating NXP i.MX8M on-SoC block control driver >>>>>> + >>>>>> config IMX8MP_HSIOMIX_BLKCTRL >>>>>> bool "Enable i.MX8MP HSIOMIX domain driver" >>>>>> depends on POWER_DOMAIN && IMX8MP >>>>>> diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile >>>>>> index 2daab73eb758..46849fd2a4db 100644 >>>>>> --- a/drivers/power/domain/Makefile >>>>>> +++ b/drivers/power/domain/Makefile >>>>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_APPLE_PMGR_POWER_DOMAIN) += apple-pmgr.o >>>>>> obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o >>>>>> obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o >>>>>> obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o >>>>>> +obj-$(CONFIG_IMX8M_BLK_CTRL) += imx8m-blk-ctrl.o >>>>>> obj-$(CONFIG_IMX8MP_HSIOMIX_BLKCTRL) += imx8mp-hsiomix.o >>>>>> obj-$(CONFIG_MTK_POWER_DOMAIN) += mtk-power-domain.o >>>>>> obj-$(CONFIG_MESON_GX_VPU_POWER_DOMAIN) += meson-gx-pwrc-vpu.o >>>>>> diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c >>>>>> new file mode 100644 >>>>>> index 000000000000..4c89078b991b >>>>>> --- /dev/null >>>>>> +++ b/drivers/power/domain/imx8m-blk-ctrl.c >>>>>> @@ -0,0 +1,438 @@ >>>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>>> +/* >>>>>> + * Copyright 2023 NXP >>>>>> + */ >>>>>> + >>>>>> +#include <dm.h> >>>>>> +#include <malloc.h> >>>>>> +#include <power-domain-uclass.h> >>>>>> +#include <asm/io.h> >>>>>> +#include <dm/device-internal.h> >>>>>> +#include <dm/device.h> >>>>>> +#include <dt-bindings/power/imx8mm-power.h> >>>>>> +#include <dt-bindings/power/imx8mn-power.h> >>>>>> +#include <dt-bindings/power/imx8mp-power.h> >>>>>> +#include <clk.h> >>>>>> +#include <linux/delay.h> >>>>>> + >>>>>> +#define BLK_SFT_RSTN 0x0 >>>>>> +#define BLK_CLK_EN 0x4 >>>>>> +#define BLK_MIPI_RESET_DIV 0x8 /* Mini/Nano/Plus DISPLAY_BLK_CTRL only */ >>>>>> + >>>>>> +#define DOMAIN_MAX_CLKS 4 >>>>>> + >>>>>> +struct imx8m_blk_ctrl_domain { >>>>>> + struct clk clks[DOMAIN_MAX_CLKS]; >>>>>> + struct power_domain power_dev; >>>>>> +}; >>>>>> + >>>>>> +struct imx8m_blk_ctrl { >>>>>> + void __iomem *base; >>>>>> + struct power_domain bus_power_dev; >>>>>> + struct imx8m_blk_ctrl_domain *domains; >>>>>> +}; >>>>>> + >>>>>> +struct imx8m_blk_ctrl_domain_data { >>>>>> + const char *name; >>>>>> + const char * const *clk_names; >>>>>> + const char *gpc_name; >>>>>> + int num_clks; >>>>>> + u32 rst_mask; >>>>>> + u32 clk_mask; >>>>>> + u32 mipi_phy_rst_mask; >>>>>> +}; >>>>>> + >>>>>> +struct imx8m_blk_ctrl_data { >>>>>> + int max_reg; >>>>>> + const struct imx8m_blk_ctrl_domain_data *domains; >>>>>> + int num_domains; >>>>>> + u32 bus_rst_mask; >>>>>> + u32 bus_clk_mask; >>>>>> +}; >>>>>> + >>>>>> +static int imx8m_blk_ctrl_request(struct power_domain *power_domain) >>>>>> +{ >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int imx8m_blk_ctrl_free(struct power_domain *power_domain) >>>>>> +{ >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int imx8m_blk_ctrl_enable_domain_clk(struct udevice *dev, ulong domain_id, bool enable) >>>>>> +{ >>>>>> + int ret, i; >>>>>> + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); >>>>>> + struct imx8m_blk_ctrl_data *drv_data = >>>>>> + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); >>>>>> + >>>>>> + debug("%s num_clk %u\n", __func__, drv_data->domains[domain_id].num_clks); >>>>>> + >>>>>> + for (i = 0; i < drv_data->domains[domain_id].num_clks; i++) { >>>>>> + debug("%s clk %s\n", __func__, drv_data->domains[domain_id].clk_names[i]); >>>>>> + if (enable) >>>>>> + ret = clk_enable(&priv->domains[domain_id].clks[i]); >>>>>> + else >>>>>> + ret = clk_disable(&priv->domains[domain_id].clks[i]); >>>>>> + if (ret && ret != -ENOENT) { >>>>>> + printf("Failed to %s domain clk %s\n", enable ? "enable" : "disable", >>>>>> + drv_data->domains[domain_id].clk_names[i]); >>>>>> + return ret; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) >>>>>> +{ >>>>>> + struct udevice *dev = power_domain->dev; >>>>>> + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); >>>>>> + struct imx8m_blk_ctrl_data *drv_data = >>>>>> + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); >>>>>> + int ret; >>>>>> + >>>>>> + debug("%s, id %lu\n", __func__, power_domain->id); >>>>>> + >>>>>> + if (!priv->domains[power_domain->id].power_dev.dev) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + ret = power_domain_on(&priv->bus_power_dev); >>>>>> + if (ret < 0) { >>>>>> + printf("Failed to power up bus domain %d\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + /* Enable bus clock and deassert bus reset */ >>>>>> + setbits_le32(priv->base + BLK_CLK_EN, drv_data->bus_clk_mask); >>>>>> + setbits_le32(priv->base + BLK_SFT_RSTN, drv_data->bus_rst_mask); >>>>>> + >>>>>> + /* wait for reset to propagate */ >>>>>> + udelay(5); >>>>>> + >>>>>> + /* put devices into reset */ >>>>>> + clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); >>>>>> + if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) >>>>>> + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d >>>>>> + rv_data->domains[power_domain->id].mipi_phy_rst_mask); >>>>> >>>>> Does this build for you? >>>>> >>>>> I needed the following fix: >>>>> >>>>> diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c >>>>> index 4c89078b99..b772d50480 100644 >>>>> --- a/drivers/power/domain/imx8m-blk-ctrl.c >>>>> +++ b/drivers/power/domain/imx8m-blk-ctrl.c >>>>> @@ -114,8 +114,8 @@ static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) >>>>> /* put devices into reset */ >>>>> clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); >>>>> if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) >>>>> - clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d >>>>> - rv_data->domains[power_domain->id].mipi_phy_rst_mask); >>>>> + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, >>>>> + drv_data->domains[power_domain->id].mipi_phy_rst_mask); >>>>> >>>>> /* enable upstream and blk-ctrl clocks to allow reset to propagate */ >>>>> ret = imx8m_blk_ctrl_enable_domain_clk(dev, power_domain->id, true); >>>>> >>>>> to get it building. >>>> >>>> Yes, you're right. I made some changes suggested by patman, and before >>>> applying the >>>> patch, I didn't recompile, so I didn't notice. >>>> >>>>> >>>>> BTW: Just also working on bootlogo support for an imx8mp based board! >>>>> >>>>> I now applied your patches: >>>>> >>>>> clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present for gate and mux >>>>> clk: clk-uclass: Implement CLK_OPS_PARENT_ENABLE >>>>> >>>>> And added in my adapted /drivers/clk/imx/clk-imx8mp.c (imported from linux) >>>>> >>>>> clk_dm(IMX8MP_CLK_MEDIA_AXI, imx8m_clk_composite_flags("media_axi", imx8mp_media_axi_sels, >>>>> ARRAY_SIZE(imx8mp_media_axi_sels), base + 0x8a00, CLK_IS_CRITICAL)); >>>>> >>>>> instead of using imx8m_clk_composite, and dropped my approach to >>>>> get clocks up and working. >>>>> >>>>> Also dropped my similiar approach for mediablock and used your >>>>> >>>>> power: Add iMX8M block ctrl driver for dispmix >>>>> >>>>> And with this 3 patches, bootlogo works also/still fine for me! >>>>> >>>>> I did not applied/need your patches: >>>>> >>>>> clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate >>>>> clk: imx8mm: Prevent clock critical path from disabling during reparent and set_rate >>>>> clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled >>>>> clk: imx8mn: Mark IMX8MN_SYS_PLL2 and IMX8MN_SYS_PLL3 as enabled >>>>> >>>>> Of course, they are for imx8mm, but I mean I do not need similiar patches >>>>> for imx8mp! >>>>> >>>>> Also not applied (as for imx8mm) >>>>> clk: imx8mn: add video clocks support >>>>> >>>>> but as said, have similiar patch for clk-imx8mp.c >>>>> >>>>> May you check if using imx8m_clk_composite_flags() is working for you? >>>> >>>> I will do it >>>> >>>>> >>>>> I did not applied your patches 09/26 and the following patches from >>>>> your series, as I made a video bridge driver based on >>>>> >>>>> linux driver drivers/gpu/drm/bridge/fsl-ldb.c >>>>> >>>>> and a lcdif driver based on linux:/drivers/gpu/drm/mxsfb/lcdif_drv.c >>>>> >>>> >>>> Thank you for you feedback Heiko >>> >>> You are welcome! >>> >>> Hmm.. unfortunately ... I had applied your 2 clock patches, which >>> fixed a problem with enabling parent clocks ... but they broke booting >>> on a carrier which has fec ethernet! After "Net: " output the board hang... >>> >>> I reverted your 2 clock patches and it bootet again ... so there is >>> a problem ... try to get some more time to look into... >>> >> >> We have a fec, but we had I think two patches more on it. I forget to >> answer Marek >> about them because I don't have my board now and because he is >> partially right (or maybe right). >> Anyway when we boot we could have and we have clocks that are enabled >> by bootrom or SPL that >> we need to declare as enable like PLL2/PLL3 those clocks are parts or >> could be part of reparent so, >> you need to have a reference counter on them that allow to not disable >> during the down chain disable >> and up chain enable. I think that what happens to your ethernet it's >> that you disable some clock that is >> critical to the board to survive. I had a patch merged by Tom that > > Yep, thats what I think too! If you access registers in a block for > which the clock is not enabled ... it just hang... > >> prints the clock name so if you enable >> the debug of the clock you will find that your board stops working >> during one of this reparinting. > > I currently work on 2024.07... will rebase if 2024.10 is out... > > Ah, you mean: > > commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08 > Author: Michael Trimarchi <michael@amarulasolutions.com> > Date: Tue Jul 9 08:28:13 2024 +0200 > > clk: clk-uclass: Print clk name in clk_enable/clk_disable > > Print clk name in clk_enable and clk_disable. Make sense to know > what clock get disabled/enabled before a system crash or system > hang. > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > > I have the same change when I debug :-P > > IIRC I did not see the clock names ... but I will recheck! I see with your patch the clock names, so fine... and see [1] Hmm... I am on imx8mp, and I think the changes the patchset do in "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate" are in clk-imx8mp already ... Ported the patch from patchset "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" to imx8mp [2] and fec ethernet works again for me on imx8mp! Could you add this if you post a v2 ? bye, Heiko [1] │ │ <> Net: clk_set_defaults(gpio@30230000) │ │ <> clk_set_default_parents: could not read assigned-clock-parents for 00000000fee74d90 name=gpio@30230000 │ │ <> clk_set_defaults(aristainetos3-fec-rgmii-grp) │ │ <> clk_set_default_parents: could not read assigned-clock-parents for 00000000fee75780 name=aristainetos3-fec-rgmii-grp │ │ <> clk_set_defaults(ethernet@30be0000) │ │ <> clk_get_by_indexed_prop(dev=00000000fee7ad20, index=0, clk=00000000fee5e3b8) │ │ <> clk_of_xlate_default(clk=00000000fee5e3b8) │ │ <> clk_request(dev=00000000fee77480, clk=00000000fee5e3b8) │ │ <> clk_get_by_indexed_prop(dev=00000000fee7ad20, index=0, clk=00000000fee5e390) │ │ <> clk_of_xlate_default(clk=00000000fee5e390) │ │ <> clk_request(dev=00000000fee77480, clk=00000000fee5e390) │ │ <> clk_set_parent(clk=00000000fee825c0, parent=00000000fee80680) │ │ <> clk_get_parent(clk=00000000fee825c0) │ │ <> clk_enable(clk=00000000fee743d0 name=clock-osc-24m) │ │ <> clk_enable(clk=00000000fee80680 name=sys_pll1_266m) │ │ <> clk_enable(clk=00000000fee7fb40 name=sys_pll1_out) │ │ <> clk_disable(clk=00000000fee80680 name=sys_pll1_266m) │ │ <> clk_disable(clk=00000000fee7fb40 name=sys_pll1_out) │ │ <> clk_disable(clk=00000000fee743d0 name=clock-osc-24m) │ │ <> clk_get_by_indexed_prop(dev=00000000fee7ad20, index=1, clk=00000000fee5e3b8) │ │ <> clk_of_xlate_default(clk=00000000fee5e3b8) │ │ <> clk_request(dev=00000000fee77480, clk=00000000fee5e3b8) │ │ <> clk_get_by_indexed_prop(dev=00000000fee7ad20, index=1, clk=00000000fee5e390) │ │ <> clk_of_xlate_default(clk=00000000fee5e390) │ │ <> clk_request(dev=00000000fee77480, clk=00000000fee5e390) │ │ <> clk_set_parent(clk=00000000fee85b80, parent=00000000fee80b80) │ │ <> clk_get_parent(clk=00000000fee85b80) │ │ <> clk_enable(clk=00000000fee743d0 name=clock-osc-24m) │ │ <> clk_enable(clk=00000000fee80b80 name=sys_pll2_100m) │ │ <> clk_enable(clk=00000000fee7fc80 name=sys_pll2_out) │ │ <> clk_enable(clk=00000000fee7f480 name=sys_pll2_bypass) │ │ <> clk_enable(clk=00000000fee7eb80 name=sys_pll2) │ │ <> clk_enable(clk=00000000fee7e280 name=sys_pll2_ref_sel) │ │ <> clk_enable(clk=00000000fee743d0 name=clock-osc-24m) │ │ <> clk_disable(clk=00000000fee80b80 name=sys_pll2_100m) │ │ <> clk_disable(clk=00000000fee7fc80 name=sys_pll2_out) │ │ <> clk_disable(clk=00000000fee7f480 name=sys_pll2_bypass) │ │ <> clk_disable(clk=00000000fee7eb80 name=sys_pll2) [2] diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c index d6865509cb..f6fa66094d 100644 --- a/drivers/clk/imx/clk-imx8mp.c +++ b/drivers/clk/imx/clk-imx8mp.c @@ -235,6 +235,7 @@ static const char * const imx8mp_clkout_sels[] = {"audio_pll1_out", "audio_pll2_ static int imx8mp_clk_probe(struct udevice *dev) { struct clk osc_24m_clk, osc_32k_clk; + struct clk *clk; void __iomem *base; int ret; @@ -303,6 +304,14 @@ static int imx8mp_clk_probe(struct udevice *dev) return ret; clk_dm(IMX8MP_CLK_32K, dev_get_clk_ptr(osc_32k_clk.dev)); + ret = clk_get_by_id(IMX8MP_SYS_PLL2, &clk); + if (!ret) + clk_enable(clk); + + ret = clk_get_by_id(IMX8MP_SYS_PLL3, &clk); + if (!ret) + clk_enable(clk); + base = dev_read_addr_ptr(dev); if (!base) return -EINVAL;
Hi Heiko, > >>> Hmm.. unfortunately ... I had applied your 2 clock patches, which > >>> fixed a problem with enabling parent clocks ... but they broke booting > >>> on a carrier which has fec ethernet! After "Net: " output the board hang... > >>> > >>> I reverted your 2 clock patches and it bootet again ... so there is > >>> a problem ... try to get some more time to look into... > >>> > >> > >> We have a fec, but we had I think two patches more on it. I forget to > >> answer Marek > >> about them because I don't have my board now and because he is > >> partially right (or maybe right). > >> Anyway when we boot we could have and we have clocks that are enabled > >> by bootrom or SPL that > >> we need to declare as enable like PLL2/PLL3 those clocks are parts or > >> could be part of reparent so, > >> you need to have a reference counter on them that allow to not disable > >> during the down chain disable > >> and up chain enable. I think that what happens to your ethernet it's > >> that you disable some clock that is > >> critical to the board to survive. I had a patch merged by Tom that > > > > Yep, thats what I think too! If you access registers in a block for > > which the clock is not enabled ... it just hang... > > > >> prints the clock name so if you enable > >> the debug of the clock you will find that your board stops working > >> during one of this reparinting. > > > > I currently work on 2024.07... will rebase if 2024.10 is out... > > > > Ah, you mean: > > > > commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08 > > Author: Michael Trimarchi <michael@amarulasolutions.com> > > Date: Tue Jul 9 08:28:13 2024 +0200 > > > > clk: clk-uclass: Print clk name in clk_enable/clk_disable > > > > Print clk name in clk_enable and clk_disable. Make sense to know > > what clock get disabled/enabled before a system crash or system > > hang. > > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > > > > I have the same change when I debug :-P > > > > IIRC I did not see the clock names ... but I will recheck! > > I see with your patch the clock names, so fine... and see [1] > > Hmm... I am on imx8mp, and I think the changes the patchset do in > > "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate" > > are in clk-imx8mp already ... > > Ported the patch from patchset > > "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" > > to imx8mp [2] and fec ethernet works again for me on imx8mp! > > Could you add this if you post a v2 ? TBH I don't feel like the below change is the correct one, it is too specific. The clock core is recursive and thus should handle the reparenting situations gracefully. I posted a series that is targeting the LVDS output on imx8mp. You should probably consider checking these patches as well if you work on imx8mp as well. I also had similar breakages with Ethernet which happened during the assigned-clocks handling. This patch, which is more future and platform agnostic, fixed it: https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ My series is complimentary, even though there are some overlaps that we need to merge. Thanks, Miquèl To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Hi Miguel On Tue, Oct 1, 2024 at 10:29 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Heiko, > > > > >>> Hmm.. unfortunately ... I had applied your 2 clock patches, which > > >>> fixed a problem with enabling parent clocks ... but they broke booting > > >>> on a carrier which has fec ethernet! After "Net: " output the board hang... > > >>> > > >>> I reverted your 2 clock patches and it bootet again ... so there is > > >>> a problem ... try to get some more time to look into... > > >>> > > >> > > >> We have a fec, but we had I think two patches more on it. I forget to > > >> answer Marek > > >> about them because I don't have my board now and because he is > > >> partially right (or maybe right). > > >> Anyway when we boot we could have and we have clocks that are enabled > > >> by bootrom or SPL that > > >> we need to declare as enable like PLL2/PLL3 those clocks are parts or > > >> could be part of reparent so, > > >> you need to have a reference counter on them that allow to not disable > > >> during the down chain disable > > >> and up chain enable. I think that what happens to your ethernet it's > > >> that you disable some clock that is > > >> critical to the board to survive. I had a patch merged by Tom that > > > > > > Yep, thats what I think too! If you access registers in a block for > > > which the clock is not enabled ... it just hang... > > > > > >> prints the clock name so if you enable > > >> the debug of the clock you will find that your board stops working > > >> during one of this reparinting. > > > > > > I currently work on 2024.07... will rebase if 2024.10 is out... > > > > > > Ah, you mean: > > > > > > commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08 > > > Author: Michael Trimarchi <michael@amarulasolutions.com> > > > Date: Tue Jul 9 08:28:13 2024 +0200 > > > > > > clk: clk-uclass: Print clk name in clk_enable/clk_disable > > > > > > Print clk name in clk_enable and clk_disable. Make sense to know > > > what clock get disabled/enabled before a system crash or system > > > hang. > > > > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > > > > > > I have the same change when I debug :-P > > > > > > IIRC I did not see the clock names ... but I will recheck! > > > > I see with your patch the clock names, so fine... and see [1] > > > > Hmm... I am on imx8mp, and I think the changes the patchset do in > > > > "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate" > > > > are in clk-imx8mp already ... > > > > Ported the patch from patchset > > > > "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" > > > > to imx8mp [2] and fec ethernet works again for me on imx8mp! > > > > Could you add this if you post a v2 ? > > TBH I don't feel like the below change is the correct one, it is too > specific. The clock core is recursive and thus should handle the > reparenting situations gracefully. > > I posted a series that is targeting the LVDS output on imx8mp. You > should probably consider checking these patches as well if you work > on imx8mp as well. I also had similar breakages with Ethernet which > happened during the assigned-clocks handling. This patch, which is more > future and platform agnostic, fixed it: > > https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ > The clock patches are not specific at all. You need to have it working for the parent for each component. This is a standard way to do it and nothing magic compared to other implementations. I don't see in your series any addressing or reparent clock or take in account that a clock should be enable before reparenting. > My series is complimentary, even though there are some overlaps that we > need to merge. The only collision I can see from your series is that you re-write the imx approach of power domain. Can you please expand here a bit your point? Michael > > Thanks, > Miquèl
Hello Miquel, On 01.10.24 10:29, Miquel Raynal wrote: > Hi Heiko, > > >>>>> Hmm.. unfortunately ... I had applied your 2 clock patches, which >>>>> fixed a problem with enabling parent clocks ... but they broke booting >>>>> on a carrier which has fec ethernet! After "Net: " output the board hang... >>>>> >>>>> I reverted your 2 clock patches and it bootet again ... so there is >>>>> a problem ... try to get some more time to look into... >>>>> >>>> >>>> We have a fec, but we had I think two patches more on it. I forget to >>>> answer Marek >>>> about them because I don't have my board now and because he is >>>> partially right (or maybe right). >>>> Anyway when we boot we could have and we have clocks that are enabled >>>> by bootrom or SPL that >>>> we need to declare as enable like PLL2/PLL3 those clocks are parts or >>>> could be part of reparent so, >>>> you need to have a reference counter on them that allow to not disable >>>> during the down chain disable >>>> and up chain enable. I think that what happens to your ethernet it's >>>> that you disable some clock that is >>>> critical to the board to survive. I had a patch merged by Tom that >>> >>> Yep, thats what I think too! If you access registers in a block for >>> which the clock is not enabled ... it just hang... >>> >>>> prints the clock name so if you enable >>>> the debug of the clock you will find that your board stops working >>>> during one of this reparinting. >>> >>> I currently work on 2024.07... will rebase if 2024.10 is out... >>> >>> Ah, you mean: >>> >>> commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08 >>> Author: Michael Trimarchi <michael@amarulasolutions.com> >>> Date: Tue Jul 9 08:28:13 2024 +0200 >>> >>> clk: clk-uclass: Print clk name in clk_enable/clk_disable >>> >>> Print clk name in clk_enable and clk_disable. Make sense to know >>> what clock get disabled/enabled before a system crash or system >>> hang. >>> >>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> >>> >>> I have the same change when I debug :-P >>> >>> IIRC I did not see the clock names ... but I will recheck! >> >> I see with your patch the clock names, so fine... and see [1] >> >> Hmm... I am on imx8mp, and I think the changes the patchset do in >> >> "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate" >> >> are in clk-imx8mp already ... >> >> Ported the patch from patchset >> >> "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" >> >> to imx8mp [2] and fec ethernet works again for me on imx8mp! >> >> Could you add this if you post a v2 ? > > TBH I don't feel like the below change is the correct one, it is too > specific. The clock core is recursive and thus should handle the > reparenting situations gracefully. > > I posted a series that is targeting the LVDS output on imx8mp. You > should probably consider checking these patches as well if you work > on imx8mp as well. I also had similar breakages with Ethernet which > happened during the assigned-clocks handling. This patch, which is more > future and platform agnostic, fixed it: > > https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ Yes, I also had such a "fix" first, before seeing Darios patchset! Damn, I did not see your patches... regarding your patch ... I am unsure which version is the best one ... I fear of side effects for other plattforms adding this change in generic "drivers/clk/clk-uclass.c" file... > My series is complimentary, even though there are some overlaps that we > need to merge. I try to find time to test them on my board! bye, Heiko
Hi Michael, michael@amarulasolutions.com wrote on Tue, 1 Oct 2024 10:33:56 +0200: > Hi Miguel > > On Tue, Oct 1, 2024 at 10:29 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Heiko, > > > > > > > >>> Hmm.. unfortunately ... I had applied your 2 clock patches, which > > > >>> fixed a problem with enabling parent clocks ... but they broke booting > > > >>> on a carrier which has fec ethernet! After "Net: " output the board hang... > > > >>> > > > >>> I reverted your 2 clock patches and it bootet again ... so there is > > > >>> a problem ... try to get some more time to look into... > > > >>> > > > >> > > > >> We have a fec, but we had I think two patches more on it. I forget to > > > >> answer Marek > > > >> about them because I don't have my board now and because he is > > > >> partially right (or maybe right). > > > >> Anyway when we boot we could have and we have clocks that are enabled > > > >> by bootrom or SPL that > > > >> we need to declare as enable like PLL2/PLL3 those clocks are parts or > > > >> could be part of reparent so, > > > >> you need to have a reference counter on them that allow to not disable > > > >> during the down chain disable > > > >> and up chain enable. I think that what happens to your ethernet it's > > > >> that you disable some clock that is > > > >> critical to the board to survive. I had a patch merged by Tom that > > > > > > > > Yep, thats what I think too! If you access registers in a block for > > > > which the clock is not enabled ... it just hang... > > > > > > > >> prints the clock name so if you enable > > > >> the debug of the clock you will find that your board stops working > > > >> during one of this reparinting. > > > > > > > > I currently work on 2024.07... will rebase if 2024.10 is out... > > > > > > > > Ah, you mean: > > > > > > > > commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08 > > > > Author: Michael Trimarchi <michael@amarulasolutions.com> > > > > Date: Tue Jul 9 08:28:13 2024 +0200 > > > > > > > > clk: clk-uclass: Print clk name in clk_enable/clk_disable > > > > > > > > Print clk name in clk_enable and clk_disable. Make sense to know > > > > what clock get disabled/enabled before a system crash or system > > > > hang. > > > > > > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > > > > > > > > I have the same change when I debug :-P > > > > > > > > IIRC I did not see the clock names ... but I will recheck! > > > > > > I see with your patch the clock names, so fine... and see [1] > > > > > > Hmm... I am on imx8mp, and I think the changes the patchset do in > > > > > > "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate" > > > > > > are in clk-imx8mp already ... > > > > > > Ported the patch from patchset > > > > > > "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" > > > > > > to imx8mp [2] and fec ethernet works again for me on imx8mp! > > > > > > Could you add this if you post a v2 ? > > > > TBH I don't feel like the below change is the correct one, it is too > > specific. The clock core is recursive and thus should handle the > > reparenting situations gracefully. > > > > I posted a series that is targeting the LVDS output on imx8mp. You > > should probably consider checking these patches as well if you work > > on imx8mp as well. I also had similar breakages with Ethernet which > > happened during the assigned-clocks handling. This patch, which is more > > future and platform agnostic, fixed it: > > > > https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ > > > > The clock patches are not specific at all. You need to have it working > for the parent for each component. The diff shown by Heiko is explicitly enabling PLLs by naming them in the iMX driver. This is not the correct approach. The problem of having non-enabled new parents is global. Parent clocks should be enabled before changing muxes, and this should be enforced by the clock core/uclass, not the SoC drivers. > This is a standard way to do it and nothing magic compared to other > implementations. No, naming PLLs explicitly is not the correct approach. > I don't see > in your series any addressing or reparent clock or take in account > that a clock should be enable before > reparenting. That's exactly the link above, whose diff is pasted here for reference: @@ -595,6 +595,10 @@ int clk_set_parent(struct clk *clk, struct clk *parent) if (!ops->set_parent) return -ENOSYS; + ret = clk_enable(parent); + if (ret) + return ret; > > > My series is complimentary, even though there are some overlaps that we > > need to merge. > > The only collision I can see from your series is that you re-write the > imx approach of power domain. > Can you please expand here a bit your point? Well, that's what I'd call an overlap :) There are also similar changes in the clock core IIRC. Well, there is a bit of merging that needs to happen I guess, but if you don't think there is any then my series can enter as-is (once the comments addressed, ofc). Thanks, Miquèl To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Hi Heiko, > >> I see with your patch the clock names, so fine... and see [1] > >> > >> Hmm... I am on imx8mp, and I think the changes the patchset do in > >> > >> "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate" > >> > >> are in clk-imx8mp already ... > >> > >> Ported the patch from patchset > >> > >> "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" > >> > >> to imx8mp [2] and fec ethernet works again for me on imx8mp! > >> > >> Could you add this if you post a v2 ? > > > > TBH I don't feel like the below change is the correct one, it is too > > specific. The clock core is recursive and thus should handle the > > reparenting situations gracefully. > > > > I posted a series that is targeting the LVDS output on imx8mp. You > > should probably consider checking these patches as well if you work > > on imx8mp as well. I also had similar breakages with Ethernet which > > happened during the assigned-clocks handling. This patch, which is more > > future and platform agnostic, fixed it: > > > > https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ > > Yes, I also had such a "fix" first, before seeing Darios patchset! > > Damn, I did not see your patches... > > regarding your patch ... I am unsure which version is the best one ... > I fear of side effects for other plattforms adding this change in > generic "drivers/clk/clk-uclass.c" file... FYI, this is the same approach as Linux. I don't expect any hardware related breakage, however if people have already tried to workaround the core's mistakes by doing more than expected in the SoC drivers, maybe we will observe issues. > > My series is complimentary, even though there are some overlaps that we > > need to merge. > > I try to find time to test them on my board! Sure, let me know if you have troubles, it's still fresh :-) I will rework it a bit as it received already interesting feedback, and will note to Cc you (as well as Dario/Michael) in v2. Thanks, Miquèl To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Hello Miquel, On 01.10.24 10:50, Miquel Raynal wrote: > Hi Michael, > > michael@amarulasolutions.com wrote on Tue, 1 Oct 2024 10:33:56 +0200: > >> Hi Miguel >> >> On Tue, Oct 1, 2024 at 10:29 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: >>> >>> Hi Heiko, >>> >>> >>>>>>> Hmm.. unfortunately ... I had applied your 2 clock patches, which >>>>>>> fixed a problem with enabling parent clocks ... but they broke booting >>>>>>> on a carrier which has fec ethernet! After "Net: " output the board hang... >>>>>>> >>>>>>> I reverted your 2 clock patches and it bootet again ... so there is >>>>>>> a problem ... try to get some more time to look into... >>>>>>> >>>>>> >>>>>> We have a fec, but we had I think two patches more on it. I forget to >>>>>> answer Marek >>>>>> about them because I don't have my board now and because he is >>>>>> partially right (or maybe right). >>>>>> Anyway when we boot we could have and we have clocks that are enabled >>>>>> by bootrom or SPL that >>>>>> we need to declare as enable like PLL2/PLL3 those clocks are parts or >>>>>> could be part of reparent so, >>>>>> you need to have a reference counter on them that allow to not disable >>>>>> during the down chain disable >>>>>> and up chain enable. I think that what happens to your ethernet it's >>>>>> that you disable some clock that is >>>>>> critical to the board to survive. I had a patch merged by Tom that >>>>> >>>>> Yep, thats what I think too! If you access registers in a block for >>>>> which the clock is not enabled ... it just hang... >>>>> >>>>>> prints the clock name so if you enable >>>>>> the debug of the clock you will find that your board stops working >>>>>> during one of this reparinting. >>>>> >>>>> I currently work on 2024.07... will rebase if 2024.10 is out... >>>>> >>>>> Ah, you mean: >>>>> >>>>> commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08 >>>>> Author: Michael Trimarchi <michael@amarulasolutions.com> >>>>> Date: Tue Jul 9 08:28:13 2024 +0200 >>>>> >>>>> clk: clk-uclass: Print clk name in clk_enable/clk_disable >>>>> >>>>> Print clk name in clk_enable and clk_disable. Make sense to know >>>>> what clock get disabled/enabled before a system crash or system >>>>> hang. >>>>> >>>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> >>>>> >>>>> I have the same change when I debug :-P >>>>> >>>>> IIRC I did not see the clock names ... but I will recheck! >>>> >>>> I see with your patch the clock names, so fine... and see [1] >>>> >>>> Hmm... I am on imx8mp, and I think the changes the patchset do in >>>> >>>> "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate" >>>> >>>> are in clk-imx8mp already ... >>>> >>>> Ported the patch from patchset >>>> >>>> "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" >>>> >>>> to imx8mp [2] and fec ethernet works again for me on imx8mp! >>>> >>>> Could you add this if you post a v2 ? >>> >>> TBH I don't feel like the below change is the correct one, it is too >>> specific. The clock core is recursive and thus should handle the >>> reparenting situations gracefully. >>> >>> I posted a series that is targeting the LVDS output on imx8mp. You >>> should probably consider checking these patches as well if you work >>> on imx8mp as well. I also had similar breakages with Ethernet which >>> happened during the assigned-clocks handling. This patch, which is more >>> future and platform agnostic, fixed it: >>> >>> https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ >>> >> >> The clock patches are not specific at all. You need to have it working >> for the parent for each component. > > The diff shown by Heiko is explicitly enabling PLLs by naming them in > the iMX driver. This is not the correct approach. The problem of > having non-enabled new parents is global. Parent clocks should be > enabled before changing muxes, and this should be enforced > by the clock core/uclass, not the SoC drivers. Okay, valid argument. > >> This is a standard way to do it and nothing magic compared to other >> implementations. > > No, naming PLLs explicitly is not the correct approach. > >> I don't see >> in your series any addressing or reparent clock or take in account >> that a clock should be enable before >> reparenting. > > That's exactly the link above, whose diff is pasted here for reference: > > @@ -595,6 +595,10 @@ int clk_set_parent(struct clk *clk, struct clk *parent) > if (!ops->set_parent) > return -ENOSYS; > > + ret = clk_enable(parent); > + if (ret) > + return ret; As I said before, I had *exact* the same patch and thought I made a big hack :-P But I wonder ... if this a generic "problem", why nobody had yet problems with it... Thanks! bye, Heiko >>> My series is complimentary, even though there are some overlaps that we >>> need to merge. >> >> The only collision I can see from your series is that you re-write the >> imx approach of power domain. >> Can you please expand here a bit your point? > > Well, that's what I'd call an overlap :) There are also similar changes > in the clock core IIRC. Well, there is a bit of merging that needs to > happen I guess, but if you don't think there is any then my series can > enter as-is (once the comments addressed, ofc). > > Thanks, > Miquèl >
Hi all On Tue, Oct 1, 2024 at 10:59 AM Heiko Schocher <hs@denx.de> wrote: > > Hello Miquel, > > On 01.10.24 10:50, Miquel Raynal wrote: > > Hi Michael, > > > > michael@amarulasolutions.com wrote on Tue, 1 Oct 2024 10:33:56 +0200: > > > >> Hi Miguel > >> > >> On Tue, Oct 1, 2024 at 10:29 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > >>> > >>> Hi Heiko, > >>> > >>> > >>>>>>> Hmm.. unfortunately ... I had applied your 2 clock patches, which > >>>>>>> fixed a problem with enabling parent clocks ... but they broke booting > >>>>>>> on a carrier which has fec ethernet! After "Net: " output the board hang... > >>>>>>> > >>>>>>> I reverted your 2 clock patches and it bootet again ... so there is > >>>>>>> a problem ... try to get some more time to look into... > >>>>>>> > >>>>>> > >>>>>> We have a fec, but we had I think two patches more on it. I forget to > >>>>>> answer Marek > >>>>>> about them because I don't have my board now and because he is > >>>>>> partially right (or maybe right). > >>>>>> Anyway when we boot we could have and we have clocks that are enabled > >>>>>> by bootrom or SPL that > >>>>>> we need to declare as enable like PLL2/PLL3 those clocks are parts or > >>>>>> could be part of reparent so, > >>>>>> you need to have a reference counter on them that allow to not disable > >>>>>> during the down chain disable > >>>>>> and up chain enable. I think that what happens to your ethernet it's > >>>>>> that you disable some clock that is > >>>>>> critical to the board to survive. I had a patch merged by Tom that > >>>>> > >>>>> Yep, thats what I think too! If you access registers in a block for > >>>>> which the clock is not enabled ... it just hang... > >>>>> > >>>>>> prints the clock name so if you enable > >>>>>> the debug of the clock you will find that your board stops working > >>>>>> during one of this reparinting. > >>>>> > >>>>> I currently work on 2024.07... will rebase if 2024.10 is out... > >>>>> > >>>>> Ah, you mean: > >>>>> > >>>>> commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08 > >>>>> Author: Michael Trimarchi <michael@amarulasolutions.com> > >>>>> Date: Tue Jul 9 08:28:13 2024 +0200 > >>>>> > >>>>> clk: clk-uclass: Print clk name in clk_enable/clk_disable > >>>>> > >>>>> Print clk name in clk_enable and clk_disable. Make sense to know > >>>>> what clock get disabled/enabled before a system crash or system > >>>>> hang. > >>>>> > >>>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > >>>>> > >>>>> I have the same change when I debug :-P > >>>>> > >>>>> IIRC I did not see the clock names ... but I will recheck! > >>>> > >>>> I see with your patch the clock names, so fine... and see [1] > >>>> > >>>> Hmm... I am on imx8mp, and I think the changes the patchset do in > >>>> > >>>> "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate" > >>>> > >>>> are in clk-imx8mp already ... > >>>> > >>>> Ported the patch from patchset > >>>> > >>>> "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" > >>>> > >>>> to imx8mp [2] and fec ethernet works again for me on imx8mp! > >>>> > >>>> Could you add this if you post a v2 ? > >>> > >>> TBH I don't feel like the below change is the correct one, it is too > >>> specific. The clock core is recursive and thus should handle the > >>> reparenting situations gracefully. > >>> > >>> I posted a series that is targeting the LVDS output on imx8mp. You > >>> should probably consider checking these patches as well if you work > >>> on imx8mp as well. I also had similar breakages with Ethernet which > >>> happened during the assigned-clocks handling. This patch, which is more > >>> future and platform agnostic, fixed it: > >>> > >>> https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ > >>> > >> > >> The clock patches are not specific at all. You need to have it working > >> for the parent for each component. > > > > The diff shown by Heiko is explicitly enabling PLLs by naming them in > > the iMX driver. This is not the correct approach. The problem of > > having non-enabled new parents is global. Parent clocks should be > > enabled before changing muxes, and this should be enforced > > by the clock core/uclass, not the SoC drivers. > > Okay, valid argument. > > > > >> This is a standard way to do it and nothing magic compared to other > >> implementations. > > > > No, naming PLLs explicitly is not the correct approach. > > > >> I don't see > >> in your series any addressing or reparent clock or take in account > >> that a clock should be enable before > >> reparenting. > > > > That's exactly the link above, whose diff is pasted here for reference: > > > > @@ -595,6 +595,10 @@ int clk_set_parent(struct clk *clk, struct clk *parent) > > if (!ops->set_parent) > > return -ENOSYS; > > > > + ret = clk_enable(parent); > > + if (ret) > > + return ret; > > As I said before, I had *exact* the same patch and thought I made a big > hack :-P > > But I wonder ... if this a generic "problem", why nobody had yet problems > with it... I think that a generic approach that takes into account the reparent is more valuable, then this. If a clock is enabled by another stage and we don't aware about it we need to mark as enabled. I think that this force of enable it's just a short path that does not solve the generic problem. I really tried to abstract what is really implemented in other OS on the same topic. I don't want to have a discussion about who should merge first. Michael > > Thanks! > > bye, > Heiko > > >>> My series is complimentary, even though there are some overlaps that we > >>> need to merge. > >> > >> The only collision I can see from your series is that you re-write the > >> imx approach of power domain. > >> Can you please expand here a bit your point? > > > > Well, that's what I'd call an overlap :) There are also similar changes > > in the clock core IIRC. Well, there is a bit of merging that needs to > > happen I guess, but if you don't think there is any then my series can > > enter as-is (once the comments addressed, ofc). > > > > Thanks, > > Miquèl > > > > -- > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de -- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com
Hi Heiko, hs@denx.de wrote on Tue, 1 Oct 2024 10:57:27 +0200: > Hello Miquel, > > On 01.10.24 10:50, Miquel Raynal wrote: > > Hi Michael, > > > > michael@amarulasolutions.com wrote on Tue, 1 Oct 2024 10:33:56 +0200: > > > >> Hi Miguel > >> > >> On Tue, Oct 1, 2024 at 10:29 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > >>> > >>> Hi Heiko, > >>> > >>> >>>>>>> Hmm.. unfortunately ... I had applied your 2 clock patches, which > >>>>>>> fixed a problem with enabling parent clocks ... but they broke booting > >>>>>>> on a carrier which has fec ethernet! After "Net: " output the board hang... > >>>>>>> > >>>>>>> I reverted your 2 clock patches and it bootet again ... so there is > >>>>>>> a problem ... try to get some more time to look into... > >>>>>>> >>>>>> > >>>>>> We have a fec, but we had I think two patches more on it. I forget to > >>>>>> answer Marek > >>>>>> about them because I don't have my board now and because he is > >>>>>> partially right (or maybe right). > >>>>>> Anyway when we boot we could have and we have clocks that are enabled > >>>>>> by bootrom or SPL that > >>>>>> we need to declare as enable like PLL2/PLL3 those clocks are parts or > >>>>>> could be part of reparent so, > >>>>>> you need to have a reference counter on them that allow to not disable > >>>>>> during the down chain disable > >>>>>> and up chain enable. I think that what happens to your ethernet it's > >>>>>> that you disable some clock that is > >>>>>> critical to the board to survive. I had a patch merged by Tom that > >>>>> > >>>>> Yep, thats what I think too! If you access registers in a block for > >>>>> which the clock is not enabled ... it just hang... > >>>>> >>>>>> prints the clock name so if you enable > >>>>>> the debug of the clock you will find that your board stops working > >>>>>> during one of this reparinting. > >>>>> > >>>>> I currently work on 2024.07... will rebase if 2024.10 is out... > >>>>> > >>>>> Ah, you mean: > >>>>> > >>>>> commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08 > >>>>> Author: Michael Trimarchi <michael@amarulasolutions.com> > >>>>> Date: Tue Jul 9 08:28:13 2024 +0200 > >>>>> > >>>>> clk: clk-uclass: Print clk name in clk_enable/clk_disable > >>>>> > >>>>> Print clk name in clk_enable and clk_disable. Make sense to know > >>>>> what clock get disabled/enabled before a system crash or system > >>>>> hang. > >>>>> > >>>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > >>>>> > >>>>> I have the same change when I debug :-P > >>>>> > >>>>> IIRC I did not see the clock names ... but I will recheck! > >>>> > >>>> I see with your patch the clock names, so fine... and see [1] > >>>> > >>>> Hmm... I am on imx8mp, and I think the changes the patchset do in > >>>> > >>>> "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate" > >>>> > >>>> are in clk-imx8mp already ... > >>>> > >>>> Ported the patch from patchset > >>>> > >>>> "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" > >>>> > >>>> to imx8mp [2] and fec ethernet works again for me on imx8mp! > >>>> > >>>> Could you add this if you post a v2 ? > >>> > >>> TBH I don't feel like the below change is the correct one, it is too > >>> specific. The clock core is recursive and thus should handle the > >>> reparenting situations gracefully. > >>> > >>> I posted a series that is targeting the LVDS output on imx8mp. You > >>> should probably consider checking these patches as well if you work > >>> on imx8mp as well. I also had similar breakages with Ethernet which > >>> happened during the assigned-clocks handling. This patch, which is more > >>> future and platform agnostic, fixed it: > >>> > >>> https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ > >>> >> > >> The clock patches are not specific at all. You need to have it working > >> for the parent for each component. > > > > The diff shown by Heiko is explicitly enabling PLLs by naming them in > > the iMX driver. This is not the correct approach. The problem of > > having non-enabled new parents is global. Parent clocks should be > > enabled before changing muxes, and this should be enforced > > by the clock core/uclass, not the SoC drivers. > > Okay, valid argument. > > > > >> This is a standard way to do it and nothing magic compared to other > >> implementations. > > > > No, naming PLLs explicitly is not the correct approach. > > > >> I don't see > >> in your series any addressing or reparent clock or take in account > >> that a clock should be enable before > >> reparenting. > > > > That's exactly the link above, whose diff is pasted here for reference: > > > > @@ -595,6 +595,10 @@ int clk_set_parent(struct clk *clk, struct clk *parent) > > if (!ops->set_parent) > > return -ENOSYS; > > > + ret = clk_enable(parent); > > + if (ret) > > + return ret; > > As I said before, I had *exact* the same patch and thought I made a big > hack :-P I don't think so :-) > But I wonder ... if this a generic "problem", why nobody had yet problems > with it... It depends on your hardware and how you use it I guess. If you look into the terribly complex clock slices explanation in the imx8mp manual, you'll see what the target clock interface does behind the scenes when you use it. There are like 25 steps internally to prevent glitch-free changes. I suspect one of those actions to stall if the parent is not enabled early enough during the reparenting procedure. Now, the reason why parents might be disabled is because of the high level of configurability this SoC offers which is likely higher than many other SoCs. But in general, to avoid these stalls, there has been a quite extensive use of assigned-clocks properties. I believe with yet another entry in this list, we could probably make it work, but that's not the correct approach either. Thanks, Miquèl To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Hi Michael, > > >>>> Ported the patch from patchset > > >>>> > > >>>> "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" > > >>>> > > >>>> to imx8mp [2] and fec ethernet works again for me on imx8mp! > > >>>> > > >>>> Could you add this if you post a v2 ? > > >>> > > >>> TBH I don't feel like the below change is the correct one, it is too > > >>> specific. The clock core is recursive and thus should handle the > > >>> reparenting situations gracefully. > > >>> > > >>> I posted a series that is targeting the LVDS output on imx8mp. You > > >>> should probably consider checking these patches as well if you work > > >>> on imx8mp as well. I also had similar breakages with Ethernet which > > >>> happened during the assigned-clocks handling. This patch, which is more > > >>> future and platform agnostic, fixed it: > > >>> > > >>> https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ > > >>> > > >> > > >> The clock patches are not specific at all. You need to have it working > > >> for the parent for each component. > > > > > > The diff shown by Heiko is explicitly enabling PLLs by naming them in > > > the iMX driver. This is not the correct approach. The problem of > > > having non-enabled new parents is global. Parent clocks should be > > > enabled before changing muxes, and this should be enforced > > > by the clock core/uclass, not the SoC drivers. > > > > Okay, valid argument. > > > > > > > >> This is a standard way to do it and nothing magic compared to other > > >> implementations. > > > > > > No, naming PLLs explicitly is not the correct approach. > > > > > >> I don't see > > >> in your series any addressing or reparent clock or take in account > > >> that a clock should be enable before > > >> reparenting. > > > > > > That's exactly the link above, whose diff is pasted here for reference: > > > > > > @@ -595,6 +595,10 @@ int clk_set_parent(struct clk *clk, struct clk *parent) > > > if (!ops->set_parent) > > > return -ENOSYS; > > > > > > + ret = clk_enable(parent); > > > + if (ret) > > > + return ret; > > > > As I said before, I had *exact* the same patch and thought I made a big > > hack :-P > > > > But I wonder ... if this a generic "problem", why nobody had yet problems > > with it... > > I think that a generic approach that takes into account the reparent > is more valuable, then > this. I'm sorry I don't fully understand your answer. I assume you agree with the generic approach quoted above. > If a clock is enabled by another stage and we don't aware about > it we need to mark > as enabled. clk_enable() already handles this kind of situation. > I think that this force of enable it's just a short path > that does not solve the generic > problem. What "force of enable"? The parent needs to be enabled before we use it as parent. The logic is always the same: - clk_enable() the new parent - change the parent - clk_disable() the old parent I don't see any short path here. > I really tried to abstract what is really implemented in > other OS on the same topic. FYI, Linux does the clk_enable(parent) like above. Thanks, Miquèl To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Hi On Tue, Oct 1, 2024 at 12:01 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Michael, > > > > >>>> Ported the patch from patchset > > > >>>> > > > >>>> "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" > > > >>>> > > > >>>> to imx8mp [2] and fec ethernet works again for me on imx8mp! > > > >>>> > > > >>>> Could you add this if you post a v2 ? > > > >>> > > > >>> TBH I don't feel like the below change is the correct one, it is too > > > >>> specific. The clock core is recursive and thus should handle the > > > >>> reparenting situations gracefully. > > > >>> > > > >>> I posted a series that is targeting the LVDS output on imx8mp. You > > > >>> should probably consider checking these patches as well if you work > > > >>> on imx8mp as well. I also had similar breakages with Ethernet which > > > >>> happened during the assigned-clocks handling. This patch, which is more > > > >>> future and platform agnostic, fixed it: > > > >>> > > > >>> https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ > > > >>> > > > >> > > > >> The clock patches are not specific at all. You need to have it working > > > >> for the parent for each component. > > > > > > > > The diff shown by Heiko is explicitly enabling PLLs by naming them in > > > > the iMX driver. This is not the correct approach. The problem of > > > > having non-enabled new parents is global. Parent clocks should be > > > > enabled before changing muxes, and this should be enforced > > > > by the clock core/uclass, not the SoC drivers. > > > > > > Okay, valid argument. > > > > > > > > > > >> This is a standard way to do it and nothing magic compared to other > > > >> implementations. > > > > > > > > No, naming PLLs explicitly is not the correct approach. > > > > > > > >> I don't see > > > >> in your series any addressing or reparent clock or take in account > > > >> that a clock should be enable before > > > >> reparenting. > > > > > > > > That's exactly the link above, whose diff is pasted here for reference: > > > > > > > > @@ -595,6 +595,10 @@ int clk_set_parent(struct clk *clk, struct clk *parent) > > > > if (!ops->set_parent) > > > > return -ENOSYS; > > > > > > > > + ret = clk_enable(parent); > > > > + if (ret) > > > > + return ret; > > > > > > As I said before, I had *exact* the same patch and thought I made a big > > > hack :-P > > > > > > But I wonder ... if this a generic "problem", why nobody had yet problems > > > with it... > > > > I think that a generic approach that takes into account the reparent > > is more valuable, then > > this. > > I'm sorry I don't fully understand your answer. I assume you agree with > the generic approach quoted above. > > > If a clock is enabled by another stage and we don't aware about > > it we need to mark > > as enabled. > > clk_enable() already handles this kind of situation. > > > I think that this force of enable it's just a short path > > that does not solve the generic > > problem. > > What "force of enable"? The parent needs to be enabled before we use it > as parent. The logic is always the same: > - clk_enable() the new parent > - change the parent > - clk_disable() the old parent > I don't see any short path here. > > > I really tried to abstract what is really implemented in > > other OS on the same topic. > > FYI, Linux does the clk_enable(parent) like above. > You mean linux enables a clock in set_parent even if the clock_chain is not yet enabled. clock a disable clock b disable clk_set_parent(a, b) b->a now b is enabled? Michael > Thanks, > Miquèl
Hi Michael, michael@amarulasolutions.com wrote on Tue, 1 Oct 2024 15:02:56 +0200: > Hi > > On Tue, Oct 1, 2024 at 12:01 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Michael, > > > > > > >>>> Ported the patch from patchset > > > > >>>> > > > > >>>> "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled" > > > > >>>> > > > > >>>> to imx8mp [2] and fec ethernet works again for me on imx8mp! > > > > >>>> > > > > >>>> Could you add this if you post a v2 ? > > > > >>> > > > > >>> TBH I don't feel like the below change is the correct one, it is too > > > > >>> specific. The clock core is recursive and thus should handle the > > > > >>> reparenting situations gracefully. > > > > >>> > > > > >>> I posted a series that is targeting the LVDS output on imx8mp. You > > > > >>> should probably consider checking these patches as well if you work > > > > >>> on imx8mp as well. I also had similar breakages with Ethernet which > > > > >>> happened during the assigned-clocks handling. This patch, which is more > > > > >>> future and platform agnostic, fixed it: > > > > >>> > > > > >>> https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/ > > > > >>> > > > > >> > > > > >> The clock patches are not specific at all. You need to have it working > > > > >> for the parent for each component. > > > > > > > > > > The diff shown by Heiko is explicitly enabling PLLs by naming them in > > > > > the iMX driver. This is not the correct approach. The problem of > > > > > having non-enabled new parents is global. Parent clocks should be > > > > > enabled before changing muxes, and this should be enforced > > > > > by the clock core/uclass, not the SoC drivers. > > > > > > > > Okay, valid argument. > > > > > > > > > > > > > >> This is a standard way to do it and nothing magic compared to other > > > > >> implementations. > > > > > > > > > > No, naming PLLs explicitly is not the correct approach. > > > > > > > > > >> I don't see > > > > >> in your series any addressing or reparent clock or take in account > > > > >> that a clock should be enable before > > > > >> reparenting. > > > > > > > > > > That's exactly the link above, whose diff is pasted here for reference: > > > > > > > > > > @@ -595,6 +595,10 @@ int clk_set_parent(struct clk *clk, struct clk *parent) > > > > > if (!ops->set_parent) > > > > > return -ENOSYS; > > > > > > > > > > + ret = clk_enable(parent); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > As I said before, I had *exact* the same patch and thought I made a big > > > > hack :-P > > > > > > > > But I wonder ... if this a generic "problem", why nobody had yet problems > > > > with it... > > > > > > I think that a generic approach that takes into account the reparent > > > is more valuable, then > > > this. > > > > I'm sorry I don't fully understand your answer. I assume you agree with > > the generic approach quoted above. > > > > > If a clock is enabled by another stage and we don't aware about > > > it we need to mark > > > as enabled. > > > > clk_enable() already handles this kind of situation. > > > > > I think that this force of enable it's just a short path > > > that does not solve the generic > > > problem. > > > > What "force of enable"? The parent needs to be enabled before we use it > > as parent. The logic is always the same: > > - clk_enable() the new parent > > - change the parent > > - clk_disable() the old parent > > I don't see any short path here. > > > > > I really tried to abstract what is really implemented in > > > other OS on the same topic. > > > > FYI, Linux does the clk_enable(parent) like above. > > > > You mean linux enables a clock in set_parent even if the clock_chain > is not yet enabled. > clock a disable > clock b disable > > clk_set_parent(a, b) b->a now b is enabled? Actually you're right Linux does the enable call only if a flag is set (and anyway it's not in the faulty case reported above) or when the reparented clock is already clocking. I'll have to check again with this clk_enable() solves the situation and what could be done instead. Thanks, Miquèl To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig index bd82d2f7044b..fb006b6e8e28 100644 --- a/drivers/power/domain/Kconfig +++ b/drivers/power/domain/Kconfig @@ -40,6 +40,12 @@ config IMX8M_POWER_DOMAIN Enable support for manipulating NXP i.MX8M on-SoC power domains via requests to the ATF. +config IMX8M_BLK_CTRL + bool "Enable i.MX8M block control driver" + depends on POWER_DOMAIN && ARCH_IMX8M + help + Enable support for manipulating NXP i.MX8M on-SoC block control driver + config IMX8MP_HSIOMIX_BLKCTRL bool "Enable i.MX8MP HSIOMIX domain driver" depends on POWER_DOMAIN && IMX8MP diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile index 2daab73eb758..46849fd2a4db 100644 --- a/drivers/power/domain/Makefile +++ b/drivers/power/domain/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_APPLE_PMGR_POWER_DOMAIN) += apple-pmgr.o obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o +obj-$(CONFIG_IMX8M_BLK_CTRL) += imx8m-blk-ctrl.o obj-$(CONFIG_IMX8MP_HSIOMIX_BLKCTRL) += imx8mp-hsiomix.o obj-$(CONFIG_MTK_POWER_DOMAIN) += mtk-power-domain.o obj-$(CONFIG_MESON_GX_VPU_POWER_DOMAIN) += meson-gx-pwrc-vpu.o diff --git a/drivers/power/domain/imx8m-blk-ctrl.c b/drivers/power/domain/imx8m-blk-ctrl.c new file mode 100644 index 000000000000..4c89078b991b --- /dev/null +++ b/drivers/power/domain/imx8m-blk-ctrl.c @@ -0,0 +1,438 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2023 NXP + */ + +#include <dm.h> +#include <malloc.h> +#include <power-domain-uclass.h> +#include <asm/io.h> +#include <dm/device-internal.h> +#include <dm/device.h> +#include <dt-bindings/power/imx8mm-power.h> +#include <dt-bindings/power/imx8mn-power.h> +#include <dt-bindings/power/imx8mp-power.h> +#include <clk.h> +#include <linux/delay.h> + +#define BLK_SFT_RSTN 0x0 +#define BLK_CLK_EN 0x4 +#define BLK_MIPI_RESET_DIV 0x8 /* Mini/Nano/Plus DISPLAY_BLK_CTRL only */ + +#define DOMAIN_MAX_CLKS 4 + +struct imx8m_blk_ctrl_domain { + struct clk clks[DOMAIN_MAX_CLKS]; + struct power_domain power_dev; +}; + +struct imx8m_blk_ctrl { + void __iomem *base; + struct power_domain bus_power_dev; + struct imx8m_blk_ctrl_domain *domains; +}; + +struct imx8m_blk_ctrl_domain_data { + const char *name; + const char * const *clk_names; + const char *gpc_name; + int num_clks; + u32 rst_mask; + u32 clk_mask; + u32 mipi_phy_rst_mask; +}; + +struct imx8m_blk_ctrl_data { + int max_reg; + const struct imx8m_blk_ctrl_domain_data *domains; + int num_domains; + u32 bus_rst_mask; + u32 bus_clk_mask; +}; + +static int imx8m_blk_ctrl_request(struct power_domain *power_domain) +{ + return 0; +} + +static int imx8m_blk_ctrl_free(struct power_domain *power_domain) +{ + return 0; +} + +static int imx8m_blk_ctrl_enable_domain_clk(struct udevice *dev, ulong domain_id, bool enable) +{ + int ret, i; + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); + struct imx8m_blk_ctrl_data *drv_data = + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); + + debug("%s num_clk %u\n", __func__, drv_data->domains[domain_id].num_clks); + + for (i = 0; i < drv_data->domains[domain_id].num_clks; i++) { + debug("%s clk %s\n", __func__, drv_data->domains[domain_id].clk_names[i]); + if (enable) + ret = clk_enable(&priv->domains[domain_id].clks[i]); + else + ret = clk_disable(&priv->domains[domain_id].clks[i]); + if (ret && ret != -ENOENT) { + printf("Failed to %s domain clk %s\n", enable ? "enable" : "disable", + drv_data->domains[domain_id].clk_names[i]); + return ret; + } + } + + return 0; +} + +static int imx8m_blk_ctrl_power_on(struct power_domain *power_domain) +{ + struct udevice *dev = power_domain->dev; + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); + struct imx8m_blk_ctrl_data *drv_data = + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); + int ret; + + debug("%s, id %lu\n", __func__, power_domain->id); + + if (!priv->domains[power_domain->id].power_dev.dev) + return -ENODEV; + + ret = power_domain_on(&priv->bus_power_dev); + if (ret < 0) { + printf("Failed to power up bus domain %d\n", ret); + return ret; + } + + /* Enable bus clock and deassert bus reset */ + setbits_le32(priv->base + BLK_CLK_EN, drv_data->bus_clk_mask); + setbits_le32(priv->base + BLK_SFT_RSTN, drv_data->bus_rst_mask); + + /* wait for reset to propagate */ + udelay(5); + + /* put devices into reset */ + clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); + if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, d + rv_data->domains[power_domain->id].mipi_phy_rst_mask); + + /* enable upstream and blk-ctrl clocks to allow reset to propagate */ + ret = imx8m_blk_ctrl_enable_domain_clk(dev, power_domain->id, true); + if (ret) { + printf("failed to enable clocks\n"); + goto bus_powerdown; + } + + /* ungate clk */ + setbits_le32(priv->base + BLK_CLK_EN, drv_data->domains[power_domain->id].clk_mask); + + /* power up upstream GPC domain */ + ret = power_domain_on(&priv->domains[power_domain->id].power_dev); + if (ret < 0) { + printf("Failed to power up peripheral domain %d\n", ret); + goto clk_disable; + } + + /* wait for reset to propagate */ + udelay(5); + + /* release reset */ + setbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); + if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) + setbits_le32(priv->base + BLK_MIPI_RESET_DIV, + drv_data->domains[power_domain->id].mipi_phy_rst_mask); + + return 0; +clk_disable: + imx8m_blk_ctrl_enable_domain_clk(dev, power_domain->id, false); +bus_powerdown: + power_domain_off(&priv->bus_power_dev); + return ret; +} + +static int imx8m_blk_ctrl_power_off(struct power_domain *power_domain) +{ + struct udevice *dev = power_domain->dev; + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); + struct imx8m_blk_ctrl_data *drv_data = + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); + + debug("%s, id %lu\n", __func__, power_domain->id); + + if (!priv->domains[power_domain->id].power_dev.dev) + return -ENODEV; + + /* put devices into reset and disable clocks */ + if (drv_data->domains[power_domain->id].mipi_phy_rst_mask) + clrbits_le32(priv->base + BLK_MIPI_RESET_DIV, + drv_data->domains[power_domain->id].mipi_phy_rst_mask); + + /* assert reset */ + clrbits_le32(priv->base + BLK_SFT_RSTN, drv_data->domains[power_domain->id].rst_mask); + + /* gate clk */ + clrbits_le32(priv->base + BLK_CLK_EN, drv_data->domains[power_domain->id].clk_mask); + + /* power down upstream GPC domain */ + power_domain_off(&priv->domains[power_domain->id].power_dev); + + imx8m_blk_ctrl_enable_domain_clk(dev, power_domain->id, false); + + /* power down bus domain */ + power_domain_off(&priv->bus_power_dev); + + return 0; +} + +static int imx8m_blk_ctrl_probe(struct udevice *dev) +{ + int ret, i, j; + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); + struct imx8m_blk_ctrl_data *drv_data = + (struct imx8m_blk_ctrl_data *)dev_get_driver_data(dev); + + priv->base = dev_read_addr_ptr(dev); + if (!priv->base) + return -EINVAL; + + priv->domains = kcalloc(drv_data->num_domains, + sizeof(struct imx8m_blk_ctrl_domain), GFP_KERNEL); + + ret = power_domain_get_by_name(dev, &priv->bus_power_dev, "bus"); + if (ret) { + printf("Failed to power_domain_get_by_name %s\n", "bus"); + return ret; + } + + for (j = 0; j < drv_data->num_domains; j++) { + ret = power_domain_get_by_name(dev, &priv->domains[j].power_dev, + drv_data->domains[j].gpc_name); + if (ret) + continue; + + for (i = 0; i < drv_data->domains[j].num_clks; i++) { + ret = clk_get_by_name(dev, drv_data->domains[j].clk_names[i], + &priv->domains[j].clks[i]); + if (ret) { + printf("Failed to get clk %s\n", drv_data->domains[j].clk_names[i]); + return ret; + } + } + } + + return 0; +} + +static int imx8m_blk_ctrl_remove(struct udevice *dev) +{ + struct imx8m_blk_ctrl *priv = (struct imx8m_blk_ctrl *)dev_get_priv(dev); + + kfree(priv->domains); + + return 0; +} + +static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[] = { + [IMX8MM_DISPBLK_PD_CSI_BRIDGE] = { + .name = "dispblk-csi-bridge", + .clk_names = (const char *[]){ "csi-bridge-axi", "csi-bridge-apb", + "csi-bridge-core", }, + .num_clks = 3, + .gpc_name = "csi-bridge", + .rst_mask = BIT(0) | BIT(1) | BIT(2), + .clk_mask = BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(4) | BIT(5), + }, + [IMX8MM_DISPBLK_PD_LCDIF] = { + .name = "dispblk-lcdif", + .clk_names = (const char *[]){ "lcdif-axi", "lcdif-apb", "lcdif-pix", }, + .num_clks = 3, + .gpc_name = "lcdif", + .clk_mask = BIT(6) | BIT(7), + }, + [IMX8MM_DISPBLK_PD_MIPI_DSI] = { + .name = "dispblk-mipi-dsi", + .clk_names = (const char *[]){ "dsi-pclk", "dsi-ref", }, + .num_clks = 2, + .gpc_name = "mipi-dsi", + .rst_mask = BIT(5), + .clk_mask = BIT(8) | BIT(9), + .mipi_phy_rst_mask = BIT(17), + }, + [IMX8MM_DISPBLK_PD_MIPI_CSI] = { + .name = "dispblk-mipi-csi", + .clk_names = (const char *[]){ "csi-aclk", "csi-pclk" }, + .num_clks = 2, + .gpc_name = "mipi-csi", + .rst_mask = BIT(3) | BIT(4), + .clk_mask = BIT(10) | BIT(11), + .mipi_phy_rst_mask = BIT(16), + }, +}; + +static const struct imx8m_blk_ctrl_data imx8mm_disp_blk_ctl_dev_data = { + .max_reg = 0x2c, + .domains = imx8mm_disp_blk_ctl_domain_data, + .num_domains = ARRAY_SIZE(imx8mm_disp_blk_ctl_domain_data), + .bus_rst_mask = BIT(6), + .bus_clk_mask = BIT(12), +}; + +static const struct imx8m_blk_ctrl_domain_data imx8mn_disp_blk_ctl_domain_data[] = { + [IMX8MN_DISPBLK_PD_MIPI_DSI] = { + .name = "dispblk-mipi-dsi", + .clk_names = (const char *[]){ "dsi-pclk", "dsi-ref", }, + .num_clks = 2, + .gpc_name = "mipi-dsi", + .rst_mask = BIT(0) | BIT(1), + .clk_mask = BIT(0) | BIT(1), + .mipi_phy_rst_mask = BIT(17), + }, + [IMX8MN_DISPBLK_PD_MIPI_CSI] = { + .name = "dispblk-mipi-csi", + .clk_names = (const char *[]){ "csi-aclk", "csi-pclk" }, + .num_clks = 2, + .gpc_name = "mipi-csi", + .rst_mask = BIT(2) | BIT(3), + .clk_mask = BIT(2) | BIT(3), + .mipi_phy_rst_mask = BIT(16), + }, + [IMX8MN_DISPBLK_PD_LCDIF] = { + .name = "dispblk-lcdif", + .clk_names = (const char *[]){ "lcdif-axi", "lcdif-apb", "lcdif-pix", }, + .num_clks = 3, + .gpc_name = "lcdif", + .rst_mask = BIT(4) | BIT(5), + .clk_mask = BIT(4) | BIT(5), + }, + [IMX8MN_DISPBLK_PD_ISI] = { + .name = "dispblk-isi", + .clk_names = (const char *[]){ "disp_axi", "disp_apb", "disp_axi_root", + "disp_apb_root"}, + .num_clks = 4, + .gpc_name = "isi", + .rst_mask = BIT(6) | BIT(7), + .clk_mask = BIT(6) | BIT(7), + }, +}; + +static const struct imx8m_blk_ctrl_data imx8mn_disp_blk_ctl_dev_data = { + .max_reg = 0x84, + .domains = imx8mn_disp_blk_ctl_domain_data, + .num_domains = ARRAY_SIZE(imx8mn_disp_blk_ctl_domain_data), + .bus_rst_mask = BIT(8), + .bus_clk_mask = BIT(8), +}; + +static const struct imx8m_blk_ctrl_domain_data imx8mp_media_blk_ctl_domain_data[] = { + [IMX8MP_MEDIABLK_PD_MIPI_DSI_1] = { + .name = "mediablk-mipi-dsi-1", + .clk_names = (const char *[]){ "apb", "phy", }, + .num_clks = 2, + .gpc_name = "mipi-dsi1", + .rst_mask = BIT(0) | BIT(1), + .clk_mask = BIT(0) | BIT(1), + .mipi_phy_rst_mask = BIT(17), + }, + [IMX8MP_MEDIABLK_PD_MIPI_CSI2_1] = { + .name = "mediablk-mipi-csi2-1", + .clk_names = (const char *[]){ "apb", "cam1" }, + .num_clks = 2, + .gpc_name = "mipi-csi1", + .rst_mask = BIT(2) | BIT(3), + .clk_mask = BIT(2) | BIT(3), + .mipi_phy_rst_mask = BIT(16), + }, + [IMX8MP_MEDIABLK_PD_LCDIF_1] = { + .name = "mediablk-lcdif-1", + .clk_names = (const char *[]){ "disp1", "apb", "axi", }, + .num_clks = 3, + .gpc_name = "lcdif1", + .rst_mask = BIT(4) | BIT(5) | BIT(23), + .clk_mask = BIT(4) | BIT(5) | BIT(23), + }, + [IMX8MP_MEDIABLK_PD_ISI] = { + .name = "mediablk-isi", + .clk_names = (const char *[]){ "axi", "apb" }, + .num_clks = 2, + .gpc_name = "isi", + .rst_mask = BIT(6) | BIT(7), + .clk_mask = BIT(6) | BIT(7), + }, + [IMX8MP_MEDIABLK_PD_MIPI_CSI2_2] = { + .name = "mediablk-mipi-csi2-2", + .clk_names = (const char *[]){ "apb", "cam2" }, + .num_clks = 2, + .gpc_name = "mipi-csi2", + .rst_mask = BIT(9) | BIT(10), + .clk_mask = BIT(9) | BIT(10), + .mipi_phy_rst_mask = BIT(30), + }, + [IMX8MP_MEDIABLK_PD_LCDIF_2] = { + .name = "mediablk-lcdif-2", + .clk_names = (const char *[]){ "disp2", "apb", "axi", }, + .num_clks = 3, + .gpc_name = "lcdif2", + .rst_mask = BIT(11) | BIT(12) | BIT(24), + .clk_mask = BIT(11) | BIT(12) | BIT(24), + }, + [IMX8MP_MEDIABLK_PD_ISP] = { + .name = "mediablk-isp", + .clk_names = (const char *[]){ "isp", "axi", "apb" }, + .num_clks = 3, + .gpc_name = "isp", + .rst_mask = BIT(16) | BIT(17) | BIT(18), + .clk_mask = BIT(16) | BIT(17) | BIT(18), + }, + [IMX8MP_MEDIABLK_PD_DWE] = { + .name = "mediablk-dwe", + .clk_names = (const char *[]){ "axi", "apb" }, + .num_clks = 2, + .gpc_name = "dwe", + .rst_mask = BIT(19) | BIT(20) | BIT(21), + .clk_mask = BIT(19) | BIT(20) | BIT(21), + }, + [IMX8MP_MEDIABLK_PD_MIPI_DSI_2] = { + .name = "mediablk-mipi-dsi-2", + .clk_names = (const char *[]){ "phy", }, + .num_clks = 1, + .gpc_name = "mipi-dsi2", + .rst_mask = BIT(22), + .clk_mask = BIT(22), + .mipi_phy_rst_mask = BIT(29), + }, +}; + +static const struct imx8m_blk_ctrl_data imx8mp_media_blk_ctl_dev_data = { + .max_reg = 0x138, + .domains = imx8mp_media_blk_ctl_domain_data, + .num_domains = ARRAY_SIZE(imx8mp_media_blk_ctl_domain_data), + .bus_rst_mask = BIT(8), + .bus_clk_mask = BIT(8), +}; + +static const struct udevice_id imx8m_blk_ctrl_ids[] = { + {.compatible = "fsl,imx8mm-disp-blk-ctrl", .data = (ulong)&imx8mm_disp_blk_ctl_dev_data}, + {.compatible = "fsl,imx8mn-disp-blk-ctrl", .data = (ulong)&imx8mn_disp_blk_ctl_dev_data}, + {.compatible = "fsl,imx8mp-media-blk-ctrl", .data = (ulong)&imx8mp_media_blk_ctl_dev_data}, + {} +}; + +struct power_domain_ops imx8m_blk_ctrl_ops = { + .request = imx8m_blk_ctrl_request, + .rfree = imx8m_blk_ctrl_free, + .on = imx8m_blk_ctrl_power_on, + .off = imx8m_blk_ctrl_power_off, +}; + +U_BOOT_DRIVER(imx8m_blk_ctrl) = { + .name = "imx8m_blk_ctrl", + .id = UCLASS_POWER_DOMAIN, + .of_match = imx8m_blk_ctrl_ids, + .bind = dm_scan_fdt_dev, + .probe = imx8m_blk_ctrl_probe, + .remove = imx8m_blk_ctrl_remove, + .priv_auto = sizeof(struct imx8m_blk_ctrl), + .ops = &imx8m_blk_ctrl_ops, + .flags = DM_FLAG_DEFAULT_PD_CTRL_OFF, +}; diff --git a/drivers/power/domain/imx8m-power-domain.c b/drivers/power/domain/imx8m-power-domain.c index 8b6870c86463..40fec70d954a 100644 --- a/drivers/power/domain/imx8m-power-domain.c +++ b/drivers/power/domain/imx8m-power-domain.c @@ -32,17 +32,31 @@ DECLARE_GLOBAL_DATA_PTR; #define IMX8M_OTG1_A53_DOMAIN BIT(4) #define IMX8M_PCIE1_A53_DOMAIN BIT(3) +#define IMX8MM_VPUH1_A53_DOMAIN BIT(15) +#define IMX8MM_VPUG2_A53_DOMAIN BIT(14) +#define IMX8MM_VPUG1_A53_DOMAIN BIT(13) +#define IMX8MM_DISPMIX_A53_DOMAIN BIT(12) +#define IMX8MM_VPUMIX_A53_DOMAIN BIT(10) +#define IMX8MM_GPUMIX_A53_DOMAIN BIT(9) +#define IMX8MM_GPU_A53_DOMAIN (BIT(8) | BIT(11)) +#define IMX8MM_DDR1_A53_DOMAIN BIT(7) #define IMX8MM_OTG2_A53_DOMAIN BIT(5) #define IMX8MM_OTG1_A53_DOMAIN BIT(4) #define IMX8MM_PCIE_A53_DOMAIN BIT(3) +#define IMX8MM_MIPI_A53_DOMAIN BIT(2) +#define IMX8MN_DISPMIX_A53_DOMAIN BIT(12) +#define IMX8MN_GPUMIX_A53_DOMAIN BIT(9) +#define IMX8MN_DDR1_A53_DOMAIN BIT(7) #define IMX8MN_OTG1_A53_DOMAIN BIT(4) #define IMX8MN_MIPI_A53_DOMAIN BIT(2) #define IMX8MP_HSIOMIX_A53_DOMAIN BIT(19) +#define IMX8MP_MEDIAMIX_A53_DOMAIN BIT(12) #define IMX8MP_USB2_PHY_A53_DOMAIN BIT(5) #define IMX8MP_USB1_PHY_A53_DOMAIN BIT(4) #define IMX8MP_PCIE_PHY_A53_DOMAIN BIT(3) +#define IMX8MP_MIPI_PHY1_A53_DOMAIN BIT(2) #define IMX8MP_GPC_PU_PGC_SW_PUP_REQ 0x0d8 #define IMX8MP_GPC_PU_PGC_SW_PDN_REQ 0x0e4 @@ -50,35 +64,72 @@ DECLARE_GLOBAL_DATA_PTR; #define GPC_PU_PGC_SW_PUP_REQ 0x0f8 #define GPC_PU_PGC_SW_PDN_REQ 0x104 +#define IMX8M_PCIE2_SW_Pxx_REQ BIT(13) +#define IMX8M_MIPI_CSI2_SW_Pxx_REQ BIT(12) +#define IMX8M_MIPI_CSI1_SW_Pxx_REQ BIT(11) +#define IMX8M_DISP_SW_Pxx_REQ BIT(10) +#define IMX8M_HDMI_SW_Pxx_REQ BIT(9) +#define IMX8M_VPU_SW_Pxx_REQ BIT(8) +#define IMX8M_GPU_SW_Pxx_REQ BIT(7) +#define IMX8M_DDR2_SW_Pxx_REQ BIT(6) +#define IMX8M_DDR1_SW_Pxx_REQ BIT(5) #define IMX8M_PCIE2_SW_Pxx_REQ BIT(13) #define IMX8M_OTG2_SW_Pxx_REQ BIT(3) #define IMX8M_OTG1_SW_Pxx_REQ BIT(2) #define IMX8M_PCIE1_SW_Pxx_REQ BIT(1) +#define IMX8MM_VPUH1_SW_Pxx_REQ BIT(13) +#define IMX8MM_VPUG2_SW_Pxx_REQ BIT(12) +#define IMX8MM_VPUG1_SW_Pxx_REQ BIT(11) +#define IMX8MM_DISPMIX_SW_Pxx_REQ BIT(10) +#define IMX8MM_VPUMIX_SW_Pxx_REQ BIT(8) +#define IMX8MM_GPUMIX_SW_Pxx_REQ BIT(7) +#define IMX8MM_GPU_SW_Pxx_REQ (BIT(6) | BIT(9)) +#define IMX8MM_DDR1_SW_Pxx_REQ BIT(5) #define IMX8MM_OTG2_SW_Pxx_REQ BIT(3) #define IMX8MM_OTG1_SW_Pxx_REQ BIT(2) #define IMX8MM_PCIE_SW_Pxx_REQ BIT(1) +#define IMX8MM_MIPI_SW_Pxx_REQ BIT(0) +#define IMX8MN_DISPMIX_SW_Pxx_REQ BIT(10) +#define IMX8MN_GPUMIX_SW_Pxx_REQ BIT(7) +#define IMX8MN_DDR1_SW_Pxx_REQ BIT(5) #define IMX8MN_OTG1_SW_Pxx_REQ BIT(2) #define IMX8MN_MIPI_SW_Pxx_REQ BIT(0) #define IMX8MP_HSIOMIX_Pxx_REQ BIT(17) +#define IMX8MP_MEDIMIX_Pxx_REQ BIT(10) #define IMX8MP_USB2_PHY_Pxx_REQ BIT(3) #define IMX8MP_USB1_PHY_Pxx_REQ BIT(2) #define IMX8MP_PCIE_PHY_SW_Pxx_REQ BIT(1) +#define IMX8MP_MIPI_PHY1_SW_Pxx_REQ BIT(0) #define GPC_M4_PU_PDN_FLG 0x1bc #define IMX8MP_GPC_PU_PWRHSK 0x190 #define GPC_PU_PWRHSK 0x1fc +#define IMX8MM_GPUMIX_HSK_PWRDNACKN BIT(29) +#define IMX8MM_GPU_HSK_PWRDNACKN (BIT(27) | BIT(28)) +#define IMX8MM_VPUMIX_HSK_PWRDNACKN BIT(26) +#define IMX8MM_DISPMIX_HSK_PWRDNACKN BIT(25) #define IMX8MM_HSIO_HSK_PWRDNACKN (BIT(23) | BIT(24)) +#define IMX8MM_GPUMIX_HSK_PWRDNREQN BIT(11) +#define IMX8MM_GPU_HSK_PWRDNREQN (BIT(9) | BIT(10)) +#define IMX8MM_VPUMIX_HSK_PWRDNREQN BIT(8) +#define IMX8MM_DISPMIX_HSK_PWRDNREQN BIT(7) #define IMX8MM_HSIO_HSK_PWRDNREQN (BIT(5) | BIT(6)) +#define IMX8MN_GPUMIX_HSK_PWRDNACKN (BIT(29) | BIT(27)) +#define IMX8MN_DISPMIX_HSK_PWRDNACKN BIT(25) #define IMX8MN_HSIO_HSK_PWRDNACKN BIT(23) +#define IMX8MN_GPUMIX_HSK_PWRDNREQN (BIT(11) | BIT(9)) +#define IMX8MN_DISPMIX_HSK_PWRDNREQN BIT(7) #define IMX8MN_HSIO_HSK_PWRDNREQN BIT(5) +#define IMX8MP_MEDIAMIX_PWRDNACKN BIT(30) #define IMX8MP_HSIOMIX_PWRDNACKN BIT(28) +#define IMX8MP_MEDIAMIX_PWRDNREQN BIT(14) #define IMX8MP_HSIOMIX_PWRDNREQN BIT(12) /* @@ -92,15 +143,31 @@ DECLARE_GLOBAL_DATA_PTR; #define IMX8M_PGC_OTG2 19 #define IMX8M_PGC_PCIE2 29 +#define IMX8MM_PGC_MIPI 16 #define IMX8MM_PGC_PCIE 17 #define IMX8MM_PGC_OTG1 18 #define IMX8MM_PGC_OTG2 19 - +#define IMX8MM_PGC_DDR1 21 +#define IMX8MM_PGC_GPU2D 22 +#define IMX8MM_PGC_GPUMIX 23 +#define IMX8MM_PGC_VPUMIX 24 +#define IMX8MM_PGC_GPU3D 25 +#define IMX8MM_PGC_DISPMIX 26 +#define IMX8MM_PGC_VPUG1 27 +#define IMX8MM_PGC_VPUG2 28 +#define IMX8MM_PGC_VPUH1 29 + +#define IMX8MN_PGC_MIPI 16 #define IMX8MN_PGC_OTG1 18 +#define IMX8MN_PGC_DDR1 21 +#define IMX8MN_PGC_GPUMIX 23 +#define IMX8MN_PGC_DISPMIX 26 +#define IMX8MP_PGC_MIPI1 12 #define IMX8MP_PGC_PCIE 13 #define IMX8MP_PGC_USB1 14 #define IMX8MP_PGC_USB2 15 +#define IMX8MP_PGC_MEDIAMIX 22 #define IMX8MP_PGC_HSIOMIX 29 #define GPC_PGC_CTRL(n) (0x800 + (n) * 0x40) @@ -142,6 +209,7 @@ struct imx8m_power_domain_plat { void __iomem *base; int resource_id; int has_pd; + int count; }; #if defined(CONFIG_IMX8MM) || defined(CONFIG_IMX8MN) || defined(CONFIG_IMX8MQ) @@ -230,6 +298,82 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = { }, .pgc = BIT(IMX8MM_PGC_OTG2), }, + + [IMX8MM_POWER_DOMAIN_GPUMIX] = { + .bits = { + .pxx = IMX8MM_GPUMIX_SW_Pxx_REQ, + .map = IMX8MM_GPUMIX_A53_DOMAIN, + .hskreq = IMX8MM_GPUMIX_HSK_PWRDNREQN, + .hskack = IMX8MM_GPUMIX_HSK_PWRDNACKN, + }, + .pgc = BIT(IMX8MM_PGC_GPUMIX), + .keep_clocks = true, + }, + + [IMX8MM_POWER_DOMAIN_GPU] = { + .bits = { + .pxx = IMX8MM_GPU_SW_Pxx_REQ, + .map = IMX8MM_GPU_A53_DOMAIN, + .hskreq = IMX8MM_GPU_HSK_PWRDNREQN, + .hskack = IMX8MM_GPU_HSK_PWRDNACKN, + }, + .pgc = BIT(IMX8MM_PGC_GPU2D) | BIT(IMX8MM_PGC_GPU3D), + }, + + [IMX8MM_POWER_DOMAIN_VPUMIX] = { + .bits = { + .pxx = IMX8MM_VPUMIX_SW_Pxx_REQ, + .map = IMX8MM_VPUMIX_A53_DOMAIN, + .hskreq = IMX8MM_VPUMIX_HSK_PWRDNREQN, + .hskack = IMX8MM_VPUMIX_HSK_PWRDNACKN, + }, + .pgc = BIT(IMX8MM_PGC_VPUMIX), + .keep_clocks = true, + }, + + [IMX8MM_POWER_DOMAIN_VPUG1] = { + .bits = { + .pxx = IMX8MM_VPUG1_SW_Pxx_REQ, + .map = IMX8MM_VPUG1_A53_DOMAIN, + }, + .pgc = BIT(IMX8MM_PGC_VPUG1), + }, + + [IMX8MM_POWER_DOMAIN_VPUG2] = { + .bits = { + .pxx = IMX8MM_VPUG2_SW_Pxx_REQ, + .map = IMX8MM_VPUG2_A53_DOMAIN, + }, + .pgc = BIT(IMX8MM_PGC_VPUG2), + }, + + [IMX8MM_POWER_DOMAIN_VPUH1] = { + .bits = { + .pxx = IMX8MM_VPUH1_SW_Pxx_REQ, + .map = IMX8MM_VPUH1_A53_DOMAIN, + }, + .pgc = BIT(IMX8MM_PGC_VPUH1), + .keep_clocks = true, + }, + + [IMX8MM_POWER_DOMAIN_DISPMIX] = { + .bits = { + .pxx = IMX8MM_DISPMIX_SW_Pxx_REQ, + .map = IMX8MM_DISPMIX_A53_DOMAIN, + .hskreq = IMX8MM_DISPMIX_HSK_PWRDNREQN, + .hskack = IMX8MM_DISPMIX_HSK_PWRDNACKN, + }, + .pgc = BIT(IMX8MM_PGC_DISPMIX), + .keep_clocks = true, + }, + + [IMX8MM_POWER_DOMAIN_MIPI] = { + .bits = { + .pxx = IMX8MM_MIPI_SW_Pxx_REQ, + .map = IMX8MM_MIPI_A53_DOMAIN, + }, + .pgc = BIT(IMX8MM_PGC_MIPI), + }, }; static const struct imx_pgc_domain_data imx8mm_pgc_domain_data = { @@ -258,6 +402,36 @@ static const struct imx_pgc_domain imx8mn_pgc_domains[] = { }, .pgc = BIT(IMX8MN_PGC_OTG1), }, + + [IMX8MN_POWER_DOMAIN_GPUMIX] = { + .bits = { + .pxx = IMX8MN_GPUMIX_SW_Pxx_REQ, + .map = IMX8MN_GPUMIX_A53_DOMAIN, + .hskreq = IMX8MN_GPUMIX_HSK_PWRDNREQN, + .hskack = IMX8MN_GPUMIX_HSK_PWRDNACKN, + }, + .pgc = BIT(IMX8MN_PGC_GPUMIX), + .keep_clocks = true, + }, + + [IMX8MN_POWER_DOMAIN_DISPMIX] = { + .bits = { + .pxx = IMX8MN_DISPMIX_SW_Pxx_REQ, + .map = IMX8MN_DISPMIX_A53_DOMAIN, + .hskreq = IMX8MN_DISPMIX_HSK_PWRDNREQN, + .hskack = IMX8MN_DISPMIX_HSK_PWRDNACKN, + }, + .pgc = BIT(IMX8MN_PGC_DISPMIX), + .keep_clocks = true, + }, + + [IMX8MN_POWER_DOMAIN_MIPI] = { + .bits = { + .pxx = IMX8MN_MIPI_SW_Pxx_REQ, + .map = IMX8MN_MIPI_A53_DOMAIN, + }, + .pgc = BIT(IMX8MN_PGC_MIPI), + }, }; static const struct imx_pgc_domain_data imx8mn_pgc_domain_data = { @@ -268,7 +442,15 @@ static const struct imx_pgc_domain_data imx8mn_pgc_domain_data = { #endif #ifdef CONFIG_IMX8MP -static const struct imx_pgc_domain imx8mp_pgc_domains[] = { +static const struct imx_pgc_domain imx8mp_pgc_domains[19] = { + [IMX8MP_POWER_DOMAIN_MIPI_PHY1] = { + .bits = { + .pxx = IMX8MP_MIPI_PHY1_SW_Pxx_REQ, + .map = IMX8MP_MIPI_PHY1_A53_DOMAIN, + }, + .pgc = BIT(IMX8MP_PGC_MIPI1), + }, + [IMX8MP_POWER_DOMAIN_PCIE_PHY] = { .bits = { .pxx = IMX8MP_PCIE_PHY_SW_Pxx_REQ, @@ -293,6 +475,17 @@ static const struct imx_pgc_domain imx8mp_pgc_domains[] = { .pgc = BIT(IMX8MP_PGC_USB2), }, + [IMX8MP_POWER_DOMAIN_MEDIAMIX] = { + .bits = { + .pxx = IMX8MP_MEDIMIX_Pxx_REQ, + .map = IMX8MP_MEDIAMIX_A53_DOMAIN, + .hskreq = IMX8MP_MEDIAMIX_PWRDNREQN, + .hskack = IMX8MP_MEDIAMIX_PWRDNACKN, + }, + .pgc = BIT(IMX8MP_PGC_MEDIAMIX), + .keep_clocks = true, + }, + [IMX8MP_POWER_DOMAIN_HSIOMIX] = { .bits = { .pxx = IMX8MP_HSIOMIX_Pxx_REQ, @@ -329,6 +522,11 @@ static int imx8m_power_domain_on(struct power_domain *power_domain) u32 pgc; int ret; + if (pdata->count > 0) { /* Already on */ + pdata->count++; + return 0; + } + if (pdata->clk.count) { ret = clk_enable_bulk(&pdata->clk); if (ret) { @@ -373,6 +571,8 @@ static int imx8m_power_domain_on(struct power_domain *power_domain) if (!domain->keep_clocks && pdata->clk.count) clk_disable_bulk(&pdata->clk); + pdata->count++; + return 0; out_clk_disable: @@ -391,6 +591,13 @@ static int imx8m_power_domain_off(struct power_domain *power_domain) u32 pgc; int ret; + if (!pdata->count) { /* Already off */ + return 0; + } else if (pdata->count > 1) { + pdata->count--; + return 0; + } + /* Enable reset clocks for all devices in the domain */ if (!domain->keep_clocks && pdata->clk.count) { ret = clk_enable_bulk(&pdata->clk); @@ -439,6 +646,8 @@ static int imx8m_power_domain_off(struct power_domain *power_domain) if (pdata->has_pd) power_domain_off(&pdata->pd); + pdata->count--; + return 0; out_clk_disable: