From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 43F09EB64DA for ; Fri, 7 Jul 2023 08:36:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229775AbjGGIfl (ORCPT ); Fri, 7 Jul 2023 04:35:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60512 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229458AbjGGIff (ORCPT ); Fri, 7 Jul 2023 04:35:35 -0400 Received: from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com [IPv6:2a00:1450:4864:20::52c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6678F90 for ; Fri, 7 Jul 2023 01:35:33 -0700 (PDT) Received: by mail-ed1-x52c.google.com with SMTP id 4fb4d7f45d1cf-51d885b0256so2207408a12.2 for ; Fri, 07 Jul 2023 01:35:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1688718932; x=1691310932; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=Gfp5YnplyNI7ZZxVR1nzYlh1znSHp2pyPlsqgetXBH0=; b=a5YGKsob76RZKcNb3APPFN49ojM4izecoEniz8wRkWSvlHkYnoI9FH8/5bfNrJa8La UUf77hiBedWBZQVl9PhNLmwYTYv+v8FVsf6/kVQhWx/HPpv1kuji9w509zUctM+BsX9u NmnBF2AtaDMGfwPMBJiRsQ2aMstNsNT+4Bt5Svlcy+SMXTCp8Gd06F31D6mMtW3e5m00 kHNUssJoXqNtKY0J4xdrW9rFHVUseww9g8FSeZcRCZBXiflYa5aIm/8vBPTg0ZpTxq4g HbHVh3fCjfjHzAZ3ZH5YN4cmzrG1rvqzi2nQMSmXrZcQ6FKw1UVzJr//r1nGcEOz6FlS tIQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688718932; x=1691310932; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Gfp5YnplyNI7ZZxVR1nzYlh1znSHp2pyPlsqgetXBH0=; b=Ircn7mJjO4DkGrFOGJ6rQONzBajD4GBZx0wl4bB1nB4N2Vf5k4sTsGeeBScbfvaA0t sI4CjCaBZjwgiTUVsQKMMAgc1AeHDbDjD/31nDG0J+nY6cPqplyPqnEomsdVd3ldcR8s 23VHbvPdLT/impv3Dq/fZswp/EwEl3AVbKwuh5kQkcXZCsV17HvxNU436vHzWPvKrLJ1 906fMyAFUPgYcjMjKSW7jXTL/kaD5COvN5Nw3y/TakUaiIQJe9g8zawtN1twgJmH08Xg eSXz7mCtP0jv7PYDO5p058r1NixeVDdotzo6ATbawfdDe5WWITDseTcViK7AhNE1DO7p v63A== X-Gm-Message-State: ABy/qLasZRvYyWzY03s5jvdqxcjKPqCr4BNrhbfGx65WcWJEQOuF80CC 2X7iIXraAvKRtL9qtx12Ouio5w== X-Google-Smtp-Source: APBJJlHVIcrQOa49qCeTblKamg9Js3+YQHLGpbElKy5lgi88UbQJqiq2QghEYQEOIEcpcwgasbL4Yw== X-Received: by 2002:a17:906:338e:b0:992:3ac7:a2d8 with SMTP id v14-20020a170906338e00b009923ac7a2d8mr2931894eja.37.1688718931759; Fri, 07 Jul 2023 01:35:31 -0700 (PDT) Received: from [192.168.1.20] ([178.197.219.26]) by smtp.gmail.com with ESMTPSA id r16-20020a170906c29000b00991d54db2acsm1869577ejz.44.2023.07.07.01.35.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 07 Jul 2023 01:35:31 -0700 (PDT) Message-ID: Date: Fri, 7 Jul 2023 10:35:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH v1 2/3] usb: gadget: udc: gxp-udc: add HPE GXP USB support Content-Language: en-US To: richard.yu@hpe.com, verdun@hpe.com, nick.hawkins@hpe.com, gregkh@linuxfoundation.org, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org References: <20230706215910.78772-1-richard.yu@hpe.com> <20230706215910.78772-3-richard.yu@hpe.com> From: Krzysztof Kozlowski In-Reply-To: <20230706215910.78772-3-richard.yu@hpe.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On 06/07/2023 23:59, richard.yu@hpe.com wrote: > From: Richard Yu > > The HPE GXP vEHCI controller presents a four port EHCI compatible PCI > function to host software. Each vEHCI port is logically connected to a > corresponding set of virtual device registers. > > Signed-off-by: Richard Yu > --- > drivers/usb/gadget/udc/Kconfig | 6 + > drivers/usb/gadget/udc/Makefile | 1 + > drivers/usb/gadget/udc/gxp-udc.c | 1401 ++++++++++++++++++++++++++++++ > 3 files changed, 1408 insertions(+) > create mode 100644 drivers/usb/gadget/udc/gxp-udc.c > > diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig > index 83cae6bb12eb..c01eb2a2c7db 100644 > --- a/drivers/usb/gadget/udc/Kconfig > +++ b/drivers/usb/gadget/udc/Kconfig > @@ -461,6 +461,12 @@ config USB_ASPEED_UDC > dynamically linked module called "aspeed_udc" and force all > gadget drivers to also be dynamically linked. ... > + > +static void gxp_udc_dev_release(struct device *dev) > +{ > + kfree(dev); > +} > + > +int gxp_udc_init_dev(struct gxp_udcg_drvdata *udcg, unsigned int idx) > +{ > + struct gxp_udc_drvdata *drvdata; > + struct device *parent = &udcg->pdev->dev; > + int rc; > + > + drvdata = devm_kzalloc(parent, sizeof(struct gxp_udc_drvdata), > + GFP_KERNEL); sizeof(*...) > + if (!drvdata) > + return -ENOMEM; > + > + udcg->udc_drvdata[idx] = drvdata; > + drvdata->pdev = udcg->pdev; Why do you store platform_device? It's usually useless because dev is used. > + drvdata->vdevnum = idx; > + drvdata->base = udcg->base + 0x1000 * idx; > + drvdata->udcg_map = udcg->udcg_map; > + drvdata->irq = udcg->irq; > + > + /* > + * we setup USB device can have up to 4 endpoints besides control > + * endpoint 0. > + */ > + drvdata->fepnum = min_t(u32, udcg->max_fepnum, 4); > + > + drvdata->vdevnum = idx; > + > + /* > + * 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 > + */ > + drvdata->port_dev = kzalloc(sizeof(*drvdata->port_dev), GFP_KERNEL); > + if (!drvdata->port_dev) { > + rc = -ENOMEM; > + goto fail_alloc; > + } > + device_initialize(drvdata->port_dev); > + drvdata->port_dev->release = gxp_udc_dev_release; > + drvdata->port_dev->parent = parent; > + dev_set_name(drvdata->port_dev, "%s:p%d", dev_name(parent), idx + 1); > + > + /* DMA setting */ > + drvdata->port_dev->dma_mask = parent->dma_mask; > + drvdata->port_dev->coherent_dma_mask = parent->coherent_dma_mask; > + drvdata->port_dev->bus_dma_limit = parent->bus_dma_limit; > + drvdata->port_dev->dma_range_map = parent->dma_range_map; > + drvdata->port_dev->dma_parms = parent->dma_parms; > + drvdata->port_dev->dma_pools = parent->dma_pools; > + > + rc = device_add(drvdata->port_dev); > + if (rc) > + goto fail_add; > + > + /* Populate gadget */ > + gxp_udc_init(drvdata); > + > + rc = usb_add_gadget_udc(drvdata->port_dev, &drvdata->gadget); > + if (rc != 0) { > + dev_err(drvdata->port_dev, "add gadget failed\n"); return dev_err_probe > + goto fail_udc; > + } > + rc = devm_request_irq(drvdata->port_dev, > + drvdata->irq, > + gxp_udc_irq, > + IRQF_SHARED, > + gxp_udc_name[drvdata->vdevnum], > + drvdata); Shared interrupt usually does not work with devm() and causes later issues. You need to triple check your unbind and error paths, because usually this just does not work. Usually we just don't allow devm for this case. If you are sure this is 100% correct, it could stay... > + > + if (rc < 0) { > + dev_err(drvdata->port_dev, "irq request failed\n"); return dev_err_probe > + goto fail_udc; > + } > + > + return 0; > + > +fail_udc: > + device_del(drvdata->port_dev); > +fail_add: > + put_device(drvdata->port_dev); > +fail_alloc: > + kfree(drvdata); Are you sure you tested it? I see here double free. > + > + return rc; > +} > + > +static int gxp_udcg_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + struct gxp_udcg_drvdata *drvdata; > + int ret, rc, err; > + u32 vdevnum; > + u32 fepnum, i; > + > + drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_udcg_drvdata), sizeof(*...) > + GFP_KERNEL); > + platform_set_drvdata(pdev, drvdata); > + drvdata->pdev = pdev; Why do you store platform_device? It's usually useless because dev is used. > + > + spin_lock_init(&drvdata->lock); > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "udcg"); > + drvdata->udcg_base = devm_ioremap_resource(&pdev->dev, res); Use helper for these two. > + if (IS_ERR(drvdata->udcg_base)) { > + err = PTR_ERR(drvdata->udcg_base); Drop > + return dev_err_probe(&pdev->dev, err, "failed to get udcg resource\n"); > + } > + > + drvdata->udcg_map = devm_regmap_init_mmio(&pdev->dev, drvdata->udcg_base, ®map_cfg); > + > + if (IS_ERR(drvdata->udcg_map)) { > + dev_err(&pdev->dev, "failed to map udcg regs"); return dev_err_probe > + return -ENODEV; Wrong error code > + } > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "udc"); > + drvdata->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(drvdata->base)) { > + err = PTR_ERR(drvdata->base); Drop > + return dev_err_probe(&pdev->dev, err, "failed to get udc resource\n"); > + } > + > + ret = platform_get_irq(pdev, 0); > + if (ret < 0) { > + dev_err(&pdev->dev, "unable to obtain IRQ number\n"); return dev_err_probe > + return ret; > + } > + drvdata->irq = ret; > + > + if (of_property_read_u32(pdev->dev.of_node, "hpe,vehci-downstream-ports", &vdevnum)) { > + dev_err(&pdev->dev, "property ports number is undefined\n"); > + return -EINVAL; > + } > + > + if (vdevnum > GXP_UDC_MAX_NUM_VDEVICE) { > + dev_err(&pdev->dev, "property max port number(%d) is invalid\n", vdevnum); > + return -EINVAL; > + } > + drvdata->max_vdevnum = vdevnum; > + > + if (of_property_read_u32(pdev->dev.of_node, "hpe,vehci-generic-endpoints", &fepnum)) { > + dev_err(&pdev->dev, "property generic endpoints is undefined\n"); > + return -EINVAL; Why? Bindings stated it has default, so should not be required. > + } > + > + if (fepnum > GXP_UDC_MAX_NUM_FLEX_EP) { > + dev_err(&pdev->dev, "property fepnum(%d) is invalid\n", fepnum); > + return -EINVAL; > + } > + drvdata->max_fepnum = fepnum; > + > + gxp_udcg_init(drvdata); > + > + /* Init child devices */ > + rc = 0; > + for (i = 0; i < drvdata->max_vdevnum && rc == 0; i++) > + rc = gxp_udc_init_dev(drvdata, i); > + > + return rc; > +} > + > +static const struct of_device_id gxp_udc_of_match[] = { > + { .compatible = "hpe,gxp-udcg" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, gxp_udc_of_match); > + > +static struct platform_driver gxp_udc_driver = { > + .driver = { > + .name = "gxp-udc", > + .of_match_table = of_match_ptr(gxp_udc_of_match), Drop of_match_ptr(), you will have warnings here. > + }, > + .probe = gxp_udcg_probe, > +}; > +module_platform_driver(gxp_udc_driver); > + > +MODULE_AUTHOR("Richard Yu "); > +MODULE_DESCRIPTION("HPE GXP vEHCI UDC Driver"); > +MODULE_LICENSE("GPL"); Best regards, Krzysztof