On Thu, Dec 2, 2021 at 6:02 PM Guenter Roeck [off-list ref] wrote:

On 12/2/21 7:50 AM, Sergio Paracuellos wrote:

quoted

Hi Guenter,

On Thu, Dec 2, 2021 at 4:06 PM Guenter Roeck [off-list ref] wrote:

quoted

On 12/2/21 12:29 AM, Sergio Paracuellos wrote:

quoted

Hi Guenter,

On Wed, Dec 1, 2021 at 11:17 PM Guenter Roeck [off-list ref] wrote:

quoted

On 12/1/21 1:51 PM, Sergio Paracuellos wrote:

quoted

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@gmail.com>
---
    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.

Thanks a lot for the clear explanation. I was assuming MIPS ralink
arch so sizeof(long) and sizeof(resource_size_t) are equal and upper
mask bits are all 1s. But you are right, my version will fail if this
sizeof(long) were eight.

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

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

This works for me also and looks like it does the right thing for any
case, thanks.

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.

Agreed, using two variables here looks weird also for me.

Best regards,
    Sergio Paracuellos

Thanks,
Guenter

quoted

Thanks,
     Sergio Paracuellos

quoted

Guenter

quoted

I will send this as a different patch, though.

Best regards,
      Sergio Paracuellos

quoted

quoted

+             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");