[U-Boot] [PATCH v3 036/108] pci: Add support for p2sb uclass

Thu Nov 21 13:50:05 UTC 2019

Hi Bin,

On Mon, 4 Nov 2019 at 00:23, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Oct 21, 2019 at 11:40 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > The Primary-to-Sideband bus (P2SB) is used to access various peripherals
> > through memory-mapped I/O in a large chunk of PCI space. The space is
> > segmented into different channels and peripherals are accessed by
> > device-specific means within those channels. Devices should be added in
> > the device tree as subnodes of the p2sb.
> >
> > This adds a uclass and enables it for sandbox.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> >  configs/sandbox_defconfig     |   1 +
> >  configs/sandbox_spl_defconfig |   1 +
> >  drivers/misc/Kconfig          |  33 ++++++
> >  drivers/misc/Makefile         |   1 +
> >  drivers/misc/p2sb-uclass.c    | 209 ++++++++++++++++++++++++++++++++++
> >  include/dm/uclass-id.h        |   1 +
> >  include/p2sb.h                | 127 +++++++++++++++++++++
> >  7 files changed, 373 insertions(+)
> >  create mode 100644 drivers/misc/p2sb-uclass.c
> >  create mode 100644 include/p2sb.h
> >
> > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > index 20ebc68997b..f8b78b8d7e1 100644
> > --- a/configs/sandbox_defconfig
> > +++ b/configs/sandbox_defconfig
> > @@ -150,6 +150,7 @@ CONFIG_PCI=y
> >  CONFIG_DM_PCI=y
> >  CONFIG_DM_PCI_COMPAT=y
> >  CONFIG_PCI_SANDBOX=y
> > +CONFIG_P2SB=y
> >  CONFIG_PHY=y
> >  CONFIG_PHY_SANDBOX=y
> >  CONFIG_PINCTRL=y
> > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
> > index 409b8a38d5e..c49e05ec319 100644
> > --- a/configs/sandbox_spl_defconfig
> > +++ b/configs/sandbox_spl_defconfig
> > @@ -135,6 +135,7 @@ CONFIG_PCI=y
> >  CONFIG_DM_PCI=y
> >  CONFIG_DM_PCI_COMPAT=y
> >  CONFIG_PCI_SANDBOX=y
> > +CONFIG_P2SB=y
> >  CONFIG_PHY=y
> >  CONFIG_PHY_SANDBOX=y
> >  CONFIG_PINCTRL=y
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index ba50893b432..1ed2e5d4aa2 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -217,6 +217,39 @@ config NUVOTON_NCT6102D
> >           disable the legacy UART, the watchdog or other devices
> >           in the Nuvoton Super IO chips on X86 platforms.
> >
> > +config P2SB
> > +       bool "Intel Primary-to-Sideband Bus"
> > +       depends on X86 || SANDBOX
> > +       help
> > +         This enables support for the Intel Primary-to-Sideband bus,
> > +         abbreviated to P2SB. The P2SB is used to access various peripherals
> > +         such as eSPI, GPIO, through memory-mapped I/O in a large chunk of PCI
> > +         space. The space is segmented into different channels and peripherals
> > +         are accessed by device-specific means within those channels. Devices
> > +         should be added in the device tree as subnodes of the P2SB. A
> > +         Peripheral Channel Register? (PCR) API is provided to access those
>
> What does the ? mean? We are not sure what PCR stands for?

That's right. I cannot actually find a reference. The abbreviations
for quite a few things seems to be used without a definition.

[..]

> > diff --git a/include/p2sb.h b/include/p2sb.h
> > new file mode 100644
> > index 00000000000..370f127058c
> > --- /dev/null
> > +++ b/include/p2sb.h
> > @@ -0,0 +1,127 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2019 Google LLC
> > + * Written by Simon Glass <sjg at chromium.org>
> > + */
> > +
> > +#ifndef __p2sb_h
> > +#define __p2sb_h
> > +
> > +/* Port Id lives in bits 23:16 and register offset lives in 15:0 of address */
> > +#define PCR_PORTID_SHIFT       16
> > +
> > +/**
> > + * struct p2sb_child_platdata - Information about each child of a p2sb device
> > + *
> > + * @pid: Port ID for this child
> > + */
> > +struct p2sb_child_platdata {
> > +       uint pid;
> > +};
> > +
> > +/**
> > + * struct p2sb_uc_priv - information for the uclass about each device
> > + *
> > + * This must be set up by the driver when it is probed
> > + *
> > + * @mmio_base: Base address of P2SB region
> > + */
> > +struct p2sb_uc_priv {
> > +       uint mmio_base;
> > +};
> > +
> > +/**
> > + * struct p2sb_ops - Operations for the P2SB (none at present)
> > + */
> > +struct p2sb_ops {
> > +};
> > +
> > +#define p2sb_get_ops(dev)        ((struct p2sb_ops *)(dev)->driver->ops)
> > +
> > +/**
> > + * pcr_read32/16/8() - Read from a PCR device
> > + *
> > + * Reads data from a PCR device within the P2SB
> > + *
> > + * @dev: Device to read from
> > + * @offset: Offset within device to read
> > + * @return value read
> > + */
> > +uint pcr_read32(struct udevice *dev, uint offset);
> > +uint pcr_read16(struct udevice *dev, uint offset);
> > +uint pcr_read8(struct udevice *dev, uint offset);
> > +
> > +/**
> > + * pcr_read32/16/8() - Write to a PCR device
> > + *
> > + * Writes data to a PCR device within the P2SB
>
> Read data
>
> > + *
> > + * @dev: Device to write to
> > + * @offset: Offset within device to write
> > + * @data: Data to write
> > + */
> > +void pcr_write32(struct udevice *dev, uint offset, uint data);
> > +void pcr_write16(struct udevice *dev, uint offset, uint data);
> > +void pcr_write8(struct udevice *dev, uint offset, uint data);
> > +
> > +/**
> > + * pcr_clrsetbits32/16/8() - Update a PCR device
> > + *
> > + * Updates dat in a PCR device within the P2SB
> > + *
> > + * This reads from the device, clears and set bits, then writes back.
> > + *
> > + * new_data = (old_data & ~clr) | set
> > + *
> > + * @dev: Device to update
> > + * @offset: Offset within device to update
> > + * @clr: Bits to clear after reading
> > + * @set: Bits to set before writing
> > + */
> > +void pcr_clrsetbits32(struct udevice *dev, uint offset, uint clr, uint set);
> > +void pcr_clrsetbits16(struct udevice *dev, uint offset, uint clr, uint set);
> > +void pcr_clrsetbits8(struct udevice *dev, uint offset, uint clr, uint set);
> > +
> > +static inline void pcr_setbits32(struct udevice *dev, uint offset, uint set)
> > +{
> > +       return pcr_clrsetbits32(dev, offset, 0, set);
> > +}
> > +
> > +static inline void pcr_setbits16(struct udevice *dev, uint offset, uint set)
> > +{
> > +       return pcr_clrsetbits16(dev, offset, 0, set);
> > +}
> > +
> > +static inline void pcr_setbits8(struct udevice *dev, uint offset, uint set)
> > +{
> > +       return pcr_clrsetbits8(dev, offset, 0, set);
> > +}
> > +
> > +static inline void pcr_clrbits32(struct udevice *dev, uint offset, uint clr)
> > +{
> > +       return pcr_clrsetbits32(dev, offset, clr, 0);
> > +}
> > +
> > +static inline void pcr_clrbits16(struct udevice *dev, uint offset, uint clr)
> > +{
> > +       return pcr_clrsetbits16(dev, offset, clr, 0);
> > +}
> > +
> > +static inline void pcr_clrbits8(struct udevice *dev, uint offset, uint clr)
> > +{
> > +       return pcr_clrsetbits8(dev, offset, clr, 0);
> > +}
> > +
> > +/**
> > + * p2sb_set_port_id() - Set the port ID for a p2sb child device
> > + *
> > + * This must be called in a device's bind() method when OF_PLATDATA is used
> > + * since the uclass cannot access the device's of-platdata.
> > + *
> > + * @dev: Child device (whose parent is UCLASS_P2SB)
> > + * @portid: Port ID of child device
> > + * @return 0 if OK, -ENODEV is the p2sb device could not be found
> > + */
> > +int p2sb_set_port_id(struct udevice *dev, int portid);
> > +
> > +#endif
> > --
>
> I still are not sure P2SB is generic enough to merit a uclass driver.

If you look at the device tree it works really well. It is a bit
similar to a simple-bus but has its own functionality, such as
children accessing their memory-mapped regions under control of the
uclass. In that it is similar to i2c or PCI, which have uclasses. We
have child nodes of the p2sb, and in the case of pinctrl they have
grandchild nodes (in my latest update, not v3).

It is true that this is specific to x86, but I don't see a problem
with that. We have other uclasses that are arch-specific.

Regards,
Simon