Re: [RFC 2/3] USB: dwc3: qcom: Add support for firmware managed resources

[Sriram Dash]

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