Re: [PATCH v3 11/19] dm: disk: add UCLASS_PARTITION

Heinrich Schuchardt
2022-04-14T07:13:38Z

On 4/14/22 08:17, AKASHI Takahiro wrote:
> On Wed, Apr 13, 2022 at 12:16:00PM +0900, AKASHI Takahiro wrote:
>> Heinrich,
>> (and Simon)
>>
>> On Sat, Apr 09, 2022 at 09:05:07PM +0200, Heinrich Schuchardt wrote:
>>> On 3/8/22 12:36, AKASHI Takahiro wrote:
>>>> With this new function, UCLASS_PARTITION devices will be created as
>>>> child nodes of UCLASS_BLK device.
>>>>
>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>> ---
>>>> disk/Makefile | 3 +
>>>> disk/disk-uclass.c | 153 +++++++++++++++++++++++++++++++++++++++++
>>>> include/dm/uclass-id.h | 1 +
>>>> include/part.h | 11 +++
>>>> 4 files changed, 168 insertions(+)
>>>> create mode 100644 disk/disk-uclass.c
>>>>
>>>> diff --git a/disk/Makefile b/disk/Makefile
>>>> index 6ce5a687b36c..ec37b74f5f40 100644
>>>> --- a/disk/Makefile
>>>> +++ b/disk/Makefile
>>>> @@ -6,6 +6,9 @@
>>>> #ccflags-y += -DET_DEBUG -DDEBUG
>>>>
>>>> obj-$(CONFIG_PARTITIONS) += part.o
>>>> +ifdef CONFIG_$(SPL_)BLK
>>>> +obj-$(CONFIG_PARTITIONS) += disk-uclass.o
>>>
>>> phycore-rk3288_defconfig has CONFIG_SPL_LIBDISK_SUPPORT=n ("Support disk
>>> partitions").
>>
>> I think that SPL_LIBDISK_SUPPORT no longer make any sense and is obsolete.
>>
>>> But spl/disk/disk-uclass is built.
>>
>> Right, but what is strange is:
>> - SPL_PARTITION is enabled by "default y if PARTITONS", while
>> - none of any particular partition types is enabled for spl
>> due to *explicit* disablement of
>> # CONFIG_SPL_DOS_PARTITION is not set
>> # CONFIG_SPL_EFI_PARTITION is not set
>> in phycore-rk3288_defconfig.
>>
>> I think that disk/Kconfig should be revised.
>
> I still believe so, but we can also fix the issue specifically on rk3288
> by adding
> # CONFIG_SPL_PARTITIONS is not set
> # CONFIG_TPL_PARTITIONS is not set
> to phycore-rk3288_defconfig.
>
> Are you satisfied with this solution?

I don't care too much about that specific board. We should avoid
unnecessary SPL size increase for all boards.

The fix should be on the Kconfig level and not on the board level.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>> -Takahiro Akashi
>>
>>> This leads to an unnecessary size increase of the SPL image.
>>> With this size increase building fails.
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>> +endif
>>>> obj-$(CONFIG_$(SPL_)MAC_PARTITION) += part_mac.o
>>>> obj-$(CONFIG_$(SPL_)DOS_PARTITION) += part_dos.o
>>>> obj-$(CONFIG_$(SPL_)ISO_PARTITION) += part_iso.o
>>>> diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
>>>> new file mode 100644
>>>> index 000000000000..4918a2f72d1e
>>>> --- /dev/null
>>>> +++ b/disk/disk-uclass.c
>>>> @@ -0,0 +1,153 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Software partition device (UCLASS_PARTITION)
>>>> + *
>>>> + * Copyright (c) 2021 Linaro Limited
>>>> + * Author: AKASHI Takahiro
>>>> + */
>>>> +
>>>> +#define LOG_CATEGORY UCLASS_PARTITION
>>>> +
>>>> +#include <blk.h>
>>>> +#include <dm.h>
>>>> +#include <log.h>
>>>> +#include <part.h>
>>>> +#include <vsprintf.h>
>>>> +#include <dm/device-internal.h>
>>>> +#include <dm/lists.h>
>>>> +
>>>> +int part_create_block_devices(struct udevice *blk_dev)
>>>> +{
>>>> + int part, count;
>>>> + struct blk_desc *desc = dev_get_uclass_plat(blk_dev);
>>>> + struct disk_partition info;
>>>> + struct disk_part *part_data;
>>>> + char devname[32];
>>>> + struct udevice *dev;
>>>> + int ret;
>>>> +
>>>> + if (!CONFIG_IS_ENABLED(PARTITIONS) ||
>>>> + !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
>>>> + return 0;
>>>> +
>>>> + if (device_get_uclass_id(blk_dev) != UCLASS_BLK)
>>>> + return 0;
>>>> +
>>>> + /* Add devices for each partition */
>>>> + for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
>>>> + if (part_get_info(desc, part, &info))
>>>> + continue;
>>>> + snprintf(devname, sizeof(devname), "%s:%d", blk_dev->name,
>>>> + part);
>>>> +
>>>> + ret = device_bind_driver(blk_dev, "blk_partition",
>>>> + strdup(devname), &dev);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + part_data = dev_get_uclass_plat(dev);
>>>> + part_data->partnum = part;
>>>> + part_data->gpt_part_info = info;
>>>> + count++;
>>>> +
>>>> + ret = device_probe(dev);
>>>> + if (ret) {
>>>> + debug("Can't probe\n");
>>>> + count--;
>>>> + device_unbind(dev);
>>>> +
>>>> + continue;
>>>> + }
>>>> + }
>>>> + debug("%s: %d partitions found in %s\n", __func__, count,
>>>> + blk_dev->name);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static ulong blk_part_read(struct udevice *dev, lbaint_t start,
>>>> + lbaint_t blkcnt, void *buffer)
>>>> +{
>>>> + struct udevice *parent;
>>>> + struct disk_part *part;
>>>> + const struct blk_ops *ops;
>>>> +
>>>> + parent = dev_get_parent(dev);
>>>> + ops = blk_get_ops(parent);
>>>> + if (!ops->read)
>>>> + return -ENOSYS;
>>>> +
>>>> + part = dev_get_uclass_plat(dev);
>>>> + if (start >= part->gpt_part_info.size)
>>>> + return 0;
>>>> +
>>>> + if ((start + blkcnt) > part->gpt_part_info.size)
>>>> + blkcnt = part->gpt_part_info.size - start;
>>>> + start += part->gpt_part_info.start;
>>>> +
>>>> + return ops->read(parent, start, blkcnt, buffer);
>>>> +}
>>>> +
>>>> +static ulong blk_part_write(struct udevice *dev, lbaint_t start,
>>>> + lbaint_t blkcnt, const void *buffer)
>>>> +{
>>>> + struct udevice *parent;
>>>> + struct disk_part *part;
>>>> + const struct blk_ops *ops;
>>>> +
>>>> + parent = dev_get_parent(dev);
>>>> + ops = blk_get_ops(parent);
>>>> + if (!ops->write)
>>>> + return -ENOSYS;
>>>> +
>>>> + part = dev_get_uclass_plat(dev);
>>>> + if (start >= part->gpt_part_info.size)
>>>> + return 0;
>>>> +
>>>> + if ((start + blkcnt) > part->gpt_part_info.size)
>>>> + blkcnt = part->gpt_part_info.size - start;
>>>> + start += part->gpt_part_info.start;
>>>> +
>>>> + return ops->write(parent, start, blkcnt, buffer);
>>>> +}
>>>> +
>>>> +static ulong blk_part_erase(struct udevice *dev, lbaint_t start,
>>>> + lbaint_t blkcnt)
>>>> +{
>>>> + struct udevice *parent;
>>>> + struct disk_part *part;
>>>> + const struct blk_ops *ops;
>>>> +
>>>> + parent = dev_get_parent(dev);
>>>> + ops = blk_get_ops(parent);
>>>> + if (!ops->erase)
>>>> + return -ENOSYS;
>>>> +
>>>> + part = dev_get_uclass_plat(dev);
>>>> + if (start >= part->gpt_part_info.size)
>>>> + return 0;
>>>> +
>>>> + if ((start + blkcnt) > part->gpt_part_info.size)
>>>> + blkcnt = part->gpt_part_info.size - start;
>>>> + start += part->gpt_part_info.start;
>>>> +
>>>> + return ops->erase(parent, start, blkcnt);
>>>> +}
>>>> +
>>>> +static const struct blk_ops blk_part_ops = {
>>>> + .read = blk_part_read,
>>>> + .write = blk_part_write,
>>>> + .erase = blk_part_erase,
>>>> +};
>>>> +
>>>> +U_BOOT_DRIVER(blk_partition) = {
>>>> + .name = "blk_partition",
>>>> + .id = UCLASS_PARTITION,
>>>> + .ops = &blk_part_ops,
>>>> +};
>>>> +
>>>> +UCLASS_DRIVER(partition) = {
>>>> + .id = UCLASS_PARTITION,
>>>> + .per_device_plat_auto = sizeof(struct disk_part),
>>>> + .name = "partition",
>>>> +};
>>>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>>>> index 0e26e1d13824..230b1ea528cf 100644
>>>> --- a/include/dm/uclass-id.h
>>>> +++ b/include/dm/uclass-id.h
>>>> @@ -83,6 +83,7 @@ enum uclass_id {
>>>> UCLASS_P2SB, /* (x86) Primary-to-Sideband Bus */
>>>> UCLASS_PANEL, /* Display panel, such as an LCD */
>>>> UCLASS_PANEL_BACKLIGHT, /* Backlight controller for panel */
>>>> + UCLASS_PARTITION, /* Logical disk partition device */
>>>> UCLASS_PCH, /* x86 platform controller hub */
>>>> UCLASS_PCI, /* PCI bus */
>>>> UCLASS_PCI_EP, /* PCI endpoint device */
>>>> diff --git a/include/part.h b/include/part.h
>>>> index 53cfbdd87671..95e30e60af10 100644
>>>> --- a/include/part.h
>>>> +++ b/include/part.h
>>>> @@ -253,6 +253,17 @@ void part_set_generic_name(const struct blk_desc *dev_desc,
>>>> int part_num, char *name);
>>>>
>>>> extern const struct block_drvr block_drvr[];
>>>> +
>>>> +struct udevice;
>>>> +/**
>>>> + * part_create_block_devices - Create block devices for disk partitions
>>>> + *
>>>> + * Create UCLASS_PARTITION udevices for each of disk partitions in @parent
>>>> + *
>>>> + * @blk_dev: Whole disk device
>>>> + */
>>>> +int part_create_block_devices(struct udevice *blk_dev);
>>>> +
>>>> #else
>>>> static inline struct blk_desc *blk_get_dev(const char *ifname, int dev)
>>>> { return NULL; }
>>>