- Linaro-mm-sig - lists.linaro.org

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 27.06.24 um 05:17 schrieb Jason-JH Lin (林睿祥): > > On Wed, 2024-06-26 at 12:49 +0200, Christian König wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > In the step 3), we need to verify the dma-buf is allocated from > > > > > "restricted_mtk_cma", but there is no way to pass the > > > secure flag > > > > > or > > > > > private data from userspace to the import interface in DRM > > > driver. > > > > > > > > Why do you need to verify that? > > > > > > I need to know the imported buffer is allocated from restricted cma > > > and > > > mark it as a secure buffer in mediatek-drm driver. Then, I will add > > > some configuration to the hardware if the buffer is secure buffer, > > > so > > > that it can get the permission to access the secure buffer. > > > > Yeah so far that makes sense. This is basically what other drivers do > > with secure buffers as well. > > > > But why do you want the kernel to transport that information? Usually > > drivers get the information from userspace what to do with a buffer. > > > > In other words the format, stride, tilling and also if it's a secure > > buffer or not comes from userspace. > > > > Thanks for your clear explanation. > I think this is what I want, but I can't find any DRM interface to pass > the secure flag from userspace. Well stuff like that is usually device driver specific. So you should probably use something device specific which tells the driver that this buffer is encrypted. > > What the hardware usually handles internally is things like > > encryption keys, but you eventually get the information where to look > > for the key from userspace as well. > > > > Handling inside the kernel would only be necessary if userspace could > > for example crash the system with invalid parameters. But for > > encryption that is usually not the case. > > > > Yes, that's true. > > > > > > > > > > So I can only verify it like this now: > > > > > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct > > > > > drm_device > > > > > *dev, struct dma_buf_attachment *attach, struct sg_table *sg) > > > > > { > > > > > struct mtk_gem_obj *mtk_gem; > > > > > > > > > > /* check if the entries in the sg_table are contiguous */ > > > > > if (drm_prime_get_contiguous_size(sg) < > > > attach->dmabuf->size) { > > > > > DRM_ERROR("sg_table is not contiguous"); > > > > > return ERR_PTR(-EINVAL); > > > > > } > > > > > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > > > > > if (IS_ERR(mtk_gem)) > > > > > return ERR_CAST(mtk_gem); > > > > > > > > > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, > > > > > "restricted", > > > > > 10)); > > > > > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > > > > > mtk_gem->size = attach->dmabuf->size; > > > > > mtk_gem->sg = sg; > > > > > > > > > > return &mtk_gem->base; > > > > > } > > > > > > > > Complete NAK from my side to that approach. Importing of a DMA- > > > buf > > > > should be independent of the exporter. > > > > > > > > What you could do is to provide the secure buffer from a device > > > and > > > > not a device heap. > > > > > > > > > > You mean I should allocate buffer in mediate-drm driver not > > > userspace? > > > > Well that depends. The question is if you have multiple drivers which > > needs to work with this secure buffer? > > > > If yes then you should have a general allocation heap for it. If no > > then the buffers could as well be allocated from the driver interface > > directly. > > > > Yes, this buffer needs work with GPU and DRM drivers, so this general > "restricted_mtk_cma" will allocated in userspace, then being passed to > GPU and DRM. Well do you really need a separate heap for that? In other words is only a certain part of the system memory capable of being encrypted? Or would the "normal" CMA heap do as well and you only need to setup your hardware properly for encryption? Additional to that in most other drivers buffer sharing and encryption are two separate things. In other words other drivers do something like this: 1. Allocate the buffer. 2. Import the buffer using DRM_IOCTL_PRIME_FD_TO_HANDLE. 3. Set additional buffer properties, e.g. format, stride, tilling, if it's secure, which encryption key to use, where to map etc... So as far as I can see the problem you are facing is that you try to mangle everything into DRM_IOCTL_PRIME_FD_TO_HANDLE. Why not make that a separate IOCTL? I mean we intentionally don't provide things like format, stride, tilling etc.. to DRM_IOCTL_PRIME_FD_TO_HANDLE. Encryption is just another of those parameters. Regards, Christian. > > > > I just have modified this to userspace by the comment here: > > > > > > > https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.36… > > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > > > > > > > > > I think it would be better to have the user configurable > > > private > > > > > information in dma-buf, so all the drivers who have the same > > > > > requirement can get their private information from dma-buf > > > directly > > > > > and > > > > > no need to change or add the interface. > > > > > > > > > > What's your opinion in this point? > > > > > > > > Well of hand I don't see the need for that. > > > > > > > > What happens if you get a non-secure buffer imported in your > > > secure > > > > device? > > > > > > We use the same mediatek-drm driver for secure and non-secure > > > buffer. > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > the > > > normal flow with normal hardware settings. > > > > > > We use different configurations to make hardware have different > > > permission to access the buffer it should access. > > > > > > So if we can't get the information of "the buffer is allocated from > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > won't > > > be able to configure the hardware correctly. > > > > Why can't you get this information from userspace? > > As I mentioned here: > > https://patchwork.kernel.org/project/linux-mediatek/cover/20240525232928.55… > > I tried some DRM interfaces using buffer FD and arg->flag as > parameters, but it didn't work. So I ask for your help here. > > But I think I should find DRM maintainer to add the secure flag to DRM > interface now. > > Regards, > Jason-JH.Lin > > ************* MEDIATEK Confidentiality Notice ******************** > The information contained in this e-mail message (including any > attachments) may be confidential, proprietary, privileged, or otherwise > exempt from disclosure under applicable laws. It is intended to be > conveyed only to the designated recipient(s). Any use, dissemination, > distribution, printing, retaining or copying of this e-mail (including its > attachments) by unintended recipient(s) is strictly prohibited and may > be unlawful. If you are not an intended recipient of this e-mail, or believe > that you have received this e-mail in error, please notify the sender > immediately (by replying to this e-mail), delete any and all copies of > this e-mail (including any attachments) from your system, and do not > disclose the content of this e-mail to any other person. Thank you!

11 months, 2 weeks

1
0
0 0

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > In the step 3), we need to verify the dma-buf is allocated from > > > "restricted_mtk_cma", but there is no way to pass the secure flag > > > or > > > private data from userspace to the import interface in DRM driver. > > > > Why do you need to verify that? > > I need to know the imported buffer is allocated from restricted cma and > mark it as a secure buffer in mediatek-drm driver. Then, I will add > some configuration to the hardware if the buffer is secure buffer, so > that it can get the permission to access the secure buffer. Yeah so far that makes sense. This is basically what other drivers do with secure buffers as well. But why do you want the kernel to transport that information? Usually drivers get the information from userspace what to do with a buffer. In other words the format, stride, tilling and also if it's a secure buffer or not comes from userspace. What the hardware usually handles internally is things like encryption keys, but you eventually get the information where to look for the key from userspace as well. Handling inside the kernel would only be necessary if userspace could for example crash the system with invalid parameters. But for encryption that is usually not the case. > > > > > So I can only verify it like this now: > > > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct > > > drm_device > > > *dev, struct dma_buf_attachment *attach, struct sg_table *sg) > > > { > > > struct mtk_gem_obj *mtk_gem; > > > > > > /* check if the entries in the sg_table are contiguous */ > > > if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { > > > DRM_ERROR("sg_table is not contiguous"); > > > return ERR_PTR(-EINVAL); > > > } > > > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > > > if (IS_ERR(mtk_gem)) > > > return ERR_CAST(mtk_gem); > > > > > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, > > > "restricted", > > > 10)); > > > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > > > mtk_gem->size = attach->dmabuf->size; > > > mtk_gem->sg = sg; > > > > > > return &mtk_gem->base; > > > } > > > > Complete NAK from my side to that approach. Importing of a DMA-buf > > should be independent of the exporter. > > > > What you could do is to provide the secure buffer from a device and > > not a device heap. > > > > You mean I should allocate buffer in mediate-drm driver not userspace? Well that depends. The question is if you have multiple drivers which needs to work with this secure buffer? If yes then you should have a general allocation heap for it. If no then the buffers could as well be allocated from the driver interface directly. > I just have modified this to userspace by the comment here: > > https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.36… > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > > > > > I think it would be better to have the user configurable private > > > information in dma-buf, so all the drivers who have the same > > > requirement can get their private information from dma-buf directly > > > and > > > no need to change or add the interface. > > > > > > What's your opinion in this point? > > > > Well of hand I don't see the need for that. > > > > What happens if you get a non-secure buffer imported in your secure > > device? > > We use the same mediatek-drm driver for secure and non-secure buffer. > If non-secure buffer imported to mediatek-drm driver, it's go to the > normal flow with normal hardware settings. > > We use different configurations to make hardware have different > permission to access the buffer it should access. > > So if we can't get the information of "the buffer is allocated from > restricted_mtk_cma" when importing the buffer into the driver, we won't > be able to configure the hardware correctly. Why can't you get this information from userspace? Regards, Christian. > > Regards, > Jason-JH.Lin > > > > > Regards, > > Christian. > > > > > Regards, > > > Jason-JH.Lin > > > > > > > Regards, > > > > Christian. > > > > > > ************* MEDIATEK Confidentiality Notice > > > ******************** > > > The information contained in this e-mail message (including any > > > attachments) may be confidential, proprietary, privileged, or > > > otherwise > > > exempt from disclosure under applicable laws. It is intended to be > > > conveyed only to the designated recipient(s). Any use, > > > dissemination, > > > distribution, printing, retaining or copying of this e-mail > > > (including its > > > attachments) by unintended recipient(s) is strictly prohibited and > > > may > > > be unlawful. If you are not an intended recipient of this e-mail, > > > or believe > > > > > > that you have received this e-mail in error, please notify the > > > sender > > > immediately (by replying to this e-mail), delete any and all copies > > > of > > > this e-mail (including any attachments) from your system, and do > > > not > > > disclose the content of this e-mail to any other person. Thank you! > > > > ************* MEDIATEK Confidentiality Notice > ******************** > The information contained in this e-mail message (including any > attachments) may be confidential, proprietary, privileged, or otherwise > exempt from disclosure under applicable laws. It is intended to be > conveyed only to the designated recipient(s). Any use, dissemination, > distribution, printing, retaining or copying of this e-mail (including its > attachments) by unintended recipient(s) is strictly prohibited and may > be unlawful. If you are not an intended recipient of this e-mail, or believe > > that you have received this e-mail in error, please notify the sender > immediately (by replying to this e-mail), delete any and all copies of > this e-mail (including any attachments) from your system, and do not > disclose the content of this e-mail to any other person. Thank you!

11 months, 2 weeks

2
1
0 0

Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 19/06/2024 16:46, Alexandre Mergnat wrote: > Add the audio codec sub-device. This sub-device is used to set the > optional voltage values according to the hardware. > The properties are: > - Setup of microphone bias voltage. > - Setup of the speaker pin pull-down. > > Also, add the audio power supply property which is dedicated for > the audio codec sub-device. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > index 37423c2e0fdf..d95307393e75 100644 > --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > @@ -37,6 +37,32 @@ properties: > "#interrupt-cells": > const: 2 > > + vaud28-supply: > + description: 2.8 volt supply phandle for the audio codec > + > + audio-codec: > + type: object Still not much improved. You do not have any resources there, so these should go to the parent node. Best regards, Krzysztof

11 months, 2 weeks

1
1
0 0

Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 26/06/2024 10:30, Alexandre Mergnat wrote: > > > On 25/06/2024 15:44, Krzysztof Kozlowski wrote: >> On 25/06/2024 11:23, Alexandre Mergnat wrote: >>> >>> >>> On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >>>> On 19/06/2024 16:46, Alexandre Mergnat wrote: >>>>> Add the audio codec sub-device. This sub-device is used to set the >>>>> optional voltage values according to the hardware. >>>>> The properties are: >>>>> - Setup of microphone bias voltage. >>>>> - Setup of the speaker pin pull-down. >>>>> >>>>> Also, add the audio power supply property which is dedicated for >>>>> the audio codec sub-device. >>>>> >>>>> Signed-off-by: Alexandre Mergnat<amergnat(a)baylibre.com> >>>>> --- >>>>> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >>>>> 1 file changed, 33 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> index 37423c2e0fdf..d95307393e75 100644 >>>>> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> @@ -37,6 +37,32 @@ properties: >>>>> "#interrupt-cells": >>>>> const: 2 >>>>> >>>>> + vaud28-supply: >>>>> + description: 2.8 volt supply phandle for the audio codec >>>>> + >>>>> + audio-codec: >>>>> + type: object >>>> Still not much improved. You do not have any resources there, so these >>>> should go to the parent node. >>> >>> Hi Krzysztof, >>> >>> vaud28-supply seems to be a mistake that I forward port. >>> In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" >>> sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as >>> power-supply. >>> >>> pwrap { >>> pmic { >>> ... >>> audio-codec { >>> ... >>> >>> Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of >>> PMIC-audio-codec subsystem. Then: >>> - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a >>> power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) >>> - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to >>> Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec >>> substystem, beside mediatek,micbias0-microvolt >> >> I don't understand why do we talk again about supply. My comment was not >> under the supply. > > Because your word are: > " > And now you should see how odd it looks. Supplies are part of entire > chip, not subblock, even if they supply dedicated domain within that chip. > > That's why I asked to put it in the parent node. > " > > My bad, I forgot to link you the old message in my previous answer [1] > > [1] https://lore.kernel.org/all/6d21da37-8be7-467c-8878-d57af0b0201b@kernel.org… And you implemented this, so why do we talk again about it? It is already solved, isn't it? Since previous version? Best regards, Krzysztof

11 months, 2 weeks

1
0
0 0

Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 25/06/2024 11:23, Alexandre Mergnat wrote: > > > On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >> On 19/06/2024 16:46, Alexandre Mergnat wrote: >>> Add the audio codec sub-device. This sub-device is used to set the >>> optional voltage values according to the hardware. >>> The properties are: >>> - Setup of microphone bias voltage. >>> - Setup of the speaker pin pull-down. >>> >>> Also, add the audio power supply property which is dedicated for >>> the audio codec sub-device. >>> >>> Signed-off-by: Alexandre Mergnat<amergnat(a)baylibre.com> >>> --- >>> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >>> 1 file changed, 33 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> index 37423c2e0fdf..d95307393e75 100644 >>> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> @@ -37,6 +37,32 @@ properties: >>> "#interrupt-cells": >>> const: 2 >>> >>> + vaud28-supply: >>> + description: 2.8 volt supply phandle for the audio codec >>> + >>> + audio-codec: >>> + type: object >> Still not much improved. You do not have any resources there, so these >> should go to the parent node. > > Hi Krzysztof, > > vaud28-supply seems to be a mistake that I forward port. > In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" > sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as > power-supply. > > pwrap { > pmic { > ... > audio-codec { > ... > > Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of > PMIC-audio-codec subsystem. Then: > - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a > power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) > - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to > Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec > substystem, beside mediatek,micbias0-microvolt I don't understand why do we talk again about supply. My comment was not under the supply. Best regards, Krzysztof

11 months, 2 weeks

1
0
0 0

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 25.06.24 um 13:02 schrieb Jason-JH Lin (林睿祥): > > Hi Christian, > > On Tue, 2024-05-21 at 20:36 +0200, Christian König wrote: > > Am 20.05.24 um 09:58 schrieb Yong Wu (吴勇): > > > On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote: > > > > > > > > External email : Please do not click links or open attachments > > > > until > > > > you have verified the sender or the content. > > > > Am 15.05.24 um 13:23 schrieb Yong Wu: > > > > > Introduce a FLAG for the restricted memory which means the > > > > > memory > > > > > > > > is > > > > > protected by TEE or hypervisor, then it's inaccessiable for > > > > > kernel. > > > > > > > > > > Currently we don't use sg_dma_unmark_restricted, thus this > > > > > > > > interface > > > > > has not been added. > > > > > > > > Why should that be part of the scatterlist? It doesn't seem to > > > > affect > > > > any of it's functionality. > > > > > > > > As far as I can see the scatterlist shouldn't be the transport of > > > > this > > > > kind of information. > > > > > > Thanks for the review. I will remove this. > > > > > > In our user scenario, DRM will import these buffers and check if > > > this > > > is a restricted buffer. If yes, it will use secure GCE takes over. > > > > > > If this judgment is not suitable to be placed in scatterlist. I > > > don't > > > know if it is ok to limit this inside dma-buf. Adding such an > > > interface: > > > > > > static bool dma_buf_is_restricted(struct dma_buf *dmabuf) > > > { > > > return !strncmp(dmabuf->exp_name, "restricted", 10); > > > } > > > > No, usually stuff like that doesn't belong into DMA buf either. > > > > Question here really is who controls the security status of the > > memory > > backing the buffer? > > > > In other words who tells the exporter that it should allocate and > > fill a > > buffer with encrypted data? > > > > If that is userspace then that is part of the format information and > > it > > is also userspace who should tell the importer that it needs to work > > with encrypted data. > > > > The kernel is intentionally not involved in stuff like that. > > > > Here is the expected protected content buffer flow in DRM: > 1) userspace allocates a dma-buf FD from the "restricted_mtk_cma" by > DMA_HEAP_IOCTL_ALLOC. > 2) userspace imports that dma-buf into the device using prime for the > drm_file. > 3) userspace uses the already implemented driver import code for the > special cases of protected content buffer. What is so special on that case? > > In the step 3), we need to verify the dma-buf is allocated from > "restricted_mtk_cma", but there is no way to pass the secure flag or > private data from userspace to the import interface in DRM driver. Why do you need to verify that? > > So I can only verify it like this now: > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device > *dev, struct dma_buf_attachment *attach, struct sg_table *sg) > { > struct mtk_gem_obj *mtk_gem; > > /* check if the entries in the sg_table are contiguous */ > if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { > DRM_ERROR("sg_table is not contiguous"); > return ERR_PTR(-EINVAL); > } > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > if (IS_ERR(mtk_gem)) > return ERR_CAST(mtk_gem); > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", > 10)); > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > mtk_gem->size = attach->dmabuf->size; > mtk_gem->sg = sg; > > return &mtk_gem->base; > } Complete NAK from my side to that approach. Importing of a DMA-buf should be independent of the exporter. What you could do is to provide the secure buffer from a device and not a device heap. > I think I have the same problem as the ECC_FLAG mention in: > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > I think it would be better to have the user configurable private > information in dma-buf, so all the drivers who have the same > requirement can get their private information from dma-buf directly and > no need to change or add the interface. > > What's your opinion in this point? Well of hand I don't see the need for that. What happens if you get a non-secure buffer imported in your secure device? Regards, Christian. > > Regards, > Jason-JH.Lin > > > Regards, > > Christian. > > ************* MEDIATEK Confidentiality Notice > ******************** > The information contained in this e-mail message (including any > attachments) may be confidential, proprietary, privileged, or otherwise > exempt from disclosure under applicable laws. It is intended to be > conveyed only to the designated recipient(s). Any use, dissemination, > distribution, printing, retaining or copying of this e-mail (including its > attachments) by unintended recipient(s) is strictly prohibited and may > be unlawful. If you are not an intended recipient of this e-mail, or believe > > that you have received this e-mail in error, please notify the sender > immediately (by replying to this e-mail), delete any and all copies of > this e-mail (including any attachments) from your system, and do not > disclose the content of this e-mail to any other person. Thank you!

11 months, 2 weeks

1
0
0 0

Re: [PATCH v6 12/16] ASoC: codecs: add MT6357 support

by Mark Brown

On Wed, Jun 19, 2024 at 04:46:48PM +0200, amergnat(a)baylibre.com wrote: > From: Nicolas Belin <nbelin(a)baylibre.com> > > Add the support of MT6357 PMIC audio codec. This breaks an x86 allmodconfig build: /build/stage/linux/sound/soc/codecs/mt6357.c: In function ‘mt_delay_250_event’: /build/stage/linux/sound/soc/codecs/mt6357.c:993:29: error: unused variable ‘pri v’ [-Werror=unused-variable] 993 | struct mt6357_priv *priv = snd_soc_component_get_drvdata(cmpnt); | ^~~~ /build/stage/linux/sound/soc/codecs/mt6357.c: In function ‘mt6357_platform_drive r_probe’: /build/stage/linux/sound/soc/codecs/mt6357.c:1867:55: error: too many arguments for format [-Werror=format-extra-args] 1867 | return dev_err_probe(&pdev->dev, ret, "Failed to parse d ts\n", __func__); | ^~~~~~~~~~~~~~~~~~ ~~~~~ cc1: all warnings being treated as errors

11 months, 3 weeks

1
0
0 0

Re: [PATCH v6 12/16] ASoC: codecs: add MT6357 support

by Mark Brown

On Wed, Jun 19, 2024 at 04:46:48PM +0200, amergnat(a)baylibre.com wrote: > + /* gain default values*/ > + regmap_update_bits(priv->regmap, MT6357_AUDENC_ANA_CON0, MT6357_AUDPREAMPLGAIN_MASK, > + UL_GAIN_0DB << MT6357_AUDPREAMPLGAIN_SFT); > + regmap_update_bits(priv->regmap, MT6357_AUDENC_ANA_CON1, MT6357_AUDPREAMPRGAIN_MASK, > + UL_GAIN_0DB << MT6357_AUDPREAMPRGAIN_SFT); > + > + regmap_update_bits(priv->regmap, MT6357_ZCD_CON1, > + MT6357_AUD_LOL_GAIN_MASK | > + MT6357_AUD_LOR_GAIN_MASK, > + DL_GAIN_0DB << MT6357_AUD_LOL_GAIN_SFT | > + DL_GAIN_0DB << MT6357_AUD_LOR_GAIN_SFT); > + > + regmap_update_bits(priv->regmap, MT6357_ZCD_CON2, > + MT6357_AUD_HPL_GAIN_MASK | > + MT6357_AUD_HPR_GAIN_MASK, > + DL_GAIN_0DB << MT6357_AUD_HPL_GAIN_SFT | > + DL_GAIN_0DB << MT6357_AUD_HPR_GAIN_SFT); > + > + regmap_update_bits(priv->regmap, MT6357_ZCD_CON3, > + MT6357_AUD_HS_GAIN_MASK, DL_GAIN_0DB); We generally leave everything at chip defaults, why is this different? > +static int mt6357_codec_probe(struct snd_soc_component *codec) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + > + mt6357_codec_init(priv); > + return 0; > +} Why not just inline mt6357_codec_init() into the one user? > +static unsigned int mt6357_read(struct snd_soc_component *codec, unsigned int reg) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + unsigned int val; > + > + regmap_read(priv->regmap, reg, &val); > + return val; > +} > + > +static int mt6357_write(struct snd_soc_component *codec, unsigned int reg, unsigned int value) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + > + return regmap_update_bits(priv->regmap, reg, 0xffff, value); > +} Why open code these, the core has standard adaptors for regmap? > +static const u32 micbias_values[MT6357_MICBIAS_ARRAY_SIZE] = { > + 1700000, 1800000, 1900000, 2000000, > + 2100000, 2500000, 2600000, 2700000 > +}; Just use ARRAY_SIZE() for the size then the number can't be out of sync.

11 months, 3 weeks

1
0
0 0

Re: [PATCH v6 0/7] Add mediate-drm secure flow for SVP

by mripard＠kernel.org

Hi, On Tue, Jun 11, 2024 at 09:13:03AM GMT, Jason-JH Lin (林睿祥) wrote: > Hi Maxime, > > [snip] > > > > > > > --- > > > > > > TODO: > > > > > > 1) Drop MTK_DRM_IOCTL_GEM_CREATE and use DMA_HEAP_IOCTL_ALLOC > > > > > > in > > > > > > userspace > > > > > > 2) DRM driver use secure mailbox channel to handle normal and > > > > > > secure flow > > > > > > 3) Implement setting mmsys routing table in the secure world > > > > > > series > > > > > > > > > > I'm not sure what you mean here. Why are you trying to upstream > > > > > something that still needs to be removed from your patch > > > > > series? > > > > > > > > > > > > > Because their is too much patches need to be fixed in this > > > > series, > > > > so I > > > > list down the remaining TODO items and send to review for the > > > > other > > > > patches. > > > > > > > > Sorry for the bothering, I'll drop this at the next version. > > > > > > If you don't intend to use it, we just shouldn't add it. Removing > > > the > > > TODO item doesn't make sense, even more so if heaps should be the > > > way > > > you handle this. > > > > > > > Sorry for this misunderstanding. > > > > I mean I'll remove the DRM_IOCTL_GEM_CREATE patch and then change > > user > > space calling DMA_HEAP_IOCTL_ALLOC to allocate buffer from secure > > heap. > > > > I have changed user space to use DMA_HEAP_IOCTL_ALLOC to allocate > secure buffer, but I still encounter the problem of determining whether > the buffer is secure in mediatek-drm driver to add some secure > configure for hardware. > > > As the comment in [1], dma driver won't provide API for use. > [1]: > https://patchwork.kernel.org/project/linux-mediatek/patch/20240515112308.10… > > > So I use name checking at [PATCH v6 3/7] like this currently: > > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device > *dev, > struct dma_buf_attachment *attach, struct sg_table *sg) > { > struct mtk_gem_obj *mtk_gem; > > /* check if the entries in the sg_table are contiguous */ > if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { > DRM_ERROR("sg_table is not contiguous"); > return ERR_PTR(-EINVAL); > } > > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > if (IS_ERR(mtk_gem)) > return ERR_CAST(mtk_gem); > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", > 10)); > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > + mtk_gem->size = attach->dmabuf->size; > mtk_gem->sg = sg; > > return &mtk_gem->base; > } > > But I want to change this name checking to the information brought from > user space. > I tried to use arg->flags to append the secure flag in user space and > call drmPrimeHandleToFD() to pass it to DRM driver, but it will be > blocked by at the beginning of the drm_prime_handle_to_fd_ioctl(). I agree with you, it's something to discuss mostly with the dma-buf maintainers but it would be better to just set a flag on the dma-buf, and use that flag whenever necessary. It might be related to the recent work I did to introduce allocation flags too: https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… Maxime

11 months, 3 weeks

1
0
0 0

Re: [RFC] Patch review challenges

by Lee Jones

On Fri, 21 Jun 2024, Markus Elfring wrote: > > The issue is one of communication and the way reviews are conducted. > > > > Reviewing other people's work is challenging and requires a certain > > skill-set, of which _excellent_ communication skills are non-negotiable. > > Patch feedback and change tolerance can vary also according to involved communities. Agreed. For this community, I suggest you build your skills for a while longer. > > Why not concentrate on more complex submissions for a while and grow > > your repertoire of common review points, > > Further collateral evolution can be considered there depending on > corresponding development resources. > > > rather than repeating the same few over and over? > > Some factors are probably known also according to corresponding statistics. > Several contributors are stumbling on recurring improvement possibilities > in published information. Right, this will always be true, however the few you've picked up on are not important enough to keep reiterating. By doing so, you're receiving undesirable attention. > > Reading other, more experienced maintainer's reviews would also be a good use > > of your time. > > I am trying to influence adjustments in desirable directions for a while. Never stop trying to improve. These are only my opinions of course. Take the advice or leave it. There's no need to reply to this. -- Lee Jones [李琼斯]

11 months, 3 weeks

1
0
0 0

Re: [v11 3/7] iio: core: Add new DMABUF interface infrastructure

by Lee Jones

On Fri, 21 Jun 2024, Markus Elfring wrote: > > Sadly, I am yet to see a constructive approach or even better a helpful > > patch which improve something, rather than vague suggestions on the list > > Can you get any more constructive impressions from another data representation? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=… > > Are you aware how many change suggestions (also from my selection) are still > in various waiting queues? No one is doubting your overall contributions Markus. The issue is one of communication and the way reviews are conducted. Reviewing other people's work is challenging and requires a certain skill-set, of which _excellent_ communication skills are non-negotiable. Why not concentrate on more complex submissions for a while and grow your repertoire of common review points, rather than repeating the same few over and over? Reading other, more experienced maintainer's reviews would also be a good use of your time. -- Lee Jones [李琼斯]

11 months, 3 weeks

1
0
0 0

Re: [v11 3/7] iio: core: Add new DMABUF interface infrastructure

by Vinod Koul

On 20-06-24, 12:45, Markus Elfring wrote: > … > > All errors (new ones prefixed by >>): > > > >>> drivers/iio/industrialio-buffer.c:1715:3: error: cannot jump from this goto statement to its label > > 1715 | goto err_dmabuf_unmap_attachment; > … > > Which software design options would you like to try out next > so that such a questionable compilation error message will be avoided finally? The one where all emails from Markus go to dev/null -- ~Vinod

11 months, 3 weeks

3
4
0 0

Re: [Patch V3] i2c: imx-lpi2c: add eDMA mode support for LPI2C

by kernel test robot

Hi, kernel test robot noticed the following build warnings: [auto build test WARNING on andi-shyti/i2c/i2c-host] [also build test WARNING on linus/master v6.10-rc4 next-20240620] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/carlos-song-nxp-com/i2c-imx-… base: git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host patch link: https://lore.kernel.org/r/20240614045616.1751784-1-carlos.song%40nxp.com patch subject: [Patch V3] i2c: imx-lpi2c: add eDMA mode support for LPI2C config: loongarch-randconfig-r071-20240620 (https://download.01.org/0day-ci/archive/20240621/202406210604.bil4fzu2-lkp@…) compiler: loongarch64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240621/202406210604.bil4fzu2-lkp@…) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp(a)intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202406210604.bil4fzu2-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/i2c/busses/i2c-imx-lpi2c.o: warning: objtool: unexpected relocation symbol type in .rela.discard.reachable -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki

11 months, 3 weeks

1
0
0 0

Re: [PATCH v12 3/7] iio: core: Add new DMABUF interface infrastructure

by Lee Jones

On Thu, 20 Jun 2024, Markus Elfring wrote: > … > > v11: > … > > - Use guard(mutex) > > > > v12: > > - Revert to mutex_lock/mutex_unlock in iio_buffer_attach_dmabuf(), > > as it uses cleanup GOTOs > … > > I would find it nice if better design options could gain acceptance. > Will the chances grow to adjust scopes another bit for involved variables > in such function implementations? > > A) Enclosing a source code part with extra curly brackets? > > B) scoped_guard()? > https://elixir.bootlin.com/linux/v6.10-rc4/source/include/linux/cleanup.h#L… > > C) Moving a locked source code part into a separate function implementation? I think it would help your cause if you quoted the exact piece of code you're referring to. Then tone down the language a bit - keep it as simple and natural as you can. Ex 1: Please place curly brackets around this section to aid with <reason> Ex 2: To save N lines of clean-up, please use scoped_guard() Ex 3: Moving out this chunk to another function would help with <reason> Etc. -- Lee Jones [李琼斯]

11 months, 3 weeks

1
0
0 0

Re: [PATCH v6 12/16] ASoC: codecs: add MT6357 support

by AngeloGioacchino Del Regno

Il 19/06/24 16:46, amergnat(a)baylibre.com ha scritto: > From: Nicolas Belin <nbelin(a)baylibre.com> > > Add the support of MT6357 PMIC audio codec. > > Signed-off-by: Nicolas Belin <nbelin(a)baylibre.com> > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com>

11 months, 3 weeks

1
0
0 0

Re: [PATCH v6 16/16] arm64: dts: mediatek: add audio support for mt8365-evk

by AngeloGioacchino Del Regno

Il 19/06/24 16:46, Alexandre Mergnat ha scritto: > Add the sound node which is linked to the MT8365 SoC AFE and > the MT6357 audio codec. > > Update the file header. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com>

11 months, 3 weeks

1
0
0 0

Re: [PATCH] dma-buf/heaps: Correct the types of fd_flags and heap_flags

by Carlos Llamas

On Thu, Jun 06, 2024 at 02:02:13PM +1200, Barry Song wrote: > From: Barry Song <v-songbaohua(a)oppo.com> > > dma_heap_allocation_data defines the UAPI as follows: > > struct dma_heap_allocation_data { > __u64 len; > __u32 fd; > __u32 fd_flags; > __u64 heap_flags; > }; > > But dma heaps are casting both fd_flags and heap_flags into > unsigned long. This patch makes dma heaps - cma heap and > system heap have consistent types with UAPI. > > Signed-off-by: Barry Song <v-songbaohua(a)oppo.com> > --- Looks good to me, thanks! Reviewed-by: Carlos Llamas <cmllamas(a)google.com>

11 months, 3 weeks

2
1
0 0

Re: [v11 3/7] iio: core: Add new DMABUF interface infrastructure

by Paul Cercueil

Le mercredi 19 juin 2024 à 13:56 +0200, Markus Elfring a écrit : > … > > https://lore.kernel.org/linux-iio/219abc43b4fdd4a13b307ed2efaa0e6869e68e3f.… > > > > (and responses below) > > > > It's more nuanced than I remembered. > … > > > > > * Will the desire grow for further collateral evolution according > > > to > > > affected software components? > > > > Not sure what you mean by that. > > Advanced programming interfaces were added a while ago. > > Example: > https://elixir.bootlin.com/linux/v6.10-rc4/source/include/linux/cleanup.h#L8 > > Corresponding attempts for increasing API usage need to adapt to > remaining change reluctance, > don't they? Sure, I guess. But that does not change the fact that I cannot use cleanup.h magic in this patchset, yet, as the required changes would have to be done in a separate one. Cheers, -Paul

11 months, 3 weeks

2
1
0 0

[PATCH v11 0/7] iio: new DMABUF based API v11

by Paul Cercueil

Hi Jonathan, Here's the v11 of my patchset that introduces DMABUF support to IIO. It addresses the few points that were raised in the review of the v10. It also adds Nuno as a co-developer. Changelog: - [3/7]: - Document .lock_queue / .unlock_queue buffer callbacks - Add small comment to explain what the spinlock protects - Use guard(mutex) - [4/7]: - Remove useless field "attach" in struct iio_dma_buffer_block - Document "sg_table" and "fence" fields in struct iio_block_state - [6/7]: - "a IIO buffer" -> "an IIO buffer" - Add variable name in IOCTL calls - [7/7]: New patch, to document the DMA changes Cheers, -Paul Paul Cercueil (7): dmaengine: Add API function dmaengine_prep_peripheral_dma_vec() dmaengine: dma-axi-dmac: Implement device_prep_peripheral_dma_vec iio: core: Add new DMABUF interface infrastructure iio: buffer-dma: Enable support for DMABUFs iio: buffer-dmaengine: Support new DMABUF based userspace API Documentation: iio: Document high-speed DMABUF based API Documentation: dmaengine: Document new dma_vec API Documentation/driver-api/dmaengine/client.rst | 9 + .../driver-api/dmaengine/provider.rst | 10 + Documentation/iio/iio_dmabuf_api.rst | 54 +++ Documentation/iio/index.rst | 1 + drivers/dma/dma-axi-dmac.c | 40 ++ drivers/iio/Kconfig | 1 + drivers/iio/buffer/industrialio-buffer-dma.c | 178 ++++++- .../buffer/industrialio-buffer-dmaengine.c | 62 ++- drivers/iio/industrialio-buffer.c | 457 ++++++++++++++++++ include/linux/dmaengine.h | 33 ++ include/linux/iio/buffer-dma.h | 31 ++ include/linux/iio/buffer_impl.h | 33 ++ include/uapi/linux/iio/buffer.h | 22 + 13 files changed, 911 insertions(+), 20 deletions(-) create mode 100644 Documentation/iio/iio_dmabuf_api.rst -- 2.43.0

11 months, 3 weeks

4
11
0 0

Re: [PATCH v11 3/7] iio: core: Add new DMABUF interface infrastructure

by Paul Cercueil

Le mercredi 19 juin 2024 à 13:43 +0200, Markus Elfring a écrit : > … > > +++ b/drivers/iio/industrialio-buffer.c > … > > +static void iio_buffer_dmabuf_release(struct kref *ref) > > +{ > … > > + dma_resv_lock(dmabuf->resv, NULL); > > + dma_buf_unmap_attachment(attach, priv->sgt, priv->dir); > > + dma_resv_unlock(dmabuf->resv); > … > > Under which circumstances will another lock guard become applicable? > https://elixir.bootlin.com/linux/v6.10-rc4/source/include/linux/cleanup.h#L… As soon as "struct dma_resv" gets a DEFINE_GUARD(). -Paul

11 months, 3 weeks

1
0
0 0

Re: [v11 3/7] iio: core: Add new DMABUF interface infrastructure

by Paul Cercueil

Le mercredi 19 juin 2024 à 13:13 +0200, Markus Elfring a écrit : > > > Would you dare to transform the remaining goto chain into further > > > applications > > > of scope-based resource management? > > > > We discussed this after v6 or v7, DRM/DMABUF maintainers were not > > keen > > on doing that *just yet*. > > * Would you like to add any links for corresponding development > discussions? https://lore.kernel.org/linux-iio/219abc43b4fdd4a13b307ed2efaa0e6869e68e3f.… (and responses below) It's more nuanced than I remembered. Christian was OK to add cleanup.h support to the DMABUF code as long as the examples were updated as well, but those aren't good candidates as they don't free up the resources in all code paths. > > * Will the desire grow for further collateral evolution according to > affected software components? Not sure what you mean by that. Cheers, -Paul

11 months, 3 weeks

1
0
0 0

Re: [v11 3/7] iio: core: Add new DMABUF interface infrastructure

by Paul Cercueil

Hi Markus, Le mercredi 19 juin 2024 à 12:03 +0200, Markus Elfring a écrit : > … > > All errors (new ones prefixed by >>): > > > > > > drivers/iio/industrialio-buffer.c:1715:3: error: cannot jump > > > > from this goto statement to its label > > 1715 | goto err_dmabuf_unmap_attachment; > … > > Would you dare to transform the remaining goto chain into further > applications > of scope-based resource management? We discussed this after v6 or v7, DRM/DMABUF maintainers were not keen on doing that *just yet*. Cheers, -Paul

11 months, 3 weeks

1
0
0 0

Re: [PATCH RESEND v5 16/16] arm64: dts: mediatek: add audio support for mt8365-evk

by AngeloGioacchino Del Regno

Il 14/06/24 09:27, Alexandre Mergnat ha scritto: > Add the sound node which is linked to the MT8365 SoC AFE and > the MT6357 audio codec. > > Update the file header. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 89 +++++++++++++++++++++++++++++ > 1 file changed, 89 insertions(+) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts > index 50cbaefa1a99..1d5457f9a4c2 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts > +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts > @@ -4,6 +4,7 @@ > * Authors: > * Fabien Parent <fparent(a)baylibre.com> > * Bernhard Rosenkränzer <bero(a)baylibre.com> > + * Alexandre Mergnat <amergnat(a)baylibre.com> > */ > > /dts-v1/; > @@ -86,6 +87,28 @@ optee_reserved: optee@43200000 { > reg = <0 0x43200000 0 0x00c00000>; > }; > }; > + > + sound: sound { > + compatible = "mediatek,mt8365-mt6357"; > + pinctrl-names = "default", > + "dmic", > + "miso_off", > + "miso_on", > + "mosi_off", > + "mosi_on"; > + pinctrl-0 = <&aud_default_pins>; > + pinctrl-1 = <&aud_dmic_pins>; > + pinctrl-2 = <&aud_miso_off_pins>; > + pinctrl-3 = <&aud_miso_on_pins>; > + pinctrl-4 = <&aud_mosi_off_pins>; > + pinctrl-5 = <&aud_mosi_on_pins>; > + mediatek,platform = <&afe>; > + }; > +}; > + > +&afe { > + mediatek,dmic-mode = <1>; > + status = "okay"; > }; > > &cpu0 { > @@ -178,9 +201,75 @@ &mt6357_pmic { > interrupts-extended = <&pio 145 IRQ_TYPE_LEVEL_HIGH>; > interrupt-controller; > #interrupt-cells = <2>; > + vaud28-supply = <&mt6357_vaud28_reg>; > + audio-codec { > + mediatek,micbias0-microvolt = <1900000>; > + mediatek,micbias1-microvolt = <1700000>; > + }; > }; > > &pio { > + aud_default_pins: audiodefault-pins { > + pins { clk-dat-pins > + pinmux = <MT8365_PIN_72_CMDAT4__FUNC_I2S3_BCK>, > + <MT8365_PIN_73_CMDAT5__FUNC_I2S3_LRCK>, > + <MT8365_PIN_74_CMDAT6__FUNC_I2S3_MCK>, > + <MT8365_PIN_75_CMDAT7__FUNC_I2S3_DO>; > + }; > + }; > + > + aud_dmic_pins: audiodmic-pins { > + pins { clk-dat-pins > + pinmux = <MT8365_PIN_117_DMIC0_CLK__FUNC_DMIC0_CLK>, > + <MT8365_PIN_118_DMIC0_DAT0__FUNC_DMIC0_DAT0>, > + <MT8365_PIN_119_DMIC0_DAT1__FUNC_DMIC0_DAT1>; > + }; > + }; > + > + aud_miso_off_pins: misooff-pins { > + pins { clk-dat-pins and same for all the others. > + pinmux = <MT8365_PIN_53_AUD_CLK_MISO__FUNC_GPIO53>, > + <MT8365_PIN_54_AUD_SYNC_MISO__FUNC_GPIO54>, > + <MT8365_PIN_55_AUD_DAT_MISO0__FUNC_GPIO55>, > + <MT8365_PIN_56_AUD_DAT_MISO1__FUNC_GPIO56>; > + input-enable; > + bias-pull-down; > + drive-strength = <MTK_DRIVE_2mA>; drive-strength = <2>; > + }; > + }; > + > + aud_miso_on_pins: misoon-pins { > + pins { > + pinmux = <MT8365_PIN_53_AUD_CLK_MISO__FUNC_AUD_CLK_MISO>, > + <MT8365_PIN_54_AUD_SYNC_MISO__FUNC_AUD_SYNC_MISO>, > + <MT8365_PIN_55_AUD_DAT_MISO0__FUNC_AUD_DAT_MISO0>, > + <MT8365_PIN_56_AUD_DAT_MISO1__FUNC_AUD_DAT_MISO1>; > + drive-strength = <MTK_DRIVE_6mA>; = <6>; > + }; > + }; > + > + aud_mosi_off_pins: mosioff-pins { > + pins { > + pinmux = <MT8365_PIN_49_AUD_CLK_MOSI__FUNC_GPIO49>, > + <MT8365_PIN_50_AUD_SYNC_MOSI__FUNC_GPIO50>, > + <MT8365_PIN_51_AUD_DAT_MOSI0__FUNC_GPIO51>, > + <MT8365_PIN_52_AUD_DAT_MOSI1__FUNC_GPIO52>; > + input-enable; > + bias-pull-down; > + drive-strength = <MTK_DRIVE_2mA>; = <2>; > + }; > + }; > + > + aud_mosi_on_pins: mosion-pins { > + pins { > + pinmux = <MT8365_PIN_49_AUD_CLK_MOSI__FUNC_AUD_CLK_MOSI>, > + <MT8365_PIN_50_AUD_SYNC_MOSI__FUNC_AUD_SYNC_MOSI>, > + <MT8365_PIN_51_AUD_DAT_MOSI0__FUNC_AUD_DAT_MOSI0>, > + <MT8365_PIN_52_AUD_DAT_MOSI1__FUNC_AUD_DAT_MOSI1>; > + drive-strength = <MTK_DRIVE_6mA>; = <6>; Cheers, Angelo

11 months, 3 weeks

1
0
0 0

Re: [PATCH RESEND v5 15/16] arm64: dts: mediatek: add afe support for mt8365 SoC

by AngeloGioacchino Del Regno

Il 14/06/24 09:27, Alexandre Mergnat ha scritto: > Add audio front end support of MT8365 SoC. > Update the file header. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com>

11 months, 3 weeks

1
0
0 0

Re: [PATCH RESEND v5 04/16] ASoC: mediatek: mt8365: Add common header

by AngeloGioacchino Del Regno

Il 14/06/24 09:27, Alexandre Mergnat ha scritto: > Add header files for register definition and structure. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > sound/soc/mediatek/mt8365/mt8365-afe-common.h | 491 +++++++++++++ > sound/soc/mediatek/mt8365/mt8365-reg.h | 991 ++++++++++++++++++++++++++ > 2 files changed, 1482 insertions(+) > > diff --git a/sound/soc/mediatek/mt8365/mt8365-afe-common.h b/sound/soc/mediatek/mt8365/mt8365-afe-common.h > new file mode 100644 > index 000000000000..4d8f8c4b19e3 > --- /dev/null > +++ b/sound/soc/mediatek/mt8365/mt8365-afe-common.h > @@ -0,0 +1,491 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * Mediatek 8365 audio driver common definitions > + * > + * Copyright (c) 2024 MediaTek Inc. > + * Authors: Jia Zeng <jia.zeng(a)mediatek.com> > + * Alexandre Mergnat <amergnat(a)baylibre.com> > + */ > + > +#ifndef _MT8365_AFE_COMMON_H_ > +#define _MT8365_AFE_COMMON_H_ > + > +#define COMMON_CLOCK_FRAMEWORK_API > +#define IDLE_TASK_DRIVER_API > +#define ENABLE_AFE_APLL_TUNER Those three definitions do not exist upstream. Please remove. > + > +#include <linux/clk.h> > +#include <linux/list.h> > +#include <linux/regmap.h> > +#include <sound/soc.h> > +#include <sound/asound.h> > +#include "../common/mtk-base-afe.h" > +#include "mt8365-reg.h" > + > +#define ENUM_TO_STR(enum) #enum Unused definition > + > +#define snd_soc_dai_stream_active_playback(dai) \ > + snd_soc_dai_stream_active(dai, SNDRV_PCM_STREAM_PLAYBACK) > +#define snd_soc_dai_stream_active_capture(dai) \ > + snd_soc_dai_stream_active(dai, SNDRV_PCM_STREAM_CAPTURE) > + Those are used only once and only in mt8365-dai-pcm.c, and I just noticed that. Can you please just remove those two and directly call snd_soc_dai_stream_active() with the right params in function mt8365_dai_pcm1_prepare()? > +enum { > + MT8365_AFE_MEMIF_DL1, > + MT8365_AFE_MEMIF_DL2, > + MT8365_AFE_MEMIF_TDM_OUT, ..snip.. > + > +#ifdef CONFIG_MTK_HIFIXDSP_SUPPORT This configuration option doesn't exist. Please remove the ifdef and the enclosed code entirely, as it's unused. > +struct mt8365_adsp_data { > + /* information adsp supply */ > + bool adsp_on; > + int (*hostless_active)(void); > + /* information afe supply */ > + int (*set_afe_memif)(struct mtk_base_afe *afe, > + int memif_id, > + unsigned int rate, > + unsigned int channels, > + snd_pcm_format_t format); > + int (*set_afe_memif_enable)(struct mtk_base_afe *afe, > + int memif_id, > + unsigned int rate, > + unsigned int period_size, > + int enable); > + void (*get_afe_memif_sram)(struct mtk_base_afe *afe, > + int memif_id, > + unsigned int *paddr, > + unsigned int *size); > + void (*set_afe_init)(struct mtk_base_afe *afe); > + void (*set_afe_uninit)(struct mtk_base_afe *afe); > +}; > +#endif > + > +struct mt8365_afe_private { > + struct clk *clocks[MT8365_CLK_NUM]; > + struct regmap *topckgen; > + struct mt8365_fe_dai_data fe_data[MT8365_AFE_MEMIF_NUM]; > + struct mt8365_be_dai_data be_data[MT8365_AFE_BACKEND_NUM]; > + struct mt8365_control_data ctrl_data; > + struct mt8365_gasrc_data gasrc_data[MT8365_TDM_ASRC_NUM]; > +#ifdef CONFIG_MTK_HIFIXDSP_SUPPORT > + struct mt8365_adsp_data adsp_data; ditto > +#endif > + int afe_on_ref_cnt; > + int top_cg_ref_cnt[MT8365_TOP_CG_NUM]; > + void __iomem *afe_sram_vir_addr; > + unsigned int afe_sram_phy_addr; > + unsigned int afe_sram_size; > + /* locks */ > + spinlock_t afe_ctrl_lock; > + struct mutex afe_clk_mutex; /* Protect & sync APLL TUNER registers access*/ > +#ifdef CONFIG_DEBUG_FS > + struct dentry *debugfs_dentry[MT8365_AFE_DEBUGFS_NUM]; > +#endif > + int apll_tuner_ref_cnt[MT8365_AFE_APLL_NUM]; > + unsigned int tdm_out_mode; > + unsigned int cm2_mux_input; > + > + /* dai */ > + bool dai_on[MT8365_AFE_BACKEND_END]; > + void *dai_priv[MT8365_AFE_BACKEND_END]; > +}; > + .... > +#ifdef CONFIG_MTK_HIFIXDSP_SUPPORT same > +struct mtk_base_afe *mt8365_afe_pcm_get_info(void); > +#endif > + > +int mt8365_dai_i2s_register(struct mtk_base_afe *afe); > +int mt8365_dai_set_priv(struct mtk_base_afe *afe, > + int id, > + int priv_size, > + const void *priv_data); > + Everything else looks good. After applying the proposed cleanups, Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com> Cheers, Angelo

11 months, 3 weeks

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig