Message-ID: <BYAPR07MB4709DAABE0A1395152375395DD590@BYAPR07MB4709.namprd07.prod.outlook.com>
Date:   Thu, 12 Jul 2018 09:19:26 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Felipe Balbi <balbi@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lukasz Tyrala <ltyrala@...ence.com>,
        Alan Douglas <adouglas@...ence.com>
Subject: RE: [PATCH 01/31] usb: usbssp: Defined register maps and other useful
 structures.

> > 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@...ence.com>
> > ---
> >  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

Powered by blists - more mailing lists