Re: [PATCH v10 5/7] PCI: mediatek-gen3: Add MSI support
On Tuesday 20 April 2021 12:01:10 Marc Zyngier wrote:
> On Tue, 20 Apr 2021 10:44:02 +0100,
> Pali Rohár wrote:
> >
> > Hello!
> >
> > On Tuesday 20 April 2021 14:17:21 Jianjun Wang wrote:
> > > +static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
> > > +{
> > > + int i;
> > > + u32 val;
> > > +
> > > + for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
> > > + struct mtk_msi_set *msi_set = >msi_sets[i];
> > > +
> > > + msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
> > > + i * PCIE_MSI_SET_OFFSET;
> > > + msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
> > > + i * PCIE_MSI_SET_OFFSET;
> > > +
> > > + /* Configure the MSI capture address */
> > > + writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
> > > + writel_relaxed(upper_32_bits(msi_set->msg_addr),
> > > +port->base + PCIE_MSI_SET_ADDR_HI_BASE +
> > > +i * PCIE_MSI_SET_ADDR_HI_OFFSET);
> >
> > This looks like as setting MSI doorbell address to MSI doorbell address.
> >
> > > +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg
> > > *msg)
> > > +{
> > > + struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> > > + struct mtk_pcie_port *port = data->domain->host_data;
> > > + unsigned long hwirq;
> > > +
> > > + hwirq = data->hwirq % PCIE_MSI_IRQS_PER_SET;
> > > +
> > > + msg->address_hi = upper_32_bits(msi_set->msg_addr);
> > > + msg->address_lo = lower_32_bits(msi_set->msg_addr);
> > > + msg->data = hwirq;
> > > + dev_dbg(port->dev, "msi#%#lx address_hi %#x address_lo %#x data %d\n",
> > > + hwirq, msg->address_hi, msg->address_lo, msg->data);
> >
> > ... which is later used in compose_msi_msg().
> >
> > Marc in some other patches for other pci controller drivers changed this
> > address to just main "port" structure. It simplified implementations and
> > also avoided need to declare additional member "msg_addr".
> >
> > Marc, would it be possible to simplify it also for this driver and just
> > set msg_addr to virt_to_phys(port)?
>
> Maybe. It really depends on what range the HW accepts, and the sole
> requirement is to use an address that the endpoint cannot DMA
> to. Here, the driver seems to be using something based on the port
> base address, which is good enough as far as I am concerned (the thing
> I usually object to is the allocation of memory just for the sake of
> getting a capture address).
>
> If you want to further simplify it, you could simply use port.reg_base
> as the MSI address for all sets, as I don't think they have to be
> distinct. But someone with access to the TRM for this should go and
> check it.
>
> I don't believe this should gate the merging od this driver though.
Of course! I'm just asking details to understand best practises and how
it works. So thanks for information!
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Re: [PATCH v10 5/7] PCI: mediatek-gen3: Add MSI support
On Tue, 20 Apr 2021 10:44:02 +0100,
Pali Rohár wrote:
>
> Hello!
>
> On Tuesday 20 April 2021 14:17:21 Jianjun Wang wrote:
> > +static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
> > +{
> > + int i;
> > + u32 val;
> > +
> > + for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
> > + struct mtk_msi_set *msi_set = >msi_sets[i];
> > +
> > + msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
> > + i * PCIE_MSI_SET_OFFSET;
> > + msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
> > + i * PCIE_MSI_SET_OFFSET;
> > +
> > + /* Configure the MSI capture address */
> > + writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
> > + writel_relaxed(upper_32_bits(msi_set->msg_addr),
> > + port->base + PCIE_MSI_SET_ADDR_HI_BASE +
> > + i * PCIE_MSI_SET_ADDR_HI_OFFSET);
>
> This looks like as setting MSI doorbell address to MSI doorbell address.
>
> > +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> > +{
> > + struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> > + struct mtk_pcie_port *port = data->domain->host_data;
> > + unsigned long hwirq;
> > +
> > + hwirq = data->hwirq % PCIE_MSI_IRQS_PER_SET;
> > +
> > + msg->address_hi = upper_32_bits(msi_set->msg_addr);
> > + msg->address_lo = lower_32_bits(msi_set->msg_addr);
> > + msg->data = hwirq;
> > + dev_dbg(port->dev, "msi#%#lx address_hi %#x address_lo %#x data %d\n",
> > + hwirq, msg->address_hi, msg->address_lo, msg->data);
>
> ... which is later used in compose_msi_msg().
>
> Marc in some other patches for other pci controller drivers changed this
> address to just main "port" structure. It simplified implementations and
> also avoided need to declare additional member "msg_addr".
>
> Marc, would it be possible to simplify it also for this driver and just
> set msg_addr to virt_to_phys(port)?
Maybe. It really depends on what range the HW accepts, and the sole
requirement is to use an address that the endpoint cannot DMA
to. Here, the driver seems to be using something based on the port
base address, which is good enough as far as I am concerned (the thing
I usually object to is the allocation of memory just for the sake of
getting a capture address).
If you want to further simplify it, you could simply use port.reg_base
as the MSI address for all sets, as I don't think they have to be
distinct. But someone with access to the TRM for this should go and
check it.
I don't believe this should gate the merging od this driver though.
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH v10 5/7] PCI: mediatek-gen3: Add MSI support
Hello!
On Tuesday 20 April 2021 14:17:21 Jianjun Wang wrote:
> +static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
> +{
> + int i;
> + u32 val;
> +
> + for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
> + struct mtk_msi_set *msi_set = >msi_sets[i];
> +
> + msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
> + i * PCIE_MSI_SET_OFFSET;
> + msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
> + i * PCIE_MSI_SET_OFFSET;
> +
> + /* Configure the MSI capture address */
> + writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
> + writel_relaxed(upper_32_bits(msi_set->msg_addr),
> +port->base + PCIE_MSI_SET_ADDR_HI_BASE +
> +i * PCIE_MSI_SET_ADDR_HI_OFFSET);
This looks like as setting MSI doorbell address to MSI doorbell address.
> +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> + struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> + struct mtk_pcie_port *port = data->domain->host_data;
> + unsigned long hwirq;
> +
> + hwirq = data->hwirq % PCIE_MSI_IRQS_PER_SET;
> +
> + msg->address_hi = upper_32_bits(msi_set->msg_addr);
> + msg->address_lo = lower_32_bits(msi_set->msg_addr);
> + msg->data = hwirq;
> + dev_dbg(port->dev, "msi#%#lx address_hi %#x address_lo %#x data %d\n",
> + hwirq, msg->address_hi, msg->address_lo, msg->data);
... which is later used in compose_msi_msg().
Marc in some other patches for other pci controller drivers changed this
address to just main "port" structure. It simplified implementations and
also avoided need to declare additional member "msg_addr".
Marc, would it be possible to simplify it also for this driver and just
set msg_addr to virt_to_phys(port)?
[PATCH v10 5/7] PCI: mediatek-gen3: Add MSI support
Add MSI support for MediaTek Gen3 PCIe controller.
This PCIe controller supports up to 256 MSI vectors, the MSI hardware
block diagram is as follows:
+-+
| GIC |
+-+
^
|
port->irq
|
+-+-+-+-+-+-+-+-+
|0|1|2|3|4|5|6|7| (PCIe intc)
+-+-+-+-+-+-+-+-+
^ ^ ^
| |...|
+---+ +--++---+
|||
+-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+
|0|1|...|30|31| |0|1|...|30|31| |0|1|...|30|31| (MSI sets)
+-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+
^ ^ ^ ^^ ^ ^ ^^ ^ ^ ^
| | | || | | || | | | (MSI vectors)
| | | || | | || | | |
(MSI SET0) (MSI SET1) ... (MSI SET7)
With 256 MSI vectors supported, the MSI vectors are composed of 8 sets,
each set has its own address for MSI message, and supports 32 MSI vectors
to generate interrupt.
Signed-off-by: Jianjun Wang
Acked-by: Ryder Lee
Reviewed-by: Marc Zyngier
---
drivers/pci/controller/pcie-mediatek-gen3.c | 276
1 file changed, 276 insertions(+)
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
b/drivers/pci/controller/pcie-mediatek-gen3.c
index ff91ad587461..ee1b51207d11 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -48,12 +49,29 @@
#define PCIE_LINK_STATUS_REG 0x154
#define PCIE_PORT_LINKUP BIT(8)
+#define PCIE_MSI_SET_NUM 8
+#define PCIE_MSI_IRQS_PER_SET 32
+#define PCIE_MSI_IRQS_NUM \
+ (PCIE_MSI_IRQS_PER_SET * PCIE_MSI_SET_NUM)
+
#define PCIE_INT_ENABLE_REG0x180
+#define PCIE_MSI_ENABLEGENMASK(PCIE_MSI_SET_NUM + 8 -
1, 8)
+#define PCIE_MSI_SHIFT 8
#define PCIE_INTX_SHIFT24
#define PCIE_INTX_ENABLE \
GENMASK(PCIE_INTX_SHIFT + PCI_NUM_INTX - 1, PCIE_INTX_SHIFT)
#define PCIE_INT_STATUS_REG0x184
+#define PCIE_MSI_SET_ENABLE_REG0x190
+#define PCIE_MSI_SET_ENABLEGENMASK(PCIE_MSI_SET_NUM - 1, 0)
+
+#define PCIE_MSI_SET_BASE_REG 0xc00
+#define PCIE_MSI_SET_OFFSET0x10
+#define PCIE_MSI_SET_STATUS_OFFSET 0x04
+#define PCIE_MSI_SET_ENABLE_OFFSET 0x08
+
+#define PCIE_MSI_SET_ADDR_HI_BASE 0xc80
+#define PCIE_MSI_SET_ADDR_HI_OFFSET0x04
#define PCIE_TRANS_TABLE_BASE_REG 0x800
#define PCIE_ATR_SRC_ADDR_MSB_OFFSET 0x4
@@ -73,6 +91,16 @@
#define PCIE_ATR_TLP_TYPE_MEM PCIE_ATR_TLP_TYPE(0)
#define PCIE_ATR_TLP_TYPE_IO PCIE_ATR_TLP_TYPE(2)
+/**
+ * struct mtk_msi_set - MSI information for each set
+ * @base: IO mapped register base
+ * @msg_addr: MSI message address
+ */
+struct mtk_msi_set {
+ void __iomem *base;
+ phys_addr_t msg_addr;
+};
+
/**
* struct mtk_pcie_port - PCIe port information
* @dev: pointer to PCIe device
@@ -86,6 +114,11 @@
* @irq: PCIe controller interrupt number
* @irq_lock: lock protecting IRQ register access
* @intx_domain: legacy INTx IRQ domain
+ * @msi_domain: MSI IRQ domain
+ * @msi_bottom_domain: MSI IRQ bottom domain
+ * @msi_sets: MSI sets information
+ * @lock: lock protecting IRQ bit map
+ * @msi_irq_in_use: bit map for assigned MSI IRQ
*/
struct mtk_pcie_port {
struct device *dev;
@@ -100,6 +133,11 @@ struct mtk_pcie_port {
int irq;
raw_spinlock_t irq_lock;
struct irq_domain *intx_domain;
+ struct irq_domain *msi_domain;
+ struct irq_domain *msi_bottom_domain;
+ struct mtk_msi_set msi_sets[PCIE_MSI_SET_NUM];
+ struct mutex lock;
+ DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);
};
/**
@@ -196,6 +234,35 @@ static int mtk_pcie_set_trans_table(struct mtk_pcie_port
*port,
return 0;
}
+static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
+{
+ int i;
+ u32 val;
+
+ for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
+ struct mtk_msi_set *msi_set = >msi_sets[i];
+
+ msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
+ i * PCIE_MSI_SET_OFFSET;
+ msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
+ i * PCIE_MSI_SET_OFFSET;
+
+ /* Configure the MSI capture address */
+ writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
+ writel_relaxed(upper_32_bits(msi_set->msg_addr),
+ port->base + PCIE_MSI_SET_ADDR_HI_BASE +
+ i * PCIE_MSI_SET_ADDR_HI_OFFSET);
+ }
+
+ val =
