| Message ID | 20240226080433.3307154-3-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | imx8mp: Enable PCIe/NVMe support | expand |
On 2/26/24 9:04 AM, Sumit Garg wrote: > Add support for i.MX8MP reset controller, it has same reset IP Same as what ? > inside > but with different module layout. > > Inspired from counterpart Linux kernel v6.8-rc3 driver: > drivers/reset/reset-imx7.c. Use commit ID of the last change in that driver: bad8a8afe19f ("reset: Explicitly include correct DT includes") I guess
On 2/26/24 9:04 AM, Sumit Garg wrote:
[...]
> +static int imx7_reset_assert_imx8mp(struct reset_ctl *rst)
Linux calls those imx8mp_reset_set() can co. which is less confusing
than imx7...imx8mp() , use it.
In fact, why not copy the code from Linux outright ?
On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex@denx.de> wrote: > > On 2/26/24 9:04 AM, Sumit Garg wrote: > > Add support for i.MX8MP reset controller, it has same reset IP > > Same as what ? I can expand it. > > > inside > > but with different module layout. > > > > Inspired from counterpart Linux kernel v6.8-rc3 driver: > > drivers/reset/reset-imx7.c. > > Use commit ID of the last change in that driver: > > bad8a8afe19f ("reset: Explicitly include correct DT includes") > > I guess Why isn't the Linux kernel tag sufficient here? I don't see the value of cross referencing commits across different git projects. -Sumit
On 2/26/24 11:33 AM, Sumit Garg wrote: > On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex@denx.de> wrote: >> >> On 2/26/24 9:04 AM, Sumit Garg wrote: >>> Add support for i.MX8MP reset controller, it has same reset IP >> >> Same as what ? > > I can expand it. This does not answer the question . >>> inside >>> but with different module layout. >>> >>> Inspired from counterpart Linux kernel v6.8-rc3 driver: >>> drivers/reset/reset-imx7.c. >> >> Use commit ID of the last change in that driver: >> >> bad8a8afe19f ("reset: Explicitly include correct DT includes") >> >> I guess > > Why isn't the Linux kernel tag sufficient here? I don't see the value > of cross referencing commits across different git projects. The commit ID is a unique identifier, tag can be rewritten .
On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex@denx.de> wrote: > > On 2/26/24 9:04 AM, Sumit Garg wrote: > > [...] > > > +static int imx7_reset_assert_imx8mp(struct reset_ctl *rst) > > Linux calls those imx8mp_reset_set() can co. which is less confusing > than imx7...imx8mp() , use it. > IMO, it would be more confusing if we choose two different naming patterns for the same driver, see existing function names imx7_reset_{deassert/assert}_imx* pattern. > In fact, why not copy the code from Linux outright ? I suppose the question here is to not convert the driver to use regmap, right? If it is then that should be done as a separate cleanup patch. However, I can extract common bits as imx8mp_reset_set() for better code reuse as the Linux kernel driver does. -Sumit
On 2/26/24 11:43 AM, Sumit Garg wrote: > On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex@denx.de> wrote: >> >> On 2/26/24 9:04 AM, Sumit Garg wrote: >> >> [...] >> >>> +static int imx7_reset_assert_imx8mp(struct reset_ctl *rst) >> >> Linux calls those imx8mp_reset_set() can co. which is less confusing >> than imx7...imx8mp() , use it. >> > > IMO, it would be more confusing if we choose two different naming > patterns for the same driver, see existing function names > imx7_reset_{deassert/assert}_imx* pattern. Maybe create a patch which updates the naming scheme ? Using two SoC names in one function name is confusing I think . >> In fact, why not copy the code from Linux outright ? > > I suppose the question here is to not convert the driver to use > regmap, right? If it is then that should be done as a separate cleanup > patch. If it isn't too hard, please do the clean up first and then add the MX8MP bits on top. [...]
On Mon, 26 Feb 2024 at 16:11, Marek Vasut <marex@denx.de> wrote: > > On 2/26/24 11:33 AM, Sumit Garg wrote: > > On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex@denx.de> wrote: > >> > >> On 2/26/24 9:04 AM, Sumit Garg wrote: > >>> Add support for i.MX8MP reset controller, it has same reset IP > >> > >> Same as what ? > > > > I can expand it. > > This does not answer the question . > The other iMX7 and iMX8 variants. > >>> inside > >>> but with different module layout. > >>> > >>> Inspired from counterpart Linux kernel v6.8-rc3 driver: > >>> drivers/reset/reset-imx7.c. > >> > >> Use commit ID of the last change in that driver: > >> > >> bad8a8afe19f ("reset: Explicitly include correct DT includes") > >> > >> I guess > > > > Why isn't the Linux kernel tag sufficient here? I don't see the value > > of cross referencing commits across different git projects. > > The commit ID is a unique identifier, tag can be rewritten . Similar arguments can be made for git commit history. But the question is have you come across such instances for Linux kernel where git release/RC tags have been rewritten? -Sumit
On Mon, 26 Feb 2024 at 16:26, Marek Vasut <marex@denx.de> wrote: > > On 2/26/24 11:43 AM, Sumit Garg wrote: > > On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex@denx.de> wrote: > >> > >> On 2/26/24 9:04 AM, Sumit Garg wrote: > >> > >> [...] > >> > >>> +static int imx7_reset_assert_imx8mp(struct reset_ctl *rst) > >> > >> Linux calls those imx8mp_reset_set() can co. which is less confusing > >> than imx7...imx8mp() , use it. > >> > > > > IMO, it would be more confusing if we choose two different naming > > patterns for the same driver, see existing function names > > imx7_reset_{deassert/assert}_imx* pattern. > > Maybe create a patch which updates the naming scheme ? I can do that as a preparatory patch. > > Using two SoC names in one function name is confusing I think . > > >> In fact, why not copy the code from Linux outright ? > > > > I suppose the question here is to not convert the driver to use > > regmap, right? If it is then that should be done as a separate cleanup > > patch. > > If it isn't too hard, please do the clean up first and then add the > MX8MP bits on top. The coding part is easy but it is rather the testing part which I am worried about as I don't have access to all the platforms which rely on this driver. -Sumit > > [...]
On 2/26/24 1:59 PM, Sumit Garg wrote: > On Mon, 26 Feb 2024 at 16:11, Marek Vasut <marex@denx.de> wrote: >> >> On 2/26/24 11:33 AM, Sumit Garg wrote: >>> On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 2/26/24 9:04 AM, Sumit Garg wrote: >>>>> Add support for i.MX8MP reset controller, it has same reset IP >>>> >>>> Same as what ? >>> >>> I can expand it. >> >> This does not answer the question . >> > > The other iMX7 and iMX8 variants. Please add that to the commit message. >>>>> inside >>>>> but with different module layout. >>>>> >>>>> Inspired from counterpart Linux kernel v6.8-rc3 driver: >>>>> drivers/reset/reset-imx7.c. >>>> >>>> Use commit ID of the last change in that driver: >>>> >>>> bad8a8afe19f ("reset: Explicitly include correct DT includes") >>>> >>>> I guess >>> >>> Why isn't the Linux kernel tag sufficient here? I don't see the value >>> of cross referencing commits across different git projects. >> >> The commit ID is a unique identifier, tag can be rewritten . > > Similar arguments can be made for git commit history. If you do rewrite history, then the commit IDs would have changed, right ? The original commit ID would still describe the exact (old) state, the new rewritten history would have new commit IDs which describe the exact (new) state.
On 2/26/24 2:03 PM, Sumit Garg wrote: > On Mon, 26 Feb 2024 at 16:26, Marek Vasut <marex@denx.de> wrote: >> >> On 2/26/24 11:43 AM, Sumit Garg wrote: >>> On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 2/26/24 9:04 AM, Sumit Garg wrote: >>>> >>>> [...] >>>> >>>>> +static int imx7_reset_assert_imx8mp(struct reset_ctl *rst) >>>> >>>> Linux calls those imx8mp_reset_set() can co. which is less confusing >>>> than imx7...imx8mp() , use it. >>>> >>> >>> IMO, it would be more confusing if we choose two different naming >>> patterns for the same driver, see existing function names >>> imx7_reset_{deassert/assert}_imx* pattern. >> >> Maybe create a patch which updates the naming scheme ? > > I can do that as a preparatory patch. > >> >> Using two SoC names in one function name is confusing I think . >> >>>> In fact, why not copy the code from Linux outright ? >>> >>> I suppose the question here is to not convert the driver to use >>> regmap, right? If it is then that should be done as a separate cleanup >>> patch. >> >> If it isn't too hard, please do the clean up first and then add the >> MX8MP bits on top. > > The coding part is easy but it is rather the testing part which I am > worried about as I don't have access to all the platforms which rely > on this driver. Don't worry about that part, I'm reasonably sure if something breaks, it will be spotted sooner rather than later.
diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c index eaef2cc2cdf4..c1de84dea8ba 100644 --- a/drivers/reset/reset-imx7.c +++ b/drivers/reset/reset-imx7.c @@ -10,6 +10,7 @@ #include <dm.h> #include <dt-bindings/reset/imx7-reset.h> #include <dt-bindings/reset/imx8mq-reset.h> +#include <dt-bindings/reset/imx8mp-reset.h> #include <reset-uclass.h> #include <linux/bitops.h> #include <linux/delay.h> @@ -252,6 +253,115 @@ static int imx7_reset_assert_imx8mq(struct reset_ctl *rst) return 0; } +enum imx8mp_src_registers { + SRC_SUPERMIX_RCR = 0x0018, + SRC_AUDIOMIX_RCR = 0x001c, + SRC_MLMIX_RCR = 0x0028, + SRC_GPU2D_RCR = 0x0038, + SRC_GPU3D_RCR = 0x003c, + SRC_VPU_G1_RCR = 0x0048, + SRC_VPU_G2_RCR = 0x004c, + SRC_VPUVC8KE_RCR = 0x0050, + SRC_NOC_RCR = 0x0054, +}; + +static const struct imx7_src_signal imx8mp_src_signals[IMX8MP_RESET_NUM] = { + [IMX8MP_RESET_A53_CORE_POR_RESET0] = { SRC_A53RCR0, BIT(0) }, + [IMX8MP_RESET_A53_CORE_POR_RESET1] = { SRC_A53RCR0, BIT(1) }, + [IMX8MP_RESET_A53_CORE_POR_RESET2] = { SRC_A53RCR0, BIT(2) }, + [IMX8MP_RESET_A53_CORE_POR_RESET3] = { SRC_A53RCR0, BIT(3) }, + [IMX8MP_RESET_A53_CORE_RESET0] = { SRC_A53RCR0, BIT(4) }, + [IMX8MP_RESET_A53_CORE_RESET1] = { SRC_A53RCR0, BIT(5) }, + [IMX8MP_RESET_A53_CORE_RESET2] = { SRC_A53RCR0, BIT(6) }, + [IMX8MP_RESET_A53_CORE_RESET3] = { SRC_A53RCR0, BIT(7) }, + [IMX8MP_RESET_A53_DBG_RESET0] = { SRC_A53RCR0, BIT(8) }, + [IMX8MP_RESET_A53_DBG_RESET1] = { SRC_A53RCR0, BIT(9) }, + [IMX8MP_RESET_A53_DBG_RESET2] = { SRC_A53RCR0, BIT(10) }, + [IMX8MP_RESET_A53_DBG_RESET3] = { SRC_A53RCR0, BIT(11) }, + [IMX8MP_RESET_A53_ETM_RESET0] = { SRC_A53RCR0, BIT(12) }, + [IMX8MP_RESET_A53_ETM_RESET1] = { SRC_A53RCR0, BIT(13) }, + [IMX8MP_RESET_A53_ETM_RESET2] = { SRC_A53RCR0, BIT(14) }, + [IMX8MP_RESET_A53_ETM_RESET3] = { SRC_A53RCR0, BIT(15) }, + [IMX8MP_RESET_A53_SOC_DBG_RESET] = { SRC_A53RCR0, BIT(20) }, + [IMX8MP_RESET_A53_L2RESET] = { SRC_A53RCR0, BIT(21) }, + [IMX8MP_RESET_SW_NON_SCLR_M7C_RST] = { SRC_M4RCR, BIT(0) }, + [IMX8MP_RESET_OTG1_PHY_RESET] = { SRC_USBOPHY1_RCR, BIT(0) }, + [IMX8MP_RESET_OTG2_PHY_RESET] = { SRC_USBOPHY2_RCR, BIT(0) }, + [IMX8MP_RESET_SUPERMIX_RESET] = { SRC_SUPERMIX_RCR, BIT(0) }, + [IMX8MP_RESET_AUDIOMIX_RESET] = { SRC_AUDIOMIX_RCR, BIT(0) }, + [IMX8MP_RESET_MLMIX_RESET] = { SRC_MLMIX_RCR, BIT(0) }, + [IMX8MP_RESET_PCIEPHY] = { SRC_PCIEPHY_RCR, BIT(2) }, + [IMX8MP_RESET_PCIEPHY_PERST] = { SRC_PCIEPHY_RCR, BIT(3) }, + [IMX8MP_RESET_PCIE_CTRL_APPS_EN] = { SRC_PCIEPHY_RCR, BIT(6) }, + [IMX8MP_RESET_PCIE_CTRL_APPS_TURNOFF] = { SRC_PCIEPHY_RCR, BIT(11) }, + [IMX8MP_RESET_HDMI_PHY_APB_RESET] = { SRC_HDMI_RCR, BIT(0) }, + [IMX8MP_RESET_MEDIA_RESET] = { SRC_DISP_RCR, BIT(0) }, + [IMX8MP_RESET_GPU2D_RESET] = { SRC_GPU2D_RCR, BIT(0) }, + [IMX8MP_RESET_GPU3D_RESET] = { SRC_GPU3D_RCR, BIT(0) }, + [IMX8MP_RESET_GPU_RESET] = { SRC_GPU_RCR, BIT(0) }, + [IMX8MP_RESET_VPU_RESET] = { SRC_VPU_RCR, BIT(0) }, + [IMX8MP_RESET_VPU_G1_RESET] = { SRC_VPU_G1_RCR, BIT(0) }, + [IMX8MP_RESET_VPU_G2_RESET] = { SRC_VPU_G2_RCR, BIT(0) }, + [IMX8MP_RESET_VPUVC8KE_RESET] = { SRC_VPUVC8KE_RCR, BIT(0) }, + [IMX8MP_RESET_NOC_RESET] = { SRC_NOC_RCR, BIT(0) }, +}; + +static int imx7_reset_assert_imx8mp(struct reset_ctl *rst) +{ + struct imx7_reset_priv *priv = dev_get_priv(rst->dev); + const struct imx7_src_signal *sig = imx8mp_src_signals; + u32 val; + + if (rst->id >= IMX8MP_RESET_NUM) + return -EINVAL; + + val = readl(priv->base + sig[rst->id].offset); + switch (rst->id) { + case IMX8MP_RESET_PCIE_CTRL_APPS_EN: + case IMX8MP_RESET_PCIEPHY_PERST: + val &= ~sig[rst->id].bit; + break; + default: + val |= sig[rst->id].bit; + break; + } + writel(val, priv->base + sig[rst->id].offset); + + return 0; +} + +static int imx7_reset_deassert_imx8mp(struct reset_ctl *rst) +{ + struct imx7_reset_priv *priv = dev_get_priv(rst->dev); + const struct imx7_src_signal *sig = imx8mp_src_signals; + u32 val; + + if (rst->id >= IMX8MP_RESET_NUM) + return -EINVAL; + + if (rst->id == IMX8MP_RESET_PCIEPHY) { + /* + * wait for more than 10us to release phy g_rst and + * btnrst + */ + udelay(10); + } + + val = readl(priv->base + sig[rst->id].offset); + switch (rst->id) { + case IMX8MP_RESET_PCIE_CTRL_APPS_EN: + case IMX8MP_RESET_PCIEPHY_PERST: + val |= sig[rst->id].bit; + break; + default: + val &= ~sig[rst->id].bit; + break; + } + writel(val, priv->base + sig[rst->id].offset); + + return 0; +} + static int imx7_reset_assert(struct reset_ctl *rst) { struct imx7_reset_priv *priv = dev_get_priv(rst->dev); @@ -272,6 +382,7 @@ static const struct reset_ops imx7_reset_reset_ops = { static const struct udevice_id imx7_reset_ids[] = { { .compatible = "fsl,imx7d-src" }, { .compatible = "fsl,imx8mq-src" }, + { .compatible = "fsl,imx8mp-src" }, { } }; @@ -289,6 +400,9 @@ static int imx7_reset_probe(struct udevice *dev) } else if (device_is_compatible(dev, "fsl,imx7d-src")) { priv->ops.rst_assert = imx7_reset_assert_imx7; priv->ops.rst_deassert = imx7_reset_deassert_imx7; + } else if (device_is_compatible(dev, "fsl,imx8mp-src")) { + priv->ops.rst_assert = imx7_reset_assert_imx8mp; + priv->ops.rst_deassert = imx7_reset_deassert_imx8mp; } return 0;
Add support for i.MX8MP reset controller, it has same reset IP inside but with different module layout. Inspired from counterpart Linux kernel v6.8-rc3 driver: drivers/reset/reset-imx7.c. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- drivers/reset/reset-imx7.c | 114 +++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+)