Skip to content

Conversation

@evgeniy-paltsev
Copy link
Contributor

@evgeniy-paltsev evgeniy-paltsev commented May 4, 2022

Pinmux is deprecated (see #39740) so let's get rid of it's usage for HSDK board.

As we call pinmux only once at init phase we simply do register setup in platform code instead of pinctrl.

@github-actions github-actions bot added area: Boards area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding labels May 4, 2022
@evgeniy-paltsev evgeniy-paltsev mentioned this pull request May 4, 2022
29 tasks
@evgeniy-paltsev evgeniy-paltsev added the area: ARC ARC Architecture label May 4, 2022
@evgeniy-paltsev evgeniy-paltsev marked this pull request as ready for review May 4, 2022 14:05
gmarull
gmarull previously requested changes May 4, 2022
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

Where is the pinctrl driver?

@evgeniy-paltsev
Copy link
Contributor Author

Hi @gmarull

Where is the pinctrl driver?

We don't need it - we used to use pinmux driver just to unconditionally set one SoC-specific register once during boot. But it's excessive to use pinmux / pinctrl for that. So we simply set this register from platform code and drop old pinmux driver.

@evgeniy-paltsev evgeniy-paltsev requested a review from gmarull May 4, 2022 19:58
@gmarull
Copy link
Member

gmarull commented May 5, 2022

Hi @gmarull

Where is the pinctrl driver?

We don't need it - we used to use pinmux driver just to unconditionally set one SoC-specific register once during boot. But it's excessive to use pinmux / pinctrl for that. So we simply set this register from platform code and drop old pinmux driver.

To my understanding current pinmux driver allows to set alt funcs, it's not "one Soc-specific register",

static int pinmux_hsdk_set(const struct device *dev, uint32_t pin,
uint32_t func)
{
if (func >= HSDK_PINMUX_FUNS || pin >= HSDK_PINMUX_SELS)
return -EINVAL;
creg_gpio_mux_reg &= ~(0x07U << (pin * 3));
creg_gpio_mux_reg |= (func << (pin * 3));
_arc_sync();
return 0;
}

/*
* to do configuration for each sel,
* please refer the doc for hsdk board.
*/
pinmux_pin_set(pinmux, HSDK_PINMUX_SEL0, HSDK_PINMUX_FUN0);
pinmux_pin_set(pinmux, HSDK_PINMUX_SEL1, HSDK_PINMUX_FUN0);
pinmux_pin_set(pinmux, HSDK_PINMUX_SEL2, HSDK_PINMUX_FUN0);
pinmux_pin_set(pinmux, HSDK_PINMUX_SEL3, HSDK_PINMUX_FUN2);
pinmux_pin_set(pinmux, HSDK_PINMUX_SEL4, HSDK_PINMUX_FUN0);
pinmux_pin_set(pinmux, HSDK_PINMUX_SEL5, HSDK_PINMUX_FUN0);
pinmux_pin_set(pinmux, HSDK_PINMUX_SEL6, HSDK_PINMUX_FUN0);
pinmux_pin_set(pinmux, HSDK_PINMUX_SEL7, HSDK_PINMUX_FUN0);

This functionality is now removed. Note that it is not acceptable anymore to do this sort of configuration at board level using custom C code.

@ruuddw
Copy link
Member

ruuddw commented May 5, 2022

The more advanced pinmux functionality isn't required/used by 'normal' applications, so this was the simpler solution. Evgeniy, maybe create an enhancement issue to add pinmux functionality for HSDK in the future using the new pinmux solution?

ruuddw
ruuddw previously approved these changes May 5, 2022
@gmarull
Copy link
Member

gmarull commented May 5, 2022

The more advanced pinmux functionality isn't required/used by 'normal' applications,

what are 'normal' applications?

Does that mean boards using this SoC are always forced to route signals to the same set of pins?

@codecov-commenter
Copy link

Codecov Report

Merging #45343 (dfc4c3f) into main (7c641e9) will decrease coverage by 0.01%.
The diff coverage is 49.60%.

❗ Current head dfc4c3f differs from pull request most recent head c941d86. Consider uploading reports for the commit c941d86 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #45343      +/-   ##
==========================================
- Coverage   49.89%   49.88%   -0.02%     
==========================================
  Files         643      643              
  Lines       80626    80677      +51     
  Branches    18933    18952      +19     
==========================================
+ Hits        40230    40246      +16     
- Misses      33733    33763      +30     
- Partials     6663     6668       +5     
Impacted Files Coverage Δ
drivers/counter/counter_cmos.c 82.97% <ø> (ø)
include/zephyr/bluetooth/gatt.h 100.00% <ø> (ø)
include/zephyr/drivers/adc.h 93.33% <ø> (ø)
kernel/sched.c 88.71% <ø> (ø)
subsys/bluetooth/host/att.c 0.00% <ø> (ø)
subsys/bluetooth/host/gatt.c 8.56% <0.00%> (ø)
subsys/net/lib/lwm2m/lwm2m_engine.c 10.85% <0.00%> (-0.04%) ⬇️
subsys/net/lib/lwm2m/lwm2m_rd_client.c 1.98% <7.14%> (+0.35%) ⬆️
kernel/timer.c 86.40% <50.00%> (-0.73%) ⬇️
subsys/net/lib/lwm2m/lwm2m_obj_server.c 51.28% <50.00%> (ø)
... and 6 more

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 7c641e9...c941d86. Read the comment docs.

abrodkin
abrodkin previously approved these changes May 5, 2022
@evgeniy-paltsev evgeniy-paltsev dismissed stale reviews from abrodkin and ruuddw via acb6374 May 9, 2022 11:26
@evgeniy-paltsev evgeniy-paltsev force-pushed the rff-hsdk-pinmux-remove-v0 branch from c941d86 to acb6374 Compare May 9, 2022 11:26
@evgeniy-paltsev
Copy link
Contributor Author

Rebase to fix conflicts. @abrodkin @ruuddw - please approve again.

@evgeniy-paltsev
Copy link
Contributor Author

Does that mean boards using this SoC are always forced to route signals to the same set of pins?

Currently we don't support other configurations, so yes.

@evgeniy-paltsev evgeniy-paltsev requested a review from abrodkin May 9, 2022 11:30
@evgeniy-paltsev evgeniy-paltsev requested a review from ruuddw May 9, 2022 11:30
ruuddw
ruuddw previously approved these changes May 9, 2022
abrodkin
abrodkin previously approved these changes May 9, 2022
Pinmux is deprecated (see zephyrproject-rtos#39740) so let's get rid of
it's usage for HSDK board.

As we call pinmux only once at init phase we simply do
register setup in platform code instead of pinmux.

Signed-off-by: Evgeniy Paltsev <[email protected]>
Signed-off-by: Eugeniy Paltsev <[email protected]>
Pinmux is depricated (see zephyrproject-rtos#39740) and shouldn't be used anymore

Signed-off-by: Evgeniy Paltsev <[email protected]>
Signed-off-by: Eugeniy Paltsev <[email protected]>
@evgeniy-paltsev evgeniy-paltsev dismissed stale reviews from abrodkin and ruuddw via b08e559 May 10, 2022 13:53
@evgeniy-paltsev evgeniy-paltsev force-pushed the rff-hsdk-pinmux-remove-v0 branch from acb6374 to b08e559 Compare May 10, 2022 13:53
@evgeniy-paltsev
Copy link
Contributor Author

Rebase to fix merge conflicts. @abrodkin @ruuddw - please approve again.

@evgeniy-paltsev
Copy link
Contributor Author

Hi @gmarull
I am wondering if you stilll have any objections or we can merge this?

Thanks!

@evgeniy-paltsev evgeniy-paltsev added this to the v3.1.0 milestone May 12, 2022
@evgeniy-paltsev
Copy link
Contributor Author

If there is no disagreement anymore, let's merge this.

Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

I think this is acceptable for now, but a proper pinctrl driver should be added going forward. Please open a enhancement request issue for this when merged.

@gmarull gmarull dismissed their stale review May 18, 2022 08:09

no pinctrl code to be reviewed

@carlescufi carlescufi changed the title ARC: boards: HSDK: ged rid of pinmux usage ARC: boards: HSDK: get rid of pinmux usage May 18, 2022
@carlescufi carlescufi merged commit 3b0517b into zephyrproject-rtos:main May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants