lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <3c5aeda7-6773-0b41-9c02-5f423117c3b3@linaro.org> Date: Sat, 10 Dec 2022 12:01:59 +0100 From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> To: Eric Biggers <ebiggers@...nel.org> Cc: Luca Weiss <luca.weiss@...rphone.com>, ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org, Andy Gross <agross@...nel.org>, Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, Alim Akhtar <alim.akhtar@...sung.com>, Avri Altman <avri.altman@....com>, Bart Van Assche <bvanassche@....org>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, linux-arm-msm@...r.kernel.org, linux-scsi@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE On 09/12/2022 20:35, Eric Biggers wrote: > On Fri, Dec 09, 2022 at 04:19:20PM +0100, Krzysztof Kozlowski wrote: >> On 09/12/2022 16:11, Luca Weiss wrote: >>> On Fri Dec 9, 2022 at 4:05 PM CET, Krzysztof Kozlowski wrote: >>>> On 09/12/2022 15:29, Luca Weiss wrote: >>>>> The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this >>>>> in the bindings so the existing dts can validate successfully. >>>>> >>>>> Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: >>>>> sm8450: add Inline Crypto Engine registers and clock") so move the >>>>> compatible to the correct if. >>>>> >>>>> Signed-off-by: Luca Weiss <luca.weiss@...rphone.com> >>>>> --- >>>>> (no cover subject) >>>>> >>>>> The only remaining validation issues I see is the following on sc8280xp-crd.dtb >>>>> and sa8540p-ride.dtb: >>>>> >>>>> Unevaluated properties are not allowed ('required-opps', 'dma-coherent' were unexpected) >>>>> >>>>> Maybe someone who knows something about this can handle this? >>>>> >>>>> And the patch adding qcom,sm6115-ufshc hasn't been applied yet. >>>>> --- >>>>> Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 8 +++++++- >>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml >>>>> index f2d6298d926c..58a2fb2c83c3 100644 >>>>> --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml >>>>> +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml >>>>> @@ -102,7 +102,6 @@ allOf: >>>>> - qcom,sc8280xp-ufshc >>>>> - qcom,sm8250-ufshc >>>>> - qcom,sm8350-ufshc >>>>> - - qcom,sm8450-ufshc >>>>> then: >>>>> properties: >>>>> clocks: >>>>> @@ -130,6 +129,7 @@ allOf: >>>>> - qcom,sdm845-ufshc >>>>> - qcom,sm6350-ufshc >>>>> - qcom,sm8150-ufshc >>>>> + - qcom,sm8450-ufshc >>>>> then: >>>>> properties: >>>>> clocks: >>>>> @@ -149,6 +149,12 @@ allOf: >>>>> reg: >>>>> minItems: 2 >>>>> maxItems: 2 >>>>> + reg-names: >>>> >>>> There are no reg-names in top-level, so it's surprising to see its >>>> customized here. It seems no one ever documented that usage... >>> >>> From what I can tell, from driver side all devices not using ICE don't >>> need reg-names, only the "ice" reg is referenced by name in the driver. >>> >>> I didn't add it top-level because with only one reg I think we're not >>> supposed to use reg-names, right? >> >> And you still won't need to use. Yet property should be rather described >> in top-level which also will unify the items here (so no different >> 2-item reg-names in variants). >> >> Just add it to top-level with minItems: 1 and per variant customize: >> 1. maxItems: 1 >> 2. minItems: 2 + required >> >> The "required" is a bit questionable... this was never added by Eric to >> the bindings. Driver support and DTS were added completely skipping >> bindings... >> > > Sorry about that. At the time > (https://lore.kernel.org/linux-scsi/20200722051143.GU388985@builder.lan/T/#t) > I didn't know there was a Documentation file that should have been updated. Any change regarding bindings (so adding/changing compatibles, properties, nodes) a driver is using must be followed by... change in the bindings. > > The UFS core assumes that the reg at index 0 is the UFS standard registers. > It is not referenced by name. > > ufs-qcom already had an optional reg at index 1. I needed to add another > optional reg. So I made the regs at index 1 and later be optional named regs: > dev_ref_clk_ctrl_mem and ice. That seemed better than hardcoding the indices. > > Is it causing a problem that the UFS standard reg at index 0 is being mixed with > named regs in the same list? The indexes should be ordered (hard-coded), not flexible. If there is already second IO address at index 1, then the patch does not look correct. When fixing, please fix it completely. Best regards, Krzysztof
Powered by blists - more mailing lists