Received-SPF: pass (google.com: domain of linux-kernel+bounces-94256-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <b3d925b8-62b5-4745-8e4b-d94a5b23346a@quicinc.com>
Date: Wed, 6 Mar 2024 21:51:31 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC 2/3] USB: dwc3: qcom: Add support for firmware managed
 resources
Content-Language: en-US
To: Bjorn Andersson <andersson@kernel.org>
CC: <konrad.dybcio@linaro.org>, <vkoul@kernel.org>, <kishon@kernel.org>,
        <robh@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <conor+dt@kernel.org>, <gregkh@linuxfoundation.org>,
        <quic_wcheng@quicinc.com>, <Thinh.Nguyen@synopsys.com>,
        <p.zabel@pengutronix.de>, <linux-arm-msm@vger.kernel.org>,
        <linux-phy@lists.infradead.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-usb@vger.kernel.org>,
        <quic_psodagud@quicinc.com>, <quic_nkela@quicinc.com>,
        <manivannan.sadhasivam@linaro.org>, <ulf.hansson@linaro.org>,
        <sudeep.holla@arm.com>, <quic_shazhuss@quicinc.com>
References: <1709657858-8563-1-git-send-email-quic_sriramd@quicinc.com>
 <1709657858-8563-3-git-send-email-quic_sriramd@quicinc.com>
 <ltjrdqxvupzjdqa22fvpzndeh7pc7zfmi5ybqxu2izjnnxjon7@jojqkltzukvv>
From: Sriram Dash <quic_sriramd@quicinc.com>
In-Reply-To: <ltjrdqxvupzjdqa22fvpzndeh7pc7zfmi5ybqxu2izjnnxjon7@jojqkltzukvv>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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@quicinc.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