Skip to content

Correct bitfield bits position and manage bus endianness#15

Open
yseraf wants to merge 1 commit intohelium:mainfrom
Wifx:fix/endianness
Open

Correct bitfield bits position and manage bus endianness#15
yseraf wants to merge 1 commit intohelium:mainfrom
Wifx:fix/endianness

Conversation

@yseraf
Copy link
Copy Markdown

@yseraf yseraf commented May 1, 2022

While bitfield should follow exactly datasheet's information, endianness must be handled for bus transport which is always little.

This fix converts endianness to/from bus for data to be handled by arch endianness (big or little).

This pull request aims to replace #13 in a more cleaner way.

While bitfield should follow exactly datasheet's information,
endianness must be handled for bus transport which is always little.

This fix converts endianness to/from bus for data to be handled
by arch endianness (big or little).

Signed-off-by: Yannick Lanz <yannick.lanz@wifx.net>
@yseraf yseraf force-pushed the fix/endianness branch from 795dfcb to 1a9cc1c Compare May 1, 2022 10:57
@madninja
Copy link
Copy Markdown
Member

madninja commented May 1, 2022

This is an interesting alternate approach.. I, see it does make it quite a bit harder to match the bitfields to the microchip documentation

@yseraf
Copy link
Copy Markdown
Author

yseraf commented May 1, 2022

Small note before moving too far, I tested it on little endian arch and ran only key, info, test and bench commands. I still need to test provision on unprogrammed atecc.

@yseraf
Copy link
Copy Markdown
Author

yseraf commented May 1, 2022

I, see it does make it quite a bit harder to match the bitfields to the microchip documentation

I don't understand, my patch uses exactly the same bitfield definitions than the datasheet

@madninja
Copy link
Copy Markdown
Member

madninja commented May 1, 2022

I, see it does make it quite a bit harder to match the bitfields to the microchip documentation

I don't understand, my patch uses exactly the same bitfield definitions than the datasheet

You're right in that they are the same definition, but the bit indices used in the documentation line up with what is in the code today.. vs reading them inverted to match a big endian definition?

@yseraf
Copy link
Copy Markdown
Author

yseraf commented May 1, 2022

This how bitfield should be managed, they are always represented as "endian free" in datasheet then bus endianess must be managed.

This is, at least from what I know, how it is managed over the whole Linux kernel for exemple (look at ntoh and hton function)

@madninja
Copy link
Copy Markdown
Member

madninja commented May 1, 2022

This how bitfield should be managed, they are always represented as "endian free" in datasheet then bus endianess must be managed.

This is, at least from what I know, how it is managed over the whole Linux kernel for exemple (look at ntoh and hton function)

Oh I agree.. my point is that the microchip ecc documentation talks about bits 0-3 being the auth_key in KeyConfig and your change makes it bits 8-11. Which makes it harder to follow

@yseraf
Copy link
Copy Markdown
Author

yseraf commented May 2, 2022

It's like we don't have the same data sheet, I wrote you an email to talk more about its content, I don't know if you received it.

}
}

// Should probably be removed as it should manage bus endianness while
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this managing bus endianness? Need to be able to construct a Key/SlotConfig from bytes right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants