Message ID | 20210104155031.9b4e39ff48a6d7accc93461d@gmx.com (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
Series | [1/6] Revert "MIPS: Remove unused R4300 CPU support" | expand |
On 1/4/21 6:50 AM, Lauri Kasanen wrote: > Signed-off-by: Lauri Kasanen <cand@gmx.com> > --- > drivers/block/Kconfig | 6 ++ > drivers/block/Makefile | 1 + > drivers/block/n64cart.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 210 insertions(+) > create mode 100644 drivers/block/n64cart.c > > block folks: rest of the series is on linux-mips. Being a > mips-specific driver, not sure which tree it should go to. It should definitely get reviewed first. One easy question - there's no commit message in this one. It's a new driver, it should include some documentation on what it is, etc.
On Mon, 4 Jan 2021 08:40:10 -0700 Jens Axboe <axboe@kernel.dk> wrote: > On 1/4/21 6:50 AM, Lauri Kasanen wrote: > > Signed-off-by: Lauri Kasanen <cand@gmx.com> > > --- > > drivers/block/Kconfig | 6 ++ > > drivers/block/Makefile | 1 + > > drivers/block/n64cart.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 210 insertions(+) > > create mode 100644 drivers/block/n64cart.c > > > > block folks: rest of the series is on linux-mips. Being a > > mips-specific driver, not sure which tree it should go to. > > It should definitely get reviewed first. One easy question - there's no > commit message in this one. It's a new driver, it should include some > documentation on what it is, etc. It's already had one (rfc) round on linux-mips. Or do you mean by others (who?) - Lauri
On Mon, Jan 04, 2021 at 03:50:31PM +0200, Lauri Kasanen wrote: > block folks: rest of the series is on linux-mips. Being a mips-specific driver, > not sure which tree it should go to. Probably through mips. > +static void n64cart_wait_dma(void) > +{ > + while (n64cart_read_reg(PI_STATUS_REG) & > + (PI_STATUS_DMA_BUSY | PI_STATUS_IO_BUSY)) > + ; > +} These sorts of loops generally call cpu_relax(). > +static blk_status_t get_seg(struct request *req) > +{ > + u32 bstart = blk_rq_pos(req) * 512; > + u32 len = blk_rq_cur_bytes(req); > + void *dst = bio_data(req->bio); > + > + if (bstart + len > size || rq_data_dir(req) == WRITE) > + return BLK_STS_IOERR; If you don't support writes (is that limitation temporary?), then you can prevent such operations from reaching the driver by setting the "disk" to read-only during initialization with set_disk_ro(disk, true). > +static blk_status_t n64cart_queue_rq(struct blk_mq_hw_ctx *hctx, > + const struct blk_mq_queue_data *bd) > +{ > + unsigned long flags; > + struct request *req = bd->rq; > + blk_status_t err; > + > + blk_mq_start_request(req); > + > + spin_lock_irqsave(&n64cart_lock, flags); The .queue_rq() isn't called from an interrupts disabled context, so there should be no need to save the flags. And since you're not taking this lock from an interrupt context anywhere else, I don't think you need to use the spin_lock_irq() variant either. But since you just want to single-thread all IO, you could get that by setting your tagset queue_depth to 1 and remove the spinlock entirely. > +static int __init n64cart_init(void) > +{ > + int err; > + > + if (!start || !size) { > + pr_err("n64cart: start and size not specified\n"); > + return -ENODEV; > + } Just curious, is it not possible to discover these values from the installed cart? Requiring module parameters seems a bit fragile.
On 1/4/21 8:43 AM, Lauri Kasanen wrote: > On Mon, 4 Jan 2021 08:40:10 -0700 > Jens Axboe <axboe@kernel.dk> wrote: > >> On 1/4/21 6:50 AM, Lauri Kasanen wrote: >>> Signed-off-by: Lauri Kasanen <cand@gmx.com> >>> --- >>> drivers/block/Kconfig | 6 ++ >>> drivers/block/Makefile | 1 + >>> drivers/block/n64cart.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 210 insertions(+) >>> create mode 100644 drivers/block/n64cart.c >>> >>> block folks: rest of the series is on linux-mips. Being a >>> mips-specific driver, not sure which tree it should go to. >> >> It should definitely get reviewed first. One easy question - there's no >> commit message in this one. It's a new driver, it should include some >> documentation on what it is, etc. > > It's already had one (rfc) round on linux-mips. Or do you mean by > others (who?) I mean the actual block driver from the block point of view. From a quick look, looks like there's no irq or anything, so it's strictly polled mode? Probably would make sense to push this out of the app path then, by using BLK_MQ_F_BLOCKING.
On Mon, 4 Jan 2021 08:49:55 -0700 Jens Axboe <axboe@kernel.dk> wrote: > On 1/4/21 8:43 AM, Lauri Kasanen wrote: > > On Mon, 4 Jan 2021 08:40:10 -0700 > > Jens Axboe <axboe@kernel.dk> wrote: > >> It should definitely get reviewed first. One easy question - there's no > >> commit message in this one. It's a new driver, it should include some > >> documentation on what it is, etc. > > > > It's already had one (rfc) round on linux-mips. Or do you mean by > > others (who?) > > I mean the actual block driver from the block point of view. From > a quick look, looks like there's no irq or anything, so it's > strictly polled mode? Probably would make sense to push this out > of the app path then, by using BLK_MQ_F_BLOCKING. It's very unclear to me what that flag does. I read all the docs available, but nothing made it clear (same applies for almost all BLK_MQ_*). - Lauri
On Mon, 4 Jan 2021 07:49:02 -0800 Keith Busch <kbusch@kernel.org> wrote: > On Mon, Jan 04, 2021 at 03:50:31PM +0200, Lauri Kasanen wrote: > > block folks: rest of the series is on linux-mips. Being a mips-specific driver, > > not sure which tree it should go to. > > Probably through mips. Thanks. > > +static blk_status_t get_seg(struct request *req) > > +{ > > + u32 bstart = blk_rq_pos(req) * 512; > > + u32 len = blk_rq_cur_bytes(req); > > + void *dst = bio_data(req->bio); > > + > > + if (bstart + len > size || rq_data_dir(req) == WRITE) > > + return BLK_STS_IOERR; > > If you don't support writes (is that limitation temporary?), then you > can prevent such operations from reaching the driver by setting the > "disk" to read-only during initialization with set_disk_ro(disk, true). The media is read-only (but not runtime removable). > > +static int __init n64cart_init(void) > > +{ > > + int err; > > + > > + if (!start || !size) { > > + pr_err("n64cart: start and size not specified\n"); > > + return -ENODEV; > > + } > > Just curious, is it not possible to discover these values from the > installed cart? Requiring module parameters seems a bit fragile. No, it's not possible. Even the cart's size is not discoverable, and where the disk image starts and how long it is could only be scanned if you knew in advance what FS what used, etc. There is no FS on the cart itself, it's just raw data. The bootloader embeds these values into itself when it's built. - Lauri
On 1/4/21 8:56 AM, Lauri Kasanen wrote: > On Mon, 4 Jan 2021 08:49:55 -0700 > Jens Axboe <axboe@kernel.dk> wrote: > >> On 1/4/21 8:43 AM, Lauri Kasanen wrote: >>> On Mon, 4 Jan 2021 08:40:10 -0700 >>> Jens Axboe <axboe@kernel.dk> wrote: >>>> It should definitely get reviewed first. One easy question - there's no >>>> commit message in this one. It's a new driver, it should include some >>>> documentation on what it is, etc. >>> >>> It's already had one (rfc) round on linux-mips. Or do you mean by >>> others (who?) >> >> I mean the actual block driver from the block point of view. From >> a quick look, looks like there's no irq or anything, so it's >> strictly polled mode? Probably would make sense to push this out >> of the app path then, by using BLK_MQ_F_BLOCKING. > > It's very unclear to me what that flag does. I read all the docs > available, but nothing made it clear (same applies for almost all > BLK_MQ_*). It pushes the actual processing to a helper thread instead of doing it inline on submit. Generally used if the driver needs to block to submit IO, as per the name, but also useful for really slow hardware like this one.
On Mon, Jan 04, 2021 at 06:01:15PM +0200, Lauri Kasanen wrote: > On Mon, 4 Jan 2021 07:49:02 -0800 > Keith Busch <kbusch@kernel.org> wrote: > > > On Mon, Jan 04, 2021 at 03:50:31PM +0200, Lauri Kasanen wrote: > > > block folks: rest of the series is on linux-mips. Being a mips-specific driver, > > > not sure which tree it should go to. > > > > Probably through mips. > > Thanks. To be more clear, the initial commit probably makes since to go through mips once the series is ready, but new block drivers do need to be sent to linux-block for the appropriate acks and reviews first. > > > +static blk_status_t get_seg(struct request *req) > > > +{ > > > + u32 bstart = blk_rq_pos(req) * 512; > > > + u32 len = blk_rq_cur_bytes(req); > > > + void *dst = bio_data(req->bio); > > > + > > > + if (bstart + len > size || rq_data_dir(req) == WRITE) > > > + return BLK_STS_IOERR; > > > > If you don't support writes (is that limitation temporary?), then you > > can prevent such operations from reaching the driver by setting the > > "disk" to read-only during initialization with set_disk_ro(disk, true). > > The media is read-only (but not runtime removable). It's been a while, but I could swear we can save state on these carts. If so, it sounds like that must be separate from the media this driver is accessing, so is that capability provided through a different driver?
On Mon, 4 Jan 2021 08:13:32 -0800 Keith Busch <kbusch@kernel.org> wrote: > > The media is read-only (but not runtime removable). > > It's been a while, but I could swear we can save state on these carts. > If so, it sounds like that must be separate from the media this driver > is accessing, so is that capability provided through a different driver? Saving uses a separate mechanism, and there are several depending on what type of cart it is (no saves, flash, eeprom, or sram). If the cart has no save possibility, there is an optional memory card attached to the joypad controller. Currently there is no driver for any of these. - Lauri
On 1/4/21 9:27 AM, Lauri Kasanen wrote: > On Mon, 4 Jan 2021 08:13:32 -0800 > Keith Busch <kbusch@kernel.org> wrote: > >>> The media is read-only (but not runtime removable). >> >> It's been a while, but I could swear we can save state on these carts. >> If so, it sounds like that must be separate from the media this driver >> is accessing, so is that capability provided through a different driver? > > Saving uses a separate mechanism, and there are several depending on > what type of cart it is (no saves, flash, eeprom, or sram). If the cart > has no save possibility, there is an optional memory card attached to > the joypad controller. > > Currently there is no driver for any of these. Maybe it'd make sense to make this runtime configurable through configfs or similar, requiring boot parameters (and hence a reboot to change them) seems pretty iffy. Similarly, why isn't it available as a module?
On Mon, 4 Jan 2021 09:28:44 -0700 Jens Axboe <axboe@kernel.dk> wrote: > On 1/4/21 9:27 AM, Lauri Kasanen wrote: > > On Mon, 4 Jan 2021 08:13:32 -0800 > > Keith Busch <kbusch@kernel.org> wrote: > > > >>> The media is read-only (but not runtime removable). > >> > >> It's been a while, but I could swear we can save state on these carts. > >> If so, it sounds like that must be separate from the media this driver > >> is accessing, so is that capability provided through a different driver? > > > > Saving uses a separate mechanism, and there are several depending on > > what type of cart it is (no saves, flash, eeprom, or sram). If the cart > > has no save possibility, there is an optional memory card attached to > > the joypad controller. > > > > Currently there is no driver for any of these. > > Maybe it'd make sense to make this runtime configurable through configfs > or similar, requiring boot parameters (and hence a reboot to change > them) seems pretty iffy. Similarly, why isn't it available as a module? You can't change the media without a reboot. What would be the use? (it's not possible to have multiple carts, if you're wondering) The lack of a module option is because the target system has 8mb ram. The kernel is already very bloated, at one time during testing just enabling the smallest IO governor (mq deadline) caused an OOM during boot. Same for configfs, debugfs, etc. No such options to save RAM. - Lauri
diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig index ecceaaa..924d768 100644 --- a/drivers/block/Kconfig +++ b/drivers/block/Kconfig @@ -72,6 +72,12 @@ config AMIGA_Z2RAM To compile this driver as a module, choose M here: the module will be called z2ram. +config N64CART + bool "N64 cart support" + depends on MACH_NINTENDO64 + help + Support for the N64 cart. + config CDROM tristate select BLK_SCSI_REQUEST diff --git a/drivers/block/Makefile b/drivers/block/Makefile index e1f6311..b9642cf 100644 --- a/drivers/block/Makefile +++ b/drivers/block/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_PS3_DISK) += ps3disk.o obj-$(CONFIG_PS3_VRAM) += ps3vram.o obj-$(CONFIG_ATARI_FLOPPY) += ataflop.o obj-$(CONFIG_AMIGA_Z2RAM) += z2ram.o +obj-$(CONFIG_N64CART) += n64cart.o obj-$(CONFIG_BLK_DEV_RAM) += brd.o obj-$(CONFIG_BLK_DEV_LOOP) += loop.o obj-$(CONFIG_XILINX_SYSACE) += xsysace.o diff --git a/drivers/block/n64cart.c b/drivers/block/n64cart.c new file mode 100644 index 0000000..ae2ad4f --- /dev/null +++ b/drivers/block/n64cart.c @@ -0,0 +1,203 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Support for the N64 cart. + * + * Copyright (c) 2020 Lauri Kasanen + */ + +#include <linux/bitops.h> +#include <linux/blk-mq.h> +#include <linux/dma-mapping.h> +#include <linux/init.h> +#include <linux/major.h> +#include <linux/module.h> +#include <linux/spinlock.h> + +#include <asm/addrspace.h> +#include <asm/io.h> + +MODULE_AUTHOR("Lauri Kasanen <cand@gmx.com>"); +MODULE_DESCRIPTION("Driver for the N64 cart"); +MODULE_LICENSE("GPL"); + +#define BUFSIZE (64 * 1024) + +static unsigned int start, size; +static int major; +static struct request_queue *queue; +static struct blk_mq_tag_set tag_set; +static struct gendisk *disk; + +static void *buf; +static dma_addr_t dma_addr; + +static DEFINE_SPINLOCK(n64cart_lock); + +#define REG_BASE ((u32 *) TO_UNCAC(0x4600000)) + +#define PI_DRAM_REG 0 +#define PI_CART_REG 1 +#define PI_READ_REG 2 +#define PI_WRITE_REG 3 +#define PI_STATUS_REG 4 + +#define PI_STATUS_DMA_BUSY (1 << 0) +#define PI_STATUS_IO_BUSY (1 << 1) + +static void n64cart_write_reg(const u8 reg, const u32 value) +{ + __raw_writel(value, REG_BASE + reg); +} + +static u32 n64cart_read_reg(const u8 reg) +{ + return __raw_readl(REG_BASE + reg); +} + +static void n64cart_wait_dma(void) +{ + while (n64cart_read_reg(PI_STATUS_REG) & + (PI_STATUS_DMA_BUSY | PI_STATUS_IO_BUSY)) + ; +} + +static blk_status_t get_seg(struct request *req) +{ + u32 bstart = blk_rq_pos(req) * 512; + u32 len = blk_rq_cur_bytes(req); + void *dst = bio_data(req->bio); + + if (bstart + len > size || rq_data_dir(req) == WRITE) + return BLK_STS_IOERR; + + bstart += start; + + while (len) { + const u32 curlen = len < BUFSIZE ? len : BUFSIZE; + + dma_cache_inv((unsigned long) buf, curlen); + + n64cart_wait_dma(); + + barrier(); + n64cart_write_reg(PI_DRAM_REG, dma_addr); + barrier(); + n64cart_write_reg(PI_CART_REG, (bstart | 0x10000000) & 0x1FFFFFFF); + barrier(); + n64cart_write_reg(PI_WRITE_REG, curlen - 1); + barrier(); + + n64cart_wait_dma(); + + memcpy(dst, buf, curlen); + + len -= curlen; + dst += curlen; + bstart += curlen; + } + + return BLK_STS_OK; +} + +static blk_status_t n64cart_queue_rq(struct blk_mq_hw_ctx *hctx, + const struct blk_mq_queue_data *bd) +{ + unsigned long flags; + struct request *req = bd->rq; + blk_status_t err; + + blk_mq_start_request(req); + + spin_lock_irqsave(&n64cart_lock, flags); + + do { + err = get_seg(req); + } while (blk_update_request(req, err, blk_rq_cur_bytes(req))); + + spin_unlock_irqrestore(&n64cart_lock, flags); + blk_mq_end_request(req, BLK_STS_OK); + return BLK_STS_OK; +} + +static const struct blk_mq_ops n64cart_mq_ops = { + .queue_rq = n64cart_queue_rq, +}; + +static const struct block_device_operations n64cart_fops = { + .owner = THIS_MODULE, +}; + +static int __init n64cart_init(void) +{ + int err; + + if (!start || !size) { + pr_err("n64cart: start and size not specified\n"); + return -ENODEV; + } + + if (size & 4095) { + pr_err("n64cart: size must be a multiple of 4K\n"); + return -ENODEV; + } + + major = register_blkdev(0, "n64cart"); + if (major <= 0) { + pr_err("n64cart: unable to get major number\n"); + return -EBUSY; + } + + queue = blk_mq_init_sq_queue(&tag_set, &n64cart_mq_ops, 16, + BLK_MQ_F_SHOULD_MERGE); + if (IS_ERR(queue)) { + err = PTR_ERR(queue); + goto fail_reg; + } + + buf = kmalloc(BUFSIZE, GFP_DMA | GFP_KERNEL); + if (!buf) { + err = -ENOMEM; + goto fail_queue; + } + dma_addr = virt_to_phys(buf); + + disk = alloc_disk(1); + if (!disk) { + err = -ENOMEM; + goto fail_dma; + } + + disk->major = major; + disk->first_minor = 0; + disk->queue = queue; + disk->flags = GENHD_FL_NO_PART_SCAN; + disk->fops = &n64cart_fops; + strcpy(disk->disk_name, "n64cart"); + + set_capacity(disk, size / 512); + + blk_queue_flag_set(QUEUE_FLAG_NONROT, queue); + blk_queue_physical_block_size(queue, 4096); + blk_queue_logical_block_size(queue, 4096); + + add_disk(disk); + + pr_info("n64cart: %u kb disk\n", size / 1024); + + return 0; +fail_dma: + kfree(buf); +fail_queue: + blk_cleanup_queue(queue); +fail_reg: + unregister_blkdev(major, "n64cart"); + return err; +} + +module_param(start, uint, 0); +MODULE_PARM_DESC(start, "Start address of the cart block data"); + +module_param(size, uint, 0); +MODULE_PARM_DESC(size, "Size of the cart block data, in bytes"); + +module_init(n64cart_init);
Signed-off-by: Lauri Kasanen <cand@gmx.com> --- drivers/block/Kconfig | 6 ++ drivers/block/Makefile | 1 + drivers/block/n64cart.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 210 insertions(+) create mode 100644 drivers/block/n64cart.c block folks: rest of the series is on linux-mips. Being a mips-specific driver, not sure which tree it should go to. -- 2.6.2