> > It is a first patch introduce Cadence USBSSP DRD controller. This
> > patch is related to device side.
> >
> > Device part of USBSSP controller base on standard XHCI specification.
> >
> > File define macros used bye USBSSP controller, structures holding and
> > grouping registers, and other object that are used by device
> > controller.
> >
> > Register map include:
> > struct usbssp_cap_regs - Capabilities Register Set.
> > struct usbssp_op_regs - Operational Register Set.
> > struct usbssp_intr_reg - Interrupter Register Set.
> > struct usbssp_run_regs - Runtime Register Set.
> >
> > Object used by hardware includes:
> > struct usbssp_doorbell_array - array used for arming command and
> > transfer rings.
> > struct usbssp_slot_ctx - holds information related to Slot Context.
> > struct usbssp_ep_ctx - hold information related to Endpoint Context.
> > struct usbssp_input_control_ctx - hold information about
> > Input Control Context.
> > struct usbssp_link_trb - define link TRB.
> > struct usbssp_transfer_event - define Transfer Event TRB.
> > struct usbssp_event_cmd - define Command Event TRB.
> > struct usbssp_generic_trb - generic used TRB object.
> >
> > Patch also add some high level object that hold information used in
> > driver.
> >
> > Some of them are:
> > struct usbssp_udc - main object in driver which is passed
> > as parameter to most of function in driver and allows
> > to access to all other structures.
> > struct usbssp_ep - holds information related to USB endpoint.
> > struct usbssp_command - describe command send to Event Ring.
> > struct usbssp_device - holds information relate do USB device.
> > struct usbssp_segment - holds information describing segments of TRBs.
> > struct usbssp_td - hold information about Transfer Descriptor.
> > struct usbssp_ring - holds information related to Transfer, Event or
> > Command ring.
> > struct usbssp_request - holds information related to single transfer.
> >
> > Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
> > ---
> > drivers/usb/usbssp/gadget.h | 1567
> > +++++++++++++++++++++++++++++++++++
> > 1 file changed, 1567 insertions(+)
> > create mode 100644 drivers/usb/usbssp/gadget.h
> >
> > diff --git a/drivers/usb/usbssp/gadget.h b/drivers/usb/usbssp/gadget.h
> > new file mode 100644 index 000000000000..486e868068b7
> > --- /dev/null
> > +++ b/drivers/usb/usbssp/gadget.h
> > @@ -0,0 +1,1567 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * USBSSP device controller driver
> > + *
> > + * Copyright (C) 2018 Cadence.
> > + *
> > + * Author: Pawel Laszczak
> > + *
> > + * A lot of code based on Linux XHCI driver.
> > + * Origin: Copyright (C) 2008 Intel Corp.
> > + */
> > +
> > +#ifndef __LINUX_USBSSP_GADGET_H
> > +#define __LINUX_USBSSP_GADGET_H
> > +
> > +#include <linux/usb.h>
> > +#include <linux/timer.h>
> > +#include <linux/kernel.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h> #include
> > +<linux/usb/gadget.h>
>
> Why does this .h file need all of these? Only include what is needed to build
> this specific file if at all possible.
I think that all are necessary to build this other file that include this one.
I will check it.
> > +
> > +/* Max number slots - only 1 is allowed */ #define DEV_MAX_SLOTS 1
> > +
> > +/* max ports for USBSSP-Dev - only 2 are allowed*/
> > +#define MAX_USBSSP_PORTS 2
> > +
> > +#define USBSSP_EP0_SETUP_SIZE 512
> > +
> > +/*16 for in and 16 for out */
>
> "/* 16..."
>
> > +#define USBSSP_ENDPOINTS_NUM 32
>
> Odd alignment of all of these defines, either make them line up or not, what
> you have is an odd mix of tabs and spaces.
>
> > +/**
> > + * struct usbssp_cap_regs - USBSSP Registers.
> > + * @hc_capbase: length of the capabilities register and DC
> version number
> > + * @hcs_params1: HCSPARAMS1 - Structural Parameters 1
> > + * @hcs_params2: HCSPARAMS2 - Structural Parameters 2
> > + * @hcs_params3: HCSPARAMS3 - Structural Parameters 3
> > + * @hcc_params: HCCPARAMS - Capability Parameters
> > + * @db_off: DBOFF - Doorbell array offset
> > + * @run_regs_off: RTSOFF - Runtime register space offset
> > + * @hcc_params2: HCCPARAMS2 Capability Parameters 2,
> > + */
> > +struct usbssp_cap_regs {
> > + __le32 hc_capbase;
> > + __le32 hcs_params1;
> > + __le32 hcs_params2;
> > + __le32 hcs_params3;
> > + __le32 hcc_params;
> > + __le32 db_off;
> > + __le32 run_regs_off;
> > + __le32 hcc_params2;
> > + /* Reserved up to (CAPLENGTH - 0x1C) */ };
>
> Does this, and your other structures that map to hardware, need to be
> __packed?
>
> > +
> > +/* Reset DC - resets internal DC state machine and all registers
> > +(except
> > + * PCI config regs).
> > + */
> > +#define CMD_RESET (1 << 1)
>
> For all of these, try using the BIT() macro.
>
> > +/* bit 2 reserved and zeroed */
> > +/* true: port has an over-current condition */
> > +#define PORT_OC (1 << 3)
> > +/* true: port reset signaling asserted */
> > +#define PORT_RESET (1 << 4)
> > +/* Port Link State - bits 5:8
> > + * A read gives the current link PM state of the port,
> > + * a write with Link State Write Strobe set sets the link state.
> > + */
> > +
> > +#define PORT_PLS_MASK (0xf << 5)
> > +#define XDEV_U0 (0x0 << 5)
>
> Your spacing just doesn't make much sense at all. Is that comment for the
> line above, or below? Please make these easier to understand, as it is, they
> are not.
>
> > +#define XDEV_RESUME (0xf << 5)
> > +
> > +/* true: port has power (see HCC_PPC) */
> > +#define PORT_POWER (1 << 9)
>
> Ok, that works, but why no blank line after this? You dive right into another
> comment:
>
> > +/* bits 10:13 indicate device speed:
> > + * 0 - undefined speed - port hasn't be initialized by a reset yet
> > + * 1 - full speed
> > + * 2 - low speed
> > + * 3 - high speed
> > + * 4 - super speed
> > + * 5-15 reserved
> > + */
> > +#define DEV_SPEED_MASK (0xf << 10)
>
> And try using the multi-line format that the rest of the kernel uses (not
> networking), it's easier to read.
>
> > +/* Port Link State Write Strobe - set this when changing link state */
> > +#define PORT_LINK_STROBE (1 << 16)
>
> Again with the BIT() usage.
>
> Just clean up the whitespace to make this whole file easier to read please,
> I'm stopping reviewing here.
>
> Oops, no, one final thing:
>
> > +#define usbssp_dbg(usbssp_data, fmt, args...) \
> > + dev_dbg(usbssp_data->dev, fmt, ## args)
> > +
> > +#define usbssp_err(usbssp_data, fmt, args...) \
> > + dev_err(usbssp_data->dev, fmt, ## args)
> > +
> > +#define usbssp_warn(usbssp_data, fmt, args...) \
> > + dev_warn(usbssp_data->dev, fmt, ## args)
> > +
> > +#define usbssp_warn_ratelimited(usbssp_data, fmt, args...) \
> > + dev_warn_ratelimited(usbssp_data->dev, fmt, ## args)
> > +
> > +#define usbssp_info(usbssp_data, fmt, args...) \
> > + dev_info(usbssp_data->dev, fmt, ## args)
>
> All drivers feel they are unique and special like all other drivers, so they want
> to create their own debugging macros. Don't do it, just use
> dev_dbg() and friends directly please. It saves no one any time to use special
> ones as then everyone has to go and look up exactly what they do.
>
> Don't be special, you are like all of the rest of us :)
I thought it was a standard, I saw this approach in many divers 😊
I will change it.
thanks,
Pawel Laszczak
��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
