Re: [RFC v2 3/8] ci13xxx_udc: rename register layouts

[Felipe Balbi <balbi@xxxxxx>]

Hi,

On Tue, Apr 10, 2012 at 05:25:12PM +0300, Alexander Shishkin wrote:
> Currently, the register prefixes in the driver seem to be mixed: the
> capability registers are the ones that contain capability information,
> such as number of hardware endpoints, while the registers that are
> used to program the controller are called operational registers.
> 
> Normally, capability registers start at 0x100 offset of the register
> window and are followed by operational registers. In some versions,
> however, capability registers start at 0x0 offset.
> 
> This patch renames the register and adjusts their offsets appropriately,
> leaving the possibility of having a non-standard capability offset.
> 
> I couldn't find any mentions of the TESTMODE register anywhere, so I
> suspect it might only be enabled in chipidea internal versions of the
> controller and I'm really inclined to remove it from the driver or at
> least hiding it behind a config option.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> ---
>  drivers/usb/gadget/ci13xxx_msm.c |    3 +-
>  drivers/usb/gadget/ci13xxx_pci.c |    7 +-
>  drivers/usb/gadget/ci13xxx_udc.c |  229 +++++++++++++++++++-------------------
>  drivers/usb/gadget/ci13xxx_udc.h |    3 +
>  4 files changed, 126 insertions(+), 116 deletions(-)
> 
> diff --git a/drivers/usb/gadget/ci13xxx_msm.c b/drivers/usb/gadget/ci13xxx_msm.c
> index d07e44c..6e77446 100644
> --- a/drivers/usb/gadget/ci13xxx_msm.c
> +++ b/drivers/usb/gadget/ci13xxx_msm.c
> @@ -79,7 +79,8 @@ static int ci13xxx_msm_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	ret = udc_probe(&ci13xxx_msm_udc_driver, &pdev->dev, regs);
> +	ret = udc_probe(&ci13xxx_msm_udc_driver, &pdev->dev, regs,
> +			DEF_CAPOFFSET);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "udc_probe failed\n");
>  		goto iounmap;
> diff --git a/drivers/usb/gadget/ci13xxx_pci.c b/drivers/usb/gadget/ci13xxx_pci.c
> index 883ab5e..59373d5 100644
> --- a/drivers/usb/gadget/ci13xxx_pci.c
> +++ b/drivers/usb/gadget/ci13xxx_pci.c
> @@ -55,6 +55,7 @@ static int __devinit ci13xxx_pci_probe(struct pci_dev *pdev,
>  				       const struct pci_device_id *id)
>  {
>  	void __iomem *regs = NULL;
> +	uintptr_t capoffset = DEF_CAPOFFSET;
>  	int retval = 0;
>  
>  	if (id == NULL)
> @@ -86,7 +87,11 @@ static int __devinit ci13xxx_pci_probe(struct pci_dev *pdev,
>  	pci_set_master(pdev);
>  	pci_try_set_mwi(pdev);
>  
> -	retval = udc_probe(&ci13xxx_pci_udc_driver, &pdev->dev, regs);
> +	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
> +		capoffset = 0;
> +
> +	retval = udc_probe(&ci13xxx_pci_udc_driver, &pdev->dev, regs,
> +			   capoffset);
>  	if (retval)
>  		goto iounmap;
>  
> diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
> index d4b9c1e..9da5044 100644
> --- a/drivers/usb/gadget/ci13xxx_udc.c
> +++ b/drivers/usb/gadget/ci13xxx_udc.c
> @@ -138,7 +138,8 @@ static int ffs_nr(u32 x)
>  static struct {
>  	unsigned      lpm;    /* is LPM? */
>  	void __iomem *abs;    /* bus map offset */
> -	void __iomem *cap;    /* bus map offset + CAP offset + CAP data */
> +	void __iomem *cap;    /* bus map offset + CAP offset */
> +	void __iomem *op;     /* bus map offset + OP offset */
>  	size_t        size;   /* bank size */
>  } hw_bank;
>  
> @@ -146,26 +147,26 @@ static struct {
>  #define ABS_AHBBURST        (0x0090UL)
>  #define ABS_AHBMODE         (0x0098UL)
>  /* UDC register map */
> -#define ABS_CAPLENGTH       (0x100UL)
> -#define ABS_HCCPARAMS       (0x108UL)
> -#define ABS_DCCPARAMS       (0x124UL)
> +#define CAP_CAPLENGTH	    (0x000UL)
> +#define CAP_HCCPARAMS	    (0x008UL)
> +#define CAP_DCCPARAMS	    (0x024UL)
>  #define ABS_TESTMODE        (hw_bank.lpm ? 0x0FCUL : 0x138UL)
>  /* offset to CAPLENTGH (addr + data) */
> -#define CAP_USBCMD          (0x000UL)
> -#define CAP_USBSTS          (0x004UL)
> -#define CAP_USBINTR         (0x008UL)
> -#define CAP_DEVICEADDR      (0x014UL)
> -#define CAP_ENDPTLISTADDR   (0x018UL)
> -#define CAP_PORTSC          (0x044UL)
> -#define CAP_DEVLC           (0x084UL)
> -#define CAP_USBMODE         (hw_bank.lpm ? 0x0C8UL : 0x068UL)
> -#define CAP_ENDPTSETUPSTAT  (hw_bank.lpm ? 0x0D8UL : 0x06CUL)
> -#define CAP_ENDPTPRIME      (hw_bank.lpm ? 0x0DCUL : 0x070UL)
> -#define CAP_ENDPTFLUSH      (hw_bank.lpm ? 0x0E0UL : 0x074UL)
> -#define CAP_ENDPTSTAT       (hw_bank.lpm ? 0x0E4UL : 0x078UL)
> -#define CAP_ENDPTCOMPLETE   (hw_bank.lpm ? 0x0E8UL : 0x07CUL)
> -#define CAP_ENDPTCTRL       (hw_bank.lpm ? 0x0ECUL : 0x080UL)
> -#define CAP_LAST            (hw_bank.lpm ? 0x12CUL : 0x0C0UL)
> +#define OP_USBCMD	    (0x000UL)
> +#define OP_USBSTS	    (0x004UL)
> +#define OP_USBINTR	    (0x008UL)
> +#define OP_DEVICEADDR	    (0x014UL)
> +#define OP_ENDPTLISTADDR    (0x018UL)
> +#define OP_PORTSC	    (0x044UL)
> +#define OP_DEVLC	    (0x084UL)
> +#define OP_USBMODE	    (hw_bank.lpm ? 0x0C8UL : 0x068UL)
> +#define OP_ENDPTSETUPSTAT   (hw_bank.lpm ? 0x0D8UL : 0x06CUL)
> +#define OP_ENDPTPRIME	    (hw_bank.lpm ? 0x0DCUL : 0x070UL)
> +#define OP_ENDPTFLUSH	    (hw_bank.lpm ? 0x0E0UL : 0x074UL)
> +#define OP_ENDPTSTAT	    (hw_bank.lpm ? 0x0E4UL : 0x078UL)
> +#define OP_ENDPTCOMPLETE    (hw_bank.lpm ? 0x0E8UL : 0x07CUL)
> +#define OP_ENDPTCTRL	    (hw_bank.lpm ? 0x0ECUL : 0x080UL)
> +#define OP_LAST		    (hw_bank.lpm ? 0x12CUL : 0x0C0UL)
>  
>  /* maximum number of enpoints: valid only after hw_device_reset() */
>  static unsigned hw_ep_max;
> @@ -193,85 +194,85 @@ static int ep_to_bit(int n)
>  }
>  
>  /**
> - * hw_aread: reads from register bitfield
> - * @addr: address relative to bus map
> + * hw_cread: reads from capability register bitfield
> + * @addr: address relative to capability register base
>   * @mask: bitfield mask
>   *
>   * This function returns register bitfield data
>   */
> -static u32 hw_aread(u32 addr, u32 mask)
> +static u32 hw_cread(u32 addr, u32 mask)
>  {
> -	return ioread32(addr + hw_bank.abs) & mask;
> +	return ioread32(addr + hw_bank.cap) & mask;
>  }
>  
>  /**
> - * hw_awrite: writes to register bitfield
> - * @addr: address relative to bus map
> + * hw_cwrite: writes to capability register bitfield
> + * @addr: address relative to capability register base
>   * @mask: bitfield mask
>   * @data: new data
>   */
> -static void hw_awrite(u32 addr, u32 mask, u32 data)
> +static void hw_cwrite(u32 addr, u32 mask, u32 data)
>  {
> -	iowrite32(hw_aread(addr, ~mask) | (data & mask),
> -		  addr + hw_bank.abs);
> +	iowrite32(hw_cread(addr, ~mask) | (data & mask),
> +		  addr + hw_bank.cap);
>  }
>  
>  /**
> - * hw_cread: reads from register bitfield
> - * @addr: address relative to CAP offset plus content
> + * hw_oread: reads from operational register bitfield
> + * @addr: address relative to operational  register base
>   * @mask: bitfield mask
>   *
>   * This function returns register bitfield data
>   */
> -static u32 hw_cread(u32 addr, u32 mask)
> +static u32 hw_oread(u32 addr, u32 mask)
>  {
> -	return ioread32(addr + hw_bank.cap) & mask;
> +	return ioread32(addr + hw_bank.op) & mask;
>  }
>  
>  /**
> - * hw_cwrite: writes to register bitfield
> - * @addr: address relative to CAP offset plus content
> + * hw_owrite: writes to operational register bitfield
> + * @addr: address relative to operational register base
>   * @mask: bitfield mask
>   * @data: new data
>   */
> -static void hw_cwrite(u32 addr, u32 mask, u32 data)
> +static void hw_owrite(u32 addr, u32 mask, u32 data)
>  {
> -	iowrite32(hw_cread(addr, ~mask) | (data & mask),
> -		  addr + hw_bank.cap);
> +	iowrite32(hw_oread(addr, ~mask) | (data & mask),
> +		  addr + hw_bank.op);
>  }

why do you need separate functions to read capability or operational
registers ? They look the same. You could just define them a little
differently:

static u32 hw_read(void *base, u32 offset, u32 mask)
{
	return ioread32(base + offset) & mask;
}

then, when calling it you can use:

hw_read(hw_bank.op, ADDRESS, mask);
hw_read(hw_bank.cap, ADDRESS, mask);

-- 
balbi

Attachment: signature.asc
Description: Digital signature