| Message ID | 20230406111142.74410-1-minda.chen@starfivetech.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add JH7110 PCIe driver support | expand |
+CC Daire Hey Minda, On Thu, Apr 06, 2023 at 07:11:39PM +0800, Minda Chen wrote: > This patchset adds PCIe driver for the StarFive JH7110 SoC. > The patch has been tested on the VisionFive 2 board. The test > devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter. I was talking with Daire last week about some changes he's working on for the microchip driver, and we seemed to recall an off-list email sent to Daire & Bjorn about extracting the common PLDA bits from the pcie-microchip-host driver to be used with an (at that point) unreleased SoC. Perhaps Bjorn has this in his mailbox somewhere still, our corporate mail policy scrubs things from over a year ago & I could not find it. I realised that that may actually have been StarFive, and the driver on your GitHub [1] certainly felt very familiar to Daire (he said it was very similar to his earlier revisions of his driver). I've not looked at a diff between this and the version you ship on GitHub, but first a quick inspection it mostly just looks like you did s/plda/sifive/ on the file. I'm obviously not a PCI maintainer, but if there are common bits between the two drivers, extracting common bits seems like a good idea to me... https://github.com/starfive-tech/linux/blob/JH7110_VisionFive2_devel/drivers/pci/controller/pcie-plda.c > > This patchset should be applied after the patchset [1], [2], [3] and[4]: > [1] https://patchwork.kernel.org/project/linux-riscv/cover/20230314124404.117592-1-xingyu.wu@starfivetech.com/ > [2] https://lore.kernel.org/all/20230315055813.94740-1-william.qiu@starfivetech.com/ > [3] https://patchwork.kernel.org/project/linux-phy/cover/20230315100421.133428-1-changhuang.liang@starfivetech.com/ > [4] https://patchwork.kernel.org/project/linux-usb/cover/20230406015216.27034-1-minda.chen@starfivetech.com/ How many of the dependencies here are compiletime for the driver & how many of them are just for the dts patch? Cheers, Conor.
Gah, I never actually CCed Daire. Apologies for the additional email. On Thu, Apr 06, 2023 at 12:47:41PM +0100, Conor Dooley wrote: > +CC Daire > > Hey Minda, > > On Thu, Apr 06, 2023 at 07:11:39PM +0800, Minda Chen wrote: > > This patchset adds PCIe driver for the StarFive JH7110 SoC. > > The patch has been tested on the VisionFive 2 board. The test > > devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter. > > I was talking with Daire last week about some changes he's working on > for the microchip driver, and we seemed to recall an off-list email > sent to Daire & Bjorn about extracting the common PLDA bits from the > pcie-microchip-host driver to be used with an (at that point) > unreleased SoC. Perhaps Bjorn has this in his mailbox somewhere still, > our corporate mail policy scrubs things from over a year ago & I could > not find it. > > I realised that that may actually have been StarFive, and the driver on > your GitHub [1] certainly felt very familiar to Daire (he said it was > very similar to his earlier revisions of his driver). > > I've not looked at a diff between this and the version you ship on > GitHub, but first a quick inspection it mostly just looks like you > did s/plda/sifive/ on the file. > > I'm obviously not a PCI maintainer, but if there are common bits between > the two drivers, extracting common bits seems like a good idea to me... > > https://github.com/starfive-tech/linux/blob/JH7110_VisionFive2_devel/drivers/pci/controller/pcie-plda.c > > > > This patchset should be applied after the patchset [1], [2], [3] and[4]: > > [1] https://patchwork.kernel.org/project/linux-riscv/cover/20230314124404.117592-1-xingyu.wu@starfivetech.com/ > > [2] https://lore.kernel.org/all/20230315055813.94740-1-william.qiu@starfivetech.com/ > > [3] https://patchwork.kernel.org/project/linux-phy/cover/20230315100421.133428-1-changhuang.liang@starfivetech.com/ > > [4] https://patchwork.kernel.org/project/linux-usb/cover/20230406015216.27034-1-minda.chen@starfivetech.com/ > > How many of the dependencies here are compiletime for the driver & how > many of them are just for the dts patch? > > Cheers, > Conor.
On 2023/4/6 19:54, Conor Dooley wrote: > Gah, I never actually CCed Daire. Apologies for the additional email. > > On Thu, Apr 06, 2023 at 12:47:41PM +0100, Conor Dooley wrote: >> +CC Daire >> >> Hey Minda, >> >> On Thu, Apr 06, 2023 at 07:11:39PM +0800, Minda Chen wrote: >> > This patchset adds PCIe driver for the StarFive JH7110 SoC. >> > The patch has been tested on the VisionFive 2 board. The test >> > devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter. >> >> I was talking with Daire last week about some changes he's working on >> for the microchip driver, and we seemed to recall an off-list email >> sent to Daire & Bjorn about extracting the common PLDA bits from the >> pcie-microchip-host driver to be used with an (at that point) >> unreleased SoC. Perhaps Bjorn has this in his mailbox somewhere still, >> our corporate mail policy scrubs things from over a year ago & I could >> not find it. >> >> I realised that that may actually have been StarFive, and the driver on >> your GitHub [1] certainly felt very familiar to Daire (he said it was >> very similar to his earlier revisions of his driver). >> >> I've not looked at a diff between this and the version you ship on >> GitHub, but first a quick inspection it mostly just looks like you >> did s/plda/sifive/ on the file. >> >> I'm obviously not a PCI maintainer, but if there are common bits between >> the two drivers, extracting common bits seems like a good idea to me... >Thanks. It is pleasure to using same common codes. Does common bits changes will upstream soon? And I see there are many difference between pcie-microchip-host and our codes. >> https://github.com/starfive-tech/linux/blob/JH7110_VisionFive2_devel/drivers/pci/controller/pcie-plda.c >> > >> > This patchset should be applied after the patchset [1], [2], [3] and[4]: >> > [1] https://patchwork.kernel.org/project/linux-riscv/cover/20230314124404.117592-1-xingyu.wu@starfivetech.com/ >> > [2] https://lore.kernel.org/all/20230315055813.94740-1-william.qiu@starfivetech.com/ >> > [3] https://patchwork.kernel.org/project/linux-phy/cover/20230315100421.133428-1-changhuang.liang@starfivetech.com/ >> > [4] https://patchwork.kernel.org/project/linux-usb/cover/20230406015216.27034-1-minda.chen@starfivetech.com/ >> >> How many of the dependencies here are compiletime for the driver & how >> many of them are just for the dts patch? >> PCIe rely on stg clock in [1], rely on stg syscon in [2]. Patch [2] is accepted now. Maybe I will delete this. both [3] and [4] is PHY dependency. >> Cheers, >> Conor. > >
Hey Minda, On Fri, Apr 07, 2023 at 10:32:51AM +0800, Minda Chen wrote: > On 2023/4/6 19:54, Conor Dooley wrote: > > On Thu, Apr 06, 2023 at 12:47:41PM +0100, Conor Dooley wrote: > >> On Thu, Apr 06, 2023 at 07:11:39PM +0800, Minda Chen wrote: > >> > This patchset adds PCIe driver for the StarFive JH7110 SoC. > >> > The patch has been tested on the VisionFive 2 board. The test > >> > devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter. > >> > >> I was talking with Daire last week about some changes he's working on > >> for the microchip driver, and we seemed to recall an off-list email > >> sent to Daire & Bjorn about extracting the common PLDA bits from the > >> pcie-microchip-host driver to be used with an (at that point) > >> unreleased SoC. Perhaps Bjorn has this in his mailbox somewhere still, > >> our corporate mail policy scrubs things from over a year ago & I could > >> not find it. > >> > >> I realised that that may actually have been StarFive, and the driver on > >> your GitHub [1] certainly felt very familiar to Daire (he said it was > >> very similar to his earlier revisions of his driver). > >> > >> I've not looked at a diff between this and the version you ship on > >> GitHub, but first a quick inspection it mostly just looks like you > >> did s/plda/sifive/ on the file. > >> > >> I'm obviously not a PCI maintainer, but if there are common bits between > >> the two drivers, extracting common bits seems like a good idea to me... > Thanks. It is pleasure to using same common codes. Does common bits changes > will upstream soon? I don't quite get what you mean. We've got some changes that are in progress here: https://lore.kernel.org/linux-pci/20230111125323.1911373-1-daire.mcnamara@microchip.com/ We've been quiet there for a while, but Daire's back looking into Robin's comments in there about the range parsing/window setup at the moment. I'm not sure if that's what you mean though, since you said "common bits" & Daire was doing that work in a world where there was no jh7110 driver in the mix. Extracting common bits would be part of the process of adding a new driver, as I don't think there's any real reason to do so without another in-tree user. > And I see there are many difference between pcie-microchip-host and our codes. Right. I'd expect there to be a fair difference between our integrations of the IP, and therefore there'll be a bunch of non-shareable bits. You need the stg,syscon & phy bits, and the clock/reset handling is clearly different too. > >> https://github.com/starfive-tech/linux/blob/JH7110_VisionFive2_devel/drivers/pci/controller/pcie-plda.c I had a bit of a read through this again today with Daire to check what the differences actually are and it *looked* like the main, non-implementation related, differences were the extra "event" domain that was created to simplify the driver & the bottom half interrupt handling. That all came out of the review process, so it's likely that some of the same requests would be made of you by the PCI maintainers anyway. As an aside, you should probably run checkpatch --strict on this submission, there's a rake of coding style "issues" in the new code you've added. Cheers, Conor.
On 2023/4/7 17:57, Conor Dooley wrote: > Hey Minda, > > On Fri, Apr 07, 2023 at 10:32:51AM +0800, Minda Chen wrote: >> On 2023/4/6 19:54, Conor Dooley wrote: >> > On Thu, Apr 06, 2023 at 12:47:41PM +0100, Conor Dooley wrote: >> >> On Thu, Apr 06, 2023 at 07:11:39PM +0800, Minda Chen wrote: >> >> > This patchset adds PCIe driver for the StarFive JH7110 SoC. >> >> > The patch has been tested on the VisionFive 2 board. The test >> >> > devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter. >> >> >> >> I was talking with Daire last week about some changes he's working on >> >> for the microchip driver, and we seemed to recall an off-list email >> >> sent to Daire & Bjorn about extracting the common PLDA bits from the >> >> pcie-microchip-host driver to be used with an (at that point) >> >> unreleased SoC. Perhaps Bjorn has this in his mailbox somewhere still, >> >> our corporate mail policy scrubs things from over a year ago & I could >> >> not find it. >> >> >> >> I realised that that may actually have been StarFive, and the driver on >> >> your GitHub [1] certainly felt very familiar to Daire (he said it was >> >> very similar to his earlier revisions of his driver). >> >> >> >> I've not looked at a diff between this and the version you ship on >> >> GitHub, but first a quick inspection it mostly just looks like you >> >> did s/plda/sifive/ on the file. >> >> >> >> I'm obviously not a PCI maintainer, but if there are common bits between >> >> the two drivers, extracting common bits seems like a good idea to me... > >> Thanks. It is pleasure to using same common codes. Does common bits changes >> will upstream soon? > > I don't quite get what you mean. We've got some changes that are in > progress here: > https://lore.kernel.org/linux-pci/20230111125323.1911373-1-daire.mcnamara@microchip.com/ > We've been quiet there for a while, but Daire's back looking into Robin's > comments in there about the range parsing/window setup at the moment. > > I'm not sure if that's what you mean though, since you said "common > bits" & Daire was doing that work in a world where there was no jh7110 > driver in the mix. > Extracting common bits would be part of the process of adding a new > driver, as I don't think there's any real reason to do so without > another in-tree user. > OK, I know extracting common bits is microchip new PCIe driver codes changed. Just ignore my previous comments. Maybe I will try to restructuring the driver code according to corporate e-mail which has been sent one year ago. >> And I see there are many difference between pcie-microchip-host and our codes. > > Right. I'd expect there to be a fair difference between our integrations > of the IP, and therefore there'll be a bunch of non-shareable bits. > > You need the stg,syscon & phy bits, and the clock/reset handling is > clearly different too. > >> >> https://github.com/starfive-tech/linux/blob/JH7110_VisionFive2_devel/drivers/pci/controller/pcie-plda.c > > I had a bit of a read through this again today with Daire to check what > the differences actually are and it *looked* like the main, > non-implementation related, differences were the extra "event" domain > that was created to simplify the driver & the bottom half interrupt > handling. > That all came out of the review process, so it's likely that some of the > same requests would be made of you by the PCI maintainers anyway. > Thanks. I will check it and change my codes. > As an aside, you should probably run checkpatch --strict on this > submission, there's a rake of coding style "issues" in the new code > you've added. > I do not run checkpatch with "--strict". I will run with it. > Cheers, > Conor.