linux-kernel - Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3

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]

Date:   Mon, 4 Sep 2017 08:50:27 +0200
From:   Maxime Ripard <maxime.ripard@...e-electrons.com>
To:     Brüns, Stefan <Stefan.Bruens@...h-aachen.de>
Cc:     linux-sunxi <linux-sunxi@...glegroups.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        "vinod.koul@...el.com" <vinod.koul@...el.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Chen-Yu Tsai <wens@...e.org>, Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3

On Fri, Sep 01, 2017 at 02:42:47PM +0000, Brüns, Stefan wrote:
> On Freitag, 1. September 2017 15:35:49 CEST Maxime Ripard wrote:
> > On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote:
> > > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote:
> > > > > +/* Between SoC generations, there are some significant differences:
> > > > > + * - A23 added a clock gate register
> > > > > + * - the H3 burst length field has a different offset
> > > > > + */
> > > > 
> > > > This is not the proper comment style.
> > > > 
> > > > > +enum dmac_variant {
> > > > > +	DMAC_VARIANT_A31,
> > > > > +	DMAC_VARIANT_A23,
> > > > > +	DMAC_VARIANT_H3,
> > > > > +};
> > > > > +
> > > > 
> > > > And this is redundant with what we already have in our structures.
> > > 
> > > Actually, its not. For H3, there are currently at least 3 register
> > > compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8
> > > channels. So if the current config structure is kept, we need 3 different
> > > compatible strings. Same for the A23, which is register compatible to
> > > e.g. A83t and V3s, but with different numbers of DMA channels.
> > > 
> > > So either you decorate the code with a cascade of
> > > 
> > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) {
> > > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...)
> > > {
> > > } else { /* A31 */
> > > }
> > > 
> > > in a number of places, or you do it just once.
> > 
> > That's not how you retrieve the structures. They are already
> > associated to the compatible, and you need to do a single lookup to
> > get them. So that's nowhere near what you're suggesting. You can have
> > a look at the of_match_device in the probe function.
> 
> Please have a look at the current implementation of how the clock autogating 
> in the probe function is done - it matches with the compatible string.

Yeah, and we should get rid of that as well.

> Of course we can replace this with a match between sdev->config and the 
> various sun6i_dma_config instances, but we would still have to do 3 matches 
> for the A23 register configuration (A23 || A83T || V3s) and 3 matches for the 
> H3 register configuration (H3 || R40 || A64). There are currently *7* 
> different configs (V3s, R40 and A64 taken into account), but only 3 different 
> register variants.
> 
> This is the same rationale as the "gate_needed" boolean property proposed by 
> Icenowy Zheng in the "Allwinner V3s DMA support" patch series. Obviously we 
> don't need a boolean, but a ternary option to cater for the gate_needed 
> differences - "NO_GATE", "A23_STYLE_GATE", "H3_STYLE_GATE".

Or we can just have an extra field in sun6i_dma_config that would set
the gate register offset? If it's non-zero, use whatever you have set
there, and then you just have to care about two cases, no matter how
many offsets we'll have in the wild.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)