linux-kernel - RE: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI device

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 for Android: free password hash cracker in your pocket

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

Message-ID: <DM6PR12MB2809F6C7D80B60556218D627DCB59@DM6PR12MB2809.namprd12.prod.outlook.com>
Date:   Thu, 23 Jun 2022 11:39:19 +0000
From:   "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@....com>
To:     Mark Brown <broonie@...nel.org>,
        Amit Kumar Mahapatra <amit.kumar-mahapatra@...inx.com>
CC:     "p.yadav@...com" <p.yadav@...com>,
        "miquel.raynal@...tlin.com" <miquel.raynal@...tlin.com>,
        "richard@....at" <richard@....at>,
        "vigneshr@...com" <vigneshr@...com>,
        "git@...inx.com" <git@...inx.com>,
        "michal.simek@...inx.com" <michal.simek@...inx.com>,
        "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "michael@...le.cc" <michael@...le.cc>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
        "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@....com>,
        "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@....com>,
        "git (AMD-Xilinx)" <git@....com>
Subject: RE: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI
 device

Hello Mark,

> -----Original Message-----
> From: linux-arm-kernel <linux-arm-kernel-bounces@...ts.infradead.org> On
> Behalf Of Mark Brown
> Sent: Thursday, June 9, 2022 5:24 PM
> To: Amit Kumar Mahapatra <amit.kumar-mahapatra@...inx.com>
> Cc: p.yadav@...com; miquel.raynal@...tlin.com; richard@....at;
> vigneshr@...com; git@...inx.com; michal.simek@...inx.com; linux-
> spi@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; michael@...le.cc; linux-mtd@...ts.infradead.org
> Subject: Re: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI
> device
> 
> On Mon, Jun 06, 2022 at 04:56:06PM +0530, Amit Kumar Mahapatra wrote:
> 
> > ---
> >  drivers/spi/spi-zynqmp-gqspi.c | 30 ++++++++++++++++++++++++++----
> >  drivers/spi/spi.c              | 10 +++++++---
> >  include/linux/spi/spi.h        | 10 +++++++++-
> >  3 files changed, 42 insertions(+), 8 deletions(-)
> 
> Please split the core and driver support into separate patches, they are
> separate things.

Ok, I will split the patches.
> 
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -2082,6 +2082,8 @@ static int of_spi_parse_dt(struct spi_controller
> > *ctlr, struct spi_device *spi,  {
> >  	u32 value;
> >  	int rc;
> > +	u32 cs[SPI_CS_CNT_MAX];
> > +	u8 idx;
> >
> >  	/* Mode (clock phase/polarity/etc.) */
> >  	if (of_property_read_bool(nc, "spi-cpha"))
> 
> This is changing the DT binding but doesn't have any updates to the binding
> document.  The binding code also doesn't validate that we don't have too
> many chip selects.

The following updates are done in the binding documents for adding multiple
CS support:
In jedec,spi-nor.yaml file " maxItems " of the "reg" DT property has been 
updated to accommodate two CS per SPI device.  
https://github.com/torvalds/linux/blob/de5c208d533a46a074eb46ea17f672cc005a7269/Documentation/devicetree/bindings/mtd/jedec%2Cspi-nor.yaml#L49

An example of a flash node with two CS has been added in spi-controller.yaml
https://github.com/torvalds/linux/blob/de5c208d533a46a074eb46ea17f672cc005a7269/Documentation/devicetree/bindings/spi/spi-controller.yaml#L141
> 
> > +	/* Bit mask of the chipselect(s) that the driver
> > +	 * need to use form the chipselect array.
> > +	 */
> > +	u8			cs_index_mask : 2;
> 
> Why make this a bitfield?

https://github.com/torvalds/linux/blob/de5c208d533a46a074eb46ea17f672cc005a7269/Documentation/devicetree/bindings/mtd/jedec%2Cspi-nor.yaml#L49

As per the DT bindings we are supporting max 2 chip selects per SPI device
that is the reason I had taken it as an bitfield of 2. But now I think that in 
future when the number of chip selects per device would increase i.e., 
more than 2, then we have to again increase the bitfield allocation for 
accommodating the increase in the number of chip selects per SPI device, 
So I think it's better to drop the bitfield for now and use cs_index_mask 
as an u8
> 
> I'm also not seeing anything here that checks that the driver supports
> multiple chip selects - it seems like something that's going to cause issues
> and we should probably have something to handle that situation.

In my approach the chip select member (chip_select) of the spi_device structure 
is changed to an array (chip_select[2]). This array is used to store the CS values 
coming from the "reg" DT property. 
In case of multiple chip selects  spi->chip_slect[0] will hold CS0 value & 
spi->chip_select[1] wil hold CS1 value.
In case of single chip select the spi->chip_select[0] will hold the chip select value.

As per the current implementation all the drivers fetch their chip select value form
spi->chip_select, but now the driver code needs to be modified to fetch the value
from spi->chip_select[0] instead and by this approach we do not need to check if the 
driver supports single or multiple CS.

Hope I addressed all your concerns and please let us know what you think.

Regards,
Amit