On Wed, Sep 01, 2021 at 08:41:10AM -0400, Tom Rini wrote:
> On Wed, Sep 01, 2021 at 02:40:21PM +0200, Pali Rohár wrote:
> > On Wednesday 01 September 2021 08:35:33 Tom Rini wrote:
> > > On Wed, Sep 01, 2021 at 02:32:43PM +0200, Pali Rohár wrote:
> > > > On Wednesday 01 September 2021 08:14:10 Tom Rini wrote:
> > > > > On Wed, Sep 01, 2021 at 11:12:58AM +0200, Stefan Roese wrote:
> > > > > 
> > > > > > Hi Pali,
> > > > > > 
> > > > > > On 16.08.21 12:02, Pali Rohár wrote:
> > > > > > > Like for all other mvebu platforms with CONFIG_SYS_TCLK macro, 
> > > > > > > define
> > > > > > > CONFIG_SYS_REF_CLK macro for a37xx with base reference clock 
> > > > > > > value which is
> > > > > > > read from latched reset register.
> > > > > > > 
> > > > > > > Replace all usages of get_ref_clk() function by this 
> > > > > > > CONFIG_SYS_REF_CLK
> > > > > > > macro and completely remove get_ref_clk() function.
> > > > > > > 
> > > > > > > Replace also custom open-coded implementation of determining 
> > > > > > > reference
> > > > > > > clock by CONFIG_SYS_REF_CLK in a37xx serial driver.
> > > > > > > 
> > > > > > > The only difference is that macro CONFIG_SYS_REF_CLK returns base 
> > > > > > > reference
> > > > > > > clock in Hz and old function get_ref_clk() returned it in MHz.
> > > > > > > 
> > > > > > > Signed-off-by: Pali Rohár <p...@kernel.org>
> > > > > > > 
> > > > > > > ---
> > > > > > > Changes in v2:
> > > > > > > * Do not remove MVEBU_TEST_PIN_LATCH_N and MVEBU_XTAL_MODE_MASK 
> > > > > > > macros
> > > > > > 
> > > > > > This patch does not apply any more, with all the other patches 
> > > > > > applied.
> > > > > > Please wait a bit until these patches are included in master and 
> > > > > > then
> > > > > > send a new version.
> > > > > > 
> > > > > > Sorry for the trouble.
> > > > > > 
> > > > > > Thanks,
> > > > > > Stefan
> > > > > > 
> > > > > > > ---
> > > > > > >   arch/arm/mach-mvebu/armada3700/cpu.c   | 24 
> > > > > > > ------------------------
> > > > > > >   arch/arm/mach-mvebu/include/mach/cpu.h |  7 -------
> > > > > > >   arch/arm/mach-mvebu/include/mach/soc.h |  6 ++++++
> > > > > > >   drivers/clk/mvebu/armada-37xx-periph.c |  6 +++---
> > > > > > >   drivers/clk/mvebu/armada-37xx-tbg.c    |  4 ++--
> > > > > > >   drivers/phy/marvell/comphy_a3700.c     | 12 ++++++------
> > > > > > >   drivers/serial/serial_mvebu_a3700.c    | 11 ++++-------
> > > > > > >   drivers/watchdog/armada-37xx-wdt.c     |  2 +-
> > > > > > >   8 files changed, 22 insertions(+), 50 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c 
> > > > > > > b/arch/arm/mach-mvebu/armada3700/cpu.c
> > > > > > > index 7702028ba19b..bdf8dc377528 100644
> > > > > > > --- a/arch/arm/mach-mvebu/armada3700/cpu.c
> > > > > > > +++ b/arch/arm/mach-mvebu/armada3700/cpu.c
> > > > > > > @@ -23,12 +23,6 @@
> > > > > > >   /* Armada 3700 */
> > > > > > >   #define MVEBU_GPIO_NB_REG_BASE          
> > > > > > > (MVEBU_REGISTER(0x13800))
> > > > > > > -#define MVEBU_TEST_PIN_LATCH_N           (MVEBU_GPIO_NB_REG_BASE 
> > > > > > > + 0x8)
> > > > > > > -#define MVEBU_XTAL_MODE_MASK             BIT(9)
> > > > > > > -#define MVEBU_XTAL_MODE_OFFS             9
> > > > > > > -#define MVEBU_XTAL_CLOCK_25MHZ           0x0
> > > > > > > -#define MVEBU_XTAL_CLOCK_40MHZ           0x1
> > > > > > > -
> > > > > > >   #define MVEBU_NB_WARM_RST_REG           (MVEBU_GPIO_NB_REG_BASE 
> > > > > > > + 0x40)
> > > > > > >   #define MVEBU_NB_WARM_RST_MAGIC_NUM     0x1d1e
> > > > > > > @@ -370,21 +364,3 @@ void reset_cpu(void)
> > > > > > >            */
> > > > > > >           writel(MVEBU_NB_WARM_RST_MAGIC_NUM, 
> > > > > > > MVEBU_NB_WARM_RST_REG);
> > > > > > >   }
> > > > > > > -
> > > > > > > -/*
> > > > > > > - * get_ref_clk
> > > > > > > - *
> > > > > > > - * return: reference clock in MHz (25 or 40)
> > > > > > > - */
> > > > > > > -u32 get_ref_clk(void)
> > > > > > > -{
> > > > > > > - u32 regval;
> > > > > > > -
> > > > > > > - regval = (readl(MVEBU_TEST_PIN_LATCH_N) & MVEBU_XTAL_MODE_MASK) 
> > > > > > > >>
> > > > > > > -         MVEBU_XTAL_MODE_OFFS;
> > > > > > > -
> > > > > > > - if (regval == MVEBU_XTAL_CLOCK_25MHZ)
> > > > > > > -         return 25;
> > > > > > > - else
> > > > > > > -         return 40;
> > > > > > > -}
> > > > > > > diff --git a/arch/arm/mach-mvebu/include/mach/cpu.h 
> > > > > > > b/arch/arm/mach-mvebu/include/mach/cpu.h
> > > > > > > index 79858858c259..9b8907e0fe55 100644
> > > > > > > --- a/arch/arm/mach-mvebu/include/mach/cpu.h
> > > > > > > +++ b/arch/arm/mach-mvebu/include/mach/cpu.h
> > > > > > > @@ -183,12 +183,5 @@ int a3700_dram_init_banksize(void);
> > > > > > >   /* A3700 PCIe regions fixer for device tree */
> > > > > > >   int a3700_fdt_fix_pcie_regions(void *blob);
> > > > > > > -/*
> > > > > > > - * get_ref_clk
> > > > > > > - *
> > > > > > > - * return: reference clock in MHz (25 or 40)
> > > > > > > - */
> > > > > > > -u32 get_ref_clk(void);
> > > > > > > -
> > > > > > >   #endif /* __ASSEMBLY__ */
> > > > > > >   #endif /* _MVEBU_CPU_H */
> > > > > > > diff --git a/arch/arm/mach-mvebu/include/mach/soc.h 
> > > > > > > b/arch/arm/mach-mvebu/include/mach/soc.h
> > > > > > > index aab61f7c15cf..b03b6de3c6cd 100644
> > > > > > > --- a/arch/arm/mach-mvebu/include/mach/soc.h
> > > > > > > +++ b/arch/arm/mach-mvebu/include/mach/soc.h
> > > > > > > @@ -210,6 +210,12 @@
> > > > > > >   #define BOOT_FROM_SPI           0x3
> > > > > > >   #define CONFIG_SYS_TCLK         250000000       /* 250MHz */
> > > > > > > +#elif defined(CONFIG_ARMADA_3700)
> > > > > > > +/* SAR values for Armada 3700 */
> > > > > > > +#define MVEBU_TEST_PIN_LATCH_N   MVEBU_REGISTER(0x13808)
> > > > > > > +#define MVEBU_XTAL_MODE_MASK     BIT(9)
> > > > > > > +#define CONFIG_SYS_REF_CLK       ((readl(MVEBU_TEST_PIN_LATCH_N) 
> > > > > > > & MVEBU_XTAL_MODE_MASK) ? \
> > > > > > > +                          40000000 : 25000000)
> > > > > 
> > > > > NAK.  CONFIG_xxx which evaluate out to a macro / function are the
> > > > > hardest to convert to Kconfig.  This patch is taking a step backwards.
> > > > > In fact, wait, how does patch apply and work?  There are no
> > > > > CONFIG_SYS_REF_CLK instances today, so the build should blow up about
> > > > > adding a new non-Kconfig symbol.
> > > > 
> > > > So, could you please provide some other solution for this issue which
> > > > Marek and Stefan pointed?
> > > 
> > > I don't know what the issue is, sorry.  But you cannot do what you're
> > > doing there with CONFIG.  If for some reason you cannot use an inline
> > > function, just don't name it CONFIG_SYS_REF_CLK.
> > 
> > Inline function is possible.
> 
> Then that please.
> 
> > > But also, really, did
> > > your build not fail when you tried to do this?  It really should have
> > > failed and told you to not add new CONFIG symbols.
> > 
> > There were no build issues, and built binary worked fine without any
> > issue.
> 
> Ugh.  I can reproduce that failure of the check here as well.  Time to
> go see what's going on there.

Ah, common.h including a whole lot less means that some uncommon
headers, such as <mach/soc.h> might slip through the checks.  Sigh, one
more reason everything needs to get migrated.

-- 
Tom

Attachment: signature.asc
Description: PGP signature

Reply via email to