Initial support for microchip cec1702 #16669
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Found the following issues, please fix and resubmit: License issuesIn most cases you do not need to do anything here, especially if the files
|
dbkinder
suggested changes
Jun 7, 2019
| Debugging | ||
| ========= | ||
|
|
||
| The MCU has standard JTAG debugging features. But the author of this port |
There was a problem hiding this comment.
Delete "But the author of this port has not used it." Can you provide a reference link about this in the microchip documentation you're linking to?
There was a problem hiding this comment.
@franciscomunoz Seems you added standard Debugging example for the MEC boards. Does that work? If yes, it should work also for CEC and I could just add the same here.
There was a problem hiding this comment.
@fabled It works on the MEC1701 and MEC1501. It could work on the CEC1702 for sure, but very basic features as it may require extra support from the tool vendor.
There was a problem hiding this comment.
Ok. Maybe I copy that over and state is as "should" work. I can also look into cloning the parts of the Building steps. But the objcopy step is not needed for me. I also have python based tool that can do the zephyr.bin conversion to SPI flashable image, but I think that is worth a separate commit.
@franciscomunoz Do you know if MEC is using same SPI image format as CEC? I think it does, it's mostly the same thing. I also saw you got the gpio and pinmux stuff done, those could be reused in CEC side too.
There was a problem hiding this comment.
@fabled It worked on the 1701, You just need to update the base address. Another thing I remember is the script ran with python2.7. It never ran with python3.X for me. Right now there are gpio,pimux, i2c, eSPI(subsystem is being reviewed). More stuff is being cooked. Drivers should work on the CEC (in fact this is why there is an Xec). However, a new HAL is going to be needed for the CEC to abstract the registers in the same way it is done for the MEC1501.
There was a problem hiding this comment.
@fabled However, a new HAL is going to be needed for the CEC to abstract the registers in the same way it is done for the MEC1501.
I am thinking it might be better to try to avoid Microchip HAL. Similar how the UART code is shared, and use DT for providing all the base addresses, interrupts etc. Perhaps the gpio,pinmux,etc. could be made to have the register offsets internally, and take everything else from DT and be independent from the HAL. What do you think?
Also does MEC have the same QMSPI? If yes, should that be renamed to xec_qmspi and also used in the MEC code?
There was a problem hiding this comment.
@fabled I can't make any of this decisions as there are several people involved. The easiest path for would be to have the CEC1702 HAL.
zephyrbot
added
area: Devicetree
area: SPI
|
Any ideas why the |
dbkinder
suggested changes
Jun 11, 2019
|
Building/Flashing and Debugging sections updated per request. Added Python script to do the SPI image creation, so there is now no dependency on proprietary Microchip SDK (Windows) tools. Rebased on top of master, and retested Any ideas why |
fabled
force-pushed
the
cec1702
branch
2 times, most recently
from
e63ee84 to
2192ee2
Compare
|
Any ideas why these come? |
|
Anything more to fix? The test suite is now fixed with the exception of License one due to new HAL, and documentation should be fixed per request. Any more comments or concerns? |
what test or sample are you seeing this on? |
Not with any test/sample. It comes only when compiling in the driver. So it happens only on my customized config. Similar to what the current in-tree spi nor driver says when it's compiled: |
dbkinder
suggested changes
Jul 24, 2019
fabled
force-pushed
the
cec1702
branch
2 times, most recently
from
6f5e617 to
dbf0fb1
Compare
I need to look into this in a bit more detail. Writing your own tool to do git submodules needs a bit of learning curve. Is there some documentation on how to use it? I would also be interested in the rationale why a) you split the code base to submodules when there's direct dependencies between the modules like Kconfigs, and b) not just use git submodules or other already existing tools? That is what is the benefit to use |
|
Seems to be failing due to not able to checkout correct revision of hal_microchip module. Perhaps zephyrproject-rtos/hal_microchip#4 could be merged first, and then trigger rebuild here. |
|
FYI @scottwcpg |
|
@galak Ping. @franciscomunoz @scottwcpg @albertofloyd @MaureenHelm @ioannisg I would like to use Zephyr on the CEC1702 which is of same family. And when I asked Microchip if they could publish the same style headers, they started asking financial justification for the work. I am still in discussion regarding this. But we are not getting the new HAL headers for other chips. Would it be more portable to use the original SDK HAL headers for the xec drivers so they actually work through out the CEC/MEC family? But hopefully we get the new HAL headers for other CEC/MEC family devices too. Maybe you can also contribute into getting them? |
|
@fabled let me provide some context on this and some ideas that could help. Early on when mec1701/mec1501 support started, HAL-based drivers were the plan all along. After been working for a while on Zephyr a few enhancements in the HAL have been discussed, hence the style of headers you see under mec1501 folder. In the long term plan these are some of the changes considered.
I think in your case you could start with mec1501 headers as your CEC17 HAL and adjust them based on CEC17 datasheet, as I mentioned before you will find a lot of commonalities. |
Ok. However, I see the new style HAL being both improvement and a step backwards in some aspects. Perhaps it's still work-in-progress, and the minor inconvenience can be fixed? It is very good thing that it's components based, and thus can be much more easier reused in compatible chips. That also means it does not clutter #define namespace etc.
Is it? I was using the original CEC HAL and as the block structures are defined as
@scottwcpg Are the new HAL headers already generated automatically, or are they manual work? Would it be possible to strip the base addresses from current component headers and make them generic and shareable? E.g. move all the base addresses and GIRQ numbers etc. to the SoC header, and keep only the register defines and structs in the component header. Perhaps doing some exceptions for the GIRQ side of things until the real driver gets worked out. If the headers are manual work, and the above plan sounds acceptable, I would be happy to help with the work and make the current tree work with CEC1702, at least with the peripherals I am currently needing.
Yes. That's why am calling in for the discussion how to share the existing drivers, and HAL :) Though, there are some peripherals unique to CEC also. I believe one example is the GP-SPI, to which I just wrote a basic driver. See fabled@6767cbd (MEC has eSPI instead). |
Basic support for the SoC and the UARTs in it. UART divider is reset to register value zero which the chip treats as 3. Thus the default baud rate for the chip is 38400. The SoC has external interrupt controller (EC) which is configured to pass-through all interrupts to NVIC. Support for the EC may need to be implemented better if/when deep sleep modes are implemented. A post-build script cec1702-image.py will create SPI flashable image. Signed-off-by: Timo Teräs <[email protected]>
Add basic secureiot1072 support with serial ports. Tested with hello_world to run and work as expected. Signed-off-by: Timo Teräs <[email protected]>
CEC1702 ns16550 compatible UART has alternate high-speed clock selectable with the high bit of the baud rate divisor register. Signed-off-by: Timo Teräs <[email protected]>
|
Rebased on top of master. Direction on how to do HAL would be appreciated. |
github-actions
bot
added
area: Documentation
area: Modules
has-conflicts
github-actions
bot
added
has-conflicts
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since "Initial support for microchip cec1702 #5383" got closed and had lot of cruft. I decided to make new PR. This is essentially the same set of changes rebased on top of current master. One commit was merged earlier, and it's removed. The other commits were updated to reflect changes in current master.