| Message ID | 20240815085725.2740390-1-quic_mdalam@quicinc.com |
|---|---|
| Headers | show |
| Series | Add cmd descriptor support | expand |
On 8/15/2024 6:38 PM, Caleb Connolly wrote: > Hi, > > A note for future patches, please scope your cover letter subject: > > "dmaengine: qcom: bam_dma: add cmd descriptor support" Sure will add this in next patch. > > On 15/08/2024 10:57, Md Sadre Alam wrote: >> This series of patches will add command descriptor >> support to read/write crypto engine register via >> BAM/DMA >> >> We need this support because if there is multiple EE's >> (Execution Environment) accessing the same CE then there >> will be race condition. To avoid this race condition >> BAM HW hsving LOC/UNLOCK feature on BAM pipes and this >> LOCK/UNLOCK will be set via command descriptor only. >> >> Since each EE's having their dedicated BAM pipe, BAM allows >> Locking and Unlocking on BAM pipe. So if one EE's requesting >> for CE5 access then that EE's first has to LOCK the BAM pipe >> while setting LOCK bit on command descriptor and then access >> it. After finishing the request EE's has to UNLOCK the BAM pipe >> so in this way we race condition will not happen. >> >> tested with "tcrypt.ko" and "kcapi" tool. >> >> Need help to test these all the patches on msm platform > > DT changes here are only for a few IPQ platforms, please explain in the cover letter if this is some IPQ specific feature which doesn't exist on other platforms, or if you're only enabling it on IPQ. This feature is BAM hardware feature so its applicable for all the QCOM Soc where bam is there. Its not IPQ specific. Will add all the explanation in cover letter in next patch > > Some broad strokes testing instructions (at the very least) and requirements (testing on what hardware?) aren't made obvious at all here. Sure will update in cover letter in next patch. > > Kind regards, >> >> v2: >> * Addressed all the comments from v1 >> * Added the dt-binding >> * Added locking/unlocking mechanism in bam driver >> >> v1: >> * https://lore.kernel.org/lkml/20231214114239.2635325-1-quic_mdalam@quicinc.com/ >> * Initial set of patches for cmd descriptor support >> >> Md Sadre Alam (16): >> dt-bindings: dma: qcom,bam: Add bam pipe lock >> dmaengine: qcom: bam_dma: add bam_pipe_lock dt property >> dmaengine: qcom: bam_dma: add LOCK & UNLOCK flag support >> crypto: qce - Add support for crypto address read >> crypto: qce - Add bam dma support for crypto register r/w >> crypto: qce - Convert register r/w for skcipher via BAM/DMA >> crypto: qce - Convert register r/w for sha via BAM/DMA >> crypto: qce - Convert register r/w for aead via BAM/DMA >> crypto: qce - Add LOCK and UNLOCK flag support >> crypto: qce - Add support for lock aquire,lock release api. >> crypto: qce - Add support for lock/unlock in skcipher >> crypto: qce - Add support for lock/unlock in sha >> crypto: qce - Add support for lock/unlock in aead >> arm64: dts: qcom: ipq9574: enable bam pipe locking/unlocking >> arm64: dts: qcom: ipq8074: enable bam pipe locking/unlocking >> arm64: dts: qcom: ipq6018: enable bam pipe locking/unlocking >> >> .../devicetree/bindings/dma/qcom,bam-dma.yaml | 8 + >> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 1 + >> arch/arm64/boot/dts/qcom/ipq8074.dtsi | 1 + >> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 1 + >> drivers/crypto/qce/aead.c | 4 + >> drivers/crypto/qce/common.c | 142 +++++++---- >> drivers/crypto/qce/core.c | 13 +- >> drivers/crypto/qce/core.h | 12 + >> drivers/crypto/qce/dma.c | 232 ++++++++++++++++++ >> drivers/crypto/qce/dma.h | 26 +- >> drivers/crypto/qce/sha.c | 4 + >> drivers/crypto/qce/skcipher.c | 4 + >> drivers/dma/qcom/bam_dma.c | 14 +- >> include/linux/dmaengine.h | 6 + >> 14 files changed, 424 insertions(+), 44 deletions(-) >> >
On Fri, Aug 16, 2024 at 05:33:43PM GMT, Md Sadre Alam wrote: > On 8/15/2024 6:38 PM, Caleb Connolly wrote: [..] > > On 15/08/2024 10:57, Md Sadre Alam wrote: [..] > > > > > > tested with "tcrypt.ko" and "kcapi" tool. > > > > > > Need help to test these all the patches on msm platform > > > > DT changes here are only for a few IPQ platforms, please explain in the cover letter if this is some IPQ specific feature which doesn't exist on other platforms, or if you're only enabling it on IPQ. > > This feature is BAM hardware feature so its applicable for all the QCOM Soc where bam is there. Its not IPQ specific. Will add all the explanation in cover letter in next patch Please configure your email client such that your replies follow the typical style of mail list discussions. I believe go/upstream has instructions for this. > > > > Some broad strokes testing instructions (at the very least) and requirements (testing on what hardware?) aren't made obvious at all here. > > Sure will update in cover letter in next patch. I'm interested in these instructions as well, but no need to wait for another version to provide these instructions. Please just reply here (and then include them if there are future versions) Regards, Bjorn
On Thu, Aug 15, 2024 at 02:27:10PM GMT, Md Sadre Alam wrote: > BAM having pipe locking mechanism. The Lock and Un-Lock bit > should be set on CMD descriptor only. Upon encountering a > descriptor with Lock bit set, the BAM will lock all other > pipes not related to the current pipe group, and keep > handling the current pipe only until it sees the Un-Lock > set. This describes the mechanism for mutual exclusion, but not really what the patch does. https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format states that you have 75 characters for your commit message, use them. > > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> > --- > > Change in [v2] > > * Added initial support for dt-binding > > Change in [v1] > > * This patch was not included in [v1] > > Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > index 3ad0d9b1fbc5..91cc2942aa62 100644 > --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > @@ -77,6 +77,12 @@ properties: > Indicates that the bam is powered up by a remote processor but must be > initialized by the local processor. > > + qcom,bam_pipe_lock: '_' is not a valid character in node names or properties. > + type: boolean > + description: > + Indicates that the bam pipe needs locking or not based on client driver > + sending the LOCK or UNLOK bit set on command descriptor. Missing 'C'? Regards, Bjorn > + > reg: > maxItems: 1 > > @@ -92,6 +98,8 @@ anyOf: > - qcom,powered-remotely > - required: > - qcom,controlled-remotely > + - required: > + - qcom,bam_pipe_lock > - required: > - clocks > - clock-names > -- > 2.34.1 >
On Thu, Aug 15, 2024 at 02:27:09PM GMT, Md Sadre Alam wrote: > This series of patches will add command descriptor > support to read/write crypto engine register via > BAM/DMA > > We need this support because if there is multiple EE's > (Execution Environment) accessing the same CE then there > will be race condition. To avoid this race condition > BAM HW hsving LOC/UNLOCK feature on BAM pipes and this > LOCK/UNLOCK will be set via command descriptor only. > > Since each EE's having their dedicated BAM pipe, BAM allows > Locking and Unlocking on BAM pipe. So if one EE's requesting > for CE5 access then that EE's first has to LOCK the BAM pipe > while setting LOCK bit on command descriptor and then access > it. After finishing the request EE's has to UNLOCK the BAM pipe > so in this way we race condition will not happen. > > tested with "tcrypt.ko" and "kcapi" tool. > > Need help to test these all the patches on msm platform > > v2: > * Addressed all the comments from v1 Please describe the actual changes you're making between your versions. > * Added the dt-binding > * Added locking/unlocking mechanism in bam driver Seems to me that this was already part of v1, as patch 6/11? Regards, Bjorn > > v1: > * https://lore.kernel.org/lkml/20231214114239.2635325-1-quic_mdalam@quicinc.com/ > * Initial set of patches for cmd descriptor support > > Md Sadre Alam (16): > dt-bindings: dma: qcom,bam: Add bam pipe lock > dmaengine: qcom: bam_dma: add bam_pipe_lock dt property > dmaengine: qcom: bam_dma: add LOCK & UNLOCK flag support > crypto: qce - Add support for crypto address read > crypto: qce - Add bam dma support for crypto register r/w > crypto: qce - Convert register r/w for skcipher via BAM/DMA > crypto: qce - Convert register r/w for sha via BAM/DMA > crypto: qce - Convert register r/w for aead via BAM/DMA > crypto: qce - Add LOCK and UNLOCK flag support > crypto: qce - Add support for lock aquire,lock release api. > crypto: qce - Add support for lock/unlock in skcipher > crypto: qce - Add support for lock/unlock in sha > crypto: qce - Add support for lock/unlock in aead > arm64: dts: qcom: ipq9574: enable bam pipe locking/unlocking > arm64: dts: qcom: ipq8074: enable bam pipe locking/unlocking > arm64: dts: qcom: ipq6018: enable bam pipe locking/unlocking > > .../devicetree/bindings/dma/qcom,bam-dma.yaml | 8 + > arch/arm64/boot/dts/qcom/ipq6018.dtsi | 1 + > arch/arm64/boot/dts/qcom/ipq8074.dtsi | 1 + > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 1 + > drivers/crypto/qce/aead.c | 4 + > drivers/crypto/qce/common.c | 142 +++++++---- > drivers/crypto/qce/core.c | 13 +- > drivers/crypto/qce/core.h | 12 + > drivers/crypto/qce/dma.c | 232 ++++++++++++++++++ > drivers/crypto/qce/dma.h | 26 +- > drivers/crypto/qce/sha.c | 4 + > drivers/crypto/qce/skcipher.c | 4 + > drivers/dma/qcom/bam_dma.c | 14 +- > include/linux/dmaengine.h | 6 + > 14 files changed, 424 insertions(+), 44 deletions(-) > > -- > 2.34.1 >
On Thu, Aug 15, 2024 at 02:27:23PM GMT, Md Sadre Alam wrote: > enable bam pipe locking/unlocking for ipq9507 SoC. Note that the commit messages for the other non-dts commits will not show up in the git history for this file. So, please follow https://docs.kernel.org/process/submitting-patches.html#describe-your-changes and give some indication of "problem description", to give future readers an idea why this is here. > > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> > --- > > Change in [v2] > > * enabled locking/unlocking support for ipq9574 > > Change in [v1] > > * This patch was not included in [v1] > > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > index 48dfafea46a7..dacaec62ec39 100644 > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > @@ -262,6 +262,7 @@ cryptobam: dma-controller@704000 { > #dma-cells = <1>; > qcom,ee = <1>; > qcom,controlled-remotely; > + qcom,bam_pipe_lock; Per the question before about what does this actually lock. Is this a property of the BAM controller, or the crypto channel? Regards, Bjorn > }; > > crypto: crypto@73a000 { > -- > 2.34.1 >
On 15/08/2024 10:57, Md Sadre Alam wrote: > BAM having pipe locking mechanism. The Lock and Un-Lock bit > should be set on CMD descriptor only. Upon encountering a > descriptor with Lock bit set, the BAM will lock all other > pipes not related to the current pipe group, and keep > handling the current pipe only until it sees the Un-Lock > set. Please wrap commit message according to Linux coding style / submission process (neither too early nor over the limit): https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 > > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> > --- > > Change in [v2] > > * Added initial support for dt-binding > > Change in [v1] > > * This patch was not included in [v1] > > Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > index 3ad0d9b1fbc5..91cc2942aa62 100644 > --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml > @@ -77,6 +77,12 @@ properties: > Indicates that the bam is powered up by a remote processor but must be > initialized by the local processor. > > + qcom,bam_pipe_lock: Please follow DTS coding style. > + type: boolean > + description: > + Indicates that the bam pipe needs locking or not based on client driver > + sending the LOCK or UNLOK bit set on command descriptor. You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc. > + > reg: > maxItems: 1 > > @@ -92,6 +98,8 @@ anyOf: > - qcom,powered-remotely > - required: > - qcom,controlled-remotely > + - required: > + - qcom,bam_pipe_lock Why is it here? What do you want to achieve? > - required: > - clocks > - clock-names Best regards, Krzysztof
On 15/08/2024 10:57, Md Sadre Alam wrote: > Get crypto base address from DT. This will use for > command descriptor support for crypto register r/w > via BAM/DMA All your commit messages are oddly wrapped. This does not make reading it easy... > > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> > --- > Change in [v2] > > * Addressed all comments from v1 > > Change in [v1] > > * Added support to read crypto base address from dt > > drivers/crypto/qce/core.c | 13 ++++++++++++- > drivers/crypto/qce/core.h | 1 + > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c > index 28b5fd823827..9b23a948078a 100644 > --- a/drivers/crypto/qce/core.c > +++ b/drivers/crypto/qce/core.c > @@ -192,6 +192,7 @@ static int qce_crypto_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct qce_device *qce; > + struct resource *res; > int ret; > > qce = devm_kzalloc(dev, sizeof(*qce), GFP_KERNEL); > @@ -201,10 +202,16 @@ static int qce_crypto_probe(struct platform_device *pdev) > qce->dev = dev; > platform_set_drvdata(pdev, qce); > > - qce->base = devm_platform_ioremap_resource(pdev, 0); > + qce->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > if (IS_ERR(qce->base)) > return PTR_ERR(qce->base); > > + qce->base_dma = dma_map_resource(dev, res->start, > + resource_size(res), > + DMA_BIDIRECTIONAL, 0); > + if (dma_mapping_error(dev, qce->base_dma)) > + return -ENXIO; > + > ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > if (ret < 0) > return ret; And how do you handle error paths? > @@ -280,6 +287,7 @@ static int qce_crypto_probe(struct platform_device *pdev) > static void qce_crypto_remove(struct platform_device *pdev) > { > struct qce_device *qce = platform_get_drvdata(pdev); > + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > tasklet_kill(&qce->done_tasklet); > qce_unregister_algs(qce); > @@ -287,6 +295,9 @@ static void qce_crypto_remove(struct platform_device *pdev) > clk_disable_unprepare(qce->bus); > clk_disable_unprepare(qce->iface); > clk_disable_unprepare(qce->core); > + > + dma_unmap_resource(&pdev->dev, qce->base_dma, resource_size(res), > + DMA_BIDIRECTIONAL, 0); If you add code to the remove callback, not adding it to error paths is suspicious by itself... Best regards, Krzysztof
On 8/16/2024 9:23 PM, Bjorn Andersson wrote: > On Thu, Aug 15, 2024 at 02:27:10PM GMT, Md Sadre Alam wrote: >> BAM having pipe locking mechanism. The Lock and Un-Lock bit >> should be set on CMD descriptor only. Upon encountering a >> descriptor with Lock bit set, the BAM will lock all other >> pipes not related to the current pipe group, and keep >> handling the current pipe only until it sees the Un-Lock >> set. > > This describes the mechanism for mutual exclusion, but not really what > the patch does. Ok, will update in next patch. > > https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format > states that you have 75 characters for your commit message, use them. ok, will update in next patch. > >> >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >> --- >> >> Change in [v2] >> >> * Added initial support for dt-binding >> >> Change in [v1] >> >> * This patch was not included in [v1] >> >> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >> index 3ad0d9b1fbc5..91cc2942aa62 100644 >> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >> @@ -77,6 +77,12 @@ properties: >> Indicates that the bam is powered up by a remote processor but must be >> initialized by the local processor. >> >> + qcom,bam_pipe_lock: > > '_' is not a valid character in node names or properties. Ok, will update in next patch. > >> + type: boolean >> + description: >> + Indicates that the bam pipe needs locking or not based on client driver >> + sending the LOCK or UNLOK bit set on command descriptor. > > Missing 'C'? Ok, will fix in next patch. > > Regards, > Bjorn > >> + >> reg: >> maxItems: 1 >> >> @@ -92,6 +98,8 @@ anyOf: >> - qcom,powered-remotely >> - required: >> - qcom,controlled-remotely >> + - required: >> + - qcom,bam_pipe_lock >> - required: >> - clocks >> - clock-names >> -- >> 2.34.1 >>
On 8/16/2024 9:31 PM, Bjorn Andersson wrote: > On Thu, Aug 15, 2024 at 02:27:09PM GMT, Md Sadre Alam wrote: >> This series of patches will add command descriptor >> support to read/write crypto engine register via >> BAM/DMA >> >> We need this support because if there is multiple EE's >> (Execution Environment) accessing the same CE then there >> will be race condition. To avoid this race condition >> BAM HW hsving LOC/UNLOCK feature on BAM pipes and this >> LOCK/UNLOCK will be set via command descriptor only. >> >> Since each EE's having their dedicated BAM pipe, BAM allows >> Locking and Unlocking on BAM pipe. So if one EE's requesting >> for CE5 access then that EE's first has to LOCK the BAM pipe >> while setting LOCK bit on command descriptor and then access >> it. After finishing the request EE's has to UNLOCK the BAM pipe >> so in this way we race condition will not happen. >> >> tested with "tcrypt.ko" and "kcapi" tool. >> >> Need help to test these all the patches on msm platform >> >> v2: >> * Addressed all the comments from v1 > > Please describe the actual changes you're making between your versions. Sure , will update in next patch. > >> * Added the dt-binding >> * Added locking/unlocking mechanism in bam driver > > Seems to me that this was already part of v1, as patch 6/11? Sorry, by mistake I have added this line in v2. > > Regards, > Bjorn > >> >> v1: >> * https://lore.kernel.org/lkml/20231214114239.2635325-1-quic_mdalam@quicinc.com/ >> * Initial set of patches for cmd descriptor support >> >> Md Sadre Alam (16): >> dt-bindings: dma: qcom,bam: Add bam pipe lock >> dmaengine: qcom: bam_dma: add bam_pipe_lock dt property >> dmaengine: qcom: bam_dma: add LOCK & UNLOCK flag support >> crypto: qce - Add support for crypto address read >> crypto: qce - Add bam dma support for crypto register r/w >> crypto: qce - Convert register r/w for skcipher via BAM/DMA >> crypto: qce - Convert register r/w for sha via BAM/DMA >> crypto: qce - Convert register r/w for aead via BAM/DMA >> crypto: qce - Add LOCK and UNLOCK flag support >> crypto: qce - Add support for lock aquire,lock release api. >> crypto: qce - Add support for lock/unlock in skcipher >> crypto: qce - Add support for lock/unlock in sha >> crypto: qce - Add support for lock/unlock in aead >> arm64: dts: qcom: ipq9574: enable bam pipe locking/unlocking >> arm64: dts: qcom: ipq8074: enable bam pipe locking/unlocking >> arm64: dts: qcom: ipq6018: enable bam pipe locking/unlocking >> >> .../devicetree/bindings/dma/qcom,bam-dma.yaml | 8 + >> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 1 + >> arch/arm64/boot/dts/qcom/ipq8074.dtsi | 1 + >> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 1 + >> drivers/crypto/qce/aead.c | 4 + >> drivers/crypto/qce/common.c | 142 +++++++---- >> drivers/crypto/qce/core.c | 13 +- >> drivers/crypto/qce/core.h | 12 + >> drivers/crypto/qce/dma.c | 232 ++++++++++++++++++ >> drivers/crypto/qce/dma.h | 26 +- >> drivers/crypto/qce/sha.c | 4 + >> drivers/crypto/qce/skcipher.c | 4 + >> drivers/dma/qcom/bam_dma.c | 14 +- >> include/linux/dmaengine.h | 6 + >> 14 files changed, 424 insertions(+), 44 deletions(-) >> >> -- >> 2.34.1 >>
On 8/16/2024 10:10 PM, Bjorn Andersson wrote: > On Thu, Aug 15, 2024 at 02:27:23PM GMT, Md Sadre Alam wrote: >> enable bam pipe locking/unlocking for ipq9507 SoC. > > Note that the commit messages for the other non-dts commits will not > show up in the git history for this file. So, please follow > https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > and give some indication of "problem description", to give future > readers an idea why this is here. Ok > >> >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >> --- >> >> Change in [v2] >> >> * enabled locking/unlocking support for ipq9574 >> >> Change in [v1] >> >> * This patch was not included in [v1] >> >> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> index 48dfafea46a7..dacaec62ec39 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> @@ -262,6 +262,7 @@ cryptobam: dma-controller@704000 { >> #dma-cells = <1>; >> qcom,ee = <1>; >> qcom,controlled-remotely; >> + qcom,bam_pipe_lock; > > Per the question before about what does this actually lock. Is this a > property of the BAM controller, or the crypto channel? This is the property of BAM controller. > > Regards, > Bjorn > >> }; >> >> crypto: crypto@73a000 { >> -- >> 2.34.1 >>
On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote: > On 15/08/2024 10:57, Md Sadre Alam wrote: >> BAM having pipe locking mechanism. The Lock and Un-Lock bit >> should be set on CMD descriptor only. Upon encountering a >> descriptor with Lock bit set, the BAM will lock all other >> pipes not related to the current pipe group, and keep >> handling the current pipe only until it sees the Un-Lock >> set. > > Please wrap commit message according to Linux coding style / submission > process (neither too early nor over the limit): > https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 Ok , will update in next patch. > >> >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >> --- >> >> Change in [v2] >> >> * Added initial support for dt-binding >> >> Change in [v1] >> >> * This patch was not included in [v1] >> >> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >> index 3ad0d9b1fbc5..91cc2942aa62 100644 >> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >> @@ -77,6 +77,12 @@ properties: >> Indicates that the bam is powered up by a remote processor but must be >> initialized by the local processor. >> >> + qcom,bam_pipe_lock: > > Please follow DTS coding style. Ok > >> + type: boolean >> + description: >> + Indicates that the bam pipe needs locking or not based on client driver >> + sending the LOCK or UNLOK bit set on command descriptor. > > You described the desired Linux feature or behavior, not the actual > hardware. The bindings are about the latter, so instead you need to > rephrase the property and its description to match actual hardware > capabilities/features/configuration etc. Ok, will update in next patch. > >> + >> reg: >> maxItems: 1 >> >> @@ -92,6 +98,8 @@ anyOf: >> - qcom,powered-remotely >> - required: >> - qcom,controlled-remotely >> + - required: >> + - qcom,bam_pipe_lock > > Why is it here? What do you want to achieve? This property added to achieve locking/unlocking of BAM pipe groups for mutual exclusion of resources that can be used across multiple EE's > >> - required: >> - clocks >> - clock-names > > Best regards, > Krzysztof >
On 8/17/2024 2:40 PM, Krzysztof Kozlowski wrote: > On 15/08/2024 10:57, Md Sadre Alam wrote: >> Get crypto base address from DT. This will use for >> command descriptor support for crypto register r/w >> via BAM/DMA > > All your commit messages are oddly wrapped. This does not make reading > it easy... > >> >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >> --- >> Change in [v2] >> >> * Addressed all comments from v1 >> >> Change in [v1] >> >> * Added support to read crypto base address from dt >> >> drivers/crypto/qce/core.c | 13 ++++++++++++- >> drivers/crypto/qce/core.h | 1 + >> 2 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c >> index 28b5fd823827..9b23a948078a 100644 >> --- a/drivers/crypto/qce/core.c >> +++ b/drivers/crypto/qce/core.c >> @@ -192,6 +192,7 @@ static int qce_crypto_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct qce_device *qce; >> + struct resource *res; >> int ret; >> >> qce = devm_kzalloc(dev, sizeof(*qce), GFP_KERNEL); >> @@ -201,10 +202,16 @@ static int qce_crypto_probe(struct platform_device *pdev) >> qce->dev = dev; >> platform_set_drvdata(pdev, qce); >> >> - qce->base = devm_platform_ioremap_resource(pdev, 0); >> + qce->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); >> if (IS_ERR(qce->base)) >> return PTR_ERR(qce->base); >> >> + qce->base_dma = dma_map_resource(dev, res->start, >> + resource_size(res), >> + DMA_BIDIRECTIONAL, 0); >> + if (dma_mapping_error(dev, qce->base_dma)) >> + return -ENXIO; >> + >> ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); >> if (ret < 0) >> return ret; > > And how do you handle error paths? Ok , will fix the error path to cleanup the resources. > > >> @@ -280,6 +287,7 @@ static int qce_crypto_probe(struct platform_device *pdev) >> static void qce_crypto_remove(struct platform_device *pdev) >> { >> struct qce_device *qce = platform_get_drvdata(pdev); >> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> >> tasklet_kill(&qce->done_tasklet); >> qce_unregister_algs(qce); >> @@ -287,6 +295,9 @@ static void qce_crypto_remove(struct platform_device *pdev) >> clk_disable_unprepare(qce->bus); >> clk_disable_unprepare(qce->iface); >> clk_disable_unprepare(qce->core); >> + >> + dma_unmap_resource(&pdev->dev, qce->base_dma, resource_size(res), >> + DMA_BIDIRECTIONAL, 0); > > If you add code to the remove callback, not adding it to error paths is > suspicious by itself... Ok , will fix the error path to cleanup the resources. > > Best regards, > Krzysztof > >
On 21/08/2024 18:34, Md Sadre Alam wrote: > > > On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote: >> On 15/08/2024 10:57, Md Sadre Alam wrote: >>> BAM having pipe locking mechanism. The Lock and Un-Lock bit >>> should be set on CMD descriptor only. Upon encountering a >>> descriptor with Lock bit set, the BAM will lock all other >>> pipes not related to the current pipe group, and keep >>> handling the current pipe only until it sees the Un-Lock >>> set. >> >> Please wrap commit message according to Linux coding style / submission >> process (neither too early nor over the limit): >> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 > Ok , will update in next patch. >> >>> >>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >>> --- >>> >>> Change in [v2] >>> >>> * Added initial support for dt-binding >>> >>> Change in [v1] >>> >>> * This patch was not included in [v1] >>> >>> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>> index 3ad0d9b1fbc5..91cc2942aa62 100644 >>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>> @@ -77,6 +77,12 @@ properties: >>> Indicates that the bam is powered up by a remote processor but must be >>> initialized by the local processor. >>> >>> + qcom,bam_pipe_lock: >> >> Please follow DTS coding style. > Ok >> >>> + type: boolean >>> + description: >>> + Indicates that the bam pipe needs locking or not based on client driver >>> + sending the LOCK or UNLOK bit set on command descriptor. >> >> You described the desired Linux feature or behavior, not the actual >> hardware. The bindings are about the latter, so instead you need to >> rephrase the property and its description to match actual hardware >> capabilities/features/configuration etc. > Ok, will update in next patch. >> >>> + >>> reg: >>> maxItems: 1 >>> >>> @@ -92,6 +98,8 @@ anyOf: >>> - qcom,powered-remotely >>> - required: >>> - qcom,controlled-remotely >>> + - required: >>> + - qcom,bam_pipe_lock >> >> Why is it here? What do you want to achieve? > This property added to achieve locking/unlocking > of BAM pipe groups for mutual exclusion of resources > that can be used across multiple EE's This explains me nothing. I am questioning the anyOf block. Why this is the fourth method of controlling BAM? Anyway, if it is, then explain this in commit msg. Best regards, Krzysztof
On 8/22/2024 11:57 AM, Krzysztof Kozlowski wrote: > On 21/08/2024 18:34, Md Sadre Alam wrote: >> >> >> On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote: >>> On 15/08/2024 10:57, Md Sadre Alam wrote: >>>> BAM having pipe locking mechanism. The Lock and Un-Lock bit >>>> should be set on CMD descriptor only. Upon encountering a >>>> descriptor with Lock bit set, the BAM will lock all other >>>> pipes not related to the current pipe group, and keep >>>> handling the current pipe only until it sees the Un-Lock >>>> set. >>> >>> Please wrap commit message according to Linux coding style / submission >>> process (neither too early nor over the limit): >>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 >> Ok , will update in next patch. >>> >>>> >>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >>>> --- >>>> >>>> Change in [v2] >>>> >>>> * Added initial support for dt-binding >>>> >>>> Change in [v1] >>>> >>>> * This patch was not included in [v1] >>>> >>>> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>> index 3ad0d9b1fbc5..91cc2942aa62 100644 >>>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>> @@ -77,6 +77,12 @@ properties: >>>> Indicates that the bam is powered up by a remote processor but must be >>>> initialized by the local processor. >>>> >>>> + qcom,bam_pipe_lock: >>> >>> Please follow DTS coding style. >> Ok >>> >>>> + type: boolean >>>> + description: >>>> + Indicates that the bam pipe needs locking or not based on client driver >>>> + sending the LOCK or UNLOK bit set on command descriptor. >>> >>> You described the desired Linux feature or behavior, not the actual >>> hardware. The bindings are about the latter, so instead you need to >>> rephrase the property and its description to match actual hardware >>> capabilities/features/configuration etc. >> Ok, will update in next patch. >>> >>>> + >>>> reg: >>>> maxItems: 1 >>>> >>>> @@ -92,6 +98,8 @@ anyOf: >>>> - qcom,powered-remotely >>>> - required: >>>> - qcom,controlled-remotely >>>> + - required: >>>> + - qcom,bam_pipe_lock >>> >>> Why is it here? What do you want to achieve? >> This property added to achieve locking/unlocking >> of BAM pipe groups for mutual exclusion of resources >> that can be used across multiple EE's > > This explains me nothing. I am questioning the anyOf block. Why this is > the fourth method of controlling BAM? Anyway, if it is, then explain > this in commit msg. This is the BAM property for locking/unlocking the BAM pipes.That's why I kept in anyOf block. Will explain in commit message in next patch. > > Best regards, > Krzysztof >
On 22/08/2024 13:45, Md Sadre Alam wrote: > > > On 8/22/2024 11:57 AM, Krzysztof Kozlowski wrote: >> On 21/08/2024 18:34, Md Sadre Alam wrote: >>> >>> >>> On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote: >>>> On 15/08/2024 10:57, Md Sadre Alam wrote: >>>>> BAM having pipe locking mechanism. The Lock and Un-Lock bit >>>>> should be set on CMD descriptor only. Upon encountering a >>>>> descriptor with Lock bit set, the BAM will lock all other >>>>> pipes not related to the current pipe group, and keep >>>>> handling the current pipe only until it sees the Un-Lock >>>>> set. >>>> >>>> Please wrap commit message according to Linux coding style / submission >>>> process (neither too early nor over the limit): >>>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 >>> Ok , will update in next patch. >>>> >>>>> >>>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >>>>> --- >>>>> >>>>> Change in [v2] >>>>> >>>>> * Added initial support for dt-binding >>>>> >>>>> Change in [v1] >>>>> >>>>> * This patch was not included in [v1] >>>>> >>>>> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++ >>>>> 1 file changed, 8 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>>> index 3ad0d9b1fbc5..91cc2942aa62 100644 >>>>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>>> @@ -77,6 +77,12 @@ properties: >>>>> Indicates that the bam is powered up by a remote processor but must be >>>>> initialized by the local processor. >>>>> >>>>> + qcom,bam_pipe_lock: >>>> >>>> Please follow DTS coding style. >>> Ok >>>> >>>>> + type: boolean >>>>> + description: >>>>> + Indicates that the bam pipe needs locking or not based on client driver >>>>> + sending the LOCK or UNLOK bit set on command descriptor. >>>> >>>> You described the desired Linux feature or behavior, not the actual >>>> hardware. The bindings are about the latter, so instead you need to >>>> rephrase the property and its description to match actual hardware >>>> capabilities/features/configuration etc. >>> Ok, will update in next patch. >>>> >>>>> + >>>>> reg: >>>>> maxItems: 1 >>>>> >>>>> @@ -92,6 +98,8 @@ anyOf: >>>>> - qcom,powered-remotely >>>>> - required: >>>>> - qcom,controlled-remotely >>>>> + - required: >>>>> + - qcom,bam_pipe_lock >>>> >>>> Why is it here? What do you want to achieve? >>> This property added to achieve locking/unlocking >>> of BAM pipe groups for mutual exclusion of resources >>> that can be used across multiple EE's >> >> This explains me nothing. I am questioning the anyOf block. Why this is >> the fourth method of controlling BAM? Anyway, if it is, then explain >> this in commit msg. > This is the BAM property for locking/unlocking the BAM pipes.That's > why I kept in anyOf block. You keep repeating the same. It's like poking me with the same comment till I agree. I am done with this. NAK. Provide proper rationale. Best regards, Krzysztof
On 8/23/2024 2:37 PM, Krzysztof Kozlowski wrote: > On 22/08/2024 13:45, Md Sadre Alam wrote: >> >> >> On 8/22/2024 11:57 AM, Krzysztof Kozlowski wrote: >>> On 21/08/2024 18:34, Md Sadre Alam wrote: >>>> >>>> >>>> On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote: >>>>> On 15/08/2024 10:57, Md Sadre Alam wrote: >>>>>> BAM having pipe locking mechanism. The Lock and Un-Lock bit >>>>>> should be set on CMD descriptor only. Upon encountering a >>>>>> descriptor with Lock bit set, the BAM will lock all other >>>>>> pipes not related to the current pipe group, and keep >>>>>> handling the current pipe only until it sees the Un-Lock >>>>>> set. >>>>> >>>>> Please wrap commit message according to Linux coding style / submission >>>>> process (neither too early nor over the limit): >>>>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 >>>> Ok , will update in next patch. >>>>> >>>>>> >>>>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >>>>>> --- >>>>>> >>>>>> Change in [v2] >>>>>> >>>>>> * Added initial support for dt-binding >>>>>> >>>>>> Change in [v1] >>>>>> >>>>>> * This patch was not included in [v1] >>>>>> >>>>>> Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++ >>>>>> 1 file changed, 8 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>>>> index 3ad0d9b1fbc5..91cc2942aa62 100644 >>>>>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>>>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml >>>>>> @@ -77,6 +77,12 @@ properties: >>>>>> Indicates that the bam is powered up by a remote processor but must be >>>>>> initialized by the local processor. >>>>>> >>>>>> + qcom,bam_pipe_lock: >>>>> >>>>> Please follow DTS coding style. >>>> Ok >>>>> >>>>>> + type: boolean >>>>>> + description: >>>>>> + Indicates that the bam pipe needs locking or not based on client driver >>>>>> + sending the LOCK or UNLOK bit set on command descriptor. >>>>> >>>>> You described the desired Linux feature or behavior, not the actual >>>>> hardware. The bindings are about the latter, so instead you need to >>>>> rephrase the property and its description to match actual hardware >>>>> capabilities/features/configuration etc. >>>> Ok, will update in next patch. >>>>> >>>>>> + >>>>>> reg: >>>>>> maxItems: 1 >>>>>> >>>>>> @@ -92,6 +98,8 @@ anyOf: >>>>>> - qcom,powered-remotely >>>>>> - required: >>>>>> - qcom,controlled-remotely >>>>>> + - required: >>>>>> + - qcom,bam_pipe_lock >>>>> >>>>> Why is it here? What do you want to achieve? >>>> This property added to achieve locking/unlocking >>>> of BAM pipe groups for mutual exclusion of resources >>>> that can be used across multiple EE's >>> >>> This explains me nothing. I am questioning the anyOf block. Why this is >>> the fourth method of controlling BAM? Anyway, if it is, then explain >>> this in commit msg. >> This is the BAM property for locking/unlocking the BAM pipes.That's >> why I kept in anyOf block. > > You keep repeating the same. It's like poking me with the same comment > till I agree. I am done with this. > > NAK. Provide proper rationale. Sorry, I misunderstood your review comment. Now as Mani suggested will keep this implementation in driver itself. Will drop the binding patch in next revision. > > Best regards, > Krzysztof > >
On 8/16/2024 9:15 PM, Bjorn Andersson wrote: > On Fri, Aug 16, 2024 at 05:33:43PM GMT, Md Sadre Alam wrote: >> On 8/15/2024 6:38 PM, Caleb Connolly wrote: > [..] >>> On 15/08/2024 10:57, Md Sadre Alam wrote: > [..] >>>> >>>> tested with "tcrypt.ko" and "kcapi" tool. >>>> >>>> Need help to test these all the patches on msm platform >>> >>> DT changes here are only for a few IPQ platforms, please explain in the cover letter if this is some IPQ specific feature which doesn't exist on other platforms, or if you're only enabling it on IPQ. >> >> This feature is BAM hardware feature so its applicable for all the QCOM Soc where bam is there. Its not IPQ specific. Will add all the explanation in cover letter in next patch > > Please configure your email client such that your replies follow the > typical style of mail list discussions. I believe go/upstream has > instructions for this. Ok > >>> >>> Some broad strokes testing instructions (at the very least) and requirements (testing on what hardware?) aren't made obvious at all here. >> >> Sure will update in cover letter in next patch. > > I'm interested in these instructions as well, but no need to wait for > another version to provide these instructions. Please just reply here > (and then include them if there are future versions) Requirements: In QCE crypto driver we are accessing the crypto engine registers directly via CPU read/write so its possible if other subsystem possibly Trust Zone also tries to perform some crypto operations simultaneously, a race condition will be created and this could result in undefined behavior. To avoid this behavior we need to use BAM HW LOCK/UNLOCK feature on BAM pipes, and this LOCK/UNLOCK will be set via sending a command descriptor, where the HLOS/TZ QCE crypto driver prepares a command descriptor with a dummy write operation on one of the QCE crypto engine register and pass the LOCK/UNLOCK flag along with it. This feature tested with tcrypt.ko and "libkcapi" with all the AES algorithm supported by QCE crypto engine. Tested on IPQ9574 and qcm6490.LE chipset. insmod tcrypt.ko mode=101 insmod tcrypt.ko mode=102 insmod tcrypt.ko mode=155 insmod tcrypt.ko mode=180 insmod tcrypt.ko mode=181 insmod tcrypt.ko mode=182 insmod tcrypt.ko mode=185 insmod tcrypt.ko mode=186 insmod tcrypt.ko mode=212 insmod tcrypt.ko mode=216 insmod tcrypt.ko mode=403 insmod tcrypt.ko mode=404 insmod tcrypt.ko mode=500 insmod tcrypt.ko mode=501 insmod tcrypt.ko mode=502 insmod tcrypt.ko mode=600 insmod tcrypt.ko mode=601 insmod tcrypt.ko mode=602 Encryption command line: ./kcapi -x 1 -e -c "cbc(aes)" -k 8d7dd9b0170ce0b5f2f8e1aa768e01e91da8bfc67fd486d081b28254c99eb423 -i 7fbc02ebf5b93322329df9bfccb635af -p 48981da18e4bb9ef7e2e3162d16b1910 * 8b19050f66582cb7f7e4b6c873819b71 * Decryption command line: * $ ./kcapi -x 1 -c "cbc(aes)" -k 3023b2418ea59a841757dcf07881b3a8def1c97b659a4dad -i 95aa5b68130be6fcf5cabe7d9f898a41 -q c313c6b50145b69a77b33404cb422598 * 836de0065f9d6f6a3dd2c53cd17e33a * $ ./kcapi -x 3 -c sha256 -p 38f86d * cc42f645c5aa76ac3154b023359b665375fc3ae42f025fe961fb0f65205ad70e * $ ./kcapi -x 3 -c sha256 -p bbb300ac5eda9d * 61f7b48577a613fbdfe0d6d90b49985e07a42c99e7a439b6efb76d5ec71b3d30 ./kcapi -x 12 -c "hmac(sha256)" -k 0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b -i 000102030405060708090a0b0c -p f0f1f2f3f4f5f6f7f8f9 -b 42 * 3cb25f25faacd57a90434f64d0362f2a2d2d0a90cf1a5a4c5db02d56ecc4c5bf3400720 8d5b887185865 Paraller test with two different EE's (Execution Environment) EE1 (Trust Zone) EE2 (HLOS) There is a TZ application which "libkcapi" or "tcrypt.ko" will run in continous loop will do continious enc/dec with to do enc/dec with different algorithm supported different AES algorithm supported QCE crypto engine. by QCE crypto engine. 1) dummy write with LOCK bit set 1) dummy write with LOCK bit set 2) bam will lock all other pipes which not 2) bam will lock all other pipes which not belongs to current EE's, i.e HLOS pipe belongs to current EE's, i.e tz pipe and and keep handling current pipe only. keep handling current pipe only. 3) tz prepare data descriptor and submit to CE5 3) hlos prepare data descriptor and submit to CE5 4) dummy write with UNLOCK bit set 4) dummy write with UNLOCK bit set 5) bam will release all the locked pipes 5) bam will release all the locked pipes Upon encountering a descriptor with Lock bit set, the BAM will lock all other pipes not related to the current pipe group, and keep handling the current pipe only until it sees the Un-Lock set (then it will release all locked pipes). The actual locking is done on the new descriptor fetching for publishing, i.e. locked pipe will not fetch new descriptors even if it got event/events adding more descriptors for this pipe. > > Regards, > Bjorn