[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b3d925b8-62b5-4745-8e4b-d94a5b23346a@quicinc.com>
Date: Wed, 6 Mar 2024 21:51:31 +0530
From: Sriram Dash <quic_sriramd@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: <konrad.dybcio@...aro.org>, <vkoul@...nel.org>, <kishon@...nel.org>,
<robh@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<conor+dt@...nel.org>, <gregkh@...uxfoundation.org>,
<quic_wcheng@...cinc.com>, <Thinh.Nguyen@...opsys.com>,
<p.zabel@...gutronix.de>, <linux-arm-msm@...r.kernel.org>,
<linux-phy@...ts.infradead.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
<quic_psodagud@...cinc.com>, <quic_nkela@...cinc.com>,
<manivannan.sadhasivam@...aro.org>, <ulf.hansson@...aro.org>,
<sudeep.holla@....com>, <quic_shazhuss@...cinc.com>
Subject: Re: [RFC 2/3] USB: dwc3: qcom: Add support for firmware managed
resources
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@...cinc.com>
>> ---
>> 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
Powered by blists - more mailing lists