RE: [PATCH v6 1/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's

From: Tan, Jui Nee
Date: Mon Jul 18 2016 - 01:51:19 EST

Next message: Namhyung Kim: "Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver"
Previous message: Peter Chen: "Re: [PATCH V2] leds: trigger: Introduce an USB port trigger"
In reply to: Paul Gortmaker: "Re: [PATCH v6 1/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Tan, Jui Nee
> Sent: Monday, July 18, 2016 11:35 AM
> To: 'Paul Gortmaker' <paul.gortmaker@xxxxxxxxxxxxx>;
> andriy.shevchenko@xxxxxxxxxxxxxxx
> Cc: mika.westerberg@xxxxxxxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx;
> tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; H. Peter Anvin <hpa@xxxxxxxxx>;
> X86 ML <x86@xxxxxxxxxx>; ptyser@xxxxxxxxxxx; Lee Jones
> <lee.jones@xxxxxxxxxx>; Linus Walleij <linus.walleij@xxxxxxxxxx>; linux-
> gpio@xxxxxxxxxxxxxxx; LKML <linux-kernel@xxxxxxxxxxxxxxx>; Yong,
> Jonathan <jonathan.yong@xxxxxxxxx>; Yu, Ong Hock
> <ong.hock.yu@xxxxxxxxx>; Voon, Weifeng <weifeng.voon@xxxxxxxxx>; Wan
> Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>
> Subject: RE: [PATCH v6 1/3] x86/platform/p2sb: New Primary to Sideband
> bridge support driver for Intel SOC's
>
>
>
> > -----Original Message-----
> > From: paul.gortmaker@xxxxxxxxx [mailto:paul.gortmaker@xxxxxxxxx] On
> > Behalf Of Paul Gortmaker
> > Sent: Friday, July 15, 2016 8:01 AM
> > To: Tan, Jui Nee <jui.nee.tan@xxxxxxxxx>
> > Cc: mika.westerberg@xxxxxxxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx;
> > andriy.shevchenko@xxxxxxxxxxxxxxx; tglx@xxxxxxxxxxxxx;
> > mingo@xxxxxxxxxx; H. Peter Anvin <hpa@xxxxxxxxx>; X86 ML
> > <x86@xxxxxxxxxx>; ptyser@xxxxxxxxxxx; Lee Jones
> <lee.jones@xxxxxxxxxx>;
> > Linus Walleij <linus.walleij@xxxxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx; LKML
> > <linux-kernel@xxxxxxxxxxxxxxx>; Yong, Jonathan
> > <jonathan.yong@xxxxxxxxx>; Yu, Ong Hock <ong.hock.yu@xxxxxxxxx>;
> Voon,
> > Weifeng <weifeng.voon@xxxxxxxxx>; Wan Mohamad, Wan Ahmad Zainie
> > <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>
> > Subject: Re: [PATCH v6 1/3] x86/platform/p2sb: New Primary to Sideband
> > bridge support driver for Intel SOC's
> >
> > On Thu, Jul 14, 2016 at 4:11 AM, Tan Jui Nee <jui.nee.tan@xxxxxxxxx>
> wrote:
> > > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > >
> > > There is already one and at least one more user coming which require
> > > an access to Primary to Sideband bridge (P2SB) in order to get IO or
> > > MMIO bar hidden by BIOS.
> > > Create a driver to access P2SB for x86 devices.
> > >
> > > Signed-off-by: Yong, Jonathan <jonathan.yong@xxxxxxxxx>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > ---
> > > Changes in V6:
> > > - No change
> > >
> > > Changes in V5:
> > > - No change
> > >
> > > Changes in V4:
> > > - Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from
> > > [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge
> > support driver for Intel SOC's
> > > to
> > > [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO
> > pinctrl in non-ACPI system
> > > since the config is used in latter patch.
> > >
> > > Changes in V3:
> > > - No change
> > >
> > > Changes in V2:
> > > - Add new config option CONFIG_X86_INTEL_NON_ACPI and "select
> > PINCTRL"
> > > to fix kbuildbot error
> > >
> > > arch/x86/Kconfig | 4 ++
> > > arch/x86/include/asm/p2sb.h | 27 +++++++++++
> > > arch/x86/platform/intel/Makefile | 1 +
> > > arch/x86/platform/intel/p2sb.c | 99
> > ++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 131 insertions(+)
> > > create mode 100644 arch/x86/include/asm/p2sb.h create mode 100644
> > > arch/x86/platform/intel/p2sb.c
> > >
> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index
> > > d9a94da..d305d81 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -604,6 +604,10 @@ config IOSF_MBI_DEBUG
> > >
> > > If you don't require the option or are in doubt, say N.
> > >
> > > +config P2SB
> > > + tristate
> >
> > OK, this is tristate, but then....
> >
> P2SB is tristate as currently it is only used by LPC_ICH that is tristate too.
> ...
> config LPC_ICH
> tristate "Intel ICH LPC"
> depends on X86 && PCI
> select MFD_CORE
> select P2SB
> ...
> > > + depends on PCI
> > > +
> > > config X86_RDC321X
> > > bool "RDC R-321x SoC"
> > > depends on X86_32
> > > diff --git a/arch/x86/include/asm/p2sb.h b/arch/x86/include/asm/p2sb.h
> > > new file mode 100644 index 0000000..686e07b
> > > --- /dev/null
> > > +++ b/arch/x86/include/asm/p2sb.h
> > > @@ -0,0 +1,27 @@
> > > +/*
> > > + * Primary to Sideband bridge (P2SB) access support */
> > > +
> > > +#ifndef P2SB_SYMS_H
> > > +#define P2SB_SYMS_H
> > > +
> > > +#include <linux/ioport.h>
> > > +#include <linux/pci.h>
> > > +
> > > +#if IS_ENABLED(CONFIG_P2SB)
> > > +
> > > +int p2sb_bar(struct pci_dev *pdev, unsigned int devfn,
> > > + struct resource *res);
> > > +
> > > +#else /* CONFIG_P2SB is not set */
> > > +
> > > +static inline
> > > +int p2sb_bar(struct pci_dev *pdev, unsigned int devfn,
> > > + struct resource *res)
> > > +{
> > > + return -ENODEV;
> > > +}
> > > +
> > > +#endif /* CONFIG_P2SB */
> > > +
> > > +#endif /* P2SB_SYMS_H */
> > > diff --git a/arch/x86/platform/intel/Makefile
> > > b/arch/x86/platform/intel/Makefile
> > > index b878032..dbf9f10 100644
> > > --- a/arch/x86/platform/intel/Makefile
> > > +++ b/arch/x86/platform/intel/Makefile
> > > @@ -1 +1,2 @@
> > > obj-$(CONFIG_IOSF_MBI) += iosf_mbi.o
> > > +obj-$(CONFIG_P2SB) += p2sb.o
> > > diff --git a/arch/x86/platform/intel/p2sb.c
> > > b/arch/x86/platform/intel/p2sb.c new file mode 100644 index
> > > 0000000..8be47a4
> > > --- /dev/null
> > > +++ b/arch/x86/platform/intel/p2sb.c
> > > @@ -0,0 +1,99 @@
> > > +/*
> > > + * Primary to Sideband bridge (P2SB) driver
> > > + *
> > > + * Copyright (c) 2016, Intel Corporation.
> > > + *
> > > + * Authors: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > + * Jonathan Yong <jonathan.yong@xxxxxxxxx>
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > +modify it
> > > + * under the terms and conditions of the GNU General Public License,
> > > + * version 2, as published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope it will be useful, but
> > > +WITHOUT
> > > + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY
> > > +or
> > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> > > +License for
> > > + * more details.
> > > + *
> > > + */
> > > +
> > > +#include <linux/ioport.h>
> > > +#include <linux/module.h>
> >
> > ...and module.h is included, but yet...
> >
> > > +#include <linux/pci.h>
> > > +#include <linux/spinlock.h>
> > > +
> > > +#include <asm/p2sb.h>
> > > +
> > > +#define SBREG_BAR 0x10
> > > +#define SBREG_HIDE 0xe1
> > > +
> > > +static DEFINE_SPINLOCK(p2sb_spinlock);
> > > +
> > > +/*
> > > + * p2sb_bar - Get Primary to Sideband bridge (P2SB) BAR
> > > + * @pdev: PCI device to get PCI bus to communicate with
> > > + * @devfn: PCI device and function to communicate with
> > > + * @res: resources to be filled in
> > > + *
> > > + * The BIOS prevents the P2SB device from being enumerated by the
> PCI
> > > + * subsystem, so we need to unhide and hide it back to lookup the P2SB
> > BAR.
> > > + *
> > > + * Locking is handled by spinlock - cannot sleep.
> > > + *
> > > + * Return:
> > > + * 0 on success or appropriate errno value on error.
> > > + */
> > > +int p2sb_bar(struct pci_dev *pdev, unsigned int devfn,
> > > + struct resource *res)
> > > +{
> > > + u32 base_addr;
> > > + u64 base64_addr;
> > > + unsigned long flags;
> > > +
> > > + if (!res)
> > > + return -EINVAL;
> > > +
> > > + spin_lock(&p2sb_spinlock);
> > > +
> > > + /* Unhide the P2SB device */
> > > + pci_bus_write_config_byte(pdev->bus, devfn, SBREG_HIDE, 0x00);
> > > +
> > > + /* Check if device present */
> > > + pci_bus_read_config_dword(pdev->bus, devfn, 0, &base_addr);
> > > + if (base_addr == 0xffffffff || base_addr == 0x00000000) {
> > > + spin_unlock(&p2sb_spinlock);
> > > + dev_warn(&pdev->dev, "P2SB device access disabled by
> > BIOS?\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + /* Get IO or MMIO BAR */
> > > + pci_bus_read_config_dword(pdev->bus, devfn, SBREG_BAR,
> > &base_addr);
> > > + if ((base_addr & PCI_BASE_ADDRESS_SPACE) ==
> > PCI_BASE_ADDRESS_SPACE_IO) {
> > > + flags = IORESOURCE_IO;
> > > + base64_addr = base_addr & PCI_BASE_ADDRESS_IO_MASK;
> > > + } else {
> > > + flags = IORESOURCE_MEM;
> > > + base64_addr = base_addr & PCI_BASE_ADDRESS_MEM_MASK;
> > > + if (base_addr & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > > + flags |= IORESOURCE_MEM_64;
> > > + pci_bus_read_config_dword(pdev->bus, devfn,
> > > + SBREG_BAR + 4, &base_addr);
> > > + base64_addr |= (u64)base_addr << 32;
> > > + }
> > > + }
> > > +
> > > + /* Hide the P2SB device */
> > > + pci_bus_write_config_byte(pdev->bus, devfn, SBREG_HIDE, 0x01);
> > > +
> > > + spin_unlock(&p2sb_spinlock);
> > > +
> > > + /* User provides prefilled resources */
> > > + res->start += (resource_size_t)base64_addr;
> > > + res->end += (resource_size_t)base64_addr;
> > > + res->flags = flags;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL(p2sb_bar);
> > > +
> > > +MODULE_LICENSE("GPL");
> >
> > ...the above is the only modular "use" that I can find. So is the
> > tristate bogus? Without a module_init and a module_exit I am
> > confused....
> >
> > I just finished an audit of arch/x86 for bogus uses of module.h so I'd like to
> > ensure we don't add more.
> >
> > Thanks,
> > Paul.
> > --
> >
> P2SB could be "bool" instead of tristate.
> My concern is if LPC_ICH built as module and not loaded, P2SB might
> consume memory when P2SB is not being used.
What I meant to say is, when LPC_ICH is built but not use by the current
hardware, the P2SB compiled code will occupy memory and cannot be
unloaded even if it is not in use. If memory usage is not the concern, I will
rework this patch to use Boolean option.
> What do you think? If that's ok for you, my next patch will be something like
> this:
> ...
> config P2SB
> bool
> depends on PCI
> ...
> In p2sb.c, module.h header file will be removed as well.
> Hi Andy, please provide your comments and/or concerns if any. Thanks.
> > > --
> > > 1.9.1
> > >

Next message: Namhyung Kim: "Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver"
Previous message: Peter Chen: "Re: [PATCH V2] leds: trigger: Introduce an USB port trigger"
In reply to: Paul Gortmaker: "Re: [PATCH v6 1/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]