RE: [PATCH 1/4] usb: cdns2: Device side header file for CDNS2 driver

[Pawel Laszczak]

>-----Original Message-----
>From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
>Sent: Thursday, February 16, 2023 12:24 PM
>To: Pawel Laszczak <pawell@xxxxxxxxxxx>
>Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
>Daisy.Barrera@xxxxxxxxxxxx; Cliff.Holden@xxxxxxxxxxxx; arnd@xxxxxxxx;
>tony@xxxxxxxxxxx; jdelvare@xxxxxxx; neal_liu@xxxxxxxxxxxxxx;
>linus.walleij@xxxxxxxxxx; egtvedt@xxxxxxxxxxxx; biju.das.jz@xxxxxxxxxxxxxx;
>herve.codina@xxxxxxxxxxx
>Subject: Re: [PATCH 1/4] usb: cdns2: Device side header file for CDNS2 driver
>
>EXTERNAL MAIL
>
>
>On Thu, Feb 16, 2023 at 05:54:08AM -0500, Pawel Laszczak wrote:
>> Patch defines macros, registers and structures used by Device side
>> driver.
>>
>> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
>> ---
>>  drivers/usb/gadget/udc/cdns2/cdns2-gadget.h | 670
>> ++++++++++++++++++++
>>  1 file changed, 670 insertions(+)
>>  create mode 100644 drivers/usb/gadget/udc/cdns2/cdns2-gadget.h
>>
>> diff --git a/drivers/usb/gadget/udc/cdns2/cdns2-gadget.h
>> b/drivers/usb/gadget/udc/cdns2/cdns2-gadget.h
>> new file mode 100644
>> index 000000000000..66cdb902163c
>> --- /dev/null
>> +++ b/drivers/usb/gadget/udc/cdns2/cdns2-gadget.h
>> @@ -0,0 +1,670 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * USBHS-DEV device controller driver header file
>> + *
>> + * Copyright (C) 2023 Cadence.
>> + *
>> + * Author: Pawel Laszczak <pawell@xxxxxxxxxxx>  */
>> +
>> +#ifndef __LINUX_CDNS2_GADGET
>> +#define __LINUX_CDNS2_GADGET
>> +
>> +#include <linux/usb/gadget.h>
>> +#include <linux/dma-direction.h>
>> +
>> +/*
>> + * USBHS register interface.
>> + * This corresponds to the USBHS Device Controller Interface.
>> + */
>> +
>> +/**
>> + * struct cdns2_ep0_regs - endpoint 0 related registers.
>> + * @rxbc: receive (OUT) 0 endpoint byte count register.
>> + * @txbc: transmit (IN) 0 endpoint byte count register.
>> + * @cs: 0 endpoint control and status register.
>> + * @fifo: 0 endpoint fifo register.
>> + * @setupdat: SETUP data register.
>> + * @maxpack: 0 endpoint max packet size.
>
>You didn't describe the "reserved" fields in here, so you will get a kerneldoc
>warning :(
>
>> + */
>> +struct cdns2_ep0_regs {
>> +	__u8 rxbc;
>> +	__u8 txbc;
>> +	__u8 cs;
>> +	__u8 reserved1[4];
>> +	__u8 fifo;
>> +	__le32 reserved2[94];
>> +	__u8 setupdat[8];
>> +	__u8 reserved4[88];
>
>Why are these all reserved?  Should they be set to 0?  Something else?
>
>Same with the other reserved fields in this file, you need to fix up the
>kerneldoc for them too.
>

Structure describes the part of register map related with ep0 endpoint.
In the reserved space there can be other registers related with another controller 
logic.  Not all reserved field are set to 0. 
The registers map is quite big and confusing, and I tried to split them into some group
to make driver more readable.

I will add some descriptions for all reserved* field in this file.

Thanks,

Pawel Laszczak