This patchset aims to:
* Fix the incorrect bindings for the s4 type of pwm that was introduced
while converting the documentation from txt to yaml format.
* Introduce a new compatible for the existing PWMs to better describe the
HW in DT, instead of describing driver settings.
* Make the introduction of a new pwm variant (s4) slightly easier.
Changes since v3 [3]:
* Split first rework patch into 3 changes
* Use dev_warn_once() to notify use of obsolete bindings
* Rebased on Uwe dev_err_probe() change.
Changes since v2 [2]:
* Drop DTS changes. These will be re-submitted later on. Possibly after
u-boot gets support for the new compatible to minimise conversion
problems.
* Position deprecated property correctly in dt-bindings for the old
meson8 type pwm bindings
* Reword commit description of patch #2 to make more obvious it does not
introduce a new HW support but fixes a bad bindings.
* Dropped Rob's Reviewed-by on patch #2. It seemed appropriate considering
the discussion on this change.
Changes since v1 [1]:
* Fix typo in the new binding compatible documentation
* Disallow clock-names for the new compatibles in the schema documenation
[1]: https://lore.kernel.org/linux-amlogic/[email protected]
[2]: https://lore.kernel.org/linux-amlogic/[email protected]
[3]: https://lore.kernel.org/linux-amlogic/[email protected]
Jerome Brunet (6):
dt-bindings: pwm: amlogic: fix s4 bindings
dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type
pwm: meson: generalize 4 inputs clock on meson8 pwm type
pwm: meson: use device data to carry information around
pwm: meson: don't carry internal clock elements around
pwm: meson: add generic compatible for meson8 to sm1
.../devicetree/bindings/pwm/pwm-amlogic.yaml | 113 ++++++-
drivers/pwm/pwm-meson.c | 291 +++++++++++-------
2 files changed, 276 insertions(+), 128 deletions(-)
--
2.42.0
Add a new compatible for the pwm found in the meson8 to sm1 Amlogic SoCs,
dealing with clocks differently. This does not enable new HW. It is meant
to fix a bad DT ABI for the currently supported HW.
The original clock bindings describe which input the PWM channel
multiplexer should pick among its possible parents, which are
hard-coded in the driver. As such, it is a setting tied to the driver
implementation and does not describe the HW.
The new bindings introduce here describe the clocks input of the PWM block
as they exist.
The old compatible is deprecated but kept to maintain ABI compatibility.
The SoC specific compatibles introduced match the SoC families supported
by the original bindings.
Signed-off-by: Jerome Brunet <[email protected]>
---
.../devicetree/bindings/pwm/pwm-amlogic.yaml | 50 +++++++++++++++++--
1 file changed, 46 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
index a1d382aacb82..eece390114a3 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
+++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
@@ -21,23 +21,35 @@ properties:
- amlogic,meson-g12a-ee-pwm
- amlogic,meson-g12a-ao-pwm-ab
- amlogic,meson-g12a-ao-pwm-cd
- - amlogic,meson-s4-pwm
+ deprecated: true
- items:
- const: amlogic,meson-gx-pwm
- const: amlogic,meson-gxbb-pwm
+ deprecated: true
- items:
- const: amlogic,meson-gx-ao-pwm
- const: amlogic,meson-gxbb-ao-pwm
+ deprecated: true
- items:
- const: amlogic,meson8-pwm
- const: amlogic,meson8b-pwm
+ deprecated: true
+ - const: amlogic,meson8-pwm-v2
+ - items:
+ - enum:
+ - amlogic,meson8b-pwm-v2
+ - amlogic,meson-gxbb-pwm-v2
+ - amlogic,meson-axg-pwm-v2
+ - amlogic,meson-g12-pwm-v2
+ - const: amlogic,meson8-pwm-v2
+ - const: amlogic,meson-s4-pwm
reg:
maxItems: 1
clocks:
minItems: 1
- maxItems: 2
+ maxItems: 4
clock-names:
minItems: 1
@@ -58,7 +70,6 @@ allOf:
compatible:
contains:
enum:
- - amlogic,meson8-pwm
- amlogic,meson8b-pwm
- amlogic,meson-gxbb-pwm
- amlogic,meson-gxbb-ao-pwm
@@ -68,11 +79,14 @@ allOf:
- amlogic,meson-g12a-ao-pwm-ab
- amlogic,meson-g12a-ao-pwm-cd
then:
- # Historic bindings tied to the driver implementation
+ # Obsolete historic bindings tied to the driver implementation
# The clocks provided here are meant to be matched with the input
# known (hard-coded) in the driver and used to select pwm clock
# source. Currently, the linux driver ignores this.
+ # This is kept to maintain ABI backward compatibility.
properties:
+ clocks:
+ maxItems: 2
clock-names:
oneOf:
- items:
@@ -81,6 +95,27 @@ allOf:
- const: clkin0
- const: clkin1
+ # Newer binding where clock describe the actual clock inputs of the pwm
+ # block. These are necessary but some inputs may be grounded.
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - amlogic,meson8-pwm-v2
+ then:
+ properties:
+ clocks:
+ minItems: 1
+ items:
+ - description: input clock 0 of the pwm block
+ - description: input clock 1 of the pwm block
+ - description: input clock 2 of the pwm block
+ - description: input clock 3 of the pwm block
+ clock-names: false
+ required:
+ - clocks
+
# Newer IP block take a single input per channel, instead of 4 inputs
# for both channels
- if:
@@ -110,6 +145,13 @@ examples:
clock-names = "clkin0", "clkin1";
#pwm-cells = <3>;
};
+ - |
+ pwm@2000 {
+ compatible = "amlogic,meson8-pwm-v2";
+ reg = <0x1000 0x10>;
+ clocks = <&xtal>, <0>, <&fdiv4>, <&fdiv5>;
+ #pwm-cells = <3>;
+ };
- |
pwm@1000 {
compatible = "amlogic,meson-s4-pwm";
--
2.42.0
s4 has been added to the compatible list while converting the Amlogic PWM
binding documentation from txt to yaml.
However, on the s4, the clock bindings have different meaning compared to
the previous SoCs.
On the previous SoCs the clock bindings used to describe which input the
PWM channel multiplexer should pick among its possible parents.
This is very much tied to the driver implementation, instead of describing
the HW for what it is. When support for the Amlogic PWM was first added,
how to deal with clocks through DT was not as clear as it nowadays.
The Linux driver now ignores this DT setting, but still relies on the
hard-coded list of clock sources.
On the s4, the input multiplexer is gone. The clock bindings actually
describe the clock as it exists, not a setting. The property has a
different meaning, even if it is still 2 clocks and it would pass the check
when support is actually added.
Also the s4 cannot work if the clocks are not provided, so the property no
longer optional.
Finally, for once it makes sense to see the input as being numbered
somehow. No need to bother with clock-names on the s4 type of PWM.
Fixes: 43a1c4ff3977 ("dt-bindings: pwm: Convert Amlogic Meson PWM binding")
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Jerome Brunet <[email protected]>
---
.../devicetree/bindings/pwm/pwm-amlogic.yaml | 67 ++++++++++++++++---
1 file changed, 58 insertions(+), 9 deletions(-)
diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
index 527864a4d855..a1d382aacb82 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
+++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
@@ -9,9 +9,6 @@ title: Amlogic PWM
maintainers:
- Heiner Kallweit <[email protected]>
-allOf:
- - $ref: pwm.yaml#
-
properties:
compatible:
oneOf:
@@ -43,12 +40,8 @@ properties:
maxItems: 2
clock-names:
- oneOf:
- - items:
- - enum: [clkin0, clkin1]
- - items:
- - const: clkin0
- - const: clkin1
+ minItems: 1
+ maxItems: 2
"#pwm-cells":
const: 3
@@ -57,6 +50,55 @@ required:
- compatible
- reg
+allOf:
+ - $ref: pwm.yaml#
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - amlogic,meson8-pwm
+ - amlogic,meson8b-pwm
+ - amlogic,meson-gxbb-pwm
+ - amlogic,meson-gxbb-ao-pwm
+ - amlogic,meson-axg-ee-pwm
+ - amlogic,meson-axg-ao-pwm
+ - amlogic,meson-g12a-ee-pwm
+ - amlogic,meson-g12a-ao-pwm-ab
+ - amlogic,meson-g12a-ao-pwm-cd
+ then:
+ # Historic bindings tied to the driver implementation
+ # The clocks provided here are meant to be matched with the input
+ # known (hard-coded) in the driver and used to select pwm clock
+ # source. Currently, the linux driver ignores this.
+ properties:
+ clock-names:
+ oneOf:
+ - items:
+ - enum: [clkin0, clkin1]
+ - items:
+ - const: clkin0
+ - const: clkin1
+
+ # Newer IP block take a single input per channel, instead of 4 inputs
+ # for both channels
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - amlogic,meson-s4-pwm
+ then:
+ properties:
+ clocks:
+ items:
+ - description: input clock of PWM channel A
+ - description: input clock of PWM channel B
+ clock-names: false
+ required:
+ - clocks
+
additionalProperties: false
examples:
@@ -68,3 +110,10 @@ examples:
clock-names = "clkin0", "clkin1";
#pwm-cells = <3>;
};
+ - |
+ pwm@1000 {
+ compatible = "amlogic,meson-s4-pwm";
+ reg = <0x1000 0x10>;
+ clocks = <&pwm_src_a>, <&pwm_src_b>;
+ #pwm-cells = <3>;
+ };
--
2.42.0
Meson8 pwm type always has 4 input clocks. Some inputs may be grounded,
like in the AO domain of some SoCs.
Drop the parent number parameter and make this is constant.
This is also done to make addition of generic meson8 compatible easier.
Signed-off-by: Jerome Brunet <[email protected]>
---
drivers/pwm/pwm-meson.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 2971bbf3b5e7..ef50c337f444 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -60,7 +60,7 @@
#define MISC_A_EN BIT(0)
#define MESON_NUM_PWMS 2
-#define MESON_MAX_MUX_PARENTS 4
+#define MESON_NUM_MUX_PARENTS 4
static struct meson_pwm_channel_data {
u8 reg_offset;
@@ -98,7 +98,6 @@ struct meson_pwm_channel {
struct meson_pwm_data {
const char * const *parent_names;
- unsigned int num_parents;
};
struct meson_pwm {
@@ -343,7 +342,6 @@ static const char * const pwm_meson8b_parent_names[] = {
static const struct meson_pwm_data pwm_meson8b_data = {
.parent_names = pwm_meson8b_parent_names,
- .num_parents = ARRAY_SIZE(pwm_meson8b_parent_names),
};
/*
@@ -351,12 +349,11 @@ static const struct meson_pwm_data pwm_meson8b_data = {
* The last 2 are grounded
*/
static const char * const pwm_gxbb_ao_parent_names[] = {
- "xtal", "clk81"
+ "xtal", "clk81", NULL, NULL,
};
static const struct meson_pwm_data pwm_gxbb_ao_data = {
.parent_names = pwm_gxbb_ao_parent_names,
- .num_parents = ARRAY_SIZE(pwm_gxbb_ao_parent_names),
};
static const char * const pwm_axg_ee_parent_names[] = {
@@ -365,7 +362,6 @@ static const char * const pwm_axg_ee_parent_names[] = {
static const struct meson_pwm_data pwm_axg_ee_data = {
.parent_names = pwm_axg_ee_parent_names,
- .num_parents = ARRAY_SIZE(pwm_axg_ee_parent_names),
};
static const char * const pwm_axg_ao_parent_names[] = {
@@ -374,7 +370,6 @@ static const char * const pwm_axg_ao_parent_names[] = {
static const struct meson_pwm_data pwm_axg_ao_data = {
.parent_names = pwm_axg_ao_parent_names,
- .num_parents = ARRAY_SIZE(pwm_axg_ao_parent_names),
};
static const char * const pwm_g12a_ao_ab_parent_names[] = {
@@ -383,16 +378,14 @@ static const char * const pwm_g12a_ao_ab_parent_names[] = {
static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
.parent_names = pwm_g12a_ao_ab_parent_names,
- .num_parents = ARRAY_SIZE(pwm_g12a_ao_ab_parent_names),
};
static const char * const pwm_g12a_ao_cd_parent_names[] = {
- "xtal", "g12a_ao_clk81",
+ "xtal", "g12a_ao_clk81", NULL, NULL,
};
static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
.parent_names = pwm_g12a_ao_cd_parent_names,
- .num_parents = ARRAY_SIZE(pwm_g12a_ao_cd_parent_names),
};
static const struct of_device_id meson_pwm_matches[] = {
@@ -434,13 +427,13 @@ MODULE_DEVICE_TABLE(of, meson_pwm_matches);
static int meson_pwm_init_channels(struct meson_pwm *meson)
{
- struct clk_parent_data mux_parent_data[MESON_MAX_MUX_PARENTS] = {};
+ struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
struct device *dev = meson->chip.dev;
unsigned int i;
char name[255];
int err;
- for (i = 0; i < meson->data->num_parents; i++) {
+ for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) {
mux_parent_data[i].index = -1;
mux_parent_data[i].name = meson->data->parent_names[i];
}
@@ -456,7 +449,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
init.ops = &clk_mux_ops;
init.flags = 0;
init.parent_data = mux_parent_data;
- init.num_parents = meson->data->num_parents;
+ init.num_parents = MESON_NUM_MUX_PARENTS;
channel->mux.reg = meson->base + REG_MISC_AB;
channel->mux.shift =
--
2.42.0
Pointers to the internal clock elements of the PWM are useless
after probe. There is no need to carry this around in the device
data. Just let devres deal with it.
Signed-off-by: Jerome Brunet <[email protected]>
---
drivers/pwm/pwm-meson.c | 67 ++++++++++++++++++++++++-----------------
1 file changed, 39 insertions(+), 28 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 15c44185d784..fb113bc8da29 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -90,9 +90,6 @@ struct meson_pwm_channel {
unsigned int hi;
unsigned int lo;
- struct clk_mux mux;
- struct clk_divider div;
- struct clk_gate gate;
struct clk *clk;
};
@@ -442,6 +439,13 @@ static int meson_pwm_init_channels(struct device *dev)
struct meson_pwm_channel *channel = &meson->channels[i];
struct clk_parent_data div_parent = {}, gate_parent = {};
struct clk_init_data init = {};
+ struct clk_divider *div;
+ struct clk_gate *gate;
+ struct clk_mux *mux;
+
+ mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+ if (!mux)
+ return -ENOMEM;
snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
@@ -451,63 +455,70 @@ static int meson_pwm_init_channels(struct device *dev)
init.parent_data = mux_parent_data;
init.num_parents = MESON_NUM_MUX_PARENTS;
- channel->mux.reg = meson->base + REG_MISC_AB;
- channel->mux.shift =
- meson_pwm_per_channel_data[i].clk_sel_shift;
- channel->mux.mask = MISC_CLK_SEL_MASK;
- channel->mux.flags = 0;
- channel->mux.lock = &meson->lock;
- channel->mux.table = NULL;
- channel->mux.hw.init = &init;
+ mux->reg = meson->base + REG_MISC_AB;
+ mux->shift = meson_pwm_per_channel_data[i].clk_sel_shift;
+ mux->mask = MISC_CLK_SEL_MASK;
+ mux->flags = 0;
+ mux->lock = &meson->lock;
+ mux->table = NULL;
+ mux->hw.init = &init;
- err = devm_clk_hw_register(dev, &channel->mux.hw);
+ err = devm_clk_hw_register(dev, &mux->hw);
if (err)
return dev_err_probe(dev, err,
"failed to register %s\n", name);
+ div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
+ if (!div)
+ return -ENOMEM;
+
snprintf(name, sizeof(name), "%s#div%u", dev_name(dev), i);
init.name = name;
init.ops = &clk_divider_ops;
init.flags = CLK_SET_RATE_PARENT;
div_parent.index = -1;
- div_parent.hw = &channel->mux.hw;
+ div_parent.hw = &mux->hw;
init.parent_data = &div_parent;
init.num_parents = 1;
- channel->div.reg = meson->base + REG_MISC_AB;
- channel->div.shift = meson_pwm_per_channel_data[i].clk_div_shift;
- channel->div.width = MISC_CLK_DIV_WIDTH;
- channel->div.hw.init = &init;
- channel->div.flags = 0;
- channel->div.lock = &meson->lock;
+ div->reg = meson->base + REG_MISC_AB;
+ div->shift = meson_pwm_per_channel_data[i].clk_div_shift;
+ div->width = MISC_CLK_DIV_WIDTH;
+ div->hw.init = &init;
+ div->flags = 0;
+ div->lock = &meson->lock;
- err = devm_clk_hw_register(dev, &channel->div.hw);
+ err = devm_clk_hw_register(dev, &div->hw);
if (err)
return dev_err_probe(dev, err,
"failed to register %s\n", name);
+ gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
+ if (!gate)
+ return -ENOMEM;
+
snprintf(name, sizeof(name), "%s#gate%u", dev_name(dev), i);
init.name = name;
init.ops = &clk_gate_ops;
init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
gate_parent.index = -1;
- gate_parent.hw = &channel->div.hw;
+ gate_parent.hw = &div->hw;
init.parent_data = &gate_parent;
init.num_parents = 1;
- channel->gate.reg = meson->base + REG_MISC_AB;
- channel->gate.bit_idx = meson_pwm_per_channel_data[i].clk_en_shift;
- channel->gate.hw.init = &init;
- channel->gate.flags = 0;
- channel->gate.lock = &meson->lock;
+ gate->reg = meson->base + REG_MISC_AB;
+ gate->bit_idx = meson_pwm_per_channel_data[i].clk_en_shift;
+ gate->hw.init = &init;
+ gate->flags = 0;
+ gate->lock = &meson->lock;
- err = devm_clk_hw_register(dev, &channel->gate.hw);
+ err = devm_clk_hw_register(dev, &gate->hw);
if (err)
return dev_err_probe(dev, err, "failed to register %s\n", name);
- channel->clk = devm_clk_hw_get_clk(dev, &channel->gate.hw, NULL);
+ channel->clk = devm_clk_hw_get_clk(dev, &gate->hw, NULL);
if (IS_ERR(channel->clk))
return dev_err_probe(dev, PTR_ERR(channel->clk),
"failed to register %s\n", name);
--
2.42.0
Use struct device data to carry the information data around, instead
of embedded the pwm structure in it and using container_of()
Doing so works just as well and makes it a little easier to add setup
callback depending on the DT compatible.
Signed-off-by: Jerome Brunet <[email protected]>
---
drivers/pwm/pwm-meson.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index ef50c337f444..15c44185d784 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -101,7 +101,6 @@ struct meson_pwm_data {
};
struct meson_pwm {
- struct pwm_chip chip;
const struct meson_pwm_data *data;
struct meson_pwm_channel channels[MESON_NUM_PWMS];
void __iomem *base;
@@ -114,7 +113,7 @@ struct meson_pwm {
static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip)
{
- return container_of(chip, struct meson_pwm, chip);
+ return dev_get_drvdata(chip->dev);
}
static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -146,6 +145,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
const struct pwm_state *state)
{
struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm];
+ struct device *dev = pwm->chip->dev;
unsigned int cnt, duty_cnt;
unsigned long fin_freq;
u64 duty, period, freq;
@@ -168,19 +168,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
fin_freq = clk_round_rate(channel->clk, freq);
if (fin_freq == 0) {
- dev_err(meson->chip.dev, "invalid source clock frequency\n");
+ dev_err(dev, "invalid source clock frequency\n");
return -EINVAL;
}
- dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
+ dev_dbg(dev, "fin_freq: %lu Hz\n", fin_freq);
cnt = div_u64(fin_freq * period, NSEC_PER_SEC);
if (cnt > 0xffff) {
- dev_err(meson->chip.dev, "unable to get period cnt\n");
+ dev_err(dev, "unable to get period cnt\n");
return -EINVAL;
}
- dev_dbg(meson->chip.dev, "period=%llu cnt=%u\n", period, cnt);
+ dev_dbg(dev, "period=%llu cnt=%u\n", period, cnt);
if (duty == period) {
channel->hi = cnt;
@@ -191,7 +191,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
} else {
duty_cnt = div_u64(fin_freq * duty, NSEC_PER_SEC);
- dev_dbg(meson->chip.dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
+ dev_dbg(dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
channel->hi = duty_cnt;
channel->lo = cnt - duty_cnt;
@@ -214,7 +214,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
err = clk_set_rate(channel->clk, channel->rate);
if (err)
- dev_err(meson->chip.dev, "setting clock rate failed\n");
+ dev_err(pwm->chip->dev, "setting clock rate failed\n");
spin_lock_irqsave(&meson->lock, flags);
@@ -425,10 +425,10 @@ static const struct of_device_id meson_pwm_matches[] = {
};
MODULE_DEVICE_TABLE(of, meson_pwm_matches);
-static int meson_pwm_init_channels(struct meson_pwm *meson)
+static int meson_pwm_init_channels(struct device *dev)
{
struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
- struct device *dev = meson->chip.dev;
+ struct meson_pwm *meson = dev_get_drvdata(dev);
unsigned int i;
char name[255];
int err;
@@ -438,7 +438,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
mux_parent_data[i].name = meson->data->parent_names[i];
}
- for (i = 0; i < meson->chip.npwm; i++) {
+ for (i = 0; i < MESON_NUM_PWMS; i++) {
struct meson_pwm_channel *channel = &meson->channels[i];
struct clk_parent_data div_parent = {}, gate_parent = {};
struct clk_init_data init = {};
@@ -519,28 +519,35 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
static int meson_pwm_probe(struct platform_device *pdev)
{
struct meson_pwm *meson;
+ struct pwm_chip *chip;
int err;
+ chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
meson = devm_kzalloc(&pdev->dev, sizeof(*meson), GFP_KERNEL);
if (!meson)
return -ENOMEM;
+ platform_set_drvdata(pdev, meson);
+
meson->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(meson->base))
return PTR_ERR(meson->base);
spin_lock_init(&meson->lock);
- meson->chip.dev = &pdev->dev;
- meson->chip.ops = &meson_pwm_ops;
- meson->chip.npwm = MESON_NUM_PWMS;
+ chip->dev = &pdev->dev;
+ chip->ops = &meson_pwm_ops;
+ chip->npwm = MESON_NUM_PWMS;
meson->data = of_device_get_match_data(&pdev->dev);
- err = meson_pwm_init_channels(meson);
+ err = meson_pwm_init_channels(&pdev->dev);
if (err < 0)
return err;
- err = devm_pwmchip_add(&pdev->dev, &meson->chip);
+ err = devm_pwmchip_add(&pdev->dev, chip);
if (err < 0)
return dev_err_probe(&pdev->dev, err,
"failed to register PWM chip\n");
--
2.42.0
Introduce a new compatible support in the Amlogic PWM driver.
The PWM HW is actually the same for all SoCs supported so far. A specific
compatible is needed only because the clock sources of the PWMs are
hard-coded in the driver.
It is better to have the clock source described in DT but this changes the
bindings so a new compatible must be introduced.
When all supported platform have migrated to the new compatible, support
for the legacy ones may be removed from the driver.
The addition of this new compatible makes the old ones obsolete, as
described in the DT documentation.
Adding a callback to setup the clock will also make it easier to add
support for the new PWM HW found in a1, s4, c3 and t7 SoC families.
Signed-off-by: Jerome Brunet <[email protected]>
---
drivers/pwm/pwm-meson.c | 240 ++++++++++++++++++++++++----------------
1 file changed, 143 insertions(+), 97 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index fb113bc8da29..9c0a8a6e4f48 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -95,6 +95,7 @@ struct meson_pwm_channel {
struct meson_pwm_data {
const char * const *parent_names;
+ int (*channels_init)(struct device *dev);
};
struct meson_pwm {
@@ -333,108 +334,14 @@ static const struct pwm_ops meson_pwm_ops = {
.get_state = meson_pwm_get_state,
};
-static const char * const pwm_meson8b_parent_names[] = {
- "xtal", NULL, "fclk_div4", "fclk_div3"
-};
-
-static const struct meson_pwm_data pwm_meson8b_data = {
- .parent_names = pwm_meson8b_parent_names,
-};
-
-/*
- * Only the 2 first inputs of the GXBB AO PWMs are valid
- * The last 2 are grounded
- */
-static const char * const pwm_gxbb_ao_parent_names[] = {
- "xtal", "clk81", NULL, NULL,
-};
-
-static const struct meson_pwm_data pwm_gxbb_ao_data = {
- .parent_names = pwm_gxbb_ao_parent_names,
-};
-
-static const char * const pwm_axg_ee_parent_names[] = {
- "xtal", "fclk_div5", "fclk_div4", "fclk_div3"
-};
-
-static const struct meson_pwm_data pwm_axg_ee_data = {
- .parent_names = pwm_axg_ee_parent_names,
-};
-
-static const char * const pwm_axg_ao_parent_names[] = {
- "xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5"
-};
-
-static const struct meson_pwm_data pwm_axg_ao_data = {
- .parent_names = pwm_axg_ao_parent_names,
-};
-
-static const char * const pwm_g12a_ao_ab_parent_names[] = {
- "xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5"
-};
-
-static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
- .parent_names = pwm_g12a_ao_ab_parent_names,
-};
-
-static const char * const pwm_g12a_ao_cd_parent_names[] = {
- "xtal", "g12a_ao_clk81", NULL, NULL,
-};
-
-static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
- .parent_names = pwm_g12a_ao_cd_parent_names,
-};
-
-static const struct of_device_id meson_pwm_matches[] = {
- {
- .compatible = "amlogic,meson8b-pwm",
- .data = &pwm_meson8b_data
- },
- {
- .compatible = "amlogic,meson-gxbb-pwm",
- .data = &pwm_meson8b_data
- },
- {
- .compatible = "amlogic,meson-gxbb-ao-pwm",
- .data = &pwm_gxbb_ao_data
- },
- {
- .compatible = "amlogic,meson-axg-ee-pwm",
- .data = &pwm_axg_ee_data
- },
- {
- .compatible = "amlogic,meson-axg-ao-pwm",
- .data = &pwm_axg_ao_data
- },
- {
- .compatible = "amlogic,meson-g12a-ee-pwm",
- .data = &pwm_meson8b_data
- },
- {
- .compatible = "amlogic,meson-g12a-ao-pwm-ab",
- .data = &pwm_g12a_ao_ab_data
- },
- {
- .compatible = "amlogic,meson-g12a-ao-pwm-cd",
- .data = &pwm_g12a_ao_cd_data
- },
- {},
-};
-MODULE_DEVICE_TABLE(of, meson_pwm_matches);
-
-static int meson_pwm_init_channels(struct device *dev)
+static int meson_pwm_init_clocks_legacy(struct device *dev,
+ struct clk_parent_data *mux_parent_data)
{
- struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
struct meson_pwm *meson = dev_get_drvdata(dev);
unsigned int i;
char name[255];
int err;
- for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) {
- mux_parent_data[i].index = -1;
- mux_parent_data[i].name = meson->data->parent_names[i];
- }
-
for (i = 0; i < MESON_NUM_PWMS; i++) {
struct meson_pwm_channel *channel = &meson->channels[i];
struct clk_parent_data div_parent = {}, gate_parent = {};
@@ -527,6 +434,145 @@ static int meson_pwm_init_channels(struct device *dev)
return 0;
}
+static int meson_pwm_init_channels_legacy(struct device *dev)
+{
+ struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
+ struct meson_pwm *meson = dev_get_drvdata(dev);
+ int i;
+
+ dev_warn_once(dev, "using obsolete compatible, please consider updating dt\n");
+
+ for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) {
+ mux_parent_data[i].index = -1;
+ mux_parent_data[i].name = meson->data->parent_names[i];
+ }
+
+ return meson_pwm_init_clocks_legacy(dev, mux_parent_data);
+}
+
+static int meson_pwm_init_channels_meson8b_v2(struct device *dev)
+{
+ struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
+ int i;
+
+ /*
+ * NOTE: Instead of relying on the hard coded names in the driver
+ * as the legacy version, this relies on DT to provide the list of
+ * clocks.
+ * For once, using input numbers actually makes more sense than names.
+ * Also DT requires clock-names to be explicitly ordered, so there is
+ * no point bothering with clock names in this case.
+ */
+ for (i = 0; i < MESON_NUM_MUX_PARENTS; i++)
+ mux_parent_data[i].index = i;
+
+ return meson_pwm_init_clocks_legacy(dev, mux_parent_data);
+}
+
+static const char * const pwm_meson8b_parent_names[] = {
+ "xtal", NULL, "fclk_div4", "fclk_div3"
+};
+
+static const struct meson_pwm_data pwm_meson8b_data = {
+ .parent_names = pwm_meson8b_parent_names,
+ .channels_init = meson_pwm_init_channels_legacy,
+};
+
+/*
+ * Only the 2 first inputs of the GXBB AO PWMs are valid
+ * The last 2 are grounded
+ */
+static const char * const pwm_gxbb_ao_parent_names[] = {
+ "xtal", "clk81", NULL, NULL,
+};
+
+static const struct meson_pwm_data pwm_gxbb_ao_data = {
+ .parent_names = pwm_gxbb_ao_parent_names,
+ .channels_init = meson_pwm_init_channels_legacy,
+};
+
+static const char * const pwm_axg_ee_parent_names[] = {
+ "xtal", "fclk_div5", "fclk_div4", "fclk_div3"
+};
+
+static const struct meson_pwm_data pwm_axg_ee_data = {
+ .parent_names = pwm_axg_ee_parent_names,
+ .channels_init = meson_pwm_init_channels_legacy,
+};
+
+static const char * const pwm_axg_ao_parent_names[] = {
+ "xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5"
+};
+
+static const struct meson_pwm_data pwm_axg_ao_data = {
+ .parent_names = pwm_axg_ao_parent_names,
+ .channels_init = meson_pwm_init_channels_legacy,
+};
+
+static const char * const pwm_g12a_ao_ab_parent_names[] = {
+ "xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5"
+};
+
+static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
+ .parent_names = pwm_g12a_ao_ab_parent_names,
+ .channels_init = meson_pwm_init_channels_legacy,
+};
+
+static const char * const pwm_g12a_ao_cd_parent_names[] = {
+ "xtal", "g12a_ao_clk81", NULL, NULL,
+};
+
+static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
+ .parent_names = pwm_g12a_ao_cd_parent_names,
+ .channels_init = meson_pwm_init_channels_legacy,
+};
+
+static const struct meson_pwm_data pwm_meson8_v2_data = {
+ .channels_init = meson_pwm_init_channels_meson8b_v2,
+};
+
+static const struct of_device_id meson_pwm_matches[] = {
+ {
+ .compatible = "amlogic,meson8-pwm-v2",
+ .data = &pwm_meson8_v2_data
+ },
+ /* The following compatibles are obsolete */
+ {
+ .compatible = "amlogic,meson8b-pwm",
+ .data = &pwm_meson8b_data
+ },
+ {
+ .compatible = "amlogic,meson-gxbb-pwm",
+ .data = &pwm_meson8b_data
+ },
+ {
+ .compatible = "amlogic,meson-gxbb-ao-pwm",
+ .data = &pwm_gxbb_ao_data
+ },
+ {
+ .compatible = "amlogic,meson-axg-ee-pwm",
+ .data = &pwm_axg_ee_data
+ },
+ {
+ .compatible = "amlogic,meson-axg-ao-pwm",
+ .data = &pwm_axg_ao_data
+ },
+ {
+ .compatible = "amlogic,meson-g12a-ee-pwm",
+ .data = &pwm_meson8b_data
+ },
+ {
+ .compatible = "amlogic,meson-g12a-ao-pwm-ab",
+ .data = &pwm_g12a_ao_ab_data
+ },
+ {
+ .compatible = "amlogic,meson-g12a-ao-pwm-cd",
+ .data = &pwm_g12a_ao_cd_data
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, meson_pwm_matches);
+
static int meson_pwm_probe(struct platform_device *pdev)
{
struct meson_pwm *meson;
@@ -554,7 +600,7 @@ static int meson_pwm_probe(struct platform_device *pdev)
meson->data = of_device_get_match_data(&pdev->dev);
- err = meson_pwm_init_channels(&pdev->dev);
+ err = meson->data->channels_init(&pdev->dev);
if (err < 0)
return err;
--
2.42.0
On 22/12/2023 12:16, Jerome Brunet wrote:
> Add a new compatible for the pwm found in the meson8 to sm1 Amlogic SoCs,
> dealing with clocks differently. This does not enable new HW. It is meant
> to fix a bad DT ABI for the currently supported HW.
>
> The original clock bindings describe which input the PWM channel
> multiplexer should pick among its possible parents, which are
> hard-coded in the driver. As such, it is a setting tied to the driver
> implementation and does not describe the HW.
>
> The new bindings introduce here describe the clocks input of the PWM block
> as they exist.
>
> The old compatible is deprecated but kept to maintain ABI compatibility.
>
> The SoC specific compatibles introduced match the SoC families supported
> by the original bindings.
>
> Signed-off-by: Jerome Brunet <[email protected]>
> ---
> .../devicetree/bindings/pwm/pwm-amlogic.yaml | 50 +++++++++++++++++--
> 1 file changed, 46 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> index a1d382aacb82..eece390114a3 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> @@ -21,23 +21,35 @@ properties:
> - amlogic,meson-g12a-ee-pwm
> - amlogic,meson-g12a-ao-pwm-ab
> - amlogic,meson-g12a-ao-pwm-cd
> - - amlogic,meson-s4-pwm
> + deprecated: true
> - items:
> - const: amlogic,meson-gx-pwm
> - const: amlogic,meson-gxbb-pwm
> + deprecated: true
> - items:
> - const: amlogic,meson-gx-ao-pwm
> - const: amlogic,meson-gxbb-ao-pwm
> + deprecated: true
> - items:
> - const: amlogic,meson8-pwm
> - const: amlogic,meson8b-pwm
> + deprecated: true
> + - const: amlogic,meson8-pwm-v2
> + - items:
> + - enum:
> + - amlogic,meson8b-pwm-v2
> + - amlogic,meson-gxbb-pwm-v2
> + - amlogic,meson-axg-pwm-v2
> + - amlogic,meson-g12-pwm-v2
> + - const: amlogic,meson8-pwm-v2
> + - const: amlogic,meson-s4-pwm
If there is going to be resend:
This and amlogic,meson8-pwm-v2 before items: should be just in enum,
it's easier to read.
>
> reg:
> maxItems: 1
>
> clocks:
> minItems: 1
> - maxItems: 2
> + maxItems: 4
>
> clock-names:
> minItems: 1
> @@ -58,7 +70,6 @@ allOf:
> compatible:
> contains:
> enum:
> - - amlogic,meson8-pwm
I don't understand why removing it here.
The rest looks ok.
Best regards,
Krzysztof
Hello,
On Fri, Dec 22, 2023 at 12:16:50PM +0100, Jerome Brunet wrote:
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> index a1d382aacb82..eece390114a3 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> @@ -21,23 +21,35 @@ properties:
> - amlogic,meson-g12a-ee-pwm
> - amlogic,meson-g12a-ao-pwm-ab
> - amlogic,meson-g12a-ao-pwm-cd
> - - amlogic,meson-s4-pwm
> + deprecated: true
> - items:
> - const: amlogic,meson-gx-pwm
> - const: amlogic,meson-gxbb-pwm
> + deprecated: true
> - items:
> - const: amlogic,meson-gx-ao-pwm
> - const: amlogic,meson-gxbb-ao-pwm
> + deprecated: true
> - items:
> - const: amlogic,meson8-pwm
> - const: amlogic,meson8b-pwm
> + deprecated: true
I think deprecating the old binding and adding a new compatible should
be done in two commits.
> + - const: amlogic,meson8-pwm-v2
> + - items:
> + - enum:
> + - amlogic,meson8b-pwm-v2
> + - amlogic,meson-gxbb-pwm-v2
> + - amlogic,meson-axg-pwm-v2
> + - amlogic,meson-g12-pwm-v2
> + - const: amlogic,meson8-pwm-v2
> + - const: amlogic,meson-s4-pwm
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Dec 22, 2023 at 12:16:49PM +0100, Jerome Brunet wrote:
> s4 has been added to the compatible list while converting the Amlogic PWM
> binding documentation from txt to yaml.
>
> However, on the s4, the clock bindings have different meaning compared to
> the previous SoCs.
>
> On the previous SoCs the clock bindings used to describe which input the
> PWM channel multiplexer should pick among its possible parents.
>
> This is very much tied to the driver implementation, instead of describing
> the HW for what it is. When support for the Amlogic PWM was first added,
> how to deal with clocks through DT was not as clear as it nowadays.
> The Linux driver now ignores this DT setting, but still relies on the
> hard-coded list of clock sources.
>
> On the s4, the input multiplexer is gone. The clock bindings actually
> describe the clock as it exists, not a setting. The property has a
> different meaning, even if it is still 2 clocks and it would pass the check
> when support is actually added.
>
> Also the s4 cannot work if the clocks are not provided, so the property no
> longer optional.
s/no/is no/
> Finally, for once it makes sense to see the input as being numbered
> somehow. No need to bother with clock-names on the s4 type of PWM.
>
> Fixes: 43a1c4ff3977 ("dt-bindings: pwm: Convert Amlogic Meson PWM binding")
> Reviewed-by: Rob Herring <[email protected]>
> Signed-off-by: Jerome Brunet <[email protected]>
> ---
> .../devicetree/bindings/pwm/pwm-amlogic.yaml | 67 ++++++++++++++++---
> 1 file changed, 58 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> index 527864a4d855..a1d382aacb82 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> @@ -9,9 +9,6 @@ title: Amlogic PWM
> maintainers:
> - Heiner Kallweit <[email protected]>
>
> -allOf:
> - - $ref: pwm.yaml#
> -
> properties:
> compatible:
> oneOf:
> @@ -43,12 +40,8 @@ properties:
> maxItems: 2
>
> clock-names:
> - oneOf:
> - - items:
> - - enum: [clkin0, clkin1]
> - - items:
> - - const: clkin0
> - - const: clkin1
> + minItems: 1
> + maxItems: 2
>
> "#pwm-cells":
> const: 3
> @@ -57,6 +50,55 @@ required:
> - compatible
> - reg
>
> +allOf:
> + - $ref: pwm.yaml#
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - amlogic,meson8-pwm
> + - amlogic,meson8b-pwm
> + - amlogic,meson-gxbb-pwm
> + - amlogic,meson-gxbb-ao-pwm
> + - amlogic,meson-axg-ee-pwm
> + - amlogic,meson-axg-ao-pwm
> + - amlogic,meson-g12a-ee-pwm
> + - amlogic,meson-g12a-ao-pwm-ab
> + - amlogic,meson-g12a-ao-pwm-cd
> + then:
> + # Historic bindings tied to the driver implementation
> + # The clocks provided here are meant to be matched with the input
> + # known (hard-coded) in the driver and used to select pwm clock
> + # source. Currently, the linux driver ignores this.
I admit I didn't understand the relevant difference between the old and
the new binding yet. (The driver currently doesn't support the s4,
right?) Is it possible to detect if the dt uses the old or the new
binding? If yes, I suggest to drop the old one from the binding and only
keep it in the driver for legacy systems.
> + properties:
> + clock-names:
> + oneOf:
> + - items:
> + - enum: [clkin0, clkin1]
> + - items:
> + - const: clkin0
> + - const: clkin1
> +
> + # Newer IP block take a single input per channel, instead of 4 inputs
> + # for both channels
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - amlogic,meson-s4-pwm
The expectation is that this list contains all compatibles that are not
included in the "historic" list above, right? Then maybe use "else:"
instead of another explicit list?
> + then:
> + properties:
> + clocks:
> + items:
> + - description: input clock of PWM channel A
> + - description: input clock of PWM channel B
> + clock-names: false
> + required:
> + - clocks
> +
> additionalProperties: false
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed 17 Jan 2024 at 10:58, Uwe Kleine-König <[email protected]> wrote:
> [[PGP Signed Part:Undecided]]
> Hello,
>
> On Fri, Dec 22, 2023 at 12:16:50PM +0100, Jerome Brunet wrote:
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> index a1d382aacb82..eece390114a3 100644
>> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> @@ -21,23 +21,35 @@ properties:
>> - amlogic,meson-g12a-ee-pwm
>> - amlogic,meson-g12a-ao-pwm-ab
>> - amlogic,meson-g12a-ao-pwm-cd
>> - - amlogic,meson-s4-pwm
>> + deprecated: true
>> - items:
>> - const: amlogic,meson-gx-pwm
>> - const: amlogic,meson-gxbb-pwm
>> + deprecated: true
>> - items:
>> - const: amlogic,meson-gx-ao-pwm
>> - const: amlogic,meson-gxbb-ao-pwm
>> + deprecated: true
>> - items:
>> - const: amlogic,meson8-pwm
>> - const: amlogic,meson8b-pwm
>> + deprecated: true
>
> I think deprecating the old binding and adding a new compatible should
> be done in two commits.
Hi Uwe,
There was the same comment on v3 and Krzysztof said it should be done
like this:
https://lore.kernel.org/linux-pwm/[email protected]
I tend to agree with Krzysztof on this but, as I previously said,
I don't really mind one way or the other. Just have to pick one.
>
>> + - const: amlogic,meson8-pwm-v2
>> + - items:
>> + - enum:
>> + - amlogic,meson8b-pwm-v2
>> + - amlogic,meson-gxbb-pwm-v2
>> + - amlogic,meson-axg-pwm-v2
>> + - amlogic,meson-g12-pwm-v2
>> + - const: amlogic,meson8-pwm-v2
>> + - const: amlogic,meson-s4-pwm
>
> Best regards
> Uwe
--
Jerome
Hello Jerome,
On Wed, Jan 17, 2024 at 11:16:31AM +0100, Jerome Brunet wrote:
> On Wed 17 Jan 2024 at 10:58, Uwe Kleine-K?nig <[email protected]> wrote:
> > [[PGP Signed Part:Undecided]]
> > Hello,
> >
> > On Fri, Dec 22, 2023 at 12:16:50PM +0100, Jerome Brunet wrote:
> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> >> index a1d382aacb82..eece390114a3 100644
> >> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> >> @@ -21,23 +21,35 @@ properties:
> >> - amlogic,meson-g12a-ee-pwm
> >> - amlogic,meson-g12a-ao-pwm-ab
> >> - amlogic,meson-g12a-ao-pwm-cd
> >> - - amlogic,meson-s4-pwm
Either I still didn't grasp all the details of this change, or removing
amlogic,meson-s4-pwm in this commit is wrong.
> >> + deprecated: true
> >> - items:
> >> - const: amlogic,meson-gx-pwm
> >> - const: amlogic,meson-gxbb-pwm
> >> + deprecated: true
> >> - items:
> >> - const: amlogic,meson-gx-ao-pwm
> >> - const: amlogic,meson-gxbb-ao-pwm
> >> + deprecated: true
> >> - items:
> >> - const: amlogic,meson8-pwm
> >> - const: amlogic,meson8b-pwm
> >> + deprecated: true
> >
> > I think deprecating the old binding and adding a new compatible should
> > be done in two commits.
>
> Hi Uwe,
>
> There was the same comment on v3 and Krzysztof said it should be done
> like this:
>
> https://lore.kernel.org/linux-pwm/[email protected]
>
> I tend to agree with Krzysztof on this but, as I previously said,
> I don't really mind one way or the other. Just have to pick one.
Ah, so the machines that used amlogic,meson-g12a-ee-pwm before are
supposed to use amlogic,meson-g12-pwm-v2 now. With that understood I
agree to you and Krzysztof.
I wonder if me not understanding that is a sign that the commit log
isn't optimal (or if it's only that I didn't properly read it :-).
Now that I understood the change better, the commit log is
understandable, but maybe still make it a bit more explicit that it
introduces a new way to formalize already supported hardware. Something
like:
dt-bindings: pwm: amlogic: Add a new binding for meson8 pwm types
The binding that is used up to now describe which input the PWM
channel multiplexer should pick among its possible parents,
which are hardcoded in the driver. This isn't a good binding in
the sense that it should describe hardware but not usage.
Add a new binding deprecating the old one that uses clocks in a
better way and how clocks are usually used today: The list of
clocks describe the inputs of the PWM block as they are realised
in hardware.
So deprecate the old bindings and introduce a compatible per SoC
family to replace these.
I think I'd understand that better, but that might be because I wrote
it and it's subjective?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed 17 Jan 2024 at 11:03, Uwe Kleine-König <[email protected]> wrote:
> [[PGP Signed Part:Undecided]]
> On Fri, Dec 22, 2023 at 12:16:49PM +0100, Jerome Brunet wrote:
>> s4 has been added to the compatible list while converting the Amlogic PWM
>> binding documentation from txt to yaml.
>>
>> However, on the s4, the clock bindings have different meaning compared to
>> the previous SoCs.
>>
>> On the previous SoCs the clock bindings used to describe which input the
>> PWM channel multiplexer should pick among its possible parents.
>>
>> This is very much tied to the driver implementation, instead of describing
>> the HW for what it is. When support for the Amlogic PWM was first added,
>> how to deal with clocks through DT was not as clear as it nowadays.
>> The Linux driver now ignores this DT setting, but still relies on the
>> hard-coded list of clock sources.
>>
>> On the s4, the input multiplexer is gone. The clock bindings actually
>> describe the clock as it exists, not a setting. The property has a
>> different meaning, even if it is still 2 clocks and it would pass the check
>> when support is actually added.
>>
>> Also the s4 cannot work if the clocks are not provided, so the property no
>> longer optional.
>
> s/no/is no/
>
>> Finally, for once it makes sense to see the input as being numbered
>> somehow. No need to bother with clock-names on the s4 type of PWM.
>>
>> Fixes: 43a1c4ff3977 ("dt-bindings: pwm: Convert Amlogic Meson PWM binding")
>> Reviewed-by: Rob Herring <[email protected]>
>> Signed-off-by: Jerome Brunet <[email protected]>
>> ---
>> .../devicetree/bindings/pwm/pwm-amlogic.yaml | 67 ++++++++++++++++---
>> 1 file changed, 58 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> index 527864a4d855..a1d382aacb82 100644
>> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> @@ -9,9 +9,6 @@ title: Amlogic PWM
>> maintainers:
>> - Heiner Kallweit <[email protected]>
>>
>> -allOf:
>> - - $ref: pwm.yaml#
>> -
>> properties:
>> compatible:
>> oneOf:
>> @@ -43,12 +40,8 @@ properties:
>> maxItems: 2
>>
>> clock-names:
>> - oneOf:
>> - - items:
>> - - enum: [clkin0, clkin1]
>> - - items:
>> - - const: clkin0
>> - - const: clkin1
>> + minItems: 1
>> + maxItems: 2
>>
>> "#pwm-cells":
>> const: 3
>> @@ -57,6 +50,55 @@ required:
>> - compatible
>> - reg
>>
>> +allOf:
>> + - $ref: pwm.yaml#
>> +
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - amlogic,meson8-pwm
>> + - amlogic,meson8b-pwm
>> + - amlogic,meson-gxbb-pwm
>> + - amlogic,meson-gxbb-ao-pwm
>> + - amlogic,meson-axg-ee-pwm
>> + - amlogic,meson-axg-ao-pwm
>> + - amlogic,meson-g12a-ee-pwm
>> + - amlogic,meson-g12a-ao-pwm-ab
>> + - amlogic,meson-g12a-ao-pwm-cd
>> + then:
>> + # Historic bindings tied to the driver implementation
>> + # The clocks provided here are meant to be matched with the input
>> + # known (hard-coded) in the driver and used to select pwm clock
>> + # source. Currently, the linux driver ignores this.
>
> I admit I didn't understand the relevant difference between the old and
> the new binding yet.
Let's try to explain differently then:
* So far each AML PWM IP has 2 pwm.
* Up to G12, 4 input PLL/clocks are wired to the HW IP and there
mux/div/gate to select which input to take.
- The historic bindings just described how to setup each of the 2
internal muxes - 2 optionnal clocks.
The actual 4 inputs names from CCF are hard coded in
the driver. This is a pretty bad description. The driver has been
updated since then to use CCF to figure out the best parent
according to pwm rate so this setting is ignored now.
- The 'new' bindings (introduced in patch #2) fixes the problem above
but the meaning of the clock binding is different. It describes the
actual HW parents - 4 clocks, optionnal since some are not wired on
some PWM blocks. To avoid breaking the ABI, a new compatible for
these SoC is introduced.
> (The driver currently doesn't support the s4, right?)
Indeed. I know Amlogic is preparing the support. I could do it in this
series but I prefer to encourage them to contribute. It should come
shortly after this series is merged.
Starting from s4 (prbably even a1 - up to people contributing this SoC
to say) the mux/div/gate is no longer in the PWM IP. It has moved to the
main clock controller. Again the clock binding describes something
different. It is the 2 input clocks of each block, they are mandatory
this time.
> Is it possible to detect if the dt uses the old or the new
> binding?
Apart from the compatible, no.
> If yes, I suggest to drop the old one from the binding and only
> keep it in the driver for legacy systems.
I don't this would be allowed by the DT maintainers.
My understanding is that the old binding doc is here to stay.
What could happen after sufficient time has past it remove the support
for the old/historic binding in the driver. Driver are not required to
support all possible bindings after all, especially if it is not used/tested
anymore.
>
>> + properties:
>> + clock-names:
>> + oneOf:
>> + - items:
>> + - enum: [clkin0, clkin1]
>> + - items:
>> + - const: clkin0
>> + - const: clkin1
>> +
>> + # Newer IP block take a single input per channel, instead of 4 inputs
>> + # for both channels
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - amlogic,meson-s4-pwm
>
> The expectation is that this list contains all compatibles that are not
> included in the "historic" list above, right? Then maybe use "else:"
> instead of another explicit list?
>
I suppose if, elseif, else is possible.
I've done it this way because is slightly easier for human to read the
doc and find what is related to what. If you this is important for you,
I can change it.
>> + then:
>> + properties:
>> + clocks:
>> + items:
>> + - description: input clock of PWM channel A
>> + - description: input clock of PWM channel B
>> + clock-names: false
>> + required:
>> + - clocks
>> +
>> additionalProperties: false
>
> Best regards
> Uwe
--
Jerome
On 17/01/2024 10:58, Uwe Kleine-König wrote:
>> - items:
>> - const: amlogic,meson-gx-pwm
>> - const: amlogic,meson-gxbb-pwm
>> + deprecated: true
>> - items:
>> - const: amlogic,meson-gx-ao-pwm
>> - const: amlogic,meson-gxbb-ao-pwm
>> + deprecated: true
>> - items:
>> - const: amlogic,meson8-pwm
>> - const: amlogic,meson8b-pwm
>> + deprecated: true
>
> I think deprecating the old binding and adding a new compatible should
> be done in two commits.
No, because you have a state with deprecated compatible and no correct
compatible. If deprecated property caused warnings, you would have
non-bisectable state.
Best regards,
Krzysztof
On 2024/1/17 18:30, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Wed 17 Jan 2024 at 11:03, Uwe Kleine-König <[email protected]> wrote:
>
>> [[PGP Signed Part:Undecided]]
>> On Fri, Dec 22, 2023 at 12:16:49PM +0100, Jerome Brunet wrote:
>>> s4 has been added to the compatible list while converting the Amlogic PWM
>>> binding documentation from txt to yaml.
>>>
>>> However, on the s4, the clock bindings have different meaning compared to
>>> the previous SoCs.
>>>
>>> On the previous SoCs the clock bindings used to describe which input the
>>> PWM channel multiplexer should pick among its possible parents.
>>>
>>> This is very much tied to the driver implementation, instead of describing
>>> the HW for what it is. When support for the Amlogic PWM was first added,
>>> how to deal with clocks through DT was not as clear as it nowadays.
>>> The Linux driver now ignores this DT setting, but still relies on the
>>> hard-coded list of clock sources.
>>>
>>> On the s4, the input multiplexer is gone. The clock bindings actually
>>> describe the clock as it exists, not a setting. The property has a
>>> different meaning, even if it is still 2 clocks and it would pass the check
>>> when support is actually added.
>>>
>>> Also the s4 cannot work if the clocks are not provided, so the property no
>>> longer optional.
>>
>> s/no/is no/
>>
>>> Finally, for once it makes sense to see the input as being numbered
>>> somehow. No need to bother with clock-names on the s4 type of PWM.
>>>
>>> Fixes: 43a1c4ff3977 ("dt-bindings: pwm: Convert Amlogic Meson PWM binding")
>>> Reviewed-by: Rob Herring <[email protected]>
>>> Signed-off-by: Jerome Brunet <[email protected]>
>>> ---
>>> .../devicetree/bindings/pwm/pwm-amlogic.yaml | 67 ++++++++++++++++---
>>> 1 file changed, 58 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>> index 527864a4d855..a1d382aacb82 100644
>>> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>> @@ -9,9 +9,6 @@ title: Amlogic PWM
>>> maintainers:
>>> - Heiner Kallweit <[email protected]>
>>>
>>> -allOf:
>>> - - $ref: pwm.yaml#
>>> -
>>> properties:
>>> compatible:
>>> oneOf:
>>> @@ -43,12 +40,8 @@ properties:
>>> maxItems: 2
>>>
>>> clock-names:
>>> - oneOf:
>>> - - items:
>>> - - enum: [clkin0, clkin1]
>>> - - items:
>>> - - const: clkin0
>>> - - const: clkin1
>>> + minItems: 1
>>> + maxItems: 2
>>>
>>> "#pwm-cells":
>>> const: 3
>>> @@ -57,6 +50,55 @@ required:
>>> - compatible
>>> - reg
>>>
>>> +allOf:
>>> + - $ref: pwm.yaml#
>>> +
>>> + - if:
>>> + properties:
>>> + compatible:
>>> + contains:
>>> + enum:
>>> + - amlogic,meson8-pwm
>>> + - amlogic,meson8b-pwm
>>> + - amlogic,meson-gxbb-pwm
>>> + - amlogic,meson-gxbb-ao-pwm
>>> + - amlogic,meson-axg-ee-pwm
>>> + - amlogic,meson-axg-ao-pwm
>>> + - amlogic,meson-g12a-ee-pwm
>>> + - amlogic,meson-g12a-ao-pwm-ab
>>> + - amlogic,meson-g12a-ao-pwm-cd
>>> + then:
>>> + # Historic bindings tied to the driver implementation
>>> + # The clocks provided here are meant to be matched with the input
>>> + # known (hard-coded) in the driver and used to select pwm clock
>>> + # source. Currently, the linux driver ignores this.
>>
>> I admit I didn't understand the relevant difference between the old and
>> the new binding yet.
>
> Let's try to explain differently then:
> * So far each AML PWM IP has 2 pwm.
> * Up to G12, 4 input PLL/clocks are wired to the HW IP and there
> mux/div/gate to select which input to take.
> - The historic bindings just described how to setup each of the 2
> internal muxes - 2 optionnal clocks.
> The actual 4 inputs names from CCF are hard coded in
> the driver. This is a pretty bad description. The driver has been
> updated since then to use CCF to figure out the best parent
> according to pwm rate so this setting is ignored now.
> - The 'new' bindings (introduced in patch #2) fixes the problem above
> but the meaning of the clock binding is different. It describes the
> actual HW parents - 4 clocks, optionnal since some are not wired on
> some PWM blocks. To avoid breaking the ABI, a new compatible for
> these SoC is introduced.
>
>> (The driver currently doesn't support the s4, right?)
>
> Indeed. I know Amlogic is preparing the support. I could do it in this
> series but I prefer to encourage them to contribute. It should come
> shortly after this series is merged.
Yes. Amlogic is waiting these patches of Jerome. after these merge, we
will start to rebase and submit s4 code.
-
Best Regards Junyi
On Fri, Dec 22, 2023 at 12:16:53PM +0100, Jerome Brunet wrote:
> Pointers to the internal clock elements of the PWM are useless
> after probe. There is no need to carry this around in the device
> data. Just let devres deal with it.
>
> Signed-off-by: Jerome Brunet <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 67 ++++++++++++++++++++++++-----------------
> 1 file changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 15c44185d784..fb113bc8da29 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -90,9 +90,6 @@ struct meson_pwm_channel {
> unsigned int hi;
> unsigned int lo;
>
> - struct clk_mux mux;
> - struct clk_divider div;
> - struct clk_gate gate;
> struct clk *clk;
> };
>
> @@ -442,6 +439,13 @@ static int meson_pwm_init_channels(struct device *dev)
> struct meson_pwm_channel *channel = &meson->channels[i];
> struct clk_parent_data div_parent = {}, gate_parent = {};
> struct clk_init_data init = {};
> + struct clk_divider *div;
> + struct clk_gate *gate;
> + struct clk_mux *mux;
> +
> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return -ENOMEM;
I don't like this change. While it doesn't increase the memory used, it
fragments the used memory and increases the overhead of memory
management and the number of devm allocations.
Are these members of meson_pwm_channel in the way for anything later?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed 24 Jan 2024 at 10:08, Uwe Kleine-König <[email protected]> wrote:
> [[PGP Signed Part:Undecided]]
> Hello Jerome,
>
> On Fri, Dec 22, 2023 at 12:16:51PM +0100, Jerome Brunet wrote:
>> Meson8 pwm type always has 4 input clocks. Some inputs may be grounded,
>> like in the AO domain of some SoCs.
>>
>> Drop the parent number parameter and make this is constant.
>> This is also done to make addition of generic meson8 compatible easier.
>>
>> Signed-off-by: Jerome Brunet <[email protected]>
>> ---
>> drivers/pwm/pwm-meson.c | 19 ++++++-------------
>> 1 file changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index 2971bbf3b5e7..ef50c337f444 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -60,7 +60,7 @@
>> #define MISC_A_EN BIT(0)
>>
>> #define MESON_NUM_PWMS 2
>> -#define MESON_MAX_MUX_PARENTS 4
>> +#define MESON_NUM_MUX_PARENTS 4
>>
>> static struct meson_pwm_channel_data {
>> u8 reg_offset;
>> @@ -98,7 +98,6 @@ struct meson_pwm_channel {
>>
>> struct meson_pwm_data {
>> const char * const *parent_names;
>
> I suggest to make this
>
> const char *parent_names[MESON_NUM_MUX_PARENTS];
Ok.
>
> to make it more explicit that really four entries are needed here. This
> also makes is unnecessary to add the additional NULL entries to
> pwm_gxbb_ao_parent_names and the other arrays.
I would normally agree but I'd prefer to be explicit.
There are some instance where the NULL is in the middle, this can't go
away. I think it looks if some inputs are explicitly NULL while the
other are implicit.
Of course, it is just a preference. I can remove these if that is
bothering you.
>
> Best regards
> Uwe
--
Jerome
On Wed 24 Jan 2024 at 10:02, Uwe Kleine-König <[email protected]> wrote:
> [[PGP Signed Part:Undecided]]
> On Fri, Dec 22, 2023 at 12:16:53PM +0100, Jerome Brunet wrote:
>> Pointers to the internal clock elements of the PWM are useless
>> after probe. There is no need to carry this around in the device
>> data. Just let devres deal with it.
>>
>> Signed-off-by: Jerome Brunet <[email protected]>
>> ---
>> drivers/pwm/pwm-meson.c | 67 ++++++++++++++++++++++++-----------------
>> 1 file changed, 39 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index 15c44185d784..fb113bc8da29 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -90,9 +90,6 @@ struct meson_pwm_channel {
>> unsigned int hi;
>> unsigned int lo;
>>
>> - struct clk_mux mux;
>> - struct clk_divider div;
>> - struct clk_gate gate;
>> struct clk *clk;
>> };
>>
>> @@ -442,6 +439,13 @@ static int meson_pwm_init_channels(struct device *dev)
>> struct meson_pwm_channel *channel = &meson->channels[i];
>> struct clk_parent_data div_parent = {}, gate_parent = {};
>> struct clk_init_data init = {};
>> + struct clk_divider *div;
>> + struct clk_gate *gate;
>> + struct clk_mux *mux;
>> +
>> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> + if (!mux)
>> + return -ENOMEM;
>
> I don't like this change. While it doesn't increase the memory used, it
> fragments the used memory and increases the overhead of memory
> management and the number of devm allocations.
>
> Are these members of meson_pwm_channel in the way for anything later?
Not really. It is just not useful on the SoCs which do use it and not
used at all starting from s4/a1.
What about a dedicated struct for the 3 clock elements and a single
devm_kzalloc() instead of 3 ?
>
> Best regards
> Uwe
--
Jerome
Hello Jerome,
On Fri, Dec 22, 2023 at 12:16:51PM +0100, Jerome Brunet wrote:
> Meson8 pwm type always has 4 input clocks. Some inputs may be grounded,
> like in the AO domain of some SoCs.
>
> Drop the parent number parameter and make this is constant.
> This is also done to make addition of generic meson8 compatible easier.
>
> Signed-off-by: Jerome Brunet <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 2971bbf3b5e7..ef50c337f444 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -60,7 +60,7 @@
> #define MISC_A_EN BIT(0)
>
> #define MESON_NUM_PWMS 2
> -#define MESON_MAX_MUX_PARENTS 4
> +#define MESON_NUM_MUX_PARENTS 4
>
> static struct meson_pwm_channel_data {
> u8 reg_offset;
> @@ -98,7 +98,6 @@ struct meson_pwm_channel {
>
> struct meson_pwm_data {
> const char * const *parent_names;
I suggest to make this
const char *parent_names[MESON_NUM_MUX_PARENTS];
to make it more explicit that really four entries are needed here. This
also makes is unnecessary to add the additional NULL entries to
pwm_gxbb_ao_parent_names and the other arrays.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed, Jan 24, 2024 at 10:11:59AM +0100, Jerome Brunet wrote:
> On Wed 24 Jan 2024 at 10:08, Uwe Kleine-K?nig <[email protected]> wrote:
> > I suggest to make this
> >
> > const char *parent_names[MESON_NUM_MUX_PARENTS];
>
> Ok.
>
> >
> > to make it more explicit that really four entries are needed here. This
> > also makes is unnecessary to add the additional NULL entries to
> > pwm_gxbb_ao_parent_names and the other arrays.
>
> I would normally agree but I'd prefer to be explicit.
>
> There are some instance where the NULL is in the middle, this can't go
> away. I think it looks if some inputs are explicitly NULL while the
> other are implicit.
Adding soem NULLs explicitly is fine for me. Using an array of fixed length
still (somewhat) ensures that later no shorter arrays are added which
result in surprises.
Best reagrds
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Jerome,
On Wed, Jan 24, 2024 at 10:16:17AM +0100, Jerome Brunet wrote:
> On Wed 24 Jan 2024 at 10:02, Uwe Kleine-K?nig <[email protected]> wrote:
> > On Fri, Dec 22, 2023 at 12:16:53PM +0100, Jerome Brunet wrote:
> >> @@ -442,6 +439,13 @@ static int meson_pwm_init_channels(struct device *dev)
> >> struct meson_pwm_channel *channel = &meson->channels[i];
> >> struct clk_parent_data div_parent = {}, gate_parent = {};
> >> struct clk_init_data init = {};
> >> + struct clk_divider *div;
> >> + struct clk_gate *gate;
> >> + struct clk_mux *mux;
> >> +
> >> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> >> + if (!mux)
> >> + return -ENOMEM;
> >
> > I don't like this change. While it doesn't increase the memory used, it
> > fragments the used memory and increases the overhead of memory
> > management and the number of devm allocations.
> >
> > Are these members of meson_pwm_channel in the way for anything later?
>
> Not really. It is just not useful on the SoCs which do use it and not
> used at all starting from s4/a1.
This remembers me about the old pwm-imx driver. This was essentially a
single file containing two drivers just because both types appeared on
imx machines. Later it was split into imx1 and imx27.
I didn't look at the relevant differences between the existing driver
and the changes needed for s4, but please don't repeat this issue for
meson. Not sure this fear is justified, just saying ...
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed 24 Jan 2024 at 10:48, Uwe Kleine-König <[email protected]> wrote:
> [[PGP Signed Part:Undecided]]
> Hello Jerome,
>
> On Wed, Jan 24, 2024 at 10:16:17AM +0100, Jerome Brunet wrote:
>> On Wed 24 Jan 2024 at 10:02, Uwe Kleine-König <[email protected]> wrote:
>> > On Fri, Dec 22, 2023 at 12:16:53PM +0100, Jerome Brunet wrote:
>> >> @@ -442,6 +439,13 @@ static int meson_pwm_init_channels(struct device *dev)
>> >> struct meson_pwm_channel *channel = &meson->channels[i];
>> >> struct clk_parent_data div_parent = {}, gate_parent = {};
>> >> struct clk_init_data init = {};
>> >> + struct clk_divider *div;
>> >> + struct clk_gate *gate;
>> >> + struct clk_mux *mux;
>> >> +
>> >> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> >> + if (!mux)
>> >> + return -ENOMEM;
>> >
>> > I don't like this change. While it doesn't increase the memory used, it
>> > fragments the used memory and increases the overhead of memory
>> > management and the number of devm allocations.
>> >
>> > Are these members of meson_pwm_channel in the way for anything later?
>>
>> Not really. It is just not useful on the SoCs which do use it and not
>> used at all starting from s4/a1.
>
> This remembers me about the old pwm-imx driver. This was essentially a
> single file containing two drivers just because both types appeared on
> imx machines. Later it was split into imx1 and imx27.
>
> I didn't look at the relevant differences between the existing driver
> and the changes needed for s4, but please don't repeat this issue for
> meson. Not sure this fear is justified, just saying ...
Noted. Don't worry. s4 is indeed the same PWM block as before, just
mux/div/gate migrated from the pwm IP to the main clk controller.
That's all ... I know ;)
Only the clock registration should change and simplify.
>
> Best regards
> Uwe
--
Jerome
On 2024/1/24 17:59, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Wed 24 Jan 2024 at 10:48, Uwe Kleine-König <[email protected]> wrote:
>
>> [[PGP Signed Part:Undecided]]
>> Hello Jerome,
>>
>> On Wed, Jan 24, 2024 at 10:16:17AM +0100, Jerome Brunet wrote:
>>> On Wed 24 Jan 2024 at 10:02, Uwe Kleine-König <[email protected]> wrote:
>>>> On Fri, Dec 22, 2023 at 12:16:53PM +0100, Jerome Brunet wrote:
>>>>> @@ -442,6 +439,13 @@ static int meson_pwm_init_channels(struct device *dev)
>>>>> struct meson_pwm_channel *channel = &meson->channels[i];
>>>>> struct clk_parent_data div_parent = {}, gate_parent = {};
>>>>> struct clk_init_data init = {};
>>>>> + struct clk_divider *div;
>>>>> + struct clk_gate *gate;
>>>>> + struct clk_mux *mux;
>>>>> +
>>>>> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>>>>> + if (!mux)
>>>>> + return -ENOMEM;
>>>>
>>>> I don't like this change. While it doesn't increase the memory used, it
>>>> fragments the used memory and increases the overhead of memory
>>>> management and the number of devm allocations.
>>>>
>>>> Are these members of meson_pwm_channel in the way for anything later?
>>>
>>> Not really. It is just not useful on the SoCs which do use it and not
>>> used at all starting from s4/a1.
>>
>> This remembers me about the old pwm-imx driver. This was essentially a
>> single file containing two drivers just because both types appeared on
>> imx machines. Later it was split into imx1 and imx27.
>>
>> I didn't look at the relevant differences between the existing driver
>> and the changes needed for s4, but please don't repeat this issue for
>> meson. Not sure this fear is justified, just saying ...
>
> Noted. Don't worry. s4 is indeed the same PWM block as before, just
> mux/div/gate migrated from the pwm IP to the main clk controller.
> That's all ... I know ;)
>
> Only the clock registration should change and simplify.
>
>>
>> Best regards
>> Uwe
>
>
> --
> Jerome
Hi ,Uwe and Jerom.
Compared with m8b g12a and sm1, s4 and A1 are new pwm ip that moved
MUX/DIV/GATE from pwm chip to clock tree module and the pwm block are same.
About new s7，compared with s4，one pwmchip corresponds one pwm channel.
Like，we separate PWMAB into PWMA and PWMB.
Here is a version from Amlogic. We will start the following work of pwm
driver. Welcom to give comments.
Best regards
--
Junyi
On Fri, Dec 22, 2023 at 12:16:52PM +0100, Jerome Brunet wrote:
> Use struct device data to carry the information data around, instead
> of embedded the pwm structure in it and using container_of()
>
> Doing so works just as well and makes it a little easier to add setup
> callback depending on the DT compatible.
>
> Signed-off-by: Jerome Brunet <[email protected]>
> ---
> drivers/pwm/pwm-meson.c | 39 +++++++++++++++++++++++----------------
> 1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index ef50c337f444..15c44185d784 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -101,7 +101,6 @@ struct meson_pwm_data {
> };
>
> struct meson_pwm {
> - struct pwm_chip chip;
> const struct meson_pwm_data *data;
> struct meson_pwm_channel channels[MESON_NUM_PWMS];
> void __iomem *base;
> @@ -114,7 +113,7 @@ struct meson_pwm {
>
> static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip)
> {
> - return container_of(chip, struct meson_pwm, chip);
> + return dev_get_drvdata(chip->dev);
> }
>
> static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -146,6 +145,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
> const struct pwm_state *state)
> {
> struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm];
> + struct device *dev = pwm->chip->dev;
> unsigned int cnt, duty_cnt;
> unsigned long fin_freq;
> u64 duty, period, freq;
> @@ -168,19 +168,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>
> fin_freq = clk_round_rate(channel->clk, freq);
> if (fin_freq == 0) {
> - dev_err(meson->chip.dev, "invalid source clock frequency\n");
> + dev_err(dev, "invalid source clock frequency\n");
> return -EINVAL;
> }
>
> - dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
> + dev_dbg(dev, "fin_freq: %lu Hz\n", fin_freq);
>
> cnt = div_u64(fin_freq * period, NSEC_PER_SEC);
> if (cnt > 0xffff) {
> - dev_err(meson->chip.dev, "unable to get period cnt\n");
> + dev_err(dev, "unable to get period cnt\n");
> return -EINVAL;
> }
>
> - dev_dbg(meson->chip.dev, "period=%llu cnt=%u\n", period, cnt);
> + dev_dbg(dev, "period=%llu cnt=%u\n", period, cnt);
>
> if (duty == period) {
> channel->hi = cnt;
> @@ -191,7 +191,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
> } else {
> duty_cnt = div_u64(fin_freq * duty, NSEC_PER_SEC);
>
> - dev_dbg(meson->chip.dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
> + dev_dbg(dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
>
> channel->hi = duty_cnt;
> channel->lo = cnt - duty_cnt;
> @@ -214,7 +214,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
>
> err = clk_set_rate(channel->clk, channel->rate);
> if (err)
> - dev_err(meson->chip.dev, "setting clock rate failed\n");
> + dev_err(pwm->chip->dev, "setting clock rate failed\n");
>
> spin_lock_irqsave(&meson->lock, flags);
>
> @@ -425,10 +425,10 @@ static const struct of_device_id meson_pwm_matches[] = {
> };
> MODULE_DEVICE_TABLE(of, meson_pwm_matches);
>
> -static int meson_pwm_init_channels(struct meson_pwm *meson)
> +static int meson_pwm_init_channels(struct device *dev)
> {
> struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
> - struct device *dev = meson->chip.dev;
> + struct meson_pwm *meson = dev_get_drvdata(dev);
> unsigned int i;
> char name[255];
> int err;
> @@ -438,7 +438,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
> mux_parent_data[i].name = meson->data->parent_names[i];
> }
>
> - for (i = 0; i < meson->chip.npwm; i++) {
> + for (i = 0; i < MESON_NUM_PWMS; i++) {
> struct meson_pwm_channel *channel = &meson->channels[i];
> struct clk_parent_data div_parent = {}, gate_parent = {};
> struct clk_init_data init = {};
> @@ -519,28 +519,35 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
> static int meson_pwm_probe(struct platform_device *pdev)
> {
> struct meson_pwm *meson;
> + struct pwm_chip *chip;
> int err;
>
> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> meson = devm_kzalloc(&pdev->dev, sizeof(*meson), GFP_KERNEL);
> if (!meson)
> return -ENOMEM;
>
> + platform_set_drvdata(pdev, meson);
> +
> meson->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(meson->base))
> return PTR_ERR(meson->base);
>
> spin_lock_init(&meson->lock);
> - meson->chip.dev = &pdev->dev;
> - meson->chip.ops = &meson_pwm_ops;
> - meson->chip.npwm = MESON_NUM_PWMS;
> + chip->dev = &pdev->dev;
> + chip->ops = &meson_pwm_ops;
> + chip->npwm = MESON_NUM_PWMS;
>
> meson->data = of_device_get_match_data(&pdev->dev);
>
> - err = meson_pwm_init_channels(meson);
> + err = meson_pwm_init_channels(&pdev->dev);
> if (err < 0)
> return err;
>
> - err = devm_pwmchip_add(&pdev->dev, &meson->chip);
> + err = devm_pwmchip_add(&pdev->dev, chip);
> if (err < 0)
> return dev_err_probe(&pdev->dev, err,
> "failed to register PWM chip\n");
Parts of this change overlap with plans I have for this driver. I
reworked the series a bit now, also affecting the meson driver, the
previous submission is available at https://lore.kernel.org/linux-pwm/bf6f7c6253041f60ee8f35b5c9c9e8d595332fb0.1706182805.git.u.kleine-koenig@pengutronix.de
I don't see the nice benefit of this patch yet, but I assume this will
become clearer when I check the next patch.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon 05 Feb 2024 at 18:12, Uwe Kleine-König <[email protected]> wrote:
> [[PGP Signed Part:Undecided]]
> On Fri, Dec 22, 2023 at 12:16:52PM +0100, Jerome Brunet wrote:
>> Use struct device data to carry the information data around, instead
>> of embedded the pwm structure in it and using container_of()
>>
>> Doing so works just as well and makes it a little easier to add setup
>> callback depending on the DT compatible.
>>
>> Signed-off-by: Jerome Brunet <[email protected]>
>> ---
>> drivers/pwm/pwm-meson.c | 39 +++++++++++++++++++++++----------------
>> 1 file changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index ef50c337f444..15c44185d784 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -101,7 +101,6 @@ struct meson_pwm_data {
>> };
>>
>> struct meson_pwm {
>> - struct pwm_chip chip;
>> const struct meson_pwm_data *data;
>> struct meson_pwm_channel channels[MESON_NUM_PWMS];
>> void __iomem *base;
>> @@ -114,7 +113,7 @@ struct meson_pwm {
>>
>> static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip)
>> {
>> - return container_of(chip, struct meson_pwm, chip);
>> + return dev_get_drvdata(chip->dev);
>> }
>>
>> static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>> @@ -146,6 +145,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>> const struct pwm_state *state)
>> {
>> struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm];
>> + struct device *dev = pwm->chip->dev;
>> unsigned int cnt, duty_cnt;
>> unsigned long fin_freq;
>> u64 duty, period, freq;
>> @@ -168,19 +168,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>>
>> fin_freq = clk_round_rate(channel->clk, freq);
>> if (fin_freq == 0) {
>> - dev_err(meson->chip.dev, "invalid source clock frequency\n");
>> + dev_err(dev, "invalid source clock frequency\n");
>> return -EINVAL;
>> }
>>
>> - dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
>> + dev_dbg(dev, "fin_freq: %lu Hz\n", fin_freq);
>>
>> cnt = div_u64(fin_freq * period, NSEC_PER_SEC);
>> if (cnt > 0xffff) {
>> - dev_err(meson->chip.dev, "unable to get period cnt\n");
>> + dev_err(dev, "unable to get period cnt\n");
>> return -EINVAL;
>> }
>>
>> - dev_dbg(meson->chip.dev, "period=%llu cnt=%u\n", period, cnt);
>> + dev_dbg(dev, "period=%llu cnt=%u\n", period, cnt);
>>
>> if (duty == period) {
>> channel->hi = cnt;
>> @@ -191,7 +191,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>> } else {
>> duty_cnt = div_u64(fin_freq * duty, NSEC_PER_SEC);
>>
>> - dev_dbg(meson->chip.dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
>> + dev_dbg(dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
>>
>> channel->hi = duty_cnt;
>> channel->lo = cnt - duty_cnt;
>> @@ -214,7 +214,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
>>
>> err = clk_set_rate(channel->clk, channel->rate);
>> if (err)
>> - dev_err(meson->chip.dev, "setting clock rate failed\n");
>> + dev_err(pwm->chip->dev, "setting clock rate failed\n");
>>
>> spin_lock_irqsave(&meson->lock, flags);
>>
>> @@ -425,10 +425,10 @@ static const struct of_device_id meson_pwm_matches[] = {
>> };
>> MODULE_DEVICE_TABLE(of, meson_pwm_matches);
>>
>> -static int meson_pwm_init_channels(struct meson_pwm *meson)
>> +static int meson_pwm_init_channels(struct device *dev)
>> {
>> struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
>> - struct device *dev = meson->chip.dev;
>> + struct meson_pwm *meson = dev_get_drvdata(dev);
>> unsigned int i;
>> char name[255];
>> int err;
>> @@ -438,7 +438,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
>> mux_parent_data[i].name = meson->data->parent_names[i];
>> }
>>
>> - for (i = 0; i < meson->chip.npwm; i++) {
>> + for (i = 0; i < MESON_NUM_PWMS; i++) {
>> struct meson_pwm_channel *channel = &meson->channels[i];
>> struct clk_parent_data div_parent = {}, gate_parent = {};
>> struct clk_init_data init = {};
>> @@ -519,28 +519,35 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
>> static int meson_pwm_probe(struct platform_device *pdev)
>> {
>> struct meson_pwm *meson;
>> + struct pwm_chip *chip;
>> int err;
>>
>> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>> + if (!chip)
>> + return -ENOMEM;
>> +
>> meson = devm_kzalloc(&pdev->dev, sizeof(*meson), GFP_KERNEL);
>> if (!meson)
>> return -ENOMEM;
>>
>> + platform_set_drvdata(pdev, meson);
>> +
>> meson->base = devm_platform_ioremap_resource(pdev, 0);
>> if (IS_ERR(meson->base))
>> return PTR_ERR(meson->base);
>>
>> spin_lock_init(&meson->lock);
>> - meson->chip.dev = &pdev->dev;
>> - meson->chip.ops = &meson_pwm_ops;
>> - meson->chip.npwm = MESON_NUM_PWMS;
>> + chip->dev = &pdev->dev;
>> + chip->ops = &meson_pwm_ops;
>> + chip->npwm = MESON_NUM_PWMS;
>>
>> meson->data = of_device_get_match_data(&pdev->dev);
>>
>> - err = meson_pwm_init_channels(meson);
>> + err = meson_pwm_init_channels(&pdev->dev);
>> if (err < 0)
>> return err;
>>
>> - err = devm_pwmchip_add(&pdev->dev, &meson->chip);
>> + err = devm_pwmchip_add(&pdev->dev, chip);
>> if (err < 0)
>> return dev_err_probe(&pdev->dev, err,
>> "failed to register PWM chip\n");
>
> Parts of this change overlap with plans I have for this driver. I
It does overlap indeed. I'll drop this one while rebasing
> reworked the series a bit now, also affecting the meson driver, the
> previous submission is available at
> https://lore.kernel.org/linux-pwm/bf6f7c6253041f60ee8f35b5c9c9e8d595332fb0.1706182805.git.u.kleine-koenig@pengutronix.de
>
> I don't see the nice benefit of this patch yet, but I assume this will
> become clearer when I check the next patch.
>
> Best regards
> Uwe
--
Jerome