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

[Stephen Boyd]

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?

> 
> 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?

> >> +       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.