Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
On Wed, 23 Oct 2019, Alistair Francis wrote: > On Tue, 2019-10-22 at 18:06 -0700, Paul Walmsley wrote: > > On Tue, 22 Oct 2019, Alistair Francis wrote: > > > > > I think it makese sense for this to go into Linux first. > > > > > > The QEMU patches are going to be accepted, just some nit picking to > > > do first :) > > > > > > After that we have to wait for a PR and then a QEMU release until > > > most people will see the change in QEMU. In that time Linux 5.4 will > > > be released, if this can make it into 5.4 then everyone using 5.4 > > > will get the new RTC as soon as they upgrade QEMU (QEMU provides the > > > device tree). If this has to wait until QEMU has support then it > > > won't be supported for users until even later. > > > > > > Users are generally slow to update kernels (buildroot is still using > > > 5.1 by default for example) so the sooner changes like this go in > > > the better. > > > > The defconfigs are really just for kernel developers. We expect users > > to define their own Kconfigs for their own needs. > > From experience most people use the defconfig, at least as a starting > point. We'll definitely add it to the defconfigs, but I think it makes sense to do that once the patches hit the QEMU master branch. (No need to wait for a QEMU release.) That roughly matches what I understand the Linux kernel's approach is to adding hardware support: no point in adding hardware support until it looks likely that it will actually exist. Otherwise it just adds churn and maintenance burden. > I was under the impression that everyone was on board with this going > in. In QEMU land it doesn't make sense to add it if the kernel isn't > going to, so we need to be on the same page here. Whatever RTC gets added into QEMU, we'll take defconfig patches for. I don't care which one it is. Based on the patches that hit the kernel lists, it initially looked like the Goldfish RTC was more complicated than it needed to be; but it turned out I just didn't look deeply enough. > From the other discussions it looks like you are happy with this change > overall right? Yes - Paul
Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
On Tue, 2019-10-22 at 18:06 -0700, Paul Walmsley wrote: > On Tue, 22 Oct 2019, Alistair Francis wrote: > > > I think it makese sense for this to go into Linux first. > > > > The QEMU patches are going to be accepted, just some nit picking to > > do > > first :) > > > > After that we have to wait for a PR and then a QEMU release until > > most > > people will see the change in QEMU. In that time Linux 5.4 will be > > released, if this can make it into 5.4 then everyone using 5.4 will > > get > > the new RTC as soon as they upgrade QEMU (QEMU provides the device > > tree). If this has to wait until QEMU has support then it won't be > > supported for users until even later. > > > > Users are generally slow to update kernels (buildroot is still > > using > > 5.1 by default for example) so the sooner changes like this go in > > the > > better. > > The defconfigs are really just for kernel developers. We expect > users to > define their own Kconfigs for their own needs. From experience most people use the defconfig, at least as a starting point. > > If using the Goldfish code really is what we all want to do (see > below), > then the kernel patch that should go in right away -- which also has > no > dependence on what QEMU does -- would be the first patch of this > series: > > https://lore.kernel.org/linux-riscv/20190925063706.56175-2-anup.pa...@wdc.com/ Ok, so it looks like this patch will be a 5.5 patch not a 5.4 patch. It looks like that can't be helped. I just don't want the defconfig change waiting on QEMU as I think that just slows everything down. > > And that should go in via whoever is maintaining the Goldfish driver, > not > the RISC-V tree. (It looks like drivers/platform/goldfish is > completely > unmaintained - a red flag! - so probably someone needs to persuade > Greg or > Andrew to take it.) > > Incidentally, just looking at drivers/platform/goldfish, that driver > seems > to be some sort of Google-specific RPC driver. Are you all really > sure > you want to enable that just for an RTC? Seems like overkill - there > are > much simpler RTCs out there. I was under the impression that everyone was on board with this going in. In QEMU land it doesn't make sense to add it if the kernel isn't going to, so we need to be on the same page here. From the other discussions it looks like you are happy with this change overall right? Alistair > > > - Paul > > ___ > linux-riscv mailing list > linux-ri...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
On Wed, 23 Oct 2019, Anup Patel wrote: > On Wed, Oct 23, 2019 at 11:30 AM Paul Walmsley > wrote: > > > Is drivers/platform/goldfish/goldfish_pipe.c required for the Goldfish RTC > > driver or not? > > No, it's not required. > > > If not, then the first patch that was sent isn't the right fix. It would > > be better to remove the Kbuild dependency between the code in > > drivers/platform/goldfish and the Goldfish RTC. > > The common GOLDFISH kconfig option is there to specify the > common expectations of all GOLDFISH drivers from Linux ARCH > support. OK, then in that case the Goldfish RTC sounds reasonable to me. - Paul
Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
On Wed, Oct 23, 2019 at 11:30 AM Paul Walmsley wrote: > > On Wed, 23 Oct 2019, Anup Patel wrote: > > > On Wed, Oct 23, 2019 at 6:37 AM Paul Walmsley > > wrote: > > > > > Incidentally, just looking at drivers/platform/goldfish, that driver seems > > > to be some sort of Google-specific RPC driver. Are you all really sure > > > > Nopes, it's not RPC driver. In fact, all Goldfish virtual platform > > devices are MMIO devices. > > Is drivers/platform/goldfish/goldfish_pipe.c required for the Goldfish RTC > driver or not? No, it's not required. > > If not, then the first patch that was sent isn't the right fix. It would > be better to remove the Kbuild dependency between the code in > drivers/platform/goldfish and the Goldfish RTC. The common GOLDFISH kconfig option is there to specify the common expectations of all GOLDFISH drivers from Linux ARCH support. Currently, all GOLDFISH drivers require HAS_IOMEM and HAS_DMA support from underlying arch. If you still think that common GOLDFISH kconfig option is not required then please go ahead and send patch. > > If it is required, then surely there must be a simpler RTC implementation > available. GOLDFISH pipe is not required so GOLDFISH RTC is certainly a simple RTC implementation. > > > The problem is VirtIO spec does not define any RTC device so instead of > > inventing our own virtual RTC device we re-use RTC device defined in > > Goldfish virtual platform for QEMU virt machine. This way we can re-use > > the Linux Goldfish RTC driver. > > With 160+ RTC drivers in the kernel tree already, we certainly agree that > it doesn't make sense to invent a new RTC. > > > - Paul Regards, Anup
Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
On Wed, 23 Oct 2019, Anup Patel wrote: > On Wed, Oct 23, 2019 at 6:37 AM Paul Walmsley > wrote: > > > Incidentally, just looking at drivers/platform/goldfish, that driver seems > > to be some sort of Google-specific RPC driver. Are you all really sure > > Nopes, it's not RPC driver. In fact, all Goldfish virtual platform > devices are MMIO devices. Is drivers/platform/goldfish/goldfish_pipe.c required for the Goldfish RTC driver or not? If not, then the first patch that was sent isn't the right fix. It would be better to remove the Kbuild dependency between the code in drivers/platform/goldfish and the Goldfish RTC. If it is required, then surely there must be a simpler RTC implementation available. > The problem is VirtIO spec does not define any RTC device so instead of > inventing our own virtual RTC device we re-use RTC device defined in > Goldfish virtual platform for QEMU virt machine. This way we can re-use > the Linux Goldfish RTC driver. With 160+ RTC drivers in the kernel tree already, we certainly agree that it doesn't make sense to invent a new RTC. - Paul
Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
On Wed, Oct 23, 2019 at 6:37 AM Paul Walmsley wrote: > > On Tue, 22 Oct 2019, Alistair Francis wrote: > > > I think it makese sense for this to go into Linux first. > > > > The QEMU patches are going to be accepted, just some nit picking to do > > first :) > > > > After that we have to wait for a PR and then a QEMU release until most > > people will see the change in QEMU. In that time Linux 5.4 will be > > released, if this can make it into 5.4 then everyone using 5.4 will get > > the new RTC as soon as they upgrade QEMU (QEMU provides the device > > tree). If this has to wait until QEMU has support then it won't be > > supported for users until even later. > > > > Users are generally slow to update kernels (buildroot is still using > > 5.1 by default for example) so the sooner changes like this go in the > > better. > > The defconfigs are really just for kernel developers. We expect users to > define their own Kconfigs for their own needs. > > If using the Goldfish code really is what we all want to do (see below), > then the kernel patch that should go in right away -- which also has no > dependence on what QEMU does -- would be the first patch of this series: > > https://lore.kernel.org/linux-riscv/20190925063706.56175-2-anup.pa...@wdc.com/ > > And that should go in via whoever is maintaining the Goldfish driver, not > the RISC-V tree. (It looks like drivers/platform/goldfish is completely > unmaintained - a red flag! - so probably someone needs to persuade Greg or > Andrew to take it.) GregKH has already queued this for Linux-5.5 and you can see this commit present in linux-next tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/platform/goldfish?h=next-20191022 > > Incidentally, just looking at drivers/platform/goldfish, that driver seems > to be some sort of Google-specific RPC driver. Are you all really sure Nopes, it's not RPC driver. In fact, all Goldfish virtual platform devices are MMIO devices. > you want to enable that just for an RTC? Seems like overkill - there are > much simpler RTCs out there. No, it's not overkill. All Goldfish virtual platform devices are quite simple MMIO devices having bare minimum registers required for device functioning. The problem is VirtIO spec does not define any RTC device so instead of inventing our own virtual RTC device we re-use RTC device defined in Goldfish virtual platform for QEMU virt machine. This way we can re-use the Linux Goldfish RTC driver. BTW, I will send-out QEMU Goldfish RTC patches today or tomorrow addressing nit comments from Alistair. Regards, Anup
Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
On Tue, 22 Oct 2019, Alistair Francis wrote: > I think it makese sense for this to go into Linux first. > > The QEMU patches are going to be accepted, just some nit picking to do > first :) > > After that we have to wait for a PR and then a QEMU release until most > people will see the change in QEMU. In that time Linux 5.4 will be > released, if this can make it into 5.4 then everyone using 5.4 will get > the new RTC as soon as they upgrade QEMU (QEMU provides the device > tree). If this has to wait until QEMU has support then it won't be > supported for users until even later. > > Users are generally slow to update kernels (buildroot is still using > 5.1 by default for example) so the sooner changes like this go in the > better. The defconfigs are really just for kernel developers. We expect users to define their own Kconfigs for their own needs. If using the Goldfish code really is what we all want to do (see below), then the kernel patch that should go in right away -- which also has no dependence on what QEMU does -- would be the first patch of this series: https://lore.kernel.org/linux-riscv/20190925063706.56175-2-anup.pa...@wdc.com/ And that should go in via whoever is maintaining the Goldfish driver, not the RISC-V tree. (It looks like drivers/platform/goldfish is completely unmaintained - a red flag! - so probably someone needs to persuade Greg or Andrew to take it.) Incidentally, just looking at drivers/platform/goldfish, that driver seems to be some sort of Google-specific RPC driver. Are you all really sure you want to enable that just for an RTC? Seems like overkill - there are much simpler RTCs out there. - Paul
Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
On Tue, 2019-10-22 at 12:23 -0700, Paul Walmsley wrote: > On Mon, 14 Oct 2019, Anup Patel wrote: > > > > -Original Message- > > > From: Palmer Dabbelt > > > Sent: Saturday, October 12, 2019 11:09 PM > > > To: Anup Patel > > > Cc: Paul Walmsley ; > > > a...@eecs.berkeley.edu; > > > Greg KH ; r...@google.com; Atish > > > Patra > > > ; Alistair Francis > > >; > > > Christoph Hellwig ; a...@brainfault.org; > > > linux- > > > ri...@lists.infradead.org; linux-kernel@vger.kernel.org; Anup > > > Patel > > > > > > Subject: Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish > > > RTC driver > > > > > > On Tue, 24 Sep 2019 23:38:08 PDT (-0700), Anup Patel wrote: > > > > We have Goldfish RTC device available on QEMU RISC-V virt > > > > machine > > > > hence enable required driver in RV32 and RV64 defconfigs. > > My understanding is that the Goldfish support is still under > discussion on the QEMU side and isn't merged yet - is that accurate? > > https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg04904.html > > > > Reviewed-by: Palmer Dabbelt > > > > > > IIRC there was supposed to be a follow-up to your QEMU patch set > > > to rebase > > > it on top of a refactoring of their RTC code, but I don't see it > > > in my inbox. LMK > > > if I missed it, as QEMU's soft freeze is in a few weeks and I'd > > > like to make > > > sure I get everything in. > > > > I was hoping for QEMU RTC refactoring to be merged soon but it has > > not > > happened so far. I will wait couple of more days then send v3 of > > QEMU > > patches. > > The patch looks fine to me, but let's wait until the underlying > support > actually appears on the QEMU "hardware". Could you resend once > that's > happened? I think it makese sense for this to go into Linux first. The QEMU patches are going to be accepted, just some nit picking to do first :) After that we have to wait for a PR and then a QEMU release until most people will see the change in QEMU. In that time Linux 5.4 will be released, if this can make it into 5.4 then everyone using 5.4 will get the new RTC as soon as they upgrade QEMU (QEMU provides the device tree). If this has to wait until QEMU has support then it won't be supported for users until even later. Users are generally slow to update kernels (buildroot is still using 5.1 by default for example) so the sooner changes like this go in the better. Alistair > > thanks, > > - Paul
RE: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
On Mon, 14 Oct 2019, Anup Patel wrote: > > -Original Message- > > From: Palmer Dabbelt > > Sent: Saturday, October 12, 2019 11:09 PM > > To: Anup Patel > > Cc: Paul Walmsley ; a...@eecs.berkeley.edu; > > Greg KH ; r...@google.com; Atish Patra > > ; Alistair Francis ; > > Christoph Hellwig ; a...@brainfault.org; linux- > > ri...@lists.infradead.org; linux-kernel@vger.kernel.org; Anup Patel > > > > Subject: Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver > > > > On Tue, 24 Sep 2019 23:38:08 PDT (-0700), Anup Patel wrote: > > > We have Goldfish RTC device available on QEMU RISC-V virt machine > > > hence enable required driver in RV32 and RV64 defconfigs. My understanding is that the Goldfish support is still under discussion on the QEMU side and isn't merged yet - is that accurate? https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg04904.html > > > > Reviewed-by: Palmer Dabbelt > > > > IIRC there was supposed to be a follow-up to your QEMU patch set to rebase > > it on top of a refactoring of their RTC code, but I don't see it in my > > inbox. LMK > > if I missed it, as QEMU's soft freeze is in a few weeks and I'd like to make > > sure I get everything in. > > I was hoping for QEMU RTC refactoring to be merged soon but it has not > happened so far. I will wait couple of more days then send v3 of QEMU > patches. The patch looks fine to me, but let's wait until the underlying support actually appears on the QEMU "hardware". Could you resend once that's happened? thanks, - Paul
RE: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
> -Original Message- > From: Palmer Dabbelt > Sent: Saturday, October 12, 2019 11:09 PM > To: Anup Patel > Cc: Paul Walmsley ; a...@eecs.berkeley.edu; > Greg KH ; r...@google.com; Atish Patra > ; Alistair Francis ; > Christoph Hellwig ; a...@brainfault.org; linux- > ri...@lists.infradead.org; linux-kernel@vger.kernel.org; Anup Patel > > Subject: Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver > > On Tue, 24 Sep 2019 23:38:08 PDT (-0700), Anup Patel wrote: > > We have Goldfish RTC device available on QEMU RISC-V virt machine > > hence enable required driver in RV32 and RV64 defconfigs. > > > > Signed-off-by: Anup Patel > > --- > > arch/riscv/configs/defconfig | 3 +++ > > arch/riscv/configs/rv32_defconfig | 3 +++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/arch/riscv/configs/defconfig > > b/arch/riscv/configs/defconfig index 3efff552a261..57b4f67b0c0b 100644 > > --- a/arch/riscv/configs/defconfig > > +++ b/arch/riscv/configs/defconfig > > @@ -73,7 +73,10 @@ CONFIG_USB_STORAGE=y CONFIG_USB_UAS=y > > CONFIG_MMC=y CONFIG_MMC_SPI=y > > +CONFIG_RTC_CLASS=y > > +CONFIG_RTC_DRV_GOLDFISH=y > > CONFIG_VIRTIO_MMIO=y > > +CONFIG_GOLDFISH=y > > CONFIG_EXT4_FS=y > > CONFIG_EXT4_FS_POSIX_ACL=y > > CONFIG_AUTOFS4_FS=y > > diff --git a/arch/riscv/configs/rv32_defconfig > > b/arch/riscv/configs/rv32_defconfig > > index 7da93e494445..50716c1395aa 100644 > > --- a/arch/riscv/configs/rv32_defconfig > > +++ b/arch/riscv/configs/rv32_defconfig > > @@ -69,7 +69,10 @@ CONFIG_USB_OHCI_HCD=y > > CONFIG_USB_OHCI_HCD_PLATFORM=y CONFIG_USB_STORAGE=y > CONFIG_USB_UAS=y > > +CONFIG_RTC_CLASS=y > > +CONFIG_RTC_DRV_GOLDFISH=y > > CONFIG_VIRTIO_MMIO=y > > +CONFIG_GOLDFISH=y > > CONFIG_SIFIVE_PLIC=y > > CONFIG_EXT4_FS=y > > CONFIG_EXT4_FS_POSIX_ACL=y > > -- > > 2.17.1 > > Reviewed-by: Palmer Dabbelt > > IIRC there was supposed to be a follow-up to your QEMU patch set to rebase > it on top of a refactoring of their RTC code, but I don't see it in my inbox. > LMK > if I missed it, as QEMU's soft freeze is in a few weeks and I'd like to make > sure I get everything in. I was hoping for QEMU RTC refactoring to be merged soon but it has not happened so far. I will wait couple of more days then send v3 of QEMU patches. > > Additionally: we should refactor our Kconfig to have some sort of > CONFIG_SOC_VIRT that selects this stuff, like we have the > CONFIG_SOC_SIFIVE. > This will explicitly document why devices are in the defconfig, avoid > duplicating a bunch of stuff between defconfigs, and provide an example of > how we support multiple SOCs in a single image. Yes, indeed we need CONFIG_SOC_VIRT but this will be a separate patch. > > I don't see why either of these should block merging the patch, though. Thanks, Anup
Re: [PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
On Tue, 24 Sep 2019 23:38:08 PDT (-0700), Anup Patel wrote: We have Goldfish RTC device available on QEMU RISC-V virt machine hence enable required driver in RV32 and RV64 defconfigs. Signed-off-by: Anup Patel --- arch/riscv/configs/defconfig | 3 +++ arch/riscv/configs/rv32_defconfig | 3 +++ 2 files changed, 6 insertions(+) diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig index 3efff552a261..57b4f67b0c0b 100644 --- a/arch/riscv/configs/defconfig +++ b/arch/riscv/configs/defconfig @@ -73,7 +73,10 @@ CONFIG_USB_STORAGE=y CONFIG_USB_UAS=y CONFIG_MMC=y CONFIG_MMC_SPI=y +CONFIG_RTC_CLASS=y +CONFIG_RTC_DRV_GOLDFISH=y CONFIG_VIRTIO_MMIO=y +CONFIG_GOLDFISH=y CONFIG_EXT4_FS=y CONFIG_EXT4_FS_POSIX_ACL=y CONFIG_AUTOFS4_FS=y diff --git a/arch/riscv/configs/rv32_defconfig b/arch/riscv/configs/rv32_defconfig index 7da93e494445..50716c1395aa 100644 --- a/arch/riscv/configs/rv32_defconfig +++ b/arch/riscv/configs/rv32_defconfig @@ -69,7 +69,10 @@ CONFIG_USB_OHCI_HCD=y CONFIG_USB_OHCI_HCD_PLATFORM=y CONFIG_USB_STORAGE=y CONFIG_USB_UAS=y +CONFIG_RTC_CLASS=y +CONFIG_RTC_DRV_GOLDFISH=y CONFIG_VIRTIO_MMIO=y +CONFIG_GOLDFISH=y CONFIG_SIFIVE_PLIC=y CONFIG_EXT4_FS=y CONFIG_EXT4_FS_POSIX_ACL=y -- 2.17.1 Reviewed-by: Palmer Dabbelt IIRC there was supposed to be a follow-up to your QEMU patch set to rebase it on top of a refactoring of their RTC code, but I don't see it in my inbox. LMK if I missed it, as QEMU's soft freeze is in a few weeks and I'd like to make sure I get everything in. Additionally: we should refactor our Kconfig to have some sort of CONFIG_SOC_VIRT that selects this stuff, like we have the CONFIG_SOC_SIFIVE. This will explicitly document why devices are in the defconfig, avoid duplicating a bunch of stuff between defconfigs, and provide an example of how we support multiple SOCs in a single image. I don't see why either of these should block merging the patch, though.
[PATCH v2 2/2] RISC-V: defconfig: Enable Goldfish RTC driver
We have Goldfish RTC device available on QEMU RISC-V virt machine hence enable required driver in RV32 and RV64 defconfigs. Signed-off-by: Anup Patel --- arch/riscv/configs/defconfig | 3 +++ arch/riscv/configs/rv32_defconfig | 3 +++ 2 files changed, 6 insertions(+) diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig index 3efff552a261..57b4f67b0c0b 100644 --- a/arch/riscv/configs/defconfig +++ b/arch/riscv/configs/defconfig @@ -73,7 +73,10 @@ CONFIG_USB_STORAGE=y CONFIG_USB_UAS=y CONFIG_MMC=y CONFIG_MMC_SPI=y +CONFIG_RTC_CLASS=y +CONFIG_RTC_DRV_GOLDFISH=y CONFIG_VIRTIO_MMIO=y +CONFIG_GOLDFISH=y CONFIG_EXT4_FS=y CONFIG_EXT4_FS_POSIX_ACL=y CONFIG_AUTOFS4_FS=y diff --git a/arch/riscv/configs/rv32_defconfig b/arch/riscv/configs/rv32_defconfig index 7da93e494445..50716c1395aa 100644 --- a/arch/riscv/configs/rv32_defconfig +++ b/arch/riscv/configs/rv32_defconfig @@ -69,7 +69,10 @@ CONFIG_USB_OHCI_HCD=y CONFIG_USB_OHCI_HCD_PLATFORM=y CONFIG_USB_STORAGE=y CONFIG_USB_UAS=y +CONFIG_RTC_CLASS=y +CONFIG_RTC_DRV_GOLDFISH=y CONFIG_VIRTIO_MMIO=y +CONFIG_GOLDFISH=y CONFIG_SIFIVE_PLIC=y CONFIG_EXT4_FS=y CONFIG_EXT4_FS_POSIX_ACL=y -- 2.17.1
