diff mbox series

[1/1] riscv: enable SATA disk on qemu-riscv64_defconfig

Message ID 20201102113729.40012-1-xypron.glpk@gmx.de
State Changes Requested, archived
Delegated to: Andes
Headers show
Series [1/1] riscv: enable SATA disk on qemu-riscv64_defconfig | expand

Commit Message

Heinrich Schuchardt Nov. 2, 2020, 11:37 a.m. UTC 
Allow attaching a virtual SATA disk to qemu-riscv64_defconfig.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 configs/qemu-riscv64_defconfig | 6 ++++++
 1 file changed, 6 insertions(+)

--
2.28.0

Comments

Rick Chen Nov. 4, 2020, 2:12 a.m. UTC | #1 
> From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Heinrich Schuchardt
> Sent: Monday, November 02, 2020 7:37 PM
> To: Bin Meng
> Cc: u-boot@lists.denx.de; Heinrich Schuchardt
> Subject: [PATCH 1/1] riscv: enable SATA disk on qemu-riscv64_defconfig
>
> Allow attaching a virtual SATA disk to qemu-riscv64_defconfig.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  configs/qemu-riscv64_defconfig | 6 ++++++
>  1 file changed, 6 insertions(+)
>

Reviewed-by: Rick Chen <rick@andestech.com>
Bin Meng Nov. 4, 2020, 2:44 a.m. UTC | #2 
Hi Heinrich,

On Mon, Nov 2, 2020 at 7:37 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Allow attaching a virtual SATA disk to qemu-riscv64_defconfig.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  configs/qemu-riscv64_defconfig | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
> index daf5d655d0..a1426a9506 100644
> --- a/configs/qemu-riscv64_defconfig
> +++ b/configs/qemu-riscv64_defconfig
> @@ -1,15 +1,21 @@
>  CONFIG_RISCV=y
>  CONFIG_NR_DRAM_BANKS=1
>  CONFIG_ENV_SIZE=0x20000
> +CONFIG_AHCI=y
>  CONFIG_TARGET_QEMU_VIRT=y
>  CONFIG_ARCH_RV64I=y
>  CONFIG_DISTRO_DEFAULTS=y
>  CONFIG_FIT=y
>  CONFIG_DISPLAY_CPUINFO=y
>  CONFIG_DISPLAY_BOARDINFO=y
> +CONFIG_PCI_INIT_R=y
>  CONFIG_CMD_BOOTEFI_SELFTEST=y
>  CONFIG_CMD_NVEDIT_EFI=y
>  # CONFIG_CMD_MII is not set
>  CONFIG_OF_PRIOR_STAGE=y
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> +CONFIG_SCSI_AHCI=y
> +CONFIG_AHCI_PCI=y
>  CONFIG_DM_MTD=y
> +CONFIG_SCSI=y
> +CONFIG_DM_SCSI=y

Please update BOARD_SPECIFIC_OPTIONS instead of the defconfig file.

Note NVMe is already enabled on this board. Why is SATA controller needed?

Regards,
Bin
Bin Meng Nov. 4, 2020, 2:45 a.m. UTC | #3 
On Wed, Nov 4, 2020 at 10:44 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Heinrich,
>
> On Mon, Nov 2, 2020 at 7:37 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > Allow attaching a virtual SATA disk to qemu-riscv64_defconfig.
> >
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >  configs/qemu-riscv64_defconfig | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
> > index daf5d655d0..a1426a9506 100644
> > --- a/configs/qemu-riscv64_defconfig
> > +++ b/configs/qemu-riscv64_defconfig
> > @@ -1,15 +1,21 @@
> >  CONFIG_RISCV=y
> >  CONFIG_NR_DRAM_BANKS=1
> >  CONFIG_ENV_SIZE=0x20000
> > +CONFIG_AHCI=y
> >  CONFIG_TARGET_QEMU_VIRT=y
> >  CONFIG_ARCH_RV64I=y
> >  CONFIG_DISTRO_DEFAULTS=y
> >  CONFIG_FIT=y
> >  CONFIG_DISPLAY_CPUINFO=y
> >  CONFIG_DISPLAY_BOARDINFO=y
> > +CONFIG_PCI_INIT_R=y
> >  CONFIG_CMD_BOOTEFI_SELFTEST=y
> >  CONFIG_CMD_NVEDIT_EFI=y
> >  # CONFIG_CMD_MII is not set
> >  CONFIG_OF_PRIOR_STAGE=y
> >  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> > +CONFIG_SCSI_AHCI=y
> > +CONFIG_AHCI_PCI=y
> >  CONFIG_DM_MTD=y
> > +CONFIG_SCSI=y
> > +CONFIG_DM_SCSI=y
>
> Please update BOARD_SPECIFIC_OPTIONS instead of the defconfig file.
>
> Note NVMe is already enabled on this board. Why is SATA controller needed?

+Rick Chen
Heinrich Schuchardt Nov. 4, 2020, 7:25 a.m. UTC | #4 
On 11/4/20 3:45 AM, Bin Meng wrote:
> On Wed, Nov 4, 2020 at 10:44 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> Hi Heinrich,
>>
>> On Mon, Nov 2, 2020 at 7:37 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>
>>> Allow attaching a virtual SATA disk to qemu-riscv64_defconfig.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>  configs/qemu-riscv64_defconfig | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
>>> index daf5d655d0..a1426a9506 100644
>>> --- a/configs/qemu-riscv64_defconfig
>>> +++ b/configs/qemu-riscv64_defconfig
>>> @@ -1,15 +1,21 @@
>>>  CONFIG_RISCV=y
>>>  CONFIG_NR_DRAM_BANKS=1
>>>  CONFIG_ENV_SIZE=0x20000
>>> +CONFIG_AHCI=y
>>>  CONFIG_TARGET_QEMU_VIRT=y
>>>  CONFIG_ARCH_RV64I=y
>>>  CONFIG_DISTRO_DEFAULTS=y
>>>  CONFIG_FIT=y
>>>  CONFIG_DISPLAY_CPUINFO=y
>>>  CONFIG_DISPLAY_BOARDINFO=y
>>> +CONFIG_PCI_INIT_R=y
>>>  CONFIG_CMD_BOOTEFI_SELFTEST=y
>>>  CONFIG_CMD_NVEDIT_EFI=y
>>>  # CONFIG_CMD_MII is not set
>>>  CONFIG_OF_PRIOR_STAGE=y
>>>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>>> +CONFIG_SCSI_AHCI=y
>>> +CONFIG_AHCI_PCI=y
>>>  CONFIG_DM_MTD=y
>>> +CONFIG_SCSI=y
>>> +CONFIG_DM_SCSI=y
>>
>> Please update BOARD_SPECIFIC_OPTIONS instead of the defconfig file.

I searched the git repository for "BOARD_SPECIFIC_OPTIONS". I only find
it as undocumented string in Kconfig files.

Please, document what it is meant to be used for.

Why do you prefer the undocumented BOARD_SPECIFIC_OPTIONS over defconfig?

>>
>> Note NVMe is already enabled on this board. Why is SATA controller needed?

Why should it be disabled?

I want to be able to run QEMU with:

        -drive if=none,file=riscv64.img,format=raw,id=mydisk \
        -device ich9-ahci,id=ahci -device ide-hd,drive=mydisk,bus=ahci.0

Another use case: emulated CD-ROM drives cannot be NVMe.

Best regards

Heinrich
Bin Meng Nov. 4, 2020, 7:28 a.m. UTC | #5 
Hi Heinrich,

On Wed, Nov 4, 2020 at 3:26 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/4/20 3:45 AM, Bin Meng wrote:
> > On Wed, Nov 4, 2020 at 10:44 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >> Hi Heinrich,
> >>
> >> On Mon, Nov 2, 2020 at 7:37 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>
> >>> Allow attaching a virtual SATA disk to qemu-riscv64_defconfig.
> >>>
> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> ---
> >>>  configs/qemu-riscv64_defconfig | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
> >>> index daf5d655d0..a1426a9506 100644
> >>> --- a/configs/qemu-riscv64_defconfig
> >>> +++ b/configs/qemu-riscv64_defconfig
> >>> @@ -1,15 +1,21 @@
> >>>  CONFIG_RISCV=y
> >>>  CONFIG_NR_DRAM_BANKS=1
> >>>  CONFIG_ENV_SIZE=0x20000
> >>> +CONFIG_AHCI=y
> >>>  CONFIG_TARGET_QEMU_VIRT=y
> >>>  CONFIG_ARCH_RV64I=y
> >>>  CONFIG_DISTRO_DEFAULTS=y
> >>>  CONFIG_FIT=y
> >>>  CONFIG_DISPLAY_CPUINFO=y
> >>>  CONFIG_DISPLAY_BOARDINFO=y
> >>> +CONFIG_PCI_INIT_R=y
> >>>  CONFIG_CMD_BOOTEFI_SELFTEST=y
> >>>  CONFIG_CMD_NVEDIT_EFI=y
> >>>  # CONFIG_CMD_MII is not set
> >>>  CONFIG_OF_PRIOR_STAGE=y
> >>>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> >>> +CONFIG_SCSI_AHCI=y
> >>> +CONFIG_AHCI_PCI=y
> >>>  CONFIG_DM_MTD=y
> >>> +CONFIG_SCSI=y
> >>> +CONFIG_DM_SCSI=y
> >>
> >> Please update BOARD_SPECIFIC_OPTIONS instead of the defconfig file.
>
> I searched the git repository for "BOARD_SPECIFIC_OPTIONS". I only find
> it as undocumented string in Kconfig files.
>
> Please, document what it is meant to be used for.
>
> Why do you prefer the undocumented BOARD_SPECIFIC_OPTIONS over defconfig?

This is what current qemu-riscv boards use. We need to follow the
convention here.

>
> >>
> >> Note NVMe is already enabled on this board. Why is SATA controller needed?
>
> Why should it be disabled?

Please document the QEMU command line in the board doc.

>
> I want to be able to run QEMU with:
>
>         -drive if=none,file=riscv64.img,format=raw,id=mydisk \
>         -device ich9-ahci,id=ahci -device ide-hd,drive=mydisk,bus=ahci.0
>
> Another use case: emulated CD-ROM drives cannot be NVMe.

Regards,
Bin
Heinrich Schuchardt Nov. 4, 2020, 9:14 a.m. UTC | #6 
Hello Bin,

why do we have

imply SPL_RAM_SUPPORT
imply SPL_RAM_DEVICE

in file board/emulation/qemu-riscv/Kconfig which have no effect because
CONFIG_RAM is not selected by any qemu-riscv*_defconfig?

Should we add a line "imply RAM"?

Best regards

Heinrich
Bin Meng Nov. 5, 2020, 7:52 a.m. UTC | #7 
Hi Heinrich,

On Wed, Nov 4, 2020 at 5:14 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Hello Bin,
>
> why do we have
>
> imply SPL_RAM_SUPPORT
> imply SPL_RAM_DEVICE
>
> in file board/emulation/qemu-riscv/Kconfig which have no effect because
> CONFIG_RAM is not selected by any qemu-riscv*_defconfig?
>
> Should we add a line "imply RAM"?
>

Thanks for reporting.

No we should not add "imply RAM". Instead we should add "if SPL". I
will send a patch soon.

Regards,
Bin
diff mbox series

Patch

diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
index daf5d655d0..a1426a9506 100644
--- a/configs/qemu-riscv64_defconfig
+++ b/configs/qemu-riscv64_defconfig
@@ -1,15 +1,21 @@ 
 CONFIG_RISCV=y
 CONFIG_NR_DRAM_BANKS=1
 CONFIG_ENV_SIZE=0x20000
+CONFIG_AHCI=y
 CONFIG_TARGET_QEMU_VIRT=y
 CONFIG_ARCH_RV64I=y
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_DISPLAY_CPUINFO=y
 CONFIG_DISPLAY_BOARDINFO=y
+CONFIG_PCI_INIT_R=y
 CONFIG_CMD_BOOTEFI_SELFTEST=y
 CONFIG_CMD_NVEDIT_EFI=y
 # CONFIG_CMD_MII is not set
 CONFIG_OF_PRIOR_STAGE=y
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
+CONFIG_SCSI_AHCI=y
+CONFIG_AHCI_PCI=y
 CONFIG_DM_MTD=y
+CONFIG_SCSI=y
+CONFIG_DM_SCSI=y