July 2023 - flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[master]: flashchips: Add support for PUYA P25Q40H
by Anne Macedo (Code Review) 21 Jul '23

21 Jul '23

Attention is currently required from: Anastasia Klimchuk. Anne Macedo has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/76251?usp=email ) Change subject: flashchips: Add support for PUYA P25Q40H ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Anne, thank you for your patch! I have few comments. […] Thanks for the review! I'll make the changes as soon as I can. -- To view, visit https://review.coreboot.org/c/flashrom/+/76251?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I58c2c0467ef19f98b7435e84b6978550f029af70 Gerrit-Change-Number: 76251 Gerrit-PatchSet: 1 Gerrit-Owner: Anne Macedo <retpolanne(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 21 Jul 2023 10:37:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: doc: Add new Development guidelines
by Anastasia Klimchuk (Code Review) 20 Jul '23

20 Jul '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/75906?usp=email ) ( 7 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: doc: Add new Development guidelines ...................................................................... doc: Add new Development guidelines Change-Id: I7fe9ab2e27fead8e795138294219b11240f15928 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/75906 Reviewed-by: Alexander Goncharov <chat(a)joursoir.net> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M doc/contact.rst A doc/dev_guide/development_guide.rst M doc/dev_guide/index.rst 3 files changed, 346 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Alexander Goncharov: Looks good to me, but someone else must approve Peter Marheine: Looks good to me, approved diff --git a/doc/contact.rst b/doc/contact.rst index dc89063..220d73f 100644 --- a/doc/contact.rst +++ b/doc/contact.rst @@ -3,6 +3,8 @@ .. The extra = is needed to prevent git from throwing a `leftover conflict marker` error when commiting. +.. _mailing list: + Mailing List ------------ Flashrom related mails are welcome on the flashrom mailing list at `flashrom(a)flashrom.org <mailto:flashrom@flashrom.org>`_. diff --git a/doc/dev_guide/development_guide.rst b/doc/dev_guide/development_guide.rst new file mode 100644 index 0000000..b4887ef --- /dev/null +++ b/doc/dev_guide/development_guide.rst @@ -0,0 +1,343 @@ +================= +Development Guide +================= + +We welcome contributions from every human being, corporate entity or club. + +This document describes the rules and recommendations about the development, contribution and review processes. + +If you introduce new features (not flash chips, but stuff like partial +programming, support for new external programmers, voltage handling, etc) +please **discuss your plans** on the :ref:`mailing list` first. That way, we +can avoid duplicated work and know about how flashrom internals need to be +adjusted and you avoid frustration if there is some disagreement about the +design. + + +Set up the git repository and dev environment +============================================= + +#. Clone git repository + + * If using https: :code:`git clone "https://review.coreboot.org/flashrom"` + * If using ssh: :code:`git clone "ssh://<gerrit_username>@review.coreboot.org:29418/flashrom"` + +#. Follow the build guidelines to install dependencies :doc:`building_from_source` + +#. Install Git hooks: :code:`./util/git-hooks/install.sh` + +#. Add upstream as a remote: + + * If using https: :code:`git remote add -f upstream https://review.coreboot.org/flashrom` + * If using ssh: :code:`git remote add -f upstream ssh://<gerrit_username>@review.coreboot.org:29418/flashrom` + +#. Check out a new local branch that tracks :code:`upstream/master`: :code:`git checkout -b <branch_name> upstream/master` + +#. Every patch is required to be signed-off (see also :ref:`sign-off`). + Set up your ``user.name`` and ``user.email`` in git config, and don't forget + to ``-s`` when creating a commit. + +#. See also build guidelines :doc:`building_from_source` and `git docs <https://git-scm.com/doc>`_ + +Creating your patch +=================== + +In short, commit your changes with a descriptive message and remember to sign off +on the commit (``git commit -s``). + +.. _commit-message: + +Commit message +-------------- + +Commit messages shall have the following format: + +.. code-block:: + + <component>: Short description (up to 72 characters) + + This is a long description. Max width of each line in the description + is 72 characters. It is separated from the summary by a blank line. You + may skip the long description if the short description is sufficient, + for example "flashchips: Add FOO25Q128" to add FOO25Q128 chip support. + + You may have multiple paragraphs in the long description, but please + do not write a novel here. For non-trivial changes you must explain + what your patch does, why, and how it was tested. + + Finally, follow the sign-off procedure to add your sign-off! + + Signed-off-by: Your Name <your@domain> + +Commit message should include: + +* Commit title +* Commit description: explain what the patch is doing, or what it is fixing. +* Testing information: how did you test the patch. +* Signed-off-by line (see below :ref:`sign-off`) +* If applicable, link to the ticket in the bugtracker `<https://ticket.coreboot.org/projects/flashrom>`_ +* Change-Id for Gerrit. If commit message doesn't have Change-Id, you forgot to install git hooks. + +.. _sign-off: + +Sign-off procedure +^^^^^^^^^^^^^^^^^^ + +We employ a similar sign-off procedure as the `Linux kernel developers +<http://web.archive.org/web/20070306195036/http://osdlab.org/newsroom/press_…>`_ +do. Add a note such as + +.. code-block:: + + Signed-off-by: Random J Developer <random(a)developer.example.org> + +to your email/patch if you agree with the Developer's Certificate of Origin 1.1 +printed below. Read `this post on the LKML +<https://lkml.org/lkml/2004/5/23/10>`_ for rationale (spoiler: SCO). + +You must use your known identity in the ``Signed-off-by`` line and in any +copyright notices you add. Anonymous patches lack provenance and cannot be +committed! + +Developer's Certificate of Origin 1.1 +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + + By making a contribution to this project, I certify that: + + (a) The contribution was created in whole or in part by me and I have + the right to submit it under the open source license indicated in the file; or + + (b) The contribution is based upon previous work that, to the best of my + knowledge, is covered under an appropriate open source license and I have the + right under that license to submit that work with modifications, whether created + in whole or in part by me, under the same open source license (unless I am + permitted to submit under a different license), as indicated in the file; or + + (c) The contribution was provided directly to me by some other person who + certified (a), (b) or (c) and I have not modified it; and + + (d) In the case of each of (a), (b), or (c), I understand and agree that + this project and the contribution are public and that a record of the contribution + (including all personal information I submit with it, including my sign-off) is + maintained indefinitely and may be redistributed consistent with this project or the + open source license indicated in the file. + +.. note:: + + The `Developer's Certificate of Origin 1.1 + <http://web.archive.org/web/20070306195036/http://osdlab.org/newsroom/press_…>`_ + is licensed under the terms of the `Creative Commons Attribution-ShareAlike + 2.5 License <http://creativecommons.org/licenses/by-sa/2.5/>`_. + +Coding style +------------ + +Flashrom generally follows Linux kernel style: +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst + +The notable exception is line length limit. Our guidelines are: + +* 80-columns soft limit for most code and comments. This is to encourage simple design and concise naming. +* 112-columns hard limit. Use this to reduce line breaks in cases where they + harm grep-ability or overall readability, such as print statements and + function signatures. Don't abuse this for long variable/function names or + deep nesting. +* Tables are the only exception to the hard limit and may be as long as needed + for practical purposes. + +Our guidelines borrow heavily from `coreboot coding style +<https://doc.coreboot.org/contributing/coding_style.html>`_ and `coreboot Gerrit +guidelines <https://doc.coreboot.org/contributing/gerrit_guidelines.html>`_, +and most of them apply to flashrom as well. The really important part is about +the :ref:`sign-off procedure <sign-off>`. + +We try to **reuse as much code as possible** and create new files only if +absolutely needed, so if you find a function somewhere in the tree which +already does what you want, please use it. + +Testing a patch +--------------- + +We expect the patch to be appropriately tested by the patch owner. +Please add the testing information in commit message, for example that could be some of these: +programmer you were using, programmer params, chip, OS, operations you were running +(read/write/erase/verify), and anything else that is relevant. + +.. _working-with-gerrit: + +Working with Gerrit +=================== + +All of the patches and code reviews need to go via +`Gerrit on review.coreboot.org <https://review.coreboot.org/#/q/project:flashrom>`_. +While it is technically possible to send a patch to the mailing list, that patch +still needs to be pushed to Gerrit by someone. We treat patches on the mailing list as a very +exceptional situation. Normal process is to push a patch to Gerrit. +Please read below for instructions and check `official Gerrit documentation <https://gerrit-review.googlesource.com/Documentation/>`_. + +Creating an account +--------------------- + +#. Go to https://review.coreboot.org/login and sign in using the credentials of + your choice. +#. Edit your settings by clicking on the gear icon in the upper right corner. +#. Set your Gerrit username (this may be the different from the username of an + external account you log in with). +#. Add an e-mail address so that Gerrit can send notifications to you about + your patch. +#. Upload an SSH public key, or click the button to generate an HTTPS password. + +.. _pushing-a-patch: + +Pushing a patch +--------------- + +To push patch to Gerrit, use the follow command: :code:`git push upstream HEAD:refs/for/master`. + +* If using HTTPS you will be prompted for the username and password you + set in the Gerrit UI. +* If successful, the Gerrit URL for your patch will be shown in the output. + +There is an option to add a topic to the patch. For one-off standalone patches this +is not necessary. However if your patch is a part of a larger effort, especially if the +work involves multiple contributors, it can be useful to mark that the patch belongs +to a certain topic. + +Adding a topic makes it easy to search "all the patches by the topic", even if the patches +have been authored by multiple people. + +To add a topic, push with the command: :code:`git push upstream HEAD:refs/for/master%topic=example_topic`. +Alternatively, you can add a topic from a Gerrit UI after the patch in pushed +(on the top-left section) of patch UI. + +Adding reviewers to the patch +----------------------------- + +After pushing the patch, ideally try to make sure there are some reviewers added to your patch. + +flashrom has MAINTAINERS file with people registered for some areas of the code. People who +are in MAINTAINERS file will be automatically added as reviewers if the patch touches that +area. However, not all areas are covered in the file, and it is possible that for the patch you +sent no one is added automatically. + +If you know someone in the dev community who can help with patch review, add the person(s) you know. + +In general, it's a good idea to add someone who has a knowledge of whatever the patch is doing, +even if the person has not been added automatically. + +If you are new, and don't know anyone, and no one has been added automatically: you can add +Anastasia Klimchuk (aklm) as a reviewer. + +Going through code reviews +-------------------------- + +You will likely get some comments on your patch, and you will need to fix the comments. +After doing the work locally, amend your commit ``git commit --amend -s`` and push to Gerrit again. +Check that Change-Id in commit message stays the same. This way Gerrit knows your change belongs +to the same patch, and will upload new change as new patchset for the same patch. + +After uploading the work, go through comments and respond to them. Mark as Done the ones you done +and mark them as resolved. If there is something that is impossible to do, or maybe you have more questions, +or maybe you are not sure what you are asked about: respond to a comment **without marking it as resolved**. + +It is completely fine to ask a clarifying questions if you don't understand what the comment is asking you to do. +If is also fine to explain why a comment can't be done, if you think it can't be done. + +The patch reviews may take some time, but please don't get discouraged. +We have quite high standards regarding code quality. + +Initial review should include a broad indication of acceptance or rejection of +the idea/rationale/motivation or the implementation + +In general, reviews should focus on the architectural changes and things that +affect flashrom as a whole. This includes (but is by no means limited to) +changes in APIs and types, safety, portability, extensibility, and +maintainability. The purpose of reviews is not to create perfect patches, but +to steer development in the right direction and produce consensus within the +community. The goal of each patch should be to improve the state of the project +- it does not need to fix all problems of the respective field perfectly. + + New contributors may need more detailed advices and should be told about + minor issues like formatting problems more precisely. The result of a review + should either be an accepted patch or a guideline how the existing code + should be changed to be eventually accepted. + +To get an idea whether the patch is ready or not, please check :ref:`merge-checklist`. + +If you sent a patch and later lost interest or no longer have time to follow up on code review, +please add a comment saying so. Then, if any of our maintainers are interested in finishing the work, +they can take over the patch. + +Downloading patch from Gerrit +----------------------------- + +Sometimes you may need to download a patch into your local repository. This can be needed for example: + +* if you want to test someone else's patch, +* if multiple developers are collaborating on a patch, +* if you are continuing someone else's work, when original author left or unable to continue. + +First prepare local repository: sync to head or to desired tag / commit. + +Open patch in Gerrit, open "three dot" menu on top-right, open Download patch. Copy Cherry-pick command (pick +the relevant tab for you: anonymous http / http / ssh) and run the copied command in your local repo. + +Now you have the commit locally and can do the testing or futher developing. To upload your local changes, +push patch to Gerrit again (see :ref:`pushing-a-patch`). + +Make sure people involved in the patch agree that you are pushing new version of someone else's patch, +so this does not come at a surprise for an original author. + +Merging patches +--------------- + +Merging to branches is limited to the "flashrom developers" group on Gerrit (see also :doc:`/about_flashrom/team`). + +The list of requirements for the patch to be ready for merging is below, see :ref:`merge-checklist`. +Some of the requirements are enforced by Gerrit, but not all of them. In general, a person who clicks +Submit button is responsible to go through Merge checklist. Code reviewers should be aware of the checklist +as well. + +Patch owners can use the checklist to detect whether the patch is ready for merging or not. + +.. _merge-checklist: + +Merge checklist +^^^^^^^^^^^^^^^ + +#. Every patch has to be reviewed and needs at least one +2 that was not given by the commit's author. + Ideally, people who were actively reviewing the patch and adding comments, would be the ones approving it. +#. If a patch is authored by more than one person (Co-developed-by), each author may +2 the other author's changes. +#. Patch needs to get Verified +1 vote, typically from Jenkins build bot. This means the patch builds successfully + and all unit tests pass. +#. Commit message should have Signed-off-by line, see :ref:`sign-off` and align with the rest + of the rules for :ref:`commit-message` +#. All the comments need to be addressed, especially if there was a negative vote in the process of review (-1 or -2). +#. flashrom developers are people from literally all around the planet, and various timezones. We usually wait + for 3 days (3 * 24hours) after the patch is fully approved just in case of last minute concerns from all timezones. +#. In the case of emergency, merging should not take place within less than 24 hours after the review + started (i.e. the first message by a reviewer on Gerrit). + +To help search for patches which are potential candidates for merging, you can try using this search in Gerrit:: + + status:open project:flashrom -is:wip -label:Verified-1 label:Verified+1 -label:Code-Review<0 age:3d is:mergeable is:submittable -has:unresolved + +Note the search is not a replacement for Merge checklist, but it can help find candidates for merging. + +Bugtracker +========== + +We have a bugtracker on `<https://ticket.coreboot.org/projects/flashrom>`_. +Anyone can view tickets, but to be able to create/update/assign tickets you need an account. + +Mirrors +======== + +The only official repository is https://review.coreboot.org/flashrom ; GitHub and GitLab are just mirrors. +**Reviewers do not look at pull requests** on mirrors. +Even if pull requests were automatically transferred to Gerrit, +requirements such as :ref:`sign-off` still present a problem. + +The quickest and best way to get your patch reviewed and merged is by sending +it to review.coreboot.org (see :ref:`working-with-Gerrit`). Conveniently, you can use your GitHub, GitLab or +Google account as an OAuth2 `login method <https://review.coreboot.org/login>`_. diff --git a/doc/dev_guide/index.rst b/doc/dev_guide/index.rst index b540127..57cd846 100644 --- a/doc/dev_guide/index.rst +++ b/doc/dev_guide/index.rst @@ -6,3 +6,4 @@ building_from_source building_with_make + development_guide -- To view, visit https://review.coreboot.org/c/flashrom/+/75906?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7fe9ab2e27fead8e795138294219b11240f15928 Gerrit-Change-Number: 75906 Gerrit-PatchSet: 9 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-MessageType: merged

1 0

[L] Change in flashrom[master]: doc: Add new Development guidelines
by Anastasia Klimchuk (Code Review) 20 Jul '23

20 Jul '23

Attention is currently required from: Angel Pons, Edward O'Callaghan, Nikolai Artemiev, Stefan Reinauer, Swift Geek (Sebastian Grzywna), Thomas Heijligen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75906?usp=email ) Change subject: doc: Add new Development guidelines ...................................................................... Patch Set 8: (1 comment) File doc/dev_guide/development_guidelines.rst: https://review.coreboot.org/c/flashrom/+/75906/comment/1474a280_ecaf2735 : PS3, Line 299: :ref:`Team` > This page is under review as well, CB:76075 Done -- To view, visit https://review.coreboot.org/c/flashrom/+/75906?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7fe9ab2e27fead8e795138294219b11240f15928 Gerrit-Change-Number: 75906 Gerrit-PatchSet: 8 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com> Gerrit-Comment-Date: Thu, 20 Jul 2023 09:15:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: doc: Add code of conduct
by Patrick Georgi (Code Review) 20 Jul '23

20 Jul '23

Attention is currently required from: Alexander Goncharov, Anastasia Klimchuk, Angel Pons, Martin L Roth, Peter Marheine, Stefan Reinauer, Thomas Heijligen. Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/76455?usp=email ) Change subject: doc: Add code of conduct ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Seems fine, but I think we either need to get confirmation from the Coreboot arbitration team (liste […] Thanks for bringing that up! As one member of the coreboot arbitration team, I'd be okay with playing that role for flashrom _for now_. However, I'd consider that a bootstrapping measure with the eventual goal of the flashrom community finding their own team (that might have some overlap into coreboot but should certainly have their own folks, too). As such, my idea is that flashrom copies the Code of Conduct instead of merely linking over: the latter leads to "That's coreboot's job". It isn't. -- To view, visit https://review.coreboot.org/c/flashrom/+/76455?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic4c646bc47d5db4d064a411d60a44f16559dfcee Gerrit-Change-Number: 76455 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-CC: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 20 Jul 2023 06:11:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: doc: Add Team page which describes Gerrit groups
by Anastasia Klimchuk (Code Review) 19 Jul '23

19 Jul '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/76075?usp=email ) Change subject: doc: Add Team page which describes Gerrit groups ...................................................................... doc: Add Team page which describes Gerrit groups Change-Id: I3118b2b036eab93e901814447543b02c760c6a80 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/76075 Reviewed-by: Nikolai Artemiev <nartemiev(a)google.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Reviewed-by: Alexander Goncharov <chat(a)joursoir.net> Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> --- A doc/about_flashrom/index.rst A doc/about_flashrom/team.rst M doc/index.rst 3 files changed, 66 insertions(+), 0 deletions(-) Approvals: Nikolai Artemiev: Looks good to me, but someone else must approve Alexander Goncharov: Looks good to me, but someone else must approve build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved Peter Marheine: Looks good to me, but someone else must approve diff --git a/doc/about_flashrom/index.rst b/doc/about_flashrom/index.rst new file mode 100644 index 0000000..11d845f --- /dev/null +++ b/doc/about_flashrom/index.rst @@ -0,0 +1,4 @@ +.. toctree:: + :maxdepth: 1 + + team diff --git a/doc/about_flashrom/team.rst b/doc/about_flashrom/team.rst new file mode 100644 index 0000000..9d002bb --- /dev/null +++ b/doc/about_flashrom/team.rst @@ -0,0 +1,61 @@ +========= +Team +========= + +flashrom development process is happening in Gerrit. +All contributors and users who have a Gerrit account can send patches, +add comments to patches and vote +1..-1 on patches. + +All contributors and users are expected to follow Development guidelines, +Code of Conduct and Friendliness guidelines. + +There are two special groups in Gerrit. + +"flashrom reviewers" group +========================== + +Members of the group can do full approval of patches (i.e. vote +2). + +In general, members of the group have some area of responsibility in the MAINTAINERS file, +and are automatically added as reviewers to patches when the patch touches this area. + +The responsibilities are the following. + +* React to patches when added as a reviewer. + +* Try to respond to technical questions on the mailing list if the topic is something you know about + and can provide a useful response. + +* Know development guidelines and check the patches you are reviewing align with the guidelines. + +"flashrom developers" group +=========================== + +Members of the group can merge patches. +The responsibilities for the members of the group are described in more details below. + +There is no expectation on how much time you spend on your duties, some non-zero amount of time, +whatever capacity you have. But in general, you stay around on flashrom. + +If you disappear for some time (life happens), especially for a longer time, like several months, +especially without a warning: you implicitly agree that the others will handle the duties and make decisions if needed +(potentially without waiting for you to come back, if the decision is needed quickly). + +* Merge all contributors's patches (when they are ready), not just your own. + +* Be at least vaguely aware what development efforts are ongoing, this helps to make decisions + in what order the patches should be merged, and where could be merge conflicts. + +* Know development guidelines, and educate other contributors if needed (e.g. give links). + +* React to patches when added as a reviewer. + +* Try to respond to technical questions on the mailing list if the topic is something you know about + and can provide a useful response. + +* From time to time show up in real-time channel(s) and/or dev meetings. + +* Follow the Code of Conduct and Friendliness guidelines, be a good example for others. + +* Bonus point: if you work in a [software] company, educate and help contributors from your company + with upstream culture and dev guidelines. diff --git a/doc/index.rst b/doc/index.rst index 6a0f815..d308155 100644 --- a/doc/index.rst +++ b/doc/index.rst @@ -9,6 +9,7 @@ dev_guide/index classic_cli_manpage contact + about_flashrom/index how_to_add_docs documentation_license -- To view, visit https://review.coreboot.org/c/flashrom/+/76075?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3118b2b036eab93e901814447543b02c760c6a80 Gerrit-Change-Number: 76075 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: doc: Add Team page which describes Gerrit groups
by Anastasia Klimchuk (Code Review) 19 Jul '23

19 Jul '23

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/76075?usp=email ) Change subject: doc: Add Team page which describes Gerrit groups ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: > Random thoughts and rephrases, feel free to ignore Thank you so much for the comments! I really appreciate that you went through this text in full details! But I think your version of the text is more formal and I am not sure that it needs to be that formal. I think it would be better to have a text in simpler language, which is easier to read and understand. And thanks again! -- To view, visit https://review.coreboot.org/c/flashrom/+/76075?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3118b2b036eab93e901814447543b02c760c6a80 Gerrit-Change-Number: 76075 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 19 Jul 2023 13:05:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: ni845x_spi: refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 19 Jul '23

19 Jul '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/72160?usp=email ) ( 4 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: ni845x_spi: refactor singleton states into reentrant pattern ...................................................................... ni845x_spi: refactor singleton states into reentrant pattern Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver. TOPIC=register_master_api Change-Id: I45fcb8e20582cb0c532c4a9f0c78543a25f8d484 Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> Ticket: https://ticket.coreboot.org/issues/391 Reviewed-on: https://review.coreboot.org/c/flashrom/+/72160 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M ni845x_spi.c 1 file changed, 62 insertions(+), 39 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/ni845x_spi.c b/ni845x_spi.c index 58a8ef4..1948387 100644 --- a/ni845x_spi.c +++ b/ni845x_spi.c @@ -47,13 +47,14 @@ USE_HIGHER }; -static unsigned char CS_number; // use chip select 0 as default -static enum USB845x_type device_pid = Unknown_NI845X_Device; - -static uInt32 device_handle; -static NiHandle configuration_handle; -static uint16_t io_voltage_in_mV; -static bool ignore_io_voltage_limits; +struct ni845x_spi_data { + unsigned char CS_number; // use chip select 0 as default + enum USB845x_type device_pid; + uInt32 device_handle; + NiHandle configuration_handle; + uint16_t io_voltage_in_mV; + bool ignore_io_voltage_limits; +}; // USB-8452 supported voltages, keep this array in ascending order! static const uint8_t usb8452_io_voltages_in_100mV[5] = { @@ -394,98 +395,104 @@ static int ni845x_spi_shutdown(void *data) { + struct ni845x_spi_data *ni_data = data; int32 ret = 0; - if (configuration_handle != 0) { - ret = ni845xSpiConfigurationClose(configuration_handle); + if (ni_data->configuration_handle != 0) { + ret = ni845xSpiConfigurationClose(ni_data->configuration_handle); if (ret) ni845x_report_error("ni845xSpiConfigurationClose", ret); } - if (device_handle != 0) { - ret = ni845xClose(device_handle); + if (ni_data->device_handle != 0) { + ret = ni845xClose(ni_data->device_handle); if (ret) ni845x_report_error("ni845xClose", ret); } - return 0; + + free(data); + return ret; } static void ni845x_warn_over_max_voltage(const struct flashctx *flash) { - if (device_pid == USB8451) { + const struct ni845x_spi_data *data = flash->mst->spi.data; + + if (data->device_pid == USB8451) { msg_pwarn("The %s chip maximum voltage is %.1fV, while the USB-8451 " "IO voltage levels are 3.3V.\n" "Ignoring this because ignore_io_voltage_limits parameter is set.\n", flash->chip->name, flash->chip->voltage.max / 1000.0f); - } else if (device_pid == USB8452) { + } else if (data->device_pid == USB8452) { msg_pwarn("The %s chip maximum voltage is %.1fV, while the USB-8452 " "IO voltage is set to %.1fV.\n" "Ignoring this because ignore_io_voltage_limits parameter is set.\n", flash->chip->name, flash->chip->voltage.max / 1000.0f, - io_voltage_in_mV / 1000.0f); + data->io_voltage_in_mV / 1000.0f); } } static int ni845x_spi_io_voltage_check(const struct flashctx *flash) { + struct ni845x_spi_data *data = flash->mst->spi.data; static bool first_transmit = true; if (first_transmit && flash->chip) { first_transmit = false; - if (io_voltage_in_mV > flash->chip->voltage.max) { - if (ignore_io_voltage_limits) { + if (data->io_voltage_in_mV > flash->chip->voltage.max) { + if (data->ignore_io_voltage_limits) { ni845x_warn_over_max_voltage(flash); return 0; } - if (device_pid == USB8451) { + if (data->device_pid == USB8451) { msg_perr("The %s chip maximum voltage is %.1fV, while the USB-8451 " "IO voltage levels are 3.3V.\nAborting operations\n", flash->chip->name, flash->chip->voltage.max / 1000.0f); return -1; - } else if (device_pid == USB8452) { + } else if (data->device_pid == USB8452) { msg_perr("Lowering IO voltage because the %s chip maximum voltage is %.1fV, " "(%.1fV was set)\n", flash->chip->name, flash->chip->voltage.max / 1000.0f, - io_voltage_in_mV / 1000.0f); + data->io_voltage_in_mV / 1000.0f); if (usb8452_spi_set_io_voltage(flash->chip->voltage.max, - &io_voltage_in_mV, + &data->io_voltage_in_mV, USE_LOWER, - device_pid, - device_handle)) { + data->device_pid, + data->device_handle)) { msg_perr("Unable to lower the IO voltage below " "the chip's maximum voltage\n"); return -1; } } - } else if (io_voltage_in_mV < flash->chip->voltage.min) { - if (device_pid == USB8451) { + } else if (data->io_voltage_in_mV < flash->chip->voltage.min) { + if (data->device_pid == USB8451) { msg_pwarn("Flash operations might be unreliable, because the %s chip's " "minimum voltage is %.1fV, while the USB-8451's " "IO voltage levels are 3.3V.\n", flash->chip->name, flash->chip->voltage.min / 1000.0f); - return ignore_io_voltage_limits ? 0 : -1; - } else if (device_pid == USB8452) { + return data->ignore_io_voltage_limits ? 0 : -1; + } else if (data->device_pid == USB8452) { msg_pwarn("Raising the IO voltage because the %s chip's " "minimum voltage is %.1fV, " "(%.1fV was set)\n", flash->chip->name, flash->chip->voltage.min / 1000.0f, - io_voltage_in_mV / 1000.0f); + data->io_voltage_in_mV / 1000.0f); if (usb8452_spi_set_io_voltage(flash->chip->voltage.min, - &io_voltage_in_mV, + &data->io_voltage_in_mV, USE_HIGHER, - device_pid, - device_handle)) { + data->device_pid, + data->device_handle)) { msg_pwarn("Unable to raise the IO voltage above the chip's " "minimum voltage\n" "Flash operations might be unreliable.\n"); - return ignore_io_voltage_limits ? 0 : -1; + return data->ignore_io_voltage_limits ? 0 : -1; } } } @@ -499,6 +506,7 @@ const unsigned char *write_arr, unsigned char *read_arr) { + const struct ni845x_spi_data *data = flash->mst->spi.data; uInt32 read_size = 0; uInt8 *transfer_buffer = NULL; int32 ret = 0; @@ -514,8 +522,8 @@ memcpy(transfer_buffer, write_arr, write_cnt); - ret = ni845xSpiWriteRead(device_handle, - configuration_handle, + ret = ni845xSpiWriteRead(data->device_handle, + data->configuration_handle, (write_cnt + read_cnt), transfer_buffer, &read_size, transfer_buffer); if (ret < 0) { // Negative specifies an error, meaning the function did not perform the expected behavior. @@ -560,6 +568,10 @@ int spi_speed_KHz = 1000; // selecting 1 MHz SCK is a good bet char *serial_number = NULL; // by default open the first connected device char *ignore_io_voltage_limits_str = NULL; + bool ignore_io_voltage_limits; + unsigned char CS_number = 0; + enum USB845x_type device_pid = Unknown_NI845X_Device; + uInt32 device_handle; int32 tmp = 0; // read the cs parameter (which Chip select should we use) @@ -618,27 +630,38 @@ } free(serial_number); + struct ni845x_spi_data *data = calloc(1, sizeof(*data)); + if (!data) { + msg_perr("Unable to allocate space for SPI master data\n"); + return 1; + } + data->CS_number = CS_number; + data->device_pid = device_pid; + data->device_handle = device_handle; + data->ignore_io_voltage_limits = ignore_io_voltage_limits; + // open the SPI config handle - tmp = ni845xSpiConfigurationOpen(&configuration_handle); + tmp = ni845xSpiConfigurationOpen(&data->configuration_handle); if (tmp != 0) { ni845x_report_error("ni845xSpiConfigurationOpen", tmp); goto err; } - if (usb8452_spi_set_io_voltage(requested_io_voltage_mV, &io_voltage_in_mV, USE_LOWER, device_pid, device_handle) < 0) { + if (usb8452_spi_set_io_voltage(requested_io_voltage_mV, &data->io_voltage_in_mV, + USE_LOWER, data->device_pid, data->device_handle) < 0) { // no alert here usb8452_spi_set_io_voltage already printed that goto err; } - if (ni845x_spi_set_speed(configuration_handle, spi_speed_KHz)) { + if (ni845x_spi_set_speed(data->configuration_handle, spi_speed_KHz)) { msg_perr("Unable to set SPI speed\n"); goto err; } - return register_spi_master(&spi_programmer_ni845x, NULL); + return register_spi_master(&spi_programmer_ni845x, data); err: - ni845x_spi_shutdown(NULL); + ni845x_spi_shutdown(data); return 1; } -- To view, visit https://review.coreboot.org/c/flashrom/+/72160?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I45fcb8e20582cb0c532c4a9f0c78543a25f8d484 Gerrit-Change-Number: 72160 Gerrit-PatchSet: 6 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: ni845x_spi: handle errors using goto during initialization
by Anastasia Klimchuk (Code Review) 19 Jul '23

19 Jul '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/72156?usp=email ) ( 3 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: ni845x_spi: handle errors using goto during initialization ...................................................................... ni845x_spi: handle errors using goto during initialization This patch prepares the programmer to move global singleton states into a struct. TOPIC=register_master_api Change-Id: Ie9620d59db229729fd8523f99b0917d938bcc4ed Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> Ticket: https://ticket.coreboot.org/issues/391 Reviewed-on: https://review.coreboot.org/c/flashrom/+/72156 Reviewed-by: Miklós Márton <martonmiklosqdev(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M ni845x_spi.c 1 file changed, 8 insertions(+), 6 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved Miklós Márton: Looks good to me, approved diff --git a/ni845x_spi.c b/ni845x_spi.c index cb6b22e..58a8ef4 100644 --- a/ni845x_spi.c +++ b/ni845x_spi.c @@ -622,22 +622,24 @@ tmp = ni845xSpiConfigurationOpen(&configuration_handle); if (tmp != 0) { ni845x_report_error("ni845xSpiConfigurationOpen", tmp); - ni845x_spi_shutdown(NULL); - return 1; + goto err; } if (usb8452_spi_set_io_voltage(requested_io_voltage_mV, &io_voltage_in_mV, USE_LOWER, device_pid, device_handle) < 0) { - ni845x_spi_shutdown(NULL); - return 1; // no alert here usb8452_spi_set_io_voltage already printed that + // no alert here usb8452_spi_set_io_voltage already printed that + goto err; } if (ni845x_spi_set_speed(configuration_handle, spi_speed_KHz)) { msg_perr("Unable to set SPI speed\n"); - ni845x_spi_shutdown(NULL); - return 1; + goto err; } return register_spi_master(&spi_programmer_ni845x, NULL); + +err: + ni845x_spi_shutdown(NULL); + return 1; } const struct programmer_entry programmer_ni845x_spi = { -- To view, visit https://review.coreboot.org/c/flashrom/+/72156?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie9620d59db229729fd8523f99b0917d938bcc4ed Gerrit-Change-Number: 72156 Gerrit-PatchSet: 6 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: ni845x_spi: pass global state through func params
by Anastasia Klimchuk (Code Review) 19 Jul '23

19 Jul '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/72154?usp=email ) ( 6 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: ni845x_spi: pass global state through func params ...................................................................... ni845x_spi: pass global state through func params Instead of relying on global variables, pass and use their value or pointer to functions where possible. The usage of `io_voltage_in_mV` global var in `usb8452_spi_set_io_voltage` function is replaced with existing function argument `set_io_voltage_mV` since `set_io_voltage_mV` already contains the pointer to global var. This patch prepares the programmer to move global singleton states into a struct. TOPIC=register_master_api Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61 Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> Ticket: https://ticket.coreboot.org/issues/391 Reviewed-on: https://review.coreboot.org/c/flashrom/+/72154 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M ni845x_spi.c 1 file changed, 22 insertions(+), 16 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/ni845x_spi.c b/ni845x_spi.c index 6e7bb76..cb6b22e 100644 --- a/ni845x_spi.c +++ b/ni845x_spi.c @@ -135,11 +135,11 @@ * @param opened_handle the opened handle from the ni845xOpen * @return the 0 on successful competition, negative error code on failure positive code on warning */ -static int32 ni845x_spi_open_resource(char *resource_handle, uInt32 *opened_handle) +static int32 ni845x_spi_open_resource(char *resource_handle, uInt32 *opened_handle, enum USB845x_type pid) { // NI-845x driver loads the FPGA bitfile at the first time // which can take couple seconds - if (device_pid == USB8452) + if (pid == USB8452) msg_pwarn("Opening NI-8452, this might take a while for the first time\n"); int32 tmp = ni845xOpen(resource_handle, opened_handle); @@ -155,14 +155,14 @@ * @param serial a null terminated string containing the serial number of the specific device or NULL * @return the 0 on successful completion, negative error code on failure */ -static int ni845x_spi_open(const char *serial, uInt32 *return_handle) +static int ni845x_spi_open(const char *serial, uInt32 *return_handle, enum USB845x_type *pid) { char resource_name[256]; NiHandle device_find_handle; uInt32 found_devices_count = 0; int32 tmp = 0; - unsigned int vid, pid, usb_bus; + unsigned int vid, dev_pid, usb_bus; unsigned long int serial_as_number; int ret = -1; @@ -182,7 +182,7 @@ // and the DEADBEEF is the serial of the device if (sscanf(resource_name, "USB%u::0x%04X::0x%04X::%08lX::RAW", - &usb_bus, &vid, &pid, &serial_as_number) != 4) { + &usb_bus, &vid, &dev_pid, &serial_as_number) != 4) { // malformed resource string detected msg_pwarn("Warning: Unable to parse the %s NI-845x resource string.\n", resource_name); @@ -190,7 +190,7 @@ continue; } - device_pid = pid; + *pid = dev_pid; if (!serial || strtoul(serial, NULL, 16) == serial_as_number) break; @@ -205,7 +205,7 @@ } if (found_devices_count) - ret = ni845x_spi_open_resource(resource_name, return_handle); + ret = ni845x_spi_open_resource(resource_name, return_handle, *pid); _close_ret: tmp = ni845xCloseFindDeviceHandle(device_find_handle); @@ -227,14 +227,16 @@ */ static int usb8452_spi_set_io_voltage(uint16_t requested_io_voltage_mV, uint16_t *set_io_voltage_mV, - enum voltage_coerce_mode coerce_mode) + enum voltage_coerce_mode coerce_mode, + enum USB845x_type pid, + uInt32 device_handle) { unsigned int i = 0; uint8_t selected_voltage_100mV = 0; uint8_t requested_io_voltage_100mV = 0; - if (device_pid == USB8451) { - io_voltage_in_mV = 3300; + if (pid == USB8451) { + *set_io_voltage_mV = 3300; msg_pwarn("USB-8451 does not support the changing of the SPI IO voltage\n"); return 0; } @@ -307,7 +309,7 @@ * @param SCK_freq_in_KHz SCK speed in KHz * @return */ -static int ni845x_spi_set_speed(uint16_t SCK_freq_in_KHz) +static int ni845x_spi_set_speed(NiHandle configuration_handle, uint16_t SCK_freq_in_KHz) { int32 i = ni845xSpiConfigurationSetClockRate(configuration_handle, SCK_freq_in_KHz); uInt16 clock_freq_read_KHz; @@ -452,7 +454,9 @@ io_voltage_in_mV / 1000.0f); if (usb8452_spi_set_io_voltage(flash->chip->voltage.max, &io_voltage_in_mV, - USE_LOWER)) { + USE_LOWER, + device_pid, + device_handle)) { msg_perr("Unable to lower the IO voltage below " "the chip's maximum voltage\n"); return -1; @@ -475,7 +479,9 @@ io_voltage_in_mV / 1000.0f); if (usb8452_spi_set_io_voltage(flash->chip->voltage.min, &io_voltage_in_mV, - USE_HIGHER)) { + USE_HIGHER, + device_pid, + device_handle)) { msg_pwarn("Unable to raise the IO voltage above the chip's " "minimum voltage\n" "Flash operations might be unreliable.\n"); @@ -596,7 +602,7 @@ ignore_io_voltage_limits = true; } - if (ni845x_spi_open(serial_number, &device_handle)) { + if (ni845x_spi_open(serial_number, &device_handle, &device_pid)) { if (serial_number) { msg_pinfo("Could not find any connected NI USB-8451/8452 with serialnumber: %s!\n", serial_number); @@ -620,12 +626,12 @@ return 1; } - if (usb8452_spi_set_io_voltage(requested_io_voltage_mV, &io_voltage_in_mV, USE_LOWER) < 0) { + if (usb8452_spi_set_io_voltage(requested_io_voltage_mV, &io_voltage_in_mV, USE_LOWER, device_pid, device_handle) < 0) { ni845x_spi_shutdown(NULL); return 1; // no alert here usb8452_spi_set_io_voltage already printed that } - if (ni845x_spi_set_speed(spi_speed_KHz)) { + if (ni845x_spi_set_speed(configuration_handle, spi_speed_KHz)) { msg_perr("Unable to set SPI speed\n"); ni845x_spi_shutdown(NULL); return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/72154?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61 Gerrit-Change-Number: 72154 Gerrit-PatchSet: 8 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: libflashrom: Add layout "exclude" API
by Edward O'Callaghan (Code Review) 18 Jul '23

18 Jul '23

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/76005?usp=email ) Change subject: libflashrom: Add layout "exclude" API ...................................................................... libflashrom: Add layout "exclude" API Layouts can be expensive to derive (reading from flash), so we might want to reuse a layout for different purposes. Today, it's not possible to undo a flashrom_layout_include_region() operation (to, say, operate on a different region). Add such an API. Change-Id: I7ea3e0674f25e34bf2cfc8f464ae7ca1c1a3fbfd Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/76005 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Nikolai Artemiev <nartemiev(a)google.com> --- M bindings/rust/libflashrom/src/lib.rs M include/libflashrom.h M layout.c M libflashrom.map M tests/layout.c 5 files changed, 58 insertions(+), 0 deletions(-) Approvals: Edward O'Callaghan: Looks good to me, approved Nikolai Artemiev: Looks good to me, approved build bot (Jenkins): Verified diff --git a/bindings/rust/libflashrom/src/lib.rs b/bindings/rust/libflashrom/src/lib.rs index 95fde56..3f3fbc0 100644 --- a/bindings/rust/libflashrom/src/lib.rs +++ b/bindings/rust/libflashrom/src/lib.rs @@ -879,6 +879,32 @@ } } + /// Exclude a region + /// + /// # Errors + /// + /// This function will return an error if the region is not a valid CString, + /// or if libflashrom returns an error. + pub fn exclude_region(&mut self, region: &str) -> std::result::Result<(), RegionError> { + let err = { + let region = CString::new(region)?; + unsafe { + libflashrom_sys::flashrom_layout_exclude_region( + self.layout.as_mut(), + region.as_ptr(), + ) + } + }; + if err != 0 { + Err(RegionError::ErrorCode(ErrorCode { + function: "flashrom_layout_exclude_region", + code: err, + })) + } else { + Ok(()) + } + } + /// Get the [`Range`] for the given region /// /// # Errors diff --git a/include/libflashrom.h b/include/libflashrom.h index 490ef03..e87776b 100644 --- a/include/libflashrom.h +++ b/include/libflashrom.h @@ -421,6 +421,16 @@ */ int flashrom_layout_include_region(struct flashrom_layout *layout, const char *name); /** + * @brief Mark given region as not included. + * + * @param layout The layout to alter. + * @param name The name of the region to exclude. + * + * @return 0 on success, + * 1 if the given name can't be found. + */ +int flashrom_layout_exclude_region(struct flashrom_layout *layout, const char *name); +/** * @brief Get given region's offset and length. * * @param[in] layout The existing layout. diff --git a/layout.c b/layout.c index cddd908..e46e61a 100644 --- a/layout.c +++ b/layout.c @@ -217,6 +217,17 @@ return 1; } +/* returns 0 to indicate success, 1 to indicate failure */ +static int exclude_region(struct flashrom_layout *const l, const char *name) +{ + struct romentry *const entry = _layout_entry_by_name(l, name); + if (entry) { + entry->included = false; + return 0; + } + return 1; +} + /* returns -1 if an entry is not found, 0 if found. */ static int romentry_exists(struct flashrom_layout *const l, char *name, char *file) { @@ -441,6 +452,11 @@ return include_region(layout, name, NULL); } +int flashrom_layout_exclude_region(struct flashrom_layout *const layout, const char *name) +{ + return exclude_region(layout, name); +} + int flashrom_layout_get_region_range(struct flashrom_layout *const l, const char *name, unsigned int *start, unsigned int *len) { diff --git a/libflashrom.map b/libflashrom.map index e3c7a76..77abf72 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -15,6 +15,7 @@ flashrom_image_write; flashrom_init; flashrom_layout_add_region; + flashrom_layout_exclude_region; flashrom_layout_get_region_range; flashrom_layout_include_region; flashrom_layout_new; diff --git a/tests/layout.c b/tests/layout.c index e71debf..917d77e 100644 --- a/tests/layout.c +++ b/tests/layout.c @@ -110,6 +110,8 @@ unsigned int region_start = 0x00021000; unsigned int region_end = 0x00031000; + unsigned int region2_start = 0x00041000; + unsigned int region2_end = 0x00051000; unsigned int start = 0; unsigned int len = 0; @@ -119,6 +121,9 @@ assert_int_equal(0, flashrom_layout_new(&layout)); assert_int_equal(0, flashrom_layout_add_region(layout, region_start, region_end, "region")); assert_int_equal(0, flashrom_layout_include_region(layout, "region")); + assert_int_equal(0, flashrom_layout_add_region(layout, region2_start, region2_end, "region2")); + assert_int_equal(0, flashrom_layout_include_region(layout, "region2")); + assert_int_equal(0, flashrom_layout_exclude_region(layout, "region2")); printf("done\n"); printf("Asserting region range... "); -- To view, visit https://review.coreboot.org/c/flashrom/+/76005?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7ea3e0674f25e34bf2cfc8f464ae7ca1c1a3fbfd Gerrit-Change-Number: 76005 Gerrit-PatchSet: 3 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit July 2023