Message ID | 20240119185537.3091366-11-echanude@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-31506-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:2bc4:b0:101:a8e8:374 with SMTP id hx4csp1216611dyb; Fri, 19 Jan 2024 11:05:41 -0800 (PST) X-Google-Smtp-Source: AGHT+IF3VhWvAPoR4Yd0Rv+TB84ji+V23087dv82YYLkBEunjFQBs3H0l9Ip6sjNot+SvCieL5HM X-Received: by 2002:a05:6a21:3103:b0:19b:68d6:47eb with SMTP id yz3-20020a056a21310300b0019b68d647ebmr2097464pzb.12.1705691141404; Fri, 19 Jan 2024 11:05:41 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1705691141; cv=pass; d=google.com; s=arc-20160816; b=syY9QO+FTdr53/2V+g9uIwEw+3M+pZcVShqbsbYy7bx0NmT8E8VsCo+Uw33FfiNgEL SliJthdBx8v1CkMbvq1+c16SE4X8A3zGQfN28l1TOZdsF6kCz12gNMi/xxcXlltQNzNL remMCZzBlUxfPGDgCMrGVyW7bT5opRQeq86D8sn1lGgtASm+GasQ3rcThZmgWA037NiW aE0Fp9c3Q5Ppi2nP5TUdOvDjeBWVUbBQjHiohGDgqe6y4g4qPqJEAGxCxmXYh1ehiaRL ViH5lZ/87PJ6C9NCLWiKaI4XbE/dhq5Z5Nxyy7nfWaBbkJLTYvDoMcGq/hM/zVQE8hgQ jqpA== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=h7dgBmNFNgtTu8p/Tt/5S3Mre4JNaLCa+imUc7DnV5Q=; fh=YN5riroVEdnCqzWiknyQJI5I81fIZkVpvouWHSl4Gxg=; b=uErmF5nqkFAwJcRQbron9/oSxgr4xu8Lwqsk4W8geCJNi7/0xR9Mo1LGWMC+97a0o5 4dyCydn22Cy7JSFxvepNXV+Nl5gz3Wsro8tdLGd/eOA5ikbeOPDTc8Y/h/gKjnYXNgS5 O5uS2agTgz8/E+lxfdu0QdLbb2vW3Dz717NUbT4RBzgWR0QGIizFYguSkJtPnUnnVzMT 45PoiMSTkMJMvBPGWXIQTDec9M53OMPrOAPJrJjLIAn6zBNFZfrcM1ygeluoWfgtQY0c 8JUqwVvc7ZSerouegMpUNvZ0Ptgmj99g2V+GZ2uYubqW5uZ/9kH8tg1Dp0w33ucsawDu 23sg== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ejtkZp7L; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-31506-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-31506-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [139.178.88.99]) by mx.google.com with ESMTPS id l7-20020a635707000000b005cecc6097f2si3769562pgb.895.2024.01.19.11.05.41 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 Jan 2024 11:05:41 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-31506-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ejtkZp7L; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-31506-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-31506-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 98332286C09 for <ouuuleilei@gmail.com>; Fri, 19 Jan 2024 19:05:38 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id B844456477; Fri, 19 Jan 2024 19:05:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="ejtkZp7L" Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7B7CC54F87 for <linux-kernel@vger.kernel.org>; Fri, 19 Jan 2024 19:05:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705691122; cv=none; b=oKXCb+ZSqS+Ty0POHvMutmYYgIjqlDBDQyRxNlLH1LCG73X8f/ZsovC7FFr+34rWnAPr5J37oxCiPTRlTglgDGdT3V+IgYwXR1QoQH+9emsuZy2VTtCnpDIV+PIzaBjxPA6Q86+MGckuiCxA/TmE7jiC0ZCXpMBSXZEk6hGaWZA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705691122; c=relaxed/simple; bh=i3mHQi3y8X+GXccdvNGpfS2jPcXaEZnke1U5uVd1P+U=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version:Content-type; b=JDzCkvP/0XdSXkKjD7POToZ+ozGsTTZb70mMV7SxjNbYiwLXt96w3KIZDMKmiQncqAodlmbNewGV9fYZ6bqaTR7RaYdGAJfes7wbAL5q+I6Rl7YaZUvPtEGHDbfPJrDiU37EK0++oCO7BWrCIEW2FAgNFcOsnJasWyleezmAkUw= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=ejtkZp7L; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705691119; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=h7dgBmNFNgtTu8p/Tt/5S3Mre4JNaLCa+imUc7DnV5Q=; b=ejtkZp7LSRCp4+bA3Hn/7M04SEIsY6kfef0mnYhXJ9dnMKyswNvRlyI6gVycoWOBYq/hnd w/nbC+ZL9scxAP70HKwgVMxAQowK9k3BFxcgFaYAjyJG1GcwFJcoi68tx4Ic27ntDkwx2b nWhIuw0GgQZTxmUB3FndvqkEl//OcNk= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-509-pDQZMomTPJu0Bz9vNI3p3Q-1; Fri, 19 Jan 2024 14:05:18 -0500 X-MC-Unique: pDQZMomTPJu0Bz9vNI3p3Q-1 Received: by mail-qk1-f199.google.com with SMTP id af79cd13be357-7831aaa797aso202323085a.1 for <linux-kernel@vger.kernel.org>; Fri, 19 Jan 2024 11:05:18 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705691117; x=1706295917; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=h7dgBmNFNgtTu8p/Tt/5S3Mre4JNaLCa+imUc7DnV5Q=; b=hAjWZFPN8Hq5qChTCdLjapD6MUnpRbz18VKNHMftE7YNZbrBj+tDz7S2yemg8clowJ gDspZTgx7D6piiS41vgnqRZwAlfSW+VPHi+oTHNbO/GwxMTzRKZngDLIcQO70lbqJUiP SLqFZ9FzfgRr3NEr3ZgVZLYPyQ9oN2NNlZ4Fx5VSAlbMUSzXN7zP8zjIZRrTCTWLjMl9 YmT2QyWQEAodJ5oaXl/F+yqfhGajqnGsELd08+WImVxW6PTk7VYoyqCcSjgj4N2/DIas iHdnoBB1uupeeBq+JTYzDKeNv3p3DM2ZfnA9aVrCpcIbbzNx/GR5J9g0OGRkywp40lpF 7h4g== X-Gm-Message-State: AOJu0Yxhnb9Lc2JVH+EK+TFMLlHdOkCPX1ilyp6+N+OJaGSpzk6uz+5O Y4f9sPJJU+pL0bOBIUggxDvt5hxBN82EjxZ63er+3U4caCgNj1aQKyuTO8BZNO9ZVq9MTIh/BAg hTXsrJyGtC+avYUra/AkZ38efYwY0sE/bR0TURjQFL512JuplxsK4XsjVU1jNfw== X-Received: by 2002:a05:620a:1452:b0:781:b188:c7ec with SMTP id i18-20020a05620a145200b00781b188c7ecmr2179294qkl.74.1705691117379; Fri, 19 Jan 2024 11:05:17 -0800 (PST) X-Received: by 2002:a05:620a:1452:b0:781:b188:c7ec with SMTP id i18-20020a05620a145200b00781b188c7ecmr2179278qkl.74.1705691117114; Fri, 19 Jan 2024 11:05:17 -0800 (PST) Received: from localhost (pool-71-184-142-128.bstnma.fios.verizon.net. [71.184.142.128]) by smtp.gmail.com with ESMTPSA id hj11-20020a05622a620b00b00428346b88bfsm7912081qtb.65.2024.01.19.11.05.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 Jan 2024 11:05:16 -0800 (PST) From: Eric Chanudet <echanude@redhat.com> To: Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>, "James E.J. Bottomley" <jejb@linux.ibm.com>, "Martin K. Petersen" <martin.petersen@oracle.com> Cc: linux-arm-msm@vger.kernel.org, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, Eric Chanudet <echanude@redhat.com> Subject: [PATCH] scsi: ufs: qcom: avoid re-init quirk when gears match Date: Fri, 19 Jan 2024 13:55:47 -0500 Message-ID: <20240119185537.3091366-11-echanude@redhat.com> X-Mailer: git-send-email 2.43.0 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-type: text/plain Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1788546794150826382 X-GMAIL-MSGID: 1788546794150826382 |
Series |
scsi: ufs: qcom: avoid re-init quirk when gears match
|
|
Commit Message
Eric Chanudet
Jan. 19, 2024, 6:55 p.m. UTC
On sa8775p-ride, probing the hba will go through the
UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH path although the power info
are same during the second init.
If the host is at least v4, ufs_qcom_get_hs_gear() picked the highest
supported gear when setting the host_params. After the negotiation, if
the host and device are on the same gear, it is the highest gear
supported between the two. Skip the re-init to save some time.
Signed-off-by: Eric Chanudet <echanude@redhat.com>
---
"trace_event=ufs:ufshcd_init" reports the time spent where the re-init
quirk is performed. On sa8775p-ride:
Baseline:
0.355879: ufshcd_init: 1d84000.ufs: took 103377 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0
With this patch:
0.297676: ufshcd_init: 1d84000.ufs: took 43553 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0
drivers/ufs/host/ufs-qcom.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Comments
On Fri, Jan 19, 2024 at 01:55:47PM -0500, Eric Chanudet wrote: > On sa8775p-ride, probing the hba will go through the > UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH path although the power info > are same during the second init. > > If the host is at least v4, ufs_qcom_get_hs_gear() picked the highest > supported gear when setting the host_params. After the negotiation, if > the host and device are on the same gear, it is the highest gear > supported between the two. Skip the re-init to save some time. > > Signed-off-by: Eric Chanudet <echanude@redhat.com> > --- > > "trace_event=ufs:ufshcd_init" reports the time spent where the re-init > quirk is performed. On sa8775p-ride: > Baseline: > 0.355879: ufshcd_init: 1d84000.ufs: took 103377 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0 > With this patch: > 0.297676: ufshcd_init: 1d84000.ufs: took 43553 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0 > > drivers/ufs/host/ufs-qcom.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 39eef470f8fa..f9f161340e78 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -738,8 +738,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > * the second init can program the optimal PHY settings. This allows one to start > * the first init with either the minimum or the maximum support gear. > */ > - if (hba->ufshcd_state == UFSHCD_STATE_RESET) > + if (hba->ufshcd_state == UFSHCD_STATE_RESET) { > + if (host->hw_ver.major >= 0x4 && Is this check really necessary? The initial phy_gear state is something like this (my phrasing of ufs_qcom_set_phy_gear()): if hw_ver < 4: # Comments about powering up with minimum gear (with no # reasoning in the comment afaict), and mentions switching # to higher gear in reinit quirk. This is opposite of the later # versions which start at the max and scale down phy_gear = UFS_HS_G2 else if hw_ver == 4: phy_gear = hs_tx_gear # (so afaict always UFS_HS_G4) else if hw_ver >= 5: phy_gear = hs_tx_gear # (What ever the max is for this version) if dev_major: # Clears the reinit quirk in ufs_qcom_set_phy_gear() if the # device version is provided by bootloader / controller # because we already found it out and can init directly # to the ideal gear quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH if dev_major < 4: # Sets gear to UFS_HS_G4 to save power for UFS 3.1 and # older devices phy_gear = UFS_HS_G4 I guess what I'm saying is that I'm not totally seeing how this check is dependent on the controller version. To me, if we're in the ideal gear already and we know it, we should *not* reinit, no matter what the controller version is. That's assuming there's not some other reasoning for the quirk, but as far as I understand it the quirk exists because we have to start with *some* phy gear value so we can talk to the device, and once we discover what the device is capable of it makes sense to scale down (or up for older controllers) to the ideal gear setting for the attached device. Unfortunately to do the change we have to reprogram the phy which I guess is only acceptable if we reprogram everything (hence the reinit). Does that make sense or do you think I'm missing something? I think say even for an older controller this makes sense, if its attached to a UFS_HS_G2 capable device there is no reason to reinit since we started up in the ideal configuration. Thanks, Andrew > + host_params->hs_tx_gear == dev_req_params->gear_tx) > + hba->quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > host->phy_gear = dev_req_params->gear_tx; > + } > > /* enable the device ref clock before changing to HS mode */ > if (!ufshcd_is_hs_mode(&hba->pwr_info) && > -- > 2.43.0 > >
On Fri, Jan 19, 2024 at 02:07:15PM -0600, Andrew Halaney wrote: > On Fri, Jan 19, 2024 at 01:55:47PM -0500, Eric Chanudet wrote: > > On sa8775p-ride, probing the hba will go through the > > UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH path although the power info > > are same during the second init. > > > > If the host is at least v4, ufs_qcom_get_hs_gear() picked the highest > > supported gear when setting the host_params. After the negotiation, if > > the host and device are on the same gear, it is the highest gear > > supported between the two. Skip the re-init to save some time. > > > > Signed-off-by: Eric Chanudet <echanude@redhat.com> > > --- > > > > "trace_event=ufs:ufshcd_init" reports the time spent where the re-init > > quirk is performed. On sa8775p-ride: > > Baseline: > > 0.355879: ufshcd_init: 1d84000.ufs: took 103377 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0 > > With this patch: > > 0.297676: ufshcd_init: 1d84000.ufs: took 43553 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0 > > > > drivers/ufs/host/ufs-qcom.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > > index 39eef470f8fa..f9f161340e78 100644 > > --- a/drivers/ufs/host/ufs-qcom.c > > +++ b/drivers/ufs/host/ufs-qcom.c > > @@ -738,8 +738,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > > * the second init can program the optimal PHY settings. This allows one to start > > * the first init with either the minimum or the maximum support gear. > > */ > > - if (hba->ufshcd_state == UFSHCD_STATE_RESET) > > + if (hba->ufshcd_state == UFSHCD_STATE_RESET) { > > + if (host->hw_ver.major >= 0x4 && > > Is this check really necessary? > > The initial phy_gear state is something like this (my phrasing of > ufs_qcom_set_phy_gear()): > > if hw_ver < 4: > # Comments about powering up with minimum gear (with no > # reasoning in the comment afaict), and mentions switching > # to higher gear in reinit quirk. This is opposite of the later > # versions which start at the max and scale down > phy_gear = UFS_HS_G2 > > else if hw_ver == 4: > phy_gear = hs_tx_gear # (so afaict always UFS_HS_G4) > > else if hw_ver >= 5: > phy_gear = hs_tx_gear # (What ever the max is for this version) > > if dev_major: > # Clears the reinit quirk in ufs_qcom_set_phy_gear() if the > # device version is provided by bootloader / controller > # because we already found it out and can init directly > # to the ideal gear > quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH > > if dev_major < 4: > # Sets gear to UFS_HS_G4 to save power for UFS 3.1 and > # older devices > phy_gear = UFS_HS_G4 > > I guess what I'm saying is that I'm not totally seeing how this check > is dependent on the controller version. To me, if we're in the ideal > gear already and we know it, we should *not* reinit, no matter what the > controller version is. That's assuming there's not some other reasoning > for the quirk, but as far as I understand it the quirk exists because we > have to start with *some* phy gear value so we can talk to the device, > and once we discover what the device is capable of it makes sense to > scale down (or up for older controllers) to the ideal gear setting for > the attached device. Unfortunately to do the change we have to > reprogram the phy which I guess is only acceptable if we reprogram > everything (hence the reinit). > > Does that make sense or do you think I'm missing something? I think say > even for an older controller this makes sense, if its attached to a > UFS_HS_G2 capable device there is no reason to reinit since we started > up in the ideal configuration. I guess what I'm saying is shouldn't the check be something like: if (dev_req_params->gear_tx == host->phy_gear) { // Skip reinit since we started up in the agreed upon gear hba->quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; } Which is independent of the controller version? > > Thanks, > Andrew > > > + host_params->hs_tx_gear == dev_req_params->gear_tx) > > + hba->quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > > host->phy_gear = dev_req_params->gear_tx; > > + } > > > > /* enable the device ref clock before changing to HS mode */ > > if (!ufshcd_is_hs_mode(&hba->pwr_info) && > > -- > > 2.43.0 > > > >
On Fri, Jan 19, 2024 at 02:33:32PM -0600, Andrew Halaney wrote: > On Fri, Jan 19, 2024 at 02:07:15PM -0600, Andrew Halaney wrote: > > On Fri, Jan 19, 2024 at 01:55:47PM -0500, Eric Chanudet wrote: > > > On sa8775p-ride, probing the hba will go through the > > > UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH path although the power info > > > are same during the second init. > > > > > > If the host is at least v4, ufs_qcom_get_hs_gear() picked the highest > > > supported gear when setting the host_params. After the negotiation, if > > > the host and device are on the same gear, it is the highest gear > > > supported between the two. Skip the re-init to save some time. > > > > > > Signed-off-by: Eric Chanudet <echanude@redhat.com> > > > --- > > > > > > "trace_event=ufs:ufshcd_init" reports the time spent where the re-init > > > quirk is performed. On sa8775p-ride: > > > Baseline: > > > 0.355879: ufshcd_init: 1d84000.ufs: took 103377 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0 > > > With this patch: > > > 0.297676: ufshcd_init: 1d84000.ufs: took 43553 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0 > > > > > > drivers/ufs/host/ufs-qcom.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > > > index 39eef470f8fa..f9f161340e78 100644 > > > --- a/drivers/ufs/host/ufs-qcom.c > > > +++ b/drivers/ufs/host/ufs-qcom.c > > > @@ -738,8 +738,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > > > * the second init can program the optimal PHY settings. This allows one to start > > > * the first init with either the minimum or the maximum support gear. > > > */ > > > - if (hba->ufshcd_state == UFSHCD_STATE_RESET) > > > + if (hba->ufshcd_state == UFSHCD_STATE_RESET) { > > > + if (host->hw_ver.major >= 0x4 && > > > > Is this check really necessary? I *think* so. For example, if hw_ver < 4, ufs_qcom_set_phy_gear() has a comment saying "power up the PHY using minimum supported gear (UFS_HS_G2). Switching to max gear will be performed during reinit if supported." > > > > The initial phy_gear state is something like this (my phrasing of > > ufs_qcom_set_phy_gear()): > > > > if hw_ver < 4: > > # Comments about powering up with minimum gear (with no > > # reasoning in the comment afaict), and mentions switching > > # to higher gear in reinit quirk. This is opposite of the later > > # versions which start at the max and scale down > > phy_gear = UFS_HS_G2 IIUC, the device would not be able to negotiate a gear higher than the minimum set for the phy_gear on initialization. ufshcd_init_host_params() and ufs_qcom_get_hs_gear() both set the controller <v4 host_params to G3. So if the device is HS capable, the re-init would set G3, instead of the G2 selected by ufs_qcom_set_phy_gear(). Assuming I'm not loosing track somewhere, the sequence of calls would go something like this: ufshcd_init ufshcd_hba_init ufshcd_variant_hba_init ufshcd_vops_init ufs_qcom_init ufs_qcom_set_host_params /* if hw_ver < 4: phy_gear = G2 */ ufshcd_hba_enable ufshcd_hba_execute_hce ufshcd_vops_hce_enable_notify(PRE_CHANGE) ufs_qcom_hce_enable_notify /* vops.hce_enable_notify */ ufs_qcom_power_up_sequence phy_set_mode_ext(phy, mode, host->phy_gear); async_schedule(ufshcd_async_scan, hba) ... ufshcd_async_scan ufshcd_device_init ufshcd_probe_hba /* where the re-init quirk happens */ ufshcd_device_init ufshcd_vops_pwr_change_notify(PRE_CHANGE) So that would limit the device to G2? In this circumstances, the re-init would instead re-initialize to G3. > I guess what I'm saying is shouldn't the check be something like: > > if (dev_req_params->gear_tx == host->phy_gear) { > // Skip reinit since we started up in the agreed upon gear > hba->quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > }
On Fri, Jan 19, 2024 at 04:33:10PM -0500, Eric Chanudet wrote: > On Fri, Jan 19, 2024 at 02:33:32PM -0600, Andrew Halaney wrote: > > On Fri, Jan 19, 2024 at 02:07:15PM -0600, Andrew Halaney wrote: > > > On Fri, Jan 19, 2024 at 01:55:47PM -0500, Eric Chanudet wrote: > > > > On sa8775p-ride, probing the hba will go through the > > > > UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH path although the power info > > > > are same during the second init. > > > > > > > > If the host is at least v4, ufs_qcom_get_hs_gear() picked the highest > > > > supported gear when setting the host_params. After the negotiation, if > > > > the host and device are on the same gear, it is the highest gear > > > > supported between the two. Skip the re-init to save some time. > > > > > > > > Signed-off-by: Eric Chanudet <echanude@redhat.com> > > > > --- > > > > > > > > "trace_event=ufs:ufshcd_init" reports the time spent where the re-init > > > > quirk is performed. On sa8775p-ride: > > > > Baseline: > > > > 0.355879: ufshcd_init: 1d84000.ufs: took 103377 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0 > > > > With this patch: > > > > 0.297676: ufshcd_init: 1d84000.ufs: took 43553 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0 > > > > > > > > drivers/ufs/host/ufs-qcom.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > > > > index 39eef470f8fa..f9f161340e78 100644 > > > > --- a/drivers/ufs/host/ufs-qcom.c > > > > +++ b/drivers/ufs/host/ufs-qcom.c > > > > @@ -738,8 +738,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > > > > * the second init can program the optimal PHY settings. This allows one to start > > > > * the first init with either the minimum or the maximum support gear. > > > > */ > > > > - if (hba->ufshcd_state == UFSHCD_STATE_RESET) > > > > + if (hba->ufshcd_state == UFSHCD_STATE_RESET) { > > > > + if (host->hw_ver.major >= 0x4 && > > > > > > Is this check really necessary? > > I *think* so. > > For example, if hw_ver < 4, ufs_qcom_set_phy_gear() has a comment saying > "power up the PHY using minimum supported gear (UFS_HS_G2). Switching to > max gear will be performed during reinit if supported." > > > > > > > The initial phy_gear state is something like this (my phrasing of > > > ufs_qcom_set_phy_gear()): > > > > > > if hw_ver < 4: > > > # Comments about powering up with minimum gear (with no > > > # reasoning in the comment afaict), and mentions switching > > > # to higher gear in reinit quirk. This is opposite of the later > > > # versions which start at the max and scale down > > > phy_gear = UFS_HS_G2 > > IIUC, the device would not be able to negotiate a gear higher than the > minimum set for the phy_gear on initialization. > > ufshcd_init_host_params() and ufs_qcom_get_hs_gear() both set the > controller <v4 host_params to G3. So if the device is HS capable, the > re-init would set G3, instead of the G2 selected by > ufs_qcom_set_phy_gear(). > REINIT quirk is applicable for controllers starting from v4 only, because legacy controllers don't need separate PHY init sequences. So you can get rid of that check. - Mani
On Fri, Jan 19, 2024 at 01:55:47PM -0500, Eric Chanudet wrote: > On sa8775p-ride, probing the hba will go through the > UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH path although the power info > are same during the second init. > > If the host is at least v4, ufs_qcom_get_hs_gear() picked the highest > supported gear when setting the host_params. After the negotiation, if > the host and device are on the same gear, it is the highest gear > supported between the two. Skip the re-init to save some time. > > Signed-off-by: Eric Chanudet <echanude@redhat.com> > --- > > "trace_event=ufs:ufshcd_init" reports the time spent where the re-init > quirk is performed. On sa8775p-ride: > Baseline: > 0.355879: ufshcd_init: 1d84000.ufs: took 103377 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0 > With this patch: > 0.297676: ufshcd_init: 1d84000.ufs: took 43553 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0 > > drivers/ufs/host/ufs-qcom.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 39eef470f8fa..f9f161340e78 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -738,8 +738,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > * the second init can program the optimal PHY settings. This allows one to start > * the first init with either the minimum or the maximum support gear. > */ > - if (hba->ufshcd_state == UFSHCD_STATE_RESET) > + if (hba->ufshcd_state == UFSHCD_STATE_RESET) { > + if (host->hw_ver.major >= 0x4 && You can get rid of this check as I said in the reply. > + host_params->hs_tx_gear == dev_req_params->gear_tx) How about? /* * Skip REINIT if the negotiated gear matches with the * initial phy_gear. Otherwise, update the phy_gear to * program the optimal gear setting during REINIT. */ if (host->phy_gear == dev_req_params->gear_tx) hba->quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; else host->phy_gear = dev_req_params->gear_tx; - Mani > + hba->quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; > host->phy_gear = dev_req_params->gear_tx; > + } > > /* enable the device ref clock before changing to HS mode */ > if (!ufshcd_is_hs_mode(&hba->pwr_info) && > -- > 2.43.0 >
On Tue, Jan 23, 2024 at 08:06:15PM +0530, Manivannan Sadhasivam wrote: > On Fri, Jan 19, 2024 at 04:33:10PM -0500, Eric Chanudet wrote: > > On Fri, Jan 19, 2024 at 02:33:32PM -0600, Andrew Halaney wrote: > > > On Fri, Jan 19, 2024 at 02:07:15PM -0600, Andrew Halaney wrote: > > > > On Fri, Jan 19, 2024 at 01:55:47PM -0500, Eric Chanudet wrote: > > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > > > > > index 39eef470f8fa..f9f161340e78 100644 > > > > > --- a/drivers/ufs/host/ufs-qcom.c > > > > > +++ b/drivers/ufs/host/ufs-qcom.c > > > > > @@ -738,8 +738,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, > > > > > * the second init can program the optimal PHY settings. This allows one to start > > > > > * the first init with either the minimum or the maximum support gear. > > > > > */ > > > > > - if (hba->ufshcd_state == UFSHCD_STATE_RESET) > > > > > + if (hba->ufshcd_state == UFSHCD_STATE_RESET) { > > > > > + if (host->hw_ver.major >= 0x4 && > > > > > > > > Is this check really necessary? > > > > I *think* so. > > > > For example, if hw_ver < 4, ufs_qcom_set_phy_gear() has a comment saying > > "power up the PHY using minimum supported gear (UFS_HS_G2). Switching to > > max gear will be performed during reinit if supported." > > > > > > > > > > The initial phy_gear state is something like this (my phrasing of > > > > ufs_qcom_set_phy_gear()): > > > > > > > > if hw_ver < 4: > > > > # Comments about powering up with minimum gear (with no > > > > # reasoning in the comment afaict), and mentions switching > > > > # to higher gear in reinit quirk. This is opposite of the later > > > > # versions which start at the max and scale down > > > > phy_gear = UFS_HS_G2 > > > > IIUC, the device would not be able to negotiate a gear higher than the > > minimum set for the phy_gear on initialization. > > > > ufshcd_init_host_params() and ufs_qcom_get_hs_gear() both set the > > controller <v4 host_params to G3. So if the device is HS capable, the > > re-init would set G3, instead of the G2 selected by > > ufs_qcom_set_phy_gear(). > > > > REINIT quirk is applicable for controllers starting from v4 only, because legacy > controllers don't need separate PHY init sequences. So you can get rid of that > check. My bad, I overlooked the check in ufs_qcom_advertise_quirks(). Thank you, I'll send a v2 soon. > - Mani > > -- > மணிவண்ணன் சதாசிவம் >
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 39eef470f8fa..f9f161340e78 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -738,8 +738,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, * the second init can program the optimal PHY settings. This allows one to start * the first init with either the minimum or the maximum support gear. */ - if (hba->ufshcd_state == UFSHCD_STATE_RESET) + if (hba->ufshcd_state == UFSHCD_STATE_RESET) { + if (host->hw_ver.major >= 0x4 && + host_params->hs_tx_gear == dev_req_params->gear_tx) + hba->quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH; host->phy_gear = dev_req_params->gear_tx; + } /* enable the device ref clock before changing to HS mode */ if (!ufshcd_is_hs_mode(&hba->pwr_info) &&