[PATCH] simplebus child device probe order control via FDT (motivated by BeagleBone Black)

[Patrick Kelsey]

On Mar 5, 2014, at 8:02 PM, Warner Losh <imp at bsdimp.com> wrote:

> 
> On Mar 5, 2014, at 5:53 PM, Patrick Kelsey <kelsey at ieee.org> wrote:
> 
>> 
>> 
>> 
>> On Wed, Mar 5, 2014 at 5:44 PM, Warner Losh <imp at bsdimp.com> wrote:
>> 
>> On Mar 5, 2014, at 2:56 PM, Patrick Kelsey <kelsey at ieee.org> wrote:
>> 
>>> 
>>> 
>>> 
>>> On Wed, Mar 5, 2014 at 4:24 PM, Warner Losh <imp at bsdimp.com> wrote:
>>> 
>>> On Mar 5, 2014, at 11:55 AM, Patrick Kelsey <kelsey at ieee.org> wrote:
>>> 
>>>> 
>>>> 
>>>> 
>>>> On Wed, Mar 5, 2014 at 8:31 AM, Warner Losh <imp at bsdimp.com> wrote:
>>>> 
>>>> On Mar 4, 2014, at 11:53 PM, Patrick Kelsey <kelsey at ieee.org> wrote:
>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> On Tue, Mar 4, 2014 at 8:21 AM, Ian Lepore <ian at freebsd.org> 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=<controller_device_name_or_better_yet_devicetree_path> slot=<slot_number> rca=<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=<controller_instance_name> slot=<value-of-MMC_IVAR_SLOT_NUMBER> ")
>> 
>> 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.  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.

> 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.

>> 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.

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?"

Agreed geom children are an open question.

-Patrick