Re: [PATCH v3] bootstd: sata: Add bootstd support for ahci sata
Hi Simon,
On Sun, Sep 10, 2023 at 3:37 PM Simon Glass wrote:
>
> Hi Tony,
>
> On Fri, 8 Sept 2023 at 13:10, Tony Dinh wrote:
> >
> > Add ahci sata bootdev and corresponding hunting function.
> >
> > Signed-off-by: Tony Dinh
> > ---
> >
> > Changes in v3:
> > - Correct drivers/ata/Makefile to compile sata_bootdev only if
> > ahci sata is enabled.
> >
> > Changes in v2:
> > - set devtype to sata in bootmeth_script for non-scsi SATA device.
> >
> > boot/bootmeth_script.c | 12 ++--
> > drivers/ata/Makefile | 2 +-
> > drivers/ata/sata.c | 25 +++
> > drivers/ata/sata_bootdev.c | 62 ++
> > include/sata.h | 1 +
> > 5 files changed, 99 insertions(+), 3 deletions(-)
> > create mode 100644 drivers/ata/sata_bootdev.c
> >
> > diff --git a/boot/bootmeth_script.c b/boot/bootmeth_script.c
> > index 58c57a2d4b..0269e0f9b0 100644
> > --- a/boot/bootmeth_script.c
> > +++ b/boot/bootmeth_script.c
> > @@ -190,10 +190,18 @@ static int script_boot(struct udevice *dev, struct
> > bootflow *bflow)
> > ulong addr;
> > int ret;
> >
> > - if (desc->uclass_id == UCLASS_USB)
> > + if (desc->uclass_id == UCLASS_USB) {
> > ret = env_set("devtype", "usb");
> > - else
> > + } else {
> > ret = env_set("devtype", blk_get_devtype(bflow->blk));
> > +
> > + /* If the parent uclass is AHCI, but the driver is ATA
> > +* (not scsi), set devtype to sata
> > +*/
> > + if (!ret && IS_ENABLED(CONFIG_SATA) &&
> > + !strcmp("ahci", env_get("devtype")))
>
> Can you not use desc->uclass_id to do the right thing here? It seems
> odd to compare with the value of the env var you just set up above.
Indeed, I must have been too caught up with the env :)
>
> > + ret = env_set("devtype", "sata");
> > + }
> > if (!ret)
> > ret = env_set_hex("devnum", desc->devnum);
> > if (!ret)
> > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> > index 6e30180b8b..0b6f91098a 100644
> > --- a/drivers/ata/Makefile
> > +++ b/drivers/ata/Makefile
> > @@ -10,7 +10,7 @@ obj-$(CONFIG_SCSI_AHCI) += ahci.o
> > obj-$(CONFIG_DWC_AHSATA) += dwc_ahsata.o
> > obj-$(CONFIG_FSL_SATA) += fsl_sata.o
> > obj-$(CONFIG_LIBATA) += libata.o
> > -obj-$(CONFIG_SATA) += sata.o
> > +obj-$(CONFIG_SATA) += sata.o sata_bootdev.o
> > obj-$(CONFIG_SATA_CEVA) += sata_ceva.o
> > obj-$(CONFIG_SATA_MV) += sata_mv.o
> > obj-$(CONFIG_SATA_SIL) += sata_sil.o
> > diff --git a/drivers/ata/sata.c b/drivers/ata/sata.c
> > index ce3e9b5a40..9da7218564 100644
> > --- a/drivers/ata/sata.c
> > +++ b/drivers/ata/sata.c
> > @@ -15,6 +15,8 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> >
> > #ifndef CONFIG_AHCI
> > struct blk_desc sata_dev_desc[CONFIG_SYS_SATA_MAX_DEVICE];
> > @@ -50,6 +52,29 @@ int sata_scan(struct udevice *dev)
> > return ops->scan(dev);
> > }
> >
> > +int sata_rescan(bool verbose)
> > +{
> > + struct udevice *dev;
> > + int devnum = 0;
> > + int ret;
> > +
> > + /* Find and probing the SATA controller */
> > + ret = uclass_get_device(UCLASS_AHCI, devnum, &dev);
>
> This does not actually rescan...if the device has already been probed,
> it won't be probed again. You will need to use the logic like in
> sata_remove().
>
> Also, please avoid using devnum. You should probably just use
> uclass_probe_all(UCLASS_AHCI)
OK.
>
> > +
> > + /* Sanity check */
> > + if (ret)
> > + ret = uclass_find_first_device(UCLASS_AHCI, &dev);
>
> That just finds it, but does not probe it. See uclass_first_device_err()
>
> > + if (ret) {
> > + printf("Cannot probe SATA device %d (err=%d)\n", devnum,
> > ret);
> > + return -ENOSYS;
>
> return ret
>
> > + }
> > + if (!dev) {
> > + printf("No SATA device found!\n");
> > + return -ENOSYS;
> > + }
> > + return 0;
> > +}
> > +
> > #ifndef CONFIG_AHCI
> > #ifdef CONFIG_PARTITIONS
> > struct blk_desc *sata_get_dev(int dev)
> > diff --git a/drivers/ata/sata_bootdev.c b/drivers/ata/sata_bootdev.c
> > new file mode 100644
> > index 00..f638493ce0
> > --- /dev/null
> > +++ b/drivers/ata/sata_bootdev.c
> > @@ -0,0 +1,62 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Bootdev for sata
> > + *
> > + * Copyright 2023 Tony Dinh
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +static int sata_bootdev_bind(struct udevice *dev)
> > +{
> > + struct bootdev_uc_plat *ucp = dev_get_uclass_plat(dev);
> > +
> > + ucp->prio = BOOTDEVP_4_SCAN_FAST;
> > +
> > + return 0;
> > +}
> > +
> > +static int sata_bootdev_hunt(struct bootdev_hunter *info, bool show)
> > +{
> > + int ret;
> > +
> > +
Re: [PATCH v3] bootstd: sata: Add bootstd support for ahci sata
Hi Tony,
On Fri, 8 Sept 2023 at 13:10, Tony Dinh wrote:
>
> Add ahci sata bootdev and corresponding hunting function.
>
> Signed-off-by: Tony Dinh
> ---
>
> Changes in v3:
> - Correct drivers/ata/Makefile to compile sata_bootdev only if
> ahci sata is enabled.
>
> Changes in v2:
> - set devtype to sata in bootmeth_script for non-scsi SATA device.
>
> boot/bootmeth_script.c | 12 ++--
> drivers/ata/Makefile | 2 +-
> drivers/ata/sata.c | 25 +++
> drivers/ata/sata_bootdev.c | 62 ++
> include/sata.h | 1 +
> 5 files changed, 99 insertions(+), 3 deletions(-)
> create mode 100644 drivers/ata/sata_bootdev.c
>
> diff --git a/boot/bootmeth_script.c b/boot/bootmeth_script.c
> index 58c57a2d4b..0269e0f9b0 100644
> --- a/boot/bootmeth_script.c
> +++ b/boot/bootmeth_script.c
> @@ -190,10 +190,18 @@ static int script_boot(struct udevice *dev, struct
> bootflow *bflow)
> ulong addr;
> int ret;
>
> - if (desc->uclass_id == UCLASS_USB)
> + if (desc->uclass_id == UCLASS_USB) {
> ret = env_set("devtype", "usb");
> - else
> + } else {
> ret = env_set("devtype", blk_get_devtype(bflow->blk));
> +
> + /* If the parent uclass is AHCI, but the driver is ATA
> +* (not scsi), set devtype to sata
> +*/
> + if (!ret && IS_ENABLED(CONFIG_SATA) &&
> + !strcmp("ahci", env_get("devtype")))
Can you not use desc->uclass_id to do the right thing here? It seems
odd to compare with the value of the env var you just set up above.
> + ret = env_set("devtype", "sata");
> + }
> if (!ret)
> ret = env_set_hex("devnum", desc->devnum);
> if (!ret)
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 6e30180b8b..0b6f91098a 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -10,7 +10,7 @@ obj-$(CONFIG_SCSI_AHCI) += ahci.o
> obj-$(CONFIG_DWC_AHSATA) += dwc_ahsata.o
> obj-$(CONFIG_FSL_SATA) += fsl_sata.o
> obj-$(CONFIG_LIBATA) += libata.o
> -obj-$(CONFIG_SATA) += sata.o
> +obj-$(CONFIG_SATA) += sata.o sata_bootdev.o
> obj-$(CONFIG_SATA_CEVA) += sata_ceva.o
> obj-$(CONFIG_SATA_MV) += sata_mv.o
> obj-$(CONFIG_SATA_SIL) += sata_sil.o
> diff --git a/drivers/ata/sata.c b/drivers/ata/sata.c
> index ce3e9b5a40..9da7218564 100644
> --- a/drivers/ata/sata.c
> +++ b/drivers/ata/sata.c
> @@ -15,6 +15,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #ifndef CONFIG_AHCI
> struct blk_desc sata_dev_desc[CONFIG_SYS_SATA_MAX_DEVICE];
> @@ -50,6 +52,29 @@ int sata_scan(struct udevice *dev)
> return ops->scan(dev);
> }
>
> +int sata_rescan(bool verbose)
> +{
> + struct udevice *dev;
> + int devnum = 0;
> + int ret;
> +
> + /* Find and probing the SATA controller */
> + ret = uclass_get_device(UCLASS_AHCI, devnum, &dev);
This does not actually rescan...if the device has already been probed,
it won't be probed again. You will need to use the logic like in
sata_remove().
Also, please avoid using devnum. You should probably just use
uclass_probe_all(UCLASS_AHCI)
> +
> + /* Sanity check */
> + if (ret)
> + ret = uclass_find_first_device(UCLASS_AHCI, &dev);
That just finds it, but does not probe it. See uclass_first_device_err()
> + if (ret) {
> + printf("Cannot probe SATA device %d (err=%d)\n", devnum, ret);
> + return -ENOSYS;
return ret
> + }
> + if (!dev) {
> + printf("No SATA device found!\n");
> + return -ENOSYS;
> + }
> + return 0;
> +}
> +
> #ifndef CONFIG_AHCI
> #ifdef CONFIG_PARTITIONS
> struct blk_desc *sata_get_dev(int dev)
> diff --git a/drivers/ata/sata_bootdev.c b/drivers/ata/sata_bootdev.c
> new file mode 100644
> index 00..f638493ce0
> --- /dev/null
> +++ b/drivers/ata/sata_bootdev.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Bootdev for sata
> + *
> + * Copyright 2023 Tony Dinh
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static int sata_bootdev_bind(struct udevice *dev)
> +{
> + struct bootdev_uc_plat *ucp = dev_get_uclass_plat(dev);
> +
> + ucp->prio = BOOTDEVP_4_SCAN_FAST;
> +
> + return 0;
> +}
> +
> +static int sata_bootdev_hunt(struct bootdev_hunter *info, bool show)
> +{
> + int ret;
> +
> + if (IS_ENABLED(CONFIG_PCI)) {
> + ret = pci_init();
> + if (ret)
> + return ret;
> + }
> +
> + ret = sata_rescan(true);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +struct bootdev_ops sata_bootdev_ops = {
> +};
> +
> +static const struct udevice_id sata_bootdev_ids[] = {
> + { .compatible = "u-boot,bootdev-sata" },
> +
