linux-kernel - Re: [PATCHv2] drivers: spi: Add qspi flash 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]
Message-ID: <51D2ADD7.4020408@ti.com>
Date:	Tue, 2 Jul 2013 16:09:19 +0530
From:	Sourav Poddar <sourav.poddar@...com>
To:	<balbi@...com>
CC:	<broonie@...nel.org>, <spi-devel-general@...ts.sourceforge.net>,
	<grant.likely@...aro.org>, <rnayak@...com>,
	<linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv2] drivers: spi: Add qspi flash controller

On Tuesday 02 July 2013 04:01 PM, Felipe Balbi wrote:
> Hi,
>
> On Tue, Jul 02, 2013 at 03:53:49PM +0530, Sourav Poddar wrote:
>> On Tuesday 02 July 2013 03:46 PM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Tue, Jul 02, 2013 at 03:30:42PM +0530, Sourav Poddar wrote:
>>>>>> +static int dra7xxx_qspi_setup(struct spi_device *spi)
>>>>>> +{
>>>>>> +	struct dra7xxx_qspi	*qspi =
>>>>>> +			spi_master_get_devdata(spi->master);
>>>>>> +
>>>>>> +	int clk_div;
>>>>>> +
>>>>>> +	if (!qspi->spi_max_frequency)
>>>>>> +		clk_div = 0;
>>>>> won't this generate division by zero ?
>>>>>
>>>> Yes, Probably only an error should be thrown here. ?
>>>> since min clk_div should be kept at 1.
>>> right, if spi_max_frequency isn't passed, this is a broken DT binding.
>>> Bail out.
>>>
>>>>>> +	pm_runtime_get_sync(qspi->dev);
>>>>>> +
>>>>>> +	/* disable SCLK */
>>>>>> +	dra7xxx_writel(qspi, dra7xxx_readl(qspi, QSPI_SPI_CLOCK_CNTRL_REG)
>>>>>> +			&    ~QSPI_CLK_EN, QSPI_SPI_CLOCK_CNTRL_REG);
>>>>>> +
>>>>>> +	if (clk_div<    0) {
>>> btw, add a space between clk_div and<
>>>
>>>>>> +			dra7xxx_writel(qspi, *txbuf++, QSPI_SPI_DATA_REG);
>>>>>> +			dra7xxx_writel(qspi, qspi->dc, QSPI_SPI_DC_REG);
>>>>>> +			dra7xxx_writel(qspi, qspi->cmd | QSPI_WR_SNGL,
>>>>>> +					QSPI_SPI_CMD_REG);
>>>>>> +			status = dra7xxx_readl(qspi, QSPI_SPI_STATUS_REG);
>>>>>> +			timeout = QSPI_TIMEOUT;
>>>>>> +			while ((status&    QSPI_WC_BUSY) != QSPI_XFER_DONE) {
>>>>> do you really need to poll ? No IRQ available ?
>>>>>
>>>> There is an interrupt available, I will try using that.
>>> look at how i2c-omap.c synchronizes interrupt with the transfer_msg
>>> code. It just uses a wait_for_completion().
>>>
>> Ok.
>>>>>> +static int dra7xxx_qspi_start_transfer_one(struct spi_master *master,
>>>>>> +		struct spi_message *m)
>>>>>> +{
>>>>>> +	struct dra7xxx_qspi *qspi = spi_master_get_devdata(master);
>>>>>> +	struct spi_device *spi = m->spi;
>>>>>> +	struct spi_transfer *t;
>>>>>> +	int status = 0;
>>>>>> +	int flags = 0;
>>>>>> +
>>>>>> +	/* setup command reg */
>>>>>> +	qspi->cmd = 0;
>>>>>> +	qspi->cmd |= QSPI_WLEN(8);
>>>>>> +	qspi->cmd |= QSPI_EN_CS(0);
>>>>>> +	qspi->cmd |= 0xfff;
>>>> Since, we dont know the number of frame lenght that need to be
>>>> transferred and it comes from the spi framework, we keep the frame
>>>> lenght to maximum.
>>>> Then depending on the count value above in while loop, we terminate
>>>> our trasnfer.
>>> what ? seriously didn't get what you meant.
>>>
>> I mean, the lower 12 bits of cmd register is meant to be filled with
>> frame lenght.
>>
>> But the frame lenght is parsed when you iterate the list. So, what is
> which list ?
>
message list, from which we iterate through each transfers.
>> done here is that
>> the framelenght is kept to its maximum value.
> why ? That seems wrong. If you can get the actual frame length at some
> point, that's what you should use.
>
Ok.Then probably it makes sense to have frame count interrupt also to
signal the end of frame.
>> Then, to signal the the end of the frame, we use
>>
>> static int qspi_transfer_msg(struct dra7xxx_qspi *qspi, unsigned count,
>>                          const u8 *txbuf, u8 *rxbuf, bool flags)
>> {
>>          uint status;
>>          int timeout;
>>
>>          while (count--) {
>>                  if (txbuf) {
>>                          pr_debug("tx cmd %08x dc %08x data %02x\n",
>>                                  qspi->cmd | QSPI_WR_SNGL, qspi->dc,
>> *txbuf);
>>                          dra7xxx_writel(qspi, *txbuf++, QSPI_SPI_DATA_REG);
>>                          dra7xxx_writel(qspi, qspi->dc, QSPI_SPI_DC_REG);
>>                          dra7xxx_writel(qspi, qspi->cmd | QSPI_WR_SNGL,
>>                                          QSPI_SPI_CMD_REG);
>>                          status = dra7xxx_readl(qspi, QSPI_SPI_STATUS_REG);
>>                          timeout = QSPI_TIMEOUT;
>>                          while ((status&  QSPI_WC_BUSY) != QSPI_XFER_DONE) {
>>                                  if (--timeout<  0) {
>>                                          pr_debug("QSPI tx timed out\n");
>>                                          return
>>
>>       .............................
>>
>>                                     status, *(rxbuf-1));
>>                  }
>>          }
>>
>>          if (flags&  XFER_END)
>>                  dra7xxx_writel(qspi, qspi->cmd | QSPI_INVAL,
>> QSPI_SPI_CMD_REG);
>>
>> }
>> INVAL will terminate the current frame.
> nevermind that this value is "RESERVED" on the documentation. You should
> not rely on reserved features, they can go away at any point in time.
>
> That's probably there only for some IP debugging kinda thing.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/