Messages in this thread | | | Date | Thu, 26 Jan 2023 08:52:00 -0600 | From | Bjorn Helgaas <> | Subject | Re: [PATCH 0/8] PCI: rockchip: Fix PCIe endpoint controller driver |
| |
Hi Rick,
Thanks very much for your work.
On Thu, Jan 26, 2023 at 02:50:40PM +0100, Rick Wertenbroek wrote: > This is a series of patches that fixes the PCIe endpoint controller driver > for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19 > > The original driver in mainline had issues and would not allow for the > RK3399 to operate in PCIe endpoint mode. This patch series fixes that so > that the PCIe core controller of the RK3399 SoC can now act as a PCIe > endpoint.
So we merged cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") when it actually didn't work? Ouch. Thanks for fixing it and testing it.
> Rick Wertenbroek (8): > PCI: rockchip: Removed writes to unused registers > PCI: rockchip: Fixed setup of Device ID > PCI: rockchip: Fixed endpoint controller Configuration Request Retry > Status > PCI: rockchip: Added poll and timeout to wait for PHY PLLs to be > locked > PCI: rockchip: Added dtsi entry for PCIe endpoint controller > PCI: rockchip: Fixed window mapping and address translation for > endpoint > PCI: rockchip: Fixed legacy IRQ generation for endpoint > PCI: rockchip: Fixed MSI generation from PCIe endpoint core
For the next iteration, can you please update these subject lines and commit logs to:
- Use imperative mood, i.e., read like a command, instead of a past tense description of what was done. For example, say "Remove writes to unused registers" instead of "Removed writes ..."
- Be more specific when possible. "Fix" conveys no information about the actual code change. For example, "Fixed endpoint controller Configuration Request Retry Status" gives a general idea, but it would be more useful if it said something about clearing config mode after probe.
- Say what the patch does in the commit log. The current ones often describe a *problem*, but do not explicitly say what the patch does. The commit log should be complete in itself even without the subject line, so it usually contains a slightly expanded version of the subject line.
- Split patches that do more than one logical thing. The commit log for "Fixed MSI generation ..." talks about a u16/u32 shift issue, but the patch also adds an unrelated check for multi-function devices.
- If a patch is a fix for an existing issue and may need to be backported, identify the commit that introduced the issue and add "Fixes: " lines. This helps distros figure out whether and how far to backport patches.
- Refer to the device consistently. I see: RK3399 PCI EP core RK3399 SoC PCIe EP core RK3399 PCIe endpoint core I vote for "RK3399 PCIe Endpoint core".
Notes about imperative mood: https://chris.beams.io/posts/git-commit/ https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n94
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++ > drivers/pci/controller/pcie-rockchip-ep.c | 149 +++++++++++----------- > drivers/pci/controller/pcie-rockchip.c | 16 +++ > drivers/pci/controller/pcie-rockchip.h | 36 ++++-- > 4 files changed, 137 insertions(+), 89 deletions(-) > > -- > 2.25.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
| |