linux-kernel - Re: [PATCH v3 2/2] spi: Introduce new driver for Qualcomm QuadSPI 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, 28 Sep 2018 11:19:51 -0700
From:   Ryan Case <ryandcase@...omium.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Mark Brown <broonie@...nel.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        linux-arm-msm@...r.kernel.org,
        Doug Anderson <dianders@...omium.org>,
        Trent Piepho <tpiepho@...inj.com>,
        Boris Brezillon <boris.brezillon@...tlin.com>,
        Girish Mahadevan <girishm@...eaurora.org>,
        linux-kernel@...r.kernel.org, linux-spi@...r.kernel.org
Subject: Re: [PATCH v3 2/2] spi: Introduce new driver for Qualcomm QuadSPI controller

On Wed, Sep 26, 2018 at 11:43 PM Stephen Boyd <swboyd@...omium.org> wrote:
> Quoting Ryan Case (2018-09-26 13:52:04)
> > From: Girish Mahadevan <girishm@...eaurora.org>
> > +#include <asm/unaligned.h>
> > +
> > +#define AHB_MIN_HZ             9600000UL
>
> Is this used?

Nope. Do you want all currently unused defines removed or specifically this
one? I saw precedent in other drivers for defining registers/flags/values of
supported but unused functionality so I left these (big endian, DDR, ...).


> > +       speed_hz = slv->max_speed_hz;
> > +       if (xfer->speed_hz)
> > +               speed_hz = xfer->speed_hz;
> > +
> > +       ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4);
>
> Why 4? Is that related to the number of wires?

In normal operation the core clock should be running at 4x the rate of the
transfer clock regardless of number of wires used.

> > +                       put_unaligned(rd_fifo, word_buf++);
> > +               }
> > +               ctrl->xfer.rx_buf = word_buf;
> > +       }
> > +
> > +       if (bytes_to_read) {
> > +               byte_buf = ctrl->xfer.rx_buf;
>
> Does this need to move forward by words_to_read bytes so that the left
> over bytes are tacked onto the end? Or this should be an else if
> statement?

When the words block completes it updates the rx_buf location so it is already
at the correct offset for bytes.

> > +
> > +       master->max_speed_hz = 300000000;
> > +       master->num_chipselect = QSPI_NUM_CS;
> > +       master->bus_num = pdev->id;
>
> Can this come from DT aliases? I've never thought about qspi and
> "regular" spi being in the same spi bus numbering system, but I suppose
> that will happen now and we need to make sure that qspi numbers start
> after the regular ones?

I'm not sure. Can look into it.

>
> > +       master->dev.of_node = pdev->dev.of_node;
> > +       master->mode_bits = SPI_MODE_0 |
> > +                           SPI_TX_DUAL | SPI_RX_DUAL |
> > +                           SPI_TX_QUAD | SPI_RX_QUAD;
> > +       master->flags = SPI_MASTER_HALF_DUPLEX;
> > +       master->prepare_message = qcom_qspi_prepare_message;
> > +       master->transfer_one = qcom_qspi_transfer_one;