Re: [PATCH 2/2] drm/ast: Support AST2500

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2017-02-16 at 17:33 +0000, Emil Velikov wrote:
> On 16 February 2017 at 09:09, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > From: "Y.C. Chen" <yc_chen@aspeedtech.com>
> > 
> > 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@aspeedtech.com>
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel