Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

2531 discussions

Re: [PATCH net-next v8 3/3] net: ethernet: ti: am65-cpsw: Add minimal XDP support

by Jakub Kicinski

On Wed, 10 Apr 2024 16:02:00 +0200 Julien Panis wrote: > > You shouldn't build the skb upfront any more. Give the page to the HW, > > once HW sends you a completion - build the skbs. If build fails > > (allocation failure) just give the page back to HW. If it succeeds, > > however, you'll get a skb which is far more likely to be cache hot. > > Not sure I get this point. > > "Give the page to the HW" = Do you mean that I should dma_map_single() > a full page (|PAGE_SIZE|) in am65_cpsw_nuss_rx_push() ? Yes, I think so. I think that's what you effectively do now anyway, you just limit the len and wrap it in an skb. But am65_cpsw_nuss_rx_push() will effectively get that page back from skb->data and map it.

9 months

Re: [PATCH net-next v8 2/3] net: ethernet: ti: Add desc_infos member to struct k3_cppi_desc_pool

by Jakub Kicinski

On Wed, 10 Apr 2024 10:36:16 +0200 Julien Panis wrote: > Also, about mem alloc failures, shouldn't we free 'pool' on kstrdup_const() > error at the beginning of k3_cppi_desc_pool_create_name() ? > I mean, it's not visible in my patch but I now wonder if this was done > properly even before I modify the file. Yes, it uses managed memory (devm_*) but prueth (I didn't check other callers) calls it from .ndo_open. So while not technically a full leak we can accumulate infinite memory by repeatedly failing here :S

9 months

Re: [PATCH net-next v8 3/3] net: ethernet: ti: am65-cpsw: Add minimal XDP support

by Jakub Kicinski

On Mon, 08 Apr 2024 11:38:04 +0200 Julien Panis wrote: > +static struct sk_buff *am65_cpsw_alloc_skb(struct am65_cpsw_rx_chn *rx_chn, > + struct net_device *ndev, > + unsigned int len, > + int desc_idx, > + bool allow_direct) > +{ > + struct sk_buff *skb; > + struct page *page; > + > + page = page_pool_dev_alloc_pages(rx_chn->page_pool); > + if (unlikely(!page)) > + return NULL; > + > + len += AM65_CPSW_HEADROOM; > + > + skb = build_skb(page_address(page), len); You shouldn't build the skb upfront any more. Give the page to the HW, once HW sends you a completion - build the skbs. If build fails (allocation failure) just give the page back to HW. If it succeeds, however, you'll get a skb which is far more likely to be cache hot. > + if (unlikely(!skb)) { > + page_pool_put_full_page(rx_chn->page_pool, page, allow_direct); > + rx_chn->pages[desc_idx] = NULL; > + return NULL; > + }

9 months

Re: [PATCH net-next v8 2/3] net: ethernet: ti: Add desc_infos member to struct k3_cppi_desc_pool

by Jakub Kicinski

On Mon, 08 Apr 2024 11:38:03 +0200 Julien Panis wrote: > goto gen_pool_create_fail; > } > > + pool->desc_infos = kcalloc(pool->num_desc, > + sizeof(*pool->desc_infos), GFP_KERNEL); > + if (!pool->desc_infos) { > + ret = -ENOMEM; > + dev_err(pool->dev, > + "pool descriptor infos alloc failed %d\n", ret); Please don't add errors on mem alloc failures. They just bloat the kernel, there will be a rather large OOM splat in the logs if GFP_KERNEL allocation fails. > + kfree_const(pool_name); > + goto gen_pool_desc_infos_alloc_fail; > + } > + > pool->gen_pool->name = pool_name; If you add the new allocation after this line, I think you wouldn't have to free pool_name under the if () explicitly. -- pw-bot: cr

9 months

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

by Krzysztof Kozlowski

On 09/04/2024 15:42, Alexandre Mergnat wrote: > 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 | 98 +++++++++++++++++++++++++++-- > 1 file changed, 94 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts > index 50cbaefa1a99..eb0c5f076dd4 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts > +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts > @@ -1,9 +1,9 @@ > // SPDX-License-Identifier: GPL-2.0 > /* > - * Copyright (c) 2021-2022 BayLibre, SAS. > - * Authors: > - * Fabien Parent <fparent(a)baylibre.com> > - * Bernhard Rosenkränzer <bero(a)baylibre.com> > + * Copyright (c) 2024 BayLibre, SAS. What is happening with your copyrights? Why do you change existing ones? > + * Authors: Fabien Parent <fparent(a)baylibre.com> > + * Bernhard Rosenkränzer <bero(a)baylibre.com> > + * Alexandre Mergnat <amergnat(a)baylibre.com> > */ > > /dts-v1/; > @@ -86,6 +86,29 @@ 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>; > + status = "okay"; Where did you disable the node? > + }; > +}; > + > +&afe { > + mediatek,dmic-mode = <1>; > + status = "okay"; > }; > > &cpu0 { > @@ -174,6 +197,12 @@ &mmc1 { > status = "okay"; > }; > > +&mt6357_codec { > + mediatek,micbias0-microvolt = <1900000>; > + mediatek,micbias1-microvolt = <1700000>; > + mediatek,vaud28-supply = <&mt6357_vaud28_reg>; > +}; > + > &mt6357_pmic { > interrupts-extended = <&pio 145 IRQ_TYPE_LEVEL_HIGH>; > interrupt-controller; > @@ -181,6 +210,67 @@ &mt6357_pmic { > }; > > &pio { > + aud_default_pins: audiodefault-pins { > + 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>; You have broken indentation everywhere. Best regards, Krzysztof

9 months

Re: [PATCH v3 16/18] arm64: dts: mediatek: add mt6357 audio codec support

by Krzysztof Kozlowski

On 09/04/2024 15:42, Alexandre Mergnat wrote: > Add audio codec support of MT6357 PMIC. > Update the file header. Why? > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > arch/arm64/boot/dts/mediatek/mt6357.dtsi | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/mediatek/mt6357.dtsi b/arch/arm64/boot/dts/mediatek/mt6357.dtsi > index 3330a03c2f74..ade410851524 100644 > --- a/arch/arm64/boot/dts/mediatek/mt6357.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt6357.dtsi > @@ -1,7 +1,7 @@ > // SPDX-License-Identifier: (GPL-2.0 OR MIT) > /* > * Copyright (c) 2020 MediaTek Inc. > - * Copyright (c) 2023 BayLibre Inc. > + * Copyright (c) 2024 BayLibre Inc. That's not a reasonable change. The file was published on 2023, wasn't it? If this is not correct, please explain why/how and make it separate patch. > */ > > #include <dt-bindings/input/input.h> > @@ -10,6 +10,9 @@ &pwrap { > mt6357_pmic: pmic { > compatible = "mediatek,mt6357"; > > + mt6357_codec: codec { > + }; There is no single point of having empty nodes. NAK. Best regards, Krzysztof

9 months

Re: [PATCH v3 04/18] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 09/04/2024 15:42, Alexandre Mergnat wrote: > Add the audio codec sub-device. This sub-device is used to set required > and optional voltage properties between the codec and the board. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > index 37423c2e0fdf..7c6a4a587b5f 100644 > --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > @@ -37,6 +37,11 @@ properties: > "#interrupt-cells": > const: 2 > > + codec: > + type: object > + $ref: /schemas/sound/mt6357.yaml > + unevaluatedProperties: false Just put the properties here, without the codec node. Also, your example is now incomplete. Best regards, Krzysztof

9 months

Re: [PATCH v3 03/18] ASoC: dt-bindings: mt6357: Add audio codec document

by Krzysztof Kozlowski

On 09/04/2024 15:42, Alexandre Mergnat wrote: > Add MT8365 audio codec bindings to set required > and optional voltage properties between the codec and the board. > The properties are: > - phandle of the requiered power supply. typo > - Setup of microphone bias voltage. > - Setup of the speaker pin pull-down. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > .../devicetree/bindings/sound/mt6357.yaml | 54 ++++++++++++++++++++++ Filename using compatible syntax, so missing vendor prefix. > 1 file changed, 54 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/mt6357.yaml b/Documentation/devicetree/bindings/sound/mt6357.yaml > new file mode 100644 > index 000000000000..381cb71b959f > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/mt6357.yaml > @@ -0,0 +1,54 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/mt6357.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Mediatek MT6357 Codec > + > +maintainers: > + - Alexandre Mergnat <amergnat(a)baylibre.com> > + > +description: | Do not need '|' unless you need to preserve formatting. > + This is the required and optional voltage properties for this subdevice. > + The communication between MT6357 and SoC is through Mediatek PMIC wrapper. > + For more detail, please visit Mediatek PMIC wrapper documentation. > + Must be a child node of PMIC wrapper. Why? > + > +properties: > + Drop blank line > + mediatek,hp-pull-down: > + description: > + Earphone driver positive output stage short to > + the audio reference ground. > + type: boolean > + > + mediatek,micbias0-microvolt: > + description: Selects MIC Bias 0 output voltage. > + enum: [1700000, 1800000, 1900000, 2000000, > + 2100000, 2500000, 2600000, 2700000] > + default: 1700000 > + > + mediatek,micbias1-microvolt: > + description: Selects MIC Bias 1 output voltage. > + enum: [1700000, 1800000, 1900000, 2000000, > + 2100000, 2500000, 2600000, 2700000] > + default: 1700000 > + > + mediatek,vaud28-supply: > + description: 2.8 volt supply phandle for the audio codec Supplies go without vendor prefixes. > + > +required: > + - mediatek,vaud28-supply That's basically no-op schema. I do not understand what you are trying to achieve here. > + > +additionalProperties: false > + > +examples: > + - | > + codec { > + mediatek,micbias0-microvolt = <1900000>; > + mediatek,micbias1-microvolt = <1700000>; > + mediatek,vaud28-supply = <&mt6357_vaud28_reg>; Sorry, this does not work. Change voltage to 1111111 and check the results. Best regards, Krzysztof

9 months

Re: [PATCH v3 02/18] ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document

by Krzysztof Kozlowski

On 09/04/2024 15:42, Alexandre Mergnat wrote: > Add soundcard bindings for the MT8365 SoC with the MT6357 audio codec. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > +patternProperties: > + "^dai-link-[0-9]+$": > + type: object > + description: > + Container for dai-link level properties and CODEC sub-nodes. > + > + properties: > + codec: > + type: object > + description: Holds subnode which indicates codec dai. > + > + properties: > + sound-dai: > + maxItems: 1 > + description: phandle of the codec DAI > + > + additionalProperties: false > + > + link-name: > + description: > + This property corresponds to the name of the BE dai-link to which > + we are going to update parameters in this node. > + items: > + const: 2ND_I2S_BE What is the type of link-name? Why is it fixed? How can you have here multiple dai links if all of them must have the same name? Best regards, Krzysztof

9 months

Re: [PATCH v3 01/18] ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document

by Krzysztof Kozlowski

On 09/04/2024 15:41, Alexandre Mergnat wrote: > Add MT8365 audio front-end bindings > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > +properties: > + compatible: > + const: mediatek,mt8365-afe-pcm > + > + reg: > + maxItems: 1 > + > + "#sound-dai-cells": > + const: 0 > + > + clocks: > + items: > + - description: 26M clock > + - description: mux for audio clock > + - description: audio i2s0 mck > + - description: audio i2s1 mck > + - description: audio i2s2 mck > + - description: audio i2s3 mck > + - description: engen 1 clock > + - description: engen 2 clock > + - description: audio 1 clock > + - description: audio 2 clock > + - description: mux for i2s0 > + - description: mux for i2s1 > + - description: mux for i2s2 > + - description: mux for i2s3 > + > + clock-names: > + items: > + - const: top_clk26m_clk > + - const: top_audio_sel > + - const: audio_i2s0_m > + - const: audio_i2s1_m > + - const: audio_i2s2_m > + - const: audio_i2s3_m > + - const: engen1 > + - const: engen2 > + - const: aud1 > + - const: aud2 > + - const: i2s0_m_sel > + - const: i2s1_m_sel > + - const: i2s2_m_sel > + - const: i2s3_m_sel > + > + interrupts: > + maxItems: 1 > + > + power-domains: > + maxItems: 1 > + > + mediatek,dmic-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Indicates how many data pins are used to transmit two channels of PDM > + signal. 1 means two wires, 0 means one wire. Default value is 0. > + enum: > + - 0 # one wire > + - 1 # two wires > + > + mediatek,topckgen: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: The phandle of the mediatek topckgen controller Nothing improved, so again, so something which is not obvious. What is it used for? Why AFE needs topckgen for example? > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - interrupts > + - power-domains > + - mediatek,topckgen > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/mediatek,mt8365-clk.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/power/mediatek,mt8365-power.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + afe@11220000 { Did you implement the comment or decided to keep afe? BTW, whatever "consistency" you have in mind, it does not really matter that much for that example. And for sure do not add incorrect code intentionally just to fix it in next patch. Best regards, Krzysztof

9 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig