[Deepin-Kernel-SIG] [linux 6.6-y] [CIX] Apply clk, DMA and soc suppors for CIX #647
Conversation
This is a simple driver for the sky1 general purpose timer. The general purpose timer is configured(freerunning mode) as a 64-bit incrementing counter with an auto-incrementing feature. It continues incrementing after sending interrupts. The gpt timer is used after system start. Signed-off-by: Jerry.Zhu <[email protected]>
- complete cyclic mode and scatter gather mode - support device to memory and memory to device transfer - support 1D transfter - support max to 8 channel transfer Signed-off-by: hongliang.yang <[email protected]> Signed-off-by: Guomin.Chen <[email protected]>
Add clk driver for cix platform. Signed-off-by: Joakim Zhang <[email protected]> Signed-off-by: Guomin.Chen <[email protected]>
1,Add cix reset controller driver. 2,Add cix sky1 audio reset driver. Signed-off-by: Zichar.Zhang <[email protected]> Signed-off-by: Guomin.Chen <[email protected]>
1,Add driver support for Sky1 SoC version. 2,Add acpi resource driver for sky1. Signed-off-by: Jerry.Zhu <[email protected]> Signed-off-by: Zichar.Zhang <[email protected]> Signed-off-by: Guomin.Chen <[email protected]>
Reviewer's Guide by SourceryThis pull request introduces clock, DMA, reset and soc supports for CIX. It includes new drivers for ARM DMA350, clock management, reset control, and SoC information, along with necessary configurations and device tree bindings. It also includes a new clocksource driver for CIX SKY1 GPT. Class diagram for dma350_cmdlink_addr_tclassDiagram
class dma350_cmdlink_addr_t {
uint32_t intren
uint32_t ctrl
uint32_t srcaddr
uint32_t srcaddrhi
uint32_t desaddr
uint32_t desaddrhi
uint32_t xsize
uint32_t xsizehi
uint32_t srctranscfg
uint32_t destranscfg
uint32_t xaddrinc
uint32_t yaddrstride
uint32_t fillval
uint32_t ysize
uint32_t tmpltcfg
uint32_t srctmplt
uint32_t destmplt
uint32_t srctrigincfg
uint32_t destrigincfg
uint32_t trigoutcfg
uint32_t gpoen0
uint32_t reserved0
uint32_t gpoval0
uint32_t reserved1
uint32_t streamintcfg
uint32_t reserved2
uint32_t linkattr
uint32_t autocfg
uint32_t linkaddr
uint32_t linkaddrhi
}
Class diagram for sky1_clk_dividerclassDiagram
class sky1_clk_divider {
clk_divider div
regmap regmap
int offset
}
sky1_clk_divider -- clk_divider : contains
sky1_clk_divider -- regmap : uses
Class diagram for sky1_clk_muxclassDiagram
class sky1_clk_mux {
clk_mux mux
regmap regmap
int offset
}
sky1_clk_mux -- clk_mux : contains
sky1_clk_mux -- regmap : uses
Class diagram for sky1_audss_clks_privclassDiagram
class sky1_audss_clks_priv {
device dev
iomem rcsu_base
regmap regmap_cru
clk clks[SKY1_AUDSS_CLKS_NUM]
reset_control rst_noc
}
sky1_audss_clks_priv -- device : uses
sky1_audss_clks_priv -- regmap : uses
sky1_audss_clks_priv -- clk : uses
sky1_audss_clks_priv -- reset_control : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Avenger-285714 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using more descriptive names for the Kconfig options to improve readability and maintainability.
- The dma350_ch_drv.h file is quite large; consider splitting it into smaller, more manageable files.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 7 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| return 0; | ||
| } | ||
| early_param("earlycon", cix_uart_clocks_keep_param); |
There was a problem hiding this comment.
suggestion: Consider using a dedicated module parameter instead of repurposing the 'earlycon' parameter.
Relying on the 'earlycon' parameter to control UART clock registration can lead to potential conflicts with other early-boot configurations. A dedicated parameter would provide clearer intent and avoid unintended interactions.
Suggested implementation:
static bool cix_uart_clocks_keep = false;
static int cix_uart_clocks_enabled;
static struct clk **cix_uart_clocks;
module_param(cix_uart_clocks_keep, bool, 0644);
MODULE_PARM_DESC(cix_uart_clocks_keep, "Keep UART clocks active when stdout is earlycon");Since this change removes the early_param mechanism, ensure that any documentation or user instructions for configuring the UART clocks via module parameters is updated accordingly. Check if other parts of the codebase reference the "earlycon" parameter for UART clock registration and update them to use the new module parameter if needed.
| "GNVL", | ||
| }; | ||
|
|
||
| static char *acpi_tb_find_sig_ptr(struct acpi_table_header *table, char *sig) |
There was a problem hiding this comment.
issue (bug_risk): Potential out-of-bound access in acpi_tb_find_sig_ptr.
The loop begins at 'ptr = cur' and then checks '*(ptr - 1)', which may access memory before the table buffer during the first iteration. Starting the loop at 'cur + 1' or adding an explicit boundary check would prevent this potential undefined behavior.
| * Copyright 2024 Cix Technology Group Co., Ltd. | ||
| */ | ||
|
|
||
| #ifndef __DMA350_CH_DRV_H |
There was a problem hiding this comment.
issue (complexity): Consider using helper macros or inline functions to consolidate repetitive register field update operations, such as the read-modify-write pattern with shifts and masks, to reduce code duplication and improve maintainability..
Consider consolidating the repetitive field updates using generic helper macros or inline functions. For instance, many of these functions execute the same read‐modify–write pattern using shifts/masks. One approach is to define a macro that generates these setters. For example:
#define DMA350_DEFINE_REG_SETTER(fn, pos, msk) \
static inline void fn(void __iomem *addr, uint32_t val) { \
SET_FIELD(addr, val, pos, msk); \
}
// Then use it for repetitive functions:
DMA350_DEFINE_REG_SETTER(dma350_ch_set_transize, DMA350_CH_CTRL_TRANSIZE_Pos, DMA350_CH_CTRL_TRANSIZE_Msk)
DMA350_DEFINE_REG_SETTER(dma350_ch_set_chprio, DMA350_CH_CTRL_CHPRIO_Pos, DMA350_CH_CTRL_CHPRIO_Msk)
// ... and so forthAlternatively, for setting structure fields you could create a helper function that takes pointers to the field member and updates it accordingly. This refactoring will reduce the duplication while preserving existing functionality.
| .is_enabled = sky1_audss_clk_gate_is_enabled, | ||
| }; | ||
|
|
||
| static struct clk_hw *sky1_audss_clk_register(struct device *dev, |
There was a problem hiding this comment.
issue (complexity): Consider extracting common initialization logic for mux, divider, and gate components into helper functions to reduce code duplication and complexity in the composite clock registration process.
The changes introduce a significant amount of duplicated “glue” code mostly for building the mux, divider, and gate types. Although the functionality is clearly separated by type, you can reduce duplication and overall complexity by extracting the common initialization parts into helper functions. For example, you can split the composite clock registration into several small functions that initialize each component separately.
Actionable steps:
-
Extract helper functions for each component initialization.
For example, you can extract functions that initialize the mux, divider, and gate elements:
static struct sky1_clk_mux *init_sky1_clk_mux(struct device *dev, struct regmap *regmap, struct muxdiv_cfg *mux_cfg, spinlock_t *lock) { struct sky1_clk_mux *sky1_mux; if (mux_cfg->offset < 0) return NULL; sky1_mux = devm_kzalloc(dev, sizeof(*sky1_mux), GFP_KERNEL); if (!sky1_mux) return ERR_PTR(-ENOMEM); sky1_mux->mux.shift = mux_cfg->shift; sky1_mux->mux.mask = BIT(mux_cfg->width) - 1; sky1_mux->mux.flags = mux_cfg->flags; sky1_mux->mux.lock = lock; sky1_mux->regmap = regmap; sky1_mux->offset = mux_cfg->offset; return sky1_mux; }
-
Do similar extraction for the divider and gate.
static struct sky1_clk_divider *init_sky1_clk_div(struct device *dev, struct regmap *regmap, struct muxdiv_cfg *div_cfg, spinlock_t *lock) { struct sky1_clk_divider *sky1_div; if (div_cfg->offset < 0) return NULL; sky1_div = devm_kzalloc(dev, sizeof(*sky1_div), GFP_KERNEL); if (!sky1_div) return ERR_PTR(-ENOMEM); sky1_div->div.shift = div_cfg->shift; sky1_div->div.width = div_cfg->width; sky1_div->div.flags = div_cfg->flags | CLK_DIVIDER_POWER_OF_TWO; sky1_div->div.lock = lock; sky1_div->regmap = regmap; sky1_div->offset = div_cfg->offset; return sky1_div; } static struct sky1_clk_gate *init_sky1_clk_gate(struct device *dev, struct regmap *regmap, struct gate_cfg *gate_cfg, spinlock_t *lock) { struct sky1_clk_gate *sky1_gate; if (gate_cfg->offset < 0) return NULL; sky1_gate = devm_kzalloc(dev, sizeof(*sky1_gate), GFP_KERNEL); if (!sky1_gate) return ERR_PTR(-ENOMEM); sky1_gate->gate.bit_idx = gate_cfg->shift; sky1_gate->gate.flags = gate_cfg->flags; sky1_gate->gate.lock = lock; sky1_gate->regmap = regmap; sky1_gate->offset = gate_cfg->offset; return sky1_gate; }
-
Simplify the composite clock registration using these helpers.
Replace the inline component initialization in your
sky1_audss_clk_register()function with calls to these helper functions. For example:static struct clk_hw *sky1_audss_clk_register(struct device *dev, const char *name, const char * const *parent_names, int num_parents, struct regmap *regmap, struct muxdiv_cfg *mux_cfg, struct muxdiv_cfg *div_cfg, struct gate_cfg *gate_cfg, unsigned long flags, spinlock_t *lock) { struct clk_parent_data *pdata; struct sky1_clk_mux *sky1_mux = init_sky1_clk_mux(dev, regmap, mux_cfg, lock); struct sky1_clk_divider *sky1_div = init_sky1_clk_div(dev, regmap, div_cfg, lock); struct sky1_clk_gate *sky1_gate = init_sky1_clk_gate(dev, regmap, gate_cfg, lock); const struct clk_ops *mux_ops = sky1_mux ? &sky1_audss_clk_mux_ops : NULL; const struct clk_ops *div_ops = sky1_div ? &sky1_audss_clk_divider_ops : NULL; const struct clk_ops *gate_ops = sky1_gate ? &sky1_audss_clk_gate_ops : NULL; struct clk_hw *hw; int i; pdata = devm_kzalloc(dev, sizeof(*pdata) * num_parents, GFP_KERNEL); if (!pdata) return ERR_PTR(-ENOMEM); for (i = 0; i < num_parents; i++) { pdata[i].fw_name = parent_names[i]; pdata[i].index = 0; } hw = devm_clk_hw_register_composite_pdata(dev, name, pdata, num_parents, sky1_mux ? &sky1_mux->mux.hw : NULL, mux_ops, sky1_div ? &sky1_div->div.hw : NULL, div_ops, sky1_gate ? &sky1_gate->gate.hw : NULL, gate_ops, flags); if (IS_ERR(hw)) dev_err(dev, "register %s clock failed with err = %ld\n", name, PTR_ERR(hw)); return hw; }
This refactoring not only splits responsibilities into smaller functions, making the code easier to follow and test, but also eases maintenance by isolating the repeated patterns into helper routines.
These changes keep all functionality intact while reducing the overall code duplication and complexity in the registration logic.
| return NULL; | ||
| } | ||
|
|
||
| static int acpi_clock_config_set(struct device *dev, u32 clk_id, u32 config) |
There was a problem hiding this comment.
issue (complexity): Consider creating a helper function to encapsulate the ACPI evaluation and buffer validation logic to reduce boilerplate code in multiple functions such as acpi_clock_config_set, acpi_cix_clk_recalc_rate and acpi_cix_clk_set_rate.
You can reduce the repetitive boilerplate by extracting the ACPI evaluation and error-checking into a helper function. For example, create a helper that wraps the call to acpi_evaluate_object() and validates the buffer result:
static int acpi_eval_buffer(struct device *dev, acpi_handle handle,
const char *method, struct acpi_object_list *args,
struct acpi_buffer *buf, union acpi_object **package)
{
acpi_status status;
status = acpi_evaluate_object(handle, method, args, buf);
if (ACPI_FAILURE(status)) {
dev_err(dev, "ACPI evaluation for %s failed\n", method);
return -ENODEV;
}
*package = buf->pointer;
if (!*package || (*package)->type != ACPI_TYPE_BUFFER) {
dev_err(dev, "ACPI result for %s invalid\n", method);
kfree(buf->pointer);
return -ENODEV;
}
return 0;
}Then update functions like acpi_clock_config_set(), acpi_cix_clk_recalc_rate(), and acpi_cix_clk_set_rate() to use this helper. For instance, in acpi_clock_config_set():
static int acpi_clock_config_set(struct device *dev, u32 clk_id, u32 config)
{
acpi_handle handle = ACPI_HANDLE(dev);
struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
union acpi_object *package;
struct acpi_object args[2] = {
{ .type = ACPI_TYPE_INTEGER, .integer.value = clk_id },
{ .type = ACPI_TYPE_INTEGER, .integer.value = config },
};
struct acpi_object_list arg_list = { args, ARRAY_SIZE(args) };
int ret;
ret = acpi_eval_buffer(dev, handle, "CLKC", &arg_list, &buffer, &package);
if (ret)
return ret;
// Use package data as needed.
if (*(u32 *)package->buffer.pointer != SUCCESS) {
dev_err(dev, "ACPI clk[%u] set config[%u] err:%d\n",
clk_id, config, *(u32 *)package->buffer.pointer);
ret = -ENODEV;
}
kfree(buffer.pointer);
return ret;
}Repeat similar refactoring in the other functions. This centralizes error checking and reduces nested conditionals while keeping the functionality unchanged.
| return dev; | ||
| } | ||
|
|
||
| static const char *acpi_obj_str_to_devname(const union acpi_object *obj) |
There was a problem hiding this comment.
issue (complexity): Consider consolidating conversion functions into generic helpers and factoring out repeated package element type checks into a helper function to reduce code duplication and improve readability..
You can reduce the duplication in your conversion functions by “merging” similar logic into generic helpers. For example, you’ve defined separate functions to convert a string or a reference to a device, and then again to a name. Consolidate these into two generic functions. One for converting an ACPI object to a device and one for fetching its name, for example:
static struct device *acpi_obj_to_device(const union acpi_object *obj)
{
if (!obj)
return NULL;
if (obj->type == ACPI_TYPE_LOCAL_REFERENCE)
return acpi_obj_ref_platform_device(obj);
else if (obj->type == ACPI_TYPE_STRING)
return acpi_obj_str_platform_device(obj);
return NULL;
}
static const char *acpi_obj_to_name(const union acpi_object *obj)
{
struct device *dev = acpi_obj_to_device(obj);
return dev ? dev_name(dev) : NULL;
}Then in your lookup handlers, replace calls like acpi_obj_str_to_devname or acpi_obj_ref_to_devname with acpi_obj_to_name(). This consolidation makes it easier to maintain the type-checking logic while preserving functionality.
Another opportunity is to factor out the repeated package element type checks into a helper. For instance:
static bool validate_elem_type(const union acpi_object *elem, u32 expected)
{
return elem->type == expected;
}Then in your loops you might write:
if (!validate_elem_type(&el[1], ACPI_TYPE_INTEGER) ||
!validate_elem_type(&el[2], ACPI_TYPE_LOCAL_REFERENCE) &&
!validate_elem_type(&el[2], ACPI_TYPE_STRING))
continue;This removes some of the noise from the loop body and clarifies your intent. These changes reduce complexity without altering functionality.
| /*DMA350 register define*/ | ||
| /*DMACH<n> register summary*/ | ||
|
|
||
| #define DMA350_REG_CMD 0x000 /* Channel DMA Command Register */ |
There was a problem hiding this comment.
issue (complexity): Consider using an X-macro table to centralize register metadata and auto-generate repetitive macro definitions, reducing code duplication and improving maintainability for register definitions and bit-field definitions if applicable
Consider reducing the duplication by centralizing the register metadata and auto‐generating the repetitive macro definitions using an X‐macro table. For example, you can define a table with each register’s name, address, and description and then “loop” over it to generate all the macro definitions. This makes it easier to add, remove, or update entries without editing hundreds of lines. One small example:
/* Define register list as an X-macro table */
#define DMA_REG_LIST \
X(CMD, 0x000, "Channel DMA Command Register") \
X(STATUS, 0x004, "Channel Status Register") \
X(INTREN, 0x008, "Channel Interrupt Enable Register") \
X(CTRL, 0x00C, "Channel Control Register") \
X(SRCADDR, 0x010, "Channel Source Address Register") \
/* ... continue for other registers */
/* Use the table to generate macros */
#define X(name, addr, desc) \
#define DMA350_REG_##name (addr) /* desc */
DMA_REG_LIST
#undef XIf there are repeated patterns for bit-field definitions as well, applying a similar X-macro technique will help keep the code concise and maintainable. This minimizes manual repetition while keeping full functionality.
| return buf; | ||
| } | ||
|
|
||
| static const char *cix_sky1_socinfo_revision(unsigned int opn) |
There was a problem hiding this comment.
issue (complexity): Consider using a lookup table and helper function to reduce the duplicated switch-case logic for revision, soc ID, and serial number mappings, improving code maintainability and readability without changing functionality
You can reduce the duplicated switch-case logic by centralizing the mapping into a lookup table and a common helper. For example, for the revision mapping you could do:
struct id_to_str {
unsigned int id;
const char *name;
};
static const struct id_to_str revision_map[] = {
{ A0_GENEARL, "A0_GENEARL" },
{ A0_MINI_DESK, "A0_MINI_DESK" },
{ A0_COCKPIT, "A0_COCKPIT" },
{ A1_CLOUND_BOOK, "A1_CLOUND_BOOK" },
{ B0_GENERAL, "B0_MINI_DESK" },
};
static const char *lookup_str(const struct id_to_str *map, size_t map_size, unsigned int id, const char *default_str)
{
size_t i;
for (i = 0; i < map_size; i++) {
if (map[i].id == id)
return map[i].name;
}
return default_str;
}
static const char *cix_sky1_socinfo_revision(unsigned int opn)
{
unsigned int revision = opn & REVISION_MASK;
return kasprintf(GFP_KERNEL, "Rev %s",
lookup_str(revision_map, ARRAY_SIZE(revision_map),
revision, "ERROR_REVISION_NUMBER"));
}You can apply a similar approach for the soc ID and serial number mappings. This refactoring centralizes the mappings, reduces repetitiveness, and improves clarity without changing functionality.
| * 3. Write the upper 32-bit Comparator Value Register. | ||
| * 4. Set the Comp Enable bit and, if necessary, the IRQ enable bit. | ||
| */ | ||
| static int sky1_set_next_event(unsigned long evt, struct clock_event_device *ced) |
There was a problem hiding this comment.
issue (complexity): Consider creating a helper function to encapsulate the repeated logic for writing the 64-bit compare value to the upper and lower registers, which is used in multiple functions.
Consider extracting the repeated compare-register update logic into a helper function. For example, in several locations (in sky1_set_next_event(), sky1_shutdown(), and sky1_set_oneshot()), you perform similar 64-bit compare value writes. A helper function like the following can simplify these updates and reduce code duplication:
static inline void sky1_write_compare(struct sky1_timer *sky1tm, u64 comp_val)
{
/* Write upper 32 bits first */
writel_relaxed(comp_val >> 32, sky1tm->base + COMP + TIMER_OFFSET);
/* Then lower 32 bits */
writel_relaxed((u32)comp_val, sky1tm->base + COMP);
}Then update your calls accordingly. For instance, in sky1_set_next_event():
tcn = _sky1_gt_counter_read() + evt;
sky1_write_compare(sky1tm, tcn);And in sky1_shutdown() and sky1_set_oneshot():
tcn = _sky1_gt_counter_read();
sky1_write_compare(sky1tm, tcn - 3);This keeps functionality intact while reducing the duplicated bit‐manipulation logic and register accesses.
2 participants
Summary by Sourcery
Adds clock, DMA, and SOC support for the CIX platform, including drivers and Kconfig entries for reset, clock, DMA, and SOC functionalities. It introduces a DMA350 driver, clock driver for SKY1 AUDSS, and ACPI resource lookup mechanism.
New Features:
Build:
Tests: