Re: [PATCH v9 3/5] drivers/soc/litex: add LiteX SoC Controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Mateusz,

There may be a problem here, see below.

On Sun, Jul 26, 2020 at 06:14:16AM +0900, Stafford Horne wrote:
> On Sat, Jul 25, 2020 at 06:29:51PM +0800, kernel test robot wrote:
> > Hi Mateusz,
> > 
> > Thank you for the patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on robh/for-next]
> > [also build test WARNING on tty/tty-testing usb/usb-testing staging/staging-testing driver-core/driver-core-testing linus/master v5.8-rc6 next-20200724]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Mateusz-Holenko/LiteX-SoC-controller-and-LiteUART-serial-driver/20200722-183024
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> > config: openrisc-randconfig-s032-20200725 (attached as .config)
> > compiler: or1k-linux-gcc (GCC) 9.3.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # apt-get install sparse
> >         # sparse version: v0.6.2-93-g4c6cbe55-dirty
> >         # save the attached .config to linux build tree
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=openrisc 
> 
> Note, this is OpenRISC
> 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > 
> > 
> > sparse warnings: (new ones prefixed by >>)
> > 
> > >> drivers/soc/litex/litex_soc_ctrl.c:61:24: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] value @@     got restricted __le32 [usertype] @@
> > >> drivers/soc/litex/litex_soc_ctrl.c:61:24: sparse:     expected unsigned int [usertype] value
> > >> drivers/soc/litex/litex_soc_ctrl.c:61:24: sparse:     got restricted __le32 [usertype]
> > >> drivers/soc/litex/litex_soc_ctrl.c:77:32: sparse: sparse: cast to restricted __le32
> > >> drivers/soc/litex/litex_soc_ctrl.c:77:32: sparse: sparse: cast to restricted __le32
> > >> drivers/soc/litex/litex_soc_ctrl.c:77:32: sparse: sparse: cast to restricted __le32
> > >> drivers/soc/litex/litex_soc_ctrl.c:77:32: sparse: sparse: cast to restricted __le32
> > >> drivers/soc/litex/litex_soc_ctrl.c:77:32: sparse: sparse: cast to restricted __le32
> > >> drivers/soc/litex/litex_soc_ctrl.c:77:32: sparse: sparse: cast to restricted __le32
> >    drivers/soc/litex/litex_soc_ctrl.c: note: in included file (through arch/openrisc/include/asm/io.h, include/linux/io.h, include/linux/litex.h):
> 
> I think this is something I need to fix up in the openrisc io.h header.

There are issues with the openrisc io.h header, but after closer inspection the
new warnings above do not seem to be related to this.  See more notes below.

> It seems the sparse annotations are messed up.  Working on it...
> 
> -Stafford
> 
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:225:22: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] value @@     got restricted __le32 [usertype] @@
> >    include/asm-generic/io.h:225:22: sparse:     expected unsigned int [usertype] value
> >    include/asm-generic/io.h:225:22: sparse:     got restricted __le32 [usertype]
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> >    include/asm-generic/io.h:179:15: sparse: sparse: cast to restricted __le32
> > 
> > vim +61 drivers/soc/litex/litex_soc_ctrl.c
> > 
> >     32	
> >     33	/*
> >     34	 * LiteX SoC Generator, depending on the configuration,
> >     35	 * can split a single logical CSR (Control & Status Register)
> >     36	 * into a series of consecutive physical registers.
> >     37	 *
> >     38	 * For example, in the configuration with 8-bit CSR Bus,
> >     39	 * 32-bit aligned (the default one for 32-bit CPUs) a 32-bit
> >     40	 * logical CSR will be generated as four 32-bit physical registers,
> >     41	 * each one containing one byte of meaningful data.
> >     42	 *
> >     43	 * For details see: https://github.com/enjoy-digital/litex/wiki/CSR-Bus
> >     44	 *
> >     45	 * The purpose of `litex_set_reg`/`litex_get_reg` is to implement
> >     46	 * the logic of writing to/reading from the LiteX CSR in a single
> >     47	 * place that can be then reused by all LiteX drivers.
> >     48	 */
> >     49	void litex_set_reg(void __iomem *reg, unsigned long reg_size,
> >     50			    unsigned long val)
> >     51	{
> >     52		unsigned long shifted_data, shift, i;
> >     53		unsigned long flags;
> >     54	
> >     55		spin_lock_irqsave(&csr_lock, flags);
> >     56	
> >     57		for (i = 0; i < reg_size; ++i) {
> >     58			shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> >     59			shifted_data = val >> shift;
> >     60	
> >   > 61			writel(cpu_to_le32(shifted_data), reg + (LITEX_REG_SIZE * i));

writel is defined to expect cpu endian u32 and it will internally convert it to
le32.  There shouldbe no need for cpu_to_le32.  This is from asm-generic/io.h
used by openrisc:

   #ifndef writel
   #define writel writel
   static inline void writel(u32 value, volatile void __iomem *addr)
   {
   	__io_bw();
   	__raw_writel(__cpu_to_le32(value), addr);
   	__io_aw();
   }
   #endif

The call to cpu_to_le32() is what is causing the sparse warnings above.

> >     62		}
> >     63	
> >     64		spin_unlock_irqrestore(&csr_lock, flags);
> >     65	}
> >     66	EXPORT_SYMBOL_GPL(litex_set_reg);
> >     67	
> >     68	unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_size)
> >     69	{
> >     70		unsigned long shifted_data, shift, i;
> >     71		unsigned long result = 0;
> >     72		unsigned long flags;
> >     73	
> >     74		spin_lock_irqsave(&csr_lock, flags);
> >     75	
> >     76		for (i = 0; i < reg_size; ++i) {
> >   > 77			shifted_data = le32_to_cpu(readl(reg + (LITEX_REG_SIZE * i)));

Same issue here, readl is already returning cpu endian u32.  No need to convert it
to cpu.

    #ifndef readl
    #define readl readl
    static inline u32 readl(const volatile void __iomem *addr)
    {
    	u32 val;

    	__io_br();
    	val = __le32_to_cpu((__le32 __force) __raw_readl(addr));
    	__io_ar(val);
    	return val;
    }
    #endif

Note, of LE machines like riscv the le32_to_cpu and le32_to_cpu will be no-ops
so this will not cause issues.  But likely this is broken on OpenRISC.  I have
not run these drivers for a while.

-Stafford

> >     78	
> >     79			shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> >     80			result |= (shifted_data << shift);
> >     81		}
> >     82	
> >     83		spin_unlock_irqrestore(&csr_lock, flags);
> >     84	
> >     85		return result;
> >     86	}
> >     87	EXPORT_SYMBOL_GPL(litex_get_reg);
> >     88	
> > 
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/kbuild-all@xxxxxxxxxxxx
> 
> 



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux