* Re: [PATCH v3 net-next 4/6] net: ethernet: ti: cpsw: add CBS Qdisc offload
From: Ilias Apalodimas @ 2018-06-20 6:31 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: grygorii.strashko, davem, corbet, akpm, netdev, linux-doc,
linux-kernel, linux-omap, vinicius.gomes, henrik,
jesus.sanchez-palencia, p-varis, spatton, francois.ozog, yogeshs,
nsekhar, andrew
In-Reply-To: <20180615181310.10437-5-ivan.khoronzhuk@linaro.org>
On Fri, Jun 15, 2018 at 09:13:08PM +0300, Ivan Khoronzhuk wrote:
> The cpsw has up to 4 FIFOs per port and upper 3 FIFOs can feed rate
> limited queue with shaping. In order to set and enable shaping for
> those 3 FIFOs queues the network device with CBS qdisc attached is
> needed. The CBS configuration is added for dual-emac/single port mode
> only, but potentially can be used in switch mode also, based on
> switchdev for instance.
>
> Despite the FIFO shapers can work w/o cpdma level shapers the base
> usage must be in combine with cpdma level shapers as described in TRM,
> that are set as maximum rates for interface queues with sysfs.
>
> One of the possible configuration with txq shapers and CBS shapers:
>
> Configured with echo RATE >
> /sys/class/net/eth0/queues/tx-0/tx_maxrate
> /---------------------------------------------------
> /
> / cpdma level shapers
> +----+ +----+ +----+ +----+ +----+ +----+ +----+ +----+
> | c7 | | c6 | | c5 | | c4 | | c3 | | c2 | | c1 | | c0 |
> \ / \ / \ / \ / \ / \ / \ / \ /
> \ / \ / \ / \ / \ / \ / \ / \ /
> \/ \/ \/ \/ \/ \/ \/ \/
> +---------|------|------|------|-------------------------------------+
> | +----+ | | +---+ |
> | | +----+ | | |
> | v v v v |
> | +----+ +----+ +----+ +----+ p p+----+ +----+ +----+ +----+ |
> | | | | | | | | | o o| | | | | | | | |
> | | f3 | | f2 | | f1 | | f0 | r CPSW r| f3 | | f2 | | f1 | | f0 | |
> | | | | | | | | | t t| | | | | | | | |
> | \ / \ / \ / \ / 0 1\ / \ / \ / \ / |
> | \ X \ / \ / \ / \ / \ / \ / \ / |
> | \/ \ \/ \/ \/ \/ \/ \/ \/ |
> +-------\------------------------------------------------------------+
> \
> \ FIFO shaper, set with CBS offload added in this patch,
> \ FIFO0 cannot be rate limited
> ------------------------------------------------------
>
> CBS shaper configuration is supposed to be used with root MQPRIO Qdisc
> offload allowing to add sk_prio->tc->txq maps that direct traffic to
> appropriate tx queue and maps L2 priority to FIFO shaper.
>
> The CBS shaper is intended to be used for AVB where L2 priority
> (pcp field) is used to differentiate class of traffic. So additionally
> vlan needs to be created with appropriate egress sk_prio->l2 prio map.
>
> If CBS has several tx queues assigned to it, the sum of their
> bandwidth has not overlap bandwidth set for CBS. It's recomended the
> CBS bandwidth to be a little bit more.
>
> The CBS shaper is configured with CBS qdisc offload interface using tc
> tool from iproute2 packet.
>
> For instance:
>
> $ tc qdisc replace dev eth0 handle 100: parent root mqprio num_tc 3 \
> map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 queues 1@0 1@1 2@2 hw 1
>
> $ tc -g class show dev eth0
> +---(100:ffe2) mqprio
> | +---(100:3) mqprio
> | +---(100:4) mqprio
> |
> +---(100:ffe1) mqprio
> | +---(100:2) mqprio
> |
> +---(100:ffe0) mqprio
> +---(100:1) mqprio
>
> $ tc qdisc add dev eth0 parent 100:1 cbs locredit -1440 \
> hicredit 60 sendslope -960000 idleslope 40000 offload 1
>
> $ tc qdisc add dev eth0 parent 100:2 cbs locredit -1470 \
> hicredit 62 sendslope -980000 idleslope 20000 offload 1
>
> The above code set CBS shapers for tc0 and tc1, for that txq0 and
> txq1 is used. Pay attention, the real set bandwidth can differ a bit
> due to discreteness of configuration parameters.
>
> Here parameters like locredit, hicredit and sendslope are ignored
> internally and are supposed to be set with assumption that maximum
> frame size for frame - 1500.
>
> It's supposed that interface speed is not changed while reconnection,
> not always is true, so inform user in case speed of interface was
> changed, as it can impact on dependent shapers configuration.
>
> For more examples see Documentation.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> drivers/net/ethernet/ti/cpsw.c | 221 +++++++++++++++++++++++++++++++++
> 1 file changed, 221 insertions(+)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index edd14def98df..19573627a9bb 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -46,6 +46,8 @@
> #include "cpts.h"
> #include "davinci_cpdma.h"
>
> +#include <net/pkt_sched.h>
> +
> #define CPSW_DEBUG (NETIF_MSG_HW | NETIF_MSG_WOL | \
> NETIF_MSG_DRV | NETIF_MSG_LINK | \
> NETIF_MSG_IFUP | NETIF_MSG_INTR | \
> @@ -154,8 +156,12 @@ do { \
> #define IRQ_NUM 2
> #define CPSW_MAX_QUEUES 8
> #define CPSW_CPDMA_DESCS_POOL_SIZE_DEFAULT 256
> +#define CPSW_FIFO_QUEUE_TYPE_SHIFT 16
> +#define CPSW_FIFO_SHAPE_EN_SHIFT 16
> +#define CPSW_FIFO_RATE_EN_SHIFT 20
> #define CPSW_TC_NUM 4
> #define CPSW_FIFO_SHAPERS_NUM (CPSW_TC_NUM - 1)
> +#define CPSW_PCT_MASK 0x7f
>
> #define CPSW_RX_VLAN_ENCAP_HDR_PRIO_SHIFT 29
> #define CPSW_RX_VLAN_ENCAP_HDR_PRIO_MSK GENMASK(2, 0)
> @@ -457,6 +463,8 @@ struct cpsw_priv {
> bool rx_pause;
> bool tx_pause;
> bool mqprio_hw;
> + int fifo_bw[CPSW_TC_NUM];
> + int shp_cfg_speed;
> u32 emac_port;
> struct cpsw_common *cpsw;
> };
> @@ -1081,6 +1089,38 @@ static void cpsw_set_slave_mac(struct cpsw_slave *slave,
> slave_write(slave, mac_lo(priv->mac_addr), SA_LO);
> }
>
> +static bool cpsw_shp_is_off(struct cpsw_priv *priv)
> +{
> + struct cpsw_common *cpsw = priv->cpsw;
> + struct cpsw_slave *slave;
> + u32 shift, mask, val;
> +
> + val = readl_relaxed(&cpsw->regs->ptype);
> +
> + slave = &cpsw->slaves[cpsw_slave_index(cpsw, priv)];
> + shift = CPSW_FIFO_SHAPE_EN_SHIFT + 3 * slave->slave_num;
> + mask = 7 << shift;
> + val = val & mask;
> +
> + return !val;
> +}
> +
> +static void cpsw_fifo_shp_on(struct cpsw_priv *priv, int fifo, int on)
> +{
> + struct cpsw_common *cpsw = priv->cpsw;
> + struct cpsw_slave *slave;
> + u32 shift, mask, val;
> +
> + val = readl_relaxed(&cpsw->regs->ptype);
> +
> + slave = &cpsw->slaves[cpsw_slave_index(cpsw, priv)];
> + shift = CPSW_FIFO_SHAPE_EN_SHIFT + 3 * slave->slave_num;
> + mask = (1 << --fifo) << shift;
> + val = on ? val | mask : val & ~mask;
> +
> + writel_relaxed(val, &cpsw->regs->ptype);
> +}
> +
> static void _cpsw_adjust_link(struct cpsw_slave *slave,
> struct cpsw_priv *priv, bool *link)
> {
> @@ -1120,6 +1160,12 @@ static void _cpsw_adjust_link(struct cpsw_slave *slave,
> mac_control |= BIT(4);
>
> *link = true;
> +
> + if (priv->shp_cfg_speed &&
> + priv->shp_cfg_speed != slave->phy->speed &&
> + !cpsw_shp_is_off(priv))
> + dev_warn(priv->dev,
> + "Speed was changed, CBS shaper speeds are changed!");
> } else {
> mac_control = 0;
> /* disable forwarding */
> @@ -1589,6 +1635,178 @@ static int cpsw_tc_to_fifo(int tc, int num_tc)
> return CPSW_FIFO_SHAPERS_NUM - tc;
> }
>
> +static int cpsw_set_fifo_bw(struct cpsw_priv *priv, int fifo, int bw)
> +{
> + struct cpsw_common *cpsw = priv->cpsw;
> + u32 val = 0, send_pct, shift;
> + struct cpsw_slave *slave;
> + int pct = 0, i;
> +
> + if (bw > priv->shp_cfg_speed * 1000)
> + goto err;
> +
> + /* shaping has to stay enabled for highest fifos linearly
> + * and fifo bw no more then interface can allow
> + */
> + slave = &cpsw->slaves[cpsw_slave_index(cpsw, priv)];
> + send_pct = slave_read(slave, SEND_PERCENT);
> + for (i = CPSW_FIFO_SHAPERS_NUM; i > 0; i--) {
> + if (!bw) {
> + if (i >= fifo || !priv->fifo_bw[i])
> + continue;
> +
> + dev_warn(priv->dev, "Prev FIFO%d is shaped", i);
> + continue;
> + }
> +
> + if (!priv->fifo_bw[i] && i > fifo) {
> + dev_err(priv->dev, "Upper FIFO%d is not shaped", i);
> + return -EINVAL;
> + }
> +
> + shift = (i - 1) * 8;
> + if (i == fifo) {
> + send_pct &= ~(CPSW_PCT_MASK << shift);
> + val = DIV_ROUND_UP(bw, priv->shp_cfg_speed * 10);
> + if (!val)
> + val = 1;
> +
> + send_pct |= val << shift;
> + pct += val;
> + continue;
> + }
> +
> + if (priv->fifo_bw[i])
> + pct += (send_pct >> shift) & CPSW_PCT_MASK;
> + }
> +
> + if (pct >= 100)
> + goto err;
> +
> + slave_write(slave, send_pct, SEND_PERCENT);
> + priv->fifo_bw[fifo] = bw;
> +
> + dev_warn(priv->dev, "set FIFO%d bw = %d\n", fifo,
> + DIV_ROUND_CLOSEST(val * priv->shp_cfg_speed, 100));
> +
> + return 0;
> +err:
> + dev_err(priv->dev, "Bandwidth doesn't fit in tc configuration");
> + return -EINVAL;
> +}
> +
> +static int cpsw_set_fifo_rlimit(struct cpsw_priv *priv, int fifo, int bw)
> +{
> + struct cpsw_common *cpsw = priv->cpsw;
> + struct cpsw_slave *slave;
> + u32 tx_in_ctl_rg, val;
> + int ret;
> +
> + ret = cpsw_set_fifo_bw(priv, fifo, bw);
> + if (ret)
> + return ret;
> +
> + slave = &cpsw->slaves[cpsw_slave_index(cpsw, priv)];
> + tx_in_ctl_rg = cpsw->version == CPSW_VERSION_1 ?
> + CPSW1_TX_IN_CTL : CPSW2_TX_IN_CTL;
> +
> + if (!bw)
> + cpsw_fifo_shp_on(priv, fifo, bw);
> +
> + val = slave_read(slave, tx_in_ctl_rg);
> + if (cpsw_shp_is_off(priv)) {
> + /* disable FIFOs rate limited queues */
> + val &= ~(0xf << CPSW_FIFO_RATE_EN_SHIFT);
> +
> + /* set type of FIFO queues to normal priority mode */
> + val &= ~(3 << CPSW_FIFO_QUEUE_TYPE_SHIFT);
> +
> + /* set type of FIFO queues to be rate limited */
> + if (bw)
> + val |= 2 << CPSW_FIFO_QUEUE_TYPE_SHIFT;
> + else
> + priv->shp_cfg_speed = 0;
> + }
> +
> + /* toggle a FIFO rate limited queue */
> + if (bw)
> + val |= BIT(fifo + CPSW_FIFO_RATE_EN_SHIFT);
> + else
> + val &= ~BIT(fifo + CPSW_FIFO_RATE_EN_SHIFT);
> + slave_write(slave, val, tx_in_ctl_rg);
> +
> + /* FIFO transmit shape enable */
> + cpsw_fifo_shp_on(priv, fifo, bw);
> + return 0;
> +}
> +
> +/* Defaults:
> + * class A - prio 3
> + * class B - prio 2
> + * shaping for class A should be set first
> + */
> +static int cpsw_set_cbs(struct net_device *ndev,
> + struct tc_cbs_qopt_offload *qopt)
> +{
> + struct cpsw_priv *priv = netdev_priv(ndev);
> + struct cpsw_common *cpsw = priv->cpsw;
> + struct cpsw_slave *slave;
> + int prev_speed = 0;
> + int tc, ret, fifo;
> + u32 bw = 0;
> +
> + tc = netdev_txq_to_tc(priv->ndev, qopt->queue);
> +
> + /* enable channels in backward order, as highest FIFOs must be rate
> + * limited first and for compliance with CPDMA rate limited channels
> + * that also used in bacward order. FIFO0 cannot be rate limited.
> + */
> + fifo = cpsw_tc_to_fifo(tc, ndev->num_tc);
> + if (!fifo) {
> + dev_err(priv->dev, "Last tc%d can't be rate limited", tc);
> + return -EINVAL;
> + }
> +
> + /* do nothing, it's disabled anyway */
> + if (!qopt->enable && !priv->fifo_bw[fifo])
> + return 0;
> +
> + /* shapers can be set if link speed is known */
> + slave = &cpsw->slaves[cpsw_slave_index(cpsw, priv)];
> + if (slave->phy && slave->phy->link) {
> + if (priv->shp_cfg_speed &&
> + priv->shp_cfg_speed != slave->phy->speed)
> + prev_speed = priv->shp_cfg_speed;
> +
> + priv->shp_cfg_speed = slave->phy->speed;
> + }
> +
> + if (!priv->shp_cfg_speed) {
> + dev_err(priv->dev, "Link speed is not known");
> + return -1;
> + }
> +
> + ret = pm_runtime_get_sync(cpsw->dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(cpsw->dev);
> + return ret;
> + }
> +
> + bw = qopt->enable ? qopt->idleslope : 0;
> + ret = cpsw_set_fifo_rlimit(priv, fifo, bw);
> + if (ret) {
> + priv->shp_cfg_speed = prev_speed;
> + prev_speed = 0;
> + }
> +
> + if (bw && prev_speed)
> + dev_warn(priv->dev,
> + "Speed was changed, CBS shaper speeds are changed!");
> +
> + pm_runtime_put_sync(cpsw->dev);
> + return ret;
> +}
> +
> static int cpsw_ndo_open(struct net_device *ndev)
> {
> struct cpsw_priv *priv = netdev_priv(ndev);
> @@ -2263,6 +2481,9 @@ static int cpsw_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> void *type_data)
> {
> switch (type) {
> + case TC_SETUP_QDISC_CBS:
> + return cpsw_set_cbs(ndev, type_data);
> +
> case TC_SETUP_QDISC_MQPRIO:
> return cpsw_set_mqprio(ndev, type_data);
>
> --
> 2.17.1
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 02/10] PCI: dwc: Add MSI-X callbacks handler
From: Kishon Vijay Abraham I @ 2018-06-20 6:44 UTC (permalink / raw)
To: Gustavo Pimentel, bhelgaas, lorenzo.pieralisi, Joao.Pinto,
jingoohan1, adouglas, jesper.nilsson
Cc: linux-pci, linux-doc, linux-kernel
In-Reply-To: <5beae521c5ee475b1c0e04f5eec07d0120b0fa2b.1529329262.git.gustavo.pimentel@synopsys.com>
Hi,
On Monday 18 June 2018 08:30 PM, Gustavo Pimentel wrote:
> Add PCIe config space capability search function.
>
> Add sysfs set/get interface to allow the change of EP MSI-X maximum number.
>
> Add EP MSI-X callback for triggering interruptions.
>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> Change v1->v2:
> - Nothing changed, just to follow the patch set version.
> Change v2->v3:
> - Moved dra7xx_pcie_raise_irq() signature change to patch file #3.
> - Moved artpec6_pcie_raise_irq() signature change to patch file #3.
> - Replaced wrong return value 0 to -EINVAL.
> - Removed an else if by code refactoring.
> - Reduced the size of ioremap_nocache mapping from ep->addr_size to
> PCI_MSIX_ENTRY_SIZE.
> - Fixed a small bug. If the MSI-X vector bit has been set, the function
> would return without executing the proper unmap.
> Change v3->v4:
> - Rebased to Lorenzo's master branch v4.18-rc1.
> - Added static prefix to __dw_pcie_ep_find_next_cap function.
>
> drivers/pci/controller/dwc/pcie-designware-ep.c | 146 +++++++++++++++++++++-
> drivers/pci/controller/dwc/pcie-designware-plat.c | 4 +-
> drivers/pci/controller/dwc/pcie-designware.h | 14 ++-
> 3 files changed, 161 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 8650416..ad25654 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -40,6 +40,39 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> __dw_pcie_ep_reset_bar(pci, bar, 0);
> }
>
> +static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
> + u8 cap)
> +{
> + u8 cap_id, next_cap_ptr;
> + u16 reg;
> +
> + reg = dw_pcie_readw_dbi(pci, cap_ptr);
> + next_cap_ptr = (reg & 0xff00) >> 8;
> + cap_id = (reg & 0x00ff);
> +
> + if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
> + return 0;
> +
> + if (cap_id == cap)
> + return cap_ptr;
> +
> + return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> +}
> +
> +u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
This should be static?
Thanks
Kishon
> +{
> + u8 next_cap_ptr;
> + u16 reg;
> +
> + reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
> + next_cap_ptr = (reg & 0x00ff);
> +
> + if (!next_cap_ptr)
> + return 0;
> +
> + return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> +}
> +
> static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
> struct pci_epf_header *hdr)
> {
> @@ -241,8 +274,47 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 encode_int)
> return 0;
> }
>
> +static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no)
> +{
> + struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> + u32 val, reg;
> +
> + if (!ep->msix_cap)
> + return -EINVAL;
> +
> + reg = ep->msix_cap + PCI_MSIX_FLAGS;
> + val = dw_pcie_readw_dbi(pci, reg);
> + if (!(val & PCI_MSIX_FLAGS_ENABLE))
> + return -EINVAL;
> +
> + val &= PCI_MSIX_FLAGS_QSIZE;
> +
> + return val;
> +}
> +
> +static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u16 interrupts)
> +{
> + struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> + u32 val, reg;
> +
> + if (!ep->msix_cap)
> + return -EINVAL;
> +
> + reg = ep->msix_cap + PCI_MSIX_FLAGS;
> + val = dw_pcie_readw_dbi(pci, reg);
> + val &= ~PCI_MSIX_FLAGS_QSIZE;
> + val |= interrupts;
> + dw_pcie_dbi_ro_wr_en(pci);
> + dw_pcie_writew_dbi(pci, reg, val);
> + dw_pcie_dbi_ro_wr_dis(pci);
> +
> + return 0;
> +}
> +
> static int dw_pcie_ep_raise_irq(struct pci_epc *epc, u8 func_no,
> - enum pci_epc_irq_type type, u8 interrupt_num)
> + enum pci_epc_irq_type type, u16 interrupt_num)
> {
> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>
> @@ -282,6 +354,8 @@ static const struct pci_epc_ops epc_ops = {
> .unmap_addr = dw_pcie_ep_unmap_addr,
> .set_msi = dw_pcie_ep_set_msi,
> .get_msi = dw_pcie_ep_get_msi,
> + .set_msix = dw_pcie_ep_set_msix,
> + .get_msix = dw_pcie_ep_get_msix,
> .raise_irq = dw_pcie_ep_raise_irq,
> .start = dw_pcie_ep_start,
> .stop = dw_pcie_ep_stop,
> @@ -322,6 +396,64 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
> return 0;
> }
>
> +int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
> + u16 interrupt_num)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> + struct pci_epc *epc = ep->epc;
> + u16 tbl_offset, bir;
> + u32 bar_addr_upper, bar_addr_lower;
> + u32 msg_addr_upper, msg_addr_lower;
> + u32 reg, msg_data, vec_ctrl;
> + u64 tbl_addr, msg_addr, reg_u64;
> + void __iomem *msix_tbl;
> + int ret;
> +
> + reg = ep->msix_cap + PCI_MSIX_TABLE;
> + tbl_offset = dw_pcie_readl_dbi(pci, reg);
> + bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
> + tbl_offset &= PCI_MSIX_TABLE_OFFSET;
> + tbl_offset >>= 3;
> +
> + reg = PCI_BASE_ADDRESS_0 + (4 * bir);
> + bar_addr_upper = 0;
> + bar_addr_lower = dw_pcie_readl_dbi(pci, reg);
> + reg_u64 = (bar_addr_lower & PCI_BASE_ADDRESS_MEM_TYPE_MASK);
> + if (reg_u64 == PCI_BASE_ADDRESS_MEM_TYPE_64)
> + bar_addr_upper = dw_pcie_readl_dbi(pci, reg + 4);
> +
> + tbl_addr = ((u64) bar_addr_upper) << 32 | bar_addr_lower;
> + tbl_addr += (tbl_offset + ((interrupt_num - 1) * PCI_MSIX_ENTRY_SIZE));
> + tbl_addr &= PCI_BASE_ADDRESS_MEM_MASK;
> +
> + msix_tbl = ioremap_nocache(ep->phys_base + tbl_addr,
> + PCI_MSIX_ENTRY_SIZE);
> + if (!msix_tbl)
> + return -EINVAL;
> +
> + msg_addr_lower = readl(msix_tbl + PCI_MSIX_ENTRY_LOWER_ADDR);
> + msg_addr_upper = readl(msix_tbl + PCI_MSIX_ENTRY_UPPER_ADDR);
> + msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
> + msg_data = readl(msix_tbl + PCI_MSIX_ENTRY_DATA);
> + vec_ctrl = readl(msix_tbl + PCI_MSIX_ENTRY_VECTOR_CTRL);
> +
> + iounmap(msix_tbl);
> +
> + if (vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)
> + return -EPERM;
> +
> + ret = dw_pcie_ep_map_addr(epc, func_no, ep->msix_mem_phys, msg_addr,
> + epc->mem->page_size);
> + if (ret)
> + return ret;
> +
> + writel(msg_data, ep->msix_mem);
> +
> + dw_pcie_ep_unmap_addr(epc, func_no, ep->msix_mem_phys);
> +
> + return 0;
> +}
> +
> void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
> {
> struct pci_epc *epc = ep->epc;
> @@ -329,6 +461,9 @@ void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
> pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
> epc->mem->page_size);
>
> + pci_epc_mem_free_addr(epc, ep->msix_mem_phys, ep->msix_mem,
> + epc->mem->page_size);
> +
> pci_epc_mem_exit(epc);
> }
>
> @@ -412,6 +547,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> dev_err(dev, "Failed to reserve memory for MSI\n");
> return -ENOMEM;
> }
> + ep->msi_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSI);
> +
> + ep->msix_mem = pci_epc_mem_alloc_addr(epc, &ep->msix_mem_phys,
> + epc->mem->page_size);
> + if (!ep->msix_mem) {
> + dev_err(dev, "Failed to reserve memory for MSI-X\n");
> + return -ENOMEM;
> + }
> + ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
>
> epc->features = EPC_FEATURE_NO_LINKUP_NOTIFIER;
> EPC_FEATURE_SET_BAR(epc->features, BAR_0);
> diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
> index 5937fed..654dcb5 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-plat.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
> @@ -78,7 +78,7 @@ static void dw_plat_pcie_ep_init(struct dw_pcie_ep *ep)
>
> static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> enum pci_epc_irq_type type,
> - u8 interrupt_num)
> + u16 interrupt_num)
> {
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>
> @@ -88,6 +88,8 @@ static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> return -EINVAL;
> case PCI_EPC_IRQ_MSI:
> return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> + case PCI_EPC_IRQ_MSIX:
> + return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
> default:
> dev_err(pci->dev, "UNKNOWN IRQ type\n");
> }
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index bee4e25..b22c5bb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -191,7 +191,7 @@ enum dw_pcie_as_type {
> struct dw_pcie_ep_ops {
> void (*ep_init)(struct dw_pcie_ep *ep);
> int (*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
> - enum pci_epc_irq_type type, u8 interrupt_num);
> + enum pci_epc_irq_type type, u16 interrupt_num);
> };
>
> struct dw_pcie_ep {
> @@ -208,6 +208,10 @@ struct dw_pcie_ep {
> u32 num_ob_windows;
> void __iomem *msi_mem;
> phys_addr_t msi_mem_phys;
> + void __iomem *msix_mem;
> + phys_addr_t msix_mem_phys;
> + u8 msi_cap; /* MSI capability offset */
> + u8 msix_cap; /* MSI-X capability offset */
> };
>
> struct dw_pcie_ops {
> @@ -359,6 +363,8 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep);
> void dw_pcie_ep_exit(struct dw_pcie_ep *ep);
> int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
> u8 interrupt_num);
> +int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
> + u16 interrupt_num);
> void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar);
> #else
> static inline void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> @@ -380,6 +386,12 @@ static inline int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
> return 0;
> }
>
> +static inline int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
> + u16 interrupt_num)
> +{
> + return 0;
> +}
> +
> static inline void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> {
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH -tip v6 26/27] Documentation: kprobes: Add how to change the execution path
From: Masami Hiramatsu @ 2018-06-20 8:26 UTC (permalink / raw)
To: Randy Dunlap
Cc: Thomas Gleixner, Ingo Molnar, Ingo Molnar, H . Peter Anvin,
linux-kernel, Ananth N Mavinakayanahalli, Andrew Morton,
Steven Rostedt, linux-arch, Jonathan Corbet, linux-doc
In-Reply-To: <706832c5-dab4-f806-96d6-2cdc37203d9d@infradead.org>
On Tue, 19 Jun 2018 09:31:49 -0700
Randy Dunlap <rdunlap@infradead.org> wrote:
> On 06/19/2018 09:16 AM, Masami Hiramatsu wrote:
> > Add a section that explaining how to change the execution
> > path with kprobes and warnings for some arch.
> >
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: linux-doc@vger.kernel.org
> > ---
> > Documentation/kprobes.txt | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
> > index 3e9e99ea751b..8a98eed1521b 100644
> > --- a/Documentation/kprobes.txt
> > +++ b/Documentation/kprobes.txt
> > @@ -80,6 +80,26 @@ After the instruction is single-stepped, Kprobes executes the
> > "post_handler," if any, that is associated with the kprobe.
> > Execution then continues with the instruction following the probepoint.
> >
>
> Hi,
> I have a few small suggestions...
Hi Randy,
Thank you for your suggestions!
All of those are good to me. I'll fix it:)
Thank you!
>
>
> > +Changing Execution Path
> > +-----------------------
> > +
> > +Since the kprobes can probe into a running kernel code, it can change
>
> Since kprobes can probe into running kernel code, it can change
>
> > +the register set, including instruction pointer. This operation
> > +requires maximum attention, such as keeping the stack frame, recovering
> > +execution path etc. Since it is operated on running kernel and need deep
>
> Since it operates on a running kernel and needs deep
>
> > +knowladge of the archtecture and concurrent computing, you can easily
>
> knowledge of the architecture
>
> > +shot your foot.
>
> shoot
>
> > +
> > +If you change the instruction pointer (and set up other related
> > +registers) in pre_handler, you must return !0 so that the kprobes
>
> so that kprobes
>
> > +stops single stepping and just returns to given address.
>
> to the given address.
>
> > +This also means post_handler should not be called anymore.
> > +
> > +Note that this operation may be harder on some architectures which
> > +use TOC (Table of Contents) for function call, since you have to
> > +setup new TOC for your function in your module, and recover old
>
> setup a new TOC for your function in your module, and recover the old
>
> > +one after back from it.
>
> one after returning from it.
>
> > +
> > Return Probes
> > -------------
>
>
> --
> ~Randy
--
Masami Hiramatsu <mhiramat@kernel.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 2/9] Uprobe: Change set_swbp definition
From: kbuild test robot @ 2018-06-20 8:07 UTC (permalink / raw)
To: Ravi Bangoria
Cc: kbuild-all, oleg, srikar, rostedt, mhiramat, peterz, mingo, acme,
alexander.shishkin, jolsa, namhyung, linux-kernel, corbet,
linux-doc, ananth, alexis.berlemont, naveen.n.rao, Ravi Bangoria
In-Reply-To: <20180620035629.7844-3-ravi.bangoria@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3437 bytes --]
Hi Ravi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tip/perf/core]
[also build test WARNING on v4.18-rc1 next-20180619]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ravi-Bangoria/Uprobes-Support-SDT-markers-having-reference-count-semaphore/20180620-120240
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm
All warnings (new ones prefixed by >>):
In file included from include/linux/swab.h:5:0,
from include/uapi/linux/byteorder/big_endian.h:13,
from include/linux/byteorder/big_endian.h:5,
from arch/arm/include/uapi/asm/byteorder.h:20,
from include/asm-generic/bitops/le.h:6,
from arch/arm/include/asm/bitops.h:342,
from include/linux/bitops.h:38,
from include/linux/kernel.h:11,
from arch/arm/probes/uprobes/core.c:9:
arch/arm/probes/uprobes/core.c: In function 'set_swbp':
arch/arm/probes/uprobes/core.c:36:32: error: dereferencing pointer to incomplete type 'struct uprobe'
__opcode_to_mem_arm(uprobe->arch.bpinsn));
^
include/uapi/linux/swab.h:114:54: note: in definition of macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
>> arch/arm/include/asm/opcodes.h:103:32: note: in expansion of macro '___opcode_swab32'
#define __opcode_to_mem_arm(x) ___opcode_swab32(x)
^~~~~~~~~~~~~~~~
arch/arm/probes/uprobes/core.c:36:6: note: in expansion of macro '__opcode_to_mem_arm'
__opcode_to_mem_arm(uprobe->arch.bpinsn));
^~~~~~~~~~~~~~~~~~~
arch/arm/probes/uprobes/core.c:37:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
vim +/___opcode_swab32 +103 arch/arm/include/asm/opcodes.h
57b9da32 Dave Martin 2012-09-03 102
0ce3de23 Dave Martin 2012-09-03 @103 #define __opcode_to_mem_arm(x) ___opcode_swab32(x)
0ce3de23 Dave Martin 2012-09-03 104 #define __opcode_to_mem_thumb16(x) ___opcode_swab16(x)
0ce3de23 Dave Martin 2012-09-03 105 #define __opcode_to_mem_thumb32(x) ___opcode_swahb32(x)
0ce3de23 Dave Martin 2012-09-03 106 #define ___asm_opcode_to_mem_arm(x) ___asm_opcode_swab32(x)
0ce3de23 Dave Martin 2012-09-03 107 #define ___asm_opcode_to_mem_thumb16(x) ___asm_opcode_swab16(x)
0ce3de23 Dave Martin 2012-09-03 108 #define ___asm_opcode_to_mem_thumb32(x) ___asm_opcode_swahb32(x)
57b9da32 Dave Martin 2012-09-03 109
:::::: The code at line 103 was first introduced by commit
:::::: 0ce3de23f2a520a6ac8c2179fb8f88342c4992ef ARM: 7509/1: opcodes: Make opcode byteswapping macros assembly-compatible
:::::: TO: Dave Martin <dave.martin@linaro.org>
:::::: CC: Russell King <rmk+kernel@arm.linux.org.uk>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65123 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] Documentation/sphinx: allow "functions" with no parameters
From: Jani Nikula @ 2018-06-20 7:19 UTC (permalink / raw)
To: Mike Rapoport, Jonathan Corbet; +Cc: Matthew Wilcox, linux-doc, Mike Rapoport
In-Reply-To: <1529468469-10088-2-git-send-email-rppt@linux.vnet.ibm.com>
On Wed, 20 Jun 2018, Mike Rapoport <rppt@linux.vnet.ibm.com> wrote:
> When kernel-doc:: specified in .rst document without explicit directives,
> it outputs both comment and DOC: sections. If a DOC: section was explicitly
> included in the same document it will be duplicated. For example, the
> output generated for Documentation/core-api/idr.rst [1] has "IDA
> description" in the "IDA usage" section and in the middle of the API
> reference.
>
> This patch enables using "functions" directive without parameters to output
> all the documentation excluding DOC: sections.
>
> [1] https://www.kernel.org/doc/html/v4.17/core-api/idr.html
>
> Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
> Acked-by: Matthew Wilcox <willy@infradead.org>
Looks good to me. Though I do realize now that I overlooked that this
applies to not only functions, but also to other non-DOC documentation
comments. I guess up to Jon to decide.
Please do give the cobbler's children some shoes, and document this in
Documentation/doc-guide/kernel-doc.rst.
Thanks,
Jani.
> ---
> Documentation/sphinx/kerneldoc.py | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
> index fbedcc3..9d0a7f0 100644
> --- a/Documentation/sphinx/kerneldoc.py
> +++ b/Documentation/sphinx/kerneldoc.py
> @@ -47,7 +47,7 @@ class KernelDocDirective(Directive):
> optional_arguments = 4
> option_spec = {
> 'doc': directives.unchanged_required,
> - 'functions': directives.unchanged_required,
> + 'functions': directives.unchanged,
> 'export': directives.unchanged,
> 'internal': directives.unchanged,
> }
> @@ -75,8 +75,12 @@ class KernelDocDirective(Directive):
> elif 'doc' in self.options:
> cmd += ['-function', str(self.options.get('doc'))]
> elif 'functions' in self.options:
> - for f in str(self.options.get('functions')).split():
> - cmd += ['-function', f]
> + functions = self.options.get('functions').split()
> + if functions:
> + for f in functions:
> + cmd += ['-function', f]
> + else:
> + cmd += ['-no-doc-sections']
>
> for pattern in export_file_patterns:
> for f in glob.glob(env.config.kerneldoc_srctree + '/' + pattern):
--
Jani Nikula, Intel Open Source Graphics Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/2] Documentation/sphinx: allow "functions" with no parameters
From: Mike Rapoport @ 2018-06-20 8:49 UTC (permalink / raw)
To: Jani Nikula; +Cc: Jonathan Corbet, Matthew Wilcox, linux-doc
In-Reply-To: <878t79x6uv.fsf@intel.com>
On Wed, Jun 20, 2018 at 10:19:36AM +0300, Jani Nikula wrote:
> On Wed, 20 Jun 2018, Mike Rapoport <rppt@linux.vnet.ibm.com> wrote:
> > When kernel-doc:: specified in .rst document without explicit directives,
> > it outputs both comment and DOC: sections. If a DOC: section was explicitly
> > included in the same document it will be duplicated. For example, the
> > output generated for Documentation/core-api/idr.rst [1] has "IDA
> > description" in the "IDA usage" section and in the middle of the API
> > reference.
> >
> > This patch enables using "functions" directive without parameters to output
> > all the documentation excluding DOC: sections.
> >
> > [1] https://www.kernel.org/doc/html/v4.17/core-api/idr.html
> >
> > Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
> > Acked-by: Matthew Wilcox <willy@infradead.org>
>
> Looks good to me. Though I do realize now that I overlooked that this
> applies to not only functions, but also to other non-DOC documentation
> comments. I guess up to Jon to decide.
We can name it "everything-except-doc-sections" ;-)
> Please do give the cobbler's children some shoes, and document this in
> Documentation/doc-guide/kernel-doc.rst.
Sure. I'd just wait until there's consensus on the name :)
> Thanks,
> Jani.
>
> > ---
> > Documentation/sphinx/kerneldoc.py | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
> > index fbedcc3..9d0a7f0 100644
> > --- a/Documentation/sphinx/kerneldoc.py
> > +++ b/Documentation/sphinx/kerneldoc.py
> > @@ -47,7 +47,7 @@ class KernelDocDirective(Directive):
> > optional_arguments = 4
> > option_spec = {
> > 'doc': directives.unchanged_required,
> > - 'functions': directives.unchanged_required,
> > + 'functions': directives.unchanged,
> > 'export': directives.unchanged,
> > 'internal': directives.unchanged,
> > }
> > @@ -75,8 +75,12 @@ class KernelDocDirective(Directive):
> > elif 'doc' in self.options:
> > cmd += ['-function', str(self.options.get('doc'))]
> > elif 'functions' in self.options:
> > - for f in str(self.options.get('functions')).split():
> > - cmd += ['-function', f]
> > + functions = self.options.get('functions').split()
> > + if functions:
> > + for f in functions:
> > + cmd += ['-function', f]
> > + else:
> > + cmd += ['-no-doc-sections']
> >
> > for pattern in export_file_patterns:
> > for f in glob.glob(env.config.kerneldoc_srctree + '/' + pattern):
>
> --
> Jani Nikula, Intel Open Source Graphics Center
>
--
Sincerely yours,
Mike.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 03/10] PCI: Update xxx_pcie_ep_raise_irq() and pci_epc_raise_irq() signatures
From: Gustavo Pimentel @ 2018-06-20 9:11 UTC (permalink / raw)
To: kbuild test robot, Gustavo Pimentel
Cc: kbuild-all@01.org, bhelgaas@google.com, lorenzo.pieralisi@arm.com,
Joao.Pinto@synopsys.com, jingoohan1@gmail.com, kishon@ti.com,
adouglas@cadence.com, jesper.nilsson@axis.com,
linux-pci@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <201806190312.UKq0zYJ1%fengguang.wu@intel.com>
Hi,
Thanks for the suggestion.
Regards,
Gustavo
On 18/06/2018 20:31, kbuild test robot wrote:
> Hi Gustavo,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on pci/next]
> [also build test WARNING on v4.18-rc1 next-20180618]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_0day-2Dci_linux_commits_Gustavo-2DPimentel_Add-2DMSI-2DX-2Dsupport-2Don-2Dpcitest-2Dtool_20180619-2D004625&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=IFx9JKJsE2KvE7siiXs4GRW2a3OkFpE8BFO41SbTjJA&s=vuOhITY-_4Y-jxzyQ1ddb6M8E9-OELpKXzcJZqgWqvE&e=
> base: https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_helgaas_pci.git&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=IFx9JKJsE2KvE7siiXs4GRW2a3OkFpE8BFO41SbTjJA&s=7xs_oUDvo5HRxpqsZAqdybMJcJDO6K3hooyy-CmXwkM&e= next
> reproduce:
> # apt-get install sparse
> make ARCH=x86_64 allmodconfig
> make C=1 CF=-D__CHECK_ENDIAN__
>
>
> sparse warnings: (new ones prefixed by >>)
>
> drivers/pci/controller/pcie-rockchip-ep.c:173:14: sparse: expression using sizeof(void)
>>> drivers/pci/controller/pcie-rockchip-ep.c:516:27: sparse: incorrect type in initializer (incompatible argument 4 (different type sizes)) @@ expected int ( *raise_irq )( ... ) @@ got int ( *raise_irq )( ... ) @@
> drivers/pci/controller/pcie-rockchip-ep.c:516:27: expected int ( *raise_irq )( ... )
> drivers/pci/controller/pcie-rockchip-ep.c:516:27: got int ( *<noident> )( ... )
> drivers/pci/controller/pcie-rockchip-ep.c:516:15: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
> .raise_irq = rockchip_pcie_ep_raise_irq,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/pci/controller/pcie-rockchip-ep.c:516:15: note: (near initialization for 'rockchip_pcie_epc_ops.raise_irq')
> cc1: some warnings being treated as errors
>
> vim +516 drivers/pci/controller/pcie-rockchip-ep.c
>
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 507
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 508 static const struct pci_epc_ops rockchip_pcie_epc_ops = {
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 509 .write_header = rockchip_pcie_ep_write_header,
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 510 .set_bar = rockchip_pcie_ep_set_bar,
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 511 .clear_bar = rockchip_pcie_ep_clear_bar,
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 512 .map_addr = rockchip_pcie_ep_map_addr,
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 513 .unmap_addr = rockchip_pcie_ep_unmap_addr,
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 514 .set_msi = rockchip_pcie_ep_set_msi,
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 515 .get_msi = rockchip_pcie_ep_get_msi,
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 @516 .raise_irq = rockchip_pcie_ep_raise_irq,
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 517 .start = rockchip_pcie_ep_start,
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 518 };
> cf590b07 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 519
>
> :::::: The code at line 516 was first introduced by commit
> :::::: cf590b07839133146842d2d3d9a68f804c2edc4b PCI: rockchip: Add EP driver for Rockchip PCIe controller
>
> :::::: TO: Shawn Lin <shawn.lin@rock-chips.com>
> :::::: CC: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.01.org_pipermail_kbuild-2Dall&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=IFx9JKJsE2KvE7siiXs4GRW2a3OkFpE8BFO41SbTjJA&s=Mf1d7cz8MbsSZPkgXFwnxZJY1579nwc7-mpjeQBdweY&e= Intel Corporation
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCH] PCI: dwc: dw_pcie_ep_find_capability() can be static
From: Gustavo Pimentel @ 2018-06-20 9:23 UTC (permalink / raw)
To: kbuild test robot, Gustavo Pimentel
Cc: kbuild-all@01.org, bhelgaas@google.com, lorenzo.pieralisi@arm.com,
Joao.Pinto@synopsys.com, jingoohan1@gmail.com, kishon@ti.com,
adouglas@cadence.com, jesper.nilsson@axis.com,
linux-pci@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20180618191207.GA111117@ivb43>
Hi,
Thanks for the suggestion.
Regards,
Gustavo
On 18/06/2018 20:12, kbuild test robot wrote:
>
> Fixes: 43f8cf4686e0 ("PCI: dwc: Add MSI-X callbacks handler")
> Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
> ---
> pcie-designware-ep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index beee64e..ffc2065 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -59,7 +59,7 @@ static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
> return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> }
>
> -u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
> +static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
> {
> u8 next_cap_ptr;
> u16 reg;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 09/10] pci_endpoint_test: Add 2 ioctl commands
From: Kishon Vijay Abraham I @ 2018-06-20 7:58 UTC (permalink / raw)
To: Gustavo Pimentel, bhelgaas, lorenzo.pieralisi, Joao.Pinto,
jingoohan1, adouglas, jesper.nilsson
Cc: linux-pci, linux-doc, linux-kernel
In-Reply-To: <b10530671bdcda8b9d2bb25048ad9048f814c608.1529329262.git.gustavo.pimentel@synopsys.com>
Hi,
On Monday 18 June 2018 08:30 PM, Gustavo Pimentel wrote:
> Add MSI-X support and update driver documentation accordingly.
>
> Add 2 new IOCTL commands:
> - Allow to reconfigure driver IRQ type in runtime.
> - Allow to retrieve current driver IRQ type configured.
>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> Change v2->v3:
> - New patch file created base on the previous patch
> "misc: pci_endpoint_test: Add MSI-X support" patch file following
> Kishon's suggestion.
> Change v3->v4:
> - Rebased to Lorenzo's master branch v4.18-rc1.
>
> Documentation/misc-devices/pci-endpoint-test.txt | 3 +
> drivers/misc/pci_endpoint_test.c | 177 +++++++++++++++++------
> 2 files changed, 132 insertions(+), 48 deletions(-)
>
> diff --git a/Documentation/misc-devices/pci-endpoint-test.txt b/Documentation/misc-devices/pci-endpoint-test.txt
> index fdfa0f6..58ccca4 100644
> --- a/Documentation/misc-devices/pci-endpoint-test.txt
> +++ b/Documentation/misc-devices/pci-endpoint-test.txt
> @@ -28,6 +28,9 @@ ioctl
> to be tested should be passed as argument.
> PCITEST_MSIX: Tests message signalled interrupts. The MSI-X number
> to be tested should be passed as argument.
> + PCITEST_SET_IRQTYPE: Changes driver IRQ type configuration. The IRQ type
> + should be passed as argument (0: Legacy, 1:MSI, 2:MSI-X).
> + PCITEST_GET_IRQTYPE: Gets driver IRQ type configuration.
> PCITEST_WRITE: Perform write tests. The size of the buffer should be passed
> as argument.
> PCITEST_READ: Perform read tests. The size of the buffer should be passed
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 8d15dbe..df2017f 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -157,6 +157,87 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static void pci_endpoint_test_free_irq_vectors(struct pci_endpoint_test *test)
> +{
> + int i;
> + struct pci_dev *pdev = test->pdev;
> + struct device *dev = &pdev->dev;
> +
> + for (i = 0; i < test->num_irqs; i++)
> + devm_free_irq(dev, pci_irq_vector(pdev, i), test);
> +
> + test->num_irqs = 0;
> +}
> +
> +static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test)
> +{
> + int irq = -1;
> + struct pci_dev *pdev = test->pdev;
> + struct device *dev = &pdev->dev;
> + bool res = true;
> +
> + switch (irq_type) {
> + case IRQ_TYPE_LEGACY:
> + irq = 0;
> + break;
> + case IRQ_TYPE_MSI:
> + irq = pci_alloc_irq_vectors(pdev, 1, 32, PCI_IRQ_MSI);
> + if (irq < 0)
> + dev_err(dev, "Failed to get MSI interrupts\n");
> + break;
> + case IRQ_TYPE_MSIX:
> + irq = pci_alloc_irq_vectors(pdev, 1, 2048, PCI_IRQ_MSIX);
> + if (irq < 0)
> + dev_err(dev, "Failed to get MSI-X interrupts\n");
> + break;
> + default:
> + dev_err(dev, "Invalid IRQ type selected\n");
> + }
> +
> + if (irq < 0) {
> + irq = 0;
> + res = false;
> + }
> + test->num_irqs = irq;
> +
> + return res;
> +}
> +
> +static void pci_endpoint_test_release_irq(struct pci_endpoint_test *test)
> +{
> + struct pci_dev *pdev = test->pdev;
> +
> + pci_disable_msi(pdev);
> + pci_disable_msix(pdev);
> +}
> +
> +static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
> +{
> + int i;
> + int err;
> + struct pci_dev *pdev = test->pdev;
> + struct device *dev = &pdev->dev;
> +
> + err = devm_request_irq(dev, pdev->irq, pci_endpoint_test_irqhandler,
> + IRQF_SHARED, DRV_MODULE_NAME, test);
> + if (err) {
> + dev_err(dev, "Failed to request IRQ %d\n", pdev->irq);
> + return false;
> + }
> +
> + for (i = 1; i < test->num_irqs; i++) {
> + err = devm_request_irq(dev, pci_irq_vector(pdev, i),
> + pci_endpoint_test_irqhandler,
> + IRQF_SHARED, DRV_MODULE_NAME, test);
> + if (err)
> + dev_err(dev, "Failed to request IRQ %d for MSI%s %d\n",
> + pci_irq_vector(pdev, i),
> + irq_type == IRQ_TYPE_MSIX ? "-X" : "", i + 1);
> + }
> +
> + return true;
> +}
> +
> static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
> enum pci_barno barno)
> {
> @@ -440,6 +521,38 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test, size_t size)
> return ret;
> }
>
> +static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
> + int req_irq_type)
> +{
> + struct pci_dev *pdev = test->pdev;
> + struct device *dev = &pdev->dev;
> +
> + if (req_irq_type < IRQ_TYPE_LEGACY || req_irq_type > IRQ_TYPE_MSIX) {
> + dev_err(dev, "Invalid IRQ type option\n");
> + return false;
> + }
> +
> + if (irq_type == req_irq_type)
> + return true;
> +
> + irq_type = req_irq_type;
> +
> + pci_endpoint_test_free_irq_vectors(test);
> + pci_endpoint_test_release_irq(test);
> +
> + if (!pci_endpoint_test_alloc_irq_vectors(test)) {
> + pci_endpoint_test_release_irq(test);
> + return false;
> + }
> +
> + if (!pci_endpoint_test_request_irq(test)) {
> + pci_endpoint_test_release_irq(test);
> + return false;
> + }
> +
> + return true;
> +}
> +
> static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -471,6 +584,12 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
> case PCITEST_COPY:
> ret = pci_endpoint_test_copy(test, arg);
> break;
> + case PCITEST_SET_IRQTYPE:
> + ret = pci_endpoint_test_set_irq(test, arg);
> + break;
> + case PCITEST_GET_IRQTYPE:
> + ret = irq_type;
Can't the set_irq be done as part of raise irq itself?
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 02/10] PCI: dwc: Add MSI-X callbacks handler
From: Gustavo Pimentel @ 2018-06-20 9:26 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Gustavo Pimentel, bhelgaas@google.com,
lorenzo.pieralisi@arm.com, Joao.Pinto@synopsys.com,
jingoohan1@gmail.com, adouglas@cadence.com,
jesper.nilsson@axis.com
Cc: linux-pci@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <88d14571-694e-db83-11df-0e647f7bf3db@ti.com>
Hi Kishon,
On 20/06/2018 07:44, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Monday 18 June 2018 08:30 PM, Gustavo Pimentel wrote:
>> Add PCIe config space capability search function.
>>
>> Add sysfs set/get interface to allow the change of EP MSI-X maximum number.
>>
>> Add EP MSI-X callback for triggering interruptions.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>> Change v1->v2:
>> - Nothing changed, just to follow the patch set version.
>> Change v2->v3:
>> - Moved dra7xx_pcie_raise_irq() signature change to patch file #3.
>> - Moved artpec6_pcie_raise_irq() signature change to patch file #3.
>> - Replaced wrong return value 0 to -EINVAL.
>> - Removed an else if by code refactoring.
>> - Reduced the size of ioremap_nocache mapping from ep->addr_size to
>> PCI_MSIX_ENTRY_SIZE.
>> - Fixed a small bug. If the MSI-X vector bit has been set, the function
>> would return without executing the proper unmap.
>> Change v3->v4:
>> - Rebased to Lorenzo's master branch v4.18-rc1.
>> - Added static prefix to __dw_pcie_ep_find_next_cap function.
>>
>> drivers/pci/controller/dwc/pcie-designware-ep.c | 146 +++++++++++++++++++++-
>> drivers/pci/controller/dwc/pcie-designware-plat.c | 4 +-
>> drivers/pci/controller/dwc/pcie-designware.h | 14 ++-
>> 3 files changed, 161 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> index 8650416..ad25654 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> @@ -40,6 +40,39 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>> __dw_pcie_ep_reset_bar(pci, bar, 0);
>> }
>>
>> +static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
>> + u8 cap)
>> +{
>> + u8 cap_id, next_cap_ptr;
>> + u16 reg;
>> +
>> + reg = dw_pcie_readw_dbi(pci, cap_ptr);
>> + next_cap_ptr = (reg & 0xff00) >> 8;
>> + cap_id = (reg & 0x00ff);
>> +
>> + if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
>> + return 0;
>> +
>> + if (cap_id == cap)
>> + return cap_ptr;
>> +
>> + return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
>> +}
>> +
>> +u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
>
> This should be static?
Yes, kbuild also point that out. It'll be fixed.
Thanks
Gustavo
>
> Thanks
> Kishon
>> +{
>> + u8 next_cap_ptr;
>> + u16 reg;
>> +
>> + reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
>> + next_cap_ptr = (reg & 0x00ff);
>> +
>> + if (!next_cap_ptr)
>> + return 0;
>> +
>> + return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
>> +}
>> +
>> static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>> struct pci_epf_header *hdr)
>> {
>> @@ -241,8 +274,47 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 encode_int)
>> return 0;
>> }
>>
>> +static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no)
>> +{
>> + struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> + u32 val, reg;
>> +
>> + if (!ep->msix_cap)
>> + return -EINVAL;
>> +
>> + reg = ep->msix_cap + PCI_MSIX_FLAGS;
>> + val = dw_pcie_readw_dbi(pci, reg);
>> + if (!(val & PCI_MSIX_FLAGS_ENABLE))
>> + return -EINVAL;
>> +
>> + val &= PCI_MSIX_FLAGS_QSIZE;
>> +
>> + return val;
>> +}
>> +
>> +static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u16 interrupts)
>> +{
>> + struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> + u32 val, reg;
>> +
>> + if (!ep->msix_cap)
>> + return -EINVAL;
>> +
>> + reg = ep->msix_cap + PCI_MSIX_FLAGS;
>> + val = dw_pcie_readw_dbi(pci, reg);
>> + val &= ~PCI_MSIX_FLAGS_QSIZE;
>> + val |= interrupts;
>> + dw_pcie_dbi_ro_wr_en(pci);
>> + dw_pcie_writew_dbi(pci, reg, val);
>> + dw_pcie_dbi_ro_wr_dis(pci);
>> +
>> + return 0;
>> +}
>> +
>> static int dw_pcie_ep_raise_irq(struct pci_epc *epc, u8 func_no,
>> - enum pci_epc_irq_type type, u8 interrupt_num)
>> + enum pci_epc_irq_type type, u16 interrupt_num)
>> {
>> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>
>> @@ -282,6 +354,8 @@ static const struct pci_epc_ops epc_ops = {
>> .unmap_addr = dw_pcie_ep_unmap_addr,
>> .set_msi = dw_pcie_ep_set_msi,
>> .get_msi = dw_pcie_ep_get_msi,
>> + .set_msix = dw_pcie_ep_set_msix,
>> + .get_msix = dw_pcie_ep_get_msix,
>> .raise_irq = dw_pcie_ep_raise_irq,
>> .start = dw_pcie_ep_start,
>> .stop = dw_pcie_ep_stop,
>> @@ -322,6 +396,64 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
>> return 0;
>> }
>>
>> +int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
>> + u16 interrupt_num)
>> +{
>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> + struct pci_epc *epc = ep->epc;
>> + u16 tbl_offset, bir;
>> + u32 bar_addr_upper, bar_addr_lower;
>> + u32 msg_addr_upper, msg_addr_lower;
>> + u32 reg, msg_data, vec_ctrl;
>> + u64 tbl_addr, msg_addr, reg_u64;
>> + void __iomem *msix_tbl;
>> + int ret;
>> +
>> + reg = ep->msix_cap + PCI_MSIX_TABLE;
>> + tbl_offset = dw_pcie_readl_dbi(pci, reg);
>> + bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
>> + tbl_offset &= PCI_MSIX_TABLE_OFFSET;
>> + tbl_offset >>= 3;
>> +
>> + reg = PCI_BASE_ADDRESS_0 + (4 * bir);
>> + bar_addr_upper = 0;
>> + bar_addr_lower = dw_pcie_readl_dbi(pci, reg);
>> + reg_u64 = (bar_addr_lower & PCI_BASE_ADDRESS_MEM_TYPE_MASK);
>> + if (reg_u64 == PCI_BASE_ADDRESS_MEM_TYPE_64)
>> + bar_addr_upper = dw_pcie_readl_dbi(pci, reg + 4);
>> +
>> + tbl_addr = ((u64) bar_addr_upper) << 32 | bar_addr_lower;
>> + tbl_addr += (tbl_offset + ((interrupt_num - 1) * PCI_MSIX_ENTRY_SIZE));
>> + tbl_addr &= PCI_BASE_ADDRESS_MEM_MASK;
>> +
>> + msix_tbl = ioremap_nocache(ep->phys_base + tbl_addr,
>> + PCI_MSIX_ENTRY_SIZE);
>> + if (!msix_tbl)
>> + return -EINVAL;
>> +
>> + msg_addr_lower = readl(msix_tbl + PCI_MSIX_ENTRY_LOWER_ADDR);
>> + msg_addr_upper = readl(msix_tbl + PCI_MSIX_ENTRY_UPPER_ADDR);
>> + msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
>> + msg_data = readl(msix_tbl + PCI_MSIX_ENTRY_DATA);
>> + vec_ctrl = readl(msix_tbl + PCI_MSIX_ENTRY_VECTOR_CTRL);
>> +
>> + iounmap(msix_tbl);
>> +
>> + if (vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)
>> + return -EPERM;
>> +
>> + ret = dw_pcie_ep_map_addr(epc, func_no, ep->msix_mem_phys, msg_addr,
>> + epc->mem->page_size);
>> + if (ret)
>> + return ret;
>> +
>> + writel(msg_data, ep->msix_mem);
>> +
>> + dw_pcie_ep_unmap_addr(epc, func_no, ep->msix_mem_phys);
>> +
>> + return 0;
>> +}
>> +
>> void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
>> {
>> struct pci_epc *epc = ep->epc;
>> @@ -329,6 +461,9 @@ void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
>> pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
>> epc->mem->page_size);
>>
>> + pci_epc_mem_free_addr(epc, ep->msix_mem_phys, ep->msix_mem,
>> + epc->mem->page_size);
>> +
>> pci_epc_mem_exit(epc);
>> }
>>
>> @@ -412,6 +547,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>> dev_err(dev, "Failed to reserve memory for MSI\n");
>> return -ENOMEM;
>> }
>> + ep->msi_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSI);
>> +
>> + ep->msix_mem = pci_epc_mem_alloc_addr(epc, &ep->msix_mem_phys,
>> + epc->mem->page_size);
>> + if (!ep->msix_mem) {
>> + dev_err(dev, "Failed to reserve memory for MSI-X\n");
>> + return -ENOMEM;
>> + }
>> + ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
>>
>> epc->features = EPC_FEATURE_NO_LINKUP_NOTIFIER;
>> EPC_FEATURE_SET_BAR(epc->features, BAR_0);
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
>> index 5937fed..654dcb5 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-plat.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
>> @@ -78,7 +78,7 @@ static void dw_plat_pcie_ep_init(struct dw_pcie_ep *ep)
>>
>> static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> enum pci_epc_irq_type type,
>> - u8 interrupt_num)
>> + u16 interrupt_num)
>> {
>> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>
>> @@ -88,6 +88,8 @@ static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> return -EINVAL;
>> case PCI_EPC_IRQ_MSI:
>> return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
>> + case PCI_EPC_IRQ_MSIX:
>> + return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
>> default:
>> dev_err(pci->dev, "UNKNOWN IRQ type\n");
>> }
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index bee4e25..b22c5bb 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -191,7 +191,7 @@ enum dw_pcie_as_type {
>> struct dw_pcie_ep_ops {
>> void (*ep_init)(struct dw_pcie_ep *ep);
>> int (*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
>> - enum pci_epc_irq_type type, u8 interrupt_num);
>> + enum pci_epc_irq_type type, u16 interrupt_num);
>> };
>>
>> struct dw_pcie_ep {
>> @@ -208,6 +208,10 @@ struct dw_pcie_ep {
>> u32 num_ob_windows;
>> void __iomem *msi_mem;
>> phys_addr_t msi_mem_phys;
>> + void __iomem *msix_mem;
>> + phys_addr_t msix_mem_phys;
>> + u8 msi_cap; /* MSI capability offset */
>> + u8 msix_cap; /* MSI-X capability offset */
>> };
>>
>> struct dw_pcie_ops {
>> @@ -359,6 +363,8 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep);
>> void dw_pcie_ep_exit(struct dw_pcie_ep *ep);
>> int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
>> u8 interrupt_num);
>> +int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
>> + u16 interrupt_num);
>> void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar);
>> #else
>> static inline void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
>> @@ -380,6 +386,12 @@ static inline int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
>> return 0;
>> }
>>
>> +static inline int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
>> + u16 interrupt_num)
>> +{
>> + return 0;
>> +}
>> +
>> static inline void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>> {
>> }
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 04/10] PCI: dwc: Rework MSI callbacks handler
From: Kishon Vijay Abraham I @ 2018-06-20 6:49 UTC (permalink / raw)
To: Gustavo Pimentel, bhelgaas, lorenzo.pieralisi, Joao.Pinto,
jingoohan1, adouglas, jesper.nilsson
Cc: linux-pci, linux-doc, linux-kernel
In-Reply-To: <c6320411aa93d76e3e861f6275cebb1caf6cf893.1529329262.git.gustavo.pimentel@synopsys.com>
Hi,
On Monday 18 June 2018 08:30 PM, Gustavo Pimentel wrote:
> Remove duplicate defines located on pcie-designware.h file already
> available on /include/uapi/linux/pci-regs.h file.
>
> Add pci_epc_set_msi() maximum 32 interrupts validation.
>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> Change v1->v2:
> - Nothing changed, just to follow the patch set version.
> Change v2->v3:
> - Replaced wrong return value 0 to -EINVAL.
> Change v3->v4:
> - Rebased to Lorenzo's master branch v4.18-rc1.
>
> drivers/pci/controller/dwc/pcie-designware-ep.c | 49 +++++++++++++++++--------
> drivers/pci/controller/dwc/pcie-designware.h | 11 ------
> drivers/pci/endpoint/pci-epc-core.c | 3 +-
> 3 files changed, 35 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index ad25654..89d9e52 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -246,29 +246,38 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no,
>
> static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no)
> {
> - int val;
> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> + u32 val, reg;
> +
> + if (!ep->msi_cap)
> + return -EINVAL;
>
> - val = dw_pcie_readw_dbi(pci, MSI_MESSAGE_CONTROL);
> - if (!(val & MSI_CAP_MSI_EN_MASK))
> + reg = ep->msi_cap + PCI_MSI_FLAGS;
> + val = dw_pcie_readw_dbi(pci, reg);
> + if (!(val & PCI_MSI_FLAGS_ENABLE))
> return -EINVAL;
>
> - val = (val & MSI_CAP_MME_MASK) >> MSI_CAP_MME_SHIFT;
> + val = (val & PCI_MSI_FLAGS_QSIZE) >> 4;
> +
> return val;
> }
>
> -static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 encode_int)
> +static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 interrupts)
> {
> - int val;
> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> + u32 val, reg;
>
> - val = dw_pcie_readw_dbi(pci, MSI_MESSAGE_CONTROL);
> - val &= ~MSI_CAP_MMC_MASK;
> - val |= (encode_int << MSI_CAP_MMC_SHIFT) & MSI_CAP_MMC_MASK;
> + if (!ep->msi_cap)
> + return -EINVAL;
> +
> + reg = ep->msi_cap + PCI_MSI_FLAGS;
> + val = dw_pcie_readw_dbi(pci, reg);
> + val &= ~PCI_MSI_FLAGS_QMASK;
> + val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
> dw_pcie_dbi_ro_wr_en(pci);
> - dw_pcie_writew_dbi(pci, MSI_MESSAGE_CONTROL, val);
> + dw_pcie_writew_dbi(pci, reg, val);
> dw_pcie_dbi_ro_wr_dis(pci);
>
> return 0;
> @@ -367,21 +376,29 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> struct pci_epc *epc = ep->epc;
> u16 msg_ctrl, msg_data;
> - u32 msg_addr_lower, msg_addr_upper;
> + u32 msg_addr_lower, msg_addr_upper, reg;
> u64 msg_addr;
> bool has_upper;
> int ret;
>
> + if (!ep->msi_cap)
> + return -EINVAL;
> +
> /* Raise MSI per the PCI Local Bus Specification Revision 3.0, 6.8.1. */
> - msg_ctrl = dw_pcie_readw_dbi(pci, MSI_MESSAGE_CONTROL);
> + reg = ep->msi_cap + PCI_MSI_FLAGS;
> + msg_ctrl = dw_pcie_readw_dbi(pci, reg);
> has_upper = !!(msg_ctrl & PCI_MSI_FLAGS_64BIT);
> - msg_addr_lower = dw_pcie_readl_dbi(pci, MSI_MESSAGE_ADDR_L32);
> + reg = ep->msi_cap + PCI_MSI_ADDRESS_LO;
> + msg_addr_lower = dw_pcie_readl_dbi(pci, reg);
> if (has_upper) {
> - msg_addr_upper = dw_pcie_readl_dbi(pci, MSI_MESSAGE_ADDR_U32);
> - msg_data = dw_pcie_readw_dbi(pci, MSI_MESSAGE_DATA_64);
> + reg = ep->msi_cap + PCI_MSI_ADDRESS_HI;
> + msg_addr_upper = dw_pcie_readl_dbi(pci, reg);
> + reg = ep->msi_cap + PCI_MSI_DATA_64;
> + msg_data = dw_pcie_readw_dbi(pci, reg);
> } else {
> msg_addr_upper = 0;
> - msg_data = dw_pcie_readw_dbi(pci, MSI_MESSAGE_DATA_32);
> + reg = ep->msi_cap + PCI_MSI_DATA_32;
> + msg_data = dw_pcie_readw_dbi(pci, reg);
> }
> msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
> ret = dw_pcie_ep_map_addr(epc, func_no, ep->msi_mem_phys, msg_addr,
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index b22c5bb..a0ab12f 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -96,17 +96,6 @@
> #define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region) \
> ((0x3 << 20) | ((region) << 9) | (0x1 << 8))
>
> -#define MSI_MESSAGE_CONTROL 0x52
> -#define MSI_CAP_MMC_SHIFT 1
> -#define MSI_CAP_MMC_MASK (7 << MSI_CAP_MMC_SHIFT)
> -#define MSI_CAP_MME_SHIFT 4
> -#define MSI_CAP_MSI_EN_MASK 0x1
> -#define MSI_CAP_MME_MASK (7 << MSI_CAP_MME_SHIFT)
> -#define MSI_MESSAGE_ADDR_L32 0x54
> -#define MSI_MESSAGE_ADDR_U32 0x58
> -#define MSI_MESSAGE_DATA_32 0x58
> -#define MSI_MESSAGE_DATA_64 0x5C
> -
> #define MAX_MSI_IRQS 256
> #define MAX_MSI_IRQS_PER_CTRL 32
> #define MAX_MSI_CTRLS (MAX_MSI_IRQS / MAX_MSI_IRQS_PER_CTRL)
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index c72e656..094dcc3 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -201,7 +201,8 @@ int pci_epc_set_msi(struct pci_epc *epc, u8 func_no, u8 interrupts)
> u8 encode_int;
> unsigned long flags;
>
> - if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> + if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
> + interrupts > 32)
> return -EINVAL;
This is not related to $patch->subject
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 03/10] PCI: Update xxx_pcie_ep_raise_irq() and pci_epc_raise_irq() signatures
From: Kishon Vijay Abraham I @ 2018-06-20 6:47 UTC (permalink / raw)
To: Gustavo Pimentel, bhelgaas, lorenzo.pieralisi, Joao.Pinto,
jingoohan1, adouglas, jesper.nilsson
Cc: linux-pci, linux-doc, linux-kernel
In-Reply-To: <5bfa0cf053878e0e859a8b97227ca03ea4d50e92.1529329262.git.gustavo.pimentel@synopsys.com>
Hi,
On Monday 18 June 2018 08:30 PM, Gustavo Pimentel wrote:
> Change {cdns, dra7xx, artpec6}_pcie_ep_raise_irq() and pci_epc_raise_irq()
> signature, namely the interrupt_num variable type from u8 to u16 to
> accommodate 2048 maximum MSI-X interrupts.
>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Acked-by: Alan Douglas <adouglas@cadence.com>
This patch should precede patch #2 and the following hunk from patch #2 should
be added here.
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -191,7 +191,7 @@ enum dw_pcie_as_type {
struct dw_pcie_ep_ops {
void (*ep_init)(struct dw_pcie_ep *ep);
int (*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
- enum pci_epc_irq_type type, u8 interrupt_num);
+ enum pci_epc_irq_type type, u16 interrupt_num);
};
Thanks
Kishon
> ---
> Change v1->v2:
> - Nothing changed, just to follow the patch set version.
> Change v2->v3:
> - Move into here the pci_epc_raise_irq() signature change from patch
> file #1.
> - Move into here the {dra7xx, artpec6}_pcie_ep_raise_irq() signature
> changes from patch file #2.
> Change v3->v4:
> - Rebased to Lorenzo's master branch v4.18-rc1.
>
> drivers/pci/controller/dwc/pci-dra7xx.c | 2 +-
> drivers/pci/controller/dwc/pcie-artpec6.c | 2 +-
> drivers/pci/controller/pcie-cadence-ep.c | 3 ++-
> drivers/pci/endpoint/pci-epc-core.c | 8 ++++----
> include/linux/pci-epc.h | 6 +++---
> 5 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> index 345aab5..ce9224a 100644
> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> @@ -370,7 +370,7 @@ static void dra7xx_pcie_raise_msi_irq(struct dra7xx_pcie *dra7xx,
> }
>
> static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> - enum pci_epc_irq_type type, u8 interrupt_num)
> + enum pci_epc_irq_type type, u16 interrupt_num)
> {
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
> diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c b/drivers/pci/controller/dwc/pcie-artpec6.c
> index 321b56c..9a2474b 100644
> --- a/drivers/pci/controller/dwc/pcie-artpec6.c
> +++ b/drivers/pci/controller/dwc/pcie-artpec6.c
> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
> }
>
> static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> - enum pci_epc_irq_type type, u8 interrupt_num)
> + enum pci_epc_irq_type type, u16 interrupt_num)
> {
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>
> diff --git a/drivers/pci/controller/pcie-cadence-ep.c b/drivers/pci/controller/pcie-cadence-ep.c
> index e3fe412..208d11f 100644
> --- a/drivers/pci/controller/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/pcie-cadence-ep.c
> @@ -363,7 +363,8 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn,
> }
>
> static int cdns_pcie_ep_raise_irq(struct pci_epc *epc, u8 fn,
> - enum pci_epc_irq_type type, u8 interrupt_num)
> + enum pci_epc_irq_type type,
> + u16 interrupt_num)
> {
> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 7d77bd0..c72e656 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -131,13 +131,13 @@ EXPORT_SYMBOL_GPL(pci_epc_start);
> * pci_epc_raise_irq() - interrupt the host system
> * @epc: the EPC device which has to interrupt the host
> * @func_no: the endpoint function number in the EPC device
> - * @type: specify the type of interrupt; legacy or MSI
> - * @interrupt_num: the MSI interrupt number
> + * @type: specify the type of interrupt; legacy, MSI or MSI-X
> + * @interrupt_num: the MSI or MSI-X interrupt number
> *
> - * Invoke to raise an MSI or legacy interrupt
> + * Invoke to raise an legacy, MSI or MSI-X interrupt
> */
> int pci_epc_raise_irq(struct pci_epc *epc, u8 func_no,
> - enum pci_epc_irq_type type, u8 interrupt_num)
> + enum pci_epc_irq_type type, u16 interrupt_num)
> {
> int ret;
> unsigned long flags;
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index 89f079f..bb2395b 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -35,7 +35,7 @@ enum pci_epc_irq_type {
> * MSI-X capability register
> * @get_msix: ops to get the number of MSI-X interrupts allocated by the RC
> * from the MSI-X capability register
> - * @raise_irq: ops to raise a legacy or MSI interrupt
> + * @raise_irq: ops to raise a legacy, MSI or MSI-X interrupt
> * @start: ops to start the PCI link
> * @stop: ops to stop the PCI link
> * @owner: the module owner containing the ops
> @@ -56,7 +56,7 @@ struct pci_epc_ops {
> int (*set_msix)(struct pci_epc *epc, u8 func_no, u16 interrupts);
> int (*get_msix)(struct pci_epc *epc, u8 func_no);
> int (*raise_irq)(struct pci_epc *epc, u8 func_no,
> - enum pci_epc_irq_type type, u8 interrupt_num);
> + enum pci_epc_irq_type type, u16 interrupt_num);
> int (*start)(struct pci_epc *epc);
> void (*stop)(struct pci_epc *epc);
> struct module *owner;
> @@ -154,7 +154,7 @@ int pci_epc_get_msi(struct pci_epc *epc, u8 func_no);
> int pci_epc_set_msix(struct pci_epc *epc, u8 func_no, u16 interrupts);
> int pci_epc_get_msix(struct pci_epc *epc, u8 func_no);
> int pci_epc_raise_irq(struct pci_epc *epc, u8 func_no,
> - enum pci_epc_irq_type type, u8 interrupt_num);
> + enum pci_epc_irq_type type, u16 interrupt_num);
> int pci_epc_start(struct pci_epc *epc);
> void pci_epc_stop(struct pci_epc *epc);
> struct pci_epc *pci_epc_get(const char *epc_name);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 03/10] PCI: Update xxx_pcie_ep_raise_irq() and pci_epc_raise_irq() signatures
From: Gustavo Pimentel @ 2018-06-20 10:05 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Gustavo Pimentel, bhelgaas@google.com,
lorenzo.pieralisi@arm.com, Joao.Pinto@synopsys.com,
jingoohan1@gmail.com, adouglas@cadence.com,
jesper.nilsson@axis.com
Cc: linux-pci@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <355c0797-1e86-fc0e-ca61-b5eb00835d36@ti.com>
Hi,
On 20/06/2018 07:47, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Monday 18 June 2018 08:30 PM, Gustavo Pimentel wrote:
>> Change {cdns, dra7xx, artpec6}_pcie_ep_raise_irq() and pci_epc_raise_irq()
>> signature, namely the interrupt_num variable type from u8 to u16 to
>> accommodate 2048 maximum MSI-X interrupts.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Acked-by: Alan Douglas <adouglas@cadence.com>
>
> This patch should precede patch #2 and the following hunk from patch #2 should
> be added here.
>
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -191,7 +191,7 @@ enum dw_pcie_as_type {
> struct dw_pcie_ep_ops {
> void (*ep_init)(struct dw_pcie_ep *ep);
> int (*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
> - enum pci_epc_irq_type type, u8 interrupt_num);
> + enum pci_epc_irq_type type, u16 interrupt_num);
> };
Yes, it it makes a lot more sense in the other patch file.
Thanks.
Gustavo
>
> Thanks
> Kishon
>
>> ---
>> Change v1->v2:
>> - Nothing changed, just to follow the patch set version.
>> Change v2->v3:
>> - Move into here the pci_epc_raise_irq() signature change from patch
>> file #1.
>> - Move into here the {dra7xx, artpec6}_pcie_ep_raise_irq() signature
>> changes from patch file #2.
>> Change v3->v4:
>> - Rebased to Lorenzo's master branch v4.18-rc1.
>>
>> drivers/pci/controller/dwc/pci-dra7xx.c | 2 +-
>> drivers/pci/controller/dwc/pcie-artpec6.c | 2 +-
>> drivers/pci/controller/pcie-cadence-ep.c | 3 ++-
>> drivers/pci/endpoint/pci-epc-core.c | 8 ++++----
>> include/linux/pci-epc.h | 6 +++---
>> 5 files changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
>> index 345aab5..ce9224a 100644
>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
>> @@ -370,7 +370,7 @@ static void dra7xx_pcie_raise_msi_irq(struct dra7xx_pcie *dra7xx,
>> }
>>
>> static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> - enum pci_epc_irq_type type, u8 interrupt_num)
>> + enum pci_epc_irq_type type, u16 interrupt_num)
>> {
>> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
>> diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c b/drivers/pci/controller/dwc/pcie-artpec6.c
>> index 321b56c..9a2474b 100644
>> --- a/drivers/pci/controller/dwc/pcie-artpec6.c
>> +++ b/drivers/pci/controller/dwc/pcie-artpec6.c
>> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
>> }
>>
>> static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> - enum pci_epc_irq_type type, u8 interrupt_num)
>> + enum pci_epc_irq_type type, u16 interrupt_num)
>> {
>> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>
>> diff --git a/drivers/pci/controller/pcie-cadence-ep.c b/drivers/pci/controller/pcie-cadence-ep.c
>> index e3fe412..208d11f 100644
>> --- a/drivers/pci/controller/pcie-cadence-ep.c
>> +++ b/drivers/pci/controller/pcie-cadence-ep.c
>> @@ -363,7 +363,8 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn,
>> }
>>
>> static int cdns_pcie_ep_raise_irq(struct pci_epc *epc, u8 fn,
>> - enum pci_epc_irq_type type, u8 interrupt_num)
>> + enum pci_epc_irq_type type,
>> + u16 interrupt_num)
>> {
>> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>>
>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>> index 7d77bd0..c72e656 100644
>> --- a/drivers/pci/endpoint/pci-epc-core.c
>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>> @@ -131,13 +131,13 @@ EXPORT_SYMBOL_GPL(pci_epc_start);
>> * pci_epc_raise_irq() - interrupt the host system
>> * @epc: the EPC device which has to interrupt the host
>> * @func_no: the endpoint function number in the EPC device
>> - * @type: specify the type of interrupt; legacy or MSI
>> - * @interrupt_num: the MSI interrupt number
>> + * @type: specify the type of interrupt; legacy, MSI or MSI-X
>> + * @interrupt_num: the MSI or MSI-X interrupt number
>> *
>> - * Invoke to raise an MSI or legacy interrupt
>> + * Invoke to raise an legacy, MSI or MSI-X interrupt
>> */
>> int pci_epc_raise_irq(struct pci_epc *epc, u8 func_no,
>> - enum pci_epc_irq_type type, u8 interrupt_num)
>> + enum pci_epc_irq_type type, u16 interrupt_num)
>> {
>> int ret;
>> unsigned long flags;
>> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
>> index 89f079f..bb2395b 100644
>> --- a/include/linux/pci-epc.h
>> +++ b/include/linux/pci-epc.h
>> @@ -35,7 +35,7 @@ enum pci_epc_irq_type {
>> * MSI-X capability register
>> * @get_msix: ops to get the number of MSI-X interrupts allocated by the RC
>> * from the MSI-X capability register
>> - * @raise_irq: ops to raise a legacy or MSI interrupt
>> + * @raise_irq: ops to raise a legacy, MSI or MSI-X interrupt
>> * @start: ops to start the PCI link
>> * @stop: ops to stop the PCI link
>> * @owner: the module owner containing the ops
>> @@ -56,7 +56,7 @@ struct pci_epc_ops {
>> int (*set_msix)(struct pci_epc *epc, u8 func_no, u16 interrupts);
>> int (*get_msix)(struct pci_epc *epc, u8 func_no);
>> int (*raise_irq)(struct pci_epc *epc, u8 func_no,
>> - enum pci_epc_irq_type type, u8 interrupt_num);
>> + enum pci_epc_irq_type type, u16 interrupt_num);
>> int (*start)(struct pci_epc *epc);
>> void (*stop)(struct pci_epc *epc);
>> struct module *owner;
>> @@ -154,7 +154,7 @@ int pci_epc_get_msi(struct pci_epc *epc, u8 func_no);
>> int pci_epc_set_msix(struct pci_epc *epc, u8 func_no, u16 interrupts);
>> int pci_epc_get_msix(struct pci_epc *epc, u8 func_no);
>> int pci_epc_raise_irq(struct pci_epc *epc, u8 func_no,
>> - enum pci_epc_irq_type type, u8 interrupt_num);
>> + enum pci_epc_irq_type type, u16 interrupt_num);
>> int pci_epc_start(struct pci_epc *epc);
>> void pci_epc_stop(struct pci_epc *epc);
>> struct pci_epc *pci_epc_get(const char *epc_name);
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 04/10] PCI: dwc: Rework MSI callbacks handler
From: Gustavo Pimentel @ 2018-06-20 10:20 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Gustavo Pimentel, bhelgaas@google.com,
lorenzo.pieralisi@arm.com, Joao.Pinto@synopsys.com,
jingoohan1@gmail.com, adouglas@cadence.com,
jesper.nilsson@axis.com
Cc: linux-pci@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <ef846300-1606-397e-c790-45ddee6b2417@ti.com>
Hi,
On 20/06/2018 07:49, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Monday 18 June 2018 08:30 PM, Gustavo Pimentel wrote:
>> Remove duplicate defines located on pcie-designware.h file already
>> available on /include/uapi/linux/pci-regs.h file.
>>
>> Add pci_epc_set_msi() maximum 32 interrupts validation.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>> Change v1->v2:
>> - Nothing changed, just to follow the patch set version.
>> Change v2->v3:
>> - Replaced wrong return value 0 to -EINVAL.
>> Change v3->v4:
>> - Rebased to Lorenzo's master branch v4.18-rc1.
>>
>> drivers/pci/controller/dwc/pcie-designware-ep.c | 49 +++++++++++++++++--------
>> drivers/pci/controller/dwc/pcie-designware.h | 11 ------
>> drivers/pci/endpoint/pci-epc-core.c | 3 +-
>> 3 files changed, 35 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> index ad25654..89d9e52 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> @@ -246,29 +246,38 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no,
>>
>> static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no)
>> {
>> - int val;
>> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> + u32 val, reg;
>> +
>> + if (!ep->msi_cap)
>> + return -EINVAL;
>>
>> - val = dw_pcie_readw_dbi(pci, MSI_MESSAGE_CONTROL);
>> - if (!(val & MSI_CAP_MSI_EN_MASK))
>> + reg = ep->msi_cap + PCI_MSI_FLAGS;
>> + val = dw_pcie_readw_dbi(pci, reg);
>> + if (!(val & PCI_MSI_FLAGS_ENABLE))
>> return -EINVAL;
>>
>> - val = (val & MSI_CAP_MME_MASK) >> MSI_CAP_MME_SHIFT;
>> + val = (val & PCI_MSI_FLAGS_QSIZE) >> 4;
>> +
>> return val;
>> }
>>
>> -static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 encode_int)
>> +static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 interrupts)
>> {
>> - int val;
>> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> + u32 val, reg;
>>
>> - val = dw_pcie_readw_dbi(pci, MSI_MESSAGE_CONTROL);
>> - val &= ~MSI_CAP_MMC_MASK;
>> - val |= (encode_int << MSI_CAP_MMC_SHIFT) & MSI_CAP_MMC_MASK;
>> + if (!ep->msi_cap)
>> + return -EINVAL;
>> +
>> + reg = ep->msi_cap + PCI_MSI_FLAGS;
>> + val = dw_pcie_readw_dbi(pci, reg);
>> + val &= ~PCI_MSI_FLAGS_QMASK;
>> + val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
>> dw_pcie_dbi_ro_wr_en(pci);
>> - dw_pcie_writew_dbi(pci, MSI_MESSAGE_CONTROL, val);
>> + dw_pcie_writew_dbi(pci, reg, val);
>> dw_pcie_dbi_ro_wr_dis(pci);
>>
>> return 0;
>> @@ -367,21 +376,29 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
>> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> struct pci_epc *epc = ep->epc;
>> u16 msg_ctrl, msg_data;
>> - u32 msg_addr_lower, msg_addr_upper;
>> + u32 msg_addr_lower, msg_addr_upper, reg;
>> u64 msg_addr;
>> bool has_upper;
>> int ret;
>>
>> + if (!ep->msi_cap)
>> + return -EINVAL;
>> +
>> /* Raise MSI per the PCI Local Bus Specification Revision 3.0, 6.8.1. */
>> - msg_ctrl = dw_pcie_readw_dbi(pci, MSI_MESSAGE_CONTROL);
>> + reg = ep->msi_cap + PCI_MSI_FLAGS;
>> + msg_ctrl = dw_pcie_readw_dbi(pci, reg);
>> has_upper = !!(msg_ctrl & PCI_MSI_FLAGS_64BIT);
>> - msg_addr_lower = dw_pcie_readl_dbi(pci, MSI_MESSAGE_ADDR_L32);
>> + reg = ep->msi_cap + PCI_MSI_ADDRESS_LO;
>> + msg_addr_lower = dw_pcie_readl_dbi(pci, reg);
>> if (has_upper) {
>> - msg_addr_upper = dw_pcie_readl_dbi(pci, MSI_MESSAGE_ADDR_U32);
>> - msg_data = dw_pcie_readw_dbi(pci, MSI_MESSAGE_DATA_64);
>> + reg = ep->msi_cap + PCI_MSI_ADDRESS_HI;
>> + msg_addr_upper = dw_pcie_readl_dbi(pci, reg);
>> + reg = ep->msi_cap + PCI_MSI_DATA_64;
>> + msg_data = dw_pcie_readw_dbi(pci, reg);
>> } else {
>> msg_addr_upper = 0;
>> - msg_data = dw_pcie_readw_dbi(pci, MSI_MESSAGE_DATA_32);
>> + reg = ep->msi_cap + PCI_MSI_DATA_32;
>> + msg_data = dw_pcie_readw_dbi(pci, reg);
>> }
>> msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
>> ret = dw_pcie_ep_map_addr(epc, func_no, ep->msi_mem_phys, msg_addr,
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index b22c5bb..a0ab12f 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -96,17 +96,6 @@
>> #define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region) \
>> ((0x3 << 20) | ((region) << 9) | (0x1 << 8))
>>
>> -#define MSI_MESSAGE_CONTROL 0x52
>> -#define MSI_CAP_MMC_SHIFT 1
>> -#define MSI_CAP_MMC_MASK (7 << MSI_CAP_MMC_SHIFT)
>> -#define MSI_CAP_MME_SHIFT 4
>> -#define MSI_CAP_MSI_EN_MASK 0x1
>> -#define MSI_CAP_MME_MASK (7 << MSI_CAP_MME_SHIFT)
>> -#define MSI_MESSAGE_ADDR_L32 0x54
>> -#define MSI_MESSAGE_ADDR_U32 0x58
>> -#define MSI_MESSAGE_DATA_32 0x58
>> -#define MSI_MESSAGE_DATA_64 0x5C
>> -
>> #define MAX_MSI_IRQS 256
>> #define MAX_MSI_IRQS_PER_CTRL 32
>> #define MAX_MSI_CTRLS (MAX_MSI_IRQS / MAX_MSI_IRQS_PER_CTRL)
>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>> index c72e656..094dcc3 100644
>> --- a/drivers/pci/endpoint/pci-epc-core.c
>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>> @@ -201,7 +201,8 @@ int pci_epc_set_msi(struct pci_epc *epc, u8 func_no, u8 interrupts)
>> u8 encode_int;
>> unsigned long flags;
>>
>> - if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
>> + if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
>> + interrupts > 32)
>> return -EINVAL;
>
> This is not related to $patch->subject
A new patch file will be created for this.
Thanks,
Gustavo
>
> Thanks
> Kishon
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 06/10] pci-epf-test/pci_endpoint_test: Cleanup PCI_ENDPOINT_TEST memspace
From: Kishon Vijay Abraham I @ 2018-06-20 7:53 UTC (permalink / raw)
To: Gustavo Pimentel, bhelgaas, lorenzo.pieralisi, Joao.Pinto,
jingoohan1, adouglas, jesper.nilsson
Cc: linux-pci, linux-doc, linux-kernel
In-Reply-To: <13077810f3630c01835041e41b338dd85fa91953.1529329262.git.gustavo.pimentel@synopsys.com>
Hi,
On Monday 18 June 2018 08:30 PM, Gustavo Pimentel wrote:
> Cleanup PCI_ENDPOINT_TEST memspace (by moving the interrupt number away
> from command section).
>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> Change v2->v3:
> - New patch file created base on the previous patch
> "misc: pci_endpoint_test: Add MSI-X support" patch file following
> Kishon's suggestion.
> Change v3->v4:
> - Rebased to Lorenzo's master branch v4.18-rc1.
>
> drivers/misc/pci_endpoint_test.c | 84 ++++++++++++++++-----------
> drivers/pci/endpoint/functions/pci-epf-test.c | 60 ++++++++++++-------
> 2 files changed, 91 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 7b37046..56be808 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -35,38 +35,42 @@
>
> #include <uapi/linux/pcitest.h>
>
> -#define DRV_MODULE_NAME "pci-endpoint-test"
> -
> -#define PCI_ENDPOINT_TEST_MAGIC 0x0
> -
> -#define PCI_ENDPOINT_TEST_COMMAND 0x4
> -#define COMMAND_RAISE_LEGACY_IRQ BIT(0)
> -#define COMMAND_RAISE_MSI_IRQ BIT(1)
> -#define MSI_NUMBER_SHIFT 2
> -/* 6 bits for MSI number */
> -#define COMMAND_READ BIT(8)
> -#define COMMAND_WRITE BIT(9)
> -#define COMMAND_COPY BIT(10)
> -
> -#define PCI_ENDPOINT_TEST_STATUS 0x8
> -#define STATUS_READ_SUCCESS BIT(0)
> -#define STATUS_READ_FAIL BIT(1)
> -#define STATUS_WRITE_SUCCESS BIT(2)
> -#define STATUS_WRITE_FAIL BIT(3)
> -#define STATUS_COPY_SUCCESS BIT(4)
> -#define STATUS_COPY_FAIL BIT(5)
> -#define STATUS_IRQ_RAISED BIT(6)
> -#define STATUS_SRC_ADDR_INVALID BIT(7)
> -#define STATUS_DST_ADDR_INVALID BIT(8)
> -
> -#define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0xc
> +#define DRV_MODULE_NAME "pci-endpoint-test"
> +
> +#define IRQ_TYPE_LEGACY 0
> +#define IRQ_TYPE_MSI 1
> +
> +#define PCI_ENDPOINT_TEST_MAGIC 0x0
> +
> +#define PCI_ENDPOINT_TEST_COMMAND 0x4
> +#define COMMAND_RAISE_LEGACY_IRQ BIT(0)
> +#define COMMAND_RAISE_MSI_IRQ BIT(1)
Maybe you can add a comment here that BIT(2) is reserved for MSIX support
> +#define COMMAND_READ BIT(3)
> +#define COMMAND_WRITE BIT(4)
> +#define COMMAND_COPY BIT(5)
> +
> +#define PCI_ENDPOINT_TEST_STATUS 0x8
> +#define STATUS_READ_SUCCESS BIT(0)
> +#define STATUS_READ_FAIL BIT(1)
> +#define STATUS_WRITE_SUCCESS BIT(2)
> +#define STATUS_WRITE_FAIL BIT(3)
> +#define STATUS_COPY_SUCCESS BIT(4)
> +#define STATUS_COPY_FAIL BIT(5)
> +#define STATUS_IRQ_RAISED BIT(6)
> +#define STATUS_SRC_ADDR_INVALID BIT(7)
> +#define STATUS_DST_ADDR_INVALID BIT(8)
> +
> +#define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0x0c
> #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10
>
> #define PCI_ENDPOINT_TEST_LOWER_DST_ADDR 0x14
> #define PCI_ENDPOINT_TEST_UPPER_DST_ADDR 0x18
>
> -#define PCI_ENDPOINT_TEST_SIZE 0x1c
> -#define PCI_ENDPOINT_TEST_CHECKSUM 0x20
> +#define PCI_ENDPOINT_TEST_SIZE 0x1c
> +#define PCI_ENDPOINT_TEST_CHECKSUM 0x20
> +
> +#define PCI_ENDPOINT_TEST_IRQ_TYPE 0x24
Is this not redundant? COMMAND_RAISE_LEGACY_IRQ, COMMAND_RAISE_MSI_IRQ already
indicates the irq type to be used.
> +#define PCI_ENDPOINT_TEST_IRQ_NUMBER 0x28
>
> static DEFINE_IDA(pci_endpoint_test_ida);
>
> @@ -179,6 +183,9 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
> {
> u32 val;
>
> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE,
> + IRQ_TYPE_LEGACY);
> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 0);
Not sure if the above writes are really required.
> pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> COMMAND_RAISE_LEGACY_IRQ);
> val = wait_for_completion_timeout(&test->irq_raised,
> @@ -190,20 +197,22 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
> }
>
> static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
> - u8 msi_num)
> + u8 irq_num)
why do you want to rename this?
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 0/5] Kernel parameter parser cleanup/enhancement
From: Michal Suchánek @ 2018-06-20 10:47 UTC (permalink / raw)
To: Andrew Morton
Cc: Jonathan Corbet, Arnd Bergmann, Frederic Weisbecker, Ingo Molnar,
Aaron Wu, Tony Luck, Thomas Gleixner, Steven Rostedt,,
Laura Abbott, Dominik Brodowski, Alexey Dobriyan, Tom Lendacky,
Jeffrey Hugo, Baoquan He, Ilya Matveychikov, linux-doc,
linux-kernel
In-Reply-To: <20180619163647.23c8a5d7370258c9e43141c3@linux-foundation.org>
On Tue, 19 Jun 2018 16:36:47 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 5 Jun 2018 18:43:07 +0200 Michal Suchanek
> <msuchanek@suse.de> wrote:
>
> > due to work on the fadump_extra_args I looked at the kernel
> > parameter parser and found its grammar rather curious.
> >
> > It supports double quotes but not any other quoting characters so
> > double quotes cannot be quoted. What's more, the quotes can be
> > anywhere in the parameter name or value and are interpteted but are
> > removed only from start and end of the parameter value.
> >
> > These are the patches not specific to fadump which somewhat
> > straighten the qouting grammar to make it on par with common shell
> > interpreters.
> >
> > Specifically double and single quotes can be used for quoting as
> > well as backslashes with the usual shell semantic. All quoting
> > characters are removed while the parameters are parsed.
>
> Well. It's nice. I guess. Is there any demand for these
> capabilities? I don't recall ever having seen a complaint - kernel
> parameters tend to be pretty simple things.
Yes, the complaint came with the nested arguments which are now not
pursued anymore. The grammar is really not very nice as it is, though.
> Also, the break_arg_end() and squash_char() macros make me want to
> cry. A macro which changes control flow hidden inside another macro!
> Are they reeeealy necessary?
Seems better than repeating the same code 3 times.
> Can't be done with some C helpers?
You could not change the control flow then, could you?
Technically you could return something and decide based on that I
suppose.
> Maybe put inquote, backslash, args, i into a new struct parser_state
> and pass a pointer to that around the place? At the very least,
> those macros should be apologetically documented :(
Yes, some description can be added, too.
Thanks
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 01/10] i3c: Add core I3C infrastructure
From: Sekhar Nori @ 2018-06-20 11:37 UTC (permalink / raw)
To: Boris Brezillon, Wolfram Sang, linux-i2c, Jonathan Corbet,
linux-doc, Greg Kroah-Hartman, Arnd Bergmann
Cc: Przemyslaw Sroka, Arkadiusz Golec, Alan Douglas, Bartosz Folta,
Damian Kos, Alicja Jurasik-Urbaniak, Cyprian Wronka,
Suresh Punnoose, Rafal Ciepiela, Thomas Petazzoni, Nishanth Menon,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
devicetree, linux-kernel, Vitor Soares, Geert Uytterhoeven,
Linus Walleij, Xiang Lin, linux-gpio
In-Reply-To: <20180330074751.25987-2-boris.brezillon@bootlin.com>
Hi Boris,
On Friday 30 March 2018 01:17 PM, Boris Brezillon wrote:
> Add core infrastructure to support I3C in Linux and document it.
>
> This infrastructure is not complete yet and will be extended over
> time.
>
> There are a few design choices that are worth mentioning because they
> impact the way I3C device drivers can interact with their devices:
>
> - all functions used to send I3C/I2C frames must be called in
> non-atomic context. Mainly done this way to ease implementation, but
> this is still open to discussion. Please let me know if you think
> it's worth considering an asynchronous model here
> - the bus element is a separate object and is not implicitly described
> by the master (as done in I2C). The reason is that I want to be able
> to handle multiple master connected to the same bus and visible to
> Linux.
> In this situation, we should only have one instance of the device and
> not one per master, and sharing the bus object would be part of the
> solution to gracefully handle this case.
> I'm not sure we will ever need to deal with multiple masters
> controlling the same bus and exposed under Linux, but separating the
> bus and master concept is pretty easy, hence the decision to do it
> like that.
There can only be one current master in I3C, so not sure of this
scenario. But agree with bus and master separation.
> The other benefit of separating the bus and master concepts is that
> master devices appear under the bus directory in sysfs.
> - I2C backward compatibility has been designed to be transparent to I2C
> drivers and the I2C subsystem. The I3C master just registers an I2C
> adapter which creates a new I2C bus. I'd say that, from a
> representation PoV it's not ideal because what should appear as a
> single I3C bus exposing I3C and I2C devices here appears as 2
> different busses connected to each other through the parenting (the
> I3C master is the parent of the I2C and I3C busses).
> On the other hand, I don't see a better solution if we want something
> that is not invasive.
>
> Missing features in this preliminary version:
> - I3C HDR modes are not supported
> - no support for multi-master and the associated concepts (mastership
> handover, support for secondary masters, ...)
> - I2C devices can only be described using DT because this is the only
> use case I have. However, the framework can easily be extended with
> ACPI and board info support
> - I3C slave framework. This has been completely omitted, but shouldn't
> have a huge impact on the I3C framework because I3C slaves don't see
> the whole bus, it's only about handling master requests and generating
> IBIs. Some of the struct, constant and enum definitions could be
> shared, but most of the I3C slave framework logic will be different
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 24cd47014657..999239dc29d4 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -111,7 +111,7 @@ obj-$(CONFIG_SERIO) += input/serio/
> obj-$(CONFIG_GAMEPORT) += input/gameport/
> obj-$(CONFIG_INPUT) += input/
> obj-$(CONFIG_RTC_LIB) += rtc/
> -obj-y += i2c/ media/
> +obj-y += i2c/ i3c/ media/
> obj-$(CONFIG_PPS) += pps/
> obj-y += ptp/
> obj-$(CONFIG_W1) += w1/
> diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig
> new file mode 100644
> index 000000000000..cf3752412ae9
> --- /dev/null
> +++ b/drivers/i3c/Kconfig
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +menuconfig I3C
> + tristate "I3C support"
> + select I2C
> + help
> + I3C is a serial protocol standardized by the MIPI alliance.
> +
> + It's supposed to be backward compatible with I2C while providing
> + support for high speed transfers and native interrupt support
> + without the need for extra pins.
> +
> + The I3C protocol also standardizes the slave device types and is
> + mainly design to communicate with sensors.
designed
> +
> + If you want I3C support, you should say Y here and also to the
> + specific driver for your bus adapter(s) below.
> +
> + This I3C support can also be built as a module. If so, the module
> + will be called i3c.
> +
> diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c
> new file mode 100644
> index 000000000000..d6d938a785a9
> --- /dev/null
> +++ b/drivers/i3c/core.c
> +static ssize_t bcr_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> + ssize_t ret;
> +
> + i3c_bus_normaluse_lock(bus);
> + ret = sprintf(buf, "%x\n", i3cdev->info.bcr);
> + i3c_bus_normaluse_unlock(bus);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(bcr);
> +
> +static ssize_t dcr_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> + ssize_t ret;
> +
> + i3c_bus_normaluse_lock(bus);
> + ret = sprintf(buf, "%x\n", i3cdev->info.dcr);
> + i3c_bus_normaluse_unlock(bus);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(dcr);
> +
> +static ssize_t pid_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> + ssize_t ret;
> +
> + i3c_bus_normaluse_lock(bus);
> + ret = sprintf(buf, "%llx\n", i3cdev->info.pid);
> + i3c_bus_normaluse_unlock(bus);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(pid);
> +
> +static ssize_t address_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> + ssize_t ret;
> +
> + i3c_bus_normaluse_lock(bus);
> + ret = sprintf(buf, "%02x\n", i3cdev->info.dyn_addr);
> + i3c_bus_normaluse_unlock(bus);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(address);
should there be separate entries for dynamic and static address?
If yes, you could also reduce the boilerplate by using a macro taking
attribute name and format string.
> +static int i3c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> + u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> + u16 ext = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> +
> + if (I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid))
> + return add_uevent_var(env, "MODALIAS=i3c:dcr%02Xmanuf%04X",
> + i3cdev->info.dcr, manuf);
> +
> + return add_uevent_var(env,
> + "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04xext%04x",
> + i3cdev->info.dcr, manuf, part, ext);
instance id should also be passed in the non-random case?
> +}
> +static const struct i3c_device_id *
> +i3c_device_match_id(struct i3c_device *i3cdev,
> + const struct i3c_device_id *id_table)
> +{
> + const struct i3c_device_id *id;
> +
> + /*
> + * The lower 32bits of the provisional ID is just filled with a random
> + * value, try to match using DCR info.
> + */
> + if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> + u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> + u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> +
> + /* First try to match by manufacturer/part ID. */
> + for (id = id_table; id->match_flags != 0; id++) {
> + if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
> + I3C_MATCH_MANUF_AND_PART)
> + continue;
> +
> + if (manuf != id->manuf_id || part != id->part_id)
> + continue;
> +
> + if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
> + ext_info != id->extra_info)
> + continue;
> +
> + return id;
Here too, instance id is ignored. Seems like done on purpose?
> + }
> + }
> +
> + /* Fallback to DCR match. */
> + for (id = id_table; id->match_flags != 0; id++) {
> + if ((id->match_flags & I3C_MATCH_DCR) &&
> + id->dcr == i3cdev->info.dcr)
> + return id;
> + }
> +
> + return NULL;
> +}
> +static int i3c_device_probe(struct device *dev)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + struct i3c_driver *driver = drv_to_i3cdrv(dev->driver);
> +
> + return driver->probe(i3cdev);
Should you pm_runtime enable the device before probe? Like done for PCI
in local_pci_probe() for example. Or I guess thats a problem because I2C
devices don't expect it?
> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> new file mode 100644
> index 000000000000..8948d9bdec82
> --- /dev/null
> +++ b/drivers/i3c/device.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 Cadence Design Systems Inc.
2018 now :)
> + *
> + * Author: Boris Brezillon <boris.brezillon@bootlin.com>
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/bug.h>
> +#include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +#include "internals.h"
> +
> +/**
> + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a
> + * specific device
> + *
> + * @dev: device with which the transfers should be done
> + * @xfers: array of transfers
> + * @nxfers: number of transfers
> + *
> + * Initiate one or several private SDR transfers with @dev.
> + *
> + * This function can sleep and thus cannot be called in atomic context.
> + *
> + * Return: 0 in case of success, a negative error core otherwise.
> + */
Curious why you specifically call out SDR here. It could be HDR too, in
future. Right?
> +int i3c_device_do_priv_xfers(struct i3c_device *dev,
> + struct i3c_priv_xfer *xfers,
> + int nxfers)
> +{
> + struct i3c_master_controller *master;
> + int ret, i;
> +
> + if (nxfers < 1)
> + return 0;
> +
> + master = i3c_device_get_master(dev);
> + if (!master || !xfers)
> + return -EINVAL;
> +
> + if (!master->ops->priv_xfers)
> + return -ENOTSUPP;
> +
> + for (i = 0; i < nxfers; i++) {
> + if (!xfers[i].len || !xfers[i].data.in)
> + return -EINVAL;
> + }
> +
> + i3c_bus_normaluse_lock(master->bus);
> + ret = master->ops->priv_xfers(dev, xfers, nxfers);
> + i3c_bus_normaluse_unlock(master->bus);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
> +/**
> + * struct i3c_device_info - I3C device information
> + * @pid: Provisional ID
> + * @bcr: Bus Characteristic Register
> + * @dcr: Device Characteristic Register
> + * @static_addr: static/I2C address
> + * @dyn_addr: dynamic address
> + * @hdr_cap: supported HDR modes
This is just for querying and display device capability. We dont
actually enter HDR mode at the moment, right?
> + * @max_read_ds: max read speed information
> + * @max_write_ds: max write speed information
> + * @max_ibi_len: max IBI payload length
> + * @max_read_turnaround: max read turn-around time in micro-seconds
> + * @max_read_len: max private SDR read length in bytes
> + * @max_write_len: max private SDR write length in bytes
> + *
> + * These are all basic information that should be advertised by an I3C device.
> + * Some of them are optional depending on the device type and device
> + * capabilities.
> + * For each I3C slave attached to a master with
> + * i3c_master_add_i3c_dev_locked(), the core will send the relevant CCC command
> + * to retrieve these data.
> + */
> +struct i3c_device_info {
> + u64 pid;
> + u8 bcr;
> + u8 dcr;
> + u8 static_addr;
> + u8 dyn_addr;
> + u8 hdr_cap;
> + u8 max_read_ds;
> + u8 max_write_ds;
> + u8 max_ibi_len;
> + u32 max_read_turnaround;
> + u16 max_read_len;
> + u16 max_write_len;
> +};
Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 01/10] i3c: Add core I3C infrastructure
From: Boris Brezillon @ 2018-06-20 12:47 UTC (permalink / raw)
To: Sekhar Nori
Cc: Wolfram Sang, linux-i2c, Jonathan Corbet, linux-doc,
Greg Kroah-Hartman, Arnd Bergmann, Przemyslaw Sroka,
Arkadiusz Golec, Alan Douglas, Bartosz Folta, Damian Kos,
Alicja Jurasik-Urbaniak, Cyprian Wronka, Suresh Punnoose,
Rafal Ciepiela, Thomas Petazzoni, Nishanth Menon, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree,
linux-kernel, Vitor Soares, Geert Uytterhoeven, Linus Walleij,
Xiang Lin, linux-gpio
In-Reply-To: <1567bbe1-0f11-4cc8-e501-6c8589570259@ti.com>
Hi Sekhar,
On Wed, 20 Jun 2018 17:07:53 +0530
Sekhar Nori <nsekhar@ti.com> wrote:
> Hi Boris,
>
> On Friday 30 March 2018 01:17 PM, Boris Brezillon wrote:
> > Add core infrastructure to support I3C in Linux and document it.
> >
> > This infrastructure is not complete yet and will be extended over
> > time.
> >
> > There are a few design choices that are worth mentioning because they
> > impact the way I3C device drivers can interact with their devices:
> >
> > - all functions used to send I3C/I2C frames must be called in
> > non-atomic context. Mainly done this way to ease implementation, but
> > this is still open to discussion. Please let me know if you think
> > it's worth considering an asynchronous model here
> > - the bus element is a separate object and is not implicitly described
> > by the master (as done in I2C). The reason is that I want to be able
> > to handle multiple master connected to the same bus and visible to
> > Linux.
> > In this situation, we should only have one instance of the device and
> > not one per master, and sharing the bus object would be part of the
> > solution to gracefully handle this case.
> > I'm not sure we will ever need to deal with multiple masters
> > controlling the same bus and exposed under Linux, but separating the
> > bus and master concept is pretty easy, hence the decision to do it
> > like that.
>
> There can only be one current master in I3C, so not sure of this
> scenario.
Yes, there's only one active master at any time, but still, you can
have several masters (one primary and several secondary masters)
connected to the same bus, and you might even have several of them
controlled by the same Linux instance (don't really see a use case for
that right now, but I'm pretty sure this will happen).
The point of having a single bus instance pointed by various I3C masters
in this case is to avoid exposing several times the same I3C device.
If we don't do that we would have one I3C device instance per I3C master
exposed under Linux even though they all control the same physical
device.
> But agree with bus and master separation.
>
> > The other benefit of separating the bus and master concepts is that
> > master devices appear under the bus directory in sysfs.
> > - I2C backward compatibility has been designed to be transparent to I2C
> > drivers and the I2C subsystem. The I3C master just registers an I2C
> > adapter which creates a new I2C bus. I'd say that, from a
> > representation PoV it's not ideal because what should appear as a
> > single I3C bus exposing I3C and I2C devices here appears as 2
> > different busses connected to each other through the parenting (the
> > I3C master is the parent of the I2C and I3C busses).
> > On the other hand, I don't see a better solution if we want something
> > that is not invasive.
> >
> > Missing features in this preliminary version:
> > - I3C HDR modes are not supported
> > - no support for multi-master and the associated concepts (mastership
> > handover, support for secondary masters, ...)
> > - I2C devices can only be described using DT because this is the only
> > use case I have. However, the framework can easily be extended with
> > ACPI and board info support
> > - I3C slave framework. This has been completely omitted, but shouldn't
> > have a huge impact on the I3C framework because I3C slaves don't see
> > the whole bus, it's only about handling master requests and generating
> > IBIs. Some of the struct, constant and enum definitions could be
> > shared, but most of the I3C slave framework logic will be different
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 24cd47014657..999239dc29d4 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -111,7 +111,7 @@ obj-$(CONFIG_SERIO) += input/serio/
> > obj-$(CONFIG_GAMEPORT) += input/gameport/
> > obj-$(CONFIG_INPUT) += input/
> > obj-$(CONFIG_RTC_LIB) += rtc/
> > -obj-y += i2c/ media/
> > +obj-y += i2c/ i3c/ media/
> > obj-$(CONFIG_PPS) += pps/
> > obj-y += ptp/
> > obj-$(CONFIG_W1) += w1/
> > diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig
> > new file mode 100644
> > index 000000000000..cf3752412ae9
> > --- /dev/null
> > +++ b/drivers/i3c/Kconfig
> > @@ -0,0 +1,24 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +menuconfig I3C
> > + tristate "I3C support"
> > + select I2C
> > + help
> > + I3C is a serial protocol standardized by the MIPI alliance.
> > +
> > + It's supposed to be backward compatible with I2C while providing
> > + support for high speed transfers and native interrupt support
> > + without the need for extra pins.
> > +
> > + The I3C protocol also standardizes the slave device types and is
> > + mainly design to communicate with sensors.
>
> designed
Will fix that.
>
> > +
> > + If you want I3C support, you should say Y here and also to the
> > + specific driver for your bus adapter(s) below.
> > +
> > + This I3C support can also be built as a module. If so, the module
> > + will be called i3c.
> > +
> > diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c
> > new file mode 100644
> > index 000000000000..d6d938a785a9
> > --- /dev/null
> > +++ b/drivers/i3c/core.c
>
> > +static ssize_t bcr_show(struct device *dev,
> > + struct device_attribute *da,
> > + char *buf)
> > +{
> > + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > + ssize_t ret;
> > +
> > + i3c_bus_normaluse_lock(bus);
> > + ret = sprintf(buf, "%x\n", i3cdev->info.bcr);
> > + i3c_bus_normaluse_unlock(bus);
> > +
> > + return ret;
> > +}
> > +static DEVICE_ATTR_RO(bcr);
> > +
> > +static ssize_t dcr_show(struct device *dev,
> > + struct device_attribute *da,
> > + char *buf)
> > +{
> > + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > + ssize_t ret;
> > +
> > + i3c_bus_normaluse_lock(bus);
> > + ret = sprintf(buf, "%x\n", i3cdev->info.dcr);
> > + i3c_bus_normaluse_unlock(bus);
> > +
> > + return ret;
> > +}
> > +static DEVICE_ATTR_RO(dcr);
> > +
> > +static ssize_t pid_show(struct device *dev,
> > + struct device_attribute *da,
> > + char *buf)
> > +{
> > + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > + ssize_t ret;
> > +
> > + i3c_bus_normaluse_lock(bus);
> > + ret = sprintf(buf, "%llx\n", i3cdev->info.pid);
> > + i3c_bus_normaluse_unlock(bus);
> > +
> > + return ret;
> > +}
> > +static DEVICE_ATTR_RO(pid);
> > +
> > +static ssize_t address_show(struct device *dev,
> > + struct device_attribute *da,
> > + char *buf)
> > +{
> > + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > + ssize_t ret;
> > +
> > + i3c_bus_normaluse_lock(bus);
> > + ret = sprintf(buf, "%02x\n", i3cdev->info.dyn_addr);
> > + i3c_bus_normaluse_unlock(bus);
> > +
> > + return ret;
> > +}
> > +static DEVICE_ATTR_RO(address);
>
> should there be separate entries for dynamic and static address?
I didn't think exposing the static address was needed since it's never
used except at initialization time.
>
> If yes, you could also reduce the boilerplate by using a macro taking
> attribute name and format string.
Hm, don't see the need for that yet, even if we expose both static and
dynamic addresses. It's not like you'll save hundreds lines of code by
doing that, we're talking about 10 lines.
>
> > +static int i3c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> > +{
> > + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > + u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > + u16 ext = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > +
> > + if (I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid))
> > + return add_uevent_var(env, "MODALIAS=i3c:dcr%02Xmanuf%04X",
> > + i3cdev->info.dcr, manuf);
> > +
> > + return add_uevent_var(env,
> > + "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04xext%04x",
> > + i3cdev->info.dcr, manuf, part, ext);
>
> instance id should also be passed in the non-random case?
MODALIAS is used by user space to know which module should be loaded
when a device is connected on the bus. Why would we need to know the
instance ID? In comparison, the USB subsystem does not pass the
->iSerialNumber value in MODALIAS.
>
> > +}
>
> > +static const struct i3c_device_id *
> > +i3c_device_match_id(struct i3c_device *i3cdev,
> > + const struct i3c_device_id *id_table)
> > +{
> > + const struct i3c_device_id *id;
> > +
> > + /*
> > + * The lower 32bits of the provisional ID is just filled with a random
> > + * value, try to match using DCR info.
> > + */
> > + if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> > + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > + u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > + u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > +
> > + /* First try to match by manufacturer/part ID. */
> > + for (id = id_table; id->match_flags != 0; id++) {
> > + if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
> > + I3C_MATCH_MANUF_AND_PART)
> > + continue;
> > +
> > + if (manuf != id->manuf_id || part != id->part_id)
> > + continue;
> > +
> > + if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
> > + ext_info != id->extra_info)
> > + continue;
> > +
> > + return id;
>
> Here too, instance id is ignored. Seems like done on purpose?
Yes, it's done on purpose, the instance ID does not impact the
driver selection logic, it's just a way to uniquely identify devices of
the same type on an I3C bus.
>
> > + }
> > + }
> > +
> > + /* Fallback to DCR match. */
> > + for (id = id_table; id->match_flags != 0; id++) {
> > + if ((id->match_flags & I3C_MATCH_DCR) &&
> > + id->dcr == i3cdev->info.dcr)
> > + return id;
> > + }
> > +
> > + return NULL;
> > +}
>
> > +static int i3c_device_probe(struct device *dev)
> > +{
> > + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > + struct i3c_driver *driver = drv_to_i3cdrv(dev->driver);
> > +
> > + return driver->probe(i3cdev);
>
> Should you pm_runtime enable the device before probe? Like done for PCI
> in local_pci_probe() for example.
Hm, I'm not sure, but I'd say that it's the device driver responsibility
to do that.
> Or I guess thats a problem because I2C
> devices don't expect it?
I2C device probing is not handled here, so that shouldn't be a problem.
Still, I think we should wait for a real need before deciding whether
calling pm_runtime enable() from the core is a wise thing to do or not.
>
> > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> > new file mode 100644
> > index 000000000000..8948d9bdec82
> > --- /dev/null
> > +++ b/drivers/i3c/device.c
> > @@ -0,0 +1,294 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017 Cadence Design Systems Inc.
>
> 2018 now :)
Will change all dates.
>
> > + *
> > + * Author: Boris Brezillon <boris.brezillon@bootlin.com>
> > + */
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/bug.h>
> > +#include <linux/completion.h>
> > +#include <linux/device.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +
> > +#include "internals.h"
> > +
> > +/**
> > + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a
> > + * specific device
> > + *
> > + * @dev: device with which the transfers should be done
> > + * @xfers: array of transfers
> > + * @nxfers: number of transfers
> > + *
> > + * Initiate one or several private SDR transfers with @dev.
> > + *
> > + * This function can sleep and thus cannot be called in atomic context.
> > + *
> > + * Return: 0 in case of success, a negative error core otherwise.
> > + */
>
> Curious why you specifically call out SDR here. It could be HDR too, in
> future. Right?
HDR transfers will be handled through a different function (see the
previous version of this patch series where HDR modes were supported).
Regarding the name itself, I just followed the naming used in the I3C
spec, but I fine changing _priv_ by _sdr_ if you prefer.
>
> > +int i3c_device_do_priv_xfers(struct i3c_device *dev,
> > + struct i3c_priv_xfer *xfers,
> > + int nxfers)
> > +{
> > + struct i3c_master_controller *master;
> > + int ret, i;
> > +
> > + if (nxfers < 1)
> > + return 0;
> > +
> > + master = i3c_device_get_master(dev);
> > + if (!master || !xfers)
> > + return -EINVAL;
> > +
> > + if (!master->ops->priv_xfers)
> > + return -ENOTSUPP;
> > +
> > + for (i = 0; i < nxfers; i++) {
> > + if (!xfers[i].len || !xfers[i].data.in)
> > + return -EINVAL;
> > + }
> > +
> > + i3c_bus_normaluse_lock(master->bus);
> > + ret = master->ops->priv_xfers(dev, xfers, nxfers);
> > + i3c_bus_normaluse_unlock(master->bus);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
>
> > +/**
> > + * struct i3c_device_info - I3C device information
> > + * @pid: Provisional ID
> > + * @bcr: Bus Characteristic Register
> > + * @dcr: Device Characteristic Register
> > + * @static_addr: static/I2C address
> > + * @dyn_addr: dynamic address
> > + * @hdr_cap: supported HDR modes
>
> This is just for querying and display device capability. We dont
> actually enter HDR mode at the moment, right?
Right now it is, but is will be used when we'll later add HDR
support (see v3 of this series if you want to have an idea of what
the HDR API looks like) ;-).
>
> > + * @max_read_ds: max read speed information
> > + * @max_write_ds: max write speed information
> > + * @max_ibi_len: max IBI payload length
> > + * @max_read_turnaround: max read turn-around time in micro-seconds
> > + * @max_read_len: max private SDR read length in bytes
> > + * @max_write_len: max private SDR write length in bytes
> > + *
> > + * These are all basic information that should be advertised by an I3C device.
> > + * Some of them are optional depending on the device type and device
> > + * capabilities.
> > + * For each I3C slave attached to a master with
> > + * i3c_master_add_i3c_dev_locked(), the core will send the relevant CCC command
> > + * to retrieve these data.
> > + */
> > +struct i3c_device_info {
> > + u64 pid;
> > + u8 bcr;
> > + u8 dcr;
> > + u8 static_addr;
> > + u8 dyn_addr;
> > + u8 hdr_cap;
> > + u8 max_read_ds;
> > + u8 max_write_ds;
> > + u8 max_ibi_len;
> > + u32 max_read_turnaround;
> > + u16 max_read_len;
> > + u16 max_write_len;
> > +};
>
Thanks for your review.
Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V4] PCI: move early dump functionality from x86 arch into the common code
From: Andy Shevchenko @ 2018-06-20 13:46 UTC (permalink / raw)
To: Sinan Kaya
Cc: linux-pci, Timur Tabi, linux-arm-msm, linux-arm Mailing List,
Jonathan Corbet, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Bjorn Helgaas,
Christoffer Dall, Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Thymo van Beers, Frederic Weisbecker, Greg Kroah-Hartman,
David Rientjes, Philippe Ombredanne, Kate Stewart, Juergen Gross,
Tom Lendacky, Borislav Petkov, Mikulas Patocka, Petr Tesarik,
Andy Lutomirski, Dou Liyang, Ram Pai, Boris Ostrovsky,
open list:DOCUMENTATION, open list
In-Reply-To: <1528164985-14099-1-git-send-email-okaya@codeaurora.org>
On Tue, Jun 5, 2018 at 5:16 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
> Move early dump functionality into common code so that it is available for
> all archtiectures. No need to carry arch specific reads around as the read
> hooks are already initialized by the time pci_setup_device() is getting
> called during scan.
>
It didn't break my setup on x86 at least. Thus,
Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 2 +-
> arch/x86/include/asm/pci-direct.h | 4 ---
> arch/x86/kernel/setup.c | 5 ---
> arch/x86/pci/common.c | 4 ---
> arch/x86/pci/early.c | 44 -------------------------
> drivers/pci/pci.c | 5 +++
> drivers/pci/pci.h | 1 +
> drivers/pci/probe.c | 19 +++++++++++
> 8 files changed, 26 insertions(+), 58 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index e490902..e64f1d8 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2995,7 +2995,7 @@
> See also Documentation/blockdev/paride.txt.
>
> pci=option[,option...] [PCI] various PCI subsystem options:
> - earlydump [X86] dump PCI config space before the kernel
> + earlydump dump PCI config space before the kernel
> changes anything
> off [X86] don't probe for the PCI bus
> bios [X86-32] force use of PCI BIOS, don't access
> diff --git a/arch/x86/include/asm/pci-direct.h b/arch/x86/include/asm/pci-direct.h
> index e1084f7..94597a3 100644
> --- a/arch/x86/include/asm/pci-direct.h
> +++ b/arch/x86/include/asm/pci-direct.h
> @@ -15,8 +15,4 @@ extern void write_pci_config_byte(u8 bus, u8 slot, u8 func, u8 offset, u8 val);
> extern void write_pci_config_16(u8 bus, u8 slot, u8 func, u8 offset, u16 val);
>
> extern int early_pci_allowed(void);
> -
> -extern unsigned int pci_early_dump_regs;
> -extern void early_dump_pci_device(u8 bus, u8 slot, u8 func);
> -extern void early_dump_pci_devices(void);
> #endif /* _ASM_X86_PCI_DIRECT_H */
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 2f86d88..480f250 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -991,11 +991,6 @@ void __init setup_arch(char **cmdline_p)
> setup_clear_cpu_cap(X86_FEATURE_APIC);
> }
>
> -#ifdef CONFIG_PCI
> - if (pci_early_dump_regs)
> - early_dump_pci_devices();
> -#endif
> -
> e820__reserve_setup_data();
> e820__finish_early_params();
>
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 563049c..d4ec117 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -22,7 +22,6 @@
> unsigned int pci_probe = PCI_PROBE_BIOS | PCI_PROBE_CONF1 | PCI_PROBE_CONF2 |
> PCI_PROBE_MMCONF;
>
> -unsigned int pci_early_dump_regs;
> static int pci_bf_sort;
> int pci_routeirq;
> int noioapicquirk;
> @@ -599,9 +598,6 @@ char *__init pcibios_setup(char *str)
> pci_probe |= PCI_BIG_ROOT_WINDOW;
> return NULL;
> #endif
> - } else if (!strcmp(str, "earlydump")) {
> - pci_early_dump_regs = 1;
> - return NULL;
> } else if (!strcmp(str, "routeirq")) {
> pci_routeirq = 1;
> return NULL;
> diff --git a/arch/x86/pci/early.c b/arch/x86/pci/early.c
> index e5f753c..f5fc953 100644
> --- a/arch/x86/pci/early.c
> +++ b/arch/x86/pci/early.c
> @@ -57,47 +57,3 @@ int early_pci_allowed(void)
> PCI_PROBE_CONF1;
> }
>
> -void early_dump_pci_device(u8 bus, u8 slot, u8 func)
> -{
> - u32 value[256 / 4];
> - int i;
> -
> - pr_info("pci 0000:%02x:%02x.%d config space:\n", bus, slot, func);
> -
> - for (i = 0; i < 256; i += 4)
> - value[i / 4] = read_pci_config(bus, slot, func, i);
> -
> - print_hex_dump(KERN_INFO, "", DUMP_PREFIX_OFFSET, 16, 1, value, 256, false);
> -}
> -
> -void early_dump_pci_devices(void)
> -{
> - unsigned bus, slot, func;
> -
> - if (!early_pci_allowed())
> - return;
> -
> - for (bus = 0; bus < 256; bus++) {
> - for (slot = 0; slot < 32; slot++) {
> - for (func = 0; func < 8; func++) {
> - u32 class;
> - u8 type;
> -
> - class = read_pci_config(bus, slot, func,
> - PCI_CLASS_REVISION);
> - if (class == 0xffffffff)
> - continue;
> -
> - early_dump_pci_device(bus, slot, func);
> -
> - if (func == 0) {
> - type = read_pci_config_byte(bus, slot,
> - func,
> - PCI_HEADER_TYPE);
> - if (!(type & 0x80))
> - break;
> - }
> - }
> - }
> - }
> -}
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 97acba7..04052dc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -115,6 +115,9 @@ static bool pcie_ari_disabled;
> /* If set, the PCIe ATS capability will not be used. */
> static bool pcie_ats_disabled;
>
> +/* If set, the PCI config space of each device is printed during boot. */
> +bool pci_early_dump;
> +
> bool pci_ats_disabled(void)
> {
> return pcie_ats_disabled;
> @@ -5805,6 +5808,8 @@ static int __init pci_setup(char *str)
> pcie_ats_disabled = true;
> } else if (!strcmp(str, "noaer")) {
> pci_no_aer();
> + } else if (!strcmp(str, "earlydump")) {
> + pci_early_dump = true;
> } else if (!strncmp(str, "realloc=", 8)) {
> pci_realloc_get_opt(str + 8);
> } else if (!strncmp(str, "realloc", 7)) {
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index c358e7a0..c33265e 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -7,6 +7,7 @@
> #define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */
>
> extern const unsigned char pcie_link_speed[];
> +extern bool pci_early_dump;
>
> bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 56771f3..3678f0a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1545,6 +1545,23 @@ static int pci_intx_mask_broken(struct pci_dev *dev)
> return 0;
> }
>
> +static void early_dump_pci_device(struct pci_dev *pdev)
> +{
> + u32 value[256 / 4];
> + int i;
> +
> + if (!pci_early_dump)
> + return;
> +
> + pci_info(pdev, "config space:\n");
> +
> + for (i = 0; i < 256; i += 4)
> + pci_read_config_dword(pdev, i, &value[i / 4]);
> +
> + print_hex_dump(KERN_INFO, "", DUMP_PREFIX_OFFSET, 16, 1, value,
> + 256, false);
> +}
> +
> /**
> * pci_setup_device - Fill in class and map information of a device
> * @dev: the device structure to fill
> @@ -1594,6 +1611,8 @@ int pci_setup_device(struct pci_dev *dev)
> pci_printk(KERN_DEBUG, dev, "[%04x:%04x] type %02x class %#08x\n",
> dev->vendor, dev->device, dev->hdr_type, dev->class);
>
> + early_dump_pci_device(dev);
> +
> /* Need to have dev->class ready */
> dev->cfg_size = pci_cfg_space_size(dev);
>
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 3/9] cpuset: Simulate auto-off of sched.domain_root at cgroup removal
From: Peter Zijlstra @ 2018-06-20 14:11 UTC (permalink / raw)
To: Waiman Long
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, Mike Galbraith,
torvalds, Roman Gushchin, Juri Lelli, Patrick Bellasi
In-Reply-To: <1529295249-5207-4-git-send-email-longman@redhat.com>
On Mon, Jun 18, 2018 at 12:14:02PM +0800, Waiman Long wrote:
> @@ -1058,7 +1060,12 @@ static int update_reserved_cpumask(struct cpuset *cpuset,
> * Check if any CPUs in addmask or delmask are in the effective_cpus
> * of a sibling cpuset. The implied cpu_exclusive of a scheduling
> * domain root will ensure there are no overlap in cpus_allowed.
> + *
> + * This check is skipped if the cpuset is dying.
Comments that state what the code does are mostly useless; please
explain _why_ if anything.
> */
> + if (dying)
> + goto updated_reserved_cpus;
> +
> rcu_read_lock();
> cpuset_for_each_child(sibling, pos_css, parent) {
> if ((sibling == cpuset) || !(sibling->css.flags & CSS_ONLINE))
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 5/9] cpuset: Make sure that domain roots work properly with CPU hotplug
From: Peter Zijlstra @ 2018-06-20 14:15 UTC (permalink / raw)
To: Waiman Long
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, Mike Galbraith,
torvalds, Roman Gushchin, Juri Lelli, Patrick Bellasi
In-Reply-To: <1529295249-5207-6-git-send-email-longman@redhat.com>
On Mon, Jun 18, 2018 at 12:14:04PM +0800, Waiman Long wrote:
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index 5ee5e77..6ef3516 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1626,6 +1626,13 @@ Cpuset Interface Files
> 2) No CPU that has been distributed to child scheduling domain
> roots is deleted.
>
> + When all the CPUs allocated to a scheduling domain are offlined,
> + that scheduling domain will be temporaily gone and all the
> + tasks in that scheduling domain will migrate to another one that
> + belongs to the parent of the scheduling domain root. When any
> + of those offlined CPUs is onlined again, a new scheduling domain
> + will be re-created and the tasks will be migrated back.
> +
You should mention that this is a destructive operation. If any of the
tasks had an affinity smaller than the original cgroup, that will be
gone.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 6/9] cpuset: Make generate_sched_domains() recognize reserved_cpus
From: Peter Zijlstra @ 2018-06-20 14:17 UTC (permalink / raw)
To: Waiman Long
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, Mike Galbraith,
torvalds, Roman Gushchin, Juri Lelli, Patrick Bellasi
In-Reply-To: <1529295249-5207-8-git-send-email-longman@redhat.com>
On Mon, Jun 18, 2018 at 12:14:06PM +0800, Waiman Long wrote:
> The generate_sched_domains() function is modified to make it work
> correctly with the newly introduced reserved_cpus mask for schedule
> domains generation.
Why isn't this (and the previous) patch part of the patch that
introduces reserved_cpus? It seems weird to have this broken
intermediate state.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 06/10] pci-epf-test/pci_endpoint_test: Cleanup PCI_ENDPOINT_TEST memspace
From: Gustavo Pimentel @ 2018-06-20 14:18 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Gustavo Pimentel, bhelgaas@google.com,
lorenzo.pieralisi@arm.com, Joao.Pinto@synopsys.com,
jingoohan1@gmail.com, adouglas@cadence.com,
jesper.nilsson@axis.com
Cc: linux-pci@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <eea616bd-a5dd-f8bb-22b3-319401c70cc6@ti.com>
Hi,
On 20/06/2018 08:53, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Monday 18 June 2018 08:30 PM, Gustavo Pimentel wrote:
>> Cleanup PCI_ENDPOINT_TEST memspace (by moving the interrupt number away
>> from command section).
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>> Change v2->v3:
>> - New patch file created base on the previous patch
>> "misc: pci_endpoint_test: Add MSI-X support" patch file following
>> Kishon's suggestion.
>> Change v3->v4:
>> - Rebased to Lorenzo's master branch v4.18-rc1.
>>
>> drivers/misc/pci_endpoint_test.c | 84 ++++++++++++++++-----------
>> drivers/pci/endpoint/functions/pci-epf-test.c | 60 ++++++++++++-------
>> 2 files changed, 91 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>> index 7b37046..56be808 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -35,38 +35,42 @@
>>
>> #include <uapi/linux/pcitest.h>
>>
>> -#define DRV_MODULE_NAME "pci-endpoint-test"
>> -
>> -#define PCI_ENDPOINT_TEST_MAGIC 0x0
>> -
>> -#define PCI_ENDPOINT_TEST_COMMAND 0x4
>> -#define COMMAND_RAISE_LEGACY_IRQ BIT(0)
>> -#define COMMAND_RAISE_MSI_IRQ BIT(1)
>> -#define MSI_NUMBER_SHIFT 2
>> -/* 6 bits for MSI number */
>> -#define COMMAND_READ BIT(8)
>> -#define COMMAND_WRITE BIT(9)
>> -#define COMMAND_COPY BIT(10)
>> -
>> -#define PCI_ENDPOINT_TEST_STATUS 0x8
>> -#define STATUS_READ_SUCCESS BIT(0)
>> -#define STATUS_READ_FAIL BIT(1)
>> -#define STATUS_WRITE_SUCCESS BIT(2)
>> -#define STATUS_WRITE_FAIL BIT(3)
>> -#define STATUS_COPY_SUCCESS BIT(4)
>> -#define STATUS_COPY_FAIL BIT(5)
>> -#define STATUS_IRQ_RAISED BIT(6)
>> -#define STATUS_SRC_ADDR_INVALID BIT(7)
>> -#define STATUS_DST_ADDR_INVALID BIT(8)
>> -
>> -#define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0xc
>> +#define DRV_MODULE_NAME "pci-endpoint-test"
>> +
>> +#define IRQ_TYPE_LEGACY 0
>> +#define IRQ_TYPE_MSI 1
>> +
>> +#define PCI_ENDPOINT_TEST_MAGIC 0x0
>> +
>> +#define PCI_ENDPOINT_TEST_COMMAND 0x4
>> +#define COMMAND_RAISE_LEGACY_IRQ BIT(0)
>> +#define COMMAND_RAISE_MSI_IRQ BIT(1)
>
> Maybe you can add a comment here that BIT(2) is reserved for MSIX support
Sure.
>> +#define COMMAND_READ BIT(3)
>> +#define COMMAND_WRITE BIT(4)
>> +#define COMMAND_COPY BIT(5)
>> +
>> +#define PCI_ENDPOINT_TEST_STATUS 0x8
>> +#define STATUS_READ_SUCCESS BIT(0)
>> +#define STATUS_READ_FAIL BIT(1)
>> +#define STATUS_WRITE_SUCCESS BIT(2)
>> +#define STATUS_WRITE_FAIL BIT(3)
>> +#define STATUS_COPY_SUCCESS BIT(4)
>> +#define STATUS_COPY_FAIL BIT(5)
>> +#define STATUS_IRQ_RAISED BIT(6)
>> +#define STATUS_SRC_ADDR_INVALID BIT(7)
>> +#define STATUS_DST_ADDR_INVALID BIT(8)
>> +
>> +#define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0x0c
>> #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10
>>
>> #define PCI_ENDPOINT_TEST_LOWER_DST_ADDR 0x14
>> #define PCI_ENDPOINT_TEST_UPPER_DST_ADDR 0x18
>>
>> -#define PCI_ENDPOINT_TEST_SIZE 0x1c
>> -#define PCI_ENDPOINT_TEST_CHECKSUM 0x20
>> +#define PCI_ENDPOINT_TEST_SIZE 0x1c
>> +#define PCI_ENDPOINT_TEST_CHECKSUM 0x20
>> +
>> +#define PCI_ENDPOINT_TEST_IRQ_TYPE 0x24
>
> Is this not redundant? COMMAND_RAISE_LEGACY_IRQ, COMMAND_RAISE_MSI_IRQ already
> indicates the irq type to be used.
In previous implementation the distinction between interrupt types was simpler,
basically legacy *number* always set as zero and the MSI *number* always
non-zero. However by introducing the MSI-X this simple mechanism is no longer
valid, because the MSI-X *number* is also always non-zero like as MSI.
Therefore is necessary to distinguish which interrupt type was been triggered
(MSI or MSI-X) especially for the Write/Read/Copy tests.
>> +#define PCI_ENDPOINT_TEST_IRQ_NUMBER 0x28
>>
>> static DEFINE_IDA(pci_endpoint_test_ida);
>>
>> @@ -179,6 +183,9 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
>> {
>> u32 val;
>>
>> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE,
>> + IRQ_TYPE_LEGACY);
>> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 0);
>
> Not sure if the above writes are really required.
Is not required, but I added those write for keeping the code coherency between
pci_endpoint_test_legacy_irq() and pci_endpoint_test_msi_irq(), basically both
functions write on the same registers. I think it doesn't cause any harm.
>> pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>> COMMAND_RAISE_LEGACY_IRQ);
>> val = wait_for_completion_timeout(&test->irq_raised,
>> @@ -190,20 +197,22 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
>> }
>>
>> static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
>> - u8 msi_num)
>> + u8 irq_num)
>
> why do you want to rename this?
Initially this patch file contained the changes related to MSI-X support and by
that time I had changed the name of this variable so that its name had a broader
meaning that contemplated these two types. Now that I see this, I think the
original name may also be valid, I will revert this name change.
Regards,
Gustavo
>
> Thanks
> Kishon
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 9/9] cpuset: Allow reporting of sched domain generation info
From: Peter Zijlstra @ 2018-06-20 14:20 UTC (permalink / raw)
To: Waiman Long
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, Mike Galbraith,
torvalds, Roman Gushchin, Juri Lelli, Patrick Bellasi
In-Reply-To: <1529295249-5207-11-git-send-email-longman@redhat.com>
On Mon, Jun 18, 2018 at 12:14:09PM +0800, Waiman Long wrote:
> +#ifdef CONFIG_DEBUG_KERNEL
> +static inline void debug_print_domains(cpumask_var_t *doms, int ndoms)
> +{
> + int i;
> + char buf[200];
> + char *ptr, *end = buf + sizeof(buf) - 1;
> +
> + for (i = 0, ptr = buf, *end = '\0'; i < ndoms; i++)
> + ptr += snprintf(ptr, end - ptr, "dom%d=%*pbl ", i,
> + cpumask_pr_args(doms[i]));
> +
> + pr_debug("Generated %d domains: %s\n", ndoms, buf);
> +}
Why not use pr_cont() and do away with that static buffer?
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 2/9] cpuset: Add new v2 cpuset.sched.domain_root flag
From: Peter Zijlstra @ 2018-06-20 14:27 UTC (permalink / raw)
To: Waiman Long
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, Mike Galbraith,
torvalds, Roman Gushchin, Juri Lelli, Patrick Bellasi
In-Reply-To: <1529295249-5207-3-git-send-email-longman@redhat.com>
On Mon, Jun 18, 2018 at 12:14:01PM +0800, Waiman Long wrote:
> + cpuset.sched.domain_root
Why are we calling this a domain_root and not a partition?
> + A read-write single value file which exists on non-root
> + cpuset-enabled cgroups. It is a binary value flag that accepts
> + either "0" (off) or "1" (on). This flag is set by the parent
> + and is not delegatable.
You still haven't answered:
https://lkml.kernel.org/r/20180531094943.GG12180@hirez.programming.kicks-ass.net
the question stands.
> + If set, it indicates that the current cgroup is the root of a
> + new scheduling domain or partition that comprises itself and
> + all its descendants except those that are scheduling domain
> + roots themselves and their descendants. The root cgroup is
> + always a scheduling domain root.
> +
> + There are constraints on where this flag can be set. It can
> + only be set in a cgroup if all the following conditions are true.
> +
> + 1) The "cpuset.cpus" is not empty and the list of CPUs are
> + exclusive, i.e. they are not shared by any of its siblings.
> + 2) The "cpuset.cpus" is also a proper subset of the parent's
> + "cpuset.cpus.effective".
> + 3) The parent cgroup is a scheduling domain root.
> + 4) There is no child cgroups with cpuset enabled. This is
> + for eliminating corner cases that have to be handled if such
> + a condition is allowed.
> +
> + Setting this flag will take the CPUs away from the effective
> + CPUs of the parent cgroup. Once it is set, this flag cannot be
> + cleared if there are any child cgroups with cpuset enabled.
> +
> + A parent scheduling domain root cgroup cannot distribute
> + all its CPUs to its child scheduling domain root cgroups.
> + There must be at least one cpu left in the parent scheduling
> + domain root cgroup.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox