| Message ID | 20211201215127.23550-3-sergio.paracuellos@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: mt7621: Remove specific MIPS code from driver | expand |
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@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); > + 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"); >
Hi Guenter, On Wed, Dec 1, 2021 at 11:17 PM Guenter Roeck <linux@roeck-us.net> 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@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); 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"); > > >
On 12/2/21 12:29 AM, Sergio Paracuellos wrote: > Hi Guenter, > > On Wed, Dec 1, 2021 at 11:17 PM Guenter Roeck <linux@roeck-us.net> 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@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. 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"); >>> >>
Hi Guenter, On Thu, Dec 2, 2021 at 4:06 PM Guenter Roeck <linux@roeck-us.net> 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@roeck-us.net> 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@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? 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"); > >>> > >> >
On 12/2/21 7:50 AM, Sergio Paracuellos wrote: > Hi Guenter, > > On Thu, Dec 2, 2021 at 4:06 PM Guenter Roeck <linux@roeck-us.net> 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@roeck-us.net> 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@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. 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"); >>>>> >>>> >>
On Thu, Dec 2, 2021 at 6:02 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 12/2/21 7:50 AM, Sergio Paracuellos wrote: > > Hi Guenter, > > > > On Thu, Dec 2, 2021 at 4:06 PM Guenter Roeck <linux@roeck-us.net> 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@roeck-us.net> 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@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 > > > 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"); > >>>>> > >>>> > >> >
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@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); > + One more comment: From the include file, #define CM_GCR_REGn_MASK_ADDRMASK GENMASK(31, 16) suggests that only the upper 16 bit are valid (relevant ?) for the mask. Given that, it might make sense to make sure that the lower 16 bit are not set, maybe with mask = ~(entry->res->end - entry->res->start) & CM_GCR_REGn_MASK_ADDRMASK; Thanks, Guenter > + 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"); >
On Thu, Dec 2, 2021 at 8:45 PM Guenter Roeck <linux@roeck-us.net> 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@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); > > + > > One more comment: From the include file, > > #define CM_GCR_REGn_MASK_ADDRMASK GENMASK(31, 16) > > suggests that only the upper 16 bit are valid (relevant ?) for the mask. > Given that, it might make sense to make sure that the lower 16 bit are not set, > maybe with > mask = ~(entry->res->end - entry->res->start) & CM_GCR_REGn_MASK_ADDRMASK; > Makes sense, thanks. Best regards, Sergio Paracuellos > Thanks, > Guenter > > > + 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"); > > >
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); + + 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");
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(+)