Re: [U-Boot] [PATCH v2 3/7] drivers: block: disk-uclass: implement scsi_init()
Hi Mugunthan and Bin, On 14 February 2016 at 20:03, Bin Meng wrote: > Hi Simon, > > On Sun, Feb 7, 2016 at 4:29 AM, Simon Glass wrote: >> Hi Bin, >> >> On 3 February 2016 at 04:59, Mugunthan V N wrote: >>> >>> Implement scsi_init() api to probe driver model based sata >>> devices. >>> >>> Signed-off-by: Mugunthan V N >>> --- >>> drivers/block/disk-uclass.c | 39 +++ >>> 1 file changed, 39 insertions(+) >> >> This patch seems odd to me. I would hope that scsi_init() would go >> away with driver model and it would happen as part of the controller >> probe. But of course we cannot probe SCSI from UCLASS_DISK. I think >> the uclass 'disk' is too broad to be useful. >> > > I agree. I raised similar comment before that this just looks a place > holder for the SCSI stuff. > >> So I am wondering whether the decision to use UCLASS_DISK instead of >> UCLASS_AHCI was a mistake. >> >> Perhaps instead we should have: >> >> UCLASS_AHCI >> UCLASS_SCSI >> UCLASS_MMC >> etc... >> > > I still think UCLASS_AHCI is not a good name. Maybe UCLASS_ATA as we > are talking about protocols here (SCSI, MMC). UCLASS_ATA seems confusing. How would we distinguish IDE and SATA? BTW since your email I have sent a patch to implement block devices. >From what I can tell the above approach will work well. I think our uclasses should mirror the current IF_TYPE values: /* Interface types: */ #define IF_TYPE_UNKNOWN 0 #define IF_TYPE_IDE 1 #define IF_TYPE_SCSI 2 #define IF_TYPE_ATAPI 3 #define IF_TYPE_USB 4 #define IF_TYPE_DOC 5 #define IF_TYPE_MMC 6 #define IF_TYPE_SD 7 #define IF_TYPE_SATA 8 #define IF_TYPE_HOST 9 > >> and each of these devices can have a UCLASS_BLK under them (the block >> device). >> >> Possibly we could even have a dummy UCLASS_DISK device under each of >> the above, but I'm not sure that is useful. >> >> What do you think? >> > > [snip] > > Regards, > Bin Since this patch is presumably destined for the current release and we don't intend to change UCLASS_DISK for that release, I think this patch can go in as is. We can fix it up later. Also note that scsi_get_device() is not the same as scsi_get_dev(). It would be better to use uclass_get_device() though. Reviewed-by: Simon Glass Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/7] drivers: block: disk-uclass: implement scsi_init()
Hi Simon, On Sun, Feb 7, 2016 at 4:29 AM, Simon Glass wrote: > Hi Bin, > > On 3 February 2016 at 04:59, Mugunthan V N wrote: >> >> Implement scsi_init() api to probe driver model based sata >> devices. >> >> Signed-off-by: Mugunthan V N >> --- >> drivers/block/disk-uclass.c | 39 +++ >> 1 file changed, 39 insertions(+) > > This patch seems odd to me. I would hope that scsi_init() would go > away with driver model and it would happen as part of the controller > probe. But of course we cannot probe SCSI from UCLASS_DISK. I think > the uclass 'disk' is too broad to be useful. > I agree. I raised similar comment before that this just looks a place holder for the SCSI stuff. > So I am wondering whether the decision to use UCLASS_DISK instead of > UCLASS_AHCI was a mistake. > > Perhaps instead we should have: > > UCLASS_AHCI > UCLASS_SCSI > UCLASS_MMC > etc... > I still think UCLASS_AHCI is not a good name. Maybe UCLASS_ATA as we are talking about protocols here (SCSI, MMC). > and each of these devices can have a UCLASS_BLK under them (the block device). > > Possibly we could even have a dummy UCLASS_DISK device under each of > the above, but I'm not sure that is useful. > > What do you think? > [snip] Regards, Bin ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/7] drivers: block: disk-uclass: implement scsi_init()
On Mon, Feb 08, 2016 at 10:45:34AM -0700, Simon Glass wrote: > Hi Muganthan, > > On 8 February 2016 at 04:23, Mugunthan V N wrote: > > On Sunday 07 February 2016 01:59 AM, Simon Glass wrote: > >> Hi Bin, > >> > >> On 3 February 2016 at 04:59, Mugunthan V N wrote: > >>> > >>> Implement scsi_init() api to probe driver model based sata > >>> devices. > >>> > >>> Signed-off-by: Mugunthan V N > >>> --- > >>> drivers/block/disk-uclass.c | 39 +++ > >>> 1 file changed, 39 insertions(+) > >> > >> This patch seems odd to me. I would hope that scsi_init() would go > >> away with driver model and it would happen as part of the controller > >> probe. But of course we cannot probe SCSI from UCLASS_DISK. I think > >> the uclass 'disk' is too broad to be useful. > >> > >> So I am wondering whether the decision to use UCLASS_DISK instead of > >> UCLASS_AHCI was a mistake. > >> > >> Perhaps instead we should have: > >> > >> UCLASS_AHCI > >> UCLASS_SCSI > >> UCLASS_MMC > >> etc... > >> > >> and each of these devices can have a UCLASS_BLK under them (the block > >> device). > >> > >> Possibly we could even have a dummy UCLASS_DISK device under each of > >> the above, but I'm not sure that is useful. > >> > >> What do you think? > >> > > > > Hmmm, this sounds a better approach to me also. So that we can abstract > > all bulk related activities to UCLASS_BLK. I can do an RFC if you are okay? > > I have been working on a block uclass and tidying up the partition > stuff. I'll see if I can get patches out next week. In the meantime > I'd like to get comments from Bin and others about this too. > > I suspect we will end up apply what you have (or similar) for this > release, but I'd like to figure out the best approach here. OK. I had marked the series deferred this morning, and moved it back to new now. Given the caveat of needing higher level rework after can you review/ack/nack 3 and 5 as much as you feel comfortable with? Thanks! -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/7] drivers: block: disk-uclass: implement scsi_init()
Hi Muganthan, On 8 February 2016 at 04:23, Mugunthan V N wrote: > On Sunday 07 February 2016 01:59 AM, Simon Glass wrote: >> Hi Bin, >> >> On 3 February 2016 at 04:59, Mugunthan V N wrote: >>> >>> Implement scsi_init() api to probe driver model based sata >>> devices. >>> >>> Signed-off-by: Mugunthan V N >>> --- >>> drivers/block/disk-uclass.c | 39 +++ >>> 1 file changed, 39 insertions(+) >> >> This patch seems odd to me. I would hope that scsi_init() would go >> away with driver model and it would happen as part of the controller >> probe. But of course we cannot probe SCSI from UCLASS_DISK. I think >> the uclass 'disk' is too broad to be useful. >> >> So I am wondering whether the decision to use UCLASS_DISK instead of >> UCLASS_AHCI was a mistake. >> >> Perhaps instead we should have: >> >> UCLASS_AHCI >> UCLASS_SCSI >> UCLASS_MMC >> etc... >> >> and each of these devices can have a UCLASS_BLK under them (the block >> device). >> >> Possibly we could even have a dummy UCLASS_DISK device under each of >> the above, but I'm not sure that is useful. >> >> What do you think? >> > > Hmmm, this sounds a better approach to me also. So that we can abstract > all bulk related activities to UCLASS_BLK. I can do an RFC if you are okay? I have been working on a block uclass and tidying up the partition stuff. I'll see if I can get patches out next week. In the meantime I'd like to get comments from Bin and others about this too. I suspect we will end up apply what you have (or similar) for this release, but I'd like to figure out the best approach here. Thinking about it more I suggest this: UCLASS_AHCI / SCSI / MMC etc. for the controllers each with a UCLASS_DISK child device for each disk attached to the controller each with a (single) UCLASS_BLK child device > > Regards > Mugunthan V N > Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/7] drivers: block: disk-uclass: implement scsi_init()
On Sunday 07 February 2016 01:59 AM, Simon Glass wrote: > Hi Bin, > > On 3 February 2016 at 04:59, Mugunthan V N wrote: >> >> Implement scsi_init() api to probe driver model based sata >> devices. >> >> Signed-off-by: Mugunthan V N >> --- >> drivers/block/disk-uclass.c | 39 +++ >> 1 file changed, 39 insertions(+) > > This patch seems odd to me. I would hope that scsi_init() would go > away with driver model and it would happen as part of the controller > probe. But of course we cannot probe SCSI from UCLASS_DISK. I think > the uclass 'disk' is too broad to be useful. > > So I am wondering whether the decision to use UCLASS_DISK instead of > UCLASS_AHCI was a mistake. > > Perhaps instead we should have: > > UCLASS_AHCI > UCLASS_SCSI > UCLASS_MMC > etc... > > and each of these devices can have a UCLASS_BLK under them (the block device). > > Possibly we could even have a dummy UCLASS_DISK device under each of > the above, but I'm not sure that is useful. > > What do you think? > Hmmm, this sounds a better approach to me also. So that we can abstract all bulk related activities to UCLASS_BLK. I can do an RFC if you are okay? Regards Mugunthan V N ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/7] drivers: block: disk-uclass: implement scsi_init()
Hi Bin,
On 3 February 2016 at 04:59, Mugunthan V N wrote:
>
> Implement scsi_init() api to probe driver model based sata
> devices.
>
> Signed-off-by: Mugunthan V N
> ---
> drivers/block/disk-uclass.c | 39 +++
> 1 file changed, 39 insertions(+)
This patch seems odd to me. I would hope that scsi_init() would go
away with driver model and it would happen as part of the controller
probe. But of course we cannot probe SCSI from UCLASS_DISK. I think
the uclass 'disk' is too broad to be useful.
So I am wondering whether the decision to use UCLASS_DISK instead of
UCLASS_AHCI was a mistake.
Perhaps instead we should have:
UCLASS_AHCI
UCLASS_SCSI
UCLASS_MMC
etc...
and each of these devices can have a UCLASS_BLK under them (the block device).
Possibly we could even have a dummy UCLASS_DISK device under each of
the above, but I'm not sure that is useful.
What do you think?
>
> diff --git a/drivers/block/disk-uclass.c b/drivers/block/disk-uclass.c
> index d665b35..4bd7b56 100644
> --- a/drivers/block/disk-uclass.c
> +++ b/drivers/block/disk-uclass.c
> @@ -7,6 +7,45 @@
>
> #include
> #include
> +#include
> +#include
> +#include
> +
> +int scsi_get_device(int index, struct udevice **devp)
> +{
> + struct udevice *dev;
> + int ret;
> +
> + ret = uclass_find_device(UCLASS_DISK, index, &dev);
> + if (ret || !dev) {
> + printf("%d device not found\n", index);
> + return ret;
> + }
> +
> + ret = device_probe(dev);
> + if (ret) {
> + error("device probe error\n");
> + return ret;
> + }
> +
> + *devp = dev;
> +
> + return ret;
> +}
> +
> +void scsi_init(void)
> +{
> + struct udevice *dev;
> + int ret;
> +
> + ret = scsi_get_device(0, &dev);
> + if (ret || !dev) {
> + error("scsi device not found\n");
> + return;
> + }
> +
> + scsi_scan(1);
> +}
>
> UCLASS_DRIVER(disk) = {
> .id = UCLASS_DISK,
> --
> 2.7.0.75.g3ee1e0f
>
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 3/7] drivers: block: disk-uclass: implement scsi_init()
Implement scsi_init() api to probe driver model based sata
devices.
Signed-off-by: Mugunthan V N
---
drivers/block/disk-uclass.c | 39 +++
1 file changed, 39 insertions(+)
diff --git a/drivers/block/disk-uclass.c b/drivers/block/disk-uclass.c
index d665b35..4bd7b56 100644
--- a/drivers/block/disk-uclass.c
+++ b/drivers/block/disk-uclass.c
@@ -7,6 +7,45 @@
#include
#include
+#include
+#include
+#include
+
+int scsi_get_device(int index, struct udevice **devp)
+{
+ struct udevice *dev;
+ int ret;
+
+ ret = uclass_find_device(UCLASS_DISK, index, &dev);
+ if (ret || !dev) {
+ printf("%d device not found\n", index);
+ return ret;
+ }
+
+ ret = device_probe(dev);
+ if (ret) {
+ error("device probe error\n");
+ return ret;
+ }
+
+ *devp = dev;
+
+ return ret;
+}
+
+void scsi_init(void)
+{
+ struct udevice *dev;
+ int ret;
+
+ ret = scsi_get_device(0, &dev);
+ if (ret || !dev) {
+ error("scsi device not found\n");
+ return;
+ }
+
+ scsi_scan(1);
+}
UCLASS_DRIVER(disk) = {
.id = UCLASS_DISK,
--
2.7.0.75.g3ee1e0f
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
