-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(PeriphDrivers): Add MAX32657 I3C driver #1026
Conversation
Please refer to the The i3c_regs.h file is scrubbed for the user guide and synced with design's finalization of the IP configuration. Also, please note that the I3C source drivers will require hardware revision files for easier portability across parts. That means, there will at least be an The The You can reference our other peripheral drivers to see how this is all set up. Let me know if you have any questions. |
I missed the latest register updates so put my own for the work in progress. I will look into adapting into feat/ME30 branch. Thanks for the response. |
41844d7
to
0a8c0f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ttmut, some minor change requests below
Thanks @Jake-Carter for your review. I have addressed your comments so could you have a look once more? |
val = 0 << 6; /* Device role: target */ | ||
if (i3c->targ_cap1 & MXC_S_I3C_REVA_TARG_CAP1_CCCH_LIMITS) { | ||
val |= 1 << 0; /* Max data speed limitation */ | ||
} | ||
if (i3c->targ_cap1 & MXC_S_I3C_REVA_TARG_CAP1_CCCH_BASIC) { | ||
val |= 1 << 5; /* Supports advanced capabiliries */ | ||
} | ||
if (i3c->targ_cap1 & MXC_S_I3C_REVA_TARG_CAP1_IBI_EVENTS_IBI) { | ||
val |= 1 << 1; /* Supports IBI generation */ | ||
} | ||
if (i3c->targ_cap1 & MXC_S_I3C_REVA_TARG_CAP1_IBI_EVENTS_PAYLOAD) { | ||
val |= 1 << 2; /* MDB and additional data bytes may follow the IBI */ | ||
} | ||
val |= 1 << 3; /* Offline capable */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a good idea to bring these out as enumerated definitions in the register file so you don't have to hardcode these values.
int retries = 3; | ||
|
||
while (retries && MXC_I3C_RevA_GetState(i3c) == MXC_V_I3C_REVA_CONT_STATUS_STATE_SDR_NORM) { | ||
MXC_I3C_RevA_EmitStop(i3c); | ||
retries--; | ||
} | ||
|
||
retries = 20; | ||
while (retries-- && GET_FIELD(cont_status, CONT_STATUS_TARG_START)) { | ||
MXC_I3C_RevA_AutoIBI(i3c); | ||
MXC_I3C_RevA_WaitForDone(i3c, 1000); | ||
MXC_I3C_RevA_ClearRXFIFO(i3c); | ||
} | ||
|
||
retries = 10; | ||
while (retries && MXC_I3C_RevA_GetState(i3c) != MXC_V_I3C_REVA_CONT_STATUS_STATE_IDLE) { | ||
retries--; | ||
} | ||
|
||
if (retries == 0) { | ||
return E_TIME_OUT; | ||
} | ||
|
||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, are the number of retries given by the spec or something we chose that is probably a good amount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the first one, I observed that it gets stuck there sometimes, i.e., TARG_START is continuously being set even after IBI request is handled. Therefore, I made it fall through after 20 retries, expecting less than 20 devices on the bus. I3C specs do not put a limit on the number of supported devices on an I3C bus. In earlier specs it was given as 11.
Second one is used for delay though I realize now that 10 loops is too short for a delay. The reason I did not want to use MXC_Delay function for delays is that it could conflict with Zephyr OS which also uses SysTick for timing and delays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a second look, it seems I forgot to clear TARG_START bit. I will try to see if it solves the problem I mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
If you want to be consistent with the Controller
vs Target
names, the Docs team and I have been separating these two terms on the ME30 for readability because this IP is designed with a clear separation between these two features.
As a general naming rule with controller and target:
The register-fields would either have a CONT_
/TARG_
prefix or _CONT
/_TARG
suffix in their names. So CONT_INTEN
/TARG_INTEN
is used instead of CONTINTEN
/TARGINTEN
for the interrupt enable register names.
The function names would have a section dedicated to these two blocks, _Controller_
and _Target_
, if one generic function can't handle a feature for both the controller and target.
The general format for the function name:
MXC_{PERIPH}_{Sub-Blocks, if any}_{FunctionName}(...)
Following the format, for example, the MXC_I3C_ControllerEnableInt(...)
function would be MXC_I3C_Controller_EnableInt(...)
. And MXC_I3C_TargetEnableInt(...)
=> MXC_I3C_Target_EnableInt(...)
Hope this made sense. A good example of this function naming convention is with the Crypto Tool Box (CTB) in the ME13, ME18, ME21, and ME55.
I see your point. I was just trying to avoid using Controller_, Target_ prefixes whenever possible to prevent function names becoming too long. For example, clock can only be generated by controller so MXC_I3C_SetXXFrequency functions are inherently controller-specific. Anyway, I will update the function names accordingly. Thanks for your detailed response. |
It's not a hard rule. I was mainly referring to "duplicate" functions like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ttmut, let me know when you are ready to merge
CONT_REQ must be 2 and HOTJOIN_REQ must be 3. Add enumerations for IBI_EVENTS and CCCH fields of TARG_CAP1 register. Signed-off-by: Tahsin Mutlugun <Tahsin.Mutlugun@analog.com>
Add I3C driver support for ME30.c. Signed-off-by: Tahsin Mutlugun <Tahsin.Mutlugun@analog.com>
I do not have anything else to add at the moment. Let's also wait @sihyung-maxim's approval. |
Description
Add I3C driver for MAX32657. Basic controller and target APIs are provided.
Note: Will be tested when the test environment is ready.I3C has been tested with an LPS22HH pressure and temperature sensor.
Legacy I2C has been tested with an AT24C04 EEPROM.
Read provisioned ID, using static address (0x5C) as dynamic address.
Read pressure data, using static address (0x5C) as dynamic address.
Assign 0x34 as dynamic address. Written to device as 0x68, 1-bit shifted to the left.
Read WHO_AM_I register using newly assigned dynamic address 0x34.
I2C write 8 bytes to EEPROM
I2C read 8 bytes from EEPROM
Checklist Before Requesting Review