Hi Felipe and Kishon, >>> >> >> This patch adds function set_speed to the generic PHY framework operation >>> >> >> structure. This function can be called to instruct the PHY underlying layer >>> >> >> at specified lane to configure for specified speed in hertz. >>> >> > >>> >> > why ? looks like clk_set_rate() is your friend here. Can you be more >>> >> > descriptive of the use case ? When will this be used ? >>> >> > >>> >> >>> >> The phy_set_speed is used to configure the operation speed of the PHY >>> >> at run-time. The clock interface in general is used to configure the >>> >> clock input to the IP. I don't believe they are the same thing. Maybe >>> >> it will be clear in my response to your second email >>> > >>> > The problem with this is that you end up adding SATA-specific details to >>> > something which is supposed to be generic. >>> >>> Setting the operation speed is pretty generic from an interface point >>> of view. An generic multi-purpose PHY can support multiple speed. If >> >> no it's not. Specially when you consider that your "speed" argument can >> be just about anything and depending on the underlying bus, it *will* be >> treated differently. Note that e.g. in OMAP devices the exact *same* PHY >> IP is used for PCIe, SATA and USB... now, let's assume for the sake of >> argument that we were to implement ->set_speed() in that environment, >> how do you plan to do that ? a 6MHz arguments isn't valid from USB stand >> point and could mean different things in PCIe or SATA. >> > > Correct me if I am wrong here. If the same PHY is used for PCIe, SATA, > and USB, would you not need to indicate what speed the PHY should be > operated at - unless the underlying IP magically negotiate and > configured automatically? If so, what about PHY isn't so intelligent? > How should you suggest that we handle these? > >>> the upper layer wish to operate at an specified speed (say for testing >>> purpose and etc), it can be allowed. >> >> anything for testing purposes, should be limited to test scenarios. > > Testing purpose is only one use case. Another use case is to limit the > speed so that I can confirm the driver actually works with various > speed of the device and handle correctly. > >> >>> > After negoatiation, don't you >>> > get any interrupt from your PHY indicating that link speed negotiation >>> > is done ? Or is that IRQ only on AHCI IP ? >>> >>> There is NO interrupt from the PHY. The IRQ is assoicated with the >>> AHCI IP. With SATA host flow, it starts off with an COMRESET to start >>> the link negotiation. At that point, it will poll for completion. >>> >>> Any other concerns? >> >> hey, calm down... just trying to prevent us from applying something >> which isn't truly generic and I don't think "->set_speed" is generic >> enough. The semantics of the "speed" argument won't be valid for all use >> cases. >> >> I can already see people using that to pass >> USB_SPEED_{LOW,FULL,HIGH,SUPER}, instead of actual speed numbers. We wil >> end up with a mess to handle from a PHY driver, specially in cases such >> as OMAP where, as mentioned above, the same IP is used for SATA, PCIe >> and USB3. >> > > This PHY isn't as "intelligent" as other PHY's. What would you suggest > as I need an method to indicate to the underlying PHY that I want to > operate at an specified speed? Sorry if I am pinging you guys too fast here. I am look from an solution and open to any solution in which it is acceptable for your original intent of the generic PHY framework. I understand that you don't believe set_speed is generic enough and may not apply to omap. Or if you prefer we can try changing the init function to take an initial MAX speed? -Loc -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html