From owner-freebsd-arm@FreeBSD.ORG Thu Mar 6 04:17:20 2014 Return-Path: Delivered-To: freebsd-arm@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 41C433A5; Thu, 6 Mar 2014 04:17:20 +0000 (UTC) Received: from mail-la0-x236.google.com (mail-la0-x236.google.com [IPv6:2a00:1450:4010:c03::236]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id E6EEA8F9; Thu, 6 Mar 2014 04:17:18 +0000 (UTC) Received: by mail-la0-f54.google.com with SMTP id mc6so1344335lab.41 for ; Wed, 05 Mar 2014 20:17:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=hkOQBHNASLYrEFplC3Gb8JbQ3Yi8Za76kbshZSBEO1o=; b=LDdV6LclMQGNvCJ9cKpREOtZlcI2TvowrsPQAzWx6c6KFpiEe/TwqcZPrdfpxJumMe ML0c9IzknHhRk7BDFM7+ARQnNfdbcpF3oNzo/gXHCnqaSusmUJ0mLxhTLE5GyX+XVIie csguAJOK3gmoUvRLqnb6MvDS1gDQWEROZhO6RhclvCbM3IRxoiRcGc7x+V2cYSoIz3DD D9ffMv8ttSSS5Ku+QpwGkA1Eqa7xAKsEFPnnE+iACCvrUy/PsvNDPFIAE7vze+tMwp4T Ugvqf4/Cu5QgIwux+816a6pqI8+9eN5UWX385Tk4s70gCbqid6e/3/hQ/79FWYVgYPmY ZbPg== MIME-Version: 1.0 X-Received: by 10.152.43.47 with SMTP id t15mr29450lal.38.1394079436185; Wed, 05 Mar 2014 20:17:16 -0800 (PST) Sender: pkelsey@gmail.com Received: by 10.112.142.5 with HTTP; Wed, 5 Mar 2014 20:17:16 -0800 (PST) In-Reply-To: <7F5BD549-6A25-48F5-989A-F2D43C241CFF@bsdimp.com> References: <7C2B7036-51CC-4C97-80C4-0A439874357D@bsdimp.com> <1393939277.1149.300.camel@revolution.hippie.lan> <438620C4-7712-4B01-A46F-CB57946A30BF@bsdimp.com> <16A5203B-B06D-4129-A54F-F34D5FA28D2B@bsdimp.com> <39ED4040-2A6A-4D85-97D5-DCDE4ECCA0EC@bsdimp.com> <899B9ABD-0ACC-49F2-9520-CCE837D39875@bsdimp.com> <9CEFA586-0319-4390-AC9A-B54EE77AD735@ieee.org> <7F5BD549-6A25-48F5-989A-F2D43C241CFF@bsdimp.com> Date: Wed, 5 Mar 2014 23:17:16 -0500 X-Google-Sender-Auth: gpLx7WI8-H7OzRN9jECszCw8tB0 Message-ID: Subject: Re: [PATCH] simplebus child device probe order control via FDT (motivated by BeagleBone Black) From: Patrick Kelsey To: Warner Losh Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.17 Cc: "freebsd-hackers@freebsd.org" , freebsd-arm , "freebsd-embedded@freebsd.org" , Ian Lepore X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "Porting FreeBSD to ARM processors." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Mar 2014 04:17:20 -0000 On Wed, Mar 5, 2014 at 10:52 PM, Warner Losh wrote: > > On Mar 5, 2014, at 7:48 PM, Patrick Kelsey wrote: > > > > > > > On Mar 5, 2014, at 8:02 PM, Warner Losh wrote: > > > >> > >> On Mar 5, 2014, at 5:53 PM, Patrick Kelsey wrote: > >> > >>> > >>> > >>> > >>> On Wed, Mar 5, 2014 at 5:44 PM, Warner Losh wrote: > >>> > >>> On Mar 5, 2014, at 2:56 PM, Patrick Kelsey wrote: > >>> > >>>> > >>>> > >>>> > >>>> On Wed, Mar 5, 2014 at 4:24 PM, Warner Losh wrote: > >>>> > >>>> On Mar 5, 2014, at 11:55 AM, Patrick Kelsey wrote: > >>>> > >>>>> > >>>>> > >>>>> > >>>>> On Wed, Mar 5, 2014 at 8:31 AM, Warner Losh wrote: > >>>>> > >>>>> On Mar 4, 2014, at 11:53 PM, Patrick Kelsey wrote: > >>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>> On Tue, Mar 4, 2014 at 8:21 AM, Ian Lepore wrote: > >>>>>> > >>>>>> There's a standard property for mmc/sd devices, "non-removable" > whose > >>>>>> presence denotes things like soldered-on eMMC parts. Only one of > our > >>>>>> many sdhci drivers supports it right now (imx). In general the core > >>>>>> part of the driver (dev/sdhci) doesn't have good support for > >>>>>> insert/remove/presence detection that's handled off to the side > (whether > >>>>>> it's non-removable or a gpio pin). > >>>>>> > >>>>>> That alone doesn't solve the wider problem, though, which I think > breaks > >>>>>> down into two pieces. Let's say you've got two slots, call them > left > >>>>>> and right. You end up with these two problems... > >>>>>> > >>>>>> * Sometimes the right slot is mmcsd0, but it turns into mmcsd1 > if > >>>>>> there's a card in the left slot when I boot; I don't want it > to > >>>>>> change. > >>>>>> > >>>>>> And not just a boot-time issue, of course. If you were to remove > those two cards and then reinsert them in the opposite time-order, their > device names would swap. > >>>>>> > >>>>>> * I want the slot on the left to be named '1' and the right to > be > >>>>>> '0'. > >>>>>> > >>>>>> The first problem is easily solved without impacting dts in any > way. We > >>>>>> just wire down the relationship controllerN -> mmcN -> mmcsdN. > This is > >>>>>> exactly equivelent to the old ATA_STATIC_ID option in its effect -- > you > >>>>>> don't get to choose the names, but you know they won't change based > on > >>>>>> which devices are present. It could be controlled with a tunable. > >>>>>> > >>>>>> It's harder to envision the fix for the second part without adding > an > >>>>>> ad-hoc property for the devices. Even with a property I'm not sure > how > >>>>>> easy it would be. > >>>>>> > >>>>>> We should be able to assign a geographic address > (controllerX:slotY) to each mmcsd device in a given system (let's ignore > for now the theoretical possibility of multiple cards on one bus). The > 'controllerX' part of the address could be the controller's pathname from > the devicetree, or an index assigned by mmcbr machinery at attach time. > The "slotY" part of the address is assigned by the specific controller > device driver using some internally-determined fixed mapping between the > assigned values and its physical slots. This geographic address could be > used to create an additional set of /dev entries with stable names. There > could be a mechanism for user-configurable aliases for the geographical > addresses. > >>>>> > >>>>> There's already a chance to run a script when a device is attached > that can create /dev/slot0 or /dev/slot1 that has geographical information > available to it. People use ddvd for this in the USB world all the time to > make sure that tty devices get a symlink based on their location or serial > number. > >>>>> > >>>>> Why is mmc so different it needs its own mechanism? > >>>>> > >>>>> I'm laying out my view of the information that would be needed and > the types of actions that would have to be taken based on that information > to solve the issue. I'm not saying devd can't be the piece that is used to > implement the actions (indeed, I noted devd as a potential building block > for a solution in my initial response). I'm also not saying mmc needs its > own mechanism, I'm saying it needs /a/ mechanism, and so far there still > seems to be something missing (because it's not there, or I'm still > ignorant of it). > >>>>> > >>>>> What seems to be missing is geographical addressing for mmc devices. > >>>>> > >>>>> I think what you might be saying is that the existing mmcsd > add/remove code could be augmented to send devctl notifications, along the > lines of: > >>>>> > >>>>> devctl_notify("MMC", "DEVICE", "ATTACH"|"DETACH", "... > controller= > slot= rca=") > >>>>> > >>>>> and then I and the fine author of devctl and devd would be pleased. > >>>> > >>>> MMC doesn't need anything special here. That's already present. > Looking at the device tree we see on one of the platforms that's handy for > me to access: > >>>> > >>>> at91_mci0 > >>>> mmc0 > >>>> mmcsd0 at rca=0xb368 > >>>> > >>>> So you know that mmcsd0 is on mmc0 bus attached to at91_mci0, which > is ultimately the FDT node where things came from. There's not a > user-defined slot associated with this (and we should have a SLOT A vs SLOT > B as a location info for this platform, because we can have two cards on > the one bus in the MMC case), true, but for your use case I don't think > that you need it. We should be attaching the host controller regardless of > whether the or not there's a card in there, so it will be fixed. While some > additional information would be useful to publish, I don't think your use > case requires it... > >>>> > >>>> > >>>> The reason you need something extra here is that what is there now > breaks down whenever you don't have a one-to-one mapping between > controllers and buses. Any controller with more than one slot can yield > something of the form: > >>>> > >>>> sdhci0 > >>>> mmc0 > >>>> mmcsd0 at rca=0xabcd > >>>> mmc1 > >>>> mmcsd1 at rca=0x1234 > >>>> > >>>> and you have no idea what physical slot in the system mmcsd0 and > mmcsd1 are in. > >>> > >>> The driver isn't going to be able to help you, because it reports mmc0 > based on the data it gets from slot 0 status registers, and mmc1 based on > slot 1 status registers. Since there's no notion of how that maps to > physical hardware, the driver can't do anything automatically. And since > mmc on down is populated by FreeSBD, there's no hints in the FDT tree for > them. > >>> > >>> > >>>> For my immediate use case, sure, I can rely on the one-to-one > relationship between controllers and buses. At this point, though, rather > than skate by on that happy coincidence, I'd rather invest what now seems > to be a rather small amount of effort adding mmcsd devctl notifications > that would cover the multiple-slots-per-controller case as well, and then > build the rest of what I want on top of that information coming out of ddvd. > >>> > >>> Trouble is, how do we know what to send with this new notification. > That's the part I'm having trouble with. Where does that data come from? > And how is it different than what's in the device tree? > >>> > >>> > >>> Each controller driver maintains an instance of struct mmc_host for > each physical bus interface (typically referred to as a 'slot' in the > drivers) it has. When a card is detected in a given slot by the driver, > the driver creates an mmc bus instance and attaches the struct mmc_host > corresponding to that slot to provide the ivar values. So let's say struct > mmc_host gets a new member 'slot_number', and a new ivar > MMC_IVAR_SLOT_NUMBER is defined. The slot number in each instance of > struct mmc_host a driver maintains gets set to a controller-relative index > of the corresponding physical interface - the controller will do this the > same way every time, because it is tied to the register layout of the > controller. > >>> > >>> After the mmc bus instance is created and its ivars are set, > probe-and-attach is run on that bus, and an mmcsd device instance is > created for each card that is found. At the point where the mmcsd device > instance is created, one knows the parent bus for that new mmcsd device, > and thus one can get the value of MMC_IVAR_SLOT_NUMBER for that bus, as > well as the name of the controller device instance that is the parent of > the parent bus. It thus possible at that point to > >>> > >>> devctl_notify("MMC", "DEVICE", "ATTACH", "... > controller= slot= > ") > >>> > >>> Because the controller attachment order is the same on every boot, as > is the slot number ivar for a given bus interface on each controller > hardware instance, an identical attach message will be generated every time > a card is discovered in that physical location in the system. For a given > system, there will thus be a fixed mapping between > {controller_instance,slot} tuples that appear in these messages and the > physical MMC/SD device locations. > >> > >> I'm curious how that's materially different than the parent's mmc > instance number. That too is invariant between boots. There's a 1:1 - onto > mapping between this instance number and any controller/slot tuple that > would be created. > >> > > > > Controller instance (unit) numbers are the same across boots. The mmc > instance (unit) number for the mmc instance created for a given bus > interface on a given controller is not, because the instance is created > dynamically in response to card detection and thus depends on the ordering > of card insertions. > > That's the problem right there. The should always be the same from boot to > boot. sdhci must be doing things wrong. I'll have to take a look. That's > the real problem here. That's certainly how it is supposed to be working. > We always attach PCI busses to PCI bridges, regardless of what's on the > bus. mmc should be the same thing. I'll work up some patches for that. > > > Sure, there's a one-to-one and onto mapping at any given moment between > mmc instance numbers and the tuples I'm talking about, but the mapping is > subject to change with card insertions and removals. The material > difference between the two sets of labels is that a given tuple value will > *always* refer to the same physical device location in the system, whereas > a given mmc unit number could refer to any physical device location in the > system depending on the time order of insertions in the various card slots. > > I understand better now... > Yes, this fully explains the disconnect we were having. I was under the impression that sdhci was doing it that way to make way for reprobe/attach/discovery because it's actually doing card detect. It felt a bit weird at the time, but this is the example I followed when I did the mmcspi driver I posted to the list sometime back, as that also had card detect support. > > >> Also, there doesn't need to be a special MMC message for this. If we do > create the notion of a slot number per controller, that would be part of > the location information that is in the location string for the normal > attach message > > > > Ah, so I can just append more variable definitions to the location > string, which is already fed through to the existing generic devctl > notification? Works for me. > > Sure. > > >>> In the above, I've left out the value of rca from the discussion > because upon further reflection, it cannot be stably tied to a physical > location. If there is a multidrop MMC bus in a system, the physical card > locations on that bus will not be able to be referred to with stable names. > This is the one area where a new property in the FDT could be useful to > convey multidrop-or-not for each bus interface on a controller. The new > property could be 'freebsd,max-devices' and would be an array of cells that > indicates the maximum number of MMC/SD devices that can be connected to the > controller bus interface corresponding to that cell index. The devctl > notification could then include a multidrop indicator in the message. > >> > >> rca is more of a serial number than a location number. Useful for other > reasons. > >> > >> I'm not sure how 'freebsd,max-devices' would solve the problem. The > controller already knows that. If we really want to tie things to a > physical location/ description, I'd opt for something more like > 'freebsd,slot-names = "Slot 0", "Slot 1";' type of thing, where you could > just as easily have "Top Card" "bottom card" or "boot card" and "customer > card" if you wanted. Then the controller could query this property to get > the names to populate somewhere in the PNP info for this device. The mmcsd > driver would then be free to also create a /dev/Blah alias as well for the > disk, but I don't know if that would cause aliases to get created for all > the geom children or not... > >> > > > > freebsd,max-devices is not already known by the controller. The > controller knows how many bus interfaces it has, but it doesn't know how > many devices can be attached to that bus, as that depends on the board > design. freebsd,max-devices informs us whether the board design is > multidrop or not for each bus interface on each controller. Passing > through a multidrop indication in the devctl notification informs the > devctl consumer as to whether or not a unique stable name can be assigned > to the mmcsd instance based on the tuple (if multidrop, then no). Not > essential, but would be thorough. > > Hmmm, this sure sounds like something that should already be in the FDT > file. I'll have to do a survey. > > > I disagree with 'freebsd,slot-names' because meaningful/descriptive slot > names are going to be something that are often defined at the product > level, so I think it's better to just let them be defined via a devd action > script. Otherwise we build in an invitation to the experience of having > board-level slot names and product-level slot names from the same > namespace, which is in my experience awful. "Oh, wait, does this bug > report refer to board-'top slot' or front-panel-'top slot'?" In other > words, I think it's handy to have up until the board goes into an > enclosure, then it could be trouble from then on. Plus it could also > encourage further knee-jerk, inappropriate .dts patching of the sort I > started out with here :) "Why make a devd script when I can just edit the > names in the .dts file?" > > Good points. > > > Agreed geom children are an open question. > > > > -Patrick > > Warner > >