Patchwork Get rid of unaligned 32-bit config space read in mcd_d.c

login
register
about
Submitter Arne Georg Gleditsch
Date 2010-04-12 06:52:21
Message ID <87tyrhduxm.fsf@taniquetil.gledits.ch>
Download mbox | patch
Permalink /patch/1224/
State New
Headers show

Comments

Arne Georg Gleditsch - 2010-04-12 06:52:21
Hi,

This patch removes a non-32-bit aligned Get_NB32 from
src/northbridge/amd/amdmct/mct/mct_d.c.  The actual behavior should be
unchanged for I/O-based config space reads, but previous behavior was
wrong for MMCFG-based config space reads.

Perhaps we want pci_mmio_*_config* to enforce natural alignment as well?


Signed-off-by: Arne Georg Gleditsch <arne.gleditsch@numascale.com>
Marc Jones - 2010-04-12 21:16:24
On Mon, Apr 12, 2010 at 12:52 AM, Arne Georg Gleditsch
<arne.gleditsch@numascale.com> wrote:
> Hi,
>
> This patch removes a non-32-bit aligned Get_NB32 from
> src/northbridge/amd/amdmct/mct/mct_d.c.  The actual behavior should be
> unchanged for I/O-based config space reads, but previous behavior was
> wrong for MMCFG-based config space reads.
>
> Perhaps we want pci_mmio_*_config* to enforce natural alignment as well?
>
>
> Signed-off-by: Arne Georg Gleditsch <arne.gleditsch@numascale.com>
>
> --
>                                                        Arne.

Hi Arne,

That code is doing something a little ugly to make the two cs
registers use the one mask register. It was ported from a routine that
assumed that GET_NB32() fixed the alignment. Maybe add a comment about
why that is happening.

Acked-by: Marc Jones <marcj303@gmail.com>

I would also ack a change to:
u32 Get_NB32(u32 dev, u32 reg)
u32 Get_NB32(u32 dev, u32 reg)

or to the  pci_mmio_*_config* to do it since the pci_io* versions do.

Marc
Arne Georg Gleditsch - 2010-04-13 07:40:09
Marc Jones <marcj303@gmail.com> writes:
> That code is doing something a little ugly to make the two cs
> registers use the one mask register. It was ported from a routine that
> assumed that GET_NB32() fixed the alignment. Maybe add a comment about
> why that is happening.

I assumed as much.  I'll see if I can't put a comment in.

> Acked-by: Marc Jones <marcj303@gmail.com>
>
> I would also ack a change to:
> u32 Get_NB32(u32 dev, u32 reg)
> u32 Get_NB32(u32 dev, u32 reg)
>
> or to the  pci_mmio_*_config* to do it since the pci_io* versions do.

Thanks, I'll include this as well.  The current situation with different
semantics on unaligned accesses isn't really helping anyone, as far as I
can see.

Patch

diff --git a/src/northbridge/amd/amdmct/mct/mct_d.c b/src/northbridge/amd/amdmct/mct/mct_d.c
index 22e9902..1779938 100644
--- a/src/northbridge/amd/amdmct/mct/mct_d.c
+++ b/src/northbridge/amd/amdmct/mct/mct_d.c
@@ -1970,7 +1970,7 @@  static void StitchMemory_D(struct MCTStatStruc *pMCTstat,
 				reg  = 0x40 + (q << 2) + reg_off;  /* Base[q] reg.*/
 				val = Get_NB32(dev, reg);
 				if (!(val & 3)) {	/* (CSEnable|Spare==1)bank is enabled already? */
-					reg = 0x60 + (q << 1) + reg_off; /*Mask[q] reg.*/
+					reg = 0x60 + ((q << 1) & 0xc) + reg_off; /*Mask[q] reg.*/
 					val = Get_NB32(dev, reg);
 					val >>= 19;
 					val++;