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 X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2D869C04EB8 for ; Fri, 30 Nov 2018 14:37:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BA75720863 for ; Fri, 30 Nov 2018 14:37:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=cadence.com header.i=@cadence.com header.b="INyFojzp"; dkim=pass (1024-bit key) header.d=cadence.com header.i=@cadence.com header.b="s+Q6RTND" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BA75720863 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cadence.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726971AbeLABqu (ORCPT ); Fri, 30 Nov 2018 20:46:50 -0500 Received: from mx0b-0014ca01.pphosted.com ([208.86.201.193]:41658 "EHLO mx0a-0014ca01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726340AbeLABqu (ORCPT ); Fri, 30 Nov 2018 20:46:50 -0500 Received: from pps.filterd (m0042333.ppops.net [127.0.0.1]) by mx0b-0014ca01.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id wAUESbVa023772; Fri, 30 Nov 2018 06:36:46 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cadence.com; h=from : to : cc : subject : date : message-id : references : in-reply-to : content-type : content-transfer-encoding : mime-version; s=proofpoint; bh=oCg20VCdm/XB1lwsG6qMTtOuGOCqtGvHX00TtGAEe6U=; b=INyFojzpwi552fDCYwPFAfDpQ9aFgYb8sia+kb5EO4+c9qYKNTkeqj66RYICb3pF6MS7 QlBQOOkf+57zViFq+jNCXNzO0rD1sTQznzH7bjAZQspQMGhVOxB3X9VkA0wG/x5xU1eI ZotYwS7yoebCbTLvk8LYR7Wnn+28LFX1dbT9KLx4Z4O5xcV7wszP50MBufDBBYcUdtgO kWlzoZ6ixp3/SMqeK2BctxhSvqsNWRHxXL2A9TmwIOW+R5DpBD0Cir2JI/PvEmt84Y3k 4ZZiRBjhvkjDxbZH4BVxbUZ4CnuFaR3+WR2ec1CUSE0hx6nfq2ZygkRiXzFjeJ53P5wI xw== Authentication-Results: cadence.com; spf=pass smtp.mailfrom=pawell@cadence.com Received: from nam01-by2-obe.outbound.protection.outlook.com (mail-by2nam01lp0178.outbound.protection.outlook.com [216.32.181.178]) by mx0b-0014ca01.pphosted.com with ESMTP id 2p270h1qe8-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 30 Nov 2018 06:36:46 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cadence.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=oCg20VCdm/XB1lwsG6qMTtOuGOCqtGvHX00TtGAEe6U=; b=s+Q6RTNDKRDQdDb44awNmo6pKIN8WSdpPOguJKJ9kCWTmgn/PQwJNESaZqPp1ge6WPDwTtagkqcdqRajpKsn1Mw5HHB1LrIZ/o+Fft4pi6InK4SZWgzGVsOjjHeqXFq7kkUHzii4F5kLqav2BR2wpxmu6BWCjBhTjkro22B98OY= Received: from BYAPR07MB4709.namprd07.prod.outlook.com (52.135.204.159) by BYAPR07MB4869.namprd07.prod.outlook.com (52.135.205.142) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1361.19; Fri, 30 Nov 2018 14:36:44 +0000 Received: from BYAPR07MB4709.namprd07.prod.outlook.com ([fe80::e0dc:ebd5:e248:d644]) by BYAPR07MB4709.namprd07.prod.outlook.com ([fe80::e0dc:ebd5:e248:d644%6]) with mapi id 15.20.1361.019; Fri, 30 Nov 2018 14:36:40 +0000 From: Pawel Laszczak To: Roger Quadros , "devicetree@vger.kernel.org" , Felipe Balbi CC: "gregkh@linuxfoundation.org" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Alan Douglas , "jbergsagel@ti.com" , "nsekhar@ti.com" , "nm@ti.com" , Suresh Punnoose , "peter.chen@nxp.com" , Pawel Jez , Rahul Kumar Subject: RE: [RFC PATCH v2 07/15] usb:cdns3: Adds Device mode support - initialization. Thread-Topic: [RFC PATCH v2 07/15] usb:cdns3: Adds Device mode support - initialization. Thread-Index: AQHUfycXki8AfRuxW0m5LfbM+YmFdqVlHjwAgAMTJeA= Date: Fri, 30 Nov 2018 14:36:40 +0000 Message-ID: References: <1542535751-16079-1-git-send-email-pawell@cadence.com> <1542535751-16079-8-git-send-email-pawell@cadence.com> <5BFE7D2E.7030702@ti.com> In-Reply-To: <5BFE7D2E.7030702@ti.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-dg-tag-bcast: x-dg-ref: PG1ldGE+PGF0IG5tPSJib2R5LnR4dCIgcD0iYzpcdXNlcnNccGF3ZWxsXGFwcGRhdGFccm9hbWluZ1wwOWQ4NDliNi0zMmQzLTRhNDAtODVlZS02Yjg0YmEyOWUzNWJcbXNnc1xtc2ctNThmMTc4M2YtZjRhZC0xMWU4LTg3MjYtMWM0ZDcwMWRmYmE0XGFtZS10ZXN0XDU4ZjE3ODQwLWY0YWQtMTFlOC04NzI2LTFjNGQ3MDFkZmJhNGJvZHkudHh0IiBzej0iMjQ4NDgiIHQ9IjEzMTg4MDYyMjAwNTM4Mjg4OSIgaD0ib1dwTWFNdUx2MlNlaXdNdFpuYmhDWEFqejNzPSIgaWQ9IiIgYmw9IjAiIGJvPSIxIi8+PC9tZXRhPg== x-dg-paste: x-dg-rorf: x-originating-ip: [185.217.253.59] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1;BYAPR07MB4869;20:aIEXpmYlos4txsAEe3/5gB0WjeyR7wIi3kWiNolCUYM6ervX21l0tdVzoi9O9ng9R+M9WBJ1MMvrsy3jWUDeoJJ/gL1E4a9V+6ONB9i+QruJ3mFhDv5D2Bv6zj7XwP9XJLJ0tOBs9ZWcmPX3e6w6XyZdQ+ej97K1Z/WDaAfpJpAJMNFtDfBRCJU79+qEEoxf5mftBy9TVWF1oEZgQEB8A8OSpquOW7RDhYhr3vk5FBktgIxluZoTIxNlWPSu9Ptb x-ms-exchange-antispam-srfa-diagnostics: SOS;SOR; x-forefront-antispam-report: SFV:SKI;SCL:-1;SFV:NSPM;SFS:(10009020)(136003)(396003)(376002)(346002)(39860400002)(366004)(199004)(189003)(36092001)(97736004)(55016002)(7736002)(105586002)(5660300001)(316002)(76176011)(2906002)(106356001)(8936002)(229853002)(11346002)(66066001)(3846002)(186003)(110136005)(4744004)(53936002)(99286004)(486006)(6436002)(7416002)(7696005)(6116002)(74316002)(4326008)(107886003)(26005)(6506007)(25786009)(14444005)(54906003)(71190400001)(71200400001)(2501003)(6246003)(33656002)(9686003)(68736007)(53946003)(476003)(217873002)(446003)(14454004)(305945005)(81166006)(575784001)(86362001)(81156014)(8676002)(102836004)(478600001)(256004)(579004)(309714004);DIR:OUT;SFP:1101;SCL:1;SRVR:BYAPR07MB4869;H:BYAPR07MB4709.namprd07.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1; x-ms-office365-filtering-correlation-id: 58702302-4b13-43fa-de76-08d656d13dba x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390098)(7020095)(4652040)(8989299)(5600074)(711020)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7153060)(7193020);SRVR:BYAPR07MB4869; x-ms-traffictypediagnostic: BYAPR07MB4869: x-microsoft-antispam-prvs: x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(93006095)(93001095)(3231453)(999002)(944501410)(52105112)(3002001)(10201501046)(148016)(149066)(150057)(6041310)(20161123558120)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123564045)(20161123560045)(201708071742011)(7699051)(76991095);SRVR:BYAPR07MB4869;BCL:0;PCL:0;RULEID:;SRVR:BYAPR07MB4869; x-forefront-prvs: 087223B4DA received-spf: None (protection.outlook.com: cadence.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: Xs7Jq0CoO7Ax/qTJ3y6obHaBnaoC6KmBNWLj7Wam+gQUYrklnaYAushwZxdkvIFMR+8MfkfTB2tnS0SzYjNhvL7kqoPgj+JNQCAKICYycPynASxX9lKKKd2P5yVlI9rwLTS2pNgiulON76zfo03vz8ORWcqdKS7f+lm/9sxBZrwDYk+CEOGvQ1sgHScXQW75uIQKDqCkEhoVYFcnvhPOZsJ8/QjPXxaxGIWM/fFL0KopyZtP3fqX8bpTCVbY7fR+5l1/apUtgYyCaDIVo8+nzxyCbyuBY/7l6QbNaPi4klUlnnQ12ef2gAOmvAfPMILrVA1V2VKG5bGv80fyTEnVd1jmU6oz5fKXQ8Wf6HU3n8A= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: cadence.com X-MS-Exchange-CrossTenant-Network-Message-Id: 58702302-4b13-43fa-de76-08d656d13dba X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Nov 2018 14:36:40.1973 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: d36035c5-6ce6-4662-a3dc-e762e61ae4c9 X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR07MB4869 X-Proofpoint-SPF-Result: pass X-Proofpoint-SPF-Record: v=spf1 include:_spf.salesforce.com include:mktomail.com include:spf-0014ca01.pphosted.com include:spf.protection.outlook.com include:auth.msgapp.com include:spf.mandrillapp.com ~all X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-11-30_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_check_notspam policy=outbound_check score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1811300124 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi,=20 > >+Felipe. > >Pawel, > >Please copy Felipe Balbi as he maintains the USB gadget stack. > >On 18/11/18 12:09, Pawel Laszczak wrote: >> Patch implements a set of functions responsible for initialization, >> configuration, starting and stopping device mode. >> This patch also adds new ep0.c that holds all functions related >> to endpoint 0. >> >> Signed-off-by: Pawel Laszczak >> --- >> drivers/usb/cdns3/Kconfig | 10 + >> drivers/usb/cdns3/Makefile | 1 + >> drivers/usb/cdns3/core.c | 5 +- >> drivers/usb/cdns3/ep0.c | 105 ++++++++ >> drivers/usb/cdns3/gadget-export.h | 27 +++ >> drivers/usb/cdns3/gadget.c | 390 ++++++++++++++++++++++++++++++ >> drivers/usb/cdns3/gadget.h | 4 + >> 7 files changed, 541 insertions(+), 1 deletion(-) >> create mode 100644 drivers/usb/cdns3/ep0.c >> create mode 100644 drivers/usb/cdns3/gadget-export.h >> create mode 100644 drivers/usb/cdns3/gadget.c >> >> diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig >> index d92bc3d68eb0..b7d71b5c4f60 100644 >> --- a/drivers/usb/cdns3/Kconfig >> +++ b/drivers/usb/cdns3/Kconfig >> @@ -10,6 +10,16 @@ config USB_CDNS3 >> >> if USB_CDNS3 >> >> +config USB_CDNS3_GADGET >> + bool "Cadence USB3 device controller" >> + depends on USB_GADGET >> + help >> + Say Y here to enable device controller functionality of the >> + cadence USBSS-DEV driver. >> + >> + This controller support FF, HS and SS mode. It doeasn't suppo= rt > >s/support/supports >s/doeasn't/doesn't > >> + LS and SSP mode >> + >> config USB_CDNS3_HOST >> bool "Cadence USB3 host controller" >> depends on USB_XHCI_HCD >> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile >> index 976117ba67ff..bea6173bf37f 100644 >> --- a/drivers/usb/cdns3/Makefile >> +++ b/drivers/usb/cdns3/Makefile >> @@ -2,5 +2,6 @@ obj-$(CONFIG_USB_CDNS3) +=3D cdns3.o >> obj-$(CONFIG_USB_CDNS3_PCI_WRAP) +=3D cdns3-pci.o >> >> cdns3-y :=3D core.o drd.o >> +cdns3-$(CONFIG_USB_CDNS3_GADGET) +=3D gadget.o ep0.o >> cdns3-$(CONFIG_USB_CDNS3_HOST) +=3D host.o >> cdns3-pci-y :=3D cdns3-pci-wrap.o >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c >> index 4cb820be9ff3..1fa233415901 100644 >> --- a/drivers/usb/cdns3/core.c >> +++ b/drivers/usb/cdns3/core.c >> @@ -18,6 +18,7 @@ >> #include "gadget.h" >> #include "core.h" >> #include "host-export.h" >> +#include "gadget-export.h" >> #include "drd.h" >> >> static inline struct cdns3_role_driver *cdns3_get_current_role_driver(s= truct cdns3 *cdns) >> @@ -104,7 +105,8 @@ static int cdns3_core_init_role(struct cdns3 *cdns) >> } >> >> if (dr_mode =3D=3D USB_DR_MODE_OTG || dr_mode =3D=3D USB_DR_MODE_PERIP= HERAL) { >> - //TODO: implements device initialization >> + if (cdns3_gadget_init(cdns)) >> + dev_info(dev, "doesn't support gadget\n"); > >dev_err() and we should should error out with error code returned by cdns3= _gadget_init(). Ok,=20 > >> } >> >> if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) = { >> @@ -144,6 +146,7 @@ static irqreturn_t cdns3_irq(int irq, void *data) >> >> static void cdns3_remove_roles(struct cdns3 *cdns) >> { > >if (dr_mode =3D=3D USB_DR_MODE_OTG || dr_mode =3D=3D USB_DR_MODE_PERIPHERA= L) I had to little change this fragment. In the latest version only single d= river (role) can be=20 initialized and started so this code has changed to : static void cdns3_exit_roles(struct cdns3 *cdns) { cdns3_role_stop(cdns); cdns3_drd_exit(cdns); } Functions cdns3_gadget_remove and cdns3_host_remove are no longer needed. >> + cdns3_gadget_remove(cdns); >> cdns3_host_remove(cdns); >> } >> >> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c >> new file mode 100644 >> index 000000000000..c08d02665f9d >> --- /dev/null >> +++ b/drivers/usb/cdns3/ep0.c >> @@ -0,0 +1,105 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Cadence USBSS DRD Driver - gadget side. >> + * >> + * Copyright (C) 2018 Cadence Design Systems. >> + * Copyright (C) 2017 NXP >> + * >> + * Authors: Pawel Jez , >> + * Pawel Laszczak >> + * Peter Chen >> + */ >> + >> +#include "gadget.h" >> + >> +static struct usb_endpoint_descriptor cdns3_gadget_ep0_desc =3D { >> + .bLength =3D USB_DT_ENDPOINT_SIZE, >> + .bDescriptorType =3D USB_DT_ENDPOINT, >> + .bmAttributes =3D USB_ENDPOINT_XFER_CONTROL, >> +}; >> + >> +static void cdns3_prepare_setup_packet(struct cdns3_device *priv_dev) >> +{ >> + //TODO: Implements this function >> +} >> + >> +/** >> + * cdns3_ep0_config - Configures default endpoint >> + * @priv_dev: extended gadget object >> + * >> + * Functions sets parameters: maximal packet size and enables interrupt= s >> + */ >> +void cdns3_ep0_config(struct cdns3_device *priv_dev) >> +{ >> + struct cdns3_usb_regs __iomem *regs; >> + u32 max_packet_size =3D 64; >> + >> + regs =3D priv_dev->regs; >> + >> + if (priv_dev->gadget.speed =3D=3D USB_SPEED_SUPER) >> + max_packet_size =3D 512; > >is gadget.speed known at this point? I think it will only be known after a= connection >is made. This function is called on connection, on reset event and during initializa= tion gadget.=20 During initialization the speed will be set to 64 and then will be updated = to correct value.=20 >> + >> + if (priv_dev->ep0_request) { >> + list_del_init(&priv_dev->ep0_request->list); >> + priv_dev->ep0_request =3D NULL; >> + } >> + >> + priv_dev->gadget.ep0->maxpacket =3D max_packet_size; >> + cdns3_gadget_ep0_desc.wMaxPacketSize =3D cpu_to_le16(max_packet_size); >> + >> + /* init ep out */ >> + cdns3_select_ep(priv_dev, USB_DIR_OUT); >> + >> + writel(EP_CFG_ENABLE | EP_CFG_MAXPKTSIZE(max_packet_size), >> + ®s->ep_cfg); >> + >> + writel(EP_STS_EN_SETUPEN | EP_STS_EN_DESCMISEN | EP_STS_EN_TRBERREN, >> + ®s->ep_sts_en); >> + >> + /* init ep in */ >> + cdns3_select_ep(priv_dev, USB_DIR_IN); >> + >> + writel(EP_CFG_ENABLE | EP_CFG_MAXPKTSIZE(max_packet_size), >> + ®s->ep_cfg); >> + >> + writel(EP_STS_EN_SETUPEN | EP_STS_EN_TRBERREN, ®s->ep_sts_en); >> + >> + cdns3_set_register_bit(®s->usb_conf, USB_CONF_U1DS | USB_CONF_U2DS)= ; >> + cdns3_prepare_setup_packet(priv_dev); > >What happens if gadget.speed is USB_SPEED_SUPER, but was connected to a hi= gh-speed host? >Are you updating the max_packet_size on connection done? As above. > >> +} >> + >> +/** >> + * cdns3_init_ep0 Initializes software endpoint 0 of gadget >> + * @cdns3: extended gadget object >> + * >> + * Returns 0 on success, error code elsewhere >> + */ >> +int cdns3_init_ep0(struct cdns3_device *priv_dev) >> +{ >> + struct cdns3_endpoint *ep0; >> + >> + ep0 =3D devm_kzalloc(&priv_dev->dev, sizeof(struct cdns3_endpoint), >> + GFP_KERNEL); >> + >> + if (!ep0) >> + return -ENOMEM; >> + >> + ep0->cdns3_dev =3D priv_dev; >> + sprintf(ep0->name, "ep0"); >> + >> + /* fill linux fields */ >> + //TODO: implements cdns3_gadget_ep0_ops object >> + //ep0->endpoint.ops =3D &cdns3_gadget_ep0_ops; >> + ep0->endpoint.maxburst =3D 1; >> + usb_ep_set_maxpacket_limit(&ep0->endpoint, ENDPOINT0_MAX_PACKET_LIMIT)= ; >> + ep0->endpoint.address =3D 0; >> + ep0->endpoint.caps.type_control =3D 1; >> + ep0->endpoint.caps.dir_in =3D 1; >> + ep0->endpoint.caps.dir_out =3D 1; >> + ep0->endpoint.name =3D ep0->name; >> + ep0->endpoint.desc =3D &cdns3_gadget_ep0_desc; >> + priv_dev->gadget.ep0 =3D &ep0->endpoint; >> + INIT_LIST_HEAD(&ep0->request_list); >> + >> + return 0; >> +} >> diff --git a/drivers/usb/cdns3/gadget-export.h b/drivers/usb/cdns3/gadge= t-export.h >> new file mode 100644 >> index 000000000000..257e5e0eef31 >> --- /dev/null >> +++ b/drivers/usb/cdns3/gadget-export.h >> @@ -0,0 +1,27 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Cadence USBSS DRD Driver -Gadget Export APIs >> + * >> + * Copyright (C) 2017 NXP >> + * >> + * Authors: Peter Chen >> + */ >> +#ifndef __LINUX_CDNS3_GADGET_EXPORT >> +#define __LINUX_CDNS3_GADGET_EXPORT >> + >> +#ifdef CONFIG_USB_CDNS3_GADGET >> + >> +int cdns3_gadget_init(struct cdns3 *cdns); >> +void cdns3_gadget_remove(struct cdns3 *cdns); >> +#else >> + >> +static inline int cdns3_gadget_init(struct cdns3 *cdns) >> +{ >> + return -ENXIO; >> +} >> + >> +static inline void cdns3_gadget_remove(struct cdns3 *cdns) { } >> + >> +#endif >> + >> +#endif /* __LINUX_CDNS3_GADGET_EXPORT */ >> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c >> new file mode 100644 >> index 000000000000..376b68b13d1b >> --- /dev/null >> +++ b/drivers/usb/cdns3/gadget.c >> @@ -0,0 +1,390 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Cadence USBSS DRD Driver - gadget side. >> + * >> + * Copyright (C) 2018 Cadence Design Systems. >> + * Copyright (C) 2017 NXP >> + * >> + * Authors: Pawel Jez , >> + * Pawel Laszczak >> + * Peter Chen >> + */ >> + >> +#include >> +#include >> + >> +#include "core.h" >> +#include "gadget-export.h" >> +#include "gadget.h" >> + >> +/** >> + * cdns3_set_register_bit - set bit in given register. >> + * @ptr: address of device controller register to be read and changed >> + * @mask: bits requested to set >> + */ >> +void cdns3_set_register_bit(void __iomem *ptr, u32 mask) >> +{ >> + mask =3D readl(ptr) | mask; >> + writel(mask, ptr); >> +} > >static inline? >I'd get rid of this function if possible. You are using it only once >and it is easier to read code if setting the bitmask is done plainly >instead of hiding it behind a function. It will be used at least 6 times in the entire driver.=20 > >> + >> +/** >> + * select_ep - selects endpoint >> + * @priv_dev: extended gadget object >> + * @ep: endpoint address >> + */ >> +void cdns3_select_ep(struct cdns3_device *priv_dev, u32 ep) >> +{ >> + if (priv_dev->selected_ep =3D=3D ep) >> + return; > >I didn't understand the purpose of this function. You can only select the = EP once? Yes I can, but the endpoint is selected in many place in the driver and som= etimes the same endpoint=20 Is selected many times subsequently.=20 Access to register can be slow, so this solution can improve performance.=20 If I'm wrong please correct me.=20 > >> + >> + dev_dbg(&priv_dev->dev, "Ep sel: 0x%02x\n", ep); > >You should move to use trace-points instead of dev_dbg. Yes, I know in next patch I will add trace-point > >> + priv_dev->selected_ep =3D ep; >> + writel(ep, &priv_dev->regs->ep_sel); >> +} >> + >> +/** >> + * cdns3_irq_handler - irq line interrupt handler >> + * @cdns: cdns3 instance >> + * >> + * Returns IRQ_HANDLED when interrupt raised by USBSS_DEV, >> + * IRQ_NONE when interrupt raised by other device connected >> + * to the irq line >> + */ >> +static irqreturn_t cdns3_irq_handler_thread(struct cdns3 *cdns) >> +{ >> + irqreturn_t ret =3D IRQ_NONE; >> + //TODO: implements this function >> + return ret; >> +} >> + >> +static void cdns3_gadget_config(struct cdns3_device *priv_dev) >> +{ >> + struct cdns3_usb_regs __iomem *regs =3D priv_dev->regs; >> + >> + cdns3_ep0_config(priv_dev); >> + >> + /* enable interrupts for endpoint 0 (in and out) */ >> + writel(EP_IEN_EP_OUT0 | EP_IEN_EP_IN0, ®s->ep_ien); >> + >> + /* enable generic interrupt*/ >> + writel(USB_IEN_INIT, ®s->usb_ien); >> + writel(USB_CONF_CLK2OFFDS | USB_CONF_L1DS, ®s->usb_conf); >> + writel(USB_CONF_DMULT, ®s->usb_conf); >> + writel(USB_CONF_DEVEN, ®s->usb_conf); > >If you are enabling interrupts in this patch you should handle them in the= ISR. > >> +} >> + >> +/** >> + * cdns3_init_ep Initializes software endpoints of gadget >> + * @cdns3: extended gadget object >> + * >> + * Returns 0 on success, error code elsewhere >> + */ >> +static int cdns3_init_ep(struct cdns3_device *priv_dev) >> +{ >> + u32 ep_enabled_reg, iso_ep_reg; >> + struct cdns3_endpoint *priv_ep; >> + int found_endpoints =3D 0; >> + int ep_dir, ep_number; >> + u32 ep_mask; >> + int i; >> + >> + /* Read it from USB_CAP3 to USB_CAP5 */ >> + ep_enabled_reg =3D readl(&priv_dev->regs->usb_cap3); >> + iso_ep_reg =3D readl(&priv_dev->regs->usb_cap4); >> + >> + dev_dbg(&priv_dev->dev, "Initializing non-zero endpoints\n"); >> + >> + for (i =3D 0; i < USB_SS_ENDPOINTS_MAX_COUNT; i++) { > >CDNS3_USB_SS_ENDPOINTS_MAX_COUNT Ok, consistence in naming is important.=20 > >> + ep_number =3D (i / 2) + 1; >> + ep_dir =3D i % 2; >> + ep_mask =3D BIT((16 * ep_dir) + ep_number); >> + >> + if (!(ep_enabled_reg & ep_mask)) >> + continue; >> + >> + priv_ep =3D devm_kzalloc(&priv_dev->dev, sizeof(*priv_ep), >> + GFP_KERNEL); >> + if (!priv_ep) >> + return -ENOMEM; >> + >> + /* set parent of endpoint object */ >> + priv_ep->cdns3_dev =3D priv_dev; >> + priv_dev->eps[found_endpoints++] =3D priv_ep; >> + >> + snprintf(priv_ep->name, sizeof(priv_ep->name), "ep%d%s", >> + ep_number, !!ep_dir ? "in" : "out"); >> + priv_ep->endpoint.name =3D priv_ep->name; >> + >> + usb_ep_set_maxpacket_limit(&priv_ep->endpoint, >> + ENDPOINT_MAX_PACKET_LIMIT); > >CDNS3_ENDPOINT_MAX_PACKET_LIMIT > >> + priv_ep->endpoint.max_streams =3D ENDPOINT_MAX_STREAMS; > >CDNS3_ENDPOINT_MAX_STREAMS > >> + //TODO: Add implementation of cdns3_gadget_ep_ops >> + //priv_ep->endpoint.ops =3D &cdns3_gadget_ep_ops; >> + if (ep_dir) >> + priv_ep->endpoint.caps.dir_in =3D 1; >> + else >> + priv_ep->endpoint.caps.dir_out =3D 1; >> + >> + if (iso_ep_reg & ep_mask) >> + priv_ep->endpoint.caps.type_iso =3D 1; >> + >> + priv_ep->endpoint.caps.type_bulk =3D 1; >> + priv_ep->endpoint.caps.type_int =3D 1; >> + priv_ep->endpoint.maxburst =3D CDNS3_EP_BUF_SIZE - 1; >> + >> + priv_ep->flags =3D 0; >> + >> + dev_info(&priv_dev->dev, "Initialized %s support: %s %s\n", >> + priv_ep->name, >> + priv_ep->endpoint.caps.type_bulk ? "BULK, INT" : "", >> + priv_ep->endpoint.caps.type_iso ? "ISO" : ""); >> + >> + list_add_tail(&priv_ep->endpoint.ep_list, >> + &priv_dev->gadget.ep_list); >> + INIT_LIST_HEAD(&priv_ep->request_list); >> + INIT_LIST_HEAD(&priv_ep->ep_match_pending_list); >> + } >> + >> + priv_dev->ep_nums =3D found_endpoints; >> + return 0; >> +} >> + >> +static void cdns3_gadget_release(struct device *dev) >> +{ >> + struct cdns3_device *priv_dev; >> + >> + priv_dev =3D container_of(dev, struct cdns3_device, dev); >> + kfree(priv_dev); >> +} >> + >> +static int __cdns3_gadget_init(struct cdns3 *cdns) >> +{ >> + struct cdns3_device *priv_dev; >> + struct device *dev; >> + int ret; >> + >> + priv_dev =3D kzalloc(sizeof(*priv_dev), GFP_KERNEL); >> + if (!priv_dev) >> + return -ENOMEM; >> + >> + dev =3D &priv_dev->dev; >> + dev->release =3D cdns3_gadget_release; >> + dev->parent =3D cdns->dev; >> + dev_set_name(dev, "gadget-cdns3"); >> + cdns->gadget_dev =3D dev; >> + >> + priv_dev->sysdev =3D cdns->dev; >> + ret =3D device_register(dev); > >Why do you need to create this dummy device? you could just use cdns3->dev= . Peter do you have any suggestion ? It's works as garbage collector during changing role. In my opinion this i= s=20 the only advantage. > >> + if (ret) >> + goto err1; >> + >> + priv_dev->regs =3D cdns->dev_regs; >> + >> + /* fill gadget fields */ >> + priv_dev->gadget.max_speed =3D USB_SPEED_SUPER; >> + priv_dev->gadget.speed =3D USB_SPEED_UNKNOWN; >> + //TODO: Add implementation of cdns3_gadget_ops >> + //priv_dev->gadget.ops =3D &cdns3_gadget_ops; >> + priv_dev->gadget.name =3D "usb-ss-gadget"; >> + priv_dev->gadget.sg_supported =3D 1; >> + priv_dev->is_connected =3D 0; >> + >> + spin_lock_init(&priv_dev->lock); >> + >> + priv_dev->in_standby_mode =3D 1; >> + >> + /* initialize endpoint container */ >> + INIT_LIST_HEAD(&priv_dev->gadget.ep_list); >> + INIT_LIST_HEAD(&priv_dev->ep_match_list); >> + >> + ret =3D cdns3_init_ep0(priv_dev); >> + if (ret) { >> + dev_err(dev, "Failed to create endpoint 0\n"); >> + ret =3D -ENOMEM; > >why not just use the ret as is. > >> + goto err2; >> + } >> + >> + ret =3D cdns3_init_ep(priv_dev); >> + if (ret) { >> + dev_err(dev, "Failed to create non zero endpoints\n"); >> + ret =3D -ENOMEM; > >here too Yes, sure. > >> + goto err2; >> + } >> + >> + /* allocate memory for default endpoint TRB */ >> + priv_dev->trb_ep0 =3D dma_alloc_coherent(priv_dev->sysdev, 24, > >what is 24? >In error path you are using TRB_SIZE * 2. Should we be using that here too= ? Yes we need TRB_SIZE * 2. The second is for DMULT mode. DMA stop working on= incorrect TRB. =20 > >> + &priv_dev->trb_ep0_dma, GFP_DMA); >> + if (!priv_dev->trb_ep0) { >> + dev_err(dev, "Failed to allocate memory for ep0 TRB\n"); >> + ret =3D -ENOMEM; >> + goto err2; >> + } >> + >> + /* allocate memory for setup packet buffer */ >> + priv_dev->setup =3D dma_alloc_coherent(priv_dev->sysdev, 8, > >is 8 enough for all use cases? What about vendor specific setup requests? It's only for SETUP packet. It always has 8 bytes. For data stage for vend= or request=20 buffer will be delivered by vendor gadget drivers.=20 > >> + &priv_dev->setup_dma, GFP_DMA); >> + if (!priv_dev->setup) { >> + dev_err(dev, "Failed to allocate memory for SETUP buffer\n"); >> + ret =3D -ENOMEM; >> + goto err3; >> + } >> + >> + dev_dbg(dev, "Device Controller version: %08x\n", >> + readl(&priv_dev->regs->usb_cap6)); >> + dev_dbg(dev, "USB Capabilities:: %08x\n", >> + readl(&priv_dev->regs->usb_cap1)); >> + dev_dbg(dev, "On-Chip memory cnfiguration: %08x\n", >> + readl(&priv_dev->regs->usb_cap2)); >> + >> + /* add USB gadget device */ >> + ret =3D usb_add_gadget_udc(&priv_dev->dev, &priv_dev->gadget); >> + if (ret < 0) { >> + dev_err(dev, "Failed to register USB device controller\n"); >> + goto err4; >> + } >> + >> + priv_dev->zlp_buf =3D kzalloc(ENDPOINT_ZLP_BUF_SIZE, GFP_KERNEL); >> + if (!priv_dev->zlp_buf) { >> + ret =3D -ENOMEM; >> + goto err4; >> + } > >how about allocating zlp_buf before usb_add_gadget_udc()? Yes, sure. It will be safer.=20 > >> + >> + return 0; >> +err4: >> + dma_free_coherent(priv_dev->sysdev, 8, priv_dev->setup, >> + priv_dev->setup_dma); >> +err3: >> + dma_free_coherent(priv_dev->sysdev, TRB_SIZE * 2, priv_dev->trb_ep0, >> + priv_dev->trb_ep0_dma); >> +err2: >> + device_del(dev); >> +err1: >> + put_device(dev); >> + cdns->gadget_dev =3D NULL; >> + return ret; >> +} >> + >> +/** >> + * cdns3_gadget_remove: parent must call this to remove UDC >> + * >> + * cdns: cdns3 instance >> + */ >> +void cdns3_gadget_remove(struct cdns3 *cdns) > >how about calling this cdns3_gadget_exit() to complememnt cdns3_gadget_ini= t() Function will be removed in next version. Code will be moved to cdns3_gadge= t_stop function > >> +{ >> + struct cdns3_device *priv_dev; >> + >> + if (!cdns->roles[CDNS3_ROLE_GADGET]) >> + return; > >why this check? It will lead to an unbalanced exit() and future init(). > Will be removed. >> + >> + priv_dev =3D container_of(cdns->gadget_dev, struct cdns3_device, dev); >> + usb_del_gadget_udc(&priv_dev->gadget); >> + dma_free_coherent(priv_dev->sysdev, 8, priv_dev->setup, >> + priv_dev->setup_dma); >> + dma_free_coherent(priv_dev->sysdev, TRB_SIZE * 2, priv_dev->trb_ep0, >> + priv_dev->trb_ep0_dma); > >You need to free all memory allocation done in cdns3_init_ep0() and cdns3_= init_ep() >else we eat memory on every role switch if we plan on getting rid of the d= ummy gadget_dev. Ok,=20 > >> + device_unregister(cdns->gadget_dev); >> + cdns->gadget_dev =3D NULL; >> + kfree(priv_dev->zlp_buf); >> +} >> + >> +static int cdns3_gadget_start(struct cdns3 *cdns) >> +{ >> + struct cdns3_device *priv_dev =3D container_of(cdns->gadget_dev, >> + struct cdns3_device, dev); >> + unsigned long flags; >> + >> + pm_runtime_get_sync(cdns->dev); > >This is another reason why we shouldn't be creating a dummy gadget_dev. >PM is tied to cdns->dev. > >> + spin_lock_irqsave(&priv_dev->lock, flags); >> + priv_dev->start_gadget =3D 1; >> + >> + if (!priv_dev->gadget_driver) { >> + spin_unlock_irqrestore(&priv_dev->lock, flags); >> + return 0; >> + } >> + >> + cdns3_gadget_config(priv_dev); >> + priv_dev->in_standby_mode =3D 0; >> + spin_unlock_irqrestore(&priv_dev->lock, flags); >> + return 0; >> +} >> + >> +static void __cdns3_gadget_stop(struct cdns3 *cdns) >> +{ >> + struct cdns3_device *priv_dev; >> + unsigned long flags; >> + >> + priv_dev =3D container_of(cdns->gadget_dev, struct cdns3_device, dev); >> + >> + if (priv_dev->gadget_driver) >> + priv_dev->gadget_driver->disconnect(&priv_dev->gadget); >> + >> + usb_gadget_disconnect(&priv_dev->gadget); >> + spin_lock_irqsave(&priv_dev->lock, flags); >> + priv_dev->gadget.speed =3D USB_SPEED_UNKNOWN; >> + >> + /* disable interrupt for device */ >> + writel(0, &priv_dev->regs->usb_ien); >> + writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf); >> + priv_dev->start_gadget =3D 0; >> + spin_unlock_irqrestore(&priv_dev->lock, flags); >> +} >> + >> +static void cdns3_gadget_stop(struct cdns3 *cdns) >> +{ >> + if (cdns->role =3D=3D CDNS3_ROLE_GADGET) >> + __cdns3_gadget_stop(cdns); > >Why worry about role here? That should be job of core/drd driver. Yes, will be corrected. > >> + >> + pm_runtime_mark_last_busy(cdns->dev); >> + pm_runtime_put_autosuspend(cdns->dev); >> +} >> + >> +static int cdns3_gadget_suspend(struct cdns3 *cdns, bool do_wakeup) >> +{ >> + __cdns3_gadget_stop(cdns); >> + return 0; >> +} >> + >> +static int cdns3_gadget_resume(struct cdns3 *cdns, bool hibernated) >> +{ >> + struct cdns3_device *priv_dev; >> + unsigned long flags; >> + >> + priv_dev =3D container_of(cdns->gadget_dev, struct cdns3_device, dev); >> + spin_lock_irqsave(&priv_dev->lock, flags); >> + priv_dev->start_gadget =3D 1; > >who is using this start_gadget flag? It's look like guard protecting before access to register when device is di= sabled.=20 It's no longer necessary in next version and will be removed. When device = role=20 Is active then access to registers is enabled.=20 > >> + if (!priv_dev->gadget_driver) { >> + spin_unlock_irqrestore(&priv_dev->lock, flags); >> + return 0; >> + } >> + >> + cdns3_gadget_config(priv_dev); >> + priv_dev->in_standby_mode =3D 0; >> + spin_unlock_irqrestore(&priv_dev->lock, flags); >> + return 0; >> +} >> + >> +/** >> + * cdns3_gadget_init - initialize device structure >> + * >> + * cdns: cdns3 instance >> + * >> + * This function initializes the gadget. >> + */ >> +int cdns3_gadget_init(struct cdns3 *cdns) >> +{ >> + struct cdns3_role_driver *rdrv; >> + >> + rdrv =3D devm_kzalloc(cdns->dev, sizeof(*rdrv), GFP_KERNEL); >> + if (!rdrv) >> + return -ENOMEM; >> + >> + rdrv->start =3D cdns3_gadget_start; >> + rdrv->stop =3D cdns3_gadget_stop; > >Why not use gadget_init()/gadget_exit() here? That sounds much cleaner >as you won't have ISR's active after a role stop. Without cdns3 prefix ?=20 cdns3_gadget_exit has removed so we need cdns3_gadget_stop as non static fu= nction.=20 So I can rename cdns3_gadget_stop function to cdns3_gadget_exti, but cdn3_= gadget_start is busy.=20 cdn3_gadget_start is used only internally in this file so maybe she can be = called gadget_init or __ cdn3_gadget_init > >> + rdrv->suspend =3D cdns3_gadget_suspend; >> + rdrv->resume =3D cdns3_gadget_resume; >> + rdrv->irq =3D cdns3_irq_handler_thread; >> + rdrv->name =3D "gadget"; >> + cdns->roles[CDNS3_ROLE_GADGET] =3D rdrv; >> + return __cdns3_gadget_init(cdns); >> +} >> diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h >> index 75ca6214e79a..3b0d4d2e4831 100644 >> --- a/drivers/usb/cdns3/gadget.h >> +++ b/drivers/usb/cdns3/gadget.h >> @@ -1068,4 +1068,8 @@ struct cdns3_device { >> struct usb_request *pending_status_request; >> }; >> >> +void cdns3_set_register_bit(void __iomem *ptr, u32 mask); >> +int cdns3_init_ep0(struct cdns3_device *priv_dev); >> +void cdns3_ep0_config(struct cdns3_device *priv_dev); >> +void cdns3_select_ep(struct cdns3_device *priv_dev, u32 ep); >> #endif /* __LINUX_CDNS3_GADGET */ >> > >cheers, >-roger > >-- >Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. >Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki