Re: [PATCH v4] drivers/misc: Add Aspeed LPC control driver

[Benjamin Herrenschmidt]

On Wed, 2017-02-08 at 02:06 +0200, Andy Shevchenko wrote:
> > On Wed, Feb 8, 2017 at 1:42 AM, Cyril Bur <cyrilbur@xxxxxxxxx> wrote:
> > In order to manage server systems, there is typically another processor
> > known as a BMC (Baseboard Management Controller) which is responsible
> > for powering the server and other various elements, sometimes fans,
> > often the system flash.
> > 
> > The Aspeed BMC family which is what is used on OpenPOWER machines and a
> > number of x86 as well is typically connected to the host via an LPC
> > (Low Pin Count) bus (among others).
> 
> Perhaps I missed the discussion on previous versions, but here my
> question. If it's available on x86, how you access it there? This
> driver seems too OF oriented which is not a case on most of x86
> platforms.

This is the BMC (ie salve) side of the LPC bus which is an Aspeed
ARM chip whose kernel is device-tree based.

What happens on the "host" side (x86, powerpc, arm64, ...) is not
directly relevant here.

 .../...

> > It is important to note that due to the way the Aspeed chip lets the
> > kernel configure the mapping between host LPC addresses and BMC ram
> > addresses the offset within the window must be a multiple of size.
> > Not doing so will fragment the accessible space rather than simply
> > moving 'zero' upwards. This is caused by the nature of HICR8 being a
> > mask and the way host LPC addresses are translated.
> > Âdrivers/misc/KconfigÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂÂ8 ++
> > Âdrivers/misc/MakefileÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂÂ1 +
> > Âdrivers/misc/aspeed-lpc-ctrl.cÂÂÂÂÂÂÂ| 263 +++++++++++++++++++++++++++++++++++
> > Âinclude/uapi/linux/aspeed-lpc-ctrl.h |ÂÂ36 +++++
> 
> Since it's UAPI can it be more generic in case there will be other LPC
> implementations / devices?

Not really. As I wrote above, this isn't exposing an LPC bus the way
you seem to think of it, it's about the *BMC* side of the LPC bus (the
slave side) where some fairly chip/board configuration is needed to
do things like control the mapping of part of the LPC FW space to the
SPI flash or control which LPC devices are made visible to the host etc...

This is meant to be used by OpenBMC to configure the LPC bus properly
on the BMC side so the host can boot.

> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO)ÂÂÂÂÂÂÂÂÂÂÂÂ+= echo/
> > Âobj-$(CONFIG_VEXPRESS_SYSCFG)ÂÂ+= vexpress-syscfg.o
> > Âobj-$(CONFIG_CXL_BASE)ÂÂÂÂÂÂÂÂÂ+= cxl/
> > Âobj-$(CONFIG_PANEL)ÂÂÂÂÂÂÂÂÂÂÂÂÂ+= panel.o
> > +obj-$(CONFIG_ASPEED_LPC_CTRL)ÂÂ+= aspeed-lpc-ctrl.o
> 
> Does it suit best under misc?

Yes. It's basically a chardev giving access to a few very specific
registers. I've already explained all of this in series of emails...

It could *almost* be UIO except for the fact that one thing that
needs to be configured is the mapping between the LPC bus FW space
and some region of the SoC address space, either the SPI controller
or some reserved memory used for flash operations, and there is
benefit in doing so in the kernel for security purposes as doing
so incorrectly would expose the BMC internal physical address space
to the host.

> > +static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned long param)
> > +{
> > +ÂÂÂÂÂÂÂlong rc;
> > +ÂÂÂÂÂÂÂstruct aspeed_lpc_ctrl_mapping map;
> > +ÂÂÂÂÂÂÂstruct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> > +ÂÂÂÂÂÂÂvoid __user *p = (void __user *)param;
> > +ÂÂÂÂÂÂÂu32 addr;
> > +ÂÂÂÂÂÂÂu32 size;
> 
> Reversed tree?

Ugh ? One of those new ridiculous coding style rules ?

> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrc = regmap_write(lpc_ctrl->regmap, HICR7,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(addr | (map.addr >> 16)));
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (rc)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rc;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn regmap_write(lpc_ctrl->regmap, HICR8,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(~(map.size - 1)) | ((map.size >> 16) - 1));
> 
> Magic stuff above and here. Perhaps some helpers?

A helper might be warranted but it's not *that* magic ... pretty
obvious what that does ;-)

> > +ÂÂÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂÂÂreturn -EINVAL;
> > +}
> > +
> > +static const struct file_operations aspeed_lpc_ctrl_fops = {
> > +ÂÂÂÂÂÂÂ.ownerÂÂÂÂÂÂÂÂÂÂ= THIS_MODULE,
> 
> Do we still need this?
> > +ÂÂÂÂÂÂÂ.mmapÂÂÂÂÂÂÂÂÂÂÂ= aspeed_lpc_ctrl_mmap,
> > +ÂÂÂÂÂÂÂ.unlocked_ioctl = aspeed_lpc_ctrl_ioctl,
> > +};
> > +static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
> > +{
> > +ÂÂÂÂÂÂÂstruct aspeed_lpc_ctrl *lpc_ctrl;
> > +ÂÂÂÂÂÂÂstruct device *dev;
> > +ÂÂÂÂÂÂÂstruct device_node *node;
> > +ÂÂÂÂÂÂÂstruct resource resm;
> > +ÂÂÂÂÂÂÂint rc;
> > +
> > +ÂÂÂÂÂÂÂdev = &pdev->dev;
> 
> You can do this in the definition block.
> 
> 
> > +ÂÂÂÂÂÂÂnode = of_parse_phandle(dev->of_node, "flash", 0);
> > +ÂÂÂÂÂÂÂif (!node) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_err(dev, "Didn't find host pnor flash node\n");
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENODEV;
> > +ÂÂÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂÂÂrc = of_property_read_u32_index(node, "reg", 3,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&lpc_ctrl->pnor_size);
> > +ÂÂÂÂÂÂÂif (rc)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rc;
> > +ÂÂÂÂÂÂÂrc = of_property_read_u32_index(node, "reg", 2,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&lpc_ctrl->pnor_base);
> > +ÂÂÂÂÂÂÂif (rc)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rc;
> 
> Isn't something you may read at once?

Actually it should be parsed using of_address_to_resource indeed
in case there's any translation in the DT.

> > +ÂÂÂÂÂÂÂlpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +ÂÂÂÂÂÂÂlpc_ctrl->miscdev.name = DEVICE_NAME;
> > +ÂÂÂÂÂÂÂlpc_ctrl->miscdev.fops = &aspeed_lpc_ctrl_fops;
> > +ÂÂÂÂÂÂÂlpc_ctrl->miscdev.parent = dev;
> > +ÂÂÂÂÂÂÂrc = misc_register(&lpc_ctrl->miscdev);
> 
> No devm_ variant?

No real need here, is there ?

> > +ÂÂÂÂÂÂÂif (rc)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_err(dev, "Unable to register device\n");
> > +ÂÂÂÂÂÂÂelse
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> 
> %pa
> %pa
> 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlpc_ctrl->mem_base, lpc_ctrl->mem_size);
> > +
> > +ÂÂÂÂÂÂÂreturn rc;
> > +}
> > +struct aspeed_lpc_ctrl_mapping {
> > +ÂÂÂÂÂÂÂ__u8ÂÂÂÂwindow_type;
> > +ÂÂÂÂÂÂÂ__u8ÂÂÂÂwindow_id;
> > +ÂÂÂÂÂÂÂ__u16ÂÂÂflags;
> > +ÂÂÂÂÂÂÂ__u32ÂÂÂaddr;
> > +ÂÂÂÂÂÂÂ__u32ÂÂÂoffset;
> > +ÂÂÂÂÂÂÂ__u32ÂÂÂsize;
> > +};
> 
> It will suck. So, no room for manoeuvre?

What do you mean ?

Cheers,
Ben.