linux-kernel - Re: [PATCH v2 2/4] spi: dw-mmio: add MSCC Ocelot support

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, 27 Jul 2018 16:07:54 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc:     Mark Brown <broonie@...nel.org>, James Hogan <jhogan@...nel.org>,
        Paul Burton <paul.burton@...s.com>,
        linux-spi <linux-spi@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux MIPS Mailing List <linux-mips@...ux-mips.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Allan Nielsen <allan.nielsen@...rosemi.com>
Subject: Re: [PATCH v2 2/4] spi: dw-mmio: add MSCC Ocelot support

On Fri, Jul 27, 2018 at 3:05 PM, Alexandre Belloni
<alexandre.belloni@...tlin.com> wrote:
> Because the SPI controller deasserts the chip select when the TX fifo is
> empty (which may happen in the middle of a transfer), the CS should be
> handled by linux. Unfortunately, some or all of the first four chip
> selects are not muxable as GPIOs, depending on the SoC.
>
> There is a way to bitbang those pins by using the SPI boot controller so
> use it to set the chip selects.
>
> At init time, it is also necessary to give control of the SPI interface to
> the Designware IP.

Thanks for an update! My comments below (most of them just about style).

First of all, can we use 'controller' over the 'master' in the code?

>  .../bindings/spi/snps,dw-apb-ssi.txt          |  5 +-

>  Required properties:
> -- compatible : "snps,dw-apb-ssi" or "mscc,<soc>-spi"
> -- reg : The register base for the controller. For "mscc,<soc>-spi", a second
> -  register set is required (named ICPU_CFG:SPI_MST)
> +- compatible : "snps,dw-apb-ssi"
> +- reg : The register base for the controller.

This hunk is odd.

>  struct dw_spi_mmio {
>         struct dw_spi  dws;
>         struct clk     *clk;
> +       void           *priv;
>  };

> +#define MSCC_SPI_MST_SW_MODE                   0x14
> +#define MSCC_SPI_MST_SW_MODE_SW_PIN_CTRL_MODE  BIT(13)
> +#define MSCC_SPI_MST_SW_MODE_SW_SPI_CS(x)      (x << 5)

+ blank line ?

> +struct dw_spi_mscc {
> +       struct regmap       *syscon;

> +       void __iomem        *spi_mst;

A nit: do we need to repeat "spi" here?

> +};

> +       if (!enable)
> +               dw_writel(dws, DW_SPI_SER, BIT(cs));

This sounds like

dw_set_cs(spi, enable);

> +       dwsmscc = devm_kzalloc(&pdev->dev, sizeof(struct dw_spi_mscc),
> +                              GFP_KERNEL);

sizeof(*dwsmcc)  and one line in the result?

> +       /* Deassert all CS */
> +       writel(0, dwsmscc->spi_mst + MSCC_SPI_MST_SW_MODE);

Hmm... Don't we need to call dw_set_cs() for all of them as well?
If yes, perhaps better to call custom set_cs() in a loop?

-- 
With Best Regards,
Andy Shevchenko