From: Peter Robinson <pbrobinson@gmail.com>
To: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Nicolas Saenz Julienne <nsaenz@kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Florian Fainelli <f.fainelli@gmail.com>,
Ray Jui <rjui@broadcom.com>,
Scott Branden <sbranden@broadcom.com>,
bcm-kernel-feedback-list@broadcom.com,
devicetree@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Lee Jones <lee.jones@linaro.org>, Emma Anholt <emma@anholt.net>,
David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
Russell King <linux@armlinux.org.uk>,
Arnd Bergmann <arnd@arndb.de>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Subject: Re: [PATCH v4 08/15] soc: bcm: bcm2835-power: Add support for BCM2711's RPiVid ASB
Date: Sun, 20 Feb 2022 19:27:48 +0000 [thread overview]
Message-ID: <CALeDE9M7Km2xsK2ooheBzfy_75pSocMGdzti-LaFNpSh6tF2Fg@mail.gmail.com> (raw)
In-Reply-To: <27143189-0cb4-7bf5-25c1-1f5eff9c2d4d@i2se.com>
Hi Stefan,
On Sat, Feb 19, 2022 at 3:39 PM Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
> Hi Peter,
>
> Am 13.02.22 um 23:56 schrieb Peter Robinson:
> > From: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> >
> > In BCM2711 the new RPiVid ASB took over V3D. The old ASB is still present
> > with the ISP and H264 bits, and V3D is in the same place in the new ASB
> > as the old one.
> >
> > Use the fact that 'pm->rpivid_asb' is populated as a hint that we're on
> > BCM2711. On top of that introduce the macro ASB_BASE() which will select
> > the correct ASB register base, based on whether we're trying to access
> > V3D and which platform we're on.
> >
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
> > ---
> > Changes since v2:
> > - Correct names again
> >
> > Changes since v1:
> > - Correct names
> >
> > drivers/soc/bcm/bcm2835-power.c | 66 ++++++++++++++++++++-------------
> > 1 file changed, 41 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/soc/bcm/bcm2835-power.c b/drivers/soc/bcm/bcm2835-power.c
> > index 1e0041ec8132..eea31f75dc64 100644
> > --- a/drivers/soc/bcm/bcm2835-power.c
> > +++ b/drivers/soc/bcm/bcm2835-power.c
> > @@ -126,8 +126,9 @@
> >
> > #define ASB_AXI_BRDG_ID 0x20
> >
> > -#define ASB_READ(reg) readl(power->asb + (reg))
> > -#define ASB_WRITE(reg, val) writel(PM_PASSWORD | (val), power->asb + (reg))
> > +#define ASB_BASE(is_v3d) (is_v3d && power->rpivid_asb ? power->rpivid_asb : power->asb)
> > +#define ASB_READ(reg, is_v3d) readl(ASB_BASE(is_v3d) + (reg))
> > +#define ASB_WRITE(reg, val, is_v3d) writel(PM_PASSWORD | (val), ASB_BASE(is_v3d) + (reg))
> just a general note: passing parameter to decide which register should
> be used is not the preferred way. It's harder to maintain. A more data
> driven approach is better in the long run.
> >
> > struct bcm2835_power_domain {
> > struct generic_pm_domain base;
> > @@ -142,13 +143,16 @@ struct bcm2835_power {
> > void __iomem *base;
> > /* AXI Async bridge registers. */
> > void __iomem *asb;
> > + /* RPiVid bridge registers. */
> > + void __iomem *rpivid_asb;
> >
> > struct genpd_onecell_data pd_xlate;
> > struct bcm2835_power_domain domains[BCM2835_POWER_DOMAIN_COUNT];
> > struct reset_controller_dev reset;
> > };
> >
> > -static int bcm2835_asb_enable(struct bcm2835_power *power, u32 reg)
> > +static int bcm2835_asb_enable(struct bcm2835_power *power, u32 reg,
> > + bool is_v3d)
> > {
> > u64 start;
> >
> > @@ -158,8 +162,8 @@ static int bcm2835_asb_enable(struct bcm2835_power *power, u32 reg)
> > start = ktime_get_ns();
> >
> > /* Enable the module's async AXI bridges. */
> > - ASB_WRITE(reg, ASB_READ(reg) & ~ASB_REQ_STOP);
> > - while (ASB_READ(reg) & ASB_ACK) {
> > + ASB_WRITE(reg, ASB_READ(reg, is_v3d) & ~ASB_REQ_STOP, is_v3d);
> > + while (ASB_READ(reg, is_v3d) & ASB_ACK) {
> > cpu_relax();
> > if (ktime_get_ns() - start >= 1000)
> > return -ETIMEDOUT;
> > @@ -168,7 +172,8 @@ static int bcm2835_asb_enable(struct bcm2835_power *power, u32 reg)
> > return 0;
> > }
> >
> > -static int bcm2835_asb_disable(struct bcm2835_power *power, u32 reg)
> > +static int bcm2835_asb_disable(struct bcm2835_power *power, u32 reg,
> > + bool is_v3d)
> > {
> > u64 start;
> >
> > @@ -178,8 +183,8 @@ static int bcm2835_asb_disable(struct bcm2835_power *power, u32 reg)
> > start = ktime_get_ns();
> >
> > /* Enable the module's async AXI bridges. */
> > - ASB_WRITE(reg, ASB_READ(reg) | ASB_REQ_STOP);
> > - while (!(ASB_READ(reg) & ASB_ACK)) {
> > + ASB_WRITE(reg, ASB_READ(reg, is_v3d) | ASB_REQ_STOP, is_v3d);
> > + while (!(ASB_READ(reg, is_v3d) & ASB_ACK)) {
> > cpu_relax();
> > if (ktime_get_ns() - start >= 1000)
> > return -ETIMEDOUT;
> > @@ -274,7 +279,8 @@ static int bcm2835_asb_power_on(struct bcm2835_power_domain *pd,
> > u32 pm_reg,
> > u32 asb_m_reg,
> > u32 asb_s_reg,
> > - u32 reset_flags)
> > + u32 reset_flags,
> > + bool is_v3d)
> > {
> > struct bcm2835_power *power = pd->power;
> > int ret;
> > @@ -301,13 +307,13 @@ static int bcm2835_asb_power_on(struct bcm2835_power_domain *pd,
> > goto err_enable_resets;
> > }
> >
> > - ret = bcm2835_asb_enable(power, asb_m_reg);
> > + ret = bcm2835_asb_enable(power, asb_m_reg, is_v3d);
> > if (ret) {
> > dev_err(power->dev, "Failed to enable ASB master for %s\n",
> > pd->base.name);
> > goto err_disable_clk;
> > }
> > - ret = bcm2835_asb_enable(power, asb_s_reg);
> > + ret = bcm2835_asb_enable(power, asb_s_reg, is_v3d);
> > if (ret) {
> > dev_err(power->dev, "Failed to enable ASB slave for %s\n",
> > pd->base.name);
> > @@ -317,7 +323,7 @@ static int bcm2835_asb_power_on(struct bcm2835_power_domain *pd,
> > return 0;
> >
> > err_disable_asb_master:
> > - bcm2835_asb_disable(power, asb_m_reg);
> > + bcm2835_asb_disable(power, asb_m_reg, is_v3d);
> > err_disable_clk:
> > clk_disable_unprepare(pd->clk);
> > err_enable_resets:
> > @@ -329,22 +335,23 @@ static int bcm2835_asb_power_off(struct bcm2835_power_domain *pd,
> > u32 pm_reg,
> > u32 asb_m_reg,
> > u32 asb_s_reg,
> > - u32 reset_flags)
> > + u32 reset_flags,
> > + bool is_v3d)
> > {
> > struct bcm2835_power *power = pd->power;
> > int ret;
> >
> > - ret = bcm2835_asb_disable(power, asb_s_reg);
> > + ret = bcm2835_asb_disable(power, asb_s_reg, is_v3d);
> > if (ret) {
> > dev_warn(power->dev, "Failed to disable ASB slave for %s\n",
> > pd->base.name);
> > return ret;
> > }
> > - ret = bcm2835_asb_disable(power, asb_m_reg);
> > + ret = bcm2835_asb_disable(power, asb_m_reg, is_v3d);
> > if (ret) {
> > dev_warn(power->dev, "Failed to disable ASB master for %s\n",
> > pd->base.name);
> > - bcm2835_asb_enable(power, asb_s_reg);
> > + bcm2835_asb_enable(power, asb_s_reg, is_v3d);
> > return ret;
> > }
> >
> > @@ -369,7 +376,7 @@ static int bcm2835_power_pd_power_on(struct generic_pm_domain *domain)
> > case BCM2835_POWER_DOMAIN_GRAFX_V3D:
> > return bcm2835_asb_power_on(pd, PM_GRAFX,
> > ASB_V3D_M_CTRL, ASB_V3D_S_CTRL,
> > - PM_V3DRSTN);
> > + PM_V3DRSTN, true);
> >
> > case BCM2835_POWER_DOMAIN_IMAGE:
> > return bcm2835_power_power_on(pd, PM_IMAGE);
> > @@ -377,17 +384,17 @@ static int bcm2835_power_pd_power_on(struct generic_pm_domain *domain)
> > case BCM2835_POWER_DOMAIN_IMAGE_PERI:
> > return bcm2835_asb_power_on(pd, PM_IMAGE,
> > 0, 0,
> > - PM_PERIRSTN);
> > + PM_PERIRSTN, false);
> >
> > case BCM2835_POWER_DOMAIN_IMAGE_ISP:
> > return bcm2835_asb_power_on(pd, PM_IMAGE,
> > ASB_ISP_M_CTRL, ASB_ISP_S_CTRL,
> > - PM_ISPRSTN);
> > + PM_ISPRSTN, false);
> >
> > case BCM2835_POWER_DOMAIN_IMAGE_H264:
> > return bcm2835_asb_power_on(pd, PM_IMAGE,
> > ASB_H264_M_CTRL, ASB_H264_S_CTRL,
> > - PM_H264RSTN);
> > + PM_H264RSTN, false);
> >
> > case BCM2835_POWER_DOMAIN_USB:
> > PM_WRITE(PM_USB, PM_USB_CTRLEN);
> > @@ -435,7 +442,7 @@ static int bcm2835_power_pd_power_off(struct generic_pm_domain *domain)
> > case BCM2835_POWER_DOMAIN_GRAFX_V3D:
> > return bcm2835_asb_power_off(pd, PM_GRAFX,
> > ASB_V3D_M_CTRL, ASB_V3D_S_CTRL,
> > - PM_V3DRSTN);
> > + PM_V3DRSTN, true);
> >
> > case BCM2835_POWER_DOMAIN_IMAGE:
> > return bcm2835_power_power_off(pd, PM_IMAGE);
> > @@ -443,17 +450,17 @@ static int bcm2835_power_pd_power_off(struct generic_pm_domain *domain)
> > case BCM2835_POWER_DOMAIN_IMAGE_PERI:
> > return bcm2835_asb_power_off(pd, PM_IMAGE,
> > 0, 0,
> > - PM_PERIRSTN);
> > + PM_PERIRSTN, false);
> >
> > case BCM2835_POWER_DOMAIN_IMAGE_ISP:
> > return bcm2835_asb_power_off(pd, PM_IMAGE,
> > ASB_ISP_M_CTRL, ASB_ISP_S_CTRL,
> > - PM_ISPRSTN);
> > + PM_ISPRSTN, false);
> >
> > case BCM2835_POWER_DOMAIN_IMAGE_H264:
> > return bcm2835_asb_power_off(pd, PM_IMAGE,
> > ASB_H264_M_CTRL, ASB_H264_S_CTRL,
> > - PM_H264RSTN);
> > + PM_H264RSTN, false);
> >
> > case BCM2835_POWER_DOMAIN_USB:
> > PM_WRITE(PM_USB, 0);
> > @@ -626,13 +633,22 @@ static int bcm2835_power_probe(struct platform_device *pdev)
> > power->dev = dev;
> > power->base = pm->base;
> > power->asb = pm->asb;
> > + power->rpivid_asb = pm->rpivid_asb;
> >
> > - id = ASB_READ(ASB_AXI_BRDG_ID);
> > + id = ASB_READ(ASB_AXI_BRDG_ID, false);
> > if (id != 0x62726467 /* "BRDG" */) {
> > dev_err(dev, "ASB register ID returned 0x%08x\n", id);
> > return -ENODEV;
> > }
> >
> > + if (pm->rpivid_asb) {
> > + id = ASB_READ(ASB_AXI_BRDG_ID, true);
> > + if (id != 0x62726467 /* "BRDG" */) {
>
> It would be nice to use a define for this id, since we use it twice.
Any preferences or suggestion as to what it should be called?
Also are you prepared to add/ok if I add your reviewed-by to the
patches you've reviewed?
Peter
> Best regards
>
> > + dev_err(dev, "RPiVid ASB register ID returned 0x%08x\n", id);
> > + return -ENODEV;
> > + }
> > + }
> > +
> > power->pd_xlate.domains = devm_kcalloc(dev,
> > ARRAY_SIZE(power_domain_names),
> > sizeof(*power->pd_xlate.domains),
>
next prev parent reply other threads:[~2022-02-20 19:28 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-13 22:56 [PATCH v4 00/15] Raspberry PI 4 V3D enablement Peter Robinson
2022-02-13 22:56 ` [PATCH v4 01/15] dt-bindings: soc: bcm: bcm2835-pm: Convert bindings to DT schema Peter Robinson
2022-02-15 15:22 ` Rob Herring
2022-02-13 22:56 ` [PATCH v4 02/15] dt-bindings: soc: bcm: bcm2835-pm: Introduce reg-names Peter Robinson
2022-02-13 22:56 ` [PATCH v4 03/15] dt-bindings: soc: bcm: bcm2835-pm: Add support for bcm2711 Peter Robinson
2022-02-14 12:11 ` Stefan Wahren
2022-02-13 22:56 ` [PATCH v4 04/15] ARM: dts: bcm2835/bcm2711: Introduce reg-names in watchdog node Peter Robinson
2022-02-13 22:56 ` [PATCH v4 05/15] ARM: dts: bcm2711: Use proper compatible in PM/Watchdog node Peter Robinson
2022-02-13 22:56 ` [PATCH v4 06/15] mfd: bcm2835-pm: Use 'reg-names' to get resources Peter Robinson
2022-02-13 22:56 ` [PATCH v4 07/15] mfd: bcm2835-pm: Add support for BCM2711 Peter Robinson
2022-02-19 15:21 ` Stefan Wahren
2022-02-13 22:56 ` [PATCH v4 08/15] soc: bcm: bcm2835-power: Add support for BCM2711's RPiVid ASB Peter Robinson
2022-02-19 15:39 ` Stefan Wahren
2022-02-20 19:27 ` Peter Robinson [this message]
2022-02-20 21:14 ` Stefan Wahren
2022-02-13 22:56 ` [PATCH v4 09/15] soc: bcm: bcm2835-power: Bypass power_on/off() calls Peter Robinson
2022-02-13 22:56 ` [PATCH v4 10/15] dt-bindings: gpu: v3d: Add BCM2711's compatible Peter Robinson
2022-02-13 22:56 ` [PATCH v4 11/15] drm/v3d: Get rid of pm code Peter Robinson
2022-02-13 22:56 ` [PATCH v4 12/15] drm/v3d: Add support for bcm2711 Peter Robinson
2022-02-24 19:56 ` Adrien Thierry
2022-02-13 22:56 ` [PATCH v4 13/15] ARM: dts: bcm2711: Enable V3D Peter Robinson
2022-02-19 15:43 ` Stefan Wahren
2022-02-13 22:56 ` [PATCH v4 14/15] ARM: configs: Enable DRM_V3D Peter Robinson
2022-02-19 15:48 ` Stefan Wahren
2022-02-13 22:56 ` [PATCH v4 15/15] arm64: config: " Peter Robinson
2022-02-19 12:12 ` [PATCH v4 00/15] Raspberry PI 4 V3D enablement Stefan Wahren
2022-02-19 12:50 ` Peter Robinson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALeDE9M7Km2xsK2ooheBzfy_75pSocMGdzti-LaFNpSh6tF2Fg@mail.gmail.com \
--to=pbrobinson@gmail.com \
--cc=airlied@linux.ie \
--cc=arnd@arndb.de \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=catalin.marinas@arm.com \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=emma@anholt.net \
--cc=f.fainelli@gmail.com \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=linux@armlinux.org.uk \
--cc=nsaenz@kernel.org \
--cc=nsaenzjulienne@suse.de \
--cc=rjui@broadcom.com \
--cc=robh+dt@kernel.org \
--cc=sbranden@broadcom.com \
--cc=stefan.wahren@i2se.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).