[PATCH v4 00/11] bus: mhi: host: Add support for mhi bus bw

[PATCH v4 00/11] bus: mhi: host: Add support for mhi bus bw

[Krishna Chaitanya Chundru]

As per MHI spec sec 14, MHI supports bandwidth scaling to reduce power
consumption. MHI bandwidth scaling is advertised in devices that contain
the bandwidth scaling capability registers. If enabled, the device
aggregates bandwidth requirements and sends them to the host in the form
of an event. After the host performs the bandwidth switch, it sends an
acknowledgment by ringing a doorbell.

if the host supports bandwidth scaling events, then it must set
BW_CFG.ENABLED bit, set BW_CFG.DB_CHAN_ID to the channel ID to the
doorbell that will be used by the host to communicate the bandwidth
scaling status and BW_CFG.ER_INDEX to the index for the event ring
to which the device should send bandwidth scaling request in the
bandwidth scaling capability register.

As part of mmio init check if the bw scale capability is present or not,
if present advertise host supports bw scale by setting all the required
fields.

MHI layer will only forward the bw scaling request to the controller
driver, it is responsibility of the controller driver to do actual bw
scaling and then pass status to the MHI. MHI will response back to the
device based up on the status of the bw scale received.

Add a new get_misc_doorbell() to get doorbell for misc capabilities to
use the doorbell with mhi events like MHI BW scale etc.

Use workqueue & mutex for the bw scale events as the pci_set_target_speed()
which will called by the mhi controller driver can sleep.

If the driver want to move higher data rate/speed then the current data
rate/speed then the controller driver may need to change certain votes
so that link may come up in requested data rate/speed like QCOM PCIe
controllers need to change their RPMh (Resource Power Manager-hardened)
state. And also once link retraining is done controller drivers needs
to adjust their votes based on the final data rate/speed.

Some controllers also may need to update their bandwidth voting like
ICC bw votings etc.

So, add pre_link_speed_change() & post_link_speed_change() op to call before & after
the link re-train. There is no explicit locking mechanisms as these are
called by a single client endpoint driver

In case of PCIe switch, if there is a request to change target speed for a
downstream port then no need to call these function ops as these are
outside the scope of the controller drivers.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
Changes in v4:
- Remove leftover type case, change the function names to pre/post_link_speed_change(Ilpo Järvinen)
- Add check's for capability read and convert le32 to cpu (Jeffrey Hugo)
- Add macros for GENMASK & BIT() (Ilpo Järvinen)
- Link to v3: https://lore.kernel.org/r/20250519-mhi_bw_up-v3-0-3acd4a17bbb5@oss.qualcomm.com

Changes in v3:
- Move update speed logic to pwrctrl driver (Mani)
- Move pre_bus_bw & post_bus_bw to bridge as these are bridge driver specific ops,
it feels to me we need to add these in the host bridge driver similar to recently
added one reset_slot.
- Remove dwc level wrapper (Mani)
- Enable ASPM only if they are enabled already (Mani)
- Change the name of mhi_get_capability_offset to mhi_find_capability() (Bjorn)
- Fix comments in the code, subjects etc (Mani & Bjorn)
- Link to v2: https://lore.kernel.org/r/20250313-mhi_bw_up-v2-0-869ca32170bf@oss.qualcomm.com

Changes in v2:
- Update the comments.
- Split the icc bw patch as sepertate one (Bjorn)
- update the aspm disablement comment (Bjorn)
- Use FIELD_GET & FIELD_PREP instead of hard macros and couple of nits
  suggested by (Ilpo Järvinen)
- Create a new function to change lnkcntrl2speed to enum pci_bus_speed (Jeff)
- Link to v1: https://lore.kernel.org/r/20250217-mhi_bw_up-v1-0-9bad1e42bdb1@oss.qualcomm.com

---
Krishna Chaitanya Chundru (9):
      PCI: Update current bus speed as part of pci_pwrctrl_notify()
      PCI/bwctrl: Add support to scale bandwidth before & after link re-training
      bus: mhi: host: Add support for Bandwidth scale
      PCI/ASPM: Return enabled ASPM states as part of pcie_aspm_enabled()
      PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()
      PCI: qcom: Extract core logic from qcom_pcie_icc_opp_update()
      PCI: qcom: Add support for PCIe pre/post_link_speed_change()
      PCI: Export pci_set_target_speed()
      PCI: Add function to convert lnkctl2speed to pci_bus_speed

Miaoqing Pan (1):
      wifi: ath11k: Add support for MHI bandwidth scaling

Vivek Pernamitta (1):
      bus: mhi: host: Add support to read MHI capabilities

 drivers/bus/mhi/common.h               |  26 +++++++
 drivers/bus/mhi/host/init.c            |  97 +++++++++++++++++++++++++-
 drivers/bus/mhi/host/internal.h        |   7 +-
 drivers/bus/mhi/host/main.c            |  98 +++++++++++++++++++++++++-
 drivers/bus/mhi/host/pm.c              |  10 ++-
 drivers/net/wireless/ath/ath11k/mhi.c  |  41 +++++++++++
 drivers/pci/controller/dwc/pcie-qcom.c | 124 ++++++++++++++++++++++++++-------
 drivers/pci/pci.c                      |  12 ++++
 drivers/pci/pcie/aspm.c                |   5 +-
 drivers/pci/pcie/bwctrl.c              |  16 +++++
 drivers/pci/pwrctrl/core.c             |   5 ++
 include/linux/mhi.h                    |  13 ++++
 include/linux/pci.h                    |  23 +++++-
 13 files changed, 442 insertions(+), 35 deletions(-)
---
base-commit: d178209b7c211256ee736ec7c920acb81ddcea48
change-id: 20250217-mhi_bw_up-f31306a5631b

Best regards,
-- 
krishnachaitanya-linux <krichai@qti.qualcomm.com>

[PATCH v4 01/11] PCI: Update current bus speed as part of pci_pwrctrl_notify()

[Krishna Chaitanya Chundru]

If the link is not up till the pwrctl drivers enable power to endpoints
then cur_bus_speed will not be updated with correct speed.

As part of rescan, pci_pwrctrl_notify() will be called when new devices
are added and as part of it update the link bus speed.

Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..4c1d3832c43503a7e434a3fe5e0bf15a148434dc 100644
--- a/drivers/pci/pwrctrl/core.c
+++ b/drivers/pci/pwrctrl/core.c
@@ -10,16 +10,21 @@
 #include <linux/pci-pwrctrl.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include "../pci.h"
 
 static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
 			      void *data)
 {
 	struct pci_pwrctrl *pwrctrl = container_of(nb, struct pci_pwrctrl, nb);
 	struct device *dev = data;
+	struct pci_bus *bus = to_pci_dev(dev)->bus;
 
 	if (dev_fwnode(dev) != dev_fwnode(pwrctrl->dev))
 		return NOTIFY_DONE;
 
+	if (bus->self)
+		pcie_update_link_speed(bus);
+
 	switch (action) {
 	case BUS_NOTIFY_ADD_DEVICE:
 		/*

-- 
2.34.1

[PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Krishna Chaitanya Chundru]

If the driver wants to move to higher data rate/speed than the current data
rate then the controller driver may need to change certain votes so that
link may come up at requested data rate/speed like QCOM PCIe controllers
need to change their RPMh (Resource Power Manager-hardened) state. Once
link retraining is done controller drivers needs to adjust their votes
based on the final data rate.

Some controllers also may need to update their bandwidth voting like
ICC BW votings etc.

So, add pre_link_speed_change() & post_link_speed_change() op to call
before & after the link re-train. There is no explicit locking mechanisms
as these are called by a single client Endpoint driver.

In case of PCIe switch, if there is a request to change target speed for a
downstream port then no need to call these function ops as these are
outside the scope of the controller drivers.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/pcie/bwctrl.c | 15 +++++++++++++++
 include/linux/pci.h       | 18 ++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
index 36f939f23d34e8a3b25a2d1b9059e015f298ca94..dafb8d4f1cfba987e1ff08edfc7caba527f0c76b 100644
--- a/drivers/pci/pcie/bwctrl.c
+++ b/drivers/pci/pcie/bwctrl.c
@@ -140,6 +140,8 @@ static int pcie_bwctrl_change_speed(struct pci_dev *port, u16 target_speed, bool
 int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
 			  bool use_lt)
 {
+	struct pci_host_bridge *host = pci_find_host_bridge(port->bus);
+	bool is_rootbus = pci_is_root_bus(port->bus);
 	struct pci_bus *bus = port->subordinate;
 	u16 target_speed;
 	int ret;
@@ -152,6 +154,16 @@ int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
 
 	target_speed = pcie_bwctrl_select_speed(port, speed_req);
 
+	/*
+	 * The host bridge driver may need to be scaled for targeted speed
+	 * otherwise link might not come up at requested speed.
+	 */
+	if (is_rootbus && host->pre_link_speed_change) {
+		ret = host->pre_link_speed_change(host, port, target_speed);
+		if (ret)
+			return ret;
+	}
+
 	scoped_guard(rwsem_read, &pcie_bwctrl_setspeed_rwsem) {
 		struct pcie_bwctrl_data *data = port->link_bwctrl;
 
@@ -176,6 +188,9 @@ int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
 	    !list_empty(&bus->devices))
 		ret = -EAGAIN;
 
+	if (bus && is_rootbus && host->post_link_speed_change)
+		host->post_link_speed_change(host, port, pci_bus_speed2lnkctl2(bus->cur_bus_speed));
+
 	return ret;
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f39238f8b9ce08df97b384d1c1e89bbe..1740bab514b0a9a61c027463a1fb154843312a22 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -599,6 +599,24 @@ struct pci_host_bridge {
 	void (*release_fn)(struct pci_host_bridge *);
 	int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
 	void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
+	/*
+	 * Callback to the host bridge drivers to update ICC BW votes, clock
+	 * frequencies etc.. for the link re-train to come up in targeted speed.
+	 * These are intended to be called by devices directly attached to the
+	 * Root Port. These are called by a single client Endpoint driver, so
+	 * there is no need for explicit locking mechanisms.
+	 */
+	int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
+				     struct pci_dev *dev, int speed);
+	/*
+	 * Callback to the host bridge drivers to adjust ICC BW votes, clock
+	 * frequencies etc.. to the updated speed after link re-train. These
+	 * are intended to be called by devices directly attached to the
+	 * Root Port. These are called by a single client Endpoint driver,
+	 * so there is no need for explicit locking mechanisms.
+	 */
+	void (*post_link_speed_change)(struct pci_host_bridge *bridge,
+				       struct pci_dev *dev, int speed);
 	void		*release_data;
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */

-- 
2.34.1

[PATCH v4 03/11] bus: mhi: host: Add support to read MHI capabilities

[Krishna Chaitanya Chundru]

From: Vivek Pernamitta <quic_vpernami@quicinc.com>

As per MHI spec v1.2,sec 6.6, MHI has capability registers which are
located after the ERDB array. The location of this group of registers is
indicated by the MISCOFF register. Each capability has a capability ID to
determine which functionality is supported and each capability will point
to the next capability supported.

Add a basic function to read those capabilities offsets.

Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/bus/mhi/common.h    | 13 +++++++++++++
 drivers/bus/mhi/host/init.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
index dda340aaed95a5573a2ec776ca712e11a1ed0b52..58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd 100644
--- a/drivers/bus/mhi/common.h
+++ b/drivers/bus/mhi/common.h
@@ -16,6 +16,7 @@
 #define MHICFG				0x10
 #define CHDBOFF				0x18
 #define ERDBOFF				0x20
+#define MISCOFF				0x24
 #define BHIOFF				0x28
 #define BHIEOFF				0x2c
 #define DEBUGOFF			0x30
@@ -113,6 +114,9 @@
 #define MHISTATUS_MHISTATE_MASK		GENMASK(15, 8)
 #define MHISTATUS_SYSERR_MASK		BIT(2)
 #define MHISTATUS_READY_MASK		BIT(0)
+#define MISC_CAP_MASK			GENMASK(31, 0)
+#define CAP_CAPID_MASK			GENMASK(31, 24)
+#define CAP_NEXT_CAP_MASK		GENMASK(23, 12)
 
 /* Command Ring Element macros */
 /* No operation command */
@@ -204,6 +208,15 @@
 #define MHI_RSCTRE_DATA_DWORD1		cpu_to_le32(FIELD_PREP(GENMASK(23, 16), \
 							       MHI_PKT_TYPE_COALESCING))
 
+enum mhi_capability_type {
+	MHI_CAP_ID_INTX = 0x1,
+	MHI_CAP_ID_TIME_SYNC = 0x2,
+	MHI_CAP_ID_BW_SCALE = 0x3,
+	MHI_CAP_ID_TSC_TIME_SYNC = 0x4,
+	MHI_CAP_ID_MAX_TRB_LEN = 0x5,
+	MHI_CAP_ID_MAX,
+};
+
 enum mhi_pkt_type {
 	MHI_PKT_TYPE_INVALID = 0x0,
 	MHI_PKT_TYPE_NOOP_CMD = 0x1,
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index 13e7a55f54ff45b83b3f18b97e2cdd83d4836fe3..9102ce13a2059f599b46d25ef631f643142642be 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -467,6 +467,40 @@ int mhi_init_dev_ctxt(struct mhi_controller *mhi_cntrl)
 	return ret;
 }
 
+static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32 capability, u32 *offset)
+{
+	u32 val, cur_cap, next_offset;
+	int ret;
+
+	/* Get the first supported capability offset */
+	ret = mhi_read_reg_field(mhi_cntrl, mhi_cntrl->regs, MISCOFF, MISC_CAP_MASK, offset);
+	if (ret)
+		return ret;
+
+	*offset = (__force u32)le32_to_cpu(*offset);
+	do {
+		if (*offset >= mhi_cntrl->reg_len)
+			return -ENXIO;
+
+		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, *offset, &val);
+		if (ret)
+			return ret;
+
+		val = (__force u32)le32_to_cpu(val);
+		cur_cap = FIELD_GET(CAP_CAPID_MASK, val);
+		next_offset = FIELD_GET(CAP_NEXT_CAP_MASK, val);
+		if (cur_cap >= MHI_CAP_ID_MAX)
+			return -ENXIO;
+
+		if (cur_cap == capability)
+			return 0;
+
+		*offset = next_offset;
+	} while (next_offset);
+
+	return -ENXIO;
+}
+
 int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
 {
 	u32 val;

-- 
2.34.1

[PATCH v4 04/11] bus: mhi: host: Add support for Bandwidth scale

[Krishna Chaitanya Chundru]

As per MHI spec v1.2, sec 14, MHI supports bandwidth scaling to reduce
power consumption. MHI bandwidth scaling is advertised by devices that
contain the bandwidth scaling capability registers. If enabled, the device
aggregates bandwidth requirements and sends them to the host through
dedicated mhi event ring. After the host performs the bandwidth switch,
it sends an acknowledgment by ringing a doorbell.

if the host supports bandwidth scaling events, then it must set
BW_CFG.ENABLED bit, set BW_CFG.DB_CHAN_ID to the channel ID to the
doorbell that will be used by the host to communicate the bandwidth
scaling status and BW_CFG.ER_INDEX to the index for the event ring
to which the device should send bandwidth scaling request in the
bandwidth scaling capability register.

As part of mmio init check if the bw scale capability is present or not,
if present advertise host supports bw scale by setting all the required
fields.

MHI layer will only forward the bw scaling request to the controller
driver since MHI doesn't have any idea about transport layer used by
the controller, it is responsibility of the controller driver to do actual
bw scaling and then pass status to the MHI. MHI will response back to the
device based up on the status of the bw scale received.

Add a new get_misc_doorbell() to get doorbell for misc capabilities to
use the doorbell with mhi events like MHI BW scale etc.

Use workqueue & mutex for the bw scale events as the pci_set_target_speed()
which will called by the mhi controller driver can sleep.

Co-developed-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/bus/mhi/common.h        | 13 ++++++
 drivers/bus/mhi/host/init.c     | 63 +++++++++++++++++++++++++-
 drivers/bus/mhi/host/internal.h |  7 ++-
 drivers/bus/mhi/host/main.c     | 98 ++++++++++++++++++++++++++++++++++++++++-
 drivers/bus/mhi/host/pm.c       | 10 ++++-
 include/linux/mhi.h             | 13 ++++++
 6 files changed, 198 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
index 58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd..6e342519d80b7725e9ef5390a3eb2a06ac69ceac 100644
--- a/drivers/bus/mhi/common.h
+++ b/drivers/bus/mhi/common.h
@@ -217,6 +217,19 @@ enum mhi_capability_type {
 	MHI_CAP_ID_MAX,
 };
 
+/* MHI Bandwidth scaling offsets */
+#define MHI_BW_SCALE_CFG_OFFSET		0x4
+#define MHI_BW_SCALE_CAP_ID		(3)
+#define MHI_BW_SCALE_DB_CHAN_ID		GENMASK(31, 25)
+#define MHI_BW_SCALE_ENABLED		BIT(24)
+#define MHI_BW_SCALE_ER_INDEX		GENMASK(23, 19)
+
+#define MHI_TRE_GET_EV_BW_REQ_SEQ(tre)	FIELD_GET(GENMASK(15, 8), (MHI_TRE_GET_DWORD(tre, 0)))
+
+#define MHI_BW_SCALE_RESULT(status, seq)	cpu_to_le32(FIELD_PREP(GENMASK(11, 8), status) | \
+						FIELD_PREP(GENMASK(7, 0), seq))
+#define MHI_BW_SCALE_NACK			0xF
+
 enum mhi_pkt_type {
 	MHI_PKT_TYPE_INVALID = 0x0,
 	MHI_PKT_TYPE_NOOP_CMD = 0x1,
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index 9102ce13a2059f599b46d25ef631f643142642be..26703fea6272de7fd19c6ee76be067f0ff0fd309 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -501,10 +501,55 @@ static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32 capability,
 	return -ENXIO;
 }
 
+static int mhi_get_er_index(struct mhi_controller *mhi_cntrl,
+			    enum mhi_er_data_type type)
+{
+	struct mhi_event *mhi_event = mhi_cntrl->mhi_event;
+	int i;
+
+	/* Find event ring for requested type */
+	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
+		if (mhi_event->data_type == type)
+			return mhi_event->er_index;
+	}
+
+	return -ENOENT;
+}
+
+static int mhi_init_bw_scale(struct mhi_controller *mhi_cntrl,
+			     int bw_scale_db)
+{
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	u32 bw_cfg_offset, val;
+	int ret, er_index;
+
+	ret = mhi_find_capability(mhi_cntrl, MHI_BW_SCALE_CAP_ID, &bw_cfg_offset);
+	if (ret)
+		return ret;
+
+	er_index = mhi_get_er_index(mhi_cntrl, MHI_ER_BW_SCALE);
+	if (er_index < 0)
+		return er_index;
+
+	bw_cfg_offset += MHI_BW_SCALE_CFG_OFFSET;
+
+	/* Advertise host support */
+	val = (__force u32)cpu_to_le32(FIELD_PREP(MHI_BW_SCALE_DB_CHAN_ID, bw_scale_db) |
+				       FIELD_PREP(MHI_BW_SCALE_ER_INDEX, er_index) |
+				       MHI_BW_SCALE_ENABLED);
+
+	mhi_write_reg(mhi_cntrl, mhi_cntrl->regs, bw_cfg_offset, val);
+
+	dev_dbg(dev, "Bandwidth scaling setup complete with event ring: %d\n",
+		er_index);
+
+	return 0;
+}
+
 int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
 {
 	u32 val;
-	int i, ret;
+	int i, ret, doorbell = 0;
 	struct mhi_chan *mhi_chan;
 	struct mhi_event *mhi_event;
 	void __iomem *base = mhi_cntrl->regs;
@@ -638,6 +683,16 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
 		return ret;
 	}
 
+	if (mhi_cntrl->get_misc_doorbell)
+		doorbell = mhi_cntrl->get_misc_doorbell(mhi_cntrl, MHI_ER_BW_SCALE);
+
+	if (doorbell > 0) {
+		ret = mhi_init_bw_scale(mhi_cntrl, doorbell);
+		if (!ret)
+			mhi_cntrl->bw_scale_db = base + val + (8 * doorbell);
+		else
+			dev_warn(dev, "Failed to setup bandwidth scaling: %d\n", ret);
+	}
 	return 0;
 }
 
@@ -783,6 +838,9 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
 		case MHI_ER_CTRL:
 			mhi_event->process_event = mhi_process_ctrl_ev_ring;
 			break;
+		case MHI_ER_BW_SCALE:
+			mhi_event->process_event = mhi_process_bw_scale_ev_ring;
+			break;
 		default:
 			dev_err(dev, "Event Ring type not supported\n");
 			goto error_ev_cfg;
@@ -1017,9 +1075,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 
 		mhi_event->mhi_cntrl = mhi_cntrl;
 		spin_lock_init(&mhi_event->lock);
+		mutex_init(&mhi_event->mutex);
 		if (mhi_event->data_type == MHI_ER_CTRL)
 			tasklet_init(&mhi_event->task, mhi_ctrl_ev_task,
 				     (ulong)mhi_event);
+		else if (mhi_event->data_type == MHI_ER_BW_SCALE)
+			INIT_WORK(&mhi_event->work, mhi_process_ev_work);
 		else
 			tasklet_init(&mhi_event->task, mhi_ev_task,
 				     (ulong)mhi_event);
diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index ce566f7d2e9240c64044407aa4124ad3cdb98003..cf64adaecad2aeec8569da5276ec60dd7e97e5e0 100644
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -248,6 +248,8 @@ struct mhi_event {
 	struct mhi_ring ring;
 	struct db_cfg db_cfg;
 	struct tasklet_struct task;
+	struct work_struct work;
+	struct mutex mutex; /* lock for synchronization */
 	spinlock_t lock;
 	int (*process_event)(struct mhi_controller *mhi_cntrl,
 			     struct mhi_event *mhi_event,
@@ -410,7 +412,8 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
 				struct mhi_event *mhi_event, u32 event_quota);
 int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
 			     struct mhi_event *mhi_event, u32 event_quota);
-
+int mhi_process_bw_scale_ev_ring(struct mhi_controller *mhi_cntrl,
+				 struct mhi_event *mhi_event, u32 event_quota);
 /* ISR handlers */
 irqreturn_t mhi_irq_handler(int irq_number, void *dev);
 irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev);
@@ -426,5 +429,5 @@ void mhi_unmap_single_no_bb(struct mhi_controller *mhi_cntrl,
 			    struct mhi_buf_info *buf_info);
 void mhi_unmap_single_use_bb(struct mhi_controller *mhi_cntrl,
 			     struct mhi_buf_info *buf_info);
-
+void mhi_process_ev_work(struct work_struct *work);
 #endif /* _MHI_INT_H */
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index 9bb0df43ceef1e54e8817422516aab1def6fdc4a..f87e9550b59227947fc7268c5799eea274c66b98 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -472,7 +472,10 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
 		if (mhi_dev)
 			mhi_notify(mhi_dev, MHI_CB_PENDING_DATA);
 	} else {
-		tasklet_schedule(&mhi_event->task);
+		if (mhi_event->data_type == MHI_ER_BW_SCALE)
+			queue_work(mhi_cntrl->hiprio_wq, &mhi_event->work);
+		else
+			tasklet_schedule(&mhi_event->task);
 	}
 
 	return IRQ_HANDLED;
@@ -1049,6 +1052,99 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
 	return count;
 }
 
+int mhi_process_bw_scale_ev_ring(struct mhi_controller *mhi_cntrl,
+				 struct mhi_event *mhi_event, u32 event_quota)
+{
+	struct mhi_event_ctxt *er_ctxt = &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index];
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	struct mhi_ring *ev_ring = &mhi_event->ring;
+	dma_addr_t ptr = le64_to_cpu(er_ctxt->rp);
+	u32 response = MHI_BW_SCALE_NACK;
+	struct mhi_ring_element *dev_rp;
+	struct mhi_link_info link_info;
+	int ret = -EINVAL;
+
+	if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state))) {
+		ret =  -EIO;
+		goto exit_bw_scale;
+	}
+
+	if (!MHI_IN_MISSION_MODE(mhi_cntrl->ee))
+		goto exit_bw_scale;
+
+	if (!is_valid_ring_ptr(ev_ring, ptr)) {
+		dev_err(dev,
+			"Event ring rp points outside of the event ring\n");
+		ret =  -EIO;
+		goto exit_bw_scale;
+	}
+
+	dev_rp = mhi_to_virtual(ev_ring, ptr);
+
+	/* If rp points to base, we need to wrap it around */
+	if (dev_rp == ev_ring->base)
+		dev_rp = ev_ring->base + ev_ring->len;
+	dev_rp--;
+
+	/* Fast forward to currently processed element and recycle er */
+	ev_ring->rp = dev_rp;
+	ev_ring->wp = dev_rp - 1;
+	if (ev_ring->wp < ev_ring->base)
+		ev_ring->wp = ev_ring->base + ev_ring->len - ev_ring->el_size;
+	mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
+
+	if (WARN_ON(MHI_TRE_GET_EV_TYPE(dev_rp) != MHI_PKT_TYPE_BW_REQ_EVENT)) {
+		dev_err(dev, "!BW SCALE REQ event\n");
+		goto exit_bw_scale;
+	}
+
+	link_info.target_link_speed = MHI_TRE_GET_EV_LINKSPEED(dev_rp);
+	link_info.target_link_width = MHI_TRE_GET_EV_LINKWIDTH(dev_rp);
+	link_info.sequence_num = MHI_TRE_GET_EV_BW_REQ_SEQ(dev_rp);
+
+	dev_dbg(dev, "Received BW_REQ with seq:%d link speed:0x%x width:0x%x\n",
+		link_info.sequence_num,
+		link_info.target_link_speed,
+		link_info.target_link_width);
+
+	/* Bring host and device out of suspended states */
+	ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
+	if (ret)
+		goto exit_bw_scale;
+
+	mhi_cntrl->runtime_get(mhi_cntrl);
+
+	ret = mhi_cntrl->bw_scale(mhi_cntrl, &link_info);
+	if (!ret)
+		response = 0;
+
+	response = MHI_BW_SCALE_RESULT(response, link_info.sequence_num);
+
+	write_lock_bh(&mhi_cntrl->pm_lock);
+	mhi_write_reg(mhi_cntrl, mhi_cntrl->bw_scale_db, 0, response);
+	write_unlock_bh(&mhi_cntrl->pm_lock);
+
+	mhi_cntrl->runtime_put(mhi_cntrl);
+	mhi_device_put(mhi_cntrl->mhi_dev);
+
+exit_bw_scale:
+	return ret;
+}
+
+void mhi_process_ev_work(struct work_struct *work)
+{
+	struct mhi_event *mhi_event = container_of(work, struct mhi_event,
+						   work);
+
+	struct mhi_controller *mhi_cntrl = mhi_event->mhi_cntrl;
+
+	if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state)))
+		return;
+
+	guard(mutex)(&mhi_event->mutex);
+	mhi_event->process_event(mhi_cntrl, mhi_event, U32_MAX);
+}
+
 void mhi_ev_task(unsigned long data)
 {
 	struct mhi_event *mhi_event = (struct mhi_event *)data;
diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
index 33d92bf2fc3ed48db5f7fe80e4f0ef9fe2d2f2ab..99ae0cdd5687dbb64906209edb18498242598e77 100644
--- a/drivers/bus/mhi/host/pm.c
+++ b/drivers/bus/mhi/host/pm.c
@@ -523,7 +523,10 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
 		if (mhi_event->offload_ev)
 			continue;
 		disable_irq(mhi_cntrl->irq[mhi_event->irq]);
-		tasklet_kill(&mhi_event->task);
+		if (mhi_event->data_type == MHI_ER_BW_SCALE)
+			cancel_work_sync(&mhi_event->work);
+		else
+			tasklet_kill(&mhi_event->task);
 	}
 
 	/* Release lock and wait for all pending threads to complete */
@@ -686,7 +689,10 @@ static void mhi_pm_sys_error_transition(struct mhi_controller *mhi_cntrl)
 	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
 		if (mhi_event->offload_ev)
 			continue;
-		tasklet_kill(&mhi_event->task);
+		if (mhi_event->data_type == MHI_ER_BW_SCALE)
+			cancel_work_sync(&mhi_event->work);
+		else
+			tasklet_kill(&mhi_event->task);
 	}
 
 	/* Release lock and wait for all pending threads to complete */
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index dd372b0123a6da5107b807ff8fe940c567eb2030..2041bbb2e2396eb2e19e6bc0a5e981aa2917ebdd 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -102,10 +102,12 @@ struct image_info {
  * struct mhi_link_info - BW requirement
  * target_link_speed - Link speed as defined by TLS bits in LinkControl reg
  * target_link_width - Link width as defined by NLW bits in LinkStatus reg
+ * sequence_num - used by device to track bw requests sent to host
  */
 struct mhi_link_info {
 	unsigned int target_link_speed;
 	unsigned int target_link_width;
+	int sequence_num;
 };
 
 /**
@@ -183,10 +185,12 @@ enum mhi_ch_ee_mask {
  * enum mhi_er_data_type - Event ring data types
  * @MHI_ER_DATA: Only client data over this ring
  * @MHI_ER_CTRL: MHI control data and client data
+ * @MHI_ER_BW_SCALE: MHI controller bandwidth scale functionality
  */
 enum mhi_er_data_type {
 	MHI_ER_DATA,
 	MHI_ER_CTRL,
+	MHI_ER_BW_SCALE,
 };
 
 /**
@@ -299,6 +303,7 @@ struct mhi_controller_config {
  * @bhi: Points to base of MHI BHI register space
  * @bhie: Points to base of MHI BHIe register space
  * @wake_db: MHI WAKE doorbell register address
+ * @wake_db: MHI BW_SCALE doorbell register address
  * @iova_start: IOMMU starting address for data (required)
  * @iova_stop: IOMMU stop address for data (required)
  * @fw_image: Firmware image name for normal booting (optional)
@@ -355,6 +360,8 @@ struct mhi_controller_config {
  * @write_reg: Write a MHI register via the physical link (required)
  * @reset: Controller specific reset function (optional)
  * @edl_trigger: CB function to trigger EDL mode (optional)
+ * @get_misc_doobell: function to get doorbell used for MISC feature like BW scale etc (optional)
+ * @bw_scale: CB function for passing BW scale info (optional)
  * @buffer_len: Bounce buffer length
  * @index: Index of the MHI controller instance
  * @bounce_buf: Use of bounce buffer
@@ -376,6 +383,7 @@ struct mhi_controller {
 	void __iomem *bhi;
 	void __iomem *bhie;
 	void __iomem *wake_db;
+	void __iomem *bw_scale_db;
 
 	dma_addr_t iova_start;
 	dma_addr_t iova_stop;
@@ -440,6 +448,11 @@ struct mhi_controller {
 	void (*reset)(struct mhi_controller *mhi_cntrl);
 	int (*edl_trigger)(struct mhi_controller *mhi_cntrl);
 
+	int (*get_misc_doorbell)(struct mhi_controller *mhi_cntrl,
+				 enum mhi_er_data_type type);
+	int (*bw_scale)(struct mhi_controller *mhi_cntrl,
+			struct mhi_link_info *link_info);
+
 	size_t buffer_len;
 	int index;
 	bool bounce_buf;

-- 
2.34.1

[PATCH v4 05/11] PCI/ASPM: Return enabled ASPM states as part of pcie_aspm_enabled()

[Krishna Chaitanya Chundru]

PCIe ASPM states can be also controlled by userspace using exposed sysfs
entries, if drivers wants to disable and enable ASPM at runtime they
need to know enabled states for correctly enabling them back.

So return the enabled aspm states as part of pcie_aspm_enabled().

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/pcie/aspm.c | 4 +++-
 include/linux/pci.h     | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 29fcb0689a918f9cb123691e1680de5a1af2c115..94324fc0d3e650cd3ca2c0bb8c1895ca7e647b9d 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1555,8 +1555,10 @@ module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
  * is deallocated only when the last child of the bridge (i.e., @pdev or a
  * sibling) is removed, and the caller should be holding a reference to
  * @pdev, so this should be safe.
+ *
+ * Return: Enabled ASPM states
  */
-bool pcie_aspm_enabled(struct pci_dev *pdev)
+int pcie_aspm_enabled(struct pci_dev *pdev)
 {
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1740bab514b0a9a61c027463a1fb154843312a22..cb703443290a013c8372333fc8d547d190d9f3b4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1848,7 +1848,7 @@ int pci_enable_link_state(struct pci_dev *pdev, int state);
 int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
 void pcie_no_aspm(void);
 bool pcie_aspm_support_enabled(void);
-bool pcie_aspm_enabled(struct pci_dev *pdev);
+int pcie_aspm_enabled(struct pci_dev *pdev);
 #else
 static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
 { return 0; }
@@ -1860,7 +1860,7 @@ static inline int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
 { return 0; }
 static inline void pcie_no_aspm(void) { }
 static inline bool pcie_aspm_support_enabled(void) { return false; }
-static inline bool pcie_aspm_enabled(struct pci_dev *pdev) { return false; }
+static inline int pcie_aspm_enabled(struct pci_dev *pdev) { return false; }
 #endif
 
 #ifdef CONFIG_HOTPLUG_PCI

-- 
2.34.1

[PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Krishna Chaitanya Chundru]

ASPM states are not being enabled back with pci_enable_link_state() when
they are disabled by pci_disable_link_state(). This is because of the
aspm_disable flag is not getting cleared in pci_enable_link_state(), this
flag is being properly cleared when ASPM is controlled by sysfs.

Clear the aspm_disable flag with the requested ASPM states requested by
pci_enable_link_state().

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/pcie/aspm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 94324fc0d3e650cd3ca2c0bb8c1895ca7e647b9d..0f858ef86111b43328bc7db01e6493ce67178458 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1453,6 +1453,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
 		down_read(&pci_bus_sem);
 	mutex_lock(&aspm_lock);
 	link->aspm_default = pci_calc_aspm_enable_mask(state);
+	link->aspm_disable &= ~state;
 	pcie_config_aspm_link(link, policy_to_aspm_state(link));
 
 	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;

-- 
2.34.1

[PATCH v4 07/11] PCI: qcom: Extract core logic from qcom_pcie_icc_opp_update()

[Krishna Chaitanya Chundru]

Extract core logic from qcom_pcie_icc_opp_update() into
qcom_pcie_set_icc_opp() to use in other parts of the code to avoid
duplications.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 61 +++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index c789e3f856550bcfa1ce09962ba9c086d117de05..02643579707f45fc7279023feb7dbc903e69822d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1294,6 +1294,40 @@ static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
 		pcie->cfg->ops->host_post_init(pcie);
 }
 
+static int qcom_pcie_set_icc_opp(struct qcom_pcie *pcie, int speed, int width)
+{
+	struct dw_pcie *pci = pcie->pci;
+	unsigned long freq_kbps;
+	struct dev_pm_opp *opp;
+	int ret = 0, freq_mbps;
+
+	if (pcie->icc_mem) {
+		ret = icc_set_bw(pcie->icc_mem, 0,
+				 width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
+		if (ret) {
+			dev_err(pci->dev, "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
+				ret);
+		}
+	} else if (pcie->use_pm_opp) {
+		freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
+		if (freq_mbps < 0)
+			return -EINVAL;
+
+		freq_kbps = freq_mbps * KILO;
+		opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
+						 true);
+		if (!IS_ERR(opp)) {
+			ret = dev_pm_opp_set_opp(pci->dev, opp);
+			if (ret)
+				dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
+					freq_kbps * width, ret);
+			dev_pm_opp_put(opp);
+		}
+	}
+
+	return ret;
+}
+
 static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
 	.init		= qcom_pcie_host_init,
 	.deinit		= qcom_pcie_host_deinit,
@@ -1478,9 +1512,6 @@ static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
 {
 	u32 offset, status, width, speed;
 	struct dw_pcie *pci = pcie->pci;
-	unsigned long freq_kbps;
-	struct dev_pm_opp *opp;
-	int ret, freq_mbps;
 
 	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
@@ -1492,29 +1523,7 @@ static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
 	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
 	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
 
-	if (pcie->icc_mem) {
-		ret = icc_set_bw(pcie->icc_mem, 0,
-				 width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
-		if (ret) {
-			dev_err(pci->dev, "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
-				ret);
-		}
-	} else if (pcie->use_pm_opp) {
-		freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
-		if (freq_mbps < 0)
-			return;
-
-		freq_kbps = freq_mbps * KILO;
-		opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
-						 true);
-		if (!IS_ERR(opp)) {
-			ret = dev_pm_opp_set_opp(pci->dev, opp);
-			if (ret)
-				dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
-					freq_kbps * width, ret);
-			dev_pm_opp_put(opp);
-		}
-	}
+	qcom_pcie_set_icc_opp(pcie, speed, width);
 }
 
 static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)

-- 
2.34.1

[PATCH v4 08/11] PCI: qcom: Add support for PCIe pre/post_link_speed_change()

[Krishna Chaitanya Chundru]

QCOM PCIe controllers need to disable ASPM before initiating link
re-train. So as part of pre_link_speed_change() disable ASPM and as
part of post_link_speed_change() enable ASPM back.

As the driver needs to enable the ASPM states that are enabled
by the system, save PCI ASPM states before disabling them and
in post_link_speed_change() use the saved ASPM states to enable
back the ASPM.

Update ICC & OPP votes based on the requested speed so that RPMh votes
get updated based on the speed.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 63 ++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 02643579707f45fc7279023feb7dbc903e69822d..c4aa193bbdcc928603ae50e8d7029b152d62f977 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -276,10 +276,16 @@ struct qcom_pcie {
 	struct dentry *debugfs;
 	bool suspended;
 	bool use_pm_opp;
+	int aspm_state; /* Store ASPM state used in pre & post link speed change */
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
+static void qcom_pcie_post_link_speed_change(struct pci_host_bridge *bridge,
+					     struct pci_dev *pdev, int current_speed);
+static int qcom_pcie_pre_link_speed_change(struct pci_host_bridge *bridge,
+					   struct pci_dev *pdev, int current_speed);
+
 static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
 {
 	gpiod_set_value_cansleep(pcie->reset, 1);
@@ -1263,6 +1269,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_assert_reset;
 	}
 
+	pp->bridge->pre_link_speed_change = qcom_pcie_pre_link_speed_change;
+	pp->bridge->post_link_speed_change = qcom_pcie_post_link_speed_change;
 	return 0;
 
 err_assert_reset:
@@ -1328,6 +1336,61 @@ static int qcom_pcie_set_icc_opp(struct qcom_pcie *pcie, int speed, int width)
 	return ret;
 }
 
+static int qcom_pcie_scale_bw(struct dw_pcie_rp *pp, int speed)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	u32 offset, status, width;
+
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+
+	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
+
+	return qcom_pcie_set_icc_opp(pcie, speed, width);
+}
+
+static void qcom_pcie_post_link_speed_change(struct pci_host_bridge *bridge,
+					     struct pci_dev *pdev, int current_speed)
+{
+	struct dw_pcie_rp *pp = bridge->bus->sysdata;
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	struct pci_dev *child;
+
+	/* Get function 0 of downstream device */
+	list_for_each_entry(child, &pdev->subordinate->devices, bus_list)
+		if (PCI_FUNC(child->devfn) == 0)
+			break;
+
+	pci_enable_link_state_locked(child, pcie->aspm_state);
+
+	qcom_pcie_scale_bw(pp, current_speed);
+}
+
+static int qcom_pcie_pre_link_speed_change(struct pci_host_bridge *bridge,
+					   struct pci_dev *pdev, int target_speed)
+{
+	struct dw_pcie_rp *pp = bridge->bus->sysdata;
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+	struct pci_dev *child;
+
+	/* Get function 0 of downstream device */
+	list_for_each_entry(child, &pdev->subordinate->devices, bus_list)
+		if (PCI_FUNC(child->devfn) == 0)
+			break;
+	/*
+	 * QCOM controllers doesn't support link re-train with ASPM enabled.
+	 * Disable ASPM as part of pre_link_speed_change() and enable them
+	 * back as part of post_link_speed_change().
+	 */
+	pcie->aspm_state = pcie_aspm_enabled(child);
+	pci_disable_link_state_locked(child, PCIE_LINK_STATE_ALL);
+
+	return qcom_pcie_scale_bw(pp, target_speed);
+}
+
 static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
 	.init		= qcom_pcie_host_init,
 	.deinit		= qcom_pcie_host_deinit,

-- 
2.34.1

[PATCH v4 09/11] PCI: Export pci_set_target_speed()

[Krishna Chaitanya Chundru]

Export pci_set_target_speed() so that other kernel drivers can use it
to change the PCIe data rate.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/bwctrl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
index dafb8d4f1cfba987e1ff08edfc7caba527f0c76b..b388e02c37ab2815b562108b24b9e98811053d62 100644
--- a/drivers/pci/pcie/bwctrl.c
+++ b/drivers/pci/pcie/bwctrl.c
@@ -193,6 +193,7 @@ int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(pcie_set_target_speed);
 
 static void pcie_bwnotif_enable(struct pcie_device *srv)
 {

-- 
2.34.1

[PATCH v4 10/11] PCI: Add function to convert lnkctl2speed to pci_bus_speed

[Krishna Chaitanya Chundru]

Add a exported function to convert lnkctl2speed to enum pci_bus_speed,
so that other kernel drivers can use it.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/pci.c   | 12 ++++++++++++
 include/linux/pci.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e9448d55113bdfd2263d8e2f6b3ec802f56b712e..8950e88826e27accfe699e31fba8f4077c26296f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6018,6 +6018,18 @@ int pcie_link_speed_mbps(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL(pcie_link_speed_mbps);
 
+/**
+ * pci_lnkctl2_bus_speed - convert lnkctl2 speed to pci_bus_speed
+ * @speed: LNKCAP2 SLS value
+ *
+ * Return: pci_bus_speed
+ */
+enum pci_bus_speed pci_lnkctl2_bus_speed(int speed)
+{
+	return pcie_link_speed[speed];
+}
+EXPORT_SYMBOL(pci_lnkctl2_bus_speed);
+
 /**
  * pcie_bandwidth_available - determine minimum link settings of a PCIe
  *			      device and its bandwidth limitation
diff --git a/include/linux/pci.h b/include/linux/pci.h
index cb703443290a013c8372333fc8d547d190d9f3b4..900f2aad5caf664eb89babca82e1921b1ca2fe5f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1654,6 +1654,7 @@ int pci_cfg_space_size(struct pci_dev *dev);
 unsigned char pci_bus_max_busnr(struct pci_bus *bus);
 resource_size_t pcibios_window_alignment(struct pci_bus *bus,
 					 unsigned long type);
+enum pci_bus_speed pci_lnkctl2_bus_speed(int speed);
 
 #define PCI_VGA_STATE_CHANGE_BRIDGE (1 << 0)
 #define PCI_VGA_STATE_CHANGE_DECODES (1 << 1)

-- 
2.34.1

[PATCH v4 11/11] wifi: ath11k: Add support for MHI bandwidth scaling

[Krishna Chaitanya Chundru]

From: Miaoqing Pan <quic_miaoqing@quicinc.com>

Add support for MHI bandwidth scaling, which will reduce power consumption
if WLAN operates with lower bandwidth. This feature is only enabled for
QCA6390.

Bandwidth scaling is initiated by the endpoint firmware based upon the
bandwidth requirements, if there is high bandwidth data endpoint requests
for higher data rates or if there is less bandwidth they request for lower
data rates to reduce power. Endpoint initiates this through MHI protocol.

Tested-on: WCN6855 hw2.1 PCI WLAN.HSP.1.1-04546-QCAHSPSWPL_V1_V2_SILICONZ_IOE-1

Signed-off-by: Miaoqing Pan <quic_miaoqing@quicinc.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/net/wireless/ath/ath11k/mhi.c | 41 +++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
index acd76e9392d31192aca6776319ef0829a1c69628..f79507c7a82244f9e9d8a3ae6765df3f9432ae8c 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.c
+++ b/drivers/net/wireless/ath/ath11k/mhi.c
@@ -20,6 +20,7 @@
 #define MHI_TIMEOUT_DEFAULT_MS	20000
 #define RDDM_DUMP_SIZE	0x420000
 #define MHI_CB_INVALID	0xff
+#define MHI_BW_SCALE_CHAN_DB 126
 
 static const struct mhi_channel_config ath11k_mhi_channels_qca6390[] = {
 	{
@@ -73,6 +74,17 @@ static struct mhi_event_config ath11k_mhi_events_qca6390[] = {
 		.client_managed = false,
 		.offload_channel = false,
 	},
+	{
+		.num_elements = 8,
+		.irq_moderation_ms = 0,
+		.irq = 1,
+		.mode = MHI_DB_BRST_DISABLE,
+		.data_type = MHI_ER_BW_SCALE,
+		.priority = 2,
+		.hardware_event = false,
+		.client_managed = false,
+		.offload_channel = false,
+	},
 };
 
 static const struct mhi_controller_config ath11k_mhi_config_qca6390 = {
@@ -313,6 +325,33 @@ static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
 	writel(val, addr);
 }
 
+static int ath11k_mhi_op_get_misc_doorbell(struct mhi_controller *mhi_cntrl,
+					   enum mhi_er_data_type type)
+{
+	if (type == MHI_ER_BW_SCALE)
+		return MHI_BW_SCALE_CHAN_DB;
+
+	return -EINVAL;
+}
+
+static int ath11k_mhi_op_bw_scale(struct mhi_controller *mhi_cntrl,
+				  struct mhi_link_info *link_info)
+{
+	enum pci_bus_speed speed = pci_lnkctl2_bus_speed(link_info->target_link_speed);
+	struct ath11k_base *ab = dev_get_drvdata(mhi_cntrl->cntrl_dev);
+	struct pci_dev *pci_dev = to_pci_dev(ab->dev);
+	struct pci_dev *pdev;
+
+	if (!pci_dev)
+		return -EINVAL;
+
+	pdev = pci_upstream_bridge(pci_dev);
+	if (!pdev)
+		return -ENODEV;
+
+	return pcie_set_target_speed(pdev, speed, true);
+}
+
 static int ath11k_mhi_read_addr_from_dt(struct mhi_controller *mhi_ctrl)
 {
 	struct device_node *np;
@@ -389,6 +428,8 @@ int ath11k_mhi_register(struct ath11k_pci *ab_pci)
 	mhi_ctrl->status_cb = ath11k_mhi_op_status_cb;
 	mhi_ctrl->read_reg = ath11k_mhi_op_read_reg;
 	mhi_ctrl->write_reg = ath11k_mhi_op_write_reg;
+	mhi_ctrl->bw_scale = ath11k_mhi_op_bw_scale;
+	mhi_ctrl->get_misc_doorbell = ath11k_mhi_op_get_misc_doorbell;
 
 	switch (ab->hw_rev) {
 	case ATH11K_HW_QCN9074_HW10:

-- 
2.34.1

Re: [PATCH v4 01/11] PCI: Update current bus speed as part of pci_pwrctrl_notify()

[Manivannan Sadhasivam]

On Mon, Jun 09, 2025 at 04:21:22PM GMT, Krishna Chaitanya Chundru wrote:
> If the link is not up till the pwrctl drivers enable power to endpoints
> then cur_bus_speed will not be updated with correct speed.
> 
> As part of rescan, pci_pwrctrl_notify() will be called when new devices
> are added and as part of it update the link bus speed.
> 
> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/pwrctrl/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..4c1d3832c43503a7e434a3fe5e0bf15a148434dc 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -10,16 +10,21 @@
>  #include <linux/pci-pwrctrl.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
> +#include "../pci.h"
>  
>  static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
>  			      void *data)
>  {
>  	struct pci_pwrctrl *pwrctrl = container_of(nb, struct pci_pwrctrl, nb);
>  	struct device *dev = data;
> +	struct pci_bus *bus = to_pci_dev(dev)->bus;
>  
>  	if (dev_fwnode(dev) != dev_fwnode(pwrctrl->dev))
>  		return NOTIFY_DONE;
>  
> +	if (bus->self)
> +		pcie_update_link_speed(bus);

I'm wondering why this should be in pwrctrl driver and not in pci_device_add()?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Manivannan Sadhasivam]

On Mon, Jun 09, 2025 at 04:21:23PM GMT, Krishna Chaitanya Chundru wrote:
> If the driver wants to move to higher data rate/speed than the current data

s/driver/PCI client driver

> rate then the controller driver may need to change certain votes so that
> link may come up at requested data rate/speed like QCOM PCIe controllers
> need to change their RPMh (Resource Power Manager-hardened) state. Once
> link retraining is done controller drivers needs to adjust their votes
> based on the final data rate.
> 
> Some controllers also may need to update their bandwidth voting like
> ICC BW votings etc.
> 
> So, add pre_link_speed_change() & post_link_speed_change() op to call
> before & after the link re-train. There is no explicit locking mechanisms
> as these are called by a single client Endpoint driver.
> 

What if client drivers of multiple endpoints connected to different RP of the
same Host Bridge call this API? Won't you need locking?

- Mani

> In case of PCIe switch, if there is a request to change target speed for a
> downstream port then no need to call these function ops as these are
> outside the scope of the controller drivers.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/pcie/bwctrl.c | 15 +++++++++++++++
>  include/linux/pci.h       | 18 ++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
> index 36f939f23d34e8a3b25a2d1b9059e015f298ca94..dafb8d4f1cfba987e1ff08edfc7caba527f0c76b 100644
> --- a/drivers/pci/pcie/bwctrl.c
> +++ b/drivers/pci/pcie/bwctrl.c
> @@ -140,6 +140,8 @@ static int pcie_bwctrl_change_speed(struct pci_dev *port, u16 target_speed, bool
>  int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
>  			  bool use_lt)
>  {
> +	struct pci_host_bridge *host = pci_find_host_bridge(port->bus);
> +	bool is_rootbus = pci_is_root_bus(port->bus);
>  	struct pci_bus *bus = port->subordinate;
>  	u16 target_speed;
>  	int ret;
> @@ -152,6 +154,16 @@ int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
>  
>  	target_speed = pcie_bwctrl_select_speed(port, speed_req);
>  
> +	/*
> +	 * The host bridge driver may need to be scaled for targeted speed
> +	 * otherwise link might not come up at requested speed.
> +	 */
> +	if (is_rootbus && host->pre_link_speed_change) {
> +		ret = host->pre_link_speed_change(host, port, target_speed);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	scoped_guard(rwsem_read, &pcie_bwctrl_setspeed_rwsem) {
>  		struct pcie_bwctrl_data *data = port->link_bwctrl;
>  
> @@ -176,6 +188,9 @@ int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
>  	    !list_empty(&bus->devices))
>  		ret = -EAGAIN;
>  
> +	if (bus && is_rootbus && host->post_link_speed_change)
> +		host->post_link_speed_change(host, port, pci_bus_speed2lnkctl2(bus->cur_bus_speed));
> +
>  	return ret;
>  }
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 05e68f35f39238f8b9ce08df97b384d1c1e89bbe..1740bab514b0a9a61c027463a1fb154843312a22 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -599,6 +599,24 @@ struct pci_host_bridge {
>  	void (*release_fn)(struct pci_host_bridge *);
>  	int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>  	void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
> +	/*
> +	 * Callback to the host bridge drivers to update ICC BW votes, clock
> +	 * frequencies etc.. for the link re-train to come up in targeted speed.
> +	 * These are intended to be called by devices directly attached to the
> +	 * Root Port. These are called by a single client Endpoint driver, so
> +	 * there is no need for explicit locking mechanisms.
> +	 */
> +	int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
> +				     struct pci_dev *dev, int speed);
> +	/*
> +	 * Callback to the host bridge drivers to adjust ICC BW votes, clock
> +	 * frequencies etc.. to the updated speed after link re-train. These
> +	 * are intended to be called by devices directly attached to the
> +	 * Root Port. These are called by a single client Endpoint driver,
> +	 * so there is no need for explicit locking mechanisms.
> +	 */
> +	void (*post_link_speed_change)(struct pci_host_bridge *bridge,
> +				       struct pci_dev *dev, int speed);
>  	void		*release_data;
>  	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
>  	unsigned int	no_ext_tags:1;		/* No Extended Tags */
> 
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 03/11] bus: mhi: host: Add support to read MHI capabilities

[Manivannan Sadhasivam]

On Mon, Jun 09, 2025 at 04:21:24PM GMT, Krishna Chaitanya Chundru wrote:
> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
> 
> As per MHI spec v1.2,sec 6.6, MHI has capability registers which are
> located after the ERDB array. The location of this group of registers is
> indicated by the MISCOFF register. Each capability has a capability ID to
> determine which functionality is supported and each capability will point
> to the next capability supported.
> 
> Add a basic function to read those capabilities offsets.
> 
> Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/bus/mhi/common.h    | 13 +++++++++++++
>  drivers/bus/mhi/host/init.c | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
> index dda340aaed95a5573a2ec776ca712e11a1ed0b52..58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd 100644
> --- a/drivers/bus/mhi/common.h
> +++ b/drivers/bus/mhi/common.h
> @@ -16,6 +16,7 @@
>  #define MHICFG				0x10
>  #define CHDBOFF				0x18
>  #define ERDBOFF				0x20
> +#define MISCOFF				0x24
>  #define BHIOFF				0x28
>  #define BHIEOFF				0x2c
>  #define DEBUGOFF			0x30
> @@ -113,6 +114,9 @@
>  #define MHISTATUS_MHISTATE_MASK		GENMASK(15, 8)
>  #define MHISTATUS_SYSERR_MASK		BIT(2)
>  #define MHISTATUS_READY_MASK		BIT(0)
> +#define MISC_CAP_MASK			GENMASK(31, 0)
> +#define CAP_CAPID_MASK			GENMASK(31, 24)
> +#define CAP_NEXT_CAP_MASK		GENMASK(23, 12)
>  
>  /* Command Ring Element macros */
>  /* No operation command */
> @@ -204,6 +208,15 @@
>  #define MHI_RSCTRE_DATA_DWORD1		cpu_to_le32(FIELD_PREP(GENMASK(23, 16), \
>  							       MHI_PKT_TYPE_COALESCING))
>  
> +enum mhi_capability_type {
> +	MHI_CAP_ID_INTX = 0x1,
> +	MHI_CAP_ID_TIME_SYNC = 0x2,
> +	MHI_CAP_ID_BW_SCALE = 0x3,
> +	MHI_CAP_ID_TSC_TIME_SYNC = 0x4,
> +	MHI_CAP_ID_MAX_TRB_LEN = 0x5,
> +	MHI_CAP_ID_MAX,
> +};
> +
>  enum mhi_pkt_type {
>  	MHI_PKT_TYPE_INVALID = 0x0,
>  	MHI_PKT_TYPE_NOOP_CMD = 0x1,
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index 13e7a55f54ff45b83b3f18b97e2cdd83d4836fe3..9102ce13a2059f599b46d25ef631f643142642be 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -467,6 +467,40 @@ int mhi_init_dev_ctxt(struct mhi_controller *mhi_cntrl)
>  	return ret;
>  }
>  
> +static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32 capability, u32 *offset)
> +{
> +	u32 val, cur_cap, next_offset;
> +	int ret;
> +
> +	/* Get the first supported capability offset */
> +	ret = mhi_read_reg_field(mhi_cntrl, mhi_cntrl->regs, MISCOFF, MISC_CAP_MASK, offset);
> +	if (ret)
> +		return ret;
> +
> +	*offset = (__force u32)le32_to_cpu(*offset);

Why do you need __force attribute? What does it suppress? Is it because the
pointer is not le32?

- Mani

> +	do {
> +		if (*offset >= mhi_cntrl->reg_len)
> +			return -ENXIO;
> +
> +		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, *offset, &val);
> +		if (ret)
> +			return ret;
> +
> +		val = (__force u32)le32_to_cpu(val);
> +		cur_cap = FIELD_GET(CAP_CAPID_MASK, val);
> +		next_offset = FIELD_GET(CAP_NEXT_CAP_MASK, val);
> +		if (cur_cap >= MHI_CAP_ID_MAX)
> +			return -ENXIO;
> +
> +		if (cur_cap == capability)
> +			return 0;
> +
> +		*offset = next_offset;
> +	} while (next_offset);
> +
> +	return -ENXIO;
> +}
> +
>  int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
>  {
>  	u32 val;
> 
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 04/11] bus: mhi: host: Add support for Bandwidth scale

[Manivannan Sadhasivam]

On Mon, Jun 09, 2025 at 04:21:25PM GMT, Krishna Chaitanya Chundru wrote:
> As per MHI spec v1.2, sec 14, MHI supports bandwidth scaling to reduce
> power consumption. MHI bandwidth scaling is advertised by devices that
> contain the bandwidth scaling capability registers. If enabled, the device
> aggregates bandwidth requirements and sends them to the host through
> dedicated mhi event ring. After the host performs the bandwidth switch,
> it sends an acknowledgment by ringing a doorbell.
> 
> if the host supports bandwidth scaling events, then it must set
> BW_CFG.ENABLED bit, set BW_CFG.DB_CHAN_ID to the channel ID to the
> doorbell that will be used by the host to communicate the bandwidth
> scaling status and BW_CFG.ER_INDEX to the index for the event ring
> to which the device should send bandwidth scaling request in the
> bandwidth scaling capability register.
> 
> As part of mmio init check if the bw scale capability is present or not,
> if present advertise host supports bw scale by setting all the required
> fields.
> 
> MHI layer will only forward the bw scaling request to the controller
> driver since MHI doesn't have any idea about transport layer used by
> the controller, it is responsibility of the controller driver to do actual
> bw scaling and then pass status to the MHI. MHI will response back to the
> device based up on the status of the bw scale received.
> 
> Add a new get_misc_doorbell() to get doorbell for misc capabilities to
> use the doorbell with mhi events like MHI BW scale etc.
> 
> Use workqueue & mutex for the bw scale events as the pci_set_target_speed()
> which will called by the mhi controller driver can sleep.
> 
> Co-developed-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/bus/mhi/common.h        | 13 ++++++
>  drivers/bus/mhi/host/init.c     | 63 +++++++++++++++++++++++++-
>  drivers/bus/mhi/host/internal.h |  7 ++-
>  drivers/bus/mhi/host/main.c     | 98 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/bus/mhi/host/pm.c       | 10 ++++-
>  include/linux/mhi.h             | 13 ++++++
>  6 files changed, 198 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
> index 58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd..6e342519d80b7725e9ef5390a3eb2a06ac69ceac 100644
> --- a/drivers/bus/mhi/common.h
> +++ b/drivers/bus/mhi/common.h
> @@ -217,6 +217,19 @@ enum mhi_capability_type {
>  	MHI_CAP_ID_MAX,
>  };
>  
> +/* MHI Bandwidth scaling offsets */
> +#define MHI_BW_SCALE_CFG_OFFSET		0x4
> +#define MHI_BW_SCALE_CAP_ID		(3)
> +#define MHI_BW_SCALE_DB_CHAN_ID		GENMASK(31, 25)
> +#define MHI_BW_SCALE_ENABLED		BIT(24)
> +#define MHI_BW_SCALE_ER_INDEX		GENMASK(23, 19)
> +
> +#define MHI_TRE_GET_EV_BW_REQ_SEQ(tre)	FIELD_GET(GENMASK(15, 8), (MHI_TRE_GET_DWORD(tre, 0)))
> +
> +#define MHI_BW_SCALE_RESULT(status, seq)	cpu_to_le32(FIELD_PREP(GENMASK(11, 8), status) | \
> +						FIELD_PREP(GENMASK(7, 0), seq))
> +#define MHI_BW_SCALE_NACK			0xF
> +
>  enum mhi_pkt_type {
>  	MHI_PKT_TYPE_INVALID = 0x0,
>  	MHI_PKT_TYPE_NOOP_CMD = 0x1,
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index 9102ce13a2059f599b46d25ef631f643142642be..26703fea6272de7fd19c6ee76be067f0ff0fd309 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -501,10 +501,55 @@ static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32 capability,
>  	return -ENXIO;
>  }
>  
> +static int mhi_get_er_index(struct mhi_controller *mhi_cntrl,
> +			    enum mhi_er_data_type type)
> +{
> +	struct mhi_event *mhi_event = mhi_cntrl->mhi_event;
> +	int i;
> +
> +	/* Find event ring for requested type */
> +	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
> +		if (mhi_event->data_type == type)
> +			return mhi_event->er_index;
> +	}
> +
> +	return -ENOENT;
> +}
> +
> +static int mhi_init_bw_scale(struct mhi_controller *mhi_cntrl,
> +			     int bw_scale_db)
> +{
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +	u32 bw_cfg_offset, val;
> +	int ret, er_index;
> +
> +	ret = mhi_find_capability(mhi_cntrl, MHI_BW_SCALE_CAP_ID, &bw_cfg_offset);
> +	if (ret)
> +		return ret;
> +
> +	er_index = mhi_get_er_index(mhi_cntrl, MHI_ER_BW_SCALE);
> +	if (er_index < 0)
> +		return er_index;
> +
> +	bw_cfg_offset += MHI_BW_SCALE_CFG_OFFSET;
> +
> +	/* Advertise host support */
> +	val = (__force u32)cpu_to_le32(FIELD_PREP(MHI_BW_SCALE_DB_CHAN_ID, bw_scale_db) |
> +				       FIELD_PREP(MHI_BW_SCALE_ER_INDEX, er_index) |
> +				       MHI_BW_SCALE_ENABLED);
> +

It is wrong to store the value of cpu_to_le32() in a non-le32 variable.
mhi_write_reg() accepts the 'val' in native endian. writel used in the
controller drivers should take care of converting to LE before writing to the
device.

> +	mhi_write_reg(mhi_cntrl, mhi_cntrl->regs, bw_cfg_offset, val);
> +
> +	dev_dbg(dev, "Bandwidth scaling setup complete with event ring: %d\n",
> +		er_index);
> +
> +	return 0;
> +}
> +
>  int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
>  {
>  	u32 val;
> -	int i, ret;
> +	int i, ret, doorbell = 0;
>  	struct mhi_chan *mhi_chan;
>  	struct mhi_event *mhi_event;
>  	void __iomem *base = mhi_cntrl->regs;
> @@ -638,6 +683,16 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
>  		return ret;
>  	}
>  
> +	if (mhi_cntrl->get_misc_doorbell)
> +		doorbell = mhi_cntrl->get_misc_doorbell(mhi_cntrl, MHI_ER_BW_SCALE);
> +
> +	if (doorbell > 0) {
> +		ret = mhi_init_bw_scale(mhi_cntrl, doorbell);
> +		if (!ret)
> +			mhi_cntrl->bw_scale_db = base + val + (8 * doorbell);
> +		else
> +			dev_warn(dev, "Failed to setup bandwidth scaling: %d\n", ret);

That's it? And you would continue to setup doorbell setup later?

> +	}

nit: newline

>  	return 0;
>  }
>  
> @@ -783,6 +838,9 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
>  		case MHI_ER_CTRL:
>  			mhi_event->process_event = mhi_process_ctrl_ev_ring;
>  			break;
> +		case MHI_ER_BW_SCALE:
> +			mhi_event->process_event = mhi_process_bw_scale_ev_ring;
> +			break;
>  		default:
>  			dev_err(dev, "Event Ring type not supported\n");
>  			goto error_ev_cfg;
> @@ -1017,9 +1075,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  
>  		mhi_event->mhi_cntrl = mhi_cntrl;
>  		spin_lock_init(&mhi_event->lock);
> +		mutex_init(&mhi_event->mutex);
>  		if (mhi_event->data_type == MHI_ER_CTRL)
>  			tasklet_init(&mhi_event->task, mhi_ctrl_ev_task,
>  				     (ulong)mhi_event);
> +		else if (mhi_event->data_type == MHI_ER_BW_SCALE)
> +			INIT_WORK(&mhi_event->work, mhi_process_ev_work);
>  		else
>  			tasklet_init(&mhi_event->task, mhi_ev_task,
>  				     (ulong)mhi_event);
> diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
> index ce566f7d2e9240c64044407aa4124ad3cdb98003..cf64adaecad2aeec8569da5276ec60dd7e97e5e0 100644
> --- a/drivers/bus/mhi/host/internal.h
> +++ b/drivers/bus/mhi/host/internal.h
> @@ -248,6 +248,8 @@ struct mhi_event {
>  	struct mhi_ring ring;
>  	struct db_cfg db_cfg;
>  	struct tasklet_struct task;
> +	struct work_struct work;
> +	struct mutex mutex; /* lock for synchronization */

synchronization of what?

>  	spinlock_t lock;
>  	int (*process_event)(struct mhi_controller *mhi_cntrl,
>  			     struct mhi_event *mhi_event,
> @@ -410,7 +412,8 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
>  				struct mhi_event *mhi_event, u32 event_quota);
>  int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
>  			     struct mhi_event *mhi_event, u32 event_quota);
> -
> +int mhi_process_bw_scale_ev_ring(struct mhi_controller *mhi_cntrl,
> +				 struct mhi_event *mhi_event, u32 event_quota);
>  /* ISR handlers */
>  irqreturn_t mhi_irq_handler(int irq_number, void *dev);
>  irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev);
> @@ -426,5 +429,5 @@ void mhi_unmap_single_no_bb(struct mhi_controller *mhi_cntrl,
>  			    struct mhi_buf_info *buf_info);
>  void mhi_unmap_single_use_bb(struct mhi_controller *mhi_cntrl,
>  			     struct mhi_buf_info *buf_info);
> -
> +void mhi_process_ev_work(struct work_struct *work);
>  #endif /* _MHI_INT_H */
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index 9bb0df43ceef1e54e8817422516aab1def6fdc4a..f87e9550b59227947fc7268c5799eea274c66b98 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -472,7 +472,10 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>  		if (mhi_dev)
>  			mhi_notify(mhi_dev, MHI_CB_PENDING_DATA);
>  	} else {
> -		tasklet_schedule(&mhi_event->task);
> +		if (mhi_event->data_type == MHI_ER_BW_SCALE)
> +			queue_work(mhi_cntrl->hiprio_wq, &mhi_event->work);
> +		else
> +			tasklet_schedule(&mhi_event->task);
>  	}
>  
>  	return IRQ_HANDLED;
> @@ -1049,6 +1052,99 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
>  	return count;
>  }
>  
> +int mhi_process_bw_scale_ev_ring(struct mhi_controller *mhi_cntrl,
> +				 struct mhi_event *mhi_event, u32 event_quota)
> +{
> +	struct mhi_event_ctxt *er_ctxt = &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index];
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +	struct mhi_ring *ev_ring = &mhi_event->ring;
> +	dma_addr_t ptr = le64_to_cpu(er_ctxt->rp);
> +	u32 response = MHI_BW_SCALE_NACK;
> +	struct mhi_ring_element *dev_rp;
> +	struct mhi_link_info link_info;
> +	int ret = -EINVAL;
> +
> +	if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state))) {
> +		ret =  -EIO;
> +		goto exit_bw_scale;
> +	}
> +
> +	if (!MHI_IN_MISSION_MODE(mhi_cntrl->ee))
> +		goto exit_bw_scale;
> +
> +	if (!is_valid_ring_ptr(ev_ring, ptr)) {
> +		dev_err(dev,
> +			"Event ring rp points outside of the event ring\n");
> +		ret =  -EIO;
> +		goto exit_bw_scale;
> +	}
> +
> +	dev_rp = mhi_to_virtual(ev_ring, ptr);
> +
> +	/* If rp points to base, we need to wrap it around */
> +	if (dev_rp == ev_ring->base)
> +		dev_rp = ev_ring->base + ev_ring->len;
> +	dev_rp--;
> +
> +	/* Fast forward to currently processed element and recycle er */
> +	ev_ring->rp = dev_rp;
> +	ev_ring->wp = dev_rp - 1;
> +	if (ev_ring->wp < ev_ring->base)
> +		ev_ring->wp = ev_ring->base + ev_ring->len - ev_ring->el_size;
> +	mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
> +
> +	if (WARN_ON(MHI_TRE_GET_EV_TYPE(dev_rp) != MHI_PKT_TYPE_BW_REQ_EVENT)) {
> +		dev_err(dev, "!BW SCALE REQ event\n");

What does it mean?

> +		goto exit_bw_scale;
> +	}
> +
> +	link_info.target_link_speed = MHI_TRE_GET_EV_LINKSPEED(dev_rp);
> +	link_info.target_link_width = MHI_TRE_GET_EV_LINKWIDTH(dev_rp);
> +	link_info.sequence_num = MHI_TRE_GET_EV_BW_REQ_SEQ(dev_rp);
> +
> +	dev_dbg(dev, "Received BW_REQ with seq:%d link speed:0x%x width:0x%x\n",
> +		link_info.sequence_num,
> +		link_info.target_link_speed,
> +		link_info.target_link_width);
> +
> +	/* Bring host and device out of suspended states */
> +	ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);

Looks like mhi_device_get_sync() is going runtime_get()/runtime_put() inside
mhi_trigger_resume(). I'm wondering why that is necessary. 

> +	if (ret)
> +		goto exit_bw_scale;
> +
> +	mhi_cntrl->runtime_get(mhi_cntrl);
> +

Here, you want to resume the controller, which is fine.

> +	ret = mhi_cntrl->bw_scale(mhi_cntrl, &link_info);
> +	if (!ret)
> +		response = 0;
> +
> +	response = MHI_BW_SCALE_RESULT(response, link_info.sequence_num);
> +
> +	write_lock_bh(&mhi_cntrl->pm_lock);
> +	mhi_write_reg(mhi_cntrl, mhi_cntrl->bw_scale_db, 0, response);
> +	write_unlock_bh(&mhi_cntrl->pm_lock);
> +
> +	mhi_cntrl->runtime_put(mhi_cntrl);
> +	mhi_device_put(mhi_cntrl->mhi_dev);
> +
> +exit_bw_scale:
> +	return ret;

Just return 'ret' directly.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Manivannan Sadhasivam]

On Mon, Jun 09, 2025 at 04:21:27PM GMT, Krishna Chaitanya Chundru wrote:
> ASPM states are not being enabled back with pci_enable_link_state() when
> they are disabled by pci_disable_link_state(). This is because of the
> aspm_disable flag is not getting cleared in pci_enable_link_state(), this
> flag is being properly cleared when ASPM is controlled by sysfs.
> 

A comment in pcie_config_aspm_link() says:

 /* Enable only the states that were not explicitly disabled */

But the function is called from both aspm_attr_store_common() and
__pci_enable_link_state(). So I don't know if this is behavior is intentional
or wrong. 

> Clear the aspm_disable flag with the requested ASPM states requested by
> pci_enable_link_state().
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

Fixes tag?

- Mani

> ---
>  drivers/pci/pcie/aspm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 94324fc0d3e650cd3ca2c0bb8c1895ca7e647b9d..0f858ef86111b43328bc7db01e6493ce67178458 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1453,6 +1453,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
>  		down_read(&pci_bus_sem);
>  	mutex_lock(&aspm_lock);
>  	link->aspm_default = pci_calc_aspm_enable_mask(state);
> +	link->aspm_disable &= ~state;
>  	pcie_config_aspm_link(link, policy_to_aspm_state(link));
>  
>  	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> 
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 08/11] PCI: qcom: Add support for PCIe pre/post_link_speed_change()

[Manivannan Sadhasivam]

On Mon, Jun 09, 2025 at 04:21:29PM GMT, Krishna Chaitanya Chundru wrote:
> QCOM PCIe controllers need to disable ASPM before initiating link
> re-train. So as part of pre_link_speed_change() disable ASPM and as
> part of post_link_speed_change() enable ASPM back.
> 
> As the driver needs to enable the ASPM states that are enabled
> by the system, save PCI ASPM states before disabling them and
> in post_link_speed_change() use the saved ASPM states to enable
> back the ASPM.
> 
> Update ICC & OPP votes based on the requested speed so that RPMh votes
> get updated based on the speed.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 63 ++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 02643579707f45fc7279023feb7dbc903e69822d..c4aa193bbdcc928603ae50e8d7029b152d62f977 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -276,10 +276,16 @@ struct qcom_pcie {
>  	struct dentry *debugfs;
>  	bool suspended;
>  	bool use_pm_opp;
> +	int aspm_state; /* Store ASPM state used in pre & post link speed change */
>  };
>  
>  #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
>  
> +static void qcom_pcie_post_link_speed_change(struct pci_host_bridge *bridge,
> +					     struct pci_dev *pdev, int current_speed);
> +static int qcom_pcie_pre_link_speed_change(struct pci_host_bridge *bridge,
> +					   struct pci_dev *pdev, int current_speed);
> +
>  static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
>  {
>  	gpiod_set_value_cansleep(pcie->reset, 1);
> @@ -1263,6 +1269,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>  			goto err_assert_reset;
>  	}
>  
> +	pp->bridge->pre_link_speed_change = qcom_pcie_pre_link_speed_change;
> +	pp->bridge->post_link_speed_change = qcom_pcie_post_link_speed_change;
>  	return 0;
>  
>  err_assert_reset:
> @@ -1328,6 +1336,61 @@ static int qcom_pcie_set_icc_opp(struct qcom_pcie *pcie, int speed, int width)
>  	return ret;
>  }
>  
> +static int qcom_pcie_scale_bw(struct dw_pcie_rp *pp, int speed)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +	u32 offset, status, width;
> +
> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> +
> +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
> +
> +	return qcom_pcie_set_icc_opp(pcie, speed, width);
> +}
> +
> +static void qcom_pcie_post_link_speed_change(struct pci_host_bridge *bridge,
> +					     struct pci_dev *pdev, int current_speed)

pdev is the 'port' isn't it?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 10/11] PCI: Add function to convert lnkctl2speed to pci_bus_speed

[Manivannan Sadhasivam]

On Mon, Jun 09, 2025 at 04:21:31PM GMT, Krishna Chaitanya Chundru wrote:
> Add a exported function to convert lnkctl2speed to enum pci_bus_speed,
> so that other kernel drivers can use it.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/pci.c   | 12 ++++++++++++
>  include/linux/pci.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e9448d55113bdfd2263d8e2f6b3ec802f56b712e..8950e88826e27accfe699e31fba8f4077c26296f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6018,6 +6018,18 @@ int pcie_link_speed_mbps(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL(pcie_link_speed_mbps);
>  
> +/**
> + * pci_lnkctl2_bus_speed - convert lnkctl2 speed to pci_bus_speed

nit: pci_linkctrl2_to_bus_speed()?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 11/11] wifi: ath11k: Add support for MHI bandwidth scaling

[Manivannan Sadhasivam]

On Mon, Jun 09, 2025 at 04:21:32PM GMT, Krishna Chaitanya Chundru wrote:
> From: Miaoqing Pan <quic_miaoqing@quicinc.com>
> 
> Add support for MHI bandwidth scaling, which will reduce power consumption
> if WLAN operates with lower bandwidth. This feature is only enabled for
> QCA6390.
> 
> Bandwidth scaling is initiated by the endpoint firmware based upon the
> bandwidth requirements, if there is high bandwidth data endpoint requests
> for higher data rates or if there is less bandwidth they request for lower
> data rates to reduce power. Endpoint initiates this through MHI protocol.
> 
> Tested-on: WCN6855 hw2.1 PCI WLAN.HSP.1.1-04546-QCAHSPSWPL_V1_V2_SILICONZ_IOE-1
> 
> Signed-off-by: Miaoqing Pan <quic_miaoqing@quicinc.com>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/net/wireless/ath/ath11k/mhi.c | 41 +++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
> index acd76e9392d31192aca6776319ef0829a1c69628..f79507c7a82244f9e9d8a3ae6765df3f9432ae8c 100644
> --- a/drivers/net/wireless/ath/ath11k/mhi.c
> +++ b/drivers/net/wireless/ath/ath11k/mhi.c
> @@ -20,6 +20,7 @@
>  #define MHI_TIMEOUT_DEFAULT_MS	20000
>  #define RDDM_DUMP_SIZE	0x420000
>  #define MHI_CB_INVALID	0xff
> +#define MHI_BW_SCALE_CHAN_DB 126
>  
>  static const struct mhi_channel_config ath11k_mhi_channels_qca6390[] = {
>  	{
> @@ -73,6 +74,17 @@ static struct mhi_event_config ath11k_mhi_events_qca6390[] = {
>  		.client_managed = false,
>  		.offload_channel = false,
>  	},
> +	{
> +		.num_elements = 8,
> +		.irq_moderation_ms = 0,
> +		.irq = 1,
> +		.mode = MHI_DB_BRST_DISABLE,
> +		.data_type = MHI_ER_BW_SCALE,
> +		.priority = 2,
> +		.hardware_event = false,
> +		.client_managed = false,
> +		.offload_channel = false,
> +	},
>  };
>  
>  static const struct mhi_controller_config ath11k_mhi_config_qca6390 = {
> @@ -313,6 +325,33 @@ static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
>  	writel(val, addr);
>  }
>  
> +static int ath11k_mhi_op_get_misc_doorbell(struct mhi_controller *mhi_cntrl,
> +					   enum mhi_er_data_type type)
> +{
> +	if (type == MHI_ER_BW_SCALE)
> +		return MHI_BW_SCALE_CHAN_DB;
> +
> +	return -EINVAL;

-EOPNOTSUPP?

> +}
> +
> +static int ath11k_mhi_op_bw_scale(struct mhi_controller *mhi_cntrl,
> +				  struct mhi_link_info *link_info)
> +{
> +	enum pci_bus_speed speed = pci_lnkctl2_bus_speed(link_info->target_link_speed);
> +	struct ath11k_base *ab = dev_get_drvdata(mhi_cntrl->cntrl_dev);
> +	struct pci_dev *pci_dev = to_pci_dev(ab->dev);
> +	struct pci_dev *pdev;
> +
> +	if (!pci_dev)
> +		return -EINVAL;

Is it really possible?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Ilpo Järvinen]

On Tue, 8 Jul 2025, Manivannan Sadhasivam wrote:

> On Mon, Jun 09, 2025 at 04:21:27PM GMT, Krishna Chaitanya Chundru wrote:
> > ASPM states are not being enabled back with pci_enable_link_state() when
> > they are disabled by pci_disable_link_state(). This is because of the
> > aspm_disable flag is not getting cleared in pci_enable_link_state(), this
> > flag is being properly cleared when ASPM is controlled by sysfs.
> > 
> 
> A comment in pcie_config_aspm_link() says:
> 
>  /* Enable only the states that were not explicitly disabled */
> 
> But the function is called from both aspm_attr_store_common() and
> __pci_enable_link_state(). So I don't know if this is behavior is intentional
> or wrong. 

Hi,

I think it's intentional. Whether the behavior is useful is another good 
question but the current behavior aligns with the explanation in the 
comment.

My understanding of the situation is:

pci_disable_link_state() and pci_enable_link_state() are not symmetric 
despite the names, never have been (this is one of those many quirks ASPM 
driver has which should be eventually cleaned up, IMO).

It might be appropriate to rename pci_enable_link_state() to 
pci_set_default_link_state() to match the name to its functionality (and 
the function comment):

 * pci_enable_link_state - Clear and set the default device link state

Note: "the default ... link state".


I've already raised this concern earlier! As you see, my comment are 
not getting addressed. I'd like to see the author does one of these:

1) Renames pci_enable_link_state() to pci_set_default_link_state()

1b) If pci_enable_link_state() is still needed after that, a new function
is added to symmetrically pair with pci_disable_link_state().

or alternatively,

2) Changelog justifies very clearly why this change is okay with the 
existing callers. (And obviously the function comment should be altered to 
match the functionality in that case too).

If approach 2 is chosen, it should be very carefully reviewed when it 
comes to the callers.


> > Clear the aspm_disable flag with the requested ASPM states requested by
> > pci_enable_link_state().
> > 
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> 
> Fixes tag?
> 
> - Mani
> 
> > ---
> >  drivers/pci/pcie/aspm.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 94324fc0d3e650cd3ca2c0bb8c1895ca7e647b9d..0f858ef86111b43328bc7db01e6493ce67178458 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1453,6 +1453,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
> >  		down_read(&pci_bus_sem);
> >  	mutex_lock(&aspm_lock);
> >  	link->aspm_default = pci_calc_aspm_enable_mask(state);
> > +	link->aspm_disable &= ~state;
> >  	pcie_config_aspm_link(link, policy_to_aspm_state(link));
> >  
> >  	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> > 
> > -- 
> > 2.34.1
> > 
> 
> 

-- 
 i.

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Krishna Chaitanya Chundru]

On 7/8/2025 9:55 PM, Manivannan Sadhasivam wrote:
> On Mon, Jun 09, 2025 at 04:21:23PM GMT, Krishna Chaitanya Chundru wrote:
>> If the driver wants to move to higher data rate/speed than the current data
> 
> s/driver/PCI client driver
> 
>> rate then the controller driver may need to change certain votes so that
>> link may come up at requested data rate/speed like QCOM PCIe controllers
>> need to change their RPMh (Resource Power Manager-hardened) state. Once
>> link retraining is done controller drivers needs to adjust their votes
>> based on the final data rate.
>>
>> Some controllers also may need to update their bandwidth voting like
>> ICC BW votings etc.
>>
>> So, add pre_link_speed_change() & post_link_speed_change() op to call
>> before & after the link re-train. There is no explicit locking mechanisms
>> as these are called by a single client Endpoint driver.
>>
> 
> What if client drivers of multiple endpoints connected to different RP of the
> same Host Bridge call this API? Won't you need locking?
> 
Yeah you are right, I will add locking in next patch.

- Krishna Chaitanya.
> - Mani
> 
>> In case of PCIe switch, if there is a request to change target speed for a
>> downstream port then no need to call these function ops as these are
>> outside the scope of the controller drivers.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>>   drivers/pci/pcie/bwctrl.c | 15 +++++++++++++++
>>   include/linux/pci.h       | 18 ++++++++++++++++++
>>   2 files changed, 33 insertions(+)
>>
>> diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
>> index 36f939f23d34e8a3b25a2d1b9059e015f298ca94..dafb8d4f1cfba987e1ff08edfc7caba527f0c76b 100644
>> --- a/drivers/pci/pcie/bwctrl.c
>> +++ b/drivers/pci/pcie/bwctrl.c
>> @@ -140,6 +140,8 @@ static int pcie_bwctrl_change_speed(struct pci_dev *port, u16 target_speed, bool
>>   int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
>>   			  bool use_lt)
>>   {
>> +	struct pci_host_bridge *host = pci_find_host_bridge(port->bus);
>> +	bool is_rootbus = pci_is_root_bus(port->bus);
>>   	struct pci_bus *bus = port->subordinate;
>>   	u16 target_speed;
>>   	int ret;
>> @@ -152,6 +154,16 @@ int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
>>   
>>   	target_speed = pcie_bwctrl_select_speed(port, speed_req);
>>   
>> +	/*
>> +	 * The host bridge driver may need to be scaled for targeted speed
>> +	 * otherwise link might not come up at requested speed.
>> +	 */
>> +	if (is_rootbus && host->pre_link_speed_change) {
>> +		ret = host->pre_link_speed_change(host, port, target_speed);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	scoped_guard(rwsem_read, &pcie_bwctrl_setspeed_rwsem) {
>>   		struct pcie_bwctrl_data *data = port->link_bwctrl;
>>   
>> @@ -176,6 +188,9 @@ int pcie_set_target_speed(struct pci_dev *port, enum pci_bus_speed speed_req,
>>   	    !list_empty(&bus->devices))
>>   		ret = -EAGAIN;
>>   
>> +	if (bus && is_rootbus && host->post_link_speed_change)
>> +		host->post_link_speed_change(host, port, pci_bus_speed2lnkctl2(bus->cur_bus_speed));
>> +
>>   	return ret;
>>   }
>>   
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 05e68f35f39238f8b9ce08df97b384d1c1e89bbe..1740bab514b0a9a61c027463a1fb154843312a22 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -599,6 +599,24 @@ struct pci_host_bridge {
>>   	void (*release_fn)(struct pci_host_bridge *);
>>   	int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>>   	void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>> +	/*
>> +	 * Callback to the host bridge drivers to update ICC BW votes, clock
>> +	 * frequencies etc.. for the link re-train to come up in targeted speed.
>> +	 * These are intended to be called by devices directly attached to the
>> +	 * Root Port. These are called by a single client Endpoint driver, so
>> +	 * there is no need for explicit locking mechanisms.
>> +	 */
>> +	int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
>> +				     struct pci_dev *dev, int speed);
>> +	/*
>> +	 * Callback to the host bridge drivers to adjust ICC BW votes, clock
>> +	 * frequencies etc.. to the updated speed after link re-train. These
>> +	 * are intended to be called by devices directly attached to the
>> +	 * Root Port. These are called by a single client Endpoint driver,
>> +	 * so there is no need for explicit locking mechanisms.
>> +	 */
>> +	void (*post_link_speed_change)(struct pci_host_bridge *bridge,
>> +				       struct pci_dev *dev, int speed);
>>   	void		*release_data;
>>   	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
>>   	unsigned int	no_ext_tags:1;		/* No Extended Tags */
>>
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH v4 03/11] bus: mhi: host: Add support to read MHI capabilities

[Krishna Chaitanya Chundru]

On 7/8/2025 10:06 PM, Manivannan Sadhasivam wrote:
> On Mon, Jun 09, 2025 at 04:21:24PM GMT, Krishna Chaitanya Chundru wrote:
>> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>
>> As per MHI spec v1.2,sec 6.6, MHI has capability registers which are
>> located after the ERDB array. The location of this group of registers is
>> indicated by the MISCOFF register. Each capability has a capability ID to
>> determine which functionality is supported and each capability will point
>> to the next capability supported.
>>
>> Add a basic function to read those capabilities offsets.
>>
>> Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>>   drivers/bus/mhi/common.h    | 13 +++++++++++++
>>   drivers/bus/mhi/host/init.c | 34 ++++++++++++++++++++++++++++++++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
>> index dda340aaed95a5573a2ec776ca712e11a1ed0b52..58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd 100644
>> --- a/drivers/bus/mhi/common.h
>> +++ b/drivers/bus/mhi/common.h
>> @@ -16,6 +16,7 @@
>>   #define MHICFG				0x10
>>   #define CHDBOFF				0x18
>>   #define ERDBOFF				0x20
>> +#define MISCOFF				0x24
>>   #define BHIOFF				0x28
>>   #define BHIEOFF				0x2c
>>   #define DEBUGOFF			0x30
>> @@ -113,6 +114,9 @@
>>   #define MHISTATUS_MHISTATE_MASK		GENMASK(15, 8)
>>   #define MHISTATUS_SYSERR_MASK		BIT(2)
>>   #define MHISTATUS_READY_MASK		BIT(0)
>> +#define MISC_CAP_MASK			GENMASK(31, 0)
>> +#define CAP_CAPID_MASK			GENMASK(31, 24)
>> +#define CAP_NEXT_CAP_MASK		GENMASK(23, 12)
>>   
>>   /* Command Ring Element macros */
>>   /* No operation command */
>> @@ -204,6 +208,15 @@
>>   #define MHI_RSCTRE_DATA_DWORD1		cpu_to_le32(FIELD_PREP(GENMASK(23, 16), \
>>   							       MHI_PKT_TYPE_COALESCING))
>>   
>> +enum mhi_capability_type {
>> +	MHI_CAP_ID_INTX = 0x1,
>> +	MHI_CAP_ID_TIME_SYNC = 0x2,
>> +	MHI_CAP_ID_BW_SCALE = 0x3,
>> +	MHI_CAP_ID_TSC_TIME_SYNC = 0x4,
>> +	MHI_CAP_ID_MAX_TRB_LEN = 0x5,
>> +	MHI_CAP_ID_MAX,
>> +};
>> +
>>   enum mhi_pkt_type {
>>   	MHI_PKT_TYPE_INVALID = 0x0,
>>   	MHI_PKT_TYPE_NOOP_CMD = 0x1,
>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>> index 13e7a55f54ff45b83b3f18b97e2cdd83d4836fe3..9102ce13a2059f599b46d25ef631f643142642be 100644
>> --- a/drivers/bus/mhi/host/init.c
>> +++ b/drivers/bus/mhi/host/init.c
>> @@ -467,6 +467,40 @@ int mhi_init_dev_ctxt(struct mhi_controller *mhi_cntrl)
>>   	return ret;
>>   }
>>   
>> +static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32 capability, u32 *offset)
>> +{
>> +	u32 val, cur_cap, next_offset;
>> +	int ret;
>> +
>> +	/* Get the first supported capability offset */
>> +	ret = mhi_read_reg_field(mhi_cntrl, mhi_cntrl->regs, MISCOFF, MISC_CAP_MASK, offset);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*offset = (__force u32)le32_to_cpu(*offset);
> 
> Why do you need __force attribute? What does it suppress? Is it because the
> pointer is not le32?
> 
yes to suppress warnings.

- Krishna Chaitanya.
> - Mani
> 
>> +	do {
>> +		if (*offset >= mhi_cntrl->reg_len)
>> +			return -ENXIO;
>> +
>> +		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, *offset, &val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		val = (__force u32)le32_to_cpu(val);
>> +		cur_cap = FIELD_GET(CAP_CAPID_MASK, val);
>> +		next_offset = FIELD_GET(CAP_NEXT_CAP_MASK, val);
>> +		if (cur_cap >= MHI_CAP_ID_MAX)
>> +			return -ENXIO;
>> +
>> +		if (cur_cap == capability)
>> +			return 0;
>> +
>> +		*offset = next_offset;
>> +	} while (next_offset);
>> +
>> +	return -ENXIO;
>> +}
>> +
>>   int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
>>   {
>>   	u32 val;
>>
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH v4 03/11] bus: mhi: host: Add support to read MHI capabilities

[Ilpo Järvinen]

On Wed, 9 Jul 2025, Krishna Chaitanya Chundru wrote:
> On 7/8/2025 10:06 PM, Manivannan Sadhasivam wrote:
> > On Mon, Jun 09, 2025 at 04:21:24PM GMT, Krishna Chaitanya Chundru wrote:
> > > From: Vivek Pernamitta <quic_vpernami@quicinc.com>
> > > 
> > > As per MHI spec v1.2,sec 6.6, MHI has capability registers which are
> > > located after the ERDB array. The location of this group of registers is
> > > indicated by the MISCOFF register. Each capability has a capability ID to
> > > determine which functionality is supported and each capability will point
> > > to the next capability supported.
> > > 
> > > Add a basic function to read those capabilities offsets.
> > > 
> > > Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
> > > Signed-off-by: Krishna Chaitanya Chundru
> > > <krishna.chundru@oss.qualcomm.com>
> > > ---
> > >   drivers/bus/mhi/common.h    | 13 +++++++++++++
> > >   drivers/bus/mhi/host/init.c | 34 ++++++++++++++++++++++++++++++++++
> > >   2 files changed, 47 insertions(+)
> > > 
> > > diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
> > > index
> > > dda340aaed95a5573a2ec776ca712e11a1ed0b52..58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd
> > > 100644
> > > --- a/drivers/bus/mhi/common.h
> > > +++ b/drivers/bus/mhi/common.h
> > > @@ -16,6 +16,7 @@
> > >   #define MHICFG				0x10
> > >   #define CHDBOFF				0x18
> > >   #define ERDBOFF				0x20
> > > +#define MISCOFF				0x24
> > >   #define BHIOFF				0x28
> > >   #define BHIEOFF				0x2c
> > >   #define DEBUGOFF			0x30
> > > @@ -113,6 +114,9 @@
> > >   #define MHISTATUS_MHISTATE_MASK		GENMASK(15, 8)
> > >   #define MHISTATUS_SYSERR_MASK		BIT(2)
> > >   #define MHISTATUS_READY_MASK		BIT(0)
> > > +#define MISC_CAP_MASK			GENMASK(31, 0)
> > > +#define CAP_CAPID_MASK			GENMASK(31, 24)
> > > +#define CAP_NEXT_CAP_MASK		GENMASK(23, 12)
> > >     /* Command Ring Element macros */
> > >   /* No operation command */
> > > @@ -204,6 +208,15 @@
> > >   #define MHI_RSCTRE_DATA_DWORD1
> > > cpu_to_le32(FIELD_PREP(GENMASK(23, 16), \
> > >   							       MHI_PKT_TYPE_COALESCING))
> > >   +enum mhi_capability_type {
> > > +	MHI_CAP_ID_INTX = 0x1,
> > > +	MHI_CAP_ID_TIME_SYNC = 0x2,
> > > +	MHI_CAP_ID_BW_SCALE = 0x3,
> > > +	MHI_CAP_ID_TSC_TIME_SYNC = 0x4,
> > > +	MHI_CAP_ID_MAX_TRB_LEN = 0x5,
> > > +	MHI_CAP_ID_MAX,
> > > +};
> > > +
> > >   enum mhi_pkt_type {
> > >   	MHI_PKT_TYPE_INVALID = 0x0,
> > >   	MHI_PKT_TYPE_NOOP_CMD = 0x1,
> > > diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> > > index
> > > 13e7a55f54ff45b83b3f18b97e2cdd83d4836fe3..9102ce13a2059f599b46d25ef631f643142642be
> > > 100644
> > > --- a/drivers/bus/mhi/host/init.c
> > > +++ b/drivers/bus/mhi/host/init.c
> > > @@ -467,6 +467,40 @@ int mhi_init_dev_ctxt(struct mhi_controller
> > > *mhi_cntrl)
> > >   	return ret;
> > >   }
> > >   +static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32
> > > capability, u32 *offset)
> > > +{
> > > +	u32 val, cur_cap, next_offset;
> > > +	int ret;
> > > +
> > > +	/* Get the first supported capability offset */
> > > +	ret = mhi_read_reg_field(mhi_cntrl, mhi_cntrl->regs, MISCOFF,
> > > MISC_CAP_MASK, offset);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	*offset = (__force u32)le32_to_cpu(*offset);
> > 
> > Why do you need __force attribute? What does it suppress? Is it because the
> > pointer is not le32?
> > 
> yes to suppress warnings.

I'm pretty sure sparce with endianness checking won't be happy with that 
construct as you pass u32 where le32_to_cpu() expects __le32. Have you 
checked this with sparse? (It might not check endianness with default args.)

> > > +	do {
> > > +		if (*offset >= mhi_cntrl->reg_len)
> > > +			return -ENXIO;
> > > +
> > > +		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, *offset, &val);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		val = (__force u32)le32_to_cpu(val);
> > > +		cur_cap = FIELD_GET(CAP_CAPID_MASK, val);
> > > +		next_offset = FIELD_GET(CAP_NEXT_CAP_MASK, val);
> > > +		if (cur_cap >= MHI_CAP_ID_MAX)
> > > +			return -ENXIO;
> > > +
> > > +		if (cur_cap == capability)
> > > +			return 0;
> > > +
> > > +		*offset = next_offset;
> > > +	} while (next_offset);
> > > +
> > > +	return -ENXIO;
> > > +}

There's a generalization of capability search in Hans Zhang's series, 
can it be used here too?


-- 
 i.

Re: [PATCH v4 04/11] bus: mhi: host: Add support for Bandwidth scale

[Krishna Chaitanya Chundru]

On 7/8/2025 10:36 PM, Manivannan Sadhasivam wrote:
> On Mon, Jun 09, 2025 at 04:21:25PM GMT, Krishna Chaitanya Chundru wrote:
>> As per MHI spec v1.2, sec 14, MHI supports bandwidth scaling to reduce
>> power consumption. MHI bandwidth scaling is advertised by devices that
>> contain the bandwidth scaling capability registers. If enabled, the device
>> aggregates bandwidth requirements and sends them to the host through
>> dedicated mhi event ring. After the host performs the bandwidth switch,
>> it sends an acknowledgment by ringing a doorbell.
>>
>> if the host supports bandwidth scaling events, then it must set
>> BW_CFG.ENABLED bit, set BW_CFG.DB_CHAN_ID to the channel ID to the
>> doorbell that will be used by the host to communicate the bandwidth
>> scaling status and BW_CFG.ER_INDEX to the index for the event ring
>> to which the device should send bandwidth scaling request in the
>> bandwidth scaling capability register.
>>
>> As part of mmio init check if the bw scale capability is present or not,
>> if present advertise host supports bw scale by setting all the required
>> fields.
>>
>> MHI layer will only forward the bw scaling request to the controller
>> driver since MHI doesn't have any idea about transport layer used by
>> the controller, it is responsibility of the controller driver to do actual
>> bw scaling and then pass status to the MHI. MHI will response back to the
>> device based up on the status of the bw scale received.
>>
>> Add a new get_misc_doorbell() to get doorbell for misc capabilities to
>> use the doorbell with mhi events like MHI BW scale etc.
>>
>> Use workqueue & mutex for the bw scale events as the pci_set_target_speed()
>> which will called by the mhi controller driver can sleep.
>>
>> Co-developed-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
>> Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>>   drivers/bus/mhi/common.h        | 13 ++++++
>>   drivers/bus/mhi/host/init.c     | 63 +++++++++++++++++++++++++-
>>   drivers/bus/mhi/host/internal.h |  7 ++-
>>   drivers/bus/mhi/host/main.c     | 98 ++++++++++++++++++++++++++++++++++++++++-
>>   drivers/bus/mhi/host/pm.c       | 10 ++++-
>>   include/linux/mhi.h             | 13 ++++++
>>   6 files changed, 198 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
>> index 58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd..6e342519d80b7725e9ef5390a3eb2a06ac69ceac 100644
>> --- a/drivers/bus/mhi/common.h
>> +++ b/drivers/bus/mhi/common.h
>> @@ -217,6 +217,19 @@ enum mhi_capability_type {
>>   	MHI_CAP_ID_MAX,
>>   };
>>   
>> +/* MHI Bandwidth scaling offsets */
>> +#define MHI_BW_SCALE_CFG_OFFSET		0x4
>> +#define MHI_BW_SCALE_CAP_ID		(3)
>> +#define MHI_BW_SCALE_DB_CHAN_ID		GENMASK(31, 25)
>> +#define MHI_BW_SCALE_ENABLED		BIT(24)
>> +#define MHI_BW_SCALE_ER_INDEX		GENMASK(23, 19)
>> +
>> +#define MHI_TRE_GET_EV_BW_REQ_SEQ(tre)	FIELD_GET(GENMASK(15, 8), (MHI_TRE_GET_DWORD(tre, 0)))
>> +
>> +#define MHI_BW_SCALE_RESULT(status, seq)	cpu_to_le32(FIELD_PREP(GENMASK(11, 8), status) | \
>> +						FIELD_PREP(GENMASK(7, 0), seq))
>> +#define MHI_BW_SCALE_NACK			0xF
>> +
>>   enum mhi_pkt_type {
>>   	MHI_PKT_TYPE_INVALID = 0x0,
>>   	MHI_PKT_TYPE_NOOP_CMD = 0x1,
>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>> index 9102ce13a2059f599b46d25ef631f643142642be..26703fea6272de7fd19c6ee76be067f0ff0fd309 100644
>> --- a/drivers/bus/mhi/host/init.c
>> +++ b/drivers/bus/mhi/host/init.c
>> @@ -501,10 +501,55 @@ static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32 capability,
>>   	return -ENXIO;
>>   }
>>   
>> +static int mhi_get_er_index(struct mhi_controller *mhi_cntrl,
>> +			    enum mhi_er_data_type type)
>> +{
>> +	struct mhi_event *mhi_event = mhi_cntrl->mhi_event;
>> +	int i;
>> +
>> +	/* Find event ring for requested type */
>> +	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>> +		if (mhi_event->data_type == type)
>> +			return mhi_event->er_index;
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>> +
>> +static int mhi_init_bw_scale(struct mhi_controller *mhi_cntrl,
>> +			     int bw_scale_db)
>> +{
>> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> +	u32 bw_cfg_offset, val;
>> +	int ret, er_index;
>> +
>> +	ret = mhi_find_capability(mhi_cntrl, MHI_BW_SCALE_CAP_ID, &bw_cfg_offset);
>> +	if (ret)
>> +		return ret;
>> +
>> +	er_index = mhi_get_er_index(mhi_cntrl, MHI_ER_BW_SCALE);
>> +	if (er_index < 0)
>> +		return er_index;
>> +
>> +	bw_cfg_offset += MHI_BW_SCALE_CFG_OFFSET;
>> +
>> +	/* Advertise host support */
>> +	val = (__force u32)cpu_to_le32(FIELD_PREP(MHI_BW_SCALE_DB_CHAN_ID, bw_scale_db) |
>> +				       FIELD_PREP(MHI_BW_SCALE_ER_INDEX, er_index) |
>> +				       MHI_BW_SCALE_ENABLED);
>> +
> 
> It is wrong to store the value of cpu_to_le32() in a non-le32 variable.
> mhi_write_reg() accepts the 'val' in native endian. writel used in the
> controller drivers should take care of converting to LE before writing to the
> device.
> 
ok then I will revert to u32.

I think we need a patch in the controller drivers seperately to handle
this.
>> +	mhi_write_reg(mhi_cntrl, mhi_cntrl->regs, bw_cfg_offset, val);
>> +
>> +	dev_dbg(dev, "Bandwidth scaling setup complete with event ring: %d\n",
>> +		er_index);
>> +
>> +	return 0;
>> +}
>> +
>>   int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
>>   {
>>   	u32 val;
>> -	int i, ret;
>> +	int i, ret, doorbell = 0;
>>   	struct mhi_chan *mhi_chan;
>>   	struct mhi_event *mhi_event;
>>   	void __iomem *base = mhi_cntrl->regs;
>> @@ -638,6 +683,16 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
>>   		return ret;
>>   	}
>>   
>> +	if (mhi_cntrl->get_misc_doorbell)
>> +		doorbell = mhi_cntrl->get_misc_doorbell(mhi_cntrl, MHI_ER_BW_SCALE);
>> +
>> +	if (doorbell > 0) {
>> +		ret = mhi_init_bw_scale(mhi_cntrl, doorbell);
>> +		if (!ret)
>> +			mhi_cntrl->bw_scale_db = base + val + (8 * doorbell);
>> +		else
>> +			dev_warn(dev, "Failed to setup bandwidth scaling: %d\n", ret);
> 
> That's it? And you would continue to setup doorbell setup later?
> 
event ring for BW scale and capability are exposed during bootup only,
if those are not present at bootup no need to retry later.
>> +	}
> 
> nit: newline
> 
>>   	return 0;
>>   }
>>   
>> @@ -783,6 +838,9 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
>>   		case MHI_ER_CTRL:
>>   			mhi_event->process_event = mhi_process_ctrl_ev_ring;
>>   			break;
>> +		case MHI_ER_BW_SCALE:
>> +			mhi_event->process_event = mhi_process_bw_scale_ev_ring;
>> +			break;
>>   		default:
>>   			dev_err(dev, "Event Ring type not supported\n");
>>   			goto error_ev_cfg;
>> @@ -1017,9 +1075,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>>   
>>   		mhi_event->mhi_cntrl = mhi_cntrl;
>>   		spin_lock_init(&mhi_event->lock);
>> +		mutex_init(&mhi_event->mutex);
>>   		if (mhi_event->data_type == MHI_ER_CTRL)
>>   			tasklet_init(&mhi_event->task, mhi_ctrl_ev_task,
>>   				     (ulong)mhi_event);
>> +		else if (mhi_event->data_type == MHI_ER_BW_SCALE)
>> +			INIT_WORK(&mhi_event->work, mhi_process_ev_work);
>>   		else
>>   			tasklet_init(&mhi_event->task, mhi_ev_task,
>>   				     (ulong)mhi_event);
>> diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
>> index ce566f7d2e9240c64044407aa4124ad3cdb98003..cf64adaecad2aeec8569da5276ec60dd7e97e5e0 100644
>> --- a/drivers/bus/mhi/host/internal.h
>> +++ b/drivers/bus/mhi/host/internal.h
>> @@ -248,6 +248,8 @@ struct mhi_event {
>>   	struct mhi_ring ring;
>>   	struct db_cfg db_cfg;
>>   	struct tasklet_struct task;
>> +	struct work_struct work;
>> +	struct mutex mutex; /* lock for synchronization */
> 
> synchronization of what?
I will update in next patch, synchronizatrion between multiple bw scale
events in this case.
> 
>>   	spinlock_t lock;
>>   	int (*process_event)(struct mhi_controller *mhi_cntrl,
>>   			     struct mhi_event *mhi_event,
>> @@ -410,7 +412,8 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
>>   				struct mhi_event *mhi_event, u32 event_quota);
>>   int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
>>   			     struct mhi_event *mhi_event, u32 event_quota);
>> -
>> +int mhi_process_bw_scale_ev_ring(struct mhi_controller *mhi_cntrl,
>> +				 struct mhi_event *mhi_event, u32 event_quota);
>>   /* ISR handlers */
>>   irqreturn_t mhi_irq_handler(int irq_number, void *dev);
>>   irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev);
>> @@ -426,5 +429,5 @@ void mhi_unmap_single_no_bb(struct mhi_controller *mhi_cntrl,
>>   			    struct mhi_buf_info *buf_info);
>>   void mhi_unmap_single_use_bb(struct mhi_controller *mhi_cntrl,
>>   			     struct mhi_buf_info *buf_info);
>> -
>> +void mhi_process_ev_work(struct work_struct *work);
>>   #endif /* _MHI_INT_H */
>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
>> index 9bb0df43ceef1e54e8817422516aab1def6fdc4a..f87e9550b59227947fc7268c5799eea274c66b98 100644
>> --- a/drivers/bus/mhi/host/main.c
>> +++ b/drivers/bus/mhi/host/main.c
>> @@ -472,7 +472,10 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>>   		if (mhi_dev)
>>   			mhi_notify(mhi_dev, MHI_CB_PENDING_DATA);
>>   	} else {
>> -		tasklet_schedule(&mhi_event->task);
>> +		if (mhi_event->data_type == MHI_ER_BW_SCALE)
>> +			queue_work(mhi_cntrl->hiprio_wq, &mhi_event->work);
>> +		else
>> +			tasklet_schedule(&mhi_event->task);
>>   	}
>>   
>>   	return IRQ_HANDLED;
>> @@ -1049,6 +1052,99 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
>>   	return count;
>>   }
>>   
>> +int mhi_process_bw_scale_ev_ring(struct mhi_controller *mhi_cntrl,
>> +				 struct mhi_event *mhi_event, u32 event_quota)
>> +{
>> +	struct mhi_event_ctxt *er_ctxt = &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index];
>> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> +	struct mhi_ring *ev_ring = &mhi_event->ring;
>> +	dma_addr_t ptr = le64_to_cpu(er_ctxt->rp);
>> +	u32 response = MHI_BW_SCALE_NACK;
>> +	struct mhi_ring_element *dev_rp;
>> +	struct mhi_link_info link_info;
>> +	int ret = -EINVAL;
>> +
>> +	if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state))) {
>> +		ret =  -EIO;
>> +		goto exit_bw_scale;
>> +	}
>> +
>> +	if (!MHI_IN_MISSION_MODE(mhi_cntrl->ee))
>> +		goto exit_bw_scale;
>> +
>> +	if (!is_valid_ring_ptr(ev_ring, ptr)) {
>> +		dev_err(dev,
>> +			"Event ring rp points outside of the event ring\n");
>> +		ret =  -EIO;
>> +		goto exit_bw_scale;
>> +	}
>> +
>> +	dev_rp = mhi_to_virtual(ev_ring, ptr);
>> +
>> +	/* If rp points to base, we need to wrap it around */
>> +	if (dev_rp == ev_ring->base)
>> +		dev_rp = ev_ring->base + ev_ring->len;
>> +	dev_rp--;
>> +
>> +	/* Fast forward to currently processed element and recycle er */
>> +	ev_ring->rp = dev_rp;
>> +	ev_ring->wp = dev_rp - 1;
>> +	if (ev_ring->wp < ev_ring->base)
>> +		ev_ring->wp = ev_ring->base + ev_ring->len - ev_ring->el_size;
>> +	mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
>> +
>> +	if (WARN_ON(MHI_TRE_GET_EV_TYPE(dev_rp) != MHI_PKT_TYPE_BW_REQ_EVENT)) {
>> +		dev_err(dev, "!BW SCALE REQ event\n");
> 
> What does it mean?
> 
I will update it in next patch saying not a bw scale event.
>> +		goto exit_bw_scale;
>> +	}
>> +
>> +	link_info.target_link_speed = MHI_TRE_GET_EV_LINKSPEED(dev_rp);
>> +	link_info.target_link_width = MHI_TRE_GET_EV_LINKWIDTH(dev_rp);
>> +	link_info.sequence_num = MHI_TRE_GET_EV_BW_REQ_SEQ(dev_rp);
>> +
>> +	dev_dbg(dev, "Received BW_REQ with seq:%d link speed:0x%x width:0x%x\n",
>> +		link_info.sequence_num,
>> +		link_info.target_link_speed,
>> +		link_info.target_link_width);
>> +
>> +	/* Bring host and device out of suspended states */
>> +	ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
> 
> Looks like mhi_device_get_sync() is going runtime_get()/runtime_put() inside
> mhi_trigger_resume(). I'm wondering why that is necessary.
> 
Before mhi_trigger_resume we are doing wake_get, which will make sure
device will not transition to the low power modes while servicing this
event. And also make sure mhi state is in M0 only.

As we are in workqueue this can be scheduled at some later time and by
that time mhi can go to m1 or m2 state.

- Krishna Chaitanya.
>> +	if (ret)
>> +		goto exit_bw_scale;
>> +
>> +	mhi_cntrl->runtime_get(mhi_cntrl);
>> +
> 
> Here, you want to resume the controller, which is fine.
> 
>> +	ret = mhi_cntrl->bw_scale(mhi_cntrl, &link_info);
>> +	if (!ret)
>> +		response = 0;
>> +
>> +	response = MHI_BW_SCALE_RESULT(response, link_info.sequence_num);
>> +
>> +	write_lock_bh(&mhi_cntrl->pm_lock);
>> +	mhi_write_reg(mhi_cntrl, mhi_cntrl->bw_scale_db, 0, response);
>> +	write_unlock_bh(&mhi_cntrl->pm_lock);
>> +
>> +	mhi_cntrl->runtime_put(mhi_cntrl);
>> +	mhi_device_put(mhi_cntrl->mhi_dev);
>> +
>> +exit_bw_scale:
>> +	return ret;
> 
> Just return 'ret' directly.
> 
> - Mani
> 

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Krishna Chaitanya Chundru]

On 7/9/2025 2:40 PM, Ilpo Järvinen wrote:
> On Tue, 8 Jul 2025, Manivannan Sadhasivam wrote:
> 
>> On Mon, Jun 09, 2025 at 04:21:27PM GMT, Krishna Chaitanya Chundru wrote:
>>> ASPM states are not being enabled back with pci_enable_link_state() when
>>> they are disabled by pci_disable_link_state(). This is because of the
>>> aspm_disable flag is not getting cleared in pci_enable_link_state(), this
>>> flag is being properly cleared when ASPM is controlled by sysfs.
>>>
>>
>> A comment in pcie_config_aspm_link() says:
>>
>>   /* Enable only the states that were not explicitly disabled */
>>
>> But the function is called from both aspm_attr_store_common() and
>> __pci_enable_link_state(). So I don't know if this is behavior is intentional
>> or wrong.
> 
> Hi,
> 
> I think it's intentional. Whether the behavior is useful is another good
> question but the current behavior aligns with the explanation in the
> comment.
> 
> My understanding of the situation is:
> 
> pci_disable_link_state() and pci_enable_link_state() are not symmetric
> despite the names, never have been (this is one of those many quirks ASPM
> driver has which should be eventually cleaned up, IMO).
> 
> It might be appropriate to rename pci_enable_link_state() to
> pci_set_default_link_state() to match the name to its functionality (and
> the function comment):
> 
>   * pci_enable_link_state - Clear and set the default device link state
> 
> Note: "the default ... link state".
> 
> 
> I've already raised this concern earlier! As you see, my comment are
> not getting addressed. I'd like to see the author does one of these:
> 
Hi llpo,

I replied to your comment on v3 patch[1], and I feel instead of having
new function() we can use same API to our purpose.
> 1) Renames pci_enable_link_state() to pci_set_default_link_state()
> 
> 1b) If pci_enable_link_state() is still needed after that, a new function
> is added to symmetrically pair with pci_disable_link_state().
> 
> or alternatively,
> 
> 2) Changelog justifies very clearly why this change is okay with the
> existing callers. (And obviously the function comment should be altered to
> match the functionality in that case too).
> 
> If approach 2 is chosen, it should be very carefully reviewed when it
> comes to the callers.
>
I am in favor of approach 2 which you suggested, but lets wait for other
reviewers feedback on this. Based up on the response i will make
necessary changes in v5.

[1] 
https://lore.kernel.org/all/b3d818f5-942c-1761-221d-af7d7e8f3624@oss.qualcomm.com/

- Krishna Chaitanya.
> 
>>> Clear the aspm_disable flag with the requested ASPM states requested by
>>> pci_enable_link_state().
>>>
>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>
>> Fixes tag?
>>
>> - Mani
>>
>>> ---
>>>   drivers/pci/pcie/aspm.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index 94324fc0d3e650cd3ca2c0bb8c1895ca7e647b9d..0f858ef86111b43328bc7db01e6493ce67178458 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -1453,6 +1453,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
>>>   		down_read(&pci_bus_sem);
>>>   	mutex_lock(&aspm_lock);
>>>   	link->aspm_default = pci_calc_aspm_enable_mask(state);
>>> +	link->aspm_disable &= ~state;
>>>   	pcie_config_aspm_link(link, policy_to_aspm_state(link));
>>>   
>>>   	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
>>>
>>> -- 
>>> 2.34.1
>>>
>>
>>
> 

Re: [PATCH v4 03/11] bus: mhi: host: Add support to read MHI capabilities

[Hans Zhang]

On 2025/7/9 20:20, Ilpo Järvinen wrote:
>>>> +	do {
>>>> +		if (*offset >= mhi_cntrl->reg_len)
>>>> +			return -ENXIO;
>>>> +
>>>> +		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, *offset, &val);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		val = (__force u32)le32_to_cpu(val);
>>>> +		cur_cap = FIELD_GET(CAP_CAPID_MASK, val);
>>>> +		next_offset = FIELD_GET(CAP_NEXT_CAP_MASK, val);
>>>> +		if (cur_cap >= MHI_CAP_ID_MAX)
>>>> +			return -ENXIO;
>>>> +
>>>> +		if (cur_cap == capability)
>>>> +			return 0;
>>>> +
>>>> +		*offset = next_offset;
>>>> +	} while (next_offset);
>>>> +
>>>> +	return -ENXIO;
>>>> +}
> 
> There's a generalization of capability search in Hans Zhang's series,
> can it be used here too?

Dear Ilpo,

Could you help add your review tag to my series of patches? Mani has 
added the Acked-by tag. If you add the review tag, I believe Bjorn will 
merge it in. At the same time, others can use the patches of my series. 
Thank you very much for your previous suggestions and time.

Best regards,
Hans

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Manivannan Sadhasivam]

On Wed, Jul 09, 2025 at 06:01:22PM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 7/9/2025 2:40 PM, Ilpo Järvinen wrote:
> > On Tue, 8 Jul 2025, Manivannan Sadhasivam wrote:
> > 
> > > On Mon, Jun 09, 2025 at 04:21:27PM GMT, Krishna Chaitanya Chundru wrote:
> > > > ASPM states are not being enabled back with pci_enable_link_state() when
> > > > they are disabled by pci_disable_link_state(). This is because of the
> > > > aspm_disable flag is not getting cleared in pci_enable_link_state(), this
> > > > flag is being properly cleared when ASPM is controlled by sysfs.
> > > > 
> > > 
> > > A comment in pcie_config_aspm_link() says:
> > > 
> > >   /* Enable only the states that were not explicitly disabled */
> > > 
> > > But the function is called from both aspm_attr_store_common() and
> > > __pci_enable_link_state(). So I don't know if this is behavior is intentional
> > > or wrong.
> > 
> > Hi,
> > 
> > I think it's intentional. Whether the behavior is useful is another good
> > question but the current behavior aligns with the explanation in the
> > comment.
> > 
> > My understanding of the situation is:
> > 
> > pci_disable_link_state() and pci_enable_link_state() are not symmetric
> > despite the names, never have been (this is one of those many quirks ASPM
> > driver has which should be eventually cleaned up, IMO).
> > 
> > It might be appropriate to rename pci_enable_link_state() to
> > pci_set_default_link_state() to match the name to its functionality (and
> > the function comment):
> > 
> >   * pci_enable_link_state - Clear and set the default device link state
> > 
> > Note: "the default ... link state".
> > 
> > 
> > I've already raised this concern earlier! As you see, my comment are
> > not getting addressed. I'd like to see the author does one of these:
> > 
> Hi llpo,
> 
> I replied to your comment on v3 patch[1], and I feel instead of having
> new function() we can use same API to our purpose.

You replied to Ilpo, but never got an agreement. Please try to close the
discussions before posting next rev. If reviewers forgot to reply to your query,
feel free to ping them in the same thread itself.

> > 1) Renames pci_enable_link_state() to pci_set_default_link_state()
> > 
> > 1b) If pci_enable_link_state() is still needed after that, a new function
> > is added to symmetrically pair with pci_disable_link_state().
> > 
> > or alternatively,
> > 
> > 2) Changelog justifies very clearly why this change is okay with the
> > existing callers. (And obviously the function comment should be altered to
> > match the functionality in that case too).
> > 
> > If approach 2 is chosen, it should be very carefully reviewed when it
> > comes to the callers.
> > 
> I am in favor of approach 2 which you suggested, but lets wait for other
> reviewers feedback on this. Based up on the response i will make
> necessary changes in v5.
> 

I would go for (1). It is always going to be a problem to change a legacy API
like this. We might end up causing regressions. So it is safe to rename to
reflect the purpose and try to come up with a new API that does what you want.
If callers want to migrate to the new API, they can also do it in the future.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 04/11] bus: mhi: host: Add support for Bandwidth scale

[Manivannan Sadhasivam]

On Wed, Jul 09, 2025 at 05:51:34PM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 7/8/2025 10:36 PM, Manivannan Sadhasivam wrote:
> > On Mon, Jun 09, 2025 at 04:21:25PM GMT, Krishna Chaitanya Chundru wrote:
> > > As per MHI spec v1.2, sec 14, MHI supports bandwidth scaling to reduce
> > > power consumption. MHI bandwidth scaling is advertised by devices that
> > > contain the bandwidth scaling capability registers. If enabled, the device
> > > aggregates bandwidth requirements and sends them to the host through
> > > dedicated mhi event ring. After the host performs the bandwidth switch,
> > > it sends an acknowledgment by ringing a doorbell.
> > > 
> > > if the host supports bandwidth scaling events, then it must set
> > > BW_CFG.ENABLED bit, set BW_CFG.DB_CHAN_ID to the channel ID to the
> > > doorbell that will be used by the host to communicate the bandwidth
> > > scaling status and BW_CFG.ER_INDEX to the index for the event ring
> > > to which the device should send bandwidth scaling request in the
> > > bandwidth scaling capability register.
> > > 
> > > As part of mmio init check if the bw scale capability is present or not,
> > > if present advertise host supports bw scale by setting all the required
> > > fields.
> > > 
> > > MHI layer will only forward the bw scaling request to the controller
> > > driver since MHI doesn't have any idea about transport layer used by
> > > the controller, it is responsibility of the controller driver to do actual
> > > bw scaling and then pass status to the MHI. MHI will response back to the
> > > device based up on the status of the bw scale received.
> > > 
> > > Add a new get_misc_doorbell() to get doorbell for misc capabilities to
> > > use the doorbell with mhi events like MHI BW scale etc.
> > > 
> > > Use workqueue & mutex for the bw scale events as the pci_set_target_speed()
> > > which will called by the mhi controller driver can sleep.
> > > 
> > > Co-developed-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> > > Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > ---
> > >   drivers/bus/mhi/common.h        | 13 ++++++
> > >   drivers/bus/mhi/host/init.c     | 63 +++++++++++++++++++++++++-
> > >   drivers/bus/mhi/host/internal.h |  7 ++-
> > >   drivers/bus/mhi/host/main.c     | 98 ++++++++++++++++++++++++++++++++++++++++-
> > >   drivers/bus/mhi/host/pm.c       | 10 ++++-
> > >   include/linux/mhi.h             | 13 ++++++
> > >   6 files changed, 198 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
> > > index 58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd..6e342519d80b7725e9ef5390a3eb2a06ac69ceac 100644
> > > --- a/drivers/bus/mhi/common.h
> > > +++ b/drivers/bus/mhi/common.h
> > > @@ -217,6 +217,19 @@ enum mhi_capability_type {
> > >   	MHI_CAP_ID_MAX,
> > >   };
> > > +/* MHI Bandwidth scaling offsets */
> > > +#define MHI_BW_SCALE_CFG_OFFSET		0x4
> > > +#define MHI_BW_SCALE_CAP_ID		(3)
> > > +#define MHI_BW_SCALE_DB_CHAN_ID		GENMASK(31, 25)
> > > +#define MHI_BW_SCALE_ENABLED		BIT(24)
> > > +#define MHI_BW_SCALE_ER_INDEX		GENMASK(23, 19)
> > > +
> > > +#define MHI_TRE_GET_EV_BW_REQ_SEQ(tre)	FIELD_GET(GENMASK(15, 8), (MHI_TRE_GET_DWORD(tre, 0)))
> > > +
> > > +#define MHI_BW_SCALE_RESULT(status, seq)	cpu_to_le32(FIELD_PREP(GENMASK(11, 8), status) | \
> > > +						FIELD_PREP(GENMASK(7, 0), seq))
> > > +#define MHI_BW_SCALE_NACK			0xF
> > > +
> > >   enum mhi_pkt_type {
> > >   	MHI_PKT_TYPE_INVALID = 0x0,
> > >   	MHI_PKT_TYPE_NOOP_CMD = 0x1,
> > > diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> > > index 9102ce13a2059f599b46d25ef631f643142642be..26703fea6272de7fd19c6ee76be067f0ff0fd309 100644
> > > --- a/drivers/bus/mhi/host/init.c
> > > +++ b/drivers/bus/mhi/host/init.c
> > > @@ -501,10 +501,55 @@ static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32 capability,
> > >   	return -ENXIO;
> > >   }
> > > +static int mhi_get_er_index(struct mhi_controller *mhi_cntrl,
> > > +			    enum mhi_er_data_type type)
> > > +{
> > > +	struct mhi_event *mhi_event = mhi_cntrl->mhi_event;
> > > +	int i;
> > > +
> > > +	/* Find event ring for requested type */
> > > +	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
> > > +		if (mhi_event->data_type == type)
> > > +			return mhi_event->er_index;
> > > +	}
> > > +
> > > +	return -ENOENT;
> > > +}
> > > +
> > > +static int mhi_init_bw_scale(struct mhi_controller *mhi_cntrl,
> > > +			     int bw_scale_db)
> > > +{
> > > +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > > +	u32 bw_cfg_offset, val;
> > > +	int ret, er_index;
> > > +
> > > +	ret = mhi_find_capability(mhi_cntrl, MHI_BW_SCALE_CAP_ID, &bw_cfg_offset);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	er_index = mhi_get_er_index(mhi_cntrl, MHI_ER_BW_SCALE);
> > > +	if (er_index < 0)
> > > +		return er_index;
> > > +
> > > +	bw_cfg_offset += MHI_BW_SCALE_CFG_OFFSET;
> > > +
> > > +	/* Advertise host support */
> > > +	val = (__force u32)cpu_to_le32(FIELD_PREP(MHI_BW_SCALE_DB_CHAN_ID, bw_scale_db) |
> > > +				       FIELD_PREP(MHI_BW_SCALE_ER_INDEX, er_index) |
> > > +				       MHI_BW_SCALE_ENABLED);
> > > +
> > 
> > It is wrong to store the value of cpu_to_le32() in a non-le32 variable.
> > mhi_write_reg() accepts the 'val' in native endian. writel used in the
> > controller drivers should take care of converting to LE before writing to the
> > device.
> > 
> ok then I will revert to u32.
> 
> I think we need a patch in the controller drivers seperately to handle
> this.

Why?

> > > +	mhi_write_reg(mhi_cntrl, mhi_cntrl->regs, bw_cfg_offset, val);
> > > +
> > > +	dev_dbg(dev, "Bandwidth scaling setup complete with event ring: %d\n",
> > > +		er_index);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
> > >   {
> > >   	u32 val;
> > > -	int i, ret;
> > > +	int i, ret, doorbell = 0;
> > >   	struct mhi_chan *mhi_chan;
> > >   	struct mhi_event *mhi_event;
> > >   	void __iomem *base = mhi_cntrl->regs;
> > > @@ -638,6 +683,16 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
> > >   		return ret;
> > >   	}
> > > +	if (mhi_cntrl->get_misc_doorbell)
> > > +		doorbell = mhi_cntrl->get_misc_doorbell(mhi_cntrl, MHI_ER_BW_SCALE);
> > > +
> > > +	if (doorbell > 0) {
> > > +		ret = mhi_init_bw_scale(mhi_cntrl, doorbell);
> > > +		if (!ret)
> > > +			mhi_cntrl->bw_scale_db = base + val + (8 * doorbell);
> > > +		else
> > > +			dev_warn(dev, "Failed to setup bandwidth scaling: %d\n", ret);
> > 
> > That's it? And you would continue to setup doorbell setup later?
> > 
> event ring for BW scale and capability are exposed during bootup only,
> if those are not present at bootup no need to retry later.

I'm not asking you to retry. I was asking why would you continue to initialize
bw_scale resources (like workqueue) even if mhi_init_bw_scale() fails.

> > > +	}
> > 
> > nit: newline
> > 
> > >   	return 0;
> > >   }

[...]

> > > +		goto exit_bw_scale;
> > > +	}
> > > +
> > > +	link_info.target_link_speed = MHI_TRE_GET_EV_LINKSPEED(dev_rp);
> > > +	link_info.target_link_width = MHI_TRE_GET_EV_LINKWIDTH(dev_rp);
> > > +	link_info.sequence_num = MHI_TRE_GET_EV_BW_REQ_SEQ(dev_rp);
> > > +
> > > +	dev_dbg(dev, "Received BW_REQ with seq:%d link speed:0x%x width:0x%x\n",
> > > +		link_info.sequence_num,
> > > +		link_info.target_link_speed,
> > > +		link_info.target_link_width);
> > > +
> > > +	/* Bring host and device out of suspended states */
> > > +	ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
> > 
> > Looks like mhi_device_get_sync() is going runtime_get()/runtime_put() inside
> > mhi_trigger_resume(). I'm wondering why that is necessary.
> > 
> Before mhi_trigger_resume we are doing wake_get, which will make sure
> device will not transition to the low power modes while servicing this
> event. And also make sure mhi state is in M0 only.
> 
> As we are in workqueue this can be scheduled at some later time and by
> that time mhi can go to m1 or m2 state.
> 

My comment was more about the behavior of mhi_trigger_resume(). Why does it call
get() and put()? Sorry if I was not clear.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 04/11] bus: mhi: host: Add support for Bandwidth scale

[Krishna Chaitanya Chundru]

On 7/11/2025 10:03 AM, Manivannan Sadhasivam wrote:
> On Wed, Jul 09, 2025 at 05:51:34PM GMT, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 7/8/2025 10:36 PM, Manivannan Sadhasivam wrote:
>>> On Mon, Jun 09, 2025 at 04:21:25PM GMT, Krishna Chaitanya Chundru wrote:
>>>> As per MHI spec v1.2, sec 14, MHI supports bandwidth scaling to reduce
>>>> power consumption. MHI bandwidth scaling is advertised by devices that
>>>> contain the bandwidth scaling capability registers. If enabled, the device
>>>> aggregates bandwidth requirements and sends them to the host through
>>>> dedicated mhi event ring. After the host performs the bandwidth switch,
>>>> it sends an acknowledgment by ringing a doorbell.
>>>>
>>>> if the host supports bandwidth scaling events, then it must set
>>>> BW_CFG.ENABLED bit, set BW_CFG.DB_CHAN_ID to the channel ID to the
>>>> doorbell that will be used by the host to communicate the bandwidth
>>>> scaling status and BW_CFG.ER_INDEX to the index for the event ring
>>>> to which the device should send bandwidth scaling request in the
>>>> bandwidth scaling capability register.
>>>>
>>>> As part of mmio init check if the bw scale capability is present or not,
>>>> if present advertise host supports bw scale by setting all the required
>>>> fields.
>>>>
>>>> MHI layer will only forward the bw scaling request to the controller
>>>> driver since MHI doesn't have any idea about transport layer used by
>>>> the controller, it is responsibility of the controller driver to do actual
>>>> bw scaling and then pass status to the MHI. MHI will response back to the
>>>> device based up on the status of the bw scale received.
>>>>
>>>> Add a new get_misc_doorbell() to get doorbell for misc capabilities to
>>>> use the doorbell with mhi events like MHI BW scale etc.
>>>>
>>>> Use workqueue & mutex for the bw scale events as the pci_set_target_speed()
>>>> which will called by the mhi controller driver can sleep.
>>>>
>>>> Co-developed-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
>>>> Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>>> ---
>>>>    drivers/bus/mhi/common.h        | 13 ++++++
>>>>    drivers/bus/mhi/host/init.c     | 63 +++++++++++++++++++++++++-
>>>>    drivers/bus/mhi/host/internal.h |  7 ++-
>>>>    drivers/bus/mhi/host/main.c     | 98 ++++++++++++++++++++++++++++++++++++++++-
>>>>    drivers/bus/mhi/host/pm.c       | 10 ++++-
>>>>    include/linux/mhi.h             | 13 ++++++
>>>>    6 files changed, 198 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
>>>> index 58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd..6e342519d80b7725e9ef5390a3eb2a06ac69ceac 100644
>>>> --- a/drivers/bus/mhi/common.h
>>>> +++ b/drivers/bus/mhi/common.h
>>>> @@ -217,6 +217,19 @@ enum mhi_capability_type {
>>>>    	MHI_CAP_ID_MAX,
>>>>    };
>>>> +/* MHI Bandwidth scaling offsets */
>>>> +#define MHI_BW_SCALE_CFG_OFFSET		0x4
>>>> +#define MHI_BW_SCALE_CAP_ID		(3)
>>>> +#define MHI_BW_SCALE_DB_CHAN_ID		GENMASK(31, 25)
>>>> +#define MHI_BW_SCALE_ENABLED		BIT(24)
>>>> +#define MHI_BW_SCALE_ER_INDEX		GENMASK(23, 19)
>>>> +
>>>> +#define MHI_TRE_GET_EV_BW_REQ_SEQ(tre)	FIELD_GET(GENMASK(15, 8), (MHI_TRE_GET_DWORD(tre, 0)))
>>>> +
>>>> +#define MHI_BW_SCALE_RESULT(status, seq)	cpu_to_le32(FIELD_PREP(GENMASK(11, 8), status) | \
>>>> +						FIELD_PREP(GENMASK(7, 0), seq))
>>>> +#define MHI_BW_SCALE_NACK			0xF
>>>> +
>>>>    enum mhi_pkt_type {
>>>>    	MHI_PKT_TYPE_INVALID = 0x0,
>>>>    	MHI_PKT_TYPE_NOOP_CMD = 0x1,
>>>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>>>> index 9102ce13a2059f599b46d25ef631f643142642be..26703fea6272de7fd19c6ee76be067f0ff0fd309 100644
>>>> --- a/drivers/bus/mhi/host/init.c
>>>> +++ b/drivers/bus/mhi/host/init.c
>>>> @@ -501,10 +501,55 @@ static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32 capability,
>>>>    	return -ENXIO;
>>>>    }
>>>> +static int mhi_get_er_index(struct mhi_controller *mhi_cntrl,
>>>> +			    enum mhi_er_data_type type)
>>>> +{
>>>> +	struct mhi_event *mhi_event = mhi_cntrl->mhi_event;
>>>> +	int i;
>>>> +
>>>> +	/* Find event ring for requested type */
>>>> +	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>>>> +		if (mhi_event->data_type == type)
>>>> +			return mhi_event->er_index;
>>>> +	}
>>>> +
>>>> +	return -ENOENT;
>>>> +}
>>>> +
>>>> +static int mhi_init_bw_scale(struct mhi_controller *mhi_cntrl,
>>>> +			     int bw_scale_db)
>>>> +{
>>>> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>>> +	u32 bw_cfg_offset, val;
>>>> +	int ret, er_index;
>>>> +
>>>> +	ret = mhi_find_capability(mhi_cntrl, MHI_BW_SCALE_CAP_ID, &bw_cfg_offset);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	er_index = mhi_get_er_index(mhi_cntrl, MHI_ER_BW_SCALE);
>>>> +	if (er_index < 0)
>>>> +		return er_index;
>>>> +
>>>> +	bw_cfg_offset += MHI_BW_SCALE_CFG_OFFSET;
>>>> +
>>>> +	/* Advertise host support */
>>>> +	val = (__force u32)cpu_to_le32(FIELD_PREP(MHI_BW_SCALE_DB_CHAN_ID, bw_scale_db) |
>>>> +				       FIELD_PREP(MHI_BW_SCALE_ER_INDEX, er_index) |
>>>> +				       MHI_BW_SCALE_ENABLED);
>>>> +
>>>
>>> It is wrong to store the value of cpu_to_le32() in a non-le32 variable.
>>> mhi_write_reg() accepts the 'val' in native endian. writel used in the
>>> controller drivers should take care of converting to LE before writing to the
>>> device.
>>>
>> ok then I will revert to u32.
>>
>> I think we need a patch in the controller drivers seperately to handle
>> this.
> 
> Why?
> 
what I understood from your previous comment is from here we need to
send u32 only and the controller drivers should take care of
converting u32 to le32.
As of today controller drivers are not considering this and writing
u32 only.
So we need a seperate patch in the controller driver to convert it to
le32.
>>>> +	mhi_write_reg(mhi_cntrl, mhi_cntrl->regs, bw_cfg_offset, val);
>>>> +
>>>> +	dev_dbg(dev, "Bandwidth scaling setup complete with event ring: %d\n",
>>>> +		er_index);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
>>>>    {
>>>>    	u32 val;
>>>> -	int i, ret;
>>>> +	int i, ret, doorbell = 0;
>>>>    	struct mhi_chan *mhi_chan;
>>>>    	struct mhi_event *mhi_event;
>>>>    	void __iomem *base = mhi_cntrl->regs;
>>>> @@ -638,6 +683,16 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
>>>>    		return ret;
>>>>    	}
>>>> +	if (mhi_cntrl->get_misc_doorbell)
>>>> +		doorbell = mhi_cntrl->get_misc_doorbell(mhi_cntrl, MHI_ER_BW_SCALE);
>>>> +
>>>> +	if (doorbell > 0) {
>>>> +		ret = mhi_init_bw_scale(mhi_cntrl, doorbell);
>>>> +		if (!ret)
>>>> +			mhi_cntrl->bw_scale_db = base + val + (8 * doorbell);
>>>> +		else
>>>> +			dev_warn(dev, "Failed to setup bandwidth scaling: %d\n", ret);
>>>
>>> That's it? And you would continue to setup doorbell setup later?
>>>
>> event ring for BW scale and capability are exposed during bootup only,
>> if those are not present at bootup no need to retry later.
> 
> I'm not asking you to retry. I was asking why would you continue to initialize
> bw_scale resources (like workqueue) even if mhi_init_bw_scale() fails.
> 
ack will do it in next patch.
>>>> +	}
>>>
>>> nit: newline
>>>
>>>>    	return 0;
>>>>    }
> 
> [...]
> 
>>>> +		goto exit_bw_scale;
>>>> +	}
>>>> +
>>>> +	link_info.target_link_speed = MHI_TRE_GET_EV_LINKSPEED(dev_rp);
>>>> +	link_info.target_link_width = MHI_TRE_GET_EV_LINKWIDTH(dev_rp);
>>>> +	link_info.sequence_num = MHI_TRE_GET_EV_BW_REQ_SEQ(dev_rp);
>>>> +
>>>> +	dev_dbg(dev, "Received BW_REQ with seq:%d link speed:0x%x width:0x%x\n",
>>>> +		link_info.sequence_num,
>>>> +		link_info.target_link_speed,
>>>> +		link_info.target_link_width);
>>>> +
>>>> +	/* Bring host and device out of suspended states */
>>>> +	ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
>>>
>>> Looks like mhi_device_get_sync() is going runtime_get()/runtime_put() inside
>>> mhi_trigger_resume(). I'm wondering why that is necessary.
>>>
>> Before mhi_trigger_resume we are doing wake_get, which will make sure
>> device will not transition to the low power modes while servicing this
>> event. And also make sure mhi state is in M0 only.
>>
>> As we are in workqueue this can be scheduled at some later time and by
>> that time mhi can go to m1 or m2 state.
>>
> 
> My comment was more about the behavior of mhi_trigger_resume(). Why does it call
> get() and put()? Sorry if I was not clear.
> Get() needed for bringing out of suspend, put() is for balancing the
get() I belive the intention here is to trigger resume only and balance
pm framework.

That said mhi_device_get_sync() has a bug we are doing wake_get() before
triggering resume and also trigger resume will not guarantee that device
had resumed, it is not safe to do wake_get untill device is out of
suspend, we might need to introduce runtime_get_sync() function and new
API for this purpose.
mhi_trigger_resume makes sense in the place like this[1], where ever
there are register writes after trigger resume it may need  to be
replaced with runtime_get_sync().

This function needs to do mhi_cntrl->runtime_get_sync(mhi_cntrl); first
to make sure controller is not in suspend and then mhi_device_get_sync()
to make sure device is staying in M0. and in the end we balance these
with mhi_cntrl->runtime_put(mhi_cntrl) & mhi_device_put().

An taughts in this approach?

[1] 
https://elixir.bootlin.com/linux/v6.6.96/source/drivers/bus/mhi/host/main.c#L1080

- Krishna Chaitanya.
> - Mani
> 

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4385 bytes --]

On Fri, 11 Jul 2025, Manivannan Sadhasivam wrote:

> On Wed, Jul 09, 2025 at 06:01:22PM GMT, Krishna Chaitanya Chundru wrote:
> > 
> > 
> > On 7/9/2025 2:40 PM, Ilpo Järvinen wrote:
> > > On Tue, 8 Jul 2025, Manivannan Sadhasivam wrote:
> > > 
> > > > On Mon, Jun 09, 2025 at 04:21:27PM GMT, Krishna Chaitanya Chundru wrote:
> > > > > ASPM states are not being enabled back with pci_enable_link_state() when
> > > > > they are disabled by pci_disable_link_state(). This is because of the
> > > > > aspm_disable flag is not getting cleared in pci_enable_link_state(), this
> > > > > flag is being properly cleared when ASPM is controlled by sysfs.
> > > > > 
> > > > 
> > > > A comment in pcie_config_aspm_link() says:
> > > > 
> > > >   /* Enable only the states that were not explicitly disabled */
> > > > 
> > > > But the function is called from both aspm_attr_store_common() and
> > > > __pci_enable_link_state(). So I don't know if this is behavior is intentional
> > > > or wrong.
> > > 
> > > Hi,
> > > 
> > > I think it's intentional. Whether the behavior is useful is another good
> > > question but the current behavior aligns with the explanation in the
> > > comment.
> > > 
> > > My understanding of the situation is:
> > > 
> > > pci_disable_link_state() and pci_enable_link_state() are not symmetric
> > > despite the names, never have been (this is one of those many quirks ASPM
> > > driver has which should be eventually cleaned up, IMO).
> > > 
> > > It might be appropriate to rename pci_enable_link_state() to
> > > pci_set_default_link_state() to match the name to its functionality (and
> > > the function comment):
> > > 
> > >   * pci_enable_link_state - Clear and set the default device link state
> > > 
> > > Note: "the default ... link state".
> > > 
> > > 
> > > I've already raised this concern earlier! As you see, my comment are
> > > not getting addressed. I'd like to see the author does one of these:
> >
> > I replied to your comment on v3 patch[1], and I feel instead of having
> > new function() we can use same API to our purpose.

It's not about what "feels" something. One should clearly write down why
such conversion is correct/acceptable when it comes to existing callers 
if changing an existing API. The note should be such that it remains a 
permanent record for future (in the changelog).

I don't have answer to what are the expectations or intent of the existing 
callers. Convincing a patch is fine is responsibility of the one who is 
submitting the patch, not reviewer's.

Unfortunately, it is usually quite hard to figure out for existing drivers 
we're not familiar with. I'm not saying your "feel" is necessarily wrong, 
but the existing callers need to be properly investigated if you choose 
that path, not just handwaved over. It likely boils down if the 
->aspm_default and controlling it are useful features to have in the ASPM 
driver as your patch would take away that ability.

> You replied to Ilpo, but never got an agreement. Please try to close the
> discussions before posting next rev. If reviewers forgot to reply to your query,
> feel free to ping them in the same thread itself.
>
> > > 1) Renames pci_enable_link_state() to pci_set_default_link_state()
> > > 
> > > 1b) If pci_enable_link_state() is still needed after that, a new function
> > > is added to symmetrically pair with pci_disable_link_state().
> > > 
> > > or alternatively,
> > > 
> > > 2) Changelog justifies very clearly why this change is okay with the
> > > existing callers. (And obviously the function comment should be altered to
> > > match the functionality in that case too).
> > > 
> > > If approach 2 is chosen, it should be very carefully reviewed when it
> > > comes to the callers.
> > > 
> > I am in favor of approach 2 which you suggested, but lets wait for other
> > reviewers feedback on this. Based up on the response i will make
> > necessary changes in v5.
> > 
> 
> I would go for (1). It is always going to be a problem to change a legacy API
> like this. We might end up causing regressions. So it is safe to rename to
> reflect the purpose and try to come up with a new API that does what you want.
> If callers want to migrate to the new API, they can also do it in the future.

That's my recommendation as well.

-- 
 i.

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Krishna Chaitanya Chundru]

On 7/11/2025 2:51 PM, Ilpo Järvinen wrote:
> On Fri, 11 Jul 2025, Manivannan Sadhasivam wrote:
> 
>> On Wed, Jul 09, 2025 at 06:01:22PM GMT, Krishna Chaitanya Chundru wrote:
>>>
>>>
>>> On 7/9/2025 2:40 PM, Ilpo Järvinen wrote:
>>>> On Tue, 8 Jul 2025, Manivannan Sadhasivam wrote:
>>>>
>>>>> On Mon, Jun 09, 2025 at 04:21:27PM GMT, Krishna Chaitanya Chundru wrote:
>>>>>> ASPM states are not being enabled back with pci_enable_link_state() when
>>>>>> they are disabled by pci_disable_link_state(). This is because of the
>>>>>> aspm_disable flag is not getting cleared in pci_enable_link_state(), this
>>>>>> flag is being properly cleared when ASPM is controlled by sysfs.
>>>>>>
>>>>>
>>>>> A comment in pcie_config_aspm_link() says:
>>>>>
>>>>>    /* Enable only the states that were not explicitly disabled */
>>>>>
>>>>> But the function is called from both aspm_attr_store_common() and
>>>>> __pci_enable_link_state(). So I don't know if this is behavior is intentional
>>>>> or wrong.
>>>>
>>>> Hi,
>>>>
>>>> I think it's intentional. Whether the behavior is useful is another good
>>>> question but the current behavior aligns with the explanation in the
>>>> comment.
>>>>
>>>> My understanding of the situation is:
>>>>
>>>> pci_disable_link_state() and pci_enable_link_state() are not symmetric
>>>> despite the names, never have been (this is one of those many quirks ASPM
>>>> driver has which should be eventually cleaned up, IMO).
>>>>
>>>> It might be appropriate to rename pci_enable_link_state() to
>>>> pci_set_default_link_state() to match the name to its functionality (and
>>>> the function comment):
>>>>
>>>>    * pci_enable_link_state - Clear and set the default device link state
>>>>
>>>> Note: "the default ... link state".
>>>>
>>>>
>>>> I've already raised this concern earlier! As you see, my comment are
>>>> not getting addressed. I'd like to see the author does one of these:
>>>
>>> I replied to your comment on v3 patch[1], and I feel instead of having
>>> new function() we can use same API to our purpose.
> 
> It's not about what "feels" something. One should clearly write down why
> such conversion is correct/acceptable when it comes to existing callers
> if changing an existing API. The note should be such that it remains a
> permanent record for future (in the changelog).
> 
> I don't have answer to what are the expectations or intent of the existing
> callers. Convincing a patch is fine is responsibility of the one who is
> submitting the patch, not reviewer's.
> 
> Unfortunately, it is usually quite hard to figure out for existing drivers
> we're not familiar with. I'm not saying your "feel" is necessarily wrong,
> but the existing callers need to be properly investigated if you choose
> that path, not just handwaved over. It likely boils down if the
> ->aspm_default and controlling it are useful features to have in the ASPM
> driver as your patch would take away that ability.
> 
>> You replied to Ilpo, but never got an agreement. Please try to close the
>> discussions before posting next rev. If reviewers forgot to reply to your query,
>> feel free to ping them in the same thread itself.
>>
>>>> 1) Renames pci_enable_link_state() to pci_set_default_link_state()
>>>>
>>>> 1b) If pci_enable_link_state() is still needed after that, a new function
>>>> is added to symmetrically pair with pci_disable_link_state().
>>>>
>>>> or alternatively,
>>>>
>>>> 2) Changelog justifies very clearly why this change is okay with the
>>>> existing callers. (And obviously the function comment should be altered to
>>>> match the functionality in that case too).
>>>>
>>>> If approach 2 is chosen, it should be very carefully reviewed when it
>>>> comes to the callers.
>>>>
>>> I am in favor of approach 2 which you suggested, but lets wait for other
>>> reviewers feedback on this. Based up on the response i will make
>>> necessary changes in v5.
>>>
>>
>> I would go for (1). It is always going to be a problem to change a legacy API
>> like this. We might end up causing regressions. So it is safe to rename to
>> reflect the purpose and try to come up with a new API that does what you want.
>> If callers want to migrate to the new API, they can also do it in the future.
> 
> That's my recommendation as well.
I will take this as learning, we will go with approach (1) as both of
you are having valid points.

llpo,
In the previous patch you said you have some patches on can you send me
those or shall I proceed with patches from myside.

- Krishna Chaitanya.
> 

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]

On Fri, 11 Jul 2025, Krishna Chaitanya Chundru wrote:
> On 7/11/2025 2:51 PM, Ilpo Järvinen wrote:
> > On Fri, 11 Jul 2025, Manivannan Sadhasivam wrote:
> > > On Wed, Jul 09, 2025 at 06:01:22PM GMT, Krishna Chaitanya Chundru wrote:
> > > > On 7/9/2025 2:40 PM, Ilpo Järvinen wrote:

> > > > > 1) Renames pci_enable_link_state() to pci_set_default_link_state()
> > > > > 
> > > > > 1b) If pci_enable_link_state() is still needed after that, a new
> > > > > function
> > > > > is added to symmetrically pair with pci_disable_link_state().
> > > > > 
> > > > > or alternatively,
> > > > > 
> > > > > 2) Changelog justifies very clearly why this change is okay with the
> > > > > existing callers. (And obviously the function comment should be
> > > > > altered to
> > > > > match the functionality in that case too).
> > > > > 
> > > > > If approach 2 is chosen, it should be very carefully reviewed when it
> > > > > comes to the callers.
> > > > > 
> > > > I am in favor of approach 2 which you suggested, but lets wait for other
> > > > reviewers feedback on this. Based up on the response i will make
> > > > necessary changes in v5.
> > > > 
> > > 
> > > I would go for (1). It is always going to be a problem to change a legacy
> > > API
> > > like this. We might end up causing regressions. So it is safe to rename to
> > > reflect the purpose and try to come up with a new API that does what you
> > > want.
> > > If callers want to migrate to the new API, they can also do it in the
> > > future.
> > 
> > That's my recommendation as well.
> I will take this as learning, we will go with approach (1) as both of
> you are having valid points.
> 
> llpo,
> In the previous patch you said you have some patches on can you send me
> those or shall I proceed with patches from myside.

Sure, attached 3 patches.

Those were based on 6.11 and I've booted them along with my ASPM rework 
series (refactoring custom ASPM code in drivers to use the ASPM driver 
instaed). I quickly checked aspm.c fixes after that point and nothing 
seemed to require adaptations AFAICT.

-- 
 i.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-PCI-ASPM-Rename-pci_enable_link_state-to-pci_set_def.patch, Size: 7342 bytes --]

From 2885d4d3764e964b15adda01f66c516ce5ff2079 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 11 Jul 2025 16:28:24 +0300
Subject: [PATCH 1/3] PCI/ASPM: Rename pci_enable_link_state() to
 pci_set_default_link_state()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

pci_enable_link_state() and pci_disable_link_state() are not paired
symmetrically despite their names suggesting otherwise.
pci_enable_link_state() tweaks link state when the "default" policy is
in use rather than exactly "enabling" some link states. Obviously, when
the default policy is in use and the default link state is changed,
some link states may get enabled but that is a secondary effect.

Thus, rename pci_enable_link_state() to pci_set_default_link_state() to
better match what it does. The rename also frees
pci_enable_link_state() name so that a function that pairs
symmetrically with pci_disable_link_state() can be added later.

There's also pci_enable_link_state_locked() variant which is similarly
renamed to pci_set_default_link_state_locked().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c |  2 +-
 drivers/pci/controller/vmd.c           |  2 +-
 drivers/pci/pcie/aspm.c                | 29 +++++++++++++-------------
 include/linux/pci.h                    |  8 +++----
 4 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index c789e3f85655..37ef3ba0c20b 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1023,7 +1023,7 @@ static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
 	 * substates.
 	 */
 	pci_set_power_state_locked(pdev, PCI_D0);
-	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
+	pci_set_default_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
 
 	return 0;
 }
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 8df064b62a2f..f45e9e39c415 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -770,7 +770,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	 * PCIe r6.0, sec 5.5.4.
 	 */
 	pci_set_power_state_locked(pdev, PCI_D0);
-	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
+	pci_set_default_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
 	return 0;
 }
 
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 29fcb0689a91..33e9885c0210 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1432,7 +1432,7 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
 }
 EXPORT_SYMBOL(pci_disable_link_state);
 
-static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
+static int __pci_set_default_link_state(struct pci_dev *pdev, int state, bool locked)
 {
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
@@ -1465,8 +1465,8 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
 }
 
 /**
- * pci_enable_link_state - Clear and set the default device link state so that
- * the link may be allowed to enter the specified states. Note that if the
+ * pci_set_default_link_state - Clear and set the default device link state so
+ * that the link may be allowed to enter the specified states. Note that if the
  * BIOS didn't grant ASPM control to the OS, this does nothing because we can't
  * touch the LNKCTL register. Also note that this does not enable states
  * disabled by pci_disable_link_state(). Return 0 or a negative errno.
@@ -1477,18 +1477,19 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
  * @pdev: PCI device
  * @state: Mask of ASPM link states to enable
  */
-int pci_enable_link_state(struct pci_dev *pdev, int state)
+int pci_set_default_link_state(struct pci_dev *pdev, int state)
 {
-	return __pci_enable_link_state(pdev, state, false);
+	return __pci_set_default_link_state(pdev, state, false);
 }
-EXPORT_SYMBOL(pci_enable_link_state);
+EXPORT_SYMBOL(pci_set_default_link_state);
 
 /**
- * pci_enable_link_state_locked - Clear and set the default device link state
- * so that the link may be allowed to enter the specified states. Note that if
- * the BIOS didn't grant ASPM control to the OS, this does nothing because we
- * can't touch the LNKCTL register. Also note that this does not enable states
- * disabled by pci_disable_link_state(). Return 0 or a negative errno.
+ * pci_set_default_link_state_locked - Clear and set the default device link
+ * state so that the link may be allowed to enter the specified states. Note
+ * that if the BIOS didn't grant ASPM control to the OS, this does nothing
+ * because we can't touch the LNKCTL register. Also note that this does not
+ * enable states disabled by pci_disable_link_state(). Return 0 or a negative
+ * errno.
  *
  * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
  * PCIe r6.0, sec 5.5.4.
@@ -1498,13 +1499,13 @@ EXPORT_SYMBOL(pci_enable_link_state);
  *
  * Context: Caller holds pci_bus_sem read lock.
  */
-int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
+int pci_set_default_link_state_locked(struct pci_dev *pdev, int state)
 {
 	lockdep_assert_held_read(&pci_bus_sem);
 
-	return __pci_enable_link_state(pdev, state, true);
+	return __pci_set_default_link_state(pdev, state, true);
 }
-EXPORT_SYMBOL(pci_enable_link_state_locked);
+EXPORT_SYMBOL(pci_set_default_link_state_locked);
 
 static int pcie_aspm_set_policy(const char *val,
 				const struct kernel_param *kp)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f392..b8f60864ef81 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
 #ifdef CONFIG_PCIEASPM
 int pci_disable_link_state(struct pci_dev *pdev, int state);
 int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
-int pci_enable_link_state(struct pci_dev *pdev, int state);
-int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
+int pci_set_default_link_state(struct pci_dev *pdev, int state);
+int pci_set_default_link_state_locked(struct pci_dev *pdev, int state);
 void pcie_no_aspm(void);
 bool pcie_aspm_support_enabled(void);
 bool pcie_aspm_enabled(struct pci_dev *pdev);
@@ -1836,9 +1836,9 @@ static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
 { return 0; }
 static inline int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
 { return 0; }
-static inline int pci_enable_link_state(struct pci_dev *pdev, int state)
+static inline int pci_set_default_link_state(struct pci_dev *pdev, int state)
 { return 0; }
-static inline int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
+static inline int pci_set_default_link_state_locked(struct pci_dev *pdev, int state)
 { return 0; }
 static inline void pcie_no_aspm(void) { }
 static inline bool pcie_aspm_support_enabled(void) { return false; }

base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.39.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name=0002-PCI-ASPM-Improve-pci_set_default_link_state-_locked-.patch, Size: 3769 bytes --]

From 68bf37222e176201f478cbae12ba4a2e2d6329af Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 11 Jul 2025 16:28:25 +0300
Subject: [PATCH 2/3] PCI/ASPM: Improve pci_set_default_link_state{,_locked}()
 kerneldoc
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Improve pci_set_default_link_state{,_locked}() documentation:

- Note the link state may get changed if the default policy is in use
- Better follow kerneldoc formatting guidelines (separate description
  block and return entries)

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/aspm.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 33e9885c0210..5721ebfdea71 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1465,17 +1465,21 @@ static int __pci_set_default_link_state(struct pci_dev *pdev, int state, bool lo
 }
 
 /**
- * pci_set_default_link_state - Clear and set the default device link state so
- * that the link may be allowed to enter the specified states. Note that if the
- * BIOS didn't grant ASPM control to the OS, this does nothing because we can't
- * touch the LNKCTL register. Also note that this does not enable states
- * disabled by pci_disable_link_state(). Return 0 or a negative errno.
+ * pci_set_default_link_state - Set the default device link state
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ *
+ * Set the default device link state so that the link may be allowed to
+ * enter the specified states. If the default policy is in use, the link
+ * state may also be updated to reflect the new default link state. Note
+ * that if the BIOS didn't grant ASPM control to the OS, this does nothing
+ * because we can't touch the LNKCTL register. Also note that this does not
+ * enable states disabled by pci_disable_link_state().
  *
  * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
  * PCIe r6.0, sec 5.5.4.
  *
- * @pdev: PCI device
- * @state: Mask of ASPM link states to enable
+ * Return: 0 or a negative errno.
  */
 int pci_set_default_link_state(struct pci_dev *pdev, int state)
 {
@@ -1484,20 +1488,23 @@ int pci_set_default_link_state(struct pci_dev *pdev, int state)
 EXPORT_SYMBOL(pci_set_default_link_state);
 
 /**
- * pci_set_default_link_state_locked - Clear and set the default device link
- * state so that the link may be allowed to enter the specified states. Note
+ * pci_set_default_link_state_locked - Set the default device link state
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ *
+ * Set the default device link state so that the link may be allowed to
+ * enter the specified states. If the default policy is in use, the link
+ * state may also be updated to reflect the new default link state. Note
  * that if the BIOS didn't grant ASPM control to the OS, this does nothing
  * because we can't touch the LNKCTL register. Also note that this does not
- * enable states disabled by pci_disable_link_state(). Return 0 or a negative
- * errno.
+ * enable states disabled by pci_disable_link_state().
+ *
+ * Context: Caller holds pci_bus_sem read lock.
  *
  * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
  * PCIe r6.0, sec 5.5.4.
  *
- * @pdev: PCI device
- * @state: Mask of ASPM link states to enable
- *
- * Context: Caller holds pci_bus_sem read lock.
+ * Return: 0 or a negative errno.
  */
 int pci_set_default_link_state_locked(struct pci_dev *pdev, int state)
 {
-- 
2.39.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Type: text/x-diff; name=0003-PCI-ASPM-Add-pci_enable_link_state.patch, Size: 3428 bytes --]

From d5ef8cf951dd135e76deb32d1c315fe95d1f4c91 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 11 Jul 2025 16:28:26 +0300
Subject: [PATCH 3/3] PCI/ASPM: Add pci_enable_link_state()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

pci_disable_link_state() lacks a symmetric pair. Some drivers want to
disable ASPM during certain phases of their operation but then
re-enable it later on. If pci_disable_link_state() is made for the
device, there is currently no way to re-enable the states that were
disabled.

Add pci_enable_link_state() to remove ASPM states from the state
disable mask.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/aspm.c | 43 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |  1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 5721ebfdea71..348bd79f049f 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1432,6 +1432,49 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
 }
 EXPORT_SYMBOL(pci_disable_link_state);
 
+/**
+ * pci_enable_link_state - Re-enable device's link state
+ * @pdev: PCI device
+ * @state: ASPM link states to re-enable
+ *
+ * Enable device's link state that were previously disable so the link is
+ * allowed to enter the specific states. Note that if the BIOS didn't grant
+ * ASPM control to the OS, this does nothing because we can't touch the
+ * LNKCTL register.
+ *
+ * Return: 0 or a negative errno.
+ */
+int pci_enable_link_state(struct pci_dev *pdev, int state)
+{
+	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
+
+	if (!link)
+		return -EINVAL;
+	/*
+	 * A driver requested that ASPM be enabled on this device, but
+	 * if we don't have permission to manage ASPM (e.g., on ACPI
+	 * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and
+	 * the _OSC method), we can't honor that request.
+	 */
+	if (aspm_disabled) {
+		pci_warn(pdev, "can't enable ASPM; OS doesn't have ASPM control\n");
+		return -EPERM;
+	}
+
+	mutex_lock(&aspm_lock);
+	/* Use the disable mask variant because it relates to aspm_disable */
+	link->aspm_disable &= ~pci_calc_aspm_disable_mask(state);
+	pcie_config_aspm_link(link, policy_to_aspm_state(link));
+
+	if (state & PCIE_LINK_STATE_CLKPM)
+		link->clkpm_disable = 0;
+	pcie_set_clkpm(link, policy_to_clkpm_state(link));
+	mutex_unlock(&aspm_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(pci_enable_link_state);
+
 static int __pci_set_default_link_state(struct pci_dev *pdev, int state, bool locked)
 {
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b8f60864ef81..d566d86e6368 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1826,6 +1826,7 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
 #ifdef CONFIG_PCIEASPM
 int pci_disable_link_state(struct pci_dev *pdev, int state);
 int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
+int pci_enable_link_state(struct pci_dev *pdev, int state);
 int pci_set_default_link_state(struct pci_dev *pdev, int state);
 int pci_set_default_link_state_locked(struct pci_dev *pdev, int state);
 void pcie_no_aspm(void);
-- 
2.39.5

Re: [PATCH v4 08/11] PCI: qcom: Add support for PCIe pre/post_link_speed_change()

[Bjorn Helgaas]

On Mon, Jun 09, 2025 at 04:21:29PM +0530, Krishna Chaitanya Chundru wrote:
> QCOM PCIe controllers need to disable ASPM before initiating link
> re-train. So as part of pre_link_speed_change() disable ASPM and as
> part of post_link_speed_change() enable ASPM back.

Is this a QCOM defect?  Or is there something in the PCIe spec about
needing to disable ASPM during retrain?  What about
pcie_retrain_link()?  Does that work on QCOM?

> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -276,10 +276,16 @@ struct qcom_pcie {
>  	struct dentry *debugfs;
>  	bool suspended;
>  	bool use_pm_opp;
> +	int aspm_state; /* Store ASPM state used in pre & post link speed change */

Whatever this is, it's definitely not an int.  Some kind of unsigned
thing of specified size, at least.

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Bjorn Helgaas]

On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya Chundru wrote:
> If the driver wants to move to higher data rate/speed than the current data
> rate then the controller driver may need to change certain votes so that
> link may come up at requested data rate/speed like QCOM PCIe controllers
> need to change their RPMh (Resource Power Manager-hardened) state. Once
> link retraining is done controller drivers needs to adjust their votes
> based on the final data rate.
> 
> Some controllers also may need to update their bandwidth voting like
> ICC BW votings etc.
> 
> So, add pre_link_speed_change() & post_link_speed_change() op to call
> before & after the link re-train. There is no explicit locking mechanisms
> as these are called by a single client Endpoint driver.
> 
> In case of PCIe switch, if there is a request to change target speed for a
> downstream port then no need to call these function ops as these are
> outside the scope of the controller drivers.

> +++ b/include/linux/pci.h
> @@ -599,6 +599,24 @@ struct pci_host_bridge {
>  	void (*release_fn)(struct pci_host_bridge *);
>  	int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>  	void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
> +	/*
> +	 * Callback to the host bridge drivers to update ICC BW votes, clock
> +	 * frequencies etc.. for the link re-train to come up in targeted speed.
> +	 * These are intended to be called by devices directly attached to the
> +	 * Root Port. These are called by a single client Endpoint driver, so
> +	 * there is no need for explicit locking mechanisms.
> +	 */
> +	int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
> +				     struct pci_dev *dev, int speed);
> +	/*
> +	 * Callback to the host bridge drivers to adjust ICC BW votes, clock
> +	 * frequencies etc.. to the updated speed after link re-train. These
> +	 * are intended to be called by devices directly attached to the
> +	 * Root Port. These are called by a single client Endpoint driver,
> +	 * so there is no need for explicit locking mechanisms.

No need to repeat the entire comment.  s/.././

These pointers feel awfully specific for being in struct
pci_host_bridge, since we only need them for a questionable QCOM
controller.  I think this needs to be pushed down into qcom somehow as
some kind of quirk.

> +	 */
> +	void (*post_link_speed_change)(struct pci_host_bridge *bridge,
> +				       struct pci_dev *dev, int speed);

Re: [PATCH v4 10/11] PCI: Add function to convert lnkctl2speed to pci_bus_speed

[Bjorn Helgaas]

On Mon, Jun 09, 2025 at 04:21:31PM +0530, Krishna Chaitanya Chundru wrote:
> Add a exported function to convert lnkctl2speed to enum pci_bus_speed,
> so that other kernel drivers can use it.

Name the function explicitly in subject and commit log.

> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/pci.c   | 12 ++++++++++++
>  include/linux/pci.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e9448d55113bdfd2263d8e2f6b3ec802f56b712e..8950e88826e27accfe699e31fba8f4077c26296f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6018,6 +6018,18 @@ int pcie_link_speed_mbps(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL(pcie_link_speed_mbps);
>  
> +/**
> + * pci_lnkctl2_bus_speed - convert lnkctl2 speed to pci_bus_speed
> + * @speed: LNKCAP2 SLS value
> + *
> + * Return: pci_bus_speed
> + */
> +enum pci_bus_speed pci_lnkctl2_bus_speed(int speed)
> +{
> +	return pcie_link_speed[speed];
> +}
> +EXPORT_SYMBOL(pci_lnkctl2_bus_speed);

You mention both "lnkctl2" and "LNKCAP2" and I don't know whether you
mean Link Capabilities 2 or Link Control 2, and I don't know what the
bits passed in are.

I expect something like FIELD_GET(PCI_EXP_LNKCAP2_SLS, value) here
that tells us clearly where it came from, so the caller isn't
responsible for extracting it.  And it takes some kind of unsigned
value, probably a u32 if it came from Link Capabilities 2.

Bjorn

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Bjorn Helgaas]

On Fri, Jul 11, 2025 at 04:38:48PM +0300, Ilpo Järvinen wrote:

> +++ b/include/linux/pci.h
> @@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
>  #ifdef CONFIG_PCIEASPM
>  int pci_disable_link_state(struct pci_dev *pdev, int state);
>  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> -int pci_enable_link_state(struct pci_dev *pdev, int state);

AFAICT there's no caller of this at all.  Why do we keep it?

> -int pci_enable_link_state_locked(struct pci_dev *pdev, int state);

We only have two callers of this (pcie-qcom.c and vmd.c, both in
drivers/pci/), so it's not clear to me that it needs to be in
include/linux/pci.h.

I'm a little dubious about it in the first place since I don't think
drivers should be enabling ASPM states on their own, but pcie-qcom.c
and vmd.c are PCIe controller drivers, not PCI device drivers, so I
guess we can live with them for now.

IMO the "someday" goal should be that we get rid of aspm_policy and
enable all the available power saving states by default.  We have
sysfs knobs that administrators can use if necessary, and drivers or
quirks can disable states if they need to work around hardware
defects.

I think the compiled-in aspm_policy default and the module parameters
are basically chicken switches that only exist because aspm.c and some
devices aren't robust enough.

Bjorn

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Bjorn Helgaas]

On Mon, Jun 09, 2025 at 04:21:27PM +0530, Krishna Chaitanya Chundru wrote:
> ASPM states are not being enabled back with pci_enable_link_state() when
> they are disabled by pci_disable_link_state(). This is because of the
> aspm_disable flag is not getting cleared in pci_enable_link_state(), this
> flag is being properly cleared when ASPM is controlled by sysfs.

Mention the name of the function where this happens for sysfs so we
can easily compare them.

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Krishna Chaitanya Chundru]

On 7/12/2025 3:06 AM, Bjorn Helgaas wrote:
> On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya Chundru wrote:
>> If the driver wants to move to higher data rate/speed than the current data
>> rate then the controller driver may need to change certain votes so that
>> link may come up at requested data rate/speed like QCOM PCIe controllers
>> need to change their RPMh (Resource Power Manager-hardened) state. Once
>> link retraining is done controller drivers needs to adjust their votes
>> based on the final data rate.
>>
>> Some controllers also may need to update their bandwidth voting like
>> ICC BW votings etc.
>>
>> So, add pre_link_speed_change() & post_link_speed_change() op to call
>> before & after the link re-train. There is no explicit locking mechanisms
>> as these are called by a single client Endpoint driver.
>>
>> In case of PCIe switch, if there is a request to change target speed for a
>> downstream port then no need to call these function ops as these are
>> outside the scope of the controller drivers.
> 
>> +++ b/include/linux/pci.h
>> @@ -599,6 +599,24 @@ struct pci_host_bridge {
>>   	void (*release_fn)(struct pci_host_bridge *);
>>   	int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>>   	void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>> +	/*
>> +	 * Callback to the host bridge drivers to update ICC BW votes, clock
>> +	 * frequencies etc.. for the link re-train to come up in targeted speed.
>> +	 * These are intended to be called by devices directly attached to the
>> +	 * Root Port. These are called by a single client Endpoint driver, so
>> +	 * there is no need for explicit locking mechanisms.
>> +	 */
>> +	int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
>> +				     struct pci_dev *dev, int speed);
>> +	/*
>> +	 * Callback to the host bridge drivers to adjust ICC BW votes, clock
>> +	 * frequencies etc.. to the updated speed after link re-train. These
>> +	 * are intended to be called by devices directly attached to the
>> +	 * Root Port. These are called by a single client Endpoint driver,
>> +	 * so there is no need for explicit locking mechanisms.
> 
> No need to repeat the entire comment.  s/.././
> 
> These pointers feel awfully specific for being in struct
> pci_host_bridge, since we only need them for a questionable QCOM
> controller.  I think this needs to be pushed down into qcom somehow as
> some kind of quirk.
> 
Currently these are needed by QCOM controllers, but it may also needed
by other controllers may also need these for updating ICC votes, any
system level votes, clock frequencies etc.
QCOM controllers is also doing one extra step in these functions to
disable and enable ASPM only as it cannot link speed change support
with ASPM enabled.

- Krishna Chaitanya.
>> +	 */
>> +	void (*post_link_speed_change)(struct pci_host_bridge *bridge,
>> +				       struct pci_dev *dev, int speed);

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Krishna Chaitanya Chundru]

On 7/12/2025 4:32 AM, Bjorn Helgaas wrote:
> On Mon, Jun 09, 2025 at 04:21:27PM +0530, Krishna Chaitanya Chundru wrote:
>> ASPM states are not being enabled back with pci_enable_link_state() when
>> they are disabled by pci_disable_link_state(). This is because of the
>> aspm_disable flag is not getting cleared in pci_enable_link_state(), this
>> flag is being properly cleared when ASPM is controlled by sysfs.
> 
> Mention the name of the function where this happens for sysfs so we
> can easily compare them.
it is aspm_attr_store_common()[1], it will be used for all ASPM sysfs

[1] 
https://elixir.bootlin.com/linux/v6.16-rc5/source/drivers/pci/pcie/aspm.c#L1595

- Krishna Chaitanya.

Re: [PATCH v4 08/11] PCI: qcom: Add support for PCIe pre/post_link_speed_change()

[Krishna Chaitanya Chundru]

On 7/12/2025 2:59 AM, Bjorn Helgaas wrote:
> On Mon, Jun 09, 2025 at 04:21:29PM +0530, Krishna Chaitanya Chundru wrote:
>> QCOM PCIe controllers need to disable ASPM before initiating link
>> re-train. So as part of pre_link_speed_change() disable ASPM and as
>> part of post_link_speed_change() enable ASPM back.
> 
> Is this a QCOM defect?  Or is there something in the PCIe spec about
This is QCOM issue only.
> needing to disable ASPM during retrain?  What about
> pcie_retrain_link()?  Does that work on QCOM?
After disabling ASPM will work pcie_retrain_link().

- Krishna Chaitanya.
> 
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -276,10 +276,16 @@ struct qcom_pcie {
>>   	struct dentry *debugfs;
>>   	bool suspended;
>>   	bool use_pm_opp;
>> +	int aspm_state; /* Store ASPM state used in pre & post link speed change */
> 
> Whatever this is, it's definitely not an int.  Some kind of unsigned
> thing of specified size, at least.

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Manivannan Sadhasivam]

On Fri, Jul 11, 2025 at 06:00:13PM GMT, Bjorn Helgaas wrote:
> On Fri, Jul 11, 2025 at 04:38:48PM +0300, Ilpo Järvinen wrote:
> 
> > +++ b/include/linux/pci.h
> > @@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
> >  #ifdef CONFIG_PCIEASPM
> >  int pci_disable_link_state(struct pci_dev *pdev, int state);
> >  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> > -int pci_enable_link_state(struct pci_dev *pdev, int state);
> 
> AFAICT there's no caller of this at all.  Why do we keep it?
> 

I'm just working on a series to convert the ath{10/11/12}k drivers to use this
API instead of modifying LNKCTL register directly:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/ath12k/pci.c#n961

> > -int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
> 
> We only have two callers of this (pcie-qcom.c and vmd.c, both in
> drivers/pci/), so it's not clear to me that it needs to be in
> include/linux/pci.h.
> 
> I'm a little dubious about it in the first place since I don't think
> drivers should be enabling ASPM states on their own, but pcie-qcom.c
> and vmd.c are PCIe controller drivers, not PCI device drivers, so I
> guess we can live with them for now.
> 
> IMO the "someday" goal should be that we get rid of aspm_policy and
> enable all the available power saving states by default.  We have
> sysfs knobs that administrators can use if necessary, and drivers or
> quirks can disable states if they need to work around hardware
> defects.
> 

Yeah, I think the default should be powersave and let the users disable it for
performance if they want.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Hans Zhang]

On 2025/7/12 17:35, Manivannan Sadhasivam wrote:
>> We only have two callers of this (pcie-qcom.c and vmd.c, both in
>> drivers/pci/), so it's not clear to me that it needs to be in
>> include/linux/pci.h.
>>
>> I'm a little dubious about it in the first place since I don't think
>> drivers should be enabling ASPM states on their own, but pcie-qcom.c
>> and vmd.c are PCIe controller drivers, not PCI device drivers, so I
>> guess we can live with them for now.
>>
>> IMO the "someday" goal should be that we get rid of aspm_policy and
>> enable all the available power saving states by default.  We have
>> sysfs knobs that administrators can use if necessary, and drivers or
>> quirks can disable states if they need to work around hardware
>> defects.
>>
> 
> Yeah, I think the default should be powersave and let the users disable it for
> performance if they want.
> 

Dear Bjorn and Mani,

Perhaps I don't think so. At present, our company's testing team has 
tested quite a few NVMe SSDS. As far as I can remember, the SSDS from 
two companies have encountered problems and will hang directly when 
turned on. We have set CONFIG_PCIEASPM_POWERSAVE=y by default. When 
encountering SSDS from these two companies, we had to add 
"pcie_aspm.policy=default" in the cmdline, and then the boot worked 
normally. Currently, we do not have a PCIe protocol analyzer to analyze 
such issues. The current approach is to modify the cmdline. So I can't 
prove whether it's a problem with the Root Port of our SOC or the SSD 
device.

Here I agree with Bjorn's statement that sometimes the EP is not 
necessarily very standard and there are no hardware issues. Personally, 
I think the default is default or performance. When users need to save 
power, they should then decide whether to configure it as powersave or 
powersupersave. Sometimes, if the EP device connected by the customer is 
perfect, they can turn it on to save power. But if the EP is not 
perfect, at least they will immediately know what caused the problem.

I wonder if there are others who have encountered similar problems as 
well. If I say anything wrong, please point it out. Thank you.

Best regards,
Hans

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Manivannan Sadhasivam]

On Sun, Jul 13, 2025 at 12:05:18AM GMT, Hans Zhang wrote:
> 
> 
> On 2025/7/12 17:35, Manivannan Sadhasivam wrote:
> > > We only have two callers of this (pcie-qcom.c and vmd.c, both in
> > > drivers/pci/), so it's not clear to me that it needs to be in
> > > include/linux/pci.h.
> > > 
> > > I'm a little dubious about it in the first place since I don't think
> > > drivers should be enabling ASPM states on their own, but pcie-qcom.c
> > > and vmd.c are PCIe controller drivers, not PCI device drivers, so I
> > > guess we can live with them for now.
> > > 
> > > IMO the "someday" goal should be that we get rid of aspm_policy and
> > > enable all the available power saving states by default.  We have
> > > sysfs knobs that administrators can use if necessary, and drivers or
> > > quirks can disable states if they need to work around hardware
> > > defects.
> > > 
> > 
> > Yeah, I think the default should be powersave and let the users disable it for
> > performance if they want.
> > 
> 
> Dear Bjorn and Mani,
> 
> Perhaps I don't think so. At present, our company's testing team has tested
> quite a few NVMe SSDS. As far as I can remember, the SSDS from two companies
> have encountered problems and will hang directly when turned on. We have set
> CONFIG_PCIEASPM_POWERSAVE=y by default. When encountering SSDS from these
> two companies, we had to add "pcie_aspm.policy=default" in the cmdline, and
> then the boot worked normally. Currently, we do not have a PCIe protocol
> analyzer to analyze such issues. The current approach is to modify the
> cmdline. So I can't prove whether it's a problem with the Root Port of our
> SOC or the SSD device.
> 
> Here I agree with Bjorn's statement that sometimes the EP is not necessarily
> very standard and there are no hardware issues. Personally, I think the
> default is default or performance. When users need to save power, they
> should then decide whether to configure it as powersave or powersupersave.
> Sometimes, if the EP device connected by the customer is perfect, they can
> turn it on to save power. But if the EP is not perfect, at least they will
> immediately know what caused the problem.
> 

We all agree that not all endpoints are standards compliant. So if they have any
issues with ASPM, then ASPM for those devices should be disabled in the quirks
or in the device driver.

That said, the change that Bjorn proposed is not going to happen in the
immediate future.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]

On Sat, 12 Jul 2025, Manivannan Sadhasivam wrote:
> On Fri, Jul 11, 2025 at 06:00:13PM GMT, Bjorn Helgaas wrote:
> > On Fri, Jul 11, 2025 at 04:38:48PM +0300, Ilpo Järvinen wrote:
> > 
> > > +++ b/include/linux/pci.h
> > > @@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
> > >  #ifdef CONFIG_PCIEASPM
> > >  int pci_disable_link_state(struct pci_dev *pdev, int state);
> > >  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> > > -int pci_enable_link_state(struct pci_dev *pdev, int state);
> > 
> > AFAICT there's no caller of this at all.  Why do we keep it?
> > 
> 
> I'm just working on a series to convert the ath{10/11/12}k drivers to use this
> API instead of modifying LNKCTL register directly:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/ath12k/pci.c#n961

Great. I assume but "this API" you meant disable/enable link state that 
are real pair unlike the current pci_enable_link_state()?

Did ath1xk need to do some hw specific register updates when changing ASPM 
state?

I tried to do similar conversion in r8169 (and actually also ath1xk too) 
but it was a while ago already. If I understood the code correctly, r8169 
seems to write some HW specific registers when changing ASPM state so I 
would have likely need to add some ops for it to play nice with state 
changes not originating from the driver itself but from the ASPM driver, 
which is where the work then stalled.

> > > -int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
> > 
> > We only have two callers of this (pcie-qcom.c and vmd.c, both in
> > drivers/pci/), so it's not clear to me that it needs to be in
> > include/linux/pci.h.
> > 
> > I'm a little dubious about it in the first place since I don't think
> > drivers should be enabling ASPM states on their own, but pcie-qcom.c
> > and vmd.c are PCIe controller drivers, not PCI device drivers, so I
> > guess we can live with them for now.
> > 
> > IMO the "someday" goal should be that we get rid of aspm_policy and
> > enable all the available power saving states by default.  We have
> > sysfs knobs that administrators can use if necessary, and drivers or
> > quirks can disable states if they need to work around hardware
> > defects.
> 
> Yeah, I think the default should be powersave and let the users disable it for
> performance if they want.

I'm certainly not against improvements in this front, but I think we need 
to get rid off custom ASPM disable code from the drivers first.

-- 
 i.

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

On Fri, 11 Jul 2025, Bjorn Helgaas wrote:

> On Fri, Jul 11, 2025 at 04:38:48PM +0300, Ilpo Järvinen wrote:
> 
> > +++ b/include/linux/pci.h
> > @@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
> >  #ifdef CONFIG_PCIEASPM
> >  int pci_disable_link_state(struct pci_dev *pdev, int state);
> >  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> > -int pci_enable_link_state(struct pci_dev *pdev, int state);
> 
> AFAICT there's no caller of this at all.  Why do we keep it?

It was added to match the disable side despite not having users. I don't 
oppose dropping the unused one.

> > -int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
> 
> We only have two callers of this (pcie-qcom.c and vmd.c, both in
> drivers/pci/), so it's not clear to me that it needs to be in
> include/linux/pci.h.
>
> I'm a little dubious about it in the first place since I don't think
> drivers should be enabling ASPM states on their own, but pcie-qcom.c
> and vmd.c are PCIe controller drivers, not PCI device drivers, so I
> guess we can live with them for now.

There seem to be some drivers which have issues if certain ASPM states are 
enabled during some phase of operation, so they'd want to disable ASPM 
for a while and re-enable it after past the danger zone, which is why I 
had to create the symmetric pair for disabling states in a series trying 
to remove custom ASPM code from other drivers (these patches are extracted 
from that series). Currently those drivers mess with LNKCTL directly.

(The driver changes we not finished because it seemed I'd have needed to 
add some ops to allow writing HW specific registers of ASPM state change 
which the ASPM driver could invoke to infor the driver about state 
changes.)

> IMO the "someday" goal should be that we get rid of aspm_policy and
> enable all the available power saving states by default.  We have
> sysfs knobs that administrators can use if necessary, and drivers or
> quirks can disable states if they need to work around hardware
> defects.
>
> I think the compiled-in aspm_policy default and the module parameters
> are basically chicken switches that only exist because aspm.c and some
> devices aren't robust enough.

There's also too much custom code in drivers currently.

-- 
 i.

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Manivannan Sadhasivam]

On Sun, Jul 13, 2025 at 07:27:57PM GMT, Ilpo Järvinen wrote:
> On Sat, 12 Jul 2025, Manivannan Sadhasivam wrote:
> > On Fri, Jul 11, 2025 at 06:00:13PM GMT, Bjorn Helgaas wrote:
> > > On Fri, Jul 11, 2025 at 04:38:48PM +0300, Ilpo Järvinen wrote:
> > > 
> > > > +++ b/include/linux/pci.h
> > > > @@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
> > > >  #ifdef CONFIG_PCIEASPM
> > > >  int pci_disable_link_state(struct pci_dev *pdev, int state);
> > > >  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> > > > -int pci_enable_link_state(struct pci_dev *pdev, int state);
> > > 
> > > AFAICT there's no caller of this at all.  Why do we keep it?
> > > 
> > 
> > I'm just working on a series to convert the ath{10/11/12}k drivers to use this
> > API instead of modifying LNKCTL register directly:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/ath12k/pci.c#n961
> 
> Great. I assume but "this API" you meant disable/enable link state that 
> are real pair unlike the current pci_enable_link_state()?
> 

Yes. I actually based my series on top of this series, but I guess I should just
merge the API change patch to my series as it is relatively small compared to
this one (so it will get merged quicker).

> Did ath1xk need to do some hw specific register updates when changing ASPM 
> state?
> 

Fortunately, no.

> I tried to do similar conversion in r8169 (and actually also ath1xk too) 
> but it was a while ago already. If I understood the code correctly, r8169 
> seems to write some HW specific registers when changing ASPM state so I 
> would have likely need to add some ops for it to play nice with state 
> changes not originating from the driver itself but from the ASPM driver, 
> which is where the work then stalled.
> 

ath driver changes are straightforward, but I need to add an inline function to
convert lnkctl setting to aspm states:

static inline int ath_pci_aspm_state(u16 lnkctl)
{
	int state = 0;

	if (lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
		state |= PCIE_LINK_STATE_L0S;
	if (lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
		state |= PCIE_LINK_STATE_L1;

	return state;
}

Currently, it is in ath/ath.h, but if you feel that we should move it to
include/linux/pci.h, let me know!

> > > > -int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
> > > 
> > > We only have two callers of this (pcie-qcom.c and vmd.c, both in
> > > drivers/pci/), so it's not clear to me that it needs to be in
> > > include/linux/pci.h.
> > > 
> > > I'm a little dubious about it in the first place since I don't think
> > > drivers should be enabling ASPM states on their own, but pcie-qcom.c
> > > and vmd.c are PCIe controller drivers, not PCI device drivers, so I
> > > guess we can live with them for now.
> > > 
> > > IMO the "someday" goal should be that we get rid of aspm_policy and
> > > enable all the available power saving states by default.  We have
> > > sysfs knobs that administrators can use if necessary, and drivers or
> > > quirks can disable states if they need to work around hardware
> > > defects.
> > 
> > Yeah, I think the default should be powersave and let the users disable it for
> > performance if they want.
> 
> I'm certainly not against improvements in this front, but I think we need 
> to get rid off custom ASPM disable code from the drivers first.
> 

It would be difficult IMO as drivers have their own usecases. For instance, the
ath drivers need to disable ASPM states to avoid issues during firmware
download. Then they need to enable ASPM back once the firmware is up and
running.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Bjorn Helgaas]

On Sat, Jul 12, 2025 at 03:05:28PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jul 11, 2025 at 06:00:13PM GMT, Bjorn Helgaas wrote:
> > On Fri, Jul 11, 2025 at 04:38:48PM +0300, Ilpo Järvinen wrote:
> > 
> > > +++ b/include/linux/pci.h
> > > @@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
> > >  #ifdef CONFIG_PCIEASPM
> > >  int pci_disable_link_state(struct pci_dev *pdev, int state);
> > >  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> > > -int pci_enable_link_state(struct pci_dev *pdev, int state);
> > 
> > AFAICT there's no caller of this at all.  Why do we keep it?
> 
> I'm just working on a series to convert the ath{10/11/12}k drivers to use this
> API instead of modifying LNKCTL register directly:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/ath12k/pci.c#n961

Awesome!  It will be great to get more of the ASPM gunk out of
drivers.

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Bjorn Helgaas]

On Sun, Jul 13, 2025 at 12:05:18AM +0800, Hans Zhang wrote:
> On 2025/7/12 17:35, Manivannan Sadhasivam wrote:
> ...

> > > IMO the "someday" goal should be that we get rid of aspm_policy
> > > and enable all the available power saving states by default.  We
> > > have sysfs knobs that administrators can use if necessary, and
> > > drivers or quirks can disable states if they need to work around
> > > hardware defects.
> > 
> > Yeah, I think the default should be powersave and let the users
> > disable it for performance if they want.
> 
> Perhaps I don't think so. At present, our company's testing team has
> tested quite a few NVMe SSDS. As far as I can remember, the SSDS
> from two companies have encountered problems and will hang directly
> when turned on. We have set CONFIG_PCIEASPM_POWERSAVE=y by default.
> When encountering SSDS from these two companies, we had to add
> "pcie_aspm.policy=default" in the cmdline, and then the boot worked
> normally. Currently, we do not have a PCIe protocol analyzer to
> analyze such issues. The current approach is to modify the cmdline.
> So I can't prove whether it's a problem with the Root Port of our
> SOC or the SSD device.

Have you reported these?

> Here I agree with Bjorn's statement that sometimes the EP is not
> necessarily very standard and there are no hardware issues.
> Personally, I think the default is default or performance. When
> users need to save power, they should then decide whether to
> configure it as powersave or powersupersave.  Sometimes, if the EP
> device connected by the customer is perfect, they can turn it on to
> save power. But if the EP is not perfect, at least they will
> immediately know what caused the problem.

We should discover device defects as early as possible so we can add
quirks for them.  Defaulting to ASPM being partly disabled means it
gets much less testing and users end up passing around "fixes" like
booting with "pcie_aspm.policy=default" or similar.  I do not want
users to trip over a device that doesn't work and have to look for
workarounds on the web.

I also think it's somewhat irresponsible of us to consume more power
than necessary.  But as Mani said, this would be a big change and
might have to be done with a BIOS date check or something to try to
avoid regressions.

Bjorn

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Bjorn Helgaas]

On Mon, Jul 14, 2025 at 07:21:21PM +0530, Manivannan Sadhasivam wrote:
> On Sun, Jul 13, 2025 at 07:27:57PM GMT, Ilpo Järvinen wrote:
> > On Sat, 12 Jul 2025, Manivannan Sadhasivam wrote:
> > > On Fri, Jul 11, 2025 at 06:00:13PM GMT, Bjorn Helgaas wrote:
> > > > On Fri, Jul 11, 2025 at 04:38:48PM +0300, Ilpo Järvinen wrote:
> > > > 
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
> > > > >  #ifdef CONFIG_PCIEASPM
> > > > >  int pci_disable_link_state(struct pci_dev *pdev, int state);
> > > > >  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> > > > > -int pci_enable_link_state(struct pci_dev *pdev, int state);
> > > > 
> > > > AFAICT there's no caller of this at all.  Why do we keep it?
> > > 
> > > I'm just working on a series to convert the ath{10/11/12}k
> > > drivers to use this API instead of modifying LNKCTL register
> > > directly:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/ath12k/pci.c#n961
> > 
> > Great. I assume but "this API" you meant disable/enable link state
> > that are real pair unlike the current pci_enable_link_state()?
> 
> Yes. I actually based my series on top of this series, but I guess I
> should just merge the API change patch to my series as it is
> relatively small compared to this one (so it will get merged
> quicker).
> 
> > Did ath1xk need to do some hw specific register updates when
> > changing ASPM state?
> 
> Fortunately, no.
> 
> > I tried to do similar conversion in r8169 (and actually also
> > ath1xk too) but it was a while ago already. If I understood the
> > code correctly, r8169 seems to write some HW specific registers
> > when changing ASPM state so I would have likely need to add some
> > ops for it to play nice with state changes not originating from
> > the driver itself but from the ASPM driver, which is where the
> > work then stalled.

Pffft.  Device-specific configuration when changing ASPM state is a
device defect in my book.  ASPM is clearly designed to be managed
outside the driver.  What is the driver supposed to do about this if
the platform retains control of ASPM?  There's no possibility of a
driver hook for that.

> ath driver changes are straightforward, but I need to add an inline
> function to convert lnkctl setting to aspm states:
> 
> static inline int ath_pci_aspm_state(u16 lnkctl)
> {
> 	int state = 0;
> 
> 	if (lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
> 		state |= PCIE_LINK_STATE_L0S;
> 	if (lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
> 		state |= PCIE_LINK_STATE_L1;
> 
> 	return state;
> }
> 
> Currently, it is in ath/ath.h, but if you feel that we should move it to
> include/linux/pci.h, let me know!

I haven't looked at the implementation here, but I don't think drivers
should check LNKCTL to decide this.  If the device doesn't work with
L0s or L1 or whatever, that's what the driver should tell us.

Bjorn

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Hans Zhang]

On 2025/7/15 03:32, Bjorn Helgaas wrote:
> On Sun, Jul 13, 2025 at 12:05:18AM +0800, Hans Zhang wrote:
>> On 2025/7/12 17:35, Manivannan Sadhasivam wrote:
>> ...
> 
>>>> IMO the "someday" goal should be that we get rid of aspm_policy
>>>> and enable all the available power saving states by default.  We
>>>> have sysfs knobs that administrators can use if necessary, and
>>>> drivers or quirks can disable states if they need to work around
>>>> hardware defects.
>>>
>>> Yeah, I think the default should be powersave and let the users
>>> disable it for performance if they want.
>>
>> Perhaps I don't think so. At present, our company's testing team has
>> tested quite a few NVMe SSDS. As far as I can remember, the SSDS
>> from two companies have encountered problems and will hang directly
>> when turned on. We have set CONFIG_PCIEASPM_POWERSAVE=y by default.
>> When encountering SSDS from these two companies, we had to add
>> "pcie_aspm.policy=default" in the cmdline, and then the boot worked
>> normally. Currently, we do not have a PCIe protocol analyzer to
>> analyze such issues. The current approach is to modify the cmdline.
>> So I can't prove whether it's a problem with the Root Port of our
>> SOC or the SSD device.
> 
> Have you reported these?

Dear Bjorn,

I haven't reported it. Because we don't have a PCIe protocol analyzer to 
analyze this situation, it's not certain whether it's a problem with our 
SOC Root Port or the NVMe SSD. If I have time later, I will conduct a 
comparison test on the RK3588.

> 
>> Here I agree with Bjorn's statement that sometimes the EP is not
>> necessarily very standard and there are no hardware issues.
>> Personally, I think the default is default or performance. When
>> users need to save power, they should then decide whether to
>> configure it as powersave or powersupersave.  Sometimes, if the EP
>> device connected by the customer is perfect, they can turn it on to
>> save power. But if the EP is not perfect, at least they will
>> immediately know what caused the problem.
> 
> We should discover device defects as early as possible so we can add
> quirks for them.  Defaulting to ASPM being partly disabled means it
> gets much less testing and users end up passing around "fixes" like
> booting with "pcie_aspm.policy=default" or similar.  I do not want
> users to trip over a device that doesn't work and have to look for
> workarounds on the web.
> 
> I also think it's somewhat irresponsible of us to consume more power
> than necessary.  But as Mani said, this would be a big change and
> might have to be done with a BIOS date check or something to try to
> avoid regressions.
> 

Ok. I understand your purpose now.

Best regards,
Hans

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Hans Zhang]

On 2025/7/13 01:02, Manivannan Sadhasivam wrote:
> On Sun, Jul 13, 2025 at 12:05:18AM GMT, Hans Zhang wrote:
>>
>>
>> On 2025/7/12 17:35, Manivannan Sadhasivam wrote:
>>>> We only have two callers of this (pcie-qcom.c and vmd.c, both in
>>>> drivers/pci/), so it's not clear to me that it needs to be in
>>>> include/linux/pci.h.
>>>>
>>>> I'm a little dubious about it in the first place since I don't think
>>>> drivers should be enabling ASPM states on their own, but pcie-qcom.c
>>>> and vmd.c are PCIe controller drivers, not PCI device drivers, so I
>>>> guess we can live with them for now.
>>>>
>>>> IMO the "someday" goal should be that we get rid of aspm_policy and
>>>> enable all the available power saving states by default.  We have
>>>> sysfs knobs that administrators can use if necessary, and drivers or
>>>> quirks can disable states if they need to work around hardware
>>>> defects.
>>>>
>>>
>>> Yeah, I think the default should be powersave and let the users disable it for
>>> performance if they want.
>>>
>>
>> Dear Bjorn and Mani,
>>
>> Perhaps I don't think so. At present, our company's testing team has tested
>> quite a few NVMe SSDS. As far as I can remember, the SSDS from two companies
>> have encountered problems and will hang directly when turned on. We have set
>> CONFIG_PCIEASPM_POWERSAVE=y by default. When encountering SSDS from these
>> two companies, we had to add "pcie_aspm.policy=default" in the cmdline, and
>> then the boot worked normally. Currently, we do not have a PCIe protocol
>> analyzer to analyze such issues. The current approach is to modify the
>> cmdline. So I can't prove whether it's a problem with the Root Port of our
>> SOC or the SSD device.
>>
>> Here I agree with Bjorn's statement that sometimes the EP is not necessarily
>> very standard and there are no hardware issues. Personally, I think the
>> default is default or performance. When users need to save power, they
>> should then decide whether to configure it as powersave or powersupersave.
>> Sometimes, if the EP device connected by the customer is perfect, they can
>> turn it on to save power. But if the EP is not perfect, at least they will
>> immediately know what caused the problem.
>>
> 
> We all agree that not all endpoints are standards compliant. So if they have any
> issues with ASPM, then ASPM for those devices should be disabled in the quirks
> or in the device driver.
> 
> That said, the change that Bjorn proposed is not going to happen in the
> immediate future.

Dear Mani,

Ok, I will keep an eye on the changes of ASPM.

Best regards,
Hans

Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4278 bytes --]

On Mon, 14 Jul 2025, Manivannan Sadhasivam wrote:
> On Sun, Jul 13, 2025 at 07:27:57PM GMT, Ilpo Järvinen wrote:
> > On Sat, 12 Jul 2025, Manivannan Sadhasivam wrote:
> > > On Fri, Jul 11, 2025 at 06:00:13PM GMT, Bjorn Helgaas wrote:
> > > > On Fri, Jul 11, 2025 at 04:38:48PM +0300, Ilpo Järvinen wrote:
> > > > 
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -1826,8 +1826,8 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
> > > > >  #ifdef CONFIG_PCIEASPM
> > > > >  int pci_disable_link_state(struct pci_dev *pdev, int state);
> > > > >  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> > > > > -int pci_enable_link_state(struct pci_dev *pdev, int state);
> > > > 
> > > > AFAICT there's no caller of this at all.  Why do we keep it?
> > > > 
> > > 
> > > I'm just working on a series to convert the ath{10/11/12}k drivers to use this
> > > API instead of modifying LNKCTL register directly:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/ath12k/pci.c#n961
> > 
> > Great. I assume but "this API" you meant disable/enable link state that 
> > are real pair unlike the current pci_enable_link_state()?
> > 
> 
> Yes. I actually based my series on top of this series, but I guess I should just
> merge the API change patch to my series as it is relatively small compared to
> this one (so it will get merged quicker).
> 
> > Did ath1xk need to do some hw specific register updates when changing ASPM 
> > state?
> > 
> 
> Fortunately, no.
> 
> > I tried to do similar conversion in r8169 (and actually also ath1xk too) 
> > but it was a while ago already. If I understood the code correctly, r8169 
> > seems to write some HW specific registers when changing ASPM state so I 
> > would have likely need to add some ops for it to play nice with state 
> > changes not originating from the driver itself but from the ASPM driver, 
> > which is where the work then stalled.
> > 
> 
> ath driver changes are straightforward, but I need to add an inline function to
> convert lnkctl setting to aspm states:
> 
> static inline int ath_pci_aspm_state(u16 lnkctl)
> {
> 	int state = 0;
> 
> 	if (lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
> 		state |= PCIE_LINK_STATE_L0S;
> 	if (lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
> 		state |= PCIE_LINK_STATE_L1;
> 
> 	return state;
> }
> 
> Currently, it is in ath/ath.h, but if you feel that we should move it to
> include/linux/pci.h, let me know!
> 
> > > > > -int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
> > > > 
> > > > We only have two callers of this (pcie-qcom.c and vmd.c, both in
> > > > drivers/pci/), so it's not clear to me that it needs to be in
> > > > include/linux/pci.h.
> > > > 
> > > > I'm a little dubious about it in the first place since I don't think
> > > > drivers should be enabling ASPM states on their own, but pcie-qcom.c
> > > > and vmd.c are PCIe controller drivers, not PCI device drivers, so I
> > > > guess we can live with them for now.
> > > > 
> > > > IMO the "someday" goal should be that we get rid of aspm_policy and
> > > > enable all the available power saving states by default.  We have
> > > > sysfs knobs that administrators can use if necessary, and drivers or
> > > > quirks can disable states if they need to work around hardware
> > > > defects.
> > > 
> > > Yeah, I think the default should be powersave and let the users disable it for
> > > performance if they want.
> > 
> > I'm certainly not against improvements in this front, but I think we need 
> > to get rid off custom ASPM disable code from the drivers first.
> > 
> 
> It would be difficult IMO as drivers have their own usecases. For instance, the
> ath drivers need to disable ASPM states to avoid issues during firmware
> download. Then they need to enable ASPM back once the firmware is up and
> running.

I understand and I didn't meant that we should disallow drivers from 
touching ASPM state. I just meant we should convert the drivers to 
do such state changes using the ASPM driver (through disable/enable API) 
rather than what they currently do with LNKCTL directly behind the ASPM 
driver's back.

-- 
 i.

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Krishna Chaitanya Chundru]

On 7/12/2025 4:36 AM, Krishna Chaitanya Chundru wrote:
> 
> 
> On 7/12/2025 3:06 AM, Bjorn Helgaas wrote:
>> On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya Chundru 
>> wrote:
>>> If the driver wants to move to higher data rate/speed than the 
>>> current data
>>> rate then the controller driver may need to change certain votes so that
>>> link may come up at requested data rate/speed like QCOM PCIe controllers
>>> need to change their RPMh (Resource Power Manager-hardened) state. Once
>>> link retraining is done controller drivers needs to adjust their votes
>>> based on the final data rate.
>>>
>>> Some controllers also may need to update their bandwidth voting like
>>> ICC BW votings etc.
>>>
>>> So, add pre_link_speed_change() & post_link_speed_change() op to call
>>> before & after the link re-train. There is no explicit locking 
>>> mechanisms
>>> as these are called by a single client Endpoint driver.
>>>
>>> In case of PCIe switch, if there is a request to change target speed 
>>> for a
>>> downstream port then no need to call these function ops as these are
>>> outside the scope of the controller drivers.
>>
>>> +++ b/include/linux/pci.h
>>> @@ -599,6 +599,24 @@ struct pci_host_bridge {
>>>       void (*release_fn)(struct pci_host_bridge *);
>>>       int (*enable_device)(struct pci_host_bridge *bridge, struct 
>>> pci_dev *dev);
>>>       void (*disable_device)(struct pci_host_bridge *bridge, struct 
>>> pci_dev *dev);
>>> +    /*
>>> +     * Callback to the host bridge drivers to update ICC BW votes, 
>>> clock
>>> +     * frequencies etc.. for the link re-train to come up in 
>>> targeted speed.
>>> +     * These are intended to be called by devices directly attached 
>>> to the
>>> +     * Root Port. These are called by a single client Endpoint 
>>> driver, so
>>> +     * there is no need for explicit locking mechanisms.
>>> +     */
>>> +    int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
>>> +                     struct pci_dev *dev, int speed);
>>> +    /*
>>> +     * Callback to the host bridge drivers to adjust ICC BW votes, 
>>> clock
>>> +     * frequencies etc.. to the updated speed after link re-train. 
>>> These
>>> +     * are intended to be called by devices directly attached to the
>>> +     * Root Port. These are called by a single client Endpoint driver,
>>> +     * so there is no need for explicit locking mechanisms.
>>
>> No need to repeat the entire comment.  s/.././
>>
>> These pointers feel awfully specific for being in struct
>> pci_host_bridge, since we only need them for a questionable QCOM
>> controller.  I think this needs to be pushed down into qcom somehow as
>> some kind of quirk.
>>
> Currently these are needed by QCOM controllers, but it may also needed
> by other controllers may also need these for updating ICC votes, any
> system level votes, clock frequencies etc.
> QCOM controllers is also doing one extra step in these functions to
> disable and enable ASPM only as it cannot link speed change support
> with ASPM enabled.
> 
Bjorn, can you check this.

For QCOM devices we need to update the RPMh vote i.e a power source
votes for the link to come up in required speed. and also we need
to update interconnect votes also. This will be applicable for
other vendors also.

If this is not correct place I can add them in the pci_ops.
- Krishna Chaitanya.
> - Krishna Chaitanya.
>>> +     */
>>> +    void (*post_link_speed_change)(struct pci_host_bridge *bridge,
>>> +                       struct pci_dev *dev, int speed);

Re: [PATCH v4 04/11] bus: mhi: host: Add support for Bandwidth scale

[Manivannan Sadhasivam]

On Fri, Jul 11, 2025 at 12:25:30PM GMT, Krishna Chaitanya Chundru wrote:

[...]

> > > > > +static int mhi_init_bw_scale(struct mhi_controller *mhi_cntrl,
> > > > > +			     int bw_scale_db)
> > > > > +{
> > > > > +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > > > > +	u32 bw_cfg_offset, val;
> > > > > +	int ret, er_index;
> > > > > +
> > > > > +	ret = mhi_find_capability(mhi_cntrl, MHI_BW_SCALE_CAP_ID, &bw_cfg_offset);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	er_index = mhi_get_er_index(mhi_cntrl, MHI_ER_BW_SCALE);
> > > > > +	if (er_index < 0)
> > > > > +		return er_index;
> > > > > +
> > > > > +	bw_cfg_offset += MHI_BW_SCALE_CFG_OFFSET;
> > > > > +
> > > > > +	/* Advertise host support */
> > > > > +	val = (__force u32)cpu_to_le32(FIELD_PREP(MHI_BW_SCALE_DB_CHAN_ID, bw_scale_db) |
> > > > > +				       FIELD_PREP(MHI_BW_SCALE_ER_INDEX, er_index) |
> > > > > +				       MHI_BW_SCALE_ENABLED);
> > > > > +
> > > > 
> > > > It is wrong to store the value of cpu_to_le32() in a non-le32 variable.
> > > > mhi_write_reg() accepts the 'val' in native endian. writel used in the
> > > > controller drivers should take care of converting to LE before writing to the
> > > > device.
> > > > 
> > > ok then I will revert to u32.
> > > 
> > > I think we need a patch in the controller drivers seperately to handle
> > > this.
> > 
> > Why?
> > 
> what I understood from your previous comment is from here we need to
> send u32 only and the controller drivers should take care of
> converting u32 to le32.
> As of today controller drivers are not considering this and writing
> u32 only.
> So we need a seperate patch in the controller driver to convert it to
> le32.

No. All controller drivers are using writel() to write the register. Since
writel() converts the value to little endian from native endian, you do not need
to do anything.

> > > > > +	mhi_write_reg(mhi_cntrl, mhi_cntrl->regs, bw_cfg_offset, val);
> > > > > +
> > > > > +	dev_dbg(dev, "Bandwidth scaling setup complete with event ring: %d\n",
> > > > > +		er_index);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +

[...]

> > > > > +	link_info.target_link_speed = MHI_TRE_GET_EV_LINKSPEED(dev_rp);
> > > > > +	link_info.target_link_width = MHI_TRE_GET_EV_LINKWIDTH(dev_rp);
> > > > > +	link_info.sequence_num = MHI_TRE_GET_EV_BW_REQ_SEQ(dev_rp);
> > > > > +
> > > > > +	dev_dbg(dev, "Received BW_REQ with seq:%d link speed:0x%x width:0x%x\n",
> > > > > +		link_info.sequence_num,
> > > > > +		link_info.target_link_speed,
> > > > > +		link_info.target_link_width);
> > > > > +
> > > > > +	/* Bring host and device out of suspended states */
> > > > > +	ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
> > > > 
> > > > Looks like mhi_device_get_sync() is going runtime_get()/runtime_put() inside
> > > > mhi_trigger_resume(). I'm wondering why that is necessary.
> > > > 
> > > Before mhi_trigger_resume we are doing wake_get, which will make sure
> > > device will not transition to the low power modes while servicing this
> > > event. And also make sure mhi state is in M0 only.
> > > 
> > > As we are in workqueue this can be scheduled at some later time and by
> > > that time mhi can go to m1 or m2 state.
> > > 
> > 
> > My comment was more about the behavior of mhi_trigger_resume(). Why does it call
> > get() and put()? Sorry if I was not clear.
> > Get() needed for bringing out of suspend, put() is for balancing the
> get() I belive the intention here is to trigger resume only and balance
> pm framework.
> 
> That said mhi_device_get_sync() has a bug we are doing wake_get() before
> triggering resume and also trigger resume will not guarantee that device
> had resumed, it is not safe to do wake_get untill device is out of
> suspend, we might need to introduce runtime_get_sync() function and new
> API for this purpose.
> mhi_trigger_resume makes sense in the place like this[1], where ever
> there are register writes after trigger resume it may need  to be
> replaced with runtime_get_sync().
> 
> This function needs to do mhi_cntrl->runtime_get_sync(mhi_cntrl); first
> to make sure controller is not in suspend and then mhi_device_get_sync()
> to make sure device is staying in M0. and in the end we balance these
> with mhi_cntrl->runtime_put(mhi_cntrl) & mhi_device_put().
> 

Sounds good to me.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Krishna Chaitanya Chundru]

On 7/22/2025 4:33 PM, Krishna Chaitanya Chundru wrote:
> 
> 
> On 7/12/2025 4:36 AM, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 7/12/2025 3:06 AM, Bjorn Helgaas wrote:
>>> On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya Chundru 
>>> wrote:
>>>> If the driver wants to move to higher data rate/speed than the 
>>>> current data
>>>> rate then the controller driver may need to change certain votes so 
>>>> that
>>>> link may come up at requested data rate/speed like QCOM PCIe 
>>>> controllers
>>>> need to change their RPMh (Resource Power Manager-hardened) state. Once
>>>> link retraining is done controller drivers needs to adjust their votes
>>>> based on the final data rate.
>>>>
>>>> Some controllers also may need to update their bandwidth voting like
>>>> ICC BW votings etc.
>>>>
>>>> So, add pre_link_speed_change() & post_link_speed_change() op to call
>>>> before & after the link re-train. There is no explicit locking 
>>>> mechanisms
>>>> as these are called by a single client Endpoint driver.
>>>>
>>>> In case of PCIe switch, if there is a request to change target speed 
>>>> for a
>>>> downstream port then no need to call these function ops as these are
>>>> outside the scope of the controller drivers.
>>>
>>>> +++ b/include/linux/pci.h
>>>> @@ -599,6 +599,24 @@ struct pci_host_bridge {
>>>>       void (*release_fn)(struct pci_host_bridge *);
>>>>       int (*enable_device)(struct pci_host_bridge *bridge, struct 
>>>> pci_dev *dev);
>>>>       void (*disable_device)(struct pci_host_bridge *bridge, struct 
>>>> pci_dev *dev);
>>>> +    /*
>>>> +     * Callback to the host bridge drivers to update ICC BW votes, 
>>>> clock
>>>> +     * frequencies etc.. for the link re-train to come up in 
>>>> targeted speed.
>>>> +     * These are intended to be called by devices directly attached 
>>>> to the
>>>> +     * Root Port. These are called by a single client Endpoint 
>>>> driver, so
>>>> +     * there is no need for explicit locking mechanisms.
>>>> +     */
>>>> +    int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
>>>> +                     struct pci_dev *dev, int speed);
>>>> +    /*
>>>> +     * Callback to the host bridge drivers to adjust ICC BW votes, 
>>>> clock
>>>> +     * frequencies etc.. to the updated speed after link re-train. 
>>>> These
>>>> +     * are intended to be called by devices directly attached to the
>>>> +     * Root Port. These are called by a single client Endpoint driver,
>>>> +     * so there is no need for explicit locking mechanisms.
>>>
>>> No need to repeat the entire comment.  s/.././
>>>
>>> These pointers feel awfully specific for being in struct
>>> pci_host_bridge, since we only need them for a questionable QCOM
>>> controller.  I think this needs to be pushed down into qcom somehow as
>>> some kind of quirk.
>>>
>> Currently these are needed by QCOM controllers, but it may also needed
>> by other controllers may also need these for updating ICC votes, any
>> system level votes, clock frequencies etc.
>> QCOM controllers is also doing one extra step in these functions to
>> disable and enable ASPM only as it cannot link speed change support
>> with ASPM enabled.
>>
> Bjorn, can you check this.
> 
> For QCOM devices we need to update the RPMh vote i.e a power source
> votes for the link to come up in required speed. and also we need
> to update interconnect votes also. This will be applicable for
> other vendors also.
> 
> If this is not correct place I can add them in the pci_ops.
Bjorn,

Can you please comment on this.

Is this fine to move these to the pci_ops of the bridge.
Again these are not specific to QCOM, any controller driver which
needs to change their clock rates, ICC bw votes etc needs to have
these.

- Krishna Chaitanya.
> - Krishna Chaitanya.
>> - Krishna Chaitanya.
>>>> +     */
>>>> +    void (*post_link_speed_change)(struct pci_host_bridge *bridge,
>>>> +                       struct pci_dev *dev, int speed);

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Konrad Dybcio]

On 8/12/25 6:05 AM, Krishna Chaitanya Chundru wrote:
> 
> 
> On 7/22/2025 4:33 PM, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 7/12/2025 4:36 AM, Krishna Chaitanya Chundru wrote:
>>>
>>>
>>> On 7/12/2025 3:06 AM, Bjorn Helgaas wrote:
>>>> On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>> If the driver wants to move to higher data rate/speed than the current data
>>>>> rate then the controller driver may need to change certain votes so that
>>>>> link may come up at requested data rate/speed like QCOM PCIe controllers
>>>>> need to change their RPMh (Resource Power Manager-hardened) state. Once
>>>>> link retraining is done controller drivers needs to adjust their votes
>>>>> based on the final data rate.
>>>>>
>>>>> Some controllers also may need to update their bandwidth voting like
>>>>> ICC BW votings etc.
>>>>>
>>>>> So, add pre_link_speed_change() & post_link_speed_change() op to call
>>>>> before & after the link re-train. There is no explicit locking mechanisms
>>>>> as these are called by a single client Endpoint driver.
>>>>>
>>>>> In case of PCIe switch, if there is a request to change target speed for a
>>>>> downstream port then no need to call these function ops as these are
>>>>> outside the scope of the controller drivers.
>>>>
>>>>> +++ b/include/linux/pci.h
>>>>> @@ -599,6 +599,24 @@ struct pci_host_bridge {
>>>>>       void (*release_fn)(struct pci_host_bridge *);
>>>>>       int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>>>>>       void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>>>>> +    /*
>>>>> +     * Callback to the host bridge drivers to update ICC BW votes, clock
>>>>> +     * frequencies etc.. for the link re-train to come up in targeted speed.
>>>>> +     * These are intended to be called by devices directly attached to the
>>>>> +     * Root Port. These are called by a single client Endpoint driver, so
>>>>> +     * there is no need for explicit locking mechanisms.
>>>>> +     */
>>>>> +    int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
>>>>> +                     struct pci_dev *dev, int speed);
>>>>> +    /*
>>>>> +     * Callback to the host bridge drivers to adjust ICC BW votes, clock
>>>>> +     * frequencies etc.. to the updated speed after link re-train. These
>>>>> +     * are intended to be called by devices directly attached to the
>>>>> +     * Root Port. These are called by a single client Endpoint driver,
>>>>> +     * so there is no need for explicit locking mechanisms.
>>>>
>>>> No need to repeat the entire comment.  s/.././
>>>>
>>>> These pointers feel awfully specific for being in struct
>>>> pci_host_bridge, since we only need them for a questionable QCOM
>>>> controller.  I think this needs to be pushed down into qcom somehow as
>>>> some kind of quirk.
>>>>
>>> Currently these are needed by QCOM controllers, but it may also needed
>>> by other controllers may also need these for updating ICC votes, any
>>> system level votes, clock frequencies etc.
>>> QCOM controllers is also doing one extra step in these functions to
>>> disable and enable ASPM only as it cannot link speed change support
>>> with ASPM enabled.
>>>
>> Bjorn, can you check this.
>>
>> For QCOM devices we need to update the RPMh vote i.e a power source
>> votes for the link to come up in required speed. and also we need
>> to update interconnect votes also. This will be applicable for
>> other vendors also.
>>
>> If this is not correct place I can add them in the pci_ops.
> Bjorn,
> 
> Can you please comment on this.
> 
> Is this fine to move these to the pci_ops of the bridge.
> Again these are not specific to QCOM, any controller driver which
> needs to change their clock rates, ICC bw votes etc needs to have
> these.

Do you even need to set the OPP explicitly? The global irq handler
already does so on linkup, and you seem to toggle the link state in
the newly introduced helpers

Now not all DTs currently have a global interrupt, but that's a mass
fixup to be done anyway..

Konrad

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Krishna Chaitanya Chundru]

On 8/12/2025 2:57 PM, Konrad Dybcio wrote:
> On 8/12/25 6:05 AM, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 7/22/2025 4:33 PM, Krishna Chaitanya Chundru wrote:
>>>
>>>
>>> On 7/12/2025 4:36 AM, Krishna Chaitanya Chundru wrote:
>>>>
>>>>
>>>> On 7/12/2025 3:06 AM, Bjorn Helgaas wrote:
>>>>> On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>> If the driver wants to move to higher data rate/speed than the current data
>>>>>> rate then the controller driver may need to change certain votes so that
>>>>>> link may come up at requested data rate/speed like QCOM PCIe controllers
>>>>>> need to change their RPMh (Resource Power Manager-hardened) state. Once
>>>>>> link retraining is done controller drivers needs to adjust their votes
>>>>>> based on the final data rate.
>>>>>>
>>>>>> Some controllers also may need to update their bandwidth voting like
>>>>>> ICC BW votings etc.
>>>>>>
>>>>>> So, add pre_link_speed_change() & post_link_speed_change() op to call
>>>>>> before & after the link re-train. There is no explicit locking mechanisms
>>>>>> as these are called by a single client Endpoint driver.
>>>>>>
>>>>>> In case of PCIe switch, if there is a request to change target speed for a
>>>>>> downstream port then no need to call these function ops as these are
>>>>>> outside the scope of the controller drivers.
>>>>>
>>>>>> +++ b/include/linux/pci.h
>>>>>> @@ -599,6 +599,24 @@ struct pci_host_bridge {
>>>>>>        void (*release_fn)(struct pci_host_bridge *);
>>>>>>        int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>>>>>>        void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
>>>>>> +    /*
>>>>>> +     * Callback to the host bridge drivers to update ICC BW votes, clock
>>>>>> +     * frequencies etc.. for the link re-train to come up in targeted speed.
>>>>>> +     * These are intended to be called by devices directly attached to the
>>>>>> +     * Root Port. These are called by a single client Endpoint driver, so
>>>>>> +     * there is no need for explicit locking mechanisms.
>>>>>> +     */
>>>>>> +    int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
>>>>>> +                     struct pci_dev *dev, int speed);
>>>>>> +    /*
>>>>>> +     * Callback to the host bridge drivers to adjust ICC BW votes, clock
>>>>>> +     * frequencies etc.. to the updated speed after link re-train. These
>>>>>> +     * are intended to be called by devices directly attached to the
>>>>>> +     * Root Port. These are called by a single client Endpoint driver,
>>>>>> +     * so there is no need for explicit locking mechanisms.
>>>>>
>>>>> No need to repeat the entire comment.  s/.././
>>>>>
>>>>> These pointers feel awfully specific for being in struct
>>>>> pci_host_bridge, since we only need them for a questionable QCOM
>>>>> controller.  I think this needs to be pushed down into qcom somehow as
>>>>> some kind of quirk.
>>>>>
>>>> Currently these are needed by QCOM controllers, but it may also needed
>>>> by other controllers may also need these for updating ICC votes, any
>>>> system level votes, clock frequencies etc.
>>>> QCOM controllers is also doing one extra step in these functions to
>>>> disable and enable ASPM only as it cannot link speed change support
>>>> with ASPM enabled.
>>>>
>>> Bjorn, can you check this.
>>>
>>> For QCOM devices we need to update the RPMh vote i.e a power source
>>> votes for the link to come up in required speed. and also we need
>>> to update interconnect votes also. This will be applicable for
>>> other vendors also.
>>>
>>> If this is not correct place I can add them in the pci_ops.
>> Bjorn,
>>
>> Can you please comment on this.
>>
>> Is this fine to move these to the pci_ops of the bridge.
>> Again these are not specific to QCOM, any controller driver which
>> needs to change their clock rates, ICC bw votes etc needs to have
>> these.
> 
> Do you even need to set the OPP explicitly? The global irq handler
> already does so on linkup, and you seem to toggle the link state in
> the newly introduced helpers
> 
> Now not all DTs currently have a global interrupt, but that's a mass
> fixup to be done anyway..
> 
Konrad,

global IRQ in the qcom controllers will only come in only on initial
linkup, and later on link speed change through bwctrl driver we will
not get global IRQ.

For QCOM controllers we need to change the RPMH vote for example if
we want to update the PCIe data rate from 5 GT/s to 8 GT/s we might
need to update the RPMh vote from low_svs to NOM corner before we
initiate link up. for that reason we are introducing pre & post
link_speed_change function pointer.

- Krishna Chaitanya.
> Konrad

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Manivannan Sadhasivam]

On Tue, Aug 12, 2025 at 09:35:46AM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 7/22/2025 4:33 PM, Krishna Chaitanya Chundru wrote:
> > 
> > 
> > On 7/12/2025 4:36 AM, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 7/12/2025 3:06 AM, Bjorn Helgaas wrote:
> > > > On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya
> > > > Chundru wrote:
> > > > > If the driver wants to move to higher data rate/speed than
> > > > > the current data
> > > > > rate then the controller driver may need to change certain
> > > > > votes so that
> > > > > link may come up at requested data rate/speed like QCOM PCIe
> > > > > controllers
> > > > > need to change their RPMh (Resource Power Manager-hardened) state. Once
> > > > > link retraining is done controller drivers needs to adjust their votes
> > > > > based on the final data rate.
> > > > > 
> > > > > Some controllers also may need to update their bandwidth voting like
> > > > > ICC BW votings etc.
> > > > > 
> > > > > So, add pre_link_speed_change() & post_link_speed_change() op to call
> > > > > before & after the link re-train. There is no explicit
> > > > > locking mechanisms
> > > > > as these are called by a single client Endpoint driver.
> > > > > 
> > > > > In case of PCIe switch, if there is a request to change
> > > > > target speed for a
> > > > > downstream port then no need to call these function ops as these are
> > > > > outside the scope of the controller drivers.
> > > > 
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -599,6 +599,24 @@ struct pci_host_bridge {
> > > > >       void (*release_fn)(struct pci_host_bridge *);
> > > > >       int (*enable_device)(struct pci_host_bridge *bridge,
> > > > > struct pci_dev *dev);
> > > > >       void (*disable_device)(struct pci_host_bridge *bridge,
> > > > > struct pci_dev *dev);
> > > > > +    /*
> > > > > +     * Callback to the host bridge drivers to update ICC BW
> > > > > votes, clock
> > > > > +     * frequencies etc.. for the link re-train to come up
> > > > > in targeted speed.
> > > > > +     * These are intended to be called by devices directly
> > > > > attached to the
> > > > > +     * Root Port. These are called by a single client
> > > > > Endpoint driver, so
> > > > > +     * there is no need for explicit locking mechanisms.
> > > > > +     */
> > > > > +    int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
> > > > > +                     struct pci_dev *dev, int speed);
> > > > > +    /*
> > > > > +     * Callback to the host bridge drivers to adjust ICC BW
> > > > > votes, clock
> > > > > +     * frequencies etc.. to the updated speed after link
> > > > > re-train. These
> > > > > +     * are intended to be called by devices directly attached to the
> > > > > +     * Root Port. These are called by a single client Endpoint driver,
> > > > > +     * so there is no need for explicit locking mechanisms.
> > > > 
> > > > No need to repeat the entire comment.  s/.././
> > > > 
> > > > These pointers feel awfully specific for being in struct
> > > > pci_host_bridge, since we only need them for a questionable QCOM
> > > > controller.  I think this needs to be pushed down into qcom somehow as
> > > > some kind of quirk.
> > > > 
> > > Currently these are needed by QCOM controllers, but it may also needed
> > > by other controllers may also need these for updating ICC votes, any
> > > system level votes, clock frequencies etc.
> > > QCOM controllers is also doing one extra step in these functions to
> > > disable and enable ASPM only as it cannot link speed change support
> > > with ASPM enabled.
> > > 
> > Bjorn, can you check this.
> > 
> > For QCOM devices we need to update the RPMh vote i.e a power source
> > votes for the link to come up in required speed. and also we need
> > to update interconnect votes also. This will be applicable for
> > other vendors also.
> > 
> > If this is not correct place I can add them in the pci_ops.
> Bjorn,
> 
> Can you please comment on this.
> 
> Is this fine to move these to the pci_ops of the bridge.
> Again these are not specific to QCOM, any controller driver which
> needs to change their clock rates, ICC bw votes etc needs to have
> these.
> 

No, moving to 'pci_ops' is terrible than having it in 'pci_host_bridge' IMO. If
we want to get rid of these ops, we can introduce a quirk flag in
'pci_host_bridge' and when set, the bwctrl code can disable/enable ASPM
before/after link retrain. This clearly states that the controller is quirky and
we need to disable/enable ASPM.

For setting OPP, you can have a separate callback in 'pci_host_bridge' that just
allows setting OPP *after* retrain, like 'pci_host_bridge:link_change_notify()'.
I don't think you really need to set OPP before retrain though. As even if you
do it pre and post link retrain, there is still a small window where the link
will operate without adequate vote.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Krishna Chaitanya Chundru]

On 8/12/2025 10:13 PM, Manivannan Sadhasivam wrote:
> On Tue, Aug 12, 2025 at 09:35:46AM GMT, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 7/22/2025 4:33 PM, Krishna Chaitanya Chundru wrote:
>>>
>>>
>>> On 7/12/2025 4:36 AM, Krishna Chaitanya Chundru wrote:
>>>>
>>>>
>>>> On 7/12/2025 3:06 AM, Bjorn Helgaas wrote:
>>>>> On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya
>>>>> Chundru wrote:
>>>>>> If the driver wants to move to higher data rate/speed than
>>>>>> the current data
>>>>>> rate then the controller driver may need to change certain
>>>>>> votes so that
>>>>>> link may come up at requested data rate/speed like QCOM PCIe
>>>>>> controllers
>>>>>> need to change their RPMh (Resource Power Manager-hardened) state. Once
>>>>>> link retraining is done controller drivers needs to adjust their votes
>>>>>> based on the final data rate.
>>>>>>
>>>>>> Some controllers also may need to update their bandwidth voting like
>>>>>> ICC BW votings etc.
>>>>>>
>>>>>> So, add pre_link_speed_change() & post_link_speed_change() op to call
>>>>>> before & after the link re-train. There is no explicit
>>>>>> locking mechanisms
>>>>>> as these are called by a single client Endpoint driver.
>>>>>>
>>>>>> In case of PCIe switch, if there is a request to change
>>>>>> target speed for a
>>>>>> downstream port then no need to call these function ops as these are
>>>>>> outside the scope of the controller drivers.
>>>>>
>>>>>> +++ b/include/linux/pci.h
>>>>>> @@ -599,6 +599,24 @@ struct pci_host_bridge {
>>>>>>        void (*release_fn)(struct pci_host_bridge *);
>>>>>>        int (*enable_device)(struct pci_host_bridge *bridge,
>>>>>> struct pci_dev *dev);
>>>>>>        void (*disable_device)(struct pci_host_bridge *bridge,
>>>>>> struct pci_dev *dev);
>>>>>> +    /*
>>>>>> +     * Callback to the host bridge drivers to update ICC BW
>>>>>> votes, clock
>>>>>> +     * frequencies etc.. for the link re-train to come up
>>>>>> in targeted speed.
>>>>>> +     * These are intended to be called by devices directly
>>>>>> attached to the
>>>>>> +     * Root Port. These are called by a single client
>>>>>> Endpoint driver, so
>>>>>> +     * there is no need for explicit locking mechanisms.
>>>>>> +     */
>>>>>> +    int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
>>>>>> +                     struct pci_dev *dev, int speed);
>>>>>> +    /*
>>>>>> +     * Callback to the host bridge drivers to adjust ICC BW
>>>>>> votes, clock
>>>>>> +     * frequencies etc.. to the updated speed after link
>>>>>> re-train. These
>>>>>> +     * are intended to be called by devices directly attached to the
>>>>>> +     * Root Port. These are called by a single client Endpoint driver,
>>>>>> +     * so there is no need for explicit locking mechanisms.
>>>>>
>>>>> No need to repeat the entire comment.  s/.././
>>>>>
>>>>> These pointers feel awfully specific for being in struct
>>>>> pci_host_bridge, since we only need them for a questionable QCOM
>>>>> controller.  I think this needs to be pushed down into qcom somehow as
>>>>> some kind of quirk.
>>>>>
>>>> Currently these are needed by QCOM controllers, but it may also needed
>>>> by other controllers may also need these for updating ICC votes, any
>>>> system level votes, clock frequencies etc.
>>>> QCOM controllers is also doing one extra step in these functions to
>>>> disable and enable ASPM only as it cannot link speed change support
>>>> with ASPM enabled.
>>>>
>>> Bjorn, can you check this.
>>>
>>> For QCOM devices we need to update the RPMh vote i.e a power source
>>> votes for the link to come up in required speed. and also we need
>>> to update interconnect votes also. This will be applicable for
>>> other vendors also.
>>>
>>> If this is not correct place I can add them in the pci_ops.
>> Bjorn,
>>
>> Can you please comment on this.
>>
>> Is this fine to move these to the pci_ops of the bridge.
>> Again these are not specific to QCOM, any controller driver which
>> needs to change their clock rates, ICC bw votes etc needs to have
>> these.
>>
> 
> No, moving to 'pci_ops' is terrible than having it in 'pci_host_bridge' IMO. If
> we want to get rid of these ops, we can introduce a quirk flag in
> 'pci_host_bridge' and when set, the bwctrl code can disable/enable ASPM
> before/after link retrain. This clearly states that the controller is quirky and
> we need to disable/enable ASPM.
> 
> For setting OPP, you can have a separate callback in 'pci_host_bridge' that just
> allows setting OPP *after* retrain, like 'pci_host_bridge:link_change_notify()'.
> I don't think you really need to set OPP before retrain though. As even if you
> do it pre and post link retrain, there is still a small window where the link
> will operate without adequate vote.
> 
Hi Mani,

we need to update the OPP votes before link retrain, for example if
there is request  to change data rate from 5 GT/s to 8 GT/s on some
platforms we need to update RPMh votes from low_svs to NOM corner
without this clocks will not scale for data rates 8 GT/s and link
retrain will fail. For that reason we are trying to add pre and post
callbacks.

- Krishna Chaitanya.
> - Mani
> 

Re: [PATCH v4 03/11] bus: mhi: host: Add support to read MHI capabilities

[Krishna Chaitanya Chundru]

On 7/9/2025 5:50 PM, Ilpo Järvinen wrote:
> On Wed, 9 Jul 2025, Krishna Chaitanya Chundru wrote:
>> On 7/8/2025 10:06 PM, Manivannan Sadhasivam wrote:
>>> On Mon, Jun 09, 2025 at 04:21:24PM GMT, Krishna Chaitanya Chundru wrote:
>>>> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>>>
>>>> As per MHI spec v1.2,sec 6.6, MHI has capability registers which are
>>>> located after the ERDB array. The location of this group of registers is
>>>> indicated by the MISCOFF register. Each capability has a capability ID to
>>>> determine which functionality is supported and each capability will point
>>>> to the next capability supported.
>>>>
>>>> Add a basic function to read those capabilities offsets.
>>>>
>>>> Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>>> Signed-off-by: Krishna Chaitanya Chundru
>>>> <krishna.chundru@oss.qualcomm.com>
>>>> ---
>>>>    drivers/bus/mhi/common.h    | 13 +++++++++++++
>>>>    drivers/bus/mhi/host/init.c | 34 ++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
>>>> index
>>>> dda340aaed95a5573a2ec776ca712e11a1ed0b52..58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd
>>>> 100644
>>>> --- a/drivers/bus/mhi/common.h
>>>> +++ b/drivers/bus/mhi/common.h
>>>> @@ -16,6 +16,7 @@
>>>>    #define MHICFG				0x10
>>>>    #define CHDBOFF				0x18
>>>>    #define ERDBOFF				0x20
>>>> +#define MISCOFF				0x24
>>>>    #define BHIOFF				0x28
>>>>    #define BHIEOFF				0x2c
>>>>    #define DEBUGOFF			0x30
>>>> @@ -113,6 +114,9 @@
>>>>    #define MHISTATUS_MHISTATE_MASK		GENMASK(15, 8)
>>>>    #define MHISTATUS_SYSERR_MASK		BIT(2)
>>>>    #define MHISTATUS_READY_MASK		BIT(0)
>>>> +#define MISC_CAP_MASK			GENMASK(31, 0)
>>>> +#define CAP_CAPID_MASK			GENMASK(31, 24)
>>>> +#define CAP_NEXT_CAP_MASK		GENMASK(23, 12)
>>>>      /* Command Ring Element macros */
>>>>    /* No operation command */
>>>> @@ -204,6 +208,15 @@
>>>>    #define MHI_RSCTRE_DATA_DWORD1
>>>> cpu_to_le32(FIELD_PREP(GENMASK(23, 16), \
>>>>    							       MHI_PKT_TYPE_COALESCING))
>>>>    +enum mhi_capability_type {
>>>> +	MHI_CAP_ID_INTX = 0x1,
>>>> +	MHI_CAP_ID_TIME_SYNC = 0x2,
>>>> +	MHI_CAP_ID_BW_SCALE = 0x3,
>>>> +	MHI_CAP_ID_TSC_TIME_SYNC = 0x4,
>>>> +	MHI_CAP_ID_MAX_TRB_LEN = 0x5,
>>>> +	MHI_CAP_ID_MAX,
>>>> +};
>>>> +
>>>>    enum mhi_pkt_type {
>>>>    	MHI_PKT_TYPE_INVALID = 0x0,
>>>>    	MHI_PKT_TYPE_NOOP_CMD = 0x1,
>>>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>>>> index
>>>> 13e7a55f54ff45b83b3f18b97e2cdd83d4836fe3..9102ce13a2059f599b46d25ef631f643142642be
>>>> 100644
>>>> --- a/drivers/bus/mhi/host/init.c
>>>> +++ b/drivers/bus/mhi/host/init.c
>>>> @@ -467,6 +467,40 @@ int mhi_init_dev_ctxt(struct mhi_controller
>>>> *mhi_cntrl)
>>>>    	return ret;
>>>>    }
>>>>    +static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32
>>>> capability, u32 *offset)
>>>> +{
>>>> +	u32 val, cur_cap, next_offset;
>>>> +	int ret;
>>>> +
>>>> +	/* Get the first supported capability offset */
>>>> +	ret = mhi_read_reg_field(mhi_cntrl, mhi_cntrl->regs, MISCOFF,
>>>> MISC_CAP_MASK, offset);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	*offset = (__force u32)le32_to_cpu(*offset);
>>>
>>> Why do you need __force attribute? What does it suppress? Is it because the
>>> pointer is not le32?
>>>
>> yes to suppress warnings.
> 
> I'm pretty sure sparce with endianness checking won't be happy with that
> construct as you pass u32 where le32_to_cpu() expects __le32. Have you
> checked this with sparse? (It might not check endianness with default args.)
> 
>>>> +	do {
>>>> +		if (*offset >= mhi_cntrl->reg_len)
>>>> +			return -ENXIO;
>>>> +
>>>> +		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, *offset, &val);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		val = (__force u32)le32_to_cpu(val);
>>>> +		cur_cap = FIELD_GET(CAP_CAPID_MASK, val);
>>>> +		next_offset = FIELD_GET(CAP_NEXT_CAP_MASK, val);
>>>> +		if (cur_cap >= MHI_CAP_ID_MAX)
>>>> +			return -ENXIO;
>>>> +
>>>> +		if (cur_cap == capability)
>>>> +			return 0;
>>>> +
>>>> +		*offset = next_offset;
>>>> +	} while (next_offset);
>>>> +
>>>> +	return -ENXIO;
>>>> +}
> 
> There's a generalization of capability search in Hans Zhang's series,
> can it be used here too?
I Checked this option, the mhi capabilities will not fit in to the
series mentioned for not matching the mask for cap ID, size etc. mhi
capability are similar to PCI, but they vary between mask of cap ID, 
size of the space etc. so going with current approach only.

- Krishna Chaitanya.
> 
> 

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Manivannan Sadhasivam]

On Wed, Aug 13, 2025 at 09:25:03AM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 8/12/2025 10:13 PM, Manivannan Sadhasivam wrote:
> > On Tue, Aug 12, 2025 at 09:35:46AM GMT, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 7/22/2025 4:33 PM, Krishna Chaitanya Chundru wrote:
> > > > 
> > > > 
> > > > On 7/12/2025 4:36 AM, Krishna Chaitanya Chundru wrote:
> > > > > 
> > > > > 
> > > > > On 7/12/2025 3:06 AM, Bjorn Helgaas wrote:
> > > > > > On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya
> > > > > > Chundru wrote:
> > > > > > > If the driver wants to move to higher data rate/speed than
> > > > > > > the current data
> > > > > > > rate then the controller driver may need to change certain
> > > > > > > votes so that
> > > > > > > link may come up at requested data rate/speed like QCOM PCIe
> > > > > > > controllers
> > > > > > > need to change their RPMh (Resource Power Manager-hardened) state. Once
> > > > > > > link retraining is done controller drivers needs to adjust their votes
> > > > > > > based on the final data rate.
> > > > > > > 
> > > > > > > Some controllers also may need to update their bandwidth voting like
> > > > > > > ICC BW votings etc.
> > > > > > > 
> > > > > > > So, add pre_link_speed_change() & post_link_speed_change() op to call
> > > > > > > before & after the link re-train. There is no explicit
> > > > > > > locking mechanisms
> > > > > > > as these are called by a single client Endpoint driver.
> > > > > > > 
> > > > > > > In case of PCIe switch, if there is a request to change
> > > > > > > target speed for a
> > > > > > > downstream port then no need to call these function ops as these are
> > > > > > > outside the scope of the controller drivers.
> > > > > > 
> > > > > > > +++ b/include/linux/pci.h
> > > > > > > @@ -599,6 +599,24 @@ struct pci_host_bridge {
> > > > > > >        void (*release_fn)(struct pci_host_bridge *);
> > > > > > >        int (*enable_device)(struct pci_host_bridge *bridge,
> > > > > > > struct pci_dev *dev);
> > > > > > >        void (*disable_device)(struct pci_host_bridge *bridge,
> > > > > > > struct pci_dev *dev);
> > > > > > > +    /*
> > > > > > > +     * Callback to the host bridge drivers to update ICC BW
> > > > > > > votes, clock
> > > > > > > +     * frequencies etc.. for the link re-train to come up
> > > > > > > in targeted speed.
> > > > > > > +     * These are intended to be called by devices directly
> > > > > > > attached to the
> > > > > > > +     * Root Port. These are called by a single client
> > > > > > > Endpoint driver, so
> > > > > > > +     * there is no need for explicit locking mechanisms.
> > > > > > > +     */
> > > > > > > +    int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
> > > > > > > +                     struct pci_dev *dev, int speed);
> > > > > > > +    /*
> > > > > > > +     * Callback to the host bridge drivers to adjust ICC BW
> > > > > > > votes, clock
> > > > > > > +     * frequencies etc.. to the updated speed after link
> > > > > > > re-train. These
> > > > > > > +     * are intended to be called by devices directly attached to the
> > > > > > > +     * Root Port. These are called by a single client Endpoint driver,
> > > > > > > +     * so there is no need for explicit locking mechanisms.
> > > > > > 
> > > > > > No need to repeat the entire comment.  s/.././
> > > > > > 
> > > > > > These pointers feel awfully specific for being in struct
> > > > > > pci_host_bridge, since we only need them for a questionable QCOM
> > > > > > controller.  I think this needs to be pushed down into qcom somehow as
> > > > > > some kind of quirk.
> > > > > > 
> > > > > Currently these are needed by QCOM controllers, but it may also needed
> > > > > by other controllers may also need these for updating ICC votes, any
> > > > > system level votes, clock frequencies etc.
> > > > > QCOM controllers is also doing one extra step in these functions to
> > > > > disable and enable ASPM only as it cannot link speed change support
> > > > > with ASPM enabled.
> > > > > 
> > > > Bjorn, can you check this.
> > > > 
> > > > For QCOM devices we need to update the RPMh vote i.e a power source
> > > > votes for the link to come up in required speed. and also we need
> > > > to update interconnect votes also. This will be applicable for
> > > > other vendors also.
> > > > 
> > > > If this is not correct place I can add them in the pci_ops.
> > > Bjorn,
> > > 
> > > Can you please comment on this.
> > > 
> > > Is this fine to move these to the pci_ops of the bridge.
> > > Again these are not specific to QCOM, any controller driver which
> > > needs to change their clock rates, ICC bw votes etc needs to have
> > > these.
> > > 
> > 
> > No, moving to 'pci_ops' is terrible than having it in 'pci_host_bridge' IMO. If
> > we want to get rid of these ops, we can introduce a quirk flag in
> > 'pci_host_bridge' and when set, the bwctrl code can disable/enable ASPM
> > before/after link retrain. This clearly states that the controller is quirky and
> > we need to disable/enable ASPM.
> > 
> > For setting OPP, you can have a separate callback in 'pci_host_bridge' that just
> > allows setting OPP *after* retrain, like 'pci_host_bridge:link_change_notify()'.
> > I don't think you really need to set OPP before retrain though. As even if you
> > do it pre and post link retrain, there is still a small window where the link
> > will operate without adequate vote.
> > 
> Hi Mani,
> 
> we need to update the OPP votes before link retrain, for example if
> there is request  to change data rate from 5 GT/s to 8 GT/s on some
> platforms we need to update RPMh votes from low_svs to NOM corner
> without this clocks will not scale for data rates 8 GT/s and link
> retrain will fail. For that reason we are trying to add pre and post
> callbacks.
> 

If we are targetting OPP only, then can't we do direct OPP setting from the
bwctrl driver itself? Or we also need to set ICC votes directly also (in the
non-opp case)?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth before & after link re-training

[Krishna Chaitanya Chundru]

On 8/18/2025 12:39 PM, Manivannan Sadhasivam wrote:
> On Wed, Aug 13, 2025 at 09:25:03AM GMT, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 8/12/2025 10:13 PM, Manivannan Sadhasivam wrote:
>>> On Tue, Aug 12, 2025 at 09:35:46AM GMT, Krishna Chaitanya Chundru wrote:
>>>>
>>>>
>>>> On 7/22/2025 4:33 PM, Krishna Chaitanya Chundru wrote:
>>>>>
>>>>>
>>>>> On 7/12/2025 4:36 AM, Krishna Chaitanya Chundru wrote:
>>>>>>
>>>>>>
>>>>>> On 7/12/2025 3:06 AM, Bjorn Helgaas wrote:
>>>>>>> On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya
>>>>>>> Chundru wrote:
>>>>>>>> If the driver wants to move to higher data rate/speed than
>>>>>>>> the current data
>>>>>>>> rate then the controller driver may need to change certain
>>>>>>>> votes so that
>>>>>>>> link may come up at requested data rate/speed like QCOM PCIe
>>>>>>>> controllers
>>>>>>>> need to change their RPMh (Resource Power Manager-hardened) state. Once
>>>>>>>> link retraining is done controller drivers needs to adjust their votes
>>>>>>>> based on the final data rate.
>>>>>>>>
>>>>>>>> Some controllers also may need to update their bandwidth voting like
>>>>>>>> ICC BW votings etc.
>>>>>>>>
>>>>>>>> So, add pre_link_speed_change() & post_link_speed_change() op to call
>>>>>>>> before & after the link re-train. There is no explicit
>>>>>>>> locking mechanisms
>>>>>>>> as these are called by a single client Endpoint driver.
>>>>>>>>
>>>>>>>> In case of PCIe switch, if there is a request to change
>>>>>>>> target speed for a
>>>>>>>> downstream port then no need to call these function ops as these are
>>>>>>>> outside the scope of the controller drivers.
>>>>>>>
>>>>>>>> +++ b/include/linux/pci.h
>>>>>>>> @@ -599,6 +599,24 @@ struct pci_host_bridge {
>>>>>>>>         void (*release_fn)(struct pci_host_bridge *);
>>>>>>>>         int (*enable_device)(struct pci_host_bridge *bridge,
>>>>>>>> struct pci_dev *dev);
>>>>>>>>         void (*disable_device)(struct pci_host_bridge *bridge,
>>>>>>>> struct pci_dev *dev);
>>>>>>>> +    /*
>>>>>>>> +     * Callback to the host bridge drivers to update ICC BW
>>>>>>>> votes, clock
>>>>>>>> +     * frequencies etc.. for the link re-train to come up
>>>>>>>> in targeted speed.
>>>>>>>> +     * These are intended to be called by devices directly
>>>>>>>> attached to the
>>>>>>>> +     * Root Port. These are called by a single client
>>>>>>>> Endpoint driver, so
>>>>>>>> +     * there is no need for explicit locking mechanisms.
>>>>>>>> +     */
>>>>>>>> +    int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
>>>>>>>> +                     struct pci_dev *dev, int speed);
>>>>>>>> +    /*
>>>>>>>> +     * Callback to the host bridge drivers to adjust ICC BW
>>>>>>>> votes, clock
>>>>>>>> +     * frequencies etc.. to the updated speed after link
>>>>>>>> re-train. These
>>>>>>>> +     * are intended to be called by devices directly attached to the
>>>>>>>> +     * Root Port. These are called by a single client Endpoint driver,
>>>>>>>> +     * so there is no need for explicit locking mechanisms.
>>>>>>>
>>>>>>> No need to repeat the entire comment.  s/.././
>>>>>>>
>>>>>>> These pointers feel awfully specific for being in struct
>>>>>>> pci_host_bridge, since we only need them for a questionable QCOM
>>>>>>> controller.  I think this needs to be pushed down into qcom somehow as
>>>>>>> some kind of quirk.
>>>>>>>
>>>>>> Currently these are needed by QCOM controllers, but it may also needed
>>>>>> by other controllers may also need these for updating ICC votes, any
>>>>>> system level votes, clock frequencies etc.
>>>>>> QCOM controllers is also doing one extra step in these functions to
>>>>>> disable and enable ASPM only as it cannot link speed change support
>>>>>> with ASPM enabled.
>>>>>>
>>>>> Bjorn, can you check this.
>>>>>
>>>>> For QCOM devices we need to update the RPMh vote i.e a power source
>>>>> votes for the link to come up in required speed. and also we need
>>>>> to update interconnect votes also. This will be applicable for
>>>>> other vendors also.
>>>>>
>>>>> If this is not correct place I can add them in the pci_ops.
>>>> Bjorn,
>>>>
>>>> Can you please comment on this.
>>>>
>>>> Is this fine to move these to the pci_ops of the bridge.
>>>> Again these are not specific to QCOM, any controller driver which
>>>> needs to change their clock rates, ICC bw votes etc needs to have
>>>> these.
>>>>
>>>
>>> No, moving to 'pci_ops' is terrible than having it in 'pci_host_bridge' IMO. If
>>> we want to get rid of these ops, we can introduce a quirk flag in
>>> 'pci_host_bridge' and when set, the bwctrl code can disable/enable ASPM
>>> before/after link retrain. This clearly states that the controller is quirky and
>>> we need to disable/enable ASPM.
>>>
>>> For setting OPP, you can have a separate callback in 'pci_host_bridge' that just
>>> allows setting OPP *after* retrain, like 'pci_host_bridge:link_change_notify()'.
>>> I don't think you really need to set OPP before retrain though. As even if you
>>> do it pre and post link retrain, there is still a small window where the link
>>> will operate without adequate vote.
>>>
>> Hi Mani,
>>
>> we need to update the OPP votes before link retrain, for example if
>> there is request  to change data rate from 5 GT/s to 8 GT/s on some
>> platforms we need to update RPMh votes from low_svs to NOM corner
>> without this clocks will not scale for data rates 8 GT/s and link
>> retrain will fail. For that reason we are trying to add pre and post
>> callbacks.
>>
> 
> If we are targetting OPP only, then can't we do direct OPP setting from the
> bwctrl driver itself? Or we also need to set ICC votes directly also (in the
> non-opp case)?
we are targeting only OPP, I can directly call OPP API's from bwctrl
driver.

As recommended, I will try to introduce a quirk flag in pci_host_bridge
to enable and disable ASPM before & after retrain. And will call OPP
API's in bwctrl driver itself.

- Krishna Chaitanya.
> 
> - Mani
>