From: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
Sent: Tuesday, 22 September 2020 14:38
To: Vincent Nicolas <Nicolas.Vincent@xxxxxxxxxxx>; jochen@xxxxxxxx <jochen@xxxxxxxx>
Cc: linuxppc-dev@xxxxxxxxxxxxxxxx <linuxppc-dev@xxxxxxxxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx <linux-i2c@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure
Le 22/09/2020 à 11:04, nico.vince@xxxxxxxxx a écrit :
> From: Nicolas VINCENT <nicolas.vincent@xxxxxxxxxxx>
>
> the i2c_ram structure is missing the sdmatmp field mentionned in
> datasheet for MPC8272 at paragraph 36.5. With this field missing, the
> hardware would write past the allocated memory done through
> cpm_muram_alloc for the i2c_ram structure and land in memory allocated
> for the buffers descriptors corrupting the cbd_bufaddr field. Since this
> field is only set during setup(), the first i2c transaction would work
> and the following would send data read from an arbitrary memory
> location.
>
> Signed-off-by: Nicolas VINCENT <nicolas.vincent@xxxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-cpm.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
> index 1213e1932ccb..c5700addbf65 100644
> --- a/drivers/i2c/busses/i2c-cpm.c
> +++ b/drivers/i2c/busses/i2c-cpm.c
> @@ -64,7 +64,8 @@ struct i2c_ram {
> uint txtmp; /* Internal */
> char res1[4]; /* Reserved */
> ushort rpbase; /* Relocation pointer */
> - char res2[2]; /* Reserved */
> + char res2[6]; /* Reserved */
> + uint sdmatmp; /* Internal */
On CPM1, I2C param RAM has size 0x30 (offset 0x1c80-0x1caf)
Your change overlaps the miscellaneous area that contains CP Microcode
Revision Number, ref MPC885 Reference Manual §18.7.3
As far as I understand the mpc885 contains in the dts the compatible=fsl,cpm1-i2c which is used in cpm-i2c.c to either determine the address of the i2c_ram structure (cpm1), or dynamically allocate it with cpm_muram_alloc (cpm2).
In the first case the structure will indeed overlaps with the miscellaneous section but since the sdmatmp is only used by cpm2 hardware it shall not be an issue.
Please, let me know if I am mistaken. If the patch cannot be accepted as is, I would gladly accept pointers on how to address this kind of issue.
Thanks,
Nicolas.
> };
>
> #define I2COM_START 0x80
>
Christophe
