[PATCH 1/4] pci: rename variable mask -> barval

[PATCH 1/4] pci: rename variable mask -> barval

[Uwe Kleine-König]

The variable isn't really a mask. And it is irritating that this "mask"
value is passed as parameter "maxbase" to the function pci_size() which
also has a parameter "mask". So rename it to a more generic term.

The main reason for this patch is that the next patch introduces another
variable with name "mask" and so this original is in the way.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pci/pci.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b2570eb15181..538903ee6607 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -170,25 +170,26 @@ static void setup_device(struct pci_dev *dev, int max_bar)
 
 	for (bar = 0; bar < max_bar; bar++) {
 		resource_size_t last_addr;
-		u32 orig, mask, size;
+		u32 orig, barval, size;
 
 		pci_read_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, &orig);
 		pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, 0xfffffffe);
-		pci_read_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, &mask);
+		pci_read_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, &barval);
 		pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, orig);
 
-		if (mask == 0 || mask == 0xffffffff) {
+		if (barval == 0 || barval == 0xffffffff) {
 			pr_debug("pbar%d set bad mask\n", bar);
 			continue;
 		}
 
-		if (mask & 0x01) { /* IO */
-			size = pci_size(orig, mask, 0xfffffffe);
+		if (barval & 0x01) { /* IO */
+			size = pci_size(orig, barval, 0xfffffffe);
 			if (!size) {
 				pr_debug("pbar%d bad IO mask\n", bar);
 				continue;
 			}
-			pr_debug("pbar%d: mask=%08x io %d bytes\n", bar, mask, size);
+			pr_debug("pbar%d: barval=%08x io %d bytes\n",
+				 bar, barval, size);
 			if (ALIGN(last_io, size) + size >
 			    dev->bus->resource[PCI_BUS_RESOURCE_IO]->end) {
 				pr_debug("BAR does not fit within bus IO res\n");
@@ -200,15 +201,15 @@ static void setup_device(struct pci_dev *dev, int max_bar)
 			dev->resource[bar].flags = IORESOURCE_IO;
 			last_addr = last_io;
 			last_io += size;
-		} else if ((mask & PCI_BASE_ADDRESS_MEM_PREFETCH) &&
+		} else if ((barval & PCI_BASE_ADDRESS_MEM_PREFETCH) &&
 		           last_mem_pref) /* prefetchable MEM */ {
-			size = pci_size(orig, mask, 0xfffffff0);
+			size = pci_size(orig, barval, 0xfffffff0);
 			if (!size) {
 				pr_debug("pbar%d bad P-MEM mask\n", bar);
 				continue;
 			}
-			pr_debug("pbar%d: mask=%08x P memory %d bytes\n",
-			    bar, mask, size);
+			pr_debug("pbar%d: barval=%08x P memory %d bytes\n",
+				 bar, barval, size);
 			if (ALIGN(last_mem_pref, size) + size >
 			    dev->bus->resource[PCI_BUS_RESOURCE_MEM_PREF]->end) {
 				pr_debug("BAR does not fit within bus p-mem res\n");
@@ -222,13 +223,13 @@ static void setup_device(struct pci_dev *dev, int max_bar)
 			last_addr = last_mem_pref;
 			last_mem_pref += size;
 		} else { /* non-prefetch MEM */
-			size = pci_size(orig, mask, 0xfffffff0);
+			size = pci_size(orig, barval, 0xfffffff0);
 			if (!size) {
 				pr_debug("pbar%d bad NP-MEM mask\n", bar);
 				continue;
 			}
-			pr_debug("pbar%d: mask=%08x NP memory %d bytes\n",
-			    bar, mask, size);
+			pr_debug("pbar%d: barval=%08x NP memory %d bytes\n",
+				 bar, barval, size);
 			if (ALIGN(last_mem, size) + size >
 			    dev->bus->resource[PCI_BUS_RESOURCE_MEM]->end) {
 				pr_debug("BAR does not fit within bus np-mem res\n");
@@ -245,7 +246,7 @@ static void setup_device(struct pci_dev *dev, int max_bar)
 		dev->resource[bar].start = last_addr;
 		dev->resource[bar].end = last_addr + size - 1;
 
-		if ((mask & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
+		if ((barval & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
 			dev->resource[bar].flags |= IORESOURCE_MEM_64;
 			pci_write_config_dword(dev,
 			       PCI_BASE_ADDRESS_1 + bar * 4, 0);
-- 
2.16.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/4] pci: refactor bar configuration

[Uwe Kleine-König]

Instead of doing nearly the same three times for the three different
memory types, use some helper variables and do the same in a more
generic way only once.

Apart from minor changes in the pr_debug output there is no semantic
change intended.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pci/pci.c | 104 ++++++++++++++++++++++--------------------------------
 1 file changed, 43 insertions(+), 61 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 538903ee6607..87359b6de82c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -169,8 +169,11 @@ static void setup_device(struct pci_dev *dev, int max_bar)
 			      cmd & ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY));
 
 	for (bar = 0; bar < max_bar; bar++) {
-		resource_size_t last_addr;
-		u32 orig, barval, size;
+		u32 orig, barval, size, mask;
+		const char *type;
+		int restype;
+		unsigned long resflags;
+		resource_size_t *last;
 
 		pci_read_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, &orig);
 		pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, 0xfffffffe);
@@ -183,68 +186,47 @@ static void setup_device(struct pci_dev *dev, int max_bar)
 		}
 
 		if (barval & 0x01) { /* IO */
-			size = pci_size(orig, barval, 0xfffffffe);
-			if (!size) {
-				pr_debug("pbar%d bad IO mask\n", bar);
-				continue;
-			}
-			pr_debug("pbar%d: barval=%08x io %d bytes\n",
-				 bar, barval, size);
-			if (ALIGN(last_io, size) + size >
-			    dev->bus->resource[PCI_BUS_RESOURCE_IO]->end) {
-				pr_debug("BAR does not fit within bus IO res\n");
-				return;
-			}
-			last_io = ALIGN(last_io, size);
-			pr_debug("pbar%d: allocated at 0x%08x\n", bar, last_io);
-			pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, last_io);
-			dev->resource[bar].flags = IORESOURCE_IO;
-			last_addr = last_io;
-			last_io += size;
+			mask = 0xfffffffe;
+			type = "IO";
+			restype = PCI_BUS_RESOURCE_IO;
+			resflags = IORESOURCE_IO;
+			last = &last_io;
 		} else if ((barval & PCI_BASE_ADDRESS_MEM_PREFETCH) &&
-		           last_mem_pref) /* prefetchable MEM */ {
-			size = pci_size(orig, barval, 0xfffffff0);
-			if (!size) {
-				pr_debug("pbar%d bad P-MEM mask\n", bar);
-				continue;
-			}
-			pr_debug("pbar%d: barval=%08x P memory %d bytes\n",
-				 bar, barval, size);
-			if (ALIGN(last_mem_pref, size) + size >
-			    dev->bus->resource[PCI_BUS_RESOURCE_MEM_PREF]->end) {
-				pr_debug("BAR does not fit within bus p-mem res\n");
-				return;
-			}
-			last_mem_pref = ALIGN(last_mem_pref, size);
-			pr_debug("pbar%d: allocated at 0x%08x\n", bar, last_mem_pref);
-			pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, last_mem_pref);
-			dev->resource[bar].flags = IORESOURCE_MEM |
-			                           IORESOURCE_PREFETCH;
-			last_addr = last_mem_pref;
-			last_mem_pref += size;
-		} else { /* non-prefetch MEM */
-			size = pci_size(orig, barval, 0xfffffff0);
-			if (!size) {
-				pr_debug("pbar%d bad NP-MEM mask\n", bar);
-				continue;
-			}
-			pr_debug("pbar%d: barval=%08x NP memory %d bytes\n",
-				 bar, barval, size);
-			if (ALIGN(last_mem, size) + size >
-			    dev->bus->resource[PCI_BUS_RESOURCE_MEM]->end) {
-				pr_debug("BAR does not fit within bus np-mem res\n");
-				return;
-			}
-			last_mem = ALIGN(last_mem, size);
-			pr_debug("pbar%d: allocated at 0x%08x\n", bar, last_mem);
-			pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, last_mem);
-			dev->resource[bar].flags = IORESOURCE_MEM;
-			last_addr = last_mem;
-			last_mem += size;
+			   last_mem_pref) { /* prefetchable MEM */
+			mask = 0xfffffff0;
+			type = "P";
+			restype = PCI_BUS_RESOURCE_MEM_PREF;
+			resflags = IORESOURCE_MEM | IORESOURCE_PREFETCH;
+			last = &last_mem_pref;
+		} else { /* non-prefetchable MEM */
+			mask = 0xfffffff0;
+			type = "NP";
+			restype = PCI_BUS_RESOURCE_MEM;
+			resflags = IORESOURCE_MEM;
+			last = &last_mem;
 		}
 
-		dev->resource[bar].start = last_addr;
-		dev->resource[bar].end = last_addr + size - 1;
+		size = pci_size(orig, barval, mask);
+		if (!size) {
+			pr_debug("pbar%d bad %s mask\n", bar, type);
+			continue;
+		}
+
+		pr_debug("pbar%d: barval=%08x type=%s size=0x%x\n",
+			 bar, barval, type, size);
+
+		if (ALIGN(*last, size) + size > dev->bus->resource[restype]->end) {
+			pr_debug("BAR does not fit within bus %s res\n", type);
+			return;
+		}
+
+		*last = ALIGN(*last, size);
+		pr_debug("pbar%d: allocated at 0x%08x\n", bar, *last);
+		pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, *last);
+		dev->resource[bar].flags = resflags;
+		dev->resource[bar].start = *last;
+		*last += size;
+		dev->resource[bar].end = *last - 1;
 
 		if ((barval & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
 			dev->resource[bar].flags |= IORESOURCE_MEM_64;
-- 
2.16.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/4] pci: improve debug output

[Uwe Kleine-König]

Print a single line per bar with all relevant information and indent it
by a space to group them together per device.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pci/pci.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 87359b6de82c..837e17aa8397 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -181,7 +181,7 @@ static void setup_device(struct pci_dev *dev, int max_bar)
 		pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, orig);
 
 		if (barval == 0 || barval == 0xffffffff) {
-			pr_debug("pbar%d set bad mask\n", bar);
+			pr_debug(" pbar%d: set bad mask (%08x)\n", bar, barval);
 			continue;
 		}
 
@@ -208,20 +208,21 @@ static void setup_device(struct pci_dev *dev, int max_bar)
 
 		size = pci_size(orig, barval, mask);
 		if (!size) {
-			pr_debug("pbar%d bad %s mask\n", bar, type);
+			pr_debug(" pbar%d: bad %s size (orig=%08x, barval=%08x, type=%s)\n",
+				 bar, type, orig, barval, type);
 			continue;
 		}
 
-		pr_debug("pbar%d: barval=%08x type=%s size=0x%x\n",
-			 bar, barval, type, size);
-
 		if (ALIGN(*last, size) + size > dev->bus->resource[restype]->end) {
-			pr_debug("BAR does not fit within bus %s res\n", type);
+			pr_debug(" pbar%d: !! type=%s allocbase=0x%08x + size=0x%08x > resource_end=%pa (barval=%08x)\n",
+				 bar, type, ALIGN(*last, size), size, &dev->bus->resource[restype]->end, barval);
 			return;
 		}
 
 		*last = ALIGN(*last, size);
-		pr_debug("pbar%d: allocated at 0x%08x\n", bar, *last);
+		pr_debug(" pbar%d: type=%s allocbase=0x%08x size=0x%08x (barval=%08x)\n",
+			 bar, type, *last, size, barval);
+
 		pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + bar * 4, *last);
 		dev->resource[bar].flags = resflags;
 		dev->resource[bar].start = *last;
-- 
2.16.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/4] pci: don't hide hard to understand sanity check in size calculation

[Uwe Kleine-König]

pci_size() had a check that made it return 0 (i.e. an error) in some
cases. It took me a while to work out in which situations exactly this
check triggered: If writing 0xfffffffe to the BAR register doesn't
change it's value and the masked value isn't in the form 0b1....10...0.

This patch replaces the check in pci_size() by a similar check in
setup_device() where it is more expected to happen. This allows to
simplify pci_size() considerably.

The new check is a bit more strict as it doesn't consider the value the
BAR register holds initially and bails out iff the maximal base value
isn't in the expected format.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pci/pci.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 837e17aa8397..72689560d598 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -144,18 +144,10 @@ static struct pci_dev *alloc_pci_dev(void)
 	return dev;
 }
 
-static u32 pci_size(u32 base, u32 maxbase, u32 mask)
+static u32 pci_size(u32 maxbase)
 {
-	u32 size = maxbase & mask;
-	if (!size)
-		return 0;
-
-	size = (size & ~(size-1)) - 1;
-
-	if (base == maxbase && ((base | size) & mask) != mask)
-		return 0;
-
-	return size + 1;
+	/* isolate right-most set bit */
+	return maxbase & ~(maxbase - 1);
 }
 
 
@@ -169,7 +161,7 @@ static void setup_device(struct pci_dev *dev, int max_bar)
 			      cmd & ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY));
 
 	for (bar = 0; bar < max_bar; bar++) {
-		u32 orig, barval, size, mask;
+		u32 orig, barval, maxbase, size, mask;
 		const char *type;
 		int restype;
 		unsigned long resflags;
@@ -206,13 +198,19 @@ static void setup_device(struct pci_dev *dev, int max_bar)
 			last = &last_mem;
 		}
 
-		size = pci_size(orig, barval, mask);
-		if (!size) {
-			pr_debug(" pbar%d: bad %s size (orig=%08x, barval=%08x, type=%s)\n",
-				 bar, type, orig, barval, type);
+		maxbase = barval & mask;
+
+		/*
+		 * if maxbase isn't in the form 0b1...10...0 there is something
+		 * fishy
+		 */
+		if ((maxbase | (maxbase - 1)) != (u32)-1 || maxbase == 0) {
+			pr_debug(" pbar%d: !! barval=%08x maxbase=0x%08x\n", bar, barval, maxbase);
 			continue;
 		}
 
+		size = pci_size(maxbase);
+
 		if (ALIGN(*last, size) + size > dev->bus->resource[restype]->end) {
 			pr_debug(" pbar%d: !! type=%s allocbase=0x%08x + size=0x%08x > resource_end=%pa (barval=%08x)\n",
 				 bar, type, ALIGN(*last, size), size, &dev->bus->resource[restype]->end, barval);
-- 
2.16.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/4] pci: improve debug output

[Uwe Kleine-König]

Hello,

On Sat, Apr 14, 2018 at 01:30:52AM +0200, Uwe Kleine-König wrote:
>  		if (ALIGN(*last, size) + size > dev->bus->resource[restype]->end) {
> -			pr_debug("BAR does not fit within bus %s res\n", type);
> +			pr_debug(" pbar%d: !! type=%s allocbase=0x%08x + size=0x%08x > resource_end=%pa (barval=%08x)\n",
> +				 bar, type, ALIGN(*last, size), size, &dev->bus->resource[restype]->end, barval);

slightly related: I have a machine where this new line results in:

	pci:  pbar0: !! type=NP allocbase=0x01800000 + size=0x00800000 > resource_end=0x01efffff (barval=ff800000)

This makes me wonder if the LHS of the check needs to read

	ALIGN(*last, size) + size - 1

. I will think about this after sleeping, and then probably send a patch
for that.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] pci: don't hide hard to understand sanity check in size calculation

[Andrey Smirnov]

On Fri, Apr 13, 2018 at 4:30 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> pci_size() had a check that made it return 0 (i.e. an error) in some
> cases. It took me a while to work out in which situations exactly this
> check triggered: If writing 0xfffffffe to the BAR register doesn't
> change it's value and the masked value isn't in the form 0b1....10...0.
>
> This patch replaces the check in pci_size() by a similar check in
> setup_device() where it is more expected to happen. This allows to
> simplify pci_size() considerably.
>
> The new check is a bit more strict as it doesn't consider the value the
> BAR register holds initially and bails out iff the maximal base value
> isn't in the expected format.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pci/pci.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 837e17aa8397..72689560d598 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -144,18 +144,10 @@ static struct pci_dev *alloc_pci_dev(void)
>         return dev;
>  }
>
> -static u32 pci_size(u32 base, u32 maxbase, u32 mask)
> +static u32 pci_size(u32 maxbase)
>  {
> -       u32 size = maxbase & mask;
> -       if (!size)
> -               return 0;
> -
> -       size = (size & ~(size-1)) - 1;
> -
> -       if (base == maxbase && ((base | size) & mask) != mask)
> -               return 0;
> -
> -       return size + 1;
> +       /* isolate right-most set bit */
> +       return maxbase & ~(maxbase - 1);
>  }
>
>
> @@ -169,7 +161,7 @@ static void setup_device(struct pci_dev *dev, int max_bar)
>                               cmd & ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY));
>
>         for (bar = 0; bar < max_bar; bar++) {
> -               u32 orig, barval, size, mask;
> +               u32 orig, barval, maxbase, size, mask;
>                 const char *type;
>                 int restype;
>                 unsigned long resflags;
> @@ -206,13 +198,19 @@ static void setup_device(struct pci_dev *dev, int max_bar)
>                         last = &last_mem;
>                 }
>
> -               size = pci_size(orig, barval, mask);
> -               if (!size) {
> -                       pr_debug(" pbar%d: bad %s size (orig=%08x, barval=%08x, type=%s)\n",
> -                                bar, type, orig, barval, type);
> +               maxbase = barval & mask;
> +
> +               /*
> +                * if maxbase isn't in the form 0b1...10...0 there is something
> +                * fishy
> +                */
> +               if ((maxbase | (maxbase - 1)) != (u32)-1 || maxbase == 0) {

U32_MAX instead of (u32)-1 ?

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] pci: don't hide hard to understand sanity check in size calculation

[Uwe Kleine-König]

Hello Andrey,

On Sat, Apr 14, 2018 at 09:25:59AM -0700, Andrey Smirnov wrote:
> On Fri, Apr 13, 2018 at 4:30 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > +               /*
> > +                * if maxbase isn't in the form 0b1...10...0 there is something
> > +                * fishy
> > +                */
> > +               if ((maxbase | (maxbase - 1)) != (u32)-1 || maxbase == 0) {
> 
> U32_MAX instead of (u32)-1 ?

I don't feel strong here. Given that the LHS uses some characteristics
of the binary representation I slightly prefer (u32)-1, but I can change
it to U32_MAX, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] pci: don't hide hard to understand sanity check in size calculation

[Andrey Smirnov]

On Sun, Apr 15, 2018 at 11:27 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello Andrey,
>
> On Sat, Apr 14, 2018 at 09:25:59AM -0700, Andrey Smirnov wrote:
>> On Fri, Apr 13, 2018 at 4:30 PM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > +               /*
>> > +                * if maxbase isn't in the form 0b1...10...0 there is something
>> > +                * fishy
>> > +                */
>> > +               if ((maxbase | (maxbase - 1)) != (u32)-1 || maxbase == 0) {
>>
>> U32_MAX instead of (u32)-1 ?
>
> I don't feel strong here.

Me neither. I just read (u32)-1 as a shortcut for 0xffffffff and we
already have a shortcut for that. But if you want to keep using
(u32)-1, then by all means.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox