* Re: [PATCH v5 09/11] clk: mediatek: add new clkmux register API
From: Owen Chen @ 2018-08-13 9:09 UTC (permalink / raw)
To: Sean Wang
Cc: Mars Cheng, Matthias Brugger, Rob Herring, Marc Zyngier,
Ryder Lee, Stephen Boyd, CC Hwang, Loda Chou, linux-kernel,
linux-mediatek, devicetree, wsd_upstream, linux-serial,
linux-arm-kernel, linux-clk
In-Reply-To: <1531983463.8953.235.camel@mtkswgap22>
On Thu, 2018-07-19 at 14:57 +0800, Sean Wang wrote:
> On Tue, 2018-07-17 at 16:52 +0800, Mars Cheng wrote:
> > From: Owen Chen <owen.chen@mediatek.com>
> >
> > MT6765 add "set/clr" register for each clkmux setting, and
> > one update register to trigger value change. It is designed
> > to prevent read-modify-write racing issue. The sw design
> > need to add a new API to handle this hw change with a new
> > mtk_clk_mux/mtk_clk_upd struct in new file "clk-mux"and
> > clk-upd".
> >
>
> I don't see any word mtk_clk_upd or clk-upd in the patch
>
> and the patch needs to be split into more patches
>
clk-upd is old description, no more clk-upd for handling update bit.
I will remove it next version.
> > Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> > ---
> > drivers/clk/mediatek/Makefile | 2 +-
> > drivers/clk/mediatek/clk-mtk.c | 41 +++++++
> > drivers/clk/mediatek/clk-mtk.h | 85 ++++++++++++---
> > drivers/clk/mediatek/clk-mux.c | 236 ++++++++++++++++++++++++++++++++++++++++
> > drivers/clk/mediatek/clk-mux.h | 38 +++++++
> > 5 files changed, 388 insertions(+), 14 deletions(-)
> > create mode 100644 drivers/clk/mediatek/clk-mux.c
> > create mode 100644 drivers/clk/mediatek/clk-mux.h
> >
> > diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> > index 844b55d..b97980d 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -1,5 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o
> > +obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o
> > obj-$(CONFIG_COMMON_CLK_MT6797) += clk-mt6797.o
> > obj-$(CONFIG_COMMON_CLK_MT6797_IMGSYS) += clk-mt6797-img.o
> > obj-$(CONFIG_COMMON_CLK_MT6797_MMSYS) += clk-mt6797-mm.o
> > diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> > index 9c0ae42..50becd0 100644
> > --- a/drivers/clk/mediatek/clk-mtk.c
> > +++ b/drivers/clk/mediatek/clk-mtk.c
> > @@ -22,6 +22,7 @@
> > #include <linux/mfd/syscon.h>
> >
> > #include "clk-mtk.h"
> > +#include "clk-mux.h"
> > #include "clk-gate.h"
> >
> > struct clk_onecell_data *mtk_alloc_clk_data(unsigned int clk_num)
> > @@ -144,6 +145,46 @@ int mtk_clk_register_gates(struct device_node *node,
> > return 0;
> > }
> >
> > +int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> > + int num, struct device_node *node,
> > + spinlock_t *lock,
> > + struct clk_onecell_data *clk_data)
> > +{
> > + struct regmap *regmap;
> > + struct clk *clk;
> > + int i;
> > +
> > + if (!clk_data)
> > + return -ENOMEM;
> > +
>
> general register function is able to handle that there is no clk_data.
> It looks like a optional, not a mandatory
>
clk_data is reused in topcksys clk registration, because of the
registration include muxes/gates/dividers/fixed_clks, so we need to make
sure clk_data is not NULL pointer so we can use to store the clk
structure we allocated.
> > + regmap = syscon_node_to_regmap(node);
> > + if (IS_ERR(regmap)) {
> > + pr_err("Cannot find regmap for %pOF: %ld\n", node,
> > + PTR_ERR(regmap));
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + for (i = 0; i < num; i++) {
> > + const struct mtk_mux *mux = &muxes[i];
> > +
> > + if (clk_data && !IS_ERR_OR_NULL(clk_data->clks[mux->id]))
> > + continue;
> > +
>
> it seems not necessary to check clk data every time
>
> and always use positive check is good to read
>
Yes, we will remove clk_data check at this point since not necessary.
the if condition check would alter next version.
> > + clk = mtk_clk_register_mux(mux, regmap, lock);
> > +
> > + if (IS_ERR(clk)) {
> > + pr_err("Failed to register clk %s: %ld\n",
> > + mux->name, PTR_ERR(clk));
> > + continue;
> > + }
> > +
> > + if (clk_data)
> > + clk_data->clks[mux->id] = clk;
>
> don't alter any data from input, that is a surprise for users
>
As I mentioned on previous question, clk_data need to be alterd because
of after registration done, we need to offer clk_data as input of
of_clk_add_provider(...,clk_data)
> > + }
> > +
> > + return 0;
> > +}
> > +
> > struct clk *mtk_clk_register_composite(const struct mtk_composite *mc,
> > void __iomem *base, spinlock_t *lock)
> > {
> > diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> > index 1882221..61693f6 100644
> > --- a/drivers/clk/mediatek/clk-mtk.h
> > +++ b/drivers/clk/mediatek/clk-mtk.h
> > @@ -24,7 +24,9 @@
> >
> > #define MAX_MUX_GATE_BIT 31
> > #define INVALID_MUX_GATE_BIT (MAX_MUX_GATE_BIT + 1)
> > -
> > +#define INVALID_OFS -1
> > +#define INVALID_SHFT -1
> > +#define INVALID_WIDTH -1
> > #define MHZ (1000 * 1000)
> >
> > struct mtk_fixed_clk {
> > @@ -84,10 +86,72 @@ struct mtk_composite {
> > signed char num_parents;
> > };
> >
> > +struct mtk_mux {
> > + int id;
> > + const char *name;
> > + const char * const *parent_names;
> > + unsigned int flags;
> > +
> > + u32 mux_ofs;
> > + u32 set_ofs;
> > + u32 clr_ofs;
> > + u32 upd_ofs;
> > +
> > + signed char mux_shift;
> > + signed char mux_width;
> > + signed char gate_shift;
> > + signed char upd_shift;
> > +
> > + const struct clk_ops *ops;
> > +
> > + signed char num_parents;
> > +};
> > +
>
> you have created a mtk-mux.h, why is you don't move the newly create
> struct in?
>
Okay, I would remove mtk_mux structure and only preserve mtk_clk_mux in
clk_mux.h.
> > /*
> > * In case the rate change propagation to parent clocks is undesirable,
> > * this macro allows to specify the clock flags manually.
> > */
> > +#define CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, _mux_set_ofs,\
> > + _mux_clr_ofs, _shift, _width, _gate, \
> > + _upd_ofs, _upd, _flags, _ops) { \
> > + .id = _id, \
> > + .name = _name, \
> > + .mux_ofs = _mux_ofs, \
> > + .set_ofs = _mux_set_ofs, \
> > + .clr_ofs = _mux_clr_ofs, \
> > + .upd_ofs = _upd_ofs, \
> > + .mux_shift = _shift, \
> > + .mux_width = _width, \
> > + .gate_shift = _gate, \
> > + .upd_shift = _upd, \
> > + .parent_names = _parents, \
> > + .num_parents = ARRAY_SIZE(_parents), \
> > + .flags = _flags, \
> > + .ops = &_ops, \
> > + }
> > +
> > +#define MUX_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, _mux_set_ofs,\
> > + _mux_clr_ofs, _shift, _width, _gate, \
> > + _upd_ofs, _upd, _flags) \
> > + CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, _flags, \
> > + mtk_mux_clr_set_upd_ops)
> > +
> > +#define MUX_CLR_SET_UPD(_id, _name, _parents, _mux_ofs, _mux_set_ofs, \
> > + _mux_clr_ofs, _shift, _width, _gate, \
> > + _upd_ofs, _upd) \
> > + MUX_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> > + _gate, _upd_ofs, _upd, CLK_SET_RATE_PARENT)
> > +
> > +#define MUX_UPD(_id, _name, _parents, _mux_ofs, _shift, _width, _gate, \
> > + _upd_ofs, _upd) \
> > + CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> > + INVALID_OFS, INVALID_OFS, _shift, _width, \
> > + _gate, _upd_ofs, _upd, CLK_SET_RATE_PARENT, \
> > + mtk_mux_upd_ops)
> > +
> > #define MUX_GATE_FLAGS(_id, _name, _parents, _reg, _shift, _width, \
> > _gate, _flags) { \
> > .id = _id, \
> > @@ -111,18 +175,8 @@ struct mtk_composite {
> > MUX_GATE_FLAGS(_id, _name, _parents, _reg, _shift, _width, \
> > _gate, CLK_SET_RATE_PARENT)
> >
> > -#define MUX(_id, _name, _parents, _reg, _shift, _width) { \
> > - .id = _id, \
> > - .name = _name, \
> > - .mux_reg = _reg, \
> > - .mux_shift = _shift, \
> > - .mux_width = _width, \
> > - .gate_shift = -1, \
> > - .divider_shift = -1, \
> > - .parent_names = _parents, \
> > - .num_parents = ARRAY_SIZE(_parents), \
> > - .flags = CLK_SET_RATE_PARENT, \
> > - }
>
> As Matthias always said that, you alter the common thing that is already
> used by a lot of SoC.
>
> You should have a dedicate patch to state why you need it, provide the
> way you propose. and use another patches for migrating old SoC, finally
> then add the patch for your own SoC.
>
> Don't mix everything in a single patch. The patch can't become reusable
> hard to read it, even hard to backport to the other SoC picking up
> patches they are really wanting.
>
Okay, I will restore it to original version.
> > +#define MUX(_id, _name, _parents, _reg, _shift, _width) \
> > + MUX_GATE(_id, _name, _parents, _reg, _shift, _width, INVALID_SHFT)
> >
> > #define DIV_GATE(_id, _name, _parent, _gate_reg, _gate_shift, _div_reg, \
> > _div_width, _div_shift) { \
> > @@ -138,6 +192,11 @@ struct mtk_composite {
> > .flags = 0, \
> > }
> >
> > +int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> > + int num, struct device_node *node,
> > + spinlock_t *lock,
> > + struct clk_onecell_data *clk_data);
> > +
>
> move to mtk-mux.h
>
Again, we can observe that other(gate/divider/fixed_clk) registration
functions all gather together in clk_mtk.h, if we want to move mtk_mux
to clk_mux.h, I think we may start a new patch to move all other three
registration functions and distribute it to their own driver code such
as clk_gate.h/clk_divider.h...etc.
> > struct clk *mtk_clk_register_composite(const struct mtk_composite *mc,
> > void __iomem *base, spinlock_t *lock);
> >
> > diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> > new file mode 100644
> > index 0000000..219181b
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mux.c
> > @@ -0,0 +1,236 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <owen.chen@mediatek.com>
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/slab.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-mux.h"
> > +
> > +static inline struct mtk_clk_mux
> > + *to_mtk_clk_mux(struct clk_hw *hw)
> > +{
> > + return container_of(hw, struct mtk_clk_mux, hw);
> > +}
> > +
> > +static int mtk_mux_enable(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = BIT(mux->gate_shift);
> > + unsigned long flags = 0;
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > +
>
> we can see many functions in kernel, similar to your need, they always
> be defined as two versions. for example.
>
> mtk_mux_enable refer to lock version
>
> mtk_mux_enable_nolock for lock-free version
>
> that makes less condition, more readable, and users don't care much
> about what stuff is put inside
>
clk.c already provides spin_lock at entrance of clk_enable/clk_disalbe
API, so the lock version seems redundant here, I would remove the lock
version and keep this API lock-free as my next modification.
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, 0);
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > + return 0;
> > +}
> > +
> > +static void mtk_mux_disable(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = BIT(mux->gate_shift);
> > + unsigned long flags = 0;
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > +
>
> ditto
>
I would remove lock next modification.
> > + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, mask);
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > +}
> > +
> > +static int mtk_mux_enable_setclr(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val;
> > + unsigned long flags = 0;
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > +
>
> ditto
>
I would remove lock next modification.
> > + val = BIT(mux->gate_shift);
> > + regmap_write(mux->regmap, mux->mux_clr_ofs, val);
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > + return 0;
> > +}
> > +
> > +static void mtk_mux_disable_setclr(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val;
> > + unsigned long flags = 0;
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > +
>
> ditto
>
I would remove lock next modification.
> > + val = BIT(mux->gate_shift);
> > + regmap_write(mux->regmap, mux->mux_set_ofs, val);
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > +}
> > +
> > +static int mtk_mux_is_enabled(struct clk_hw *hw)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 val = 0;
> > +
> > + if (mux->gate_shift < 0)
> > + return true;
> > +
>
> return value should be bool
>
due to the proto type of is_enabled function is defined to return
integer. I will alter the return value to 1 or 0.
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > +
> > + return (val & BIT(mux->gate_shift)) == 0;
> > +}
> > +
> > +static u8 mtk_mux_get_parent(struct clk_hw *hw)
> > +{
>
>
> return value should be int
Okay, I would alter it next version.
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + int num_parents = clk_hw_get_num_parents(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val;
> > +
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + val = (val >> mux->mux_shift) & mask;
> > +
> > + if (val >= num_parents)
> > + return -EINVAL;
> > +
> > + return val;
> > +}
> > +
> > +static int mtk_mux_set_parent(struct clk_hw *hw, u8 index)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val, orig;
> > + unsigned long flags = 0;
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > +
>
> use lock-free-or-not funciton
>
I would remove lock next modification.
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + orig = val;
> > + val &= ~(mask << mux->mux_shift);
> > + val |= index << mux->mux_shift;
> > +
> > + if (val != orig) {
> > + regmap_write(mux->regmap, mux->mux_ofs, val);
> > +
> > + if (mux->upd_shift >= 0)
> > + regmap_write(mux->regmap, mux->upd_ofs,
> > + BIT(mux->upd_shift));
>
>
> why not use regmap_update_bits like function ?
>
Okay, I would alter it next version..
> > + }
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_mux_set_parent_setclr(struct clk_hw *hw, u8 index)
> > +{
> > + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> > + u32 mask = GENMASK(mux->mux_width - 1, 0);
> > + u32 val, orig;
> > + unsigned long flags = 0;
> > +
> > + if (mux->lock)
> > + spin_lock_irqsave(mux->lock, flags);
> > +
>
> use lock-free-or-not funciton
>
I would remove lock next modification.
> > + regmap_read(mux->regmap, mux->mux_ofs, &val);
> > + orig = val;
> > + val &= ~(mask << mux->mux_shift);
> > + val |= index << mux->mux_shift;
> > +
> > + if (val != orig) {
> > + val = (mask << mux->mux_shift);
> > + regmap_write(mux->regmap, mux->mux_clr_ofs, val);
> > + val = (index << mux->mux_shift);
> > + regmap_write(mux->regmap, mux->mux_set_ofs, val);
> > +
>
> why not use regmap_update_bits like function ?
This function specified that mux use set/clr register to update, so it
would be better to use regmap_write to write the whole register instead
of regmap_update_bit.
> > + if (mux->upd_shift >= 0)
> > + regmap_write(mux->regmap, mux->upd_ofs,
> > + BIT(mux->upd_shift));
> > + }
> > +
> > + if (mux->lock)
> > + spin_unlock_irqrestore(mux->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +const struct clk_ops mtk_mux_upd_ops = {
> > + .enable = mtk_mux_enable,
> > + .disable = mtk_mux_disable,
> > + .is_enabled = mtk_mux_is_enabled,
> > + .get_parent = mtk_mux_get_parent,
> > + .set_parent = mtk_mux_set_parent,
> > + .determine_rate = NULL,
>
> explicitly set as NULL can be removed
Okay, I would remove it next version.
> > +};
> > +
> > +const struct clk_ops mtk_mux_clr_set_upd_ops = {
> > + .enable = mtk_mux_enable_setclr,
> > + .disable = mtk_mux_disable_setclr,
> > + .is_enabled = mtk_mux_is_enabled,
> > + .get_parent = mtk_mux_get_parent,
> > + .set_parent = mtk_mux_set_parent_setclr,
> > + .determine_rate = NULL,
>
> explicitly set as NULL can be removed
Okay, I would remove it next version.
> > +};
> > +
> > +struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
> > + struct regmap *regmap,
> > + spinlock_t *lock)
> > +{
> > + struct clk *clk;
> > + struct clk_init_data init;
> > + struct mtk_clk_mux *mtk_mux = NULL;
> > + int ret;
> > +
> make declaration as reverse xmas tree
>
Okay, I would alter it next version.
> > + mtk_mux = kzalloc(sizeof(*mtk_mux), GFP_KERNEL);
> > + if (!mtk_mux)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + init.name = mux->name;
> > + init.flags = (mux->flags) | CLK_SET_RATE_PARENT;
> > + init.parent_names = mux->parent_names;
> > + init.num_parents = mux->num_parents;
> > + init.ops = mux->ops;
> > +
> > + mtk_mux->regmap = regmap;
> > + mtk_mux->name = mux->name;
> > + mtk_mux->mux_ofs = mux->mux_ofs;
> > + mtk_mux->mux_set_ofs = mux->set_ofs;
> > + mtk_mux->mux_clr_ofs = mux->clr_ofs;
> > + mtk_mux->upd_ofs = mux->upd_ofs;
> > + mtk_mux->mux_shift = mux->mux_shift;
> > + mtk_mux->mux_width = mux->mux_width;
> > + mtk_mux->gate_shift = mux->gate_shift;
> > + mtk_mux->upd_shift = mux->upd_shift;
> > +
> > + mtk_mux->lock = lock;
> > + mtk_mux->hw.init = &init;
> > +
> > + clk = clk_register(NULL, &mtk_mux->hw);
> > + if (IS_ERR(clk)) {
> > + ret = PTR_ERR(clk);
>
>
> ret is superfluous
>
Okay, I would remove it next version.
> > + goto err_out;
> > + }
> > +
> > + return clk;
> > +err_out:
> > + kfree(mtk_mux);
> > +
>
> I felt err path can be optimized
>
Okay, I would remove it next version.
> > + return ERR_PTR(ret);
> > +}
> > diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> > new file mode 100644
> > index 0000000..64f8e7c
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mux.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <owen.chen@mediatek.com>
> > + */
> > +
> > +#ifndef __DRV_CLK_MUX_H
> > +#define __DRV_CLK_MUX_H
> > +
> > +#include <linux/clk-provider.h>
> > +
> > +struct mtk_clk_mux {
> > + struct clk_hw hw;
> > + struct regmap *regmap;
> > +
> > + const char *name;
> > +
> > + int mux_set_ofs;
> > + int mux_clr_ofs;
> > + int mux_ofs;
> > + int upd_ofs;
> > +
> > + s8 mux_shift;
> > + s8 mux_width;
> > + s8 gate_shift;
> > + s8 upd_shift;
> > +
> > + spinlock_t *lock;
> > +};
> > +
> > +extern const struct clk_ops mtk_mux_upd_ops;
> > +extern const struct clk_ops mtk_mux_clr_set_upd_ops;
> > +
>
>
> extern is superfluous
>
Okay, I would remove it next version.
> > +struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
> > + struct regmap *regmap,
> > + spinlock_t *lock);
> > +
> > +#endif /* __DRV_CLK_MUX_H */
>
>
^ permalink raw reply
* [PATCH 5/5] serial: sprd: Fix the indentation issue
From: Baolin Wang @ 2018-08-11 1:34 UTC (permalink / raw)
To: gregkh, jslaby
Cc: orsonzhai, baolin.wang, zhang.lyra, broonie, linux-serial,
linux-kernel
In-Reply-To: <50e1f7f4673bff9d71936b4eb34cbf2571b0fda9.1533950271.git.baolin.wang@linaro.org>
Make the macros' definition and code have the same correct indentation.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/tty/serial/sprd_serial.c | 46 +++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 8d5c9cd..4287ca3 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -68,24 +68,24 @@
#define SPRD_LCR_DATA_LEN6 0x4
#define SPRD_LCR_DATA_LEN7 0x8
#define SPRD_LCR_DATA_LEN8 0xc
-#define SPRD_LCR_PARITY (BIT(0) | BIT(1))
+#define SPRD_LCR_PARITY (BIT(0) | BIT(1))
#define SPRD_LCR_PARITY_EN 0x2
#define SPRD_LCR_EVEN_PAR 0x0
#define SPRD_LCR_ODD_PAR 0x1
/* control register 1 */
-#define SPRD_CTL1 0x001C
+#define SPRD_CTL1 0x001C
#define RX_HW_FLOW_CTL_THLD BIT(6)
#define RX_HW_FLOW_CTL_EN BIT(7)
#define TX_HW_FLOW_CTL_EN BIT(8)
#define RX_TOUT_THLD_DEF 0x3E00
-#define RX_HFC_THLD_DEF 0x40
+#define RX_HFC_THLD_DEF 0x40
/* fifo threshold register */
#define SPRD_CTL2 0x0020
-#define THLD_TX_EMPTY 0x40
+#define THLD_TX_EMPTY 0x40
#define THLD_TX_EMPTY_SHIFT 8
-#define THLD_RX_FULL 0x40
+#define THLD_RX_FULL 0x40
/* config baud rate register */
#define SPRD_CLKD0 0x0024
@@ -95,11 +95,11 @@
#define SPRD_CLKD1_SHIFT 16
/* interrupt mask status register */
-#define SPRD_IMSR 0x002C
-#define SPRD_IMSR_RX_FIFO_FULL BIT(0)
+#define SPRD_IMSR 0x002C
+#define SPRD_IMSR_RX_FIFO_FULL BIT(0)
#define SPRD_IMSR_TX_FIFO_EMPTY BIT(1)
-#define SPRD_IMSR_BREAK_DETECT BIT(7)
-#define SPRD_IMSR_TIMEOUT BIT(13)
+#define SPRD_IMSR_BREAK_DETECT BIT(7)
+#define SPRD_IMSR_TIMEOUT BIT(13)
struct sprd_uart_port {
struct uart_port port;
@@ -229,7 +229,7 @@ static inline void sprd_rx(struct uart_port *port)
port->icount.rx++;
if (lsr & (SPRD_LSR_BI | SPRD_LSR_PE |
- SPRD_LSR_FE | SPRD_LSR_OE))
+ SPRD_LSR_FE | SPRD_LSR_OE))
if (handle_lsr_errors(port, &lsr, &flag))
continue;
if (uart_handle_sysrq_char(port, ch))
@@ -292,8 +292,8 @@ static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
if (ims & SPRD_IMSR_TIMEOUT)
serial_out(port, SPRD_ICLR, SPRD_ICLR_TIMEOUT);
- if (ims & (SPRD_IMSR_RX_FIFO_FULL |
- SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT))
+ if (ims & (SPRD_IMSR_RX_FIFO_FULL | SPRD_IMSR_BREAK_DETECT |
+ SPRD_IMSR_TIMEOUT))
sprd_rx(port);
if (ims & SPRD_IMSR_TX_FIFO_EMPTY)
@@ -333,7 +333,7 @@ static int sprd_startup(struct uart_port *port)
sp = container_of(port, struct sprd_uart_port, port);
snprintf(sp->name, sizeof(sp->name), "sprd_serial%d", port->line);
ret = devm_request_irq(port->dev, port->irq, sprd_handle_irq,
- IRQF_SHARED, sp->name, port);
+ IRQF_SHARED, sp->name, port);
if (ret) {
dev_err(port->dev, "fail to request serial irq %d, ret=%d\n",
port->irq, ret);
@@ -361,8 +361,8 @@ static void sprd_shutdown(struct uart_port *port)
}
static void sprd_set_termios(struct uart_port *port,
- struct ktermios *termios,
- struct ktermios *old)
+ struct ktermios *termios,
+ struct ktermios *old)
{
unsigned int baud, quot;
unsigned int lcr = 0, fc;
@@ -480,8 +480,7 @@ static void sprd_config_port(struct uart_port *port, int flags)
port->type = PORT_SPRD;
}
-static int sprd_verify_port(struct uart_port *port,
- struct serial_struct *ser)
+static int sprd_verify_port(struct uart_port *port, struct serial_struct *ser)
{
if (ser->type != PORT_SPRD)
return -EINVAL;
@@ -531,7 +530,7 @@ static void sprd_console_putchar(struct uart_port *port, int ch)
}
static void sprd_console_write(struct console *co, const char *s,
- unsigned int count)
+ unsigned int count)
{
struct uart_port *port = &sprd_port[co->index]->port;
int locked = 1;
@@ -594,7 +593,7 @@ static void sprd_putc(struct uart_port *port, int c)
unsigned int timeout = SPRD_TIMEOUT;
while (timeout-- &&
- !(readl(port->membase + SPRD_LSR) & SPRD_LSR_TX_OVER))
+ !(readl(port->membase + SPRD_LSR) & SPRD_LSR_TX_OVER))
cpu_relax();
writeb(c, port->membase + SPRD_TXD);
@@ -607,9 +606,8 @@ static void sprd_early_write(struct console *con, const char *s, unsigned int n)
uart_console_write(&dev->port, s, n, sprd_putc);
}
-static int __init sprd_early_console_setup(
- struct earlycon_device *device,
- const char *opt)
+static int __init sprd_early_console_setup(struct earlycon_device *device,
+ const char *opt)
{
if (!device->port.membase)
return -ENODEV;
@@ -691,8 +689,8 @@ static int sprd_probe(struct platform_device *pdev)
index = sprd_probe_dt_alias(index, &pdev->dev);
- sprd_port[index] = devm_kzalloc(&pdev->dev,
- sizeof(*sprd_port[index]), GFP_KERNEL);
+ sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
+ GFP_KERNEL);
if (!sprd_port[index])
return -ENOMEM;
--
1.9.1
^ permalink raw reply related
* [PATCH 4/5] serial: sprd: Change 'int' to 'unsigned int'
From: Baolin Wang @ 2018-08-11 1:34 UTC (permalink / raw)
To: gregkh, jslaby
Cc: orsonzhai, baolin.wang, zhang.lyra, broonie, linux-serial,
linux-kernel
In-Reply-To: <50e1f7f4673bff9d71936b4eb34cbf2571b0fda9.1533950271.git.baolin.wang@linaro.org>
The register offset value should be 'unsigned int' type.
Moreover, prefer 'unsigned int' to bare use of 'unsigned'.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/tty/serial/sprd_serial.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 03b0cd4..8d5c9cd 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -109,12 +109,14 @@ struct sprd_uart_port {
static struct sprd_uart_port *sprd_port[UART_NR_MAX];
static int sprd_ports_num;
-static inline unsigned int serial_in(struct uart_port *port, int offset)
+static inline unsigned int serial_in(struct uart_port *port,
+ unsigned int offset)
{
return readl_relaxed(port->membase + offset);
}
-static inline void serial_out(struct uart_port *port, int offset, int value)
+static inline void serial_out(struct uart_port *port, unsigned int offset,
+ int value)
{
writel_relaxed(value, port->membase + offset);
}
@@ -598,8 +600,7 @@ static void sprd_putc(struct uart_port *port, int c)
writeb(c, port->membase + SPRD_TXD);
}
-static void sprd_early_write(struct console *con, const char *s,
- unsigned n)
+static void sprd_early_write(struct console *con, const char *s, unsigned int n)
{
struct earlycon_device *dev = con->data;
--
1.9.1
^ permalink raw reply related
* [PATCH 3/5] serial: sprd: Remove unnecessary resource validation
From: Baolin Wang @ 2018-08-11 1:34 UTC (permalink / raw)
To: gregkh, jslaby
Cc: orsonzhai, baolin.wang, zhang.lyra, broonie, linux-serial,
linux-kernel
In-Reply-To: <50e1f7f4673bff9d71936b4eb34cbf2571b0fda9.1533950271.git.baolin.wang@linaro.org>
The devm_ioremap_resource() will valid the resources, thus remove the
unnecessary resource validation in the driver.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/tty/serial/sprd_serial.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index e18d8af..03b0cd4 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -710,15 +710,12 @@ static int sprd_probe(struct platform_device *pdev)
up->uartclk = clk_get_rate(clk);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- dev_err(&pdev->dev, "not provide mem resource\n");
- return -ENODEV;
- }
- up->mapbase = res->start;
up->membase = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(up->membase))
return PTR_ERR(up->membase);
+ up->mapbase = res->start;
+
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
dev_err(&pdev->dev, "not provide irq resource: %d\n", irq);
--
1.9.1
^ permalink raw reply related
* [PATCH 2/5] serial: sprd: Use readable macros instead of magic number
From: Baolin Wang @ 2018-08-11 1:34 UTC (permalink / raw)
To: gregkh, jslaby
Cc: orsonzhai, baolin.wang, zhang.lyra, broonie, linux-serial,
linux-kernel
In-Reply-To: <50e1f7f4673bff9d71936b4eb34cbf2571b0fda9.1533950271.git.baolin.wang@linaro.org>
Define readable macros instead of magic number to make code more readable.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/tty/serial/sprd_serial.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 1b0e3fb..e18d8af 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -45,6 +45,8 @@
/* data number in TX and RX fifo */
#define SPRD_STS1 0x000C
+#define SPRD_RX_FIFO_CNT_MASK GENMASK(7, 0)
+#define SPRD_TX_FIFO_CNT_MASK GENMASK(15, 8)
/* interrupt enable register and its BITs */
#define SPRD_IEN 0x0010
@@ -82,11 +84,15 @@
/* fifo threshold register */
#define SPRD_CTL2 0x0020
#define THLD_TX_EMPTY 0x40
+#define THLD_TX_EMPTY_SHIFT 8
#define THLD_RX_FULL 0x40
/* config baud rate register */
#define SPRD_CLKD0 0x0024
+#define SPRD_CLKD0_MASK GENMASK(15, 0)
#define SPRD_CLKD1 0x0028
+#define SPRD_CLKD1_MASK GENMASK(20, 16)
+#define SPRD_CLKD1_SHIFT 16
/* interrupt mask status register */
#define SPRD_IMSR 0x002C
@@ -115,7 +121,7 @@ static inline void serial_out(struct uart_port *port, int offset, int value)
static unsigned int sprd_tx_empty(struct uart_port *port)
{
- if (serial_in(port, SPRD_STS1) & 0xff00)
+ if (serial_in(port, SPRD_STS1) & SPRD_TX_FIFO_CNT_MASK)
return 0;
else
return TIOCSER_TEMT;
@@ -213,7 +219,8 @@ static inline void sprd_rx(struct uart_port *port)
struct tty_port *tty = &port->state->port;
unsigned int ch, flag, lsr, max_count = SPRD_TIMEOUT;
- while ((serial_in(port, SPRD_STS1) & 0x00ff) && max_count--) {
+ while ((serial_in(port, SPRD_STS1) & SPRD_RX_FIFO_CNT_MASK) &&
+ max_count--) {
lsr = serial_in(port, SPRD_LSR);
ch = serial_in(port, SPRD_RXD);
flag = TTY_NORMAL;
@@ -303,16 +310,17 @@ static int sprd_startup(struct uart_port *port)
struct sprd_uart_port *sp;
unsigned long flags;
- serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
+ serial_out(port, SPRD_CTL2,
+ THLD_TX_EMPTY << THLD_TX_EMPTY_SHIFT | THLD_RX_FULL);
/* clear rx fifo */
timeout = SPRD_TIMEOUT;
- while (timeout-- && serial_in(port, SPRD_STS1) & 0x00ff)
+ while (timeout-- && serial_in(port, SPRD_STS1) & SPRD_RX_FIFO_CNT_MASK)
serial_in(port, SPRD_RXD);
/* clear tx fifo */
timeout = SPRD_TIMEOUT;
- while (timeout-- && serial_in(port, SPRD_STS1) & 0xff00)
+ while (timeout-- && serial_in(port, SPRD_STS1) & SPRD_TX_FIFO_CNT_MASK)
cpu_relax();
/* clear interrupt */
@@ -433,10 +441,11 @@ static void sprd_set_termios(struct uart_port *port,
}
/* clock divider bit0~bit15 */
- serial_out(port, SPRD_CLKD0, quot & 0xffff);
+ serial_out(port, SPRD_CLKD0, quot & SPRD_CLKD0_MASK);
/* clock divider bit16~bit20 */
- serial_out(port, SPRD_CLKD1, (quot & 0x1f0000) >> 16);
+ serial_out(port, SPRD_CLKD1,
+ (quot & SPRD_CLKD1_MASK) >> SPRD_CLKD1_SHIFT);
serial_out(port, SPRD_LCR, lcr);
fc |= RX_TOUT_THLD_DEF | RX_HFC_THLD_DEF;
serial_out(port, SPRD_CTL1, fc);
@@ -510,7 +519,7 @@ static void wait_for_xmitr(struct uart_port *port)
if (--tmout == 0)
break;
udelay(1);
- } while (status & 0xff00);
+ } while (status & SPRD_TX_FIFO_CNT_MASK);
}
static void sprd_console_putchar(struct uart_port *port, int ch)
--
1.9.1
^ permalink raw reply related
* [PATCH 1/5] serial: sprd: Remove unused structure
From: Baolin Wang @ 2018-08-11 1:34 UTC (permalink / raw)
To: gregkh, jslaby
Cc: orsonzhai, baolin.wang, zhang.lyra, broonie, linux-serial,
linux-kernel
Remove the unused reg_backup structure.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/tty/serial/sprd_serial.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 828f114..1b0e3fb 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -95,19 +95,8 @@
#define SPRD_IMSR_BREAK_DETECT BIT(7)
#define SPRD_IMSR_TIMEOUT BIT(13)
-struct reg_backup {
- u32 ien;
- u32 ctrl0;
- u32 ctrl1;
- u32 ctrl2;
- u32 clkd0;
- u32 clkd1;
- u32 dspwait;
-};
-
struct sprd_uart_port {
struct uart_port port;
- struct reg_backup reg_bak;
char name[16];
};
--
1.9.1
^ permalink raw reply related
* Re: [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports
From: Rob Herring @ 2018-08-10 18:11 UTC (permalink / raw)
To: Andreas Färber
Cc: open list:SERIAL DRIVERS, Linux-MIPS, jringle, Michael Allwright,
Jakub Kicinski, liuxuenetmail, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel@vger.kernel.org
In-Reply-To: <0aa77961-fe60-6afe-e6c5-d2db4250cb22@suse.de>
On Fri, Aug 10, 2018 at 11:45 AM Andreas Färber <afaerber@suse.de> wrote:
>
> Am 10.08.2018 um 19:34 schrieb Rob Herring:
> > On Sun, Aug 5, 2018 at 5:27 PM Andreas Färber <afaerber@suse.de> wrote:
> >>
> >> This is to allow using serdev.
> >>
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> ---
> >> drivers/tty/serial/sc16is7xx.c | 25 +++++++++++++++++++++++++
> >> 1 file changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> >> index 243c96025053..ad7267274f65 100644
> >> --- a/drivers/tty/serial/sc16is7xx.c
> >> +++ b/drivers/tty/serial/sc16is7xx.c
> >> @@ -1213,9 +1213,31 @@ static int sc16is7xx_probe(struct device *dev,
> >> SC16IS7XX_IOCONTROL_SRESET_BIT);
> >>
> >> for (i = 0; i < devtype->nr_uart; ++i) {
> >> +#ifdef CONFIG_OF
> >> + struct device_node *np;
> >> + struct platform_device *pdev;
> >> + char name[6] = "uartx";
> >> +#endif
> >> +
> >> s->p[i].line = i;
> >> /* Initialize port data */
> >> +#ifdef CONFIG_OF
> >> + name[4] = '0' + i;
> >> + np = of_get_child_by_name(dev->of_node, name);
> >> + if (IS_ERR(np)) {
> >> + ret = PTR_ERR(np);
> >> + goto out_ports;
> >> + }
> >> + pdev = of_platform_device_create(np, NULL, dev);
> >
> > Ideally, you would use of_platform_default_populate here. I think
> > you'd have to add a compatible to the child nodes, but that wouldn't
> > be a bad thing. I could envision that the child nodes ultimately
> > become their own driver utilizing the standard 8250 driver and a
> > compatible string would be needed in that case.
>
> Separate compatibles would mean separate drivers.
No. Having a compatible doesn't mean you have to have a driver.
> Unlike your DUART example this is not an MMIO device that we can easily
> split but a SPI slave (well, regmap due to some I2C models).
A SPI slave could provide a regmap, right?
> I don't see how separate drivers could work, given that the whole
> spi_device has a single interrupt for all functions of this device.
A shared interrupt or a parent driver that creates an irqchip like MFD
drivers often do.
>
> That left me with this ugly but working construct.
In any case, I'm was suggesting that you do any of this now. I just
want the binding to be designed to work either way.
> Is the uartX naming correct, or should it be serialX?
Ah, yes. Should be serial@... I'm fine if both the parent and child
are named serial@...
Rob
^ permalink raw reply
* Re: [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports
From: Andreas Färber @ 2018-08-10 17:45 UTC (permalink / raw)
To: Rob Herring
Cc: SERIAL DRIVERS, Linux-MIPS, jringle, Michael Allwright,
Jakub Kicinski, liuxuenetmail, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel@vger.kernel.org
In-Reply-To: <CAL_Jsq+f5VMWZg9GNF=e-UmFjcjbVnE7Dr0EBF56E6gUrdTnuQ@mail.gmail.com>
Am 10.08.2018 um 19:34 schrieb Rob Herring:
> On Sun, Aug 5, 2018 at 5:27 PM Andreas Färber <afaerber@suse.de> wrote:
>>
>> This is to allow using serdev.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>> drivers/tty/serial/sc16is7xx.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
>> index 243c96025053..ad7267274f65 100644
>> --- a/drivers/tty/serial/sc16is7xx.c
>> +++ b/drivers/tty/serial/sc16is7xx.c
>> @@ -1213,9 +1213,31 @@ static int sc16is7xx_probe(struct device *dev,
>> SC16IS7XX_IOCONTROL_SRESET_BIT);
>>
>> for (i = 0; i < devtype->nr_uart; ++i) {
>> +#ifdef CONFIG_OF
>> + struct device_node *np;
>> + struct platform_device *pdev;
>> + char name[6] = "uartx";
>> +#endif
>> +
>> s->p[i].line = i;
>> /* Initialize port data */
>> +#ifdef CONFIG_OF
>> + name[4] = '0' + i;
>> + np = of_get_child_by_name(dev->of_node, name);
>> + if (IS_ERR(np)) {
>> + ret = PTR_ERR(np);
>> + goto out_ports;
>> + }
>> + pdev = of_platform_device_create(np, NULL, dev);
>
> Ideally, you would use of_platform_default_populate here. I think
> you'd have to add a compatible to the child nodes, but that wouldn't
> be a bad thing. I could envision that the child nodes ultimately
> become their own driver utilizing the standard 8250 driver and a
> compatible string would be needed in that case.
Separate compatibles would mean separate drivers.
Unlike your DUART example this is not an MMIO device that we can easily
split but a SPI slave (well, regmap due to some I2C models).
I don't see how separate drivers could work, given that the whole
spi_device has a single interrupt for all functions of this device.
That left me with this ugly but working construct.
Is the uartX naming correct, or should it be serialX?
Regards,
Andreas
>
> You'd then have to loop over each child of 'dev' instead of the DT nodes.
>
>> + if (IS_ERR(pdev)) {
>> + ret = PTR_ERR(pdev);
>> + goto out_ports;
>> + }
>> + platform_set_drvdata(pdev, dev_get_drvdata(dev));
>> + s->p[i].port.dev = &pdev->dev;
>> +#else
>> s->p[i].port.dev = dev;
>> +#endif
>> s->p[i].port.irq = irq;
>> s->p[i].port.type = PORT_SC16IS7XX;
>> s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports
From: Rob Herring @ 2018-08-10 17:34 UTC (permalink / raw)
To: Andreas Färber
Cc: open list:SERIAL DRIVERS, Linux-MIPS, jringle, Michael Allwright,
Jakub Kicinski, liuxuenetmail, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel@vger.kernel.org
In-Reply-To: <20180805232651.10605-1-afaerber@suse.de>
On Sun, Aug 5, 2018 at 5:27 PM Andreas Färber <afaerber@suse.de> wrote:
>
> This is to allow using serdev.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> drivers/tty/serial/sc16is7xx.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 243c96025053..ad7267274f65 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1213,9 +1213,31 @@ static int sc16is7xx_probe(struct device *dev,
> SC16IS7XX_IOCONTROL_SRESET_BIT);
>
> for (i = 0; i < devtype->nr_uart; ++i) {
> +#ifdef CONFIG_OF
> + struct device_node *np;
> + struct platform_device *pdev;
> + char name[6] = "uartx";
> +#endif
> +
> s->p[i].line = i;
> /* Initialize port data */
> +#ifdef CONFIG_OF
> + name[4] = '0' + i;
> + np = of_get_child_by_name(dev->of_node, name);
> + if (IS_ERR(np)) {
> + ret = PTR_ERR(np);
> + goto out_ports;
> + }
> + pdev = of_platform_device_create(np, NULL, dev);
Ideally, you would use of_platform_default_populate here. I think
you'd have to add a compatible to the child nodes, but that wouldn't
be a bad thing. I could envision that the child nodes ultimately
become their own driver utilizing the standard 8250 driver and a
compatible string would be needed in that case.
You'd then have to loop over each child of 'dev' instead of the DT nodes.
> + if (IS_ERR(pdev)) {
> + ret = PTR_ERR(pdev);
> + goto out_ports;
> + }
> + platform_set_drvdata(pdev, dev_get_drvdata(dev));
> + s->p[i].port.dev = &pdev->dev;
> +#else
> s->p[i].port.dev = dev;
> +#endif
> s->p[i].port.irq = irq;
> s->p[i].port.type = PORT_SC16IS7XX;
> s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
^ permalink raw reply
* Re: [PATCH v2 08/18] serial: intel: Get serial id from dts
From: Wu, Songjun @ 2018-08-10 8:13 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu,
Linux MIPS Mailing List, linux-clk, open list:SERIAL DRIVERS,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg KH, Linux Kernel Mailing List, Jiri Slaby
In-Reply-To: <CAMuHMdVgC__AJHCEKT9UhdK4Jh4bm-s0ihH8mvx7KQ7+vzh6Ag@mail.gmail.com>
On 8/8/2018 4:33 PM, Geert Uytterhoeven wrote:
> Hi Songjun,
>
> On Wed, Aug 8, 2018 at 6:05 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote:
>> On 8/7/2018 3:33 PM, Geert Uytterhoeven wrote:
>>> On Fri, Aug 3, 2018 at 5:04 AM Songjun Wu <songjun.wu@linux.intel.com> wrote:
>>>> Get serial id from dts.
>>>>
>>>> "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
>>>> macro is defined in lantiq_soc.h.
>>>> lantiq_soc.h is in arch path for legacy product support.
>>>>
>>>> arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
>>>>
>>>> If "#ifdef preprocessor" is changed to
>>>> "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
>>>> code using LTQ_EARLY_ASC is compiled.
>>>> Compilation will fail for no LTQ_EARLY_ASC defined.
>>>>
>>>> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
>>> Thanks for your patch!
>>>
>>>> @@ -699,9 +700,19 @@ lqasc_probe(struct platform_device *pdev)
>>>> return -ENODEV;
>>>> }
>>>>
>>>> - /* check if this is the console port */
>>>> - if (mmres->start != CPHYSADDR(LTQ_EARLY_ASC))
>>>> - line = 1;
>>>> + /* get serial id */
>>>> + line = of_alias_get_id(node, "serial");
>>>> + if (line < 0) {
>>>> +#ifdef CONFIG_LANTIQ
>>>> + if (mmres->start == CPHYSADDR(LTQ_EARLY_ASC))
>>>> + line = 0;
>>>> + else
>>>> + line = 1;
>>>> +#else
>>>> + dev_err(&pdev->dev, "failed to get alias id, errno %d\n", line);
>>>> + return line;
>>> Please note that not providing a fallback here makes life harder when using
>>> DT overlays.
>>> See the description of commit 7678f4c20fa7670f ("serial: sh-sci: Add support
>>> for dynamic instances") for background info.
>> Thanks for your comment.
>> The logic in commit 7678f4c20fa7670f is not suitable here.
>> We need to know which serial instance is used for console.
>> We cannot use dynamic serial instance here.
> Why does the driver need to use which serial instance is used for the console?
> Hardcoding that is not an option, as the board DTS may specify the console using
> chosen/stdout-path.
In legacy platform in open source, it only defined asc1 in dts.
There's no asc0 in legacy dts.While in the new platform, asc0
is defined in dts. There's no asc1 in new platform dts.
To avoid hard code in driver, alias serial0 is used to unified
driver code. Actually only one serial is supported in SoC.
aliases {
serial0 = &asc0;
};
chosen {
bootargs = "earlycon clk_ignore_unused";
stdout-path = "serial0";
};
^ permalink raw reply
* Re: [PATCH v3 2/2] tty/serial: atmel: add ISO7816 support
From: Ludovic Desroches @ 2018-08-09 11:30 UTC (permalink / raw)
To: Richard Genoud
Cc: linux-serial, linux-arch, linux-arm-kernel, alexandre.belloni,
arnd, gregkh, linux-kernel, jslaby
In-Reply-To: <d66c4318-1d31-5c4e-3952-6b69a010b028@sorico.fr>
Hi Richard,
On Thu, Aug 09, 2018 at 10:47:17AM +0200, Richard Genoud wrote:
> Hi !
>
> On 07/08/2018 15:00, Ludovic Desroches wrote:
> > From: Nicolas Ferre <nicolas.ferre@microchip.com>
> >
> > When mode is set in atmel_config_iso7816() we backup last RS232 mode
> > for coming back to this mode if requested.
> > Also allow setup of T=0 and T=1 parameter and basic support in set_termios
> > function as well.
> >
> > Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> > [ludovic.desroches@microchip.com: rebase, add check on fidi ratio, checkpatch fixes]
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> > ---
> > drivers/tty/serial/atmel_serial.c | 211 +++++++++++++++++++++++++++++++++++---
> > drivers/tty/serial/atmel_serial.h | 6 +-
> > 2 files changed, 201 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> > index 8e4428725848..4a7ec44b0ace 100644
> > --- a/drivers/tty/serial/atmel_serial.c
> > +++ b/drivers/tty/serial/atmel_serial.c
[...]
> > #if defined(CONFIG_OF)
> > +static struct atmel_uart_pdata at91rm9200_pdata = {
> > + .fidi_min = 1,
> > + .fidi_max = 2047,
> > +};
> > +
> > +static struct atmel_uart_pdata at91sam9260_pdata = {
> > + .fidi_min = 1,
> > + .fidi_max = 2047,
> > +};
> > +
> > +static struct atmel_uart_pdata sama5d3_pdata = {
> > + .fidi_min = 3,
> > + .fidi_max = 65535,
> Are you sure this is for sama5d3 ?
> From the datasheets I have, 65535 is for sama5d4/sama5d2
I checked it and I missed it. What a pity... In fact, it's a bit more
tricky since the min value for d3 is 3 and no longer 1.
> And also, you'll have to s/atmel,at91sam9260-usart/atmel,sama5d2-usart/g
> in sama5d{2,4}.dtsi
>
Yes, I planed to send it later but I can add those patches within this
set of patches.
> But I wonder if it could be detected via ATMEL_US_VERSION instead ?
>
I have not checked, I tend to prefer the compatible string for this kind
of thing. But as we already use the version number, I can investigate
this solution if it's the one you prefer.
Regards
Ludovic
^ permalink raw reply
* [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend
From: Anson Huang @ 2018-08-09 10:06 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial, linux-kernel; +Cc: Linux-imx
In-Reply-To: <1533809169-24133-1-git-send-email-Anson.Huang@nxp.com>
On some i.MX SoCs' low power mode, UART iomux settings
may be lost, need to add pinctrl sleep/default mode switch
during suspend/resume to make sure UART iomux settings are
correct after resume.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/tty/serial/imx.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index a09ccef..c280d43 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -24,6 +24,7 @@
#include <linux/serial.h>
#include <linux/clk.h>
#include <linux/delay.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/rational.h>
#include <linux/slab.h>
#include <linux/of.h>
@@ -2447,6 +2448,8 @@ static int imx_uart_suspend_noirq(struct device *dev)
clk_disable(sport->clk_ipg);
+ pinctrl_pm_select_sleep_state(dev);
+
return 0;
}
@@ -2455,6 +2458,8 @@ static int imx_uart_resume_noirq(struct device *dev)
struct imx_port *sport = dev_get_drvdata(dev);
int ret;
+ pinctrl_pm_select_default_state(dev);
+
ret = clk_enable(sport->clk_ipg);
if (ret)
return ret;
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] tty: serial: imx: add lock for registers save/restore
From: Anson Huang @ 2018-08-09 10:06 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial, linux-kernel; +Cc: Linux-imx
In noirq suspend/resume stage with no_console_suspend enabled,
.imx_console_write() may be called to print out log_buf
message by .printk(), so there will be race condition between
.imx_console_write() and .serial_imx_save/restore_context(),
need to add lock to protect the registers save/restore operations.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/tty/serial/imx.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 239c0fa..a09ccef 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2376,9 +2376,12 @@ static int imx_uart_remove(struct platform_device *pdev)
static void imx_uart_restore_context(struct imx_port *sport)
{
+ unsigned long flags;
+
if (!sport->context_saved)
return;
+ spin_lock_irqsave(&sport->port.lock, flags);
imx_uart_writel(sport, sport->saved_reg[4], UFCR);
imx_uart_writel(sport, sport->saved_reg[5], UESC);
imx_uart_writel(sport, sport->saved_reg[6], UTIM);
@@ -2390,11 +2393,15 @@ static void imx_uart_restore_context(struct imx_port *sport)
imx_uart_writel(sport, sport->saved_reg[2], UCR3);
imx_uart_writel(sport, sport->saved_reg[3], UCR4);
sport->context_saved = false;
+ spin_unlock_irqrestore(&sport->port.lock, flags);
}
static void imx_uart_save_context(struct imx_port *sport)
{
+ unsigned long flags;
+
/* Save necessary regs */
+ spin_lock_irqsave(&sport->port.lock, flags);
sport->saved_reg[0] = imx_uart_readl(sport, UCR1);
sport->saved_reg[1] = imx_uart_readl(sport, UCR2);
sport->saved_reg[2] = imx_uart_readl(sport, UCR3);
@@ -2406,6 +2413,7 @@ static void imx_uart_save_context(struct imx_port *sport)
sport->saved_reg[8] = imx_uart_readl(sport, UBMR);
sport->saved_reg[9] = imx_uart_readl(sport, IMX21_UTS);
sport->context_saved = true;
+ spin_unlock_irqrestore(&sport->port.lock, flags);
}
static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v3 3/8] mailbox: Add transmit done by blocking option
From: Jassi Brar @ 2018-08-09 9:26 UTC (permalink / raw)
To: Mikko Perttunen
Cc: Mikko Perttunen, Greg KH, Thierry Reding, Jon Hunter,
Devicetree List, linux-serial, linux-tegra,
", linux-arm-kernel", srv_heupstream, srv_heupstream,
Linux Kernel Mailing List
In-Reply-To: <b3045f85-eacb-a5ec-1690-b9c63d0f3373@kapsi.fi>
[dropping everyone else while we discuss the code]
Thanks. I assume that branch as all the code that there is. Let me
look and get back to you.
On Thu, Aug 9, 2018 at 2:19 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
> Here's my current code:
>
> https://github.com/cyndis/linux/commits/wip/t194-tcu-4
>
> "fixup! mailbox: tegra-hsp: Add support for shared mailboxes" splits up the
> controller into two. "tegra-hsp: use polling" changes it to use polling.
>
> There are two lines in the top patch with comments:
>
> - at the end of tegra_hsp_mailbox_send_data, I left a "while
> (!tegra_hsp_mailbox_last_tx_done(chan));". Without it I wasn't able to see
> even a few garbled characters in the output.
>
> - as mentioned, if I enable tx_block on the client side, I get a BUG:
> scheduling while atomic. I assume this gets printed through the earlycon as
> it's printing out correctly.
>
> Thanks,
> Mikko
>
>
> On 08.08.2018 17:46, Mikko Perttunen wrote:
>>
>> On 08/08/2018 05:39 PM, Jassi Brar wrote:
>>>
>>> On Wed, Aug 8, 2018 at 8:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>>>>
>>>>
>>>>
>>>> On 08/08/2018 05:10 PM, Jassi Brar wrote:
>>>>>
>>>>>
>>>>> On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 04.08.2018 13:45, Mikko Perttunen wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 08/03/2018 03:54 PM, Jassi Brar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen
>>>>>>>> <mperttunen@nvidia.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the
>>>>>>>>> send_data function of the mailbox driver is expected to block until
>>>>>>>>> the message has been sent. The new option is used with the Tegra
>>>>>>>>> Combined UART driver to minimize unnecessary overhead when
>>>>>>>>> transmitting
>>>>>>>>> data.
>>>>>>>>>
>>>>>>>> 1) TXDONE_BY_BLOCK flag :-
>>>>>>>> Have you tried setting the flag
>>>>>>>> mbox_chan->mbox_client->tx_block
>>>>>>>> ?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> No - I suppose I should have done that. I'm a bit concerned about
>>>>>>> overhead
>>>>>>> as send_data may be called thousands of times per second, so I tried
>>>>>>> to
>>>>>>> make
>>>>>>> it as close as possible to the downstream driver that just pokes the
>>>>>>> mailbox
>>>>>>> register directly.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I tried using polling in the mailbox framework. Some printing is done
>>>>>> from
>>>>>> atomic context so it seems tx_block cannot be used -
>>>>>> wait_for_completion_timeout understandably does not work in atomic
>>>>>> context.
>>>>>> I also tried without tx_block, in which case I got some horribly
>>>>>> garbled
>>>>>> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there.
>>>>>>
>>>>>> Any opinions?
>>>>>>
>>>>> The problems arise because your hardware (SM) supports TXDONE_BY_POLL,
>>>>> but your client drives it by TXDONE_BY_ACK because the older DB
>>>>> channels are so.
>>>>>
>>>>> Please populate SM channels as a separate controller than DB.
>>>>> The DB controller, as is, run by ACK method.
>>>>> The SM controller should be run by polling, i.e, set txdone_poll =
>>>>> true and the poll period small enough. The virtual tty client driver
>>>>> should be able to safely set tx_block from appropriate context.
>>>>>
>>>>
>>>> Sorry, I should have clarified that I already split up the controllers.
>>>> The
>>>> SM controller has txdone_poll = true. I didn't adjust txpoll_period so I
>>>> guess it's zero.
>>>>
>>> Can you please share your code (controller and client) ? Maybe offline
>>> if you wish.
>>>
>>
>> I'll upload a git branch tomorrow -- I'm not at the machine with the code
>> now.
>>
>> Mikko
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 3/8] mailbox: Add transmit done by blocking option
From: Mikko Perttunen @ 2018-08-09 8:49 UTC (permalink / raw)
To: Jassi Brar
Cc: Mikko Perttunen, Greg KH, Thierry Reding, Jon Hunter,
Devicetree List, linux-serial, linux-tegra,
", linux-arm-kernel", srv_heupstream, srv_heupstream,
Linux Kernel Mailing List
In-Reply-To: <a57fccaa-fcfe-6611-45a5-8c224f4ee2b6@kapsi.fi>
Here's my current code:
https://github.com/cyndis/linux/commits/wip/t194-tcu-4
"fixup! mailbox: tegra-hsp: Add support for shared mailboxes" splits up
the controller into two. "tegra-hsp: use polling" changes it to use polling.
There are two lines in the top patch with comments:
- at the end of tegra_hsp_mailbox_send_data, I left a "while
(!tegra_hsp_mailbox_last_tx_done(chan));". Without it I wasn't able to
see even a few garbled characters in the output.
- as mentioned, if I enable tx_block on the client side, I get a BUG:
scheduling while atomic. I assume this gets printed through the earlycon
as it's printing out correctly.
Thanks,
Mikko
On 08.08.2018 17:46, Mikko Perttunen wrote:
> On 08/08/2018 05:39 PM, Jassi Brar wrote:
>> On Wed, Aug 8, 2018 at 8:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>>>
>>>
>>> On 08/08/2018 05:10 PM, Jassi Brar wrote:
>>>>
>>>> On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 04.08.2018 13:45, Mikko Perttunen wrote:
>>>>>>
>>>>>>
>>>>>> On 08/03/2018 03:54 PM, Jassi Brar wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen
>>>>>>> <mperttunen@nvidia.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the
>>>>>>>> send_data function of the mailbox driver is expected to block until
>>>>>>>> the message has been sent. The new option is used with the Tegra
>>>>>>>> Combined UART driver to minimize unnecessary overhead when
>>>>>>>> transmitting
>>>>>>>> data.
>>>>>>>>
>>>>>>> 1) TXDONE_BY_BLOCK flag :-
>>>>>>> Have you tried setting the flag
>>>>>>> mbox_chan->mbox_client->tx_block
>>>>>>> ?
>>>>>>
>>>>>>
>>>>>>
>>>>>> No - I suppose I should have done that. I'm a bit concerned about
>>>>>> overhead
>>>>>> as send_data may be called thousands of times per second, so I
>>>>>> tried to
>>>>>> make
>>>>>> it as close as possible to the downstream driver that just pokes the
>>>>>> mailbox
>>>>>> register directly.
>>>>>
>>>>>
>>>>>
>>>>> I tried using polling in the mailbox framework. Some printing is done
>>>>> from
>>>>> atomic context so it seems tx_block cannot be used -
>>>>> wait_for_completion_timeout understandably does not work in atomic
>>>>> context.
>>>>> I also tried without tx_block, in which case I got some horribly
>>>>> garbled
>>>>> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there.
>>>>>
>>>>> Any opinions?
>>>>>
>>>> The problems arise because your hardware (SM) supports TXDONE_BY_POLL,
>>>> but your client drives it by TXDONE_BY_ACK because the older DB
>>>> channels are so.
>>>>
>>>> Please populate SM channels as a separate controller than DB.
>>>> The DB controller, as is, run by ACK method.
>>>> The SM controller should be run by polling, i.e, set txdone_poll =
>>>> true and the poll period small enough. The virtual tty client driver
>>>> should be able to safely set tx_block from appropriate context.
>>>>
>>>
>>> Sorry, I should have clarified that I already split up the
>>> controllers. The
>>> SM controller has txdone_poll = true. I didn't adjust txpoll_period so I
>>> guess it's zero.
>>>
>> Can you please share your code (controller and client) ? Maybe offline
>> if you wish.
>>
>
> I'll upload a git branch tomorrow -- I'm not at the machine with the
> code now.
>
> Mikko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/2] tty/serial: atmel: add ISO7816 support
From: Richard Genoud @ 2018-08-09 8:47 UTC (permalink / raw)
To: Ludovic Desroches, linux-serial, linux-arch, linux-arm-kernel
Cc: gregkh, jslaby, arnd, richard.genoud, nicolas.ferre,
alexandre.belloni, linux-kernel
In-Reply-To: <20180807130049.5957-3-ludovic.desroches@microchip.com>
Hi !
On 07/08/2018 15:00, Ludovic Desroches wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>
> When mode is set in atmel_config_iso7816() we backup last RS232 mode
> for coming back to this mode if requested.
> Also allow setup of T=0 and T=1 parameter and basic support in set_termios
> function as well.
>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> [ludovic.desroches@microchip.com: rebase, add check on fidi ratio, checkpatch fixes]
> Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> ---
> drivers/tty/serial/atmel_serial.c | 211 +++++++++++++++++++++++++++++++++++---
> drivers/tty/serial/atmel_serial.h | 6 +-
> 2 files changed, 201 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 8e4428725848..4a7ec44b0ace 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -34,6 +34,7 @@
> #include <linux/suspend.h>
> #include <linux/mm.h>
>
> +#include <asm/div64.h>
> #include <asm/io.h>
> #include <asm/ioctls.h>
>
> @@ -83,6 +84,11 @@ static void atmel_stop_rx(struct uart_port *port);
>
> #define ATMEL_ISR_PASS_LIMIT 256
>
> +struct atmel_uart_pdata {
> + unsigned int fidi_min;
> + unsigned int fidi_max;
> +};
> +
> struct atmel_dma_buffer {
> unsigned char *buf;
> dma_addr_t dma_addr;
> @@ -114,6 +120,7 @@ struct atmel_uart_char {
> */
> struct atmel_uart_port {
> struct uart_port uart; /* uart */
> + const struct atmel_uart_pdata *pdata; /* SoC specific parameters */
> struct clk *clk; /* uart clock */
> int may_wakeup; /* cached value of device_may_wakeup for times we need to disable it */
> u32 backup_imr; /* IMR saved during suspend */
> @@ -147,6 +154,8 @@ struct atmel_uart_port {
> struct circ_buf rx_ring;
>
> struct mctrl_gpios *gpios;
> + u32 backup_mode; /* MR saved during iso7816 operations */
> + u32 backup_brgr; /* BRGR saved during iso7816 operations */
> unsigned int tx_done_mask;
> u32 fifo_size;
> u32 rts_high;
> @@ -192,10 +201,34 @@ static struct console atmel_console;
> #endif
>
> #if defined(CONFIG_OF)
> +static struct atmel_uart_pdata at91rm9200_pdata = {
> + .fidi_min = 1,
> + .fidi_max = 2047,
> +};
> +
> +static struct atmel_uart_pdata at91sam9260_pdata = {
> + .fidi_min = 1,
> + .fidi_max = 2047,
> +};
> +
> +static struct atmel_uart_pdata sama5d3_pdata = {
> + .fidi_min = 3,
> + .fidi_max = 65535,
Are you sure this is for sama5d3 ?
>From the datasheets I have, 65535 is for sama5d4/sama5d2
And also, you'll have to s/atmel,at91sam9260-usart/atmel,sama5d2-usart/g
in sama5d{2,4}.dtsi
But I wonder if it could be detected via ATMEL_US_VERSION instead ?
> +};
> +
> static const struct of_device_id atmel_serial_dt_ids[] = {
> - { .compatible = "atmel,at91rm9200-usart" },
> - { .compatible = "atmel,at91sam9260-usart" },
> - { /* sentinel */ }
> + {
> + .compatible = "atmel,at91rm9200-usart",
> + .data = &at91rm9200_pdata,
> + }, {
> + .compatible = "atmel,at91sam9260-usart",
> + .data = &at91sam9260_pdata,
> + }, {
> + .compatible = "atmel,sama5d3-usart",
> + .data = &sama5d3_pdata,
> + }, {
> + /* sentinel */
> + }
> };
> #endif
>
> @@ -362,6 +395,127 @@ static int atmel_config_rs485(struct uart_port *port,
> return 0;
> }
>
> +static unsigned int atmel_calc_cd(struct uart_port *port,
> + struct serial_iso7816 *iso7816conf)
> +{
> + struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> + unsigned int cd;
> + u64 mck_rate;
> +
> + mck_rate = (u64)clk_get_rate(atmel_port->clk);
> + do_div(mck_rate, iso7816conf->clk);
> + cd = mck_rate;
> + return cd;
> +}
> +
> +static unsigned int atmel_calc_fidi(struct uart_port *port,
> + struct serial_iso7816 *iso7816conf)
> +{
> + u64 fidi = 0;
> +
> + if (iso7816conf->sc_fi && iso7816conf->sc_di) {
> + fidi = (u64)iso7816conf->sc_fi;
> + do_div(fidi, iso7816conf->sc_di);
> + }
> + return (u32)fidi;
> +}
> +
> +/* Enable or disable the iso7816 support */
> +/* Called with interrupts disabled */
> +static int atmel_config_iso7816(struct uart_port *port,
> + struct serial_iso7816 *iso7816conf)
> +{
> + struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> + unsigned int mode;
> + unsigned int cd, fidi;
> + int ret = 0;
> +
> + /* Disable interrupts */
> + atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
> +
> + mode = atmel_uart_readl(port, ATMEL_US_MR);
> +
> + if (iso7816conf->flags & SER_ISO7816_ENABLED) {
> + mode &= ~ATMEL_US_USMODE;
> +
> + if (iso7816conf->tg > 255) {
> + dev_err(port->dev, "ISO7816: Timeguard exceeding 255\n");
> + memset(iso7816conf, 0, sizeof(struct serial_iso7816));
> + ret = -EINVAL;
> + goto err_out;
> + }
> +
> + if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
> + == SER_ISO7816_T(0)) {
> + mode |= ATMEL_US_USMODE_ISO7816_T0 | ATMEL_US_DSNACK;
> + } else if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
> + == SER_ISO7816_T(1)) {
> + mode |= ATMEL_US_USMODE_ISO7816_T1 | ATMEL_US_INACK;
> + } else {
> + dev_err(port->dev, "ISO7816: Type not supported\n");
> + memset(iso7816conf, 0, sizeof(struct serial_iso7816));
> + ret = -EINVAL;
> + goto err_out;
> + }
> +
> + mode &= ~(ATMEL_US_USCLKS | ATMEL_US_NBSTOP | ATMEL_US_PAR);
> +
> + /* select mck clock, and output */
> + mode |= ATMEL_US_USCLKS_MCK | ATMEL_US_CLKO;
> + /* set parity for normal/inverse mode + max iterations */
> + mode |= ATMEL_US_PAR_EVEN | ATMEL_US_NBSTOP_1 | ATMEL_US_MAX_ITER(3);
> +
> + cd = atmel_calc_cd(port, iso7816conf);
> + fidi = atmel_calc_fidi(port, iso7816conf);
> + if (fidi == 0) {
> + dev_warn(port->dev, "ISO7816 fidi = 0, Generator generates no signal\n");
> + } else if (fidi < atmel_port->pdata->fidi_min
> + || fidi > atmel_port->pdata->fidi_max) {
> + dev_err(port->dev, "ISO7816 fidi = %u, value not supported\n", fidi);
> + memset(iso7816conf, 0, sizeof(struct serial_iso7816));
> + ret = -EINVAL;
> + goto err_out;
> + }
> +
> + if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) {
> + /* port not yet in iso7816 mode: store configuration */
> + atmel_port->backup_mode = atmel_uart_readl(port, ATMEL_US_MR);
> + atmel_port->backup_brgr = atmel_uart_readl(port, ATMEL_US_BRGR);
> + }
> +
> + atmel_uart_writel(port, ATMEL_US_TTGR, iso7816conf->tg);
> + atmel_uart_writel(port, ATMEL_US_BRGR, cd);
> + atmel_uart_writel(port, ATMEL_US_FIDIR, fidi);
> +
> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS | ATMEL_US_RXEN);
> + atmel_port->tx_done_mask = ATMEL_US_TXEMPTY | ATMEL_US_NACK | ATMEL_US_ITERATION;
> + } else {
> + dev_dbg(port->dev, "Setting UART back to RS232\n");
> + /* back to last RS232 settings */
> + mode = atmel_port->backup_mode;
> + memset(iso7816conf, 0, sizeof(struct serial_iso7816));
> + atmel_uart_writel(port, ATMEL_US_TTGR, 0);
> + atmel_uart_writel(port, ATMEL_US_BRGR, atmel_port->backup_brgr);
> + atmel_uart_writel(port, ATMEL_US_FIDIR, 0x174);
> +
> + if (atmel_use_pdc_tx(port))
> + atmel_port->tx_done_mask = ATMEL_US_ENDTX |
> + ATMEL_US_TXBUFE;
> + else
> + atmel_port->tx_done_mask = ATMEL_US_TXRDY;
> + }
> +
> + port->iso7816 = *iso7816conf;
> +
> + atmel_uart_writel(port, ATMEL_US_MR, mode);
> +
> +err_out:
> + /* Enable interrupts */
> + atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask);
> +
> + return ret;
> +}
> +
> /*
> * Return TIOCSER_TEMT when transmitter FIFO and Shift register is empty.
> */
> @@ -481,8 +635,9 @@ static void atmel_stop_tx(struct uart_port *port)
> /* Disable interrupts */
> atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
>
> - if ((port->rs485.flags & SER_RS485_ENABLED) &&
> - !(port->rs485.flags & SER_RS485_RX_DURING_TX))
> + if (((port->rs485.flags & SER_RS485_ENABLED) &&
> + !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> + port->iso7816.flags & SER_ISO7816_ENABLED)
> atmel_start_rx(port);
> }
>
> @@ -500,8 +655,9 @@ static void atmel_start_tx(struct uart_port *port)
> return;
>
> if (atmel_use_pdc_tx(port) || atmel_use_dma_tx(port))
> - if ((port->rs485.flags & SER_RS485_ENABLED) &&
> - !(port->rs485.flags & SER_RS485_RX_DURING_TX))
> + if (((port->rs485.flags & SER_RS485_ENABLED) &&
> + !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> + port->iso7816.flags & SER_ISO7816_ENABLED)
> atmel_stop_rx(port);
>
> if (atmel_use_pdc_tx(port))
> @@ -799,8 +955,9 @@ static void atmel_complete_tx_dma(void *arg)
> */
> if (!uart_circ_empty(xmit))
> atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
> - else if ((port->rs485.flags & SER_RS485_ENABLED) &&
> - !(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
> + else if (((port->rs485.flags & SER_RS485_ENABLED) &&
> + !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> + port->iso7816.flags & SER_ISO7816_ENABLED) {
> /* DMA done, stop TX, start RX for RS485 */
> atmel_start_rx(port);
> }
> @@ -1281,6 +1438,9 @@ atmel_handle_status(struct uart_port *port, unsigned int pending,
> wake_up_interruptible(&port->state->port.delta_msr_wait);
> }
> }
> +
> + if (pending & (ATMEL_US_NACK | ATMEL_US_ITERATION))
> + dev_dbg(port->dev, "ISO7816 ERROR (0x%08x)\n", pending);
> }
>
> /*
> @@ -1373,8 +1533,9 @@ static void atmel_tx_pdc(struct uart_port *port)
> atmel_uart_writel(port, ATMEL_US_IER,
> atmel_port->tx_done_mask);
> } else {
> - if ((port->rs485.flags & SER_RS485_ENABLED) &&
> - !(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
> + if (((port->rs485.flags & SER_RS485_ENABLED) &&
> + !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> + port->iso7816.flags & SER_ISO7816_ENABLED) {
> /* DMA done, stop TX, start RX for RS485 */
> atmel_start_rx(port);
> }
> @@ -2099,6 +2260,17 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> atmel_uart_writel(port, ATMEL_US_TTGR,
> port->rs485.delay_rts_after_send);
> mode |= ATMEL_US_USMODE_RS485;
> + } else if (port->iso7816.flags & SER_ISO7816_ENABLED) {
> + atmel_uart_writel(port, ATMEL_US_TTGR, port->iso7816.tg);
> + /* select mck clock, and output */
> + mode |= ATMEL_US_USCLKS_MCK | ATMEL_US_CLKO;
> + /* set max iterations */
> + mode |= ATMEL_US_MAX_ITER(3);
> + if ((port->iso7816.flags & SER_ISO7816_T_PARAM)
> + == SER_ISO7816_T(0))
> + mode |= ATMEL_US_USMODE_ISO7816_T0;
> + else
> + mode |= ATMEL_US_USMODE_ISO7816_T1;
> } else if (termios->c_cflag & CRTSCTS) {
> /* RS232 with hardware handshake (RTS/CTS) */
> if (atmel_use_fifo(port) &&
> @@ -2175,7 +2347,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> }
> quot = cd | fp << ATMEL_US_FP_OFFSET;
>
> - atmel_uart_writel(port, ATMEL_US_BRGR, quot);
> + if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> + atmel_uart_writel(port, ATMEL_US_BRGR, quot);
> atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
> atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> atmel_port->tx_stopped = false;
> @@ -2355,6 +2528,7 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
> port->mapbase = pdev->resource[0].start;
> port->irq = pdev->resource[1].start;
> port->rs485_config = atmel_config_rs485;
> + port->iso7816_config = atmel_config_iso7816;
> port->membase = NULL;
>
> memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
> @@ -2378,8 +2552,12 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
> /* only enable clock when USART is in use */
> }
>
> - /* Use TXEMPTY for interrupt when rs485 else TXRDY or ENDTX|TXBUFE */
> - if (port->rs485.flags & SER_RS485_ENABLED)
> + /*
> + * Use TXEMPTY for interrupt when rs485 or ISO7816 else TXRDY or
> + * ENDTX|TXBUFE
> + */
> + if (port->rs485.flags & SER_RS485_ENABLED ||
> + port->iso7816.flags & SER_ISO7816_ENABLED)
> atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;
> else if (atmel_use_pdc_tx(port)) {
> port->fifosize = PDC_BUFFER_SIZE;
> @@ -2719,6 +2897,11 @@ static int atmel_serial_probe(struct platform_device *pdev)
> }
>
> atmel_port = &atmel_ports[ret];
> + atmel_port->pdata = of_device_get_match_data(&pdev->dev);
> + if (!atmel_port->pdata) {
> + ret = -EINVAL;
> + goto err;
> + }
> atmel_port->backup_imr = 0;
> atmel_port->uart.line = ret;
> atmel_serial_probe_fifos(atmel_port, pdev);
> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
> index ba3a2437cde4..6911177964ad 100644
> --- a/drivers/tty/serial/atmel_serial.h
> +++ b/drivers/tty/serial/atmel_serial.h
> @@ -78,7 +78,8 @@
> #define ATMEL_US_OVER BIT(19) /* Oversampling Mode */
> #define ATMEL_US_INACK BIT(20) /* Inhibit Non Acknowledge */
> #define ATMEL_US_DSNACK BIT(21) /* Disable Successive NACK */
> -#define ATMEL_US_MAX_ITER GENMASK(26, 24) /* Max Iterations */
> +#define ATMEL_US_MAX_ITER_MASK GENMASK(26, 24) /* Max Iterations */
> +#define ATMEL_US_MAX_ITER(n) (((n) << 24) & ATMEL_US_MAX_ITER_MASK)
> #define ATMEL_US_FILTER BIT(28) /* Infrared Receive Line Filter */
>
> #define ATMEL_US_IER 0x08 /* Interrupt Enable Register */
> @@ -124,7 +125,8 @@
> #define ATMEL_US_TTGR 0x28 /* Transmitter Timeguard Register */
> #define ATMEL_US_TG GENMASK(7, 0) /* Timeguard Value */
>
> -#define ATMEL_US_FIDI 0x40 /* FI DI Ratio Register */
> +#define ATMEL_US_FIDIR 0x40 /* FI DI Ratio Register */
> +#define ATMEL_US_FIDI GENMASK(15, 0) /* FIDI ratio */
> #define ATMEL_US_NER 0x44 /* Number of Errors Register */
> #define ATMEL_US_IF 0x4c /* IrDA Filter Register */
>
>
Thanks !
^ permalink raw reply
* [PATCH] kgdboc: Passing ekgdboc to command line causes panic
From: zhe.he @ 2018-08-08 15:59 UTC (permalink / raw)
To: jason.wessel, daniel.thompson, gregkh, jslaby, kgdb-bugreport,
linux-serial, linux-kernel
Cc: zhe.he
From: He Zhe <zhe.he@windriver.com>
kgdboc_option_setup does not check input argument before passing it
to strlen. The argument would be a NULL pointer if "ekgdboc", without
its value, is set in command line and thus cause the following panic.
PANIC: early exception 0xe3 IP 10:ffffffff8fbbb620 error 0 cr2 0x0
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.18-rc8+ #1
[ 0.000000] RIP: 0010:strlen+0x0/0x20
...
[ 0.000000] Call Trace
[ 0.000000] ? kgdboc_option_setup+0x9/0xa0
[ 0.000000] ? kgdboc_early_init+0x6/0x1b
[ 0.000000] ? do_early_param+0x4d/0x82
[ 0.000000] ? parse_args+0x212/0x330
[ 0.000000] ? rdinit_setup+0x26/0x26
[ 0.000000] ? parse_early_options+0x20/0x23
[ 0.000000] ? rdinit_setup+0x26/0x26
[ 0.000000] ? parse_early_param+0x2d/0x39
[ 0.000000] ? setup_arch+0x2f7/0xbf4
[ 0.000000] ? start_kernel+0x5e/0x4c2
[ 0.000000] ? load_ucode_bsp+0x113/0x12f
[ 0.000000] ? secondary_startup_64+0xa5/0xb0
This patch adds a check to prevent the panic and changes some printk
to right fashion.
Signed-off-by: He Zhe <zhe.he@windriver.com>
---
drivers/tty/serial/kgdboc.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index b4ba2b1..0003d6c 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -130,8 +130,13 @@ static void kgdboc_unregister_kbd(void)
static int kgdboc_option_setup(char *opt)
{
+ if (!opt) {
+ pr_err("kgdboc: null option\n");
+ return -EINVAL;
+ }
+
if (strlen(opt) >= MAX_CONFIG_LEN) {
- printk(KERN_ERR "kgdboc: config string too long\n");
+ pr_err("kgdboc: config string too long\n");
return -ENOSPC;
}
strcpy(config, opt);
@@ -248,7 +253,7 @@ static int param_set_kgdboc_var(const char *kmessage,
int len = strlen(kmessage);
if (len >= MAX_CONFIG_LEN) {
- printk(KERN_ERR "kgdboc: config string too long\n");
+ pr_err("kgdboc: config string too long\n");
return -ENOSPC;
}
@@ -259,8 +264,7 @@ static int param_set_kgdboc_var(const char *kmessage,
}
if (kgdb_connected) {
- printk(KERN_ERR
- "kgdboc: Cannot reconfigure while KGDB is connected.\n");
+ pr_err("kgdboc: Cannot reconfigure while KGDB is connected.\n");
return -EBUSY;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2 03/18] dt-bindings: clk: Add documentation of grx500 clock controller
From: Rob Herring @ 2018-08-08 14:54 UTC (permalink / raw)
To: yixin zhu
Cc: Songjun Wu, hua.ma, chuanhua.lei, qi-ming.wu, Linux-MIPS,
linux-clk, open list:SERIAL DRIVERS, devicetree,
Michael Turquette, Stephen Boyd, linux-kernel@vger.kernel.org,
Mark Rutland
In-Reply-To: <9c0cbdfa-8109-be0d-8e14-7d303c764f5c@linux.intel.com>
On Tue, Aug 7, 2018 at 9:08 PM yixin zhu <yixin.zhu@linux.intel.com> wrote:
>
>
>
> On 8/6/2018 11:18 PM, Rob Herring wrote:
> > On Thu, Aug 2, 2018 at 9:03 PM Songjun Wu <songjun.wu@linux.intel.com> wrote:
> >> From: Yixin Zhu <yixin.zhu@linux.intel.com>
> >>
> >> This patch adds binding documentation for grx500 clock controller.
> >>
> >> Signed-off-by: YiXin Zhu <yixin.zhu@linux.intel.com>
> >> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
> >> ---
> >>
> >> Changes in v2:
> >> - Rewrite clock driver's dt-binding document according to Rob Herring's
> >> comments.
> >> - Simplify device tree docoment, remove some clock description.
> >>
> >> .../devicetree/bindings/clock/intel,grx500-clk.txt | 39 ++++++++++++++++++++++
> > Please match the compatible string: intel,grx500-cgu.txt
> Will update to use same name.
>
> >
> >> 1 file changed, 39 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/clock/intel,grx500-clk.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/intel,grx500-clk.txt b/Documentation/devicetree/bindings/clock/intel,grx500-clk.txt
> >> new file mode 100644
> >> index 000000000000..e54e1dad9196
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/intel,grx500-clk.txt
> >> @@ -0,0 +1,39 @@
> >> +Device Tree Clock bindings for grx500 PLL controller.
> >> +
> >> +This binding uses the common clock binding:
> >> + Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +
> >> +The grx500 clock controller supplies clock to various controllers within the
> >> +SoC.
> >> +
> >> +Required properties for clock node
> >> +- compatible: Should be "intel,grx500-cgu".
> >> +- reg: physical base address of the controller and length of memory range.
> >> +- #clock-cells: should be 1.
> >> +
> >> +Optional Propteries:
> >> +- intel,osc-frequency: frequency of the osc clock.
> >> +if missing, driver will use clock rate defined in the driver.
> > This should use a fixed-clock node instead.
> Yes, This is a fixed clock node registered in driver code.
> The frequency of the fixed clock is designed to be overwritten by device
> tree in case some one verify
> clock driver in the emulation platform or in some cases frequency other
> than driver defined one is preferred.
Emulation platforms often need several hacks that shouldn't be upstream...
> These kinds of cases are very rare. But I feel it would be better to
> have a way to use customized frequency.
> The frequency defined in device tree will overwritten driver defined
> frequency before registering fixed-clock node.
I don't think you understand what I meant. Add a DT node with
"fixed-clock" compatible and have this node refer to it with "clocks"
property. The frequency is still in DT, but uses the clock binding.
Rob
^ permalink raw reply
* Re: [PATCH v3 3/8] mailbox: Add transmit done by blocking option
From: Mikko Perttunen @ 2018-08-08 14:46 UTC (permalink / raw)
To: Jassi Brar
Cc: Mikko Perttunen, Greg KH, Thierry Reding, Jon Hunter,
Devicetree List, linux-serial, linux-tegra,
", linux-arm-kernel", srv_heupstream, srv_heupstream,
Linux Kernel Mailing List
In-Reply-To: <CABb+yY3Ng+rbKRYXTncfHKv6HXBW5hHjMKDpepz31MxpE9bwRQ@mail.gmail.com>
On 08/08/2018 05:39 PM, Jassi Brar wrote:
> On Wed, Aug 8, 2018 at 8:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>>
>>
>> On 08/08/2018 05:10 PM, Jassi Brar wrote:
>>>
>>> On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>>>>
>>>>
>>>>
>>>> On 04.08.2018 13:45, Mikko Perttunen wrote:
>>>>>
>>>>>
>>>>> On 08/03/2018 03:54 PM, Jassi Brar wrote:
>>>>>>
>>>>>>
>>>>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the
>>>>>>> send_data function of the mailbox driver is expected to block until
>>>>>>> the message has been sent. The new option is used with the Tegra
>>>>>>> Combined UART driver to minimize unnecessary overhead when
>>>>>>> transmitting
>>>>>>> data.
>>>>>>>
>>>>>> 1) TXDONE_BY_BLOCK flag :-
>>>>>> Have you tried setting the flag
>>>>>> mbox_chan->mbox_client->tx_block
>>>>>> ?
>>>>>
>>>>>
>>>>>
>>>>> No - I suppose I should have done that. I'm a bit concerned about
>>>>> overhead
>>>>> as send_data may be called thousands of times per second, so I tried to
>>>>> make
>>>>> it as close as possible to the downstream driver that just pokes the
>>>>> mailbox
>>>>> register directly.
>>>>
>>>>
>>>>
>>>> I tried using polling in the mailbox framework. Some printing is done
>>>> from
>>>> atomic context so it seems tx_block cannot be used -
>>>> wait_for_completion_timeout understandably does not work in atomic
>>>> context.
>>>> I also tried without tx_block, in which case I got some horribly garbled
>>>> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there.
>>>>
>>>> Any opinions?
>>>>
>>> The problems arise because your hardware (SM) supports TXDONE_BY_POLL,
>>> but your client drives it by TXDONE_BY_ACK because the older DB
>>> channels are so.
>>>
>>> Please populate SM channels as a separate controller than DB.
>>> The DB controller, as is, run by ACK method.
>>> The SM controller should be run by polling, i.e, set txdone_poll =
>>> true and the poll period small enough. The virtual tty client driver
>>> should be able to safely set tx_block from appropriate context.
>>>
>>
>> Sorry, I should have clarified that I already split up the controllers. The
>> SM controller has txdone_poll = true. I didn't adjust txpoll_period so I
>> guess it's zero.
>>
> Can you please share your code (controller and client) ? Maybe offline
> if you wish.
>
I'll upload a git branch tomorrow -- I'm not at the machine with the
code now.
Mikko
^ permalink raw reply
* Re: [PATCH v3 3/8] mailbox: Add transmit done by blocking option
From: Jassi Brar @ 2018-08-08 14:39 UTC (permalink / raw)
To: Mikko Perttunen
Cc: Mikko Perttunen, Greg KH, Thierry Reding, Jon Hunter,
Devicetree List, linux-serial, linux-tegra,
", linux-arm-kernel", srv_heupstream, srv_heupstream,
Linux Kernel Mailing List
In-Reply-To: <b5a91f9e-c3ff-371c-d355-4a5e8a147796@kapsi.fi>
On Wed, Aug 8, 2018 at 8:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>
>
> On 08/08/2018 05:10 PM, Jassi Brar wrote:
>>
>> On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>>>
>>>
>>>
>>> On 04.08.2018 13:45, Mikko Perttunen wrote:
>>>>
>>>>
>>>> On 08/03/2018 03:54 PM, Jassi Brar wrote:
>>>>>
>>>>>
>>>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the
>>>>>> send_data function of the mailbox driver is expected to block until
>>>>>> the message has been sent. The new option is used with the Tegra
>>>>>> Combined UART driver to minimize unnecessary overhead when
>>>>>> transmitting
>>>>>> data.
>>>>>>
>>>>> 1) TXDONE_BY_BLOCK flag :-
>>>>> Have you tried setting the flag
>>>>> mbox_chan->mbox_client->tx_block
>>>>> ?
>>>>
>>>>
>>>>
>>>> No - I suppose I should have done that. I'm a bit concerned about
>>>> overhead
>>>> as send_data may be called thousands of times per second, so I tried to
>>>> make
>>>> it as close as possible to the downstream driver that just pokes the
>>>> mailbox
>>>> register directly.
>>>
>>>
>>>
>>> I tried using polling in the mailbox framework. Some printing is done
>>> from
>>> atomic context so it seems tx_block cannot be used -
>>> wait_for_completion_timeout understandably does not work in atomic
>>> context.
>>> I also tried without tx_block, in which case I got some horribly garbled
>>> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there.
>>>
>>> Any opinions?
>>>
>> The problems arise because your hardware (SM) supports TXDONE_BY_POLL,
>> but your client drives it by TXDONE_BY_ACK because the older DB
>> channels are so.
>>
>> Please populate SM channels as a separate controller than DB.
>> The DB controller, as is, run by ACK method.
>> The SM controller should be run by polling, i.e, set txdone_poll =
>> true and the poll period small enough. The virtual tty client driver
>> should be able to safely set tx_block from appropriate context.
>>
>
> Sorry, I should have clarified that I already split up the controllers. The
> SM controller has txdone_poll = true. I didn't adjust txpoll_period so I
> guess it's zero.
>
Can you please share your code (controller and client) ? Maybe offline
if you wish.
^ permalink raw reply
* Re: [PATCH v3 3/8] mailbox: Add transmit done by blocking option
From: Mikko Perttunen @ 2018-08-08 14:34 UTC (permalink / raw)
To: Jassi Brar
Cc: Mikko Perttunen, Greg KH, Thierry Reding, Jon Hunter,
Devicetree List, linux-serial, linux-tegra,
", linux-arm-kernel"
In-Reply-To: <CABb+yY2mpNV186=M2anmZANgMOra4JM7ybyUT4yCxhqAG6-AZg@mail.gmail.com>
On 08/08/2018 05:10 PM, Jassi Brar wrote:
> On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>>
>>
>> On 04.08.2018 13:45, Mikko Perttunen wrote:
>>>
>>> On 08/03/2018 03:54 PM, Jassi Brar wrote:
>>>>
>>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com>
>>>> wrote:
>>>>>
>>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the
>>>>> send_data function of the mailbox driver is expected to block until
>>>>> the message has been sent. The new option is used with the Tegra
>>>>> Combined UART driver to minimize unnecessary overhead when transmitting
>>>>> data.
>>>>>
>>>> 1) TXDONE_BY_BLOCK flag :-
>>>> Have you tried setting the flag mbox_chan->mbox_client->tx_block
>>>> ?
>>>
>>>
>>> No - I suppose I should have done that. I'm a bit concerned about overhead
>>> as send_data may be called thousands of times per second, so I tried to make
>>> it as close as possible to the downstream driver that just pokes the mailbox
>>> register directly.
>>
>>
>> I tried using polling in the mailbox framework. Some printing is done from
>> atomic context so it seems tx_block cannot be used -
>> wait_for_completion_timeout understandably does not work in atomic context.
>> I also tried without tx_block, in which case I got some horribly garbled
>> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there.
>>
>> Any opinions?
>>
> The problems arise because your hardware (SM) supports TXDONE_BY_POLL,
> but your client drives it by TXDONE_BY_ACK because the older DB
> channels are so.
>
> Please populate SM channels as a separate controller than DB.
> The DB controller, as is, run by ACK method.
> The SM controller should be run by polling, i.e, set txdone_poll =
> true and the poll period small enough. The virtual tty client driver
> should be able to safely set tx_block from appropriate context.
>
Sorry, I should have clarified that I already split up the controllers.
The SM controller has txdone_poll = true. I didn't adjust txpoll_period
so I guess it's zero.
Mikko
^ permalink raw reply
* Re: [PATCH v3 3/8] mailbox: Add transmit done by blocking option
From: Jassi Brar @ 2018-08-08 14:10 UTC (permalink / raw)
To: Mikko Perttunen
Cc: Mikko Perttunen, Greg KH, Thierry Reding, Jon Hunter,
Devicetree List, linux-serial, linux-tegra,
", linux-arm-kernel",
, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, srv_heupstream
In-Reply-To: <afc8a305-79ea-2194-0af0-919bbb8f675e@kapsi.fi>
On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>
>
> On 04.08.2018 13:45, Mikko Perttunen wrote:
>>
>> On 08/03/2018 03:54 PM, Jassi Brar wrote:
>>>
>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com>
>>> wrote:
>>>>
>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the
>>>> send_data function of the mailbox driver is expected to block until
>>>> the message has been sent. The new option is used with the Tegra
>>>> Combined UART driver to minimize unnecessary overhead when transmitting
>>>> data.
>>>>
>>> 1) TXDONE_BY_BLOCK flag :-
>>> Have you tried setting the flag mbox_chan->mbox_client->tx_block
>>> ?
>>
>>
>> No - I suppose I should have done that. I'm a bit concerned about overhead
>> as send_data may be called thousands of times per second, so I tried to make
>> it as close as possible to the downstream driver that just pokes the mailbox
>> register directly.
>
>
> I tried using polling in the mailbox framework. Some printing is done from
> atomic context so it seems tx_block cannot be used -
> wait_for_completion_timeout understandably does not work in atomic context.
> I also tried without tx_block, in which case I got some horribly garbled
> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there.
>
> Any opinions?
>
The problems arise because your hardware (SM) supports TXDONE_BY_POLL,
but your client drives it by TXDONE_BY_ACK because the older DB
channels are so.
Please populate SM channels as a separate controller than DB.
The DB controller, as is, run by ACK method.
The SM controller should be run by polling, i.e, set txdone_poll =
true and the poll period small enough. The virtual tty client driver
should be able to safely set tx_block from appropriate context.
^ permalink raw reply
* Re: [PATCH v3 3/8] mailbox: Add transmit done by blocking option
From: Mikko Perttunen @ 2018-08-08 11:38 UTC (permalink / raw)
To: Jassi Brar, Mikko Perttunen
Cc: Greg KH, Thierry Reding, Jon Hunter, Devicetree List,
linux-serial, linux-tegra, ", linux-arm-kernel",
linux-mediatek, srv_heupstream, Linux Kernel Mailing List
In-Reply-To: <907bac36-0c54-8f38-0f5e-f59196d414c6@kapsi.fi>
On 04.08.2018 13:45, Mikko Perttunen wrote:
> On 08/03/2018 03:54 PM, Jassi Brar wrote:
>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen
>> <mperttunen@nvidia.com> wrote:
>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the
>>> send_data function of the mailbox driver is expected to block until
>>> the message has been sent. The new option is used with the Tegra
>>> Combined UART driver to minimize unnecessary overhead when transmitting
>>> data.
>>>
>> 1) TXDONE_BY_BLOCK flag :-
>> Have you tried setting the flag
>> mbox_chan->mbox_client->tx_block ?
>
> No - I suppose I should have done that. I'm a bit concerned about
> overhead as send_data may be called thousands of times per second, so I
> tried to make it as close as possible to the downstream driver that just
> pokes the mailbox register directly.
I tried using polling in the mailbox framework. Some printing is done
from atomic context so it seems tx_block cannot be used -
wait_for_completion_timeout understandably does not work in atomic
context. I also tried without tx_block, in which case I got some
horribly garbled output, but "Try increasing MBOX_TX_QUEUE_LEN" was
readable there.
Any opinions?
Thanks,
Mikko
>
>>
>> 2) Implementing TEGRA_HSP_MBOX_TYPE_SM :-
>> In mailbox framework, a controller is a collection of identical
>> channels. That is, instances of the same class.
>> So ideally, in probe you should populate a controller for each
>> type of channel, i.e, DB, SM, SS and AS.
>
> Hmm, yes, I guess this would be possible if I change the mailbox core to
> allow registering multiple controllers per device.
>
> Thanks!
> Mikko
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 02/18] clk: intel: Add clock driver for Intel MIPS SoCs
From: yixin zhu @ 2018-08-08 8:52 UTC (permalink / raw)
To: Stephen Boyd, Songjun Wu, chuanhua.lei, hua.ma, qi-ming.wu
Cc: linux-mips, linux-clk, linux-serial, devicetree,
Michael Turquette, linux-kernel, Rob Herring, Mark Rutland
In-Reply-To: <153370742214.220756.2039365625963765922@swboyd.mtv.corp.google.com>
On 8/8/2018 1:50 PM, Stephen Boyd wrote:
> Quoting Songjun Wu (2018-08-02 20:02:21)
>> From: Yixin Zhu <yixin.zhu@linux.intel.com>
>>
>> This driver provides PLL clock registration as well as various clock
>> branches, e.g. MUX clock, gate clock, divider clock and so on.
>>
>> PLLs that provide clock to DDR, CPU and peripherals are shown below:
>>
>> +---------+
>> |--->| LCPLL3 0|--PCIe clk-->
>> XO | +---------+
>> +-----------|
>> | +---------+
>> | | 3|--PAE clk-->
>> |--->| PLL0B 2|--GSWIP clk-->
>> | | 1|--DDR clk-->DDR PHY clk-->
>> | | 0|--CPU1 clk--+ +-----+
>> | +---------+ |--->0 |
>> | | MUX |--CPU clk-->
>> | +---------+ |--->1 |
>> | | 0|--CPU0 clk--+ +-----+
>> |--->| PLLOA 1|--SSX4 clk-->
>> | 2|--NGI clk-->
>> | 3|--CBM clk-->
>> +---------+
> Thanks for the picture!
Thanks for the review.
>
>> Signed-off-by: Yixin Zhu <yixin.zhu@linux.intel.com>
>> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index 0bb25dd009d1..d929ca4607cf 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -72,6 +72,9 @@ obj-$(CONFIG_ARCH_HISI) += hisilicon/
>> obj-y += imgtec/
>> obj-$(CONFIG_ARCH_MXC) += imx/
>> obj-$(CONFIG_MACH_INGENIC) += ingenic/
>> +ifeq ($(CONFIG_COMMON_CLK), y)
>> +obj-y +=intel/
>> +endif
> Why not obj-$(CONFIG_INTEL_CCF) or something like that?
Will use obj-$(CONFIG_INTEL_CCF)
>> obj-$(CONFIG_ARCH_KEYSTONE) += keystone/
>> obj-$(CONFIG_MACH_LOONGSON32) += loongson1/
>> obj-y += mediatek/
>> diff --git a/drivers/clk/intel/Kconfig b/drivers/clk/intel/Kconfig
>> new file mode 100644
>> index 000000000000..c7d3fb1721fa
>> --- /dev/null
>> +++ b/drivers/clk/intel/Kconfig
>> @@ -0,0 +1,20 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +config INTEL_CGU_CLK
>> + depends on COMMON_CLK
>> + depends on INTEL_MIPS || COMPILE_TEST
>> + select MFD_SYSCON
>> + bool "Intel clock controller support"
>> + help
>> + This driver support Intel CGU (Clock Generation Unit).
> Is it really called a clock generation unit? Or that's just copied from
> sunxi driver?
Yes, It's called clock generation unit(CGU) in our HW chip spec.
>> +
>> +choice
>> + prompt "SoC platform selection"
>> + depends on INTEL_CGU_CLK
>> + default INTEL_GRX500_CGU_CLK
>> +
>> +config INTEL_GRX500_CGU_CLK
>> + bool "GRX500 CLK"
>> + help
>> + Clock driver of GRX500 platform.
>> +
>> +endchoice
>> diff --git a/drivers/clk/intel/Makefile b/drivers/clk/intel/Makefile
>> new file mode 100644
>> index 000000000000..16a0138e52c2
>> --- /dev/null
>> +++ b/drivers/clk/intel/Makefile
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Makefile for intel specific clk
>> +
>> +obj-$(CONFIG_INTEL_CGU_CLK) += clk-cgu.o clk-cgu-pll.o
>> +ifneq ($(CONFIG_INTEL_GRX500_CGU_CLK),)
>> + obj-y += clk-grx500.o
>> +endif
>> diff --git a/drivers/clk/intel/clk-cgu-pll.c b/drivers/clk/intel/clk-cgu-pll.c
>> new file mode 100644
>> index 000000000000..20759bc27e95
>> --- /dev/null
>> +++ b/drivers/clk/intel/clk-cgu-pll.c
>> @@ -0,0 +1,166 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Intel Corporation.
>> + * Zhu YiXin <Yixin.zhu@intel.com>
>> + */
>> +
>> +#include <linux/clk.h>
> Is this include used?
Not used. Will remove it.
>> +#include <linux/clk-provider.h>
>> +#include <linux/clkdev.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +#include "clk-cgu-pll.h"
>> +#include "clk-cgu.h"
>> +
>> +#define to_intel_clk_pll(_hw) container_of(_hw, struct intel_clk_pll, hw)
>> +
>> +/*
>> + * Calculate formula:
>> + * rate = (prate * mult + (prate * frac) / frac_div) / div
>> + */
>> +static unsigned long
>> +intel_pll_calc_rate(unsigned long prate, unsigned int mult,
>> + unsigned int div, unsigned int frac,
>> + unsigned int frac_div)
>> +{
>> + u64 crate, frate, rate64;
>> +
>> + rate64 = prate;
>> + crate = rate64 * mult;
>> +
>> + if (frac) {
>> + frate = rate64 * frac;
>> + do_div(frate, frac_div);
>> + crate += frate;
>> + }
>> + do_div(crate, div);
>> +
>> + return (unsigned long)crate;
>> +}
>> +
>> +static void
>> +grx500_pll_get_params(struct intel_clk_pll *pll, unsigned int *mult,
>> + unsigned int *frac)
>> +{
>> + *mult = intel_get_clk_val(pll->map, pll->reg, 2, 7);
>> + *frac = intel_get_clk_val(pll->map, pll->reg, 9, 21);
>> +}
>> +
>> +static int intel_wait_pll_lock(struct intel_clk_pll *pll, int bit_idx)
>> +{
>> + unsigned int val;
>> +
>> + return regmap_read_poll_timeout(pll->map, pll->reg, val,
>> + val & BIT(bit_idx), 10, 1000);
>> +}
>> +
>> +static unsigned long
>> +intel_grx500_pll_recalc_rate(struct clk_hw *hw, unsigned long prate)
>> +{
>> + struct intel_clk_pll *pll = to_intel_clk_pll(hw);
>> + unsigned int mult, frac;
>> +
>> + grx500_pll_get_params(pll, &mult, &frac);
>> +
>> + return intel_pll_calc_rate(prate, mult, 1, frac, BIT(20));
>> +}
>> +
>> +static int intel_grx500_pll_is_enabled(struct clk_hw *hw)
>> +{
>> + struct intel_clk_pll *pll = to_intel_clk_pll(hw);
>> +
>> + if (intel_wait_pll_lock(pll, 1)) {
>> + pr_err("%s: pll: %s is not locked!\n",
>> + __func__, clk_hw_get_name(hw));
>> + return 0;
>> + }
>> +
>> + return intel_get_clk_val(pll->map, pll->reg, 1, 1);
>> +}
>> +
>> +const static struct clk_ops intel_grx500_pll_ops = {
> Should be static const struct ...
Will update it.
>> + .recalc_rate = intel_grx500_pll_recalc_rate,
>> + .is_enabled = intel_grx500_pll_is_enabled,
>> +};
>> +
>> +static struct clk
>> +*intel_clk_register_pll(struct intel_clk_provider *ctx,
>> + enum intel_pll_type type, const char *cname,
>> + const char *const *pname, u8 num_parents,
>> + unsigned long flags, unsigned int reg,
>> + const struct intel_pll_rate_table *table,
>> + unsigned int mult, unsigned int div, unsigned int frac)
>> +{
>> + struct clk_init_data init;
>> + struct intel_clk_pll *pll;
>> + struct clk_hw *hw;
>> + int ret, i;
>> +
>> + if (type != pll_grx500) {
>> + pr_err("%s: pll type %d not supported!\n",
>> + __func__, type);
>> + return ERR_PTR(-EINVAL);
>> + }
>> + init.name = cname;
>> + init.ops = &intel_grx500_pll_ops;
>> + init.flags = CLK_IS_BASIC;
> Don't use this flag unless you have some reason to need it.
Will remove it.
>> + init.parent_names = pname;
>> + init.num_parents = num_parents;
>> +
>> + pll = kzalloc(sizeof(*pll), GFP_KERNEL);
>> + if (!pll)
>> + return ERR_PTR(-ENOMEM);
>> + pll->map = ctx->map;
>> + pll->reg = reg;
>> + pll->flags = flags;
>> + pll->mult = mult;
>> + pll->div = div;
>> + pll->frac = frac;
>> + pll->hw.init = &init;
>> + if (table) {
>> + for (i = 0; table[i].rate != 0; i++)
>> + ;
>> + pll->table_sz = i;
>> + pll->rate_table = kmemdup(table, i * sizeof(table[0]),
>> + GFP_KERNEL);
>> + if (!pll->rate_table) {
>> + ret = -ENOMEM;
>> + goto err_free_pll;
>> + }
>> + }
>> + hw = &pll->hw;
>> + ret = clk_hw_register(NULL, hw);
>> + if (ret)
>> + goto err_free_pll;
>> +
>> + return hw->clk;
>> +
>> +err_free_pll:
>> + kfree(pll);
>> + return ERR_PTR(ret);
>> +}
>> +
>> +void intel_clk_register_plls(struct intel_clk_provider *ctx,
>> + struct intel_pll_clk *list, unsigned int nr_clk)
>> +{
>> + struct clk *clk;
>> + int i;
>> +
>> + for (i = 0; i < nr_clk; i++, list++) {
>> + clk = intel_clk_register_pll(ctx, list->type, list->name,
>> + list->parent_names, list->num_parents,
>> + list->flags, list->reg, list->rate_table,
>> + list->mult, list->div, list->frac);
>> + if (IS_ERR(clk)) {
>> + pr_err("%s: failed to register pll: %s\n",
>> + __func__, list->name);
>> + continue;
>> + }
>> +
>> + intel_clk_add_lookup(ctx, clk, list->id);
>> + }
>> +}
>> diff --git a/drivers/clk/intel/clk-cgu-pll.h b/drivers/clk/intel/clk-cgu-pll.h
>> new file mode 100644
>> index 000000000000..3e7cff1d5e16
>> --- /dev/null
>> +++ b/drivers/clk/intel/clk-cgu-pll.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright(c) 2018 Intel Corporation.
>> + * Zhu YiXin <Yixin.zhu@intel.com>
>> + */
>> +
>> +#ifndef __INTEL_CLK_PLL_H
>> +#define __INTEL_CLK_PLL_H
>> +
>> +enum intel_pll_type {
>> + pll_grx500,
>> +};
>> +
>> +struct intel_pll_rate_table {
>> + unsigned long prate;
>> + unsigned long rate;
>> + unsigned int mult;
>> + unsigned int div;
>> + unsigned int frac;
>> +};
>> +
>> +struct intel_clk_pll {
>> + struct clk_hw hw;
>> + struct regmap *map;
>> + unsigned int reg;
>> + unsigned long flags;
>> + unsigned int mult;
>> + unsigned int div;
>> + unsigned int frac;
>> + unsigned int table_sz;
>> + const struct intel_pll_rate_table *rate_table;
>> +};
>> +
>> +#endif /* __INTEL_CLK_PLL_H */
>> diff --git a/drivers/clk/intel/clk-cgu.c b/drivers/clk/intel/clk-cgu.c
>> new file mode 100644
>> index 000000000000..10cacbe0fbcd
>> --- /dev/null
>> +++ b/drivers/clk/intel/clk-cgu.c
>> @@ -0,0 +1,470 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Intel Corporation.
>> + * Zhu YiXin <Yixin.zhu@intel.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clkdev.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +#include "clk-cgu-pll.h"
>> +#include "clk-cgu.h"
>> +
>> +#define GATE_HW_REG_STAT(reg) (reg)
>> +#define GATE_HW_REG_EN(reg) ((reg) + 0x4)
>> +#define GATE_HW_REG_DIS(reg) ((reg) + 0x8)
>> +
>> +#define to_intel_clk_mux(_hw) container_of(_hw, struct intel_clk_mux, hw)
>> +#define to_intel_clk_divider(_hw) \
>> + container_of(_hw, struct intel_clk_divider, hw)
>> +#define to_intel_clk_gate(_hw) container_of(_hw, struct intel_clk_gate, hw)
>> +
>> +void intel_set_clk_val(struct regmap *map, u32 reg, u8 shift,
>> + u8 width, u32 set_val)
>> +{
>> + u32 mask = GENMASK(width + shift, shift);
>> +
>> + regmap_update_bits(map, reg, mask, set_val << shift);
>> +}
>> +
>> +u32 intel_get_clk_val(struct regmap *map, u32 reg, u8 shift,
>> + u8 width)
>> +{
>> + u32 val;
>> +
>> + if (regmap_read(map, reg, &val)) {
>> + WARN_ONCE(1, "Failed to read clk reg: 0x%x\n", reg);
>> + return 0;
>> + }
>> + val >>= shift;
>> + val &= BIT(width) - 1;
>> +
>> + return val;
>> +}
>> +
>> +void intel_clk_add_lookup(struct intel_clk_provider *ctx,
>> + struct clk *clk, unsigned int id)
>> +{
>> + pr_debug("Add clk: %s, id: %u\n", __clk_get_name(clk), id);
>> + if (ctx->clk_data.clks && id)
>> + ctx->clk_data.clks[id] = clk;
>> +}
>> +
>> +static struct clk
>> +*intel_clk_register_fixed(struct intel_clk_provider *ctx,
>> + struct intel_clk_branch *list)
>> +{
>> + if (list->div_flags & CLOCK_FLAG_VAL_INIT)
>> + intel_set_clk_val(ctx->map, list->div_off, list->div_shift,
>> + list->div_width, list->div_val);
>> +
>> + return clk_register_fixed_rate(NULL, list->name, list->parent_names[0],
>> + list->flags, list->mux_flags);
>> +}
>> +
>> +static u8 intel_clk_mux_get_parent(struct clk_hw *hw)
>> +{
>> + struct intel_clk_mux *mux = to_intel_clk_mux(hw);
>> + u32 val;
>> +
>> + val = intel_get_clk_val(mux->map, mux->reg, mux->shift, mux->width);
>> + return clk_mux_val_to_index(hw, NULL, mux->flags, val);
>> +}
>> +
>> +static int intel_clk_mux_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> + struct intel_clk_mux *mux = to_intel_clk_mux(hw);
>> + u32 val;
>> +
>> + val = clk_mux_index_to_val(NULL, mux->flags, index);
>> + intel_set_clk_val(mux->map, mux->reg, mux->shift, mux->width, val);
>> +
>> + return 0;
>> +}
>> +
>> +static int intel_clk_mux_determine_rate(struct clk_hw *hw,
>> + struct clk_rate_request *req)
>> +{
>> + struct intel_clk_mux *mux = to_intel_clk_mux(hw);
>> +
>> + return clk_mux_determine_rate_flags(hw, req, mux->flags);
>> +}
>> +
>> +const static struct clk_ops intel_clk_mux_ops = {
>> + .get_parent = intel_clk_mux_get_parent,
>> + .set_parent = intel_clk_mux_set_parent,
>> + .determine_rate = intel_clk_mux_determine_rate,
>> +};
>> +
>> +static struct clk
>> +*intel_clk_register_mux(struct intel_clk_provider *ctx,
>> + struct intel_clk_branch *list)
>> +{
>> + struct clk_init_data init;
>> + struct clk_hw *hw;
>> + struct intel_clk_mux *mux;
>> + u32 reg = list->mux_off;
>> + u8 shift = list->mux_shift;
>> + u8 width = list->mux_width;
>> + unsigned long cflags = list->mux_flags;
>> + int ret;
>> +
>> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
>> + if (!mux)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + init.name = list->name;
>> + init.ops = &intel_clk_mux_ops;
>> + init.flags = list->flags | CLK_IS_BASIC;
>> + init.parent_names = list->parent_names;
>> + init.num_parents = list->num_parents;
>> +
>> + mux->map = ctx->map;
>> + mux->reg = reg;
>> + mux->shift = shift;
>> + mux->width = width;
>> + mux->flags = cflags;
>> + mux->hw.init = &init;
>> +
>> + hw = &mux->hw;
>> + ret = clk_hw_register(NULL, hw);
>> + if (ret) {
>> + kfree(mux);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + if (cflags & CLOCK_FLAG_VAL_INIT)
>> + intel_set_clk_val(ctx->map, reg, shift, width, list->mux_val);
>> +
>> + return hw->clk;
>> +}
>> +
>> +static unsigned long
>> +intel_clk_divider_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + struct intel_clk_divider *divider = to_intel_clk_divider(hw);
>> + unsigned int val;
>> +
>> + val = intel_get_clk_val(divider->map, divider->reg,
>> + divider->shift, divider->width);
>> + return divider_recalc_rate(hw, parent_rate, val, divider->table,
>> + divider->flags, divider->width);
>> +}
>> +
>> +static long
>> +intel_clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
>> + unsigned long *prate)
>> +{
>> + struct intel_clk_divider *divider = to_intel_clk_divider(hw);
>> +
>> + return divider_round_rate(hw, rate, prate, divider->table,
>> + divider->width, divider->flags);
>> +}
>> +
>> +static int
>> +intel_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
>> + unsigned long prate)
>> +{
>> + struct intel_clk_divider *divider = to_intel_clk_divider(hw);
>> + int value;
>> +
>> + value = divider_get_val(rate, prate, divider->table,
>> + divider->width, divider->flags);
>> + if (value < 0)
>> + return value;
>> +
>> + intel_set_clk_val(divider->map, divider->reg,
>> + divider->shift, divider->width, value);
>> +
>> + return 0;
>> +}
>> +
>> +const static struct clk_ops intel_clk_divider_ops = {
>> + .recalc_rate = intel_clk_divider_recalc_rate,
>> + .round_rate = intel_clk_divider_round_rate,
>> + .set_rate = intel_clk_divider_set_rate,
>> +};
>> +
>> +static struct clk
>> +*intel_clk_register_divider(struct intel_clk_provider *ctx,
>> + struct intel_clk_branch *list)
>> +{
>> + struct clk_init_data init;
>> + struct clk_hw *hw;
>> + struct intel_clk_divider *div;
>> + u32 reg = list->div_off;
>> + u8 shift = list->div_shift;
>> + u8 width = list->div_width;
>> + unsigned long cflags = list->div_flags;
>> + int ret;
>> +
>> + div = kzalloc(sizeof(*div), GFP_KERNEL);
>> + if (!div)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + init.name = list->name;
>> + init.ops = &intel_clk_divider_ops;
>> + init.flags = list->flags | CLK_IS_BASIC;
>> + init.parent_names = &list->parent_names[0];
>> + init.num_parents = 1;
>> +
>> + div->map = ctx->map;
>> + div->reg = reg;
>> + div->shift = shift;
>> + div->width = width;
>> + div->flags = cflags;
>> + div->table = list->div_table;
>> + div->hw.init = &init;
>> +
>> + hw = &div->hw;
>> + ret = clk_hw_register(NULL, hw);
>> + if (ret) {
>> + pr_err("%s: register clk: %s failed!\n",
>> + __func__, list->name);
>> + kfree(div);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + if (cflags & CLOCK_FLAG_VAL_INIT)
>> + intel_set_clk_val(ctx->map, reg, shift, width, list->div_val);
>> +
>> + return hw->clk;
>> +}
>> +
>> +static struct clk
>> +*intel_clk_register_fixed_factor(struct intel_clk_provider *ctx,
>> + struct intel_clk_branch *list)
>> +{
>> + struct clk_hw *hw;
>> +
>> + hw = clk_hw_register_fixed_factor(NULL, list->name,
>> + list->parent_names[0], list->flags,
>> + list->mult, list->div);
>> + if (IS_ERR(hw))
>> + return ERR_CAST(hw);
>> +
>> + if (list->div_flags & CLOCK_FLAG_VAL_INIT)
>> + intel_set_clk_val(ctx->map, list->div_off, list->div_shift,
>> + list->div_width, list->div_val);
>> +
>> + return hw->clk;
>> +}
>> +
>> +static int
>> +intel_clk_gate_enable(struct clk_hw *hw)
>> +{
>> + struct intel_clk_gate *gate = to_intel_clk_gate(hw);
>> + unsigned int reg;
>> +
>> + if (gate->flags & GATE_CLK_VT) {
>> + gate->reg = 1;
>> + return 0;
>> + }
>> +
>> + if (gate->flags & GATE_CLK_HW) {
>> + reg = GATE_HW_REG_EN(gate->reg);
>> + } else if (gate->flags & GATE_CLK_SW) {
>> + reg = gate->reg;
>> + } else {
>> + pr_err("%s: gate clk: %s: flag 0x%lx not supported!\n",
>> + __func__, clk_hw_get_name(hw), gate->flags);
>> + return 0;
>> + }
>> +
>> + intel_set_clk_val(gate->map, reg, gate->shift, 1, 1);
>> +
>> + return 0;
>> +}
>> +
>> +static void
>> +intel_clk_gate_disable(struct clk_hw *hw)
>> +{
>> + struct intel_clk_gate *gate = to_intel_clk_gate(hw);
>> + unsigned int reg;
>> + unsigned int set;
>> +
>> + if (gate->flags & GATE_CLK_VT) {
>> + gate->reg = 0;
>> + return;
>> + }
>> +
>> + if (gate->flags & GATE_CLK_HW) {
>> + reg = GATE_HW_REG_DIS(gate->reg);
>> + set = 1;
>> + } else if (gate->flags & GATE_CLK_SW) {
>> + reg = gate->reg;
>> + set = 0;
>> + } else {
>> + pr_err("%s: gate clk: %s: flag 0x%lx not supported!\n",
>> + __func__, clk_hw_get_name(hw), gate->flags);
>> + return;
>> + }
>> +
>> + intel_set_clk_val(gate->map, reg, gate->shift, 1, set);
>> +}
>> +
>> +static int
>> +intel_clk_gate_is_enabled(struct clk_hw *hw)
>> +{
>> + struct intel_clk_gate *gate = to_intel_clk_gate(hw);
>> + unsigned int reg;
>> +
>> + if (gate->flags & GATE_CLK_VT)
>> + return gate->reg;
>> +
>> + if (gate->flags & GATE_CLK_HW) {
>> + reg = GATE_HW_REG_STAT(gate->reg);
>> + } else if (gate->flags & GATE_CLK_SW) {
>> + reg = gate->reg;
>> + } else {
>> + pr_err("%s: gate clk: %s: flag 0x%lx not supported!\n",
>> + __func__, clk_hw_get_name(hw), gate->flags);
>> + return 0;
>> + }
>> +
>> + return intel_get_clk_val(gate->map, reg, gate->shift, 1);
>> +}
>> +
>> +const static struct clk_ops intel_clk_gate_ops = {
>> + .enable = intel_clk_gate_enable,
>> + .disable = intel_clk_gate_disable,
>> + .is_enabled = intel_clk_gate_is_enabled,
>> +};
>> +
>> +static struct clk
>> +*intel_clk_register_gate(struct intel_clk_provider *ctx,
>> + struct intel_clk_branch *list)
>> +{
>> + struct clk_init_data init;
> Please init the init struct with { } so that future possible additions
> to the structure don't require us to hunt this silent corruption down
> later.
Will update it.
>> + struct clk_hw *hw;
>> + struct intel_clk_gate *gate;
>> + u32 reg = list->gate_off;
>> + u8 shift = list->gate_shift;
>> + unsigned long cflags = list->gate_flags;
>> + const char *pname = list->parent_names[0];
>> + int ret;
>> +
>> + gate = kzalloc(sizeof(*gate), GFP_KERNEL);
>> + if (!gate)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + init.name = list->name;
>> + init.ops = &intel_clk_gate_ops;
>> + init.flags = list->flags | CLK_IS_BASIC;
>> + init.parent_names = pname ? &pname : NULL;
>> + init.num_parents = pname ? 1 : 0;
>> +
>> + gate->map = ctx->map;
>> + gate->reg = reg;
>> + gate->shift = shift;
>> + gate->flags = cflags;
>> + gate->hw.init = &init;
>> +
>> + hw = &gate->hw;
>> + ret = clk_hw_register(NULL, hw);
>> + if (ret) {
>> + kfree(gate);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + if (cflags & CLOCK_FLAG_VAL_INIT)
>> + intel_set_clk_val(ctx->map, reg, shift, 1, list->gate_val);
>> +
>> + return hw->clk;
>> +}
>> +
>> +void intel_clk_register_branches(struct intel_clk_provider *ctx,
>> + struct intel_clk_branch *list,
>> + unsigned int nr_clk)
>> +{
>> + struct clk *clk;
>> + unsigned int idx;
>> +
>> + for (idx = 0; idx < nr_clk; idx++, list++) {
>> + switch (list->type) {
>> + case intel_clk_fixed:
> Please use uppercase for enums.
Will update.
>
>> + clk = intel_clk_register_fixed(ctx, list);
>> + break;
>> + case intel_clk_mux:
>> + clk = intel_clk_register_mux(ctx, list);
>> + break;
>> + case intel_clk_divider:
>> + clk = intel_clk_register_divider(ctx, list);
>> + break;
>> + case intel_clk_fixed_factor:
>> + clk = intel_clk_register_fixed_factor(ctx, list);
>> + break;
>> + case intel_clk_gate:
>> + clk = intel_clk_register_gate(ctx, list);
>> + break;
>> + default:
>> + pr_err("%s: type: %u not supported!\n",
>> + __func__, list->type);
>> + return;
>> + }
>> +
>> + if (IS_ERR(clk)) {
>> + pr_err("%s: register clk: %s, type: %u failed!\n",
>> + __func__, list->name, list->type);
>> + return;
>> + }
>> +
>> + intel_clk_add_lookup(ctx, clk, list->id);
>> + }
>> +}
>> +
>> +struct intel_clk_provider * __init
>> +intel_clk_init(struct device_node *np, struct regmap *map, unsigned int nr_clks)
>> +{
>> + struct intel_clk_provider *ctx;
>> + struct clk **clks;
>> +
>> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> + if (!ctx)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + clks = kcalloc(nr_clks, sizeof(*clks), GFP_KERNEL);
>> + if (!clks) {
>> + kfree(ctx);
>> + return ERR_PTR(-ENOMEM);
>> + }
>> +
>> + memset_p((void **)clks, ERR_PTR(-ENOENT), nr_clks);
>> + ctx->map = map;
>> + ctx->clk_data.clks = clks;
>> + ctx->clk_data.clk_num = nr_clks;
>> + ctx->np = np;
>> +
>> + return ctx;
>> +}
>> +
>> +void __init intel_clk_register_osc(struct intel_clk_provider *ctx,
>> + struct intel_osc_clk *osc,
>> + unsigned int nr_clks)
>> +{
>> + u32 freq;
>> + struct clk *clk;
>> + int idx;
>> +
>> + for (idx = 0; idx < nr_clks; idx++, osc++) {
>> + if (!osc->dt_freq ||
>> + of_property_read_u32(ctx->np, osc->dt_freq, &freq))
>> + freq = osc->def_rate;
>> +
>> + clk = clk_register_fixed_rate(NULL, osc->name, NULL, 0, freq);
> Should come from DT itself.
Yes. It can be defined as fixed-clock node in device tree.
Do you mean it should be defined in device tree and driver reference it
via device tree?
>> + iS_ERR(clk)) {
>> + pr_err("%s: Failed to register clock: %s\n",
>> + __func__, osc->name);
>> + return;
>> + }
>> +
>> + intel_clk_add_lookup(ctx, clk, osc->id);
>> + }
>> +}
>> diff --git a/drivers/clk/intel/clk-cgu.h b/drivers/clk/intel/clk-cgu.h
>> new file mode 100644
>> index 000000000000..6dc4e45fc499
>> --- /dev/null
>> +++ b/drivers/clk/intel/clk-cgu.h
>> @@ -0,0 +1,259 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright(c) 2018 Intel Corporation.
>> + * Zhu YiXin <Yixin.zhu@intel.com>
>> + */
>> +
>> +#ifndef __INTEL_CLK_H
>> +#define __INTEL_CLK_H
>> +
>> +#define PNAME(x) static const char *const x[] __initconst
>> +
>> +struct intel_clk_mux {
>> + struct clk_hw hw;
>> + struct regmap *map;
>> + unsigned int reg;
>> + u8 shift;
>> + u8 width;
>> + unsigned long flags;
>> +};
>> +
>> +struct intel_clk_divider {
>> + struct clk_hw hw;
>> + struct regmap *map;
>> + unsigned int reg;
>> + u8 shift;
>> + u8 width;
>> + unsigned long flags;
>> + const struct clk_div_table *table;
>> +};
>> +
>> +struct intel_clk_gate {
>> + struct clk_hw hw;
>> + struct regmap *map;
>> + unsigned int reg;
>> + u8 shift;
>> + unsigned long flags;
>> +};
>> +
>> +enum intel_clk_type {
>> + intel_clk_fixed,
>> + intel_clk_mux,
>> + intel_clk_divider,
>> + intel_clk_fixed_factor,
>> + intel_clk_gate,
>> +};
>> +
>> +/**
>> + * struct intel_clk_provider
>> + * @map: regmap type base address for register.
>> + * @np: device node
>> + * @clk_data: array of hw clocks and clk number.
>> + */
>> +struct intel_clk_provider {
>> + struct regmap *map;
>> + struct device_node *np;
>> + struct clk_onecell_data clk_data;
> Please register clk_hw pointers instead of clk pointers with the of
> provider APIs.
Sorry. I'm not sure I understand you correctly.
If only registering clk_hw pointer, not registering of_provider API, then
how to reference it in the user drivers ?
Could you please give me more hints ?
>> +};
>> +
>> +/**
>> + * struct intel_pll_clk
>> + * @id: plaform specific id of the clock.
>> + * @name: name of this pll clock.
>> + * @parent_names: name of the parent clock.
>> + * @num_parents: number of parents.
>> + * @flags: optional flags for basic clock.
>> + * @type: platform type of pll.
>> + * @reg: offset of the register.
>> + * @mult: init value of mulitplier.
>> + * @div: init value of divider.
>> + * @frac: init value of fraction.
>> + * @rate_table: table of pll clock rate.
> Please drop the full-stop on kernel doc one-liners like this.
Will update it.
>
>> + */
>> +struct intel_pll_clk {
>> + unsigned int id;
>> + const char *name;
>> + const char *const *parent_names;
>> + u8 num_parents;
> Can the PLL have multiple parents?
Yes. But not in this platform.
The define here make it easy to expand to support new platform.
>> + unsigned long flags;
>> + enum intel_pll_type type;
>> + int reg;
>> + unsigned int mult;
>> + unsigned int div;
>> + unsigned int frac;
>> + const struct intel_pll_rate_table *rate_table;
>> +};
>> +
>> +#define INTEL_PLL(_id, _type, _name, _pnames, _flags, \
>> + _reg, _rtable, _mult, _div, _frac) \
>> + { \
>> + .id = _id, \
>> + .type = _type, \
>> + .name = _name, \
>> + .parent_names = _pnames, \
>> + .num_parents = ARRAY_SIZE(_pnames), \
>> + .flags = _flags, \
>> + .reg = _reg, \
>> + .rate_table = _rtable, \
>> + .mult = _mult, \
>> + .div = _div, \
>> + .frac = _frac \
>> + }
>> +
>> +/**
>> + * struct intel_osc_clk
>> + * @id: platform specific id of the clock.
>> + * @name: name of the osc clock.
>> + * @dt_freq: frequency node name in device tree.
>> + * @def_rate: default rate of the osc clock.
>> + * @flags: optional flags for basic clock.
> There aren't flags though. I'm very confused by this kernel-doc too.
> Looks like something that should be done with a fixed rate clk in DT.
Will remove the flags comments.
>> + */
>> +struct intel_osc_clk {
>> + unsigned int id;
>> + const char *name;
>> + const char *dt_freq;
>> + const u32 def_rate;
>> +};
>> +
>> +#define INTEL_OSC(_id, _name, _freq, _rate) \
>> + { \
>> + .id = _id, \
>> + .name = _name, \
>> + .dt_freq = _freq, \
>> + .def_rate = _rate, \
>> + }
>> +
>> +struct intel_clk_branch {
> Seems to be more like intel_clk instead of intel_clk_branch because it
> does lots of stuff.
Will update.
>> + unsigned int id;
>> + enum intel_clk_type type;
>> + const char *name;
>> + const char *const *parent_names;
>> + u8 num_parents;
>> + unsigned long flags;
>> + unsigned int mux_off;
>> + u8 mux_shift;
>> + u8 mux_width;
>> + unsigned long mux_flags;
>> + unsigned int mux_val;
>> + unsigned int div_off;
>> + u8 div_shift;
>> + u8 div_width;
>> + unsigned long div_flags;
>> + unsigned int div_val;
>> + const struct clk_div_table *div_table;
>> + unsigned int gate_off;
>> + u8 gate_shift;
>> + unsigned long gate_flags;
>> + unsigned int gate_val;
>> + unsigned int mult;
>> + unsigned int div;
>> +};
>> +
>> +/* clock flags definition */
>> +#define CLOCK_FLAG_VAL_INIT BIT(16)
>> +#define GATE_CLK_HW BIT(17)
>> +#define GATE_CLK_SW BIT(18)
>> +#define GATE_CLK_VT BIT(19)
> What does VT mean? Virtual?
Yes. VT means virtual here.
Will change to GATE_CLK_VIRT.
>> +
>> +#define INTEL_MUX(_id, _name, _pname, _f, _reg, \
>> + _shift, _width, _cf, _v) \
>> + { \
>> + .id = _id, \
>> + .type = intel_clk_mux, \
>> + .name = _name, \
>> + .parent_names = _pname, \
>> + .num_parents = ARRAY_SIZE(_pname), \
>> + .flags = _f, \
>> + .mux_off = _reg, \
>> + .mux_shift = _shift, \
>> + .mux_width = _width, \
>> + .mux_flags = _cf, \
>> + .mux_val = _v, \
>> + }
>> +
>> +#define INTEL_DIV(_id, _name, _pname, _f, _reg, \
>> + _shift, _width, _cf, _v, _dtable) \
>> + { \
>> + .id = _id, \
>> + .type = intel_clk_divider, \
>> + .name = _name, \
>> + .parent_names = (const char *[]) { _pname }, \
>> + .num_parents = 1, \
>> + .flags = _f, \
>> + .div_off = _reg, \
>> + .div_shift = _shift, \
>> + .div_width = _width, \
>> + .div_flags = _cf, \
>> + .div_val = _v, \
>> + .div_table = _dtable, \
>> + }
>> +
>> +#define INTEL_GATE(_id, _name, _pname, _f, _reg, \
>> + _shift, _cf, _v) \
>> + { \
>> + .id = _id, \
>> + .type = intel_clk_gate, \
>> + .name = _name, \
>> + .parent_names = (const char *[]) { _pname }, \
>> + .num_parents = !_pname ? 0 : 1, \
>> + .flags = _f, \
>> + .gate_off = _reg, \
>> + .gate_shift = _shift, \
>> + .gate_flags = _cf, \
>> + .gate_val = _v, \
>> + }
>> +
>> +#define INTEL_FIXED(_id, _name, _pname, _f, _reg, \
>> + _shift, _width, _cf, _freq, _v) \
>> + { \
>> + .id = _id, \
>> + .type = intel_clk_fixed, \
>> + .name = _name, \
>> + .parent_names = (const char *[]) { _pname }, \
>> + .num_parents = !_pname ? 0 : 1, \
>> + .flags = _f, \
>> + .div_off = _reg, \
>> + .div_shift = _shift, \
>> + .div_width = _width, \
>> + .div_flags = _cf, \
>> + .div_val = _v, \
>> + .mux_flags = _freq, \
>> + }
>> +
>> +#define INTEL_FIXED_FACTOR(_id, _name, _pname, _f, _reg, \
>> + _shift, _width, _cf, _v, _m, _d) \
>> + { \
>> + .id = _id, \
>> + .type = intel_clk_fixed_factor, \
>> + .name = _name, \
>> + .parent_names = (const char *[]) { _pname }, \
>> + .num_parents = 1, \
>> + .flags = _f, \
>> + .div_off = _reg, \
>> + .div_shift = _shift, \
>> + .div_width = _width, \
>> + .div_flags = _cf, \
>> + .div_val = _v, \
>> + .mult = _m, \
>> + .div = _d, \
>> + }
>> +
>> +void intel_set_clk_val(struct regmap *map, u32 reg, u8 shift,
>> + u8 width, u32 set_val);
>> +u32 intel_get_clk_val(struct regmap *map, u32 reg, u8 shift, u8 width);
>> +void intel_clk_add_lookup(struct intel_clk_provider *ctx,
>> + struct clk *clk, unsigned int id);
>> +void __init intel_clk_of_add_provider(struct device_node *np,
>> + struct intel_clk_provider *ctx);
>> +struct intel_clk_provider * __init
>> +intel_clk_init(struct device_node *np, struct regmap *map,
>> + unsigned int nr_clks);
>> +void __init intel_clk_register_osc(struct intel_clk_provider *ctx,
>> + struct intel_osc_clk *osc,
>> + unsigned int nr_clks);
> Remove __init from headers files. It does nothing.
Will remove it.
>
>> +void intel_clk_register_branches(struct intel_clk_provider *ctx,
>> + struct intel_clk_branch *list,
>> + unsigned int nr_clk);
>> +void intel_clk_register_plls(struct intel_clk_provider *ctx,
>> + struct intel_pll_clk *list, unsigned int nr_clk);
>> +#endif /* __INTEL_CLK_H */
>> diff --git a/drivers/clk/intel/clk-grx500.c b/drivers/clk/intel/clk-grx500.c
>> new file mode 100644
>> index 000000000000..5c2546f82579
>> --- /dev/null
>> +++ b/drivers/clk/intel/clk-grx500.c
>> @@ -0,0 +1,168 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Intel Corporation.
>> + * Zhu YiXin <Yixin.zhu@intel.com>
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spinlock.h>
> Used?
Will remove it
>> +#include <dt-bindings/clock/intel,grx500-clk.h>
>> +
>> +#include "clk-cgu-pll.h"
>> +#include "clk-cgu.h"
>> +
>> +#define PLL_DIV_WIDTH 4
>> +
>> +/* Gate1 clock shift */
>> +#define G_VCODEC_SHIFT 2
>> +#define G_DMA0_SHIFT 5
>> +#define G_USB0_SHIFT 6
>> +#define G_SPI1_SHIFT 7
>> +#define G_SPI0_SHIFT 8
>> +#define G_CBM_SHIFT 9
>> +#define G_EBU_SHIFT 10
>> +#define G_SSO_SHIFT 11
>> +#define G_GPTC0_SHIFT 12
>> +#define G_GPTC1_SHIFT 13
>> +#define G_GPTC2_SHIFT 14
>> +#define G_UART_SHIFT 17
>> +#define G_CPYTO_SHIFT 20
>> +#define G_SECPT_SHIFT 21
>> +#define G_TOE_SHIFT 22
>> +#define G_MPE_SHIFT 23
>> +#define G_TDM_SHIFT 25
>> +#define G_PAE_SHIFT 26
>> +#define G_USB1_SHIFT 27
>> +#define G_SWITCH_SHIFT 28
>> +
>> +/* Gate2 clock shift */
>> +#define G_PCIE0_SHIFT 1
>> +#define G_PCIE1_SHIFT 17
>> +#define G_PCIE2_SHIFT 25
>> +
>> +/* Register definition */
>> +#define GRX500_PLL0A_CFG0 0x0004
>> +#define GRX500_PLL0A_CFG1 0x0008
>> +#define GRX500_PLL0B_CFG0 0x0034
>> +#define GRX500_PLL0B_CFG1 0x0038
>> +#define GRX500_LCPLL_CFG0 0x0094
>> +#define GRX500_LCPLL_CFG1 0x0098
>> +#define GRX500_IF_CLK 0x00c4
>> +#define GRX500_CLK_GSR1 0x0120
>> +#define GRX500_CLK_GSR2 0x0130
>> +
>> +static const struct clk_div_table pll_div[] = {
>> + {1, 2},
> Please write it like
>
> { 1, 2 },
>
> instead.
Will update.
>> + {2, 3},
>> + {3, 4},
>> + {4, 5},
>> + {5, 6},
>> + {6, 8},
>> + {7, 10},
>> + {8, 12},
>> + {9, 16},
>> + {10, 20},
>> + {11, 24},
>> + {12, 32},
>> + {13, 40},
>> + {14, 48},
>> + {15, 64}
>> +};
>> +
>> +enum grx500_plls {
>> + pll0a, pll0b, pll3,
>> +};
> What's the point of the enum?
Will remove it.
>> +
>> +PNAME(pll_p) = { "osc" };
>> +PNAME(cpu_p) = { "cpu0", "cpu1" };
>> +
>> +static struct intel_osc_clk grx500_osc_clks[] __initdata = {
>> + INTEL_OSC(CLK_OSC, "osc", "intel,osc-frequency", 40000000),
>> +};
>> +
>> +static struct intel_pll_clk grx500_pll_clks[] __initdata = {
>> + [pll0a] = INTEL_PLL(CLK_PLL0A, pll_grx500, "pll0a",
>> + pll_p, 0, GRX500_PLL0A_CFG0, NULL, 0, 0, 0),
>> + [pll0b] = INTEL_PLL(CLK_PLL0B, pll_grx500, "pll0b",
>> + pll_p, 0, GRX500_PLL0B_CFG0, NULL, 0, 0, 0),
>> + [pll3] = INTEL_PLL(CLK_PLL3, pll_grx500, "pll3",
>> + pll_p, 0, GRX500_LCPLL_CFG0, NULL, 0, 0, 0),
>> +};
>> +
>> +static struct intel_clk_branch grx500_branch_clks[] __initdata = {
>> + INTEL_DIV(CLK_CBM, "cbm", "pll0a", 0, GRX500_PLL0A_CFG1,
>> + 0, PLL_DIV_WIDTH, 0, 0, pll_div),
>> + INTEL_DIV(CLK_NGI, "ngi", "pll0a", 0, GRX500_PLL0A_CFG1,
>> + 4, PLL_DIV_WIDTH, 0, 0, pll_div),
>> + INTEL_DIV(CLK_SSX4, "ssx4", "pll0a", 0, GRX500_PLL0A_CFG1,
>> + 8, PLL_DIV_WIDTH, 0, 0, pll_div),
>> + INTEL_DIV(CLK_CPU0, "cpu0", "pll0a", 0, GRX500_PLL0A_CFG1,
>> + 12, PLL_DIV_WIDTH, 0, 0, pll_div),
>> + INTEL_DIV(CLK_PAE, "pae", "pll0b", 0, GRX500_PLL0B_CFG1,
>> + 0, PLL_DIV_WIDTH, 0, 0, pll_div),
>> + INTEL_DIV(CLK_GSWIP, "gswip", "pll0b", 0, GRX500_PLL0B_CFG1,
>> + 4, PLL_DIV_WIDTH, 0, 0, pll_div),
>> + INTEL_DIV(CLK_DDR, "ddr", "pll0b", 0, GRX500_PLL0B_CFG1,
>> + 8, PLL_DIV_WIDTH, 0, 0, pll_div),
>> + INTEL_DIV(CLK_CPU1, "cpu1", "pll0b", 0, GRX500_PLL0B_CFG1,
>> + 12, PLL_DIV_WIDTH, 0, 0, pll_div),
>> + INTEL_MUX(CLK_CPU, "cpu", cpu_p, CLK_SET_RATE_PARENT,
>> + GRX500_PLL0A_CFG1, 29, 1, 0, 0),
>> + INTEL_GATE(GCLK_DMA0, "g_dma0", NULL, 0, GRX500_CLK_GSR1,
>> + G_DMA0_SHIFT, GATE_CLK_HW, 0),
>> + INTEL_GATE(GCLK_USB0, "g_usb0", NULL, 0, GRX500_CLK_GSR1,
>> + G_USB0_SHIFT, GATE_CLK_HW, 0),
>> + INTEL_GATE(GCLK_GPTC0, "g_gptc0", NULL, 0, GRX500_CLK_GSR1,
>> + G_GPTC0_SHIFT, GATE_CLK_HW, 0),
>> + INTEL_GATE(GCLK_GPTC1, "g_gptc1", NULL, 0, GRX500_CLK_GSR1,
>> + G_GPTC1_SHIFT, GATE_CLK_HW, 0),
>> + INTEL_GATE(GCLK_GPTC2, "g_gptc2", NULL, 0, GRX500_CLK_GSR1,
>> + G_GPTC2_SHIFT, GATE_CLK_HW, 0),
>> + INTEL_GATE(GCLK_UART, "g_uart", NULL, 0, GRX500_CLK_GSR1,
>> + G_UART_SHIFT, GATE_CLK_HW, 0),
>> + INTEL_GATE(GCLK_PCIE0, "g_pcie0", NULL, 0, GRX500_CLK_GSR2,
>> + G_PCIE0_SHIFT, GATE_CLK_HW, 0),
>> + INTEL_GATE(GCLK_PCIE1, "g_pcie1", NULL, 0, GRX500_CLK_GSR2,
>> + G_PCIE1_SHIFT, GATE_CLK_HW, 0),
>> + INTEL_GATE(GCLK_PCIE2, "g_pcie2", NULL, 0, GRX500_CLK_GSR2,
>> + G_PCIE2_SHIFT, GATE_CLK_HW, 0),
>> + INTEL_GATE(GCLK_I2C, "g_i2c", NULL, 0, 0, 0, GATE_CLK_VT, 0),
>> + INTEL_FIXED(CLK_VOICE, "voice", NULL, 0, GRX500_IF_CLK, 14, 2,
>> + CLOCK_FLAG_VAL_INIT, 8192000, 2),
>> + INTEL_FIXED_FACTOR(CLK_DDRPHY, "ddrphy", "ddr", 0, 0, 0,
>> + 0, 0, 0, 2, 1),
>> + INTEL_FIXED_FACTOR(CLK_PCIE, "pcie", "pll3", 0, 0, 0,
>> + 0, 0, 0, 1, 40),
>> +};
>> +
>> +static void __init grx500_clk_init(struct device_node *np)
>> +{
>> + struct intel_clk_provider *ctx;
>> + struct regmap *map;
>> +
>> + map = syscon_node_to_regmap(np);
>> + if (IS_ERR(map))
>> + return;
>> +
>> + ctx = intel_clk_init(np, map, CLK_NR_CLKS);
>> + if (IS_ERR(ctx)) {
>> + regmap_exit(map);
>> + return;
>> + }
>> +
>> + intel_clk_register_osc(ctx, grx500_osc_clks,
>> + ARRAY_SIZE(grx500_osc_clks));
>> + intel_clk_register_plls(ctx, grx500_pll_clks,
>> + ARRAY_SIZE(grx500_pll_clks));
>> + intel_clk_register_branches(ctx, grx500_branch_clks,
>> + ARRAY_SIZE(grx500_branch_clks));
>> + of_clk_add_provider(np, of_clk_src_onecell_get, &ctx->clk_data);
>> +
>> + pr_debug("%s clk init done!\n", __func__);
> Yay!!!
>
>> +}
>> +
>> +CLK_OF_DECLARE(intel_grx500_cgu, "intel,grx500-cgu", grx500_clk_init);
> Any reason a platform driver can't be used instead of CLK_OF_DECLARE()?
It provides CPU clock which is used in early boot stage.
^ permalink raw reply
* Re: [PATCH v2 08/18] serial: intel: Get serial id from dts
From: Geert Uytterhoeven @ 2018-08-08 8:33 UTC (permalink / raw)
To: songjun.wu
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu,
Linux MIPS Mailing List, linux-clk, open list:SERIAL DRIVERS,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg KH, Linux Kernel Mailing List, Jiri Slaby
In-Reply-To: <734bddbc-a5aa-d50d-0e7b-d8adc4d1afb2@linux.intel.com>
Hi Songjun,
On Wed, Aug 8, 2018 at 6:05 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote:
> On 8/7/2018 3:33 PM, Geert Uytterhoeven wrote:
> > On Fri, Aug 3, 2018 at 5:04 AM Songjun Wu <songjun.wu@linux.intel.com> wrote:
> >> Get serial id from dts.
> >>
> >> "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
> >> macro is defined in lantiq_soc.h.
> >> lantiq_soc.h is in arch path for legacy product support.
> >>
> >> arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
> >>
> >> If "#ifdef preprocessor" is changed to
> >> "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
> >> code using LTQ_EARLY_ASC is compiled.
> >> Compilation will fail for no LTQ_EARLY_ASC defined.
> >>
> >> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
> > Thanks for your patch!
> >
> >> @@ -699,9 +700,19 @@ lqasc_probe(struct platform_device *pdev)
> >> return -ENODEV;
> >> }
> >>
> >> - /* check if this is the console port */
> >> - if (mmres->start != CPHYSADDR(LTQ_EARLY_ASC))
> >> - line = 1;
> >> + /* get serial id */
> >> + line = of_alias_get_id(node, "serial");
> >> + if (line < 0) {
> >> +#ifdef CONFIG_LANTIQ
> >> + if (mmres->start == CPHYSADDR(LTQ_EARLY_ASC))
> >> + line = 0;
> >> + else
> >> + line = 1;
> >> +#else
> >> + dev_err(&pdev->dev, "failed to get alias id, errno %d\n", line);
> >> + return line;
> > Please note that not providing a fallback here makes life harder when using
> > DT overlays.
> > See the description of commit 7678f4c20fa7670f ("serial: sh-sci: Add support
> > for dynamic instances") for background info.
> Thanks for your comment.
> The logic in commit 7678f4c20fa7670f is not suitable here.
> We need to know which serial instance is used for console.
> We cannot use dynamic serial instance here.
Why does the driver need to use which serial instance is used for the console?
Hardcoding that is not an option, as the board DTS may specify the console using
chosen/stdout-path.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox