From: Baoqi Zhang <zhangbaoqi@loongson.cn> This patch remove the fixed mapping between the LS7A interrupt source and the HT interrupt vector, and replaced it with a dynamically allocated approach. We introduce a mapping table in struct pch_pic, where each interrupt source will allocate an index as a 'hwirq' from the table in the order of application and set table value as interrupt source number. This hwirq will be configured as its vector in the HT interrupt controller. For an interrupt source, the validity period of the obtained hwirq will last until the system reset. This will be more conducive to fully utilizing existing vectors to support more devices. Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn> Signed-off-by: Biao Dong <dongbiao@loongson.cn> Signed-off-by: Tianyang Zhang <zhangtianyang@loongson.cn> --- drivers/irqchip/irq-loongson-pch-pic.c | 76 ++++++++++++++++++++------ 1 file changed, 59 insertions(+), 17 deletions(-) diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c index 63db8e2172e0..1ee63fcf4b5a 100644 --- a/drivers/irqchip/irq-loongson-pch-pic.c +++ b/drivers/irqchip/irq-loongson-pch-pic.c @@ -33,6 +33,7 @@ #define PIC_COUNT (PIC_COUNT_PER_REG * PIC_REG_COUNT) #define PIC_REG_IDX(irq_id) ((irq_id) / PIC_COUNT_PER_REG) #define PIC_REG_BIT(irq_id) ((irq_id) % PIC_COUNT_PER_REG) +#define PIC_UNDEF_VECTOR 255 static int nr_pics; @@ -46,12 +47,19 @@ struct pch_pic { u32 saved_vec_en[PIC_REG_COUNT]; u32 saved_vec_pol[PIC_REG_COUNT]; u32 saved_vec_edge[PIC_REG_COUNT]; + u8 table[PIC_COUNT]; + int inuse; }; static struct pch_pic *pch_pic_priv[MAX_IO_PICS]; struct fwnode_handle *pch_pic_handle[MAX_IO_PICS]; +static inline u8 hwirq_to_bit(struct pch_pic *priv, int hirq) +{ + return priv->table[hirq]; +} + static void pch_pic_bitset(struct pch_pic *priv, int offset, int bit) { u32 reg; @@ -80,45 +88,47 @@ static void pch_pic_mask_irq(struct irq_data *d) { struct pch_pic *priv = irq_data_get_irq_chip_data(d); - pch_pic_bitset(priv, PCH_PIC_MASK, d->hwirq); + pch_pic_bitset(priv, PCH_PIC_MASK, hwirq_to_bit(priv, d->hwirq)); irq_chip_mask_parent(d); } static void pch_pic_unmask_irq(struct irq_data *d) { struct pch_pic *priv = irq_data_get_irq_chip_data(d); + int bit = hwirq_to_bit(priv, d->hwirq); - writel(BIT(PIC_REG_BIT(d->hwirq)), - priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4); + writel(BIT(PIC_REG_BIT(bit)), + priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4); irq_chip_unmask_parent(d); - pch_pic_bitclr(priv, PCH_PIC_MASK, d->hwirq); + pch_pic_bitclr(priv, PCH_PIC_MASK, bit); } static int pch_pic_set_type(struct irq_data *d, unsigned int type) { struct pch_pic *priv = irq_data_get_irq_chip_data(d); + int bit = hwirq_to_bit(priv, d->hwirq); int ret = 0; switch (type) { case IRQ_TYPE_EDGE_RISING: - pch_pic_bitset(priv, PCH_PIC_EDGE, d->hwirq); - pch_pic_bitclr(priv, PCH_PIC_POL, d->hwirq); + pch_pic_bitset(priv, PCH_PIC_EDGE, bit); + pch_pic_bitclr(priv, PCH_PIC_POL, bit); irq_set_handler_locked(d, handle_edge_irq); break; case IRQ_TYPE_EDGE_FALLING: - pch_pic_bitset(priv, PCH_PIC_EDGE, d->hwirq); - pch_pic_bitset(priv, PCH_PIC_POL, d->hwirq); + pch_pic_bitset(priv, PCH_PIC_EDGE, bit); + pch_pic_bitset(priv, PCH_PIC_POL, bit); irq_set_handler_locked(d, handle_edge_irq); break; case IRQ_TYPE_LEVEL_HIGH: - pch_pic_bitclr(priv, PCH_PIC_EDGE, d->hwirq); - pch_pic_bitclr(priv, PCH_PIC_POL, d->hwirq); + pch_pic_bitclr(priv, PCH_PIC_EDGE, bit); + pch_pic_bitclr(priv, PCH_PIC_POL, bit); irq_set_handler_locked(d, handle_level_irq); break; case IRQ_TYPE_LEVEL_LOW: - pch_pic_bitclr(priv, PCH_PIC_EDGE, d->hwirq); - pch_pic_bitset(priv, PCH_PIC_POL, d->hwirq); + pch_pic_bitclr(priv, PCH_PIC_EDGE, bit); + pch_pic_bitset(priv, PCH_PIC_POL, bit); irq_set_handler_locked(d, handle_level_irq); break; default: @@ -133,11 +143,12 @@ static void pch_pic_ack_irq(struct irq_data *d) { unsigned int reg; struct pch_pic *priv = irq_data_get_irq_chip_data(d); + int bit = hwirq_to_bit(priv, d->hwirq); - reg = readl(priv->base + PCH_PIC_EDGE + PIC_REG_IDX(d->hwirq) * 4); - if (reg & BIT(PIC_REG_BIT(d->hwirq))) { - writel(BIT(PIC_REG_BIT(d->hwirq)), - priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4); + reg = readl(priv->base + PCH_PIC_EDGE + PIC_REG_IDX(bit) * 4); + if (reg & BIT(PIC_REG_BIT(bit))) { + writel(BIT(PIC_REG_BIT(bit)), + priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4); } irq_chip_ack_parent(d); } @@ -159,6 +170,8 @@ static int pch_pic_domain_translate(struct irq_domain *d, { struct pch_pic *priv = d->host_data; struct device_node *of_node = to_of_node(fwspec->fwnode); + unsigned long flags; + int i; if (of_node) { if (fwspec->param_count < 2) @@ -171,6 +184,27 @@ static int pch_pic_domain_translate(struct irq_domain *d, return -EINVAL; *hwirq = fwspec->param[0] - priv->gsi_base; + + raw_spin_lock_irqsave(&priv->pic_lock, flags); + /* Check pic-table to confirm if the hwirq has been assigned */ + for (i = 0; i < priv->inuse; i++) { + if (priv->table[i] == *hwirq) { + *hwirq = i; + break; + } + } + if (i == priv->inuse) { + /* Assign a new hwirq in pic-table */ + if (priv->inuse >= PIC_COUNT) { + pr_err("pch-pic domain has no free vectors\n"); + raw_spin_unlock_irqrestore(&priv->pic_lock, flags); + return -EINVAL; + } + priv->table[priv->inuse] = *hwirq; + *hwirq = priv->inuse++; + } + raw_spin_unlock_irqrestore(&priv->pic_lock, flags); + if (fwspec->param_count > 1) *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; else @@ -194,6 +228,9 @@ static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq, if (err) return err; + /* Write vector ID */ + writeb(priv->ht_vec_base + hwirq, priv->base + PCH_INT_HTVEC(hwirq_to_bit(priv, hwirq))); + parent_fwspec.fwnode = domain->parent->fwnode; parent_fwspec.param_count = 1; parent_fwspec.param[0] = hwirq + priv->ht_vec_base; @@ -222,7 +259,7 @@ static void pch_pic_reset(struct pch_pic *priv) for (i = 0; i < PIC_COUNT; i++) { /* Write vector ID */ - writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(i)); + writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(hwirq_to_bit(priv, i))); /* Hardcode route to HT0 Lo */ writeb(1, priv->base + PCH_INT_ROUTE(i)); } @@ -284,6 +321,7 @@ static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base, u32 gsi_base) { struct pch_pic *priv; + int i; priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) @@ -294,6 +332,10 @@ static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base, if (!priv->base) goto free_priv; + priv->inuse = 0; + for (i = 0; i < PIC_COUNT; i++) + priv->table[i] = PIC_UNDEF_VECTOR; + priv->ht_vec_base = vec_base; priv->vec_count = ((readq(priv->base) >> 48) & 0xff) + 1; priv->gsi_base = gsi_base; -- 2.20.1
Hi Christophe
This part of the code is not a hotspot path. In fact, it may only be
executed once at startup,
so some optimizations that we consider extreme can be omitted
Tianyang
在 2024/3/16 下午10:03, Christophe JAILLET 写道:
> Le 16/03/2024 à 09:21, Tianyang Zhang a écrit :
>> From: Baoqi Zhang <zhangbaoqi@loongson.cn>
>>
>> This patch remove the fixed mapping between the 7A interrupt source
>> and the HT interrupt vector, and replaced it with a dynamically
>> allocated approach.
>>
>> We introduce a mapping table in struct pch_pic, where each interrupt
>> source will allocate an index as a 'hwirq' from the table in the order
>> of application and set table value as interrupt source number. This
>> hwirq
>> will be configured as its vector in the HT interrupt controller. For an
>> interrupt source, the validity period of the obtained hwirq will last
>> until
>> the system reset
>>
>> This will be more conducive to fully utilizing existing vectors to
>> support more devices
>>
>> Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn>
>> Signed-off-by: Biao Dong <dongbiao@loongson.cn>
>> Signed-off-by: Tianyang Zhang <zhangtianyang@loongson.cn>
>> ---
>> drivers/irqchip/irq-loongson-pch-pic.c | 77 ++++++++++++++++++++------
>> 1 file changed, 59 insertions(+), 18 deletions(-)
>>
>
> ...
>
>> @@ -171,6 +183,27 @@ static int pch_pic_domain_translate(struct
>> irq_domain *d,
>> return -EINVAL;
>> *hwirq = fwspec->param[0] - priv->gsi_base;
>> +
>> + raw_spin_lock_irqsave(&priv->pic_lock, flags);
>> + /* Check pic-table to confirm if the hwirq has been assigned */
>> + for (i = 0; i < priv->inuse; i++) {
>> + if (priv->table[i] == *hwirq) {
>> + *hwirq = i;
>> + break;
>> + }
>> + }
>> + if (i == priv->inuse) {
>> + /* Assign a new hwirq in pic-table */
>> + if (priv->inuse >= PIC_COUNT) {
>> + pr_err("pch-pic domain has no free vectors\n");
>> + raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
>> + return -EINVAL;
>> + }
>> + priv->table[priv->inuse] = *hwirq;
>> + *hwirq = priv->inuse++;
>> + }
>> + raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
>
> Hi,
>
> not sure if it helps or if this is a hot path, but, IIUC, taking the
> lock could be avoided with some code reordering and 'inuse' being an
> atomic_t.
>
> IIUC, the lock is only needed to protect 'inuse' and 'table' is never
> modified afterwards.
>
> Somehing like:
>
> > + int cur_inuse;
> ...
> > + cur_inuse = atomic_read(&priv->inuse);
> > + /* Check pic-table to confirm if the hwirq has been
> assigned */
> > + for (i = 0; i < cur_inuse; i++) {
> > + if (priv->table[i] == *hwirq) {
> > + *hwirq = i;
> > + break;
> > + }
> > + }
> > + if (i == cur_inuse) {
> > + /* Assign a new hwirq in pic-table */
> > + if (cur_inuse >= PIC_COUNT) {
> > + pr_err("pch-pic domain has no free vectors\n");
> > + return -EINVAL;
> > + }
> > + priv->table[cur_inuse] = *hwirq;
> > + *hwirq = atomic_inc_return(&priv->inuse);
> > + }
>
>
>> +
>> if (fwspec->param_count > 1)
>> *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
>> else
>> @@ -194,6 +227,9 @@ static int pch_pic_alloc(struct irq_domain
>> *domain, unsigned int virq,
>> if (err)
>> return err;
>> + /* Write vector ID */
>> + writeb(priv->ht_vec_base + hwirq, priv->base +
>> PCH_INT_HTVEC(hwirq_to_bit(priv, hwirq)));
>> +
>> parent_fwspec.fwnode = domain->parent->fwnode;
>> parent_fwspec.param_count = 1;
>> parent_fwspec.param[0] = hwirq + priv->ht_vec_base;
>
> ...
Hi, Huacai
Thanks for your suggestions and corrections. I will make the
modifications as required
and provide the next version of the patch as soon as possible
Tianyang
在 2024/3/18 下午10:35, Huacai Chen 写道:
> Hi, Tianyang,
>
> On Sat, Mar 16, 2024 at 4:22 PM Tianyang Zhang
> <zhangtianyang@loongson.cn> wrote:
>> From: Baoqi Zhang <zhangbaoqi@loongson.cn>
>>
>> This patch remove the fixed mapping between the 7A interrupt source
>> and the HT interrupt vector, and replaced it with a dynamically
>> allocated approach.
> Use LS7A instead of 7A here.
>
>> We introduce a mapping table in struct pch_pic, where each interrupt
>> source will allocate an index as a 'hwirq' from the table in the order
>> of application and set table value as interrupt source number. This hwirq
>> will be configured as its vector in the HT interrupt controller. For an
>> interrupt source, the validity period of the obtained hwirq will last until
>> the system reset
> Missing a . at the end.
>
>> This will be more conducive to fully utilizing existing vectors to
>> support more devices
>>
>> Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn>
>> Signed-off-by: Biao Dong <dongbiao@loongson.cn>
>> Signed-off-by: Tianyang Zhang <zhangtianyang@loongson.cn>
>> ---
>> drivers/irqchip/irq-loongson-pch-pic.c | 77 ++++++++++++++++++++------
>> 1 file changed, 59 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
>> index 63db8e2172e0..f17187641154 100644
>> --- a/drivers/irqchip/irq-loongson-pch-pic.c
>> +++ b/drivers/irqchip/irq-loongson-pch-pic.c
>> @@ -33,7 +33,7 @@
>> #define PIC_COUNT (PIC_COUNT_PER_REG * PIC_REG_COUNT)
>> #define PIC_REG_IDX(irq_id) ((irq_id) / PIC_COUNT_PER_REG)
>> #define PIC_REG_BIT(irq_id) ((irq_id) % PIC_COUNT_PER_REG)
>> -
>> +#define PIC_UNDEF_VECTOR 255
> Keep a new line after the macro, please.
>
> Huacai
>
>> static int nr_pics;
>>
>> struct pch_pic {
>> @@ -46,12 +46,19 @@ struct pch_pic {
>> u32 saved_vec_en[PIC_REG_COUNT];
>> u32 saved_vec_pol[PIC_REG_COUNT];
>> u32 saved_vec_edge[PIC_REG_COUNT];
>> + u8 table[PIC_COUNT];
>> + int inuse;
>> };
>>
>> static struct pch_pic *pch_pic_priv[MAX_IO_PICS];
>>
>> struct fwnode_handle *pch_pic_handle[MAX_IO_PICS];
>>
>> +static inline u8 hwirq_to_bit(struct pch_pic *priv, int hirq)
>> +{
>> + return priv->table[hirq];
>> +}
>> +
>> static void pch_pic_bitset(struct pch_pic *priv, int offset, int bit)
>> {
>> u32 reg;
>> @@ -80,45 +87,47 @@ static void pch_pic_mask_irq(struct irq_data *d)
>> {
>> struct pch_pic *priv = irq_data_get_irq_chip_data(d);
>>
>> - pch_pic_bitset(priv, PCH_PIC_MASK, d->hwirq);
>> + pch_pic_bitset(priv, PCH_PIC_MASK, hwirq_to_bit(priv, d->hwirq));
>> irq_chip_mask_parent(d);
>> }
>>
>> static void pch_pic_unmask_irq(struct irq_data *d)
>> {
>> struct pch_pic *priv = irq_data_get_irq_chip_data(d);
>> + int bit = hwirq_to_bit(priv, d->hwirq);
>>
>> - writel(BIT(PIC_REG_BIT(d->hwirq)),
>> - priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4);
>> + writel(BIT(PIC_REG_BIT(bit)),
>> + priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4);
>>
>> irq_chip_unmask_parent(d);
>> - pch_pic_bitclr(priv, PCH_PIC_MASK, d->hwirq);
>> + pch_pic_bitclr(priv, PCH_PIC_MASK, bit);
>> }
>>
>> static int pch_pic_set_type(struct irq_data *d, unsigned int type)
>> {
>> struct pch_pic *priv = irq_data_get_irq_chip_data(d);
>> + int bit = hwirq_to_bit(priv, d->hwirq);
>> int ret = 0;
>>
>> switch (type) {
>> case IRQ_TYPE_EDGE_RISING:
>> - pch_pic_bitset(priv, PCH_PIC_EDGE, d->hwirq);
>> - pch_pic_bitclr(priv, PCH_PIC_POL, d->hwirq);
>> + pch_pic_bitset(priv, PCH_PIC_EDGE, bit);
>> + pch_pic_bitclr(priv, PCH_PIC_POL, bit);
>> irq_set_handler_locked(d, handle_edge_irq);
>> break;
>> case IRQ_TYPE_EDGE_FALLING:
>> - pch_pic_bitset(priv, PCH_PIC_EDGE, d->hwirq);
>> - pch_pic_bitset(priv, PCH_PIC_POL, d->hwirq);
>> + pch_pic_bitset(priv, PCH_PIC_EDGE, bit);
>> + pch_pic_bitset(priv, PCH_PIC_POL, bit);
>> irq_set_handler_locked(d, handle_edge_irq);
>> break;
>> case IRQ_TYPE_LEVEL_HIGH:
>> - pch_pic_bitclr(priv, PCH_PIC_EDGE, d->hwirq);
>> - pch_pic_bitclr(priv, PCH_PIC_POL, d->hwirq);
>> + pch_pic_bitclr(priv, PCH_PIC_EDGE, bit);
>> + pch_pic_bitclr(priv, PCH_PIC_POL, bit);
>> irq_set_handler_locked(d, handle_level_irq);
>> break;
>> case IRQ_TYPE_LEVEL_LOW:
>> - pch_pic_bitclr(priv, PCH_PIC_EDGE, d->hwirq);
>> - pch_pic_bitset(priv, PCH_PIC_POL, d->hwirq);
>> + pch_pic_bitclr(priv, PCH_PIC_EDGE, bit);
>> + pch_pic_bitset(priv, PCH_PIC_POL, bit);
>> irq_set_handler_locked(d, handle_level_irq);
>> break;
>> default:
>> @@ -133,11 +142,12 @@ static void pch_pic_ack_irq(struct irq_data *d)
>> {
>> unsigned int reg;
>> struct pch_pic *priv = irq_data_get_irq_chip_data(d);
>> + int bit = hwirq_to_bit(priv, d->hwirq);
>>
>> - reg = readl(priv->base + PCH_PIC_EDGE + PIC_REG_IDX(d->hwirq) * 4);
>> - if (reg & BIT(PIC_REG_BIT(d->hwirq))) {
>> - writel(BIT(PIC_REG_BIT(d->hwirq)),
>> - priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4);
>> + reg = readl(priv->base + PCH_PIC_EDGE + PIC_REG_IDX(bit) * 4);
>> + if (reg & BIT(PIC_REG_BIT(bit))) {
>> + writel(BIT(PIC_REG_BIT(bit)),
>> + priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4);
>> }
>> irq_chip_ack_parent(d);
>> }
>> @@ -159,6 +169,8 @@ static int pch_pic_domain_translate(struct irq_domain *d,
>> {
>> struct pch_pic *priv = d->host_data;
>> struct device_node *of_node = to_of_node(fwspec->fwnode);
>> + unsigned long flags;
>> + int i;
>>
>> if (of_node) {
>> if (fwspec->param_count < 2)
>> @@ -171,6 +183,27 @@ static int pch_pic_domain_translate(struct irq_domain *d,
>> return -EINVAL;
>>
>> *hwirq = fwspec->param[0] - priv->gsi_base;
>> +
>> + raw_spin_lock_irqsave(&priv->pic_lock, flags);
>> + /* Check pic-table to confirm if the hwirq has been assigned */
>> + for (i = 0; i < priv->inuse; i++) {
>> + if (priv->table[i] == *hwirq) {
>> + *hwirq = i;
>> + break;
>> + }
>> + }
>> + if (i == priv->inuse) {
>> + /* Assign a new hwirq in pic-table */
>> + if (priv->inuse >= PIC_COUNT) {
>> + pr_err("pch-pic domain has no free vectors\n");
>> + raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
>> + return -EINVAL;
>> + }
>> + priv->table[priv->inuse] = *hwirq;
>> + *hwirq = priv->inuse++;
>> + }
>> + raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
>> +
>> if (fwspec->param_count > 1)
>> *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
>> else
>> @@ -194,6 +227,9 @@ static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq,
>> if (err)
>> return err;
>>
>> + /* Write vector ID */
>> + writeb(priv->ht_vec_base + hwirq, priv->base + PCH_INT_HTVEC(hwirq_to_bit(priv, hwirq)));
>> +
>> parent_fwspec.fwnode = domain->parent->fwnode;
>> parent_fwspec.param_count = 1;
>> parent_fwspec.param[0] = hwirq + priv->ht_vec_base;
>> @@ -222,7 +258,7 @@ static void pch_pic_reset(struct pch_pic *priv)
>>
>> for (i = 0; i < PIC_COUNT; i++) {
>> /* Write vector ID */
>> - writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(i));
>> + writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(hwirq_to_bit(priv, i)));
>> /* Hardcode route to HT0 Lo */
>> writeb(1, priv->base + PCH_INT_ROUTE(i));
>> }
>> @@ -284,6 +320,7 @@ static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base,
>> u32 gsi_base)
>> {
>> struct pch_pic *priv;
>> + int i;
>>
>> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> if (!priv)
>> @@ -294,6 +331,10 @@ static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base,
>> if (!priv->base)
>> goto free_priv;
>>
>> + priv->inuse = 0;
>> + for (i = 0; i < PIC_COUNT; i++)
>> + priv->table[i] = PIC_UNDEF_VECTOR;
>> +
>> priv->ht_vec_base = vec_base;
>> priv->vec_count = ((readq(priv->base) >> 48) & 0xff) + 1;
>> priv->gsi_base = gsi_base;
>> --
>> 2.20.1
>>
>>
On Mon, Mar 18, 2024 at 2:18 PM Keguang Zhang <keguang.zhang@gmail.com> wrote: > > On Sun, Mar 17, 2024 at 10:40 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Sat, Mar 16, 2024 at 07:33:53PM +0800, Keguang Zhang via B4 Relay wrote: > > > From: Keguang Zhang <keguang.zhang@gmail.com> > > > > > > Add devicetree binding document for Loongson-1 DMA. > > > > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > > > --- > > > V5 -> V6: > > > Change the compatible to the fallback > > > Some minor fixes > > > V4 -> V5: > > > A newly added patch > > > --- > > > .../devicetree/bindings/dma/loongson,ls1x-dma.yaml | 66 ++++++++++++++++++++++ > > > 1 file changed, 66 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/dma/loongson,ls1x-dma.yaml b/Documentation/devicetree/bindings/dma/loongson,ls1x-dma.yaml > > > new file mode 100644 > > > index 000000000000..06358df725c6 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/dma/loongson,ls1x-dma.yaml > > > @@ -0,0 +1,66 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/dma/loongson,ls1x-dma.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Loongson-1 DMA Controller > > > + > > > +maintainers: > > > + - Keguang Zhang <keguang.zhang@gmail.com> > > > + > > > +description: > > > + Loongson-1 DMA controller provides 3 independent channels for > > > + peripherals such as NAND and AC97. > > > + > > > +properties: > > > + compatible: > > > + oneOf: > > > + - const: loongson,ls1b-dma > > > + - items: > > > + - enum: > > > + - loongson,ls1c-dma > > > + - const: loongson,ls1b-dma > > > > Aren't there several more devices in this family? Do they not have DMA > > controllers? > > > You are right. Loongson1 is a SoC family. > However, only 1A/1B/1C have DMA controller. > > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + description: Each channel has a dedicated interrupt line. > > > + minItems: 1 > > > + maxItems: 3 > > > > Is this number not fixed for each SoC? > > > Yes. Actually, it's fixed for the whole family. > > > > + interrupt-names: > > > + minItems: 1 > > > + items: > > > + - pattern: ch0 > > > + - pattern: ch1 > > > + - pattern: ch2 > > > > Why have you made these a pattern? There's no regex being used here at > > all. > > > Will change items to the following regex. > interrupt-names: > minItems: 1 > items: > - pattern: "^ch[0-2]$" > Sorry. This pattern fails in dt_binding_check. Will use const instead of pattern. interrupt-names: items: - const: ch0 - const: ch1 - const: ch2 > Thanks! > > > Cheers, > > Cono4. > > > > -- > Best regards, > > Keguang Zhang -- Best regards, Keguang Zhang
On Tue, Mar 19, 2024 at 10:32 AM Keguang Zhang <keguang.zhang@gmail.com> wrote: > > On Mon, Mar 18, 2024 at 11:42 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Mon, Mar 18, 2024 at 10:26:51PM +0800, Huacai Chen wrote: > > > Hi, Conor, > > > > > > On Mon, Mar 18, 2024 at 7:28 PM Conor Dooley <conor@kernel.org> wrote: > > > > > > > > On Mon, Mar 18, 2024 at 03:31:59PM +0800, Huacai Chen wrote: > > > > > On Mon, Mar 18, 2024 at 10:08 AM Keguang Zhang <keguang.zhang@gmail.com> wrote: > > > > > > > > > > > > Hi Huacai, > > > > > > > > > > > > > Hi, Keguang, > > > > > > > > > > > > > > Sorry for the late reply, there is already a ls2x-apb-dma driver, I'm > > > > > > > not sure but can they share the same code base? If not, can rename > > > > > > > this driver to ls1x-apb-dma for consistency? > > > > > > > > > > > > There are some differences between ls1x DMA and ls2x DMA, such as > > > > > > registers and DMA descriptors. > > > > > > I will rename it to ls1x-apb-dma. > > > > > OK, please also rename the yaml file to keep consistency. > > > > > > > > No, the yaml file needs to match the (one of the) compatible strings. > > > OK, then I think we can also rename the compatible strings, if possible. > > > > If there are no other types of dma controller on this device, I do not > > see why would we add "apb" into the compatible as there is nothing to > > differentiate this controller from. > > That's true. 1A/1B/1C only have one APB DMA. > Should I keep the compatible "ls1b-dma" and "ls1c-dma"? The name "apbdma" comes from the user manual, "exchange data between memory and apb devices", at present there are two drivers using this naming: tegra20-apb-dma.c and ls2x-apb-dma.c. Huacai > > -- > Best regards, > > Keguang Zhang
On Mon, Mar 18, 2024 at 11:42 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Mon, Mar 18, 2024 at 10:26:51PM +0800, Huacai Chen wrote:
> > Hi, Conor,
> >
> > On Mon, Mar 18, 2024 at 7:28 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Mon, Mar 18, 2024 at 03:31:59PM +0800, Huacai Chen wrote:
> > > > On Mon, Mar 18, 2024 at 10:08 AM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > > > >
> > > > > Hi Huacai,
> > > > >
> > > > > > Hi, Keguang,
> > > > > >
> > > > > > Sorry for the late reply, there is already a ls2x-apb-dma driver, I'm
> > > > > > not sure but can they share the same code base? If not, can rename
> > > > > > this driver to ls1x-apb-dma for consistency?
> > > > >
> > > > > There are some differences between ls1x DMA and ls2x DMA, such as
> > > > > registers and DMA descriptors.
> > > > > I will rename it to ls1x-apb-dma.
> > > > OK, please also rename the yaml file to keep consistency.
> > >
> > > No, the yaml file needs to match the (one of the) compatible strings.
> > OK, then I think we can also rename the compatible strings, if possible.
>
> If there are no other types of dma controller on this device, I do not
> see why would we add "apb" into the compatible as there is nothing to
> differentiate this controller from.
That's true. 1A/1B/1C only have one APB DMA.
Should I keep the compatible "ls1b-dma" and "ls1c-dma"?
--
Best regards,
Keguang Zhang
On Mon, Mar 18, 2024 at 7:29 PM Conor Dooley <conor@kernel.org> wrote: > > On Mon, Mar 18, 2024 at 02:18:27PM +0800, Keguang Zhang wrote: > > On Sun, Mar 17, 2024 at 10:40 PM Conor Dooley <conor@kernel.org> wrote: > > > > > > On Sat, Mar 16, 2024 at 07:33:53PM +0800, Keguang Zhang via B4 Relay wrote: > > > > From: Keguang Zhang <keguang.zhang@gmail.com> > > > > > > > > Add devicetree binding document for Loongson-1 DMA. > > > > > > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > > > > --- > > > > V5 -> V6: > > > > Change the compatible to the fallback > > > > Some minor fixes > > > > V4 -> V5: > > > > A newly added patch > > > > --- > > > > .../devicetree/bindings/dma/loongson,ls1x-dma.yaml | 66 ++++++++++++++++++++++ > > > > 1 file changed, 66 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/dma/loongson,ls1x-dma.yaml b/Documentation/devicetree/bindings/dma/loongson,ls1x-dma.yaml > > > > new file mode 100644 > > > > index 000000000000..06358df725c6 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/dma/loongson,ls1x-dma.yaml > > > > @@ -0,0 +1,66 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/dma/loongson,ls1x-dma.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Loongson-1 DMA Controller > > > > + > > > > +maintainers: > > > > + - Keguang Zhang <keguang.zhang@gmail.com> > > > > + > > > > +description: > > > > + Loongson-1 DMA controller provides 3 independent channels for > > > > + peripherals such as NAND and AC97. > > > > + > > > > +properties: > > > > + compatible: > > > > + oneOf: > > > > + - const: loongson,ls1b-dma > > > > + - items: > > > > + - enum: > > > > + - loongson,ls1c-dma > > > > + - const: loongson,ls1b-dma > > > > > > Aren't there several more devices in this family? Do they not have DMA > > > controllers? > > > > > You are right. Loongson1 is a SoC family. > > However, only 1A/1B/1C have DMA controller. > > You're missing the 1A then. > Will add 1A. > > > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + interrupts: > > > > + description: Each channel has a dedicated interrupt line. > > > > + minItems: 1 > > > > + maxItems: 3 > > > > > > Is this number not fixed for each SoC? > > > > > Yes. Actually, it's fixed for the whole family. > > Then why do you have minItems: 1? Are there multiple DMA controllers > on each SoC that only make use of a subset of the possible channels? > All channels are available on each SoC. Sorry, I will remove the minItems. Thanks for your review! > > > > + interrupt-names: > > > > + minItems: 1 > > > > + items: > > > > + - pattern: ch0 > > > > + - pattern: ch1 > > > > + - pattern: ch2 > > > > > > Why have you made these a pattern? There's no regex being used here at > > > all. > > > > > Will change items to the following regex. > > interrupt-names: > > minItems: 1 > > items: > > - pattern: "^ch[0-2]$" > -- Best regards, Keguang Zhang
On Sun, Mar 17, 2024 at 07:49:43PM -0700, David Wei wrote: > I'm working on a similar proposal for zero copy Rx but to host memory > and depend on this memory provider API. How do you need a different provider for that vs just udmabuf? > Jakub also designed this API for hugepages too IIRC. Basically there's > going to be at least three fancy ways of providing pages (one of which > isn't actually pages, hence the merged netmem_t series) to drivers. How do hugepages different from a normal page allocation? They should just a different ordered passed to the page allocator.
Hi Hucai,
On Mon, 2024-03-18 at 22:21 +0800, Huacai Chen wrote:
> Hi, SuperH maintainers,
>
> On Wed, Feb 8, 2023 at 8:59 PM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
> >
> > On Thu, 2022-07-14 at 16:41 +0800, Huacai Chen wrote:
> > > When CONFIG_CPUMASK_OFFSTACK and CONFIG_DEBUG_PER_CPU_MAPS is selected,
> > > cpu_max_bits_warn() generates a runtime warning similar as below while
> > > we show /proc/cpuinfo. Fix this by using nr_cpu_ids (the runtime limit)
> > > instead of NR_CPUS to iterate CPUs.
> > >
> > > [ 3.052463] ------------[ cut here ]------------
> > > [ 3.059679] WARNING: CPU: 3 PID: 1 at include/linux/cpumask.h:108 show_cpuinfo+0x5e8/0x5f0
> > > [ 3.070072] Modules linked in: efivarfs autofs4
> > > [ 3.076257] CPU: 0 PID: 1 Comm: systemd Not tainted 5.19-rc5+ #1052
> > > [ 3.099465] Stack : 9000000100157b08 9000000000f18530 9000000000cf846c 9000000100154000
> > > [ 3.109127] 9000000100157a50 0000000000000000 9000000100157a58 9000000000ef7430
> > > [ 3.118774] 90000001001578e8 0000000000000040 0000000000000020 ffffffffffffffff
> > > [ 3.128412] 0000000000aaaaaa 1ab25f00eec96a37 900000010021de80 900000000101c890
> > > [ 3.138056] 0000000000000000 0000000000000000 0000000000000000 0000000000aaaaaa
> > > [ 3.147711] ffff8000339dc220 0000000000000001 0000000006ab4000 0000000000000000
> > > [ 3.157364] 900000000101c998 0000000000000004 9000000000ef7430 0000000000000000
> > > [ 3.167012] 0000000000000009 000000000000006c 0000000000000000 0000000000000000
> > > [ 3.176641] 9000000000d3de08 9000000001639390 90000000002086d8 00007ffff0080286
> > > [ 3.186260] 00000000000000b0 0000000000000004 0000000000000000 0000000000071c1c
> > > [ 3.195868] ...
> > > [ 3.199917] Call Trace:
> > > [ 3.203941] [<90000000002086d8>] show_stack+0x38/0x14c
> > > [ 3.210666] [<9000000000cf846c>] dump_stack_lvl+0x60/0x88
> > > [ 3.217625] [<900000000023d268>] __warn+0xd0/0x100
> > > [ 3.223958] [<9000000000cf3c90>] warn_slowpath_fmt+0x7c/0xcc
> > > [ 3.231150] [<9000000000210220>] show_cpuinfo+0x5e8/0x5f0
> > > [ 3.238080] [<90000000004f578c>] seq_read_iter+0x354/0x4b4
> > > [ 3.245098] [<90000000004c2e90>] new_sync_read+0x17c/0x1c4
> > > [ 3.252114] [<90000000004c5174>] vfs_read+0x138/0x1d0
> > > [ 3.258694] [<90000000004c55f8>] ksys_read+0x70/0x100
> > > [ 3.265265] [<9000000000cfde9c>] do_syscall+0x7c/0x94
> > > [ 3.271820] [<9000000000202fe4>] handle_syscall+0xc4/0x160
> > > [ 3.281824] ---[ end trace 8b484262b4b8c24c ]---
> > >
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > > ---
> > > arch/sh/kernel/cpu/proc.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/sh/kernel/cpu/proc.c b/arch/sh/kernel/cpu/proc.c
> > > index a306bcd6b341..5f6d0e827bae 100644
> > > --- a/arch/sh/kernel/cpu/proc.c
> > > +++ b/arch/sh/kernel/cpu/proc.c
> > > @@ -132,7 +132,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
> > >
> > > static void *c_start(struct seq_file *m, loff_t *pos)
> > > {
> > > - return *pos < NR_CPUS ? cpu_data + *pos : NULL;
> > > + return *pos < nr_cpu_ids ? cpu_data + *pos : NULL;
> > > }
> > > static void *c_next(struct seq_file *m, void *v, loff_t *pos)
> > > {
> >
> > I build-tested the patch and also booted the patched kernel successfully
> > on my SH-7785LCR board.
> >
> > Showing the contents of /proc/cpuinfo works fine, too:
> >
> > root@tirpitz:~> cat /proc/cpuinfo
> > machine : SH7785LCR
> > processor : 0
> > cpu family : sh4a
> > cpu type : SH7785
> > cut : 7.x
> > cpu flags : fpu perfctr llsc
> > cache type : split (harvard)
> > icache size : 32KiB (4-way)
> > dcache size : 32KiB (4-way)
> > address sizes : 32 bits physical
> > bogomips : 599.99
> > root@tirpitz:~>
> >
> > Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> >
> > I am not sure yet whether the change is also correct as I don't know whether
> > it's possible to change the number of CPUs at runtime on SuperH.
> Can this patch be merged? This is the only one still unmerged in the
> whole series.
Thanks for the reminder. I will pick it up for 6.10.
Got sick this week, so I can't pick up anymore patches for 6.9 and will just
send Linus a PR later this week.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
[-- Attachment #1: Type: text/plain, Size: 1174 bytes --] On Mon, Mar 18, 2024 at 10:26:51PM +0800, Huacai Chen wrote: > Hi, Conor, > > On Mon, Mar 18, 2024 at 7:28 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Mon, Mar 18, 2024 at 03:31:59PM +0800, Huacai Chen wrote: > > > On Mon, Mar 18, 2024 at 10:08 AM Keguang Zhang <keguang.zhang@gmail.com> wrote: > > > > > > > > Hi Huacai, > > > > > > > > > Hi, Keguang, > > > > > > > > > > Sorry for the late reply, there is already a ls2x-apb-dma driver, I'm > > > > > not sure but can they share the same code base? If not, can rename > > > > > this driver to ls1x-apb-dma for consistency? > > > > > > > > There are some differences between ls1x DMA and ls2x DMA, such as > > > > registers and DMA descriptors. > > > > I will rename it to ls1x-apb-dma. > > > OK, please also rename the yaml file to keep consistency. > > > > No, the yaml file needs to match the (one of the) compatible strings. > OK, then I think we can also rename the compatible strings, if possible. If there are no other types of dma controller on this device, I do not see why would we add "apb" into the compatible as there is nothing to differentiate this controller from. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --]
Hi, Tianyang, On Sat, Mar 16, 2024 at 4:22 PM Tianyang Zhang <zhangtianyang@loongson.cn> wrote: > > From: Baoqi Zhang <zhangbaoqi@loongson.cn> > > This patch remove the fixed mapping between the 7A interrupt source > and the HT interrupt vector, and replaced it with a dynamically > allocated approach. Use LS7A instead of 7A here. > > We introduce a mapping table in struct pch_pic, where each interrupt > source will allocate an index as a 'hwirq' from the table in the order > of application and set table value as interrupt source number. This hwirq > will be configured as its vector in the HT interrupt controller. For an > interrupt source, the validity period of the obtained hwirq will last until > the system reset Missing a . at the end. > > This will be more conducive to fully utilizing existing vectors to > support more devices > > Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn> > Signed-off-by: Biao Dong <dongbiao@loongson.cn> > Signed-off-by: Tianyang Zhang <zhangtianyang@loongson.cn> > --- > drivers/irqchip/irq-loongson-pch-pic.c | 77 ++++++++++++++++++++------ > 1 file changed, 59 insertions(+), 18 deletions(-) > > diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c > index 63db8e2172e0..f17187641154 100644 > --- a/drivers/irqchip/irq-loongson-pch-pic.c > +++ b/drivers/irqchip/irq-loongson-pch-pic.c > @@ -33,7 +33,7 @@ > #define PIC_COUNT (PIC_COUNT_PER_REG * PIC_REG_COUNT) > #define PIC_REG_IDX(irq_id) ((irq_id) / PIC_COUNT_PER_REG) > #define PIC_REG_BIT(irq_id) ((irq_id) % PIC_COUNT_PER_REG) > - > +#define PIC_UNDEF_VECTOR 255 Keep a new line after the macro, please. Huacai > static int nr_pics; > > struct pch_pic { > @@ -46,12 +46,19 @@ struct pch_pic { > u32 saved_vec_en[PIC_REG_COUNT]; > u32 saved_vec_pol[PIC_REG_COUNT]; > u32 saved_vec_edge[PIC_REG_COUNT]; > + u8 table[PIC_COUNT]; > + int inuse; > }; > > static struct pch_pic *pch_pic_priv[MAX_IO_PICS]; > > struct fwnode_handle *pch_pic_handle[MAX_IO_PICS]; > > +static inline u8 hwirq_to_bit(struct pch_pic *priv, int hirq) > +{ > + return priv->table[hirq]; > +} > + > static void pch_pic_bitset(struct pch_pic *priv, int offset, int bit) > { > u32 reg; > @@ -80,45 +87,47 @@ static void pch_pic_mask_irq(struct irq_data *d) > { > struct pch_pic *priv = irq_data_get_irq_chip_data(d); > > - pch_pic_bitset(priv, PCH_PIC_MASK, d->hwirq); > + pch_pic_bitset(priv, PCH_PIC_MASK, hwirq_to_bit(priv, d->hwirq)); > irq_chip_mask_parent(d); > } > > static void pch_pic_unmask_irq(struct irq_data *d) > { > struct pch_pic *priv = irq_data_get_irq_chip_data(d); > + int bit = hwirq_to_bit(priv, d->hwirq); > > - writel(BIT(PIC_REG_BIT(d->hwirq)), > - priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4); > + writel(BIT(PIC_REG_BIT(bit)), > + priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4); > > irq_chip_unmask_parent(d); > - pch_pic_bitclr(priv, PCH_PIC_MASK, d->hwirq); > + pch_pic_bitclr(priv, PCH_PIC_MASK, bit); > } > > static int pch_pic_set_type(struct irq_data *d, unsigned int type) > { > struct pch_pic *priv = irq_data_get_irq_chip_data(d); > + int bit = hwirq_to_bit(priv, d->hwirq); > int ret = 0; > > switch (type) { > case IRQ_TYPE_EDGE_RISING: > - pch_pic_bitset(priv, PCH_PIC_EDGE, d->hwirq); > - pch_pic_bitclr(priv, PCH_PIC_POL, d->hwirq); > + pch_pic_bitset(priv, PCH_PIC_EDGE, bit); > + pch_pic_bitclr(priv, PCH_PIC_POL, bit); > irq_set_handler_locked(d, handle_edge_irq); > break; > case IRQ_TYPE_EDGE_FALLING: > - pch_pic_bitset(priv, PCH_PIC_EDGE, d->hwirq); > - pch_pic_bitset(priv, PCH_PIC_POL, d->hwirq); > + pch_pic_bitset(priv, PCH_PIC_EDGE, bit); > + pch_pic_bitset(priv, PCH_PIC_POL, bit); > irq_set_handler_locked(d, handle_edge_irq); > break; > case IRQ_TYPE_LEVEL_HIGH: > - pch_pic_bitclr(priv, PCH_PIC_EDGE, d->hwirq); > - pch_pic_bitclr(priv, PCH_PIC_POL, d->hwirq); > + pch_pic_bitclr(priv, PCH_PIC_EDGE, bit); > + pch_pic_bitclr(priv, PCH_PIC_POL, bit); > irq_set_handler_locked(d, handle_level_irq); > break; > case IRQ_TYPE_LEVEL_LOW: > - pch_pic_bitclr(priv, PCH_PIC_EDGE, d->hwirq); > - pch_pic_bitset(priv, PCH_PIC_POL, d->hwirq); > + pch_pic_bitclr(priv, PCH_PIC_EDGE, bit); > + pch_pic_bitset(priv, PCH_PIC_POL, bit); > irq_set_handler_locked(d, handle_level_irq); > break; > default: > @@ -133,11 +142,12 @@ static void pch_pic_ack_irq(struct irq_data *d) > { > unsigned int reg; > struct pch_pic *priv = irq_data_get_irq_chip_data(d); > + int bit = hwirq_to_bit(priv, d->hwirq); > > - reg = readl(priv->base + PCH_PIC_EDGE + PIC_REG_IDX(d->hwirq) * 4); > - if (reg & BIT(PIC_REG_BIT(d->hwirq))) { > - writel(BIT(PIC_REG_BIT(d->hwirq)), > - priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4); > + reg = readl(priv->base + PCH_PIC_EDGE + PIC_REG_IDX(bit) * 4); > + if (reg & BIT(PIC_REG_BIT(bit))) { > + writel(BIT(PIC_REG_BIT(bit)), > + priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4); > } > irq_chip_ack_parent(d); > } > @@ -159,6 +169,8 @@ static int pch_pic_domain_translate(struct irq_domain *d, > { > struct pch_pic *priv = d->host_data; > struct device_node *of_node = to_of_node(fwspec->fwnode); > + unsigned long flags; > + int i; > > if (of_node) { > if (fwspec->param_count < 2) > @@ -171,6 +183,27 @@ static int pch_pic_domain_translate(struct irq_domain *d, > return -EINVAL; > > *hwirq = fwspec->param[0] - priv->gsi_base; > + > + raw_spin_lock_irqsave(&priv->pic_lock, flags); > + /* Check pic-table to confirm if the hwirq has been assigned */ > + for (i = 0; i < priv->inuse; i++) { > + if (priv->table[i] == *hwirq) { > + *hwirq = i; > + break; > + } > + } > + if (i == priv->inuse) { > + /* Assign a new hwirq in pic-table */ > + if (priv->inuse >= PIC_COUNT) { > + pr_err("pch-pic domain has no free vectors\n"); > + raw_spin_unlock_irqrestore(&priv->pic_lock, flags); > + return -EINVAL; > + } > + priv->table[priv->inuse] = *hwirq; > + *hwirq = priv->inuse++; > + } > + raw_spin_unlock_irqrestore(&priv->pic_lock, flags); > + > if (fwspec->param_count > 1) > *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; > else > @@ -194,6 +227,9 @@ static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq, > if (err) > return err; > > + /* Write vector ID */ > + writeb(priv->ht_vec_base + hwirq, priv->base + PCH_INT_HTVEC(hwirq_to_bit(priv, hwirq))); > + > parent_fwspec.fwnode = domain->parent->fwnode; > parent_fwspec.param_count = 1; > parent_fwspec.param[0] = hwirq + priv->ht_vec_base; > @@ -222,7 +258,7 @@ static void pch_pic_reset(struct pch_pic *priv) > > for (i = 0; i < PIC_COUNT; i++) { > /* Write vector ID */ > - writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(i)); > + writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(hwirq_to_bit(priv, i))); > /* Hardcode route to HT0 Lo */ > writeb(1, priv->base + PCH_INT_ROUTE(i)); > } > @@ -284,6 +320,7 @@ static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base, > u32 gsi_base) > { > struct pch_pic *priv; > + int i; > > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > if (!priv) > @@ -294,6 +331,10 @@ static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base, > if (!priv->base) > goto free_priv; > > + priv->inuse = 0; > + for (i = 0; i < PIC_COUNT; i++) > + priv->table[i] = PIC_UNDEF_VECTOR; > + > priv->ht_vec_base = vec_base; > priv->vec_count = ((readq(priv->base) >> 48) & 0xff) + 1; > priv->gsi_base = gsi_base; > -- > 2.20.1 > >
Hi, Conor,
On Mon, Mar 18, 2024 at 7:28 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Mon, Mar 18, 2024 at 03:31:59PM +0800, Huacai Chen wrote:
> > On Mon, Mar 18, 2024 at 10:08 AM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > >
> > > Hi Huacai,
> > >
> > > > Hi, Keguang,
> > > >
> > > > Sorry for the late reply, there is already a ls2x-apb-dma driver, I'm
> > > > not sure but can they share the same code base? If not, can rename
> > > > this driver to ls1x-apb-dma for consistency?
> > >
> > > There are some differences between ls1x DMA and ls2x DMA, such as
> > > registers and DMA descriptors.
> > > I will rename it to ls1x-apb-dma.
> > OK, please also rename the yaml file to keep consistency.
>
> No, the yaml file needs to match the (one of the) compatible strings.
OK, then I think we can also rename the compatible strings, if possible.
Huacai
Hi, SuperH maintainers, On Wed, Feb 8, 2023 at 8:59 PM John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote: > > On Thu, 2022-07-14 at 16:41 +0800, Huacai Chen wrote: > > When CONFIG_CPUMASK_OFFSTACK and CONFIG_DEBUG_PER_CPU_MAPS is selected, > > cpu_max_bits_warn() generates a runtime warning similar as below while > > we show /proc/cpuinfo. Fix this by using nr_cpu_ids (the runtime limit) > > instead of NR_CPUS to iterate CPUs. > > > > [ 3.052463] ------------[ cut here ]------------ > > [ 3.059679] WARNING: CPU: 3 PID: 1 at include/linux/cpumask.h:108 show_cpuinfo+0x5e8/0x5f0 > > [ 3.070072] Modules linked in: efivarfs autofs4 > > [ 3.076257] CPU: 0 PID: 1 Comm: systemd Not tainted 5.19-rc5+ #1052 > > [ 3.099465] Stack : 9000000100157b08 9000000000f18530 9000000000cf846c 9000000100154000 > > [ 3.109127] 9000000100157a50 0000000000000000 9000000100157a58 9000000000ef7430 > > [ 3.118774] 90000001001578e8 0000000000000040 0000000000000020 ffffffffffffffff > > [ 3.128412] 0000000000aaaaaa 1ab25f00eec96a37 900000010021de80 900000000101c890 > > [ 3.138056] 0000000000000000 0000000000000000 0000000000000000 0000000000aaaaaa > > [ 3.147711] ffff8000339dc220 0000000000000001 0000000006ab4000 0000000000000000 > > [ 3.157364] 900000000101c998 0000000000000004 9000000000ef7430 0000000000000000 > > [ 3.167012] 0000000000000009 000000000000006c 0000000000000000 0000000000000000 > > [ 3.176641] 9000000000d3de08 9000000001639390 90000000002086d8 00007ffff0080286 > > [ 3.186260] 00000000000000b0 0000000000000004 0000000000000000 0000000000071c1c > > [ 3.195868] ... > > [ 3.199917] Call Trace: > > [ 3.203941] [<90000000002086d8>] show_stack+0x38/0x14c > > [ 3.210666] [<9000000000cf846c>] dump_stack_lvl+0x60/0x88 > > [ 3.217625] [<900000000023d268>] __warn+0xd0/0x100 > > [ 3.223958] [<9000000000cf3c90>] warn_slowpath_fmt+0x7c/0xcc > > [ 3.231150] [<9000000000210220>] show_cpuinfo+0x5e8/0x5f0 > > [ 3.238080] [<90000000004f578c>] seq_read_iter+0x354/0x4b4 > > [ 3.245098] [<90000000004c2e90>] new_sync_read+0x17c/0x1c4 > > [ 3.252114] [<90000000004c5174>] vfs_read+0x138/0x1d0 > > [ 3.258694] [<90000000004c55f8>] ksys_read+0x70/0x100 > > [ 3.265265] [<9000000000cfde9c>] do_syscall+0x7c/0x94 > > [ 3.271820] [<9000000000202fe4>] handle_syscall+0xc4/0x160 > > [ 3.281824] ---[ end trace 8b484262b4b8c24c ]--- > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> > > --- > > arch/sh/kernel/cpu/proc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/sh/kernel/cpu/proc.c b/arch/sh/kernel/cpu/proc.c > > index a306bcd6b341..5f6d0e827bae 100644 > > --- a/arch/sh/kernel/cpu/proc.c > > +++ b/arch/sh/kernel/cpu/proc.c > > @@ -132,7 +132,7 @@ static int show_cpuinfo(struct seq_file *m, void *v) > > > > static void *c_start(struct seq_file *m, loff_t *pos) > > { > > - return *pos < NR_CPUS ? cpu_data + *pos : NULL; > > + return *pos < nr_cpu_ids ? cpu_data + *pos : NULL; > > } > > static void *c_next(struct seq_file *m, void *v, loff_t *pos) > > { > > I build-tested the patch and also booted the patched kernel successfully > on my SH-7785LCR board. > > Showing the contents of /proc/cpuinfo works fine, too: > > root@tirpitz:~> cat /proc/cpuinfo > machine : SH7785LCR > processor : 0 > cpu family : sh4a > cpu type : SH7785 > cut : 7.x > cpu flags : fpu perfctr llsc > cache type : split (harvard) > icache size : 32KiB (4-way) > dcache size : 32KiB (4-way) > address sizes : 32 bits physical > bogomips : 599.99 > root@tirpitz:~> > > Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> > > I am not sure yet whether the change is also correct as I don't know whether > it's possible to change the number of CPUs at runtime on SuperH. Can this patch be merged? This is the only one still unmerged in the whole series. Huacai > > Adrian > > -- > .''`. John Paul Adrian Glaubitz > : :' : Debian Developer > `. `' Physicist > `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
[-- Attachment #1: Type: text/plain, Size: 2864 bytes --] On Mon, Mar 18, 2024 at 02:18:27PM +0800, Keguang Zhang wrote: > On Sun, Mar 17, 2024 at 10:40 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Sat, Mar 16, 2024 at 07:33:53PM +0800, Keguang Zhang via B4 Relay wrote: > > > From: Keguang Zhang <keguang.zhang@gmail.com> > > > > > > Add devicetree binding document for Loongson-1 DMA. > > > > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > > > --- > > > V5 -> V6: > > > Change the compatible to the fallback > > > Some minor fixes > > > V4 -> V5: > > > A newly added patch > > > --- > > > .../devicetree/bindings/dma/loongson,ls1x-dma.yaml | 66 ++++++++++++++++++++++ > > > 1 file changed, 66 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/dma/loongson,ls1x-dma.yaml b/Documentation/devicetree/bindings/dma/loongson,ls1x-dma.yaml > > > new file mode 100644 > > > index 000000000000..06358df725c6 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/dma/loongson,ls1x-dma.yaml > > > @@ -0,0 +1,66 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/dma/loongson,ls1x-dma.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Loongson-1 DMA Controller > > > + > > > +maintainers: > > > + - Keguang Zhang <keguang.zhang@gmail.com> > > > + > > > +description: > > > + Loongson-1 DMA controller provides 3 independent channels for > > > + peripherals such as NAND and AC97. > > > + > > > +properties: > > > + compatible: > > > + oneOf: > > > + - const: loongson,ls1b-dma > > > + - items: > > > + - enum: > > > + - loongson,ls1c-dma > > > + - const: loongson,ls1b-dma > > > > Aren't there several more devices in this family? Do they not have DMA > > controllers? > > > You are right. Loongson1 is a SoC family. > However, only 1A/1B/1C have DMA controller. You're missing the 1A then. > > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + description: Each channel has a dedicated interrupt line. > > > + minItems: 1 > > > + maxItems: 3 > > > > Is this number not fixed for each SoC? > > > Yes. Actually, it's fixed for the whole family. Then why do you have minItems: 1? Are there multiple DMA controllers on each SoC that only make use of a subset of the possible channels? > > > + interrupt-names: > > > + minItems: 1 > > > + items: > > > + - pattern: ch0 > > > + - pattern: ch1 > > > + - pattern: ch2 > > > > Why have you made these a pattern? There's no regex being used here at > > all. > > > Will change items to the following regex. > interrupt-names: > minItems: 1 > items: > - pattern: "^ch[0-2]$" [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #1: Type: text/plain, Size: 688 bytes --] On Mon, Mar 18, 2024 at 03:31:59PM +0800, Huacai Chen wrote: > On Mon, Mar 18, 2024 at 10:08 AM Keguang Zhang <keguang.zhang@gmail.com> wrote: > > > > Hi Huacai, > > > > > Hi, Keguang, > > > > > > Sorry for the late reply, there is already a ls2x-apb-dma driver, I'm > > > not sure but can they share the same code base? If not, can rename > > > this driver to ls1x-apb-dma for consistency? > > > > There are some differences between ls1x DMA and ls2x DMA, such as > > registers and DMA descriptors. > > I will rename it to ls1x-apb-dma. > OK, please also rename the yaml file to keep consistency. No, the yaml file needs to match the (one of the) compatible strings. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --]
On Sat, Mar 16 2024 at 15:03, Christophe JAILLET wrote:
> Le 16/03/2024 à 09:21, Tianyang Zhang a écrit :
>> @@ -171,6 +183,27 @@ static int pch_pic_domain_translate(struct irq_domain *d,
>> return -EINVAL;
>>
>> *hwirq = fwspec->param[0] - priv->gsi_base;
>> +
>> + raw_spin_lock_irqsave(&priv->pic_lock, flags);
>> + /* Check pic-table to confirm if the hwirq has been assigned */
>> + for (i = 0; i < priv->inuse; i++) {
>> + if (priv->table[i] == *hwirq) {
>> + *hwirq = i;
>> + break;
>> + }
>> + }
>> + if (i == priv->inuse) {
>> + /* Assign a new hwirq in pic-table */
>> + if (priv->inuse >= PIC_COUNT) {
>> + pr_err("pch-pic domain has no free vectors\n");
>> + raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
>> + return -EINVAL;
>> + }
>> + priv->table[priv->inuse] = *hwirq;
>> + *hwirq = priv->inuse++;
>> + }
>> + raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
>
> Hi,
>
> not sure if it helps or if this is a hot path, but, IIUC, taking the
> lock could be avoided with some code reordering and 'inuse' being an
> atomic_t.
It's the translate and setup path, so nothing to optimize here.
On 2024-03-18 11:23, Krzysztof Kozlowski wrote: > On 17/03/2024 16:43, Justin Swartz wrote: >> On 2024-03-17 17:29, Krzysztof Kozlowski wrote: >>> Objections to what? Coding style? Coding style is defined so you >>> either >>> implement it or not... and even if someone disagrees with one line >>> swap, >>> why it cannot be done like for every contribution: inline? >> >> I had been asked to include empty lines when I had left them out when >> I had contributed a patch regarding the serial nodes, which resulted >> in >> a second version of that patch. > > I don't understand why would that matter. It's expected Linux > development process to receive comments inline in the patch. >>> Organize your patches how described in submitting patches: one per >>> logical change. Logical change is to reorder all properties in one >>> file, >>> without functional impact. >> >> If I had accidentally deleted or modified an attribute in the process >> of cleanup, this could have had a functional impact. It's easier to > > How is it relevant? But you did not and splitting simple cleanup > one-line-per-patch is not affecting this. Just because you could make > mistake it does not affect patch readability at all. > > Nothing improved with your patch split. >> notice this sort of omission when the wall of text you're confronted >> with is as small as possible, and not multiple pages long. > > We are used to handle some length of patches. Multiple scrolls for > obvious cleanups are not problems. Why aren't you applying this > approach > to everything? Add a new driver with one function per patch and then > finally Makefile? It would be bisectable and "easy to read" plus > absolutely unmanageable. >> But for future reference: is it not enough for the Reviewed-by: >> trailer >> to be sent in response to the cover letter of a patch set if a >> reviewer >> has looked at the entire set? > > Sure, one can. I still need to open and download 14 patches. Thanks for your input. I can imagine how these sets of very minor changes might greatly reduce your signal-to-noise ratio as an upstream maintainer. I'll try your suggested approach next time.
On 2024-03-18 10:48, Sergio Paracuellos wrote:
> On Sun, Mar 17, 2024 at 4:43 PM Justin Swartz
> <justin.swartz@risingedge.co.za> wrote:
>> But for future reference: is it not enough for the Reviewed-by:
>> trailer
>> to be sent in response to the cover letter of a patch set if a
>> reviewer
>> has looked at the entire set?
>
> It is enough, AFAICT. I found your patchset very easy to review so I
> am ok with the patchset as it is. However, at the end this will be
> through the mips tree, so let's do what Thomas prefers: add all
> patches as they are or squash all of them in one commit.
>
> Thanks,
> Sergio Paracuellos
Thank you for the review. I'm glad to squash them if need be.
Il 16/03/24 05:54, Justin Swartz ha scritto:
> Shuffle the attributes of the gpio node to appease the DTS
> style guide.
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 16/03/24 05:54, Justin Swartz ha scritto:
> Reorder the CPU Interrupt Controller node's attributes to follow
> what the DTS Coding Style dictates.
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 16/03/24 05:54, Justin Swartz ha scritto:
> Move the pinctrl node prior to the nodes that feature unit
> addresses.
>
> Sort pinctrl's child nodes into alphabetical order.
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 16/03/24 05:54, Justin Swartz ha scritto:
> Reorder cpu node attributes to fit the DTS Coding Style.
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 16/03/24 05:54, Justin Swartz ha scritto:
> Reorder the attributes of MMC fixed voltage regulator nodes
> for the sake of compliance with the DTS style guide.
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 16/03/24 05:54, Justin Swartz ha scritto:
> Reorder the attributes of the SPI controller node so that
> they're aligned with the DTS style guide.
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 16/03/24 05:54, Justin Swartz ha scritto:
> Rearrange the order of the i2c node's attributes so that they
> are inline with the DTS style guide.
>
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>