On 3/6/2024 12:52 AM, Bjorn Andersson wrote:
On Tue, Mar 05, 2024 at 10:27:37PM +0530, Sriram Dash wrote:
Some target systems allow multiple resources to be managed by firmware.
On these targets, tasks related to clocks, regulators, resets, and
interconnects can be delegated to the firmware, while the remaining
responsibilities are handled by Linux.
The driver is responsible for managing multiple power domains and
linking them to consumers as needed. Incase there is only single
power domain, it is considered to be a standard GDSC hooked on to
the qcom dt node which is read and assigned to device structure
(by genpd framework) before the driver probe even begins.
This differentiation logic allows the driver to determine whether
device resources are managed by Linux or firmware, ensuring
backward compatibility.
Furthermore, minor cleanup is performed for the private data of
No "futhermore"s please, separate matters should be proposed as separate
patches. Perhaps these can be sent separately and merged immediately?
Thanks Bjorn.
Will take this separately.
the SNPS Femto PHY. However, ACPI handling is omitted due to the
absence of clients on the ACPI side.
Signed-off-by: Sriram Dash <quic_sriramd@xxxxxxxxxxx>
---
drivers/phy/qualcomm/phy-qcom-qmp-usb.c | 290 ++++++++++++++++++++------
drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 213 +++++++++++++++----
drivers/usb/dwc3/dwc3-qcom.c | 259 +++++++++++++++++------
You're making independent changes across three different drivers across
two different subsystems, with different maintainers, this is not
acceptable as a single patch.
Sure. will split the patches in next version.
3 files changed, 594 insertions(+), 168 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
index 8525393..1ac1b50 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
@@ -21,6 +21,9 @@
#include "phy-qcom-qmp-common.h"
+#include <linux/pm_opp.h>
+#include <linux/pm_domain.h>
Why are these includes alone here? Integrate your changes with the
driver properly.
Sure. will take care in the next version.
+
#include "phy-qcom-qmp.h"
#include "phy-qcom-qmp-pcs-misc-v3.h"
#include "phy-qcom-qmp-pcs-misc-v4.h"
@@ -1212,6 +1215,9 @@ struct qmp_phy_cfg {
unsigned int pcs_usb_offset;
};
+#define DOMAIN_GENPD_TRANSFER 0
+#define DOMAIN_GENPD_CORE 1
Does this really represent the hardware? What hardware constructs does
"transfer" and "core" maps to?
The idea was to club the resources in 2 buckets.
Which are essential for the IP core to be active
(ex : regulators, gdsc ) form the part or genpd core
and the resources which are controlled from Clock cluster
in another bucket, used for transfers.
+
struct qmp_usb {
struct device *dev;
@@ -1236,6 +1242,19 @@ struct qmp_usb {
struct phy *phy;
struct clk_fixed_rate pipe_clk_fixed;
+
+ struct dev_pm_domain_list *pd_list;
+ struct device *genpd_core;
+ struct device *genpd_transfer;
+
+ bool fw_managed;
+ /* separate resource management for fw_managed vs locally managed devices */
+ struct qmp_usb_device_ops {
+ int (*bus_resume_resource)(struct qmp_usb *qmp);
Not only does these function pointers make the drivers much harder to
follow, your naming of these seems chosen to maximize the confusion.
In your managed case this doesn't seem to relate to any "bus", in the
"local" case, this doesn't relate to a "bus", and these callbacks are
decoupled from the actual runtime resume and suspend cycle of the QMP
device itself...
Understood. Will make the decision to use fw managed
method or local management of resources based on the
fw_managed property rather than fixing it to function
pointer.
+ int (*runtime_resume_resource)(struct qmp_usb *qmp);
+ int (*bus_suspend_resource)(struct qmp_usb *qmp);
+ int (*runtime_suspend_resource)(struct qmp_usb *qmp);
+ } qmp_usb_device_ops;
};
static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
@@ -1598,6 +1617,41 @@ static const struct qmp_phy_cfg x1e80100_usb3_uniphy_cfg = {
.regs = qmp_v7_usb3phy_regs_layout,
};
+static void qmp_fw_managed_domain_remove(struct qmp_usb *qmp)
+{
+ dev_pm_domain_detach_list(qmp->pd_list);
+}
+
+static int qmp_fw_managed_domain_init(struct qmp_usb *qmp)
+{
+ struct device *dev = qmp->dev;
+ struct dev_pm_domain_attach_data pd_data = {
+ .pd_flags = PD_FLAG_NO_DEV_LINK,
Iiuc, you attach the two power-domains with NO_DEV_LINK, such that the
pm runtime state of the device itself won't reflect on the power
domains, and then you hand-code all the involved logic yourself?
> Why can't you integrate with the device and use its runtime state?
Please clearly explain why you're doing it like this in your commit
messages.
OK.
Got suggestion from Dmitry to either pass empty list or
dev_pm_domain_attach twice. I will use the later.
Regards,
Bjorn
