| Message ID | 20240329-llcc-broadcast-and-v4-0-107c76fd8ceb@quicinc.com |
|---|---|
| Headers | show |
| Series | LLCC: Support for Broadcast_AND region | expand |
On Fri, Mar 29, 2024 at 02:53:41PM -0700, Unnathi Chalicheemala wrote: > Define new regmap structure for Broadcast_AND region and initialize > this regmap when HW block version is greater than 4.1, otherwise > initialize as a NULL pointer for backwards compatibility. > > Switch from broadcast_OR to broadcast_AND region (when defined in DT) > for checking status bit 1 as Broadcast_OR region checks only for bit 0. > Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com> Regards, Bjorn
On 29/03/2024 22:53, Unnathi Chalicheemala wrote: > This series adds: > 1. Device tree register mapping for Broadcast_AND region in SM8450, > SM8550, SM8650. > 2. LLCC driver updates to reflect addition of Broadcast_AND regmap. > > To support CSR programming, a broadcast interface is used to program all > channels in a single command. Until SM8450 there was only one broadcast > region (Broadcast_OR) used to broadcast write and check for status bit > 0. From SM8450 onwards another broadcast region (Broadcast_AND) has been > added which checks for status bit 1. > > This series updates the device trees from SM8450 onwards to have a > mapping to this Broadcast_AND region. It also updates the llcc_drv_data > structure with a regmap for Broadcast_AND region and corrects the > broadcast region used to check for status bit 1. Your way of sending patches makes it difficult for us to review them. b4 diff -C '<20240329-llcc-broadcast-and-v4-0-107c76fd8ceb@quicinc.com>' Grabbing thread from lore.kernel.org/all/20240329-llcc-broadcast-and-v4-0-107c76fd8ceb@quicinc.com/t.mbox.gz Checking for older revisions Added from v3: 5 patches --- Analyzing 39 messages in the thread Preparing fake-am for v3: dt-bindings: arm: msm: Add llcc Broadcast_AND register ERROR: v3 series incomplete; unable to create a fake-am range --- Could not create fake-am range for lower series v3 Please reach internally within Qualcomm to get some guidance how to properly set up your work environment. Best regards, Krzysztof
On 29/03/2024 22:53, Unnathi Chalicheemala wrote: > Define new regmap structure for Broadcast_AND region and initialize > this regmap when HW block version is greater than 4.1, otherwise > initialize as a NULL pointer for backwards compatibility. > > + struct regmap *regmap; > u32 act_ctrl_reg; > u32 act_clear_reg; > u32 status_reg; > @@ -849,7 +850,8 @@ static int llcc_update_act_ctrl(u32 sid, > return ret; > > if (drv_data->version >= LLCC_VERSION_4_1_0_0) { > - ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg, > + regmap = drv_data->bcast_and_regmap ?: drv_data->bcast_regmap; > + ret = regmap_read_poll_timeout(regmap, status_reg, > slice_status, (slice_status & ACT_COMPLETE), > 0, LLCC_STATUS_READ_DELAY); > if (ret) > @@ -1284,6 +1286,16 @@ static int qcom_llcc_probe(struct platform_device *pdev) > > drv_data->version = version; > > + /* Applicable only when drv_data->version >= 4.1 */ > + drv_data->bcast_and_regmap = qcom_llcc_init_mmio(pdev, i + 1, "llcc_broadcast_and_base"); > + if (IS_ERR(drv_data->bcast_and_regmap)) { I am pretty sure this breaks all users. Can you please explain how do you maintain ABI and that IS_ERR() is applied only for version >= 4.1? Best regards, Krzysztof
On 3/30/2024 4:46 AM, Krzysztof Kozlowski wrote: > On 29/03/2024 22:53, Unnathi Chalicheemala wrote: >> Define new regmap structure for Broadcast_AND region and initialize >> this regmap when HW block version is greater than 4.1, otherwise >> initialize as a NULL pointer for backwards compatibility. >> > >> + struct regmap *regmap; >> u32 act_ctrl_reg; >> u32 act_clear_reg; >> u32 status_reg; >> @@ -849,7 +850,8 @@ static int llcc_update_act_ctrl(u32 sid, >> return ret; >> >> if (drv_data->version >= LLCC_VERSION_4_1_0_0) { >> - ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg, >> + regmap = drv_data->bcast_and_regmap ?: drv_data->bcast_regmap; >> + ret = regmap_read_poll_timeout(regmap, status_reg, >> slice_status, (slice_status & ACT_COMPLETE), >> 0, LLCC_STATUS_READ_DELAY); >> if (ret) >> @@ -1284,6 +1286,16 @@ static int qcom_llcc_probe(struct platform_device *pdev) >> >> drv_data->version = version; >> >> + /* Applicable only when drv_data->version >= 4.1 */ >> + drv_data->bcast_and_regmap = qcom_llcc_init_mmio(pdev, i + 1, "llcc_broadcast_and_base"); >> + if (IS_ERR(drv_data->bcast_and_regmap)) { > > I am pretty sure this breaks all users. Can you please explain how do > you maintain ABI and that IS_ERR() is applied only for version >= 4.1? > > Best regards, > Krzysztof > IS_ERR() check is done for all versions. If new register isn't defined in DT(for version < 4.1) it simply sets bcast_and_regmap to NULL. Otherwise, for version >= 4.1, it goes to err(in the case bcast_and_regmap isn't set properly). Thank you for reviewing Krzysztof!
On 4/2/24 21:34, Unnathi Chalicheemala wrote: > On 3/30/2024 4:46 AM, Krzysztof Kozlowski wrote: >> On 29/03/2024 22:53, Unnathi Chalicheemala wrote: >>> Define new regmap structure for Broadcast_AND region and initialize >>> this regmap when HW block version is greater than 4.1, otherwise >>> initialize as a NULL pointer for backwards compatibility. >>> >> >>> + struct regmap *regmap; >>> u32 act_ctrl_reg; >>> u32 act_clear_reg; >>> u32 status_reg; >>> @@ -849,7 +850,8 @@ static int llcc_update_act_ctrl(u32 sid, >>> return ret; >>> >>> if (drv_data->version >= LLCC_VERSION_4_1_0_0) { >>> - ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg, >>> + regmap = drv_data->bcast_and_regmap ?: drv_data->bcast_regmap; >>> + ret = regmap_read_poll_timeout(regmap, status_reg, >>> slice_status, (slice_status & ACT_COMPLETE), >>> 0, LLCC_STATUS_READ_DELAY); >>> if (ret) >>> @@ -1284,6 +1286,16 @@ static int qcom_llcc_probe(struct platform_device *pdev) >>> >>> drv_data->version = version; >>> >>> + /* Applicable only when drv_data->version >= 4.1 */ >>> + drv_data->bcast_and_regmap = qcom_llcc_init_mmio(pdev, i + 1, "llcc_broadcast_and_base"); >>> + if (IS_ERR(drv_data->bcast_and_regmap)) { >> >> I am pretty sure this breaks all users. Can you please explain how do >> you maintain ABI and that IS_ERR() is applied only for version >= 4.1? >> >> Best regards, >> Krzysztof >> > IS_ERR() check is done for all versions. > If new register isn't defined in DT(for version < 4.1) it simply sets bcast_and_regmap to NULL. > Otherwise, for version >= 4.1, it goes to err(in the case bcast_and_regmap isn't set properly). b4 shazam <this series> booting on 8250, I get: [ 2.794850] qcom-llcc 9200000.system-cache-controller: invalid resource (null) which comes from lib/devres.c : __devm_ioremap_resource() Now, this is gonna get you an angry Johan(+CC) response when he sees this land in the next release. Perhaps, this warning could either be removed from libdevres, or some sort of an _optional variant that doesn't print it could be introduced. Konrad
On 4/10/2024 11:24 AM, Konrad Dybcio wrote: > > > On 4/2/24 21:34, Unnathi Chalicheemala wrote: >> On 3/30/2024 4:46 AM, Krzysztof Kozlowski wrote: >>> On 29/03/2024 22:53, Unnathi Chalicheemala wrote: >>>> Define new regmap structure for Broadcast_AND region and initialize >>>> this regmap when HW block version is greater than 4.1, otherwise >>>> initialize as a NULL pointer for backwards compatibility. >>>> >>> >>>> + struct regmap *regmap; >>>> u32 act_ctrl_reg; >>>> u32 act_clear_reg; >>>> u32 status_reg; >>>> @@ -849,7 +850,8 @@ static int llcc_update_act_ctrl(u32 sid, >>>> return ret; >>>> if (drv_data->version >= LLCC_VERSION_4_1_0_0) { >>>> - ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg, >>>> + regmap = drv_data->bcast_and_regmap ?: drv_data->bcast_regmap; >>>> + ret = regmap_read_poll_timeout(regmap, status_reg, >>>> slice_status, (slice_status & ACT_COMPLETE), >>>> 0, LLCC_STATUS_READ_DELAY); >>>> if (ret) >>>> @@ -1284,6 +1286,16 @@ static int qcom_llcc_probe(struct platform_device *pdev) >>>> drv_data->version = version; >>>> + /* Applicable only when drv_data->version >= 4.1 */ >>>> + drv_data->bcast_and_regmap = qcom_llcc_init_mmio(pdev, i + 1, "llcc_broadcast_and_base"); >>>> + if (IS_ERR(drv_data->bcast_and_regmap)) { >>> >>> I am pretty sure this breaks all users. Can you please explain how do >>> you maintain ABI and that IS_ERR() is applied only for version >= 4.1? >>> >>> Best regards, >>> Krzysztof >>> >> IS_ERR() check is done for all versions. >> If new register isn't defined in DT(for version < 4.1) it simply sets bcast_and_regmap to NULL. >> Otherwise, for version >= 4.1, it goes to err(in the case bcast_and_regmap isn't set properly). > > b4 shazam <this series> > > booting on 8250, I get: > > [ 2.794850] qcom-llcc 9200000.system-cache-controller: invalid resource (null) > > which comes from lib/devres.c : __devm_ioremap_resource() > > Now, this is gonna get you an angry Johan(+CC) response when he sees this land in > the next release. Perhaps, this warning could either be removed from libdevres, > or some sort of an _optional variant that doesn't print it could be introduced. > > Konrad Apologies for extremely late reply Konrad. Let me try to recap quickly. The part you pointed out initializes a new regmap LLCC Boradcast AND region which is available only SM8450 onward. This patch set is updating respective DTs and driver code. Regarding the resource error on booting, I had added this check in previous version of patch set (https://lore.kernel.org/all/1ca4d384-9df4-4c00-a4c9-0c5ff491616e@linaro.org/) @@ -1282,6 +1287,17 @@ static int qcom_llcc_probe(struct platform_device *pdev) drv_data->version = version; + if (drv_data->version >= LLCC_VERSION_4_1_0_0) { + drv_data->bcast_and_regmap = qcom_llcc_init_mmio(pdev, i + 1, "llcc_broadcast_and_base"); + if (IS_ERR(drv_data->bcast_and_regmap)) { + ret = PTR_ERR(drv_data->bcast_and_regmap); + if (ret == -EINVAL) + drv_data->bcast_and_regmap = NULL; + else + goto err; + } + } + This check will make sure we call devm_ioremap_resource() only when LLCC Boradcast AND region is defined in the devicetree and error is not shown in log.
This series adds: 1. Device tree register mapping for Broadcast_AND region in SM8450, SM8550, SM8650. 2. LLCC driver updates to reflect addition of Broadcast_AND regmap. To support CSR programming, a broadcast interface is used to program all channels in a single command. Until SM8450 there was only one broadcast region (Broadcast_OR) used to broadcast write and check for status bit 0. From SM8450 onwards another broadcast region (Broadcast_AND) has been added which checks for status bit 1. This series updates the device trees from SM8450 onwards to have a mapping to this Broadcast_AND region. It also updates the llcc_drv_data structure with a regmap for Broadcast_AND region and corrects the broadcast region used to check for status bit 1. Changes in v4: - Updated Devicetree patches' commit messages to make problem statement clearer - Resolved Konrad's comments on driver code patch - Updated v3 changelog to include dropped R-b tag Changes in v3: - Removed new example in dt-bindings patch and ran 'make DT_CHECKER_FLAGS=-m dt_binding_check' - Dropped Krzysztof's R-b tag on dt-bindings patch - Use of ternary operator in llcc_update_act_ctrl() - Add comment before initialization of Broadcast_AND regmap in probe function - Move DeviceTree patches to the end Changes in v2: - Added an additional check in the case old DT files are used for above mentioned chipsets for backwards compatibility - Moved addition of if check in llcc_update_act_ctrl() to a separate "Fixes" patch; not part of this series Link to v3: https://lore.kernel.org/all/cover.1708551850.git.quic_uchalich@quicinc.com/ Link to v2: https://lore.kernel.org/all/cover.1707202761.git.quic_uchalich@quicinc.com/ Link to v1: https://lore.kernel.org/all/cover.1706296015.git.quic_uchalich@quicinc.com/ Unnathi Chalicheemala (5): dt-bindings: arm: msm: Add llcc Broadcast_AND register soc: qcom: llcc: Add regmap for Broadcast_AND region arm64: dts: qcom: sm8450: Add mapping to llcc Broadcast_AND region arm64: dts: qcom: sm8550: Add mapping to llcc Broadcast_AND region arm64: dts: qcom: sm8650: Add mapping to llcc Broadcast_AND region .../devicetree/bindings/cache/qcom,llcc.yaml | 27 ++++++++++++++++++- arch/arm64/boot/dts/qcom/sm8450.dtsi | 5 ++-- arch/arm64/boot/dts/qcom/sm8550.dtsi | 6 +++-- arch/arm64/boot/dts/qcom/sm8650.dtsi | 6 +++-- drivers/soc/qcom/llcc-qcom.c | 15 ++++++++++- include/linux/soc/qcom/llcc-qcom.h | 4 ++- 6 files changed, 54 insertions(+), 9 deletions(-) -- 2.25.1 --- Unnathi Chalicheemala (5): dt-bindings: arm: msm: Add llcc Broadcast_AND register soc: qcom: llcc: Add regmap for Broadcast_AND region arm64: dts: qcom: sm8450: Add mapping to llcc Broadcast_AND region arm64: dts: qcom: sm8550: Add Broadcast_AND register in LLCC block arm64: dts: qcom: sm8650: Add Broadcast_AND register in LLCC block .../devicetree/bindings/cache/qcom,llcc.yaml | 27 +++++++++++++++++++++- arch/arm64/boot/dts/qcom/sm8450.dtsi | 5 ++-- arch/arm64/boot/dts/qcom/sm8550.dtsi | 6 +++-- arch/arm64/boot/dts/qcom/sm8650.dtsi | 6 +++-- drivers/soc/qcom/llcc-qcom.c | 14 ++++++++++- include/linux/soc/qcom/llcc-qcom.h | 4 +++- 6 files changed, 53 insertions(+), 9 deletions(-) --- base-commit: 4535e1a4174c4111d92c5a9a21e542d232e0fcaa change-id: 20240329-llcc-broadcast-and-eec0838308d6 Best regards,