[PATCH RFC 5/5] reset: Add BCMBCA block resets through the PMB

[Linus Walleij]

Hi Philipp,

addressing comments before reposting the patches.

On Wed, Oct 14, 2026 at 1:14 PM Philipp Zabel <p.zabel at pengutronix.de> wrote:

> > +struct bcmbca_pmb_reset_group {
> > +     const char *name;
> > +     int (*reset)(struct bcmbca_pmb_reset *rst,
> > +                  const struct bcmbca_pmb_reset_group *group);
>
> What is the point of this indirection?
>
> There is only a single array of struct bcmbca_pmb_reset_group and all
> entries use the same bcmbca_pmb_reset_sequence().

No... one of them use bcm6846_xrdp_reset() instead?

> > +static int bcmbca_pmb_reset_sequence(struct bcmbca_pmb_reset *rst,
> > +                                  const struct bcmbca_pmb_reset_group *group);
> > +
> > +static const struct bcmbca_pmb_reset_block bcm6846_xrdp_resets[] = {
> > +     /* Keep the XRDP bus interfaces out of reset. */
>
> What does this comment refer to? assert_value being GENMASK(2, 0)
> instead of 0?

Indeed. I fixed up the comment so it's clear what is going on.



> > +     {
> > +             .bus = 1,
> > +             .device = 12,
> > +             .offset = 5,
> > +             .mask = GENMASK(27, 0),
> > +             .assert_value = GENMASK(2, 0),
> > +             .deassert_value = GENMASK(27, 0),
>
> Are the four different devices the packet engine and the tree runner
> cores mentioned in the dt-bindings patch?

Yes.

> Do the 28 bits refer to 28 different resets?

The only driver I have writes 0x0fffffff to all four registers
to release the resets, and this being Broadcom that means
that is also the only documentation.(I don't work for them,
my mission here is to have a working platform.) I could of course
experiment with different values but that turns out to
be more of a hardware safari, I made it like this because
I don't want to fix something that's not broken.

> > +/* Bits 6 and 7 select the lane configuration and must remain unchanged. */
> > +static const struct bcmbca_pmb_reset_block bcm6846_pcie_resets[] = {
> > +     {
> > +             .bus = 0,
> > +             .device = 18,
> > +             .offset = 5,
> > +             .mask = GENMASK(5, 0),
> > +             .assert_value = GENMASK(5, 0),
>
> Same here, are these 6 different resets? Does the hardware require them
> to be asserted/deasserted at the same time?

Same here, this is what the Broadcom driver does. If you insist,
we can do hardware safari, but there are no docs or other examples.

When I look closer at the PCIe support it has some more weird
sequencing going on, so I'll drop PCIe support from the patch
series for now.

> > +     },
> > +     {
> > +             .bus = 0,
> > +             .device = 19,
> > +             .offset = 5,
> > +             .mask = GENMASK(5, 0),
> > +             .assert_value = GENMASK(5, 0),
> > +     },
> > +};
>
> Why put these in an array?

Right there is just two of them. Individual taste I assume,
I dropped these anyway.

> > +/* Best effort with both locks held; never restart a partially reset group. */
> > +static void bcmbca_pmb_reset_cleanup(struct bcmbca_pmb_reset *rst,
> > +                                  const struct bcmbca_pmb_reset_group *group)
> > +{
> > +     const struct bcmbca_pmb_reset_block *block;
> > +     unsigned int i;
> > +     u32 value;
> > +     int ret;
> > +
> > +     for (i = 0; i < group->num_blocks; i++) {
> > +             block = &group->blocks[i];
> > +             ret = bcmbca_pmb_read(rst->pmb, block->bus, block->device,
> > +                                   block->offset, &value);
> > +             if (!ret) {
> > +                     value &= ~block->mask;
> > +                     value |= block->assert_value;
> > +                     ret = bcmbca_pmb_write(rst->pmb, block->bus, block->device,
> > +                                            block->offset, value);
> > +             }
>
> Could the PMB bus driver provide an _update_bits() function for these
> read-modify-write sequences? It is the one handling the locking anyway.
>
> This whole driver feels a bit like if PMB could just export a regmap,
> it would collapse into a few regmap_multi_reg_write()s.

You're damn right about that.

I will rinse and repeat.

>
> > +             if (ret)
> > +                     dev_err(rst->rcdev.dev,
> > +                             "failed to hold PMB bus %u device %u in reset: %d\n",
> > +                             block->bus, block->device, ret);
> > +     }
> > +}
> > +
> > +/* Called with both PMB locks held, including during error cleanup. */
>
> Which two locks are "both"? pmb->lock mutex and the PMB_MUTEX hardware
> lock?

Fixed with regmap abstraction!

> > +static int bcmbca_pmb_reset_sequence(struct bcmbca_pmb_reset *rst,
> > +                                  const struct bcmbca_pmb_reset_group *group)
> > +{
> > +     const struct bcmbca_pmb_reset_block *block;
> > +     unsigned int i;
> > +     u32 value;
> > +     int ret, pass;
> > +
> > +     if (!group->num_blocks)
> > +             return -EINVAL;
> > +
> > +     /* Assert every block before releasing any of them. */
> > +     for (pass = 0; pass < 2; pass++) {
>
> Maybe   for (pass = PASS_ASSERT; pass <= PASS_DEASSERT

OK I'll take a stab and see how it looks.

> > +             for (i = 0; i < group->num_blocks; i++) {
> > +                     block = &group->blocks[i];
> > +                     ret = bcmbca_pmb_read(rst->pmb, block->bus, block->device,
> > +                                           block->offset, &value);
> > +                     if (ret)
> > +                             goto release;
> > +                     value &= ~block->mask;
> > +                     value |= pass ? block->deassert_value : block->assert_value;
> > +                     ret = bcmbca_pmb_write(rst->pmb, block->bus, block->device,
> > +                                            block->offset, value);
> > +                     if (ret)
> > +                             goto release;
> > +             }
> > +             if (!pass && group->assert_us)
> > +                     usleep_range(group->assert_us, group->assert_us * 2);
>
> Is that assertion delay the time the reset needs to stay asserted or is
> this really a delay that needs to be observed between successive
> assertions in a group?

The former. I will rename the variable and add a comment so
this is clear.

> > +release:
> > +     if (ret) {
> > +             dev_err(rst->rcdev.dev, "%s %s failed at PMB bus %u device %u: %d\n",
> > +                     group->name, pass ? "release" : "assert",
>
> Please use "deassert" to avoid possible confusion with the
> reset_control_release() meaning of "release".

OK

Yours,
Linus Walleij