On Fri, Nov 5, 2021 at 9:01 AM Lucas Stach [off-list ref] wrote:

Hi Adam,

Am Freitag, dem 05.11.2021 um 08:42 -0500 schrieb Adam Ford:

quoted

Most of the blk-ctrl reset bits are found in one register, however
there are two bits in offset 8 for pulling the MIPI DPHY out of reset
and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
out of reset or the MIPI_CSI hangs.

I'm undecided yet if I like this solution, as register 0x8 has
different content on different instances of the blk-ctrl, so naming it
mipi_rst_mask is confusing for others.

 Both Mini and Nano use it, so I could add a comment that this
register only applies to Mini and Nano.  For all others, it would be
empty and do nothing anyway.  If you have a name you'd prefer, I can
do that.

My initial thought was that we should habe a MIPI PHY driver
controlling all the registers in the disp-blk-ctrl, other than SFT_RSTN
and CLK_EN, however I see how the reset handling for the PHY would then
be inconsistent with how we handle all the other devices in the disp-
blk domain. But then the HW guys seemed to think along the same lines,
as they placed the PHY resets into the PHY registers, instead of the
SFT_RSTN, which had more than enough spare bits to carry those
resets...

The NXP ATF code sets these bits in the GPC code with the other power
domain stuff, so it seems like they intended it to be used as a power
domain thing and not MIPI PHY driver.

quoted

Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
Signed-off-by: Adam Ford <redacted>
---
 drivers/soc/imx/imx8m-blk-ctrl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
index 519b3651d1d9..5506bd075c35 100644
--- a/drivers/soc/imx/imx8m-blk-ctrl.c
+++ b/drivers/soc/imx/imx8m-blk-ctrl.c

@@ -17,6 +17,7 @@

 #define BLK_SFT_RSTN 0x0
 #define BLK_CLK_EN   0x4
+#define BLK_MIPI_RESET_DIV   0x8

 struct imx8m_blk_ctrl_domain;

@@ -36,6 +37,7 @@ struct imx8m_blk_ctrl_domain_data {
      const char *gpc_name;
      u32 rst_mask;
      u32 clk_mask;
+     u32 mipi_rst_mask;
 };

 #define DOMAIN_MAX_CLKS 3

@@ -78,6 +80,7 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)

      /* put devices into reset */
      regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
+     regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_rst_mask);

      /* enable upstream and blk-ctrl clocks to allow reset to propagate */
      ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);

@@ -99,6 +102,7 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)

      /* release reset */
      regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
+     regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_rst_mask);

      /* disable upstream clocks */
      clk_bulk_disable_unprepare(data->num_clks, domain->clks);

@@ -122,6 +126,7 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
      /* put devices into reset and disable clocks */
      regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
      regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
+     regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_rst_mask);

For consistency the reset assertion should be put before the clock
disable.

I can do that.  How do you want me to proceed?

adam

Regards,
Lucas

quoted

      /* power down upstream GPC domain */
      pm_runtime_put(domain->power_dev);

@@ -488,6 +493,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
              .gpc_name = "mipi-csi",
              .rst_mask = BIT(3) | BIT(4),
              .clk_mask = BIT(10) | BIT(11),
+             .mipi_rst_mask = BIT(16) | BIT(17),
      },
 };

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel