* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-10 2:47 ` Joel Stanley
0 siblings, 0 replies; 42+ messages in thread
From: Joel Stanley @ 2020-02-10 2:47 UTC (permalink / raw)
To: Tao Ren, Benjamin Herrenschmidt
Cc: Mark Rutland, Felipe Balbi, linux-aspeed, devicetree,
Andrew Jeffery, Greg Kroah-Hartman, OpenBMC Maillist, linux-usb,
Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
Chunfeng Yun, Colin Ian King, Linux ARM
On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
>
> From: Tao Ren <rentao.bupt@gmail.com>
>
> The patch moves hardcoded vhub attributes (maximum downstream ports and
> generic endpoints) to "ast_vhub_config" structure which is attached to
> struct of_device_id. The major purpose is to add AST2600 vhub support
> because AST2600 vhub provides more downstream ports and endpoints.
>
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
This looks generally okay. We should wait for Ben's ack before applying.
Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---
> drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--------
> drivers/usb/gadget/udc/aspeed-vhub/dev.c | 30 +++++--
> drivers/usb/gadget/udc/aspeed-vhub/epn.c | 4 +-
> drivers/usb/gadget/udc/aspeed-vhub/hub.c | 26 ++++--
> drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 23 ++---
> 5 files changed, 112 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> index 90b134d5dca9..94081cc04113 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> @@ -32,6 +32,29 @@
>
> #include "vhub.h"
>
> +struct ast_vhub_config {
> + u32 max_ports; /* max number of downstream ports */
> + u32 max_epns; /* max number of generic endpoints */
> +};
> +
> +static const struct ast_vhub_config ast2400_config = {
> + .max_ports = 5,
> + .max_epns = 15,
> +};
> +
> +static const struct of_device_id ast_vhub_dt_ids[] = {
> + {
> + .compatible = "aspeed,ast2400-usb-vhub",
> + .data = &ast2400_config,
> + },
> + {
> + .compatible = "aspeed,ast2500-usb-vhub",
> + .data = &ast2400_config,
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> +
> void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req *req,
> int status)
> {
> @@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> {
> struct ast_vhub *vhub = data;
> irqreturn_t iret = IRQ_NONE;
> - u32 istat;
> + u32 i, istat;
>
> /* Stale interrupt while tearing down */
> if (!vhub->ep0_bufs)
> @@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>
> /* Handle generic EPs first */
> if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> - u32 i, ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
> + u32 ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
> writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
>
> - for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs; i++) {
> + for (i = 0; ep_acks && i < vhub->max_epns; i++) {
> u32 mask = VHUB_EP_IRQ(i);
> if (ep_acks & mask) {
> ast_vhub_epn_ack_irq(&vhub->epns[i]);
> @@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> }
>
> /* Handle device interrupts */
> - if (istat & (VHUB_IRQ_DEVICE1 |
> - VHUB_IRQ_DEVICE2 |
> - VHUB_IRQ_DEVICE3 |
> - VHUB_IRQ_DEVICE4 |
> - VHUB_IRQ_DEVICE5)) {
> - if (istat & VHUB_IRQ_DEVICE1)
> - ast_vhub_dev_irq(&vhub->ports[0].dev);
> - if (istat & VHUB_IRQ_DEVICE2)
> - ast_vhub_dev_irq(&vhub->ports[1].dev);
> - if (istat & VHUB_IRQ_DEVICE3)
> - ast_vhub_dev_irq(&vhub->ports[2].dev);
> - if (istat & VHUB_IRQ_DEVICE4)
> - ast_vhub_dev_irq(&vhub->ports[3].dev);
> - if (istat & VHUB_IRQ_DEVICE5)
> - ast_vhub_dev_irq(&vhub->ports[4].dev);
> + for (i = 0; i < vhub->max_ports; i++) {
> + u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
> +
> + if (istat & dev_mask)
> + ast_vhub_dev_irq(&vhub->ports[i].dev);
> }
>
> /* Handle top-level vHub EP0 interrupts */
> @@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>
> void ast_vhub_init_hw(struct ast_vhub *vhub)
> {
> - u32 ctrl;
> + u32 ctrl, port_mask, epn_mask;
>
> UDCDBG(vhub,"(Re)Starting HW ...\n");
>
> @@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
> }
>
> /* Reset all devices */
> - writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
> + port_mask = GENMASK(vhub->max_ports, 1);
> + writel(VHUB_SW_RESET_ROOT_HUB |
> + VHUB_SW_RESET_DMA_CONTROLLER |
> + VHUB_SW_RESET_EP_POOL |
> + port_mask, vhub->regs + AST_VHUB_SW_RESET);
> udelay(1);
> writel(0, vhub->regs + AST_VHUB_SW_RESET);
>
> /* Disable and cleanup EP ACK/NACK interrupts */
> + epn_mask = GENMASK(vhub->max_epns - 1, 0);
> writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
> writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
> - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
> + writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
> + writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
>
> /* Default settings for EP0, enable HW hub EP1 */
> writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> @@ -273,7 +291,7 @@ static int ast_vhub_remove(struct platform_device *pdev)
> return 0;
>
> /* Remove devices */
> - for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> + for (i = 0; i < vhub->max_ports; i++)
> ast_vhub_del_dev(&vhub->ports[i].dev);
>
> spin_lock_irqsave(&vhub->lock, flags);
> @@ -295,7 +313,7 @@ static int ast_vhub_remove(struct platform_device *pdev)
> if (vhub->ep0_bufs)
> dma_free_coherent(&pdev->dev,
> AST_VHUB_EP0_MAX_PACKET *
> - (AST_VHUB_NUM_PORTS + 1),
> + (vhub->max_ports + 1),
> vhub->ep0_bufs,
> vhub->ep0_bufs_dma);
> vhub->ep0_bufs = NULL;
> @@ -309,11 +327,30 @@ static int ast_vhub_probe(struct platform_device *pdev)
> struct ast_vhub *vhub;
> struct resource *res;
> int i, rc = 0;
> + const struct of_device_id *ofdid;
> + const struct ast_vhub_config *config;
>
> vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
> if (!vhub)
> return -ENOMEM;
>
> + ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
> + if (!ofdid)
> + return -EINVAL;
> + config = ofdid->data;
> +
> + vhub->max_ports = config->max_ports;
> + vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
> + sizeof(*vhub->ports), GFP_KERNEL);
> + if (!vhub->ports)
> + return -ENOMEM;
> +
> + vhub->max_epns = config->max_epns;
> + vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
> + sizeof(*vhub->epns), GFP_KERNEL);
> + if (!vhub->epns)
> + return -ENOMEM;
> +
> spin_lock_init(&vhub->lock);
> vhub->pdev = pdev;
>
> @@ -366,7 +403,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
> */
> vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
> AST_VHUB_EP0_MAX_PACKET *
> - (AST_VHUB_NUM_PORTS + 1),
> + (vhub->max_ports + 1),
> &vhub->ep0_bufs_dma, GFP_KERNEL);
> if (!vhub->ep0_bufs) {
> dev_err(&pdev->dev, "Failed to allocate EP0 DMA buffers\n");
> @@ -380,7 +417,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
> ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
>
> /* Init devices */
> - for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
> + for (i = 0; i < vhub->max_ports && rc == 0; i++)
> rc = ast_vhub_init_dev(vhub, i);
> if (rc)
> goto err;
> @@ -400,17 +437,6 @@ static int ast_vhub_probe(struct platform_device *pdev)
> return rc;
> }
>
> -static const struct of_device_id ast_vhub_dt_ids[] = {
> - {
> - .compatible = "aspeed,ast2400-usb-vhub",
> - },
> - {
> - .compatible = "aspeed,ast2500-usb-vhub",
> - },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> -
> static struct platform_driver ast_vhub_driver = {
> .probe = ast_vhub_probe,
> .remove = ast_vhub_remove,
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> index 4008e7a51188..d268306a7bfe 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> @@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct ast_vhub_dev *d)
> writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
>
> /* Clear stall on all EPs */
> - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> + for (i = 0; i < d->max_epns; i++) {
> struct ast_vhub_ep *ep = d->epns[i];
>
> if (ep && (ep->epn.stalled || ep->epn.wedged)) {
> @@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct ast_vhub_dev *d,
> is_set ? "SET" : "CLEAR", ep_num, wValue);
> if (ep_num == 0)
> return std_req_complete;
> - if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
> + if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
> return std_req_stall;
> if (wValue != USB_ENDPOINT_HALT)
> return std_req_driver;
> @@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct ast_vhub_dev *d,
>
> DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
>
> - if (ep_num >= AST_VHUB_NUM_GEN_EPs)
> + if (ep_num >= d->max_epns)
> return std_req_stall;
> if (ep_num != 0) {
> ep = d->epns[ep_num - 1];
> @@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct ast_vhub_dev *d)
> {
> unsigned int i;
>
> - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> + for (i = 0; i < d->max_epns; i++) {
> if (!d->epns[i])
> continue;
> ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
> @@ -416,10 +416,10 @@ static struct usb_ep *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
> * that will allow the generic code to use our
> * assigned address.
> */
> - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> + for (i = 0; i < d->max_epns; i++)
> if (d->epns[i] == NULL)
> break;
> - if (i >= AST_VHUB_NUM_GEN_EPs)
> + if (i >= d->max_epns)
> return NULL;
> addr = i + 1;
>
> @@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
>
> usb_del_gadget_udc(&d->gadget);
> device_unregister(d->port_dev);
> + kfree(d->epns);
> }
>
> static void ast_vhub_dev_release(struct device *dev)
> @@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub, unsigned int idx)
>
> ast_vhub_init_ep0(vhub, &d->ep0, d);
>
> + /*
> + * A USB device can have up to 30 endpoints besides control
> + * endpoint 0.
> + */
> + d->max_epns = min_t(u32, vhub->max_epns, 30);
> + d->epns = kcalloc(d->max_epns, sizeof(*d->epns), GFP_KERNEL);
> + if (!d->epns)
> + return -ENOMEM;
> +
> /*
> * The UDC core really needs us to have separate and uniquely
> * named "parent" devices for each port so we create a sub device
> * here for that purpose
> */
> d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> - if (!d->port_dev)
> - return -ENOMEM;
> + if (!d->port_dev) {
> + rc = -ENOMEM;
> + goto fail_alloc;
> + }
> device_initialize(d->port_dev);
> d->port_dev->release = ast_vhub_dev_release;
> d->port_dev->parent = parent;
> @@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub, unsigned int idx)
> device_del(d->port_dev);
> fail_add:
> put_device(d->port_dev);
> + fail_alloc:
> + kfree(d->epns);
>
> return rc;
> }
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> index 7475c74aa5c5..0bd6b20435b8 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> @@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct ast_vhub_dev *d, u8 addr)
>
> /* Find a free one (no device) */
> spin_lock_irqsave(&vhub->lock, flags);
> - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> + for (i = 0; i < vhub->max_epns; i++)
> if (vhub->epns[i].dev == NULL)
> break;
> - if (i >= AST_VHUB_NUM_GEN_EPs) {
> + if (i >= vhub->max_epns) {
> spin_unlock_irqrestore(&vhub->lock, flags);
> return NULL;
> }
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> index 19b3517e04c0..9c7e57fbd8ef 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> @@ -133,10 +133,13 @@ static const struct ast_vhub_full_cdesc {
>
> #define AST_VHUB_HUB_DESC_SIZE (USB_DT_HUB_NONVAR_SIZE + 2)
>
> -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> +/*
> + * "bNbrPorts" field is updated in "ast_vhub_init_hub" function based on
> + * "max_ports" of the vhub.
> + */
> +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> .bDescLength = AST_VHUB_HUB_DESC_SIZE,
> .bDescriptorType = USB_DT_HUB,
> - .bNbrPorts = AST_VHUB_NUM_PORTS,
> .wHubCharacteristics = cpu_to_le16(HUB_CHAR_NO_LPSM),
> .bPwrOn2PwrGood = 10,
> .bHubContrCurrent = 0,
> @@ -504,7 +507,7 @@ static void ast_vhub_wake_work(struct work_struct *work)
> * we let the normal host wake path deal with it later.
> */
> spin_lock_irqsave(&vhub->lock, flags);
> - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> + for (i = 0; i < vhub->max_ports; i++) {
> struct ast_vhub_port *p = &vhub->ports[i];
>
> if (!(p->status & USB_PORT_STAT_SUSPEND))
> @@ -587,7 +590,7 @@ static enum std_req_rc ast_vhub_set_port_feature(struct ast_vhub_ep *ep,
> struct ast_vhub *vhub = ep->vhub;
> struct ast_vhub_port *p;
>
> - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> + if (port == 0 || port > vhub->max_ports)
> return std_req_stall;
> port--;
> p = &vhub->ports[port];
> @@ -630,7 +633,7 @@ static enum std_req_rc ast_vhub_clr_port_feature(struct ast_vhub_ep *ep,
> struct ast_vhub *vhub = ep->vhub;
> struct ast_vhub_port *p;
>
> - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> + if (port == 0 || port > vhub->max_ports)
> return std_req_stall;
> port--;
> p = &vhub->ports[port];
> @@ -676,7 +679,7 @@ static enum std_req_rc ast_vhub_get_port_stat(struct ast_vhub_ep *ep,
> struct ast_vhub *vhub = ep->vhub;
> u16 stat, chg;
>
> - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> + if (port == 0 || port > vhub->max_ports)
> return std_req_stall;
> port--;
>
> @@ -757,7 +760,7 @@ void ast_vhub_hub_suspend(struct ast_vhub *vhub)
> * Forward to unsuspended ports without changing
> * their connection status.
> */
> - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> + for (i = 0; i < vhub->max_ports; i++) {
> struct ast_vhub_port *p = &vhub->ports[i];
>
> if (!(p->status & USB_PORT_STAT_SUSPEND))
> @@ -780,7 +783,7 @@ void ast_vhub_hub_resume(struct ast_vhub *vhub)
> * Forward to unsuspended ports without changing
> * their connection status.
> */
> - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> + for (i = 0; i < vhub->max_ports; i++) {
> struct ast_vhub_port *p = &vhub->ports[i];
>
> if (!(p->status & USB_PORT_STAT_SUSPEND))
> @@ -814,7 +817,7 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
> * Clear all port status, disable gadgets and "suspend"
> * them. They will be woken up by a port reset.
> */
> - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> + for (i = 0; i < vhub->max_ports; i++) {
> struct ast_vhub_port *p = &vhub->ports[i];
>
> /* Only keep the connected flag */
> @@ -838,5 +841,10 @@ void ast_vhub_init_hub(struct ast_vhub *vhub)
> {
> vhub->speed = USB_SPEED_UNKNOWN;
> INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
> +
> + /*
> + * Fixup number of ports in hub descriptor.
> + */
> + ast_vhub_hub_desc.bNbrPorts = vhub->max_ports;
> }
>
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> index 761919e220d3..e46980fe66f2 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> @@ -76,17 +76,9 @@
> #define VHUB_SW_RESET_DEVICE2 (1 << 2)
> #define VHUB_SW_RESET_DEVICE1 (1 << 1)
> #define VHUB_SW_RESET_ROOT_HUB (1 << 0)
> -#define VHUB_SW_RESET_ALL (VHUB_SW_RESET_EP_POOL | \
> - VHUB_SW_RESET_DMA_CONTROLLER | \
> - VHUB_SW_RESET_DEVICE5 | \
> - VHUB_SW_RESET_DEVICE4 | \
> - VHUB_SW_RESET_DEVICE3 | \
> - VHUB_SW_RESET_DEVICE2 | \
> - VHUB_SW_RESET_DEVICE1 | \
> - VHUB_SW_RESET_ROOT_HUB)
> +
> /* EP ACK/NACK IRQ masks */
> #define VHUB_EP_IRQ(n) (1 << (n))
> -#define VHUB_EP_IRQ_ALL 0x7fff /* 15 EPs */
>
> /* USB status reg */
> #define VHUB_USBSTS_HISPEED (1 << 27)
> @@ -210,8 +202,6 @@
> * *
> ****************************************/
>
> -#define AST_VHUB_NUM_GEN_EPs 15 /* Generic non-0 EPs */
> -#define AST_VHUB_NUM_PORTS 5 /* vHub ports */
> #define AST_VHUB_EP0_MAX_PACKET 64 /* EP0's max packet size */
> #define AST_VHUB_EPn_MAX_PACKET 1024 /* Generic EPs max packet size */
> #define AST_VHUB_DESCS_COUNT 256 /* Use 256 descriptor mode (valid
> @@ -342,7 +332,7 @@ struct ast_vhub_dev {
> struct ast_vhub *vhub;
> void __iomem *regs;
>
> - /* Device index (0...4) and name string */
> + /* Device index (zero-based) and name string */
> unsigned int index;
> const char *name;
>
> @@ -358,7 +348,8 @@ struct ast_vhub_dev {
>
> /* Endpoint structures */
> struct ast_vhub_ep ep0;
> - struct ast_vhub_ep *epns[AST_VHUB_NUM_GEN_EPs];
> + struct ast_vhub_ep **epns;
> + u32 max_epns;
>
> };
> #define to_ast_dev(__g) container_of(__g, struct ast_vhub_dev, gadget)
> @@ -393,10 +384,12 @@ struct ast_vhub {
> bool ep1_stalled : 1;
>
> /* Per-port info */
> - struct ast_vhub_port ports[AST_VHUB_NUM_PORTS];
> + struct ast_vhub_port *ports;
> + u32 max_ports;
>
> /* Generic EP data structures */
> - struct ast_vhub_ep epns[AST_VHUB_NUM_GEN_EPs];
> + struct ast_vhub_ep *epns;
> + u32 max_epns;
>
> /* Upstream bus is suspended ? */
> bool suspended : 1;
> --
> 2.17.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-10 2:47 ` Joel Stanley
0 siblings, 0 replies; 42+ messages in thread
From: Joel Stanley @ 2020-02-10 2:47 UTC (permalink / raw)
To: Tao Ren, Benjamin Herrenschmidt
Cc: Felipe Balbi, Greg Kroah-Hartman, Andrew Jeffery, Chunfeng Yun,
Colin Ian King, Stephen Boyd, Rob Herring, Mark Rutland,
linux-usb, Linux ARM, linux-aspeed, devicetree,
Linux Kernel Mailing List, OpenBMC Maillist
On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
>
> From: Tao Ren <rentao.bupt@gmail.com>
>
> The patch moves hardcoded vhub attributes (maximum downstream ports and
> generic endpoints) to "ast_vhub_config" structure which is attached to
> struct of_device_id. The major purpose is to add AST2600 vhub support
> because AST2600 vhub provides more downstream ports and endpoints.
>
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
This looks generally okay. We should wait for Ben's ack before applying.
Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---
> drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--------
> drivers/usb/gadget/udc/aspeed-vhub/dev.c | 30 +++++--
> drivers/usb/gadget/udc/aspeed-vhub/epn.c | 4 +-
> drivers/usb/gadget/udc/aspeed-vhub/hub.c | 26 ++++--
> drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 23 ++---
> 5 files changed, 112 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> index 90b134d5dca9..94081cc04113 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> @@ -32,6 +32,29 @@
>
> #include "vhub.h"
>
> +struct ast_vhub_config {
> + u32 max_ports; /* max number of downstream ports */
> + u32 max_epns; /* max number of generic endpoints */
> +};
> +
> +static const struct ast_vhub_config ast2400_config = {
> + .max_ports = 5,
> + .max_epns = 15,
> +};
> +
> +static const struct of_device_id ast_vhub_dt_ids[] = {
> + {
> + .compatible = "aspeed,ast2400-usb-vhub",
> + .data = &ast2400_config,
> + },
> + {
> + .compatible = "aspeed,ast2500-usb-vhub",
> + .data = &ast2400_config,
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> +
> void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req *req,
> int status)
> {
> @@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> {
> struct ast_vhub *vhub = data;
> irqreturn_t iret = IRQ_NONE;
> - u32 istat;
> + u32 i, istat;
>
> /* Stale interrupt while tearing down */
> if (!vhub->ep0_bufs)
> @@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>
> /* Handle generic EPs first */
> if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> - u32 i, ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
> + u32 ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
> writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
>
> - for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs; i++) {
> + for (i = 0; ep_acks && i < vhub->max_epns; i++) {
> u32 mask = VHUB_EP_IRQ(i);
> if (ep_acks & mask) {
> ast_vhub_epn_ack_irq(&vhub->epns[i]);
> @@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> }
>
> /* Handle device interrupts */
> - if (istat & (VHUB_IRQ_DEVICE1 |
> - VHUB_IRQ_DEVICE2 |
> - VHUB_IRQ_DEVICE3 |
> - VHUB_IRQ_DEVICE4 |
> - VHUB_IRQ_DEVICE5)) {
> - if (istat & VHUB_IRQ_DEVICE1)
> - ast_vhub_dev_irq(&vhub->ports[0].dev);
> - if (istat & VHUB_IRQ_DEVICE2)
> - ast_vhub_dev_irq(&vhub->ports[1].dev);
> - if (istat & VHUB_IRQ_DEVICE3)
> - ast_vhub_dev_irq(&vhub->ports[2].dev);
> - if (istat & VHUB_IRQ_DEVICE4)
> - ast_vhub_dev_irq(&vhub->ports[3].dev);
> - if (istat & VHUB_IRQ_DEVICE5)
> - ast_vhub_dev_irq(&vhub->ports[4].dev);
> + for (i = 0; i < vhub->max_ports; i++) {
> + u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
> +
> + if (istat & dev_mask)
> + ast_vhub_dev_irq(&vhub->ports[i].dev);
> }
>
> /* Handle top-level vHub EP0 interrupts */
> @@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>
> void ast_vhub_init_hw(struct ast_vhub *vhub)
> {
> - u32 ctrl;
> + u32 ctrl, port_mask, epn_mask;
>
> UDCDBG(vhub,"(Re)Starting HW ...\n");
>
> @@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
> }
>
> /* Reset all devices */
> - writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
> + port_mask = GENMASK(vhub->max_ports, 1);
> + writel(VHUB_SW_RESET_ROOT_HUB |
> + VHUB_SW_RESET_DMA_CONTROLLER |
> + VHUB_SW_RESET_EP_POOL |
> + port_mask, vhub->regs + AST_VHUB_SW_RESET);
> udelay(1);
> writel(0, vhub->regs + AST_VHUB_SW_RESET);
>
> /* Disable and cleanup EP ACK/NACK interrupts */
> + epn_mask = GENMASK(vhub->max_epns - 1, 0);
> writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
> writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
> - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
> + writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
> + writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
>
> /* Default settings for EP0, enable HW hub EP1 */
> writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> @@ -273,7 +291,7 @@ static int ast_vhub_remove(struct platform_device *pdev)
> return 0;
>
> /* Remove devices */
> - for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> + for (i = 0; i < vhub->max_ports; i++)
> ast_vhub_del_dev(&vhub->ports[i].dev);
>
> spin_lock_irqsave(&vhub->lock, flags);
> @@ -295,7 +313,7 @@ static int ast_vhub_remove(struct platform_device *pdev)
> if (vhub->ep0_bufs)
> dma_free_coherent(&pdev->dev,
> AST_VHUB_EP0_MAX_PACKET *
> - (AST_VHUB_NUM_PORTS + 1),
> + (vhub->max_ports + 1),
> vhub->ep0_bufs,
> vhub->ep0_bufs_dma);
> vhub->ep0_bufs = NULL;
> @@ -309,11 +327,30 @@ static int ast_vhub_probe(struct platform_device *pdev)
> struct ast_vhub *vhub;
> struct resource *res;
> int i, rc = 0;
> + const struct of_device_id *ofdid;
> + const struct ast_vhub_config *config;
>
> vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
> if (!vhub)
> return -ENOMEM;
>
> + ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
> + if (!ofdid)
> + return -EINVAL;
> + config = ofdid->data;
> +
> + vhub->max_ports = config->max_ports;
> + vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
> + sizeof(*vhub->ports), GFP_KERNEL);
> + if (!vhub->ports)
> + return -ENOMEM;
> +
> + vhub->max_epns = config->max_epns;
> + vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
> + sizeof(*vhub->epns), GFP_KERNEL);
> + if (!vhub->epns)
> + return -ENOMEM;
> +
> spin_lock_init(&vhub->lock);
> vhub->pdev = pdev;
>
> @@ -366,7 +403,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
> */
> vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
> AST_VHUB_EP0_MAX_PACKET *
> - (AST_VHUB_NUM_PORTS + 1),
> + (vhub->max_ports + 1),
> &vhub->ep0_bufs_dma, GFP_KERNEL);
> if (!vhub->ep0_bufs) {
> dev_err(&pdev->dev, "Failed to allocate EP0 DMA buffers\n");
> @@ -380,7 +417,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
> ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
>
> /* Init devices */
> - for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
> + for (i = 0; i < vhub->max_ports && rc == 0; i++)
> rc = ast_vhub_init_dev(vhub, i);
> if (rc)
> goto err;
> @@ -400,17 +437,6 @@ static int ast_vhub_probe(struct platform_device *pdev)
> return rc;
> }
>
> -static const struct of_device_id ast_vhub_dt_ids[] = {
> - {
> - .compatible = "aspeed,ast2400-usb-vhub",
> - },
> - {
> - .compatible = "aspeed,ast2500-usb-vhub",
> - },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> -
> static struct platform_driver ast_vhub_driver = {
> .probe = ast_vhub_probe,
> .remove = ast_vhub_remove,
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> index 4008e7a51188..d268306a7bfe 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> @@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct ast_vhub_dev *d)
> writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
>
> /* Clear stall on all EPs */
> - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> + for (i = 0; i < d->max_epns; i++) {
> struct ast_vhub_ep *ep = d->epns[i];
>
> if (ep && (ep->epn.stalled || ep->epn.wedged)) {
> @@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct ast_vhub_dev *d,
> is_set ? "SET" : "CLEAR", ep_num, wValue);
> if (ep_num == 0)
> return std_req_complete;
> - if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
> + if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
> return std_req_stall;
> if (wValue != USB_ENDPOINT_HALT)
> return std_req_driver;
> @@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct ast_vhub_dev *d,
>
> DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
>
> - if (ep_num >= AST_VHUB_NUM_GEN_EPs)
> + if (ep_num >= d->max_epns)
> return std_req_stall;
> if (ep_num != 0) {
> ep = d->epns[ep_num - 1];
> @@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct ast_vhub_dev *d)
> {
> unsigned int i;
>
> - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> + for (i = 0; i < d->max_epns; i++) {
> if (!d->epns[i])
> continue;
> ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
> @@ -416,10 +416,10 @@ static struct usb_ep *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
> * that will allow the generic code to use our
> * assigned address.
> */
> - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> + for (i = 0; i < d->max_epns; i++)
> if (d->epns[i] == NULL)
> break;
> - if (i >= AST_VHUB_NUM_GEN_EPs)
> + if (i >= d->max_epns)
> return NULL;
> addr = i + 1;
>
> @@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
>
> usb_del_gadget_udc(&d->gadget);
> device_unregister(d->port_dev);
> + kfree(d->epns);
> }
>
> static void ast_vhub_dev_release(struct device *dev)
> @@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub, unsigned int idx)
>
> ast_vhub_init_ep0(vhub, &d->ep0, d);
>
> + /*
> + * A USB device can have up to 30 endpoints besides control
> + * endpoint 0.
> + */
> + d->max_epns = min_t(u32, vhub->max_epns, 30);
> + d->epns = kcalloc(d->max_epns, sizeof(*d->epns), GFP_KERNEL);
> + if (!d->epns)
> + return -ENOMEM;
> +
> /*
> * The UDC core really needs us to have separate and uniquely
> * named "parent" devices for each port so we create a sub device
> * here for that purpose
> */
> d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> - if (!d->port_dev)
> - return -ENOMEM;
> + if (!d->port_dev) {
> + rc = -ENOMEM;
> + goto fail_alloc;
> + }
> device_initialize(d->port_dev);
> d->port_dev->release = ast_vhub_dev_release;
> d->port_dev->parent = parent;
> @@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub, unsigned int idx)
> device_del(d->port_dev);
> fail_add:
> put_device(d->port_dev);
> + fail_alloc:
> + kfree(d->epns);
>
> return rc;
> }
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> index 7475c74aa5c5..0bd6b20435b8 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> @@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct ast_vhub_dev *d, u8 addr)
>
> /* Find a free one (no device) */
> spin_lock_irqsave(&vhub->lock, flags);
> - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> + for (i = 0; i < vhub->max_epns; i++)
> if (vhub->epns[i].dev == NULL)
> break;
> - if (i >= AST_VHUB_NUM_GEN_EPs) {
> + if (i >= vhub->max_epns) {
> spin_unlock_irqrestore(&vhub->lock, flags);
> return NULL;
> }
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> index 19b3517e04c0..9c7e57fbd8ef 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> @@ -133,10 +133,13 @@ static const struct ast_vhub_full_cdesc {
>
> #define AST_VHUB_HUB_DESC_SIZE (USB_DT_HUB_NONVAR_SIZE + 2)
>
> -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> +/*
> + * "bNbrPorts" field is updated in "ast_vhub_init_hub" function based on
> + * "max_ports" of the vhub.
> + */
> +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> .bDescLength = AST_VHUB_HUB_DESC_SIZE,
> .bDescriptorType = USB_DT_HUB,
> - .bNbrPorts = AST_VHUB_NUM_PORTS,
> .wHubCharacteristics = cpu_to_le16(HUB_CHAR_NO_LPSM),
> .bPwrOn2PwrGood = 10,
> .bHubContrCurrent = 0,
> @@ -504,7 +507,7 @@ static void ast_vhub_wake_work(struct work_struct *work)
> * we let the normal host wake path deal with it later.
> */
> spin_lock_irqsave(&vhub->lock, flags);
> - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> + for (i = 0; i < vhub->max_ports; i++) {
> struct ast_vhub_port *p = &vhub->ports[i];
>
> if (!(p->status & USB_PORT_STAT_SUSPEND))
> @@ -587,7 +590,7 @@ static enum std_req_rc ast_vhub_set_port_feature(struct ast_vhub_ep *ep,
> struct ast_vhub *vhub = ep->vhub;
> struct ast_vhub_port *p;
>
> - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> + if (port == 0 || port > vhub->max_ports)
> return std_req_stall;
> port--;
> p = &vhub->ports[port];
> @@ -630,7 +633,7 @@ static enum std_req_rc ast_vhub_clr_port_feature(struct ast_vhub_ep *ep,
> struct ast_vhub *vhub = ep->vhub;
> struct ast_vhub_port *p;
>
> - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> + if (port == 0 || port > vhub->max_ports)
> return std_req_stall;
> port--;
> p = &vhub->ports[port];
> @@ -676,7 +679,7 @@ static enum std_req_rc ast_vhub_get_port_stat(struct ast_vhub_ep *ep,
> struct ast_vhub *vhub = ep->vhub;
> u16 stat, chg;
>
> - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> + if (port == 0 || port > vhub->max_ports)
> return std_req_stall;
> port--;
>
> @@ -757,7 +760,7 @@ void ast_vhub_hub_suspend(struct ast_vhub *vhub)
> * Forward to unsuspended ports without changing
> * their connection status.
> */
> - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> + for (i = 0; i < vhub->max_ports; i++) {
> struct ast_vhub_port *p = &vhub->ports[i];
>
> if (!(p->status & USB_PORT_STAT_SUSPEND))
> @@ -780,7 +783,7 @@ void ast_vhub_hub_resume(struct ast_vhub *vhub)
> * Forward to unsuspended ports without changing
> * their connection status.
> */
> - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> + for (i = 0; i < vhub->max_ports; i++) {
> struct ast_vhub_port *p = &vhub->ports[i];
>
> if (!(p->status & USB_PORT_STAT_SUSPEND))
> @@ -814,7 +817,7 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
> * Clear all port status, disable gadgets and "suspend"
> * them. They will be woken up by a port reset.
> */
> - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> + for (i = 0; i < vhub->max_ports; i++) {
> struct ast_vhub_port *p = &vhub->ports[i];
>
> /* Only keep the connected flag */
> @@ -838,5 +841,10 @@ void ast_vhub_init_hub(struct ast_vhub *vhub)
> {
> vhub->speed = USB_SPEED_UNKNOWN;
> INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
> +
> + /*
> + * Fixup number of ports in hub descriptor.
> + */
> + ast_vhub_hub_desc.bNbrPorts = vhub->max_ports;
> }
>
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> index 761919e220d3..e46980fe66f2 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> @@ -76,17 +76,9 @@
> #define VHUB_SW_RESET_DEVICE2 (1 << 2)
> #define VHUB_SW_RESET_DEVICE1 (1 << 1)
> #define VHUB_SW_RESET_ROOT_HUB (1 << 0)
> -#define VHUB_SW_RESET_ALL (VHUB_SW_RESET_EP_POOL | \
> - VHUB_SW_RESET_DMA_CONTROLLER | \
> - VHUB_SW_RESET_DEVICE5 | \
> - VHUB_SW_RESET_DEVICE4 | \
> - VHUB_SW_RESET_DEVICE3 | \
> - VHUB_SW_RESET_DEVICE2 | \
> - VHUB_SW_RESET_DEVICE1 | \
> - VHUB_SW_RESET_ROOT_HUB)
> +
> /* EP ACK/NACK IRQ masks */
> #define VHUB_EP_IRQ(n) (1 << (n))
> -#define VHUB_EP_IRQ_ALL 0x7fff /* 15 EPs */
>
> /* USB status reg */
> #define VHUB_USBSTS_HISPEED (1 << 27)
> @@ -210,8 +202,6 @@
> * *
> ****************************************/
>
> -#define AST_VHUB_NUM_GEN_EPs 15 /* Generic non-0 EPs */
> -#define AST_VHUB_NUM_PORTS 5 /* vHub ports */
> #define AST_VHUB_EP0_MAX_PACKET 64 /* EP0's max packet size */
> #define AST_VHUB_EPn_MAX_PACKET 1024 /* Generic EPs max packet size */
> #define AST_VHUB_DESCS_COUNT 256 /* Use 256 descriptor mode (valid
> @@ -342,7 +332,7 @@ struct ast_vhub_dev {
> struct ast_vhub *vhub;
> void __iomem *regs;
>
> - /* Device index (0...4) and name string */
> + /* Device index (zero-based) and name string */
> unsigned int index;
> const char *name;
>
> @@ -358,7 +348,8 @@ struct ast_vhub_dev {
>
> /* Endpoint structures */
> struct ast_vhub_ep ep0;
> - struct ast_vhub_ep *epns[AST_VHUB_NUM_GEN_EPs];
> + struct ast_vhub_ep **epns;
> + u32 max_epns;
>
> };
> #define to_ast_dev(__g) container_of(__g, struct ast_vhub_dev, gadget)
> @@ -393,10 +384,12 @@ struct ast_vhub {
> bool ep1_stalled : 1;
>
> /* Per-port info */
> - struct ast_vhub_port ports[AST_VHUB_NUM_PORTS];
> + struct ast_vhub_port *ports;
> + u32 max_ports;
>
> /* Generic EP data structures */
> - struct ast_vhub_ep epns[AST_VHUB_NUM_GEN_EPs];
> + struct ast_vhub_ep *epns;
> + u32 max_epns;
>
> /* Upstream bus is suspended ? */
> bool suspended : 1;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
2020-02-10 2:47 ` Joel Stanley
(?)
@ 2020-02-10 7:27 ` Benjamin Herrenschmidt
-1 siblings, 0 replies; 42+ messages in thread
From: Benjamin Herrenschmidt @ 2020-02-10 7:27 UTC (permalink / raw)
To: linux-aspeed
On Mon, 2020-02-10 at 02:47 +0000, Joel Stanley wrote:
> On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
> >
> > From: Tao Ren <rentao.bupt@gmail.com>
> >
> > The patch moves hardcoded vhub attributes (maximum downstream ports
> > and
> > generic endpoints) to "ast_vhub_config" structure which is attached
> > to
> > struct of_device_id. The major purpose is to add AST2600 vhub
> > support
> > because AST2600 vhub provides more downstream ports and endpoints.
> >
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
>
> This looks generally okay. We should wait for Ben's ack before
> applying.
Shouldn't we instead have DT fields indicating those values ?
Also we should add a DT representation for the various ID/strings of
the hub itself so manufacturers can customize them.
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> > ---
> > drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--
> > ------
> > drivers/usb/gadget/udc/aspeed-vhub/dev.c | 30 +++++--
> > drivers/usb/gadget/udc/aspeed-vhub/epn.c | 4 +-
> > drivers/usb/gadget/udc/aspeed-vhub/hub.c | 26 ++++--
> > drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 23 ++---
> > 5 files changed, 112 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > index 90b134d5dca9..94081cc04113 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > @@ -32,6 +32,29 @@
> >
> > #include "vhub.h"
> >
> > +struct ast_vhub_config {
> > + u32 max_ports; /* max number of downstream ports */
> > + u32 max_epns; /* max number of generic endpoints */
> > +};
> > +
> > +static const struct ast_vhub_config ast2400_config = {
> > + .max_ports = 5,
> > + .max_epns = 15,
> > +};
> > +
> > +static const struct of_device_id ast_vhub_dt_ids[] = {
> > + {
> > + .compatible = "aspeed,ast2400-usb-vhub",
> > + .data = &ast2400_config,
> > + },
> > + {
> > + .compatible = "aspeed,ast2500-usb-vhub",
> > + .data = &ast2400_config,
> > + },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > +
> > void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req
> > *req,
> > int status)
> > {
> > @@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> > {
> > struct ast_vhub *vhub = data;
> > irqreturn_t iret = IRQ_NONE;
> > - u32 istat;
> > + u32 i, istat;
> >
> > /* Stale interrupt while tearing down */
> > if (!vhub->ep0_bufs)
> > @@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> >
> > /* Handle generic EPs first */
> > if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> > - u32 i, ep_acks = readl(vhub->regs +
> > AST_VHUB_EP_ACK_ISR);
> > + u32 ep_acks = readl(vhub->regs +
> > AST_VHUB_EP_ACK_ISR);
> > writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
> >
> > - for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs;
> > i++) {
> > + for (i = 0; ep_acks && i < vhub->max_epns; i++) {
> > u32 mask = VHUB_EP_IRQ(i);
> > if (ep_acks & mask) {
> > ast_vhub_epn_ack_irq(&vhub-
> > >epns[i]);
> > @@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> > }
> >
> > /* Handle device interrupts */
> > - if (istat & (VHUB_IRQ_DEVICE1 |
> > - VHUB_IRQ_DEVICE2 |
> > - VHUB_IRQ_DEVICE3 |
> > - VHUB_IRQ_DEVICE4 |
> > - VHUB_IRQ_DEVICE5)) {
> > - if (istat & VHUB_IRQ_DEVICE1)
> > - ast_vhub_dev_irq(&vhub->ports[0].dev);
> > - if (istat & VHUB_IRQ_DEVICE2)
> > - ast_vhub_dev_irq(&vhub->ports[1].dev);
> > - if (istat & VHUB_IRQ_DEVICE3)
> > - ast_vhub_dev_irq(&vhub->ports[2].dev);
> > - if (istat & VHUB_IRQ_DEVICE4)
> > - ast_vhub_dev_irq(&vhub->ports[3].dev);
> > - if (istat & VHUB_IRQ_DEVICE5)
> > - ast_vhub_dev_irq(&vhub->ports[4].dev);
> > + for (i = 0; i < vhub->max_ports; i++) {
> > + u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
> > +
> > + if (istat & dev_mask)
> > + ast_vhub_dev_irq(&vhub->ports[i].dev);
> > }
> >
> > /* Handle top-level vHub EP0 interrupts */
> > @@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> >
> > void ast_vhub_init_hw(struct ast_vhub *vhub)
> > {
> > - u32 ctrl;
> > + u32 ctrl, port_mask, epn_mask;
> >
> > UDCDBG(vhub,"(Re)Starting HW ...\n");
> >
> > @@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
> > }
> >
> > /* Reset all devices */
> > - writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
> > + port_mask = GENMASK(vhub->max_ports, 1);
> > + writel(VHUB_SW_RESET_ROOT_HUB |
> > + VHUB_SW_RESET_DMA_CONTROLLER |
> > + VHUB_SW_RESET_EP_POOL |
> > + port_mask, vhub->regs + AST_VHUB_SW_RESET);
> > udelay(1);
> > writel(0, vhub->regs + AST_VHUB_SW_RESET);
> >
> > /* Disable and cleanup EP ACK/NACK interrupts */
> > + epn_mask = GENMASK(vhub->max_epns - 1, 0);
> > writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
> > writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
> > + writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > + writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
> >
> > /* Default settings for EP0, enable HW hub EP1 */
> > writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> > @@ -273,7 +291,7 @@ static int ast_vhub_remove(struct
> > platform_device *pdev)
> > return 0;
> >
> > /* Remove devices */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> > + for (i = 0; i < vhub->max_ports; i++)
> > ast_vhub_del_dev(&vhub->ports[i].dev);
> >
> > spin_lock_irqsave(&vhub->lock, flags);
> > @@ -295,7 +313,7 @@ static int ast_vhub_remove(struct
> > platform_device *pdev)
> > if (vhub->ep0_bufs)
> > dma_free_coherent(&pdev->dev,
> > AST_VHUB_EP0_MAX_PACKET *
> > - (AST_VHUB_NUM_PORTS + 1),
> > + (vhub->max_ports + 1),
> > vhub->ep0_bufs,
> > vhub->ep0_bufs_dma);
> > vhub->ep0_bufs = NULL;
> > @@ -309,11 +327,30 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > struct ast_vhub *vhub;
> > struct resource *res;
> > int i, rc = 0;
> > + const struct of_device_id *ofdid;
> > + const struct ast_vhub_config *config;
> >
> > vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
> > if (!vhub)
> > return -ENOMEM;
> >
> > + ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
> > + if (!ofdid)
> > + return -EINVAL;
> > + config = ofdid->data;
> > +
> > + vhub->max_ports = config->max_ports;
> > + vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
> > + sizeof(*vhub->ports),
> > GFP_KERNEL);
> > + if (!vhub->ports)
> > + return -ENOMEM;
> > +
> > + vhub->max_epns = config->max_epns;
> > + vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
> > + sizeof(*vhub->epns), GFP_KERNEL);
> > + if (!vhub->epns)
> > + return -ENOMEM;
> > +
> > spin_lock_init(&vhub->lock);
> > vhub->pdev = pdev;
> >
> > @@ -366,7 +403,7 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > */
> > vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
> > AST_VHUB_EP0_MAX_PACKET
> > *
> > - (AST_VHUB_NUM_PORTS +
> > 1),
> > + (vhub->max_ports + 1),
> > &vhub->ep0_bufs_dma,
> > GFP_KERNEL);
> > if (!vhub->ep0_bufs) {
> > dev_err(&pdev->dev, "Failed to allocate EP0 DMA
> > buffers\n");
> > @@ -380,7 +417,7 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
> >
> > /* Init devices */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
> > + for (i = 0; i < vhub->max_ports && rc == 0; i++)
> > rc = ast_vhub_init_dev(vhub, i);
> > if (rc)
> > goto err;
> > @@ -400,17 +437,6 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > return rc;
> > }
> >
> > -static const struct of_device_id ast_vhub_dt_ids[] = {
> > - {
> > - .compatible = "aspeed,ast2400-usb-vhub",
> > - },
> > - {
> > - .compatible = "aspeed,ast2500-usb-vhub",
> > - },
> > - { }
> > -};
> > -MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > -
> > static struct platform_driver ast_vhub_driver = {
> > .probe = ast_vhub_probe,
> > .remove = ast_vhub_remove,
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > index 4008e7a51188..d268306a7bfe 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > @@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct
> > ast_vhub_dev *d)
> > writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
> >
> > /* Clear stall on all EPs */
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > + for (i = 0; i < d->max_epns; i++) {
> > struct ast_vhub_ep *ep = d->epns[i];
> >
> > if (ep && (ep->epn.stalled || ep->epn.wedged)) {
> > @@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct
> > ast_vhub_dev *d,
> > is_set ? "SET" : "CLEAR", ep_num, wValue);
> > if (ep_num == 0)
> > return std_req_complete;
> > - if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
> > + if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
> > return std_req_stall;
> > if (wValue != USB_ENDPOINT_HALT)
> > return std_req_driver;
> > @@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct
> > ast_vhub_dev *d,
> >
> > DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
> >
> > - if (ep_num >= AST_VHUB_NUM_GEN_EPs)
> > + if (ep_num >= d->max_epns)
> > return std_req_stall;
> > if (ep_num != 0) {
> > ep = d->epns[ep_num - 1];
> > @@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct
> > ast_vhub_dev *d)
> > {
> > unsigned int i;
> >
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > + for (i = 0; i < d->max_epns; i++) {
> > if (!d->epns[i])
> > continue;
> > ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
> > @@ -416,10 +416,10 @@ static struct usb_ep
> > *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
> > * that will allow the generic code to use our
> > * assigned address.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > + for (i = 0; i < d->max_epns; i++)
> > if (d->epns[i] == NULL)
> > break;
> > - if (i >= AST_VHUB_NUM_GEN_EPs)
> > + if (i >= d->max_epns)
> > return NULL;
> > addr = i + 1;
> >
> > @@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
> >
> > usb_del_gadget_udc(&d->gadget);
> > device_unregister(d->port_dev);
> > + kfree(d->epns);
> > }
> >
> > static void ast_vhub_dev_release(struct device *dev)
> > @@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > unsigned int idx)
> >
> > ast_vhub_init_ep0(vhub, &d->ep0, d);
> >
> > + /*
> > + * A USB device can have up to 30 endpoints besides control
> > + * endpoint 0.
> > + */
> > + d->max_epns = min_t(u32, vhub->max_epns, 30);
> > + d->epns = kcalloc(d->max_epns, sizeof(*d->epns),
> > GFP_KERNEL);
> > + if (!d->epns)
> > + return -ENOMEM;
> > +
> > /*
> > * The UDC core really needs us to have separate and
> > uniquely
> > * named "parent" devices for each port so we create a sub
> > device
> > * here for that purpose
> > */
> > d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> > - if (!d->port_dev)
> > - return -ENOMEM;
> > + if (!d->port_dev) {
> > + rc = -ENOMEM;
> > + goto fail_alloc;
> > + }
> > device_initialize(d->port_dev);
> > d->port_dev->release = ast_vhub_dev_release;
> > d->port_dev->parent = parent;
> > @@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > unsigned int idx)
> > device_del(d->port_dev);
> > fail_add:
> > put_device(d->port_dev);
> > + fail_alloc:
> > + kfree(d->epns);
> >
> > return rc;
> > }
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > index 7475c74aa5c5..0bd6b20435b8 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > @@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct
> > ast_vhub_dev *d, u8 addr)
> >
> > /* Find a free one (no device) */
> > spin_lock_irqsave(&vhub->lock, flags);
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > + for (i = 0; i < vhub->max_epns; i++)
> > if (vhub->epns[i].dev == NULL)
> > break;
> > - if (i >= AST_VHUB_NUM_GEN_EPs) {
> > + if (i >= vhub->max_epns) {
> > spin_unlock_irqrestore(&vhub->lock, flags);
> > return NULL;
> > }
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > index 19b3517e04c0..9c7e57fbd8ef 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > @@ -133,10 +133,13 @@ static const struct ast_vhub_full_cdesc {
> >
> > #define AST_VHUB_HUB_DESC_SIZE (USB_DT_HUB_NONVAR_SIZE + 2)
> >
> > -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> > +/*
> > + * "bNbrPorts" field is updated in "ast_vhub_init_hub" function
> > based on
> > + * "max_ports" of the vhub.
> > + */
> > +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> > .bDescLength = AST_VHUB_HUB_DESC_SIZE,
> > .bDescriptorType = USB_DT_HUB,
> > - .bNbrPorts = AST_VHUB_NUM_PORTS,
> > .wHubCharacteristics =
> > cpu_to_le16(HUB_CHAR_NO_LPSM),
> > .bPwrOn2PwrGood = 10,
> > .bHubContrCurrent = 0,
> > @@ -504,7 +507,7 @@ static void ast_vhub_wake_work(struct
> > work_struct *work)
> > * we let the normal host wake path deal with it later.
> > */
> > spin_lock_irqsave(&vhub->lock, flags);
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > @@ -587,7 +590,7 @@ static enum std_req_rc
> > ast_vhub_set_port_feature(struct ast_vhub_ep *ep,
> > struct ast_vhub *vhub = ep->vhub;
> > struct ast_vhub_port *p;
> >
> > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > + if (port == 0 || port > vhub->max_ports)
> > return std_req_stall;
> > port--;
> > p = &vhub->ports[port];
> > @@ -630,7 +633,7 @@ static enum std_req_rc
> > ast_vhub_clr_port_feature(struct ast_vhub_ep *ep,
> > struct ast_vhub *vhub = ep->vhub;
> > struct ast_vhub_port *p;
> >
> > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > + if (port == 0 || port > vhub->max_ports)
> > return std_req_stall;
> > port--;
> > p = &vhub->ports[port];
> > @@ -676,7 +679,7 @@ static enum std_req_rc
> > ast_vhub_get_port_stat(struct ast_vhub_ep *ep,
> > struct ast_vhub *vhub = ep->vhub;
> > u16 stat, chg;
> >
> > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > + if (port == 0 || port > vhub->max_ports)
> > return std_req_stall;
> > port--;
> >
> > @@ -757,7 +760,7 @@ void ast_vhub_hub_suspend(struct ast_vhub
> > *vhub)
> > * Forward to unsuspended ports without changing
> > * their connection status.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > @@ -780,7 +783,7 @@ void ast_vhub_hub_resume(struct ast_vhub *vhub)
> > * Forward to unsuspended ports without changing
> > * their connection status.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > @@ -814,7 +817,7 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
> > * Clear all port status, disable gadgets and "suspend"
> > * them. They will be woken up by a port reset.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > /* Only keep the connected flag */
> > @@ -838,5 +841,10 @@ void ast_vhub_init_hub(struct ast_vhub *vhub)
> > {
> > vhub->speed = USB_SPEED_UNKNOWN;
> > INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
> > +
> > + /*
> > + * Fixup number of ports in hub descriptor.
> > + */
> > + ast_vhub_hub_desc.bNbrPorts = vhub->max_ports;
> > }
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > index 761919e220d3..e46980fe66f2 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > @@ -76,17 +76,9 @@
> > #define VHUB_SW_RESET_DEVICE2 (1 << 2)
> > #define VHUB_SW_RESET_DEVICE1 (1 << 1)
> > #define VHUB_SW_RESET_ROOT_HUB (1 << 0)
> > -#define
> > VHUB_SW_RESET_ALL (VHUB_SW_RESET_EP_POOL | \
> > - VHUB_SW_RESET_DMA_
> > CONTROLLER | \
> > - VHUB_SW_RESET_DEVI
> > CE5 | \
> > - VHUB_SW_RESET_DEVI
> > CE4 | \
> > - VHUB_SW_RESET_DEVI
> > CE3 | \
> > - VHUB_SW_RESET_DEVI
> > CE2 | \
> > - VHUB_SW_RESET_DEVI
> > CE1 | \
> > - VHUB_SW_RESET_ROOT
> > _HUB)
> > +
> > /* EP ACK/NACK IRQ masks */
> > #define VHUB_EP_IRQ(n) (1 << (n))
> > -#define VHUB_EP_IRQ_ALL 0x7fff /*
> > 15 EPs */
> >
> > /* USB status reg */
> > #define VHUB_USBSTS_HISPEED (1 << 27)
> > @@ -210,8 +202,6 @@
> > * *
> > ****************************************/
> >
> > -#define AST_VHUB_NUM_GEN_EPs 15 /* Generic non-0 EPs */
> > -#define AST_VHUB_NUM_PORTS 5 /* vHub ports */
> > #define AST_VHUB_EP0_MAX_PACKET 64 /* EP0's max packet
> > size */
> > #define AST_VHUB_EPn_MAX_PACKET 1024 /* Generic EPs max
> > packet size */
> > #define AST_VHUB_DESCS_COUNT 256 /* Use 256 descriptor mode
> > (valid
> > @@ -342,7 +332,7 @@ struct ast_vhub_dev {
> > struct ast_vhub *vhub;
> > void __iomem *regs;
> >
> > - /* Device index (0...4) and name string */
> > + /* Device index (zero-based) and name string */
> > unsigned int index;
> > const char *name;
> >
> > @@ -358,7 +348,8 @@ struct ast_vhub_dev {
> >
> > /* Endpoint structures */
> > struct ast_vhub_ep ep0;
> > - struct
> > ast_vhub_ep *epns[AST_VHUB_NUM_GEN_EPs];
> > + struct ast_vhub_ep **epns;
> > + u32 max_epns;
> >
> > };
> > #define to_ast_dev(__g) container_of(__g, struct ast_vhub_dev,
> > gadget)
> > @@ -393,10 +384,12 @@ struct ast_vhub {
> > bool ep1_stalled : 1;
> >
> > /* Per-port info */
> > - struct ast_vhub_port ports[AST_VHUB_NUM_PORTS];
> > + struct ast_vhub_port *ports;
> > + u32 max_ports;
> >
> > /* Generic EP data structures */
> > - struct ast_vhub_ep epns[AST_VHUB_NUM_GEN_EPs];
> > + struct ast_vhub_ep *epns;
> > + u32 max_epns;
> >
> > /* Upstream bus is suspended ? */
> > bool suspended : 1;
> > --
> > 2.17.1
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-10 7:27 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 42+ messages in thread
From: Benjamin Herrenschmidt @ 2020-02-10 7:27 UTC (permalink / raw)
To: Joel Stanley, Tao Ren
Cc: Mark Rutland, Felipe Balbi, linux-aspeed, devicetree,
Andrew Jeffery, Greg Kroah-Hartman, OpenBMC Maillist, linux-usb,
Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
Chunfeng Yun, Colin Ian King, Linux ARM
On Mon, 2020-02-10 at 02:47 +0000, Joel Stanley wrote:
> On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
> >
> > From: Tao Ren <rentao.bupt@gmail.com>
> >
> > The patch moves hardcoded vhub attributes (maximum downstream ports
> > and
> > generic endpoints) to "ast_vhub_config" structure which is attached
> > to
> > struct of_device_id. The major purpose is to add AST2600 vhub
> > support
> > because AST2600 vhub provides more downstream ports and endpoints.
> >
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
>
> This looks generally okay. We should wait for Ben's ack before
> applying.
Shouldn't we instead have DT fields indicating those values ?
Also we should add a DT representation for the various ID/strings of
the hub itself so manufacturers can customize them.
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> > ---
> > drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--
> > ------
> > drivers/usb/gadget/udc/aspeed-vhub/dev.c | 30 +++++--
> > drivers/usb/gadget/udc/aspeed-vhub/epn.c | 4 +-
> > drivers/usb/gadget/udc/aspeed-vhub/hub.c | 26 ++++--
> > drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 23 ++---
> > 5 files changed, 112 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > index 90b134d5dca9..94081cc04113 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > @@ -32,6 +32,29 @@
> >
> > #include "vhub.h"
> >
> > +struct ast_vhub_config {
> > + u32 max_ports; /* max number of downstream ports */
> > + u32 max_epns; /* max number of generic endpoints */
> > +};
> > +
> > +static const struct ast_vhub_config ast2400_config = {
> > + .max_ports = 5,
> > + .max_epns = 15,
> > +};
> > +
> > +static const struct of_device_id ast_vhub_dt_ids[] = {
> > + {
> > + .compatible = "aspeed,ast2400-usb-vhub",
> > + .data = &ast2400_config,
> > + },
> > + {
> > + .compatible = "aspeed,ast2500-usb-vhub",
> > + .data = &ast2400_config,
> > + },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > +
> > void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req
> > *req,
> > int status)
> > {
> > @@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> > {
> > struct ast_vhub *vhub = data;
> > irqreturn_t iret = IRQ_NONE;
> > - u32 istat;
> > + u32 i, istat;
> >
> > /* Stale interrupt while tearing down */
> > if (!vhub->ep0_bufs)
> > @@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> >
> > /* Handle generic EPs first */
> > if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> > - u32 i, ep_acks = readl(vhub->regs +
> > AST_VHUB_EP_ACK_ISR);
> > + u32 ep_acks = readl(vhub->regs +
> > AST_VHUB_EP_ACK_ISR);
> > writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
> >
> > - for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs;
> > i++) {
> > + for (i = 0; ep_acks && i < vhub->max_epns; i++) {
> > u32 mask = VHUB_EP_IRQ(i);
> > if (ep_acks & mask) {
> > ast_vhub_epn_ack_irq(&vhub-
> > >epns[i]);
> > @@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> > }
> >
> > /* Handle device interrupts */
> > - if (istat & (VHUB_IRQ_DEVICE1 |
> > - VHUB_IRQ_DEVICE2 |
> > - VHUB_IRQ_DEVICE3 |
> > - VHUB_IRQ_DEVICE4 |
> > - VHUB_IRQ_DEVICE5)) {
> > - if (istat & VHUB_IRQ_DEVICE1)
> > - ast_vhub_dev_irq(&vhub->ports[0].dev);
> > - if (istat & VHUB_IRQ_DEVICE2)
> > - ast_vhub_dev_irq(&vhub->ports[1].dev);
> > - if (istat & VHUB_IRQ_DEVICE3)
> > - ast_vhub_dev_irq(&vhub->ports[2].dev);
> > - if (istat & VHUB_IRQ_DEVICE4)
> > - ast_vhub_dev_irq(&vhub->ports[3].dev);
> > - if (istat & VHUB_IRQ_DEVICE5)
> > - ast_vhub_dev_irq(&vhub->ports[4].dev);
> > + for (i = 0; i < vhub->max_ports; i++) {
> > + u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
> > +
> > + if (istat & dev_mask)
> > + ast_vhub_dev_irq(&vhub->ports[i].dev);
> > }
> >
> > /* Handle top-level vHub EP0 interrupts */
> > @@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> >
> > void ast_vhub_init_hw(struct ast_vhub *vhub)
> > {
> > - u32 ctrl;
> > + u32 ctrl, port_mask, epn_mask;
> >
> > UDCDBG(vhub,"(Re)Starting HW ...\n");
> >
> > @@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
> > }
> >
> > /* Reset all devices */
> > - writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
> > + port_mask = GENMASK(vhub->max_ports, 1);
> > + writel(VHUB_SW_RESET_ROOT_HUB |
> > + VHUB_SW_RESET_DMA_CONTROLLER |
> > + VHUB_SW_RESET_EP_POOL |
> > + port_mask, vhub->regs + AST_VHUB_SW_RESET);
> > udelay(1);
> > writel(0, vhub->regs + AST_VHUB_SW_RESET);
> >
> > /* Disable and cleanup EP ACK/NACK interrupts */
> > + epn_mask = GENMASK(vhub->max_epns - 1, 0);
> > writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
> > writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
> > + writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > + writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
> >
> > /* Default settings for EP0, enable HW hub EP1 */
> > writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> > @@ -273,7 +291,7 @@ static int ast_vhub_remove(struct
> > platform_device *pdev)
> > return 0;
> >
> > /* Remove devices */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> > + for (i = 0; i < vhub->max_ports; i++)
> > ast_vhub_del_dev(&vhub->ports[i].dev);
> >
> > spin_lock_irqsave(&vhub->lock, flags);
> > @@ -295,7 +313,7 @@ static int ast_vhub_remove(struct
> > platform_device *pdev)
> > if (vhub->ep0_bufs)
> > dma_free_coherent(&pdev->dev,
> > AST_VHUB_EP0_MAX_PACKET *
> > - (AST_VHUB_NUM_PORTS + 1),
> > + (vhub->max_ports + 1),
> > vhub->ep0_bufs,
> > vhub->ep0_bufs_dma);
> > vhub->ep0_bufs = NULL;
> > @@ -309,11 +327,30 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > struct ast_vhub *vhub;
> > struct resource *res;
> > int i, rc = 0;
> > + const struct of_device_id *ofdid;
> > + const struct ast_vhub_config *config;
> >
> > vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
> > if (!vhub)
> > return -ENOMEM;
> >
> > + ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
> > + if (!ofdid)
> > + return -EINVAL;
> > + config = ofdid->data;
> > +
> > + vhub->max_ports = config->max_ports;
> > + vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
> > + sizeof(*vhub->ports),
> > GFP_KERNEL);
> > + if (!vhub->ports)
> > + return -ENOMEM;
> > +
> > + vhub->max_epns = config->max_epns;
> > + vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
> > + sizeof(*vhub->epns), GFP_KERNEL);
> > + if (!vhub->epns)
> > + return -ENOMEM;
> > +
> > spin_lock_init(&vhub->lock);
> > vhub->pdev = pdev;
> >
> > @@ -366,7 +403,7 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > */
> > vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
> > AST_VHUB_EP0_MAX_PACKET
> > *
> > - (AST_VHUB_NUM_PORTS +
> > 1),
> > + (vhub->max_ports + 1),
> > &vhub->ep0_bufs_dma,
> > GFP_KERNEL);
> > if (!vhub->ep0_bufs) {
> > dev_err(&pdev->dev, "Failed to allocate EP0 DMA
> > buffers\n");
> > @@ -380,7 +417,7 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
> >
> > /* Init devices */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
> > + for (i = 0; i < vhub->max_ports && rc == 0; i++)
> > rc = ast_vhub_init_dev(vhub, i);
> > if (rc)
> > goto err;
> > @@ -400,17 +437,6 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > return rc;
> > }
> >
> > -static const struct of_device_id ast_vhub_dt_ids[] = {
> > - {
> > - .compatible = "aspeed,ast2400-usb-vhub",
> > - },
> > - {
> > - .compatible = "aspeed,ast2500-usb-vhub",
> > - },
> > - { }
> > -};
> > -MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > -
> > static struct platform_driver ast_vhub_driver = {
> > .probe = ast_vhub_probe,
> > .remove = ast_vhub_remove,
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > index 4008e7a51188..d268306a7bfe 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > @@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct
> > ast_vhub_dev *d)
> > writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
> >
> > /* Clear stall on all EPs */
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > + for (i = 0; i < d->max_epns; i++) {
> > struct ast_vhub_ep *ep = d->epns[i];
> >
> > if (ep && (ep->epn.stalled || ep->epn.wedged)) {
> > @@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct
> > ast_vhub_dev *d,
> > is_set ? "SET" : "CLEAR", ep_num, wValue);
> > if (ep_num == 0)
> > return std_req_complete;
> > - if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
> > + if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
> > return std_req_stall;
> > if (wValue != USB_ENDPOINT_HALT)
> > return std_req_driver;
> > @@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct
> > ast_vhub_dev *d,
> >
> > DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
> >
> > - if (ep_num >= AST_VHUB_NUM_GEN_EPs)
> > + if (ep_num >= d->max_epns)
> > return std_req_stall;
> > if (ep_num != 0) {
> > ep = d->epns[ep_num - 1];
> > @@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct
> > ast_vhub_dev *d)
> > {
> > unsigned int i;
> >
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > + for (i = 0; i < d->max_epns; i++) {
> > if (!d->epns[i])
> > continue;
> > ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
> > @@ -416,10 +416,10 @@ static struct usb_ep
> > *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
> > * that will allow the generic code to use our
> > * assigned address.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > + for (i = 0; i < d->max_epns; i++)
> > if (d->epns[i] == NULL)
> > break;
> > - if (i >= AST_VHUB_NUM_GEN_EPs)
> > + if (i >= d->max_epns)
> > return NULL;
> > addr = i + 1;
> >
> > @@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
> >
> > usb_del_gadget_udc(&d->gadget);
> > device_unregister(d->port_dev);
> > + kfree(d->epns);
> > }
> >
> > static void ast_vhub_dev_release(struct device *dev)
> > @@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > unsigned int idx)
> >
> > ast_vhub_init_ep0(vhub, &d->ep0, d);
> >
> > + /*
> > + * A USB device can have up to 30 endpoints besides control
> > + * endpoint 0.
> > + */
> > + d->max_epns = min_t(u32, vhub->max_epns, 30);
> > + d->epns = kcalloc(d->max_epns, sizeof(*d->epns),
> > GFP_KERNEL);
> > + if (!d->epns)
> > + return -ENOMEM;
> > +
> > /*
> > * The UDC core really needs us to have separate and
> > uniquely
> > * named "parent" devices for each port so we create a sub
> > device
> > * here for that purpose
> > */
> > d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> > - if (!d->port_dev)
> > - return -ENOMEM;
> > + if (!d->port_dev) {
> > + rc = -ENOMEM;
> > + goto fail_alloc;
> > + }
> > device_initialize(d->port_dev);
> > d->port_dev->release = ast_vhub_dev_release;
> > d->port_dev->parent = parent;
> > @@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > unsigned int idx)
> > device_del(d->port_dev);
> > fail_add:
> > put_device(d->port_dev);
> > + fail_alloc:
> > + kfree(d->epns);
> >
> > return rc;
> > }
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > index 7475c74aa5c5..0bd6b20435b8 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > @@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct
> > ast_vhub_dev *d, u8 addr)
> >
> > /* Find a free one (no device) */
> > spin_lock_irqsave(&vhub->lock, flags);
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > + for (i = 0; i < vhub->max_epns; i++)
> > if (vhub->epns[i].dev == NULL)
> > break;
> > - if (i >= AST_VHUB_NUM_GEN_EPs) {
> > + if (i >= vhub->max_epns) {
> > spin_unlock_irqrestore(&vhub->lock, flags);
> > return NULL;
> > }
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > index 19b3517e04c0..9c7e57fbd8ef 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > @@ -133,10 +133,13 @@ static const struct ast_vhub_full_cdesc {
> >
> > #define AST_VHUB_HUB_DESC_SIZE (USB_DT_HUB_NONVAR_SIZE + 2)
> >
> > -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> > +/*
> > + * "bNbrPorts" field is updated in "ast_vhub_init_hub" function
> > based on
> > + * "max_ports" of the vhub.
> > + */
> > +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> > .bDescLength = AST_VHUB_HUB_DESC_SIZE,
> > .bDescriptorType = USB_DT_HUB,
> > - .bNbrPorts = AST_VHUB_NUM_PORTS,
> > .wHubCharacteristics =
> > cpu_to_le16(HUB_CHAR_NO_LPSM),
> > .bPwrOn2PwrGood = 10,
> > .bHubContrCurrent = 0,
> > @@ -504,7 +507,7 @@ static void ast_vhub_wake_work(struct
> > work_struct *work)
> > * we let the normal host wake path deal with it later.
> > */
> > spin_lock_irqsave(&vhub->lock, flags);
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > @@ -587,7 +590,7 @@ static enum std_req_rc
> > ast_vhub_set_port_feature(struct ast_vhub_ep *ep,
> > struct ast_vhub *vhub = ep->vhub;
> > struct ast_vhub_port *p;
> >
> > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > + if (port == 0 || port > vhub->max_ports)
> > return std_req_stall;
> > port--;
> > p = &vhub->ports[port];
> > @@ -630,7 +633,7 @@ static enum std_req_rc
> > ast_vhub_clr_port_feature(struct ast_vhub_ep *ep,
> > struct ast_vhub *vhub = ep->vhub;
> > struct ast_vhub_port *p;
> >
> > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > + if (port == 0 || port > vhub->max_ports)
> > return std_req_stall;
> > port--;
> > p = &vhub->ports[port];
> > @@ -676,7 +679,7 @@ static enum std_req_rc
> > ast_vhub_get_port_stat(struct ast_vhub_ep *ep,
> > struct ast_vhub *vhub = ep->vhub;
> > u16 stat, chg;
> >
> > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > + if (port == 0 || port > vhub->max_ports)
> > return std_req_stall;
> > port--;
> >
> > @@ -757,7 +760,7 @@ void ast_vhub_hub_suspend(struct ast_vhub
> > *vhub)
> > * Forward to unsuspended ports without changing
> > * their connection status.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > @@ -780,7 +783,7 @@ void ast_vhub_hub_resume(struct ast_vhub *vhub)
> > * Forward to unsuspended ports without changing
> > * their connection status.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > @@ -814,7 +817,7 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
> > * Clear all port status, disable gadgets and "suspend"
> > * them. They will be woken up by a port reset.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > /* Only keep the connected flag */
> > @@ -838,5 +841,10 @@ void ast_vhub_init_hub(struct ast_vhub *vhub)
> > {
> > vhub->speed = USB_SPEED_UNKNOWN;
> > INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
> > +
> > + /*
> > + * Fixup number of ports in hub descriptor.
> > + */
> > + ast_vhub_hub_desc.bNbrPorts = vhub->max_ports;
> > }
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > index 761919e220d3..e46980fe66f2 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > @@ -76,17 +76,9 @@
> > #define VHUB_SW_RESET_DEVICE2 (1 << 2)
> > #define VHUB_SW_RESET_DEVICE1 (1 << 1)
> > #define VHUB_SW_RESET_ROOT_HUB (1 << 0)
> > -#define
> > VHUB_SW_RESET_ALL (VHUB_SW_RESET_EP_POOL | \
> > - VHUB_SW_RESET_DMA_
> > CONTROLLER | \
> > - VHUB_SW_RESET_DEVI
> > CE5 | \
> > - VHUB_SW_RESET_DEVI
> > CE4 | \
> > - VHUB_SW_RESET_DEVI
> > CE3 | \
> > - VHUB_SW_RESET_DEVI
> > CE2 | \
> > - VHUB_SW_RESET_DEVI
> > CE1 | \
> > - VHUB_SW_RESET_ROOT
> > _HUB)
> > +
> > /* EP ACK/NACK IRQ masks */
> > #define VHUB_EP_IRQ(n) (1 << (n))
> > -#define VHUB_EP_IRQ_ALL 0x7fff /*
> > 15 EPs */
> >
> > /* USB status reg */
> > #define VHUB_USBSTS_HISPEED (1 << 27)
> > @@ -210,8 +202,6 @@
> > * *
> > ****************************************/
> >
> > -#define AST_VHUB_NUM_GEN_EPs 15 /* Generic non-0 EPs */
> > -#define AST_VHUB_NUM_PORTS 5 /* vHub ports */
> > #define AST_VHUB_EP0_MAX_PACKET 64 /* EP0's max packet
> > size */
> > #define AST_VHUB_EPn_MAX_PACKET 1024 /* Generic EPs max
> > packet size */
> > #define AST_VHUB_DESCS_COUNT 256 /* Use 256 descriptor mode
> > (valid
> > @@ -342,7 +332,7 @@ struct ast_vhub_dev {
> > struct ast_vhub *vhub;
> > void __iomem *regs;
> >
> > - /* Device index (0...4) and name string */
> > + /* Device index (zero-based) and name string */
> > unsigned int index;
> > const char *name;
> >
> > @@ -358,7 +348,8 @@ struct ast_vhub_dev {
> >
> > /* Endpoint structures */
> > struct ast_vhub_ep ep0;
> > - struct
> > ast_vhub_ep *epns[AST_VHUB_NUM_GEN_EPs];
> > + struct ast_vhub_ep **epns;
> > + u32 max_epns;
> >
> > };
> > #define to_ast_dev(__g) container_of(__g, struct ast_vhub_dev,
> > gadget)
> > @@ -393,10 +384,12 @@ struct ast_vhub {
> > bool ep1_stalled : 1;
> >
> > /* Per-port info */
> > - struct ast_vhub_port ports[AST_VHUB_NUM_PORTS];
> > + struct ast_vhub_port *ports;
> > + u32 max_ports;
> >
> > /* Generic EP data structures */
> > - struct ast_vhub_ep epns[AST_VHUB_NUM_GEN_EPs];
> > + struct ast_vhub_ep *epns;
> > + u32 max_epns;
> >
> > /* Upstream bus is suspended ? */
> > bool suspended : 1;
> > --
> > 2.17.1
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-10 7:27 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 42+ messages in thread
From: Benjamin Herrenschmidt @ 2020-02-10 7:27 UTC (permalink / raw)
To: Joel Stanley, Tao Ren
Cc: Mark Rutland, Felipe Balbi, linux-aspeed, devicetree,
Andrew Jeffery, Greg Kroah-Hartman, OpenBMC Maillist, linux-usb,
Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
Chunfeng Yun, Colin Ian King, Linux ARM
On Mon, 2020-02-10 at 02:47 +0000, Joel Stanley wrote:
> On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
> >
> > From: Tao Ren <rentao.bupt@gmail.com>
> >
> > The patch moves hardcoded vhub attributes (maximum downstream ports
> > and
> > generic endpoints) to "ast_vhub_config" structure which is attached
> > to
> > struct of_device_id. The major purpose is to add AST2600 vhub
> > support
> > because AST2600 vhub provides more downstream ports and endpoints.
> >
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
>
> This looks generally okay. We should wait for Ben's ack before
> applying.
Shouldn't we instead have DT fields indicating those values ?
Also we should add a DT representation for the various ID/strings of
the hub itself so manufacturers can customize them.
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> > ---
> > drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--
> > ------
> > drivers/usb/gadget/udc/aspeed-vhub/dev.c | 30 +++++--
> > drivers/usb/gadget/udc/aspeed-vhub/epn.c | 4 +-
> > drivers/usb/gadget/udc/aspeed-vhub/hub.c | 26 ++++--
> > drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 23 ++---
> > 5 files changed, 112 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > index 90b134d5dca9..94081cc04113 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > @@ -32,6 +32,29 @@
> >
> > #include "vhub.h"
> >
> > +struct ast_vhub_config {
> > + u32 max_ports; /* max number of downstream ports */
> > + u32 max_epns; /* max number of generic endpoints */
> > +};
> > +
> > +static const struct ast_vhub_config ast2400_config = {
> > + .max_ports = 5,
> > + .max_epns = 15,
> > +};
> > +
> > +static const struct of_device_id ast_vhub_dt_ids[] = {
> > + {
> > + .compatible = "aspeed,ast2400-usb-vhub",
> > + .data = &ast2400_config,
> > + },
> > + {
> > + .compatible = "aspeed,ast2500-usb-vhub",
> > + .data = &ast2400_config,
> > + },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > +
> > void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req
> > *req,
> > int status)
> > {
> > @@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> > {
> > struct ast_vhub *vhub = data;
> > irqreturn_t iret = IRQ_NONE;
> > - u32 istat;
> > + u32 i, istat;
> >
> > /* Stale interrupt while tearing down */
> > if (!vhub->ep0_bufs)
> > @@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> >
> > /* Handle generic EPs first */
> > if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> > - u32 i, ep_acks = readl(vhub->regs +
> > AST_VHUB_EP_ACK_ISR);
> > + u32 ep_acks = readl(vhub->regs +
> > AST_VHUB_EP_ACK_ISR);
> > writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
> >
> > - for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs;
> > i++) {
> > + for (i = 0; ep_acks && i < vhub->max_epns; i++) {
> > u32 mask = VHUB_EP_IRQ(i);
> > if (ep_acks & mask) {
> > ast_vhub_epn_ack_irq(&vhub-
> > >epns[i]);
> > @@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> > }
> >
> > /* Handle device interrupts */
> > - if (istat & (VHUB_IRQ_DEVICE1 |
> > - VHUB_IRQ_DEVICE2 |
> > - VHUB_IRQ_DEVICE3 |
> > - VHUB_IRQ_DEVICE4 |
> > - VHUB_IRQ_DEVICE5)) {
> > - if (istat & VHUB_IRQ_DEVICE1)
> > - ast_vhub_dev_irq(&vhub->ports[0].dev);
> > - if (istat & VHUB_IRQ_DEVICE2)
> > - ast_vhub_dev_irq(&vhub->ports[1].dev);
> > - if (istat & VHUB_IRQ_DEVICE3)
> > - ast_vhub_dev_irq(&vhub->ports[2].dev);
> > - if (istat & VHUB_IRQ_DEVICE4)
> > - ast_vhub_dev_irq(&vhub->ports[3].dev);
> > - if (istat & VHUB_IRQ_DEVICE5)
> > - ast_vhub_dev_irq(&vhub->ports[4].dev);
> > + for (i = 0; i < vhub->max_ports; i++) {
> > + u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
> > +
> > + if (istat & dev_mask)
> > + ast_vhub_dev_irq(&vhub->ports[i].dev);
> > }
> >
> > /* Handle top-level vHub EP0 interrupts */
> > @@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > *data)
> >
> > void ast_vhub_init_hw(struct ast_vhub *vhub)
> > {
> > - u32 ctrl;
> > + u32 ctrl, port_mask, epn_mask;
> >
> > UDCDBG(vhub,"(Re)Starting HW ...\n");
> >
> > @@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
> > }
> >
> > /* Reset all devices */
> > - writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
> > + port_mask = GENMASK(vhub->max_ports, 1);
> > + writel(VHUB_SW_RESET_ROOT_HUB |
> > + VHUB_SW_RESET_DMA_CONTROLLER |
> > + VHUB_SW_RESET_EP_POOL |
> > + port_mask, vhub->regs + AST_VHUB_SW_RESET);
> > udelay(1);
> > writel(0, vhub->regs + AST_VHUB_SW_RESET);
> >
> > /* Disable and cleanup EP ACK/NACK interrupts */
> > + epn_mask = GENMASK(vhub->max_epns - 1, 0);
> > writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
> > writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
> > + writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > + writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
> >
> > /* Default settings for EP0, enable HW hub EP1 */
> > writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> > @@ -273,7 +291,7 @@ static int ast_vhub_remove(struct
> > platform_device *pdev)
> > return 0;
> >
> > /* Remove devices */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> > + for (i = 0; i < vhub->max_ports; i++)
> > ast_vhub_del_dev(&vhub->ports[i].dev);
> >
> > spin_lock_irqsave(&vhub->lock, flags);
> > @@ -295,7 +313,7 @@ static int ast_vhub_remove(struct
> > platform_device *pdev)
> > if (vhub->ep0_bufs)
> > dma_free_coherent(&pdev->dev,
> > AST_VHUB_EP0_MAX_PACKET *
> > - (AST_VHUB_NUM_PORTS + 1),
> > + (vhub->max_ports + 1),
> > vhub->ep0_bufs,
> > vhub->ep0_bufs_dma);
> > vhub->ep0_bufs = NULL;
> > @@ -309,11 +327,30 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > struct ast_vhub *vhub;
> > struct resource *res;
> > int i, rc = 0;
> > + const struct of_device_id *ofdid;
> > + const struct ast_vhub_config *config;
> >
> > vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
> > if (!vhub)
> > return -ENOMEM;
> >
> > + ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
> > + if (!ofdid)
> > + return -EINVAL;
> > + config = ofdid->data;
> > +
> > + vhub->max_ports = config->max_ports;
> > + vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
> > + sizeof(*vhub->ports),
> > GFP_KERNEL);
> > + if (!vhub->ports)
> > + return -ENOMEM;
> > +
> > + vhub->max_epns = config->max_epns;
> > + vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
> > + sizeof(*vhub->epns), GFP_KERNEL);
> > + if (!vhub->epns)
> > + return -ENOMEM;
> > +
> > spin_lock_init(&vhub->lock);
> > vhub->pdev = pdev;
> >
> > @@ -366,7 +403,7 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > */
> > vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
> > AST_VHUB_EP0_MAX_PACKET
> > *
> > - (AST_VHUB_NUM_PORTS +
> > 1),
> > + (vhub->max_ports + 1),
> > &vhub->ep0_bufs_dma,
> > GFP_KERNEL);
> > if (!vhub->ep0_bufs) {
> > dev_err(&pdev->dev, "Failed to allocate EP0 DMA
> > buffers\n");
> > @@ -380,7 +417,7 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
> >
> > /* Init devices */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
> > + for (i = 0; i < vhub->max_ports && rc == 0; i++)
> > rc = ast_vhub_init_dev(vhub, i);
> > if (rc)
> > goto err;
> > @@ -400,17 +437,6 @@ static int ast_vhub_probe(struct
> > platform_device *pdev)
> > return rc;
> > }
> >
> > -static const struct of_device_id ast_vhub_dt_ids[] = {
> > - {
> > - .compatible = "aspeed,ast2400-usb-vhub",
> > - },
> > - {
> > - .compatible = "aspeed,ast2500-usb-vhub",
> > - },
> > - { }
> > -};
> > -MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > -
> > static struct platform_driver ast_vhub_driver = {
> > .probe = ast_vhub_probe,
> > .remove = ast_vhub_remove,
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > index 4008e7a51188..d268306a7bfe 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > @@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct
> > ast_vhub_dev *d)
> > writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
> >
> > /* Clear stall on all EPs */
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > + for (i = 0; i < d->max_epns; i++) {
> > struct ast_vhub_ep *ep = d->epns[i];
> >
> > if (ep && (ep->epn.stalled || ep->epn.wedged)) {
> > @@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct
> > ast_vhub_dev *d,
> > is_set ? "SET" : "CLEAR", ep_num, wValue);
> > if (ep_num == 0)
> > return std_req_complete;
> > - if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
> > + if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
> > return std_req_stall;
> > if (wValue != USB_ENDPOINT_HALT)
> > return std_req_driver;
> > @@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct
> > ast_vhub_dev *d,
> >
> > DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
> >
> > - if (ep_num >= AST_VHUB_NUM_GEN_EPs)
> > + if (ep_num >= d->max_epns)
> > return std_req_stall;
> > if (ep_num != 0) {
> > ep = d->epns[ep_num - 1];
> > @@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct
> > ast_vhub_dev *d)
> > {
> > unsigned int i;
> >
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > + for (i = 0; i < d->max_epns; i++) {
> > if (!d->epns[i])
> > continue;
> > ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
> > @@ -416,10 +416,10 @@ static struct usb_ep
> > *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
> > * that will allow the generic code to use our
> > * assigned address.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > + for (i = 0; i < d->max_epns; i++)
> > if (d->epns[i] == NULL)
> > break;
> > - if (i >= AST_VHUB_NUM_GEN_EPs)
> > + if (i >= d->max_epns)
> > return NULL;
> > addr = i + 1;
> >
> > @@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
> >
> > usb_del_gadget_udc(&d->gadget);
> > device_unregister(d->port_dev);
> > + kfree(d->epns);
> > }
> >
> > static void ast_vhub_dev_release(struct device *dev)
> > @@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > unsigned int idx)
> >
> > ast_vhub_init_ep0(vhub, &d->ep0, d);
> >
> > + /*
> > + * A USB device can have up to 30 endpoints besides control
> > + * endpoint 0.
> > + */
> > + d->max_epns = min_t(u32, vhub->max_epns, 30);
> > + d->epns = kcalloc(d->max_epns, sizeof(*d->epns),
> > GFP_KERNEL);
> > + if (!d->epns)
> > + return -ENOMEM;
> > +
> > /*
> > * The UDC core really needs us to have separate and
> > uniquely
> > * named "parent" devices for each port so we create a sub
> > device
> > * here for that purpose
> > */
> > d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> > - if (!d->port_dev)
> > - return -ENOMEM;
> > + if (!d->port_dev) {
> > + rc = -ENOMEM;
> > + goto fail_alloc;
> > + }
> > device_initialize(d->port_dev);
> > d->port_dev->release = ast_vhub_dev_release;
> > d->port_dev->parent = parent;
> > @@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > unsigned int idx)
> > device_del(d->port_dev);
> > fail_add:
> > put_device(d->port_dev);
> > + fail_alloc:
> > + kfree(d->epns);
> >
> > return rc;
> > }
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > index 7475c74aa5c5..0bd6b20435b8 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > @@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct
> > ast_vhub_dev *d, u8 addr)
> >
> > /* Find a free one (no device) */
> > spin_lock_irqsave(&vhub->lock, flags);
> > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > + for (i = 0; i < vhub->max_epns; i++)
> > if (vhub->epns[i].dev == NULL)
> > break;
> > - if (i >= AST_VHUB_NUM_GEN_EPs) {
> > + if (i >= vhub->max_epns) {
> > spin_unlock_irqrestore(&vhub->lock, flags);
> > return NULL;
> > }
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > index 19b3517e04c0..9c7e57fbd8ef 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > @@ -133,10 +133,13 @@ static const struct ast_vhub_full_cdesc {
> >
> > #define AST_VHUB_HUB_DESC_SIZE (USB_DT_HUB_NONVAR_SIZE + 2)
> >
> > -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> > +/*
> > + * "bNbrPorts" field is updated in "ast_vhub_init_hub" function
> > based on
> > + * "max_ports" of the vhub.
> > + */
> > +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> > .bDescLength = AST_VHUB_HUB_DESC_SIZE,
> > .bDescriptorType = USB_DT_HUB,
> > - .bNbrPorts = AST_VHUB_NUM_PORTS,
> > .wHubCharacteristics =
> > cpu_to_le16(HUB_CHAR_NO_LPSM),
> > .bPwrOn2PwrGood = 10,
> > .bHubContrCurrent = 0,
> > @@ -504,7 +507,7 @@ static void ast_vhub_wake_work(struct
> > work_struct *work)
> > * we let the normal host wake path deal with it later.
> > */
> > spin_lock_irqsave(&vhub->lock, flags);
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > @@ -587,7 +590,7 @@ static enum std_req_rc
> > ast_vhub_set_port_feature(struct ast_vhub_ep *ep,
> > struct ast_vhub *vhub = ep->vhub;
> > struct ast_vhub_port *p;
> >
> > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > + if (port == 0 || port > vhub->max_ports)
> > return std_req_stall;
> > port--;
> > p = &vhub->ports[port];
> > @@ -630,7 +633,7 @@ static enum std_req_rc
> > ast_vhub_clr_port_feature(struct ast_vhub_ep *ep,
> > struct ast_vhub *vhub = ep->vhub;
> > struct ast_vhub_port *p;
> >
> > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > + if (port == 0 || port > vhub->max_ports)
> > return std_req_stall;
> > port--;
> > p = &vhub->ports[port];
> > @@ -676,7 +679,7 @@ static enum std_req_rc
> > ast_vhub_get_port_stat(struct ast_vhub_ep *ep,
> > struct ast_vhub *vhub = ep->vhub;
> > u16 stat, chg;
> >
> > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > + if (port == 0 || port > vhub->max_ports)
> > return std_req_stall;
> > port--;
> >
> > @@ -757,7 +760,7 @@ void ast_vhub_hub_suspend(struct ast_vhub
> > *vhub)
> > * Forward to unsuspended ports without changing
> > * their connection status.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > @@ -780,7 +783,7 @@ void ast_vhub_hub_resume(struct ast_vhub *vhub)
> > * Forward to unsuspended ports without changing
> > * their connection status.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > @@ -814,7 +817,7 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
> > * Clear all port status, disable gadgets and "suspend"
> > * them. They will be woken up by a port reset.
> > */
> > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > + for (i = 0; i < vhub->max_ports; i++) {
> > struct ast_vhub_port *p = &vhub->ports[i];
> >
> > /* Only keep the connected flag */
> > @@ -838,5 +841,10 @@ void ast_vhub_init_hub(struct ast_vhub *vhub)
> > {
> > vhub->speed = USB_SPEED_UNKNOWN;
> > INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
> > +
> > + /*
> > + * Fixup number of ports in hub descriptor.
> > + */
> > + ast_vhub_hub_desc.bNbrPorts = vhub->max_ports;
> > }
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > index 761919e220d3..e46980fe66f2 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > @@ -76,17 +76,9 @@
> > #define VHUB_SW_RESET_DEVICE2 (1 << 2)
> > #define VHUB_SW_RESET_DEVICE1 (1 << 1)
> > #define VHUB_SW_RESET_ROOT_HUB (1 << 0)
> > -#define
> > VHUB_SW_RESET_ALL (VHUB_SW_RESET_EP_POOL | \
> > - VHUB_SW_RESET_DMA_
> > CONTROLLER | \
> > - VHUB_SW_RESET_DEVI
> > CE5 | \
> > - VHUB_SW_RESET_DEVI
> > CE4 | \
> > - VHUB_SW_RESET_DEVI
> > CE3 | \
> > - VHUB_SW_RESET_DEVI
> > CE2 | \
> > - VHUB_SW_RESET_DEVI
> > CE1 | \
> > - VHUB_SW_RESET_ROOT
> > _HUB)
> > +
> > /* EP ACK/NACK IRQ masks */
> > #define VHUB_EP_IRQ(n) (1 << (n))
> > -#define VHUB_EP_IRQ_ALL 0x7fff /*
> > 15 EPs */
> >
> > /* USB status reg */
> > #define VHUB_USBSTS_HISPEED (1 << 27)
> > @@ -210,8 +202,6 @@
> > * *
> > ****************************************/
> >
> > -#define AST_VHUB_NUM_GEN_EPs 15 /* Generic non-0 EPs */
> > -#define AST_VHUB_NUM_PORTS 5 /* vHub ports */
> > #define AST_VHUB_EP0_MAX_PACKET 64 /* EP0's max packet
> > size */
> > #define AST_VHUB_EPn_MAX_PACKET 1024 /* Generic EPs max
> > packet size */
> > #define AST_VHUB_DESCS_COUNT 256 /* Use 256 descriptor mode
> > (valid
> > @@ -342,7 +332,7 @@ struct ast_vhub_dev {
> > struct ast_vhub *vhub;
> > void __iomem *regs;
> >
> > - /* Device index (0...4) and name string */
> > + /* Device index (zero-based) and name string */
> > unsigned int index;
> > const char *name;
> >
> > @@ -358,7 +348,8 @@ struct ast_vhub_dev {
> >
> > /* Endpoint structures */
> > struct ast_vhub_ep ep0;
> > - struct
> > ast_vhub_ep *epns[AST_VHUB_NUM_GEN_EPs];
> > + struct ast_vhub_ep **epns;
> > + u32 max_epns;
> >
> > };
> > #define to_ast_dev(__g) container_of(__g, struct ast_vhub_dev,
> > gadget)
> > @@ -393,10 +384,12 @@ struct ast_vhub {
> > bool ep1_stalled : 1;
> >
> > /* Per-port info */
> > - struct ast_vhub_port ports[AST_VHUB_NUM_PORTS];
> > + struct ast_vhub_port *ports;
> > + u32 max_ports;
> >
> > /* Generic EP data structures */
> > - struct ast_vhub_ep epns[AST_VHUB_NUM_GEN_EPs];
> > + struct ast_vhub_ep *epns;
> > + u32 max_epns;
> >
> > /* Upstream bus is suspended ? */
> > bool suspended : 1;
> > --
> > 2.17.1
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
2020-02-10 7:27 ` Benjamin Herrenschmidt
(?)
@ 2020-02-10 19:07 ` Tao Ren
-1 siblings, 0 replies; 42+ messages in thread
From: Tao Ren @ 2020-02-10 19:07 UTC (permalink / raw)
To: linux-aspeed
On Mon, Feb 10, 2020 at 08:27:20AM +0100, Benjamin Herrenschmidt wrote:
> On Mon, 2020-02-10 at 02:47 +0000, Joel Stanley wrote:
> > On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
> > >
> > > From: Tao Ren <rentao.bupt@gmail.com>
> > >
> > > The patch moves hardcoded vhub attributes (maximum downstream ports
> > > and
> > > generic endpoints) to "ast_vhub_config" structure which is attached
> > > to
> > > struct of_device_id. The major purpose is to add AST2600 vhub
> > > support
> > > because AST2600 vhub provides more downstream ports and endpoints.
> > >
> > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> >
> > This looks generally okay. We should wait for Ben's ack before
> > applying.
>
> Shouldn't we instead have DT fields indicating those values ?
May I ask why we prefer adding dt fields (such as "aspeed,vhub-max-ports"
and "aspeed,vhub-max-endpoints") instead of assigning these values based
on aspeed family? For example, is it to allow users to set a smaller
number of ports/endpoints?
>
> Also we should add a DT representation for the various ID/strings of
> the hub itself so manufacturers can customize them.
Sure. I will add DT nodes for vendor/product/device IDs/strings. As it's
not directly related to ast2600-support, shall I handle it in a separate
patch? Or I can include the patch in this patch series?
Cheers,
Tao
>
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> >
> > > ---
> > > drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--
> > > ------
> > > drivers/usb/gadget/udc/aspeed-vhub/dev.c | 30 +++++--
> > > drivers/usb/gadget/udc/aspeed-vhub/epn.c | 4 +-
> > > drivers/usb/gadget/udc/aspeed-vhub/hub.c | 26 ++++--
> > > drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 23 ++---
> > > 5 files changed, 112 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > index 90b134d5dca9..94081cc04113 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > @@ -32,6 +32,29 @@
> > >
> > > #include "vhub.h"
> > >
> > > +struct ast_vhub_config {
> > > + u32 max_ports; /* max number of downstream ports */
> > > + u32 max_epns; /* max number of generic endpoints */
> > > +};
> > > +
> > > +static const struct ast_vhub_config ast2400_config = {
> > > + .max_ports = 5,
> > > + .max_epns = 15,
> > > +};
> > > +
> > > +static const struct of_device_id ast_vhub_dt_ids[] = {
> > > + {
> > > + .compatible = "aspeed,ast2400-usb-vhub",
> > > + .data = &ast2400_config,
> > > + },
> > > + {
> > > + .compatible = "aspeed,ast2500-usb-vhub",
> > > + .data = &ast2400_config,
> > > + },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > > +
> > > void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req
> > > *req,
> > > int status)
> > > {
> > > @@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > > {
> > > struct ast_vhub *vhub = data;
> > > irqreturn_t iret = IRQ_NONE;
> > > - u32 istat;
> > > + u32 i, istat;
> > >
> > > /* Stale interrupt while tearing down */
> > > if (!vhub->ep0_bufs)
> > > @@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > >
> > > /* Handle generic EPs first */
> > > if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> > > - u32 i, ep_acks = readl(vhub->regs +
> > > AST_VHUB_EP_ACK_ISR);
> > > + u32 ep_acks = readl(vhub->regs +
> > > AST_VHUB_EP_ACK_ISR);
> > > writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > >
> > > - for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs;
> > > i++) {
> > > + for (i = 0; ep_acks && i < vhub->max_epns; i++) {
> > > u32 mask = VHUB_EP_IRQ(i);
> > > if (ep_acks & mask) {
> > > ast_vhub_epn_ack_irq(&vhub-
> > > >epns[i]);
> > > @@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > > }
> > >
> > > /* Handle device interrupts */
> > > - if (istat & (VHUB_IRQ_DEVICE1 |
> > > - VHUB_IRQ_DEVICE2 |
> > > - VHUB_IRQ_DEVICE3 |
> > > - VHUB_IRQ_DEVICE4 |
> > > - VHUB_IRQ_DEVICE5)) {
> > > - if (istat & VHUB_IRQ_DEVICE1)
> > > - ast_vhub_dev_irq(&vhub->ports[0].dev);
> > > - if (istat & VHUB_IRQ_DEVICE2)
> > > - ast_vhub_dev_irq(&vhub->ports[1].dev);
> > > - if (istat & VHUB_IRQ_DEVICE3)
> > > - ast_vhub_dev_irq(&vhub->ports[2].dev);
> > > - if (istat & VHUB_IRQ_DEVICE4)
> > > - ast_vhub_dev_irq(&vhub->ports[3].dev);
> > > - if (istat & VHUB_IRQ_DEVICE5)
> > > - ast_vhub_dev_irq(&vhub->ports[4].dev);
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > + u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
> > > +
> > > + if (istat & dev_mask)
> > > + ast_vhub_dev_irq(&vhub->ports[i].dev);
> > > }
> > >
> > > /* Handle top-level vHub EP0 interrupts */
> > > @@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > >
> > > void ast_vhub_init_hw(struct ast_vhub *vhub)
> > > {
> > > - u32 ctrl;
> > > + u32 ctrl, port_mask, epn_mask;
> > >
> > > UDCDBG(vhub,"(Re)Starting HW ...\n");
> > >
> > > @@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
> > > }
> > >
> > > /* Reset all devices */
> > > - writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
> > > + port_mask = GENMASK(vhub->max_ports, 1);
> > > + writel(VHUB_SW_RESET_ROOT_HUB |
> > > + VHUB_SW_RESET_DMA_CONTROLLER |
> > > + VHUB_SW_RESET_EP_POOL |
> > > + port_mask, vhub->regs + AST_VHUB_SW_RESET);
> > > udelay(1);
> > > writel(0, vhub->regs + AST_VHUB_SW_RESET);
> > >
> > > /* Disable and cleanup EP ACK/NACK interrupts */
> > > + epn_mask = GENMASK(vhub->max_epns - 1, 0);
> > > writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
> > > writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> > > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
> > > + writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > > + writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
> > >
> > > /* Default settings for EP0, enable HW hub EP1 */
> > > writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> > > @@ -273,7 +291,7 @@ static int ast_vhub_remove(struct
> > > platform_device *pdev)
> > > return 0;
> > >
> > > /* Remove devices */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> > > + for (i = 0; i < vhub->max_ports; i++)
> > > ast_vhub_del_dev(&vhub->ports[i].dev);
> > >
> > > spin_lock_irqsave(&vhub->lock, flags);
> > > @@ -295,7 +313,7 @@ static int ast_vhub_remove(struct
> > > platform_device *pdev)
> > > if (vhub->ep0_bufs)
> > > dma_free_coherent(&pdev->dev,
> > > AST_VHUB_EP0_MAX_PACKET *
> > > - (AST_VHUB_NUM_PORTS + 1),
> > > + (vhub->max_ports + 1),
> > > vhub->ep0_bufs,
> > > vhub->ep0_bufs_dma);
> > > vhub->ep0_bufs = NULL;
> > > @@ -309,11 +327,30 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > struct ast_vhub *vhub;
> > > struct resource *res;
> > > int i, rc = 0;
> > > + const struct of_device_id *ofdid;
> > > + const struct ast_vhub_config *config;
> > >
> > > vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
> > > if (!vhub)
> > > return -ENOMEM;
> > >
> > > + ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
> > > + if (!ofdid)
> > > + return -EINVAL;
> > > + config = ofdid->data;
> > > +
> > > + vhub->max_ports = config->max_ports;
> > > + vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
> > > + sizeof(*vhub->ports),
> > > GFP_KERNEL);
> > > + if (!vhub->ports)
> > > + return -ENOMEM;
> > > +
> > > + vhub->max_epns = config->max_epns;
> > > + vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
> > > + sizeof(*vhub->epns), GFP_KERNEL);
> > > + if (!vhub->epns)
> > > + return -ENOMEM;
> > > +
> > > spin_lock_init(&vhub->lock);
> > > vhub->pdev = pdev;
> > >
> > > @@ -366,7 +403,7 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > */
> > > vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
> > > AST_VHUB_EP0_MAX_PACKET
> > > *
> > > - (AST_VHUB_NUM_PORTS +
> > > 1),
> > > + (vhub->max_ports + 1),
> > > &vhub->ep0_bufs_dma,
> > > GFP_KERNEL);
> > > if (!vhub->ep0_bufs) {
> > > dev_err(&pdev->dev, "Failed to allocate EP0 DMA
> > > buffers\n");
> > > @@ -380,7 +417,7 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
> > >
> > > /* Init devices */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
> > > + for (i = 0; i < vhub->max_ports && rc == 0; i++)
> > > rc = ast_vhub_init_dev(vhub, i);
> > > if (rc)
> > > goto err;
> > > @@ -400,17 +437,6 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > return rc;
> > > }
> > >
> > > -static const struct of_device_id ast_vhub_dt_ids[] = {
> > > - {
> > > - .compatible = "aspeed,ast2400-usb-vhub",
> > > - },
> > > - {
> > > - .compatible = "aspeed,ast2500-usb-vhub",
> > > - },
> > > - { }
> > > -};
> > > -MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > > -
> > > static struct platform_driver ast_vhub_driver = {
> > > .probe = ast_vhub_probe,
> > > .remove = ast_vhub_remove,
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > index 4008e7a51188..d268306a7bfe 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > @@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct
> > > ast_vhub_dev *d)
> > > writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
> > >
> > > /* Clear stall on all EPs */
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > > + for (i = 0; i < d->max_epns; i++) {
> > > struct ast_vhub_ep *ep = d->epns[i];
> > >
> > > if (ep && (ep->epn.stalled || ep->epn.wedged)) {
> > > @@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct
> > > ast_vhub_dev *d,
> > > is_set ? "SET" : "CLEAR", ep_num, wValue);
> > > if (ep_num == 0)
> > > return std_req_complete;
> > > - if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
> > > + if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
> > > return std_req_stall;
> > > if (wValue != USB_ENDPOINT_HALT)
> > > return std_req_driver;
> > > @@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct
> > > ast_vhub_dev *d,
> > >
> > > DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
> > >
> > > - if (ep_num >= AST_VHUB_NUM_GEN_EPs)
> > > + if (ep_num >= d->max_epns)
> > > return std_req_stall;
> > > if (ep_num != 0) {
> > > ep = d->epns[ep_num - 1];
> > > @@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct
> > > ast_vhub_dev *d)
> > > {
> > > unsigned int i;
> > >
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > > + for (i = 0; i < d->max_epns; i++) {
> > > if (!d->epns[i])
> > > continue;
> > > ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
> > > @@ -416,10 +416,10 @@ static struct usb_ep
> > > *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
> > > * that will allow the generic code to use our
> > > * assigned address.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > > + for (i = 0; i < d->max_epns; i++)
> > > if (d->epns[i] == NULL)
> > > break;
> > > - if (i >= AST_VHUB_NUM_GEN_EPs)
> > > + if (i >= d->max_epns)
> > > return NULL;
> > > addr = i + 1;
> > >
> > > @@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
> > >
> > > usb_del_gadget_udc(&d->gadget);
> > > device_unregister(d->port_dev);
> > > + kfree(d->epns);
> > > }
> > >
> > > static void ast_vhub_dev_release(struct device *dev)
> > > @@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > > unsigned int idx)
> > >
> > > ast_vhub_init_ep0(vhub, &d->ep0, d);
> > >
> > > + /*
> > > + * A USB device can have up to 30 endpoints besides control
> > > + * endpoint 0.
> > > + */
> > > + d->max_epns = min_t(u32, vhub->max_epns, 30);
> > > + d->epns = kcalloc(d->max_epns, sizeof(*d->epns),
> > > GFP_KERNEL);
> > > + if (!d->epns)
> > > + return -ENOMEM;
> > > +
> > > /*
> > > * The UDC core really needs us to have separate and
> > > uniquely
> > > * named "parent" devices for each port so we create a sub
> > > device
> > > * here for that purpose
> > > */
> > > d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> > > - if (!d->port_dev)
> > > - return -ENOMEM;
> > > + if (!d->port_dev) {
> > > + rc = -ENOMEM;
> > > + goto fail_alloc;
> > > + }
> > > device_initialize(d->port_dev);
> > > d->port_dev->release = ast_vhub_dev_release;
> > > d->port_dev->parent = parent;
> > > @@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > > unsigned int idx)
> > > device_del(d->port_dev);
> > > fail_add:
> > > put_device(d->port_dev);
> > > + fail_alloc:
> > > + kfree(d->epns);
> > >
> > > return rc;
> > > }
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > index 7475c74aa5c5..0bd6b20435b8 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > @@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct
> > > ast_vhub_dev *d, u8 addr)
> > >
> > > /* Find a free one (no device) */
> > > spin_lock_irqsave(&vhub->lock, flags);
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > > + for (i = 0; i < vhub->max_epns; i++)
> > > if (vhub->epns[i].dev == NULL)
> > > break;
> > > - if (i >= AST_VHUB_NUM_GEN_EPs) {
> > > + if (i >= vhub->max_epns) {
> > > spin_unlock_irqrestore(&vhub->lock, flags);
> > > return NULL;
> > > }
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > index 19b3517e04c0..9c7e57fbd8ef 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > @@ -133,10 +133,13 @@ static const struct ast_vhub_full_cdesc {
> > >
> > > #define AST_VHUB_HUB_DESC_SIZE (USB_DT_HUB_NONVAR_SIZE + 2)
> > >
> > > -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> > > +/*
> > > + * "bNbrPorts" field is updated in "ast_vhub_init_hub" function
> > > based on
> > > + * "max_ports" of the vhub.
> > > + */
> > > +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> > > .bDescLength = AST_VHUB_HUB_DESC_SIZE,
> > > .bDescriptorType = USB_DT_HUB,
> > > - .bNbrPorts = AST_VHUB_NUM_PORTS,
> > > .wHubCharacteristics =
> > > cpu_to_le16(HUB_CHAR_NO_LPSM),
> > > .bPwrOn2PwrGood = 10,
> > > .bHubContrCurrent = 0,
> > > @@ -504,7 +507,7 @@ static void ast_vhub_wake_work(struct
> > > work_struct *work)
> > > * we let the normal host wake path deal with it later.
> > > */
> > > spin_lock_irqsave(&vhub->lock, flags);
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > > @@ -587,7 +590,7 @@ static enum std_req_rc
> > > ast_vhub_set_port_feature(struct ast_vhub_ep *ep,
> > > struct ast_vhub *vhub = ep->vhub;
> > > struct ast_vhub_port *p;
> > >
> > > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > > + if (port == 0 || port > vhub->max_ports)
> > > return std_req_stall;
> > > port--;
> > > p = &vhub->ports[port];
> > > @@ -630,7 +633,7 @@ static enum std_req_rc
> > > ast_vhub_clr_port_feature(struct ast_vhub_ep *ep,
> > > struct ast_vhub *vhub = ep->vhub;
> > > struct ast_vhub_port *p;
> > >
> > > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > > + if (port == 0 || port > vhub->max_ports)
> > > return std_req_stall;
> > > port--;
> > > p = &vhub->ports[port];
> > > @@ -676,7 +679,7 @@ static enum std_req_rc
> > > ast_vhub_get_port_stat(struct ast_vhub_ep *ep,
> > > struct ast_vhub *vhub = ep->vhub;
> > > u16 stat, chg;
> > >
> > > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > > + if (port == 0 || port > vhub->max_ports)
> > > return std_req_stall;
> > > port--;
> > >
> > > @@ -757,7 +760,7 @@ void ast_vhub_hub_suspend(struct ast_vhub
> > > *vhub)
> > > * Forward to unsuspended ports without changing
> > > * their connection status.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > > @@ -780,7 +783,7 @@ void ast_vhub_hub_resume(struct ast_vhub *vhub)
> > > * Forward to unsuspended ports without changing
> > > * their connection status.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > > @@ -814,7 +817,7 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
> > > * Clear all port status, disable gadgets and "suspend"
> > > * them. They will be woken up by a port reset.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > /* Only keep the connected flag */
> > > @@ -838,5 +841,10 @@ void ast_vhub_init_hub(struct ast_vhub *vhub)
> > > {
> > > vhub->speed = USB_SPEED_UNKNOWN;
> > > INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
> > > +
> > > + /*
> > > + * Fixup number of ports in hub descriptor.
> > > + */
> > > + ast_vhub_hub_desc.bNbrPorts = vhub->max_ports;
> > > }
> > >
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > index 761919e220d3..e46980fe66f2 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > @@ -76,17 +76,9 @@
> > > #define VHUB_SW_RESET_DEVICE2 (1 << 2)
> > > #define VHUB_SW_RESET_DEVICE1 (1 << 1)
> > > #define VHUB_SW_RESET_ROOT_HUB (1 << 0)
> > > -#define
> > > VHUB_SW_RESET_ALL (VHUB_SW_RESET_EP_POOL | \
> > > - VHUB_SW_RESET_DMA_
> > > CONTROLLER | \
> > > - VHUB_SW_RESET_DEVI
> > > CE5 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE4 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE3 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE2 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE1 | \
> > > - VHUB_SW_RESET_ROOT
> > > _HUB)
> > > +
> > > /* EP ACK/NACK IRQ masks */
> > > #define VHUB_EP_IRQ(n) (1 << (n))
> > > -#define VHUB_EP_IRQ_ALL 0x7fff /*
> > > 15 EPs */
> > >
> > > /* USB status reg */
> > > #define VHUB_USBSTS_HISPEED (1 << 27)
> > > @@ -210,8 +202,6 @@
> > > * *
> > > ****************************************/
> > >
> > > -#define AST_VHUB_NUM_GEN_EPs 15 /* Generic non-0 EPs */
> > > -#define AST_VHUB_NUM_PORTS 5 /* vHub ports */
> > > #define AST_VHUB_EP0_MAX_PACKET 64 /* EP0's max packet
> > > size */
> > > #define AST_VHUB_EPn_MAX_PACKET 1024 /* Generic EPs max
> > > packet size */
> > > #define AST_VHUB_DESCS_COUNT 256 /* Use 256 descriptor mode
> > > (valid
> > > @@ -342,7 +332,7 @@ struct ast_vhub_dev {
> > > struct ast_vhub *vhub;
> > > void __iomem *regs;
> > >
> > > - /* Device index (0...4) and name string */
> > > + /* Device index (zero-based) and name string */
> > > unsigned int index;
> > > const char *name;
> > >
> > > @@ -358,7 +348,8 @@ struct ast_vhub_dev {
> > >
> > > /* Endpoint structures */
> > > struct ast_vhub_ep ep0;
> > > - struct
> > > ast_vhub_ep *epns[AST_VHUB_NUM_GEN_EPs];
> > > + struct ast_vhub_ep **epns;
> > > + u32 max_epns;
> > >
> > > };
> > > #define to_ast_dev(__g) container_of(__g, struct ast_vhub_dev,
> > > gadget)
> > > @@ -393,10 +384,12 @@ struct ast_vhub {
> > > bool ep1_stalled : 1;
> > >
> > > /* Per-port info */
> > > - struct ast_vhub_port ports[AST_VHUB_NUM_PORTS];
> > > + struct ast_vhub_port *ports;
> > > + u32 max_ports;
> > >
> > > /* Generic EP data structures */
> > > - struct ast_vhub_ep epns[AST_VHUB_NUM_GEN_EPs];
> > > + struct ast_vhub_ep *epns;
> > > + u32 max_epns;
> > >
> > > /* Upstream bus is suspended ? */
> > > bool suspended : 1;
> > > --
> > > 2.17.1
> > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-10 19:07 ` Tao Ren
0 siblings, 0 replies; 42+ messages in thread
From: Tao Ren @ 2020-02-10 19:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Mark Rutland, Felipe Balbi, linux-aspeed, devicetree,
Andrew Jeffery, Greg Kroah-Hartman, OpenBMC Maillist, linux-usb,
Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
Joel Stanley, Chunfeng Yun, Colin Ian King, Linux ARM
On Mon, Feb 10, 2020 at 08:27:20AM +0100, Benjamin Herrenschmidt wrote:
> On Mon, 2020-02-10 at 02:47 +0000, Joel Stanley wrote:
> > On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
> > >
> > > From: Tao Ren <rentao.bupt@gmail.com>
> > >
> > > The patch moves hardcoded vhub attributes (maximum downstream ports
> > > and
> > > generic endpoints) to "ast_vhub_config" structure which is attached
> > > to
> > > struct of_device_id. The major purpose is to add AST2600 vhub
> > > support
> > > because AST2600 vhub provides more downstream ports and endpoints.
> > >
> > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> >
> > This looks generally okay. We should wait for Ben's ack before
> > applying.
>
> Shouldn't we instead have DT fields indicating those values ?
May I ask why we prefer adding dt fields (such as "aspeed,vhub-max-ports"
and "aspeed,vhub-max-endpoints") instead of assigning these values based
on aspeed family? For example, is it to allow users to set a smaller
number of ports/endpoints?
>
> Also we should add a DT representation for the various ID/strings of
> the hub itself so manufacturers can customize them.
Sure. I will add DT nodes for vendor/product/device IDs/strings. As it's
not directly related to ast2600-support, shall I handle it in a separate
patch? Or I can include the patch in this patch series?
Cheers,
Tao
>
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> >
> > > ---
> > > drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--
> > > ------
> > > drivers/usb/gadget/udc/aspeed-vhub/dev.c | 30 +++++--
> > > drivers/usb/gadget/udc/aspeed-vhub/epn.c | 4 +-
> > > drivers/usb/gadget/udc/aspeed-vhub/hub.c | 26 ++++--
> > > drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 23 ++---
> > > 5 files changed, 112 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > index 90b134d5dca9..94081cc04113 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > @@ -32,6 +32,29 @@
> > >
> > > #include "vhub.h"
> > >
> > > +struct ast_vhub_config {
> > > + u32 max_ports; /* max number of downstream ports */
> > > + u32 max_epns; /* max number of generic endpoints */
> > > +};
> > > +
> > > +static const struct ast_vhub_config ast2400_config = {
> > > + .max_ports = 5,
> > > + .max_epns = 15,
> > > +};
> > > +
> > > +static const struct of_device_id ast_vhub_dt_ids[] = {
> > > + {
> > > + .compatible = "aspeed,ast2400-usb-vhub",
> > > + .data = &ast2400_config,
> > > + },
> > > + {
> > > + .compatible = "aspeed,ast2500-usb-vhub",
> > > + .data = &ast2400_config,
> > > + },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > > +
> > > void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req
> > > *req,
> > > int status)
> > > {
> > > @@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > > {
> > > struct ast_vhub *vhub = data;
> > > irqreturn_t iret = IRQ_NONE;
> > > - u32 istat;
> > > + u32 i, istat;
> > >
> > > /* Stale interrupt while tearing down */
> > > if (!vhub->ep0_bufs)
> > > @@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > >
> > > /* Handle generic EPs first */
> > > if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> > > - u32 i, ep_acks = readl(vhub->regs +
> > > AST_VHUB_EP_ACK_ISR);
> > > + u32 ep_acks = readl(vhub->regs +
> > > AST_VHUB_EP_ACK_ISR);
> > > writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > >
> > > - for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs;
> > > i++) {
> > > + for (i = 0; ep_acks && i < vhub->max_epns; i++) {
> > > u32 mask = VHUB_EP_IRQ(i);
> > > if (ep_acks & mask) {
> > > ast_vhub_epn_ack_irq(&vhub-
> > > >epns[i]);
> > > @@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > > }
> > >
> > > /* Handle device interrupts */
> > > - if (istat & (VHUB_IRQ_DEVICE1 |
> > > - VHUB_IRQ_DEVICE2 |
> > > - VHUB_IRQ_DEVICE3 |
> > > - VHUB_IRQ_DEVICE4 |
> > > - VHUB_IRQ_DEVICE5)) {
> > > - if (istat & VHUB_IRQ_DEVICE1)
> > > - ast_vhub_dev_irq(&vhub->ports[0].dev);
> > > - if (istat & VHUB_IRQ_DEVICE2)
> > > - ast_vhub_dev_irq(&vhub->ports[1].dev);
> > > - if (istat & VHUB_IRQ_DEVICE3)
> > > - ast_vhub_dev_irq(&vhub->ports[2].dev);
> > > - if (istat & VHUB_IRQ_DEVICE4)
> > > - ast_vhub_dev_irq(&vhub->ports[3].dev);
> > > - if (istat & VHUB_IRQ_DEVICE5)
> > > - ast_vhub_dev_irq(&vhub->ports[4].dev);
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > + u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
> > > +
> > > + if (istat & dev_mask)
> > > + ast_vhub_dev_irq(&vhub->ports[i].dev);
> > > }
> > >
> > > /* Handle top-level vHub EP0 interrupts */
> > > @@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > >
> > > void ast_vhub_init_hw(struct ast_vhub *vhub)
> > > {
> > > - u32 ctrl;
> > > + u32 ctrl, port_mask, epn_mask;
> > >
> > > UDCDBG(vhub,"(Re)Starting HW ...\n");
> > >
> > > @@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
> > > }
> > >
> > > /* Reset all devices */
> > > - writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
> > > + port_mask = GENMASK(vhub->max_ports, 1);
> > > + writel(VHUB_SW_RESET_ROOT_HUB |
> > > + VHUB_SW_RESET_DMA_CONTROLLER |
> > > + VHUB_SW_RESET_EP_POOL |
> > > + port_mask, vhub->regs + AST_VHUB_SW_RESET);
> > > udelay(1);
> > > writel(0, vhub->regs + AST_VHUB_SW_RESET);
> > >
> > > /* Disable and cleanup EP ACK/NACK interrupts */
> > > + epn_mask = GENMASK(vhub->max_epns - 1, 0);
> > > writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
> > > writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> > > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
> > > + writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > > + writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
> > >
> > > /* Default settings for EP0, enable HW hub EP1 */
> > > writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> > > @@ -273,7 +291,7 @@ static int ast_vhub_remove(struct
> > > platform_device *pdev)
> > > return 0;
> > >
> > > /* Remove devices */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> > > + for (i = 0; i < vhub->max_ports; i++)
> > > ast_vhub_del_dev(&vhub->ports[i].dev);
> > >
> > > spin_lock_irqsave(&vhub->lock, flags);
> > > @@ -295,7 +313,7 @@ static int ast_vhub_remove(struct
> > > platform_device *pdev)
> > > if (vhub->ep0_bufs)
> > > dma_free_coherent(&pdev->dev,
> > > AST_VHUB_EP0_MAX_PACKET *
> > > - (AST_VHUB_NUM_PORTS + 1),
> > > + (vhub->max_ports + 1),
> > > vhub->ep0_bufs,
> > > vhub->ep0_bufs_dma);
> > > vhub->ep0_bufs = NULL;
> > > @@ -309,11 +327,30 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > struct ast_vhub *vhub;
> > > struct resource *res;
> > > int i, rc = 0;
> > > + const struct of_device_id *ofdid;
> > > + const struct ast_vhub_config *config;
> > >
> > > vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
> > > if (!vhub)
> > > return -ENOMEM;
> > >
> > > + ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
> > > + if (!ofdid)
> > > + return -EINVAL;
> > > + config = ofdid->data;
> > > +
> > > + vhub->max_ports = config->max_ports;
> > > + vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
> > > + sizeof(*vhub->ports),
> > > GFP_KERNEL);
> > > + if (!vhub->ports)
> > > + return -ENOMEM;
> > > +
> > > + vhub->max_epns = config->max_epns;
> > > + vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
> > > + sizeof(*vhub->epns), GFP_KERNEL);
> > > + if (!vhub->epns)
> > > + return -ENOMEM;
> > > +
> > > spin_lock_init(&vhub->lock);
> > > vhub->pdev = pdev;
> > >
> > > @@ -366,7 +403,7 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > */
> > > vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
> > > AST_VHUB_EP0_MAX_PACKET
> > > *
> > > - (AST_VHUB_NUM_PORTS +
> > > 1),
> > > + (vhub->max_ports + 1),
> > > &vhub->ep0_bufs_dma,
> > > GFP_KERNEL);
> > > if (!vhub->ep0_bufs) {
> > > dev_err(&pdev->dev, "Failed to allocate EP0 DMA
> > > buffers\n");
> > > @@ -380,7 +417,7 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
> > >
> > > /* Init devices */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
> > > + for (i = 0; i < vhub->max_ports && rc == 0; i++)
> > > rc = ast_vhub_init_dev(vhub, i);
> > > if (rc)
> > > goto err;
> > > @@ -400,17 +437,6 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > return rc;
> > > }
> > >
> > > -static const struct of_device_id ast_vhub_dt_ids[] = {
> > > - {
> > > - .compatible = "aspeed,ast2400-usb-vhub",
> > > - },
> > > - {
> > > - .compatible = "aspeed,ast2500-usb-vhub",
> > > - },
> > > - { }
> > > -};
> > > -MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > > -
> > > static struct platform_driver ast_vhub_driver = {
> > > .probe = ast_vhub_probe,
> > > .remove = ast_vhub_remove,
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > index 4008e7a51188..d268306a7bfe 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > @@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct
> > > ast_vhub_dev *d)
> > > writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
> > >
> > > /* Clear stall on all EPs */
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > > + for (i = 0; i < d->max_epns; i++) {
> > > struct ast_vhub_ep *ep = d->epns[i];
> > >
> > > if (ep && (ep->epn.stalled || ep->epn.wedged)) {
> > > @@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct
> > > ast_vhub_dev *d,
> > > is_set ? "SET" : "CLEAR", ep_num, wValue);
> > > if (ep_num == 0)
> > > return std_req_complete;
> > > - if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
> > > + if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
> > > return std_req_stall;
> > > if (wValue != USB_ENDPOINT_HALT)
> > > return std_req_driver;
> > > @@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct
> > > ast_vhub_dev *d,
> > >
> > > DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
> > >
> > > - if (ep_num >= AST_VHUB_NUM_GEN_EPs)
> > > + if (ep_num >= d->max_epns)
> > > return std_req_stall;
> > > if (ep_num != 0) {
> > > ep = d->epns[ep_num - 1];
> > > @@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct
> > > ast_vhub_dev *d)
> > > {
> > > unsigned int i;
> > >
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > > + for (i = 0; i < d->max_epns; i++) {
> > > if (!d->epns[i])
> > > continue;
> > > ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
> > > @@ -416,10 +416,10 @@ static struct usb_ep
> > > *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
> > > * that will allow the generic code to use our
> > > * assigned address.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > > + for (i = 0; i < d->max_epns; i++)
> > > if (d->epns[i] == NULL)
> > > break;
> > > - if (i >= AST_VHUB_NUM_GEN_EPs)
> > > + if (i >= d->max_epns)
> > > return NULL;
> > > addr = i + 1;
> > >
> > > @@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
> > >
> > > usb_del_gadget_udc(&d->gadget);
> > > device_unregister(d->port_dev);
> > > + kfree(d->epns);
> > > }
> > >
> > > static void ast_vhub_dev_release(struct device *dev)
> > > @@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > > unsigned int idx)
> > >
> > > ast_vhub_init_ep0(vhub, &d->ep0, d);
> > >
> > > + /*
> > > + * A USB device can have up to 30 endpoints besides control
> > > + * endpoint 0.
> > > + */
> > > + d->max_epns = min_t(u32, vhub->max_epns, 30);
> > > + d->epns = kcalloc(d->max_epns, sizeof(*d->epns),
> > > GFP_KERNEL);
> > > + if (!d->epns)
> > > + return -ENOMEM;
> > > +
> > > /*
> > > * The UDC core really needs us to have separate and
> > > uniquely
> > > * named "parent" devices for each port so we create a sub
> > > device
> > > * here for that purpose
> > > */
> > > d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> > > - if (!d->port_dev)
> > > - return -ENOMEM;
> > > + if (!d->port_dev) {
> > > + rc = -ENOMEM;
> > > + goto fail_alloc;
> > > + }
> > > device_initialize(d->port_dev);
> > > d->port_dev->release = ast_vhub_dev_release;
> > > d->port_dev->parent = parent;
> > > @@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > > unsigned int idx)
> > > device_del(d->port_dev);
> > > fail_add:
> > > put_device(d->port_dev);
> > > + fail_alloc:
> > > + kfree(d->epns);
> > >
> > > return rc;
> > > }
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > index 7475c74aa5c5..0bd6b20435b8 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > @@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct
> > > ast_vhub_dev *d, u8 addr)
> > >
> > > /* Find a free one (no device) */
> > > spin_lock_irqsave(&vhub->lock, flags);
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > > + for (i = 0; i < vhub->max_epns; i++)
> > > if (vhub->epns[i].dev == NULL)
> > > break;
> > > - if (i >= AST_VHUB_NUM_GEN_EPs) {
> > > + if (i >= vhub->max_epns) {
> > > spin_unlock_irqrestore(&vhub->lock, flags);
> > > return NULL;
> > > }
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > index 19b3517e04c0..9c7e57fbd8ef 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > @@ -133,10 +133,13 @@ static const struct ast_vhub_full_cdesc {
> > >
> > > #define AST_VHUB_HUB_DESC_SIZE (USB_DT_HUB_NONVAR_SIZE + 2)
> > >
> > > -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> > > +/*
> > > + * "bNbrPorts" field is updated in "ast_vhub_init_hub" function
> > > based on
> > > + * "max_ports" of the vhub.
> > > + */
> > > +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> > > .bDescLength = AST_VHUB_HUB_DESC_SIZE,
> > > .bDescriptorType = USB_DT_HUB,
> > > - .bNbrPorts = AST_VHUB_NUM_PORTS,
> > > .wHubCharacteristics =
> > > cpu_to_le16(HUB_CHAR_NO_LPSM),
> > > .bPwrOn2PwrGood = 10,
> > > .bHubContrCurrent = 0,
> > > @@ -504,7 +507,7 @@ static void ast_vhub_wake_work(struct
> > > work_struct *work)
> > > * we let the normal host wake path deal with it later.
> > > */
> > > spin_lock_irqsave(&vhub->lock, flags);
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > > @@ -587,7 +590,7 @@ static enum std_req_rc
> > > ast_vhub_set_port_feature(struct ast_vhub_ep *ep,
> > > struct ast_vhub *vhub = ep->vhub;
> > > struct ast_vhub_port *p;
> > >
> > > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > > + if (port == 0 || port > vhub->max_ports)
> > > return std_req_stall;
> > > port--;
> > > p = &vhub->ports[port];
> > > @@ -630,7 +633,7 @@ static enum std_req_rc
> > > ast_vhub_clr_port_feature(struct ast_vhub_ep *ep,
> > > struct ast_vhub *vhub = ep->vhub;
> > > struct ast_vhub_port *p;
> > >
> > > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > > + if (port == 0 || port > vhub->max_ports)
> > > return std_req_stall;
> > > port--;
> > > p = &vhub->ports[port];
> > > @@ -676,7 +679,7 @@ static enum std_req_rc
> > > ast_vhub_get_port_stat(struct ast_vhub_ep *ep,
> > > struct ast_vhub *vhub = ep->vhub;
> > > u16 stat, chg;
> > >
> > > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > > + if (port == 0 || port > vhub->max_ports)
> > > return std_req_stall;
> > > port--;
> > >
> > > @@ -757,7 +760,7 @@ void ast_vhub_hub_suspend(struct ast_vhub
> > > *vhub)
> > > * Forward to unsuspended ports without changing
> > > * their connection status.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > > @@ -780,7 +783,7 @@ void ast_vhub_hub_resume(struct ast_vhub *vhub)
> > > * Forward to unsuspended ports without changing
> > > * their connection status.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > > @@ -814,7 +817,7 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
> > > * Clear all port status, disable gadgets and "suspend"
> > > * them. They will be woken up by a port reset.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > /* Only keep the connected flag */
> > > @@ -838,5 +841,10 @@ void ast_vhub_init_hub(struct ast_vhub *vhub)
> > > {
> > > vhub->speed = USB_SPEED_UNKNOWN;
> > > INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
> > > +
> > > + /*
> > > + * Fixup number of ports in hub descriptor.
> > > + */
> > > + ast_vhub_hub_desc.bNbrPorts = vhub->max_ports;
> > > }
> > >
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > index 761919e220d3..e46980fe66f2 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > @@ -76,17 +76,9 @@
> > > #define VHUB_SW_RESET_DEVICE2 (1 << 2)
> > > #define VHUB_SW_RESET_DEVICE1 (1 << 1)
> > > #define VHUB_SW_RESET_ROOT_HUB (1 << 0)
> > > -#define
> > > VHUB_SW_RESET_ALL (VHUB_SW_RESET_EP_POOL | \
> > > - VHUB_SW_RESET_DMA_
> > > CONTROLLER | \
> > > - VHUB_SW_RESET_DEVI
> > > CE5 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE4 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE3 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE2 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE1 | \
> > > - VHUB_SW_RESET_ROOT
> > > _HUB)
> > > +
> > > /* EP ACK/NACK IRQ masks */
> > > #define VHUB_EP_IRQ(n) (1 << (n))
> > > -#define VHUB_EP_IRQ_ALL 0x7fff /*
> > > 15 EPs */
> > >
> > > /* USB status reg */
> > > #define VHUB_USBSTS_HISPEED (1 << 27)
> > > @@ -210,8 +202,6 @@
> > > * *
> > > ****************************************/
> > >
> > > -#define AST_VHUB_NUM_GEN_EPs 15 /* Generic non-0 EPs */
> > > -#define AST_VHUB_NUM_PORTS 5 /* vHub ports */
> > > #define AST_VHUB_EP0_MAX_PACKET 64 /* EP0's max packet
> > > size */
> > > #define AST_VHUB_EPn_MAX_PACKET 1024 /* Generic EPs max
> > > packet size */
> > > #define AST_VHUB_DESCS_COUNT 256 /* Use 256 descriptor mode
> > > (valid
> > > @@ -342,7 +332,7 @@ struct ast_vhub_dev {
> > > struct ast_vhub *vhub;
> > > void __iomem *regs;
> > >
> > > - /* Device index (0...4) and name string */
> > > + /* Device index (zero-based) and name string */
> > > unsigned int index;
> > > const char *name;
> > >
> > > @@ -358,7 +348,8 @@ struct ast_vhub_dev {
> > >
> > > /* Endpoint structures */
> > > struct ast_vhub_ep ep0;
> > > - struct
> > > ast_vhub_ep *epns[AST_VHUB_NUM_GEN_EPs];
> > > + struct ast_vhub_ep **epns;
> > > + u32 max_epns;
> > >
> > > };
> > > #define to_ast_dev(__g) container_of(__g, struct ast_vhub_dev,
> > > gadget)
> > > @@ -393,10 +384,12 @@ struct ast_vhub {
> > > bool ep1_stalled : 1;
> > >
> > > /* Per-port info */
> > > - struct ast_vhub_port ports[AST_VHUB_NUM_PORTS];
> > > + struct ast_vhub_port *ports;
> > > + u32 max_ports;
> > >
> > > /* Generic EP data structures */
> > > - struct ast_vhub_ep epns[AST_VHUB_NUM_GEN_EPs];
> > > + struct ast_vhub_ep *epns;
> > > + u32 max_epns;
> > >
> > > /* Upstream bus is suspended ? */
> > > bool suspended : 1;
> > > --
> > > 2.17.1
> > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-10 19:07 ` Tao Ren
0 siblings, 0 replies; 42+ messages in thread
From: Tao Ren @ 2020-02-10 19:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joel Stanley, Mark Rutland, Felipe Balbi, linux-aspeed,
devicetree, Andrew Jeffery, Greg Kroah-Hartman, OpenBMC Maillist,
linux-usb, Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
Chunfeng Yun, Colin Ian King, Linux ARM
On Mon, Feb 10, 2020 at 08:27:20AM +0100, Benjamin Herrenschmidt wrote:
> On Mon, 2020-02-10 at 02:47 +0000, Joel Stanley wrote:
> > On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
> > >
> > > From: Tao Ren <rentao.bupt@gmail.com>
> > >
> > > The patch moves hardcoded vhub attributes (maximum downstream ports
> > > and
> > > generic endpoints) to "ast_vhub_config" structure which is attached
> > > to
> > > struct of_device_id. The major purpose is to add AST2600 vhub
> > > support
> > > because AST2600 vhub provides more downstream ports and endpoints.
> > >
> > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> >
> > This looks generally okay. We should wait for Ben's ack before
> > applying.
>
> Shouldn't we instead have DT fields indicating those values ?
May I ask why we prefer adding dt fields (such as "aspeed,vhub-max-ports"
and "aspeed,vhub-max-endpoints") instead of assigning these values based
on aspeed family? For example, is it to allow users to set a smaller
number of ports/endpoints?
>
> Also we should add a DT representation for the various ID/strings of
> the hub itself so manufacturers can customize them.
Sure. I will add DT nodes for vendor/product/device IDs/strings. As it's
not directly related to ast2600-support, shall I handle it in a separate
patch? Or I can include the patch in this patch series?
Cheers,
Tao
>
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> >
> > > ---
> > > drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--
> > > ------
> > > drivers/usb/gadget/udc/aspeed-vhub/dev.c | 30 +++++--
> > > drivers/usb/gadget/udc/aspeed-vhub/epn.c | 4 +-
> > > drivers/usb/gadget/udc/aspeed-vhub/hub.c | 26 ++++--
> > > drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 23 ++---
> > > 5 files changed, 112 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > index 90b134d5dca9..94081cc04113 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > @@ -32,6 +32,29 @@
> > >
> > > #include "vhub.h"
> > >
> > > +struct ast_vhub_config {
> > > + u32 max_ports; /* max number of downstream ports */
> > > + u32 max_epns; /* max number of generic endpoints */
> > > +};
> > > +
> > > +static const struct ast_vhub_config ast2400_config = {
> > > + .max_ports = 5,
> > > + .max_epns = 15,
> > > +};
> > > +
> > > +static const struct of_device_id ast_vhub_dt_ids[] = {
> > > + {
> > > + .compatible = "aspeed,ast2400-usb-vhub",
> > > + .data = &ast2400_config,
> > > + },
> > > + {
> > > + .compatible = "aspeed,ast2500-usb-vhub",
> > > + .data = &ast2400_config,
> > > + },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > > +
> > > void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req
> > > *req,
> > > int status)
> > > {
> > > @@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > > {
> > > struct ast_vhub *vhub = data;
> > > irqreturn_t iret = IRQ_NONE;
> > > - u32 istat;
> > > + u32 i, istat;
> > >
> > > /* Stale interrupt while tearing down */
> > > if (!vhub->ep0_bufs)
> > > @@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > >
> > > /* Handle generic EPs first */
> > > if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> > > - u32 i, ep_acks = readl(vhub->regs +
> > > AST_VHUB_EP_ACK_ISR);
> > > + u32 ep_acks = readl(vhub->regs +
> > > AST_VHUB_EP_ACK_ISR);
> > > writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > >
> > > - for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs;
> > > i++) {
> > > + for (i = 0; ep_acks && i < vhub->max_epns; i++) {
> > > u32 mask = VHUB_EP_IRQ(i);
> > > if (ep_acks & mask) {
> > > ast_vhub_epn_ack_irq(&vhub-
> > > >epns[i]);
> > > @@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > > }
> > >
> > > /* Handle device interrupts */
> > > - if (istat & (VHUB_IRQ_DEVICE1 |
> > > - VHUB_IRQ_DEVICE2 |
> > > - VHUB_IRQ_DEVICE3 |
> > > - VHUB_IRQ_DEVICE4 |
> > > - VHUB_IRQ_DEVICE5)) {
> > > - if (istat & VHUB_IRQ_DEVICE1)
> > > - ast_vhub_dev_irq(&vhub->ports[0].dev);
> > > - if (istat & VHUB_IRQ_DEVICE2)
> > > - ast_vhub_dev_irq(&vhub->ports[1].dev);
> > > - if (istat & VHUB_IRQ_DEVICE3)
> > > - ast_vhub_dev_irq(&vhub->ports[2].dev);
> > > - if (istat & VHUB_IRQ_DEVICE4)
> > > - ast_vhub_dev_irq(&vhub->ports[3].dev);
> > > - if (istat & VHUB_IRQ_DEVICE5)
> > > - ast_vhub_dev_irq(&vhub->ports[4].dev);
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > + u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
> > > +
> > > + if (istat & dev_mask)
> > > + ast_vhub_dev_irq(&vhub->ports[i].dev);
> > > }
> > >
> > > /* Handle top-level vHub EP0 interrupts */
> > > @@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void
> > > *data)
> > >
> > > void ast_vhub_init_hw(struct ast_vhub *vhub)
> > > {
> > > - u32 ctrl;
> > > + u32 ctrl, port_mask, epn_mask;
> > >
> > > UDCDBG(vhub,"(Re)Starting HW ...\n");
> > >
> > > @@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
> > > }
> > >
> > > /* Reset all devices */
> > > - writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
> > > + port_mask = GENMASK(vhub->max_ports, 1);
> > > + writel(VHUB_SW_RESET_ROOT_HUB |
> > > + VHUB_SW_RESET_DMA_CONTROLLER |
> > > + VHUB_SW_RESET_EP_POOL |
> > > + port_mask, vhub->regs + AST_VHUB_SW_RESET);
> > > udelay(1);
> > > writel(0, vhub->regs + AST_VHUB_SW_RESET);
> > >
> > > /* Disable and cleanup EP ACK/NACK interrupts */
> > > + epn_mask = GENMASK(vhub->max_epns - 1, 0);
> > > writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
> > > writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> > > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > > - writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
> > > + writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > > + writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
> > >
> > > /* Default settings for EP0, enable HW hub EP1 */
> > > writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> > > @@ -273,7 +291,7 @@ static int ast_vhub_remove(struct
> > > platform_device *pdev)
> > > return 0;
> > >
> > > /* Remove devices */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> > > + for (i = 0; i < vhub->max_ports; i++)
> > > ast_vhub_del_dev(&vhub->ports[i].dev);
> > >
> > > spin_lock_irqsave(&vhub->lock, flags);
> > > @@ -295,7 +313,7 @@ static int ast_vhub_remove(struct
> > > platform_device *pdev)
> > > if (vhub->ep0_bufs)
> > > dma_free_coherent(&pdev->dev,
> > > AST_VHUB_EP0_MAX_PACKET *
> > > - (AST_VHUB_NUM_PORTS + 1),
> > > + (vhub->max_ports + 1),
> > > vhub->ep0_bufs,
> > > vhub->ep0_bufs_dma);
> > > vhub->ep0_bufs = NULL;
> > > @@ -309,11 +327,30 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > struct ast_vhub *vhub;
> > > struct resource *res;
> > > int i, rc = 0;
> > > + const struct of_device_id *ofdid;
> > > + const struct ast_vhub_config *config;
> > >
> > > vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
> > > if (!vhub)
> > > return -ENOMEM;
> > >
> > > + ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
> > > + if (!ofdid)
> > > + return -EINVAL;
> > > + config = ofdid->data;
> > > +
> > > + vhub->max_ports = config->max_ports;
> > > + vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
> > > + sizeof(*vhub->ports),
> > > GFP_KERNEL);
> > > + if (!vhub->ports)
> > > + return -ENOMEM;
> > > +
> > > + vhub->max_epns = config->max_epns;
> > > + vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
> > > + sizeof(*vhub->epns), GFP_KERNEL);
> > > + if (!vhub->epns)
> > > + return -ENOMEM;
> > > +
> > > spin_lock_init(&vhub->lock);
> > > vhub->pdev = pdev;
> > >
> > > @@ -366,7 +403,7 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > */
> > > vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
> > > AST_VHUB_EP0_MAX_PACKET
> > > *
> > > - (AST_VHUB_NUM_PORTS +
> > > 1),
> > > + (vhub->max_ports + 1),
> > > &vhub->ep0_bufs_dma,
> > > GFP_KERNEL);
> > > if (!vhub->ep0_bufs) {
> > > dev_err(&pdev->dev, "Failed to allocate EP0 DMA
> > > buffers\n");
> > > @@ -380,7 +417,7 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
> > >
> > > /* Init devices */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
> > > + for (i = 0; i < vhub->max_ports && rc == 0; i++)
> > > rc = ast_vhub_init_dev(vhub, i);
> > > if (rc)
> > > goto err;
> > > @@ -400,17 +437,6 @@ static int ast_vhub_probe(struct
> > > platform_device *pdev)
> > > return rc;
> > > }
> > >
> > > -static const struct of_device_id ast_vhub_dt_ids[] = {
> > > - {
> > > - .compatible = "aspeed,ast2400-usb-vhub",
> > > - },
> > > - {
> > > - .compatible = "aspeed,ast2500-usb-vhub",
> > > - },
> > > - { }
> > > -};
> > > -MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > > -
> > > static struct platform_driver ast_vhub_driver = {
> > > .probe = ast_vhub_probe,
> > > .remove = ast_vhub_remove,
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > index 4008e7a51188..d268306a7bfe 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > > @@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct
> > > ast_vhub_dev *d)
> > > writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
> > >
> > > /* Clear stall on all EPs */
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > > + for (i = 0; i < d->max_epns; i++) {
> > > struct ast_vhub_ep *ep = d->epns[i];
> > >
> > > if (ep && (ep->epn.stalled || ep->epn.wedged)) {
> > > @@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct
> > > ast_vhub_dev *d,
> > > is_set ? "SET" : "CLEAR", ep_num, wValue);
> > > if (ep_num == 0)
> > > return std_req_complete;
> > > - if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
> > > + if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
> > > return std_req_stall;
> > > if (wValue != USB_ENDPOINT_HALT)
> > > return std_req_driver;
> > > @@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct
> > > ast_vhub_dev *d,
> > >
> > > DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
> > >
> > > - if (ep_num >= AST_VHUB_NUM_GEN_EPs)
> > > + if (ep_num >= d->max_epns)
> > > return std_req_stall;
> > > if (ep_num != 0) {
> > > ep = d->epns[ep_num - 1];
> > > @@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct
> > > ast_vhub_dev *d)
> > > {
> > > unsigned int i;
> > >
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > > + for (i = 0; i < d->max_epns; i++) {
> > > if (!d->epns[i])
> > > continue;
> > > ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
> > > @@ -416,10 +416,10 @@ static struct usb_ep
> > > *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
> > > * that will allow the generic code to use our
> > > * assigned address.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > > + for (i = 0; i < d->max_epns; i++)
> > > if (d->epns[i] == NULL)
> > > break;
> > > - if (i >= AST_VHUB_NUM_GEN_EPs)
> > > + if (i >= d->max_epns)
> > > return NULL;
> > > addr = i + 1;
> > >
> > > @@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
> > >
> > > usb_del_gadget_udc(&d->gadget);
> > > device_unregister(d->port_dev);
> > > + kfree(d->epns);
> > > }
> > >
> > > static void ast_vhub_dev_release(struct device *dev)
> > > @@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > > unsigned int idx)
> > >
> > > ast_vhub_init_ep0(vhub, &d->ep0, d);
> > >
> > > + /*
> > > + * A USB device can have up to 30 endpoints besides control
> > > + * endpoint 0.
> > > + */
> > > + d->max_epns = min_t(u32, vhub->max_epns, 30);
> > > + d->epns = kcalloc(d->max_epns, sizeof(*d->epns),
> > > GFP_KERNEL);
> > > + if (!d->epns)
> > > + return -ENOMEM;
> > > +
> > > /*
> > > * The UDC core really needs us to have separate and
> > > uniquely
> > > * named "parent" devices for each port so we create a sub
> > > device
> > > * here for that purpose
> > > */
> > > d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> > > - if (!d->port_dev)
> > > - return -ENOMEM;
> > > + if (!d->port_dev) {
> > > + rc = -ENOMEM;
> > > + goto fail_alloc;
> > > + }
> > > device_initialize(d->port_dev);
> > > d->port_dev->release = ast_vhub_dev_release;
> > > d->port_dev->parent = parent;
> > > @@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub,
> > > unsigned int idx)
> > > device_del(d->port_dev);
> > > fail_add:
> > > put_device(d->port_dev);
> > > + fail_alloc:
> > > + kfree(d->epns);
> > >
> > > return rc;
> > > }
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > index 7475c74aa5c5..0bd6b20435b8 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > > @@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct
> > > ast_vhub_dev *d, u8 addr)
> > >
> > > /* Find a free one (no device) */
> > > spin_lock_irqsave(&vhub->lock, flags);
> > > - for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > > + for (i = 0; i < vhub->max_epns; i++)
> > > if (vhub->epns[i].dev == NULL)
> > > break;
> > > - if (i >= AST_VHUB_NUM_GEN_EPs) {
> > > + if (i >= vhub->max_epns) {
> > > spin_unlock_irqrestore(&vhub->lock, flags);
> > > return NULL;
> > > }
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > index 19b3517e04c0..9c7e57fbd8ef 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > @@ -133,10 +133,13 @@ static const struct ast_vhub_full_cdesc {
> > >
> > > #define AST_VHUB_HUB_DESC_SIZE (USB_DT_HUB_NONVAR_SIZE + 2)
> > >
> > > -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> > > +/*
> > > + * "bNbrPorts" field is updated in "ast_vhub_init_hub" function
> > > based on
> > > + * "max_ports" of the vhub.
> > > + */
> > > +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> > > .bDescLength = AST_VHUB_HUB_DESC_SIZE,
> > > .bDescriptorType = USB_DT_HUB,
> > > - .bNbrPorts = AST_VHUB_NUM_PORTS,
> > > .wHubCharacteristics =
> > > cpu_to_le16(HUB_CHAR_NO_LPSM),
> > > .bPwrOn2PwrGood = 10,
> > > .bHubContrCurrent = 0,
> > > @@ -504,7 +507,7 @@ static void ast_vhub_wake_work(struct
> > > work_struct *work)
> > > * we let the normal host wake path deal with it later.
> > > */
> > > spin_lock_irqsave(&vhub->lock, flags);
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > > @@ -587,7 +590,7 @@ static enum std_req_rc
> > > ast_vhub_set_port_feature(struct ast_vhub_ep *ep,
> > > struct ast_vhub *vhub = ep->vhub;
> > > struct ast_vhub_port *p;
> > >
> > > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > > + if (port == 0 || port > vhub->max_ports)
> > > return std_req_stall;
> > > port--;
> > > p = &vhub->ports[port];
> > > @@ -630,7 +633,7 @@ static enum std_req_rc
> > > ast_vhub_clr_port_feature(struct ast_vhub_ep *ep,
> > > struct ast_vhub *vhub = ep->vhub;
> > > struct ast_vhub_port *p;
> > >
> > > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > > + if (port == 0 || port > vhub->max_ports)
> > > return std_req_stall;
> > > port--;
> > > p = &vhub->ports[port];
> > > @@ -676,7 +679,7 @@ static enum std_req_rc
> > > ast_vhub_get_port_stat(struct ast_vhub_ep *ep,
> > > struct ast_vhub *vhub = ep->vhub;
> > > u16 stat, chg;
> > >
> > > - if (port == 0 || port > AST_VHUB_NUM_PORTS)
> > > + if (port == 0 || port > vhub->max_ports)
> > > return std_req_stall;
> > > port--;
> > >
> > > @@ -757,7 +760,7 @@ void ast_vhub_hub_suspend(struct ast_vhub
> > > *vhub)
> > > * Forward to unsuspended ports without changing
> > > * their connection status.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > > @@ -780,7 +783,7 @@ void ast_vhub_hub_resume(struct ast_vhub *vhub)
> > > * Forward to unsuspended ports without changing
> > > * their connection status.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > if (!(p->status & USB_PORT_STAT_SUSPEND))
> > > @@ -814,7 +817,7 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
> > > * Clear all port status, disable gadgets and "suspend"
> > > * them. They will be woken up by a port reset.
> > > */
> > > - for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
> > > + for (i = 0; i < vhub->max_ports; i++) {
> > > struct ast_vhub_port *p = &vhub->ports[i];
> > >
> > > /* Only keep the connected flag */
> > > @@ -838,5 +841,10 @@ void ast_vhub_init_hub(struct ast_vhub *vhub)
> > > {
> > > vhub->speed = USB_SPEED_UNKNOWN;
> > > INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
> > > +
> > > + /*
> > > + * Fixup number of ports in hub descriptor.
> > > + */
> > > + ast_vhub_hub_desc.bNbrPorts = vhub->max_ports;
> > > }
> > >
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > index 761919e220d3..e46980fe66f2 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> > > @@ -76,17 +76,9 @@
> > > #define VHUB_SW_RESET_DEVICE2 (1 << 2)
> > > #define VHUB_SW_RESET_DEVICE1 (1 << 1)
> > > #define VHUB_SW_RESET_ROOT_HUB (1 << 0)
> > > -#define
> > > VHUB_SW_RESET_ALL (VHUB_SW_RESET_EP_POOL | \
> > > - VHUB_SW_RESET_DMA_
> > > CONTROLLER | \
> > > - VHUB_SW_RESET_DEVI
> > > CE5 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE4 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE3 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE2 | \
> > > - VHUB_SW_RESET_DEVI
> > > CE1 | \
> > > - VHUB_SW_RESET_ROOT
> > > _HUB)
> > > +
> > > /* EP ACK/NACK IRQ masks */
> > > #define VHUB_EP_IRQ(n) (1 << (n))
> > > -#define VHUB_EP_IRQ_ALL 0x7fff /*
> > > 15 EPs */
> > >
> > > /* USB status reg */
> > > #define VHUB_USBSTS_HISPEED (1 << 27)
> > > @@ -210,8 +202,6 @@
> > > * *
> > > ****************************************/
> > >
> > > -#define AST_VHUB_NUM_GEN_EPs 15 /* Generic non-0 EPs */
> > > -#define AST_VHUB_NUM_PORTS 5 /* vHub ports */
> > > #define AST_VHUB_EP0_MAX_PACKET 64 /* EP0's max packet
> > > size */
> > > #define AST_VHUB_EPn_MAX_PACKET 1024 /* Generic EPs max
> > > packet size */
> > > #define AST_VHUB_DESCS_COUNT 256 /* Use 256 descriptor mode
> > > (valid
> > > @@ -342,7 +332,7 @@ struct ast_vhub_dev {
> > > struct ast_vhub *vhub;
> > > void __iomem *regs;
> > >
> > > - /* Device index (0...4) and name string */
> > > + /* Device index (zero-based) and name string */
> > > unsigned int index;
> > > const char *name;
> > >
> > > @@ -358,7 +348,8 @@ struct ast_vhub_dev {
> > >
> > > /* Endpoint structures */
> > > struct ast_vhub_ep ep0;
> > > - struct
> > > ast_vhub_ep *epns[AST_VHUB_NUM_GEN_EPs];
> > > + struct ast_vhub_ep **epns;
> > > + u32 max_epns;
> > >
> > > };
> > > #define to_ast_dev(__g) container_of(__g, struct ast_vhub_dev,
> > > gadget)
> > > @@ -393,10 +384,12 @@ struct ast_vhub {
> > > bool ep1_stalled : 1;
> > >
> > > /* Per-port info */
> > > - struct ast_vhub_port ports[AST_VHUB_NUM_PORTS];
> > > + struct ast_vhub_port *ports;
> > > + u32 max_ports;
> > >
> > > /* Generic EP data structures */
> > > - struct ast_vhub_ep epns[AST_VHUB_NUM_GEN_EPs];
> > > + struct ast_vhub_ep *epns;
> > > + u32 max_epns;
> > >
> > > /* Upstream bus is suspended ? */
> > > bool suspended : 1;
> > > --
> > > 2.17.1
> > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
2020-02-10 19:07 ` Tao Ren
(?)
@ 2020-02-11 8:50 ` Benjamin Herrenschmidt
-1 siblings, 0 replies; 42+ messages in thread
From: Benjamin Herrenschmidt @ 2020-02-11 8:50 UTC (permalink / raw)
To: linux-aspeed
On Mon, 2020-02-10 at 11:07 -0800, Tao Ren wrote:
> > > This looks generally okay. We should wait for Ben's ack before
> > > applying.
> >
> > Shouldn't we instead have DT fields indicating those values ?
>
> May I ask why we prefer adding dt fields (such as "aspeed,vhub-max-ports"
> and "aspeed,vhub-max-endpoints") instead of assigning these values based
> on aspeed family? For example, is it to allow users to set a smaller
> number of ports/endpoints?
It's not a strong drive but it makes it more convenient to add support
to newer revisions if the only differences are those numbers.
>
> > Also we should add a DT representation for the various ID/strings of
> > the hub itself so manufacturers can customize them.
>
> Sure. I will add DT nodes for vendor/product/device IDs/strings. As it's
> not directly related to ast2600-support, shall I handle it in a separate
> patch? Or I can include the patch in this patch series?
Separate. Thanks !
Cheers,
Ben.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-11 8:50 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 42+ messages in thread
From: Benjamin Herrenschmidt @ 2020-02-11 8:50 UTC (permalink / raw)
To: Tao Ren
Cc: Mark Rutland, Felipe Balbi, linux-aspeed, devicetree,
Andrew Jeffery, Greg Kroah-Hartman, OpenBMC Maillist, linux-usb,
Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
Joel Stanley, Chunfeng Yun, Colin Ian King, Linux ARM
On Mon, 2020-02-10 at 11:07 -0800, Tao Ren wrote:
> > > This looks generally okay. We should wait for Ben's ack before
> > > applying.
> >
> > Shouldn't we instead have DT fields indicating those values ?
>
> May I ask why we prefer adding dt fields (such as "aspeed,vhub-max-ports"
> and "aspeed,vhub-max-endpoints") instead of assigning these values based
> on aspeed family? For example, is it to allow users to set a smaller
> number of ports/endpoints?
It's not a strong drive but it makes it more convenient to add support
to newer revisions if the only differences are those numbers.
>
> > Also we should add a DT representation for the various ID/strings of
> > the hub itself so manufacturers can customize them.
>
> Sure. I will add DT nodes for vendor/product/device IDs/strings. As it's
> not directly related to ast2600-support, shall I handle it in a separate
> patch? Or I can include the patch in this patch series?
Separate. Thanks !
Cheers,
Ben.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-11 8:50 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 42+ messages in thread
From: Benjamin Herrenschmidt @ 2020-02-11 8:50 UTC (permalink / raw)
To: Tao Ren
Cc: Joel Stanley, Mark Rutland, Felipe Balbi, linux-aspeed,
devicetree, Andrew Jeffery, Greg Kroah-Hartman, OpenBMC Maillist,
linux-usb, Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
Chunfeng Yun, Colin Ian King, Linux ARM
On Mon, 2020-02-10 at 11:07 -0800, Tao Ren wrote:
> > > This looks generally okay. We should wait for Ben's ack before
> > > applying.
> >
> > Shouldn't we instead have DT fields indicating those values ?
>
> May I ask why we prefer adding dt fields (such as "aspeed,vhub-max-ports"
> and "aspeed,vhub-max-endpoints") instead of assigning these values based
> on aspeed family? For example, is it to allow users to set a smaller
> number of ports/endpoints?
It's not a strong drive but it makes it more convenient to add support
to newer revisions if the only differences are those numbers.
>
> > Also we should add a DT representation for the various ID/strings of
> > the hub itself so manufacturers can customize them.
>
> Sure. I will add DT nodes for vendor/product/device IDs/strings. As it's
> not directly related to ast2600-support, shall I handle it in a separate
> patch? Or I can include the patch in this patch series?
Separate. Thanks !
Cheers,
Ben.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
2020-02-11 8:50 ` Benjamin Herrenschmidt
(?)
@ 2020-02-12 2:37 ` Tao Ren
-1 siblings, 0 replies; 42+ messages in thread
From: Tao Ren @ 2020-02-12 2:37 UTC (permalink / raw)
To: linux-aspeed
On Tue, Feb 11, 2020 at 09:50:42AM +0100, Benjamin Herrenschmidt wrote:
> On Mon, 2020-02-10 at 11:07 -0800, Tao Ren wrote:
> > > > This looks generally okay. We should wait for Ben's ack before
> > > > applying.
> > >
> > > Shouldn't we instead have DT fields indicating those values ?
> >
> > May I ask why we prefer adding dt fields (such as "aspeed,vhub-max-ports"
> > and "aspeed,vhub-max-endpoints") instead of assigning these values based
> > on aspeed family? For example, is it to allow users to set a smaller
> > number of ports/endpoints?
>
> It's not a strong drive but it makes it more convenient to add support
> to newer revisions if the only differences are those numbers.
Got it. Thanks for the clarify. Will send out v2 patches after more
testing.
> >
> > > Also we should add a DT representation for the various ID/strings of
> > > the hub itself so manufacturers can customize them.
> >
> > Sure. I will add DT nodes for vendor/product/device IDs/strings. As it's
> > not directly related to ast2600-support, shall I handle it in a separate
> > patch? Or I can include the patch in this patch series?
>
> Separate. Thanks !
Will take care of the change once this patch series is accepted.
Cheers,
Tao
>
> Cheers,
> Ben.
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-12 2:37 ` Tao Ren
0 siblings, 0 replies; 42+ messages in thread
From: Tao Ren @ 2020-02-12 2:37 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Mark Rutland, Felipe Balbi, linux-aspeed, devicetree,
Andrew Jeffery, Greg Kroah-Hartman, OpenBMC Maillist, linux-usb,
Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
Joel Stanley, Chunfeng Yun, Colin Ian King, Linux ARM
On Tue, Feb 11, 2020 at 09:50:42AM +0100, Benjamin Herrenschmidt wrote:
> On Mon, 2020-02-10 at 11:07 -0800, Tao Ren wrote:
> > > > This looks generally okay. We should wait for Ben's ack before
> > > > applying.
> > >
> > > Shouldn't we instead have DT fields indicating those values ?
> >
> > May I ask why we prefer adding dt fields (such as "aspeed,vhub-max-ports"
> > and "aspeed,vhub-max-endpoints") instead of assigning these values based
> > on aspeed family? For example, is it to allow users to set a smaller
> > number of ports/endpoints?
>
> It's not a strong drive but it makes it more convenient to add support
> to newer revisions if the only differences are those numbers.
Got it. Thanks for the clarify. Will send out v2 patches after more
testing.
> >
> > > Also we should add a DT representation for the various ID/strings of
> > > the hub itself so manufacturers can customize them.
> >
> > Sure. I will add DT nodes for vendor/product/device IDs/strings. As it's
> > not directly related to ast2600-support, shall I handle it in a separate
> > patch? Or I can include the patch in this patch series?
>
> Separate. Thanks !
Will take care of the change once this patch series is accepted.
Cheers,
Tao
>
> Cheers,
> Ben.
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-12 2:37 ` Tao Ren
0 siblings, 0 replies; 42+ messages in thread
From: Tao Ren @ 2020-02-12 2:37 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joel Stanley, Mark Rutland, Felipe Balbi, linux-aspeed,
devicetree, Andrew Jeffery, Greg Kroah-Hartman, OpenBMC Maillist,
linux-usb, Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
Chunfeng Yun, Colin Ian King, Linux ARM
On Tue, Feb 11, 2020 at 09:50:42AM +0100, Benjamin Herrenschmidt wrote:
> On Mon, 2020-02-10 at 11:07 -0800, Tao Ren wrote:
> > > > This looks generally okay. We should wait for Ben's ack before
> > > > applying.
> > >
> > > Shouldn't we instead have DT fields indicating those values ?
> >
> > May I ask why we prefer adding dt fields (such as "aspeed,vhub-max-ports"
> > and "aspeed,vhub-max-endpoints") instead of assigning these values based
> > on aspeed family? For example, is it to allow users to set a smaller
> > number of ports/endpoints?
>
> It's not a strong drive but it makes it more convenient to add support
> to newer revisions if the only differences are those numbers.
Got it. Thanks for the clarify. Will send out v2 patches after more
testing.
> >
> > > Also we should add a DT representation for the various ID/strings of
> > > the hub itself so manufacturers can customize them.
> >
> > Sure. I will add DT nodes for vendor/product/device IDs/strings. As it's
> > not directly related to ast2600-support, shall I handle it in a separate
> > patch? Or I can include the patch in this patch series?
>
> Separate. Thanks !
Will take care of the change once this patch series is accepted.
Cheers,
Tao
>
> Cheers,
> Ben.
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
2020-02-10 2:47 ` Joel Stanley
(?)
@ 2020-02-10 19:16 ` Tao Ren
-1 siblings, 0 replies; 42+ messages in thread
From: Tao Ren @ 2020-02-10 19:16 UTC (permalink / raw)
To: linux-aspeed
On Mon, Feb 10, 2020 at 02:47:02AM +0000, Joel Stanley wrote:
> On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
> >
> > From: Tao Ren <rentao.bupt@gmail.com>
> >
> > The patch moves hardcoded vhub attributes (maximum downstream ports and
> > generic endpoints) to "ast_vhub_config" structure which is attached to
> > struct of_device_id. The major purpose is to add AST2600 vhub support
> > because AST2600 vhub provides more downstream ports and endpoints.
> >
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
>
> This looks generally okay. We should wait for Ben's ack before applying.
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
Thanks Joel for reviewing the patches.
Cheers,
Tao
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-10 19:16 ` Tao Ren
0 siblings, 0 replies; 42+ messages in thread
From: Tao Ren @ 2020-02-10 19:16 UTC (permalink / raw)
To: Joel Stanley
Cc: Mark Rutland, Felipe Balbi, linux-aspeed, devicetree,
Andrew Jeffery, Benjamin Herrenschmidt, OpenBMC Maillist,
linux-usb, Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
Greg Kroah-Hartman, Chunfeng Yun, Colin Ian King, Linux ARM
On Mon, Feb 10, 2020 at 02:47:02AM +0000, Joel Stanley wrote:
> On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
> >
> > From: Tao Ren <rentao.bupt@gmail.com>
> >
> > The patch moves hardcoded vhub attributes (maximum downstream ports and
> > generic endpoints) to "ast_vhub_config" structure which is attached to
> > struct of_device_id. The major purpose is to add AST2600 vhub support
> > because AST2600 vhub provides more downstream ports and endpoints.
> >
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
>
> This looks generally okay. We should wait for Ben's ack before applying.
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
Thanks Joel for reviewing the patches.
Cheers,
Tao
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] usb: gadget: aspeed: read vhub config from of_device_id
@ 2020-02-10 19:16 ` Tao Ren
0 siblings, 0 replies; 42+ messages in thread
From: Tao Ren @ 2020-02-10 19:16 UTC (permalink / raw)
To: Joel Stanley
Cc: Benjamin Herrenschmidt, Felipe Balbi, Greg Kroah-Hartman,
Andrew Jeffery, Chunfeng Yun, Colin Ian King, Stephen Boyd,
Rob Herring, Mark Rutland, linux-usb, Linux ARM, linux-aspeed,
devicetree, Linux Kernel Mailing List, OpenBMC Maillist
On Mon, Feb 10, 2020 at 02:47:02AM +0000, Joel Stanley wrote:
> On Fri, 31 Jan 2020 at 22:22, <rentao.bupt@gmail.com> wrote:
> >
> > From: Tao Ren <rentao.bupt@gmail.com>
> >
> > The patch moves hardcoded vhub attributes (maximum downstream ports and
> > generic endpoints) to "ast_vhub_config" structure which is attached to
> > struct of_device_id. The major purpose is to add AST2600 vhub support
> > because AST2600 vhub provides more downstream ports and endpoints.
> >
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
>
> This looks generally okay. We should wait for Ben's ack before applying.
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
Thanks Joel for reviewing the patches.
Cheers,
Tao
^ permalink raw reply [flat|nested] 42+ messages in thread