Hi Heko
On Tue, Oct 1, 2024 at 6:23 AM Heiko Schocher <h...@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 <h...@denx.de> wrote:
> >>
> >> Hello Dario,
> >>
> >> On 13.09.24 11:55, Dario Binacchi wrote:
> >>> From: Michael Trimarchi <mich...@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...@nxp.com>
> >>> Signed-off-by: Michael Trimarchi <mich...@amarulasolutions.com>
> >>> Signed-off-by: Dario Binacchi <dario.binac...@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: h...@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: h...@denx.de
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
mich...@amarulasolutions.com
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
i...@amarulasolutions.com
www.amarulasolutions.com
