Re: [U-Boot] [PATCH v2 09/13] pci: Use common functions to read/write config

Hi Simon,

On Sun, Nov 22, 2015 at 7:28 AM, Simon Glass sjg@chromium.org wrote:

Currently we use switch() and access PCI configuration via several functions, one for each data size. Adjust the code to use generic functions, where the data size is a parameter.

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2:

• Add back the leading 0 in the printf() statements

common/cmd_pci.c | 49 +++++++++++++++---------------------------------- 1 file changed, 15 insertions(+), 34 deletions(-)

diff --git a/common/cmd_pci.c b/common/cmd_pci.c index 7b83c1d..51e1f1c 100644 --- a/common/cmd_pci.c +++ b/common/cmd_pci.c @@ -325,7 +325,8 @@ static pci_dev_t get_pci_dev(char *name) return PCI_BDF(bdfs[0], bdfs[1], bdfs[2]); }

-static int pci_cfg_display(pci_dev_t bdf, ulong addr, ulong size, ulong length) +static int pci_cfg_display(pci_dev_t bdf, ulong addr, enum pci_size_t size,

Here parameter 'size' has been changed from 1/2/4 to type enum pci_size_t whose valid number is 0/1/2.

•                      ulong length)
  

{ #define DISP_LINE_LEN 16 ulong i, nbytes, linebytes;

The following two lines will cause divide by zero exception when size is 0.

335 if (length == 0) 336 length = 0x40 / size; /* Standard PCI configuration space */

@@ -339,23 +340,13 @@ static int pci_cfg_display(pci_dev_t bdf, ulong addr, ulong size, ulong length) */ nbytes = length * size;

And here nbytes is not correct too.

    do {

•           uint    val4;
  

•           ushort  val2;
  

•           u_char  val1;
  

•           printf("%08lx:", addr);
            linebytes = (nbytes>DISP_LINE_LEN)?DISP_LINE_LEN:nbytes;
            for (i=0; i<linebytes; i+= size) {
  

•                   if (size == 4) {
  

•                           pci_read_config_dword(bdf, addr, &val4);
  

•                           printf(" %08x", val4);
  

•                   } else if (size == 2) {
  

•                           pci_read_config_word(bdf, addr, &val2);
  

•                           printf(" %04x", val2);
  

•                   } else {
  

•                           pci_read_config_byte(bdf, addr, &val1);
  

•                           printf(" %02x", val1);
  

•                   }
  

•                   unsigned long val;
  

•                   val = pci_read_config(bdf, addr, size);
  

•                   printf(" %0*lx", pci_field_width(size), val);
                    addr += size;
            }
            printf("\n");
  

@@ -385,32 +376,20 @@ static int pci_cfg_write (pci_dev_t bdf, ulong addr, ulong size, ulong value) return 0; }

-static int -pci_cfg_modify (pci_dev_t bdf, ulong addr, ulong size, ulong value, int incrflag) +static int pci_cfg_modify(pci_dev_t bdf, ulong addr, enum pci_size_t size,

•                     ulong value, int incrflag)
  

{ ulong i; int nbytes;

•   uint    val4;
  

•   ushort  val2;
  

•   u_char  val1;
  

•   ulong val;
  
    /* Print the address, followed by value.  Then accept input for
     * the next value.  A non-converted value exits.
     */
    do {
            printf("%08lx:", addr);
  

•           if (size == 4) {
  

•                   pci_read_config_dword(bdf, addr, &val4);
  

•                   printf(" %08x", val4);
  

•           }
  

•           else if (size == 2) {
  

•                   pci_read_config_word(bdf, addr, &val2);
  

•                   printf(" %04x", val2);
  

•           }
  

•           else {
  

•                   pci_read_config_byte(bdf, addr, &val1);
  

•                   printf(" %02x", val1);
  

•           }
  

•           val = pci_read_config(bdf, addr, size);
  

•           printf(" %0*lx", pci_field_width(size), val);
  
            nbytes = cli_readline(" ? ");
            if (nbytes == 0 || (nbytes == 1 && console_buffer[0] == '-')) {
  

@@ -456,7 +435,8 @@ pci_cfg_modify (pci_dev_t bdf, ulong addr, ulong size, ulong value, int incrflag */ static int do_pci(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) {

•   ulong addr = 0, value = 0, size = 0;
  

•   ulong addr = 0, value = 0, cmd_size = 0;
  

•   enum pci_size_t size;
    int busnum = 0;
    pci_dev_t bdf = 0;
    char cmd = 's';
  

@@ -471,7 +451,8 @@ static int do_pci(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) case 'm': /* modify */ case 'w': /* write */ /* Check for a size specification. */

•           size = cmd_get_data_size(argv[1], 4);
  

•           cmd_size = cmd_get_data_size(argv[1], 4);
  

•           size = (cmd_size == 4) ? PCI_SIZE_32 : size - 1;
  

This is wrong, should be:

size = (cmd_size == 4) ? PCI_SIZE_32 : cmd_size - 1;

            if (argc > 3)
                    addr = simple_strtoul(argv[3], NULL, 16);
            if (argc > 4)

--

Regards, Bin