Initial support for microchip cec1702 #5383

fabled · 2017-12-13T17:58:33Z

@galak Finally got to making this PR as promised way earlier. Please take a look if things are done in a sane way, or need fixing.

galak

The board, SoC, and HAL import should all be separate commits. The HAL commit message should conform to what is specified in CONTRIBUTING.rst

galak

All *.c & *.h files should have copyright/SPDX license info.

Also missing board docs. All new boards require board docs.

fabled · 2017-12-14T05:46:50Z

@galak Thanks, will fix accordingly.

Any comments on the Kconfig things? I was not fully sure how to name/use some of the board specifics, but I did what seemed to make sense and works.

UART_NS16550_PORT_0_CLK_FREQ is also hard coded in the fixup. Wonder how to put that into the dts.

I am next working on SPI and SPI Flash drivers. Is there any work on a generic SPI flash driver that I could reuse? I will need few flash specific overrides, but otherwise it's pretty generic chip on the demo board: SST26VF016B.

PINCTRL and GPIO drivers will probably come too later.

MaureenHelm · 2017-12-14T14:41:27Z

arch/arm/soc/microchip_cec/soc_config.c

+#ifdef CONFIG_UART_NS16550_PORT_0
+	PCR->CLK_REQ_2 |= 1 << 1;			/* Clock: UART0 */
+	GPIO_100_137->GPIO_104_PIN_CONTROL = 1 << 12;	/* GPIO104 Alt1 = UART0_TX */
+	GPIO_100_137->GPIO_105_PIN_CONTROL = 1 << 12;	/* GPIO105 Alt1 = UART0_RX */


We usually do pinmuxing in the board init rather than the SoC init

Ah, ok. I'll move the board specifics there. Eventually the pinmuxing will be done from the dts when pinctrl driver works.

MaureenHelm · 2017-12-14T14:43:29Z

boards/arm/secureiot1702/secureiot1702.dts

+	GPIO054 = LED Red
+	GPIO053 = LED Green
+	GPIO001 = LED Blue
+	GPIO157 = LED White


This comment block should be removed

MaureenHelm · 2017-12-14T14:45:00Z

boards/arm/secureiot1702/secureiot1702_defconfig

+CONFIG_BOARD_SECUREIOT1702=y
+CONFIG_CORTEX_M_SYSTICK=y
+CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=48000000
+CONFIG_HAS_DTS=y


This line shouldn't be here

MaureenHelm · 2017-12-14T14:47:39Z

ext/hal/Kconfig


 source "ext/hal/altera/Kconfig"

+source "ext/hal/cec/Kconfig"


Let's use "ext/hal/microchip/cec" instead

I make it cec1702 then. There's also cec1302 which is older version. Has same cpu but different peripheral blocks (especially the crypto side).

MaureenHelm · 2017-12-14T14:52:28Z

ext/hal/cec/include/system_MEC2016.h

+* CLAIMS IN ANY WAY RELATED TO THIS SOFTWARE WILL NOT EXCEED THE AMOUNT OF
+* FEES, IF ANY, THAT YOU HAVE PAID DIRECTLY TO MICROCHIP FOR THIS SOFTWARE.
+* MICROCHIP PROVIDES THIS SOFTWARE CONDITIONALLY UPON YOUR ACCEPTANCE
+* OF THESE TERMS.


This looks like a new license and will need approval by the gov board

This file should not be there. I'll remove it, it's unneeded.

fabled · 2017-12-15T20:55:08Z

Change requests addressed. Please review again. Copyright notices also added. This work from @CryptaLabs.

dbkinder · 2017-12-18T18:01:23Z

arch/arm/soc/microchip_cec/Kconfig.soc

CPU_FAMILY_CEC seems undefined

Right. This was little bit hazy for me on how and where to define SOC_FAMILY, SOC_SERIES and which type of CPU_{FAMILY,SERIES}_CEC things to define. Having that config split to Kconfig, Kconfig.soc, Kconfig.defconfig, etc. could be little bit less cluttered. I ended up not defining the family at all since there seems to be only two CEC CPUs at the moment. Maybe I'll just change this to CPU_SERIES_CEC

dbkinder · 2017-12-18T18:01:58Z

boards/arm/secureiot1702/Kconfig.board

SOC_PART_NUMBER_CEC1702 seems undefined

Same here. I just change this to SOC_CEC1702.

dbkinder · 2017-12-18T18:35:45Z

boards/arm/secureiot1702/doc/board.rst

Thanks for getting the documentation started. We've got a recommended template for board documentation at doc/templates/board.tmpl (in your cloned copy of the zephyr tree).

Ok, will take it from there. I also have pending patches for IRQ support, and QMSPI host driver for CEC. As well as generic SPI flash (auto-detects way to access chip using SFDP) driver. I will possibly also work on gpio, pinmux, clock drivers.

galak · 2018-01-13T19:51:30Z

Can update this to current and I'll review again towards getting it in tree

fabled · 2018-01-13T21:10:34Z

Rebase against master, addressed most checkpatch issues.

codecov-io · 2018-01-13T21:29:55Z

Codecov Report

Merging #5383 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5383   +/-   ##
=======================================
  Coverage   48.25%   48.25%           
=======================================
  Files         293      293           
  Lines       44130    44130           
  Branches    10591    10591           
=======================================
  Hits        21296    21296           
  Misses      18567    18567           
  Partials     4267     4267

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56d144c...efdb477. Read the comment docs.

fabled · 2018-11-17T07:47:38Z

It seems to be pretty free. The header file carries this:

 * @par      ARM Limited (ARM) is supplying this software for use with Cortex-M
 *           processor based microcontroller, but can be equally used for other
 *           suitable processor architectures. This file can be freely distributed.
 *           Modifications to this file shall be clearly marked.
 *           
 *           THIS SOFTWARE IS PROVIDED "AS IS". NO WARRANTIES, WHETHER EXPRESS, IMPLIED
 *           OR STATUTORY, INCLUDING, BUT NOT LIMITED TO, IMPLIED WARRANTIES OF
 *           MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE APPLY TO THIS SOFTWARE.
 *           ARM SHALL NOT, IN ANY CIRCUMSTANCES, BE LIABLE FOR SPECIAL, INCIDENTAL, OR
 *           CONSEQUENTIAL DAMAGES, FOR ANY REASON WHATSOEVER.

galak · 2018-11-21T10:40:26Z

Do you mind bring this PR up to date w/latest mater. I need to start a license review for the HAL since its not using a standard license. I don't see any issues w/it.

fabled · 2018-11-21T17:33:34Z

@galak The conflict is simple to resolve. But seems the SoC stuff resides in new directory. Could you point an example commit how to add a new soc / soc-family (+ soc-series if needed) on the new layout and what the Kconfig files should be named like?

galak · 2018-11-21T17:57:18Z

@galak The conflict is simple to resolve. But seems the SoC stuff resides in new directory. Could you point an example commit how to add a new soc / soc-family (+ soc-series if needed) on the new layout and what the Kconfig files should be named like?

Sure, pretty much we just moved arch/arm/soc/microchip_cec to soc/arm/microchip_cec. Take a look at:

commit 537798d
Author: Diego Sueiro [email protected]
Date: Wed Oct 17 07:58:14 2018 +0100

soc: arm: exx32: Add Silabs EFR32MG12P soc files

fabled · 2018-11-21T18:15:05Z

@galak Mechanical rebase done. Builds again the hello sample. Unfortunately, have not boot tested yet.

galak · 2018-11-21T18:22:42Z

@galak Mechanical rebase done. Builds again the hello sample. Unfortunately, have not boot tested yet.

thanks, I submitted the license for review, with the Thanksgiving Holiday in the US its going to delay things a bit.

galak

So the whole CONFIG_UART_NS16550_PORT_0_BRD_MASK needs to be changed.

I don't see this used anywhere
If we need this, do it for the more specific feature you need (ie alternate clock source)

galak

Some updates to the board code needed.

galak · 2018-11-21T18:29:14Z

boards/arm/secureiot1702/board.h

remove board.h

galak · 2018-11-21T18:29:36Z

boards/arm/secureiot1702/dts_fixup.h

clock frequency should be a property in the .dts file

galak

The qmspi change seems to be missing a dts binding file. Also I'd like to pull that PR out so its get seen by the SPI guys.

zephyrbot · 2018-11-21T18:32:08Z

Found the following issues, please fix and resubmit:

License/Copyright issues

ext/hal/microchip/cec/MCHP_CEC1702_C0.h is not apache-2.0 licensed: arm-cortex-mx
ext/hal/microchip/cec/MCHP_CEC1702_C0.h has non-permissive license: arm-cortex-mx
ext/hal/microchip/cec/MCHP_CEC1702_C0.h missing copyright.

galak · 2018-11-21T18:32:38Z

@tbursztyka Can you look at the SPI driver in this PR.

fabled · 2018-11-21T18:57:41Z

Fixed bunch of the review issues. Regarding CONFIG_UART_NS16550_PORT_0_BRD_MASK, it is used to select alternate clock (high speeds like 1Mbaud). It is probably specific to CEC NS16550-like block. It is sort of "ugly" and I would like to do it properly, but am not sure how to name it or make the logic. Ideally the driver would automatically set the bit when high speed is supported. However, that is bundled with the CLK_FREQ.

Perhaps, I add a CONFIG_UART_NS16550_CEC_CLOCK_SELECT, and it will set the correct bit if CLK_FREQ >= 10Mhz or similar? Or even just add that to happen automatically if SOC_SERIES_CEC is defined?

It is SOC / chip implementation specific feature, and does not always correspond to the IO access method. Signed-off-by: Timo Teräs <[email protected]>

Add the Microchip CEC1702 HAL sources from CEC1702 SDK. Origin: Microchip CEC1702 SDK URL: http://www.microchip.com/SWLibraryWeb/product.aspx?product=CEC1702_SDK Version: B0 build 0300 Purpose: Provides HAL support to Microchip CEC1702 Maintained-by: External Signed-off-by: Timo Teräs <[email protected]>

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. 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]>

This is the Quad Master-only SPI host driver. Signed-off-by: Timo Teräs <[email protected]>

fabled · 2018-12-21T14:38:30Z

@galak Ping. Reworked The NS16550 UART alternate clock select to have SoC specific block in the uart driver. That simplifies things quite a bit. Let me know if would have a better approach. I hope we could get this merged in soonish. It's been pending a long time...

dbkinder

Documentation is better, but still missing Flashing and Debugging section content.

dbkinder · 2018-12-28T22:31:56Z

@nashif @galak What should we do with PRs that are a year old and still open?

fabled · 2018-12-29T07:00:09Z

@dbkinder

Documentation is better, but still missing Flashing and Debugging section content.

I will work out the Flashing section. The vendor provider stuff is all Windows -based. Getting it done in generic way is doable, but may need some extras. Any way it boots from external SPI chip, so the thing is to have SPI flasher. What comes to Debugging, I don't have much experience as I never used hardware debugger. The SoC and board has standard JTAG support, but I basically never used it. So It is not really possible for me anything else that "Standard JTAG supported." or similar. Would that be sufficient for the time being?

What should we do with PRs that are a year old and still open?

Much of the time has been waiting review - or scheduling some cycles to this. Howevre, I am still happy to do requested changes as possible. I really hope we could get this to acceptable state so others can start building on it. I have had some private questions asked on this. There is now another demo board built on this SoC, and there is also four new SoCs (MEC1701, MEC1703, MEC1704 and MEC1705) which are of same core/family.

nashif · 2019-06-06T19:44:36Z

closing as stale

fabled requested review from nashif and SebastianBoe as code owners December 13, 2017 17:58

galak added area: ARM ARM (32-bit) Architecture area: Boards labels Dec 13, 2017

galak requested a review from dbkinder December 13, 2017 23:44

galak requested changes Dec 13, 2017

View reviewed changes

galak requested review from MaureenHelm and removed request for dbkinder December 13, 2017 23:47

galak self-assigned this Dec 13, 2017

SebastianBoe removed their request for review December 14, 2017 08:27

MaureenHelm requested changes Dec 14, 2017

View reviewed changes

fabled force-pushed the cec1702 branch from 4d09a30 to 7329030 Compare December 15, 2017 20:51

fabled requested a review from dbkinder as a code owner December 15, 2017 20:51

fabled changed the title ~~minimal support for microchip cec1702~~ Initial support for microchip cec1702 Dec 18, 2017

dbkinder suggested changes Dec 18, 2017

View reviewed changes

fabled force-pushed the cec1702 branch from 7329030 to e3fe4fc Compare December 20, 2017 17:31

fabled requested a review from tbursztyka as a code owner December 20, 2017 17:31

fabled force-pushed the cec1702 branch 4 times, most recently from f753b77 to fd8e4a6 Compare December 27, 2017 11:23

fabled force-pushed the cec1702 branch from fd8e4a6 to 31802ca Compare January 13, 2018 21:07

fabled force-pushed the cec1702 branch from c85804d to e8b165d Compare November 21, 2018 18:14

galak requested changes Nov 21, 2018

View reviewed changes

fabled force-pushed the cec1702 branch from e8b165d to d58874b Compare November 21, 2018 18:53

fabled added 6 commits December 21, 2018 15:50

serial/uart_ns16550: make UART_REG_ADDR_INTERVAL configurable

c1e41d4

It is SOC / chip implementation specific feature, and does not always correspond to the IO access method. Signed-off-by: Timo Teräs <[email protected]>

boards: Add support for SecureIoT1702

faca4e6

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]>

serial/uart_ns16550: support cec1702 alternate clock

d3963c6

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]>

arm: cec: Implement CEC QMSPI host controller support

efdb477

This is the Quad Master-only SPI host driver. Signed-off-by: Timo Teräs <[email protected]>

fabled force-pushed the cec1702 branch from d58874b to efdb477 Compare December 21, 2018 14:36

dbkinder suggested changes Dec 28, 2018

View reviewed changes

fabled mentioned this pull request Feb 19, 2019

Add CEC1702 headers MicrochipTech/Peripheral-mec1701#3

Open

nashif added the Stale PR label Jun 6, 2019

nashif closed this Jun 6, 2019

fabled mentioned this pull request Jun 7, 2019

Initial support for microchip cec1702 #16669

Closed

Initial support for microchip cec1702 #5383

Initial support for microchip cec1702 #5383

Uh oh!

Conversation

fabled commented Dec 13, 2017

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

fabled commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabled commented Dec 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galak commented Jan 13, 2018

Uh oh!

fabled commented Jan 13, 2018

Uh oh!

codecov-io commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fabled commented Nov 17, 2018

Uh oh!

galak commented Nov 21, 2018

Uh oh!

fabled commented Nov 21, 2018

Uh oh!

galak commented Nov 21, 2018

Uh oh!

fabled commented Nov 21, 2018

Uh oh!

galak commented Nov 21, 2018

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

zephyrbot commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License/Copyright issues

codecov-io commented Jan 13, 2018 •

edited

Loading

zephyrbot commented Nov 21, 2018 •

edited

Loading

fabled commented Nov 21, 2018 •

edited

Loading