On 16 February 2017 at 09:09, Benjamin Herrenschmidt
<benh@xxxxxxxxxxxxxxxxxxx> wrote:
> From: "Y.C. Chen" <yc_chen@xxxxxxxxxxxxxx>
>
> Add detection and POST code for AST2500 generation chip,
> code originally from Aspeed and slightly reworked for
> coding style mostly by Ben.
>
> Signed-off-by: Y.C. Chen <yc_chen@xxxxxxxxxxxxxx>
> Signed-off-by: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> ---
> v2. [BenH]
> - Coding style fixes
> - Add timeout to main DRAM init loop
> - Improve boot time display of RAM info
>
> Y.C. Please check that I didn't break POST as I haven't manage to test
> that (we can't boot our machines if the BMC isn't already POSTed).
>
Hi Ben,
Barring the checkpatch.pl warnings/errors that you've spotted there's
a few other comments below.
I'm not familiar with the hardware, so it's just things that strike me
as odd ;-)
> +/*
> + * AST2500 DRAM settings modules
> + */
> +#define REGTBL_NUM 17
> +#define REGIDX_010 0
> +#define REGIDX_014 1
> +#define REGIDX_018 2
> +#define REGIDX_020 3
> +#define REGIDX_024 4
> +#define REGIDX_02C 5
> +#define REGIDX_030 6
> +#define REGIDX_214 7
> +#define REGIDX_2E0 8
> +#define REGIDX_2E4 9
> +#define REGIDX_2E8 10
> +#define REGIDX_2EC 11
> +#define REGIDX_2F0 12
> +#define REGIDX_2F4 13
> +#define REGIDX_2F8 14
> +#define REGIDX_RFC 15
> +#define REGIDX_PLL 16
These are used to address the correct value in the array, Worth using
C99 initailizer to ensure that things end in the right place ?
IIRC there was some security related GCC plugin which can fuzz the
order of array(?) and/or struct members ?
> +
> +static u32 ast2500_ddr3_1600_timing_table[REGTBL_NUM] = {
> +static u32 ast2500_ddr4_1600_timing_table[REGTBL_NUM] = {
These two are constant data, although you'll need to annotate the users as well.
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -32,8 +32,6 @@
> #include <drm/drm_fb_helper.h>
> #include <drm/drm_crtc_helper.h>
>
> -#include "ast_dram_tables.h"
> -
Unrelated change ?
> - DRM_INFO("dram %d %d %d %08x\n", ast->mclk, ast->dram_type, ast->dram_bus_width, ast->vram_size);
> + DRM_INFO("dram MCLK=%u type=%d bus_width=%d size=%08x\n",
> + ast->mclk, ast->dram_type, ast->dram_bus_width, ast->vram_size);
Unrelated change ?
> static void ast_set_ext_reg(struct drm_crtc *crtc, struct drm_display_mode *mode,
> @@ -421,7 +432,7 @@ static void ast_set_ext_reg(struct drm_crtc *crtc, struct drm_display_mode *mode
> ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa8, 0xfd, jregA8);
>
> /* Set Threshold */
> - if (ast->chip == AST2300 || ast->chip == AST2400) {
> + if (ast->chip == AST2300 || ast->chip == AST2400 || ast->chip == AST2500) {
> ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0x78);
> ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0x60);
> } else if (ast->chip == AST2100 ||
> @@ -794,7 +805,7 @@ static int ast_mode_valid(struct drm_connector *connector,
> if ((mode->hdisplay == 1600) && (mode->vdisplay == 900))
> return MODE_OK;
>
> - if ((ast->chip == AST2100) || (ast->chip == AST2200) || (ast->chip == AST2300) || (ast->chip == AST2400) || (ast->chip == AST1180)) {
> + if ((ast->chip == AST2100) || (ast->chip == AST2200) || (ast->chip == AST2300) || (ast->chip == AST2400) || (ast->chip == AST2500) || (ast->chip == AST1180)) {
178 columns is getting a bit too much.
> -static int mmc_test_burst(struct ast_private *ast, u32 datagen)
> +static int mmc_test(struct ast_private *ast, u32 datagen, u8 test_ctl)
> {
> u32 data, timeout;
>
> ast_moutdwm(ast, 0x1e6e0070, 0x00000000);
> - ast_moutdwm(ast, 0x1e6e0070, 0x000000c1 | (datagen << 3));
> + ast_moutdwm(ast, 0x1e6e0070, (datagen << 3) | test_ctl);
> timeout = 0;
> do {
> data = ast_mindwm(ast, 0x1e6e0070) & 0x3000;
> if (data & 0x2000) {
> - return 0;
> + return -1;
> }
> if (++timeout > TIMEOUT) {
> ast_moutdwm(ast, 0x1e6e0070, 0x00000000);
> - return 0;
> + return -1;
> }
> } while (!data);
> + data = ast_mindwm(ast, 0x1e6e0078);
> + data = (data | (data >> 16)) & 0xffff;
> ast_moutdwm(ast, 0x1e6e0070, 0x00000000);
> - return 1;
> + return data;
> }
>
> -static int mmc_test_burst2(struct ast_private *ast, u32 datagen)
> +
> +static int mmc_test_burst(struct ast_private *ast, u32 datagen)
> {
> - u32 data, timeout;
> + return mmc_test(ast, datagen, 0xc1);
> +}
>
> - ast_moutdwm(ast, 0x1e6e0070, 0x00000000);
> - ast_moutdwm(ast, 0x1e6e0070, 0x00000041 | (datagen << 3));
> - timeout = 0;
> - do {
> - data = ast_mindwm(ast, 0x1e6e0070) & 0x1000;
> - if (++timeout > TIMEOUT) {
> - ast_moutdwm(ast, 0x1e6e0070, 0x0);
> - return -1;
> - }
> - } while (!data);
> - data = ast_mindwm(ast, 0x1e6e0078);
> - data = (data | (data >> 16)) & 0xffff;
> - ast_moutdwm(ast, 0x1e6e0070, 0x0);
> - return data;
> +static int mmc_test_burst2(struct ast_private *ast, u32 datagen)
> +{
> + return mmc_test(ast, datagen, 0x41);
> }
>
> static int mmc_test_single(struct ast_private *ast, u32 datagen)
> {
> - u32 data, timeout;
> -
> - ast_moutdwm(ast, 0x1e6e0070, 0x00000000);
> - ast_moutdwm(ast, 0x1e6e0070, 0x000000c5 | (datagen << 3));
> - timeout = 0;
> - do {
> - data = ast_mindwm(ast, 0x1e6e0070) & 0x3000;
> - if (data & 0x2000)
> - return 0;
> - if (++timeout > TIMEOUT) {
> - ast_moutdwm(ast, 0x1e6e0070, 0x0);
> - return 0;
> - }
> - } while (!data);
> - ast_moutdwm(ast, 0x1e6e0070, 0x0);
> - return 1;
> + return mmc_test(ast, datagen, 0xc5);
> }
>
> static int mmc_test_single2(struct ast_private *ast, u32 datagen)
> {
> - u32 data, timeout;
> + return mmc_test(ast, datagen, 0x05);
> +}
>
> - ast_moutdwm(ast, 0x1e6e0070, 0x00000000);
> - ast_moutdwm(ast, 0x1e6e0070, 0x00000005 | (datagen << 3));
> - timeout = 0;
> - do {
> - data = ast_mindwm(ast, 0x1e6e0070) & 0x1000;
> - if (++timeout > TIMEOUT) {
> - ast_moutdwm(ast, 0x1e6e0070, 0x0);
> - return -1;
> - }
> - } while (!data);
> - data = ast_mindwm(ast, 0x1e6e0078);
> - data = (data | (data >> 16)) & 0xffff;
> - ast_moutdwm(ast, 0x1e6e0070, 0x0);
> - return data;
> +static int mmc_test_single_2500(struct ast_private *ast, u32 datagen)
> +{
> + return mmc_test(ast, datagen, 0x85);
> }
>
> static int cbr_test(struct ast_private *ast)
> @@ -603,9 +574,9 @@ static u32 cbr_scan2(struct ast_private *ast)
>
> static u32 cbr_test3(struct ast_private *ast)
> {
> - if (!mmc_test_burst(ast, 0))
> + if (mmc_test_burst(ast, 0) < 0)
> return 0;
> - if (!mmc_test_single(ast, 0))
> + if (mmc_test_single(ast, 0) < 0)
The above seems like a _lot_ of unrelated rework. Keep the refactoring
separate ?
New code seems to assume that only(?) -1 is returned on error, yet we do < 0.
This will come to bite you/others as the tests are extended/reworked.
> return 0;
> return 1;
> }
> @@ -1609,7 +1580,7 @@ static void ddr2_init(struct ast_private *ast, struct ast2300_dram_param *param)
>
> }
>
> -static void ast_init_dram_2300(struct drm_device *dev)
> +static void ast_post_chip_2300(struct drm_device *dev)
Unrelated rename ?
> +static bool ddr_test_2500(struct ast_private *ast)
> +{
> + ast_moutdwm(ast, 0x1E6E0074, 0x0000FFFF);
> + ast_moutdwm(ast, 0x1E6E007C, 0xFF00FF00);
> + if (mmc_test_burst(ast, 0) < 0)
> + return false;
> + if (mmc_test_burst(ast, 1) < 0)
> + return false;
> + if (mmc_test_burst(ast, 2) < 0)
> + return false;
> + if (mmc_test_burst(ast, 3) < 0)
> + return false;
> + if (mmc_test_single_2500(ast, 0) < 0)
> + return false;
> + return false;
Final return should be true... either things got funny with v2 or the
this patch was never tested ?
> +static void ddr_phy_init_2500(struct ast_private *ast)
> +{
> + u32 data, pass, timecnt;
> +
> + pass = 0;
> + ast_moutdwm(ast, 0x1E6E0060,0x00000005);
> + while (!pass) {
> + for(timecnt = 0;timecnt < TIMEOUT;timecnt++) {
> + data = ast_mindwm(ast, 0x1E6E0060) & 0x1;
> + if (!data)
> + break;
> + }
> + if (timecnt != TIMEOUT) {
> + data = ast_mindwm(ast, 0x1E6E0300) & 0x000A0000;
> + if (!data)
> + pass = 1;
> + }
> + if (!pass) {
> + ast_moutdwm(ast, 0x1E6E0060,0x00000000);
> + udelay(10); /* delay 10 us */
> + ast_moutdwm(ast, 0x1E6E0060,0x00000005);
> + }
> + }
> +
> + ast_moutdwm(ast, 0x1E6E0060,0x00000006);
I realise that there's likely no documentation, yet repeating the same
magic numbers multiple times is a bit meh.
> +}
> +
> +/******************************************************************************
> + Check DRAM Size
> + 1Gb : 0x80000000 ~ 0x87FFFFFF
> + 2Gb : 0x80000000 ~ 0x8FFFFFFF
> + 4Gb : 0x80000000 ~ 0x9FFFFFFF
> + 8Gb : 0x80000000 ~ 0xBFFFFFFF
> +*****************************************************************************/
> +static void check_dram_size_2500(struct ast_private *ast, u32 tRFC)
> +{
> + u32 reg_04, reg_14;
> +
> + reg_04 = ast_mindwm(ast, 0x1E6E0004) & 0xfffffffc;
> + reg_14 = ast_mindwm(ast, 0x1E6E0014) & 0xffffff00;
> +
> + ast_moutdwm(ast, 0xA0100000, 0x41424344);
> + ast_moutdwm(ast, 0x90100000, 0x35363738);
> + ast_moutdwm(ast, 0x88100000, 0x292A2B2C);
> + ast_moutdwm(ast, 0x80100000, 0x1D1E1F10);
> +
> + /* Check 8Gbit */
> + if (ast_mindwm(ast, 0xA0100000) == 0x41424344) {
> + reg_04 |= 0x03;
> + reg_14 |= (tRFC >> 24) & 0xFF;
> + /* Check 4Gbit */
> + } else if (ast_mindwm(ast, 0x90100000) == 0x35363738) {
> + reg_04 |= 0x02;
> + reg_14 |= (tRFC >> 16) & 0xFF;
> + /* Check 2Gbit */
> + } else if (ast_mindwm(ast, 0x88100000) == 0x292A2B2C) {
> + reg_04 |= 0x01;
> + reg_14 |= (tRFC >> 8) & 0xFF;
> + } else {
> + reg_14 |= tRFC & 0xFF;
> + }
Like in the case above...
> +static void reset_mmc_2500(struct ast_private *ast)
> +{
> + ast_moutdwm(ast, 0x1E78505C,0x00000004);
> + ast_moutdwm(ast, 0x1E785044,0x00000001);
> + ast_moutdwm(ast, 0x1E785048,0x00004755);
> + ast_moutdwm(ast, 0x1E78504C,0x00000013);
> + mdelay(100); /* delay 100ms */
"Water is wet" type of comment. Worth mentioning why it's so large -
mentioned in the documentation, experimental result, other ?
Same suggestion goes for the other mdelay(100) instances.
> +static bool ast_dram_init_2500(struct ast_private *ast)
> +{
> + u32 data;
> + u32 max_tries = 5;
> +
> + do {
> + if (max_tries-- == 0)
> + return false;
> + set_mpll_2500(ast);
> + reset_mmc_2500(ast);
> + ddr_init_common_2500(ast);
> +
> + data = ast_mindwm(ast, 0x1E6E2070);
> + if (data & 0x01000000)
> + ddr4_init_2500(ast, ast2500_ddr4_1600_timing_table);
> + else
> + ddr3_init_2500(ast, ast2500_ddr3_1600_timing_table);
> + } while (!ddr_test_2500(ast));
> +
> + ast_moutdwm(ast, 0x1E6E2040, ast_mindwm(ast, 0x1E6E2040) | 0x41);
> +
> + /* Patch code */
> + data = ast_mindwm(ast, 0x1E6E200C) & 0xF9FFFFFF;
> + ast_moutdwm(ast, 0x1E6E200C, data | 0x10000000);
> +
> + return true;
Drop the return type - function always returns true ?
I think there were a few other functions that could do the same.
Regards,
Emil
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
