Message-ID: <CAK8P3a1bbpmD0wJkhkjqW9YttBpMmdn8Z5oTLm0cr-0gjyU2zA@mail.gmail.com>
Date:   Tue, 30 Jun 2020 22:57:59 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Daniel Gutson <daniel@...ypsium.com>
Cc:     Derek Kiernan <derek.kiernan@...inx.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Richard Hughes <hughsient@...il.com>,
        Alex Bazhaniuk <alex@...ypsium.com>
Subject: Re: [PATCH] SPI LPC information kernel module

On Tue, Jun 30, 2020 at 9:08 PM Daniel Gutson <daniel@...ypsium.com> wrote:
> On Tue, Jun 30, 2020 at 5:58 AM Arnd Bergmann <arnd@...db.de> wrote:
>> On Tue, Jun 30, 2020 at 12:59 AM Daniel Gutson <daniel.gutson@...ypsium.com> wrote:
>> The description should start with a little more background for those that
>> don't already know what this driver is for. What is a "system SPI chip"?
>> Is this an SPI host connected over LPC, or an LPC bus connected over
>> SPI? Is there a particular spec that this follows?
>
>
> "System SPI chip" refers to the main system firmware, which is accessed through an SPI interface.
> AFAIK there's no spec for this, though it's a de-facto standard applying to the earliest days of legacy BIOS.
> This driver provides visibility to the system firmware configuration access.

Oh, so it isn't even the SPI controller, just a flash chip hanging off
a random SPI controller, or possibly something else. I suppose it could
be any MFD device, or possibly something different.

>> > +int spi_read_sbase(enum PCH_Arch pch_arch __maybe_unused,
>> > +                  enum CPU_Arch cpu_arch, struct spi_sbase *reg)
>> > +{
>> > +       int ret = 0;
>> > +
>> > +       reg->register_arch.source = RegSource_CPU;
>> > +       reg->register_arch.cpu_arch = cpu_arch;
>> > +
>> > +       switch (cpu_arch) {
>> > +       case cpu_avn:
>> > +       case cpu_byt:
>> > +               ret = read_sbase_atom_avn_byt(&reg->cpu_byt);
>> > +               break;
>> > +       default:
>> > +               ret = -EIO;
>> > +       }
>> > +       return ret;
>> > +}
>> > +EXPORT_SYMBOL_GPL(spi_read_sbase);
>>
>> This function seems to be Intel Atom specific but has a rather generic
>> name for an exported symbol.
>
>
> It 'currently' is atom specific, but as I mentioned in the mail, the idea was
> to submit an initial patch with some attributes. The more platforms and fields
> I need, this switch will get more and more populated. This is about reading the
>  SBASE register, which in my current state, is only used for Atom. I can submit
> a driver with more fields where these switches get more populated, but I
> thought it would be easier starting with three fields only.
>
> Anyways I'll give up on the exports for now until a kernel module shows up needing the reading layer.

The problem is more the namespace: as the driver has almost nothing
to do with spi, this is not what the function should be named. The other
problem is that the function should not really exist in the first place,
as no driver has any business reading the register base of a random
other device that already has a driver.

>> > +enum CPU_Arch {
>> > +       cpu_none,
>> > +       cpu_bdw,
>> > +       cpu_bdx,
>> > +       cpu_hsw,
>> > +       cpu_hsx,
>> > +       cpu_ivt,
>> > +       cpu_jkt,
>>
>> You might want to avoid having a central instance listing all possible
>> CPUs. Instead, structure it so that the common parts know nothing
>> about a specific implementation and each one can be kept in a separate
>> file for easier extension.
>
>
> CPUs differ in register structure, not only extending, but having different fields too.
> All this information comes from datasheets that don't tell "this is CPU XX with these extensions",
> so sometimes it is easier to copy what the datasheet says.
> I didn't understand what's wrong with having a central enumeration of all available CPUs?

> This is a way of polymorphism, where I can just do a single
> read_[register](struct register* register) and then "browse" inside the register definition by knowing the architecture.
> Could you please explain your alternative with more detail? The user wants its architecture's definition, and will not
...
>> The driver that owns the MMIO region normally maps it once during its
>> probe() function and then keeps a pointer around
>
>
> This case is different. Please note that this is not a "device driver",
> that's why it doesn't own the MMIO region. This driver gathers information
> from the SPI controller only in the current state (it will be expanded).
> That's why it maps and then unmaps in the same operation.

To answer all the above points: This needs to be in a driver, and
in case of pch, that driver is drivers/mtd/spi-nor/intel-spi.c.

This driver already has access to the registers you need, and the
information you pass corresponds to the devices that this driver
manages, which in turn is where user space would logically search
for it. You can read the registers in the intel_spi_probe() function
and then add attributes to the mtd device in sysfs.

I think a good way to handle this in a generic way would be to add
members to the mtd_info structure and then have the attributes
created by the mtd core code for any device that initializes those
struct members, along with the existing 'type', 'size', 'flags',  etc
attributes.

     Arnd

Powered by blists - more mailing lists