Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Thu, Dec 20, 2007 at 09:53:54PM +, Adrian McMenamin wrote: > On 16/12/2007, Paul Mundt <[EMAIL PROTECTED]> wrote: > > Also, __devinit/__devexit annotations? > > > > Is there any difference between __init and __devint? Yes. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On 16/12/2007, Paul Mundt <[EMAIL PROTECTED]> wrote:
>
> Also, __devinit/__devexit annotations?
>
Is there any difference between __init and __devint? I am using
__init/__exit already
> > +static struct platform_driver gdrom_driver = {
> > + .probe = probe_gdrom,
> > + .remove = remove_gdrom,
>
> __devexit_p()?
>
> > +static void __exit exit_gdrom(void)
> > +{
> > + blk_cleanup_queue(gd.gdrom_rq);
> > + free_irq(HW_EVENT_GDROM_CMD, );
> > + free_irq(HW_EVENT_GDROM_DMA, );
> > + del_gendisk(gd.disk);
> > + if (gdrom_major)
> > + unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
> > + platform_device_unregister(pd);
> > + platform_driver_unregister(_driver);
> > + if (gd.toc)
> > + kfree(gd.toc);
> > +}
> > +
> Ah, here's where you do the rest of the cleanup. This is non-intuitive,
> remove should balance the work done by probe and exit the work done by
> init.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On 16/12/2007, Paul Mundt [EMAIL PROTECTED] wrote:
Also, __devinit/__devexit annotations?
Is there any difference between __init and __devint? I am using
__init/__exit already
+static struct platform_driver gdrom_driver = {
+ .probe = probe_gdrom,
+ .remove = remove_gdrom,
__devexit_p()?
+static void __exit exit_gdrom(void)
+{
+ blk_cleanup_queue(gd.gdrom_rq);
+ free_irq(HW_EVENT_GDROM_CMD, gd);
+ free_irq(HW_EVENT_GDROM_DMA, gd);
+ del_gendisk(gd.disk);
+ if (gdrom_major)
+ unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
+ platform_device_unregister(pd);
+ platform_driver_unregister(gdrom_driver);
+ if (gd.toc)
+ kfree(gd.toc);
+}
+
Ah, here's where you do the rest of the cleanup. This is non-intuitive,
remove should balance the work done by probe and exit the work done by
init.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Thu, Dec 20, 2007 at 09:53:54PM +, Adrian McMenamin wrote: On 16/12/2007, Paul Mundt [EMAIL PROTECTED] wrote: Also, __devinit/__devexit annotations? Is there any difference between __init and __devint? Yes. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Dec 16 2007 18:50, Paul Mundt wrote:
>On Sun, Dec 16, 2007 at 12:21:21AM +, Adrian McMenamin wrote:
>> +/* GD Rom registers */
>> +#define GDROM_BASE_REG 0xA05F7000
>> +#define GDROM_ALTSTATUS_REG GDROM_BASE_REG + 0x18
>> +#define GDROM_DATA_REG GDROM_BASE_REG + 0x80
>> +#define GDROM_ERROR_REG GDROM_BASE_REG + 0x84
>> +#define GDROM_INTSEC_REG GDROM_BASE_REG + 0x88
>> +#define GDROM_SECNUM_REG GDROM_BASE_REG + 0x8C
>> +#define GDROM_BCL_REG GDROM_BASE_REG + 0x90
>> +#define GDROM_BCH_REG GDROM_BASE_REG + 0x94
>> +#define GDROM_DSEL_REG GDROM_BASE_REG + 0x98
>> +#define GDROM_STATUSCOMMAND_REG GDROM_BASE_REG + 0x9C
>> +#define GDROM_RESET_REG GDROM_BASE_REG + 0x4E4
>> +
>> +#define GDROM_DATA_REG_P0 0x005F7080
>> +
>> +#define GDROM_DMA_STARTADDR_REG GDROM_BASE_REG + 0x404
>> +#define GDROM_DMA_LENGTH_REG GDROM_BASE_REG + 0x408
>> +#define GDROM_DMA_DIRECTION_REG GDROM_BASE_REG + 0x40C
>> +#define GDROM_DMA_ENABLE_REG GDROM_BASE_REG + 0x414
>> +#define GDROM_DMA_STATUS_REG GDROM_BASE_REG + 0x418
>> +#define GDROM_DMA_WAIT_REG GDROM_BASE_REG + 0x4A0
>> +#define GDROM_DMA_ACCESS_CTRL_REG GDROM_BASE_REG + 0x4B8
>> +
>These should all be encapsulated by brackets.
Or making it an enum {
GDROM_DMA_ACCESS_CTL_REG = GDROM_BASE_REG + 0x4B8,
}; without brackets.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Mon, December 17, 2007 2:06 pm, Jens Axboe wrote: > On Sun, Dec 16 2007, Adrian McMenamin wrote: > > Few notes: Thanks for these, very helpful. > > - Compare rq_data_dir() with WRITE, don't just assume that any non-zero > will be a write. > > - You need to offload this request handling to a workqueue of some sort, > your current request handling is very broken for two reasons: One is > that interrupts are still disabled when you drop your queue lock, so > you cannot use sleeping functions like GFP_KERNEL allocations or > wait_event(). The other is that it's illegal to sleep from your > request_fn context in the first place, since you could be stalling > others. Ah. OK. Funnily enough an earlier version did offload to a workqueue but i thought that was too convoluted. I didn't realise interrupts were disabled inside the request_fn context. > > - You also seem to be busy waiting for other transactions to finish. Any > idea how long those might take? Perhaps put an upper bound on this > waiting, and/or do blocking waits? > Well, when the register bit clears they are over, seems to be about 200 - 400 microseconds or there abouts (ie that length of time between the interrupt signalling that the transfer is over and the bit clearing to allow a new transfer) - so I could time that out at say HZ/2 and be very safe. > - I'm assuming this hardware can't do sg transfers? > Afraid not. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, Dec 16 2007, Adrian McMenamin wrote:
> +static int gdrom_readdisk_dma(int block, int block_cnt, char *buffer)
> +{
> + int err;
> + struct packet_command *read_command;
> + /* release the spin lock but check later
> + * we're not in the middle of some dma */
> + spin_unlock(_lock);
> + ctrl_outl(0x8843407F, GDROM_DMA_ACCESS_CTRL_REG); /* memory setting */
> + ctrl_outl(9, GDROM_DMA_WAIT_REG); /* DMA word setting */
> + ctrl_outl(PHYSADDR(buffer), GDROM_DMA_STARTADDR_REG);
> + ctrl_outl(block_cnt * GDROM_HARD_SECTOR, GDROM_DMA_LENGTH_REG);
> + ctrl_outl(1, GDROM_DMA_DIRECTION_REG);
> + ctrl_outl(1, GDROM_DMA_ENABLE_REG);
> + /* send command */
> + read_command = kzalloc(sizeof(struct packet_command), GFP_KERNEL);
> + if (!read_command)
> + return -ENOMEM;
> + read_command->cmd[0] = 0x30;
> + read_command->cmd[1] = 0x20;
> + read_command->cmd[2] = (block >> 16) & 0xFF;
> + read_command->cmd[3] = (block >> 8) & 0xFF;
> + read_command->cmd[4] = block & 0xFF;
> + read_command->cmd[8] = (block_cnt >> 16) & 0xFF;
> + read_command->cmd[9] = (block_cnt >> 8) & 0xFF;
> + read_command->cmd[10] = block_cnt & 0xFF;
> + /* set for DMA */
> + ctrl_outb(1, GDROM_ERROR_REG);
> + /* other parameters */
> + ctrl_outb(0, GDROM_SECNUM_REG);
> + ctrl_outb(0, GDROM_BCL_REG);
> + ctrl_outb(0, GDROM_BCH_REG);
> + ctrl_outb(0, GDROM_DSEL_REG);
> + ctrl_outb(0, GDROM_INTSEC_REG);
> + /* In multiple DMA transfers need to wait */
> + while (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80)
> + cpu_relax();
> + ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
> + while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8)
> + cpu_relax(); /* wait for DRQ to be set to 1 */
> + gd.pending = 1;
> + gd.transfer = 1;
> + outsw(PHYSADDR(GDROM_DATA_REG), _command->cmd, 6);
> + while (ctrl_inb(GDROM_DMA_STATUS_REG))
> + cpu_relax();
> + ctrl_outb(1, GDROM_DMA_STATUS_REG);
> + /* 5 second error margin here seems more reasonable */
> + wait_event_interruptible_timeout(request_queue, gd.transfer == 0, HZ *
> 5);
> + err = ctrl_inb(GDROM_DMA_STATUS_REG);
> + err = gd.transfer;
> + gd.transfer = 0;
> + gd.pending = 0;
> + kfree(read_command);
> + spin_lock(_lock);
> + return err;
> +}
> +
> +static void gdrom_request_handler_dma(struct request *req)
> +{
> + int err, block, block_cnt;
> + err = 0;
> + block = req->sector/GD_TO_BLK + GD_SESSION_OFFSET;
> + block_cnt = req->nr_sectors/GD_TO_BLK;
> + err = gdrom_readdisk_dma(block, block_cnt, req->buffer);
> + if (unlikely(err)) {
> + end_request(req, 0);
> + spin_unlock(_lock);
> + gdrom_getsense(NULL);
> + spin_lock(_lock);
> + }
> + else
> + end_request(req, 1);
> +}
> +
> +static void gdrom_request(struct request_queue *rq)
> +{
> + struct request *req;
> + unsigned long pages;
> + pages = rq->backing_dev_info.ra_pages;
> + while ((req = elv_next_request(rq)) != NULL) {
> + if (! blk_fs_request(req)) {
> + printk(KERN_DEBUG "GDROM: Non-fs request ignored\n");
> + end_request(req, 0);
> + }
> + if (rq_data_dir(req)) {
> + printk(KERN_NOTICE "GDROM: Read only device - write
> request ignored\n");
> + end_request(req, 0);
> + }
> + if (req->nr_sectors) {
> + gdrom_request_handler_dma(req);
> + }
> + }
> +}
> +
Few notes:
- Compare rq_data_dir() with WRITE, don't just assume that any non-zero
will be a write.
- You need to offload this request handling to a workqueue of some sort,
your current request handling is very broken for two reasons: One is
that interrupts are still disabled when you drop your queue lock, so
you cannot use sleeping functions like GFP_KERNEL allocations or
wait_event(). The other is that it's illegal to sleep from your
request_fn context in the first place, since you could be stalling
others.
- You also seem to be busy waiting for other transactions to finish. Any
idea how long those might take? Perhaps put an upper bound on this
waiting, and/or do blocking waits?
- I'm assuming this hardware can't do sg transfers?
--
Jens Axboe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, Dec 16 2007, Adrian McMenamin wrote:
+static int gdrom_readdisk_dma(int block, int block_cnt, char *buffer)
+{
+ int err;
+ struct packet_command *read_command;
+ /* release the spin lock but check later
+ * we're not in the middle of some dma */
+ spin_unlock(gdrom_lock);
+ ctrl_outl(0x8843407F, GDROM_DMA_ACCESS_CTRL_REG); /* memory setting */
+ ctrl_outl(9, GDROM_DMA_WAIT_REG); /* DMA word setting */
+ ctrl_outl(PHYSADDR(buffer), GDROM_DMA_STARTADDR_REG);
+ ctrl_outl(block_cnt * GDROM_HARD_SECTOR, GDROM_DMA_LENGTH_REG);
+ ctrl_outl(1, GDROM_DMA_DIRECTION_REG);
+ ctrl_outl(1, GDROM_DMA_ENABLE_REG);
+ /* send command */
+ read_command = kzalloc(sizeof(struct packet_command), GFP_KERNEL);
+ if (!read_command)
+ return -ENOMEM;
+ read_command-cmd[0] = 0x30;
+ read_command-cmd[1] = 0x20;
+ read_command-cmd[2] = (block 16) 0xFF;
+ read_command-cmd[3] = (block 8) 0xFF;
+ read_command-cmd[4] = block 0xFF;
+ read_command-cmd[8] = (block_cnt 16) 0xFF;
+ read_command-cmd[9] = (block_cnt 8) 0xFF;
+ read_command-cmd[10] = block_cnt 0xFF;
+ /* set for DMA */
+ ctrl_outb(1, GDROM_ERROR_REG);
+ /* other parameters */
+ ctrl_outb(0, GDROM_SECNUM_REG);
+ ctrl_outb(0, GDROM_BCL_REG);
+ ctrl_outb(0, GDROM_BCH_REG);
+ ctrl_outb(0, GDROM_DSEL_REG);
+ ctrl_outb(0, GDROM_INTSEC_REG);
+ /* In multiple DMA transfers need to wait */
+ while (ctrl_inb(GDROM_ALTSTATUS_REG) 0x80)
+ cpu_relax();
+ ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
+ while ((ctrl_inb(GDROM_ALTSTATUS_REG) 0x88) != 8)
+ cpu_relax(); /* wait for DRQ to be set to 1 */
+ gd.pending = 1;
+ gd.transfer = 1;
+ outsw(PHYSADDR(GDROM_DATA_REG), read_command-cmd, 6);
+ while (ctrl_inb(GDROM_DMA_STATUS_REG))
+ cpu_relax();
+ ctrl_outb(1, GDROM_DMA_STATUS_REG);
+ /* 5 second error margin here seems more reasonable */
+ wait_event_interruptible_timeout(request_queue, gd.transfer == 0, HZ *
5);
+ err = ctrl_inb(GDROM_DMA_STATUS_REG);
+ err = gd.transfer;
+ gd.transfer = 0;
+ gd.pending = 0;
+ kfree(read_command);
+ spin_lock(gdrom_lock);
+ return err;
+}
+
+static void gdrom_request_handler_dma(struct request *req)
+{
+ int err, block, block_cnt;
+ err = 0;
+ block = req-sector/GD_TO_BLK + GD_SESSION_OFFSET;
+ block_cnt = req-nr_sectors/GD_TO_BLK;
+ err = gdrom_readdisk_dma(block, block_cnt, req-buffer);
+ if (unlikely(err)) {
+ end_request(req, 0);
+ spin_unlock(gdrom_lock);
+ gdrom_getsense(NULL);
+ spin_lock(gdrom_lock);
+ }
+ else
+ end_request(req, 1);
+}
+
+static void gdrom_request(struct request_queue *rq)
+{
+ struct request *req;
+ unsigned long pages;
+ pages = rq-backing_dev_info.ra_pages;
+ while ((req = elv_next_request(rq)) != NULL) {
+ if (! blk_fs_request(req)) {
+ printk(KERN_DEBUG GDROM: Non-fs request ignored\n);
+ end_request(req, 0);
+ }
+ if (rq_data_dir(req)) {
+ printk(KERN_NOTICE GDROM: Read only device - write
request ignored\n);
+ end_request(req, 0);
+ }
+ if (req-nr_sectors) {
+ gdrom_request_handler_dma(req);
+ }
+ }
+}
+
Few notes:
- Compare rq_data_dir() with WRITE, don't just assume that any non-zero
will be a write.
- You need to offload this request handling to a workqueue of some sort,
your current request handling is very broken for two reasons: One is
that interrupts are still disabled when you drop your queue lock, so
you cannot use sleeping functions like GFP_KERNEL allocations or
wait_event(). The other is that it's illegal to sleep from your
request_fn context in the first place, since you could be stalling
others.
- You also seem to be busy waiting for other transactions to finish. Any
idea how long those might take? Perhaps put an upper bound on this
waiting, and/or do blocking waits?
- I'm assuming this hardware can't do sg transfers?
--
Jens Axboe
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Mon, December 17, 2007 2:06 pm, Jens Axboe wrote: On Sun, Dec 16 2007, Adrian McMenamin wrote: Few notes: Thanks for these, very helpful. - Compare rq_data_dir() with WRITE, don't just assume that any non-zero will be a write. - You need to offload this request handling to a workqueue of some sort, your current request handling is very broken for two reasons: One is that interrupts are still disabled when you drop your queue lock, so you cannot use sleeping functions like GFP_KERNEL allocations or wait_event(). The other is that it's illegal to sleep from your request_fn context in the first place, since you could be stalling others. Ah. OK. Funnily enough an earlier version did offload to a workqueue but i thought that was too convoluted. I didn't realise interrupts were disabled inside the request_fn context. - You also seem to be busy waiting for other transactions to finish. Any idea how long those might take? Perhaps put an upper bound on this waiting, and/or do blocking waits? Well, when the register bit clears they are over, seems to be about 200 - 400 microseconds or there abouts (ie that length of time between the interrupt signalling that the transfer is over and the bit clearing to allow a new transfer) - so I could time that out at say HZ/2 and be very safe. - I'm assuming this hardware can't do sg transfers? Afraid not. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Dec 16 2007 18:50, Paul Mundt wrote:
On Sun, Dec 16, 2007 at 12:21:21AM +, Adrian McMenamin wrote:
+/* GD Rom registers */
+#define GDROM_BASE_REG 0xA05F7000
+#define GDROM_ALTSTATUS_REG GDROM_BASE_REG + 0x18
+#define GDROM_DATA_REG GDROM_BASE_REG + 0x80
+#define GDROM_ERROR_REG GDROM_BASE_REG + 0x84
+#define GDROM_INTSEC_REG GDROM_BASE_REG + 0x88
+#define GDROM_SECNUM_REG GDROM_BASE_REG + 0x8C
+#define GDROM_BCL_REG GDROM_BASE_REG + 0x90
+#define GDROM_BCH_REG GDROM_BASE_REG + 0x94
+#define GDROM_DSEL_REG GDROM_BASE_REG + 0x98
+#define GDROM_STATUSCOMMAND_REG GDROM_BASE_REG + 0x9C
+#define GDROM_RESET_REG GDROM_BASE_REG + 0x4E4
+
+#define GDROM_DATA_REG_P0 0x005F7080
+
+#define GDROM_DMA_STARTADDR_REG GDROM_BASE_REG + 0x404
+#define GDROM_DMA_LENGTH_REG GDROM_BASE_REG + 0x408
+#define GDROM_DMA_DIRECTION_REG GDROM_BASE_REG + 0x40C
+#define GDROM_DMA_ENABLE_REG GDROM_BASE_REG + 0x414
+#define GDROM_DMA_STATUS_REG GDROM_BASE_REG + 0x418
+#define GDROM_DMA_WAIT_REG GDROM_BASE_REG + 0x4A0
+#define GDROM_DMA_ACCESS_CTRL_REG GDROM_BASE_REG + 0x4B8
+
These should all be encapsulated by brackets.
Or making it an enum {
GDROM_DMA_ACCESS_CTL_REG = GDROM_BASE_REG + 0x4B8,
}; without brackets.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Mon, 2007-12-17 at 06:59 +0900, Paul Mundt wrote: > > While I realize that this is all undocumented and based entirely on > reverse engineering, you should at least verify that that's precisely > what is going on, and that this is not just a precaution for flushing > posted writes. You can test that by removing the loop and doing a dummy > read after your write (to the same register, rather than the entire ROM > space). If it's a posting issue, then you will have to do your own > read/write_reg routines that handle the flush. I've looked at this some more and, from the various things on the web, it seems SEGA introduced some sort of crude checksum security feature which ctrl_outl(0x1f, GDROM_RESET_REG) appears to start. Unless it is used then the GD Rom remains disabled. After that, the hardware executes a checksum of the ROM to ensure that you are actually booting from a Dreamcast. Well, that is my interpretation of http://www.ludd.ltu.se/~jlo/dc/bootROM.c and similar. Certainly any other sort of read seems to keep the gdrom disabled. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, Dec 16, 2007 at 05:32:51PM +, Adrian McMenamin wrote: > On Sun, 2007-12-16 at 18:50 +0900, Paul Mundt wrote: > > > + for (count = 0xa000; count < 0xa020; count += 4) > > > + ctrl_inl(count); > > > > Er? This ranged dummy reading of the P2 space needs some explanation. The > > GD-ROM isn't even mapped in to this space, so this seems like a hack to > > either work around a timing issue or a write ordering problem. > > I'll confess to not knowing what it's up to here either, other than it > looks like a mechanism to cause a G1 bus reset. This is a progressive > read of the Boot ROM area and that is all under G1 control (as is the GD > Rom obviously) > Ok, then this should be moved out to a g1_bus_reset() or something similar with a comment explaining what it's doing, that way it can be trivially reworked if a saner method of forcing a G1 reset is discovered. While I realize that this is all undocumented and based entirely on reverse engineering, you should at least verify that that's precisely what is going on, and that this is not just a precaution for flushing posted writes. You can test that by removing the loop and doing a dummy read after your write (to the same register, rather than the entire ROM space). If it's a posting issue, then you will have to do your own read/write_reg routines that handle the flush. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Saturday 15 December 2007, Adrian McMenamin wrote:
> diff -ruN ./linux-2.6-orig/drivers/sh/gdrom/gdrom.c
> ./linux-2.6/drivers/sh/gdrom/gdrom.c
i think your e-mail client word wrapped a little ...
> + if (gd.toc)
> + kfree(gd.toc);
i dont know how the kernel functions, but in userspace, free(NULL) is
acceptable ...
> + memcpy(gd.toc, tocB, sizeof (struct gdromtoc));
> + else
> + memcpy(gd.toc, tocA, sizeof (struct gdromtoc));
since gd.toc and toc[BA] are of the same type, cant you just:
*gd.toc = *tocA
also, since tocB/tocA only exist in this function (you kzalloc() at the top
and kfree() at the bottom), and you dont do something like "gd.toc = tocA",
why use the memory allocator at all ? i dont think they are too large for
the stack (~400bytes a piece) ... maybe they are ...
> +static int gdrom_open(struct cdrom_device_info *cd_info, int purpose)
> +{
> + int err;
> + /* spin up the disk */
> + err = gdrom_preparedisk_cmd();
> + if (err)
> + return -EIO;
> +
> + return 0;
> +}
is it normal to normalize all errors like this ? it'd be a much simpler
function like:
{ return gdrom_preparedisk_cmd(); }
> +static irqreturn_t gdrom_command_interrupt(int irq, void *dev_id)
> +{
> + if (dev_id != )
> + return IRQ_NONE;
> + gd.status = ctrl_inb(GDROM_STATUSCOMMAND_REG);
> + if (gd.pending != 1)
> + return IRQ_HANDLED;
>
> +static irqreturn_t gdrom_dma_interrupt(int irq, void *dev_id)
> +{
> + if (dev_id != )
> + return IRQ_NONE;
> + gd.status = ctrl_inb(GDROM_STATUSCOMMAND_REG);
> + if (gd.transfer != 1)
> + return IRQ_HANDLED;
if you dont have a pending interrupt, shouldnt it return IRQ_NONE here ? Paul
already mentioned the weird dev_id check.
> +static int gdrom_readdisk_dma(int block, int block_cnt, char *buffer)
> +{
> + int err;
> + struct packet_command *read_command;
> + /* release the spin lock but check later
> + * we're not in the middle of some dma */
> + spin_unlock(_lock);
> + ctrl_outl(0x8843407F, GDROM_DMA_ACCESS_CTRL_REG); /* memory setting */
it'd be nice if these magic #'s had more explanation behind them, but you may
simply not have that information :/
> +static void gdrom_request(struct request_queue *rq)
> +{
> + struct request *req;
> + unsigned long pages;
> + pages = rq->backing_dev_info.ra_pages;
> + while ((req = elv_next_request(rq)) != NULL) {
> + if (! blk_fs_request(req)) {
> + printk(KERN_DEBUG "GDROM: Non-fs request ignored\n");
> + end_request(req, 0);
> + }
> + if (rq_data_dir(req)) {
> + printk(KERN_NOTICE "GDROM: Read only device - write
> request
> ignored\n"); +end_request(req, 0);
> + }
> + if (req->nr_sectors) {
> + gdrom_request_handler_dma(req);
> + }
> + }
> +}
no need for all the {} in the last two if()'s
> +/* Print string identifying GD ROM device */
> +static void gdrom_outputversion(void)
> +{
> + struct gdrom_id *id;
> + char *model_name, *manuf_name, *firmw_ver;
> + /* query device ID */
> + id = kzalloc(sizeof(struct gdrom_id), GFP_KERNEL);
i dont know how other people feel, but i think the style:
sizeof(*id)
is cleaner and leads to less bitrot ... also, you dont have an "if (!id)"
check there ... Paul pointed out plenty of other stuff for this func ;)
also, wrt to sizes ("16" and "17"), arent there some defines you can key off
of or something ?
> +MODULE_DESCRIPTION("GD-ROM Driver");
SEGA Dreamcast GD-ROM Driver ?
-mike
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, 2007-12-16 at 18:50 +0900, Paul Mundt wrote:
> > +static int gdrom_readdisk_dma(int block, int block_cnt, char *buffer)
> > +{
> > + int err;
> > + struct packet_command *read_command;
> > + /* release the spin lock but check later
> > +* we're not in the middle of some dma */
> > + spin_unlock(_lock);
>
> The locking strategy here is a bit interesting, has spinlock debugging
> tossed any profanities to your console?
>
No, no errors reported.
I think I've been careful - I release the lock because we're doing
things like allocating memory and then sleeping on the queue.
But I also need to check various registers so that (a) we don't try to
do a second DMA until the hardware is ready and (b) we properly
serialize access to the device by two different threads of execution.
Seems to work.
Though I have an issue with the AICA driver: as it just locks out all
other DMA when doing its own DMA, it is causing the GD Rom driver to
miss some reads (as the interrupts are locked between the DMA being
requested and the interrupt returning afaics). It's not terminal by any
means, but it can be annoying!
/ # /mpg123 -g 25 /mnt/01-Come\ together.mp3
High Performance MPEG 1.0/2.0/2.5 Audio Player for Layer 1, 2 and 3.
Version 0.59r-gpl (2005/04/08). Copyright 1995-2005 by The Mpg123
Project.
Uses code from various people, see 'AUTHORS' for full list.
This software comes with ABSOLUTELY NO WARRANTY. For details, see
the enclosed file COPYING for license information (GPL).
Directory: /mnt/
Playing MPEG stream from 01-Come together.mp3 ...
Junk at the beginning 49443303
MPEG 1.0 layer III, 192 kbit/s, 44100 Hz joint-stereo
[ 168.794478] irq 11, desc: 8c2723dc, depth: 1, count: 0, unhandled: 0
[ 168.800318] ->handle_irq(): 8c048580, handle_bad_irq+0x0/0x2c0
[ 168.806404] ->chip(): 8c2768c4, no_irq_chip+0x0/0x40
[ 168.811522] ->action():
[ 168.814846] IRQ_DISABLED set
[ 168.817992] unexpected IRQ trap at vector 0b
[ 173.548850] irq 11, desc: 8c2723dc, depth: 1, count: 0, unhandled: 0
[ 173.554691] ->handle_irq(): 8c048580, handle_bad_irq+0x0/0x2c0
[ 173.560777] ->chip(): 8c2768c4, no_irq_chip+0x0/0x40
[ 173.565895] ->action():
[ 173.569219] IRQ_DISABLED set
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, 2007-12-16 at 18:50 +0900, Paul Mundt wrote:
> Ok, I don't know anything about the CD-ROM layer, so I've just commented
> on the general and SH-specific stuff. Hopefully someone with a clue
> in this area (ie, not me) can offer input on the rest of the bits.
>
> > +static char gdrom_execute_diagnostic(void)
> > +{
> > + int count;
> > + /* Restart the GDROM */
> > + ctrl_outl(0x1f, GDROM_RESET_REG);
> > + for (count = 0xa000; count < 0xa020; count += 4)
> > + ctrl_inl(count);
>
> Er? This ranged dummy reading of the P2 space needs some explanation. The
> GD-ROM isn't even mapped in to this space, so this seems like a hack to
> either work around a timing issue or a write ordering problem.
>
I'll confess to not knowing what it's up to here either, other than it
looks like a mechanism to cause a G1 bus reset. This is a progressive
read of the Boot ROM area and that is all under G1 control (as is the GD
Rom obviously)
Replacing it with a simple delay of any length or, say, a read of one
word and then a delay, doesn't work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, Dec 16, 2007 at 06:50:19PM +0900, Paul Mundt wrote:
> > +static irqreturn_t gdrom_command_interrupt(int irq, void *dev_id)
> > +{
> > + if (dev_id != )
> > + return IRQ_NONE;
>
> You aren't setting this up as a shared IRQ, so this shouldn't be
> necessary.
It's not nessecary for shared irqs either. The irq code will never
pass you a different cookied back than the one you passed in.
Everything else would be a nighmare and these cargo cult workarounds
wouldn't help either.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
Ok, I don't know anything about the CD-ROM layer, so I've just commented
on the general and SH-specific stuff. Hopefully someone with a clue
in this area (ie, not me) can offer input on the rest of the bits.
On Sun, Dec 16, 2007 at 12:21:21AM +, Adrian McMenamin wrote:
> +/* GD Rom registers */
> +#define GDROM_BASE_REG 0xA05F7000
> +#define GDROM_ALTSTATUS_REG GDROM_BASE_REG + 0x18
> +#define GDROM_DATA_REG GDROM_BASE_REG + 0x80
> +#define GDROM_ERROR_REG GDROM_BASE_REG + 0x84
> +#define GDROM_INTSEC_REG GDROM_BASE_REG + 0x88
> +#define GDROM_SECNUM_REG GDROM_BASE_REG + 0x8C
> +#define GDROM_BCL_REG GDROM_BASE_REG + 0x90
> +#define GDROM_BCH_REG GDROM_BASE_REG + 0x94
> +#define GDROM_DSEL_REG GDROM_BASE_REG + 0x98
> +#define GDROM_STATUSCOMMAND_REG GDROM_BASE_REG + 0x9C
> +#define GDROM_RESET_REG GDROM_BASE_REG + 0x4E4
> +
> +#define GDROM_DATA_REG_P0 0x005F7080
> +
> +#define GDROM_DMA_STARTADDR_REG GDROM_BASE_REG + 0x404
> +#define GDROM_DMA_LENGTH_REG GDROM_BASE_REG + 0x408
> +#define GDROM_DMA_DIRECTION_REG GDROM_BASE_REG + 0x40C
> +#define GDROM_DMA_ENABLE_REG GDROM_BASE_REG + 0x414
> +#define GDROM_DMA_STATUS_REG GDROM_BASE_REG + 0x418
> +#define GDROM_DMA_WAIT_REG GDROM_BASE_REG + 0x4A0
> +#define GDROM_DMA_ACCESS_CTRL_REG GDROM_BASE_REG + 0x4B8
> +
These should all be encapsulated by brackets.
> +static void wait_clrbusy(void)
> +{
> + while (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80)
> + schedule();
> +}
> +
> +static void gdrom_wait_busy_sleeps(void)
> +{
> + /* Wait to get busy first */
> + while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) == 0)
> + schedule();
> + /* Now wait for busy to clear */
> + wait_clrbusy();
> +}
> +
Are you sure you can tolerate a schedule() in here, as opposed to a
cpu_relax()? If you're going to schedule away whilst polling a bit, you
may as well just use a wait queue and wait_on_bit() or so. This seems
like a lot of extra latency for this though.
> +static void gdrom_spicommand(void *spi_string, int buflen)
> +{
> + short *cmd = spi_string;
[snip]
> + wait_clrbusy();
> + ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
> + while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8)
> + ; /* wait for DRQ to be set to 1 */
cpu_relax()
> +static char gdrom_execute_diagnostic(void)
> +{
> + int count;
> + /* Restart the GDROM */
> + ctrl_outl(0x1f, GDROM_RESET_REG);
> + for (count = 0xa000; count < 0xa020; count += 4)
> + ctrl_inl(count);
Er? This ranged dummy reading of the P2 space needs some explanation. The
GD-ROM isn't even mapped in to this space, so this seems like a hack to
either work around a timing issue or a write ordering problem.
> +static int gdrom_get_last_session(struct cdrom_device_info *cd_info,
> struct cdrom_multisession *ms_info)
> +{
[snip]
> + }
> + }
> + else
Questionable placement of else.
> + printk(KERN_DEBUG "Disk is GDROM\n");
> + if (gd.toc)
> + kfree(gd.toc);
Useless if.
> + gd.toc = kzalloc(sizeof(struct gdromtoc), GFP_KERNEL);
> + if (!gd.toc) {
> + err = -ENOMEM;
> + goto clean_tocB;
> + }
> + if (tocuse)
Broken indendation.
> +static int gdrom_open(struct cdrom_device_info *cd_info, int purpose)
> +{
> + int err;
> + /* spin up the disk */
> + err = gdrom_preparedisk_cmd();
> + if (err)
> + return -EIO;
> +
> + return 0;
Perhaps gdrom_preparedisk_cmd() should just hand back -EIO in the error
case and you can just pass that on directly. If you have no other users
of it, then just move the work in to the gdrom_open() directly.
> +static void gdrom_release(struct cdrom_device_info *cd_info)
> +{
> +}
> +
Do you really need this? If this is some sort of driver model damage, a
comment to that extent would be helpful, otherwise just kill this off.
> +static int gdrom_mediachanged(struct cdrom_device_info *cd_info, int ignore)
> +{
> + /* check the sense key */
> + char sense = ctrl_inb(GDROM_ERROR_REG);
> + if ((sense & 0xF0) == 0x60)
> + return 1;
> + return 0;
> +}
> +
Just return (ctrl_inb(GDROM_ERROR_REG) & 0xf0) == 0x60 ?
> +static int gdrom_hardreset(struct cdrom_device_info * cd_info)
> +{
> + int count;
> + ctrl_outl(0x1f, GDROM_RESET_REG);
> + for (count = 0xa000; count < 0xa020; count += 4)
> + ctrl_inl(count);
> + return 0;
> +}
> +
More strange P2 abuse. If this is the officially recommended reset
method in the GD-ROM errat^H^H^H^Hspecification, it paints a pretty good
picture of its commercial success.
> + if (bufstring){
> + memcpy(bufstring, [4], 2); /* return additional sense
> data */
> + }
> +
Useless braces.
> + if (sense_key < 2)
> + return 0;
> + return -EIO;
> +}
> +
> +static struct cdrom_device_ops gdrom_ops = {
> + .open
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, Dec 16, 2007 at 05:32:51PM +, Adrian McMenamin wrote: On Sun, 2007-12-16 at 18:50 +0900, Paul Mundt wrote: + for (count = 0xa000; count 0xa020; count += 4) + ctrl_inl(count); Er? This ranged dummy reading of the P2 space needs some explanation. The GD-ROM isn't even mapped in to this space, so this seems like a hack to either work around a timing issue or a write ordering problem. I'll confess to not knowing what it's up to here either, other than it looks like a mechanism to cause a G1 bus reset. This is a progressive read of the Boot ROM area and that is all under G1 control (as is the GD Rom obviously) Ok, then this should be moved out to a g1_bus_reset() or something similar with a comment explaining what it's doing, that way it can be trivially reworked if a saner method of forcing a G1 reset is discovered. While I realize that this is all undocumented and based entirely on reverse engineering, you should at least verify that that's precisely what is going on, and that this is not just a precaution for flushing posted writes. You can test that by removing the loop and doing a dummy read after your write (to the same register, rather than the entire ROM space). If it's a posting issue, then you will have to do your own read/write_reg routines that handle the flush. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Mon, 2007-12-17 at 06:59 +0900, Paul Mundt wrote: While I realize that this is all undocumented and based entirely on reverse engineering, you should at least verify that that's precisely what is going on, and that this is not just a precaution for flushing posted writes. You can test that by removing the loop and doing a dummy read after your write (to the same register, rather than the entire ROM space). If it's a posting issue, then you will have to do your own read/write_reg routines that handle the flush. I've looked at this some more and, from the various things on the web, it seems SEGA introduced some sort of crude checksum security feature which ctrl_outl(0x1f, GDROM_RESET_REG) appears to start. Unless it is used then the GD Rom remains disabled. After that, the hardware executes a checksum of the ROM to ensure that you are actually booting from a Dreamcast. Well, that is my interpretation of http://www.ludd.ltu.se/~jlo/dc/bootROM.c and similar. Certainly any other sort of read seems to keep the gdrom disabled. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
Ok, I don't know anything about the CD-ROM layer, so I've just commented
on the general and SH-specific stuff. Hopefully someone with a clue
in this area (ie, not me) can offer input on the rest of the bits.
On Sun, Dec 16, 2007 at 12:21:21AM +, Adrian McMenamin wrote:
+/* GD Rom registers */
+#define GDROM_BASE_REG 0xA05F7000
+#define GDROM_ALTSTATUS_REG GDROM_BASE_REG + 0x18
+#define GDROM_DATA_REG GDROM_BASE_REG + 0x80
+#define GDROM_ERROR_REG GDROM_BASE_REG + 0x84
+#define GDROM_INTSEC_REG GDROM_BASE_REG + 0x88
+#define GDROM_SECNUM_REG GDROM_BASE_REG + 0x8C
+#define GDROM_BCL_REG GDROM_BASE_REG + 0x90
+#define GDROM_BCH_REG GDROM_BASE_REG + 0x94
+#define GDROM_DSEL_REG GDROM_BASE_REG + 0x98
+#define GDROM_STATUSCOMMAND_REG GDROM_BASE_REG + 0x9C
+#define GDROM_RESET_REG GDROM_BASE_REG + 0x4E4
+
+#define GDROM_DATA_REG_P0 0x005F7080
+
+#define GDROM_DMA_STARTADDR_REG GDROM_BASE_REG + 0x404
+#define GDROM_DMA_LENGTH_REG GDROM_BASE_REG + 0x408
+#define GDROM_DMA_DIRECTION_REG GDROM_BASE_REG + 0x40C
+#define GDROM_DMA_ENABLE_REG GDROM_BASE_REG + 0x414
+#define GDROM_DMA_STATUS_REG GDROM_BASE_REG + 0x418
+#define GDROM_DMA_WAIT_REG GDROM_BASE_REG + 0x4A0
+#define GDROM_DMA_ACCESS_CTRL_REG GDROM_BASE_REG + 0x4B8
+
These should all be encapsulated by brackets.
+static void wait_clrbusy(void)
+{
+ while (ctrl_inb(GDROM_ALTSTATUS_REG) 0x80)
+ schedule();
+}
+
+static void gdrom_wait_busy_sleeps(void)
+{
+ /* Wait to get busy first */
+ while ((ctrl_inb(GDROM_ALTSTATUS_REG) 0x80) == 0)
+ schedule();
+ /* Now wait for busy to clear */
+ wait_clrbusy();
+}
+
Are you sure you can tolerate a schedule() in here, as opposed to a
cpu_relax()? If you're going to schedule away whilst polling a bit, you
may as well just use a wait queue and wait_on_bit() or so. This seems
like a lot of extra latency for this though.
+static void gdrom_spicommand(void *spi_string, int buflen)
+{
+ short *cmd = spi_string;
[snip]
+ wait_clrbusy();
+ ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
+ while ((ctrl_inb(GDROM_ALTSTATUS_REG) 0x88) != 8)
+ ; /* wait for DRQ to be set to 1 */
cpu_relax()
+static char gdrom_execute_diagnostic(void)
+{
+ int count;
+ /* Restart the GDROM */
+ ctrl_outl(0x1f, GDROM_RESET_REG);
+ for (count = 0xa000; count 0xa020; count += 4)
+ ctrl_inl(count);
Er? This ranged dummy reading of the P2 space needs some explanation. The
GD-ROM isn't even mapped in to this space, so this seems like a hack to
either work around a timing issue or a write ordering problem.
+static int gdrom_get_last_session(struct cdrom_device_info *cd_info,
struct cdrom_multisession *ms_info)
+{
[snip]
+ }
+ }
+ else
Questionable placement of else.
+ printk(KERN_DEBUG Disk is GDROM\n);
+ if (gd.toc)
+ kfree(gd.toc);
Useless if.
+ gd.toc = kzalloc(sizeof(struct gdromtoc), GFP_KERNEL);
+ if (!gd.toc) {
+ err = -ENOMEM;
+ goto clean_tocB;
+ }
+ if (tocuse)
Broken indendation.
+static int gdrom_open(struct cdrom_device_info *cd_info, int purpose)
+{
+ int err;
+ /* spin up the disk */
+ err = gdrom_preparedisk_cmd();
+ if (err)
+ return -EIO;
+
+ return 0;
Perhaps gdrom_preparedisk_cmd() should just hand back -EIO in the error
case and you can just pass that on directly. If you have no other users
of it, then just move the work in to the gdrom_open() directly.
+static void gdrom_release(struct cdrom_device_info *cd_info)
+{
+}
+
Do you really need this? If this is some sort of driver model damage, a
comment to that extent would be helpful, otherwise just kill this off.
+static int gdrom_mediachanged(struct cdrom_device_info *cd_info, int ignore)
+{
+ /* check the sense key */
+ char sense = ctrl_inb(GDROM_ERROR_REG);
+ if ((sense 0xF0) == 0x60)
+ return 1;
+ return 0;
+}
+
Just return (ctrl_inb(GDROM_ERROR_REG) 0xf0) == 0x60 ?
+static int gdrom_hardreset(struct cdrom_device_info * cd_info)
+{
+ int count;
+ ctrl_outl(0x1f, GDROM_RESET_REG);
+ for (count = 0xa000; count 0xa020; count += 4)
+ ctrl_inl(count);
+ return 0;
+}
+
More strange P2 abuse. If this is the officially recommended reset
method in the GD-ROM errat^H^H^H^Hspecification, it paints a pretty good
picture of its commercial success.
+ if (bufstring){
+ memcpy(bufstring, sense[4], 2); /* return additional sense
data */
+ }
+
Useless braces.
+ if (sense_key 2)
+ return 0;
+ return -EIO;
+}
+
+static struct cdrom_device_ops gdrom_ops = {
+ .open = gdrom_open,
+ .release= gdrom_release,
+ .drive_status =
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, Dec 16, 2007 at 06:50:19PM +0900, Paul Mundt wrote:
+static irqreturn_t gdrom_command_interrupt(int irq, void *dev_id)
+{
+ if (dev_id != gd)
+ return IRQ_NONE;
You aren't setting this up as a shared IRQ, so this shouldn't be
necessary.
It's not nessecary for shared irqs either. The irq code will never
pass you a different cookied back than the one you passed in.
Everything else would be a nighmare and these cargo cult workarounds
wouldn't help either.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, 2007-12-16 at 18:50 +0900, Paul Mundt wrote:
Ok, I don't know anything about the CD-ROM layer, so I've just commented
on the general and SH-specific stuff. Hopefully someone with a clue
in this area (ie, not me) can offer input on the rest of the bits.
+static char gdrom_execute_diagnostic(void)
+{
+ int count;
+ /* Restart the GDROM */
+ ctrl_outl(0x1f, GDROM_RESET_REG);
+ for (count = 0xa000; count 0xa020; count += 4)
+ ctrl_inl(count);
Er? This ranged dummy reading of the P2 space needs some explanation. The
GD-ROM isn't even mapped in to this space, so this seems like a hack to
either work around a timing issue or a write ordering problem.
I'll confess to not knowing what it's up to here either, other than it
looks like a mechanism to cause a G1 bus reset. This is a progressive
read of the Boot ROM area and that is all under G1 control (as is the GD
Rom obviously)
Replacing it with a simple delay of any length or, say, a read of one
word and then a delay, doesn't work.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, 2007-12-16 at 18:50 +0900, Paul Mundt wrote:
+static int gdrom_readdisk_dma(int block, int block_cnt, char *buffer)
+{
+ int err;
+ struct packet_command *read_command;
+ /* release the spin lock but check later
+* we're not in the middle of some dma */
+ spin_unlock(gdrom_lock);
The locking strategy here is a bit interesting, has spinlock debugging
tossed any profanities to your console?
No, no errors reported.
I think I've been careful - I release the lock because we're doing
things like allocating memory and then sleeping on the queue.
But I also need to check various registers so that (a) we don't try to
do a second DMA until the hardware is ready and (b) we properly
serialize access to the device by two different threads of execution.
Seems to work.
Though I have an issue with the AICA driver: as it just locks out all
other DMA when doing its own DMA, it is causing the GD Rom driver to
miss some reads (as the interrupts are locked between the DMA being
requested and the interrupt returning afaics). It's not terminal by any
means, but it can be annoying!
/ # /mpg123 -g 25 /mnt/01-Come\ together.mp3
High Performance MPEG 1.0/2.0/2.5 Audio Player for Layer 1, 2 and 3.
Version 0.59r-gpl (2005/04/08). Copyright 1995-2005 by The Mpg123
Project.
Uses code from various people, see 'AUTHORS' for full list.
This software comes with ABSOLUTELY NO WARRANTY. For details, see
the enclosed file COPYING for license information (GPL).
Directory: /mnt/
Playing MPEG stream from 01-Come together.mp3 ...
Junk at the beginning 49443303
MPEG 1.0 layer III, 192 kbit/s, 44100 Hz joint-stereo
[ 168.794478] irq 11, desc: 8c2723dc, depth: 1, count: 0, unhandled: 0
[ 168.800318] -handle_irq(): 8c048580, handle_bad_irq+0x0/0x2c0
[ 168.806404] -chip(): 8c2768c4, no_irq_chip+0x0/0x40
[ 168.811522] -action():
[ 168.814846] IRQ_DISABLED set
[ 168.817992] unexpected IRQ trap at vector 0b
[ 173.548850] irq 11, desc: 8c2723dc, depth: 1, count: 0, unhandled: 0
[ 173.554691] -handle_irq(): 8c048580, handle_bad_irq+0x0/0x2c0
[ 173.560777] -chip(): 8c2768c4, no_irq_chip+0x0/0x40
[ 173.565895] -action():
[ 173.569219] IRQ_DISABLED set
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Saturday 15 December 2007, Adrian McMenamin wrote:
diff -ruN ./linux-2.6-orig/drivers/sh/gdrom/gdrom.c
./linux-2.6/drivers/sh/gdrom/gdrom.c
i think your e-mail client word wrapped a little ...
+ if (gd.toc)
+ kfree(gd.toc);
i dont know how the kernel functions, but in userspace, free(NULL) is
acceptable ...
+ memcpy(gd.toc, tocB, sizeof (struct gdromtoc));
+ else
+ memcpy(gd.toc, tocA, sizeof (struct gdromtoc));
since gd.toc and toc[BA] are of the same type, cant you just:
*gd.toc = *tocA
also, since tocB/tocA only exist in this function (you kzalloc() at the top
and kfree() at the bottom), and you dont do something like gd.toc = tocA,
why use the memory allocator at all ? i dont think they are too large for
the stack (~400bytes a piece) ... maybe they are ...
+static int gdrom_open(struct cdrom_device_info *cd_info, int purpose)
+{
+ int err;
+ /* spin up the disk */
+ err = gdrom_preparedisk_cmd();
+ if (err)
+ return -EIO;
+
+ return 0;
+}
is it normal to normalize all errors like this ? it'd be a much simpler
function like:
{ return gdrom_preparedisk_cmd(); }
+static irqreturn_t gdrom_command_interrupt(int irq, void *dev_id)
+{
+ if (dev_id != gd)
+ return IRQ_NONE;
+ gd.status = ctrl_inb(GDROM_STATUSCOMMAND_REG);
+ if (gd.pending != 1)
+ return IRQ_HANDLED;
+static irqreturn_t gdrom_dma_interrupt(int irq, void *dev_id)
+{
+ if (dev_id != gd)
+ return IRQ_NONE;
+ gd.status = ctrl_inb(GDROM_STATUSCOMMAND_REG);
+ if (gd.transfer != 1)
+ return IRQ_HANDLED;
if you dont have a pending interrupt, shouldnt it return IRQ_NONE here ? Paul
already mentioned the weird dev_id check.
+static int gdrom_readdisk_dma(int block, int block_cnt, char *buffer)
+{
+ int err;
+ struct packet_command *read_command;
+ /* release the spin lock but check later
+ * we're not in the middle of some dma */
+ spin_unlock(gdrom_lock);
+ ctrl_outl(0x8843407F, GDROM_DMA_ACCESS_CTRL_REG); /* memory setting */
it'd be nice if these magic #'s had more explanation behind them, but you may
simply not have that information :/
+static void gdrom_request(struct request_queue *rq)
+{
+ struct request *req;
+ unsigned long pages;
+ pages = rq-backing_dev_info.ra_pages;
+ while ((req = elv_next_request(rq)) != NULL) {
+ if (! blk_fs_request(req)) {
+ printk(KERN_DEBUG GDROM: Non-fs request ignored\n);
+ end_request(req, 0);
+ }
+ if (rq_data_dir(req)) {
+ printk(KERN_NOTICE GDROM: Read only device - write
request
ignored\n); +end_request(req, 0);
+ }
+ if (req-nr_sectors) {
+ gdrom_request_handler_dma(req);
+ }
+ }
+}
no need for all the {} in the last two if()'s
+/* Print string identifying GD ROM device */
+static void gdrom_outputversion(void)
+{
+ struct gdrom_id *id;
+ char *model_name, *manuf_name, *firmw_ver;
+ /* query device ID */
+ id = kzalloc(sizeof(struct gdrom_id), GFP_KERNEL);
i dont know how other people feel, but i think the style:
sizeof(*id)
is cleaner and leads to less bitrot ... also, you dont have an if (!id)
check there ... Paul pointed out plenty of other stuff for this func ;)
also, wrt to sizes (16 and 17), arent there some defines you can key off
of or something ?
+MODULE_DESCRIPTION(GD-ROM Driver);
SEGA Dreamcast GD-ROM Driver ?
-mike
signature.asc
Description: This is a digitally signed message part.
[PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
diff -ruN ./linux-2.6-orig/drivers/sh/gdrom/gdrom.c
./linux-2.6/drivers/sh/gdrom/gdrom.c
--- ./linux-2.6-orig/drivers/sh/gdrom/gdrom.c 1970-01-01
01:00:00.0 +0100
+++ ./linux-2.6/drivers/sh/gdrom/gdrom.c2007-12-15 22:58:17.0
+
@@ -0,0 +1,793 @@
+/* GD ROM driver for the SEGA Dreamcast
+ * copyright Adrian McMenamin, 2007
+ * With thanks to Marcus Comstedt and Nathan Keynes
+ * for work in reversing PIO and DMA
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define GDROM_DEV_NAME "gdrom"
+#define GD_SESSION_OFFSET 150
+
+/* GD Rom commands */
+#define GDROM_COM_SOFTRESET 0x08
+#define GDROM_COM_EXECDIAG 0x90
+#define GDROM_COM_PACKET 0xA0
+#define GDROM_COM_IDDEV 0xA1
+
+/* GD Rom registers */
+#define GDROM_BASE_REG 0xA05F7000
+#define GDROM_ALTSTATUS_REG GDROM_BASE_REG + 0x18
+#define GDROM_DATA_REG GDROM_BASE_REG + 0x80
+#define GDROM_ERROR_REG GDROM_BASE_REG + 0x84
+#define GDROM_INTSEC_REG GDROM_BASE_REG + 0x88
+#define GDROM_SECNUM_REG GDROM_BASE_REG + 0x8C
+#define GDROM_BCL_REG GDROM_BASE_REG + 0x90
+#define GDROM_BCH_REG GDROM_BASE_REG + 0x94
+#define GDROM_DSEL_REG GDROM_BASE_REG + 0x98
+#define GDROM_STATUSCOMMAND_REG GDROM_BASE_REG + 0x9C
+#define GDROM_RESET_REG GDROM_BASE_REG + 0x4E4
+
+#define GDROM_DATA_REG_P0 0x005F7080
+
+#define GDROM_DMA_STARTADDR_REG GDROM_BASE_REG + 0x404
+#define GDROM_DMA_LENGTH_REG GDROM_BASE_REG + 0x408
+#define GDROM_DMA_DIRECTION_REG GDROM_BASE_REG + 0x40C
+#define GDROM_DMA_ENABLE_REG GDROM_BASE_REG + 0x414
+#define GDROM_DMA_STATUS_REG GDROM_BASE_REG + 0x418
+#define GDROM_DMA_WAIT_REG GDROM_BASE_REG + 0x4A0
+#define GDROM_DMA_ACCESS_CTRL_REG GDROM_BASE_REG + 0x4B8
+
+#define GDROMDEBUG 0
+
+#define GDROM_HARD_SECTOR 2048
+#define BLOCK_LAYER_SECTOR 512
+#define GD_TO_BLK 4
+
+static struct platform_device *pd;
+static int gdrom_major;
+static wait_queue_head_t command_queue;
+static wait_queue_head_t request_queue;
+
+static DEFINE_SPINLOCK(gdrom_lock);
+
+struct gdromtoc {
+ unsigned int entry[99];
+ unsigned int first, last;
+ unsigned int leadout;
+};
+
+static struct gdrom_unit {
+ struct gendisk *disk;
+ struct cdrom_device_info *cd_info;
+ int status;
+ int pending;
+ int transfer;
+ char disk_type;
+ struct gdromtoc *toc;
+ struct request_queue *gdrom_rq;
+} gd;
+
+struct gdrom_id {
+ char mid;
+ char modid;
+ char verid;
+ char padA[13];
+ char mname[16];
+ char modname[16];
+ char firmver[16];
+ char padB[16];
+};
+
+static int gdrom_getsense(short *bufstring);
+static int gdrom_packetcommand(struct cdrom_device_info * cd_info,
struct packet_command *command);
+
+static void wait_clrbusy(void)
+{
+ while (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80)
+ schedule();
+}
+
+static void gdrom_wait_busy_sleeps(void)
+{
+ /* Wait to get busy first */
+ while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) == 0)
+ schedule();
+ /* Now wait for busy to clear */
+ wait_clrbusy();
+}
+
+static void gdrom_identifydevice(void *buf)
+{
+ int c;
+ short *data = buf;
+ wait_clrbusy();
+ ctrl_outb(GDROM_COM_IDDEV, GDROM_STATUSCOMMAND_REG);
+ gdrom_wait_busy_sleeps();
+ /* now read in the data */
+ for (c = 0; c < 40; c++)
+ data[c] = ctrl_inw(GDROM_DATA_REG);
+}
+
+static void gdrom_spicommand(void *spi_string, int buflen)
+{
+ short *cmd = spi_string;
+ /* ensure IRQ_WAIT is set */
+ ctrl_outb(0x08, GDROM_ALTSTATUS_REG);
+ /* specify how many bytes we expect back */
+ ctrl_outb(buflen & 0xFF, GDROM_BCL_REG);
+ ctrl_outb((buflen >> 8) & 0xFF, GDROM_BCH_REG);
+ /* other parameters */
+ ctrl_outb(0, GDROM_INTSEC_REG);
+ ctrl_outb(0, GDROM_SECNUM_REG);
+ ctrl_outb(0, GDROM_ERROR_REG);
+ /* Wait until we can go */
+ wait_clrbusy();
+ ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
+ while
[PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
diff -ruN ./linux-2.6-orig/drivers/sh/gdrom/gdrom.c
./linux-2.6/drivers/sh/gdrom/gdrom.c
--- ./linux-2.6-orig/drivers/sh/gdrom/gdrom.c 1970-01-01
01:00:00.0 +0100
+++ ./linux-2.6/drivers/sh/gdrom/gdrom.c2007-12-15 22:58:17.0
+
@@ -0,0 +1,793 @@
+/* GD ROM driver for the SEGA Dreamcast
+ * copyright Adrian McMenamin, 2007
+ * With thanks to Marcus Comstedt and Nathan Keynes
+ * for work in reversing PIO and DMA
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include linux/init.h
+#include linux/module.h
+#include linux/fs.h
+#include linux/kernel.h
+#include linux/list.h
+#include linux/slab.h
+#include linux/dma-mapping.h
+#include linux/cdrom.h
+#include linux/genhd.h
+#include linux/bio.h
+#include linux/blkdev.h
+#include linux/interrupt.h
+#include linux/device.h
+#include linux/wait.h
+#include linux/workqueue.h
+#include linux/platform_device.h
+#include asm/io.h
+#include asm/dma.h
+#include asm/delay.h
+#include asm/mach/dma.h
+#include asm/mach/sysasic.h
+
+#define GDROM_DEV_NAME gdrom
+#define GD_SESSION_OFFSET 150
+
+/* GD Rom commands */
+#define GDROM_COM_SOFTRESET 0x08
+#define GDROM_COM_EXECDIAG 0x90
+#define GDROM_COM_PACKET 0xA0
+#define GDROM_COM_IDDEV 0xA1
+
+/* GD Rom registers */
+#define GDROM_BASE_REG 0xA05F7000
+#define GDROM_ALTSTATUS_REG GDROM_BASE_REG + 0x18
+#define GDROM_DATA_REG GDROM_BASE_REG + 0x80
+#define GDROM_ERROR_REG GDROM_BASE_REG + 0x84
+#define GDROM_INTSEC_REG GDROM_BASE_REG + 0x88
+#define GDROM_SECNUM_REG GDROM_BASE_REG + 0x8C
+#define GDROM_BCL_REG GDROM_BASE_REG + 0x90
+#define GDROM_BCH_REG GDROM_BASE_REG + 0x94
+#define GDROM_DSEL_REG GDROM_BASE_REG + 0x98
+#define GDROM_STATUSCOMMAND_REG GDROM_BASE_REG + 0x9C
+#define GDROM_RESET_REG GDROM_BASE_REG + 0x4E4
+
+#define GDROM_DATA_REG_P0 0x005F7080
+
+#define GDROM_DMA_STARTADDR_REG GDROM_BASE_REG + 0x404
+#define GDROM_DMA_LENGTH_REG GDROM_BASE_REG + 0x408
+#define GDROM_DMA_DIRECTION_REG GDROM_BASE_REG + 0x40C
+#define GDROM_DMA_ENABLE_REG GDROM_BASE_REG + 0x414
+#define GDROM_DMA_STATUS_REG GDROM_BASE_REG + 0x418
+#define GDROM_DMA_WAIT_REG GDROM_BASE_REG + 0x4A0
+#define GDROM_DMA_ACCESS_CTRL_REG GDROM_BASE_REG + 0x4B8
+
+#define GDROMDEBUG 0
+
+#define GDROM_HARD_SECTOR 2048
+#define BLOCK_LAYER_SECTOR 512
+#define GD_TO_BLK 4
+
+static struct platform_device *pd;
+static int gdrom_major;
+static wait_queue_head_t command_queue;
+static wait_queue_head_t request_queue;
+
+static DEFINE_SPINLOCK(gdrom_lock);
+
+struct gdromtoc {
+ unsigned int entry[99];
+ unsigned int first, last;
+ unsigned int leadout;
+};
+
+static struct gdrom_unit {
+ struct gendisk *disk;
+ struct cdrom_device_info *cd_info;
+ int status;
+ int pending;
+ int transfer;
+ char disk_type;
+ struct gdromtoc *toc;
+ struct request_queue *gdrom_rq;
+} gd;
+
+struct gdrom_id {
+ char mid;
+ char modid;
+ char verid;
+ char padA[13];
+ char mname[16];
+ char modname[16];
+ char firmver[16];
+ char padB[16];
+};
+
+static int gdrom_getsense(short *bufstring);
+static int gdrom_packetcommand(struct cdrom_device_info * cd_info,
struct packet_command *command);
+
+static void wait_clrbusy(void)
+{
+ while (ctrl_inb(GDROM_ALTSTATUS_REG) 0x80)
+ schedule();
+}
+
+static void gdrom_wait_busy_sleeps(void)
+{
+ /* Wait to get busy first */
+ while ((ctrl_inb(GDROM_ALTSTATUS_REG) 0x80) == 0)
+ schedule();
+ /* Now wait for busy to clear */
+ wait_clrbusy();
+}
+
+static void gdrom_identifydevice(void *buf)
+{
+ int c;
+ short *data = buf;
+ wait_clrbusy();
+ ctrl_outb(GDROM_COM_IDDEV, GDROM_STATUSCOMMAND_REG);
+ gdrom_wait_busy_sleeps();
+ /* now read in the data */
+ for (c = 0; c 40; c++)
+ data[c] = ctrl_inw(GDROM_DATA_REG);
+}
+
+static void gdrom_spicommand(void *spi_string, int buflen)
+{
+ short *cmd = spi_string;
+ /* ensure IRQ_WAIT is set */
+ ctrl_outb(0x08, GDROM_ALTSTATUS_REG);
+ /* specify how many bytes we expect back */
+ ctrl_outb(buflen 0xFF, GDROM_BCL_REG);
+ ctrl_outb((buflen 8) 0xFF, GDROM_BCH_REG);
+ /* other parameters
