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

Add Support for TOSHIBA TMPM066 board #4840

Merged
merged 7 commits into from Aug 24, 2017

Conversation

Projects
None yet
6 participants
@ganesh-ramachandran
Contributor

ganesh-ramachandran commented Aug 1, 2017

Description

Add mbed support for TOSHIBA's TMPM066 board.
DAPLink PR: ARMmbed/DAPLink#295

Status

READY

Migrations

NO

Test Status

Pass: Mbed SDK automated test suite and greentea with ARM, GCC_ARM and IAR toolchains.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 2, 2017

@ganesh-ramachandran Hello, thanks for your contribution. It would be good to clean the git history as the last 3 commits are identical. The first two also but I assume they are adding different bits.

we recommend our contributors follow Chris Beam’s seven rules of great commit messages to keep the commit history clear. We find the commit.template feature particularly helpful.

Pass: Mbed SDK automated test suite and greentea with ARM, GCC_ARM and IAR toolchains.

Would it be possible to share them here (paste or attach txt file with the test results) .

@ganesh-ramachandran

This comment has been minimized.

Contributor

ganesh-ramachandran commented Aug 3, 2017

@0xc0170 Hello, Thank you for your inputs. Cleaned the git history. Please find below the test results attached.

Pass: Mbed SDK automated test suite and greentea with ARM, GCC_ARM and IAR toolchains.

Would it be possible to share them here (paste or attach txt file with the test results) .

M066_SingleAutomated_TestResults.txt
M066_GreenteaAutomated_TestResults.txt

targets/TARGET_TOSHIBA/mbed_rtx.h Outdated
#define INITIAL_SP (0x20004000UL)
#endif
#ifdef TOOLCHAIN_GCC_ARM

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

why is this main stack dependant on toolchain?

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 4, 2017

Contributor

GCC tool chain usually generates bigger code than other tool chains. Because of this few of the test cases need more memory on the Heap. That's why, we have reduced mbed main stack size to 2KB from 4KB for our target.

This comment has been minimized.

@0xc0170

0xc0170 Aug 9, 2017

Member

I believe it should not be tool dependent. neither the library should change this. This is an application specific, therefore I would recommend to remove it

cc @c1728p9 @bulislaw

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 10, 2017

Contributor

Removed the main stack updated for GCC tool chain. However, on removal two test suites of greentea fails due to out of memory. Please find attached the log.

M066_GCC_ARM_GreenTea_fail.txt

This comment has been minimized.

@0xc0170

0xc0170 Aug 10, 2017

Member

I cant spot from the logs why it timeouts ? What is the reason for the failure? not enough memory? We got targets that are quite low with the memory, and haven't seen this errors.

What is the memory for this target - flash/ram, how much is left for these 2 tests?

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 11, 2017

Contributor

Not enough memory is the message from the log - "Operator new[] out of memory" - Line numbers 191 & 289 from the attached log "M066_GCC_ARM_GreenTea_MainStack_4K.txt". Did not face this memory issue with ARM and IAR.

M066 Memory : RAM - 16K and Flash - 128K.

Attached below the GCC_ARM logs with 4K and 2K main stack size.
M066_GCC_ARM_GreenTea_MainStack_2K.txt
M066_GCC_ARM_GreenTea_MainStack_4K.txt

This comment has been minimized.

@0xc0170

0xc0170 Aug 14, 2017

Member

Can you create a separate issue for this? If 16K is available, it should be sufficient for this test. we got targets with the same numbers that do not cause the issues.

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 14, 2017

Contributor

Separate issue #4901 created

targets/TARGET_TOSHIBA/TARGET_TMPM066/us_ticker.c Outdated
static volatile uint32_t acc_us_ticker = 0;
// 16Bb high timer counter
static uint32_t us_ticker_16h = 0;

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

should be volatile, as its acc_us_ticker

targets/TARGET_TOSHIBA/TARGET_TMPM066/us_ticker.c Outdated
NVIC_EnableIRQ(INT16A0_IRQn);
// Match counter set to max value
TSB_T16A0->RG = TMR16A_100US;
// TSB_T16A0->CP = 0x00;

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

please remove any dead code

targets/TARGET_TOSHIBA/TARGET_TMPM066/serial_api.c Outdated
pinmap_pinout(tx, PinMap_UART_TX);
}
void serial_set_flow_control(serial_t *obj, FlowControl type, PinName rxflow, PinName txflow)

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

This can be removed, and SERIAL_FC disabled

obj->myi2c.PrescalerClkDiv = clk.prsck;
I2C_Init(obj->i2c, &obj->myi2c);
NVIC_DisableIRQ(obj->IRQn);

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

why is this disabling IRQ (related to slave functionality that operates with IRQs) ?

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 4, 2017

Contributor

Currently, the I2C implementation is based on polling. Hence disabled IRQ.

targets/TARGET_TOSHIBA/TARGET_TMPM066/i2c_api.c Outdated
#endif
void INTI2C0_IRQHandler(void)

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

it's recommended to use nvic set /get vectors to register own handlers, than using weak symbols.

Why are these empty?

targets/TARGET_TOSHIBA/TARGET_TMPM066/gpio_irq_api.c Outdated
obj->irq_id = pinmap_peripheral(pin, PinMap_GPIO_IRQ);
// Disable interrupt by CPU
__set_PRIMASK(1);

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

please use critical section API

targets/TARGET_TOSHIBA/TARGET_TMPM066/gpio_irq_api.c Outdated
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include "string.h"

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

why is string header included here?

targets/TARGET_TOSHIBA/TARGET_TMPM066/gpio_api.c Outdated
}
}
int gpio_read (gpio_t *obj)

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

one less space around gpio_read

@0xc0170 0xc0170 added needs: work and removed needs: review labels Aug 3, 2017

@ganesh-ramachandran

This comment has been minimized.

Contributor

ganesh-ramachandran commented Aug 4, 2017

@0xc0170 Hello, Thanks for your review comments. We have addressed the comments and committed the changes. Also checked for additional format changes.

// Enter stop1 mode
__WFI();
// Switch over from IHOSC to EHOSC
external_losc_enable();

This comment has been minimized.

@0xc0170

0xc0170 Aug 4, 2017

Member

Out of curiosity, how long it takes to switch from IHOSC to EHOSC ? What is thte stop1 power consumptions and wake up time?

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 7, 2017

Contributor

Switching from Internal Oscillator to External Oscillator takes as WarmUpTime (CG_WUODR_INT).
600mW is the peak power consumption at T = 85degC.
When using IOSC 10MHz,<WUP[15:0]> = 0x2700
Warm-up time = (0x2700 + 0x10) × (1/10 MHz)
= 0x2710 × 100ns
= 10000 × 100ns
= 1.0 ms
When using EOSC 16MHz,<WUPT[15:0]> = 0x3E70
Warm-up time = (0x3E70 + 0x10) × (1/16 MHz)
= 0x3E80 × 62.5 ns
= 16000 × 62.5 ns
= 1.0 ms

targets/TARGET_TOSHIBA/TARGET_TMPM066/analogin_api.c Outdated
// Set pin function as ADC
pinmap_pinout(pin, PinMap_ADC);
// Software reset ADC
ADC_SWReset();

This comment has been minimized.

@0xc0170

0xc0170 Aug 4, 2017

Member

What does this reset do? Can it conflict with some already configured ADC pins?

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 7, 2017

Contributor

A software reset will initializes all the registers again (even pre-configured ADC settings), except clock register. This is fixed to avoid conflict with some already configured ADC pins and committed the changes.

targets/TARGET_TOSHIBA/TARGET_TMPM066/us_ticker.c Outdated
if (!us_ticker_inited) {
us_ticker_init();
}
ret_val = (((acc_us_ticker << 16) + us_ticker_16h) * 100);

This comment has been minimized.

@0xc0170

0xc0170 Aug 7, 2017

Member

How this read() works? There is not hw counter used, only sw counters? When accessing this variable, shouldn't you protect it via at least disabling ticker interrupt (while this is accessing this variable, interrupt might happen and plus check again if there is pending, reread those sw counters to be up to date) ?

How much does this ticker ticks (interrupt invoked)? I am reading this as it ticks every 1us (16b counter) ?

This comment has been minimized.

@soramame21

soramame21 Aug 9, 2017

Contributor

Assume a single tick is 100us, the acc_us_ticker (lower 16bits) will overflow in 119.3 hours after initializing us_ticker. Is it the intended behavior?

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 9, 2017

Contributor

@0xc0170, Thank you for your input. Yes, we have used only sw counter. Now we have handled to check again the variable, if interrupt happens while accessing. Ticker ticks for every 100 us.

@soramame21, Thank you for your input. Overflow has been handled now using 32 bit value.

targets/TARGET_TOSHIBA/TARGET_TMPM066/us_ticker.c Outdated
@@ -98,5 +110,5 @@ void us_ticker_disable_interrupt(void)
void us_ticker_clear_interrupt(void)
{
//no flags to clear
NVIC_ClearPendingIRQ(INT16A1_IRQn);

This comment has been minimized.

@0xc0170

0xc0170 Aug 9, 2017

Member

I believe this should be part of fire interrupt now, clear can stay empty as it was?

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 9, 2017

Contributor

Yes, thank you. Made empty as it was.

targets/TARGET_TOSHIBA/TARGET_TMPM066/us_ticker.c Outdated
// Stops and clear count operation
TSB_T16A1->RUN = TMR16A_STOP;
TSB_T16A1->CR = TMR16A_SYSCK;
// Set the compare register
delta = (int)(timestamp - us_ticker_read());
if (delta < 0) {

This comment has been minimized.

@0xc0170

0xc0170 Aug 9, 2017

Member

This should not happen (it is handled in the upper layer, if it is, then fire interrupt is invoked, not set interrupt), will be documented in tickers API once we do update soon. Plus even if this would handle, should use set pending bit, rather than invoking handler directly (might cause stack overflow)

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 9, 2017

Contributor

Thank you for the update. Removed the check.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 10, 2017

@0xc0170 Could you review again?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 14, 2017

/morph test

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 14, 2017

/morph export-build

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 14, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 14, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph export-build

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 14, 2017

@ganesh-ramachandran It looks like you have conflicts in targets/targets.json. Could you rebase to resolve those conflicts? We can continue testing afterwards.

@ganesh-ramachandran

This comment has been minimized.

Contributor

ganesh-ramachandran commented Aug 16, 2017

@theotherjimmy Rebased with master and resolved the conflict.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

Rebased with master and resolved the conflict.

Thanks! But I'm questioning if it was a merge or a rebase. You have a commit, Resolved merge conflict, that indicates that you may have done a merge (the default action of git pull). Generally, a rebase to resolve a conflict will not create a commit, and it will change the dates of all of the commits you rebased to be after the date of my "could you rebase" comment above.

Is the aforementioned commit a merge commit? Could you do git fetch armmbed then git rebase armmbed/master (assuming your remote for this repo is armmbed to remove that merge commit ?

ganesh-ramachandran added some commits Aug 10, 2017

Issue with mbed compile for GCC_ARM tool resolved
Functions 'serial_break_set' & 'serial_break_clear' wrongly removed during 'SERIAL_FC disabled, critical section API Updation' commit.
Due to this 'mbed compile' command for GCC_ARM tool fails to compile.
@ganesh-ramachandran

This comment has been minimized.

Contributor

ganesh-ramachandran commented Aug 17, 2017

@theotherjimmy Yes, the previous one was merge commit. Got it about rebase. Now we have rebased.

@@ -239,3 +239,11 @@ void serial_pinout_tx(PinName tx)
{
pinmap_pinout(tx, PinMap_UART_TX);
}
void serial_break_set(serial_t *obj)

This comment has been minimized.

@0xc0170

0xc0170 Aug 17, 2017

Member

Can you implement these 2 functions? A question is why these 2 are empty

This comment has been minimized.

@ganesh-ramachandran

ganesh-ramachandran Aug 18, 2017

Contributor

@0xc0170 Line control register is not available in M066 controller. Due to this these 2 functions are not implemented and hence they are kept empty.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 17, 2017

/morph test

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Aug 17, 2017

/morph export-build

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 18, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph export-build

Output

mbed Build Number: 128

All exports and builds passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 18, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2017

/morph test

@0xc0170 0xc0170 added needs: CI and removed ready for merge labels Aug 21, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 21, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 21, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1063

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 22, 2017

@theotherjimmy theotherjimmy merged commit 0a063a0 into ARMmbed:master Aug 24, 2017

5 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-export-build Job has completed
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

exmachina-auto-deployer pushed a commit to exmachina-dev/mbed-os that referenced this pull request Sep 13, 2017

Merge tag 'mbed-os-5.5.6' of github.com:ARMmbed/mbed-os into 5.5.6
mbed OS 5.5.6 release

We are pleased to announce the [mbed OS 5.5.6
release](https://github.com/ARMmbed/mbed-os/releases/tag/mbed-os-5.5.6)
is now available.

This release includes ...

Known Issues

The following list of known issues apply to this release:

Contents

Ports for Upcoming Targets

[4608](ARMmbed#4608)
Support Nuvoton's new target NUMAKER_PFM_M487

[4840](ARMmbed#4840)
Add Support for TOSHIBA TMPM066 board

Fixes and Changes

[4801](ARMmbed#4801)
STM32 CAN: Fix issue with speed function calculation

[4808](ARMmbed#4808)
Make HAL & US tickers idle safe

[4812](ARMmbed#4812)
Use DSPI SDK driver API's in SPI HAL driver

[4832](ARMmbed#4832)
NUC472/M453: Fix several startup and hal bugs

[4842](ARMmbed#4842)
Add call to DAC_Enable as this is no longer done as part

[4849](ARMmbed#4849)
Allow using of malloc() for reserving the Nanostack's heap.

[4850](ARMmbed#4850)
Add list of defines to vscode exporter

[4863](ARMmbed#4863)
Optimize memory usage of wifi scan for REALTEK_RTW8195AM

[4869](ARMmbed#4869)
HAL LPCs SPI: Fix mask bits for SPI clock rate

[4873](ARMmbed#4873)
Fix Cortex-A cache file

[4878](ARMmbed#4878)
STM32 : Separate internal ADC channels with new pinmap

[4392](ARMmbed#4392)
Enhance memap, and configure depth level

[4895](ARMmbed#4895)
Turn on doxygen for DEVICE_* features

[4817](ARMmbed#4817)
Move RTX error handlers into RTX handler file

[4902](ARMmbed#4902)
Using CMSIS/RTX Exclusive access macro

[4923](ARMmbed#4923)
fix export static_files to zip

[4844](ARMmbed#4844)
bd: Add ProfilingBlockDevice for measuring higher-level applications

[4896](ARMmbed#4896)
target BLUEPILL_F103C8 compile fix

[4921](ARMmbed#4921)
Update gcc-arm-embedded PPA in Travis

[4926](ARMmbed#4926)
STM32L053x8: Refactor NUCLEO_L053R8 and DISCO_L053C8 targets

[4831](ARMmbed#4831)
Remove excessive use of printf/scanf in mbed_fdopen/_open

[4922](ARMmbed#4922)
bug fix: xdot clock config

[4935](ARMmbed#4935)
STM32: fix F410RB vectors size

[4940](ARMmbed#4940)
Update mbed-coap to version 4.0.9

[4941](ARMmbed#4941)
Update of MemoryPool.h header file.

You can fetch this release from the [mbed-os
GitHub](https://github.com/ARMmbed/mbed-os) repository,
using the tag "mbed-os-5.5.6".

Please feel free to ask any questions or provide feedback on this
release [on the forum](https://forums.mbed.com/),
or to contact us at [support@mbed.org](mailto:support@mbed.org).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment