[PW_SID:1089787] Split Generic PHY consumer and provider API#1870
Closed
linux-riscv-bot wants to merge 31 commits into
Closed
[PW_SID:1089787] Split Generic PHY consumer and provider API#1870linux-riscv-bot wants to merge 31 commits into
linux-riscv-bot wants to merge 31 commits into
Conversation
The blamed commit functionally changed the error path of cdns_pcie_host_probe(), now cdns_plat_pcie_probe(). When the old code path executed "goto err_get_sync", the PCIe controller probe function propagated the pm_runtime_get_sync() error code. The new code doesn't, and returns 0. Similarly for the "goto err_init" previously triggered by cdns_pcie_host_init() errors, and now triggered by cdns_pcie_host_setup() and cdns_pcie_ep_setup() errors. These are not propagated and will result in probing success, which is incorrect. Fixes: bd22885 ("PCI: cadence: Refactor driver to use as a core library") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
It appears that libahci.c, ahci.c as well as the ahci_brcm, ahci_ceva and ahci_qoriq drivers are using runtime PM operations without including <linux/pm_runtime.h>. This header is somehow being indirectly provided by <linux/phy/phy.h>, which would like to drop it (none of the functions it exports need it). Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Acked-by: Damien Le Moal <dlemoal@kernel.org> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The tegra as well as a few dwc PCI controller drivers uses PM runtime operations without including the required <linux/pm_runtime.h> header. Similarly, pcie-rockchip-host, pcie-starfive as well as a few dwc PCI controllers use the regulator consumer API without including <linux/regulator/consumer.h>. pcie-spacemit-k1.c uses of_get_next_available_child() and of_node_put() without including <linux/of.h>. It seems these function prototypes were indirectly provided by <linux/phy/phy.h>, mostly by mistake (none of the functions it exports need it). Before the PHY header can drop the unnecessary includes, make sure the PCI controller drivers include what they use. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The chipidea ci_hdrc_imx driver uses regulator consumer API like regulator_enable() but does not include <linux/regulator/consumer.h>. The core USB HCD driver calls invalidate_kernel_vmap_range() and flush_kernel_vmap_range(), but does not include <linux/highmem.h>. The DWC3 gadget driver calls: - device_property_present() - device_property_count_u8() - device_property_read_u8_array() but does not include <linux/property.h> Similarly, dwc3-imx uses device_property_read_bool() without including <linux/property.h>. The dwc3-generic-plat driver uses of_device_get_match_data() but does not include <linux/of.h>. In all these cases, the necessary includes were still provided somehow, directly or indirectly, through <linux/phy/phy.h>. I found the following command to be quite helpful in figuring out the include chain: $ make KCFLAGS="-H" drivers/usb/dwc3/dwc3-imx.o Since <linux/phy/phy.h> wants to drop the unnecessary includes, fill in the required headers to avoid any breakage. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> # dwc3 Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Multiple DRM bridge drivers use runtime PM operations without including the proper header, instead relying on transitive inclusion by <linux/phy/phy.h>. The PHY subsystem wants to get rid of headers it provides for no reason, so modify these drivers to include what they need directly. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
It appears that a number of PHY provider drivers call runtime PM operations without including the proper header. This was provided by <linux/phy/phy.h> but no function exported by this header directly needs it. So we need to drop it from there, and fix up drivers that used to depend on that. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> # renesas Reviewed-by: André Draszik <andre.draszik@linaro.org> # google Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
This driver relies on a transitive inclusion of the PHY API header through the USB headers. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Yixun Lan <dlan@kernel.org> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
This file is calling of_property_read_u32() without including the proper header for it. It is provided by <linux/phy/phy.h>, which wants to get rid of it. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Acked-by: Daniel Machon <daniel.machon@microchip.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
This is practically a full revert of commit 7a4db65 ("PCI: dra7xx: Create functional dependency between PCIe and PHY") and a partial revert of the device link pieces from commits dfb8053 ("PCI: cadence: Add generic PHY support to host and EP drivers") 4922923 ("PCI: keystone: Cleanup PHY handling") The trouble with these commits is that they dereference fields inside struct phy from a consumer driver, which will become no longer possible. Since commit 987351e ("phy: core: Add consumer device link support") from 2019, the PHY core also adds a device link to order PHY provider and consumer suspend/resume operations. All reverted commits are from 2017-2018, and what they do should actually be redundant now. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
I am trying to get rid of code instances where PHY consumers (like the Exynos UFS HCD) poke inside struct phy fields, in order to further turn struct phy into an opaque data structure. The ufs-exynos.c driver interacts with phy-samsung-ufs.c in order to power it on and to update the lane count. For the later purpose, it (ab)uses phy_set_bus_width(). The phy_set_bus_width() function is a PHY provider function, not a consumer one, and I am calling its use from ufs-exynos.c an abuse because (1) commit 8feed34 ("phy: add phy_get_bus_width()/phy_set_bus_width() calls") clearly states the intended use. (2) phy_set_bus_width() only alters phy->attrs.bus_width, and does not call into phy_ops at all. So a consumer that makes a call to phy_set_bus_width() can not possibly produce any hardware change in the provider at all. This is where the Exynos UFS HCD driver decided to be creative and hijacked phy_init() to pick up the change of the bus_width attribute. This requires a very careful dance where the PHY consumer needs to simultaneously juggle multiple requirements: - the UFS PHY needs to pick up the updated lane count in its samsung_ufs_phy_init() handler for the phy_init() call - phy_init() calls need to be balanced with phy_exit(), otherwise subsequent phy_init() calls don't make it into samsung_ufs_phy_init() and just leave the PHY with an elevated init_count - phy_power_on() can't be called without phy_init() The difficulty to observe all requirements is why the following bug fix commits exist: 3d73b20 ("scsi: ufs: ufs-exynos: Change ufs phy control sequence") 7f05fd9 ("scsi: ufs: exynos: Ensure consistent phy reference counts") Currently the UFS HCD driver tries to keep the PHY init_count and power_count in tight lockstep, but even this is error-prone. For example, if exynos_ufs_suspend() runs and then exynos_ufs_exit(), the PHY power_count will underflow. If we address the root issue first (phy_init() abused to pick up new lane count) by introducing a new PHY consumer method which actually does call into the PHY provider driver, then we are able to absorb the entire UFS HCD dance and update the lane count without altering the PHY init_count or power_count. This allows more consumer flexibility to call phy_init() from other places, and same goes for phy_power_on(). It is common practice to call phy_init() only once, right after phy_get(), and doing so will naturally balance it with phy_exit(). We can also leave the phy_power_on() call to be on demand, placed inside exynos_ufs_pre_link(). But the UFS core (specifically ufshcd_link_startup()) may call the variant operation exynos_ufs_pre_link() -> exynos_ufs_phy_init() multiple times if the link startup fails and needs to be retried. For this reason we need a consumer-specific "bool phy_powered_on" which ensures that we call phy_power_on() at most once, and that exynos_ufs_exit() only calls phy_power_off() if phy_power_on() was previously called. Using the phy->power_count for this purpose is undesirable because it is going away, and the PHY API is not offering a helper for it (would be a foot gun, because multiple consumers of the same provider shouldn't interfere with each other; each should only undo the effects of what it did itself) Inside the new samsung_ufs_phy_request_bus_width(), I've sanity checked that the bus width is either 1 or 2 lanes. This coincides with samsung_ufs_phy_config() which only configures LANE_0 and LANE_1. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The Qualcomm UFS host controller interacts with the QMP PHY in a way
which violates the Generic PHY API expectation, documented in section
"Order of API calls" from Documentation/driver-api/phy/phy.rst, and then
tries to hide it.
Namely, calls must be made in the phy_init() -> phy_power_on() ->
phy_power_off() -> phy_exit() sequence.
What we actually have is:
ufshcd_init()
-> ufshcd_hba_init()
-> ufshcd_setup_clocks(hba, true)
-> ufshcd_vops_setup_clocks(hba, true, POST_CHANGE)
-> ufs_qcom_setup_clocks(hba, true, POST_CHANGE)
-> ufs_qcom_init() has not run, simply ignore
-> ufshcd_variant_hba_init()
-> ufs_qcom_init()
-> ufs_qcom_setup_clocks(hba, true, POST_CHANGE)
-> phy_power_on(phy)
-> ufshcd_hba_enable()
-> ufshcd_vops_hce_enable_notify()
-> ufs_qcom_hce_enable_notify()
-> ufs_qcom_power_up_sequence()
-> if (phy->power_count) phy_power_off(phy)
-> phy_init(phy)
This "works" because the way that the "phy_power_on was called before
phy_init\n" warning condition is detected in phy-core.c is if the
power_count is positive at the phy_init() call time.
By having that "if (phy->power_count) phy_power_off(phy)" logic, the
ufs-qcom.c technically sidesteps the test, but actually violates the
Generic PHY API even more (calls phy_power_on() *and* phy_power_off()
before phy_init()).
The reason why I stumbled upon this was that I was trying to remove
dereferences of phy->power_count (a PHY internal field) from consumer
drivers.
phy_init(), implemented as qmp_ufs_phy_init(), calls qmp->ufs_reset =
devm_reset_control_get_exclusive(), so my understanding is that it needs
to be called:
- no earlier than ufs_qcom_init() -> devm_reset_controller_register()
which makes qmp->ufs_reset available
- no later than ufs_qcom_power_up_sequence() -> phy_calibrate() ->
qmp_ufs_phy_calibrate() where the qmp->ufs_reset is needed; although
phy_init() should be the first PHY API call made.
The only mystery is why is the current phy_init() placement so late, in
ufs_qcom_power_up_sequence(), but I guess the answer is that the
placement is vestigial. After the incremental work of commit
c9b5897 ("phy: qcom: Utilize UFS reset controller") from
Evan Green and commit cbfd6c1 ("phy: qcom-qmp-ufs: Refactor
phy_power_on and phy_calibrate callbacks") from Nitin Rawat, the entire
multi-stage PHY init procedure was moved to phy_power_on(), but nobody
bothered to move phy_init() anywhere else more natural.
So hopefully if the calculations are right, any placement within that
bounding box should be good, and I'm picking the new phy_init() location
to be in ufs_qcom_init().
Even with phy_init() out of the way, ufs_qcom_power_up_sequence() ->
phy_power_off() is still needed, for a separate reason which will be
dealt with separately.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Currently, phy_set_mode_ext() on the QMP UFS PHY makes no change to the hardware state, instead it is mandatory that phy_power_on() followed by phy_calibrate() be run afterwards, for the new mode to be picked up. By absorbing the phy_power_off() -> ... -> phy_power_on() -> phy_calibrate() surrounding sequence into phy_set_mode_ext(), the UFS HCD consumer driver can be greatly simplified, and we also have a proper self-standing phy_set_mode_ext() implementation which does not rely on other calls to do its job. So simplify ufs_qcom_power_up_sequence() to only call phy_set_mode_ext() and let PHY power management be handled just by ufs_qcom_setup_clocks(). Actually, after this change, ufs_qcom_power_up_sequence() becomes an inadequate name, since from the consumer perspective the powering up is invisible. So change it to ufs_qcom_phy_change_mode(). The consumer and the provider are modified at once because ufs-qcom.c already calls phy_set_mode_ext() while the QMP PHY is powered on, so introducing the extra logic in qmp_ufs_set_mode() would cause a potentially breaking second QMP PHY power sequence until the consumer is patched to remove its own calls. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The Generic PHY API needs the following call order: phy_init() -> phy_power_on() -> phy_power_off() -> phy_exit() with a balanced number of phy_init() <-> phy_exit() and phy_power_on() <-> phy_power_off() calls. The UFS framework is not exactly great in helping out with obeying these requirements. For example, the Qualcomm UFS HCD driver insists pairing the PHY power to the clock setup operations. But during driver removal, we have: ufshcd_hba_exit() -> ufshcd_variant_hba_exit() -> ufs_qcom_exit() -> ufshcd_setup_clocks(hba, false) which means that we will underflow the PHY power_count. Adding a "bool power_count" to the driver and checking it before each call helps avoid this issue. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
This driver uses devm_request_irq() without including <linux/interrupt.h>
by itself, which would lead to build failures if the headers providing
this transitively were to stop providing it.
On aarch64, we can see, using KCFLAGS='-H' make drivers/ufs/host/ufs-qcom.o,
that the inclusion path is:
drivers/ufs/host/ufs-qcom.c
-> include/linux/acpi.h
-> arch/arm64/include/asm/acpi.h
-> include/linux/efi.h
-> include/linux/rtc.h
-> include/linux/interrupt.h
Whereas on armv7, the situation is quite different. This architecture
has no CONFIG_ACPI symbol, and therefore on it, <linux/acpi.h> does not
include <asm/acpi.h>, and <linux/interrupt.h> is not provided that way.
It is provided, however, through this "fallback" path:
drivers/ufs/host/ufs-qcom.c
-> include/linux/phy/phy.h
-> include/linux/regulator/consumer.h
-> include/linux/suspend.h
-> include/linux/swap.h
-> include/linux/memcontrol.h
-> include/linux/writeback.h
-> include/linux/interrupt.h
The point is that <linux/phy/phy.h> will stop providing
<linux/regulator/consumer.h>, and this would break the transitive
include chain on armv7.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The dw_hdmi-rockchip driver validates pixel clock rates against the HDMI PHY's internal clock provider on certain SoCs like RK3328. This is currently achieved by dereferencing hdmi->phy->dev.of_node to obtain the provider node, which violates the Generic PHY API's encapsulation (the goal is for struct phy to be an opaque pointer with a hidden definition, to be interacted with only using API functions or NULL pointer checks, for the case where optional variants of phy_get() did not find a PHY). Refactor dw_hdmi_rockchip_bind() to perform a manual phandle lookup on the "hdmi" PHY index within the controller's DT node. This provides a parallel path to the clock provider's OF node without relying on the internal structure of the struct phy handle. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Heiko Stueber <heiko@sntech.de> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
A piece of context which seems relevant here is that the USB subsystem is transitioning from struct usb_phy to struct phy (belonging to the Generic PHY subsystem). Commit 1a229d8 ("Revert "usb: phy: add usb phy notify port status API"") seems to confirm that this is the case. In the transition process, some PHY provider drivers register themselves as both Generic PHY and USB PHY in an attempt to bridge the API gap. Such is the case with drivers/phy/tegra/xusb.c, accessed here by the Tegra USB host driver. This USB host expects the PHY device behind the Generic PHY to also be a USB PHY, and calls devm_usb_get_phy_by_node(phy->dev.of_node). The Generic PHY exposes no API to get the OF node from a PHY device, so the Tegra USB host driver gets it directly. However, "struct phy" will be made an opaque pointer, to avoid misuse, so this will no longer be possible. Considering the fact that the Generic PHY/USB PHY duality is a transitional state, I am deliberately not planning to make the life of this driver any easier by providing a helper to get to the OF node somehow. Instead, implement a parallel lookup path through which the Tegra USB host driver can continue to get to the OF node provided by the padctl component, using the 'phys' phandle. Secondly (minor issue) the driver uses the phy->dev.of_node again to print using dev_dbg() that a "remote wake" was detected. Just print the index at which the PHY appears inside the driver's tegra->phys[] array instead. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
In a somewhat similar situation as the Tegra USB host controller driver, the Tegra XUDC driver for USB gadget mode needs to get to a struct usb_phy that sits behind the same OF node as the Generic PHY. It does that directly, which will no longer be possible. The PHY provider is also the xusb padctl driver. The rework here is also to implement a parallel OF node lookup path based on the "phys" phandle and the #phy-cells of the padctl provider. Some further notes: - create a local "usbphy" variable to hold the devm_usb_get_phy_by_node() output. This makes the error checks more obvious (avoids keeping an error-encoded pointer in xudc->usbphy[i] even temporarily). - the "if (IS_ERR(utmi_phy)) .. else if (utmi_phy) .. else if (!utmi_phy)" pattern can be simplified, considering that neither the IS_ERR() nor the NULL case continue execution in the current block. Therefore, we can move the case where the "utmi_phy" is a valid pointer outside the "if" checks, and this reduces the code indentation level. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The major goal is to hide the contents of struct phy from consumer drivers. The idea with "phy-props.h" is that both consumers and providers make use of some data types. So both headers include "phy-props.h". Some slight points of contention. 1. phy-provider.h should go to include/linux/phy/ or to drivers/phy/? We do have 3 PHY providers outside of drivers/phy/: drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.c drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c drivers/pinctrl/tegra/pinctrl-tegra-xusb.c but the practice is not encouraged, and with time, these should be moved to the subsystem. This is not something that I can do now. 2. We can no longer tolerate static inline helpers. Allowing these would make it impossible to hide the struct phy definition from consumers. I've made phy_get_mode(), phy_get_bus_width() exported symbols in drivers/phy/phy-core.c. 3. This is not a change without side effects. In the transition we are no longer providing <linux/pm_runtime.h> at all, and <linux/regulator/consumer.h> to PHY consumer drivers. However, the in-tree dependencies should all have been resolved. Also, the movement of phy-provider.h to drivers/phy/ is at least "interesting" for out of tree PHY provider drivers (this header is not deployed by make headers_install). However, it seems to be what Vinod is looking to see. For temporary compatibility, keep including the provider header. This will be removed when abuses are all gotten rid of. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The PHY API has an optional "get" which returns NULL, so it needs to accept that NULL coming back in. Most PHY functions do this, only the formerly static inline attribute dereferences did not. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Consumer drivers shouldn't dereference struct phy, not even to get to its attributes. We have phy_get_bus_width() as a precedent for getting the bus_width attribute, so let's add phy_get_max_link_rate() and use it in DRM and CAN drivers. In CAN drivers, the transceiver is acquired through devm_phy_optional_get() and NULL is given by the API as a non-error case, so the PHY API should also tolerate NULL coming back to it. This means we can further simplify the call sites that test for the NULL quality of the transceiver. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Acked-by: Markus Schneider-Pargmann <msp@baylibre.com> # m_can Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> # rcar_canfd Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The Rockchip DSI controller is a PHY consumer driver, which is also a PHY provider (calls devm_phy_create()) that lives out of drivers/phy/. According to Vinod, this is discouraged, although it would be difficult for me to address a proper movement here. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The Cadence MHDP8546 DP bridge driver gets the PHY bus_width attribute (holding number of lanes) directly, but doing this will no longer be possible after the definition of struct phy is hidden from consumers. Use the phy_get_bus_width() API function designed specifically for consumers. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The introduction commit 576d196 ("media: sunxi: Add support for the A83T MIPI CSI-2 controller") says: This implementation splits the protocol and D-PHY registers and uses the PHY framework internally. The D-PHY is not registered as a standalone PHY driver since it cannot be used with any other controller. However, this does not matter, and is not the only instance of tight PHY provider <-> consumer pairing. According to Vinod Koul, using the PHY provider API outside of drivers/phy/ is discouraged, although it would be difficult for me to address a proper movement here. So just include the private provider API header from drivers/phy/ and leave a FIXME in place. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Acked-by: Chen-Yu Tsai <wens@kernel.org> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
As a PHY consumer driver, the Renesas rswitch dereferences internal fields of struct phy, something which shouldn't be done, as that is going to be made an opaque pointer. It is quite clearly visible that the driver is tightly coupled with the drivers/phy/renesas/r8a779f0-ether-serdes.c, which puts heavy pressure on the Generic PHY subsystem. This was discussed before here: https://lore.kernel.org/linux-phy/20260211194541.cdmibrpfn6ej6e74@skbuf/ but to summarize, it is generally expected that when a Generic PHY function is called, it takes effect immediately. When this doesn't happen, the PHY provider driver must change its implementation rather than the consumer be made to work around it. PHY providers which rely on a hardcoded call sequence in the consumer are just lazy and wrong. The most obvious example is commit 5cb6309 ("net: renesas: rswitch: Add phy_power_{on,off}() calling"). Problem description: - Ethernet PHYs may change phydev->interface. When this happens, the SerDes must learn of the new phydev->interface using phy_set_mode_ext(). - drivers/phy/renesas/r8a779f0-ether-serdes.c implements phy_set_mode_ext(), but this only caches the mode and submode into channel->phy_interface and applies this to hardware during phy_power_on(). The commit author decided to work around this at the consumer site, by power cycling the PHY for the configuration to take effect. This had a worse implication from an API perspective in subsequent commit 053f13f ("rswitch: Fix imbalance phy_power_off() calling"). It was observed that phy_power_on() and phy_power_off() calls need to be balanced, and so, the consumer decided to start looking at the struct phy :: power_count (the technical reason why I'm making this change). This is also wrong from an API perspective because - a consumer should only care about its own vote on the PHY power state. If this is a multi-port submode like QSGMII, a single phy_power_off() call will not actually turn the PHY off (nor should it). - the power_count is written under the &phy->mutex, but read unlocked here. The rswitch and r8a779f0-ether-serdes drivers both need to be completely rethought in terms of Generic PHY API call sequence. There is no quick fix to apply. Just include the PHY provider API along with the consumer one, to keep working as before when struct phy will be made an opaque pointer to normal PHY consumers. But this is a bad offender (and it's not even a provider) so add a FIXME. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The tegra-xusb pinctrl driver is also a PHY provider (calls devm_phy_create() for PCIe and SATA). However, according to Vinod Koul, having PHY provider drivers outside of drivers/phy/ is discouraged, although it would be difficult for me to address a proper movement here. Include the private provider API header from drivers/phy/, but leave a FIXME in place. It will have to be moved, eventually. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Linus Walleij <linusw@kernel.org> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
This file uses dev_fwnode() without including the proper header for it,
relying on transitive header inclusion from:
drivers/power/supply/cpcap-charger.c
- include/linux/phy/omap_usb.h
- include/linux/usb/phy_companion.h
- include/linux/usb/otg.h
- include/linux/phy/phy.h
- drivers/phy/phy-provider.h
- include/linux/of.h
- include/linux/property.h
With the future removal of drivers/phy/phy-provider.h from
include/linux/phy/phy.h, this transitive inclusion would break.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Since the PHY provider API has moved to drivers/phy/phy-provider.h and include/linux/phy/ulpi_phy.h needs it (phy_create(), phy_create_lookup(), etc), naturally it means ulpi_phy.h is also a PHY provider header and should be moved to the same location. The header is included only from drivers/phy/ti/phy-tusb1210.c, which confirms that PHY consumers do not need it. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The majority of PHY drivers are PHY providers (obviously). Some are providers *and* consumers (phy-meson-axg-mipi-dphy, phy-meson-axg-pcie). These are the Amlogic AXG SoCs, which split the physical layer into two chained PHYs: the digital layer and the analog layer. The DSI or PCIe controller interacts only with the digital PHY, presumably for simplicity. The rest of PHY drivers which include <linux/phy/phy.h> do so because they call phy_pm_runtime_*(), phy_get_bus_width() or phy_get_mode(), nominally consumer functions. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Acked-by: Chen-Yu Tsai <wens@kernel.org> # allwinner Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The majority of PHY drivers are PHY providers (obviously). Some are chained PHY provider+consumer (phy-qcom-m31-eusb2.c, phy-exynos5-usbdrd.c). Others include <linux/phy/phy.h> because they call consumer functions such as phy_pm_runtime_get() - phy-mapphone-mdm6600.c. See commit 2ad2af0 ("phy: mapphone-mdm6600: Improve phy related runtime PM calls") for the story behind that. My understanding is it's a pragmatic shortcut, but it doesn't bother much. Another is the recently moved drivers/phy/ulpi_phy.h, which is used by the TI phy-tusb1210.c provider. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> #phy/qualcomm Acked-by: Shawn Lin <shawn.lin@rock-chips.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Now that all consumers have been updated to no longer dereference fields inside struct phy, we can hide its definition altogether from public view. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Author
|
Patch 29: "[v8,phy-next,29/31] phy: include PHY provider header (2/2)" |
Author
|
Patch 29: "[v8,phy-next,29/31] phy: include PHY provider header (2/2)" |
Author
|
Patch 29: "[v8,phy-next,29/31] phy: include PHY provider header (2/2)" |
Author
|
Patch 29: "[v8,phy-next,29/31] phy: include PHY provider header (2/2)" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 30: "[v8,phy-next,30/31] phy: remove temporary provider compatibility from consumer header" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
Author
|
Patch 31: "[v8,phy-next,31/31] MAINTAINERS: add regexes for linux-phy" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR for series 1089787 applied to workflow__riscv__fixes
Name: Split Generic PHY consumer and provider API
URL: https://patchwork.kernel.org/project/linux-riscv/list/?series=1089787
Version: 8