On Thu, 2017-02-16 at 17:33 +0000, Emil Velikov wrote:
> 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 ;-)
Heh ok. I don't want to change that POST code too much as I'm not
equipped to test it, but I'll have a look later today.
Thanks,
Ben.
>
> > +/*
> > + * 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
