* [PATCH] ARM: dts: aspeed: tiogapass: Enable VUART
From: Vijay Khemka @ 2019-03-05 20:01 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190130181427.2469674-1-vijaykhemka@fb.com>
Please review below patch.
Regards
-Vijay
?On 1/30/19, 10:14 AM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
Enabling vuart for Facebook tiogapass
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 42e0d7a8e8d0..a058fb2985f7 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -64,6 +64,11 @@
status = "okay";
};
+&vuart {
+ // VUART Host Console
+ status = "okay";
+};
+
&uart1 {
// Host Console
status = "okay";
--
2.17.1
^ permalink raw reply
* [PATCH 2/6] drivers/misc: Add Aspeed XDMA engine driver
From: Eddie James @ 2019-03-05 21:45 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAK8P3a0eS11_T9wk4iLRT7A94Lh9gSf5QNGH4--pdA5HdEGMCw@mail.gmail.com>
On 3/5/19 2:01 AM, Arnd Bergmann wrote:
> On Mon, Mar 4, 2019 at 10:37 PM Eddie James <eajames@linux.ibm.com> wrote:
>> The XDMA engine embedded in the AST2500 SOC performs PCI DMA operations
>> between the SOC (acting as a BMC) and a host processor in a server.
>>
>> This commit adds a driver to control the XDMA engine and adds functions
>> to initialize the hardware and memory and start DMA operations.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Hi Eddie,
>
> Thanks for your submission! Overall this looks well-implemented, but
> I fear we already have too many ways of doing the same thing at
> the moment, and I would hope to avoid adding yet another user space
> interface for a specific hardware that does this.
>
> Your interface appears to be a fairly low-level variant, just doing
> single DMA transfers through ioctls, but configuring the PCIe
> endpoint over sysfs.
Hi, thanks for the quick response!
There is actually no PCIe configuration done in this driver. The two
sysfs entries control the system control unit (SCU) on the AST2500
purely to enable and disable entire PCIe devices. It might be possible
to control those devices more finely with a PCI endpoint driver, but
there is no need to do so. The XDMA engine does that by itself to
perform DMA fairly automatically.
If the sysfs entries are really troublesome, we can probably remove
those and find another way to control the SCU.
>
> Please have a look at the drivers/pci/endpoint framework first
> and see if you can work on top of that interface instead.
> Even if it doesn't quite do what you need here, we may be
> able to extend it in a way that works for you, and lets others
> use the same user interface extensions in the future.
>
> It may also be necessary to split out the DMA engine portion
> into a regular drivers/dma/ back-end to make that fit in with
> the PCIe endpoint framework.
Right, I did look into the normal DMA framework. There were a couple of
problems. First and foremost, the "device" (really, host processor)
address that we use is 64 bit, but the AST2500 is of course 32 bit. So I
couldn't find a good way to get the address through the DMA API into the
driver. It's entirely possible I missed something there though.
The other issue was that the vast majority of the DMA framework was
unused, resulting in a large amount of boilerplate that did nothing
except satisfy the API... I thought simplicity would be better in this case.
Let me know what you think... I could certainly switch to ioctl instead
of the write() if that's better. Or if you really think the DMA
framework is required here, let me know.
Thanks,
Eddie
>
> If you have already tried this without success, please let us
> know in the description what problems you have hit, and why you
> decided to create a new framework instead.
>
>> +/*
>> + * aspeed_xdma_op
>> + *
>> + * upstream: boolean indicating the direction of the DMA operation; upstream
>> + * means a transfer from the BMC to the host
>> + *
>> + * host_addr: the DMA address on the host side, typically configured by PCI
>> + * subsystem
>> + *
>> + * len: the size of the transfer in bytes; it should be a multiple of 16 bytes
>> + */
>> +struct aspeed_xdma_op {
>> + __u8 upstream;
>> + __u64 host_addr;
>> + __u32 len;
>> +} __packed;
> Side-note: packed structures are generally not great user space
> interfaces. Regardless of where we end up with this, I'd recommend
> naturally aligning each member inside of the structure, and using
> explicit padding here.
Understood, thanks.
>
>
> Arnd
>
^ permalink raw reply
* [PATCH 2/6] drivers/misc: Add Aspeed XDMA engine driver
From: Andrew Jeffery @ 2019-03-06 0:00 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <84551e54-0382-6042-48cb-842d79214a98@linux.ibm.com>
On Wed, 6 Mar 2019, at 08:15, Eddie James wrote:
>
> On 3/5/19 2:01 AM, Arnd Bergmann wrote:
> > On Mon, Mar 4, 2019 at 10:37 PM Eddie James <eajames@linux.ibm.com> wrote:
> >> The XDMA engine embedded in the AST2500 SOC performs PCI DMA operations
> >> between the SOC (acting as a BMC) and a host processor in a server.
> >>
> >> This commit adds a driver to control the XDMA engine and adds functions
> >> to initialize the hardware and memory and start DMA operations.
> >>
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > Hi Eddie,
> >
> > Thanks for your submission! Overall this looks well-implemented, but
> > I fear we already have too many ways of doing the same thing at
> > the moment, and I would hope to avoid adding yet another user space
> > interface for a specific hardware that does this.
> >
> > Your interface appears to be a fairly low-level variant, just doing
> > single DMA transfers through ioctls, but configuring the PCIe
> > endpoint over sysfs.
>
> Hi, thanks for the quick response!
>
> There is actually no PCIe configuration done in this driver. The two
> sysfs entries control the system control unit (SCU) on the AST2500
> purely to enable and disable entire PCIe devices. It might be possible
> to control those devices more finely with a PCI endpoint driver, but
> there is no need to do so. The XDMA engine does that by itself to
> perform DMA fairly automatically.
I had a series a while back to expose random bits from devices in sysfs. It got
shot down pretty well, but the main contention was over the devicetree
bindings.
I think we could revive it as a library-type thing that drivers can use to expose
bits like what you're describing without putting the grubby details in the
devicetree. The we would have a consistent approach to exposing otherwise
hard to describe functions (which is what a lot of a BMC turns out to be).
Andrew
^ permalink raw reply
* [Potential Spoof] Re: [PATCH v2] misc: aspeed-lpc-ctrl: make parameter optional
From: Vijay Khemka @ 2019-03-06 0:15 UTC (permalink / raw)
To: linux-aspeed
Joel,
Did this patch apply upstream. Somehow I can't find this patch in linux or linux-next or our obmc dev4.19.
Regards
-Vijay
?On 1/17/19, 10:53 AM, "Linux-aspeed on behalf of Vijay Khemka" <linux-aspeed-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of vijaykhemka@fb.com> wrote:
On 1/16/19, 10:17 PM, "Joel Stanley" <joel@jms.id.au> wrote:
On Thu, 17 Jan 2019 at 09:02, Vijay Khemka <vijaykhemka@fb.com> wrote:
>
> Makiing memory-region and flash as optional parameter in device
> tree if user needs to use these parameter through ioctl then
> need to define in devicetree.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
Thanks! This looks okay to me. I tested it on one of our systems which
uses both flash and reserved memory and it was fine.
Reviewed-by: Joel Stanley <joel@jms.id.au>
Can you also send a patch to update the bindings at
Documentation/devicetree/bindings/mfd/aspeed-lpc.txt ? I think the
only change you need to make is to move the memory region and flash
properties to optional (instead of required).
Sure I will do this.
Cheers,
Joel
> ---
> drivers/misc/aspeed-lpc-ctrl.c | 58 +++++++++++++++++++++-------------
> 1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
> index a024f8042259..332210e06e98 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> unsigned long param)
> {
> struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> + struct device *dev = file->private_data;
> void __user *p = (void __user *)param;
> struct aspeed_lpc_ctrl_mapping map;
> u32 addr;
> @@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> if (map.window_id != 0)
> return -EINVAL;
>
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved memory\n");
> + return -EINVAL;
> + }
> +
> map.size = lpc_ctrl->mem_size;
>
> return copy_to_user(p, &map, sizeof(map)) ? -EFAULT : 0;
> @@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> return -EINVAL;
>
> if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
> + if (!lpc_ctrl->pnor_size) {
> + dev_err(dev, "Didn't find host pnor flash\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->pnor_base;
> size = lpc_ctrl->pnor_size;
> } else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved memory\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->mem_base;
> size = lpc_ctrl->mem_size;
> } else {
> @@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
> if (!lpc_ctrl)
> return -ENOMEM;
>
> + /* If flash is described in device tree then store */
> 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_address_to_resource(node, 1, &resm);
> - of_node_put(node);
> - if (rc) {
> - dev_err(dev, "Couldn't address to resource for flash\n");
> - return rc;
> + dev_dbg(dev, "Didn't find host pnor flash node\n");
> + } else {
> + rc = of_address_to_resource(node, 1, &resm);
> + of_node_put(node);
> + if (rc) {
> + dev_err(dev, "Couldn't address to resource for flash\n");
> + return rc;
> + }
> }
>
> lpc_ctrl->pnor_size = resource_size(&resm);
> @@ -214,22 +230,22 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
>
> dev_set_drvdata(&pdev->dev, lpc_ctrl);
>
> + /* If memory-region is described in device tree then store */
> node = of_parse_phandle(dev->of_node, "memory-region", 0);
> if (!node) {
> - dev_err(dev, "Didn't find reserved memory\n");
> - return -EINVAL;
> - }
> + dev_dbg(dev, "Didn't find reserved memory\n");
> + } else {
> + rc = of_address_to_resource(node, 0, &resm);
> + of_node_put(node);
> + if (rc) {
> + dev_err(dev, "Couldn't address to resource for reserved memory\n");
> + return -ENOMEM;
> + }
>
> - rc = of_address_to_resource(node, 0, &resm);
> - of_node_put(node);
> - if (rc) {
> - dev_err(dev, "Couldn't address to resource for reserved memory\n");
> - return -ENOMEM;
> + lpc_ctrl->mem_size = resource_size(&resm);
> + lpc_ctrl->mem_base = resm.start;
> }
>
> - lpc_ctrl->mem_size = resource_size(&resm);
> - lpc_ctrl->mem_base = resm.start;
> -
> lpc_ctrl->regmap = syscon_node_to_regmap(
> pdev->dev.parent->of_node);
> if (IS_ERR(lpc_ctrl->regmap)) {
> @@ -258,8 +274,6 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
> goto err;
> }
>
> - dev_info(dev, "Loaded at %pr\n", &resm);
> -
> return 0;
>
> err:
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH 2/6] drivers/misc: Add Aspeed XDMA engine driver
From: Arnd Bergmann @ 2019-03-06 10:48 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <84551e54-0382-6042-48cb-842d79214a98@linux.ibm.com>
On Tue, Mar 5, 2019 at 10:45 PM Eddie James <eajames@linux.ibm.com> wrote:
> On 3/5/19 2:01 AM, Arnd Bergmann wrote:
> > On Mon, Mar 4, 2019 at 10:37 PM Eddie James <eajames@linux.ibm.com> wrote:
> >> The XDMA engine embedded in the AST2500 SOC performs PCI DMA operations
> >> between the SOC (acting as a BMC) and a host processor in a server.
> >>
> >> This commit adds a driver to control the XDMA engine and adds functions
> >> to initialize the hardware and memory and start DMA operations.
> >>
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > Hi Eddie,
> >
> > Thanks for your submission! Overall this looks well-implemented, but
> > I fear we already have too many ways of doing the same thing at
> > the moment, and I would hope to avoid adding yet another user space
> > interface for a specific hardware that does this.
> >
> > Your interface appears to be a fairly low-level variant, just doing
> > single DMA transfers through ioctls, but configuring the PCIe
> > endpoint over sysfs.
>
> Hi, thanks for the quick response!
>
> There is actually no PCIe configuration done in this driver. The two
> sysfs entries control the system control unit (SCU) on the AST2500
> purely to enable and disable entire PCIe devices. It might be possible
> to control those devices more finely with a PCI endpoint driver, but
> there is no need to do so. The XDMA engine does that by itself to
> perform DMA fairly automatically.
>
> If the sysfs entries are really troublesome, we can probably remove
> those and find another way to control the SCU.
I think the main advantage of tying this to a PCIe endpoint driver
is that this would give us a logical object in the kernel that we
can add the user space interface to, and have the protocol on
top of it be portable between different SoCs.
> > Please have a look at the drivers/pci/endpoint framework first
> > and see if you can work on top of that interface instead.
> > Even if it doesn't quite do what you need here, we may be
> > able to extend it in a way that works for you, and lets others
> > use the same user interface extensions in the future.
> >
> > It may also be necessary to split out the DMA engine portion
> > into a regular drivers/dma/ back-end to make that fit in with
> > the PCIe endpoint framework.
>
> Right, I did look into the normal DMA framework. There were a couple of
> problems. First and foremost, the "device" (really, host processor)
> address that we use is 64 bit, but the AST2500 is of course 32 bit. So I
> couldn't find a good way to get the address through the DMA API into the
> driver. It's entirely possible I missed something there though.
32-bit ARM SoCs can be built with a 64-bit dma_addr_t. Would that
help you here?
> The other issue was that the vast majority of the DMA framework was
> unused, resulting in a large amount of boilerplate that did nothing
> except satisfy the API... I thought simplicity would be better in this case.
Simplicity is important indeed, but we have to weigh it against
having a consistent interface. What the dmaengine driver would
give us in combination with the PCIe endpoint driver is that it abstracts
the hardware from the protocol on top, which could then be done
in a way that is not specific to an AST2xxx chip.
> Let me know what you think... I could certainly switch to ioctl instead
> of the write() if that's better. Or if you really think the DMA
> framework is required here, let me know.
I don't think that replacing the ioctl() with a write() call specifically
would make much of a difference here. The question I'd like to
discuss further is what high-level user space interface you actually
need in order to implement what kind of functionality. We can then
look at whether this interface can be implemented on top of a
PCIe endpoint and a dmaengine driver in a portable way. If all
of those are true, then I'd definitely go with the modular approach
of having two standard drivers for the PCIe endpoint (should be
a trivial wrapper) and the dma engine (not trivial, but there are
many examples), plus a generic front-end in
drivers/pci/endpoint/functions/.
Arnd
^ permalink raw reply
* [PATCH v6 1/2] dt-bindings: misc: aspeed-p2a-ctrl: add support
From: Rob Herring @ 2019-03-11 22:20 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190304185536.211084-1-venture@google.com>
On Mon, Mar 04, 2019 at 10:55:36AM -0800, Patrick Venture wrote:
> Document the ast2400, ast2500 PCI-to-AHB bridge control driver bindings.
>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
> Changes for v6:
> - None
> Changes for v5:
> - None
> Changes for v4:
> - None
> Changes for v3:
> - None
> Changes for v2:
> - Added comment about syscon required parameter.
> ---
> .../bindings/misc/aspeed-p2a-ctrl.txt | 32 +++++++++++++++++++
> 1 file changed, 32 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
>
> diff --git a/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> new file mode 100644
> index 000000000000..1092d62d1c92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> @@ -0,0 +1,32 @@
> +======================================================================
> +Device tree bindings for Aspeed AST2400/AST2500 PCI-to-AHB Bridge Control Driver
> +======================================================================
> +
> +The bridge is available on platforms with the VGA enabled on the Aspeed device.
> +In this case, the host has access to a 64KiB window into all of the BMC's
> +memory. The BMC can disable this bridge. If the bridge is enabled, the host
> +has read access to all the regions of memory, however the host only has read
> +and write access depending on a register controlled by the BMC.
> +
> +Required properties:
> +===================
> +
> + - compatible: must be one of:
> + - "aspeed,ast2400-p2a-ctrl"
> + - "aspeed,ast2500-p2a-ctrl"
> +
> + - syscon: handle to syscon device node controlling PCI.
> +
> +Optional properties:
> +===================
> +
> +- memory-region: A phandle to a reserved_memory region to be used for the PCI
> + to AHB mapping
> +
> +Example:
> +
> +p2a: p2a-control at 1e6e2000 {
> + compatible = "aspeed,ast2400-p2a-ctrl";
> + memory-region = <&reserved_memory>;
> + syscon = <&syscon>;
Make this node a child of what you are pointing to instead if this the
only control interface.
Rob
^ permalink raw reply
* [PATCH v6 1/2] dt-bindings: misc: aspeed-p2a-ctrl: add support
From: Patrick Venture @ 2019-03-11 23:49 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190311222007.GA16254@bogus>
On Mon, Mar 11, 2019 at 3:20 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Mar 04, 2019 at 10:55:36AM -0800, Patrick Venture wrote:
> > Document the ast2400, ast2500 PCI-to-AHB bridge control driver bindings.
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> > Changes for v6:
> > - None
> > Changes for v5:
> > - None
> > Changes for v4:
> > - None
> > Changes for v3:
> > - None
> > Changes for v2:
> > - Added comment about syscon required parameter.
> > ---
> > .../bindings/misc/aspeed-p2a-ctrl.txt | 32 +++++++++++++++++++
> > 1 file changed, 32 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> >
> > diff --git a/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> > new file mode 100644
> > index 000000000000..1092d62d1c92
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> > @@ -0,0 +1,32 @@
> > +======================================================================
> > +Device tree bindings for Aspeed AST2400/AST2500 PCI-to-AHB Bridge Control Driver
> > +======================================================================
> > +
> > +The bridge is available on platforms with the VGA enabled on the Aspeed device.
> > +In this case, the host has access to a 64KiB window into all of the BMC's
> > +memory. The BMC can disable this bridge. If the bridge is enabled, the host
> > +has read access to all the regions of memory, however the host only has read
> > +and write access depending on a register controlled by the BMC.
> > +
> > +Required properties:
> > +===================
> > +
> > + - compatible: must be one of:
> > + - "aspeed,ast2400-p2a-ctrl"
> > + - "aspeed,ast2500-p2a-ctrl"
> > +
> > + - syscon: handle to syscon device node controlling PCI.
> > +
> > +Optional properties:
> > +===================
> > +
> > +- memory-region: A phandle to a reserved_memory region to be used for the PCI
> > + to AHB mapping
> > +
> > +Example:
> > +
> > +p2a: p2a-control at 1e6e2000 {
> > + compatible = "aspeed,ast2400-p2a-ctrl";
> > + memory-region = <&reserved_memory>;
> > + syscon = <&syscon>;
>
> Make this node a child of what you are pointing to instead if this the
> only control interface.
You're suggesting I make this a child of the syscon?
>
> Rob
>
>
>
^ permalink raw reply
* [PATCH v6 1/2] dt-bindings: misc: aspeed-p2a-ctrl: add support
From: Rob Herring @ 2019-03-12 2:38 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAO=notzjsdOxJSf+p+OqB71N5-c1QPCcfd9OVAV8ft18e+SngQ@mail.gmail.com>
On Mon, Mar 11, 2019 at 6:49 PM Patrick Venture <venture@google.com> wrote:
>
> On Mon, Mar 11, 2019 at 3:20 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Mar 04, 2019 at 10:55:36AM -0800, Patrick Venture wrote:
> > > Document the ast2400, ast2500 PCI-to-AHB bridge control driver bindings.
> > >
> > > Signed-off-by: Patrick Venture <venture@google.com>
> > > ---
> > > Changes for v6:
> > > - None
> > > Changes for v5:
> > > - None
> > > Changes for v4:
> > > - None
> > > Changes for v3:
> > > - None
> > > Changes for v2:
> > > - Added comment about syscon required parameter.
> > > ---
> > > .../bindings/misc/aspeed-p2a-ctrl.txt | 32 +++++++++++++++++++
> > > 1 file changed, 32 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> > > new file mode 100644
> > > index 000000000000..1092d62d1c92
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> > > @@ -0,0 +1,32 @@
> > > +======================================================================
> > > +Device tree bindings for Aspeed AST2400/AST2500 PCI-to-AHB Bridge Control Driver
> > > +======================================================================
> > > +
> > > +The bridge is available on platforms with the VGA enabled on the Aspeed device.
> > > +In this case, the host has access to a 64KiB window into all of the BMC's
> > > +memory. The BMC can disable this bridge. If the bridge is enabled, the host
> > > +has read access to all the regions of memory, however the host only has read
> > > +and write access depending on a register controlled by the BMC.
> > > +
> > > +Required properties:
> > > +===================
> > > +
> > > + - compatible: must be one of:
> > > + - "aspeed,ast2400-p2a-ctrl"
> > > + - "aspeed,ast2500-p2a-ctrl"
> > > +
> > > + - syscon: handle to syscon device node controlling PCI.
> > > +
> > > +Optional properties:
> > > +===================
> > > +
> > > +- memory-region: A phandle to a reserved_memory region to be used for the PCI
> > > + to AHB mapping
> > > +
> > > +Example:
> > > +
> > > +p2a: p2a-control at 1e6e2000 {
> > > + compatible = "aspeed,ast2400-p2a-ctrl";
> > > + memory-region = <&reserved_memory>;
> > > + syscon = <&syscon>;
> >
> > Make this node a child of what you are pointing to instead if this the
> > only control interface.
>
> You're suggesting I make this a child of the syscon?
Yes.
Rob
^ permalink raw reply
* [PATCH v6 1/2] dt-bindings: misc: aspeed-p2a-ctrl: add support
From: Patrick Venture @ 2019-03-12 14:23 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAL_Jsq+M2P8eEkwS9-mLXfoA3LC6K-_WHUns5sH75E6h6qC2fA@mail.gmail.com>
On Mon, Mar 11, 2019 at 7:38 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Mar 11, 2019 at 6:49 PM Patrick Venture <venture@google.com> wrote:
> >
> > On Mon, Mar 11, 2019 at 3:20 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Mon, Mar 04, 2019 at 10:55:36AM -0800, Patrick Venture wrote:
> > > > Document the ast2400, ast2500 PCI-to-AHB bridge control driver bindings.
> > > >
> > > > Signed-off-by: Patrick Venture <venture@google.com>
> > > > ---
> > > > Changes for v6:
> > > > - None
> > > > Changes for v5:
> > > > - None
> > > > Changes for v4:
> > > > - None
> > > > Changes for v3:
> > > > - None
> > > > Changes for v2:
> > > > - Added comment about syscon required parameter.
> > > > ---
> > > > .../bindings/misc/aspeed-p2a-ctrl.txt | 32 +++++++++++++++++++
> > > > 1 file changed, 32 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> > > > new file mode 100644
> > > > index 000000000000..1092d62d1c92
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> > > > @@ -0,0 +1,32 @@
> > > > +======================================================================
> > > > +Device tree bindings for Aspeed AST2400/AST2500 PCI-to-AHB Bridge Control Driver
> > > > +======================================================================
> > > > +
> > > > +The bridge is available on platforms with the VGA enabled on the Aspeed device.
> > > > +In this case, the host has access to a 64KiB window into all of the BMC's
> > > > +memory. The BMC can disable this bridge. If the bridge is enabled, the host
> > > > +has read access to all the regions of memory, however the host only has read
> > > > +and write access depending on a register controlled by the BMC.
> > > > +
> > > > +Required properties:
> > > > +===================
> > > > +
> > > > + - compatible: must be one of:
> > > > + - "aspeed,ast2400-p2a-ctrl"
> > > > + - "aspeed,ast2500-p2a-ctrl"
> > > > +
> > > > + - syscon: handle to syscon device node controlling PCI.
> > > > +
> > > > +Optional properties:
> > > > +===================
> > > > +
> > > > +- memory-region: A phandle to a reserved_memory region to be used for the PCI
> > > > + to AHB mapping
> > > > +
> > > > +Example:
> > > > +
> > > > +p2a: p2a-control at 1e6e2000 {
> > > > + compatible = "aspeed,ast2400-p2a-ctrl";
> > > > + memory-region = <&reserved_memory>;
> > > > + syscon = <&syscon>;
> > >
> > > Make this node a child of what you are pointing to instead if this the
> > > only control interface.
> >
> > You're suggesting I make this a child of the syscon?
>
> Yes.
Roger that, will update and send out a patchset v6 with the
corresponding changes.
>
> Rob
^ permalink raw reply
* [PATCH v7 1/2] dt-bindings: misc: aspeed-p2a-ctrl: add support
From: Patrick Venture @ 2019-03-12 16:30 UTC (permalink / raw)
To: linux-aspeed
Document the ast2400, ast2500 PCI-to-AHB bridge control driver bindings.
Signed-off-by: Patrick Venture <venture@google.com>
---
Changes for v7:
- Moved node under the syscon node it requires
Changes for v6:
- None
Changes for v5:
- None
Changes for v4:
- None
Changes for v3:
- None
Changes for v2:
- Added comment about syscon required parameter.
---
.../bindings/misc/aspeed-p2a-ctrl.txt | 48 +++++++++++++++++++
1 file changed, 48 insertions(+)
create mode 100644 Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
diff --git a/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
new file mode 100644
index 000000000000..088cc4e3dc54
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
@@ -0,0 +1,48 @@
+======================================================================
+Device tree bindings for Aspeed AST2400/AST2500 PCI-to-AHB Bridge Control Driver
+======================================================================
+
+The bridge is available on platforms with the VGA enabled on the Aspeed device.
+In this case, the host has access to a 64KiB window into all of the BMC's
+memory. The BMC can disable this bridge. If the bridge is enabled, the host
+has read access to all the regions of memory, however the host only has read
+and write access depending on a register controlled by the BMC.
+
+Required properties:
+===================
+
+ - compatible: must be one of:
+ - "aspeed,ast2400-p2a-ctrl"
+ - "aspeed,ast2500-p2a-ctrl"
+
+ - syscon: handle to syscon device node controlling PCI.
+
+Optional properties:
+===================
+
+- memory-region: A phandle to a reserved_memory region to be used for the PCI
+ to AHB mapping
+
+The p2a-control node should be the child of a syscon node with the required
+property:
+
+- compatible : Should be one of the following:
+ "aspeed,ast2400-scu", "syscon", "simple-mfd"
+ "aspeed,g4-scu", "syscon", "simple-mfd"
+ "aspeed,ast2500-scu", "syscon", "simple-mfd"
+ "aspeed,g5-scu", "syscon", "simple-mfd"
+
+Example:
+
+g4 Example
+----------
+
+syscon: scu at 1e6e2000 {
+ compatible = "aspeed,ast2400-scu", "syscon", "simple-mfd";
+ reg = <0x1e6e2000 0x1a8>;
+
+ p2a: p2a-control {
+ compatible = "aspeed,ast2400-p2a-ctrl";
+ memory-region = <&reserved_memory>;
+ };
+};
--
2.21.0.360.g471c308f928-goog
^ permalink raw reply related
* [PATCH v7 2/2] drivers/misc: Add Aspeed P2A control driver
From: Patrick Venture @ 2019-03-12 16:31 UTC (permalink / raw)
To: linux-aspeed
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 <venture@google.com>
---
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 000000000000..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 to various regions of the BMC's memory.
+ */
+
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include <linux/aspeed-p2a-ctrl.h>
+
+#define DEVICE_NAME "aspeed-p2a-ctrl"
+
+/* SCU2C is a Misc. Control Register. */
+#define SCU2C 0x2c
+/* SCU180 is the PCIe Configuration Setting Control Register. */
+#define SCU180 0x180
+/* Bit 1 controls the P2A bridge, while bit 0 controls the entire VGA device
+ * on the PCI bus.
+ */
+#define SCU180_ENP2A BIT(1)
+
+/* The ast2400/2500 both have six ranges. */
+#define P2A_REGION_COUNT 6
+
+struct region {
+ u32 min;
+ u32 max;
+ u32 bit;
+};
+
+struct aspeed_p2a_model_data {
+ /* min, max, bit */
+ struct region regions[P2A_REGION_COUNT];
+};
+
+struct aspeed_p2a_ctrl {
+ struct miscdevice miscdev;
+ struct regmap *regmap;
+
+ const struct aspeed_p2a_model_data *config;
+
+ /* Access to these needs to be locked, held via probe, mapping ioctl,
+ * and release, remove.
+ */
+ struct mutex tracking;
+ u32 readers;
+ u32 readerwriters[P2A_REGION_COUNT];
+
+ phys_addr_t mem_base;
+ resource_size_t mem_size;
+};
+
+struct aspeed_p2a_user {
+ struct file *file;
+ struct aspeed_p2a_ctrl *parent;
+
+ /* The entire memory space is opened for reading once the bridge is
+ * enabled, therefore this needs only to be tracked once per user.
+ * If any user has it open for read, the bridge must stay enabled.
+ */
+ u32 read;
+
+ /* Each entry of the array corresponds to a P2A Region. If the user
+ * opens for read or readwrite, the reference goes up here. On
+ * release, this array is walked and references adjusted accordingly.
+ */
+ u32 readwrite[P2A_REGION_COUNT];
+};
+
+static void aspeed_p2a_enable_bridge(struct aspeed_p2a_ctrl *p2a_ctrl)
+{
+ regmap_update_bits(p2a_ctrl->regmap,
+ SCU180, SCU180_ENP2A, SCU180_ENP2A);
+}
+
+static void aspeed_p2a_disable_bridge(struct aspeed_p2a_ctrl *p2a_ctrl)
+{
+ regmap_update_bits(p2a_ctrl->regmap, SCU180, SCU180_ENP2A, 0);
+}
+
+static int aspeed_p2a_mmap(struct file *file, struct vm_area_struct *vma)
+{
+ struct aspeed_p2a_user *priv = file->private_data;
+ struct aspeed_p2a_ctrl *ctrl = priv->parent;
+
+ if (ctrl->mem_base == 0 && ctrl->mem_size == 0)
+ return -EINVAL;
+
+ unsigned long vsize = vma->vm_end - vma->vm_start;
+ pgprot_t prot = vma->vm_page_prot;
+
+ if (vma->vm_pgoff + vsize > ctrl->mem_base + ctrl->mem_size)
+ return -EINVAL;
+
+ /* ast2400/2500 AHB accesses are not cache coherent */
+ prot = pgprot_noncached(prot);
+
+ if (remap_pfn_range(vma, vma->vm_start,
+ (ctrl->mem_base >> PAGE_SHIFT) + vma->vm_pgoff,
+ vsize, prot))
+ return -EAGAIN;
+
+ return 0;
+}
+
+static void aspeed_p2a_region_acquire(struct aspeed_p2a_user *priv,
+ struct aspeed_p2a_ctrl *ctrl,
+ struct aspeed_p2a_ctrl_mapping *map)
+{
+ int i;
+ u32 base, end;
+
+ base = map->addr;
+ end = map->addr + (map->length - 1);
+
+ /* If the value is a legal u32, it will find a match. */
+ for (i = 0; i < P2A_REGION_COUNT; i++) {
+ const struct region *curr = &ctrl->config->regions[i];
+
+ /* If the top of this region is lower than your base, skip it.
+ */
+ if (curr->max < base)
+ continue;
+
+ /* If the bottom of this region is higher than your end, bail.
+ */
+ if (curr->min > end)
+ break;
+ /* Lock this and update it, therefore it someone else is
+ * closing their file out, this'll preserve the increment.
+ */
+ mutex_lock(&ctrl->tracking);
+ ctrl->readerwriters[i] += 1;
+ mutex_unlock(&ctrl->tracking);
+
+ /* Track with the user, so when they close their file, we can
+ * decrement properly.
+ */
+ priv->readwrite[i] += 1;
+
+ /* Enable the region as read-write. */
+ regmap_update_bits(ctrl->regmap, SCU2C, curr->bit, 0);
+ }
+}
+
+static long aspeed_p2a_ioctl(struct file *file, unsigned int cmd,
+ unsigned long data)
+{
+ struct aspeed_p2a_user *priv = file->private_data;
+ struct aspeed_p2a_ctrl *ctrl = priv->parent;
+ void __user *arg = (void __user *)data;
+ struct aspeed_p2a_ctrl_mapping map;
+
+ if (copy_from_user(&map, arg, sizeof(map)))
+ return -EFAULT;
+
+ switch (cmd) {
+ case ASPEED_P2A_CTRL_IOCTL_SET_WINDOW:
+ /* If they want a region to be read-only, since the entire
+ * region is read-only once enabled, we just need to track this
+ * user wants to read from the bridge, and if it's not enabled.
+ * Enable it.
+ */
+ if (map.flags == ASPEED_P2A_CTRL_READ_ONLY) {
+ mutex_lock(&ctrl->tracking);
+ ctrl->readers += 1;
+ mutex_unlock(&ctrl->tracking);
+
+ /* Track with the user, so when they close their file,
+ * we can decrement properly.
+ */
+ priv->read += 1;
+ } else if (map.flags == ASPEED_P2A_CTRL_READWRITE) {
+ aspeed_p2a_region_acquire(priv, ctrl, &map);
+ } else {
+ /* Invalid map flags. */
+ return -EINVAL;
+ }
+
+ aspeed_p2a_enable_bridge(ctrl);
+ return 0;
+ case ASPEED_P2A_CTRL_IOCTL_GET_MEMORY_CONFIG:
+ /* This is a request for the memory-region and corresponding
+ * length that is used by the driver for mmap.
+ */
+
+ map.flags = 0;
+ map.addr = ctrl->mem_base;
+ map.length = ctrl->mem_size;
+
+ return copy_to_user(arg, &map, sizeof(map)) ? -EFAULT : 0;
+ }
+
+ return -EINVAL;
+}
+
+
+/*
+ * When a user opens this file, we create a structure to track their mappings.
+ *
+ * A user can map a region as read-only (bridge enabled), or read-write (bit
+ * flipped, and bridge enabled). Either way, this tracking is used, s.t. when
+ * they release the device references are handled.
+ *
+ * The bridge is not enabled until a user calls an ioctl to map a region,
+ * simply opening the device does not enable it.
+ */
+static int aspeed_p2a_open(struct inode *inode, struct file *file)
+{
+ struct aspeed_p2a_user *priv;
+
+ priv = kmalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->file = file;
+ priv->read = 0;
+ memset(priv->readwrite, 0, sizeof(priv->readwrite));
+
+ /* The file's private_data is initialized to the p2a_ctrl. */
+ priv->parent = file->private_data;
+
+ /* Set the file's private_data to the user's data. */
+ file->private_data = priv;
+
+ return 0;
+}
+
+/*
+ * This will close the users mappings. It will go through what they had opened
+ * for readwrite, and decrement those counts. If at the end, this is the last
+ * user, it'll close the bridge.
+ */
+static int aspeed_p2a_release(struct inode *inode, struct file *file)
+{
+ int i;
+ u32 value;
+ u32 bits = 0;
+ bool open_regions = false;
+ struct aspeed_p2a_user *priv = file->private_data;
+
+ /* Lock others from changing these values until everything is updated
+ * in one pass.
+ */
+ mutex_lock(&priv->parent->tracking);
+
+ priv->parent->readers -= priv->read;
+
+ for (i = 0; i < P2A_REGION_COUNT; i++) {
+ priv->parent->readerwriters[i] -= priv->readwrite[i];
+
+ if (priv->parent->readerwriters[i] > 0)
+ open_regions = true;
+ else
+ bits |= priv->parent->config->regions[i].bit;
+ }
+
+ /* Setting a bit to 1 disables the region, so let's just OR with the
+ * above to disable any.
+ */
+
+ /* Note, if another user is trying to ioctl, they can't grab tracking,
+ * and therefore can't grab either register mutex.
+ * If another user is trying to close, they can't grab tracking either.
+ */
+ regmap_update_bits(priv->parent->regmap, SCU2C, bits, bits);
+
+ /* If parent->readers is zero and open windows is 0, disable the
+ * bridge.
+ */
+ if (!open_regions && priv->parent->readers == 0)
+ aspeed_p2a_disable_bridge(priv->parent);
+
+ mutex_unlock(&priv->parent->tracking);
+
+ kfree(priv);
+
+ return 0;
+}
+
+static const struct file_operations aspeed_p2a_ctrl_fops = {
+ .owner = THIS_MODULE,
+ .mmap = aspeed_p2a_mmap,
+ .unlocked_ioctl = aspeed_p2a_ioctl,
+ .open = aspeed_p2a_open,
+ .release = aspeed_p2a_release,
+};
+
+/* The regions are controlled by SCU2C */
+static void aspeed_p2a_disable_all(struct aspeed_p2a_ctrl *p2a_ctrl)
+{
+ int i;
+ u32 value = 0;
+
+ for (i = 0; i < P2A_REGION_COUNT; i++)
+ value |= p2a_ctrl->config->regions[i].bit;
+
+ regmap_update_bits(p2a_ctrl->regmap, SCU2C, value, value);
+
+ /* Disable the bridge. */
+ aspeed_p2a_disable_bridge(p2a_ctrl);
+}
+
+static int aspeed_p2a_ctrl_probe(struct platform_device *pdev)
+{
+ struct aspeed_p2a_ctrl *misc_ctrl;
+ struct device *dev;
+ struct resource *res, resm;
+ struct device_node *node;
+ int rc = 0;
+
+ dev = &pdev->dev;
+
+ misc_ctrl = devm_kzalloc(dev, sizeof(*misc_ctrl), GFP_KERNEL);
+ if (!misc_ctrl)
+ return -ENOMEM;
+
+ mutex_init(&misc_ctrl->tracking);
+ misc_ctrl->readers = 0;
+ memset(misc_ctrl->readerwriters, 0, sizeof(misc_ctrl->readerwriters));
+
+ misc_ctrl->mem_size = 0;
+ misc_ctrl->mem_base = 0;
+
+ /* optional. */
+ node = of_parse_phandle(dev->of_node, "memory-region", 0);
+ if (node) {
+ rc = of_address_to_resource(node, 0, &resm);
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for reserved memory\n");
+ return -ENODEV;
+ }
+
+ misc_ctrl->mem_size = resource_size(&resm);
+ misc_ctrl->mem_base = resm.start;
+ }
+
+ misc_ctrl->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
+ if (IS_ERR(misc_ctrl->regmap)) {
+ dev_err(dev, "Couldn't get regmap\n");
+ return -ENODEV;
+ }
+
+ misc_ctrl->config = of_device_get_match_data(dev);
+
+ dev_set_drvdata(&pdev->dev, misc_ctrl);
+
+ aspeed_p2a_disable_all(misc_ctrl);
+
+ misc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
+ misc_ctrl->miscdev.name = DEVICE_NAME;
+ misc_ctrl->miscdev.fops = &aspeed_p2a_ctrl_fops;
+ misc_ctrl->miscdev.parent = dev;
+
+ rc = misc_register(&misc_ctrl->miscdev);
+ if (rc)
+ dev_err(dev, "Unable to register device\n");
+
+ return rc;
+}
+
+static int aspeed_p2a_ctrl_remove(struct platform_device *pdev)
+{
+ struct aspeed_p2a_ctrl *p2a_ctrl = dev_get_drvdata(&pdev->dev);
+
+ misc_deregister(&p2a_ctrl->miscdev);
+
+ return 0;
+}
+
+#define SCU2C_DRAM BIT(25)
+#define SCU2C_SPI BIT(24)
+#define SCU2C_SOC BIT(23)
+#define SCU2C_FLASH BIT(22)
+
+static const struct aspeed_p2a_model_data ast2400_model_data = {
+ .regions = {
+ {0x00000000, 0x17FFFFFF, SCU2C_FLASH},
+ {0x18000000, 0x1FFFFFFF, SCU2C_SOC},
+ {0x20000000, 0x2FFFFFFF, SCU2C_FLASH},
+ {0x30000000, 0x3FFFFFFF, SCU2C_SPI},
+ {0x40000000, 0x5FFFFFFF, SCU2C_DRAM},
+ {0x60000000, 0xFFFFFFFF, SCU2C_SOC},
+ }
+};
+
+static const struct aspeed_p2a_model_data ast2500_model_data = {
+ .regions = {
+ {0x00000000, 0x0FFFFFFF, SCU2C_FLASH},
+ {0x10000000, 0x1FFFFFFF, SCU2C_SOC},
+ {0x20000000, 0x3FFFFFFF, SCU2C_FLASH},
+ {0x40000000, 0x5FFFFFFF, SCU2C_SOC},
+ {0x60000000, 0x7FFFFFFF, SCU2C_SPI},
+ {0x80000000, 0xFFFFFFFF, SCU2C_DRAM},
+ }
+};
+
+static const struct of_device_id aspeed_p2a_ctrl_match[] = {
+ { .compatible = "aspeed,ast2400-p2a-ctrl",
+ .data = &ast2400_model_data },
+ { .compatible = "aspeed,ast2500-p2a-ctrl",
+ .data = &ast2500_model_data },
+ { },
+};
+
+static struct platform_driver aspeed_p2a_ctrl_driver = {
+ .driver = {
+ .name = DEVICE_NAME,
+ .of_match_table = aspeed_p2a_ctrl_match,
+ },
+ .probe = aspeed_p2a_ctrl_probe,
+ .remove = aspeed_p2a_ctrl_remove,
+};
+
+module_platform_driver(aspeed_p2a_ctrl_driver);
+
+MODULE_DEVICE_TABLE(of, aspeed_p2a_ctrl_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Patrick Venture <venture@google.com>");
+MODULE_DESCRIPTION("Control for aspeed 2400/2500 P2A VGA HOST to BMC mappings");
diff --git a/include/uapi/linux/aspeed-p2a-ctrl.h b/include/uapi/linux/aspeed-p2a-ctrl.h
new file mode 100644
index 000000000000..c1d08f7bce6d
--- /dev/null
+++ b/include/uapi/linux/aspeed-p2a-ctrl.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * 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 to various regions of the BMC's memory.
+ */
+
+#ifndef _UAPI_LINUX_ASPEED_P2A_CTRL_H
+#define _UAPI_LINUX_ASPEED_P2A_CTRL_H
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+#define ASPEED_P2A_CTRL_READ_ONLY 0
+#define ASPEED_P2A_CTRL_READWRITE 1
+
+/*
+ * This driver provides a mechanism for enabling or disabling the read-write
+ * property of specific windows into the ASPEED BMC's memory.
+ *
+ * A user can map a region of the BMC's memory as read-only or read-write, with
+ * the caveat that once any region is mapped, all regions are unlocked for
+ * reading.
+ */
+
+/*
+ * Unlock a region of BMC physical memory for access from the host.
+ *
+ * Also used to read back the optional memory-region configuration for the
+ * driver.
+ */
+struct aspeed_p2a_ctrl_mapping {
+ __u32 addr;
+ __u32 length;
+ __u32 flags;
+};
+
+#define __ASPEED_P2A_CTRL_IOCTL_MAGIC 0xb3
+
+/*
+ * This IOCTL is meant to configure a region or regions of memory given a
+ * starting address and length to be readable by the host, or
+ * readable-writeable.
+ */
+#define ASPEED_P2A_CTRL_IOCTL_SET_WINDOW _IOW(__ASPEED_P2A_CTRL_IOCTL_MAGIC, \
+ 0x00, struct aspeed_p2a_ctrl_mapping)
+
+/*
+ * This IOCTL is meant to read back to the user the base address and length of
+ * the memory-region specified to the driver for use with mmap.
+ */
+#define ASPEED_P2A_CTRL_IOCTL_GET_MEMORY_CONFIG \
+ _IOWR(__ASPEED_P2A_CTRL_IOCTL_MAGIC, \
+ 0x01, struct aspeed_p2a_ctrl_mapping)
+
+#endif /* _UAPI_LINUX_ASPEED_P2A_CTRL_H */
--
2.21.0.360.g471c308f928-goog
^ permalink raw reply related
* [PATCH] ARM: dts: Aspeed: Witherspoon: Update BMC partitioning
From: Adriana Kobylak @ 2019-03-12 16:50 UTC (permalink / raw)
To: linux-aspeed
From: "Edward A. James" <eajames@us.ibm.com>
Add simplified partitions for BMC and alternate flash. Include these by
default in Witherspoon.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
.../boot/dts/aspeed-bmc-alt-opp-flash-layout-ubi.dtsi | 18 ++++++++++++++++++
arch/arm/boot/dts/aspeed-bmc-opp-flash-layout-ubi.dtsi | 18 ++++++++++++++++++
arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 3 ++-
3 files changed, 38 insertions(+), 1 deletion(-)
create mode 100644 arch/arm/boot/dts/aspeed-bmc-alt-opp-flash-layout-ubi.dtsi
create mode 100644 arch/arm/boot/dts/aspeed-bmc-opp-flash-layout-ubi.dtsi
diff --git a/arch/arm/boot/dts/aspeed-bmc-alt-opp-flash-layout-ubi.dtsi b/arch/arm/boot/dts/aspeed-bmc-alt-opp-flash-layout-ubi.dtsi
new file mode 100644
index 0000000..9277599
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed-bmc-alt-opp-flash-layout-ubi.dtsi
@@ -0,0 +1,18 @@
+ label = "alt-bmc";
+ partitions {
+ #address-cells = < 1 >;
+ #size-cells = < 1 >;
+ compatible = "fixed-partitions";
+ u-boot at 0 {
+ reg = < 0 0x60000 >;
+ label = "alt-u-boot";
+ };
+ u-boot-env at 60000 {
+ reg = < 0x60000 0x20000 >;
+ label = "alt-u-boot-env";
+ };
+ obmc-ubi at 80000 {
+ reg = < 0x80000 0x1F80000 >;
+ label = "alt-obmc-ubi";
+ };
+ };
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-flash-layout-ubi.dtsi b/arch/arm/boot/dts/aspeed-bmc-opp-flash-layout-ubi.dtsi
new file mode 100644
index 0000000..0059ad1
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-flash-layout-ubi.dtsi
@@ -0,0 +1,18 @@
+ label = "bmc";
+ partitions {
+ #address-cells = < 1 >;
+ #size-cells = < 1 >;
+ compatible = "fixed-partitions";
+ u-boot at 0 {
+ reg = < 0 0x60000 >;
+ label = "u-boot";
+ };
+ u-boot-env at 60000 {
+ reg = < 0x60000 0x20000 >;
+ label = "u-boot-env";
+ };
+ obmc-ubi at 80000 {
+ reg = < 0x80000 0x1F80000 >;
+ label = "obmc-ubi";
+ };
+ };
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
index c51e3e8..058b9b7 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
@@ -191,7 +191,7 @@
label = "bmc";
m25p,fast-read;
spi-max-frequency = <50000000>;
-#include "openbmc-flash-layout.dtsi"
+#include "aspeed-bmc-opp-flash-layout-ubi.dtsi"
};
flash at 1 {
@@ -199,6 +199,7 @@
label = "alt";
m25p,fast-read;
spi-max-frequency = <50000000>;
+#include "aspeed-bmc-alt-opp-flash-layout-ubi.dtsi"
};
};
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/6] drivers/misc: Add Aspeed XDMA engine driver
From: Eddie James @ 2019-03-12 18:46 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAK8P3a2kcaVGXifqrBLt9PfD1n2M6WCBxDDcaF8d7EdYxGRvVg@mail.gmail.com>
On 3/6/19 4:48 AM, Arnd Bergmann wrote:
> On Tue, Mar 5, 2019 at 10:45 PM Eddie James <eajames@linux.ibm.com> wrote:
>> On 3/5/19 2:01 AM, Arnd Bergmann wrote:
>>> On Mon, Mar 4, 2019 at 10:37 PM Eddie James <eajames@linux.ibm.com> wrote:
>>>> The XDMA engine embedded in the AST2500 SOC performs PCI DMA operations
>>>> between the SOC (acting as a BMC) and a host processor in a server.
>>>>
>>>> This commit adds a driver to control the XDMA engine and adds functions
>>>> to initialize the hardware and memory and start DMA operations.
>>>>
>>>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>>> Hi Eddie,
>>>
>>> Thanks for your submission! Overall this looks well-implemented, but
>>> I fear we already have too many ways of doing the same thing at
>>> the moment, and I would hope to avoid adding yet another user space
>>> interface for a specific hardware that does this.
>>>
>>> Your interface appears to be a fairly low-level variant, just doing
>>> single DMA transfers through ioctls, but configuring the PCIe
>>> endpoint over sysfs.
>> Hi, thanks for the quick response!
>>
>> There is actually no PCIe configuration done in this driver. The two
>> sysfs entries control the system control unit (SCU) on the AST2500
>> purely to enable and disable entire PCIe devices. It might be possible
>> to control those devices more finely with a PCI endpoint driver, but
>> there is no need to do so. The XDMA engine does that by itself to
>> perform DMA fairly automatically.
>>
>> If the sysfs entries are really troublesome, we can probably remove
>> those and find another way to control the SCU.
> I think the main advantage of tying this to a PCIe endpoint driver
> is that this would give us a logical object in the kernel that we
> can add the user space interface to, and have the protocol on
> top of it be portable between different SoCs.
>
>>> Please have a look at the drivers/pci/endpoint framework first
>>> and see if you can work on top of that interface instead.
>>> Even if it doesn't quite do what you need here, we may be
>>> able to extend it in a way that works for you, and lets others
>>> use the same user interface extensions in the future.
>>>
>>> It may also be necessary to split out the DMA engine portion
>>> into a regular drivers/dma/ back-end to make that fit in with
>>> the PCIe endpoint framework.
>> Right, I did look into the normal DMA framework. There were a couple of
>> problems. First and foremost, the "device" (really, host processor)
>> address that we use is 64 bit, but the AST2500 is of course 32 bit. So I
>> couldn't find a good way to get the address through the DMA API into the
>> driver. It's entirely possible I missed something there though.
> 32-bit ARM SoCs can be built with a 64-bit dma_addr_t. Would that
> help you here?
Yep, thanks, that's helpful.
>
>> The other issue was that the vast majority of the DMA framework was
>> unused, resulting in a large amount of boilerplate that did nothing
>> except satisfy the API... I thought simplicity would be better in this case.
> Simplicity is important indeed, but we have to weigh it against
> having a consistent interface. What the dmaengine driver would
> give us in combination with the PCIe endpoint driver is that it abstracts
> the hardware from the protocol on top, which could then be done
> in a way that is not specific to an AST2xxx chip.
>
>> Let me know what you think... I could certainly switch to ioctl instead
>> of the write() if that's better. Or if you really think the DMA
>> framework is required here, let me know.
> I don't think that replacing the ioctl() with a write() call specifically
> would make much of a difference here. The question I'd like to
> discuss further is what high-level user space interface you actually
> need in order to implement what kind of functionality. We can then
> look at whether this interface can be implemented on top of a
> PCIe endpoint and a dmaengine driver in a portable way. If all
> of those are true, then I'd definitely go with the modular approach
> of having two standard drivers for the PCIe endpoint (should be
> a trivial wrapper) and the dma engine (not trivial, but there are
> many examples), plus a generic front-end in
> drivers/pci/endpoint/functions/.
Hi Arnd,
Let me describe the top level interface we really need. The objective is
just to transfer arbitrary data between the two memory spaces (memory on
the AST2500 as the BMC, where the driver is running, and the memory on
the host processor). The user on the BMC (in user space; I can't think
of a use case for another driver needing to access this interface) has
the host address, transfer size, and, if it's a write, the data. User
needs to pass this into the driver and, if it's a read, retrieve the
transferred data.
I did start trying to implement the dmaengine framework, and I think it
could technically work. The addressing is no longer a problem, thanks to
your tip. However, I realized there are some other issues.
The main problem is that the only memory that the XDMA engine hardware
can access is the VGA reserved memory area on the AST2xxx. So I don't
see how it can ever be a pure dmaengine driver; it would always need an
additional interface or something to handle that memory area. If I
completed the dmaengine framework, any and all users would be required
to go through an additional step to get memory in the reserved area and
copy in/out of there. The way the driver stands, this memory management
is integrated, resulting in a fairly clean interface, though of course
it is unique.
As for the PCIe endpoint part, I'm not sure it fits this driver. I could
drop the sysfs entries and find another way to configure the SCU for
now... this driver really doesn't have anything to do with PCIe, except
for the fact that the XDMA hardware uses PCIe to do the actual work of
the data transfer.
What do you think? One other thought I had was that the driver might be
more suitable to go in drivers/soc/ as it is very specific to the
AST2xxx. But, up to you.
Thanks,
Eddie
>
> Arnd
>
^ permalink raw reply
* [PATCH] ARM: dts: Aspeed: Witherspoon: Update BMC partitioning
From: Andrew Jeffery @ 2019-03-12 23:21 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1552409402-15881-1-git-send-email-anoo@linux.ibm.com>
On Wed, 13 Mar 2019, at 03:21, Adriana Kobylak wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add simplified partitions for BMC and alternate flash. Include these by
> default in Witherspoon.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
Yes please! Lets apply this so I can stop bashing my head against the desk when doing
kernel work on a Witherspoon.
> ---
> .../boot/dts/aspeed-bmc-alt-opp-flash-layout-ubi.dtsi | 18 ++++++++++++++++++
> arch/arm/boot/dts/aspeed-bmc-opp-flash-layout-ubi.dtsi | 18 ++++++++++++++++++
> arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 3 ++-
> 3 files changed, 38 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/boot/dts/aspeed-bmc-alt-opp-flash-layout-ubi.dtsi
> create mode 100644 arch/arm/boot/dts/aspeed-bmc-opp-flash-layout-ubi.dtsi
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-alt-opp-flash-layout-ubi.dtsi
> b/arch/arm/boot/dts/aspeed-bmc-alt-opp-flash-layout-ubi.dtsi
> new file mode 100644
> index 0000000..9277599
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-alt-opp-flash-layout-ubi.dtsi
> @@ -0,0 +1,18 @@
> + label = "alt-bmc";
> + partitions {
> + #address-cells = < 1 >;
> + #size-cells = < 1 >;
> + compatible = "fixed-partitions";
> + u-boot at 0 {
> + reg = < 0 0x60000 >;
> + label = "alt-u-boot";
> + };
> + u-boot-env at 60000 {
> + reg = < 0x60000 0x20000 >;
> + label = "alt-u-boot-env";
> + };
> + obmc-ubi at 80000 {
> + reg = < 0x80000 0x1F80000 >;
> + label = "alt-obmc-ubi";
> + };
> + };
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-flash-layout-ubi.dtsi
> b/arch/arm/boot/dts/aspeed-bmc-opp-flash-layout-ubi.dtsi
> new file mode 100644
> index 0000000..0059ad1
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-flash-layout-ubi.dtsi
> @@ -0,0 +1,18 @@
> + label = "bmc";
> + partitions {
> + #address-cells = < 1 >;
> + #size-cells = < 1 >;
> + compatible = "fixed-partitions";
> + u-boot at 0 {
> + reg = < 0 0x60000 >;
> + label = "u-boot";
> + };
> + u-boot-env at 60000 {
> + reg = < 0x60000 0x20000 >;
> + label = "u-boot-env";
> + };
> + obmc-ubi at 80000 {
> + reg = < 0x80000 0x1F80000 >;
> + label = "obmc-ubi";
> + };
> + };
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> index c51e3e8..058b9b7 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> @@ -191,7 +191,7 @@
> label = "bmc";
> m25p,fast-read;
> spi-max-frequency = <50000000>;
> -#include "openbmc-flash-layout.dtsi"
> +#include "aspeed-bmc-opp-flash-layout-ubi.dtsi"
> };
>
> flash at 1 {
> @@ -199,6 +199,7 @@
> label = "alt";
> m25p,fast-read;
> spi-max-frequency = <50000000>;
> +#include "aspeed-bmc-alt-opp-flash-layout-ubi.dtsi"
> };
> };
>
> --
> 1.8.3.1
>
>
^ permalink raw reply
* [PATCH v7 1/2] dt-bindings: misc: aspeed-p2a-ctrl: add support
From: Rob Herring @ 2019-03-13 19:54 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190312163051.155405-1-venture@google.com>
On Tue, Mar 12, 2019 at 11:30 AM Patrick Venture <venture@google.com> wrote:
>
> Document the ast2400, ast2500 PCI-to-AHB bridge control driver bindings.
>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
> Changes for v7:
> - Moved node under the syscon node it requires
> Changes for v6:
> - None
> Changes for v5:
> - None
> Changes for v4:
> - None
> Changes for v3:
> - None
> Changes for v2:
> - Added comment about syscon required parameter.
> ---
> .../bindings/misc/aspeed-p2a-ctrl.txt | 48 +++++++++++++++++++
> 1 file changed, 48 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
>
> diff --git a/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> new file mode 100644
> index 000000000000..088cc4e3dc54
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> @@ -0,0 +1,48 @@
> +======================================================================
> +Device tree bindings for Aspeed AST2400/AST2500 PCI-to-AHB Bridge Control Driver
> +======================================================================
> +
> +The bridge is available on platforms with the VGA enabled on the Aspeed device.
> +In this case, the host has access to a 64KiB window into all of the BMC's
> +memory. The BMC can disable this bridge. If the bridge is enabled, the host
> +has read access to all the regions of memory, however the host only has read
> +and write access depending on a register controlled by the BMC.
> +
> +Required properties:
> +===================
> +
> + - compatible: must be one of:
> + - "aspeed,ast2400-p2a-ctrl"
> + - "aspeed,ast2500-p2a-ctrl"
> +
> + - syscon: handle to syscon device node controlling PCI.
> +
> +Optional properties:
> +===================
> +
> +- memory-region: A phandle to a reserved_memory region to be used for the PCI
> + to AHB mapping
> +
> +The p2a-control node should be the child of a syscon node with the required
> +property:
> +
> +- compatible : Should be one of the following:
> + "aspeed,ast2400-scu", "syscon", "simple-mfd"
> + "aspeed,g4-scu", "syscon", "simple-mfd"
> + "aspeed,ast2500-scu", "syscon", "simple-mfd"
> + "aspeed,g5-scu", "syscon", "simple-mfd"
> +
> +Example:
> +
> +g4 Example
> +----------
> +
> +syscon: scu at 1e6e2000 {
> + compatible = "aspeed,ast2400-scu", "syscon", "simple-mfd";
> + reg = <0x1e6e2000 0x1a8>;
> +
> + p2a: p2a-control {
> + compatible = "aspeed,ast2400-p2a-ctrl";
If there's a defined register range, then you could add a reg property
(even though Linux doesn't use it).
Either way,
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH v7 1/2] dt-bindings: misc: aspeed-p2a-ctrl: add support
From: Patrick Venture @ 2019-03-13 20:41 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAL_JsqLRO8m+voL_6mSpcrGmAT-SPwXhXDSOMEgQW6wmmTxZfQ@mail.gmail.com>
On Wed, Mar 13, 2019 at 12:54 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Mar 12, 2019 at 11:30 AM Patrick Venture <venture@google.com> wrote:
> >
> > Document the ast2400, ast2500 PCI-to-AHB bridge control driver bindings.
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> > Changes for v7:
> > - Moved node under the syscon node it requires
> > Changes for v6:
> > - None
> > Changes for v5:
> > - None
> > Changes for v4:
> > - None
> > Changes for v3:
> > - None
> > Changes for v2:
> > - Added comment about syscon required parameter.
> > ---
> > .../bindings/misc/aspeed-p2a-ctrl.txt | 48 +++++++++++++++++++
> > 1 file changed, 48 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> >
> > diff --git a/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> > new file mode 100644
> > index 000000000000..088cc4e3dc54
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/aspeed-p2a-ctrl.txt
> > @@ -0,0 +1,48 @@
> > +======================================================================
> > +Device tree bindings for Aspeed AST2400/AST2500 PCI-to-AHB Bridge Control Driver
> > +======================================================================
> > +
> > +The bridge is available on platforms with the VGA enabled on the Aspeed device.
> > +In this case, the host has access to a 64KiB window into all of the BMC's
> > +memory. The BMC can disable this bridge. If the bridge is enabled, the host
> > +has read access to all the regions of memory, however the host only has read
> > +and write access depending on a register controlled by the BMC.
> > +
> > +Required properties:
> > +===================
> > +
> > + - compatible: must be one of:
> > + - "aspeed,ast2400-p2a-ctrl"
> > + - "aspeed,ast2500-p2a-ctrl"
> > +
> > + - syscon: handle to syscon device node controlling PCI.
> > +
> > +Optional properties:
> > +===================
> > +
> > +- memory-region: A phandle to a reserved_memory region to be used for the PCI
> > + to AHB mapping
> > +
> > +The p2a-control node should be the child of a syscon node with the required
> > +property:
> > +
> > +- compatible : Should be one of the following:
> > + "aspeed,ast2400-scu", "syscon", "simple-mfd"
> > + "aspeed,g4-scu", "syscon", "simple-mfd"
> > + "aspeed,ast2500-scu", "syscon", "simple-mfd"
> > + "aspeed,g5-scu", "syscon", "simple-mfd"
> > +
> > +Example:
> > +
> > +g4 Example
> > +----------
> > +
> > +syscon: scu at 1e6e2000 {
> > + compatible = "aspeed,ast2400-scu", "syscon", "simple-mfd";
> > + reg = <0x1e6e2000 0x1a8>;
> > +
> > + p2a: p2a-control {
> > + compatible = "aspeed,ast2400-p2a-ctrl";
>
> If there's a defined register range, then you could add a reg property
> (even though Linux doesn't use it).
in this case, the device just needs access to two separate registers
controlled by the syscon to which it is now a child.
>
> Either way,
>
> Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [Potential Spoof] Re: [Potential Spoof] Re: [PATCH v2] misc: aspeed-lpc-ctrl: make parameter optional
From: Vijay Khemka @ 2019-03-18 19:44 UTC (permalink / raw)
To: linux-aspeed
Hi Joel,
Can you please apply this patch as " Documentation/devicetree/bindings/mfd/aspeed-lpc.txt" has already been applied
Regards
-Vijay
?On 3/5/19, 4:15 PM, "Linux-aspeed on behalf of Vijay Khemka" <linux-aspeed-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of vijaykhemka@fb.com> wrote:
Joel,
Did this patch apply upstream. Somehow I can't find this patch in linux or linux-next or our obmc dev4.19.
Regards
-Vijay
On 1/17/19, 10:53 AM, "Linux-aspeed on behalf of Vijay Khemka" <linux-aspeed-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of vijaykhemka@fb.com> wrote:
On 1/16/19, 10:17 PM, "Joel Stanley" <joel@jms.id.au> wrote:
On Thu, 17 Jan 2019 at 09:02, Vijay Khemka <vijaykhemka@fb.com> wrote:
>
> Makiing memory-region and flash as optional parameter in device
> tree if user needs to use these parameter through ioctl then
> need to define in devicetree.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
Thanks! This looks okay to me. I tested it on one of our systems which
uses both flash and reserved memory and it was fine.
Reviewed-by: Joel Stanley <joel@jms.id.au>
Can you also send a patch to update the bindings at
Documentation/devicetree/bindings/mfd/aspeed-lpc.txt ? I think the
only change you need to make is to move the memory region and flash
properties to optional (instead of required).
Sure I will do this.
Cheers,
Joel
> ---
> drivers/misc/aspeed-lpc-ctrl.c | 58 +++++++++++++++++++++-------------
> 1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
> index a024f8042259..332210e06e98 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> unsigned long param)
> {
> struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> + struct device *dev = file->private_data;
> void __user *p = (void __user *)param;
> struct aspeed_lpc_ctrl_mapping map;
> u32 addr;
> @@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> if (map.window_id != 0)
> return -EINVAL;
>
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved memory\n");
> + return -EINVAL;
> + }
> +
> map.size = lpc_ctrl->mem_size;
>
> return copy_to_user(p, &map, sizeof(map)) ? -EFAULT : 0;
> @@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> return -EINVAL;
>
> if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
> + if (!lpc_ctrl->pnor_size) {
> + dev_err(dev, "Didn't find host pnor flash\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->pnor_base;
> size = lpc_ctrl->pnor_size;
> } else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved memory\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->mem_base;
> size = lpc_ctrl->mem_size;
> } else {
> @@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
> if (!lpc_ctrl)
> return -ENOMEM;
>
> + /* If flash is described in device tree then store */
> 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_address_to_resource(node, 1, &resm);
> - of_node_put(node);
> - if (rc) {
> - dev_err(dev, "Couldn't address to resource for flash\n");
> - return rc;
> + dev_dbg(dev, "Didn't find host pnor flash node\n");
> + } else {
> + rc = of_address_to_resource(node, 1, &resm);
> + of_node_put(node);
> + if (rc) {
> + dev_err(dev, "Couldn't address to resource for flash\n");
> + return rc;
> + }
> }
>
> lpc_ctrl->pnor_size = resource_size(&resm);
> @@ -214,22 +230,22 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
>
> dev_set_drvdata(&pdev->dev, lpc_ctrl);
>
> + /* If memory-region is described in device tree then store */
> node = of_parse_phandle(dev->of_node, "memory-region", 0);
> if (!node) {
> - dev_err(dev, "Didn't find reserved memory\n");
> - return -EINVAL;
> - }
> + dev_dbg(dev, "Didn't find reserved memory\n");
> + } else {
> + rc = of_address_to_resource(node, 0, &resm);
> + of_node_put(node);
> + if (rc) {
> + dev_err(dev, "Couldn't address to resource for reserved memory\n");
> + return -ENOMEM;
> + }
>
> - rc = of_address_to_resource(node, 0, &resm);
> - of_node_put(node);
> - if (rc) {
> - dev_err(dev, "Couldn't address to resource for reserved memory\n");
> - return -ENOMEM;
> + lpc_ctrl->mem_size = resource_size(&resm);
> + lpc_ctrl->mem_base = resm.start;
> }
>
> - lpc_ctrl->mem_size = resource_size(&resm);
> - lpc_ctrl->mem_base = resm.start;
> -
> lpc_ctrl->regmap = syscon_node_to_regmap(
> pdev->dev.parent->of_node);
> if (IS_ERR(lpc_ctrl->regmap)) {
> @@ -258,8 +274,6 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
> goto err;
> }
>
> - dev_info(dev, "Loaded at %pr\n", &resm);
> -
> return 0;
>
> err:
> --
> 2.17.1
>
^ permalink raw reply
* [Potential Spoof] Re: [PATCH] ARM: dts: aspeed: tiogapass: Enable VUART
From: Vijay Khemka @ 2019-03-18 19:44 UTC (permalink / raw)
To: linux-aspeed
Hi Joel,
Please apply this patch.
Regards
-Vijay
?On 3/5/19, 12:06 PM, "openbmc on behalf of Vijay Khemka" <openbmc-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of vijaykhemka@fb.com> wrote:
Please review below patch.
Regards
-Vijay
On 1/30/19, 10:14 AM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
Enabling vuart for Facebook tiogapass
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 42e0d7a8e8d0..a058fb2985f7 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -64,6 +64,11 @@
status = "okay";
};
+&vuart {
+ // VUART Host Console
+ status = "okay";
+};
+
&uart1 {
// Host Console
status = "okay";
--
2.17.1
^ permalink raw reply
* [PATCH v2] usb: introduce usb_ep_type_string() function
From: Chunfeng Yun @ 2019-03-19 7:44 UTC (permalink / raw)
To: linux-aspeed
In some places, the code prints a human-readable USB endpoint
transfer type (e.g. "Bulk"). This involves a switch statement
sometimes wrapped around in ({ ... }) block leading to code
repetition.
To make this scenario easier, here introduces usb_ep_type_string()
function, which returns a human-readable name of provided
endpoint type.
It also changes a few places switch was used to use this
new function.
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2 changes:
1. keep type string consistent with type_show() used in endpoint.c
---
drivers/usb/common/common.c | 16 ++++++++++++++++
drivers/usb/core/endpoint.c | 18 ++----------------
drivers/usb/core/hcd.c | 17 ++---------------
drivers/usb/dwc3/debugfs.c | 23 ++++-------------------
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 6 +-----
drivers/usb/gadget/udc/dummy_hcd.c | 16 +---------------
drivers/usb/host/xhci-trace.h | 19 ++-----------------
include/linux/usb/ch9.h | 8 ++++++++
8 files changed, 36 insertions(+), 87 deletions(-)
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index 48277bbc15e4..529b01e786fd 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -16,6 +16,22 @@
#include <linux/usb/otg.h>
#include <linux/of_platform.h>
+static const char *const ep_type_names[] = {
+ [USB_ENDPOINT_XFER_CONTROL] = "Control",
+ [USB_ENDPOINT_XFER_ISOC] = "Isoc",
+ [USB_ENDPOINT_XFER_BULK] = "Bulk",
+ [USB_ENDPOINT_XFER_INT] = "Interrupt",
+};
+
+const char *usb_ep_type_string(int ep_type)
+{
+ if (ep_type < 0 || ep_type >= ARRAY_SIZE(ep_type_names))
+ return "unknown";
+
+ return ep_type_names[ep_type];
+}
+EXPORT_SYMBOL_GPL(usb_ep_type_string);
+
const char *usb_otg_state_string(enum usb_otg_state state)
{
static const char *const names[] = {
diff --git a/drivers/usb/core/endpoint.c b/drivers/usb/core/endpoint.c
index 1c2c04079676..afa43f9a47b2 100644
--- a/drivers/usb/core/endpoint.c
+++ b/drivers/usb/core/endpoint.c
@@ -60,23 +60,9 @@ static ssize_t type_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct ep_device *ep = to_ep_device(dev);
- char *type = "unknown";
+ int ep_type = usb_endpoint_type(ep->desc);
- switch (usb_endpoint_type(ep->desc)) {
- case USB_ENDPOINT_XFER_CONTROL:
- type = "Control";
- break;
- case USB_ENDPOINT_XFER_ISOC:
- type = "Isoc";
- break;
- case USB_ENDPOINT_XFER_BULK:
- type = "Bulk";
- break;
- case USB_ENDPOINT_XFER_INT:
- type = "Interrupt";
- break;
- }
- return sprintf(buf, "%s\n", type);
+ return sprintf(buf, "%s\n", usb_ep_type_string(ep_type));
}
static DEVICE_ATTR_RO(type);
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 3189181bb628..0ccf2efeb40b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1878,23 +1878,10 @@ void usb_hcd_flush_endpoint(struct usb_device *udev,
/* kick hcd */
unlink1(hcd, urb, -ESHUTDOWN);
dev_dbg (hcd->self.controller,
- "shutdown urb %pK ep%d%s%s\n",
+ "shutdown urb %pK ep%d%s-%s\n",
urb, usb_endpoint_num(&ep->desc),
is_in ? "in" : "out",
- ({ char *s;
-
- switch (usb_endpoint_type(&ep->desc)) {
- case USB_ENDPOINT_XFER_CONTROL:
- s = ""; break;
- case USB_ENDPOINT_XFER_BULK:
- s = "-bulk"; break;
- case USB_ENDPOINT_XFER_INT:
- s = "-intr"; break;
- default:
- s = "-iso"; break;
- };
- s;
- }));
+ usb_ep_type_string(usb_endpoint_type(&ep->desc)));
usb_put_urb (urb);
/* list contents may have changed */
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 1c792710348f..d9e2a63835fe 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -750,28 +750,13 @@ static int dwc3_transfer_type_show(struct seq_file *s, void *unused)
unsigned long flags;
spin_lock_irqsave(&dwc->lock, flags);
- if (!(dep->flags & DWC3_EP_ENABLED) ||
- !dep->endpoint.desc) {
- seq_printf(s, "--\n");
+ if (!(dep->flags & DWC3_EP_ENABLED) || !dep->endpoint.desc) {
+ seq_puts(s, "unknown\n");
goto out;
}
- switch (usb_endpoint_type(dep->endpoint.desc)) {
- case USB_ENDPOINT_XFER_CONTROL:
- seq_printf(s, "control\n");
- break;
- case USB_ENDPOINT_XFER_ISOC:
- seq_printf(s, "isochronous\n");
- break;
- case USB_ENDPOINT_XFER_BULK:
- seq_printf(s, "bulk\n");
- break;
- case USB_ENDPOINT_XFER_INT:
- seq_printf(s, "interrupt\n");
- break;
- default:
- seq_printf(s, "--\n");
- }
+ seq_printf(s, "%s\n",
+ usb_ep_type_string(usb_endpoint_type(dep->endpoint.desc)));
out:
spin_unlock_irqrestore(&dwc->lock, flags);
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 83340f4fdc6e..35941dc125f9 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -593,10 +593,6 @@ static int ast_vhub_epn_disable(struct usb_ep* u_ep)
static int ast_vhub_epn_enable(struct usb_ep* u_ep,
const struct usb_endpoint_descriptor *desc)
{
- static const char *ep_type_string[] __maybe_unused = { "ctrl",
- "isoc",
- "bulk",
- "intr" };
struct ast_vhub_ep *ep = to_ast_ep(u_ep);
struct ast_vhub_dev *dev;
struct ast_vhub *vhub;
@@ -646,7 +642,7 @@ static int ast_vhub_epn_enable(struct usb_ep* u_ep,
ep->epn.wedged = false;
EPDBG(ep, "Enabling [%s] %s num %d maxpacket=%d\n",
- ep->epn.is_in ? "in" : "out", ep_type_string[type],
+ ep->epn.is_in ? "in" : "out", usb_ep_type_string(type),
usb_endpoint_num(desc), maxpacket);
/* Can we use DMA descriptor mode ? */
diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index baf72f95f0f1..40c6a484e232 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -617,21 +617,7 @@ static int dummy_enable(struct usb_ep *_ep,
_ep->name,
desc->bEndpointAddress & 0x0f,
(desc->bEndpointAddress & USB_DIR_IN) ? "in" : "out",
- ({ char *val;
- switch (usb_endpoint_type(desc)) {
- case USB_ENDPOINT_XFER_BULK:
- val = "bulk";
- break;
- case USB_ENDPOINT_XFER_ISOC:
- val = "iso";
- break;
- case USB_ENDPOINT_XFER_INT:
- val = "intr";
- break;
- default:
- val = "ctrl";
- break;
- } val; }),
+ usb_ep_type_string(usb_endpoint_type(desc)),
max, ep->stream_en ? "enabled" : "disabled");
/* at this point real hardware should be NAKing transfers
diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
index 88b427434bd8..bac73d688f11 100644
--- a/drivers/usb/host/xhci-trace.h
+++ b/drivers/usb/host/xhci-trace.h
@@ -289,23 +289,8 @@ DECLARE_EVENT_CLASS(xhci_log_urb,
),
TP_printk("ep%d%s-%s: urb %p pipe %u slot %d length %d/%d sgs %d/%d stream %d flags %08x",
__entry->epnum, __entry->dir_in ? "in" : "out",
- ({ char *s;
- switch (__entry->type) {
- case USB_ENDPOINT_XFER_INT:
- s = "intr";
- break;
- case USB_ENDPOINT_XFER_CONTROL:
- s = "control";
- break;
- case USB_ENDPOINT_XFER_BULK:
- s = "bulk";
- break;
- case USB_ENDPOINT_XFER_ISOC:
- s = "isoc";
- break;
- default:
- s = "UNKNOWN";
- } s; }), __entry->urb, __entry->pipe, __entry->slot_id,
+ usb_ep_type_string(__entry->type),
+ __entry->urb, __entry->pipe, __entry->slot_id,
__entry->actual, __entry->length, __entry->num_mapped_sgs,
__entry->num_sgs, __entry->stream, __entry->flags
)
diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
index 523aa088f6ab..da82606be605 100644
--- a/include/linux/usb/ch9.h
+++ b/include/linux/usb/ch9.h
@@ -36,6 +36,14 @@
#include <linux/device.h>
#include <uapi/linux/usb/ch9.h>
+/**
+ * usb_ep_type_string() - Returns human readable-name of the endpoint type.
+ * @ep_type: The endpoint type to return human-readable name for. If it's not
+ * any of the types: USB_ENDPOINT_XFER_{CONTROL, ISOC, BULK, INT},
+ * usually got by usb_endpoint_type(), the string 'unknown' will be returned.
+ */
+extern const char *usb_ep_type_string(int ep_type);
+
/**
* usb_speed_string() - Returns human readable-name of the speed.
* @speed: The speed to return human-readable name for. If it's not
--
2.20.1
^ permalink raw reply related
* [PATCH v2] usb: introduce usb_ep_type_string() function
From: Chunfeng Yun @ 2019-03-19 7:54 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <28a8e9f1518e47c35e4563706cbe592b972a7f9d.1552977710.git.chunfeng.yun@mediatek.com>
Hi Felipe & Mathias,
Could you please comment this patch, if there is some-effect on dwc3 and
xhci, I'll remove its change.
Sorry for bothering you
On Tue, 2019-03-19 at 15:44 +0800, Chunfeng Yun wrote:
> In some places, the code prints a human-readable USB endpoint
> transfer type (e.g. "Bulk"). This involves a switch statement
> sometimes wrapped around in ({ ... }) block leading to code
> repetition.
> To make this scenario easier, here introduces usb_ep_type_string()
> function, which returns a human-readable name of provided
> endpoint type.
> It also changes a few places switch was used to use this
> new function.
>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v2 changes:
> 1. keep type string consistent with type_show() used in endpoint.c
> ---
> drivers/usb/common/common.c | 16 ++++++++++++++++
> drivers/usb/core/endpoint.c | 18 ++----------------
> drivers/usb/core/hcd.c | 17 ++---------------
> drivers/usb/dwc3/debugfs.c | 23 ++++-------------------
> drivers/usb/gadget/udc/aspeed-vhub/epn.c | 6 +-----
> drivers/usb/gadget/udc/dummy_hcd.c | 16 +---------------
> drivers/usb/host/xhci-trace.h | 19 ++-----------------
> include/linux/usb/ch9.h | 8 ++++++++
> 8 files changed, 36 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> index 48277bbc15e4..529b01e786fd 100644
> --- a/drivers/usb/common/common.c
> +++ b/drivers/usb/common/common.c
> @@ -16,6 +16,22 @@
> #include <linux/usb/otg.h>
> #include <linux/of_platform.h>
>
> +static const char *const ep_type_names[] = {
> + [USB_ENDPOINT_XFER_CONTROL] = "Control",
> + [USB_ENDPOINT_XFER_ISOC] = "Isoc",
> + [USB_ENDPOINT_XFER_BULK] = "Bulk",
> + [USB_ENDPOINT_XFER_INT] = "Interrupt",
> +};
> +
> +const char *usb_ep_type_string(int ep_type)
> +{
> + if (ep_type < 0 || ep_type >= ARRAY_SIZE(ep_type_names))
> + return "unknown";
> +
> + return ep_type_names[ep_type];
> +}
> +EXPORT_SYMBOL_GPL(usb_ep_type_string);
> +
> const char *usb_otg_state_string(enum usb_otg_state state)
> {
> static const char *const names[] = {
> diff --git a/drivers/usb/core/endpoint.c b/drivers/usb/core/endpoint.c
> index 1c2c04079676..afa43f9a47b2 100644
> --- a/drivers/usb/core/endpoint.c
> +++ b/drivers/usb/core/endpoint.c
> @@ -60,23 +60,9 @@ static ssize_t type_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> struct ep_device *ep = to_ep_device(dev);
> - char *type = "unknown";
> + int ep_type = usb_endpoint_type(ep->desc);
>
> - switch (usb_endpoint_type(ep->desc)) {
> - case USB_ENDPOINT_XFER_CONTROL:
> - type = "Control";
> - break;
> - case USB_ENDPOINT_XFER_ISOC:
> - type = "Isoc";
> - break;
> - case USB_ENDPOINT_XFER_BULK:
> - type = "Bulk";
> - break;
> - case USB_ENDPOINT_XFER_INT:
> - type = "Interrupt";
> - break;
> - }
> - return sprintf(buf, "%s\n", type);
> + return sprintf(buf, "%s\n", usb_ep_type_string(ep_type));
> }
> static DEVICE_ATTR_RO(type);
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 3189181bb628..0ccf2efeb40b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1878,23 +1878,10 @@ void usb_hcd_flush_endpoint(struct usb_device *udev,
> /* kick hcd */
> unlink1(hcd, urb, -ESHUTDOWN);
> dev_dbg (hcd->self.controller,
> - "shutdown urb %pK ep%d%s%s\n",
> + "shutdown urb %pK ep%d%s-%s\n",
> urb, usb_endpoint_num(&ep->desc),
> is_in ? "in" : "out",
> - ({ char *s;
> -
> - switch (usb_endpoint_type(&ep->desc)) {
> - case USB_ENDPOINT_XFER_CONTROL:
> - s = ""; break;
> - case USB_ENDPOINT_XFER_BULK:
> - s = "-bulk"; break;
> - case USB_ENDPOINT_XFER_INT:
> - s = "-intr"; break;
> - default:
> - s = "-iso"; break;
> - };
> - s;
> - }));
> + usb_ep_type_string(usb_endpoint_type(&ep->desc)));
> usb_put_urb (urb);
>
> /* list contents may have changed */
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index 1c792710348f..d9e2a63835fe 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -750,28 +750,13 @@ static int dwc3_transfer_type_show(struct seq_file *s, void *unused)
> unsigned long flags;
>
> spin_lock_irqsave(&dwc->lock, flags);
> - if (!(dep->flags & DWC3_EP_ENABLED) ||
> - !dep->endpoint.desc) {
> - seq_printf(s, "--\n");
> + if (!(dep->flags & DWC3_EP_ENABLED) || !dep->endpoint.desc) {
> + seq_puts(s, "unknown\n");
> goto out;
> }
>
> - switch (usb_endpoint_type(dep->endpoint.desc)) {
> - case USB_ENDPOINT_XFER_CONTROL:
> - seq_printf(s, "control\n");
> - break;
> - case USB_ENDPOINT_XFER_ISOC:
> - seq_printf(s, "isochronous\n");
> - break;
> - case USB_ENDPOINT_XFER_BULK:
> - seq_printf(s, "bulk\n");
> - break;
> - case USB_ENDPOINT_XFER_INT:
> - seq_printf(s, "interrupt\n");
> - break;
> - default:
> - seq_printf(s, "--\n");
> - }
> + seq_printf(s, "%s\n",
> + usb_ep_type_string(usb_endpoint_type(dep->endpoint.desc)));
>
> out:
> spin_unlock_irqrestore(&dwc->lock, flags);
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> index 83340f4fdc6e..35941dc125f9 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> @@ -593,10 +593,6 @@ static int ast_vhub_epn_disable(struct usb_ep* u_ep)
> static int ast_vhub_epn_enable(struct usb_ep* u_ep,
> const struct usb_endpoint_descriptor *desc)
> {
> - static const char *ep_type_string[] __maybe_unused = { "ctrl",
> - "isoc",
> - "bulk",
> - "intr" };
> struct ast_vhub_ep *ep = to_ast_ep(u_ep);
> struct ast_vhub_dev *dev;
> struct ast_vhub *vhub;
> @@ -646,7 +642,7 @@ static int ast_vhub_epn_enable(struct usb_ep* u_ep,
> ep->epn.wedged = false;
>
> EPDBG(ep, "Enabling [%s] %s num %d maxpacket=%d\n",
> - ep->epn.is_in ? "in" : "out", ep_type_string[type],
> + ep->epn.is_in ? "in" : "out", usb_ep_type_string(type),
> usb_endpoint_num(desc), maxpacket);
>
> /* Can we use DMA descriptor mode ? */
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
> index baf72f95f0f1..40c6a484e232 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -617,21 +617,7 @@ static int dummy_enable(struct usb_ep *_ep,
> _ep->name,
> desc->bEndpointAddress & 0x0f,
> (desc->bEndpointAddress & USB_DIR_IN) ? "in" : "out",
> - ({ char *val;
> - switch (usb_endpoint_type(desc)) {
> - case USB_ENDPOINT_XFER_BULK:
> - val = "bulk";
> - break;
> - case USB_ENDPOINT_XFER_ISOC:
> - val = "iso";
> - break;
> - case USB_ENDPOINT_XFER_INT:
> - val = "intr";
> - break;
> - default:
> - val = "ctrl";
> - break;
> - } val; }),
> + usb_ep_type_string(usb_endpoint_type(desc)),
> max, ep->stream_en ? "enabled" : "disabled");
>
> /* at this point real hardware should be NAKing transfers
> diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
> index 88b427434bd8..bac73d688f11 100644
> --- a/drivers/usb/host/xhci-trace.h
> +++ b/drivers/usb/host/xhci-trace.h
> @@ -289,23 +289,8 @@ DECLARE_EVENT_CLASS(xhci_log_urb,
> ),
> TP_printk("ep%d%s-%s: urb %p pipe %u slot %d length %d/%d sgs %d/%d stream %d flags %08x",
> __entry->epnum, __entry->dir_in ? "in" : "out",
> - ({ char *s;
> - switch (__entry->type) {
> - case USB_ENDPOINT_XFER_INT:
> - s = "intr";
> - break;
> - case USB_ENDPOINT_XFER_CONTROL:
> - s = "control";
> - break;
> - case USB_ENDPOINT_XFER_BULK:
> - s = "bulk";
> - break;
> - case USB_ENDPOINT_XFER_ISOC:
> - s = "isoc";
> - break;
> - default:
> - s = "UNKNOWN";
> - } s; }), __entry->urb, __entry->pipe, __entry->slot_id,
> + usb_ep_type_string(__entry->type),
> + __entry->urb, __entry->pipe, __entry->slot_id,
> __entry->actual, __entry->length, __entry->num_mapped_sgs,
> __entry->num_sgs, __entry->stream, __entry->flags
> )
> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
> index 523aa088f6ab..da82606be605 100644
> --- a/include/linux/usb/ch9.h
> +++ b/include/linux/usb/ch9.h
> @@ -36,6 +36,14 @@
> #include <linux/device.h>
> #include <uapi/linux/usb/ch9.h>
>
> +/**
> + * usb_ep_type_string() - Returns human readable-name of the endpoint type.
> + * @ep_type: The endpoint type to return human-readable name for. If it's not
> + * any of the types: USB_ENDPOINT_XFER_{CONTROL, ISOC, BULK, INT},
> + * usually got by usb_endpoint_type(), the string 'unknown' will be returned.
> + */
> +extern const char *usb_ep_type_string(int ep_type);
> +
> /**
> * usb_speed_string() - Returns human readable-name of the speed.
> * @speed: The speed to return human-readable name for. If it's not
^ permalink raw reply
* [PATCH v2] usb: introduce usb_ep_type_string() function
From: Greg Kroah-Hartman @ 2019-03-19 9:23 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1552982059.10179.143.camel@mhfsdcap03>
On Tue, Mar 19, 2019 at 03:54:19PM +0800, Chunfeng Yun wrote:
>
> Hi Felipe & Mathias,
>
> Could you please comment this patch, if there is some-effect on dwc3 and
> xhci, I'll remove its change.
To be specific, you are changing the string values for things that might
care about it and are seen by userspace.
Please be explicit about this, as it could cause problems.
greg k-h
^ permalink raw reply
* [PATCH v2] usb: introduce usb_ep_type_string() function
From: Chunfeng Yun @ 2019-03-20 1:16 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190319092331.GA21652@kroah.com>
Hi Greg,
On Tue, 2019-03-19 at 10:23 +0100, Greg Kroah-Hartman wrote:
> On Tue, Mar 19, 2019 at 03:54:19PM +0800, Chunfeng Yun wrote:
> >
> > Hi Felipe & Mathias,
> >
> > Could you please comment this patch, if there is some-effect on dwc3 and
> > xhci, I'll remove its change.
typo: some-effect/side-effect
>
> To be specific, you are changing the string values for things that might
> care about it and are seen by userspace.
>
> Please be explicit about this, as it could cause problems.
Thank you for making it more clearer
>
> greg k-h
^ permalink raw reply
* [PATCH v2] usb: introduce usb_ep_type_string() function
From: Felipe Balbi @ 2019-03-20 6:55 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1553044567.10179.148.camel@mhfsdcap03>
Chunfeng Yun <chunfeng.yun@mediatek.com> writes:
> Hi Greg,
>
> On Tue, 2019-03-19 at 10:23 +0100, Greg Kroah-Hartman wrote:
>> On Tue, Mar 19, 2019 at 03:54:19PM +0800, Chunfeng Yun wrote:
>> >
>> > Hi Felipe & Mathias,
>> >
>> > Could you please comment this patch, if there is some-effect on dwc3 and
>> > xhci, I'll remove its change.
> typo: some-effect/side-effect
well, we don't really know if there are tools parsing the output. The
side-effect _does_ exist (i.e. you change the string :-), what we don't
know is if this will cause problems to possibly existing tools.
Personally, I don't mind the patch, but if it breaks existing parsers,
that would be a little annoying.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20190320/7719f1d8/attachment.sig>
^ permalink raw reply
* [PATCH v2] usb: introduce usb_ep_type_string() function
From: Chunfeng Yun @ 2019-03-20 7:49 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <87r2b2ujao.fsf@linux.intel.com>
Hi Felipe,
On Wed, 2019-03-20 at 08:55 +0200, Felipe Balbi wrote:
> Chunfeng Yun <chunfeng.yun@mediatek.com> writes:
>
> > Hi Greg,
> >
> > On Tue, 2019-03-19 at 10:23 +0100, Greg Kroah-Hartman wrote:
> >> On Tue, Mar 19, 2019 at 03:54:19PM +0800, Chunfeng Yun wrote:
> >> >
> >> > Hi Felipe & Mathias,
> >> >
> >> > Could you please comment this patch, if there is some-effect on dwc3 and
> >> > xhci, I'll remove its change.
> > typo: some-effect/side-effect
>
> well, we don't really know if there are tools parsing the output. The
> side-effect _does_ exist (i.e. you change the string :-), what we don't
> know is if this will cause problems to possibly existing tools.
>
> Personally, I don't mind the patch, but if it breaks existing parsers,
> that would be a little annoying.
Thank you for the feedback.
I'll abandon the changes about dwc3 & xhci
>
^ permalink raw reply
* [v3 PATCH] usb: introduce usb_ep_type_string() function
From: Chunfeng Yun @ 2019-03-21 2:27 UTC (permalink / raw)
To: linux-aspeed
In some places, the code prints a human-readable USB endpoint
transfer type (e.g. "bulk"). This involves a switch statement
sometimes wrapped around in ({ ... }) block leading to code
repetition.
To make this scenario easier, here introduces usb_ep_type_string()
function, which returns a human-readable name of provided
endpoint type.
It also changes a few places switch was used to use this
new function.
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2 changes:
1. keep type string consistent with type_show() used in endpoint.c
v3 changes:
1. abandon changes that will break user/kernel apis
---
drivers/usb/common/common.c | 16 ++++++++++++++++
drivers/usb/core/hcd.c | 17 ++---------------
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 6 +-----
drivers/usb/gadget/udc/dummy_hcd.c | 16 +---------------
include/linux/usb/ch9.h | 8 ++++++++
5 files changed, 28 insertions(+), 35 deletions(-)
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index 48277bbc15e4..2174dd9ec176 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -16,6 +16,22 @@
#include <linux/usb/otg.h>
#include <linux/of_platform.h>
+static const char *const ep_type_names[] = {
+ [USB_ENDPOINT_XFER_CONTROL] = "ctrl",
+ [USB_ENDPOINT_XFER_ISOC] = "isoc",
+ [USB_ENDPOINT_XFER_BULK] = "bulk",
+ [USB_ENDPOINT_XFER_INT] = "intr",
+};
+
+const char *usb_ep_type_string(int ep_type)
+{
+ if (ep_type < 0 || ep_type >= ARRAY_SIZE(ep_type_names))
+ return "unknown";
+
+ return ep_type_names[ep_type];
+}
+EXPORT_SYMBOL_GPL(usb_ep_type_string);
+
const char *usb_otg_state_string(enum usb_otg_state state)
{
static const char *const names[] = {
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 3189181bb628..0ccf2efeb40b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1878,23 +1878,10 @@ void usb_hcd_flush_endpoint(struct usb_device *udev,
/* kick hcd */
unlink1(hcd, urb, -ESHUTDOWN);
dev_dbg (hcd->self.controller,
- "shutdown urb %pK ep%d%s%s\n",
+ "shutdown urb %pK ep%d%s-%s\n",
urb, usb_endpoint_num(&ep->desc),
is_in ? "in" : "out",
- ({ char *s;
-
- switch (usb_endpoint_type(&ep->desc)) {
- case USB_ENDPOINT_XFER_CONTROL:
- s = ""; break;
- case USB_ENDPOINT_XFER_BULK:
- s = "-bulk"; break;
- case USB_ENDPOINT_XFER_INT:
- s = "-intr"; break;
- default:
- s = "-iso"; break;
- };
- s;
- }));
+ usb_ep_type_string(usb_endpoint_type(&ep->desc)));
usb_put_urb (urb);
/* list contents may have changed */
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 83340f4fdc6e..35941dc125f9 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -593,10 +593,6 @@ static int ast_vhub_epn_disable(struct usb_ep* u_ep)
static int ast_vhub_epn_enable(struct usb_ep* u_ep,
const struct usb_endpoint_descriptor *desc)
{
- static const char *ep_type_string[] __maybe_unused = { "ctrl",
- "isoc",
- "bulk",
- "intr" };
struct ast_vhub_ep *ep = to_ast_ep(u_ep);
struct ast_vhub_dev *dev;
struct ast_vhub *vhub;
@@ -646,7 +642,7 @@ static int ast_vhub_epn_enable(struct usb_ep* u_ep,
ep->epn.wedged = false;
EPDBG(ep, "Enabling [%s] %s num %d maxpacket=%d\n",
- ep->epn.is_in ? "in" : "out", ep_type_string[type],
+ ep->epn.is_in ? "in" : "out", usb_ep_type_string(type),
usb_endpoint_num(desc), maxpacket);
/* Can we use DMA descriptor mode ? */
diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index baf72f95f0f1..40c6a484e232 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -617,21 +617,7 @@ static int dummy_enable(struct usb_ep *_ep,
_ep->name,
desc->bEndpointAddress & 0x0f,
(desc->bEndpointAddress & USB_DIR_IN) ? "in" : "out",
- ({ char *val;
- switch (usb_endpoint_type(desc)) {
- case USB_ENDPOINT_XFER_BULK:
- val = "bulk";
- break;
- case USB_ENDPOINT_XFER_ISOC:
- val = "iso";
- break;
- case USB_ENDPOINT_XFER_INT:
- val = "intr";
- break;
- default:
- val = "ctrl";
- break;
- } val; }),
+ usb_ep_type_string(usb_endpoint_type(desc)),
max, ep->stream_en ? "enabled" : "disabled");
/* at this point real hardware should be NAKing transfers
diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
index 523aa088f6ab..da82606be605 100644
--- a/include/linux/usb/ch9.h
+++ b/include/linux/usb/ch9.h
@@ -36,6 +36,14 @@
#include <linux/device.h>
#include <uapi/linux/usb/ch9.h>
+/**
+ * usb_ep_type_string() - Returns human readable-name of the endpoint type.
+ * @ep_type: The endpoint type to return human-readable name for. If it's not
+ * any of the types: USB_ENDPOINT_XFER_{CONTROL, ISOC, BULK, INT},
+ * usually got by usb_endpoint_type(), the string 'unknown' will be returned.
+ */
+extern const char *usb_ep_type_string(int ep_type);
+
/**
* usb_speed_string() - Returns human readable-name of the speed.
* @speed: The speed to return human-readable name for. If it's not
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox