Re: [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass
On Thu, Apr 25, 2019 at 2:44 AM Simon Glass wrote:
> HI Ramon,
>
> On Mon, 22 Apr 2019 at 10:33, Ramon Fried wrote:
> >
> >
> > Hi Simon,
> > Thanks for the review.
> > please see inline, I have few questions/suggestions regarding
> > your notes.
> >
> > Thanks,
> > Ramon.
> > On Mon, Apr 22, 2019 at 5:56 AM Simon Glass wrote:
> >>
> >> Hi Ramon,
> >>
> >> On Fri, 5 Apr 2019 at 19:12, Ramon Fried wrote:
> >> >
> >> > Introduce new UCLASS_PCI_EP class for handling PCI endpoint
> >> > devices, allowing to set various attributes of the PCI endpoint
> >> > device, such as:
> >> > * configuration space header
> >> > * BAR definitions
> >> > * outband memory mapping
> >> > * start/stop PCI link
> >> >
> >> > Signed-off-by: Ramon Fried
> >> >
> >> > ---
> >> >
> >> > drivers/Kconfig | 2 +
> >> > drivers/Makefile | 1 +
> >> > drivers/pci_endpoint/Kconfig | 16 ++
> >> > drivers/pci_endpoint/Makefile| 6 +
> >> > drivers/pci_endpoint/pci_ep-uclass.c | 192 ++
> >> > include/dm/uclass-id.h | 1 +
> >> > include/pci_ep.h | 375
> +++
> >> > 7 files changed, 593 insertions(+)
> >> > create mode 100644 drivers/pci_endpoint/Kconfig
> >> > create mode 100644 drivers/pci_endpoint/Makefile
> >> > create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c
> >> > create mode 100644 include/pci_ep.h
> >>
>
> [..]
>
> >> > +int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no,
> >>
> >> Please use uint for func_no. There is no need to limit it to 8 bits,
> >> and this may not be efficient for non-8-bit machines. Please fix
> >> globally.
> >
> > I tried to keep the API as similar as it can to the Linux API.
> > I can change it, no problem, but IMHO I think it's better to keep it
> similar.
>
> Hmm, why?
>
Nevermind, I already changed my mind :)
>
> >> Perhaps you should declare a 'mask' variable? In any case, it is
> >> confusing for the same variable to have two different meanings, and it
> >> does not save code at run-time.
> >
> > as before, this is practically a copy-paste from Linux, I can change it,
> but I think it's clearer if the code looks the same as in Linux,
> > might be easier the port patches like that.
>
> It's a minor thing so you can keep the linux code if you like.
>
> [..]
>
> >> > +int dm_pci_ep_get_msix(struct udevice *dev, u8 func_no)
> >> > +{
> >> > + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
> >> > + int interrupt;
> >> > +
> >> > + if (!ops->get_msix)
> >> > + return -ENODEV;
> >> > +
> >> > + interrupt = ops->get_msix(dev, func_no);
> >> > +
> >> > + if (interrupt < 0)
> >> > + return 0;
> >> > +
> >> > + return interrupt + 1;
> >>
> >> It's odd that your uclass function returns a different value from your
> >> driver function. I think that will be confusing. Is it necessary?
> >
> > As you can understand from the code, 0 means 1 interrupt. basically the
> driver functions just return the msix field in the PCI registers,
> > The translation to real number is done by the uclass wrapper.
>
> OK, but why? Why not use the same return value for the drive methods
> and the uclass? If you are using a different API, then please rename
> one of the functions.
>
I'm here advocating for the Linux implementation, you're right, I'll change
it.
>
> [..]
>
> >> What is a DWORD? I think i is 32-bits, so just say that, since WORD is
> >> a confusing term on a non-16-bit machine.
> >
> > hehe, I presume it's just a copy-paste from the PCI spec. I'll check, if
> so, I'll prefer to keep the original naming.
>
> OK, then how about adding the length as well, since DWORD is pretty
> obscure these days.
>
OK, will do.
>
> >
> >
> >>
> >>
> >> > + * @subsys_vendor_id: vendor of the add-in card or subsystem
> >> > + * @subsys_id: id specific to vendor
> >> > + * @interrupt_pin: interrupt pin the device (or device function) uses
> >> > + */
> >> > +struct pci_ep_header {
> >> > + u16 vendorid;
> >> > + u16 deviceid;
> >> > + u8 revid;
> >> > + u8 progif_code;
> >> > + u8 subclass_code;
> >> > + u8 baseclass_code;
> >> > + u8 cache_line_size;
> >> > + u16 subsys_vendor_id;
> >> > + u16 subsys_id;
> >> > + enum pci_interrupt_pin interrupt_pin;
> >>
> >> Shouldn't that be a u16 or something? The enum does not have a defined
> >> size, I think.
> >
> > well, there can be only 4 different interrupt pins, so it doesn't matter
> as long as the rage
> > is checked.
> >
>
> My concern is that if this is a hardware-mapped register, then the
> enum could be any size, and may not map over the correct bits.
>
> If this is not a hardware-mapped register, then OK.
>
Not mapped, phew.
>
> [..]
>
> >> > + /**
> >> > +* set_msix() - set msix capability property
> >> > +*
> >> > +* set the number
Re: [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass
HI Ramon,
On Mon, 22 Apr 2019 at 10:33, Ramon Fried wrote:
>
>
> Hi Simon,
> Thanks for the review.
> please see inline, I have few questions/suggestions regarding
> your notes.
>
> Thanks,
> Ramon.
> On Mon, Apr 22, 2019 at 5:56 AM Simon Glass wrote:
>>
>> Hi Ramon,
>>
>> On Fri, 5 Apr 2019 at 19:12, Ramon Fried wrote:
>> >
>> > Introduce new UCLASS_PCI_EP class for handling PCI endpoint
>> > devices, allowing to set various attributes of the PCI endpoint
>> > device, such as:
>> > * configuration space header
>> > * BAR definitions
>> > * outband memory mapping
>> > * start/stop PCI link
>> >
>> > Signed-off-by: Ramon Fried
>> >
>> > ---
>> >
>> > drivers/Kconfig | 2 +
>> > drivers/Makefile | 1 +
>> > drivers/pci_endpoint/Kconfig | 16 ++
>> > drivers/pci_endpoint/Makefile| 6 +
>> > drivers/pci_endpoint/pci_ep-uclass.c | 192 ++
>> > include/dm/uclass-id.h | 1 +
>> > include/pci_ep.h | 375 +++
>> > 7 files changed, 593 insertions(+)
>> > create mode 100644 drivers/pci_endpoint/Kconfig
>> > create mode 100644 drivers/pci_endpoint/Makefile
>> > create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c
>> > create mode 100644 include/pci_ep.h
>>
[..]
>> > +int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no,
>>
>> Please use uint for func_no. There is no need to limit it to 8 bits,
>> and this may not be efficient for non-8-bit machines. Please fix
>> globally.
>
> I tried to keep the API as similar as it can to the Linux API.
> I can change it, no problem, but IMHO I think it's better to keep it similar.
Hmm, why?
>> Perhaps you should declare a 'mask' variable? In any case, it is
>> confusing for the same variable to have two different meanings, and it
>> does not save code at run-time.
>
> as before, this is practically a copy-paste from Linux, I can change it, but
> I think it's clearer if the code looks the same as in Linux,
> might be easier the port patches like that.
It's a minor thing so you can keep the linux code if you like.
[..]
>> > +int dm_pci_ep_get_msix(struct udevice *dev, u8 func_no)
>> > +{
>> > + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
>> > + int interrupt;
>> > +
>> > + if (!ops->get_msix)
>> > + return -ENODEV;
>> > +
>> > + interrupt = ops->get_msix(dev, func_no);
>> > +
>> > + if (interrupt < 0)
>> > + return 0;
>> > +
>> > + return interrupt + 1;
>>
>> It's odd that your uclass function returns a different value from your
>> driver function. I think that will be confusing. Is it necessary?
>
> As you can understand from the code, 0 means 1 interrupt. basically the
> driver functions just return the msix field in the PCI registers,
> The translation to real number is done by the uclass wrapper.
OK, but why? Why not use the same return value for the drive methods
and the uclass? If you are using a different API, then please rename
one of the functions.
[..]
>> What is a DWORD? I think i is 32-bits, so just say that, since WORD is
>> a confusing term on a non-16-bit machine.
>
> hehe, I presume it's just a copy-paste from the PCI spec. I'll check, if so,
> I'll prefer to keep the original naming.
OK, then how about adding the length as well, since DWORD is pretty
obscure these days.
>
>
>>
>>
>> > + * @subsys_vendor_id: vendor of the add-in card or subsystem
>> > + * @subsys_id: id specific to vendor
>> > + * @interrupt_pin: interrupt pin the device (or device function) uses
>> > + */
>> > +struct pci_ep_header {
>> > + u16 vendorid;
>> > + u16 deviceid;
>> > + u8 revid;
>> > + u8 progif_code;
>> > + u8 subclass_code;
>> > + u8 baseclass_code;
>> > + u8 cache_line_size;
>> > + u16 subsys_vendor_id;
>> > + u16 subsys_id;
>> > + enum pci_interrupt_pin interrupt_pin;
>>
>> Shouldn't that be a u16 or something? The enum does not have a defined
>> size, I think.
>
> well, there can be only 4 different interrupt pins, so it doesn't matter as
> long as the rage
> is checked.
>
My concern is that if this is a hardware-mapped register, then the
enum could be any size, and may not map over the correct bits.
If this is not a hardware-mapped register, then OK.
[..]
>> > + /**
>> > +* set_msix() - set msix capability property
>> > +*
>> > +* set the number of required MSIx vectors the device
>> > +* needs for operation.
>> > +*
>> > +* @dev:device to set
>> > +* @func_num: Function to set
>> > +* @interrupts: required interrupts count
>>
>> This is too vague, please expand it.
>
> You're referring to the set_msix() or the whole section,
> can you elaborate ?
I mean the interrupts line. Can you given example values perhaps? Does
it mean 'number of interrupts required to be
Re: [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass
Hi Simon,
Thanks for the review.
please see inline, I have few questions/suggestions regarding
your notes.
Thanks,
Ramon.
On Mon, Apr 22, 2019 at 5:56 AM Simon Glass wrote:
> Hi Ramon,
>
> On Fri, 5 Apr 2019 at 19:12, Ramon Fried wrote:
> >
> > Introduce new UCLASS_PCI_EP class for handling PCI endpoint
> > devices, allowing to set various attributes of the PCI endpoint
> > device, such as:
> > * configuration space header
> > * BAR definitions
> > * outband memory mapping
> > * start/stop PCI link
> >
> > Signed-off-by: Ramon Fried
> >
> > ---
> >
> > drivers/Kconfig | 2 +
> > drivers/Makefile | 1 +
> > drivers/pci_endpoint/Kconfig | 16 ++
> > drivers/pci_endpoint/Makefile| 6 +
> > drivers/pci_endpoint/pci_ep-uclass.c | 192 ++
> > include/dm/uclass-id.h | 1 +
> > include/pci_ep.h | 375 +++
> > 7 files changed, 593 insertions(+)
> > create mode 100644 drivers/pci_endpoint/Kconfig
> > create mode 100644 drivers/pci_endpoint/Makefile
> > create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c
> > create mode 100644 include/pci_ep.h
>
> This looks pretty good but I have a lot of nits, sorry.
>
> no worries.
> Please can you add a sandbox driver and a test for this (can be in a
> separate patch).
>
> sure.
> >
> > diff --git a/drivers/Kconfig b/drivers/Kconfig
> > index f24351ac4f..59e2c22cc6 100644
> > --- a/drivers/Kconfig
> > +++ b/drivers/Kconfig
> > @@ -64,6 +64,8 @@ source "drivers/nvme/Kconfig"
> >
> > source "drivers/pci/Kconfig"
> >
> > +source "drivers/pci_endpoint/Kconfig"
> > +
> > source "drivers/pch/Kconfig"
> >
> > source "drivers/pcmcia/Kconfig"
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index a7bba3ed56..480b97ef58 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -85,6 +85,7 @@ obj-$(CONFIG_FPGA) += fpga/
> > obj-y += misc/
> > obj-$(CONFIG_MMC) += mmc/
> > obj-$(CONFIG_NVME) += nvme/
> > +obj-$(CONFIG_PCI_ENDPOINT) += pci_endpoint/
> > obj-y += pcmcia/
> > obj-y += dfu/
> > obj-$(CONFIG_PCH) += pch/
> > diff --git a/drivers/pci_endpoint/Kconfig b/drivers/pci_endpoint/Kconfig
> > new file mode 100644
> > index 00..2c0a399a08
> > --- /dev/null
> > +++ b/drivers/pci_endpoint/Kconfig
> > @@ -0,0 +1,16 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# PCI Endpoint Support
> > +#
> > +
> > +menu "PCI Endpoint"
> > +
> > +config PCI_ENDPOINT
> > + bool "PCI Endpoint Support"
> > + depends on DM
> > + help
> > + Enable this configuration option to support configurable PCI
> > + endpoint. This should be enabled if the platform has a PCI
>
> s/endpoint/endpoints/ I think
>
> Right.
> > + controller that can operate in endpoint mode.
>
Can you explain a bit more about what this means. I understand that
> people can look at the spec, but what is the purpose of 'endpoint
> mode' and what does it enable?
>
> > +
> > +endmenu
> > diff --git a/drivers/pci_endpoint/Makefile
> b/drivers/pci_endpoint/Makefile
> > new file mode 100644
> > index 00..80a1066925
> > --- /dev/null
> > +++ b/drivers/pci_endpoint/Makefile
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +#
> > +# (C) Copyright 2019
> > +# Ramon Fried
> > +
> > +obj-y += pci_ep-uclass.o
> > diff --git a/drivers/pci_endpoint/pci_ep-uclass.c
> b/drivers/pci_endpoint/pci_ep-uclass.c
> > new file mode 100644
> > index 00..06fcfc5d14
> > --- /dev/null
> > +++ b/drivers/pci_endpoint/pci_ep-uclass.c
> > @@ -0,0 +1,192 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * PCI Endpoint uclass
> > + *
> > + * Based on Linux PCI-EP driver written by
> > + * Kishon Vijay Abraham I
> > + *
> > + * Copyright (c) 2019
> > + * Written by Ramon Fried
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +int dm_pci_ep_write_header(struct udevice *dev, u8 fn,
> > + struct pci_ep_header *hdr)
> > +{
> > + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
> > +
> > + if (!ops->write_header)
> > + return -ENODEV;
>
> -ENOSYS (please fix globally)
>
> OK.
> There is a device. The problem here is that the driver does not
> implement the method.
>
> > +
> > + return ops->write_header(dev, fn, hdr);
> > +}
> > +
> > +int dm_pci_ep_read_header(struct udevice *dev, u8 fn,
> > + struct pci_ep_header *hdr)
> > +{
> > + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
> > +
> > + if (!ops->read_header)
> > + return -ENODEV;
> > +
> > + return ops->read_header(dev, fn, hdr);
> > +}
> > +
> > +int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no,
>
> Please use uint for func_no. There is no need to limit it to 8 bits,
> and this may not be efficient for non-8-bit machines. Please fix
> globally.
Re: [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass
Hi Ramon,
On Fri, 5 Apr 2019 at 19:12, Ramon Fried wrote:
>
> Introduce new UCLASS_PCI_EP class for handling PCI endpoint
> devices, allowing to set various attributes of the PCI endpoint
> device, such as:
> * configuration space header
> * BAR definitions
> * outband memory mapping
> * start/stop PCI link
>
> Signed-off-by: Ramon Fried
>
> ---
>
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/pci_endpoint/Kconfig | 16 ++
> drivers/pci_endpoint/Makefile| 6 +
> drivers/pci_endpoint/pci_ep-uclass.c | 192 ++
> include/dm/uclass-id.h | 1 +
> include/pci_ep.h | 375 +++
> 7 files changed, 593 insertions(+)
> create mode 100644 drivers/pci_endpoint/Kconfig
> create mode 100644 drivers/pci_endpoint/Makefile
> create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c
> create mode 100644 include/pci_ep.h
This looks pretty good but I have a lot of nits, sorry.
Please can you add a sandbox driver and a test for this (can be in a
separate patch).
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index f24351ac4f..59e2c22cc6 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -64,6 +64,8 @@ source "drivers/nvme/Kconfig"
>
> source "drivers/pci/Kconfig"
>
> +source "drivers/pci_endpoint/Kconfig"
> +
> source "drivers/pch/Kconfig"
>
> source "drivers/pcmcia/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index a7bba3ed56..480b97ef58 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_FPGA) += fpga/
> obj-y += misc/
> obj-$(CONFIG_MMC) += mmc/
> obj-$(CONFIG_NVME) += nvme/
> +obj-$(CONFIG_PCI_ENDPOINT) += pci_endpoint/
> obj-y += pcmcia/
> obj-y += dfu/
> obj-$(CONFIG_PCH) += pch/
> diff --git a/drivers/pci_endpoint/Kconfig b/drivers/pci_endpoint/Kconfig
> new file mode 100644
> index 00..2c0a399a08
> --- /dev/null
> +++ b/drivers/pci_endpoint/Kconfig
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# PCI Endpoint Support
> +#
> +
> +menu "PCI Endpoint"
> +
> +config PCI_ENDPOINT
> + bool "PCI Endpoint Support"
> + depends on DM
> + help
> + Enable this configuration option to support configurable PCI
> + endpoint. This should be enabled if the platform has a PCI
s/endpoint/endpoints/ I think
> + controller that can operate in endpoint mode.
Can you explain a bit more about what this means. I understand that
people can look at the spec, but what is the purpose of 'endpoint
mode' and what does it enable?
> +
> +endmenu
> diff --git a/drivers/pci_endpoint/Makefile b/drivers/pci_endpoint/Makefile
> new file mode 100644
> index 00..80a1066925
> --- /dev/null
> +++ b/drivers/pci_endpoint/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# (C) Copyright 2019
> +# Ramon Fried
> +
> +obj-y += pci_ep-uclass.o
> diff --git a/drivers/pci_endpoint/pci_ep-uclass.c
> b/drivers/pci_endpoint/pci_ep-uclass.c
> new file mode 100644
> index 00..06fcfc5d14
> --- /dev/null
> +++ b/drivers/pci_endpoint/pci_ep-uclass.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * PCI Endpoint uclass
> + *
> + * Based on Linux PCI-EP driver written by
> + * Kishon Vijay Abraham I
> + *
> + * Copyright (c) 2019
> + * Written by Ramon Fried
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +int dm_pci_ep_write_header(struct udevice *dev, u8 fn,
> + struct pci_ep_header *hdr)
> +{
> + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
> +
> + if (!ops->write_header)
> + return -ENODEV;
-ENOSYS (please fix globally)
There is a device. The problem here is that the driver does not
implement the method.
> +
> + return ops->write_header(dev, fn, hdr);
> +}
> +
> +int dm_pci_ep_read_header(struct udevice *dev, u8 fn,
> + struct pci_ep_header *hdr)
> +{
> + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
> +
> + if (!ops->read_header)
> + return -ENODEV;
> +
> + return ops->read_header(dev, fn, hdr);
> +}
> +
> +int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no,
Please use uint for func_no. There is no need to limit it to 8 bits,
and this may not be efficient for non-8-bit machines. Please fix
globally.
> + struct pci_bar *ep_bar)
> +{
> + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
> + int flags = ep_bar->flags;
> +
> + /* Some basic bar validity checks */
> + if ((ep_bar->barno == BAR_5 && flags &
> +PCI_BASE_ADDRESS_MEM_TYPE_64) ||
> + (flags & PCI_BASE_ADDRESS_SPACE_IO &&
> +flags & PCI_BASE_ADDRESS_IO_MASK) ||
Can you add another set of () in those two lines?
> + (upper_32_bits(ep_bar->size) &&
> +!(flags &
[U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass
Introduce new UCLASS_PCI_EP class for handling PCI endpoint
devices, allowing to set various attributes of the PCI endpoint
device, such as:
* configuration space header
* BAR definitions
* outband memory mapping
* start/stop PCI link
Signed-off-by: Ramon Fried
---
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/pci_endpoint/Kconfig | 16 ++
drivers/pci_endpoint/Makefile| 6 +
drivers/pci_endpoint/pci_ep-uclass.c | 192 ++
include/dm/uclass-id.h | 1 +
include/pci_ep.h | 375 +++
7 files changed, 593 insertions(+)
create mode 100644 drivers/pci_endpoint/Kconfig
create mode 100644 drivers/pci_endpoint/Makefile
create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c
create mode 100644 include/pci_ep.h
diff --git a/drivers/Kconfig b/drivers/Kconfig
index f24351ac4f..59e2c22cc6 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -64,6 +64,8 @@ source "drivers/nvme/Kconfig"
source "drivers/pci/Kconfig"
+source "drivers/pci_endpoint/Kconfig"
+
source "drivers/pch/Kconfig"
source "drivers/pcmcia/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index a7bba3ed56..480b97ef58 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_FPGA) += fpga/
obj-y += misc/
obj-$(CONFIG_MMC) += mmc/
obj-$(CONFIG_NVME) += nvme/
+obj-$(CONFIG_PCI_ENDPOINT) += pci_endpoint/
obj-y += pcmcia/
obj-y += dfu/
obj-$(CONFIG_PCH) += pch/
diff --git a/drivers/pci_endpoint/Kconfig b/drivers/pci_endpoint/Kconfig
new file mode 100644
index 00..2c0a399a08
--- /dev/null
+++ b/drivers/pci_endpoint/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# PCI Endpoint Support
+#
+
+menu "PCI Endpoint"
+
+config PCI_ENDPOINT
+ bool "PCI Endpoint Support"
+ depends on DM
+ help
+ Enable this configuration option to support configurable PCI
+ endpoint. This should be enabled if the platform has a PCI
+ controller that can operate in endpoint mode.
+
+endmenu
diff --git a/drivers/pci_endpoint/Makefile b/drivers/pci_endpoint/Makefile
new file mode 100644
index 00..80a1066925
--- /dev/null
+++ b/drivers/pci_endpoint/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# (C) Copyright 2019
+# Ramon Fried
+
+obj-y += pci_ep-uclass.o
diff --git a/drivers/pci_endpoint/pci_ep-uclass.c
b/drivers/pci_endpoint/pci_ep-uclass.c
new file mode 100644
index 00..06fcfc5d14
--- /dev/null
+++ b/drivers/pci_endpoint/pci_ep-uclass.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * PCI Endpoint uclass
+ *
+ * Based on Linux PCI-EP driver written by
+ * Kishon Vijay Abraham I
+ *
+ * Copyright (c) 2019
+ * Written by Ramon Fried
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+DECLARE_GLOBAL_DATA_PTR;
+
+int dm_pci_ep_write_header(struct udevice *dev, u8 fn,
+ struct pci_ep_header *hdr)
+{
+ struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
+
+ if (!ops->write_header)
+ return -ENODEV;
+
+ return ops->write_header(dev, fn, hdr);
+}
+
+int dm_pci_ep_read_header(struct udevice *dev, u8 fn,
+ struct pci_ep_header *hdr)
+{
+ struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
+
+ if (!ops->read_header)
+ return -ENODEV;
+
+ return ops->read_header(dev, fn, hdr);
+}
+
+int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no,
+ struct pci_bar *ep_bar)
+{
+ struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
+ int flags = ep_bar->flags;
+
+ /* Some basic bar validity checks */
+ if ((ep_bar->barno == BAR_5 && flags &
+PCI_BASE_ADDRESS_MEM_TYPE_64) ||
+ (flags & PCI_BASE_ADDRESS_SPACE_IO &&
+flags & PCI_BASE_ADDRESS_IO_MASK) ||
+ (upper_32_bits(ep_bar->size) &&
+!(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)))
+ return -EINVAL;
+
+ if (!ops->set_bar)
+ return -ENODEV;
+
+ return ops->set_bar(dev, func_no, ep_bar);
+}
+
+void pci_ep_clear_bar(struct udevice *dev, u8 func_num, enum pci_barno bar)
+{
+ struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
+
+ if (!ops->clear_bar)
+ return;
+
+ ops->clear_bar(dev, func_num, bar);
+}
+
+int dm_pci_ep_map_addr(struct udevice *dev, u8 func_no, phys_addr_t addr,
+ u64 pci_addr, size_t size)
+{
+ struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
+
+ if (!ops->map_addr)
+ return -ENODEV;
+
+ return ops->map_addr(dev, func_no, addr, pci_addr, size);
+}
+
+void dm_pci_ep_unmap_addr(struct udevice *dev, u8 func_no, phys_addr_t addr)
+{
+ struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
+
+ if (!ops->unmap_addr)
+ return;
+
+ ops->unmap_addr(dev, func_no, addr);
+}
+
+int
