Re: [PATCH v2 2/5] MIPS: ralink: implement 'pcibios_root_bridge_prepare()'

[Guenter Roeck]

On 12/2/21 7:50 AM, Sergio Paracuellos wrote:
> Hi Guenter,
>
> On Thu, Dec 2, 2021 at 4:06 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > On 12/2/21 12:29 AM, Sergio Paracuellos wrote:
> > > Hi Guenter,
> > >
> > > On Wed, Dec 1, 2021 at 11:17 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > > > On 12/1/21 1:51 PM, Sergio Paracuellos wrote:
> > > > > PCI core code call 'pcibios_root_bridge_prepare()' function inside function
> > > > > 'pci_register_host_bridge()'. This point is very good way to properly enter
> > > > > into this MIPS ralink specific code to properly setup I/O coherency units
> > > > > with PCI memory addresses.
> > > > >
> > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> > > > > ---
> > > > >     arch/mips/ralink/mt7621.c | 30 ++++++++++++++++++++++++++++++
> > > > >     1 file changed, 30 insertions(+)
> > > > >
> > > > > diff --git a/arch/mips/ralink/mt7621.c b/arch/mips/ralink/mt7621.c
> > > > > index bd71f5b14238..7649416c1cd7 100644
> > > > > --- a/arch/mips/ralink/mt7621.c
> > > > > +++ b/arch/mips/ralink/mt7621.c
> > > > > @@ -10,6 +10,7 @@
> > > > >     #include <linux/slab.h>
> > > > >     #include <linux/sys_soc.h>
> > > > >     #include <linux/memblock.h>
> > > > > +#include <linux/pci.h>
> > > > >
> > > > >     #include <asm/bootinfo.h>
> > > > >     #include <asm/mipsregs.h>
> > > > > @@ -22,6 +23,35 @@
> > > > >
> > > > >     static void *detect_magic __initdata = detect_memory_region;
> > > > >
> > > > > +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> > > > > +{
> > > > > +     struct resource_entry *entry;
> > > > > +     resource_size_t mask;
> > > > > +
> > > > > +     entry = resource_list_first_type(&bridge->windows, IORESOURCE_MEM);
> > > > > +     if (!entry) {
> > > > > +             pr_err("Cannot get memory resource\n");
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +
> > > > > +     if (mips_cps_numiocu(0)) {
> > > > > +             /*
> > > > > +              * FIXME: hardware doesn't accept mask values with 1s after
> > > > > +              * 0s (e.g. 0xffef), so it would be great to warn if that's
> > > > > +              * about to happen
> > > > > +              */ > +         mask = ~(entry->res->end - entry->res->start);
> > > > > +
> > > >
> > > > Try something like this:
> > > >                   WARN_ON((mask != ~0UL && BIT(ffz(mask)) - 1 != mask);
> > >
> > > Thanks for the tip. The following works for me:
> > >
> > >                     WARN_ON(mask != ~0UL && ~(BIT(__ffs(mask)) - 1) != mask);
> >
> > Are you sure ? __ffs() returns the first bit set, which isn't useful
> > for this test.
>
> My mask is calculated as follows:
>   mask = ~(entry->res->end - entry->res->start);
>
> Where for normal memory resource:
>   - entry->res->end = 0x6fffffff;
>   - entry->res->start = 0x60000000;
>
> So I end up with a mask: 0xf0000000.
>
> So applying ~(BIT(__ffs(mask)) - 1) I get a good '0xf0000000' for this
> particular case which looks correct.
>
> Suppose an invalid case with the mask being 0xffef0000.
>
> Applying ~(BIT(__ffs(mask)) - 1) will be 0xffff0000 which will trigger
> the WARN_ON since 0xffff0000 != 0xffef0000
>
> So I think this is correct... Am I missing something?

Your description says "hardware doesn't accept mask values with 1s after 0s
(e.g. 0xffef)". 0xf0000000 has 1s after 0s.

Your version works (I think) as long as the upper mask bits are all 1s.
It will fail, for example, if the mask value is 0xf000000 and
sizeof(long) == 8. Your test is the equivalent of "no mask value
with 0s after 1s", assuming that sizeof(resource_size_t) == sizeof(long).
As far as I can see with test code, it fails if sizeof(resource_size_t)
!= sizeof(long). Also, it returns an error if mask == 0. I guess that is
a corner case, but it would be interesting to know if it is theoretically
valid.

I _think_ the following works even if sizeof(resource_size_t) != sizeof(long).

	WARN_ON(mask && BIT(ffz(~mask)) - 1 != ~mask);

or, alternatively, something like

	mask2 = entry->res->end - entry->res->start;
	mask = ~mask2;
	WARN_ON(mask && BIT(ffz(mask2)) - 1 != mask2);

though that looks a bit weird.

Thanks,
Guenter

> Thanks,
>      Sergio Paracuellos
> > Guenter
> >
> > > I will send this as a different patch, though.
> > >
> > > Best regards,
> > >       Sergio Paracuellos
> > >
> > > > > +             write_gcr_reg1_base(entry->res->start);
> > > > > +             write_gcr_reg1_mask(mask | CM_GCR_REGn_MASK_CMTGT_IOCU0);
> > > > > +             pr_info("PCI coherence region base: 0x%08llx, mask/settings: 0x%08llx\n",
> > > > > +                     (unsigned long long)read_gcr_reg1_base(),
> > > > > +                     (unsigned long long)read_gcr_reg1_mask());
> > > > > +     }
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > >     phys_addr_t mips_cpc_default_phys_base(void)
> > > > >     {
> > > > >         panic("Cannot detect cpc address");