Hi Nick,
Am 10.08.2020 um 21:34 schrieb Nick Elsmore nicholaselsmore@gmail.com:
There are some pretty serious issues regarding the current hardware strategy of the txs02612 mmc mux and compatibility with the Linux mmc stack. I would first like to propose disabling its functionality with the following patch and supporting just the eMMC until a viable method of handling it can be found. The reasoning is as follows.
The mmc subsystem does not directly support multiplexing like other subsystems do (ie i2c).
Yes, there is no mux driver available yet. It has to be written.
The current strategy involves intercepting mmc request operations to the cards on the downstream side of the mux and switching the mux before calling the parent host's request op.
Ideally the tx02612 device node would be a subnode of the root mmc host node.
Yes, that is how the hardware is (and must be).
However, the mmc host driver does not directly probe child nodes without first performing a handshake on the bus. Therefore the subdriver would only be probed if a valid device was connected in the default state of the mux at the time the root host is probed. Therefore, in order to guarantee the txs02612 driver is probed, the bootloader and kernel would need to ensure that a valid device is connected on the SDIO lines to ensure the mux driver came up. This should work as the tx02612 driver would be registered as a card on the root host and the tx02612 driver would allocate and register two new hosts for the eMMC and uSD which would properly go through the SDIO/SD/MMC initialization procedures when they are registered.
Yes, this is the idea.
There also may be an issue if the default card used for detection goes through the full probe procedure and is registered on the root host AND the new host. However, since the driver even being probed in the first place relies on the default state of the GPIO set by the bootloader, this implementation seems shaky to me.
Maybe there must be a new flag to prevent the code to go this way. I.e. if the mmc2 host is flagged as "mux-host-port" it should not do all the steps mentioned above but initialize the mux driver. And the mux driver must ensure that both ports being probed.
Another option is to register the tx02612 driver as a platform driver and link in the device_node in the dts in a method similar to the one below. This way we can be sure the driver is always probed, regardless of the mmc handshake.
txs02612 { compatible = "ti,txs02612"; select-gpio = <&gpio3 18 0>; /* gpio3_82 soft-select */ host = <&mmc2>; status = "disabled"; };
Well, this is not the design patterns DT maintainers want to see. I had similar discussions with them e.g. for SGX or GPS. For GPS on the GTA04 we simply had a driver that referenced the UART where the GPS chip was connected to. Was working fine for 5 years - outside of mainline. But DTS maintainers demanded that the idea is that the processor is sitting like a spider in the net (or like an octopus)and going outwards is represented by child nodes. I.e. each interface is a "bus". Even an UART is a "bus", although you can only connect one client. And if a device is indirectly connected through some subsystem, the subsystem has to be a child node of the processor and the device a child of the subsystem.
Therefore I am quite sure that your proposal will never be accepted mainline (but we should ask Rob Herring).
Generally I have seen the tendency that drivers for new devices are first discussed from DTS perspective. So the first thing which is needed is a .yaml bindings document, then some DTS. And finally the driver code has to be adapted to make use of what it gets. It is no longer possible to fit the DTS to a quick and simple driver implementation.
With this sort of link, the mmc device_node can be grabbed using of_parse_phandle. However, getting the device and then mmc_host from this device_node is not easy since the mmc subsystem doesn't currently provide a method of accessing host nodes from their of_node by iterating through the class devices like some other subsystems provide (ie gpio, fpga-mgr). So far, I have not found a viable method to access the linked node as we don't have access to the correct resources within the mmc subsystem. This method also poses the same issue exhibted by the first method, where the card may be registered on both the root host and the newly registered host.
It is possible by adding some list and registration code. There is a similar mechanism for referencing PHY devices.
The current implementation allows for manual switching between the eMMC and uSD from userspace using a sysfs hook. This works, but I worry about its behavior when interfacing with the mmc subsystem.
It is a simple hack to make use of the DTS (as I think it must be).
The subsystem is not aware of the switching and so this method will likely pose issues with regard to filesystem cache sync and similar.
I have a simple switch script that does an eject, then switch and finally partprobe. So it should be "safe" - unless it is switched by accessing the gpio directly. Of course this scheme does not allow to use both channels in parallel like the ideal solution would do.
The host is using polling to handle card detect, and so if the card disappears due to unsynchronized userland switching, it may view this as a card disconnect, even though the card is still powered. Because of this, a number of risks are possible including improper handling of the card itself (potentially causing damage), improper or missed filesystem cache synchronization, and data corruption.
It is not much different than extracting an SD card from an SD reader without card detect switch, while it is mounted.
Fortunately for the Pyra the µSD card can't be extracted while the Pyra is powered (well it may be possible but is very unlikely) and the eMMC isn't swappable at all.
Until I or someone else can find a safe and (hopefully) graceful solution to this issue,
IMHO it is complex and goes deeper into the MMC code. A big task for a small function and device...
I am proposing to simply disable the tx02612 driver and hardcode the mux to the eMMC for now with the following patch.
Hm. Why do you want to disable it completely? For me the switching script works as it should. It may be possible to have it part of the initrd so that a boot choice screen can allow the user to select to boot from eMMC or µSD. If we ever have a better txs02612 driver we can replace this.
BR and thanks, Nikolaus
Signed-off-by: Nick Elsmore <nicholaselsmore@gmail.com mailto:nicholaselsmore@gmail.com>
index d59d9fd4c8af..60822100bcdf 100644 --- a/arch/arm/boot/dts/omap5-letux-cortex15-common.dtsi +++ b/arch/arm/boot/dts/omap5-letux-cortex15-common.dtsi @@ -248,6 +248,7 @@ mmcmux: txs02612@0 {
#adress-cells = <1>; #size-cells = <0>;
status = "disabled";
eMMC: port@0 { reg = <0>;
@@ -268,9 +269,25 @@ / { /* eMMC / uSD-card */ txs02612 { compatible = "ti,txs02612"; select-gpio = <&gpio3 18 0>; /* gpio3_82 soft-select */
};status = "disabled";
};
+/* hardcode txs02612 mmc mux to eMMC input for now */ +&gpio3 {
- txs02612_soft_control {
gpio-hog;
gpios = <18 GPIO_ACTIVE_HIGH>;
output-low;
label = "txs02612-soft-control";
- };
+};
+&mmc3 { /* WiFi-SDIO */
- /* has been predefined for a wl1271 in ompa5-common.dtsi */
+};
&mmc3 { /* WiFi-SDIO */ /* has been predefined for a wl1271 in ompa5-common.dtsi */ }; _______________________________________________ https://projects.goldelico.com/p/gta04-kernel/ Letux-kernel mailing list Letux-kernel@openphoenux.org http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel
Hi Niklaus,
There also may be an issue if the default card used for detection goes
through the full probe procedure and is registered on the root host AND the new host. However, since the driver even being probed in the first place relies on the default state of the GPIO set by the bootloader, this implementation seems shaky to me.
Maybe there must be a new flag to prevent the code to go this way. I.e. if the mmc2 host is flagged as "mux-host-port" it should not do all the steps mentioned above but initialize the mux driver. And the mux driver must ensure that both ports being probed.
The current implementation actually only works safely if there is no device present when the root host is probed. This way the device is not registered as a card on the root host and is only registered on the new host which gets added by the txs02612 driver. IMHO this is dangerous.
Another option is to register the tx02612 driver as a platform driver and link in the device_node in the dts in a method similar to the one below. This way we can be sure the driver is always probed, regardless of the mmc handshake.
txs02612 { compatible = "ti,txs02612"; select-gpio = <&gpio3 18 0>; /* gpio3_82 soft-select */ host = <&mmc2>; status = "disabled"; };
Well, this is not the design patterns DT maintainers want to see. I had similar discussions with them e.g. for SGX or GPS. For GPS on the GTA04 we simply had a driver that referenced the UART where the GPS chip was connected to. Was working fine for 5 years - outside of mainline. But DTS maintainers demanded that the idea is that the processor is sitting like a spider in the net (or like an octopus)and going outwards is represented by child nodes. I.e. each interface is a "bus". Even an UART is a "bus", although you can only connect one client. And if a device is indirectly connected through some subsystem, the subsystem has to be a child node of the processor and the device a child of the subsystem.
Therefore I am quite sure that your proposal will never be accepted mainline (but we should ask Rob Herring).
Generally I have seen the tendency that drivers for new devices are first discussed from DTS perspective. So the first thing which is needed is a .yaml bindings document, then some DTS. And finally the driver code has to be adapted to make use of what it gets. It is no longer possible to fit the DTS to a quick and simple driver implementation.
Definitely agree here. This method is not good, particularly for trying to get submitted into mainline. This was mainly a discussion of a method which may work with the current mmc architecture, but it poses its own issues as you mentioned.
With this sort of link, the mmc device_node can be grabbed using of_parse_phandle. However, getting the device and then mmc_host from this device_node is not easy since the mmc subsystem doesn't currently provide a method of accessing host nodes from their of_node by iterating through the class devices like some other subsystems provide (ie gpio, fpga-mgr). So far, I have not found a viable method to access the linked node as we don't have access to the correct resources within the mmc subsystem. This method also poses the same issue exhibted by the first method, where the card may be registered on both the root host and the newly registered host.
It is possible by adding some list and registration code. There is a similar mechanism for referencing PHY devices.
Yes this would need to be added to the mmc host code. We would need to iterate through the devices registered as mmc_host_class. The pointer to mmc_host_class is not accessible outside of the host subsystem so the API would need to be extended to allow this.
Until I or someone else can find a safe and (hopefully) graceful solution to this issue,
IMHO it is complex and goes deeper into the MMC code. A big task for a small function and device...
Agreed. I think we will have a difficult time getting support here into mainline as it will require some significant changes to the mmc host architecture. Perhaps this could be done simply by adding a new dts property to the standard mmc host node like you suggested, such as "mmc-host-port" or "mmc-host-muxed" it just probes the child node (mux driver) and passes its ops to the mux driver rather than performing card initialization directly. However, mainline maintainers would likely prefer that both mmc cards present on the downstream side of the mux show up as child devices of the root host rather than on new hosts which aren't properly represented in the class tree.
I am proposing to simply disable the tx02612 driver and hardcode the mux to
the eMMC for now with the following patch.
Hm. Why do you want to disable it completely? For me the switching script works as it should. It may be possible to have it part of the initrd so that a boot choice screen can allow the user to select to boot from eMMC or µSD. If we ever have a better txs02612 driver we can replace this.
The issue I am having with it is it makes it particularly difficult to manage the boot process when trying to use a rootfs on the eMMC (or uSD). Basically when booting from either the eMMC or the uSD, the bootloader and the kernel need to align. If we use a script in the initramfs to manage this, it would need to detect the boot device selected by the bootloader (which reflects the boot device selected by the user using the shoulder button on the Pyra). Many gpio drivers do not properly expose pin direction and value from userspace until they are set from userspace when accessed via sysfs. In this case, a script cannot reliably get the state of the pin as set by the bootloader from userspace to determine the correct device to select after initialization of all downstream devices. Granted I have not verified whether or not this works properly on the omap5 driver, I should and will.
Overall, this should be handled by the kernel. I tried to do a small modification to the txs02612 driver to grab the gpio without modifying it using the GPIOD_ASIS flag rather than GPIOD_OUT_HIGH and reading the state as set by U-Boot. After initialization of the new hosts (for eMMC and uSD), I tried setting it back to the value chosen by the bootloader as this reflects the boot device selected by the user. This does not work properly since the initialization procedure of the slots, when mmc_add_host is called, is handled asynchronously so I cannot properly synchronize selection of the correct device after allocation (the request op is also switching the gpio during the mmc_add_host procedure).
I am coming at this from a standpoint of trying to get the Pyra to a release ready state. This involves getting the software to a point where it works in an intuitive manner to an end user. A very common use-case for the Pyra (and the state in which it will ship) is to boot off the eMMC. The current management of the mmc mux makes this difficult to configure and involves synchronization of the boot selection in the bootloader, initramfs, and kernel which is quite complicated and makes modification of this difficult. It seems sensible to me to limit functionality to the eMMC for now until a safe method which is properly managed by the kernel can be merged in.
Another option here is to disable all control of the selection gpio in the kernel and initramfs so the pin remains in the state set by the bootloader. This way, the eMMC/uSD selection is determined at boot time and isn't changed at runtime.
I do recognize that the change proposed modifies the device tree structure in omap5-letux-cortex15-common.dtsi which may impact devices other than the Pyra. If this is the case, it would make sense to move these changes into a Pyra-specific device tree to leave the device tree of other devices using this include unaffected.
_______________________________________________ https://projects.goldelico.com/p/gta04-kernel/ Letux-kernel mailing list Letux-kernel@openphoenux.org http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel
On 8/11/20 8:38 PM, Nick Elsmore wrote:
If we use a script in the initramfs to manage this
I hope you are aware that there is already a script in the initramfs that does some emmc/microsd magic to handle booting from either one. This could get in the way of any experiments you are trying to do.
https://dev.pyra-handheld.com/packages/pyra-config/-/blob/master/initramfs/p...
On Tue, Aug 11, 2020 at 2:59 PM aTc atc@k-n-p.org wrote:
On 8/11/20 8:38 PM, Nick Elsmore wrote:
If we use a script in the initramfs to manage this
I hope you are aware that there is already a script in the initramfs that does some emmc/microsd magic to handle booting from either one. This could get in the way of any experiments you are trying to do.
https://dev.pyra-handheld.com/packages/pyra-config/-/blob/master/initramfs/p...
Yep I'm aware of this script. Thanks for pointing it out with the link, though. The issue with this script detecting the mux state as it does is that the driver will not reflect the device selected by the bootloader. When the txs02612 driver is probed, it will first add the mmc host for the eMMC and then the uSD. When these hosts are added, their request op functions get called which switches the mux state. The state of the mux will be left with whichever is requested last. It is usually the uSD since it is added last, but since the detect function is called asynchronously there is no guarantee of this.
I can see that the script is detecting whether a mmcblk device has been registered, but the existence of this device does not reflect the state of the mux either. It is possible that the eMMC device was registered and the mux is switched to uSD afterwards, but the mmcblk device still exists as the polling has not occurred yet to trigger its de-enumeration. While the situation is unlikely, unfortunately "unlikely" in this scenario means potential bug.