linux-kernel - Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

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: <CAAtXAHeO+qF=X-7L8uOCuFjCcVnUhEFkucYd3xfsFgVUp8JdcQ@mail.gmail.com>
Date:   Mon, 7 Nov 2016 13:41:28 -0800
From:   Moritz Fischer <moritz.fischer@...us.com>
To:     Joel Holdsworth <joel@...webreathe.org.uk>
Cc:     Alan Tull <atull@...nsource.altera.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Rob Herring <robh@...nel.org>,
        Devicetree List <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-spi@...r.kernel.org,
        Marek Vašut <marex@...x.de>,
        clifford@...fford.at
Subject: Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

Hi Joel,

On Mon, Nov 7, 2016 at 11:02 AM, Joel Holdsworth
<joel@...webreathe.org.uk> wrote:
> Hi Moritz - thanks for your comments.
>
>
>>> An example of such a device is the icoBoard; a RaspberryPI HAT which
>>> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
>>> Digilent-compatible PMOD modules. A PMOD module may contain a device
>>> with which the kernel communicates, via the FPGA.
>>
>>
>> Personally I find this a bit verbose, but that's just me.
>
>
> I could cut it down a bit if you want. I was just trying to make the case
> for why this *has* to be in the kernel rather than just being done from
> user-space.
>
>
>>> +       struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1,
>>> +               .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY};
>>
>>
>> Formatting looks odd, can you move the .cs_change to the next line?
>
>
> Marek made the same comment. I'll make the change.
>
>
>>> +static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr, u32
>>> flags)
>>> +{
>>> +       struct ice40_fpga_priv *priv = mgr->priv;
>>> +       struct spi_device *dev = priv->dev;
>>> +       const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,};
>>> +
>>> +       /* Check CDONE is asserted */
>>> +       if (!gpiod_get_value(priv->cdone)) {
>>> +               dev_err(&dev->dev,
>>> +                       "CDONE was not asserted after firmware
>>> transfer\n");
>>> +               return -EIO;
>>> +       }
>>> +
>>> +       /* Send of zero-padding to activate the firmware */
>>> +       return spi_write(dev, padding, sizeof(padding));
>>
>>
>> I'd move all of these into the write() callback.
>
>
> Is that correct? I was under the impression that write might be called
> multiple times to load firmware in multiple chunks.

Nah, you're right. Don't listen to me :)
>
>
>>> +       /* Enter reset */
>>> +       gpiod_set_value(priv->reset, 1);
>>
>>
>> I know Marek had suggested this, none of the other drivers behave like
>> that.
>> I'm not sure this is expected behavior for most people.
>
>
> For me it's not a big deal either way, but I think it's quite good for the
> driver to reset the device.
>
> When you remove most other drivers, you expect the driver to shut the device
> down, so isn't this just the same?
>
> ...but then the FPGA manager is a unique best with its own conventions, so
> perhaps it should behave differently.
>
> How can we get a consensus about this?

Fair enough. I don't mind either way all that much, just thought I'd
bring it up.

Thanks,

Moritz