* Re: [patch 4/7] ps3: Storage Driver Probing
From: Benjamin Herrenschmidt @ 2007-05-25 22:54 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linuxppc-dev, linux-kernel, Arnd Bergmann
In-Reply-To: <Pine.LNX.4.62.0705252143270.2657@pademelon.sonytel.be>
On Fri, 2007-05-25 at 21:48 +0200, Geert Uytterhoeven wrote:
> > > + msleep_interruptible(ms);
> > > + if (ms < 60000)
> > > + ms <<= 1;
> >
> > Is this timeout only for the disk spinup, or also for detecting
> media
> > added at run time, like inserting a DVD? One minute timeout for
> > detecting a DVD would sound very long to me.
>
> It's not for inserting DVDs, only for new devices showing up in the
> repository.
> Apparently new devices may keep on showing up a while after boot up,
> but I
> think this matters only for the kboot kernel, that's why I went with
> the
> exponential back-off with upper limit.
Why not just have a kthread poll at 2 second interval for new devices or
removed ones ?
(And not for request processing)
Ben.
^ permalink raw reply
* Re: [patch 5/7] ps3: Disk Storage Driver
From: Benjamin Herrenschmidt @ 2007-05-25 22:53 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linuxppc-dev, linux-kernel, Arnd Bergmann
In-Reply-To: <Pine.LNX.4.62.0705252137230.2657@pademelon.sonytel.be>
On Fri, 2007-05-25 at 21:40 +0200, Geert Uytterhoeven wrote:
>
> I used a kthread because the request function of a block device driver
> must be
> non-blocking, and ps3stor_read_write_sectors() calls
> wait_for_completion().
Which as I said before looks terribly sad... Why the heck would it have
to do that ?
Ben.
^ permalink raw reply
* Re: [patch 5/7] ps3: Disk Storage Driver
From: Benjamin Herrenschmidt @ 2007-05-25 22:48 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Geert.Uytterhoeven, linuxppc-dev, linux-kernel
In-Reply-To: <200705251826.25572.arnd@arndb.de>
> So the hypervison uses guest-real addresses here? I would have expected
> it to use the kernel page tables, which lets you use vmap() to do
> scatter-gather.
Ugh ? Maybe s390 can do that but no other hypervisor that I know
about :-) It would be nice, sure, but heh.
> I don't really understand what the kthread is needed for. You probably
> thought about multiple options and ended up with this, but having
> a comment in front of it might be helpful.
Yeah, me neither... the driver looks very very very unefficient to me. I
though the kthread was useful for hotplug detection becasue the
hypervisor don't signal us, but from the patch, it looks like it's also
used for actual request processing which is very yucky.
Ben.
^ permalink raw reply
* Re: [patch 4/7] ps3: Storage Driver Probing
From: Benjamin Herrenschmidt @ 2007-05-25 22:47 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Geert.Uytterhoeven, linuxppc-dev, linux-kernel
In-Reply-To: <200705251818.03964.arnd@arndb.de>
On Fri, 2007-05-25 at 18:18 +0200, Arnd Bergmann wrote:
> > +static u64 ps3stor_wait_for_completion(u64 devid, u64 tag,
> > + unsigned int timeout)
> > +{
> > + unsigned int retries = 0;
> > + u64 res = -1, status;
> > +
> > + for (retries = 0; retries < timeout; retries++) {
> > + res =
> lv1_storage_check_async_status(NOTIFICATION_DEVID, tag,
> > + &status);
> > + if (!res)
> > + break;
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + schedule_timeout(1);
> > + }
>
> Any reason not to use msleep(1) instead of the schedule_timeout?
Both look equally ugly though... do you really have to poll ?
Ben.
^ permalink raw reply
* Re: [patch 6/7] ps3: ROM Storage Driver
From: Benjamin Herrenschmidt @ 2007-05-25 22:45 UTC (permalink / raw)
To: Olaf Hering; +Cc: Geert.Uytterhoeven, linuxppc-dev, linux-kernel
In-Reply-To: <20070525112414.GA32336@aepfle.de>
On Fri, 2007-05-25 at 13:24 +0200, Olaf Hering wrote:
> On Fri, May 25, Geert.Uytterhoeven@sonycom.com wrote:
>
> > +++ b/drivers/scsi/ps3rom.c
>
> > + kaddr = kmap_atomic(sgpnt->page, KM_USER0);
>
> linux/highmem.h is not included to get the kmap_* prototypes.
Beside, I don't see the point of using kmap on ppc64...
Ben.
^ permalink raw reply
* Re: [patch 5/7] ps3: Disk Storage Driver
From: Arnd Bergmann @ 2007-05-25 22:45 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <Pine.LNX.4.62.0705252318160.2657@pademelon.sonytel.be>
On Friday 25 May 2007, Geert Uytterhoeven wrote:
> On Fri, 25 May 2007, Arnd Bergmann wrote:
>
> > Ok, but why does it call wait_for_completion() then?
> > I thought you could end_that_request_* from the interrupt handler instead.
>
> Actually I tried that first, but I ran into other problems, like my request
> handler being called continuously and requests gotten stuck. But maybe it was
> just a locking bug on my side.
>
> I can retry, but a disadvantage will be that there will be less code shared
> with ps3flash and ps3rom.
Not sure how much difference it will make performance-wise, but the context
switch for each bio adds some extra cost at least. Changing it
means you no longer share the ps3stor_read_write_sectors, but can at
the same time simplify the disk driver, so that won't hurt in total.
I don't care much, but I think it's worth trying.
Arnd <><
^ permalink raw reply
* Re: [Patch ] Kexec/Kdump support - POWER6
From: Benjamin Herrenschmidt @ 2007-05-25 22:43 UTC (permalink / raw)
To: sachinp
Cc: Olof Johansson, linuxppc-dev, Paul Mackerras, kexec,
Milton Miller II
In-Reply-To: <4656A488.2020507@in.ibm.com>
>
> /* TODO: Use bulk call */
> - for (i = 0; i < hpte_count; i++)
> - plpar_pte_remove_raw(0, i, 0, &dummy1, &dummy2);
> + for (i = 0; i < hpte_count; i++) {
> + /* dont remove HPTEs with VRMA mappings */
> + lpar_rc = plpar_pte_remove_raw(H_ANDCOND, i, VRMA_HPTE_B_1TB,
> + &dummy1, &dummy2);
If you're going to use the B (1T segment) bit instead of the bolted bit,
at least define a proper constant in line with the existing naming of
the hash table constants in mmu-hash64.h. I would suggest doing the same
with the VRMA_MASK/VALUE thing and calling it HPTE_V_VRMA_MASK or
something similar.
> + if (lpar_rc == H_NOT_FOUND) {
> + lpar_rc = plpar_pte_read_raw(0, i, &dword0, &dummy1);
> + if (!lpar_rc &&
> + (((dword0 >> 24) & VRMA_MASK) != VRMA_MASK))
> + /* Can be hpte for 1TB Seg. So remove it */
> + plpar_pte_remove_raw(0, i, 0, &dummy1, &dummy2);
> + }
> + }
> }
In addition, I would recommend following Michael's advice and using
using the bulk remove Hcall whenever possible.
Cheers,
Ben.
^ permalink raw reply
* Re: [patch 1/7] ps3: Preallocate bootmem memory for the PS3 FLASH ROM storage driver
From: Benjamin Herrenschmidt @ 2007-05-25 22:35 UTC (permalink / raw)
To: Geert.Uytterhoeven; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20070525083632.177254000@sonycom.com>
On Fri, 2007-05-25 at 10:36 +0200, Geert.Uytterhoeven@sonycom.com wrote:
> -#if defined(CONFIG_FB_PS3) || defined(CONFIG_FB_PS3_MODULE)
> +#if defined(CONFIG_FB_PS3) || defined(CONFIG_FB_PS3_MODULE) || \
> + defined(CONFIG_PS3_FLASH_MODULE) ||
> defined(CONFIG_PS3_FLASH_MODULE)
As I said multiple times, imho, #ifdef CONFIG_xxx_MODULE in the kernel
is always a bug.
You should always be able to build the module out of tree afteward and
use it on a kernel that didn't have the CONFIG_xxx_MODULE set imho.
Ben.
^ permalink raw reply
* Re: Latest GCC/GLIBC working combo for a Linux 2.4.x kernel?
From: Wolfgang Denk @ 2007-05-25 21:39 UTC (permalink / raw)
To: Sasvata Chatterjee; +Cc: linuxppc-embedded
In-Reply-To: <a9fc2a630705241728q4d203569n6eb008cc65a7eba9@mail.gmail.com>
In message <a9fc2a630705241728q4d203569n6eb008cc65a7eba9@mail.gmail.com> you wrote:
>
> Does anybody have any working version using newer versions of the 2.4 kernel
> and newer GCC versions for a powerpc-603e target? One motivation is to use
> BusyBox-1.5.1, but I am told on that list that GCC 2/x is too old (I tried
> before I asked, it doesn't compile).
Did you have a look at the ELDK?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
There are bugs and then there are bugs. And then there are bugs.
- Karl Lehenbauer
^ permalink raw reply
* Re: [patch 5/7] ps3: Disk Storage Driver
From: Geert Uytterhoeven @ 2007-05-25 21:22 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <200705252243.41508.arnd@arndb.de>
On Fri, 25 May 2007, Arnd Bergmann wrote:
> On Friday 25 May 2007, Geert Uytterhoeven wrote:
> > > I don't really understand what the kthread is needed for. You probably
> > > thought about multiple options and ended up with this, but having
> > > a comment in front of it might be helpful.
> >
> > I used a kthread because the request function of a block device driver must be
> > non-blocking, and ps3stor_read_write_sectors() calls wait_for_completion().
>
> Ok, but why does it call wait_for_completion() then?
> I thought you could end_that_request_* from the interrupt handler instead.
Actually I tried that first, but I ran into other problems, like my request
handler being called continuously and requests gotten stuck. But maybe it was
just a locking bug on my side.
I can retry, but a disadvantage will be that there will be less code shared
with ps3flash and ps3rom.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium
^ permalink raw reply
* Re: [patch 6/7] ps3: ROM Storage Driver
From: Arnd Bergmann @ 2007-05-25 21:04 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Geert Uytterhoeven, linux-kernel
In-Reply-To: <Pine.LNX.4.62.0705252135010.2657@pademelon.sonytel.be>
On Friday 25 May 2007, Geert Uytterhoeven wrote:
>
> > What is the problem? Is there infrastructure missing in the
> > CD-ROM layer?
>
> As the CD/DVD/BD part just accepts SCSI/ATAPI commands (except for plain
> read/write), I was suggested to keep it as a SCSI driver.
Ok, so I guess the tradeoff here is that by writing it as a SCSI
driver, you don't need to implement any of the cdrom_device_ops
yourself but instead need to fake a few of the SCSI commands
in ps3rom_request(). Fair enough.
Arnd <><
^ permalink raw reply
* Re: [PATCH 1/13] powerpc: Add Makefile rules to wrap dts file in zImage
From: Mark A. Greer @ 2007-05-25 20:50 UTC (permalink / raw)
To: Mark A. Greer, Paul Mackerras, linuxppc-dev
In-Reply-To: <20070525013359.GD11789@localhost.localdomain>
On Fri, May 25, 2007 at 11:33:59AM +1000, David Gibson wrote:
> On Fri, May 25, 2007 at 11:32:42AM +1000, David Gibson wrote:
> > On Thu, May 24, 2007 at 02:28:32PM -0700, Mark A. Greer wrote:
> > > On Tue, May 22, 2007 at 10:23:49AM +1000, David Gibson wrote:
> > > > Just leave them out. As long as you add the correct specific
> > > > zImage.dts.% and zImage.dts_initrd.% targets to image-y and initrd-y,
> > > > then a "make zImage" from the top-level Makefile will get them built.
> > >
> > > That defeats the whole purpose of the patch--to allow the selection of
> > > wrapping a dtb or not at build time. Its just like initrd, you choose
> > > whether to "do it" at build time.
> >
> > Ah, right. Remind me why that's useful again?
>
> Sorry, to be more precise, why is that more useful than having "make
> zImage" at the top level build both the dts and non-dts versions.
Well, that is a good question. This is my reasoning which I put in the
patch description:
"Having separate rules allows the user to choose whether to include
a device tree--and which device tree--at build time. This is
useful when one Makefile target builds a zImage that runs on several
platforms except for differing device trees. By just setting
CONFIG_DEVICE_TREE and running "make zImage.dts" the exact zImage
you want is built without Makefile bloat or manually running the
wrapper script."
Making both seems like a waste but its such a small waste that it
doesn't really matter. If you still object to this feature, I'll
take it out since I (read: prpmc2800) doesn't really need this
feature anyway.
Mark
^ permalink raw reply
* Re: [patch 5/7] ps3: Disk Storage Driver
From: Olaf Hering @ 2007-05-25 20:47 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <Pine.LNX.4.62.0705252143070.2657@pademelon.sonytel.be>
On Fri, May 25, Geert Uytterhoeven wrote:
> On Fri, 25 May 2007, Olaf Hering wrote:
> > On Fri, May 25, Geert.Uytterhoeven@sonycom.com wrote:
> > > Add a Disk Storage Driver for the PS3:
> >
> > There is no device symlink in /sys/block/ps3da/
>
> Interesting... Do you know how to create it?
No, that was not obvious.
But for gelic it goes like this:
+++ b/drivers/net/gelic_net.c
@@ -1364,6 +1364,7 @@ static int gelic_net_setup_netdev(struct
u64 v1, v2;
SET_MODULE_OWNER(netdev);
+ SET_NETDEV_DEV(netdev, &card->dev->core);
spin_lock_init(&card->tx_dma_lock);
card->rx_csum = GELIC_NET_RX_CSUM_DEFAULT;
^ permalink raw reply
* Re: [patch 5/7] ps3: Disk Storage Driver
From: Arnd Bergmann @ 2007-05-25 20:43 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Geert Uytterhoeven, linux-kernel
In-Reply-To: <Pine.LNX.4.62.0705252137230.2657@pademelon.sonytel.be>
On Friday 25 May 2007, Geert Uytterhoeven wrote:
>
> > I don't really understand what the kthread is needed for. You probably
> > thought about multiple options and ended up with this, but having
> > a comment in front of it might be helpful.
>
> I used a kthread because the request function of a block device driver must be
> non-blocking, and ps3stor_read_write_sectors() calls wait_for_completion().
Ok, but why does it call wait_for_completion() then?
I thought you could end_that_request_* from the interrupt handler instead.
Arnd <><
^ permalink raw reply
* Re: [patch 4/7] ps3: Storage Driver Probing
From: Geert Uytterhoeven @ 2007-05-25 19:48 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <200705251818.03964.arnd@arndb.de>
On Fri, 25 May 2007, Arnd Bergmann wrote:
> On Friday 25 May 2007, Geert.Uytterhoeven@sonycom.com wrote:
> > +static u64 ps3stor_wait_for_completion(u64 devid, u64 tag,
> > + unsigned int timeout)
> > +{
> > + unsigned int retries = 0;
> > + u64 res = -1, status;
> > +
> > + for (retries = 0; retries < timeout; retries++) {
> > + res = lv1_storage_check_async_status(NOTIFICATION_DEVID, tag,
> > + &status);
> > + if (!res)
> > + break;
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + schedule_timeout(1);
> > + }
>
> Any reason not to use msleep(1) instead of the schedule_timeout?
Not really. I'll change it.
> > + switch (dev_type) {
> > + case PS3_DEV_TYPE_STOR_DISK:
> > + match_id = PS3_MATCH_ID_STOR_DISK;
> > + break;
> > +
> > + case PS3_DEV_TYPE_STOR_ROM:
> > + match_id = PS3_MATCH_ID_STOR_ROM;
> > + break;
> > +
> > + case PS3_DEV_TYPE_STOR_FLASH:
> > + match_id = PS3_MATCH_ID_STOR_FLASH;
> > + break;
> > +
> > + default:
> > + return 0;
> > + }
>
> Why do you have separate constants for PS3_DEV_TYPE_* and
> PS3_MATCH_ID_*? If you don't do any conversion, this driver
> will immediately work for additional types as well, if more
> get added later.
The PS3_DEV_TYPE_* IDs are imposed by the repository, as created by the
hypervisor.
The PS3_MATCH_ID_* IDs are created by us, for all PS3-specific devices.
As Geoff already pointed out, we may be able to use PS3_DEV_TYPE_* IDs for
everything, but unfortunately not all PS3-specific devices are present in the
repository.
> > +
> > +// pr_debug("%s:%u: Checking for new storage devices...\n",
> > +// __func__, __LINE__);
>
> Should be removed, or not in comments, either way is fine, as pr_debug
> normally does not get compiled in anyway.
Oops, forgot to uncomment it (it was a bit noisy while I lived with DEBUG
defined ;-)
> > + msleep_interruptible(ms);
> > + if (ms < 60000)
> > + ms <<= 1;
>
> Is this timeout only for the disk spinup, or also for detecting media
> added at run time, like inserting a DVD? One minute timeout for
> detecting a DVD would sound very long to me.
It's not for inserting DVDs, only for new devices showing up in the repository.
Apparently new devices may keep on showing up a while after boot up, but I
think this matters only for the kboot kernel, that's why I went with the
exponential back-off with upper limit.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium
^ permalink raw reply
* Re: [patch 5/7] ps3: Disk Storage Driver
From: Geert Uytterhoeven @ 2007-05-25 19:43 UTC (permalink / raw)
To: Olaf Hering; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20070525114550.GA32552@aepfle.de>
On Fri, 25 May 2007, Olaf Hering wrote:
> On Fri, May 25, Geert.Uytterhoeven@sonycom.com wrote:
> > Add a Disk Storage Driver for the PS3:
>
> There is no device symlink in /sys/block/ps3da/
Interesting... Do you know how to create it?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium
^ permalink raw reply
* taiga/7448 questions
From: Leisner, Martin @ 2007-05-25 19:40 UTC (permalink / raw)
To: linuxppc-embedded; +Cc: Lund, Nathan
OK, I built 2.6.20.11 and 2.6.21.1 for taiga...with the supplied config=20
(and I didn't know about the dtc for powerpc until I booted and nothing
happened).
I changed a few things (like /proc/config.gz)
A few issues I've seen:
1) after we start booting, the console turns off until the login message
appears:
taiga login: [ 0.000000] Zone PFN ranges:
000000] Normal 131072 -> 131072
[ 0.000000] early_node_map[1] active PFN ranges
[ 0.000000] 0: 0 -> 131072
[ 0.000000] Built 1 zonelists. Total pages: 130048
[ 0.000000] Kernel command line: root=3D/dev/sda2 =
console=3DttyS0,11520
[ 0.000000] mpic: Setting up MPIC "Tsi108_PIC" version 1.2 at
c0007400, max 4 CPUs
[ 0.000000] mpic: ISU size: 24, shift: 5, mask: 1f
[ 0.000000] mpic: Initializing for 24 sources
[ 0.000000] PID hash table entries: 2048 (order: 11, 8192 bytes)
Linux System
Kernel 2.6.21.1 on an ppc
Version: #2 Thu May 24 14:37:53 EDT 2007
This is consistent on 2.6.20.11 and 2.6.21.1...
2) Does "reboot" work? Even when I do a <sysrq-magic-b> it doesn't want
to boot
until I do a hardware reset....
3) It seems 2.6.21.1 consistently works, 2.6.20.11 sometimes hangs (and
since I can't see the
console, I don't know what's hanging and why)
4) The dtb strategy seems to a little clumsy...is there a way to embed a
default dtc into a kernel
(so if you just tftp one file, something happens?) I like the ability
to do fewer operations (i.e.
initramfs embedded in the image) -- I like reducing steps into of
increasing them...
Is there a way to tell the kernel "use a default dtb if you don't have
one better?"
marty
^ permalink raw reply
* Re: [patch 5/7] ps3: Disk Storage Driver
From: Geert Uytterhoeven @ 2007-05-25 19:40 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <200705251826.25572.arnd@arndb.de>
On Fri, 25 May 2007, Arnd Bergmann wrote:
> On Friday 25 May 2007, Geert.Uytterhoeven@sonycom.com wrote:
> > +static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
> > + struct request *req, int gather)
> > +{
> > + unsigned int sectors = 0, offset = 0;
> > + struct bio *bio;
> > + sector_t sector;
> > + struct bio_vec *bvec;
> > + unsigned int i = 0, j;
> > + size_t size;
> > + void *buf;
> > +
> > + rq_for_each_bio(bio, req) {
> > + sector = bio->bi_sector;
> > + dev_dbg(&dev->sbd.core,
> > + "%s:%u: bio %u: %u segs %u sectors from %lu\n",
> > + __func__, __LINE__, i, bio_segments(bio),
> > + bio_sectors(bio), sector);
> > + bio_for_each_segment(bvec, bio, j) {
> > + size = bio_cur_sectors(bio)*KERNEL_SECTOR_SIZE;
> > + buf = __bio_kmap_atomic(bio, j, KM_USER0);
> > + if (gather)
> > + memcpy(dev->bounce_buf+offset, buf, size);
> > + else
> > + memcpy(buf, dev->bounce_buf+offset, size);
> > + offset += size;
> > + __bio_kunmap_atomic(bio, KM_USER0);
> > + }
> > + sectors += bio_sectors(bio);
> > + i++;
> > + }
> > +}
>
> So the hypervison uses guest-real addresses here? I would have expected
> it to use the kernel page tables, which lets you use vmap() to do
> scatter-gather.
Yes, it uses logical partion addresses, so we cannot create a virtually
contiguous mapping.
> > +static int ps3disk_thread(void *data)
> > +{
> > + struct ps3_storage_device *dev = data;
> > + struct ps3disk_private *priv = ps3disk_priv(dev);
> > + request_queue_t *q = priv->queue;
> > + struct request *req;
> > +
> > + dev_dbg(&dev->sbd.core, "%s thread init\n", __func__);
> > +
> > + current->flags |= PF_NOFREEZE;
> > +
> > + while (!kthread_should_stop()) {
> > + spin_lock_irq(&priv->lock);
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + req = elv_next_request(q);
> > + if (!req) {
> > + spin_unlock_irq(&priv->lock);
> > + schedule();
> > + continue;
> > + }
> > + if (!blk_fs_request(req)) {
> > + blk_dump_rq_flags(req, DEVICE_NAME " bad request");
> > + end_request(req, 0);
> > + spin_unlock_irq(&priv->lock);
> > + continue;
> > + }
> > + spin_unlock_irq(&priv->lock);
> > + ps3disk_handle_request_sg(dev, req);
> > + }
> > +
> > + dev_dbg(&dev->sbd.core, "%s thread exit\n", __func__);
> > + return 0;
> > +}
>
> I don't really understand what the kthread is needed for. You probably
> thought about multiple options and ended up with this, but having
> a comment in front of it might be helpful.
I used a kthread because the request function of a block device driver must be
non-blocking, and ps3stor_read_write_sectors() calls wait_for_completion().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium
^ permalink raw reply
* Re: [patch 6/7] ps3: ROM Storage Driver
From: Geert Uytterhoeven @ 2007-05-25 19:36 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <200705251850.30342.arnd@arndb.de>
On Fri, 25 May 2007, Arnd Bergmann wrote:
> On Friday 25 May 2007, Geert.Uytterhoeven@sonycom.com wrote:
> > Add a CD/DVD/BD Storage Driver for the PS3:
> > - Implemented as a SCSI device driver
>
> I assume you tried implementing it as a block device driver,
> like you PS3 disk driver does, and failed for some reason.
>
> What is the problem? Is there infrastructure missing in the
> CD-ROM layer?
As the CD/DVD/BD part just accepts SCSI/ATAPI commands (except for plain
read/write), I was suggested to keep it as a SCSI driver.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium
^ permalink raw reply
* Re: [RFC 3/3] Re: [PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE
From: Domen Puncer @ 2007-05-25 18:00 UTC (permalink / raw)
To: David Brownell; +Cc: Dragos Carp, linuxppc-embedded
In-Reply-To: <200705250934.47045.david-b@pacbell.net>
On 25/05/07 09:34 -0700, David Brownell wrote:
> On Friday 25 May 2007, Domen Puncer wrote:
>
> > For clk.h, it does seem quite some code, compared what's there currently.
>
> You're using it wrong then ...
I meant this + the clock.c part.
>
>
> > --- work-powerpc.git.orig/drivers/spi/mpc52xx_psc_spi.c
> > +++ work-powerpc.git/drivers/spi/mpc52xx_psc_spi.c
> > @@ -18,6 +18,7 @@
> >
> > #if defined(CONFIG_PPC_MERGE)
> > #include <asm/of_platform.h>
> > +#include <linux/clk.h>
> > #else
> > #include <linux/platform_device.h>
> > #endif
> > @@ -106,13 +107,52 @@ static void mpc52xx_psc_spi_activate_cs(
> > /* Set clock frequency and bits per word
> > * Because psc->ccr is defined as 16bit register instead of 32bit
> > * just set the lower byte of BitClkDiv
> > + * Because BitClkDiv is 8-bit on mpc5200. Lets stay compatible.
> > */
> > +#if defined(CONFIG_PPC_MERGE)
> > ccr = in_be16(&psc->ccr);
> > ccr &= 0xFF00;
> > + {
> > + u8 bitclkdiv = 2; /* minimum bitclkdiv */
> > + int speed = cs->speed_hz ? cs->speed_hz : 1000000;
> > + char clk_name[10];
> > + struct clk *clk; // TODO into mps, clk_get, clk_put
> > + int mclk;
> > + int system;
> > + /*
> > + * pscclk = mclk/(bitclkdiv+1)) bitclkdiv is 8-bit, >= 2
> > + * mclk = fsys/(mclkdiv+1) mclkdiv is 9-bit, >= 1
> > + * as mclkdiv has higher precision, we want is as big as possible
> > + * => we get < 0.002*clock error
> > + */
> > +
> > + snprintf(clk_name, 10, "psc%i_mclk", spi->master->bus_num);
> > +
> > + clk = clk_get(&spi->dev, clk_name);
>
> Don't expect spi->dev to have a clock. Instead, the host controller
> has a clock, which would be looked up the probe() routine, then released
> in remove(): clock = clk_get(dev, "clockname"), with the clock framework
> using "dev" to identify which clock it returns. (That is, the function
> clock for all SPI controllers would have the same name ... not for
> example "spi1_fclk", "spi2_fclk", etc. but instead "spi_fclk".)
Aha, so that's what struct dev* is for. :-)
Now I see two options:
- create something like at91_clock_associate(), and call it from
_probe (of_devices are created automatically from device tree,
so we can't call it the way at91_clock_associate is).
- in clk_get() call dev_get_drvdata(dev), cast it to spi_master *,
_hope_ it really is that, use bus_id.
Sure there must be a third, right one?
Domen
>
>
> > + if (!clk)
> > + dev_err(&spi->dev, "couldn't get %s clock\n", clk_name);
> > +
> > + system = clk_get_rate(clk_get_parent(clk)); // TODO, checking, refcounting
> > + mclk = speed * (bitclkdiv+1);
> > + if (system/mclk > 512) { /* bigger than mclkdiv */
> > + bitclkdiv = system/512/speed;
> > + mclk = speed * (bitclkdiv+1);
> > + }
> > +
> > + if (clk_set_rate(clk, mclk))
> > + dev_err(&spi->dev, "couldn't set %s's rate\n", clk_name);
> > +
> > + dev_info(&spi->dev, "clock: wanted: %i, got: %li\n", speed,
> > + clk_round_rate(clk, mclk) / (bitclkdiv+1));
> > +
> > + ccr |= bitclkdiv;
> > + }
> > +#else
> > if (cs->speed_hz)
> > ccr |= (MCLK / cs->speed_hz - 1) & 0xFF;
> > else /* by default SPI Clk 1MHz */
> > ccr |= (MCLK / 1000000 - 1) & 0xFF;
> > +#endif
> > out_be16(&psc->ccr, ccr);
> > mps->bits_per_word = cs->bits_per_word;
> >
> \
^ permalink raw reply
* Re: [patch 4/7] ps3: Storage Driver Probing
From: Geoff Levand @ 2007-05-25 17:09 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Geert.Uytterhoeven, linuxppc-dev, linux-kernel
In-Reply-To: <200705251818.03964.arnd@arndb.de>
Hi.
Arnd Bergmann wrote:
>> + switch (dev_type) {
>> + case PS3_DEV_TYPE_STOR_DISK:
>> + match_id = PS3_MATCH_ID_STOR_DISK;
>> + break;
>> +
>> + case PS3_DEV_TYPE_STOR_ROM:
>> + match_id = PS3_MATCH_ID_STOR_ROM;
>> + break;
>> +
>> + case PS3_DEV_TYPE_STOR_FLASH:
>> + match_id = PS3_MATCH_ID_STOR_FLASH;
>> + break;
>> +
>> + default:
>> + return 0;
>> + }
>
> Why do you have separate constants for PS3_DEV_TYPE_* and
> PS3_MATCH_ID_*? If you don't do any conversion, this driver
> will immediately work for additional types as well, if more
> get added later.
I noticed we have some redundancy in the constants and such
now that we have unified the device support. I planned to go
through and try to clean up what I can.
-Geoff
^ permalink raw reply
* Re: [RFC 1/3] Re: [PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE
From: Grant Likely @ 2007-05-25 17:02 UTC (permalink / raw)
To: Sylvain Munaut; +Cc: Dragos Carp, linuxppc-embedded, Domen Puncer, domen.puncer
In-Reply-To: <4656F7D2.8050603@246tNt.com>
On 5/25/07, Sylvain Munaut <tnt@246tnt.com> wrote:
> NACK
>
> Not at all what I meant ...
>
> We should not modify port config in any automatic way, just let the boot
> loader set it up.
> If it known not to do it propertly, then in the platform file
> (lite5200b, efika, ...)
> adjust port config according to what's really on the board.
I second that NACK. Drivers must *never* touch port config.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Advice needed
From: Charles Krinke @ 2007-05-25 16:56 UTC (permalink / raw)
To: linuxppc-embedded; +Cc: Randy Brown, Chris Carlson, Kevin Smith
In-Reply-To: <10805157.post@talk.nabble.com>
I'm having a little trouble with memory mapped registers in both a NAND
chip and a CPLD on an 8541 design where the kernel is based on
linux-2.6.17.11 and mpc85xx_cds_common.
The NAND and CPLD are obviously not part of the memory map of the
mpc85xx_cds_common, and in our case are at local memory addresses
NAND=3D0xFF008000, 32K and CPLD=3D0xFF000000, 32K. Both need to be =
mapped
into memory in Linux.
I am looking at arch/ppc/platforms/85xx/mpc85xx_cds_common.c around
platform_init.
I wonder if a Freescale person could give me an opinion of where the
appropriate place is to call settlbcam to allow access to these two 32K
memory regions?
Charles Krinke
^ permalink raw reply
* Re: [RFC 3/3] Re: [PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE
From: David Brownell @ 2007-05-25 16:34 UTC (permalink / raw)
To: Domen Puncer; +Cc: Dragos Carp, linuxppc-embedded
In-Reply-To: <20070525084750.GC23149@nd47.coderock.org>
On Friday 25 May 2007, Domen Puncer wrote:
> For clk.h, it does seem quite some code, compared what's there currently.
You're using it wrong then ...
> --- work-powerpc.git.orig/drivers/spi/mpc52xx_psc_spi.c
> +++ work-powerpc.git/drivers/spi/mpc52xx_psc_spi.c
> @@ -18,6 +18,7 @@
>
> #if defined(CONFIG_PPC_MERGE)
> #include <asm/of_platform.h>
> +#include <linux/clk.h>
> #else
> #include <linux/platform_device.h>
> #endif
> @@ -106,13 +107,52 @@ static void mpc52xx_psc_spi_activate_cs(
> /* Set clock frequency and bits per word
> * Because psc->ccr is defined as 16bit register instead of 32bit
> * just set the lower byte of BitClkDiv
> + * Because BitClkDiv is 8-bit on mpc5200. Lets stay compatible.
> */
> +#if defined(CONFIG_PPC_MERGE)
> ccr = in_be16(&psc->ccr);
> ccr &= 0xFF00;
> + {
> + u8 bitclkdiv = 2; /* minimum bitclkdiv */
> + int speed = cs->speed_hz ? cs->speed_hz : 1000000;
> + char clk_name[10];
> + struct clk *clk; // TODO into mps, clk_get, clk_put
> + int mclk;
> + int system;
> + /*
> + * pscclk = mclk/(bitclkdiv+1)) bitclkdiv is 8-bit, >= 2
> + * mclk = fsys/(mclkdiv+1) mclkdiv is 9-bit, >= 1
> + * as mclkdiv has higher precision, we want is as big as possible
> + * => we get < 0.002*clock error
> + */
> +
> + snprintf(clk_name, 10, "psc%i_mclk", spi->master->bus_num);
> +
> + clk = clk_get(&spi->dev, clk_name);
Don't expect spi->dev to have a clock. Instead, the host controller
has a clock, which would be looked up the probe() routine, then released
in remove(): clock = clk_get(dev, "clockname"), with the clock framework
using "dev" to identify which clock it returns. (That is, the function
clock for all SPI controllers would have the same name ... not for
example "spi1_fclk", "spi2_fclk", etc. but instead "spi_fclk".)
> + if (!clk)
> + dev_err(&spi->dev, "couldn't get %s clock\n", clk_name);
> +
> + system = clk_get_rate(clk_get_parent(clk)); // TODO, checking, refcounting
> + mclk = speed * (bitclkdiv+1);
> + if (system/mclk > 512) { /* bigger than mclkdiv */
> + bitclkdiv = system/512/speed;
> + mclk = speed * (bitclkdiv+1);
> + }
> +
> + if (clk_set_rate(clk, mclk))
> + dev_err(&spi->dev, "couldn't set %s's rate\n", clk_name);
> +
> + dev_info(&spi->dev, "clock: wanted: %i, got: %li\n", speed,
> + clk_round_rate(clk, mclk) / (bitclkdiv+1));
> +
> + ccr |= bitclkdiv;
> + }
> +#else
> if (cs->speed_hz)
> ccr |= (MCLK / cs->speed_hz - 1) & 0xFF;
> else /* by default SPI Clk 1MHz */
> ccr |= (MCLK / 1000000 - 1) & 0xFF;
> +#endif
> out_be16(&psc->ccr, ccr);
> mps->bits_per_word = cs->bits_per_word;
>
\
^ permalink raw reply
* Re: [patch 6/7] ps3: ROM Storage Driver
From: Arnd Bergmann @ 2007-05-25 16:50 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Geert.Uytterhoeven, linux-kernel
In-Reply-To: <20070525083632.677742000@sonycom.com>
On Friday 25 May 2007, Geert.Uytterhoeven@sonycom.com wrote:
> Add a CD/DVD/BD Storage Driver for the PS3:
> - Implemented as a SCSI device driver
I assume you tried implementing it as a block device driver,
like you PS3 disk driver does, and failed for some reason.
What is the problem? Is there infrastructure missing in the
CD-ROM layer?
Arnd <><
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox