diff mbox series

[v2,09/39] x86: apl: Move p2sb ofdata reading to the correct method

Message ID 20200308214442.v2.9.Iff8dfbeef5f76f776cf3a84807b0ff3fd5a491ac@changeid
State Superseded
Headers show
Series dm: Add programmatic generation of ACPI tables (part A) | expand

Commit Message

Simon Glass March 9, 2020, 3:44 a.m. UTC 
With P2SB the initial BAR (base-address register) is set up by TPL and
this is used unchanged right through U-Boot.

At present the reading of this address is split between the ofdata() and
probe() methods. There are a few problems that are unique to the p2sb.
One is that its children need to call pcr_read32(), etc. which needs to
have the p2sb address correct. Also some of its children are pinctrl
devices and pinctrl is used when any device is probed. So p2sb really
needs to get its base address set up in ofdata_to_platdata(), before it is
probed.

Another point is that reading the p2sb BAR will not work if the p2sb is
hidden. The FSP-S seems to hide it, presumably to avoid confusing PCI
enumeration.

Reading ofdata in ofdata_to_platdata() is the correct place anyway, so
this is easy to fix.

Move the code into one place and use the early-regs property in all cases
for simplicity and to avoid needing to probe any PCI devices just to read
the BAR.

Signed-off-by: Simon Glass <sjg at chromium.org>
Reviewed-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com>
Tested-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com>
---

Changes in v2: None

 arch/x86/cpu/intel_common/p2sb.c | 35 +++++++++++---------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

Comments

Andy Shevchenko March 10, 2020, 2:39 p.m. UTC | #1 
On Sun, Mar 08, 2020 at 09:44:33PM -0600, Simon Glass wrote:
> With P2SB the initial BAR (base-address register) is set up by TPL and
> this is used unchanged right through U-Boot.
> 
> At present the reading of this address is split between the ofdata() and
> probe() methods. There are a few problems that are unique to the p2sb.
> One is that its children need to call pcr_read32(), etc. which needs to
> have the p2sb address correct. Also some of its children are pinctrl
> devices and pinctrl is used when any device is probed. So p2sb really
> needs to get its base address set up in ofdata_to_platdata(), before it is
> probed.
> 
> Another point is that reading the p2sb BAR will not work if the p2sb is
> hidden. The FSP-S seems to hide it, presumably to avoid confusing PCI
> enumeration.
> 
> Reading ofdata in ofdata_to_platdata() is the correct place anyway, so
> this is easy to fix.
> 
> Move the code into one place and use the early-regs property in all cases
> for simplicity and to avoid needing to probe any PCI devices just to read
> the BAR.

>  		if (plat->bdf < 0)
>  			return log_msg_ret("Cannot get p2sb PCI address",
> -					   plat->bdf);
> +						plat->bdf);

Not sure I understand this hunk WRT the patch itself.

> +	if (spl_phase() == PHASE_TPL)
>  		return p2sb_early_init(dev);

> +	else if (spl_phase() == PHASE_SPL)

Redundant 'else', but I think we already discussed that and you prefer this
way. However, I think this is waste of compilation time. In any case, matter
of taste.

> +		return p2sb_spl_init(dev);
Simon Glass March 11, 2020, 12:17 p.m. UTC | #2 
Hi Andy,

On Tue, 10 Mar 2020 at 08:39, Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
>
> On Sun, Mar 08, 2020 at 09:44:33PM -0600, Simon Glass wrote:
> > With P2SB the initial BAR (base-address register) is set up by TPL and
> > this is used unchanged right through U-Boot.
> >
> > At present the reading of this address is split between the ofdata() and
> > probe() methods. There are a few problems that are unique to the p2sb.
> > One is that its children need to call pcr_read32(), etc. which needs to
> > have the p2sb address correct. Also some of its children are pinctrl
> > devices and pinctrl is used when any device is probed. So p2sb really
> > needs to get its base address set up in ofdata_to_platdata(), before it is
> > probed.
> >
> > Another point is that reading the p2sb BAR will not work if the p2sb is
> > hidden. The FSP-S seems to hide it, presumably to avoid confusing PCI
> > enumeration.
> >
> > Reading ofdata in ofdata_to_platdata() is the correct place anyway, so
> > this is easy to fix.
> >
> > Move the code into one place and use the early-regs property in all cases
> > for simplicity and to avoid needing to probe any PCI devices just to read
> > the BAR.
>
> >               if (plat->bdf < 0)
> >                       return log_msg_ret("Cannot get p2sb PCI address",
> > -                                        plat->bdf);
> > +                                             plat->bdf);
>
> Not sure I understand this hunk WRT the patch itself.

This is to fix a checkpatch error - it reports problems in nearby lines.

>
> > +     if (spl_phase() == PHASE_TPL)
> >               return p2sb_early_init(dev);
>
> > +     else if (spl_phase() == PHASE_SPL)
>
> Redundant 'else', but I think we already discussed that and you prefer this
> way. However, I think this is waste of compilation time. In any case, matter
> of taste.

Yes. I feel that it shows that there are two options.

Just for fun I wrote a program to try to benchmark the difference and
it does not seem to be detectable.

>
> > +             return p2sb_spl_init(dev);

Regards,
Simon
Andy Shevchenko March 11, 2020, 1:06 p.m. UTC | #3 
On Wed, Mar 11, 2020 at 06:17:32AM -0600, Simon Glass wrote:
> On Tue, 10 Mar 2020 at 08:39, Andy Shevchenko
> <andriy.shevchenko at linux.intel.com> wrote:
> > On Sun, Mar 08, 2020 at 09:44:33PM -0600, Simon Glass wrote:

> > >               if (plat->bdf < 0)
> > >                       return log_msg_ret("Cannot get p2sb PCI address",
> > > -                                        plat->bdf);
> > > +                                             plat->bdf);
> >
> > Not sure I understand this hunk WRT the patch itself.
> 
> This is to fix a checkpatch error - it reports problems in nearby lines.

So, checkpatch insist on wrong indentation?!

> > > +     if (spl_phase() == PHASE_TPL)
> > >               return p2sb_early_init(dev);
> >
> > > +     else if (spl_phase() == PHASE_SPL)
> >
> > Redundant 'else', but I think we already discussed that and you prefer this
> > way. However, I think this is waste of compilation time. In any case, matter
> > of taste.
> 
> Yes. I feel that it shows that there are two options.
> 
> Just for fun I wrote a program to try to benchmark the difference and
> it does not seem to be detectable.

Compiler optimizes it away in any case, so, you won't see any difference.

> > > +             return p2sb_spl_init(dev);
diff mbox series

Patch

diff --git a/arch/x86/cpu/intel_common/p2sb.c b/arch/x86/cpu/intel_common/p2sb.c
index d5b4846e0a..4af55786e6 100644
--- a/arch/x86/cpu/intel_common/p2sb.c
+++ b/arch/x86/cpu/intel_common/p2sb.c
@@ -92,46 +92,35 @@  int p2sb_ofdata_to_platdata(struct udevice *dev)
 
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
 	int ret;
+	u32 base[2];
 
+	ret = dev_read_u32_array(dev, "early-regs", base, ARRAY_SIZE(base));
+	if (ret)
+		return log_msg_ret("Missing/short early-regs", ret);
+	plat->mmio_base = base[0];
+	/* TPL sets up the initial BAR */
 	if (spl_phase() == PHASE_TPL) {
-		u32 base[2];
-
-		/* TPL sets up the initial BAR */
-		ret = dev_read_u32_array(dev, "early-regs", base,
-					 ARRAY_SIZE(base));
-		if (ret)
-			return log_msg_ret("Missing/short early-regs", ret);
-		plat->mmio_base = base[0];
 		plat->bdf = pci_get_devfn(dev);
 		if (plat->bdf < 0)
 			return log_msg_ret("Cannot get p2sb PCI address",
-					   plat->bdf);
+						plat->bdf);
 	}
+	upriv->mmio_base = plat->mmio_base;
 #else
 	plat->mmio_base = plat->dtplat.early_regs[0];
 	plat->bdf = pci_ofplat_get_devfn(plat->dtplat.reg[0]);
-#endif
 	upriv->mmio_base = plat->mmio_base;
-	debug("p2sb: mmio_base=%x\n", (uint)plat->mmio_base);
+#endif
 
 	return 0;
 }
 
 static int p2sb_probe(struct udevice *dev)
 {
-	if (spl_phase() == PHASE_TPL) {
+	if (spl_phase() == PHASE_TPL)
 		return p2sb_early_init(dev);
-	} else {
-		struct p2sb_platdata *plat = dev_get_platdata(dev);
-
-		plat->mmio_base = dev_read_addr_pci(dev);
-		/* Don't set BDF since it should not be used */
-		if (!plat->mmio_base || plat->mmio_base == FDT_ADDR_T_NONE)
-			return -EINVAL;
-
-		if (spl_phase() == PHASE_SPL)
-			return p2sb_spl_init(dev);
-	}
+	else if (spl_phase() == PHASE_SPL)
+		return p2sb_spl_init(dev);
 
 	return 0;
 }