Skip to content

realtek rtl8195am Add MCU_target #7704

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

Closed

Conversation

M-ichae-l
Copy link
Contributor

Description

1, edit "targets.json" to let "REALTEK_RTL8195AM" target inherit from "MCU_RTL8195A"

Pull request type

[X] Refactor

1, edit "targets.json" to let "REALTEK_RTL8195AM" target inherit from "MCU_RTL8195A"
@0xc0170
Copy link
Contributor

0xc0170 commented Aug 6, 2018

Isn't this also missing folder structure reorganization? To be specific, PinNames are platform specific so they should be in TARGET_RTL8195A for instance. Opposite to that, HAL implementation can be moved to MCU_RTL8195A

What I expect to see in TARGET_RTL8195A - peripheral names/pins and Pinnames plus any specific target setting.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 6, 2018

@M-ichae-l
Copy link
Contributor Author

M-ichae-l commented Aug 7, 2018

@0xc0170

Could you be more specific about the folder structure reorganization?

For the MCU_RTL8195A, will have "core", "default_toolchain", "macros" and "supported_toolchains", "post_binary_hook", "release_versions", "overrides".
For the REALTEK_RTL8195AM, will have "supported_form_factors", "detect_code", "extra_labels", "device_has".
SDT8195B can inherits MCU_RTL8195A with its own "detect_code", "extra_labels", "device_has", "features"

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 7, 2018

This basically creates only json changes but no folder tree.

If I create a new target named MY_NEW_TARGET that inherits from MCU_RTL8195A, what files would I get? Only SDK and wifi. But I would not have HAL implementation (this is related to SDK therefore my new target should have the same HAL implementation, is that correct?).

My assumption would be that this folder https://github.com/ARMmbed/mbed-os/tree/master/targets/TARGET_Realtek/TARGET_AMEBA/TARGET_RTL8195A needs to be renamed to MCU_RTL8195A, and new folder inside this one to be created named TARGET_RTL8195A (this one contains board specific configuration).

To Ilustrate the tree:

- targets/TARGET_Realtek/TARGET_AMEBA
  - TARGET_MCU_RTL8195A
    - TARGET_RTL8195A
    - TARGET_MY_NEW_TARGET

Then all I need to do for MY_NEW_TARGET would be to add board specific files (pinnames, peripheral names, board config like init different if I need one) inside TARGET_MY_NEW_TARGET and that is it !

1, edit "targets.json"
@cmonr cmonr requested a review from 0xc0170 August 8, 2018 03:10
@cmonr
Copy link
Contributor

cmonr commented Aug 8, 2018

@M-ichae-l Fyi, this still looks like it needs some work to pass Travis.

@M-ichae-l
Copy link
Contributor Author

M-ichae-l commented Aug 8, 2018

@0xc0170
Thank you!
Please refer to my branch https://github.com/M-ichae-l/mbed-os/tree/realtek-rtl8195am-Add-MCU_target-update/targets. Check if this update is OK.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 8, 2018

@M-ichae-l
Copy link
Contributor Author

M-ichae-l commented Aug 8, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 8, 2018

One last thing I can think of: https://github.com/M-ichae-l/mbed-os/blob/realtek-rtl8195am-Add-MCU_target-update/targets/TARGET_Realtek/TARGET_AMEBA/TARGET_MCU_RTL8195A/serial_api.c#L30 - this pinmap is board specific. Shall it be moved to out of the implementation and inside TARGET_RTL8195A.

Look at

The example is here https://github.com/M-ichae-l/mbed-os/blob/realtek-rtl8195am-Add-MCU_target-update/targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/TARGET_MTS_GAMBIT/PeripheralPins.c for instance.

@M-ichae-l
Copy link
Contributor Author

Yes, There are 4 files are pinmap board specific. i2c_api.c, pwmout_api.c, serial_api.c and spi_api.c. Sould they all move to TARGET_RTL8195A ?

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 8, 2018

The file serial_api.c is fine as it is - this is not board specific? The only code that it is are pins (each board can have different pinout). Therefore move those pins definitions to the TARGET_RTL8195A

TARGET_RTL8195A would have for instance PeripheralPins.c file that will contain:

const PinMap PinMap_UART_TX[] = {
    {PC_3,  RTL_PIN_PERI(UART0, 0, S0), RTL_PIN_FUNC(UART0, S0)},
    {PE_0,  RTL_PIN_PERI(UART0, 0, S1), RTL_PIN_FUNC(UART0, S1)},
    {PA_7,  RTL_PIN_PERI(UART0, 0, S2), RTL_PIN_FUNC(UART0, S2)},
    {PD_3,  RTL_PIN_PERI(UART1, 1, S0), RTL_PIN_FUNC(UART1, S0)},
    {PE_4,  RTL_PIN_PERI(UART1, 1, S1), RTL_PIN_FUNC(UART1, S1)},
    {PB_5,  RTL_PIN_PERI(UART1, 1, S2), RTL_PIN_FUNC(UART1, S2)},
    {PA_4,  RTL_PIN_PERI(UART2, 2, S0), RTL_PIN_FUNC(UART2, S0)},
    {PC_9,  RTL_PIN_PERI(UART2, 2, S1), RTL_PIN_FUNC(UART2, S1)},
    {PD_7,  RTL_PIN_PERI(UART2, 2, S2), RTL_PIN_FUNC(UART2, S2)},
    {PB_0,  RTL_PIN_PERI(LOG_UART, 3, S0), RTL_PIN_FUNC(LOG_UART, S0)},
    {NC,    NC,     0}
};

const PinMap PinMap_UART_RX[] = {
    {PC_0,  RTL_PIN_PERI(UART0, 0, S0), RTL_PIN_FUNC(UART0, S0)},
    {PE_3,  RTL_PIN_PERI(UART0, 0, S1), RTL_PIN_FUNC(UART0, S1)},
    {PA_6,  RTL_PIN_PERI(UART0, 0, S2), RTL_PIN_FUNC(UART0, S2)},
    {PD_0,  RTL_PIN_PERI(UART1, 1, S0), RTL_PIN_FUNC(UART1, S0)},
    {PE_7,  RTL_PIN_PERI(UART1, 1, S1), RTL_PIN_FUNC(UART1, S1)},
    {PB_4,  RTL_PIN_PERI(UART1, 1, S2), RTL_PIN_FUNC(UART1, S2)},
    {PA_0,  RTL_PIN_PERI(UART2, 2, S0), RTL_PIN_FUNC(UART2, S0)},
    {PC_6,  RTL_PIN_PERI(UART2, 2, S1), RTL_PIN_FUNC(UART2, S1)},
    {PD_4,  RTL_PIN_PERI(UART2, 2, S2), RTL_PIN_FUNC(UART2, S2)},
    {PB_1,  RTL_PIN_PERI(LOG_UART, 3, S0), RTL_PIN_FUNC(LOG_UART, S0)},
    {NC,    NC,     0}
};

and serial_api.c file would just pull it via header file (PeripheralPins.h that we would need to create in MCU_ folder - valid for all _api files) or use extern const PinMap PinMap_SPI_SCLK[]; at the top of the serial_api.c file.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 8, 2018

I've noticed this now that is in line what we are doing here https://github.com/ARMmbed/mbed-os-5-docs/pull/625/files#diff-1b25b067ef8a80bc82c45ca7e4809ec5R73 (docs update ongoing). There's the structure I am describing here!

@M-ichae-l
Copy link
Contributor Author

@cmonr
Copy link
Contributor

cmonr commented Aug 14, 2018

@ashok-rao @bentcooke Could I ask for y'alls help here? Looking at the branch's directory structure, the port looks ok, but this is the first time I've looked at the file structure for a port and could use some help.

@cmonr
Copy link
Contributor

cmonr commented Aug 14, 2018

@M-ichae-l Making sure, the links in your comment aren't yet pushed to this PR, correct?

@M-ichae-l
Copy link
Contributor Author

@cmonr
No, I have not pushed the PR yet.

@ashok-rao
Copy link
Contributor

@cmonr & @M-ichae-l : a minor comment regarding port_api.c, objects.h and device.h -> here ..Can these 3 be moved to the MCU level rather than the board level?

Rest all LGTM! Thanks.

Copy link
Contributor

@ashok-rao ashok-rao left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I've left 1 comment. Great if you could address this @M-ichae-l . Thanks!

@M-ichae-l
Copy link
Contributor Author

@ashok-rao @cmonr
I have moved the three files under MCU folder. I have tested with greentea there are no issues.
Please refer to https://github.com/M-ichae-l/mbed-os/tree/realtek-rtl8195am-Add-MCU_target-update/targets/TARGET_Realtek/TARGET_AMEBA/TARGET_MCU_RTL8195A

Copy link
Contributor

@ashok-rao ashok-rao left a comment

Choose a reason for hiding this comment

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

Thanks @M-ichae-l .. LGTM!

@M-ichae-l
Copy link
Contributor Author

Great! The #7715 is updating the Rtl8195am lib. It is related to the structure changes here. I suggest to create PR after #7715 is merged.

@bentcooke
Copy link
Contributor

@M-ichae-l could you please add the following entry to "MCU_RTL8195A"

"public": false,

This prevents the build system from allowing a user to build the target and is typically used for MCU definitions. Users target a board that inherits a MCU, but not the MCU by itself.

@M-ichae-l
Copy link
Contributor Author

@bentcooke
I have added the "public": false,. Thank you!

@cmonr
Copy link
Contributor

cmonr commented Aug 23, 2018

@M-ichae-l Is there a reason that this PR doesn't have the new files yet?

@M-ichae-l
Copy link
Contributor Author

@cmonr
I have a branch that has all new changes
https://github.com/M-ichae-l/mbed-os/tree/realtek-rtl8195am-Add-MCU_target-update/targets/TARGET_Realtek/TARGET_AMEBA/TARGET_MCU_RTL8195A
The branch is waiting for the #7715 to be merged. After it merged, I will create a PR base on the branch above.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 3, 2018

The branch is waiting for the #7715 to be merged. After it merged, I will create a PR base on the branch above.

As that one is in progress, can this be non dependent on it? How can we progress with this MCU target ?

We have now new target addition based on this MCU

@M-ichae-l
Copy link
Contributor Author

@0xc0170
I have created a PR just for lib update. #8014 After lib updated we can process for this MCU update.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 26, 2018

I have created a PR just for lib update. #8014 After lib updated we can process for this MCU update.

Integrated, rebase here or ?

"inherits": ["MCU_RTL8195A"],
"detect_code": ["4600"],
"extra_labels": ["Realtek", "AMEBA", "RTL8195A", "RTW_EMAC"],
"device_has": ["ANALOGIN", "ANALOGOUT", "I2C", "I2CSLAVE", "INTERRUPTIN", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "SERIAL", "SPI", "TRNG", "FLASH"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't MCU define these ? I imagine ANALOGIN supported for that MCU. If a new derived target does not implement analogin for specific reason, it could remove it. Same for extra_labels - Realtek is specific for this target or can be applied to MCU_ one ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"device_has" will move to the under MCU. So do "extra_labels". "extra_labels_add" will add to the specific target.

@M-ichae-l
Copy link
Contributor Author

@0xc0170
I have created a new PR #8266 It includes all updates.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 1, 2018

I have created a new PR #8266 It includes all updates.

I am closing this one.

@0xc0170 0xc0170 closed this Oct 1, 2018
@M-ichae-l M-ichae-l deleted the realtek-rtl8195am-Add-MCU_target branch October 26, 2018 03:20
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.

6 participants