Date:   Wed, 22 May 2019 15:12:53 +0000
From:   Liming Sun <lsun@...lanox.com>
To:     Arnd Bergmann <arnd@...db.de>, Mark Rutland <mark.rutland@....com>
CC:     Greg KH <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andy@...radead.org>,
        Darren Hart <dvhart@...radead.org>,
        Vadim Pasternak <vadimp@...lanox.com>,
        David Woods <dwoods@...lanox.com>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v5 1/2] platform/mellanox: Add bootctl driver for Mellanox
 BlueField Soc

Thanks Arnd for the comments! Please also see my response below.

> -----Original Message-----
> From: Arnd Bergmann <arnd@...db.de>
> Sent: Tuesday, May 21, 2019 3:59 AM
> To: Liming Sun <lsun@...lanox.com>
> Cc: Greg KH <gregkh@...uxfoundation.org>; Andy Shevchenko <andy@...radead.org>; Darren Hart <dvhart@...radead.org>; Vadim
> Pasternak <vadimp@...lanox.com>; David Woods <dwoods@...lanox.com>; platform-driver-x86@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCH v5 1/2] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc
> 
> On Mon, May 20, 2019 at 10:44 PM Liming Sun <lsun@...lanox.com> wrote:
> > > -----Original Message-----
> > > From: Greg KH <gregkh@...uxfoundation.org>
> > > Sent: Monday, May 20, 2019 3:12 PM
> > > To: Liming Sun <lsun@...lanox.com>
> > > Cc: Andy Shevchenko <andy@...radead.org>; Darren Hart <dvhart@...radead.org>; Vadim Pasternak <vadimp@...lanox.com>;
> David
> > > Woods <dwoods@...lanox.com>; platform-driver-x86@...r.kernel.org; linux-kernel@...r.kernel.org
> > > Subject: Re: [PATCH v5 1/2] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc
> > >
> > > On Mon, May 20, 2019 at 06:07:44PM +0000, Liming Sun wrote:
> > > > > > +static struct platform_driver mlxbf_bootctl_driver = {
> > > > > > +       .probe = mlxbf_bootctl_probe,
> > > > > > +       .driver = {
> > > > > > +               .name = "mlxbf-bootctl",
> > > > > > +               .groups = mlxbf_bootctl_groups,
> > > > > > +               .acpi_match_table = mlxbf_bootctl_acpi_ids,
> > > > >
> > > > > Why is an acpi driver a platform driver?  Isn't there a "real" acpi
> > > > > driver interface you should be tieing into instead?
> > > > >
> > > > > Only use a platform driver as an absolute last resort.  I don't think
> > > > > that is the case here.
> > > >
> > > > The driver is trying to configure boot-swapping and display secure state,
> > > > and is defined/initiated in ACPI table in UEFI. It seems a little hard to
> > > > categorize this driver to any existing subsystem. Any suggestion
> > > > where it might be a better fit (like drivers/misc, drivers/firmware, etc)?
> > > > Please correct me if I misunderstand the comments. Thanks!.
> > >
> > > The comment was asking why an acpi driver is a platform driver, but then
> > > I went and looked now at a bunch of acpi drivers, and they all are
> > > platform drivers :(
> > >
> > > Anyway, drivers/acpi/ seems like the best place for this file, right?
> >
> > My understanding is that the "drivers/acpi" is mainly for the acpi common code.
> > The vendor or platform specific drivers are spread in other various directories,
> > most of which are 'platform' drivers.
> 
> It depends on how closely you are following the acpi specification.
> If you just implement access to a standard ACPI feature, or you have
> added your interface to the ACPI specification, then the driver
> should work on any system that supports this feature.
> 
> > For this driver, we didn't find better sub-component for it, thus put it under
> > 'drivers/platform/mellanox' which is vendor specific driver by its name.
> 
> drivers/platform/mellanox/ would be a good place for drivers running on
> a host platform with a bluefield accelerator card as an add-on, but as I
> understand, this is a driver that actually just runs in Linux on the bluefield
> itself, so it should go in a different place.

Yes, the driver is actually running on the bluefield itself.
So looks like we'll need to find another location for it.

> 
> We use drivers/soc/ for things that are specific to one SoC, and that
> are typically used by other drivers, but that don't have (and should not
> have) a generic abstraction, which probably is not the case here either.

I did a 'grep' for 'module_platform_driver' and 'DEVICE_ATTR' under
drivers/soc, and found quite some under drivers/soc. This 'bootctl' driver
is SoC specific and tries to control the low-level boot partition (not
Linux related). Please also see my response to Mark's comments about
the SoC boot flow. Probably the 'drivers/soc' would be better fit for
this driver? Please advise.

> 
> What we do have in drivers/power/reset is a couple of drivers that
> set the "reboot reason", communicating that to the firmware for the
> next boot, using the reboot_mode_register() interface. I don't
> know too much about that interface, but maybe you can use that
> instead of adding another sysfs interface?

I checked the history of the 'reboot_mode'. Looks like it tries to
control how Linux boots, which is different than what this commit
does. This commit tries to control how ATF/UEFI boots, not how
Linux boots.

> 
> If you have a complex firmware on the system that you can talk
> to, there is also drivers/firmware/ as another option to put
> abstractions into.

I feel like 'drivers/soc' migh be better fit. We could certainly try
drivers/firmware/ and abstractions if you think 'drivers/soc' is not 
a good option.

> 
>          Arnd

Powered by blists - more mailing lists