[PATCH v2 0/5] PCI: mt7621: Remove specific MIPS code from driver

[PATCH v2 0/5] PCI: mt7621: Remove specific MIPS code from driver

[Sergio Paracuellos]

Hi all,

MIPS specific code can be removed from driver and put into ralink mt7621
instead which is a more accurate place to do this. To make this possible
we need to have access to 'bridge->windows' in 'pcibios_root_bridge_prepare()'
which has been implemented for ralink mt7621 platform (there is no real
need to implement this for any other platforms since those ones haven't got
I/O coherency units). This also allow us to properly enable this driver to
completely be enabled for COMPILE_TEST. This patchset appoarch:
- Move windows list splice in 'pci_register_host_bridge()' after function
  'pcibios_root_bridge_prepare()' is called.
- Implement 'pcibios_root_bridge_prepare()' for ralink mt7621.
- Avoid custom MIPs code in pcie-mt7621 driver.
- Add missing 'MODULE_LICENSE()' to pcie-mt7621 driver to avoid compile test
  module compilation to complain (already sent patch from Yanteng Si that
  I have rewrite commit message and long description a bit.
- Remove MIPS conditional code from Kconfig.

This patchset also fix some errors reported by Kernel Test Robot about
implicit mips functions used in driver code and fix errors in driver when
is compiled as a module [1] (mips:allmodconfig).

There was an ongoing discussion about this here [0] but I preferred to send
my proposal for better review and understanding.

Changes in v2:
 - Collect Acked-by from Arnd Bergmann for PATCH 1.
 - Collect Reviewed-by from Krzysztof Wilczyński for PATCH 4.
 - Adjust some patches commit subject and message as pointed out by Bjorn in review of v1 of the series[2]. 

This patchset is the good way of properly compile driver as a module removing
all MIPS specific code into arch ralink mt7621 place. To avoid mips:allmodconfig reported
problems for v5.16 the following patch has been sent [3]. This patch should be reverted
for properly add this series.

[0]: https://lore.kernel.org/linux-mips/CAMhs-H8ShoaYiFOOzJaGC68nZz=V365RXN_Kjuj=fPFENGJiiw@mail.gmail.com/T/#t
[1]: https://lkml.org/lkml/2021/11/14/436
[2]: https://lore.kernel.org/r/20211115070809.15529-1-sergio.paracuellos@gmail.com
[3]: https://lore.kernel.org/linux-pci/20211201214343.23307-1-sergio.paracuellos@gmail.com/T/#u

Thanks in advance for your time.

Best regards,
   Sergio Paracuellos

Sergio Paracuellos (5):
  PCI: Let pcibios_root_bridge_prepare() access to 'bridge->windows'
  MIPS: ralink: implement 'pcibios_root_bridge_prepare()'
  PCI: mt7621: Avoid custom MIPS code in driver code
  PCI: mt7621: Add missing 'MODULE_LICENSE()' definition
  PCI: mt7621: Kconfig: Allow COMPILE_TEST for all arches

 arch/mips/ralink/mt7621.c            | 30 +++++++++++++++++++++
 drivers/pci/controller/Kconfig       |  2 +-
 drivers/pci/controller/pcie-mt7621.c | 39 ++--------------------------
 drivers/pci/probe.c                  |  4 +--
 4 files changed, 35 insertions(+), 40 deletions(-)

-- 
2.33.0

[PATCH v2 1/5] PCI: Let pcibios_root_bridge_prepare() access to 'bridge->windows'

[Sergio Paracuellos]

When function pci_register_host_bridge() is called, 'bridge->windows' are
already available. However this windows are being moved temporarily from
there. To let pcibios_root_bridge_prepare() to have access to this windows
move this windows movement after call this function. This is interesting for
MIPS ralink mt7621 platform to be able to properly set I/O coherence units
with this information and avoid custom MIPS code in generic PCIe controller
drivers.

Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/pci/probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 087d3658f75c..372a70efccc6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -898,8 +898,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 
 	bridge->bus = bus;
 
-	/* Temporarily move resources off the list */
-	list_splice_init(&bridge->windows, &resources);
 	bus->sysdata = bridge->sysdata;
 	bus->ops = bridge->ops;
 	bus->number = bus->busn_res.start = bridge->busnr;
@@ -925,6 +923,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	if (err)
 		goto free;
 
+	/* Temporarily move resources off the list */
+	list_splice_init(&bridge->windows, &resources);
 	err = device_add(&bridge->dev);
 	if (err) {
 		put_device(&bridge->dev);
-- 
2.33.0

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

[Sergio Paracuellos]

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

[PATCH v2 3/5] PCI: mt7621: Avoid custom MIPS code in driver code

[Sergio Paracuellos]

Driver code is setting up MIPS specific I/O coherency units addresses config.
This MIPS specific thing has been moved to be done when PCI code call the
'pcibios_root_bridge_prepare()' function which has been implemented for MIPS
ralink mt7621 platform. Hence, remove MIPS specific code from driver code.
After this change there is also no need to add any MIPS specific includes
to avoid some errors reported by Kernet Test Robot with W=1 builds.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/pci/controller/pcie-mt7621.c | 37 ----------------------------
 1 file changed, 37 deletions(-)

diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
index 4138c0e83513..42cce31df943 100644
--- a/drivers/pci/controller/pcie-mt7621.c
+++ b/drivers/pci/controller/pcie-mt7621.c
@@ -208,37 +208,6 @@ static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
 		reset_control_assert(port->pcie_rst);
 }
 
-static int setup_cm_memory_region(struct pci_host_bridge *host)
-{
-	struct mt7621_pcie *pcie = pci_host_bridge_priv(host);
-	struct device *dev = pcie->dev;
-	struct resource_entry *entry;
-	resource_size_t mask;
-
-	entry = resource_list_first_type(&host->windows, IORESOURCE_MEM);
-	if (!entry) {
-		dev_err(dev, "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);
-		dev_info(dev, "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;
-}
-
 static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
 				  struct device_node *node,
 				  int slot)
@@ -557,12 +526,6 @@ static int mt7621_pci_probe(struct platform_device *pdev)
 		goto remove_resets;
 	}
 
-	err = setup_cm_memory_region(bridge);
-	if (err) {
-		dev_err(dev, "error setting up iocu mem regions\n");
-		goto remove_resets;
-	}
-
 	return mt7621_pcie_register_host(bridge);
 
 remove_resets:
-- 
2.33.0

[PATCH v2 4/5] PCI: mt7621: Add missing 'MODULE_LICENSE()' definition

[Sergio Paracuellos]

MT7621 PCIe host controller driver can be built as a module but there is no
'MODULE_LICENSE()' specified in code, causing a build error due to missing
license information.

ERROR: modpost: missing MODULE_LICENSE() in drivers/pci/controller/pcie-mt7621.o

Fix this by adding 'MODULE_LICENSE()' to the driver.

Fixes: 2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/pci/controller/pcie-mt7621.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
index 42cce31df943..9da7452f565e 100644
--- a/drivers/pci/controller/pcie-mt7621.c
+++ b/drivers/pci/controller/pcie-mt7621.c
@@ -561,3 +561,5 @@ static struct platform_driver mt7621_pci_driver = {
 	},
 };
 builtin_platform_driver(mt7621_pci_driver);
+
+MODULE_LICENSE("GPL v2");
-- 
2.33.0

[PATCH v2 5/5] PCI: mt7621: Kconfig: Allow COMPILE_TEST for all arches

[Sergio Paracuellos]

Since all MIPS specific code has been moved from the controller driver side
there is no more stoppers to enable the driver to be completely enable for
'COMPILE_TEST'. Hence, remove MIPS conditional statement.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/pci/controller/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 93b141110537..18d41d2b0392 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -333,7 +333,7 @@ config PCIE_APPLE
 
 config PCIE_MT7621
 	tristate "MediaTek MT7621 PCIe Controller"
-	depends on (RALINK && SOC_MT7621) || (MIPS && COMPILE_TEST)
+	depends on (RALINK && SOC_MT7621) || COMPILE_TEST
 	select PHY_MT7621_PCI
 	default SOC_MT7621
 	help
-- 
2.33.0

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

[Guenter Roeck]

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

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

[Sergio Paracuellos]

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

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

[Guenter Roeck]

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

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

[Sergio Paracuellos]

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

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

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

[Sergio Paracuellos]

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

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

[Guenter Roeck]

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

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

[Sergio Paracuellos]

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