| Message ID | 20210827092753.2359-3-pali@kernel.org |
|---|---|
| State | Accepted |
| Commit | 0c1f5f2a5581827fadf493711c4e95528a5730aa |
| Headers | show |
| Series | [1/3] phy: marvell: phy-mvebu-cp110-comphy: Rename HS-SGMMI to 2500Base-X | expand |
On Fri, 27 Aug 2021 11:27:53 +0200 Pali Rohár <pali@kernel.org> wrote: > Armada 3700 does not support RXAUI, XFI and neither SFI. Remove unused > macros for these unsupported modes. > > Signed-off-by: Pali Rohár <pali@kernel.org> > Fixes: 9695375a3f4a ("phy: add A3700 COMPHY support") BTW I was thinking about merging some parts of these 2 drivers into common code. Not entirely sure if I should follow, though.
On Friday 27 August 2021 13:27:13 Marek Behún wrote: > On Fri, 27 Aug 2021 11:27:53 +0200 > Pali Rohár <pali@kernel.org> wrote: > > > Armada 3700 does not support RXAUI, XFI and neither SFI. Remove unused > > macros for these unsupported modes. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Fixes: 9695375a3f4a ("phy: add A3700 COMPHY support") > > BTW I was thinking about merging some parts of these 2 drivers into > common code. Not entirely sure if I should follow, though. cp110-comphy and a3700-comphy are just RPC drivers which calls SMC and relay on firmware support which implements real work. And both uses same RPC / SMC API. So merging drivers into one is possible. But I do not think that it is a good idea that Linux kernel depends on some external firmware which implements RPC API for configuring HW. Rather kernel should implements its native drivers without dependency on external firmware. We know from history that kernel tried to avoid using x86 BIOS/firmware due to bugs and all functionality (re)-implemented itself without using firmware RPC functionality. Kernel has already "hacks" in other drivers which are using these comphy drivers, just because older versions of firmware do not support all necessary functionality and upgrading this firmware is not easy step (and sometimes even not possible; e.g. when is cryptographically signed).
On Fri, Aug 27, 2021 at 08:25:02PM +0200, Pali Rohár wrote: > cp110-comphy and a3700-comphy are just RPC drivers which calls SMC and > relay on firmware support which implements real work. And both uses same > RPC / SMC API. So merging drivers into one is possible. > > But I do not think that it is a good idea that Linux kernel depends on > some external firmware which implements RPC API for configuring HW. > > Rather kernel should implements its native drivers without dependency on > external firmware. > > We know from history that kernel tried to avoid using x86 BIOS/firmware > due to bugs and all functionality (re)-implemented itself without using > firmware RPC functionality. Not really an argument in this case. We're not talking about closed source firmware. > Kernel has already "hacks" in other drivers which are using these comphy > drivers, just because older versions of firmware do not support all > necessary functionality and upgrading this firmware is not easy step > (and sometimes even not possible; e.g. when is cryptographically > signed). The kernel used to (and probably still does) contain code to configure the comphys. Having worked on trying to get the 10G lanes stable on Macchiatobin, I much prefer the existing solution where it's in the ATF firmware. I've rebuilt the firmware several times during the course of that. The advantage is that fixing the setup of the COMPHY is done in one place, and it fixes it not only for the kernel, but also for u-boot and UEFI too. So rather than having to maintain three different places for a particular board, we can maintain the parameters in one place - in the ATF firmware. The problem with the past has been that stuff gets accepted into the kernel without the "full system" view and without regard for "should this actually be done in the firmware". Then, when it's decided that it really should be done in the firmware, we end up needing to keep the old stuff in the kernel for compatibility with older firmware, which incidentally may not be up to date. If we were to drop the comphy setup from firmware, then we will need a lot of additional properties in the kernel's DT and u-boot DT for the comphy to configure it appropriately. And ACPI. I don't think that scales very well, and is a recipe for things getting out of step.
On Friday 27 August 2021 19:33:55 Russell King (Oracle) wrote: > On Fri, Aug 27, 2021 at 08:25:02PM +0200, Pali Rohár wrote: > > cp110-comphy and a3700-comphy are just RPC drivers which calls SMC and > > relay on firmware support which implements real work. And both uses same > > RPC / SMC API. So merging drivers into one is possible. > > > > But I do not think that it is a good idea that Linux kernel depends on > > some external firmware which implements RPC API for configuring HW. > > > > Rather kernel should implements its native drivers without dependency on > > external firmware. > > > > We know from history that kernel tried to avoid using x86 BIOS/firmware > > due to bugs and all functionality (re)-implemented itself without using > > firmware RPC functionality. > > Not really an argument in this case. We're not talking about closed > source firmware. > > > Kernel has already "hacks" in other drivers which are using these comphy > > drivers, just because older versions of firmware do not support all > > necessary functionality and upgrading this firmware is not easy step > > (and sometimes even not possible; e.g. when is cryptographically > > signed). > > The kernel used to (and probably still does) contain code to configure > the comphys. Kernel does not have code for A3700. Hence reason why there are hacks in other drivers, like libata/ahci or usb/xhci. > Having worked on trying to get the 10G lanes stable on > Macchiatobin, I much prefer the existing solution where it's in the > ATF firmware. I've rebuilt the firmware several times during the course > of that. In some cases rebuilt of firmware does not have to be possible (e.g. when it it signed). > The advantage is that fixing the setup of the COMPHY is done in one > place, and it fixes it not only for the kernel, but also for u-boot U-Boot for A3720 has its own implementation and does not use firmware implementation (yet). So currently the only consumer of firmware implementation is just Linux kernel. > and UEFI too. So rather than having to maintain three different > places for a particular board, we can maintain the parameters in one > place - in the ATF firmware. Same argument can be used for any other driver which is implemented in both bootloader and kernel... But I understand also your argument. > The problem with the past has been that stuff gets accepted into the > kernel without the "full system" view and without regard for "should > this actually be done in the firmware". Then, when it's decided that > it really should be done in the firmware, we end up needing to keep > the old stuff in the kernel for compatibility with older firmware, > which incidentally may not be up to date. > > If we were to drop the comphy setup from firmware, then we will need > a lot of additional properties in the kernel's DT and u-boot DT for > the comphy to configure it appropriately. And ACPI. I don't think > that scales very well, and is a recipe for things getting out of > step. I think that whatever is used (firmware code, kernel code, ...), DT should always contains full HW description with all nodes, and not only some "subset". DT should be independent of current driver / firmware implementation.
On Fri, Aug 27, 2021 at 09:02:34PM +0200, Pali Rohár wrote: > I think that whatever is used (firmware code, kernel code, ...), DT > should always contains full HW description with all nodes, and not only > some "subset". DT should be independent of current driver / firmware > implementation. A "full" description of the hardware settings for the comphy on Armada 8040 would be very big if we included every single setting. There are about a thousand settings - and that is likely an under-estimate. I know, I've a shell script that decodes around a thousand settings from the registers for a _single_ lane, and that's incomplete. With many of the settings not very well documented in the manual, we would struggle to describe it sufficiently well to get it past the DT maintainers. So, "full hardware description" is impractical. Sorry.
diff --git a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c index cc534a5c4b3b..6781488cfc58 100644 --- a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c +++ b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c @@ -33,18 +33,12 @@ #define COMPHY_FW_MODE_USB3H 0x4 #define COMPHY_FW_MODE_USB3D 0x5 #define COMPHY_FW_MODE_PCIE 0x6 -#define COMPHY_FW_MODE_RXAUI 0x7 -#define COMPHY_FW_MODE_XFI 0x8 -#define COMPHY_FW_MODE_SFI 0x9 #define COMPHY_FW_MODE_USB3 0xa #define COMPHY_FW_SPEED_1_25G 0 /* SGMII 1G */ #define COMPHY_FW_SPEED_2_5G 1 #define COMPHY_FW_SPEED_3_125G 2 /* 2500BASE-X */ #define COMPHY_FW_SPEED_5G 3 -#define COMPHY_FW_SPEED_5_15625G 4 /* XFI 5G */ -#define COMPHY_FW_SPEED_6G 5 -#define COMPHY_FW_SPEED_10_3125G 6 /* XFI 10G */ #define COMPHY_FW_SPEED_MAX 0x3F #define COMPHY_FW_MODE(mode) ((mode) << 12)
Armada 3700 does not support RXAUI, XFI and neither SFI. Remove unused macros for these unsupported modes. Signed-off-by: Pali Rohár <pali@kernel.org> Fixes: 9695375a3f4a ("phy: add A3700 COMPHY support") --- drivers/phy/marvell/phy-mvebu-a3700-comphy.c | 6 ------ 1 file changed, 6 deletions(-)