Date: Wed, 5 Mar 2014 20:52:57 -0700 From: Warner Losh <imp@bsdimp.com> To: Patrick Kelsey <kelsey@ieee.org> Cc: "freebsd-hackers@freebsd.org" <freebsd-hackers@freebsd.org>, freebsd-arm <freebsd-arm@freebsd.org>, "freebsd-embedded@freebsd.org" <freebsd-embedded@freebsd.org>, Ian Lepore <ian@freebsd.org> Subject: Re: [PATCH] simplebus child device probe order control via FDT (motivated by BeagleBone Black) Message-ID: <7F5BD549-6A25-48F5-989A-F2D43C241CFF@bsdimp.com> In-Reply-To: <9CEFA586-0319-4390-AC9A-B54EE77AD735@ieee.org> References: <CAD44qMUyqzaFtjgXdgThgHcHjPctx-oZAdhvHp4Kf0G7N4HVog@mail.gmail.com> <7C2B7036-51CC-4C97-80C4-0A439874357D@bsdimp.com> <CAD44qMXe8bh0cR60tm8%2BLZ9W3WhJDuGX6xz9FLNHrzmXNd5FDQ@mail.gmail.com> <1393939277.1149.300.camel@revolution.hippie.lan> <CAD44qMWLox0yY1-%2B2bn4dQ=z0jWKow95b=mnCJEkwmSiSipf4g@mail.gmail.com> <438620C4-7712-4B01-A46F-CB57946A30BF@bsdimp.com> <CAD44qMWNGPY70NMkTWoWcMTf1pywhSPGdbJpmbxxSxEdENSOJQ@mail.gmail.com> <16A5203B-B06D-4129-A54F-F34D5FA28D2B@bsdimp.com> <CAD44qMU46nqsw155qqKs7DrxXqawURgaUVEAaetzDExYM2LhYg@mail.gmail.com> <39ED4040-2A6A-4D85-97D5-DCDE4ECCA0EC@bsdimp.com> <CAD44qMU6ksf7Z_QmX853=bRdEQQu-oxTkx9-uAbms0F6E%2Bahgg@mail.gmail.com> <899B9ABD-0ACC-49F2-9520-CCE837D39875@bsdimp.com> <9CEFA586-0319-4390-AC9A-B54EE77AD735@ieee.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mar 5, 2014, at 7:48 PM, Patrick Kelsey <kelsey@ieee.org> wrote:
>=20
>=20
> On Mar 5, 2014, at 8:02 PM, Warner Losh <imp@bsdimp.com> wrote:
>=20
>>=20
>> On Mar 5, 2014, at 5:53 PM, Patrick Kelsey <kelsey@ieee.org> wrote:
>>=20
>>>=20
>>>=20
>>>=20
>>> On Wed, Mar 5, 2014 at 5:44 PM, Warner Losh <imp@bsdimp.com> wrote:
>>>=20
>>> On Mar 5, 2014, at 2:56 PM, Patrick Kelsey <kelsey@ieee.org> wrote:
>>>=20
>>>>=20
>>>>=20
>>>>=20
>>>> On Wed, Mar 5, 2014 at 4:24 PM, Warner Losh <imp@bsdimp.com> wrote:
>>>>=20
>>>> On Mar 5, 2014, at 11:55 AM, Patrick Kelsey <kelsey@ieee.org> =
wrote:
>>>>=20
>>>>>=20
>>>>>=20
>>>>>=20
>>>>> On Wed, Mar 5, 2014 at 8:31 AM, Warner Losh <imp@bsdimp.com> =
wrote:
>>>>>=20
>>>>> On Mar 4, 2014, at 11:53 PM, Patrick Kelsey <kelsey@ieee.org> =
wrote:
>>>>>=20
>>>>>>=20
>>>>>>=20
>>>>>>=20
>>>>>> On Tue, Mar 4, 2014 at 8:21 AM, Ian Lepore <ian@freebsd.org> =
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<controller_device_name_or_better_yet_devicetree_path> =
slot=3D<slot_number> rca=3D<rca>")
>>>>>=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<controller_instance_name> =
slot=3D<value-of-MMC_IVAR_SLOT_NUMBER> ")
>>>=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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7F5BD549-6A25-48F5-989A-F2D43C241CFF>