From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4FA7AEB64DA for ; Wed, 12 Jul 2023 17:23:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231797AbjGLRXJ (ORCPT ); Wed, 12 Jul 2023 13:23:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35606 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232096AbjGLRXI (ORCPT ); Wed, 12 Jul 2023 13:23:08 -0400 Received: from mail-lf1-x130.google.com (mail-lf1-x130.google.com [IPv6:2a00:1450:4864:20::130]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 592461BD for ; Wed, 12 Jul 2023 10:23:05 -0700 (PDT) Received: by mail-lf1-x130.google.com with SMTP id 2adb3069b0e04-4f96d680399so10900974e87.0 for ; Wed, 12 Jul 2023 10:23:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1689182583; x=1689787383; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=XPaxsksQJ1TH6iZEGOUjk8TgWOqMia45twF/E2He4DE=; b=Z80mvnG823CbpEvanfATB6fSBt9Y0OEy0oKSZYTLVjDlVO/aWjj8Wk2tb9VqYEU0Iw GuLkqv2UAXclv/5WEkc+8uR3uT+vanC8IChXwGaDEkySLOlJ80HPL3/gwHjq0BA4Uczi LXyOg0+agBXPBCGsUcYsY15+sdIeUoS1JyC0U4xr3oX2MDsCyB0inKZUzRipetu3PvnZ IfiQhODECE6jw5NbEce2RsLGugS7cNWGeditPOIGGC7s9XlcNGHtMfJAij+yKSg6qAwV JeLMy99cXrS53UeUrf02zVEFBIkBTS94CjmSX0EbZ7qS+HcLpY2VzpMVH98/QuDPKOGc dE2A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689182583; x=1689787383; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=XPaxsksQJ1TH6iZEGOUjk8TgWOqMia45twF/E2He4DE=; b=NXEH0JUrLAntmFxnCEZ+Ulpe9G9ZKzHhya+aiJRfytaYJOBQJP/3S34agHv0fLrvPg 4xGH8JWcZDcGDtGtc9UdDvHRPA37KCJj2nNG47//ZvAqW2sJOo61zNKCo/MVd/i90zJL Mzs4jbmfCEL0B/kMLqJdSH9hSW3Vl2pewB344W8Z1eDSwGkAmbsHF6sJi/yno1nIz3Tr 2tpZ7RueuDo/FuWpgtyrVo2Da+2DSMH49+S6TokRascItvQTxZuX3I0BmWroyWo6CeEH WKSyj2BSgdmdovrOT5HaBunhne5DylHpCPmULBu4or0D6gcsrlMa9wv7xd4/eDAeubWF OuZA== X-Gm-Message-State: ABy/qLaRFx6jrhK5Z7utQUd3xO86fgTjo1KEvrrL3VLlR41Dh0kSVsrQ O5n+BhDp7xIBc6vWAzYOUvnW/A== X-Google-Smtp-Source: APBJJlGIZtbg43PJ+1uxHAV+h9svTZSPCEN2YoXLV3LMGXeS2aVo8OZgH6vyuK1ny6pAopBcAE9ihg== X-Received: by 2002:a05:6512:36c5:b0:4fb:8f79:631 with SMTP id e5-20020a05651236c500b004fb8f790631mr15483556lfs.46.1689182583538; Wed, 12 Jul 2023 10:23:03 -0700 (PDT) Received: from ?IPV6:2001:14ba:a0db:1f00::8a5? (dzdqv0yyyyyyyyyyybcwt-3.rev.dnainternet.fi. [2001:14ba:a0db:1f00::8a5]) by smtp.gmail.com with ESMTPSA id l15-20020ac2554f000000b004fb87638fc4sm796860lfk.19.2023.07.12.10.23.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 Jul 2023 10:23:03 -0700 (PDT) Message-ID: Date: Wed, 12 Jul 2023 20:23:02 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH 14/14] scsi: ufs: qcom: Add support for scaling interconnects Content-Language: en-GB To: Manivannan Sadhasivam Cc: vireshk@kernel.org, nm@ti.com, sboyd@kernel.org, myungjoo.ham@samsung.com, kyungmin.park@samsung.com, cw00.choi@samsung.com, andersson@kernel.org, konrad.dybcio@linaro.org, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, quic_asutoshd@quicinc.com, quic_cang@quicinc.com, quic_nitirawa@quicinc.com, quic_narepall@quicinc.com, quic_bhaskarv@quicinc.com, quic_richardp@quicinc.com, quic_nguyenb@quicinc.com, quic_ziqichen@quicinc.com, bmasney@redhat.com, krzysztof.kozlowski@linaro.org References: <20230712103213.101770-1-manivannan.sadhasivam@linaro.org> <20230712103213.101770-19-manivannan.sadhasivam@linaro.org> <107aad9f-40c0-f32b-9f74-6c82ee6785bf@linaro.org> <20230712164114.GH102757@thinkpad> From: Dmitry Baryshkov In-Reply-To: <20230712164114.GH102757@thinkpad> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On 12/07/2023 19:41, Manivannan Sadhasivam wrote: > On Wed, Jul 12, 2023 at 04:22:51PM +0300, Dmitry Baryshkov wrote: >> On 12/07/2023 13:32, Manivannan Sadhasivam wrote: >>> Qcom SoCs require scaling the interconnect paths for proper working of the >>> peripherals connected through interconnects. Even for accessing the UFS >>> controller, someone should setup the interconnect paths. So far, the >>> bootloaders used to setup the interconnect paths before booting linux as >>> they need to access the UFS storage for things like fetching boot firmware. >>> But with the advent of multi boot options, bootloader nowadays like in >>> SA8540p SoC do not setup the interconnect paths at all. >>> >>> So trying to configure UFS in the absence of the interconnect path >>> configuration, results in boot crash. >>> >>> To fix this issue and also to dynamically scale the interconnects (UFS-DDR >>> and CPU-UFS), interconnect API support is added to the Qcom UFS driver. >>> With this support, the interconnect paths are scaled dynamically based on >>> the gear configuration. >>> >>> During the early stage of ufs_qcom_init(), ufs_qcom_icc_init() will setup >>> the paths to max bandwidth to allow configuring the UFS registers. Touching >>> the registers without configuring the icc paths would result in a crash. >>> However, we don't really need to set max vote for the icc paths as any >>> minimal vote would suffice. But the max value would allow initialization to >>> be done faster. After init, the bandwidth will get updated using >>> ufs_qcom_icc_update_bw() based on the gear and lane configuration. >>> >>> The bandwidth values defined in ufs_qcom_bw_table struct are taken from >>> Qcom downstream vendor devicetree source and are calculated as per the >>> UFS3.1 Spec, Section 6.4.1, HS Gear Rates. So it is fixed across platforms. >>> >>> Cc: Brian Masney >>> Signed-off-by: Manivannan Sadhasivam >>> --- >>> drivers/ufs/host/ufs-qcom.c | 131 +++++++++++++++++++++++++++++++++++- >>> drivers/ufs/host/ufs-qcom.h | 3 + >>> 2 files changed, 133 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >>> index 8d6fd4c3324f..8a3132d45a65 100644 >>> --- a/drivers/ufs/host/ufs-qcom.c >>> +++ b/drivers/ufs/host/ufs-qcom.c >>> @@ -7,6 +7,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -46,6 +47,49 @@ enum { >>> TSTBUS_MAX, >>> }; >>> +#define QCOM_UFS_MAX_GEAR 4 >>> +#define QCOM_UFS_MAX_LANE 2 >>> + >>> +enum { >>> + MODE_MIN, >>> + MODE_PWM, >>> + MODE_HS_RA, >>> + MODE_HS_RB, >>> + MODE_MAX, >> >> MODE_MIN and MODE_MAX seem to be unused >> > > No, they are used in the driver. Ack, missed that. Will comment inline. > >>> +}; >>> + >>> +struct __ufs_qcom_bw_table { >>> + u32 bw1; >>> + u32 bw2; >> >> Please consider: >> >> s/bw1/mem_bw/ >> s/bw2/cfg_bw/ >> > > Ok. > >>> +} ufs_qcom_bw_table[MODE_MAX + 1][QCOM_UFS_MAX_GEAR + 1][QCOM_UFS_MAX_LANE + 1] = { >> >> I'd say, these +1's are slightly confusing and unnecessary. >> > > These max values itself are used in the table. Without adding 1 to them, we will > end up with out of bounds access. > >>> + [MODE_MIN][0][0] = { 0, 0 }, /* Bandwidth values are in KB/s */ >> >> I'd say, this becomes impossible to check. Can you please structure it? > > What do you mean by "impossible to check"? > >> Either by inclusion: >> >> [MODE_PWM] = { >> [1] = { >> [1] = { .. }, >> [2] = { .. }, >> }, >> // etc. > > Now, this seems confusion to me. The existing table looks fine IMO. Fine with me then, your call! > >> }; >> >> Also, do we have defines for gears? Can we use them instead of indices? >> > > There are defines for the gears but not for lanes. So I ended up using numbers > for simplicity. My suggestion would be to use them for gears at least. Then it becomes cleaner (and maybe will solve some of my other comments). > > - Mani > >>> + [MODE_PWM][1][1] = { 922, 1000 }, >>> + [MODE_PWM][2][1] = { 1844, 1000 }, >>> + [MODE_PWM][3][1] = { 3688, 1000 }, >>> + [MODE_PWM][4][1] = { 7376, 1000 }, >>> + [MODE_PWM][1][2] = { 1844, 1000 }, >>> + [MODE_PWM][2][2] = { 3688, 1000 }, >>> + [MODE_PWM][3][2] = { 7376, 1000 }, >>> + [MODE_PWM][4][2] = { 14752, 1000 }, >>> + [MODE_HS_RA][1][1] = { 127796, 1000 }, >>> + [MODE_HS_RA][2][1] = { 255591, 1000 }, >>> + [MODE_HS_RA][3][1] = { 1492582, 102400 }, >>> + [MODE_HS_RA][4][1] = { 2915200, 204800 }, >>> + [MODE_HS_RA][1][2] = { 255591, 1000 }, >>> + [MODE_HS_RA][2][2] = { 511181, 1000 }, >>> + [MODE_HS_RA][3][2] = { 1492582, 204800 }, >>> + [MODE_HS_RA][4][2] = { 2915200, 409600 }, >>> + [MODE_HS_RB][1][1] = { 149422, 1000 }, >>> + [MODE_HS_RB][2][1] = { 298189, 1000 }, >>> + [MODE_HS_RB][3][1] = { 1492582, 102400 }, >>> + [MODE_HS_RB][4][1] = { 2915200, 204800 }, >>> + [MODE_HS_RB][1][2] = { 298189, 1000 }, >>> + [MODE_HS_RB][2][2] = { 596378, 1000 }, >>> + [MODE_HS_RB][3][2] = { 1492582, 204800 }, >>> + [MODE_HS_RB][4][2] = { 2915200, 409600 }, >>> + [MODE_MAX][0][0] = { 7643136, 307200 }, >>> +}; >>> + >>> static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS]; >>> static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host); >>> @@ -789,6 +833,51 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable) >>> } >>> } >>> +static int ufs_qcom_icc_set_bw(struct ufs_qcom_host *host, u32 bw1, u32 bw2) >>> +{ >>> + struct device *dev = host->hba->dev; >>> + int ret; >>> + >>> + ret = icc_set_bw(host->icc_ddr, 0, bw1); >>> + if (ret < 0) { >>> + dev_err(dev, "failed to set bandwidth request: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + ret = icc_set_bw(host->icc_cpu, 0, bw2); >>> + if (ret < 0) { >>> + dev_err(dev, "failed to set bandwidth request: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static struct __ufs_qcom_bw_table ufs_qcom_get_bw_table(struct ufs_qcom_host *host) >>> +{ >>> + struct ufs_pa_layer_attr *p = &host->dev_req_params; >>> + int gear = max_t(u32, p->gear_rx, p->gear_tx); >>> + int lane = max_t(u32, p->lane_rx, p->lane_tx); >>> + >>> + if (ufshcd_is_hs_mode(p)) { >>> + if (p->hs_rate == PA_HS_MODE_B) >>> + return ufs_qcom_bw_table[MODE_HS_RB][gear][lane]; >>> + else >>> + return ufs_qcom_bw_table[MODE_HS_RA][gear][lane]; >>> + } else { >>> + return ufs_qcom_bw_table[MODE_PWM][gear][lane]; >>> + } >>> +} >>> + >>> +static int ufs_qcom_icc_update_bw(struct ufs_qcom_host *host) >>> +{ >>> + struct __ufs_qcom_bw_table bw_table; >>> + >>> + bw_table = ufs_qcom_get_bw_table(host); >>> + >>> + return ufs_qcom_icc_set_bw(host, bw_table.bw1, bw_table.bw2); >>> +} >>> + >>> static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, >>> enum ufs_notify_change_status status, >>> struct ufs_pa_layer_attr *dev_max_params, >>> @@ -852,6 +941,8 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba, >>> memcpy(&host->dev_req_params, >>> dev_req_params, sizeof(*dev_req_params)); >>> + ufs_qcom_icc_update_bw(host); >>> + >>> /* disable the device ref clock if entered PWM mode */ >>> if (ufshcd_is_hs_mode(&hba->pwr_info) && >>> !ufshcd_is_hs_mode(dev_req_params)) >>> @@ -981,7 +1072,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on, >>> switch (status) { >>> case PRE_CHANGE: >>> - if (!on) { >>> + if (on) { >>> + ufs_qcom_icc_update_bw(host); >>> + } else { >>> if (!ufs_qcom_is_link_active(hba)) { >>> /* disable device ref_clk */ >>> ufs_qcom_dev_ref_clk_ctrl(host, false); >>> @@ -993,6 +1086,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on, >>> /* enable the device ref clock for HS mode*/ >>> if (ufshcd_is_hs_mode(&hba->pwr_info)) >>> ufs_qcom_dev_ref_clk_ctrl(host, true); >>> + } else { >>> + ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MIN][0][0].bw1, >>> + ufs_qcom_bw_table[MODE_MIN][0][0].bw2); With MODE_MIN values being initialised to 0, can we use the value directly instead? You are not defining the whole table for MODE_MIN anyway. >>> } >>> break; >>> } >>> @@ -1031,6 +1127,34 @@ static const struct reset_control_ops ufs_qcom_reset_ops = { >>> .deassert = ufs_qcom_reset_deassert, >>> }; >>> +static int ufs_qcom_icc_init(struct ufs_qcom_host *host) >>> +{ >>> + struct device *dev = host->hba->dev; >>> + int ret; >>> + >>> + host->icc_ddr = devm_of_icc_get(dev, "ufs-ddr"); >>> + if (IS_ERR(host->icc_ddr)) >>> + return dev_err_probe(dev, PTR_ERR(host->icc_ddr), >>> + "failed to acquire interconnect path\n"); >>> + >>> + host->icc_cpu = devm_of_icc_get(dev, "cpu-ufs"); >>> + if (IS_ERR(host->icc_cpu)) >>> + return dev_err_probe(dev, PTR_ERR(host->icc_cpu), >>> + "failed to acquire interconnect path\n"); >>> + >>> + /* >>> + * Set Maximum bandwidth vote before initializing the UFS controller and >>> + * device. Ideally, a minimal interconnect vote would suffice for the >>> + * initialization, but a max vote would allow faster initialization. >>> + */ >>> + ret = ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MAX][0][0].bw1, >>> + ufs_qcom_bw_table[MODE_MAX][0][0].bw2); If this is a static value, can we define it separately? You don't have a full gear/lanes enumeration anyway. >>> + if (ret < 0) >>> + return dev_err_probe(dev, ret, "failed to set bandwidth request\n"); >>> + >>> + return 0; >>> +} >>> + >>> /** >>> * ufs_qcom_init - bind phy with controller >>> * @hba: host controller instance >>> @@ -1085,6 +1209,10 @@ static int ufs_qcom_init(struct ufs_hba *hba) >>> } >>> } >>> + err = ufs_qcom_icc_init(host); >>> + if (err) >>> + goto out_variant_clear; >>> + >>> host->device_reset = devm_gpiod_get_optional(dev, "reset", >>> GPIOD_OUT_HIGH); >>> if (IS_ERR(host->device_reset)) { >>> @@ -1282,6 +1410,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, >>> dev_req_params->pwr_rx, >>> dev_req_params->hs_rate, >>> false); >>> + ufs_qcom_icc_update_bw(host); >>> ufshcd_uic_hibern8_exit(hba); >>> } >>> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h >>> index 6289ad5a42d0..dc27395ecba1 100644 >>> --- a/drivers/ufs/host/ufs-qcom.h >>> +++ b/drivers/ufs/host/ufs-qcom.h >>> @@ -206,6 +206,9 @@ struct ufs_qcom_host { >>> struct clk *tx_l1_sync_clk; >>> bool is_lane_clks_enabled; >>> + struct icc_path *icc_ddr; >>> + struct icc_path *icc_cpu; >>> + >>> #ifdef CONFIG_SCSI_UFS_CRYPTO >>> struct qcom_ice *ice; >>> #endif >> >> -- >> With best wishes >> Dmitry >> > -- With best wishes Dmitry