| Submitter | Myles Watson |
|---|---|
| Date | 2009-10-23 23:06:35 |
| Message ID | <2831fecf0910231606s2e77596dv1cd67e8e70123f1a@mail.gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/468/ |
| State | Accepted |
| Headers | show |
Comments
On Fri, Oct 23, 2009 at 05:06:35PM -0600, Myles Watson wrote: > On Fri, Oct 23, 2009 at 4:56 PM, Uwe Hermann <uwe@hermann-uwe.de> wrote: > > > On Fri, Oct 23, 2009 at 02:43:27PM -0600, Myles Watson wrote: > > > Ping. I think I've addressed all of Peter and Uwe's concerns. > > > > I Think so, but please repost the updated patch so we can have a look > > at the final version. > > > I don't think the first patches have changed much, but there are two new > ones that apply on top: peter.diff and uwe.diff. > > Signed-off-by: Myles Watson <mylesgw@gmail.com> Acked-by: Uwe Hermann <uwe@hermann-uwe.de> > +config HAVE_LOW_TABLES > + bool > + default y > + help > + This Option is unused in the code. Since two boards try to set it to > + 'n', they may be broken. We either need to make the option useful or > + get rid of it. The broken boards are: > + asus/m2v-mx_se > + supermicro/h8dme Please add a "# TODO" comment on top of that for easy grepping. > +config MEM_TRAIN_SEQ > + int > + default 2 We should add a MEM_TRAIN_SEQ comment or help text, it's unclear to me what it is supposed to do. Is it a per-chipset, per-cpu, or per-board option? What do the values of the variable mean? Regardless of that, it seems like it should be a user-visible option in menuconfig with useful option names for the values of 0, 1, and 2 (which seem to be the only valid ones). That's for another patch though. Uwe.
Uwe Hermann wrote: > We should add a MEM_TRAIN_SEQ comment or help text, it's unclear to me what it is > supposed to do. Is it a per-chipset, per-cpu, or per-board option? What > do the values of the variable mean? > > Regardless of that, it seems like it should be a user-visible option in > menuconfig Shouldn't we clarify what it does _before_ deciding it should be user visible? ;-) Just an idea.... Going through the AMD ram init code it loops like the code is used to trigger certain code paths during ram init. It does look somewhat random to me, but I guess it's due to board requirements to choose one or the other option. In which case adding it as a user visible option just creates another way to produce a non-booting image without any real gain.. The Tyan s2912 uses 1, the s2912_fam10 uses 2. Maybe we can even drop the option completely? Going through more code I see a lot of this: #if CONFIG_MEM_TRAIN_SEQ == 0 #define DQS_DELAY_COPY NODE_NUMS #else // #define DQS_DELAY_COPY 1 #define DQS_DELAY_COPY NODE_NUMS #endif which is about as good as #define DQS_DELAY_COPY NODE_NUMS Anyone knows more details what the option is good for? Stefan
On Sat, Oct 24, 2009 at 5:11 PM, Uwe Hermann <uwe@hermann-uwe.de> wrote: > On Fri, Oct 23, 2009 at 05:06:35PM -0600, Myles Watson wrote: > > On Fri, Oct 23, 2009 at 4:56 PM, Uwe Hermann <uwe@hermann-uwe.de> wrote: > > > > > On Fri, Oct 23, 2009 at 02:43:27PM -0600, Myles Watson wrote: > > > > Ping. I think I've addressed all of Peter and Uwe's concerns. > > > > > > I Think so, but please repost the updated patch so we can have a look > > > at the final version. > > > > > I don't think the first patches have changed much, but there are two new > > ones that apply on top: peter.diff and uwe.diff. > > > > Signed-off-by: Myles Watson <mylesgw@gmail.com> > > Acked-by: Uwe Hermann <uwe@hermann-uwe.de> > Rev 4856. > > > +config HAVE_LOW_TABLES > > + bool > > + default y > > + help > > + This Option is unused in the code. Since two boards try to set > it to > > + 'n', they may be broken. We either need to make the option > useful or > > + get rid of it. The broken boards are: > > + asus/m2v-mx_se > > + supermicro/h8dme > > Please add a "# TODO" comment on top of that for easy grepping. > Done. > > +config MEM_TRAIN_SEQ > > + int > > + default 2 > > We should add a MEM_TRAIN_SEQ comment or help text, it's unclear to me what > it is > supposed to do. Is it a per-chipset, per-cpu, or per-board option? What > do the values of the variable mean? > All good questions. It's unclear to me. > Regardless of that, it seems like it should be a user-visible option in > menuconfig with useful option names for the values of 0, 1, and 2 (which > seem to be the only valid ones). > The more things we make "user configurable", the more ways a user can break the build/ brick his board. Thanks, Myles
> > We should add a MEM_TRAIN_SEQ comment or help text, it's unclear to me > what it is > > supposed to do. Is it a per-chipset, per-cpu, or per-board option? What > > do the values of the variable mean? > > > > Regardless of that, it seems like it should be a user-visible option in > > menuconfig > Shouldn't we clarify what it does _before_ deciding it should be user > visible? ;-) Agreed. > Maybe we can even drop the option completely? > Anyone knows more details what the option is good for? Unfortunately, I don't. I was just trying to match newconfig. Thanks, Myles
I don't think any of these options should be user configurable ... it's all black magic and setting it wrong is going to ruin your day. ron
Patch
Index: svn/src/southbridge/via/vt8237r/vt8237r.h =================================================================== --- svn.orig/src/southbridge/via/vt8237r/vt8237r.h +++ svn/src/southbridge/via/vt8237r/vt8237r.h @@ -65,7 +65,7 @@ #define I2C_TRANS_CMD 0x40 #define CLOCK_SLAVE_ADDRESS 0x69 -#if defined(DEBUG_SMBUS) && DEBUG_SMBUS == 1 +#if DEBUG_SMBUS == 1 #define PRINT_DEBUG(x) print_debug(x) #define PRINT_DEBUG_HEX16(x) print_debug_hex16(x) #else Index: svn/src/cpu/amd/model_10xxx/init_cpus.c =================================================================== --- svn.orig/src/cpu/amd/model_10xxx/init_cpus.c +++ svn/src/cpu/amd/model_10xxx/init_cpus.c @@ -473,7 +473,7 @@ static void start_node(u8 node) /* Enable routing table */ printk_debug("Start node %02x", node); -#if CONFIG_NORTHBRIDGE_AMD_AMDFAM10 == 1 +#if CONFIG_NORTHBRIDGE_AMD_AMDFAM10 /* For FAM10 support, we need to set Dram base/limit for the new node */ pci_write_config32(NODE_MP(node), 0x44, 0); pci_write_config32(NODE_MP(node), 0x40, 3);