Re: [PATCH v7 2/2] drivers/misc: Add Aspeed P2A control driver
On Wed, Mar 27, 2019 at 11:52 PM Greg KH wrote:
>
> On Wed, Mar 27, 2019 at 12:01:50PM -0700, Patrick Venture wrote:
> > On Wed, Mar 27, 2019 at 11:54 AM Greg KH wrote:
> > >
> > > On Wed, Mar 27, 2019 at 11:44:36AM -0700, Patrick Venture wrote:
> > > > On Wed, Mar 27, 2019 at 11:28 AM Greg KH
> > > > wrote:
> > > > >
> > > > > On Tue, Mar 12, 2019 at 09:31:01AM -0700, Patrick Venture wrote:
> > > > > > + phys_addr_t mem_base;
> > > > >
> > > > > Is this really a 32bit value?
> > > >
> > > > It's going to be a 32-bit value if this is in the dts for one of the
> > > > correspondingly supported aspeed models.
> > > >
> > > > >
> > > > > Your ioctl thinks it is:
> > > > >
> > > > > > +struct aspeed_p2a_ctrl_mapping {
> > > > > > + __u32 addr;
> > > > >
> > > > > Does this driver not work on a 64bit kernel?
> > > >
> > > > This driver is aimed at only 32-bit hardware (ast2400/2500). I
> > > > modeled the approach after the aspeed-lpc-ctrl driver as it's
> > > > providing similar functionality.
> > > >
> > > > >
> > > > > > + __u32 length;
> > > > > > + __u32 flags;
> > > > > > +};
> > > > >
> > > > > addr really should be __u32 here so you don't have to mess with 32/64
> > > > > bit user/kernel issues, right?
> > > >
> > > > Add is __u32 there. Are you suggesting it shouldn't be?
> > >
> > > Ugh, yes, sorry, I meant to say "__u64".
> > >
> > > If you all insist that this is all that is ever going to be needed, ok,
> > > but I reserve the right to complain in 4 years when this needs to be
> > > changed :)
> >
> > In the event the ast2600 comes out and is 64-bit -- I can't imagine
> > that's likely to happen. I can take solace that this won't be the
> > only thing that needs retrofitting. But it wouldn't kill me to just
> > make the change. I'll just have to tweak it to return failure in the
> > event the address provided isn't found in any region...
> >
> > Is that all that needs to change for 64-bit addressing support - given
> > your read of the driver?
>
> That's all that I noticed at first glance, yes.
Thanks, that's addressed in v8.
> I do dislike having
> custom user/kernel apis for random chips like this, but I don't know of
> a way to have a generic api for them at the moment as I really do not
> know what these chips do :(
>
> One would think that the firmware api would work for you, but given the
> complexity here, it does not seem that it would match up.
Yeah, this driver is basically just allowing control over a bridge and
allows for a convenient common use-case for such bridges. I don't
have enough exposure to see if there's some commonality for
configuration and control of bridges across different chips, and I
would imagine they're very distinct.
>
> thanks,
>
> greg k-h
Thanks
Re: [PATCH v7 2/2] drivers/misc: Add Aspeed P2A control driver
On Wed, Mar 27, 2019 at 12:01:50PM -0700, Patrick Venture wrote:
> On Wed, Mar 27, 2019 at 11:54 AM Greg KH wrote:
> >
> > On Wed, Mar 27, 2019 at 11:44:36AM -0700, Patrick Venture wrote:
> > > On Wed, Mar 27, 2019 at 11:28 AM Greg KH
> > > wrote:
> > > >
> > > > On Tue, Mar 12, 2019 at 09:31:01AM -0700, Patrick Venture wrote:
> > > > > + phys_addr_t mem_base;
> > > >
> > > > Is this really a 32bit value?
> > >
> > > It's going to be a 32-bit value if this is in the dts for one of the
> > > correspondingly supported aspeed models.
> > >
> > > >
> > > > Your ioctl thinks it is:
> > > >
> > > > > +struct aspeed_p2a_ctrl_mapping {
> > > > > + __u32 addr;
> > > >
> > > > Does this driver not work on a 64bit kernel?
> > >
> > > This driver is aimed at only 32-bit hardware (ast2400/2500). I
> > > modeled the approach after the aspeed-lpc-ctrl driver as it's
> > > providing similar functionality.
> > >
> > > >
> > > > > + __u32 length;
> > > > > + __u32 flags;
> > > > > +};
> > > >
> > > > addr really should be __u32 here so you don't have to mess with 32/64
> > > > bit user/kernel issues, right?
> > >
> > > Add is __u32 there. Are you suggesting it shouldn't be?
> >
> > Ugh, yes, sorry, I meant to say "__u64".
> >
> > If you all insist that this is all that is ever going to be needed, ok,
> > but I reserve the right to complain in 4 years when this needs to be
> > changed :)
>
> In the event the ast2600 comes out and is 64-bit -- I can't imagine
> that's likely to happen. I can take solace that this won't be the
> only thing that needs retrofitting. But it wouldn't kill me to just
> make the change. I'll just have to tweak it to return failure in the
> event the address provided isn't found in any region...
>
> Is that all that needs to change for 64-bit addressing support - given
> your read of the driver?
That's all that I noticed at first glance, yes. I do dislike having
custom user/kernel apis for random chips like this, but I don't know of
a way to have a generic api for them at the moment as I really do not
know what these chips do :(
One would think that the firmware api would work for you, but given the
complexity here, it does not seem that it would match up.
thanks,
greg k-h
Re: [PATCH v7 2/2] drivers/misc: Add Aspeed P2A control driver
On Wed, Mar 27, 2019 at 12:01 PM Patrick Venture wrote:
>
> On Wed, Mar 27, 2019 at 11:54 AM Greg KH wrote:
> >
> > On Wed, Mar 27, 2019 at 11:44:36AM -0700, Patrick Venture wrote:
> > > On Wed, Mar 27, 2019 at 11:28 AM Greg KH
> > > wrote:
> > > >
> > > > On Tue, Mar 12, 2019 at 09:31:01AM -0700, Patrick Venture wrote:
> > > > > + phys_addr_t mem_base;
> > > >
> > > > Is this really a 32bit value?
> > >
> > > It's going to be a 32-bit value if this is in the dts for one of the
> > > correspondingly supported aspeed models.
> > >
> > > >
> > > > Your ioctl thinks it is:
> > > >
> > > > > +struct aspeed_p2a_ctrl_mapping {
> > > > > + __u32 addr;
> > > >
> > > > Does this driver not work on a 64bit kernel?
> > >
> > > This driver is aimed at only 32-bit hardware (ast2400/2500). I
> > > modeled the approach after the aspeed-lpc-ctrl driver as it's
> > > providing similar functionality.
> > >
> > > >
> > > > > + __u32 length;
> > > > > + __u32 flags;
> > > > > +};
> > > >
> > > > addr really should be __u32 here so you don't have to mess with 32/64
> > > > bit user/kernel issues, right?
> > >
> > > Add is __u32 there. Are you suggesting it shouldn't be?
> >
> > Ugh, yes, sorry, I meant to say "__u64".
> >
> > If you all insist that this is all that is ever going to be needed, ok,
> > but I reserve the right to complain in 4 years when this needs to be
> > changed :)
>
> In the event the ast2600 comes out and is 64-bit -- I can't imagine
> that's likely to happen. I can take solace that this won't be the
> only thing that needs retrofitting. But it wouldn't kill me to just
> make the change. I'll just have to tweak it to return failure in the
> event the address provided isn't found in any region...
>
> Is that all that needs to change for 64-bit addressing support - given
> your read of the driver?
I should have v8 for review shortly.
>
> >
> > thanks,
> >
> > greg k-h
Re: [PATCH v7 2/2] drivers/misc: Add Aspeed P2A control driver
On Wed, Mar 27, 2019 at 11:54 AM Greg KH wrote:
>
> On Wed, Mar 27, 2019 at 11:44:36AM -0700, Patrick Venture wrote:
> > On Wed, Mar 27, 2019 at 11:28 AM Greg KH wrote:
> > >
> > > On Tue, Mar 12, 2019 at 09:31:01AM -0700, Patrick Venture wrote:
> > > > + phys_addr_t mem_base;
> > >
> > > Is this really a 32bit value?
> >
> > It's going to be a 32-bit value if this is in the dts for one of the
> > correspondingly supported aspeed models.
> >
> > >
> > > Your ioctl thinks it is:
> > >
> > > > +struct aspeed_p2a_ctrl_mapping {
> > > > + __u32 addr;
> > >
> > > Does this driver not work on a 64bit kernel?
> >
> > This driver is aimed at only 32-bit hardware (ast2400/2500). I
> > modeled the approach after the aspeed-lpc-ctrl driver as it's
> > providing similar functionality.
> >
> > >
> > > > + __u32 length;
> > > > + __u32 flags;
> > > > +};
> > >
> > > addr really should be __u32 here so you don't have to mess with 32/64
> > > bit user/kernel issues, right?
> >
> > Add is __u32 there. Are you suggesting it shouldn't be?
>
> Ugh, yes, sorry, I meant to say "__u64".
>
> If you all insist that this is all that is ever going to be needed, ok,
> but I reserve the right to complain in 4 years when this needs to be
> changed :)
In the event the ast2600 comes out and is 64-bit -- I can't imagine
that's likely to happen. I can take solace that this won't be the
only thing that needs retrofitting. But it wouldn't kill me to just
make the change. I'll just have to tweak it to return failure in the
event the address provided isn't found in any region...
Is that all that needs to change for 64-bit addressing support - given
your read of the driver?
>
> thanks,
>
> greg k-h
Re: [PATCH v7 2/2] drivers/misc: Add Aspeed P2A control driver
On Wed, Mar 27, 2019 at 11:44:36AM -0700, Patrick Venture wrote:
> On Wed, Mar 27, 2019 at 11:28 AM Greg KH wrote:
> >
> > On Tue, Mar 12, 2019 at 09:31:01AM -0700, Patrick Venture wrote:
> > > + phys_addr_t mem_base;
> >
> > Is this really a 32bit value?
>
> It's going to be a 32-bit value if this is in the dts for one of the
> correspondingly supported aspeed models.
>
> >
> > Your ioctl thinks it is:
> >
> > > +struct aspeed_p2a_ctrl_mapping {
> > > + __u32 addr;
> >
> > Does this driver not work on a 64bit kernel?
>
> This driver is aimed at only 32-bit hardware (ast2400/2500). I
> modeled the approach after the aspeed-lpc-ctrl driver as it's
> providing similar functionality.
>
> >
> > > + __u32 length;
> > > + __u32 flags;
> > > +};
> >
> > addr really should be __u32 here so you don't have to mess with 32/64
> > bit user/kernel issues, right?
>
> Add is __u32 there. Are you suggesting it shouldn't be?
Ugh, yes, sorry, I meant to say "__u64".
If you all insist that this is all that is ever going to be needed, ok,
but I reserve the right to complain in 4 years when this needs to be
changed :)
thanks,
greg k-h
Re: [PATCH v7 2/2] drivers/misc: Add Aspeed P2A control driver
On Wed, Mar 27, 2019 at 11:28 AM Greg KH wrote:
>
> On Tue, Mar 12, 2019 at 09:31:01AM -0700, Patrick Venture wrote:
> > + phys_addr_t mem_base;
>
> Is this really a 32bit value?
It's going to be a 32-bit value if this is in the dts for one of the
correspondingly supported aspeed models.
>
> Your ioctl thinks it is:
>
> > +struct aspeed_p2a_ctrl_mapping {
> > + __u32 addr;
>
> Does this driver not work on a 64bit kernel?
This driver is aimed at only 32-bit hardware (ast2400/2500). I
modeled the approach after the aspeed-lpc-ctrl driver as it's
providing similar functionality.
>
> > + __u32 length;
> > + __u32 flags;
> > +};
>
> addr really should be __u32 here so you don't have to mess with 32/64
> bit user/kernel issues, right?
Add is __u32 there. Are you suggesting it shouldn't be?
>
> thanks,
>
> greg k-h
Re: [PATCH v7 2/2] drivers/misc: Add Aspeed P2A control driver
On Tue, Mar 12, 2019 at 09:31:01AM -0700, Patrick Venture wrote:
> + phys_addr_t mem_base;
Is this really a 32bit value?
Your ioctl thinks it is:
> +struct aspeed_p2a_ctrl_mapping {
> + __u32 addr;
Does this driver not work on a 64bit kernel?
> + __u32 length;
> + __u32 flags;
> +};
addr really should be __u32 here so you don't have to mess with 32/64
bit user/kernel issues, right?
thanks,
greg k-h
[PATCH v7 2/2] drivers/misc: Add Aspeed P2A control driver
The ASPEED AST2400, and AST2500 in some configurations include a PCI-to-AHB MMIO bridge. This bridge allows a server to read and write in the BMC's physical address space. This feature is especially useful when using this bridge to send large files to the BMC. The host may use this to send down a firmware image by staging data at a specific memory address, and in a coordinated effort with the BMC's software stack and kernel, transmit the bytes. This driver enables the BMC to unlock the PCI bridge on demand, and configure it via ioctl to allow the host to write bytes to an agreed upon location. In the primary use-case, the region to use is known apriori on the BMC, and the host requests this information. Once this request is received, the BMC's software stack will enable the bridge and the region and then using some software flow control (possibly via IPMI packets), copy the bytes down. Once the process is complete, the BMC will disable the bridge and unset any region involved. The default behavior of this bridge when present is: enabled and all regions marked read-write. This driver will fix the regions to be read-only and then disable the bridge entirely. The memory regions protected are: * BMC flash MMIO window * System flash MMIO windows * SOC IO (peripheral MMIO) * DRAM The DRAM region itself is all of DRAM and cannot be further specified. Once the PCI bridge is enabled, the host can read all of DRAM, and if the DRAM section is write-enabled, then it can write to all of it. Signed-off-by: Patrick Venture --- Changes for v7: - Moved node under the syscon node and changed therefore how it grabs the phandle for the regmap. Changes for v6: - Cleaned up documentation - Added missing machine-readable copyright lines. - Fixed over 80 chars instances. - Changed error from invalid memory-region node to ENODEV. Changes for v5: - Fixup missing exit condition and remove extra jump. Changes for v4: - Added ioctl for reading back the memory-region configuration. - Cleaned up some unused variables. Changes for v3: - Deleted unused read and write methods. Changes for v2: - Dropped unused reads. - Moved call to disable bridge to before registering device. - Switch from using regs to using a syscon regmap. <<< IN PROGRESS - Updated the commit message. <<< TODO - Updated the bit flipped for SCU180_ENP2A - Dropped boolean region_specified variable. - Renamed p2a_ctrl in _probe to misc_ctrl per suggestion. - Renamed aspeed_p2a_region_search to aspeed_p2a_region_acquire - Updated commit message. --- drivers/misc/Kconfig | 8 + drivers/misc/Makefile| 1 + drivers/misc/aspeed-p2a-ctrl.c | 440 +++ include/uapi/linux/aspeed-p2a-ctrl.h | 62 4 files changed, 511 insertions(+) create mode 100644 drivers/misc/aspeed-p2a-ctrl.c create mode 100644 include/uapi/linux/aspeed-p2a-ctrl.h diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index f417b06e11c5..9de1bafe5606 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -485,6 +485,14 @@ config VEXPRESS_SYSCFG bus. System Configuration interface is one of the possible means of generating transactions on this bus. +config ASPEED_P2A_CTRL + depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON + tristate "Aspeed ast2400/2500 HOST P2A VGA MMIO to BMC bridge control" + help + Control Aspeed ast2400/2500 HOST P2A VGA MMIO to BMC mappings through + ioctl()s, the driver also provides an interface for userspace mappings to + a pre-defined region. + config ASPEED_LPC_CTRL depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON tristate "Aspeed ast2400/2500 HOST LPC to BMC bridge control" diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index e39ccbbc1b3a..57577aee354f 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -55,6 +55,7 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o obj-$(CONFIG_CXL_BASE) += cxl/ obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o +obj-$(CONFIG_ASPEED_P2A_CTRL) += aspeed-p2a-ctrl.o obj-$(CONFIG_PCI_ENDPOINT_TEST)+= pci_endpoint_test.o obj-$(CONFIG_OCXL) += ocxl/ obj-y += cardreader/ diff --git a/drivers/misc/aspeed-p2a-ctrl.c b/drivers/misc/aspeed-p2a-ctrl.c new file mode 100644 index ..9b9f831ef334 --- /dev/null +++ b/drivers/misc/aspeed-p2a-ctrl.c @@ -0,0 +1,440 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2019 Google Inc + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + * + * Provides a simple driver to control the ASPEED P2A interface which allows + * the host to read and write
