[PATCH v8 4/9] phy: fsl: Add Lynx 10G SerDes driver

[Sean Anderson]

On 11/1/22 16:07, Stephen Boyd wrote:
> Quoting Sean Anderson (2022-10-28 09:13:57)
>> On 10/27/22 19:03, Stephen Boyd wrote:
>> > Quoting Sean Anderson (2022-10-27 12:11:08)
>> >> diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
>> >> index 853958fb2c06..a6ccccf9e39b 100644
>> >> --- a/drivers/phy/freescale/Kconfig
>> >> +++ b/drivers/phy/freescale/Kconfig
>> >> @@ -47,3 +47,25 @@ config PHY_FSL_LYNX_28G
>> >>            found on NXP's Layerscape platforms such as LX2160A.
>> >>            Used to change the protocol running on SerDes lanes at runtime.
>> >>            Only useful for a restricted set of Ethernet protocols.
>> >> +
>> >> +config PHY_FSL_LYNX_10G
>> >> +       tristate "Freescale QorIQ Lynx 10G SerDes support"
>> >> +       depends on COMMON_CLK
>> > 
>> > Does something not compile if COMMON_CLK is disabled?
>> 
>> ld: drivers/phy/freescale/phy-fsl-lynx-10g-clk.o: in function `lynx_pll_round_rate':
>> phy-fsl-lynx-10g-clk.c:(.text+0x444): undefined reference to `clk_hw_round_rate'
>> ld: drivers/phy/freescale/phy-fsl-lynx-10g-clk.o: in function `lynx_clks_init':
>> phy-fsl-lynx-10g-clk.c:(.text+0x5eb): undefined reference to `devm_clk_hw_register'
>> ld: phy-fsl-lynx-10g-clk.c:(.text+0x625): undefined reference to `devm_clk_hw_register'
> 
> Cool thanks!
> 
>> 
>> >> diff --git a/drivers/phy/freescale/lynx-10g.h b/drivers/phy/freescale/lynx-10g.h
>> >> new file mode 100644
>> >> index 000000000000..75d9353a867b
>> >> --- /dev/null
>> >> +++ b/drivers/phy/freescale/lynx-10g.h
>> >> @@ -0,0 +1,16 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0 */
>> >> +/*
>> >> + * Copyright (C) 2022 Sean Anderson <sean.anderson at seco.com>
>> >> + */
>> >> +
>> >> +#ifndef LYNX_10G
>> >> +#define LYNX_10G
>> >> +
>> >> +struct clk;
>> >> +struct device;
>> >> +struct regmap;
>> >> +
>> >> +int lynx_clks_init(struct device *dev, struct regmap *regmap,
>> > 
>> > Can you use auxiliary bus to register this clk controller instead and
>> > then move the clk file to drivers/clk/?
>> 
>> I don't want to have to deal with my clock driver getting unbound (aka
>> the user has come and decided to make my life harder). Dynamic binding
>> will only add complexity in this situation.
> 
> We have 'suppress_bind_attrs' for that. What dynamic binding are you
> thinking about?

Huh, neat. That may save me a lot of trouble in another subsystem.

This was actually my main objection. I'll have a look at this bus...

>> 
>> I don't know how much context you've picked up, but this driver
>> 
>> - Has one consumer, and is is the serdes.
>> - Is not accessible from outside the serdes.
>> - Does not share any code with other drivers.
>> - Has bits in its registers which can control the reset process of lanes
>>    using the PLLs.
>> 
>> These drivers are tightly coupled to each other. It is very likely IMO
>> that changes to one (bugs, features, etc) will affect the other. For
>> this reason, I think it makes sense to keep them in the same source
>> directory. I actually would have preferred to keep them in the same
>> file.
>> 
> 
> Using the auxiliary bus is about getting better code review on the
> subsystem specific parts of a device. I'm not going to be paying a lot
> of attention to the clk parts of this driver if it is outside
> drivers/clk. Making this change helps with better code review.
> 
> The Kconfig symbol could be the same for the clk part and the phy part,
> and this is already split to a different file. It seems that your
> argument for keeping the clk file in the phy directory is because
> they're part of the same phy device. Do you expect to get clk driver
> review on the clk parts with the clk implementation in a different
> directory?

...yes? Actually, I was mainly hoping for phy driver review (since I
consider these PLLs part of the serdes), but you seem to be much more
responsive, so I'll take what I can get.

>> >> +       for (i = 0; i < NUM_PLLS; i++) {
>> >> +               ret = lynx_clk_init(hw_data, dev, regmap, i);
>> >> +               if (ret)
>> >> +                       return ret;
>> >> +
>> >> +               plls[i] = hw_data->hws[LYNX10G_PLLa(i)]->clk;
>> >> +               ex_dlys[i] = hw_data->hws[LYNX10G_PLLa_EX_DLY(i)]->clk;
>> > 
>> > Use clk_hw_get_clk() please.
>> 
>> I don't want to do that, because then I'd have to generate the clock ID
>> again. And why do we even need a new clock consumer in the first place?
>> This is only for internal use by the driver; the consumer is the same as
>> the producer.
>> 
> 
> We want to get rid of the clk pointer stashed in clk_hw. Using
> clk_hw_get_clk() lets us easily change that and not need to change this
> driver. I don't quite understand why you need to generate a clock ID.
> Are you concerned about passing in the 'con_id' argument? That string
> can be anything.

Yeah, but it has to be unique (there are usually multiple serdes on the same
system), and it should be something useful (so e.g. clock_summary is useful).
So it needs to be generated at runtime, and I'm already doing it when I create
the clock.

IMO this would be much easier with numeric IDs; generating strings is a pain.

--Sean