linux-kernel - Re: [PATCH 2/2] SPI: Add support for Zynq Quad SPI controller

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:	Fri, 4 Apr 2014 12:08:37 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Harini Katakam <harinikatakamlinux@...il.com>
Cc:	Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@...inx.com>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, linux-spi@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	Michal Simek <michal.simek@...inx.com>,
	Punnaiah Choudary <kpc528@...il.com>,
	punnaiah choudary kalluri <kalluripunnaiahchoudary@...il.com>,
	Punnaiah Choudary Kalluri <punnaia@...inx.com>
Subject: Re: [PATCH 2/2] SPI: Add support for Zynq Quad SPI controller

On Fri, Apr 04, 2014 at 08:59:47AM +0530, Harini Katakam wrote:
> On Fri, Apr 4, 2014 at 2:59 AM, Mark Brown <broonie@...nel.org> wrote:

> > Why would a transfer be being set up without a transfer being provided?

> The setup function calls this function before a transfer is initiated.
> In this case NULL is passed to setup_transfer (see below) and
> SPI is initialized with default clock configuration.
> This initialization is necessary because otherwise this clock config
> would be done
> only after SPI is enabled in prepare_hardware, which is wrong.
> (I'm checking for master->busy in setup to address your previous
> comment on SPI).

The requirement for setup() to work when other transfers are in progress
is clear and unambiguous, it really isn't acceptable to reconfigure
hardware in use by a runing transfer in setup().

> I explained the same in SPI v2 changes and this valid there too.

This is v2?

> >> +static int zynq_qspi_setup(struct spi_device *qspi)
> >> +{
> >> +     if (qspi->master->busy)
> >> +             return -EBUSY;
> >> +
> >> +     return zynq_qspi_setup_transfer(qspi, NULL);
> >> +}

> > No, this is broken - you have to support setup() while the hardware is
> > active.  Just remove this if there's nothing to do and set up on the
> > transfer.

> But where do you suggest this clock configuration be done?
> I've looked at the option of doing it in prepare_hardware but
> spi_device structure is not passed to it.

You can readily access the device data from the master - look at how
other drivers do this.

> >> +static int __maybe_unused zynq_qspi_suspend(struct device *_dev)
> >> +{
> >> +     struct platform_device *pdev = container_of(_dev,
> >> +                     struct platform_device, dev);
> >> +     struct spi_master *master = platform_get_drvdata(pdev);
> >> +
> >> +     spi_master_suspend(master);
> >> +
> >> +     zynq_unprepare_transfer_hardware(master);

> > Why are you unpreparing the hardware - the framework should be doing
> > that for you if the device is active, if it's not you've got an extra
> > clock disable here?

> I called unprepare_hardware  becuase it does the things necessary
> after master suspend - disable clock and controller.
> (I thought this was your suggestion for SPI?)

Why are these things required after the core has already idled the
device (using exactly the same function)?

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