Skip to content
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

boards/nucleo-l152: configure LSI by default #8545

Merged
merged 1 commit into from
Mar 1, 2018

Conversation

kYc0o
Copy link
Contributor

@kYc0o kYc0o commented Feb 9, 2018

Contribution description

Some nucleo-l152 boards, namely revision C-01, don't have a LSE to rely on for low speed clocking, which is mostly used for RTC.

This PR makes it optional, and define LSI by default.

Issues/PRs references

Fixes #8024 and fixes #8240, depends on #8518 to fix everything.

@kYc0o kYc0o added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: ARM Platform: This PR/issue effects ARM-based platforms CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 9, 2018
@kYc0o kYc0o added this to the Release 2018.04 milestone Feb 9, 2018
@kYc0o kYc0o added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 9, 2018
@aabadie aabadie removed the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 12, 2018
@aabadie
Copy link
Contributor

aabadie commented Feb 12, 2018

I don't think this PR fully fix #8024 and #8240, because it doesn't work when using LSE when the board has an external oscillator.

@kYc0o
Copy link
Contributor Author

kYc0o commented Feb 12, 2018

I think your problem is not related to the LSE in itself, but maybe something else. This PR makes LSI the default Low Speed clock source for the nucleo-l152 under any revision. If for some reason LSE breaks the board it needs to be investigated.

The real problem in #8024 was what it was fixed in #8518, this is only complementary to make work the nucleo-l152 by default (and not related to the stm32l152 CPU, which was affected by #7687).

I can't provide a fix for LSE (I don't even know what's the problem) since I don't have a board with it.

* 1: external crystal available (always 32.768kHz)
*
* LSE might not be available by default in early (C-01) Nucleo boards.
* If you're sure it is present, define CLOCK_LSE=1 in your project
Copy link
Contributor

Choose a reason for hiding this comment

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

If someone with an C-03 board revision follows what is described here, it will result in a failing board (e.g. no stdio for example).
I suggest removing this line until we have a solution with LSE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I'm still not completely convinced by this. I think that's obvious that if your board is not working after you set CLOCK_LSE=1 the reason is obviously that LSE is not working properly, so this message still make sense.

Besides, the fact that this board is broken with CLOCK_LSE is a bug, which should be documented somewhere e.g. an issue. Afterwards, of course it needs to be fixed, and it's out of the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but this is not what this comment is saying: 'If you're sure it is present' doesn't mean LSE might be broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I can add such information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you open an issue for this BTW?

@aabadie
Copy link
Contributor

aabadie commented Feb 13, 2018

BTW, with the current change nucleo-l152 works like a charm.

@kYc0o
Copy link
Contributor Author

kYc0o commented Feb 28, 2018

Changed issue description and directly amended.

*
* LSE might not be available by default in early (C-01) Nucleo boards.
* For newer revisions, LSE crystal is present, but currently is not working.
* (issue #8545).
Copy link
Contributor

@aabadie aabadie Mar 1, 2018

Choose a reason for hiding this comment

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

@kYc0o, use the full url, e.g. https://github.com/RIOT-OS/RIOT/pull/8545

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's evident that the comment it's in the context of a github issue. Though I've put the complete address and amended directly the change.

@kYc0o kYc0o force-pushed the nucleo-l152_lse branch from 7eafff1 to 6a32f1d Compare March 1, 2018 11:17
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

ACK !

@aabadie aabadie merged commit 18f9024 into RIOT-OS:master Mar 1, 2018
@kYc0o kYc0o deleted the nucleo-l152_lse branch March 2, 2018 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

periph/rtc breaks nucleo-l152 boards/cpu: nucleo-l152 (stm32l1) broken on master
2 participants