[U-Boot] [PATCH] Add support for kirkwood sata controller

[Wolfgang Denk]

Dear Tor Krill,

In message <1277387833-6925-1-git-send-email-tor at excito.com> you wrote:
> 
> A rudimentary sata driver for the Marvell Kirkwood CPU based on
> the architecture of the fsl_sata driver.
> 
> Signed-off-by: Tor Krill <tor at excito.com>
> ---
>  drivers/block/Makefile  |    1 +
>  drivers/block/sata_mv.c |  800 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/block/sata_mv.h |  229 ++++++++++++++
>  3 files changed, 1030 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/block/sata_mv.c
>  create mode 100644 drivers/block/sata_mv.h
...
> +#ifdef MALLOC_QUEUE
> +	void		*crqb_alloc;
> +	struct crqb	*request;
> +
> +	void		*crpb_alloc;
> +	struct crpb	*response;
> +
> +#else
> +	struct crqb request[REQUEST_QUEUE_SIZE] __attribute__ ((aligned(CRQB_ALIGN)));
> +	struct crpb response[REQUEST_QUEUE_SIZE] __attribute__ ((aligned(CRPB_ALIGN)));
> +#endif

What about always using pointers for request and response, and
initializing these statically (taking the address of a diffrently
named static array) in one case and dynamically (from malloc() in the
other case?   Then we would need the #ifdef's only in one or two
places, and avoid constructs like these:

> +#ifdef MALLOC_QUEUE
> +	out_le32(priv->regbase+EDMA_RQIPR, priv->request);
> +#else
> +	out_le32(priv->regbase+EDMA_RQIPR, &priv->request);
> +#endif
> +	out_le32(priv->regbase+EDMA_RQOPR, 0x0);
> +
> +	/* Setup response queue */
> +	out_le32(priv->regbase+EDMA_RSBA_HI, 0x0);
> +#ifdef MALLOC_QUEUE
> +	out_le32(priv->regbase+EDMA_RSOPR, priv->response);
> +#else
> +	out_le32(priv->regbase+EDMA_RSOPR, &priv->response);
> +#endif
> +	out_le32(priv->regbase+EDMA_RSIPR, 0x0);

...
> +	debug("Probe port: %d\n\r",port);
> +
> +	for ( tries=0; tries<2; tries++){

No space after the '('.

> +		tmp = in_le32(priv->regbase + SIR_SCONTROL);
> +		tries2 = 5;
> +		do{

Space: "do {"

> +		if ( !set15 ){

Make this:

	if (!set15) {

> +	tmp = in_le32(priv->regbase+EDMA_RQIPR) & EDMA_RQIPR_IPMASK;
> +	tmp = tmp >> EDMA_RQIPR_IPSHIFT;
> +
> +	return tmp;

combine the last 2 lines into

	return tmp >> EDMA_RQIPR_IPSHIFT;


> +	tmp = in_le32(priv->regbase+EDMA_RQOPR) & EDMA_RQOPR_OPMASK;
> +	tmp = tmp >> EDMA_RQOPR_OPSHIFT;
> +
> +	return tmp;

Ditto, and so on.

> +
> +/* Get response queue in pointer */
> +static int get_rspip(int port){
> +	u32 tmp;
> +	struct mv_priv *priv = (struct mv_priv *) sata_dev_desc[port].priv;
> +
> +	tmp = in_le32(priv->regbase+EDMA_RSIPR) & EDMA_RSIPR_IPMASK;
> +	tmp = tmp >> EDMA_RSIPR_IPSHIFT;
> +
> +	return tmp;
> +}
> +
> +/* Get response queue out pointer */
> +static int get_rspop(int port){
> +	u32 tmp;
> +	struct mv_priv *priv = (struct mv_priv *) sata_dev_desc[port].priv;
> +	if ( (res = ata_wait_register(
> +			(u32*)(KW_SATAHC_BASE+SATAHC_ICR), tmp ,tmp, timeout_msec))){

Line too long, spaces after ',' (not before), space before '{'.

> +		printf("Failed to wait for completion on port %d\n",port);

Space after comma.

> +	if ( port == 0 ){
> +		tmp &= ~( 1<<0 | 1<<8);
> +	}else{
> +		tmp &= ~(1<<1 | 1<<9);
> +	}

No braces needed for one line statements. If you  use braches, they
are always separated by a space.

> +	while(get_rspip(port)!=outind){
> +		debug("Response index %d flags %08x on port %d\n",outind,priv->response[outind].flags,port);

Line too long. Please fix globally.

> +static int mv_ata_exec_ata_cmd(int port, struct sata_fis_h2d *cfis,
> +				u8 *buffer, u32 len, u32 iswrite){

Brace goes on next line.

> +	priv->request[slot].ata_sect_count |=
> +			((cfis->sector_count_exp<<CRQB_SECTCOUNT_COUNT_EXP_SHIFT) &
> +					CRQB_SECTCOUNT_COUNT_EXP_MASK);

Line too long.

> +	/* Wait for completion */
> +	if( wait_dma_completion(port,slot,10000) ){

	if (wait_dma_completion(port,slot,10000)) {

Please fix brace style globally.

> +	debug("pio %04x, mwdma %04x, udma %04x\n\r"
> +			,priv->pio, priv->mwdma, priv->udma);

Comma goes at end of first line.

> +	/* First check the device capablity */
> +	udma_cap = (u8)(priv->udma & 0xff);
> +
> +	if (udma_cap == ATA_UDMA6)
> +		cfis.sector_count = XFER_UDMA_6;
> +	if (udma_cap == ATA_UDMA5)
> +		cfis.sector_count = XFER_UDMA_5;
> +	if (udma_cap == ATA_UDMA4)
> +		cfis.sector_count = XFER_UDMA_4;
> +	if (udma_cap == ATA_UDMA3)
> +		cfis.sector_count = XFER_UDMA_3;

Use a switch() ?

Handle "default:" case?

> +	if ( dev < 0 || dev >= CONFIG_SYS_SATA_MAX_DEVICE) {
...     ----^
> +	if ( !priv ){
...	----^-----^-^
> +	memset((void*) priv,0 , sizeof(struct mv_priv));
...     --------------------^^

There are tons of such coding style issues. Please fix globally.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
HEALTH WARNING: Care Should Be Taken When Lifting This Product, Since
Its Mass, and Thus Its Weight, Is Dependent on Its Velocity  Relative
to the User.