linux-kernel - RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

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: <SEZPR06MB52694CC2B6AE36B14F8D2303F2AD9@SEZPR06MB5269.apcprd06.prod.outlook.com>
Date:   Wed, 1 Mar 2023 03:40:54 +0000
From:   Ryan Chen <ryan_chen@...eedtech.com>
To:     Jeremy Kerr <jk@...econstruct.com.au>,
        Andrew Jeffery <andrew@...id.au>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Joel Stanley <joel@....id.au>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
 AST2600-i2cv2

Hello Jeremy,

> -----Original Message-----
> From: Jeremy Kerr <jk@...econstruct.com.au>
> Sent: Wednesday, March 1, 2023 11:24 AM
> To: Ryan Chen <ryan_chen@...eedtech.com>; Andrew Jeffery
> <andrew@...id.au>; Brendan Higgins <brendan.higgins@...ux.dev>; Benjamin
> Herrenschmidt <benh@...nel.crashing.org>; Joel Stanley <joel@....id.au>;
> Rob Herring <robh+dt@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Philipp Zabel <p.zabel@...gutronix.de>;
> linux-i2c@...r.kernel.org; openbmc@...ts.ozlabs.org;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
> linux-aspeed@...ts.ozlabs.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> Hi Ryan,
> 
> > Sorry, Do you mean add in description like following??
> >   aspeed,xfer-mode:
> >     description: |
> >       I2C bus transfer mode selection.
> >           ERRATA "I2C DMA fails when DRAM bus is busy and it can
> not
> > take DMA write data Immediately", only 1 i2c bus can be enable for DMA
> > mode.
> >       - "byte": I2C bus byte transfer mode.
> >       - "buffered": I2C bus buffer register transfer mode.
> >       - "dma": I2C bus dma transfer mode (default)
> 
> I would suggest putting some background about the transfer mode as a
> top-level description in the binding.
> 
> There has been a lot of discussion here on why the binding specifies the
> transfer mode; it would be useful (for future readers) to have a bit of context
> on what modes they should use.
> 
> Perhaps something like:
> 
>     description: |
>       [general binding description]
> 
>       ASPEED ast2600 platforms have a number of i2c controllers, and share
> a
>       single DMA engine between the set. DTSes can specify the mode of
> data
>       transfer to/from the device - either DMA or programmed I/O - but
>       hardware limitations may require a DTS to manually allocate which
>       controller can use DMA mode; the enable-dma property allows control
> of
>       this.
> 
>       In cases where one the hardware design results in a specific
>       controller handling a larger amount of data, a DTS would likely
>       allocate DMA mode for that one controller.
> 
> - adjusted for whatever property interface we settle on here, of course.
> 
It is more clear now, I will add in next patch.

> > > So, it sounds like:
> > >
> > >  - there's no point in using byte mode, as buffer mode provides
> > >    equivalent functionality with fewer drawbacks (ie, less interrupt
> > >    load)
> > >
> > >  - this just leaves the dma and buffer modes
> > >
> > >  - only one controller can use dma mode
> > >
> > > So: how about just a single boolean property to indicate "use DMA on
> > > this controller"? Something like aspeed,enable-dma? Or if DT binding
> > > experts can suggest something common that might be more suitable?
> >
> > If so, just leave enable-dma and only support for buffer mode and dma
> > mode, am I right?
> 
> Yes, from what you have said so far, I think just a single switch between DMA /
> not-DMA is all you need here (unless there is any time that byte mode is
> preferable?)

Yes, I also think so, but if I only for dma to be single Boolean property.
Should I remove all byte mode capability in driver?

Best Regards.
Ryan

> If there is already an existing DT convention for indicating/enabling DMA
> capability, I would suggest using that. Otherwise, just a boolean flag with a
> sensible name would seem to work fine. The DT experts probably have a good
> idea of what works best here :)
> 
> Cheers,
> 
> 
> Jeremy