linux-kernel - Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <IA1PR20MB4953EA589A0FF36DC6FCF0E8BB352@IA1PR20MB4953.namprd20.prod.outlook.com>
Date: Tue, 26 Mar 2024 19:41:55 +0800
From: Inochi Amaoto <inochiama@...look.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, 
	Inochi Amaoto <inochiama@...look.com>, Vinod Koul <vkoul@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Chen Wang <unicorn_wang@...look.com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>
Cc: Jisheng Zhang <jszhang@...nel.org>, Liu Gui <kenneth.liu@...hgo.com>, 
	Jingbao Qiu <qiujingbao.dlmu@...il.com>, dlan@...too.org, dmaengine@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for
 CV18XX/SG200X series SoC

On Tue, Mar 26, 2024 at 12:31:18PM +0100, Krzysztof Kozlowski wrote:
> On 26/03/2024 12:15, Inochi Amaoto wrote:
> > On Tue, Mar 26, 2024 at 09:53:09AM +0100, Krzysztof Kozlowski wrote:
> >> On 26/03/2024 08:35, Inochi Amaoto wrote:
> >>>>> +
> >>>>> +required:
> >>>>> +  - '#dma-cells'
> >>>>> +  - dma-masters
> >>>>> +
> >>>>
> >>>>
> >>>> I don't understand what happened here. Previously you had a child and I
> >>>> proposed to properly describe it with $ref.
> >>>>
> >>>> Now, all children are gone. Binding is supposed to be complete. Based on
> >>>> your cover letter, this is not complete, but why? What is missing and
> >>>> why it cannot be added?
> >>>>
> >>>
> >>> The binding of syscon is removed due to a usb phy subdevices, which needs 
> >>> sometime to figure out the actual property. This is why the syscon binding 
> >>> is removed.
> >>>
> >>> I think it is better to use the origianl syscon series to evolve after
> >>> the usb phy binding is submitted. The subdevices of syscon may need
> >>> much reverse engineering to know its parameters. So at least for now,
> >>> the syscon binding is hard to be complete.
> >>
> >> Some explanation why dma-router is gone would be useful, but fine.
> >>
> > 
> > OK, I will add some comments on the why it is gone.
> > 
> >>>
> >>>>
> >>>>> +additionalProperties: false
> >>>>> +
> >>>>> +examples:
> >>>>> +  - |
> >>>>> +    dma-router {
> >>>>> +      compatible = "sophgo,cv1800-dmamux";
> >>>>> +      #dma-cells = <2>;
> >>>>> +      dma-masters = <&dmac>;
> >>>>> +      dma-requests = <8>;
> >>>>> +    };
> >>>>> diff --git a/include/dt-bindings/dma/cv1800-dma.h b/include/dt-bindings/dma/cv1800-dma.h
> >>>>> new file mode 100644
> >>>>> index 000000000000..3ce9dac25259
> >>>>> --- /dev/null
> >>>>> +++ b/include/dt-bindings/dma/cv1800-dma.h
> >>>>
> >>>> Filename should match bindings filename.
> >>>>
> >>>
> >>> Thanks.
> >>>
> >>>>
> >>>> Anyway, the problem is that it is a dead header. I don't see it being
> >>>> used, so it is not a binding.
> >>>>
> >>>
> >>> This header is not used because the dmamux node is not defined at now.
> >>
> >> In the driver? The binding header is supposed to be used in the driver,
> >> otherwise it is not a binding.
> >>
> > 
> > The driver does use this file.
> 
> I checked and could not find. Please point me to specific parts of the code.
> 

In cv1800_dmamux_route_allocate.
>+	regmap_set_bits(dmamux->regmap,
>+			DMAMUX_CH_REG(chid),
>+			DMAMUX_CH_SET(chid, devid));
>+
>+	regmap_update_bits(dmamux->regmap, CV1800_SDMA_DMA_INT_MUX,
>+			   DMAMUX_INT_CH_MASK(chid, cpuid),
>+			   DMAMUX_INT_CH_BIT(chid, cpuid));

I think this is.

> > 
> >>> And considering the limitation of this dmamux, maybe only devices that 
> >>> require dma as a must can have the dma assigned. 
> >>> Due to the fact, I think it may be a long time to wait for this header
> >>> to be used as the binding header.
> >>
> >> I don't understand. You did not provide a single reason why this is a
> >> binding. Reason is: mapping IDs between DTS and driver. Where is this
> >> reason?
> >>
> > 
> > It seems like that I misunderstood something. This file provides one-one
> > mapping between the dma device id and cpuid, which is both used in the
> > dts and driver. For dts, it provides device id and cpu id mapping. And
> > for driver, it is used as the directive to tell how to write the mapping
> > register.
> 
> So where is it? I looked for DMA_TDM0_RX - nothing. Then DMA_I2C1_RX -
> nothing. Then any "DMA_" - also looks nothing.
> 

It is just the value writed, so I say it is just a one-one mapping.
Maybe I misunderstand the binding meaning? Is the binding a mapping 
between the dts and something defind in the driver (not the real 
device)?

Regards,
Inochi.