On 6/9/15 6:55 AM, Pavel Machek wrote: > Hi! > >> +struct sdram_prot_rule { >> + uint64_t sdram_start; /* SDRAM start address */ >> + uint64_t sdram_end; /* SDRAM end address */ >> + uint32_t rule; /* SDRAM protection rule number: 0-19 */ >> + int valid; /* Rule valid or not? 1 - valid, 0 not*/ > > There should be space before "*/". >
Ok... >> diff --git a/arch/arm/include/asm/arch-socfpga/sdram_config.h >> b/arch/arm/include/asm/arch-socfpga/sdram_config.h >> new file mode 100644 >> index 0000000..f6d51ca >> --- /dev/null >> +++ b/arch/arm/include/asm/arch-socfpga/sdram_config.h >> @@ -0,0 +1,100 @@ >> +/* >> + * Copyright Altera Corporation (C) 2012-2015 >> + * >> + * SPDX-License-Identifier: BSD-3-Clause >> + */ >> + >> +/* This file is autogenerated from tools provided by Altera.*/ > > Here too. > Ok... >> +#endif /*#ifndef__SDRAM_CONFIG_H*/ > > You should not need to comment for include guards... (and comment > style). > >> +static int compute_errata_rows(unsigned long long memsize, int cs, int >> width, >> + int rows, int banks, int cols) >> +{ > > Comment what kind of errata this is working around? > I'll have to ask around. > >> +#if defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_CSBITS) && \ >> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS) && \ >> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_BANKBITS) && \ >> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_COLBITS) && \ >> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS) >> + > > Hmm? Is this really neccessary? Is it valid to provide configuration > w/o those defines? > These defines are necessary as I want to keep some level of continuity with the Altera tools that generates these config files to this. >> + writel(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS, >> + &sysmgr_regs->iswgrp_handoff[4]); >> +#endif > >> + >> + /* Restore the SDR PHY Register if valid */ >> + if (sdr_phy_reg != 0xffffffff) >> + writel(sdr_phy_reg, &sdr_ctrl->phy_ctrl0); >> + >> +/***** Final step - apply configuration changes *****/ > > Comment style... > Ok.. >> +/* >> + * To calculate SDRAM device size based on SDRAM controller parameters. >> + * Size is specified in bytes. >> + * >> + * NOTE: >> + * This function is compiled and linked into the preloader and >> + * Uboot (there may be others). So if this function changes, the Preloader >> + * and UBoot must be updated simultaneously. >> + */ >> +unsigned long sdram_calculate_size(void) >> +{ >> + unsigned long temp; >> + unsigned long row, bank, col, cs, width; >> + >> + temp = readl(&sdr_ctrl->dram_addrw); >> + col = (temp & SDR_CTRLGRP_DRAMADDRW_COLBITS_MASK) >> >> + SDR_CTRLGRP_DRAMADDRW_COLBITS_LSB; >> + >> + /* SDRAM Failure When Accessing Non-Existent Memory >> + * Use ROWBITS from Quartus/QSys to calculate SDRAM size >> + * since the FB specifies we modify ROWBITs to work around SDRAM >> + * controller issue. >> + * >> + * If the stored handoff value for rows is 0, it probably means >> + * the preloader is older than UBoot. Use the >> + * #define from the SOCEDS Tools per Crucible review >> + * uboot-socfpga-204. Note that this is not a supported >> + * configuration and is not tested. The customer >> + * should be using preloader and uboot built from the >> + * same tag. >> + */ > > U-Boot is normally spelled "U-Boot". You have two different variants > in comments here. Thanks for the comment here, and will be more cognizant in the future on this fact. > > Second part of the comment is probably not relevant any more....? > removed... > Acked-by: Pavel Machek <pa...@denx.de> > Pavel Thanks, Dinh _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot