[mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[kernel test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
head:   63de24fccb6b831be1abfe07292449105b467731
commit: 7e7dc04774b18c0e42ce74aa3357021cda979674 [11/31] mtd: rawnand: cadence: support 64-bit slave dma interface
config: x86_64-randconfig-a001 (https://download.01.org/0day-ci/archive/20220921/202209210641.MziHAbW7-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/commit/?id=7e7dc04774b18c0e42ce74aa3357021cda979674
        git remote add mtd https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
        git fetch --no-tags mtd nand/next
        git checkout 7e7dc04774b18c0e42ce74aa3357021cda979674
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/mtd/nand/raw/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                           ioread64_rep(cdns_ctrl->io.virt, buf, len_in_words);
                           ^
>> drivers/mtd/nand/raw/cadence-nand-controller.c:1962:4: error: implicit declaration of function 'iowrite64_rep' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                           iowrite64_rep(cdns_ctrl->io.virt, buf, len_in_words);
                           ^
   2 errors generated.


vim +/ioread64_rep +1893 drivers/mtd/nand/raw/cadence-nand-controller.c

  1871	
  1872	static int cadence_nand_read_buf(struct cdns_nand_ctrl *cdns_ctrl,
  1873					 u8 *buf, int len)
  1874	{
  1875		u8 thread_nr = 0;
  1876		u32 sdma_size;
  1877		int status;
  1878	
  1879		/* Wait until slave DMA interface is ready to data transfer. */
  1880		status = cadence_nand_wait_on_sdma(cdns_ctrl, &thread_nr, &sdma_size);
  1881		if (status)
  1882			return status;
  1883	
  1884		if (!cdns_ctrl->caps1->has_dma) {
  1885			u8 data_dma_width = cdns_ctrl->caps2.data_dma_width;
  1886	
  1887			int len_in_words = (data_dma_width == 4) ? len >> 2 : len >> 3;
  1888	
  1889			/* read alingment data */
  1890			if (data_dma_width == 4)
  1891				ioread32_rep(cdns_ctrl->io.virt, buf, len_in_words);
  1892			else
> 1893				ioread64_rep(cdns_ctrl->io.virt, buf, len_in_words);
  1894	
  1895			if (sdma_size > len) {
  1896				int read_bytes = (data_dma_width == 4) ?
  1897					len_in_words << 2 : len_in_words << 3;
  1898	
  1899				/* read rest data from slave DMA interface if any */
  1900				if (data_dma_width == 4)
  1901					ioread32_rep(cdns_ctrl->io.virt,
  1902						     cdns_ctrl->buf,
  1903						     sdma_size / 4 - len_in_words);
  1904				else
  1905					ioread64_rep(cdns_ctrl->io.virt,
  1906						     cdns_ctrl->buf,
  1907						     sdma_size / 8 - len_in_words);
  1908	
  1909				/* copy rest of data */
  1910				memcpy(buf + read_bytes, cdns_ctrl->buf,
  1911				       len - read_bytes);
  1912			}
  1913			return 0;
  1914		}
  1915	
  1916		if (cadence_nand_dma_buf_ok(cdns_ctrl, buf, len)) {
  1917			status = cadence_nand_slave_dma_transfer(cdns_ctrl, buf,
  1918								 cdns_ctrl->io.dma,
  1919								 len, DMA_FROM_DEVICE);
  1920			if (status == 0)
  1921				return 0;
  1922	
  1923			dev_warn(cdns_ctrl->dev,
  1924				 "Slave DMA transfer failed. Try again using bounce buffer.");
  1925		}
  1926	
  1927		/* If DMA transfer is not possible or failed then use bounce buffer. */
  1928		status = cadence_nand_slave_dma_transfer(cdns_ctrl, cdns_ctrl->buf,
  1929							 cdns_ctrl->io.dma,
  1930							 sdma_size, DMA_FROM_DEVICE);
  1931	
  1932		if (status) {
  1933			dev_err(cdns_ctrl->dev, "Slave DMA transfer failed");
  1934			return status;
  1935		}
  1936	
  1937		memcpy(buf, cdns_ctrl->buf, len);
  1938	
  1939		return 0;
  1940	}
  1941	
  1942	static int cadence_nand_write_buf(struct cdns_nand_ctrl *cdns_ctrl,
  1943					  const u8 *buf, int len)
  1944	{
  1945		u8 thread_nr = 0;
  1946		u32 sdma_size;
  1947		int status;
  1948	
  1949		/* Wait until slave DMA interface is ready to data transfer. */
  1950		status = cadence_nand_wait_on_sdma(cdns_ctrl, &thread_nr, &sdma_size);
  1951		if (status)
  1952			return status;
  1953	
  1954		if (!cdns_ctrl->caps1->has_dma) {
  1955			u8 data_dma_width = cdns_ctrl->caps2.data_dma_width;
  1956	
  1957			int len_in_words = (data_dma_width == 4) ? len >> 2 : len >> 3;
  1958	
  1959			if (data_dma_width == 4)
  1960				iowrite32_rep(cdns_ctrl->io.virt, buf, len_in_words);
  1961			else
> 1962				iowrite64_rep(cdns_ctrl->io.virt, buf, len_in_words);
  1963	
  1964			if (sdma_size > len) {
  1965				int written_bytes = (data_dma_width == 4) ?
  1966					len_in_words << 2 : len_in_words << 3;
  1967	
  1968				/* copy rest of data */
  1969				memcpy(cdns_ctrl->buf, buf + written_bytes,
  1970				       len - written_bytes);
  1971	
  1972				/* write all expected by nand controller data */
  1973				if (data_dma_width == 4)
  1974					iowrite32_rep(cdns_ctrl->io.virt,
  1975						      cdns_ctrl->buf,
  1976						      sdma_size / 4 - len_in_words);
  1977				else
  1978					iowrite64_rep(cdns_ctrl->io.virt,
  1979						      cdns_ctrl->buf,
  1980						      sdma_size / 8 - len_in_words);
  1981			}
  1982	
  1983			return 0;
  1984		}
  1985	
  1986		if (cadence_nand_dma_buf_ok(cdns_ctrl, buf, len)) {
  1987			status = cadence_nand_slave_dma_transfer(cdns_ctrl, (void *)buf,
  1988								 cdns_ctrl->io.dma,
  1989								 len, DMA_TO_DEVICE);
  1990			if (status == 0)
  1991				return 0;
  1992	
  1993			dev_warn(cdns_ctrl->dev,
  1994				 "Slave DMA transfer failed. Try again using bounce buffer.");
  1995		}
  1996	
  1997		/* If DMA transfer is not possible or failed then use bounce buffer. */
  1998		memcpy(cdns_ctrl->buf, buf, len);
  1999	
  2000		status = cadence_nand_slave_dma_transfer(cdns_ctrl, cdns_ctrl->buf,
  2001							 cdns_ctrl->io.dma,
  2002							 sdma_size, DMA_TO_DEVICE);
  2003	
  2004		if (status)
  2005			dev_err(cdns_ctrl->dev, "Slave DMA transfer failed");
  2006	
  2007		return status;
  2008	}
  2009	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Miquel Raynal]

Hi Valentin,

lkp@intel.com wrote on Wed, 21 Sep 2022 06:31:23 +0800:

> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
> head:   63de24fccb6b831be1abfe07292449105b467731
> commit: 7e7dc04774b18c0e42ce74aa3357021cda979674 [11/31] mtd: rawnand: cadence: support 64-bit slave dma interface
> config: x86_64-randconfig-a001 (https://download.01.org/0day-ci/archive/20220921/202209210641.MziHAbW7-lkp@intel.com/config)
> compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/commit/?id=7e7dc04774b18c0e42ce74aa3357021cda979674
>         git remote add mtd https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
>         git fetch --no-tags mtd nand/next
>         git checkout 7e7dc04774b18c0e42ce74aa3357021cda979674
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/mtd/nand/raw/
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
> >> drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99 [-Werror,-Wimplicit-function-declaration]  
>                            ioread64_rep(cdns_ctrl->io.virt, buf, len_in_words);
>                            ^
> >> drivers/mtd/nand/raw/cadence-nand-controller.c:1962:4: error: implicit declaration of function 'iowrite64_rep' is invalid in C99 [-Werror,-Wimplicit-function-declaration]  
>                            iowrite64_rep(cdns_ctrl->io.virt, buf, len_in_words);
>                            ^
>    2 errors generated.

I've dropped your patch from nand/next, here you can check what's
missing in the configuration, should not be too hard to solve. Once
done, you can send a new version of the patch.

Thanks,
Miquèl

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Valentin Korenblit]

Hi Miquel,

I will take a look at it today.

Best regards,

Valentin

On 9/21/22 10:40, Miquel Raynal wrote:
> Hi Valentin,
>
> lkp@intel.com wrote on Wed, 21 Sep 2022 06:31:23 +0800:
>
>> tree:   https://secure-web.cisco.com/1Z7olKSJj_FWb335mmjHNiDM4FObh2m2Kr4CVmzRDX7-wZ9HzSUvxLN_OyF3NWS1vplF7quihKo97ocKqspSHS_xSUcBq7SoYyT4aiP0BVJtAGm55P1k9Mssa77KKPNUbAMyq4MzD4d9367IHSlU-8_Yma1xswyK72pByqojTDRaGeSLMcd3Efp9JB-7etBNOZ0Mq_7dqMyKuUwbZU8WIna-ZwHW_CCvS7DHXp86ZFUoRXPszt3szYeRa8qRCGcnf5H_LmZXUU46OcwE1MO85qwzYn-0jrSBK0Z0LnL63DxMcgpLBkIUBtTgPtqFi4eHLWI6u2BTdadLQdfQrkT7Ucrl1UbGhmfniZR7GvpQcpCslyPVbEPjQM94cygTmfnRfCQeEsIkB2SxIBR7D_XKq_8VDlmXBV1PuielMjqML7CbtTiWUZ8UQBcaHprmlKDYh/https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmtd%2Flinux.git nand/next
>> head:   63de24fccb6b831be1abfe07292449105b467731
>> commit: 7e7dc04774b18c0e42ce74aa3357021cda979674 [11/31] mtd: rawnand: cadence: support 64-bit slave dma interface
>> config: x86_64-randconfig-a001 (https://secure-web.cisco.com/10lsHqk23p4vjII4hdwyDVKjI5rVkWwyx1jI7f1QftDDRv-ChD7hGjvTtAOzKDcC5WNVJzgIOH_H5iX14jjNXRZmHnxAlNscjTbzSQ6XFgA__Gk-uZA21dWuP-k-xSOQmagW6OnnsvlbNvaj4z3B3-K10Mio7ZhaHmKpvXkuFnTkopDEFB_18jYwKBzqFH4HqK7OQs8z2ldAFAPyeS4pf2QB6gWtU7uw3_DM0Ffn22F7e6ErZ5TARWROXHEra3q8HTLL8YuuBtg-o98Lt_uo-p0lp_B50lGPRb1aotz73TGtcMPqacct_gweoopRth138iqcqhoKVUtwDmvi0Cq7MNEUW2I1SnlB7jl7mLNbp66sw2cCdqt_97tioK6I1NazsNUZ5gW4kuKFevcgnarV4Z8KOQtPXyyC6DReQJ9tBCH4yWC8IyYUQN0246z2s1lcb/https%3A%2F%2Fdownload.01.org%2F0day-ci%2Farchive%2F20220921%2F202209210641.MziHAbW7-lkp%40intel.com%2Fconfig)
>> compiler: clang version 14.0.6 (https://secure-web.cisco.com/1hFQ6b9I5rEbgJTBrSNYQ96NB2HBZl-vFn04fAzzx9TgVr-ZI2Yh78eJSHuVIXXUt7Y0FAQd5Hc5WbfhA3fBv9CZgiR1tSFSEk-TlQ8fHqEzUsO6pKVxVbq9NwFv3zIICt7M0Kjt_xyFZFD_pLrl-IlgkdddvWWGkrs6AUXwLKqfCRKrb4d3tGsfDNMN5PTj2SDjwnXBrFFC92LBxjNU1cK3VajqDxJ99oVgNDlhaJTlmsv48TcpOVugnYTf9qpQh2gxoQj6RqjoY-a9YlMYKtGjubYGgVyv0t9begalDgEeHU-YYOgLRNChdYZn5_DcojgYxjgSVr65RzBDt782Jnk94SSMcI4RV9yETmRwpS6hyiQKXxW2iigxlisod4NqU34ctO3xUECSsxQEnHjkodp72aMWWF2IEUpnnnZ9kQHHldB5_qQNZ0jGyM2N-Yrgm/https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
>> reproduce (this is a W=1 build):
>>          wget https://secure-web.cisco.com/1NixR8yM6cIQS7ncp2355n_85DjQ7PwQ8LJ4AtQRZ5vVAKsZUe2UDZW75tWk6nSotBZh4xXA4HlaBWc_XXiD5WNwEPEG6RIaFDZw7vpNSEO5UcrmnjqlejH8aJ6qBsNLGSi-9NxzBboH3tX2PFjnd751CP7XZNayy-jQeaodwkRS3qWaGdwnQI75u6_E2T4zhNUCwS5nxmh5GkTN7gk77lQoH89_EyptFbGvU1zk1cbCdMV_XS9aDfjH0SyloO_Je38ire_caQsnZbRi5lGRjczbhbhvlbgnsH5jS4q8kNgxMQcaYXxr57u-YhtX9Fqr3vJinftsYTt00wtexc4kdx2nUDuY1F1NNU_wT0OiH0MxuuzvAmazz1tjgwExi9D_KnHDu4cXnbfOyS_aEr3FPZIUvgJKbeJyvDpE6F1sRY4qrSO4dmuBRTQO3Ya4T-xTJ/https%3A%2F%2Fraw.githubusercontent.com%2Fintel%2Flkp-tests%2Fmaster%2Fsbin%2Fmake.cross -O ~/bin/make.cross
>>          chmod +x ~/bin/make.cross
>>          # https://secure-web.cisco.com/1GbV0WGD053fzgXwafNb9gIz6l_aVKdavZ9hBiGrxLQ4FuFGlfprHSkexNTIiRSf7WMRS2n0YEMNksbrqzajQre4UdkaGPrh-mUGZDWPWTD5X90ypVAk7Lt9t_5k2WBYPECGzW9U0HNMDgZqHuxMPZ1PnAguUNmw7TttND56Hvw6agQd0NzAboe1bcSg51IGYxLZXqDb1SSd__2WCkrAb5kNEcxMS8nKNrB55ZD111-UV3kZkzvEweSsciS0LOTa-OqYDUEeA0y94DhSOmjGqZpZXIU1ZA4mZ0_6hSkdmc1DF1sPrB2QiQ_E63CY9C-908GCmIGmrBZivyuRuxsuR0y9al38fKX8Zvmk9szy8afe36sGUpMoXijrPLUnPKmdV-2OCVj26sSaXKRtktrOsOI0b8esgEHuB1FxBHZjKxLCHONZecIdNO5B9mAUTu3UV/https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmtd%2Flinux.git%2Fcommit%2F%3Fid%3D7e7dc04774b18c0e42ce74aa3357021cda979674
>>          git remote add mtd https://secure-web.cisco.com/1Z7olKSJj_FWb335mmjHNiDM4FObh2m2Kr4CVmzRDX7-wZ9HzSUvxLN_OyF3NWS1vplF7quihKo97ocKqspSHS_xSUcBq7SoYyT4aiP0BVJtAGm55P1k9Mssa77KKPNUbAMyq4MzD4d9367IHSlU-8_Yma1xswyK72pByqojTDRaGeSLMcd3Efp9JB-7etBNOZ0Mq_7dqMyKuUwbZU8WIna-ZwHW_CCvS7DHXp86ZFUoRXPszt3szYeRa8qRCGcnf5H_LmZXUU46OcwE1MO85qwzYn-0jrSBK0Z0LnL63DxMcgpLBkIUBtTgPtqFi4eHLWI6u2BTdadLQdfQrkT7Ucrl1UbGhmfniZR7GvpQcpCslyPVbEPjQM94cygTmfnRfCQeEsIkB2SxIBR7D_XKq_8VDlmXBV1PuielMjqML7CbtTiWUZ8UQBcaHprmlKDYh/https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmtd%2Flinux.git
>>          git fetch --no-tags mtd nand/next
>>          git checkout 7e7dc04774b18c0e42ce74aa3357021cda979674
>>          # save the config file
>>          mkdir build_dir && cp config build_dir/.config
>>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/mtd/nand/raw/
>>
>> If you fix the issue, kindly add following tag where applicable
>> | Reported-by: kernel test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>):
>>
>>>> drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
>>                             ioread64_rep(cdns_ctrl->io.virt, buf, len_in_words);
>>                             ^
>>>> drivers/mtd/nand/raw/cadence-nand-controller.c:1962:4: error: implicit declaration of function 'iowrite64_rep' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
>>                             iowrite64_rep(cdns_ctrl->io.virt, buf, len_in_words);
>>                             ^
>>     2 errors generated.
> I've dropped your patch from nand/next, here you can check what's
> missing in the configuration, should not be too hard to solve. Once
> done, you can send a new version of the patch.
>
> Thanks,
> Miquèl
>
-- IMPORTANT NOTICE:



The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.



Thank you.

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Miquel Raynal]

Hi Valentin,

+ Arnd

The (erased) context of this thread:
https://lore.kernel.org/llvm/202209210641.MziHAbW7-lkp@intel.com/

vkorenblit@sequans.com wrote on Wed, 21 Sep 2022 14:27:47 +0200:

> Hi Miquel,
> 
> I see that x86_64 doesn't support readsq/writesq because of
> CONFIG_GENERIC_IOMAP (I was building for arm64). However,
> __raw_readq/writeq are available and there are a few drivers
> using them.

I would suggest to rather try using [read|write]sq() to get rid
of the CONFIG_GENERIC_IOMAP dependency. But it looks like those
functions are not defined on 32-bit architectures anyway. So if we want
the driver to compile on both arm and aarch64, it will not be enough.

In practice, I guess we should never have the 64-bit accessor executed
when running on a 32-bit platform thanks to the following conditions:

  1885			u8 data_dma_width = cdns_ctrl->caps2.data_dma_width;
  1886	
  1887			int len_in_words = (data_dma_width == 4) ? len >> 2 : len >> 3;
  1888	
  1889			/* read alingment data */
  1890			if (data_dma_width == 4)
  1891				ioread32_rep(cdns_ctrl->io.virt, buf, len_in_words);
  1892			else
> 1893				ioread64_rep(cdns_ctrl->io.virt, buf, len_in_words);

So maybe we could have something awful yet simple, like the following
within the Cadence driver:

#if !CONFIG_64BIT
readsq(...) { BUG()? }
#endif

Arnd, I've seen a couple of similar issues on the mailing lists in the
past 5 years but I could not find any working solution, I don't know
how all these threads have settled in the end. I thought maybe you
would have a better idea than the above hack :)

> Do you want me to re-implement readsq and writesq in Cadence
> driver privately or do you suggest a different (cleaner) approach?

I would rather prefer to avoid this solution, as anyway I believe there
is no "generic" implementation working in all cases, there were a
couple of attempts IIRC to bring generic helpers like the above for all
architectures, but none of them landed in Linus' tree, probably because
it just cannot be made...

Thanks,
Miquèl

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Arnd Bergmann]

On Wed, Sep 21, 2022, at 4:47 PM, Miquel Raynal wrote:
>
> The (erased) context of this thread:
> https://lore.kernel.org/llvm/202209210641.MziHAbW7-lkp@intel.com/
>
> vkorenblit@sequans.com wrote on Wed, 21 Sep 2022 14:27:47 +0200:
>
>> Hi Miquel,
>> 
>> I see that x86_64 doesn't support readsq/writesq because of
>> CONFIG_GENERIC_IOMAP (I was building for arm64). However,
>> __raw_readq/writeq are available and there are a few drivers
>> using them.
>
> I would suggest to rather try using [read|write]sq() to get rid
> of the CONFIG_GENERIC_IOMAP dependency. But it looks like those
> functions are not defined on 32-bit architectures anyway. So if we want
> the driver to compile on both arm and aarch64, it will not be enough.
>
> In practice, I guess we should never have the 64-bit accessor executed
> when running on a 32-bit platform thanks to the following conditions:
>
>   1885			u8 data_dma_width = cdns_ctrl->caps2.data_dma_width;
>   1886	
>   1887			int len_in_words = (data_dma_width == 4) ? len >> 2 : len >> 3;
>   1888	
>   1889			/* read alingment data */
>   1890			if (data_dma_width == 4)
>   1891				ioread32_rep(cdns_ctrl->io.virt, buf, len_in_words);
>   1892			else
>> 1893				ioread64_rep(cdns_ctrl->io.virt, buf, len_in_words);
>
> So maybe we could have something awful yet simple, like the following
> within the Cadence driver:
>
> #if !CONFIG_64BIT
> readsq(...) { BUG()? }
> #endif
>
> Arnd, I've seen a couple of similar issues on the mailing lists in the
> past 5 years but I could not find any working solution, I don't know
> how all these threads have settled in the end. I thought maybe you
> would have a better idea than the above hack :)

There are a lot of different things going on here, so I need
to unwind a bit:

- every architecture should provide readsq()/readsl()/readsw()/readsb()
  these days, regardless of CONFIG_GENERIC_IOMAP. If x86 does
  not have that, we should fix asm-generic/io.h.

- CONFIG_GENERIC_IOMAP just means an architecture uses the generic
  ioread32_rep() style wrapper around readsl()/insl(). On most
  architectures (not x86), insl() is implemented as a wrapper around
  readsl() itself, so readsl() and ioread32_rep() should be identical.

- For a FIFO, you cannot use readq() but have to use __raw_readq()
  to get the correct endianness. You cannot use this for an
  MMIO register with side-effects though, as this needs the byteswap
  and the barrier in readsl().

- On 32-bit architectures, there is no generic way to access a 64-bit
  MMIO location as you say. However, depending on the device you are
  accessing, you can replace __raw_readq() with a pair of
  __raw_readl(), along the lines of
  include/linux/io-64-nonatomic-{lo-hi,hi-lo}.h
 
>> Do you want me to re-implement readsq and writesq in Cadence
>> driver privately or do you suggest a different (cleaner) approach?
>
> I would rather prefer to avoid this solution, as anyway I believe there
> is no "generic" implementation working in all cases, there were a
> couple of attempts IIRC to bring generic helpers like the above for all
> architectures, but none of them landed in Linus' tree, probably because
> it just cannot be made...

If anyone has a datasheet for the cadence driver, they should be
able to look up how one can access the FIFO in 8-byte mode using
32-bit accesses. I think it's something like

static inline void cadence_nand_readsq(const volatile void __iomem *addr,
               void *buffer, unsigned int count)
{
        if (count) {
                u64 *buf = buffer;

                do {
#ifdef __raw_readq
                        u64 x = __raw_readq(addr);
                        *buf++ = x;
#else
                        u32 *buf32 = (void *)buf;
                        buf32[0] = __raw_readl(addr + OFF0);
                        buf32[1] = __raw_readl(addr + OFF1);
                        buf++;
#endif
                } while (--count);
        }
}

Most likely, OFF0 is zero, while OFF1 is 4 here, but that
depends on how the FIFO register is implemented.

     Arnd

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Miquel Raynal]

Hi Arnd,

arnd@arndb.de wrote on Wed, 21 Sep 2022 17:49:11 +0200:

> On Wed, Sep 21, 2022, at 4:47 PM, Miquel Raynal wrote:
> >
> > The (erased) context of this thread:
> > https://lore.kernel.org/llvm/202209210641.MziHAbW7-lkp@intel.com/
> >
> > vkorenblit@sequans.com wrote on Wed, 21 Sep 2022 14:27:47 +0200:
> >  
> >> Hi Miquel,
> >> 
> >> I see that x86_64 doesn't support readsq/writesq because of
> >> CONFIG_GENERIC_IOMAP (I was building for arm64). However,
> >> __raw_readq/writeq are available and there are a few drivers
> >> using them.  
> >
> > I would suggest to rather try using [read|write]sq() to get rid
> > of the CONFIG_GENERIC_IOMAP dependency. But it looks like those
> > functions are not defined on 32-bit architectures anyway. So if we want
> > the driver to compile on both arm and aarch64, it will not be enough.
> >
> > In practice, I guess we should never have the 64-bit accessor executed
> > when running on a 32-bit platform thanks to the following conditions:
> >
> >   1885			u8 data_dma_width = cdns_ctrl->caps2.data_dma_width;
> >   1886	
> >   1887			int len_in_words = (data_dma_width == 4) ? len >> 2 : len >> 3;
> >   1888	
> >   1889			/* read alingment data */
> >   1890			if (data_dma_width == 4)
> >   1891				ioread32_rep(cdns_ctrl->io.virt, buf, len_in_words);
> >   1892			else  
> >> 1893				ioread64_rep(cdns_ctrl->io.virt, buf, len_in_words);  
> >
> > So maybe we could have something awful yet simple, like the following
> > within the Cadence driver:
> >
> > #if !CONFIG_64BIT
> > readsq(...) { BUG()? }
> > #endif
> >
> > Arnd, I've seen a couple of similar issues on the mailing lists in the
> > past 5 years but I could not find any working solution, I don't know
> > how all these threads have settled in the end. I thought maybe you
> > would have a better idea than the above hack :)  
> 
> There are a lot of different things going on here, so I need
> to unwind a bit:

Thanks for all your feedback!

> - every architecture should provide readsq()/readsl()/readsw()/readsb()
>   these days, regardless of CONFIG_GENERIC_IOMAP. If x86 does
>   not have that, we should fix asm-generic/io.h.

ARM does not seem to define readsq/writesq. Should it be fixed?

> - CONFIG_GENERIC_IOMAP just means an architecture uses the generic
>   ioread32_rep() style wrapper around readsl()/insl(). On most
>   architectures (not x86), insl() is implemented as a wrapper around
>   readsl() itself, so readsl() and ioread32_rep() should be identical.

Ok. But if CONFIG_GENERIC_IOMAP=n (ARM, aarch64, x86_64),
ioread64_rep is then only defined if CONFIG_64BIT. As it is based
on readsq/writesq() and those must be defined (as you said), I don't get
why the *64_rep() helpers are not defined in all cases. Maybe because no
32-bit system _should_ need them? But then compile testing gets more
difficult.

> - For a FIFO, you cannot use readq() but have to use __raw_readq()
>   to get the correct endianness. You cannot use this for an
>   MMIO register with side-effects though, as this needs the byteswap
>   and the barrier in readsl().

I'm not sure about the true definition of "FIFO" as you say. I guess
you just mean reading from a BE device?

In this case I guess we need the barrier+byteswap helpers.

> - On 32-bit architectures, there is no generic way to access a 64-bit
>   MMIO location as you say. However, depending on the device you are
>   accessing, you can replace __raw_readq() with a pair of
>   __raw_readl(), along the lines of
>   include/linux/io-64-nonatomic-{lo-hi,hi-lo}.h

Ah ok, so in the end the per-driver implementation is preferred.

> >> Do you want me to re-implement readsq and writesq in Cadence
> >> driver privately or do you suggest a different (cleaner) approach?  
> >
> > I would rather prefer to avoid this solution, as anyway I believe there
> > is no "generic" implementation working in all cases, there were a
> > couple of attempts IIRC to bring generic helpers like the above for all
> > architectures, but none of them landed in Linus' tree, probably because
> > it just cannot be made...  
> 
> If anyone has a datasheet for the cadence driver, they should be
> able to look up how one can access the FIFO in 8-byte mode using
> 32-bit accesses. I think it's something like

I don't think this is actually what we want. My understanding is
(Valentin, please correct me if I'm wrong):
- on ARM: we will always use 32-bit accesses
- on aarch64: we may use either 32-bit or 64-bit accesses
- on other architectures: we only want to compile test

I believe what Valentin wanted to achieve in the first place, was to
use 64-bit accesses when relevant (otherwise it does not work).

> static inline void cadence_nand_readsq(const volatile void __iomem *addr,
>                void *buffer, unsigned int count)
> {
>         if (count) {
>                 u64 *buf = buffer;
> 
>                 do {
> #ifdef __raw_readq
>                         u64 x = __raw_readq(addr);
>                         *buf++ = x;
> #else
>                         u32 *buf32 = (void *)buf;
>                         buf32[0] = __raw_readl(addr + OFF0);
>                         buf32[1] = __raw_readl(addr + OFF1);
>                         buf++;
> #endif
>                 } while (--count);
>         }
> }
> 
> Most likely, OFF0 is zero, while OFF1 is 4 here, but that
> depends on how the FIFO register is implemented.
> 
>      Arnd


Thanks,
Miquèl

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Arnd Bergmann]

On Wed, Sep 21, 2022, at 6:38 PM, Miquel Raynal wrote:
> arnd@arndb.de wrote on Wed, 21 Sep 2022 17:49:11 +0200:
>> On Wed, Sep 21, 2022, at 4:47 PM, Miquel Raynal wrote:
>
>> - every architecture should provide readsq()/readsl()/readsw()/readsb()
>>   these days, regardless of CONFIG_GENERIC_IOMAP. If x86 does
>>   not have that, we should fix asm-generic/io.h.
>
> ARM does not seem to define readsq/writesq. Should it be fixed?

64-bit Arm should get it from include/asm-generic/io.h. If it does
not, this should be fixed. 32-bit Arm obviously cannot define them
in a generic way.

>> - CONFIG_GENERIC_IOMAP just means an architecture uses the generic
>>   ioread32_rep() style wrapper around readsl()/insl(). On most
>>   architectures (not x86), insl() is implemented as a wrapper around
>>   readsl() itself, so readsl() and ioread32_rep() should be identical.
>
> Ok. But if CONFIG_GENERIC_IOMAP=n (ARM, aarch64, x86_64),

x86_64 has GENERIC_IOMAP=y

> ioread64_rep is then only defined if CONFIG_64BIT. As it is based
> on readsq/writesq() and those must be defined (as you said), I don't get
> why the *64_rep() helpers are not defined in all cases. Maybe because no
> 32-bit system _should_ need them? But then compile testing gets more
> difficult.

Both readsq/writesq and ioread64_rep/iowrite64_rep must be defined
for 64-bit architectures and cannot be defined for 32-bit ones.

>> - For a FIFO, you cannot use readq() but have to use __raw_readq()
>>   to get the correct endianness. You cannot use this for an
>>   MMIO register with side-effects though, as this needs the byteswap
>>   and the barrier in readsl().
>
> I'm not sure about the true definition of "FIFO" as you say. I guess
> you just mean reading from a BE device?
>
> In this case I guess we need the barrier+byteswap helpers.

The difference is that a register has a fixed length, and gets
accessed with a device specific endianness, which may have to
be swapped if the device and the CPU disagree.

A FIFO register is what you use for transferring a stream of
bytes, such as reading a file system block from disk. The
first byte in the register corresponds to the first byte in
memory later, so there must not be any byteswap while copying
to/from memory. If the data itself is structured (i.e. an
on-disk inode or a network packet), then the byteswap will
happen if necessary while interpreting the data.

> I don't think this is actually what we want. My understanding is
> (Valentin, please correct me if I'm wrong):
> - on ARM: we will always use 32-bit accesses
> - on aarch64: we may use either 32-bit or 64-bit accesses
> - on other architectures: we only want to compile test
>
> I believe what Valentin wanted to achieve in the first place, was to
> use 64-bit accesses when relevant (otherwise it does not work).

The width is read from a device specific register at
runtime, it is not related to the architecture you are
running on, presumably this is hardwired during the
design of an SoC, based on the capabilities of the DMA
engine:

        reg = readl_relaxed(cdns_ctrl->reg + CTRL_FEATURES);
        if (FIELD_GET(CTRL_FEATURES_DMA_DWITH64, reg))
                cdns_ctrl->caps2.data_dma_width = 8;
        else
                cdns_ctrl->caps2.data_dma_width = 4;

This usually means the largest access that is valid for
reading from the FIFO, but usually smaller accesses work
as well, just slower.

     Arnd

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Miquel Raynal]

Hi Valentin,

vkorenblit@sequans.com wrote on Thu, 22 Sep 2022 10:18:46 +0200:

> Hi Arnd, Miquel,
> 
> On 9/21/22 22:01, Arnd Bergmann wrote:
> > On Wed, Sep 21, 2022, at 6:38 PM, Miquel Raynal wrote:  
> >> arnd@arndb.de  wrote on Wed, 21 Sep 2022 17:49:11 +0200:  
> >>> On Wed, Sep 21, 2022, at 4:47 PM, Miquel Raynal wrote:
> >>> - every architecture should provide readsq()/readsl()/readsw()/readsb()
> >>>    these days, regardless of CONFIG_GENERIC_IOMAP. If x86 does
> >>>    not have that, we should fix asm-generic/io.h.  
> >> ARM does not seem to define readsq/writesq. Should it be fixed?  
> > 64-bit Arm should get it from include/asm-generic/io.h. If it does
> > not, this should be fixed. 32-bit Arm obviously cannot define them
> > in a generic way.  
> 
> This is ok for 64-bit arm.
> 
> >  
> >>> - CONFIG_GENERIC_IOMAP just means an architecture uses the generic
> >>>    ioread32_rep() style wrapper around readsl()/insl(). On most
> >>>    architectures (not x86), insl() is implemented as a wrapper around
> >>>    readsl() itself, so readsl() and ioread32_rep() should be identical.  
> >> Ok. But if CONFIG_GENERIC_IOMAP=n (ARM, aarch64, x86_64),  
> > x86_64 has GENERIC_IOMAP=y
> >  
> >> ioread64_rep is then only defined if CONFIG_64BIT. As it is based
> >> on readsq/writesq() and those must be defined (as you said), I don't get
> >> why the *64_rep() helpers are not defined in all cases. Maybe because no
> >> 32-bit system _should_ need them? But then compile testing gets more
> >> difficult.  
> > Both readsq/writesq and ioread64_rep/iowrite64_rep must be defined
> > for 64-bit architectures and cannot be defined for 32-bit ones.  
> 
> So the first issue here is that they are not defined for x86_64
> (CONFIG_64BIT=y).

I would say this is not very important as long as we could use
readsq/writesq() for the same purpose.

> >>> - For a FIFO, you cannot use readq() but have to use __raw_readq()
> >>>    to get the correct endianness. You cannot use this for an
> >>>    MMIO register with side-effects though, as this needs the byteswap
> >>>    and the barrier in readsl().  
> >> I'm not sure about the true definition of "FIFO" as you say. I guess
> >> you just mean reading from a BE device?
> >>
> >> In this case I guess we need the barrier+byteswap helpers.  
> > The difference is that a register has a fixed length, and gets
> > accessed with a device specific endianness, which may have to
> > be swapped if the device and the CPU disagree.
> >
> > A FIFO register is what you use for transferring a stream of
> > bytes, such as reading a file system block from disk. The
> > first byte in the register corresponds to the first byte in
> > memory later, so there must not be any byteswap while copying
> > to/from memory. If the data itself is structured (i.e. an
> > on-disk inode or a network packet), then the byteswap will
> > happen if necessary while interpreting the data.
> >  
> >> I don't think this is actually what we want. My understanding is
> >> (Valentin, please correct me if I'm wrong):
> >> - on ARM: we will always use 32-bit accesses
> >> - on aarch64: we may use either 32-bit or 64-bit accesses
> >> - on other architectures: we only want to compile test  
> 
> Correct, this was my initial idea. However, this driver should work
> with every architecture or do we limit the scope to arm/arm64/x86_64?

The driver should work on ARM and aarch64, I'm not aware of other
architectures with this IP.

The driver should compile when COMPILE_TEST=y.

> >> I believe what Valentin wanted to achieve in the first place, was to
> >> use 64-bit accesses when relevant (otherwise it does not work).  
> > The width is read from a device specific register at
> > runtime, it is not related to the architecture you are
> > running on, presumably this is hardwired during the
> > design of an SoC, based on the capabilities of the DMA
> > engine:

Well, yes, but in the mean time 64-bit DMA width will never be
used on 32-bit platforms.

> >          reg = readl_relaxed(cdns_ctrl->reg + CTRL_FEATURES);
> >          if (FIELD_GET(CTRL_FEATURES_DMA_DWITH64, reg))
> >                  cdns_ctrl->caps2.data_dma_width = 8;
> >          else
> >                  cdns_ctrl->caps2.data_dma_width = 4;
> >
> > This usually means the largest access that is valid for
> > reading from the FIFO, but usually smaller accesses work
> > as well, just slower.  

Mmh, ok, that's interesting, thanks for the pointer.

But in the mean time I am only half satisfied, because we plan to do
twice more accesses than needed _just_ because of a the COMPILE_TEST
constraint.

> Thanks for all the info. I can check if consecutive smaller accesses
> trigger sdma_err interrupt. The datasheet only specifies: "if host sends an
> unsupported transaction to slave interface, the slave dma ignores
> this access and sets sdma_err flag in intr_status register.
> 
> Valentin

Thanks,
Miquèl

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Miquel Raynal]

Hi Arnd,

> > ioread64_rep is then only defined if CONFIG_64BIT. As it is based
> > on readsq/writesq() and those must be defined (as you said), I don't get
> > why the *64_rep() helpers are not defined in all cases. Maybe because no
> > 32-bit system _should_ need them? But then compile testing gets more
> > difficult.  
> 
> Both readsq/writesq and ioread64_rep/iowrite64_rep must be defined
> for 64-bit architectures and cannot be defined for 32-bit ones.

Yeah, ok.

> >> - For a FIFO, you cannot use readq() but have to use __raw_readq()
> >>   to get the correct endianness. You cannot use this for an
> >>   MMIO register with side-effects though, as this needs the byteswap
> >>   and the barrier in readsl().  
> >
> > I'm not sure about the true definition of "FIFO" as you say. I guess
> > you just mean reading from a BE device?
> >
> > In this case I guess we need the barrier+byteswap helpers.  
> 
> The difference is that a register has a fixed length, and gets
> accessed with a device specific endianness, which may have to
> be swapped if the device and the CPU disagree.
> 
> A FIFO register is what you use for transferring a stream of
> bytes, such as reading a file system block from disk. The
> first byte in the register corresponds to the first byte in
> memory later, so there must not be any byteswap while copying
> to/from memory. If the data itself is structured (i.e. an
> on-disk inode or a network packet), then the byteswap will
> happen if necessary while interpreting the data.

Ok, I fully get what you mean, I was just not used to the word FIFO for
this definition as I use it as a more generic term, but it completely
makes sense. Thanks for taking the time to explain.

Thanks,
Miquèl

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Arnd Bergmann]

On Thu, Sep 22, 2022, at 11:36 AM, Miquel Raynal wrote:
> vkorenblit@sequans.com wrote on Thu, 22 Sep 2022 10:18:46 +0200:
>> 
>> Correct, this was my initial idea. However, this driver should work
>> with every architecture or do we limit the scope to arm/arm64/x86_64?
>
> The driver should work on ARM and aarch64, I'm not aware of other
> architectures with this IP.
>
> The driver should compile when COMPILE_TEST=y.

It should also be written in a way that makes it plausible to
use elsewhere. Since this is just a licensed IP core, there is
a good chance that someone reused it on mips or riscv, or
anything else.

>> >> I believe what Valentin wanted to achieve in the first place, was to
>> >> use 64-bit accesses when relevant (otherwise it does not work).  
>> > The width is read from a device specific register at
>> > runtime, it is not related to the architecture you are
>> > running on, presumably this is hardwired during the
>> > design of an SoC, based on the capabilities of the DMA
>> > engine:
>
> Well, yes, but in the mean time 64-bit DMA width will never be
> used on 32-bit platforms.

Why? Most architectures (including x86 and arm) allow you to
run a 32-bit kernel on a 64-bit SoC. While this is almost always
a bad idea to actually do, a driver should be written to
work correctly in this setup.

>> > This usually means the largest access that is valid for
>> > reading from the FIFO, but usually smaller accesses work
>> > as well, just slower.  
>
> Mmh, ok, that's interesting, thanks for the pointer.
>
> But in the mean time I am only half satisfied, because we plan to do
> twice more accesses than needed _just_ because of a the COMPILE_TEST
> constraint.

In my example, I had an #ifdef so it would only fall back
to 32-bit accesses on the 64-bit register when running an
actual 32-bit kernel, but leaving the 64-bit case efficient.

    Arnd

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Miquel Raynal]

Hi Arnd,

arnd@arndb.de wrote on Thu, 22 Sep 2022 12:52:36 +0200:

> On Thu, Sep 22, 2022, at 11:36 AM, Miquel Raynal wrote:
> > vkorenblit@sequans.com wrote on Thu, 22 Sep 2022 10:18:46 +0200:  
> >> 
> >> Correct, this was my initial idea. However, this driver should work
> >> with every architecture or do we limit the scope to arm/arm64/x86_64?  
> >
> > The driver should work on ARM and aarch64, I'm not aware of other
> > architectures with this IP.
> >
> > The driver should compile when COMPILE_TEST=y.  
> 
> It should also be written in a way that makes it plausible to
> use elsewhere. Since this is just a licensed IP core, there is
> a good chance that someone reused it on mips or riscv, or
> anything else.

Fair enough.

> >> >> I believe what Valentin wanted to achieve in the first place, was to
> >> >> use 64-bit accesses when relevant (otherwise it does not work).    
> >> > The width is read from a device specific register at
> >> > runtime, it is not related to the architecture you are
> >> > running on, presumably this is hardwired during the
> >> > design of an SoC, based on the capabilities of the DMA
> >> > engine:  
> >
> > Well, yes, but in the mean time 64-bit DMA width will never be
> > used on 32-bit platforms.  
> 
> Why? Most architectures (including x86 and arm) allow you to
> run a 32-bit kernel on a 64-bit SoC. While this is almost always
> a bad idea to actually do, a driver should be written to
> work correctly in this setup.

Oh right, I forgot about that.

> >> > This usually means the largest access that is valid for
> >> > reading from the FIFO, but usually smaller accesses work
> >> > as well, just slower.    
> >
> > Mmh, ok, that's interesting, thanks for the pointer.
> >
> > But in the mean time I am only half satisfied, because we plan to do
> > twice more accesses than needed _just_ because of a the COMPILE_TEST
> > constraint.  
> 
> In my example, I had an #ifdef so it would only fall back
> to 32-bit accesses on the 64-bit register when running an
> actual 32-bit kernel, but leaving the 64-bit case efficient.

All right, thanks for all your valuable feedback Arnd!

Cheers,
Miquèl

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Arnd Bergmann]

On Tue, Sep 27, 2022, at 4:56 PM, Valentin Korenblit wrote:
>>> 
>>> But in the mean time I am only half satisfied, because we plan to do
>>> twice more accesses than needed _just_ because of a the COMPILE_TEST
>>> constraint.
>> In my example, I had an #ifdef so it would only fall back
>> to 32-bit accesses on the 64-bit register when running an
>> actual 32-bit kernel, but leaving the 64-bit case efficient. Sorry for my late reply. I've just tested this and unfortunately
> the two sequential 32-bit accesses (with OFF0==0 and OFF1==4 seem
> to trigger sdma_err. I need to check some waveforms to verify if it 
> happens right after the first access. 

What happens if you do pairs of read from offset 0, effectively
doing a readsl() instead of readql(), obviously with twice the number
of accesses.

It's also possible you have to read from the second word first,
like

  u32 *buf;
  do {
     buf[1] = __raw_readl(reg + 4);
     buf[0] = __raw_readl(reg);
     buf += 2;
  }  while (buf < end);

   Arnd

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Valentin Korenblit]

On 9/27/22 22:02, Arnd Bergmann wrote:
> On Tue, Sep 27, 2022, at 4:56 PM, Valentin Korenblit wrote:
>>>> But in the mean time I am only half satisfied, because we plan to do
>>>> twice more accesses than needed _just_ because of a the COMPILE_TEST
>>>> constraint.
>>> In my example, I had an #ifdef so it would only fall back
>>> to 32-bit accesses on the 64-bit register when running an
>>> actual 32-bit kernel, but leaving the 64-bit case efficient. Sorry for my late reply. I've just tested this and unfortunately
>> the two sequential 32-bit accesses (with OFF0==0 and OFF1==4 seem
>> to trigger sdma_err. I need to check some waveforms to verify if it
>> happens right after the first access.
> What happens if you do pairs of read from offset 0, effectively
> doing a readsl() instead of readql(), obviously with twice the number
> of accesses.
>
> It's also possible you have to read from the second word first,
> like
>
>    u32 *buf;
>    do {
>       buf[1] = __raw_readl(reg + 4);
>       buf[0] = __raw_readl(reg);
>       buf += 2;
>    }  while (buf < end);
>
>     Arnd

Same result with pairs of readl at OFF0 and when reading at OFF1 first too,
I still see sdma_err. I've just opened a case to Cadence to see if there
is any workaround for this or if it is just not possible.

I'll be back as soon as I have some news.

Valentin

-- IMPORTANT NOTICE:



The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.



Thank you.

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Arnd Bergmann]

On Wed, Sep 28, 2022, at 10:41 AM, Valentin Korenblit wrote:
> On 9/27/22 22:02, Arnd Bergmann wrote:
>> On Tue, Sep 27, 2022, at 4:56 PM, Valentin Korenblit wrote:
>>>>> But in the mean time I am only half satisfied, because we plan to do
>>>>> twice more accesses than needed _just_ because of a the COMPILE_TEST
>>>>> constraint.
>>
>> It's also possible you have to read from the second word first,
>> like
>>
>>    u32 *buf;
>>    do {
>>       buf[1] = __raw_readl(reg + 4);
>>       buf[0] = __raw_readl(reg);
>>       buf += 2;
>>    }  while (buf < end);

>
> Same result with pairs of readl at OFF0 and when reading at OFF1 first too,
> I still see sdma_err. I've just opened a case to Cadence to see if there
> is any workaround for this or if it is just not possible.

I think this just means that the access has to be done with the exact
width that is configured, and you cannot implement the access on
32-bit architectures. The only possibility is that you can reconfigure
the nand controller to 32-bit mode at runtime, which is what Cadence
should be able to tell you.

    Arnd

Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Valentin Korenblit]

On 9/28/22 10:56, Arnd Bergmann wrote:
> On Wed, Sep 28, 2022, at 10:41 AM, Valentin Korenblit wrote:
>> On 9/27/22 22:02, Arnd Bergmann wrote:
>>> On Tue, Sep 27, 2022, at 4:56 PM, Valentin Korenblit wrote:
>>>>>> But in the mean time I am only half satisfied, because we plan to do
>>>>>> twice more accesses than needed _just_ because of a the COMPILE_TEST
>>>>>> constraint.
>>> It's also possible you have to read from the second word first,
>>> like
>>>
>>>     u32 *buf;
>>>     do {
>>>        buf[1] = __raw_readl(reg + 4);
>>>        buf[0] = __raw_readl(reg);
>>>        buf += 2;
>>>     }  while (buf < end);
>> Same result with pairs of readl at OFF0 and when reading at OFF1 first too,
>> I still see sdma_err. I've just opened a case to Cadence to see if there
>> is any workaround for this or if it is just not possible.
> I think this just means that the access has to be done with the exact
> width that is configured, and you cannot implement the access on
> 32-bit architectures. The only possibility is that you can reconfigure
> the nand controller to 32-bit mode at runtime, which is what Cadence
> should be able to tell you.
>
>      Arnd

I've just confirmed that we cannot change sdma width in runtime and
that sdma expects to be addressed in burst width, which is the bus
width (64-bit).

Valentin

-- IMPORTANT NOTICE:



The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.



Thank you.