From owner-freebsd-embedded@FreeBSD.ORG Thu Mar 6 03:53:08 2014 Return-Path: Delivered-To: freebsd-embedded@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 53A9FE24 for ; Thu, 6 Mar 2014 03:53:08 +0000 (UTC) Received: from mail-pd0-f173.google.com (mail-pd0-f173.google.com [209.85.192.173]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 18473771 for ; Thu, 6 Mar 2014 03:53:07 +0000 (UTC) Received: by mail-pd0-f173.google.com with SMTP id z10so1960320pdj.32 for ; Wed, 05 Mar 2014 19:53:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:content-type:mime-version:subject:from :in-reply-to:date:cc:content-transfer-encoding:message-id:references :to; bh=tiIANlcRjuRuB95hHe5CQhbKfLDg1t4mKn9lwSaGQKU=; b=OACE/solOrOLS+hO6azJ1bZgwCjaBN6ca6Cx+9nJ2R3qd7HjUIOA7fIuS0E54JMUMi 2+Lmjx9eh7s6fv6aajblAdZzP+BRxxIS8NY1SmYUkPnL3Xe+1N6A4JHJawqOu/ID2uKj zfnI9nQ2O8v+WFL8dcINz5NeZF81dVn3L5L4sfQSc/QxC/IOJ+XeNNcDqydaOGQtAtwF TK2+uJ1zzYKfdv5gumnLOQtd0fJysG5YuYa7HduKwjEtdXVl/6BB1Tu1Kl/YbWP1BjWs O0NZWDOe1zvpmuQczdNZ5jRHEqBugPf5ZhlFBL0fsujRUWfxWXt5SNI5aOrNcNg1ZH54 s2Xw== X-Gm-Message-State: ALoCoQkJ/Gn42Z7QRRzqdCEvjyhv+m8Semr1J8HIFiY+21yhuqIWSr+XuQEUMa35JOPVly7HgwmQ X-Received: by 10.66.141.165 with SMTP id rp5mr11713909pab.90.1394077981033; Wed, 05 Mar 2014 19:53:01 -0800 (PST) Received: from lglt-rottaway.corp.netflix.com (dc1-prod.netflix.com. [69.53.236.251]) by mx.google.com with ESMTPSA id yo9sm27484830pab.16.2014.03.05.19.52.59 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 05 Mar 2014 19:53:00 -0800 (PST) Sender: Warner Losh Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 7.2 \(1874\)) Subject: Re: [PATCH] simplebus child device probe order control via FDT (motivated by BeagleBone Black) From: Warner Losh In-Reply-To: <9CEFA586-0319-4390-AC9A-B54EE77AD735@ieee.org> Date: Wed, 5 Mar 2014 20:52:57 -0700 Content-Transfer-Encoding: quoted-printable Message-Id: <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> To: Patrick Kelsey X-Mailer: Apple Mail (2.1874) Cc: "freebsd-hackers@freebsd.org" , freebsd-arm , "freebsd-embedded@freebsd.org" , Ian Lepore X-BeenThere: freebsd-embedded@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Dedicated and Embedded Systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Mar 2014 03:53:08 -0000 On Mar 5, 2014, at 7:48 PM, Patrick Kelsey wrote: >=20 >=20 > On Mar 5, 2014, at 8:02 PM, Warner Losh wrote: >=20 >>=20 >> On Mar 5, 2014, at 5:53 PM, Patrick Kelsey wrote: >>=20 >>>=20 >>>=20 >>>=20 >>> On Wed, Mar 5, 2014 at 5:44 PM, Warner Losh wrote: >>>=20 >>> On Mar 5, 2014, at 2:56 PM, Patrick Kelsey wrote: >>>=20 >>>>=20 >>>>=20 >>>>=20 >>>> On Wed, Mar 5, 2014 at 4:24 PM, Warner Losh wrote: >>>>=20 >>>> On Mar 5, 2014, at 11:55 AM, Patrick Kelsey = wrote: >>>>=20 >>>>>=20 >>>>>=20 >>>>>=20 >>>>> On Wed, Mar 5, 2014 at 8:31 AM, Warner Losh = wrote: >>>>>=20 >>>>> On Mar 4, 2014, at 11:53 PM, Patrick Kelsey = wrote: >>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>> On Tue, Mar 4, 2014 at 8:21 AM, Ian Lepore = wrote: >>>>>>=20 >>>>>> 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). >>>>>>=20 >>>>>> 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... >>>>>>=20 >>>>>> * 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. >>>>>>=20 >>>>>> 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. >>>>>>=20 >>>>>> * I want the slot on the left to be named '1' and the right = to be >>>>>> '0'. >>>>>>=20 >>>>>> 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. >>>>>>=20 >>>>>> 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. >>>>>>=20 >>>>>> 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. >>>>>=20 >>>>> There=92s 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. >>>>>=20 >>>>> Why is mmc so different it needs its own mechanism? >>>>>=20 >>>>> 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). >>>>>=20 >>>>> What seems to be missing is geographical addressing for mmc = devices. >>>>>=20 >>>>> 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: >>>>>=20 >>>>> devctl_notify("MMC", "DEVICE", "ATTACH"|"DETACH", "... = controller=3D = slot=3D rca=3D") >>>>>=20 >>>>> and then I and the fine author of devctl and devd would be = pleased. >>>>=20 >>>> MMC doesn=92t need anything special here. That=92s already present. = Looking at the device tree we see on one of the platforms that=92s handy = for me to access: >>>>=20 >>>> at91_mci0 >>>> mmc0 >>>> mmcsd0 at rca=3D0xb368 >>>>=20 >>>> So you know that mmcsd0 is on mmc0 bus attached to at91_mci0, which = is ultimately the FDT node where things came from. There=92s 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=92t think that you need it. We should be attaching the host = controller regardless of whether the or not there=92s a card in there, = so it will be fixed. While some additional information would be useful = to publish, I don=92t think your use case requires it=85 >>>>=20 >>>>=20 >>>> 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: >>>>=20 >>>> sdhci0 >>>> mmc0 >>>> mmcsd0 at rca=3D0xabcd >>>> mmc1 >>>> mmcsd1 at rca=3D0x1234 >>>>=20 >>>> and you have no idea what physical slot in the system mmcsd0 and = mmcsd1 are in. >>>=20 >>> The driver isn=92t 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=92s no notion of how that = maps to physical hardware, the driver can=92t do anything automatically. = And since mmc on down is populated by FreeSBD, there=92s no hints in the = FDT tree for them. >>>=20 >>>=20 >>>> 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. >>>=20 >>> Trouble is, how do we know what to send with this new notification. = That=92s the part I=92m having trouble with. Where does that data come = from? And how is it different than what=92s in the device tree? >>>=20 >>>=20 >>> 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. >>>=20 >>> 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=20 >>>=20 >>> devctl_notify("MMC", "DEVICE", "ATTACH", "... = controller=3D = slot=3D ") >>>=20 >>> 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. >>=20 >> I=92m curious how that=92s materially different than the parent=92s = mmc instance number. That too is invariant between boots. There=92s a = 1:1 - onto mapping between this instance number and any controller/slot = tuple that would be created. >>=20 >=20 > 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=92s the problem right there. The should always be the same from = boot to boot. sdhci must be doing things wrong. I=92ll have to take a = look. That=92s the real problem here. That=92s certainly how it is = supposed to be working. We always attach PCI busses to PCI bridges, = regardless of what=92s on the bus. mmc should be the same thing. I=92ll = 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... >> Also, there doesn=92t 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 >=20 > 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. >>=20 >> rca is more of a serial number than a location number. Useful for = other reasons. >>=20 >> I=92m not sure how =91freebsd,max-devices=92 would solve the problem. = The controller already knows that. If we really want to tie things to a = physical location/ description, I=92d opt for something more like = =91freebsd,slot-names =3D =93Slot 0=94, =93Slot 1=94;=92 type of thing, = where you could just as easily have =93Top Card=94 =93bottom card=94 or = =93boot card=94 and =93customer card=94 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=92t know if that would cause aliases to get created for all the geom = children or not... >>=20 >=20 > 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=92ll 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?=94 Good points. > Agreed geom children are an open question. >=20 > -Patrick Warner