| Message ID | 20210714121116.v2.6.I15e2419141a69b2e5c7e700c34d92a69df47e04d@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support to the mmsys driver to be a reset controller | expand |
Hi Enric, On Wed, 2021-07-14 at 12:11 +0200, Enric Balletbo i Serra wrote: > Among other features the mmsys driver should implement a reset > controller to be able to reset different bits from their space. > > Cc: Jitao Shi <jitao.shi@mediatek.com> > Suggested-by: Chun-Kuang Hu <chunkuang.hu@kernel.org> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> The reset controller driver looks fine, just two questions below. > --- > > (no changes since v1) > > drivers/soc/mediatek/mtk-mmsys.c | 69 ++++++++++++++++++++++++++++++++ > drivers/soc/mediatek/mtk-mmsys.h | 2 + > 2 files changed, 71 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c > index e681029fe804..6ac4deff0164 100644 > --- a/drivers/soc/mediatek/mtk-mmsys.c > +++ b/drivers/soc/mediatek/mtk-mmsys.c [...] > @@ -91,6 +95,59 @@ void mtk_mmsys_ddp_disconnect(struct device *dev, [...] > +static int mtk_mmsys_reset(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + int ret; > + > + ret = mtk_mmsys_reset_assert(rcdev, id); > + if (ret) > + return ret; > + > + usleep_range(1000, 1100); Is this known to be enough for all IP cores that can be reset by this controller? > + return mtk_mmsys_reset_deassert(rcdev, id); > +} > + > +static const struct reset_control_ops mtk_mmsys_reset_ops = { > + .assert = mtk_mmsys_reset_assert, > + .deassert = mtk_mmsys_reset_deassert, > + .reset = mtk_mmsys_reset, > +}; > + > static int mtk_mmsys_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -111,6 +168,18 @@ static int mtk_mmsys_probe(struct platform_device *pdev) > return ret; > } > > + spin_lock_init(&mmsys->lock); > + > + mmsys->rcdev.owner = THIS_MODULE; > + mmsys->rcdev.nr_resets = 32; Are all bits in the MMSYS_SW0_RST_B register individual reset controls? regards Philipp
Hi Philipp, Thank you to take a look On 20/7/21 12:52, Philipp Zabel wrote: > Hi Enric, > > On Wed, 2021-07-14 at 12:11 +0200, Enric Balletbo i Serra wrote: >> Among other features the mmsys driver should implement a reset >> controller to be able to reset different bits from their space. >> >> Cc: Jitao Shi <jitao.shi@mediatek.com> >> Suggested-by: Chun-Kuang Hu <chunkuang.hu@kernel.org> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > The reset controller driver looks fine, just two questions below. > >> --- >> >> (no changes since v1) >> >> drivers/soc/mediatek/mtk-mmsys.c | 69 ++++++++++++++++++++++++++++++++ >> drivers/soc/mediatek/mtk-mmsys.h | 2 + >> 2 files changed, 71 insertions(+) >> >> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c >> index e681029fe804..6ac4deff0164 100644 >> --- a/drivers/soc/mediatek/mtk-mmsys.c >> +++ b/drivers/soc/mediatek/mtk-mmsys.c > [...] >> @@ -91,6 +95,59 @@ void mtk_mmsys_ddp_disconnect(struct device *dev, > [...] >> +static int mtk_mmsys_reset(struct reset_controller_dev *rcdev, unsigned long id) >> +{ >> + int ret; >> + >> + ret = mtk_mmsys_reset_assert(rcdev, id); >> + if (ret) >> + return ret; >> + >> + usleep_range(1000, 1100); > > Is this known to be enough for all IP cores that can be reset by this > controller? > This time is copied from the downstream kernel, so, tbh, I am not totally sure is enough or needed. Let me try to reach the Mediatek people for if they can answer this. >> + return mtk_mmsys_reset_deassert(rcdev, id); >> +} >> + >> +static const struct reset_control_ops mtk_mmsys_reset_ops = { >> + .assert = mtk_mmsys_reset_assert, >> + .deassert = mtk_mmsys_reset_deassert, >> + .reset = mtk_mmsys_reset, >> +}; >> + >> static int mtk_mmsys_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -111,6 +168,18 @@ static int mtk_mmsys_probe(struct platform_device *pdev) >> return ret; >> } >> >> + spin_lock_init(&mmsys->lock); >> + >> + mmsys->rcdev.owner = THIS_MODULE; >> + mmsys->rcdev.nr_resets = 32; > > Are all bits in the MMSYS_SW0_RST_B register individual reset controls? Yes, all are individual reset controls, mostly related to display but not all (i.e dsi, dpi ...) Thanks, Enric > > regards > Philipp >
On Tue, 2021-07-20 at 19:07 +0200, Enric Balletbo i Serra wrote: > Hi Philipp, > > Thank you to take a look > > On 20/7/21 12:52, Philipp Zabel wrote: > > Hi Enric, > > > > On Wed, 2021-07-14 at 12:11 +0200, Enric Balletbo i Serra wrote: > > > Among other features the mmsys driver should implement a reset > > > controller to be able to reset different bits from their space. > > > > > > Cc: Jitao Shi <jitao.shi@mediatek.com> > > > Suggested-by: Chun-Kuang Hu <chunkuang.hu@kernel.org> > > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > > > The reset controller driver looks fine, just two questions below. > > > > > --- > > > > > > (no changes since v1) > > > > > > drivers/soc/mediatek/mtk-mmsys.c | 69 ++++++++++++++++++++++++++++++++ > > > drivers/soc/mediatek/mtk-mmsys.h | 2 + > > > 2 files changed, 71 insertions(+) > > > > > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c > > > index e681029fe804..6ac4deff0164 100644 > > > --- a/drivers/soc/mediatek/mtk-mmsys.c > > > +++ b/drivers/soc/mediatek/mtk-mmsys.c > > [...] > > > @@ -91,6 +95,59 @@ void mtk_mmsys_ddp_disconnect(struct device *dev, > > [...] > > > +static int mtk_mmsys_reset(struct reset_controller_dev *rcdev, unsigned long id) > > > +{ > > > + int ret; > > > + > > > + ret = mtk_mmsys_reset_assert(rcdev, id); > > > + if (ret) > > > + return ret; > > > + > > > + usleep_range(1000, 1100); > > > > Is this known to be enough for all IP cores that can be reset by this > > controller? > > > > This time is copied from the downstream kernel, so, tbh, I am not totally sure > is enough or needed. Let me try to reach the Mediatek people for if they can > answer this. That would be great. When this is resolved either way, feel free to add Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
On 14/07/2021 12:11, Enric Balletbo i Serra wrote: > Among other features the mmsys driver should implement a reset > controller to be able to reset different bits from their space. > > Cc: Jitao Shi <jitao.shi@mediatek.com> > Suggested-by: Chun-Kuang Hu <chunkuang.hu@kernel.org> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> I'm happy to take this, as soon as we have the updated binding description. Regards, Matthias > --- > > (no changes since v1) > > drivers/soc/mediatek/mtk-mmsys.c | 69 ++++++++++++++++++++++++++++++++ > drivers/soc/mediatek/mtk-mmsys.h | 2 + > 2 files changed, 71 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c > index e681029fe804..6ac4deff0164 100644 > --- a/drivers/soc/mediatek/mtk-mmsys.c > +++ b/drivers/soc/mediatek/mtk-mmsys.c > @@ -4,10 +4,12 @@ > * Author: James Liao <jamesjj.liao@mediatek.com> > */ > > +#include <linux/delay.h> > #include <linux/device.h> > #include <linux/io.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > +#include <linux/reset-controller.h> > #include <linux/soc/mediatek/mtk-mmsys.h> > > #include "mtk-mmsys.h" > @@ -55,6 +57,8 @@ static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = { > struct mtk_mmsys { > void __iomem *regs; > const struct mtk_mmsys_driver_data *data; > + spinlock_t lock; /* protects mmsys_sw_rst_b reg */ > + struct reset_controller_dev rcdev; > }; > > void mtk_mmsys_ddp_connect(struct device *dev, > @@ -91,6 +95,59 @@ void mtk_mmsys_ddp_disconnect(struct device *dev, > } > EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_disconnect); > > +static int mtk_mmsys_reset_update(struct reset_controller_dev *rcdev, unsigned long id, > + bool assert) > +{ > + struct mtk_mmsys *mmsys = container_of(rcdev, struct mtk_mmsys, rcdev); > + unsigned long flags; > + u32 reg; > + int i; > + > + spin_lock_irqsave(&mmsys->lock, flags); > + > + reg = readl_relaxed(mmsys->regs + MMSYS_SW0_RST_B); > + > + if (assert) > + reg &= ~BIT(id); > + else > + reg |= BIT(id); > + > + writel_relaxed(reg, mmsys->regs + MMSYS_SW0_RST_B); > + > + spin_unlock_irqrestore(&mmsys->lock, flags); > + > + return 0; > +} > + > +static int mtk_mmsys_reset_assert(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + return mtk_mmsys_reset_update(rcdev, id, true); > +} > + > +static int mtk_mmsys_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + return mtk_mmsys_reset_update(rcdev, id, false); > +} > + > +static int mtk_mmsys_reset(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + int ret; > + > + ret = mtk_mmsys_reset_assert(rcdev, id); > + if (ret) > + return ret; > + > + usleep_range(1000, 1100); > + > + return mtk_mmsys_reset_deassert(rcdev, id); > +} > + > +static const struct reset_control_ops mtk_mmsys_reset_ops = { > + .assert = mtk_mmsys_reset_assert, > + .deassert = mtk_mmsys_reset_deassert, > + .reset = mtk_mmsys_reset, > +}; > + > static int mtk_mmsys_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -111,6 +168,18 @@ static int mtk_mmsys_probe(struct platform_device *pdev) > return ret; > } > > + spin_lock_init(&mmsys->lock); > + > + mmsys->rcdev.owner = THIS_MODULE; > + mmsys->rcdev.nr_resets = 32; > + mmsys->rcdev.ops = &mtk_mmsys_reset_ops; > + mmsys->rcdev.of_node = pdev->dev.of_node; > + ret = devm_reset_controller_register(&pdev->dev, &mmsys->rcdev); > + if (ret) { > + dev_err(&pdev->dev, "Couldn't register mmsys reset controller: %d\n", ret); > + return ret; > + } > + > mmsys->data = of_device_get_match_data(&pdev->dev); > platform_set_drvdata(pdev, mmsys); > > diff --git a/drivers/soc/mediatek/mtk-mmsys.h b/drivers/soc/mediatek/mtk-mmsys.h > index 11388961dded..f9f9e12ceab8 100644 > --- a/drivers/soc/mediatek/mtk-mmsys.h > +++ b/drivers/soc/mediatek/mtk-mmsys.h > @@ -66,6 +66,8 @@ > #define DPI_SEL_IN_BLS 0x0 > #define DSI_SEL_IN_RDMA 0x1 > > +#define MMSYS_SW0_RST_B 0x140 > + > struct mtk_mmsys_routes { > u32 from_comp; > u32 to_comp; >
diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c index e681029fe804..6ac4deff0164 100644 --- a/drivers/soc/mediatek/mtk-mmsys.c +++ b/drivers/soc/mediatek/mtk-mmsys.c @@ -4,10 +4,12 @@ * Author: James Liao <jamesjj.liao@mediatek.com> */ +#include <linux/delay.h> #include <linux/device.h> #include <linux/io.h> #include <linux/of_device.h> #include <linux/platform_device.h> +#include <linux/reset-controller.h> #include <linux/soc/mediatek/mtk-mmsys.h> #include "mtk-mmsys.h" @@ -55,6 +57,8 @@ static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = { struct mtk_mmsys { void __iomem *regs; const struct mtk_mmsys_driver_data *data; + spinlock_t lock; /* protects mmsys_sw_rst_b reg */ + struct reset_controller_dev rcdev; }; void mtk_mmsys_ddp_connect(struct device *dev, @@ -91,6 +95,59 @@ void mtk_mmsys_ddp_disconnect(struct device *dev, } EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_disconnect); +static int mtk_mmsys_reset_update(struct reset_controller_dev *rcdev, unsigned long id, + bool assert) +{ + struct mtk_mmsys *mmsys = container_of(rcdev, struct mtk_mmsys, rcdev); + unsigned long flags; + u32 reg; + int i; + + spin_lock_irqsave(&mmsys->lock, flags); + + reg = readl_relaxed(mmsys->regs + MMSYS_SW0_RST_B); + + if (assert) + reg &= ~BIT(id); + else + reg |= BIT(id); + + writel_relaxed(reg, mmsys->regs + MMSYS_SW0_RST_B); + + spin_unlock_irqrestore(&mmsys->lock, flags); + + return 0; +} + +static int mtk_mmsys_reset_assert(struct reset_controller_dev *rcdev, unsigned long id) +{ + return mtk_mmsys_reset_update(rcdev, id, true); +} + +static int mtk_mmsys_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id) +{ + return mtk_mmsys_reset_update(rcdev, id, false); +} + +static int mtk_mmsys_reset(struct reset_controller_dev *rcdev, unsigned long id) +{ + int ret; + + ret = mtk_mmsys_reset_assert(rcdev, id); + if (ret) + return ret; + + usleep_range(1000, 1100); + + return mtk_mmsys_reset_deassert(rcdev, id); +} + +static const struct reset_control_ops mtk_mmsys_reset_ops = { + .assert = mtk_mmsys_reset_assert, + .deassert = mtk_mmsys_reset_deassert, + .reset = mtk_mmsys_reset, +}; + static int mtk_mmsys_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -111,6 +168,18 @@ static int mtk_mmsys_probe(struct platform_device *pdev) return ret; } + spin_lock_init(&mmsys->lock); + + mmsys->rcdev.owner = THIS_MODULE; + mmsys->rcdev.nr_resets = 32; + mmsys->rcdev.ops = &mtk_mmsys_reset_ops; + mmsys->rcdev.of_node = pdev->dev.of_node; + ret = devm_reset_controller_register(&pdev->dev, &mmsys->rcdev); + if (ret) { + dev_err(&pdev->dev, "Couldn't register mmsys reset controller: %d\n", ret); + return ret; + } + mmsys->data = of_device_get_match_data(&pdev->dev); platform_set_drvdata(pdev, mmsys); diff --git a/drivers/soc/mediatek/mtk-mmsys.h b/drivers/soc/mediatek/mtk-mmsys.h index 11388961dded..f9f9e12ceab8 100644 --- a/drivers/soc/mediatek/mtk-mmsys.h +++ b/drivers/soc/mediatek/mtk-mmsys.h @@ -66,6 +66,8 @@ #define DPI_SEL_IN_BLS 0x0 #define DSI_SEL_IN_RDMA 0x1 +#define MMSYS_SW0_RST_B 0x140 + struct mtk_mmsys_routes { u32 from_comp; u32 to_comp;
Among other features the mmsys driver should implement a reset controller to be able to reset different bits from their space. Cc: Jitao Shi <jitao.shi@mediatek.com> Suggested-by: Chun-Kuang Hu <chunkuang.hu@kernel.org> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> --- (no changes since v1) drivers/soc/mediatek/mtk-mmsys.c | 69 ++++++++++++++++++++++++++++++++ drivers/soc/mediatek/mtk-mmsys.h | 2 + 2 files changed, 71 insertions(+)