Date: Wed, 5 Mar 2014 23:17:16 -0500 From: Patrick Kelsey <kelsey@ieee.org> To: Warner Losh <imp@bsdimp.com> 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: <CAD44qMVBAbT3u4-PtDCQY%2BdLKYmoBDkS-QvZmK9k2NVYL8W9Fw@mail.gmail.com> In-Reply-To: <7F5BD549-6A25-48F5-989A-F2D43C241CFF@bsdimp.com> 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> <7F5BD549-6A25-48F5-989A-F2D43C241CFF@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Mar 5, 2014 at 10:52 PM, Warner Losh <imp@bsdimp.com> wrote:
>
> On Mar 5, 2014, at 7:48 PM, Patrick Kelsey <kelsey@ieee.org> wrote:
>
> >
> >
> > On Mar 5, 2014, at 8:02 PM, Warner Losh <imp@bsdimp.com> wrote:
> >
> >>
> >> On Mar 5, 2014, at 5:53 PM, Patrick Kelsey <kelsey@ieee.org> wrote:
> >>
> >>>
> >>>
> >>>
> >>> On Wed, Mar 5, 2014 at 5:44 PM, Warner Losh <imp@bsdimp.com> wrote:
> >>>
> >>> On Mar 5, 2014, at 2:56 PM, Patrick Kelsey <kelsey@ieee.org> wrote:
> >>>
> >>>>
> >>>>
> >>>>
> >>>> On Wed, Mar 5, 2014 at 4:24 PM, Warner Losh <imp@bsdimp.com> wrote:
> >>>>
> >>>> On Mar 5, 2014, at 11:55 AM, Patrick Kelsey <kelsey@ieee.org> wrote:
> >>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Wed, Mar 5, 2014 at 8:31 AM, Warner Losh <imp@bsdimp.com> wrote:
> >>>>>
> >>>>> On Mar 4, 2014, at 11:53 PM, Patrick Kelsey <kelsey@ieee.org> wrote:
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On Tue, Mar 4, 2014 at 8:21 AM, Ian Lepore <ian@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.
>
> 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
>
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAD44qMVBAbT3u4-PtDCQY%2BdLKYmoBDkS-QvZmK9k2NVYL8W9Fw>