Re: [PATCH v4 08/15] soc: bcm: bcm2835-power: Add support for BCM2711's RPiVid ASB

[Stefan Wahren <stefan.wahren@i2se.com>]

Hi Peter,

Am 20.02.22 um 20:27 schrieb Peter Robinson:
> 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?
Unfortunately i don't have any insight into these register. So a
suggestion would be BCM2835_BRDG_ID
>
> Also are you prepared to add/ok if I add your reviewed-by to the
> patches you've reviewed?

I will give my reviewed-by to the next version. I hope we will get more
feedback. So please add <maxime@cerno.tech> and <melissa.srw@gmail.com>
for the next round.

Best regards

>
> 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),
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel